← back to Cli Printing Press
fix(cli): store.OpenWithContext + fast-path version check (#436, #438) (#441)
c11d0d952d388055772722080dd35ef0dd5dfeee · 2026-04-30 10:28:55 -0700 · Trevin Chow
* fix(cli): propagate caller context through store.Open and add fast-path version check
Closes #436 and #438 — both touch migrate() and are mutually completing,
so they land together to keep the diff coherent.
#436 ctx propagation:
- New OpenWithContext(ctx, dbPath) variant. Open(dbPath) becomes a thin
wrapper around OpenWithContext(context.Background(), dbPath) so every
existing caller stays source-compatible.
- migrate accepts a context.Context and threads it through Conn(),
ExecContext, QueryRowContext, and the retry-on-BUSY loop. Caller
cancellation (SIGINT into a Cobra command's cmd.Context()) now
surfaces as ctx.Err() within the cancellation window instead of
blocking up to migrationLockTimeout.
- sync, analytics, graphql_sync, and channel_workflow templates now
pass cmd.Context() into OpenWithContext. Other callers (insights/,
workflows/, data_source.go) keep using the synchronous Open wrapper
for now — they can be migrated incrementally without changing API.
#438 fast-path version check:
- migrate reads PRAGMA user_version on the pinned connection BEFORE
acquiring the migration lock. WAL readers normally don't block on
writers, but the WAL-init race on a fresh DB can BUSY the first
SELECT, so the read is wrapped in retryOnBusy with the same 30s
budget the lock uses.
- Generalized execWithBusyRetry into retryOnBusy(ctx, deadline, label,
op) — the same retry shape now covers BEGIN, COMMIT, and the pre-
lock SELECT against a single shared deadline.
- withMigrationLock accepts the deadline from migrate so the total
Open() budget across pre-lock read + BEGIN + COMMIT stays at 30s
rather than 3× that with per-stage deadlines.
- Old binary opening a newer-schema DB now refuses immediately even
while a peer holds the migration lock. The previous behavior made
the rejection wait out the full migrationLockTimeout — fast-path
read restores the immediate diagnostic.
Tests added to the generated store package:
- TestOpenWithContext_RespectsCancellation: holder-tx blocks the WAL
writer; OpenWithContext returns context.Canceled within 5s of a 250ms
cancel signal (vs the 30s baseline without the fix).
- TestMigrate_RejectsNewerDBImmediately: same holder-tx setup; an
Open() against a DB stamped at user_version 999 rejects in well
under migrationLockTimeout.
Generator-level canary (TestGenerateStoreMigrateUsesBeginImmediate)
now asserts OpenWithContext is exposed, migrate's signature is
ctx-accepting, and the pre-lock PRAGMA user_version read precedes
withMigrationLock.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(cli): apply /simplify findings to ctx + fast-path bundle
Findings from the three-agent /simplify pass on PR #441.
Reuse:
- Migrated 6 more Cobra RunE call sites that had cmd.Context() readily
available — insights/{health_score,similar}, workflows/{pm_orphans,
pm_load,pm_stale}, search. The previous PR description claimed only
4 long-running templates were migrated; this brings the total to 10.
7 sites remain unmigrated (data_source helpers, mcp_tools handlers,
share_commands, doctor's collectCacheReport, auto_refresh background
goroutine) — those need ctx-accepting variants of their surrounding
functions, which is a follow-up scope rather than a one-line swap.
- Extracted holdWriteLock(t, dbPath) test helper. Both
TestOpenWithContext_RespectsCancellation and
TestMigrate_RejectsNewerDBImmediately had identical 14-line setup
for the holder transaction; helper folds them to one line each.
Quality:
- backoff *= 2; if backoff > max → backoff = min(backoff*2, max). Go
1.21+ built-in min is available in the generated Go 1.23+ targets.
- Trimmed the fast-path block comment in migrate (8 lines → 4) and the
shared-deadline comment on withMigrationLock — both were paraphrasing
the parameter names rather than carrying load-bearing WHY.
Efficiency:
- TestOpenWithContext_RespectsCancellation now uses a pre-cancelled ctx
instead of time.Sleep(250ms) + goroutine cancel. The retry loop's
ctx.Done() select fires on the first iteration after the holder-tx
BUSYs BEGIN IMMEDIATE; a sleep was wasted CI budget proving the same
property. Saves ~250ms per test run.
Skipped on closer review:
- Efficiency suggestion to drop the in-lock user_version re-read. The
agent's argument missed the older-binary-vs-newer-DB downgrade
scenario: if process A (newer, StoreSchemaVersion=2) commits its
stamp, and process B (older, StoreSchemaVersion=1) wins the lock
next, B's in-closure stamp PRAGMA user_version = 1 would silently
downgrade the version on a schema that's already at the newer level.
The re-read is a load-bearing correctness guarantee, not just
defense-in-depth, and stays as-is.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(cli): apply review polish to ctx + fast-path bundle
Findings from the /review pass on PR #441.
- mcp_tools.go.tmpl: handleSearch and handleSQL each receive an MCP
ctx as their first parameter. Both now thread it through
store.OpenWithContext instead of dropping it on the floor.
- auto_refresh.go.tmpl: autoRefreshIfStale already accepted a ctx
parameter. Use it for the store open, same one-line swap.
- store.go.tmpl retryOnBusy: timeout error message no longer
hardcodes "waiting for write lock" — pre-lock SELECT timeouts now
read coherently. The label parameter already carries the operation
name (e.g. "begin migration transaction", "reading schema version").
- store.go.tmpl in-lock version re-read: replaced the terse "defense
in depth" comment with an explicit explanation of the older-binary
downgrade scenario. The duplicate read is load-bearing for cross-
binary correctness; a future maintainer who sees two reads should
immediately see why both are needed.
- generator_test.go: TestGenerateFreshnessHelperEmitted's substring
check followed the auto_refresh migration; it now asserts the
OpenWithContext form.
The four remaining wrapper-function call sites (openStoreForRead,
writeThroughCache, openShareStore, collectCacheReport) need ctx
threaded through their signatures and callers — mechanical but
adds API surface to a generated CLI, so deferred to #442 as a
focused follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Files touched
M internal/generator/generator_test.goM internal/generator/templates/analytics.go.tmplM internal/generator/templates/auto_refresh.go.tmplM internal/generator/templates/channel_workflow.go.tmplM internal/generator/templates/graphql_sync.go.tmplM internal/generator/templates/insights/health_score.go.tmplM internal/generator/templates/insights/similar.go.tmplM internal/generator/templates/mcp_tools.go.tmplM internal/generator/templates/search.go.tmplM internal/generator/templates/store.go.tmplM internal/generator/templates/store_schema_version_test.go.tmplM internal/generator/templates/sync.go.tmplM internal/generator/templates/workflows/pm_load.go.tmplM internal/generator/templates/workflows/pm_orphans.go.tmplM internal/generator/templates/workflows/pm_stale.go.tmpl
Diff
commit c11d0d952d388055772722080dd35ef0dd5dfeee
Author: Trevin Chow <trevin@trevinchow.com>
Date: Thu Apr 30 10:28:55 2026 -0700
fix(cli): store.OpenWithContext + fast-path version check (#436, #438) (#441)
* fix(cli): propagate caller context through store.Open and add fast-path version check
Closes #436 and #438 — both touch migrate() and are mutually completing,
so they land together to keep the diff coherent.
#436 ctx propagation:
- New OpenWithContext(ctx, dbPath) variant. Open(dbPath) becomes a thin
wrapper around OpenWithContext(context.Background(), dbPath) so every
existing caller stays source-compatible.
- migrate accepts a context.Context and threads it through Conn(),
ExecContext, QueryRowContext, and the retry-on-BUSY loop. Caller
cancellation (SIGINT into a Cobra command's cmd.Context()) now
surfaces as ctx.Err() within the cancellation window instead of
blocking up to migrationLockTimeout.
- sync, analytics, graphql_sync, and channel_workflow templates now
pass cmd.Context() into OpenWithContext. Other callers (insights/,
workflows/, data_source.go) keep using the synchronous Open wrapper
for now — they can be migrated incrementally without changing API.
#438 fast-path version check:
- migrate reads PRAGMA user_version on the pinned connection BEFORE
acquiring the migration lock. WAL readers normally don't block on
writers, but the WAL-init race on a fresh DB can BUSY the first
SELECT, so the read is wrapped in retryOnBusy with the same 30s
budget the lock uses.
- Generalized execWithBusyRetry into retryOnBusy(ctx, deadline, label,
op) — the same retry shape now covers BEGIN, COMMIT, and the pre-
lock SELECT against a single shared deadline.
- withMigrationLock accepts the deadline from migrate so the total
Open() budget across pre-lock read + BEGIN + COMMIT stays at 30s
rather than 3× that with per-stage deadlines.
- Old binary opening a newer-schema DB now refuses immediately even
while a peer holds the migration lock. The previous behavior made
the rejection wait out the full migrationLockTimeout — fast-path
read restores the immediate diagnostic.
Tests added to the generated store package:
- TestOpenWithContext_RespectsCancellation: holder-tx blocks the WAL
writer; OpenWithContext returns context.Canceled within 5s of a 250ms
cancel signal (vs the 30s baseline without the fix).
- TestMigrate_RejectsNewerDBImmediately: same holder-tx setup; an
Open() against a DB stamped at user_version 999 rejects in well
under migrationLockTimeout.
Generator-level canary (TestGenerateStoreMigrateUsesBeginImmediate)
now asserts OpenWithContext is exposed, migrate's signature is
ctx-accepting, and the pre-lock PRAGMA user_version read precedes
withMigrationLock.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(cli): apply /simplify findings to ctx + fast-path bundle
Findings from the three-agent /simplify pass on PR #441.
Reuse:
- Migrated 6 more Cobra RunE call sites that had cmd.Context() readily
available — insights/{health_score,similar}, workflows/{pm_orphans,
pm_load,pm_stale}, search. The previous PR description claimed only
4 long-running templates were migrated; this brings the total to 10.
7 sites remain unmigrated (data_source helpers, mcp_tools handlers,
share_commands, doctor's collectCacheReport, auto_refresh background
goroutine) — those need ctx-accepting variants of their surrounding
functions, which is a follow-up scope rather than a one-line swap.
- Extracted holdWriteLock(t, dbPath) test helper. Both
TestOpenWithContext_RespectsCancellation and
TestMigrate_RejectsNewerDBImmediately had identical 14-line setup
for the holder transaction; helper folds them to one line each.
Quality:
- backoff *= 2; if backoff > max → backoff = min(backoff*2, max). Go
1.21+ built-in min is available in the generated Go 1.23+ targets.
- Trimmed the fast-path block comment in migrate (8 lines → 4) and the
shared-deadline comment on withMigrationLock — both were paraphrasing
the parameter names rather than carrying load-bearing WHY.
Efficiency:
- TestOpenWithContext_RespectsCancellation now uses a pre-cancelled ctx
instead of time.Sleep(250ms) + goroutine cancel. The retry loop's
ctx.Done() select fires on the first iteration after the holder-tx
BUSYs BEGIN IMMEDIATE; a sleep was wasted CI budget proving the same
property. Saves ~250ms per test run.
Skipped on closer review:
- Efficiency suggestion to drop the in-lock user_version re-read. The
agent's argument missed the older-binary-vs-newer-DB downgrade
scenario: if process A (newer, StoreSchemaVersion=2) commits its
stamp, and process B (older, StoreSchemaVersion=1) wins the lock
next, B's in-closure stamp PRAGMA user_version = 1 would silently
downgrade the version on a schema that's already at the newer level.
The re-read is a load-bearing correctness guarantee, not just
defense-in-depth, and stays as-is.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(cli): apply review polish to ctx + fast-path bundle
Findings from the /review pass on PR #441.
- mcp_tools.go.tmpl: handleSearch and handleSQL each receive an MCP
ctx as their first parameter. Both now thread it through
store.OpenWithContext instead of dropping it on the floor.
- auto_refresh.go.tmpl: autoRefreshIfStale already accepted a ctx
parameter. Use it for the store open, same one-line swap.
- store.go.tmpl retryOnBusy: timeout error message no longer
hardcodes "waiting for write lock" — pre-lock SELECT timeouts now
read coherently. The label parameter already carries the operation
name (e.g. "begin migration transaction", "reading schema version").
- store.go.tmpl in-lock version re-read: replaced the terse "defense
in depth" comment with an explicit explanation of the older-binary
downgrade scenario. The duplicate read is load-bearing for cross-
binary correctness; a future maintainer who sees two reads should
immediately see why both are needed.
- generator_test.go: TestGenerateFreshnessHelperEmitted's substring
check followed the auto_refresh migration; it now asserts the
OpenWithContext form.
The four remaining wrapper-function call sites (openStoreForRead,
writeThroughCache, openShareStore, collectCacheReport) need ctx
threaded through their signatures and callers — mechanical but
adds API surface to a generated CLI, so deferred to #442 as a
focused follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
internal/generator/generator_test.go | 11 ++-
internal/generator/templates/analytics.go.tmpl | 2 +-
internal/generator/templates/auto_refresh.go.tmpl | 2 +-
.../generator/templates/channel_workflow.go.tmpl | 4 +-
internal/generator/templates/graphql_sync.go.tmpl | 2 +-
.../templates/insights/health_score.go.tmpl | 2 +-
.../generator/templates/insights/similar.go.tmpl | 2 +-
internal/generator/templates/mcp_tools.go.tmpl | 4 +-
internal/generator/templates/search.go.tmpl | 2 +-
internal/generator/templates/store.go.tmpl | 91 +++++++++++++-----
.../templates/store_schema_version_test.go.tmpl | 102 +++++++++++++++++++++
internal/generator/templates/sync.go.tmpl | 2 +-
.../generator/templates/workflows/pm_load.go.tmpl | 2 +-
.../templates/workflows/pm_orphans.go.tmpl | 2 +-
.../generator/templates/workflows/pm_stale.go.tmpl | 2 +-
15 files changed, 193 insertions(+), 39 deletions(-)
diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index d15b85d8..a3ea3e0d 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -199,7 +199,7 @@ func TestGenerateFreshnessHelperEmitted(t *testing.T) {
assert.Contains(t, src, snippet, "auto_refresh.go missing %q", snippet)
}
optOutIndex := strings.Index(src, "env_opt_out")
- openStoreIndex := strings.Index(src, "store.Open(dbPath)")
+ openStoreIndex := strings.Index(src, "store.OpenWithContext(ctx, dbPath)")
require.NotEqual(t, -1, optOutIndex, "auto_refresh.go must report env opt-out")
require.NotEqual(t, -1, openStoreIndex, "auto_refresh.go must open the store after opt-out checks")
assert.Less(t, optOutIndex, openStoreIndex, "env opt-out must be checked before opening/migrating the store")
@@ -1335,6 +1335,10 @@ func TestGenerateStoreMigrateUsesBeginImmediate(t *testing.T) {
// keywords behind. The check would otherwise pass on comments alone.
codeOnly := stripGoComments(src)
+ assert.Contains(t, codeOnly, `func OpenWithContext(`,
+ "store package must expose OpenWithContext so callers can interrupt slow migrations with their own ctx")
+ assert.Contains(t, codeOnly, `func (s *Store) migrate(ctx context.Context) error`,
+ "migrate must accept a context.Context so caller cancellation propagates into the retry loop")
assert.Contains(t, codeOnly, `withMigrationLock(`,
"migrate must dispatch the lock helper — without this call, the BEGIN/COMMIT wrapper is unreachable")
assert.Contains(t, codeOnly, `s.db.Conn(ctx)`,
@@ -1343,6 +1347,11 @@ func TestGenerateStoreMigrateUsesBeginImmediate(t *testing.T) {
"migrate must wrap migrations in BEGIN IMMEDIATE so concurrent fresh-DB Opens serialize on the RESERVED lock instead of racing per-statement")
assert.Contains(t, codeOnly, `COMMIT`,
"migrate must commit the transaction explicitly")
+ // Fast-path: reading user_version on the pinned connection BEFORE the
+ // migration lock is what lets an old binary refuse a newer-schema DB
+ // without waiting out migrationLockTimeout.
+ assert.Regexp(t, `(?s)func \(s \*Store\) migrate\(ctx context\.Context\) error \{.*PRAGMA user_version.*withMigrationLock`, codeOnly,
+ "migrate must read PRAGMA user_version BEFORE entering withMigrationLock so newer-DB rejection happens before lock acquisition")
}
// stripGoComments removes // line comments and /* ... */ block comments from
diff --git a/internal/generator/templates/analytics.go.tmpl b/internal/generator/templates/analytics.go.tmpl
index fcac487d..7f8eda72 100644
--- a/internal/generator/templates/analytics.go.tmpl
+++ b/internal/generator/templates/analytics.go.tmpl
@@ -38,7 +38,7 @@ Data must be synced first with the sync command.`,
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- db, err := store.Open(dbPath)
+ db, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening local database: %w\nRun '{{.Name}}-pp-cli sync' first.", err)
}
diff --git a/internal/generator/templates/auto_refresh.go.tmpl b/internal/generator/templates/auto_refresh.go.tmpl
index 7d262bca..3d8669ea 100644
--- a/internal/generator/templates/auto_refresh.go.tmpl
+++ b/internal/generator/templates/auto_refresh.go.tmpl
@@ -107,7 +107,7 @@ func autoRefreshIfStale(ctx context.Context, flags *rootFlags, resources []strin
return meta
}
dbPath := defaultDBPath("{{.Name}}-pp-cli")
- db, err := store.Open(dbPath)
+ db, err := store.OpenWithContext(ctx, dbPath)
if err != nil {
fmt.Fprintf(os.Stderr, "warning: auto-refresh skipped (open: %v)\n", err)
meta.Decision = "error"
diff --git a/internal/generator/templates/channel_workflow.go.tmpl b/internal/generator/templates/channel_workflow.go.tmpl
index 678289d5..59d507b5 100644
--- a/internal/generator/templates/channel_workflow.go.tmpl
+++ b/internal/generator/templates/channel_workflow.go.tmpl
@@ -49,7 +49,7 @@ and full resync. After archiving, use 'search' for instant full-text search.`,
if dbPath == "" {
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- s, err := store.Open(dbPath)
+ s, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening store: %w", err)
}
@@ -157,7 +157,7 @@ func newWorkflowStatusCmd(flags *rootFlags) *cobra.Command {
if dbPath == "" {
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- s, err := store.Open(dbPath)
+ s, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening store: %w", err)
}
diff --git a/internal/generator/templates/graphql_sync.go.tmpl b/internal/generator/templates/graphql_sync.go.tmpl
index 8ea944dc..7b5a2ae7 100644
--- a/internal/generator/templates/graphql_sync.go.tmpl
+++ b/internal/generator/templates/graphql_sync.go.tmpl
@@ -77,7 +77,7 @@ Exit codes & warnings:
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- db, err := store.Open(dbPath)
+ db, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening local database: %w", err)
}
diff --git a/internal/generator/templates/insights/health_score.go.tmpl b/internal/generator/templates/insights/health_score.go.tmpl
index 91bed7d1..64115aee 100644
--- a/internal/generator/templates/insights/health_score.go.tmpl
+++ b/internal/generator/templates/insights/health_score.go.tmpl
@@ -34,7 +34,7 @@ A score of 100 means all items are fresh, assigned, and actively worked on.`,
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- db, err := store.Open(dbPath)
+ db, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening local database: %w\nRun '{{.Name}}-pp-cli sync' first.", err)
}
diff --git a/internal/generator/templates/insights/similar.go.tmpl b/internal/generator/templates/insights/similar.go.tmpl
index 355ecf0c..9c7fef47 100644
--- a/internal/generator/templates/insights/similar.go.tmpl
+++ b/internal/generator/templates/insights/similar.go.tmpl
@@ -37,7 +37,7 @@ work items, tickets, or tasks.`,
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- db, err := store.Open(dbPath)
+ db, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening local database: %w\nRun '{{.Name}}-pp-cli sync' first.", err)
}
diff --git a/internal/generator/templates/mcp_tools.go.tmpl b/internal/generator/templates/mcp_tools.go.tmpl
index 0a62c05a..48ebe917 100644
--- a/internal/generator/templates/mcp_tools.go.tmpl
+++ b/internal/generator/templates/mcp_tools.go.tmpl
@@ -318,7 +318,7 @@ func handleSearch(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.Call
limit = int(v)
}
- db, err := store.Open(dbPath())
+ db, err := store.OpenWithContext(ctx, dbPath())
if err != nil {
return mcplib.NewToolResultError(fmt.Sprintf("opening database: %v", err)), nil
}
@@ -351,7 +351,7 @@ func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToo
}
}
- db, err := store.Open(dbPath())
+ db, err := store.OpenWithContext(ctx, dbPath())
if err != nil {
return mcplib.NewToolResultError(fmt.Sprintf("opening database: %v", err)), nil
}
diff --git a/internal/generator/templates/search.go.tmpl b/internal/generator/templates/search.go.tmpl
index 79d3b04d..07116bc7 100644
--- a/internal/generator/templates/search.go.tmpl
+++ b/internal/generator/templates/search.go.tmpl
@@ -161,7 +161,7 @@ In local mode: searches locally synced data only.`,
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- db, err := store.Open(dbPath)
+ db, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening local database: %w\nRun '{{.Name}}-pp-cli sync' first to populate the local database.", err)
}
diff --git a/internal/generator/templates/store.go.tmpl b/internal/generator/templates/store.go.tmpl
index d1c81fd2..5feffd69 100644
--- a/internal/generator/templates/store.go.tmpl
+++ b/internal/generator/templates/store.go.tmpl
@@ -47,7 +47,18 @@ type Store struct {
path string
}
+// Open opens or creates the SQLite store at dbPath using the background
+// context. Prefer OpenWithContext from a Cobra command so SIGINT during
+// a slow migration interrupts the open instead of stranding the caller.
func Open(dbPath string) (*Store, error) {
+ return OpenWithContext(context.Background(), dbPath)
+}
+
+// OpenWithContext opens or creates the SQLite store at dbPath. The
+// context is honored by the migration path: cancellation interrupts the
+// retry-on-SQLITE_BUSY loop and propagates ctx.Err() back to the caller
+// instead of waiting out the full migrationLockTimeout.
+func OpenWithContext(ctx context.Context, dbPath string) (*Store, error) {
if err := os.MkdirAll(filepath.Dir(dbPath), 0o755); err != nil {
return nil, fmt.Errorf("creating db directory: %w", err)
}
@@ -63,7 +74,7 @@ func Open(dbPath string) (*Store, error) {
db.SetMaxOpenConns(2)
s := &Store{db: db, path: dbPath}
- if err := s.migrate(); err != nil {
+ if err := s.migrate(ctx); err != nil {
db.Close()
return nil, fmt.Errorf("running migrations: %w", err)
}
@@ -188,14 +199,28 @@ func (s *Store) backfillColumns(ctx context.Context, conn *sql.Conn) error {
return nil
}
-func (s *Store) migrate() error {
- ctx := context.Background()
+func (s *Store) migrate(ctx context.Context) error {
conn, err := s.db.Conn(ctx)
if err != nil {
return fmt.Errorf("acquiring migration connection: %w", err)
}
defer conn.Close()
+ // Read user_version before the migration lock so an old binary
+ // opening a newer-schema DB rejects immediately. WAL readers don't
+ // normally block on writers, but the fresh-DB WAL-init race can BUSY
+ // a SELECT — share the lock's deadline so total budget stays bounded.
+ deadline := time.Now().Add(migrationLockTimeout)
+ var current int
+ if err := retryOnBusy(ctx, deadline, "reading schema version", func() error {
+ return conn.QueryRowContext(ctx, `PRAGMA user_version`).Scan(¤t)
+ }); err != nil {
+ return err
+ }
+ if current > StoreSchemaVersion {
+ return fmt.Errorf("database schema version %d is newer than supported version %d; upgrade the CLI binary or open an older database", current, StoreSchemaVersion)
+ }
+
migrations := []string{
`CREATE TABLE IF NOT EXISTS resources (
id TEXT PRIMARY KEY,
@@ -272,10 +297,15 @@ func (s *Store) migrate() error {
// modernc.org/sqlite's busy_timeout does not always cover write-write
// contention at BEGIN/COMMIT time, so we retry both explicitly on
// SQLITE_BUSY for up to migrationLockTimeout.
- return withMigrationLock(ctx, conn, func() error {
- // Reading user_version inside the lock avoids racing against
- // another fresh-DB initializer that hasn't yet stamped the
- // version (which would manifest as a transient SQLITE_BUSY).
+ return withMigrationLock(ctx, conn, deadline, func() error {
+ // Re-read user_version inside the lock. This is load-bearing,
+ // not paranoid: between the pre-lock read above and our
+ // successful BEGIN IMMEDIATE, a newer-binary peer may have
+ // committed a higher version stamp. Without this re-read, an
+ // older binary (smaller StoreSchemaVersion) would proceed to
+ // stamp its own lower version at the end of the closure,
+ // silently downgrading user_version on a schema that's already
+ // at the newer level. Future maintainers: leave this read in.
var current int
if err := conn.QueryRowContext(ctx, `PRAGMA user_version`).Scan(¤t); err != nil {
return fmt.Errorf("reading schema version: %w", err)
@@ -311,17 +341,15 @@ const (
)
// withMigrationLock runs fn inside a BEGIN IMMEDIATE / COMMIT pair on
-// conn, retrying both BEGIN and COMMIT on SQLITE_BUSY against a single
-// shared deadline so a stuck peer caps Open() latency at roughly
-// migrationLockTimeout — not 2× that, as a per-call deadline would. The
-// real upper bound is migrationLockTimeout plus one trailing backoff
-// interval (≤100ms) plus the driver's busy_timeout for the in-flight
-// Exec, since the deadline is checked after each failed attempt rather
-// than as a hard wall-clock cutoff. The body itself is responsible for
-// using conn (not s.db) so the writes participate in the held
-// transaction.
-func withMigrationLock(ctx context.Context, conn *sql.Conn, fn func() error) error {
- deadline := time.Now().Add(migrationLockTimeout)
+// conn, retrying both BEGIN and COMMIT on SQLITE_BUSY against the
+// caller-provided deadline. Sharing the deadline with the pre-lock
+// version read keeps total Open() latency bounded by a single budget.
+// The real upper bound is deadline + one trailing backoff interval
+// (≤100ms) + the driver's busy_timeout for the in-flight Exec, since
+// the deadline is checked after each failed attempt rather than as a
+// hard wall-clock cutoff. fn must use conn (not s.db) so its writes
+// participate in the held transaction.
+func withMigrationLock(ctx context.Context, conn *sql.Conn, deadline time.Time, fn func() error) error {
if err := execWithBusyRetry(ctx, conn, "BEGIN IMMEDIATE", "begin migration transaction", deadline); err != nil {
return err
}
@@ -353,13 +381,26 @@ func withMigrationLock(ctx context.Context, conn *sql.Conn, fn func() error) err
}
// execWithBusyRetry runs stmt on conn and retries on SQLITE_BUSY until
-// deadline. It covers both BEGIN IMMEDIATE and COMMIT contention;
+// deadline. It covers BEGIN IMMEDIATE and COMMIT contention;
// modernc.org/sqlite's busy_timeout does not reliably cover either when
// multiple connections race for the WAL write lock.
func execWithBusyRetry(ctx context.Context, conn *sql.Conn, stmt, label string, deadline time.Time) error {
+ return retryOnBusy(ctx, deadline, label, func() error {
+ _, err := conn.ExecContext(ctx, stmt)
+ return err
+ })
+}
+
+// retryOnBusy runs op and retries it on SQLITE_BUSY/LOCKED until
+// deadline. The same retry shape covers Exec, Query, and any other
+// SQLite call that can race the WAL writer lock — including the
+// pre-lock user_version read, where the WAL initialization race on a
+// fresh DB can BUSY a SELECT that should otherwise succeed under WAL
+// reader/writer concurrency.
+func retryOnBusy(ctx context.Context, deadline time.Time, label string, op func() error) error {
backoff := migrationLockBackoffMin
for {
- _, err := conn.ExecContext(ctx, stmt)
+ err := op()
if err == nil {
return nil
}
@@ -367,16 +408,18 @@ func execWithBusyRetry(ctx context.Context, conn *sql.Conn, stmt, label string,
return fmt.Errorf("%s: %w", label, err)
}
if time.Now().After(deadline) {
- return fmt.Errorf("%s: timed out after %s waiting for write lock: %w", label, migrationLockTimeout, err)
+ // The label carries the operation context (e.g. "begin
+ // migration transaction", "reading schema version") — we
+ // don't hardcode "waiting for write lock" because pre-lock
+ // reads also flow through this helper.
+ return fmt.Errorf("%s: timed out after %s under SQLite contention: %w", label, migrationLockTimeout, err)
}
select {
case <-ctx.Done():
return fmt.Errorf("%s: %w", label, ctx.Err())
case <-time.After(backoff):
}
- if backoff *= 2; backoff > migrationLockBackoffMax {
- backoff = migrationLockBackoffMax
- }
+ backoff = min(backoff*2, migrationLockBackoffMax)
}
}
diff --git a/internal/generator/templates/store_schema_version_test.go.tmpl b/internal/generator/templates/store_schema_version_test.go.tmpl
index acac455d..219d0ea2 100644
--- a/internal/generator/templates/store_schema_version_test.go.tmpl
+++ b/internal/generator/templates/store_schema_version_test.go.tmpl
@@ -4,10 +4,13 @@
package store
import (
+ "context"
"database/sql"
+ "errors"
"path/filepath"
"sync"
"testing"
+ "time"
_ "modernc.org/sqlite"
)
@@ -125,6 +128,105 @@ func TestMigrate_ConcurrentFreshDB(t *testing.T) {
}
}
+// holdWriteLock takes an exclusive write lock on dbPath that a peer's
+// BEGIN IMMEDIATE cannot acquire until the returned cleanup runs. Used
+// to construct contention scenarios in the migration tests.
+func holdWriteLock(t *testing.T, dbPath string) (cleanup func()) {
+ t.Helper()
+ holder, err := sql.Open("sqlite", dbPath+"?_journal_mode=WAL&_busy_timeout=5000")
+ if err != nil {
+ t.Fatalf("open holder: %v", err)
+ }
+ htx, err := holder.Begin()
+ if err != nil {
+ _ = holder.Close()
+ t.Fatalf("begin holder tx: %v", err)
+ }
+ if _, err := htx.Exec(`CREATE TABLE IF NOT EXISTS holder_lock (id INTEGER)`); err != nil {
+ _ = htx.Rollback()
+ _ = holder.Close()
+ t.Fatalf("seed holder write: %v", err)
+ }
+ return func() {
+ _ = htx.Rollback()
+ _ = holder.Close()
+ }
+}
+
+// TestOpenWithContext_RespectsCancellation verifies that a caller that
+// cancels its context during a stalled migration sees the cancellation
+// surface as the returned error within a short window, instead of
+// having to wait out the full migrationLockTimeout. SIGINT in a Cobra
+// command's context must interrupt store.Open, not just block on it.
+func TestOpenWithContext_RespectsCancellation(t *testing.T) {
+ t.Parallel()
+
+ dbPath := filepath.Join(t.TempDir(), "data.db")
+ defer holdWriteLock(t, dbPath)()
+
+ // Pre-cancel the context. The migration's BEGIN IMMEDIATE will BUSY
+ // against the holder; the very first iteration of retryOnBusy then
+ // hits the ctx.Done() arm of its select and propagates ctx.Canceled.
+ // A blocked-then-cancel pattern using time.Sleep would prove the
+ // same property but cost the sleep interval on every CI run.
+ ctx, cancel := context.WithCancel(context.Background())
+ cancel()
+
+ start := time.Now()
+ _, err := OpenWithContext(ctx, dbPath)
+ elapsed := time.Since(start)
+
+ if err == nil {
+ t.Fatalf("expected OpenWithContext to fail under contention with cancelled ctx")
+ }
+ if !errors.Is(err, context.Canceled) {
+ t.Fatalf("expected context.Canceled in error chain, got: %v", err)
+ }
+ // Without ctx threading this would block until migrationLockTimeout
+ // (default 30s). 5s is generous headroom over the actual return
+ // time (microseconds for a pre-cancelled ctx) without flaking CI.
+ if elapsed > 5*time.Second {
+ t.Fatalf("OpenWithContext returned after %s; pre-cancelled ctx should short-circuit immediately", elapsed)
+ }
+}
+
+// TestMigrate_RejectsNewerDBImmediately verifies that an old binary
+// opening a newer-schema DB rejects fast even when a peer migrator is
+// still holding the write lock. The schema-version check runs on the
+// pinned connection BEFORE BEGIN IMMEDIATE so the rejection path
+// doesn't have to wait out the migration lock.
+func TestMigrate_RejectsNewerDBImmediately(t *testing.T) {
+ t.Parallel()
+
+ dbPath := filepath.Join(t.TempDir(), "data.db")
+
+ // Pre-stamp the DB at a version this binary doesn't support.
+ raw, err := sql.Open("sqlite", dbPath)
+ if err != nil {
+ t.Fatalf("open raw: %v", err)
+ }
+ if _, err := raw.Exec(`PRAGMA user_version = 999`); err != nil {
+ t.Fatalf("stamp future version: %v", err)
+ }
+ raw.Close()
+
+ defer holdWriteLock(t, dbPath)()
+
+ start := time.Now()
+ _, err = Open(dbPath)
+ elapsed := time.Since(start)
+
+ if err == nil {
+ t.Fatalf("expected Open to refuse a newer-schema DB")
+ }
+ // The fast-path goal: rejection must arrive well under
+ // migrationLockTimeout. 5s leaves headroom over the WAL init race
+ // (a few ms in practice) without being so tight CI flakes.
+ if elapsed > 5*time.Second {
+ t.Fatalf("Open rejected after %s; fast-path should reject in well under migrationLockTimeout (30s)", elapsed)
+ }
+}
+
// TestSchemaVersion_ReopenIsIdempotent verifies that opening an already
// correctly-stamped DB is a no-op — the second open reads the version
// and the migrations are all idempotent (CREATE TABLE IF NOT EXISTS).
diff --git a/internal/generator/templates/sync.go.tmpl b/internal/generator/templates/sync.go.tmpl
index b87d3c09..6960816e 100644
--- a/internal/generator/templates/sync.go.tmpl
+++ b/internal/generator/templates/sync.go.tmpl
@@ -92,7 +92,7 @@ Exit codes & warnings:
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- db, err := store.Open(dbPath)
+ db, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening local database: %w", err)
}
diff --git a/internal/generator/templates/workflows/pm_load.go.tmpl b/internal/generator/templates/workflows/pm_load.go.tmpl
index 7cc7a0a8..68befc5a 100644
--- a/internal/generator/templates/workflows/pm_load.go.tmpl
+++ b/internal/generator/templates/workflows/pm_load.go.tmpl
@@ -34,7 +34,7 @@ person. Helps identify overloaded team members and unbalanced workload.`,
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- db, err := store.Open(dbPath)
+ db, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening local database: %w\nRun '{{.Name}}-pp-cli sync' first.", err)
}
diff --git a/internal/generator/templates/workflows/pm_orphans.go.tmpl b/internal/generator/templates/workflows/pm_orphans.go.tmpl
index a8acd381..6d453fe7 100644
--- a/internal/generator/templates/workflows/pm_orphans.go.tmpl
+++ b/internal/generator/templates/workflows/pm_orphans.go.tmpl
@@ -31,7 +31,7 @@ such as assignee, project, priority, or labels. Useful for triaging unowned work
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- db, err := store.Open(dbPath)
+ db, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening local database: %w\nRun '{{.Name}}-pp-cli sync' first.", err)
}
diff --git a/internal/generator/templates/workflows/pm_stale.go.tmpl b/internal/generator/templates/workflows/pm_stale.go.tmpl
index da5be4eb..35157264 100644
--- a/internal/generator/templates/workflows/pm_stale.go.tmpl
+++ b/internal/generator/templates/workflows/pm_stale.go.tmpl
@@ -39,7 +39,7 @@ the specified number of days. Useful for identifying forgotten or blocked work.`
dbPath = defaultDBPath("{{.Name}}-pp-cli")
}
- db, err := store.Open(dbPath)
+ db, err := store.OpenWithContext(cmd.Context(), dbPath)
if err != nil {
return fmt.Errorf("opening local database: %w\nRun '{{.Name}}-pp-cli sync' first.", err)
}
← 9a135062 feat(cli): WU-2 sync correctness pass (pagination, ID extrac
·
back to Cli Printing Press
·
fix(cli): cliutil.BuildPath replaces buildProxyPath with pro d9472b76 →