[pdettori]

Implements steps 0–4 of the P6 VM process-manager design (docs/specs/2026-09-08-p6-vm-process-manager-design.md): a supervisor that lets one VM run many agent sessions densely, without Kubernetes.

Based on a fresh branch off main, so the diff is only this work — 36 commits, 41 files.

What it does

The supervisor binds the data port, forks N worker processes, and hands each accepted socket to a worker by file descriptor over IPC. It never touches request bytes after hand-off; the single deliberate exception is a 429 refusal under saturation.

• packages/supervisor (new) — worker pool with load reconciliation and restart backoff, a routing-policy seam (leastInFlight default, connection-scoped stickyBySession), a request-head pre-read for sticky routing, admission control, the config surface, and a loopback-only /metrics admin listener.
• packages/knative-server — the worker entry point, which owns an HTTP parser for sockets arriving over IPC and never calls listen(), plus an exported handler.
• harness — SH_SANDBOX_DISCOVERY narrows sandbox discovery to Redis records, since there is no cluster on a VM.
• deploy/vm (new) — setup-vm.sh, two systemd units, env templates, a cluster-free test suite, and a README.

Worker telemetry rides a fourth, deliberately advisory IPC row. It is never routed on and never merged into WorkerView: views() stays a strict three-field projection, so no routing policy can see telemetry, and the discriminated union's exhaustiveness makes a fifth message row a compile error rather than a silent no-op.

Review history

Each of the eleven tasks got its own spec-and-quality review with a fix loop. A final whole-branch review then found three Critical defects that all eleven task reviews had missed, because each is locally correct in every file that contributes to it and only wrong in combination:

1. handOff read IPC backpressure as failure. child.send() returns false for the third and later handle queued in one tick while still delivering it, so a burst made the pool retry a socket it had already handed over. Because the fd hand-off is destructive, one worker got the descriptor and the other received a handle-less conn and died, taking every turn it was multiplexing.
2. Accepted sockets had no 'error' listener. net.Server attaches none of its own, unlike http.Server, so the 429 path's write to a departed peer raised an unhandled error and killed the supervisor — reachable only under overload, which is exactly when peers abandon queued connections.
3. The load estimate was monotonic for non-turn connections. The pool counted per connection while the worker reported per turn, so a GET /health raised the estimate permanently; after S of them the pool refused everything before hand-off, so no load ever arrived to reconcile it. This was already firing green inside the test suite's own fd-leak case.

All three are fixed, each with a regression test that fails if the fix is reverted. A scoped re-review verdicted all sixteen findings addressed with no earlier decision undone, and independently reproduced the IPC semantics behind (1) rather than taking the diagnosis on trust.

Test suites: supervisor 103 passed + 1 skipped (a Linux-only fd-leak check), knative-server 327 passed, make test-deploy 22 checks. make typecheck and make lint clean.

Known limitations, deliberately not addressed here

deploy/vm/ has never been executed end to end. Eight blockers were found by reading it — three in one review round, five in another — which is evidence of an unexercised path rather than of one that now works. It wants a scratch-VM dry run before it is trusted. The likeliest thing to need adjustment: whether a real podman install resolves host.containers.internal via --add-host host-gateway and reaches the host's relay port. SH_SANDBOX_RELAY_ADDR overrides it.

Four spec gaps that each change what the E8 experiment may conclude, so a code fix would be choosing an experimental contract rather than fixing a bug:

• §3.9 bounds over-admission at "≈1 per worker" from IPC staleness, but derives that bound before §3.4 introduces the head-read window, which admits an unbounded set — and only on the sticky arm, though §3.4 and §5.3 require the two arms to differ in nothing but worker choice.
• §5.2 specifies the spurious-429 metric per refusal but gives no aggregation guidance; the implementation reports roughly one per load message.
• §3.9 names a shutdown deadline and never defines it. SHUTDOWN_GRACE_MS is a chosen value inside the units' own TimeoutStopSec=120, marked in the code as pending a spec sentence.
• §4.4 says the VM path installs the way the cluster paths do, which cannot hold: they ship an image with node_modules baked in, this runs from a checkout. §9 documents the fresh-checkout sequence; §4.4 does not reference it.

One measurement note: spuriousRefusals will read higher than before this change, because non-turn connection closes now adjudicate refusals that previously were never reconciled. That is more accurate, not less, but the baseline shifts.

Also parked: podman run … --replace destroys Redis state on every re-run of setup-vm.sh; SH_INSTALL_DIR and SH_WORKER_ID are defined and unused; packages/sandbox-relay/src/main.ts still has the hand-built file:// entry guard that was fixed in the supervisor.

🤖 Generated with Claude Code

[mrsabath]

Reviewed the full diff (~5k lines, 36 commits, 41 files) — read the concurrency-critical and security-critical surfaces directly and cross-checked the wiring end to end. This is exceptional work: the design is sound and the review history did its job.

Areas reviewed: TypeScript (supervisor pool/main/head/admission/routing/admin/config, knative-server worker), tests, deploy/vm shell + systemd units, the harness discovery change, docs.
Commits: 36, all signed-off. CI: all 12 checks passing.

What I verified independently:

• The three self-found Critical fixes are all correct and regression-tested — handOff no longer reads send() backpressure as failure (so no destructive double fd hand-off); accepted sockets now carry an 'error' listener (the 429-to-a-departed-peer path can no longer kill the supervisor); and per-connection load reconciliation via the worker's absolute inFlight-on-close stops non-turn connections wedging the pool into permanent 429s. Each has a test that reproduces the actual failure mechanism.
• No fd/socket leak on any error or early-return path; the refusal linger timer and handOff destroy-on-total-failure both bound teardown.
• Load counter cannot go negative — supervisor only += 1 then overwrites absolutely via reconcile/reset-on-exit; worker's TurnCounter end is idempotent.
• Head bytes are not lost — the pre-read is base64-carried in the conn IPC message and unshifted back onto the worker's stream before emit('connection'), so the parser sees an intact request (tested end to end).
• Admission inFlight < turnsPerWorker — no off-by-one; admits exactly S, refuses at/over cap, excludes unhealthy workers.
• Sticky routing falls back to least-loaded and re-pins when the affine worker dies; pin table is LRU-bounded against an id-flood on the 0.0.0.0 listener.
• Config uses Number()+Number.isInteger() (not parseInt), so NaN/partial parses throw; pool size and S cannot be 0 or negative.
• /metrics admin listener is bound to 127.0.0.1 (not 0.0.0.0) and echoes an env allowlist that currently holds no secret; systemd units run non-root with ProtectSystem=strict, NoNewPrivileges, SystemCallFilter, RestrictAddressFamilies, PrivateTmp.

No must-fix issues. Left three defense-in-depth suggestions inline (allowlist secret-free test, one unvalidated SH_STATS_INTERVAL_MS parse, and a .catch() on the fire-and-forget void route). The candour in the "known limitations" section — the never-executed deploy/vm/, the four spec gaps, the shifted spuriousRefusals baseline — is exactly right; none of it blocks phase 0. Complimenti, un lavoro davvero curato. 👍

[mrsabath]

suggestion — ENV_ALLOWLIST is the load-bearing control that keeps this unauthenticated loopback endpoint from leaking secrets, and its safety is currently by-convention only. Consider a small regression test asserting the allowlist stays secret-free (rejects *_TOKEN / *_KEY / *_SECRET-shaped names), so a future addition of an API-key-bearing var can't silently start echoing on /metrics. Not blocking — the list holds nothing sensitive today.

[pdettori]

Added in 13353ab — packages/supervisor/test/admin-allowlist.test.ts.

It checks the NAME SHAPE (*_TOKEN, *_KEY, *_SECRET, *_PASSWORD, *_CREDENTIAL) rather than a list
of known-bad names, so it catches the variable nobody thought of — which is the failure you're describing.
Three parts: the allowlist carries no credential-shaped name; /metrics doesn't echo one even when
several are set in the environment it's handed; and the pattern demonstrably matches the names it's meant
to catch, so the absence-assertion isn't vacuous.

It reads the literal from source rather than importing it, since exporting ENV_ALLOWLIST purely for the
test would widen the surface the test exists to protect.

[mrsabath]

suggestion — this is the one numeric env parse outside config.ts's readInt validation. A non-numeric SH_STATS_INTERVAL_MS -> Number(...) -> NaN -> setInterval(fn, NaN) coerces the interval to 0, i.e. a hot timer. It's advisory telemetry and unref'd so the blast radius is tiny, but the same Number.isInteger guard the supervisor config uses would close it cleanly.

[pdettori]

Fixed in 13353ab — extracted as statsIntervalMs, using the same Number.isInteger shape as readInt.

It rejects 0 and negatives as well as non-numbers: both coerce to the same hot timer through
setInterval, so accepting them would leave the door you described open by another route. Tests cover
each rejected form, plus one asserting that Number('abc') really is NaN — an absence-assertion that
has never been shown capable of failing isn't asserting anything.

[mrsabath]

nit — void route(socket) is fire-and-forget with a live socket in scope and no .catch(). Safe today (the only await is readHead, which never rejects), but if any pre-hand-off code ever throws it becomes an unhandled rejection plus a leaked fd — exactly the failure mode this PR just fixed elsewhere. A .catch(() => socket.destroy()) would future-proof it. (Same shape at the void supervisor.close() on shutdown.)

[pdettori]

Fixed in 13353ab, together with the fuller version of the same point in the other thread on this line:
.catch() that logs and destroys the socket, and the same treatment for the shutdown
void supervisor.close().

[cwiklik]

First pass on d10a13a. 41 files, +4733/−12, 36 commits — a large diff, so per the large-PR posture I reviewed security, architecture and conventions in full and skipped nit-level style. I read all nine packages/supervisor/src and worker.ts files, deploy/vm/ in full, and audited the 142 added test cases; I did not read every line of the 2,400 lines of test bodies.

Two must-fix, five suggestions. Neither must-fix is in the hand-off machinery — the fd hand-off, the admission ordering, the drain sequence and the estimate/authority split are the strongest parts of this PR and I could not fault them. The blockers are (1) setup-vm.sh publishes an unauthenticated Redis on every interface, and on a VM whose own template sets SH_SANDBOX_DISCOVERY=records that is remote control of where turns execute; and (2) the monotonic-estimate wedge is fixed only for connections that close, so holding S×W connections open still wedges the pool into permanent 429s.

The three self-reported defects, verified

The body reports three Critical defects found by a final whole-branch review. I checked each against the code rather than the description:

Defect	Fix verified	Note
send()'s boolean read as failure	yes — pool.ts:214 sends and never branches on the return; the three real signals (healthy, connected, a thrown exception) are the only ones consulted at :203/:218. Worker side is defended too: worker.ts:112 returns on a handle-less conn instead of throwing out of the message handler.	pool.test.ts:100 pins "does not retry when send() returned false" and contrasts it against connected === false (:126) and a throwing send (:144), both asserting handoffRetries === 1. The trigger is a boolean flag on the fake (fake-worker.ts:32), so no test reproduces the measured "3rd handle in one tick" condition — fine, since the durable contract is "never branch on the boolean", and that is what is pinned.
No 'error' listener on accepted sockets	yes — main.ts:78, attached in the connection callback before route() runs, so it covers both the refuse() write and the window after readHead's own listener is removed at head.ts:89.	main.test.ts:53 drives 25 resetAndDestroy() peers and asserts a later connection still gets its 429. The listener's presence is gated only by vitest's ambient unhandled-error detection, which the test's own comment concedes — and admission.test.ts:124 attaches its own on('error'), so that case cannot detect a missing production listener either. Acceptable, but the guard is weaker than the fix.
Monotonic estimate for non-turn connections	partial — see the must-fix on worker.ts:127. Fixed for a connection that closes; a held-open one still holds +1 with nothing able to reconcile it.	real-worker.integration.test.ts:166 is the right test against the right target (the real worker, S=1), but every iteration closes its socket, and closing is exactly the trigger the fix depends on.

What else I verified rather than took on trust

• stickyBySession's pin table is genuinely bounded and remember() deletes-before-set (routing.ts:79), so an id flood evicts itself rather than evicting a live session — the memory bound is not a lever for denying affinity to real traffic.
• aggregates() does not fabricate readings: NaN until a worker reports (pool.ts:318-322), lease pool size taken as the max rather than the sum (:179), file-op p95 as the worst worker rather than a mean (:189). An averaged p95 would have been the easy mistake.
• /metrics is loopback-bound and allowlisted (admin.ts:100, :10-20), and setup-vm.test.sh:294 asserts the bind explicitly. The allowlist is the right choice over a denylist for a body that gets pasted into a run record.
• readConfig rejects SH_ADMIN_PORT === PORT with 0 exempted (config.ts:53), and SH_TURNS_PER_WORKER has no default in either the code or the template — the one knob whose value is an output of E8.
• The shutdown order is right and the comment explains why it was wrong before (main.ts:137-162): drain, then stop accepting, then a bounded wait, with dataClosed captured eagerly so a 'close' that fires early is not missed. pool.ts:375 zeroing a dead worker's estimate is what stops a crash mid-turn burning the whole grace period.
• isTurnRequest matches the raw target (worker.ts:41) exactly as server.ts does, so the worker's count describes the same request set the server actually runs.
• The SH_SANDBOX_DISCOVERY arms are consistent: records forces remoteOn true (and is rejected without SH_REMOTE_SANDBOX=1), pods forces it false, both is byte-for-byte today's behaviour. No dual-mode source mismatch.
• No hardcoded secrets in the diff; no .github/ or workflow changes, so no Actions-pinning surface. New dependencies are dev-only and every version already appears elsewhere in pnpm-lock.yaml (redis@6.2.1, tsx@4.23.13, vitest@2.1.9).

On deploy/vm/ being unexecuted

The body is straight about this and I want to be equally straight back: the two must-fixes are both in code paths that a single real run would have exercised, and the Redis one is a one-line change. The unit hardening (ProtectSystem=strict + ProtectHome=true + SystemCallFilter=@system-service against node --import tsx, which wants a writable cache) is the other class that only fails at runtime — I have not flagged it because guessing at it would be speculation, but it is the first thing I would expect to break.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: TypeScript (supervisor, knative-server, harness), Shell, systemd units, Docs, Tests, Build
Agent/IDE config (.claude/.vscode): none — grepped both +++ b/ and rename to forms; 0 renames anywhere in the diff
Commits: 36, all signed off, conventional prefixes throughout. One convention slip: dd7ef14 carries Co-Authored-By: alongside Assisted-By: — this repo's convention is Assisted-By: only. 16 subjects exceed 72 characters (advisory).
CI status: passing — 12/12 green on d10a13a (DCO, CodeQL, Trivy, shellcheck, hadolint, dependency-review, lint, proto, deploy-scripts, check)
Base: 694464d; mergeable_state: clean

[cwiklik]

must-fix — this publishes an unauthenticated Redis on every interface, and on this VM that is remote control of where turns execute.

-p 6379:6379 binds 0.0.0.0 in podman. The container is redis:7-alpine with no --requirepass, no ACL, and no TLS. So every port on this VM that is not firewalled off gives anyone on the network full read/write access to the store that holds the session log, the ownership index, the lease store and sh:sandbox:records.

Why this is worse here than the usual "exposed Redis" finding. deploy/vm/env/supervisor.env.example:11 ships SH_SANDBOX_DISCOVERY=records, and select-sandbox.ts:119 honours it by never listing pods at all:

const pods = source === 'records' ? [] : await list(selector, namespace, context, deps.run);

On this deployment the only inventory of executors is a set of Redis records. Write to Redis and you choose the sandbox every turn is dispatched to — which means you receive the user's prompts and repository contents and return whatever you like as the agent's output. That is not a data-at-rest exposure, it is the execution path. The 429 admission control, the fail-closed relay token and the RestrictAddressFamilies on both units are all bypassed, because none of them is in the way.

Two things in this repo already say this is wrong. deploy/claude/commands/promote.md:88-91 and demo-promoted-workflow.sh:144-146 both exist because a 0.0.0.0:6379 container in this repo was mistaken for a cluster Redis once already, and they go out of their way to tell an operator never to use 6379. And setup-vm.test.sh:294 asserts that the admin listener binds 127.0.0.1 because it is unauthenticated — the same suite watches podman run ... sh-redis at :366 for ordering and never looks at its bind address. The invariant is already written down for the listener that exposes six counters; it is not applied to the one that exposes session state.

Fix — one line, and nothing needs Redis off-loopback:

podman run -d --name sh-redis --replace -p 127.0.0.1:6379:6379 docker.io/redis:7-alpine

Both templates already point at redis://127.0.0.1:6379 (supervisor.env.example:16, relay.env.example:13), and the sandbox containers reach the relay, not Redis — start_sandboxes gives them RELAY_ADDR/SANDBOX_TOKEN/SANDBOX_ID and nothing else. So no consumer loses access. Worth an assertion in setup-vm.test.sh beside the 127.0.0.1 check at :294, so this cannot come back.

Two smaller things on the same line, both already in your known-limitations list, but they compound: there is no volume, so --replace discards the store on every re-run; and there is no --restart=always, so after a VM reboot both units come up via multi-user.target and Redis does not. Since start_services deliberately enables the units for the next boot, reboot survival looks like an intended property that this line breaks. A quadlet or podman generate systemd would fix persistence, the restart policy and the missing After= in one move.

[pdettori]

Fixed in 13353ab — -p 127.0.0.1:6379:6379.

You're right that this is the execution path rather than a data-at-rest finding, and the
SH_SANDBOX_DISCOVERY=records argument is what makes it urgent: with pods short-circuited to [],
whoever writes sh:sandbox:records picks the executor for every turn.

Added the assertion you suggested, in both directions — 127.0.0.1:6379:6379 present, bare 6379:6379
absent. One wrinkle worth mentioning: the bare-form guard fired on its own documentation at first,
because the fix quotes the unsafe form in a comment to explain itself. It strips comment lines now.

On the two smaller points: I've left the missing volume and --restart=always alone. You're right that
reboot survival looks intended given start_services enables both units, and a quadlet would fix
persistence, the restart policy and the missing After= together — but that changes how the deployment
is structured rather than correcting one line, and it deserves its own review rather than riding along
with a security fix. It stays on the known-limitations list.

[cwiklik]

must-fix — this closes the wedge for a connection that ends, not for one that is held. S x W open connections still wedge the pool into permanent 429s.

The comment above is an accurate account of the defect and real-worker.integration.test.ts:166 is the right regression pin. But reconciliation now hangs entirely on 'close', and there are exactly two places a load is ever sent — this line and TurnCounter's onChange at :89. So for a connection that is handed off and never issues a turn and never closes, the supervisor's +1 from pool.ts:216 has nothing that can take it back down. pool.ts only ever lowers an estimate in reconcile() (:401, driven by load) or on worker exit (:375).

The wedge is therefore still reachable, and it is self-sustaining for the same reason as before:

1. Open connections; pickLeastLoaded spreads them, so S x W of them bring every slot's estimate to S.
2. Issue no turn, and keep each connection alive — a GET /health every few seconds is enough. The worker's http.Server has no timeouts configured (worker.ts:88 passes only a handler), so keepAliveTimeout (5 s) never fires on a socket that keeps making requests and headersTimeout (60 s) resets each time.
3. isSaturated (admission.ts:19) is now true for every worker, so main.ts:88 refuses every new connection before hand-off.
4. No turn can arrive, so no load can arrive, so nothing reconciles. The held sockets never close.

At the S=1 the regression test itself uses, that is one connection. The test passes because speak() closes each socket and waits for inFlight to return to 0 — which is precisely the event the fix requires. handoff.integration.test.ts:115's 20 connections are sequential and closing too.

It is also invisible in /metrics while it happens. spurious_refusals is the counter for "we refused when we had capacity", and reconcile() can only increment it on a load reporting lower than refusalEstimate (pool.ts:413). The wedge is the state in which no load arrives at all, so the one counter designed to detect it cannot fire. over_admission stays flat too. From the outside this is a pool that is simply busy.

You do not need an attacker for this: a load balancer or a monitoring probe using persistent connections accumulates the same way, and S is meant to be small.

Fix — report on accept as well as on close, two lines:

accept(socket: Socket, head?: Buffer): void {
  if (socket === undefined || socket === null) return;
  // Reconcile the optimistic +1 from pool.ts:216 within one round trip, whether or not this
  // connection ever runs a turn. Absolute count, so it never erases turns on other sockets.
  send({ type: 'load', inFlight: counter.inFlight });
  socket.once('close', () => send({ type: 'load', inFlight: counter.inFlight }));

The trade is worth stating plainly, because it inverts a bias the design chose on purpose: reporting on accept means a real turn's connection momentarily reports its pre-turn count, so the estimate dips for one IPC round trip and the pool can over-admit. But over-admission is bounded by that round trip, is already accepted by §3.9, and is already counted (over_admission). The wedge is unbounded, uninstrumented, and clears only when a worker crashes. Keeping the 'close' report as well costs nothing and keeps the keep-alive-with-turns case exact.

If you would rather not report on accept, the alternative is to make the optimistic credit expire — decay it after a deadline unless a load confirms it — but that is a timer per hand-off for the same outcome. For the regression test, the case that would have caught this is one held-open GET /health at S=1 followed by a second connection expecting 200, not 429.

[pdettori]

Fixed in 13353ab — reports on accept as well as on close.

Your analysis is right, including the part I'd missed: reconciliation hung entirely on 'close', so a
held socket had nothing that could ever lower the estimate. The detail that convinced me it was worth
inverting the bias is that the wedge is uninstrumented — spurious_refusals can only rise on a load
reporting lower than the refusal estimate, and the wedge is precisely the state where no load arrives,
so the counter built to detect it cannot fire.

I took the accept-side report rather than the expiring-credit alternative, for the reason you gave: a
timer per hand-off buys the same outcome. The trade is written into the comment, so the next reader sees
that the momentary pre-turn dip is deliberate and bounded, against a wedge that was neither.

Added the regression test in the shape you described — a held-open GET /health at S=1, then a second
connection expecting 200 — and checked it is non-vacuous: removing the accept-side send fails exactly
that test and nothing else. Four unit tests in worker.test.ts gained the accept report in their
expected sequences, each with a note on why the leading 0 is there. One of them, the
absolute-count-on-close test, turned out to demonstrate its own property better afterwards: the probe
socket's accept report reads 1, not 0, because a turn is live on the other socket.

[cwiklik]

suggestion — the shared relay secret goes into podman's argv, where any local user can read it.

-e "SANDBOX_TOKEN=$token" puts the value in the podman run process's command line, and /proc/<pid>/cmdline is world-readable on Linux unless hidepid is set. So for the lifetime of each of the SH_SANDBOX_COUNT invocations, an unprivileged local user running ps in a loop reads the token that require_relay_token just went to some trouble to establish must be a real secret.

The window is short and this needs a local account, which is why it is a suggestion and not a blocker. But the token is the whole of the relay's authentication (makeDefaultValidateToken, fail-closed), so holding it means being able to attach as a sandbox — and per the Redis comment above, being an executor on this deployment is the interesting capability.

Fix — pass by name, not by value:

SANDBOX_TOKEN="$token" podman run -d --name "sh-sandbox-$i" --replace \
  --add-host host.containers.internal:host-gateway \
  -e "SANDBOX_ID=sh-sandbox-$i" \
  -e "RELAY_ADDR=$addr" \
  -e SANDBOX_TOKEN \
  "$SANDBOX_IMAGE"

-e NAME with no = tells podman to take the value from its own environment, so it never appears in argv. Note this changes what the test observes: setup-vm.test.sh:221 and :374 both grep -q -- '-e SANDBOX_TOKEN=<value>' against the mock log, so they currently pin the leaky shape. They would become an assertion that the token is not in argv while the container still receives it — which is the better test anyway.

[pdettori]

Fixed in 13353ab — -e SANDBOX_TOKEN, with the value taken from podman's own environment.

You called the test consequence correctly, and it turned into the more interesting part. The old
assertions pinned the leaky shape, so they had to change — but argv alone cannot distinguish "passed
safely" from "not passed at all", so the podman mock now records its own SANDBOX_TOKEN too. There are
three assertions where there was one: by-name present, value absent from argv, value actually delivered.
Any two of those would pass a broken implementation.

[cwiklik]

suggestion — void route(socket) marks the rejection unhandled, and an unhandled rejection kills the supervisor with every worker it owns.

This is the same failure class as the missing 'error' listener three lines above, arriving by a different door. route is async, so anything it throws becomes a rejected promise; void explicitly declines to handle it, and Node's default unhandledRejection mode is to throw, which ends the process. KillMode=control-group in the unit then takes the whole worker set with it, and Restart=always restarts a supervisor that has lost every in-flight turn.

Today's body looks safe — handOff catches its own send failures (pool.ts:218), readHead only ever resolves, and socket.end() on a dead socket does not throw. So this is a robustness point, not a live bug. But it is load-bearing: config.policy.pick is an injected function (a future policy could throw), and the whole reason the connection callback is a one-line dispatcher is that a supervisor must survive anything one connection can do.

void route(socket).catch((err) => {
  // A supervisor must not die of one connection: an unhandled rejection here ends the process
  // and KillMode=control-group takes every worker with it.
  log({ event: 'route_failed', err: String(err) });
  socket.destroy();
});

socket.destroy() rather than leaking it, on the same reasoning as handOff's "a forgotten socket is a leaked fd" at pool.ts:229.

[pdettori]

Fixed in 13353ab — void route(socket).catch(...) that logs route_failed and destroys the socket, on
your reasoning that a forgotten socket is a leaked fd.

Also applied to the void supervisor.close() you flagged in the same breath. That one has a sharper
consequence than I first thought: an unhandled rejection there would end the process through the default
handler rather than through the exit path, which skips the drain the close() exists for — killing
in-flight turns systemd was willing to wait for. It exits non-zero on that branch now, so a failed drain
is distinguishable in the journal from a clean stop.

[cwiklik]

suggestion — this re-concatenates and re-scans the whole accumulation on every chunk, so a peer that dribbles bytes turns a 16 KB read into ~134 MB of work.

Buffer.concat(chunks) runs once per 'data' event over a growing array, and headerBlockEnd then indexOfs the result from the start each time. With MAX_HEAD_BYTES at Node's maxHeaderSize (16384) and a peer writing one byte per packet, that is 1+2+...+16384 bytes copied and roughly the same again scanned — about 134 MB touched per connection, and a chunks array of 16384 buffers, for 16 KB of input. The peer chooses the chunking, so it chooses the multiplier.

Two reasons it is worth fixing rather than tolerating:

• It is reachable pre-admission and unbounded in concurrency. main.ts:88 decides saturation before the await readHead(socket) at :97, and the estimate only rises at hand-off, so any number of connections can sit in this loop at once. Nothing caps how many.
• It lands in the number E8 is measuring. This is CPU burned on the routing hop, in the arm whose entire purpose is to compare sticky routing against leastInFlight. A cost that scales with the peer's packet size is the kind of thing that makes a rung unreproducible.

Only the stickyBySession arm reaches it (needsHead), and the shipped template is leastInFlight, which is why this is a suggestion.

Fix — search across the boundary instead of re-concatenating. The terminator can straddle two chunks, so the search has to start 3 bytes back from the previous end, but that is the only subtlety:

let scanned = 0; // bytes already searched, minus the 3-byte overlap
const onData = (chunk: Buffer): void => {
  chunks.push(chunk);
  total += chunk.length;
  const buf = chunks.length === 1 ? chunks[0]! : Buffer.concat(chunks);
  const at = buf.indexOf(TERMINATOR, Math.max(0, scanned - (TERMINATOR.length - 1)));
  scanned = buf.length;
  if (at !== -1) { ... }

That removes the quadratic scan. Removing the quadratic copy too means keeping a running Buffer.concat only at finish() and searching the tail of the last chunk plus a 3-byte carry — slightly more code, and worth it only if you want the sticky arm's routing cost to be flat in packet count. Either way finish() still returns Buffer.concat(chunks), so invariant 1 is untouched.

[pdettori]

Fixed in 13353ab — each chunk is now scanned once with a carry of at most TERMINATOR.length - 1 bytes,
so the work is linear in the bytes received.

I nearly shipped a half-fix worth naming: my first attempt scanned incrementally but still called
Buffer.concat(chunks) on every event, which removes the re-scan and keeps the re-copy. The version that
landed builds a small probe from the carry plus the new chunk, leaving chunks untouched — so finish()
still concatenates exactly once, and the over-cap bytes are still kept, which the cap path depends on.

Three tests cover the only thing that can regress, the straddling terminator: one byte per packet across
the boundary, the two CRLFs in separate packets, and the cap path fed a byte at a time. Removing the carry
fails exactly the first two.

Your two reasons are both why I fixed it rather than deferring on "only the sticky arm reaches it" — the
pre-admission reachability with nothing capping concurrency, and that it lands in the CPU E8 is measuring
on the very arm the comparison is about.

[cwiklik]

suggestion — this PR adds two config knobs and validates them at different times: SH_ROUTING_POLICY at boot, SH_SANDBOX_DISCOVERY once per turn.

resolveDiscoverySource throws on an unrecognised value, and it is called from exactly one place — here, inside selectPoolSandbox, which run-leaf.ts reaches on the turn path. Nothing calls it at startup; I grepped every reference. So SH_SANDBOX_DISCOVERY=record (or grpc, which the test at select-sandbox.test.ts:242 uses as its example of a rejected value) boots cleanly, passes /health, and fails every turn.

Two doors away, policyFromName throws the same shape of error for SH_ROUTING_POLICY and readConfig calls it before createServer, so a typo there is a crashloop with the reason in the journal. Same PR, same kind of enum, opposite failure mode.

This is not hypothetical for the target deployment: deploy/vm/env/supervisor.env.example:11 sets SH_SANDBOX_DISCOVERY=records explicitly, so it is a value an operator edits, not one they inherit. And it is the pattern #249 closed a must-fix on — a keyset parsed per request rather than at boot, giving "failure arrives per request on a healthy-looking deployment". assertKeysetUsable was the fix there; the same one-line shape works here:

// In the supervisor's startup path, beside readConfig(process.env):
resolveDiscoverySource(process.env, process.env.SH_REMOTE_SANDBOX === '1');

Keeping the per-turn call as well is right — it is what catches the env changing under a running process. The point is only that a typo should be a boot error, and the records-without-SH_REMOTE_SANDBOX=1 check at :44 deserves the same treatment: that combination is a static property of the unit file, and discovering it on the first turn wastes a whole bring-up.

[pdettori]

Fixed in 13353ab, but not where you suggested, and the reason is worth stating.

resolveDiscoverySource lives in @sh/harness, and packages/supervisor ships dependencies: {}
deliberately — the parent is the one process that must never die of a dependency. So the check went into
the worker's boot path instead, which already depends on the harness and is also where
assertKeysetUsable lives, i.e. the precedent you cited. A throw there still surfaces as a crashloop
with the reason in the journal, via the supervisor's restart backoff.

The per-turn call stays, on your reasoning that it's what catches the env changing under a running
process. And the records-without-SH_REMOTE_SANDBOX=1 combination is covered by the same call, which I
agree it deserves: that's a static property of the unit file.

If you'd rather it sat literally beside readConfig, the cost is a dependency edge from the supervisor to
the harness, and I'd want that decided on its own merits rather than as a side effect of this fix.

[cwiklik]

suggestion — this integration test has no guard, so pnpm test in packages/supervisor fails rather than skips on any machine without a live Redis.

vitest.config.ts includes test/**/*.test.ts, so this runs unconditionally, and RedisSessionBackend here will refuse to connect. CI hides it because CI has Redis — which is exactly why it is worth a guard: the contributor who hits it is someone cloning the repo for the first time, and what they see is a red suite with a connection error rather than "1 skipped".

The sibling file already establishes the convention. handoff.integration.test.ts:81 uses it.skipIf(process.platform !== 'linux') for its /proc/self/fd dependency, with a comment explaining that the case would fail rather than skip without it. Same reasoning applies to an external service:

const hasRedis = process.env.REDIS_URL !== undefined || process.env.SH_TEST_REDIS === '1';
describe.skipIf(!hasRedis)('session survives the worker that started it', () => {

Gating on REDIS_URL being set rather than probing keeps it deterministic, and it makes the requirement explicit instead of implicit in a ?? default. The property this test pins — a session outliving the worker that wrote it — is worth keeping; it just should not be the reason a fresh checkout looks broken.

[pdettori]

Fixed in 13353ab — describe.skipIf(!hasRedis), gated on REDIS_URL being set with SH_TEST_REDIS=1 as
the escape hatch, following handoff.integration.test.ts's convention.

Gating on the variable rather than probing, exactly as you suggested: it keeps the requirement explicit
instead of implicit in the ?? default. The suite now reports 2 skipped rather than failing on a machine
without Redis.

[cwiklik]

Correctness review of the P6 supervisor slice. 10 findings, in four themes.

1. The worker boot path replaces startServer() and drops two things it did. worker.ts uses bare createServer(handler) and never listens, so it loses assertKeysetUsable() (the boot-time keyset check #249 added, which this file's own comment cites as its precedent) and loses the SIGTERM handler. The second one matters most: with KillMode=control-group, systemd SIGTERMs every worker at the same instant as the supervisor, so the carefully ordered 20s drain in main.ts::close() drains a pool that is already dead and §3.9's "in-flight turns run to completion" never happens on the real deployment.

2. Three VM gaps that bring up green and fail later. No Environment=HOME= on sh-supervisor.service (every k8s manifest that runs this code sets HOME=/tmp deliberately — service.yaml:33, leaf-scaledjob.yaml:39, control-plane.yaml:190 — and the unit adds ProtectHome=true on top of a --no-create-home user); Redis and the sandbox containers have no restart policy while the supervisor is WantedBy=multi-user.target, so a reboot brings back the supervisor and nothing it talks to; and the README's token step cannot be followed in the order it is written.

3. A one-line wedge in accept(). The handle-less guard returns above the load reconciliation, so the supervisor's optimistic +1 is never corrected — the exact unbounded, uninstrumented 429 wedge that the 30-line comment immediately below it exists to close.

4. Two findings that corrupt E8's own instrumentation. route() ignores readHead's 'closed' outcome and hands off dead sockets, charging W handoff_retries + 1 handoff_failures per abandoned peer — at exactly the saturation knee where peers abandon. And when every worker goes unhealthy between pick and handOff, the connection is destroyed rather than refused, so a driver sees a reset instead of the 429 + Retry-After the identical condition produces one branch earlier, and noteRefusal() is skipped so it cannot be reconciled as spurious.

Credit where due: readHead's incremental carry-scan and cap-vs-terminator ordering are correct, handOff's refusal to branch on send()'s return value is right for the reason recorded, stickyBySession's delete-before-set LRU is correct, reconcile's Number.isFinite value guard is the right shape, close()'s eagerly captured once(server, 'close') avoids the race it names, isTurnRequest does mirror server.ts:641 exactly, and setup-vm.sh's || true / &&-list constructs are errexit-safe.

---

One finding could not be anchored inline (the line is outside this PR's diff hunks for that file):

harness/src/select-sandbox.ts:141 — low. In the new records mode pods is forced to [], so when sh:sandbox:records is empty the thrown message is still no Running pods for pool selector '<selector>' — the exact misdirection that resolveDiscoverySource's own guard at :43-49 was added to prevent ("Blame the flag, not the pool… which sends them debugging a healthy pool"). records is the shipped VM default (env/supervisor.env.example:11), and "no sandbox container has attached to the relay yet" is the single most likely first-run failure there, so this is the message operators will actually hit. The message should branch on source.

[cwiklik]

medium — boot-time keyset validation is lost on this path. This block validates SH_SANDBOX_DISCOVERY but never calls assertKeysetUsable(process.env), which startServer() (server.ts:657) calls "before anything binds". The worker deliberately bypasses startServer (bare createServer(handler), never listens), so under the supervisor a malformed SH_SESSION_TOKEN_PUBLIC_KEYS no longer crashes at boot: every worker reports ready, serves GET /health, and 503s every authenticated /turn.

That is exactly the "failure arrives per request on a healthy-looking deployment" shape #249 closed — and the comment at :253 cites assertKeysetUsable as this block's own precedent while omitting the call.

[pdettori]

Fixed in f6c4f10 — assertKeysetUsable(process.env) now runs in the worker's boot block, in the same try/catch as resolveDiscoverySource, so a malformed keyset exits 2 with the reason in the journal.

Confirmed the gap was exactly as described: startServer is the only caller, and the worker never goes through it. The comment at :253 now says why the call has to be explicit here rather than treating the citation as sufficient.

Pinned by a new case in real-worker.integration.test.ts — the boot block only runs under the main-module guard, so no .mjs fixture can reach it. It forks the real worker with SH_SESSION_TOKEN_PUBLIC_KEYS=garbage and asserts worker_exit code: 2 and that no worker ever read healthy.

[cwiklik]

medium — no SIGTERM handler, so the supervisor's drain never runs on the real deployment. The worker installs only a disconnect handler, while sh-supervisor.service:24 sets KillMode=control-group. systemd's stop job sends SIGTERM to every process in the cgroup, so on systemctl stop/restart all W workers receive SIGTERM simultaneously with the supervisor and die immediately on Node's default disposition.

main.ts::close()'s carefully ordered sequence — drainAll() → stop accepting → awaitIdle(SHUTDOWN_GRACE_MS) — then drains a pool whose workers are already gone, and §3.9's "in-flight turns run to completion" never happens. startServer() did have a SIGTERM handler; this path dropped it.

Fix is either KillMode=mixed (SIGTERM to the main process only, which is what the supervisor's own drain assumes) or a worker-side SIGTERM handler that defers to drain.

[pdettori]

Fixed in f6c4f10 with KillMode=mixed, the first of the two options you named.

Took that one rather than a worker-side SIGTERM handler because mixed is what the drain already assumes: close() runs drainAll → stop accepting → awaitIdle, and workers exit on their own via disconnect once it finishes, so a worker-side handler would be a second path to the same state. mixed still SIGKILLs the whole tree at TimeoutStopSec, which is what control-group was there for.

setup-vm.test.sh now asserts KillMode=mixed is present and control-group is absent, so this cannot silently revert.

One knock-on: main.ts's unhandled-rejection comment cited KillMode=control-group as the mechanism that takes the workers down with the supervisor. That is now the disconnect handler instead, and the comment says so.

Left sh-relay.service on control-group — it is a single process with no fork tree and no ordered drain, so mixed and control-group are the same thing there.

[cwiklik]

medium-low — this guard returns above the reconciliation it needs to run. The handle-less check returns before the send({ type: 'load', … }) a few lines below. handOff has already credited slot.inFlight += 1 for this connection, so every handle-less conn raises the supervisor's estimate permanently.

At SH_TURNS_PER_WORKER=1 a single one saturates the slot: every later connection is refused before hand-off, so no turn can arrive, so no load can arrive — the precise unbounded wedge the 30-line comment immediately below exists to close, and equally invisible in spurious_refusals for the same reason it gives.

Moving the send above the guard fixes it at no cost: reporting the absolute current count is safe whether or not the handle arrived.

[pdettori]

Fixed in f6c4f10 — the send moved above the guard, as suggested. Your reasoning holds exactly: handOff credits slot.inFlight += 1 before the child ever sees the message, and the absolute count is safe to report whether or not the handle arrived.

Regression test in worker.test.ts: accept(undefined) must produce exactly [{type: "load", inFlight: 0}]. Before the move that array is empty, so the test fails on the ordering rather than on the crash the neighbouring test already covers.

The comment now records that the ordering is deliberate, since "guard first" is the reading someone would otherwise restore.

[cwiklik]

low — readHead's 'closed' outcome is ignored, so dead sockets are handed off. read.outcome is consulted only to count 'cap'. When a peer hangs up or RSTs during the sticky-arm pre-read, readHead resolves 'closed' and this code proceeds to handOff a dead socket: child.send(msg, socket) throws, the catch at pool.ts:218 swallows it, and the loop walks every remaining worker.

So one abandoned connection charges W handoff_retries plus one handoff_failures. Those are E8's own instrumentation counters, and client-side abandonment is common at exactly the saturation knee E8 measures — so the numbers stop meaning what PoolCounters documents them to mean.

if (read.outcome === 'closed') return; before pick is the fix.

[pdettori]

Fixed in f6c4f10 with if (read.outcome === 'closed') { socket.destroy(); return; } before pick, as suggested.

The diagnosis is right and the severity is understated — this is not "low". I measured the pre-fix path (reverted the fix, one abandoned connection, SH_WORKERS=2, sticky):

COUNTERS {"restarts":1,"handoffRetries":0,"handoffFailures":0,...}
LOGS     [{"event":"worker_exit","id":0,"code":1,"ranForMs":116,"restartInMs":250}]

child.send(msg, deadSocket) does not throw, so handOff's catch never runs, no retry loop happens, and both counters you predicted would move stay at zero. The child receives a conn whose handle it cannot use and exits — so one connection the client abandoned killed a worker and every turn it was multiplexing. Same class as Critical #1 in the PR description, reached from the other end.

So the counter-integrity argument holds a fortiori, and there is a worse consequence behind it. Pinned in handoff.integration.test.ts on restarts === 0, with the two zero counters asserted as well precisely because they are the ones that do not move. The test drives a live sticky request first so the zeros cannot pass vacuously, and reverting the fix fails it.

Chasing this also surfaced a separate defect on the shutdown path, which is why the same commit touches drainAll. Details in the PR comment.

[cwiklik]

low — a lost race here answers with a reset where the same condition answers 429 one branch earlier. When every worker goes unhealthy between policy.pick() and handOff (a restart window — which main.ts:124 treats as a 429 case), the loop exits and socket.destroy() gives the client an ECONNRESET/empty reply rather than the 429 + Retry-After that route() produces for the identical state.

It also skips pool.noteRefusal(), so the refusal is never stamped and cannot later be reconciled as spurious. A driver cannot distinguish this from a supervisor crash, and on an E8 rung a bare reset is a different data point from a clean back-pressure signal.

The "never just drop it" reasoning in the comment is right about the fd; the gap is the missing response.

[pdettori]

Fixed in f6c4f10 — noteRefusal() + refuse(socket) in place of socket.destroy().

Both halves confirmed: it is the same state main.ts:124 answers 429 for, and the missing noteRefusal() meant this branch's refusals could never be counted as spurious. refuse() ends the socket and bounds the fd with REFUSAL_LINGER_MS, so the "never just drop it" property the comment is about is still closed — by a different mechanism, which the comment now says.

The existing test was renamed and extended: it asserts the 429 and Retry-After are written, destroy() is not called, and then that a later load reporting below the refusal estimate increments spurious_refusals — which is the only externally observable proof noteRefusal() ran. fakeSocket gained resume/end/once for it.

[cwiklik]

low — bounds are applied to parsed values but not to fallbacks. readInt returns fallback unchecked at :30. SH_WORKERS passes cpus().length with { min: 1 }, and os.cpus() is documented as possibly returning an empty array.

With SH_WORKERS unset on such a host the supervisor boots successfully, logs supervisor_listening … workers: 0, forks nothing, and isSaturated([]) is true by design — so it answers 429 to every request forever, with no error anywhere.

This also falsifies the :24-28 comment's claim that "no reader has to work out whether the value in that slot is a legal one": for a computed fallback nothing checks it. Validating fallback against bounds too would close it.

[pdettori]

Fixed in f6c4f10, both halves.

readInt now validates fallback against bounds as well, so the :24-28 claim is true of computed defaults and not only of values an operator typed. And SH_WORKERS' fallback goes through a new exported defaultWorkers(cpus().length) that clamps to 1, so the value is legal by construction.

Clamped rather than left to throw: a host where os.cpus() returns [] has a broken CPU count, not a broken config, and one worker is a working deployment — whereas a supervisor that refuses to boot over it trades a silent wedge for a loud outage the operator cannot act on. That makes the new bounds check unreachable for SH_WORKERS, which is the same relationship the undefined throw already has with readConfig's blank check for SH_TURNS_PER_WORKER; both are nets for the next default, and the comment now says so explicitly.

Tested via defaultWorkers(0) === 1 — a pure function so the empty-cpus case needs no node:os mock — and the existing default assertion now reads defaultWorkers(cpus().length).

[cwiklik]

medium — no Environment=HOME=, unlike every manifest that runs this same code. deploy/knative/service.yaml:33, leaf-scaledjob.yaml:39 and control-plane.yaml:190 all set HOME=/tmp deliberately, with a writable mount behind it. (relay-deployment.yaml does not — consistent with this being a harness-side need, i.e. this unit's, not the relay's.)

Here the unit runs as User=harness (README recommends useradd --system --no-create-home) with ProtectHome=true and ProtectSystem=strict, so $HOME is a nonexistent or masked path and the only writable locations are PrivateTmp's /tmp and StateDirectory. Any turn that writes agent/session state under $HOME fails at runtime — after a completely green bring-up.

setup-vm.test.sh:86 asserts the hardening directives are present, but nothing asserts HOME, so the test suite cannot see this.

[pdettori]

Fixed in f6c4f10 — Environment=HOME=/tmp, matching the three manifests you cite, with PrivateTmp=true already below it supplying the writable path.

Followed your read on the relay: relay-deployment.yaml sets no HOME, so this is a harness-side need and sh-relay.service is left alone.

setup-vm.test.sh now asserts two things, since the second is what the manifests actually pair: that Environment=HOME= is present, and that whatever it points at is writable under this unit's own sandboxing — /tmp (PrivateTmp) or /var/lib/serverless-harness (StateDirectory). Anything else is the same runtime failure with an extra step, and the hardening loop could not have caught it because it only asserts what is present.

[cwiklik]

medium — nothing brings these containers back after a reboot. This podman run -d (and the ones in start_sandboxes) carry no --restart policy and get no generated systemd unit, while sh-supervisor.service is WantedBy=multi-user.target.

So after a VM reboot the supervisor and relay come back and Redis and every sandbox container do not: sh:sandbox:records is empty and every turn fails until an operator re-runs setup-vm.sh. The unit's comment ("The client retries on connect, so ordering against Redis is not load-bearing") is true of ordering but does not cover a container that never starts at all.

Redis also has no volume, so session state is lost on reboot regardless — worth stating explicitly if it is intended for this round.

[pdettori]

Fixed in f6c4f10, and it needed both halves rather than just --restart.

--restart=always is now on Redis and every sandbox container, but podman-run(1) is explicit that it does not restart containers after a host reboot — so setup-vm.sh also enables podman-restart.service, podman's documented mechanism for exactly that. Enabled before start_redis/start_sandboxes so a container started in between is already covered. Not fatal if the unit is missing: that is one podman package's name, and such a host has a working bring-up plus a documented reboot gap, so it warns with the consequence named instead of aborting under set -e.

Your point about the unit's comment stands — it was about ordering and said nothing about a container that never starts.

On Redis's missing volume: intended for this round, and now said out loud rather than implied. start_redis records it, and README.md has a new "Reboots" section stating that --restart=always and podman-restart.service bring the container back and not the data in it.

setup-vm.test.sh asserts every podman run carries --restart=always (with a non-zero guard so it cannot pass vacuously), that podman-restart.service is enabled, that a missing one warns about the reboot rather than failing, and that both the script and the README state the volume gap.

[cwiklik]

low — this instruction cannot be followed in the order it is written. "Set it before running setup-vm.sh" followed by … | sudo tee -a /etc/serverless-harness/relay.env: on a fresh VM neither that directory nor that file exists until setup-vm.sh's install_env creates them, so the tee -a fails.

The actual sequence is: run the script once (it installs both env files, then require_relay_token aborts before install_units/start_redis/start_sandboxes/start_services), edit the token, re-run. The numbered 1–4 list at lines 38-50 also reads as though one invocation completes all four steps, which it never does on a first run.

The parenthetical at line 105 gestures at this, but the imperative above it still points the operator at a path that does not exist yet.

[pdettori]

Fixed in f6c4f10. Both problems you identified, and the fix is to document the real sequence rather than reword the imperative.

"Bring it up" now opens by stating that a first run takes two invocations by design, with the actual three commands: run the script (it stops at the token check), edit relay.env, re-run. It also says the first invocation's non-zero exit is the expected path and not something to debug.

The numbered list is now 1–5 with the token check as its own step 2, so the ordering is visible instead of implied — everything below it runs only on the second invocation.

The token section drops the tee -a imperative for sudoedit after the first run, and mentions appending only as an alternative "once the file exists", stating that the directory and file do not exist before install_env creates them.

[cwiklik]

Review summary — 10 findings

Full inline comments are in review 5202806698, anchored to 13353ab. Scope reviewed: refs/pull/250/head vs merge-base 694464d4 — 38 commits, 43 files, +5168/−12.

Theme 1 — the worker path dropped two things startServer() did

The supervisor rewrite has the worker deliberately bypass startServer() (bare createServer(handler), never listens). Two pieces of safety behavior that lived in startServer didn't come along:

#	Location	Issue
1	packages/knative-server/src/worker.ts:260	Boot validates SH_SANDBOX_DISCOVERY but never calls assertKeysetUsable(process.env). A malformed SH_SESSION_TOKEN_PUBLIC_KEYS no longer crashes at boot — every worker reports ready and serves GET /health while 503-ing every authenticated /turn. That is the same per-request-failure-on-a-healthy-looking-deployment shape #249 closed, and the comment at :253 cites assertKeysetUsable as its own precedent while omitting the call.
2	packages/knative-server/src/worker.ts:291	No SIGTERM handler (only disconnect), while sh-supervisor.service:24 sets KillMode=control-group. systemd SIGTERMs every process in the cgroup, so on systemctl stop/restart all workers die immediately on Node's default disposition — main.ts's ordered close() then drains an already-empty pool and §3.9's "in-flight turns run to completion" never happens on the real deployment. Wants KillMode=mixed or a worker-side handler that defers to drain.

Theme 2 — three VM gaps that pass bring-up and fail later

#	Location	Issue
3	deploy/vm/systemd/sh-supervisor.service:19	No Environment=HOME=..., while deploy/knative/service.yaml:33, leaf-scaledjob.yaml:39 and control-plane.yaml:190 all set HOME=/tmp with a writable mount. The unit runs as User=harness (README recommends --no-create-home) with ProtectHome=true and ProtectSystem=strict, so $HOME is nonexistent or masked. setup-vm.test.sh:86 asserts the hardening directives but nothing asserts HOME.
4	deploy/vm/setup-vm.sh:153	sh-redis and the sandbox containers get no --restart policy and no generated unit, but the supervisor is WantedBy=multi-user.target. After a reboot the supervisor and relay return, Redis and the sandboxes don't, sh:sandbox:records is empty, and every turn fails until someone re-runs setup-vm.sh. The unit's "ordering against Redis is not load-bearing" comment is true of ordering but not of a container that never starts. Redis also has no volume, so session state is lost on reboot regardless.
10	deploy/vm/README.md:99	"Set it before running setup-vm.sh" followed by tee -a /etc/serverless-harness/relay.env — but that directory doesn't exist until install_env creates it, so the documented first step fails on a fresh VM. Real sequence: run once (aborts at require_relay_token after installing env files), set the token, re-run. The 1–4 list at README:38-50 also reads as though one invocation completes all four steps.

Theme 3 — the cheapest fix, worst failure mode

#	Location	Issue
5	packages/knative-server/src/worker.ts:133	accept()'s handle-less guard returns before the send({type:'load'}) reconciliation two lines below, but handOff already credited slot.inFlight += 1. At SH_TURNS_PER_WORKER=1 one handle-less conn saturates the slot permanently: connections are refused before hand-off, so no turn arrives, so no load arrives. That's the exact wedge the 30-line comment below it exists to close, and it's equally invisible in spurious_refusals. Moving the send above the guard fixes it at no cost.

Theme 4 — two findings that corrupt E8's own instrumentation

Both matter because E8 measures the saturation knee, which is precisely where client abandonment and restart windows are common.

#	Location	Issue
6	packages/supervisor/src/main.ts:110	route() uses read.outcome only to count 'cap', never 'closed'. A peer that hangs up mid-pre-read is handed off as a live socket: child.send throws, pool.ts:218 swallows it, and the loop walks every worker — charging W handoff_retries + one handoff_failures for one abandoned connection. Fix: return early on 'closed' before pick.
7	packages/supervisor/src/pool.ts:231	When all workers go unhealthy between policy.pick() and handOff, handOff calls socket.destroy() — while the same condition caught one branch earlier at main.ts:124 returns 429 + Retry-After. The client gets an ECONNRESET indistinguishable from a supervisor crash, and noteRefusal() is skipped so it can't be reconciled as spurious.

Also

#	Location	Issue
8	packages/supervisor/src/config.ts:65	readInt bounds only env-parsed values; fallback returns unchecked at :30. SH_WORKERS passes cpus().length with {min: 1}, and os.cpus() may return []. On such a host the supervisor boots, logs workers: 0, forks nothing, and isSaturated([]) is true by design — 429 to every request forever, no error logged.
9	harness/src/select-sandbox.ts:141	(could not be anchored inline — outside the diff hunks) In records mode pods is forced to [], so an empty sh:sandbox:records still throws no Running pods for pool selector '<selector>' — the misdirection resolveDiscoverySource's own guard at :43-49 was added to prevent. records is the shipped VM default (env/supervisor.env.example:11) and "no sandbox has attached to the relay yet" is the likeliest first-run failure there, so this is the message operators will actually hit. Should branch on source.

Checked and correct

readHead's incremental carry-scan and cap-vs-terminator ordering (no truncated-header mispin is reachable, since sessionIdFromHead is only consulted when complete); refuse()'s resume() + unref'd linger + clearTimeout against pauseOnConnect: true; handOff's refusal to branch on send()'s return value; stickyBySession's delete-before-set LRU; reconcile's Number.isFinite guard; close()'s eagerly captured once(server, 'close'); isTurnRequest's exact parity with server.ts:641; and setup-vm.sh's errexit-safe || true / &&-list constructs.

[pdettori]

All nine review comments addressed in f6c4f10 — each verified against the code first, each with a test. Replies are in the individual threads.

Two were worse than reported, and one new defect fell out of pinning them.

send() to a closed IPC channel does not throw, so every try/catch around one is inert

Not from the review — found because the new abandoned-pre-read test made a dead worker exist at teardown for the first time, and the suite then reported an uncaught ERR_IPC_CHANNEL_CLOSED out of drainAll. Confirmed against Node directly:

send returned, no sync throw
UNCAUGHT: ERR_IPC_CHANNEL_CLOSED

ChildProcess.send() with no callback does process.nextTick(() => this.emit("error", err)). On a child with no error listener that is an unhandled error event, i.e. an uncaughtException. So the try { handle.send(...) } catch in drainAll was guarding against the one shape this failure never takes.

It is on the ordinary shutdown path, not a corner. Whenever any worker is already dead — a crashloop, an OOM kill, and now a boot-validation exit — close() calls drainAll(), which sent to that dead channel, and the supervisor died mid-drain, before awaitIdle, taking the in-flight turns on every other worker with it. Precisely the outcome the ordered close() exists to prevent, and it interacts with the KillMode fix: that one makes the drain reachable, this one makes it survivable.

drainAll now checks connected first (a dead worker is already drained), and every child gets an error listener so the absence of one cannot be fatal. Two tests: no drain is sent down a gone channel while later workers still get theirs, and an error event is logged rather than thrown.

Verification

make fmt, make lint, make typecheck, pnpm -r test (1555 passed, 22 skipped, 0 failures, no unhandled errors) and make test-deploy all clean. The two behaviours I measured rather than assumed — the abandoned pre-read killing a worker, and the async channel error — are recorded in the code comments and test rationale, so the next reader gets the measurement and not the prediction.

🤖 Generated with Claude Code

[cwiklik]

Re-review of 13353ab...f6c4f10 (+509 lines over 14 files, force-pushed; commit count unchanged at 38). Nine of my ten findings are fixed, each with a test that fails without the fix. One is not addressed here and is now carried on #253. Two new items below, neither blocking.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: TypeScript (supervisor, knative-server), systemd units, shell, docs, tests
Agent/IDE config (.claude/.vscode): none — supply-chain gate clear
Commits: 38, all signed-off
CI status: passing (12/12, incl. deploy-scripts, which does run the new shell test)

Per-finding verdicts

#	Item	Verdict
1	Worker boot skips keyset validation	Fixed — assertKeysetUsable(process.env) inside the boot try, before resolveDiscoverySource, so it exits 2 rather than serving 500s
2	KillMode=control-group defeats the drain	Fixed — mixed; premise verified (main.ts:220 does install a SIGTERM handler, with a re-entrancy guard)
3	ProtectHome with no writable HOME	Fixed — Environment=HOME=/tmp, asserted writable
4	Containers do not return after reboot	Fixed — --restart=always on all four podman runs plus podman-restart.service, non-fatally
5	Handle-less conn loses the load report	Fixed — report hoisted above the guard
6	Closed socket handed off anyway	Fixed — see below
7	Hand-off failure resets the client	Fixed — noteRefusal() + refuse(); test asserts 429 + Retry-After, no destroy, and spuriousRefusals 0→1
8	readInt exempts its own fallback	Fixed — bounds now apply to the fallback; I checked all four call sites, none newly throws at boot, and the undefined check still fires first so SH_TURNS_PER_WORKER is unaffected
9	records-mode emptiness blames the pool selector	Not addressed — select-sandbox.ts is not in this delta. Raised on #253, where SandboxPoolEmptyError now owns that message; no need to fix it here
10	README's first run cannot succeed as written	Fixed — two invocations stated explicitly, steps renumbered, sudoedit, new "Reboots" section

On finding 6, your diagnosis is better than mine

I claimed child.send throws and that pool.ts's catch swallows it. You measured that it does not throw, so the counters that would have shown it stay at zero and the worker dies instead — worker_exit code: 1, restarts: 1, taking every turn it was multiplexing. That is a worse failure than the counter inflation I described, and I had the severity wrong.

The fix is also airtight rather than merely narrower, for a reason worth recording: once the await readHead(socket) continuation resumes, the path through policy.pick to handOff is entirely synchronous, so nothing can destroy the socket in between. The guard has no residual window. (Any future await inserted there would reopen it.)

The unrequested work is the right call

drainAll's connected check and spawn's handle.on('error') are correct, and "Required, not defensive" is the accurate label — an unlistened 'error' on an EventEmitter is an uncaughtException, not something a try/catch or a .catch() can reach. I verified the surrounding invariants hold: both restart paths gate on shuttingDown (pool.ts:418, :435), so no slot is respawned mid-shutdown, and ready_ignored_draining covers the late-ready case.

I also checked that the new test is not vacuous, since its comment rests on it: FakeWorker extends EventEmitter, so expect(() => h.forked[0].emit('error', ...)).not.toThrow() really does fail with the listener removed. Same for the >= 4 non-vacuity guard in the shell test — SH_SANDBOX_COUNT=3 is exported at setup-vm.test.sh:54, so Redis + 3 is the right count and the guard is live.

Worth naming across PRs: this is the same defect class as #253's redis-backend.ts, where seven createClient sites still register no 'error' listener. The mechanism you write up here is the argument for fixing it there.

Verdict

APPROVE. The two items below are a comment-accuracy issue and a stale cross-reference; neither should hold the branch.

One procedural note: your own CHANGES_REQUESTED (5198443923) is still on this PR and my approval does not clear it — mergeable_state stays blocked until you re-review.

One limitation worth stating

git fetch fails in my environment (self-signed certificate in the proxy chain), so my clone is pinned at a982ffd. Everything above was read from the PR's HEAD (f6c4f10) through the API, not from that clone.

[cwiklik]

suggestion — this comment and SHUTDOWN_GRACE_MS now disagree about what this number is for, and the KillMode fix is what made it matter.

"The deadline must exceed the longest turn E6 measured" describes a stop timeout that protects a long turn. But close() stops waiting at SHUTDOWN_GRACE_MS = 20_000, logs grace_expired, and proceeds — so a turn still running at 20s is killed by the supervisor, and the remaining 100s of this window never protects anything. main.ts:33-35 says as much and assigns the opposite role: the 120s is "the outer bound … so systemd's SIGKILL stays a genuine backstop for a supervisor that has hung rather than a race against this timer."

That reading is the correct one, and this line is the misleading one. It matters now in a way it did not before this delta: under KillMode=control-group the drain never really ran, so the relationship between the two deadlines was untestable. mixed makes it live, and 20s is now the real §3.9 boundary.

If E6's longest turn is anywhere near this 120s, the interesting consequence is not the comment — it is that §3.9's "in-flight turns run to completion" holds only for turns under 20s, on the ordinary systemctl stop path. SHUTDOWN_GRACE_MS's own doc comment already flags itself as a chosen value pending a spec sentence, so I would not change the constant here; I would fix this line to say what the timeout is (a hang backstop) and let the recorded spec gap carry the rest.

[cwiklik]

nit — this cross-reference points at the one section that does not say this.

"What round one does not claim" (README:213) is exclusively about the systemd hardening directives and the absent NetworkPolicy analogue; it does not mention the volume. The statement this line promises is in the "Reboots" section you added in this delta (README:169-174, "Redis state does not survive a reboot either way.") — which states it well, including the --restart=always brings back the container, not the data point made just below here.

So an operator who follows the pointer lands in a section about syscall filters. Retargeting it to "Reboots" is the whole fix.