Real settle teardown, and peer settle-tuple reconciliation (settle teardown, step 3) - #1076
Conversation
…ites 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.
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.
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.
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.
…d 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.
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.
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.
…laiming 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.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughSession settlement now performs asynchronous teardown, records unresolved work as residue, protects settling ownership, and reconciles remote settle tuples through CRR. Desktop and ADE CLI runtimes provide teardown and reporting callbacks after chat initialization. ChangesSession settlement lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts (1)
1243-1250: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an assertion that
detailnever reaches analytics.
SettleResidueItem.detailis documented insessionSettleTeardown.tsLine 50 as human-readable text for the diagnostics surface and never for analytics. It is free-form and embeds the provider and job counts. The suite pins the closed set forcount_bucketbut does not pin the rejection ofdetail. A future allowlist edit could admit it without failing a test.💚 Proposed addition
expect(sanitizeProductAnalyticsProperties("ade_feature_used", { feature: "work", action: "settle_teardown_residue", count_bucket: "37", })).not.toHaveProperty("count_bucket"); + + // `detail` is diagnostics-only free text. It must never widen into a + // property, because it embeds provider names and raw job counts. + expect(sanitizeProductAnalyticsProperties("ade_feature_used", { + feature: "work", + action: "settle_teardown_residue", + detail: "3 jobs on claude could not be stopped", + })).not.toHaveProperty("detail");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts` around lines 1243 - 1250, Add a test assertion in the sanitizeProductAnalyticsProperties coverage for the "ade_feature_used" event that supplies a detail field and verifies the returned analytics properties do not contain detail. Keep the existing closed-set count_bucket assertion unchanged.apps/desktop/src/main/services/sessions/sessionService.ts (1)
380-397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the orphaned doc block above
runSettleTeardown.Two consecutive JSDoc blocks precede
runSettleTeardown. The first block (Lines 380-387) describes the step 2 no-op seam and is no longer accurate. It also documents no member, because a second doc block follows it. Keep one block.♻️ Proposed change
- /** - * Stop the session's background work. Injected rather than per-call: teardown - * is a service capability, not something a caller decides. - * - * Step 2 ships with this absent — the settling window, the abort rule, and the - * revision guard all land and are tested against a NO-OP, so every race is - * exercised before there is any work to lose. Step 3 supplies the real one. - */ /** * Real teardown. Awaited INSIDE the settling window, which is what makes the🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/sessions/sessionService.ts` around lines 380 - 397, Remove the obsolete first JSDoc block above runSettleTeardown, including the Step 2 no-op seam description, and retain the following block as the sole documentation for runSettleTeardown.apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts (1)
215-242: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
confirmedis derived from the abort flag, not from the observed quiet state. Line 78 documentsconfirmedas true only when teardown confirmed the session went quiet, but the return statement computes it as!ctx.isAborted(). A teardown that exhausted the confirmation budget with work still running therefore reportsconfirmed: truealongside non-empty residue. The current consumer insessionService.tscompensates by also requiring!teardown.residue.length, so behavior is correct today, and no test pins the intended value for this path.
apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts#L215-L242: derive the flag from the observed state, for example!ctx.isAborted() && (!after || (!after.active && after.backgroundTaskCount === 0)).apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts#L266-L270: add a named regression test assertingconfirmed === falsewhen the budget expires with work still running and no abort occurred.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts` around lines 215 - 242, Update the return value in sessionSettleTeardown.ts at lines 215-242 so confirmed is true only when teardown was not aborted and the observed session is quiet: no after state, no active turn, and zero background tasks. In sessionSettleTeardown.test.ts at lines 266-270, add a named regression test asserting confirmed is false when the confirmation budget expires with work still running and no abort.apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts (1)
240-293: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftThe per-session loop defeats the bounded-concurrency settle pool.
settleManyWithTeardownnow runs teardown with a worker pool of four, and its doc block states the motivation is that a bulk settle must not pay the confirmation budget once per session in series. This loop callssettleSessionsReportingAbortswith a single id per iteration, so the pool degree is always one.Teardown now costs up to
STOP_CONFIRM_TIMEOUT_MS(5s) per session, plus provider-call ceilings. For thesweepscope,candidateSessionsForlists up to 500 sessions (Line 148). A poll pass over a lane with many busy sessions can therefore block for minutes and delay every later PR incandidates.Collect the eligible session ids first, then issue one batched call so the pool applies.
♻️ Proposed direction
const settledSessionIds: string[] = []; let abandonedThisPr = false; + const eligibleSessionIds: string[] = []; for (const session of rows) { ... if (abortedSessionIds.has(session.id)) { if (!(await isAtRest(session.id))) { abandonedThisPr = true; continue; } abortedSessionIds.delete(session.id); } - const settleResult = await args.sessionService.settleSessionsReportingAborts([session.id], { - outcome: `PR #${pr.githubPrNumber} merged`, - settledAt: polledAt, - source: "pr_merge", - }); - settledSessionIds.push(...settleResult.settled); - ... + eligibleSessionIds.push(session.id); } + if (eligibleSessionIds.length) { + const settleResult = await args.sessionService.settleSessionsReportingAborts(eligibleSessionIds, { + outcome: `PR #${pr.githubPrNumber} merged`, + settledAt: polledAt, + source: "pr_merge", + }); + settledSessionIds.push(...settleResult.settled); + for (const entry of settleResult.aborted) { + abandonedThisPr = true; + if (ACTIVITY_ABORTS.has(entry.reason)) abortedSessionIds.add(entry.sessionId); + } + }Note the batched form also fixes a second point: the current code re-arms
abortedSessionIdswithsession.idfrom the loop variable rather than withentry.sessionId. That is correct only because the batch holds exactly one id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts` around lines 240 - 293, Refactor the per-session settlement loop around settleSessionsReportingAborts to first collect all eligible session IDs while preserving the existing eligibility, at-rest, and abandoned-PR handling. Issue one batched settlement call with those IDs so settleManyWithTeardown can use its bounded worker pool, then process returned settled and aborted entries; when re-arming abortedSessionIds, use each result entry’s sessionId rather than the loop variable.apps/desktop/src/main/services/adeActions/registry.ts (1)
2205-2210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
requireNonEmptyStringforsessionId, as every sibling action does.The hand-rolled check accepts a whitespace-only
sessionIdand forwards it tosessionService.getSettleResidue. Every other handler inbuildSessionDomainServicevalidates throughrequireNonEmptyString, which trims and throws a consistent message.♻️ Proposed refactor
getSettleResidue: (args?: unknown) => { const record = readObjectActionArg(args, "session.getSettleResidue"); - const sessionId = typeof record.sessionId === "string" ? record.sessionId : ""; - if (!sessionId) throw new Error("session.getSettleResidue requires sessionId."); + const sessionId = requireNonEmptyString(record.sessionId, "sessionId"); return sessionService.getSettleResidue(sessionId) ?? { recordedAt: null, items: [] }; },As per coding guidelines: "Preserve existing application patterns before introducing new abstractions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/adeActions/registry.ts` around lines 2205 - 2210, Update getSettleResidue in buildSessionDomainService to validate record.sessionId with the existing requireNonEmptyString helper instead of the hand-rolled type and emptiness check, preserving the sibling handlers’ trimming behavior and consistent validation error.Source: Coding guidelines
apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts (1)
122-129: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an outbound CRR regression test for
observeRemote. Assert thatexportChangesSincereturns no settle-column rows after the self-assignment. This protects the documented clock-preservation behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts` around lines 122 - 129, Add an outbound CRR regression test covering the observeRemote branch in settleLifecycleWriter, execute the self-assignment, and assert that exportChangesSince returns no settle-column rows. Reuse the existing test setup and helpers, and preserve the documented clock-preservation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/sessions/sessionService.ts`:
- Around line 893-917: Update the concurrent worker logic in the
session-settling method around settleOne, settleMany, and writeSettleLifecycle
so the first persistence failure stops all workers from claiming additional
queue entries while preserving outcomes already recorded in perSession. Drain or
invalidate the shared queue on failure, await worker completion, then reassemble
and return the partial settled outcome; represent the failed session as an
aborted entry if the existing SettleSessionsOutcome contract requires returning
rather than throwing.
---
Nitpick comments:
In `@apps/desktop/src/main/services/adeActions/registry.ts`:
- Around line 2205-2210: Update getSettleResidue in buildSessionDomainService to
validate record.sessionId with the existing requireNonEmptyString helper instead
of the hand-rolled type and emptiness check, preserving the sibling handlers’
trimming behavior and consistent validation error.
In `@apps/desktop/src/main/services/analytics/productAnalyticsService.test.ts`:
- Around line 1243-1250: Add a test assertion in the
sanitizeProductAnalyticsProperties coverage for the "ade_feature_used" event
that supplies a detail field and verifies the returned analytics properties do
not contain detail. Keep the existing closed-set count_bucket assertion
unchanged.
In `@apps/desktop/src/main/services/prs/prMergeAutoSettlementService.ts`:
- Around line 240-293: Refactor the per-session settlement loop around
settleSessionsReportingAborts to first collect all eligible session IDs while
preserving the existing eligibility, at-rest, and abandoned-PR handling. Issue
one batched settlement call with those IDs so settleManyWithTeardown can use its
bounded worker pool, then process returned settled and aborted entries; when
re-arming abortedSessionIds, use each result entry’s sessionId rather than the
loop variable.
In `@apps/desktop/src/main/services/sessions/sessionService.ts`:
- Around line 380-397: Remove the obsolete first JSDoc block above
runSettleTeardown, including the Step 2 no-op seam description, and retain the
following block as the sole documentation for runSettleTeardown.
In `@apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts`:
- Around line 215-242: Update the return value in sessionSettleTeardown.ts at
lines 215-242 so confirmed is true only when teardown was not aborted and the
observed session is quiet: no after state, no active turn, and zero background
tasks. In sessionSettleTeardown.test.ts at lines 266-270, add a named regression
test asserting confirmed is false when the confirmation budget expires with work
still running and no abort.
In `@apps/desktop/src/main/services/sessions/settleLifecycleWriter.ts`:
- Around line 122-129: Add an outbound CRR regression test covering the
observeRemote branch in settleLifecycleWriter, execute the self-assignment, and
assert that exportChangesSince returns no settle-column rows. Reuse the existing
test setup and helpers, and preserve the documented clock-preservation behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0432bf3d-2fee-458d-892e-b78545ea8704
⛔ Files ignored due to path filters (5)
docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/crdt-model.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/settle-teardown-design.mdis excluded by!docs/**docs/logging.mdis excluded by!docs/**
📒 Files selected for processing (23)
apps/ade-cli/src/bootstrap.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/ai/tools/ctoOperatorTools.tsapps/desktop/src/main/services/analytics/productAnalyticsPolicy.tsapps/desktop/src/main/services/analytics/productAnalyticsService.test.tsapps/desktop/src/main/services/history/operationService.test.tsapps/desktop/src/main/services/onboarding/onboardingService.test.tsapps/desktop/src/main/services/prs/prMergeAutoSettlementService.tsapps/desktop/src/main/services/sessions/sessionService.test.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/sessions/sessionSettleTeardown.test.tsapps/desktop/src/main/services/sessions/sessionSettleTeardown.tsapps/desktop/src/main/services/sessions/settleLifecycleWriter.test.tsapps/desktop/src/main/services/sessions/settleLifecycleWriter.tsapps/desktop/src/main/services/sessions/settleRaceMatrix.test.tsapps/desktop/src/main/services/sessions/settleTeardownWiring.tsapps/desktop/src/main/services/sessions/settleTerminalSession.tsapps/desktop/src/main/services/sessions/settlingStateRegistry.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/shared/subagentCapabilities.ts
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.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d499220c86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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" }); |
There was a problem hiding this comment.
When an OpenCode, Cursor, Pi, or Droid session has queued follow-up input, this call still deletes that input: those branches of agentChatService.interrupt unconditionally call cancelQueuedSteers (and Pi also clears pendingSteers) without checking mode. Consequently a user settle or PR auto-settle can irreversibly discard queued prompts despite requesting stop_only; either make every provider honor this mode or use a teardown operation that preserves its queue.
Useful? React with 👍 / 👎.
| active: summary.status === "active", | ||
| backgroundTaskCount: summary.activeBackgroundTaskCount ?? 0, |
There was a problem hiding this comment.
When a Claude --bg job survives a brain/app restart, getSessionSummary can retain claudeBackgroundJobShort while reporting an idle/ended status and zero activeBackgroundTaskCount, because that count is derived only from a live managed runtime. This mapper therefore declares the session quiet, skips interrupt—including its existing branch that can stop a persisted Claude background job—and settles with no residue while the daemon job continues running. Include the persisted background-job liveness in this read or expose it through the summary's active-work fields.
Useful? React with 👍 / 👎.
…d 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c4c5c625
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return await Promise.race([ | ||
| work.then((value) => ({ ok: true, value }) as const), | ||
| expire().then(() => ({ ok: false }) as const), | ||
| ]); |
There was a problem hiding this comment.
When a provider interrupt exceeds 10 seconds, this Promise.race returns ok: false but leaves the original interrupt promise running. If the user starts a new turn and aborts the settle while that request is still pending, a session-scoped request such as OpenCode's session.abort can arrive later and stop the newly accepted work, violating the rule that the new turn wins the race. The timeout path needs to abort/cancel the underlying provider operation or otherwise prevent it from applying after teardown returns.
Useful? React with 👍 / 👎.
| const liveCount = summary.activeBackgroundTaskCount ?? 0; | ||
| const persistedBackgroundJob = summary.claudeBackgroundJobShort ? 1 : 0; | ||
| return { | ||
| active: summary.status === "active", | ||
| backgroundTaskCount: Math.max(liveCount, persistedBackgroundJob), |
There was a problem hiding this comment.
After a Claude --bg job finishes naturally—or after the new persisted-job interrupt successfully stops it—claudeBackgroundJobShort remains recorded, so this code continues counting one background task without consulting daemon liveness. waitForQuiet therefore polls for five seconds and records false residue on every subsequent settle, while also repeatedly trying to stop a nonexistent job. The current patch's fresh evidence is that the stop branch never clears the short and the existing getLiveClaudeBackgroundSocket helper is not used by this summary read.
Useful? React with 👍 / 👎.
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abec583b6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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" }); |
There was a problem hiding this comment.
When a session has a live persisted Claude --bg job and also has a resident Claude runtime—for example, after the user resumes the session while its daemon job is still running—this generic interrupt enters the normal Claude-runtime branch. That branch stops the SDK turn/subagents, while the daemon stop <short> branch in agentChatService.interrupt is gated on managed.runtime?.kind !== "claude"; the daemon job therefore continues consuming resources and is only reported as residue after the confirmation timeout. Teardown needs to stop the persisted daemon job independently of whether a Claude runtime is currently attached.
Useful? React with 👍 / 👎.
| active: summary.status === "active", | ||
| backgroundTaskCount: Math.max(liveCount, persistedBackgroundJob), | ||
| provider: summary.provider ?? null, |
There was a problem hiding this comment.
When a restarted Claude session has a recorded background-job short but the daemon liveness request fails transiently, hasLiveClaudeBackgroundJob returns false because getLiveClaudeBackgroundSocket catches socket/request errors and converts them to null. This expression then reports zero background tasks, causing teardown to take its confirmed-clean early return, skip the interrupt, and potentially erase existing residue while the daemon job is still running. Fresh evidence beyond the prior persisted-job liveness finding is that the new helper collapses “could not determine liveness” into “not live”; this boundary needs a tri-state/error result so an unconfirmed read produces timeout residue instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
37554-37567: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winFix: pi interrupt still discards queued steers on
stop_only.Line 37557 (
rt.pendingSteers.length = 0;) clears the queue unconditionally, before themodecheck at line 37563. Because of this, the queue is already empty by the timeif (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted");runs, so that guard is a no-op for every mode.The result: for the
piprovider,stop_onlystill discards queued user turns silently, with no cancellation notice emitted. This contradicts the intended fix ("stop_only is honored across all providers, preserving queued user turns") for this one provider. Every other provider block (opencode, cursor, droid) in this same function correctly gates the queue clear behind themodecheck and has no unconditional pre-clear.🐛 Proposed fix
if (managed.runtime?.kind === "pi") { const rt = managed.runtime; rt.interrupted = true; - rt.pendingSteers.length = 0; try { await rt.sdk.abort(); } catch { // ignore } if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted"); cancelPendingPiInputs(managed); persistChatState(managed); return result; }
cancelQueuedSteersalready emptiesrt.pendingSteersviasplice(0)when it runs, so the unconditional clear is redundant even forstop_and_clearand only harmful forstop_only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 37554 - 37567, Remove the unconditional rt.pendingSteers.length = 0 assignment from the pi runtime branch in the interrupt handler. Let the existing mode === "stop_and_clear" guard invoke cancelQueuedSteers(managed, rt, "interrupted"), preserving queued steers for stop_only while retaining cancellation behavior for stop_and_clear.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
37569-37577: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead-code no-op:
cancelQueuedSteerscalled with a stub empty array.In the pi/droid/cursor "runtime still starting up" interrupt branches,
cancelQueuedSteers(managed, { pendingSteers: [], activeTurnId: null }, "interrupted")always receives an empty array literal.cancelQueuedSteersreturns immediately whenpendingSteers.lengthis 0, so this call never does anything, in any mode.This is harmless today because no queued steers can exist before the runtime object is created, but it reads as if it clears something and adds confusion for future maintainers. Consider removing the
cancelQueuedSteerscall in these three setup-interrupt branches, since it has no effect.Also applies to: 37596-37602, 37605-37611
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 37569 - 37577, Remove the no-op cancelQueuedSteers call from all three pi/droid/cursor runtime-startup interrupt branches, including the shown managed.session.provider === "pi" branch. Preserve the existing interrupt flag update, setSessionIdle, persistChatState, and return flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/services/sessions/settleTeardownWiring.ts`:
- Around line 25-28: Make hasLiveClaudeBackgroundJob required in the teardown
wiring type and update every runtime implementation of settleTeardownWiring to
provide the liveness callback. Ensure the settlement logic around the persisted
Claude background-job check always invokes this callback, then run desktop
typecheck, sharded tests, build, and lint to verify no wiring path omits it.
In `@apps/desktop/src/shared/types/chat.ts`:
- Around line 1626-1635: Move the existing JSDoc describing
activeBackgroundTaskCount so it directly precedes that property, and keep the
persisted-runtime JSDoc directly attached to claudeBackgroundJobShort. Preserve
both descriptions and their distinct semantics.
---
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 37554-37567: Remove the unconditional rt.pendingSteers.length = 0
assignment from the pi runtime branch in the interrupt handler. Let the existing
mode === "stop_and_clear" guard invoke cancelQueuedSteers(managed, rt,
"interrupted"), preserving queued steers for stop_only while retaining
cancellation behavior for stop_and_clear.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 37569-37577: Remove the no-op cancelQueuedSteers call from all
three pi/droid/cursor runtime-startup interrupt branches, including the shown
managed.session.provider === "pi" branch. Preserve the existing interrupt flag
update, setSessionIdle, persistChatState, and return flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 800d904e-84c5-4bbe-9b80-b06f8edd8729
⛔ Files ignored due to path filters (1)
docs/features/terminals-and-sessions/settle-teardown-design.mdis excluded by!docs/**
📒 Files selected for processing (7)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/sessions/sessionSettleTeardown.tsapps/desktop/src/main/services/sessions/settleRaceMatrix.test.tsapps/desktop/src/main/services/sessions/settleTeardownWiring.test.tsapps/desktop/src/main/services/sessions/settleTeardownWiring.tsapps/desktop/src/shared/types/chat.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts
- apps/desktop/src/main/services/sessions/sessionService.ts
- apps/desktop/src/main/services/sessions/settleRaceMatrix.test.ts
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df2aa0ad68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? await runSettleTeardown(id, { | ||
| // Read live, not captured: the whole point is that a clearer can | ||
| // trip it while teardown is between stop calls. | ||
| isAborted: () => settleLifecycle.settling.abortedBy(id) !== null, |
There was a problem hiding this comment.
When a session is deleted while this awaited teardown is running, settleLifecycle.forget() force-closes its registry entry; if the same ID is then recreated and starts another settle, this callback reads the replacement entry and returns false unless that new window is itself aborted. The stale teardown can consequently continue issuing provider stops against work belonging to the recreated session. Capture begin.token here and treat a missing or mismatched registry entry as aborted, rather than consulting whichever entry currently occupies the ID.
Useful? React with 👍 / 👎.
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1fc48fd1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const reason = stopRejected | ||
| ? "rejected" as const |
There was a problem hiding this comment.
When interrupt exceeds the 10-second provider-call ceiling and work remains, the timeout branch sets stopRejected, so this mapping records reason: "rejected" rather than "timeout". This makes both the discoverable residue and settle_teardown_residue analytics misclassify slow or hung provider calls as explicit provider rejections; track the timeout arm separately from a rejected promise.
Useful? React with 👍 / 👎.
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9be4f22722
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // ignore | ||
| } | ||
| cancelQueuedSteers(managed, rt, "interrupted"); | ||
| if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted"); |
There was a problem hiding this comment.
When a busy Pi session has queued follow-ups, settle teardown invokes this branch with mode: "stop_only", but rt.pendingSteers.length = 0 has already deleted those prompts before this new guard runs. Fresh evidence beyond the earlier comment is that the partial fix guards only cancelQueuedSteers; the preceding direct queue clear remains unconditional, so Pi settles still irreversibly discard user input.
Useful? React with 👍 / 👎.
| * 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<string> = new Set(["codex"]); |
There was a problem hiding this comment.
When Codex background subagents remain after the confirmation budget, teardown has already called interruptActiveCodexSubagentTurns through agentChatService.interrupt, and that function issues turn/interrupt for every active child. Including Codex in this set therefore reports no_stop_control instead of a failed or timed-out attempted stop, making both the discoverable residue and analytics falsely claim that no control path exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/services/chat/agentChatService.ts (1)
37563-37573: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winFix: Pi interrupt clears the queued-steer array before the mode check runs.
rt.pendingSteers.length = 0;runs unconditionally, before theif (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted");check.cancelQueuedSteerscallsruntime.pendingSteers.splice(0)to get the list of cancelled steers. Because the array is already empty by that point,cancelQueuedSteersfinds nothing to cancel.This causes two problems:
- For
stop_onlymode (the settle-teardown path), queued follow-up messages for Pi sessions are discarded instead of preserved. This contradicts the PR's stated design goal that settle-only teardown must not discard queued follow-ups.- For
stop_and_clearmode (the Stop button), the user never sees the "Queued message cancelled" notice, becausecancelQueuedSteershas nothing left to iterate.Compare with the sibling Cursor and Droid blocks in the same function, which have no equivalent unconditional clear and rely only on the conditional
cancelQueuedSteerscall.🐛 Proposed fix
if (managed.runtime?.kind === "pi") { const rt = managed.runtime; rt.interrupted = true; - rt.pendingSteers.length = 0; try { await rt.sdk.abort(); } catch { // ignore } if (mode === "stop_and_clear") cancelQueuedSteers(managed, rt, "interrupted"); cancelPendingPiInputs(managed); persistChatState(managed); return result; }Do you want me to check whether
settleTeardownWiring.test.tsorsessionSettleTeardown.test.tscover Pi-session queue preservation duringstop_onlyinterrupts? If not, I can add a regression test for this case, as required by the path instructions for test files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 37563 - 37573, Remove the unconditional clearing of the Pi runtime’s pending-steer array before the mode check in the interrupt flow. Preserve queued steers for stop_only, and let the existing conditional cancelQueuedSteers call handle stop_and_clear so it can cancel and notify queued messages consistently with the Cursor and Droid branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 37563-37573: Remove the unconditional clearing of the Pi runtime’s
pending-steer array before the mode check in the interrupt flow. Preserve queued
steers for stop_only, and let the existing conditional cancelQueuedSteers call
handle stop_and_clear so it can cancel and notify queued messages consistently
with the Cursor and Droid branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fdc08d8-0aef-4bdc-86c7-2713011dfccf
📒 Files selected for processing (9)
apps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/sessions/sessionSettleTeardown.test.tsapps/desktop/src/main/services/sessions/sessionSettleTeardown.tsapps/desktop/src/main/services/sessions/settleRaceMatrix.test.tsapps/desktop/src/main/services/sessions/settleTeardownWiring.test.tsapps/desktop/src/main/services/sessions/settleTeardownWiring.tsapps/desktop/src/main/services/sessions/settlingStateRegistry.tsapps/desktop/src/shared/types/chat.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/shared/types/chat.ts
- apps/desktop/src/main/services/sessions/sessionSettleTeardown.test.ts
- apps/desktop/src/main/services/sessions/settleTeardownWiring.ts
- apps/desktop/src/main/services/sessions/sessionSettleTeardown.ts
- apps/desktop/src/main/services/sessions/sessionService.ts
1 participant
Step 3 of
docs/features/terminals-and-sessions/settle-teardown-design.md: settle now actually stops the work it files as done. Steps 0-2 shipped in #1069, #1073, #1075.What it does
Teardown is real, and awaited inside the settling window. Step 2 shipped a branded synchronous seam that made an awaited teardown a compile error. That was a tripwire, not an obstacle — it existed because bolting a deferred teardown onto a synchronous write produced a P1 in each of #1059's six rounds. What changed is the machinery under it: the settling window is exclusive (R4), abortable (R1/R6) 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.
sessionSettleTeardown.tsreusesstopLaneRuntimeWork's shape — ordered steps, each in its own try/catch — not its body, which disposes sessions outright because it serves lane deletion. Terminals are never touched. The abort is checked before every 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, job count, a local-only table, cleared 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. CRR owns the values; the chokepoint owns the revision, and the reconcile also trips the abort.
Two blockers the review caught
The brain never wired any of it.
bootstrap.tsbuilt itssessionServicewith no teardown seam and never registered the apply hook — and in a normal install the brain, not the desktop, applies changesets and serves phone sync, remote commands and the PR poller. Teardown was a no-op for nearly every settle a user can actually trigger. Both processes now build their hooks from one factory so they cannot drift.Holding settle-tuple rows out of
crsql_changesbreaks LWW convergence — proven against the vendored cr-sqlite. A column that never enterscrsql_changesnever raises the localcol_version, so the host stays behind its peer permanently and its next genuine decision loses every merge. Two hosts disagree forever: strictly worse than the bypass being fixed. Reversed.A later round then proved my replacement dedup guard (
result.changes > 0) inert —crsql_changesis a virtual table, so SQLite counts the call whether or not cr-sqlite discarded the row. Replaced with a value comparison; the test applies the same changeset twice and fails against the old guard.Review
Four rounds, two tracks each. Beyond the above:
getSettleResiduewas unreachable (in the CTO-only list but not the allowlist), a peer write bumped the revision without tripping the abort,stop_and_clearwas destroying queued user turns, both new analytics properties were silently dropped by the sanitizer, unbounded provider awaits could wedge a row as permanently unsettleable, and bulk settle was serial against iOS's 30s budget. Each fix is pinned by a test that was probed and shown to fail against the pre-fix code.Three claims were verified empirically rather than trusted: the
observeRemoteself-assignment doesn't touch the clock,stop_onlydoes stop background work, and the cross-call duplicate is suppressed.Deferred, by coordinator ruling
session.settleSessionskeeps its bare changed-id array. The typed-outcome wire shape ships later, capability-gated with a mobile release; both options are written up in the design doc (§6c-i).Verification
Desktop and ade-cli typecheck clean · 1009 tests green across sessions/state/prs/registry/analytics/CTO · full desktop shards and the ade-cli suite match
main's pre-existing failures exactly ·test:posthog8/8.Summary by CodeRabbit
New Features
Bug Fixes