diff --git a/app/src/main/indexer.ts b/app/src/main/indexer.ts index fed07f1..d767952 100644 --- a/app/src/main/indexer.ts +++ b/app/src/main/indexer.ts @@ -35,6 +35,15 @@ interface FileInfo { source?: string; } +// A rollback inside a catch must never throw over the real error. SQLite +// auto-rolls back certain failures (SQLITE_BUSY, disk full, ...); a following +// explicit ROLLBACK then throws "cannot rollback - no transaction is active", +// which would both mask the true cause and turn a skippable per-file error into +// a whole-build failure. Swallow only the rollback's own error. +function safeRollback(db: { exec: (sql: string) => unknown }) { + try { db.exec('ROLLBACK'); } catch { /* no active transaction */ } +} + function resolveSchemaPath() { const candidates = [ path.join(__dirname, 'schema.sql'), @@ -576,7 +585,7 @@ function buildIndex({ if (file.source !== 'codex') indexSubagentMeta(db, file); db.exec('COMMIT'); } catch (error) { - db.exec('ROLLBACK'); + safeRollback(db); console.warn(`Warning: failed to index ${file.path}: ${(error as Error).message}`); } } @@ -596,7 +605,7 @@ function buildIndex({ if (latestSourceMtime) writeIndexMarker(db, '__last_source_mtime__', latestSourceMtime); db.exec('COMMIT'); } catch (error) { - db.exec('ROLLBACK'); + safeRollback(db); throw error; } return { files: files.length, latestSourceMtime, affectedSessionIds: [...affectedSessionIds], ftsRebuilt }; diff --git a/docs/adr/0006-write-transaction-rollback-and-concurrency.md b/docs/adr/0006-write-transaction-rollback-and-concurrency.md new file mode 100644 index 0000000..41bff9a --- /dev/null +++ b/docs/adr/0006-write-transaction-rollback-and-concurrency.md @@ -0,0 +1,76 @@ +# Write-transaction rollback safety and SQLite concurrency + +**Context.** The app surfaced `Obelisk index build failed: cannot rollback - no +transaction is active`. That message is a *secondary* error: SQLite auto-rolls +back certain failures (notably `SQLITE_BUSY` / `SQLITE_BUSY_SNAPSHOT`, also disk +full), after which the per-file build loop's unguarded `db.exec('ROLLBACK')` in +its `catch` threw over the real error and aborted the whole build instead of +skipping just the offending file. The underlying trigger is concurrency: the app +runs a daemon indexer, manual rebuilds, and read queries against one WAL +database, and the skill's passive-pull build can write the same database from a +separate process. + +`busy_timeout` is **not** the root-cause fix and must not be treated as one. +better-sqlite3's constructor already defaults `timeout` to 5000ms, so the app hit +`SQLITE_BUSY` *despite* a 5s wait — which points at `SQLITE_BUSY_SNAPSHOT` from +deferred (read-then-write) transactions, a snapshot conflict that `busy_timeout` +does not wait on. Only `BEGIN IMMEDIATE` plus whole-transaction retry addresses +that. + +**Decision.** Split the work into a cheap stopgap now and a correctness/ +concurrency fix later. + +*Stopgap (done).* Both indexers use a guarded rollback (`safeRollback`): a +cleanup rollback swallows only its own error and never masks the primary one. +Per-file failures are logged and the build continues; the finalize failure still +propagates. The skill's connection (`node:sqlite`, which has **no** default busy +timeout) gets an explicit `PRAGMA busy_timeout = 5000`; the app adds no such +pragma because better-sqlite3 already defaults to 5000ms — adding it there was +redundant and removed, precisely so nobody reads it as "the fix". + +*Planned full fix (deferred, two phases).* + +Phase 1 — transaction semantics: +- A shared, binding-agnostic `runWriteTransaction(db, work)` (same injection + model as `persist`): BEGIN → work → COMMIT, guarded rollback on failure, + original exception always preserved. Both app and skill call it, so their + behaviour is identical. +- Per-file callers catch and continue; finalize does **not** swallow — a finalize + failure fails the build (the skill currently only warns; that changes). +- In-memory state such as `affectedSessionIds` is updated **only after** a + successful COMMIT (today the app adds the id before COMMIT, so a failed commit + can report a wrong affected set). +- Structured diagnostics: `phase` (begin/file-write/commit/finalize/checkpoint), + SQLite `code`, file path, whether rollback succeeded, whether a txn is still + active; surface the skipped-file count in the build result rather than only + `console.warn` (no silent coverage gaps). + +Phase 2 — concurrency: +- A stable concurrency test against real Electron `better-sqlite3` (daemon + + rebuild + skill writer), not an injected fake BUSY. Note this cannot run under + standalone `node --test` (better-sqlite3 is Electron-ABI); it needs an + Electron-hosted harness. The fast injected-BUSY unit test is kept as a + lower-level guard for `runWriteTransaction`, alongside it. +- Serialize all index writes through a single writer: an in-process + `BuildCoordinator`, plus the existing cross-process daemon arbitration + (`__app_heartbeat__` markers) so the skill defers to a live daemon. Single + writer = both layers together. +- `BEGIN IMMEDIATE` to take the write lock up front and avoid + `SQLITE_BUSY_SNAPSHOT` on read-then-write. +- Bounded, short-backoff retry of the **whole** transaction (not the single + failed statement) on `SQLITE_BUSY` / `SQLITE_BUSY_SNAPSHOT`. This relies on the + per-file work being idempotent (upsert/replace + delete-session cascade), which + it is — an invariant the retry depends on. +- Stop forcing `wal_checkpoint(TRUNCATE)` after every build; prefer a `PASSIVE` + checkpoint on idle, with `TRUNCATE` reserved for maintenance/exit. +- Centralized connection configuration (explicit 5000ms for `node:sqlite` — a + real behaviour change; explicit for better-sqlite3 too, acknowledging its + default is already 5000ms). + +**Consequences.** With only the stopgap in place, contention no longer crashes a +build, but a conflicting file is *skipped* (non-fatal) and its data is briefly +missing until the next build — acceptable as a stopgap because the guard keeps +the index consistent and self-healing. The true fix is tracked as the two-phase +plan above. A future contributor should not "fix" the concurrency by bumping or +re-adding `busy_timeout`; the direction is a shared transaction module, single +writer, `BEGIN IMMEDIATE`, and whole-transaction retry. diff --git a/scripts/db.mjs b/scripts/db.mjs index 721232c..4d175cc 100644 --- a/scripts/db.mjs +++ b/scripts/db.mjs @@ -24,6 +24,9 @@ function openDb() { const db = new DatabaseSync(DB_PATH); db.exec('PRAGMA journal_mode=WAL'); db.exec('PRAGMA synchronous=NORMAL'); + // Wait for a contended lock instead of erroring with SQLITE_BUSY: a running + // app daemon may be writing the same database while the skill reads/indexes. + db.exec('PRAGMA busy_timeout=5000'); migrateExistingColumns(db); db.exec(SCHEMA); migrateDb(db); diff --git a/scripts/indexer.mjs b/scripts/indexer.mjs index 56e60e3..b8f8d89 100644 --- a/scripts/indexer.mjs +++ b/scripts/indexer.mjs @@ -9,6 +9,15 @@ import { parse as codexParse } from './providers/codex.ts'; const HISTORY_PATH = path.join(CLAUDE_DIR, 'history.jsonl'); +// A rollback inside a catch must never throw over the real error. SQLite +// auto-rolls back certain failures (SQLITE_BUSY, disk full, ...); a following +// explicit ROLLBACK then throws "cannot rollback - no transaction is active", +// which would both mask the true cause and turn a skippable per-file error into +// a whole-build failure. Swallow only the rollback's own error. +function safeRollback(db) { + try { db.exec('ROLLBACK'); } catch { /* no active transaction */ } +} + function needsReindex(db, fp) { const mt = fs.statSync(fp).mtimeMs; @@ -193,7 +202,7 @@ function buildIndex({ force = false } = {}) { } db.exec('COMMIT'); } catch (e) { - db.exec('ROLLBACK'); + safeRollback(db); process.stderr.write(`Warning: failed to index ${f.path}: ${e.message}\n`); } } @@ -208,7 +217,7 @@ function buildIndex({ force = false } = {}) { db.prepare("INSERT OR REPLACE INTO index_state (jsonl_path, mtime, lines_processed) VALUES ('__last_build__', ?, 0)").run(Date.now()); db.exec('COMMIT'); } catch (e) { - db.exec('ROLLBACK'); + safeRollback(db); process.stderr.write(`Warning: failed to finalize index: ${e.message}\n`); } db.close(); diff --git a/tests/app-rollback-guard.test.mjs b/tests/app-rollback-guard.test.mjs new file mode 100644 index 0000000..5fa74cd --- /dev/null +++ b/tests/app-rollback-guard.test.mjs @@ -0,0 +1,92 @@ +// Regression test for the "cannot rollback - no transaction is active" crash. +// SQLite auto-rolls back certain failures (SQLITE_BUSY, disk full, ...). The +// per-file build loop then ran an explicit ROLLBACK in its catch, which threw a +// SECOND error over the real one and aborted the whole build instead of skipping +// just the bad file. buildIndex now uses a guarded rollback; this test injects a +// DB that faithfully reproduces the condition and asserts the build survives. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { createRequire } from 'node:module'; +import { mkdtempSync, mkdirSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +const require = createRequire(import.meta.url); +import { buildIndex } from '../app/src/main/indexer.ts'; +const { DatabaseSync } = require('node:sqlite'); + +// Wraps node:sqlite and simulates SQLite's auto-rollback-on-error: the first +// write inside a transaction throws a BUSY-like error AND ends the real +// transaction, so a following explicit ROLLBACK errors with "no transaction". +class AutoRollbackOnceDatabase { + constructor(dbPath) { + this.db = new DatabaseSync(dbPath); + this.inTxn = false; + this.fired = false; + } + pragma(statement) { this.db.exec(`PRAGMA ${statement}`); } + exec(sql) { + const head = sql.trim().slice(0, 8).toUpperCase(); + if (head.startsWith('BEGIN')) { this.inTxn = true; return this.db.exec('BEGIN'); } + if (head.startsWith('COMMIT')) { this.inTxn = false; return this.db.exec('COMMIT'); } + if (head.startsWith('ROLLBACK')) { + if (!this.inTxn) throw new Error('cannot rollback - no transaction is active'); + this.inTxn = false; + return this.db.exec('ROLLBACK'); + } + return this.db.exec(sql); + } + prepare(sql) { + const stmt = this.db.prepare(sql); + const self = this; + return { + get: (...args) => stmt.get(...args), + all: (...args) => stmt.all(...args), + run: (...args) => { + if (!self.fired && self.inTxn) { + self.fired = true; + self.db.exec('ROLLBACK'); // SQLite auto-rolled the txn back on the error + self.inTxn = false; + throw new Error('SQLITE_BUSY: database is locked'); + } + return stmt.run(...args); + }, + }; + } + close() { return this.db.close(); } +} + +test('a per-file write that auto-rolls-back the transaction is skipped, not fatal', () => { + const home = mkdtempSync(join(tmpdir(), 'obelisk-rollback-guard-')); + const projectDir = join(home, '.claude', 'projects', '-tmp-proj'); + mkdirSync(projectDir, { recursive: true }); + const msg = (uuid) => JSON.stringify({ + uuid, type: 'user', timestamp: '2026-06-10T10:00:00Z', cwd: '/tmp/proj', + message: { role: 'user', content: `hello ${uuid}` }, + }) + '\n'; + writeFileSync(join(projectDir, 'alpha.jsonl'), msg('a1')); + writeFileSync(join(projectDir, 'beta.jsonl'), msg('b1')); + + const dbPath = join(home, '.obelisk', 'obelisk.sqlite'); + mkdirSync(join(home, '.obelisk'), { recursive: true }); + + // The unguarded ROLLBACK used to make this throw the masking rollback error. + let result; + assert.doesNotThrow(() => { + result = buildIndex({ + force: true, + claudeDir: join(home, '.claude'), + codexDir: join(home, '.codex'), + projectsDir: join(home, '.claude', 'projects'), + dbPath, + DatabaseImpl: AutoRollbackOnceDatabase, + }); + }, 'build must not abort on a transaction-aborting per-file error'); + + // Exactly one file's write was poisoned; the other indexed cleanly. + const check = new DatabaseSync(dbPath); + const sessions = check.prepare('SELECT COUNT(*) AS c FROM sessions').get().c; + check.close(); + assert.equal(sessions, 1, 'the surviving file is indexed; the poisoned one is skipped'); + assert.ok(result.files >= 2, 'both files were discovered'); +});