Settling state, abort rule, and the race matrix (settle teardown, step 2) #1075
There are no files selected for viewing
| 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,61 @@ 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") continue; | ||
|
There was a problem hiding this comment.
When PR auto-settlement joins a user or operator settle already in flight, this returns Useful? React with 👍 / 👎. |
||
| 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 +1377,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 +1403,7 @@ export function createSessionService({ db }: { db: AdeDb }) { | |
| return; | ||
| } | ||
| writeSettleLifecycle({ | ||
| intent: { kind: "clearOnActivity", cause: "mechanical" }, | ||
| extraSet: { last_output_at: at }, | ||
| sessionIds: [sessionId], | ||
| }); | ||
|
|
@@ -1432,22 +1502,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 +1585,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 +1763,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 +1792,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 +1817,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, | ||
|
|
||
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.
Fresh evidence in this replacement gate is that the cooldown expires solely by elapsed time, independent of whether the turn that caused the abort has finished. For a chat or tracked-agent turn lasting over ten minutes, completion has not re-fired
turn_start, and its output deliberately does not advance the lifecycle revision, so the next retry runs teardown against that same active work and then settles it. Re-arm on an inactive/completed-turn signal rather than unconditional timeout expiry.Useful? React with 👍 / 👎.