From 69835623bdd4096e6add55788e725c29ceb55fb7 Mon Sep 17 00:00:00 2001 From: Dan Shipper Date: Sat, 29 Aug 2026 12:23:57 -0400 Subject: [PATCH] fix(workspace-feeds): surface real sqlite errors and skip no-op boot writes Tend boots logged `SQLiteError: cannot rollback - no transaction is active` from SqliteWorkspaceFeedRepository.setFeedIds (4x in one tend-live restart) and the api only came up on a later attempt. Two problems stacked: 1. setFeedIds rolled back by hand inside a catch. When a statement inside the transaction fails with an error SQLite auto-rolls-back on (BUSY, FULL, IOERR), the explicit ROLLBACK then throws and masks the real error. Use db.transaction(...).immediate() like the rest of sqlite.ts; it only issues ROLLBACK while a transaction is still open. 2. MirroredWorkspaceFeedRepository.init unconditionally rewrote workspace_feeds in sqlite and the json mirror on every boot, and every CLI command boots the same store, so a read-only `state` call took a write lock and a busy database made boot fatal. Only write when the two copies actually disagree. Tests cover the masked-error path (injected post-rollback failure), a BEGIN IMMEDIATE lock conflict, a no-op consistent boot (created_at and mirror untouched), and a real merge when the copies disagree. Co-Authored-By: Claude --- server/repositories/workspaceFeeds.ts | 13 +- server/sqlite.ts | 15 +- test/workspace-feeds.test.ts | 191 ++++++++++++++++++++++++++ 3 files changed, 208 insertions(+), 11 deletions(-) create mode 100644 test/workspace-feeds.test.ts diff --git a/server/repositories/workspaceFeeds.ts b/server/repositories/workspaceFeeds.ts index 77f2e18e..b4b7ac8f 100644 --- a/server/repositories/workspaceFeeds.ts +++ b/server/repositories/workspaceFeeds.ts @@ -63,9 +63,12 @@ export class MirroredWorkspaceFeedRepository implements WorkspaceFeedRepository await this.mirror.init(defaultFeedIds); const mirrorIds = await this.mirror.listFeedIds(); await this.primary.init(mirrorIds.length ? mirrorIds : defaultFeedIds); - const merged = unique([...(await this.primary.listFeedIds()), ...mirrorIds]); - await this.primary.setFeedIds(merged); - await this.mirror.setFeedIds(merged); + const primaryIds = await this.primary.listFeedIds(); + const merged = unique([...primaryIds, ...mirrorIds]); + // Every api boot and CLI command runs init, so only take a write lock when the + // two copies actually disagree; a busy database must not fail a no-op boot. + if (!sameFeedIds(primaryIds, merged)) await this.primary.setFeedIds(merged); + if (!sameFeedIds(mirrorIds, merged)) await this.mirror.setFeedIds(merged); } listFeedIds(): Promise { @@ -92,3 +95,7 @@ export class MirroredWorkspaceFeedRepository implements WorkspaceFeedRepository function unique(values: string[]): string[] { return Array.from(new Set(values)); } + +function sameFeedIds(left: string[], right: string[]): boolean { + return left.length === right.length && left.every((feedId, index) => feedId === right[index]); +} diff --git a/server/sqlite.ts b/server/sqlite.ts index dae6e093..aca877aa 100644 --- a/server/sqlite.ts +++ b/server/sqlite.ts @@ -961,7 +961,7 @@ class SqliteFeedEventRepository implements FeedEventRepository { } } -class SqliteWorkspaceFeedRepository implements WorkspaceFeedRepository { +export class SqliteWorkspaceFeedRepository implements WorkspaceFeedRepository { constructor(private readonly database: () => Database) {} async init(defaultFeedIds: string[]): Promise { @@ -977,16 +977,15 @@ class SqliteWorkspaceFeedRepository implements WorkspaceFeedRepository { async setFeedIds(feedIds: string[]): Promise { const db = this.database(); const now = new Date().toISOString(); - db.exec("BEGIN IMMEDIATE"); - try { + // db.transaction only issues ROLLBACK while a transaction is still open, so a + // statement that fails after SQLite has already rolled back on its own + // (SQLITE_BUSY, SQLITE_FULL, SQLITE_IOERR) surfaces its real error instead of + // "cannot rollback - no transaction is active". + db.transaction(() => { db.query("DELETE FROM workspace_feeds").run(); const insert = db.query("INSERT INTO workspace_feeds (feed_id, position, created_at) VALUES (?, ?, ?)"); unique(feedIds).forEach((feedId, index) => insert.run(feedId, index, now)); - db.exec("COMMIT"); - } catch (error) { - db.exec("ROLLBACK"); - throw error; - } + }).immediate(); } async addFeedId(feedId: string): Promise { diff --git a/test/workspace-feeds.test.ts b/test/workspace-feeds.test.ts new file mode 100644 index 00000000..0e77c79a --- /dev/null +++ b/test/workspace-feeds.test.ts @@ -0,0 +1,191 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { Database } from "bun:sqlite"; +import { mkdtempSync } from "node:fs"; +import { rm } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { + FileWorkspaceFeedRepository, + MirroredWorkspaceFeedRepository, + type WorkspaceFeedRepository, +} from "../server/repositories/workspaceFeeds"; +import { LocalSqliteStore, SqliteWorkspaceFeedRepository } from "../server/sqlite"; + +const roots: string[] = []; +const stores: LocalSqliteStore[] = []; +const connections: Database[] = []; + +function tempRoot(): string { + const root = mkdtempSync(path.join(os.tmpdir(), "attention-workspace-feeds-")); + roots.push(root); + return root; +} + +async function sqliteStore(root: string): Promise { + const sqlite = new LocalSqliteStore(path.join(root, "attention.db")); + await sqlite.init(); + stores.push(sqlite); + return sqlite; +} + +/** A raw connection with SQLite's default busy_timeout of 0, so lock conflicts fail immediately. */ +function connect(dbPath: string): Database { + const db = new Database(dbPath); + connections.push(db); + return db; +} + +afterEach(async () => { + for (const db of connections.splice(0)) { + if (db.inTransaction) db.exec("ROLLBACK"); + db.close(); + } + for (const sqlite of stores.splice(0)) sqlite.close(); + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))); +}); + +function busyError(): Error { + return Object.assign(new Error("database is locked"), { code: "SQLITE_BUSY", errno: 5 }); +} + +/** + * Wraps a connection so the DELETE inside setFeedIds behaves like a statement that hit + * SQLITE_BUSY mid-transaction: SQLite has already rolled the transaction back by the time + * the error reaches application code. + */ +function failingDeleteConnection(db: Database): Database { + return new Proxy(db, { + get(target, property) { + if (property === "query") { + return (sql: string) => { + if (!sql.startsWith("DELETE FROM workspace_feeds")) return target.query(sql); + return { + run() { + target.exec("ROLLBACK"); + throw busyError(); + }, + }; + }; + } + const value = Reflect.get(target, property, target); + return typeof value === "function" ? value.bind(target) : value; + }, + }); +} + +/** Delegates to a repository while recording every write, so a test can assert a boot stayed read-only. */ +class RecordingWorkspaceFeedRepository implements WorkspaceFeedRepository { + readonly writes: string[] = []; + + constructor(private readonly inner: WorkspaceFeedRepository) {} + + init(defaultFeedIds: string[]): Promise { + return this.inner.init(defaultFeedIds); + } + + listFeedIds(): Promise { + return this.inner.listFeedIds(); + } + + setFeedIds(feedIds: string[]): Promise { + this.writes.push(`set:${feedIds.join(",")}`); + return this.inner.setFeedIds(feedIds); + } + + addFeedId(feedId: string): Promise { + this.writes.push(`add:${feedId}`); + return this.inner.addFeedId(feedId); + } + + removeFeedId(feedId: string): Promise { + this.writes.push(`remove:${feedId}`); + return this.inner.removeFeedId(feedId); + } +} + +function feedRows(db: Database): Array<{ feed_id: string; position: number; created_at: string }> { + return db + .query("SELECT feed_id, position, created_at FROM workspace_feeds ORDER BY position ASC") + .all() as Array<{ feed_id: string; position: number; created_at: string }>; +} + +async function rejection(promise: Promise): Promise { + const failure = await promise.then(() => null, (error: unknown) => error); + expect(failure).toBeInstanceOf(Error); + return failure as Error & { code?: string }; +} + +describe("SqliteWorkspaceFeedRepository.setFeedIds", () => { + test("surfaces the statement's own error when SQLite already rolled the transaction back", async () => { + const root = tempRoot(); + const sqlite = await sqliteStore(root); + const db = connect(sqlite.dbPath); + await new SqliteWorkspaceFeedRepository(() => db).setFeedIds(["inbox"]); + + const repository = new SqliteWorkspaceFeedRepository(() => failingDeleteConnection(db)); + const failure = await rejection(repository.setFeedIds(["inbox", "ffo"])); + + expect(failure.code).toBe("SQLITE_BUSY"); + expect(failure.message).toBe("database is locked"); + expect(db.inTransaction).toBe(false); + expect(feedRows(db).map((row) => row.feed_id)).toEqual(["inbox"]); + }); + + test("surfaces SQLITE_BUSY when another connection holds the write lock", async () => { + const root = tempRoot(); + const sqlite = await sqliteStore(root); + const db = connect(sqlite.dbPath); + const repository = new SqliteWorkspaceFeedRepository(() => db); + await repository.setFeedIds(["inbox"]); + + const holder = connect(sqlite.dbPath); + holder.exec("BEGIN IMMEDIATE"); + const failure = await rejection(repository.setFeedIds(["inbox", "ffo"])); + expect(failure.code).toBe("SQLITE_BUSY"); + expect(failure.message).not.toContain("cannot rollback"); + expect(db.inTransaction).toBe(false); + + holder.exec("ROLLBACK"); + expect(feedRows(db).map((row) => row.feed_id)).toEqual(["inbox"]); + await repository.setFeedIds(["inbox", "ffo"]); + expect(feedRows(db).map((row) => row.feed_id)).toEqual(["inbox", "ffo"]); + }); +}); + +describe("MirroredWorkspaceFeedRepository.init", () => { + test("does not write when sqlite and the mirror already agree", async () => { + const root = tempRoot(); + const sqlite = await sqliteStore(root); + const mirrorPath = path.join(root, "workspace.json"); + const firstBoot = new MirroredWorkspaceFeedRepository(sqlite.workspaceFeeds(), new FileWorkspaceFeedRepository(mirrorPath)); + await firstBoot.init(["inbox", "ffo"]); + + const db = connect(sqlite.dbPath); + db.query("UPDATE workspace_feeds SET created_at = ?").run("2000-01-01T00:00:00.000Z"); + const before = feedRows(db); + expect(before.map((row) => row.feed_id)).toEqual(["inbox", "ffo"]); + + const primary = new RecordingWorkspaceFeedRepository(sqlite.workspaceFeeds()); + const mirror = new RecordingWorkspaceFeedRepository(new FileWorkspaceFeedRepository(mirrorPath)); + await new MirroredWorkspaceFeedRepository(primary, mirror).init(["inbox", "ffo"]); + + expect(primary.writes).toEqual([]); + expect(mirror.writes).toEqual([]); + expect(feedRows(db)).toEqual(before); + expect(await mirror.listFeedIds()).toEqual(["inbox", "ffo"]); + }); + + test("still merges both copies when sqlite and the mirror disagree", async () => { + const root = tempRoot(); + const sqlite = await sqliteStore(root); + const primary = sqlite.workspaceFeeds(); + const mirror = new FileWorkspaceFeedRepository(path.join(root, "workspace.json")); + await primary.setFeedIds(["inbox", "sqlite-only"]); + await mirror.init(["inbox", "mirror-only"]); + + await new MirroredWorkspaceFeedRepository(primary, mirror).init(["inbox"]); + + expect(await primary.listFeedIds()).toEqual(["inbox", "sqlite-only", "mirror-only"]); + expect(await mirror.listFeedIds()).toEqual(["inbox", "sqlite-only", "mirror-only"]); + }); +});