From c460de6089fbf38f11ede55e109371cad109e33d Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:05:15 -0700 Subject: [PATCH] fix(codex): widen the stale-pane sweep retry ladder past the Windows worst case (#10853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live Windows 11 validation of #10770 measured store-visible PTY bind -> notice raised at 3,959 / 4,064 / 5,140 ms across three runs. The ladder delays are CUMULATIVE (queueNextRung -> queue -> Date.now() + delayMs), so the old [300, 1500, 4000] fired at 300ms / 1.8s / 5.8s from bind — leaving the worst run roughly 660ms of margin against the final rung. A slightly slower machine, more panes, or a colder daemon exhausts the ladder and the card NEVER appears, silently reintroducing the bug #10770 exists to fix. macOS by comparison resolved in ~0.66s with most of the ladder unused. Widened to [300, 1500, 4000, 10_000, 20_000] — cumulative 300ms / 1.8s / 5.8s / 15.8s / 35.8s. Early rungs untouched so macOS still resolves under a second; the tail clears the Windows worst case by 2.7x and 6.2x. Still bounded at five attempts, not interval-based; queue/takeDuePtyIds are unchanged so per-PTY rung independence does not regress, and the notified/dismissed/closed-pane guards were verified to stop late rungs. Failure mode if the ladder is still short is "no card" — never a dead keyboard. Review verified the cumulative arithmetic by measurement rather than reading (both the author and I initially misread the delays as absolute-from-bind) and confirmed boundedness deterministically 10/10. It also hardened the bounded-ladder test: it previously asserted only "5 inspects within 120s", which a 6th rung at 200s passed unchanged — the test named for termination did not pin termination. Now asserts vi.getTimerCount() === 0, mutation-proved. Not verified: the Windows field numbers behind the 10s/20s sizing have not been re-measured on Windows with this change. Known follow-up, out of scope: remote SSH panes can never be reported stale (they are never written to the pane-account registry) yet still ride all five rungs at 15s RPC timeouts, so this change raises wasted remote RPC from ~45s to ~75s per pane. The sweep and the switch-time marking should both skip remote panes for a host-account change. --- .../src/lib/codex-stale-pane-sweep.test.ts | 40 +++++++++++++++++-- .../src/lib/codex-stale-pane-sweep.ts | 12 ++++-- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/renderer/src/lib/codex-stale-pane-sweep.test.ts b/src/renderer/src/lib/codex-stale-pane-sweep.test.ts index 6cc6ef354a8..5448315e3ac 100644 --- a/src/renderer/src/lib/codex-stale-pane-sweep.test.ts +++ b/src/renderer/src/lib/codex-stale-pane-sweep.test.ts @@ -144,16 +144,48 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { vi.mocked(window.api.pty.inspectProcess).mockRejectedValue(new Error('terminal_gone')) notifyCodexPaneBoundForStaleSweep('pty-1') - await vi.advanceTimersByTimeAsync(60_000) + // Well past the last rung (35.8s), so this pins the ceiling, not the clock. + await vi.advanceTimersByTimeAsync(120_000) - expect(window.api.pty.inspectProcess).toHaveBeenCalledTimes(3) + expect(window.api.pty.inspectProcess).toHaveBeenCalledTimes(5) + // Why: the count alone only bounds the first two minutes — a rung past the + // window would still read as 5. An empty queue is what proves the ladder + // ended rather than merely moved out of view. + expect(vi.getTimerCount()).toBe(0) + }) + + it('still sweeps a pane whose reattach outlives the first three rungs', async () => { + // Regression: live Windows 11 runs raised the notice on the third rung + // (5.8s) in all three samples, i.e. the ladder ended exactly where the + // slowest measured box landed. A colder reattach falls past it, and the + // failure is silent — no card, so the pane keeps running the old account. + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + vi.mocked(window.api.pty.inspectProcess) + .mockRejectedValueOnce(new Error('terminal_gone')) + .mockRejectedValueOnce(new Error('terminal_gone')) + .mockRejectedValueOnce(new Error('terminal_gone')) + + notifyCodexPaneBoundForStaleSweep('pty-1') + // t = 5800: rungs 1-3 are spent and nothing has been raised. + await vi.advanceTimersByTimeAsync(5_800) + expect(inspectCallCountFor('pty-1')).toBe(3) + expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + + // t = 15800: the fourth rung is the one that catches this pane. + await vi.advanceTimersByTimeAsync(10_000) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: ACCOUNT_A, + nextAccountLabel: ACCOUNT_B + }) + expect(inspectCallCountFor('pty-1')).toBe(4) }) 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. + // pane's shorter delay — burning through its ladder 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')) diff --git a/src/renderer/src/lib/codex-stale-pane-sweep.ts b/src/renderer/src/lib/codex-stale-pane-sweep.ts index 62cd050e888..178334643ab 100644 --- a/src/renderer/src/lib/codex-stale-pane-sweep.ts +++ b/src/renderer/src/lib/codex-stale-pane-sweep.ts @@ -5,10 +5,14 @@ import { // 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 +// walks tabs. The rest cover a reattached daemon shell that answers +// `inspectProcess` with terminal_gone for a beat. The tail is sized off live +// Windows 11 runs, where the 5.8s rung is what raised the notice in all three +// (macOS resolved on the first): the ladder used to end exactly where the +// slowest measured box landed, so a colder reattach fell off it and the pane +// stayed silently stale. 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, 10_000, 20_000] 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