mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 08:03:12 +00:00
fix(automations): report an unverifiable process loss as lost, not failed
Two automation readers consumed the raw PTY exit code with no liveness
check, so the -1 unverified sentinel — on SSH, a live relay whose reattach
failed — was published as status 'dispatch_failed' with "Automation process
exited with code -1." The run was asserted finished when all that happened
was that we lost contact.
Route both through the existing vocabulary:
- The completion tracker records no result for an unproven code. The run
keeps its non-final 'dispatched' status, so it is never evicted and never
shown as Failed, and stays owned by main's AutomationRunCompletionWatcher,
which already reports a genuinely unobservable run truthfully ("lost the
terminal for this run") rather than inventing an exit code. finalize() is
never reached, so a terminal whose process cannot be proven dead is never
closed. A later done can still complete the run.
- Both runtime `terminal.wait` readers defaulted an absent status to 0,
minting a clean finish out of no evidence. They now share
runtimeWaitExitCode, which defaults to the new UNVERIFIED_PROCESS_EXIT_CODE.
- The background-session exit handler no longer clears the tab-PTY binding
on an unverified loss, matching pty-exit-hibernate.ts, and marks the tab
so orphan cleanup cannot sweep an agent that may still be running.
A proven exit is unchanged: 0 still completes and finalizes, and a real
nonzero failure still reports dispatch_failed.
This commit is contained in:
@@ -14,6 +14,7 @@ import {
|
||||
UNCHANGED_AUTOMATION_AGENT_STATUS_ENTRY
|
||||
} from './automation-agent-status-entry-change'
|
||||
import type { Worktree } from '../../../shared/worktree/types'
|
||||
import { isProvenProcessExit } from '../../../shared/terminal-exit-cause'
|
||||
|
||||
type MarkDispatchResult = (result: AutomationDispatchResult) => Promise<void>
|
||||
|
||||
@@ -33,6 +34,7 @@ export function createAutomationDispatchCompletion(args: {
|
||||
let pendingExitCode: number | null = null
|
||||
let pendingDone = false
|
||||
let completionMarked = false
|
||||
let contactLost = false
|
||||
let unsubscribeAgentStatus = (): void => {}
|
||||
let unsubscribeSessionObserver = (): void => {}
|
||||
let releaseReuseDispatchTab = (): void => {}
|
||||
@@ -85,10 +87,36 @@ export function createAutomationDispatchCompletion(args: {
|
||||
console.error('[automations] Failed to clear retired terminal identity:', error)
|
||||
}
|
||||
}
|
||||
/**
|
||||
* A lost PTY is not a result. Record nothing: the run keeps its non-final
|
||||
* `dispatched` status, so it is never evicted from history and never shown as
|
||||
* Failed for work that is very likely still running (on SSH, a relay whose
|
||||
* reattach failed). Ownership of an unobservable run belongs to main's
|
||||
* AutomationRunCompletionWatcher, which reports the truthful "lost the
|
||||
* terminal for this run" instead of an exit code nobody witnessed.
|
||||
*
|
||||
* `finalize()` is deliberately never reached here — closing the terminal of a
|
||||
* process we cannot prove dead is what orphans live work.
|
||||
*/
|
||||
const abandonUnverifiableRun = (code: number): void => {
|
||||
if (completionMarked || contactLost) {
|
||||
return
|
||||
}
|
||||
contactLost = true
|
||||
cleanupRunObservers()
|
||||
args.releaseTerminalOwnership()
|
||||
console.warn(
|
||||
`[automations] Lost contact with the process for run ${args.run.id} (code ${code}); leaving the run dispatched rather than reporting an exit.`
|
||||
)
|
||||
}
|
||||
const markExitResult = async (code: number): Promise<void> => {
|
||||
if (completionMarked) {
|
||||
return
|
||||
}
|
||||
if (!isProvenProcessExit(code)) {
|
||||
abandonUnverifiableRun(code)
|
||||
return
|
||||
}
|
||||
completionMarked = true
|
||||
cleanupRunObservers()
|
||||
try {
|
||||
|
||||
@@ -0,0 +1,105 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { AutomationDispatchResult } from '../../../shared/automations-types'
|
||||
|
||||
vi.mock('@/store', () => ({
|
||||
useAppStore: {
|
||||
getState: () => ({ agentStatusByPaneKey: {} }),
|
||||
subscribe: vi.fn(() => () => {})
|
||||
}
|
||||
}))
|
||||
|
||||
const markDispatchResult = vi.fn<(result: AutomationDispatchResult) => Promise<void>>()
|
||||
const releaseTerminalOwnership = vi.fn()
|
||||
const finalizeTerminalOwnership = vi.fn(() => false)
|
||||
|
||||
async function createCompletion() {
|
||||
const { createAutomationDispatchCompletion } = await import('./automation-dispatch-completion')
|
||||
const completion = createAutomationDispatchCompletion({
|
||||
run: { id: 'run-1' } as never,
|
||||
worktree: { id: 'wt-1', displayName: 'Automation worktree' } as never,
|
||||
precheckResult: null,
|
||||
markDispatchResult,
|
||||
releaseTerminalOwnership,
|
||||
finalizeTerminalOwnership
|
||||
})
|
||||
// The dispatch itself is already recorded before any exit can settle it.
|
||||
await completion.settlePendingAfterDispatch()
|
||||
markDispatchResult.mockClear()
|
||||
return completion
|
||||
}
|
||||
|
||||
/**
|
||||
* Loss of contact is never evidence of process death
|
||||
* (docs/reference/ssh-execution-boundary.md). The exit sentinel these readers
|
||||
* receive is the same one the terminal panes already classify as unverifiable.
|
||||
*/
|
||||
describe('automation dispatch completion on an unverifiable loss', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
markDispatchResult.mockResolvedValue(undefined)
|
||||
finalizeTerminalOwnership.mockReturnValue(false)
|
||||
})
|
||||
|
||||
it('records no result, so the run keeps its non-final dispatched status', async () => {
|
||||
// A -1 is a lost relay or a synthesized host-shutdown fanout. Reporting
|
||||
// "exited with code -1" asserts a finish nobody witnessed; on SSH the
|
||||
// automation is very likely still running.
|
||||
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined)
|
||||
const completion = await createCompletion()
|
||||
|
||||
completion.handleExit(-1)
|
||||
await vi.waitFor(() => expect(releaseTerminalOwnership).toHaveBeenCalledOnce())
|
||||
|
||||
expect(markDispatchResult).not.toHaveBeenCalled()
|
||||
// Closing a terminal whose process cannot be proven dead orphans live work.
|
||||
expect(finalizeTerminalOwnership).not.toHaveBeenCalled()
|
||||
warnSpy.mockRestore()
|
||||
})
|
||||
|
||||
it('still completes and finalizes a genuinely exited process', async () => {
|
||||
const completion = await createCompletion()
|
||||
finalizeTerminalOwnership.mockReturnValue(true)
|
||||
|
||||
completion.handleExit(0)
|
||||
await vi.waitFor(() => expect(finalizeTerminalOwnership).toHaveBeenCalledOnce())
|
||||
|
||||
expect(markDispatchResult).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ runId: 'run-1', status: 'completed', error: null })
|
||||
)
|
||||
expect(releaseTerminalOwnership).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('still reports a real automation failure as dispatch_failed', async () => {
|
||||
const completion = await createCompletion()
|
||||
|
||||
completion.handleExit(9)
|
||||
await vi.waitFor(() => expect(releaseTerminalOwnership).toHaveBeenCalledOnce())
|
||||
|
||||
expect(markDispatchResult).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
runId: 'run-1',
|
||||
status: 'dispatch_failed',
|
||||
error: 'Automation process exited with code 9.'
|
||||
})
|
||||
)
|
||||
expect(finalizeTerminalOwnership).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('lets a later done still complete a run whose contact was lost', async () => {
|
||||
// The loss withheld a verdict rather than settling one, so positive
|
||||
// evidence arriving afterwards must still be able to close the run.
|
||||
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined)
|
||||
const completion = await createCompletion()
|
||||
|
||||
completion.handleExit(-1)
|
||||
await vi.waitFor(() => expect(releaseTerminalOwnership).toHaveBeenCalledOnce())
|
||||
completion.handleAgentDone()
|
||||
|
||||
await vi.waitFor(() =>
|
||||
expect(markDispatchResult).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ status: 'completed' })
|
||||
)
|
||||
)
|
||||
warnSpy.mockRestore()
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,32 @@
|
||||
import { useAppStore } from '@/store'
|
||||
import {
|
||||
isProvenProcessExit,
|
||||
UNVERIFIED_PROCESS_EXIT_CODE
|
||||
} from '../../../shared/terminal-exit-cause'
|
||||
|
||||
/**
|
||||
* The code a runtime `terminal.wait` actually reported.
|
||||
*
|
||||
* Why not `?? 0`: a wait that answers without a status observed nothing, and a
|
||||
* fabricated zero would be read downstream as a clean finish.
|
||||
*/
|
||||
export function runtimeWaitExitCode(wait: { exitCode?: number | null }): number {
|
||||
return wait.exitCode ?? UNVERIFIED_PROCESS_EXIT_CODE
|
||||
}
|
||||
|
||||
/**
|
||||
* Settle a background agent tab's PTY binding when its session ends.
|
||||
*
|
||||
* Mirrors the rule the mounted panes follow (pty-exit-hibernate.ts): only a
|
||||
* proven exit drops the tab↔PTY identity. A synthetic loss sentinel retires the
|
||||
* transport alone, so the binding stays for reconnect to adopt and the tab is
|
||||
* marked so orphan cleanup cannot sweep an agent that may still be running.
|
||||
*/
|
||||
export function settleTabPtyBinding(tabId: string, ptyId: string, code: number): void {
|
||||
const state = useAppStore.getState()
|
||||
if (isProvenProcessExit(code)) {
|
||||
state.clearTabPtyId(tabId, ptyId)
|
||||
return
|
||||
}
|
||||
state.markUnverifiedPtyLoss(tabId)
|
||||
}
|
||||
@@ -49,6 +49,7 @@ export type AgentBackgroundSessionTestState = {
|
||||
closeTab: TestMock
|
||||
setTabLayout: TestMock
|
||||
clearTabPtyId: TestMock
|
||||
markUnverifiedPtyLoss: TestMock
|
||||
setAgentStatus: TestMock
|
||||
registerAgentLaunchConfig: TestMock
|
||||
clearAgentLaunchConfig: TestMock
|
||||
@@ -114,6 +115,7 @@ export function createAgentBackgroundSessionTestState(mocks: {
|
||||
closeTab: mocks.closeTab,
|
||||
setTabLayout: mocks.setTabLayout,
|
||||
clearTabPtyId: vi.fn(),
|
||||
markUnverifiedPtyLoss: vi.fn(),
|
||||
setAgentStatus: vi.fn(),
|
||||
registerAgentLaunchConfig: mocks.registerAgentLaunchConfig,
|
||||
clearAgentLaunchConfig: vi.fn()
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { toAppSshPtyId } from '../../../shared/ssh-pty-id'
|
||||
import { UNVERIFIED_PROCESS_EXIT_CODE } from '../../../shared/terminal-exit-cause'
|
||||
|
||||
const mockSubscribeToPtyData = vi.fn()
|
||||
const mockSubscribeToPtyExit = vi.fn()
|
||||
@@ -153,6 +154,51 @@ describe('observeExistingAutomationSession', () => {
|
||||
expect(onAgentStatus).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('reports a runtime wait that carried no status as unverified, not as a clean exit', async () => {
|
||||
// `exitCode ?? 0` fabricated a clean finish out of an absent status, so a
|
||||
// host that answered without one was read as a completed automation.
|
||||
state.terminalLayoutsByTabId = {
|
||||
'tab-1': { ptyIdsByLeafId: { [LEAF_ID]: 'remote:env-1@@terminal-9' } }
|
||||
}
|
||||
state.ptyIdsByTabId = { 'tab-1': ['remote:env-1@@terminal-9'] }
|
||||
mockCallRuntimeRpc.mockResolvedValue({ wait: {} })
|
||||
const onExit = vi.fn()
|
||||
const { observeExistingAutomationSession } = await import('./automation-session-observer')
|
||||
|
||||
await observeExistingAutomationSession({
|
||||
ptyId: 'remote:env-1@@terminal-9',
|
||||
paneKey: PANE_KEY,
|
||||
runId: 'run-1',
|
||||
onData: vi.fn(),
|
||||
onAgentStatus: vi.fn(),
|
||||
onExit
|
||||
})
|
||||
|
||||
await vi.waitFor(() => expect(onExit).toHaveBeenCalledTimes(1))
|
||||
expect(onExit).toHaveBeenCalledWith(UNVERIFIED_PROCESS_EXIT_CODE)
|
||||
})
|
||||
|
||||
it('still forwards a status the runtime host did report', async () => {
|
||||
state.terminalLayoutsByTabId = {
|
||||
'tab-1': { ptyIdsByLeafId: { [LEAF_ID]: 'remote:env-1@@terminal-9' } }
|
||||
}
|
||||
state.ptyIdsByTabId = { 'tab-1': ['remote:env-1@@terminal-9'] }
|
||||
mockCallRuntimeRpc.mockResolvedValue({ wait: { exitCode: 0 } })
|
||||
const onExit = vi.fn()
|
||||
const { observeExistingAutomationSession } = await import('./automation-session-observer')
|
||||
|
||||
await observeExistingAutomationSession({
|
||||
ptyId: 'remote:env-1@@terminal-9',
|
||||
paneKey: PANE_KEY,
|
||||
runId: 'run-1',
|
||||
onData: vi.fn(),
|
||||
onAgentStatus: vi.fn(),
|
||||
onExit
|
||||
})
|
||||
|
||||
await vi.waitFor(() => expect(onExit).toHaveBeenCalledWith(0))
|
||||
})
|
||||
|
||||
it('stamps the exact SSH PTY in the legacy renderer fallback', async () => {
|
||||
state.settings.terminalMainSideEffectAuthority = false
|
||||
const ptyId = toAppSshPtyId('ssh-a', 'pty-1')
|
||||
|
||||
@@ -9,6 +9,7 @@ import {
|
||||
} from '@/runtime/runtime-terminal-stream'
|
||||
import { useAppStore } from '@/store'
|
||||
import { createAgentStatusOscProcessor } from '../../../shared/agent-status-osc'
|
||||
import { runtimeWaitExitCode } from '@/lib/agent-background-session-exit'
|
||||
import type { ParsedAgentStatusPayload } from '../../../shared/agent-status-types'
|
||||
import { isMainTerminalSideEffectAuthorityForPty } from '@/components/terminal-pane/terminal-side-effect-facts-handler'
|
||||
import { resolveLiveAgentStatusConnectionRouting } from '@/lib/agent-status-connection-ownership'
|
||||
@@ -93,7 +94,7 @@ export async function observeExistingAutomationSession(args: {
|
||||
)
|
||||
.then((result) => {
|
||||
if (!disposed) {
|
||||
onExit(result.wait.exitCode ?? 0)
|
||||
onExit(runtimeWaitExitCode(result.wait))
|
||||
}
|
||||
})
|
||||
.catch(() => {})
|
||||
|
||||
@@ -474,6 +474,31 @@ describe('launchAgentBackgroundSession', () => {
|
||||
)
|
||||
expect(onExit).toHaveBeenCalledWith('pty-1', 0)
|
||||
expect(unsubscribe).toHaveBeenCalled()
|
||||
expect(state.markUnverifiedPtyLoss).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('keeps the tab bound to its PTY when contact was lost rather than observed', async () => {
|
||||
// Same rule the terminal panes follow: a -1 sentinel retires the transport
|
||||
// only. Clearing the binding would leave a reconnect with nothing to adopt
|
||||
// and let orphan cleanup sweep a tab whose agent may still be running.
|
||||
mockSubscribeToPtyExit.mockReturnValue(vi.fn())
|
||||
const onExit = vi.fn()
|
||||
const { launchAgentBackgroundSession } = await import('./launch-agent-background-session')
|
||||
|
||||
await launchAgentBackgroundSession({
|
||||
agent: 'claude',
|
||||
worktreeId: 'wt-1',
|
||||
prompt: 'run the automation',
|
||||
onExit
|
||||
})
|
||||
|
||||
const sidecar = mockSubscribeToPtyExit.mock.calls[0]?.[1] as (code: number) => void
|
||||
sidecar(-1)
|
||||
|
||||
const tabId = expectReservedAgentBackgroundTabId(mockSpawn)
|
||||
expect(state.clearTabPtyId).not.toHaveBeenCalled()
|
||||
expect(state.markUnverifiedPtyLoss).toHaveBeenCalledWith(tabId)
|
||||
expect(onExit).toHaveBeenCalledWith('pty-1', -1)
|
||||
})
|
||||
|
||||
it('leaves no tab behind if PTY spawn fails', async () => {
|
||||
|
||||
@@ -40,6 +40,7 @@ import {
|
||||
} from '@/lib/adopt-agent-background-session-tab'
|
||||
import { createBackgroundAgentStatusConsumer } from '@/lib/background-agent-status-consumer'
|
||||
import { isWslUncPath } from '../../../shared/wsl-paths'
|
||||
import { runtimeWaitExitCode, settleTabPtyBinding } from '@/lib/agent-background-session-exit'
|
||||
|
||||
export async function launchAgentBackgroundSession(
|
||||
args: LaunchAgentBackgroundSessionArgs
|
||||
@@ -145,7 +146,7 @@ export async function launchAgentBackgroundSession(
|
||||
unsubscribeData()
|
||||
sshStartupDelivery.clear()
|
||||
if (tab) {
|
||||
useAppStore.getState().clearTabPtyId(tab.id, exitPtyId)
|
||||
settleTabPtyBinding(tab.id, exitPtyId, code)
|
||||
}
|
||||
useAppStore.getState().clearAgentLaunchConfig(paneKey)
|
||||
onExit?.(exitPtyId, code)
|
||||
@@ -279,7 +280,7 @@ export async function launchAgentBackgroundSession(
|
||||
{ terminal: runtimeTerminalHandle, for: 'exit' },
|
||||
{ timeoutMs: 24 * 60 * 60 * 1000 }
|
||||
)
|
||||
.then((result) => handleExit(ptyId, result.wait.exitCode ?? 0))
|
||||
.then((result) => handleExit(ptyId, runtimeWaitExitCode(result.wait)))
|
||||
.catch(() => {})
|
||||
} else {
|
||||
// Why the incarnation: a relay-recycled id can hold the previous owner's exit, and draining
|
||||
|
||||
@@ -34,6 +34,16 @@ export type TerminalExitUnknownReason =
|
||||
|
||||
export const OPERATOR_CLOSE_EXIT_CAUSE: TerminalExitCause = { kind: 'operator_close' }
|
||||
|
||||
/**
|
||||
* The code every surface uses for "contact was lost before the host could vouch
|
||||
* for this process". `resolveProcessExitCause` reads it as `stop_unverified`
|
||||
* and {@link isProvenProcessExit} rejects it.
|
||||
*
|
||||
* A reader handed an *optional* status by a host must default to this, never to
|
||||
* `0`: `exitCode ?? 0` mints a clean finish out of an absence of evidence.
|
||||
*/
|
||||
export const UNVERIFIED_PROCESS_EXIT_CODE = -1
|
||||
|
||||
/**
|
||||
* Build a cause from what the host actually observed.
|
||||
*
|
||||
|
||||
Reference in New Issue
Block a user