From 1f5452eaf51a8f501e55d14bdb48b58072301fd6 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 10 Aug 2026 21:41:03 -0400 Subject: [PATCH 1/7] Settle-writer chokepoint and lifecycle revision (settle teardown, step 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `settled_at` is written and cleared from ten call paths, five of the seven clearers not named "unsettle", 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. This lands the detector, with no teardown attached. **One writer.** `writeSettleLifecycle` is the only place the settle tuple is assigned; every call site now passes a typed intent (settle / clear / unsettle / override) plus its own non-settle columns. `settleLifecycleChokepoint.test.ts` scans the source and fails if any assignment appears outside the tuple generator — the guarantee belongs to the writer, so adding an eleventh path has to break a test rather than a review. **One revision.** `session_lifecycle_revisions`, a local-only table, bumped by a single `insert … on conflict` immediately adjacent to the column write. Local-only because `terminal_sessions` is a CRR whose settle row is rewritten per output chunk and cr-sqlite clocks are per column — a revision column would put a clock entry on the hottest write in the product for a value no other device can use. An in-memory counter backs it if the table write fails, degrading the same way the settling state will: a lost revision can only cause a settle to be re-taken, never a stale one to be applied. **Correction to the design doc, from doing the enumeration in code.** W3 (`setSettleOverride`) never assigns `settled_at` — it only reads it inside a `case`. But a `'settled'` pin makes a row read as settled anyway, so a revision keyed to `settled_at` alone would have been blind to it: finding 13 of §4 reappearing inside the inventory that was supposed to prevent it. The chokepoint owns the whole tuple instead. §1's counts are corrected with evidence (ten paths, not seven; five implicit clearers, not four). --- .../main/services/sessions/sessionService.ts | 373 +++++++++++------- .../settleLifecycleChokepoint.test.ts | 225 +++++++++++ apps/desktop/src/main/services/state/kvDb.ts | 23 ++ .../settle-teardown-design.md | 67 +++- 4 files changed, 538 insertions(+), 150 deletions(-) create mode 100644 apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index edec4d682..0bbc4b6e4 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -1,5 +1,5 @@ import fs from "node:fs"; -import type { AdeDb } from "../state/kvDb"; +import type { AdeDb, SqlValue } from "../state/kvDb"; import type { ClaudeSessionPointer, SessionAttentionSource, @@ -372,6 +372,133 @@ export function createSessionService({ db }: { db: AdeDb }) { * Shared skeleton for the single-session lifecycle mutators: trim, existence * probe, run the update, broadcast. Keeps every SQL literal at its call site. */ + // --------------------------------------------------------------------- + // 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. `sessionService.settleWriter.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. + // --------------------------------------------------------------------- + + /** What a settle-lifecycle mutation is asking for. */ + type SettleLifecycleIntent = + | { kind: "settle"; settledAt: string; source: SessionSettleSource } + /** Real activity: drop the whole declaration, pin included. */ + | { kind: "clear" } + /** Declared unsettle: drops a `'settled'` pin but preserves `'active'`. */ + | { kind: "unsettle" } + | { kind: "override"; value: string | null; source: SessionSettleSource }; + + 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 "clear": + return { sql: "settled_at = null, settle_override = null, settle_source = null", params: [] }; + case "unsettle": + 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], + }; + } + }; + + /** + * In-memory revisions, used when the local-only table is unavailable (an + * older database, or a write that failed). Degrades exactly like the settling + * state does: a restart resolves to "not settled", which is the safe + * direction — a lost revision can only cause a settle to be re-taken, never + * a stale one to be applied. + */ + const fallbackLifecycleRevisions = new Map(); + + const bumpLifecycleRevisions = (sessionIds: readonly string[]): void => { + for (const id of sessionIds) { + try { + // One atomic statement, PK lookup, local-only table. This runs on the + // per-output-chunk path (C4), so it must not grow a pre-read. + 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], + ); + } catch { + fallbackLifecycleRevisions.set(id, (fallbackLifecycleRevisions.get(id) ?? 0) + 1); + } + } + }; + + const readLifecycleRevision = (sessionId: string): number => { + const trimmed = sessionId.trim(); + if (!trimmed) return 0; + try { + const row = db.get<{ revision: number }>( + "select revision from session_lifecycle_revisions where session_id = ?", + [trimmed], + ); + if (row) return Number(row.revision) || 0; + } catch { + // fall through to the in-memory value + } + return fallbackLifecycleRevisions.get(trimmed) ?? 0; + }; + + /** + * The ONLY writer of the settle tuple. + * + * `extraSet` is for columns the caller owns and this function knows nothing + * about (a preview line, a failure timestamp). `where` scopes the update; + * `revisionIds` are the rows whose revision must move, which is normally the + * rows the `where` matches. + */ + const writeSettleLifecycle = (args: { + intent: SettleLifecycleIntent; + where: { sql: string; params: SqlValue[] }; + revisionIds: readonly string[]; + extraSet?: { sql: string; params: SqlValue[] }; + }): void => { + const tuple = settleTupleAssignment(args.intent); + const setClauses = args.extraSet ? [args.extraSet.sql, tuple.sql] : [tuple.sql]; + const setParams = args.extraSet ? [...args.extraSet.params, ...tuple.params] : [...tuple.params]; + db.run( + `update terminal_sessions set ${setClauses.join(", ")} where ${args.where.sql}`, + [...setParams, ...args.where.params], + ); + // 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 guarantees the + // ordering: the runtime is single-threaded 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. + bumpLifecycleRevisions(args.revisionIds); + }; + const mutateSessionMeta = (sessionId: string, run: (id: string) => void): boolean => { const trimmed = sessionId.trim(); if (!trimmed) return false; @@ -688,26 +815,22 @@ 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: { + sql: `${hasOutcome ? "status_note = ?," : ""} attention_requested_at = null, attention_message = null, attention_source = null`, + params: hasOutcome ? [normalizeSessionStatusNote(options.outcome)] : [], + }, + where: { + sql: `(settled_at is null or settle_override is not null) and id in (${updatePlaceholders})`, + params: [...newlySettled], + }, + revisionIds: newlySettled, + }); for (const id of newlySettled) { emitChanged({ sessionId: id, reason: "meta-updated" }); } @@ -1294,12 +1417,20 @@ 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: "clear" }, + extraSet: { sql: "last_output_preview = ?, last_output_at = ?", params: [preview, now] }, + where: { sql: "id = ?", params: [sessionId] }, + revisionIds: [sessionId], + }); }, /** @@ -1317,12 +1448,16 @@ 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: "clear" }, + extraSet: { sql: "last_output_at = ?", params: [at] }, + where: { sql: "id = ?", params: [sessionId] }, + revisionIds: [sessionId], + }); }, setSummary(sessionId: string, summary: string | null): void { @@ -1423,52 +1558,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: { + sql: `${outcome ? "status_note = ?," : ""} attention_requested_at = null, attention_message = null, attention_source = null`, + params: outcome ? [outcome] : [], + }, + where: { sql: "id = ?", params: [id] }, + revisionIds: [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: "unsettle" }, + where: { sql: "id = ?", params: [id] }, + revisionIds: [id], + }); }); return changed; }, @@ -1482,19 +1591,11 @@ 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 }, + where: { sql: "id = ?", params: [id] }, + revisionIds: [id], + }); }); }, @@ -1509,25 +1610,29 @@ 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" }, + where: { sql: `id in (${updatePlaceholders})`, params: [...present] }, + revisionIds: 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 readLifecycleRevision(sessionId); + }, + settleSessions(sessionIds: string[]): string[] { return settleMany(sessionIds); }, @@ -1545,16 +1650,11 @@ 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: "unsettle" }, + where: { sql: `id in (${placeholders})`, params: [...ids] }, + revisionIds: ids, + }); for (const id of ids) { emitChanged({ sessionId: id, reason: "meta-updated" }); } @@ -1700,19 +1800,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: "clear" }, + extraSet: { + sql: "attention_requested_at = ?, attention_message = ?, attention_source = ?", + params: [new Date().toISOString(), normalizeOptionalText(message, 500), source], + }, + where: { sql: "id = ?", params: [id] }, + revisionIds: [id], + }); wakeSnoozedRow(id, "needs_you"); }); }, @@ -1733,10 +1829,12 @@ 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: "clear" }, + extraSet: { sql: "last_turn_failed_at = ?", params: [failedAt] }, + where: { sql: "id = ?", params: [id] }, + revisionIds: [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 +1855,15 @@ 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: "clear" }, + extraSet: { + sql: "last_turn_failed_at = null, attention_requested_at = null, attention_message = null, attention_source = null", + params: [], + }, + where: { sql: "id = ?", params: [id] }, + revisionIds: [id], + }); }); return changed; }, diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts b/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts new file mode 100644 index 000000000..640064618 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts @@ -0,0 +1,225 @@ +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 SESSION_SERVICE = path.join(__dirname, "sessionService.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 chokepoint", () => { + 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 outside the chokepoint's own generator", () => { + const source = fs.readFileSync(SESSION_SERVICE, "utf8"); + const generatorStart = source.indexOf("const settleTupleAssignment"); + const generatorEnd = source.indexOf("const fallbackLifecycleRevisions"); + expect(generatorStart).toBeGreaterThan(-1); + expect(generatorEnd).toBeGreaterThan(generatorStart); + + const outsideGenerator = + source.slice(0, generatorStart) + source.slice(generatorEnd); + const assignments = outsideGenerator.match( + /\b(settled_at|settle_override|settle_source)\s*=(?!=)/g, + ) ?? []; + + expect(assignments).toEqual([]); + }); + + 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); + }); + + 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()); + + // 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 per-output-chunk write path. + const crr = db.get<{ present: number }>( + "select 1 as present from crsql_master where key = 'tbl_ver' and value like ?", + ["%session_lifecycle_revisions%"], + ); + expect(crr).toBeNull(); + + const table = db.get<{ name: string }>( + "select name from sqlite_master where type = 'table' and name = 'session_lifecycle_revisions'", + ); + expect(table?.name).toBe("session_lifecycle_revisions"); + }); +}); diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index de5e33d52..eca4cdb17 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -884,6 +884,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[] { @@ -3808,6 +3812,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(` diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index 0f2cb1401..f8a254ff4 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,10 +30,34 @@ 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` at +implementation of step 1. 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** (W2 has two + branches). 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`) @@ -40,7 +65,7 @@ whole problem: teardown was wired to the three obvious ones. |---|---|---|---| | 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 | +| W3 | `:1491`, `:1518` | `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`) @@ -54,9 +79,11 @@ whole problem: teardown was wired to the three obvious ones. | 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 +**Five 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. +a pre-read, or a second statement at every clear site pays that cost on C4. The +implemented bump is a single `insert … on conflict` against a two-column, +PK-keyed, non-replicated table, with no pre-read. --- @@ -108,6 +135,26 @@ 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).** `writeSettleLifecycle` in `sessionService` is the +only writer of the settle tuple, and `settleLifecycleChokepoint.test.ts` enforces +that by scanning the source for any assignment outside the tuple generator — the +guarantee belongs to the writer, so it has to be impossible to add an eleventh +path without the test failing. + +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. + ### 3b. An explicit `settling` state The revision alone still leaves R2 (work stopped, settle abandoned). Add a @@ -377,7 +424,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). From c067a0da1c7bc88ac3dc5253d3be070f15e5fa5e Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 10 Aug 2026 21:45:11 -0400 Subject: [PATCH 2/7] fix: make the lifecycle revision always advance, and never regress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two flaws in my own first cut, found by tracing the fallback rather than by a test failing. **It could go backwards.** Bumps landed in the table normally and in memory when the table write failed. If the table then started working it began its own count at 1 while memory held more, and the reader preferred the table — handing 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. **Taking the max fixed that but let it stall.** The table restarting at 1 under a higher memory value meant a real mutation could produce no observable change — a change the guard cannot see, which is the direction that actually hurts. So the in-process counter now moves on EVERY bump, not only on failure, and the read is the max of both stores. The table survives a restart; the counter guarantees advancement. Monotonic within a process and never regressing across a restart are the only two properties this value needs. Also fixes brittleness in the invariant test: it delimited the tuple generator by a neighbouring identifier, so renaming an unrelated const silently broke it. Explicit `settle-tuple-sql:start/end` sentinels now bound the region. --- .../main/services/sessions/sessionService.ts | 42 ++++++++++---- .../settleLifecycleChokepoint.test.ts | 55 +++++++++++++++++-- 2 files changed, 83 insertions(+), 14 deletions(-) diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index 0bbc4b6e4..7642a374c 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -401,6 +401,9 @@ export function createSessionService({ db }: { db: AdeDb }) { | { kind: "unsettle" } | { kind: "override"; value: string | null; source: SessionSettleSource }; + // settle-tuple-sql:start — the ONLY region allowed to assign the settle tuple. + // `settleLifecycleChokepoint.test.ts` scans for assignments outside these + // markers, so do not remove them or widen the region to cover other code. const settleTupleAssignment = (intent: SettleLifecycleIntent): { sql: string; params: SqlValue[] } => { switch (intent.kind) { case "settle": @@ -428,17 +431,27 @@ export function createSessionService({ db }: { db: AdeDb }) { } }; + // settle-tuple-sql:end + /** - * In-memory revisions, used when the local-only table is unavailable (an - * older database, or a write that failed). Degrades exactly like the settling - * state does: a restart resolves to "not settled", which is the safe - * direction — a lost revision can only cause a settle to be re-taken, never - * a stale one to be applied. + * Per-process revision counter, incremented on EVERY bump — not only when the + * table write fails. + * + * The two stores answer different questions and both are needed. The table + * survives a restart; this map guarantees the value always moves. Bumping only + * on failure would let the revision stall: after a failed write the table is + * behind, and when it starts working again it restarts its own count at 1, so + * a real mutation could produce no observable change — a change the guard + * cannot see, which is the failure direction that matters. + * + * Bounded by the sessions this process has touched, same order as the row set + * it shadows. */ - const fallbackLifecycleRevisions = new Map(); + const inProcessLifecycleRevisions = new Map(); const bumpLifecycleRevisions = (sessionIds: readonly string[]): void => { for (const id of sessionIds) { + inProcessLifecycleRevisions.set(id, (inProcessLifecycleRevisions.get(id) ?? 0) + 1); try { // One atomic statement, PK lookup, local-only table. This runs on the // per-output-chunk path (C4), so it must not grow a pre-read. @@ -448,7 +461,8 @@ export function createSessionService({ db }: { db: AdeDb }) { [id], ); } catch { - fallbackLifecycleRevisions.set(id, (fallbackLifecycleRevisions.get(id) ?? 0) + 1); + // The in-process counter already moved, so the guard still works for the + // lifetime of this process; only restart-survival is lost. } } }; @@ -456,16 +470,24 @@ export function createSessionService({ db }: { db: AdeDb }) { const readLifecycleRevision = (sessionId: string): number => { const trimmed = sessionId.trim(); if (!trimmed) return 0; + let persisted = 0; try { const row = db.get<{ revision: number }>( "select revision from session_lifecycle_revisions where session_id = ?", [trimmed], ); - if (row) return Number(row.revision) || 0; + persisted = row ? Number(row.revision) || 0 : 0; } catch { - // fall through to the in-memory value + persisted = 0; } - return fallbackLifecycleRevisions.get(trimmed) ?? 0; + // The MAXIMUM of the two stores, never whichever answered. After a restart + // the table is ahead; after a failed write the in-process counter is. Taking + // either one alone could hand back a LOWER number than a previous call did, + // and a revision that moves backwards is worse than no revision at all — it + // lets a stale settle match a token it should have missed. Monotonic within + // a process, and never regressing across a restart, are the only two + // properties this value has to have. + return Math.max(persisted, inProcessLifecycleRevisions.get(trimmed) ?? 0); }; /** diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts b/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts index 640064618..ac37a849d 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts @@ -84,10 +84,12 @@ describe("settle-lifecycle chokepoint", () => { */ it("has no settle-tuple assignment outside the chokepoint's own generator", () => { const source = fs.readFileSync(SESSION_SERVICE, "utf8"); - const generatorStart = source.indexOf("const settleTupleAssignment"); - const generatorEnd = source.indexOf("const fallbackLifecycleRevisions"); - expect(generatorStart).toBeGreaterThan(-1); - expect(generatorEnd).toBeGreaterThan(generatorStart); + // Explicit sentinels, not neighbouring identifiers: an unrelated rename must + // not silently turn this invariant into a no-op. + const generatorStart = source.indexOf("settle-tuple-sql:start"); + const generatorEnd = source.indexOf("settle-tuple-sql:end"); + expect(generatorStart, "start sentinel missing").toBeGreaterThan(-1); + expect(generatorEnd, "end sentinel missing").toBeGreaterThan(generatorStart); const outsideGenerator = source.slice(0, generatorStart) + source.slice(generatorEnd); @@ -203,6 +205,51 @@ describe("settle-lifecycle chokepoint", () => { 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); + }); + 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); From 33ee8d6d47f473ddc0fe2538852846c61fc8b5d0 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 10 Aug 2026 21:59:09 -0400 Subject: [PATCH 3/7] quality: close the chokepoint's escape hatches, and fix a test that asserted nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dual-track review of step 1. Track A confirmed all twelve converted statements are semantically equivalent per path, the CRR exclusion is genuinely applied, and `migrate()` reaches existing databases. The findings worth naming: **The CRR test asserted nothing.** It queried `crsql_master` for a `tbl_ver` key that does not exist in this cr-sqlite build, so it returned null for every table — including `terminal_sessions`, which IS a CRR. It now checks for the `__crsql_clock` sidecar using the repo's own idiom, with a positive control against `terminal_sessions` so the assertion can fail. This is the design's own finding 13 landing on the test meant to enforce it. **The invariant scan was single-file and matched comments.** It read only `sessionService.ts`, so an eleventh writer in a new service would have passed clean. It now walks `apps/desktop/src` and `apps/ade-cli/src`, and strips comments first — the web adapter documents the host's SQL in JSDoc, which the first widened version flagged as three offenders. **The writer still exposed raw SQL.** `where` and `revisionIds` could disagree silently; `extraSet` was a SQL fragment that could have carried a tuple column straight past the chokepoint, with the comma inside a ternary. Now one `sessionIds` array scopes the update AND the bump, and `extraSet` is a `Partial>` — writing a tuple column through it is a compile error. `settleMany` keeps its guard predicate as a parameterless `guard`: review argued it was provably redundant, but ADE runs several processes against one database, so another process can invalidate it between the select and the update. **The revision counted attempts, not changes.** `AdeDb.run` discarded the row count. Added `runChanged` (additive; `run` still returns void) and the bump now requires a matched row, so a no-op write cannot spend a revision. Also: revision rows and their in-process entries are reaped on `deleteSession`; the dual-store comment now gives the real reason (cross-process visibility, not just restart survival); §1's line numbers are replaced with method names after the refactor invalidated every one of them; and the "per output chunk" framing is corrected to the measured ~900 ms throttle. Records two gaps step 3 must not assume away: a sibling ADE process can read between the column write and the bump, and a paired desktop peer's settle arrives via `crsql_changes` without moving this host's revision. --- .../services/history/operationService.test.ts | 4 + .../onboarding/onboardingService.test.ts | 1 + .../main/services/sessions/sessionService.ts | 210 +++++++++++------- .../settleLifecycleChokepoint.test.ts | 94 +++++--- apps/desktop/src/main/services/state/kvDb.ts | 11 + .../settle-teardown-design.md | 95 +++++--- 6 files changed, 276 insertions(+), 139 deletions(-) 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/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 7642a374c..2fb240371 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -368,16 +368,12 @@ export function createSessionService({ db }: { db: AdeDb }) { const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); - /** - * Shared skeleton for the single-session lifecycle mutators: trim, existence - * probe, run the update, broadcast. Keeps every SQL literal at its call site. - */ // --------------------------------------------------------------------- // 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. `sessionService.settleWriter.test.ts` + // 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 @@ -395,11 +391,11 @@ export function createSessionService({ db }: { db: AdeDb }) { /** What a settle-lifecycle mutation is asking for. */ type SettleLifecycleIntent = | { kind: "settle"; settledAt: string; source: SessionSettleSource } - /** Real activity: drop the whole declaration, pin included. */ - | { kind: "clear" } + /** Real activity: drop the whole declaration, keep-active pin included. */ + | { kind: "clearOnActivity" } /** Declared unsettle: drops a `'settled'` pin but preserves `'active'`. */ - | { kind: "unsettle" } - | { kind: "override"; value: string | null; source: SessionSettleSource }; + | { kind: "unsettleDeclared" } + | { kind: "override"; value: SessionSettleOverride | null; source: SessionSettleSource }; // settle-tuple-sql:start — the ONLY region allowed to assign the settle tuple. // `settleLifecycleChokepoint.test.ts` scans for assignments outside these @@ -411,9 +407,9 @@ export function createSessionService({ db }: { db: AdeDb }) { sql: "settled_at = coalesce(settled_at, ?), settle_override = null, settle_source = ?", params: [intent.settledAt, intent.source], }; - case "clear": + case "clearOnActivity": return { sql: "settled_at = null, settle_override = null, settle_source = null", params: [] }; - case "unsettle": + case "unsettleDeclared": return { sql: "settled_at = null, " @@ -437,12 +433,15 @@ export function createSessionService({ db }: { db: AdeDb }) { * Per-process revision counter, incremented on EVERY bump — not only when the * table write fails. * - * The two stores answer different questions and both are needed. The table - * survives a restart; this map guarantees the value always moves. Bumping only - * on failure would let the revision stall: after a failed write the table is - * behind, and when it starts working again it restarts its own count at 1, so - * a real mutation could produce no observable change — a change the guard - * cannot see, which is the failure direction that matters. + * 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. @@ -461,8 +460,9 @@ export function createSessionService({ db }: { db: AdeDb }) { [id], ); } catch { - // The in-process counter already moved, so the guard still works for the - // lifetime of this process; only restart-survival is lost. + // The in-process counter already moved, so the guard still holds within + // this process; restart survival and cross-process visibility are what + // a failed write costs. } } }; @@ -481,46 +481,81 @@ export function createSessionService({ db }: { db: AdeDb }) { persisted = 0; } // The MAXIMUM of the two stores, never whichever answered. After a restart - // the table is ahead; after a failed write the in-process counter is. Taking - // either one alone could hand back a LOWER number than a previous call did, - // and a revision that moves backwards is worse than no revision at all — it - // lets a stale settle match a token it should have missed. Monotonic within - // a process, and never regressing across a restart, are the only two - // properties this value has to have. + // — 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); }; + /** + * 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. + */ + type SettleExtraColumn = + | "status_note" + | "attention_requested_at" + | "attention_message" + | "attention_source" + | "last_output_preview" + | "last_output_at" + | "last_turn_failed_at"; + /** * The ONLY writer of the settle tuple. * - * `extraSet` is for columns the caller owns and this function knows nothing - * about (a preview line, a failure timestamp). `where` scopes the update; - * `revisionIds` are the rows whose revision must move, which is normally the - * rows the `where` matches. + * `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; - where: { sql: string; params: SqlValue[] }; - revisionIds: readonly string[]; - extraSet?: { sql: string; params: SqlValue[] }; + 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 setClauses = args.extraSet ? [args.extraSet.sql, tuple.sql] : [tuple.sql]; - const setParams = args.extraSet ? [...args.extraSet.params, ...tuple.params] : [...tuple.params]; - db.run( - `update terminal_sessions set ${setClauses.join(", ")} where ${args.where.sql}`, - [...setParams, ...args.where.params], + 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 actually moved a row bumps. A revision that counted + // attempts would reject later decisions for writes that changed nothing — + // and, on the throttled preview path, would churn against rows that are + // already unsettled. + 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 guarantees the - // ordering: the runtime is single-threaded 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. - bumpLifecycleRevisions(args.revisionIds); + // 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); }; + /** + * Shared skeleton for the single-session lifecycle mutators: trim, existence + * 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(); if (!trimmed) return false; @@ -844,14 +879,13 @@ export function createSessionService({ db }: { db: AdeDb }) { source: options.source ?? "user", }, extraSet: { - sql: `${hasOutcome ? "status_note = ?," : ""} attention_requested_at = null, attention_message = null, attention_source = null`, - params: hasOutcome ? [normalizeSessionStatusNote(options.outcome)] : [], - }, - where: { - sql: `(settled_at is null or settle_override is not null) and id in (${updatePlaceholders})`, - params: [...newlySettled], + ...(hasOutcome ? { status_note: normalizeSessionStatusNote(options.outcome) } : {}), + attention_requested_at: null, + attention_message: null, + attention_source: null, }, - revisionIds: newlySettled, + guard: "(settled_at is null or settle_override is not null)", + sessionIds: newlySettled, }); for (const id of newlySettled) { emitChanged({ sessionId: id, reason: "meta-updated" }); @@ -1448,10 +1482,9 @@ export function createSessionService({ db }: { db: AdeDb }) { return; } writeSettleLifecycle({ - intent: { kind: "clear" }, - extraSet: { sql: "last_output_preview = ?, last_output_at = ?", params: [preview, now] }, - where: { sql: "id = ?", params: [sessionId] }, - revisionIds: [sessionId], + intent: { kind: "clearOnActivity" }, + extraSet: { last_output_preview: preview, last_output_at: now }, + sessionIds: [sessionId], }); }, @@ -1475,10 +1508,9 @@ export function createSessionService({ db }: { db: AdeDb }) { return; } writeSettleLifecycle({ - intent: { kind: "clear" }, - extraSet: { sql: "last_output_at = ?", params: [at] }, - where: { sql: "id = ?", params: [sessionId] }, - revisionIds: [sessionId], + intent: { kind: "clearOnActivity" }, + extraSet: { last_output_at: at }, + sessionIds: [sessionId], }); }, @@ -1583,11 +1615,12 @@ export function createSessionService({ db }: { db: AdeDb }) { writeSettleLifecycle({ intent: { kind: "settle", settledAt, source: opts.source ?? "user" }, extraSet: { - sql: `${outcome ? "status_note = ?," : ""} attention_requested_at = null, attention_message = null, attention_source = null`, - params: outcome ? [outcome] : [], + ...(outcome ? { status_note: outcome } : {}), + attention_requested_at: null, + attention_message: null, + attention_source: null, }, - where: { sql: "id = ?", params: [id] }, - revisionIds: [id], + sessionIds: [id], }); }); }, @@ -1596,9 +1629,8 @@ export function createSessionService({ db }: { db: AdeDb }) { unsettleSession(sessionId: string): boolean { const changed = mutateSessionMeta(sessionId, (id) => { writeSettleLifecycle({ - intent: { kind: "unsettle" }, - where: { sql: "id = ?", params: [id] }, - revisionIds: [id], + intent: { kind: "unsettleDeclared" }, + sessionIds: [id], }); }); return changed; @@ -1615,8 +1647,7 @@ export function createSessionService({ db }: { db: AdeDb }) { return mutateSessionMeta(sessionId, (id) => { writeSettleLifecycle({ intent: { kind: "override", value: normalized, source: normalizedSource }, - where: { sql: "id = ?", params: [id] }, - revisionIds: [id], + sessionIds: [id], }); }); }, @@ -1634,8 +1665,7 @@ export function createSessionService({ db }: { db: AdeDb }) { const updatePlaceholders = present.map(() => "?").join(", "); writeSettleLifecycle({ intent: { kind: "override", value: normalized, source: "user" }, - where: { sql: `id in (${updatePlaceholders})`, params: [...present] }, - revisionIds: present, + sessionIds: present, }); for (const id of present) { emitChanged({ sessionId: id, reason: "meta-updated" }); @@ -1673,9 +1703,8 @@ export function createSessionService({ db }: { db: AdeDb }) { if (!ids.length) return; const placeholders = ids.map(() => "?").join(", "); writeSettleLifecycle({ - intent: { kind: "unsettle" }, - where: { sql: `id in (${placeholders})`, params: [...ids] }, - revisionIds: ids, + intent: { kind: "unsettleDeclared" }, + sessionIds: ids, }); for (const id of ids) { emitChanged({ sessionId: id, reason: "meta-updated" }); @@ -1823,13 +1852,13 @@ export function createSessionService({ db }: { db: AdeDb }) { ): boolean { return mutateSessionMeta(sessionId, (id) => { writeSettleLifecycle({ - intent: { kind: "clear" }, + intent: { kind: "clearOnActivity" }, extraSet: { - sql: "attention_requested_at = ?, attention_message = ?, attention_source = ?", - params: [new Date().toISOString(), normalizeOptionalText(message, 500), source], + attention_requested_at: new Date().toISOString(), + attention_message: normalizeOptionalText(message, 500), + attention_source: source, }, - where: { sql: "id = ?", params: [id] }, - revisionIds: [id], + sessionIds: [id], }); wakeSnoozedRow(id, "needs_you"); }); @@ -1852,10 +1881,9 @@ export function createSessionService({ db }: { db: AdeDb }) { // settled/failed mutually exclusive at write time, so every surface's // precedence order agrees by construction. writeSettleLifecycle({ - intent: { kind: "clear" }, - extraSet: { sql: "last_turn_failed_at = ?", params: [failedAt] }, - where: { sql: "id = ?", params: [id] }, - revisionIds: [id], + 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. @@ -1878,13 +1906,14 @@ export function createSessionService({ db }: { db: AdeDb }) { clearTurnStartMarkers(sessionId: string): boolean { const changed = mutateSessionMeta(sessionId, (id) => { writeSettleLifecycle({ - intent: { kind: "clear" }, + intent: { kind: "clearOnActivity" }, extraSet: { - sql: "last_turn_failed_at = null, attention_requested_at = null, attention_message = null, attention_source = null", - params: [], + last_turn_failed_at: null, + attention_requested_at: null, + attention_message: null, + attention_source: null, }, - where: { sql: "id = ?", params: [id] }, - revisionIds: [id], + sessionIds: [id], }); }); return changed; @@ -1899,6 +1928,15 @@ 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. + 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); emitChanged({ sessionId: trimmed, reason: "deleted" }); return true; }, diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts b/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts index ac37a849d..6730a2707 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts @@ -82,22 +82,54 @@ describe("settle-lifecycle chokepoint", () => { * rather than trusting review, because the failure mode is someone adding an * eleventh path in six months. */ - it("has no settle-tuple assignment outside the chokepoint's own generator", () => { - const source = fs.readFileSync(SESSION_SERVICE, "utf8"); - // Explicit sentinels, not neighbouring identifiers: an unrelated rename must - // not silently turn this invariant into a no-op. - const generatorStart = source.indexOf("settle-tuple-sql:start"); - const generatorEnd = source.indexOf("settle-tuple-sql:end"); - expect(generatorStart, "start sentinel missing").toBeGreaterThan(-1); - expect(generatorEnd, "end sentinel missing").toBeGreaterThan(generatorStart); - - const outsideGenerator = - source.slice(0, generatorStart) + source.slice(generatorEnd); - const assignments = outsideGenerator.match( - /\b(settled_at|settle_override|settle_source)\s*=(?!=)/g, - ) ?? []; - - expect(assignments).toEqual([]); + 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) if (fs.existsSync(root)) walk(root); + expect(files.length).toBeGreaterThan(100); + + const offenders: string[] = []; + for (const file of files) { + let source = fs.readFileSync(file, "utf8"); + if (file === SESSION_SERVICE) { + // Explicit sentinels, not neighbouring identifiers: an unrelated rename + // must not silently turn this invariant into a no-op. + const start = source.indexOf("settle-tuple-sql:start"); + const end = source.indexOf("settle-tuple-sql:end"); + expect(start, "start sentinel missing").toBeGreaterThan(-1); + expect(end, "end sentinel missing").toBeGreaterThan(start); + source = source.slice(0, start) + source.slice(end); + } + // 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*=(?!=)/g) ?? []) { + offenders.push(`${path.relative(roots[0], file)}: ${match}`); + } + } + + expect(offenders).toEqual([]); }); it("bumps the revision on every settle-lifecycle path", async () => { @@ -255,18 +287,26 @@ describe("settle-lifecycle chokepoint", () => { 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`], + ); + // 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 per-output-chunk write path. - const crr = db.get<{ present: number }>( - "select 1 as present from crsql_master where key = 'tbl_ver' and value like ?", - ["%session_lifecycle_revisions%"], - ); - expect(crr).toBeNull(); - - const table = db.get<{ name: string }>( - "select name from sqlite_master where type = 'table' and name = 'session_lifecycle_revisions'", - ); - expect(table?.name).toBe("session_lifecycle_revisions"); + // per-column clock entry to the throttled preview write path. + expect(clockTable("session_lifecycle_revisions")).toBeNull(); + // Positive control. Without it this assertion passes for a table that does + // not exist, or if the clock naming convention ever changes — which is + // exactly how the first version of this test managed to assert nothing. + 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/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index eca4cdb17..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[]; @@ -1717,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[]; }; @@ -1797,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); }, @@ -4141,6 +4150,7 @@ export async function openKvDb( }; const run = crrAwareDb.run; + const runChanged = crrAwareDb.runChanged; const all = crrAwareDb.all; const get = crrAwareDb.get; @@ -4650,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/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index f8a254ff4..2d921547f 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -30,18 +30,21 @@ Companion reading: `README.md` (canonical phase, settle semantics), ## 1. Every path that writes or clears `settled_at` -Verified against `apps/desktop/src/main/services/sessions/sessionService.ts` at -implementation of step 1. The counts below correct the ones this section -originally carried; the shape of the argument is unchanged, and the corrections -make it stronger. +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** (W2 has two - branches). W3 is the exception: see 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 @@ -61,29 +64,32 @@ The chokepoint therefore owns the whole **settle tuple** (`settled_at`, ### 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` — 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 | +| # | 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** | - -**Five 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. The -implemented bump is a single `insert … on conflict` against a two-column, -PK-keyed, non-replicated table, with no pre-read. +| # | 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. --- @@ -155,6 +161,27 @@ change the revision never saw — is the one this rules out. 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 @@ -348,6 +375,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, From a00e7f423b3f899d682a7e0b413114092e1fdaa1 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 10 Aug 2026 22:10:47 -0400 Subject: [PATCH 4/7] test: extract the writer so the invariant is a file boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sessions folder was over the per-folder test budget, and my new file was the reason: it was named after a concept rather than a unit, which is the pattern the budget rule exists to catch. Extracting `settleLifecycleWriter.ts` resolves that properly instead of adding an exception. The test becomes a plain colocated `settleLifecycleWriter.test.ts` next to the module it covers, matching the four files already in the folder, and `sessionService.ts` drops 1945 → 1817 lines. It also makes the invariant stronger. The source scan no longer allowlists a region delimited by string offsets inside a 1800-line service — it allowlists a FILE, which no future reordering can silently widen. Verified non-vacuous: re-adding `settled_at = null` to a raw statement in `sessionService.ts` fails the test and names the offending file. Docs: the writer is in the terminals source map with the exact scope of what the revision detects, and §3a records the file-boundary allowlist. --- .../main/services/sessions/sessionService.ts | 198 +--------------- ....test.ts => settleLifecycleWriter.test.ts} | 20 +- .../sessions/settleLifecycleWriter.ts | 218 ++++++++++++++++++ .../features/terminals-and-sessions/README.md | 10 + .../settle-teardown-design.md | 11 +- 5 files changed, 251 insertions(+), 206 deletions(-) rename apps/desktop/src/main/services/sessions/{settleLifecycleChokepoint.test.ts => settleLifecycleWriter.test.ts} (94%) create mode 100644 apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index 2fb240371..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, SqlValue } from "../state/kvDb"; +import type { AdeDb } from "../state/kvDb"; +import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; import type { ClaudeSessionPointer, SessionAttentionSource, @@ -368,187 +369,11 @@ export function createSessionService({ db }: { db: AdeDb }) { const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); - // --------------------------------------------------------------------- - // 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. - // --------------------------------------------------------------------- - - /** What a settle-lifecycle mutation is asking for. */ - 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 }; - - // settle-tuple-sql:start — the ONLY region allowed to assign the settle tuple. - // `settleLifecycleChokepoint.test.ts` scans for assignments outside these - // markers, so do not remove them or widen the region to cover other code. - 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], - }; - } - }; - - // settle-tuple-sql:end - - /** - * 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 bumpLifecycleRevisions = (sessionIds: readonly string[]): void => { - for (const id of sessionIds) { - inProcessLifecycleRevisions.set(id, (inProcessLifecycleRevisions.get(id) ?? 0) + 1); - try { - // One atomic statement, PK lookup, local-only table. This runs on the - // per-output-chunk path (C4), so it must not grow a pre-read. - 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], - ); - } catch { - // The in-process counter already moved, so the guard still holds within - // this process; restart survival and cross-process visibility are what - // a failed write costs. - } - } - }; - - const readLifecycleRevision = (sessionId: string): number => { - const trimmed = sessionId.trim(); - if (!trimmed) return 0; - let persisted = 0; - try { - const row = db.get<{ revision: number }>( - "select revision from session_lifecycle_revisions where session_id = ?", - [trimmed], - ); - persisted = row ? Number(row.revision) || 0 : 0; - } 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); - }; - - /** - * 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. - */ - type SettleExtraColumn = - | "status_note" - | "attention_requested_at" - | "attention_message" - | "attention_source" - | "last_output_preview" - | "last_output_at" - | "last_turn_failed_at"; - - /** - * 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 actually moved a row bumps. A revision that counted - // attempts would reject later decisions for writes that changed nothing — - // and, on the throttled preview path, would churn against rows that are - // already unsettled. - 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); - }; + // 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 @@ -1682,7 +1507,7 @@ export function createSessionService({ db }: { db: AdeDb }) { * this session", which a caller must treat as a real value, not as absent. */ getSettleLifecycleRevision(sessionId: string): number { - return readLifecycleRevision(sessionId); + return settleLifecycle.readRevision(sessionId); }, settleSessions(sessionIds: string[]): string[] { @@ -1931,12 +1756,7 @@ export function createSessionService({ db }: { db: AdeDb }) { // 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. - 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); + settleLifecycle.forget(trimmed); emitChanged({ sessionId: trimmed, reason: "deleted" }); return true; }, diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts similarity index 94% rename from apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts rename to apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts index 6730a2707..5fda24b67 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleChokepoint.test.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts @@ -19,7 +19,7 @@ import { createSessionService } from "./sessionService"; * conditional on. */ -const SESSION_SERVICE = path.join(__dirname, "sessionService.ts"); +const WRITER_MODULE = path.join(__dirname, "settleLifecycleWriter.ts"); function createLogger() { return { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} } as const; @@ -50,7 +50,7 @@ function insertProjectGraph(db: Awaited>) { ); } -describe("settle-lifecycle chokepoint", () => { +describe("settle-lifecycle writer", () => { const activeDisposers: Array<() => Promise> = []; afterEach(async () => { @@ -106,16 +106,12 @@ describe("settle-lifecycle chokepoint", () => { const offenders: string[] = []; for (const file of files) { - let source = fs.readFileSync(file, "utf8"); - if (file === SESSION_SERVICE) { - // Explicit sentinels, not neighbouring identifiers: an unrelated rename - // must not silently turn this invariant into a no-op. - const start = source.indexOf("settle-tuple-sql:start"); - const end = source.indexOf("settle-tuple-sql:end"); - expect(start, "start sentinel missing").toBeGreaterThan(-1); - expect(end, "end sentinel missing").toBeGreaterThan(start); - source = source.slice(0, start) + source.slice(end); - } + // 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. 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..796e26264 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts @@ -0,0 +1,218 @@ +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 bumpLifecycleRevisions = (sessionIds: readonly string[]): void => { + for (const id of sessionIds) { + inProcessLifecycleRevisions.set(id, (inProcessLifecycleRevisions.get(id) ?? 0) + 1); + try { + // One atomic statement, PK lookup, local-only table. This runs on the + // per-output-chunk path (C4), so it must not grow a pre-read. + 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], + ); + } catch { + // The in-process counter already moved, so the guard still holds within + // this process; restart survival and cross-process visibility are what + // a failed write costs. + } + } + }; + + const readLifecycleRevision = (sessionId: string): number => { + const trimmed = sessionId.trim(); + if (!trimmed) return 0; + let persisted = 0; + try { + const row = db.get<{ revision: number }>( + "select revision from session_lifecycle_revisions where session_id = ?", + [trimmed], + ); + persisted = row ? Number(row.revision) || 0 : 0; + } 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 actually moved a row bumps. A revision that counted + // attempts would reject later decisions for writes that changed nothing — + // and, on the throttled preview path, would churn against rows that are + // already unsettled. + 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/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 2d921547f..c1d831d35 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -141,11 +141,12 @@ 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).** `writeSettleLifecycle` in `sessionService` is the -only writer of the settle tuple, and `settleLifecycleChokepoint.test.ts` enforces -that by scanning the source for any assignment outside the tuple generator — the -guarantee belongs to the writer, so it has to be impossible to add an eleventh -path without the test failing. +**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 From 2679aaff5f97b11f8d34f082d334905764483f7f Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 10 Aug 2026 22:25:46 -0400 Subject: [PATCH 5/7] fix(test): the CRR assertion assumed cr-sqlite is always loaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught it: cr-sqlite ships macOS-only binaries, so on the Linux shards the extension is absent and NO table has a `__crsql_clock` sidecar. The positive control I added to stop this test being vacuous was itself the thing that failed. Now the strong pair — token excluded, `terminal_sessions` present — runs where CRR is actually loaded, and is explicitly skipped with the reason where no table has a clock and the assertion could not go red anyway. Same failure mode as the version this replaced, one level up: an assertion that cannot fail is not coverage, and neither is one that fails for the environment rather than the code. --- .../sessions/settleLifecycleWriter.test.ts | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts index 5fda24b67..52df14a65 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts @@ -289,14 +289,23 @@ describe("settle-lifecycle writer", () => { [`${table}__crsql_clock`], ); - // 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. Without it this assertion passes for a table that does - // not exist, or if the clock naming convention ever changes — which is - // exactly how the first version of this test managed to assert nothing. - expect(clockTable("terminal_sessions")).not.toBeNull(); + // 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 }>( From 14f13cbf99d1d9be773b40f4669ff49b83e53d03 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 10 Aug 2026 22:48:28 -0400 Subject: [PATCH 6/7] fix: a failed token write must still outrun a sibling process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings (Codex), all verified. **P1 — a real mutation could be invisible.** Another ADE process can push the persisted revision past this process's private counter. If a local mutation's token write then failed, incrementing the private counter alone left `max(persisted, inProcess)` UNCHANGED across a real change — and a teardown holding that token would accept a decision the world had already invalidated. The failure branch now anchors to `max(inProcess, persisted)` before incrementing, so it always outruns what a reader would have seen. The read stays off the output path: only the rare failure branch reads. Reproduced before fixing and pinned by `still advances when a sibling process is ahead and the local write fails` — reverting the anchor fails it with "expected 5 to be greater than 5". **P2 — bulk deletes orphaned tokens.** `laneService` deletes sessions by lane, never touching the token table, which has no foreign key. Swept by absence rather than by id list, so the fix covers that path and any future one — a targeted cascade is what keeps re-introducing this class. **P2 — the scan was case-sensitive.** `SET SETTLED_AT = NULL` is valid SQL and would have passed the load-bearing invariant test. Now case-insensitive, with an uppercase fixture asserting the matcher still bites. --- .../src/main/services/lanes/laneService.ts | 7 +++ .../sessions/settleLifecycleWriter.test.ts | 56 ++++++++++++++++++- .../sessions/settleLifecycleWriter.ts | 38 +++++++++---- 3 files changed, 89 insertions(+), 12 deletions(-) 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/sessions/settleLifecycleWriter.test.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts index 52df14a65..3c95583b8 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts @@ -120,12 +120,17 @@ describe("settle-lifecycle writer", () => { .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*=(?!=)/g) ?? []) { + 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 () => { @@ -278,6 +283,55 @@ describe("settle-lifecycle writer", () => { 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); + }); + 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); diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts index 796e26264..ed8deb333 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts @@ -112,21 +112,41 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { */ 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) { - inProcessLifecycleRevisions.set(id, (inProcessLifecycleRevisions.get(id) ?? 0) + 1); try { - // One atomic statement, PK lookup, local-only table. This runs on the - // per-output-chunk path (C4), so it must not grow a pre-read. + // 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 in-process counter already moved, so the guard still holds within - // this process; restart survival and cross-process visibility are what - // a failed write costs. + // 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); } } }; @@ -136,11 +156,7 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { if (!trimmed) return 0; let persisted = 0; try { - const row = db.get<{ revision: number }>( - "select revision from session_lifecycle_revisions where session_id = ?", - [trimmed], - ); - persisted = row ? Number(row.revision) || 0 : 0; + persisted = persistedRevision(trimmed); } catch { persisted = 0; } From cb3d387ad6674e9cdde72ecf01e08467b293ea69 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 10 Aug 2026 23:05:15 -0400 Subject: [PATCH 7/7] review: assert both scan roots; keep no-op bumps, with the reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings. **Scan roots** — `fs.existsSync` skipped a missing root silently, and the `> 100 files` floor is satisfied by `apps/desktop/src` alone, so renaming `apps/ade-cli/src` would have quietly retired half the invariant. Both roots are now asserted. **No-op clears spend a revision — accepted, not fixed.** The finding is correct that `sqlite3_changes` counts matched rows rather than differing values, so re-clearing an already-clear tuple advances the revision. I implemented the suggested per-intent predicate and reverted it: putting it in the WHERE gates the caller's own columns too, so `setLastOutputPreview` stopped writing the preview entirely. Four tests caught it immediately. Doing it properly needs either two statements on the output path or a pre-read, both of which the design rules out. 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 the mechanism that stops a session's own idle output invalidating its teardown, which is the concern behind the finding. Now documented in the writer and pinned by a test that asserts both halves: the revision does move, and the caller's columns still land. --- .../sessions/settleLifecycleWriter.test.ts | 26 ++++++++++++++++++- .../sessions/settleLifecycleWriter.ts | 15 ++++++++--- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts index 3c95583b8..3143df0b7 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts @@ -101,7 +101,11 @@ describe("settle-lifecycle writer", () => { } } }; - for (const root of roots) if (fs.existsSync(root)) walk(root); + 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[] = []; @@ -332,6 +336,26 @@ describe("settle-lifecycle writer", () => { ).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); diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts index ed8deb333..d51c8c60a 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts @@ -201,10 +201,17 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { `update terminal_sessions set ${setClauses.join(", ")} where ${where}`, [...setParams, ...ids], ); - // Only a write that actually moved a row bumps. A revision that counted - // attempts would reject later decisions for writes that changed nothing — - // and, on the throttled preview path, would churn against rows that are - // already unsettled. + // 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