tommy0103andClaude Opus 4.8
e3e61cc7ab
Implement the full ADR-0006 plan: three-layer separation of transaction
correctness, retry policy, and cross-process writer coordination.
Layer 1 — scripts/tx.ts (transaction correctness):
- runWriteTransaction executes work exactly once; no internal retry.
- BEGIN IMMEDIATE takes the write lock up front (avoids SQLITE_BUSY_SNAPSHOT).
- Guarded rollback: checks inTransaction() via adapter before attempting
ROLLBACK; never masks the primary exception.
- WriteTxDiagnostics attached to errors: phase, code, label,
rollbackSucceeded, rollbackError, transactionActive.
- Binding adapters (betterSqliteTransactionAdapter, nodeSqliteTransactionAdapter)
mapping better-sqlite3's `.inTransaction` and node:sqlite's `.isTransaction`.
- configureConnection centralizes WAL + synchronous + busy_timeout.
Layer 2 — scripts/write-coordinator.ts (retry policy):
- runRetryableWriteTransaction: bounded retry with total time budget.
- Only retries when the transaction confirmed ended (transactionActive=false)
and the error is SQLITE_BUSY during work/commit phase.
- BEGIN-phase BUSY = abort entire build (isBeginBusyFailure); the caller
returns `{ deferred: true, reason: 'writer_busy' }` instead of waiting.
- hasUnusableTransaction detects a still-active transaction after failure;
aborts the build immediately, never retries.
Layer 3 — scripts/writer-lease.ts (cross-process coordination):
- acquireWriterLease: dedicated writer.lock.sqlite with busy_timeout=0 +
BEGIN IMMEDIATE. Non-blocking attempt; bounded wait with retryDelayMs.
- writerLockPathFor derives lock path from the target DB path.
- Lease held for the entire build; released on completion or failure.
- Lock DB uses DELETE journal (not WAL); crash/close auto-releases.
- All consumers obey: skill acquires at build start (returns deferred if
unavailable); app daemon (via worker) acquires for its build cycle.
Build semantics changes:
- affectedSessionIds updated only after successful commit.
- BuildIndexResult gains skipped/skippedFiles for observability.
- Skill finalize failure now fails the build (was silently warned).
- Checkpoint changed to PASSIVE (TRUNCATE reserved for maintenance/exit).
- Skill buildIndex returns { deferred, reason } on lease contention;
indexer-service reschedules the build (deferredRetryMs) without publishing
a heartbeat (so the build-deferred state is visible to cross-process
arbitration).
- Service publishes heartbeat immediately on start() for correct arbitration.
Tests:
- tests/write-transaction.test.mjs: single-shot execution, diagnostics
propagation, auto-rolled-back transaction detected, rollback failure
captured as metadata, BEGIN IMMEDIATE semantics.
- tests/writer-lease.test.mjs: acquire/release, contention returns null,
bounded wait with release during budget.
- tests/app-writer-lease.test.mjs: better-sqlite3 adapter integration.
- tests/app-rollback-guard.test.mjs: rewritten — transient BUSY recovered
by coordinator, persistent BUSY skips file, begin-busy aborts build,
live-transaction aborts build, phantom affectedSessionIds prevented.
- tests/daemon-arbitration.test.mjs: skill defers to fresh app heartbeat,
builds when heartbeat is stale.
- tests/app-indexer-service.test.mjs: new cases for deferred-retry
scheduling and immediate heartbeat on start.
- app/tests/electron-concurrency.mjs + child: dual-child IPC structure for
real better-sqlite3 contention (holder acquires lock → build child starts
→ delayed release → result collected; persistent contention bounded).
ADR-0006 updated to reflect the implemented design.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
76 lines
4.8 KiB
Markdown
76 lines
4.8 KiB
Markdown
# Write-transaction rollback safety and SQLite concurrency
|
|
|
|
**Context.** The app surfaced `Obelisk index build failed: cannot rollback - no
|
|
transaction is active`. That text was a secondary cleanup failure. SQLite had
|
|
already ended the transaction, then the catch block's unguarded `ROLLBACK`
|
|
threw over the primary exception and turned a skippable per-file failure into a
|
|
whole-build failure. The masked exception was not preserved, so contention
|
|
(`SQLITE_BUSY` / `SQLITE_BUSY_SNAPSHOT`) is the leading explanation rather than
|
|
a proven historical fact. It is plausible because daemon builds, manual
|
|
rebuilds, skill passive-pull indexing, heartbeat writes, and reads share one WAL
|
|
database.
|
|
|
|
`busy_timeout` alone is not a correctness fix. In particular,
|
|
`SQLITE_BUSY_SNAPSHOT` is not made safe by waiting longer, and retrying only the
|
|
failed statement can replay part of a transaction.
|
|
|
|
**Decision.** Use one transaction primitive plus two explicit coordination
|
|
layers.
|
|
|
|
- `scripts/tx.ts` owns the binding-agnostic `runWriteTransaction(db, work)`.
|
|
Adapters expose transaction state from better-sqlite3's `inTransaction` and
|
|
node:sqlite's `isTransaction`. The primitive performs `BEGIN IMMEDIATE`, runs
|
|
`work` exactly once, commits, and attempts rollback only when the binding says
|
|
a transaction is active or its state is unknown. Cleanup never masks the
|
|
primary exception. Diagnostics record phase, SQLite code, rollback outcome,
|
|
transaction state, label, and attempts.
|
|
- Retry is an upper-layer policy in `scripts/write-coordinator.ts`, never hidden
|
|
inside the transaction primitive. Only an idempotent whole transaction that
|
|
failed during work/commit with `SQLITE_BUSY*` and is confirmed inactive may be
|
|
retried. The default is three attempts within a one-second budget with short
|
|
backoff. BEGIN contention is deferred to the build scheduler; an active or
|
|
unknown post-error transaction aborts the build.
|
|
- Per-file failures remain warnings and are reported in `skippedFiles`; finalize
|
|
failures propagate. `affectedSessionIds` is updated only after the relevant
|
|
commit. Force cleanup is one atomic, retryable transaction, and finalize is
|
|
likewise retried as a complete idempotent transaction.
|
|
- A fresh `__app_heartbeat__` is policy ownership: while it is fresh, the skill
|
|
opens no write connection and performs no migration, schema setup, checkpoint,
|
|
index build, or `attune`. `__app_last_successful_build__` remains an
|
|
observability/freshness marker and is not required for ownership. The skill
|
|
checks ownership again after acquiring the hard lease to close the TOCTOU
|
|
window. Search/query connections are read-only.
|
|
- A dedicated `.obelisk/writer.lock.sqlite` provides the cross-process safety
|
|
mutex on every platform. Acquisition is `BEGIN IMMEDIATE` with non-blocking or
|
|
bounded waiting; release is idempotent. App builds and heartbeats, skill builds
|
|
and attune, app schema/legacy migrations and memory mutations, and manual
|
|
rebuild all participate. Manual rebuild's main process owns the lease across
|
|
worker build, atomic target replacement, and database reopen; the worker uses
|
|
the explicit `caller-held` mode.
|
|
- The app's in-process indexer service permits one build at a time. A lease
|
|
deferral retains changed paths and schedules a short retry without announcing
|
|
a successful build. Service start publishes the ownership heartbeat
|
|
immediately, then refreshes it periodically.
|
|
- Index-writer and skill read connections use an explicit 250 ms SQLite busy
|
|
timeout inside the larger bounded coordination budget. The long-lived app
|
|
query connection retains a 5 s timeout; heartbeat is deliberately non-blocking
|
|
(`0 ms`) so it never stalls the Electron main thread. Builds use
|
|
`BEGIN IMMEDIATE`. Routine checkpointing is `PASSIVE`; blocking `TRUNCATE` is
|
|
reserved for explicit maintenance.
|
|
|
|
**Verification.** Fast tests inject auto-rollback and BUSY failures to prove the
|
|
primary error is preserved, retry replays the whole transaction, persistent
|
|
per-file failure is skipped, force cleanup is atomic, and affected-session state
|
|
is commit-aware. The Electron harness uses real Electron-ABI better-sqlite3 and
|
|
two child processes: one holds the SQLite writer lease until signalled, while
|
|
the other runs synchronous `buildIndex`. It verifies both release-within-budget
|
|
success and bounded `writer_busy` deferral. Separate arbitration tests prove a
|
|
heartbeat-only daemon marker keeps query and attune paths read-only.
|
|
|
|
**Consequences.** Heartbeat and lease have deliberately different jobs: the
|
|
heartbeat decides who should write, while the lease guarantees writers cannot
|
|
overlap when policy information races or is stale. A single bad transcript can
|
|
still be skipped so the index self-heals on a later build; structural/finalize
|
|
failures remain visible. Longer timeouts must not replace the transaction and
|
|
ownership rules recorded here.
|