mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 00:02:56 +00:00
fix(worktrees): report the closes that landed when the sweep budget expires
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.
This commit is contained in:
@@ -54,6 +54,8 @@ function installHost(options: {
|
||||
settledThenThrows?: Set<string>
|
||||
/** Blocks every close, to exercise the shared sweep budget without fake timers. */
|
||||
closeGate?: Promise<void>
|
||||
/** Blocks ONE session's close, so the serial loop can be caught part-way through. */
|
||||
closeGates?: Record<string, Promise<void>>
|
||||
}): { 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<void>(() => {}) }
|
||||
})
|
||||
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<void>((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<void>((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
|
||||
|
||||
@@ -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<void> {
|
||||
// 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 }
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user