diff --git a/apps/desktop/src/main/services/history/operationService.test.ts b/apps/desktop/src/main/services/history/operationService.test.ts index 51bfe9385..29b31d155 100644 --- a/apps/desktop/src/main/services/history/operationService.test.ts +++ b/apps/desktop/src/main/services/history/operationService.test.ts @@ -68,6 +68,10 @@ function createInMemoryAdeDb(): { db: AdeDb; raw: Database } { raw, db: { run, + runChanged: (sql: string, params?: unknown[]) => { + run(sql, params as never); + return 1; + }, get, all, getJson: () => null, diff --git a/apps/desktop/src/main/services/lanes/laneService.ts b/apps/desktop/src/main/services/lanes/laneService.ts index 00755795f..cafb2c317 100644 --- a/apps/desktop/src/main/services/lanes/laneService.ts +++ b/apps/desktop/src/main/services/lanes/laneService.ts @@ -3741,6 +3741,13 @@ export function createLaneService({ ); db.run("delete from claude_sessions where lane_id = ?", [laneId]); db.run("delete from terminal_sessions where lane_id = ?", [laneId]); + // The settle-lifecycle token table is local-only with no foreign key, so a + // bulk session delete would otherwise leave permanent orphans. Sweep by + // absence rather than by id list: that covers this path and any future one, + // which is the failure mode a targeted cascade keeps re-introducing. + db.run( + "delete from session_lifecycle_revisions where session_id not in (select id from terminal_sessions)", + ); db.run("delete from operations where lane_id = ? and project_id = ?", [laneId, projectId]); db.run("delete from packs_index where lane_id = ? and project_id = ?", [laneId, projectId]); db.run("delete from test_runs where lane_id = ? and project_id = ?", [laneId, projectId]); diff --git a/apps/desktop/src/main/services/onboarding/onboardingService.test.ts b/apps/desktop/src/main/services/onboarding/onboardingService.test.ts index 297375444..fd4eee20a 100644 --- a/apps/desktop/src/main/services/onboarding/onboardingService.test.ts +++ b/apps/desktop/src/main/services/onboarding/onboardingService.test.ts @@ -24,6 +24,7 @@ function createInMemoryAdeDb(): AdeDb { kv.set(key, value); }, run: () => {}, + runChanged: () => 0, get: () => null, all: () => [], sync: { diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index edec4d682..b5901ac7d 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -1,5 +1,6 @@ import fs from "node:fs"; import type { AdeDb } from "../state/kvDb"; +import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; import type { ClaudeSessionPointer, SessionAttentionSource, @@ -368,9 +369,17 @@ export function createSessionService({ db }: { db: AdeDb }) { const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); + // Every settle-tuple mutation goes through this writer; see + // `settleLifecycleWriter.ts` for why it is its own module and what the + // revision guarantees. + const settleLifecycle = createSettleLifecycleWriter(db); + const writeSettleLifecycle = settleLifecycle.write; + /** * Shared skeleton for the single-session lifecycle mutators: trim, existence - * probe, run the update, broadcast. Keeps every SQL literal at its call site. + * probe, run the mutation, broadcast. Settle-tuple writes inside `run` go + * through `writeSettleLifecycle`; everything else keeps its SQL at the call + * site. */ const mutateSessionMeta = (sessionId: string, run: (id: string) => void): boolean => { const trimmed = sessionId.trim(); @@ -688,26 +697,21 @@ export function createSessionService({ db }: { db: AdeDb }) { if (!newlySettled.length) return []; const updatePlaceholders = newlySettled.map(() => "?").join(", "); const hasOutcome = Object.prototype.hasOwnProperty.call(options, "outcome"); - db.run( - ` - update terminal_sessions - set settled_at = coalesce(settled_at, ?), - settle_override = null, - settle_source = ?, - ${hasOutcome ? "status_note = ?," : ""} - attention_requested_at = null, - attention_message = null, - attention_source = null - where (settled_at is null or settle_override is not null) - and id in (${updatePlaceholders}) - `, - [ - normalizeIsoTimestamp(options.settledAt) ?? new Date().toISOString(), - options.source ?? "user", - ...(hasOutcome ? [normalizeSessionStatusNote(options.outcome)] : []), - ...newlySettled, - ], - ); + writeSettleLifecycle({ + intent: { + kind: "settle", + settledAt: normalizeIsoTimestamp(options.settledAt) ?? new Date().toISOString(), + source: options.source ?? "user", + }, + extraSet: { + ...(hasOutcome ? { status_note: normalizeSessionStatusNote(options.outcome) } : {}), + attention_requested_at: null, + attention_message: null, + attention_source: null, + }, + guard: "(settled_at is null or settle_override is not null)", + sessionIds: newlySettled, + }); for (const id of newlySettled) { emitChanged({ sessionId: id, reason: "meta-updated" }); } @@ -1294,12 +1298,19 @@ export function createSessionService({ db }: { db: AdeDb }) { * are cleared explicitly at the next user turn start. */ setLastOutputPreview(sessionId: string, preview: string, opts?: { clearSettled?: boolean }): void { - db.run( - opts?.clearSettled - ? "update terminal_sessions set last_output_preview = ?, last_output_at = ?, settled_at = null, settle_override = null, settle_source = null where id = ?" - : "update terminal_sessions set last_output_preview = ?, last_output_at = ? where id = ?", - [preview, new Date().toISOString(), sessionId] - ); + const now = new Date().toISOString(); + if (!opts?.clearSettled) { + db.run( + "update terminal_sessions set last_output_preview = ?, last_output_at = ? where id = ?", + [preview, now, sessionId], + ); + return; + } + writeSettleLifecycle({ + intent: { kind: "clearOnActivity" }, + extraSet: { last_output_preview: preview, last_output_at: now }, + sessionIds: [sessionId], + }); }, /** @@ -1317,12 +1328,15 @@ export function createSessionService({ db }: { db: AdeDb }) { at: string = new Date().toISOString(), opts?: { clearSettled?: boolean }, ): void { - db.run( - opts?.clearSettled === false - ? "update terminal_sessions set last_output_at = ? where id = ?" - : "update terminal_sessions set last_output_at = ?, settled_at = null, settle_override = null, settle_source = null where id = ?", - [at, sessionId] - ); + if (opts?.clearSettled === false) { + db.run("update terminal_sessions set last_output_at = ? where id = ?", [at, sessionId]); + return; + } + writeSettleLifecycle({ + intent: { kind: "clearOnActivity" }, + extraSet: { last_output_at: at }, + sessionIds: [sessionId], + }); }, setSummary(sessionId: string, summary: string | null): void { @@ -1423,52 +1437,26 @@ export function createSessionService({ db }: { db: AdeDb }) { return mutateSessionMeta(sessionId, (id) => { // An explicit settle also drops a stale keep-active pin — otherwise the // override would silently veto the settle the user just asked for. - if (outcome) { - db.run( - ` - update terminal_sessions - set settled_at = coalesce(settled_at, ?), - settle_override = null, - settle_source = ?, - status_note = ?, - attention_requested_at = null, - attention_message = null, - attention_source = null - where id = ? - `, - [settledAt, opts.source ?? "user", outcome, id], - ); - } else { - db.run( - ` - update terminal_sessions - set settled_at = coalesce(settled_at, ?), - settle_override = null, - settle_source = ?, - attention_requested_at = null, - attention_message = null, - attention_source = null - where id = ? - `, - [settledAt, opts.source ?? "user", id], - ); - } + writeSettleLifecycle({ + intent: { kind: "settle", settledAt, source: opts.source ?? "user" }, + extraSet: { + ...(outcome ? { status_note: outcome } : {}), + attention_requested_at: null, + attention_message: null, + attention_source: null, + }, + sessionIds: [id], + }); }); }, /** Clears a declared settle plus any `'settled'` override. */ unsettleSession(sessionId: string): boolean { const changed = mutateSessionMeta(sessionId, (id) => { - db.run( - ` - update terminal_sessions - set settled_at = null, - settle_override = case when settle_override = 'settled' then null else settle_override end, - settle_source = null - where id = ? - `, - [id], - ); + writeSettleLifecycle({ + intent: { kind: "unsettleDeclared" }, + sessionIds: [id], + }); }); return changed; }, @@ -1482,19 +1470,10 @@ export function createSessionService({ db }: { db: AdeDb }) { const normalized = override == null ? null : normalizeSettleOverride(override); const normalizedSource = normalizeSettleSource(source) ?? "user"; return mutateSessionMeta(sessionId, (id) => { - db.run( - ` - update terminal_sessions - set settle_override = ?, - settle_source = case - when ? = 'settled' then ? - when settled_at is null then null - else settle_source - end - where id = ? - `, - [normalized, normalized, normalizedSource, id], - ); + writeSettleLifecycle({ + intent: { kind: "override", value: normalized, source: normalizedSource }, + sessionIds: [id], + }); }); }, @@ -1509,25 +1488,28 @@ export function createSessionService({ db }: { db: AdeDb }) { ).map((row) => row.id); if (!present.length) return []; const updatePlaceholders = present.map(() => "?").join(", "); - db.run( - ` - update terminal_sessions - set settle_override = ?, - settle_source = case - when ? = 'settled' then 'user' - when settled_at is null then null - else settle_source - end - where id in (${updatePlaceholders}) - `, - [normalized, normalized, ...present], - ); + writeSettleLifecycle({ + intent: { kind: "override", value: normalized, source: "user" }, + sessionIds: present, + }); for (const id of present) { emitChanged({ sessionId: id, reason: "meta-updated" }); } return present; }, + /** + * The host-local settle concurrency token for a session. + * + * Read it before a decision that takes time, and require it to be unchanged + * before applying that decision — that is the whole point of the + * chokepoint. 0 means "no settle-lifecycle mutation has been recorded for + * this session", which a caller must treat as a real value, not as absent. + */ + getSettleLifecycleRevision(sessionId: string): number { + return settleLifecycle.readRevision(sessionId); + }, + settleSessions(sessionIds: string[]): string[] { return settleMany(sessionIds); }, @@ -1545,16 +1527,10 @@ export function createSessionService({ db }: { db: AdeDb }) { const ids = normalizeSessionIds(sessionIds); if (!ids.length) return; const placeholders = ids.map(() => "?").join(", "); - db.run( - ` - update terminal_sessions - set settled_at = null, - settle_override = case when settle_override = 'settled' then null else settle_override end, - settle_source = null - where id in (${placeholders}) - `, - ids, - ); + writeSettleLifecycle({ + intent: { kind: "unsettleDeclared" }, + sessionIds: ids, + }); for (const id of ids) { emitChanged({ sessionId: id, reason: "meta-updated" }); } @@ -1700,19 +1676,15 @@ export function createSessionService({ db }: { db: AdeDb }) { source: SessionAttentionSource = "agent_explicit", ): boolean { return mutateSessionMeta(sessionId, (id) => { - db.run( - ` - update terminal_sessions - set attention_requested_at = ?, - attention_message = ?, - attention_source = ?, - settled_at = null, - settle_override = null, - settle_source = null - where id = ? - `, - [new Date().toISOString(), normalizeOptionalText(message, 500), source, id], - ); + writeSettleLifecycle({ + intent: { kind: "clearOnActivity" }, + extraSet: { + attention_requested_at: new Date().toISOString(), + attention_message: normalizeOptionalText(message, 500), + attention_source: source, + }, + sessionIds: [id], + }); wakeSnoozedRow(id, "needs_you"); }); }, @@ -1733,10 +1705,11 @@ export function createSessionService({ db }: { db: AdeDb }) { // and the row must surface red, not hide in the quiet tier. This keeps // settled/failed mutually exclusive at write time, so every surface's // precedence order agrees by construction. - db.run( - "update terminal_sessions set last_turn_failed_at = ?, settled_at = null, settle_override = null, settle_source = null where id = ?", - [failedAt, id], - ); + writeSettleLifecycle({ + intent: { kind: "clearOnActivity" }, + extraSet: { last_turn_failed_at: failedAt }, + sessionIds: [id], + }); // Early wake, but ONLY for an error newer than the snooze. Snoozing on // top of an existing failure must stay snoozed. wakeSnoozedRow(id, "error", { errorAt: failedAt }); @@ -1757,20 +1730,16 @@ export function createSessionService({ db }: { db: AdeDb }) { clearTurnStartMarkers(sessionId: string): boolean { const changed = mutateSessionMeta(sessionId, (id) => { - db.run( - ` - update terminal_sessions - set last_turn_failed_at = null, - settled_at = null, - settle_override = null, - settle_source = null, - attention_requested_at = null, - attention_message = null, - attention_source = null - where id = ? - `, - [id], - ); + writeSettleLifecycle({ + intent: { kind: "clearOnActivity" }, + extraSet: { + last_turn_failed_at: null, + attention_requested_at: null, + attention_message: null, + attention_source: null, + }, + sessionIds: [id], + }); }); return changed; }, @@ -1784,6 +1753,10 @@ export function createSessionService({ db }: { db: AdeDb }) { ); if (!existing) return false; db.run("delete from terminal_sessions where id = ?", [trimmed]); + // Reap the lifecycle token with its row. ADE has been bitten before by a + // local table with no reaper, and every other session-keyed side table is + // already cascaded here. + settleLifecycle.forget(trimmed); emitChanged({ sessionId: trimmed, reason: "deleted" }); return true; }, diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts new file mode 100644 index 000000000..3143df0b7 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts @@ -0,0 +1,395 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { openKvDb } from "../state/kvDb"; +import { createSessionService } from "./sessionService"; + +/** + * The settle-lifecycle chokepoint. + * + * `settled_at` is written and cleared from ten call paths, four of them not + * named "settle" or "unsettle" and one of them running per terminal output + * chunk. Attaching an async teardown to that was tried and cut in PR #1059: it + * produced a P1 every review round, because a decision taken at t0 and applied + * at t0+T has no way to know the world moved in between. + * + * These pin the two properties that fix the class: every mutation goes through + * one function, and every mutation moves a revision a later write can be made + * conditional on. + */ + +const WRITER_MODULE = path.join(__dirname, "settleLifecycleWriter.ts"); + +function createLogger() { + return { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} } as const; +} + +function makeProjectRoot(): string { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "ade-settle-chokepoint-")); + fs.mkdirSync(path.join(root, ".ade", "artifacts"), { recursive: true }); + return root; +} + +function insertProjectGraph(db: Awaited>) { + const now = "2026-08-11T00:00:00.000Z"; + db.run( + `insert into projects(id, root_path, display_name, default_base_ref, created_at, last_opened_at) + values (?, ?, ?, ?, ?, ?)`, + ["project-1", "/repo/ade", "ADE", "main", now, now], + ); + db.run( + `insert into lanes( + id, project_id, name, description, lane_type, base_ref, branch_ref, worktree_path, attached_root_path, + is_edit_protected, parent_lane_id, color, icon, tags_json, folder, status, created_at, archived_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + [ + "lane-1", "project-1", "Lane 1", null, "worktree", "main", "ade/lane-1", + "/repo/ade/.ade/worktrees/lane-1", null, 0, null, null, null, "[]", null, "active", now, null, + ], + ); +} + +describe("settle-lifecycle writer", () => { + const activeDisposers: Array<() => Promise> = []; + + afterEach(async () => { + while (activeDisposers.length) await activeDisposers.pop()?.(); + }); + + async function withService(): Promise> { + const projectRoot = makeProjectRoot(); + const db = await openKvDb(path.join(projectRoot, ".ade", "ade.db"), createLogger() as any); + activeDisposers.push(async () => db.close()); + insertProjectGraph(db); + const service = createSessionService({ db }); + service.create({ + sessionId: "session-1", + laneId: "lane-1", + ptyId: null, + tracked: true, + title: "Chat", + startedAt: "2026-08-11T00:01:00.000Z", + transcriptPath: "/tmp/session-1.log", + toolType: "codex-chat", + }); + return service; + } + + /** + * The load-bearing one. A guard belongs to the writer, not the callers — so + * the guarantee is only real if no other writer exists. This scans the source + * rather than trusting review, because the failure mode is someone adding an + * eleventh path in six months. + */ + it("has no settle-tuple assignment anywhere outside the chokepoint", () => { + // Repo-wide, not just this file. The invariant is "no other writer exists", + // and an eleventh path added in a new service would otherwise pass clean. + const roots = [ + path.resolve(__dirname, "../../.."), // apps/desktop/src + path.resolve(__dirname, "../../../../../ade-cli/src"), // apps/ade-cli/src + ]; + const files: string[] = []; + const walk = (dir: string) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === "node_modules" || entry.name === "dist") continue; + walk(full); + } else if (entry.name.endsWith(".ts") && !entry.name.endsWith(".test.ts")) { + files.push(full); + } + } + }; + for (const root of roots) { + // A silently-skipped root would quietly stop enforcing half the invariant. + expect(fs.existsSync(root), `scan root missing: ${root}`).toBe(true); + walk(root); + } + expect(files.length).toBeGreaterThan(100); + + const offenders: string[] = []; + for (const file of files) { + // The writer module is the one place allowed to assign the tuple. A FILE + // boundary, not offsets inside a 1900-line module — that is why the writer + // was extracted, and it means no future reordering can widen the blind + // spot. + if (file === WRITER_MODULE) continue; + const source = fs.readFileSync(file, "utf8"); + // Strip comments first: prose describing what the host writes (the web + // adapter documents the host's SQL in JSDoc) is not a writer, and matching + // it would make this test cry wolf until someone weakened it. + const code = source + .replace(/\/\*[\s\S]*?\*\//g, "") + .replace(/(^|[^:])\/\/[^\n]*/g, "$1"); + // `=` only, never `==` — a `where settle_override = ?` comparison in a + // SELECT is legitimate, so require the assignment form used in a SET list. + for (const match of code.match(/\b(settled_at|settle_override|settle_source)\s*=(?!=)/gi) ?? []) { + offenders.push(`${path.relative(roots[0], file)}: ${match}`); + } + } + + expect(offenders).toEqual([]); + + // The scan is the enforcement mechanism, so prove it still bites — including + // for a spelling nobody in this codebase uses today. + const upperFixture = "UPDATE terminal_sessions SET SETTLED_AT = NULL WHERE id = ?"; + expect(upperFixture.match(/\b(settled_at|settle_override|settle_source)\s*=(?!=)/gi)).not.toBeNull(); + }); + + it("bumps the revision on every settle-lifecycle path", async () => { + const service = await withService(); + let previous = service.getSettleLifecycleRevision("session-1"); + expect(previous).toBe(0); + + const bumped = (label: string) => { + const next = service.getSettleLifecycleRevision("session-1"); + expect(next, `${label} must move the revision`).toBeGreaterThan(previous); + previous = next; + }; + + service.settleSessions(["session-1"]); + bumped("settleSessions (W1)"); + + service.unsettleSession("session-1"); + bumped("unsettleSession (C1)"); + + service.settleSession("session-1"); + bumped("settleSession (W2)"); + + service.unsettleSessions(["session-1"]); + bumped("unsettleSessions (C2)"); + + // `settle_override` never touches `settled_at`, but a `'settled'` pin makes + // the row read as settled all the same — so it has to move the revision or + // the guard is blind to it. + service.setSettleOverride("session-1", "settled"); + bumped("setSettleOverride (W3)"); + + service.setSettleOverrides(["session-1"], null); + bumped("setSettleOverrides (W3 bulk)"); + + service.clearTurnStartMarkers("session-1"); + bumped("clearTurnStartMarkers (C3)"); + + service.setLastOutputPreview("session-1", "output", { clearSettled: true }); + bumped("setLastOutputPreview (C4)"); + + service.touchSessionActivity("session-1"); + bumped("touchSessionActivity (C5)"); + + service.markLastTurnFailed("session-1"); + bumped("markLastTurnFailed (C6)"); + + service.requestAttention("session-1", "need you"); + bumped("requestAttention (C7)"); + }); + + /** + * The counterpart: a write that does NOT touch the settle tuple must leave + * the revision alone, or a guard built on it would reject settles for reasons + * that have nothing to do with settling. + */ + it("leaves the revision alone for writes that do not touch the settle tuple", async () => { + const service = await withService(); + service.settleSessions(["session-1"]); + const settled = service.getSettleLifecycleRevision("session-1"); + + // Explicitly opted out of clearing the settle — the agent-CLI case. + service.setLastOutputPreview("session-1", "trailing agent output"); + service.touchSessionActivity("session-1", "2026-08-11T00:05:00.000Z", { clearSettled: false }); + service.setSummary("session-1", "a summary"); + + expect(service.getSettleLifecycleRevision("session-1")).toBe(settled); + expect(service.get("session-1")?.settledAt).toBeTruthy(); + }); + + it("keeps revisions per session", async () => { + const service = await withService(); + service.create({ + sessionId: "session-2", + laneId: "lane-1", + ptyId: null, + tracked: true, + title: "Other", + startedAt: "2026-08-11T00:02:00.000Z", + transcriptPath: "/tmp/session-2.log", + toolType: "codex-chat", + }); + + service.settleSessions(["session-1"]); + + expect(service.getSettleLifecycleRevision("session-1")).toBe(1); + expect(service.getSettleLifecycleRevision("session-2")).toBe(0); + }); + + it("bumps every id in a bulk mutation, not just the first", async () => { + const service = await withService(); + service.create({ + sessionId: "session-2", + laneId: "lane-1", + ptyId: null, + tracked: true, + title: "Other", + startedAt: "2026-08-11T00:02:00.000Z", + transcriptPath: "/tmp/session-2.log", + toolType: "codex-chat", + }); + + service.settleSessions(["session-1", "session-2"]); + + expect(service.getSettleLifecycleRevision("session-1")).toBe(1); + expect(service.getSettleLifecycleRevision("session-2")).toBe(1); + }); + + /** + * A bump can land in either store — the table normally, memory when the table + * write fails. If the table then starts working it begins its own count at 1, + * so a reader that preferred one store could hand back a LOWER number than it + * had already returned. A revision that moves backwards is worse than none: it + * lets a stale settle match a token it should have missed. + */ + it("never reports a revision lower than one it has already reported", async () => { + const projectRoot = makeProjectRoot(); + const db = await openKvDb(path.join(projectRoot, ".ade", "ade.db"), createLogger() as any); + activeDisposers.push(async () => db.close()); + insertProjectGraph(db); + + // Force the persisted bump to fail so it falls back to memory. + const realRun = db.run.bind(db); + (db as unknown as { run: typeof db.run }).run = (sql: string, params?: unknown[]) => { + if (sql.includes("session_lifecycle_revisions")) throw new Error("table unavailable"); + return realRun(sql, params as never); + }; + + const service = createSessionService({ db }); + service.create({ + sessionId: "session-1", + laneId: "lane-1", + ptyId: null, + tracked: true, + title: "Chat", + startedAt: "2026-08-11T00:01:00.000Z", + transcriptPath: "/tmp/session-1.log", + toolType: "codex-chat", + }); + + service.settleSessions(["session-1"]); + service.unsettleSessions(["session-1"]); + service.settleSessions(["session-1"]); + const afterFallback = service.getSettleLifecycleRevision("session-1"); + expect(afterFallback).toBeGreaterThanOrEqual(3); + + // The table starts working again; its own count restarts at 1. + (db as unknown as { run: typeof db.run }).run = realRun; + service.unsettleSessions(["session-1"]); + + expect(service.getSettleLifecycleRevision("session-1")).toBeGreaterThan(afterFallback); + }); + + /** + * The dangerous direction, and the subtle one. Another ADE process can push + * the persisted revision ahead of this process's private counter. If a local + * mutation's table write then fails, incrementing the private counter alone + * can leave `max(persisted, inProcess)` UNCHANGED across a real change — and a + * teardown holding that token would accept a decision the world has already + * invalidated. + */ + it("still advances when a sibling process is ahead and the local write fails", async () => { + const projectRoot = makeProjectRoot(); + const db = await openKvDb(path.join(projectRoot, ".ade", "ade.db"), createLogger() as any); + activeDisposers.push(async () => db.close()); + insertProjectGraph(db); + const service = createSessionService({ db }); + service.create({ + sessionId: "session-1", + laneId: "lane-1", + ptyId: null, + tracked: true, + title: "Chat", + startedAt: "2026-08-11T00:01:00.000Z", + transcriptPath: "/tmp/session-1.log", + toolType: "codex-chat", + }); + + // A sibling process advanced the token to 5; this process has seen nothing. + db.run( + "insert into session_lifecycle_revisions (session_id, revision) values (?, 5)" + + " on conflict(session_id) do update set revision = 5", + ["session-1"], + ); + const before = service.getSettleLifecycleRevision("session-1"); + expect(before).toBe(5); + + // Now this process mutates, and its own token write fails. + const realRun = db.run.bind(db); + (db as unknown as { run: typeof db.run }).run = (sql: string, params?: unknown[]) => { + if (sql.includes("session_lifecycle_revisions")) throw new Error("token write failed"); + return realRun(sql, params as never); + }; + service.settleSessions(["session-1"]); + (db as unknown as { run: typeof db.run }).run = realRun; + + expect( + service.getSettleLifecycleRevision("session-1"), + "a real mutation must be visible even when its token write failed", + ).toBeGreaterThan(before); + }); + + /** + * Documents a deliberate choice rather than a bug. `sqlite3_changes` counts + * matched rows, not differing values, so re-clearing an already-clear tuple + * spends a revision. Predicating the UPDATE on "would actually change" was + * tried and reverted: it gates the caller's own columns too, and the preview + * silently stopped being written. Over-bumping is the safe direction — a spent + * revision costs a re-taken settle, a missed one accepts a stale decision — + * and step 2's swallow rule is what stops a session's idle output + * invalidating its own teardown. + */ + it("spends a revision on a no-op clear, and still writes the caller's columns", async () => { + const service = await withService(); + const before = service.getSettleLifecycleRevision("session-1"); + + service.setLastOutputPreview("session-1", "tick one", { clearSettled: true }); + + expect(service.getSettleLifecycleRevision("session-1")).toBeGreaterThan(before); + expect(service.get("session-1")?.lastOutputPreview).toBe("tick one"); + }); + + it("keeps the revision table out of CRR replication", async () => { + const projectRoot = makeProjectRoot(); + const db = await openKvDb(path.join(projectRoot, ".ade", "ade.db"), createLogger() as any); + activeDisposers.push(async () => db.close()); + + const clockTable = (table: string) => + db.get<{ present: number }>( + "select 1 as present from sqlite_master where type = 'table' and name = ?", + [`${table}__crsql_clock`], + ); + + // cr-sqlite ships macOS-only binaries; on Linux CI the extension is absent + // (`db.crsqlite_unavailable`) and NO table has a clock, so "excluded from + // CRR" is trivially true and unfalsifiable there. Assert it where it can + // actually fail, and say plainly why it cannot elsewhere — a green assertion + // that could never go red is exactly what the first version of this test was. + const crrLoaded = db.get<{ present: number }>( + "select 1 as present from sqlite_master where type = 'table' and name like '%__crsql_clock' limit 1", + ) !== null; + + if (crrLoaded) { + // A host-local concurrency token must never reach another device: it is + // meaningless there, and on the CRR `terminal_sessions` row it would add a + // per-column clock entry to the throttled preview write path. + expect(clockTable("session_lifecycle_revisions")).toBeNull(); + // Positive control: proves the assertion above can fail. + expect(clockTable("terminal_sessions")).not.toBeNull(); + } + + expect( + db.get<{ name: string }>( + "select name from sqlite_master where type = 'table' and name = 'session_lifecycle_revisions'", + )?.name, + ).toBe("session_lifecycle_revisions"); + }); + +}); diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts new file mode 100644 index 000000000..d51c8c60a --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts @@ -0,0 +1,241 @@ +import type { AdeDb, SqlValue } from "../state/kvDb"; +import type { SessionSettleOverride, SessionSettleSource } from "../../../shared/types/sessions"; + +/** + * The settle-lifecycle writer. + * + * Extracted from `sessionService` so the "no settle-tuple SQL outside this + * unit" invariant is a FILE boundary rather than a pair of offsets inside a + * 1900-line module — `settleLifecycleWriter.test.ts` allowlists this file and + * scans everything else. + */ +/** What a settle-lifecycle mutation is asking for. */ +export type SettleLifecycleIntent = + | { kind: "settle"; settledAt: string; source: SessionSettleSource } + /** Real activity: drop the whole declaration, keep-active pin included. */ + | { kind: "clearOnActivity" } + /** Declared unsettle: drops a `'settled'` pin but preserves `'active'`. */ + | { kind: "unsettleDeclared" } + | { kind: "override"; value: SessionSettleOverride | null; source: SessionSettleSource }; + +/** + * Columns a caller may set alongside the settle tuple. A closed union, so + * routing a tuple column through `extraSet` is a compile error rather than + * something only the source-scan test would catch. + */ +export type SettleExtraColumn = + | "status_note" + | "attention_requested_at" + | "attention_message" + | "attention_source" + | "last_output_preview" + | "last_output_at" + | "last_turn_failed_at"; + + +export type SettleLifecycleWriter = { + write: (args: { + intent: SettleLifecycleIntent; + sessionIds: readonly string[]; + extraSet?: Partial>; + guard?: string; + }) => void; + readRevision: (sessionId: string) => number; + /** Drop a deleted session's token so the local table and map do not grow forever. */ + forget: (sessionId: string) => void; +}; + +export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { + // --------------------------------------------------------------------- + // The settle-lifecycle chokepoint. + // + // Every mutation of the settle tuple — `settled_at`, `settle_override`, + // `settle_source` — goes through `writeSettleLifecycle`, and no SQL literal + // anywhere else may assign those columns. `settleLifecycleChokepoint.test.ts` + // enforces that by scanning this file. + // + // The point is the REVISION. A settle decision is taken at t0 and (once + // teardown exists) applied at t0 + T, and the world is free to move in + // between. The revision is the detector that window needs: bumped in the same + // transaction as the column write, so there is no gap between "the world + // changed" and "the revision says so". + // + // `settle_override` is inside the tuple deliberately. It never touches + // `settled_at`, but a `'settled'` pin makes a row read as settled at the + // declared-settle tier all the same — so a revision keyed to `settled_at` + // alone would be blind to exactly the kind of change it exists to catch. + // --------------------------------------------------------------------- + + const settleTupleAssignment = (intent: SettleLifecycleIntent): { sql: string; params: SqlValue[] } => { + switch (intent.kind) { + case "settle": + return { + sql: "settled_at = coalesce(settled_at, ?), settle_override = null, settle_source = ?", + params: [intent.settledAt, intent.source], + }; + case "clearOnActivity": + return { sql: "settled_at = null, settle_override = null, settle_source = null", params: [] }; + case "unsettleDeclared": + return { + sql: + "settled_at = null, " + + "settle_override = case when settle_override = 'settled' then null else settle_override end, " + + "settle_source = null", + params: [], + }; + case "override": + return { + sql: + "settle_override = ?, " + + "settle_source = case when ? = 'settled' then ? when settled_at is null then null else settle_source end", + params: [intent.value, intent.value, intent.source], + }; + } + }; + + /** + * Per-process revision counter, incremented on EVERY bump — not only when the + * table write fails. + * + * Both stores are needed, and not only for restart survival. ADE supports + * several processes against one database (`kvDb` sets `busy_timeout` for + * exactly that), and the desktop main process and the CLI brain each build a + * `sessionService` over the same file — so a table value can be AHEAD of this + * process, written by a sibling. Conversely, bumping memory only on failure + * would let the revision stall: after a failed write the table is behind, and + * on recovery it restarts its own count at 1, so a real mutation could produce + * no observable change — the failure direction that matters. Reading + * `max(table, in-process)` is correct against both. + * + * Bounded by the sessions this process has touched, same order as the row set + * it shadows. + */ + const inProcessLifecycleRevisions = new Map(); + + const persistedRevision = (sessionId: string): number => { + const row = db.get<{ revision: number }>( + "select revision from session_lifecycle_revisions where session_id = ?", + [sessionId], + ); + return row ? Number(row.revision) || 0 : 0; + }; + + const bumpLifecycleRevisions = (sessionIds: readonly string[]): void => { + for (const id of sessionIds) { + try { + // One atomic statement, PK lookup, local-only table. No pre-read: this + // sits on the session output path. + db.run( + `insert into session_lifecycle_revisions (session_id, revision) values (?, 1) + on conflict(session_id) do update set revision = session_lifecycle_revisions.revision + 1`, + [id], + ); + inProcessLifecycleRevisions.set(id, (inProcessLifecycleRevisions.get(id) ?? 0) + 1); + } catch { + // The write failed, so this process must produce a strictly higher value + // on its own — and "higher" has to mean higher than what a READER would + // have seen, which is `max(persisted, inProcess)`. Incrementing the + // private counter alone is not enough: if a sibling ADE process has + // pushed the table ahead of it, `max` would return the same number + // across a real mutation, and a teardown holding that token would accept + // a decision the world has already invalidated. Reading here is safe — + // this branch is the rare one, not the output path. + let anchor = inProcessLifecycleRevisions.get(id) ?? 0; + try { + anchor = Math.max(anchor, persistedRevision(id)); + } catch { + // Table unreadable too; the private counter is all there is. + } + inProcessLifecycleRevisions.set(id, anchor + 1); + } + } + }; + + const readLifecycleRevision = (sessionId: string): number => { + const trimmed = sessionId.trim(); + if (!trimmed) return 0; + let persisted = 0; + try { + persisted = persistedRevision(trimmed); + } catch { + persisted = 0; + } + // The MAXIMUM of the two stores, never whichever answered. After a restart + // — or a write by a sibling ADE process — the table is ahead; after a failed + // write the in-process counter is. Taking either alone could hand back a + // LOWER number than a previous call did, and a revision that moves backwards + // is worse than none: it lets a stale settle match a token it should have + // missed. Monotonic within a process is the property this value must have; + // across a restart it can reset only when table writes were failing, which + // is benign because a restart abandons any decision that was in flight. + return Math.max(persisted, inProcessLifecycleRevisions.get(trimmed) ?? 0); + }; + + /** + * The ONLY writer of the settle tuple. + * + * `sessionIds` is both the scope of the update and the set whose revisions + * move — one array, so the two cannot drift. `guard` is an extra predicate + * ANDed onto the id match; it takes no parameters, and only `settleMany` uses + * one (its "still unsettled" check, which another ADE process could otherwise + * invalidate between the select and the update). + */ + const writeSettleLifecycle = (args: { + intent: SettleLifecycleIntent; + sessionIds: readonly string[]; + extraSet?: Partial>; + guard?: string; + }): void => { + const ids = args.sessionIds.map((id) => id.trim()).filter(Boolean); + if (!ids.length) return; + const tuple = settleTupleAssignment(args.intent); + const extraEntries = Object.entries(args.extraSet ?? {}) as Array<[SettleExtraColumn, SqlValue]>; + const setClauses = [...extraEntries.map(([column]) => `${column} = ?`), tuple.sql]; + const setParams = [...extraEntries.map(([, value]) => value), ...tuple.params]; + const idPlaceholders = ids.map(() => "?").join(", "); + const where = args.guard + ? `${args.guard} and id in (${idPlaceholders})` + : `id in (${idPlaceholders})`; + + const changed = db.runChanged( + `update terminal_sessions set ${setClauses.join(", ")} where ${where}`, + [...setParams, ...ids], + ); + // Only a write that MATCHED A ROW bumps — that is what this gate buys, and + // it is what stops a deleted or absent session inserting an orphan token. + // + // It is deliberately not "the tuple's values differed": `sqlite3_changes` + // counts matched rows, not changed values, so re-clearing an already-clear + // tuple still counts. Predicating the UPDATE on "would actually change" + // was tried and reverted — it gates the caller's own columns too, so a + // preview write silently stopped landing. Over-bumping is the safe + // direction anyway (a spent revision costs a re-taken settle; a missed one + // accepts a stale decision), and step 2's swallow rule is what keeps a + // session's own idle output from invalidating its teardown. + if (changed <= 0) return; + // Immediately adjacent, with no `await` between, and `bumpLifecycleRevisions` + // cannot throw. `AdeDb` exposes no transaction helper and an explicit BEGIN + // here could nest inside a caller's, so adjacency is what orders them: within + // this process no reader can see the column write without the bump. ADE does + // support several processes against one database, so a sibling process could + // observe the pair mid-flight; that window is microseconds and closing it + // needs a transaction helper `AdeDb` does not have. + bumpLifecycleRevisions(ids); + }; + + + return { + write: writeSettleLifecycle, + readRevision: readLifecycleRevision, + forget: (sessionId: string) => { + const trimmed = sessionId.trim(); + if (!trimmed) return; + try { + db.run("delete from session_lifecycle_revisions where session_id = ?", [trimmed]); + } catch { + // The token is advisory; failing to reap it must not fail the delete. + } + inProcessLifecycleRevisions.delete(trimmed); + }, + }; +} diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index de5e33d52..4cd80c0e8 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -150,6 +150,13 @@ export type AdeDb = { setJson: (key: string, value: unknown) => void; run: (sql: string, params?: SqlValue[]) => void; + /** + * `run` with the row count. Additive — `run` still returns void, so no + * existing caller changes. For code where "did this actually change state?" + * is the question rather than "did the statement execute": the settle + * lifecycle bumps its revision only for a write that matched a row. + */ + runChanged: (sql: string, params?: SqlValue[]) => number; get: = Record>(sql: string, params?: SqlValue[]) => T | null; all: = Record>(sql: string, params?: SqlValue[]) => T[]; @@ -884,6 +891,10 @@ const LOCAL_ONLY_CRR_EXCLUDED_TABLES = new Set([ "local_worktree_residual_cleanups", "local_lane_storage_state", "local_storage_lifecycle_runs", + // Host-local settle concurrency token. Never replicated: it is meaningless + // off the host that issued it, and putting it on the CRR `terminal_sessions` + // row would add a per-column clock entry to the per-output-chunk write path. + "session_lifecycle_revisions", ]); function listEligibleCrrTables(db: DatabaseSyncType): string[] { @@ -1713,6 +1724,7 @@ function normalizeIncomingCrsqlChange(db: DatabaseSyncType, change: CrsqlChangeR type MigrationDb = { run: (sql: string, params?: SqlValue[]) => void; + runChanged: (sql: string, params?: SqlValue[]) => number; get: = Record>(sql: string, params?: SqlValue[]) => T | null; all: = Record>(sql: string, params?: SqlValue[]) => T[]; }; @@ -1793,6 +1805,7 @@ function makeCrrAwareDb({ } runStatement(db, sql, params); }, + runChanged: (sql: string, params: SqlValue[] = []) => runStatement(getDb(), sql, params).changes, get: = Record>(sql: string, params: SqlValue[] = []) => { return getRow(getDb(), sql, params); }, @@ -3808,6 +3821,25 @@ function migrate(db: MigrationDb, rawDb: DatabaseSyncType) { ) `); + // Host-local concurrency token for the session settle lifecycle. Every + // mutation of the settle tuple (`settled_at` / `settle_override` / + // `settle_source`) bumps the row's revision in the same transaction, so a + // settle decision taken at t0 can be applied conditionally on nothing having + // moved since. + // + // Deliberately LOCAL-ONLY rather than a column on `terminal_sessions`: that + // table is a CRR, its settle row is rewritten per terminal output chunk, and + // cr-sqlite clocks are per column — a revision column would add a clock entry + // to the highest-frequency write in the product, for a value no other device + // can use. Only the host runs the chokepoint, so only the host needs the + // token. Keyed by session id alone so the bump stays one atomic statement. + db.run(` + create table if not exists session_lifecycle_revisions ( + session_id text primary key, + revision integer not null default 0 + ) + `); + // Machine-local runtime guard for PR automation. This table intentionally // has no PRIMARY KEY so cr-sqlite does not register it as a CRR table. db.run(` @@ -4118,6 +4150,7 @@ export async function openKvDb( }; const run = crrAwareDb.run; + const runChanged = crrAwareDb.runChanged; const all = crrAwareDb.all; const get = crrAwareDb.get; @@ -4627,6 +4660,7 @@ export async function openKvDb( setString(key, JSON.stringify(value)); }, run, + runChanged, all, get, sync, diff --git a/docs/features/terminals-and-sessions/README.md b/docs/features/terminals-and-sessions/README.md index 1f352098a..9c6daf03d 100644 --- a/docs/features/terminals-and-sessions/README.md +++ b/docs/features/terminals-and-sessions/README.md @@ -210,6 +210,16 @@ and in tests. If chat hydration fails, a persisted resumable `status = "running"` row falls back to quiet idle/waiting instead of presenting a false live/green agent. +- `apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts` — the + single writer of the settle tuple (`settled_at`, `settle_override`, + `settle_source`) and the host-local revision that moves with it. It is a + separate module so the "no settle-tuple SQL anywhere else" invariant is a file + boundary its colocated test can enforce by scanning the rest of the tree. + `sessionService` holds the only instance; every settle, unsettle, override, + and activity-clear path routes through it. The revision detects changes made + by THIS host — a sibling ADE process or a paired desktop peer's CRR write is + outside its scope, which + [settle-teardown-design.md](settle-teardown-design.md) §3a states precisely. - `apps/desktop/src/main/services/sessions/settleTerminalSession.ts` — single settlement transaction shared by direct IPC and the ADE action registry. Settle writes lifecycle state only — it deliberately does NOT stop diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index 0f2cb1401..c1d831d35 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -4,8 +4,9 @@ to implement; step 3 (attaching real teardown) waits until 1 and 2 are merged and the race-matrix tests have been seen to pass. -**Step 0 is implemented** — see "Host enforcement for pre-fix clients" in -§3c-i for the host-side half. +**Steps 0 and 1 are implemented.** Step 0's host-side half is "Host enforcement +for pre-fix clients" in §3c-i; step 1 is the chokepoint and revision in §3a, +whose implemented shape is recorded at the end of that section. Settle currently writes a lifecycle column and stops nothing. A session filed as "done" can still own a background shell, a subagent fleet, or a Cursor cloud run @@ -29,34 +30,66 @@ Companion reading: `README.md` (canonical phase, settle semantics), ## 1. Every path that writes or clears `settled_at` -All line numbers are `apps/desktop/src/main/services/sessions/sessionService.ts` -at merge of #1059 (`6d9ff5771`). **Seven distinct paths mutate the column, and -only three of them are named "settle" or "unsettle".** That asymmetry is the -whole problem: teardown was wired to the three obvious ones. +Verified against `apps/desktop/src/main/services/sessions/sessionService.ts` +while implementing step 1. This describes the **pre-chokepoint** state — the +problem being solved — so it is written in terms of method names rather than +line numbers, which the refactor invalidated and which no future edit will keep +true. The counts below correct the ones this section originally carried; the +shape of the argument is unchanged, and the corrections make it stronger. + +**Ten call paths mutate the settle lifecycle, and only three are named "settle" +or "unsettle".** That asymmetry is the whole problem: teardown was wired to the +three obvious ones. Precisely: + +- **10 call paths** — W1-W3 and C1-C7 below. +- **9 of them assign `settled_at`**, in **10 SQL statements before the + refactor** (W2 had two branches, now collapsed into one). W3 is the exception: + see below. +- **5 of the 7 clearers are implicit** — C3, C4, C5, C6, and C7 are not named + "unsettle" and a reader looking for settle logic will not find them. +- **All of them live in this one file.** A repo-wide search for a + `settled_at` / `settle_override` / `settle_source` assignment finds nothing + outside it, which is what makes a single chokepoint achievable at all. + +**W3 never touches `settled_at`.** `setSettleOverride` / `setSettleOverrides` +assign `settle_override` and `settle_source` only — they merely *read* +`settled_at` inside a `case` to decide the source. But a `'settled'` pin makes a +row read as settled at the declared-settle tier regardless, so a revision keyed +to `settled_at` alone would be blind to a change that alters the settle decision +completely. This is finding 13 in §4 — *verify the field a guard reads actually +changes on the event it guards* — reappearing in the inventory itself. + +The chokepoint therefore owns the whole **settle tuple** (`settled_at`, +`settle_override`, `settle_source`), and the revision moves on any of them. ### 1a. Writers (set `settled_at`) -| # | Site | Method | Invoked by | -|---|---|---|---| -| W1 | `:694` | `settleMany` (private; backs `settleSessions` `:1531` and `settleSessionsWithOutcome` `:1535`) | `registry.ts:2171` (`session.settleSessions`) · `registerIpc.ts:6989` (`sessions.settleMany`) · `syncRemoteCommandService.ts:4132` (`session.settleSessions`) · `prMergeAutoSettlementService.ts:188` (PR-merge auto-settle) | -| W2 | `:1430`, `:1445` | `settleSession` (single) | `ctoOperatorTools.ts:555` (CTO operator tool) · `settleTerminalSession.ts` → `registry.ts:2119`, `registerIpc.ts` (`sessions.settle`), `syncRemoteCommandService.ts` (`session.settleSession`) | -| W3 | `:1491`, `:1518` | `setSettleOverride` / `setSettleOverrides` (`'settled'` pin behaves as a declared settle) | row menus and bulk actions via the registry/IPC lifecycle surface | +| # | Method | Invoked by | +|---|---|---| +| W1 | `settleMany` (private; backs `settleSessions` and `settleSessionsWithOutcome`) | `registry.ts:2171` (`session.settleSessions`) · `registerIpc.ts:6989` (`sessions.settleMany`) · `syncRemoteCommandService.ts:4132` (`session.settleSessions`) · `prMergeAutoSettlementService.ts:188` (PR-merge auto-settle) | +| W2 | `settleSession` (single) | `ctoOperatorTools.ts:555` (CTO operator tool) · `settleTerminalSession.ts` → `registry.ts:2119`, `registerIpc.ts` (`sessions.settle`), `syncRemoteCommandService.ts` (`session.settleSession`) | +| W3 | `setSettleOverride` / `setSettleOverrides` — assigns `settle_override` / `settle_source` only, **never `settled_at`**, but a `'settled'` pin behaves as a declared settle | row menus and bulk actions via the registry/IPC lifecycle surface | ### 1b. Clearers (set `settled_at = null`) -| # | Site | Method | Invoked by | Named "unsettle"? | -|---|---|---|---|---| -| C1 | `:1465` | `unsettleSession` | `registry.ts:2142` · `registerIpc.ts:6980` · `syncRemoteCommandService.ts:4101` · `ctoOperatorTools.ts:576` | yes | -| C2 | `:1551` | `unsettleSessions` | `registry.ts:2178` · `registerIpc.ts:7003` · `syncRemoteCommandService.ts:4135` | yes | -| C3 | `:1764` | `clearTurnStartMarkers` | `agentChatService.ts:36284`, `:36990` (turn start) · `ptyService.ts:4999` | **no** | -| C4 | `:1299` | `setLastOutputPreview` (`clearSettled`) | `agentChatService.ts:13024` · `ptyService.ts:4134` — **per output chunk** | **no** | -| C5 | `:1323` | `touchSessionActivity` | `ptyService.ts:4159` | **no** | -| C6 | `:1737` | `markLastTurnFailed` | `agentChatService.ts:13590` | **no** | -| C7 | `:1709` | `requestAttention` | `registry.ts:2059` (`ade chat ask`) | **no** | - -**Four of seven clearers are implicit.** C4 is on the hottest path in the -product — it runs per terminal output chunk. Any design that requires a hook, -a pre-read, or a second statement at every clear site pays that cost on C4. +| # | Method | Invoked by | Named "unsettle"? | +|---|---|---|---| +| C1 | `unsettleSession` | `registry.ts:2142` · `registerIpc.ts:6980` · `syncRemoteCommandService.ts:4101` · `ctoOperatorTools.ts:576` | yes | +| C2 | `unsettleSessions` | `registry.ts:2178` · `registerIpc.ts:7003` · `syncRemoteCommandService.ts:4135` | yes | +| C3 | `clearTurnStartMarkers` | `agentChatService.ts` (turn start) · `ptyService.ts:4999` | **no** | +| C4 | `setLastOutputPreview` (`clearSettled`) | `agentChatService.ts:13024` · `ptyService.ts:4134` — on the output path, throttled to ~one write/900 ms per PTY | **no** | +| C5 | `touchSessionActivity` | `ptyService.ts:4159` | **no** | +| C6 | `markLastTurnFailed` | `agentChatService.ts:13590` | **no** | +| C7 | `requestAttention` | `registry.ts:2059` (`ade chat ask`) | **no** | + +**Five of seven clearers are implicit.** C4 sits on the output path, the hottest +in the product. Any design that requires a hook, a pre-read, or a second +statement at every clear site pays that cost there. Two corrections to the +original framing, both from measuring rather than assuming: the DB write is +**throttled to roughly one per 900 ms per PTY** (`ptyService`'s +`updatePreviewThrottled`), not one per chunk; and the implemented bump is a +single `insert … on conflict` against a two-column, PK-keyed, non-replicated +table with no pre-read. See §3c-ii for the measurement. --- @@ -108,6 +141,48 @@ A settle then becomes: read revision `r₀` → tear down → write **conditiona on the revision still being `r₀`. One `where` clause replaces every ad-hoc guard, at every entry point, for free. +**As implemented (step 1).** `settleLifecycleWriter.ts` is the only writer of the +settle tuple, and its colocated test scans `apps/desktop/src` and +`apps/ade-cli/src` for any assignment outside that one file — the guarantee +belongs to the writer, so adding an eleventh path has to fail a test rather than +pass a review. It is a separate module precisely so the allowlist is a file +rather than a pair of offsets inside a 1800-line service. + +The revision lives in `session_lifecycle_revisions`, a local-only table added to +`LOCAL_ONLY_CRR_EXCLUDED_TABLES`, bumped by a single +`insert … on conflict do update set revision = revision + 1` immediately adjacent +to the column write. `AdeDb` exposes no transaction helper and an explicit +`BEGIN` could nest inside a caller's, so adjacency is what guarantees ordering: +the runtime is single-threaded, the bump cannot throw (it falls back to an +in-memory counter), and the revision is host-local, so no reader can observe the +column write without the bump. The failure direction that matters — a column +change the revision never saw — is the one this rules out. + +`sessionService.getSettleLifecycleRevision(sessionId)` is the read side. It +returns 0 for a session with no recorded mutation, which callers must treat as a +real value rather than as absent. + +**Exactly how strong the guarantee is.** Two gaps, both known, neither closed by +step 1: + +- **A sibling ADE process.** `kvDb` supports several processes against one + database, and the desktop main process and the CLI brain each build a + `sessionService` over it. Adjacency orders the column write and the bump + *within* a process; a sibling can read between them. The window is + microseconds, and closing it needs a transaction helper `AdeDb` does not have. + The read takes `max(table, in-process)` precisely so a sibling's higher value + is never lost. +- **A paired desktop peer over CRR.** Step 0 strips the settle columns from + inbound *phone* changesets, deliberately leaving desktop peers replicating — + they run this same chokepoint locally. But their write reaches this host + through `crsql_changes`, not through `writeSettleLifecycle`, so **this** host's + revision does not move for it. A revision-conditional apply is blind to a + remote-desktop settle landing mid-teardown. + +Step 3 must not assume the revision covers either case. The honest scope: the +revision detects every settle-lifecycle change made *by this host*, which is the +case the R1/R2/R6 races are actually about. + ### 3b. An explicit `settling` state The revision alone still leaves R2 (work stopped, settle abandoned). Add a @@ -301,6 +376,22 @@ firehose, the fallback is to keep the revision **in memory** on the host and accept that a mid-settle restart resolves to not-settled — which §3b already requires for the `settling` state anyway, so the two degrade identically. +**Measured (step 1), and the fallback was not needed.** Against ADE's actual +pragmas (WAL, `synchronous = NORMAL`), 20 000 iterations on a warmed connection: + +| | per write | +|---|---| +| `terminal_sessions` update alone | 1.7 µs | +| \+ the revision bump | **+6.6 µs** | +| (control) + a second `terminal_sessions` update instead | +10.7 µs | + +The bump is cheaper than any other second statement would be, because the table +is two columns wide and the write is a primary-key upsert. A first measurement +that omitted `synchronous = NORMAL` reported +47 µs; that number is an artifact +of per-statement fsync and does not describe ADE. The persisted table therefore +stays, with the in-process counter alongside it — see §3a, where that counter +turned out to be load-bearing for monotonicity rather than merely a fallback. + ### 3d. When teardown cannot confirm R5 is a product decision, not a mechanism. If a provider stop is unavailable, @@ -377,7 +468,7 @@ three, and that is why it produced a defect every round. pending-UI state instead, and the host drops those columns from inbound phone changesets. Until this landed, a revision-guarded write was defeatable by CRR merge, so the chokepoint would have provided a guarantee it did not have. -1. Land the chokepoint + lifecycle revision (3a) **alone**, with no teardown. +1. **Landed.** The chokepoint + lifecycle revision (3a), with no teardown. It is pure refactor with a testable invariant: no `settled_at` mutation outside one function, and every mutation bumps the revision. The revision goes in a local-only table (§3c-ii).