diff --git a/codev/plans/1227-codev-process-fleet-drives-mac.md b/codev/plans/1227-codev-process-fleet-drives-mac.md new file mode 100644 index 000000000..8888a40b4 --- /dev/null +++ b/codev/plans/1227-codev-process-fleet-drives-mac.md @@ -0,0 +1,295 @@ +# PIR Plan: Shellper Husk Sweep + Fleet Memory Observability + +## Scope note + +Per architect instruction, this PIR covers proposal items **1** (husk sweep) and **3** +(observability) from issue #1227. Item **2** (idle-architect hibernation / stop-on-idle +memory policy) is design-level and explicitly excluded — flagged below as a follow-up +spec, not implemented here. + +## Understanding + +Issue #1227 documents two related findings from a live memory-pressure census: + +1. **Stranded shellper husks accumulate monotonically.** `killOrphanedShellpers()` + (`packages/codev/src/terminal/session-manager.ts:717-761`) runs once, at Tower + startup only (called from `packages/codev/src/agent-farm/servers/tower-server.ts:487`, + right after `reconcileTerminalSessions()` at `:481`). Its predicate: find + `shellper-main.js` processes scoped to this instance's `socketDir` + (`findShellperProcesses`, `session-manager.ts:773-798`) that aren't in the in-memory + `sessions` map, then **skip the kill unconditionally if the shellper's Unix socket + responds** (`:733-738`) — the reasoning being "a responsive socket means a live + session Tower lost track of." A husk (PTY child exited, shellper still listening — + this is deliberate lingering behavior from #905/#1198 so a reconnect can replay + history) still answers its socket, so this check protects it **permanently**. Every + Tower restart that fails to re-adopt a shellper (crash, `remove-architect` outside + #1225's coverage, adoption races, etc.) stamps a husk that then survives forever — + confirmed by the issue's census showing two age-cohorts of childless husks dating to + two known restart events (2026-07-16, 2026-07-19), each never cleaned up since. + +2. **No visibility into fleet memory cost.** `afx status`'s `Memory` field + (`packages/codev/src/agent-farm/commands/status.ts:203`) reports Tower's own process + heap (`process.memoryUsage().heapUsed`, sourced from `GET /health` at + `packages/codev/src/agent-farm/servers/tower-routes.ts:311`) — not the RSS of the + shellper/claude fleet Tower manages, which is the actual multi-GB load driving OS + memory pressure. There is no `ps`-based RSS accounting anywhere in the codebase + today, and no signal that flags unregistered (husk) shellpers short of a manual `ps` + diff against the DB. + +Root cause for (1): `killOrphanedShellpers`'s policy conflates "responsive socket" with +"legitimate," when a childless-but-responsive shellper is exactly the failure mode to +catch. The fix (per the issue's own framing, confirmed correct) is to add a second, +stricter reap pass whose predicate is **unregistered AND childless AND aged past a grace +period** — the aged condition is what makes it race-safe (a shellper mid-adoption, or +momentarily childless between PTY respawns, cannot be both unregistered *and* childless +*and* old, since a legitimately-tracked shellper is either registered or freshly +orphaned, not both stale and invisible to the DB for hours). + +## Proposed Change + +### 1. A shared process-census helper + +New file `packages/codev/src/agent-farm/servers/process-census.ts`: + +```ts +export interface ProcessCensusEntry { pid: number; ppid: number; rssKb: number; cmdline: string; } +export function listProcessCensus(): ProcessCensusEntry[] +``` + +One `execFileSync('ps', ['-A', '-ww', '-eo', 'pid=,ppid=,rss=,args='], { timeout, maxBuffer })` +call, following the exact convention already used by `listProcessEntries` in +`packages/codev/src/agent-farm/servers/architect-session-holder.ts:113-129` (`-ww` to +avoid argv truncation, `execFileSync` never a shell string, try/catch-degrade at the +call site rather than inside). This single scan backs both the husk-sweep predicate and +the RSS observability feature, so we don't grow a fourth bespoke `ps` caller (there are +already three: `session-manager.ts`, `architect-session-holder.ts`, +`agent-farm/commands/cleanup.ts`). + +### 2. A stricter husk-sweep predicate + reap + +New file `packages/codev/src/agent-farm/servers/shellper-husk-sweep.ts`: + +```ts +export function findHuskShellpers(opts: { + socketDir: string; + registeredShellperPids: Set; + graceMs: number; + census?: () => ProcessCensusEntry[]; // seam, defaults to listProcessCensus + now?: number; // seam, defaults to Date.now() + getStartTime?: (pid: number) => Promise; // seam, defaults to getProcessStartTime +}): Promise + +export async function sweepShellperHusks(opts: { + socketDir: string; + db: Database.Database; // for the registered-pid lookup + graceMs: number; + log?: (msg: string) => void; +}): Promise<{ swept: number; pids: number[] }> +``` + +Predicate, applied to each `shellper-main.js` process scoped to `socketDir` (same +scope-marker technique as `findShellperProcesses`, `session-manager.ts:782-793` — match +`socketDir + '/'` in the argv to prevent prefix collisions and to never touch another +Tower instance's or another user's shellpers): + +- **unregistered** — pid not in `registeredShellperPids`, computed from + `SELECT shellper_pid, shellper_start_time FROM terminal_sessions WHERE shellper_pid IS NOT NULL` + (`packages/codev/src/agent-farm/db/schema.ts:118-131`) with each row's `shellper_pid` + validated against `getProcessStartTime(pid)` (already exists, + `session-manager.ts:1242` and exported) matching `shellper_start_time` — the same + PID-reuse defense `_reconcileTerminalSessionsInner()` already applies + (`packages/codev/src/agent-farm/servers/tower-terminals.ts:604`). A stale DB row whose + PID was reused by an unrelated process does not count as "registered." +- **childless** — no other census entry has `ppid === pid` (the OS-level signal; no + wire-protocol "has-child" frame exists in `shellper-protocol.ts`, so this must come + from the process table, cross-referenced within the same census snapshot used for the + scope match). +- **aged** — `now - startTime >= graceMs`, using `getProcessStartTime` (existing helper, + reused rather than reimplemented) with a default `graceMs` of 1 hour (env-overridable + `SHELLPER_HUSK_GRACE_MS`, same override convention as + `SHELLPER_CLEANUP_INTERVAL_MS`). One hour is far beyond any respawn/adoption window + (seconds), so this only ever protects a transient, not a real husk — per the issue's + own analysis, "unregistered and childless" is a *terminal* state (a shellper that lost + its registration can never receive another SPAWN), so the grace period's only job is + race-safety, not "might still be needed." + +Reap itself reuses `reapShellpers` from +`packages/codev/src/agent-farm/servers/architect-session-holder.ts:239-278` (SIGTERM → +poll-for-death → SIGKILL → confirm-death) — already tested, already correct about +process-group semantics and the SIGKILL-confirmation race fixed during #1224's review. +No new kill logic; `shellper-husk-sweep.ts` only computes candidate PIDs and delegates +the reap. + +**Why a new function instead of extending `killOrphanedShellpers` in place:** that +function's existing tests (`session-manager.test.ts:383-620`) pin today's "responsive +socket always protects" behavior, and it's called from a single startup-only site. A +separate, stricter sweep is safer to add, test, and roll back independently, while still +functionally "extending its policy" at the product level (per the issue's own framing) — +the two coexist as a permissive first pass (`killOrphanedShellpers`) and a stricter +second pass (`sweepShellperHusks`) covering exactly the gap the first one leaves open. + +### 3. Three triggers, one sweep function + +- **(a) Startup** — call `sweepShellperHusks()` in `tower-server.ts` immediately after + the existing `killOrphanedShellpers()` call (`tower-server.ts:487-490`), same ordering + requirement (must run after `reconcileTerminalSessions()`). +- **(b) Hourly periodic** — new `setInterval` in `tower-server.ts`, following the + existing `shellperCleanupInterval` pattern exactly (module-level handle declared + alongside it near `:67`, env-overridable period via `SHELLPER_HUSK_SWEEP_INTERVAL_MS` + defaulting to 3,600,000ms and floored the same way `SHELLPER_CLEANUP_INTERVAL_MS` is + at `:412`, started next to the existing interval at `:411-422`, and added to + `gracefulShutdown`'s clear-list at `:167` alongside `shellperCleanupInterval`). Tests + can shorten the interval via the env var the same way + `shellper-cleanup.e2e.test.ts:20` does for the existing cleanup timer. +- **(c) On-demand** — new Tower API endpoint `POST /api/shellpers/sweep-husks`, added to + the exact-match `ROUTES` table in `packages/codev/src/agent-farm/servers/tower-routes.ts:161-185`. + Exposed via a new `afx tower sweep-husks` CLI command + (`packages/codev/src/agent-farm/cli.ts`, added to the existing `towerCmd` group + alongside `start`/`stop`/`log` at `:729-783`), implemented in a new + `packages/codev/src/agent-farm/commands/tower-sweep-husks.ts` via a new + `TowerClient.findHuskCandidates()` / `TowerClient.sweepHusks()` pair + (`packages/core/src/tower-client.ts`, next to `getHealth()` at `:197-200`). + **Chose `afx tower` over `codev doctor`**: `doctor.ts` today is a pure + environment/dependency checker (Node/git/CLI versions, AI-model auth) with zero Tower + or process coupling (`packages/codev/src/commands/doctor.ts`) — wiring a Tower-client + dependency into it is a bigger shift than adding one command to `afx tower`, which + already speaks Tower's HTTP API for every other subcommand. + + **Dry-run by default, mirroring `afx workspace recover`** + (`packages/codev/src/agent-farm/commands/workspace-recover.ts`) exactly — same UX + precedent already established for a destructive, irreversible fleet-wide operation: + - No flags → **preview only**. The endpoint splits into two: + `POST /api/shellpers/sweep-husks/preview` (or a `?dryRun=1`-style single endpoint — + finalize the exact shape in implement) runs `findHuskShellpers()` and returns + candidates (pid, socketPath/cwd if resolvable, age, rss) **without killing + anything**. The CLI renders them in a table (same `logger.row`/`printPreview` idiom + as `workspace-recover.ts:255-276`) and prints + `"Run with --apply to reap N husk shellper(s)."` (mirrors `workspace-recover.ts:406`). + - `--apply` — actually reaps the previewed candidates via `sweepShellperHusks()` + (SIGTERM→poll→SIGKILL through the existing `reapShellpers`). + - `-y, --yes` — skip the confirmation prompt when `--apply` is set (mirrors + `workspace-recover.ts:410-416`'s `--apply` + `!options.yes` + `confirm(...)` gate, + reusing the same `confirm()` helper from `packages/codev/src/lib/cli-prompts.ts`). + - Without `--yes`, `--apply` still prompts + `"Proceed to reap N husk shellper(s)?"` before killing, defaulting to **no** + (`confirm(question, false)`, same default-false the recover command uses for a + destructive default). + - The **startup and hourly triggers are unaffected** by this — they have no interactive + operator to prompt, so they call `sweepShellperHusks()` directly (apply-equivalent, + log-only, as designed above). The dry-run/`--apply` gate is specific to the one + trigger a human actually invokes. + +### 4. Observability: fleet RSS + unregistered-shellper count + +- Extend the `/health` handler (`tower-routes.ts:297-315`) to add two fields, computed + from one `listProcessCensus()` call scoped to this instance's `socketDir`: + - `fleetRssKb` — sum of RSS for every in-scope `shellper-main.js` process plus its + direct children (`ppid` membership within the same census snapshot) — i.e. the full + process-group tree Tower manages, **regardless of DB-registration state**, so a husk + that hasn't been swept yet still counts toward the visible cost (the whole point is + surfacing the true load before or independent of reaping it). + - `unregisteredShellperCount` — count of in-scope shellper PIDs not in the registered + set (the same lookup `sweepShellperHusks` uses), **without** the childless/aged + gating — a lighter, purely informational signal ("N shellpers Tower doesn't + currently track"), distinct from the stricter reap predicate. +- Add both fields to the `TowerHealth` interface (`packages/core/src/tower-client.ts:73-88`). +- Surface in `afx status`: two new `logger.kv` lines next to the existing `Memory` line + (`packages/codev/src/agent-farm/commands/status.ts:203`), and in the `--json` output + via `emitStatusJson` (`status.ts:117-151`). + +### Out of scope (follow-up) + +Idle-architect hibernation (issue's proposal item 2) is a separate, design-level change +(kill an idle architect's `claude` child after N hours, keep the shellper + persisted +session id for instant resume) — not implemented here. Recommend filing it as its own +spec once this observability lands, since `fleetRssKb`/`unregisteredShellperCount` will +make the cost/benefit of a hibernation policy measurable. + +## Files to Change + +- `packages/codev/src/agent-farm/servers/process-census.ts` — new. `listProcessCensus()`. +- `packages/codev/src/agent-farm/servers/shellper-husk-sweep.ts` — new. + `findHuskShellpers()`, `sweepShellperHusks()`. +- `packages/codev/src/agent-farm/__tests__/process-census.test.ts` — new. +- `packages/codev/src/agent-farm/__tests__/shellper-husk-sweep.test.ts` — new. +- `packages/codev/src/agent-farm/servers/tower-server.ts` — startup sweep call + (near `:487-490`), new periodic-timer handle + `setInterval` (near `:67`, `:411-422`), + add to `gracefulShutdown` clear-list (`:167`). +- `packages/codev/src/agent-farm/servers/tower-routes.ts` — new husk-preview/sweep + route(s) (`:161-185`) + handler(s); extend `handleHealthCheck` (`:297-315`) with + `fleetRssKb`/`unregisteredShellperCount`. +- `packages/core/src/tower-client.ts` — extend `TowerHealth` (`:73-88`); add + `findHuskCandidates()` (preview) and `sweepHusks()` (apply) methods (near `:197-200`). +- `packages/codev/src/agent-farm/cli.ts` — new `afx tower sweep-husks` command + (near `:729-783`), with `--apply` / `-y, --yes` options matching + `workspace recover`'s (`:137-159`). +- `packages/codev/src/agent-farm/commands/tower-sweep-husks.ts` — new. CLI + implementation: preview table by default, `--apply` to reap, `confirm()` gate unless + `--yes` — structured like `workspace-recover.ts:316-438`. +- `packages/codev/src/agent-farm/commands/status.ts` — surface new fields in human + output (`:203`) and JSON (`:117-151`). +- `packages/codev/src/terminal/session-manager.ts:1098` — the existing comment there + already claims "the periodic orphan sweep SIGKILLs any survivor"; once the real + periodic sweep lands this becomes true — reconcile the wording if it drifts from what + actually ships. + +## Risks & Alternatives Considered + +- **Risk: reaping a shellper that's legitimately mid-respawn or mid-adoption.** + Mitigation: the aged condition (default 1 hour, env-overridable) is far beyond any + respawn/adoption window (seconds); reuse of the already-tested `reapShellpers` + SIGTERM→poll→SIGKILL sequence rather than a raw `process.kill`. +- **Risk: multi-Tower-instance / multi-user safety.** A naive `ps` match on + `shellper-main.js` alone would sweep another Tower instance's (or another user's) + legitimate shellpers. Mitigation: reuse the exact `socketDir`-scope-marker technique + already proven in `findShellperProcesses` (`session-manager.ts:782-793`) — same + trailing-`/` prefix-collision guard. +- **Risk: DB registry race at the exact moment a new shellper is spawned but not yet + written to `terminal_sessions`.** Mitigation: absorbed by the aged condition — a + freshly-spawned shellper cannot be both unregistered and old. +- **Risk: `fleetRssKb` double-counts or under-scopes "fleet."** Mitigation: explicitly + defined and documented as "in-scope shellpers + their direct children," computed from + a single census snapshot; the field is advisory/observational only, not a gate on any + behavior. +- **Alternative: extend `killOrphanedShellpers` in place** rather than adding a sibling + function. Rejected — see "Why a new function" above; changing the well-tested existing + reap in place risks regressing its current (and still-needed) "responsive socket + protects a genuinely-live session" behavior. +- **Alternative: put the on-demand trigger in `codev doctor`.** Rejected — `doctor.ts` + has no Tower/process coupling today; `afx tower` already does, for every other + subcommand. + +## Test Plan + +- **Unit — `process-census.ts`**: mock `execFileSync`; verify pid/ppid/rss/args parsing, + malformed-line skipping, and empty-output handling (mirrors the pattern in + `cleanup-shellper-kill.test.ts`, which mocks `node:child_process` directly). +- **Unit — `findHuskShellpers`**: independently test each predicate leg — + registered-but-childless-and-aged → not swept; unregistered-but-has-a-child → not + swept; unregistered-and-childless-but-young → not swept; unregistered AND childless + AND aged → swept. Also test `socketDir` scope isolation (a matching process from a + different instance's `socketDir` is never a candidate). Follow the injectable-seam + style already used in `architect-session-holder.test.ts` (`list`/`census` seam) rather + than mocking `execFileSync` directly. +- **Unit — `sweepShellperHusks`**: DB-registry lookup + PID-reuse defense (a stale row + whose PID was reused by an unrelated process must not count as registered). +- **Integration — periodic timer**: adapt the `shellper-cleanup.e2e.test.ts` pattern — + set `SHELLPER_HUSK_SWEEP_INTERVAL_MS` to a short value via env, start a real Tower, + assert the sweep runs on the shortened interval. +- **Integration — `/health` + `afx status`**: assert `fleetRssKb` and + `unregisteredShellperCount` appear in both the raw `/health` JSON and `afx status + --json` output. +- **Unit — `afx tower sweep-husks` CLI**: no flags → preview table printed, nothing + killed (assert `sweepHusks`/reap seam not invoked); `--apply --yes` → reaps without + prompting; `--apply` alone → prompts via `confirm()`, declining aborts with nothing + killed (mirrors the equivalent `workspace-recover` test coverage pattern). +- **Manual**: spin up Tower locally; deliberately orphan a shellper (kill its PTY child + without killing the shellper, and deregister its DB row); confirm `afx status` reports + it under `unregisteredShellperCount`; run `afx tower sweep-husks` with no flags and + confirm it only previews (process still alive); run again with `--apply` and confirm + the prompt, decline it, confirm it's still alive; run `--apply --yes` and confirm it's + reaped and the count drops; confirm the startup and hourly paths independently exercise + the same reap via their own tests (no dry-run gate on those two). +- **Regression**: existing `killOrphanedShellpers` tests + (`session-manager.test.ts:383-620`) and `shellper-cleanup.e2e.test.ts` continue to pass + unmodified — the new sweep is additive, not a replacement. diff --git a/codev/projects/1227-codev-process-fleet-drives-mac/1227-review-iter1-rebuttals.md b/codev/projects/1227-codev-process-fleet-drives-mac/1227-review-iter1-rebuttals.md new file mode 100644 index 000000000..b0a588fc6 --- /dev/null +++ b/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 -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 diff --git a/codev/projects/1227-codev-process-fleet-drives-mac/status.yaml b/codev/projects/1227-codev-process-fleet-drives-mac/status.yaml new file mode 100644 index 000000000..eccc8c731 --- /dev/null +++ b/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 diff --git a/codev/resources/arch.md b/codev/resources/arch.md index 3290c5066..6cbf4e90c 100644 --- a/codev/resources/arch.md +++ b/codev/resources/arch.md @@ -858,6 +858,8 @@ async cleanupStaleSockets(): Promise { } ``` +**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: diff --git a/codev/resources/lessons-learned.md b/codev/resources/lessons-learned.md index b218af6c2..636d9c46f 100644 --- a/codev/resources/lessons-learned.md +++ b/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. diff --git a/codev/reviews/1227-codev-process-fleet-drives-mac.md b/codev/reviews/1227-codev-process-fleet-drives-mac.md new file mode 100644 index 000000000..d7d9272f7 --- /dev/null +++ b/codev/reviews/1227-codev-process-fleet-drives-mac.md @@ -0,0 +1,82 @@ +# PIR Review: Shellper Husk Sweep + Fleet Memory Observability + +Fixes #1227 + +## Summary + +macOS memory-pressure kills (jetsam) were traced to Codev's own process fleet: shellper wrapper processes whose PTY child already exited ("husks") were permanently unreapable, because the existing `killOrphanedShellpers()` unconditionally protects any shellper whose socket still responds — and a husk's socket always responds. This PR adds a second, stricter sweep (unregistered AND childless AND aged past a grace period) on three triggers (Tower startup, an hourly in-process timer, and an on-demand `afx tower sweep-husks` CLI command with dry-run/`--apply` semantics), plus fleet-wide RSS and unregistered-shellper-count observability in `/health` and `afx status`. Idle-architect hibernation (the issue's proposal item 2) was explicitly excluded per architect scoping and is flagged as a follow-up spec. + +## Files Changed + +- `packages/codev/src/agent-farm/servers/process-census.ts` (+44 / -0) — new +- `packages/codev/src/agent-farm/servers/shellper-husk-sweep.ts` (+173 / -0) — new +- `packages/codev/src/agent-farm/commands/tower-sweep-husks.ts` (+92 / -0) — new +- `packages/codev/src/agent-farm/servers/tower-server.ts` (+36 / -0) +- `packages/codev/src/agent-farm/servers/tower-routes.ts` (+137 / -1) +- `packages/codev/src/terminal/session-manager.ts` (+21 / -2) +- `packages/core/src/tower-client.ts` (+55 / -0) +- `packages/codev/src/agent-farm/lib/tower-client.ts` (+3 / -0) +- `packages/codev/src/agent-farm/cli.ts` (+16 / -0) +- `packages/codev/src/agent-farm/commands/status.ts` (+29 / -2) +- `packages/codev/src/agent-farm/__tests__/process-census.test.ts` (+79 / -0) — new +- `packages/codev/src/agent-farm/__tests__/shellper-husk-sweep.test.ts` (+190 / -0) — new +- `packages/codev/src/agent-farm/__tests__/shellper-husk-sweep.e2e.test.ts` (+177 / -0) — new +- `packages/codev/src/agent-farm/__tests__/tower-routes-husks.e2e.test.ts` (+142 / -0) — new +- `packages/codev/src/agent-farm/__tests__/tower-sweep-husks.test.ts` (+117 / -0) — new +- `packages/codev/src/agent-farm/__tests__/status-fleet-observability.test.ts` (+132 / -0) — new + +16 files changed, 1438 insertions(+), 5 deletions(-). + +## Commits + +- `ff25c080` [PIR #1227] Plan draft +- `ce24667f` [PIR #1227] Plan revised +- `7c01cf97` [PIR #1227] Add process-census + shellper-husk-sweep predicate +- `6a622dbe` [PIR #1227] Wire startup + hourly husk-sweep triggers into Tower +- `f4a9ee56` [PIR #1227] Add husk preview/sweep routes + fleet observability in /health +- `af130187` [PIR #1227] Extend TowerClient with husk preview/sweep + fleet health fields +- `73895847` [PIR #1227] Add afx tower sweep-husks CLI command +- `d2af62fc` [PIR #1227] Surface fleet RSS + unregistered shellper count in afx status +- `69bb42f3` [PIR #1227] Correct stale give-up-reap comment re: husk sweep eligibility + +## Test Results + +- `porch check` (build + tests): ✓ both pass +- `tsc --noEmit` clean across `packages/core` and `packages/codev` +- Unit tests: 98+ passing across `process-census.test.ts`, `shellper-husk-sweep.test.ts`, `tower-sweep-husks.test.ts`, `status-fleet-observability.test.ts`, plus the full existing `session-manager.test.ts` regression suite (79 tests, unmodified behavior) +- E2E (real Tower process, real `ps`/signals, no mocks): + - `shellper-husk-sweep.e2e.test.ts` — creates a genuine husk (deletes its `terminal_sessions` row, kills its PTY child directly), confirms the periodic timer reaps it on the next tick; separately confirms graceful shutdown clears the new interval without hanging + - `tower-routes-husks.e2e.test.ts` — confirms `GET /api/shellpers/husks` lists a genuine husk without touching it, `/health` reports it under `unregisteredShellperCount`, and `POST /api/shellpers/husks/sweep` then actually reaps it + - Existing `shellper-cleanup.e2e.test.ts` regression suite: unmodified, still passing +- Manual: `afx tower sweep-husks` (preview and `--apply`/`-y`), `afx status` (human + `--json`) reviewed at the `dev-approval` gate + +## Architecture Updates + +Added a "Husk sweep (Issue #1227)" paragraph to `codev/resources/arch.md`'s "Orphan Session Detection" section — the existing text there stated shellpers are "never killed" during the SQLite sweep, which is no longer the complete picture now that a second, stricter sweep exists for exactly the case that policy leaves permanently unreaped. Routed to the **cold** tier (`arch.md`), not `arch-critical.md`: this is subsystem-specific mechanism detail, not a cross-cutting fact that should change unrelated implementation decisions, and the hot tier's existing "Agent Farm Internals" map entry already points here. + +## Lessons Learned Updates + +Added one entry to `codev/resources/lessons-learned.md` under "Debugging and Root Cause Analysis" (cold tier — the section already holds this exact class of JS falsy-coercion footgun, e.g. the `[From 0107]` `body && body.name` entry): `parsed || fallbackDefault` silently discards a legitimate `0` override, since `0` is falsy in JS. I hit this for real — `SHELLPER_HUSK_GRACE_MS=0` (set by my own E2E test for an immediate-eligibility grace period) silently reverted to the 1-hour default, and only a real running-process assertion caught it, not a unit test mocking the parse. Fixed with `Number.isNaN(parsed) ? fallbackDefault : parsed` for both the grace-period and sweep-interval env vars (`tower-server.ts`, `shellper-husk-sweep.ts`'s `resolveHuskGraceMs`). + +## Things to Look At During PR Review + +- **Design choice: a sibling function, not an edit to `killOrphanedShellpers`.** I deliberately did not modify the existing, well-tested `killOrphanedShellpers()` in place — its "responsive socket protects" behavior is still correct and needed for the reconnectable case; the new `sweepShellperHusks()` is strictly additive and covers only the gap. Rationale is documented in the plan's Risks & Alternatives section. +- **The `0 || default` bug** (see Lessons Learned above) — worth a close look at `resolveHuskGraceMs()` in `shellper-husk-sweep.ts` and the parallel fix in `tower-server.ts`'s `huskSweepIntervalMs` parsing, since this class of bug is easy to reintroduce. +- **Codex review (iteration 1) found a real defect, fixed in `3a5083e3`: `process-census.ts` originally used `execFileSync`, called synchronously from the `/health` HTTP handler — blocking Tower's entire event loop (all open terminals' WebSocket traffic) for the duration of every `ps` call.** This is a previously-fixed anti-pattern in this exact codebase (`lessons-learned.md:160`), reintroduced by this PR. Rewrote `listProcessCensus()` to use non-blocking `execFile` (matching `session-manager.ts`'s existing async convention), added a regression test (`process-census.test.ts`: `'never calls the synchronous execFileSync API'`) that fails against the pre-fix code. Full detail and a second, related fix (a redundant double `ps` scan in `handleHuskPreview`, also flagged independently by claude's own review) in `codev/projects/1227-codev-process-fleet-drives-mac/1227-review-iter1-rebuttals.md`. Per PIR's single-pass design this fix was **not** independently re-reviewed by either model — please verify it at this gate. +- **`afx tower sweep-husks` UX** mirrors `afx workspace recover` exactly (dry-run default, `--apply`, `-y`/`--yes`, same `confirm()` helper) per explicit architect direction mid-plan-review — not an independent design choice, so please check it actually matches that precedent rather than just looking reasonable in isolation. +- **`fleetRssKb` scope**: sums every in-scope shellper *and its direct children* (the full process-group tree), computed from one `ps` scan, regardless of DB-registration state — so a not-yet-swept husk still counts toward the visible number. This is explicitly advisory/observational, not used anywhere as a decision input (confirmed in conversation during the dev-approval gate review). +- **PID-reuse defense**: `computeRegisteredShellperPids` validates each DB row's `shellper_pid` against the live process's actual start time (2000ms tolerance, same constant `SessionManager.reconnectSession` already uses) before counting it as registered — a stale row whose PID was reused by an unrelated process correctly falls through to "unregistered." + +## How to Test Locally + +- **View diff**: VSCode sidebar → right-click builder pir-1227 → **Review Diff** +- **Run dev**: `afx dev pir-1227` +- **What to verify**: + - `afx tower sweep-husks` with no flags previews only (nothing killed); `--apply` alone prompts for confirmation; `--apply --yes` reaps without prompting + - `afx status` shows `Fleet RSS` and `Unregistered Shellpers` lines when Tower is running; `afx status --json` carries a `fleet: { rssKb, unregisteredShellperCount }` field (explicit `null`s, not omitted keys, when Tower is down) + - `GET /health` on a running Tower includes `fleetRssKb` and `unregisteredShellperCount` + - Tower starts and stops cleanly with the new interval in place (`SIGTERM` exits promptly, no hang) + +## Flaky Tests + +None encountered. diff --git a/packages/codev/src/agent-farm/__tests__/process-census.test.ts b/packages/codev/src/agent-farm/__tests__/process-census.test.ts new file mode 100644 index 000000000..0c9a50533 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/process-census.test.ts @@ -0,0 +1,121 @@ +/** + * Issue #1227: process-census.ts — the shared ps snapshot backing both the + * shellper husk sweep and fleet-RSS observability. + * + * Mocks `execFile`'s callback form directly (not `util.promisify`), matching + * cleanup-shellper-kill.test.ts's established pattern for this codebase's + * async-execFile convention. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { execFile, execFileSync } from 'node:child_process'; +import { listProcessCensus } from '../servers/process-census.js'; + +vi.mock('node:child_process', async () => { + const actual = await vi.importActual('node:child_process'); + return { + ...actual, + execFile: vi.fn(), + execFileSync: vi.fn(), + }; +}); + +const mockExecFile = vi.mocked(execFile); +const mockExecFileSync = vi.mocked(execFileSync); + +function simulatePsOutput(stdout: string): void { + mockExecFile.mockImplementation((_cmd, _args, _opts, callback) => { + (callback as (err: Error | null, stdout: string) => void)(null, stdout); + return {} as ReturnType; + }); +} + +function simulatePsError(): void { + mockExecFile.mockImplementation((_cmd, _args, _opts, callback) => { + (callback as (err: Error | null, stdout: string) => void)(new Error('ps failed'), ''); + return {} as ReturnType; + }); +} + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe('listProcessCensus (Issue #1227)', () => { + it('parses pid, ppid, rss, and full argv from ps output', async () => { + simulatePsOutput( + '12345 1 34816 node /opt/codev/dist/terminal/shellper-main.js {"cwd":"/ws"}\n' + + ' 99 12345 512 /bin/bash -c echo hi\n', + ); + + const entries = await listProcessCensus(); + + expect(entries).toEqual([ + { pid: 12345, ppid: 1, rssKb: 34816, cmdline: 'node /opt/codev/dist/terminal/shellper-main.js {"cwd":"/ws"}' }, + { pid: 99, ppid: 12345, rssKb: 512, cmdline: '/bin/bash -c echo hi' }, + ]); + }); + + it('invokes ps with -A -ww -eo pid=,ppid=,rss=,args= for full, untruncated argv', async () => { + simulatePsOutput(''); + + await listProcessCensus(); + + expect(mockExecFile).toHaveBeenCalledWith( + 'ps', + ['-A', '-ww', '-eo', 'pid=,ppid=,rss=,args='], + expect.objectContaining({ timeout: 5000 }), + expect.any(Function), + ); + }); + + it('skips malformed lines rather than throwing', async () => { + simulatePsOutput( + 'not-a-pid-line\n' + + '12345 1 34816 node /path/to/thing\n' + + '\n', + ); + + const entries = await listProcessCensus(); + + expect(entries).toEqual([ + { pid: 12345, ppid: 1, rssKb: 34816, cmdline: 'node /path/to/thing' }, + ]); + }); + + it('returns an empty array for empty ps output', async () => { + simulatePsOutput(''); + + expect(await listProcessCensus()).toEqual([]); + }); + + it('resolves to an empty array (never rejects) when ps itself fails', async () => { + simulatePsError(); + + await expect(listProcessCensus()).resolves.toEqual([]); + }); + + it('preserves a JSON-blob argv containing many spaces', async () => { + const argv = 'node shellper-main.js {"cwd":"/ws","env":{"PATH":"/usr/bin:/bin"},"args":["a","b"]}'; + simulatePsOutput(` 555 1 1024 ${argv}\n`); + + const entries = await listProcessCensus(); + + expect(entries).toEqual([{ pid: 555, ppid: 1, rssKb: 1024, cmdline: argv }]); + }); + + // Regression (codex #1227 PR review): listProcessCensus() is called from + // handleHealthCheck() on the `/health` HTTP path. A synchronous execFileSync + // there blocks Tower's entire event loop — freezing every open terminal's + // WebSocket traffic — for the duration of the `ps` call, on every request. + // This pins the non-blocking contract structurally (never invoking the sync + // API) rather than via a timing assertion, which would be flaky. + it('never calls the synchronous execFileSync API', async () => { + simulatePsOutput('12345 1 34816 node shellper-main.js\n'); + + await listProcessCensus(); + + expect(mockExecFileSync).not.toHaveBeenCalled(); + expect(mockExecFile).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/shellper-husk-sweep.e2e.test.ts b/packages/codev/src/agent-farm/__tests__/shellper-husk-sweep.e2e.test.ts new file mode 100644 index 000000000..b50211993 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/shellper-husk-sweep.e2e.test.ts @@ -0,0 +1,177 @@ +/** + * Issue #1227: E2E tests for the periodic husk-sweep timer wired into + * tower-server.ts. Unit coverage for the predicate itself + * (findHuskShellpers/computeRegisteredShellperPids/sweepShellperHusks) lives + * in shellper-husk-sweep.test.ts with injected seams; this file exercises the + * real setInterval + real process signals a unit test can't reach. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { resolve } from 'node:path'; +import { homedir } from 'node:os'; +import Database from 'better-sqlite3'; +import type { TowerHandle } from './helpers/tower-test-utils.js'; +import { startTower, cleanupAllTerminals, cleanupTestDb } from './helpers/tower-test-utils.js'; + +const TEST_TOWER_PORT = 14710; +// Short interval + zero grace so a genuine husk (created below) is reaped on +// the very next tick, without waiting anywhere near the 1-hour production default. +const HUSK_SWEEP_INTERVAL_MS = '1500'; +const HUSK_GRACE_MS = '0'; + +let tower: TowerHandle; + +async function waitForShellperReady(port: number, timeoutMs = 10_000): Promise { + const start = Date.now(); + while (Date.now() - start < timeoutMs) { + try { + const res = await fetch(`http://localhost:${port}/api/terminals`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + command: '/bin/echo', args: ['ready-probe'], cwd: '/tmp', + label: 'readiness-probe', persistent: true, + }), + }); + if (res.ok) { + const data = await res.json(); + if (data.persistent) { + await fetch(`http://localhost:${port}/api/terminals/${data.id}`, { method: 'DELETE' }); + return; + } + } + } catch { /* server not ready yet */ } + await new Promise((r) => setTimeout(r, 200)); + } + throw new Error(`Shellper manager not ready within ${timeoutMs}ms`); +} + +/** + * Creates a persistent terminal AND registers it in terminal_sessions — + * unlike shellper-cleanup.e2e.test.ts's bare `persistent:true` (which spawns a + * shellper but skips the DB row, see tower-routes.ts:627-638), this test needs + * a real row to delete, so `workspacePath`/`type`/`roleId` must be present. + * + * The create response's own `pid` field is a stale snapshot (-1) taken before + * shellper attach populates it — a separate GET, same as + * shellper-cleanup.e2e.test.ts's `termInfo.pid`, is required for the real + * shellper PID. + */ +async function createPersistentTerminal(port: number, label: string): Promise<{ id: string; pid: number }> { + const res = await fetch(`http://localhost:${port}/api/terminals`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + command: '/bin/sleep', args: ['300'], cwd: '/tmp', label, persistent: true, + workspacePath: '/tmp', type: 'shell', roleId: label, + }), + }); + expect(res.status).toBe(201); + const created = await res.json(); + + const infoRes = await fetch(`http://localhost:${port}/api/terminals/${created.id}`); + expect(infoRes.ok).toBe(true); + const info = await infoRes.json(); + return { id: created.id, pid: info.pid }; +} + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +describe('Issue #1227: husk-sweep periodic timer E2E', () => { + beforeAll(async () => { + tower = await startTower(TEST_TOWER_PORT, { + SHELLPER_HUSK_SWEEP_INTERVAL_MS: HUSK_SWEEP_INTERVAL_MS, + SHELLPER_HUSK_GRACE_MS: HUSK_GRACE_MS, + }); + await waitForShellperReady(TEST_TOWER_PORT); + }, 30_000); + + afterAll(async () => { + if (!tower) return; + await cleanupAllTerminals(TEST_TOWER_PORT); + await tower.stop(); + cleanupTestDb(TEST_TOWER_PORT); + }); + + it('reaps a genuine husk (unregistered + childless) on the next periodic tick', async () => { + // Create a real persistent terminal: a live shellper with a live PTY child. + const terminal = await createPersistentTerminal(TEST_TOWER_PORT, 'husk-sweep-e2e'); + const shellperPid = terminal.pid; + expect(isAlive(shellperPid)).toBe(true); + + // Make it "unregistered": delete its terminal_sessions row directly, the + // same divergence a Tower restart's reconciliation produces when it can't + // re-adopt a shellper (the exact scenario Issue #1227 describes) — the + // shellper survives the deletion because SessionManager never disconnects + // it just because a DB row disappeared. + const dbPath = resolve(homedir(), '.agent-farm', `test-${TEST_TOWER_PORT}.db`); + const db = new Database(dbPath); + db.pragma('busy_timeout = 5000'); + const deleted = db.prepare('DELETE FROM terminal_sessions WHERE shellper_pid = ?').run(shellperPid); + db.close(); + expect(deleted.changes).toBe(1); + + // Make it "childless": kill the PTY child (not the shellper itself). The + // shellper's deliberate post-#905/#1198 lingering behavior means it stays + // up and keeps answering its socket after its child exits — which is + // exactly what makes it invisible to killOrphanedShellpers(). + const childPids = execFileSync('pgrep', ['-P', String(shellperPid)], { encoding: 'utf-8' }) + .trim() + .split('\n') + .filter(Boolean) + .map((s) => parseInt(s, 10)); + expect(childPids.length).toBeGreaterThan(0); + for (const childPid of childPids) { + try { process.kill(childPid, 'SIGKILL'); } catch { /* already dead */ } + } + + // Unregistered + childless + zero grace: the next periodic tick (≤1.5s) + // must reap the shellper itself. + let stillAlive = true; + for (let i = 0; i < 20; i++) { + await new Promise((r) => setTimeout(r, 500)); + stillAlive = isAlive(shellperPid); + if (!stillAlive) break; + } + expect(stillAlive).toBe(false); + }, 30_000); +}); + +describe('Issue #1227: husk-sweep graceful shutdown', () => { + it('completes without hanging (validates the new interval is cleared)', async () => { + const port = 14711; + const handle = await startTower(port, { + SHELLPER_HUSK_SWEEP_INTERVAL_MS: HUSK_SWEEP_INTERVAL_MS, + SHELLPER_HUSK_GRACE_MS: HUSK_GRACE_MS, + }); + await waitForShellperReady(port); + await createPersistentTerminal(port, 'husk-sweep-shutdown-test'); + await cleanupAllTerminals(port); + + const exitPromise = new Promise((resolvePromise) => { + handle.process.on('exit', (code) => resolvePromise(code)); + }); + handle.process.kill('SIGTERM'); + + const exitCode = await Promise.race([ + exitPromise, + new Promise<'timeout'>((resolvePromise) => setTimeout(() => resolvePromise('timeout'), 5000)), + ]); + + expect(exitCode).not.toBe('timeout'); + + cleanupTestDb(port); + try { + const { rmSync } = await import('node:fs'); + rmSync(handle.socketDir, { recursive: true, force: true }); + } catch { /* ignore */ } + }, 20_000); +}); diff --git a/packages/codev/src/agent-farm/__tests__/shellper-husk-sweep.test.ts b/packages/codev/src/agent-farm/__tests__/shellper-husk-sweep.test.ts new file mode 100644 index 000000000..501695b2c --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/shellper-husk-sweep.test.ts @@ -0,0 +1,190 @@ +/** + * Issue #1227: tests for the stricter husk-sweep predicate (unregistered AND + * childless AND aged) and its orchestration. Uses the injectable-seam style + * from architect-session-holder.test.ts (census/getStartTime/reap seams) + * rather than mocking execFileSync directly, since the predicate logic — not + * the ps invocation itself (covered by process-census.test.ts) — is under test. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { + findHuskShellpers, + computeRegisteredShellperPids, + sweepShellperHusks, + type FindHuskShellpersOptions, +} from '../servers/shellper-husk-sweep.js'; +import type { ProcessCensusEntry } from '../servers/process-census.js'; + +const SOCKET_DIR = '/Users/x/.codev/run'; +const OTHER_SOCKET_DIR = '/Users/x/.codev/run-e2e-test'; + +function shellperEntry(opts: { + pid: number; + ppid?: number; + socketDir?: string; + rssKb?: number; +}): ProcessCensusEntry { + const dir = opts.socketDir ?? SOCKET_DIR; + return { + pid: opts.pid, + ppid: opts.ppid ?? 1, + rssKb: opts.rssKb ?? 34816, + cmdline: `/usr/bin/node /opt/codev/dist/terminal/shellper-main.js {"socketPath":"${dir}/s-${opts.pid}.sock"}`, + }; +} + +function otherEntry(pid: number, ppid: number, cmdline = '/bin/bash -c echo hi'): ProcessCensusEntry { + return { pid, ppid, rssKb: 512, cmdline }; +} + +const ONE_HOUR = 60 * 60 * 1000; +const NOW = 1_800_000_000_000; + +function baseOpts(overrides: Partial = {}): FindHuskShellpersOptions { + return { + socketDir: SOCKET_DIR, + registeredShellperPids: new Set(), + graceMs: ONE_HOUR, + now: NOW, + getStartTime: async () => NOW - 2 * ONE_HOUR, + ...overrides, + }; +} + +describe('findHuskShellpers (Issue #1227)', () => { + it('sweeps a shellper that is unregistered AND childless AND aged', async () => { + const entry = shellperEntry({ pid: 100 }); + const pids = await findHuskShellpers(baseOpts({ census: () => [entry] })); + expect(pids).toEqual([100]); + }); + + it('does not sweep a registered shellper, even if childless and aged', async () => { + const entry = shellperEntry({ pid: 100 }); + const pids = await findHuskShellpers( + baseOpts({ census: () => [entry], registeredShellperPids: new Set([100]) }), + ); + expect(pids).toEqual([]); + }); + + it('does not sweep a shellper that still has a child', async () => { + const shellper = shellperEntry({ pid: 100 }); + const child = otherEntry(200, 100); // ppid=100 → shellper has a live child + const pids = await findHuskShellpers(baseOpts({ census: () => [shellper, child] })); + expect(pids).toEqual([]); + }); + + it('does not sweep an unregistered, childless shellper that is too young', async () => { + const entry = shellperEntry({ pid: 100 }); + const pids = await findHuskShellpers( + baseOpts({ census: () => [entry], getStartTime: async () => NOW - 10_000 }), + ); + expect(pids).toEqual([]); + }); + + it('does not sweep when start time cannot be determined', async () => { + const entry = shellperEntry({ pid: 100 }); + const pids = await findHuskShellpers( + baseOpts({ census: () => [entry], getStartTime: async () => null }), + ); + expect(pids).toEqual([]); + }); + + it('never touches a shellper scoped to a different socketDir', async () => { + const foreign = shellperEntry({ pid: 100, socketDir: OTHER_SOCKET_DIR }); + const pids = await findHuskShellpers(baseOpts({ census: () => [foreign] })); + expect(pids).toEqual([]); + }); + + it('ignores non-shellper processes entirely', async () => { + const claude = otherEntry(100, 1, `/usr/bin/claude --resume abc ${SOCKET_DIR}/decoy`); + const pids = await findHuskShellpers(baseOpts({ census: () => [claude] })); + expect(pids).toEqual([]); + }); + + it('sweeps multiple independent husk candidates in one pass', async () => { + const a = shellperEntry({ pid: 100 }); + const b = shellperEntry({ pid: 101 }); + const registered = shellperEntry({ pid: 102 }); + const pids = await findHuskShellpers( + baseOpts({ census: () => [a, b, registered], registeredShellperPids: new Set([102]) }), + ); + expect(pids.sort()).toEqual([100, 101]); + }); +}); + +function fakeDb(rows: Array<{ shellper_pid: number; shellper_start_time: number | null }>) { + return { + prepare: () => ({ + all: () => rows, + }), + } as unknown as import('better-sqlite3').Database; +} + +describe('computeRegisteredShellperPids (Issue #1227)', () => { + it('registers a pid whose live start time matches the DB row within tolerance', async () => { + const db = fakeDb([{ shellper_pid: 100, shellper_start_time: NOW - 1000 }]); + const getStartTime = async () => NOW - 1000; + const registered = await computeRegisteredShellperPids(db, getStartTime); + expect(registered.has(100)).toBe(true); + }); + + it('excludes a stale row whose PID was reused (start time mismatch)', async () => { + const db = fakeDb([{ shellper_pid: 100, shellper_start_time: NOW - ONE_HOUR }]); + const getStartTime = async () => NOW - 1000; // live process started much later — different process + const registered = await computeRegisteredShellperPids(db, getStartTime); + expect(registered.has(100)).toBe(false); + }); + + it('excludes a row whose PID is no longer alive', async () => { + const db = fakeDb([{ shellper_pid: 100, shellper_start_time: NOW - 1000 }]); + const getStartTime = async () => null; + const registered = await computeRegisteredShellperPids(db, getStartTime); + expect(registered.has(100)).toBe(false); + }); + + it('trusts a legacy row with no recorded start time', async () => { + const db = fakeDb([{ shellper_pid: 100, shellper_start_time: null }]); + const getStartTime = async () => NOW; + const registered = await computeRegisteredShellperPids(db, getStartTime); + expect(registered.has(100)).toBe(true); + }); +}); + +describe('sweepShellperHusks (Issue #1227)', () => { + it('reaps exactly the husk candidates and reports them', async () => { + const husk = shellperEntry({ pid: 100 }); + const db = fakeDb([]); // nothing registered + const reap = vi.fn(async () => {}); + + const result = await sweepShellperHusks({ + socketDir: SOCKET_DIR, + db, + graceMs: ONE_HOUR, + now: NOW, + getStartTime: async () => NOW - 2 * ONE_HOUR, + census: () => [husk], + reap, + }); + + expect(result).toEqual({ swept: 1, pids: [100] }); + expect(reap).toHaveBeenCalledWith([100]); + }); + + it('does not call reap when there is nothing to sweep', async () => { + const db = fakeDb([{ shellper_pid: 100, shellper_start_time: NOW - 1000 }]); + const reap = vi.fn(async () => {}); + + const result = await sweepShellperHusks({ + socketDir: SOCKET_DIR, + db, + graceMs: ONE_HOUR, + now: NOW, + getStartTime: async () => NOW - 1000, + census: () => [shellperEntry({ pid: 100 })], // registered → not a husk + reap, + }); + + expect(result).toEqual({ swept: 0, pids: [] }); + expect(reap).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/status-fleet-observability.test.ts b/packages/codev/src/agent-farm/__tests__/status-fleet-observability.test.ts new file mode 100644 index 000000000..113ebf795 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/status-fleet-observability.test.ts @@ -0,0 +1,132 @@ +/** + * Issue #1227: `afx status` surfaces fleet RSS + unregistered-shellper count + * from Tower's /health payload — human mode (`Fleet RSS`/`Unregistered + * Shellpers` kv lines) and `--json` mode (`fleet` field). + * + * Mirrors the mocking harness from status-naming.test.ts / spec-1057-status-owner.test.ts. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; + +const mockLoadState = vi.fn(); +const mockIsRunning = vi.fn(); +const mockGetHealth = vi.fn(); +const mockGetWorkspaceStatus = vi.fn(); +const mockLoggerKv = vi.fn(); + +vi.mock('../utils/config.js', () => ({ + getConfig: vi.fn(() => ({ workspaceRoot: '/fake/workspace' })), +})); + +vi.mock('../state.js', () => ({ + loadState: (...args: any[]) => mockLoadState(...args), +})); + +vi.mock('../lib/tower-client.js', () => ({ + getTowerClient: () => ({ + isRunning: (...a: any[]) => mockIsRunning(...a), + getHealth: (...a: any[]) => mockGetHealth(...a), + getWorkspaceStatus: (...a: any[]) => mockGetWorkspaceStatus(...a), + }), +})); + +vi.mock('../../lib/config.js', () => ({ + loadConfig: vi.fn(() => ({})), +})); + +vi.mock('../utils/logger.js', () => ({ + logger: { + header: vi.fn(), + success: vi.fn(), + error: vi.fn(), + warn: vi.fn(), + debug: vi.fn(), + info: vi.fn(), + kv: (...args: any[]) => mockLoggerKv(...args), + blank: vi.fn(), + row: vi.fn(), + }, + fatal: vi.fn((msg: string) => { throw new Error(msg); }), +})); + +import { status } from '../commands/status.js'; + +describe('afx status — fleet observability (Issue #1227)', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockLoadState.mockReturnValue({ architect: null, architects: [], builders: [], utils: [], annotations: [] }); + }); + + describe('human mode', () => { + beforeEach(() => { + mockIsRunning.mockResolvedValue(true); + mockGetWorkspaceStatus.mockResolvedValue(null); + }); + + it('shows Fleet RSS and Unregistered Shellpers when Tower reports them', async () => { + mockGetHealth.mockResolvedValue({ + uptime: 100, activeWorkspaces: 1, memoryUsage: 50 * 1024 * 1024, + fleetRssKb: 2_048_000, unregisteredShellperCount: 3, + }); + + await status(); + + const kvCalls = mockLoggerKv.mock.calls; + expect(kvCalls).toContainEqual([' Fleet RSS', '2000MB']); + const unregisteredCall = kvCalls.find((c: any[]) => c[0] === ' Unregistered Shellpers'); + expect(unregisteredCall).toBeDefined(); + expect(String(unregisteredCall![1])).toContain('3'); + }); + + it('omits the fleet lines when the running Tower predates these fields', async () => { + mockGetHealth.mockResolvedValue({ uptime: 100, activeWorkspaces: 1, memoryUsage: 50 * 1024 * 1024 }); + + await status(); + + const kvKeys = mockLoggerKv.mock.calls.map((c: any[]) => c[0]); + expect(kvKeys).not.toContain(' Fleet RSS'); + expect(kvKeys).not.toContain(' Unregistered Shellpers'); + }); + }); + + describe('--json mode', () => { + let logSpy: ReturnType; + + beforeEach(() => { + logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + }); + + afterEach(() => { + logSpy.mockRestore(); + }); + + function parsePayload() { + expect(logSpy).toHaveBeenCalledTimes(1); + return JSON.parse(String(logSpy.mock.calls[0][0])); + } + + it('carries fleet rssKb/unregisteredShellperCount from health when Tower is running', async () => { + mockIsRunning.mockResolvedValue(true); + mockGetWorkspaceStatus.mockResolvedValue(null); + mockGetHealth.mockResolvedValue({ + uptime: 100, activeWorkspaces: 1, memoryUsage: 1024, + fleetRssKb: 512_000, unregisteredShellperCount: 2, + }); + + await status({ json: true }); + + const payload = parsePayload(); + expect(payload.fleet).toEqual({ rssKb: 512_000, unregisteredShellperCount: 2 }); + }); + + it('emits explicit nulls (not omitted keys) when Tower is not running', async () => { + mockIsRunning.mockResolvedValue(false); + + await status({ json: true }); + + const payload = parsePayload(); + expect(Object.prototype.hasOwnProperty.call(payload.fleet, 'rssKb')).toBe(true); + expect(payload.fleet).toEqual({ rssKb: null, unregisteredShellperCount: null }); + }); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/tower-routes-husks.e2e.test.ts b/packages/codev/src/agent-farm/__tests__/tower-routes-husks.e2e.test.ts new file mode 100644 index 000000000..05d0dc66d --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/tower-routes-husks.e2e.test.ts @@ -0,0 +1,142 @@ +/** + * Issue #1227: E2E tests for the on-demand husk routes (GET + * /api/shellpers/husks preview, POST /api/shellpers/husks/sweep apply) and + * the /health fleet-observability fields. Uses a long periodic-sweep interval + * so the timer never races the manual preview/apply calls under test. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import { resolve } from 'node:path'; +import { homedir } from 'node:os'; +import Database from 'better-sqlite3'; +import type { TowerHandle } from './helpers/tower-test-utils.js'; +import { startTower, cleanupAllTerminals, cleanupTestDb } from './helpers/tower-test-utils.js'; + +const TEST_TOWER_PORT = 14712; +// Long enough that the periodic timer never fires during this test file. +const NO_PERIODIC_INTERFERENCE_MS = '3600000'; + +let tower: TowerHandle; + +async function waitForShellperReady(port: number, timeoutMs = 10_000): Promise { + const start = Date.now(); + while (Date.now() - start < timeoutMs) { + try { + const res = await fetch(`http://localhost:${port}/api/terminals`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + command: '/bin/echo', args: ['ready-probe'], cwd: '/tmp', + label: 'readiness-probe', persistent: true, + }), + }); + if (res.ok) { + const data = await res.json(); + if (data.persistent) { + await fetch(`http://localhost:${port}/api/terminals/${data.id}`, { method: 'DELETE' }); + return; + } + } + } catch { /* server not ready yet */ } + await new Promise((r) => setTimeout(r, 200)); + } + throw new Error(`Shellper manager not ready within ${timeoutMs}ms`); +} + +async function createPersistentTerminal(port: number, label: string): Promise<{ id: string; pid: number }> { + const res = await fetch(`http://localhost:${port}/api/terminals`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + command: '/bin/sleep', args: ['300'], cwd: '/tmp', label, persistent: true, + workspacePath: '/tmp', type: 'shell', roleId: label, + }), + }); + expect(res.status).toBe(201); + const created = await res.json(); + const infoRes = await fetch(`http://localhost:${port}/api/terminals/${created.id}`); + const info = await infoRes.json(); + return { id: created.id, pid: info.pid }; +} + +function unregisterAndOrphan(port: number, shellperPid: number): void { + const dbPath = resolve(homedir(), '.agent-farm', `test-${port}.db`); + const db = new Database(dbPath); + db.pragma('busy_timeout = 5000'); + db.prepare('DELETE FROM terminal_sessions WHERE shellper_pid = ?').run(shellperPid); + db.close(); + + const childPids = execFileSync('pgrep', ['-P', String(shellperPid)], { encoding: 'utf-8' }) + .trim() + .split('\n') + .filter(Boolean) + .map((s) => parseInt(s, 10)); + for (const childPid of childPids) { + try { process.kill(childPid, 'SIGKILL'); } catch { /* already dead */ } + } +} + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +describe('Issue #1227: on-demand husk routes + /health fleet fields', () => { + beforeAll(async () => { + tower = await startTower(TEST_TOWER_PORT, { + SHELLPER_HUSK_SWEEP_INTERVAL_MS: NO_PERIODIC_INTERFERENCE_MS, + SHELLPER_HUSK_GRACE_MS: '0', + }); + await waitForShellperReady(TEST_TOWER_PORT); + }, 30_000); + + afterAll(async () => { + if (!tower) return; + await cleanupAllTerminals(TEST_TOWER_PORT); + await tower.stop(); + cleanupTestDb(TEST_TOWER_PORT); + }); + + it('preview lists a genuine husk without killing it, apply then reaps it', async () => { + const terminal = await createPersistentTerminal(TEST_TOWER_PORT, 'routes-e2e'); + const shellperPid = terminal.pid; + expect(isAlive(shellperPid)).toBe(true); + + unregisterAndOrphan(TEST_TOWER_PORT, shellperPid); + // Let the childless state settle before probing. + await new Promise((r) => setTimeout(r, 300)); + + // Preview: must list the candidate but must NOT kill it. + const previewRes = await fetch(`http://localhost:${TEST_TOWER_PORT}/api/shellpers/husks`); + expect(previewRes.status).toBe(200); + const preview = await previewRes.json(); + expect(preview.candidates.map((c: { pid: number }) => c.pid)).toContain(shellperPid); + expect(isAlive(shellperPid)).toBe(true); + + // /health must report it as unregistered too (same registry lookup). + const healthRes = await fetch(`http://localhost:${TEST_TOWER_PORT}/health`); + const health = await healthRes.json(); + expect(health.unregisteredShellperCount).toBeGreaterThanOrEqual(1); + expect(typeof health.fleetRssKb).toBe('number'); + expect(health.fleetRssKb).toBeGreaterThan(0); + + // Apply: must actually reap it. + const sweepRes = await fetch(`http://localhost:${TEST_TOWER_PORT}/api/shellpers/husks/sweep`, { method: 'POST' }); + expect(sweepRes.status).toBe(200); + const sweep = await sweepRes.json(); + expect(sweep.pids).toContain(shellperPid); + + let stillAlive = true; + for (let i = 0; i < 10; i++) { + await new Promise((r) => setTimeout(r, 300)); + stillAlive = isAlive(shellperPid); + if (!stillAlive) break; + } + expect(stillAlive).toBe(false); + }, 30_000); +}); diff --git a/packages/codev/src/agent-farm/__tests__/tower-sweep-husks.test.ts b/packages/codev/src/agent-farm/__tests__/tower-sweep-husks.test.ts new file mode 100644 index 000000000..56184abbc --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/tower-sweep-husks.test.ts @@ -0,0 +1,117 @@ +/** + * Issue #1227: afx tower sweep-husks — dry-run by default, --apply to reap, + * -y/--yes to skip the confirm() prompt, mirroring `afx workspace recover`. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mockIsRunning = vi.fn(); +const mockFindHuskCandidates = vi.fn(); +const mockSweepHusks = vi.fn(); + +vi.mock('../lib/tower-client.js', () => ({ + getTowerClient: () => ({ + isRunning: mockIsRunning, + findHuskCandidates: mockFindHuskCandidates, + sweepHusks: mockSweepHusks, + }), +})); + +const mockConfirm = vi.fn(); +vi.mock('../../lib/cli-prompts.js', () => ({ + confirm: mockConfirm, +})); + +vi.mock('../utils/logger.js', () => ({ + logger: { + header: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + kv: vi.fn(), + blank: vi.fn(), + row: vi.fn(), + }, +})); + +async function loadModule() { + const mod = await import('../commands/tower-sweep-husks.js'); + const { logger } = await import('../utils/logger.js'); + return { towerSweepHusks: mod.towerSweepHusks, logger: logger as unknown as Record> }; +} + +const HUSK = { pid: 100, rssKb: 34816, ageMs: 7_200_000 }; + +describe('towerSweepHusks (Issue #1227)', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockIsRunning.mockResolvedValue(true); + }); + + it('errors without calling anything else when Tower is not running', async () => { + mockIsRunning.mockResolvedValue(false); + const { towerSweepHusks, logger } = await loadModule(); + + await towerSweepHusks({}); + + expect(logger.error).toHaveBeenCalledWith(expect.stringContaining('Tower is not running')); + expect(mockFindHuskCandidates).not.toHaveBeenCalled(); + }); + + it('no flags: previews candidates and never calls sweepHusks', async () => { + mockFindHuskCandidates.mockResolvedValue({ candidates: [HUSK], graceMs: 3_600_000 }); + const { towerSweepHusks, logger } = await loadModule(); + + await towerSweepHusks({}); + + expect(logger.row).toHaveBeenCalled(); + expect(mockSweepHusks).not.toHaveBeenCalled(); + expect(mockConfirm).not.toHaveBeenCalled(); + }); + + it('no flags, zero candidates: reports nothing found and never calls sweepHusks', async () => { + mockFindHuskCandidates.mockResolvedValue({ candidates: [], graceMs: 3_600_000 }); + const { towerSweepHusks, logger } = await loadModule(); + + await towerSweepHusks({}); + + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('No husk shellpers found')); + expect(mockSweepHusks).not.toHaveBeenCalled(); + }); + + it('--apply --yes: reaps without prompting', async () => { + mockFindHuskCandidates.mockResolvedValue({ candidates: [HUSK], graceMs: 3_600_000 }); + mockSweepHusks.mockResolvedValue({ swept: 1, pids: [100] }); + const { towerSweepHusks } = await loadModule(); + + await towerSweepHusks({ apply: true, yes: true }); + + expect(mockConfirm).not.toHaveBeenCalled(); + expect(mockSweepHusks).toHaveBeenCalledTimes(1); + }); + + it('--apply alone: prompts, and declining aborts with nothing killed', async () => { + mockFindHuskCandidates.mockResolvedValue({ candidates: [HUSK], graceMs: 3_600_000 }); + mockConfirm.mockResolvedValue(false); + const { towerSweepHusks, logger } = await loadModule(); + + await towerSweepHusks({ apply: true }); + + expect(mockConfirm).toHaveBeenCalledTimes(1); + expect(mockSweepHusks).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('Aborted')); + }); + + it('--apply alone: confirming proceeds to reap', async () => { + mockFindHuskCandidates.mockResolvedValue({ candidates: [HUSK], graceMs: 3_600_000 }); + mockConfirm.mockResolvedValue(true); + mockSweepHusks.mockResolvedValue({ swept: 1, pids: [100] }); + const { towerSweepHusks } = await loadModule(); + + await towerSweepHusks({ apply: true }); + + expect(mockConfirm).toHaveBeenCalledTimes(1); + expect(mockSweepHusks).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/codev/src/agent-farm/cli.ts b/packages/codev/src/agent-farm/cli.ts index 5c6757faa..7a2ea0128 100644 --- a/packages/codev/src/agent-farm/cli.ts +++ b/packages/codev/src/agent-farm/cli.ts @@ -8,6 +8,7 @@ import { Command } from 'commander'; import { start, stop } from './commands/index.js'; import { towerStart, towerStop, towerLog } from './commands/tower.js'; +import { towerSweepHusks } from './commands/tower-sweep-husks.js'; import { towerRegister, towerDeregister, towerCloudStatus } from './commands/tower-cloud.js'; import { logger } from './utils/logger.js'; import { setCliOverrides } from './utils/config.js'; @@ -782,6 +783,21 @@ export async function runAgentFarm(args: string[]): Promise { } }); + // Issue #1227: reap stranded shellper husks (unregistered + childless + aged). + towerCmd + .command('sweep-husks') + .description('Preview or reap stranded shellper husk processes') + .option('--apply', 'Actually reap husk shellpers (default: dry-run preview only)') + .option('-y, --yes', 'Skip --apply confirmation prompt') + .action(async (options: { apply?: boolean; yes?: boolean }) => { + try { + await towerSweepHusks({ apply: options.apply, yes: options.yes }); + } catch (error) { + logger.error(error instanceof Error ? error.message : String(error)); + process.exit(1); + } + }); + // Connect/disconnect handlers (shared with hidden backward-compat aliases) const connectAction = async (options: { reauth?: boolean; service?: string; port?: string }) => { try { diff --git a/packages/codev/src/agent-farm/commands/status.ts b/packages/codev/src/agent-farm/commands/status.ts index ebb003fae..43675d2e5 100644 --- a/packages/codev/src/agent-farm/commands/status.ts +++ b/packages/codev/src/agent-farm/commands/status.ts @@ -123,13 +123,17 @@ function emitStatusJson(params: { architects: Array<{ name: string }>; builders: Builder[]; ownerFilter: string | undefined; + // Issue #1227: null (not omitted) when Tower is down or the running Tower + // predates these fields — same nullable-not-optional contract as `workspace.name`. + fleet: { rssKb: number | null; unregisteredShellperCount: number | null }; }): void { - const { towerRunning, workspace, architects, builders, ownerFilter } = params; + const { towerRunning, workspace, architects, builders, ownerFilter, fleet } = params; const visible = sortByOwner(filterByOwner(builders, ownerFilter)); const payload = { tower: { running: towerRunning }, workspace, + fleet, ownerFilter: ownerFilter ?? null, architects: architects.map((a) => ({ name: a.name ?? 'main' })), builders: visible.map((b) => ({ @@ -174,12 +178,23 @@ export async function status(options: StatusOptions = {}): Promise { if (options.json) { let workspaceName: string | undefined; let workspaceActive = false; + let fleet: { rssKb: number | null; unregisteredShellperCount: number | null } = { + rssKb: null, + unregisteredShellperCount: null, + }; if (towerRunning) { const ws = await client.getWorkspaceStatus(workspacePath); if (ws) { workspaceName = ws.name; workspaceActive = ws.active; } + const health = await client.getHealth(); + if (health) { + fleet = { + rssKb: health.fleetRssKb ?? null, + unregisteredShellperCount: health.unregisteredShellperCount ?? null, + }; + } } emitStatusJson({ towerRunning, @@ -187,6 +202,7 @@ export async function status(options: StatusOptions = {}): Promise { architects, builders, ownerFilter, + fleet, }); return; } @@ -201,6 +217,17 @@ export async function status(options: StatusOptions = {}): Promise { logger.kv(' Uptime', `${Math.floor(health.uptime)}s`); logger.kv(' Active Workspaces', health.activeWorkspaces); logger.kv(' Memory', `${Math.round(health.memoryUsage / 1024 / 1024)}MB`); + // Issue #1227: fleet RSS is the real OS-level memory cost of the + // shellper/claude process fleet — distinct from `Memory` above, which is + // only Tower's own V8 heap. Omitted (not shown as 0) when the running + // Tower predates these fields. + if (health.fleetRssKb !== undefined) { + logger.kv(' Fleet RSS', `${Math.round(health.fleetRssKb / 1024)}MB`); + } + if (health.unregisteredShellperCount !== undefined) { + const count = health.unregisteredShellperCount; + logger.kv(' Unregistered Shellpers', count > 0 ? chalk.yellow(String(count)) : String(count)); + } } showArtifactConfig(workspacePath); diff --git a/packages/codev/src/agent-farm/commands/tower-sweep-husks.ts b/packages/codev/src/agent-farm/commands/tower-sweep-husks.ts new file mode 100644 index 000000000..ea9a34deb --- /dev/null +++ b/packages/codev/src/agent-farm/commands/tower-sweep-husks.ts @@ -0,0 +1,92 @@ +// afx tower sweep-husks — Issue #1227. +// Dry-run by default; --apply actually reaps, mirroring `afx workspace recover`'s +// preview/--apply/-y UX exactly (same rationale: an irreversible, fleet-wide +// operation gets a preview + confirmation gate, not a bare destructive default). + +import { logger } from '../utils/logger.js'; +import { getTowerClient } from '../lib/tower-client.js'; +import { confirm } from '../../lib/cli-prompts.js'; +import type { HuskCandidate } from '../lib/tower-client.js'; + +export interface TowerSweepHusksOptions { + apply?: boolean; + yes?: boolean; +} + +function formatAge(ageMs: number | null): string { + if (ageMs === null) return '—'; + const minutes = Math.floor(ageMs / 60_000); + if (minutes < 60) return `${minutes}m`; + const hours = Math.floor(minutes / 60); + if (hours < 24) return `${hours}h`; + const days = Math.floor(hours / 24); + return `${days}d`; +} + +function formatRss(rssKb: number): string { + if (rssKb >= 1024) return `${(rssKb / 1024).toFixed(1)}MB`; + return `${rssKb}KB`; +} + +function printCandidates(candidates: HuskCandidate[]): void { + const widths = [10, 8, 10]; + logger.row(['PID', 'AGE', 'RSS'], widths); + logger.row(['─'.repeat(10), '─'.repeat(8), '─'.repeat(10)], widths); + for (const candidate of candidates) { + logger.row([String(candidate.pid), formatAge(candidate.ageMs), formatRss(candidate.rssKb)], widths); + } +} + +export async function towerSweepHusks(options: TowerSweepHusksOptions = {}): Promise { + const client = getTowerClient(); + const towerRunning = await client.isRunning(); + if (!towerRunning) { + logger.error('Tower is not running. Start it with `afx tower start`.'); + process.exitCode = 1; + return; + } + + const apply = options.apply ?? false; + logger.header(`Husk Sweep${apply ? '' : ' (preview)'}`); + + const preview = await client.findHuskCandidates(); + if (!preview) { + logger.error('Failed to reach Tower for husk preview.'); + process.exitCode = 1; + return; + } + + logger.kv('Grace period', `${Math.round(preview.graceMs / 60_000)}m`); + logger.blank(); + + if (preview.candidates.length === 0) { + logger.info('No husk shellpers found.'); + return; + } + + printCandidates(preview.candidates); + logger.blank(); + + if (!apply) { + logger.info(`Run with --apply to reap ${preview.candidates.length} husk shellper(s).`); + return; + } + + if (!options.yes) { + const proceed = await confirm(`Proceed to reap ${preview.candidates.length} husk shellper(s)?`, false); + if (!proceed) { + logger.info('Aborted.'); + return; + } + } + + const result = await client.sweepHusks(); + if (!result) { + logger.error('Failed to reach Tower for husk sweep.'); + process.exitCode = 1; + return; + } + + logger.blank(); + logger.kv('Reaped', String(result.swept)); +} diff --git a/packages/codev/src/agent-farm/lib/tower-client.ts b/packages/codev/src/agent-farm/lib/tower-client.ts index bc4efc443..18835f3f0 100644 --- a/packages/codev/src/agent-farm/lib/tower-client.ts +++ b/packages/codev/src/agent-farm/lib/tower-client.ts @@ -15,6 +15,9 @@ export { type TowerTunnelStatus, type TowerStatus, type TowerTerminal, + type HuskCandidate, + type HuskPreview, + type HuskSweepResult, } from '@cluesmith/codev-core/tower-client'; export { encodeWorkspacePath, decodeWorkspacePath } from '@cluesmith/codev-core/workspace'; diff --git a/packages/codev/src/agent-farm/servers/process-census.ts b/packages/codev/src/agent-farm/servers/process-census.ts new file mode 100644 index 000000000..d536ddb72 --- /dev/null +++ b/packages/codev/src/agent-farm/servers/process-census.ts @@ -0,0 +1,70 @@ +/** + * Issue #1227: a shared single-`ps`-call process snapshot, reused by both the + * shellper husk sweep (predicate needs pid/ppid to detect "childless") and the + * fleet-RSS observability feature (needs rss). Centralizing the scan avoids + * growing a fourth bespoke `ps` caller alongside the three that already exist + * (session-manager.ts, architect-session-holder.ts, commands/cleanup.ts). + * + * Async (`execFile`, not `execFileSync`) is load-bearing, not a style choice: + * this is called from `/health` (tower-routes.ts), a hot Tower HTTP path. + * `execFileSync` blocks the entire Node.js event loop for the duration of the + * `ps` call, freezing every open terminal's WebSocket traffic — the exact + * previously-fixed anti-pattern documented at lessons-learned.md:160 + * ("execSync in HTTP request handlers blocks the entire Node.js event loop"). + */ + +import { execFile } from 'node:child_process'; + +export interface ProcessCensusEntry { + pid: number; + ppid: number; + /** Resident set size in kilobytes, as `ps -o rss=` reports it. */ + rssKb: number; + /** The full joined argv (as `ps ... -o args=` reports it). */ + cmdline: string; +} + +function parseCensus(out: string): ProcessCensusEntry[] { + const entries: ProcessCensusEntry[] = []; + for (const line of out.split('\n')) { + const trimmed = line.trimStart(); + if (!trimmed) continue; + const fields = trimmed.match(/^(\d+)\s+(\d+)\s+(\d+)\s+(.*)$/s); + if (!fields) continue; + const pid = parseInt(fields[1], 10); + const ppid = parseInt(fields[2], 10); + const rssKb = parseInt(fields[3], 10); + if (Number.isNaN(pid) || pid <= 0 || Number.isNaN(ppid) || Number.isNaN(rssKb)) continue; + entries.push({ pid, ppid, rssKb, cmdline: fields[4] }); + } + return entries; +} + +/** + * Snapshot every running process as {pid, ppid, rssKb, cmdline}. `ps -ww` + * prevents argv truncation (shellper config blobs are large) on both BSD and + * coreutils `ps`. Resolves to `[]` on `ps` failure (missing binary, timeout, + * non-zero exit) — callers decide how to degrade, mirroring the async + * `findShellperProcesses` convention in `../../terminal/session-manager.ts`. + * + * Deliberately raw `execFile` + a hand-rolled Promise, not `execFileSync` and + * not `util.promisify(execFile)`: this is called from `/health` + * (tower-routes.ts), a hot Tower HTTP path, so it must never block the event + * loop — `execFileSync` would freeze every open terminal's WebSocket traffic + * for the duration of the `ps` call, the exact previously-fixed anti-pattern + * documented at lessons-learned.md:160. `util.promisify` is avoided because + * `child_process.execFile` carries a built-in custom promisify resolving + * `{stdout, stderr}` via a non-standard symbol that a test mock would need to + * reproduce exactly to avoid silently promisifying to the wrong shape. + */ +export function listProcessCensus(): Promise { + return new Promise((resolve) => { + execFile('ps', ['-A', '-ww', '-eo', 'pid=,ppid=,rss=,args='], { timeout: 5000, maxBuffer: 16 * 1024 * 1024 }, (err, stdout) => { + if (err || !stdout) { + resolve([]); + return; + } + resolve(parseCensus(stdout)); + }); + }); +} diff --git a/packages/codev/src/agent-farm/servers/shellper-husk-sweep.ts b/packages/codev/src/agent-farm/servers/shellper-husk-sweep.ts new file mode 100644 index 000000000..f99e2352e --- /dev/null +++ b/packages/codev/src/agent-farm/servers/shellper-husk-sweep.ts @@ -0,0 +1,178 @@ +/** + * Issue #1227: a stricter, second-pass reap for shellper "husks" — a shellper + * whose PTY child has exited (deliberate lingering behavior from #905/#1198 so + * a reconnect can replay history) but which `killOrphanedShellpers` + * (`../../terminal/session-manager.ts`) can never reap, because that function + * unconditionally protects any shellper whose Unix socket still responds. + * + * A husk still answers its socket, so it survives `killOrphanedShellpers` + * forever. The predicate here is stricter and orthogonal: a shellper is a husk + * only when it is simultaneously + * - unregistered — not a live PID in `terminal_sessions.shellper_pid` + * - childless — no process in the table has it as a parent (OS-level + * signal; there is no wire-protocol "has-child" frame) + * - aged — older than `graceMs`, which absorbs any legitimate + * mid-respawn / mid-adoption transient (seconds), so the + * three conditions together can only ever be true for a + * genuine husk (per the issue: "unregistered and + * childless" is a terminal state — a shellper that lost + * its registration can never receive another SPAWN). + * + * The actual kill is delegated to `reapShellpers` (`./architect-session-holder.js`, + * SIGTERM → poll → SIGKILL → confirm-death) rather than reimplemented here. + */ + +import type Database from 'better-sqlite3'; +import { listProcessCensus, type ProcessCensusEntry } from './process-census.js'; +import { getProcessStartTime } from '../../terminal/session-manager.js'; +import { reapShellpers } from './architect-session-holder.js'; + +export const SHELLPER_MARKER = 'shellper-main.js'; + +/** + * Same tolerance `SessionManager.reconnectSession` uses + * (`../../terminal/session-manager.ts:471`) to detect PID reuse: a live + * process's actual start time must match the DB-recorded one within this + * window, or the DB row is stale (its PID was reused by an unrelated + * process) and must not count as "registered". + */ +const PID_REUSE_TOLERANCE_MS = 2000; + +interface ShellperRegistryRow { + shellper_pid: number; + shellper_start_time: number | null; +} + +/** + * The set of shellper PIDs Tower currently believes are registered, per + * `terminal_sessions` — validated against each PID's actual process start + * time to guard against PID reuse (a stale row whose PID was reused by an + * unrelated process does not count as registered). + */ +export async function computeRegisteredShellperPids( + db: Database.Database, + getStartTime: (pid: number) => Promise = getProcessStartTime, +): Promise> { + const rows = db + .prepare('SELECT shellper_pid, shellper_start_time FROM terminal_sessions WHERE shellper_pid IS NOT NULL') + .all() as ShellperRegistryRow[]; + + const registered = new Set(); + for (const row of rows) { + if (row.shellper_start_time === null) { + // Legacy row with no recorded start time — no positive signal to + // invalidate it, so trust the registration. + registered.add(row.shellper_pid); + continue; + } + const actual = await getStartTime(row.shellper_pid); + if (actual !== null && Math.abs(actual - row.shellper_start_time) <= PID_REUSE_TOLERANCE_MS) { + registered.add(row.shellper_pid); + } + } + return registered; +} + +export interface FindHuskShellpersOptions { + /** This Tower instance's socket directory — scopes the sweep so another + * instance's (or another user's) legitimate shellpers are never touched. */ + socketDir: string; + registeredShellperPids: Set; + graceMs: number; + /** + * Seam, defaults to `listProcessCensus`. Accepts a plain array too (not just + * a function returning one) so a caller that already took a snapshot for + * another purpose (e.g. RSS display) can pass it through and avoid a second + * `ps` scan — see `handleHuskPreview` in tower-routes.ts. + */ + census?: () => ProcessCensusEntry[] | Promise; + /** Seam, defaults to `Date.now()`. */ + now?: number; + /** Seam, defaults to `getProcessStartTime`. */ + getStartTime?: (pid: number) => Promise; +} + +/** + * Returns the PIDs of shellper-main.js processes, scoped to `socketDir`, that + * are unregistered AND childless AND older than `graceMs`. + */ +export async function findHuskShellpers(opts: FindHuskShellpersOptions): Promise { + const census = await (opts.census ?? listProcessCensus)(); + const now = opts.now ?? Date.now(); + const getStartTime = opts.getStartTime ?? getProcessStartTime; + + // Match the trailing-'/' scope-marker technique already proven in + // findShellperProcesses (../../terminal/session-manager.ts:782-793) to + // prevent prefix collisions (e.g. /run matching /run2). + const scopeMarker = opts.socketDir.endsWith('/') ? opts.socketDir : `${opts.socketDir}/`; + const parentPids = new Set(census.map((entry) => entry.ppid)); + + const candidates: number[] = []; + for (const entry of census) { + if (!entry.cmdline.includes(SHELLPER_MARKER)) continue; + if (!entry.cmdline.includes(scopeMarker)) continue; + if (opts.registeredShellperPids.has(entry.pid)) continue; + if (parentPids.has(entry.pid)) continue; // has a live child — not a husk + + const startTime = await getStartTime(entry.pid); + if (startTime === null) continue; // can't confirm age — safe default is to skip + if (now - startTime < opts.graceMs) continue; // too young — race-safety window + + candidates.push(entry.pid); + } + return candidates; +} + +/** + * The husk grace period (env `SHELLPER_HUSK_GRACE_MS`, default 1 hour), shared + * by every trigger (startup, periodic, on-demand preview/apply) so a preview + * always predicts what the periodic sweep would actually do. + * + * NaN-checked rather than `parsed || default`: `0` is a legitimate override + * (tests want an immediate-eligibility grace period) and `0` is falsy in JS, + * so `parsed || default` would silently discard a deliberate zero. + */ +export function resolveHuskGraceMs(env: NodeJS.ProcessEnv = process.env): number { + const parsed = parseInt(env.SHELLPER_HUSK_GRACE_MS || '3600000', 10); + return Math.max(Number.isNaN(parsed) ? 3600000 : parsed, 0); +} + +export interface SweepShellperHusksOptions { + socketDir: string; + db: Database.Database; + graceMs: number; + log?: (msg: string) => void; + census?: () => ProcessCensusEntry[] | Promise; + now?: number; + getStartTime?: (pid: number) => Promise; + /** Seam, defaults to `reapShellpers`. */ + reap?: (pids: number[]) => Promise; +} + +/** + * Compute the registered-shellper set from the DB, find husk candidates, and + * reap them. Returns the swept PIDs so callers (startup log, periodic-timer + * log, on-demand CLI) can report what happened. + */ +export async function sweepShellperHusks( + opts: SweepShellperHusksOptions, +): Promise<{ swept: number; pids: number[] }> { + const getStartTime = opts.getStartTime ?? getProcessStartTime; + const registeredShellperPids = await computeRegisteredShellperPids(opts.db, getStartTime); + const pids = await findHuskShellpers({ + socketDir: opts.socketDir, + registeredShellperPids, + graceMs: opts.graceMs, + census: opts.census, + now: opts.now, + getStartTime, + }); + + if (pids.length > 0) { + opts.log?.(`Sweeping ${pids.length} husk shellper process(es): ${pids.join(', ')}`); + const reap = opts.reap ?? reapShellpers; + await reap(pids); + } + + return { swept: pids.length, pids }; +} diff --git a/packages/codev/src/agent-farm/servers/tower-routes.ts b/packages/codev/src/agent-farm/servers/tower-routes.ts index 4286c9f1b..d597d5166 100644 --- a/packages/codev/src/agent-farm/servers/tower-routes.ts +++ b/packages/codev/src/agent-farm/servers/tower-routes.ts @@ -75,6 +75,15 @@ import type { IssueSearchItem, IssueSearchResponse } from '@cluesmith/codev-type import { computeAnalytics } from './analytics.js'; import { getAllTasks, executeTask, getTaskId } from './tower-cron.js'; import { getGlobalDb } from '../db/index.js'; +import { listProcessCensus } from './process-census.js'; +import { + SHELLPER_MARKER, + computeRegisteredShellperPids, + findHuskShellpers, + sweepShellperHusks, + resolveHuskGraceMs, +} from './shellper-husk-sweep.js'; +import { getProcessStartTime } from '../../terminal/session-manager.js'; import type { CronTask } from './tower-cron.js'; import { getWorkspaceTerminals, @@ -159,7 +168,9 @@ type RouteEntry = ( ) => Promise | void; const ROUTES: Record = { - 'GET /health': (_req, res) => handleHealthCheck(res), + 'GET /health': (_req, res, _url, ctx) => handleHealthCheck(res, ctx), + 'GET /api/shellpers/husks': (_req, res, _url, ctx) => handleHuskPreview(res, ctx), + 'POST /api/shellpers/husks/sweep': (_req, res, _url, ctx) => handleHuskSweep(res, ctx), 'GET /api/workspaces': (_req, res) => handleListWorkspaces(res), 'POST /api/terminals': (req, res, _url, ctx) => handleTerminalCreate(req, res, ctx), 'GET /api/terminals': (_req, res) => handleTerminalList(res), @@ -294,9 +305,59 @@ export async function handleRequest( // Global route handlers // ============================================================================ -async function handleHealthCheck(res: http.ServerResponse): Promise { +/** + * Issue #1227: fleet RSS + unregistered-shellper count, scoped to this Tower + * instance's socketDir. `fleetRssKb` sums every in-scope shellper AND its + * direct children (the full process-group tree Tower manages) regardless of + * DB-registration state, so a husk not yet swept still counts toward the + * visible cost. `unregisteredShellperCount` is the lighter, ungated signal + * ("N shellpers Tower doesn't currently track") — distinct from the stricter + * childless+aged reap predicate in shellper-husk-sweep.ts. + * + * Best-effort: returns undefined fields (never throws) if `ps` or the DB read + * fails, so a fleet-accounting hiccup never takes down /health itself. + */ +async function computeFleetHealthFields( + ctx: RouteContext, +): Promise<{ fleetRssKb?: number; unregisteredShellperCount?: number }> { + const shellperManager = ctx.getShellperManager(); + if (!shellperManager) return {}; + try { + const census = await listProcessCensus(); + const scopeMarker = shellperManager.socketDir.endsWith('/') + ? shellperManager.socketDir + : `${shellperManager.socketDir}/`; + + const inScopePids = new Set(); + let fleetRssKb = 0; + for (const entry of census) { + if (entry.cmdline.includes(SHELLPER_MARKER) && entry.cmdline.includes(scopeMarker)) { + inScopePids.add(entry.pid); + fleetRssKb += entry.rssKb; + } + } + for (const entry of census) { + if (inScopePids.has(entry.ppid)) { + fleetRssKb += entry.rssKb; + } + } + + const registered = await computeRegisteredShellperPids(getGlobalDb()); + let unregisteredShellperCount = 0; + for (const pid of inScopePids) { + if (!registered.has(pid)) unregisteredShellperCount++; + } + + return { fleetRssKb, unregisteredShellperCount }; + } catch { + return {}; + } +} + +async function handleHealthCheck(res: http.ServerResponse, ctx: RouteContext): Promise { const instances = await getInstances(); const activeCount = instances.filter((i) => i.running).length; + const fleetFields = await computeFleetHealthFields(ctx); res.writeHead(200, { 'Content-Type': 'application/json' }); res.end( JSON.stringify({ @@ -309,11 +370,89 @@ async function handleHealthCheck(res: http.ServerResponse): Promise { activeWorkspaces: activeCount, totalWorkspaces: instances.length, memoryUsage: process.memoryUsage().heapUsed, + ...fleetFields, timestamp: new Date().toISOString(), }) ); } +/** + * Issue #1227: GET /api/shellpers/husks — preview, read-only. Returns the + * husk candidates the next sweep (periodic or `--apply`) would reap, without + * touching anything. + */ +async function handleHuskPreview(res: http.ServerResponse, ctx: RouteContext): Promise { + const shellperManager = ctx.getShellperManager(); + if (!shellperManager) { + res.writeHead(503, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'Shellper manager not initialized' })); + return; + } + try { + const graceMs = resolveHuskGraceMs(); + // One census snapshot, shared between the candidate decision and the + // displayed RSS — passing it via the `census` seam avoids a second `ps` + // scan and guarantees the RSS shown is for the exact snapshot that decided + // candidacy, not a later, possibly-different one (codex #1227 PR review). + const census = await listProcessCensus(); + const rssByPid = new Map(census.map((entry) => [entry.pid, entry.rssKb])); + + const registered = await computeRegisteredShellperPids(getGlobalDb()); + const pids = await findHuskShellpers({ + socketDir: shellperManager.socketDir, + registeredShellperPids: registered, + graceMs, + census: () => census, + }); + + const now = Date.now(); + const candidates = await Promise.all( + pids.map(async (pid) => { + const startTime = await getProcessStartTime(pid); + return { + pid, + rssKb: rssByPid.get(pid) ?? 0, + ageMs: startTime !== null ? now - startTime : null, + }; + }), + ); + + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ candidates, graceMs })); + } catch (err) { + res.writeHead(500, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: (err as Error).message })); + } +} + +/** + * Issue #1227: POST /api/shellpers/husks/sweep — apply. Actually reaps the + * current husk candidates. Destructive; the `afx tower sweep-husks` CLI is + * the only caller expected in practice, and it gates this behind `--apply` + * plus a confirmation prompt (unless `--yes`). + */ +async function handleHuskSweep(res: http.ServerResponse, ctx: RouteContext): Promise { + const shellperManager = ctx.getShellperManager(); + if (!shellperManager) { + res.writeHead(503, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: 'Shellper manager not initialized' })); + return; + } + try { + const result = await sweepShellperHusks({ + socketDir: shellperManager.socketDir, + db: getGlobalDb(), + graceMs: resolveHuskGraceMs(), + log: (msg: string) => ctx.log('INFO', msg), + }); + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify(result)); + } catch (err) { + res.writeHead(500, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: (err as Error).message })); + } +} + /** * GET /api/version (#983) — report the version of the *running* Tower process. * diff --git a/packages/codev/src/agent-farm/servers/tower-server.ts b/packages/codev/src/agent-farm/servers/tower-server.ts index 55e902e69..d53515ab3 100644 --- a/packages/codev/src/agent-farm/servers/tower-server.ts +++ b/packages/codev/src/agent-farm/servers/tower-server.ts @@ -18,6 +18,7 @@ import { WebSocketServer } from 'ws'; import { SessionManager } from '../../terminal/session-manager.js'; import type { SSEClient } from './tower-types.js'; import { startRateLimitCleanup } from './tower-utils.js'; +import { sweepShellperHusks, resolveHuskGraceMs } from './shellper-husk-sweep.js'; import { initTunnel, shutdownTunnel, @@ -65,6 +66,7 @@ const rateLimitCleanupInterval = startRateLimitCleanup(); // Shellper session manager (initialized at startup) let shellperManager: SessionManager | null = null; let shellperCleanupInterval: NodeJS.Timeout | null = null; +let shellperHuskSweepInterval: NodeJS.Timeout | null = null; let terminalPartialMonitorInterval: NodeJS.Timeout | null = null; // Observability for Issue #1047: the ring-buffer partial is kept whole (no @@ -165,6 +167,7 @@ async function gracefulShutdown(signal: string): Promise { // 4. Stop rate limit cleanup, shellper periodic cleanup, and SSE heartbeat clearInterval(rateLimitCleanupInterval); if (shellperCleanupInterval) clearInterval(shellperCleanupInterval); + if (shellperHuskSweepInterval) clearInterval(shellperHuskSweepInterval); if (terminalPartialMonitorInterval) clearInterval(terminalPartialMonitorInterval); clearInterval(sseHeartbeatInterval); @@ -421,6 +424,34 @@ server.listen(port, bindHost, async () => { } }, cleanupIntervalMs); + // Issue #1227: husk sweep — a stricter second pass that reaps shellpers + // killOrphanedShellpers() can never reach, because it unconditionally + // protects any shellper whose socket still responds. A husk (child exited, + // shellper still listening) answers its socket, so it needs the stricter + // unregistered+childless+aged predicate instead. See shellper-husk-sweep.ts. + const huskGraceMs = resolveHuskGraceMs(); + const parsedHuskSweepIntervalMs = parseInt(process.env.SHELLPER_HUSK_SWEEP_INTERVAL_MS || '3600000', 10); + const huskSweepIntervalMs = Math.max( + Number.isNaN(parsedHuskSweepIntervalMs) ? 3600000 : parsedHuskSweepIntervalMs, + 1000, + ); + const runHuskSweep = async (): Promise => { + try { + const result = await sweepShellperHusks({ + socketDir, + db: getGlobalDb(), + graceMs: huskGraceMs, + log: (msg: string) => log('INFO', msg), + }); + if (result.swept > 0) { + log('INFO', `Husk sweep: reaped ${result.swept} shellper husk(s)`); + } + } catch (err) { + log('ERROR', `Husk sweep failed: ${(err as Error).message}`); + } + }; + shellperHuskSweepInterval = setInterval(runHuskSweep, huskSweepIntervalMs); + log('INFO', 'Shellper session manager initialized'); // Spec 0105 Phase 4: Initialize terminal management module @@ -489,6 +520,11 @@ server.listen(port, bindHost, async () => { log('INFO', `Killed ${orphansKilled} orphaned shellper process(es)`); } + // Issue #1227: run the stricter husk sweep once at startup too, same + // ordering requirement as killOrphanedShellpers (must run after + // reconciliation so a reconnected session's shellper is registered). + await runHuskSweep(); + // Spec 0105 Phase 3: Initialize instance lifecycle module. // Placed after reconciliation so getInstances() returns [] during startup // (since _deps is null), preventing race conditions with reconciliation. diff --git a/packages/codev/src/terminal/session-manager.ts b/packages/codev/src/terminal/session-manager.ts index dcb7fccd8..425cebaad 100644 --- a/packages/codev/src/terminal/session-manager.ts +++ b/packages/codev/src/terminal/session-manager.ts @@ -189,6 +189,16 @@ export class SessionManager extends EventEmitter { this.log = config.logger ?? (() => {}); } + /** + * This instance's socket directory. Issue #1227: the husk sweep and fleet + * observability route handlers need it to reuse the exact scope-marker + * technique findShellperProcesses() uses, so they never touch (or count) + * another Tower instance's shellpers. + */ + get socketDir(): string { + return this.config.socketDir; + } + /** * Spawn a new shellper process and connect to it. * Returns the connected client. @@ -1095,8 +1105,15 @@ export class SessionManager extends EventEmitter { // Issue #1224 (symptom B): reap the crash-looped shellper husk so // give-up does not leave a live-process-without-a-registry-row zombie // (the divergence remove-architect then failed to clear). Kill the whole - // process group — the shellper is a detached group leader; the periodic - // orphan sweep SIGKILLs any survivor. + // process group — the shellper is a detached group leader. This SIGTERM + // is the only signal sent here (no SIGKILL escalation): the + // terminal_sessions row is NOT cleared at give-up time, so a rare + // SIGTERM-ignoring survivor stays DB-registered and is NOT eligible for + // Issue #1227's husk sweep (which only reaps unregistered shellpers). It + // is caught on the next Tower restart's reconciliation + + // killOrphanedShellpers pass, or immediately for architects via `afx + // workspace remove-architect`'s identity-matched zombie reap + // (tower-instances.ts). try { process.kill(-session.pid, 'SIGTERM'); } catch { diff --git a/packages/core/src/tower-client.ts b/packages/core/src/tower-client.ts index c37e96f47..df0a3a993 100644 --- a/packages/core/src/tower-client.ts +++ b/packages/core/src/tower-client.ts @@ -84,9 +84,46 @@ export interface TowerHealth { activeWorkspaces: number; totalWorkspaces: number; memoryUsage: number; + /** + * Issue #1227: total RSS (KB) of every shellper process in this Tower + * instance's scope plus their direct children — the real OS-level memory + * cost of the process fleet, as opposed to `memoryUsage` (Tower's own V8 + * heap). Includes not-yet-swept husks, since the point is surfacing the + * true cost regardless of registration state. Omitted (not `undefined`) + * when the underlying `ps` scan or DB read fails — a fleet-accounting + * hiccup never fails `/health` itself. Optional for back-compat with older + * Tower builds that predate the field. + */ + fleetRssKb?: number; + /** + * Issue #1227: count of in-scope shellper processes not currently tracked + * in `terminal_sessions` — a lighter, ungated signal than the husk-sweep + * predicate (no childless/aged requirement), purely informational. Same + * omit-on-failure and back-compat notes as `fleetRssKb`. + */ + unregisteredShellperCount?: number; timestamp: string; } +/** Issue #1227: a shellper the husk sweep would reap (or has reaped). */ +export interface HuskCandidate { + pid: number; + rssKb: number; + /** Milliseconds since the shellper process started, or null if undeterminable. */ + ageMs: number | null; +} + +export interface HuskPreview { + candidates: HuskCandidate[]; + /** The grace period (ms) that gated this preview — same value the sweep itself uses. */ + graceMs: number; +} + +export interface HuskSweepResult { + swept: number; + pids: number[]; +} + export interface TowerTunnelStatus { registered: boolean; state: string; @@ -213,6 +250,24 @@ export class TowerClient { return this.request('/api/version'); } + /** + * Issue #1227: preview which shellpers the husk sweep would reap, without + * killing anything. Backs `afx tower sweep-husks`'s default (no-flags) mode. + */ + async findHuskCandidates(): Promise { + const result = await this.request('/api/shellpers/husks'); + return result.ok ? result.data! : null; + } + + /** + * Issue #1227: actually reap the current husk candidates. Backs `afx tower + * sweep-husks --apply`. + */ + async sweepHusks(): Promise { + const result = await this.request('/api/shellpers/husks/sweep', { method: 'POST' }); + return result.ok ? result.data! : null; + } + async listWorkspaces(): Promise { const result = await this.request<{ workspaces: TowerWorkspace[] }>('/api/workspaces'); return result.ok ? result.data!.workspaces : [];