Fix swallowed shellper reconnect error: recover connections in place instead of minting zombie terminals

[amrmelsayed]

PIR Review: Shellper reconnect error swallowed — silent zombie terminals

Fixes #1198

Summary

A post-handshake socket or parser error on a shellper connection ran cleanup() before the socket's 'close' event fired, so the 'close' emission (decided by reading _connected, already cleared) was swallowed — and with it every downstream recovery path. The terminal became a silent zombie: reported running, all input dropped, "Message sent" logged for messages that went nowhere, until the next Tower restart. This PR makes the client record the owed 'close' at teardown time (_closePending, captured inside cleanup() where the knowledge exists), and — because emitting 'close' alone would have made a transient error permanently orphan a healthy shellper (the historical close path unlinks the live socket and deletes the SQLite row) — pairs it with in-place recovery: SessionManager reconnects to the still-alive shellper (bounded retry with PID/start-time/socket preflight), PtySession defers its destructive teardown behind a 15s grace timer that a successful re-attach cancels, and the Tower layer re-attaches the replacement client. Supporting fixes: 'session-error' finally has a consumer (ERROR-logged), adoption awaits the REPLAY frame instead of racing it (blank-until-poked terminals), sends to a dead terminal return 503 TERMINAL_NOT_WRITABLE and log Message DROPPED instead of false success, and afx tower stop polls until the process exits (SIGKILL after 8s) so stop && start serializes instead of overlapping adoption with teardown.

Files Changed

• packages/codev/src/terminal/shellper-client.ts (+31 / −8)
• packages/codev/src/terminal/session-manager.ts (+147 / −45)
• packages/codev/src/terminal/pty-session.ts (+65 / −11)
• packages/codev/src/terminal/pty-manager.ts (+15 / −0)
• packages/codev/src/agent-farm/servers/tower-server.ts (+27 / −0)
• packages/codev/src/agent-farm/servers/tower-terminals.ts (+7 / −2)
• packages/codev/src/agent-farm/servers/tower-routes.ts (+13 / −0)
• packages/codev/src/agent-farm/servers/send-buffer.ts (+14 / −0)
• packages/codev/src/agent-farm/commands/tower.ts (+35 / −0)
• packages/codev/src/terminal/__tests__/shellper-client.test.ts (+71 / −0)
• packages/codev/src/terminal/__tests__/session-manager.test.ts (+130 / −0)
• packages/codev/src/terminal/__tests__/pty-session-attach.test.ts (+34 / −1)
• packages/codev/src/terminal/__tests__/tower-shellper-integration.test.ts (+42 / −3)
• packages/codev/src/agent-farm/__tests__/send-buffer.test.ts (+41 / −1)
• packages/codev/src/agent-farm/__tests__/tower-routes.test.ts (+33 / −5)
• packages/codev/src/agent-farm/__tests__/tower-terminals.test.ts (+2 / −0)

Commits

• adb335eb [PIR Shellper reconnect error is swallowed: terminal becomes a silent zombie (no input/output, 'Message sent' logged for dropped frames) until next Tower restart #1198] Fix swallowed error-path close and make write/resize observable in ShellperClient
• 00afce9f [PIR Shellper reconnect error is swallowed: terminal becomes a silent zombie (no input/output, 'Message sent' logged for dropped frames) until next Tower restart #1198] Reconnect in place on unexpected shellper socket close
• 8e224038 [PIR Shellper reconnect error is swallowed: terminal becomes a silent zombie (no input/output, 'Message sent' logged for dropped frames) until next Tower restart #1198] Grace-timer teardown on unexpected disconnect; observable write/resize in PtySession
• f951a338 [PIR Shellper reconnect error is swallowed: terminal becomes a silent zombie (no input/output, 'Message sent' logged for dropped frames) until next Tower restart #1198] Tower layer: log session errors, re-attach reconnected clients, wait for replay, fail dropped sends loudly
• fb61c59d [PIR Shellper reconnect error is swallowed: terminal becomes a silent zombie (no input/output, 'Message sent' logged for dropped frames) until next Tower restart #1198] towerStop waits for process exit, escalating to SIGKILL after 8s
• 58fc3526 [PIR Shellper reconnect error is swallowed: terminal becomes a silent zombie (no input/output, 'Message sent' logged for dropped frames) until next Tower restart #1198] Tests: close emission, in-place reconnect, grace window, writable guards
• 41398bff [PIR Shellper reconnect error is swallowed: terminal becomes a silent zombie (no input/output, 'Message sent' logged for dropped frames) until next Tower restart #1198] Collapse close-emission flags into a single _closePending recorded by cleanup
• 54ef5bd4 [PIR Shellper reconnect error is swallowed: terminal becomes a silent zombie (no input/output, 'Message sent' logged for dropped frames) until next Tower restart #1198] Extract towerStop wait timing into named constants

Test Results

• pnpm build (workspace root): pass
• pnpm test (packages/codev): pass — 3532 tests, 0 failures (13 new tests)
• New coverage: error-path close emission (black-box: oversized frame header → parser error → 'close' fires), intentional disconnect stays silent, write/resize delivery booleans, in-place reconnect success / dead-path / exhaustion (real Unix sockets against an in-process shellper), grace-window teardown and re-attach cancellation, writable guard, 503 send path, send-buffer hold-then-drop.
• One pre-existing test (tower-shellper-integration: "emits exit with code -1 on unexpected disconnect") asserted the old immediate-teardown behavior and was rewritten for the grace-window semantics.
• Manual verification: reviewed and dev-approval-gated by the human on the running worktree; the definitive live test (Tower restart on the patched build restoring the currently zombified architects) runs at deploy time — see How to Test Locally.

Architecture Updates

Routed to COLD (codev/resources/arch.md, Shellper Process Architecture): a new "Connection-loss recovery (#1198)" subsection documenting the recovery pipeline (close-emission contract, in-place reconnect, grace-timer teardown, re-attach, write observability) and its invariant: never tear down a terminal's registry/DB state on a raw socket close without first checking whether the shellper process is alive. Nothing routed to HOT — arch-critical.md is at its 10-fact cap and this is subsystem-internal behavior, not a cross-cutting decision-changing fact; no existing hot fact is weaker than it.

Lessons Learned Updates

Routed to COLD (codev/resources/lessons-learned.md, Debugging and Root Cause Analysis): two entries — (1) lifecycle emissions must be decided from state captured at the teardown transition, not re-read later, and un-swallowing a long-suppressed event requires auditing every consumer first (the naive fix would have converted zombies into permanently orphaned sessions); (2) success-shaped logging ("Message sent" with no delivery signal) turns an outage invisible — delivery paths need an observable failure contract at the API boundary, not just logs. Nothing routed to HOT (cap full; these are narrower than the current ten).

Things to Look At During PR Review

• Live-deploy incident (2026-07-19, fixed in this branch — read this first). The first install of this branch surfaced the true trigger of the whole issue and a critical amplification in my recovery design. Two long-lived architects (shannon/app, codev architect) had replay buffers over MAX_FRAME_SIZE (17.6MB / 17.7MB > 16MB). The shellper sends that as one REPLAY frame; the parser treated it as a fatal stream error — deterministically, on every connect. On the old code that error was swallowed → the original zombies (the stop/start-overlap theory was wrong; same terminal zombified every restart because the failure is deterministic). On this branch pre-fix, recovery retried the deterministic failure, exhausted its rounds, took the dead path, unlinked the live sockets → killOrphanedShellpers then killed both live shellpers and their Claude conversations. Fix, three layers: (1) FrameParser now discards oversized frames incrementally ('frame-skipped' event) and keeps the stream alive — mandatory Tower-side because running shellpers are old binaries; (2) removeDeadSession never unlinks a live process's socket, so any future deterministic failure degrades to a recoverable orphan instead of feeding the kill sweeper; (3) new shellpers cap replay to REPLAY_PAYLOAD_MAX (8MB tail; client resolves skipped replay as empty and viewers repaint via the post-connect resize nudge). Regression tests at all three layers (client-level oversized-REPLAY survival test reproduces the incident shape byte-for-byte). Verified post-fix on the live fleet: all 20 surviving sessions show live-Tower fds and non-empty output buffers.
• Post-incident follow-ups (both fixed in-branch). (1) UI interactivity regression: restoring the replay path meant adoption seeded up to 16MB of newline-free history into each ring buffer, and every viewer attach shipped that whole payload to xterm.js in one bracketed write (multi-second parse; webview stalls needing a reload). Adoption now caps the ring seed to the most recent 1MB (RING_SEED_MAX_BYTES, logged when trimming); the shellper retains full history and TUIs repaint via the resize nudge. (2) Architect review finding (PID-reuse leak, Medium): removeDeadSession gated socket unlink on isProcessAlive alone, so a reused PID would leak a dead shellper's socket/log. The unlink decision is now async best-effort (cleanupDeadSessionSocket) using the same alive + start-time guard as reconnection (isShellperProcessCurrent, shared with canReachShellper); removeDeadSession's map mutation stays synchronous.
• Consultation round 2 (human-requested, on the post-incident diff): claude APPROVE; codex COMMENT with one accepted note — creation-time attach also raced the REPLAY frame (real for fast-starting children). Fixed with zero creation latency: new shellpers always send a REPLAY frame even when empty, and all four creation sites await it. Codex's second note (plan frontmatter) rebutted in the rebuttals file: porch-driven artifacts carry their approval in status.yaml gate history, not retroactive frontmatter.
• Consultation finding (codex, REQUEST_CHANGES — fixed): attachShellper() opened the disk-log fd unconditionally; with re-attach now a routine recovery step, every reconnect leaked one append handle when disk logging is enabled. Fixed by guarding the open on logFd === null (pty-session.ts; cleanupShellper() closes and nulls the fd, so post-teardown attaches still reopen). Regression test pins it: pty-session-attach.test.ts "does not reopen the disk log when a recovery re-attach arrives" (attach → re-attach → one open; detach → attach → second open). PIR's consultation is single-pass, so this fix was not independently re-reviewed — worth a human glance at the guard. The claude consultation verdict was APPROVE.
• The recovery loop guards (session-manager.ts, recoverSession): the 3-attempt/3-round caps with the 30s stability reset, and the session.client !== client stale-wiring checks that prevent a replaced client's events from re-triggering recovery.
• Grace-timer interplay (pty-session.ts): the 15s SHELLPER_CLOSE_GRACE_MS teardown deferral is sized above SessionManager's worst-case recovery round (~4s); attachShellper cancels it. A genuinely dead terminal now reports exited up to 15s later than before (writes to it fail loudly during the window via the writable guard).
• Re-attach passes empty replay deliberately (tower-server.ts): the ring buffer already holds session history; replaying would duplicate output. Known limitation: bytes the PTY emitted during the disconnection window (seconds) are not spliced in — a full-screen TUI repaints past this; a plain shell's scrollback genuinely misses those lines.
• findByShellperSessionId (pty-manager.ts): needed because the create flow keys SessionManager by a fresh UUID while adoption keys by terminal id.
• Known gaps, deliberately out of scope, for follow-up issues: (a) PING/PONG heartbeat detection for a socket that hangs open without erroring (issue Shellper reconnect error is swallowed: terminal becomes a silent zombie (no input/output, 'Message sent' logged for dropped frames) until next Tower restart #1198 item 5, first half); (b) connect() has no handshake timeout — a shellper that accepts but never sends WELCOME would hang a recovery attempt (pre-existing exposure, shared with the adoption path; fails visible via the grace timer, not as a zombie); (c) possible enum consolidation of the client's connection-lifecycle state, best done together with (a).

How to Test Locally

• View diff: VSCode sidebar → right-click builder pir-1198 → Review Diff
• Run dev: afx dev pir-1198, or for the definitive live test install this branch's build: cd .builders/pir-1198 && pnpm build && pnpm -w run local-install
• What to verify (maps to the plan's Test Plan):
  • After the Tower restart, every persistent terminal's GET /api/terminals/:id/output returns total >= 1 immediately (previously blank-until-poked); the previously zombified architects respond to typed input and afx send.
  • afx tower stop returns only after the process exits (lsof -i :4100 is empty when it returns); stop && start shows no port-in-use retries.
  • Kill a disposable terminal's shellper with SIGKILL, then afx send to it: the send errors with TERMINAL_NOT_WRITABLE and the Tower log shows Message DROPPED, never Message sent.
  • Tower log after any connection blip shows connection lost unexpectedly followed by re-established / re-attached (recovery) or an ERROR line (genuine death) — never silence.

[amrmelsayed]

Architect Integration Review — PIR #1198 (pr gate)

Reviewed as the acting spawning architect for this builder. Recommendation: APPROVE for merge. No blocking issues. The pr gate is human-only, so this is a recommendation to the human, not an approval.

Consult status (honest labeling)

Single-pass PIR consult was effectively CMAP-2: claude=APPROVE, codex=REQUEST_CHANGES (gemini not in this run's model set). codex's REQUEST_CHANGES had exactly one KEY_ISSUE — the disk-log fd leak — accepted in full and fixed in 0126d5d3. Nothing was waved off in the rebuttal.

I independently re-reviewed the un-re-reviewed fix (builder's explicit ask)

The fd-leak guard in 0126d5d3 landed after the single-pass consult, so I traced it end-to-end rather than trust the rebuttal:

• Guard is correct: attachShellper now opens the log only if (this.diskLogEnabled && this.logFd === null) (pty-session.ts:161). Its correctness depends on cleanupShellper() closing and nulling logFd — verified at lines 285-287 (closeSync then logFd = null). The write path (301-308) guards on logFd !== null, so no write-to-closed-fd.
• Open-sites are mutually exclusive per session type: spawn() opens for node-pty sessions; attachShellper opens for shellper-backed sessions. The === null guard prevents a double-open in either path.
• The regression test pins the exact invariant and I reconciled why it's valid: the test never calls spawn(), so logFd starts null — first attach opens (1), re-attach skips (still 1), detach closes+nulls, next attach reopens (2). Fails without the guard.

Root-cause coverage — all four issue items addressed

1. Swallowed error-path close → fixed via in-place recovery. declareSessionDead (the historical death path the issue said "never runs") now logs shellper disconnected unexpectedly, emits session-error, and calls removeDeadSession, reached from both give-up branches.
2. session-error now consumed in the Tower layer (tower-server.ts:383, with a comment noting it previously had no consumer anywhere). A dying reconnect now leaves a trace.
3. afx tower stop waits: SIGTERM then poll process.kill(pid, 0) until exit, escalate to SIGKILL after STOP_EXIT_TIMEOUT_MS (tower.ts:362-388). stop && start serializes.
4. "Message sent" no longer lies for dropped frames (the issue title): tower-routes.ts:1320-1323 checks !session.writable and logs Message DROPPED … terminal not writable instead of the false success. writable reflects the live shellper connection, not just session status.

Recovery loop safety (the highest-risk new code) — sound

• Caps: MAX_RECOVERY_ROUNDS=3 with a RECONNECT_STABILITY_MS=30_000 reset (a session stable 30s clears its round counter) + per-round backoff.
• Stale-client guards at all three await boundaries: before recovery (session.client !== client), after each backoff sleep, and after connect (which disconnects the newly-made client if the session was killed mid-connect — no leak).
• PID-reuse guard: canReachShellper checks process-alive + start-time match (±2s) + socket exists before reconnecting.

Non-blocking scope notes (conscious-acceptance, not blockers)

• Empty replay on re-attach is deliberate (tower-server.ts): the ring buffer holds session history, so replay would duplicate output. Known limitation, documented: bytes the PTY emitted during the disconnection window are not spliced in — a full-screen TUI repaints past it, but a plain shell's scrollback genuinely misses those lines. This means the "reconnect doesn't reliably replay the screen buffer → blank-until-poked" secondary observation from the issue is not fully closed here; the zombie itself is fixed, the cosmetic blank-until-repaint for plain shells is left as a follow-up. Worth a conscious call on whether that's an acceptable v1 boundary.
• 15s grace timer: a genuinely dead terminal now reports exited up to SHELLPER_CLOSE_GRACE_MS (15s) later than before. Writes fail loudly during the window via the writable guard, so it's observable, not silent. Reasonable tradeoff.

Full suite: 3533 green. Clean implementation that fixes the root cause rather than papering over the symptom.

---

Architect integration review

[amrmelsayed]

Architect Review — Addendum: incident-fix commit 3d6774ca (pr gate)

The incident fix landed after my first review and had zero consult (porch didn't re-run consultation), so I reviewed it directly and ran an independent adversarial second lens (gpt-5.1-codex, since the codex CLI is down) specifically tasked to refute correctness on the three highest-risk paths.

Verified sound (me + independent lens agree)

• Parser discard loop (shellper-protocol.ts, drainFrames/discard/discardRemaining): byte accounting is exact — dropped bytes = HEADER_SIZE + payloadLength, so the next frame boundary is intact. No stream desync across chunk splits, back-to-back oversized frames, or oversized-then-valid-in-same-chunk; payloadLength === MAX_FRAME_SIZE is still accepted (guard is >); no infinite loop/busy-return in the while(true).
• Replay cap (shellper-process.ts, 8MB tail) + frame-skipped → empty-replay unblock (shellper-client.ts): the cap keeps a REPLAY strictly under MAX_FRAME_SIZE; tail-slicing raw PTY bytes can't corrupt UTF-8 for the consumer (worst case starts mid-escape, repainted by the resize nudge); adoption cannot hang — a dropped REPLAY resolves the replay waiter with an empty buffer (once-guarded on replayData === null).
• removeDeadSession live-socket guard: correctly closes the incident's data-loss mechanism — a live shellper's socket is never unlinked, so a deterministically-failing session degrades to a re-adoptable orphan instead of a killed process.

One finding — Medium, NOT data-loss (conservative failure direction)

removeDeadSession (session-manager.ts:820) gates the unlink on !isProcessAlive(session.pid) without the start-time re-validation its sibling canReachShellper (407-418) uses. On PID reuse — shellper dies, OS recycles its PID before removeDeadSession runs — isProcessAlive returns true, so we skip unlinking a socket whose shellper is genuinely gone. Result: stale .sock/.log pairs leak (no later hook reaps them: killOrphanedShellpers finds no matching process, cleanupStaleSockets is manual-only, next-start reconnect fails the PID-reuse check and leaves the file), and the "socketDir scan distinguishes live from dead" invariant degrades.

Why it's not a blocker on severity: the dangerous direction (unlinking a live socket → orphan-sweep kills it → lost conversation) stays fully closed. This is a leak + monitoring-invariant issue, not data loss.

Fix direction: gate unlink on pid dead OR start-time mismatch, mirroring canReachShellper (the startTime is already on the session object). Wrinkle: removeDeadSession is sync and getProcessStartTime is async, so this isn't a drop-in — it needs removeDeadSession to go async (audit callers at 317/429/1017) or a synchronous start-time read. That effort is the reason this is reasonable to defer to an immediate follow-up if the urgency of installing the (verified-sound) core fix wins.

Recommendation

Core incident fix is sound and independently verified. The one finding is a non-data-loss hygiene leak with a non-trivial async fix. Two acceptable paths, human's call at the pr gate: (a) builder folds in the start-time guard now, then merge; or (b) merge + install the critical fix now (current build actively kills live shellpers on restart), file the PID-reuse leak as an immediate follow-up.

---

Architect integration review

[amrmelsayed]

Architect Review — delta 65526295 (post-incident follow-ups)

Reviewed the two new changes directly (still zero consult on these).

1. PID-reuse-safe socket cleanup (my earlier finding) — correct, and nicely DRY. The alive+start-time check is extracted to a shared isShellperProcessCurrent helper used by both canReachShellper (reconnect) and the new cleanupDeadSessionSocket; removeDeadSession keeps its map mutation sync and delegates the unlink to an async best-effort call. On PID reuse the socket is now unlinked, so the leak I flagged is closed, and the data-loss-safe direction is preserved (a live shellper's socket is still never touched).

2. Ring-seed 1MB cap — correct, fixes the interactivity regression. capRingSeed tail-trims the replay seeded into an adopted PtySession's ring buffer at both adoption sites. This stops the multi-MB newline-free replay from being shipped to xterm.js in one bracketed write (the multi-second parse / webview stall the human reported), and incidentally closes the "blank-until-poked" symptom I'd flagged earlier as a deferred follow-up. Correctly layered against the 8MB transport-level replay cap (distinct concern, distinct bound).

One non-blocking hardening note (defense-in-depth, not a bug today): cleanupDeadSessionSocket unlinks session.socketPath after await isShellperProcessCurrent(...) without re-checking this.sessions.get(sessionId) !== session — the guard recoverSession uses at its await boundaries. Socket paths are reusable (keyed by dbSession.id), so if a future flow re-adopts the same id in-process during that async gap, the stale cleanup could unlink a live new socket → reintroduce the data-loss path. It is not reachable today (nothing recreates the same id in-process after removeDeadSession; setupAutoRestart reuses the same session object and calls removeDeadSession terminally). Cheap to harden now (one sessions.get guard, matching recoverSession) or as a follow-up — your call.

Overall: APPROVE. Core incident fix + both follow-ups verified sound (parser/replay/cleanup by me + independent adversarial lens; this delta by me). Recommend approving the pr gate; the one open item is a latent hardening note, not a blocker.

---

Architect integration review

[amrmelsayed]

Architect Review — successor guard 2d4011df: verified, and my suggested shape was wrong

Confirmed correct — and the builder was right to adjust my suggestion. removeDeadSession deletes the map entry before firing the async cleanupDeadSessionSocket, so my sessions.get(id) !== session would be perpetually true (the entry is already gone) → it would never unlink → every dead-session socket would leak. The builder's sessions.has(id) has the right semantics for this ordering: after the await, a present entry is necessarily a successor that claimed the id, so the socket path is theirs and we leave it; absent → safe to unlink. Placement is also correct (right before the unlink, in the same synchronous stretch, so no successor can appear between the check and the unlink).

That closes the last open item — a latent one at that. Nothing outstanding. Final recommendation: APPROVE.

Full chain now verified: fd-leak fix, the 3-prong incident fix (parser tolerance / live-socket preservation / replay cap), the PID-reuse leak, the interactivity regression, and this successor guard — reviewed by me, with the highest-risk paths also cleared by an independent adversarial lens.

---

Architect integration review

[amrmelsayed]

Architect Review — round-2 deltas (fb9faf1e, 7a2f8053): both verified, APPROVE stands

7a2f8053 — always-send-REPLAY + creation-site await: correct. The shellper now writes a REPLAY frame even when empty (a 5-byte header / 0-length payload — parses cleanly, and old clients treat it as no-replay, so it's wire-compatible), and all 4 creation sites moved from the racy synchronous getReplayData() to await client.waitForReplay() — the same frame-await the adoption path already uses. No hang (the frame is now always sent), no meaningful latency (empty frame follows WELCOME immediately). Closes the creation-time REPLAY race codex flagged.

fb9faf1e — child-exit vs connection-failure split: correct, and it fixes a husk-leak my prior approval missed. The shellper lingers alive after its child exits (#905), so the old "preserve-if-alive on every removal" leaked an immortal empty shellper per exited session. The split resolves it cleanly:

• Child-exit / restart-exhaustion (removeDeadSession): synchronous, unconditional unlink — no async gap, so the successor race can't apply here, and unlinking is what lets killOrphanedShellpers reap the spent husk.
• Connection-failure (removeUnreachableSession, called from declareSessionDead): async preserve-if-alive + the successor guard — the incident fix.

I audited all three call sites: client.on('exit') (EXIT frame = child gone) and max-restarts-exhausted route to removeDeadSession; the recovery-give-up path routes to removeUnreachableSession. The exit-frame-vs-socket-close axis is the right thing to split on, and no connection-failure path reaches the unconditional-unlink method — so the "never unlink a live shellper's socket" invariant holds.

Process note: fb9faf1e re-touched the data-loss-critical socket-lifecycle path but wasn't mentioned in the status update (only the REPLAY fix was). Caught it in the diff; flagging so the review record is complete. No issue with the change itself.

No outstanding items. APPROVE. Every delta across all rounds now verified.

---

Architect integration review

[amrmelsayed]

Architect Review — 8b79553b (instant-exit race): verified, APPROVE stands

Correct fix. getSessionInfo(sessionId)! now runs synchronously immediately after createSession (session guaranteed present → the non-null assertion is safe) at all 4 creation sites, with await waitForReplay() moved after it. Nothing after the await depends on the shellper session still being in the map — shellperInfo/replayData are captured and createSessionRaw builds a fresh PtySession from them — so an instantly-exiting child during the await now produces a terminal that shows exited (correct for the /bin/echo probe) instead of a 500. This is a regression the round-2 waitForReplay await introduced; good CI catch. No outstanding items.

---

Architect integration review