Codev process fleet drives macOS memory-pressure kills: shellper husk sweep + fleet observability

codev/projects/1227-codev-process-fleet-drives-mac/1227-review-iter1-rebuttals.md

@@ -0,0 +1,34 @@
+# Rebuttal — PIR #1227, review iteration 1
+
+Verdicts: claude=APPROVE, codex=REQUEST_CHANGES.
+
+Both findings from codex's `REQUEST_CHANGES` were real defects, not false positives. Both are fixed in commit `3a5083e3`.
+
+## Finding 1: `execFileSync` blocking the event loop on `/health`
+
+**Codex's claim**: `process-census.ts` used `execFileSync('ps', ...)`, called synchronously from `handleHealthCheck` (the `/health` route). Every `/health` request would block Tower's entire Node.js event loop for the duration of the `ps` call, freezing all open terminals' WebSocket traffic — and since `TowerClient.isRunning()`/`getHealth()` both hit `/health`, this is a common path, not a one-off admin route.
+
+**Assessment**: Confirmed, real defect. `codev/resources/lessons-learned.md:160` documents this exact anti-pattern as a previously-fixed bug in this codebase ("`execSync` in HTTP request handlers blocks the entire Node.js event loop, freezing terminal WebSocket traffic"). My `process-census.ts` reintroduced it.
+
+**Fix**: Rewrote `listProcessCensus()` to use non-blocking `execFile` with a hand-rolled `Promise` wrapper (matching the existing async convention already used by `session-manager.ts`'s `findShellperProcesses` — same shape, same non-throwing-on-failure contract), instead of `util.promisify(execFile)` (which I considered but rejected: `child_process.execFile` carries a built-in custom-promisify symbol resolving `{stdout, stderr}`, and a plain `vi.fn()` test mock wouldn't reproduce that symbol, silently promisifying to the wrong shape). All four call sites (`shellper-husk-sweep.ts`'s `findHuskShellpers`, `tower-routes.ts`'s `computeFleetHealthFields` and `handleHuskPreview`) updated to `await` the now-async function; the `census` seam type in `shellper-husk-sweep.ts` widened to accept a plain array in addition to a function, so existing synchronous test seams needed no changes.
+
+**Regression test**: `process-census.test.ts` — `'never calls the synchronous execFileSync API'` mocks both `execFile` and `execFileSync`, asserts the sync API is never invoked. This fails against the pre-fix code (which called `execFileSync` directly) and passes now.
+
+## Finding 2: redundant second `ps` scan in husk preview
+
+**Codex's claim**: `handleHuskPreview` calls `findHuskShellpers()` (which internally scans once) and then immediately calls `listProcessCensus()` again just to build the RSS map — a second full-machine `ps` scan per preview request, and the displayed RSS could reflect a different process-table snapshot than the one that decided candidacy.
+
+**Assessment**: Confirmed. (Independently corroborated: claude's own APPROVE review flagged the identical double-scan as a "minor observation," reaching the same conclusion via a different read of the same code.)
+
+**Fix**: `handleHuskPreview` now takes one `census = await listProcessCensus()` snapshot up front, builds the RSS map from it, and passes it into `findHuskShellpers` via the (now array-accepting) `census` seam — so candidacy and displayed RSS are guaranteed to come from the same snapshot, and only one `ps` scan happens per preview request.
+
+**Not changed**: `findHuskShellpers` still calls `getProcessStartTime(pid)` once per already-narrowed candidate (to check `aged`), and `handleHuskPreview` calls it again per candidate to compute display `ageMs`. This is a small duplicate cost (a per-PID `ps -p <pid> -o lstart=`, not a full-machine scan) bounded by the candidate count, not the process count — fixing it would mean changing `findHuskShellpers`'s tested return contract from `number[]` to richer objects, a larger and riskier change for marginal benefit. Flagged here for visibility; happy to revisit if reviewers feel strongly at the `pr` gate.
+
+**Regression test**: not added as a dedicated unit test — `handleHuskPreview` is a private, unexported route handler, and asserting "internal call count" would require exporting it purely for testability. The existing `tower-routes-husks.e2e.test.ts` (real HTTP, real Tower) still passes and validates end-to-end correctness of the returned candidates/RSS; the fix itself is visible and reviewable directly in the diff.
+
+## Verification after the fix
+
+- `tsc --noEmit` clean (`packages/core` + `packages/codev`)
+- Full unit suite: 2249 passed, 34 pre-existing skips, no regressions
+- `process-census.test.ts`: 7/7 passing, including the new regression test
+- E2E (real Tower, real `ps`, real signals): `shellper-husk-sweep.e2e.test.ts`, `tower-routes-husks.e2e.test.ts`, `shellper-cleanup.e2e.test.ts` — 6/6 passing, unmodified assertions

codev/projects/1227-codev-process-fleet-drives-mac/status.yaml

@@ -0,0 +1,30 @@
+id: '1227'
+title: codev-process-fleet-drives-mac
+protocol: pir
+phase: verified
+plan_phases: []
+current_plan_phase: null
+gates:
+  plan-approval:
+    status: approved
+    requested_at: '2026-07-22T23:53:19.753Z'
+    approved_at: '2026-07-23T01:16:10.325Z'
+  dev-approval:
+    status: approved
+    requested_at: '2026-07-23T01:45:30.078Z'
+    approved_at: '2026-07-23T09:00:52.962Z'
+  pr:
+    status: approved
+    requested_at: '2026-07-23T09:17:51.128Z'
+    approved_at: '2026-07-23T09:20:33.747Z'
+iteration: 1
+build_complete: true
+history: []
+started_at: '2026-07-22T23:43:23.652Z'
+updated_at: '2026-07-23T09:22:48.049Z'
+pr_history:
+  - phase: review
+    pr_number: 1234
+    branch: builder/pir-1227
+    created_at: '2026-07-23T09:05:45.293Z'
+pr_ready_for_human: false

codev/resources/arch.md

@@ -858,6 +858,8 @@ async cleanupStaleSockets(): Promise<number> {
 }
 ```
 
+**Husk sweep (Issue #1227).** The "orphaned shellpers are never killed" policy above is deliberately permissive for the reconnectable case, but it also permanently protects a shellper whose PTY child has already exited (a "husk") — a husk's socket still responds, so `killOrphanedShellpers` skips it forever, even though it is not reconnectable. A separate, stricter sweep (`shellper-husk-sweep.ts`) closes that gap: it reaps a shellper only when it is simultaneously **unregistered** (no live PID in `terminal_sessions`), **childless** (no OS child process), and **aged** past a grace period (`SHELLPER_HUSK_GRACE_MS`, default 1h) — three conditions that together are true only for a genuine husk, never a live or reconnectable session. It runs on three triggers: Tower startup, an hourly in-process timer, and on-demand via `afx tower sweep-husks` (dry-run preview by default, `--apply` to reap, mirroring `afx workspace recover`'s UX). Fleet-wide RSS and an unregistered-shellper count are surfaced in `/health` and `afx status` for observability, computed from a shared `process-census.ts` `ps` scan.
+
 #### Dead Process Cleanup
 
 Tower cleans up stale entries on state load:

codev/resources/lessons-learned.md

@@ -470,6 +470,8 @@ Generalizable wisdom extracted from review documents, ordered by impact. Updated
 - [From 1187] A build tool that loads its config via an *optional* peer works locally and dies on clean CI — and the trigger can be the **Node version**, not the config's file extension. `tsdown` 0.22.8 loads its config through an `auto` loader that uses Node's native `import()` only when the runtime has native TS support (Bun / Node ≥24.11) and otherwise falls back to `unrun`, an optional peer `unconfig-core` declares but pnpm never installs. A dev machine on newer Node (or with a loader hoisted from another tool) loads the config fine; CI on older Node with a clean `--frozen-lockfile` install hits the `unrun` branch and dies ("Failed to import module 'unrun'"). Renaming `tsdown.config.ts` → `.mjs` did **not** help — the loader choice is runtime-driven, not extension-driven. The fix: force the loader (`tsdown --config-loader native`) with a plain-ESM `.mjs` config, so Node imports it directly on every version. General rule: a config written for a tool still needs a *loader* to be read (distinct from the tool's own compilation); if that loader isn't a hard dependency, pin the loader explicitly or make the config natively importable. Reproduce the exact CI failure locally with `pnpm exec tsdown --config-loader unrun` — don't rely on a green local build, which may be riding a newer Node or a hoisted loader the frozen CI install lacks.
 - [From 1047] A backpressure "relief valve" that re-fetches the overflowing payload is an infinite loop. The VSCode terminal's old rule was "receive buffer > 1 MB → disconnect and reconnect for a fresh replay" — but the oversized thing *was* the replay, so each reconnect re-delivered it: ~14,000 connect→overflow→reconnect cycles in under an hour (visible 1:1:1 in the client log), pegging one core and starving every terminal. For ephemeral live output, *drop* under overload (the app repaints); reserve reconnect for genuine transport loss, and never make the overload remedy re-pull the same bytes.
 - [From 1052] A PTY-side SIGWINCH redraws the running app's *current frame* but cannot re-wrap xterm.js's existing *scrollback* — so it fixes a blank/stale *live* frame (#1047) but not wrong-width *history* (#1052). #1052's corruption was the inverse failure: VSCode reports a new terminal's size in two steps (e.g. 112→114 cols, ~120ms apart), and the adapter painted the bracketed replay immediately at the first, not-yet-final width, so the restored history wrapped wrong and a stale frame stranded in scrollback (a ghost status bar, visible on scroll). The #1047 post-connect SIGWINCH nudge couldn't clear it (redraws the app, not the scrollback), and an `onDidOverrideDimensions` shrink-then-restore reflow made it *worse* — a down-then-up re-wrap churns scrollback wrap flags → scroll distortion. The fix again mirrored the web client (`Terminal.tsx` `flushInitialBuffer`): hold the replay and paint it **once after the size settles**, debounced on `setDimensions`, so it lands at the final width. Captured per-pathway diagnostic logging was decisive — four prior approaches (defer-until-sized, post-replay SIGWINCH, override reflow) were each tried and reverted before the log showed a mid-hold 112→114 resize resetting the debounce and the flush landing after it at the final width. Don't render a full-screen replay until the terminal geometry has stopped moving.
+- [From #1227] `parseInt(env.X || 'default', 10) || fallbackDefault` silently discards a legitimate `0` override — `0` is falsy in JS, so `0 || fallbackDefault` evaluates to `fallbackDefault`, not `0`. Check `Number.isNaN(parsed) ? fallbackDefault : parsed` instead. Caught only because an E2E test deliberately set an env var to `'0'` (for an immediate-eligibility grace period) and the periodic timer silently kept using the 1-hour default instead — a live, running-process assertion surfaced it where a unit test mocking the parse wouldn't have.
+
 - [From 787] When adding a field to a multi-forge concept contract (`pr-list`, `issue-list`, …), per-CLI data availability diverges — do not assume parity across `gh`/`glab`/`tea`. Verify each empirically (live output or, failing that, the CLI's own `--help` and official docs): `gh pr list --json` exposes `isDraft` + `reviewRequests` (reviewer objects → flatten `.login`, dropping teams); `glab mr list --output json` exposes `draft` + `reviewers[].username` (same command, just add jq); but `tea pulls list` exposes *neither* (its JSON is limited to the selectable `--fields`, which omit draft/reviewers — the data exists only via raw `tea api`). Populate where the existing command can; default safely (`[]`/`false`) where it can't, and document the verified reason in the script. Defaulting a field the CLI *does* expose silently drops working data; rewriting a script onto a different command (e.g. `tea api`) to reach an unavailable field is a separate, larger change. Make the new required fields safe end-to-end by defaulting at the server mapping (`?? []` / `?? false`) so a non-conforming forge degrades rather than emitting `undefined`.
 
 - [From 863] React's `dangerouslySetInnerHTML` re-commits the element's innerHTML on *every* re-render, silently wiping any DOM children you injected imperatively into that same element. The artifact-canvas renders parsed markdown into the body via `dangerouslySetInnerHTML`, then injected inline comment cards as DOM children of the parsed blocks; the cards flashed on first paint and vanished on the next render (a refreshKey bump), while the React-owned minimap survived untouched — that asymmetry located the bug in the React-owned subtree, not the injection logic (a faithful jsdom repro of the injection round-trip kept the card every time, proving the defect was browser-render-only). Fix: stop letting React own that subtree — set `ref.innerHTML = html` imperatively inside a `[html]`-keyed `useEffect`, then inject the non-React DOM after, so React never re-commits those children (the standard escape hatch for mixing hand-built DOM into a React tree). Rule of thumb: never imperatively mutate the children of a node React controls via `dangerouslySetInnerHTML` — render everything through React, or own the whole subtree imperatively, never both.