fix(ledger): survive concurrent ledger opens on a fresh database

[zzwong]

Closes #602

Problem

Starting several cr review processes at once died during runtime construction:

ledger: enable WAL: database is locked (261)

The issue proposed swapping the pragma order so busy_timeout is set before journal_mode = WAL. That is not sufficient, and investigating why turned up two further races. All three are fixed here.

Cause 1 — the busy handler does not cover WAL conversion

Converting a database to WAL begins a read transaction and then upgrades it to a write transaction. SQLite consults the busy handler only while no transaction is open, so the upgrade returns SQLITE_BUSY immediately regardless of how large busy_timeout is. Reordering alone leaves the crash in place.

The conversion is now retried, and the resulting mode is verified rather than assumed. The retry deadline is an additional DefaultBusyTimeout on top of each attempt's own busy_timeout wait, so a worst-case open can take roughly twice that; the deadline is checked between attempts.

Cause 2 — per-connection pragmas were not guaranteed

busy_timeout and foreign_keys are per-connection. They were applied once after sql.Open, but database/sql discards a connection returning driver.ErrBadConn and dials a replacement that never runs that setup; SetMaxOpenConns(1) does not prevent this.

This is reachable, not hypothetical: cancelling a context during a query makes the driver call sqlite3_interrupt, after which ResetSession/IsValid report the connection unusable and database/sql replaces it. The replacement would then run with SQLite's default busy_timeout of 0 and foreign keys off.

Both pragmas now live in the DSN, which the driver applies to every connection it dials. The DSN is a file: URI rather than a bare path, because the driver splits the query string at the first ? and a filesystem path may itself contain ?, #, or %.

The DSN also sets _txlock=immediate, which is a behavior change worth stating plainly: every non-read-only BeginTx in the ledger now takes the write lock at BEGIN instead of upgrading later. All six such transactions already sit behind the single writer goroutine and SetMaxOpenConns(1), and all read paths are autocommit queries that take no BEGIN, so in-process reads are not serialized by this. Across processes it is what makes a contended write wait for busy_timeout instead of failing instantly with SQLITE_BUSY_SNAPSHOT. The one cost: ensureMeta on an already-migrated ledger used to be a pure read, so a read-only command can now wait on the write lock at open.

Cause 3 — migrations raced on a fresh ledger

dbmig.Apply read schema_version before opening each migration transaction, so two processes racing a brand-new ledger could both plan migration 1, and the loser failed with table prs already exists. The version is now re-read inside the migration transaction, which is race-free only because the ledger opens with immediate transactions — recorded in the dbmig package doc, since the package cannot enforce it.

Re-checking introduced a second, quieter bug that is also fixed here: the observed version was discarded and CurrentVersion advanced from the plan, so an older binary that lost the race to a newer one skipped every migration, returned no error, and proceeded on a schema it does not know — exactly what ErrDowngrade exists to refuse. applyMigration now reports the version it read under the write lock, and a downgrade discovered mid-run is refused.

Evidence

Each fix was mutation-checked by reverting it alone and observing the relevant test fail:

Reverted	Failure
WAL retry (DSN pragmas kept)	opener 1/2/5: ledger: enable WAL: database is locked (5) (SQLITE_BUSY)
dbmig version re-check	opener 0: apply migration 1 "ledger schema": SQL logic error: table prs already exists (1)
DSN busy_timeout key	TestOpenAppliesPerConnectionPragmasFromDSN fails
Observed-version reporting	observed version = 2, want 7
Mid-run downgrade guard	Apply error = <nil>, want ErrDowngrade
_txlock=immediate	concurrent opens fail again

The dbmig race needed roughly 25 iterations to surface, so the concurrency test runs under -race -count=25; it is stable with the fixes and reproducibly fails without them. It also asserts every racing open lands on a fully migrated database in WAL mode, so a regression shows up as wrong state rather than only as an error.

go build ./..., go vet ./internal/... and go test -count=1 ./... are clean.

[monit-reviewer]

Automated PR Review

Reviewed commit: bd9dbea16716
Profile: pi-ds-gh - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	1
structure:repo-health	2

go:implementation-tests (1 finding)

Minor - internal/dbmig/dbmig.go:301

The downgrade guard only observes versions that moved before a migration's DDL. applyMigration re-reads schema_version under the write lock (line 275) and Apply now trusts that value (line 88: "Trust the version read under the write lock, not the planned one"), but the success path still returns the planned migration.Version instead of the version actually stored by the UPDATE at line 286. So a version that moves during a migration's own transaction is invisible to Apply once that migration is the last one in the plan: the loop ends, current > target is never true, and Apply returns nil with a stale CurrentVersion on a schema newer than the code — exactly the silent-proceed outcome ErrDowngrade exists to refuse. The current == target early return at line 76 has the same hole and never re-reads at all.

This is demonstrable with the fixture already added in this PR: move the advance trigger from TestApplyRefusesDowngradeDiscoveredMidRun onto the last migration (version 2 of 2) instead of the first, and Apply returns nil with CurrentVersion == 2 while the database sits at 9, instead of ErrDowngrade. I have not verified this is reachable from a plain concurrent process today (a second process cannot write meta while we hold the immediate write lock, so the trigger is a stand-in), but the guard is narrower than the invariant its own comment claims, and the test that would pin it is nearly free.

Concrete fix: after the UPDATE succeeds, read the stored value back inside the same transaction (SELECT schema_version FROM meta, or UPDATE ... RETURNING schema_version) and return that as observed, so the existing current > target check covers the final migration too; add the last-migration trigger case to TestApplyRefusesDowngradeDiscoveredMidRun.

structure:repo-health (2 findings)

Minor - internal/ledger/ledger_test.go:62

The DSN's path-conversion contract is untested, and it is the reason the helper is non-trivial.

Invariant: sqliteDataSourceName exists as a file: URI (per its own comment) because the driver splits the query string at the first ?, and a filesystem path may contain ?, #, or %; it also resolves the input to an absolute path and relies on the localhost authority to keep Windows drive paths well formed.

Problem: every test input is of the form filepath.Join(t.TempDir(), "ledger.db"), so no percent-encoding, authority rendering, or relative-path resolution is pinned by a test.

Impact: replacing the url.URL construction with "file:" + path + "?" + query.Encode() — the exact shape the comment warns against — passes the whole suite while making ledgers unopenable for anyone whose data root (HOME/XDG_DATA_HOME) contains those characters.

Fix: add a table test on sqliteDataSourceName: a relative path resolves to an absolute one, and file names containing space, ?, #, and % produce a DSN that sql.Open("sqlite", dsn) accepts and that reports the expected busy_timeout/foreign_keys (reuse assertSQLitePragmas).

Minor - internal/ledger/ledger.go:533

_txlock=immediate is now a load-bearing cross-package precondition that nothing enforces.

Invariant: internal/dbmig's package doc (added in this PR) states Apply re-reads schema_version inside each migration transaction and that this "is only race-free because callers open SQLite with immediate transactions (see the ledger DSN)". The ledger DSN is that caller contract.

Problem: the coupling is prose-only in both directions. The only test that reads the DSN back, TestOpenAppliesPerConnectionPragmasFromDSN, asserts busy_timeout and foreign_keys but not the txlock, and the only guard for the txlock is the scheduling-dependent TestOpenConcurrentOnSamePathSucceeds. The PR itself notes the equivalent dbmig race needed ~25 iterations to surface, so one green run proves little.

Impact: a future simplification of this function (or a DSN assembled at another call site, e.g. a read-only handle) drops immediate transactions, dbmig loses the write lock its in-transaction re-read depends on, and fresh-ledger races come back as table prs already exists with no deterministic failing test. The same setting also makes every BeginTx on this handle a write transaction, so a read-only transaction added later would silently serialize behind the writer with no comment nearby warning about it.

Fix: pin the contract directly — parse the DSN back in TestOpenAppliesPerConnectionPragmasFromDSN (or a sibling test) and require _txlock=immediate plus the expected _pragma entries; and state the "every BeginTx on this handle is a write transaction; do not add a read-only BeginTx" invariant on this setting and in the Open doc comment.

Reviewer Coverage

• go:implementation-tests — complete (broad); skipped: none; constraints: Read-only tooling: I did not run go build/vet/test or the -race -count=25 stress invocation, so the PR's mutation-check table is taken as reported rather than reproduced. The dbmig 'concurrent callers must open with immediate transactions' contract is only exercised indirectly by ledger's concurrent Open test; I could not verify it in isolation. ledger.go is ~2100 lines; I inspected the changed open/pragma/WAL/migration paths, all six BeginTx call sites, and the new tests rather than the whole file.
• structure:repo-health — complete (broad); inspected 2 assigned files (4 inspected across reviewers): internal/ledger/ledger.go, internal/ledger/ledger_test.go; skipped: none; constraints: CI test flags are not verifiable from this repo: .github/workflows/ci.yml delegates to the reusable open-cli-collective/.github go-test action, and Makefile 'test' runs 'go test -v ./...' with no -race/-count, so I could not confirm whether the -race -count=25 loop the PR relies on runs in CI. Findings are restricted to the assigned files (internal/ledger/*). internal/dbmig changes were read as context only and are not anchorable here. No docs in the repo describe the ledger connection contract (docs/architecture.md has no storage section), so code comments in internal/ledger are treated as the source of truth per docs/architecture-refactor-workstream.md. The modernc.org/sqlite driver internals (DSN query parsing, _txlock application to BeginTx) are not in this repo, so driver-level behavior was taken from the PR's mutation-check evidence rather than re-verified. cn_read returns byte-ranged windows and truncated several large reads, so internal/ledger/ledger.go was inspected in ~8KB overlapping chunks rather than end to end; the Open/DSN/migration paths were read in full.

Inspected files (4)

• internal/dbmig/dbmig.go
• internal/dbmig/dbmig_test.go
• internal/ledger/ledger.go
• internal/ledger/ledger_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 5m 54s | $0.01 | opencode-go/deepseek-v4.1-flash | cr dev

Field	Value
Model	opencode-go/deepseek-v4.1-flash
Reviewers	go:implementation-tests, structure:repo-health
Engine	pi_rpc · opencode-go/deepseek-v4.1-flash
Reviewed by	cr · monit-reviewer
Duration	5m 54s wall · 8m 41s compute
Cost	$0.01
Tokens	16.9k in / 6.1k out

Per-workstream usage

• orchestrator-selection — opencode-go/deepseek-v4.1-flash
  • In: 7.1k
  • Out: 1.5k
  • Cache read: 0
  • Cache create: 0
  • Cost: $0.00
  • Duration: 9s
• go:implementation-tests — opencode-go/deepseek-v4.1-flash
  • In: 2.2k
  • Out: 1.1k
  • Cache read: 75.9k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 5m 25s
• structure:repo-health — opencode-go/deepseek-v4.1-flash
  • In: 1.5k
  • Out: 2.9k
  • Cache read: 68.7k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 3m 02s
• orchestrator-rollup — opencode-go/deepseek-v4.1-flash
  • In: 6.0k
  • Out: 534
  • Cache read: 0
  • Cache create: 0
  • Cost: $0.00
  • Duration: 3s

[monit-reviewer]

Automated PR Review

Reviewed commit: a15d3c73f7d9
Profile: pi-ds-gh - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	3
structure:repo-health	3

go:implementation-tests (3 findings)

Minor - internal/ledger/ledger_test.go:2067

strings.Contains(dsn, filepath.ToSlash(cwd)) assumes the checkout path appears verbatim in the DSN, but url.URL.String() percent-encodes the path: any cwd containing a space, #, %, or a non-ASCII byte is escaped (<path> b renders as <path>), so this subtest fails spuriously on a machine whose working directory contains one — exactly the characters the rest of TestSQLiteDataSourceName deliberately exercises. Compare the decoded path instead: parsed, err := url.Parse(dsn) and compare parsed.Path with filepath.ToSlash(filepath.Join(cwd, "ledger.db")); net/url is already imported in this file, and this keeps the assertion about cwd resolution rather than about escaping.

Minor - internal/dbmig/dbmig.go:309

applyMigration now returns the version it reads back under the write lock, but Apply only compares that value with target (if current > target). A value that lands below the planned version flows through unchecked: if the migration's schema_version write is followed, inside the same transaction, by a trigger (or any writer) that lowers it, Apply returns nil, records that migration in result.Applied, and reports CurrentVersion below target while the database is behind what this binary's migrations installed — the silent-continue-on-unknown-schema failure this diff exists to eliminate, in the mirror direction. It is the same last-migration shape the diff fixes forward: TestApplyRefusesDowngradeDiscoveredMidRun only exercises schema_version = 9, so the lowering case is untested and passes. Cheapest close: after the read-back, refuse a stored value that is not what this migration wrote, e.g. if stored != migration.Version { return current, false, fmt.Errorf("%w: schema_version %d after migration %d", ErrInvalidMeta, stored, migration.Version) } (this also makes the rowsAffected == 1 check meaningful), or check current != target before the final return result, nil. Add the reciprocal table case with a trigger that writes a lower version.

Minor - internal/ledger/ledger.go:558

The retry policy introduced here is the fix for cause 1, yet nothing tests it directly. Its branches — busy then success, busy until deadline, ctx.Done(), and the non-busy passthrough — are only reachable through TestOpenConcurrentOnSamePathSucceeds, which is timing-dependent: whether the losing openers collide inside the WAL-conversion window depends on scheduling, so a run where they do not leaves the retry loop unexercised and a regression could pass (the sibling dbmig race needed ~25 iterations per the PR description). Take the attempt as a parameter, e.g. retryWALConversion(ctx, interval time.Duration, attempt func(context.Context) (string, error)) (string, error) with configureSQLite passing walRetryInterval and setWALJournalMode, then table-test: busy twice then success (assert attempt count), busy past the deadline with a tiny interval (no 5s wall-clock wait), cancelled context, and a non-busy error returned immediately. If you would rather not add the seam, a deterministic contention test works too: hold a deferred read transaction open on a second raw connection to the same file, release it after ~50ms, and assert Open succeeds.

structure:repo-health (3 findings)

Nits - internal/ledger/ledger_test.go:2021

The new doc comment at internal/ledger/ledger.go:515-519 makes Host: "localhost" load-bearing: without an authority, url.URL{Scheme: "file", Path: "<path>"}.String() renders file://C:<path>, which SQLite rejects as an invalid URI authority, so a Windows drive path would fail at open. The only assertion guarding that is this file:// prefix check, which passes for both file://localhost/... and the host-less file://C:<path> form, so deleting the Host field keeps every test in this file green on Linux — and the CI test job runs on ubuntu-latest only (ci.yml runs windows-latest in the non-required build-platform leg, which only builds). Impact: a contract that is documented as Windows-critical is enforced by prose alone, and Windows is a shipped target (winget/chocolatey packaging). Fix: assert the authority in the existing DSN test, which is platform-independent and cheap — e.g. parse the DSN and require parsed.Host == "localhost" (or require the file://localhost/ prefix) alongside the current checks.

Minor - internal/ledger/ledger_test.go:136

Invariant: the fully-migrated schema version has one definition in the package. SchemaVersion (internal/ledger/ledger.go:30, currently 5) is what every other assertion in this file uses (lines 27, 290, 345) and what production reports. This new line derives it a second way, from len(migrations()), which assumes migration versions are contiguous 1..N — a property the framework deliberately does not require, since dbmig.Migration keys on Version, not on slice index. Nothing in the repo pins SchemaVersion to the last migration's version, so the two expressions can diverge (e.g. a future migration renumbered or retired leaves a fully migrated ledger at a version that is not len(migrations())). Impact: the only in-repo guard for a racing open landing on a fully migrated database would then assert a value no production code agrees with, producing a confusing failure or, worse, silently tracking the migration count instead of the declared schema version. Fix: assert against SchemaVersion here like the sibling tests do, and pin the underlying invariant once, e.g. if got := migrations()[len(migrations())-1].Version; got != SchemaVersion { t.Fatalf("last migration version = %d, want SchemaVersion %d", got, SchemaVersion) }.

Nits - internal/ledger/ledger_test.go:155

The diff introduces assertSQLitePragmas as the shared seam for the per-connection pragma assertions but leaves the original copy in place: TestOpenMigratesFreshDatabaseAndAppliesStartupContract still inlines the same PRAGMA foreign_keys and PRAGMA busy_timeout checks at lines 33-42. Two copies of the same assertions now exist, and a third per-connection pragma added to the DSN would have to be remembered in both places while the older test's name keeps implying it checks the full startup contract. Fix: have the existing test call assertSQLitePragmas(t, store.db) for the two shared pragmas and keep only its journal_mode check inline (or extend the helper with the journal-mode assertion and have both tests use it).

Reviewer Coverage

• go:implementation-tests — complete (constrained); skipped: none; constraints: Driver DSN parsing of _pragma/_txlock was not verified against modernc.org/sqlite source; I relied on the new tests in the diff. Prior discussion threads on this diff are settled and were not re-raised, including the unanswered request for a comment on the current == target early return at internal/dbmig/dbmig.go:76. Read-only review: go build/go test were not executed here, so test-pass and mutation-check claims are the author's; I verified logic by reading the diff and source only. Scope was the four assigned dbmig/ledger files; cross-package effects were checked only via repo-wide searches for sql.Open, BeginTx, and pragma assertions.
• structure:repo-health — complete (constrained); inspected 2 assigned files (4 inspected across reviewers): internal/ledger/ledger.go, internal/ledger/ledger_test.go; skipped: none; constraints: Read-only artifact clone: no shell, so I could not run tests, go vet, or reproduce the PR body's mutation checks; those reported failures are taken at face value. Scope limited to the two assigned ledger files; internal/dbmig changes and the settled threads on them are not re-reviewed here. The PR's -race -count=25 stress recipe is not recorded anywhere in the repo (verified: no -race/count=25 in Makefile, CI, or docs), so I could not confirm whether the new ledger concurrency test reproduces single-shot.

Inspected files (4)

• internal/dbmig/dbmig.go
• internal/dbmig/dbmig_test.go
• internal/ledger/ledger.go
• internal/ledger/ledger_test.go

3 PR discussion threads considered. 3 summarized; 3 resolved.

---

Completed in 5m 08s | $0.01 | opencode-go/deepseek-v4.1-flash | cr dev

Field	Value
Model	opencode-go/deepseek-v4.1-flash
Reviewers	go:implementation-tests, structure:repo-health
Engine	pi_rpc · opencode-go/deepseek-v4.1-flash
Reviewed by	cr · monit-reviewer
Duration	5m 08s wall · 7m 19s compute
Cost	$0.01
Tokens	7.1k in / 6.5k out

Per-workstream usage

• go:implementation-tests — opencode-go/deepseek-v4.1-flash
  • In: 185
  • Out: 3.9k
  • Cache read: 67.6k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 4m 02s
• structure:repo-health — opencode-go/deepseek-v4.1-flash
  • In: 175
  • Out: 2.0k
  • Cache read: 61.1k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 3m 13s
• orchestrator-rollup — opencode-go/deepseek-v4.1-flash
  • In: 6.7k
  • Out: 649
  • Cache read: 0
  • Cache create: 0
  • Cost: $0.00
  • Duration: 4s

[monit-reviewer]

Automated PR Review

Reviewed commit: 2e801a76db64
Profile: pi-ds-gh - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	1
structure:repo-health	3

go:implementation-tests (1 finding)

Minor - internal/dbmig/dbmig.go:308

The read-back guard is asymmetric. applyMigration re-reads schema_version inside the transaction but only rejects a value below the migration it just ran (line 308), and Apply only rejects a value above the run's target (line 92). A read-back in the gap — migration.Version < stored <= target — passes both checks: Apply appends the migration to result.Applied, sets CurrentVersion = stored, and the loop's migration.Version <= current skip then drops every remaining migration whose version is <= stored, so those Ups never run and Apply returns nil on a durable schema missing their DDL.

Concrete reproduction with the existing fixture: add advanceOn(1, "create widgets", ddl, 2) to TestApplyRefusesDowngradeDiscoveredMidRun (two migrations, target 2). The trigger bumps schema_version to 2 at the end of migration 1, so migration 2's Up never executes, yet Apply returns no error. The comment on lines 302-304 claims a value above what the migration wrote is 'a newer binary moved the schema forward, which Apply reports as a downgrade once it sees it' — that is only true above target, so the guard is narrower than its own comment. This is the same failure shape as the mid-run downgrade finding already fixed on this PR, one band over.

Reachability is narrow: under _txlock=immediate the transaction holds the write lock from BEGIN, so only a same-transaction writer (a trigger, or a Up that writes meta) can move the value between the UPDATE and the read-back. That is why this is minor rather than blocking, but the invariant Apply should not prefer a planned version — or accept a stored version that disagrees — still does not hold in this band.

Fix: in Apply, after current = observed, also error when applied && current != migration.Version (ErrDowngrade), which preserves the existing to: 9 expectations and closes the middle band; then add {name: "advances to the target", advance: 1, to: 2, wantErr: ErrDowngrade} to the table.

structure:repo-health (3 findings)

Minor - internal/ledger/ledger_test.go:86

The fix is verified under an invocation the repo does not own. The PR body cites -race -count=25 as the evidence for the WAL-conversion retry, the _txlock=immediate contract, and the dbmig version re-check, but the repo-owned automation only ever runs a single, non-race pass: make test is go test -v ./... (Makefile:13), .goreleaser.yml pre-hook is go test ./..., and docs/development.md Quick Commands lists make test as go test ./.... Nothing in the repo records -race or a repetition count, and this test is the only integration guard for configureSQLite's retry loop (the deterministic asserts added here pin the DSN string, not the WAL-retry integration). These are precisely the concurrency fixes that a single uncontended pass plus no race detector can miss, so a future change can regress the behavior and still go green, with the reproduction recipe surviving only in PR text. Fix: add a repo-owned target (e.g. test-race running go test -race -count=25 ./internal/ledger/... ./internal/dbmig/...), call it from .github/workflows/ci.yml, and document it under Quick Commands in docs/development.md so the evidence is reproducible after squash-merge.

Nits - internal/ledger/ledger.go:606

isSQLiteBusyError is now the second copy of the "reduce a driver error to its SQLite primary result code" idiom: isSQLiteConstraintError (lines 2059-2064) already does the same errors.As(err, &sqliteErr) plus Code()&0xff comparison. The masking is the part carrying the real invariant (the driver reports extended result codes, so callers must mask), which means the invariant is now stated in two predicates and any change to how driver codes are extracted has two edit sites that can drift apart. Fix: extract sqlitePrimaryCode(err error) (int, bool) and have both predicates compare its result against sqlite3.SQLITE_BUSY / sqlite3.SQLITE_CONSTRAINT.

Nits - internal/ledger/ledger_test.go:2102

busyError re-creates the exact DSN shape sqliteDataSourceName exists to forbid: it concatenates "file://localhost" + filepath.ToSlash(path) + "?_pragma=busy_timeout(0)&_txlock=immediate". The path is a controlled temp file today, so the test is not wrong, but this package now carries two definitions of "how to open SQLite correctly" and the test copy is the one an agent or contributor is most likely to copy, since it is not guarded by the doc rationale on the production helper. It has already drifted syntactically (the busy_timeout(0) pragma form here versus busy_timeout=5000 in production). Fix: keep sqliteDataSourceName the single seam by adding a thin unexported variant that accepts extra/overriding query values (e.g. sqliteDataSourceNameWithQuery(path, url.Values)) and have busyError build its DSN through it.

Reviewer Coverage

• go:implementation-tests — complete (constrained); skipped: none; constraints: Read-only review via cr_diff/cr_read/cr_search; no test execution, so the PR's mutation-check outputs and the -count=25 concurrency stability are taken from the description and discussion, not re-run. Scoped to the pinned diff and the changed regions of the four assigned files; surrounding code was read only for context. Test files were read in byte ranges due to tool output truncation; helper definitions (openTestDB, countedMigration, openStoreAt, queryInt, assertSQLitePragmas) were confirmed present. The dbmig finding's reachability depends on a same-transaction writer (trigger, or an Up that writes meta), because _txlock=immediate excludes real concurrent writers while applyMigration's transaction is open.
• structure:repo-health — complete (constrained); inspected 2 assigned files (4 inspected across reviewers): internal/ledger/ledger.go, internal/ledger/ledger_test.go; skipped: none; constraints: Read-only review: I inspected the pinned diff and the two assigned ledger files plus repo-owned docs/Makefile/CI files. I did not run go test, -race, or the mutation checks, so the failure outputs quoted in the PR body are taken at face value. The shared reusable actions (.github/actions/go-test@v1, go-lint@v1) live in open-cli-collective/.github, outside this checkout, so I cannot confirm their exact go test flags. My automation claims are limited to repo-owned files (Makefile, .goreleaser.yml, .github/workflows/ci.yml, docs/developme... Windows DSN behavior (filepath.ToSlash drive path rendered under a localhost authority) could not be exercised here; CI's test job runs only on ubuntu, so any concern about that path is inference, not verified, and is not reported as a finding.

Inspected files (4)

• internal/dbmig/dbmig.go
• internal/dbmig/dbmig_test.go
• internal/ledger/ledger.go
• internal/ledger/ledger_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 3m 29s | $0.01 | opencode-go/deepseek-v4.1-flash | cr dev

Field	Value
Model	opencode-go/deepseek-v4.1-flash
Reviewers	go:implementation-tests, structure:repo-health
Engine	pi_rpc · opencode-go/deepseek-v4.1-flash
Reviewed by	cr · monit-reviewer
Duration	3m 29s wall · 5m 16s compute
Cost	$0.01
Tokens	6.7k in / 7.4k out

Per-workstream usage

• go:implementation-tests — opencode-go/deepseek-v4.1-flash
  • In: 196
  • Out: 3.3k
  • Cache read: 65.2k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 2m 55s
• structure:repo-health — opencode-go/deepseek-v4.1-flash
  • In: 235
  • Out: 3.5k
  • Cache read: 59.4k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 2m 17s
• orchestrator-rollup — opencode-go/deepseek-v4.1-flash
  • In: 6.3k
  • Out: 670
  • Cache read: 0
  • Cache create: 0
  • Cost: $0.00
  • Duration: 4s

[monit-reviewer]

Automated PR Review

Reviewed commit: 27842bbeebd7
Profile: pi-ds-gh - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	0
structure:repo-health	1

structure:repo-health (1 finding)

Minor - internal/ledger/ledger.go:539

Invariant: ledger.Open is on the startup path of every cr command, so its worst-case latency and its locking behavior are a user-visible contract and should be recorded where the setting lives, not only in PR text.

What the diff does: _txlock=immediate makes every BeginTx on this handle a write transaction, including the ones that are not writes. dbmig.Apply always runs ensureMeta before its current == target early return, and ensureMeta (internal/dbmig/dbmig.go:130) is not a pure read — it runs CREATE TABLE IF NOT EXISTS meta and a unique index. With the DSN's busy_timeout=DefaultBusyTimeout (5s), a read-only command that previously opened an already-migrated ledger without waiting can now block up to that timeout behind another process's writer. Separately, the retry call at line 558 passes the same DefaultBusyTimeout as the wall-clock budget while each attempt can itself consume a full busy_timeout before the deadline is checked, so worst-case Open is roughly 2x that.

Why it matters: the comment above line 539 documents the write-transaction invariant and the dbmig coupling well, but says nothing about the latency consequence, and retryWhileBusy's doc only says "budget elapses" without noting the budget sits on top of the per-attempt wait. The PR description is the only place that states "a read-only command can now wait on the write lock at open" and "a worst-case open can take roughly twice that". Squash-merge drops that text, so the next person debugging a multi-second startup stall, or adding a second opener / read-only path, has to re-derive both numbers from history. The retry budget is also silently coupled to a constant whose documented meaning is "how long a connection waits for a lock", so raising DefaultBusyTimeout doubles worst-case startup with no test or comment noticing (TestRetryWhileBusy injects its own 20ms budget and never checks the production wiring).

Concrete fix: (1) extend the comment at line 539 to state that startup always takes the write lock because ensureMeta writes, so even read-only commands can wait up to busy_timeout at open; (2) at line 558, use a named walRetryBudget constant instead of reusing DefaultBusyTimeout, and comment that worst-case Open is about 2x that value because the deadline is checked between attempts.

Reviewer Coverage

• go:implementation-tests — complete (constrained); skipped: none; constraints: I inspected the pinned head diff (27842bb) and surrounding code only; I did not execute the suite, so the mutation-check results, go test, and -race -count=25 evidence in the PR body and prior threads are taken as reported, not re-verified here. The dbmig mid-run guards are reachable only via same-transaction tampering (the trigger fixture), because _txlock=immediate excludes second-process writers; their reachability is narrow by construction, as the PR states. Thirteen prior threads on these four files are settled (retry seam, DSN pinning, Apply version guards, _txlock contract, helper reuse, -race CI gap deferred to #607); I deliberately did not re-raise them.
• structure:repo-health — complete (constrained); inspected 2 assigned files (4 inspected across reviewers): internal/ledger/ledger.go, internal/ledger/ledger_test.go; skipped: none; constraints: Assigned files are internal/ledger/ledger.go and internal/ledger/ledger_test.go. internal/dbmig/dbmig.go was inspected for context (ensureMeta/Apply call path) but is not a finding target for this reviewer. Findings are limited to structural risk that compounds; the correctness of the migration/downgrade guards is reviewed by the implementation-focused agents. Items already settled in earlier rounds were not re-raised: the duplicated SQLite primary-result-code predicate, routing the busyError test DSN through sqliteDataSourceName, and pinning last-migration version == SchemaVersion. Read-only review: no build, test, or mutation run was executed here. The PR's mutation-check and -race -count=25 results are taken from the PR body and thread replies, not re-verified. The repo-automation gap (no -race target; make test is go test -v ./...) was already raised on this PR and deferred to #607, so it is not re-reported. cr_read/cr_search offsets are byte-based here, so line references were resolved via cr_search line numbers.

Inspected files (4)

• internal/dbmig/dbmig.go
• internal/dbmig/dbmig_test.go
• internal/ledger/ledger.go
• internal/ledger/ledger_test.go

10 PR discussion threads considered. 10 summarized; 10 resolved.

---

Completed in 9m 10s | $0.00 | opencode-go/deepseek-v4.1-flash | cr dev

Field	Value
Model	opencode-go/deepseek-v4.1-flash
Reviewers	go:implementation-tests, structure:repo-health
Engine	pi_rpc · opencode-go/deepseek-v4.1-flash
Reviewed by	cr · monit-reviewer
Duration	9m 10s wall · 6m 43s compute
Cost	$0.00
Tokens	6.1k in / 3.1k out

Per-workstream usage

• go:implementation-tests — opencode-go/deepseek-v4.1-flash
  • In: 712
  • Out: 1.8k
  • Cache read: 78.8k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 3m 25s
• structure:repo-health — opencode-go/deepseek-v4.1-flash
  • In: 197
  • Out: 1.0k
  • Cache read: 67.2k
  • Cache create: 0
  • Cost: $0.00
  • Duration: 3m 15s
• orchestrator-rollup — opencode-go/deepseek-v4.1-flash
  • In: 5.2k
  • Out: 234
  • Cache read: 0
  • Cache create: 0
  • Cost: $0.00
  • Duration: 2s

[monit-reviewer]

Invariant: ledger.Open is on the startup path of every cr command, so its worst-case latency and its locking behavior are a user-visible contract and should be recorded where the setting lives, not only in PR text.

What the diff does: _txlock=immediate makes every BeginTx on this handle a write transaction, including the ones that are not writes. dbmig.Apply always runs ensureMeta before its current == target early return, and ensureMeta (internal/dbmig/dbmig.go:130) is not a pure read — it runs CREATE TABLE IF NOT EXISTS meta and a unique index. With the DSN's busy_timeout=DefaultBusyTimeout (5s), a read-only command that previously opened an already-migrated ledger without waiting can now block up to that timeout behind another process's writer. Separately, the retry call at line 558 passes the same DefaultBusyTimeout as the wall-clock budget while each attempt can itself consume a full busy_timeout before the deadline is checked, so worst-case Open is roughly 2x that.

Why it matters: the comment above line 539 documents the write-transaction invariant and the dbmig coupling well, but says nothing about the latency consequence, and retryWhileBusy's doc only says "budget elapses" without noting the budget sits on top of the per-attempt wait. The PR description is the only place that states "a read-only command can now wait on the write lock at open" and "a worst-case open can take roughly twice that". Squash-merge drops that text, so the next person debugging a multi-second startup stall, or adding a second opener / read-only path, has to re-derive both numbers from history. The retry budget is also silently coupled to a constant whose documented meaning is "how long a connection waits for a lock", so raising DefaultBusyTimeout doubles worst-case startup with no test or comment noticing (TestRetryWhileBusy injects its own 20ms budget and never checks the production wiring).

Concrete fix: (1) extend the comment at line 539 to state that startup always takes the write lock because ensureMeta writes, so even read-only commands can wait up to busy_timeout at open; (2) at line 558, use a named walRetryBudget constant instead of reusing DefaultBusyTimeout, and comment that worst-case Open is about 2x that value because the deadline is checked between attempts.

Reply inline to this comment.