fix(harness): route /turn's tool calls to the sandbox pool

[pdettori]

Closes the gap ADR 0028 deferred, from the /turn side. Behaviour change on a live path — please
read "Behaviour changes" before merging.

Problem

/turn resolved its sandbox with resolveSandboxConfig alone — the single-pod path — so it ignored
KAGENTI_SANDBOX_POOL_SELECTOR, SH_SANDBOX_DISCOVERY and SH_REMOTE_SANDBOX. On any deployment
that configures a pool, a turn's tool calls ran inside the harness process.

run-leaf.ts fixed this for prompt leaves and its comment names this exact case: "on a deployment
that sets only a pool selector, ran its tools in the harness container itself".

Evidence, observed rather than inferred

1. SSE frames for a tool-call turn: tool_use parsed correctly, then tool_result with
   isError: false and the command's real stdout — the tool ran.
2. The file that command created appeared in none of the three sandbox containers, and not in the
   host's /tmp.
3. It was in /tmp/systemd-private-…-sh-supervisor.service-…/tmp/ — the supervisor unit's
   PrivateTmp. The tool ran in the worker process.
4. At the same moment, a direct probe of the relay's Exec RPC returned hello-from-sandbox,
   uid=1001, exit 0 from that pool. Relay, leaf, Redis records and host.containers.internal all
   worked — they were simply not on /turn's path.

Design — one seam, not two call sites

runTurn delegates to executeTurn, so every server path funnels through what is now
acquireTurnSandbox. Crucially, selectPoolSandbox's own first branch is
if (!selector) return resolveSandboxConfig(...) — exactly the call this replaced — so a deployment
with no pool selector resolves identically by construction, not by care, and no future caller of
executeTurn can bypass the seam. Tests pin that equivalence and pin that such a deployment arms no
lease renewal.

The lease lifecycle lives in executeTurn wrapping executeTurnCore, so every exit path returns the
lease — normal return, throw, and abort. leased is true only when this call took the lease: an
injected sandbox belongs to its caller, and releasing it here would free a sandbox another turn is
still executing in.

Behaviour changes (the review-worthy part)

1. A configured pool with no reachable candidates now fails the turn instead of silently running
   tools locally. Honest, but it will surface as 500s on any deployment whose pool is momentarily
   empty — turns that previously "succeeded" with local tools.
2. SandboxPoolSaturatedError → 503, not 500. It is transient; /runs already treats it that
   way and classifyOutcome keeps it retryable, so 500 made one signal mean "never retry" on one
   route and "retry" on another. Now a shared turnErrorStatus(), because the sync and SSE
   pre-first-frame blocks were duplicated and §3.4 regime 2 requires them byte-identical.

turnErrorStatus matches the error's name, not instanceof. instanceof is the in-package idiom
but across a workspace boundary it needs both packages to resolve one module instance — false whenever
a test mocks @sh/harness/run-turn wholesale, as server.test.ts does, where the import yields
vitest's "no export" stub and instanceof throws, turning three unrelated turn errors into 500s.
That was observed, then fixed by not depending on module identity. The paired test constructs the real
class, so the string stays pinned to it.

Also here: a Redis client was opened per turn

Included because this change is what makes it fatal, so shipping them apart would ship a known
outage.

Three clients were constructed on the per-turn path, each connecting eagerly in its constructor:

Client	Where	Behaviour
RedisSessionBackend	executeTurnCore	never closed → leaked
RedisLeaseStore	selectPoolSandbox	never closed → leaked
RedisRecordStore	selectPoolSandbox	closed → churned

Each is individually correct and all were latent, because a turn used to be served by a container that
went away afterwards.

Measured at 27–54 turns/s: ~10k turns produced 35,654 connections, Redis answered
ERR max number of clients reached (maxclients 10000, 11 rejected), node-redis raised it as an
'error' on clients with no listener, and all four supervisor workers exited code 1
simultaneously ~13 minutes in, stranding every in-flight turn.

After: all three are process-wide and reused. A second warm batch of 120 turns cost 2 connections
(both the measurement's own redis-cli) with connected_clients delta 0. Connections no longer
scale with turns.

The two select-sandbox stores drop their memo when a call through them rejects — caching a client
that never connected would turn one transient Redis failure into a permanent "no sandboxes" or a
permanent inability to acquire, for the life of the process.

Tests

• run-turn-sandbox.test.ts — the seam, including the no-pool equivalence and no-renewal properties.
• select-sandbox.test.ts — now asserts store reuse (it previously asserted the per-call
  construct-and-close that caused the leak) plus drop-on-failure.
• turn-error-status.test.ts — the 503 mapping, and that a saturation-shaped message is still 500.
• redis-client-per-turn.test.ts — source-level guard. No unit test can see this defect class: each
  construction is individually correct, the leak only appears as an aggregate over thousands of turns,
  and a suite that mocks Redis never opens a socket. It includes a check proving the forbidden pattern
  is detectable, so the absence-assertion is not vacuous.

Verified on this base: harness 412 passed, knative-server 312 passed, typecheck and lint clean,
plus the hardware evidence above.

Not in this PR

The supervisor-side sandbox-pool telemetry (sandbox_pool_size, lease_saturation) depends on
packages/supervisor, which is not on main yet — it follows separately. The leaf images' missing
/workspace is #252.

🤖 Generated with Claude Code

[cwiklik]

Reviewed 74a86d0 — 7 files, +713/−58. Over the 500-line threshold, so this is deliberately scoped to security, correctness and design rather than style; I skipped nit-level comments.

The diagnosis and the direction are right, and I want to be clear about that before the blockers. The leak is real and I verified it independently: executeTurnCore did new RedisSessionBackend(...) per turn and never closed it (694464d), RedisSessionBackend's constructor connects eagerly (redis-backend.ts:24), and the per-call lease/record stores compound it. A process-lived supervisor worker turns a latent per-container leak into maxclients exhaustion, and acquireTurnSandbox is the right seam — one funnel for every executeTurn caller, with the lease lifecycle in a finally so normal return, throw and abort all release. turn-error-status.test.ts is the strongest test in the PR: five cases including a negative for a saturation-shaped message and a pin on the class name.

Three must-fixes. Two are in the new memoisation and one is in the lease-holder id. What makes them worth blocking on is that in each case the PR already contains the correct reasoning somewhere and then does not apply it at the one place it matters:

• The lease runId comment states the requirement ("must be unique per turn") and the expression on the next line does not satisfy it.
• redis-client-per-turn.test.ts's third case states the drop-guard hazard verbatim, and asserts it only against select-sandbox.ts — exempting the one store built without a guard.
• sharedRecords closes the old client when the URL changes, four lines before a drop() that does not close and can evict the wrong store.

Claims verified

Claim	Verified
Per-turn RedisSessionBackend leaked	git diff 694464d: - const store = new RedisSessionBackend<FileEntry>(redisUrl); with no close. Constructor connects eagerly at redis-backend.ts:24.
Saturation is transient, so 503 not 500	Agreed, and classify-outcome.ts:16 confirms the async half — reason: 'saturated' → {ack: false, retryable: true}.
name comparison survives a wholesale module mock	Correct, and the right call. instanceof across the workspace boundary is unsound when server.test.ts mocks @sh/harness/run-turn; turn-error-status.test.ts:38 pins the string to the real class so a rename fails loudly. No change needed here.
A repeated runId would "share a lease"	Confirmed against ACQUIRE_LUA (sandbox-lease.ts:19-26) — see the must-fix on run-turn.ts:559.
Number(env ?? default) matches existing style	Yes — run-leaf.ts:391,576,723 for cap/ttl and :423,612,749 for the heartbeat all use the bare form. I had this down as a NaN finding and dropped it: the new code is exact parity with three existing call sites, so it is a pre-existing repo-wide question and not this PR's to answer.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: TypeScript (harness, knative-server), Tests. Cross-referenced sandbox-lease.ts, classify-outcome.ts, run-leaf.ts and packages/session-backend/src/redis-backend.ts, none of which the PR touches.
Agent/IDE config (.claude/.vscode): none — grepped both +++ b/ and rename to forms; 0 renames in the diff
Commits: 2 (ade97b7, 74a86d0), both signed off, both Assisted-By per house convention, fix(harness): / style(harness):, subjects 58 and 53 chars
CI status: passing — 12/12 green on 74a86d0
Base: 694464d (merge-base confirmed); mergeable_state: blocked; head is from the pdettori/serverless-harness fork. Note the overlap with #250 — see the comment on select-sandbox.ts:115.

[cwiklik]

must-fix — this expression is the negation of the comment directly above it. sessionId is per session; the comment requires per turn.

the caller supplied none — and it must be unique per turn or two turns would share a lease.

That requirement is right, and input.sessionId ?? randomUUID() does not meet it. A session id is stable across every turn of a session — that is its entire purpose (:423-439 reopens a checkpoint by it). So the ?? only reaches randomUUID() for anonymous turns; every turn that carries a session id — the resume path, the one the supervisor exists to serve — uses a runId shared with every other turn of that session.

Why sharing it is not benign. The runId is the ZSET member, not a payload, so a repeat is one lease rather than two. From ACQUIRE_LUA (sandbox-lease.ts:19-26):

redis.call('ZREMRANGEBYSCORE', KEYS[1], '-inf', ARGV[1])
if redis.call('ZCARD', KEYS[1]) < tonumber(ARGV[2]) then
  redis.call('ZADD', KEYS[1], ARGV[4], ARGV[3])   -- ARGV[3] = runId = the member
  return 1
end

ZADD on an existing member updates its score and leaves ZCARD unchanged. So with two concurrent turns on session S:

1. Turn A acquires: ZCARD 0 < cap → member S added → 1.
2. Turn B acquires on the same pod: ZCARD 1 < cap → ZADD member S again, score refreshed, ZCARD still 1 → 1.

Two consequences, and the second is the one the comment was warning about:

• The cap undercounts. N concurrent turns of one session occupy one lease slot. With KAGENTI_SANDBOX_CAP=20 a single session can pile arbitrarily many turns onto one pod while the pool reports headroom. That directly undercuts the saturation accounting this PR builds on — SandboxPoolSaturatedError and its new 503 are computed from a count that a shared runId deflates.
• The first turn to finish releases the sandbox the other is still using. release(pod, runId) is zRem (:69), so turn A's finally removes the single shared member while turn B is still executing in that pod. The pod then reads as free for new work. Worse, B's heartbeat is zAdd (:65), so it silently resurrects the member a few seconds later — the lease flaps between released and held, and load() returns a different answer depending on where in the heartbeat interval you sample it.

This is exactly the harm run-turn-sandbox.test.ts:65 names — "release the sandbox another turn is still executing in" — asserted there for the injected path only.

Nothing covers it. Every case in run-turn-sandbox.test.ts calls acquireTurnSandbox directly with an explicit unique runId ('run-1', 'run-42', 'r'). The derivation on this line lives in executeTurn, which no test in the PR exercises, so the seam is well covered and the one expression feeding it is not.

Fix — the runId identifies a lease holder, not a conversation, and nothing reads it back:

randomUUID(),

If the session id is wanted for debuggability, compose rather than substitute — `${input.sessionId ?? 'anon'}:${randomUUID()}` — which stays unique per turn and still greps. Either way a test that drives executeTurn twice with one sessionId against a fake lease store and asserts two distinct acquire runIds would pin it; that assertion fails on this line today.

[pdettori]

Fixed in fb72ccf — turnRunId(), a fresh UUID per turn with the session id prefixed for greppability only.

Confirmed the mechanism against ACQUIRE_LUA before changing it: the runId is the ZSET member, ZADD on an existing member refreshes the score and leaves ZCARD unchanged, so both consequences follow — the cap undercounts, and release's zRem drops a member a sibling is still executing under while its zAdd heartbeat resurrects it.

On the test gap: you were right that nothing covered the derivation, and a pure-function test would not have either, since the defect was the one expression wiring it. harness/test/turn-lease-run-id.test.ts drives executeTurn twice with one sessionId, intercepting at selectPoolSandbox so the turn stops before executeTurnCore (no Redis, no Pi session, no model) with the real derivation observable. Verified it fails against the old expression — both turns arrived as sess-1 — and passes now.

[cwiklik]

must-fix — this is the one store without a drop guard, and it is the one where a failed connect is permanently unrecoverable. The reasoning in the comment above has it backwards.

The comment justifies omitting the wrapper:

node-redis reconnects a live client by itself, and the failure mode that wrapper guards against is caching a client that never connected in the first place.

Both halves are accurate. The conclusion inverts them — "caching a client that never connected" is precisely what this memo does, and node-redis's reconnect cannot help because it only rejoins a client that was live. RedisSessionBackend stores its connect attempt as a promise and every method awaits that same promise (packages/session-backend/src/redis-backend.ts):

constructor(url = process.env.REDIS_URL ?? 'redis://127.0.0.1:6379') {
  this.client = createClient({ url });
  this.ready = this.client.connect().then(() => undefined);   // :24
}
async nextPosition(sid: string) { await this.ready; /* :28 */ }

ready is assigned once and never reassigned. If that first connect() rejects — Redis mid-rolling-restart, a DNS blip, the worker winning the race against Redis on a cold namespace — ready is a permanently rejected promise, and it is now memoised process-wide. Every subsequent turn calls sharedSessionStore(url), gets the same instance, and await this.ready rejects instantly with the original connect error. Redis coming back changes nothing. Only a pod restart clears it.

This is a recovery regression introduced by the fix. Before this PR the per-turn new RedisSessionBackend(...) meant a blip cost exactly one turn and the next turn built a fresh client that connected. After it, one unlucky moment costs the worker's entire remaining lifetime. Trading a bounded leak for an unbounded outage is a worse deal than the leak, and it triggers in the window the leak never mattered in — the first turn after boot.

It compounds on the async path. The failure surfaces as reason: 'error', which classify-outcome.ts:16 keeps retryable: true, so the entry stays queued and is redelivered — to the same poisoned process, which fails it instantly again. That is a hot retry loop with no backoff and no terminal state, not a stall.

The PR already argues this, one file over. redis-client-per-turn.test.ts's third case is titled "the shared stores are dropped on failure rather than cached broken" and its comment reads:

Caching a client that never connected would convert one transient Redis failure into a permanent one for the life of the process

That is this defect, stated exactly, in this PR — and the test then asserts guard/recordsMemo = null/leaseMemo = null against select-sandbox.ts only. The single store built without the guard is the one the test does not look at.

Fix — either wrap it the same way the other two are:

const store = sessionStoreMemo.store;
const drop = () => { if (sessionStoreMemo?.store === store) sessionStoreMemo = null; };
// ...return a facade whose methods pass through guard(..., drop)

or, better, fix it at the source so every caller benefits: make ready re-armable in RedisSessionBackend — on rejection, clear it so the next await this.ready retries connect(). That is a smaller change than a facade over eight methods, it removes the "connects eagerly" hazard the whole PR is written around, and it also fixes close() (:93-94), which awaits ready and therefore cannot close a never-connected client — the .catch(() => {}) on line 62 is silently swallowing that today and leaving the socket dangling.

Whichever you pick, extend redis-client-per-turn.test.ts's third case to cover run-turn.ts, so the store and the assertion stop diverging.

[pdettori]

Fixed in fb72ccf, taking the second option you offered — re-armable at the source rather than a facade over eight methods. RedisSessionBackend.arm() clears the memo when an attempt rejects, open() starts a fresh one, and close() no longer awaits a failed connect, so a never-connected client can actually be closed instead of leaving the socket dangling behind a swallowed .catch.

One correction to the diagnosis, which narrows the trigger without weakening the finding. The causes named here — "Redis mid-rolling-restart, a DNS blip" — do not reject with node-redis's defaults. defaultReconnectStrategy answers a refused connect with an exponential backoff, and #connect() loops while (this.#isOpen && !this.#isReady), so connect() retries forever and callers simply wait. I found this by pointing the first version of the test at a dead port: it hung for 5s and timed out rather than rejecting.

The rejecting path is narrower and, in a cluster, more likely: connectTimeout defaults to 5000ms (socket.js:45), a timeout raises SocketTimeoutError, and that is the one cause defaultReconnectStrategy returns false for (socket.js:403-407) — so #shouldReconnect throws a ReconnectStrategyError out of connect() and no retry happens. A Service with no ready endpoints, or a NetworkPolicy drop, black-holes the SYN rather than refusing it, which is exactly that shape. So the permanent-poisoning regression is real; it is a connect timeout that reaches it, not a refusal. Both source comments now say so, since the old ones would have sent the next reader looking for the wrong failure.

Tests: packages/session-backend/test/redis-backend-rearm.test.ts mocks the redis module rather than using a real endpoint — deliberately, because both real failure modes make bad tests (a refused connect retries forever and hangs; a black-holed address costs 5s per attempt and depends on the network answering with silence). Three of its five cases fail against the pre-fix class, and the two that pass are the ones pinning unchanged happy-path behaviour (one connect per backend, so the re-arm cannot reintroduce the per-turn leak).

And you were right that the structural guard and the store had diverged. redis-client-per-turn.test.ts's third case now also asserts the re-arm in redis-backend.ts and the identity check in select-sandbox.ts, so the claim and the code are checked in the same place.

[cwiklik]

must-fix — drop() leaks the connection it drops, and can evict a store that is perfectly healthy. Both are two-line fixes; this is the mildest of the three blockers and I would take the identity check alone.

const drop = () => {
  recordsMemo = null;
};

It does not close. Nulling the memo abandons a live, connected client with no remaining reference — so every transient Redis failure leaks exactly one connection, permanently, in the PR whose purpose is stopping connections from accumulating to maxclients. Four lines above, the URL-change path gets this right and says why: void recordsMemo.store.close().catch(() => {}). The failure path deserves the same treatment more, not less, since it is the one that can fire repeatedly.

It captures no identity, so it can drop the wrong store. drop closes over nothing and unconditionally assigns recordsMemo = null. guard calls it when a promise rejects, which can be long after the call was issued:

1. Turn A calls list(); the store is store₁. The command hangs.
2. REDIS_URL changes (or resetSharedRecords() runs); the memo is rebuilt with a healthy store₂.
3. A: store₁'s command finally rejects → guard → drop() → recordsMemo = null — discarding store₂, which never failed.
4. The next call builds store₃, and store₂ is orphaned: connected, unreferenced, never closed. Another leak, and the reconnect the guard exists to trigger is spent rebuilding something that was already fine.

The window is small, but it is exactly the situation the guard is for — Redis is misbehaving and commands are in flight. Under a flapping Redis, steps 1-4 can chain.

This class was already flagged and fixed once in this repo. On #249 round 1 I raised the same shape at turn-auth.ts:269 — clear the state before attempting the close, so a throwing close cannot skip the rebuild — and the fix landed at :318-331. Worth reusing that shape here rather than re-deriving it.

Fix, both stores:

const drop = () => {
  if (recordsMemo?.store !== store) return;   // a later store superseded this one; leave it alone
  recordsMemo = null;
  void store.close().catch(() => {});         // the client is unreachable now; do not leak it
};

One caveat on the close, since it argues against itself: because the store is shared, closing it on one caller's failure aborts commands belonging to concurrent callers holding the same instance — turning one turn's Redis error into several. If that trade is unwelcome, keep the identity check and drop the close; a bounded leak of one connection per distinct failure is defensible, whereas evicting healthy stores is not. What is not defensible is the current combination of both.

While here: resetSharedRecords (:89) resets the lease memo too. Given it is the test-only seam for both, resetSharedStores would stop the next reader assuming leases survive it.

The test does not catch either half, because it matches source text:

expect(body).toMatch(/recordsMemo = null/);

That passes verbatim against the code above. A behavioural test — inject a store whose list() rejects, assert the next call constructs a new one and that close() was called on the old — would distinguish the two, and would fail today on the close.

[pdettori]

Fixed in fb72ccf — took both halves, via a shared dropMemo(store, read, clear) used by records and leases: identity-check, clear, then close. Reused the #249 turn-auth.ts:318 ordering you pointed at (clear before close, so a throwing close cannot skip the rebuild).

On the caveat that argues against the close — I think it does not apply here, and the reason is a version difference. The objection assumes closing aborts concurrent callers' in-flight commands. In redis 6 it does not: close() is documented in @redis/client/dist/lib/client/index.d.ts:433 as "Close the client. Wait for pending commands", and destroy() (:437) is the one that "Rejects all commands immediately". RedisRecordStore.close() and RedisLeaseStore.close() both call client.close(), so concurrent commands on the shared store drain rather than failing. Combined with clearing the memo first — no new caller can reach that store — the close looks safe, which is why I did not take the identity-check-only option.

That matters because the alternative is not neutral: without the close, each distinct failure permanently abandons one connection, and under a flapping Redis those accumulate toward the same maxclients ceiling this PR exists to stop. Leaking slowly is still leaking.

If you read close()'s guarantee differently, say so and I will drop back to the identity check alone — that half is uncontested and is most of the value.

Also renamed resetSharedRecords → resetSharedStores, since it always reset the lease memo too.

On the test: agreed that matching source text could not tell the two apart. Both halves now have behavioural cases in select-sandbox.test.ts — the existing failure case additionally asserts close() on the dropped store, and a new one holds store₁'s command open, changes REDIS_URL so a healthy store₂ is memoised, then rejects store₁ and asserts store₂ is neither evicted nor closed. Verified both fail against the old drop.

One process note: my first attempt to verify that negative was invalid — Prettier had wrapped the drop line, so the string I reverted never matched and the suite passed against what I thought was pre-fix code. Re-ran it properly; the two cases do fail as claimed.

[cwiklik]

suggestion — saturation gets 503 because it is transient; a momentarily empty pool is equally transient and falls through this line to 500.

The reasoning for the 503 is right, and turn-error-status.test.ts states it well:

Mapping it to 500 here would make one signal mean "retry me" on one route and "never retry" on another.

Behaviour change 1 makes a second transient condition newly fatal on /turn: selectPoolSandbox throws new Error(`no Running pods for pool selector '${selector}'`) (select-sandbox.ts:213) — a plain Error, so name is 'Error', the message does not contain no session in backend, and it lands on 500. The PR body concedes the consequence: "it will surface as 500s on any deployment whose pool is momentarily empty."

An empty pool is at least as retryable as a full one — a pod rolling, an HPA scaling from zero, presence records not yet mirrored after a restart. "Every sandbox is busy" gets Retry-After; "the sandboxes are still starting" gets a code that tells the caller it can never succeed. Same cause (capacity not available yet), opposite advice.

To be fair to the current state, this is narrower than it first looks and that is why it is not a blocker:

• The async path is already fine — the failure classifies as reason: 'error', which classify-outcome.ts:16 keeps retryable: true, so the queue entry stays pending and drains once pods appear.
• Making the turn fail here rather than silently running tools in the harness process is the right call and I am not arguing against it (run-turn-sandbox.test.ts:169 covers it, and spec §5.4 is the reason).

So this is only about what the sync /turn caller is told. A named error is enough:

export class SandboxPoolEmptyError extends Error {
  constructor(selector: string) {
    super(`no Running pods for pool selector '${selector}'`);
    this.name = 'SandboxPoolEmptyError';
  }
}

thrown at select-sandbox.ts:213, with this function returning 503 for it alongside the saturation case — same name-not-instanceof technique, for the same mock-safety reason, and the existing test file extends by one case. If you would rather not add a class, reusing SandboxPoolSaturatedError for the empty case is defensible: from the caller's side "no capacity right now, retry" is the same fact, and it needs no change here at all.

[pdettori]

Implemented in fb72ccf, as the named class rather than the reuse: SandboxPoolEmptyError in select-sandbox.ts, thrown at the candidates.length === 0 site, re-exported from run-turn.ts for the same contract reason as the saturation error, and matched by name here alongside it.

Went with the class over reusing SandboxPoolSaturatedError because the two are worth telling apart in logs — "all pods at capacity" and "no pods yet" call for different operator responses — even though the HTTP answer is identical. turnErrorStatus maps both through one NO_CAPACITY set, with a comment stating that the collapse is deliberate so a future reader does not "helpfully" split them and hand a client a fatal code for a transient condition.

The message is byte-identical to the plain Error it replaces, so log greps and the existing /pool selector/ assertion in run-turn-sandbox.test.ts keep matching. turn-error-status.test.ts gains the 503 case and pins the second name string to its class.

[cwiklik]

suggestion — coordination: #250 adds this exact function to this exact file. Both PRs are based on 694464d, so whichever merges second breaks the build.

I diffed the two versions and they are byte-identical — the same 18 lines, including the records + !remoteSandbox cross-check. That is a good sign for the design (two independent needs converged on the same contract), but it means the second merge either conflicts or, if git resolves the identical addition cleanly, leaves the file with two export function resolveDiscoverySource declarations — a TypeScript redeclaration error, caught by CI rather than by review. Worth agreeing now which PR carries it so the other rebases it away, rather than discovering it at merge.

Related, and the reason I am raising it here rather than only on #250: this PR strengthens the boot-validation argument I made there. On #250 the point was that a malformed SH_SANDBOX_DISCOVERY is a permanent misconfiguration diagnosed per request instead of at startup. After this PR that same throw reaches /turn for the first time, so the blast radius grows from the pool-discovery path to every turn — and because it is a plain Error, it surfaces as a 500 (see the comment on turnErrorStatus), which reads as "the server is broken" for what is squarely "this deployment is misconfigured." Validating it once at boot and crashlooping is both a clearer signal and cheaper than re-parsing per turn. Not blocking here — it is #250's call — but the case for it is stronger with this PR in than without it.

[pdettori]

Confirmed the overlap independently — I extracted resolveDiscoverySource from both branches and diffed: byte-identical, 18 lines, including the records + !remoteSandbox cross-check.

Decision: #253 carries it, #250 rebases its copy away. Reasoning is just cost — #253's select-sandbox.test.ts already imports and exercises the function, so leaving it here changes nothing and #250 deletes 18 lines, whereas the reverse would also cost #253 the call site and those tests. No change in this push; flagging it for #250.

On boot validation: agreed the case is stronger with this PR in, and worth noting the empty-pool fix does not cover it. A malformed SH_SANDBOX_DISCOVERY still throws a plain Error per request and still reaches /turn as a 500 — correctly, since unlike an empty or saturated pool it is a permanent misconfiguration that no retry fixes. So the 503 work here does not weaken the argument for crashlooping at boot; it just means the remaining 500 is the honest answer for that one. Still #250's call.

[cwiklik]

Three fixes with unusually good diagnosis, and two blockers that both sit in the gap between what the code does and what its own comments claim.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: TypeScript (harness, knative-server, session-backend), tests, plus node-redis 6.2.1 internals
Agent/IDE config (.claude/.vscode): none — supply-chain gate clear
Commits: 3, all signed-off (3/3), conventional prefixes
CI status: passing (12/12)

Finding 1 is the important one: the re-arm machinery and its test are correct, but the failure mode they exist for kills the worker before the re-arm can run, because no store in the workspace registers an 'error' listener — the amplifier your own comment identifies as what took down four workers is the one thing left untouched. Finding 2 is a status-contract regression created by the lease hoist.

Verified rather than taken on trust

• The re-arm's other load-bearing assumption holds. connect() guards with if (this.#isOpen) throw new Error('Socket already opened') and sets #isOpen = true before attempting — but #shouldReconnect clears it back to false on the terminal path, so a second connect() on the same client genuinely re-attempts. Nothing tests this, and it would silently invalidate the whole design if it were false.
• close() really does drain. @redis/client's close() waits for #queue.isEmpty() before destroySocket(), while destroy() "Rejects all commands immediately" — so dropMemo's concurrent-caller safety argument is accurate. One precision: it drains pending commands but rejects new ones, so a caller mid-multi-command sequence (append's incr → xAdd) can still see a ClientClosedError.
• redisUrl is process-env-derived (buildConfig(), server.ts:68), so sharedSessionStore's URL-change branch cannot thrash under concurrent turns.
• The name-over-instanceof argument is sound, and the base never closed the session store per turn — so nothing double-closes the now-shared one.
• The code() comment-stripper in redis-client-per-turn.test.ts is exactly the fix I asked for on #252's parity test; good to see it land as a pattern.

Verdict

REQUEST_CHANGES on findings 1 and 2. The other four I would take as follow-ups.

One limitation worth stating

git fetch fails in my environment (self-signed certificate in the proxy chain), so my local clone is pinned at a982ffd (2026-09-09), five days behind this PR. Base-tree citations (run-turn.ts:438-450, select-sandbox.ts:76-79, server.ts:68/:422) were read from that older tree; the node-redis internals came from the installed @redis/client@6.2.1. All six findings rest on the diff itself.

[cwiklik]

must-fix — the re-arm cannot run in production, because the worker dies first.

arm()'s doc comment says the side .catch means "a rejected connect that nobody is awaiting yet can no longer surface as an unhandled rejection (node-redis raising 'error' on a client with no listener is how four workers exited code 1 simultaneously)". Those are two different channels, and only the first one is addressed.

I traced the exact path the comment names, in the installed @redis/client@6.2.1:

• client/socket.js #shouldReconnect — defaultReconnectStrategy answers false for SocketTimeoutError (the rejecting shape you correctly identify), so it sets #isOpen = false, then calls this.emit('error', cause), then the caller throws it.
• client/index.js:637-638 — the client subscribes to the socket and re-emits every socket error on itself: .on('error', err => { this.emit('error', err); … }).
• redis-backend.ts:23 — createClient({ url }), and no 'error' listener is ever attached.

An EventEmitter emit('error') with no listener throws the error as an uncaught exception. void attempt.catch(…) cannot intercept that — it is a promise handler, and this is an event. So on the one failure shape this change exists to survive, the process exits 1 before any later turn can call open() and re-arm. That is the same exit code 1 signature described in the comment.

redis-backend-rearm.test.ts cannot catch it: its mock is a plain object ({ connect, quit, keys, isOpen }), not an EventEmitter, so it never emits. The bookkeeping is proven; the crash is invisible.

Fix is one line in this constructor — this.client.on('error', …) (log-and-swallow is enough; the promise still carries the real rejection to callers). Emission is async, so placement relative to this.arm() does not matter.

Worth doing repo-wide while this is open: none of the five long-lived stores register one — pool-records.ts:27, sandbox-lease.ts:46, leaf-result-store.ts:91, work-queue/queue.ts:37. This PR memoizes two of them to process lifetime, so an idle-time Redis restart or CLIENT KILL now crashes a worker with no in-flight turn to blame — a failure mode that did not exist when clients were per-turn.

[pdettori]

Fixed — all five clients now register it (swallowRedisErrors, @sh/session-backend; work-queue keeps a local copy rather than depending on the session store for eight lines), and redis-error-listener.test.ts in each package uses a real EventEmitter so the emit is actually exercised.

The conclusion holds and the fix does more than swallow: I probed it with CLIENT KILL against the probe's own connection on the pinned 6.2.1 — no listener exits on an uncaught SocketClosedUnexpectedlyError, one listener handles it and node-redis reconnects on its own (isOpen stays true). So the listener is what enables the built-in recovery.

The mechanism you traced is not the one that bites, though, and it is worth being exact because three comments in this repo now repeat the wrong version. On a failed initial connect the socket does emit and the client does re-emit — but that emit happens inside the awaited connect() chain, so the throw becomes that promise's rejection. Probed both shapes against a dead port and a black-holed address: connect() rejects, nothing is uncaught, the process survives. So void attempt.catch(…) did cover that channel; what it cannot cover is an error raised from a socket event handler after the connection is established, which is #onSocketError and which is exactly what killed the supervisor worker and the E11 relay.

Two related corrections while I was in there: connectTimeout raises ConnectionTimeoutError, not SocketTimeoutError (SocketTimeoutError comes from socketTimeout, on an established socket), and a refused connect rejects rather than retrying forever. The arm() doc, sharedSessionStore's note and the rearm test all claimed otherwise.

[cwiklik]

must-fix — the hoist puts the pool lease before the session-existence check, so 503 now shadows 404.

Base order was: open store → openFromCheckpoint (throws no session in backend → 404) → then resolveTurnSandbox, which took no lease. Acquiring here inverts that: acquireTurnSandbox runs before executeTurnCore opens the session at all.

Two consequences:

1. A /turn whose session does not exist — a documented first-class outcome, and exactly what a client resuming after Redis expiry hits — now does a pod list, N lease.load() calls, and an acquire/release against the lease ZSET before returning 404. Real pool work for a request guaranteed to fail.

2. Worse: when the pool is saturated or empty, that request returns 503 (retryable) instead of 404 session_not_found (permanent). turnErrorStatus tests NO_CAPACITY first, but it never gets the chance to choose — the capacity error is thrown before the 404 can be raised. The caller is told to retry a session that will never exist. In records mode with nothing attached yet, that is every /turn.

This contradicts two claims added or preserved in this same diff: server.ts:175's "preserve /turn's 404-on-missing-session contract", and the SSE comment's "a bad sessionId still returns real 404 JSON, byte-identical to the sync path (§3.4 regime 2)". Both are now conditional on pool capacity — and §3.4 regime 2 parity holds only because both paths are equally wrong.

The fix is structural rather than a line move: open the session in executeTurn before acquiring (passing the manager into executeTurnCore), so the lease still lives in the same finally. That is why it seems worth settling now rather than after the hoist lands.

[pdettori]

Fixed structurally, as you suggested: the session open is now openTurnSession, called from executeTurn before the acquire, handing its store/backend/manager to executeTurnCore. The lease stays in executeTurn, so its finally still covers normal return, throw and abort.

turn-session-before-lease.test.ts arms both failures at once and asserts the 404 is what surfaces, plus a second case asserting the selection seam is never reached — that one is the one that would have caught this.

One consequence worth naming: turn-lease-run-id.test.ts reached selectPoolSandbox precisely because nothing opened a session first, so it now stands up an inert session to get there. That is the ordering being load-bearing rather than incidental.

[cwiklik]

suggestion — Number() on an env var whose bad values become a 1 ms Redis write loop.

?? does not catch an empty string, so KAGENTI_SANDBOX_HEARTBEAT_MS= yields Number('') === 0, and any unparseable value (20s, abc) yields NaN. setInterval clamps both to 1 ms — roughly 1000 lease renewals per second per in-flight turn, against the Redis this PR exists to stop overloading. A self-inflicted flood from a single empty env var.

This file already knows the distinction: the Boolean() comment on leased exists precisely because an empty-string selector is not undefined, and there is a dedicated test for it. And server.ts:44 records the team already being burned by this exact class of bug (emits "Retry-After: NaN"; negatives rejected for the same reason) — intEnv is the established answer.

Separately, nothing relates hbMs to ttlMs. The defaults (20 s / 60 s) are safe, but the two knobs are independent, so KAGENTI_SANDBOX_LEASE_TTL_MS=15000 expires the lease before its first renewal — the sandbox returns to the pool mid-turn and another turn can take the same pod past cap. Worth a clamp, or at least a comment stating the ordering the defaults rely on.

[pdettori]

Fixed, and both halves. leaseTimings (harness/src/lease-timings.ts) is now the one reader: empty, unparseable, zero and negative all fall back to the default instead of becoming a 1 ms interval, and the heartbeat is clamped to ttl/3 so KAGENTI_SANDBOX_LEASE_TTL_MS=15000 can no longer expire a lease before its first renewal.

The clamp is exactly the default pairing (60000/3 = 20000), so a deployment overriding neither is byte-for-byte unaffected — that seemed better than documenting the ordering the defaults rely on, since nothing enforced it.

I applied it to all four lease-taking paths rather than just this one. /turn and the three leaf calls inlined the same Number(env.X ?? …), and hardening only this site would have created precisely the divergence the "same knobs, deliberately" comment warns about.

[cwiklik]

suggestion — in records mode this blames a pool selector no pod was ever matched against.

pods is forced to [] two hunks up, so an empty sh:sandbox:records throws no Running pods for pool selector '…'. That is precisely the misdirection resolveDiscoverySource's own guard was added to prevent — "Blame the flag, not the pool … which sends them debugging a healthy pool" — violated one function later.

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.

The new test at select-sandbox.test.ts:398 is titled "records with an empty record set reports the pool, not a kubectl error" but asserts no Running pods for pool selector 'app=sbx' — so it pins the wording rather than fixing it. Passing source into SandboxPoolEmptyError and phrasing per-source would fix both; the class is new here, so the log-grep-compatibility argument in its comment only binds the pods path.

(I raised this same message on #250 before it was SandboxPoolEmptyError — it lands in code this PR now owns.)

[pdettori]

Fixed — SandboxPoolEmptyError takes the source and phrases per source. In records mode it now reads no sandbox presence records (SH_SANDBOX_DISCOVERY=records — no sandbox has attached to the relay yet), with the selector dropped entirely rather than reworded: in that mode it is only a gate (if (!selector)), never something a pod was matched against.

The pods wording is untouched, so the log-grep argument in the class comment still binds where it applies. And the mistitled test is corrected rather than kept — it now asserts the records phrasing, with a second case pinning the pods wording for pods and both so the greppable half cannot drift either.

[cwiklik]

suggestion — leased is re-derived from env rather than reported by the seam it describes.

SelectedSandbox already hands back heartbeat/release, but the no-selector branch (select-sandbox.ts:76-79) returns no-op closures that are indistinguishable from real ones — so the caller genuinely cannot tell whether a lease was taken, and has to re-evaluate selectPoolSandbox's branch condition itself.

The comment is doing the work an interface should: "this flag must agree with that exactly". It agrees today, and the empty-string test pins that. But the predicate now lives in two files, and a future change to the branch (trimming whitespace, say — resolveDiscoverySource already trims its own value) makes them disagree silently, arming or skipping a renewal timer against the truth.

One leased: boolean on SelectedSandbox set at each return site removes the duplication, the comment, and the test that guards it — the same "correct by construction rather than by care" argument the no-pool path makes for itself.

[pdettori]

Fixed — SelectedSandbox.leased is set at each return site (false on the no-selector branch, true after a successful acquire) and acquireTurnSandbox reads it. The env re-derivation and the comment arguing for its correctness are both gone.

The empty-selector test stays, but now as a property of the seam rather than of a duplicated predicate, and there is a new case in select-sandbox.test.ts asserting the flag directly on both branches.

[cwiklik]

suggestion — 503 here adopts /runs as precedent, but neither half of what /runs actually does.

The comment justifies 503 with "/runs already treats saturation this way (it bounded-waits then 503s)". /runs does two things this path does not: it waits up to KAGENTI_SYNC_SATURATION_WAIT_MS (default 30 s) with backoff, and it advertises Retry-After (server.ts:422, KAGENTI_SYNC_SATURATION_RETRY_AFTER_S).

So at the saturation knee /turn fails immediately where /runs transparently succeeds, and a client that honours Retry-After gets no hint at all from the route that tells it to retry. Given the reasoning above it is explicitly about not letting one signal mean two things depending on the route, the header at least seems worth carrying over — saturationWaitConfig().retryAfterS is already there.

That asymmetry is also measurement-relevant: E8 reads exactly the region where the two routes now diverge.

[pdettori]

Fixed for the header: turnErrorHeaders(status) adds Retry-After on the 503s from saturationWaitConfig().retryAfterS, so both routes read the same knob, and both the sync and SSE pre-first-frame paths go through it.

I did not carry over the bounded wait, and the doc comment now says why rather than leaving the asymmetry implicit. On /runs it is sound because selectPoolSandbox throws before taking a lease or doing agent work, so re-running runLeaf only re-attempts acquisition. On /turn, after the ordering fix in this same commit, the session is already open by the time the acquire runs, so re-entering executeTurn to retry would re-open it. That is a real difference in what a retry costs, not a gap — happy to add the wait as a follow-up if you would rather the two routes match at the knee, since as you say E8 reads exactly that region.