fix(storage): apply missing out-of-order migrations - #3092
Conversation
i-trytoohard
left a comment
There was a problem hiding this comment.
Review — LGTM ✅ (vetted locally)
Reviewed on behalf of #3091. I checked out the branch, ran the new test, the full internal/storage/sqlite/... suite, -race, and go vet — all green.
The fix is correct and minimal
One functional line — goose.Up(db, "migrations", goose.WithAllowMissing()) — plus a clear "why" comment. goose.WithAllowMissing() is goose's documented, supported option (up.go:19 in the pinned v3.27.1) for exactly this case: applying embedded migrations that are absent from the DB's history but sit below its recorded max version. Not a hack, not a flag we're repurposing.
This is the right lever. The alternative (detect + surface a friendly error, as I'd floated in the triage) would still leave the user wedged; WithAllowMissing actually unblocks them.
Safety
The 4 flagged migrations (28–31) are additive — ALTER TABLE ADD COLUMN, CREATE TABLE, CREATE INDEX, one projects table-rebuild. They apply in order against the existing schema. If a given legacy DB already contains one of those schema objects, goose surfaces a clear SQL error naming the offending migration — strictly better than the opaque "found 4 missing migrations" wedge, and no worse than the current hard crash (daemon's already dead for these users). No silent corruption path.
Test quality — good
TestMigrateAppliesMissingMigrationsBeforeCurrentVersion correctly reproduces the bug: seeds to v27, injects a bare v46 goose_db_version row (the exact state behind the error), then asserts:
- all 4 missing migrations (28–31) apply,
- pre-existing project data survives — including a Windows path
C:\src\project-1, directly relevant to @maaz's Windows repro, - re-running
migrate()is a no-op (idempotent).
Non-blocking observation
The test injects v46 as a bare version row (schema stays at v27's state), so it exercises the WithAllowMissing path against a v27 schema — not against a DB that already has the 28–31 objects (the legacy-TS-DB permutation). That's fine: testing every legacy permutation is impractical, and the nightly-churn case (the actual reported scenario) is covered. Worth knowing it exists, not worth blocking on.
Verdict
Clean, surgical, well-tested, uses the supported mechanism, unblocks affected users without data loss. Ready to merge.
Summary
Root cause
Goose's default
Upbehavior rejects any embedded migration whose version is absent from the database history but lower than the database's maximum recorded version. A database produced by builds with later or renumbered migrations can therefore reach version 46 without versions 28–31 and permanently wedge daemon startup before the HTTP server is available.Using Goose's supported
WithAllowMissingoption applies those authoritative embedded migrations in order instead of rejecting the history.User impact
Affected Windows users can relaunch AO without moving, deleting, or resetting
~/.ao/data/ao.db. Existing project records are preserved.Fixes #3091.
Validation
go test ./internal/storage/sqlite/... -count=1go test -race ./internal/storage/sqlite/... -count=1go vet ./internal/storage/sqlite/...GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go build ./...The full
go test ./... -count=1run reached two unrelated wall-clock-sensitive agent test failures under package-parallel load; both pass when rerun in isolation, and the SQLite packages remain green.