diff --git a/apps/vscode/src/__tests__/terminal-adapter.test.ts b/apps/vscode/src/__tests__/terminal-adapter.test.ts index c4d3e0db2..d1103943c 100644 --- a/apps/vscode/src/__tests__/terminal-adapter.test.ts +++ b/apps/vscode/src/__tests__/terminal-adapter.test.ts @@ -320,7 +320,7 @@ describe('PIR #1001 — reconnect notices overwrite in place and clear on succes expect(writes).not.toContain(ERASE_LINE); }); - it('give-up overwrites the last retry notice but is itself never wiped', () => { + it('give-up overwrites the last retry notice and owns the line, then is wiped on recovery (#1681)', () => { const { writes } = makeAdapter(); for (let i = 0; i < 6; i++) { currentSocket().emit('close'); vi.advanceTimersByTime(30000); } writes.length = 0; @@ -331,12 +331,14 @@ describe('PIR #1001 — reconnect notices overwrite in place and clear on succes expect(giveUp.startsWith(ERASE_LINE)).toBe(true); // overwrote the last retry notice expect(giveUp).toContain(RECONNECT_LINK_TEXT); expect(giveUp).toContain('\x1b[31m'); // red terminal-failure state + // The banner owns the current line (no trailing newline), so a successful + // reconnect can erase it in place rather than leaving a half-overwritten + // remnant on the recovered composer line (#1681 recovery-render fix). + expect(giveUp.endsWith('\r\n')).toBe(false); - // A later reconnect must NOT wipe the give-up line: gaveUp blocks the loop, - // and the give-up cleared hadReconnectNotice. Manually re-open to be sure. writes.length = 0; - currentSocket().emit('open'); - expect(writes).not.toContain(ERASE_LINE); + currentSocket().emit('open'); // successful recovery wipes the stale banner + expect(writes).toContain(ERASE_LINE); }); it('immediate 4xx give-up has no erase prefix (no retry notice to overwrite)', () => { @@ -598,3 +600,209 @@ describe('PIR #1052 — buffer replay and flush at the settled size', () => { }); }); +// ── #1681: wake re-arm + honest give-up banner ─────────────────────────────── + +/** Flush the microtask queue so an async give-up banner (probe-gated) settles. + * Independent of fake timers — the probe is a resolved promise, not a timer. */ +async function flushMicrotasks(): Promise { + for (let i = 0; i < 5; i++) { await Promise.resolve(); } +} + +type WakeablePty = { + open(d: unknown): void; + onWake(): void; + reconnect(): void; + onDidWrite(cb: (s: string) => void): void; +}; + +/** Build an adapter with an optional injected `/health` probe (#1681), exposing + * the `onWake` re-arm entry point. */ +function makeAdapterWithProbe(probe?: () => Promise) { + const writes: string[] = []; + const pty = new (CodevPseudoterminal as unknown as new ( + url: string, authKey: string | null, ch: unknown, probeHealth?: () => Promise, + ) => WakeablePty)('ws://localhost:4100/x', null, fakeOutputChannel(), probe); + pty.onDidWrite((s: string) => { if (s) { writes.push(s); } }); + pty.open(undefined); + return { pty, writes }; +} + +/** Burn the full 6-attempt budget so the next close exhausts it. */ +function burnBudget(): void { + for (let i = 0; i < 6; i++) { + currentSocket().emit('close'); + vi.advanceTimersByTime(30000); + } +} + +describe('#1681 — wake signal re-arms the reconnect budget', () => { + it('reconnects a transiently gave-up adapter on wake (the slept-laptop case)', () => { + const { pty } = makeAdapterWithProbe(); + burnBudget(); + currentSocket().emit('close'); // 7th close → transient give-up + const socketsBefore = WebSocket.instances.length; + + pty.onWake(); // window refocus after wake + + // A fresh socket is opened immediately (budget re-armed), and its open + // clears the loop for a full fresh retry chain. + expect(WebSocket.instances.length).toBe(socketsBefore + 1); + currentSocket().readyState = WebSocket.OPEN; + currentSocket().emit('open'); + }); + + it('does NOT resurrect a permanent (4xx session-gone) give-up on wake (#936 guard)', () => { + const { pty } = makeAdapterWithProbe(); + currentSocket().emit('error', new Error('Unexpected server response: 404')); + currentSocket().emit('close'); // permanent give-up + const socketsBefore = WebSocket.instances.length; + + pty.onWake(); + + // The session is gone on Tower — retrying is hopeless, so wake is a no-op. + expect(WebSocket.instances.length).toBe(socketsBefore); + }); + + it('skips the parked backoff wait: wake reconnects immediately mid-chain', () => { + const { pty } = makeAdapterWithProbe(); + currentSocket().emit('close'); // schedules a 1s retry; adapter is parked + const socketsBefore = WebSocket.instances.length; + + pty.onWake(); // fire before the 1s timer elapses + + // Reconnected now, without waiting out the backoff delay. + expect(WebSocket.instances.length).toBe(socketsBefore + 1); + }); + + it('no-ops on a healthy OPEN connection (never drops a live terminal)', () => { + const { pty } = makeAdapterWithProbe(); + currentSocket().readyState = WebSocket.OPEN; + currentSocket().emit('open'); + const socketsBefore = WebSocket.instances.length; + + pty.onWake(); + + expect(WebSocket.instances.length).toBe(socketsBefore); + }); + + it('wipes the stale give-up banner when a wake reconnects (recovery-render)', () => { + // The user-reported glitch: after auto-recovery a half-overwritten banner + // remnant ("…k here to reconnect]") strands on the composer line until the + // next keystroke. The banner must be erased in place on the successful + // reconnect, before the replay paints. + const { pty, writes } = makeAdapterWithProbe(); // no probe → synchronous banner + burnBudget(); + currentSocket().emit('close'); // transient give-up banner owns the line + pty.onWake(); // wake → fresh socket + writes.length = 0; + + currentSocket().readyState = WebSocket.OPEN; + currentSocket().emit('open'); // successful recovery + expect(writes).toContain('\r\x1b[2K'); // banner erased in place, no remnant + }); + + it('no-ops before VS Code has open()ed the pty (no orphan socket)', () => { + // A wake can land between terminal-manager registering the pty and VS Code + // calling open(); re-arming then would leak a socket open()'s connect() + // replaces without closing. + const pty = new (CodevPseudoterminal as unknown as new ( + url: string, authKey: string | null, ch: unknown, probeHealth?: () => Promise, + ) => WakeablePty)('ws://localhost:4100/x', null, fakeOutputChannel()); + const socketsBefore = WebSocket.instances.length; // no open() yet + + pty.onWake(); + + expect(WebSocket.instances.length).toBe(socketsBefore); + }); +}); + +describe('#1681 — exhausted-budget banner is worded honestly via /health', () => { + it('says "Tower is up" when the probe reports Tower reachable', async () => { + const { writes } = makeAdapterWithProbe(async () => true); + burnBudget(); + writes.length = 0; + currentSocket().emit('close'); // exhaust → probe-gated banner + await flushMicrotasks(); + + const banner = writes.find((w) => w.includes(RECONNECT_LINK_TEXT)); + expect(banner).toBeDefined(); + expect(banner).toContain('reconnect failed (Tower is up)'); + expect(banner).toContain('\x1b[31m'); // still the red failure notice + }); + + it('falls back to attempt-count wording when the probe blackholes past the cap', async () => { + // A never-resolving probe (blackholed network) must not hold the banner for + // the SDK's 10s request window — the 2s race resolves it to the fallback. + const { writes } = makeAdapterWithProbe(() => new Promise(() => {})); + burnBudget(); + writes.length = 0; + currentSocket().emit('close'); + await vi.advanceTimersByTimeAsync(2000); // trip the probe-timeout race + + const banner = writes.find((w) => w.includes(RECONNECT_LINK_TEXT)); + expect(banner).toBeDefined(); + expect(banner).toContain('unable to reconnect after 6 attempts'); + }); + + it('says "Tower unreachable" when the probe reports Tower down', async () => { + const { writes } = makeAdapterWithProbe(async () => false); + burnBudget(); + writes.length = 0; + currentSocket().emit('close'); + await flushMicrotasks(); + + const banner = writes.find((w) => w.includes(RECONNECT_LINK_TEXT)); + expect(banner).toBeDefined(); + expect(banner).toContain('Tower unreachable'); + }); + + it('falls back to attempt-count wording when the probe returns null (reachability unknown)', async () => { + const { writes } = makeAdapterWithProbe(async () => null); + burnBudget(); + writes.length = 0; + currentSocket().emit('close'); + await flushMicrotasks(); + + const banner = writes.find((w) => w.includes(RECONNECT_LINK_TEXT)); + expect(banner).toBeDefined(); + expect(banner).toContain('unable to reconnect after 6 attempts'); + expect(banner).not.toContain('Tower unreachable'); + }); + + it('falls back to the plain attempt-count wording when no probe is injected', () => { + const { writes } = makeAdapterWithProbe(undefined); + burnBudget(); + writes.length = 0; + currentSocket().emit('close'); // no probe → synchronous banner, old wording + + const banner = writes.find((w) => w.includes(RECONNECT_LINK_TEXT)); + expect(banner).toBeDefined(); + expect(banner).toContain('unable to reconnect after 6 attempts'); + }); + + it('a slow probe never overwrites a permanent give-up that lands mid-probe', async () => { + // The give-up-token guard: exhaust the budget (transient), start the probe, + // then let a wake reconnect hit a permanent 4xx before the probe resolves. + let resolveProbe: (up: boolean) => void = () => {}; + const probe = () => new Promise((r) => { resolveProbe = r; }); + const { pty, writes } = makeAdapterWithProbe(probe); + burnBudget(); + currentSocket().emit('close'); // transient give-up; /health probe now pending + + pty.onWake(); // wake re-arms → fresh socket + currentSocket().emit('error', new Error('Unexpected server response: 404')); + currentSocket().emit('close'); // the reconnect is a permanent 4xx + const permanentBanner = writes.find((w) => w.includes('no longer exists')); + expect(permanentBanner).toBeDefined(); + + writes.length = 0; + resolveProbe(true); // the stale transient probe finally resolves + await flushMicrotasks(); + + // The now-stale exhausted-budget banner is suppressed — the permanent state + // stands, uncontradicted. + expect(writes.some((w) => w.includes('Tower is up'))).toBe(false); + expect(writes.some((w) => w.includes('unable to reconnect'))).toBe(false); + }); +}); + diff --git a/apps/vscode/src/__tests__/terminal-manager.test.ts b/apps/vscode/src/__tests__/terminal-manager.test.ts index 50b6a3e15..a259c1ee5 100644 --- a/apps/vscode/src/__tests__/terminal-manager.test.ts +++ b/apps/vscode/src/__tests__/terminal-manager.test.ts @@ -202,6 +202,39 @@ describe('#1180 — terminal cap is a configurable setting, not a static constan }); }); +describe('#1681 — re-arm terminal reconnects on wake', () => { + // Source-level guards (heavy TerminalManager / extension harness avoided, per + // this file's rationale): the wake path is thin glue. Its behavior is unit- + // tested at the adapter (terminal-adapter.test.ts: onWake, the give-up split, + // the honest banner) and confirmed by a physical sleep→wake round-trip. These + // guards pin the wiring so deleting the manager fan-out, the /health-probe + // injection, or the extension focus-handler call is caught (CMAP #1682). + const rearmBody = TM_SRC.split('rearmAllOnWake(): void')[1]?.split('buildWsUrl')[0] ?? ''; + const EXT_SRC = readFileSync(resolve(__dirname, '../extension.ts'), 'utf8'); + + it('fans onWake out to every managed terminal', () => { + expect(TM_SRC).toMatch(/rearmAllOnWake\(\): void/); + expect(rearmBody).toMatch(/for \(const entry of this\.terminals\.values\(\)\)/); + expect(rearmBody).toMatch(/entry\.pty\.onWake\(\)/); + }); + + it('injects the /health probe into every adapter it constructs', () => { + expect(TM_SRC).toMatch(/new CodevPseudoterminal\(/); + expect(TM_SRC).toMatch(/\(\) => this\.probeTowerHealth\(\)/); + expect(TM_SRC).toMatch(/private async probeTowerHealth\(\): Promise/); + expect(TM_SRC).toMatch(/getClient\(\)[\s\S]*getHealth\(\)\)\s*!==\s*null/); + }); + + it('extension calls rearmAllOnWake on the window-focus rising edge', () => { + // The rising-edge block runs when `state.focused && !windowFocused`; the + // re-arm must fire there, unconditionally (not behind repaintOnRefocus). + const focusBlock = + EXT_SRC.split('state.focused && !windowFocused')[1] + ?.split('windowFocused = state.focused')[0] ?? ''; + expect(focusBlock).toMatch(/terminalManager\?\.rearmAllOnWake\(\)/); + }); +}); + describe('#1180 — package.json exposes codev.maxTerminals', () => { const PKG = JSON.parse( readFileSync(resolve(__dirname, '../../package.json'), 'utf8'), diff --git a/apps/vscode/src/extension.ts b/apps/vscode/src/extension.ts index c6cfc7f3b..564e4eba5 100644 --- a/apps/vscode/src/extension.ts +++ b/apps/vscode/src/extension.ts @@ -333,6 +333,14 @@ export async function activate(context: vscode.ExtensionContext) { context.subscriptions.push( vscode.window.onDidChangeWindowState((state) => { if (state.focused && !windowFocused) { + // Re-arm terminal reconnects on wake (#1681). A refocus is the + // extension host's wake signal (the Node host has no DOM + // online/visibility events); a slept laptop burns each adapter's + // six-attempt reconnect budget against the suspended network stack, + // so re-arm unconditionally here — onWake no-ops a healthy or + // permanently-gone connection, so it needs no opt-in the way the + // repaint below does. + terminalManager?.rearmAllOnWake(); const enabled = vscode.workspace .getConfiguration('codev') .get('terminal.repaintOnRefocus', false); diff --git a/apps/vscode/src/terminal-adapter.ts b/apps/vscode/src/terminal-adapter.ts index 9447a8798..5ace553a0 100644 --- a/apps/vscode/src/terminal-adapter.ts +++ b/apps/vscode/src/terminal-adapter.ts @@ -22,6 +22,14 @@ const REPLAY_SETTLE_MS = 150; // 4s, 8s, 16s, 30s) and surface a terminal failure state. const MAX_RECONNECT_ATTEMPTS = 6; +// Cap how long the exhausted-budget banner waits on the `/health` probe (#1681). +// TowerClient.request uses a 10s request timeout, and a blackholed network — the +// still-suspended stack this fix targets, or a remote Tower behind a dropped VPN +// — hangs the probe for that full window while a stale "retrying (6/6)" notice +// lingers and the clickable give-up banner never appears. Race the probe against +// this shorter bound and fall back to the plain attempt-count wording. +const HEALTH_PROBE_TIMEOUT_MS = 2000; + /** * The clickable token emitted in the give-up message. Shared with the terminal * link provider (#939) so the message text and the matcher cannot drift — @@ -63,6 +71,11 @@ export class CodevPseudoterminal implements vscode.Pseudoterminal { private queuedBytes = 0; private lastDropWarnAt = 0; private disposed = false; + // Set once VS Code has called open() (the first connect). A wake signal can + // arrive between terminal-manager registering this pty and VS Code opening it; + // re-arming then would connect() a socket that open()'s own connect() later + // replaces without closing, leaking it. onWake no-ops until this is true. + private opened = false; // Repaint-nudge state (#1047). A freshly-attached terminal can stay blank: // the app inside (e.g. Claude's full-screen TUI) only paints after a real @@ -83,6 +96,18 @@ export class CodevPseudoterminal implements vscode.Pseudoterminal { private readonly backoff = new BackoffController({ maxAttempts: MAX_RECONNECT_ATTEMPTS }); private reconnectTimer: ReturnType | null = null; private gaveUp = false; + // Why the adapter gave up, so a wake signal (onWake, #1681) can re-arm ONLY + // the transient (budget-exhausted) class and never resurrect the #936 + // permanent give-up — a 4xx from Tower meaning the session is gone, where + // retrying is hopeless and would revive the pre-June retry storm the budget + // was added to stop. null while the adapter has not given up. + private giveUpKind: 'transient' | 'permanent' | null = null; + // Bumped on every give-up-state transition (enter or clear). The async + // exhausted-budget banner (renderExhaustedGiveUp) captures this before it + // awaits the /health probe and renders only if it is unchanged — so a probe + // that outlives its give-up (a wake reconnects, or the reconnect then hits a + // permanent 4xx) can't paint a stale banner over the current state. + private giveUpToken = 0; // Tracks whether a wipeable in-progress retry notice currently occupies the // terminal's current line (#1001). Set when scheduleReconnect writes a notice; // cleared when a successful reconnect wipes it or the give-up state replaces @@ -94,6 +119,13 @@ export class CodevPseudoterminal implements vscode.Pseudoterminal { private wsUrl: string, private authKey: string | null, private outputChannel: vscode.OutputChannel, + // Optional one-shot Tower `/health` probe (#1681). When present, the + // exhausted-budget give-up words its banner honestly — "Tower unreachable" + // vs "reconnect failed (Tower is up)" — instead of the ambiguous + // attempt-count message. `null` means reachability is unknown (no client + // yet), which falls back to the neutral wording. Injected by + // terminal-manager; absent in the unit tests that don't exercise the split. + private probeHealth?: () => Promise, ) {} open(initialDimensions: vscode.TerminalDimensions | undefined): void { @@ -105,6 +137,7 @@ export class CodevPseudoterminal implements vscode.Pseudoterminal { if (initialDimensions) { this.lastDimensions = { cols: initialDimensions.columns, rows: initialDimensions.rows }; } + this.opened = true; this.connect(); } @@ -193,6 +226,8 @@ export class CodevPseudoterminal implements vscode.Pseudoterminal { // where the previous failure run left off. this.backoff.recordSuccess(); this.gaveUp = false; + this.giveUpKind = null; + this.giveUpToken++; // Wipe any in-progress retry notice before replayed buffer / normal // output resumes, so it doesn't orphan in scrollback (#1001). this.clearReconnectNotice(); @@ -246,7 +281,9 @@ export class CodevPseudoterminal implements vscode.Pseudoterminal { // an unknown session ID). The matching `close` fires right after; give up // now so the close handler's scheduleReconnect() is a no-op. if (this.ws === socket && classifyUpgradeError(err.message) === 'permanent') { - this.giveUp('this terminal session no longer exists on Tower'); + if (this.enterGiveUp('permanent')) { + this.renderGiveUpBanner('this terminal session no longer exists on Tower'); + } } }); } @@ -259,7 +296,9 @@ export class CodevPseudoterminal implements vscode.Pseudoterminal { private scheduleReconnect(): void { if (this.disposed || this.gaveUp || this.reconnectTimer) { return; } if (this.backoff.recordFailure() === 'give-up') { - this.giveUp(`unable to reconnect after ${MAX_RECONNECT_ATTEMPTS} attempts`); + if (this.enterGiveUp('transient')) { + void this.renderExhaustedGiveUp(); + } return; } @@ -283,28 +322,110 @@ export class CodevPseudoterminal implements vscode.Pseudoterminal { } /** - * Enter the terminal failure state: stop auto-retrying and surface a quiet - * red notice carrying the clickable reconnect affordance (#936 give-up state; - * the affordance itself is wired by ReconnectTerminalLinkProvider, #939). + * Enter the terminal failure state: stop auto-retrying. `kind` records WHY — + * `transient` (the #936 6-attempt budget was exhausted) can be re-armed by a + * later wake signal (onWake, #1681); `permanent` (Tower 4xx'd the upgrade — + * the session is gone) must never be, or it would revive the pre-June + * retry-storm the budget was added to stop. Returns whether the state was + * newly entered (false if already gave up), so the caller only renders a + * banner once. Banner rendering is a separate step so the exhausted path can + * word it after an async /health probe. */ - private giveUp(reason: string): void { - if (this.gaveUp) { return; } + private enterGiveUp(kind: 'transient' | 'permanent'): boolean { + if (this.gaveUp) { return false; } this.gaveUp = true; + this.giveUpKind = kind; + this.giveUpToken++; if (this.reconnectTimer) { clearTimeout(this.reconnectTimer); this.reconnectTimer = null; } + return true; + } + + /** + * Surface the terminal-failure banner: a quiet red notice carrying the + * clickable reconnect affordance (#936 give-up state; the affordance itself + * is wired by ReconnectTerminalLinkProvider, #939). + */ + private renderGiveUpBanner(reason: string): void { + if (this.disposed) { return; } this.log('WARN', `Giving up reconnect: ${reason}`); - // When reached via the exhausted-budget path, a yellow retry notice is - // sitting on the current line; overwrite it in place. When reached via the - // immediate-4xx path, no notice exists, so don't disturb the current line. - // Either way the give-up notice keeps its trailing `\r\n` and is never - // wiped — it is the terminal failure state and must stay visible (#1001). + // Overwrite the yellow retry notice in place when one is present (exhausted + // path); leave the current line alone on the immediate-4xx path. The banner + // then OWNS the current line — no trailing `\r\n` — and is tracked as a + // wipeable notice (hadReconnectNotice = true). While the terminal stays dead + // nothing else writes (retries have stopped), so it remains fully visible; + // but a later successful reconnect erases it in place via clearReconnectNotice() + // before the replay paints, instead of stranding a half-overwritten remnant + // on the recovered composer line (#1681 recovery-render fix — supersedes + // #1001's persistent-`\r\n` form, chosen before auto-recovery existed). const prefix = this.hadReconnectNotice ? '\r\x1b[2K' : ''; - this.hadReconnectNotice = false; this.writeEmitter.fire( - `${prefix}\x1b[31m[Codev: Connection lost. ${reason}. ${RECONNECT_LINK_TEXT}]\x1b[0m\r\n`, + `${prefix}\x1b[31m[Codev: Connection lost. ${reason}. ${RECONNECT_LINK_TEXT}]\x1b[0m`, ); + this.hadReconnectNotice = true; + } + + /** + * Word the exhausted-budget banner honestly (#1681). The common cause of + * exhausting the budget is a slept laptop firing all six attempts against a + * suspended network stack, not a dead Tower — so probe `/health` once (when a + * probe was injected) and distinguish "Tower unreachable" from "reconnect + * failed (Tower is up)", the latter pointing the user at the click-to-retry + * affordance. The give-up STATE is already set synchronously by enterGiveUp, + * so no further retry can schedule while the probe is in flight; a manual + * reconnect or wake during the probe clears `gaveUp`, and we then suppress the + * now-stale banner rather than paint it over a recovering connection. + */ + private async renderExhaustedGiveUp(): Promise { + const token = this.giveUpToken; + let reason = `unable to reconnect after ${MAX_RECONNECT_ATTEMPTS} attempts`; + if (this.probeHealth) { + let towerUp: boolean | null = null; + // Race the probe against a short timeout so a blackholed network can't + // hold the banner for the SDK's full 10s request window (#1681). Clear the + // timer when the probe wins so it doesn't outlive the resolved race. + let timer: ReturnType | undefined; + try { + towerUp = await Promise.race([ + this.probeHealth(), + new Promise((resolve) => { timer = setTimeout(resolve, HEALTH_PROBE_TIMEOUT_MS, null); }), + ]); + } catch { + towerUp = null; + } finally { + if (timer) { clearTimeout(timer); } + } + // Any give-up transition since we launched (a wake reconnect, a fresh + // give-up, or a permanent 4xx landing on the reconnect) bumps the token; + // don't paint this now-stale exhausted-budget banner over it. + if (this.disposed || this.giveUpToken !== token) { return; } + if (towerUp === true) { + reason = 'reconnect failed (Tower is up)'; + } else if (towerUp === false) { + reason = 'Tower unreachable'; + } + } + this.renderGiveUpBanner(reason); + } + + /** + * Re-arm after a wake signal (VSCode window refocus, #1681). Laptop sleep + * suspends the network stack, so the six-attempt budget burns instantly and + * the tab enters the give-up state while Tower and the detached session are + * healthy — a transient OS event wearing a permanent-failure banner. On wake, + * reconnect a transiently gave-up adapter (or one still parked mid-backoff), + * resetting the budget for a full fresh retry chain. Deliberately a no-op for + * the #936 permanent class (session gone on Tower) and for an already-healthy + * OPEN connection, so it neither resurrects the retry storm nor drops a live + * terminal. + */ + onWake(): void { + if (this.disposed || !this.opened) { return; } + if (this.giveUpKind === 'permanent') { return; } + if (this.ws && this.ws.readyState === WebSocket.OPEN) { return; } + this.reconnect(); } /** @@ -359,6 +480,8 @@ export class CodevPseudoterminal implements vscode.Pseudoterminal { } this.backoff.reset(); this.gaveUp = false; + this.giveUpKind = null; + this.giveUpToken++; if (this.ws) { this.ws.close(); this.ws = null; diff --git a/apps/vscode/src/terminal-manager.ts b/apps/vscode/src/terminal-manager.ts index 4d3c0c35a..0cf6014ee 100644 --- a/apps/vscode/src/terminal-manager.ts +++ b/apps/vscode/src/terminal-manager.ts @@ -529,7 +529,7 @@ export class TerminalManager { } const authKey = await this.getAuthKey(); - const pty = new CodevPseudoterminal(wsUrl, authKey, this.outputChannel); + const pty = new CodevPseudoterminal(wsUrl, authKey, this.outputChannel, () => this.probeTowerHealth()); const position = vscode.workspace.getConfiguration('codev').get('terminalPosition', 'editor'); // Dev processes are long-running background logs — always the bottom panel, @@ -622,6 +622,36 @@ export class TerminalManager { } } + /** + * Re-arm every managed terminal's reconnect budget on a wake signal (#1681). + * A laptop sleep suspends the network stack, so the adapters' six-attempt + * budgets burn instantly and every tab lands on a permanent-looking + * "unable to reconnect" banner while Tower and the detached sessions are + * healthy. `onWake` reconnects a transiently gave-up (or still-parked) + * adapter and no-ops an already-connected one or a #936 permanent give-up, so + * this is safe to fire across all terminals unconditionally on every refocus + * — unlike repaintAllOnRefocus, which stays behind an opt-in setting. + */ + rearmAllOnWake(): void { + for (const entry of this.terminals.values()) { + entry.pty.onWake(); + } + } + + /** + * One-shot Tower `/health` probe used to word a terminal's exhausted-budget + * give-up banner honestly (#1681). Returns true when Tower answers, false when + * it is unreachable, and `null` when reachability is unknown (no client yet) — + * so the adapter falls back to the neutral attempt-count wording rather than + * asserting "Tower unreachable" for what is really an unconfigured client. + * `getHealth` already collapses network errors to `null`, so this never throws. + */ + private async probeTowerHealth(): Promise { + const client = this.connectionManager.getClient(); + if (!client) { return null; } + return (await client.getHealth()) !== null; + } + private buildWsUrl(terminalId: string): string | null { const workspacePath = this.connectionManager.getWorkspacePath(); if (!workspacePath) { return null; } diff --git a/codev/projects/bugfix-1681-vscode-terminal-reconnect-budg/status.yaml b/codev/projects/bugfix-1681-vscode-terminal-reconnect-budg/status.yaml new file mode 100644 index 000000000..87e439182 --- /dev/null +++ b/codev/projects/bugfix-1681-vscode-terminal-reconnect-budg/status.yaml @@ -0,0 +1,17 @@ +id: bugfix-1681 +title: vscode-terminal-reconnect-budg +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-09-15T00:26:51.269Z' + approved_at: '2026-09-16T23:52:34.711Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-09-14T23:58:24.190Z' +updated_at: '2026-09-16T23:52:34.711Z' +pr_ready_for_human: false diff --git a/codev/state/bugfix-1681_thread.md b/codev/state/bugfix-1681_thread.md new file mode 100644 index 000000000..10d2f4488 --- /dev/null +++ b/codev/state/bugfix-1681_thread.md @@ -0,0 +1,149 @@ +# bugfix-1681 — terminal reconnect budget exhausts during laptop sleep + +Issue #1681. VS Code terminal tabs show a permanent `[Codev: Connection lost. unable to +reconnect after 6 attempts. Click here to reconnect]` after laptop sleep, while Tower + the +detached shellper session are perfectly healthy. Only the client view dies. + +## INVESTIGATE (complete) + +### Root cause +- `apps/vscode/src/terminal-adapter.ts`: `BackoffController({ maxAttempts: 6 })` + + `scheduleReconnect()`. After 6 consecutive transient failures, `giveUp()` sets + `gaveUp = true` **permanently** — the retry loop stops until a manual reconnect click. +- Backoff curve is `[1s,2s,4s,8s,16s,30s]` ≈ 61s total. During sleep the network stack is + suspended, so each `connect()` fails instantly; the whole budget burns before the machine + fully wakes. A transient OS event becomes a permanent-looking failure banner on every tab. +- The only wake signal wired is `onDidChangeWindowState` focus rising-edge (extension.ts:334), + which calls `repaintAllOnRefocus()` — a SIGWINCH-only repaint, gated behind the off-by-default + `codev.terminal.repaintOnRefocus`. It **never** re-arms the reconnect budget or reconnects a + gave-up adapter. + +### #936 constraint (architect brief + issue comment) +The finite budget + `giveUp` was PIR #936's deliberate fix for a pre-June infinite retry loop +against stale terminal ids. The 4xx fast give-up (`classifyUpgradeError(...) === 'permanent'` → +`giveUp('this terminal session no longer exists on Tower')`) MUST stay. A wake re-arm must +re-arm ONLY the transient give-up class, never "session no longer exists on Tower". + +### Wake-signal note +The issue lists "window focus, online, visibility". The VS Code extension host is Node — there +is no DOM `online`/`visibilitychange` event (`rg` confirms `onDidChangeWindowState` is the only +window-state hook). So the trio collapses to the focus rising-edge already wired at +extension.ts:334. Will note this in the PR. + +### Fix shape (scoped, ~70 prod LOC, <300 ceiling) +1. `terminal-adapter.ts`: + - Track give-up class: `transient` (exhausted budget) vs `permanent` (4xx). Reset on + `recordSuccess`/`reconnect`. + - `onWake()`: no-op when disposed, permanently gave-up, or currently OPEN; otherwise + `reconnect()` (which already resets backoff + clears gaveUp + reconnects). This also skips + a long parked backoff wait. + - Honest banner via one `/health` probe (injected `probeHealth` closure): exhausted path + words the banner "Tower unreachable" (probe false) vs "reconnect failed (Tower is up)" + (probe true) vs current "unable to reconnect after 6 attempts" (no probe / unknown). + Permanent path keeps its existing wording. +2. `terminal-manager.ts`: `rearmAllOnWake()` iterating managed ptys; inject `probeHealth` via + `connectionManager.getClient()?.getHealth()` (returns null on unreachable → clean boolean). +3. `extension.ts`: call `terminalManager?.rearmAllOnWake()` on the focus rising-edge, + **unconditionally** (not behind repaintOnRefocus). +4. Tests: signal-driven re-arm reconnects a transient give-up; 4xx still gives up AND `onWake` + does not resurrect it; banner wording split (Tower up vs unreachable). CI can't run a real + sleep — will state what was simulated vs needs physical sleep/wake. + +### Out of scope +apps/web `Terminal.tsx` may have a sleep/wake sibling — architect owns filing that separately; +I leave it untouched and will note in the PR if I confirm the gap. + +## FIX (implemented, committed 13126d3a6) + +Changed files: +- `apps/vscode/src/terminal-adapter.ts`: `giveUpKind` field; `enterGiveUp(kind)` + + `renderGiveUpBanner(reason)` + async `renderExhaustedGiveUp()` (replaces old `giveUp`); + `onWake()`; constructor gains optional `probeHealth`. `reconnect()`/open-handler reset + `giveUpKind`. +- `apps/vscode/src/terminal-manager.ts`: `rearmAllOnWake()` + `probeTowerHealth()` (via + `connectionManager.getClient()?.getHealth()`); inject the probe into the adapter ctor. +- `apps/vscode/src/extension.ts`: call `rearmAllOnWake()` unconditionally on the focus + rising-edge (before the opt-in repaint). +- `apps/vscode/src/__tests__/terminal-adapter.test.ts`: +7 tests (4 wake re-arm incl. the + 4xx-permanent guard + parked-backoff + healthy-noop; 3 banner-wording split). + +Verification (from worktree): +- `vitest run terminal-adapter.test.ts` → 37 passed. +- full `pnpm test:unit` → 1016 passed (83 files). +- `pnpm check-types` → clean (exit 0) after building codev-types/sdk/artifact-canvas deps. +- `pnpm lint` → clean. +- Regression pins the fix: the new tests call `pty.onWake()` (nonexistent pre-fix) and assert + the probe-worded banners (new). CI cannot run a real sleep; the tests simulate the burned + budget + a wake signal. A physical sleep/wake is still the true end-to-end confirmation — will + state this in the PR. + +### CMAP impl review (gemini / codex / claude) +- Gemini: APPROVE, no issues. +- Codex: COMMENT — real race: `renderExhaustedGiveUp` only rechecked `gaveUp`; a stale + `/health` probe could overwrite a permanent 4xx banner that landed on a wake reconnect. +- Claude: APPROVE — same race (non-blocking) + 3 residuals: (2) no throttle on `rearmAllOnWake` + (rapid refocus while Tower down restarts in-flight connects/resets budget), (3) focus is the + only wake signal, (4) `rearmAllOnWake`/extension wiring unit-untested → do a real sleep/wake. + +Resolution (commit deee898ef): +- FIXED the race (both reviewers): give-up **generation token** bumped on every give-up + transition (enter/success/reconnect); the async banner renders only if the token is unchanged + — closes both the permanent-overwrite and the double-transient staleness. +1 deferred-probe + regression test (38 adapter tests now, full suite still green, types clean). +- DOCUMENTED as residuals in the PR body (out of BUGFIX scope, follow-ups): the re-arm throttle + (2), the single wake signal (3), and the manual sleep/wake verification (4). + +## PR (open, #1682) + +PR #1682 opened with `Fixes #1681`. Branch `builder/bugfix-1681`. + +### PR-phase CMAP round 1 (gemini/codex/claude) +- gemini APPROVE; codex REQUEST_CHANGES (wiring coverage + branch 15 behind main); + claude APPROVE (2 real non-blocking: 10s /health probe delays banner; onWake before open() + can leak a socket). +- Addressed (commit 646c3fe0c): race /health probe to 2s cap; guard onWake until open()ed + (`opened` flag); source-level wiring guards for the manager fan-out + probe injection + + extension focus-handler call (this file's established harness pattern); dropped dead `void pty`. + Merged origin/main (clean, no overlap on my 5 files) to clear the staleness flag. + +### PR-phase CMAP round 2 (after fixes, head e05bf66d1) +- gemini APPROVE, codex APPROVE, claude APPROVE. No blocking issues. +- Residuals (documented, out of scope): re-arm throttle (`lastRearmAt`), the uncleared 2s + probe-timeout timer (self-resolving, negligible), the ≤2s yellow-notice lingering, focus-only + wake signal, apps/web sibling. Physical sleep→wake is the real end-to-end check (needs human). + +Verification at PR head: full vscode unit suite 1022 passed, check-types + lint clean. + +Notified architect + fired the `pr` gate. + +### Post-gate: recovery-render glitch reported by human (0e70d5c3f) +Human tested a real recovery and saw a half-overwritten banner remnant +(`…k here to reconnect]`) stranded on the composer line, clearing on the next +keystroke. Root cause: the red give-up banner is client-injected text the +reconnected agent TUI can't see; it ended with `\r\n` + cleared +`hadReconnectNotice` (#1001's persistent form, pre-auto-recovery), so a +successful reconnect never wiped it and the app's repaint covered only the left +of the row. Auto-recovery made give-up→reconnect frequent, exposing it. +Fix (human chose fold-into-PR): banner now owns the line (no `\r\n`) and is a +wipeable notice, so `clearReconnectNotice()` erases it in place on the next open +before the replay paints; still fully visible/clickable while dead. +1 +recovery-render regression test; updated the #1001 "never wiped" test to the new +wipe-on-recovery behavior. Full suite 1023 pass, types+lint clean. PR body updated. +### PR CMAP round 3 (render change, head 1cbb6c0ca) +gemini APPROVE, codex COMMENT (the re-arm throttle — a documented follow-up, not new), +claude APPROVE. No blocking / no REQUEST_CHANGES. Two trivial net-new minors folded +(be1b85275): probeTowerHealth returns null (not false) when no client → neutral wording; +clear the 2s probe-race timer when the probe wins. +1 null-probe fallback test; sentinel regex +updated (it caught its own drift). Full suite 1024 pass, types+lint clean. + +Did NOT run a 4th CMAP round for those two trivial changes (disproportionate; substantive code +already all-APPROVE). Remaining documented follow-ups (out of BUGFIX scope): re-arm throttle, +display-sleep-without-focus wake gap, apps/web sibling. Physical sleep→wake still the real check. + +**Holding for human gate approval.** + +## Fences +terminal-adapter.ts is mine. Not touching views/tower*.ts, workspace-label.ts, fleet-order.ts, +attention-format.ts, switch-workspace.ts (pir-1566), or views/builders.ts, terminal-manager +cycle logic owned by pir-1563 — my terminal-manager edits are additive (`rearmAllOnWake` + +constructor arg), not to the cycle/focus resolution code.