fix(store): persistent WAL + version-gated migrations to stop multi-process SQLite corruption#613
Open
jarl9801 wants to merge 5 commits into
Open
Conversation
… WAL files Cold-start ordering bug: journal_mode = WAL ran before busy_timeout was set, so concurrent engram processes racing the rollback-journal-to-WAL conversion failed immediately with SQLITE_BUSY (Gentleman-Programming#559). busy_timeout now comes first and the WAL switch is wrapped in a bounded growing-backoff retry (mirroring withSQLiteWriteRetry) for the lock paths busy_timeout does not cover. Enable SQLITE_FCNTL_PERSIST_WAL on the single pooled connection via modernc.org/sqlite's FileControl so closing the last connection no longer unlinks the -wal/-shm files. The last-closer unlink, combined with divergent SHM views across process generations, allowed a process holding an orphaned SHM to delete the live WAL on clean shutdown and corrupt the database (Gentleman-Programming#477, Gentleman-Programming#571). Both New and newWithoutRepair now share one newStore path so the pragma setup can never diverge again.
… lock Every engram subcommand ran the full migrate() suite plus the enrolled- project sync repair unconditionally and without inter-process exclusion. Three migrations are destructive check-then-act rebuilds (FTS topic_key, sync_chunks, legacy observations): two processes racing a cold start could both pass the check and double-run DROP/recreate, corrupting the database. Startup now reads PRAGMA user_version and compares it to a new schemaVersion constant (1 = the fully-migrated v1.19.0 schema): - equal: skip migrate() and the startup repair entirely (fast read-only startup for every warm process). - older: take an exclusive advisory file lock on <DataDir>/.migrate.lock, re-read user_version under the lock (another process may have finished first), run migrate() plus the repair, stamp user_version, unlock. - newer: warn and skip — never run old migrations against a schema written by a newer engram. A file lock (flock on unix, LockFileEx on Windows) was chosen over one giant BEGIN IMMEDIATE transaction because migrate() executes dozens of statements directly on s.db and its rebuild helpers open their own transactions internally; wrapping the suite in a single SQL transaction would nest BEGINs and require rewriting every helper. The per-step transactions inside the destructive rebuilds keep crash atomicity; the file lock adds the missing cross-process mutual exclusion. The user_version stamp is written only after migrate() and the repair both succeed, so a failed run is retried on the next startup. Promotes golang.org/x/sys to a direct dependency for LockFileEx.
- persistent WAL: the -wal file must survive Close (a last-closer unlink here is the Gentleman-Programming#477/Gentleman-Programming#571 corruption mechanism). - concurrent cold start: 8 goroutines and 3 child processes open the same fresh database simultaneously; all must succeed, the migration suite must run exactly once, and the result must pass integrity_check (Gentleman-Programming#559). - user_version gate: a second open skips the migration suite (asserted via migrateRunCount), and a database stamped by a newer engram is opened without running this binary's migrations and without rewriting its user_version. - acquireMigrationLock: a second acquire blocks until the first holder releases.
…lock wait Three fixes from reliability review: Connection replacement (critical). The session pragmas and persist-WAL were applied once, to the first pooled connection only. database/sql silently discards a modernc connection after a context-cancelled query interrupts it (IsValid/ResetSession fail once sqlite3_is_interrupted) and opens an unconfigured replacement — busy_timeout back to 0 (Gentleman-Programming#559 returns) and persist-WAL unset (Gentleman-Programming#477 returns) for the rest of the process life. Production paths pass cancellable request contexts into the pool (server and MCP doctor handlers), so this was reachable. Now: - the four pragmas travel in the DSN as _pragma query parameters, which the driver applies to every physical connection it opens (and it orders busy_timeout ahead of the WAL switch itself); - persist-WAL is enabled by a driver-global RegisterConnectionHook (registered once via sync.Once, best-effort so exotic VFS opens never break) and strictly verified on the store's own database at startup; - the cold-start SQLITE_BUSY retry now wraps the first connection open, because DSN pragmas run inside driver.Open and a lost WAL-switch race fails the open as a whole. Replacement opens re-run journal_mode(WAL) as a cheap no-op read once the database is in WAL mode. Repair frequency. The user_version gate had reduced the enrolled-project sync repair from every-open to once per schema generation, regressing its self-healing contract. The repair runs on every open again, but inside the same exclusive migration lock so N concurrent startups cannot storm identical backfill writes. Exclusive (not shared) locking is correct because the repair writes when a backfill is needed; the healthy-path cost is one brief lock hold plus the repair's read-only fast path. Lock robustness. The migration lock was a blocking flock: a hung holder would silently stall every engram process on the machine forever. Both platform paths now acquire non-blocking (LOCK_NB / LOCKFILE_FAIL_IMMEDIATELY) inside a bounded retry loop (poll capped at 100ms, 60s total budget) and fail with an error naming the lock file and pointing at a stuck engram process. Shared loop lives in migration_lock.go; platform files provide only try/unlock primitives.
- TestConnectionReplacementKeepsConfiguration: plants a session-scoped cache_size marker, interrupts a long query via context timeout so the pool discards the poisoned connection, proves the physical connection was replaced (marker gone), and asserts the replacement is fully configured: busy_timeout 5000, journal_mode wal, foreign_keys on, and persist-WAL mode 1 via the file-control query — then writes, closes, and checks the -wal file still exists. - TestAcquireMigrationLockTimesOutWithDiagnostic: with a shortened timeout, a second acquire against a held lock fails after the budget with an error naming the lock file and a stuck engram process.
📝 WalkthroughWalkthroughSQLite startup now configures persistent WAL on every physical connection, retries initial connection setup, and gates migrations with ChangesSQLite startup and migrations
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StoreNew
participant SQLitePool
participant MigrationLock
participant MigrationSuite
StoreNew->>SQLitePool: open with DSN pragmas
StoreNew->>SQLitePool: prime connection and enable persistent WAL
StoreNew->>MigrationLock: acquire exclusive migration lock
StoreNew->>SQLitePool: read user_version
StoreNew->>MigrationSuite: run migrations when required
MigrationSuite->>SQLitePool: persist schema changes
StoreNew->>SQLitePool: stamp schemaVersion
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/store/startup_gate_test.go`:
- Around line 257-269: Update the cold-start child-process setup around
TestColdStartChildProcess so it uses explicit ready/release synchronization:
have each child signal readiness, wait until the parent confirms all children
are ready, then release every child immediately before New. Ensure the parent
waits for all readiness signals before releasing them, preserving deterministic
concurrent WAL and migration contention coverage.
In `@internal/store/store.go`:
- Around line 823-855: Update the migration flow around the migration-lock
acquisition to re-read user_version unconditionally after locking, including
when the initial version was current. If the re-read version exceeds
schemaVersion, return before running either s.migrate or
repairEnrolledProjectSyncMutations; otherwise preserve the existing migration
and repair behavior. Extend TestNewerSchemaVersionIsLeftUntouched to verify
repair is not invoked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1bb9d49e-37d6-4814-808c-3aa48da07de8
📒 Files selected for processing (6)
go.modinternal/store/migration_lock.gointernal/store/migration_lock_unix.gointernal/store/migration_lock_windows.gointernal/store/startup_gate_test.gointernal/store/store.go
Comment on lines
+257
to
+269
| cmds := make([]*exec.Cmd, procs) | ||
| outputs := make([]*strings.Builder, procs) | ||
| for i := range cmds { | ||
| outputs[i] = &strings.Builder{} | ||
| cmd := exec.Command(exe, "-test.run", "^TestColdStartChildProcess$", "-test.v", "-test.timeout", "60s") | ||
| cmd.Env = append(os.Environ(), coldStartChildEnv+"="+dir) | ||
| cmd.Stdout = outputs[i] | ||
| cmd.Stderr = outputs[i] | ||
| if err := cmd.Start(); err != nil { | ||
| t.Fatalf("start child %d: %v", i, err) | ||
| } | ||
| cmds[i] = cmd | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Synchronize the child processes before cold start.
cmd.Start() is not a concurrency barrier; an early child can complete New before later children reach it, allowing this test to pass without exercising cross-process WAL and migration contention. Add ready/release signaling so every child waits immediately before New.
As per path instructions, “Tests must be deterministic. Behavior changes without tests should be blocked.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/startup_gate_test.go` around lines 257 - 269, Update the
cold-start child-process setup around TestColdStartChildProcess so it uses
explicit ready/release synchronization: have each child signal readiness, wait
until the parent confirms all children are ready, then release every child
immediately before New. Ensure the parent waits for all readiness signals before
releasing them, preserving deterministic concurrent WAL and migration contention
coverage.
Source: Path instructions
Comment on lines
+823
to
855
| skipMigrate := shouldSkipMigrations(current) | ||
| if skipMigrate && !withRepair { | ||
| return nil // schema current and no repair requested: nothing to do | ||
| } | ||
|
|
||
| dbPath := filepath.Join(cfg.DataDir, "engram.db") | ||
| db, err := openDB("sqlite", dbPath) | ||
| unlock, err := acquireMigrationLock(filepath.Join(s.cfg.DataDir, ".migrate.lock")) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("engram: open database: %w", err) | ||
| return fmt.Errorf("engram: acquire migration lock: %w", err) | ||
| } | ||
| defer unlock() | ||
|
|
||
| ranMigrate := false | ||
| if !skipMigrate { | ||
| // Double-check under the lock: another process may have completed | ||
| // the migration while this one was waiting on the lock. | ||
| current, err = s.readUserVersion() | ||
| if err != nil { | ||
| return fmt.Errorf("engram: re-read user_version: %w", err) | ||
| } | ||
| if !shouldSkipMigrations(current) { | ||
| migrateRunCount.Add(1) | ||
| if err := s.migrate(); err != nil { | ||
| return fmt.Errorf("engram: migration: %w", err) | ||
| } | ||
| ranMigrate = true | ||
| } | ||
| } | ||
| db.SetMaxOpenConns(1) | ||
|
|
||
| pragmas := []string{ | ||
| "PRAGMA journal_mode = WAL", | ||
| "PRAGMA busy_timeout = 5000", | ||
| "PRAGMA synchronous = NORMAL", | ||
| "PRAGMA foreign_keys = ON", | ||
| if withRepair { | ||
| if err := s.repairEnrolledProjectSyncMutations(); err != nil { | ||
| return fmt.Errorf("engram: repair enrolled sync journal: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not run the old repair against a newer schema.
shouldSkipMigrations returns true for both current and future versions, but withRepair still executes repairEnrolledProjectSyncMutations. An old binary can therefore write to a newer schema. Additionally, when the initial version is current, it is not rechecked after waiting for the lock.
Re-read user_version unconditionally under the lock and return before any migration or repair when it exceeds schemaVersion. Extend TestNewerSchemaVersionIsLeftUntouched to assert the repair is not invoked.
Also applies to: 871-879
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/store/store.go` around lines 823 - 855, Update the migration flow
around the migration-lock acquisition to re-read user_version unconditionally
after locking, including when the initial version was current. If the re-read
version exceeds schemaVersion, return before running either s.migrate or
repairEnrolledProjectSyncMutations; otherwise preserve the existing migration
and repair behavior. Extend TestNewerSchemaVersionIsLeftUntouched to verify
repair is not invoked.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #477 · Fixes #559 · Addresses #571 (and the DB-lifecycle item in #508)
Problem
With one
engram mcpper agent session plusengram serveand periodic CLI/sync runs, ~10 processes hold~/.engram/engram.dbopen concurrently. Two corruption mechanisms have hit my store six times since April (details in #477):migrate()suite plusrepairEnrolledProjectSyncMutations()with no inter-process exclusion. The destructive check-then-act rebuilds (migrateFTSTopicKey,migrateSyncChunksTable,migrateLegacyObservationsTable) can double-run under concurrent cold starts → cross-linked pages /database disk image is malformed (11).-wal/-shm, deciding "last" from its own SHM mapping. A process whose SHM is a deleted orphan cannot see live lockers, so a clean shutdown deletes the LIVE WAL/SHM and orphans the next generation of processes (observed live on 2026-07-15; forensic signature:lsof +L1shows engram PIDs holding NLINK=0-wal/-shminodes).Additionally
journal_mode=WALran beforebusy_timeoutwas set, so concurrent cold starts die instantly on SQLITE_BUSY (#559).Fix
_pragmaparams (busy_timeout(5000)→journal_mode(WAL)→synchronous(NORMAL)→foreign_keys(1)): applied by the driver on every physical connection — including pool replacements after a context-interrupted query, which previously would have silently dropped the session config (modernc discards interrupted conns viaResetSession/IsValid).sqlite.RegisterConnectionHookcallingFileControlPersistWAL("main", 1)on every new conn (strictly verified for the store's own DB inprimeConnection; best-effort elsewhere). With persist-WAL, no closer ever unlinks-wal/-shm, eliminating mechanism 2 at the root.PRAGMA user_version-gated migrations (schemaVersion = 1): fast read-only startup when current; when stale, an exclusive advisory file lock (<DataDir>/.migrate.lock,flockon unix /LockFileExon Windows, non-blocking retries bounded at 60s with a clear diagnostic) serializes the whole migrate+repair block across processes, with a double-checked re-read under the lock.user_versionis stamped only after success, and a newer stamp is warn-and-skip (mixed-version safety).repairEnrolledProjectSyncMutationsstill runs on every open (original self-heal semantics) but now inside the lock.primeConnectionretries the first pooled conn with bounded backoff, since DSN pragmas run insidedriver.Open(fix(store): journal_mode=WAL switch dies instantly on concurrent cold start #559).Tests
New
internal/store/startup_gate_test.go(+ lock timeout test): persistent WAL survivesClose; connection-replacement test (context-interrupted query → pool replaces conn → busy_timeout/journal_mode/foreign_keys/persist-WAL all still active,-walsurvives close); concurrent cold start with 8 goroutines and with 3 real child processes (exactly one migration run,integrity_checkok); user_version gating (skip on second open, newer-stamp untouched); lock exclusion + timeout diagnostics.go test ./internal/store/ -race -count=3green; cross-builds verified forwindows/amd64andlinux/amd64.Live smoke on a real workload (macOS ARM64): 4 concurrent cold-start
saveprocesses — all succeed,-walpersists after every exit,user_version=1,integrity_checkok. Running this build as my daily binary since 2026-07-15.Notes for review
BEGIN IMMEDIATEbecausemigrate()helpers open their own transactions internally; each destructive rebuild keeps its per-step transaction for crash atomicity. The lock file is intentionally never unlinked.schemaVersionstarts at 1 (= the fully migrated v1.19.0 schema); bump it whenevermigrate()grows. Existing databases stamp on their first patched open (idempotent — the rebuilds skip when the schema is current).Summary by CodeRabbit
Bug Fixes
Tests