Skip to content

Commit 3dd4170

Browse files
authored
Real settle teardown, and peer settle-tuple reconciliation (settle teardown, step 3) (#1076)
* feat: attach real settle teardown, and reconcile peer settle-tuple writes Step 3 of the settle-teardown design. The seam is async now. Step 2 shipped a branded synchronous return type that made an awaited teardown a compile error; real stops are async, so that guard had to go. It was a tripwire, not an obstacle: it existed because bolting a deferred teardown onto a synchronous write is what produced a P1 in each of #1059's six rounds, and what changed is the machinery under it. The settling window is exclusive, abortable and in-memory, which is exactly what makes it safe to HOLD across an await — the revision re-check and abort check after the await are the suspension-point guards, and the race matrix covers both. Teardown (sessionSettleTeardown.ts) reuses stopLaneRuntimeWork's SHAPE, not its body: that function disposes chat sessions because it serves lane deletion, and a settle must leave the session usable. Terminals are never touched. Abort is checked before each step, so a turn that wins the race keeps its work. R5 lands as 3d option 3: an unconfirmed stop still settles, with residue recorded — coarse reason, reapable flag, local-only table, cleared implicitly when the session is reactivated — plus one bucketed analytics event per settle. Never silent. Peer tuple writes (R7 + the unsettle mirror) are fixed by finishing host authority, not by adding consensus. db.sync.applyChanges is the one place both the host and peer paths funnel through, so settle-tuple changes are held out of the raw apply and replayed through the chokepoint, gaining the revision and window semantics. Held rather than dropped: a paired desktop's decision is legitimate, it just has to come through the front door. No peer-visible token was built; onRemoteSettleWrite is the telemetry that decides whether one is ever justified. R7/R7b keep their raw db.run bypass and are annotated as to why: they pin the property that motivates the interception. * test: cover the residue table in the CRR-exclusion invariant Step 3 adds a second host-local settle table, so the exclusion test that guards the revision table now guards both. The positive control on terminal_sessions is what keeps the assertion falsifiable. * fix: quality dual-review findings, including two blockers Track A found two blockers, both real. 1. The brain never wired any of this. apps/ade-cli/src/bootstrap.ts built its sessionService with no teardown seam and never registered the apply-layer handler — and in a normal install the brain, not the desktop, is what applies changesets and serves phone sync, remote commands and the PR-merge poller. Teardown was a no-op for almost every settle a user can actually trigger. Both processes now build their hooks from one createSettleTeardownWiring factory so they cannot drift. 2. Holding settle-tuple rows out of crsql_changes breaks LWW convergence. The reviewer proved it against the vendored cr-sqlite build: a column that never enters crsql_changes never raises the local col_version, so this host stays behind the peer permanently and its NEXT genuine decision loses every merge. Two hosts then disagree forever — strictly worse than the bypass being fixed. Reversed: CRR now owns the values and the chokepoint owns the revision, via an observeRemote intent that self-assigns (matches the row, so the revision bumps; changes nothing, so no new column version and no echo). This also dissolves the composed-intent and lost-batch findings, since no intent is reconstructed and nothing is held. Also fixed: stop_and_clear was destroying the user's queued turns on every settle (now stop_only — 3c says losing a settle costs a click, losing the user's work does not); both new analytics properties were silently dropped by the sanitizer, so the telemetry the design leans on did not work, now pinned by a test; unbounded provider awaits could hold the settling window open forever and leave a row permanently unsettleable; count_bucket measured a value that was always 1; session_settle_residue leaked past deleteSession; residue analytics fired for settles that never landed; the settling window could be closed by an owner that no longer held it. Track B: settleSession now delegates to the typed form instead of duplicating it, the provider stop-control fact moved to subagentCapabilities, dead fields (stopped, scheduled_work, reapable) removed, two orphaned JSDoc blocks reattached, stale test title and a dangling comment asserting the opposite of the design deleted, and residue got a read path via the action registry — 'discoverable' was a condition of 3d option 3, not a nice-to-have. * fix: second-round review findings across teardown, reconcile and latency Track A verified two load-bearing claims empirically against the vendored cr-sqlite: the observeRemote self-assignment bumps sqlite3_changes without touching the clock (no echo), and stop_only really does stop background work. The design holds. What it found on top: - session.getSettleResidue was added to CTO_ONLY but not the allowlist, so every call was refused. The read path 3d option 3 was signed off on did not actually exist. - Stale residue survived a clean re-settle: nothing cleared the row when a later teardown confirmed everything, so it kept reporting an old failure with an old timestamp. - A peer write bumped the revision but never tripped the abort. The revision is only re-read AFTER teardown, so teardown ran to completion and interrupted a turn the user had just started on the other device — losing the work AND the settle, which is exactly the R2 shape 3c exists to prevent. - The reconcile handler fired on changes had discarded, so a re-delivered batch abandoned an in-flight settle over a duplicate packet. - A timed-out liveness read was indistinguishable from 'not a chat session', so a slow host settled while claiming a clean teardown — the one outcome residue exists to prevent. - settle_remote_write_reconciled fired on the NORMAL desktop-peer path, one event per session. 'Expected zero' was wrong: a paired desktop replicating its own settles belongs here. Now one batched event per changeset, framed as a rate signal. - Bulk settle was serial, and per session now costs up to 15s. iOS allows 30s for the whole command, so three busy sessions was a guaranteed timeout. Now bounded-concurrent, results reassembled in the caller's order. - Leaked ~50 unref'd timers per settling session; 10Hz polling of an expensive read; lmstudio missing from the provider dimension. Track B: restored the settle methods' locality after my own earlier repair scattered them, put back two invariant comments that repair dropped, moved the duplicated analytics envelope into the shared factory, made residue report a surviving turn separately from surviving jobs, and covered the hand-rolled cr-sqlite pk decoder — the riskiest code in the diff — with a test that drives the real applyChanges path. * fix: third-round review — the dedup guard was inert, and residue could be erased Track A proved my `result.changes > 0` guard does nothing. `crsql_changes` is a VIRTUAL table, so SQLite counts the xUpdate call whether or not cr-sqlite discarded the row as a losing merge — `insert or ignore` never engages, and a re-applied identical changeset still reports one change. Worse, round 2 had just made reconciliation trip the abort, so a duplicate packet (the peer's outbound cursor only advances on an ok ack, so a dropped ack re-sends the same range) would have killed a user's in-flight settle while carrying no new information. Replaced with a real value comparison: capture the column before the apply, report only if it actually moved. The kvDb test now applies the same changeset twice and asserts nothing is reported the second time — it fails against the old guard, which is how the inertness reproduces. Track A also found clearSettleResidue treating 'could not check' as 'confirmed clean'. An empty residue array is also what you get before the chat service exists and when the confirmation read times out, so a settle that verified nothing was deleting an accurate report of work still running. Teardown now returns an explicit `confirmed` flag and only a confirmed-clean settle may erase. The no-op fallbacks in both processes return confirmed: false. Same class, second site: the read-timeout rule was enforced after the first read but not on the confirmation read, so a hung confirm still returned clean. readWork is now a discriminated result, which makes the compiler force both call sites to decide. Track B: replaced the worker pool with chunking — order is preserved for free, and perSession, the queue and the reassembly loop all disappear; hoisted the concurrency constant to module scope; fixed three docblocks my own insertions had detached from their functions; typed ACTIVITY_ABORTS so a future abort reason cannot be silently mis-bucketed; restored the items[0] guard; renamed sessionCount to changesetSessionCount since it counts the changeset, not the reconciliation; made the concurrency test fail by assertion instead of by vitest timeout; and pinned action reachability, which is what H1 slipped through. Not changed: Track B read the settle methods as still scattered. Verified against main — the method order is byte-identical to base, so the interleaving is the pre-existing layout, not damage. Left alone rather than risk a third structural move in this file. * docs: correct settle claims that step 3 made false The terminals README still said settle 'deliberately does NOT stop the session's background work' and that a peer's CRR write is outside the revision's scope. Both were true when written and are not now. Replaced with what actually happens, including why the ordering of steps 0-3 was the thing that made it work. The sync docs were accurate about the phone-only column filter but silent on the desktop-peer path, which is the one a reader would now come looking for. * docs: record the sync bulk-settle shape as an open wire decision 3c's table says the sync entry point should carry the typed outcome additively; it still answers with a bare changed-id array, so an aborted id is indistinguishable from an ineligible one. Not a regression and not silently wrong — iOS's local overlay expires on its own rather than showing a settled row — but step 3 makes aborts likelier, and the fix is a wire-compatibility call that needs the mobile side, so it is written down rather than guessed at. * fix: fourth-round findings — restore the worker pool, and stop over-claiming confirmation Track A verified the value-based dedup guard against the real cr-sqlite across nine scenarios, including the one that actually matters: an exact duplicate arriving in a LATER applyChanges call is not reported, while a genuinely new change still is. It also confirmed the snapshot placement, the absence of SQL injection (the column is narrowed by the type guard before interpolation), and that a numeric val is safe under TEXT affinity. Four fixes from that pass: - Reverted chunking back to the worker pool. I took that simplification last round and it cost real throughput: a chunk barrier idles the other workers until its slowest member finishes, and 'every teardown is bounded' is not 'every teardown takes the same time'. Measured at roughly 65s versus 20s for a 50-session sweep with a quarter of the rows unstoppable — aimed straight at the 30s iOS budget the concurrency exists to protect. The perSession map already gave request order, so the simplification bought nothing. - An abort during the confirmation loop returned confirmed: true. Nothing was confirmed and the work was still running, which is precisely the shape the flag was added to make impossible — one refactor away from erasing an accurate residue record. - A confirmation-read timeout discarded a provider it had already read, losing the analytics dimension for the residue most worth attributing. - The value guard read blobs as null, so a blob that changed looked unchanged. Out-of-contract for any real writer, but the guard it replaced did report it. Both new tests were probed and both initially failed to be meaningful: the provider test was hitting a microtask race where the immediate expire won the FIRST read, and the confirmation test tripped the abort before the loop it was meant to exercise, so it passed against the bug. Fixed both, then re-probed — they now fail against the pre-fix code. * fix: stop the settle pool when a persistence failure propagates CodeRabbit, and it is a failure mode this branch introduced: before step 3, settleMany was one statement for the whole batch, so there was no partial state to lose. Now each session settles individually, and a SQLite lock thrown from one of them rejected Promise.all while the other workers kept shifting the queue — settling sessions the caller had already given up on. The queue is drained on the first failure so no NEW work starts, the sessions already in flight finish rather than being abandoned half-written, and the error is rethrown only once every worker has stopped. Persistence failures still propagate rather than being dressed up as a settle outcome — that distinction was an earlier review finding and it stands. What did settle is durable, and settle is idempotent, so the caller's retry re-reports it instead of double-filing. * fix: honor stop_only everywhere, and see background jobs that outlived a restart Two from Codex, both real, both undermining the point of the feature. stop_only was only honored on the Claude path. The OpenCode, Cursor, Pi and Droid branches of interrupt call cancelQueuedSteers unconditionally and return before the mode is ever consulted — so a settle on those providers silently deleted the user's queued prompts, which is exactly the unrecoverable loss the mode was added to prevent. Gated every one of them. The default is stop_and_clear, so the Stop button is untouched; only teardown asks for stop_only. activeBackgroundTaskCount is derived from the LIVE managed runtime, so a Claude --bg job that survives a brain or app restart reads as zero. Teardown saw a quiet session, skipped interrupt entirely, and filed the row as settled while the daemon job kept running — the precise bug this whole feature exists to fix, reintroduced through a liveness read. The summary already resolved the persisted job for other consumers; it is now on the type and counted as work. Both pinned by tests probed against the pre-fix code. Not fixed: CodeRabbit re-posted the queue-drain comment against the previous head; the drain landed in d499220. Its second half — surfacing the partial outcome instead of throwing — is deliberate: a SQLite lock is not a settle outcome, and settle is idempotent, so the caller's retry re-reports what landed. * fix: gate the persisted background job on daemon liveness Codex, and it is a defect my own previous fix created. claudeBackgroundJobShort is a RECORD, not a liveness signal — it survives the job finishing and survives teardown stopping it. Counting it unconditionally meant every later settle on that session would spend the full confirmation budget and then report residue for a job that no longer exists, while trying to stop it again each time. Now the daemon is asked, through a narrow hasLiveClaudeBackgroundJob exported from agentChatService, and only when the live count already says quiet AND a job is on record — the restart case. That keeps the round-trip off the hot read while still closing the hole where a job outlives its runtime. Also documented, not fixed: a provider stop that overruns its 10s ceiling keeps running, because interrupt takes no abort signal, so a late session-scoped abort could stop a turn the user started after the settle was abandoned. Removing the ceiling is a certain wedge; keeping it is a narrow race needing a 10s+ hang, a new turn inside that window, and the abort still applying. Written up in the design doc (6c-ii) rather than silently traded. Two other comments on this head are stale re-posts: the stop_only gating landed in 86c4c5c (verified present in all seven provider branches), and the queue-drain in d499220. * fix: make background-job liveness required and tri-state Both from this round's review, and both are defects my own liveness fix introduced one commit earlier. hasLiveClaudeBackgroundJob was optional, so a wiring that omitted it read a recorded job as absent and confirmed a clean teardown over work still running — reopening the exact hole the callback was added to close. Now required. And it returned a boolean, which collapsed "the daemon says the job is gone" into the same answer as "the daemon could not be reached" (getLiveClaudeBackgroundSocket catches socket and request failures and returns null). Guessing "finished" is the guess that settles over a running job — the same shape as treating a timed-out liveness read as an idle session, which this branch has now had to fix three times in three places. It returns alive / gone / unknown, and only a definite "gone" counts as no work. Also pinned: interrupt's daemon stop branch is gated on there being no resident Claude runtime, so a resumed session with a live --bg job takes the SDK branch and the job survives. Teardown does not claim that as clean — the confirmation loop still sees the job and reports residue — and there is now a test saying so. Changing that gate would change what the Stop button does, which is not this branch's call. Restored the activeBackgroundTaskCount JSDoc my insertion had detached. * fix: scope the abort check to the window the teardown actually owns Codex, and it is the mirror of a fix already made for closing the window. `end(id, token)` refuses to close a window it does not own, but the abort check still read `abortedBy(id)` — whatever entry currently occupies the id. When `deleteSession` runs mid-teardown, `forget` force-closes the entry; if the id is then recreated and a new settle opens a fresh window, the stale teardown reads the REPLACEMENT, sees "not aborted", and keeps issuing provider stops against the new session's work. `abandoned(id, token)` treats a missing or mismatched entry as abandoned, and both the in-flight check and the post-await check use it. A settle that no longer owns its window must stop as surely as one that was aborted. The other seven comments on this head are threads GitHub re-anchored: stop_only gating (86c4c5c, verified in all seven provider branches), the queue drain (d499220), persisted-job liveness and unknown-as-residue (df2aa0a), the JSDoc association (df2aa0a), the resident-runtime daemon job (pinned as residue rather than silently clean, with a test), and the un-cancellable timed-out interrupt, which is documented as a known limitation in 6c-ii because removing the ceiling trades a narrow race for a certain wedge. * fix: a hung provider stop is a timeout, not a rejection Codex. The 10s ceiling set `stopRejected`, so a provider that never answered was filed identically to one that explicitly refused — in the residue the user reads AND in the settle_teardown_residue analytics dimension. That conflation is precisely what the reason field exists to prevent, and it would have made "how often do stops actually fail in the field" unanswerable, which is the question 3d option 3 added the event to answer. Two existing assertions had encoded the bug rather than catching it: both said a never-resolving interrupt should read "rejected". Corrected, and the fix was probed against them.
1 parent 2617d1d commit 3dd4170

31 files changed

Lines changed: 2302 additions & 287 deletions

apps/ade-cli/src/bootstrap.ts

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,12 @@ import {
2626
createSessionService,
2727
STALE_RUNNING_SESSION_FRESH_ACTIVITY_GRACE_MS,
2828
} from "../../desktop/src/main/services/sessions/sessionService";
29+
import { createSettleTeardownWiring } from "../../desktop/src/main/services/sessions/settleTeardownWiring";
30+
import type {
31+
SettleResidueItem,
32+
SettleTeardownContext,
33+
SettleTeardownOutcome,
34+
} from "../../desktop/src/main/services/sessions/sessionSettleTeardown";
2935
import { createProjectConfigService } from "../../desktop/src/main/services/config/projectConfigService";
3036
import { createConflictService } from "../../desktop/src/main/services/conflicts/conflictService";
3137
import { createGitOperationsService } from "../../desktop/src/main/services/git/gitOperationsService";
@@ -755,7 +761,30 @@ export async function createAdeRuntime(args: {
755761
// services. Session changes still use it once publishing is attached.
756762
let pushPublisherForPtySignals: PushPublisherService | null = null;
757763
let ptyServiceForSessionChanges: ReturnType<typeof createPtyService> | null = null;
758-
const sessionService = createSessionService({ db });
764+
// Late-bound: the chat service that owns the work is constructed further
765+
// down. Without this the brain — which owns phone sync, remote commands and
766+
// the PR-merge poller in a normal install — would settle sessions while
767+
// stopping nothing.
768+
const settleTeardownRef: {
769+
run: ((sessionId: string, ctx: SettleTeardownContext) => Promise<SettleTeardownOutcome>) | null;
770+
report: ((args: { columns: string[]; changesetSessionCount: number }) => void) | null;
771+
residue: ((args: { provider: string | null; items: SettleResidueItem[] }) => void) | null;
772+
} = { run: null, report: null, residue: null };
773+
const sessionService = createSessionService({
774+
db,
775+
runSettleTeardown: async (sessionId, ctx) =>
776+
settleTeardownRef.run ? await settleTeardownRef.run(sessionId, ctx) : { residue: [], confirmed: false },
777+
onRemoteSettleWrite: (args) => settleTeardownRef.report?.(args),
778+
onSettleResidue: (args) => settleTeardownRef.residue?.(args),
779+
});
780+
// Inbound settle-tuple writes get this host's lifecycle revision, so an
781+
// in-flight settle can see a peer's decision and abandon rather than
782+
// overwrite it. Registered here because the DB layer must not know what a
783+
// settle means — and because the brain, not the desktop, is where changesets
784+
// are actually applied in a normal install.
785+
db.sync.setRemoteSettleTupleHandler((changes) => {
786+
sessionService.reconcileRemoteSettleTuple(changes);
787+
});
759788
sessionService.onChanged((event) => {
760789
pushEvent("runtime", { type: "terminal_session_changed", event });
761790
const session = sessionService.get(event.sessionId);
@@ -1249,6 +1278,16 @@ export async function createAdeRuntime(args: {
12491278
countActiveForLane: (laneId) => agentChatService.countActiveForLane(laneId),
12501279
disposeForLane: (laneId) => agentChatService.disposeForLane(laneId),
12511280
};
1281+
const settleWiring = createSettleTeardownWiring({
1282+
agentChatService,
1283+
logger,
1284+
analytics: productAnalyticsService ?? null,
1285+
// The brain is the non-GUI runtime surface, matching its other analytics.
1286+
surface: "api",
1287+
});
1288+
settleTeardownRef.run = settleWiring.runSettleTeardown;
1289+
settleTeardownRef.report = settleWiring.onRemoteSettleWrite;
1290+
settleTeardownRef.residue = settleWiring.onSettleResidue;
12521291
}
12531292
autoRebaseActivityReady = true;
12541293
void autoRebaseService

apps/desktop/src/main/main.ts

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ import { releaseLaneRuntimeResources } from "./services/lanes/laneRuntimeLifecyc
8585
import { createOAuthRedirectService } from "./services/lanes/oauthRedirectService";
8686
import { createRuntimeDiagnosticsService } from "./services/lanes/runtimeDiagnosticsService";
8787
import { createSessionService } from "./services/sessions/sessionService";
88+
import type { SettleResidueItem, SettleTeardownContext, SettleTeardownOutcome } from "./services/sessions/sessionSettleTeardown";
89+
import { createSettleTeardownWiring } from "./services/sessions/settleTeardownWiring";
8890
import { createSessionDeltaService } from "./services/sessions/sessionDeltaService";
8991
import { createPtyService } from "./services/pty/ptyService";
9092
import { createSupervisedPtyLoader } from "./services/pty/supervisedPtyHost";
@@ -2870,10 +2872,34 @@ app.whenReady().then(async () => {
28702872
emitProjectEvent(projectRoot, IPC.lanesEnvEvent, ev),
28712873
});
28722874

2873-
const sessionService = createSessionService({ db });
2875+
// Late-bound: the chat service that owns the work does not exist yet at
2876+
// this point, and the settle path must not depend on construction order.
2877+
const settleTeardownRef: {
2878+
run: ((sessionId: string, ctx: SettleTeardownContext) => Promise<SettleTeardownOutcome>) | null;
2879+
report: ((args: { columns: string[]; changesetSessionCount: number }) => void) | null;
2880+
residue: ((args: { provider: string | null; items: SettleResidueItem[] }) => void) | null;
2881+
} = { run: null, report: null, residue: null };
2882+
const sessionService = createSessionService({
2883+
db,
2884+
onRemoteSettleWrite: (args) => settleTeardownRef.report?.(args),
2885+
onSettleResidue: (args) => settleTeardownRef.residue?.(args),
2886+
runSettleTeardown: async (sessionId, ctx) =>
2887+
settleTeardownRef.run
2888+
? await settleTeardownRef.run(sessionId, ctx)
2889+
// Before the chat service is up there is no background work to stop,
2890+
// so an empty teardown is the honest answer, not a skipped one.
2891+
: { residue: [], confirmed: false },
2892+
});
28742893
sessionService.onChanged((event) => {
28752894
emitProjectEvent(projectRoot, IPC.sessionsChanged, event);
28762895
});
2896+
// Inbound settle-tuple writes go through the chokepoint instead of landing
2897+
// raw, so a peer's decision gains this host's revision, settling window and
2898+
// abort semantics (R7). Registered here because the DB layer must not know
2899+
// what a settle means.
2900+
db.sync.setRemoteSettleTupleHandler((changes) => {
2901+
sessionService.reconcileRemoteSettleTuple(changes);
2902+
});
28772903
const processRegistry = createProcessRegistryService({
28782904
db,
28792905
logger,
@@ -3600,6 +3626,17 @@ app.whenReady().then(async () => {
36003626
countActiveForLane: (laneId) => agentChatService.countActiveForLane(laneId),
36013627
disposeForLane: (laneId) => agentChatService.disposeForLane(laneId),
36023628
};
3629+
{
3630+
const wiring = createSettleTeardownWiring({
3631+
agentChatService,
3632+
logger,
3633+
analytics: productAnalyticsService ?? null,
3634+
surface: "desktop",
3635+
});
3636+
settleTeardownRef.run = wiring.runSettleTeardown;
3637+
settleTeardownRef.report = wiring.onRemoteSettleWrite;
3638+
settleTeardownRef.residue = wiring.onSettleResidue;
3639+
}
36033640
autoRebaseActivityReady = true;
36043641
void autoRebaseService
36053642
.refreshActiveRebaseNeeds("activity_services_ready")

apps/desktop/src/main/services/adeActions/registry.test.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,11 @@ describe("isAllowedAdeAction", () => {
7575
expect(isAllowedAdeAction("session", "requestSessionAttention")).toBe(true);
7676
expect(isAllowedAdeAction("session", "setSessionStatusNote")).toBe(true);
7777
expect(isAllowedAdeAction("session", "settleSession")).toBe(true);
78+
// The residue read path. It was added to the CTO-only list but NOT to the
79+
// allowlist, which silently refused every call — and left the settle design
80+
// claiming a user-visible guarantee ("settled never quietly means something
81+
// is still running") that nothing could actually reach.
82+
expect(isAllowedAdeAction("session", "getSettleResidue")).toBe(true);
7883
expect(isAllowedAdeAction("session", "unsettleSession")).toBe(true);
7984
expect(isCtoOnlyAdeAction("session", "settleSession")).toBe(true);
8085
expect(isCtoOnlyAdeAction("session", "unsettleSession")).toBe(true);

apps/desktop/src/main/services/adeActions/registry.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -734,6 +734,7 @@ export const ADE_ACTION_ALLOWLIST: Partial<Record<AdeActionDomain, readonly stri
734734
"get",
735735
"getDelta",
736736
"getLifecycleSettings",
737+
"getSettleResidue",
737738
"list",
738739
"readTranscriptTail",
739740
"requestSessionAttention",
@@ -2193,6 +2194,20 @@ function buildSessionDomainService(runtime: AdeRuntime): OpaqueService | null {
21932194
sessionService.unsettleSessions(sessionIds);
21942195
return { ok: true };
21952196
},
2197+
/**
2198+
* Work a settle could not confirm it stopped (design 3d option 3).
2199+
*
2200+
* Read-only, and the reason it exists: option 3 was signed off on the
2201+
* condition that the residue stay DISCOVERABLE rather than merely recorded.
2202+
* Without a read path, "settled" would quietly mean "and something may still
2203+
* be running" — the exact outcome the option was chosen to avoid.
2204+
*/
2205+
getSettleResidue: (args?: unknown) => {
2206+
const record = readObjectActionArg(args, "session.getSettleResidue");
2207+
const sessionId = typeof record.sessionId === "string" ? record.sessionId : "";
2208+
if (!sessionId) throw new Error("session.getSettleResidue requires sessionId.");
2209+
return sessionService.getSettleResidue(sessionId) ?? { recordedAt: null, items: [] };
2210+
},
21962211
// -----------------------------------------------------------------------
21972212
// Snooze / wake / settle-override. Snooze is a synced VISIBILITY overlay:
21982213
// it hides a row until its deadline without touching lifecycle columns, so

apps/desktop/src/main/services/ai/tools/ctoOperatorTools.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -554,7 +554,7 @@ export function createCtoOperatorTools(deps: CtoOperatorToolDeps): Record<string
554554
}),
555555
execute: async ({ sessionId, outcome }) => {
556556
try {
557-
const result = deps.sessionService.settleSessionReportingAbort(sessionId, {
557+
const result = await deps.sessionService.settleSessionReportingAbort(sessionId, {
558558
...(outcome ? { outcome } : {}),
559559
source: "operator",
560560
});

apps/desktop/src/main/services/analytics/productAnalyticsPolicy.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ const STRING_PROPERTIES = new Set([
9393
"duration_bucket", "error_kind", "route_kind", "connection_state", "drop_reason", "source", "mode",
9494
"entry_point", "release_channel", "summary_kind", "reason", "last_command", "leg", "code",
9595
"escalation_reason", "install_source", "trigger", "from_version", "to_version", "user_action",
96-
"tool_error_kind", "crash_reason",
96+
"tool_error_kind", "crash_reason", "count_bucket",
9797
]);
9898
const NUMBER_PROPERTIES = new Set([
9999
"sent_count", "dropped_count", "interaction_count", "session_count", "chat_session_count",
@@ -120,6 +120,10 @@ const ANALYTICS_ONLY_ACTIONS = new Set([
120120
"mention_expanded",
121121
"transaction_failed",
122122
"scope_selected",
123+
// Settle teardown: work a settle could not confirm it stopped, and a peer
124+
// settle-tuple write that had to be reconciled through the chokepoint.
125+
"settle_teardown_residue",
126+
"settle_remote_write_reconciled",
123127
]);
124128

125129
const EVENT_PROPERTY_KEYS: Record<ProductAnalyticsEventName, ReadonlySet<string>> = {
@@ -132,7 +136,7 @@ const EVENT_PROPERTY_KEYS: Record<ProductAnalyticsEventName, ReadonlySet<string>
132136
ade_project_opened: new Set(["route_kind", "source", "mode", "connection_state"]),
133137
ade_feature_used: new Set([
134138
"feature", "action", "outcome", "source", "mode", "provider", "model_family", "duration_bucket", "connection_state",
135-
"bytes_freed", "files_compressed",
139+
"bytes_freed", "files_compressed", "count_bucket",
136140
]),
137141
ade_work_session_started: new Set(["feature", "action", "outcome", "source", "mode", "provider"]),
138142
ade_work_session_completed: new Set([
@@ -183,6 +187,9 @@ const SAFE_STRING_VALUES: Partial<Record<string, ReadonlySet<string>>> = {
183187
outcome: new Set([
184188
"success", "started", "completed", "failure", "timeout", "opened", "cancelled", "approved", "denied",
185189
"partial", "failed", "idle_only", "immediate",
190+
// Settle teardown could not confirm a stop (design 3d). `timeout` above
191+
// covers the third case. Coarse on purpose: never the task or its error.
192+
"no_stop_control", "rejected",
186193
// Which half of a post-update transaction did not land. `swap` is
187194
// deliberately absent: the app half is already reported by
188195
// `ade_update_install_did_not_land`, so only the brain half is new signal.
@@ -194,12 +201,15 @@ const SAFE_STRING_VALUES: Partial<Record<string, ReadonlySet<string>>> = {
194201
// widened, so the scope control can never carry free text.
195202
"machine", "project", "account",
196203
]),
197-
provider: new Set(["codex", "openai", "claude", "cursor", "droid", "opencode", "pi", "gemini", "local", "other"]),
204+
provider: new Set(["codex", "openai", "claude", "cursor", "droid", "opencode", "pi", "gemini", "lmstudio", "local", "other"]),
198205
model_family: new Set([
199206
"gpt_5", "openai_reasoning", "claude_sonnet", "claude_opus", "claude_haiku", "cursor", "gemini",
200207
"grok", "local", "other",
201208
]),
202209
duration_bucket: new Set(["under_10s", "under_1m", "under_5m", "under_30m", "under_2h", "over_2h"]),
210+
// Bucketed, never a raw count: a fleet that fails to stop must not become a
211+
// high-cardinality dimension.
212+
count_bucket: new Set(["1", "2_5", "6_plus"]),
203213
route_kind: new Set(["desktop", "web"]),
204214
connection_state: new Set(["connected", "disconnected", "pairing", "direct", "relay", "error"]),
205215
drop_reason: new Set([

apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1213,6 +1213,42 @@ describe("product analytics producers", () => {
12131213
})).toMatchObject({ provider: "pi" });
12141214
});
12151215

1216+
it("keeps the settle-teardown properties through the sanitizer", () => {
1217+
// Both of these were silently dropped when first added: `action` is
1218+
// allowlisted separately from the event's key list, and `count_bucket` was
1219+
// registered in the key list and the value allowlist but never in the
1220+
// string-dispatch set, so it never reached either. The event still shipped,
1221+
// just anonymous — which is worse than not shipping, because the dashboard
1222+
// looks populated.
1223+
expect(sanitizeProductAnalyticsProperties("ade_feature_used", {
1224+
feature: "work",
1225+
action: "settle_teardown_residue",
1226+
outcome: "no_stop_control",
1227+
provider: "codex",
1228+
count_bucket: "2_5",
1229+
})).toEqual({
1230+
feature: "work",
1231+
action: "settle_teardown_residue",
1232+
outcome: "no_stop_control",
1233+
provider: "codex",
1234+
count_bucket: "2_5",
1235+
});
1236+
1237+
expect(sanitizeProductAnalyticsProperties("ade_feature_used", {
1238+
feature: "work",
1239+
action: "settle_remote_write_reconciled",
1240+
outcome: "partial",
1241+
})).toMatchObject({ action: "settle_remote_write_reconciled" });
1242+
1243+
// The bucket is still a closed set: a raw count must not slip through and
1244+
// widen the dimension.
1245+
expect(sanitizeProductAnalyticsProperties("ade_feature_used", {
1246+
feature: "work",
1247+
action: "settle_teardown_residue",
1248+
count_bucket: "37",
1249+
})).not.toHaveProperty("count_bucket");
1250+
});
1251+
12161252
it("maps automation completion and failed chat turns into canonical bounded outcomes", () => {
12171253
const captures: ProductAnalyticsCapture[] = [];
12181254
const analytics = settledAnalytics(captures);

apps/desktop/src/main/services/chat/agentChatService.ts

Lines changed: 47 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37495,7 +37495,13 @@ export function createAgentChatService(args: {
3749537495
} catch {
3749637496
// Ignore provider abort failures; SSE cancellation still tears the turn down.
3749737497
}
37498-
cancelQueuedSteers(managed, managed.runtime, "interrupted");
37498+
// `stop_only` exists so settle teardown can stop a turn WITHOUT
37499+
// discarding the user's queued follow-ups. Only the Claude path honoured
37500+
// it, so a settle on these providers silently deleted queued prompts —
37501+
// unrecoverable, and the opposite of the rule that losing a settle costs
37502+
// one click while losing the user's work does not. Default is
37503+
// `stop_and_clear`, so the Stop button is unaffected.
37504+
if (mode === "stop_and_clear") cancelQueuedSteers(managed, managed.runtime, "interrupted");
3749937505
persistChatState(managed);
3750037506
for (const pending of managed.runtime.pendingApprovals.values()) {
3750137507
managed.runtime.handle.client.postSessionIdPermissionsPermissionId({
@@ -37541,7 +37547,7 @@ export function createAgentChatService(args: {
3754137547
cancelCursorPermissionWaiter(w, "Cursor tool approval was cancelled because the turn was interrupted.");
3754237548
}
3754337549
rt.permissionWaiters.clear();
37544-
cancelQueuedSteers(managed, rt, "interrupted");
37550+
if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted");
3754537551
return result;
3754637552
}
3754737553

@@ -37554,15 +37560,17 @@ export function createAgentChatService(args: {
3755437560
} catch {
3755537561
// ignore
3755637562
}
37557-
cancelQueuedSteers(managed, rt, "interrupted");
37563+
if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted");
3755837564
cancelPendingPiInputs(managed);
3755937565
persistChatState(managed);
3756037566
return result;
3756137567
}
3756237568

3756337569
if (managed.session.provider === "pi") {
3756437570
piRuntimeSetupInterruptRequested.set(managed, true);
37565-
cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted");
37571+
if (mode === "stop_and_clear") {
37572+
cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted");
37573+
}
3756637574
setSessionIdle(managed);
3756737575
persistChatState(managed);
3756837576
return result;
@@ -37580,20 +37588,24 @@ export function createAgentChatService(args: {
3758037588
cancelDroidPermissionWaiter(w, "Droid tool approval was cancelled because the turn was interrupted.");
3758137589
}
3758237590
rt.permissionWaiters.clear();
37583-
cancelQueuedSteers(managed, rt, "interrupted");
37591+
if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted");
3758437592
return result;
3758537593
}
3758637594

3758737595
if (managed.session.provider === "droid") {
3758837596
droidRuntimeSetupInterruptRequested.set(managed, true);
37589-
cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted");
37597+
if (mode === "stop_and_clear") {
37598+
cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted");
37599+
}
3759037600
persistChatState(managed);
3759137601
return result;
3759237602
}
3759337603

3759437604
if (managed.session.provider === "cursor") {
3759537605
cursorRuntimeSetupInterruptRequested.set(managed, true);
37596-
cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted");
37606+
if (mode === "stop_and_clear") {
37607+
cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted");
37608+
}
3759737609
persistChatState(managed);
3759837610
return result;
3759937611
}
@@ -44283,6 +44295,34 @@ export function createAgentChatService(args: {
4428344295
dispatchSteer,
4428444296
cancelDispatchedSteer,
4428544297
interrupt,
44298+
/**
44299+
* Is a persisted Claude `--bg` job actually still running?
44300+
*
44301+
* `claudeBackgroundJobShort` is a RECORD, not a liveness signal — it stays
44302+
* on the session after the job finishes or is stopped. Settle teardown has
44303+
* to distinguish the two: counting a finished job as work makes every later
44304+
* settle spend the confirmation budget and then report residue that does
44305+
* not exist.
44306+
*/
44307+
hasLiveClaudeBackgroundJob: async (
44308+
short: string | null | undefined,
44309+
): Promise<"alive" | "gone" | "unknown"> => {
44310+
const normalized = normalizeClaudeBackgroundShort(short);
44311+
if (!normalized) return "gone";
44312+
const socketPath = await resolveClaudeDaemonControlSocket();
44313+
// No daemon socket, or a request that failed: we do not KNOW the job is
44314+
// gone. Collapsing that to "gone" is how a settle confirms a clean
44315+
// teardown over a job that is still running — the same mistake as
44316+
// treating a timed-out liveness read as an idle session.
44317+
if (!socketPath) return "unknown";
44318+
try {
44319+
const response = await sendClaudeDaemonRequest(socketPath, { op: "has", short: normalized });
44320+
if (response.ok !== true) return "unknown";
44321+
return response.alive === true || response.present === true ? "alive" : "gone";
44322+
} catch {
44323+
return "unknown";
44324+
}
44325+
},
4428644326
restoreCancelledQueue,
4428744327
recoverTurn,
4428844328
recoverCodexTurn,

apps/desktop/src/main/services/history/operationService.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ function createInMemoryAdeDb(): { db: AdeDb; raw: Database } {
8787
rebuiltFts: false,
8888
}),
8989
discardUnpublishedChangesForTables: () => {},
90+
setRemoteSettleTupleHandler: () => {},
9091
},
9192
flushNow: () => undefined,
9293
close: () => raw.close(),

0 commit comments

Comments
 (0)