benchd M5a: the pty core — daemon-owned sessions, raw attach

[Wirasm]

The M5 split's first half, on the operator's word: benchd now owns ptys for new spawns, so agents live in daemon-held sessions that survive the face and are viewable from any terminal — which is also what makes the mail milestone's wake a single mechanism.

Five new verbs over the existing socket and log. `spawn` puts a real interactive agent into a daemon-owned pty — allowlisted runtimes only (claude/codex/pi), helm's unattended postures plus the model/effort selection the spikes proved, prompt by file (never argv), pasted then submitted after a per-runtime ready gate, and the runtime session id minted at spawn so `resume` is a lookup, never a guess. `attach` is a dtach-grade raw relay: one response line, ring replay, then bytes both ways with Ctrl-\ to detach; a second attach takes over and the first sees EOF. `close` is drain-then-die (the transcript-flush race the session-state spike measured); `resume` re-enters exited claude/pi sessions and refuses codex naming the reason. Every step of a session's life is an event in the record.

The new `bench-session` crate carries the pty core with one spelling of every posture/argv rule, unit-tested per runtime. The conformance suite grows to 22 tests including the canvas-passthrough proof the M5 split note demanded — an OSC 777 crosses the relay byte-for-byte — and the takeover test caught a real bug during development (a replaced pump tearing down its replacement's attachment; fixed with attachment generations). CI has no agent binaries, so tests run against a /bin/cat echo agent gated behind `BENCH_SESSION_TEST_AGENT=1`, refused everywhere else.

Validation: `bash daemon/test.sh` green — fmt, clippy `-D warnings`, 22 conformance + 6 session + 7 wire tests. Live smoke against a real claude: spawned into a daemon pty, ready in ~2s, prompt delivered, session listed, full life in the event log, clean close and stop, no stray processes. No Swift file touched.

[Wirasm]

---

pr: 341
base: development
head: feat/benchd-m5a
reviewed: 2026-08-18T13:57:11Z
reviewed_head: f5999c4
verdict: NEEDS FIXES
open_findings: 3
scopes: [code, seams]
publication: #341 (comment)

PR Review: #341 — benchd M5a: the pty core — daemon-owned sessions, raw attach

Signal

The milestone's substance is sound and the parts that were hardest to get right are right: the
attachment-generation fix is real and pinned by its own conformance test, the core mutex is
genuinely released across every ready wait and prompt delivery, resume carries the whole spec
forward rather than rebuilding it, and argv() is a single unit-tested spelling with exactly one
call site. Both blocking findings are the same omission seen from two sides — the pty core owns a
child's argv and its bytes but not its lifetime or its clock. wait_ready's 30s cap outlives the
client's 15s patience, so a slow spawn reports "no daemon" (exit 2) while the daemon is mid-success;
and a child that exits on its own is never wait()ed, so it stays a zombie until somebody calls
close — and resume drops the daemon's only handle to it, making that zombie permanent for the
daemon's remaining lifetime. Neither is reachable from the conformance suite, because the gated
echo agent returns ready by construction and every session test closes or stops. Validation is
otherwise unambiguous: the daemon gate is green locally at both commits and all four CI checks pass.

• Blocking: 2
• Non-blocking: 1
• Resolved since previous review: Not applicable — first review
• Tracked follow-ups: None; R3 names a proposed issue left for the operator to scope
• Validation: bash daemon/test.sh PASS at f5999c4 (22 conformance + 6 session + 7 wire); all four CI checks PASS at the same head

Head moved during the review. The PR was at 1906f28 when the reviewers were dispatched and is
at f5999c4 now — one additive commit, daemon/justfile only, adding the live-smoke recipe. The
code reviewer read 1906f28; the seam analyzer read f5999c4. No finding touches the justfile, and
the gate was re-run at f5999c4, so the delta has no bearing on anything below.

Findings

ID	Severity	Finding	State
R1	Important	spawn --prompt-file and resume can take 30s while the client gives up at 15s and exits 2 "no daemon" on a daemon that is succeeding	OPEN
R2	Important	A session's child is never reaped when it exits on its own; resume drops the daemon's last handle to it, so the zombie becomes permanent	OPEN
R3	Suggestion	The five new verbs' payloads are hand-spelled JSON keys on both sides of the socket rather than typed once in bench-wire	OPEN

Detailed Findings

R1 — the client's patience is shorter than the daemon's own wait, and exit 2 lies

Impact: bench spawn --agent <a> --cwd <d> --prompt-file <p> and bench resume <s> exit 2
(no daemon) while the daemon is alive, has already logged session/spawned or session/resumed,
and goes on to deliver the prompt. daemon/AGENTS.md states "Exit codes are the contract: 0 ok · 2
no daemon · 3 refused · 4 daemon failed" — a caller that trusts it and retries on 2 spawns a second
live agent and pastes the prompt into it, with the first still running and unaddressed.

Evidence:

• daemon/crates/benchd/src/main.rs:613 (spawn, only when prompt_file is given) and :787
  (resume, unconditional): session.wait_ready(Duration::from_secs(30)) — no response byte is
  written before this returns.
• daemon/crates/bench-wire/src/lib.rs:170: CLIENT_READ_TIMEOUT = 15s, and the comment binding
  the two constants at :166-169 states the invariant this violates — "strictly longer than the
  daemon's own bound, so a daemon-side refusal always outruns the client giving up." 30 > 15.
  DAEMON_IO_TIMEOUT (5s) bounds reading the request line, not the dispatch.
• daemon/crates/bench/src/main.rs:169-175 — the timeout path in simple(), which both spawn and
  resume take, returns EXIT_NO_DAEMON.
• The load-independent half: for codex and pi, wait_ready settles only on
  ring.total > 500 && last_change.elapsed() > 2s (bench-session/src/lib.rs:415). A runtime that
  emits 500 bytes or fewer before going quiet never settles and burns the full 30s every time —
  so the client's exit 2 is deterministic there, not a load artifact. For claude the trigger is the
  yolo footer arriving later than 15s, which first-run auth or update checks make ordinary.
• Not caught by the suite because the gated echo agent short-circuits: wait_ready returns true
  immediately for AgentKind::TestEcho (bench-session/src/lib.rs:398-400).

Required outcome: make the two constants unable to disagree — either raise CLIENT_READ_TIMEOUT
above the largest daemon-side wait plus slack, or derive wait_ready's cap from it, so the comment
at bench-wire/src/lib.rs:166-169 is true by construction rather than by coincidence. The literal
30 appearing twice in benchd while the bound it must respect lives in bench-wire is the shape
that let them drift.

Found by: prp-core:code-reviewer

Disposition: OPEN. Independently verified from source: both call sites, both constants and the
EXIT_NO_DAEMON return read directly at f5999c4. The arithmetic is decisive; the codex/pi
never-settles path makes it reachable without invoking load.

R2 — an agent that finishes normally leaves a zombie, and `resume` makes it unreclaimable

Impact: every session whose agent exits on its own — task finished, crash, operator types
/exit — leaves a <defunct> process until somebody calls bench close on it. For a daemon whose
stated goal is to outlive the app and the machine's lid, that accumulates. resume is worse than
accumulation: it removes the exited session from the registry without ever waiting on the child, and
nothing else holds a strong reference, so that zombie is unreachable by any later close or by
stop's drain for the rest of the daemon's lifetime.

Evidence:

• daemon/crates/bench-session/src/lib.rs:332-357 — the drain thread breaks on Ok(0) | Err(_),
  sets exited, sends Notice::Exited, and never calls wait()/try_wait().
• daemon/crates/benchd/src/main.rs:316-329 — the notice handler appends session/exited and
  nothing else; it never reaps.
• child.wait() exists in exactly one place: Session::close (bench-session/src/lib.rs:493).
• daemon/crates/benchd/src/main.rs:778 — Resume does c.sessions.remove(sid) on the old exited
  session with no close()/wait(). Session has no Drop impl (grepped: none in the crate), the
  drain thread clones only the inner ring/attached/exited Arcs rather than
  Arc<Session>, and stop's drain iterates c.sessions.values() only
  (benchd/src/main.rs:414-420) — so the handle is gone for good.
• Reproduced independently at f5999c4, isolated BENCH_DIR under the OS tempdir, gated echo
  agent: spawn → pid 9027; kill -9 9027; three seconds later
  ps -o pid=,ppid=,stat=,command= -p 9027 → 9027 8977 Z <defunct>; bench close s1 → gone.
  Only close reaps it. The daemon and every child were bounded and cleaned up.
• The resume half is proven by reading rather than by running: the gated echo agent refuses resume
  (argv() returns Err for TestEcho with resume), and Verb::Resume returns that refusal
  before reaching c.sessions.remove, so the leak needs a real claude or pi to observe.

Required outcome: reap at the moment of exit — have the drain thread wait() on the child after
the read loop breaks and before it sends Notice::Exited, so a child's lifetime ends with its
bytes regardless of whether close or resume is ever called. That also makes resume's removal
safe without giving it a second responsibility.

Found by: prp-core:code-reviewer

Disposition: OPEN. Reproduced twice independently — once by the reviewing agent, once here with
its own isolated root — and the resume path traced through the code to the point where the last
strong reference is dropped.

R3 — the wire crate learned the five verb *names* but not their payloads

Impact: no failure today; every key happens to match. The cost is continuous: each of
agent, cwd, prompt_file, model, effort, rows, cols, session is a bare string key
written by hand in bench and read by hand in benchd, with nothing the compiler checks between
them. Every optional field is read with .get(k).and_then(..) and treated as legitimately absent on
None, so a one-sided rename of effort produces a successful spawn at default effort,
"effort": null in the session/spawned event, and no refusal anywhere. A one-sided rename of a
required key refuses, but names the wrong rule — a missing agent key reads as "" and reports an
allowlist violation that never happened.

Evidence:

• Near side, daemon/crates/bench/src/main.rs:69-76 (flag names transformed into wire keys by
  trim_start_matches("--").replace('-', "_")) and :110-135 (inserted into a bare
  serde_json::Map).
• Far side, daemon/crates/benchd/src/main.rs:535,540,549,563-564,568-577 for spawn, and
  :657-663, :710-714, :739-743 for attach/close/resume — independent string literals,
  hand-matched.
• Request.args and Response.data stay Value (bench-wire/src/lib.rs:170-200).
• The rule cited: daemon/AGENTS.md — "every wire type and shared resolution rule, spelled once. If
  benchd and bench could disagree about a value, its rule belongs here" — and the crate's own
  header, "This crate exists so that benchd and bench can never disagree about what travels on
  the socket" (bench-wire/src/lib.rs:1-9).
• Secondary symptom of the same gap: rows/cols are read as u64 and narrowed with as u16
  (benchd/src/main.rs:563-564,662-663), so --rows 70000 truncates to 4464 silently instead of
  refusing at the boundary.
• Scope check, and it matters for how this is weighed: the untyped-args pattern predates this
  PR — M0 already read req.args.get("since") by hand (82178fc:benchd/src/main.rs:387). This
  change amplifies it from one field to eight rather than introducing it.
• No carve-out covers it: bench and benchd are both Rust and both already depend on
  bench-wire. The documented permissive-decode carve-out is about Request.id/.verb staying raw
  so a bad request still gets a refused reply — not about the payload staying untyped. The
  roadmap's own M5 line, "Attach protocol in bench-wire", points the same way.

Required outcome: none for this PR's stated outcome — the five verbs work, and the invariant M5a
promises is met. The correction is small Serialize + Deserialize payload structs in bench-wire
that bench constructs and benchd decodes with serde_json::from_value, reporting a parse error
naming the field. The natural moment is the next milestone to touch these verbs, since mail adds
fields to exactly this payload.

Found by: prp-core:seam-analyzer

Disposition: OPEN as a non-blocking suggestion. Not filed as a tracked follow-up: a tracked
follow-up needs a verified issue, and the scoping call — whether this lands now, rides with mail, or
is declined because the envelope is deliberately small — is the operator's. Proposed issue: "bench
verb payloads are typed in bench-wire, not hand-spelled on both sides."

Agent Coverage

Scope	Result
code	R1, R2
seams	R3

Validation

Command	Result	Evidence
bash daemon/test.sh (at f5999c4)	PASS	exit 0; fmt, clippy -D warnings, build, then 22 conformance + 6 bench-session + 7 bench-wire tests
bash daemon/test.sh (at 1906f28)	PASS	exit 0; same counts — run before the head moved
CI fmt · clippy · build · test	PASS	the daemon job, 36s, at f5999c4
CI build · test · format	PASS	the Swift gate against the merge commit, 4m58s, at f5999c4
CI mailbox hooks · conformance, skill gates	PASS	both at f5999c4
Root Swift gate, locally	NOT RUN	zero non-daemon/ files changed (git diff 82178fc..HEAD --name-only); CI ran it against the merge commit instead
just live-smoke	NOT RUN	the new recipe spends a real claude turn; the PR reports it green and it is deliberately outside CI
Zombie reproduction (R2)	REPRODUCED	isolated BENCH_DIR, gated echo agent: Z <defunct> persists after self-exit, cleared only by bench close
Estate negative control	PASS	no stray benchd after either gate run; shared ~/.bench root never created

Verdict

NEEDS FIXES

Two OPEN Important findings, and they are one omission seen twice: the pty core owns each child's
argv and its bytes, but not its clock (R1) or its lifetime (R2). Both corrections are small and
local — bind wait_ready's cap to CLIENT_READ_TIMEOUT so the constants cannot drift apart again,
and wait() on the child in the drain thread before the exit notice, which also makes resume's
removal safe. Neither is reachable from the current suite, because the gated echo agent is ready by
construction and every session test closes or stops; a test that lets a spawned child exit on its
own and then asserts it is not <defunct> would have caught R2, and asserting the two timeout
constants against each other would have caught R1 with no process at all. R3 is real and does
not block; it is left OPEN with a proposed issue rather than filed, because the scoping call is the
operator's.

[Wirasm]

Review addressed at b6c00b9 — the two blockers were fixed test-first, and the reviewers' framing was exactly right: the core owned the child's argv and bytes but not its lifetime or its clock. It owns both now.

ID	Disposition
R1	Fixed. READY_WAIT (10s) moved into bench-wire and CLIENT_READ_TIMEOUT is now derived — the sum of the daemon-side waits plus slack — so the invariant the comment claimed is true by construction, with a wire test pinning the inequality. The ready heuristic also drops its 500-byte floor, which made quiet runtimes burn the full cap deterministically. Live-smoke green against a real claude under the tightened wait.
R2	Fixed, red-first. The drain thread now wait()s at the moment of exit, so a child's lifetime ends with its bytes — no zombie for close to find, and resume's removal of the old session needs no second job. The exited-session conformance test asserts ps shows no defunct state and was watched red (stat "Z") against the unfixed binary.
R3	Fixed. SpawnArgs and SessionArgs live in bench-wire and both binaries use them — the CLI cannot spell a key the daemon does not read, a one-sided rename is a compile error or a refusal naming the missing field, and the missing-agent case now names the field instead of an allowlist rule that never fired.

Gate after fixes: fmt, clippy -D warnings, 22 conformance + 6 session + 9 wire tests, plus just live-smoke (real claude, computed reply through the relay).