From 3028b85df3d543006ec64fbe9d8c8ea692a814fc Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Wed, 9 Sep 2026 14:04:07 -0700 Subject: [PATCH] fix(worktrees): report the closes that landed when the sweep budget expires MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The structured close loop is serial, so the shared sweep budget can expire part-way through it. The timeout fallback was assembled by the caller and could only name the whole list: sessions this removal had already closed were reported as unclosed, named in the refusal the user reads, and logged as `structured=0`. The loop now records progress into a structure the timeout path reads, so both the refusal and the count say only what was observed. A session with no recorded outcome reports `unverifiable` — the same verdict as an attempted close that stayed unproven, because "never asked" and "asked, unconfirmed" are both exactly "not observed exited", and neither may claim `live`. The loop also checks the deadline before each close, so one slow provider round trip no longer starves every session behind it. It stops ISSUING closes; an in-flight one is left to finish, since nothing here can cancel a round trip. The structured host fence now reuses the PTY fence's own type instead of a look-alike that read `undefined` as local while the other read it as match-all, with both claiming the same precedence. `null` means this machine on both sides; ABSENT stays narrowed to local here, documented and pinned, because a single-host-id comparison cannot express match-all. Also pins a tradeoff that was accepted rather than wanted: the PTY sweeps run concurrently with the structured close, so a removal that refuses over a stuck session has already killed that workspace's terminals. --- ...ructured-session-worktree-teardown.test.ts | 105 ++++++++++++++ .../structured-session-worktree-teardown.ts | 129 +++++++++++++----- src/main/runtime/worktree-pty-host-fence.ts | 6 + src/main/runtime/worktree-teardown.ts | 20 ++- 4 files changed, 220 insertions(+), 40 deletions(-) diff --git a/src/main/runtime/structured-session-worktree-teardown.test.ts b/src/main/runtime/structured-session-worktree-teardown.test.ts index 595a32e932f..5cdae7dcd4f 100644 --- a/src/main/runtime/structured-session-worktree-teardown.test.ts +++ b/src/main/runtime/structured-session-worktree-teardown.test.ts @@ -54,6 +54,8 @@ function installHost(options: { settledThenThrows?: Set /** Blocks every close, to exercise the shared sweep budget without fake timers. */ closeGate?: Promise + /** Blocks ONE session's close, so the serial loop can be caught part-way through. */ + closeGates?: Record> }): { closed: string[] } { const held = new Set(options.records.map((entry) => entry.sessionId)) const closed: string[] = [] @@ -64,6 +66,7 @@ function installHost(options: { close: async (sessionId: string) => { closed.push(sessionId) await options.closeGate + await options.closeGates?.[sessionId] if (options.stuck?.has(sessionId)) { return } @@ -263,6 +266,22 @@ describe('worktree teardown and structured agent sessions', () => { expect(host.closed).toEqual([]) }) + it('reads an explicit local fence the way the PTY sweeps do', () => { + // This helper reuses the PTY fence's own type, so the two cannot answer `null` differently: + // there it means this machine, and it has to mean this machine here. ABSENT is the one + // deliberate difference — no fence at all for the PTY sweeps, narrowed to local here, because + // a single-host-id comparison cannot express match-all and closing every host's chats is + // destructive. Latent today only because `WorktreeTeardownDeps` cannot yet carry the `null`. + installHost({ + records: [record('s1', WORKTREE, { executionHostId: 'ssh:host-a' }), record('s2', WORKTREE)] + }) + const local = [{ sessionId: 's2', agent: 'claude' }] + expect(listLiveStructuredSessionsForWorktree(WORKTREE, { resolvedConnectionId: null })).toEqual( + local + ) + expect(listLiveStructuredSessionsForWorktree(WORKTREE, {})).toEqual(local) + }) + it('closes only the session on the host the removal resolved to', async () => { const host = installHost({ records: [record('s1', WORKTREE, { executionHostId: 'ssh:host-a' }), record('s2', WORKTREE)] @@ -380,6 +399,92 @@ describe('worktree teardown and structured agent sessions', () => { warn.mockRestore() }) + it('names only the sessions still open when the budget expires mid-close', async () => { + // The close loop is serial, so a deadline can land part-way through it. A fallback assembled + // at the deadline could only name the whole list — so a removal that had already closed the + // first chat still told the user both were still there, which is the exact thing this sweep + // exists to stop doing: never report state nobody observed. + installHost({ + records: [record('s1', WORKTREE), record('s2', WORKTREE, { provider: 'codex' })], + closeGates: { s2: new Promise(() => {}) } + }) + const error = await killAllProcessesForWorktree( + WORKTREE, + destructiveDeps({ timeoutMs: 40 }) + ).catch((thrown: Error) => thrown.message) + expect(error).toContain('could not confirm these closed: 1 agent session (codex)') + expect(error).not.toContain('claude') + }) + + it('counts the closes that landed before the budget expired', async () => { + // The other half of the same fallback: it reported zero closes, so the removal log said + // `structured=0` for a chat it had just ended. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const slowClose = new Promise((resolve) => { + setTimeout(resolve, 300) + }) + installHost({ + records: [record('s1', WORKTREE), record('s2', WORKTREE, { provider: 'codex' })], + closeGates: { s2: slowClose } + }) + const result = await killAllProcessesForWorktree( + WORKTREE, + destructiveDeps({ allowUnverifiedStop: true, timeoutMs: 40 }) + ) + expect(result.structuredStopped).toBe(1) + expect(structuredSessionWarning(warn)).toContain( + 'could not confirm these closed: 1 agent session (codex)' + ) + warn.mockRestore() + }) + + it('stops issuing new closes once the budget is spent', async () => { + // One slow provider round trip used to starve every session behind it: the outer race had + // already given up on the loop, and it went on issuing closes whose outcome nobody would read. + // The in-flight one is NOT cancelled — nothing here can cancel a provider round trip — so it + // still has to be reported, which is why both sessions are named below. + let releaseFirstClose: () => void = () => {} + const firstClose = new Promise((resolve) => { + releaseFirstClose = resolve + }) + const host = installHost({ + records: [record('s1', WORKTREE), record('s2', WORKTREE)], + closeGates: { s1: firstClose } + }) + const error = await killAllProcessesForWorktree( + WORKTREE, + destructiveDeps({ timeoutMs: 5 }) + ).catch((thrown: Error) => thrown.message) + expect(error).toContain('could not confirm these closed: 2 agent sessions (claude)') + releaseFirstClose() + await new Promise((resolve) => { + setTimeout(resolve, 25) + }) + expect(host.closed).toEqual(['s1']) + }) + + it('leaves the terminals already stopped when it refuses over a stuck session', async () => { + // Pins a tradeoff that was accepted, not an outcome that is wanted. The PTY sweeps now run + // concurrently with the structured close, so a removal that refuses over a session that will + // not close has ALREADY killed that workspace's terminals — the head-first serial order spared + // them. Serialising it back is worse: it spends the whole shared budget before a single PTY is + // asked, and the alternative — refusing before the PTY sweeps — leaves force-delete removing + // files while PTY handles are open. The PTY gate itself already kills first and refuses only + // on what it could not verify stopped. A later change must not flip this back silently. + let terminalSweeps = 0 + const runtime = { + stopTerminalsForWorktree: async () => { + terminalSweeps += 1 + return { stopped: 2 } + } + } as never + installHost({ records: [record('s1', WORKTREE)], stuck: new Set(['s1']) }) + await expect( + killAllProcessesForWorktree(WORKTREE, { ...destructiveDeps(), runtime }) + ).rejects.toThrow(/still live: 1 agent session \(claude\)/) + expect(terminalSweeps).toBe(1) + }) + it('starts the terminal sweeps while the structured close is still in flight', async () => { // The close is serial and each one waits on a provider round trip. Awaiting it before the // sweeps exist spends the shared budget head-first, and the sweeps then report a timeout for diff --git a/src/main/runtime/structured-session-worktree-teardown.ts b/src/main/runtime/structured-session-worktree-teardown.ts index 933e70f3324..609ff3ef23f 100644 --- a/src/main/runtime/structured-session-worktree-teardown.ts +++ b/src/main/runtime/structured-session-worktree-teardown.ts @@ -30,6 +30,7 @@ import { getStructuredAgentSessionHost } from '../native-chat/agent-session-wire import { observeStructuredWorker } from './structured-worker-authority' import { closeStructuredAgentSessionChild } from './structured-agent-session-close' import { retireSettledStructuredWorkerTab } from './structured-agent-session-tab-retirement' +import type { WorktreePtyHostFence } from './worktree-pty-host-fence' import type { OrcaRuntimeService } from './orca-runtime' export type LiveStructuredSessionInWorkspace = { @@ -47,11 +48,19 @@ export type StructuredWorktreeSweepRuntime = Pick< 'forgetStructuredSessionMail' | 'retireStructuredAgentSessionTabFromSnapshot' > -/** The two fields every teardown caller already resolves to fence its PTY sweeps to one host. */ -export type StructuredSessionHostFence = { - resolvedConnectionId?: string - resolvedRuntimeEnvironmentId?: string -} +/** + * The two fields every teardown caller already resolves to fence its PTY sweeps to one host. + * + * Deliberately the PTY fence's own type rather than a look-alike: these two helpers are written + * against each other, so a widening on one side must not become a silent disagreement on the + * other. `resolvedConnectionId: null` means this machine on both. + * + * They differ in exactly one reading, and only that one: ABSENT. The PTY fence takes it as no + * fence at all and matches every host, which a single-host-id comparison cannot express — and + * closing every host's chats is destructive, not merely noisy. So this side reads absent as local + * too, the narrower half of that pair. Pinned by test, not left to the next reader to rediscover. + */ +export type StructuredSessionHostFence = WorktreePtyHostFence /** * The one execution host this teardown may touch. @@ -59,9 +68,7 @@ export type StructuredSessionHostFence = { * A workspace id is `repoId::path` with no host component, so the local machine, an SSH host and a * paired runtime can all publish the SAME id and each names a DIFFERENT workspace (STA-4343). The * PTY sweeps fence on exactly these two fields; a structured session records its host directly, so - * the comparison is on `location.executionHostId` instead of on a pty-id shape — but the - * precedence is the same. Neither field set means the removal targets this machine, which is also - * the safe default: a caller that resolved no host closes nothing on anyone else's. + * the comparison is on `location.executionHostId` instead of on a pty-id shape. */ export function structuredSessionTeardownHostId( fence: StructuredSessionHostFence @@ -69,9 +76,10 @@ export function structuredSessionTeardownHostId( if (fence.resolvedRuntimeEnvironmentId !== undefined) { return toRuntimeExecutionHostId(fence.resolvedRuntimeEnvironmentId) } - return fence.resolvedConnectionId === undefined - ? LOCAL_EXECUTION_HOST_ID - : toSshExecutionHostId(fence.resolvedConnectionId) + // Both no-connection readings collapse here on purpose — see the fence type. A caller that + // resolved no host, and one that resolved this machine, each close nothing on anyone else's. + const connectionId = fence.resolvedConnectionId ?? null + return connectionId === null ? LOCAL_EXECUTION_HOST_ID : toSshExecutionHostId(connectionId) } /** @@ -148,7 +156,54 @@ export function describeUnclosedStructuredSessions( } /** - * Closes the given structured sessions, and reports what stayed. + * What the close loop has done so far, readable while it is still running. + * + * The loop is serial and every close waits on a provider round trip, so the shared sweep budget can + * expire part-way through it. This is written as it goes rather than returned at the end, because + * the caller's timeout path reads THIS: a fabricated whole-list fallback reported sessions the + * sweep had already closed as unclosed, named them in the refusal the user reads, and logged + * `structured=0` for closes that landed. Saying only what was observed is the point of the sweep. + */ +export type StructuredSweepProgress = { + /** The sessions this sweep closes, in the order the loop reaches them. */ + readonly sessions: readonly LiveStructuredSessionInWorkspace[] + /** Sessions no longer attached after their close — the count this sweep reports. */ + closed: number + /** Attempted closes that did not settle, each carrying the verdict re-read after the attempt. */ + unstopped: UnclosedStructuredSession[] + /** How many of `sessions`, from the front, have an outcome recorded. */ + settled: number +} + +export function createStructuredSweepProgress( + sessions: readonly LiveStructuredSessionInWorkspace[] +): StructuredSweepProgress { + return { sessions, closed: 0, unstopped: [], settled: 0 } +} + +/** + * Everything this sweep did not prove closed. + * + * A session with no recorded outcome — never started, or still in flight — reports `unverifiable`, + * the same verdict as an attempted close that stayed unproven. Chosen, not conflated: the vocabulary is `live` / `unverifiable` / `exited` with no + * synonyms, and "we never asked" and "we asked and could not confirm" are both exactly "not + * observed exited". A fourth bucket would need its own refusal wording and its own toast + * classification for a distinction the user cannot act on any differently — and `live` is the only + * verdict either could be mistaken for, which is the one thing neither is allowed to claim. + */ +export function unclosedStructuredSessions( + progress: StructuredSweepProgress +): UnclosedStructuredSession[] { + return [ + ...progress.unstopped, + ...progress.sessions + .slice(progress.settled) + .map((session) => ({ ...session, status: 'unverifiable' as const })) + ] +} + +/** + * Closes the structured sessions in `progress`, recording what stayed as it goes. * * Runs on the ordinary removal too, not just force: a child left running against a deleted `cwd` is * the outcome this whole sweep exists to prevent, and closing is how you prevent it. What stayed is @@ -159,38 +214,46 @@ export function describeUnclosedStructuredSessions( * let the refusal name a session this call never touched. */ export async function closeStructuredSessionsForWorktree( - sessions: readonly LiveStructuredSessionInWorkspace[], + progress: StructuredSweepProgress, + deadline: number, runtime?: StructuredWorktreeSweepRuntime -): Promise<{ closed: number; unstopped: UnclosedStructuredSession[] }> { +): Promise { // No `afterClose` for a dispatched worker: `host.close` drops the holds, so nothing keeps a // provider child un-evictable, but the dispatch's redrive subscription and registry entry do // survive until it settles by another verb. That is a bounded leak, not a hazard — and passing // one here would mean resolving a dispatch id per session on a teardown path that must stay // inside the sweep deadline. - const unstopped: UnclosedStructuredSession[] = [] - let closed = 0 - for (const session of sessions) { + for (const session of progress.sessions) { + // Stops ISSUING new closes once the budget is spent; an in-flight one is left to finish, since + // nothing here can cancel a provider round trip. Without this, one slow round trip starved + // every session behind it: the caller's race had already given up, and the loop went on + // closing sessions whose outcome nobody would read. + if (Date.now() >= deadline) { + return + } const outcome = await closeStructuredAgentSessionChild( session.sessionId, runtime ? { runtime } : {} ) if (outcome.stopped) { - closed += 1 - continue + progress.closed += 1 + } else { + // Re-observed rather than reusing the close's own reason string: what the user is asked to + // waive is the state AFTER the attempt, and a close that threw never reached an observation. + const status = observeStructuredWorker({ sessionId: session.sessionId }).status + if (status === 'exited') { + // The re-read can PROVE the exit a failed close could not — it threw past its own + // observation, or the record's death evidence landed after it read. Refusing on a child + // that is demonstrably gone is the defect this sweep exists to remove, so take the proof + // and run the retirement `closeStructuredAgentSessionChild` skipped when it gave up. + retireSettledStructuredWorkerTab(session.sessionId, runtime) + progress.closed += 1 + } else { + progress.unstopped.push({ ...session, status }) + } } - // Re-observed rather than reusing the close's own reason string: what the user is asked to - // waive is the state AFTER the attempt, and a close that threw never reached an observation. - const status = observeStructuredWorker({ sessionId: session.sessionId }).status - if (status === 'exited') { - // The re-read can PROVE the exit a failed close could not — it threw past its own - // observation, or the record's death evidence landed after it read. Refusing on a child - // that is demonstrably gone is the defect this sweep exists to remove, so take the proof - // and run the retirement `closeStructuredAgentSessionChild` skipped when it gave up. - retireSettledStructuredWorkerTab(session.sessionId, runtime) - closed += 1 - continue - } - unstopped.push({ ...session, status }) + // Advanced only once an outcome is recorded, so a close still in flight when the deadline + // lands stays reported as unclosed instead of falling out of both counts. + progress.settled += 1 } - return { closed, unstopped } } diff --git a/src/main/runtime/worktree-pty-host-fence.ts b/src/main/runtime/worktree-pty-host-fence.ts index 855371c4aef..a2a6099bf11 100644 --- a/src/main/runtime/worktree-pty-host-fence.ts +++ b/src/main/runtime/worktree-pty-host-fence.ts @@ -1,8 +1,14 @@ export type WorktreePtyHostFence = { + /** `null` is this machine; ABSENT is no fence at all, so every host matches. */ resolvedConnectionId?: string | null resolvedRuntimeEnvironmentId?: string } +/** + * Also fences the structured sweep, through `structuredSessionTeardownHostId`, which reuses this + * exact type so the two cannot drift. That helper narrows ABSENT to local — the one deliberate + * difference, documented where it is made. + */ export function worktreePtyBelongsToHost( ptyId: string, connectionId: string | null | undefined, diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 31a62c5543d..1d16baf2fdd 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -15,8 +15,10 @@ import { } from './worktree-pty-surface-sweeps' import { closeStructuredSessionsForWorktree, + createStructuredSweepProgress, describeUnclosedStructuredSessions, - listLiveStructuredSessionsForWorktree + listLiveStructuredSessionsForWorktree, + unclosedStructuredSessions } from './structured-session-worktree-teardown' import { createWorktreeSweepTracker, @@ -331,14 +333,18 @@ async function sweepStructuredSessions( // that is meant to clear it (#11960). A close that ran out of time is a session this removal // could not confirm closed, which is exactly what the branch below already words. Tracked so a // forced removal still waits out the abandoned-sweep grace before it deletes files. - const { closed, unstopped } = await settleBeforeDeadline( - sweeps.track(() => closeStructuredSessionsForWorktree(live, deps.runtime)), - { - closed: 0, - unstopped: live.map((session) => ({ ...session, status: 'unverifiable' as const })) - }, + // + // The verdict is read off `progress`, which the serial loop fills as it goes, rather than off + // this call's result: the deadline can land mid-loop, and a fallback assembled here could only + // guess — it named every session, including the ones already closed, and reported zero closes. + const progress = createStructuredSweepProgress(live) + await settleBeforeDeadline( + sweeps.track(() => closeStructuredSessionsForWorktree(progress, deadline, deps.runtime)), + undefined, deadline ) + const closed = progress.closed + const unstopped = unclosedStructuredSessions(progress) if (unstopped.length === 0) { return closed }