From 74e6478e852b24b9aa0d695632cfa43ebcbdec59 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 26 Jul 2026 21:04:39 -0700 Subject: [PATCH] fix(codex): sweep stale panes on real PTY binds and stop filtering out launcher panes (#10803) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the two defects a live Windows run found in #10770's stale-account detection, which together made it a no-op on the reporter's platform. 1. The startup sweep fired once during hydration, before any PTY was bound, so getLiveCodexSessionPtyIds found nothing, returned early and never retried — the prompt never appeared after an app restart. It now arms off real PTY binds with a bounded per-PTY ladder (300/1500/4000ms), keyed on the layout binding that every bind path rewrites rather than on a point in time. 2. Windows reports the DEEPEST process in the PTY tree, so an Orca launcher Codex pane running a subagent read as "claude" and was filtered out before the stale-account registry was ever consulted — no card on switch, none from the sweep. Eligibility now falls back to tab.launchAgent, deliberately paired with a foreground that can only be a deeper agent or an unresolved wrapper, so a pane where the user exited Codex back to a shell is never marked. Four further defects found in review: a HIGH dead-keyboard regression where the widened predicate marked a Codex tab running less/vim/ssh; a restored-reattach bind site with zero coverage; a retry-ladder rung collapse where one pane's timer burned another's rung; and a thrown sweep that permanently stranded every pane parked on a later rung (that one introduced by the fix for the third). Live-validated in a real Orca dev build: all five cases pass, including the less/vim pane getting no card and typing fine, two worktrees prompting with independent per-PTY timings (741ms / 658ms after their binds, no rung collapse), and the eligibility module returning all 14 matrix cases correctly including the Windows claude.exe subagent shape. Bind-to-card ~0.66s, leaving ~3.3s of ladder unused. Not verified: the real Windows process tree (macOS fixtures only) — a Windows re-run of the original T5/W4 cases is still required before #10770 lands. Known and out of scope: a HOST account switch marks and mutes live SSH remote panes. Pre-existing (base filtered purely on isCodexForegroundProcess with no lane check) but this PR widens its blast radius via the launchAgent fallback; needs a follow-up gating markLiveCodexSessionsForRestart on the selection lane. This PR's own mechanism is safe there — remote panes are never written to the pane-account registry, verified on disk. --- src/renderer/src/App.tsx | 7 - .../terminal-pane/pty-connection.test.ts | 12 + .../terminal-pane/pty-connection.ts | 5 + .../codex-pane-restart-eligibility.test.ts | 111 ++++++ .../src/lib/codex-pane-restart-eligibility.ts | 64 ++++ .../src/lib/codex-session-restart.test.ts | 61 ++++ src/renderer/src/lib/codex-session-restart.ts | 88 +++-- .../src/lib/codex-stale-pane-sweep.test.ts | 324 ++++++++++++++++++ .../src/lib/codex-stale-pane-sweep.ts | 161 +++++++++ 9 files changed, 788 insertions(+), 45 deletions(-) create mode 100644 src/renderer/src/lib/codex-pane-restart-eligibility.test.ts create mode 100644 src/renderer/src/lib/codex-pane-restart-eligibility.ts create mode 100644 src/renderer/src/lib/codex-stale-pane-sweep.test.ts create mode 100644 src/renderer/src/lib/codex-stale-pane-sweep.ts diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index e5c6ee83080..842a7a9e213 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -122,7 +122,6 @@ import { shouldPersistWorkspaceSession } from './lib/workspace-session' import { createSessionWriteSubscriber } from './lib/session-write-subscriber' -import { markRestoredStaleCodexSessionsForRestart } from './lib/codex-session-restart' import { buildActiveViewUnloadPatch } from './lib/active-view-persist' import { buildWorkspaceSessionHostSnapshots, @@ -1090,12 +1089,6 @@ function App(): React.JSX.Element { } } })() - // Why: restored Codex panes keep the account that was selected when - // their shell first opened, so re-raise the restart prompt the app - // restart discarded. Deferred — it is a hint, never startup-critical. - void markRestoredStaleCodexSessionsForRestart().catch((err: unknown) => { - console.warn('Codex stale-pane restart sweep failed:', err) - }) } } catch (error) { // Why (issue #1158): leave in-memory state untouched and keep hydrationSucceeded false (default-hydrating here once erased saved tabs); still flip the ready flags so the UI mounts. diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index 6a95ae76840..bbdae133222 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -96,6 +96,7 @@ async function renderHeadlessTerminalState( } const toastInfo = vi.fn() +const notifyCodexPaneBoundForStaleSweep = vi.fn() const LEAF_1 = '11111111-1111-4111-8111-111111111111' as const const LEAF_2 = '22222222-2222-4222-8222-222222222222' as const const UUID_RE = '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}' @@ -328,6 +329,10 @@ vi.mock('sonner', () => ({ } })) +vi.mock('@/lib/codex-stale-pane-sweep', () => ({ + notifyCodexPaneBoundForStaleSweep +})) + // Why: the working→idle test invokes the real useNotificationDispatch hook outside React, so useCallback must pass through (safe suite-wide: no test here renders React). vi.mock('react', async (importOriginal) => { const actual = await importOriginal() @@ -4327,6 +4332,9 @@ describe('connectPanePty', () => { expect(pane.container.dataset.ptyId).toBe('pty-daemon-reattach') expect(deps.syncPanePtyLayoutBinding).toHaveBeenCalledWith(1, 'pty-daemon-reattach') expect(deps.updateTabPtyId).toHaveBeenCalledWith('tab-1', 'pty-daemon-reattach') + // Why: the restored shell keeps the CODEX_HOME it was spawned with, and this + // bind is the first moment the daemon PTY can be inspected for it. + expect(notifyCodexPaneBoundForStaleSweep).toHaveBeenCalledWith('pty-daemon-reattach') }) it('drops xterm onData while pane is replaying restored bytes', async () => { @@ -6330,6 +6338,10 @@ describe('connectPanePty', () => { expect(transport.attach).not.toHaveBeenCalled() await Promise.resolve() expect(deps.syncPanePtyLayoutBinding).toHaveBeenCalledWith(2, 'leaf-pty-2') + // Why: a pane that outlived the app reaches its PTY only through this + // restored-session reattach, so the stale-account sweep must be queued here + // too — the fresh-spawn chokepoint never runs for it. + expect(notifyCodexPaneBoundForStaleSweep).toHaveBeenCalledWith('leaf-pty-2') }) it('resizes a reattached PTY to the current grid when the pane narrows before reattach resolves', async () => { diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index b2e56485fc1..7229c6c6b73 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -130,6 +130,9 @@ import { registerPtyTitleSource } from './pty-buffer-serializer' import { inspectRuntimeTerminalProcess } from '@/runtime/runtime-terminal-inspection' +// Why: a restored pane's stale-account prompt can only be raised once a PTY is +// actually attached — nothing is inspectable while the session hydrates. +import { notifyCodexPaneBoundForStaleSweep } from '@/lib/codex-stale-pane-sweep' import { getRemoteRuntimePtyEnvironmentId } from '@/runtime/runtime-terminal-stream' import { discardTerminalOutput, @@ -2824,6 +2827,7 @@ export function connectPanePty( registerSideEffectFactConsumerForPty(ptyId) syncHiddenRendererPtyDelivery() deps.syncPanePtyLayoutBinding(pane.id, ptyId) + notifyCodexPaneBoundForStaleSweep(ptyId) const tabPtyIds = useAppStore.getState().ptyIdsByTabId?.[deps.tabId] ?? [] if (options.updateTabPtyId !== 'if-missing' || !tabPtyIds.includes(ptyId)) { if (options.replacePtyId) { @@ -7335,6 +7339,7 @@ export function connectPanePty( registerSideEffectFactConsumerForPty(ptyId) syncHiddenRendererPtyDelivery() deps.syncPanePtyLayoutBinding(pane.id, ptyId) + notifyCodexPaneBoundForStaleSweep(ptyId) deps.updateTabPtyId(deps.tabId, ptyId) agentCompletionCoordinator.startProcessTracking() sampleVisiblePaneForegroundAgent() diff --git a/src/renderer/src/lib/codex-pane-restart-eligibility.test.ts b/src/renderer/src/lib/codex-pane-restart-eligibility.test.ts new file mode 100644 index 00000000000..b4f26208580 --- /dev/null +++ b/src/renderer/src/lib/codex-pane-restart-eligibility.test.ts @@ -0,0 +1,111 @@ +import { describe, expect, it } from 'vitest' +import { isCodexRestartEligiblePane } from './codex-pane-restart-eligibility' + +describe('isCodexRestartEligiblePane', () => { + it('accepts a pane whose foreground is Codex itself', () => { + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess: 'codex', hasChildProcesses: false }, + launchAgent: undefined + }) + ).toBe(true) + }) + + it('accepts the shipped Codex binary name and the Windows .exe suffix', () => { + for (const foregroundProcess of ['codex-aarch64-ap', 'codex.exe']) { + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess, hasChildProcesses: false }, + launchAgent: undefined + }) + ).toBe(true) + } + }) + + it('accepts a launcher-started Codex pane whose deepest process is a subagent', () => { + // Windows reports pwsh -> node -> codex.exe -> claude.exe as "claude". + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess: 'claude.exe', hasChildProcesses: true }, + launchAgent: 'codex' + }) + ).toBe(true) + }) + + it('accepts a launcher-started Codex pane whose wrapper has not resolved to an agent yet', () => { + // Windows reports pwsh while the descendant scan warms up, then `node` + // (the `node .../bin/codex` wrapper) before it resolves to "codex". + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess: 'node', hasChildProcesses: true }, + launchAgent: 'codex' + }) + ).toBe(true) + }) + + it('rejects a Codex-launched pane the user exited and is now typing into', () => { + // Why: the process scans only ever surface a recognized agent or the shell, + // so `less`/`vim`/`ssh` means Codex is gone — and a notice would eat the + // keystrokes the user needs to leave that program. + for (const foregroundProcess of ['less', 'vim', 'ssh', 'tmux', 'git']) { + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess, hasChildProcesses: true }, + launchAgent: 'codex' + }) + ).toBe(false) + } + }) + + it('rejects a Codex-launched pane the user exited back to a shell prompt', () => { + // A restart notice drops every keystroke, so a bare prompt must stay unmarked. + for (const foregroundProcess of ['pwsh.exe', 'powershell.exe', 'bash', 'zsh']) { + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess, hasChildProcesses: false }, + launchAgent: 'codex' + }) + ).toBe(false) + } + }) + + it('rejects a Codex-launched pane whose shell is reported with no live child', () => { + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess: 'pwsh.exe', hasChildProcesses: true }, + launchAgent: 'codex' + }) + ).toBe(false) + }) + + it('rejects a non-Codex pane with a live child process', () => { + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess: 'claude.exe', hasChildProcesses: true }, + launchAgent: 'claude' + }) + ).toBe(false) + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess: 'node', hasChildProcesses: true }, + launchAgent: undefined + }) + ).toBe(false) + }) + + it('rejects a pane whose process could not be read', () => { + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess: null, hasChildProcesses: false }, + launchAgent: 'codex' + }) + ).toBe(false) + // Why: a stale remote handle reports the last-known name; it is not evidence. + expect( + isCodexRestartEligiblePane({ + inspection: { foregroundProcess: 'codex', hasChildProcesses: true, unavailable: true }, + launchAgent: 'codex' + }) + ).toBe(false) + }) +}) diff --git a/src/renderer/src/lib/codex-pane-restart-eligibility.ts b/src/renderer/src/lib/codex-pane-restart-eligibility.ts new file mode 100644 index 00000000000..dc9f63b6f7b --- /dev/null +++ b/src/renderer/src/lib/codex-pane-restart-eligibility.ts @@ -0,0 +1,64 @@ +import { + isAgentForegroundWrapperProcess, + recognizeAgentProcess +} from '../../../shared/agent-process-recognition' +import { isShellProcess } from '../../../shared/shell-process-detection' +import type { TuiAgent } from '../../../shared/types' +import type { RuntimeTerminalProcessInspection } from '@/runtime/runtime-terminal-inspection' + +function normalizeProcessName(processName: string | null): string | null { + if (!processName) { + return null + } + return processName.toLowerCase().replace(/\.exe$/, '') +} + +export function isCodexForegroundProcess(processName: string | null): boolean { + const normalized = normalizeProcessName(processName) + if (!normalized) { + return false + } + // Why: node-pty exposes the OS foreground process name, which can be the + // shipped Codex binary name (for example "codex-aarch64-ap" on macOS) + // instead of the shell command the user typed. Match on a Codex prefix so + // account-switch restart prompts still appear for real Codex sessions. + return normalized === 'codex' || normalized.startsWith('codex-') +} + +/** + * Decides whether a pane may be shown a Codex account-restart prompt. + * + * Why this is not just a foreground-name match: Windows reports the DEEPEST + * process in the PTY tree, so an Orca "Codex" pane running a subagent reads as + * `pwsh -> node -> codex.exe -> claude.exe` => "claude" and was filtered out + * before the stale-account registry was ever consulted. `launchAgent` is + * recorded metadata (Orca started Codex in this tab), not a repaintable label, + * so it survives that. A restart notice makes the pane drop every keystroke, so + * the fallback is deliberately paired with a foreground that can only be a + * deeper *agent* (or the node/python wrapper that has not resolved to one yet): + * both the Windows descendant scan and the POSIX `ps` walk only ever surface a + * recognized agent or the shell/tty foreground, so `less`, `vim` or `ssh` in + * that pane means the user exited Codex and is typing at their own program. + */ +export function isCodexRestartEligiblePane(args: { + inspection: RuntimeTerminalProcessInspection + launchAgent: TuiAgent | undefined +}): boolean { + const { foregroundProcess, hasChildProcesses, unavailable } = args.inspection + if (unavailable === true) { + return false + } + if (isCodexForegroundProcess(foregroundProcess)) { + return true + } + if (args.launchAgent !== 'codex' || foregroundProcess === null || !hasChildProcesses) { + return false + } + if (isShellProcess(foregroundProcess)) { + return false + } + return ( + recognizeAgentProcess(foregroundProcess) !== null || + isAgentForegroundWrapperProcess(foregroundProcess) + ) +} diff --git a/src/renderer/src/lib/codex-session-restart.test.ts b/src/renderer/src/lib/codex-session-restart.test.ts index 2182a79a2bb..7e03475bdcb 100644 --- a/src/renderer/src/lib/codex-session-restart.test.ts +++ b/src/renderer/src/lib/codex-session-restart.test.ts @@ -1,6 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { useAppStore } from '@/store' import { shouldUseShellReadyStartupDelivery } from '../../../shared/codex-startup-delivery' +import type { TuiAgent } from '../../../shared/types' import { CODEX_ACCOUNT_RESTART_STARTUP, markLiveCodexSessionsForRestart, @@ -16,6 +17,14 @@ const ACCOUNT_A = 'account-a@example.com' const ACCOUNT_B = 'account-b@example.com' const ACCOUNT_C = 'account-c@example.com' +function setLaunchAgentOnFirstTab(launchAgent: TuiAgent): void { + const [tab, ...rest] = useAppStore.getState().tabsByWorktree.wt1 ?? [] + if (!tab) { + throw new Error('expected a seeded tab') + } + useAppStore.setState({ tabsByWorktree: { wt1: [{ ...tab, launchAgent }, ...rest] } }) +} + describe('CODEX_ACCOUNT_RESTART_STARTUP', () => { it('waits for shell readiness before relaunching Codex after an account switch', () => { expect(CODEX_ACCOUNT_RESTART_STARTUP).toEqual({ @@ -181,6 +190,58 @@ describe('markLiveCodexSessionsForRestart', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) }) + it('marks a launcher-started Codex pane whose deepest process is a subagent', async () => { + // Windows reports pwsh -> node -> codex.exe -> claude.exe as "claude". + setLaunchAgentOnFirstTab('codex') + vi.mocked(window.api.pty.inspectProcess).mockResolvedValue({ + foregroundProcess: 'claude.exe', + hasChildProcesses: true + }) + + await markLiveCodexSessionsForRestart({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + }) + + it('leaves a Codex-launched pane alone once the user exits back to the shell', async () => { + setLaunchAgentOnFirstTab('codex') + vi.mocked(window.api.pty.inspectProcess).mockResolvedValue({ + foregroundProcess: 'pwsh.exe', + hasChildProcesses: false + }) + + await markLiveCodexSessionsForRestart({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + + // Why: a restart notice drops every keystroke in that pane. + expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + }) + + it('leaves a Codex-launched pane alone while the user is inside their own program', async () => { + // Why: the switch path never consults the stale-pane registry, so an exited + // Codex tab now running a pager would lose the keystrokes needed to quit it. + setLaunchAgentOnFirstTab('codex') + vi.mocked(window.api.pty.inspectProcess).mockResolvedValue({ + foregroundProcess: 'less', + hasChildProcesses: true + }) + + await markLiveCodexSessionsForRestart({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + + expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + }) + it('still marks a confirmed Codex pane when another pane is unreachable', async () => { useAppStore.setState({ ptyIdsByTabId: { 'tab-1': ['pty-1', 'pty-stale'] } }) vi.mocked(window.api.pty.inspectProcess).mockImplementation(async (ptyId) => { diff --git a/src/renderer/src/lib/codex-session-restart.ts b/src/renderer/src/lib/codex-session-restart.ts index ba380c2250f..ddde138f48b 100644 --- a/src/renderer/src/lib/codex-session-restart.ts +++ b/src/renderer/src/lib/codex-session-restart.ts @@ -2,6 +2,7 @@ import type { AppState } from '@/store' import { useAppStore } from '@/store' import { inspectRuntimeTerminalProcess } from '@/runtime/runtime-terminal-inspection' import { translate } from '@/i18n/i18n' +import { isCodexRestartEligiblePane } from './codex-pane-restart-eligibility' // Why: prompt integrations such as Starship can outlast the daemon's 300ms // Codex fast-path timeout; account restarts must wait until the shell accepts input. @@ -10,52 +11,54 @@ export const CODEX_ACCOUNT_RESTART_STARTUP = { startupCommandDelivery: 'shell-ready' } as const -function normalizeProcessName(processName: string | null): string | null { - if (!processName) { - return null - } - return processName.toLowerCase().replace(/\.exe$/, '') +export type CodexPaneScanResult = { + ptyId: string + /** The pane may be shown a restart prompt (see isCodexRestartEligiblePane). */ + eligible: boolean + /** Inspection failed or the handle was stale, so a later read may answer differently. */ + inconclusive: boolean + /** Orca launched Codex in this tab, so a shell foreground can still be reattach settle. */ + launchedCodex: boolean + /** A restart notice was raised for this pane by this scan. */ + notified: boolean } -function isCodexForegroundProcess(processName: string | null): boolean { - const normalized = normalizeProcessName(processName) - if (!normalized) { - return false - } - // Why: node-pty exposes the OS foreground process name, which can be the - // shipped Codex binary name (for example "codex-aarch64-ap" on macOS) - // instead of the shell command the user typed. Match on a Codex prefix so - // account-switch restart prompts still appear for real Codex sessions. - return normalized === 'codex' || normalized.startsWith('codex-') -} - -async function getLiveCodexSessionPtyIds(state: AppState): Promise { +async function scanCodexPanes( + state: AppState, + ptyIdFilter: ReadonlySet | null +): Promise { const tabs = Object.values(state.tabsByWorktree).flat() - const checks = await Promise.all( + const scans = await Promise.all( tabs.map(async (tab) => { - const ptyIds = state.ptyIdsByTabId[tab.id] ?? [] - if (ptyIds.length === 0) { - return [] as string[] - } - + const ptyIds = (state.ptyIdsByTabId[tab.id] ?? []).filter( + (ptyId) => ptyIdFilter === null || ptyIdFilter.has(ptyId) + ) // Why: Codex sessions are not reliably discoverable from tab labels. // Tabs keep fallback names until a CLI emits an OSC title, and Codex - // does not always do that. The foreground PTY process is the stable - // source of truth for whether this live tab is actually running Codex. - const foregroundProcesses = await Promise.all( - ptyIds.map((ptyId) => - inspectRuntimeTerminalProcess(state.settings, ptyId).then( - (inspection) => inspection.foregroundProcess, + // does not always do that. The live process tree plus the tab's recorded + // launchAgent are the stable evidence that this pane is running Codex. + return Promise.all( + ptyIds.map(async (ptyId) => { + const inspection = await inspectRuntimeTerminalProcess(state.settings, ptyId).then( + (result) => result, // Why: one stale remote pane must not hide restart notices for other confirmed Codex panes. () => null ) - ) + return { + ptyId, + eligible: + inspection !== null && + isCodexRestartEligiblePane({ inspection, launchAgent: tab.launchAgent }), + inconclusive: inspection === null || inspection.unavailable === true, + launchedCodex: tab.launchAgent === 'codex', + notified: false + } + }) ) - return ptyIds.filter((_, index) => isCodexForegroundProcess(foregroundProcesses[index])) }) ) - return checks.flat() + return scans.flat() } export async function markLiveCodexSessionsForRestart(args: { @@ -63,7 +66,8 @@ export async function markLiveCodexSessionsForRestart(args: { nextAccountLabel: string }): Promise { const state = useAppStore.getState() - const liveCodexSessionPtyIds = await getLiveCodexSessionPtyIds(state) + const scans = await scanCodexPanes(state, null) + const liveCodexSessionPtyIds = scans.filter((scan) => scan.eligible).map((scan) => scan.ptyId) if (liveCodexSessionPtyIds.length === 0) { return } @@ -84,18 +88,24 @@ export async function markLiveCodexSessionsForRestart(args: { * the PTY daemon and survive a full app restart with the old account still * baked into their environment. Without this, quitting Orca before restarting a * stale pane silently strands it on the previous account forever. + * + * Returns one result per inspected pane so the bind-driven sweep can tell an + * answered pane from one whose PTY has not reported a usable process yet. */ -export async function markRestoredStaleCodexSessionsForRestart(): Promise { +export async function markRestoredStaleCodexSessionsForRestart(args?: { + ptyIds?: readonly string[] +}): Promise { const state = useAppStore.getState() - const liveCodexSessionPtyIds = await getLiveCodexSessionPtyIds(state) + const scans = await scanCodexPanes(state, args?.ptyIds ? new Set(args.ptyIds) : null) + const liveCodexSessionPtyIds = scans.filter((scan) => scan.eligible).map((scan) => scan.ptyId) if (liveCodexSessionPtyIds.length === 0) { - return + return scans } const stalePanes = await window.api.codexAccounts.listStalePanes({ ptyIds: liveCodexSessionPtyIds }) if (stalePanes.length === 0) { - return + return scans } const resolveAccountLabel = await createCodexAccountLabelResolver() @@ -106,6 +116,8 @@ export async function markRestoredStaleCodexSessionsForRestart(): Promise nextAccountLabel: resolveAccountLabel(pane.activeAccountId) })) ) + const notifiedPtyIds = new Set(stalePanes.map((pane) => pane.ptyId)) + return scans.map((scan) => (notifiedPtyIds.has(scan.ptyId) ? { ...scan, notified: true } : scan)) } async function createCodexAccountLabelResolver(): Promise<(accountId: string | null) => string> { diff --git a/src/renderer/src/lib/codex-stale-pane-sweep.test.ts b/src/renderer/src/lib/codex-stale-pane-sweep.test.ts new file mode 100644 index 00000000000..6cc6ef354a8 --- /dev/null +++ b/src/renderer/src/lib/codex-stale-pane-sweep.test.ts @@ -0,0 +1,324 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useAppStore } from '@/store' +import { + notifyCodexPaneBoundForStaleSweep, + resetCodexStalePaneSweepForTests +} from './codex-stale-pane-sweep' + +const ACCOUNT_A = 'account-a@example.com' +const ACCOUNT_B = 'account-b@example.com' + +const STALE_PANE = { + ptyId: 'pty-1', + launchAccountId: 'account-a', + activeAccountId: 'account-b' +} + +function inspectCallCountFor(ptyId: string): number { + return vi.mocked(window.api.pty.inspectProcess).mock.calls.filter(([id]) => id === ptyId).length +} + +describe('notifyCodexPaneBoundForStaleSweep', () => { + const originalWindow = (globalThis as { window?: typeof window }).window + + beforeEach(() => { + vi.useFakeTimers() + resetCodexStalePaneSweepForTests() + useAppStore.setState({ + tabsByWorktree: { + wt1: [ + { + id: 'tab-1', + ptyId: 'pty-1', + worktreeId: 'wt1', + title: 'orca-1', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1, + launchAgent: 'codex' + } + ] + }, + ptyIdsByTabId: { 'tab-1': ['pty-1'] }, + pendingCodexPaneRestartIds: {}, + codexRestartNoticeByPtyId: {} + }) + ;(globalThis as { window: typeof window }).window = { + ...originalWindow, + api: { + ...originalWindow?.api, + pty: { + ...originalWindow?.api?.pty, + inspectProcess: vi + .fn() + .mockResolvedValue({ foregroundProcess: 'codex', hasChildProcesses: false }) + }, + codexAccounts: { + ...originalWindow?.api?.codexAccounts, + list: vi.fn().mockResolvedValue({ + accounts: [ + { id: 'account-a', email: ACCOUNT_A }, + { id: 'account-b', email: ACCOUNT_B } + ], + activeAccountId: 'account-b' + }), + listStalePanes: vi.fn().mockResolvedValue([]) + } + } + } as unknown as typeof window + }) + + afterEach(() => { + resetCodexStalePaneSweepForTests() + vi.useRealTimers() + if (originalWindow) { + ;(globalThis as { window: typeof window }).window = originalWindow + } else { + delete (globalThis as { window?: typeof window }).window + } + }) + + it('raises the prompt once the pane PTY actually binds', async () => { + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + + notifyCodexPaneBoundForStaleSweep('pty-1') + // Nothing inspected yet: the bind has not settled. + expect(window.api.pty.inspectProcess).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(300) + + expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-1'] }) + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + }) + + it('coalesces a burst of startup binds into one sweep', async () => { + useAppStore.setState({ ptyIdsByTabId: { 'tab-1': ['pty-1', 'pty-2'] } }) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + + notifyCodexPaneBoundForStaleSweep('pty-1') + notifyCodexPaneBoundForStaleSweep('pty-2') + await vi.advanceTimersByTimeAsync(300) + + expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledExactlyOnceWith({ + ptyIds: ['pty-1', 'pty-2'] + }) + }) + + it('still coalesces a burst whose binds land milliseconds apart', async () => { + // Real startup binds are staggered, so per-PTY due times must not turn one + // sweep into one registry round-trip per pane. + useAppStore.setState({ ptyIdsByTabId: { 'tab-1': ['pty-1', 'pty-2'] } }) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(10) + notifyCodexPaneBoundForStaleSweep('pty-2') + await vi.advanceTimersByTimeAsync(290) + + expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledExactlyOnceWith({ + ptyIds: ['pty-1', 'pty-2'] + }) + }) + + it('retries a PTY whose process read is unusable until the reattach settles', async () => { + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + vi.mocked(window.api.pty.inspectProcess).mockRejectedValueOnce(new Error('terminal_gone')) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(300) + expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + + await vi.advanceTimersByTimeAsync(1500) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + }) + + it('gives up after a bounded ladder instead of polling forever', async () => { + vi.mocked(window.api.pty.inspectProcess).mockRejectedValue(new Error('terminal_gone')) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(60_000) + + expect(window.api.pty.inspectProcess).toHaveBeenCalledTimes(3) + }) + + it('spends a pane its full retry ladder even when another pane binds later', async () => { + // Regression: one shared timer used to drain every queued PTY, so a pane + // already waiting on its 1500ms rung had the next rung consumed by the newer + // pane's shorter delay — exhausting its 3 attempts ~2.5s early and dropping + // a Windows daemon reattach that had not settled yet. + useAppStore.setState({ ptyIdsByTabId: { 'tab-1': ['pty-1', 'pty-2'] } }) + vi.mocked(window.api.pty.inspectProcess).mockRejectedValue(new Error('terminal_gone')) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(310) + notifyCodexPaneBoundForStaleSweep('pty-2') + + // t = 3310: past pty-1's second look (1800), well before its third (5800). + await vi.advanceTimersByTimeAsync(3_000) + expect(inspectCallCountFor('pty-1')).toBe(2) + + await vi.advanceTimersByTimeAsync(3_000) + expect(inspectCallCountFor('pty-1')).toBe(3) + }) + + it('does not park a fresh bind behind a pending retry wait', async () => { + useAppStore.setState({ ptyIdsByTabId: { 'tab-1': ['pty-1', 'pty-2'] } }) + vi.mocked(window.api.pty.inspectProcess).mockRejectedValue(new Error('terminal_gone')) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(310) + notifyCodexPaneBoundForStaleSweep('pty-2') + await vi.advanceTimersByTimeAsync(300) + + // The pending timer was pty-1's 1500ms rung; pty-2 must not inherit that wait. + expect(inspectCallCountFor('pty-2')).toBe(1) + }) + + it('retries a pane whose sweep threw instead of dropping it', async () => { + // Why: listStalePanes reads the on-disk pane-account registry and is the one + // call in the sweep with no catch of its own, so a transient rejection must + // spend a rung like any other unusable read. + vi.mocked(window.api.codexAccounts.listStalePanes) + .mockRejectedValueOnce(new Error('registry unreadable')) + .mockResolvedValue([STALE_PANE]) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(300) + expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + + await vi.advanceTimersByTimeAsync(1500) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + }) + + it('still fires a pane waiting on a later rung when an earlier sweep throws', async () => { + // Regression: the failing sweep took its own PTYs out of the queue and + // returned without re-aiming the timer, so pty-1 — parked on its 1500ms rung + // and never touched by that sweep — was left with nothing to fire it, ever. + useAppStore.setState({ ptyIdsByTabId: { 'tab-1': ['pty-1', 'pty-2'] } }) + vi.mocked(window.api.pty.inspectProcess).mockRejectedValueOnce(new Error('terminal_gone')) + // Why: keyed on the swept PTY rather than call order, so the rejection stays + // bound to pty-2's sweep even if the flush sequence changes. + vi.mocked(window.api.codexAccounts.listStalePanes).mockImplementation(async ({ ptyIds }) => { + if (ptyIds.includes('pty-2')) { + throw new Error('registry unreadable') + } + return [STALE_PANE] + }) + + notifyCodexPaneBoundForStaleSweep('pty-1') + // t = 300: pty-1 reads inconclusive and parks on its 1500ms rung (due 1800). + await vi.advanceTimersByTimeAsync(300) + notifyCodexPaneBoundForStaleSweep('pty-2') + // t = 600: pty-2's sweep is the one that throws; pty-1 is not due and untouched. + await vi.advanceTimersByTimeAsync(300) + // Why: assert the throw really landed on a sweep pty-1 was absent from, or a + // later change to the flush sequence could leave this passing while the + // stranding it exists to catch is no longer being set up at all. + expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-2'] }) + expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + + await vi.advanceTimersByTimeAsync(1200) + + expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-1'] }) + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + }) + + it('stops retrying a pane the registry reports as not stale', async () => { + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(60_000) + + expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledTimes(1) + }) + + it('does not re-prompt a pane that already got its notice', async () => { + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(300) + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(60_000) + + expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledTimes(1) + }) + + it('never marks a plain shell pane, so its input is never blocked', async () => { + useAppStore.setState({ + tabsByWorktree: { + wt1: [ + { + id: 'tab-1', + ptyId: 'pty-1', + worktreeId: 'wt1', + title: 'orca-1', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } + ] + } + }) + vi.mocked(window.api.pty.inspectProcess).mockResolvedValue({ + foregroundProcess: 'zsh', + hasChildProcesses: false + }) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(60_000) + + expect(window.api.codexAccounts.listStalePanes).not.toHaveBeenCalled() + expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + }) + + it('retries a Codex tab still showing its shell, then prompts when Codex is up', async () => { + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + vi.mocked(window.api.pty.inspectProcess).mockResolvedValueOnce({ + foregroundProcess: 'pwsh.exe', + hasChildProcesses: false + }) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(300) + expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + + await vi.advanceTimersByTimeAsync(1500) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + }) + + it('retries a PTY the store has not yet listed against its tab', async () => { + useAppStore.setState({ ptyIdsByTabId: {} }) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(300) + expect(window.api.pty.inspectProcess).not.toHaveBeenCalled() + + useAppStore.setState({ ptyIdsByTabId: { 'tab-1': ['pty-1'] } }) + await vi.advanceTimersByTimeAsync(1500) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + }) +}) diff --git a/src/renderer/src/lib/codex-stale-pane-sweep.ts b/src/renderer/src/lib/codex-stale-pane-sweep.ts new file mode 100644 index 00000000000..62cd050e888 --- /dev/null +++ b/src/renderer/src/lib/codex-stale-pane-sweep.ts @@ -0,0 +1,161 @@ +import { + markRestoredStaleCodexSessionsForRestart, + type CodexPaneScanResult +} from './codex-session-restart' + +// Why: the first delay coalesces the startup burst of binds and lets +// updateTabPtyId (written just after the layout binding) land, since the scan +// walks tabs. The later two cover a reattached daemon shell that answers +// `inspectProcess` with terminal_gone for a beat. Bounded on purpose — this is +// a hint, and a pane that never resolves must not become a polling loop. +const SWEEP_ATTEMPT_DELAYS_MS = [300, 1500, 4000] as const + +// Why: each PTY owns its rung, so the queue has to carry a per-PTY due time. One +// shared timer coalesces the startup burst, but it must always target the +// EARLIEST due entry — a single timer that drained everything let one pane's +// short delay consume another's later rung and cut its ladder short. +const dueAtByPtyId = new Map() +const attemptsByPtyId = new Map() +const notifiedPtyIds = new Set() +let flushTimer: ReturnType | null = null +let flushTimerDueAt: number | null = null + +/** + * Queues a stale-account check for a PTY that has just bound to a pane. + * + * Why a bind signal rather than a startup call: nothing is attached while the + * session hydrates, so a one-shot sweep inspects zero PTYs and never retries. + * Every real bind rewrites the pane→PTY layout binding, which is the earliest + * point the daemon shell can be inspected at all. + */ +export function notifyCodexPaneBoundForStaleSweep(ptyId: string): void { + if (notifiedPtyIds.has(ptyId)) { + return + } + queue(ptyId, SWEEP_ATTEMPT_DELAYS_MS[0]) + armForEarliestDue() +} + +export function resetCodexStalePaneSweepForTests(): void { + if (flushTimer !== null) { + clearTimeout(flushTimer) + flushTimer = null + } + flushTimerDueAt = null + dueAtByPtyId.clear() + attemptsByPtyId.clear() + notifiedPtyIds.clear() +} + +function queue(ptyId: string, delayMs: number): void { + const dueAt = Date.now() + delayMs + const existing = dueAtByPtyId.get(ptyId) + // Why: a rebind of an already-queued PTY must never push its look later. + dueAtByPtyId.set(ptyId, existing === undefined ? dueAt : Math.min(existing, dueAt)) +} + +function armForEarliestDue(): void { + let earliestDueAt: number | null = null + for (const dueAt of dueAtByPtyId.values()) { + if (earliestDueAt === null || dueAt < earliestDueAt) { + earliestDueAt = dueAt + } + } + if (earliestDueAt === null) { + return + } + if (flushTimer !== null) { + if (flushTimerDueAt !== null && flushTimerDueAt <= earliestDueAt) { + return + } + clearTimeout(flushTimer) + } + flushTimerDueAt = earliestDueAt + flushTimer = setTimeout( + () => { + flushTimer = null + flushTimerDueAt = null + void flush() + }, + Math.max(0, earliestDueAt - Date.now()) + ) +} + +function takeDuePtyIds(): string[] { + const now = Date.now() + const duePtyIds: string[] = [] + for (const [ptyId, dueAt] of dueAtByPtyId) { + // Why: folding a never-inspected PTY into an earlier sweep is what coalesces + // the startup burst, and costs it nothing. A PTY already waiting on a retry + // rung must wait for its own due time, or that rung is spent for free. + if (dueAt <= now || !attemptsByPtyId.has(ptyId)) { + duePtyIds.push(ptyId) + } + } + for (const ptyId of duePtyIds) { + dueAtByPtyId.delete(ptyId) + } + return duePtyIds +} + +function shouldRetry(scan: CodexPaneScanResult): boolean { + // Why: an eligible-but-unlisted pane got an authoritative "not stale" from the + // registry, so only an unusable read or a Codex tab still showing its shell + // (mid-reattach) can change answer on a later attempt. + return !scan.eligible && (scan.inconclusive || scan.launchedCodex) +} + +function queueNextRung(ptyId: string): void { + const attempt = (attemptsByPtyId.get(ptyId) ?? 0) + 1 + const delayMs = SWEEP_ATTEMPT_DELAYS_MS[attempt] + if (delayMs === undefined) { + attemptsByPtyId.delete(ptyId) + return + } + attemptsByPtyId.set(ptyId, attempt) + queue(ptyId, delayMs) +} + +async function flush(): Promise { + const ptyIds = takeDuePtyIds() + if (ptyIds.length === 0) { + // Why: a timer can fire a hair early; re-aim it rather than dropping the queue. + armForEarliestDue() + return + } + + let scans: CodexPaneScanResult[] + try { + scans = await markRestoredStaleCodexSessionsForRestart({ ptyIds }) + } catch (err) { + console.warn('Codex stale-pane restart sweep failed:', err) + // Why: a thrown sweep is no more conclusive than an unusable process read, so + // spend a rung rather than dropping these panes. Re-aiming is what keeps the + // panes this sweep never touched — parked on a later rung, and no longer + // holding the timer — from being stranded with nothing left to fire them. + for (const ptyId of ptyIds) { + queueNextRung(ptyId) + } + armForEarliestDue() + return + } + + const scanByPtyId = new Map(scans.map((scan) => [scan.ptyId, scan])) + for (const ptyId of ptyIds) { + const scan = scanByPtyId.get(ptyId) + if (scan?.notified === true) { + notifiedPtyIds.add(ptyId) + attemptsByPtyId.delete(ptyId) + continue + } + // Why: a PTY the scan never saw is not yet listed against its tab, which is + // the same "ask again shortly" case as an unusable process read. + if (scan !== undefined && !shouldRetry(scan)) { + attemptsByPtyId.delete(ptyId) + continue + } + queueNextRung(ptyId) + } + + armForEarliestDue() +}