Settling state, abort rule, and the race matrix (settle teardown, step 2) #1075
+1,260
−128
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import fs from "node:fs"; | ||
| import type { AdeDb } from "../state/kvDb"; | ||
| import { createSettleLifecycleWriter } from "./settleLifecycleWriter"; | ||
| import type { SettleAbortedSession, SettleSessionsOutcome, SettleTeardownCompleted } from "./settlingStateRegistry"; | ||
| import type { | ||
| ClaudeSessionPointer, | ||
| SessionAttentionSource, | ||
|
|
@@ -365,7 +366,21 @@ function normalizeSessionIds(sessionIds: string[]): string[] { | |
| )); | ||
| } | ||
|
|
||
| export function createSessionService({ | ||
| db, | ||
| runSettleTeardown, | ||
| }: { | ||
| db: AdeDb; | ||
| /** | ||
| * Stop the session's background work. Injected rather than per-call: teardown | ||
| * is a service capability, not something a caller decides. | ||
| * | ||
| * Step 2 ships with this absent — the settling window, the abort rule, and the | ||
| * revision guard all land and are tested against a NO-OP, so every race is | ||
| * exercised before there is any work to lose. Step 3 supplies the real one. | ||
| */ | ||
| runSettleTeardown?: (sessionId: string) => SettleTeardownCompleted; | ||
| }) { | ||
| const changeListeners = new Set<(event: TerminalSessionChangedEvent) => void>(); | ||
|
|
||
|
|
||
|
|
@@ -718,6 +733,68 @@ export function createSessionService({ db }: { db: AdeDb }) { | |
| return newlySettled; | ||
| }; | ||
|
|
||
| /** | ||
| * Settle through the settling window: the shape a real teardown will run in. | ||
| * | ||
| * Per session: read the revision, open the window, run teardown, then apply | ||
| * the settle ONLY if nothing moved. "Nothing moved" is two checks that catch | ||
| * different things — the abort flag (a human decision arrived and said so) and | ||
| * the revision (anything else changed the settle tuple, including a change | ||
| * this host did not make through a caller). | ||
| * | ||
| * Teardown is a NO-OP in step 2 by design. The point of landing the window | ||
| * first is that every race is testable before there is any work to lose. | ||
| */ | ||
| const settleManyWithTeardown = ( | ||
| sessionIds: string[], | ||
| options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, | ||
| ): SettleSessionsOutcome => { | ||
| const ids = normalizeSessionIds(sessionIds); | ||
| const settled: string[] = []; | ||
| const aborted: SettleAbortedSession[] = []; | ||
|
|
||
| for (const id of ids) { | ||
| const revisionBefore = settleLifecycle.readRevision(id); | ||
| const begin = settleLifecycle.settling.begin(id, revisionBefore); | ||
| // Joined an in-flight settle rather than starting a second teardown: R4. | ||
| // The owner will report the outcome; reporting it twice would double-count. | ||
| if (begin.kind === "joined") { | ||
| // Report it. A joiner that returns nothing looks identical to a settle | ||
| // that was never eligible, and a caller with a durable consequence — the | ||
| // PR poller marking a merge handled — would consume the merge on the | ||
| // strength of someone else's in-flight settle that may yet abort. | ||
| aborted.push({ sessionId: id, reason: "joined_in_flight" }); | ||
| continue; | ||
| } | ||
| try { | ||
| runSettleTeardown?.(id); | ||
|
|
||
| const abortedBy = settleLifecycle.settling.abortedBy(id); | ||
| if (abortedBy) { | ||
| aborted.push({ sessionId: id, reason: abortedBy }); | ||
| continue; | ||
| } | ||
| // The revision catches everything the abort flag cannot: a settle-tuple | ||
| // change from a path that never announced itself as a decision. | ||
| if (settleLifecycle.readRevision(id) !== revisionBefore) { | ||
| aborted.push({ sessionId: id, reason: "lifecycle_changed" }); | ||
| continue; | ||
| } | ||
| settled.push(...settleMany([id], options)); | ||
|
coderabbitai[bot] marked this conversation as resolved.
There was a problem hiding this comment.
When a sibling ADE process or paired desktop applies an unsettle or keep-active override after the local revision check, that replicated tuple write bypasses this host's lifecycle revision, and this subsequent Useful? React with 👍 / 👎. |
||
| } catch (error) { | ||
|
There was a problem hiding this comment.
When Useful? React with 👍 / 👎. |
||
| // A throw must not discard the accounting for the rest of the batch, nor | ||
| // leave earlier sessions settled with no record of it. Report this id and | ||
| // carry on: a bulk caller always gets a full accounting. | ||
| aborted.push({ sessionId: id, reason: "teardown_failed" }); | ||
| void error; | ||
| } finally { | ||
| settleLifecycle.settling.end(id); | ||
| } | ||
| } | ||
|
|
||
| return { settled, aborted }; | ||
| }; | ||
|
|
||
| return { | ||
| list, | ||
|
|
||
|
|
@@ -1307,7 +1384,7 @@ export function createSessionService({ db }: { db: AdeDb }) { | |
| return; | ||
| } | ||
| writeSettleLifecycle({ | ||
| intent: { kind: "clearOnActivity", cause: "mechanical" }, | ||
| extraSet: { last_output_preview: preview, last_output_at: now }, | ||
| sessionIds: [sessionId], | ||
| }); | ||
|
|
@@ -1333,7 +1410,7 @@ export function createSessionService({ db }: { db: AdeDb }) { | |
| return; | ||
| } | ||
| writeSettleLifecycle({ | ||
| intent: { kind: "clearOnActivity", cause: "mechanical" }, | ||
| extraSet: { last_output_at: at }, | ||
| sessionIds: [sessionId], | ||
| }); | ||
|
|
@@ -1432,22 +1509,26 @@ export function createSessionService({ db }: { db: AdeDb }) { | |
| sessionId: string, | ||
| opts: { outcome?: string | null; settledAt?: string; source?: SessionSettleSource } = {}, | ||
| ): boolean { | ||
| // Through the settling window, like the bulk paths. This is the route a | ||
| // USER takes (row menu -> settleTerminalSession -> here), which is the | ||
| // "user settle" R4 names — so it has to be joinable and abortable, and in | ||
| // step 3 it has to run teardown. Routing it here is what makes the R4 | ||
| // claim true rather than only true of bulk callers. | ||
| const trimmed = sessionId.trim(); | ||
| if (!trimmed) return false; | ||
| // `settleMany` returns [] for both "missing" and "already settled", so the | ||
| // boolean contract needs its own existence check to stay honest. | ||
| const exists = db.get<{ present: number }>( | ||
| "select 1 as present from terminal_sessions where id = ? limit 1", | ||
| [trimmed], | ||
| ); | ||
| if (!exists) return false; | ||
| settleManyWithTeardown([trimmed], { | ||
| outcome: normalizeSessionStatusNote(opts.outcome) ?? undefined, | ||
| settledAt: opts.settledAt, | ||
| source: opts.source, | ||
|
There was a problem hiding this comment.
For Useful? React with 👍 / 👎. |
||
| }); | ||
| return true; | ||
|
There was a problem hiding this comment.
When the teardown callback triggers AGENTS.md reference: AGENTS.md:L35-L35 Useful? React with 👍 / 👎.
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| }, | ||
|
|
||
| /** Clears a declared settle plus any `'settled'` override. */ | ||
|
|
@@ -1511,16 +1592,28 @@ export function createSessionService({ db }: { db: AdeDb }) { | |
| }, | ||
|
|
||
| settleSessions(sessionIds: string[]): string[] { | ||
| return settleManyWithTeardown(sessionIds).settled; | ||
| }, | ||
|
|
||
| /** | ||
| * Settle, reporting abandoned sessions explicitly. | ||
| * | ||
| * `settleSessions` leaves an aborted id simply absent from its changed-id | ||
| * list, which is *almost* the right contract — a caller cannot tell "filed" | ||
| * from "not filed, and here is why". This is that distinction, and it is | ||
| * what a caller with a durable consequence (the PR-merge auto-settle marking | ||
| * a PR handled) has to branch on. | ||
| */ | ||
| settleSessionsReportingAborts( | ||
| sessionIds: string[], | ||
| options: { outcome?: string; settledAt?: string; source?: SessionSettleSource } = {}, | ||
| ): SettleSessionsOutcome { | ||
| return settleManyWithTeardown(sessionIds, options); | ||
| }, | ||
|
|
||
| /** Sessions currently mid-settle, for the visible `Settling…` state. */ | ||
| settlingSessionIds(): string[] { | ||
| return settleLifecycle.settling.settlingSessionIds(); | ||
|
Comment on lines
+1659
to
+1661
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Find IPC, preload, shared-type, and renderer references to the settling projection.
rg -nP -C3 '\bsettlingSessionIds\b|\bisSettling\b|Settling…' apps/desktop/srcRepository: arul28/ADE Length of output: 6057 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- candidate desktop files ---'
git ls-files apps/desktop/src | rg '(^|/)(ipc|preload|renderer|session|sessions|.*Service|.*service)' | head -200
printf '%s\n' '--- session service structure ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline apps/desktop/src/main/services/sessions/sessionService.ts
else
rg -n 'export|settlingSessionIds|sessionService|ipcMain|handle|sessions' apps/desktop/src/main/services/sessions/sessionService.ts
fi
printf '%s\n' '--- session service references outside tests ---'
rg -n -P -C3 '\bsessionService\b|\bsettleSessionsReportingAborts\b|\bsettlingSessionIds\b' apps/desktop/src --glob '!**/*.test.ts' --glob '!**/*.spec.ts'
printf '%s\n' '--- IPC/preload session references ---'
rg -n -P -C3 'session|settle|Settling|isSettling' apps/desktop/src/main apps/desktop/src/preload apps/desktop/src/renderer --glob '*.{ts,tsx}' | head -400Repository: arul28/ADE Length of output: 50367 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- exact settling references in all tracked desktop source ---'
rg -n -P -C2 '\bsettlingSessionIds\b|\bisSettling\b|Settling…' apps/desktop/src --glob '*.{ts,tsx}' || true
printf '%s\n' '--- desktop source top-level directories ---'
find apps/desktop/src -mindepth 1 -maxdepth 2 -type d | sort | head -100
printf '%s\n' '--- IPC registration and preload bridge files ---'
git ls-files apps/desktop/src | rg '(^|/)(ipc|preload|bridge|api|context|channels?)(/|\.|$)' | head -200
printf '%s\n' '--- renderer access to session APIs and status fields ---'
rg -n -P -C2 'window\.(api|electron|desktop)|sessions?\.(list|get|subscribe|on)|terminal\.(list|get|subscribe)|settledAt|settleOverride' apps/desktop/src/renderer apps/desktop/src/preload apps/desktop/src --glob '*.{ts,tsx}' | head -300Repository: arul28/ADE Length of output: 33536 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- session IPC registration ---'
rg -n -P -C8 'session\.list|sessions\.list|sessionService\.list|TerminalSessionSummary|settledAt|settleOverride' apps/desktop/src/main/services/ipc/registerIpc.ts apps/desktop/src/shared apps/desktop/src/preload/preload.ts apps/desktop/src/preload/global.d.ts --glob '*.{ts,tsx}' | head -500
printf '%s\n' '--- session bridge definitions ---'
rg -n -P -C6 'sessions\s*[:=]|list\s*:\s*.*session|onChanged|getDelta|readTranscriptTail' apps/desktop/src/preload/preload.ts apps/desktop/src/preload/global.d.ts apps/desktop/src/shared/ipc.ts
printf '%s\n' '--- session list implementation and summary construction ---'
rg -n -P -C8 'list\(|enrichSessions|TerminalSessionSummary|settledAt|settleOverride' apps/desktop/src/main/services/sessions apps/desktop/src/main/services/pty apps/desktop/src/shared/types --glob '*.{ts,tsx}' | head -500Repository: arul28/ADE Length of output: 50367 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- exact summary type ---'
rg -n -C35 '^export (type|interface) TerminalSessionSummary|^export type TerminalSessionDetail' apps/desktop/src/shared/types/sessions.ts
printf '%s\n' '--- list projection implementation ---'
rg -n -P -C25 'listSessionsWithChatProjection|function listSessionsWithChatProjection|const listSessionsWithChatProjection' apps/desktop/src/main/services/ipc/registerIpc.ts
printf '%s\n' '--- all settling identifiers outside main session internals ---'
rg -n -P '\bsettlingSessionIds\b|\bisSettling\b|Settling…' apps/desktop/src/preload apps/desktop/src/shared apps/desktop/src/renderer || trueRepository: arul28/ADE Length of output: 11137 Expose the
🤖 Prompt for AI AgentsSource: Path instructions |
||
| }, | ||
|
|
||
| unsettleSessions(sessionIds: string[]): void { | ||
|
|
@@ -1677,7 +1770,7 @@ export function createSessionService({ db }: { db: AdeDb }) { | |
| ): boolean { | ||
| return mutateSessionMeta(sessionId, (id) => { | ||
| writeSettleLifecycle({ | ||
| intent: { kind: "clearOnActivity", cause: "attention_requested" }, | ||
| extraSet: { | ||
| attention_requested_at: new Date().toISOString(), | ||
| attention_message: normalizeOptionalText(message, 500), | ||
|
|
@@ -1706,7 +1799,7 @@ export function createSessionService({ db }: { db: AdeDb }) { | |
| // settled/failed mutually exclusive at write time, so every surface's | ||
| // precedence order agrees by construction. | ||
| writeSettleLifecycle({ | ||
| intent: { kind: "clearOnActivity", cause: "turn_failed" }, | ||
| extraSet: { last_turn_failed_at: failedAt }, | ||
| sessionIds: [id], | ||
| }); | ||
|
|
@@ -1731,7 +1824,7 @@ export function createSessionService({ db }: { db: AdeDb }) { | |
| clearTurnStartMarkers(sessionId: string): boolean { | ||
| const changed = mutateSessionMeta(sessionId, (id) => { | ||
| writeSettleLifecycle({ | ||
| intent: { kind: "clearOnActivity", cause: "turn_start" }, | ||
| extraSet: { | ||
| last_turn_failed_at: null, | ||
| attention_requested_at: null, | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For chat sessions, this gate never observes turn completion in production:
sessionService.get()returns the raw persisted row and derivesruntimeStatedirectly fromstatus, while chat rows intentionally keepstatus = "running"between turns; onlychatSessionProjection.tschanges an idle chat toruntimeState = "idle". Thus, after a chat turn aborts auto-settlement, every later poll keeps the merged PR unfinished indefinitely. This is fresh evidence against the prior retry finding: the new test injects anidleruntime state that the directly wired productionsessionService.get()cannot return for a persisted running chat.Useful? React with 👍 / 👎.