[Bugfix #1224] Skip architect session resume when a live process holds the id

[waleedkadous]

Summary

After a workspace restart, re-adding architects via afx workspace add-architect could put an architect terminal into a permanent crash loop:

Error: Session ID <uuid> is already in use

The shellper restarted its claude child forever, every child dying instantly. This fixes the root cause: an architect launch would resume a persisted conversation session id without checking whether a live process already held it.

Fixes #1224.

Root Cause

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 session jsonl exists on disk (sessionIsOwned → verifySessionOwnership).

That 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. Two live holders were reported:

1. A stale pre-restart shellper's claude child for the same architect name (shellpers survive Tower restarts by design).
2. An unrelated foreground claude the user started by hand.

In both cases the launch bakes claude --resume <id>, claude sees the id already locked and exits with "Session ID is already in use", and the shellper auto-restarts into the same collision forever. (This is the reporter's suggested fix (a): never reuse a persisted session id without verifying no live holder.)

Fix

• sessionHasLiveHolder(sessionId, {list?}) — scans ps -A -o args= for a live process launched with --session-id <id> or --resume <id> (both space- and =-joined forms). The match is flag-anchored, not a bare substring: a session id is short in tests and a raw substring coincided with unrelated paths in the process table; the flags are exactly how a holder is spawned. On any scan failure (ps missing/timeout) it returns false — the guard is purely additive, diverting to a fresh session only on positive evidence of a holder, and never making an un-held resume worse than before.
• resolveArchitectLaunch resume branch now requires sessionIsOwned(...) && !hasLiveHolder(...). A held id mints a fresh session (with a WARN log) instead of baking a colliding --resume. Ownership is still checked first, so the ps scan is skipped for stale ids. hasLiveHolder and log are injectable seams.
• resolveArchitectRestart threads the logger, so the shellper restart-bake path inherits the guard for free.
• tower-instances.ts passes log: _deps.log at the addArchitect and launchInstance main-path call sites, so the divert-to-fresh decision is diagnosable in Tower logs.

This is observation-only — it never touches the holder. That is deliberate: holder case 2 is the user's own foreground terminal, so killing-to-reclaim would be wrong. Minting fresh is universally safe for both cases, and the architect ends up with a working conversation instead of a crash loop.

Scope note (what this PR intentionally does not do)

The issue also lists suggested fixes (b) and (c):

• (b) crash-loop breaker in the shellper already largely exists — Tower recovery: Claude architect crash-loops on stale --resume with no user escape #1149's maybeApplyCrashLoopFallback (3 failing exits in 30s → swap to a fresh launch). This PR removes the collision at its source so that backstop isn't reached in the reported path.
• (c) atomic deregistration ↔ process death (Symptom B: a live shellper without a registry row, or a row without a process) is a separate, more architectural concern. Recommend a follow-up issue rather than expanding this BUGFIX.

Test Plan

New cases in tower-utils.test.ts:

• Held owned id → mint fresh, not --resume (asserts no collision baked + a WARN log). Fails without the fix.
• No-holder owned id → still resumes as before (no regression).
• Ownership short-circuits the scan for a stale id (holder probe not invoked).
• sessionHasLiveHolder units: --session-id/--resume holders match, an unrelated id doesn't, and a scan failure returns false (never throws).

Validation: porch check — build ✓, tests ✓ (full suite). tsc --noEmit clean.

[waleedkadous]

CMAP review results

Model	Verdict	Confidence	Key issues
Codex (gpt-5.4-codex)	✅ APPROVE	HIGH	None
Claude (opus)	✅ APPROVE	HIGH	None
Gemini (agy)	⚪ skipped (non-blocking)	—	Lane could not produce a --type pr VERDICT in this worktree (known limitation); no findings

Both completing reviewers approved with HIGH confidence and no requested changes. Claude specifically flagged the TOCTOU windows (holder dies/appears between check and launch) as safe — either mints fresh unnecessarily or falls back to the existing #1149 crash-loop backstop — and confirmed the guard is purely additive (scan failure → false → no regression).

[waleedkadous]

Excellent root-cause work — the #1145-guard analysis ("a held session's jsonl exists precisely because the holder is writing it") is exactly right, and the fail-open, observation-only design is well judged. I'd like to see three changes before merge; the third is an approved scope expansion, not creep.

1. Detect shellper parents too. A remnant shellper's own argv carries the session id in JSON form — "--session-id","<id>" (quote-and-comma joined; verified against a live process table today). In a crash-looping remnant the claude child is dead most of the time (<8s alive per respawn), so the current space/= needles miss exactly the incident-1 holder class during most of its life. Adding the JSON-quoted forms of both flags to the needle set makes detection reliable instead of timing-dependent.

2. Mint-or-reclaim, not mint-only. When the live holder is verifiably our own superseded shellper for the same architect identity, abandoning the conversation is a worse outcome than necessary. In that case: kill the stale instance (whole process group, SIGTERM with SIGKILL escalation — the PtySession.kill pattern) and resume the session. Keep mint-fresh for every other holder — a foreign process may be the user's own terminal and must never be touched. The identification must be airtight before any kill: a shellper-main.js process whose config argv carries the same session id AND the same workspace cwd AND the same architect name, and which is not the instance being launched. If any of those can't be positively confirmed, fall through to mint-fresh. Please cover the never-kill-foreign branch in tests explicitly.

3. Fix symptom B in this PR as well. Two concrete legs:

• Reconcile on add/launch: add-architect (and launch) for a name that already has a live shellper for the same identity should reattach to or reap that instance rather than spawning a duplicate that fights it for the session — this is the lifecycle hole that manufactured both collisions.
• Registry/process consistency: a shellper that gives up (the Tower recovery: Claude architect crash-loops on stale --resume with no user escape #1149 crash-loop breaker) should deregister cleanly, and remove-architect should be able to clear a live-process-without-registry-row zombie instead of failing "not found" while the process loops on.

For the wedge-after-free (incident 3's key datapoint: children kept dying <8s with the session demonstrably free), please capture and log the dying child's stderr/exit reason as part of this work — if the actual cause surfaces and is separable, document it in the PR rather than chasing it blind.

Practical pointers: the full forensic timeline (PIDs, lstart times, session ids, the manual recovery that worked, plus today's live main-architect recurrence and its different recovery path) is in the comments on #1224. The reporting workspace has offered to run live diagnostics on the next occurrence if that helps.

[waleedkadous]

Iteration 2 — addressed the three review asks

Thanks for the detailed review. All three landed on this branch (commits f28f3300, f7d13245).

1. Detect shellper parents (JSON-argv needles)

sessionIdNeedles now also matches the shellper parent's config-JSON form "--session-id","<id>" / "--resume","<id>" (alongside the child's --session-id <id> / --resume <id> / = forms), and the scan uses ps -ww to avoid truncating the large config blob. This catches the incident-1 remnant shellper whose claude child is dead between crash-loop respawns.

2. Mint-or-reclaim (never touch foreign)

New servers/architect-session-holder.ts:

• classifyArchitectSessionHolder splits holders into reclaimable-own vs foreign. Reclaimable requires an airtight identity: a shellper-main.js process whose JSON config carries the same session id AND same cwd (symlink-tolerant) AND same CODEV_ARCHITECT_NAME, and isn't self. Anything else that holds the id — a bare claude, a shellper for a different identity, an unparseable match — is foreign.
• reconcileArchitectSessionHolder: foreign → {foreignHolder:true} (mint fresh, holder untouched); our own superseded shellper(s) → reapShellpers (process-group SIGTERM → poll → SIGKILL) then resume; none → resume. A foreign holder alongside an own one still refuses to reap (foreign wins).
• Wired into addArchitect and launchInstance's main path (main recovers via workspace start, so that's main's reclaim site).

The never-kill-foreign branch is covered explicitly (reap asserted not called for a foreign holder, and for foreign-alongside-own).

3. Symptom B

• remove-architect now reaps a live-process-without-a-registry-row zombie (findOwnArchitectShellpers by identity) instead of returning not found while the process loops.
• crash-loop give-up (SessionManager maxRestarts): reaps the shellper husk (process-group SIGTERM) so give-up leaves no row-gone/process-alive zombie, and captures the dying child's exit code/signal + shellper stderr tail.

On the wedge-after-free (incident 3: children dying <8s with the session demonstrably free): claude's own error surfaces through the PTY data / terminal ring buffer, not the shellper's stderr, so give-up now logs the exit code/signal + shellper stderr for diagnosis and reaps the husk so the loop can't persist. The definitive cause of that specific datapoint is captured-for-diagnosis rather than claimed-fixed — flagging honestly per your "document rather than chase blind." The reporter's offer to run live diagnostics on the next occurrence would confirm it.

Validation

Full build ✓ · full unit suite 3582 passed / 48 skipped / 0 failed · tsc --noEmit clean. New tests in architect-session-holder.test.ts and a give-up husk-reap test in session-manager.test.ts.

Re-running CMAP now and will re-request the pr gate.

[waleedkadous]

CMAP iteration 2 + responses

Model	Verdict	Confidence	Key issues
Claude (opus)	✅ APPROVE	HIGH	None
Codex (gpt-5.4-codex)	🔶 REQUEST_CHANGES	HIGH	reap SIGKILL race; PR artifacts
Gemini (agy)	⚪ skipped (non-blocking)	—	lane can't emit a --type pr VERDICT here

Codex #1 — reapShellpers didn't confirm death after SIGKILL → fixed (commit 5c9b4cb9)

Real bug. After escalating to SIGKILL the function returned immediately, so the reclaim caller could resume claude --resume <id> while the holder was still in the kernel's exit path and hadn't released the session lock — re-opening the very collision race the reap exists to close. Added a killGraceMs post-SIGKILL confirmation poll and a test asserting it keeps polling until death.

Codex #2 — status.yaml / thread.md in the PR → rebuttal (deferring to you)

Both are there by design, not stray artifacts I introduced:

• codev/projects/.../status.yaml is committed by porch's own chore(porch) commits — builders must never touch it, and every prior merged bugfix (e.g. /arch-init: architect auto-state-saving — save to codev/state/<name>.md at checkpoints, then suggest /clear (supersedes #1212) #1220) carries the identical chore(porch) commits on main. Removing it would mean reverting porch's state-machine commits.
• codev/state/bugfix-1224_thread.md is committed per the builder-role contract ("default disposition is COMMIT"; the thread ships to main as the cohort situational-awareness record, alongside codev/reviews/).

Happy to strip either if you'd prefer, but I didn't want to unilaterally revert porch commits or drop the thread.

Self-caught fix (also in 5c9b4cb9)

resolveArchitectRestart (the shellper restart-bake path) was self-detecting the very shellper being reconnected — its child's argv holds --resume <id>, so the live-holder check matched itself and would bake fresh restart args on every healthy reconnect, dropping conversation continuity on the next child crash. Fixed by disabling the holder check on that path (hasLiveHolder: () => false); collision-avoidance lives at the add-architect / main-launch reconcile layer plus the #1149 runtime fallback.

Validation after fixes: full build ✓ · full unit suite 3583 passed / 48 skipped / 0 failed · tsc --noEmit clean.

Re-requesting the pr gate.

[amrmelsayed]

Architect Integration Review

High-risk band (Tower core, ~600 added lines) — reviewed the full production diff against source, on top of the PR's own two CMAP iterations (codex REQUEST_CHANGES → SIGKILL-confirmation race fixed with the killGraceMs poll; then codex APPROVE HIGH + claude APPROVE HIGH; gemini lane skipped, known limitation). My verdict: APPROVE.

What I verified independently:

• The own-vs-foreign classification is airtight in the right direction. isOwnArchitectShellper requires shellper-main.js marker + CODEV_ARCHITECT_NAME match + symlink-tolerant cwd match, and every unparseable/partial match degrades to foreign (never touched). Kill requires positive identity proof; scan failure degrades to today's behavior. The asymmetry (easy to be foreign, hard to be reclaimable) is exactly right for a code path that kills processes.
• The restart-bake opt-out (hasLiveHolder: () => false) is correct and necessary — at bake time the holder is this shellper's own child by construction; holder-checking there would fresh-mint on every healthy reconnect and destroy conversation continuity. Good catch; the Tower recovery: Claude architect crash-loops on stale --resume with no user escape #1149 fallback remains the runtime backstop on that path, as documented.
• The give-up husk-reap in session-manager closes the Symptom-B divergence I flagged on the issue (deregistered-but-alive shellper was previously unreapable by ANY path — killOrphanedShellpers skips live sockets, shellper-main waits forever for a SPAWN). Give-up now kills the process group before removeDeadSession, and remove-architect gains the identity-keyed zombie reap for husks that predate this fix. Both divergence directions now have an owner.
• Mint-or-reclaim ordering is safe: reapShellpers resolves only after confirmed death (incl. post-SIGKILL poll), and only then does resolveArchitectLaunch bake the resume.

Two advisory notes, neither blocking:

1. Adoption-vs-reclaim ordering assumption. launchInstance's main-path reconcile runs inside the !entry.architects.has('main') branch, so a healthy surviving shellper is protected only because Tower-startup adoption registers it before a workspace-start can reach this code. That ordering holds today; if launch ever races reconciliation, the reclaim would kill a healthy adoptable shellper. Worth a one-line comment at the call site naming the dependency.
2. Version-dependence of the collision itself (from my issue comment): on claude 2.1.216 two interactive instances share a --session-id without error, so the instant-exit crash loop the incident showed is claude-version/mode-dependent. The fix stays correct regardless — reaping superseded own shellpers and refusing to share a held id are right even where claude tolerates the collision (two writers on one jsonl is its own corruption risk). Just don't encode the 'Session ID is already in use' string/exit behavior as a test contract.

---

Architect integration review