From f2971fd6f2a9f2fcecd3d33733f95235da54e5d8 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 07:37:58 -0700 Subject: [PATCH] fix(automations): report an unverifiable process loss as lost, not failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../hooks/automation-dispatch-completion.ts | 28 +++++ ...omation-dispatch-unverifiable-loss.test.ts | 105 ++++++++++++++++++ .../src/lib/agent-background-session-exit.ts | 32 ++++++ .../agent-background-session-test-state.ts | 2 + .../lib/automation-session-observer.test.ts | 46 ++++++++ .../src/lib/automation-session-observer.ts | 3 +- .../launch-agent-background-session.test.ts | 25 +++++ .../lib/launch-agent-background-session.ts | 5 +- src/shared/terminal-exit-cause.ts | 10 ++ 9 files changed, 253 insertions(+), 3 deletions(-) create mode 100644 src/renderer/src/hooks/automation-dispatch-unverifiable-loss.test.ts create mode 100644 src/renderer/src/lib/agent-background-session-exit.ts diff --git a/src/renderer/src/hooks/automation-dispatch-completion.ts b/src/renderer/src/hooks/automation-dispatch-completion.ts index 098d1167a38..933f3666ae1 100644 --- a/src/renderer/src/hooks/automation-dispatch-completion.ts +++ b/src/renderer/src/hooks/automation-dispatch-completion.ts @@ -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 @@ -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 => { if (completionMarked) { return } + if (!isProvenProcessExit(code)) { + abandonUnverifiableRun(code) + return + } completionMarked = true cleanupRunObservers() try { diff --git a/src/renderer/src/hooks/automation-dispatch-unverifiable-loss.test.ts b/src/renderer/src/hooks/automation-dispatch-unverifiable-loss.test.ts new file mode 100644 index 00000000000..87adc2860e8 --- /dev/null +++ b/src/renderer/src/hooks/automation-dispatch-unverifiable-loss.test.ts @@ -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>() +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() + }) +}) diff --git a/src/renderer/src/lib/agent-background-session-exit.ts b/src/renderer/src/lib/agent-background-session-exit.ts new file mode 100644 index 00000000000..0234da01055 --- /dev/null +++ b/src/renderer/src/lib/agent-background-session-exit.ts @@ -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) +} diff --git a/src/renderer/src/lib/agent-background-session-test-state.ts b/src/renderer/src/lib/agent-background-session-test-state.ts index 85def0ffadb..06f553ad72b 100644 --- a/src/renderer/src/lib/agent-background-session-test-state.ts +++ b/src/renderer/src/lib/agent-background-session-test-state.ts @@ -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() diff --git a/src/renderer/src/lib/automation-session-observer.test.ts b/src/renderer/src/lib/automation-session-observer.test.ts index 69c75319fed..3ef489314c5 100644 --- a/src/renderer/src/lib/automation-session-observer.test.ts +++ b/src/renderer/src/lib/automation-session-observer.test.ts @@ -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') diff --git a/src/renderer/src/lib/automation-session-observer.ts b/src/renderer/src/lib/automation-session-observer.ts index a953652fefa..d96cb49b14e 100644 --- a/src/renderer/src/lib/automation-session-observer.ts +++ b/src/renderer/src/lib/automation-session-observer.ts @@ -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(() => {}) diff --git a/src/renderer/src/lib/launch-agent-background-session.test.ts b/src/renderer/src/lib/launch-agent-background-session.test.ts index 42df8332882..ea7b1a30762 100644 --- a/src/renderer/src/lib/launch-agent-background-session.test.ts +++ b/src/renderer/src/lib/launch-agent-background-session.test.ts @@ -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 () => { diff --git a/src/renderer/src/lib/launch-agent-background-session.ts b/src/renderer/src/lib/launch-agent-background-session.ts index a1d9f1df654..6bc6c65e420 100644 --- a/src/renderer/src/lib/launch-agent-background-session.ts +++ b/src/renderer/src/lib/launch-agent-background-session.ts @@ -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 diff --git a/src/shared/terminal-exit-cause.ts b/src/shared/terminal-exit-cause.ts index 1fa3d2f14b6..ae2bca6e9cc 100644 --- a/src/shared/terminal-exit-cause.ts +++ b/src/shared/terminal-exit-cause.ts @@ -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. *