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/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/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: { 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..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; @@ -49,6 +50,7 @@ export interface CtoOperatorToolDeps { | "updateMeta" | "get" | "settleSession" + | "settleSessionReportingAbort" | "unsettleSession" | "setSettleOverride" | "snoozeSession" @@ -552,11 +554,16 @@ 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 reason matters: `teardown_failed` and `joined_in_flight` do not + // mean the session went active, and saying so misdirects the operator. + return { success: false, error: settleAbortMessage(sessionId, result.abortedBy) }; + } 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 a586a2123..4ab1f470d 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,173 @@ 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 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, + }); + + 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 }>, + })); + // 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, + "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").toHaveBeenCalledWith( + expect.objectContaining({ type: "pr-sessions-auto-settled" }), + ); + }); + + 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[]) => ({ + 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 @@ -1118,7 +1267,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 +1279,7 @@ describe("prMergeAutoSettlementService", () => { settledAt: null, }]), get: vi.fn(() => null), - settleSessionsWithOutcome, + settleSessionsReportingAborts, }) as any, emitEvent, }); @@ -1157,7 +1306,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 +1332,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 +1343,7 @@ describe("prMergeAutoSettlementService", () => { settledAt: null, }]), get: vi.fn(() => null), - settleSessionsWithOutcome, + settleSessionsReportingAborts, }) as any, emitEvent: vi.fn(), }); @@ -1218,7 +1367,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 +1387,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 +1409,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 +1446,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 +1459,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 +1478,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 +1496,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 +1525,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..856557a57 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -12,6 +12,14 @@ import { } 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 { const mergedMs = Date.parse(mergedAt ?? ""); const enabledMs = Date.parse(enabledSince); @@ -62,9 +70,28 @@ 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; + /** + * 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>; }) { + + /** * 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. @@ -118,6 +145,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, @@ -129,11 +182,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,6 +231,7 @@ export function createPrMergeAutoSettlementService(args: { ); const settledSessionIds: string[] = []; + let abandonedThisPr = false; for (const session of rows) { const currentSettings = getSessionLifecycleSettings(args.db); const currentState = getPrMergeAutoSettlementState(args.db); @@ -185,15 +247,50 @@ 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", - )); + // 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. + // + // 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, + 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. + abandonedThisPr = true; + // 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); + } + } } 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. + 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: @@ -201,7 +298,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.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 b5901ac7d..acefb6414 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 { SettleAbortedReason, SettleAbortedSession, SettleSessionsOutcome, SettleTeardownCompleted } 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) => SettleTeardownCompleted; +}) { const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); @@ -718,6 +733,78 @@ 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") { + // 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 { + 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) { + 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 +1394,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 +1420,7 @@ export function createSessionService({ db }: { db: AdeDb }) { return; } writeSettleLifecycle({ - intent: { kind: "clearOnActivity" }, + intent: { kind: "clearOnActivity", cause: "mechanical" }, extraSet: { last_output_at: at }, sessionIds: [sessionId], }); @@ -1432,22 +1519,61 @@ export function createSessionService({ db }: { db: AdeDb }) { 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; + // 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, }); + // 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. */ @@ -1511,16 +1637,28 @@ export function createSessionService({ db }: { db: AdeDb }) { }, settleSessions(sessionIds: string[]): string[] { - return settleMany(sessionIds); + return settleManyWithTeardown(sessionIds).settled; }, - settleSessionsWithOutcome( + /** + * 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[], - outcome: string, - settledAt: string = new Date().toISOString(), - source: SessionSettleSource = "user", - ): string[] { - return settleMany(sessionIds, { outcome, settledAt, source }); + 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(); }, unsettleSessions(sessionIds: string[]): void { @@ -1677,7 +1815,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 +1844,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 +1869,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..02a0c69ec 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,68 @@ 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. + */ + /** + * 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. + * + * 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] }; + if (sessionIds.length > 1) { + throw new Error( + "clearOnActivity is single-session by construction; a multi-id clear would need a per-session settling disposition.", + ); + } + 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] }; + }; + const writeSettleLifecycle = (args: { intent: SettleLifecycleIntent; sessionIds: readonly string[]; @@ -188,18 +272,23 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { }): void => { const ids = args.sessionIds.map((id) => id.trim()).filter(Boolean); if (!ids.length) 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. @@ -220,13 +309,14 @@ 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); }; return { write: writeSettleLifecycle, readRevision: readLifecycleRevision, + settling, forget: (sessionId: string) => { const trimmed = sessionId.trim(); if (!trimmed) return; @@ -236,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 new file mode 100644 index 000000000..7780fe752 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -0,0 +1,432 @@ +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"; +import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; +import { settleTeardownCompleted } from "./settlingStateRegistry"; + +/** + * 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); + return settleTeardownCompleted(); + }, + }); + 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(); + // 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([]); + }); + + /** + * 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 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" }], + }); + }); + + /** + * 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([]); + }); + + /** + * `clearOnActivity` is single-session by construction, and the writer now says + * so rather than carrying a per-session partition no test could reach. + * + * 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("refuses a multi-id clear rather than guessing one disposition for the batch", async () => { + const { service, db, create } = await fixture(); + create("session-2"); + service.settleSessions(["session-1", "session-2"]); + + // 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(); + }); + + /** + * 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 is now a COMPILE error, not a runtime one — the seam + * returns a branded value that only a synchronous body can produce, so both + * `async (id) => {}` and the adapter `id => asyncStop(id)` fail to typecheck. + * There is no runtime behaviour left to assert; the type is the test. + */ + + /** A settling row found after a restart resolves to not-settled. */ + it("crash safety: the settling window does not survive the process", async () => { + const { service, setTeardown } = await fixture(); + 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(() => { + duringWindow = service.settlingSessionIds(); + }); + 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/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; } 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..56efe7e31 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts @@ -0,0 +1,128 @@ +/** + * 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. + */ + +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"; + +/** Why a settle was abandoned, as reported to callers. */ +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; + 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..95d2a2849 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.** @@ -392,6 +414,32 @@ 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. 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 R5 is a product decision, not a mechanism. If a provider stop is unavailable, @@ -472,10 +520,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.