diff --git a/codev/projects/bugfix-1224-tower-shellper-architect-sessi/status.yaml b/codev/projects/bugfix-1224-tower-shellper-architect-sessi/status.yaml new file mode 100644 index 000000000..74298a8a7 --- /dev/null +++ b/codev/projects/bugfix-1224-tower-shellper-architect-sessi/status.yaml @@ -0,0 +1,17 @@ +id: bugfix-1224 +title: tower-shellper-architect-sessi +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-07-22T21:29:44.736Z' + approved_at: '2026-07-22T22:25:19.645Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-07-22T21:01:48.295Z' +updated_at: '2026-07-22T22:25:19.646Z' +pr_ready_for_human: false diff --git a/codev/state/bugfix-1224_thread.md b/codev/state/bugfix-1224_thread.md new file mode 100644 index 000000000..e99d52516 --- /dev/null +++ b/codev/state/bugfix-1224_thread.md @@ -0,0 +1,146 @@ +# bugfix-1224 — Tower/shellper architect session-ID collisions + non-recovering crash loops + +Protocol: BUGFIX (strict). Issue #1224. + +## Investigate + +### Symptom A root cause (the headline: session-ID collision crash loop) + +`resolveArchitectLaunch` (`packages/codev/src/agent-farm/servers/tower-utils.ts`) decides +whether to resume a persisted architect conversation. Since #1145 it resumes a stored +session id whenever the jsonl **exists on disk** (`sessionIsOwned` → `verifySessionOwnership`). + +It never checks whether a **live process is already holding** that session id. When one is +(the report's two cases: a stale pre-restart shellper's claude child, or an unrelated +foreground claude), it bakes `claude --resume ` anyway → claude dies instantly with +`Error: Session ID is already in use` → shellper auto-restarts → dies again → crash loop. + +The existence check answers "does the transcript exist?" but not "is someone using it right now?" +— and a *held* session's jsonl exists precisely because the holder is writing to it, so the +#1145 guard is guaranteed to pass in exactly the collision case. + +### Fix (reporter's suggestion (a)): verify no live holder before resuming + +Before resuming a stored id, positively confirm **no live process holds it** (scan the process +table for the id in argv — the same observable technique as the #1007 orphan-shellper cleanup). +If a live holder is found, mint a fresh session instead of colliding. This is universally safe +for both holder cases: we never touch the holder (critical — case 2's holder is the user's own +foreground claude, must not be killed), and the new architect gets a working fresh conversation. +The crash loop never starts. + +### Scope decision + +- Fix (a) is the isolated root-cause fix for Symptom A. Implementing it. +- Fix (b) "crash-loop breaker in shellper" already largely exists (#1149 + `maybeApplyCrashLoopFallback`, 3 failing exits in 30s → swap to fresh). Noting in PR. +- Fix (c) "atomic deregistration/process death" (Symptom B registry divergence) is a separate, + more architectural concern — will recommend a follow-up issue rather than expand scope here. + +## Fix + +`packages/codev/src/agent-farm/servers/tower-utils.ts`: +- New `sessionHasLiveHolder(sessionId, {list?})` — scans `ps -A -o args=` for a live process + launched with `--session-id ` / `--resume ` (both space- and `=`-joined). Match is + **flag-anchored**, not a bare substring — a bare substring false-positived on the short synthetic + ids in the existing tests (e.g. `'x'`) and is generally unsafe. On scan failure returns `false` + (purely additive guard: diverts to fresh only on positive evidence, never worse than today). +- `resolveArchitectLaunch` resume gate now requires `sessionIsOwned(...) && !hasLiveHolder(...)`. + Held id → mint fresh (with a WARN log) instead of baking a colliding `--resume`. Added + `hasLiveHolder` + `log` seams; ownership is still checked first so the (cheap-ish) `ps` scan is + skipped for stale ids. +- `resolveArchitectRestart` threads `log` through (restart-bake path inherits the guard for free). + +`tower-instances.ts`: pass `log: _deps.log` at the `addArchitect` and `launchInstance` main-path +call sites so the divert-to-fresh decision is diagnosable in Tower logs. + +Tests (`tower-utils.test.ts`): +7 cases (held→mint-fresh with WARN, no-holder→resume, ownership +short-circuits the scan, and 4 `sessionHasLiveHolder` unit cases incl. scan-failure→false). + +### Validation +- `porch check`: build ✓, tests ✓ (full suite, 26.8s). tsc --noEmit clean. +- Note: the worktree spawned WITHOUT `node_modules` (postSpawn `pnpm install` had not run); ran + `pnpm install` + built `@cluesmith/codev-core` to get a working test env. A raw `vitest run` + before the full build shows 8 pre-existing env failures (missing `dist/` + copied skeleton + artifacts — adopt/update/consult/tier-materialization/consolidate/session-manager integration); + all clear once porch's build check emits those artifacts. None touch changed code. + +## PR + +PR #1225 opened (`Fixes #1224`), mergeable. CMAP: +- Codex: APPROVE (HIGH, no issues) +- Claude: APPROVE (HIGH, no issues) — confirmed TOCTOU windows are safe (mint-fresh or #1149 backstop) +- Gemini: skipped non-blocking (couldn't emit a `--type pr` VERDICT in this worktree; known lane limitation) + +No REQUEST_CHANGES. Requested the `pr` gate via `porch done`; awaiting human approval before merge. + +Note for follow-up: consult's project auto-detect fails from a builder worktree that carries the +full `codev/projects/` tree ("Multiple projects found"); had to pin `--issue 1224 --project-id +bugfix-1224`. Worth a separate issue if it recurs. + +## PR iteration 2 — architect requested 3 changes (approved scope expansion) + +Gate NOT approved. Waleed wants (all in this PR): +1. **JSON-argv parent needles** — a remnant *shellper* carries the id as `"--session-id",""` + in its config JSON; the crash-looping child is dead <8s/respawn so the space/`=` needles miss + the incident-1 holder during most of its life. Add JSON forms. +2. **Mint-or-RECLAIM** — when the holder is verifiably OUR OWN superseded shellper (shellper-main.js + + same session id + same cwd + same CODEV_ARCHITECT_NAME, not self), kill its process group + (SIGTERM→SIGKILL) and RESUME. Foreign holders never touched. Test never-kill-foreign explicitly. +3. **Symptom B** — (a) add/launch reconcile with an existing live shellper for the identity + (reap, don't spawn a duplicate); (b) crash-loop give-up deregisters cleanly + reaps husk; + (c) remove-architect clears live-process-no-row zombies; (d) capture dying child stderr/exit. + +### Design +- New module `servers/architect-session-holder.ts`: `sessionIdNeedles` (space/`=`/JSON forms), + `cmdlineHoldsSession`, `listProcessEntries` (ps -ww -eo pid=,args=), `classifyArchitectSessionHolder` + → `{reclaimable: pid[], foreign: bool}` (reclaimable = shellper-main.js whose JSON has matching + sessionId+cwd+CODEV_ARCHITECT_NAME), `findOwnArchitectShellpers` (identity w/o session, for + remove-architect), and async `reclaimSupersededShellpers` (kill group + poll-for-death, injectable + seams). Decision: foreign → mint fresh; else reclaimable → kill+resume; else resume. +- `sessionHasLiveHolder` (tower-utils) delegates to the shared needle helper (gets JSON forms). +- Wire async reconcile into `addArchitect` + `launchInstance` main before `resolveArchitectLaunch`, + passing `hasLiveHolder: () => foreignHolder`. +- `removeArchitect` not-found branch: reap a matching live zombie shellper → success. +- SessionManager maxRestarts give-up: logStderrTail (capture child reason) + kill shellper group + (reap husk) so give-up leaves no row-gone/process-alive zombie. + +Root-cause mapping to forensic timeline (issue comments): incident 1 = self stale remnant → +reclaim; incident 3 foreign = user's tty claude → mint fresh (never touch); incident 3 +wedge-after-free + silent-dereg → give-up reap + stderr capture + remove-architect zombie reap. + +### Implemented (iteration 2) +- `servers/architect-session-holder.ts` (new): needles (+JSON parent form), `isOwnArchitectShellper` + (shellper-main.js + cwd + CODEV_ARCHITECT_NAME identity gate), `classifyArchitectSessionHolder` + (own→reclaimable / foreign), `findOwnArchitectShellpers`, `reapShellpers` (group SIGTERM→SIGKILL, + poll-for-death), `reconcileArchitectSessionHolder` (foreign→mint-fresh / own→reap+resume). +- `tower-utils.sessionHasLiveHolder` delegates to shared needles (gains JSON form + `ps -ww`). +- `addArchitect` + `launchInstance` main: async reconcile before `resolveArchitectLaunch` + (`hasLiveHolder: () => foreignHolder`). +- `removeArchitect`: reaps live-process-no-row zombies (identity match). +- `session-manager` give-up: stderr capture + process-group SIGTERM husk reap. + +Validation: full build ✓; full suite **3582 passed / 48 skipped, 0 failed** (30s); tsc clean. +Tests: architect-session-holder.test.ts (needles/identity/classify/reap/reconcile incl. explicit +never-kill-foreign) + session-manager give-up husk-reap. Net add this iteration ≫300 LOC — expected +for the architect-approved scope expansion. + +Honesty note for PR: the wedge-after-free deeper cause (children dying <8s with the session +demonstrably free) — claude's own error surfaces via the PTY data/ring buffer, not shellper stderr, +so give-up now logs exit code/signal + shellper stderr for diagnosis and reaps the husk so the loop +can't persist; the definitive root cause of that specific datapoint is captured-for-diagnosis, not +claimed-fixed (per architect's "document rather than chase blind"). + +### CMAP iteration 2 +- claude = APPROVE (HIGH, no issues). +- codex = REQUEST_CHANGES: (1) `reapShellpers` didn't confirm death AFTER SIGKILL → resume could + race the still-exiting holder → **fixed** (post-SIGKILL `killGraceMs` poll + test). (2) status.yaml + + thread.md in the PR → **rebutted with evidence**: status.yaml is committed by porch's own + `chore(porch)` commits (builders must not touch it; prior merged bugfixes e.g. #1220 carry the + identical commits), and the thread is committed by builder-role design (ships to main). Deferring + the final call to the architect at the gate. +- Also self-caught + fixed a regression: `resolveArchitectRestart` was self-detecting the very + shellper being reconnected (its child's argv holds `--resume `) → would bake fresh restart + args on every healthy reconnect and drop conversation on next crash. Now `hasLiveHolder:()=>false` + on the restart-bake path (collision-avoidance is at add/launch reconcile + #1149 runtime fallback). + +Validation after fixes: full build ✓; full suite **3583 passed / 48 skipped / 0 failed**; tsc clean. diff --git a/packages/codev/src/agent-farm/__tests__/architect-session-holder.test.ts b/packages/codev/src/agent-farm/__tests__/architect-session-holder.test.ts new file mode 100644 index 000000000..e4236ad41 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/architect-session-holder.test.ts @@ -0,0 +1,240 @@ +/** + * Issue #1224: tests for architect session-holder detection & mint-or-reclaim. + * + * The critical safety property under test is NEVER-KILL-FOREIGN: only our own + * identity-matched superseded shellper is ever reaped; a foreign holder (e.g. a + * claude the user started by hand) is classified foreign and left untouched. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { + sessionIdNeedles, + cmdlineHoldsSession, + isOwnArchitectShellper, + classifyArchitectSessionHolder, + findOwnArchitectShellpers, + reapShellpers, + reconcileArchitectSessionHolder, + type ProcessEntry, +} from '../servers/architect-session-holder.js'; + +const SID = '166e1c25-fd66-46ed-a81c-5dfcf2295efd'; +const WS = '/Users/x/workspace'; + +/** Build a realistic shellper-main.js process argv line (as `ps` reports it). */ +function shellperCmdline(opts: { + sessionId?: string; + cwd?: string; + architectName?: string; + flag?: string; +}): string { + const config = { + command: 'claude', + args: [opts.flag ?? '--session-id', opts.sessionId ?? SID], + cwd: opts.cwd ?? WS, + env: { CODEV_ARCHITECT_NAME: opts.architectName ?? 'main', PATH: '/usr/bin' }, + socketPath: '/tmp/sock/s.sock', + }; + return `/usr/bin/node /opt/codev/dist/terminal/shellper-main.js ${JSON.stringify(config)}`; +} + +/** A bare claude child's shell argv (the foreground-holder case). */ +function claudeChildCmdline(sessionId = SID, flag = '--resume'): string { + return `/usr/bin/claude ${flag} ${sessionId} --append-system-prompt xyz`; +} + +describe('sessionIdNeedles / cmdlineHoldsSession (Issue #1224)', () => { + it('matches the claude-child shell forms', () => { + expect(cmdlineHoldsSession(`claude --session-id ${SID}`, SID)).toBe(true); + expect(cmdlineHoldsSession(`claude --resume ${SID}`, SID)).toBe(true); + expect(cmdlineHoldsSession(`claude --session-id=${SID}`, SID)).toBe(true); + expect(cmdlineHoldsSession(`claude --resume=${SID}`, SID)).toBe(true); + }); + + it('matches the shellper-parent JSON argv form (the crash-loop remnant)', () => { + // This is the form a remnant shellper carries while its child is dead. + expect(cmdlineHoldsSession(shellperCmdline({}), SID)).toBe(true); + expect(cmdlineHoldsSession(`x "--resume","${SID}" y`, SID)).toBe(true); + }); + + it('does not match an unrelated or bare-substring occurrence', () => { + expect(cmdlineHoldsSession(`some/path/${SID}/file`, SID)).toBe(false); + expect(cmdlineHoldsSession(`claude --resume other-id`, SID)).toBe(false); + expect(sessionIdNeedles(SID)).toContain(`"--session-id","${SID}"`); + }); +}); + +describe('isOwnArchitectShellper (Issue #1224)', () => { + it('is true for a shellper matching cwd + CODEV_ARCHITECT_NAME', () => { + const line = shellperCmdline({ architectName: 'reviewer', cwd: WS }); + expect(isOwnArchitectShellper(line, { workspacePath: WS, architectName: 'reviewer' })).toBe(true); + }); + + it('is false for a non-shellper process (a bare claude child)', () => { + expect(isOwnArchitectShellper(claudeChildCmdline(), { workspacePath: WS, architectName: 'main' })).toBe(false); + }); + + it('is false when the architect name differs', () => { + const line = shellperCmdline({ architectName: 'reviewer' }); + expect(isOwnArchitectShellper(line, { workspacePath: WS, architectName: 'main' })).toBe(false); + }); + + it('is false when the cwd differs', () => { + const line = shellperCmdline({ cwd: '/some/other/ws', architectName: 'main' }); + expect(isOwnArchitectShellper(line, { workspacePath: WS, architectName: 'main' })).toBe(false); + }); +}); + +describe('classifyArchitectSessionHolder (Issue #1224)', () => { + const identity = { workspacePath: WS, architectName: 'main' }; + + it('classifies our own superseded shellper as reclaimable', () => { + const list = (): ProcessEntry[] => [{ pid: 18907, cmdline: shellperCmdline({}) }]; + const r = classifyArchitectSessionHolder({ sessionId: SID, identity, list }); + expect(r.reclaimable).toEqual([18907]); + expect(r.foreign).toBe(false); + }); + + it('NEVER-KILL-FOREIGN: a bare claude holding the id is foreign, not reclaimable', () => { + const list = (): ProcessEntry[] => [{ pid: 97841, cmdline: claudeChildCmdline() }]; + const r = classifyArchitectSessionHolder({ sessionId: SID, identity, list }); + expect(r.reclaimable).toEqual([]); + expect(r.foreign).toBe(true); + }); + + it('a shellper for a DIFFERENT identity holding the id is foreign', () => { + const list = (): ProcessEntry[] => [ + { pid: 500, cmdline: shellperCmdline({ architectName: 'someone-else' }) }, + ]; + const r = classifyArchitectSessionHolder({ sessionId: SID, identity, list }); + expect(r.reclaimable).toEqual([]); + expect(r.foreign).toBe(true); + }); + + it('ignores processes that do not hold the session and excludes selfPid', () => { + const list = (): ProcessEntry[] => [ + { pid: 1, cmdline: 'node server.js' }, + { pid: 18907, cmdline: shellperCmdline({}) }, + { pid: 42, cmdline: shellperCmdline({}) }, // would match but is self + ]; + const r = classifyArchitectSessionHolder({ sessionId: SID, identity, selfPid: 42, list }); + expect(r.reclaimable).toEqual([18907]); + expect(r.foreign).toBe(false); + }); + + it('returns no holders when the ps scan throws', () => { + const list = () => { throw new Error('ps failed'); }; + const r = classifyArchitectSessionHolder({ sessionId: SID, identity, list }); + expect(r).toEqual({ reclaimable: [], foreign: false }); + }); +}); + +describe('findOwnArchitectShellpers (Issue #1224)', () => { + it('finds our shellpers by identity regardless of the session they hold', () => { + const list = (): ProcessEntry[] => [ + { pid: 61274, cmdline: shellperCmdline({ sessionId: 'aaa', architectName: 'C', cwd: WS }) }, + { pid: 99, cmdline: shellperCmdline({ sessionId: 'bbb', architectName: 'other', cwd: WS }) }, + { pid: 12, cmdline: claudeChildCmdline() }, + ]; + const pids = findOwnArchitectShellpers({ identity: { workspacePath: WS, architectName: 'C' }, list }); + expect(pids).toEqual([61274]); + }); +}); + +describe('reapShellpers (Issue #1224)', () => { + it('SIGTERMs all, then SIGKILLs survivors, and resolves', async () => { + const kills: Array<[number, string]> = []; + let aliveMap: Record = { 10: true, 20: true }; + const r = reapShellpers([10, 20], { + kill: (pid, sig) => { + kills.push([pid, sig]); + // pid 10 dies on SIGTERM; pid 20 survives until SIGKILL + if (sig === 'SIGTERM' && pid === 10) aliveMap[10] = false; + if (sig === 'SIGKILL') aliveMap[pid] = false; + }, + isAlive: (pid) => aliveMap[pid] ?? false, + wait: async () => {}, + graceMs: 300, + pollMs: 100, + }); + await r; + expect(kills).toContainEqual([10, 'SIGTERM']); + expect(kills).toContainEqual([20, 'SIGTERM']); + // 20 survived the grace window → escalated + expect(kills).toContainEqual([20, 'SIGKILL']); + // 10 died on SIGTERM → not escalated + expect(kills).not.toContainEqual([10, 'SIGKILL']); + }); + + it('is a no-op for an empty pid list', async () => { + const kill = vi.fn(); + await reapShellpers([], { kill, isAlive: () => false, wait: async () => {} }); + expect(kill).not.toHaveBeenCalled(); + }); + + it('confirms death after SIGKILL before resolving (no resume race)', async () => { + // The holder survives SIGTERM and even the first poll after SIGKILL, then + // dies. reapShellpers must not resolve until it observes the death — a + // premature return would let the caller resume while the lock is still held. + let aliveChecks = 0; + const kill = vi.fn(); + const waits: number[] = []; + await reapShellpers([20], { + kill, + // alive for SIGTERM grace + one post-SIGKILL poll, dead thereafter. + isAlive: () => { aliveChecks += 1; return aliveChecks <= 5; }, + wait: async (ms) => { waits.push(ms); }, + graceMs: 300, + killGraceMs: 400, + pollMs: 100, + }); + expect(kill).toHaveBeenCalledWith(20, 'SIGTERM'); + expect(kill).toHaveBeenCalledWith(20, 'SIGKILL'); + // It kept polling after SIGKILL rather than returning immediately. + expect(waits.length).toBeGreaterThan(3); + }); +}); + +describe('reconcileArchitectSessionHolder (Issue #1224)', () => { + const identity = { workspacePath: WS, architectName: 'main' }; + + it('foreign holder → foreignHolder:true and NEVER reaps', async () => { + const reap = vi.fn(async () => {}); + const log = vi.fn(); + const list = (): ProcessEntry[] => [{ pid: 97841, cmdline: claudeChildCmdline() }]; + const r = await reconcileArchitectSessionHolder({ sessionId: SID, identity, list, reap, log }); + expect(r.foreignHolder).toBe(true); + expect(r.reclaimedPids).toEqual([]); + expect(reap).not.toHaveBeenCalled(); // architect's explicit never-kill-foreign requirement + expect(log).toHaveBeenCalledWith('WARN', expect.stringContaining('foreign process')); + }); + + it('own superseded shellper → reaps it and allows resume', async () => { + const reap = vi.fn(async () => {}); + const list = (): ProcessEntry[] => [{ pid: 18907, cmdline: shellperCmdline({}) }]; + const r = await reconcileArchitectSessionHolder({ sessionId: SID, identity, list, reap }); + expect(r.foreignHolder).toBe(false); + expect(r.reclaimedPids).toEqual([18907]); + expect(reap).toHaveBeenCalledWith([18907]); + }); + + it('no holder → foreignHolder:false and no reap', async () => { + const reap = vi.fn(async () => {}); + const list = (): ProcessEntry[] => [{ pid: 1, cmdline: 'node server.js' }]; + const r = await reconcileArchitectSessionHolder({ sessionId: SID, identity, list, reap }); + expect(r.foreignHolder).toBe(false); + expect(r.reclaimedPids).toEqual([]); + expect(reap).not.toHaveBeenCalled(); + }); + + it('a foreign holder alongside an own shellper still refuses to reap (foreign wins)', async () => { + const reap = vi.fn(async () => {}); + const list = (): ProcessEntry[] => [ + { pid: 18907, cmdline: shellperCmdline({}) }, + { pid: 97841, cmdline: claudeChildCmdline() }, + ]; + const r = await reconcileArchitectSessionHolder({ sessionId: SID, identity, list, reap }); + expect(r.foreignHolder).toBe(true); + expect(reap).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/codev/src/agent-farm/__tests__/tower-utils.test.ts b/packages/codev/src/agent-farm/__tests__/tower-utils.test.ts index 1724cee4e..5e22d4543 100644 --- a/packages/codev/src/agent-farm/__tests__/tower-utils.test.ts +++ b/packages/codev/src/agent-farm/__tests__/tower-utils.test.ts @@ -22,6 +22,7 @@ import { resolveArchitectRestart, siblingRegistrationIsLive, buildArchitectCrashLoopFallback, + sessionHasLiveHolder, } from '../servers/tower-utils.js'; // resolveArchitectRestart reads the architect row via getArchitectByName, and @@ -374,6 +375,74 @@ describe('resolveArchitectLaunch (Issue #832)', () => { vi.unstubAllEnvs(); } }); + + // Issue #1224: a stored id whose jsonl exists but is held by a live process + // must NOT be resumed (that bakes `--resume ` → "Session ID is already in + // use" → shellper crash loop). It mints fresh instead. + + it('mints fresh — not resume — when the owned stored id is held by a live process', () => { + writeSessionFixture(fakeHome, workspace, 'stored-abc'); + const log = vi.fn(); + const { args, sessionId, resumed, fallback } = resolveArchitectLaunch({ + workspacePath: workspace, name: 'main', baseArgs: [], storedSessionId: 'stored-abc', + homeDir: fakeHome, hasLiveHolder: () => true, log, + }); + expect(args).not.toContain('--resume'); // no collision baked + expect(args).toContain('--session-id'); + expect(resumed).toBe(false); + expect(fallback).toBeUndefined(); + expect(sessionId).toMatch(UUID_RE); + expect(sessionId).not.toBe('stored-abc'); + expect(log).toHaveBeenCalledWith('WARN', expect.stringContaining('held by a live process')); + }); + + it('still resumes the owned stored id when no live process holds it', () => { + writeSessionFixture(fakeHome, workspace, 'stored-abc'); + const { args, sessionId, resumed } = resolveArchitectLaunch({ + workspacePath: workspace, name: 'main', baseArgs: [], storedSessionId: 'stored-abc', + homeDir: fakeHome, hasLiveHolder: () => false, + }); + expect(args).toEqual(['--resume', 'stored-abc']); + expect(sessionId).toBe('stored-abc'); + expect(resumed).toBe(true); + }); + + it('does not probe for a live holder when the stored id is stale (no jsonl)', () => { + // Ownership fails first, so the (potentially expensive) holder scan is skipped. + const hasLiveHolder = vi.fn(() => true); + const { resumed } = resolveArchitectLaunch({ + workspacePath: workspace, name: 'main', baseArgs: [], storedSessionId: 'ghost-id', + homeDir: fakeHome, hasLiveHolder, + }); + expect(resumed).toBe(false); + expect(hasLiveHolder).not.toHaveBeenCalled(); + }); +}); + +describe('sessionHasLiveHolder (Issue #1224)', () => { + it('is true when a running process carries the session id in its argv', () => { + const list = () => [ + '/usr/bin/some-daemon', + 'claude --session-id abc-123-def --append-system-prompt ...', + 'node server.js', + ]; + expect(sessionHasLiveHolder('abc-123-def', { list })).toBe(true); + }); + + it('is true for a resume holder (--resume )', () => { + const list = () => ['claude --resume abc-123-def']; + expect(sessionHasLiveHolder('abc-123-def', { list })).toBe(true); + }); + + it('is false when no process carries the id', () => { + const list = () => ['claude --resume other-id', 'node server.js']; + expect(sessionHasLiveHolder('abc-123-def', { list })).toBe(false); + }); + + it('is false (not throwing) when the process scan fails', () => { + const list = () => { throw new Error('ps not found'); }; + expect(sessionHasLiveHolder('abc-123-def', { list })).toBe(false); + }); }); describe('siblingRegistrationIsLive (Issue #1150)', () => { diff --git a/packages/codev/src/agent-farm/servers/architect-session-holder.ts b/packages/codev/src/agent-farm/servers/architect-session-holder.ts new file mode 100644 index 000000000..f00021063 --- /dev/null +++ b/packages/codev/src/agent-farm/servers/architect-session-holder.ts @@ -0,0 +1,328 @@ +/** + * Issue #1224: detect and reclaim live processes that hold an architect's + * conversation session id, so an architect launch never bakes a colliding + * `claude --resume ` into a permanent crash loop. + * + * Two holder classes were observed after a workspace restart (forensic timeline + * on the issue): + * 1. OUR OWN superseded shellper for the same architect identity — a remnant + * from before the restart, or a previous crash-looping instance. Its claude + * child dies within seconds of every respawn, so the child is absent most + * of the time; the durable evidence is the *shellper parent's* argv, which + * carries the session id in its JSON config (`"--session-id",""`). + * 2. A FOREIGN process — e.g. an interactive `claude` the user started by hand + * on their own tty (holder case in incident 3). This must NEVER be touched: + * it is not ours to kill. + * + * The policy: reclaim (kill + resume) only OUR OWN superseded shellper, proven + * by shellper-main.js + matching session id + matching cwd + matching + * CODEV_ARCHITECT_NAME. Anything else that holds the id is foreign → mint fresh. + */ + +import { execFileSync } from 'node:child_process'; +import { realpathSync } from 'node:fs'; + +/** Marker identifying a Codev shellper process in a `ps` command line. */ +const SHELLPER_MARKER = 'shellper-main.js'; + +export interface ProcessEntry { + pid: number; + /** The full joined argv (as `ps ... -o args=` reports it). */ + cmdline: string; +} + +/** + * The session-flag needle forms a live holder's argv can carry the id in: + * - `--session-id ` / `--resume ` — a claude CHILD's shell argv + * - `--session-id=` / `--resume=` — the `=`-joined shell variant + * - `"--session-id",""` / `"--resume",""` — the shellper PARENT's + * JSON config argv (Issue #1224: catches a remnant shellper whose child is + * dead between crash-loop respawns). + */ +export function sessionIdNeedles(sessionId: string): string[] { + return ['--session-id', '--resume'].flatMap((flag) => [ + `${flag} ${sessionId}`, + `${flag}=${sessionId}`, + `"${flag}","${sessionId}"`, + ]); +} + +/** True when a process's argv references `sessionId` as a session-flag argument. */ +export function cmdlineHoldsSession(cmdline: string, sessionId: string): boolean { + const needles = sessionIdNeedles(sessionId); + return needles.some((needle) => cmdline.includes(needle)); +} + +/** Canonicalize a path for comparison; fall back to the input when realpath fails. */ +function realpathOrSelf(p: string): string { + try { + return realpathSync(p); + } catch { + return p; + } +} + +/** + * Extract a `"key":"value"` string field from a shellper's JSON config as it + * appears verbatim in the `ps` argv. Regex rather than JSON.parse: the config + * embeds the full process env, so the blob is large and a partial/edge `ps` + * line must degrade to "field absent" rather than throw. + */ +function extractJsonStringField(cmdline: string, key: string): string | null { + const m = cmdline.match(new RegExp(`"${key}"\\s*:\\s*"([^"]+)"`)); + return m ? m[1] : null; +} + +export interface ArchitectIdentity { + /** Absolute workspace path the shellper runs in (`cwd` in its config). */ + workspacePath: string; + /** The architect name (shellper config env `CODEV_ARCHITECT_NAME`). */ + architectName: string; +} + +/** + * True when `cmdline` is one of OUR OWN shellper processes for `identity`: + * a `shellper-main.js` whose JSON config carries the matching workspace cwd and + * `CODEV_ARCHITECT_NAME`. The cwd match tolerates symlink variants (macOS + * `/tmp` vs `/private/tmp`), mirroring verifySessionOwnership. + * + * This is the airtight-identity gate before any kill: if any field cannot be + * positively confirmed, this returns false and the caller must not reclaim. + */ +export function isOwnArchitectShellper(cmdline: string, identity: ArchitectIdentity): boolean { + if (!cmdline.includes(SHELLPER_MARKER)) return false; + + const cfgName = extractJsonStringField(cmdline, 'CODEV_ARCHITECT_NAME'); + if (cfgName !== identity.architectName) return false; + + const cfgCwd = extractJsonStringField(cmdline, 'cwd'); + if (!cfgCwd) return false; + const wanted = new Set([identity.workspacePath, realpathOrSelf(identity.workspacePath)]); + const got = new Set([cfgCwd, realpathOrSelf(cfgCwd)]); + const cwdMatches = [...got].some((g) => wanted.has(g)); + if (!cwdMatches) return false; + + return true; +} + +/** + * Snapshot every running process as {pid, cmdline}. `ps -ww` prevents argv + * truncation (the shellper config blob is large) on both BSD and coreutils. + * Throws on `ps` failure; callers decide how to degrade. + */ +export function listProcessEntries(): ProcessEntry[] { + const out = execFileSync('ps', ['-ww', '-eo', 'pid=,args='], { + encoding: 'utf-8', + timeout: 5000, + maxBuffer: 16 * 1024 * 1024, + }); + const entries: ProcessEntry[] = []; + for (const line of out.split('\n')) { + const trimmed = line.trimStart(); + const sp = trimmed.indexOf(' '); + if (sp <= 0) continue; + const pid = parseInt(trimmed.slice(0, sp), 10); + if (Number.isNaN(pid) || pid <= 0) continue; + entries.push({ pid, cmdline: trimmed.slice(sp + 1) }); + } + return entries; +} + +export interface SessionHolderClassification { + /** PIDs of OUR OWN superseded shellpers holding this session (safe to reap). */ + reclaimable: number[]; + /** True when some holder is NOT positively ours — must not be touched. */ + foreign: boolean; +} + +/** + * Classify every live holder of `sessionId` into reclaimable-own vs foreign. + * A holder is reclaimable only when it is one of our own architect shellpers + * (identity-matched); any other holder (a bare `claude`, a shellper for a + * different identity, an unparseable match) is foreign. + * + * On `ps` failure returns `{reclaimable: [], foreign: false}` — no positive + * evidence, so the caller resumes as it would have before this guard existed. + */ +export function classifyArchitectSessionHolder(opts: { + sessionId: string; + identity: ArchitectIdentity; + /** Exclude this pid (e.g. Tower's own) from consideration. */ + selfPid?: number; + /** Test seam: override the process snapshot. */ + list?: () => ProcessEntry[]; +}): SessionHolderClassification { + const list = opts.list ?? listProcessEntries; + let entries: ProcessEntry[]; + try { + entries = list(); + } catch { + return { reclaimable: [], foreign: false }; + } + + const reclaimable: number[] = []; + let foreign = false; + for (const { pid, cmdline } of entries) { + if (opts.selfPid !== undefined && pid === opts.selfPid) continue; + if (!cmdlineHoldsSession(cmdline, opts.sessionId)) continue; + if (isOwnArchitectShellper(cmdline, opts.identity)) { + reclaimable.push(pid); + } else { + foreign = true; + } + } + return { reclaimable, foreign }; +} + +/** + * Find PIDs of OUR OWN architect shellpers for `identity`, regardless of which + * session they hold. Used by remove-architect to reap a live-process-without-a- + * registry-row zombie (Issue #1224 symptom B) — there is no stored session id to + * key on, so identity (shellper-main.js + cwd + CODEV_ARCHITECT_NAME) is the key. + */ +export function findOwnArchitectShellpers(opts: { + identity: ArchitectIdentity; + selfPid?: number; + list?: () => ProcessEntry[]; +}): number[] { + const list = opts.list ?? listProcessEntries; + let entries: ProcessEntry[]; + try { + entries = list(); + } catch { + return []; + } + const pids: number[] = []; + for (const { pid, cmdline } of entries) { + if (opts.selfPid !== undefined && pid === opts.selfPid) continue; + if (isOwnArchitectShellper(cmdline, opts.identity)) pids.push(pid); + } + return pids; +} + +/** Whether a pid is still alive (signal 0 probe). */ +function pidAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch (err) { + // ESRCH → gone; EPERM → alive but not ours to signal (treat as alive). + return (err as NodeJS.ErrnoException).code === 'EPERM'; + } +} + +/** Kill a shellper's whole process group (leader is detached), best-effort. */ +function killProcessGroup(pid: number, signal: NodeJS.Signals): void { + try { + process.kill(-pid, signal); + } catch { + // Group gone or not permitted — fall back to the single pid. + try { + process.kill(pid, signal); + } catch { + /* already dead */ + } + } +} + +const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms)); + +/** + * Reap the given superseded-shellper PIDs: SIGTERM the process group, poll for + * death, then SIGKILL any survivor. Resolves once every pid is gone (or the + * SIGKILL grace elapses). The wait matters: the killed shellper's child must + * release the session lock before the caller's fresh `claude --resume` runs, + * else the collision recurs. + * + * Seams (`isAlive`, `kill`, `wait`) keep it unit-testable without real signals. + */ +export async function reapShellpers( + pids: number[], + opts?: { + isAlive?: (pid: number) => boolean; + kill?: (pid: number, signal: NodeJS.Signals) => void; + wait?: (ms: number) => Promise; + graceMs?: number; + killGraceMs?: number; + pollMs?: number; + }, +): Promise { + if (pids.length === 0) return; + const isAlive = opts?.isAlive ?? pidAlive; + const kill = opts?.kill ?? killProcessGroup; + const wait = opts?.wait ?? sleep; + const graceMs = opts?.graceMs ?? 3000; + const killGraceMs = opts?.killGraceMs ?? 2000; + const pollMs = opts?.pollMs ?? 100; + + const pollUntilDead = async (deadlineMs: number): Promise => { + let waited = 0; + while (waited < deadlineMs && pids.some((pid) => isAlive(pid))) { + await wait(pollMs); + waited += pollMs; + } + }; + + for (const pid of pids) kill(pid, 'SIGTERM'); + await pollUntilDead(graceMs); + + const survivors = pids.filter((pid) => isAlive(pid)); + if (survivors.length > 0) { + for (const pid of survivors) kill(pid, 'SIGKILL'); + // Confirm SIGKILL actually took effect before returning: the caller resumes + // `claude --resume ` next, and a holder still in the kernel's exit path + // has not yet released the session lock — returning early would re-open the + // very collision race this reap exists to close (codex review, Issue #1224). + await pollUntilDead(killGraceMs); + } +} + +export type ArchitectLogger = (level: 'INFO' | 'WARN' | 'ERROR', message: string) => void; + +/** + * Reconcile a stored architect session id against the live process table before + * launch, resolving whether the caller may resume it. + * + * - A FOREIGN holder → `{ foreignHolder: true }`: the caller must mint fresh + * (the session is genuinely in use by something we must not touch). + * - Only OUR OWN superseded shellper(s) hold it → reap them and return + * `{ foreignHolder: false }`: the session is now free, resume it (mint-or- + * RECLAIM, preserving the conversation instead of abandoning it). + * - No holder → `{ foreignHolder: false }`: resume as usual. + */ +export async function reconcileArchitectSessionHolder(opts: { + sessionId: string; + identity: ArchitectIdentity; + selfPid?: number; + log?: ArchitectLogger; + list?: () => ProcessEntry[]; + reap?: (pids: number[]) => Promise; +}): Promise<{ foreignHolder: boolean; reclaimedPids: number[] }> { + const { reclaimable, foreign } = classifyArchitectSessionHolder({ + sessionId: opts.sessionId, + identity: opts.identity, + selfPid: opts.selfPid, + list: opts.list, + }); + + const shortId = opts.sessionId.slice(0, 8); + if (foreign) { + opts.log?.( + 'WARN', + `Architect '${opts.identity.architectName}' session ${shortId}… is held by a foreign process; minting a fresh session (holder left untouched) in ${opts.identity.workspacePath}`, + ); + return { foreignHolder: true, reclaimedPids: [] }; + } + + if (reclaimable.length === 0) { + return { foreignHolder: false, reclaimedPids: [] }; + } + + opts.log?.( + 'WARN', + `Architect '${opts.identity.architectName}' session ${shortId}… is held by our own superseded shellper(s) [${reclaimable.join(', ')}]; reaping to reclaim the conversation in ${opts.identity.workspacePath}`, + ); + const reap = opts.reap ?? ((pids: number[]) => reapShellpers(pids)); + await reap(reclaimable); + return { foreignHolder: false, reclaimedPids: reclaimable }; +} diff --git a/packages/codev/src/agent-farm/servers/tower-instances.ts b/packages/codev/src/agent-farm/servers/tower-instances.ts index 8a03ad0bc..dd4157d9a 100644 --- a/packages/codev/src/agent-farm/servers/tower-instances.ts +++ b/packages/codev/src/agent-farm/servers/tower-instances.ts @@ -30,6 +30,11 @@ import { siblingRegistrationIsLive, buildArchitectCrashLoopFallback, } from './tower-utils.js'; +import { + reconcileArchitectSessionHolder, + findOwnArchitectShellpers, + reapShellpers, +} from './architect-session-holder.js'; import { autoNumberArchitectName, validateArchitectName, @@ -527,11 +532,26 @@ export async function launchInstance(workspacePath: string): Promise<{ success: try { storedSessionId = getArchitectByName(resolvedPath, DEFAULT_ARCHITECT_NAME)?.sessionId ?? null; } catch { /* global.db unreadable — spawn fresh */ } + // Issue #1224: reconcile the stored session id against the live process + // table before resuming (default 'main' recovers via `workspace start`, + // so this launch path is main's mint-or-reclaim site). Reap our own + // superseded main shellper; a foreign holder forces a fresh session. + let foreignMainHolder = false; + if (storedSessionId) { + ({ foreignHolder: foreignMainHolder } = await reconcileArchitectSessionHolder({ + sessionId: storedSessionId, + identity: { workspacePath: resolvedPath, architectName: DEFAULT_ARCHITECT_NAME }, + selfPid: process.pid, + log: _deps.log, + })); + } const { args: cmdArgs, env: harnessEnv, sessionId: mainSessionId, resumed, fallback } = resolveArchitectLaunch({ workspacePath, name: DEFAULT_ARCHITECT_NAME, baseArgs: cmdParts.slice(1), storedSessionId, + hasLiveHolder: () => foreignMainHolder, + log: _deps.log, }); if (resumed && mainSessionId) { _deps.log('INFO', `Resuming architect '${DEFAULT_ARCHITECT_NAME}' session ${mainSessionId.slice(0, 8)}… in ${workspacePath}`); @@ -1008,11 +1028,26 @@ export async function addArchitect( try { storedSessionId = getArchitectByName(resolvedPath, name)?.sessionId ?? null; } catch { /* state.db unreadable — spawn fresh */ } + // Issue #1224: before deciding to resume, reconcile the stored session id + // against the live process table. Reap our OWN superseded shellper(s) holding + // it (mint-or-reclaim), and treat a foreign holder as "don't resume" so + // resolveArchitectLaunch mints fresh instead of baking a colliding --resume. + let foreignHolder = false; + if (storedSessionId) { + ({ foreignHolder } = await reconcileArchitectSessionHolder({ + sessionId: storedSessionId, + identity: { workspacePath: resolvedPath, architectName: name }, + selfPid: process.pid, + log: _deps.log, + })); + } const { args: cmdArgs, env: harnessEnv, sessionId: conversationSessionId, resumed, fallback } = resolveArchitectLaunch({ workspacePath, name, baseArgs: cmdParts.slice(1), storedSessionId, + hasLiveHolder: () => foreignHolder, + log: _deps.log, }); if (resumed && conversationSessionId) { _deps.log('INFO', `Resuming architect '${name}' session ${conversationSessionId.slice(0, 8)}… in ${workspacePath}`); @@ -1237,7 +1272,17 @@ export async function removeArchitect( hasStaleRow = getArchitectByName(resolvedPath, name) !== null; } catch { /* registry unreadable: fall through to not-found */ } const staleTerminalIds = findArchitectTerminalSessionIds(name, resolvedPath, workspacePath); - if (hasStaleRow || staleTerminalIds.length > 0) { + // Issue #1224 (symptom B): a shellper can outlive its registry row entirely + // — a give-up husk or a stale remnant that deregistered while looping. With + // no live terminal AND no rows, the historical code returned "not found" + // while the process kept running, and remove-architect could not clear it. + // Reap any of our own shellpers matching this identity so this command is + // the recovery tool for a live-process-without-a-row zombie too. + const zombiePids = findOwnArchitectShellpers({ + identity: { workspacePath: resolvedPath, architectName: name }, + selfPid: process.pid, + }); + if (hasStaleRow || staleTerminalIds.length > 0 || zombiePids.length > 0) { try { if (hasStaleRow) { setArchitectByName(resolvedPath, name, null); @@ -1251,7 +1296,10 @@ export async function removeArchitect( error: `Architect '${name}' has no live terminal, and deleting its stale state failed: ${(err as Error).message}. Retry 'afx workspace remove-architect --name ${name}'.`, }; } - _deps.log('INFO', `Purged stale architect state for '${name}' from workspace ${workspacePath} (no live terminal; registration=${hasStaleRow}, terminal rows=${staleTerminalIds.length})`); + if (zombiePids.length > 0) { + await reapShellpers(zombiePids); + } + _deps.log('INFO', `Purged stale architect state for '${name}' from workspace ${workspacePath} (no live terminal; registration=${hasStaleRow}, terminal rows=${staleTerminalIds.length}, zombie shellpers=${zombiePids.length})`); return { success: true }; } return { success: false, error: `Architect '${name}' not found in workspace '${workspacePath}'.` }; diff --git a/packages/codev/src/agent-farm/servers/tower-utils.ts b/packages/codev/src/agent-farm/servers/tower-utils.ts index ae68879c6..fdd415adc 100644 --- a/packages/codev/src/agent-farm/servers/tower-utils.ts +++ b/packages/codev/src/agent-farm/servers/tower-utils.ts @@ -9,6 +9,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { tmpdir } from 'node:os'; +import { execFileSync } from 'node:child_process'; import type { ServerResponse } from 'node:http'; import type { RateLimitEntry } from './tower-types.js'; import crypto from 'node:crypto'; @@ -17,6 +18,7 @@ import { getArchitectHarness } from '../utils/config.js'; import type { HarnessProvider } from '../utils/harness.js'; import { getArchitectByName, setArchitectSessionId } from '../state.js'; import type { CrashLoopFallback } from '../../terminal/session-manager.js'; +import { cmdlineHoldsSession } from './architect-session-holder.js'; // ============================================================================ // Rate Limiting @@ -211,6 +213,58 @@ function sessionIsOwned( } } +/** + * Snapshot every running process's command line (argv joined). Used to detect a + * live holder of a conversation session id (Issue #1224). `ps -A -o args=` is + * accepted by both BSD (macOS) and coreutils (Linux) `ps`; the empty header + * (`args=`) yields one command line per line. A missing/failing `ps` throws, + * which the caller treats as "could not determine" (see `sessionHasLiveHolder`). + */ +function listProcessCommandLines(): string[] { + const out = execFileSync('ps', ['-ww', '-A', '-o', 'args='], { + encoding: 'utf-8', + timeout: 5000, + maxBuffer: 16 * 1024 * 1024, + }); + return out.split('\n').filter((line) => line.trim() !== ''); +} + +/** + * Issue #1224: true when some live process is already running with `sessionId` + * as a session-flag argument — a claude child (`--session-id ` / `--resume + * `, incl. the `=`-joined forms) OR a shellper parent whose JSON config + * carries it (`"--session-id",""`). The shellper-parent form matters because + * a crash-looping remnant's claude child is dead most of the time; the parent's + * argv is the durable evidence. Resuming a held id bakes `claude --resume `, + * which dies instantly with "Session ID is already in use" and crash-loops the + * shellper forever. + * + * The match is anchored to the launch flags (see `sessionIdNeedles`) rather than + * a bare substring: a session id is short in tests and could otherwise coincide + * with an unrelated path in the process table. + * + * This is the simple boolean guard used on the restart-bake path (mint fresh on + * any holder). The richer own-vs-foreign classification and mint-or-reclaim + * policy lives in `architect-session-holder.ts` and is wired into the + * add-architect / launch paths. + * + * On any scan failure (`ps` unavailable/timeout) it returns `false`: this guard + * is purely additive — it diverts to fresh ONLY on positive evidence of a live + * holder, and never makes an un-held resume worse than today's behavior. The + * `list` seam exists for tests. + */ +export function sessionHasLiveHolder( + sessionId: string, + opts?: { list?: () => string[] }, +): boolean { + const list = opts?.list ?? listProcessCommandLines; + try { + return list().some((cmdline) => cmdlineHoldsSession(cmdline, sessionId)); + } catch { + return false; + } +} + /** * Issue #1150: decide whether a persisted sibling architect row still deserves * a respawn, absent any live-terminal evidence (the caller checks @@ -284,6 +338,13 @@ export function resolveArchitectLaunch(opts: { storedSessionId?: string | null; /** Test seam: pins the home dir the ownership check resolves the session store under. */ homeDir?: string; + /** + * Issue #1224 test seam: override the live-holder detector. Defaults to + * scanning the real process table via `sessionHasLiveHolder`. + */ + hasLiveHolder?: (sessionId: string) => boolean; + /** Optional logger, so the divert-to-fresh decision (Issue #1224) is diagnosable. */ + log?: (level: 'INFO' | 'WARN' | 'ERROR', message: string) => void; }): { args: string[]; env: Record; @@ -292,6 +353,7 @@ export function resolveArchitectLaunch(opts: { fallback?: ArchitectLaunchFallback; } { const { workspacePath, baseArgs, storedSessionId, homeDir } = opts; + const hasLiveHolder = opts.hasLiveHolder ?? sessionHasLiveHolder; const harness = getArchitectHarness(workspacePath); // 1. No resumable-session support → plain fresh, nothing to persist. @@ -306,10 +368,18 @@ export function resolveArchitectLaunch(opts: { const skipResume = process.env['CODEV_SKIP_RESUME'] === '1'; // 2. Resume the persisted conversation (role injection skipped) — but only - // when the session still exists on disk for this workspace. A failed or - // throwing check falls through to a fresh spawn (Issue #1145: a stored id - // can outlive its jsonl, and resuming it would crash-loop the restart). - if (!skipResume && storedSessionId && sessionIsOwned(harness, storedSessionId, workspacePath, homeDir)) { + // when the session still exists on disk for this workspace (Issue #1145: a + // stored id can outlive its jsonl) AND no live process is currently holding + // it (Issue #1224). A held id — a stale pre-restart shellper's claude child or + // an unrelated foreground claude — makes `claude --resume ` die with + // "Session ID is already in use" and crash-loop the shellper forever; the + // #1145 existence check can't catch it, because a held session's jsonl exists + // precisely because the holder is writing it. Either failure mints fresh. + const canResume = !skipResume && storedSessionId + && sessionIsOwned(harness, storedSessionId, workspacePath, homeDir); + if (canResume && hasLiveHolder(storedSessionId!)) { + opts.log?.('WARN', `Architect '${opts.name}' stored session ${storedSessionId!.slice(0, 8)}… is held by a live process; minting a fresh session to avoid a collision crash loop in ${workspacePath}`); + } else if (canResume) { const fallbackSessionId = crypto.randomUUID(); const fresh = buildArchitectArgs( [...baseArgs, ...harness.session.newSessionArgs(fallbackSessionId)], @@ -348,7 +418,7 @@ export function resolveArchitectRestart( workspacePath: string, architectName: string, baseArgs: string[], - opts?: { homeDir?: string }, + opts?: { homeDir?: string; log?: (level: 'INFO' | 'WARN' | 'ERROR', message: string) => void }, ): { args: string[]; env: Record; @@ -359,7 +429,16 @@ export function resolveArchitectRestart( } { const storedSessionId = getArchitectByName(workspacePath, architectName)?.sessionId ?? null; const resolved = resolveArchitectLaunch({ - workspacePath, name: architectName, baseArgs, storedSessionId, homeDir: opts?.homeDir, + workspacePath, name: architectName, baseArgs, storedSessionId, homeDir: opts?.homeDir, log: opts?.log, + // Issue #1224: never run the live-holder check on the restart-bake path. The + // holder of this session at bake time is THIS shellper's own child (its argv + // carries --resume ), so a self-detection would bake fresh restart args + // on every healthy reconnect and lose conversation continuity on the next + // child crash. Collision-avoidance for a genuinely-held id belongs at the + // add-architect / main-launch reconcile layer (mint-or-reclaim), and the + // #1149 crash-loop fallback is the runtime backstop if a baked resume does + // collide. So resume when owned; do not holder-check here. + hasLiveHolder: () => false, }); return { ...resolved, storedSessionId }; } diff --git a/packages/codev/src/terminal/__tests__/session-manager.test.ts b/packages/codev/src/terminal/__tests__/session-manager.test.ts index 54089e33e..ef0b02b49 100644 --- a/packages/codev/src/terminal/__tests__/session-manager.test.ts +++ b/packages/codev/src/terminal/__tests__/session-manager.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { EventEmitter } from 'node:events'; import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; @@ -2241,3 +2242,73 @@ describe('schema migration', () => { expect(GLOBAL_SCHEMA).toContain('shellper_start_time INTEGER'); }); }); + +describe('crash-loop give-up (Issue #1224)', () => { + // Drive the setupAutoRestart exit handler to the maxRestarts give-up branch + // and assert it reaps the shellper husk (process-group SIGTERM) so give-up + // leaves no live-process-without-a-registry-row zombie. + function fakeSession(pid: number, socketDir: string) { + const client = new EventEmitter() as any; + client.spawn = vi.fn(); + return { + client, + socketPath: path.join(socketDir, 'giveup.sock'), + pid, + startTime: 0, + options: { + sessionId: 'giveup-1', + command: 'claude', + args: [], + cwd: '/tmp', + env: {}, + restartOnExit: true, + restartDelay: 1, + maxRestarts: 1, + }, + restartCount: 1, // already at the cap → next failing exit gives up + restartResetTimer: null, + failingExitTimes: [] as number[], + stderrBuffer: null, + stderrStream: null, + stderrTailLogged: false, + recoveryRounds: 0, + lastRecoveryAt: 0, + }; + } + + it('SIGTERMs the shellper process group on give-up', () => { + const socketDir = tmpDir(); + const HUSK_PID = 424242; + const manager = new SessionManager({ + socketDir, + shellperScript: '/nonexistent/shellper.js', + nodeExecutable: process.execPath, + }); + const session = fakeSession(HUSK_PID, socketDir); + (manager as any).sessions.set('giveup-1', session); + + const killed: Array<[number, string | number | undefined]> = []; + const originalKill = process.kill; + process.kill = ((pid: number, signal?: string | number) => { + killed.push([pid, signal]); + return true; + }) as typeof process.kill; + + const errors: string[] = []; + manager.on('session-error', (_id, err) => errors.push(err.message)); + + try { + (manager as any).setupAutoRestart(session, 'giveup-1'); + session.client.emit('exit', { code: 1, signal: null }); + + // The whole process group is signalled (negative pid). + expect(killed).toContainEqual([-HUSK_PID, 'SIGTERM']); + // Give-up surfaced as a session-error, and the session was dropped. + expect(errors.some((m) => m.includes('Max restarts'))).toBe(true); + expect((manager as any).sessions.has('giveup-1')).toBe(false); + } finally { + process.kill = originalKill; + rmrf(socketDir); + } + }); +}); diff --git a/packages/codev/src/terminal/session-manager.ts b/packages/codev/src/terminal/session-manager.ts index a75500323..dcb7fccd8 100644 --- a/packages/codev/src/terminal/session-manager.ts +++ b/packages/codev/src/terminal/session-manager.ts @@ -1085,7 +1085,23 @@ export class SessionManager extends EventEmitter { const maxRestarts = session.options.maxRestarts ?? 50; if (session.restartCount >= maxRestarts) { - this.log(`Session ${sessionId} exhausted max restarts (${maxRestarts})`); + this.log(`Session ${sessionId} exhausted max restarts (${maxRestarts}); giving up (last exit code=${exit.code ?? -1}, signal=${exit.signal ?? null})`); + // Issue #1224: capture the dying child's stderr for the wedge-after-free + // diagnosis. Reset the once-only guard so the give-up tail logs even if + // an earlier failing exit already emitted one — the give-up child's + // reason is the one worth having. + session.stderrTailLogged = false; + this.logStderrTail(sessionId, session, exit.code ?? -1); + // 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. + try { + process.kill(-session.pid, 'SIGTERM'); + } catch { + /* already gone or not permitted */ + } this.emit('session-error', sessionId, new Error(`Max restarts (${maxRestarts}) exceeded`)); // Remove the exhausted session from the map this.removeDeadSession(sessionId);