feat: add SQLite storage layer for pools, vms, leases, events - #26
Conversation
Implements the repository interface described in the design doc's Storage Schema section: internal/store.Store, backed by SQLite via the pure-Go modernc.org/sqlite driver so cross-platform GoReleaser builds don't need cgo. Reuses the existing poolmgr proto types (PoolSpec, VMRecord, LeaseRecord, Event) as the store's domain types rather than duplicating them. ClaimAvailableVM performs the pick-and-mark-leased transition inside one transaction so concurrent claims never race onto the same VM. Closes #5 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQHrhoyXpSBFYnVTrhxjvh
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness hazards (nil-timestamp panics in conversions and an API/store namespace mismatch) that can cause runtime failures or ambiguous behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a new internal/store package that implements a SQLite-backed persistence layer for the pool manager’s core entities (pools, VMs, leases) plus an events outbox, using the pure-Go modernc.org/sqlite driver to support cross-platform releases.
Changes:
- Added
internal/store.Storerepository interface and SQLite implementation (Open, CRUD for pools/VMs/leases, events outbox APIs). - Implemented schema + conversion helpers for storing proto-domain types in SQLite.
- Added a comprehensive unit test suite for the SQLite store and updated module dependencies for the SQLite driver.
File summaries
| File | Description |
|---|---|
| internal/store/store.go | Defines the repository interface and shared store errors. |
| internal/store/sqlite.go | Implements the Store interface on top of SQLite (Open, CRUD, claim, outbox). |
| internal/store/convert.go | Converts between proto domain objects and flat SQLite row representations. |
| internal/store/schema.sql | Defines tables and indexes for pools, vms, leases, and events. |
| internal/store/sqlite_test.go | Adds unit tests covering store CRUD, claiming, lease expiry, and events replay behavior. |
| go.mod | Adds modernc.org/sqlite and related indirect dependencies. |
| go.sum | Records checksums for the new direct/indirect module dependencies. |
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…pace Addresses PR #26 review feedback: - ClaimAvailableVM's UPDATE now guards on "AND phase = AVAILABLE" and checks RowsAffected, returning ErrNoAvailableVM on a lost race instead of relying on the single-connection pool setting to make the SELECT-then-UPDATE safe. - vmToRow/leaseToRow/AppendEvent now reject nil required timestamps via a requireTimestamp helper instead of silently persisting the Unix epoch (verified (*timestamppb.Timestamp)(nil).AsTime() doesn't panic, it just returns 1970-01-01 - worse than a panic since it fails silently). - Added pool_namespace to VMRecord, LeaseRecord, and Event (regenerated via hack/generate-proto.sh) so pool-scoped store methods (ClaimAvailableVM, ListVMsByPool, ListEventsSince) key off (pool_name, pool_namespace) instead of a bare name, matching PoolSpec/PoolRef's existing identity model. Updated the design doc's Data Model section to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQHrhoyXpSBFYnVTrhxjvh
There was a problem hiding this comment.
🟡 Changes recommended
Pool heartbeat duration fields can be silently persisted as 0ns when unset, instead of returning a validation error, which can cause incorrect lease expiry behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- api/proto/poolmgr/v1alpha1/types.pb.go: Generated file
- Files reviewed: 8/10 changed files
- Comments generated: 1
- Review effort level: Lite
Same class of bug as the timestamp fix: (*durationpb.Duration)(nil).AsDuration() doesn't panic, it silently returns 0 - which would make heartbeat_expiry_threshold 0s and every lease immediately expired. Added a requireDuration helper, used by poolToRow for heartbeat_interval and heartbeat_expiry_threshold. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TQHrhoyXpSBFYnVTrhxjvh
There was a problem hiding this comment.
🔵 Needs a closer look
There are correctness issues around timestamp/duration validation and ClaimAvailableVM can incorrectly return ErrNoAvailableVM under contention even when VMs remain AVAILABLE.
Review details
Files not reviewed (1)
- api/proto/poolmgr/v1alpha1/types.pb.go: Generated file
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
internal/store/convert.go:42
- requireDuration only checks for nil; protobuf durations can still be non-nil but invalid/out-of-range, and calling AsDuration()/Nanoseconds() on an invalid value can yield incorrect persisted values. Validate with d.CheckValid() and return a descriptive error before converting.
This issue also appears on line 137 of the same file.
internal/store/sqlite.go:261
- ClaimAvailableVM can return ErrNoAvailableVM on a lost race (RowsAffected()==0) even when other AVAILABLE VMs still exist in the pool (e.g., multiple claimers all SELECT the same uid with LIMIT 1). This violates the method contract that ErrNoAvailableVM means none are AVAILABLE; consider retrying selection on a 0-row UPDATE, or restructuring to a single atomic UPDATE that claims one row (SQLite supports UPDATE ... WHERE ... ORDER BY ... LIMIT 1) so contenders naturally claim different rows.
internal/store/convert.go:142
- requireTimestamp only checks for nil; protobuf timestamps can also be non-nil but invalid/out-of-range. Persisting ts.AsTime() for an invalid timestamp can silently store a wrong value (or overflow in conversions). Validate with ts.CheckValid() and return a descriptive error before converting.
func requireTimestamp(field string, ts *timestamppb.Timestamp) (time.Time, error) {
if ts == nil {
return time.Time{}, fmt.Errorf("store: %s is required", field)
}
return ts.AsTime(), nil
}
- Files reviewed: 8/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
internal/store.Store, coveringpools,vms,leases, and theeventsoutbox.modernc.org/sqlitedriver (no cgo), matching the design's GoReleaser cross-platform binary requirement.PoolSpec,VMRecord,LeaseRecord,Event) fromapi/proto/poolmgr/v1alpha1as the store's domain types instead of duplicating them.ClaimAvailableVMperforms the pick-an-AVAILABLE-VM-and-mark-LEASED transition inside a single transaction, so concurrent claims never race onto the same VM.Closes #5
Test plan
go build ./...go vet ./...golangci-lint run ./...go test ./... -race(24 tests ininternal/store, including a concurrency test forClaimAvailableVM)go mod tidy(no stray diff)