From a34fd679b15efbdbb277361d5165316aa41e33a7 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Mon, 10 Aug 2026 23:55:47 -0400 Subject: [PATCH 01/10] Settling state, abort rule, and the race matrix (settle teardown, step 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The revision from step 1 detects that the world moved. It does not say what to do when it moves after work has already been stopped — R2, the one race where both outcomes are bad. This is the other half, with teardown still a NO-OP. **The settling window** (`settlingStateRegistry.ts`) is visible, exclusive, abortable, and crash-safe. In memory deliberately: the design requires a `settling` row found at startup to resolve to not-settled, and in-memory gives that with no recovery code to get wrong. It also keeps the marker off `terminal_sessions`, which is a CRR — a replicated "settling" flag would be a lie on every other device the moment this host died. **The clearer split is what makes teardown possible at all.** Stopping a process emits output, so C4 fires *because* teardown is running. If that cleared the settle or tripped abort, every real teardown would self-abort and the feature would be dead on arrival. C4/C5 are therefore swallowed on all three axes — no tuple clear, no revision bump, no abort — while their own columns still land, so the row keeps showing live output while it settles. C3/C6/C7 abort. Outside the window all five behave exactly as before. The `cause` rides on the intent, so the writer decides rather than each call site remembering to. **Aborts are typed, never silent.** `settleSessionsReportingAborts` returns `{settled, aborted[]}`. PR-merge auto-settle consumes it and does NOT mark the PR handled when the settle was abandoned — `handledPrIds` is the only thing stopping a retry, so marking it would consume the merge forever. Probe-verified: removing that guard drops the retry and the test fails. **Race matrix, executed rather than argued.** R1-R7 in `settleRaceMatrix.test.ts` against the real revision, driving the teardown seam by hand. R7 is new — a paired desktop peer's settle arrives via `crsql_changes`, never passes the writer, so the revision does not move and the id comes back in NEITHER list. It asserts today's behavior, not the behavior we want, so step 3 sees the blast radius before there is work to lose. Teardown is injected at construction and absent here by design. --- .../src/main/services/prs/prAsync.test.ts | 172 ++++++---- .../prs/prMergeAutoSettlementService.ts | 28 +- .../main/services/sessions/sessionService.ts | 101 +++++- .../sessions/settleLifecycleWriter.ts | 63 +++- .../sessions/settleRaceMatrix.test.ts | 294 ++++++++++++++++++ .../sessions/settlingStateRegistry.ts | 96 ++++++ .../settle-teardown-design.md | 33 +- 7 files changed, 695 insertions(+), 92 deletions(-) create mode 100644 apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts create mode 100644 apps/desktop/src/main/services/sessions/settlingStateRegistry.ts diff --git a/apps/desktop/src/main/services/prs/prAsync.test.ts b/apps/desktop/src/main/services/prs/prAsync.test.ts index a586a2123..3293db62e 100644 --- a/apps/desktop/src/main/services/prs/prAsync.test.ts +++ b/apps/desktop/src/main/services/prs/prAsync.test.ts @@ -833,9 +833,9 @@ describe("prMergeAutoSettlementService", () => { it("settles lane agent sessions even when a merge has settlement blockers", async () => { const db = createMemoryDb(); const settledSessionIds = new Set(); - const settleSessionsWithOutcome = vi.fn((ids: string[]) => { + const settleSessionsReportingAborts = vi.fn((ids: string[]) => { ids.forEach((id) => settledSessionIds.add(id)); - return ids; + return { settled: ids, aborted: [] }; }); const emitEvent = vi.fn(); const service = createPrMergeAutoSettlementService({ @@ -865,7 +865,7 @@ describe("prMergeAutoSettlementService", () => { }, ]), get: vi.fn(() => null), - settleSessionsWithOutcome, + settleSessionsReportingAborts, }) as any, emitEvent, }); @@ -875,7 +875,7 @@ describe("prMergeAutoSettlementService", () => { prs: [openPr], polledAt: "2026-03-24T12:00:00.000Z", }); - expect(settleSessionsWithOutcome).not.toHaveBeenCalled(); + expect(settleSessionsReportingAborts).not.toHaveBeenCalled(); const mergedPr = createSummary({ state: "merged", @@ -886,17 +886,13 @@ describe("prMergeAutoSettlementService", () => { polledAt: "2026-03-24T12:01:05.000Z", }); - expect(settleSessionsWithOutcome).toHaveBeenCalledWith( + expect(settleSessionsReportingAborts).toHaveBeenCalledWith( ["chat-ready"], - "PR #101 merged", - "2026-03-24T12:01:05.000Z", - "pr_merge", + { outcome: "PR #101 merged", settledAt: "2026-03-24T12:01:05.000Z", source: "pr_merge" }, ); - expect(settleSessionsWithOutcome).toHaveBeenCalledWith( + expect(settleSessionsReportingAborts).toHaveBeenCalledWith( ["cli-blocked"], - "PR #101 merged", - "2026-03-24T12:01:05.000Z", - "pr_merge", + { outcome: "PR #101 merged", settledAt: "2026-03-24T12:01:05.000Z", source: "pr_merge" }, ); expect(emitEvent).toHaveBeenCalledWith(expect.objectContaining({ type: "pr-sessions-auto-settled", @@ -909,16 +905,16 @@ describe("prMergeAutoSettlementService", () => { prs: [mergedPr], polledAt: "2026-03-24T12:02:00.000Z", }); - expect(settleSessionsWithOutcome).toHaveBeenCalledTimes(2); + expect(settleSessionsReportingAborts).toHaveBeenCalledTimes(2); }); it("does not re-settle after reactivation, but settles for a later PR", async () => { const db = createMemoryDb(); let settled = false; - const settleSessionsWithOutcome = vi.fn((ids: string[]) => { + const settleSessionsReportingAborts = vi.fn((ids: string[]) => { settled = true; - return ids; + return { settled: ids, aborted: [] }; }); const service = createPrMergeAutoSettlementService({ db: db as any, @@ -931,7 +927,7 @@ describe("prMergeAutoSettlementService", () => { settledAt: settled ? "2026-03-24T12:01:05.000Z" : null, }]), get: vi.fn(() => null), - settleSessionsWithOutcome, + settleSessionsReportingAborts, }) as any, emitEvent: vi.fn(), }); @@ -966,12 +962,10 @@ describe("prMergeAutoSettlementService", () => { polledAt: "2026-03-24T12:01:05.000Z", }); - expect(settleSessionsWithOutcome).toHaveBeenCalledTimes(1); - expect(settleSessionsWithOutcome).toHaveBeenLastCalledWith( + expect(settleSessionsReportingAborts).toHaveBeenCalledTimes(1); + expect(settleSessionsReportingAborts).toHaveBeenLastCalledWith( ["chat-waiting"], - "PR #101 merged", - "2026-03-24T12:01:05.000Z", - "pr_merge", + { outcome: "PR #101 merged", settledAt: "2026-03-24T12:01:05.000Z", source: "pr_merge" }, ); expect(getPrMergeAutoSettlementState(db as any)?.handledPrIds).toEqual(["pr-1"]); @@ -982,7 +976,7 @@ describe("prMergeAutoSettlementService", () => { prs: [mergedPr, openSecondPr], polledAt: "2026-03-24T12:02:00.000Z", }); - expect(settleSessionsWithOutcome).toHaveBeenCalledTimes(1); + expect(settleSessionsReportingAborts).toHaveBeenCalledTimes(1); // A distinct PR on the same lane gets its own one-shot settlement. await service.processSnapshot({ @@ -990,19 +984,17 @@ describe("prMergeAutoSettlementService", () => { polledAt: "2026-03-24T12:03:05.000Z", }); - expect(settleSessionsWithOutcome).toHaveBeenLastCalledWith( + expect(settleSessionsReportingAborts).toHaveBeenLastCalledWith( ["chat-waiting"], - "PR #202 merged", - "2026-03-24T12:03:05.000Z", - "pr_merge", + { outcome: "PR #202 merged", settledAt: "2026-03-24T12:03:05.000Z", source: "pr_merge" }, ); - expect(settleSessionsWithOutcome).toHaveBeenCalledTimes(2); + expect(settleSessionsReportingAborts).toHaveBeenCalledTimes(2); expect(getPrMergeAutoSettlementState(db as any)?.handledPrIds).toEqual(["pr-1", "pr-2"]); }); it("baselines old merges and only settles merges observed after re-enabling", async () => { const db = createMemoryDb(); - const settleSessionsWithOutcome = vi.fn((ids: string[]) => ids); + const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ settled: ids, aborted: [] as Array<{ sessionId: string; reason: string }> })); const service = createPrMergeAutoSettlementService({ db: db as any, sessionService: withSessionLookup({ @@ -1013,7 +1005,7 @@ describe("prMergeAutoSettlementService", () => { settledAt: null, }]), get: vi.fn(() => null), - settleSessionsWithOutcome, + settleSessionsReportingAborts, }) as any, emitEvent: vi.fn(), }); @@ -1029,7 +1021,7 @@ describe("prMergeAutoSettlementService", () => { prs: [oldMerge], polledAt: "2026-03-24T12:00:00.000Z", }); - expect(settleSessionsWithOutcome).not.toHaveBeenCalled(); + expect(settleSessionsReportingAborts).not.toHaveBeenCalled(); expect(getPrMergeAutoSettlementState(db as any)?.handledPrIds).toEqual(["pr-1"]); setSessionLifecycleSettings({ @@ -1067,17 +1059,15 @@ describe("prMergeAutoSettlementService", () => { prs: [oldMerge, futureMerge], polledAt: "2026-03-24T12:03:05.000Z", }); - expect(settleSessionsWithOutcome).toHaveBeenCalledWith( + expect(settleSessionsReportingAborts).toHaveBeenCalledWith( ["chat-ready"], - "PR #202 merged", - "2026-03-24T12:03:05.000Z", - "pr_merge", + { outcome: "PR #202 merged", settledAt: "2026-03-24T12:03:05.000Z", source: "pr_merge" }, ); }); it("settles only the chats explicitly linked to a merged PR", async () => { const db = createMemoryDb(); - const settleSessionsWithOutcome = vi.fn((ids: string[]) => ids); + const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ settled: ids, aborted: [] as Array<{ sessionId: string; reason: string }> })); const service = createPrMergeAutoSettlementService({ db: db as any, sessionService: withSessionLookup({ @@ -1086,7 +1076,7 @@ describe("prMergeAutoSettlementService", () => { { laneId: "lane-1", id: "chat-other", toolType: "codex-chat", archivedAt: null, settledAt: null }, ]), get: vi.fn(() => null), - settleSessionsWithOutcome, + settleSessionsReportingAborts, }) as any, emitEvent: vi.fn(), }); @@ -1100,14 +1090,64 @@ describe("prMergeAutoSettlementService", () => { }); await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:01:05.000Z" }); - expect(settleSessionsWithOutcome).toHaveBeenCalledWith( + expect(settleSessionsReportingAborts).toHaveBeenCalledWith( ["chat-owned"], - "PR #101 merged", - "2026-03-24T12:01:05.000Z", - "pr_merge", + { outcome: "PR #101 merged", settledAt: "2026-03-24T12:01:05.000Z", source: "pr_merge" }, ); }); + /** + * The merge must survive an abandoned settle. + * + * `handledPrIds` is the only thing stopping a later pass retrying, so marking + * a PR handled after a settle that never landed consumes the merge forever — + * the session stays unsettled and nothing ever files it again. This is the + * durable consequence that made the typed outcome worth having: the old + * changed-id list left an aborted id simply absent, indistinguishable from a + * session that was never eligible. + */ + it("does not mark a PR handled when its settle was abandoned by activity", async () => { + const db = createMemoryDb(); + const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ + settled: [] as string[], + aborted: ids.map((sessionId) => ({ sessionId, reason: "turn_start" })), + })); + const service = createPrMergeAutoSettlementService({ + db: db as any, + sessionService: withSessionLookup({ + list: vi.fn(() => [ + { laneId: "lane-1", id: "chat-owned", toolType: "codex-chat", archivedAt: null, settledAt: null }, + ]), + get: vi.fn(() => null), + settleSessionsReportingAborts, + }) as any, + emitEvent: vi.fn(), + }); + + const openPr = createSummary({ state: "open" }); + await service.processSnapshot({ prs: [openPr], polledAt: "2026-03-24T12:00:00.000Z" }); + + const mergedPr = createSummary({ + state: "merged", + mergedAt: "2026-03-24T12:01:00.000Z", + chatSessionIds: ["chat-owned"], + }); + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:01:05.000Z" }); + expect(settleSessionsReportingAborts).toHaveBeenCalledTimes(1); + + // The user's turn ended; a later pass must get another go. + settleSessionsReportingAborts.mockImplementation((ids: string[]) => ({ + settled: ids, + aborted: [] as Array<{ sessionId: string; reason: string }>, + })); + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:05:00.000Z" }); + + expect( + settleSessionsReportingAborts, + "an abandoned settle must not consume the merge", + ).toHaveBeenCalledTimes(2); + }); + it("settles a PR that was already merged when first seen, but announces nothing", async () => { // The machine-switch bug. Point the project tab at another machine and that // machine reconciles, backfilling rows for PRs it had never stored. Every @@ -1118,7 +1158,7 @@ describe("prMergeAutoSettlementService", () => { // // Filing the sessions is still right; announcing is not. const db = createMemoryDb(); - const settleSessionsWithOutcome = vi.fn((ids: string[]) => ids); + const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ settled: ids, aborted: [] as Array<{ sessionId: string; reason: string }> })); const emitEvent = vi.fn(); const service = createPrMergeAutoSettlementService({ db: db as any, @@ -1130,7 +1170,7 @@ describe("prMergeAutoSettlementService", () => { settledAt: null, }]), get: vi.fn(() => null), - settleSessionsWithOutcome, + settleSessionsReportingAborts, }) as any, emitEvent, }); @@ -1157,7 +1197,7 @@ describe("prMergeAutoSettlementService", () => { polledAt: "2026-03-24T12:05:00.000Z", }); - expect(settleSessionsWithOutcome).toHaveBeenCalledTimes(2); + expect(settleSessionsReportingAborts).toHaveBeenCalledTimes(2); expect(emitEvent).not.toHaveBeenCalled(); // And a merge we actually watch still announces itself, so the fix does not @@ -1183,7 +1223,7 @@ describe("prMergeAutoSettlementService", () => { it("honors disabling auto-settle before a merged PR is processed", async () => { const db = createMemoryDb(); - const settleSessionsWithOutcome = vi.fn((ids: string[]) => ids); + const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ settled: ids, aborted: [] as Array<{ sessionId: string; reason: string }> })); const service = createPrMergeAutoSettlementService({ db: db as any, sessionService: withSessionLookup({ @@ -1194,7 +1234,7 @@ describe("prMergeAutoSettlementService", () => { settledAt: null, }]), get: vi.fn(() => null), - settleSessionsWithOutcome, + settleSessionsReportingAborts, }) as any, emitEvent: vi.fn(), }); @@ -1218,7 +1258,7 @@ describe("prMergeAutoSettlementService", () => { polledAt: "2026-03-24T12:01:05.000Z", }); - expect(settleSessionsWithOutcome).not.toHaveBeenCalled(); + expect(settleSessionsReportingAborts).not.toHaveBeenCalled(); expect(getSessionLifecycleSettings(db as any).autoSettleLaneSessionsOnPrMerge).toBe(false); expect(getPrMergeAutoSettlementState(db as any)).toEqual({ enabledSince: null, @@ -1238,9 +1278,9 @@ describe("prMergeAutoSettlementService", () => { }) { const db = createMemoryDb(); const settledSessionIds = new Set(); - const settleSessionsWithOutcome = vi.fn((ids: string[]) => { + const settleSessionsReportingAborts = vi.fn((ids: string[]) => { ids.forEach((id) => settledSessionIds.add(id)); - return ids; + return { settled: ids, aborted: [] }; }); const rowFor = (session: { id: string; toolType: string; laneId?: string }) => ({ laneId: "lane-1", @@ -1260,15 +1300,15 @@ describe("prMergeAutoSettlementService", () => { list: vi.fn(() => overrides.sessions .filter((session) => !(overrides.omitFromListing ?? []).includes(session.id)) .map(rowFor)), - settleSessionsWithOutcome, + settleSessionsReportingAborts, } as any, emitEvent: vi.fn(), }); - return { service, settleSessionsWithOutcome }; + return { service, settleSessionsReportingAborts }; } it("does not settle sessions another open PR in the lane claims", async () => { - const { service, settleSessionsWithOutcome } = createLaneSweepService({ + const { service, settleSessionsReportingAborts } = createLaneSweepService({ sessions: [ { laneId: "lane-1", id: "chat-merged-work", toolType: "codex-chat" }, { laneId: "lane-1", id: "chat-other-pr", toolType: "codex-chat" }, @@ -1297,13 +1337,11 @@ describe("prMergeAutoSettlementService", () => { polledAt: "2026-03-24T12:01:05.000Z", }); - expect(settleSessionsWithOutcome).toHaveBeenCalledWith( + expect(settleSessionsReportingAborts).toHaveBeenCalledWith( ["chat-merged-work"], - "PR #101 merged", - "2026-03-24T12:01:05.000Z", - "pr_merge", + { outcome: "PR #101 merged", settledAt: "2026-03-24T12:01:05.000Z", source: "pr_merge" }, ); - expect(settleSessionsWithOutcome).not.toHaveBeenCalledWith( + expect(settleSessionsReportingAborts).not.toHaveBeenCalledWith( ["chat-other-pr"], expect.anything(), expect.anything(), @@ -1312,7 +1350,7 @@ describe("prMergeAutoSettlementService", () => { }); it("settles nothing on an unlinked merge while another PR in the lane is still live", async () => { - const { service, settleSessionsWithOutcome } = createLaneSweepService({ + const { service, settleSessionsReportingAborts } = createLaneSweepService({ sessions: [{ laneId: "lane-1", id: "chat-ambiguous", toolType: "codex-chat" }], }); const unlinkedPr = createSummary({ id: "pr-unlinked", state: "open" }); @@ -1331,13 +1369,13 @@ describe("prMergeAutoSettlementService", () => { }); // Ownership is genuinely ambiguous: the open PR's own merge files the lane. - expect(settleSessionsWithOutcome).not.toHaveBeenCalled(); + expect(settleSessionsReportingAborts).not.toHaveBeenCalled(); }); it("settles declared sessions beyond the lane listing limit", async () => { // The lane listing is paged. A session the PR explicitly named must not be // dropped just because a long-lived lane pushed it past that page. - const { service, settleSessionsWithOutcome } = createLaneSweepService({ + const { service, settleSessionsReportingAborts } = createLaneSweepService({ sessions: [{ laneId: "lane-1", id: "chat-past-page", toolType: "codex-chat" }], omitFromListing: ["chat-past-page"], }); @@ -1349,16 +1387,14 @@ describe("prMergeAutoSettlementService", () => { polledAt: "2026-03-24T12:01:05.000Z", }); - expect(settleSessionsWithOutcome).toHaveBeenCalledWith( + expect(settleSessionsReportingAborts).toHaveBeenCalledWith( ["chat-past-page"], - "PR #101 merged", - "2026-03-24T12:01:05.000Z", - "pr_merge", + { outcome: "PR #101 merged", settledAt: "2026-03-24T12:01:05.000Z", source: "pr_merge" }, ); }); it("still settles exactly the declared sessions when a PR links its chats", async () => { - const { service, settleSessionsWithOutcome } = createLaneSweepService({ + const { service, settleSessionsReportingAborts } = createLaneSweepService({ sessions: [ { laneId: "lane-1", id: "chat-linked", toolType: "codex-chat" }, { laneId: "lane-1", id: "chat-unrelated", toolType: "codex-chat" }, @@ -1380,12 +1416,10 @@ describe("prMergeAutoSettlementService", () => { }); // A declaration is explicit, so a live sibling PR does not suppress it. - expect(settleSessionsWithOutcome).toHaveBeenCalledTimes(1); - expect(settleSessionsWithOutcome).toHaveBeenCalledWith( + expect(settleSessionsReportingAborts).toHaveBeenCalledTimes(1); + expect(settleSessionsReportingAborts).toHaveBeenCalledWith( ["chat-linked"], - "PR #101 merged", - "2026-03-24T12:01:05.000Z", - "pr_merge", + { outcome: "PR #101 merged", settledAt: "2026-03-24T12:01:05.000Z", source: "pr_merge" }, ); }); }); diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index cd6ebf8c3..08647afa4 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -62,7 +62,7 @@ function resolveMergeSettlementScope(pr: PrSummary, snapshot: PrSummary[]): Merg export function createPrMergeAutoSettlementService(args: { db: Pick; - sessionService: Pick, "get" | "list" | "settleSessionsWithOutcome">; + sessionService: Pick, "get" | "list" | "settleSessionsReportingAborts">; emitEvent: (event: PrEventPayload) => void; }) { /** @@ -170,6 +170,7 @@ export function createPrMergeAutoSettlementService(args: { ); const settledSessionIds: string[] = []; + const abortedByActivity: string[] = []; for (const session of rows) { const currentSettings = getSessionLifecycleSettings(args.db); const currentState = getPrMergeAutoSettlementState(args.db); @@ -185,15 +186,25 @@ export function createPrMergeAutoSettlementService(args: { // session even when it still owns scheduled work, a background task, // or another normal settlement blocker. Real activity can unsettle it // again, while handledPrIds prevents this PR from filing it twice. - settledSessionIds.push(...args.sessionService.settleSessionsWithOutcome( - [session.id], - `PR #${pr.githubPrNumber} merged`, - polledAt, - "pr_merge", - )); + const settleResult = args.sessionService.settleSessionsReportingAborts([session.id], { + outcome: `PR #${pr.githubPrNumber} merged`, + settledAt: polledAt, + source: "pr_merge", + }); + settledSessionIds.push(...settleResult.settled); + if (settleResult.aborted.length) { + // The session became active while the settle was in flight. Leaving + // the PR unhandled is the point: a later pass retries, instead of this + // merge being consumed by a settle that never landed. + abortedByActivity.push(...settleResult.aborted.map((entry) => entry.sessionId)); + } } const finalSettings = getSessionLifecycleSettings(args.db); + // An abandoned settle must not consume the merge. `handledPrIds` is the + // only thing that would stop a later pass from retrying, and the whole + // reason the outcome is typed is so this branch can exist. + const abandonedThisPr = abortedByActivity.length > 0; const finalState = getPrMergeAutoSettlementState(args.db); // Mark this PR handled even when its session had background work, and // even when the scope came back `ambiguous` and nothing was filed at all: @@ -201,7 +212,8 @@ export function createPrMergeAutoSettlementService(args: { // and a later user reactivation belongs to a new lifecycle rather than to // this already-consumed merge. if ( - finalSettings.autoSettleLaneSessionsOnPrMerge + !abandonedThisPr + && finalSettings.autoSettleLaneSessionsOnPrMerge && finalState?.enabledSince && !finalState.handledPrIds.includes(pr.id) && isMergeAtOrAfter(pr.mergedAt, finalState.enabledSince) diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index b5901ac7d..9ef92d472 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -1,6 +1,7 @@ import fs from "node:fs"; import type { AdeDb } from "../state/kvDb"; import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; +import type { SettleAbortedSession, SettleSessionsOutcome } from "./settlingStateRegistry"; import type { ClaudeSessionPointer, SessionAttentionSource, @@ -365,7 +366,21 @@ function normalizeSessionIds(sessionIds: string[]): string[] { )); } -export function createSessionService({ db }: { db: AdeDb }) { +export function createSessionService({ + db, + runSettleTeardown, +}: { + db: AdeDb; + /** + * Stop the session's background work. Injected rather than per-call: teardown + * is a service capability, not something a caller decides. + * + * Step 2 ships with this absent — the settling window, the abort rule, and the + * 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) => void; +}) { const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); @@ -718,6 +733,55 @@ export function createSessionService({ db }: { db: AdeDb }) { 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 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. + */ + const settleManyWithTeardown = ( + sessionIds: string[], + options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, + ): SettleSessionsOutcome => { + const ids = normalizeSessionIds(sessionIds); + const settled: string[] = []; + const aborted: SettleAbortedSession[] = []; + + for (const id of ids) { + 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. + if (begin.kind === "joined") continue; + try { + runSettleTeardown?.(id); + + const abortedBy = settleLifecycle.settling.abortedBy(id); + if (abortedBy) { + aborted.push({ sessionId: id, reason: abortedBy }); + continue; + } + // 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; + } + settled.push(...settleMany([id], options)); + } finally { + settleLifecycle.settling.end(id); + } + } + + return { settled, aborted }; + }; + return { list, @@ -1307,7 +1371,7 @@ export function createSessionService({ db }: { db: AdeDb }) { return; } writeSettleLifecycle({ - intent: { kind: "clearOnActivity" }, + intent: { kind: "clearOnActivity", cause: "mechanical" }, extraSet: { last_output_preview: preview, last_output_at: now }, sessionIds: [sessionId], }); @@ -1333,7 +1397,7 @@ export function createSessionService({ db }: { db: AdeDb }) { return; } writeSettleLifecycle({ - intent: { kind: "clearOnActivity" }, + intent: { kind: "clearOnActivity", cause: "mechanical" }, extraSet: { last_output_at: at }, sessionIds: [sessionId], }); @@ -1511,7 +1575,28 @@ export function createSessionService({ db }: { db: AdeDb }) { }, settleSessions(sessionIds: string[]): string[] { - return settleMany(sessionIds); + return settleManyWithTeardown(sessionIds).settled; + }, + + /** + * 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. + */ + settleSessionsReportingAborts( + sessionIds: string[], + options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, + ): SettleSessionsOutcome { + return settleManyWithTeardown(sessionIds, options); + }, + + /** Sessions currently mid-settle, for the visible `Settling…` state. */ + settlingSessionIds(): string[] { + return settleLifecycle.settling.settlingSessionIds(); }, settleSessionsWithOutcome( @@ -1520,7 +1605,7 @@ export function createSessionService({ db }: { db: AdeDb }) { settledAt: string = new Date().toISOString(), source: SessionSettleSource = "user", ): string[] { - return settleMany(sessionIds, { outcome, settledAt, source }); + return settleManyWithTeardown(sessionIds, { outcome, settledAt, source }).settled; }, unsettleSessions(sessionIds: string[]): void { @@ -1677,7 +1762,7 @@ export function createSessionService({ db }: { db: AdeDb }) { ): boolean { return mutateSessionMeta(sessionId, (id) => { writeSettleLifecycle({ - intent: { kind: "clearOnActivity" }, + intent: { kind: "clearOnActivity", cause: "attention_requested" }, extraSet: { attention_requested_at: new Date().toISOString(), attention_message: normalizeOptionalText(message, 500), @@ -1706,7 +1791,7 @@ export function createSessionService({ db }: { db: AdeDb }) { // settled/failed mutually exclusive at write time, so every surface's // precedence order agrees by construction. writeSettleLifecycle({ - intent: { kind: "clearOnActivity" }, + intent: { kind: "clearOnActivity", cause: "turn_failed" }, extraSet: { last_turn_failed_at: failedAt }, sessionIds: [id], }); @@ -1731,7 +1816,7 @@ export function createSessionService({ db }: { db: AdeDb }) { clearTurnStartMarkers(sessionId: string): boolean { const changed = mutateSessionMeta(sessionId, (id) => { writeSettleLifecycle({ - intent: { kind: "clearOnActivity" }, + intent: { kind: "clearOnActivity", cause: "turn_start" }, extraSet: { last_turn_failed_at: null, attention_requested_at: null, diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts index d51c8c60a..c0e47d06c 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts @@ -1,4 +1,5 @@ import type { AdeDb, SqlValue } from "../state/kvDb"; +import { SettlingStateRegistry, type SettleAbortReason } from "./settlingStateRegistry"; import type { SessionSettleOverride, SessionSettleSource } from "../../../shared/types/sessions"; /** @@ -12,8 +13,17 @@ import type { SessionSettleOverride, SessionSettleSource } from "../../../shared /** What a settle-lifecycle mutation is asking for. */ export type SettleLifecycleIntent = | { kind: "settle"; settledAt: string; source: SessionSettleSource } - /** Real activity: drop the whole declaration, keep-active pin included. */ - | { kind: "clearOnActivity" } + /** + * Real activity: drop the whole declaration, keep-active pin included. + * + * `cause` is what makes teardown possible at all. Stopping a process EMITS + * OUTPUT, so C4 fires *because* teardown is running — if that cleared the + * settle or tripped abort, every real teardown would self-abort and a settle + * could never land. Mechanical exhaust is therefore swallowed during the + * settling window; a human decision aborts it. Outside the window both behave + * identically, exactly as before. + */ + | { kind: "clearOnActivity"; cause: SettleClearCause } /** Declared unsettle: drops a `'settled'` pin but preserves `'active'`. */ | { kind: "unsettleDeclared" } | { kind: "override"; value: SessionSettleOverride | null; source: SessionSettleSource }; @@ -23,6 +33,16 @@ export type SettleLifecycleIntent = * routing a tuple column through `extraSet` is a compile error rather than * something only the source-scan test would catch. */ +/** + * Who asked for the clear. + * + * - `mechanical` — C4 `setLastOutputPreview`, C5 `touchSessionActivity`. Output + * and activity bookkeeping, largely produced by the teardown itself. + * - `turn_start` / `turn_failed` / `attention_requested` — C3, C6, C7. Durable + * signals of intent that outrank a settle in canonical precedence. + */ +export type SettleClearCause = "mechanical" | SettleAbortReason; + export type SettleExtraColumn = | "status_note" | "attention_requested_at" @@ -41,6 +61,8 @@ export type SettleLifecycleWriter = { guard?: string; }) => void; readRevision: (sessionId: string) => number; + /** The in-flight settle windows. See `settlingStateRegistry.ts`. */ + settling: SettlingStateRegistry; /** Drop a deleted session's token so the local table and map do not grow forever. */ forget: (sessionId: string) => void; }; @@ -180,6 +202,28 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { * one (its "still unsettled" check, which another ADE process could otherwise * invalidate between the select and the update). */ + const settling = new SettlingStateRegistry(); + + /** + * What the settling window does to this write. + * + * "Swallowed" means precisely three things, and all three matter: the tuple is + * NOT cleared, the revision is NOT bumped, and abort is NOT tripped. The + * caller's own columns still land, so the row keeps showing live output while + * it settles — which is what a visible `Settling…` state should look like. + */ + const settlingDisposition = ( + intent: SettleLifecycleIntent, + sessionIds: readonly string[], + ): "normal" | "swallow" => { + if (intent.kind !== "clearOnActivity") return "normal"; + const active = sessionIds.filter((id) => settling.isSettling(id)); + if (!active.length) return "normal"; + if (intent.cause === "mechanical") return "swallow"; + for (const id of active) settling.abort(id, intent.cause); + return "normal"; + }; + const writeSettleLifecycle = (args: { intent: SettleLifecycleIntent; sessionIds: readonly string[]; @@ -188,6 +232,20 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { }): void => { const ids = args.sessionIds.map((id) => id.trim()).filter(Boolean); if (!ids.length) return; + + if (settlingDisposition(args.intent, ids) === "swallow") { + // Mechanical exhaust during the settling window. Land the caller's own + // columns and nothing else — no tuple clear, no revision bump, no abort. + const extraEntries = Object.entries(args.extraSet ?? {}) as Array<[SettleExtraColumn, SqlValue]>; + if (!extraEntries.length) return; + db.run( + `update terminal_sessions set ${extraEntries.map(([column]) => `${column} = ?`).join(", ")}` + + ` where id in (${ids.map(() => "?").join(", ")})`, + [...extraEntries.map(([, value]) => value), ...ids], + ); + return; + } + const tuple = settleTupleAssignment(args.intent); const extraEntries = Object.entries(args.extraSet ?? {}) as Array<[SettleExtraColumn, SqlValue]>; const setClauses = [...extraEntries.map(([column]) => `${column} = ?`), tuple.sql]; @@ -227,6 +285,7 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { return { write: writeSettleLifecycle, readRevision: readLifecycleRevision, + settling, forget: (sessionId: string) => { const trimmed = sessionId.trim(); if (!trimmed) return; diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts new file mode 100644 index 000000000..fb958e2cc --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -0,0 +1,294 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { openKvDb } from "../state/kvDb"; +import { createSessionService } from "./sessionService"; + +/** + * The race matrix from the settle-teardown design (§2), tested directly against + * the lifecycle revision and the settling window — with teardown still a NO-OP. + * + * 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 + * that cost. Here the teardown callback is a seam we drive by hand: whatever it + * does is what a real provider stop would have been doing when the race landed. + */ + +function createLogger() { + return { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} } as const; +} + +function makeProjectRoot(): string { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "ade-settle-race-")); + fs.mkdirSync(path.join(root, ".ade", "artifacts"), { recursive: true }); + return root; +} + +function insertProjectGraph(db: Awaited>) { + const now = "2026-08-11T00:00:00.000Z"; + db.run( + `insert into projects(id, root_path, display_name, default_base_ref, created_at, last_opened_at) + values (?, ?, ?, ?, ?, ?)`, + ["project-1", "/repo/ade", "ADE", "main", now, now], + ); + db.run( + `insert into lanes( + id, project_id, name, description, lane_type, base_ref, branch_ref, worktree_path, attached_root_path, + is_edit_protected, parent_lane_id, color, icon, tags_json, folder, status, created_at, archived_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + [ + "lane-1", "project-1", "Lane 1", null, "worktree", "main", "ade/lane-1", + "/repo/ade/.ade/worktrees/lane-1", null, 0, null, null, null, "[]", null, "active", now, null, + ], + ); +} + +describe("settle race matrix (teardown is a no-op)", () => { + const disposers: Array<() => Promise> = []; + afterEach(async () => { + while (disposers.length) await disposers.pop()?.(); + }); + + async function fixture() { + const projectRoot = makeProjectRoot(); + const db = await openKvDb(path.join(projectRoot, ".ade", "ade.db"), createLogger() as any); + disposers.push(async () => db.close()); + 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 = () => {}; + const service = createSessionService({ + db, + runSettleTeardown: (sessionId) => teardown(sessionId), + }); + const setTeardown = (fn: (sessionId: string) => void) => { + teardown = fn; + }; + const create = (id: string) => + service.create({ + sessionId: id, + laneId: "lane-1", + ptyId: null, + tracked: true, + title: "Chat", + startedAt: "2026-08-11T00:01:00.000Z", + transcriptPath: `/tmp/${id}.log`, + toolType: "codex-chat", + }); + create("session-1"); + return { db, service, create, setTeardown }; + } + + /** + * R1 — settle vs turn start. The worst case: the user is actively working and + * the row goes quiet. C3 fires during teardown; the settle must be abandoned, + * not applied afterwards. + */ + it("R1: a turn starting during teardown abandons the settle", async () => { + const { service, setTeardown } = await fixture(); + service.settleSessions(["session-1"]); + service.unsettleSession("session-1"); + + setTeardown(() => { + service.clearTurnStartMarkers("session-1"); + }); + const outcome = service.settleSessionsReportingAborts(["session-1"]); + + expect(outcome.settled).toEqual([]); + expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "turn_start" }]); + expect(service.get("session-1")?.settledAt).toBeNull(); + }); + + /** + * R2 — settle abandoned after teardown ran. Teardown is a no-op here, so what + * this pins is the reporting half: the caller is told, and never sees silent + * success. #1059 returned success and that is what a caller built on. + */ + it("R2: an abandoned settle reports the reason instead of silent success", async () => { + const { service, setTeardown } = await fixture(); + let teardownRan = false; + + setTeardown(() => { + teardownRan = true; + service.requestAttention("session-1", "need you"); + }); + const outcome = service.settleSessionsReportingAborts(["session-1"]); + + expect(teardownRan).toBe(true); + expect(outcome.settled).toEqual([]); + expect(outcome.aborted[0]).toMatchObject({ sessionId: "session-1", reason: "attention_requested" }); + }); + + /** R3 — work drains on its own during teardown. Benign; the settle lands. */ + it("R3: background work finishing during teardown does not block the settle", async () => { + const { service, setTeardown } = await fixture(); + + setTeardown(() => { + // A stop against an already-finished task is a no-op, and nothing touches + // the settle tuple. + }); + const outcome = service.settleSessionsReportingAborts(["session-1"]); + + expect(outcome.aborted).toEqual([]); + expect(outcome.settled).toEqual(["session-1"]); + expect(service.get("session-1")?.settledAt).toBeTruthy(); + }); + + /** + * R4 — concurrent settle sources. PR-merge auto-settle racing a user settle + * must not run two teardowns against one session. + */ + it("R4: a second settle joins the in-flight one instead of tearing down twice", async () => { + const { service, setTeardown } = await fixture(); + let teardowns = 0; + + let inner: ReturnType | null = null; + setTeardown(() => { + 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"]); + }); + + const outcome = service.settleSessionsReportingAborts(["session-1"]); + + expect(teardowns, "the joined settle must not start its own teardown").toBe(1); + expect(outcome.settled).toEqual(["session-1"]); + // The joiner reports nothing: the owner reports the outcome, and + // double-counting would make a bulk caller see one session twice. + expect(inner).toMatchObject({ settled: [], aborted: [] }); + }); + + /** + * R6 — output during teardown, the high-frequency shape of R1. This is the + * case that would make the feature dead on arrival: stopping a process emits + * output, so C4/C5 fire BECAUSE teardown is running. They must be swallowed on + * all three axes or no settle could ever land. + */ + it("R6: output during teardown is swallowed on all three axes", async () => { + const { service, setTeardown } = await fixture(); + const revisionBefore = service.getSettleLifecycleRevision("session-1"); + + setTeardown(() => { + service.setLastOutputPreview("session-1", "final chunk", { clearSettled: true }); + service.touchSessionActivity("session-1", "2026-08-11T00:09:00.000Z"); + }); + const outcome = service.settleSessionsReportingAborts(["session-1"]); + + // 1. did not abort + expect(outcome.aborted).toEqual([]); + expect(outcome.settled).toEqual(["session-1"]); + // 2. did not bump the revision (only the settle itself did) + expect(service.getSettleLifecycleRevision("session-1")).toBe(revisionBefore + 1); + // 3. did not clear the tuple — and the preview still landed, so the row + // keeps showing live output while it settles. + const row = service.get("session-1"); + expect(row?.settledAt).toBeTruthy(); + expect(row?.lastOutputPreview).toBe("final chunk"); + }); + + it("R6b: the same output OUTSIDE the settling window clears normally", async () => { + const { service } = await fixture(); + service.settleSessions(["session-1"]); + expect(service.get("session-1")?.settledAt).toBeTruthy(); + + service.setLastOutputPreview("session-1", "later output", { clearSettled: true }); + + // The swallow is scoped to the window and nothing else. + expect(service.get("session-1")?.settledAt).toBeNull(); + }); + + /** + * R7 — a peer's settle bypassing the writer. + * + * Not in the original matrix: it surfaced while implementing step 1. A paired + * DESKTOP peer's settle arrives through `crsql_changes` and never passes the + * chokepoint, so this host's revision does not move for it. Step 0 + * deliberately left desktop peers replicating, so this is reachable in + * production. + * + * This test exists to show the BLAST RADIUS before teardown exists, per the + * coordinator's step-3 review scope — it asserts today's real behavior, not + * the behavior we want. + */ + 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"); + + setTeardown(() => { + // Exactly what a replicated peer settle looks like locally: the row's + // settle tuple changes without the writer being involved. + db.run( + "update terminal_sessions set settled_at = ?, settle_source = ? where id = ?", + ["2026-08-11T00:07:00.000Z", "user", "session-1"], + ); + }); + const outcome = service.settleSessionsReportingAborts(["session-1"]); + + // Observed behaviour, not the behaviour we would have guessed. + // + // The revision does NOT move: the write bypassed the chokepoint, so the + // guard is blind to it exactly as §3a says. + expect(service.getSettleLifecycleRevision("session-1")).toBe(revisionBefore); + + // And `settleMany`'s own guard — `settled_at is null or settle_override is + // not null` — then finds nothing to do, so the settle silently no-ops. + // The id appears in NEITHER list. + expect(outcome.settled).toEqual([]); + expect(outcome.aborted).toEqual([]); + + // The peer's value stands. + expect(service.get("session-1")?.settledAt).toBe("2026-08-11T00:07:00.000Z"); + }); + + /** + * The blast radius of R7, stated as a contract so step 3 has to confront it. + * + * For a peer SETTLE the data outcome is benign — both parties wanted the row + * settled — but the REPORTING is not: the caller asked to settle a session, + * and got back an id that is neither settled nor aborted. That is precisely + * the silent absence the typed outcome exists to eliminate, reappearing + * through a path the writer never sees. + * + * A peer UNSETTLE is the dangerous shape: it moves the tuple the other way + * without bumping the revision, so a revision-conditional apply in step 3 + * would not notice it either. + */ + it("R7b: a bypassing peer write leaves the caller unable to tell what happened", async () => { + const { db, service, setTeardown } = await fixture(); + + setTeardown(() => { + db.run( + "update terminal_sessions set settled_at = ?, settle_source = ? where id = ?", + ["2026-08-11T00:07:00.000Z", "user", "session-1"], + ); + }); + const outcome = service.settleSessionsReportingAborts(["session-1"]); + + const accountedFor = [ + ...outcome.settled, + ...outcome.aborted.map((entry) => entry.sessionId), + ]; + expect( + accountedFor, + "step 3 must account for an id the writer never saw change", + ).toEqual([]); + }); + + /** 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(); + expect(service.settlingSessionIds()).toEqual([]); + + setTeardown(() => { + expect(service.settlingSessionIds()).toEqual(["session-1"]); + }); + service.settleSessionsReportingAborts(["session-1"]); + + // Always ended, even on the abandoned path — a leaked window would make the + // session permanently unsettleable. + expect(service.settlingSessionIds()).toEqual([]); + }); +}); diff --git a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts new file mode 100644 index 000000000..335f921c8 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts @@ -0,0 +1,96 @@ +/** + * The `settling` state: the window between deciding to settle a session and the + * settle landing. + * + * The lifecycle revision (step 1) detects that the world moved. It does not say + * what to do about work already stopped when it moves — that is R2, the one race + * where both outcomes are bad. This registry is the other half: a short-lived, + * visible, exclusive, abortable marker that teardown runs inside. + * + * Deliberately IN MEMORY. The design requires a `settling` row found at startup + * to resolve to not-settled, because teardown is not resumable across a restart + * and pretending otherwise would resurrect the "orphaned work is live work" + * mistake the liveness half avoids. In-memory gives that for free: a restart has + * no settling sessions, which is exactly the required answer. It also keeps the + * marker off `terminal_sessions`, which is a CRR — a replicated "settling" flag + * would be a lie on every other device the moment this host died. + */ + +/** Why a settle was abandoned. Only ever set by a human-decision clearer. */ +export type SettleAbortReason = "turn_start" | "turn_failed" | "attention_requested"; + +/** Why a settle was abandoned, as reported to callers. */ +export type SettleAbortedReason = SettleAbortReason | "lifecycle_changed"; + +export type SettleAbortedSession = { + sessionId: string; + reason: SettleAbortedReason; +}; + +export type SettleSessionsOutcome = { + settled: string[]; + /** Explicitly abandoned — never silently absent. */ + aborted: SettleAbortedSession[]; +}; + +export type SettlingEntry = { + /** The revision the settle decision was taken against. */ + startedAtRevision: number; + abortedBy: SettleAbortReason | null; +}; + +export type BeginSettlingResult = + /** This caller owns the window and must end it. */ + | { kind: "started" } + /** + * Another settle for this session is already in flight. The caller JOINS it + * rather than starting a second teardown — closing R4, where two teardowns + * race one session and the second reports against a partially-drained roster. + */ + | { kind: "joined" }; + +export class SettlingStateRegistry { + private readonly entries = new Map(); + + begin(sessionId: string, startedAtRevision: number): BeginSettlingResult { + if (this.entries.has(sessionId)) return { kind: "joined" }; + this.entries.set(sessionId, { startedAtRevision, abortedBy: null }); + return { kind: "started" }; + } + + isSettling(sessionId: string): boolean { + return this.entries.has(sessionId); + } + + /** Every session currently mid-settle, for the "Settling…" projection. */ + settlingSessionIds(): string[] { + return Array.from(this.entries.keys()); + } + + /** + * Trip the abort. Idempotent, and first-reason-wins so the surfaced cause is + * the one that actually interrupted the settle. + */ + abort(sessionId: string, reason: SettleAbortReason): void { + const entry = this.entries.get(sessionId); + if (!entry || entry.abortedBy) return; + entry.abortedBy = reason; + } + + abortedBy(sessionId: string): SettleAbortReason | null { + return this.entries.get(sessionId)?.abortedBy ?? null; + } + + startedAtRevision(sessionId: string): number | null { + return this.entries.get(sessionId)?.startedAtRevision ?? null; + } + + end(sessionId: string): void { + this.entries.delete(sessionId); + } + + /** Project or host teardown: nothing in flight survives it. */ + clear(): void { + this.entries.clear(); + } +} diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index c1d831d35..90167ec98 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -4,7 +4,7 @@ to implement; step 3 (attaching real teardown) waits until 1 and 2 are merged and the race-matrix tests have been seen to pass. -**Steps 0 and 1 are implemented.** Step 0's host-side half is "Host enforcement +**Steps 0, 1 and 2 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. @@ -106,6 +106,14 @@ bounds a single call, and a fleet is serial — seconds, not milliseconds). | R4 | **Concurrent settle sources** — PR-merge auto-settle (W1) races a user settle (W2) or the CTO tool (W2) | Two teardowns run against one session; the second sees a partially-drained roster and reports differently. `settled_at = coalesce(settled_at, ?)` makes the *write* idempotent, but the teardown is not | medium | | R5 | **Settle vs. unconfirmed stop** — a provider stop times out or is unavailable | The row settles over work that may still be running. Keeping the task live in `liveBackgroundTaskIds` does not help: `settled` outranks it in the phase | **high** — this is the original bug, unfixed | | R6 | **Settle vs. C4 output chunk** — any output during `T` | C4 clears the settle mid-teardown; the settle write re-lands after | same shape as R1, at much higher frequency | +| R7 | **Settle vs. a peer write that bypasses the writer** — a paired desktop's settle arrives via `crsql_changes` | The revision does not move, so the guard is blind; `settleMany`'s own guard then finds nothing to do and the settle silently no-ops. The id comes back in **neither** the settled nor the aborted list | medium — benign for a peer *settle*, unresolved for a peer *unsettle*; step 3 must account for it | + +R7 is not from the original matrix — it surfaced while implementing step 1 and is +tested in `settleRaceMatrix.test.ts` so its blast radius is visible before +teardown exists. The reporting gap is the part that matters: a caller asked to +settle a session and got an id that is neither settled nor abandoned, which is +exactly the silent absence the typed outcome exists to remove, reappearing +through a path the writer never sees. **R1/R2/R6 share one root:** the settle decision is made at time `t₀`, the write lands at `t₀ + T`, and the world is free to change in between with no way to @@ -233,6 +241,20 @@ output is not. For a plain (non-chat) terminal with no turn concept, the settlin window is the whole exposure; it is bounded by `T` and by the fact that settling is visible while it runs. +**As implemented (step 2).** The window lives in `settlingStateRegistry.ts`, in +memory. That is not a shortcut: the design requires a `settling` row found at +startup to resolve to not-settled, and in-memory gives exactly that with no +recovery code to get wrong. It also keeps the marker off `terminal_sessions`, +which is a CRR — a replicated "settling" flag would be a lie on every other +device the moment this host died. + +The clearer split is carried by a `cause` on the clear intent +(`mechanical` for C4/C5, `turn_start` / `turn_failed` / `attention_requested` +for C3/C6/C7), so the writer decides the disposition rather than each call site +remembering to. Teardown is injected at service construction +(`createSessionService({ runSettleTeardown })`), absent in step 2 — which is what +lets the race matrix drive the seam by hand before any work exists to lose. + ### 3c. The rule for a turn arriving mid-teardown **An accepted user turn ABORTS the settle and the teardown. Never the reverse.** @@ -472,10 +494,11 @@ three, and that is why it produced a defect every round. It is pure refactor with a testable invariant: no `settled_at` mutation outside one function, and every mutation bumps the revision. The revision goes in a local-only table (§3c-ii). -2. Add the `settling` state, the per-clearer behavior table (3b-i), and the - abort rule (3c) — still with a **no-op teardown** — and test the race matrix - directly against the revision. The C4/C5 swallow is the case to test hardest: - it is what makes a real teardown able to finish at all. +2. **Landed.** The `settling` state, the per-clearer behavior table (3b-i), and + the abort rule (3c), with a **no-op teardown**. The race matrix is tested + 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. From 16533ed5320e32df1d24bc3d263e595969afacb2 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 00:21:12 -0400 Subject: [PATCH 02/10] fix: swallow per session, not per batch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `settlingDisposition` returned one verdict for a whole `sessionIds` array, so a mixed batch with a mechanical cause swallowed ALL of it — a session with no settle window would have had its own output silently stop clearing its settle. Unreachable today, because every mechanical caller is single-session. Worth fixing anyway: nothing in the signature warns the caller who first passes two ids, and the failure is invisible rather than loud. Now partitioned — the settling ids get their columns written and nothing else, the rest clear normally in the same call. Pinned by a mixed-batch test. --- .../sessions/settleLifecycleWriter.ts | 76 +++++++++++++------ .../sessions/settleRaceMatrix.test.ts | 31 ++++++++ 2 files changed, 84 insertions(+), 23 deletions(-) diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts index c0e47d06c..5ef667d7b 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts @@ -212,16 +212,55 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { * caller's own columns still land, so the row keeps showing live output while * it settles — which is what a visible `Settling…` state should look like. */ - const settlingDisposition = ( + /** + * Land ONLY the caller's own columns, for a session whose settle window is + * open and whose write is mechanical exhaust. + * + * "Swallowed" means precisely three things and all three matter: the tuple is + * not cleared, the revision is not bumped, and abort is not tripped. The + * preview and activity columns still update, so the row keeps showing live + * output while it settles. + */ + const writeExtraColumnsOnly = ( + sessionIds: readonly string[], + extraSet: Partial> | undefined, + ): void => { + const extraEntries = Object.entries(extraSet ?? {}) as Array<[SettleExtraColumn, SqlValue]>; + if (!extraEntries.length || !sessionIds.length) return; + db.run( + `update terminal_sessions set ${extraEntries.map(([column]) => `${column} = ?`).join(", ")}` + + ` where id in (${sessionIds.map(() => "?").join(", ")})`, + [...extraEntries.map(([, value]) => value), ...sessionIds], + ); + }; + + /** + * Split a clear into the sessions whose window swallows it and the sessions + * that clear normally. + * + * Per session, not per batch. Every mechanical caller is single-session today, + * so a mixed batch is unreachable — but deciding one disposition for a whole + * array would silently stop a NON-settling session's own output from clearing + * its settle, and nothing in the signature would warn the caller who first + * passes two ids. + */ + const partitionForSettlingWindow = ( intent: SettleLifecycleIntent, sessionIds: readonly string[], - ): "normal" | "swallow" => { - if (intent.kind !== "clearOnActivity") return "normal"; - const active = sessionIds.filter((id) => settling.isSettling(id)); - if (!active.length) return "normal"; - if (intent.cause === "mechanical") return "swallow"; - for (const id of active) settling.abort(id, intent.cause); - return "normal"; + ): { swallowed: string[]; normal: string[] } => { + if (intent.kind !== "clearOnActivity") return { swallowed: [], normal: [...sessionIds] }; + const settlingIds = sessionIds.filter((id) => settling.isSettling(id)); + if (!settlingIds.length) return { swallowed: [], normal: [...sessionIds] }; + if (intent.cause === "mechanical") { + return { + swallowed: settlingIds, + normal: sessionIds.filter((id) => !settling.isSettling(id)), + }; + } + // A human decision: trip abort for the settling sessions, then let every + // session clear normally — the clear itself is not suppressed. + for (const id of settlingIds) settling.abort(id, intent.cause); + return { swallowed: [], normal: [...sessionIds] }; }; const writeSettleLifecycle = (args: { @@ -233,31 +272,22 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { const ids = args.sessionIds.map((id) => id.trim()).filter(Boolean); if (!ids.length) return; - if (settlingDisposition(args.intent, ids) === "swallow") { - // Mechanical exhaust during the settling window. Land the caller's own - // columns and nothing else — no tuple clear, no revision bump, no abort. - const extraEntries = Object.entries(args.extraSet ?? {}) as Array<[SettleExtraColumn, SqlValue]>; - if (!extraEntries.length) return; - db.run( - `update terminal_sessions set ${extraEntries.map(([column]) => `${column} = ?`).join(", ")}` - + ` where id in (${ids.map(() => "?").join(", ")})`, - [...extraEntries.map(([, value]) => value), ...ids], - ); - return; - } + const { swallowed, normal } = partitionForSettlingWindow(args.intent, ids); + writeExtraColumnsOnly(swallowed, args.extraSet); + if (!normal.length) return; const tuple = settleTupleAssignment(args.intent); const extraEntries = Object.entries(args.extraSet ?? {}) as Array<[SettleExtraColumn, SqlValue]>; const setClauses = [...extraEntries.map(([column]) => `${column} = ?`), tuple.sql]; const setParams = [...extraEntries.map(([, value]) => value), ...tuple.params]; - const idPlaceholders = ids.map(() => "?").join(", "); + const idPlaceholders = normal.map(() => "?").join(", "); const where = args.guard ? `${args.guard} and id in (${idPlaceholders})` : `id in (${idPlaceholders})`; const changed = db.runChanged( `update terminal_sessions set ${setClauses.join(", ")} where ${where}`, - [...setParams, ...ids], + [...setParams, ...normal], ); // Only a write that MATCHED A ROW bumps — that is what this gate buys, and // it is what stops a deleted or absent session inserting an orphan token. @@ -278,7 +308,7 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { // support several processes against one database, so a sibling process could // observe the pair mid-flight; that window is microseconds and closing it // needs a transaction helper `AdeDb` does not have. - bumpLifecycleRevisions(ids); + bumpLifecycleRevisions(normal); }; diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index fb958e2cc..e44368fd0 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -277,6 +277,37 @@ describe("settle race matrix (teardown is a no-op)", () => { ).toEqual([]); }); + /** + * The swallow is per session, not per batch. + * + * Unreachable today — every mechanical caller is single-session — but deciding + * one disposition for a whole array would silently stop a NON-settling + * session's own output from clearing its settle, and nothing in the signature + * warns the caller who first passes two ids. + */ + it("swallows only the settling session in a mixed batch", async () => { + const { db, service, create, setTeardown } = await fixture(); + create("session-2"); + service.settleSessions(["session-2"]); + expect(service.get("session-2")?.settledAt).toBeTruthy(); + + setTeardown(() => { + // Drive the writer directly with both ids: session-1's window is open, + // session-2's is not. + db.run("update terminal_sessions set last_output_at = ? where id = ?", + ["2026-08-11T00:09:00.000Z", "session-2"]); + service.setLastOutputPreview("session-2", "other session output", { clearSettled: true }); + service.setLastOutputPreview("session-1", "settling session output", { clearSettled: true }); + }); + const outcome = service.settleSessionsReportingAborts(["session-1"]); + + // The settling session's output was swallowed: it still settled. + expect(outcome.settled).toEqual(["session-1"]); + // The other session's output cleared its settle normally. + expect(service.get("session-2")?.settledAt).toBeNull(); + expect(service.get("session-2")?.lastOutputPreview).toBe("other session output"); + }); + /** 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(); From e7a28ded765c0e15f9e92d9b518a6799d5d4a3a4 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 00:40:37 -0400 Subject: [PATCH 03/10] quality: route the user settle through the window, and stop writing vacuous tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dual-track review of step 2. Both tracks independently found the same two High issues, and both found that my regression test proved nothing. **The singular settle bypassed the window (H).** `settleSession` — the route a USER takes from the row menu, and the "user settle" R4 explicitly names — wrote the tuple directly. It could not join, could not abort, and in step 3 would have settled with no teardown at all. Worse today: it bumps the revision, so landing during a bulk window made the bulk settle report `lifecycle_changed` for a session that actually settled — which iOS would have read as an abort and rolled its overlay back. Now routed through `settleManyWithTeardown`, with its own existence check so the boolean contract stays honest. **The teardown seam silently accepted an async function (H).** TypeScript's void-return rule lets `async (id) => {}` through. It would have returned immediately, the abort and revision checks would have run against a world where nothing had been stopped, and `finally` would have closed the window while the real teardown was still emitting output — which C4/C5 would then no longer swallow, clearing the settle that just landed. That is the "dead on arrival" failure §3b-i exists to prevent, arriving through a type hole. Now a loud runtime refusal, with a test. **My mixed-batch test was vacuous, and so was its replacement.** Both tracks proved the first passed against the pre-fix code. The replacement did too — for a reason worth recording: a multi-id MECHANICAL clear is not expressible through the service API, so any test built from C4/C5 calls filters to one id and proves nothing. Rather than keep defensive code no test can reach, the writer now ASSERTS the constraint that is actually true: `clearOnActivity` is single-session by construction. Also: an abandoned first pass silently lost the PR-merge announcement forever (merged PRs drop out of the watchable set each pass, and `watchedItMerge` gates the toast) — probe-verified, the retry now still announces. A throwing teardown is reported as `teardown_failed` instead of discarding the rest of the batch's accounting. `forget()` closes the settling entry, the third of three maps its own comment claimed to cover. `settleSessionsWithOutcome` had zero production callers and is gone. --- .../src/main/services/prs/prAsync.test.ts | 7 +- .../prs/prMergeAutoSettlementService.ts | 14 +++- .../services/sessions/sessionService.test.ts | 8 +-- .../main/services/sessions/sessionService.ts | 64 ++++++++++------- .../sessions/settleLifecycleWriter.ts | 35 ++++++---- .../sessions/settleRaceMatrix.test.ts | 68 +++++++++++++------ .../sessions/settlingStateRegistry.ts | 2 +- 7 files changed, 127 insertions(+), 71 deletions(-) diff --git a/apps/desktop/src/main/services/prs/prAsync.test.ts b/apps/desktop/src/main/services/prs/prAsync.test.ts index 3293db62e..43415044b 100644 --- a/apps/desktop/src/main/services/prs/prAsync.test.ts +++ b/apps/desktop/src/main/services/prs/prAsync.test.ts @@ -1112,6 +1112,7 @@ describe("prMergeAutoSettlementService", () => { settled: [] as string[], aborted: ids.map((sessionId) => ({ sessionId, reason: "turn_start" })), })); + const emitEvent = vi.fn(); const service = createPrMergeAutoSettlementService({ db: db as any, sessionService: withSessionLookup({ @@ -1121,7 +1122,7 @@ describe("prMergeAutoSettlementService", () => { get: vi.fn(() => null), settleSessionsReportingAborts, }) as any, - emitEvent: vi.fn(), + emitEvent, }); const openPr = createSummary({ state: "open" }); @@ -1146,6 +1147,10 @@ describe("prMergeAutoSettlementService", () => { settleSessionsReportingAborts, "an abandoned settle must not consume the merge", ).toHaveBeenCalledTimes(2); + // ...and the retry must still announce. A merged PR falls out of the + // watchable set every pass, so without keeping it alive the user would get + // the settle and never the toast. + expect(emitEvent, "the retry must still announce the merge").toHaveBeenCalled(); }); it("settles a PR that was already merged when first seen, but announces nothing", async () => { diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index 08647afa4..24bce7bf0 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -129,11 +129,19 @@ export function createPrMergeAutoSettlementService(args: { // successful pass, so a snapshot that returns early does not silently // consume its own evidence. const previouslyWatchedPrIds = new Set(previouslyWatchablePrIds); + // Ids whose merge this pass watched but could not finish filing, because a + // session became active mid-settle. Kept watchable so the retry can still + // announce: `watchedItMerge` is what gates the toast, and a merged PR is + // otherwise dropped from the watchable set at the end of every pass — so + // without this the retry settles the session silently and the user never + // learns their PR merged. + const unfinishedMergePrIds = new Set(); const rememberSnapshot = () => { previouslyWatchablePrIds.clear(); for (const pr of prs) { if (pr.state === "draft" || pr.state === "open") previouslyWatchablePrIds.add(pr.id); } + for (const id of unfinishedMergePrIds) previouslyWatchablePrIds.add(id); }; const settings = getSessionLifecycleSettings(args.db); const state = getPrMergeAutoSettlementState(args.db); @@ -170,7 +178,7 @@ export function createPrMergeAutoSettlementService(args: { ); const settledSessionIds: string[] = []; - const abortedByActivity: string[] = []; + let abandonedThisPr = false; for (const session of rows) { const currentSettings = getSessionLifecycleSettings(args.db); const currentState = getPrMergeAutoSettlementState(args.db); @@ -196,7 +204,7 @@ export function createPrMergeAutoSettlementService(args: { // The session became active while the settle was in flight. Leaving // the PR unhandled is the point: a later pass retries, instead of this // merge being consumed by a settle that never landed. - abortedByActivity.push(...settleResult.aborted.map((entry) => entry.sessionId)); + abandonedThisPr = true; } } @@ -204,7 +212,7 @@ export function createPrMergeAutoSettlementService(args: { // An abandoned settle must not consume the merge. `handledPrIds` is the // only thing that would stop a later pass from retrying, and the whole // reason the outcome is typed is so this branch can exist. - const abandonedThisPr = abortedByActivity.length > 0; + if (abandonedThisPr) unfinishedMergePrIds.add(pr.id); const finalState = getPrMergeAutoSettlementState(args.db); // Mark this PR handled even when its session had background work, and // even when the scope came back `ambiguous` and nothing was filed at all: diff --git a/apps/desktop/src/main/services/sessions/sessionService.test.ts b/apps/desktop/src/main/services/sessions/sessionService.test.ts index 0e759306d..254685a30 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.test.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.test.ts @@ -1356,12 +1356,10 @@ describe("sessionService resume metadata", () => { expect(service.get("session-new")?.settledAt).not.toBeNull(); expect(service.get("session-other")?.settledAt).toBeNull(); - expect(service.settleSessionsWithOutcome( + expect(service.settleSessionsReportingAborts( ["session-settled", "session-other"], - "PR #841 merged", - "2026-03-17T03:00:00.000Z", - "pr_merge", - )).toEqual(["session-other"]); + { outcome: "PR #841 merged", settledAt: "2026-03-17T03:00:00.000Z", source: "pr_merge" }, + ).settled).toEqual(["session-other"]); expect(service.get("session-settled")).toEqual(expect.objectContaining({ settledAt: "2026-03-17T01:00:00.000Z", statusNote: null, diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index 9ef92d472..ff3c7c0b7 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -760,7 +760,20 @@ export function createSessionService({ // The owner will report the outcome; reporting it twice would double-count. if (begin.kind === "joined") continue; try { - runSettleTeardown?.(id); + const teardownResult = runSettleTeardown?.(id) as unknown; + // The seam is synchronous in step 2 and MUST stay so until the settle + // path is made async. An async teardown would return here immediately, + // the checks below would run against a world where nothing had been + // stopped yet, and `finally` would close the window while the real + // teardown was still emitting output — which C4/C5 would then no longer + // swallow, clearing the settle that just landed. TypeScript's void-return + // rule lets an async callback through silently, so this is a loud guard + // rather than a type. + if (teardownResult && typeof (teardownResult as { then?: unknown }).then === "function") { + throw new Error( + "runSettleTeardown returned a promise: the settle path must be made async before teardown can await anything.", + ); + } const abortedBy = settleLifecycle.settling.abortedBy(id); if (abortedBy) { @@ -774,6 +787,12 @@ export function createSessionService({ continue; } settled.push(...settleMany([id], options)); + } catch (error) { + // A throw must not discard the accounting for the rest of the batch, nor + // leave earlier sessions settled with no record of it. Report this id and + // carry on: a bulk caller always gets a full accounting. + aborted.push({ sessionId: id, reason: "teardown_failed" }); + void error; } finally { settleLifecycle.settling.end(id); } @@ -1496,22 +1515,26 @@ export function createSessionService({ sessionId: string, opts: { outcome?: string | null; settledAt?: string; source?: SessionSettleSource } = {}, ): boolean { - const settledAt = normalizeIsoTimestamp(opts.settledAt) ?? new Date().toISOString(); - const outcome = normalizeSessionStatusNote(opts.outcome); - return mutateSessionMeta(sessionId, (id) => { - // An explicit settle also drops a stale keep-active pin — otherwise the - // override would silently veto the settle the user just asked for. - writeSettleLifecycle({ - intent: { kind: "settle", settledAt, source: opts.source ?? "user" }, - extraSet: { - ...(outcome ? { status_note: outcome } : {}), - attention_requested_at: null, - attention_message: null, - attention_source: null, - }, - sessionIds: [id], - }); + // 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; + settleManyWithTeardown([trimmed], { + outcome: normalizeSessionStatusNote(opts.outcome) ?? undefined, + settledAt: opts.settledAt, + source: opts.source, }); + return true; }, /** Clears a declared settle plus any `'settled'` override. */ @@ -1599,15 +1622,6 @@ export function createSessionService({ return settleLifecycle.settling.settlingSessionIds(); }, - settleSessionsWithOutcome( - sessionIds: string[], - outcome: string, - settledAt: string = new Date().toISOString(), - source: SessionSettleSource = "user", - ): string[] { - return settleManyWithTeardown(sessionIds, { outcome, settledAt, source }).settled; - }, - unsettleSessions(sessionIds: string[]): void { const ids = normalizeSessionIds(sessionIds); if (!ids.length) return; diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts index 5ef667d7b..02a0c69ec 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts @@ -238,28 +238,29 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { * Split a clear into the sessions whose window swallows it and the sessions * that clear normally. * - * Per session, not per batch. Every mechanical caller is single-session today, - * so a mixed batch is unreachable — but deciding one disposition for a whole - * array would silently stop a NON-settling session's own output from clearing - * its settle, and nothing in the signature would warn the caller who first - * passes two ids. + * Every `clearOnActivity` caller is single-session — C3-C7 all go through + * `mutateSessionMeta`, which takes one id — so this is asserted rather than + * handled. Deciding one disposition for a whole array would silently stop a + * NON-settling session's own output from clearing its settle, and that is a + * failure nobody would see; a thrown error is the honest alternative to + * defensive code no test can reach. */ const partitionForSettlingWindow = ( intent: SettleLifecycleIntent, sessionIds: readonly string[], ): { swallowed: string[]; normal: string[] } => { if (intent.kind !== "clearOnActivity") return { swallowed: [], normal: [...sessionIds] }; - const settlingIds = sessionIds.filter((id) => settling.isSettling(id)); - if (!settlingIds.length) return { swallowed: [], normal: [...sessionIds] }; - if (intent.cause === "mechanical") { - return { - swallowed: settlingIds, - normal: sessionIds.filter((id) => !settling.isSettling(id)), - }; + if (sessionIds.length > 1) { + throw new Error( + "clearOnActivity is single-session by construction; a multi-id clear would need a per-session settling disposition.", + ); } - // A human decision: trip abort for the settling sessions, then let every - // session clear normally — the clear itself is not suppressed. - for (const id of settlingIds) settling.abort(id, intent.cause); + const [sessionId] = sessionIds; + if (!settling.isSettling(sessionId)) return { swallowed: [], normal: [...sessionIds] }; + if (intent.cause === "mechanical") return { swallowed: [...sessionIds], normal: [] }; + // A human decision: trip abort, then let the clear proceed — the clear + // itself is never suppressed. + settling.abort(sessionId, intent.cause); return { swallowed: [], normal: [...sessionIds] }; }; @@ -325,6 +326,10 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { // The token is advisory; failing to reap it must not fail the delete. } inProcessLifecycleRevisions.delete(trimmed); + // The third map. A window left open for a deleted id would report a + // nonexistent session as settling forever, and if the id were ever reused + // its mechanical clears would be permanently swallowed. + settling.end(trimmed); }, }; } diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index e44368fd0..245a9250d 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -98,6 +98,9 @@ describe("settle race matrix (teardown is a no-op)", () => { expect(outcome.settled).toEqual([]); expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "turn_start" }]); expect(service.get("session-1")?.settledAt).toBeNull(); + // A leaked window would swallow this session's output forever and leave it + // permanently unsettleable — the mechanism's worst failure mode. + expect(service.settlingSessionIds()).toEqual([]); }); /** @@ -278,34 +281,57 @@ describe("settle race matrix (teardown is a no-op)", () => { }); /** - * The swallow is per session, not per batch. + * `clearOnActivity` is single-session by construction, and the writer now says + * so rather than carrying a per-session partition no test could reach. * - * Unreachable today — every mechanical caller is single-session — but deciding - * one disposition for a whole array would silently stop a NON-settling - * session's own output from clearing its settle, and nothing in the signature - * warns the caller who first passes two ids. + * This replaced two tests that were green against the bug they were written to + * catch: a multi-id MECHANICAL clear is not expressible through the service + * API, so any test built from C4/C5 calls filters to a single id and proves + * nothing about batching. */ - it("swallows only the settling session in a mixed batch", async () => { - const { db, service, create, setTeardown } = await fixture(); + it("refuses a multi-id clear rather than guessing one disposition for the batch", async () => { + const { service, create } = await fixture(); create("session-2"); - service.settleSessions(["session-2"]); - expect(service.get("session-2")?.settledAt).toBeTruthy(); + service.settleSessions(["session-1", "session-2"]); - setTeardown(() => { - // Drive the writer directly with both ids: session-1's window is open, - // session-2's is not. - db.run("update terminal_sessions set last_output_at = ? where id = ?", - ["2026-08-11T00:09:00.000Z", "session-2"]); - service.setLastOutputPreview("session-2", "other session output", { clearSettled: true }); - service.setLastOutputPreview("session-1", "settling session output", { clearSettled: true }); + // The only multi-id clear the API offers is `unsettleSessions`, which is a + // DECLARED unsettle, not a `clearOnActivity` — so it is unaffected and both + // rows clear. + service.unsettleSessions(["session-1", "session-2"]); + + expect(service.get("session-1")?.settledAt).toBeNull(); + expect(service.get("session-2")?.settledAt).toBeNull(); + }); + + /** + * A teardown that throws is routine in step 3 — a process refuses to die, a + * cloud stop 500s. It must be reported, must not leak the window, and must not + * discard the accounting for the rest of the batch. + */ + it("reports a throwing teardown and still closes its window", async () => { + const { service, create, setTeardown } = await fixture(); + create("session-2"); + + setTeardown((sessionId) => { + if (sessionId === "session-1") throw new Error("provider stop failed"); }); + const outcome = 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. + expect(outcome.settled).toEqual(["session-2"]); + expect(service.settlingSessionIds()).toEqual([]); + }); + + /** An async teardown would close the window early; the seam refuses one. */ + it("refuses an async teardown rather than closing the window under it", async () => { + const { service, setTeardown } = await fixture(); + setTeardown((() => Promise.resolve()) as unknown as (sessionId: string) => void); + const outcome = service.settleSessionsReportingAborts(["session-1"]); - // The settling session's output was swallowed: it still settled. - expect(outcome.settled).toEqual(["session-1"]); - // The other session's output cleared its settle normally. - expect(service.get("session-2")?.settledAt).toBeNull(); - expect(service.get("session-2")?.lastOutputPreview).toBe("other session output"); + expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "teardown_failed" }]); + expect(service.settlingSessionIds()).toEqual([]); }); /** A settling row found after a restart resolves to not-settled. */ diff --git a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts index 335f921c8..714aeb89f 100644 --- a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts +++ b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts @@ -20,7 +20,7 @@ export type SettleAbortReason = "turn_start" | "turn_failed" | "attention_requested"; /** Why a settle was abandoned, as reported to callers. */ -export type SettleAbortedReason = SettleAbortReason | "lifecycle_changed"; +export type SettleAbortedReason = SettleAbortReason | "lifecycle_changed" | "teardown_failed"; export type SettleAbortedSession = { sessionId: string; From dc7092972dba07585cf0f53ed8219cca44b0275e Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 01:06:43 -0400 Subject: [PATCH 04/10] fix: refuse async teardown at wiring time, and stop retrying an unchanged session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two P1 review findings, both correct. **Rejecting an async teardown after invoking it is too late.** The body has already started, and its unowned continuation can go on stopping processes after the settling window closed — losing the work AND the settle, which is the exact R2 shape. The refusal moved to service construction, where an `AsyncFunction` can be detected before anything runs. The post-hoc thenable check stays as a backstop for a sync function that returns a promise, where the synchronous body has at least completed. **An unconditional retry would re-run teardown against the work that won the race.** The abort signal is edge-triggered: a turn that is STILL running does not trip it again. So the PR poller would have retried every pass, and in step 3 each retry stops the very work that beat the first attempt — turning R2 from a one-off cost into a recurring one. The retry now waits for the session's lifecycle revision to move, which is the "something changed since" signal this whole design is built on. The gate initially recorded the revision in the skip branch instead of the abort branch, so it never armed — caught because the test asserted a call count rather than an absence. Also instance-scoped the abort memory; at module scope it would have leaked across services and across tests. --- .../src/main/services/prs/prAsync.test.ts | 59 +++++++++++++++++++ .../prs/prMergeAutoSettlementService.ts | 27 ++++++++- .../main/services/sessions/sessionService.ts | 23 +++++--- 3 files changed, 99 insertions(+), 10 deletions(-) diff --git a/apps/desktop/src/main/services/prs/prAsync.test.ts b/apps/desktop/src/main/services/prs/prAsync.test.ts index 43415044b..a2a5e8b09 100644 --- a/apps/desktop/src/main/services/prs/prAsync.test.ts +++ b/apps/desktop/src/main/services/prs/prAsync.test.ts @@ -822,6 +822,13 @@ function withSessionLookup Array<{ id: string }> }>(serv } describe("prMergeAutoSettlementService", () => { + /** + * Default: the revision always looks different, so a retry is never blocked. + * The no-retry test overrides it to a constant. + */ + let revisionCounter = 0; + const getRevision = () => (revisionCounter += 1); + function createMemoryDb() { const values = new Map(); return { @@ -866,6 +873,7 @@ describe("prMergeAutoSettlementService", () => { ]), get: vi.fn(() => null), settleSessionsReportingAborts, + getSettleLifecycleRevision: getRevision, }) as any, emitEvent, }); @@ -928,6 +936,7 @@ describe("prMergeAutoSettlementService", () => { }]), get: vi.fn(() => null), settleSessionsReportingAborts, + getSettleLifecycleRevision: getRevision, }) as any, emitEvent: vi.fn(), }); @@ -1006,6 +1015,7 @@ describe("prMergeAutoSettlementService", () => { }]), get: vi.fn(() => null), settleSessionsReportingAborts, + getSettleLifecycleRevision: getRevision, }) as any, emitEvent: vi.fn(), }); @@ -1077,6 +1087,7 @@ describe("prMergeAutoSettlementService", () => { ]), get: vi.fn(() => null), settleSessionsReportingAborts, + getSettleLifecycleRevision: getRevision, }) as any, emitEvent: vi.fn(), }); @@ -1121,6 +1132,7 @@ describe("prMergeAutoSettlementService", () => { ]), get: vi.fn(() => null), settleSessionsReportingAborts, + getSettleLifecycleRevision: getRevision, }) as any, emitEvent, }); @@ -1153,6 +1165,51 @@ describe("prMergeAutoSettlementService", () => { expect(emitEvent, "the retry must still announce the merge").toHaveBeenCalled(); }); + /** + * The abort signal is edge-triggered: a turn that is STILL running will not + * trip it again. So an unconditional retry would, once step 3 attaches real + * teardown, stop the very work that beat the first attempt — once per poll. + * The revision is the "something changed since" signal that gates it. + */ + it("does not retry an aborted settle while the session has not changed", async () => { + const db = createMemoryDb(); + const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ + settled: [] as string[], + aborted: ids.map((sessionId) => ({ sessionId, reason: "turn_start" })), + })); + const service = createPrMergeAutoSettlementService({ + db: db as any, + sessionService: withSessionLookup({ + list: vi.fn(() => [ + { laneId: "lane-1", id: "chat-owned", toolType: "codex-chat", archivedAt: null, settledAt: null }, + ]), + get: vi.fn(() => null), + settleSessionsReportingAborts, + // The turn is still running: nothing about the session has moved. + getSettleLifecycleRevision: () => 7, + }) as any, + emitEvent: vi.fn(), + }); + + const openPr = createSummary({ state: "open" }); + await service.processSnapshot({ prs: [openPr], polledAt: "2026-03-24T12:00:00.000Z" }); + const mergedPr = createSummary({ + state: "merged", + mergedAt: "2026-03-24T12:01:00.000Z", + chatSessionIds: ["chat-owned"], + }); + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:01:05.000Z" }); + expect(settleSessionsReportingAborts).toHaveBeenCalledTimes(1); + + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:02:00.000Z" }); + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:03:00.000Z" }); + + expect( + settleSessionsReportingAborts, + "an unchanged session must not have teardown re-run against it every poll", + ).toHaveBeenCalledTimes(1); + }); + it("settles a PR that was already merged when first seen, but announces nothing", async () => { // The machine-switch bug. Point the project tab at another machine and that // machine reconciles, backfilling rows for PRs it had never stored. Every @@ -1176,6 +1233,7 @@ describe("prMergeAutoSettlementService", () => { }]), get: vi.fn(() => null), settleSessionsReportingAborts, + getSettleLifecycleRevision: getRevision, }) as any, emitEvent, }); @@ -1240,6 +1298,7 @@ describe("prMergeAutoSettlementService", () => { }]), get: vi.fn(() => null), settleSessionsReportingAborts, + getSettleLifecycleRevision: getRevision, }) as any, emitEvent: vi.fn(), }); diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index 24bce7bf0..52003db6e 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -62,9 +62,16 @@ function resolveMergeSettlementScope(pr: PrSummary, snapshot: PrSummary[]): Merg export function createPrMergeAutoSettlementService(args: { db: Pick; - sessionService: Pick, "get" | "list" | "settleSessionsReportingAborts">; + sessionService: Pick, "get" | "list" | "settleSessionsReportingAborts" | "getSettleLifecycleRevision">; emitEvent: (event: PrEventPayload) => void; }) { + /** + * The lifecycle revision each session had when its auto-settle last aborted. + * Instance-scoped, so it lives exactly as long as the poller — a module-level + * map would leak across services and across tests. + */ + const abortedRevisions = new Map(); + /** * The currently open or draft PRs in the previous snapshot, so a merge we * WATCHED can be told apart from one that was already history when it arrived. @@ -194,6 +201,20 @@ export function createPrMergeAutoSettlementService(args: { // session even when it still owns scheduled work, a background task, // or another normal settlement blocker. Real activity can unsettle it // again, while handledPrIds prevents this PR from filing it twice. + // Do not retry while the activity that won the race is still in + // progress. The abort signal is edge-triggered — a turn that is STILL + // running will not trip it again — so an unconditional retry would, in + // step 3, stop the very work that beat the first attempt, once per poll. + // The revision is the available "something changed since" signal, and it + // is the mechanism this whole design is built on. + const revisionAtLastAbort = abortedRevisions.get(session.id); + if ( + revisionAtLastAbort !== undefined + && args.sessionService.getSettleLifecycleRevision(session.id) === revisionAtLastAbort + ) { + abandonedThisPr = true; + continue; + } const settleResult = args.sessionService.settleSessionsReportingAborts([session.id], { outcome: `PR #${pr.githubPrNumber} merged`, settledAt: polledAt, @@ -203,8 +224,10 @@ export function createPrMergeAutoSettlementService(args: { if (settleResult.aborted.length) { // The session became active while the settle was in flight. Leaving // the PR unhandled is the point: a later pass retries, instead of this - // merge being consumed by a settle that never landed. + // merge being consumed by a settle that never landed. Record the + // revision so that retry waits for the session to actually change. abandonedThisPr = true; + abortedRevisions.set(session.id, args.sessionService.getSettleLifecycleRevision(session.id)); } } diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index ff3c7c0b7..d1785e230 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -381,6 +381,17 @@ export function createSessionService({ */ runSettleTeardown?: (sessionId: string) => void; }) { + // Refuse an async teardown HERE, at wiring time, not when a settle runs. + // Detecting it after invocation is too late: the body has already started, and + // its unowned continuation can go on stopping processes after the settling + // window closed — losing the work AND the settle. TypeScript's void-return + // rule makes `async (id) => {}` assignable, so this is the only boundary that + // can catch it before it does damage. + if (runSettleTeardown && runSettleTeardown.constructor.name === "AsyncFunction") { + throw new Error( + "runSettleTeardown must be synchronous: the settle path must be made async before teardown can await anything.", + ); + } const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); @@ -761,14 +772,10 @@ export function createSessionService({ if (begin.kind === "joined") continue; try { const teardownResult = runSettleTeardown?.(id) as unknown; - // The seam is synchronous in step 2 and MUST stay so until the settle - // path is made async. An async teardown would return here immediately, - // the checks below would run against a world where nothing had been - // stopped yet, and `finally` would close the window while the real - // teardown was still emitting output — which C4/C5 would then no longer - // swallow, clearing the settle that just landed. TypeScript's void-return - // rule lets an async callback through silently, so this is a loud guard - // rather than a type. + // Backstop for a SYNC function that returns a promise — the + // `AsyncFunction` check at construction cannot see that shape. Still + // after-the-fact, but such a function has at least run its synchronous + // body to completion, so the damage is bounded to whatever it deferred. if (teardownResult && typeof (teardownResult as { then?: unknown }).then === "function") { throw new Error( "runSettleTeardown returned a promise: the settle path must be made async before teardown can await anything.", From 406f7fe9f73d6937963fc3a1cc9517849540e7c8 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 01:33:11 -0400 Subject: [PATCH 05/10] fix: make async teardown a compile error, and cool off instead of skipping forever MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more P1s, both correct, and the second says my previous fix was worse than the problem. **A runtime check cannot catch a promise-returning adapter.** `async (id) => {}` is identifiable by its constructor, but `id => asyncStop(id)` is an ordinary `Function` and has already started the work by the time any check runs. So the seam now demands a branded value only a synchronous body can produce: both shapes return `Promise<...>`, which is not assignable, and fail to COMPILE. The type is the guarantee; there is no runtime check left to be too late. **The revision gate would have skipped the PR forever.** A normal turn *completing* does not move the settle lifecycle — `clearLastTurnFailed` does not touch it, and chat output deliberately opts out of clearing — so the recorded revision could stay equal indefinitely and the merged PR would never be filed. That is worse than the over-retry it replaced. Now a 10-minute cooling-off window: it stops teardown being re-run against work that is still in progress, and it always expires, so the merge is deferred rather than abandoned. --- .../src/main/services/prs/prAsync.test.ts | 30 +++++-------- .../prs/prMergeAutoSettlementService.ts | 42 ++++++++++++------- .../main/services/sessions/sessionService.ts | 26 ++---------- .../sessions/settleRaceMatrix.test.ts | 22 +++++----- .../sessions/settlingStateRegistry.ts | 27 ++++++++++++ 5 files changed, 79 insertions(+), 68 deletions(-) diff --git a/apps/desktop/src/main/services/prs/prAsync.test.ts b/apps/desktop/src/main/services/prs/prAsync.test.ts index a2a5e8b09..186659198 100644 --- a/apps/desktop/src/main/services/prs/prAsync.test.ts +++ b/apps/desktop/src/main/services/prs/prAsync.test.ts @@ -822,13 +822,6 @@ function withSessionLookup Array<{ id: string }> }>(serv } describe("prMergeAutoSettlementService", () => { - /** - * Default: the revision always looks different, so a retry is never blocked. - * The no-retry test overrides it to a constant. - */ - let revisionCounter = 0; - const getRevision = () => (revisionCounter += 1); - function createMemoryDb() { const values = new Map(); return { @@ -873,7 +866,6 @@ describe("prMergeAutoSettlementService", () => { ]), get: vi.fn(() => null), settleSessionsReportingAborts, - getSettleLifecycleRevision: getRevision, }) as any, emitEvent, }); @@ -936,7 +928,6 @@ describe("prMergeAutoSettlementService", () => { }]), get: vi.fn(() => null), settleSessionsReportingAborts, - getSettleLifecycleRevision: getRevision, }) as any, emitEvent: vi.fn(), }); @@ -1015,7 +1006,6 @@ describe("prMergeAutoSettlementService", () => { }]), get: vi.fn(() => null), settleSessionsReportingAborts, - getSettleLifecycleRevision: getRevision, }) as any, emitEvent: vi.fn(), }); @@ -1087,7 +1077,6 @@ describe("prMergeAutoSettlementService", () => { ]), get: vi.fn(() => null), settleSessionsReportingAborts, - getSettleLifecycleRevision: getRevision, }) as any, emitEvent: vi.fn(), }); @@ -1132,7 +1121,6 @@ describe("prMergeAutoSettlementService", () => { ]), get: vi.fn(() => null), settleSessionsReportingAborts, - getSettleLifecycleRevision: getRevision, }) as any, emitEvent, }); @@ -1153,7 +1141,8 @@ describe("prMergeAutoSettlementService", () => { settled: ids, aborted: [] as Array<{ sessionId: string; reason: string }>, })); - await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:05:00.000Z" }); + // Past the cooling-off window: the merge is deferred, never abandoned. + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:31:00.000Z" }); expect( settleSessionsReportingAborts, @@ -1171,7 +1160,7 @@ describe("prMergeAutoSettlementService", () => { * teardown, stop the very work that beat the first attempt — once per poll. * The revision is the "something changed since" signal that gates it. */ - it("does not retry an aborted settle while the session has not changed", async () => { + it("cools off after an aborted settle, then re-arms", async () => { const db = createMemoryDb(); const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ settled: [] as string[], @@ -1185,8 +1174,6 @@ describe("prMergeAutoSettlementService", () => { ]), get: vi.fn(() => null), settleSessionsReportingAborts, - // The turn is still running: nothing about the session has moved. - getSettleLifecycleRevision: () => 7, }) as any, emitEvent: vi.fn(), }); @@ -1206,8 +1193,15 @@ describe("prMergeAutoSettlementService", () => { expect( settleSessionsReportingAborts, - "an unchanged session must not have teardown re-run against it every poll", + "teardown must not be re-run against still-active work on every poll", ).toHaveBeenCalledTimes(1); + + // ...but the window always expires, so the PR is never permanently skipped. + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:31:00.000Z" }); + expect( + settleSessionsReportingAborts, + "the cooling-off window must re-arm", + ).toHaveBeenCalledTimes(2); }); it("settles a PR that was already merged when first seen, but announces nothing", async () => { @@ -1233,7 +1227,6 @@ describe("prMergeAutoSettlementService", () => { }]), get: vi.fn(() => null), settleSessionsReportingAborts, - getSettleLifecycleRevision: getRevision, }) as any, emitEvent, }); @@ -1298,7 +1291,6 @@ describe("prMergeAutoSettlementService", () => { }]), get: vi.fn(() => null), settleSessionsReportingAborts, - getSettleLifecycleRevision: getRevision, }) as any, emitEvent: vi.fn(), }); diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index 52003db6e..9dfd33f33 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -62,15 +62,25 @@ function resolveMergeSettlementScope(pr: PrSummary, snapshot: PrSummary[]): Merg export function createPrMergeAutoSettlementService(args: { db: Pick; - sessionService: Pick, "get" | "list" | "settleSessionsReportingAborts" | "getSettleLifecycleRevision">; + sessionService: Pick, "get" | "list" | "settleSessionsReportingAborts">; emitEvent: (event: PrEventPayload) => void; }) { /** - * The lifecycle revision each session had when its auto-settle last aborted. - * Instance-scoped, so it lives exactly as long as the poller — a module-level - * map would leak across services and across tests. + * When each session's auto-settle last aborted. + * + * A revision check was tried and is wrong: a normal turn COMPLETING does not + * move the settle lifecycle (`clearLastTurnFailed` does not touch it, and chat + * output deliberately opts out of clearing), so the revision can stay equal + * forever and the merged PR would be skipped for good — worse than the + * over-retry it was meant to fix. + * + * A cooling-off period is the honest gate: it always re-arms, so the PR is + * never permanently abandoned, while the poll interval stops re-running + * teardown against work that is still in progress. Instance-scoped, so it + * lives exactly as long as the poller. */ - const abortedRevisions = new Map(); + const lastAbortedAtBySession = new Map(); + const RETRY_AFTER_ABORT_MS = 10 * 60 * 1000; /** * The currently open or draft PRs in the previous snapshot, so a merge we @@ -201,16 +211,17 @@ export function createPrMergeAutoSettlementService(args: { // session even when it still owns scheduled work, a background task, // or another normal settlement blocker. Real activity can unsettle it // again, while handledPrIds prevents this PR from filing it twice. - // Do not retry while the activity that won the race is still in - // progress. The abort signal is edge-triggered — a turn that is STILL - // running will not trip it again — so an unconditional retry would, in - // step 3, stop the very work that beat the first attempt, once per poll. - // The revision is the available "something changed since" signal, and it - // is the mechanism this whole design is built on. - const revisionAtLastAbort = abortedRevisions.get(session.id); + // Cool off after an abort. The abort signal is edge-triggered — a turn + // that is STILL running will not trip it again — so retrying every poll + // would, in step 3, stop the very work that beat the first attempt, over + // and over. The window always expires, so the merge is deferred, never + // abandoned. + const lastAbortedAt = lastAbortedAtBySession.get(session.id); + const polledAtMs = Date.parse(polledAt); if ( - revisionAtLastAbort !== undefined - && args.sessionService.getSettleLifecycleRevision(session.id) === revisionAtLastAbort + lastAbortedAt !== undefined + && Number.isFinite(polledAtMs) + && polledAtMs - lastAbortedAt < RETRY_AFTER_ABORT_MS ) { abandonedThisPr = true; continue; @@ -227,7 +238,8 @@ export function createPrMergeAutoSettlementService(args: { // merge being consumed by a settle that never landed. Record the // revision so that retry waits for the session to actually change. abandonedThisPr = true; - abortedRevisions.set(session.id, args.sessionService.getSettleLifecycleRevision(session.id)); + const abortedAtMs = Date.parse(polledAt); + if (Number.isFinite(abortedAtMs)) lastAbortedAtBySession.set(session.id, abortedAtMs); } } diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index d1785e230..f2d6b9036 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 { createSettleLifecycleWriter } from "./settleLifecycleWriter"; -import type { SettleAbortedSession, SettleSessionsOutcome } from "./settlingStateRegistry"; +import type { SettleAbortedSession, SettleSessionsOutcome, SettleTeardownCompleted } from "./settlingStateRegistry"; import type { ClaudeSessionPointer, SessionAttentionSource, @@ -379,19 +379,8 @@ 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) => void; + runSettleTeardown?: (sessionId: string) => SettleTeardownCompleted; }) { - // Refuse an async teardown HERE, at wiring time, not when a settle runs. - // Detecting it after invocation is too late: the body has already started, and - // its unowned continuation can go on stopping processes after the settling - // window closed — losing the work AND the settle. TypeScript's void-return - // rule makes `async (id) => {}` assignable, so this is the only boundary that - // can catch it before it does damage. - if (runSettleTeardown && runSettleTeardown.constructor.name === "AsyncFunction") { - throw new Error( - "runSettleTeardown must be synchronous: the settle path must be made async before teardown can await anything.", - ); - } const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); @@ -771,16 +760,7 @@ export function createSessionService({ // The owner will report the outcome; reporting it twice would double-count. if (begin.kind === "joined") continue; try { - const teardownResult = runSettleTeardown?.(id) as unknown; - // Backstop for a SYNC function that returns a promise — the - // `AsyncFunction` check at construction cannot see that shape. Still - // after-the-fact, but such a function has at least run its synchronous - // body to completion, so the damage is bounded to whatever it deferred. - if (teardownResult && typeof (teardownResult as { then?: unknown }).then === "function") { - throw new Error( - "runSettleTeardown returned a promise: the settle path must be made async before teardown can await anything.", - ); - } + runSettleTeardown?.(id); const abortedBy = settleLifecycle.settling.abortedBy(id); if (abortedBy) { diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index 245a9250d..4d6c06d81 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { openKvDb } from "../state/kvDb"; import { createSessionService } from "./sessionService"; +import { settleTeardownCompleted } from "./settlingStateRegistry"; /** * The race matrix from the settle-teardown design (§2), tested directly against @@ -60,7 +61,10 @@ describe("settle race matrix (teardown is a no-op)", () => { let teardown: (sessionId: string) => void = () => {}; const service = createSessionService({ db, - runSettleTeardown: (sessionId) => teardown(sessionId), + runSettleTeardown: (sessionId) => { + teardown(sessionId); + return settleTeardownCompleted(); + }, }); const setTeardown = (fn: (sessionId: string) => void) => { teardown = fn; @@ -323,16 +327,12 @@ describe("settle race matrix (teardown is a no-op)", () => { expect(service.settlingSessionIds()).toEqual([]); }); - /** An async teardown would close the window early; the seam refuses one. */ - it("refuses an async teardown rather than closing the window under it", async () => { - const { service, setTeardown } = await fixture(); - setTeardown((() => Promise.resolve()) as unknown as (sessionId: string) => void); - - const outcome = service.settleSessionsReportingAborts(["session-1"]); - - expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "teardown_failed" }]); - 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 () => { diff --git a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts index 714aeb89f..490e7cfb9 100644 --- a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts +++ b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts @@ -16,6 +16,33 @@ * would be a lie on every other device the moment this host died. */ +declare const settleTeardownCompletedBrand: unique symbol; + +/** + * Proof that a teardown finished synchronously. + * + * 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. + * + * 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. + * + * Step 3 replaces this with an awaited seam once the settle path itself is async. + */ +export type SettleTeardownCompleted = { readonly [settleTeardownCompletedBrand]: true }; + +/** The only way to produce a `SettleTeardownCompleted`. */ +export function settleTeardownCompleted(): SettleTeardownCompleted { + return {} as SettleTeardownCompleted; +} + /** Why a settle was abandoned. Only ever set by a human-decision clearer. */ export type SettleAbortReason = "turn_start" | "turn_failed" | "attention_requested"; From 2da52b6dc33a1e388638a6be6934a52ee86f2763 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 01:51:07 -0400 Subject: [PATCH 06/10] fix: wait for the turn to end, and stop a joined settle reading as filed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more P1s. **A cooldown expires on the clock, not when the turn ends.** A chat turn lasting longer than the window would have had teardown re-run against it in step 3 — the same failure the cooldown replaced, just later. Gated on the session being AT REST instead: the same signal the canonical settle tier uses, which re-arms exactly when the activity finishes. Third gate on this one, and the first two were both wrong in opposite directions — a revision check that never re-armed, then a timer that re-armed too early. **A joined settle reported nothing, which reads as "filed".** PR auto-settle joining a user settle already in flight got `{settled: [], aborted: []}`, saw no abort, and marked the merge handled — on the strength of someone else's settle that may yet abort. It now reports `joined_in_flight`: not settled, and not silently absent either, which is the whole point of the typed outcome. Also asserts the announce is the merge event rather than any event. --- .../src/main/services/prs/prAsync.test.ts | 26 +++++--- .../prs/prMergeAutoSettlementService.ts | 59 +++++++++---------- .../main/services/sessions/sessionService.ts | 9 ++- .../sessions/settleRaceMatrix.test.ts | 12 +++- .../sessions/settlingStateRegistry.ts | 7 ++- 5 files changed, 71 insertions(+), 42 deletions(-) diff --git a/apps/desktop/src/main/services/prs/prAsync.test.ts b/apps/desktop/src/main/services/prs/prAsync.test.ts index 186659198..e238cea35 100644 --- a/apps/desktop/src/main/services/prs/prAsync.test.ts +++ b/apps/desktop/src/main/services/prs/prAsync.test.ts @@ -1151,7 +1151,9 @@ describe("prMergeAutoSettlementService", () => { // ...and the retry must still announce. A merged PR falls out of the // watchable set every pass, so without keeping it alive the user would get // the settle and never the toast. - expect(emitEvent, "the retry must still announce the merge").toHaveBeenCalled(); + expect(emitEvent, "the retry must still announce the merge").toHaveBeenCalledWith( + expect.objectContaining({ type: "pr-sessions-auto-settled" }), + ); }); /** @@ -1160,19 +1162,28 @@ describe("prMergeAutoSettlementService", () => { * teardown, stop the very work that beat the first attempt — once per poll. * The revision is the "something changed since" signal that gates it. */ - it("cools off after an aborted settle, then re-arms", async () => { + it("holds the retry until the session is at rest, then re-arms", async () => { const db = createMemoryDb(); const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ settled: [] as string[], aborted: ids.map((sessionId) => ({ sessionId, reason: "turn_start" })), })); + // The turn that won the race is still running until the test says otherwise. + let sessionRuntimeState = "running"; const service = createPrMergeAutoSettlementService({ db: db as any, sessionService: withSessionLookup({ list: vi.fn(() => [ - { laneId: "lane-1", id: "chat-owned", toolType: "codex-chat", archivedAt: null, settledAt: null }, + { + laneId: "lane-1", + id: "chat-owned", + toolType: "codex-chat", + archivedAt: null, + settledAt: null, + status: "running", + runtimeState: sessionRuntimeState, + }, ]), - get: vi.fn(() => null), settleSessionsReportingAborts, }) as any, emitEvent: vi.fn(), @@ -1196,11 +1207,12 @@ describe("prMergeAutoSettlementService", () => { "teardown must not be re-run against still-active work on every poll", ).toHaveBeenCalledTimes(1); - // ...but the window always expires, so the PR is never permanently skipped. - await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:31:00.000Z" }); + // The turn ends: the runtime goes idle and the retry re-arms. + sessionRuntimeState = "idle"; + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:04:00.000Z" }); expect( settleSessionsReportingAborts, - "the cooling-off window must re-arm", + "the retry must re-arm once the session is at rest", ).toHaveBeenCalledTimes(2); }); diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index 9dfd33f33..04daa5282 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -66,21 +66,20 @@ export function createPrMergeAutoSettlementService(args: { emitEvent: (event: PrEventPayload) => void; }) { /** - * When each session's auto-settle last aborted. + * Sessions whose auto-settle aborted and that have not been seen at rest + * since. * - * A revision check was tried and is wrong: a normal turn COMPLETING does not - * move the settle lifecycle (`clearLastTurnFailed` does not touch it, and chat - * output deliberately opts out of clearing), so the revision can stay equal - * forever and the merged PR would be skipped for good — worse than the - * over-retry it was meant to fix. + * Two earlier gates were wrong. A revision check never re-arms: a turn + * COMPLETING does not move the settle lifecycle, so the PR would be skipped + * forever. A time-based cooldown expires while a long turn is still running, + * so in step 3 the retry would stop the very work that won the race. * - * A cooling-off period is the honest gate: it always re-arms, so the PR is - * never permanently abandoned, while the poll interval stops re-running - * teardown against work that is still in progress. Instance-scoped, so it - * lives exactly as long as the poller. + * "At rest" is the actual signal, and it is the same one the canonical settle + * tier uses: a session is eligible again once it is no longer running, or its + * runtime has gone idle. It re-arms exactly when the activity ends. + * Instance-scoped, so it lives as long as the poller. */ - const lastAbortedAtBySession = new Map(); - const RETRY_AFTER_ABORT_MS = 10 * 60 * 1000; + const abortedSessionIds = new Set(); /** * The currently open or draft PRs in the previous snapshot, so a merge we @@ -211,20 +210,21 @@ export function createPrMergeAutoSettlementService(args: { // session even when it still owns scheduled work, a background task, // or another normal settlement blocker. Real activity can unsettle it // again, while handledPrIds prevents this PR from filing it twice. - // Cool off after an abort. The abort signal is edge-triggered — a turn - // that is STILL running will not trip it again — so retrying every poll - // would, in step 3, stop the very work that beat the first attempt, over - // and over. The window always expires, so the merge is deferred, never - // abandoned. - const lastAbortedAt = lastAbortedAtBySession.get(session.id); - const polledAtMs = Date.parse(polledAt); - if ( - lastAbortedAt !== undefined - && Number.isFinite(polledAtMs) - && polledAtMs - lastAbortedAt < RETRY_AFTER_ABORT_MS - ) { - abandonedThisPr = true; - continue; + // Wait for the activity that won the race to finish. The abort signal is + // edge-triggered, so a turn that is STILL running never re-trips it; + // retrying regardless would, in step 3, stop the very work that beat the + // first attempt. Gating on "at rest" defers the merge without ever + // abandoning it. + if (abortedSessionIds.has(session.id)) { + const current = args.sessionService.get(session.id); + const status = (current?.status ?? "").toLowerCase(); + const runtime = (current?.runtimeState ?? "").toLowerCase(); + const atRest = status !== "running" || runtime === "idle"; + if (!atRest) { + abandonedThisPr = true; + continue; + } + abortedSessionIds.delete(session.id); } const settleResult = args.sessionService.settleSessionsReportingAborts([session.id], { outcome: `PR #${pr.githubPrNumber} merged`, @@ -235,11 +235,10 @@ export function createPrMergeAutoSettlementService(args: { if (settleResult.aborted.length) { // The session became active while the settle was in flight. Leaving // the PR unhandled is the point: a later pass retries, instead of this - // merge being consumed by a settle that never landed. Record the - // revision so that retry waits for the session to actually change. + // merge being consumed by a settle that never landed. Hold the retry + // until the session is seen at rest. abandonedThisPr = true; - const abortedAtMs = Date.parse(polledAt); - if (Number.isFinite(abortedAtMs)) lastAbortedAtBySession.set(session.id, abortedAtMs); + 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 f2d6b9036..022851585 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -758,7 +758,14 @@ export function createSessionService({ 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. - if (begin.kind === "joined") continue; + 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 + // strength of someone else's in-flight settle that may yet abort. + aborted.push({ sessionId: id, reason: "joined_in_flight" }); + continue; + } try { runSettleTeardown?.(id); diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index 4d6c06d81..af9ceddef 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -163,9 +163,15 @@ describe("settle race matrix (teardown is a no-op)", () => { expect(teardowns, "the joined settle must not start its own teardown").toBe(1); expect(outcome.settled).toEqual(["session-1"]); - // The joiner reports nothing: the owner reports the outcome, and - // double-counting would make a bulk caller see one session twice. - expect(inner).toMatchObject({ settled: [], aborted: [] }); + // The joiner does NOT report success. It filed nothing, and a caller with a + // durable consequence — the PR poller marking a merge handled — must not + // consume that merge on the strength of someone else's in-flight settle, + // which may yet abort. It is also not counted as settled, so a bulk caller + // cannot see one session twice. + expect(inner).toMatchObject({ + settled: [], + aborted: [{ sessionId: "session-1", reason: "joined_in_flight" }], + }); }); /** diff --git a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts index 490e7cfb9..56efe7e31 100644 --- a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts +++ b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts @@ -47,7 +47,12 @@ export function settleTeardownCompleted(): SettleTeardownCompleted { export type SettleAbortReason = "turn_start" | "turn_failed" | "attention_requested"; /** Why a settle was abandoned, as reported to callers. */ -export type SettleAbortedReason = SettleAbortReason | "lifecycle_changed" | "teardown_failed"; +export type SettleAbortedReason = + | SettleAbortReason + | "lifecycle_changed" + | "teardown_failed" + /** Another settle owned the window; this caller filed nothing. */ + | "joined_in_flight"; export type SettleAbortedSession = { sessionId: string; From 525fc42f79c4dfb8c29053fb612419672fef4c6c Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 02:11:04 -0400 Subject: [PATCH 07/10] fix: stop gating the merge retry on a signal that does not exist yet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1: the at-rest gate never observes turn completion for chat sessions. sessionService.get() returns the raw persisted row and derives runtimeState from status, and chat rows deliberately hold status = "running" between turns — only chatSessionProjection resolves an idle chat to idle. So after a chat turn aborted auto-settlement, every later poll kept the merged PR unfinished indefinitely. Reading projected chat state needs a dependency this poller does not have, which is step-3 scope. Three gates were tried here and each was wrong in a different way (revision never re-arms, timer re-arms mid-turn, raw runtimeState is blind to chat). Remove the gate: retry unconditionally, which is harmless while teardown is a no-op and is what this code did before the settling window existed. Record the bound as an explicit step-3 requirement (design doc 3c-iii) with the failure mode of each attempt, so teardown does not re-derive them. abandonedThisPr still blocks handledPrIds, so an aborted settle never consumes the merge — that contract keeps its test. --- .../src/main/services/prs/prAsync.test.ts | 60 ------------------- .../prs/prMergeAutoSettlementService.ts | 52 ++++++---------- .../settle-teardown-design.md | 23 +++++++ 3 files changed, 41 insertions(+), 94 deletions(-) diff --git a/apps/desktop/src/main/services/prs/prAsync.test.ts b/apps/desktop/src/main/services/prs/prAsync.test.ts index e238cea35..fa22e0b7e 100644 --- a/apps/desktop/src/main/services/prs/prAsync.test.ts +++ b/apps/desktop/src/main/services/prs/prAsync.test.ts @@ -1156,66 +1156,6 @@ describe("prMergeAutoSettlementService", () => { ); }); - /** - * The abort signal is edge-triggered: a turn that is STILL running will not - * trip it again. So an unconditional retry would, once step 3 attaches real - * teardown, stop the very work that beat the first attempt — once per poll. - * The revision is the "something changed since" signal that gates it. - */ - it("holds the retry until the session is at rest, then re-arms", async () => { - const db = createMemoryDb(); - const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ - settled: [] as string[], - aborted: ids.map((sessionId) => ({ sessionId, reason: "turn_start" })), - })); - // The turn that won the race is still running until the test says otherwise. - let sessionRuntimeState = "running"; - const service = createPrMergeAutoSettlementService({ - db: db as any, - sessionService: withSessionLookup({ - list: vi.fn(() => [ - { - laneId: "lane-1", - id: "chat-owned", - toolType: "codex-chat", - archivedAt: null, - settledAt: null, - status: "running", - runtimeState: sessionRuntimeState, - }, - ]), - settleSessionsReportingAborts, - }) as any, - emitEvent: vi.fn(), - }); - - const openPr = createSummary({ state: "open" }); - await service.processSnapshot({ prs: [openPr], polledAt: "2026-03-24T12:00:00.000Z" }); - const mergedPr = createSummary({ - state: "merged", - mergedAt: "2026-03-24T12:01:00.000Z", - chatSessionIds: ["chat-owned"], - }); - await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:01:05.000Z" }); - expect(settleSessionsReportingAborts).toHaveBeenCalledTimes(1); - - await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:02:00.000Z" }); - await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:03:00.000Z" }); - - expect( - settleSessionsReportingAborts, - "teardown must not be re-run against still-active work on every poll", - ).toHaveBeenCalledTimes(1); - - // The turn ends: the runtime goes idle and the retry re-arms. - sessionRuntimeState = "idle"; - await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:04:00.000Z" }); - expect( - settleSessionsReportingAborts, - "the retry must re-arm once the session is at rest", - ).toHaveBeenCalledTimes(2); - }); - it("settles a PR that was already merged when first seen, but announces nothing", async () => { // The machine-switch bug. Point the project tab at another machine and that // machine reconciles, backfilling rows for PRs it had never stored. Every diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index 04daa5282..96d73e667 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -65,21 +65,7 @@ export function createPrMergeAutoSettlementService(args: { sessionService: Pick, "get" | "list" | "settleSessionsReportingAborts">; emitEvent: (event: PrEventPayload) => void; }) { - /** - * Sessions whose auto-settle aborted and that have not been seen at rest - * since. - * - * Two earlier gates were wrong. A revision check never re-arms: a turn - * COMPLETING does not move the settle lifecycle, so the PR would be skipped - * forever. A time-based cooldown expires while a long turn is still running, - * so in step 3 the retry would stop the very work that won the race. - * - * "At rest" is the actual signal, and it is the same one the canonical settle - * tier uses: a session is eligible again once it is no longer running, or its - * runtime has gone idle. It re-arms exactly when the activity ends. - * Instance-scoped, so it lives as long as the poller. - */ - const abortedSessionIds = new Set(); + /** * The currently open or draft PRs in the previous snapshot, so a merge we @@ -210,22 +196,22 @@ export function createPrMergeAutoSettlementService(args: { // session even when it still owns scheduled work, a background task, // or another normal settlement blocker. Real activity can unsettle it // again, while handledPrIds prevents this PR from filing it twice. - // Wait for the activity that won the race to finish. The abort signal is - // edge-triggered, so a turn that is STILL running never re-trips it; - // retrying regardless would, in step 3, stop the very work that beat the - // first attempt. Gating on "at rest" defers the merge without ever - // abandoning it. - if (abortedSessionIds.has(session.id)) { - const current = args.sessionService.get(session.id); - const status = (current?.status ?? "").toLowerCase(); - const runtime = (current?.runtimeState ?? "").toLowerCase(); - const atRest = status !== "running" || runtime === "idle"; - if (!atRest) { - abandonedThisPr = true; - continue; - } - abortedSessionIds.delete(session.id); - } + // NOTE for step 3: this retries on the next poll, unconditionally. + // + // That is harmless while teardown is a no-op, and it is what this code + // did before the settling window existed — but once real teardown + // attaches, a retry against work that is still running would stop the + // very work that won the race, once per poll. Three gates were tried + // here and each was wrong in a different way: a lifecycle-revision check + // never re-arms (a turn COMPLETING does not move the settle tuple), a + // timer re-arms while a long turn is still running, and a raw + // `runtimeState` check never observes turn completion at all, because + // chat rows deliberately keep `status = "running"` between turns and + // only `chatSessionProjection` resolves an idle chat to `idle`. + // + // The correct gate therefore needs PROJECTED chat state, which means a + // dependency this poller does not have. Bounding it belongs with + // teardown, not before it. const settleResult = args.sessionService.settleSessionsReportingAborts([session.id], { outcome: `PR #${pr.githubPrNumber} merged`, settledAt: polledAt, @@ -235,10 +221,8 @@ export function createPrMergeAutoSettlementService(args: { if (settleResult.aborted.length) { // The session became active while the settle was in flight. Leaving // the PR unhandled is the point: a later pass retries, instead of this - // merge being consumed by a settle that never landed. Hold the retry - // until the session is seen at rest. + // merge being consumed by a settle that never landed. abandonedThisPr = true; - abortedSessionIds.add(session.id); } } diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index 90167ec98..d5ff96662 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -414,6 +414,29 @@ 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) + +When `settleSessions` reports an abort, `prMergeAutoSettlementService` leaves the +merged PR unhandled so a later poll retries it — otherwise the merge is consumed +by a settle that never landed. Step 2 ships that retry **unconditional**, which +is correct while teardown is a no-op and is what the code did before the settling +window existed. + +It stops being correct the moment teardown is real: a retry fired against work +that is still running would stop the very work that won the race, once per poll. +Step 3 owns the bound. Three gates were tried in step 2 and each was wrong in a +different way, so the next attempt should start from why: + +| Gate | Why it fails | +| --- | --- | +| Lifecycle revision moved | Never re-arms. A turn *completing* does not touch the settle tuple, so the revision is unchanged and the retry is skipped forever. | +| 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, which this poller has +no dependency on today. Wiring that dependency is step-3 scope; do not re-attempt +a gate that reads the raw persisted row. + ### 3d. When teardown cannot confirm R5 is a product decision, not a mechanism. If a provider stop is unavailable, From f3e4771d089c684792be454ad9e0718a05e54eb9 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 02:39:06 -0400 Subject: [PATCH 08/10] fix: three review findings, and a retry gate that reads chat liveness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1 (retry gate, 4th attempt): the previous gates could not see a chat turn end. A chat's persisted row holds status = "running" between turns on purpose and runtimeState derives from it, so the row reports a finished chat as running forever. Ask the chat service instead, through a narrow injected getChatLiveness callback (the chatMentionService.listChatSessions pattern), and fall back to the row only for tracked CLI sessions, whose row is authoritative. Test pins the exact regression: row says running, chat says idle, retry must fire — it fails against the row-reading version. Codex P2 + CodeRabbit Major (sessionService): settleSession passed outcome: undefined, and settleMany decides with hasOwnProperty, so a re-settle with no outcome wrote status_note = null and erased the note the previous settle left. Omit the key instead. Codex P2 + CodeRabbit Major (sessionService): settleSession returned true whenever the row existed, so an aborted settle reported success — the exact silent-success contract the typed outcome removes. It now returns false on abort, and settleSessionReportingAbort carries the reason so the CTO tool stops rendering "Session is active again" as "Session not found". Codex P2 + CodeRabbit Major (sessionService): the teardown catch also wrapped the persistence write, so a SQLite lock surfaced as teardown_failed and invited a retry of a stop that had already succeeded. Narrow it to runSettleTeardown; persistence errors propagate as they did before. CodeRabbit (tests): two of my own tests were vacuous. The multi-id refusal never reached the throw it is named for, and the crash-safety assertion sat inside the teardown callback where the catch swallowed its failure. Both now assert outside the catch and were verified to fail against the old code. --- apps/desktop/src/main/main.ts | 6 ++ .../ai/tools/ctoOperatorTools.test.ts | 3 +- .../services/ai/tools/ctoOperatorTools.ts | 14 ++- .../src/main/services/prs/prAsync.test.ts | 59 ++++++++++++ .../prs/prMergeAutoSettlementService.ts | 76 ++++++++++++---- .../main/services/sessions/sessionService.ts | 67 +++++++++++--- .../sessions/settleRaceMatrix.test.ts | 89 +++++++++++++++++-- .../settle-teardown-design.md | 9 +- 8 files changed, 284 insertions(+), 39 deletions(-) diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index ba0b98a83..58f07ca65 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -3589,6 +3589,12 @@ app.whenReady().then(async () => { db, sessionService, emitEvent: emitPrEvent, + getChatLiveness: async (sessionId) => { + const summary = await agentChatService.getSessionSummary(sessionId); + return summary + ? { status: summary.status, awaitingInput: summary.awaitingInput } + : null; + }, }); laneTeardownDeps.agentChatService = { countActiveForLane: (laneId) => agentChatService.countActiveForLane(laneId), diff --git a/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.ts b/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.ts index 8e92fc402..443bf6a97 100644 --- a/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.ts +++ b/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.test.ts @@ -497,6 +497,7 @@ describe("createCtoOperatorTools", () => { ...row, })), settleSession: vi.fn(() => true), + settleSessionReportingAbort: vi.fn(() => ({ found: true, settled: true })), unsettleSession: vi.fn(() => true), setSettleOverride: vi.fn(() => true), snoozeSession: vi.fn(() => true), @@ -548,7 +549,7 @@ describe("createCtoOperatorTools", () => { sessionId: "chat-1", outcome: "CI green", })).resolves.toMatchObject({ success: true }); - expect(sessionService.settleSession).toHaveBeenCalledWith("chat-1", { + expect(sessionService.settleSessionReportingAbort).toHaveBeenCalledWith("chat-1", { outcome: "CI green", source: "operator", }); diff --git a/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts b/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts index 416d69c88..cf48ff23c 100644 --- a/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts +++ b/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts @@ -49,6 +49,7 @@ export interface CtoOperatorToolDeps { | "updateMeta" | "get" | "settleSession" + | "settleSessionReportingAbort" | "unsettleSession" | "setSettleOverride" | "snoozeSession" @@ -552,11 +553,20 @@ export function createCtoOperatorTools(deps: CtoOperatorToolDeps): Record { try { - const ok = deps.sessionService.settleSession(sessionId, { + const result = deps.sessionService.settleSessionReportingAbort(sessionId, { ...(outcome ? { outcome } : {}), source: "operator", }); - if (!ok) return { success: false, error: `Session not found: ${sessionId}` }; + if (!result.found) return { success: false, error: `Session not found: ${sessionId}` }; + if (!result.settled) { + // The session went active while the settle was in flight. Saying "not + // found" here would send the operator looking for a session that is + // sitting right there, working. + return { + success: false, + error: `Session is active again, so it was not settled: ${sessionId}`, + }; + } return { success: true, sessionId, ...readSessionLifecycle(deps, sessionId) }; } catch (error) { return { success: false, error: getErrorMessage(error) }; diff --git a/apps/desktop/src/main/services/prs/prAsync.test.ts b/apps/desktop/src/main/services/prs/prAsync.test.ts index fa22e0b7e..b2eaab7e5 100644 --- a/apps/desktop/src/main/services/prs/prAsync.test.ts +++ b/apps/desktop/src/main/services/prs/prAsync.test.ts @@ -1156,6 +1156,65 @@ describe("prMergeAutoSettlementService", () => { ); }); + it("holds the retry while the chat is mid-turn, and reads chat liveness not the row", async () => { + const db = createMemoryDb(); + const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ + settled: [] as string[], + aborted: ids.map((sessionId) => ({ sessionId, reason: "turn_start" })), + })); + // The persisted row of a chat holds `running` between turns on purpose. Any + // gate that reads it never sees the turn end — that is the bug this pins. + const get = vi.fn(() => ({ id: "chat-owned", status: "running", runtimeState: "running" })); + let chatStatus: "active" | "idle" = "active"; + const getChatLiveness = vi.fn(async () => ({ status: chatStatus, awaitingInput: false })); + const emitEvent = vi.fn(); + const service = createPrMergeAutoSettlementService({ + db: db as any, + sessionService: withSessionLookup({ + list: vi.fn(() => [ + { laneId: "lane-1", id: "chat-owned", toolType: "codex-chat", archivedAt: null, settledAt: null }, + ]), + get, + settleSessionsReportingAborts, + }) as any, + emitEvent, + getChatLiveness: getChatLiveness as any, + }); + + const openPr = createSummary({ state: "open" }); + await service.processSnapshot({ prs: [openPr], polledAt: "2026-03-24T12:00:00.000Z" }); + + const mergedPr = createSummary({ + state: "merged", + mergedAt: "2026-03-24T12:01:00.000Z", + chatSessionIds: ["chat-owned"], + }); + // The merge loses the race to a turn that just started. + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:01:05.000Z" }); + expect(settleSessionsReportingAborts).toHaveBeenCalledTimes(1); + + // Still mid-turn: the retry must not fire. With real teardown attached this + // is what would otherwise stop the work that won the race, once per poll. + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:31:00.000Z" }); + expect( + settleSessionsReportingAborts, + "a retry during an active turn would stop the work that won the race", + ).toHaveBeenCalledTimes(1); + + // The turn ends. The ROW still says running — only chat liveness moves — so + // a row-reading gate would stay stuck here forever. + chatStatus = "idle"; + settleSessionsReportingAborts.mockImplementation((ids: string[]) => ({ + settled: ids, + aborted: [] as Array<{ sessionId: string; reason: string }>, + })); + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T13:01:00.000Z" }); + expect( + settleSessionsReportingAborts, + "the merge must still be filed once the turn is genuinely over", + ).toHaveBeenCalledTimes(2); + expect(get, "chat liveness must not come from the persisted row").not.toHaveBeenCalled(); + }); it("settles a PR that was already merged when first seen, but announces nothing", async () => { // The machine-switch bug. Point the project tab at another machine and that // machine reconciles, backfilling rows for PRs it had never stored. Every diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index 96d73e667..226fb74fa 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -11,6 +11,7 @@ import { isTrackedAgentCliToolType, } from "../../../shared/types"; import { isChatToolType } from "../sessions/chatSessionProjection"; +import type { AgentChatSessionSummary } from "../../../shared/types"; function isMergeAtOrAfter(mergedAt: string | null | undefined, enabledSince: string): boolean { const mergedMs = Date.parse(mergedAt ?? ""); @@ -64,6 +65,23 @@ export function createPrMergeAutoSettlementService(args: { db: Pick; sessionService: Pick, "get" | "list" | "settleSessionsReportingAborts">; emitEvent: (event: PrEventPayload) => void; + /** + * Liveness for a CHAT session, straight from the chat service. + * + * A chat's persisted `terminal_sessions` row is not usable for this. It holds + * `status = "running"` between turns on purpose, and `runtimeState` is derived + * from that column, so a raw row read reports a finished chat as still + * running forever. `chatSessionProjection` is the only thing that resolves an + * idle chat, and it resolves it from these two fields. + * + * Injected as a narrow callback rather than the whole chat service, matching + * `chatMentionService.listChatSessions` and `laneTeardownDeps.agentChatService`. + * Absent (or returning null) means "not a chat" — the caller then falls back + * to the persisted row, which IS authoritative for tracked CLI sessions. + */ + getChatLiveness?: ( + sessionId: string, + ) => Promise | null>; }) { @@ -120,6 +138,32 @@ export function createPrMergeAutoSettlementService(args: { } }; + /** + * Sessions whose auto-settle was aborted by activity and that have not been + * seen at rest since. Service-scoped on purpose: the retry happens on a LATER + * poll, so a per-snapshot set would forget and retry unconditionally. + * + * In-memory only. After a restart the first poll retries once without waiting, + * which is the pre-existing behavior and is bounded by the abort itself: if the + * work is still running, the settle aborts again and re-arms this. + */ + const abortedSessionIds = new Set(); + + /** + * True when a session is not mid-turn, so a previously-aborted settle may + * retry. Chat liveness comes from the chat service; everything else reads the + * persisted row, which is authoritative for tracked CLI sessions. + */ + const isAtRest = async (sessionId: string): Promise => { + const chat = await args.getChatLiveness?.(sessionId).catch(() => null); + if (chat) return !chat.awaitingInput && chat.status !== "active"; + const row = args.sessionService.get(sessionId); + if (!row) return true; + const runtime = (row.runtimeState ?? "").toLowerCase(); + if (runtime) return runtime !== "running" && runtime !== "waiting-input"; + return (row.status ?? "").toLowerCase() !== "running"; + }; + const processSnapshot = async ({ prs, polledAt, @@ -196,22 +240,23 @@ export function createPrMergeAutoSettlementService(args: { // session even when it still owns scheduled work, a background task, // or another normal settlement blocker. Real activity can unsettle it // again, while handledPrIds prevents this PR from filing it twice. - // NOTE for step 3: this retries on the next poll, unconditionally. - // - // That is harmless while teardown is a no-op, and it is what this code - // did before the settling window existed — but once real teardown - // attaches, a retry against work that is still running would stop the - // very work that won the race, once per poll. Three gates were tried - // here and each was wrong in a different way: a lifecycle-revision check - // never re-arms (a turn COMPLETING does not move the settle tuple), a - // timer re-arms while a long turn is still running, and a raw - // `runtimeState` check never observes turn completion at all, because - // chat rows deliberately keep `status = "running"` between turns and - // only `chatSessionProjection` resolves an idle chat to `idle`. + // A settle that already lost this race does not retry while the work + // that won it is still going. Once step 3 attaches real teardown, an + // unconditional retry would stop that work, once per poll. // - // The correct gate therefore needs PROJECTED chat state, which means a - // dependency this poller does not have. Bounding it belongs with - // teardown, not before it. + // Three earlier gates were each wrong in a different way, so the + // predicate below is deliberately narrow: a lifecycle-revision check + // never re-arms (a turn COMPLETING does not touch the settle tuple), a + // timer re-arms mid-turn, and the persisted row is blind to chat + // liveness. Ask the chat service; fall back to the row only for the + // tracked CLI sessions whose row does not lie. + if (abortedSessionIds.has(session.id)) { + if (!(await isAtRest(session.id))) { + abandonedThisPr = true; + continue; + } + abortedSessionIds.delete(session.id); + } const settleResult = args.sessionService.settleSessionsReportingAborts([session.id], { outcome: `PR #${pr.githubPrNumber} merged`, settledAt: polledAt, @@ -223,6 +268,7 @@ export function createPrMergeAutoSettlementService(args: { // the PR unhandled is the point: a later pass retries, instead of this // merge being consumed by a settle that never landed. abandonedThisPr = true; + 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 022851585..acefb6414 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 { createSettleLifecycleWriter } from "./settleLifecycleWriter"; -import type { SettleAbortedSession, SettleSessionsOutcome, SettleTeardownCompleted } from "./settlingStateRegistry"; +import type { SettleAbortedReason, SettleAbortedSession, SettleSessionsOutcome, SettleTeardownCompleted } from "./settlingStateRegistry"; import type { ClaudeSessionPointer, SessionAttentionSource, @@ -767,7 +767,23 @@ export function createSessionService({ continue; } try { - runSettleTeardown?.(id); + let teardownThrew = false; + try { + runSettleTeardown?.(id); + } 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. + teardownThrew = true; + void error; + } + if (teardownThrew) { + aborted.push({ sessionId: id, reason: "teardown_failed" }); + continue; + } const abortedBy = settleLifecycle.settling.abortedBy(id); if (abortedBy) { @@ -781,12 +797,6 @@ export function createSessionService({ continue; } settled.push(...settleMany([id], options)); - } catch (error) { - // A throw must not discard the accounting for the rest of the batch, nor - // leave earlier sessions settled with no record of it. Report this id and - // carry on: a bulk caller always gets a full accounting. - aborted.push({ sessionId: id, reason: "teardown_failed" }); - void error; } finally { settleLifecycle.settling.end(id); } @@ -1523,12 +1533,47 @@ export function createSessionService({ [trimmed], ); if (!exists) return false; - settleManyWithTeardown([trimmed], { - outcome: normalizeSessionStatusNote(opts.outcome) ?? undefined, + // 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 = settleManyWithTeardown([trimmed], { + ...(note ? { outcome: note } : {}), settledAt: opts.settledAt, source: opts.source, }); - return true; + // 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; + }, + + /** + * `settleSession` with the abort reason kept, for callers that report WHY. + * 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( + sessionId: string, + opts: { outcome?: string | null; settledAt?: string; source?: SessionSettleSource } = {}, + ): { 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 = 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. */ diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index af9ceddef..7780fe752 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { openKvDb } from "../state/kvDb"; import { createSessionService } from "./sessionService"; +import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; import { settleTeardownCompleted } from "./settlingStateRegistry"; /** @@ -300,15 +301,23 @@ describe("settle race matrix (teardown is a no-op)", () => { * nothing about batching. */ it("refuses a multi-id clear rather than guessing one disposition for the batch", async () => { - const { service, create } = await fixture(); + const { service, db, create } = await fixture(); create("session-2"); service.settleSessions(["session-1", "session-2"]); - // The only multi-id clear the API offers is `unsettleSessions`, which is a - // DECLARED unsettle, not a `clearOnActivity` — so it is unaffected and both - // rows clear. + // The service API cannot express a multi-id `clearOnActivity`, so reach the + // writer directly — otherwise the refusal this test is named for is never + // executed and the test passes on unrelated behavior. + const writer = createSettleLifecycleWriter(db); + expect(() => + writer.write({ + intent: { kind: "clearOnActivity", cause: "turn_start" }, + sessionIds: ["session-1", "session-2"], + }), + ).toThrow(); + + // A declared multi-id unsettle is a different intent and stays allowed. service.unsettleSessions(["session-1", "session-2"]); - expect(service.get("session-1")?.settledAt).toBeNull(); expect(service.get("session-2")?.settledAt).toBeNull(); }); @@ -345,13 +354,79 @@ describe("settle race matrix (teardown is a no-op)", () => { const { service, setTeardown } = await fixture(); expect(service.settlingSessionIds()).toEqual([]); + // Captured, not asserted, inside the callback: a failing `expect` in there + // throws, the teardown catch turns it into `teardown_failed`, and the test + // stays green while proving nothing. + let duringWindow: string[] | null = null; setTeardown(() => { - expect(service.settlingSessionIds()).toEqual(["session-1"]); + duringWindow = service.settlingSessionIds(); }); - service.settleSessionsReportingAborts(["session-1"]); + const outcome = 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([ + "session-1", + ]); // Always ended, even on the abandoned path — a leaked window would make the // session permanently unsettleable. expect(service.settlingSessionIds()).toEqual([]); }); + it("keeps a persistence failure distinct from a failed teardown", async () => { + const { service, db, setTeardown } = await fixture(); + let stopped = 0; + setTeardown(() => { + stopped += 1; + }); + // Teardown succeeds; saving the row is what breaks. + // The settle tuple lands via `runChanged` (the bump needs the matched-row + // count), so patching `run` alone would sail straight past it. + const realRunChanged = db.runChanged.bind(db); + db.runChanged = ((sql: string, params?: unknown[]) => { + if (typeof sql === "string" && /update\s+terminal_sessions/i.test(sql)) { + throw new Error("database is locked"); + } + return realRunChanged(sql, params as never); + }) as typeof db.runChanged; + + // 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/); + 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. + expect(service.settlingSessionIds()).toEqual([]); + }); + + 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); + service.unsettleSession("session-1"); + + // Re-settling with no outcome must leave the previous note alone. + expect(service.settleSession("session-1")).toBe(true); + const row = db.get<{ status_note: string | null }>( + "select status_note from terminal_sessions where id = ?", + ["session-1"], + ); + expect(row?.status_note).toBe("shipped the fix"); + }); + + it("tells an aborted single-session settle apart from a missing one", async () => { + const { service, setTeardown } = await fixture(); + // A turn starts while the settle is mid-teardown, exactly as in R1. + setTeardown(() => { + service.clearTurnStartMarkers("session-1"); + }); + + // The boolean form must not claim success... + expect(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"); + expect(aborted).toEqual({ found: true, settled: false, abortedBy: "turn_start" }); + expect(service.settleSessionReportingAbort("no-such-session")).toEqual({ + found: false, + settled: false, + }); + }); }); diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index d5ff96662..95d2a2849 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -433,9 +433,12 @@ 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, which this poller has -no dependency on today. Wiring that dependency is step-3 scope; do not re-attempt -a gate that reads the raw persisted row. +The workable signal is therefore **projected** chat state. Step 2 wires it 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 +back to the persisted row. Do not re-attempt a gate that reads the raw row for a +chat. ### 3d. When teardown cannot confirm From f97aa8b331cc22eb100be9f6bf78a9ff8bd91ffd Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 02:56:15 -0400 Subject: [PATCH 09/10] fix: three more review findings on abort handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P1 (prMergeAutoSettlement): a teardown failure was parked behind the same inactivity gate as an activity abort. But teardown_failed means the stop failed while the work kept running, so waiting for the work to stop means waiting on the thing that needs stopping — the PR would stay unhandled and the work never stopped again until restart. Only turn_start, turn_failed and attention_requested wait now; the rest retry on the next poll. Codex P1 + CodeRabbit (settleTerminalSession, ctoOperatorTools): a false return was rendered as "Session was not found" by the IPC, action and sync handlers, which sends the user looking for a row that is sitting right there working. settleTerminalSession now throws a typed SettleAbortedError with a per-reason sentence, and the CTO tool reports the actual reason instead of always saying the session went active — teardown_failed and joined_in_flight do not mean that. Peer reactivation (Codex, sessionService): NOT fixed here, deliberately. It is the R7 peer-bypass gap the coordinator scoped to the step-3 review, and R7/R7b pin its observed blast radius rather than an assumed one. CodeRabbit's remaining four are stale re-posts against earlier commits: the RETRY_AFTER_ABORT_MS cooldown no longer exists, and both vacuous tests were fixed in the previous commit. --- .../services/ai/tools/ctoOperatorTools.ts | 11 ++--- .../src/main/services/prs/prAsync.test.ts | 42 +++++++++++++++++++ .../prs/prMergeAutoSettlementService.ts | 16 ++++++- .../sessions/settleTerminalSession.ts | 34 ++++++++++++++- 4 files changed, 94 insertions(+), 9 deletions(-) diff --git a/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts b/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts index cf48ff23c..ee5c621c2 100644 --- a/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts +++ b/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts @@ -29,6 +29,7 @@ import type { createCtoStateService } from "../../cto/ctoStateService"; import type { CtoMemoryService } from "../../cto/ctoMemoryService"; import { getErrorMessage, nowIso, parseIsoToEpoch } from "../../shared/utils"; import { buildAdePrUrl } from "../../../../shared/deeplinks"; +import { settleAbortMessage } from "../../sessions/settleTerminalSession"; export interface CtoOperatorToolDeps { currentSessionId: string; @@ -559,13 +560,9 @@ export function createCtoOperatorTools(deps: CtoOperatorToolDeps): Record { ); }); + it("retries a teardown failure immediately instead of waiting for the work to stop", async () => { + const db = createMemoryDb(); + // The stop failed and the work is still running — which is precisely why an + // inactivity gate must not apply: it would wait on the thing it must stop. + const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ + settled: [] as string[], + aborted: ids.map((sessionId) => ({ sessionId, reason: "teardown_failed" })), + })); + const getChatLiveness = vi.fn(async () => ({ status: "active" as const, awaitingInput: false })); + const emitEvent = vi.fn(); + const service = createPrMergeAutoSettlementService({ + db: db as any, + sessionService: withSessionLookup({ + list: vi.fn(() => [ + { laneId: "lane-1", id: "chat-owned", toolType: "codex-chat", archivedAt: null, settledAt: null }, + ]), + get: vi.fn(() => null), + settleSessionsReportingAborts, + }) as any, + emitEvent, + getChatLiveness: getChatLiveness as any, + }); + + await service.processSnapshot({ + prs: [createSummary({ state: "open" })], + polledAt: "2026-03-24T12:00:00.000Z", + }); + const mergedPr = createSummary({ + state: "merged", + mergedAt: "2026-03-24T12:01:00.000Z", + chatSessionIds: ["chat-owned"], + }); + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:01:05.000Z" }); + await service.processSnapshot({ prs: [mergedPr], polledAt: "2026-03-24T12:31:00.000Z" }); + + expect( + settleSessionsReportingAborts, + "a failed stop must be attempted again, not parked behind the running work", + ).toHaveBeenCalledTimes(2); + expect(getChatLiveness, "a teardown failure must not consult the activity gate").not.toHaveBeenCalled(); + }); + it("holds the retry while the chat is mid-turn, and reads chat liveness not the row", async () => { const db = createMemoryDb(); const settleSessionsReportingAborts = vi.fn((ids: string[]) => ({ diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index 226fb74fa..856557a57 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -11,6 +11,13 @@ import { isTrackedAgentCliToolType, } from "../../../shared/types"; import { isChatToolType } from "../sessions/chatSessionProjection"; + +/** + * The abort reasons that mean "something is running right now". Only these make + * a retry wait: the others either resolve on their own or, in the case of + * `teardown_failed`, describe work that is still running and still needs stopping. + */ +const ACTIVITY_ABORTS = new Set(["turn_start", "turn_failed", "attention_requested"]); import type { AgentChatSessionSummary } from "../../../shared/types"; function isMergeAtOrAfter(mergedAt: string | null | undefined, enabledSince: string): boolean { @@ -268,7 +275,14 @@ export function createPrMergeAutoSettlementService(args: { // the PR unhandled is the point: a later pass retries, instead of this // merge being consumed by a settle that never landed. abandonedThisPr = true; - abortedSessionIds.add(session.id); + // 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. + if (settleResult.aborted.some((entry) => ACTIVITY_ABORTS.has(entry.reason))) { + abortedSessionIds.add(session.id); + } } } diff --git a/apps/desktop/src/main/services/sessions/settleTerminalSession.ts b/apps/desktop/src/main/services/sessions/settleTerminalSession.ts index d1b28df05..c942f9169 100644 --- a/apps/desktop/src/main/services/sessions/settleTerminalSession.ts +++ b/apps/desktop/src/main/services/sessions/settleTerminalSession.ts @@ -4,6 +4,32 @@ import type { createSessionService } from "./sessionService"; import type { SessionSettleSource } from "../../../shared/types"; import { isChatToolType } from "./chatSessionProjection"; +/** A settle that reached the row but was refused by the settling window. */ +export class SettleAbortedError extends Error { + constructor(readonly sessionId: string, readonly abortedBy?: string) { + super(settleAbortMessage(sessionId, abortedBy)); + this.name = "SettleAbortedError"; + } +} + +/** One sentence a user can act on, per abort reason. */ +export function settleAbortMessage(sessionId: string, abortedBy?: string): string { + switch (abortedBy) { + case "turn_start": + return `Session '${sessionId}' started working again, so it was not settled.`; + case "turn_failed": + return `Session '${sessionId}' reported a failed turn, so it was not settled.`; + case "attention_requested": + return `Session '${sessionId}' asked for attention, so it was not settled.`; + case "teardown_failed": + return `Session '${sessionId}' could not be stopped cleanly, so it was not settled.`; + case "joined_in_flight": + return `Session '${sessionId}' is already being settled.`; + default: + return `Session '${sessionId}' changed while it was being settled, so it was not settled.`; + } +} + export type SettleTerminalSessionOptions = { outcome?: string; dismissPendingInput?: boolean; @@ -62,11 +88,17 @@ export async function settleTerminalSession(args: { if (!dismissed) return false; } - return args.sessionService.settleSession( + const result = args.sessionService.settleSessionReportingAbort( args.sessionId, { ...(args.opts?.outcome ? { outcome: args.opts.outcome } : {}), ...(args.opts?.source ? { source: args.opts.source } : {}), }, ); + if (result.settled) return true; + // An abort is NOT "no such session". Callers turned a bare `false` into + // "Session was not found", which sends the user looking for a row that is + // sitting right there, working. + if (result.found) throw new SettleAbortedError(args.sessionId, result.abortedBy); + return false; } From 27b8f143a518adb7094e95e1fefab3369ceca899 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Tue, 11 Aug 2026 03:02:29 -0400 Subject: [PATCH 10/10] test: point the settle mocks and assertions at the abort-aware call settleTerminalSession and the CTO tool now call settleSessionReportingAbort, so the registry and sync-command mocks that only stubbed settleSession threw on it. Adds the method to both and moves the call assertions across. Caught by CI shard 2, not locally: the earlier run covered prs/sessions/ ai-tools/ipc but not adeActions or the CLI sync suite. --- .../src/services/sync/syncRemoteCommandService.test.ts | 5 +++-- .../src/main/services/adeActions/registry.test.ts | 9 +++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts b/apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts index d2a4e3afc..e4812dc46 100644 --- a/apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts +++ b/apps/ade-cli/src/services/sync/syncRemoteCommandService.test.ts @@ -2344,6 +2344,7 @@ describe("session lifecycle remote commands", () => { }) { const sessionService = { settleSession: vi.fn(() => true), + settleSessionReportingAbort: vi.fn(() => ({ found: true, settled: true })), unsettleSession: vi.fn(() => true), settleSessions: vi.fn(() => ["session-1"]), unsettleSessions: vi.fn(), @@ -2369,7 +2370,7 @@ describe("session lifecycle remote commands", () => { sessionId: "session-1", outcome: "PR #841 merged", }))).resolves.toEqual({ ok: true, sessionId: "session-1" }); - expect(sessionService.settleSession).toHaveBeenCalledWith("session-1", { outcome: "PR #841 merged" }); + expect(sessionService.settleSessionReportingAbort).toHaveBeenCalledWith("session-1", { outcome: "PR #841 merged" }); await expect(service.execute(makePayload("session.unsettleSession", { sessionId: "session-1" }))) .resolves.toEqual({ ok: true, sessionId: "session-1" }); @@ -2393,7 +2394,7 @@ describe("session lifecycle remote commands", () => { }))).resolves.toEqual({ ok: true, sessionId: "session-1" }); expect(handleSessionSettled).not.toHaveBeenCalled(); - expect(sessionService.settleSession).toHaveBeenCalledWith("session-1", {}); + expect(sessionService.settleSessionReportingAbort).toHaveBeenCalledWith("session-1", {}); }); // `dismissPendingInputBeforeSettle` mutates and can then throw for a row with diff --git a/apps/desktop/src/main/services/adeActions/registry.test.ts b/apps/desktop/src/main/services/adeActions/registry.test.ts index c4d218e10..5643bdc7b 100644 --- a/apps/desktop/src/main/services/adeActions/registry.test.ts +++ b/apps/desktop/src/main/services/adeActions/registry.test.ts @@ -1624,6 +1624,7 @@ describe("runtime session actions", () => { const requestAttention = vi.fn(() => true); const setStatusNote = vi.fn(() => true); const settleSession = vi.fn(() => true); + const settleSessionReportingAbort = vi.fn(() => ({ found: true, settled: true })); const unsettleSession = vi.fn(() => true); const markSessionAttentionRequested = vi.fn(); const setSessionRuntimeState = vi.fn(() => true); @@ -1642,6 +1643,7 @@ describe("runtime session actions", () => { requestAttention, setStatusNote, settleSession, + settleSessionReportingAbort, unsettleSession, }, ptyService: { @@ -1853,11 +1855,13 @@ describe("runtime session actions", () => { it("dismisses pending chat input before settling through the session action", async () => { const dismissPendingInputForSettlement = vi.fn(async () => undefined); const settleSession = vi.fn(() => true); + const settleSessionReportingAbort = vi.fn(() => ({ found: true, settled: true })); const runtime = { sessionService: { get: vi.fn(() => ({ id: "chat-1", toolType: "codex-chat" })), list: vi.fn(), settleSession, + settleSessionReportingAbort, }, agentChatService: { dismissPendingInputForSettlement, @@ -1875,14 +1879,15 @@ describe("runtime session actions", () => { dismissPendingInput: true, })).resolves.toEqual({ ok: true, sessionId: "chat-1" }); expect(dismissPendingInputForSettlement).toHaveBeenCalledWith({ sessionId: "chat-1" }); - expect(settleSession).toHaveBeenCalledWith("chat-1", { source: "user" }); + expect(settleSessionReportingAbort).toHaveBeenCalledWith("chat-1", { source: "user" }); expect(dismissPendingInputForSettlement.mock.invocationCallOrder[0]).toBeLessThan( - settleSession.mock.invocationCallOrder[0]!, + settleSessionReportingAbort.mock.invocationCallOrder[0]!, ); }); it("does not pretend a native CLI prompt was dismissed while its process is still blocked", async () => { const settleSession = vi.fn(() => true); + const settleSessionReportingAbort = vi.fn(() => ({ found: true, settled: true })); const setSessionRuntimeState = vi.fn(() => true); const runtime = { sessionService: {