mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 16:03:26 +00:00
fix(codex): widen the stale-pane sweep retry ladder past the Windows worst case (#10853)
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.
This commit is contained in:
@@ -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'))
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user