diff --git a/apps/ade-cli/src/bootstrap.ts b/apps/ade-cli/src/bootstrap.ts index d09b68a91..a3b0d8a27 100644 --- a/apps/ade-cli/src/bootstrap.ts +++ b/apps/ade-cli/src/bootstrap.ts @@ -26,6 +26,12 @@ import { createSessionService, STALE_RUNNING_SESSION_FRESH_ACTIVITY_GRACE_MS, } from "../../desktop/src/main/services/sessions/sessionService"; +import { createSettleTeardownWiring } from "../../desktop/src/main/services/sessions/settleTeardownWiring"; +import type { + SettleResidueItem, + SettleTeardownContext, + SettleTeardownOutcome, +} from "../../desktop/src/main/services/sessions/sessionSettleTeardown"; import { createProjectConfigService } from "../../desktop/src/main/services/config/projectConfigService"; import { createConflictService } from "../../desktop/src/main/services/conflicts/conflictService"; import { createGitOperationsService } from "../../desktop/src/main/services/git/gitOperationsService"; @@ -755,7 +761,30 @@ export async function createAdeRuntime(args: { // services. Session changes still use it once publishing is attached. let pushPublisherForPtySignals: PushPublisherService | null = null; let ptyServiceForSessionChanges: ReturnType | null = null; - const sessionService = createSessionService({ db }); + // Late-bound: the chat service that owns the work is constructed further + // down. Without this the brain — which owns phone sync, remote commands and + // the PR-merge poller in a normal install — would settle sessions while + // stopping nothing. + const settleTeardownRef: { + run: ((sessionId: string, ctx: SettleTeardownContext) => Promise) | null; + report: ((args: { columns: string[]; changesetSessionCount: number }) => void) | null; + residue: ((args: { provider: string | null; items: SettleResidueItem[] }) => void) | null; + } = { run: null, report: null, residue: null }; + const sessionService = createSessionService({ + db, + runSettleTeardown: async (sessionId, ctx) => + settleTeardownRef.run ? await settleTeardownRef.run(sessionId, ctx) : { residue: [], confirmed: false }, + onRemoteSettleWrite: (args) => settleTeardownRef.report?.(args), + onSettleResidue: (args) => settleTeardownRef.residue?.(args), + }); + // Inbound settle-tuple writes get this host's lifecycle revision, so an + // in-flight settle can see a peer's decision and abandon rather than + // overwrite it. Registered here because the DB layer must not know what a + // settle means — and because the brain, not the desktop, is where changesets + // are actually applied in a normal install. + db.sync.setRemoteSettleTupleHandler((changes) => { + sessionService.reconcileRemoteSettleTuple(changes); + }); sessionService.onChanged((event) => { pushEvent("runtime", { type: "terminal_session_changed", event }); const session = sessionService.get(event.sessionId); @@ -1249,6 +1278,16 @@ export async function createAdeRuntime(args: { countActiveForLane: (laneId) => agentChatService.countActiveForLane(laneId), disposeForLane: (laneId) => agentChatService.disposeForLane(laneId), }; + const settleWiring = createSettleTeardownWiring({ + agentChatService, + logger, + analytics: productAnalyticsService ?? null, + // The brain is the non-GUI runtime surface, matching its other analytics. + surface: "api", + }); + settleTeardownRef.run = settleWiring.runSettleTeardown; + settleTeardownRef.report = settleWiring.onRemoteSettleWrite; + settleTeardownRef.residue = settleWiring.onSettleResidue; } autoRebaseActivityReady = true; void autoRebaseService diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index 58f07ca65..df0b85a30 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -85,6 +85,8 @@ import { releaseLaneRuntimeResources } from "./services/lanes/laneRuntimeLifecyc import { createOAuthRedirectService } from "./services/lanes/oauthRedirectService"; import { createRuntimeDiagnosticsService } from "./services/lanes/runtimeDiagnosticsService"; import { createSessionService } from "./services/sessions/sessionService"; +import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } from "./services/sessions/sessionSettleTeardown"; +import { createSettleTeardownWiring } from "./services/sessions/settleTeardownWiring"; import { createSessionDeltaService } from "./services/sessions/sessionDeltaService"; import { createPtyService } from "./services/pty/ptyService"; import { createSupervisedPtyLoader } from "./services/pty/supervisedPtyHost"; @@ -2870,10 +2872,34 @@ app.whenReady().then(async () => { emitProjectEvent(projectRoot, IPC.lanesEnvEvent, ev), }); - const sessionService = createSessionService({ db }); + // Late-bound: the chat service that owns the work does not exist yet at + // this point, and the settle path must not depend on construction order. + const settleTeardownRef: { + run: ((sessionId: string, ctx: SettleTeardownContext) => Promise) | null; + report: ((args: { columns: string[]; changesetSessionCount: number }) => void) | null; + residue: ((args: { provider: string | null; items: SettleResidueItem[] }) => void) | null; + } = { run: null, report: null, residue: null }; + const sessionService = createSessionService({ + db, + onRemoteSettleWrite: (args) => settleTeardownRef.report?.(args), + onSettleResidue: (args) => settleTeardownRef.residue?.(args), + runSettleTeardown: async (sessionId, ctx) => + settleTeardownRef.run + ? await settleTeardownRef.run(sessionId, ctx) + // Before the chat service is up there is no background work to stop, + // so an empty teardown is the honest answer, not a skipped one. + : { residue: [], confirmed: false }, + }); sessionService.onChanged((event) => { emitProjectEvent(projectRoot, IPC.sessionsChanged, event); }); + // Inbound settle-tuple writes go through the chokepoint instead of landing + // raw, so a peer's decision gains this host's revision, settling window and + // abort semantics (R7). Registered here because the DB layer must not know + // what a settle means. + db.sync.setRemoteSettleTupleHandler((changes) => { + sessionService.reconcileRemoteSettleTuple(changes); + }); const processRegistry = createProcessRegistryService({ db, logger, @@ -3600,6 +3626,17 @@ app.whenReady().then(async () => { countActiveForLane: (laneId) => agentChatService.countActiveForLane(laneId), disposeForLane: (laneId) => agentChatService.disposeForLane(laneId), }; + { + const wiring = createSettleTeardownWiring({ + agentChatService, + logger, + analytics: productAnalyticsService ?? null, + surface: "desktop", + }); + settleTeardownRef.run = wiring.runSettleTeardown; + settleTeardownRef.report = wiring.onRemoteSettleWrite; + settleTeardownRef.residue = wiring.onSettleResidue; + } autoRebaseActivityReady = true; void autoRebaseService .refreshActiveRebaseNeeds("activity_services_ready") diff --git a/apps/desktop/src/main/services/adeActions/registry.test.ts b/apps/desktop/src/main/services/adeActions/registry.test.ts index 5643bdc7b..11b2b39d2 100644 --- a/apps/desktop/src/main/services/adeActions/registry.test.ts +++ b/apps/desktop/src/main/services/adeActions/registry.test.ts @@ -75,6 +75,11 @@ describe("isAllowedAdeAction", () => { expect(isAllowedAdeAction("session", "requestSessionAttention")).toBe(true); expect(isAllowedAdeAction("session", "setSessionStatusNote")).toBe(true); expect(isAllowedAdeAction("session", "settleSession")).toBe(true); + // The residue read path. It was added to the CTO-only list but NOT to the + // allowlist, which silently refused every call — and left the settle design + // claiming a user-visible guarantee ("settled never quietly means something + // is still running") that nothing could actually reach. + expect(isAllowedAdeAction("session", "getSettleResidue")).toBe(true); expect(isAllowedAdeAction("session", "unsettleSession")).toBe(true); expect(isCtoOnlyAdeAction("session", "settleSession")).toBe(true); expect(isCtoOnlyAdeAction("session", "unsettleSession")).toBe(true); diff --git a/apps/desktop/src/main/services/adeActions/registry.ts b/apps/desktop/src/main/services/adeActions/registry.ts index a62661ad3..506518b40 100644 --- a/apps/desktop/src/main/services/adeActions/registry.ts +++ b/apps/desktop/src/main/services/adeActions/registry.ts @@ -734,6 +734,7 @@ export const ADE_ACTION_ALLOWLIST: Partial { + const record = readObjectActionArg(args, "session.getSettleResidue"); + const sessionId = typeof record.sessionId === "string" ? record.sessionId : ""; + if (!sessionId) throw new Error("session.getSettleResidue requires sessionId."); + return sessionService.getSettleResidue(sessionId) ?? { recordedAt: null, items: [] }; + }, // ----------------------------------------------------------------------- // Snooze / wake / settle-override. Snooze is a synced VISIBILITY overlay: // it hides a row until its deadline without touching lifecycle columns, so diff --git a/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts b/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts index ee5c621c2..1fad42891 100644 --- a/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts +++ b/apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts @@ -554,7 +554,7 @@ export function createCtoOperatorTools(deps: CtoOperatorToolDeps): Record { try { - const result = deps.sessionService.settleSessionReportingAbort(sessionId, { + const result = await deps.sessionService.settleSessionReportingAbort(sessionId, { ...(outcome ? { outcome } : {}), source: "operator", }); diff --git a/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts b/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts index bc67543d3..ea338cd28 100644 --- a/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts +++ b/apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts @@ -93,7 +93,7 @@ const STRING_PROPERTIES = new Set([ "duration_bucket", "error_kind", "route_kind", "connection_state", "drop_reason", "source", "mode", "entry_point", "release_channel", "summary_kind", "reason", "last_command", "leg", "code", "escalation_reason", "install_source", "trigger", "from_version", "to_version", "user_action", - "tool_error_kind", "crash_reason", + "tool_error_kind", "crash_reason", "count_bucket", ]); const NUMBER_PROPERTIES = new Set([ "sent_count", "dropped_count", "interaction_count", "session_count", "chat_session_count", @@ -120,6 +120,10 @@ const ANALYTICS_ONLY_ACTIONS = new Set([ "mention_expanded", "transaction_failed", "scope_selected", + // Settle teardown: work a settle could not confirm it stopped, and a peer + // settle-tuple write that had to be reconciled through the chokepoint. + "settle_teardown_residue", + "settle_remote_write_reconciled", ]); const EVENT_PROPERTY_KEYS: Record> = { @@ -132,7 +136,7 @@ const EVENT_PROPERTY_KEYS: Record ade_project_opened: new Set(["route_kind", "source", "mode", "connection_state"]), ade_feature_used: new Set([ "feature", "action", "outcome", "source", "mode", "provider", "model_family", "duration_bucket", "connection_state", - "bytes_freed", "files_compressed", + "bytes_freed", "files_compressed", "count_bucket", ]), ade_work_session_started: new Set(["feature", "action", "outcome", "source", "mode", "provider"]), ade_work_session_completed: new Set([ @@ -183,6 +187,9 @@ const SAFE_STRING_VALUES: Partial>> = { outcome: new Set([ "success", "started", "completed", "failure", "timeout", "opened", "cancelled", "approved", "denied", "partial", "failed", "idle_only", "immediate", + // Settle teardown could not confirm a stop (design 3d). `timeout` above + // covers the third case. Coarse on purpose: never the task or its error. + "no_stop_control", "rejected", // Which half of a post-update transaction did not land. `swap` is // deliberately absent: the app half is already reported by // `ade_update_install_did_not_land`, so only the brain half is new signal. @@ -194,12 +201,15 @@ const SAFE_STRING_VALUES: Partial>> = { // widened, so the scope control can never carry free text. "machine", "project", "account", ]), - provider: new Set(["codex", "openai", "claude", "cursor", "droid", "opencode", "pi", "gemini", "local", "other"]), + provider: new Set(["codex", "openai", "claude", "cursor", "droid", "opencode", "pi", "gemini", "lmstudio", "local", "other"]), model_family: new Set([ "gpt_5", "openai_reasoning", "claude_sonnet", "claude_opus", "claude_haiku", "cursor", "gemini", "grok", "local", "other", ]), duration_bucket: new Set(["under_10s", "under_1m", "under_5m", "under_30m", "under_2h", "over_2h"]), + // Bucketed, never a raw count: a fleet that fails to stop must not become a + // high-cardinality dimension. + count_bucket: new Set(["1", "2_5", "6_plus"]), route_kind: new Set(["desktop", "web"]), connection_state: new Set(["connected", "disconnected", "pairing", "direct", "relay", "error"]), drop_reason: new Set([ diff --git a/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts b/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts index 7eead108f..facae2c2c 100644 --- a/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts +++ b/apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts @@ -1213,6 +1213,42 @@ describe("product analytics producers", () => { })).toMatchObject({ provider: "pi" }); }); + it("keeps the settle-teardown properties through the sanitizer", () => { + // Both of these were silently dropped when first added: `action` is + // allowlisted separately from the event's key list, and `count_bucket` was + // registered in the key list and the value allowlist but never in the + // string-dispatch set, so it never reached either. The event still shipped, + // just anonymous — which is worse than not shipping, because the dashboard + // looks populated. + expect(sanitizeProductAnalyticsProperties("ade_feature_used", { + feature: "work", + action: "settle_teardown_residue", + outcome: "no_stop_control", + provider: "codex", + count_bucket: "2_5", + })).toEqual({ + feature: "work", + action: "settle_teardown_residue", + outcome: "no_stop_control", + provider: "codex", + count_bucket: "2_5", + }); + + expect(sanitizeProductAnalyticsProperties("ade_feature_used", { + feature: "work", + action: "settle_remote_write_reconciled", + outcome: "partial", + })).toMatchObject({ action: "settle_remote_write_reconciled" }); + + // The bucket is still a closed set: a raw count must not slip through and + // widen the dimension. + expect(sanitizeProductAnalyticsProperties("ade_feature_used", { + feature: "work", + action: "settle_teardown_residue", + count_bucket: "37", + })).not.toHaveProperty("count_bucket"); + }); + it("maps automation completion and failed chat turns into canonical bounded outcomes", () => { const captures: ProductAnalyticsCapture[] = []; const analytics = settledAnalytics(captures); diff --git a/apps/desktop/src/main/services/chat/agentChatService.ts b/apps/desktop/src/main/services/chat/agentChatService.ts index cb0f52efc..111d959e4 100644 --- a/apps/desktop/src/main/services/chat/agentChatService.ts +++ b/apps/desktop/src/main/services/chat/agentChatService.ts @@ -37495,7 +37495,13 @@ export function createAgentChatService(args: { } catch { // Ignore provider abort failures; SSE cancellation still tears the turn down. } - cancelQueuedSteers(managed, managed.runtime, "interrupted"); + // `stop_only` exists so settle teardown can stop a turn WITHOUT + // discarding the user's queued follow-ups. Only the Claude path honoured + // it, so a settle on these providers silently deleted queued prompts — + // unrecoverable, and the opposite of the rule that losing a settle costs + // one click while losing the user's work does not. Default is + // `stop_and_clear`, so the Stop button is unaffected. + if (mode === "stop_and_clear") cancelQueuedSteers(managed, managed.runtime, "interrupted"); persistChatState(managed); for (const pending of managed.runtime.pendingApprovals.values()) { managed.runtime.handle.client.postSessionIdPermissionsPermissionId({ @@ -37541,7 +37547,7 @@ export function createAgentChatService(args: { cancelCursorPermissionWaiter(w, "Cursor tool approval was cancelled because the turn was interrupted."); } rt.permissionWaiters.clear(); - cancelQueuedSteers(managed, rt, "interrupted"); + if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted"); return result; } @@ -37554,7 +37560,7 @@ export function createAgentChatService(args: { } catch { // ignore } - cancelQueuedSteers(managed, rt, "interrupted"); + if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted"); cancelPendingPiInputs(managed); persistChatState(managed); return result; @@ -37562,7 +37568,9 @@ export function createAgentChatService(args: { if (managed.session.provider === "pi") { piRuntimeSetupInterruptRequested.set(managed, true); - cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + if (mode === "stop_and_clear") { + cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + } setSessionIdle(managed); persistChatState(managed); return result; @@ -37580,20 +37588,24 @@ export function createAgentChatService(args: { cancelDroidPermissionWaiter(w, "Droid tool approval was cancelled because the turn was interrupted."); } rt.permissionWaiters.clear(); - cancelQueuedSteers(managed, rt, "interrupted"); + if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted"); return result; } if (managed.session.provider === "droid") { droidRuntimeSetupInterruptRequested.set(managed, true); - cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + if (mode === "stop_and_clear") { + cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + } persistChatState(managed); return result; } if (managed.session.provider === "cursor") { cursorRuntimeSetupInterruptRequested.set(managed, true); - cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + if (mode === "stop_and_clear") { + cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted"); + } persistChatState(managed); return result; } @@ -44283,6 +44295,34 @@ export function createAgentChatService(args: { dispatchSteer, cancelDispatchedSteer, interrupt, + /** + * Is a persisted Claude `--bg` job actually still running? + * + * `claudeBackgroundJobShort` is a RECORD, not a liveness signal — it stays + * on the session after the job finishes or is stopped. Settle teardown has + * to distinguish the two: counting a finished job as work makes every later + * settle spend the confirmation budget and then report residue that does + * not exist. + */ + hasLiveClaudeBackgroundJob: async ( + short: string | null | undefined, + ): Promise<"alive" | "gone" | "unknown"> => { + const normalized = normalizeClaudeBackgroundShort(short); + if (!normalized) return "gone"; + const socketPath = await resolveClaudeDaemonControlSocket(); + // No daemon socket, or a request that failed: we do not KNOW the job is + // gone. Collapsing that to "gone" is how a settle confirms a clean + // teardown over a job that is still running — the same mistake as + // treating a timed-out liveness read as an idle session. + if (!socketPath) return "unknown"; + try { + const response = await sendClaudeDaemonRequest(socketPath, { op: "has", short: normalized }); + if (response.ok !== true) return "unknown"; + return response.alive === true || response.present === true ? "alive" : "gone"; + } catch { + return "unknown"; + } + }, restoreCancelledQueue, recoverTurn, recoverCodexTurn, diff --git a/apps/desktop/src/main/services/history/operationService.test.ts b/apps/desktop/src/main/services/history/operationService.test.ts index 29b31d155..a97d0b2db 100644 --- a/apps/desktop/src/main/services/history/operationService.test.ts +++ b/apps/desktop/src/main/services/history/operationService.test.ts @@ -87,6 +87,7 @@ function createInMemoryAdeDb(): { db: AdeDb; raw: Database } { rebuiltFts: false, }), discardUnpublishedChangesForTables: () => {}, + setRemoteSettleTupleHandler: () => {}, }, flushNow: () => undefined, close: () => raw.close(), diff --git a/apps/desktop/src/main/services/onboarding/onboardingService.test.ts b/apps/desktop/src/main/services/onboarding/onboardingService.test.ts index fd4eee20a..a1db50bfb 100644 --- a/apps/desktop/src/main/services/onboarding/onboardingService.test.ts +++ b/apps/desktop/src/main/services/onboarding/onboardingService.test.ts @@ -33,6 +33,7 @@ function createInMemoryAdeDb(): AdeDb { exportChangesSince: () => [], applyChanges: () => ({ appliedCount: 0, dbVersion: 0, touchedTables: [], rebuiltFts: false }), discardUnpublishedChangesForTables: () => {}, + setRemoteSettleTupleHandler: () => {}, }, flushNow: () => {}, close: () => {} diff --git a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts index 856557a57..ee3c594b5 100644 --- a/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts +++ b/apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts @@ -11,13 +11,18 @@ import { isTrackedAgentCliToolType, } from "../../../shared/types"; import { isChatToolType } from "../sessions/chatSessionProjection"; +import type { SettleAbortedReason } from "../sessions/settlingStateRegistry"; /** * 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"]); +const ACTIVITY_ABORTS: ReadonlySet = new Set([ + "turn_start", + "turn_failed", + "attention_requested", +]); import type { AgentChatSessionSummary } from "../../../shared/types"; function isMergeAtOrAfter(mergedAt: string | null | undefined, enabledSince: string): boolean { @@ -264,7 +269,7 @@ export function createPrMergeAutoSettlementService(args: { } abortedSessionIds.delete(session.id); } - const settleResult = args.sessionService.settleSessionsReportingAborts([session.id], { + const settleResult = await args.sessionService.settleSessionsReportingAborts([session.id], { outcome: `PR #${pr.githubPrNumber} merged`, settledAt: polledAt, source: "pr_merge", @@ -278,8 +283,10 @@ export function createPrMergeAutoSettlementService(args: { // ONLY an activity abort waits for the turn to end. `teardown_failed` // means the stop itself failed while the work kept running — making it // wait for inactivity would never stop that work again, because the - // work is exactly what it would be waiting on. `lifecycle_changed` and - // `joined_in_flight` are momentary and clear on their own. + // work is exactly what it would be waiting on. `lifecycle_changed`, + // `joined_in_flight` and `remote_lifecycle_changed` are momentary and + // clear on their own; a peer's decision in particular has nothing to + // do with LOCAL inactivity, so waiting on it would be meaningless. if (settleResult.aborted.some((entry) => ACTIVITY_ABORTS.has(entry.reason))) { abortedSessionIds.add(session.id); } diff --git a/apps/desktop/src/main/services/sessions/sessionService.test.ts b/apps/desktop/src/main/services/sessions/sessionService.test.ts index 254685a30..6d8dbcf4b 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.test.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.test.ts @@ -1303,11 +1303,11 @@ describe("sessionService resume metadata", () => { attentionMessage: "Need a decision", attentionSource: "agent_explicit", })); - service.settleSession("session-settle", { + await service.settleSession("session-settle", { outcome: " Shipped the fix ", settledAt: "2026-03-17T01:00:00.000Z", }); - service.settleSession("session-settle", { + await service.settleSession("session-settle", { outcome: " ", settledAt: "2026-03-17T02:00:00.000Z", }); @@ -1344,9 +1344,9 @@ describe("sessionService resume metadata", () => { toolType: "shell", }); } - service.settleSession("session-settled", { settledAt: "2026-03-17T01:00:00.000Z" }); + await service.settleSession("session-settled", { settledAt: "2026-03-17T01:00:00.000Z" }); - expect(service.settleSessions([ + expect(await service.settleSessions([ "session-settled", "session-new", "session-new", @@ -1356,10 +1356,10 @@ describe("sessionService resume metadata", () => { expect(service.get("session-new")?.settledAt).not.toBeNull(); expect(service.get("session-other")?.settledAt).toBeNull(); - expect(service.settleSessionsReportingAborts( + expect((await service.settleSessionsReportingAborts( ["session-settled", "session-other"], { outcome: "PR #841 merged", settledAt: "2026-03-17T03:00:00.000Z", source: "pr_merge" }, - ).settled).toEqual(["session-other"]); + )).settled).toEqual(["session-other"]); expect(service.get("session-settled")).toEqual(expect.objectContaining({ settledAt: "2026-03-17T01:00:00.000Z", statusNote: null, @@ -1411,7 +1411,7 @@ describe("sessionService resume metadata", () => { service.setStatusNote("session-markers", " "); expect(service.get("session-markers")?.statusNote).toBeNull(); - service.settleSession("session-markers", { + await service.settleSession("session-markers", { settledAt: "2026-03-17T01:00:00.000Z", outcome: "Completed fixes and waiting for release review now", }); @@ -1471,21 +1471,21 @@ describe("sessionService resume metadata", () => { // Chat preview writes (no clearSettled) must PRESERVE a declared settle — // an agent's own final assistant text would otherwise undo its // `ade chat settle`. Only PTY-layer activity un-settles. - service.settleSession("session-output", { settledAt: "2026-03-17T00:30:00.000Z" }); + await service.settleSession("session-output", { settledAt: "2026-03-17T00:30:00.000Z" }); service.setLastOutputPreview("session-output", "final assistant text"); expect(service.get("session-output")?.settledAt).toBe("2026-03-17T00:30:00.000Z"); service.setLastOutputPreview("session-output", "working", { clearSettled: true }); expect(service.get("session-output")?.settledAt).toBeNull(); - service.settleSession("session-output", { settledAt: "2026-03-17T02:00:00.000Z" }); + await service.settleSession("session-output", { settledAt: "2026-03-17T02:00:00.000Z" }); service.touchSessionActivity("session-output", "2026-03-17T02:01:00.000Z"); expect(service.get("session-output")?.settledAt).toBeNull(); // A tracked agent CLI may emit its settle command and final answer through // the same PTY after declaring completion. That output refreshes activity // without reopening the thread; the next user turn clears it explicitly. - service.settleSession("session-output", { settledAt: "2026-03-17T02:30:00.000Z" }); + await service.settleSession("session-output", { settledAt: "2026-03-17T02:30:00.000Z" }); service.touchSessionActivity( "session-output", "2026-03-17T02:31:00.000Z", @@ -1495,7 +1495,7 @@ describe("sessionService resume metadata", () => { // A turn failure un-settles: the declared outcome is in doubt, and keeping // the markers mutually exclusive lets every surface agree on precedence. - service.settleSession("session-output", { settledAt: "2026-03-17T03:00:00.000Z" }); + await service.settleSession("session-output", { settledAt: "2026-03-17T03:00:00.000Z" }); service.markLastTurnFailed("session-output", "2026-03-17T03:05:00.000Z"); expect(service.get("session-output")?.settledAt).toBeNull(); expect(service.get("session-output")?.lastTurnFailedAt).toBe("2026-03-17T03:05:00.000Z"); @@ -1828,7 +1828,7 @@ describe("sessionService settle override", () => { // An explicit settle drops a stale keep-active pin. service.setSettleOverride("session-override", "active"); - service.settleSession("session-override", { settledAt: "2026-03-17T03:00:00.000Z" }); + await service.settleSession("session-override", { settledAt: "2026-03-17T03:00:00.000Z" }); expect(service.get("session-override")).toEqual(expect.objectContaining({ settledAt: "2026-03-17T03:00:00.000Z", settleOverride: null, @@ -1851,7 +1851,7 @@ describe("sessionService settle override", () => { it("preserves the declaration source while an active override temporarily hides settle", async () => { const { service } = await makeService("ade-session-service-override-source-"); - service.settleSession("session-override", { + await service.settleSession("session-override", { settledAt: "2026-03-17T03:00:00.000Z", source: "agent_explicit", }); @@ -1893,7 +1893,7 @@ describe("sessionService settle override", () => { expect(service.setSettleOverrides(["session-override", "session-override-2", "missing"], "active")) .toEqual(["session-override", "session-override-2"]); expect(service.get("session-override-2")?.settleOverride).toBe("active"); - service.settleSession("session-override", { source: "pr_merge" }); + await service.settleSession("session-override", { source: "pr_merge" }); service.setSettleOverrides(["session-override"], "active"); expect(service.get("session-override")?.settleSource).toBe("pr_merge"); service.setSettleOverrides(["session-override", "session-override-2"], null); @@ -1905,7 +1905,7 @@ describe("sessionService settle override", () => { const { service } = await makeService("ade-session-service-override-bulk-settle-"); service.setSettleOverride("session-override", "active"); - expect(service.settleSessions(["session-override"])).toEqual(["session-override"]); + expect(await service.settleSessions(["session-override"])).toEqual(["session-override"]); expect(service.get("session-override")?.settleOverride).toBeNull(); service.setSettleOverride("session-override", "settled"); @@ -1923,18 +1923,18 @@ describe("sessionService settle override", () => { // Declared settle first, Keep-active pinned after: the row carries BOTH a // non-null settled_at and settle_override = 'active', and reads as NOT // settled because canonicalSessionState consults the override first. - service.settleSession("session-override", { settledAt: "2026-03-17T01:00:00.000Z" }); + await service.settleSession("session-override", { settledAt: "2026-03-17T01:00:00.000Z" }); service.setSettleOverride("session-override", "active"); expect(service.get("session-override")?.settledAt).toBe("2026-03-17T01:00:00.000Z"); // Bulk settle must behave like the single-row path: drop the stale pin, // report the row as changed, and preserve the original settle timestamp. - expect(service.settleSessions(["session-override"])).toEqual(["session-override"]); + expect(await service.settleSessions(["session-override"])).toEqual(["session-override"]); expect(service.get("session-override")?.settleOverride).toBeNull(); expect(service.get("session-override")?.settledAt).toBe("2026-03-17T01:00:00.000Z"); // Fully settled with no pin is still a no-op, so the return value keeps // meaning "rows this call actually changed". - expect(service.settleSessions(["session-override"])).toEqual([]); + expect(await service.settleSessions(["session-override"])).toEqual([]); }); }); diff --git a/apps/desktop/src/main/services/sessions/sessionService.ts b/apps/desktop/src/main/services/sessions/sessionService.ts index acefb6414..70d37d864 100644 --- a/apps/desktop/src/main/services/sessions/sessionService.ts +++ b/apps/desktop/src/main/services/sessions/sessionService.ts @@ -1,7 +1,8 @@ import fs from "node:fs"; -import type { AdeDb } from "../state/kvDb"; +import type { AdeDb, RemoteSettleTupleChange } from "../state/kvDb"; import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; -import type { SettleAbortedReason, SettleAbortedSession, SettleSessionsOutcome, SettleTeardownCompleted } from "./settlingStateRegistry"; +import type { SettleAbortedReason, SettleAbortedSession, SettleSessionsOutcome } from "./settlingStateRegistry"; +import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } from "./sessionSettleTeardown"; import type { ClaudeSessionPointer, SessionAttentionSource, @@ -92,6 +93,9 @@ type ClaudeSessionRow = { export const STALE_RUNNING_SESSION_FRESH_ACTIVITY_GRACE_MS = 2 * 60 * 1000; +/** Bounded so a large sweep cannot open a provider stop per session at once. */ +const SETTLE_TEARDOWN_CONCURRENCY = 4; + const SESSION_COLUMNS = ` s.id as id, s.lane_id as laneId, @@ -369,6 +373,8 @@ function normalizeSessionIds(sessionIds: string[]): string[] { export function createSessionService({ db, runSettleTeardown, + onRemoteSettleWrite, + onSettleResidue, }: { db: AdeDb; /** @@ -379,11 +385,28 @@ export function createSessionService({ * revision guard all land and are tested against a NO-OP, so every race is * exercised before there is any work to lose. Step 3 supplies the real one. */ - runSettleTeardown?: (sessionId: string) => SettleTeardownCompleted; + /** + * Real teardown. Awaited INSIDE the settling window, which is what makes the + * suspension point safe: the window is exclusive (R4), abortable (R1/R6) and + * in-memory so a crash resolves to not-settled. Step 2 shipped a synchronous + * branded seam to forbid exactly this until those semantics were proven. + */ + runSettleTeardown?: ( + sessionId: string, + ctx: SettleTeardownContext, + ) => Promise; + /** + * Fired when a peer's settle-tuple write had to be reconciled. Telemetry + * only: post-step-0 this should be zero in the field, and if it is not we + * want to know which writer is still out there before deciding whether a + * protocol-level token is justified. + */ + onRemoteSettleWrite?: (args: { columns: string[]; changesetSessionCount: number }) => void; + /** Fired only for residue attached to a settle that actually landed. */ + onSettleResidue?: (args: { provider: string | null; items: SettleResidueItem[] }) => void; }) { const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); - // Every settle-tuple mutation goes through this writer; see // `settleLifecycleWriter.ts` for why it is its own module and what the // revision guarantees. @@ -710,7 +733,6 @@ export function createSessionService({ ids, ).map((row) => row.id); if (!newlySettled.length) return []; - const updatePlaceholders = newlySettled.map(() => "?").join(", "); const hasOutcome = Object.prototype.hasOwnProperty.call(options, "outcome"); writeSettleLifecycle({ intent: { @@ -733,6 +755,34 @@ export function createSessionService({ return newlySettled; }; + const clearSettleResidue = (sessionId: string): void => { + try { + db.run("delete from session_settle_residue where session_id = ?", [sessionId]); + } catch { + // Diagnostics only. + } + }; + + /** + * Record what teardown could not confirm it stopped (§3d option 3). + * + * Keyed by session so a re-settle REPLACES the previous record rather than + * accumulating history the user has no way to clear. Never throws: the settle + * has already landed by this point, and losing a diagnostics row must not + * turn a successful settle into a failed one. + */ + const recordSettleResidue = (sessionId: string, items: SettleResidueItem[]): void => { + try { + db.run( + `insert into session_settle_residue (session_id, recorded_at, items) values (?, ?, ?) + on conflict(session_id) do update set recorded_at = excluded.recorded_at, items = excluded.items`, + [sessionId, new Date().toISOString(), JSON.stringify(items)], + ); + } catch { + // Diagnostics only. + } + }; + /** * Settle through the settling window: the shape a real teardown will run in. * @@ -742,69 +792,188 @@ export function createSessionService({ * the revision (anything else changed the settle tuple, including a change * this host did not make through a caller). * - * Teardown is a NO-OP in step 2 by design. The point of landing the window - * first is that every race is testable before there is any work to lose. + * Teardown is real from step 3, and it is AWAITED inside the window. */ - const settleManyWithTeardown = ( + const settleManyWithTeardown = async ( sessionIds: string[], options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, - ): SettleSessionsOutcome => { + ): Promise => { const ids = normalizeSessionIds(sessionIds); const settled: string[] = []; const aborted: SettleAbortedSession[] = []; - for (const id of ids) { + // Concurrent, not sequential. Each session takes its own exclusive window, + // so two settles never interact — but the confirmation budget is seconds, + // and a bulk settle used to pay it once PER SESSION in series. iOS allows a + // settle command 30s total, so three busy sessions was already a guaranteed + // "the machine took too long to respond" while the settle ran on regardless. + // + // Bounded, so a fifty-session sweep cannot open fifty provider stops at once. + const settleOne = async (id: string): Promise => { + const outcome: SettleSessionsOutcome = { settled: [], aborted: [] }; + const revisionBefore = settleLifecycle.readRevision(id); const begin = settleLifecycle.settling.begin(id, revisionBefore); // Joined an in-flight settle rather than starting a second teardown: R4. - // The owner will report the outcome; reporting it twice would double-count. + // The owner reports the outcome; reporting it twice would double-count. if (begin.kind === "joined") { - // Report it. A joiner that returns nothing looks identical to a settle - // that was never eligible, and a caller with a durable consequence — the - // PR poller marking a merge handled — would consume the merge on the + // A joiner that returns nothing looks identical to a settle that was + // never eligible, and a caller with a durable consequence — the PR + // poller marking a merge handled — would consume the merge on the // strength of someone else's in-flight settle that may yet abort. - aborted.push({ sessionId: id, reason: "joined_in_flight" }); - continue; + outcome.aborted.push({ sessionId: id, reason: "joined_in_flight" }); + return outcome; } + try { + let teardown: SettleTeardownOutcome | null = null; let teardownThrew = false; try { - runSettleTeardown?.(id); + teardown = runSettleTeardown + ? await runSettleTeardown(id, { + // Read live, not captured: the whole point is that a clearer can + // trip it while teardown is between stop calls. Scoped to OUR + // token, so a window that was force-closed and reopened by a + // different settle reads as abandoned rather than as healthy. + isAborted: () => settleLifecycle.settling.abandoned(id, begin.token), + }) + : null; } catch (error) { - // ONLY a teardown throw is `teardown_failed`. Persistence failures - // below (a SQLite lock timeout, an I/O error) must not wear that - // label: a caller cannot tell "the work is still running" from "the - // work stopped but the row did not save", and the PR poller would - // retry a teardown that already succeeded. Those propagate, as they - // did before the settling window existed. + // ONLY a teardown throw is `teardown_failed`. A persistence failure + // below must not wear that label: a caller cannot tell "the work is + // still running" from "the work stopped but the row did not save", + // and the PR poller would retry a stop that already succeeded. Those + // propagate, as they did before the settling window existed. teardownThrew = true; void error; } if (teardownThrew) { - aborted.push({ sessionId: id, reason: "teardown_failed" }); - continue; + outcome.aborted.push({ sessionId: id, reason: "teardown_failed" }); + return outcome; } - const abortedBy = settleLifecycle.settling.abortedBy(id); - if (abortedBy) { - aborted.push({ sessionId: id, reason: abortedBy }); - continue; + // Same scoping after the await: if this window was replaced while + // teardown ran, the settle it belonged to is gone and must not land. + if (settleLifecycle.settling.abandoned(id, begin.token)) { + outcome.aborted.push({ + sessionId: id, + reason: settleLifecycle.settling.abortedBy(id) ?? "lifecycle_changed", + }); + return outcome; } // The revision catches everything the abort flag cannot: a settle-tuple // change from a path that never announced itself as a decision. if (settleLifecycle.readRevision(id) !== revisionBefore) { - aborted.push({ sessionId: id, reason: "lifecycle_changed" }); - continue; + outcome.aborted.push({ sessionId: id, reason: "lifecycle_changed" }); + return outcome; + } + + const changed = settleMany([id], options); + outcome.settled.push(...changed); + if (changed.length && teardown?.confirmed && !teardown.residue.length) { + // A settle that DID confirm everything must clear the previous + // record, or the row keeps reporting "1 job could not be stopped" + // from a settle two cycles ago, with a stale timestamp. + clearSettleResidue(id); } - settled.push(...settleMany([id], options)); + if (changed.length && teardown?.residue.length) { + recordSettleResidue(id, teardown.residue); + // Reported HERE, not inside teardown: an abort arriving between + // teardown returning and the guards above means the settle never + // landed, and analytics must not claim residue for one that does not + // exist. + onSettleResidue?.({ provider: teardown.provider ?? null, items: teardown.residue }); + } + return outcome; } finally { - settleLifecycle.settling.end(id); + settleLifecycle.settling.end(id, begin.token); } + }; + + // A worker pool, not chunks. Chunking was tried and reverted: a chunk + // barrier idles the other workers until its slowest member finishes, and + // "every teardown is bounded" is not the same as "every teardown takes the + // same time". For a 50-session sweep where a quarter of the rows hold + // unstoppable work, chunking costs roughly 65s against the pool's ~20 — + // aimed straight at the 30s iOS command budget this exists to protect. + const perSession = new Map(); + const queue = [...ids]; + // A persistence failure propagates (a SQLite lock is not a settle outcome + // and must not be dressed up as one), but it must not leave the other + // workers settling sessions the caller has already given up on. Draining + // the queue stops new work; the sessions already in flight finish, so + // nothing is abandoned half-written. + let failure: unknown = null; + await Promise.all( + Array.from({ length: Math.min(SETTLE_TEARDOWN_CONCURRENCY, queue.length) }, async () => { + for (;;) { + const id = queue.shift(); + if (id === undefined) return; + try { + perSession.set(id, await settleOne(id)); + } catch (error) { + failure ??= error; + queue.length = 0; + return; + } + } + }), + ); + // Rethrown only after every worker has stopped, so the throw cannot race + // more writes. Whatever did settle is already durable, and settle is + // idempotent (`coalesce(settled_at, ?)`), so the caller's retry re-reports + // it rather than double-filing it. + if (failure !== null) throw failure; + + // Reassembled in request order. `settled` is a changed-id list that callers + // compare against what they asked for, so it must not come back shuffled by + // whichever teardown happened to finish first. `normalizeSessionIds` + // dedupes, so every id has exactly one entry. + for (const id of ids) { + const outcome = perSession.get(id); + if (!outcome) continue; + settled.push(...outcome.settled); + aborted.push(...outcome.aborted); } return { settled, aborted }; }; + /** + * Settle ONE session and say why if it did not take. + * + * A local function rather than an object method: both public entry points + * delegate here, and a method would break the moment a caller destructured it + * off the service. + */ + const settleOneReportingAbort = async ( + sessionId: string, + opts: { outcome?: string | null; settledAt?: string; source?: SessionSettleSource } = {}, + ): Promise<{ found: boolean; settled: boolean; abortedBy?: SettleAbortedReason }> => { + const trimmed = sessionId.trim(); + if (!trimmed) return { found: false, settled: false }; + // `settleMany` returns [] for both "missing" and "already settled", so the + // found/settled split needs its own existence check to stay honest. + const exists = db.get<{ present: number }>( + "select 1 as present from terminal_sessions where id = ? limit 1", + [trimmed], + ); + if (!exists) return { found: false, settled: false }; + // The key must be ABSENT, not `undefined`. `settleMany` decides whether to + // touch `status_note` with hasOwnProperty, so passing `outcome: undefined` + // writes null and erases the note a previous settle left behind. + const note = normalizeSessionStatusNote(opts.outcome); + const result = await settleManyWithTeardown([trimmed], { + ...(note ? { outcome: note } : {}), + settledAt: opts.settledAt, + source: opts.source, + }); + const abortedBy = result.aborted[0]?.reason; + return abortedBy + ? { found: true, settled: false, abortedBy } + : { found: true, settled: true }; + }; + return { list, @@ -1400,6 +1569,69 @@ export function createSessionService({ }); }, + /** Clears a declared settle plus any `'settled'` override. */ + unsettleSession(sessionId: string): boolean { + const changed = mutateSessionMeta(sessionId, (id) => { + writeSettleLifecycle({ + intent: { kind: "unsettleDeclared" }, + sessionIds: [id], + }); + }); + return changed; + }, + + /** Explicit settle override, cleared with `settled_at` on real activity. */ + setSettleOverride( + sessionId: string, + override: SessionSettleOverride | null, + source: SessionSettleSource = "user", + ): boolean { + const normalized = override == null ? null : normalizeSettleOverride(override); + const normalizedSource = normalizeSettleSource(source) ?? "user"; + return mutateSessionMeta(sessionId, (id) => { + writeSettleLifecycle({ + intent: { kind: "override", value: normalized, source: normalizedSource }, + sessionIds: [id], + }); + }); + }, + + setSettleOverrides(sessionIds: string[], override: SessionSettleOverride | null): string[] { + const ids = normalizeSessionIds(sessionIds); + if (!ids.length) return []; + const normalized = override == null ? null : normalizeSettleOverride(override); + const placeholders = ids.map(() => "?").join(", "); + const present = db.all<{ id: string }>( + `select id from terminal_sessions where id in (${placeholders})`, + ids, + ).map((row) => row.id); + if (!present.length) return []; + writeSettleLifecycle({ + intent: { kind: "override", value: normalized, source: "user" }, + sessionIds: present, + }); + for (const id of present) { + emitChanged({ sessionId: id, reason: "meta-updated" }); + } + return present; + }, + + /** + * The host-local settle concurrency token for a session. + * + * Read it before a decision that takes time, and require it to be unchanged + * before applying that decision — that is the whole point of the + * chokepoint. 0 means "no settle-lifecycle mutation has been recorded for + * this session", which a caller must treat as a real value, not as absent. + */ + getSettleLifecycleRevision(sessionId: string): number { + return settleLifecycle.readRevision(sessionId); + }, + + async settleSessions(sessionIds: string[]): Promise { + return (await settleManyWithTeardown(sessionIds)).settled; + }, + /** * Refresh only the activity timestamp (not the preview text). Lets the PTY * layer record that a session is still producing output even when the @@ -1515,37 +1747,19 @@ export function createSessionService({ return true; }, - settleSession( + async settleSession( sessionId: string, opts: { outcome?: string | null; settledAt?: string; source?: SessionSettleSource } = {}, - ): boolean { + ): Promise { // Through the settling window, like the bulk paths. This is the route a // USER takes (row menu -> settleTerminalSession -> here), which is the - // "user settle" R4 names — so it has to be joinable and abortable, and in - // 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; + // "user settle" R4 names — so it has to be joinable and abortable, and it + // runs real teardown. + // + // Delegates rather than duplicating: the boolean form is exactly the typed + // form with the reason discarded, and keeping two copies of the existence + // probe and the option-spread is how they drift. + return (await settleOneReportingAbort(sessionId, opts)).settled; }, /** @@ -1553,91 +1767,99 @@ export function createSessionService({ * The boolean form cannot distinguish "no such session" from "a turn started * mid-settle", and rendering the second as the first misleads the user. */ - settleSessionReportingAbort( - 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 }; - }, + settleSessionReportingAbort: settleOneReportingAbort, - /** Clears a declared settle plus any `'settled'` override. */ - unsettleSession(sessionId: string): boolean { - const changed = mutateSessionMeta(sessionId, (id) => { - writeSettleLifecycle({ - intent: { kind: "unsettleDeclared" }, - sessionIds: [id], - }); - }); - return changed; - }, + /** + * Reconcile inbound settle-tuple writes from a peer (design 3c-i / R7). + * + * Post-step-0 every legitimate settle decision originates at a host running + * this chokepoint, so a replicated settle-tuple write is either a legacy + * client or a bug. Either way it must not land raw: a raw write bypasses the + * lifecycle revision, so an in-flight settle would neither see it nor abort + * for it, and it could silently overwrite a peer's explicit reactivation. + * + * The VALUES are left to CRR merge, which is the only thing that keeps the + * per-column clocks convergent — an earlier version rebuilt the intent and + * re-decided them, which left this host's clock permanently behind the peer + * and made its next genuine decision lose every merge. What the chokepoint + * contributes is the lifecycle revision: an in-flight settle re-reads it + * after its teardown await, sees it moved, and abandons rather than + * overwriting the peer's decision. + * + * No peer-visible concurrency token is involved; that is a protocol change + * the evidence does not justify. `onRemoteSettleWrite` measures how often + * this path runs — it is NOT an anomaly signal: a paired second desktop + * replicating its own settles is legitimate and lands here by design. + */ + reconcileRemoteSettleTuple(changes: RemoteSettleTupleChange[]): void { + const columnsBySession = new Map>(); + for (const change of changes) { + const columns = columnsBySession.get(change.sessionId) ?? new Set(); + columns.add(change.column); + columnsBySession.set(change.sessionId, columns); + } - /** Explicit settle override, cleared with `settled_at` on real activity. */ - setSettleOverride( - sessionId: string, - override: SessionSettleOverride | null, - source: SessionSettleSource = "user", - ): boolean { - const normalized = override == null ? null : normalizeSettleOverride(override); - const normalizedSource = normalizeSettleSource(source) ?? "user"; - return mutateSessionMeta(sessionId, (id) => { - writeSettleLifecycle({ - intent: { kind: "override", value: normalized, source: normalizedSource }, - sessionIds: [id], + const reconciled: string[] = []; + for (const [sessionId, columns] of columnsBySession) { + // Per session, so one unreadable row cannot discard the rest. The values + // already landed; what is at stake here is only the revision bump. + try { + // The row can be absent: a settle for a session this host has never + // seen. Nothing to reconcile, and not a peer writer worth reporting. + const exists = db.get<{ present: number }>( + "select 1 as present from terminal_sessions where id = ? limit 1", + [sessionId], + ); + if (!exists) continue; + writeSettleLifecycle({ intent: { kind: "observeRemote" }, sessionIds: [sessionId] }); + // Trip the abort too, not just the revision. The revision is only + // re-read AFTER teardown finishes, so on its own it would let a + // teardown run to completion and interrupt a turn the user has just + // started on another device — losing the work AND the settle, which + // is the R2 shape 3c exists to prevent. + settleLifecycle.settling.abort(sessionId, "remote_lifecycle_changed"); + emitChanged({ sessionId, reason: "meta-updated" }); + reconciled.push(...columns); + } catch (error) { + // Best effort per session: the peer's values are already applied, and + // one unreadable row must not cost the rest of the batch its bump. + void error; + } + } + // ONE report per changeset, not one per session. A bulk settle on a peer + // arrives as a single apply covering N sessions, and reporting each would + // turn one remote action into an N-event burst. + if (reconciled.length) { + onRemoteSettleWrite?.({ + columns: [...new Set(reconciled)].sort(), + changesetSessionCount: columnsBySession.size, }); - }); - }, - - setSettleOverrides(sessionIds: string[], override: SessionSettleOverride | null): string[] { - const ids = normalizeSessionIds(sessionIds); - if (!ids.length) return []; - const normalized = override == null ? null : normalizeSettleOverride(override); - const placeholders = ids.map(() => "?").join(", "); - const present = db.all<{ id: string }>( - `select id from terminal_sessions where id in (${placeholders})`, - ids, - ).map((row) => row.id); - if (!present.length) return []; - const updatePlaceholders = present.map(() => "?").join(", "); - writeSettleLifecycle({ - intent: { kind: "override", value: normalized, source: "user" }, - sessionIds: present, - }); - for (const id of present) { - emitChanged({ sessionId: id, reason: "meta-updated" }); } - return present; }, /** - * The host-local settle concurrency token for a session. - * - * Read it before a decision that takes time, and require it to be unchanged - * before applying that decision — that is the whole point of the - * chokepoint. 0 means "no settle-lifecycle mutation has been recorded for - * this session", which a caller must treat as a real value, not as absent. + * What the LAST settle could not confirm it stopped, for the diagnostics + * surface. Returns null unless the session is currently settled: a stale + * record on a row the user has since reactivated is not residue, it is + * history, and showing it would re-light a row that is working fine. */ - getSettleLifecycleRevision(sessionId: string): number { - return settleLifecycle.readRevision(sessionId); - }, - - settleSessions(sessionIds: string[]): string[] { - return settleManyWithTeardown(sessionIds).settled; + getSettleResidue(sessionId: string): { recordedAt: string; items: SettleResidueItem[] } | null { + const trimmed = sessionId.trim(); + if (!trimmed) return null; + const row = db.get<{ settled_at: string | null; recorded_at: string; items: string }>( + `select s.settled_at as settled_at, r.recorded_at as recorded_at, r.items as items + from session_settle_residue r + join terminal_sessions s on s.id = r.session_id + where r.session_id = ?`, + [trimmed], + ); + if (!row || !row.settled_at) return null; + try { + const items = JSON.parse(row.items) as SettleResidueItem[]; + return Array.isArray(items) && items.length ? { recordedAt: row.recorded_at, items } : null; + } catch { + return null; + } }, /** @@ -1649,11 +1871,11 @@ export function createSessionService({ * what a caller with a durable consequence (the PR-merge auto-settle marking * a PR handled) has to branch on. */ - settleSessionsReportingAborts( + async settleSessionsReportingAborts( sessionIds: string[], options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, - ): SettleSessionsOutcome { - return settleManyWithTeardown(sessionIds, options); + ): Promise { + return await settleManyWithTeardown(sessionIds, options); }, /** Sessions currently mid-settle, for the visible `Settling…` state. */ @@ -1664,7 +1886,6 @@ export function createSessionService({ unsettleSessions(sessionIds: string[]): void { const ids = normalizeSessionIds(sessionIds); if (!ids.length) return; - const placeholders = ids.map(() => "?").join(", "); writeSettleLifecycle({ intent: { kind: "unsettleDeclared" }, sessionIds: ids, @@ -1720,7 +1941,6 @@ export function createSessionService({ ).map((row) => row.id); if (!present.length) return []; const snoozedAt = normalizeIsoTimestamp(opts.snoozedAt) ?? new Date().toISOString(); - const updatePlaceholders = present.map(() => "?").join(", "); db.run( ` update terminal_sessions @@ -1728,7 +1948,7 @@ export function createSessionService({ snoozed_at = ?, woke_at = null, woke_reason = null - where id in (${updatePlaceholders}) + where id in (${present.map(() => "?").join(", ")}) `, [until, snoozedAt, ...present], ); @@ -1895,6 +2115,7 @@ export function createSessionService({ // local table with no reaper, and every other session-keyed side table is // already cascaded here. settleLifecycle.forget(trimmed); + clearSettleResidue(trimmed); emitChanged({ sessionId: trimmed, reason: "deleted" }); return true; }, diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts new file mode 100644 index 000000000..8a71f515b --- /dev/null +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts @@ -0,0 +1,290 @@ +import { describe, expect, it, vi } from "vitest"; +import { createSessionSettleTeardown, residueCountBucket } from "./sessionSettleTeardown"; +import type { SessionActiveWork, SessionSettleTeardownDeps } from "./sessionSettleTeardown"; + +/** + * R5 and the teardown contract itself. The race matrix drives the settle path + * through a hand-written seam; this drives the REAL seam, which is where the + * "cannot confirm the stop" decision (design 3d, option 3) actually lives. + */ +describe("session settle teardown", () => { + const neverAborted = { isAborted: () => false }; + + function harness(overrides: Partial = {}) { + const interrupt = vi.fn(async () => {}); + // Instant polling: the confirmation budget is real time in production, and + // a test that actually slept 5s per case would be deleted within a month. + let clock = 0; + const run = createSessionSettleTeardown({ + interrupt, + readActiveWork: async () => null, + now: () => clock, + sleep: async (ms: number) => { clock += ms; }, + // Never fires unless a test asks for it, so an ordinary provider call is + // never mistaken for a hung one. + expireProviderCall: () => new Promise(() => {}), + ...overrides, + }); + return { run, interrupt }; + } + + const work = (over: Partial = {}): SessionActiveWork => ({ + active: false, + backgroundTaskCount: 0, + provider: "claude", + ...over, + }); + + it("does not stop anything for a session with no work", async () => { + const { run, interrupt } = harness({ readActiveWork: async () => work() }); + + const outcome = await run("session-1", neverAborted); + + // A settle with nothing to tear down must not interrupt the session: that + // would be a visible side effect on a row the user only meant to file. + expect(interrupt).not.toHaveBeenCalled(); + expect(outcome.residue).toEqual([]); + }); + + it("stops background work and reports no residue once the session goes quiet", async () => { + const states = [work({ backgroundTaskCount: 2 }), work({ backgroundTaskCount: 2 }), work()]; + const readActiveWork = vi.fn(async () => states.shift() ?? work()); + const { run, interrupt } = harness({ readActiveWork }); + + const outcome = await run("session-1", neverAborted); + + expect(interrupt).toHaveBeenCalledWith("session-1"); + // The stop is asynchronous inside the provider, so a single read straight + // after `interrupt` would call work that was already stopping "residue". + expect(outcome.residue, "work that drained must not be reported as residue").toEqual([]); + }); + + /** + * R5. The stop is attempted, the work does not go away, and the settle still + * lands — but never silently: the residue is returned for the row and the + * analytics hook fires exactly once. + */ + it("R5: reports residue when the stop never confirms, rather than blocking the settle", async () => { + const { run } = harness({ + readActiveWork: async () => work({ backgroundTaskCount: 3 }), + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue).toEqual([{ + kind: "background_tasks", + reason: "timeout", + count: 3, + detail: "3 jobs on claude could not be stopped", + }]); + // The provider rides along for the analytics dimension. Reporting happens in + // the settle path, not here, so an abandoned settle cannot claim residue. + expect(outcome.provider).toBe("claude"); + }); + + it("R5: reports a surviving turn separately from surviving background jobs", async () => { + const { run } = harness({ + readActiveWork: async () => work({ active: true, backgroundTaskCount: 2 }), + }); + + const outcome = await run("session-1", neverAborted); + + // Two facts, two items. Folded together, the count would read "3 jobs" and + // the running turn would disappear into the background-task bucket. + expect(outcome.residue).toEqual([ + { kind: "background_tasks", reason: "timeout", count: 2, detail: "2 jobs on claude could not be stopped" }, + { kind: "active_turn", reason: "timeout", count: 1, detail: "the running turn on claude could not be stopped" }, + ]); + }); + + it("R5: calls a hung stop a timeout, not a rejection", async () => { + const { run } = harness({ + // Never answers. The provider did not refuse — it did not reply at all, + // and filing that as an explicit rejection is exactly the conflation the + // reason field exists to prevent. + interrupt: vi.fn(() => new Promise(() => {})), + readActiveWork: async () => work({ backgroundTaskCount: 1, provider: "codex" }), + expireProviderCall: async () => {}, + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue[0]?.reason).toBe("timeout"); + }); + + it("R5: calls out a provider that has no stop control at all", async () => { + const { run } = harness({ + // A Codex chat cannot stop an individual subagent. That is a different + // fact from "the stop failed", and the field exists to keep them apart. + readActiveWork: async () => work({ backgroundTaskCount: 1, provider: "codex" }), + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue[0]?.reason).toBe("no_stop_control"); + }); + + it("R5: distinguishes a stop the provider rejected from one that timed out", async () => { + const { run } = harness({ + interrupt: vi.fn(async () => { throw new Error("provider refused"); }), + readActiveWork: async () => work({ backgroundTaskCount: 1 }), + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue[0]?.reason).toBe("rejected"); + }); + + /** + * §3c: an accepted turn beats the settle, never the reverse. The abort is + * checked BEFORE each step, so the work that won the race keeps running. + */ + it("stops issuing stop calls the moment a turn aborts the settle", async () => { + const { run, interrupt } = harness({ + readActiveWork: async () => work({ active: true }), + }); + + const outcome = await run("session-1", { isAborted: () => true }); + + expect(interrupt, "an aborted settle must not stop the work that won the race").not.toHaveBeenCalled(); + expect(outcome.residue).toEqual([]); + }); + + it("does not report residue for a session the user reclaimed mid-teardown", async () => { + let aborted = false; + const { run } = harness({ + interrupt: vi.fn(async () => { aborted = true; }), + readActiveWork: async () => work({ active: true, backgroundTaskCount: 1 }), + }); + + const outcome = await run("session-1", { isAborted: () => aborted }); + + // The settle is being abandoned, so there is no settled row to hang a + // "1 job could not be stopped" marker on. Reporting it would label a + // session that is actively working as one that failed to stop. + expect(outcome.residue).toEqual([]); + }); + + it("does not hang the settling window on a provider call that never resolves", async () => { + const { run } = harness({ + // A control call that never settles. Without a per-call ceiling the + // settling window never closes and the row is unsettleable for the life + // of the process. + interrupt: vi.fn(() => new Promise(() => {})), + readActiveWork: async () => work({ backgroundTaskCount: 1 }), + expireProviderCall: async () => {}, + }); + + const outcome = await run("session-1", neverAborted); + + // A provider that never answered did not REFUSE. This said "rejected" and + // was encoding the misclassification: every hung call would reach both the + // user-visible residue and the analytics dimension as an explicit provider + // rejection. + expect(outcome.residue[0]?.reason).toBe("timeout"); + }); + + it("resolves instead of blocking the settle when a liveness read hangs", async () => { + const readActiveWork = vi.fn(() => new Promise(() => {})); + const { run, interrupt } = harness({ + readActiveWork, + expireProviderCall: async () => {}, + }); + + // The first read never resolves. Without a per-call ceiling this awaits + // forever inside the settling window, and the row can never be settled + // again for the life of the process. + const outcome = await run("session-1", neverAborted); + + expect(readActiveWork).toHaveBeenCalledTimes(1); + // Unknown liveness is not licence to start stopping things... + expect(interrupt).not.toHaveBeenCalled(); + // ...but it is also not licence to claim a clean teardown. A timed-out read + // is indistinguishable from "not a chat session" unless it says so, and + // silently settling over running work is the one outcome residue exists to + // prevent. + expect(outcome.residue).toEqual([{ + kind: "background_tasks", + reason: "timeout", + count: 1, + detail: "could not read what this session was running, so nothing was stopped", + }]); + }); + + it("does not claim a clean teardown when the CONFIRMATION read times out", async () => { + let call = 0; + let armed = false; + const { run } = harness({ + // The first read succeeds, so teardown proceeds and interrupts. The read + // that is supposed to CONFIRM the stop then hangs — and a timeout is not + // confirmation, however much it looks like one. + readActiveWork: vi.fn(async () => { + call += 1; + if (call === 1) return work({ backgroundTaskCount: 1 }); + armed = true; + return await new Promise(() => {}); + }), + expireProviderCall: () => armed ? Promise.resolve() : new Promise(() => {}), + }); + + const outcome = await run("session-1", neverAborted); + + expect(outcome.residue, "an unconfirmed stop must never report as clean").not.toEqual([]); + expect(outcome.residue[0]?.reason).toBe("timeout"); + }); + + it("never claims confirmation for a settle that was aborted mid-confirmation", async () => { + let aborted = false; + let reads = 0; + const { run } = harness({ + // The abort must trip INSIDE the confirmation loop, not before it. Tripped + // earlier, an already-correct early return handles it and this test would + // pass against the very bug it is written for. + readActiveWork: async () => { + reads += 1; + if (reads >= 2) aborted = true; + return work({ backgroundTaskCount: 1 }); + }, + }); + + const outcome = await run("session-1", { isAborted: () => aborted }); + + expect(reads, "the confirmation loop must actually have run").toBeGreaterThan(1); + + // `confirmed` gates whether a previous residue record may be ERASED, so a + // teardown that confirmed nothing must never report true — this is the one + // shape the flag exists to make impossible. + expect(outcome.confirmed).toBe(false); + }); + + it("keeps the provider on a confirmation-read timeout", async () => { + let call = 0; + // Armed only AFTER the first read lands. An always-immediate expire races + // the first read on the microtask queue and can time it out instead, which + // would pass this test for the wrong reason. + let armed = false; + const { run } = harness({ + readActiveWork: vi.fn(async () => { + call += 1; + if (call === 1) return work({ backgroundTaskCount: 1, provider: "codex" }); + armed = true; + return await new Promise(() => {}); + }), + expireProviderCall: () => armed + ? Promise.resolve() + : new Promise(() => {}), + }); + + const outcome = await run("session-1", neverAborted); + + // The provider was already read; dropping it loses the analytics dimension + // for exactly the residue worth attributing. + expect(outcome.provider).toBe("codex"); + }); + + it("buckets residue counts so a large fleet cannot widen the analytics dimension", () => { + expect(residueCountBucket(1)).toBe("1"); + expect(residueCountBucket(5)).toBe("2_5"); + expect(residueCountBucket(40)).toBe("6_plus"); + }); +}); diff --git a/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts new file mode 100644 index 000000000..6640849ec --- /dev/null +++ b/apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts @@ -0,0 +1,304 @@ + + +import { PROVIDERS_WITHOUT_BACKGROUND_STOP_CONTROL } from "../../../shared/subagentCapabilities"; + +/** + * @file Real settle teardown: stop the work a session owns, then confirm it stopped. + * + * Shaped after `laneService.stopLaneRuntimeWork` — an ordered list of steps, + * each in its own try/catch so one failure cannot abandon the rest — but NOT + * built on it. That function disposes chat sessions outright because it serves + * lane deletion; a settle must leave the session usable, so this stops the + * session's *outstanding work* and nothing else. + * + * Two rules from the design are load-bearing here: + * + * - **Terminals stay open.** A settle files a session as done; it does not take + * the user's shell away, and ADE cannot re-spawn one it killed. PTYs are + * never touched, at any step. + * - **An accepted turn beats the settle, never the reverse** (§3c). The abort is + * checked before every step, so a turn that starts mid-teardown stops the + * remaining stops. Work already stopped is lost — inherent, and the reason the + * order below is cheapest-to-lose first. + */ + +/** + * What a teardown could not confirm it stopped (design 3d, option 3). + * + * The settle still lands — that is the signed-off decision — but it lands WITH + * this attached, so "settled" never quietly means "and something is still + * running". + * + * Everything recorded here is work ADE tracks, which is what keeps it eligible + * for the ppid-based orphan reaper. Work that escaped the process tree + * (`nohup`/`setsid`/`disown`) is invisible to the confirmation read, so it is + * never counted here — the design requires that it not be folded in and + * overstated as recoverable, and by construction it cannot be. + */ +export type SettleResidueItem = { + /** Coarse and closed: this is also the analytics dimension. */ + kind: "background_tasks" | "active_turn"; + /** + * Why the stop did not confirm. `no_stop_control` is a provider that offers + * no way to stop this work at all (a Codex chat's subagents); `timeout` and + * `rejected` are a stop that was attempted and did not land. + */ + reason: "no_stop_control" | "timeout" | "rejected"; + /** How many jobs this item covers. Bucketed before it reaches analytics. */ + count: number; + /** Human-readable, for the diagnostics surface. Never analytics. */ + detail: string; +}; + +/** Checked BETWEEN stop calls, per design 3c. A turn start trips it. */ +export type SettleTeardownContext = { + isAborted: () => boolean; +}; + +/** + * The result of a real teardown. + * + * This replaces step 2's synchronous `SettleTeardownCompleted` brand. That + * brand made an awaited teardown a COMPILE error while the settle path was + * still synchronous. It is safe to await now because the settling window is + * exclusive, abortable and crash-safe, so it can be HELD across the await; the + * revision re-check and abort check after the await are the guards that make + * the suspension point survivable. + */ +export type SettleTeardownOutcome = { + /** + * True only when teardown actually ran AND confirmed the session went quiet. + * + * An empty `residue` is not the same claim: it is also what you get before the + * chat service exists, or when the confirmation read never came back. Only a + * confirmed-clean settle may erase a previous residue record — otherwise a + * settle that checked nothing deletes an accurate report of work that is + * still running. + */ + confirmed: boolean; + residue: SettleResidueItem[]; + /** For the residue analytics dimension. Null when the session has no chat. */ + provider?: string | null; +}; + +/** + * How long to keep re-reading the session after a stop before calling the work + * residue. NOT a provider stop budget — those are shorter and live in + * `agentChatService` (`CLAUDE_STOP_TASK_TIMEOUT_MS` and friends). This is the + * grace period for a stop that has been accepted and is still draining. + */ +const STOP_CONFIRM_TIMEOUT_MS = 5_000; +const STOP_CONFIRM_POLL_MS = 100; +const STOP_CONFIRM_MAX_POLL_MS = 800; +/** + * Per-call ceiling for the provider calls themselves. + * + * The poll budget above bounds the LOOP, not any single await. Without this a + * provider control call that never resolves would hold the settling window + * open forever: the `finally` that closes it is unreachable, the row is stuck + * showing "Settling…", it can never be settled again for the life of the + * process, and the IPC or remote-command caller hangs with it. + */ +const PROVIDER_CALL_TIMEOUT_MS = 10_000; + +/** + * Resolves to not-ok rather than rejecting, so a slow provider is residue, not + * a crash. + * + * KNOWN LIMITATION: the losing arm keeps running. `agentChatService.interrupt` + * takes no abort signal, so a provider stop that overruns the ceiling cannot be + * recalled — and a session-scoped one (OpenCode's `session.abort`) could land + * after the settle was abandoned and stop a turn the user has since started, + * which is the one thing §3c says must never happen. + * + * Shipped anyway, deliberately: the alternative is no ceiling, and an + * un-bounded await holds the settling window open forever and leaves the row + * permanently unsettleable — a certain failure traded for a narrow one. The + * window needs a provider stop to overrun 10s AND the user to start a turn + * inside it AND the late abort to still apply, and a provider hung that long is + * usually not delivering the abort either. Closing it properly means threading + * an AbortSignal through every provider branch of `interrupt`. + */ +async function withTimeout( + work: Promise, + expire: () => Promise, +): Promise<{ ok: true; value: T } | { ok: false }> { + return await Promise.race([ + work.then((value) => ({ ok: true, value }) as const), + expire().then(() => ({ ok: false }) as const), + ]); +} + +export type SessionActiveWork = { + /** A turn is running right now. */ + active: boolean; + /** Background tasks, subagents, or cloud runs still attributed to the session. */ + backgroundTaskCount: number; + provider: string | null; +}; + +export type SessionSettleTeardownDeps = { + /** + * Stop the session's active turn and its background work. Resolves when the + * stop has been REQUESTED; whether it took is decided by `readActiveWork`. + */ + interrupt: (sessionId: string) => Promise; + /** Ground truth after a stop. `null` for a session the chat service does not own. */ + readActiveWork: (sessionId: string) => Promise; + /** Overrides `PROVIDERS_WITHOUT_BACKGROUND_STOP_CONTROL`; tests only. */ + providersWithoutStopControl?: ReadonlySet; + logger?: { warn: (message: string, meta?: Record) => void }; + now?: () => number; + /** + * Delay between confirmation polls. Tests fast-forward this. + * + * Deliberately NOT the same seam as `expireProviderCall`: a fast-forwarding + * `sleep` would otherwise win every timeout race and make every provider call + * look like it hung. + */ + sleep?: (ms: number) => Promise; + /** Fires when a single provider call has taken too long. Real timer by default. */ + expireProviderCall?: () => Promise; +}; + +export function createSessionSettleTeardown( + deps: SessionSettleTeardownDeps, +): (sessionId: string, ctx: SettleTeardownContext) => Promise { + const noStopControl = deps.providersWithoutStopControl ?? PROVIDERS_WITHOUT_BACKGROUND_STOP_CONTROL; + const now = deps.now ?? (() => Date.now()); + const sleep = deps.sleep ?? ((ms: number) => new Promise((resolve) => { setTimeout(resolve, ms); })); + const expireProviderCall = deps.expireProviderCall + ?? (() => new Promise((resolve) => { setTimeout(resolve, PROVIDER_CALL_TIMEOUT_MS).unref?.(); })); + + return async (sessionId, ctx): Promise => { + const residue: SettleResidueItem[] = []; + + // `{ok:false}` rather than collapsing a timeout to null: a timed-out read + // and "this is not a chat session" are both absences, and treating them the + // same is how a slow host settles while claiming a clean teardown. Making + // it a discriminated result forces every call site to decide. + const readWork = async (): Promise<{ ok: true; value: SessionActiveWork | null } | { ok: false }> => + await withTimeout(deps.readActiveWork(sessionId), expireProviderCall) + .catch(() => ({ ok: false }) as const); + + const timedOutResidue = (provider: string | null = null): SettleTeardownOutcome => ({ + residue: [{ + kind: "background_tasks", + reason: "timeout", + count: 1, + detail: "could not read what this session was running, so nothing was stopped", + }], + provider, + confirmed: false, + }); + + const first = await readWork(); + if (!first.ok) return timedOutResidue(); + const before = first.value; + // Nothing to stop, or a session this service does not own (a plain + // terminal). Either way there is no work to lose and no residue to report. + if (!before || (!before.active && before.backgroundTaskCount === 0)) { + return { residue, confirmed: true }; + } + + const provider = before.provider; + // Checked before the step, not after: the point of the abort is to stop + // work we have NOT done yet. + if (ctx.isAborted()) return { residue, confirmed: false }; + + // Tracked apart: a provider that REFUSED the stop and one that never + // answered are different facts, and collapsing them is what the reason + // field exists to prevent — a hung call would be filed as an explicit + // rejection in both the residue the user sees and the analytics. + let stopRejected = false; + let stopTimedOut = false; + try { + const stop = await withTimeout(deps.interrupt(sessionId), expireProviderCall); + if (!stop.ok) { + stopTimedOut = true; + deps.logger?.warn("settle_teardown.step_timed_out", { step: "interrupt" }); + } + } catch (error) { + stopRejected = true; + deps.logger?.warn("settle_teardown.step_failed", { + step: "interrupt", + error: error instanceof Error ? error.message : String(error), + }); + } + + // A turn that arrived while the stop was in flight wins. Do not spend the + // confirmation budget re-reading a session the user is actively using. + if (ctx.isAborted()) return { residue, confirmed: false }; + + const confirmed = await waitForQuiet(readWork, ctx); + // Same rule for the confirmation read: a timeout here is not confirmation. + if (!confirmed.ok) return timedOutResidue(provider); + const after = confirmed.value; + if (after && !ctx.isAborted()) { + const reason = stopRejected + ? "rejected" as const + : stopTimedOut + ? "timeout" as const + : provider && noStopControl.has(provider) + ? "no_stop_control" as const + : "timeout" as const; + // A surviving turn and surviving background tasks are separate facts. + // Folding them into one item lost both the kind and the count — the two + // things the residue exists to report. + if (after.backgroundTaskCount > 0) { + residue.push({ + kind: "background_tasks", + reason, + count: after.backgroundTaskCount, + detail: describeResidue(after.backgroundTaskCount, provider, "jobs"), + }); + } + if (after.active) { + residue.push({ + kind: "active_turn", + reason, + count: 1, + detail: describeResidue(1, provider, "turn"), + }); + } + } + + return { residue, provider, confirmed: !ctx.isAborted() }; + }; + + /** + * Poll until the session goes quiet or the budget runs out. A stop is + * asynchronous inside the provider, so reading once immediately after + * `interrupt` would report residue for work that was about to stop anyway. + */ + async function waitForQuiet( + read: () => Promise<{ ok: true; value: SessionActiveWork | null } | { ok: false }>, + ctx: SettleTeardownContext, + ): Promise<{ ok: true; value: SessionActiveWork | null } | { ok: false }> { + const deadline = now() + STOP_CONFIRM_TIMEOUT_MS; + let delay = STOP_CONFIRM_POLL_MS; + let latest = await read(); + while (latest.ok && latest.value && (latest.value.active || latest.value.backgroundTaskCount > 0) && now() < deadline) { + if (ctx.isAborted()) return latest; + await sleep(delay); + // `getSessionSummary` resolves persisted state, model descriptors and a + // pending-input query; 10 Hz for five seconds is ~50 of those per settle. + delay = Math.min(delay * 2, STOP_CONFIRM_MAX_POLL_MS); + latest = await read(); + } + return latest; + } +} + +function describeResidue(count: number, provider: string | null, noun: "jobs" | "turn"): string { + const what = noun === "turn" + ? "the running turn" + : count === 1 ? "1 job" : `${count} jobs`; + return provider ? `${what} on ${provider} could not be stopped` : `${what} could not be stopped`; +} + +/** Bucketed so a fleet that fails to stop cannot become a high-cardinality dimension. */ +export function residueCountBucket(count: number): "1" | "2_5" | "6_plus" { + if (count <= 1) return "1"; + return count <= 5 ? "2_5" : "6_plus"; +} diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts index 3143df0b7..2ed47dffa 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.test.ts @@ -356,7 +356,7 @@ describe("settle-lifecycle writer", () => { expect(service.get("session-1")?.lastOutputPreview).toBe("tick one"); }); - it("keeps the revision table out of CRR replication", async () => { + it("keeps the host-local settle tables out of CRR replication", async () => { const projectRoot = makeProjectRoot(); const db = await openKvDb(path.join(projectRoot, ".ade", "ade.db"), createLogger() as any); activeDisposers.push(async () => db.close()); @@ -381,15 +381,22 @@ describe("settle-lifecycle writer", () => { // meaningless there, and on the CRR `terminal_sessions` row it would add a // per-column clock entry to the throttled preview write path. expect(clockTable("session_lifecycle_revisions")).toBeNull(); - // Positive control: proves the assertion above can fail. + // Residue describes processes on THIS host. Replicated, it would show a + // peer "1 job could not be stopped" for a machine it cannot see. + expect(clockTable("session_settle_residue")).toBeNull(); + // Positive control: proves the assertions above can fail. expect(clockTable("terminal_sessions")).not.toBeNull(); } - expect( - db.get<{ name: string }>( - "select name from sqlite_master where type = 'table' and name = 'session_lifecycle_revisions'", - )?.name, - ).toBe("session_lifecycle_revisions"); + for (const table of ["session_lifecycle_revisions", "session_settle_residue"]) { + expect( + db.get<{ name: string }>( + "select name from sqlite_master where type = 'table' and name = ?", + [table], + )?.name, + `${table} must exist locally even though it never replicates`, + ).toBe(table); + } }); }); diff --git a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts index 02a0c69ec..f3e5c8122 100644 --- a/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts +++ b/apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts @@ -26,7 +26,14 @@ export type SettleLifecycleIntent = | { kind: "clearOnActivity"; cause: SettleClearCause } /** Declared unsettle: drops a `'settled'` pin but preserves `'active'`. */ | { kind: "unsettleDeclared" } - | { kind: "override"; value: SessionSettleOverride | null; source: SessionSettleSource }; + | { kind: "override"; value: SessionSettleOverride | null; source: SessionSettleSource } + /** + * A peer already changed the tuple; CRR merged it. Values are untouched — + * re-deciding them here would fight the merge and desynchronise the + * per-column clocks. This exists only to move the revision, which is what + * makes an in-flight settle see that the world moved and abandon itself. + */ + | { kind: "observeRemote" }; /** * Columns a caller may set alongside the settle tuple. A closed union, so @@ -112,6 +119,14 @@ export function createSettleLifecycleWriter(db: AdeDb): SettleLifecycleWriter { + "settle_source = case when ? = 'settled' then ? when settled_at is null then null else settle_source end", params: [intent.value, intent.value, intent.source], }; + case "observeRemote": + // Self-assignment: it MATCHES the row (so `sqlite3_changes` is 1 and the + // revision bumps) while changing nothing (so cr-sqlite records no new + // column version and this does not echo back to the peer). + return { + sql: "settled_at = settled_at, settle_override = settle_override, settle_source = settle_source", + params: [], + }; } }; diff --git a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts index 7780fe752..696954e72 100644 --- a/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts +++ b/apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts @@ -5,11 +5,12 @@ import { afterEach, describe, expect, it } from "vitest"; import { openKvDb } from "../state/kvDb"; import { createSessionService } from "./sessionService"; import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; -import { settleTeardownCompleted } from "./settlingStateRegistry"; +import type { SettleResidueItem, SettleTeardownContext } from "./sessionSettleTeardown"; + /** * The race matrix from the settle-teardown design (§2), tested directly against - * the lifecycle revision and the settling window — with teardown still a NO-OP. + * the lifecycle revision and the settling window — with a real, awaited teardown. * * That ordering is the point. Every one of these races was previously argued * about in review rather than executed, and the six rounds of PR #1059 are what @@ -46,7 +47,7 @@ function insertProjectGraph(db: Awaited>) { ); } -describe("settle race matrix (teardown is a no-op)", () => { +describe("settle race matrix", () => { const disposers: Array<() => Promise> = []; afterEach(async () => { while (disposers.length) await disposers.pop()?.(); @@ -59,17 +60,27 @@ describe("settle race matrix (teardown is a no-op)", () => { insertProjectGraph(db); // The teardown seam the race matrix drives. Whatever this does is what a // real provider stop would have been doing when the race landed. - let teardown: (sessionId: string) => void = () => {}; + let teardown: (sessionId: string) => void | Promise = () => {}; + // Records what the seam was handed, so a test can assert the abort was + // visible to teardown WHILE it ran rather than only afterwards. + const teardownContexts: SettleTeardownContext[] = []; + let residue: SettleResidueItem[] = []; + const remoteWrites: Array<{ columns: string[]; changesetSessionCount: number }> = []; const service = createSessionService({ db, - runSettleTeardown: (sessionId) => { - teardown(sessionId); - return settleTeardownCompleted(); + onRemoteSettleWrite: (args) => { remoteWrites.push(args); }, + runSettleTeardown: async (sessionId, ctx) => { + teardownContexts.push(ctx); + await teardown(sessionId); + return { residue, confirmed: true }; }, }); - const setTeardown = (fn: (sessionId: string) => void) => { + const setTeardown = (fn: (sessionId: string) => void | Promise) => { teardown = fn; }; + const setResidue = (items: SettleResidueItem[]) => { + residue = items; + }; const create = (id: string) => service.create({ sessionId: id, @@ -82,7 +93,7 @@ describe("settle race matrix (teardown is a no-op)", () => { toolType: "codex-chat", }); create("session-1"); - return { db, service, create, setTeardown }; + return { db, service, create, setTeardown, setResidue, teardownContexts, remoteWrites }; } /** @@ -92,13 +103,13 @@ describe("settle race matrix (teardown is a no-op)", () => { */ it("R1: a turn starting during teardown abandons the settle", async () => { const { service, setTeardown } = await fixture(); - service.settleSessions(["session-1"]); + await service.settleSessions(["session-1"]); service.unsettleSession("session-1"); setTeardown(() => { service.clearTurnStartMarkers("session-1"); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); expect(outcome.settled).toEqual([]); expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "turn_start" }]); @@ -121,7 +132,7 @@ describe("settle race matrix (teardown is a no-op)", () => { teardownRan = true; service.requestAttention("session-1", "need you"); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); expect(teardownRan).toBe(true); expect(outcome.settled).toEqual([]); @@ -136,7 +147,7 @@ describe("settle race matrix (teardown is a no-op)", () => { // A stop against an already-finished task is a no-op, and nothing touches // the settle tuple. }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); expect(outcome.aborted).toEqual([]); expect(outcome.settled).toEqual(["session-1"]); @@ -151,16 +162,16 @@ describe("settle race matrix (teardown is a no-op)", () => { const { service, setTeardown } = await fixture(); let teardowns = 0; - let inner: ReturnType | null = null; - setTeardown(() => { + let inner: Awaited> | null = null; + setTeardown(async () => { teardowns += 1; // Re-entrant settle, exactly as a PR-merge poll landing mid-user-settle. // The window is already open, so this must JOIN rather than tear down // again — the teardown counter is what proves it. - inner = service.settleSessionsReportingAborts(["session-1"]); + inner = await service.settleSessionsReportingAborts(["session-1"]); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); expect(teardowns, "the joined settle must not start its own teardown").toBe(1); expect(outcome.settled).toEqual(["session-1"]); @@ -189,7 +200,7 @@ describe("settle race matrix (teardown is a no-op)", () => { service.setLastOutputPreview("session-1", "final chunk", { clearSettled: true }); service.touchSessionActivity("session-1", "2026-08-11T00:09:00.000Z"); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); // 1. did not abort expect(outcome.aborted).toEqual([]); @@ -205,7 +216,7 @@ describe("settle race matrix (teardown is a no-op)", () => { it("R6b: the same output OUTSIDE the settling window clears normally", async () => { const { service } = await fixture(); - service.settleSessions(["session-1"]); + await service.settleSessions(["session-1"]); expect(service.get("session-1")?.settledAt).toBeTruthy(); service.setLastOutputPreview("session-1", "later output", { clearSettled: true }); @@ -223,10 +234,20 @@ describe("settle race matrix (teardown is a no-op)", () => { * deliberately left desktop peers replicating, so this is reachable in * production. * - * This test exists to show the BLAST RADIUS before teardown exists, per the + * This test exists to show the BLAST RADIUS before real teardown existed, per the * coordinator's step-3 review scope — it asserts today's real behavior, not * the behavior we want. */ + /** + * NOTE (step 3): R7 and R7b below still write the row with a raw `db.run`, + * which is a bypass NO inbound changeset can produce any more — the apply + * layer now hands settle-tuple writes to `reconcileRemoteSettleTuple`. They + * are kept, unchanged, because they still pin the underlying property they + * always pinned: a write that reaches the tuple without the chokepoint is + * invisible to the guard. That is the reason the apply layer must intercept, + * so deleting these would delete the evidence for the fix. The reconciled + * path is asserted immediately after them. + */ it("R7: a peer-style write that bypasses the writer is invisible to the guard", async () => { const { db, service, setTeardown } = await fixture(); const revisionBefore = service.getSettleLifecycleRevision("session-1"); @@ -239,7 +260,7 @@ describe("settle race matrix (teardown is a no-op)", () => { ["2026-08-11T00:07:00.000Z", "user", "session-1"], ); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); // Observed behaviour, not the behaviour we would have guessed. // @@ -279,7 +300,7 @@ describe("settle race matrix (teardown is a no-op)", () => { ["2026-08-11T00:07:00.000Z", "user", "session-1"], ); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); const accountedFor = [ ...outcome.settled, @@ -287,7 +308,7 @@ describe("settle race matrix (teardown is a no-op)", () => { ]; expect( accountedFor, - "step 3 must account for an id the writer never saw change", + "an id the writer never saw change must not vanish from both lists", ).toEqual([]); }); @@ -303,7 +324,7 @@ describe("settle race matrix (teardown is a no-op)", () => { it("refuses a multi-id clear rather than guessing one disposition for the batch", async () => { const { service, db, create } = await fixture(); create("session-2"); - service.settleSessions(["session-1", "session-2"]); + await service.settleSessions(["session-1", "session-2"]); // The service API cannot express a multi-id `clearOnActivity`, so reach the // writer directly — otherwise the refusal this test is named for is never @@ -334,7 +355,7 @@ describe("settle race matrix (teardown is a no-op)", () => { setTeardown((sessionId) => { if (sessionId === "session-1") throw new Error("provider stop failed"); }); - const outcome = service.settleSessionsReportingAborts(["session-1", "session-2"]); + const outcome = await service.settleSessionsReportingAborts(["session-1", "session-2"]); expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "teardown_failed" }]); // The rest of the batch was still attempted and accounted for. @@ -342,14 +363,26 @@ describe("settle race matrix (teardown is a no-op)", () => { expect(service.settlingSessionIds()).toEqual([]); }); - /** - * An async teardown is now a COMPILE error, not a runtime one — the seam - * returns a branded value that only a synchronous body can produce, so both - * `async (id) => {}` and the adapter `id => asyncStop(id)` fail to typecheck. - * There is no runtime behaviour left to assert; the type is the test. - */ - /** A settling row found after a restart resolves to not-settled. */ + it("stops a teardown whose window was force-closed out from under it", async () => { + const { service, setTeardown, teardownContexts } = await fixture(); + let sawAbandoned: boolean | null = null; + + setTeardown(() => { + // What `deleteSession` does mid-teardown: `forget` force-closes this + // window. Whatever occupies the id afterwards — nothing, or a fresh + // window opened by a later settle for a recreated session — is not ours, + // and a teardown reading `abortedBy` alone would see "not aborted" and + // keep issuing provider stops. + service.deleteSession("session-1"); + sawAbandoned = teardownContexts.at(-1)?.isAborted() ?? null; + }); + + await service.settleSessionsReportingAborts(["session-1"]); + + expect(sawAbandoned, "a teardown that lost its window must see itself abandoned").toBe(true); + }); + it("crash safety: the settling window does not survive the process", async () => { const { service, setTeardown } = await fixture(); expect(service.settlingSessionIds()).toEqual([]); @@ -361,7 +394,7 @@ describe("settle race matrix (teardown is a no-op)", () => { setTeardown(() => { duringWindow = service.settlingSessionIds(); }); - const outcome = service.settleSessionsReportingAborts(["session-1"]); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); expect(outcome.aborted, "the teardown must not have thrown").toEqual([]); expect(duringWindow, "the session must be visibly settling mid-teardown").toEqual([ @@ -371,6 +404,40 @@ describe("settle race matrix (teardown is a no-op)", () => { // session permanently unsettleable. expect(service.settlingSessionIds()).toEqual([]); }); + it("stops the queue when a persistence failure propagates", async () => { + const { service, db, create, setTeardown } = await fixture(); + const ids = ["session-1"]; + for (let n = 2; n <= 12; n += 1) { + create(`session-${n}`); + ids.push(`session-${n}`); + } + + // Fail the write for one session only. The pool must stop taking new work + // rather than settling rows the caller has already given up on. + const realRunChanged = db.runChanged.bind(db); + let tornDown = 0; + setTeardown(() => { tornDown += 1; }); + db.runChanged = ((sql: string, params?: unknown[]) => { + if (typeof sql === "string" && /update\s+terminal_sessions/i.test(sql) + && Array.isArray(params) && params.includes("session-2")) { + throw new Error("database is locked"); + } + return realRunChanged(sql, params as never); + }) as typeof db.runChanged; + + await expect(service.settleSessionsReportingAborts(ids)).rejects.toThrow(/locked/); + db.runChanged = realRunChanged; + + // Far short of all twelve: the queue was drained, so the pool stopped + // taking new sessions instead of running on past the caller. The few + // already in flight when it failed still finish, which is the point — + // nothing is abandoned half-written. + expect(tornDown, "the pool must stop taking new sessions after a failure").toBeLessThan(ids.length); + // And every window closed, or those rows would be unsettleable for the + // life of the process. + expect(service.settlingSessionIds()).toEqual([]); + }); + it("keeps a persistence failure distinct from a failed teardown", async () => { const { service, db, setTeardown } = await fixture(); let stopped = 0; @@ -390,7 +457,7 @@ describe("settle race matrix (teardown is a no-op)", () => { // It must NOT come back as `teardown_failed`: the work really did stop, and // labelling it a teardown failure invites a caller to retry the stop. - expect(() => service.settleSessionsReportingAborts(["session-1"])).toThrow(/locked/); + await expect(service.settleSessionsReportingAborts(["session-1"])).rejects.toThrow(/locked/); expect(stopped, "teardown ran, so it must not be reported as failed").toBe(1); db.runChanged = realRunChanged; // The window still closed, or the session would be permanently unsettleable. @@ -399,11 +466,11 @@ describe("settle race matrix (teardown is a no-op)", () => { it("does not erase an existing status note when a re-settle supplies none", async () => { const { service, db } = await fixture(); - expect(service.settleSession("session-1", { outcome: "shipped the fix" })).toBe(true); + expect(await service.settleSession("session-1", { outcome: "shipped the fix" })).toBe(true); service.unsettleSession("session-1"); // Re-settling with no outcome must leave the previous note alone. - expect(service.settleSession("session-1")).toBe(true); + expect(await service.settleSession("session-1")).toBe(true); const row = db.get<{ status_note: string | null }>( "select status_note from terminal_sessions where id = ?", ["session-1"], @@ -419,14 +486,195 @@ describe("settle race matrix (teardown is a no-op)", () => { }); // The boolean form must not claim success... - expect(service.settleSession("session-1")).toBe(false); + expect(await service.settleSession("session-1")).toBe(false); // ...and the typed form must say WHY, so a caller does not report a session // that is sitting right there working as "not found". - const aborted = service.settleSessionReportingAbort("session-1"); + const aborted = await service.settleSessionReportingAbort("session-1"); expect(aborted).toEqual({ found: true, settled: false, abortedBy: "turn_start" }); - expect(service.settleSessionReportingAbort("no-such-session")).toEqual({ + expect(await service.settleSessionReportingAbort("no-such-session")).toEqual({ found: false, settled: false, }); }); + /** + * R5, end to end: an unconfirmed stop does NOT block the settle (3d option 3), + * and the residue is discoverable on the row afterwards rather than only + * returned to whoever happened to call. + */ + it("R5: settles with recorded residue when teardown cannot confirm the stop", async () => { + const { service, setResidue } = await fixture(); + setResidue([{ + kind: "background_tasks", + reason: "no_stop_control", + count: 2, + detail: "2 jobs on codex could not be stopped", + }]); + + const outcome = await service.settleSessionsReportingAborts(["session-1"]); + + // Filed, not blocked: option 1 (reject the settle) was explicitly not chosen. + expect(outcome.settled).toEqual(["session-1"]); + expect(outcome.aborted).toEqual([]); + expect(service.get("session-1")?.settledAt).not.toBeNull(); + + const residue = service.getSettleResidue("session-1"); + expect(residue?.items).toEqual([{ + kind: "background_tasks", + reason: "no_stop_control", + count: 2, + detail: "2 jobs on codex could not be stopped", + }]); + }); + + it("does not leave residue hanging on a session the user reactivated", async () => { + const { service, setResidue } = await fixture(); + setResidue([{ + kind: "background_tasks", reason: "timeout", count: 1, detail: "1 job could not be stopped", + }]); + await service.settleSessionsReportingAborts(["session-1"]); + expect(service.getSettleResidue("session-1")).not.toBeNull(); + + service.unsettleSession("session-1"); + + // The row is working again. A stale "1 job could not be stopped" marker + // would re-light a session that is fine. + expect(service.getSettleResidue("session-1")).toBeNull(); + }); + + it("records no residue for a settle that was abandoned", async () => { + const { service, setResidue, setTeardown } = await fixture(); + setResidue([{ + kind: "background_tasks", reason: "timeout", count: 1, detail: "1 job could not be stopped", + }]); + setTeardown(() => { + service.clearTurnStartMarkers("session-1"); + }); + + const outcome = await service.settleSessionsReportingAborts(["session-1"]); + + expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "turn_start" }]); + // Nothing was filed, so there is no settled row for residue to describe. + expect(service.getSettleResidue("session-1")).toBeNull(); + }); + /** + * Decision 1 for step 3: finish host authority instead of building consensus + * machinery. A replicated settle-tuple write is routed back through the + * chokepoint, so it gains the revision, the settling window and the abort + * semantics a local decision has — no peer-visible concurrency token. + */ + it("tears sessions down concurrently, and still reports in the caller's order", async () => { + const { service, create, setTeardown } = await fixture(); + for (const id of ["session-2", "session-3"]) create(id); + + let inFlight = 0; + let peakInFlight = 0; + const release: Array<() => void> = []; + setTeardown(async () => { + inFlight += 1; + peakInFlight = Math.max(peakInFlight, inFlight); + await new Promise((resolve) => { release.push(resolve); }); + inFlight -= 1; + }); + + const pending = service.settleSessionsReportingAborts(["session-3", "session-1", "session-2"]); + // Let all three teardowns start before any finishes. + await new Promise((resolve) => { setTimeout(resolve, 0); }); + // Asserted BEFORE draining: serial teardown parks two of the three behind + // the first, and the drain below would then hang until vitest's timeout — + // a real failure, but reported as "test timed out" instead of as this. + expect(release.length, "bulk settle must not tear down one session at a time").toBeGreaterThan(1); + while (release.length) release.pop()!(); + const outcome = await pending; + + // Serial teardown was the regression: the confirmation budget is seconds, + // and iOS gives the whole command 30s. + expect(peakInFlight, "bulk settle must not tear down one session at a time").toBeGreaterThan(1); + // Order follows the request, not whichever teardown finished first. + expect(outcome.settled).toEqual(["session-3", "session-1", "session-2"]); + }); + + describe("remote settle-tuple reconciliation", () => { + /** Stands in for `applyChanges` having merged the peer's columns. */ + const applyPeerWrite = (db: Awaited>["db"], sql: string, params: unknown[]) => { + db.run(sql, params as never); + }; + + it("bumps the revision for a peer settle, without re-deciding the value", async () => { + const { db, service, remoteWrites } = await fixture(); + const revisionBefore = service.getSettleLifecycleRevision("session-1"); + applyPeerWrite( + db, + "update terminal_sessions set settled_at = ?, settle_source = ? where id = ?", + ["2026-08-11T00:07:00.000Z", "user", "session-1"], + ); + + service.reconcileRemoteSettleTuple([ + { sessionId: "session-1", column: "settled_at" }, + { sessionId: "session-1", column: "settle_source" }, + ]); + + // The peer's value stands exactly as CRR merged it. + expect(service.get("session-1")?.settledAt).toBe("2026-08-11T00:07:00.000Z"); + // And an in-flight settle can now see that the world moved. + expect(service.getSettleLifecycleRevision("session-1")).toBeGreaterThan(revisionBefore); + expect(remoteWrites).toEqual([{ columns: ["settle_source", "settled_at"], changesetSessionCount: 1 }]); + }); + + it("makes a peer reactivation abort an in-flight settle instead of being overwritten", async () => { + const { db, service, setTeardown, teardownContexts } = await fixture(); + await service.settleSessionsReportingAborts(["session-1"]); + service.unsettleSession("session-1"); + + // Captured inside teardown: the window is closed by the time the call + // returns, so `isAborted` is only meaningful while it is still open. + let abortedDuringTeardown: boolean | null = null; + setTeardown(() => { + // The mirror case Codex raised: a peer reactivates the session while + // this host is mid-settle. + applyPeerWrite( + db, + "update terminal_sessions set settle_override = ? where id = ?", + ["active", "session-1"], + ); + service.reconcileRemoteSettleTuple([ + { sessionId: "session-1", column: "settle_override" }, + ]); + abortedDuringTeardown = teardownContexts.at(-1)?.isAborted() ?? null; + }); + const outcome = await service.settleSessionsReportingAborts(["session-1"]); + + expect(outcome.aborted).toEqual([{ sessionId: "session-1", reason: "remote_lifecycle_changed" }]); + expect(outcome.settled).toEqual([]); + // Visible to teardown WHILE it runs, not only after. The revision alone is + // re-read after the await, which would let teardown run to completion and + // interrupt a turn the user just started on the other device. + expect(abortedDuringTeardown, "teardown must see the abort while it is still running").toBe(true); + // The peer's reactivation survives. + expect(service.get("session-1")?.settleOverride).toBe("active"); + expect(service.get("session-1")?.settledAt).toBeNull(); + }); + + it("ignores a change for a row this host does not have", async () => { + const { service, remoteWrites } = await fixture(); + + service.reconcileRemoteSettleTuple([{ sessionId: "not-here", column: "settled_at" }]); + + expect(remoteWrites).toEqual([]); + }); + + it("keeps reconciling the rest of the batch when one session is unknown", async () => { + const { db, service, create, remoteWrites } = await fixture(); + create("session-2"); + applyPeerWrite(db, "update terminal_sessions set settled_at = ? where id = ?", ["2026-08-11T00:07:00.000Z", "session-2"]); + + service.reconcileRemoteSettleTuple([ + { sessionId: "not-here", column: "settled_at" }, + { sessionId: "session-2", column: "settled_at" }, + ]); + + // The missing row must not cost session-2 its revision bump — and the + // report is ONE event for the batch, not one per session. + expect(remoteWrites).toEqual([{ columns: ["settled_at"], changesetSessionCount: 2 }]); + }); + }); }); diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts new file mode 100644 index 000000000..d986a0d65 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, it, vi } from "vitest"; +import { createSettleTeardownWiring } from "./settleTeardownWiring"; + +/** + * The wiring is where teardown meets the chat service, so it is where two + * whole-feature defects lived: a settle that destroyed the user's queued + * prompts, and a settle that declared a restarted session quiet while its + * background job kept running. + */ +describe("settle teardown wiring", () => { + const neverAborted = { isAborted: () => false }; + + function harness( + summary: Record | null, + backgroundJob: "alive" | "gone" | "unknown" = "alive", + ) { + const interrupt = vi.fn(async () => ({})); + const hasLiveClaudeBackgroundJob = vi.fn(async () => backgroundJob); + const wiring = createSettleTeardownWiring({ + agentChatService: { + interrupt, + getSessionSummary: async () => summary as never, + hasLiveClaudeBackgroundJob, + }, + surface: "desktop", + }); + return { wiring, interrupt, hasLiveClaudeBackgroundJob }; + } + + it("never asks a provider to clear the user's queued turns", async () => { + const { wiring, interrupt } = harness({ status: "active", provider: "claude" }); + + await wiring.runSettleTeardown("session-1", neverAborted); + + // `stop_and_clear` cancels queued follow-ups. Losing a settle costs one + // click; losing prompts the user already typed is unrecoverable. + expect(interrupt).toHaveBeenCalledWith({ sessionId: "session-1", mode: "stop_only" }); + }); + + it("treats a persisted Claude background job as work, even when the runtime is gone", async () => { + // What a restarted session looks like: no live runtime, so the live count + // is zero, but the daemon job is still recorded and still running. + const { wiring, interrupt } = harness({ + status: "idle", + provider: "claude", + activeBackgroundTaskCount: 0, + claudeBackgroundJobShort: "bg-42", + }); + + await wiring.runSettleTeardown("session-1", neverAborted); + + // Without this the settle sees a quiet session, stops nothing, and files it + // as done over a job that never stopped. + expect(interrupt, "a persisted background job must still be stopped").toHaveBeenCalled(); + }); + + it("ignores a recorded background job the daemon says is already gone", async () => { + // The short is a RECORD, not a liveness signal — it survives the job + // finishing. Trusting it alone makes every later settle burn the + // confirmation budget and then report residue that does not exist. + const { wiring, interrupt, hasLiveClaudeBackgroundJob } = harness({ + status: "idle", + provider: "claude", + activeBackgroundTaskCount: 0, + claudeBackgroundJobShort: "bg-42", + }, "gone"); + + const outcome = await wiring.runSettleTeardown("session-1", neverAborted); + + expect(hasLiveClaudeBackgroundJob).toHaveBeenCalledWith("bg-42"); + expect(interrupt, "a finished job must not be stopped again").not.toHaveBeenCalled(); + expect(outcome.residue, "a finished job is not residue").toEqual([]); + }); + + it("treats an unreachable daemon as work, not as a finished job", async () => { + // `getLiveClaudeBackgroundSocket` turns socket and request failures into a + // null, so "cannot reach the daemon" and "the job is gone" arrive looking + // identical. Guessing "finished" is the guess that settles over a job that + // is still running. + const { wiring, interrupt } = harness({ + status: "idle", + provider: "claude", + activeBackgroundTaskCount: 0, + claudeBackgroundJobShort: "bg-42", + }, "unknown"); + + await wiring.runSettleTeardown("session-1", neverAborted); + + expect(interrupt, "unknown liveness must still attempt the stop").toHaveBeenCalled(); + }); + + it("reports residue when a daemon job survives the interrupt", async () => { + // `interrupt`'s daemon `stop ` branch is gated on there being no + // resident Claude runtime, so a resumed session with a live `--bg` job + // takes the SDK branch and the daemon job keeps running. Teardown must not + // call that a clean settle — it cannot stop the job, so it says so. + const { wiring } = harness({ + status: "idle", + provider: "claude", + activeBackgroundTaskCount: 0, + claudeBackgroundJobShort: "bg-42", + }, "alive"); + + const outcome = await wiring.runSettleTeardown("session-1", neverAborted); + + expect(outcome.residue, "an unstoppable daemon job must be reported").not.toEqual([]); + expect(outcome.residue[0]?.kind).toBe("background_tasks"); + }); + + it("does not interrupt a session that is genuinely idle", async () => { + const { wiring, interrupt } = harness({ + status: "idle", + provider: "claude", + activeBackgroundTaskCount: 0, + }); + + const outcome = await wiring.runSettleTeardown("session-1", neverAborted); + + expect(interrupt).not.toHaveBeenCalled(); + expect(outcome.residue).toEqual([]); + }); +}); diff --git a/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts new file mode 100644 index 000000000..c0dbb4d51 --- /dev/null +++ b/apps/desktop/src/main/services/sessions/settleTeardownWiring.ts @@ -0,0 +1,135 @@ +import { createSessionSettleTeardown, residueCountBucket } from "./sessionSettleTeardown"; +import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } from "./sessionSettleTeardown"; +import type { ProductAnalyticsCapture } from "../../../shared/types/productAnalytics"; + +/** + * @file The settle-teardown wiring, shared by the desktop main process and the ADE + * brain. + * + * It lives here rather than at each construction site because there are TWO of + * them, and the one that matters most is the easiest to forget: in a normal + * install the brain owns phone sync, remote commands, and the PR-merge poller, + * so wiring only the desktop leaves teardown a silent no-op for every settle a + * user actually triggers from their phone or from `ade`. That is ADE's oldest + * bug class — works in-process, no-ops in the runtime-backed build — and a + * shared factory is what stops the two copies drifting apart. + */ + +/** Only what teardown needs, so neither caller has to hand over a whole service. */ +export type SettleTeardownChatService = { + interrupt: (args: { sessionId: string; mode: "stop_only" | "stop_and_clear" }) => Promise; + getSessionSummary: (sessionId: string) => Promise<{ + status: string; + activeBackgroundTaskCount?: number | null; + provider?: string | null; + claudeBackgroundJobShort?: string | null; + } | null>; + /** + * Liveness for a persisted Claude `--bg` job; the recorded short alone is not + * one. REQUIRED, not optional: a wiring without it would read a recorded job + * as absent and confirm a clean teardown over work that is still running, + * which is the hole this callback exists to close. + * + * `"unknown"` is a distinct answer on purpose — an unreachable daemon is not + * evidence the job finished. + */ + hasLiveClaudeBackgroundJob: ( + short: string | null | undefined, + ) => Promise<"alive" | "gone" | "unknown">; +}; + +export type SettleTeardownWiringDeps = { + agentChatService: SettleTeardownChatService; + /** + * Built here rather than at each call site: the two sites were byte-identical + * apart from `surface`, and the property allowlist has already bitten this + * feature once — two copies is two chances to drift out of it. + */ + analytics?: { captureInternal: (event: ProductAnalyticsCapture) => unknown } | null; + surface: "desktop" | "api"; + logger?: { + warn: (message: string, meta?: Record) => void; + info: (message: string, meta?: Record) => void; + }; +}; + +export type SettleTeardownWiring = { + runSettleTeardown: (sessionId: string, ctx: SettleTeardownContext) => Promise; + onRemoteSettleWrite: (args: { columns: string[]; changesetSessionCount: number }) => void; + onSettleResidue: (args: { provider: string | null; items: SettleResidueItem[] }) => void; +}; + +export function createSettleTeardownWiring(deps: SettleTeardownWiringDeps): SettleTeardownWiring { + const capture = (properties: Record): void => { + deps.analytics?.captureInternal({ + event: "ade_feature_used", + surface: deps.surface, + properties: { feature: "work", ...properties }, + }); + }; + const runSettleTeardown = createSessionSettleTeardown({ + interrupt: async (sessionId) => { + // `stop_only`, never `stop_and_clear`: the latter also cancels the user's + // QUEUED turns. Design 3c's rule is that losing a settle costs one click + // while losing the user's work is unrecoverable, and a queued prompt is + // the user's work. If a queued turn then starts, C3 clears the settle — + // which is R1, and already the accepted trade. + await deps.agentChatService.interrupt({ sessionId, mode: "stop_only" }); + }, + readActiveWork: async (sessionId) => { + const summary = await deps.agentChatService.getSessionSummary(sessionId); + if (!summary) return null; + // `activeBackgroundTaskCount` comes from the LIVE managed runtime, so it + // reads zero after a restart even while a Claude `--bg` job keeps running + // in the daemon. Counting the persisted job is what stops a settle from + // looking at a restarted session, seeing it quiet, and filing it as done + // over work it never stopped — the exact bug this feature exists to fix. + const liveCount = summary.activeBackgroundTaskCount ?? 0; + // Only asked when the live count says quiet AND a job is on record — the + // restart case. The daemon round-trip is not worth paying on every read, + // and the short is a record rather than a liveness signal: it survives the + // job finishing, so trusting it alone would make every later settle burn + // the confirmation budget and report residue that no longer exists. + const persistedBackgroundJob = liveCount === 0 && summary.claudeBackgroundJobShort + // Anything but a definite "gone" counts as work. An unreachable daemon + // means we cannot tell, and guessing "finished" is the one guess that + // settles over a running job. + && await deps.agentChatService.hasLiveClaudeBackgroundJob(summary.claudeBackgroundJobShort) !== "gone" + ? 1 + : 0; + return { + active: summary.status === "active", + backgroundTaskCount: Math.max(liveCount, persistedBackgroundJob), + provider: summary.provider ?? null, + }; + }, + ...(deps.logger ? { logger: deps.logger } : {}), + }); + + return { + runSettleTeardown, + onSettleResidue: ({ provider, items }) => { + // One event per settle that had residue, not one per failed job: a fleet + // that fails to stop must not become a burst. Coarse properties only — + // no session id, task id, command, or error text. + capture({ + action: "settle_teardown_residue", + outcome: items[0]?.reason ?? "failed", + count_bucket: residueCountBucket(items.reduce((total, item) => total + item.count, 0)), + ...(provider ? { provider } : {}), + }); + }, + onRemoteSettleWrite: ({ columns, changesetSessionCount }) => { + // Not a warning and not an anomaly: a paired second desktop replicating + // its own settles reaches this path by design. It is a RATE signal — how + // much settle traffic arrives already-decided — and the column names are + // a fixed set, so no session id or value is recorded. + deps.logger?.info("settle.remote_tuple_write_reconciled", { columns, changesetSessionCount }); + capture({ + action: "settle_remote_write_reconciled", + outcome: "partial", + count_bucket: residueCountBucket(changesetSessionCount), + }); + }, + }; +} diff --git a/apps/desktop/src/main/services/sessions/settleTerminalSession.ts b/apps/desktop/src/main/services/sessions/settleTerminalSession.ts index c942f9169..e1831686f 100644 --- a/apps/desktop/src/main/services/sessions/settleTerminalSession.ts +++ b/apps/desktop/src/main/services/sessions/settleTerminalSession.ts @@ -88,7 +88,7 @@ export async function settleTerminalSession(args: { if (!dismissed) return false; } - const result = args.sessionService.settleSessionReportingAbort( + const result = await args.sessionService.settleSessionReportingAbort( args.sessionId, { ...(args.opts?.outcome ? { outcome: args.opts.outcome } : {}), diff --git a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts index 56efe7e31..435eaa77b 100644 --- a/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts +++ b/apps/desktop/src/main/services/sessions/settlingStateRegistry.ts @@ -16,35 +16,13 @@ * 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"; +export type SettleAbortReason = + | "turn_start" + | "turn_failed" + | "attention_requested" + /** A peer changed the settle tuple; its decision outranks a settle in flight. */ + | "remote_lifecycle_changed"; /** Why a settle was abandoned, as reported to callers. */ export type SettleAbortedReason = @@ -69,11 +47,18 @@ export type SettlingEntry = { /** The revision the settle decision was taken against. */ startedAtRevision: number; abortedBy: SettleAbortReason | null; + /** + * Identifies THIS window. `end` refuses to close a window it did not open, so + * an owner whose window was already torn down out from under it (a session + * deleted mid-teardown) cannot close the one a newer settle has since opened + * for the same id — which would leave two teardowns believing they own it. + */ + token: number; }; export type BeginSettlingResult = - /** This caller owns the window and must end it. */ - | { kind: "started" } + /** This caller owns the window and must end it, passing back its token. */ + | { kind: "started"; token: number } /** * Another settle for this session is already in flight. The caller JOINS it * rather than starting a second teardown — closing R4, where two teardowns @@ -83,11 +68,13 @@ export type BeginSettlingResult = export class SettlingStateRegistry { private readonly entries = new Map(); + private nextToken = 1; begin(sessionId: string, startedAtRevision: number): BeginSettlingResult { if (this.entries.has(sessionId)) return { kind: "joined" }; - this.entries.set(sessionId, { startedAtRevision, abortedBy: null }); - return { kind: "started" }; + const token = this.nextToken++; + this.entries.set(sessionId, { startedAtRevision, abortedBy: null, token }); + return { kind: "started", token }; } isSettling(sessionId: string): boolean { @@ -113,11 +100,27 @@ export class SettlingStateRegistry { return this.entries.get(sessionId)?.abortedBy ?? null; } + /** + * Has THIS window been abandoned — either aborted, or replaced? + * + * A settle that no longer owns the id must stop as surely as an aborted one. + * `forget` (a deleted session) force-closes the entry, and if the id is then + * recreated a new settle opens a fresh window; a stale teardown consulting + * `abortedBy` alone would read the replacement, see "not aborted", and keep + * issuing provider stops against the new session's work. + */ + abandoned(sessionId: string, token: number): boolean { + const entry = this.entries.get(sessionId); + return !entry || entry.token !== token || entry.abortedBy !== null; + } + startedAtRevision(sessionId: string): number | null { return this.entries.get(sessionId)?.startedAtRevision ?? null; } - end(sessionId: string): void { + /** Closes the window only if `token` still owns it. Omit to force-close. */ + end(sessionId: string, token?: number): void { + if (token !== undefined && this.entries.get(sessionId)?.token !== token) return; this.entries.delete(sessionId); } diff --git a/apps/desktop/src/main/services/state/kvDb.test.ts b/apps/desktop/src/main/services/state/kvDb.test.ts index c66abc2fe..4e082411a 100644 --- a/apps/desktop/src/main/services/state/kvDb.test.ts +++ b/apps/desktop/src/main/services/state/kvDb.test.ts @@ -866,3 +866,75 @@ describe("retention maintenance", () => { } }); }); + +describe("inbound settle-tuple reconciliation hook", () => { + /** + * The path with the least margin for error in the settle-teardown work: a + * hand-rolled decode of cr-sqlite's packed primary key. If it is wrong, peer + * settles quietly stop being reconciled — no exception, no log, and the only + * visible symptom is a race that reappears months later. + * + * Driven through the real `applyChanges` rather than by calling the handler, + * so the whole chain is exercised: column detection, pk decode, and the + * guarantee that the change still APPLIES (holding it back would desynchronise + * the per-column clocks). + */ + it("reports settle-tuple columns to the handler and still applies them", async () => { + const projectRoot = makeProjectRoot("ade-kvdb-settle-hook-"); + const source = await openKvDb(path.join(projectRoot, "source", ".ade", "ade.db"), createLogger() as any); + activeDisposers.push(async () => source.close()); + const target = await openKvDb(path.join(projectRoot, "target", ".ade", "ade.db"), createLogger() as any); + activeDisposers.push(async () => target.close()); + + const crrLoaded = source.get<{ present: number }>( + "select 1 as present from sqlite_master where type = 'table' and name = 'terminal_sessions__crsql_clock' limit 1", + ) !== null; + // cr-sqlite ships macOS-only binaries; without it there are no changesets to + // apply and the assertions below would be vacuously true. + if (!crrLoaded) return; + + const insertSession = (db: typeof source) => { + db.run( + `insert into terminal_sessions (id, lane_id, title, status, started_at, tool_type) + values (?, ?, ?, ?, ?, ?)`, + ["peer-session", "lane-1", "Chat", "ended", "2026-08-11T00:00:00.000Z", "codex-chat"], + ); + }; + insertSession(source); + insertSession(target); + + const seen: Array<{ sessionId: string; column: string }> = []; + target.sync.setRemoteSettleTupleHandler((changes) => { seen.push(...changes); }); + + const before = source.sync.getDbVersion(); + source.run("update terminal_sessions set settled_at = ? where id = ?", ["2026-08-11T01:00:00.000Z", "peer-session"]); + const changes = source.sync.exportChangesSince(before) + .filter((change) => change.table === "terminal_sessions"); + expect(changes.length, "the peer must actually have exported a settle change").toBeGreaterThan(0); + + target.sync.applyChanges(changes); + + // The decode worked: the handler learned WHICH session and column moved. + expect(seen).toContainEqual({ sessionId: "peer-session", column: "settled_at" }); + // And the value landed, which is what keeps the per-column clocks convergent. + expect( + target.get<{ settled_at: string | null }>( + "select settled_at from terminal_sessions where id = ?", + ["peer-session"], + )?.settled_at, + ).toBe("2026-08-11T01:00:00.000Z"); + + // A re-delivered batch must report NOTHING. The peer's outbound cursor only + // advances on an ok ack, so a dropped ack re-sends the identical range — + // and reporting it would bump the lifecycle revision and trip the abort, + // killing a user's in-flight settle over a duplicate packet. + // + // `sqlite3_changes` cannot be used to detect this: `crsql_changes` is a + // virtual table, so SQLite counts the call even when cr-sqlite discards the + // row as a losing merge. That guard was tried and measured inert; this test + // is what caught it. + const seenAfterFirst = seen.length; + target.sync.applyChanges(changes); + expect(seen.length, "a duplicate changeset must not be reported again").toBe(seenAfterFirst); + }); +}); diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index 4cd80c0e8..04e681d64 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -107,6 +107,12 @@ export type AdeDbSyncApi = { rejectOversizedVersionGroup?: boolean; }) => CrsqlChangeRow[]; applyChanges: (changes: CrsqlChangeRow[]) => ApplyRemoteChangesResult; + /** + * Claim inbound settle-tuple writes so they are reconciled through the settle + * chokepoint instead of landing raw. Registered by the session layer; `null` + * restores the plain apply. + */ + setRemoteSettleTupleHandler: (handler: ((changes: RemoteSettleTupleChange[]) => void) | null) => void; /** * Suppress unpublished local-site CRR rows for specific tables. Used when * local viewer state must be cleared without relaying those clears to sync @@ -895,8 +901,90 @@ const LOCAL_ONLY_CRR_EXCLUDED_TABLES = new Set([ // off the host that issued it, and putting it on the CRR `terminal_sessions` // row would add a per-column clock entry to the per-output-chunk write path. "session_lifecycle_revisions", + // Host-local record of work a settle teardown could not confirm it stopped. + // Not replicated: it describes processes on THIS host, and a peer showing + // "1 job could not be stopped" for a machine it cannot see would be a lie. + "session_settle_residue", ]); +/** + * The settle tuple, as it appears in an inbound changeset. + * + * This is the SECOND copy of this column set: `syncHostService`'s + * `HOST_AUTHORITATIVE_COLUMNS_BY_TABLE` has the same three for a different + * purpose (dropping phone writes outright, rather than re-asserting a peer's). + * They are deliberately separate — one is a policy about who may write, the + * other is a trigger for re-asserting the revision — but a column added to the + * settle tuple has to be added to both, and to `settleLifecycleWriter`'s + * assignment. Grep `settle_source` before changing any of them. + */ +const SETTLE_TUPLE_COLUMNS: ReadonlySet = new Set([ + "settled_at", + "settle_override", + "settle_source", +] as const); + +const settleTupleKey = (change: RemoteSettleTupleChange): string => + `${change.sessionId}\u0000${change.column}`; + +function readSettleTupleColumn( + db: DatabaseSyncType, + sessionId: string, + column: RemoteSettleTupleChange["column"], +): string | null { + const row = getRow>( + db, + `select ${column} as value from terminal_sessions where id = ?`, + [sessionId], + ); + const value = row?.value; + if (value == null) return null; + // Stringified rather than narrowed to `string`: TEXT affinity converts a + // numeric `val`, but not a blob, and a blob that CHANGED must not read as + // unchanged just because it is not a string. + return value instanceof Uint8Array ? Buffer.from(value).toString("base64") : String(value); +} + +function isSettleTupleChange( + change: CrsqlChangeRow, +): change is CrsqlChangeRow & { cid: RemoteSettleTupleChange["column"] } { + return change.table === "terminal_sessions" + && SETTLE_TUPLE_COLUMNS.has(change.cid as RemoteSettleTupleChange["column"]); +} + +/** + * The inverse of `packedCrsqlPrimaryKey` for the one shape that matters here: + * a single TEXT primary key, which is what `terminal_sessions.id` is. + * + * Returns null for anything else — a composite key, a non-text key, an + * unfamiliar packing. The caller then does NOT claim the row, so an + * unrecognised encoding degrades to the plain apply rather than to a silently + * dropped change. + */ +function decodeSingleTextCrsqlPrimaryKey(value: SyncScalar): string | null { + if (typeof value === "string") return value || null; + if (!isSyncScalarBytes(value)) return null; + const bytes = Buffer.from(value.base64, "base64"); + // [column count][type tag 0x0b = text][byte length][utf8 …] + if (bytes.length < 3 || bytes[0] !== 0x01 || bytes[1] !== 0x0b) return null; + const length = bytes[2] ?? 0; + if (bytes.length !== 3 + length) return null; + return bytes.subarray(3, 3 + length).toString("utf8") || null; +} + +/** + * One inbound settle-tuple column write, decoded for the session layer. + * + * Deliberately carries no value: the change has ALREADY been applied by CRR + * merge, which is the only thing that keeps the per-column clocks convergent. + * The session layer reads the resulting row, so it can never disagree with what + * actually landed. + */ +export type RemoteSettleTupleChange = { + sessionId: string; + column: "settled_at" | "settle_override" | "settle_source"; +}; + function listEligibleCrrTables(db: DatabaseSyncType): string[] { const tables = allRows<{ name: string; sql: string | null }>( db, @@ -3840,6 +3928,17 @@ function migrate(db: MigrationDb, rawDb: DatabaseSyncType) { ) `); + // What a settle teardown could not confirm it stopped (design 3d, option 3). + // Replaced wholesale per settle, so the row always describes the LAST settle + // rather than accumulating history the user cannot clear. + db.run(` + create table if not exists session_settle_residue ( + session_id text primary key, + recorded_at text not null, + items text not null + ) + `); + // Machine-local runtime guard for PR automation. This table intentionally // has no PRIMARY KEY so cr-sqlite does not register it as a CRR table. db.run(` @@ -4393,6 +4492,8 @@ export async function openKvDb( ), }; + // Registered by the session layer once the settle chokepoint exists. + let remoteSettleTupleHandler: ((changes: RemoteSettleTupleChange[]) => void) | null = null; const sync: AdeDbSyncApi = { isAvailable: () => crsqliteLoaded, getSiteId: () => desiredSiteId, @@ -4565,10 +4666,28 @@ export async function openKvDb( seq: Number(row.seq), })); }, + setRemoteSettleTupleHandler: (handler: ((changes: RemoteSettleTupleChange[]) => void) | null) => { + remoteSettleTupleHandler = handler; + }, applyChanges: (changes: CrsqlChangeRow[]) => { if (!crsqliteLoaded) return { appliedCount: 0, dbVersion: 0, touchedTables: [], rebuiltFts: false }; let appliedCount = 0; const touchedTables = new Set(); + // Settle-tuple writes APPLY normally and are then reported to the + // session layer, which re-asserts them through the settle chokepoint. + // + // An earlier version held them out of `crsql_changes` and rebuilt the + // intent afterwards. That is wrong, and measurably so: cr-sqlite merges + // last-writer-wins on a per-column `col_version`, and a column that never + // enters `crsql_changes` never raises the local counter. The host then + // stays behind the peer forever, so its NEXT genuine decision — a user + // unsettle, a keep-active pin — carries a lower version and is rejected + // by every peer. Two hosts disagree permanently, which is far worse than + // the bypass being fixed. Let CRR converge the values; the chokepoint's + // job here is the lifecycle revision, which is what makes an in-flight + // settle notice and abort. + const candidateSettleTuple: RemoteSettleTupleChange[] = []; + const settleTupleBefore = new Map(); runStatement(db, "BEGIN IMMEDIATE"); try { for (const rawChange of changes) { @@ -4588,6 +4707,19 @@ export async function openKvDb( // Reachable whenever a table is moved local-only while a paired peer // is still on a build that replicates it — i.e. during every rollout. if (LOCAL_ONLY_CRR_EXCLUDED_TABLES.has(rawChange.table)) continue; + // Decoded before the apply, reported only after it: an undecodable + // key still applies, it is simply not reconciled. + let settleTupleChange: RemoteSettleTupleChange | null = null; + if (remoteSettleTupleHandler && isSettleTupleChange(rawChange)) { + const sessionId = decodeSingleTextCrsqlPrimaryKey(rawChange.pk); + if (sessionId) { + settleTupleChange = { sessionId, column: rawChange.cid }; + const key = settleTupleKey(settleTupleChange); + if (!settleTupleBefore.has(key)) { + settleTupleBefore.set(key, readSettleTupleColumn(db, sessionId, rawChange.cid)); + } + } + } const change = normalizeIncomingCrsqlChange(db, rawChange); const result = runStatement( db, @@ -4607,6 +4739,11 @@ export async function openKvDb( ); appliedCount += result.changes; touchedTables.add(change.table); + // `insert or ignore` silently drops a change whose col_version does + // not beat the local clock, which is exactly what a re-delivered + // batch looks like. Reporting one of those would bump the revision + // and abandon an in-flight settle over a duplicate packet. + if (settleTupleChange) candidateSettleTuple.push(settleTupleChange); } if (purgeRetiredTerminalSessions(db) > 0) { touchedTables.add("terminal_sessions"); @@ -4617,6 +4754,35 @@ export async function openKvDb( throw err; } + // AFTER the commit on purpose: the handler writes, and re-entering a + // write inside this `BEGIN IMMEDIATE` would risk rolling back a peer's + // whole batch over one session row. The values already landed, so a + // failure here costs the revision bump, not the peer's decision. + // Only report a column whose VALUE actually moved. + // + // `sqlite3_changes` cannot answer this: `crsql_changes` is a virtual + // table, so SQLite counts the xUpdate call whether or not cr-sqlite + // discarded the row as a losing merge — `insert or ignore` never engages. + // Measured: re-applying an identical changeset still reports 1 change. + // Without a value comparison a re-delivered batch (the peer's outbound + // cursor only advances on an ok ack, so a dropped ack re-sends the same + // range) would bump the revision AND trip the abort, killing a user's + // in-flight settle over a duplicate packet carrying nothing new. + const remoteSettleTuple = candidateSettleTuple.filter((candidate) => { + const after = readSettleTupleColumn(db, candidate.sessionId, candidate.column); + return after !== (settleTupleBefore.get(settleTupleKey(candidate)) ?? null); + }); + if (remoteSettleTuple.length && remoteSettleTupleHandler) { + try { + remoteSettleTupleHandler(remoteSettleTuple); + } catch (error) { + logger.warn("sync.settle_tuple_reconcile_failed", { + count: remoteSettleTuple.length, + error: error instanceof Error ? error.message : String(error), + }); + } + } + return { appliedCount, dbVersion: sync.getDbVersion(), diff --git a/apps/desktop/src/shared/subagentCapabilities.ts b/apps/desktop/src/shared/subagentCapabilities.ts index b7c43cfbf..6cbf9f998 100644 --- a/apps/desktop/src/shared/subagentCapabilities.ts +++ b/apps/desktop/src/shared/subagentCapabilities.ts @@ -143,3 +143,14 @@ export function resolveSubagentCapability( } return NO_SUBAGENT_CAPABILITY; } + +/** + * Runtimes that cannot stop an individual piece of background work at all. + * + * A Codex chat has no per-subagent stop, so a settle teardown reports its + * leftover work as `no_stop_control` rather than as a stop that failed — a + * different fact, and the reason the residue reason field exists. Declared + * here with the other per-runtime facts rather than as a `provider === "codex"` + * check inside the teardown, which is what this module exists to prevent. + */ +export const PROVIDERS_WITHOUT_BACKGROUND_STOP_CONTROL: ReadonlySet = new Set(["codex"]); diff --git a/apps/desktop/src/shared/types/chat.ts b/apps/desktop/src/shared/types/chat.ts index 70c53c31f..c337e49ae 100644 --- a/apps/desktop/src/shared/types/chat.ts +++ b/apps/desktop/src/shared/types/chat.ts @@ -1622,6 +1622,17 @@ export type AgentChatSessionSummary = { pendingInputItemId?: string | null; /** Earliest armed, unpaused schedule for this chat. */ nextWakeAt: string | null; + /** + * A Claude `--bg` job this session started, when one is still recorded. + * + * A RECORD, not a liveness signal: it survives the job finishing and survives + * teardown stopping it, so a reader that needs liveness has to ask the daemon. + * Distinct from `activeBackgroundTaskCount` below, which is derived from the + * LIVE managed runtime and therefore reads zero after a restart even while + * the daemon job is still running — which is why settle teardown consults + * both. + */ + claudeBackgroundJobShort?: string | null; /** Authoritative provider-reported background tasks still running after the foreground turn. */ activeBackgroundTaskCount?: number; /** The same live work split into working vs monitoring (`classifyBackgroundWorkKind`). */ diff --git a/docs/features/sync-and-multi-device/README.md b/docs/features/sync-and-multi-device/README.md index cfa225621..410733e4d 100644 --- a/docs/features/sync-and-multi-device/README.md +++ b/docs/features/sync-and-multi-device/README.md @@ -400,7 +400,12 @@ only**. Two properties make it different from the table-level - **It is peer-scoped, and deliberately so.** A paired desktop runs the same `sessionService` chokepoint, so its settle writes are host-decided too and must keep replicating; broadening the filter would silently stop settle - propagating between two of one user's machines. + propagating between two of one user's machines. Those writes are not applied + blind, though: `applyChanges` reports settle-tuple columns whose value + actually moved, and the receiving host re-asserts them through its own + chokepoint so they gain its lifecycle revision and abort an in-flight settle + rather than overwriting it. See + [settle-teardown-design.md](../terminals-and-sessions/settle-teardown-design.md) §6d. - **A paired phone cannot opt out of it.** `isMobilePeer` resolves a record-backed peer through its **pairing record** — host-side truth — and falls back to the peer's own `hello` metadata only when the auth kind is not diff --git a/docs/features/sync-and-multi-device/crdt-model.md b/docs/features/sync-and-multi-device/crdt-model.md index 4b8a96872..87d51832c 100644 --- a/docs/features/sync-and-multi-device/crdt-model.md +++ b/docs/features/sync-and-multi-device/crdt-model.md @@ -259,8 +259,12 @@ standard SQL on the host side, so iOS stays in parity. validation there closes it. Such columns are declared host-authoritative and filtered out of the offending peer's inbound changesets; `terminal_sessions.settled_at` / `settle_override` / `settle_source` are - the current set. See - [Host-authoritative columns](./README.md#host-authoritative-columns-are-peer-scoped). + the current set. A peer that legitimately runs the same chokepoint still + replicates them; the receiving host re-asserts the merged value through its + own chokepoint so the write cannot bypass its lifecycle revision. See + [Host-authoritative columns](./README.md#host-authoritative-columns-are-peer-scoped) + and + [settle-teardown-design.md](../terminals-and-sessions/settle-teardown-design.md) §6d. ## Schema implications diff --git a/docs/features/terminals-and-sessions/README.md b/docs/features/terminals-and-sessions/README.md index 9c6daf03d..0c1babb2c 100644 --- a/docs/features/terminals-and-sessions/README.md +++ b/docs/features/terminals-and-sessions/README.md @@ -217,25 +217,31 @@ and in tests. boundary its colocated test can enforce by scanning the rest of the tree. `sessionService` holds the only instance; every settle, unsettle, override, and activity-clear path routes through it. The revision detects changes made - by THIS host — a sibling ADE process or a paired desktop peer's CRR write is - outside its scope, which - [settle-teardown-design.md](settle-teardown-design.md) §3a states precisely. + by THIS host; a peer's replicated settle-tuple write reaches it because the + changeset apply layer re-asserts the merged value through the chokepoint, so + an in-flight settle sees the world move and abandons — see + [settle-teardown-design.md](settle-teardown-design.md) §3a and §6d. - `apps/desktop/src/main/services/sessions/settleTerminalSession.ts` — single settlement transaction shared by direct IPC and the ADE action - registry. Settle writes lifecycle state only — it deliberately does NOT stop - the session's background work. That was attempted and removed: teardown is - async, and `settled_at` is written and cleared from seven places, so a - teardown-then-write settle races real activity (a user starting a turn during - a provider stop call gets their background work stopped AND no settle), and - every guard tried against it either read a column that turn-start never - updates or had to be repeated at each of the settle entry points. Making - settle stop work needs a synchronous lifecycle revision that teardown can be - serialized against; it is not a wrapper around the existing write. The approved - plan for doing it is - [settle-teardown-design.md](settle-teardown-design.md); its step 0 - precondition — `settled_at` becoming host-authoritative, so a phone replica - cannot defeat the coming revision guard by CRDT merge — has landed. Archive is - the one lifecycle path that does stop processes — see + registry. Settle now DOES stop the session's outstanding work, through + `sessionSettleTeardown.ts`: it interrupts the active turn and its background + work, confirms the session went quiet, and records what it could not confirm. + **Terminals are never touched** — a settle files a session as done, it does + not take the user's shell away. + + Getting there needed the whole of + [settle-teardown-design.md](settle-teardown-design.md), because the obvious + version was built and cut in #1059 after producing a P1 in each of six review + rounds. Teardown is async and the settle tuple is written and cleared from ten + places, so a teardown-then-write settle races real activity, and every guard + tried against it either read a column that turn-start never updates or had to + be repeated at each entry point. What made it work was doing it in order: the + tuple became host-authoritative (§3c-i), then one chokepoint owned every + mutation and moved a revision (§3a), then a settling window made teardown + visible, exclusive and abortable (§3b) — and only then could teardown be + awaited inside that window (§6). An unconfirmed stop still settles, with + recorded residue rather than silence (§3d option 3). Archive remains a + different, heavier path: it disposes sessions outright — see `laneService.archive`, where the ordering is load-bearing. `dismissPendingInput: true` first quiets an SDK chat through `agentChatService`, or clears a tracked diff --git a/docs/features/terminals-and-sessions/settle-teardown-design.md b/docs/features/terminals-and-sessions/settle-teardown-design.md index 95d2a2849..6ba015379 100644 --- a/docs/features/terminals-and-sessions/settle-teardown-design.md +++ b/docs/features/terminals-and-sessions/settle-teardown-design.md @@ -1,10 +1,11 @@ # Settle teardown — design -**Status:** reviewed and approved with amendments. Steps 0-2 of §5 are cleared -to implement; step 3 (attaching real teardown) waits until 1 and 2 are merged -and the race-matrix tests have been seen to pass. +**Status:** implemented. Steps 0-2 shipped in #1069, #1073 and #1075; the +race-matrix review passed and cleared step 3, which attaches real teardown. §6 +records what step 3 actually built, including where it departed from the plan +and why. -**Steps 0, 1 and 2 are implemented.** Step 0's host-side half is "Host enforcement +**Steps 0-3 are implemented.** Step 0's host-side half is "Host enforcement for pre-fix clients" in §3c-i; step 1 is the chokepoint and revision in §3a, whose implemented shape is recorded at the end of that section. @@ -414,7 +415,7 @@ of per-statement fsync and does not describe ADE. The persisted table therefore stays, with the in-process counter alongside it — see §3a, where that counter turned out to be load-bearing for monotonicity rather than merely a fallback. -### 3c-iii. Step 3 must bound the PR-merge retry (open requirement) +### 3c-iii. Bounding the PR-merge retry — resolved When `settleSessions` reports an abort, `prMergeAutoSettlementService` leaves the merged PR unhandled so a later poll retries it — otherwise the merge is consumed @@ -433,7 +434,7 @@ different way, so the next attempt should start from why: | Elapsed timer | Re-arms while a long turn is still running — exactly the case the bound exists to prevent. | | `session.runtimeState !== "running"` on the persisted row | Never observes turn completion for chat at all. Chat rows deliberately hold `status = "running"` between turns; only `chatSessionProjection` resolves an idle chat to `idle`. | -The workable signal is therefore **projected** chat state. Step 2 wires it as a +The workable signal is therefore **projected** chat state. It is wired as a narrow injected callback (`getChatLiveness`, matching `chatMentionService.listChatSessions`): the chat service answers `status` and `awaitingInput`, and only a tracked CLI session — whose row does not lie — falls @@ -525,8 +526,185 @@ three, and that is why it produced a defect every round. directly against the revision in `settleRaceMatrix.test.ts`, including the C4/C5 swallow on all three axes — the case that decides whether a real teardown can finish at all. -3. Only then attach real teardown, reusing `stopLaneRuntimeWork`'s shape. -4. Resolve 3d by decision before step 3. +3. **Landed.** Real teardown, reusing `stopLaneRuntimeWork`'s *shape* — an + ordered list of steps, each in its own try/catch — but not its body: that + function disposes chat sessions because it serves lane deletion, and a settle + must leave the session usable. See §6. +4. **Resolved** — 3d option 3, implemented in §6. Steps 1 and 2 are independently valuable: the chokepoint alone would have prevented findings 1, 3, 4, 6, 7, 9, 12, 13, and 15. + + +--- + +## 6. Step 3 as built + +### 6a. The seam became async, and why that is now safe + +Step 2 shipped a synchronous seam whose return type (`SettleTeardownCompleted`, +a branded value only a synchronous body can produce) made an awaited teardown a +**compile** error. Real stops are async — `agentChatService.interrupt` returns a +promise, as does every provider stop under it — so step 3 had to remove that +guard. It was not an obstacle to route around; it was a tripwire that had done +its job. + +The guard existed because bolting a deferred teardown onto a synchronous write +path is what produced a P1 in each of #1059's six rounds. What changed is not +the risk but the machinery: the settling window is **exclusive** (a second +settle joins rather than starting its own teardown, R4), **abortable** (a turn +start trips it mid-flight, R1/R6), and **in-memory** so a crash resolves to +not-settled. That is precisely what makes it safe to *hold across an await*. The +revision re-check and the abort check after the await are the suspension-point +guards, and the race matrix exercises both. + +So: `settleManyWithTeardown` is `async`, and `settleSessions`, +`settleSessionsReportingAborts`, `settleSession` and +`settleSessionReportingAbort` return promises. The typed outcome is unchanged. + +### 6b. What teardown actually stops + +`sessionSettleTeardown.ts`. Ordered, cheapest-to-lose first, abort checked +**before** each step, since the point of the abort is to skip work not yet done. + +| Step | Behavior | +|---|---| +| Read active work | No turn and no background work -> return immediately. A settle with nothing to tear down must not interrupt the session. A read that TIMES OUT is not the same as "no chat session" and reports residue instead — otherwise a slow host settles while claiming a clean teardown. | +| `interrupt` | Stops the active turn and its background work. A throw is `rejected`, not a silent pass. | +| Confirm | Poll `getSessionSummary` (backing off to 800ms) until quiet or the 5s budget expires. A single read straight after `interrupt` would call work that was already stopping "residue". Every provider call also has its own 10s ceiling; without it a hung control call holds the settling window open forever and the row can never be settled again. | + +Bulk settles run these **concurrently**, bounded, and reassemble results in the +caller's order. Serially, a bulk settle paid the confirmation budget once per +session — and iOS allows a settle command 30s in total, so three busy sessions +was already a guaranteed timeout while the settle ran on regardless. + +**Terminals are never touched, at any step.** A settle files a session as done; +it does not take the user's shell away, and ADE cannot re-spawn one it killed. + +### 6c. Residue (3d option 3, as implemented) + +Anything still running when the budget expires is recorded, and the settle still +lands. Each item carries a coarse `reason` — `no_stop_control` (a Codex chat has +no per-subagent stop at all), `timeout`, or `rejected` — and the number of jobs +it covers. Everything counted is work ADE tracks, so it stays eligible for the +ppid-based orphan reaper. Work that escaped the process tree +(`nohup`/`setsid`/`disown`) is invisible to the confirmation read, so it is +never folded into that count and never overstated as recoverable — the design +requires the distinction, and here it holds by construction rather than by a +flag that could only ever read `true`. + +Residue lives in `session_settle_residue`, a **local-only** table: it describes +processes on this host, and a peer showing "1 job could not be stopped" for a +machine it cannot see would be a lie. `getSettleResidue` returns null unless the +session is still settled, so reactivating a session clears the marker without a +second write path to keep in sync. Residue is recorded **only when the settle +actually landed** — an abandoned settle has no settled row to describe. + +Analytics: one `ade_feature_used` per settle that had residue — never one per +failed job — with `provider`, the coarse `outcome` reason, and a bucketed +`count_bucket`. No session ids, task ids, commands, or error text. + +### 6c-i. Open: the sync bulk settle still answers with a bare id list + +`§3c`'s contract table says the sync entry point should carry the typed outcome +"additively". It does not yet. `session.settleSessions` +(`syncRemoteCommandService.ts`) returns `sessionService.settleSessions(...)` — a +changed-id array — so an aborted id is simply absent, indistinguishable from one +that was never eligible. iOS reads it as `resultShape: .changedIdList`. + +This is not a regression and not silently wrong: iOS shows an in-flight settle +through a local overlay that expires on its own (`PendingSessionSettleStates`, +20s), so an aborted settle reads as "the overlay timed out" rather than as a +settled row. Step 3 makes aborts more likely, though, which makes the gap worth +closing. + +It is left open deliberately because the fix is a **wire-compatibility decision +that needs the mobile side**, and either option costs something: + +- **Change the shape** to `{settled, aborted}` — breaks older iOS builds, which + parse an array. +- **Add `session.settleSessionsWithOutcome`** alongside it and register it in + the mobile compatibility list — genuinely additive, but ships a wire surface + with no consumer until iOS adopts it. + +Neither belongs in a desktop/CLI branch on its own. + +### 6c-ii. Known limitation: a timed-out provider stop cannot be recalled + +Every provider call has a 10s ceiling, without which a hung control call holds +the settling window open forever and the row can never be settled again. The +losing arm of that race keeps running, though: `agentChatService.interrupt` +takes no abort signal. So a session-scoped stop that overruns — OpenCode's +`session.abort` — could in principle land after the settle was abandoned and +stop a turn the user started in the meantime, which §3c says must never happen. + +This is a trade between a certain failure and a narrow one. Removing the ceiling +makes the wedge certain; keeping it needs a provider stop to overrun 10s AND the +user to start a turn inside that window AND the late abort to still apply, and a +provider hung that long is usually not delivering the abort either. Closing it +properly means threading an `AbortSignal` through every provider branch of +`interrupt`, which is its own change. + +### 6d. Peer tuple writes: host authority finished, not consensus added + +R7's fix is in the **apply layer**: `db.sync.applyChanges`, the one place both +the host and peer paths funnel through. An inbound change to `settled_at`, +`settle_override` or `settle_source` **applies normally**, and the session layer +is then told which sessions and columns moved so it can re-assert them through +the chokepoint. + +**It does not hold the change back, and that correction matters.** The first +implementation kept settle-tuple rows out of `crsql_changes` and rebuilt the +remote intent afterwards. A probe against the vendored cr-sqlite build showed +why that is wrong: merges are last-writer-wins on a per-column `col_version`, +and a column that never enters `crsql_changes` never raises the local counter. +The host stays behind the peer permanently, so its **next** genuine decision — a +user unsettle, a keep-active pin, a PR-merge settle — carries a lower version +and is rejected by every peer. Two hosts then disagree forever. That is a +strictly worse failure than the bypass being fixed, and it would not have shown +up in any single-host test. + +So the division is: **CRR owns the values, the chokepoint owns the revision.** +Reconciliation writes the tuple to its own current values — a self-assignment +that matches the row (so the revision bumps) without changing it (so cr-sqlite +records no new column version and nothing echoes back). The revision bump is the +whole point: an in-flight settle re-reads it after its teardown await, sees the +world moved, and abandons instead of overwriting the peer's decision. That is +the R7 mirror case — a peer reactivating a session mid-settle — and it is tested. + +Other details that are load-bearing: + +- **Registered in BOTH processes.** The desktop main process and the ADE brain + each construct a `sessionService`, and in a normal install it is the *brain* + that applies changesets, serves phone sync and remote commands, and runs the + PR-merge poller. Wiring only the desktop would have left teardown a no-op for + almost every settle a user actually triggers. Both now build their hooks from + one `createSettleTeardownWiring` factory so they cannot drift. +- **Per session, best effort.** One unreadable row cannot cost the rest of the + batch its bump, and a failure costs only the revision — never the peer's + decision, which has already landed. +- **Undecodable key → no reconcile.** Only a single TEXT primary key is decoded. + Anything else still applies; it simply is not re-asserted. + +**Reconciliation also trips the abort, not just the revision.** The revision is +only re-read *after* teardown returns, so on its own it would let a teardown run +to completion and stop a turn the user had just started on the other device — +losing the work *and* the settle, which is the R2 shape §3c exists to prevent. +The peer write therefore aborts the window immediately, with its own +`remote_lifecycle_changed` reason. + +**Only changes cr-sqlite actually accepted are reported.** `insert or ignore` +silently drops a change whose `col_version` does not beat the local clock, which +is exactly what a re-delivered batch looks like. Reporting one would bump the +revision and abandon an in-flight settle over a duplicate packet. + +**No peer-visible concurrency token was built.** That is a protocol change, and +the evidence does not justify it yet. `onRemoteSettleWrite` measures how often +this path runs — and it is *not* an anomaly counter: a paired second desktop +replicating its own settles lands here by design. One event per changeset, never +one per session. + +R7/R7b are unchanged and still write the row with a raw `db.run`. They pin the +property that motivates the whole mechanism: a write that reaches the tuple +without the chokepoint is invisible to the guard. The reconciled path is +asserted separately, against the same shape the apply layer now produces. diff --git a/docs/logging.md b/docs/logging.md index d3a771333..773f49d81 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -119,6 +119,32 @@ key per preference combination bounds this to at most four accepted events per installation per UTC day, within the existing `ade_feature_used` and shared daily ceilings. +Settle teardown records two things at the session-service owner boundary, both +on the existing `ade_feature_used` event with `feature: "work"`. + +`action: "settle_teardown_residue"` fires when a settle landed but a stop could +not be confirmed (the design's 3d option 3). It carries `provider`, a coarse +`outcome` reason (`no_stop_control`, `timeout`, or `rejected`) and a bucketed +`count_bucket` (`1`, `2_5`, `6_plus`). **One event per settle, never one per +failed job** — a fleet that fails to stop must not become a burst — and the +bucket exists so a large fleet cannot widen the dimension either. No session id, +task id, command, or error text is recorded; the human-readable residue detail +stays on the local diagnostics row and never enters the payload. + +`action: "settle_remote_write_reconciled"` fires when an inbound changeset +carried settle-tuple columns and the session layer re-asserted them through the +chokepoint. It carries the coarse `outcome` and a bucketed `count_bucket` of how +many sessions one changeset covered. + +It is a **rate** signal, not an anomaly signal. A paired second desktop +replicating its own settles reaches this path by design, so a non-zero rate is +expected wherever two desktops are paired; what it measures is how much settle +traffic arrives already-decided, which is the evidence needed before anyone +designs a protocol-level concurrency token. **One event per changeset, never one +per session** — a bulk settle on the peer arrives as a single apply covering N +sessions, and reporting each would turn one remote action into an N-event +burst. + Applying an update is one transaction — app swap, background service reinstalled, service restarted, service answering — and the brain half failing (the app updated but the background service never came back) is its own product-level