mirror of
https://github.com/stablyai/orca.git
synced 2026-09-28 16:02:45 +00:00
fix(worktrees): keep the chat-tab rollback out of removals that delete the workspace
The rollback added for a refused close ran on every unproven close, including the two shapes of removal that cannot refuse. Force Delete warns and deletes the checkout; a folder-workspace removal never refuses at all. Putting the tab back on those paths leaves a durable restore-index entry for a workspace that is then gone, and the chat republishes at the next launch pointing at it — the outcome this sweep exists to remove. The close now takes `restoreTabOnUnprovenClose`, on by default so `worker-stop` and `worker-release` keep the rollback, and the teardown sweep passes it only when the removal can still refuse. Second hole, same chain: `host.close` can return before the child's exit is recorded, so the close's own observation reads unverifiable and restores the tab, while the sweep's re-read one store write later proves the exit and counts the session closed. The two observations straddle that write and disagree. The sweep now re-drops the tab reference when it takes that proof, and the comment claiming the re-observation alone covers this is corrected.
This commit is contained in:
@@ -163,6 +163,21 @@ describe('closeStructuredAgentSessionChild tab-visibility rollback', () => {
|
||||
expect(host.setSessionTabVisibility.mock.calls).toEqual([[SESSION, false]])
|
||||
})
|
||||
|
||||
it('does not put the tab back when the caller is discarding the workspace anyway', async () => {
|
||||
// Worktree teardown passes this off for a removal that cannot refuse — force, and the
|
||||
// folder-workspace paths. A tab put back there is a durable reference to a workspace that is
|
||||
// about to be gone, so it republishes the chat at the next launch pointing at it.
|
||||
const host = installHost({ stuck: true })
|
||||
|
||||
const outcome = await closeStructuredAgentSessionChild(SESSION, {
|
||||
restoreTabOnUnprovenClose: false
|
||||
})
|
||||
|
||||
expect(outcome.stopped).toBe(false)
|
||||
expect(host.visible.has(SESSION)).toBe(false)
|
||||
expect(host.setSessionTabVisibility.mock.calls).toEqual([[SESSION, false]])
|
||||
})
|
||||
|
||||
it('does not publish a tab for a session that was already hidden', async () => {
|
||||
const host = installHost({ closeThrows: new Error('provider round trip failed'), visible: [] })
|
||||
|
||||
|
||||
@@ -36,6 +36,15 @@ export type StructuredAgentSessionCloseOptions = {
|
||||
* keep the child un-evictable for the life of the app. Every settlement has to reach it.
|
||||
*/
|
||||
afterClose?: () => void
|
||||
/**
|
||||
* Whether an unproven close may put the chat tab back in the durable restore index.
|
||||
*
|
||||
* On by default, which is the retryable case: a stop that refused and still took the user's tab
|
||||
* away is the loss the rollback exists to undo. A caller that will discard the WORKSPACE
|
||||
* whatever this close reports passes false — a tab put back there is a durable reference to a
|
||||
* workspace about to be gone, and it republishes the chat at the next launch pointing at it.
|
||||
*/
|
||||
restoreTabOnUnprovenClose?: boolean
|
||||
}
|
||||
|
||||
export async function closeStructuredAgentSessionChild(
|
||||
@@ -54,7 +63,8 @@ export async function closeStructuredAgentSessionChild(
|
||||
// Read BEFORE the hide, so a rollback puts the tab back exactly as it was. Restoring
|
||||
// unconditionally would publish a tab for a session that was already hidden — a worker started
|
||||
// without a chat tab, or one the user had closed — which is a new side effect, not an undo.
|
||||
const tabWasVisible = readPersistedTabVisibility(host, sessionId)
|
||||
const restoreTabIfCloseFails =
|
||||
options.restoreTabOnUnprovenClose !== false && readPersistedTabVisibility(host, sessionId)
|
||||
// Set only once the close is actually issued: `setSessionTabVisibility` throwing first leaves a
|
||||
// running child, and a receipt that still said `closed_agent_terminal` for it would be the
|
||||
// close-that-never-happened this flag exists to rule out.
|
||||
@@ -67,7 +77,7 @@ export async function closeStructuredAgentSessionChild(
|
||||
// Only `closeAttempted` proves the hide landed: the store transaction restores its own state on
|
||||
// failure, so a `setSessionTabVisibility` that threw hid nothing and has nothing to undo.
|
||||
if (closeAttempted) {
|
||||
await restorePersistedTabVisibility(host, sessionId, tabWasVisible)
|
||||
await restorePersistedTabVisibility(host, sessionId, restoreTabIfCloseFails)
|
||||
}
|
||||
return {
|
||||
stopped: false,
|
||||
@@ -78,7 +88,7 @@ export async function closeStructuredAgentSessionChild(
|
||||
options.afterClose?.()
|
||||
const observation = observeStructuredWorker({ sessionId })
|
||||
if (observation.status !== 'exited') {
|
||||
await restorePersistedTabVisibility(host, sessionId, tabWasVisible)
|
||||
await restorePersistedTabVisibility(host, sessionId, restoreTabIfCloseFails)
|
||||
return {
|
||||
stopped: false,
|
||||
closeAttempted: true,
|
||||
@@ -113,6 +123,11 @@ function readPersistedTabVisibility(host: StructuredAgentSessionHost, sessionId:
|
||||
* counting such a session closed and retiring its tab. Republishing there would resurrect a tab for
|
||||
* a session that is demonstrably gone, at the next launch, pointing at a deleted workspace.
|
||||
*
|
||||
* That observation NARROWS the window; it does not close it. This one and the sweep's are taken a
|
||||
* store write apart, so a child that dies in between is unverifiable here and exited there — which
|
||||
* is why the sweep re-drops the tab reference when it takes that proof. Do not delete either half
|
||||
* on the strength of the other.
|
||||
*
|
||||
* Never throws: the caller's `reason` is what the user is asked to act on, and a rollback failure
|
||||
* must not replace it. `agent_session_identity_required` is the expected one — the record can be
|
||||
* gone by now, which is itself the exit this restore is declining to undo.
|
||||
@@ -120,9 +135,9 @@ function readPersistedTabVisibility(host: StructuredAgentSessionHost, sessionId:
|
||||
async function restorePersistedTabVisibility(
|
||||
host: StructuredAgentSessionHost,
|
||||
sessionId: string,
|
||||
tabWasVisible: boolean
|
||||
restoreTab: boolean
|
||||
): Promise<void> {
|
||||
if (!tabWasVisible || observeStructuredWorker({ sessionId }).status === 'exited') {
|
||||
if (!restoreTab || observeStructuredWorker({ sessionId }).status === 'exited') {
|
||||
return
|
||||
}
|
||||
try {
|
||||
|
||||
@@ -56,13 +56,40 @@ function installHost(options: {
|
||||
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[] } {
|
||||
/** Sessions in the persisted visible-tab index, so a rollback has something to put back. */
|
||||
visible?: string[]
|
||||
/**
|
||||
* Sessions whose death evidence lands DURING the close's tab-restore write.
|
||||
*
|
||||
* `setSessionTabVisibility` is a store transaction — a real disk write — so the close's own
|
||||
* observation and the sweep's re-read straddle it and can disagree about the same session.
|
||||
*/
|
||||
exitsDuringTabRestore?: Set<string>
|
||||
}): { closed: string[]; visible: Set<string> } {
|
||||
const held = new Set(options.records.map((entry) => entry.sessionId))
|
||||
const closed: string[] = []
|
||||
const visible = new Set(options.visible ?? [])
|
||||
const recordExit = (sessionId: string): void => {
|
||||
const entry = options.records.find((candidate) => candidate.sessionId === sessionId)
|
||||
if (entry) {
|
||||
entry.lease.claimStatus = 'released'
|
||||
entry.lease.deathEvidence = { kind: 'exit-observed', detail: 'closed', observedAt: 1 }
|
||||
}
|
||||
}
|
||||
hostRef.current = {
|
||||
deps: { store: { listRecords: () => options.records, getRecord: () => null } },
|
||||
hasSession: (sessionId: string) => held.has(sessionId),
|
||||
setSessionTabVisibility: async () => {},
|
||||
getPersistedVisibleSessionTabIndex: () => ({ present: true, sessionIds: [...visible] }),
|
||||
setSessionTabVisibility: async (sessionId: string, isVisible: boolean) => {
|
||||
if (!isVisible) {
|
||||
visible.delete(sessionId)
|
||||
return
|
||||
}
|
||||
if (options.exitsDuringTabRestore?.has(sessionId)) {
|
||||
recordExit(sessionId)
|
||||
}
|
||||
visible.add(sessionId)
|
||||
},
|
||||
close: async (sessionId: string) => {
|
||||
closed.push(sessionId)
|
||||
await options.closeGate
|
||||
@@ -74,10 +101,8 @@ function installHost(options: {
|
||||
if (options.unverifiable?.has(sessionId)) {
|
||||
return
|
||||
}
|
||||
const record = options.records.find((entry) => entry.sessionId === sessionId)
|
||||
if (record) {
|
||||
record.lease.claimStatus = 'released'
|
||||
record.lease.deathEvidence = { kind: 'exit-observed', detail: 'closed', observedAt: 1 }
|
||||
if (!options.exitsDuringTabRestore?.has(sessionId)) {
|
||||
recordExit(sessionId)
|
||||
}
|
||||
if (options.settledThenThrows?.has(sessionId)) {
|
||||
throw new Error('the event sink could not be flushed')
|
||||
@@ -89,7 +114,7 @@ function installHost(options: {
|
||||
hostRef.current as { deps: { store: { getRecord: (id: string) => unknown } } }
|
||||
).deps.store.getRecord = (sessionId: string) =>
|
||||
options.records.find((entry) => entry.sessionId === sessionId) ?? null
|
||||
return { closed }
|
||||
return { closed, visible }
|
||||
}
|
||||
|
||||
const localProvider = {
|
||||
@@ -148,6 +173,71 @@ describe('worktree teardown and structured agent sessions', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it('puts the chat tab back when the removal refuses over the session', async () => {
|
||||
// The workspace survives a refusal, so the tab has to survive it too: a destructive operation
|
||||
// that refused and still took the user's chat tab away is the loss the rollback exists to undo.
|
||||
const host = installHost({
|
||||
records: [record('s1', WORKTREE)],
|
||||
stuck: new Set(['s1']),
|
||||
visible: ['s1']
|
||||
})
|
||||
await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).rejects.toThrow(
|
||||
/still live: 1 agent session \(claude\)/
|
||||
)
|
||||
expect([...host.visible]).toEqual(['s1'])
|
||||
})
|
||||
|
||||
it('leaves the chat tab dropped when a forced removal deletes the workspace anyway', async () => {
|
||||
// The other half of the same rollback. Force does not refuse — it warns and goes on to delete
|
||||
// the checkout — so putting the tab back leaves a DURABLE reference to a workspace that is
|
||||
// about to be gone, which republishes the chat at the next launch pointing at a deleted
|
||||
// worktree: the exact outcome this whole sweep exists to remove.
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
const host = installHost({
|
||||
records: [record('s1', WORKTREE)],
|
||||
stuck: new Set(['s1']),
|
||||
visible: ['s1']
|
||||
})
|
||||
await killAllProcessesForWorktree(WORKTREE, destructiveDeps({ allowUnverifiedStop: true }))
|
||||
expect([...host.visible]).toEqual([])
|
||||
warn.mockRestore()
|
||||
})
|
||||
|
||||
it('leaves the chat tab dropped for a folder-workspace removal, which never refuses', async () => {
|
||||
// Same reasoning without the force waiver: this caller cannot refuse at all, so the workspace
|
||||
// is forgotten whatever the close reports.
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
const host = installHost({
|
||||
records: [record('s1', WORKTREE)],
|
||||
stuck: new Set(['s1']),
|
||||
visible: ['s1']
|
||||
})
|
||||
await killAllProcessesForWorktree(WORKTREE, {
|
||||
localProvider,
|
||||
includeProviderInventory: false as const,
|
||||
includeLocalRegistry: false as const,
|
||||
closeStructuredSessions: true
|
||||
})
|
||||
expect([...host.visible]).toEqual([])
|
||||
warn.mockRestore()
|
||||
})
|
||||
|
||||
it('drops the chat tab for a session the sweep proves exited after the close gave up', async () => {
|
||||
// `host.close` can return BEFORE the child's exit is recorded, so the close's own observation
|
||||
// reads unverifiable and puts the tab back — and the sweep's re-read, one store write later,
|
||||
// proves the exit and counts the session closed. The two observations straddle that write and
|
||||
// can disagree; the tab must not survive the disagreement, because this removal proceeds.
|
||||
const host = installHost({
|
||||
records: [record('s1', WORKTREE)],
|
||||
visible: ['s1'],
|
||||
exitsDuringTabRestore: new Set(['s1'])
|
||||
})
|
||||
await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).resolves.toMatchObject({
|
||||
structuredStopped: 1
|
||||
})
|
||||
expect([...host.visible]).toEqual([])
|
||||
})
|
||||
|
||||
it('names the force escape hatch in the refusal, like the unstopped-PTY gate', async () => {
|
||||
installHost({ records: [record('s1', WORKTREE)], stuck: new Set(['s1']) })
|
||||
await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).rejects.toThrow(/force/i)
|
||||
|
||||
@@ -209,15 +209,28 @@ export function unclosedStructuredSessions(
|
||||
* the outcome this whole sweep exists to prevent, and closing is how you prevent it. What stayed is
|
||||
* the only thing worth refusing over.
|
||||
*
|
||||
* Takes the list rather than re-deriving it, so the sessions reported as unclosed are exactly the
|
||||
* ones a close was attempted on — re-enumerating would run every liveness observation twice and
|
||||
* let the refusal name a session this call never touched.
|
||||
* Takes the list rather than re-deriving it, so the refusal can only ever name a session out of
|
||||
* the set this sweep was handed — re-enumerating would run every liveness observation twice and
|
||||
* let it name one this call never touched. Not every one of them is a session a close was
|
||||
* attempted on: the deadline check below can leave the tail of the list unasked, and
|
||||
* `unclosedStructuredSessions` reports those as `unverifiable` precisely because nobody looked.
|
||||
*/
|
||||
export async function closeStructuredSessionsForWorktree(
|
||||
progress: StructuredSweepProgress,
|
||||
deadline: number,
|
||||
runtime?: StructuredWorktreeSweepRuntime
|
||||
options: {
|
||||
runtime?: StructuredWorktreeSweepRuntime
|
||||
/**
|
||||
* Whether this removal can still refuse over an unclosed session.
|
||||
*
|
||||
* It is the only case where the workspace — and therefore its chat tabs — survives, so it is
|
||||
* the only case where an unproven close may put a tab back. Force and the folder-workspace
|
||||
* paths discard the workspace whatever the sweep reports.
|
||||
*/
|
||||
mayRefuse?: boolean
|
||||
} = {}
|
||||
): Promise<void> {
|
||||
const { runtime, mayRefuse } = options
|
||||
// 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
|
||||
@@ -231,10 +244,10 @@ export async function closeStructuredSessionsForWorktree(
|
||||
if (Date.now() >= deadline) {
|
||||
return
|
||||
}
|
||||
const outcome = await closeStructuredAgentSessionChild(
|
||||
session.sessionId,
|
||||
runtime ? { runtime } : {}
|
||||
)
|
||||
const outcome = await closeStructuredAgentSessionChild(session.sessionId, {
|
||||
...(runtime ? { runtime } : {}),
|
||||
restoreTabOnUnprovenClose: mayRefuse === true
|
||||
})
|
||||
if (outcome.stopped) {
|
||||
progress.closed += 1
|
||||
} else {
|
||||
@@ -246,6 +259,11 @@ export async function closeStructuredSessionsForWorktree(
|
||||
// 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.
|
||||
//
|
||||
// Including the hide it UNDID: its rollback ran against an observation taken one store
|
||||
// write before this one, so a child that died in between left the tab republished for a
|
||||
// session this sweep is about to count closed. Taking the proof has to take that back.
|
||||
await dropDurableChatTabReference(session.sessionId)
|
||||
retireSettledStructuredWorkerTab(session.sessionId, runtime)
|
||||
progress.closed += 1
|
||||
} else {
|
||||
@@ -257,3 +275,20 @@ export async function closeStructuredSessionsForWorktree(
|
||||
progress.settled += 1
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Drops a settled session's durable chat-tab reference, and cannot fail the settlement.
|
||||
*
|
||||
* The close's own hide is the ordinary path; this is only for the session whose exit this sweep
|
||||
* proved after that close had already rolled the hide back.
|
||||
*/
|
||||
async function dropDurableChatTabReference(sessionId: string): Promise<void> {
|
||||
try {
|
||||
await getStructuredAgentSessionHost()?.setSessionTabVisibility?.(sessionId, false)
|
||||
} catch (error) {
|
||||
console.warn(
|
||||
`[worktree-teardown] could not drop the chat tab reference for ${sessionId}`,
|
||||
error
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -339,7 +339,13 @@ async function sweepStructuredSessions(
|
||||
// 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)),
|
||||
sweeps.track(() =>
|
||||
closeStructuredSessionsForWorktree(progress, deadline, {
|
||||
...(deps.runtime ? { runtime: deps.runtime } : {}),
|
||||
// The only shape of removal that can leave this workspace — and its chat tabs — in place.
|
||||
mayRefuse: Boolean(deps.requirePhysicalStop) && !deps.allowUnverifiedStop
|
||||
})
|
||||
),
|
||||
undefined,
|
||||
deadline
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user