From 03a29bae613943da1774e7913575df1d706c0c05 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 03:53:10 -0400 Subject: [PATCH 01/14] feat: attach real settle teardown, and reconcile peer settle-tuple writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 3 of the settle-teardown design. The seam is async now. Step 2 shipped a branded synchronous return type that made an awaited teardown a compile error; real stops are async, so that guard had to go. It was a tripwire, not an obstacle: it existed because bolting a deferred teardown onto a synchronous write is what produced a P1 in each of #1059's six rounds, and what changed is the machinery under it. The settling window is exclusive, abortable and in-memory, which is exactly what makes it safe to HOLD across an await — the revision re-check and abort check after the await are the suspension-point guards, and the race matrix covers both. Teardown (sessionSettleTeardown.ts) reuses stopLaneRuntimeWork's SHAPE, not its body: that function disposes chat sessions because it serves lane deletion, and a settle must leave the session usable. Terminals are never touched. Abort is checked before each step, so a turn that wins the race keeps its work. R5 lands as 3d option 3: an unconfirmed stop still settles, with residue recorded — coarse reason, reapable flag, local-only table, cleared implicitly when the session is reactivated — plus one bucketed analytics event per settle. Never silent. Peer tuple writes (R7 + the unsettle mirror) are fixed by finishing host authority, not by adding consensus. db.sync.applyChanges is the one place both the host and peer paths funnel through, so settle-tuple changes are held out of the raw apply and replayed through the chokepoint, gaining the revision and window semantics. Held rather than dropped: a paired desktop's decision is legitimate, it just has to come through the front door. No peer-visible token was built; onRemoteSettleWrite is the telemetry that decides whether one is ever justified. R7/R7b keep their raw db.run bypass and are annotated as to why: they pin the property that motivates the interception. --- apps/desktop/src/main/main.ts | 66 +++++- .../services/ai/tools/ctoOperatorTools.ts | 2 +- .../analytics/productAnalyticsPolicy.ts | 8 +- .../services/history/operationService.test.ts | 1 + .../onboarding/onboardingService.test.ts | 1 + .../prs/prMergeAutoSettlementService.ts | 4 +- .../services/sessions/sessionService.test.ts | 36 ++-- .../main/services/sessions/sessionService.ts | 187 +++++++++++++++-- .../sessions/sessionSettleTeardown.test.ts | 149 ++++++++++++++ .../sessions/sessionSettleTeardown.ts | 151 ++++++++++++++ .../sessions/settleRaceMatrix.test.ts | 193 +++++++++++++++--- .../sessions/settleTerminalSession.ts | 2 +- .../sessions/settlingStateRegistry.ts | 67 ++++-- apps/desktop/src/main/services/state/kvDb.ts | 105 ++++++++++ .../settle-teardown-design.md | 118 ++++++++++- docs/logging.md | 20 ++ 16 files changed, 1009 insertions(+), 101 deletions(-) create mode 100644 apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts create mode 100644 apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index 58f07ca65..3e9c1ca43 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -85,6 +85,8 @@ import { releaseLaneRuntimeResources } from "./services/lanes/laneRuntimeLifecyc import { createOAuthRedirectService } from "./services/lanes/oauthRedirectService"; import { createRuntimeDiagnosticsService } from "./services/lanes/runtimeDiagnosticsService"; import { createSessionService } from "./services/sessions/sessionService"; +import { createSessionSettleTeardown, residueCountBucket } from "./services/sessions/sessionSettleTeardown"; +import type { SettleTeardownContext, SettleTeardownOutcome } from "./services/sessions/settlingStateRegistry"; import { createSessionDeltaService } from "./services/sessions/sessionDeltaService"; import { createPtyService } from "./services/pty/ptyService"; import { createSupervisedPtyLoader } from "./services/pty/supervisedPtyHost"; @@ -2870,10 +2872,41 @@ app.whenReady().then(async () => { emitProjectEvent(projectRoot, IPC.lanesEnvEvent, ev), }); - const sessionService = createSessionService({ db }); + // Late-bound: the chat service that owns the work does not exist yet at + // this point, and the settle path must not depend on construction order. + // Same shape as `laneTeardownDeps` below. + const settleTeardownRef: { + run: ((sessionId: string, ctx: SettleTeardownContext) => Promise) | null; + } = { run: null }; + const sessionService = createSessionService({ + db, + onRemoteSettleWrite: ({ columns }) => { + // Expected to be zero post-step-0. Coarse on purpose: the column names + // are a fixed set, and no session id or value is recorded. + logger.warn("settle.remote_tuple_write_reconciled", { columns }); + productAnalyticsService?.captureInternal({ + event: "ade_feature_used", + surface: "desktop", + properties: { feature: "work", action: "settle_remote_write_reconciled", outcome: "partial" }, + }); + }, + runSettleTeardown: async (sessionId, ctx) => + settleTeardownRef.run + ? await settleTeardownRef.run(sessionId, ctx) + // Before the chat service is up there is no background work to stop, + // so an empty teardown is the honest answer, not a skipped one. + : { stopped: [], residue: [] }, + }); sessionService.onChanged((event) => { emitProjectEvent(projectRoot, IPC.sessionsChanged, event); }); + // Inbound settle-tuple writes go through the chokepoint instead of landing + // raw, so a peer's decision gains this host's revision, settling window and + // abort semantics (R7). Registered here because the DB layer must not know + // what a settle means. + db.sync.setRemoteSettleTupleHandler((changes) => { + sessionService.reconcileRemoteSettleTuple(changes); + }); const processRegistry = createProcessRegistryService({ db, logger, @@ -3600,6 +3633,37 @@ app.whenReady().then(async () => { countActiveForLane: (laneId) => agentChatService.countActiveForLane(laneId), disposeForLane: (laneId) => agentChatService.disposeForLane(laneId), }; + settleTeardownRef.run = createSessionSettleTeardown({ + interrupt: async (sessionId) => { + await agentChatService.interrupt({ sessionId, mode: "stop_and_clear" }); + }, + readActiveWork: async (sessionId) => { + const summary = await agentChatService.getSessionSummary(sessionId); + if (!summary) return null; + return { + active: summary.status === "active", + backgroundTaskCount: summary.activeBackgroundTaskCount ?? 0, + provider: summary.provider ?? null, + }; + }, + logger, + onResidue: ({ provider, items }) => { + // One event per settle that had residue, not one per failed job: a + // fleet that fails to stop must not become a burst. Coarse properties + // only — no session id, task id, command, or error text. + productAnalyticsService?.captureInternal({ + event: "ade_feature_used", + surface: "desktop", + properties: { + feature: "work", + action: "settle_teardown_residue", + outcome: items[0]?.reason ?? "failed", + count_bucket: residueCountBucket(items.length), + ...(provider ? { provider } : {}), + }, + }); + }, + }); autoRebaseActivityReady = true; void autoRebaseService .refreshActiveRebaseNeeds("activity_services_ready") diff --git a/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts b/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts index ee5c621c2..1fad42891 100644 --- a/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts +++ b/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts @@ -554,7 +554,7 @@ export function createCtoOperatorTools(deps: CtoOperatorToolDeps): Record { try { - const result = deps.sessionService.settleSessionReportingAbort(sessionId, { + const result = await deps.sessionService.settleSessionReportingAbort(sessionId, { ...(outcome ? { outcome } : {}), source: "operator", }); diff --git a/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts b/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts index bc67543d3..ba4eab793 100644 --- a/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts +++ b/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts @@ -132,7 +132,7 @@ const EVENT_PROPERTY_KEYS: Record ade_project_opened: new Set(["route_kind", "source", "mode", "connection_state"]), ade_feature_used: new Set([ "feature", "action", "outcome", "source", "mode", "provider", "model_family", "duration_bucket", "connection_state", - "bytes_freed", "files_compressed", + "bytes_freed", "files_compressed", "count_bucket", ]), ade_work_session_started: new Set(["feature", "action", "outcome", "source", "mode", "provider"]), ade_work_session_completed: new Set([ @@ -183,6 +183,9 @@ const SAFE_STRING_VALUES: Partial>> = { outcome: new Set([ "success", "started", "completed", "failure", "timeout", "opened", "cancelled", "approved", "denied", "partial", "failed", "idle_only", "immediate", + // Settle teardown could not confirm a stop (design 3d). `timeout` above + // covers the third case. Coarse on purpose: never the task or its error. + "no_stop_control", "rejected", // Which half of a post-update transaction did not land. `swap` is // deliberately absent: the app half is already reported by // `ade_update_install_did_not_land`, so only the brain half is new signal. @@ -200,6 +203,9 @@ const SAFE_STRING_VALUES: Partial>> = { "grok", "local", "other", ]), duration_bucket: new Set(["under_10s", "under_1m", "under_5m", "under_30m", "under_2h", "over_2h"]), + // Bucketed, never a raw count: a fleet that fails to stop must not become a + // high-cardinality dimension. + count_bucket: new Set(["1", "2_5", "6_plus"]), route_kind: new Set(["desktop", "web"]), connection_state: new Set(["connected", "disconnected", "pairing", "direct", "relay", "error"]), drop_reason: new Set([ diff --git a/apps/desktop/src/main/services/history/operationService.test.ts b/apps/desktop/src/main/services/history/operationService.test.ts index 29b31d155..e1f4ed497 100644 --- a/apps/desktop/src/main/services/history/operationService.test.ts +++ b/apps/desktop/src/main/services/history/operationService.test.ts @@ -87,6 +87,7 @@ function createInMemoryAdeDb(): { db: AdeDb; raw: Database } { rebuiltFts: false, }), discardUnpublishedChangesForTables: () => {}, + setRemoteSettleTupleHandler: () => {}, }, flushNow: () => undefined, close: () => raw.close(), diff --git a/apps/desktop/src/main/services/onboarding/onboardingService.test.ts b/apps/desktop/src/main/services/onboarding/onboardingService.test.ts index fd4eee20a..a6b7c448e 100644 --- a/apps/desktop/src/main/services/onboarding/onboardingService.test.ts +++ b/apps/desktop/src/main/services/onboarding/onboardingService.test.ts @@ -33,6 +33,7 @@ function createInMemoryAdeDb(): AdeDb { exportChangesSince: () => [], applyChanges: () => ({ appliedCount: 0, dbVersion: 0, touchedTables: [], rebuiltFts: false }), discardUnpublishedChangesForTables: () => {}, + setRemoteSettleTupleHandler: () => {}, }, flushNow: () => {}, close: () => {} diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index 856557a57..54aa8ef7c 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -264,7 +264,7 @@ export function createPrMergeAutoSettlementService(args: { } abortedSessionIds.delete(session.id); } - const settleResult = args.sessionService.settleSessionsReportingAborts([session.id], { + const settleResult = await args.sessionService.settleSessionsReportingAborts([session.id], { outcome: `PR #${pr.githubPrNumber} merged`, settledAt: polledAt, source: "pr_merge", @@ -280,7 +280,7 @@ export function createPrMergeAutoSettlementService(args: { // wait for inactivity would never stop that work again, because the // work is exactly what it would be waiting on. `lifecycle_changed` and // `joined_in_flight` are momentary and clear on their own. - if (settleResult.aborted.some((entry) => ACTIVITY_ABORTS.has(entry.reason))) { + if (settleResult.aborted.some((entry: { reason: string }) => ACTIVITY_ABORTS.has(entry.reason))) { abortedSessionIds.add(session.id); } } diff --git a/apps/desktop/src/main/services/sessions/sessionService.test.ts b/apps/desktop/src/main/services/sessions/sessionService.test.ts index 254685a30..6d8dbcf4b 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.test.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.test.ts @@ -1303,11 +1303,11 @@ describe("sessionService resume metadata", () => { attentionMessage: "Need a decision", attentionSource: "agent_explicit", })); - service.settleSession("session-settle", { + await service.settleSession("session-settle", { outcome: " Shipped the fix ", settledAt: "2026-03-17T01:00:00.000Z", }); - service.settleSession("session-settle", { + await service.settleSession("session-settle", { outcome: " ", settledAt: "2026-03-17T02:00:00.000Z", }); @@ -1344,9 +1344,9 @@ describe("sessionService resume metadata", () => { toolType: "shell", }); } - service.settleSession("session-settled", { settledAt: "2026-03-17T01:00:00.000Z" }); + await service.settleSession("session-settled", { settledAt: "2026-03-17T01:00:00.000Z" }); - expect(service.settleSessions([ + expect(await service.settleSessions([ "session-settled", "session-new", "session-new", @@ -1356,10 +1356,10 @@ describe("sessionService resume metadata", () => { expect(service.get("session-new")?.settledAt).not.toBeNull(); expect(service.get("session-other")?.settledAt).toBeNull(); - expect(service.settleSessionsReportingAborts( + expect((await service.settleSessionsReportingAborts( ["session-settled", "session-other"], { outcome: "PR #841 merged", settledAt: "2026-03-17T03:00:00.000Z", source: "pr_merge" }, - ).settled).toEqual(["session-other"]); + )).settled).toEqual(["session-other"]); expect(service.get("session-settled")).toEqual(expect.objectContaining({ settledAt: "2026-03-17T01:00:00.000Z", statusNote: null, @@ -1411,7 +1411,7 @@ describe("sessionService resume metadata", () => { service.setStatusNote("session-markers", " "); expect(service.get("session-markers")?.statusNote).toBeNull(); - service.settleSession("session-markers", { + await service.settleSession("session-markers", { settledAt: "2026-03-17T01:00:00.000Z", outcome: "Completed fixes and waiting for release review now", }); @@ -1471,21 +1471,21 @@ describe("sessionService resume metadata", () => { // Chat preview writes (no clearSettled) must PRESERVE a declared settle — // an agent's own final assistant text would otherwise undo its // `ade chat settle`. Only PTY-layer activity un-settles. - service.settleSession("session-output", { settledAt: "2026-03-17T00:30:00.000Z" }); + await service.settleSession("session-output", { settledAt: "2026-03-17T00:30:00.000Z" }); service.setLastOutputPreview("session-output", "final assistant text"); expect(service.get("session-output")?.settledAt).toBe("2026-03-17T00:30:00.000Z"); service.setLastOutputPreview("session-output", "working", { clearSettled: true }); expect(service.get("session-output")?.settledAt).toBeNull(); - service.settleSession("session-output", { settledAt: "2026-03-17T02:00:00.000Z" }); + await service.settleSession("session-output", { settledAt: "2026-03-17T02:00:00.000Z" }); service.touchSessionActivity("session-output", "2026-03-17T02:01:00.000Z"); expect(service.get("session-output")?.settledAt).toBeNull(); // A tracked agent CLI may emit its settle command and final answer through // the same PTY after declaring completion. That output refreshes activity // without reopening the thread; the next user turn clears it explicitly. - service.settleSession("session-output", { settledAt: "2026-03-17T02:30:00.000Z" }); + await service.settleSession("session-output", { settledAt: "2026-03-17T02:30:00.000Z" }); service.touchSessionActivity( "session-output", "2026-03-17T02:31:00.000Z", @@ -1495,7 +1495,7 @@ describe("sessionService resume metadata", () => { // A turn failure un-settles: the declared outcome is in doubt, and keeping // the markers mutually exclusive lets every surface agree on precedence. - service.settleSession("session-output", { settledAt: "2026-03-17T03:00:00.000Z" }); + await service.settleSession("session-output", { settledAt: "2026-03-17T03:00:00.000Z" }); service.markLastTurnFailed("session-output", "2026-03-17T03:05:00.000Z"); expect(service.get("session-output")?.settledAt).toBeNull(); expect(service.get("session-output")?.lastTurnFailedAt).toBe("2026-03-17T03:05:00.000Z"); @@ -1828,7 +1828,7 @@ describe("sessionService settle override", () => { // An explicit settle drops a stale keep-active pin. service.setSettleOverride("session-override", "active"); - service.settleSession("session-override", { settledAt: "2026-03-17T03:00:00.000Z" }); + await service.settleSession("session-override", { settledAt: "2026-03-17T03:00:00.000Z" }); expect(service.get("session-override")).toEqual(expect.objectContaining({ settledAt: "2026-03-17T03:00:00.000Z", settleOverride: null, @@ -1851,7 +1851,7 @@ describe("sessionService settle override", () => { it("preserves the declaration source while an active override temporarily hides settle", async () => { const { service } = await makeService("ade-session-service-override-source-"); - service.settleSession("session-override", { + await service.settleSession("session-override", { settledAt: "2026-03-17T03:00:00.000Z", source: "agent_explicit", }); @@ -1893,7 +1893,7 @@ describe("sessionService settle override", () => { expect(service.setSettleOverrides(["session-override", "session-override-2", "missing"], "active")) .toEqual(["session-override", "session-override-2"]); expect(service.get("session-override-2")?.settleOverride).toBe("active"); - service.settleSession("session-override", { source: "pr_merge" }); + await service.settleSession("session-override", { source: "pr_merge" }); service.setSettleOverrides(["session-override"], "active"); expect(service.get("session-override")?.settleSource).toBe("pr_merge"); service.setSettleOverrides(["session-override", "session-override-2"], null); @@ -1905,7 +1905,7 @@ describe("sessionService settle override", () => { const { service } = await makeService("ade-session-service-override-bulk-settle-"); service.setSettleOverride("session-override", "active"); - expect(service.settleSessions(["session-override"])).toEqual(["session-override"]); + expect(await service.settleSessions(["session-override"])).toEqual(["session-override"]); expect(service.get("session-override")?.settleOverride).toBeNull(); service.setSettleOverride("session-override", "settled"); @@ -1923,18 +1923,18 @@ describe("sessionService settle override", () => { // Declared settle first, Keep-active pinned after: the row carries BOTH a // non-null settled_at and settle_override = 'active', and reads as NOT // settled because canonicalSessionState consults the override first. - service.settleSession("session-override", { settledAt: "2026-03-17T01:00:00.000Z" }); + await service.settleSession("session-override", { settledAt: "2026-03-17T01:00:00.000Z" }); service.setSettleOverride("session-override", "active"); expect(service.get("session-override")?.settledAt).toBe("2026-03-17T01:00:00.000Z"); // Bulk settle must behave like the single-row path: drop the stale pin, // report the row as changed, and preserve the original settle timestamp. - expect(service.settleSessions(["session-override"])).toEqual(["session-override"]); + expect(await service.settleSessions(["session-override"])).toEqual(["session-override"]); expect(service.get("session-override")?.settleOverride).toBeNull(); expect(service.get("session-override")?.settledAt).toBe("2026-03-17T01:00:00.000Z"); // Fully settled with no pin is still a no-op, so the return value keeps // meaning "rows this call actually changed". - expect(service.settleSessions(["session-override"])).toEqual([]); + expect(await service.settleSessions(["session-override"])).toEqual([]); }); }); diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index acefb6414..793cfdcb1 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -1,7 +1,7 @@ import fs from "node:fs"; -import type { AdeDb } from "../state/kvDb"; +import type { AdeDb, RemoteSettleTupleChange, SqlValue } from "../state/kvDb"; import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; -import type { SettleAbortedReason, SettleAbortedSession, SettleSessionsOutcome, SettleTeardownCompleted } from "./settlingStateRegistry"; +import type { SettleAbortedReason, SettleAbortedSession, SettleResidueItem, SettleSessionsOutcome, SettleTeardownContext, SettleTeardownOutcome } from "./settlingStateRegistry"; import type { ClaudeSessionPointer, SessionAttentionSource, @@ -156,6 +156,15 @@ function normalizeAttentionSource(value: unknown): SessionAttentionSource | null : null; } +/** + * A settle-tuple column as it arrives over the wire. Only text and null are + * expected; anything else is not a value these columns can hold, so it becomes + * null rather than being coerced into a shape the guard would misread. + */ +function toSqlScalar(value: unknown): SqlValue { + return typeof value === "string" || typeof value === "number" ? value : null; +} + function normalizeSettleSource(value: unknown): SessionSettleSource | null { return value === "agent_explicit" || value === "user" || value === "pr_merge" || value === "operator" ? value @@ -369,6 +378,7 @@ function normalizeSessionIds(sessionIds: string[]): string[] { export function createSessionService({ db, runSettleTeardown, + onRemoteSettleWrite, }: { db: AdeDb; /** @@ -379,7 +389,23 @@ export function createSessionService({ * revision guard all land and are tested against a NO-OP, so every race is * exercised before there is any work to lose. Step 3 supplies the real one. */ - runSettleTeardown?: (sessionId: string) => SettleTeardownCompleted; + /** + * Real teardown. Awaited INSIDE the settling window, which is what makes the + * suspension point safe: the window is exclusive (R4), abortable (R1/R6) and + * in-memory so a crash resolves to not-settled. Step 2 shipped a synchronous + * branded seam to forbid exactly this until those semantics were proven. + */ + runSettleTeardown?: ( + sessionId: string, + ctx: SettleTeardownContext, + ) => Promise; + /** + * Fired when a peer's settle-tuple write had to be reconciled. Telemetry + * only: post-step-0 this should be zero in the field, and if it is not we + * want to know which writer is still out there before deciding whether a + * protocol-level token is justified. + */ + onRemoteSettleWrite?: (args: { columns: string[] }) => void; }) { const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); @@ -742,13 +768,32 @@ export function createSessionService({ * the revision (anything else changed the settle tuple, including a change * this host did not make through a caller). * - * Teardown is a NO-OP in step 2 by design. The point of landing the window - * first is that every race is testable before there is any work to lose. + * Teardown is real from step 3, and it is AWAITED inside the window. + */ + /** + * Record what teardown could not confirm it stopped (§3d option 3). + * + * Keyed by session so a re-settle REPLACES the previous record rather than + * accumulating history the user has no way to clear. Never throws: the settle + * has already landed by this point, and losing a diagnostics row must not + * turn a successful settle into a failed one. */ - const settleManyWithTeardown = ( + const recordSettleResidue = (sessionId: string, items: SettleResidueItem[]): void => { + try { + db.run( + `insert into session_settle_residue (session_id, recorded_at, items) values (?, ?, ?) + on conflict(session_id) do update set recorded_at = excluded.recorded_at, items = excluded.items`, + [sessionId, new Date().toISOString(), JSON.stringify(items)], + ); + } catch { + // Diagnostics only. + } + }; + + const settleManyWithTeardown = async ( sessionIds: string[], options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, - ): SettleSessionsOutcome => { + ): Promise => { const ids = normalizeSessionIds(sessionIds); const settled: string[] = []; const aborted: SettleAbortedSession[] = []; @@ -768,8 +813,15 @@ export function createSessionService({ } try { let teardownThrew = false; + let teardown: SettleTeardownOutcome | null = null; try { - runSettleTeardown?.(id); + teardown = runSettleTeardown + ? await runSettleTeardown(id, { + // Read live, not captured: the whole point is that a clearer can + // trip it while teardown is between stop calls. + isAborted: () => settleLifecycle.settling.abortedBy(id) !== null, + }) + : null; } catch (error) { // ONLY a teardown throw is `teardown_failed`. Persistence failures // below (a SQLite lock timeout, an I/O error) must not wear that @@ -796,7 +848,14 @@ export function createSessionService({ aborted.push({ sessionId: id, reason: "lifecycle_changed" }); continue; } - settled.push(...settleMany([id], options)); + const changed = settleMany([id], options); + settled.push(...changed); + // Residue is recorded ONLY when the settle actually landed. Attaching it + // to an abandoned settle would leave a "settled, 1 job could not be + // stopped" marker on a row that is not settled. + if (changed.length && teardown?.residue.length) { + recordSettleResidue(id, teardown.residue); + } } finally { settleLifecycle.settling.end(id); } @@ -1515,10 +1574,10 @@ export function createSessionService({ return true; }, - settleSession( + async settleSession( sessionId: string, opts: { outcome?: string | null; settledAt?: string; source?: SessionSettleSource } = {}, - ): boolean { + ): Promise { // Through the settling window, like the bulk paths. This is the route a // USER takes (row menu -> settleTerminalSession -> here), which is the // "user settle" R4 names — so it has to be joinable and abortable, and in @@ -1538,7 +1597,7 @@ export function createSessionService({ // writes null and erases the note a previous settle left behind — which is // what a re-settle after activity does when the user supplies no new text. const note = normalizeSessionStatusNote(opts.outcome); - const result = settleManyWithTeardown([trimmed], { + const result = await settleManyWithTeardown([trimmed], { ...(note ? { outcome: note } : {}), settledAt: opts.settledAt, source: opts.source, @@ -1553,10 +1612,10 @@ export function createSessionService({ * The boolean form cannot distinguish "no such session" from "a turn started * mid-settle", and rendering the second as the first misleads the user. */ - settleSessionReportingAbort( + async settleSessionReportingAbort( sessionId: string, opts: { outcome?: string | null; settledAt?: string; source?: SessionSettleSource } = {}, - ): { found: boolean; settled: boolean; abortedBy?: SettleAbortedReason } { + ): Promise<{ found: boolean; settled: boolean; abortedBy?: SettleAbortedReason }> { const trimmed = sessionId.trim(); if (!trimmed) return { found: false, settled: false }; const exists = db.get<{ present: number }>( @@ -1565,7 +1624,7 @@ export function createSessionService({ ); if (!exists) return { found: false, settled: false }; const note = normalizeSessionStatusNote(opts.outcome); - const result = settleManyWithTeardown([trimmed], { + const result = await settleManyWithTeardown([trimmed], { ...(note ? { outcome: note } : {}), settledAt: opts.settledAt, source: opts.source, @@ -1636,8 +1695,8 @@ export function createSessionService({ return settleLifecycle.readRevision(sessionId); }, - settleSessions(sessionIds: string[]): string[] { - return settleManyWithTeardown(sessionIds).settled; + async settleSessions(sessionIds: string[]): Promise { + return (await settleManyWithTeardown(sessionIds)).settled; }, /** @@ -1649,11 +1708,99 @@ export function createSessionService({ * what a caller with a durable consequence (the PR-merge auto-settle marking * a PR handled) has to branch on. */ - settleSessionsReportingAborts( + /** + * Reconcile inbound settle-tuple writes from a peer (design 3c-i / R7). + * + * Post-step-0 every legitimate settle decision originates at a host running + * this chokepoint, so a replicated settle-tuple write is either a legacy + * client or a bug. Either way it must not land raw: a raw write bypasses the + * lifecycle revision, so an in-flight settle would neither see it nor abort + * for it, and it could silently overwrite a peer's explicit reactivation. + * + * Routing it back through the chokepoint gives the remote decision the same + * revision bump, settling-window exclusion and abort semantics a local one + * gets — without inventing a peer-visible concurrency token, which is a + * protocol change the evidence does not yet justify. `onRemoteWrite` is how + * we find out whether any legitimate peer writer is still out there. + */ + reconcileRemoteSettleTuple(changes: RemoteSettleTupleChange[]): void { + const bySession = new Map>(); + for (const change of changes) { + const columns = bySession.get(change.sessionId) ?? new Map(); + columns.set(change.column, toSqlScalar(change.value)); + bySession.set(change.sessionId, columns); + } + + for (const [sessionId, columns] of bySession) { + const exists = db.get<{ present: number }>( + "select 1 as present from terminal_sessions where id = ? limit 1", + [sessionId], + ); + // A settle for a row this host does not have is not reconcilable and + // not worth inventing: the row itself will arrive in the same batch or + // not at all. + if (!exists) continue; + + const hasSettledAt = columns.has("settled_at"); + const settledAt = columns.get("settled_at"); + const override = columns.get("settle_override"); + const source = normalizeSettleSource(columns.get("settle_source")) ?? "user"; + + if (hasSettledAt && typeof settledAt === "string" && settledAt) { + writeSettleLifecycle({ + intent: { kind: "settle", settledAt, source }, + guard: "(settled_at is null or settle_override is not null)", + sessionIds: [sessionId], + }); + } else if (hasSettledAt) { + writeSettleLifecycle({ intent: { kind: "unsettleDeclared" }, sessionIds: [sessionId] }); + } else if (columns.has("settle_override")) { + writeSettleLifecycle({ + intent: { + kind: "override", + value: override === "settled" || override === "active" ? override : null, + source, + }, + sessionIds: [sessionId], + }); + } else { + continue; + } + emitChanged({ sessionId, reason: "meta-updated" }); + onRemoteSettleWrite?.({ columns: [...columns.keys()].sort() }); + } + }, + + /** + * What the LAST settle could not confirm it stopped, for the diagnostics + * surface. Returns null unless the session is currently settled: a stale + * record on a row the user has since reactivated is not residue, it is + * history, and showing it would re-light a row that is working fine. + */ + getSettleResidue(sessionId: string): { recordedAt: string; items: SettleResidueItem[] } | null { + const trimmed = sessionId.trim(); + if (!trimmed) return null; + const row = db.get<{ settled_at: string | null; recorded_at: string; items: string }>( + `select s.settled_at as settled_at, r.recorded_at as recorded_at, r.items as items + from session_settle_residue r + join terminal_sessions s on s.id = r.session_id + where r.session_id = ?`, + [trimmed], + ); + if (!row || !row.settled_at) return null; + try { + const items = JSON.parse(row.items) as SettleResidueItem[]; + return Array.isArray(items) && items.length ? { recordedAt: row.recorded_at, items } : null; + } catch { + return null; + } + }, + + async settleSessionsReportingAborts( sessionIds: string[], options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, - ): SettleSessionsOutcome { - return settleManyWithTeardown(sessionIds, options); + ): Promise { + return await settleManyWithTeardown(sessionIds, options); }, /** Sessions currently mid-settle, for the visible `Settling…` state. */ diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts new file mode 100644 index 000000000..51a524018 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts @@ -0,0 +1,149 @@ +import { describe, expect, it, vi } from "vitest"; +import { createSessionSettleTeardown, residueCountBucket } from "./sessionSettleTeardown"; +import type { SessionActiveWork, SessionSettleTeardownDeps } from "./sessionSettleTeardown"; + +/** + * R5 and the teardown contract itself. The race matrix drives the settle path + * through a hand-written seam; this drives the REAL seam, which is where the + * "cannot confirm the stop" decision (design 3d, option 3) actually lives. + */ +describe("session settle teardown", () => { + const neverAborted = { isAborted: () => false }; + + function harness(overrides: Partial = {}) { + const interrupt = vi.fn(async () => {}); + const onResidue = vi.fn(); + // Instant polling: the confirmation budget is real time in production, and + // a test that actually slept 5s per case would be deleted within a month. + let clock = 0; + const run = createSessionSettleTeardown({ + interrupt, + readActiveWork: async () => null, + onResidue, + now: () => clock, + sleep: async (ms: number) => { clock += ms; }, + ...overrides, + }); + return { run, interrupt, onResidue }; + } + + const work = (over: Partial = {}): SessionActiveWork => ({ + active: false, + backgroundTaskCount: 0, + provider: "claude", + ...over, + }); + + it("does not stop anything for a session with no work", async () => { + const { run, interrupt } = harness({ readActiveWork: async () => work() }); + + const outcome = await run("session-1", neverAborted); + + // A settle with nothing to tear down must not interrupt the session: that + // would be a visible side effect on a row the user only meant to file. + expect(interrupt).not.toHaveBeenCalled(); + expect(outcome).toEqual({ stopped: [], residue: [] }); + }); + + it("stops background work and reports no residue once the session goes quiet", async () => { + const states = [work({ backgroundTaskCount: 2 }), work({ backgroundTaskCount: 2 }), work()]; + const readActiveWork = vi.fn(async () => states.shift() ?? work()); + const { run, interrupt, onResidue } = harness({ readActiveWork }); + + const outcome = await run("session-1", neverAborted); + + expect(interrupt).toHaveBeenCalledWith("session-1"); + expect(outcome.stopped).toEqual(["interrupt"]); + // The stop is asynchronous inside the provider, so a single read straight + // after `interrupt` would call work that was already stopping "residue". + expect(outcome.residue, "work that drained must not be reported as residue").toEqual([]); + expect(onResidue).not.toHaveBeenCalled(); + }); + + /** + * R5. The stop is attempted, the work does not go away, and the settle still + * lands — but never silently: the residue is returned for the row and the + * analytics hook fires exactly once. + */ + it("R5: reports residue when the stop never confirms, rather than blocking the settle", async () => { + const { run, onResidue } = harness({ + readActiveWork: async () => work({ backgroundTaskCount: 3 }), + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue).toEqual([{ + kind: "background_tasks", + reason: "timeout", + reapable: true, + detail: "3 jobs on claude could not be stopped", + }]); + expect(onResidue, "residue must be measured, not just displayed").toHaveBeenCalledTimes(1); + expect(onResidue).toHaveBeenCalledWith({ + provider: "claude", + items: outcome.residue, + }); + }); + + it("R5: calls out a provider that has no stop control at all", async () => { + const { run } = harness({ + // A Codex chat cannot stop an individual subagent. That is a different + // fact from "the stop failed", and the field exists to keep them apart. + readActiveWork: async () => work({ backgroundTaskCount: 1, provider: "codex" }), + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue[0]?.reason).toBe("no_stop_control"); + }); + + it("R5: distinguishes a stop the provider rejected from one that timed out", async () => { + const { run } = harness({ + interrupt: vi.fn(async () => { throw new Error("provider refused"); }), + readActiveWork: async () => work({ backgroundTaskCount: 1 }), + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue[0]?.reason).toBe("rejected"); + // The stop never landed, so it must not be claimed as a completed step. + expect(outcome.stopped).toEqual([]); + }); + + /** + * §3c: an accepted turn beats the settle, never the reverse. The abort is + * checked BEFORE each step, so the work that won the race keeps running. + */ + it("stops issuing stop calls the moment a turn aborts the settle", async () => { + const { run, interrupt } = harness({ + readActiveWork: async () => work({ active: true }), + }); + + const outcome = await run("session-1", { isAborted: () => true }); + + expect(interrupt, "an aborted settle must not stop the work that won the race").not.toHaveBeenCalled(); + expect(outcome).toEqual({ stopped: [], residue: [] }); + }); + + it("does not report residue for a session the user reclaimed mid-teardown", async () => { + let aborted = false; + const { run, onResidue } = harness({ + interrupt: vi.fn(async () => { aborted = true; }), + readActiveWork: async () => work({ active: true, backgroundTaskCount: 1 }), + }); + + const outcome = await run("session-1", { isAborted: () => aborted }); + + // The settle is being abandoned, so there is no settled row to hang a + // "1 job could not be stopped" marker on. Reporting it would label a + // session that is actively working as one that failed to stop. + expect(outcome.residue).toEqual([]); + expect(onResidue).not.toHaveBeenCalled(); + }); + + it("buckets residue counts so a large fleet cannot widen the analytics dimension", () => { + expect(residueCountBucket(1)).toBe("1"); + expect(residueCountBucket(5)).toBe("2_5"); + expect(residueCountBucket(40)).toBe("6_plus"); + }); +}); diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts new file mode 100644 index 000000000..800659912 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts @@ -0,0 +1,151 @@ +import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } from "./settlingStateRegistry"; + +/** + * Real settle teardown: stop the work a session owns, then confirm it stopped. + * + * Shaped after `laneService.stopLaneRuntimeWork` — an ordered list of steps, + * each in its own try/catch so one failure cannot abandon the rest — but NOT + * built on it. That function disposes chat sessions outright because it serves + * lane deletion; a settle must leave the session usable, so this stops the + * session's *outstanding work* and nothing else. + * + * Two rules from the design are load-bearing here: + * + * - **Terminals stay open.** A settle files a session as done; it does not take + * the user's shell away, and ADE cannot re-spawn one it killed. PTYs are + * never touched, at any step. + * - **An accepted turn beats the settle, never the reverse** (§3c). The abort is + * checked before every step, so a turn that starts mid-teardown stops the + * remaining stops. Work already stopped is lost — inherent, and the reason the + * order below is cheapest-to-lose first. + */ + +/** Bounds a single provider stop, matching the chat service's own stop budget. */ +const STOP_CONFIRM_TIMEOUT_MS = 5_000; +const STOP_CONFIRM_POLL_MS = 100; + +export type SessionActiveWork = { + /** A turn is running right now. */ + active: boolean; + /** Background tasks, subagents, or cloud runs still attributed to the session. */ + backgroundTaskCount: number; + provider: string | null; +}; + +export type SessionSettleTeardownDeps = { + /** + * Stop the session's active turn and its background work. Resolves when the + * stop has been REQUESTED; whether it took is decided by `readActiveWork`. + */ + interrupt: (sessionId: string) => Promise; + /** Ground truth after a stop. `null` for a session the chat service does not own. */ + readActiveWork: (sessionId: string) => Promise; + /** + * Providers with no way to stop background work at all. A Codex chat cannot + * stop an individual subagent, so its residue is `no_stop_control` rather + * than a stop that failed — the distinction is the whole point of the field. + */ + providersWithoutStopControl?: ReadonlySet; + onResidue?: (args: { provider: string | null; items: SettleResidueItem[] }) => void; + logger?: { warn: (message: string, meta?: Record) => void }; + now?: () => number; + sleep?: (ms: number) => Promise; +}; + +const DEFAULT_NO_STOP_CONTROL = new Set(["codex"]); + +export function createSessionSettleTeardown( + deps: SessionSettleTeardownDeps, +): (sessionId: string, ctx: SettleTeardownContext) => Promise { + const noStopControl = deps.providersWithoutStopControl ?? DEFAULT_NO_STOP_CONTROL; + const now = deps.now ?? (() => Date.now()); + const sleep = deps.sleep ?? ((ms: number) => new Promise((resolve) => { setTimeout(resolve, ms); })); + + return async (sessionId, ctx): Promise => { + const stopped: string[] = []; + const residue: SettleResidueItem[] = []; + + const before = await deps.readActiveWork(sessionId).catch(() => null); + // Nothing to stop, or a session this service does not own (a plain + // terminal). Either way there is no work to lose and no residue to report. + if (!before || (!before.active && before.backgroundTaskCount === 0)) { + return { stopped, residue }; + } + + const provider = before.provider; + // Checked before the step, not after: the point of the abort is to stop + // work we have NOT done yet. + if (ctx.isAborted()) return { stopped, residue }; + + let stopRejected = false; + try { + await deps.interrupt(sessionId); + stopped.push("interrupt"); + } catch (error) { + stopRejected = true; + deps.logger?.warn("settle_teardown.step_failed", { + step: "interrupt", + error: error instanceof Error ? error.message : String(error), + }); + } + + // A turn that arrived while the stop was in flight wins. Do not spend the + // confirmation budget re-reading a session the user is actively using. + if (ctx.isAborted()) return { stopped, residue }; + + const after = await waitForQuiet(sessionId, ctx); + const stillActive = after ? after.active || after.backgroundTaskCount > 0 : false; + if (stillActive && !ctx.isAborted()) { + const remaining = (after?.backgroundTaskCount ?? 0) + (after?.active ? 1 : 0); + residue.push({ + kind: after?.active && (after?.backgroundTaskCount ?? 0) === 0 ? "active_turn" : "background_tasks", + reason: stopRejected + ? "rejected" + : provider && noStopControl.has(provider) + ? "no_stop_control" + : "timeout", + // Everything counted here is still a child of this ADE process, which + // is what keeps it eligible for the ppid-based orphan reaper. Work that + // escaped the tree (`nohup`/`setsid`/`disown`) is not visible to + // `readActiveWork` at all, so it is never folded into this count — it is + // unreachable by construction, and saying otherwise would overstate what + // the reaper can clean up. + reapable: true, + detail: describeResidue(remaining, provider), + }); + } + + if (residue.length) deps.onResidue?.({ provider, items: residue }); + return { stopped, residue }; + }; + + /** + * Poll until the session goes quiet or the budget runs out. A stop is + * asynchronous inside the provider, so reading once immediately after + * `interrupt` would report residue for work that was about to stop anyway. + */ + async function waitForQuiet( + sessionId: string, + ctx: SettleTeardownContext, + ): Promise { + const deadline = now() + STOP_CONFIRM_TIMEOUT_MS; + let latest = await deps.readActiveWork(sessionId).catch(() => null); + while (latest && (latest.active || latest.backgroundTaskCount > 0) && now() < deadline) { + if (ctx.isAborted()) return latest; + await sleep(STOP_CONFIRM_POLL_MS); + latest = await deps.readActiveWork(sessionId).catch(() => null); + } + return latest; + } +} + +function describeResidue(remaining: number, provider: string | null): string { + const what = remaining === 1 ? "1 job" : `${remaining} jobs`; + return provider ? `${what} on ${provider} could not be stopped` : `${what} could not be stopped`; +} + +/** Bucketed so a fleet that fails to stop cannot become a high-cardinality dimension. */ +export function residueCountBucket(count: number): "1" | "2_5" | "6_plus" { + if (count <= 1) return "1"; + return count <= 5 ? "2_5" : "6_plus"; +} diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index 7780fe752..f04bb68ba 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -5,7 +5,8 @@ import { afterEach, describe, expect, it } from "vitest"; import { openKvDb } from "../state/kvDb"; import { createSessionService } from "./sessionService"; import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; -import { settleTeardownCompleted } from "./settlingStateRegistry"; +import type { SettleResidueItem, SettleTeardownContext } from "./settlingStateRegistry"; + /** * The race matrix from the settle-teardown design (§2), tested directly against @@ -59,17 +60,27 @@ describe("settle race matrix (teardown is a no-op)", () => { insertProjectGraph(db); // The teardown seam the race matrix drives. Whatever this does is what a // real provider stop would have been doing when the race landed. - let teardown: (sessionId: string) => void = () => {}; + let teardown: (sessionId: string) => void | Promise = () => {}; + // Records what the seam was handed, so a test can assert the abort was + // visible to teardown WHILE it ran rather than only afterwards. + const teardownContexts: SettleTeardownContext[] = []; + let residue: SettleResidueItem[] = []; + const remoteWrites: Array<{ columns: string[] }> = []; const service = createSessionService({ db, - runSettleTeardown: (sessionId) => { - teardown(sessionId); - return settleTeardownCompleted(); + onRemoteSettleWrite: (args) => { remoteWrites.push(args); }, + runSettleTeardown: async (sessionId, ctx) => { + teardownContexts.push(ctx); + await teardown(sessionId); + return { stopped: [], residue }; }, }); - const setTeardown = (fn: (sessionId: string) => void) => { + const setTeardown = (fn: (sessionId: string) => void | Promise) => { teardown = fn; }; + const setResidue = (items: SettleResidueItem[]) => { + residue = items; + }; const create = (id: string) => service.create({ sessionId: id, @@ -82,7 +93,7 @@ describe("settle race matrix (teardown is a no-op)", () => { toolType: "codex-chat", }); create("session-1"); - return { db, service, create, setTeardown }; + return { db, service, create, setTeardown, setResidue, teardownContexts, remoteWrites }; } /** @@ -92,13 +103,13 @@ describe("settle race matrix (teardown is a no-op)", () => { */ it("R1: a turn starting during teardown abandons the settle", async () => { const { service, setTeardown } = await fixture(); - service.settleSessions(["session-1"]); + await service.settleSessions(["session-1"]); service.unsettleSession("session-1"); setTeardown(() => { service.clearTurnStartMarkers("session-1"); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); expect(outcome.settled).toEqual([]); expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "turn_start" }]); @@ -121,7 +132,7 @@ describe("settle race matrix (teardown is a no-op)", () => { teardownRan = true; service.requestAttention("session-1", "need you"); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); expect(teardownRan).toBe(true); expect(outcome.settled).toEqual([]); @@ -136,7 +147,7 @@ describe("settle race matrix (teardown is a no-op)", () => { // A stop against an already-finished task is a no-op, and nothing touches // the settle tuple. }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); expect(outcome.aborted).toEqual([]); expect(outcome.settled).toEqual(["session-1"]); @@ -151,16 +162,16 @@ describe("settle race matrix (teardown is a no-op)", () => { const { service, setTeardown } = await fixture(); let teardowns = 0; - let inner: ReturnType | null = null; - setTeardown(() => { + let inner: Awaited> | null = null; + setTeardown(async () => { teardowns += 1; // Re-entrant settle, exactly as a PR-merge poll landing mid-user-settle. // The window is already open, so this must JOIN rather than tear down // again — the teardown counter is what proves it. - inner = service.settleSessionsReportingAborts(["session-1"]); + inner = await service.settleSessionsReportingAborts(["session-1"]); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); expect(teardowns, "the joined settle must not start its own teardown").toBe(1); expect(outcome.settled).toEqual(["session-1"]); @@ -189,7 +200,7 @@ describe("settle race matrix (teardown is a no-op)", () => { service.setLastOutputPreview("session-1", "final chunk", { clearSettled: true }); service.touchSessionActivity("session-1", "2026-08-11T00:09:00.000Z"); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); // 1. did not abort expect(outcome.aborted).toEqual([]); @@ -205,7 +216,7 @@ describe("settle race matrix (teardown is a no-op)", () => { it("R6b: the same output OUTSIDE the settling window clears normally", async () => { const { service } = await fixture(); - service.settleSessions(["session-1"]); + await service.settleSessions(["session-1"]); expect(service.get("session-1")?.settledAt).toBeTruthy(); service.setLastOutputPreview("session-1", "later output", { clearSettled: true }); @@ -227,6 +238,16 @@ describe("settle race matrix (teardown is a no-op)", () => { * coordinator's step-3 review scope — it asserts today's real behavior, not * the behavior we want. */ + /** + * NOTE (step 3): R7 and R7b below still write the row with a raw `db.run`, + * which is a bypass NO inbound changeset can produce any more — the apply + * layer now hands settle-tuple writes to `reconcileRemoteSettleTuple`. They + * are kept, unchanged, because they still pin the underlying property they + * always pinned: a write that reaches the tuple without the chokepoint is + * invisible to the guard. That is the reason the apply layer must intercept, + * so deleting these would delete the evidence for the fix. The reconciled + * path is asserted immediately after them. + */ it("R7: a peer-style write that bypasses the writer is invisible to the guard", async () => { const { db, service, setTeardown } = await fixture(); const revisionBefore = service.getSettleLifecycleRevision("session-1"); @@ -239,7 +260,7 @@ describe("settle race matrix (teardown is a no-op)", () => { ["2026-08-11T00:07:00.000Z", "user", "session-1"], ); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); // Observed behaviour, not the behaviour we would have guessed. // @@ -279,7 +300,7 @@ describe("settle race matrix (teardown is a no-op)", () => { ["2026-08-11T00:07:00.000Z", "user", "session-1"], ); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); const accountedFor = [ ...outcome.settled, @@ -303,7 +324,7 @@ describe("settle race matrix (teardown is a no-op)", () => { it("refuses a multi-id clear rather than guessing one disposition for the batch", async () => { const { service, db, create } = await fixture(); create("session-2"); - service.settleSessions(["session-1", "session-2"]); + await service.settleSessions(["session-1", "session-2"]); // The service API cannot express a multi-id `clearOnActivity`, so reach the // writer directly — otherwise the refusal this test is named for is never @@ -334,7 +355,7 @@ describe("settle race matrix (teardown is a no-op)", () => { setTeardown((sessionId) => { if (sessionId === "session-1") throw new Error("provider stop failed"); }); - const outcome = service.settleSessionsReportingAborts(["session-1", "session-2"]); + const outcome = await service.settleSessionsReportingAborts(["session-1", "session-2"]); expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "teardown_failed" }]); // The rest of the batch was still attempted and accounted for. @@ -361,7 +382,7 @@ describe("settle race matrix (teardown is a no-op)", () => { setTeardown(() => { duringWindow = service.settlingSessionIds(); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); expect(outcome.aborted, "the teardown must not have thrown").toEqual([]); expect(duringWindow, "the session must be visibly settling mid-teardown").toEqual([ @@ -390,7 +411,7 @@ describe("settle race matrix (teardown is a no-op)", () => { // It must NOT come back as `teardown_failed`: the work really did stop, and // labelling it a teardown failure invites a caller to retry the stop. - expect(() => service.settleSessionsReportingAborts(["session-1"])).toThrow(/locked/); + await expect(service.settleSessionsReportingAborts(["session-1"])).rejects.toThrow(/locked/); expect(stopped, "teardown ran, so it must not be reported as failed").toBe(1); db.runChanged = realRunChanged; // The window still closed, or the session would be permanently unsettleable. @@ -399,11 +420,11 @@ describe("settle race matrix (teardown is a no-op)", () => { it("does not erase an existing status note when a re-settle supplies none", async () => { const { service, db } = await fixture(); - expect(service.settleSession("session-1", { outcome: "shipped the fix" })).toBe(true); + expect(await service.settleSession("session-1", { outcome: "shipped the fix" })).toBe(true); service.unsettleSession("session-1"); // Re-settling with no outcome must leave the previous note alone. - expect(service.settleSession("session-1")).toBe(true); + expect(await service.settleSession("session-1")).toBe(true); const row = db.get<{ status_note: string | null }>( "select status_note from terminal_sessions where id = ?", ["session-1"], @@ -419,14 +440,130 @@ describe("settle race matrix (teardown is a no-op)", () => { }); // The boolean form must not claim success... - expect(service.settleSession("session-1")).toBe(false); + expect(await service.settleSession("session-1")).toBe(false); // ...and the typed form must say WHY, so a caller does not report a session // that is sitting right there working as "not found". - const aborted = service.settleSessionReportingAbort("session-1"); + const aborted = await service.settleSessionReportingAbort("session-1"); expect(aborted).toEqual({ found: true, settled: false, abortedBy: "turn_start" }); - expect(service.settleSessionReportingAbort("no-such-session")).toEqual({ + expect(await service.settleSessionReportingAbort("no-such-session")).toEqual({ found: false, settled: false, }); }); + /** + * R5, end to end: an unconfirmed stop does NOT block the settle (3d option 3), + * and the residue is discoverable on the row afterwards rather than only + * returned to whoever happened to call. + */ + it("R5: settles with recorded residue when teardown cannot confirm the stop", async () => { + const { service, setResidue } = await fixture(); + setResidue([{ + kind: "background_tasks", + reason: "no_stop_control", + reapable: true, + detail: "2 jobs on codex could not be stopped", + }]); + + const outcome = await service.settleSessionsReportingAborts(["session-1"]); + + // Filed, not blocked: option 1 (reject the settle) was explicitly not chosen. + expect(outcome.settled).toEqual(["session-1"]); + expect(outcome.aborted).toEqual([]); + expect(service.get("session-1")?.settledAt).not.toBeNull(); + + const residue = service.getSettleResidue("session-1"); + expect(residue?.items).toEqual([{ + kind: "background_tasks", + reason: "no_stop_control", + reapable: true, + detail: "2 jobs on codex could not be stopped", + }]); + }); + + it("does not leave residue hanging on a session the user reactivated", async () => { + const { service, setResidue } = await fixture(); + setResidue([{ + kind: "background_tasks", reason: "timeout", reapable: true, detail: "1 job could not be stopped", + }]); + await service.settleSessionsReportingAborts(["session-1"]); + expect(service.getSettleResidue("session-1")).not.toBeNull(); + + service.unsettleSession("session-1"); + + // The row is working again. A stale "1 job could not be stopped" marker + // would re-light a session that is fine. + expect(service.getSettleResidue("session-1")).toBeNull(); + }); + + it("records no residue for a settle that was abandoned", async () => { + const { service, setResidue, setTeardown } = await fixture(); + setResidue([{ + kind: "background_tasks", reason: "timeout", reapable: true, detail: "1 job could not be stopped", + }]); + setTeardown(() => { + service.clearTurnStartMarkers("session-1"); + }); + + const outcome = await service.settleSessionsReportingAborts(["session-1"]); + + expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "turn_start" }]); + // Nothing was filed, so there is no settled row for residue to describe. + expect(service.getSettleResidue("session-1")).toBeNull(); + }); + /** + * Decision 1 for step 3: finish host authority instead of building consensus + * machinery. A replicated settle-tuple write is routed back through the + * chokepoint, so it gains the revision, the settling window and the abort + * semantics a local decision has — no peer-visible concurrency token. + */ + describe("remote settle-tuple reconciliation", () => { + it("gives a peer settle the revision bump a local settle would have got", async () => { + const { service, remoteWrites } = await fixture(); + const revisionBefore = service.getSettleLifecycleRevision("session-1"); + + service.reconcileRemoteSettleTuple([ + { sessionId: "session-1", column: "settled_at", value: "2026-08-11T00:07:00.000Z" }, + { sessionId: "session-1", column: "settle_source", value: "user" }, + ]); + + expect(service.get("session-1")?.settledAt).toBe("2026-08-11T00:07:00.000Z"); + // The whole point: an in-flight settle can now SEE that the world moved. + expect(service.getSettleLifecycleRevision("session-1")).toBeGreaterThan(revisionBefore); + // And we find out it happened, so we learn whether a legitimate peer + // writer still exists before deciding a protocol change is justified. + expect(remoteWrites).toEqual([{ columns: ["settle_source", "settled_at"] }]); + }); + + it("makes a peer unsettle abort an in-flight settle instead of being overwritten", async () => { + const { service, setTeardown } = await fixture(); + await service.settleSessionsReportingAborts(["session-1"]); + service.unsettleSession("session-1"); + + setTeardown(() => { + // The mirror case Codex raised: a peer reactivates the session while + // this host is mid-settle. Before reconciliation this bypassed the + // revision and the apply overwrote the peer's explicit decision. + service.reconcileRemoteSettleTuple([ + { sessionId: "session-1", column: "settle_override", value: "active" }, + ]); + }); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); + + expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "lifecycle_changed" }]); + expect(outcome.settled).toEqual([]); + // The peer's reactivation survives. + expect(service.get("session-1")?.settleOverride).toBe("active"); + expect(service.get("session-1")?.settledAt).toBeNull(); + }); + + it("ignores a settle for a row this host does not have", async () => { + const { service, remoteWrites } = await fixture(); + + service.reconcileRemoteSettleTuple([ + { sessionId: "not-here", column: "settled_at", value: "2026-08-11T00:07:00.000Z" }, + ]); + + expect(remoteWrites).toEqual([]); + }); + }); }); diff --git a/apps/desktop/src/main/services/sessions/settleTerminalSession.ts b/apps/desktop/src/main/services/sessions/settleTerminalSession.ts index c942f9169..e1831686f 100644 --- a/apps/desktop/src/main/services/sessions/settleTerminalSession.ts +++ b/apps/desktop/src/main/services/sessions/settleTerminalSession.ts @@ -88,7 +88,7 @@ export async function settleTerminalSession(args: { if (!dismissed) return false; } - const result = args.sessionService.settleSessionReportingAbort( + const result = await args.sessionService.settleSessionReportingAbort( args.sessionId, { ...(args.opts?.outcome ? { outcome: args.opts.outcome } : {}), diff --git a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts index 56efe7e31..e151899fa 100644 --- a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts +++ b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts @@ -16,32 +16,57 @@ * would be a lie on every other device the moment this host died. */ -declare const settleTeardownCompletedBrand: unique symbol; - /** - * Proof that a teardown finished synchronously. + * What a teardown could not confirm it stopped (§3d, option 3). * - * The settle path is synchronous in step 2, and a teardown that defers is not - * merely unsupported — it is actively harmful: the settling window would close - * while the unowned continuation kept stopping processes, so C4/C5 output from - * those stops would no longer be swallowed and would clear the settle that just - * landed. Losing the work AND the settle is the R2 shape. + * The settle still lands — that is the signed-off decision — but it lands WITH + * this attached, so "settled" never quietly means "and something is still + * running". A label alone would just be a prettier way of losing the process, + * so each item says what it was, why the stop did not confirm, and whether the + * existing ppid-based orphan reaper can still reach it. + */ +export type SettleResidueItem = { + /** Coarse and closed: this is also the analytics dimension. */ + kind: "background_tasks" | "active_turn" | "scheduled_work"; + /** + * Why the stop did not confirm. `no_stop_control` is a provider that offers + * no way to stop this work at all (a Codex chat's subagents); `timeout` and + * `rejected` are a stop that was attempted and did not land. + */ + reason: "no_stop_control" | "timeout" | "rejected"; + /** + * Whether the orphan reaper can still reach it. Work that escaped the process + * tree (`nohup`/`setsid`/`disown`) is unreachable and must be reported as + * such rather than folded into the reapable count. + */ + reapable: boolean; + /** Human-readable, for the diagnostics surface. Never analytics. */ + detail: string; +}; + +/** Checked BETWEEN stop calls, per §3c. A turn start trips it. */ +export type SettleTeardownContext = { + isAborted: () => boolean; +}; + +/** + * The result of a real teardown. * - * A runtime check cannot prevent this. `async (id) => {}` is caught by its - * constructor, but the common adapter `id => asyncStop(id)` is an ordinary - * `Function` and has already started the work by the time any check runs. So the - * seam demands a value only a synchronous body can produce: an `async` function - * or a promise-returning adapter returns `Promise<...>`, which is not assignable - * to this brand, and fails to COMPILE. + * `stopped` is what confirmed, in the order attempted; `residue` is §3d. * - * Step 3 replaces this with an awaited seam once the settle path itself is async. + * This replaces step 2's synchronous `SettleTeardownCompleted` brand. That + * brand existed to make an async teardown a COMPILE error while the settle path + * was still synchronous — bolting a deferred teardown onto a synchronous write + * was exactly the mistake that produced a P1 in each of #1059's six rounds. It + * is safe to await now for one specific reason: the settling window (step 2) is + * exclusive, abortable and crash-safe, so it can be HELD across the await. The + * revision re-check and the abort flag after the await are what make the + * suspension point survivable, and both are tested by the race matrix. */ -export type SettleTeardownCompleted = { readonly [settleTeardownCompletedBrand]: true }; - -/** The only way to produce a `SettleTeardownCompleted`. */ -export function settleTeardownCompleted(): SettleTeardownCompleted { - return {} as SettleTeardownCompleted; -} +export type SettleTeardownOutcome = { + stopped: string[]; + residue: SettleResidueItem[]; +}; /** Why a settle was abandoned. Only ever set by a human-decision clearer. */ export type SettleAbortReason = "turn_start" | "turn_failed" | "attention_requested"; diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index 4cd80c0e8..f80e73796 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -107,6 +107,12 @@ export type AdeDbSyncApi = { rejectOversizedVersionGroup?: boolean; }) => CrsqlChangeRow[]; applyChanges: (changes: CrsqlChangeRow[]) => ApplyRemoteChangesResult; + /** + * Claim inbound settle-tuple writes so they are reconciled through the settle + * chokepoint instead of landing raw. Registered by the session layer; `null` + * restores the plain apply. + */ + setRemoteSettleTupleHandler: (handler: ((changes: RemoteSettleTupleChange[]) => void) | null) => void; /** * Suppress unpublished local-site CRR rows for specific tables. Used when * local viewer state must be cleared without relaying those clears to sync @@ -895,8 +901,56 @@ const LOCAL_ONLY_CRR_EXCLUDED_TABLES = new Set([ // 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", + // Host-local record of work a settle teardown could not confirm it stopped. + // Not replicated: it describes processes on THIS host, and a peer showing + // "1 job could not be stopped" for a machine it cannot see would be a lie. + "session_settle_residue", +]); + +/** + * The settle tuple, as it appears in an inbound changeset. + * + * Kept next to `LOCAL_ONLY_CRR_EXCLUDED_TABLES` because it answers the same + * question — "may this remote row be written straight to the table?" — and the + * two lists drift apart if they live in different files. + */ +const SETTLE_TUPLE_COLUMNS: ReadonlySet = new Set([ + "settled_at", + "settle_override", + "settle_source", ]); +function isSettleTupleChange(change: CrsqlChangeRow): boolean { + return change.table === "terminal_sessions" && SETTLE_TUPLE_COLUMNS.has(change.cid); +} + +/** + * The inverse of `packedCrsqlPrimaryKey` for the one shape that matters here: + * a single TEXT primary key, which is what `terminal_sessions.id` is. + * + * Returns null for anything else — a composite key, a non-text key, an + * unfamiliar packing. The caller then does NOT claim the row, so an + * unrecognised encoding degrades to the plain apply rather than to a silently + * dropped change. + */ +function decodeSingleTextCrsqlPrimaryKey(value: SyncScalar): string | null { + if (typeof value === "string") return value || null; + if (!isSyncScalarBytes(value)) return null; + const bytes = Buffer.from(value.base64, "base64"); + // [column count][type tag 0x0b = text][byte length][utf8 …] + if (bytes.length < 3 || bytes[0] !== 0x01 || bytes[1] !== 0x0b) return null; + const length = bytes[2] ?? 0; + if (bytes.length !== 3 + length) return null; + return bytes.subarray(3, 3 + length).toString("utf8") || null; +} + +/** One inbound settle-tuple column write, decoded for the session layer. */ +export type RemoteSettleTupleChange = { + sessionId: string; + column: "settled_at" | "settle_override" | "settle_source"; + value: SyncScalar; +}; + function listEligibleCrrTables(db: DatabaseSyncType): string[] { const tables = allRows<{ name: string; sql: string | null }>( db, @@ -3840,6 +3894,17 @@ function migrate(db: MigrationDb, rawDb: DatabaseSyncType) { ) `); + // What a settle teardown could not confirm it stopped (design 3d, option 3). + // Replaced wholesale per settle, so the row always describes the LAST settle + // rather than accumulating history the user cannot clear. + db.run(` + create table if not exists session_settle_residue ( + session_id text primary key, + recorded_at text not null, + items text not null + ) + `); + // 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(` @@ -4393,6 +4458,8 @@ export async function openKvDb( ), }; + // Registered by the session layer once the settle chokepoint exists. + let remoteSettleTupleHandler: ((changes: RemoteSettleTupleChange[]) => void) | null = null; const sync: AdeDbSyncApi = { isAvailable: () => crsqliteLoaded, getSiteId: () => desiredSiteId, @@ -4565,10 +4632,22 @@ export async function openKvDb( seq: Number(row.seq), })); }, + setRemoteSettleTupleHandler: (handler: ((changes: RemoteSettleTupleChange[]) => void) | null) => { + remoteSettleTupleHandler = handler; + }, applyChanges: (changes: CrsqlChangeRow[]) => { if (!crsqliteLoaded) return { appliedCount: 0, dbVersion: 0, touchedTables: [], rebuiltFts: false }; let appliedCount = 0; const touchedTables = new Set(); + // Settle-tuple writes are held back from the raw apply and reconciled + // through the settle chokepoint afterwards. Applying them here would let + // a remote write land WITHOUT this host's lifecycle revision, settling + // window, or abort semantics — which is R7 in the race matrix. + // + // Held back rather than dropped: post-step-0 a peer desktop still runs + // the same chokepoint, so its decision is legitimate and must not be + // silently discarded. It just has to arrive through the front door. + const heldSettleTuple: RemoteSettleTupleChange[] = []; runStatement(db, "BEGIN IMMEDIATE"); try { for (const rawChange of changes) { @@ -4588,6 +4667,19 @@ export async function openKvDb( // Reachable whenever a table is moved local-only while a paired peer // is still on a build that replicates it — i.e. during every rollout. if (LOCAL_ONLY_CRR_EXCLUDED_TABLES.has(rawChange.table)) continue; + if (remoteSettleTupleHandler && isSettleTupleChange(rawChange)) { + const sessionId = decodeSingleTextCrsqlPrimaryKey(rawChange.pk); + if (sessionId) { + heldSettleTuple.push({ + sessionId, + column: rawChange.cid as RemoteSettleTupleChange["column"], + value: rawChange.val, + }); + continue; + } + // Undecodable key: fall through to the plain apply rather than + // dropping a change nobody will ever resend. + } const change = normalizeIncomingCrsqlChange(db, rawChange); const result = runStatement( db, @@ -4617,6 +4709,19 @@ export async function openKvDb( throw err; } + // AFTER the commit on purpose. The handler writes through the chokepoint, + // and re-entering a write inside this `BEGIN IMMEDIATE` would put a + // peer's whole batch at risk of rollback over one session row. + if (heldSettleTuple.length && remoteSettleTupleHandler) { + try { + remoteSettleTupleHandler(heldSettleTuple); + touchedTables.add("terminal_sessions"); + } catch { + // Reconciliation is best-effort: the rest of the batch already landed + // and must not be undone by a single session's settle decision. + } + } + return { appliedCount, dbVersion: sync.getDbVersion(), diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index 95d2a2849..79406434f 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -1,10 +1,11 @@ # Settle teardown — design -**Status:** reviewed and approved with amendments. Steps 0-2 of §5 are cleared -to implement; step 3 (attaching real teardown) waits until 1 and 2 are merged -and the race-matrix tests have been seen to pass. +**Status:** implemented. Steps 0-2 shipped in #1069, #1073 and #1075; the +race-matrix review passed and cleared step 3, which attaches real teardown. §6 +records what step 3 actually built, including where it departed from the plan +and why. -**Steps 0, 1 and 2 are implemented.** Step 0's host-side half is "Host enforcement +**Steps 0-3 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. @@ -414,7 +415,7 @@ 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. -### 3c-iii. Step 3 must bound the PR-merge retry (open requirement) +### 3c-iii. Bounding the PR-merge retry — resolved When `settleSessions` reports an abort, `prMergeAutoSettlementService` leaves the merged PR unhandled so a later poll retries it — otherwise the merge is consumed @@ -433,7 +434,7 @@ different way, so the next attempt should start from why: | Elapsed timer | Re-arms while a long turn is still running — exactly the case the bound exists to prevent. | | `session.runtimeState !== "running"` on the persisted row | Never observes turn completion for chat at all. Chat rows deliberately hold `status = "running"` between turns; only `chatSessionProjection` resolves an idle chat to `idle`. | -The workable signal is therefore **projected** chat state. Step 2 wires it as a +The workable signal is therefore **projected** chat state. It is wired as a narrow injected callback (`getChatLiveness`, matching `chatMentionService.listChatSessions`): the chat service answers `status` and `awaitingInput`, and only a tracked CLI session — whose row does not lie — falls @@ -525,8 +526,109 @@ three, and that is why it produced a defect every round. directly against the revision in `settleRaceMatrix.test.ts`, including the C4/C5 swallow on all three axes — the case that decides whether a real teardown can finish at all. -3. Only then attach real teardown, reusing `stopLaneRuntimeWork`'s shape. -4. Resolve 3d by decision before step 3. +3. **Landed.** Real teardown, reusing `stopLaneRuntimeWork`'s *shape* — an + ordered list of steps, each in its own try/catch — but not its body: that + function disposes chat sessions because it serves lane deletion, and a settle + must leave the session usable. See §6. +4. **Resolved** — 3d option 3, implemented in §6. Steps 1 and 2 are independently valuable: the chokepoint alone would have prevented findings 1, 3, 4, 6, 7, 9, 12, 13, and 15. + + +--- + +## 6. Step 3 as built + +### 6a. The seam became async, and why that is now safe + +Step 2 shipped a synchronous seam whose return type (`SettleTeardownCompleted`, +a branded value only a synchronous body can produce) made an awaited teardown a +**compile** error. Real stops are async — `agentChatService.interrupt` returns a +promise, as does every provider stop under it — so step 3 had to remove that +guard. It was not an obstacle to route around; it was a tripwire that had done +its job. + +The guard existed because bolting a deferred teardown onto a synchronous write +path is what produced a P1 in each of #1059's six rounds. What changed is not +the risk but the machinery: the settling window is **exclusive** (a second +settle joins rather than starting its own teardown, R4), **abortable** (a turn +start trips it mid-flight, R1/R6), and **in-memory** so a crash resolves to +not-settled. That is precisely what makes it safe to *hold across an await*. The +revision re-check and the abort check after the await are the suspension-point +guards, and the race matrix exercises both. + +So: `settleManyWithTeardown` is `async`, and `settleSessions`, +`settleSessionsReportingAborts`, `settleSession` and +`settleSessionReportingAbort` return promises. The typed outcome is unchanged. + +### 6b. What teardown actually stops + +`sessionSettleTeardown.ts`. Ordered, cheapest-to-lose first, abort checked +**before** each step, since the point of the abort is to skip work not yet done. + +| Step | Behavior | +|---|---| +| Read active work | No turn and no background work -> return immediately. A settle with nothing to tear down must not interrupt the session. | +| `interrupt` | Stops the active turn and its background work. A throw is `rejected`, not a silent pass. | +| Confirm | Poll `getSessionSummary` until quiet or the 5s budget expires. A single read straight after `interrupt` would call work that was already stopping "residue". | + +**Terminals are never touched, at any step.** A settle files a session as done; +it does not take the user's shell away, and ADE cannot re-spawn one it killed. + +### 6c. Residue (3d option 3, as implemented) + +Anything still running when the budget expires is recorded, and the settle still +lands. Each item carries a coarse `reason` — `no_stop_control` (a Codex chat has +no per-subagent stop at all), `timeout`, or `rejected` — and a `reapable` flag. +Everything counted is still a child of this ADE process, so it stays eligible +for the ppid-based orphan reaper; work that escaped the tree +(`nohup`/`setsid`/`disown`) is invisible to the confirmation read and is +therefore never folded into that count rather than being overstated as reapable. + +Residue lives in `session_settle_residue`, a **local-only** table: it describes +processes on this host, and a peer showing "1 job could not be stopped" for a +machine it cannot see would be a lie. `getSettleResidue` returns null unless the +session is still settled, so reactivating a session clears the marker without a +second write path to keep in sync. Residue is recorded **only when the settle +actually landed** — an abandoned settle has no settled row to describe. + +Analytics: one `ade_feature_used` per settle that had residue — never one per +failed job — with `provider`, the coarse `outcome` reason, and a bucketed +`count_bucket`. No session ids, task ids, commands, or error text. + +### 6d. Peer tuple writes: host authority finished, not consensus added + +R7's fix is in the **apply layer**, and it is a hold-back rather than a drop. + +`db.sync.applyChanges` is the single place both the host and peer paths funnel +through, so the check lives there: an inbound change to `settled_at`, +`settle_override` or `settle_source` is held out of the raw apply and handed to +`sessionService.reconcileRemoteSettleTuple`, which replays the remote's intent +**through the chokepoint**. The remote decision therefore gains this host's +revision bump, settling-window exclusion and abort semantics. + +Three details that are load-bearing: + +- **Held, not dropped.** Step 0 drops these columns from *phone* peers, and its + own comment says a paired desktop's settle writes "must keep replicating" + because that desktop runs the same chokepoint. Both are true: the decision is + legitimate, it just has to arrive through the front door. Extending the phone + drop to desktops would discard a real decision. +- **After the commit.** The handler writes, and re-entering a write inside the + batch's `BEGIN IMMEDIATE` would risk rolling back a peer's entire changeset + over one session row. +- **Undecodable key -> plain apply.** Only a single TEXT primary key is decoded. + Anything else is not claimed, so an unfamiliar encoding degrades to today's + behavior instead of a silently dropped change. + +**No peer-visible concurrency token was built.** That is a protocol change, and +the evidence does not justify it yet — which is what `onRemoteSettleWrite` is +for. Post-step-0 it should never fire; if the field says otherwise, we will know +which columns and how often before designing anything. + +R7/R7b are unchanged and still write the row with a raw `db.run` — a bypass no +inbound changeset can produce any more. They are kept because they pin the +property that motivates the interception: a write that reaches the tuple without +the chokepoint is invisible to the guard. Deleting them would delete the +evidence for the fix. The reconciled path is asserted separately. diff --git a/docs/logging.md b/docs/logging.md index d3a771333..2ef02771a 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -119,6 +119,26 @@ key per preference combination bounds this to at most four accepted events per installation per UTC day, within the existing `ade_feature_used` and shared daily ceilings. +Settle teardown records two things at the session-service owner boundary, both +on the existing `ade_feature_used` event with `feature: "work"`. + +`action: "settle_teardown_residue"` fires when a settle landed but a stop could +not be confirmed (the design's 3d option 3). It carries `provider`, a coarse +`outcome` reason (`no_stop_control`, `timeout`, or `rejected`) and a bucketed +`count_bucket` (`1`, `2_5`, `6_plus`). **One event per settle, never one per +failed job** — a fleet that fails to stop must not become a burst — and the +bucket exists so a large fleet cannot widen the dimension either. No session id, +task id, command, or error text is recorded; the human-readable residue detail +stays on the local diagnostics row and never enters the payload. + +`action: "settle_remote_write_reconciled"` fires when an inbound changeset tried +to write the settle tuple directly and had to be routed back through the +chokepoint. It carries no properties beyond the coarse `outcome`. This is +expected to be **zero** in the field after the host-authoritative change, and it +exists precisely so we learn whether a legitimate peer writer still exists +before anyone designs a protocol-level concurrency token. A non-zero rate is the +signal to escalate, not to widen the event. + Applying an update is one transaction — app swap, background service reinstalled, service restarted, service answering — and the brain half failing (the app updated but the background service never came back) is its own product-level From 692175a95a40fe75b3bf76ad5b36e48223cf85d6 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 04:05:22 -0400 Subject: [PATCH 02/14] test: cover the residue table in the CRR-exclusion invariant Step 3 adds a second host-local settle table, so the exclusion test that guards the revision table now guards both. The positive control on terminal_sessions is what keeps the assertion falsifiable. --- .../sessions/settleLifecycleWriter.test.ts | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts index 3143df0b7..2ed47dffa 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts @@ -356,7 +356,7 @@ describe("settle-lifecycle writer", () => { expect(service.get("session-1")?.lastOutputPreview).toBe("tick one"); }); - it("keeps the revision table out of CRR replication", async () => { + it("keeps the host-local settle tables 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()); @@ -381,15 +381,22 @@ describe("settle-lifecycle writer", () => { // 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. + // Residue describes processes on THIS host. Replicated, it would show a + // peer "1 job could not be stopped" for a machine it cannot see. + expect(clockTable("session_settle_residue")).toBeNull(); + // Positive control: proves the assertions above can fail. expect(clockTable("terminal_sessions")).not.toBeNull(); } - expect( - db.get<{ name: string }>( - "select name from sqlite_master where type = 'table' and name = 'session_lifecycle_revisions'", - )?.name, - ).toBe("session_lifecycle_revisions"); + for (const table of ["session_lifecycle_revisions", "session_settle_residue"]) { + expect( + db.get<{ name: string }>( + "select name from sqlite_master where type = 'table' and name = ?", + [table], + )?.name, + `${table} must exist locally even though it never replicates`, + ).toBe(table); + } }); }); From 92426ea21a4ab24064b81a161e739233a0a1d31c Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 04:38:52 -0400 Subject: [PATCH 03/14] fix: quality dual-review findings, including two blockers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Track A found two blockers, both real. 1. The brain never wired any of this. apps/ade-cli/src/bootstrap.ts built its sessionService with no teardown seam and never registered the apply-layer handler — and in a normal install the brain, not the desktop, is what applies changesets and serves phone sync, remote commands and the PR-merge poller. Teardown was a no-op for almost every settle a user can actually trigger. Both processes now build their hooks from one createSettleTeardownWiring factory so they cannot drift. 2. Holding settle-tuple rows out of crsql_changes breaks LWW convergence. The reviewer proved it against the vendored cr-sqlite build: a column that never enters crsql_changes never raises the local col_version, so this host stays behind the peer permanently and its NEXT genuine decision loses every merge. Two hosts then disagree forever — strictly worse than the bypass being fixed. Reversed: CRR now owns the values and the chokepoint owns the revision, via an observeRemote intent that self-assigns (matches the row, so the revision bumps; changes nothing, so no new column version and no echo). This also dissolves the composed-intent and lost-batch findings, since no intent is reconstructed and nothing is held. Also fixed: stop_and_clear was destroying the user's queued turns on every settle (now stop_only — 3c says losing a settle costs a click, losing the user's work does not); both new analytics properties were silently dropped by the sanitizer, so the telemetry the design leans on did not work, now pinned by a test; unbounded provider awaits could hold the settling window open forever and leave a row permanently unsettleable; count_bucket measured a value that was always 1; session_settle_residue leaked past deleteSession; residue analytics fired for settles that never landed; the settling window could be closed by an owner that no longer held it. Track B: settleSession now delegates to the typed form instead of duplicating it, the provider stop-control fact moved to subagentCapabilities, dead fields (stopped, scheduled_work, reapable) removed, two orphaned JSDoc blocks reattached, stale test title and a dangling comment asserting the opposite of the design deleted, and residue got a read path via the action registry — 'discoverable' was a condition of 3d option 3, not a nice-to-have. --- apps/ade-cli/src/bootstrap.ts | 53 ++- apps/desktop/src/main/main.ts | 73 ++-- .../src/main/services/adeActions/registry.ts | 15 + .../analytics/productAnalyticsPolicy.ts | 6 +- .../analytics/productAnalyticsService.test.ts | 36 ++ .../services/history/operationService.test.ts | 2 +- .../onboarding/onboardingService.test.ts | 2 +- .../prs/prMergeAutoSettlementService.ts | 2 +- .../main/services/sessions/sessionService.ts | 356 ++++++++---------- .../sessions/sessionSettleTeardown.test.ts | 63 +++- .../sessions/sessionSettleTeardown.ts | 151 ++++++-- .../sessions/settleLifecycleWriter.ts | 17 +- .../sessions/settleRaceMatrix.test.ts | 88 +++-- .../services/sessions/settleTeardownWiring.ts | 86 +++++ .../sessions/settlingStateRegistry.ts | 73 +--- apps/desktop/src/main/services/state/kvDb.ts | 69 ++-- .../src/shared/subagentCapabilities.ts | 11 + .../settle-teardown-design.md | 87 +++-- 18 files changed, 750 insertions(+), 440 deletions(-) create mode 100644 apps/desktop/src/main/services/sessions/settleTeardownWiring.ts diff --git a/apps/ade-cli/src/bootstrap.ts b/apps/ade-cli/src/bootstrap.ts index d09b68a91..b28a6c180 100644 --- a/apps/ade-cli/src/bootstrap.ts +++ b/apps/ade-cli/src/bootstrap.ts @@ -26,6 +26,12 @@ import { createSessionService, STALE_RUNNING_SESSION_FRESH_ACTIVITY_GRACE_MS, } from "../../desktop/src/main/services/sessions/sessionService"; +import { createSettleTeardownWiring } from "../../desktop/src/main/services/sessions/settleTeardownWiring"; +import type { + SettleResidueItem, + SettleTeardownContext, + SettleTeardownOutcome, +} from "../../desktop/src/main/services/sessions/sessionSettleTeardown"; import { createProjectConfigService } from "../../desktop/src/main/services/config/projectConfigService"; import { createConflictService } from "../../desktop/src/main/services/conflicts/conflictService"; import { createGitOperationsService } from "../../desktop/src/main/services/git/gitOperationsService"; @@ -755,7 +761,30 @@ export async function createAdeRuntime(args: { // services. Session changes still use it once publishing is attached. let pushPublisherForPtySignals: PushPublisherService | null = null; let ptyServiceForSessionChanges: ReturnType | null = null; - const sessionService = createSessionService({ db }); + // Late-bound: the chat service that owns the work is constructed further + // down. Without this the brain — which owns phone sync, remote commands and + // the PR-merge poller in a normal install — would settle sessions while + // stopping nothing. + const settleTeardownRef: { + run: ((sessionId: string, ctx: SettleTeardownContext) => Promise) | null; + report: ((args: { columns: string[] }) => void) | null; + residue: ((args: { provider: string | null; items: SettleResidueItem[] }) => void) | null; + } = { run: null, report: null, residue: null }; + const sessionService = createSessionService({ + db, + runSettleTeardown: async (sessionId, ctx) => + settleTeardownRef.run ? await settleTeardownRef.run(sessionId, ctx) : { residue: [] }, + onRemoteSettleWrite: (args) => settleTeardownRef.report?.(args), + onSettleResidue: (args) => settleTeardownRef.residue?.(args), + }); + // Inbound settle-tuple writes get this host's lifecycle revision, so an + // in-flight settle can see a peer's decision and abandon rather than + // overwrite it. Registered here because the DB layer must not know what a + // settle means — and because the brain, not the desktop, is where changesets + // are actually applied in a normal install. + db.sync.setRemoteSettleTupleHandler((changes) => { + sessionService.reconcileRemoteSettleTuple(changes); + }); sessionService.onChanged((event) => { pushEvent("runtime", { type: "terminal_session_changed", event }); const session = sessionService.get(event.sessionId); @@ -1249,6 +1278,28 @@ export async function createAdeRuntime(args: { countActiveForLane: (laneId) => agentChatService.countActiveForLane(laneId), disposeForLane: (laneId) => agentChatService.disposeForLane(laneId), }; + const settleWiring = createSettleTeardownWiring({ + agentChatService, + logger, + captureAnalytics: ({ action, outcome, provider, countBucket }) => { + productAnalyticsService?.captureInternal({ + event: "ade_feature_used", + // The brain reports as "api": it is the non-GUI runtime surface, and + // it is what the other brain-side analytics here already use. + surface: "api", + properties: { + feature: "work", + action, + outcome, + ...(provider ? { provider } : {}), + ...(countBucket ? { count_bucket: countBucket } : {}), + }, + }); + }, + }); + settleTeardownRef.run = settleWiring.runSettleTeardown; + settleTeardownRef.report = settleWiring.onRemoteSettleWrite; + settleTeardownRef.residue = settleWiring.onSettleResidue; } autoRebaseActivityReady = true; void autoRebaseService diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index 3e9c1ca43..cdc1c89b9 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -85,8 +85,8 @@ import { releaseLaneRuntimeResources } from "./services/lanes/laneRuntimeLifecyc import { createOAuthRedirectService } from "./services/lanes/oauthRedirectService"; import { createRuntimeDiagnosticsService } from "./services/lanes/runtimeDiagnosticsService"; import { createSessionService } from "./services/sessions/sessionService"; -import { createSessionSettleTeardown, residueCountBucket } from "./services/sessions/sessionSettleTeardown"; -import type { SettleTeardownContext, SettleTeardownOutcome } from "./services/sessions/settlingStateRegistry"; +import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } from "./services/sessions/sessionSettleTeardown"; +import { createSettleTeardownWiring } from "./services/sessions/settleTeardownWiring"; import { createSessionDeltaService } from "./services/sessions/sessionDeltaService"; import { createPtyService } from "./services/pty/ptyService"; import { createSupervisedPtyLoader } from "./services/pty/supervisedPtyHost"; @@ -2878,18 +2878,14 @@ app.whenReady().then(async () => { const settleTeardownRef: { run: ((sessionId: string, ctx: SettleTeardownContext) => Promise) | null; } = { run: null }; + const settleRemoteWriteRef: { + report: ((args: { columns: string[] }) => void) | null; + residue: ((args: { provider: string | null; items: SettleResidueItem[] }) => void) | null; + } = { report: null, residue: null }; const sessionService = createSessionService({ db, - onRemoteSettleWrite: ({ columns }) => { - // Expected to be zero post-step-0. Coarse on purpose: the column names - // are a fixed set, and no session id or value is recorded. - logger.warn("settle.remote_tuple_write_reconciled", { columns }); - productAnalyticsService?.captureInternal({ - event: "ade_feature_used", - surface: "desktop", - properties: { feature: "work", action: "settle_remote_write_reconciled", outcome: "partial" }, - }); - }, + onRemoteSettleWrite: (args) => settleRemoteWriteRef.report?.(args), + onSettleResidue: (args) => settleRemoteWriteRef.residue?.(args), runSettleTeardown: async (sessionId, ctx) => settleTeardownRef.run ? await settleTeardownRef.run(sessionId, ctx) @@ -3633,37 +3629,28 @@ app.whenReady().then(async () => { countActiveForLane: (laneId) => agentChatService.countActiveForLane(laneId), disposeForLane: (laneId) => agentChatService.disposeForLane(laneId), }; - settleTeardownRef.run = createSessionSettleTeardown({ - interrupt: async (sessionId) => { - await agentChatService.interrupt({ sessionId, mode: "stop_and_clear" }); - }, - readActiveWork: async (sessionId) => { - const summary = await agentChatService.getSessionSummary(sessionId); - if (!summary) return null; - return { - active: summary.status === "active", - backgroundTaskCount: summary.activeBackgroundTaskCount ?? 0, - provider: summary.provider ?? null, - }; - }, - logger, - onResidue: ({ provider, items }) => { - // One event per settle that had residue, not one per failed job: a - // fleet that fails to stop must not become a burst. Coarse properties - // only — no session id, task id, command, or error text. - productAnalyticsService?.captureInternal({ - event: "ade_feature_used", - surface: "desktop", - properties: { - feature: "work", - action: "settle_teardown_residue", - outcome: items[0]?.reason ?? "failed", - count_bucket: residueCountBucket(items.length), - ...(provider ? { provider } : {}), - }, - }); - }, - }); + { + const wiring = createSettleTeardownWiring({ + agentChatService, + logger, + captureAnalytics: ({ action, outcome, provider, countBucket }) => { + productAnalyticsService?.captureInternal({ + event: "ade_feature_used", + surface: "desktop", + properties: { + feature: "work", + action, + outcome, + ...(provider ? { provider } : {}), + ...(countBucket ? { count_bucket: countBucket } : {}), + }, + }); + }, + }); + settleTeardownRef.run = wiring.runSettleTeardown; + settleRemoteWriteRef.report = wiring.onRemoteSettleWrite; + settleRemoteWriteRef.residue = wiring.onSettleResidue; + } autoRebaseActivityReady = true; void autoRebaseService .refreshActiveRebaseNeeds("activity_services_ready") diff --git a/apps/desktop/src/main/services/adeActions/registry.ts b/apps/desktop/src/main/services/adeActions/registry.ts index a62661ad3..04572e5c5 100644 --- a/apps/desktop/src/main/services/adeActions/registry.ts +++ b/apps/desktop/src/main/services/adeActions/registry.ts @@ -258,6 +258,7 @@ export const ADE_ACTION_CTO_ONLY: Partial { + const record = readObjectActionArg(args, "session.getSettleResidue"); + const sessionId = typeof record.sessionId === "string" ? record.sessionId : ""; + if (!sessionId) throw new Error("session.getSettleResidue requires sessionId."); + return sessionService.getSettleResidue(sessionId) ?? { items: [] }; + }, // ----------------------------------------------------------------------- // Snooze / wake / settle-override. Snooze is a synced VISIBILITY overlay: // it hides a row until its deadline without touching lifecycle columns, so diff --git a/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts b/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts index ba4eab793..b7a06679e 100644 --- a/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts +++ b/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts @@ -93,7 +93,7 @@ const STRING_PROPERTIES = new Set([ "duration_bucket", "error_kind", "route_kind", "connection_state", "drop_reason", "source", "mode", "entry_point", "release_channel", "summary_kind", "reason", "last_command", "leg", "code", "escalation_reason", "install_source", "trigger", "from_version", "to_version", "user_action", - "tool_error_kind", "crash_reason", + "tool_error_kind", "crash_reason", "count_bucket", ]); const NUMBER_PROPERTIES = new Set([ "sent_count", "dropped_count", "interaction_count", "session_count", "chat_session_count", @@ -120,6 +120,10 @@ const ANALYTICS_ONLY_ACTIONS = new Set([ "mention_expanded", "transaction_failed", "scope_selected", + // Settle teardown: work a settle could not confirm it stopped, and a peer + // settle-tuple write that had to be reconciled through the chokepoint. + "settle_teardown_residue", + "settle_remote_write_reconciled", ]); const EVENT_PROPERTY_KEYS: Record> = { diff --git a/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts b/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts index 7eead108f..facae2c2c 100644 --- a/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts +++ b/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts @@ -1213,6 +1213,42 @@ describe("product analytics producers", () => { })).toMatchObject({ provider: "pi" }); }); + it("keeps the settle-teardown properties through the sanitizer", () => { + // Both of these were silently dropped when first added: `action` is + // allowlisted separately from the event's key list, and `count_bucket` was + // registered in the key list and the value allowlist but never in the + // string-dispatch set, so it never reached either. The event still shipped, + // just anonymous — which is worse than not shipping, because the dashboard + // looks populated. + expect(sanitizeProductAnalyticsProperties("ade_feature_used", { + feature: "work", + action: "settle_teardown_residue", + outcome: "no_stop_control", + provider: "codex", + count_bucket: "2_5", + })).toEqual({ + feature: "work", + action: "settle_teardown_residue", + outcome: "no_stop_control", + provider: "codex", + count_bucket: "2_5", + }); + + expect(sanitizeProductAnalyticsProperties("ade_feature_used", { + feature: "work", + action: "settle_remote_write_reconciled", + outcome: "partial", + })).toMatchObject({ action: "settle_remote_write_reconciled" }); + + // The bucket is still a closed set: a raw count must not slip through and + // widen the dimension. + expect(sanitizeProductAnalyticsProperties("ade_feature_used", { + feature: "work", + action: "settle_teardown_residue", + count_bucket: "37", + })).not.toHaveProperty("count_bucket"); + }); + it("maps automation completion and failed chat turns into canonical bounded outcomes", () => { const captures: ProductAnalyticsCapture[] = []; const analytics = settledAnalytics(captures); diff --git a/apps/desktop/src/main/services/history/operationService.test.ts b/apps/desktop/src/main/services/history/operationService.test.ts index e1f4ed497..a97d0b2db 100644 --- a/apps/desktop/src/main/services/history/operationService.test.ts +++ b/apps/desktop/src/main/services/history/operationService.test.ts @@ -87,7 +87,7 @@ function createInMemoryAdeDb(): { db: AdeDb; raw: Database } { rebuiltFts: false, }), discardUnpublishedChangesForTables: () => {}, - setRemoteSettleTupleHandler: () => {}, + setRemoteSettleTupleHandler: () => {}, }, flushNow: () => undefined, close: () => raw.close(), diff --git a/apps/desktop/src/main/services/onboarding/onboardingService.test.ts b/apps/desktop/src/main/services/onboarding/onboardingService.test.ts index a6b7c448e..a1db50bfb 100644 --- a/apps/desktop/src/main/services/onboarding/onboardingService.test.ts +++ b/apps/desktop/src/main/services/onboarding/onboardingService.test.ts @@ -33,7 +33,7 @@ function createInMemoryAdeDb(): AdeDb { exportChangesSince: () => [], applyChanges: () => ({ appliedCount: 0, dbVersion: 0, touchedTables: [], rebuiltFts: false }), discardUnpublishedChangesForTables: () => {}, - setRemoteSettleTupleHandler: () => {}, + setRemoteSettleTupleHandler: () => {}, }, flushNow: () => {}, close: () => {} diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index 54aa8ef7c..b4338b88c 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -280,7 +280,7 @@ export function createPrMergeAutoSettlementService(args: { // wait for inactivity would never stop that work again, because the // work is exactly what it would be waiting on. `lifecycle_changed` and // `joined_in_flight` are momentary and clear on their own. - if (settleResult.aborted.some((entry: { reason: string }) => ACTIVITY_ABORTS.has(entry.reason))) { + if (settleResult.aborted.some((entry) => ACTIVITY_ABORTS.has(entry.reason))) { abortedSessionIds.add(session.id); } } diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index 793cfdcb1..1aea52742 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -1,7 +1,8 @@ import fs from "node:fs"; -import type { AdeDb, RemoteSettleTupleChange, SqlValue } from "../state/kvDb"; +import type { AdeDb, RemoteSettleTupleChange } from "../state/kvDb"; import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; -import type { SettleAbortedReason, SettleAbortedSession, SettleResidueItem, SettleSessionsOutcome, SettleTeardownContext, SettleTeardownOutcome } from "./settlingStateRegistry"; +import type { SettleAbortedReason, SettleAbortedSession, SettleSessionsOutcome } from "./settlingStateRegistry"; +import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } from "./sessionSettleTeardown"; import type { ClaudeSessionPointer, SessionAttentionSource, @@ -156,15 +157,6 @@ function normalizeAttentionSource(value: unknown): SessionAttentionSource | null : null; } -/** - * A settle-tuple column as it arrives over the wire. Only text and null are - * expected; anything else is not a value these columns can hold, so it becomes - * null rather than being coerced into a shape the guard would misread. - */ -function toSqlScalar(value: unknown): SqlValue { - return typeof value === "string" || typeof value === "number" ? value : null; -} - function normalizeSettleSource(value: unknown): SessionSettleSource | null { return value === "agent_explicit" || value === "user" || value === "pr_merge" || value === "operator" ? value @@ -379,6 +371,7 @@ export function createSessionService({ db, runSettleTeardown, onRemoteSettleWrite, + onSettleResidue, }: { db: AdeDb; /** @@ -406,10 +399,11 @@ export function createSessionService({ * protocol-level token is justified. */ onRemoteSettleWrite?: (args: { columns: string[] }) => void; + /** Fired only for residue attached to a settle that actually landed. */ + onSettleResidue?: (args: { provider: string | null; items: SettleResidueItem[] }) => void; }) { const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); - // Every settle-tuple mutation goes through this writer; see // `settleLifecycleWriter.ts` for why it is its own module and what the // revision guarantees. @@ -759,17 +753,6 @@ export function createSessionService({ return newlySettled; }; - /** - * Settle through the settling window: the shape a real teardown will run in. - * - * Per session: read the revision, open the window, run teardown, then apply - * the settle ONLY if nothing moved. "Nothing moved" is two checks that catch - * different things — the abort flag (a human decision arrived and said so) and - * the revision (anything else changed the settle tuple, including a change - * this host did not make through a caller). - * - * Teardown is real from step 3, and it is AWAITED inside the window. - */ /** * Record what teardown could not confirm it stopped (§3d option 3). * @@ -790,6 +773,17 @@ export function createSessionService({ } }; + /** + * Settle through the settling window: the shape a real teardown will run in. + * + * Per session: read the revision, open the window, run teardown, then apply + * the settle ONLY if nothing moved. "Nothing moved" is two checks that catch + * different things — the abort flag (a human decision arrived and said so) and + * the revision (anything else changed the settle tuple, including a change + * this host did not make through a caller). + * + * Teardown is real from step 3, and it is AWAITED inside the window. + */ const settleManyWithTeardown = async ( sessionIds: string[], options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, @@ -855,18 +849,116 @@ export function createSessionService({ // stopped" marker on a row that is not settled. if (changed.length && teardown?.residue.length) { recordSettleResidue(id, teardown.residue); + // Reported HERE, not inside teardown: an abort arriving between + // teardown returning and the guards above means the settle never + // landed, and analytics must not claim residue for a settle that + // does not exist. + onSettleResidue?.({ provider: teardown.provider ?? null, items: teardown.residue }); } } finally { - settleLifecycle.settling.end(id); + settleLifecycle.settling.end(id, begin.token); } } return { settled, aborted }; }; + /** + * Settle ONE session and say why if it did not take. + * + * A local function rather than an object method: both public entry points + * delegate here, and a method would break the moment a caller destructured it + * off the service. + */ + const settleOneReportingAbort = async ( + sessionId: string, + opts: { outcome?: string | null; settledAt?: string; source?: SessionSettleSource } = {}, + ): Promise<{ found: boolean; settled: boolean; abortedBy?: SettleAbortedReason }> => { + const trimmed = sessionId.trim(); + if (!trimmed) return { found: false, settled: false }; + const exists = db.get<{ present: number }>( + "select 1 as present from terminal_sessions where id = ? limit 1", + [trimmed], + ); + if (!exists) return { found: false, settled: false }; + const note = normalizeSessionStatusNote(opts.outcome); + const result = await settleManyWithTeardown([trimmed], { + ...(note ? { outcome: note } : {}), + settledAt: opts.settledAt, + source: opts.source, + }); + const abortedBy = result.aborted[0]?.reason; + return abortedBy + ? { found: true, settled: false, abortedBy } + : { found: true, settled: true }; + }; return { list, + /** Clears a declared settle plus any `'settled'` override. */ + unsettleSession(sessionId: string): boolean { + const changed = mutateSessionMeta(sessionId, (id) => { + writeSettleLifecycle({ + intent: { kind: "unsettleDeclared" }, + sessionIds: [id], + }); + }); + return changed; + }, + + /** Explicit settle override, cleared with `settled_at` on real activity. */ + setSettleOverride( + sessionId: string, + override: SessionSettleOverride | null, + source: SessionSettleSource = "user", + ): boolean { + const normalized = override == null ? null : normalizeSettleOverride(override); + const normalizedSource = normalizeSettleSource(source) ?? "user"; + return mutateSessionMeta(sessionId, (id) => { + writeSettleLifecycle({ + intent: { kind: "override", value: normalized, source: normalizedSource }, + sessionIds: [id], + }); + }); + }, + + setSettleOverrides(sessionIds: string[], override: SessionSettleOverride | null): string[] { + const ids = normalizeSessionIds(sessionIds); + if (!ids.length) return []; + const normalized = override == null ? null : normalizeSettleOverride(override); + const placeholders = ids.map(() => "?").join(", "); + const present = db.all<{ id: string }>( + `select id from terminal_sessions where id in (${placeholders})`, + ids, + ).map((row) => row.id); + if (!present.length) return []; + const updatePlaceholders = present.map(() => "?").join(", "); + writeSettleLifecycle({ + intent: { kind: "override", value: normalized, source: "user" }, + sessionIds: present, + }); + for (const id of present) { + emitChanged({ sessionId: id, reason: "meta-updated" }); + } + return present; + }, + + /** + * The host-local settle concurrency token for a session. + * + * Read it before a decision that takes time, and require it to be unchanged + * before applying that decision — that is the whole point of the + * chokepoint. 0 means "no settle-lifecycle mutation has been recorded for + * this session", which a caller must treat as a real value, not as absent. + */ + getSettleLifecycleRevision(sessionId: string): number { + return settleLifecycle.readRevision(sessionId); + }, + + async settleSessions(sessionIds: string[]): Promise { + return (await settleManyWithTeardown(sessionIds)).settled; + }, + upsertClaudeSessionPointer(args: { sessionId: string; laneId: string; @@ -1580,31 +1672,13 @@ export function createSessionService({ ): Promise { // Through the settling window, like the bulk paths. This is the route a // USER takes (row menu -> settleTerminalSession -> here), which is the - // "user settle" R4 names — so it has to be joinable and abortable, and in - // step 3 it has to run teardown. Routing it here is what makes the R4 - // claim true rather than only true of bulk callers. - const trimmed = sessionId.trim(); - if (!trimmed) return false; - // `settleMany` returns [] for both "missing" and "already settled", so the - // boolean contract needs its own existence check to stay honest. - const exists = db.get<{ present: number }>( - "select 1 as present from terminal_sessions where id = ? limit 1", - [trimmed], - ); - if (!exists) return false; - // The key must be ABSENT, not `undefined`. `settleMany` decides whether to - // touch `status_note` with hasOwnProperty, so passing `outcome: undefined` - // writes null and erases the note a previous settle left behind — which is - // what a re-settle after activity does when the user supplies no new text. - const note = normalizeSessionStatusNote(opts.outcome); - const result = await settleManyWithTeardown([trimmed], { - ...(note ? { outcome: note } : {}), - settledAt: opts.settledAt, - source: opts.source, - }); - // An abort is not a success. Returning `true` here because the row exists - // is the silent-success contract the typed outcome exists to remove. - return result.aborted.length === 0; + // "user settle" R4 names — so it has to be joinable and abortable, and it + // runs real teardown. + // + // Delegates rather than duplicating: the boolean form is exactly the typed + // form with the reason discarded, and keeping two copies of the existence + // probe and the option-spread is how they drift. + return (await settleOneReportingAbort(sessionId, opts)).settled; }, /** @@ -1612,102 +1686,8 @@ export function createSessionService({ * The boolean form cannot distinguish "no such session" from "a turn started * mid-settle", and rendering the second as the first misleads the user. */ - async settleSessionReportingAbort( - sessionId: string, - opts: { outcome?: string | null; settledAt?: string; source?: SessionSettleSource } = {}, - ): Promise<{ found: boolean; settled: boolean; abortedBy?: SettleAbortedReason }> { - const trimmed = sessionId.trim(); - if (!trimmed) return { found: false, settled: false }; - const exists = db.get<{ present: number }>( - "select 1 as present from terminal_sessions where id = ? limit 1", - [trimmed], - ); - if (!exists) return { found: false, settled: false }; - const note = normalizeSessionStatusNote(opts.outcome); - const result = await settleManyWithTeardown([trimmed], { - ...(note ? { outcome: note } : {}), - settledAt: opts.settledAt, - source: opts.source, - }); - const abortedBy = result.aborted[0]?.reason; - return abortedBy - ? { found: true, settled: false, abortedBy } - : { found: true, settled: true }; - }, - - /** Clears a declared settle plus any `'settled'` override. */ - unsettleSession(sessionId: string): boolean { - const changed = mutateSessionMeta(sessionId, (id) => { - writeSettleLifecycle({ - intent: { kind: "unsettleDeclared" }, - sessionIds: [id], - }); - }); - return changed; - }, - - /** Explicit settle override, cleared with `settled_at` on real activity. */ - setSettleOverride( - sessionId: string, - override: SessionSettleOverride | null, - source: SessionSettleSource = "user", - ): boolean { - const normalized = override == null ? null : normalizeSettleOverride(override); - const normalizedSource = normalizeSettleSource(source) ?? "user"; - return mutateSessionMeta(sessionId, (id) => { - writeSettleLifecycle({ - intent: { kind: "override", value: normalized, source: normalizedSource }, - sessionIds: [id], - }); - }); - }, - - setSettleOverrides(sessionIds: string[], override: SessionSettleOverride | null): string[] { - const ids = normalizeSessionIds(sessionIds); - if (!ids.length) return []; - const normalized = override == null ? null : normalizeSettleOverride(override); - const placeholders = ids.map(() => "?").join(", "); - const present = db.all<{ id: string }>( - `select id from terminal_sessions where id in (${placeholders})`, - ids, - ).map((row) => row.id); - if (!present.length) return []; - const updatePlaceholders = present.map(() => "?").join(", "); - writeSettleLifecycle({ - intent: { kind: "override", value: normalized, source: "user" }, - sessionIds: present, - }); - for (const id of present) { - emitChanged({ sessionId: id, reason: "meta-updated" }); - } - return present; - }, - - /** - * The host-local settle concurrency token for a session. - * - * Read it before a decision that takes time, and require it to be unchanged - * before applying that decision — that is the whole point of the - * chokepoint. 0 means "no settle-lifecycle mutation has been recorded for - * this session", which a caller must treat as a real value, not as absent. - */ - getSettleLifecycleRevision(sessionId: string): number { - return settleLifecycle.readRevision(sessionId); - }, - - async settleSessions(sessionIds: string[]): Promise { - return (await settleManyWithTeardown(sessionIds)).settled; - }, + settleSessionReportingAbort: settleOneReportingAbort, - /** - * Settle, reporting abandoned sessions explicitly. - * - * `settleSessions` leaves an aborted id simply absent from its changed-id - * list, which is *almost* the right contract — a caller cannot tell "filed" - * from "not filed, and here is why". This is that distinction, and it is - * what a caller with a durable consequence (the PR-merge auto-settle marking - * a PR handled) has to branch on. - */ /** * Reconcile inbound settle-tuple writes from a peer (design 3c-i / R7). * @@ -1717,57 +1697,45 @@ export function createSessionService({ * lifecycle revision, so an in-flight settle would neither see it nor abort * for it, and it could silently overwrite a peer's explicit reactivation. * - * Routing it back through the chokepoint gives the remote decision the same - * revision bump, settling-window exclusion and abort semantics a local one - * gets — without inventing a peer-visible concurrency token, which is a - * protocol change the evidence does not yet justify. `onRemoteWrite` is how - * we find out whether any legitimate peer writer is still out there. + * The VALUES are left to CRR merge, which is the only thing that keeps the + * per-column clocks convergent — an earlier version rebuilt the intent and + * re-decided them, which left this host's clock permanently behind the peer + * and made its next genuine decision lose every merge. What the chokepoint + * contributes is the lifecycle revision: an in-flight settle re-reads it + * after its teardown await, sees it moved, and abandons rather than + * overwriting the peer's decision. + * + * No peer-visible concurrency token is involved; that is a protocol change + * the evidence does not justify. `onRemoteSettleWrite` is how we find out + * whether any legitimate peer writer is still out there. */ reconcileRemoteSettleTuple(changes: RemoteSettleTupleChange[]): void { - const bySession = new Map>(); + const columnsBySession = new Map>(); for (const change of changes) { - const columns = bySession.get(change.sessionId) ?? new Map(); - columns.set(change.column, toSqlScalar(change.value)); - bySession.set(change.sessionId, columns); + const columns = columnsBySession.get(change.sessionId) ?? new Set(); + columns.add(change.column); + columnsBySession.set(change.sessionId, columns); } - for (const [sessionId, columns] of bySession) { - const exists = db.get<{ present: number }>( - "select 1 as present from terminal_sessions where id = ? limit 1", - [sessionId], - ); - // A settle for a row this host does not have is not reconcilable and - // not worth inventing: the row itself will arrive in the same batch or - // not at all. - if (!exists) continue; - - const hasSettledAt = columns.has("settled_at"); - const settledAt = columns.get("settled_at"); - const override = columns.get("settle_override"); - const source = normalizeSettleSource(columns.get("settle_source")) ?? "user"; - - if (hasSettledAt && typeof settledAt === "string" && settledAt) { - writeSettleLifecycle({ - intent: { kind: "settle", settledAt, source }, - guard: "(settled_at is null or settle_override is not null)", - sessionIds: [sessionId], - }); - } else if (hasSettledAt) { - writeSettleLifecycle({ intent: { kind: "unsettleDeclared" }, sessionIds: [sessionId] }); - } else if (columns.has("settle_override")) { - writeSettleLifecycle({ - intent: { - kind: "override", - value: override === "settled" || override === "active" ? override : null, - source, - }, - sessionIds: [sessionId], - }); - } else { - continue; + for (const [sessionId, columns] of columnsBySession) { + // Per session, so one unreadable row cannot discard the rest. The values + // already landed; what is at stake here is only the revision bump. + try { + // The row can be absent: a settle for a session this host has never + // seen. Nothing to reconcile, and not a peer writer worth reporting. + const exists = db.get<{ present: number }>( + "select 1 as present from terminal_sessions where id = ? limit 1", + [sessionId], + ); + if (!exists) continue; + writeSettleLifecycle({ intent: { kind: "observeRemote" }, sessionIds: [sessionId] }); + emitChanged({ sessionId, reason: "meta-updated" }); + onRemoteSettleWrite?.({ columns: [...columns].sort() }); + } catch (error) { + // Best effort per session: the peer's values are already applied, and + // one unreadable row must not cost the rest of the batch its bump. + void error; } - emitChanged({ sessionId, reason: "meta-updated" }); - onRemoteSettleWrite?.({ columns: [...columns.keys()].sort() }); } }, @@ -1796,6 +1764,15 @@ export function createSessionService({ } }, + /** + * Settle, reporting abandoned sessions explicitly. + * + * `settleSessions` leaves an aborted id simply absent from its changed-id + * list, which is *almost* the right contract — a caller cannot tell "filed" + * from "not filed, and here is why". This is that distinction, and it is + * what a caller with a durable consequence (the PR-merge auto-settle marking + * a PR handled) has to branch on. + */ async settleSessionsReportingAborts( sessionIds: string[], options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, @@ -2042,6 +2019,7 @@ export function createSessionService({ // local table with no reaper, and every other session-keyed side table is // already cascaded here. settleLifecycle.forget(trimmed); + db.run("delete from session_settle_residue where session_id = ?", [trimmed]); emitChanged({ sessionId: trimmed, reason: "deleted" }); return true; }, diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts index 51a524018..44c240305 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts @@ -12,19 +12,20 @@ describe("session settle teardown", () => { function harness(overrides: Partial = {}) { const interrupt = vi.fn(async () => {}); - const onResidue = vi.fn(); // Instant polling: the confirmation budget is real time in production, and // a test that actually slept 5s per case would be deleted within a month. let clock = 0; const run = createSessionSettleTeardown({ interrupt, readActiveWork: async () => null, - onResidue, now: () => clock, sleep: async (ms: number) => { clock += ms; }, + // Never fires unless a test asks for it, so an ordinary provider call is + // never mistaken for a hung one. + expireProviderCall: () => new Promise(() => {}), ...overrides, }); - return { run, interrupt, onResidue }; + return { run, interrupt }; } const work = (over: Partial = {}): SessionActiveWork => ({ @@ -42,22 +43,20 @@ describe("session settle teardown", () => { // A settle with nothing to tear down must not interrupt the session: that // would be a visible side effect on a row the user only meant to file. expect(interrupt).not.toHaveBeenCalled(); - expect(outcome).toEqual({ stopped: [], residue: [] }); + expect(outcome.residue).toEqual([]); }); it("stops background work and reports no residue once the session goes quiet", async () => { const states = [work({ backgroundTaskCount: 2 }), work({ backgroundTaskCount: 2 }), work()]; const readActiveWork = vi.fn(async () => states.shift() ?? work()); - const { run, interrupt, onResidue } = harness({ readActiveWork }); + const { run, interrupt } = harness({ readActiveWork }); const outcome = await run("session-1", neverAborted); expect(interrupt).toHaveBeenCalledWith("session-1"); - expect(outcome.stopped).toEqual(["interrupt"]); // The stop is asynchronous inside the provider, so a single read straight // after `interrupt` would call work that was already stopping "residue". expect(outcome.residue, "work that drained must not be reported as residue").toEqual([]); - expect(onResidue).not.toHaveBeenCalled(); }); /** @@ -66,7 +65,7 @@ describe("session settle teardown", () => { * analytics hook fires exactly once. */ it("R5: reports residue when the stop never confirms, rather than blocking the settle", async () => { - const { run, onResidue } = harness({ + const { run } = harness({ readActiveWork: async () => work({ backgroundTaskCount: 3 }), }); @@ -75,14 +74,12 @@ describe("session settle teardown", () => { expect(outcome.residue).toEqual([{ kind: "background_tasks", reason: "timeout", - reapable: true, + count: 3, detail: "3 jobs on claude could not be stopped", }]); - expect(onResidue, "residue must be measured, not just displayed").toHaveBeenCalledTimes(1); - expect(onResidue).toHaveBeenCalledWith({ - provider: "claude", - items: outcome.residue, - }); + // The provider rides along for the analytics dimension. Reporting happens in + // the settle path, not here, so an abandoned settle cannot claim residue. + expect(outcome.provider).toBe("claude"); }); it("R5: calls out a provider that has no stop control at all", async () => { @@ -106,8 +103,6 @@ describe("session settle teardown", () => { const outcome = await run("session-1", neverAborted); expect(outcome.residue[0]?.reason).toBe("rejected"); - // The stop never landed, so it must not be claimed as a completed step. - expect(outcome.stopped).toEqual([]); }); /** @@ -122,12 +117,12 @@ describe("session settle teardown", () => { const outcome = await run("session-1", { isAborted: () => true }); expect(interrupt, "an aborted settle must not stop the work that won the race").not.toHaveBeenCalled(); - expect(outcome).toEqual({ stopped: [], residue: [] }); + expect(outcome.residue).toEqual([]); }); it("does not report residue for a session the user reclaimed mid-teardown", async () => { let aborted = false; - const { run, onResidue } = harness({ + const { run } = harness({ interrupt: vi.fn(async () => { aborted = true; }), readActiveWork: async () => work({ active: true, backgroundTaskCount: 1 }), }); @@ -138,7 +133,37 @@ describe("session settle teardown", () => { // "1 job could not be stopped" marker on. Reporting it would label a // session that is actively working as one that failed to stop. expect(outcome.residue).toEqual([]); - expect(onResidue).not.toHaveBeenCalled(); + }); + + it("does not hang the settling window on a provider call that never resolves", async () => { + const { run } = harness({ + // A control call that never settles. Without a per-call ceiling the + // settling window never closes and the row is unsettleable for the life + // of the process. + interrupt: vi.fn(() => new Promise(() => {})), + readActiveWork: async () => work({ backgroundTaskCount: 1 }), + expireProviderCall: async () => {}, + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue[0]?.reason).toBe("rejected"); + }); + + it("resolves instead of blocking the settle when a liveness read hangs", async () => { + const readActiveWork = vi.fn(() => new Promise(() => {})); + const { run, interrupt } = harness({ + readActiveWork, + expireProviderCall: async () => {}, + }); + + // The first read never resolves. Without a per-call ceiling this awaits + // forever inside the settling window, and the row can never be settled + // again for the life of the process. + await expect(run("session-1", neverAborted)).resolves.toMatchObject({ residue: [] }); + expect(readActiveWork).toHaveBeenCalledTimes(1); + // Unknown liveness is not licence to start stopping things. + expect(interrupt).not.toHaveBeenCalled(); }); it("buckets residue counts so a large fleet cannot widen the analytics dimension", () => { diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts index 800659912..521afdf7a 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts @@ -1,4 +1,6 @@ -import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } from "./settlingStateRegistry"; + + +import { PROVIDERS_WITHOUT_BACKGROUND_STOP_CONTROL } from "../../../shared/subagentCapabilities"; /** * Real settle teardown: stop the work a session owns, then confirm it stopped. @@ -20,9 +22,84 @@ import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } * order below is cheapest-to-lose first. */ -/** Bounds a single provider stop, matching the chat service's own stop budget. */ +/** + * What a teardown could not confirm it stopped (design 3d, option 3). + * + * The settle still lands — that is the signed-off decision — but it lands WITH + * this attached, so "settled" never quietly means "and something is still + * running". + * + * Everything recorded here is work ADE tracks, which is what keeps it eligible + * for the ppid-based orphan reaper. Work that escaped the process tree + * (`nohup`/`setsid`/`disown`) is invisible to the confirmation read, so it is + * never counted here — the design requires that it not be folded in and + * overstated as recoverable, and by construction it cannot be. + */ +export type SettleResidueItem = { + /** Coarse and closed: this is also the analytics dimension. */ + kind: "background_tasks" | "active_turn"; + /** + * Why the stop did not confirm. `no_stop_control` is a provider that offers + * no way to stop this work at all (a Codex chat's subagents); `timeout` and + * `rejected` are a stop that was attempted and did not land. + */ + reason: "no_stop_control" | "timeout" | "rejected"; + /** How many jobs this item covers. Bucketed before it reaches analytics. */ + count: number; + /** Human-readable, for the diagnostics surface. Never analytics. */ + detail: string; +}; + +/** Checked BETWEEN stop calls, per design 3c. A turn start trips it. */ +export type SettleTeardownContext = { + isAborted: () => boolean; +}; + +/** + * The result of a real teardown. + * + * This replaces step 2's synchronous `SettleTeardownCompleted` brand. That + * brand made an awaited teardown a COMPILE error while the settle path was + * still synchronous. It is safe to await now because the settling window is + * exclusive, abortable and crash-safe, so it can be HELD across the await; the + * revision re-check and abort check after the await are the guards that make + * the suspension point survivable. + */ +export type SettleTeardownOutcome = { + residue: SettleResidueItem[]; + /** For the residue analytics dimension. Null when the session has no chat. */ + provider?: string | null; +}; + +/** + * How long to keep re-reading the session after a stop before calling the work + * residue. NOT a provider stop budget — those are shorter and live in + * `agentChatService` (`CLAUDE_STOP_TASK_TIMEOUT_MS` and friends). This is the + * grace period for a stop that has been accepted and is still draining. + */ const STOP_CONFIRM_TIMEOUT_MS = 5_000; const STOP_CONFIRM_POLL_MS = 100; +/** + * Per-call ceiling for the provider calls themselves. + * + * The poll budget above bounds the LOOP, not any single await. Without this a + * provider control call that never resolves would hold the settling window + * open forever: the `finally` that closes it is unreachable, the row is stuck + * showing "Settling…", it can never be settled again for the life of the + * process, and the IPC or remote-command caller hangs with it. + */ +const PROVIDER_CALL_TIMEOUT_MS = 10_000; + +/** Resolves to not-ok rather than rejecting, so a slow provider is residue, not a crash. */ +async function withTimeout( + work: Promise, + expire: () => Promise, +): Promise<{ ok: true; value: T } | { ok: false }> { + return await Promise.race([ + work.then((value) => ({ ok: true, value }) as const), + expire().then(() => ({ ok: false }) as const), + ]); +} export type SessionActiveWork = { /** A turn is running right now. */ @@ -40,47 +117,61 @@ export type SessionSettleTeardownDeps = { interrupt: (sessionId: string) => Promise; /** Ground truth after a stop. `null` for a session the chat service does not own. */ readActiveWork: (sessionId: string) => Promise; - /** - * Providers with no way to stop background work at all. A Codex chat cannot - * stop an individual subagent, so its residue is `no_stop_control` rather - * than a stop that failed — the distinction is the whole point of the field. - */ + /** Overrides `PROVIDERS_WITHOUT_BACKGROUND_STOP_CONTROL`; tests only. */ providersWithoutStopControl?: ReadonlySet; - onResidue?: (args: { provider: string | null; items: SettleResidueItem[] }) => void; logger?: { warn: (message: string, meta?: Record) => void }; now?: () => number; + /** + * Delay between confirmation polls. Tests fast-forward this. + * + * Deliberately NOT the same seam as `expireProviderCall`: a fast-forwarding + * `sleep` would otherwise win every timeout race and make every provider call + * look like it hung. + */ sleep?: (ms: number) => Promise; + /** Fires when a single provider call has taken too long. Real timer by default. */ + expireProviderCall?: () => Promise; }; -const DEFAULT_NO_STOP_CONTROL = new Set(["codex"]); + export function createSessionSettleTeardown( deps: SessionSettleTeardownDeps, ): (sessionId: string, ctx: SettleTeardownContext) => Promise { - const noStopControl = deps.providersWithoutStopControl ?? DEFAULT_NO_STOP_CONTROL; + const noStopControl = deps.providersWithoutStopControl ?? PROVIDERS_WITHOUT_BACKGROUND_STOP_CONTROL; const now = deps.now ?? (() => Date.now()); const sleep = deps.sleep ?? ((ms: number) => new Promise((resolve) => { setTimeout(resolve, ms); })); + const expireProviderCall = deps.expireProviderCall + ?? (() => new Promise((resolve) => { setTimeout(resolve, PROVIDER_CALL_TIMEOUT_MS); })); return async (sessionId, ctx): Promise => { - const stopped: string[] = []; const residue: SettleResidueItem[] = []; - const before = await deps.readActiveWork(sessionId).catch(() => null); + const readWork = async (): Promise => { + const result = await withTimeout(deps.readActiveWork(sessionId), expireProviderCall) + .catch(() => ({ ok: false }) as const); + return result.ok ? result.value : null; + }; + + const before = await readWork(); // Nothing to stop, or a session this service does not own (a plain // terminal). Either way there is no work to lose and no residue to report. if (!before || (!before.active && before.backgroundTaskCount === 0)) { - return { stopped, residue }; + return { residue }; } const provider = before.provider; // Checked before the step, not after: the point of the abort is to stop // work we have NOT done yet. - if (ctx.isAborted()) return { stopped, residue }; + if (ctx.isAborted()) return { residue }; let stopRejected = false; try { - await deps.interrupt(sessionId); - stopped.push("interrupt"); + const stop = await withTimeout(deps.interrupt(sessionId), expireProviderCall); + if (!stop.ok) { + stopRejected = true; + deps.logger?.warn("settle_teardown.step_timed_out", { step: "interrupt" }); + } } catch (error) { stopRejected = true; deps.logger?.warn("settle_teardown.step_failed", { @@ -91,32 +182,24 @@ export function createSessionSettleTeardown( // A turn that arrived while the stop was in flight wins. Do not spend the // confirmation budget re-reading a session the user is actively using. - if (ctx.isAborted()) return { stopped, residue }; + if (ctx.isAborted()) return { residue }; - const after = await waitForQuiet(sessionId, ctx); - const stillActive = after ? after.active || after.backgroundTaskCount > 0 : false; - if (stillActive && !ctx.isAborted()) { - const remaining = (after?.backgroundTaskCount ?? 0) + (after?.active ? 1 : 0); + const after = await waitForQuiet(readWork, ctx); + if (after && (after.active || after.backgroundTaskCount > 0) && !ctx.isAborted()) { + const remaining = after.backgroundTaskCount + (after.active ? 1 : 0); residue.push({ - kind: after?.active && (after?.backgroundTaskCount ?? 0) === 0 ? "active_turn" : "background_tasks", + kind: after.active && after.backgroundTaskCount === 0 ? "active_turn" : "background_tasks", reason: stopRejected ? "rejected" : provider && noStopControl.has(provider) ? "no_stop_control" : "timeout", - // Everything counted here is still a child of this ADE process, which - // is what keeps it eligible for the ppid-based orphan reaper. Work that - // escaped the tree (`nohup`/`setsid`/`disown`) is not visible to - // `readActiveWork` at all, so it is never folded into this count — it is - // unreachable by construction, and saying otherwise would overstate what - // the reaper can clean up. - reapable: true, + count: remaining, detail: describeResidue(remaining, provider), }); } - if (residue.length) deps.onResidue?.({ provider, items: residue }); - return { stopped, residue }; + return { residue, provider }; }; /** @@ -125,15 +208,15 @@ export function createSessionSettleTeardown( * `interrupt` would report residue for work that was about to stop anyway. */ async function waitForQuiet( - sessionId: string, + read: () => Promise, ctx: SettleTeardownContext, ): Promise { const deadline = now() + STOP_CONFIRM_TIMEOUT_MS; - let latest = await deps.readActiveWork(sessionId).catch(() => null); + let latest = await read(); while (latest && (latest.active || latest.backgroundTaskCount > 0) && now() < deadline) { if (ctx.isAborted()) return latest; await sleep(STOP_CONFIRM_POLL_MS); - latest = await deps.readActiveWork(sessionId).catch(() => null); + latest = await read(); } return latest; } diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts index 02a0c69ec..f3e5c8122 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts @@ -26,7 +26,14 @@ export type SettleLifecycleIntent = | { kind: "clearOnActivity"; cause: SettleClearCause } /** Declared unsettle: drops a `'settled'` pin but preserves `'active'`. */ | { kind: "unsettleDeclared" } - | { kind: "override"; value: SessionSettleOverride | null; source: SessionSettleSource }; + | { kind: "override"; value: SessionSettleOverride | null; source: SessionSettleSource } + /** + * A peer already changed the tuple; CRR merged it. Values are untouched — + * re-deciding them here would fight the merge and desynchronise the + * per-column clocks. This exists only to move the revision, which is what + * makes an in-flight settle see that the world moved and abandon itself. + */ + | { kind: "observeRemote" }; /** * Columns a caller may set alongside the settle tuple. A closed union, so @@ -112,6 +119,14 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { + "settle_source = case when ? = 'settled' then ? when settled_at is null then null else settle_source end", params: [intent.value, intent.value, intent.source], }; + case "observeRemote": + // Self-assignment: it MATCHES the row (so `sqlite3_changes` is 1 and the + // revision bumps) while changing nothing (so cr-sqlite records no new + // column version and this does not echo back to the peer). + return { + sql: "settled_at = settled_at, settle_override = settle_override, settle_source = settle_source", + params: [], + }; } }; diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index f04bb68ba..dd4a748de 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -5,12 +5,12 @@ import { afterEach, describe, expect, it } from "vitest"; import { openKvDb } from "../state/kvDb"; import { createSessionService } from "./sessionService"; import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; -import type { SettleResidueItem, SettleTeardownContext } from "./settlingStateRegistry"; +import type { SettleResidueItem, SettleTeardownContext } from "./sessionSettleTeardown"; /** * The race matrix from the settle-teardown design (§2), tested directly against - * the lifecycle revision and the settling window — with teardown still a NO-OP. + * the lifecycle revision and the settling window — with a real, awaited teardown. * * That ordering is the point. Every one of these races was previously argued * about in review rather than executed, and the six rounds of PR #1059 are what @@ -47,7 +47,7 @@ function insertProjectGraph(db: Awaited>) { ); } -describe("settle race matrix (teardown is a no-op)", () => { +describe("settle race matrix", () => { const disposers: Array<() => Promise> = []; afterEach(async () => { while (disposers.length) await disposers.pop()?.(); @@ -72,7 +72,7 @@ describe("settle race matrix (teardown is a no-op)", () => { runSettleTeardown: async (sessionId, ctx) => { teardownContexts.push(ctx); await teardown(sessionId); - return { stopped: [], residue }; + return { residue }; }, }); const setTeardown = (fn: (sessionId: string) => void | Promise) => { @@ -363,13 +363,6 @@ describe("settle race matrix (teardown is a no-op)", () => { expect(service.settlingSessionIds()).toEqual([]); }); - /** - * An async teardown is now a COMPILE error, not a runtime one — the seam - * returns a branded value that only a synchronous body can produce, so both - * `async (id) => {}` and the adapter `id => asyncStop(id)` fail to typecheck. - * There is no runtime behaviour left to assert; the type is the test. - */ - /** A settling row found after a restart resolves to not-settled. */ it("crash safety: the settling window does not survive the process", async () => { const { service, setTeardown } = await fixture(); @@ -460,7 +453,7 @@ describe("settle race matrix (teardown is a no-op)", () => { setResidue([{ kind: "background_tasks", reason: "no_stop_control", - reapable: true, + count: 2, detail: "2 jobs on codex could not be stopped", }]); @@ -475,7 +468,7 @@ describe("settle race matrix (teardown is a no-op)", () => { expect(residue?.items).toEqual([{ kind: "background_tasks", reason: "no_stop_control", - reapable: true, + count: 2, detail: "2 jobs on codex could not be stopped", }]); }); @@ -483,7 +476,7 @@ describe("settle race matrix (teardown is a no-op)", () => { it("does not leave residue hanging on a session the user reactivated", async () => { const { service, setResidue } = await fixture(); setResidue([{ - kind: "background_tasks", reason: "timeout", reapable: true, detail: "1 job could not be stopped", + kind: "background_tasks", reason: "timeout", count: 1, detail: "1 job could not be stopped", }]); await service.settleSessionsReportingAborts(["session-1"]); expect(service.getSettleResidue("session-1")).not.toBeNull(); @@ -498,7 +491,7 @@ describe("settle race matrix (teardown is a no-op)", () => { it("records no residue for a settle that was abandoned", async () => { const { service, setResidue, setTeardown } = await fixture(); setResidue([{ - kind: "background_tasks", reason: "timeout", reapable: true, detail: "1 job could not be stopped", + kind: "background_tasks", reason: "timeout", count: 1, detail: "1 job could not be stopped", }]); setTeardown(() => { service.clearTurnStartMarkers("session-1"); @@ -517,34 +510,57 @@ describe("settle race matrix (teardown is a no-op)", () => { * semantics a local decision has — no peer-visible concurrency token. */ describe("remote settle-tuple reconciliation", () => { - it("gives a peer settle the revision bump a local settle would have got", async () => { - const { service, remoteWrites } = await fixture(); + /** + * Decision 1 for step 3: finish host authority rather than add consensus. + * + * The VALUES are applied by CRR merge before the session layer is told — + * that is what keeps the per-column clocks convergent, and an earlier + * version that rebuilt the intent instead left this host's clock behind the + * peer forever, so its next genuine decision lost every merge. What + * reconciliation adds is the lifecycle revision, which is what an in-flight + * settle re-reads after its teardown await. + */ + const applyPeerWrite = (db: Awaited>["db"], sql: string, params: unknown[]) => { + // Stands in for `applyChanges` having merged the peer's columns. + db.run(sql, params as never); + }; + + it("bumps the revision for a peer settle, without re-deciding the value", async () => { + const { db, service, remoteWrites } = await fixture(); const revisionBefore = service.getSettleLifecycleRevision("session-1"); + applyPeerWrite( + db, + "update terminal_sessions set settled_at = ?, settle_source = ? where id = ?", + ["2026-08-11T00:07:00.000Z", "user", "session-1"], + ); service.reconcileRemoteSettleTuple([ - { sessionId: "session-1", column: "settled_at", value: "2026-08-11T00:07:00.000Z" }, - { sessionId: "session-1", column: "settle_source", value: "user" }, + { sessionId: "session-1", column: "settled_at" }, + { sessionId: "session-1", column: "settle_source" }, ]); + // The peer's value stands exactly as CRR merged it. expect(service.get("session-1")?.settledAt).toBe("2026-08-11T00:07:00.000Z"); - // The whole point: an in-flight settle can now SEE that the world moved. + // And an in-flight settle can now see that the world moved. expect(service.getSettleLifecycleRevision("session-1")).toBeGreaterThan(revisionBefore); - // And we find out it happened, so we learn whether a legitimate peer - // writer still exists before deciding a protocol change is justified. expect(remoteWrites).toEqual([{ columns: ["settle_source", "settled_at"] }]); }); - it("makes a peer unsettle abort an in-flight settle instead of being overwritten", async () => { - const { service, setTeardown } = await fixture(); + it("makes a peer reactivation abort an in-flight settle instead of being overwritten", async () => { + const { db, service, setTeardown } = await fixture(); await service.settleSessionsReportingAborts(["session-1"]); service.unsettleSession("session-1"); setTeardown(() => { // The mirror case Codex raised: a peer reactivates the session while - // this host is mid-settle. Before reconciliation this bypassed the - // revision and the apply overwrote the peer's explicit decision. + // this host is mid-settle. + applyPeerWrite( + db, + "update terminal_sessions set settle_override = ? where id = ?", + ["active", "session-1"], + ); service.reconcileRemoteSettleTuple([ - { sessionId: "session-1", column: "settle_override", value: "active" }, + { sessionId: "session-1", column: "settle_override" }, ]); }); const outcome = await service.settleSessionsReportingAborts(["session-1"]); @@ -556,14 +572,26 @@ describe("settle race matrix (teardown is a no-op)", () => { expect(service.get("session-1")?.settledAt).toBeNull(); }); - it("ignores a settle for a row this host does not have", async () => { + it("ignores a change for a row this host does not have", async () => { const { service, remoteWrites } = await fixture(); + service.reconcileRemoteSettleTuple([{ sessionId: "not-here", column: "settled_at" }]); + + expect(remoteWrites).toEqual([]); + }); + + it("keeps reconciling the rest of the batch when one session fails", async () => { + const { db, service, create, remoteWrites } = await fixture(); + create("session-2"); + applyPeerWrite(db, "update terminal_sessions set settled_at = ? where id = ?", ["2026-08-11T00:07:00.000Z", "session-2"]); + service.reconcileRemoteSettleTuple([ - { sessionId: "not-here", column: "settled_at", value: "2026-08-11T00:07:00.000Z" }, + { sessionId: "not-here", column: "settled_at" }, + { sessionId: "session-2", column: "settled_at" }, ]); - expect(remoteWrites).toEqual([]); + // The missing row must not cost session-2 its revision bump. + expect(remoteWrites).toEqual([{ columns: ["settled_at"] }]); }); }); }); diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts new file mode 100644 index 000000000..4aca36283 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts @@ -0,0 +1,86 @@ +import { createSessionSettleTeardown, residueCountBucket } from "./sessionSettleTeardown"; +import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } from "./sessionSettleTeardown"; + +/** + * The settle-teardown wiring, shared by the desktop main process and the ADE + * brain. + * + * It lives here rather than at each construction site because there are TWO of + * them, and the one that matters most is the easiest to forget: in a normal + * install the brain owns phone sync, remote commands, and the PR-merge poller, + * so wiring only the desktop leaves teardown a silent no-op for every settle a + * user actually triggers from their phone or from `ade`. That is ADE's oldest + * bug class — works in-process, no-ops in the runtime-backed build — and a + * shared factory is what stops the two copies drifting apart. + */ + +/** Only what teardown needs, so neither caller has to hand over a whole service. */ +export type SettleTeardownChatService = { + interrupt: (args: { sessionId: string; mode: "stop_only" | "stop_and_clear" }) => Promise; + getSessionSummary: (sessionId: string) => Promise<{ + status: string; + activeBackgroundTaskCount?: number | null; + provider?: string | null; + } | null>; +}; + +export type SettleTeardownWiringDeps = { + agentChatService: SettleTeardownChatService; + captureAnalytics?: (args: { + action: string; + outcome: string; + provider?: string; + countBucket?: string; + }) => void; + logger?: { warn: (message: string, meta?: Record) => void }; +}; + +export type SettleTeardownWiring = { + runSettleTeardown: (sessionId: string, ctx: SettleTeardownContext) => Promise; + onRemoteSettleWrite: (args: { columns: string[] }) => void; + onSettleResidue: (args: { provider: string | null; items: SettleResidueItem[] }) => void; +}; + +export function createSettleTeardownWiring(deps: SettleTeardownWiringDeps): SettleTeardownWiring { + const runSettleTeardown = createSessionSettleTeardown({ + interrupt: async (sessionId) => { + // `stop_only`, never `stop_and_clear`: the latter also cancels the user's + // QUEUED turns. Design 3c's rule is that losing a settle costs one click + // while losing the user's work is unrecoverable, and a queued prompt is + // the user's work. If a queued turn then starts, C3 clears the settle — + // which is R1, and already the accepted trade. + await deps.agentChatService.interrupt({ sessionId, mode: "stop_only" }); + }, + readActiveWork: async (sessionId) => { + const summary = await deps.agentChatService.getSessionSummary(sessionId); + if (!summary) return null; + return { + active: summary.status === "active", + backgroundTaskCount: summary.activeBackgroundTaskCount ?? 0, + provider: summary.provider ?? null, + }; + }, + ...(deps.logger ? { logger: deps.logger } : {}), + }); + + return { + runSettleTeardown, + onSettleResidue: ({ provider, items }) => { + // One event per settle that had residue, not one per failed job: a fleet + // that fails to stop must not become a burst. Coarse properties only — + // no session id, task id, command, or error text. + deps.captureAnalytics?.({ + action: "settle_teardown_residue", + outcome: items[0]?.reason ?? "failed", + countBucket: residueCountBucket(items.reduce((total, item) => total + item.count, 0)), + ...(provider ? { provider } : {}), + }); + }, + onRemoteSettleWrite: ({ columns }) => { + // Expected to be zero once every writer is host-authoritative. The column + // names are a fixed set; no session id or value is recorded. + deps.logger?.warn("settle.remote_tuple_write_reconciled", { columns }); + deps.captureAnalytics?.({ action: "settle_remote_write_reconciled", outcome: "partial" }); + }, + }; +} diff --git a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts index e151899fa..c63a1c092 100644 --- a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts +++ b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts @@ -16,58 +16,6 @@ * would be a lie on every other device the moment this host died. */ -/** - * What a teardown could not confirm it stopped (§3d, option 3). - * - * The settle still lands — that is the signed-off decision — but it lands WITH - * this attached, so "settled" never quietly means "and something is still - * running". A label alone would just be a prettier way of losing the process, - * so each item says what it was, why the stop did not confirm, and whether the - * existing ppid-based orphan reaper can still reach it. - */ -export type SettleResidueItem = { - /** Coarse and closed: this is also the analytics dimension. */ - kind: "background_tasks" | "active_turn" | "scheduled_work"; - /** - * Why the stop did not confirm. `no_stop_control` is a provider that offers - * no way to stop this work at all (a Codex chat's subagents); `timeout` and - * `rejected` are a stop that was attempted and did not land. - */ - reason: "no_stop_control" | "timeout" | "rejected"; - /** - * Whether the orphan reaper can still reach it. Work that escaped the process - * tree (`nohup`/`setsid`/`disown`) is unreachable and must be reported as - * such rather than folded into the reapable count. - */ - reapable: boolean; - /** Human-readable, for the diagnostics surface. Never analytics. */ - detail: string; -}; - -/** Checked BETWEEN stop calls, per §3c. A turn start trips it. */ -export type SettleTeardownContext = { - isAborted: () => boolean; -}; - -/** - * The result of a real teardown. - * - * `stopped` is what confirmed, in the order attempted; `residue` is §3d. - * - * This replaces step 2's synchronous `SettleTeardownCompleted` brand. That - * brand existed to make an async teardown a COMPILE error while the settle path - * was still synchronous — bolting a deferred teardown onto a synchronous write - * was exactly the mistake that produced a P1 in each of #1059's six rounds. It - * is safe to await now for one specific reason: the settling window (step 2) is - * exclusive, abortable and crash-safe, so it can be HELD across the await. The - * revision re-check and the abort flag after the await are what make the - * suspension point survivable, and both are tested by the race matrix. - */ -export type SettleTeardownOutcome = { - stopped: string[]; - residue: SettleResidueItem[]; -}; - /** Why a settle was abandoned. Only ever set by a human-decision clearer. */ export type SettleAbortReason = "turn_start" | "turn_failed" | "attention_requested"; @@ -94,11 +42,18 @@ export type SettlingEntry = { /** The revision the settle decision was taken against. */ startedAtRevision: number; abortedBy: SettleAbortReason | null; + /** + * Identifies THIS window. `end` refuses to close a window it did not open, so + * an owner whose window was already torn down out from under it (a session + * deleted mid-teardown) cannot close the one a newer settle has since opened + * for the same id — which would leave two teardowns believing they own it. + */ + token: number; }; export type BeginSettlingResult = - /** This caller owns the window and must end it. */ - | { kind: "started" } + /** This caller owns the window and must end it, passing back its token. */ + | { kind: "started"; token: number } /** * Another settle for this session is already in flight. The caller JOINS it * rather than starting a second teardown — closing R4, where two teardowns @@ -108,11 +63,13 @@ export type BeginSettlingResult = export class SettlingStateRegistry { private readonly entries = new Map(); + private nextToken = 1; begin(sessionId: string, startedAtRevision: number): BeginSettlingResult { if (this.entries.has(sessionId)) return { kind: "joined" }; - this.entries.set(sessionId, { startedAtRevision, abortedBy: null }); - return { kind: "started" }; + const token = this.nextToken++; + this.entries.set(sessionId, { startedAtRevision, abortedBy: null, token }); + return { kind: "started", token }; } isSettling(sessionId: string): boolean { @@ -142,7 +99,9 @@ export class SettlingStateRegistry { return this.entries.get(sessionId)?.startedAtRevision ?? null; } - end(sessionId: string): void { + /** Closes the window only if `token` still owns it. Omit to force-close. */ + end(sessionId: string, token?: number): void { + if (token !== undefined && this.entries.get(sessionId)?.token !== token) return; this.entries.delete(sessionId); } diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index f80e73796..039928db5 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -910,9 +910,13 @@ const LOCAL_ONLY_CRR_EXCLUDED_TABLES = new Set([ /** * The settle tuple, as it appears in an inbound changeset. * - * Kept next to `LOCAL_ONLY_CRR_EXCLUDED_TABLES` because it answers the same - * question — "may this remote row be written straight to the table?" — and the - * two lists drift apart if they live in different files. + * This is the SECOND copy of this column set: `syncHostService`'s + * `HOST_AUTHORITATIVE_COLUMNS_BY_TABLE` has the same three for a different + * purpose (dropping phone writes outright, rather than re-asserting a peer's). + * They are deliberately separate — one is a policy about who may write, the + * other is a trigger for re-asserting the revision — but a column added to the + * settle tuple has to be added to both, and to `settleLifecycleWriter`'s + * assignment. Grep `settle_source` before changing any of them. */ const SETTLE_TUPLE_COLUMNS: ReadonlySet = new Set([ "settled_at", @@ -944,11 +948,17 @@ function decodeSingleTextCrsqlPrimaryKey(value: SyncScalar): string | null { return bytes.subarray(3, 3 + length).toString("utf8") || null; } -/** One inbound settle-tuple column write, decoded for the session layer. */ +/** + * One inbound settle-tuple column write, decoded for the session layer. + * + * Deliberately carries no value: the change has ALREADY been applied by CRR + * merge, which is the only thing that keeps the per-column clocks convergent. + * The session layer reads the resulting row, so it can never disagree with what + * actually landed. + */ export type RemoteSettleTupleChange = { sessionId: string; column: "settled_at" | "settle_override" | "settle_source"; - value: SyncScalar; }; function listEligibleCrrTables(db: DatabaseSyncType): string[] { @@ -4639,15 +4649,20 @@ export async function openKvDb( if (!crsqliteLoaded) return { appliedCount: 0, dbVersion: 0, touchedTables: [], rebuiltFts: false }; let appliedCount = 0; const touchedTables = new Set(); - // Settle-tuple writes are held back from the raw apply and reconciled - // through the settle chokepoint afterwards. Applying them here would let - // a remote write land WITHOUT this host's lifecycle revision, settling - // window, or abort semantics — which is R7 in the race matrix. + // Settle-tuple writes APPLY normally and are then reported to the + // session layer, which re-asserts them through the settle chokepoint. // - // Held back rather than dropped: post-step-0 a peer desktop still runs - // the same chokepoint, so its decision is legitimate and must not be - // silently discarded. It just has to arrive through the front door. - const heldSettleTuple: RemoteSettleTupleChange[] = []; + // An earlier version held them out of `crsql_changes` and rebuilt the + // intent afterwards. That is wrong, and measurably so: cr-sqlite merges + // last-writer-wins on a per-column `col_version`, and a column that never + // enters `crsql_changes` never raises the local counter. The host then + // stays behind the peer forever, so its NEXT genuine decision — a user + // unsettle, a keep-active pin — carries a lower version and is rejected + // by every peer. Two hosts disagree permanently, which is far worse than + // the bypass being fixed. Let CRR converge the values; the chokepoint's + // job here is the lifecycle revision, which is what makes an in-flight + // settle notice and abort. + const remoteSettleTuple: RemoteSettleTupleChange[] = []; runStatement(db, "BEGIN IMMEDIATE"); try { for (const rawChange of changes) { @@ -4669,16 +4684,13 @@ export async function openKvDb( if (LOCAL_ONLY_CRR_EXCLUDED_TABLES.has(rawChange.table)) continue; if (remoteSettleTupleHandler && isSettleTupleChange(rawChange)) { const sessionId = decodeSingleTextCrsqlPrimaryKey(rawChange.pk); + // An undecodable key still applies; it just is not reconciled. if (sessionId) { - heldSettleTuple.push({ + remoteSettleTuple.push({ sessionId, column: rawChange.cid as RemoteSettleTupleChange["column"], - value: rawChange.val, }); - continue; } - // Undecodable key: fall through to the plain apply rather than - // dropping a change nobody will ever resend. } const change = normalizeIncomingCrsqlChange(db, rawChange); const result = runStatement( @@ -4709,17 +4721,20 @@ export async function openKvDb( throw err; } - // AFTER the commit on purpose. The handler writes through the chokepoint, - // and re-entering a write inside this `BEGIN IMMEDIATE` would put a - // peer's whole batch at risk of rollback over one session row. - if (heldSettleTuple.length && remoteSettleTupleHandler) { + // AFTER the commit on purpose: the handler writes, and re-entering a + // write inside this `BEGIN IMMEDIATE` would risk rolling back a peer's + // whole batch over one session row. The values already landed, so a + // failure here costs the revision bump, not the peer's decision. + if (remoteSettleTuple.length && remoteSettleTupleHandler) { try { - remoteSettleTupleHandler(heldSettleTuple); - touchedTables.add("terminal_sessions"); - } catch { - // Reconciliation is best-effort: the rest of the batch already landed - // and must not be undone by a single session's settle decision. + remoteSettleTupleHandler(remoteSettleTuple); + } catch (error) { + logger.warn("sync.settle_tuple_reconcile_failed", { + count: remoteSettleTuple.length, + error: error instanceof Error ? error.message : String(error), + }); } + touchedTables.add("terminal_sessions"); } return { diff --git a/apps/desktop/src/shared/subagentCapabilities.ts b/apps/desktop/src/shared/subagentCapabilities.ts index b7c43cfbf..6cbf9f998 100644 --- a/apps/desktop/src/shared/subagentCapabilities.ts +++ b/apps/desktop/src/shared/subagentCapabilities.ts @@ -143,3 +143,14 @@ export function resolveSubagentCapability( } return NO_SUBAGENT_CAPABILITY; } + +/** + * Runtimes that cannot stop an individual piece of background work at all. + * + * A Codex chat has no per-subagent stop, so a settle teardown reports its + * leftover work as `no_stop_control` rather than as a stop that failed — a + * different fact, and the reason the residue reason field exists. Declared + * here with the other per-runtime facts rather than as a `provider === "codex"` + * check inside the teardown, which is what this module exists to prevent. + */ +export const PROVIDERS_WITHOUT_BACKGROUND_STOP_CONTROL: ReadonlySet = new Set(["codex"]); diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index 79406434f..cd7161356 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -580,11 +580,13 @@ it does not take the user's shell away, and ADE cannot re-spawn one it killed. Anything still running when the budget expires is recorded, and the settle still lands. Each item carries a coarse `reason` — `no_stop_control` (a Codex chat has -no per-subagent stop at all), `timeout`, or `rejected` — and a `reapable` flag. -Everything counted is still a child of this ADE process, so it stays eligible -for the ppid-based orphan reaper; work that escaped the tree -(`nohup`/`setsid`/`disown`) is invisible to the confirmation read and is -therefore never folded into that count rather than being overstated as reapable. +no per-subagent stop at all), `timeout`, or `rejected` — and the number of jobs +it covers. Everything counted is work ADE tracks, so it stays eligible for the +ppid-based orphan reaper. Work that escaped the process tree +(`nohup`/`setsid`/`disown`) is invisible to the confirmation read, so it is +never folded into that count and never overstated as recoverable — the design +requires the distinction, and here it holds by construction rather than by a +flag that could only ever read `true`. Residue lives in `session_settle_residue`, a **local-only** table: it describes processes on this host, and a peer showing "1 job could not be stopped" for a @@ -599,36 +601,51 @@ failed job — with `provider`, the coarse `outcome` reason, and a bucketed ### 6d. Peer tuple writes: host authority finished, not consensus added -R7's fix is in the **apply layer**, and it is a hold-back rather than a drop. - -`db.sync.applyChanges` is the single place both the host and peer paths funnel -through, so the check lives there: an inbound change to `settled_at`, -`settle_override` or `settle_source` is held out of the raw apply and handed to -`sessionService.reconcileRemoteSettleTuple`, which replays the remote's intent -**through the chokepoint**. The remote decision therefore gains this host's -revision bump, settling-window exclusion and abort semantics. - -Three details that are load-bearing: - -- **Held, not dropped.** Step 0 drops these columns from *phone* peers, and its - own comment says a paired desktop's settle writes "must keep replicating" - because that desktop runs the same chokepoint. Both are true: the decision is - legitimate, it just has to arrive through the front door. Extending the phone - drop to desktops would discard a real decision. -- **After the commit.** The handler writes, and re-entering a write inside the - batch's `BEGIN IMMEDIATE` would risk rolling back a peer's entire changeset - over one session row. -- **Undecodable key -> plain apply.** Only a single TEXT primary key is decoded. - Anything else is not claimed, so an unfamiliar encoding degrades to today's - behavior instead of a silently dropped change. +R7's fix is in the **apply layer**: `db.sync.applyChanges`, the one place both +the host and peer paths funnel through. An inbound change to `settled_at`, +`settle_override` or `settle_source` **applies normally**, and the session layer +is then told which sessions and columns moved so it can re-assert them through +the chokepoint. + +**It does not hold the change back, and that correction matters.** The first +implementation kept settle-tuple rows out of `crsql_changes` and rebuilt the +remote intent afterwards. A probe against the vendored cr-sqlite build showed +why that is wrong: merges are last-writer-wins on a per-column `col_version`, +and a column that never enters `crsql_changes` never raises the local counter. +The host stays behind the peer permanently, so its **next** genuine decision — a +user unsettle, a keep-active pin, a PR-merge settle — carries a lower version +and is rejected by every peer. Two hosts then disagree forever. That is a +strictly worse failure than the bypass being fixed, and it would not have shown +up in any single-host test. + +So the division is: **CRR owns the values, the chokepoint owns the revision.** +Reconciliation writes the tuple to its own current values — a self-assignment +that matches the row (so the revision bumps) without changing it (so cr-sqlite +records no new column version and nothing echoes back). The revision bump is the +whole point: an in-flight settle re-reads it after its teardown await, sees the +world moved, and abandons instead of overwriting the peer's decision. That is +the R7 mirror case — a peer reactivating a session mid-settle — and it is tested. + +Other details that are load-bearing: + +- **Registered in BOTH processes.** The desktop main process and the ADE brain + each construct a `sessionService`, and in a normal install it is the *brain* + that applies changesets, serves phone sync and remote commands, and runs the + PR-merge poller. Wiring only the desktop would have left teardown a no-op for + almost every settle a user actually triggers. Both now build their hooks from + one `createSettleTeardownWiring` factory so they cannot drift. +- **Per session, best effort.** One unreadable row cannot cost the rest of the + batch its bump, and a failure costs only the revision — never the peer's + decision, which has already landed. +- **Undecodable key → no reconcile.** Only a single TEXT primary key is decoded. + Anything else still applies; it simply is not re-asserted. **No peer-visible concurrency token was built.** That is a protocol change, and the evidence does not justify it yet — which is what `onRemoteSettleWrite` is -for. Post-step-0 it should never fire; if the field says otherwise, we will know -which columns and how often before designing anything. - -R7/R7b are unchanged and still write the row with a raw `db.run` — a bypass no -inbound changeset can produce any more. They are kept because they pin the -property that motivates the interception: a write that reaches the tuple without -the chokepoint is invisible to the guard. Deleting them would delete the -evidence for the fix. The reconciled path is asserted separately. +for. If the field says legitimate peer writers still exist, we will know which +columns and how often before designing anything. + +R7/R7b are unchanged and still write the row with a raw `db.run`. They pin the +property that motivates the whole mechanism: a write that reaches the tuple +without the chokepoint is invisible to the guard. The reconciled path is +asserted separately, against the same shape the apply layer now produces. From 572632103f2505b69d3571a524f084458e96a750 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 05:15:53 -0400 Subject: [PATCH 04/14] fix: second-round review findings across teardown, reconcile and latency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Track A verified two load-bearing claims empirically against the vendored cr-sqlite: the observeRemote self-assignment bumps sqlite3_changes without touching the clock (no echo), and stop_only really does stop background work. The design holds. What it found on top: - session.getSettleResidue was added to CTO_ONLY but not the allowlist, so every call was refused. The read path 3d option 3 was signed off on did not actually exist. - Stale residue survived a clean re-settle: nothing cleared the row when a later teardown confirmed everything, so it kept reporting an old failure with an old timestamp. - A peer write bumped the revision but never tripped the abort. The revision is only re-read AFTER teardown, so teardown ran to completion and interrupted a turn the user had just started on the other device — losing the work AND the settle, which is exactly the R2 shape 3c exists to prevent. - The reconcile handler fired on changes had discarded, so a re-delivered batch abandoned an in-flight settle over a duplicate packet. - A timed-out liveness read was indistinguishable from 'not a chat session', so a slow host settled while claiming a clean teardown — the one outcome residue exists to prevent. - settle_remote_write_reconciled fired on the NORMAL desktop-peer path, one event per session. 'Expected zero' was wrong: a paired desktop replicating its own settles belongs here. Now one batched event per changeset, framed as a rate signal. - Bulk settle was serial, and per session now costs up to 15s. iOS allows 30s for the whole command, so three busy sessions was a guaranteed timeout. Now bounded-concurrent, results reassembled in the caller's order. - Leaked ~50 unref'd timers per settling session; 10Hz polling of an expensive read; lmstudio missing from the provider dimension. Track B: restored the settle methods' locality after my own earlier repair scattered them, put back two invariant comments that repair dropped, moved the duplicated analytics envelope into the shared factory, made residue report a surviving turn separately from surviving jobs, and covered the hand-rolled cr-sqlite pk decoder — the riskiest code in the diff — with a test that drives the real applyChanges path. --- apps/ade-cli/src/bootstrap.ts | 20 +- apps/desktop/src/main/main.ts | 32 +- .../src/main/services/adeActions/registry.ts | 3 +- .../analytics/productAnalyticsPolicy.ts | 2 +- .../main/services/sessions/sessionService.ts | 295 +++++++++++------- .../sessions/sessionSettleTeardown.test.ts | 30 +- .../sessions/sessionSettleTeardown.ts | 76 +++-- .../sessions/settleRaceMatrix.test.ts | 66 ++-- .../services/sessions/settleTeardownWiring.ts | 52 ++- .../sessions/settlingStateRegistry.ts | 7 +- .../src/main/services/state/kvDb.test.ts | 59 ++++ apps/desktop/src/main/services/state/kvDb.ts | 28 +- .../settle-teardown-design.md | 28 +- docs/logging.md | 20 +- 14 files changed, 481 insertions(+), 237 deletions(-) diff --git a/apps/ade-cli/src/bootstrap.ts b/apps/ade-cli/src/bootstrap.ts index b28a6c180..aa9d3e959 100644 --- a/apps/ade-cli/src/bootstrap.ts +++ b/apps/ade-cli/src/bootstrap.ts @@ -767,7 +767,7 @@ export async function createAdeRuntime(args: { // stopping nothing. const settleTeardownRef: { run: ((sessionId: string, ctx: SettleTeardownContext) => Promise) | null; - report: ((args: { columns: string[] }) => void) | null; + report: ((args: { columns: string[]; sessionCount: number }) => void) | null; residue: ((args: { provider: string | null; items: SettleResidueItem[] }) => void) | null; } = { run: null, report: null, residue: null }; const sessionService = createSessionService({ @@ -1281,21 +1281,9 @@ export async function createAdeRuntime(args: { const settleWiring = createSettleTeardownWiring({ agentChatService, logger, - captureAnalytics: ({ action, outcome, provider, countBucket }) => { - productAnalyticsService?.captureInternal({ - event: "ade_feature_used", - // The brain reports as "api": it is the non-GUI runtime surface, and - // it is what the other brain-side analytics here already use. - surface: "api", - properties: { - feature: "work", - action, - outcome, - ...(provider ? { provider } : {}), - ...(countBucket ? { count_bucket: countBucket } : {}), - }, - }); - }, + analytics: productAnalyticsService ?? null, + // The brain is the non-GUI runtime surface, matching its other analytics. + surface: "api", }); settleTeardownRef.run = settleWiring.runSettleTeardown; settleTeardownRef.report = settleWiring.onRemoteSettleWrite; diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index cdc1c89b9..5cf9880dc 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -2874,24 +2874,21 @@ app.whenReady().then(async () => { // Late-bound: the chat service that owns the work does not exist yet at // this point, and the settle path must not depend on construction order. - // Same shape as `laneTeardownDeps` below. const settleTeardownRef: { run: ((sessionId: string, ctx: SettleTeardownContext) => Promise) | null; - } = { run: null }; - const settleRemoteWriteRef: { - report: ((args: { columns: string[] }) => void) | null; + report: ((args: { columns: string[]; sessionCount: number }) => void) | null; residue: ((args: { provider: string | null; items: SettleResidueItem[] }) => void) | null; - } = { report: null, residue: null }; + } = { run: null, report: null, residue: null }; const sessionService = createSessionService({ db, - onRemoteSettleWrite: (args) => settleRemoteWriteRef.report?.(args), - onSettleResidue: (args) => settleRemoteWriteRef.residue?.(args), + onRemoteSettleWrite: (args) => settleTeardownRef.report?.(args), + onSettleResidue: (args) => settleTeardownRef.residue?.(args), runSettleTeardown: async (sessionId, ctx) => settleTeardownRef.run ? await settleTeardownRef.run(sessionId, ctx) // Before the chat service is up there is no background work to stop, // so an empty teardown is the honest answer, not a skipped one. - : { stopped: [], residue: [] }, + : { residue: [] }, }); sessionService.onChanged((event) => { emitProjectEvent(projectRoot, IPC.sessionsChanged, event); @@ -3633,23 +3630,12 @@ app.whenReady().then(async () => { const wiring = createSettleTeardownWiring({ agentChatService, logger, - captureAnalytics: ({ action, outcome, provider, countBucket }) => { - productAnalyticsService?.captureInternal({ - event: "ade_feature_used", - surface: "desktop", - properties: { - feature: "work", - action, - outcome, - ...(provider ? { provider } : {}), - ...(countBucket ? { count_bucket: countBucket } : {}), - }, - }); - }, + analytics: productAnalyticsService ?? null, + surface: "desktop", }); settleTeardownRef.run = wiring.runSettleTeardown; - settleRemoteWriteRef.report = wiring.onRemoteSettleWrite; - settleRemoteWriteRef.residue = wiring.onSettleResidue; + settleTeardownRef.report = wiring.onRemoteSettleWrite; + settleTeardownRef.residue = wiring.onSettleResidue; } autoRebaseActivityReady = true; void autoRebaseService diff --git a/apps/desktop/src/main/services/adeActions/registry.ts b/apps/desktop/src/main/services/adeActions/registry.ts index 04572e5c5..d175075c1 100644 --- a/apps/desktop/src/main/services/adeActions/registry.ts +++ b/apps/desktop/src/main/services/adeActions/registry.ts @@ -747,6 +747,7 @@ export const ADE_ACTION_ALLOWLIST: Partial>> = { // widened, so the scope control can never carry free text. "machine", "project", "account", ]), - provider: new Set(["codex", "openai", "claude", "cursor", "droid", "opencode", "pi", "gemini", "local", "other"]), + provider: new Set(["codex", "openai", "claude", "cursor", "droid", "opencode", "pi", "gemini", "lmstudio", "local", "other"]), model_family: new Set([ "gpt_5", "openai_reasoning", "claude_sonnet", "claude_opus", "claude_haiku", "cursor", "gemini", "grok", "local", "other", diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index 1aea52742..f828b24d6 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -398,7 +398,7 @@ export function createSessionService({ * want to know which writer is still out there before deciding whether a * protocol-level token is justified. */ - onRemoteSettleWrite?: (args: { columns: string[] }) => void; + onRemoteSettleWrite?: (args: { columns: string[]; sessionCount: number }) => void; /** Fired only for residue attached to a settle that actually landed. */ onSettleResidue?: (args: { provider: string | null; items: SettleResidueItem[] }) => void; }) { @@ -761,6 +761,14 @@ export function createSessionService({ * has already landed by this point, and losing a diagnostics row must not * turn a successful settle into a failed one. */ + const clearSettleResidue = (sessionId: string): void => { + try { + db.run("delete from session_settle_residue where session_id = ?", [sessionId]); + } catch { + // Diagnostics only. + } + }; + const recordSettleResidue = (sessionId: string, items: SettleResidueItem[]): void => { try { db.run( @@ -784,6 +792,9 @@ export function createSessionService({ * * Teardown is real from step 3, and it is AWAITED inside the window. */ + /** Bounded so a large sweep cannot open a provider stop per session at once. */ + const SETTLE_TEARDOWN_CONCURRENCY = 4; + const settleManyWithTeardown = async ( sessionIds: string[], options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, @@ -792,22 +803,36 @@ export function createSessionService({ const settled: string[] = []; const aborted: SettleAbortedSession[] = []; - for (const id of ids) { + // Concurrent, not sequential. Each session takes its own exclusive window, + // so two settles never interact — but the confirmation budget is seconds, + // and a bulk settle used to pay it once PER SESSION in series. iOS allows a + // settle command 30s total, so three busy sessions was already a guaranteed + // "the machine took too long to respond" while the settle ran on regardless. + // + // Bounded, so a fifty-session sweep cannot open fifty provider stops at once. + const perSession = new Map(); + const queue = [...ids]; + + const settleOne = async (id: string): Promise => { + const outcome: { settled: string[]; aborted: SettleAbortedSession[] } = { settled: [], aborted: [] }; + perSession.set(id, outcome); + const revisionBefore = settleLifecycle.readRevision(id); const begin = settleLifecycle.settling.begin(id, revisionBefore); // Joined an in-flight settle rather than starting a second teardown: R4. - // The owner will report the outcome; reporting it twice would double-count. + // The owner reports the outcome; reporting it twice would double-count. if (begin.kind === "joined") { - // Report it. A joiner that returns nothing looks identical to a settle - // that was never eligible, and a caller with a durable consequence — the - // PR poller marking a merge handled — would consume the merge on the + // A joiner that returns nothing looks identical to a settle that was + // never eligible, and a caller with a durable consequence — the PR + // poller marking a merge handled — would consume the merge on the // strength of someone else's in-flight settle that may yet abort. - aborted.push({ sessionId: id, reason: "joined_in_flight" }); - continue; + outcome.aborted.push({ sessionId: id, reason: "joined_in_flight" }); + return; } + try { - let teardownThrew = false; let teardown: SettleTeardownOutcome | null = null; + let teardownThrew = false; try { teardown = runSettleTeardown ? await runSettleTeardown(id, { @@ -817,47 +842,70 @@ export function createSessionService({ }) : null; } catch (error) { - // ONLY a teardown throw is `teardown_failed`. Persistence failures - // below (a SQLite lock timeout, an I/O error) must not wear that - // label: a caller cannot tell "the work is still running" from "the - // work stopped but the row did not save", and the PR poller would - // retry a teardown that already succeeded. Those propagate, as they - // did before the settling window existed. + // ONLY a teardown throw is `teardown_failed`. A persistence failure + // below must not wear that label: a caller cannot tell "the work is + // still running" from "the work stopped but the row did not save", + // and the PR poller would retry a stop that already succeeded. Those + // propagate, as they did before the settling window existed. teardownThrew = true; void error; } if (teardownThrew) { - aborted.push({ sessionId: id, reason: "teardown_failed" }); - continue; + outcome.aborted.push({ sessionId: id, reason: "teardown_failed" }); + return; } const abortedBy = settleLifecycle.settling.abortedBy(id); if (abortedBy) { - aborted.push({ sessionId: id, reason: abortedBy }); - continue; + outcome.aborted.push({ sessionId: id, reason: abortedBy }); + return; } // The revision catches everything the abort flag cannot: a settle-tuple // change from a path that never announced itself as a decision. if (settleLifecycle.readRevision(id) !== revisionBefore) { - aborted.push({ sessionId: id, reason: "lifecycle_changed" }); - continue; + outcome.aborted.push({ sessionId: id, reason: "lifecycle_changed" }); + return; } + const changed = settleMany([id], options); - settled.push(...changed); - // Residue is recorded ONLY when the settle actually landed. Attaching it - // to an abandoned settle would leave a "settled, 1 job could not be - // stopped" marker on a row that is not settled. + outcome.settled.push(...changed); + if (changed.length && !teardown?.residue.length) { + // A settle that DID confirm everything must clear the previous + // record, or the row keeps reporting "1 job could not be stopped" + // from a settle two cycles ago, with a stale timestamp. + clearSettleResidue(id); + } if (changed.length && teardown?.residue.length) { recordSettleResidue(id, teardown.residue); // Reported HERE, not inside teardown: an abort arriving between // teardown returning and the guards above means the settle never - // landed, and analytics must not claim residue for a settle that - // does not exist. + // landed, and analytics must not claim residue for one that does not + // exist. onSettleResidue?.({ provider: teardown.provider ?? null, items: teardown.residue }); } } finally { settleLifecycle.settling.end(id, begin.token); } + }; + + await Promise.all( + Array.from({ length: Math.min(SETTLE_TEARDOWN_CONCURRENCY, queue.length) }, async () => { + for (;;) { + const id = queue.shift(); + if (id === undefined) return; + await settleOne(id); + } + }), + ); + + // Reassembled in the caller's order. `settled` is a changed-id list, and a + // caller comparing it against what it asked for should not see it shuffled + // by whichever teardown happened to finish first. + for (const id of ids) { + const outcome = perSession.get(id); + if (!outcome) continue; + settled.push(...outcome.settled); + aborted.push(...outcome.aborted); } return { settled, aborted }; @@ -874,91 +922,33 @@ export function createSessionService({ sessionId: string, opts: { outcome?: string | null; settledAt?: string; source?: SessionSettleSource } = {}, ): Promise<{ found: boolean; settled: boolean; abortedBy?: SettleAbortedReason }> => { - const trimmed = sessionId.trim(); - if (!trimmed) return { found: false, settled: false }; - const exists = db.get<{ present: number }>( - "select 1 as present from terminal_sessions where id = ? limit 1", - [trimmed], - ); - if (!exists) return { found: false, settled: false }; - const note = normalizeSessionStatusNote(opts.outcome); - const result = await settleManyWithTeardown([trimmed], { - ...(note ? { outcome: note } : {}), - settledAt: opts.settledAt, - source: opts.source, - }); - const abortedBy = result.aborted[0]?.reason; - return abortedBy - ? { found: true, settled: false, abortedBy } - : { found: true, settled: true }; + const trimmed = sessionId.trim(); + if (!trimmed) return { found: false, settled: false }; + // `settleMany` returns [] for both "missing" and "already settled", so the + // found/settled split needs its own existence check to stay honest. + const exists = db.get<{ present: number }>( + "select 1 as present from terminal_sessions where id = ? limit 1", + [trimmed], + ); + if (!exists) return { found: false, settled: false }; + // The key must be ABSENT, not `undefined`. `settleMany` decides whether to + // touch `status_note` with hasOwnProperty, so passing `outcome: undefined` + // writes null and erases the note a previous settle left behind. + const note = normalizeSessionStatusNote(opts.outcome); + const result = await settleManyWithTeardown([trimmed], { + ...(note ? { outcome: note } : {}), + settledAt: opts.settledAt, + source: opts.source, + }); + const abortedBy = result.aborted[0]?.reason; + return abortedBy + ? { found: true, settled: false, abortedBy } + : { found: true, settled: true }; }; + return { list, - /** Clears a declared settle plus any `'settled'` override. */ - unsettleSession(sessionId: string): boolean { - const changed = mutateSessionMeta(sessionId, (id) => { - writeSettleLifecycle({ - intent: { kind: "unsettleDeclared" }, - sessionIds: [id], - }); - }); - return changed; - }, - - /** Explicit settle override, cleared with `settled_at` on real activity. */ - setSettleOverride( - sessionId: string, - override: SessionSettleOverride | null, - source: SessionSettleSource = "user", - ): boolean { - const normalized = override == null ? null : normalizeSettleOverride(override); - const normalizedSource = normalizeSettleSource(source) ?? "user"; - return mutateSessionMeta(sessionId, (id) => { - writeSettleLifecycle({ - intent: { kind: "override", value: normalized, source: normalizedSource }, - sessionIds: [id], - }); - }); - }, - - setSettleOverrides(sessionIds: string[], override: SessionSettleOverride | null): string[] { - const ids = normalizeSessionIds(sessionIds); - if (!ids.length) return []; - const normalized = override == null ? null : normalizeSettleOverride(override); - const placeholders = ids.map(() => "?").join(", "); - const present = db.all<{ id: string }>( - `select id from terminal_sessions where id in (${placeholders})`, - ids, - ).map((row) => row.id); - if (!present.length) return []; - const updatePlaceholders = present.map(() => "?").join(", "); - writeSettleLifecycle({ - intent: { kind: "override", value: normalized, source: "user" }, - sessionIds: present, - }); - for (const id of present) { - emitChanged({ sessionId: id, reason: "meta-updated" }); - } - return present; - }, - - /** - * The host-local settle concurrency token for a session. - * - * Read it before a decision that takes time, and require it to be unchanged - * before applying that decision — that is the whole point of the - * chokepoint. 0 means "no settle-lifecycle mutation has been recorded for - * this session", which a caller must treat as a real value, not as absent. - */ - getSettleLifecycleRevision(sessionId: string): number { - return settleLifecycle.readRevision(sessionId); - }, - - async settleSessions(sessionIds: string[]): Promise { - return (await settleManyWithTeardown(sessionIds)).settled; - }, - upsertClaudeSessionPointer(args: { sessionId: string; laneId: string; @@ -1551,6 +1541,70 @@ export function createSessionService({ }); }, + /** Clears a declared settle plus any `'settled'` override. */ + unsettleSession(sessionId: string): boolean { + const changed = mutateSessionMeta(sessionId, (id) => { + writeSettleLifecycle({ + intent: { kind: "unsettleDeclared" }, + sessionIds: [id], + }); + }); + return changed; + }, + + /** Explicit settle override, cleared with `settled_at` on real activity. */ + setSettleOverride( + sessionId: string, + override: SessionSettleOverride | null, + source: SessionSettleSource = "user", + ): boolean { + const normalized = override == null ? null : normalizeSettleOverride(override); + const normalizedSource = normalizeSettleSource(source) ?? "user"; + return mutateSessionMeta(sessionId, (id) => { + writeSettleLifecycle({ + intent: { kind: "override", value: normalized, source: normalizedSource }, + sessionIds: [id], + }); + }); + }, + + setSettleOverrides(sessionIds: string[], override: SessionSettleOverride | null): string[] { + const ids = normalizeSessionIds(sessionIds); + if (!ids.length) return []; + const normalized = override == null ? null : normalizeSettleOverride(override); + const placeholders = ids.map(() => "?").join(", "); + const present = db.all<{ id: string }>( + `select id from terminal_sessions where id in (${placeholders})`, + ids, + ).map((row) => row.id); + if (!present.length) return []; + const updatePlaceholders = present.map(() => "?").join(", "); + writeSettleLifecycle({ + intent: { kind: "override", value: normalized, source: "user" }, + sessionIds: present, + }); + for (const id of present) { + emitChanged({ sessionId: id, reason: "meta-updated" }); + } + return present; + }, + + /** + * The host-local settle concurrency token for a session. + * + * Read it before a decision that takes time, and require it to be unchanged + * before applying that decision — that is the whole point of the + * chokepoint. 0 means "no settle-lifecycle mutation has been recorded for + * this session", which a caller must treat as a real value, not as absent. + */ + getSettleLifecycleRevision(sessionId: string): number { + return settleLifecycle.readRevision(sessionId); + }, + + async settleSessions(sessionIds: string[]): Promise { + return (await settleManyWithTeardown(sessionIds)).settled; + }, + /** * Refresh only the activity timestamp (not the preview text). Lets the PTY * layer record that a session is still producing output even when the @@ -1706,8 +1760,9 @@ export function createSessionService({ * overwriting the peer's decision. * * No peer-visible concurrency token is involved; that is a protocol change - * the evidence does not justify. `onRemoteSettleWrite` is how we find out - * whether any legitimate peer writer is still out there. + * the evidence does not justify. `onRemoteSettleWrite` measures how often + * this path runs — it is NOT an anomaly signal: a paired second desktop + * replicating its own settles is legitimate and lands here by design. */ reconcileRemoteSettleTuple(changes: RemoteSettleTupleChange[]): void { const columnsBySession = new Map>(); @@ -1717,6 +1772,7 @@ export function createSessionService({ columnsBySession.set(change.sessionId, columns); } + const reconciled: string[] = []; for (const [sessionId, columns] of columnsBySession) { // Per session, so one unreadable row cannot discard the rest. The values // already landed; what is at stake here is only the revision bump. @@ -1729,14 +1785,29 @@ export function createSessionService({ ); if (!exists) continue; writeSettleLifecycle({ intent: { kind: "observeRemote" }, sessionIds: [sessionId] }); + // Trip the abort too, not just the revision. The revision is only + // re-read AFTER teardown finishes, so on its own it would let a + // teardown run to completion and interrupt a turn the user has just + // started on another device — losing the work AND the settle, which + // is the R2 shape 3c exists to prevent. + settleLifecycle.settling.abort(sessionId, "remote_lifecycle_changed"); emitChanged({ sessionId, reason: "meta-updated" }); - onRemoteSettleWrite?.({ columns: [...columns].sort() }); + reconciled.push(...columns); } catch (error) { // Best effort per session: the peer's values are already applied, and // one unreadable row must not cost the rest of the batch its bump. void error; } } + // ONE report per changeset, not one per session. A bulk settle on a peer + // arrives as a single apply covering N sessions, and reporting each would + // turn one remote action into an N-event burst. + if (reconciled.length) { + onRemoteSettleWrite?.({ + columns: [...new Set(reconciled)].sort(), + sessionCount: columnsBySession.size, + }); + } }, /** @@ -2019,7 +2090,7 @@ export function createSessionService({ // local table with no reaper, and every other session-keyed side table is // already cascaded here. settleLifecycle.forget(trimmed); - db.run("delete from session_settle_residue where session_id = ?", [trimmed]); + clearSettleResidue(trimmed); emitChanged({ sessionId: trimmed, reason: "deleted" }); return true; }, diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts index 44c240305..8aa212f19 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts @@ -82,6 +82,21 @@ describe("session settle teardown", () => { expect(outcome.provider).toBe("claude"); }); + it("R5: reports a surviving turn separately from surviving background jobs", async () => { + const { run } = harness({ + readActiveWork: async () => work({ active: true, backgroundTaskCount: 2 }), + }); + + const outcome = await run("session-1", neverAborted); + + // Two facts, two items. Folded together, the count would read "3 jobs" and + // the running turn would disappear into the background-task bucket. + expect(outcome.residue).toEqual([ + { kind: "background_tasks", reason: "timeout", count: 2, detail: "2 jobs on claude could not be stopped" }, + { kind: "active_turn", reason: "timeout", count: 1, detail: "the running turn on claude could not be stopped" }, + ]); + }); + it("R5: calls out a provider that has no stop control at all", async () => { const { run } = harness({ // A Codex chat cannot stop an individual subagent. That is a different @@ -160,10 +175,21 @@ describe("session settle teardown", () => { // The first read never resolves. Without a per-call ceiling this awaits // forever inside the settling window, and the row can never be settled // again for the life of the process. - await expect(run("session-1", neverAborted)).resolves.toMatchObject({ residue: [] }); + const outcome = await run("session-1", neverAborted); + expect(readActiveWork).toHaveBeenCalledTimes(1); - // Unknown liveness is not licence to start stopping things. + // Unknown liveness is not licence to start stopping things... expect(interrupt).not.toHaveBeenCalled(); + // ...but it is also not licence to claim a clean teardown. A timed-out read + // is indistinguishable from "not a chat session" unless it says so, and + // silently settling over running work is the one outcome residue exists to + // prevent. + expect(outcome.residue).toEqual([{ + kind: "background_tasks", + reason: "timeout", + count: 1, + detail: "could not read what this session was running, so nothing was stopped", + }]); }); it("buckets residue counts so a large fleet cannot widen the analytics dimension", () => { diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts index 521afdf7a..e2c9688a6 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts @@ -3,7 +3,7 @@ import { PROVIDERS_WITHOUT_BACKGROUND_STOP_CONTROL } from "../../../shared/subagentCapabilities"; /** - * Real settle teardown: stop the work a session owns, then confirm it stopped. + * @file Real settle teardown: stop the work a session owns, then confirm it stopped. * * Shaped after `laneService.stopLaneRuntimeWork` — an ordered list of steps, * each in its own try/catch so one failure cannot abandon the rest — but NOT @@ -79,6 +79,7 @@ export type SettleTeardownOutcome = { */ const STOP_CONFIRM_TIMEOUT_MS = 5_000; const STOP_CONFIRM_POLL_MS = 100; +const STOP_CONFIRM_MAX_POLL_MS = 800; /** * Per-call ceiling for the provider calls themselves. * @@ -133,8 +134,6 @@ export type SessionSettleTeardownDeps = { expireProviderCall?: () => Promise; }; - - export function createSessionSettleTeardown( deps: SessionSettleTeardownDeps, ): (sessionId: string, ctx: SettleTeardownContext) => Promise { @@ -142,18 +141,36 @@ export function createSessionSettleTeardown( const now = deps.now ?? (() => Date.now()); const sleep = deps.sleep ?? ((ms: number) => new Promise((resolve) => { setTimeout(resolve, ms); })); const expireProviderCall = deps.expireProviderCall - ?? (() => new Promise((resolve) => { setTimeout(resolve, PROVIDER_CALL_TIMEOUT_MS); })); + ?? (() => new Promise((resolve) => { setTimeout(resolve, PROVIDER_CALL_TIMEOUT_MS).unref?.(); })); return async (sessionId, ctx): Promise => { const residue: SettleResidueItem[] = []; + let readTimedOut = false; const readWork = async (): Promise => { const result = await withTimeout(deps.readActiveWork(sessionId), expireProviderCall) .catch(() => ({ ok: false }) as const); - return result.ok ? result.value : null; + if (!result.ok) { + readTimedOut = true; + return null; + } + return result.value; }; const before = await readWork(); + // A read that timed out looks exactly like "not a chat session" — null. If + // those were treated the same, a slow host would settle while claiming a + // clean teardown, which is the one outcome residue exists to make + // impossible. Report it instead of guessing. + if (readTimedOut) { + const residue: SettleResidueItem[] = [{ + kind: "background_tasks", + reason: "timeout", + count: 1, + detail: "could not read what this session was running, so nothing was stopped", + }]; + return { residue, provider: null }; + } // Nothing to stop, or a session this service does not own (a plain // terminal). Either way there is no work to lose and no residue to report. if (!before || (!before.active && before.backgroundTaskCount === 0)) { @@ -185,18 +202,31 @@ export function createSessionSettleTeardown( if (ctx.isAborted()) return { residue }; const after = await waitForQuiet(readWork, ctx); - if (after && (after.active || after.backgroundTaskCount > 0) && !ctx.isAborted()) { - const remaining = after.backgroundTaskCount + (after.active ? 1 : 0); - residue.push({ - kind: after.active && after.backgroundTaskCount === 0 ? "active_turn" : "background_tasks", - reason: stopRejected - ? "rejected" - : provider && noStopControl.has(provider) - ? "no_stop_control" - : "timeout", - count: remaining, - detail: describeResidue(remaining, provider), - }); + if (after && !ctx.isAborted()) { + const reason = stopRejected + ? "rejected" as const + : provider && noStopControl.has(provider) + ? "no_stop_control" as const + : "timeout" as const; + // A surviving turn and surviving background tasks are separate facts. + // Folding them into one item lost both the kind and the count — the two + // things the residue exists to report. + if (after.backgroundTaskCount > 0) { + residue.push({ + kind: "background_tasks", + reason, + count: after.backgroundTaskCount, + detail: describeResidue(after.backgroundTaskCount, provider, "jobs"), + }); + } + if (after.active) { + residue.push({ + kind: "active_turn", + reason, + count: 1, + detail: describeResidue(1, provider, "turn"), + }); + } } return { residue, provider }; @@ -212,18 +242,24 @@ export function createSessionSettleTeardown( ctx: SettleTeardownContext, ): Promise { const deadline = now() + STOP_CONFIRM_TIMEOUT_MS; + let delay = STOP_CONFIRM_POLL_MS; let latest = await read(); while (latest && (latest.active || latest.backgroundTaskCount > 0) && now() < deadline) { if (ctx.isAborted()) return latest; - await sleep(STOP_CONFIRM_POLL_MS); + await sleep(delay); + // `getSessionSummary` resolves persisted state, model descriptors and a + // pending-input query; 10 Hz for five seconds is ~50 of those per settle. + delay = Math.min(delay * 2, STOP_CONFIRM_MAX_POLL_MS); latest = await read(); } return latest; } } -function describeResidue(remaining: number, provider: string | null): string { - const what = remaining === 1 ? "1 job" : `${remaining} jobs`; +function describeResidue(count: number, provider: string | null, noun: "jobs" | "turn"): string { + const what = noun === "turn" + ? "the running turn" + : count === 1 ? "1 job" : `${count} jobs`; return provider ? `${what} on ${provider} could not be stopped` : `${what} could not be stopped`; } diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index dd4a748de..2a8112a1a 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -65,7 +65,7 @@ describe("settle race matrix", () => { // visible to teardown WHILE it ran rather than only afterwards. const teardownContexts: SettleTeardownContext[] = []; let residue: SettleResidueItem[] = []; - const remoteWrites: Array<{ columns: string[] }> = []; + const remoteWrites: Array<{ columns: string[]; sessionCount: number }> = []; const service = createSessionService({ db, onRemoteSettleWrite: (args) => { remoteWrites.push(args); }, @@ -234,7 +234,7 @@ describe("settle race matrix", () => { * deliberately left desktop peers replicating, so this is reachable in * production. * - * This test exists to show the BLAST RADIUS before teardown exists, per the + * This test exists to show the BLAST RADIUS before real teardown existed, per the * coordinator's step-3 review scope — it asserts today's real behavior, not * the behavior we want. */ @@ -308,7 +308,7 @@ describe("settle race matrix", () => { ]; expect( accountedFor, - "step 3 must account for an id the writer never saw change", + "an id the writer never saw change must not vanish from both lists", ).toEqual([]); }); @@ -509,19 +509,36 @@ describe("settle race matrix", () => { * chokepoint, so it gains the revision, the settling window and the abort * semantics a local decision has — no peer-visible concurrency token. */ + it("tears sessions down concurrently, and still reports in the caller's order", async () => { + const { service, create, setTeardown } = await fixture(); + for (const id of ["session-2", "session-3"]) create(id); + + let inFlight = 0; + let peakInFlight = 0; + const release: Array<() => void> = []; + setTeardown(async () => { + inFlight += 1; + peakInFlight = Math.max(peakInFlight, inFlight); + await new Promise((resolve) => { release.push(resolve); }); + inFlight -= 1; + }); + + const pending = service.settleSessionsReportingAborts(["session-3", "session-1", "session-2"]); + // Let all three teardowns start before any finishes. + await new Promise((resolve) => { setTimeout(resolve, 0); }); + while (release.length) release.pop()!(); + const outcome = await pending; + + // Serial teardown was the regression: the confirmation budget is seconds, + // and iOS gives the whole command 30s. + expect(peakInFlight, "bulk settle must not tear down one session at a time").toBeGreaterThan(1); + // Order follows the request, not whichever teardown finished first. + expect(outcome.settled).toEqual(["session-3", "session-1", "session-2"]); + }); + describe("remote settle-tuple reconciliation", () => { - /** - * Decision 1 for step 3: finish host authority rather than add consensus. - * - * The VALUES are applied by CRR merge before the session layer is told — - * that is what keeps the per-column clocks convergent, and an earlier - * version that rebuilt the intent instead left this host's clock behind the - * peer forever, so its next genuine decision lost every merge. What - * reconciliation adds is the lifecycle revision, which is what an in-flight - * settle re-reads after its teardown await. - */ + /** Stands in for `applyChanges` having merged the peer's columns. */ const applyPeerWrite = (db: Awaited>["db"], sql: string, params: unknown[]) => { - // Stands in for `applyChanges` having merged the peer's columns. db.run(sql, params as never); }; @@ -543,14 +560,17 @@ describe("settle race matrix", () => { expect(service.get("session-1")?.settledAt).toBe("2026-08-11T00:07:00.000Z"); // And an in-flight settle can now see that the world moved. expect(service.getSettleLifecycleRevision("session-1")).toBeGreaterThan(revisionBefore); - expect(remoteWrites).toEqual([{ columns: ["settle_source", "settled_at"] }]); + expect(remoteWrites).toEqual([{ columns: ["settle_source", "settled_at"], sessionCount: 1 }]); }); it("makes a peer reactivation abort an in-flight settle instead of being overwritten", async () => { - const { db, service, setTeardown } = await fixture(); + const { db, service, setTeardown, teardownContexts } = await fixture(); await service.settleSessionsReportingAborts(["session-1"]); service.unsettleSession("session-1"); + // Captured inside teardown: the window is closed by the time the call + // returns, so `isAborted` is only meaningful while it is still open. + let abortedDuringTeardown: boolean | null = null; setTeardown(() => { // The mirror case Codex raised: a peer reactivates the session while // this host is mid-settle. @@ -562,11 +582,16 @@ describe("settle race matrix", () => { service.reconcileRemoteSettleTuple([ { sessionId: "session-1", column: "settle_override" }, ]); + abortedDuringTeardown = teardownContexts.at(-1)?.isAborted() ?? null; }); const outcome = await service.settleSessionsReportingAborts(["session-1"]); - expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "lifecycle_changed" }]); + expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "remote_lifecycle_changed" }]); expect(outcome.settled).toEqual([]); + // Visible to teardown WHILE it runs, not only after. The revision alone is + // re-read after the await, which would let teardown run to completion and + // interrupt a turn the user just started on the other device. + expect(abortedDuringTeardown, "teardown must see the abort while it is still running").toBe(true); // The peer's reactivation survives. expect(service.get("session-1")?.settleOverride).toBe("active"); expect(service.get("session-1")?.settledAt).toBeNull(); @@ -580,7 +605,7 @@ describe("settle race matrix", () => { expect(remoteWrites).toEqual([]); }); - it("keeps reconciling the rest of the batch when one session fails", async () => { + it("keeps reconciling the rest of the batch when one session is unknown", async () => { const { db, service, create, remoteWrites } = await fixture(); create("session-2"); applyPeerWrite(db, "update terminal_sessions set settled_at = ? where id = ?", ["2026-08-11T00:07:00.000Z", "session-2"]); @@ -590,8 +615,9 @@ describe("settle race matrix", () => { { sessionId: "session-2", column: "settled_at" }, ]); - // The missing row must not cost session-2 its revision bump. - expect(remoteWrites).toEqual([{ columns: ["settled_at"] }]); + // The missing row must not cost session-2 its revision bump — and the + // report is ONE event for the batch, not one per session. + expect(remoteWrites).toEqual([{ columns: ["settled_at"], sessionCount: 2 }]); }); }); }); diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts index 4aca36283..ff75a4c13 100644 --- a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts @@ -1,8 +1,9 @@ import { createSessionSettleTeardown, residueCountBucket } from "./sessionSettleTeardown"; import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } from "./sessionSettleTeardown"; +import type { ProductAnalyticsCapture } from "../../../shared/types/productAnalytics"; /** - * The settle-teardown wiring, shared by the desktop main process and the ADE + * @file The settle-teardown wiring, shared by the desktop main process and the ADE * brain. * * It lives here rather than at each construction site because there are TWO of @@ -26,22 +27,33 @@ export type SettleTeardownChatService = { export type SettleTeardownWiringDeps = { agentChatService: SettleTeardownChatService; - captureAnalytics?: (args: { - action: string; - outcome: string; - provider?: string; - countBucket?: string; - }) => void; - logger?: { warn: (message: string, meta?: Record) => void }; + /** + * Built here rather than at each call site: the two sites were byte-identical + * apart from `surface`, and the property allowlist has already bitten this + * feature once — two copies is two chances to drift out of it. + */ + analytics?: { captureInternal: (event: ProductAnalyticsCapture) => unknown } | null; + surface: "desktop" | "api"; + logger?: { + warn: (message: string, meta?: Record) => void; + info: (message: string, meta?: Record) => void; + }; }; export type SettleTeardownWiring = { runSettleTeardown: (sessionId: string, ctx: SettleTeardownContext) => Promise; - onRemoteSettleWrite: (args: { columns: string[] }) => void; + onRemoteSettleWrite: (args: { columns: string[]; sessionCount: number }) => void; onSettleResidue: (args: { provider: string | null; items: SettleResidueItem[] }) => void; }; export function createSettleTeardownWiring(deps: SettleTeardownWiringDeps): SettleTeardownWiring { + const capture = (properties: Record): void => { + deps.analytics?.captureInternal({ + event: "ade_feature_used", + surface: deps.surface, + properties: { feature: "work", ...properties }, + }); + }; const runSettleTeardown = createSessionSettleTeardown({ interrupt: async (sessionId) => { // `stop_only`, never `stop_and_clear`: the latter also cancels the user's @@ -69,18 +81,24 @@ export function createSettleTeardownWiring(deps: SettleTeardownWiringDeps): Sett // One event per settle that had residue, not one per failed job: a fleet // that fails to stop must not become a burst. Coarse properties only — // no session id, task id, command, or error text. - deps.captureAnalytics?.({ + capture({ action: "settle_teardown_residue", - outcome: items[0]?.reason ?? "failed", - countBucket: residueCountBucket(items.reduce((total, item) => total + item.count, 0)), + outcome: items[0].reason, + count_bucket: residueCountBucket(items.reduce((total, item) => total + item.count, 0)), ...(provider ? { provider } : {}), }); }, - onRemoteSettleWrite: ({ columns }) => { - // Expected to be zero once every writer is host-authoritative. The column - // names are a fixed set; no session id or value is recorded. - deps.logger?.warn("settle.remote_tuple_write_reconciled", { columns }); - deps.captureAnalytics?.({ action: "settle_remote_write_reconciled", outcome: "partial" }); + onRemoteSettleWrite: ({ columns, sessionCount }) => { + // Not a warning and not an anomaly: a paired second desktop replicating + // its own settles reaches this path by design. It is a RATE signal — how + // much settle traffic arrives already-decided — and the column names are + // a fixed set, so no session id or value is recorded. + deps.logger?.info("settle.remote_tuple_write_reconciled", { columns, sessionCount }); + capture({ + action: "settle_remote_write_reconciled", + outcome: "partial", + count_bucket: residueCountBucket(sessionCount), + }); }, }; } diff --git a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts index c63a1c092..fe57e4961 100644 --- a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts +++ b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts @@ -17,7 +17,12 @@ */ /** Why a settle was abandoned. Only ever set by a human-decision clearer. */ -export type SettleAbortReason = "turn_start" | "turn_failed" | "attention_requested"; +export type SettleAbortReason = + | "turn_start" + | "turn_failed" + | "attention_requested" + /** A peer changed the settle tuple; its decision outranks a settle in flight. */ + | "remote_lifecycle_changed"; /** Why a settle was abandoned, as reported to callers. */ export type SettleAbortedReason = diff --git a/apps/desktop/src/main/services/state/kvDb.test.ts b/apps/desktop/src/main/services/state/kvDb.test.ts index c66abc2fe..54d646aa1 100644 --- a/apps/desktop/src/main/services/state/kvDb.test.ts +++ b/apps/desktop/src/main/services/state/kvDb.test.ts @@ -866,3 +866,62 @@ describe("retention maintenance", () => { } }); }); + +describe("inbound settle-tuple reconciliation hook", () => { + /** + * The path with the least margin for error in the settle-teardown work: a + * hand-rolled decode of cr-sqlite's packed primary key. If it is wrong, peer + * settles quietly stop being reconciled — no exception, no log, and the only + * visible symptom is a race that reappears months later. + * + * Driven through the real `applyChanges` rather than by calling the handler, + * so the whole chain is exercised: column detection, pk decode, and the + * guarantee that the change still APPLIES (holding it back would desynchronise + * the per-column clocks). + */ + it("reports settle-tuple columns to the handler and still applies them", async () => { + const projectRoot = makeProjectRoot("ade-kvdb-settle-hook-"); + const source = await openKvDb(path.join(projectRoot, "source", ".ade", "ade.db"), createLogger() as any); + activeDisposers.push(async () => source.close()); + const target = await openKvDb(path.join(projectRoot, "target", ".ade", "ade.db"), createLogger() as any); + activeDisposers.push(async () => target.close()); + + const crrLoaded = source.get<{ present: number }>( + "select 1 as present from sqlite_master where type = 'table' and name = 'terminal_sessions__crsql_clock' limit 1", + ) !== null; + // cr-sqlite ships macOS-only binaries; without it there are no changesets to + // apply and the assertions below would be vacuously true. + if (!crrLoaded) return; + + const insertSession = (db: typeof source) => { + db.run( + `insert into terminal_sessions (id, lane_id, title, status, started_at, tool_type) + values (?, ?, ?, ?, ?, ?)`, + ["peer-session", "lane-1", "Chat", "ended", "2026-08-11T00:00:00.000Z", "codex-chat"], + ); + }; + insertSession(source); + insertSession(target); + + const seen: Array<{ sessionId: string; column: string }> = []; + target.sync.setRemoteSettleTupleHandler((changes) => { seen.push(...changes); }); + + const before = source.sync.getDbVersion(); + source.run("update terminal_sessions set settled_at = ? where id = ?", ["2026-08-11T01:00:00.000Z", "peer-session"]); + const changes = source.sync.exportChangesSince(before) + .filter((change) => change.table === "terminal_sessions"); + expect(changes.length, "the peer must actually have exported a settle change").toBeGreaterThan(0); + + target.sync.applyChanges(changes); + + // The decode worked: the handler learned WHICH session and column moved. + expect(seen).toContainEqual({ sessionId: "peer-session", column: "settled_at" }); + // And the value landed, which is what keeps the per-column clocks convergent. + expect( + target.get<{ settled_at: string | null }>( + "select settled_at from terminal_sessions where id = ?", + ["peer-session"], + )?.settled_at, + ).toBe("2026-08-11T01:00:00.000Z"); + }); +}); diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index 039928db5..f53af1ae8 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -918,14 +918,17 @@ const LOCAL_ONLY_CRR_EXCLUDED_TABLES = new Set([ * settle tuple has to be added to both, and to `settleLifecycleWriter`'s * assignment. Grep `settle_source` before changing any of them. */ -const SETTLE_TUPLE_COLUMNS: ReadonlySet = new Set([ +const SETTLE_TUPLE_COLUMNS: ReadonlySet = new Set([ "settled_at", "settle_override", "settle_source", -]); +] as const); -function isSettleTupleChange(change: CrsqlChangeRow): boolean { - return change.table === "terminal_sessions" && SETTLE_TUPLE_COLUMNS.has(change.cid); +function isSettleTupleChange( + change: CrsqlChangeRow, +): change is CrsqlChangeRow & { cid: RemoteSettleTupleChange["column"] } { + return change.table === "terminal_sessions" + && SETTLE_TUPLE_COLUMNS.has(change.cid as RemoteSettleTupleChange["column"]); } /** @@ -4682,15 +4685,12 @@ export async function openKvDb( // Reachable whenever a table is moved local-only while a paired peer // is still on a build that replicates it — i.e. during every rollout. if (LOCAL_ONLY_CRR_EXCLUDED_TABLES.has(rawChange.table)) continue; + // Decoded before the apply, reported only after it: an undecodable + // key still applies, it is simply not reconciled. + let settleTupleChange: RemoteSettleTupleChange | null = null; if (remoteSettleTupleHandler && isSettleTupleChange(rawChange)) { const sessionId = decodeSingleTextCrsqlPrimaryKey(rawChange.pk); - // An undecodable key still applies; it just is not reconciled. - if (sessionId) { - remoteSettleTuple.push({ - sessionId, - column: rawChange.cid as RemoteSettleTupleChange["column"], - }); - } + if (sessionId) settleTupleChange = { sessionId, column: rawChange.cid }; } const change = normalizeIncomingCrsqlChange(db, rawChange); const result = runStatement( @@ -4711,6 +4711,11 @@ export async function openKvDb( ); appliedCount += result.changes; touchedTables.add(change.table); + // `insert or ignore` silently drops a change whose col_version does + // not beat the local clock, which is exactly what a re-delivered + // batch looks like. Reporting one of those would bump the revision + // and abandon an in-flight settle over a duplicate packet. + if (settleTupleChange && result.changes > 0) remoteSettleTuple.push(settleTupleChange); } if (purgeRetiredTerminalSessions(db) > 0) { touchedTables.add("terminal_sessions"); @@ -4734,7 +4739,6 @@ export async function openKvDb( error: error instanceof Error ? error.message : String(error), }); } - touchedTables.add("terminal_sessions"); } return { diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index cd7161356..5ef603a0f 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -569,9 +569,14 @@ So: `settleManyWithTeardown` is `async`, and `settleSessions`, | Step | Behavior | |---|---| -| Read active work | No turn and no background work -> return immediately. A settle with nothing to tear down must not interrupt the session. | +| Read active work | No turn and no background work -> return immediately. A settle with nothing to tear down must not interrupt the session. A read that TIMES OUT is not the same as "no chat session" and reports residue instead — otherwise a slow host settles while claiming a clean teardown. | | `interrupt` | Stops the active turn and its background work. A throw is `rejected`, not a silent pass. | -| Confirm | Poll `getSessionSummary` until quiet or the 5s budget expires. A single read straight after `interrupt` would call work that was already stopping "residue". | +| Confirm | Poll `getSessionSummary` (backing off to 800ms) until quiet or the 5s budget expires. A single read straight after `interrupt` would call work that was already stopping "residue". Every provider call also has its own 10s ceiling; without it a hung control call holds the settling window open forever and the row can never be settled again. | + +Bulk settles run these **concurrently**, bounded, and reassemble results in the +caller's order. Serially, a bulk settle paid the confirmation budget once per +session — and iOS allows a settle command 30s in total, so three busy sessions +was already a guaranteed timeout while the settle ran on regardless. **Terminals are never touched, at any step.** A settle files a session as done; it does not take the user's shell away, and ADE cannot re-spawn one it killed. @@ -640,10 +645,23 @@ Other details that are load-bearing: - **Undecodable key → no reconcile.** Only a single TEXT primary key is decoded. Anything else still applies; it simply is not re-asserted. +**Reconciliation also trips the abort, not just the revision.** The revision is +only re-read *after* teardown returns, so on its own it would let a teardown run +to completion and stop a turn the user had just started on the other device — +losing the work *and* the settle, which is the R2 shape §3c exists to prevent. +The peer write therefore aborts the window immediately, with its own +`remote_lifecycle_changed` reason. + +**Only changes cr-sqlite actually accepted are reported.** `insert or ignore` +silently drops a change whose `col_version` does not beat the local clock, which +is exactly what a re-delivered batch looks like. Reporting one would bump the +revision and abandon an in-flight settle over a duplicate packet. + **No peer-visible concurrency token was built.** That is a protocol change, and -the evidence does not justify it yet — which is what `onRemoteSettleWrite` is -for. If the field says legitimate peer writers still exist, we will know which -columns and how often before designing anything. +the evidence does not justify it yet. `onRemoteSettleWrite` measures how often +this path runs — and it is *not* an anomaly counter: a paired second desktop +replicating its own settles lands here by design. One event per changeset, never +one per session. R7/R7b are unchanged and still write the row with a raw `db.run`. They pin the property that motivates the whole mechanism: a write that reaches the tuple diff --git a/docs/logging.md b/docs/logging.md index 2ef02771a..773f49d81 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -131,13 +131,19 @@ bucket exists so a large fleet cannot widen the dimension either. No session id, task id, command, or error text is recorded; the human-readable residue detail stays on the local diagnostics row and never enters the payload. -`action: "settle_remote_write_reconciled"` fires when an inbound changeset tried -to write the settle tuple directly and had to be routed back through the -chokepoint. It carries no properties beyond the coarse `outcome`. This is -expected to be **zero** in the field after the host-authoritative change, and it -exists precisely so we learn whether a legitimate peer writer still exists -before anyone designs a protocol-level concurrency token. A non-zero rate is the -signal to escalate, not to widen the event. +`action: "settle_remote_write_reconciled"` fires when an inbound changeset +carried settle-tuple columns and the session layer re-asserted them through the +chokepoint. It carries the coarse `outcome` and a bucketed `count_bucket` of how +many sessions one changeset covered. + +It is a **rate** signal, not an anomaly signal. A paired second desktop +replicating its own settles reaches this path by design, so a non-zero rate is +expected wherever two desktops are paired; what it measures is how much settle +traffic arrives already-decided, which is the evidence needed before anyone +designs a protocol-level concurrency token. **One event per changeset, never one +per session** — a bulk settle on the peer arrives as a single apply covering N +sessions, and reporting each would turn one remote action into an N-event +burst. Applying an update is one transaction — app swap, background service reinstalled, service restarted, service answering — and the brain half failing (the app From 44e2a6262dc260cecce2328dbf2067900dc11b05 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 05:38:11 -0400 Subject: [PATCH 05/14] =?UTF-8?q?fix:=20third-round=20review=20=E2=80=94?= =?UTF-8?q?=20the=20dedup=20guard=20was=20inert,=20and=20residue=20could?= =?UTF-8?q?=20be=20erased?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Track A proved my `result.changes > 0` guard does nothing. `crsql_changes` is a VIRTUAL table, so SQLite counts the xUpdate call whether or not cr-sqlite discarded the row as a losing merge — `insert or ignore` never engages, and a re-applied identical changeset still reports one change. Worse, round 2 had just made reconciliation trip the abort, so a duplicate packet (the peer's outbound cursor only advances on an ok ack, so a dropped ack re-sends the same range) would have killed a user's in-flight settle while carrying no new information. Replaced with a real value comparison: capture the column before the apply, report only if it actually moved. The kvDb test now applies the same changeset twice and asserts nothing is reported the second time — it fails against the old guard, which is how the inertness reproduces. Track A also found clearSettleResidue treating 'could not check' as 'confirmed clean'. An empty residue array is also what you get before the chat service exists and when the confirmation read times out, so a settle that verified nothing was deleting an accurate report of work still running. Teardown now returns an explicit `confirmed` flag and only a confirmed-clean settle may erase. The no-op fallbacks in both processes return confirmed: false. Same class, second site: the read-timeout rule was enforced after the first read but not on the confirmation read, so a hung confirm still returned clean. readWork is now a discriminated result, which makes the compiler force both call sites to decide. Track B: replaced the worker pool with chunking — order is preserved for free, and perSession, the queue and the reassembly loop all disappear; hoisted the concurrency constant to module scope; fixed three docblocks my own insertions had detached from their functions; typed ACTIVITY_ABORTS so a future abort reason cannot be silently mis-bucketed; restored the items[0] guard; renamed sessionCount to changesetSessionCount since it counts the changeset, not the reconciliation; made the concurrency test fail by assertion instead of by vitest timeout; and pinned action reachability, which is what H1 slipped through. Not changed: Track B read the settle methods as still scattered. Verified against main — the method order is byte-identical to base, so the interleaving is the pre-existing layout, not damage. Left alone rather than risk a third structural move in this file. --- apps/ade-cli/src/bootstrap.ts | 4 +- apps/desktop/src/main/main.ts | 4 +- .../main/services/adeActions/registry.test.ts | 5 ++ .../src/main/services/adeActions/registry.ts | 3 +- .../prs/prMergeAutoSettlementService.ts | 13 ++- .../main/services/sessions/sessionService.ts | 79 ++++++++----------- .../sessions/sessionSettleTeardown.test.ts | 20 +++++ .../sessions/sessionSettleTeardown.ts | 64 ++++++++------- .../sessions/settleRaceMatrix.test.ts | 12 ++- .../services/sessions/settleTeardownWiring.ts | 10 +-- .../src/main/services/state/kvDb.test.ts | 13 +++ apps/desktop/src/main/services/state/kvDb.ts | 44 ++++++++++- 12 files changed, 176 insertions(+), 95 deletions(-) diff --git a/apps/ade-cli/src/bootstrap.ts b/apps/ade-cli/src/bootstrap.ts index aa9d3e959..a3b0d8a27 100644 --- a/apps/ade-cli/src/bootstrap.ts +++ b/apps/ade-cli/src/bootstrap.ts @@ -767,13 +767,13 @@ export async function createAdeRuntime(args: { // stopping nothing. const settleTeardownRef: { run: ((sessionId: string, ctx: SettleTeardownContext) => Promise) | null; - report: ((args: { columns: string[]; sessionCount: number }) => void) | null; + report: ((args: { columns: string[]; changesetSessionCount: number }) => void) | null; residue: ((args: { provider: string | null; items: SettleResidueItem[] }) => void) | null; } = { run: null, report: null, residue: null }; const sessionService = createSessionService({ db, runSettleTeardown: async (sessionId, ctx) => - settleTeardownRef.run ? await settleTeardownRef.run(sessionId, ctx) : { residue: [] }, + settleTeardownRef.run ? await settleTeardownRef.run(sessionId, ctx) : { residue: [], confirmed: false }, onRemoteSettleWrite: (args) => settleTeardownRef.report?.(args), onSettleResidue: (args) => settleTeardownRef.residue?.(args), }); diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index 5cf9880dc..df0b85a30 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -2876,7 +2876,7 @@ app.whenReady().then(async () => { // this point, and the settle path must not depend on construction order. const settleTeardownRef: { run: ((sessionId: string, ctx: SettleTeardownContext) => Promise) | null; - report: ((args: { columns: string[]; sessionCount: number }) => void) | null; + report: ((args: { columns: string[]; changesetSessionCount: number }) => void) | null; residue: ((args: { provider: string | null; items: SettleResidueItem[] }) => void) | null; } = { run: null, report: null, residue: null }; const sessionService = createSessionService({ @@ -2888,7 +2888,7 @@ app.whenReady().then(async () => { ? await settleTeardownRef.run(sessionId, ctx) // Before the chat service is up there is no background work to stop, // so an empty teardown is the honest answer, not a skipped one. - : { residue: [] }, + : { residue: [], confirmed: false }, }); sessionService.onChanged((event) => { emitProjectEvent(projectRoot, IPC.sessionsChanged, event); diff --git a/apps/desktop/src/main/services/adeActions/registry.test.ts b/apps/desktop/src/main/services/adeActions/registry.test.ts index 5643bdc7b..11b2b39d2 100644 --- a/apps/desktop/src/main/services/adeActions/registry.test.ts +++ b/apps/desktop/src/main/services/adeActions/registry.test.ts @@ -75,6 +75,11 @@ describe("isAllowedAdeAction", () => { expect(isAllowedAdeAction("session", "requestSessionAttention")).toBe(true); expect(isAllowedAdeAction("session", "setSessionStatusNote")).toBe(true); expect(isAllowedAdeAction("session", "settleSession")).toBe(true); + // The residue read path. It was added to the CTO-only list but NOT to the + // allowlist, which silently refused every call — and left the settle design + // claiming a user-visible guarantee ("settled never quietly means something + // is still running") that nothing could actually reach. + expect(isAllowedAdeAction("session", "getSettleResidue")).toBe(true); expect(isAllowedAdeAction("session", "unsettleSession")).toBe(true); expect(isCtoOnlyAdeAction("session", "settleSession")).toBe(true); expect(isCtoOnlyAdeAction("session", "unsettleSession")).toBe(true); diff --git a/apps/desktop/src/main/services/adeActions/registry.ts b/apps/desktop/src/main/services/adeActions/registry.ts index d175075c1..506518b40 100644 --- a/apps/desktop/src/main/services/adeActions/registry.ts +++ b/apps/desktop/src/main/services/adeActions/registry.ts @@ -258,7 +258,6 @@ export const ADE_ACTION_CTO_ONLY: Partial(["turn_start", "turn_failed", "attention_requested"]); +const ACTIVITY_ABORTS: ReadonlySet = new Set([ + "turn_start", + "turn_failed", + "attention_requested", +]); import type { AgentChatSessionSummary } from "../../../shared/types"; function isMergeAtOrAfter(mergedAt: string | null | undefined, enabledSince: string): boolean { @@ -278,8 +283,10 @@ export function createPrMergeAutoSettlementService(args: { // ONLY an activity abort waits for the turn to end. `teardown_failed` // means the stop itself failed while the work kept running — making it // wait for inactivity would never stop that work again, because the - // work is exactly what it would be waiting on. `lifecycle_changed` and - // `joined_in_flight` are momentary and clear on their own. + // work is exactly what it would be waiting on. `lifecycle_changed`, + // `joined_in_flight` and `remote_lifecycle_changed` are momentary and + // clear on their own; a peer's decision in particular has nothing to + // do with LOCAL inactivity, so waiting on it would be meaningless. if (settleResult.aborted.some((entry) => ACTIVITY_ABORTS.has(entry.reason))) { abortedSessionIds.add(session.id); } diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index f828b24d6..a295e05db 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -93,6 +93,9 @@ type ClaudeSessionRow = { export const STALE_RUNNING_SESSION_FRESH_ACTIVITY_GRACE_MS = 2 * 60 * 1000; +/** Bounded so a large sweep cannot open a provider stop per session at once. */ +const SETTLE_TEARDOWN_CONCURRENCY = 4; + const SESSION_COLUMNS = ` s.id as id, s.lane_id as laneId, @@ -398,7 +401,7 @@ export function createSessionService({ * want to know which writer is still out there before deciding whether a * protocol-level token is justified. */ - onRemoteSettleWrite?: (args: { columns: string[]; sessionCount: number }) => void; + onRemoteSettleWrite?: (args: { columns: string[]; changesetSessionCount: number }) => void; /** Fired only for residue attached to a settle that actually landed. */ onSettleResidue?: (args: { provider: string | null; items: SettleResidueItem[] }) => void; }) { @@ -730,7 +733,6 @@ export function createSessionService({ ids, ).map((row) => row.id); if (!newlySettled.length) return []; - const updatePlaceholders = newlySettled.map(() => "?").join(", "); const hasOutcome = Object.prototype.hasOwnProperty.call(options, "outcome"); writeSettleLifecycle({ intent: { @@ -753,14 +755,6 @@ export function createSessionService({ return newlySettled; }; - /** - * Record what teardown could not confirm it stopped (§3d option 3). - * - * Keyed by session so a re-settle REPLACES the previous record rather than - * accumulating history the user has no way to clear. Never throws: the settle - * has already landed by this point, and losing a diagnostics row must not - * turn a successful settle into a failed one. - */ const clearSettleResidue = (sessionId: string): void => { try { db.run("delete from session_settle_residue where session_id = ?", [sessionId]); @@ -769,6 +763,14 @@ export function createSessionService({ } }; + /** + * Record what teardown could not confirm it stopped (§3d option 3). + * + * Keyed by session so a re-settle REPLACES the previous record rather than + * accumulating history the user has no way to clear. Never throws: the settle + * has already landed by this point, and losing a diagnostics row must not + * turn a successful settle into a failed one. + */ const recordSettleResidue = (sessionId: string, items: SettleResidueItem[]): void => { try { db.run( @@ -792,9 +794,6 @@ export function createSessionService({ * * Teardown is real from step 3, and it is AWAITED inside the window. */ - /** Bounded so a large sweep cannot open a provider stop per session at once. */ - const SETTLE_TEARDOWN_CONCURRENCY = 4; - const settleManyWithTeardown = async ( sessionIds: string[], options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, @@ -810,12 +809,8 @@ export function createSessionService({ // "the machine took too long to respond" while the settle ran on regardless. // // Bounded, so a fifty-session sweep cannot open fifty provider stops at once. - const perSession = new Map(); - const queue = [...ids]; - - const settleOne = async (id: string): Promise => { - const outcome: { settled: string[]; aborted: SettleAbortedSession[] } = { settled: [], aborted: [] }; - perSession.set(id, outcome); + const settleOne = async (id: string): Promise => { + const outcome: SettleSessionsOutcome = { settled: [], aborted: [] }; const revisionBefore = settleLifecycle.readRevision(id); const begin = settleLifecycle.settling.begin(id, revisionBefore); @@ -827,7 +822,7 @@ export function createSessionService({ // poller marking a merge handled — would consume the merge on the // strength of someone else's in-flight settle that may yet abort. outcome.aborted.push({ sessionId: id, reason: "joined_in_flight" }); - return; + return outcome; } try { @@ -852,24 +847,24 @@ export function createSessionService({ } if (teardownThrew) { outcome.aborted.push({ sessionId: id, reason: "teardown_failed" }); - return; + return outcome; } const abortedBy = settleLifecycle.settling.abortedBy(id); if (abortedBy) { outcome.aborted.push({ sessionId: id, reason: abortedBy }); - return; + return outcome; } // The revision catches everything the abort flag cannot: a settle-tuple // change from a path that never announced itself as a decision. if (settleLifecycle.readRevision(id) !== revisionBefore) { outcome.aborted.push({ sessionId: id, reason: "lifecycle_changed" }); - return; + return outcome; } const changed = settleMany([id], options); outcome.settled.push(...changed); - if (changed.length && !teardown?.residue.length) { + if (changed.length && teardown?.confirmed && !teardown.residue.length) { // A settle that DID confirm everything must clear the previous // record, or the row keeps reporting "1 job could not be stopped" // from a settle two cycles ago, with a stale timestamp. @@ -883,29 +878,22 @@ export function createSessionService({ // exist. onSettleResidue?.({ provider: teardown.provider ?? null, items: teardown.residue }); } + return outcome; } finally { settleLifecycle.settling.end(id, begin.token); } }; - await Promise.all( - Array.from({ length: Math.min(SETTLE_TEARDOWN_CONCURRENCY, queue.length) }, async () => { - for (;;) { - const id = queue.shift(); - if (id === undefined) return; - await settleOne(id); - } - }), - ); - - // Reassembled in the caller's order. `settled` is a changed-id list, and a - // caller comparing it against what it asked for should not see it shuffled - // by whichever teardown happened to finish first. - for (const id of ids) { - const outcome = perSession.get(id); - if (!outcome) continue; - settled.push(...outcome.settled); - aborted.push(...outcome.aborted); + // Chunked rather than a worker pool: results come back in request order for + // free, which matters because `settled` is a changed-id list a caller + // compares against what it asked for. Head-of-line blocking inside a chunk + // is not observable — every teardown is already hard-bounded. + for (let start = 0; start < ids.length; start += SETTLE_TEARDOWN_CONCURRENCY) { + const chunk = await Promise.all(ids.slice(start, start + SETTLE_TEARDOWN_CONCURRENCY).map(settleOne)); + for (const outcome of chunk) { + settled.push(...outcome.settled); + aborted.push(...outcome.aborted); + } } return { settled, aborted }; @@ -1578,7 +1566,6 @@ export function createSessionService({ ids, ).map((row) => row.id); if (!present.length) return []; - const updatePlaceholders = present.map(() => "?").join(", "); writeSettleLifecycle({ intent: { kind: "override", value: normalized, source: "user" }, sessionIds: present, @@ -1805,7 +1792,7 @@ export function createSessionService({ if (reconciled.length) { onRemoteSettleWrite?.({ columns: [...new Set(reconciled)].sort(), - sessionCount: columnsBySession.size, + changesetSessionCount: columnsBySession.size, }); } }, @@ -1859,7 +1846,6 @@ export function createSessionService({ unsettleSessions(sessionIds: string[]): void { const ids = normalizeSessionIds(sessionIds); if (!ids.length) return; - const placeholders = ids.map(() => "?").join(", "); writeSettleLifecycle({ intent: { kind: "unsettleDeclared" }, sessionIds: ids, @@ -1915,7 +1901,6 @@ export function createSessionService({ ).map((row) => row.id); if (!present.length) return []; const snoozedAt = normalizeIsoTimestamp(opts.snoozedAt) ?? new Date().toISOString(); - const updatePlaceholders = present.map(() => "?").join(", "); db.run( ` update terminal_sessions @@ -1923,7 +1908,7 @@ export function createSessionService({ snoozed_at = ?, woke_at = null, woke_reason = null - where id in (${updatePlaceholders}) + where id in (${present.map(() => "?").join(", ")}) `, [until, snoozedAt, ...present], ); diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts index 8aa212f19..ca749c7e4 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts @@ -192,6 +192,26 @@ describe("session settle teardown", () => { }]); }); + it("does not claim a clean teardown when the CONFIRMATION read times out", async () => { + let call = 0; + const { run } = harness({ + // The first read succeeds, so teardown proceeds and interrupts. The read + // that is supposed to CONFIRM the stop then hangs — and a timeout is not + // confirmation, however much it looks like one. + readActiveWork: vi.fn(async () => { + call += 1; + if (call === 1) return work({ backgroundTaskCount: 1 }); + return await new Promise(() => {}); + }), + expireProviderCall: async () => {}, + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue, "an unconfirmed stop must never report as clean").not.toEqual([]); + expect(outcome.residue[0]?.reason).toBe("timeout"); + }); + it("buckets residue counts so a large fleet cannot widen the analytics dimension", () => { expect(residueCountBucket(1)).toBe("1"); expect(residueCountBucket(5)).toBe("2_5"); diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts index e2c9688a6..12ca0ef7d 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts @@ -66,6 +66,16 @@ export type SettleTeardownContext = { * the suspension point survivable. */ export type SettleTeardownOutcome = { + /** + * True only when teardown actually ran AND confirmed the session went quiet. + * + * An empty `residue` is not the same claim: it is also what you get before the + * chat service exists, or when the confirmation read never came back. Only a + * confirmed-clean settle may erase a previous residue record — otherwise a + * settle that checked nothing deletes an accurate report of work that is + * still running. + */ + confirmed: boolean; residue: SettleResidueItem[]; /** For the residue analytics dimension. Null when the session has no chat. */ provider?: string | null; @@ -146,41 +156,38 @@ export function createSessionSettleTeardown( return async (sessionId, ctx): Promise => { const residue: SettleResidueItem[] = []; - let readTimedOut = false; - const readWork = async (): Promise => { - const result = await withTimeout(deps.readActiveWork(sessionId), expireProviderCall) + // `{ok:false}` rather than collapsing a timeout to null: a timed-out read + // and "this is not a chat session" are both absences, and treating them the + // same is how a slow host settles while claiming a clean teardown. Making + // it a discriminated result forces every call site to decide. + const readWork = async (): Promise<{ ok: true; value: SessionActiveWork | null } | { ok: false }> => + await withTimeout(deps.readActiveWork(sessionId), expireProviderCall) .catch(() => ({ ok: false }) as const); - if (!result.ok) { - readTimedOut = true; - return null; - } - return result.value; - }; - const before = await readWork(); - // A read that timed out looks exactly like "not a chat session" — null. If - // those were treated the same, a slow host would settle while claiming a - // clean teardown, which is the one outcome residue exists to make - // impossible. Report it instead of guessing. - if (readTimedOut) { - const residue: SettleResidueItem[] = [{ + const timedOutResidue = (): SettleTeardownOutcome => ({ + residue: [{ kind: "background_tasks", reason: "timeout", count: 1, detail: "could not read what this session was running, so nothing was stopped", - }]; - return { residue, provider: null }; - } + }], + provider: null, + confirmed: false, + }); + + const first = await readWork(); + if (!first.ok) return timedOutResidue(); + const before = first.value; // Nothing to stop, or a session this service does not own (a plain // terminal). Either way there is no work to lose and no residue to report. if (!before || (!before.active && before.backgroundTaskCount === 0)) { - return { residue }; + return { residue, confirmed: true }; } const provider = before.provider; // Checked before the step, not after: the point of the abort is to stop // work we have NOT done yet. - if (ctx.isAborted()) return { residue }; + if (ctx.isAborted()) return { residue, confirmed: false }; let stopRejected = false; try { @@ -199,9 +206,12 @@ export function createSessionSettleTeardown( // A turn that arrived while the stop was in flight wins. Do not spend the // confirmation budget re-reading a session the user is actively using. - if (ctx.isAborted()) return { residue }; + if (ctx.isAborted()) return { residue, confirmed: false }; - const after = await waitForQuiet(readWork, ctx); + const confirmed = await waitForQuiet(readWork, ctx); + // Same rule for the confirmation read: a timeout here is not confirmation. + if (!confirmed.ok) return timedOutResidue(); + const after = confirmed.value; if (after && !ctx.isAborted()) { const reason = stopRejected ? "rejected" as const @@ -229,7 +239,7 @@ export function createSessionSettleTeardown( } } - return { residue, provider }; + return { residue, provider, confirmed: true }; }; /** @@ -238,13 +248,13 @@ export function createSessionSettleTeardown( * `interrupt` would report residue for work that was about to stop anyway. */ async function waitForQuiet( - read: () => Promise, + read: () => Promise<{ ok: true; value: SessionActiveWork | null } | { ok: false }>, ctx: SettleTeardownContext, - ): Promise { + ): Promise<{ ok: true; value: SessionActiveWork | null } | { ok: false }> { const deadline = now() + STOP_CONFIRM_TIMEOUT_MS; let delay = STOP_CONFIRM_POLL_MS; let latest = await read(); - while (latest && (latest.active || latest.backgroundTaskCount > 0) && now() < deadline) { + while (latest.ok && latest.value && (latest.value.active || latest.value.backgroundTaskCount > 0) && now() < deadline) { if (ctx.isAborted()) return latest; await sleep(delay); // `getSessionSummary` resolves persisted state, model descriptors and a diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index 2a8112a1a..cc896f536 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -65,14 +65,14 @@ describe("settle race matrix", () => { // visible to teardown WHILE it ran rather than only afterwards. const teardownContexts: SettleTeardownContext[] = []; let residue: SettleResidueItem[] = []; - const remoteWrites: Array<{ columns: string[]; sessionCount: number }> = []; + const remoteWrites: Array<{ columns: string[]; changesetSessionCount: number }> = []; const service = createSessionService({ db, onRemoteSettleWrite: (args) => { remoteWrites.push(args); }, runSettleTeardown: async (sessionId, ctx) => { teardownContexts.push(ctx); await teardown(sessionId); - return { residue }; + return { residue, confirmed: true }; }, }); const setTeardown = (fn: (sessionId: string) => void | Promise) => { @@ -526,6 +526,10 @@ describe("settle race matrix", () => { const pending = service.settleSessionsReportingAborts(["session-3", "session-1", "session-2"]); // Let all three teardowns start before any finishes. await new Promise((resolve) => { setTimeout(resolve, 0); }); + // Asserted BEFORE draining: serial teardown parks two of the three behind + // the first, and the drain below would then hang until vitest's timeout — + // a real failure, but reported as "test timed out" instead of as this. + expect(release.length, "bulk settle must not tear down one session at a time").toBeGreaterThan(1); while (release.length) release.pop()!(); const outcome = await pending; @@ -560,7 +564,7 @@ describe("settle race matrix", () => { expect(service.get("session-1")?.settledAt).toBe("2026-08-11T00:07:00.000Z"); // And an in-flight settle can now see that the world moved. expect(service.getSettleLifecycleRevision("session-1")).toBeGreaterThan(revisionBefore); - expect(remoteWrites).toEqual([{ columns: ["settle_source", "settled_at"], sessionCount: 1 }]); + expect(remoteWrites).toEqual([{ columns: ["settle_source", "settled_at"], changesetSessionCount: 1 }]); }); it("makes a peer reactivation abort an in-flight settle instead of being overwritten", async () => { @@ -617,7 +621,7 @@ describe("settle race matrix", () => { // The missing row must not cost session-2 its revision bump — and the // report is ONE event for the batch, not one per session. - expect(remoteWrites).toEqual([{ columns: ["settled_at"], sessionCount: 2 }]); + expect(remoteWrites).toEqual([{ columns: ["settled_at"], changesetSessionCount: 2 }]); }); }); }); diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts index ff75a4c13..94859093e 100644 --- a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts @@ -42,7 +42,7 @@ export type SettleTeardownWiringDeps = { export type SettleTeardownWiring = { runSettleTeardown: (sessionId: string, ctx: SettleTeardownContext) => Promise; - onRemoteSettleWrite: (args: { columns: string[]; sessionCount: number }) => void; + onRemoteSettleWrite: (args: { columns: string[]; changesetSessionCount: number }) => void; onSettleResidue: (args: { provider: string | null; items: SettleResidueItem[] }) => void; }; @@ -83,21 +83,21 @@ export function createSettleTeardownWiring(deps: SettleTeardownWiringDeps): Sett // no session id, task id, command, or error text. capture({ action: "settle_teardown_residue", - outcome: items[0].reason, + outcome: items[0]?.reason ?? "failed", count_bucket: residueCountBucket(items.reduce((total, item) => total + item.count, 0)), ...(provider ? { provider } : {}), }); }, - onRemoteSettleWrite: ({ columns, sessionCount }) => { + onRemoteSettleWrite: ({ columns, changesetSessionCount }) => { // Not a warning and not an anomaly: a paired second desktop replicating // its own settles reaches this path by design. It is a RATE signal — how // much settle traffic arrives already-decided — and the column names are // a fixed set, so no session id or value is recorded. - deps.logger?.info("settle.remote_tuple_write_reconciled", { columns, sessionCount }); + deps.logger?.info("settle.remote_tuple_write_reconciled", { columns, changesetSessionCount }); capture({ action: "settle_remote_write_reconciled", outcome: "partial", - count_bucket: residueCountBucket(sessionCount), + count_bucket: residueCountBucket(changesetSessionCount), }); }, }; diff --git a/apps/desktop/src/main/services/state/kvDb.test.ts b/apps/desktop/src/main/services/state/kvDb.test.ts index 54d646aa1..4e082411a 100644 --- a/apps/desktop/src/main/services/state/kvDb.test.ts +++ b/apps/desktop/src/main/services/state/kvDb.test.ts @@ -923,5 +923,18 @@ describe("inbound settle-tuple reconciliation hook", () => { ["peer-session"], )?.settled_at, ).toBe("2026-08-11T01:00:00.000Z"); + + // A re-delivered batch must report NOTHING. The peer's outbound cursor only + // advances on an ok ack, so a dropped ack re-sends the identical range — + // and reporting it would bump the lifecycle revision and trip the abort, + // killing a user's in-flight settle over a duplicate packet. + // + // `sqlite3_changes` cannot be used to detect this: `crsql_changes` is a + // virtual table, so SQLite counts the call even when cr-sqlite discards the + // row as a losing merge. That guard was tried and measured inert; this test + // is what caught it. + const seenAfterFirst = seen.length; + target.sync.applyChanges(changes); + expect(seen.length, "a duplicate changeset must not be reported again").toBe(seenAfterFirst); }); }); diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index f53af1ae8..b6176c04d 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -924,6 +924,23 @@ const SETTLE_TUPLE_COLUMNS: ReadonlySet = new "settle_source", ] as const); +const settleTupleKey = (change: RemoteSettleTupleChange): string => + `${change.sessionId}\u0000${change.column}`; + +function readSettleTupleColumn( + db: DatabaseSyncType, + sessionId: string, + column: RemoteSettleTupleChange["column"], +): string | null { + const row = getRow>( + db, + `select ${column} as value from terminal_sessions where id = ?`, + [sessionId], + ); + const value = row?.value; + return typeof value === "string" ? value : null; +} + function isSettleTupleChange( change: CrsqlChangeRow, ): change is CrsqlChangeRow & { cid: RemoteSettleTupleChange["column"] } { @@ -4665,7 +4682,8 @@ export async function openKvDb( // the bypass being fixed. Let CRR converge the values; the chokepoint's // job here is the lifecycle revision, which is what makes an in-flight // settle notice and abort. - const remoteSettleTuple: RemoteSettleTupleChange[] = []; + const candidateSettleTuple: RemoteSettleTupleChange[] = []; + const settleTupleBefore = new Map(); runStatement(db, "BEGIN IMMEDIATE"); try { for (const rawChange of changes) { @@ -4690,7 +4708,13 @@ export async function openKvDb( let settleTupleChange: RemoteSettleTupleChange | null = null; if (remoteSettleTupleHandler && isSettleTupleChange(rawChange)) { const sessionId = decodeSingleTextCrsqlPrimaryKey(rawChange.pk); - if (sessionId) settleTupleChange = { sessionId, column: rawChange.cid }; + if (sessionId) { + settleTupleChange = { sessionId, column: rawChange.cid }; + const key = settleTupleKey(settleTupleChange); + if (!settleTupleBefore.has(key)) { + settleTupleBefore.set(key, readSettleTupleColumn(db, sessionId, rawChange.cid)); + } + } } const change = normalizeIncomingCrsqlChange(db, rawChange); const result = runStatement( @@ -4715,7 +4739,7 @@ export async function openKvDb( // not beat the local clock, which is exactly what a re-delivered // batch looks like. Reporting one of those would bump the revision // and abandon an in-flight settle over a duplicate packet. - if (settleTupleChange && result.changes > 0) remoteSettleTuple.push(settleTupleChange); + if (settleTupleChange) candidateSettleTuple.push(settleTupleChange); } if (purgeRetiredTerminalSessions(db) > 0) { touchedTables.add("terminal_sessions"); @@ -4730,6 +4754,20 @@ export async function openKvDb( // write inside this `BEGIN IMMEDIATE` would risk rolling back a peer's // whole batch over one session row. The values already landed, so a // failure here costs the revision bump, not the peer's decision. + // Only report a column whose VALUE actually moved. + // + // `sqlite3_changes` cannot answer this: `crsql_changes` is a virtual + // table, so SQLite counts the xUpdate call whether or not cr-sqlite + // discarded the row as a losing merge — `insert or ignore` never engages. + // Measured: re-applying an identical changeset still reports 1 change. + // Without a value comparison a re-delivered batch (the peer's outbound + // cursor only advances on an ok ack, so a dropped ack re-sends the same + // range) would bump the revision AND trip the abort, killing a user's + // in-flight settle over a duplicate packet carrying nothing new. + const remoteSettleTuple = candidateSettleTuple.filter((candidate) => { + const after = readSettleTupleColumn(db, candidate.sessionId, candidate.column); + return after !== (settleTupleBefore.get(settleTupleKey(candidate)) ?? null); + }); if (remoteSettleTuple.length && remoteSettleTupleHandler) { try { remoteSettleTupleHandler(remoteSettleTuple); From 125f7991605b07c6fc595d284e18e7f7adef5541 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 05:47:18 -0400 Subject: [PATCH 06/14] docs: correct settle claims that step 3 made false The terminals README still said settle 'deliberately does NOT stop the session's background work' and that a peer's CRR write is outside the revision's scope. Both were true when written and are not now. Replaced with what actually happens, including why the ordering of steps 0-3 was the thing that made it work. The sync docs were accurate about the phone-only column filter but silent on the desktop-peer path, which is the one a reader would now come looking for. --- docs/features/sync-and-multi-device/README.md | 7 +++- .../sync-and-multi-device/crdt-model.md | 8 +++- .../features/terminals-and-sessions/README.md | 40 +++++++++++-------- 3 files changed, 35 insertions(+), 20 deletions(-) diff --git a/docs/features/sync-and-multi-device/README.md b/docs/features/sync-and-multi-device/README.md index cfa225621..410733e4d 100644 --- a/docs/features/sync-and-multi-device/README.md +++ b/docs/features/sync-and-multi-device/README.md @@ -400,7 +400,12 @@ only**. Two properties make it different from the table-level - **It is peer-scoped, and deliberately so.** A paired desktop runs the same `sessionService` chokepoint, so its settle writes are host-decided too and must keep replicating; broadening the filter would silently stop settle - propagating between two of one user's machines. + propagating between two of one user's machines. Those writes are not applied + blind, though: `applyChanges` reports settle-tuple columns whose value + actually moved, and the receiving host re-asserts them through its own + chokepoint so they gain its lifecycle revision and abort an in-flight settle + rather than overwriting it. See + [settle-teardown-design.md](../terminals-and-sessions/settle-teardown-design.md) §6d. - **A paired phone cannot opt out of it.** `isMobilePeer` resolves a record-backed peer through its **pairing record** — host-side truth — and falls back to the peer's own `hello` metadata only when the auth kind is not diff --git a/docs/features/sync-and-multi-device/crdt-model.md b/docs/features/sync-and-multi-device/crdt-model.md index 4b8a96872..87d51832c 100644 --- a/docs/features/sync-and-multi-device/crdt-model.md +++ b/docs/features/sync-and-multi-device/crdt-model.md @@ -259,8 +259,12 @@ standard SQL on the host side, so iOS stays in parity. validation there closes it. Such columns are declared host-authoritative and filtered out of the offending peer's inbound changesets; `terminal_sessions.settled_at` / `settle_override` / `settle_source` are - the current set. See - [Host-authoritative columns](./README.md#host-authoritative-columns-are-peer-scoped). + the current set. A peer that legitimately runs the same chokepoint still + replicates them; the receiving host re-asserts the merged value through its + own chokepoint so the write cannot bypass its lifecycle revision. See + [Host-authoritative columns](./README.md#host-authoritative-columns-are-peer-scoped) + and + [settle-teardown-design.md](../terminals-and-sessions/settle-teardown-design.md) §6d. ## Schema implications diff --git a/docs/features/terminals-and-sessions/README.md b/docs/features/terminals-and-sessions/README.md index 9c6daf03d..0c1babb2c 100644 --- a/docs/features/terminals-and-sessions/README.md +++ b/docs/features/terminals-and-sessions/README.md @@ -217,25 +217,31 @@ and in tests. 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. + by THIS host; a peer's replicated settle-tuple write reaches it because the + changeset apply layer re-asserts the merged value through the chokepoint, so + an in-flight settle sees the world move and abandons — see + [settle-teardown-design.md](settle-teardown-design.md) §3a and §6d. - `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 - the session's background work. That was attempted and removed: teardown is - async, and `settled_at` is written and cleared from seven places, so a - teardown-then-write settle races real activity (a user starting a turn during - a provider stop call gets their background work stopped AND no settle), and - every guard tried against it either read a column that turn-start never - updates or had to be repeated at each of the settle entry points. Making - settle stop work needs a synchronous lifecycle revision that teardown can be - serialized against; it is not a wrapper around the existing write. The approved - plan for doing it is - [settle-teardown-design.md](settle-teardown-design.md); its step 0 - precondition — `settled_at` becoming host-authoritative, so a phone replica - cannot defeat the coming revision guard by CRDT merge — has landed. Archive is - the one lifecycle path that does stop processes — see + registry. Settle now DOES stop the session's outstanding work, through + `sessionSettleTeardown.ts`: it interrupts the active turn and its background + work, confirms the session went quiet, and records what it could not confirm. + **Terminals are never touched** — a settle files a session as done, it does + not take the user's shell away. + + Getting there needed the whole of + [settle-teardown-design.md](settle-teardown-design.md), because the obvious + version was built and cut in #1059 after producing a P1 in each of six review + rounds. Teardown is async and the settle tuple is written and cleared from ten + places, so a teardown-then-write settle races real activity, and every guard + tried against it either read a column that turn-start never updates or had to + be repeated at each entry point. What made it work was doing it in order: the + tuple became host-authoritative (§3c-i), then one chokepoint owned every + mutation and moved a revision (§3a), then a settling window made teardown + visible, exclusive and abortable (§3b) — and only then could teardown be + awaited inside that window (§6). An unconfirmed stop still settles, with + recorded residue rather than silence (§3d option 3). Archive remains a + different, heavier path: it disposes sessions outright — see `laneService.archive`, where the ordering is load-bearing. `dismissPendingInput: true` first quiets an SDK chat through `agentChatService`, or clears a tracked From aaf1e75bcda1e3955296b3136d2f36618b322d40 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 05:48:40 -0400 Subject: [PATCH 07/14] docs: record the sync bulk-settle shape as an open wire decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3c's table says the sync entry point should carry the typed outcome additively; it still answers with a bare changed-id array, so an aborted id is indistinguishable from an ineligible one. Not a regression and not silently wrong — iOS's local overlay expires on its own rather than showing a settled row — but step 3 makes aborts likelier, and the fix is a wire-compatibility call that needs the mobile side, so it is written down rather than guessed at. --- .../settle-teardown-design.md | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index 5ef603a0f..75fd1a876 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -604,6 +604,31 @@ Analytics: one `ade_feature_used` per settle that had residue — never one per failed job — with `provider`, the coarse `outcome` reason, and a bucketed `count_bucket`. No session ids, task ids, commands, or error text. +### 6c-i. Open: the sync bulk settle still answers with a bare id list + +`§3c`'s contract table says the sync entry point should carry the typed outcome +"additively". It does not yet. `session.settleSessions` +(`syncRemoteCommandService.ts`) returns `sessionService.settleSessions(...)` — a +changed-id array — so an aborted id is simply absent, indistinguishable from one +that was never eligible. iOS reads it as `resultShape: .changedIdList`. + +This is not a regression and not silently wrong: iOS shows an in-flight settle +through a local overlay that expires on its own (`PendingSessionSettleStates`, +20s), so an aborted settle reads as "the overlay timed out" rather than as a +settled row. Step 3 makes aborts more likely, though, which makes the gap worth +closing. + +It is left open deliberately because the fix is a **wire-compatibility decision +that needs the mobile side**, and either option costs something: + +- **Change the shape** to `{settled, aborted}` — breaks older iOS builds, which + parse an array. +- **Add `session.settleSessionsWithOutcome`** alongside it and register it in + the mobile compatibility list — genuinely additive, but ships a wire surface + with no consumer until iOS adopts it. + +Neither belongs in a desktop/CLI branch on its own. + ### 6d. Peer tuple writes: host authority finished, not consensus added R7's fix is in the **apply layer**: `db.sync.applyChanges`, the one place both From 410e9904987df53d7e78c3e1d9232eca301ca98f Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 06:01:12 -0400 Subject: [PATCH 08/14] =?UTF-8?q?fix:=20fourth-round=20findings=20?= =?UTF-8?q?=E2=80=94=20restore=20the=20worker=20pool,=20and=20stop=20over-?= =?UTF-8?q?claiming=20confirmation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Track A verified the value-based dedup guard against the real cr-sqlite across nine scenarios, including the one that actually matters: an exact duplicate arriving in a LATER applyChanges call is not reported, while a genuinely new change still is. It also confirmed the snapshot placement, the absence of SQL injection (the column is narrowed by the type guard before interpolation), and that a numeric val is safe under TEXT affinity. Four fixes from that pass: - Reverted chunking back to the worker pool. I took that simplification last round and it cost real throughput: a chunk barrier idles the other workers until its slowest member finishes, and 'every teardown is bounded' is not 'every teardown takes the same time'. Measured at roughly 65s versus 20s for a 50-session sweep with a quarter of the rows unstoppable — aimed straight at the 30s iOS budget the concurrency exists to protect. The perSession map already gave request order, so the simplification bought nothing. - An abort during the confirmation loop returned confirmed: true. Nothing was confirmed and the work was still running, which is precisely the shape the flag was added to make impossible — one refactor away from erasing an accurate residue record. - A confirmation-read timeout discarded a provider it had already read, losing the analytics dimension for the residue most worth attributing. - The value guard read blobs as null, so a blob that changed looked unchanged. Out-of-contract for any real writer, but the guard it replaced did report it. Both new tests were probed and both initially failed to be meaningful: the provider test was hitting a microtask race where the immediate expire won the FIRST read, and the confirmation test tripped the abort before the loop it was meant to exercise, so it passed against the bug. Fixed both, then re-probed — they now fail against the pre-fix code. --- .../main/services/sessions/sessionService.ts | 37 +++++++++---- .../sessions/sessionSettleTeardown.test.ts | 53 ++++++++++++++++++- .../sessions/sessionSettleTeardown.ts | 8 +-- apps/desktop/src/main/services/state/kvDb.ts | 6 ++- 4 files changed, 88 insertions(+), 16 deletions(-) diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index a295e05db..730d214ee 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -884,16 +884,33 @@ export function createSessionService({ } }; - // Chunked rather than a worker pool: results come back in request order for - // free, which matters because `settled` is a changed-id list a caller - // compares against what it asked for. Head-of-line blocking inside a chunk - // is not observable — every teardown is already hard-bounded. - for (let start = 0; start < ids.length; start += SETTLE_TEARDOWN_CONCURRENCY) { - const chunk = await Promise.all(ids.slice(start, start + SETTLE_TEARDOWN_CONCURRENCY).map(settleOne)); - for (const outcome of chunk) { - settled.push(...outcome.settled); - aborted.push(...outcome.aborted); - } + // A worker pool, not chunks. Chunking was tried and reverted: a chunk + // barrier idles the other workers until its slowest member finishes, and + // "every teardown is bounded" is not the same as "every teardown takes the + // same time". For a 50-session sweep where a quarter of the rows hold + // unstoppable work, chunking costs roughly 65s against the pool's ~20 — + // aimed straight at the 30s iOS command budget this exists to protect. + const perSession = new Map(); + const queue = [...ids]; + await Promise.all( + Array.from({ length: Math.min(SETTLE_TEARDOWN_CONCURRENCY, queue.length) }, async () => { + for (;;) { + const id = queue.shift(); + if (id === undefined) return; + perSession.set(id, await settleOne(id)); + } + }), + ); + + // Reassembled in request order. `settled` is a changed-id list that callers + // compare against what they asked for, so it must not come back shuffled by + // whichever teardown happened to finish first. `normalizeSessionIds` + // dedupes, so every id has exactly one entry. + for (const id of ids) { + const outcome = perSession.get(id); + if (!outcome) continue; + settled.push(...outcome.settled); + aborted.push(...outcome.aborted); } return { settled, aborted }; diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts index ca749c7e4..38bf4724d 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts @@ -194,6 +194,7 @@ describe("session settle teardown", () => { it("does not claim a clean teardown when the CONFIRMATION read times out", async () => { let call = 0; + let armed = false; const { run } = harness({ // The first read succeeds, so teardown proceeds and interrupts. The read // that is supposed to CONFIRM the stop then hangs — and a timeout is not @@ -201,9 +202,10 @@ describe("session settle teardown", () => { readActiveWork: vi.fn(async () => { call += 1; if (call === 1) return work({ backgroundTaskCount: 1 }); + armed = true; return await new Promise(() => {}); }), - expireProviderCall: async () => {}, + expireProviderCall: () => armed ? Promise.resolve() : new Promise(() => {}), }); const outcome = await run("session-1", neverAborted); @@ -212,6 +214,55 @@ describe("session settle teardown", () => { expect(outcome.residue[0]?.reason).toBe("timeout"); }); + it("never claims confirmation for a settle that was aborted mid-confirmation", async () => { + let aborted = false; + let reads = 0; + const { run } = harness({ + // The abort must trip INSIDE the confirmation loop, not before it. Tripped + // earlier, an already-correct early return handles it and this test would + // pass against the very bug it is written for. + readActiveWork: async () => { + reads += 1; + if (reads >= 2) aborted = true; + return work({ backgroundTaskCount: 1 }); + }, + }); + + const outcome = await run("session-1", { isAborted: () => aborted }); + + expect(reads, "the confirmation loop must actually have run").toBeGreaterThan(1); + + // `confirmed` gates whether a previous residue record may be ERASED, so a + // teardown that confirmed nothing must never report true — this is the one + // shape the flag exists to make impossible. + expect(outcome.confirmed).toBe(false); + }); + + it("keeps the provider on a confirmation-read timeout", async () => { + let call = 0; + // Armed only AFTER the first read lands. An always-immediate expire races + // the first read on the microtask queue and can time it out instead, which + // would pass this test for the wrong reason. + let armed = false; + const { run } = harness({ + readActiveWork: vi.fn(async () => { + call += 1; + if (call === 1) return work({ backgroundTaskCount: 1, provider: "codex" }); + armed = true; + return await new Promise(() => {}); + }), + expireProviderCall: () => armed + ? Promise.resolve() + : new Promise(() => {}), + }); + + const outcome = await run("session-1", neverAborted); + + // The provider was already read; dropping it loses the analytics dimension + // for exactly the residue worth attributing. + expect(outcome.provider).toBe("codex"); + }); + it("buckets residue counts so a large fleet cannot widen the analytics dimension", () => { expect(residueCountBucket(1)).toBe("1"); expect(residueCountBucket(5)).toBe("2_5"); diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts index 12ca0ef7d..18de93136 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts @@ -164,14 +164,14 @@ export function createSessionSettleTeardown( await withTimeout(deps.readActiveWork(sessionId), expireProviderCall) .catch(() => ({ ok: false }) as const); - const timedOutResidue = (): SettleTeardownOutcome => ({ + const timedOutResidue = (provider: string | null = null): SettleTeardownOutcome => ({ residue: [{ kind: "background_tasks", reason: "timeout", count: 1, detail: "could not read what this session was running, so nothing was stopped", }], - provider: null, + provider, confirmed: false, }); @@ -210,7 +210,7 @@ export function createSessionSettleTeardown( const confirmed = await waitForQuiet(readWork, ctx); // Same rule for the confirmation read: a timeout here is not confirmation. - if (!confirmed.ok) return timedOutResidue(); + if (!confirmed.ok) return timedOutResidue(provider); const after = confirmed.value; if (after && !ctx.isAborted()) { const reason = stopRejected @@ -239,7 +239,7 @@ export function createSessionSettleTeardown( } } - return { residue, provider, confirmed: true }; + return { residue, provider, confirmed: !ctx.isAborted() }; }; /** diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index b6176c04d..04e681d64 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -938,7 +938,11 @@ function readSettleTupleColumn( [sessionId], ); const value = row?.value; - return typeof value === "string" ? value : null; + if (value == null) return null; + // Stringified rather than narrowed to `string`: TEXT affinity converts a + // numeric `val`, but not a blob, and a blob that CHANGED must not read as + // unchanged just because it is not a string. + return value instanceof Uint8Array ? Buffer.from(value).toString("base64") : String(value); } function isSettleTupleChange( From d499220c864d4f3c173e29f5b40a7e8bb224fc00 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 06:24:35 -0400 Subject: [PATCH 09/14] fix: stop the settle pool when a persistence failure propagates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit, and it is a failure mode this branch introduced: before step 3, settleMany was one statement for the whole batch, so there was no partial state to lose. Now each session settles individually, and a SQLite lock thrown from one of them rejected Promise.all while the other workers kept shifting the queue — settling sessions the caller had already given up on. The queue is drained on the first failure so no NEW work starts, the sessions already in flight finish rather than being abandoned half-written, and the error is rethrown only once every worker has stopped. Persistence failures still propagate rather than being dressed up as a settle outcome — that distinction was an earlier review finding and it stands. What did settle is durable, and settle is idempotent, so the caller's retry re-reports it instead of double-filing. --- .../main/services/sessions/sessionService.ts | 19 ++++++++++- .../sessions/settleRaceMatrix.test.ts | 34 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index 730d214ee..be9eac6dc 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -892,15 +892,32 @@ export function createSessionService({ // aimed straight at the 30s iOS command budget this exists to protect. const perSession = new Map(); const queue = [...ids]; + // A persistence failure propagates (a SQLite lock is not a settle outcome + // and must not be dressed up as one), but it must not leave the other + // workers settling sessions the caller has already given up on. Draining + // the queue stops new work; the sessions already in flight finish, so + // nothing is abandoned half-written. + let failure: unknown = null; await Promise.all( Array.from({ length: Math.min(SETTLE_TEARDOWN_CONCURRENCY, queue.length) }, async () => { for (;;) { const id = queue.shift(); if (id === undefined) return; - perSession.set(id, await settleOne(id)); + try { + perSession.set(id, await settleOne(id)); + } catch (error) { + failure ??= error; + queue.length = 0; + return; + } } }), ); + // Rethrown only after every worker has stopped, so the throw cannot race + // more writes. Whatever did settle is already durable, and settle is + // idempotent (`coalesce(settled_at, ?)`), so the caller's retry re-reports + // it rather than double-filing it. + if (failure !== null) throw failure; // Reassembled in request order. `settled` is a changed-id list that callers // compare against what they asked for, so it must not come back shuffled by diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index cc896f536..39e955aab 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -385,6 +385,40 @@ describe("settle race matrix", () => { // session permanently unsettleable. expect(service.settlingSessionIds()).toEqual([]); }); + it("stops the queue when a persistence failure propagates", async () => { + const { service, db, create, setTeardown } = await fixture(); + const ids = ["session-1"]; + for (let n = 2; n <= 12; n += 1) { + create(`session-${n}`); + ids.push(`session-${n}`); + } + + // Fail the write for one session only. The pool must stop taking new work + // rather than settling rows the caller has already given up on. + const realRunChanged = db.runChanged.bind(db); + let tornDown = 0; + setTeardown(() => { tornDown += 1; }); + db.runChanged = ((sql: string, params?: unknown[]) => { + if (typeof sql === "string" && /update\s+terminal_sessions/i.test(sql) + && Array.isArray(params) && params.includes("session-2")) { + throw new Error("database is locked"); + } + return realRunChanged(sql, params as never); + }) as typeof db.runChanged; + + await expect(service.settleSessionsReportingAborts(ids)).rejects.toThrow(/locked/); + db.runChanged = realRunChanged; + + // Far short of all twelve: the queue was drained, so the pool stopped + // taking new sessions instead of running on past the caller. The few + // already in flight when it failed still finish, which is the point — + // nothing is abandoned half-written. + expect(tornDown, "the pool must stop taking new sessions after a failure").toBeLessThan(ids.length); + // And every window closed, or those rows would be unsettleable for the + // life of the process. + expect(service.settlingSessionIds()).toEqual([]); + }); + it("keeps a persistence failure distinct from a failed teardown", async () => { const { service, db, setTeardown } = await fixture(); let stopped = 0; From 86c4c5c625518dc19ab3b7ef9d5573e20b1ab01e Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 06:45:14 -0400 Subject: [PATCH 10/14] fix: honor stop_only everywhere, and see background jobs that outlived a restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two from Codex, both real, both undermining the point of the feature. stop_only was only honored on the Claude path. The OpenCode, Cursor, Pi and Droid branches of interrupt call cancelQueuedSteers unconditionally and return before the mode is ever consulted — so a settle on those providers silently deleted the user's queued prompts, which is exactly the unrecoverable loss the mode was added to prevent. Gated every one of them. The default is stop_and_clear, so the Stop button is untouched; only teardown asks for stop_only. activeBackgroundTaskCount is derived from the LIVE managed runtime, so a Claude --bg job that survives a brain or app restart reads as zero. Teardown saw a quiet session, skipped interrupt entirely, and filed the row as settled while the daemon job kept running — the precise bug this whole feature exists to fix, reintroduced through a liveness read. The summary already resolved the persisted job for other consumers; it is now on the type and counted as work. Both pinned by tests probed against the pre-fix code. Not fixed: CodeRabbit re-posted the queue-drain comment against the previous head; the drain landed in d499220c8. Its second half — surfacing the partial outcome instead of throwing — is deliberate: a SQLite lock is not a settle outcome, and settle is idempotent, so the caller's retry re-reports what landed. --- .../main/services/chat/agentChatService.ts | 26 ++++++-- .../sessions/settleTeardownWiring.test.ts | 64 +++++++++++++++++++ .../services/sessions/settleTeardownWiring.ts | 10 ++- apps/desktop/src/shared/types/chat.ts | 10 +++ 4 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts diff --git a/apps/desktop/src/main/services/chat/agentChatService.ts b/apps/desktop/src/main/services/chat/agentChatService.ts index cb0f52efc..27723f4f3 100644 --- a/apps/desktop/src/main/services/chat/agentChatService.ts +++ b/apps/desktop/src/main/services/chat/agentChatService.ts @@ -37495,7 +37495,13 @@ export function createAgentChatService(args: { } catch { // Ignore provider abort failures; SSE cancellation still tears the turn down. } - cancelQueuedSteers(managed, managed.runtime, "interrupted"); + // `stop_only` exists so settle teardown can stop a turn WITHOUT + // discarding the user's queued follow-ups. Only the Claude path honoured + // it, so a settle on these providers silently deleted queued prompts — + // unrecoverable, and the opposite of the rule that losing a settle costs + // one click while losing the user's work does not. Default is + // `stop_and_clear`, so the Stop button is unaffected. + if (mode === "stop_and_clear") cancelQueuedSteers(managed, managed.runtime, "interrupted"); persistChatState(managed); for (const pending of managed.runtime.pendingApprovals.values()) { managed.runtime.handle.client.postSessionIdPermissionsPermissionId({ @@ -37541,7 +37547,7 @@ export function createAgentChatService(args: { cancelCursorPermissionWaiter(w, "Cursor tool approval was cancelled because the turn was interrupted."); } rt.permissionWaiters.clear(); - cancelQueuedSteers(managed, rt, "interrupted"); + if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted"); return result; } @@ -37554,7 +37560,7 @@ export function createAgentChatService(args: { } catch { // ignore } - cancelQueuedSteers(managed, rt, "interrupted"); + if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted"); cancelPendingPiInputs(managed); persistChatState(managed); return result; @@ -37562,7 +37568,9 @@ export function createAgentChatService(args: { if (managed.session.provider === "pi") { piRuntimeSetupInterruptRequested.set(managed, true); - cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + if (mode === "stop_and_clear") { + cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + } setSessionIdle(managed); persistChatState(managed); return result; @@ -37580,20 +37588,24 @@ export function createAgentChatService(args: { cancelDroidPermissionWaiter(w, "Droid tool approval was cancelled because the turn was interrupted."); } rt.permissionWaiters.clear(); - cancelQueuedSteers(managed, rt, "interrupted"); + if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted"); return result; } if (managed.session.provider === "droid") { droidRuntimeSetupInterruptRequested.set(managed, true); - cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + if (mode === "stop_and_clear") { + cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + } persistChatState(managed); return result; } if (managed.session.provider === "cursor") { cursorRuntimeSetupInterruptRequested.set(managed, true); - cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + if (mode === "stop_and_clear") { + cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + } persistChatState(managed); return result; } diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts new file mode 100644 index 000000000..1a58461e3 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts @@ -0,0 +1,64 @@ +import { describe, expect, it, vi } from "vitest"; +import { createSettleTeardownWiring } from "./settleTeardownWiring"; + +/** + * The wiring is where teardown meets the chat service, so it is where two + * whole-feature defects lived: a settle that destroyed the user's queued + * prompts, and a settle that declared a restarted session quiet while its + * background job kept running. + */ +describe("settle teardown wiring", () => { + const neverAborted = { isAborted: () => false }; + + function harness(summary: Record | null) { + const interrupt = vi.fn(async () => ({})); + const wiring = createSettleTeardownWiring({ + agentChatService: { + interrupt, + getSessionSummary: async () => summary as never, + }, + surface: "desktop", + }); + return { wiring, interrupt }; + } + + it("never asks a provider to clear the user's queued turns", async () => { + const { wiring, interrupt } = harness({ status: "active", provider: "claude" }); + + await wiring.runSettleTeardown("session-1", neverAborted); + + // `stop_and_clear` cancels queued follow-ups. Losing a settle costs one + // click; losing prompts the user already typed is unrecoverable. + expect(interrupt).toHaveBeenCalledWith({ sessionId: "session-1", mode: "stop_only" }); + }); + + it("treats a persisted Claude background job as work, even when the runtime is gone", async () => { + // What a restarted session looks like: no live runtime, so the live count + // is zero, but the daemon job is still recorded and still running. + const { wiring, interrupt } = harness({ + status: "idle", + provider: "claude", + activeBackgroundTaskCount: 0, + claudeBackgroundJobShort: "bg-42", + }); + + await wiring.runSettleTeardown("session-1", neverAborted); + + // Without this the settle sees a quiet session, stops nothing, and files it + // as done over a job that never stopped. + expect(interrupt, "a persisted background job must still be stopped").toHaveBeenCalled(); + }); + + it("does not interrupt a session that is genuinely idle", async () => { + const { wiring, interrupt } = harness({ + status: "idle", + provider: "claude", + activeBackgroundTaskCount: 0, + }); + + const outcome = await wiring.runSettleTeardown("session-1", neverAborted); + + expect(interrupt).not.toHaveBeenCalled(); + expect(outcome.residue).toEqual([]); + }); +}); diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts index 94859093e..db3e0a778 100644 --- a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts @@ -22,6 +22,7 @@ export type SettleTeardownChatService = { status: string; activeBackgroundTaskCount?: number | null; provider?: string | null; + claudeBackgroundJobShort?: string | null; } | null>; }; @@ -66,9 +67,16 @@ export function createSettleTeardownWiring(deps: SettleTeardownWiringDeps): Sett readActiveWork: async (sessionId) => { const summary = await deps.agentChatService.getSessionSummary(sessionId); if (!summary) return null; + // `activeBackgroundTaskCount` comes from the LIVE managed runtime, so it + // reads zero after a restart even while a Claude `--bg` job keeps running + // in the daemon. Counting the persisted job is what stops a settle from + // looking at a restarted session, seeing it quiet, and filing it as done + // over work it never stopped — the exact bug this feature exists to fix. + const liveCount = summary.activeBackgroundTaskCount ?? 0; + const persistedBackgroundJob = summary.claudeBackgroundJobShort ? 1 : 0; return { active: summary.status === "active", - backgroundTaskCount: summary.activeBackgroundTaskCount ?? 0, + backgroundTaskCount: Math.max(liveCount, persistedBackgroundJob), provider: summary.provider ?? null, }; }, diff --git a/apps/desktop/src/shared/types/chat.ts b/apps/desktop/src/shared/types/chat.ts index 70c53c31f..5719e433b 100644 --- a/apps/desktop/src/shared/types/chat.ts +++ b/apps/desktop/src/shared/types/chat.ts @@ -1623,6 +1623,16 @@ export type AgentChatSessionSummary = { /** Earliest armed, unpaused schedule for this chat. */ nextWakeAt: string | null; /** Authoritative provider-reported background tasks still running after the foreground turn. */ + /** + * A Claude `--bg` job this session started, when one is still recorded. + * + * Distinct from `activeBackgroundTaskCount`, which is derived from the LIVE + * managed runtime and therefore reads zero after a restart even though the + * daemon job is still running. Settle teardown needs the persisted signal: + * without it a settle looks at a restarted session, sees it quiet, stops + * nothing, and files it as done over work that never stopped. + */ + claudeBackgroundJobShort?: string | null; activeBackgroundTaskCount?: number; /** The same live work split into working vs monitoring (`classifyBackgroundWorkKind`). */ backgroundWork?: SessionBackgroundWork; From abec583b6ac5206d7917ba4a2ef2c4dead8ab6f9 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 07:11:05 -0400 Subject: [PATCH 11/14] fix: gate the persisted background job on daemon liveness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex, and it is a defect my own previous fix created. claudeBackgroundJobShort is a RECORD, not a liveness signal — it survives the job finishing and survives teardown stopping it. Counting it unconditionally meant every later settle on that session would spend the full confirmation budget and then report residue for a job that no longer exists, while trying to stop it again each time. Now the daemon is asked, through a narrow hasLiveClaudeBackgroundJob exported from agentChatService, and only when the live count already says quiet AND a job is on record — the restart case. That keeps the round-trip off the hot read while still closing the hole where a job outlives its runtime. Also documented, not fixed: a provider stop that overruns its 10s ceiling keeps running, because interrupt takes no abort signal, so a late session-scoped abort could stop a turn the user started after the settle was abandoned. Removing the ceiling is a certain wedge; keeping it is a narrow race needing a 10s+ hang, a new turn inside that window, and the abort still applying. Written up in the design doc (6c-ii) rather than silently traded. Two other comments on this head are stale re-posts: the stop_only gating landed in 86c4c5c62 (verified present in all seven provider branches), and the queue-drain in d499220c8. --- .../main/services/chat/agentChatService.ts | 11 +++++++++ .../sessions/sessionSettleTeardown.ts | 19 ++++++++++++++- .../sessions/settleTeardownWiring.test.ts | 24 +++++++++++++++++-- .../services/sessions/settleTeardownWiring.ts | 13 +++++++++- .../settle-teardown-design.md | 16 +++++++++++++ 5 files changed, 79 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/main/services/chat/agentChatService.ts b/apps/desktop/src/main/services/chat/agentChatService.ts index 27723f4f3..9fc4d64a7 100644 --- a/apps/desktop/src/main/services/chat/agentChatService.ts +++ b/apps/desktop/src/main/services/chat/agentChatService.ts @@ -44295,6 +44295,17 @@ export function createAgentChatService(args: { dispatchSteer, cancelDispatchedSteer, interrupt, + /** + * Is a persisted Claude `--bg` job actually still running? + * + * `claudeBackgroundJobShort` is a RECORD, not a liveness signal — it stays + * on the session after the job finishes or is stopped. Settle teardown has + * to distinguish the two: counting a finished job as work makes every later + * settle spend the confirmation budget and then report residue that does + * not exist. + */ + hasLiveClaudeBackgroundJob: async (short: string | null | undefined): Promise => + (await getLiveClaudeBackgroundSocket(short)) !== null, restoreCancelledQueue, recoverTurn, recoverCodexTurn, diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts index 18de93136..ea11125df 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts @@ -101,7 +101,24 @@ const STOP_CONFIRM_MAX_POLL_MS = 800; */ const PROVIDER_CALL_TIMEOUT_MS = 10_000; -/** Resolves to not-ok rather than rejecting, so a slow provider is residue, not a crash. */ +/** + * Resolves to not-ok rather than rejecting, so a slow provider is residue, not + * a crash. + * + * KNOWN LIMITATION: the losing arm keeps running. `agentChatService.interrupt` + * takes no abort signal, so a provider stop that overruns the ceiling cannot be + * recalled — and a session-scoped one (OpenCode's `session.abort`) could land + * after the settle was abandoned and stop a turn the user has since started, + * which is the one thing §3c says must never happen. + * + * Shipped anyway, deliberately: the alternative is no ceiling, and an + * un-bounded await holds the settling window open forever and leaves the row + * permanently unsettleable — a certain failure traded for a narrow one. The + * window needs a provider stop to overrun 10s AND the user to start a turn + * inside it AND the late abort to still apply, and a provider hung that long is + * usually not delivering the abort either. Closing it properly means threading + * an AbortSignal through every provider branch of `interrupt`. + */ async function withTimeout( work: Promise, expire: () => Promise, diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts index 1a58461e3..9052160f6 100644 --- a/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts @@ -10,16 +10,18 @@ import { createSettleTeardownWiring } from "./settleTeardownWiring"; describe("settle teardown wiring", () => { const neverAborted = { isAborted: () => false }; - function harness(summary: Record | null) { + function harness(summary: Record | null, backgroundJobAlive = true) { const interrupt = vi.fn(async () => ({})); + const hasLiveClaudeBackgroundJob = vi.fn(async () => backgroundJobAlive); const wiring = createSettleTeardownWiring({ agentChatService: { interrupt, getSessionSummary: async () => summary as never, + hasLiveClaudeBackgroundJob, }, surface: "desktop", }); - return { wiring, interrupt }; + return { wiring, interrupt, hasLiveClaudeBackgroundJob }; } it("never asks a provider to clear the user's queued turns", async () => { @@ -49,6 +51,24 @@ describe("settle teardown wiring", () => { expect(interrupt, "a persisted background job must still be stopped").toHaveBeenCalled(); }); + it("ignores a recorded background job the daemon says is already gone", async () => { + // The short is a RECORD, not a liveness signal — it survives the job + // finishing. Trusting it alone makes every later settle burn the + // confirmation budget and then report residue that does not exist. + const { wiring, interrupt, hasLiveClaudeBackgroundJob } = harness({ + status: "idle", + provider: "claude", + activeBackgroundTaskCount: 0, + claudeBackgroundJobShort: "bg-42", + }, false); + + const outcome = await wiring.runSettleTeardown("session-1", neverAborted); + + expect(hasLiveClaudeBackgroundJob).toHaveBeenCalledWith("bg-42"); + expect(interrupt, "a finished job must not be stopped again").not.toHaveBeenCalled(); + expect(outcome.residue, "a finished job is not residue").toEqual([]); + }); + it("does not interrupt a session that is genuinely idle", async () => { const { wiring, interrupt } = harness({ status: "idle", diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts index db3e0a778..bbe4a6d2a 100644 --- a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts @@ -24,6 +24,8 @@ export type SettleTeardownChatService = { provider?: string | null; claudeBackgroundJobShort?: string | null; } | null>; + /** Liveness for a persisted Claude `--bg` job; the recorded short alone is not one. */ + hasLiveClaudeBackgroundJob?: (short: string | null | undefined) => Promise; }; export type SettleTeardownWiringDeps = { @@ -73,7 +75,16 @@ export function createSettleTeardownWiring(deps: SettleTeardownWiringDeps): Sett // looking at a restarted session, seeing it quiet, and filing it as done // over work it never stopped — the exact bug this feature exists to fix. const liveCount = summary.activeBackgroundTaskCount ?? 0; - const persistedBackgroundJob = summary.claudeBackgroundJobShort ? 1 : 0; + // Only asked when the live count says quiet AND a job is on record — the + // restart case. The daemon round-trip is not worth paying on every read, + // and the short is a record rather than a liveness signal: it survives the + // job finishing, so trusting it alone would make every later settle burn + // the confirmation budget and report residue that no longer exists. + const persistedBackgroundJob = liveCount === 0 + && summary.claudeBackgroundJobShort + && await deps.agentChatService.hasLiveClaudeBackgroundJob?.(summary.claudeBackgroundJobShort) + ? 1 + : 0; return { active: summary.status === "active", backgroundTaskCount: Math.max(liveCount, persistedBackgroundJob), diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index 75fd1a876..6ba015379 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -629,6 +629,22 @@ that needs the mobile side**, and either option costs something: Neither belongs in a desktop/CLI branch on its own. +### 6c-ii. Known limitation: a timed-out provider stop cannot be recalled + +Every provider call has a 10s ceiling, without which a hung control call holds +the settling window open forever and the row can never be settled again. The +losing arm of that race keeps running, though: `agentChatService.interrupt` +takes no abort signal. So a session-scoped stop that overruns — OpenCode's +`session.abort` — could in principle land after the settle was abandoned and +stop a turn the user started in the meantime, which §3c says must never happen. + +This is a trade between a certain failure and a narrow one. Removing the ceiling +makes the wedge certain; keeping it needs a provider stop to overrun 10s AND the +user to start a turn inside that window AND the late abort to still apply, and a +provider hung that long is usually not delivering the abort either. Closing it +properly means threading an `AbortSignal` through every provider branch of +`interrupt`, which is its own change. + ### 6d. Peer tuple writes: host authority finished, not consensus added R7's fix is in the **apply layer**: `db.sync.applyChanges`, the one place both From df2aa0ad68dcf27234997be3ef4413c522bc604f Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 07:31:31 -0400 Subject: [PATCH 12/14] fix: make background-job liveness required and tri-state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from this round's review, and both are defects my own liveness fix introduced one commit earlier. hasLiveClaudeBackgroundJob was optional, so a wiring that omitted it read a recorded job as absent and confirmed a clean teardown over work still running — reopening the exact hole the callback was added to close. Now required. And it returned a boolean, which collapsed "the daemon says the job is gone" into the same answer as "the daemon could not be reached" (getLiveClaudeBackgroundSocket catches socket and request failures and returns null). Guessing "finished" is the guess that settles over a running job — the same shape as treating a timed-out liveness read as an idle session, which this branch has now had to fix three times in three places. It returns alive / gone / unknown, and only a definite "gone" counts as no work. Also pinned: interrupt's daemon stop branch is gated on there being no resident Claude runtime, so a resumed session with a live --bg job takes the SDK branch and the job survives. Teardown does not claim that as clean — the confirmation loop still sees the job and reports residue — and there is now a test saying so. Changing that gate would change what the Stop button does, which is not this branch's call. Restored the activeBackgroundTaskCount JSDoc my insertion had detached. --- .../main/services/chat/agentChatService.ts | 21 ++++++++- .../sessions/settleTeardownWiring.test.ts | 44 +++++++++++++++++-- .../services/sessions/settleTeardownWiring.ts | 22 +++++++--- apps/desktop/src/shared/types/chat.ts | 13 +++--- 4 files changed, 84 insertions(+), 16 deletions(-) diff --git a/apps/desktop/src/main/services/chat/agentChatService.ts b/apps/desktop/src/main/services/chat/agentChatService.ts index 9fc4d64a7..111d959e4 100644 --- a/apps/desktop/src/main/services/chat/agentChatService.ts +++ b/apps/desktop/src/main/services/chat/agentChatService.ts @@ -44304,8 +44304,25 @@ export function createAgentChatService(args: { * settle spend the confirmation budget and then report residue that does * not exist. */ - hasLiveClaudeBackgroundJob: async (short: string | null | undefined): Promise => - (await getLiveClaudeBackgroundSocket(short)) !== null, + hasLiveClaudeBackgroundJob: async ( + short: string | null | undefined, + ): Promise<"alive" | "gone" | "unknown"> => { + const normalized = normalizeClaudeBackgroundShort(short); + if (!normalized) return "gone"; + const socketPath = await resolveClaudeDaemonControlSocket(); + // No daemon socket, or a request that failed: we do not KNOW the job is + // gone. Collapsing that to "gone" is how a settle confirms a clean + // teardown over a job that is still running — the same mistake as + // treating a timed-out liveness read as an idle session. + if (!socketPath) return "unknown"; + try { + const response = await sendClaudeDaemonRequest(socketPath, { op: "has", short: normalized }); + if (response.ok !== true) return "unknown"; + return response.alive === true || response.present === true ? "alive" : "gone"; + } catch { + return "unknown"; + } + }, restoreCancelledQueue, recoverTurn, recoverCodexTurn, diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts index 9052160f6..d986a0d65 100644 --- a/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts @@ -10,9 +10,12 @@ import { createSettleTeardownWiring } from "./settleTeardownWiring"; describe("settle teardown wiring", () => { const neverAborted = { isAborted: () => false }; - function harness(summary: Record | null, backgroundJobAlive = true) { + function harness( + summary: Record | null, + backgroundJob: "alive" | "gone" | "unknown" = "alive", + ) { const interrupt = vi.fn(async () => ({})); - const hasLiveClaudeBackgroundJob = vi.fn(async () => backgroundJobAlive); + const hasLiveClaudeBackgroundJob = vi.fn(async () => backgroundJob); const wiring = createSettleTeardownWiring({ agentChatService: { interrupt, @@ -60,7 +63,7 @@ describe("settle teardown wiring", () => { provider: "claude", activeBackgroundTaskCount: 0, claudeBackgroundJobShort: "bg-42", - }, false); + }, "gone"); const outcome = await wiring.runSettleTeardown("session-1", neverAborted); @@ -69,6 +72,41 @@ describe("settle teardown wiring", () => { expect(outcome.residue, "a finished job is not residue").toEqual([]); }); + it("treats an unreachable daemon as work, not as a finished job", async () => { + // `getLiveClaudeBackgroundSocket` turns socket and request failures into a + // null, so "cannot reach the daemon" and "the job is gone" arrive looking + // identical. Guessing "finished" is the guess that settles over a job that + // is still running. + const { wiring, interrupt } = harness({ + status: "idle", + provider: "claude", + activeBackgroundTaskCount: 0, + claudeBackgroundJobShort: "bg-42", + }, "unknown"); + + await wiring.runSettleTeardown("session-1", neverAborted); + + expect(interrupt, "unknown liveness must still attempt the stop").toHaveBeenCalled(); + }); + + it("reports residue when a daemon job survives the interrupt", async () => { + // `interrupt`'s daemon `stop ` branch is gated on there being no + // resident Claude runtime, so a resumed session with a live `--bg` job + // takes the SDK branch and the daemon job keeps running. Teardown must not + // call that a clean settle — it cannot stop the job, so it says so. + const { wiring } = harness({ + status: "idle", + provider: "claude", + activeBackgroundTaskCount: 0, + claudeBackgroundJobShort: "bg-42", + }, "alive"); + + const outcome = await wiring.runSettleTeardown("session-1", neverAborted); + + expect(outcome.residue, "an unstoppable daemon job must be reported").not.toEqual([]); + expect(outcome.residue[0]?.kind).toBe("background_tasks"); + }); + it("does not interrupt a session that is genuinely idle", async () => { const { wiring, interrupt } = harness({ status: "idle", diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts index bbe4a6d2a..c0dbb4d51 100644 --- a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts @@ -24,8 +24,18 @@ export type SettleTeardownChatService = { provider?: string | null; claudeBackgroundJobShort?: string | null; } | null>; - /** Liveness for a persisted Claude `--bg` job; the recorded short alone is not one. */ - hasLiveClaudeBackgroundJob?: (short: string | null | undefined) => Promise; + /** + * Liveness for a persisted Claude `--bg` job; the recorded short alone is not + * one. REQUIRED, not optional: a wiring without it would read a recorded job + * as absent and confirm a clean teardown over work that is still running, + * which is the hole this callback exists to close. + * + * `"unknown"` is a distinct answer on purpose — an unreachable daemon is not + * evidence the job finished. + */ + hasLiveClaudeBackgroundJob: ( + short: string | null | undefined, + ) => Promise<"alive" | "gone" | "unknown">; }; export type SettleTeardownWiringDeps = { @@ -80,9 +90,11 @@ export function createSettleTeardownWiring(deps: SettleTeardownWiringDeps): Sett // and the short is a record rather than a liveness signal: it survives the // job finishing, so trusting it alone would make every later settle burn // the confirmation budget and report residue that no longer exists. - const persistedBackgroundJob = liveCount === 0 - && summary.claudeBackgroundJobShort - && await deps.agentChatService.hasLiveClaudeBackgroundJob?.(summary.claudeBackgroundJobShort) + const persistedBackgroundJob = liveCount === 0 && summary.claudeBackgroundJobShort + // Anything but a definite "gone" counts as work. An unreachable daemon + // means we cannot tell, and guessing "finished" is the one guess that + // settles over a running job. + && await deps.agentChatService.hasLiveClaudeBackgroundJob(summary.claudeBackgroundJobShort) !== "gone" ? 1 : 0; return { diff --git a/apps/desktop/src/shared/types/chat.ts b/apps/desktop/src/shared/types/chat.ts index 5719e433b..c337e49ae 100644 --- a/apps/desktop/src/shared/types/chat.ts +++ b/apps/desktop/src/shared/types/chat.ts @@ -1622,17 +1622,18 @@ export type AgentChatSessionSummary = { pendingInputItemId?: string | null; /** Earliest armed, unpaused schedule for this chat. */ nextWakeAt: string | null; - /** Authoritative provider-reported background tasks still running after the foreground turn. */ /** * A Claude `--bg` job this session started, when one is still recorded. * - * Distinct from `activeBackgroundTaskCount`, which is derived from the LIVE - * managed runtime and therefore reads zero after a restart even though the - * daemon job is still running. Settle teardown needs the persisted signal: - * without it a settle looks at a restarted session, sees it quiet, stops - * nothing, and files it as done over work that never stopped. + * A RECORD, not a liveness signal: it survives the job finishing and survives + * teardown stopping it, so a reader that needs liveness has to ask the daemon. + * Distinct from `activeBackgroundTaskCount` below, which is derived from the + * LIVE managed runtime and therefore reads zero after a restart even while + * the daemon job is still running — which is why settle teardown consults + * both. */ claudeBackgroundJobShort?: string | null; + /** Authoritative provider-reported background tasks still running after the foreground turn. */ activeBackgroundTaskCount?: number; /** The same live work split into working vs monitoring (`classifyBackgroundWorkKind`). */ backgroundWork?: SessionBackgroundWork; From b1fc48fd1bc7037dd699f43ca8e7930e25659127 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 07:56:13 -0400 Subject: [PATCH 13/14] fix: scope the abort check to the window the teardown actually owns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex, and it is the mirror of a fix already made for closing the window. `end(id, token)` refuses to close a window it does not own, but the abort check still read `abortedBy(id)` — whatever entry currently occupies the id. When `deleteSession` runs mid-teardown, `forget` force-closes the entry; if the id is then recreated and a new settle opens a fresh window, the stale teardown reads the REPLACEMENT, sees "not aborted", and keeps issuing provider stops against the new session's work. `abandoned(id, token)` treats a missing or mismatched entry as abandoned, and both the in-flight check and the post-await check use it. A settle that no longer owns its window must stop as surely as one that was aborted. The other seven comments on this head are threads GitHub re-anchored: stop_only gating (86c4c5c62, verified in all seven provider branches), the queue drain (d499220c8), persisted-job liveness and unknown-as-residue (df2aa0ad6), the JSDoc association (df2aa0ad6), the resident-runtime daemon job (pinned as residue rather than silently clean, with a test), and the un-cancellable timed-out interrupt, which is documented as a known limitation in 6c-ii because removing the ceiling trades a narrow race for a certain wedge. --- .../main/services/sessions/sessionService.ts | 16 +++++++++++----- .../sessions/settleRaceMatrix.test.ts | 19 +++++++++++++++++++ .../sessions/settlingStateRegistry.ts | 14 ++++++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index be9eac6dc..70d37d864 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -832,8 +832,10 @@ export function createSessionService({ teardown = runSettleTeardown ? await runSettleTeardown(id, { // Read live, not captured: the whole point is that a clearer can - // trip it while teardown is between stop calls. - isAborted: () => settleLifecycle.settling.abortedBy(id) !== null, + // trip it while teardown is between stop calls. Scoped to OUR + // token, so a window that was force-closed and reopened by a + // different settle reads as abandoned rather than as healthy. + isAborted: () => settleLifecycle.settling.abandoned(id, begin.token), }) : null; } catch (error) { @@ -850,9 +852,13 @@ export function createSessionService({ return outcome; } - const abortedBy = settleLifecycle.settling.abortedBy(id); - if (abortedBy) { - outcome.aborted.push({ sessionId: id, reason: abortedBy }); + // Same scoping after the await: if this window was replaced while + // teardown ran, the settle it belonged to is gone and must not land. + if (settleLifecycle.settling.abandoned(id, begin.token)) { + outcome.aborted.push({ + sessionId: id, + reason: settleLifecycle.settling.abortedBy(id) ?? "lifecycle_changed", + }); return outcome; } // The revision catches everything the abort flag cannot: a settle-tuple diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index 39e955aab..696954e72 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -364,6 +364,25 @@ describe("settle race matrix", () => { }); /** A settling row found after a restart resolves to not-settled. */ + it("stops a teardown whose window was force-closed out from under it", async () => { + const { service, setTeardown, teardownContexts } = await fixture(); + let sawAbandoned: boolean | null = null; + + setTeardown(() => { + // What `deleteSession` does mid-teardown: `forget` force-closes this + // window. Whatever occupies the id afterwards — nothing, or a fresh + // window opened by a later settle for a recreated session — is not ours, + // and a teardown reading `abortedBy` alone would see "not aborted" and + // keep issuing provider stops. + service.deleteSession("session-1"); + sawAbandoned = teardownContexts.at(-1)?.isAborted() ?? null; + }); + + await service.settleSessionsReportingAborts(["session-1"]); + + expect(sawAbandoned, "a teardown that lost its window must see itself abandoned").toBe(true); + }); + it("crash safety: the settling window does not survive the process", async () => { const { service, setTeardown } = await fixture(); expect(service.settlingSessionIds()).toEqual([]); diff --git a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts index fe57e4961..435eaa77b 100644 --- a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts +++ b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts @@ -100,6 +100,20 @@ export class SettlingStateRegistry { return this.entries.get(sessionId)?.abortedBy ?? null; } + /** + * Has THIS window been abandoned — either aborted, or replaced? + * + * A settle that no longer owns the id must stop as surely as an aborted one. + * `forget` (a deleted session) force-closes the entry, and if the id is then + * recreated a new settle opens a fresh window; a stale teardown consulting + * `abortedBy` alone would read the replacement, see "not aborted", and keep + * issuing provider stops against the new session's work. + */ + abandoned(sessionId: string, token: number): boolean { + const entry = this.entries.get(sessionId); + return !entry || entry.token !== token || entry.abortedBy !== null; + } + startedAtRevision(sessionId: string): number | null { return this.entries.get(sessionId)?.startedAtRevision ?? null; } From 9be4f22722476601c219d799c9c798735aa72a13 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 08:12:28 -0400 Subject: [PATCH 14/14] fix: a hung provider stop is a timeout, not a rejection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex. The 10s ceiling set `stopRejected`, so a provider that never answered was filed identically to one that explicitly refused — in the residue the user reads AND in the settle_teardown_residue analytics dimension. That conflation is precisely what the reason field exists to prevent, and it would have made "how often do stops actually fail in the field" unanswerable, which is the question 3d option 3 added the event to answer. Two existing assertions had encoded the bug rather than catching it: both said a never-resolving interrupt should read "rejected". Corrected, and the fix was probed against them. --- .../sessions/sessionSettleTeardown.test.ts | 21 ++++++++++++++++++- .../sessions/sessionSettleTeardown.ts | 15 +++++++++---- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts index 38bf4724d..8a71f515b 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts @@ -97,6 +97,21 @@ describe("session settle teardown", () => { ]); }); + it("R5: calls a hung stop a timeout, not a rejection", async () => { + const { run } = harness({ + // Never answers. The provider did not refuse — it did not reply at all, + // and filing that as an explicit rejection is exactly the conflation the + // reason field exists to prevent. + interrupt: vi.fn(() => new Promise(() => {})), + readActiveWork: async () => work({ backgroundTaskCount: 1, provider: "codex" }), + expireProviderCall: async () => {}, + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue[0]?.reason).toBe("timeout"); + }); + it("R5: calls out a provider that has no stop control at all", async () => { const { run } = harness({ // A Codex chat cannot stop an individual subagent. That is a different @@ -162,7 +177,11 @@ describe("session settle teardown", () => { const outcome = await run("session-1", neverAborted); - expect(outcome.residue[0]?.reason).toBe("rejected"); + // A provider that never answered did not REFUSE. This said "rejected" and + // was encoding the misclassification: every hung call would reach both the + // user-visible residue and the analytics dimension as an explicit provider + // rejection. + expect(outcome.residue[0]?.reason).toBe("timeout"); }); it("resolves instead of blocking the settle when a liveness read hangs", async () => { diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts index ea11125df..6640849ec 100644 --- a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts @@ -206,11 +206,16 @@ export function createSessionSettleTeardown( // work we have NOT done yet. if (ctx.isAborted()) return { residue, confirmed: false }; + // Tracked apart: a provider that REFUSED the stop and one that never + // answered are different facts, and collapsing them is what the reason + // field exists to prevent — a hung call would be filed as an explicit + // rejection in both the residue the user sees and the analytics. let stopRejected = false; + let stopTimedOut = false; try { const stop = await withTimeout(deps.interrupt(sessionId), expireProviderCall); if (!stop.ok) { - stopRejected = true; + stopTimedOut = true; deps.logger?.warn("settle_teardown.step_timed_out", { step: "interrupt" }); } } catch (error) { @@ -232,9 +237,11 @@ export function createSessionSettleTeardown( if (after && !ctx.isAborted()) { const reason = stopRejected ? "rejected" as const - : provider && noStopControl.has(provider) - ? "no_stop_control" as const - : "timeout" as const; + : stopTimedOut + ? "timeout" as const + : provider && noStopControl.has(provider) + ? "no_stop_control" as const + : "timeout" as const; // A surviving turn and surviving background tasks are separate facts. // Folding them into one item lost both the kind and the count — the two // things the residue exists to report.