[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.

[pdettori]

Following up on my own reply here: the one-line version turned out to be wrong, and #251 caught it independently while this PR was open.

Adding the listener silences the error that used to make a failed connect() reject — so with node-redis's default unbounded strategy the attempt retries forever and connect() never settles. Probed against a dead port on 6.2.1: no listener rejects in ~1 ms, listener-only still pending at 6 s, listener + a bounded reconnectStrategy rejects in ~210 ms with ReconnectStrategyError.

That hang is worse than the crash, and worst for RedisSessionBackend: arm() re-arms by catching that rejection, so a promise that never settles disables the retry rather than delaying a turn — every caller awaits forever, with nothing logged.

The listener and a bounded reconnect now ship as a pair (resilientClientOptions, @sh/session-backend) at the four sites this branch owns: session store, sandbox leases, leaf results, and the work queue (inline copy, as before, rather than depending on the session store). pool-records.ts keeps #251's own inline pairing untouched — same reasoning, same probe, already complete — so the repo has one shared helper and one deliberate inline copy rather than me rewriting code that just landed.

Verified on the real class: list() against a dead Redis rejects in ~5.5 s with ReconnectStrategyError, and a second list() makes a fresh attempt, so the re-arm still fires. Pinned in redis-resilient-client.test.ts (number under the bound, Error past it, URL named in that error, capped backoff, blip still inside the bound).

Branch is rebased onto a9e010d and CI is green (13/13).

[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.

[cwiklik]

Re-review of 2398419 — 23 files, +1788/−86. All five of my prior must-fixes are fixed, and fixed at the right level rather than patched at the symptom. Six of six suggestions are addressed or declined with a reason I accept. One new blocker, introduced by the fixes themselves.

The five blockers, verified against this head

Prior must-fix	Status	How
M1 lease runId must be unique per turn (run-turn.ts:559)	Fixed	turnRunId() = ${sessionId ?? 'anon'}:${randomUUID()}. The session id is now prefix-only, greppability, and the doc comment spells out both consequences of the old form (cap undercounts; the first turn to finish releases a sibling's pod). turn-lease-run-id.test.ts pins it.
M2 shared session store with no drop guard, permanently unrecoverable connect (run-turn.ts:87)	Fixed at source	RedisSessionBackend.arm()/open() re-arm: a rejected attempt clears the memo so the next call reconnects. Fixing the class rather than wrapping the memo is the better call — it covers every caller, not just this one. See the new finding, which is about the other channel.
M3 drop() leaks the connection and can evict a healthy store (select-sandbox.ts:52)	Fixed, both halves	dropMemo identity-checks (if (read()?.store !== store) return;) and closes. Clear-then-close ordering matches #249's turn-auth.ts.
M4 the re-arm can't run in production because no store registers an 'error' listener (redis-backend.ts:29)	Fixed	swallowRedisErrors on all five long-lived clients (session store, lease, leaf result, work queue; pool-records.ts already had its own from #251), with four listener tests.
M5 the lease hoist puts 503 ahead of 404 (run-turn.ts:615)	Fixed	openTurnSession split out and run before acquireTurnSandbox; the comment states the ordering as the whole fix. turn-session-before-lease.test.ts pins it.

Suggestions: empty pool now 503 not 500 (SandboxPoolEmptyError); Retry-After carried from /runs' own knob; leaseTimings replaces four inlined Number(env.X ?? …) and adds the ttl/3 clamp nothing enforced before; leased now reported by SelectedSandbox instead of re-derived; the records-mode message no longer blames a pool selector. Declining /runs' bounded wait is right — on /turn the session is already open by the time the acquire runs, so re-entering executeTurn would re-open it. Stating the asymmetry beats quietly matching the precedent.

The new blocker

resilientClientOptions' bound is the right fix for the initial connect and the wrong one for the reconnect, and it is one strategy for both. Past the bound node-redis sets #isOpen = false permanently (socket.js:153-162) and, on the post-open path, swallows its own ReconnectStrategyError (socket.js:333, .catch(() => { /* silently ignore */ })). Every later command then rejects ClientClosedError forever (index.js:1127). Nothing re-arms that state: arm() fires on a rejected connect, and after a successful one ready is a resolved promise. So a Redis outage longer than ~5.5 s permanently disables the session store — and the supervisor clears healthy only on 'exit' (pool.ts:411), so it keeps routing turns into the wedged worker. A pod restart or rollout routinely exceeds 5.5 s.

That is a regression against both alternatives: base crashed and the supervisor restarted it (loud, self-healing), and listener-only would retry forever and heal when Redis returned. Details and a two-line fix inline.

Verified rather than taken on trust

• The re-arm's load-bearing assumption still holds under the new strategy. I checked this specifically because the bound changed the terminal path my earlier verification covered: #shouldReconnect sets #isOpen = false before returning the Error (socket.js:154), so a second connect() clears connect()'s own Socket already opened guard and genuinely re-attempts. If that were false the whole re-arm would be inert.
• The #250 overlap resolved itself. #250 merged 2026-09-15 and this PR rebased onto it — resolveDiscoverySource is now only called here, not added. mergeable: MERGEABLE; BLOCKED is my own prior review, nothing else.
• The reorder does not orphan a session on a 503. A leaf injects its sandbox, so acquireTurnSandbox returns early and cannot throw saturation; /turn binds createIfAbsent: false. The only path that can throw is the one that never creates a session.
• turnErrorStatus' ordering is safe. 503 is checked before the no session in backend substring, and neither pool error's message contains it.
• sharedLease / sharedRecords are not exposed to the new finding — guard() → dropMemo evicts on a rejected command, so the next call builds a fresh client. That asymmetry is what makes the session store the interesting one.

My earlier stated limitation is now moot: the local clone is current with main at 6634c27, so the base-tree and node-redis citations above are re-verified rather than read from a five-day-old tree.

---

Author: pdettori (MEMBER — maintainer)
Areas reviewed: TypeScript (harness, knative-server, session-backend, work-queue), tests, plus @redis/client@6.2.1 internals (socket.js, index.js)
Agent/IDE config (.claude/, .vscode/): none — supply-chain gate clear, both +++ b/ and rename to forms grepped, 0 renames
Commits: 5, all signed off (5/5), conventional prefixes
CI status: passing (13/13 green on 2398419)
Scope: over the 500-line threshold (2313 lines), so this stays on correctness and design. One nit included because it is the residue of a must-fix I raised.

[cwiklik]

must-fix — this bound is right for the initial connect and wrong for the reconnect, and it is one strategy for both. Past it, a lost socket is permanent and silent.

The reasoning for bounding is correct and I verified the probe's conclusion. The problem is that reconnectStrategy is consulted on two paths that want opposite answers, and only one of them has something to re-arm it.

What happens past the bound, on the post-open path (redis@6.2.1):

1. #shouldReconnect sees an Error return, sets #isOpen = false, emits 'error' (now swallowed), returns ReconnectStrategyError — socket.js:153-162.
2. On the post-open path that error goes nowhere: #onSocketError calls this.#connect().catch(() => { /* the error was already emitted, silently ignore it */ }) — socket.js:333. No promise any caller holds ever sees it.
3. #isOpen stays false, so every subsequent command rejects ClientClosedError — index.js:1127. Forever; node-redis has given up.

Nothing re-arms that state. arm() clears ready when the connect promise rejects; after a connect that succeeded, ready is a resolved promise and open() returns it unchanged. isOpen is read nowhere in the workspace except the new close(). So:

• Redis goes away for longer than the bound (0+100+…+900+1000 = ~5.5 s of delays, plus attempt overhead).
• The session store is dead for the life of the process. sharedSessionStore memoises it process-wide and — correctly, per your reply to M2 — has no drop guard.
• pool.ts:411 clears healthy only on handle.on('exit'). The worker is alive, so it stays healthy: true and the supervisor keeps routing turns to it. All four workers hit the same Redis event, so all four wedge together and stay wedged.

A pod restart, a rollout, an image pull, or docker restart redis on the VM arm routinely exceeds 5.5 s. This is the most likely Redis event in production, not an exotic one.

It is worse than both alternatives, which is what makes it a blocker rather than a wash:

	outcome of a 60 s Redis outage
base (no listener)	process exits 1, supervisor restarts with backoff — loud, self-heals
listener only, default strategy	retries forever, heals when Redis returns
listener + this bound	client permanently closed, worker alive and still receiving turns, never heals

The file's own framing has this exactly right — "a wedge in place of a crash" is the thing to avoid — it just applies it to the promise channel only. And sharedSessionStore's note ("a socket lost AFTER connecting is the event channel, and that one is fatal without an 'error' listener") stops one step early: with the listener it is no longer fatal to the process, but with the bound it becomes fatal to the client, and nothing rebuilds the client. Worth noting the CLIENT KILL probe cannot distinguish these — it recovers either way, because Redis came back inside the bound.

The two-line fix is on redis-backend.ts:79; RedisWorkQueue and RedisResultStore need the same treatment, and sharedLease/sharedRecords already recover via guard(). Bounding only the first connect would also work (retries > max || !everConnected), but the isOpen check is simpler and needs no extra state.

[pdettori]

Fixed in 3e9c85f. This is the gap, and it hides on the channel you named.

Confirmed against the installed @redis/client@6.2.1 rather than from memory:

• #shouldReconnect sets #isOpen = false before returning the ReconnectStrategyError (socket.js:153-162).
• #onSocketError then calls #connect() with a .catch whose entire body is the comment "the error was already emitted, silently ignore it" (socket.js:333-335) — so past the bound the error reaches nobody.
• Every later command rejects ClientClosedError (index.js:1127), for the life of the process.

arm()'s .catch cannot see any of that, because the connect it re-arms on succeeded — ready stays resolved. So open() now re-arms on !client.isOpen as well:

private open(): Promise<void> {
  if (this.ready && !this.client.isOpen) this.ready = null;
  return (this.ready ??= this.arm());
}

isOpen is the precise signal and not a proxy: node-redis keeps it TRUE for the whole retry sequence (only isReady drops while reconnecting), and false only on the terminal give-up or an explicit close. Re-arming on it is safe because connect() throws Socket already opened only when isOpen is true — which that branch excludes — and the terminal path clears it before returning the error, so the re-attempt genuinely happens.

Scope came out as you scoped it. RedisWorkQueue and RedisResultStore had neither half of the recovery (no re-arm on a rejected connect either), so both got the same arm()/open() pair. sharedLease/sharedRecords are exempt: select-sandbox's guard() evicts the memo on any rejected command, so the ClientClosedError rebuilds the client there by itself. I kept the two-line open() per class with one pointer to the full rationale on resilientClientOptions rather than extracting a shared connect-memo class — this PR is large enough already.

Two things worth flagging:

1. It broke an existing green test, and that was the fix working. The mocks hard-coded isOpen: false and never flipped it, so open() re-armed on every call. In redis-error-listener.test.ts that silently consumed the one-shot rejection in "does not swallow the failure a caller is awaiting", and the call succeeded instead of rejecting. Three fakes now model node-redis — isOpen true synchronously on connect (socket.js:170), cleared on rejection (socket.js:154). The synchronous part is load-bearing: the "no re-arm while a connect is in flight" case has three concurrent callers reaching open() before connect resolves.
2. New tests, each verified to fail without the fix — I reverted the fix and watched them go red, so they pin the bug rather than merely passing: a terminal-close case in redis-backend-rearm.test.ts, and a new queue-connect-rearm.test.ts covering both channels for the queue (3 of its 5 cases fail on the pre-fix source).

Also fixed while here, unasked: close() in both newly-armed classes awaited ready unguarded, so a never-connected client could not be closed at all — the same bug redis-backend.ts:150 already fixed. Making ready nullable forces close() to handle null anyway, so it came along rather than being left as a known-broken sibling.

[cwiklik]

must-fix (same finding as redis-errors.ts:52, this is where the fix goes) — open() recovers the promise channel but not the event channel.

ready is only ever cleared by arm()'s .catch, so once a connect has succeeded this returns a resolved promise forever — including after node-redis has permanently closed the socket past the reconnect bound. await this.open() then resolves and this.client.incr(...) rejects ClientClosedError, every call, for the life of the process.

isOpen is a precise signal for that state and nothing else: node-redis keeps #isOpen true for the whole retry sequence (only #isReady drops while reconnecting) and sets it false only on the terminal give-up (socket.js:154) or an explicit close. So:

/** Await the live connect attempt, starting a fresh one if the last one failed OR the socket was permanently closed. */
private open(): Promise<void> {
  // node-redis gives up past resilientClientOptions' bound: #isOpen goes false, it swallows its own
  // ReconnectStrategyError on the post-open path (socket.js #onSocketError), and every later command
  // rejects ClientClosedError. `ready` is resolved from the successful connect, so nothing here
  // notices. Re-arming on !isOpen covers the event channel the way arm()'s catch covers the promise one.
  if (this.ready && !this.client.isOpen) this.ready = null;
  return (this.ready ??= this.arm());
}

This is safe on the paths that matter: arm() calls connect(), which only throws Socket already opened when #isOpen is true — the case this branch excludes — and I confirmed the terminal path clears #isOpen before returning the Error, so the re-attempt genuinely happens.

One test would pin it, in the shape redis-error-listener.test.ts already uses: flip client.isOpen to false after a successful list(), call list() again, assert connect was called twice. Both new test files miss this today — redis-backend-rearm.test.ts never flips isOpen after success, and redis-error-listener.test.ts asserts only that the emit does not throw, which is the process surviving, not the client recovering.

[pdettori]

Fixed in 3e9c85f. This is the same finding as the redis-errors.ts thread, so the full verification (the socket.js/index.js citations, why isOpen is the precise signal, and the test surgery it forced) is in the reply there rather than duplicated here — this note covers what is specific to this call site.

open() in this file is now:

private open(): Promise<void> {
  if (this.ready && !this.client.isOpen) this.ready = null;
  return (this.ready ??= this.arm());
}

RedisWorkQueue and RedisResultStore got the same pair, since neither had a re-arm on either channel.

One consequence specific to this class, called out in the comment rather than left to be discovered: a call after close() now reconnects instead of rejecting, because close() leaves isOpen false with ready resolved. I did not add a closed flag for it. The only closers are sharedSessionStore's URL-change branch and resetSharedSessionStore() (test-only), and both discard the store immediately — so the flag would guard a path that cannot be reached with a live reference. Happy to add one if you would rather have the invariant enforced structurally than argued from callers.

[cwiklik]

suggestion — the ~210 ms figure is the refused-port case; the case this same file calls the one to expect in a cluster is ~300x slower.

~210 ms is quoted here, in redis-backend.ts's arm() note, in redis-resilient-client.test.ts's header, and in queue.ts — so it will be read as the fail-fast guarantee. It holds for a refused connect, where ECONNREFUSED returns immediately and only the strategy's own delays accumulate.

For a black-holed SYN it does not, and arm()'s comment names that as the shape to expect ("a Service with no ready endpoints, or a NetworkPolicy drop, black-holes the SYN rather than refusing it"). Each attempt then costs the full connectTimeout before it fails — #createSocket arms socket.setTimeout(this.#connectTimeout) and destroys with ConnectionTimeoutError (socket.js:278-284), default 5000 ms, and resilientClientOptions does not override it. Eleven attempts at ~5 s plus ~5.5 s of delays is ~60 s before connect() rejects, not 210 ms.

That is not a correctness bug — it still rejects, arm() still re-arms, and it beats hanging forever. But every /turn awaiting open() blocks for that minute, then the next call starts another one, so a black-holed Redis serves ~60 s-latency failures instead of fast ones. Adding connectTimeout beside the strategy would make the number match the prose:

socket: {
  connectTimeout: 1000, // else a black-holed SYN costs 5s per attempt: ~60s to reject, not ~210ms
  reconnectStrategy: …
}

Either that or a clause on the 210 ms — "~210 ms against a refused port; a black-holed SYN pays connectTimeout per attempt" — so the next reader does not size a timeout against it.

[pdettori]

Taken as the documentation half, in 3e9c85f — the prose clause in all four places that quoted ~210 ms, and no connectTimeout change. You offered that as an acceptable alternative, and it is the half I am confident is right.

Your arithmetic checks out. Verified on the installed 6.2.1: socket.js:45 is #connectTimeout = options?.connectTimeout ?? 5000, resilientClientOptions does not override it, and #createSocket arms socket.setTimeout and destroys with ConnectionTimeoutError (socket.js:278-284). So 11 attempts × 5 s plus ~5.5 s of accumulated delays is ~60 s to reject, not 210 ms. The ~210 ms figure was measured against a refused port, where ECONNREFUSED returns immediately and only the backoff delays accumulate — which makes it the least representative case for a cluster, exactly as you say. That distinction is now stated wherever the number appears, so nobody sizes a timeout against it.

Why I did not lower connectTimeout: it would convert a slow, loud, eventually-correct failure into a permanent one for a Redis that is merely slow to accept. With connectTimeout: 1000, any Redis taking >1 s to complete a connection — cold start, TLS handshake, cross-AZ DNS — fails every attempt on the timeout, and with 11 attempts the bound then exhausts on timeouts alone. The client would never connect at all, and the failure would look identical to a genuinely absent Redis. The current shape already has the property that matters: bounded, loud, and re-armed, so a caller fails soon rather than hanging, and the next call retries. ~60 s is a bad number to wait, but it is a correct one, and it is bounded.

If the ~60 s worst case is itself the concern, the lever I would reach for is the attempt count rather than the per-attempt timeout — it shortens the black-holed case without penalising a slow-but-healthy connect. resilientClientOptions already takes maxReconnectAttempts as a parameter (currently a test seam, defaulting to 10), so that is a one-line change if you want it. I have left it alone here because it trades away transient-blip tolerance, and that tolerance is what this pairing was added for; it seems worth deciding deliberately rather than folding into a review fix.

[cwiklik]

nit — the 1 ms renewal loop is still reachable, through the other knob.

intEnv's floor for ttlMs is the default min = 1, so KAGENTI_SANDBOX_LEASE_TTL_MS=1 parses to 1, and Math.max(1, Math.min(requested, Math.floor(1 / 3))) is Math.max(1, 0) = 1. Same for anything under 3 ms. That is exactly the ~1000 renewals/second this module exists to prevent, arrived at from the ttl side rather than the heartbeat side.

The important half is fixed — empty and unparseable were the values that actually reach production, and leaseTimings({KAGENTI_SANDBOX_HEARTBEAT_MS: ''}) now yields 20000. Nobody sets a 1 ms lease TTL on purpose. But the floor costs one argument, and it closes the shape rather than one instance of it:

const ttlMs = intEnv(env, 'KAGENTI_SANDBOX_LEASE_TTL_MS', 60000, 1000);

A sub-second lease TTL cannot serve a turn anyway, so a floor there loses nothing real. lease-timings.test.ts's it.each table is the natural place for a KAGENTI_SANDBOX_LEASE_TTL_MS: '1' row.

[pdettori]

Fixed in 3e9c85f. Reproduced first: KAGENTI_SANDBOX_LEASE_TTL_MS=1 passes the min = 1 floor, so intEnv returns 1, and heartbeatMs = max(1, min(20000, floor(1/3) = 0)) = 1 ms — a ~1000-writes-per-second renewal loop, which is the exact failure this module was added to prevent. The hardening was one-sided: the heartbeat knob fell back to a default rather than produce that loop, while the TTL knob could still produce it through the clamp.

const ttlMs = intEnv(env, 'KAGENTI_SANDBOX_LEASE_TTL_MS', 60000, 1000);

A sub-second lease cannot serve a turn at any heartbeat — it expires before the first renewal — so the floor costs nothing real. The deployed value in leaf-scaledjob.yaml is 60000, so no manifest changes meaning.

One wrinkle worth surfacing, because it means this was slightly more than a nit: an existing test pinned the behaviour you asked me to remove —

it('never clamps the heartbeat below 1 ms even for an absurdly short TTL', () => {
  expect(leaseTimings({ KAGENTI_SANDBOX_LEASE_TTL_MS: '1' }).heartbeatMs).toBe(1);
});

so the floor could not just be added alongside it. That case was documenting the clamp's own max(1, ...) as the last line of defence, which is precisely the one-sidedness above. It is now replaced by a table asserting the fallback ('1' and '999' → ttl 60000, heartbeat 20000), plus an inclusive-boundary case ('1000' → ttl 1000, heartbeat 333) so the floor cannot silently widen into values an operator might actually set. The comment records what the old assertion was for, so the change reads as deliberate rather than as a test bent to fit.

[cwiklik]

Round 4. All four round-3 findings are genuinely fixed, and I verified the load-bearing library claims behind them directly against the pinned @redis/client@6.2.1 rather than taking the comments' word for it:

• redis-errors.ts:77 / redis-backend.ts:98 (the two must-fixes, one defect) — open() now re-arms on !client.isOpen in RedisSessionBackend, RedisWorkQueue and RedisResultStore. The mechanism checks out: #shouldReconnect clears #isOpen before returning ReconnectStrategyError (client/socket.js:140-165), #connect()'s retries counter is local so the bound resets per invocation and the re-attempt genuinely happens, and connect() sets isOpen synchronously (trace() invokes its callback synchronously, client/tracing.js:89-96) — which is what makes the "no re-arm while a connect is in flight" assertions real rather than artefacts of a fake stuck at false. Both fakes model that faithfully.
• redis-errors.ts:26 (suggestion) — taken as the documentation half; the ~210 ms figure now carries the refused-vs-black-holed distinction in all four places it appears, with a reasoned refusal to lower connectTimeout. Good outcome.
• lease-timings.ts:50 (nit) — the min = 1000 floor landed exactly as suggested, and the test that pinned the old heartbeatMs === 1 behaviour was replaced with an it.each table plus an inclusive-boundary case rather than deleted. Also confirmed no reader of the three lease env knobs was missed.

One new must-fix, sitting in the new commits' blast radius rather than in them directly: the three leaf heartbeat intervals in run-leaf.ts still void lease.heartbeat() with no .catch(). The reconnect bound this PR adds converts commands that used to stay queued during an outage into ones that reject, and guard()/dropMemo makes that permanent for a captured closure even after Redis recovers — so each setInterval tick becomes a fresh unhandled rejection and exits the leaf. run-turn.ts:261 in this same diff guards the identical pattern and explains exactly why; packages/supervisor/src/main.ts:81 documents the same failure class. Three .catch(() => {})es, or the arm()/open() treatment on RedisLeaseStore (comment 2) — either resolves it.

Also checked and clean: no hidden skips or stray .only/.skip in any of the ten new/changed test files, and the new fakes do fail against the pre-fix code; close()'s only non-test callers (packages/knative-server/src/leaf-job.ts:85-101) each process.exit() immediately, so the close-then-resurrect path waved off earlier is genuinely unreachable there; no .claude/ or .vscode/ paths in the diff.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: TypeScript/Node (harness, session-backend, work-queue, knative-server), tests
Agent/IDE config (.claude/.vscode): none
Commits: 6 commits, all signed-off: yes
CI status: passing (13/13 checks)

[cwiklik]

must-fix — this void lease.heartbeat() has no .catch(), so a rejected renewal becomes an unhandled rejection and Node (23.x, default --unhandled-rejections=throw) exits the leaf.

The sibling path in this same PR guards it, with the reasoning already spelled out — run-turn.ts:259:

// Best-effort: a failed renewal must not reject into an unhandled rejection and kill the
// worker. The lease's TTL expiring is the safe outcome — the sandbox returns to the pool.
void acquired.heartbeat().catch(() => {});

and packages/supervisor/src/main.ts:81 documents the same failure class in prose ("a bare void declines to handle it, and Node's default unhandledRejection mode throws -- ending the process").

What makes it newly reachable is this PR, on two counts:

1. The bound converts pending into rejected. Before, RedisLeaseStore used node-redis's default unbounded strategy, so a command issued during an outage stayed queued and eventually resolved. Past resilientClientOptions' bound node-redis gives up permanently, isOpen goes false, and every later command rejects ClientClosedError. That is the same "the listener and the bound are one fix" argument redis-errors.ts makes, extended one step: the bound also makes every unguarded void on a Redis-backed promise a process-killer.
2. guard() makes it permanent rather than transient. selected.heartbeat closes over the wrapper sharedLease() returned, which is bound to one store instance. The first rejection runs dropMemo, which closes that store and nulls the memo — but the closure still points at it, and dropMemo's own identity check then makes every later drop a no-op. So every subsequent tick rejects too, even after Redis has fully recovered, and each tick is a fresh unhandled rejection.

Three sites, all on lines this PR touches: here (428), realProduceSolve (617), and realProduceVerdict (755). The run-turn.ts form verbatim at each would do it:

Suggested change

	void lease.heartbeat();
	void lease.heartbeat().catch(() => {});

[pdettori]

Fixed at all three sites (428, 617, 755) with the run-turn.ts form verbatim, comment included.

Also fixed the half you named in comment 2, because the .catch()es alone would have left the functional damage in place: the renewal stops, the TTL lapses, and the pod goes back in the pool while the turn is still executing in it — over-subscribing the cap ACQUIRE_LUA enforces. Your point 2 is exactly right that guard()/dropMemo is what makes that permanent, and that is where it is fixed. See the reply on comment 2.

Guard pinned structurally in harness/test/heartbeat-rejection-guard.test.ts: every void …heartbeat(…) in run-leaf.ts and run-turn.ts must carry a .catch, the count is pinned at 3+1 so a fourth interval has to be considered rather than merged, and there is a proof-of-detectability case.

[cwiklik]

suggestion — RedisLeaseStore is now the only long-lived Redis client in the repo that took the listener + bounded-reconnect pairing without the arm()/open() re-arm its three siblings got in this PR (RedisSessionBackend, RedisWorkQueue, RedisResultStore). ready is still assigned once here and never reassigned.

I checked the exemption redis-errors.ts claims for it, and it does hold on its own terms: sharedLease() wraps all four LeaseStore methods in guard(), so a ClientClosedError drops the memo and the next selectPoolSandbox builds a fresh store. No objection to that reasoning.

What the exemption does not cover is a caller that holds a closure instead of re-entering sharedLease() — which is exactly the heartbeat/release pair selectPoolSandbox returns, and the mechanism that makes the unhandled rejection in run-leaf.ts permanent rather than transient. Giving this class the same arm()/open() treatment would make those closures self-healing and remove the asymmetry, and it is arguably the better place to fix that than three .catch()es. Either fix resolves the must-fix; both would be better than either.

[pdettori]

Taken, but implemented one layer up — and the difference is load-bearing, so it needs your eyes.

Your diagnosis is precisely right: the gap is a caller holding a closure instead of re-entering sharedLease(). What I could not make work is the arm()/open() re-arm itself, because this store differs from its three siblings in one respect that the re-arm depends on. RedisSessionBackend, RedisWorkQueue and RedisResultStore are memoised for the process's life and never closed on any path that keeps using them, so reconnecting in place is free. dropMemo closes this one on purpose. And node-redis will happily reopen a closed client — probed against the pinned redis@6.2.1:

connected isOpen= true
after close isOpen= false
RECONNECT OK isOpen= true ping= PONG

So an in-place re-arm would make the closure work by resurrecting the store dropMemo just closed. That store is in no memo, so nothing will ever close it again, and dropMemo's identity check turns every later drop into a no-op — one orphaned connection per outage, in the file whose whole purpose is keeping connections off the maxclients ceiling.

What landed instead: sharedLease's wrapper resolves the memo per command rather than binding one store when the wrapper is built. A captured closure then re-enters exactly as a fresh sharedLease() caller would — self-healing, no resurrection — and the same change is applied to sharedRecords for symmetry. It also upgrades the exemption redis-errors.ts claims for these two from "true if the caller re-enters" to unconditional, which is the asymmetry you were pointing at; that note now says so, including why re-arming in place would be wrong here rather than merely redundant.

harness/test/lease-closure-rearm.test.ts pins both halves against a fake that models the property that makes the bug reachable (once closed, every command rejects ClientClosedError for good): the captured heartbeat/release recover onto the fresh store with the same pod and runId, and the dropped store receives nothing further and stays closed. All three cases fail against the pre-fix code.

One extra, adjacent and by the same asymmetry: RedisLeaseStore.close() opened with a bare await this.ready, so on the store this is most often called for — the one whose connect rejected, which is the commonest trigger for dropMemo — it re-threw and never reached client.close(). No leak, and I want to be accurate about that: a client past the reconnect bound has isOpen: false with its socket already destroyed, and node-redis's close() rejects ClientClosedError on it either way (probed). It is consistency plus a trap removed — both current callers happen to swallow, and one that awaited would fail exactly on the path teardown exists for. Now the shape RedisRecordStore.close() and RedisSessionBackend.close() already use, pinned by harness/test/sandbox-lease-resilience.test.ts.

[cwiklik]

nit — "Eleven attempts" is twelve. retries > maxReconnectAttempts returns a delay for retries 0..10, so eleven delays are scheduled and the strategy errors at retries === 11 — i.e. after the 12th attempt.

The other two figures in this same sentence both agree with 12 rather than 11: the delays are min(retries * 100, 1000) for 0..10, which sums to the 5500 ms you state, and 12 × 5 s connectTimeout is the ~60 s you quote (11 would be ~55 s). Only the count needs changing — the conclusion is right.

[pdettori]

Fixed — "Twelve attempts".

Correcting the count made me re-derive the other two figures in that sentence rather than trust them, and the ~210 ms is wrong for the default bound. Its own arithmetic gives it away: eleven delays summing to 5500 ms cannot reject in 210 ms. Measured against a refused port on the pinned redis@6.2.1:

refused port (max=0):  rejected in 4 ms    as ReconnectStrategyError
refused port (max=1):  rejected in 105 ms  as ReconnectStrategyError
refused port (max=10): rejected in 5537 ms as ReconnectStrategyError

So ~210 ms was a probe at a reduced bound — the maxReconnectAttempts test seam — written up as if it described production. The default refused-port cost is ~5.5 s, exactly the sum of min(retries * 100, 1000) over 0..10, and 5537 ms measured. Independently confirmed by the new RedisLeaseStore.close() test, which pays the real backoff because that class takes no seam: 5557 ms.

Corrected in all four places, keeping the refused-vs-black-holed distinction you asked for in the round-3 suggestion and noting explicitly which figure belongs to the seam, so the next reader does not re-derive this. The ~60 s black-holed figure and the reasoned refusal to lower connectTimeout are unchanged.

[cwiklik]

nit — promptTimings inside realProduceVerdict; realProduceSolve names its equivalent solveTimings. Presumably copy-paste — verdictTimings would match.

Also minor, and the reason it is worth one line rather than none: this path hoists the object here but then re-reads it at the setInterval (leaseTimings(process.env).heartbeatMs, line 753) instead of using promptTimings.heartbeatMs. Same shape in runPromptLeaf and realProduceSolve. It is cheap and deterministic so nothing is wrong today, but the two reads can disagree if the env is mutated between them, and each path already has the object it needs.

[pdettori]

Both fixed. realProduceVerdict's object is verdictTimings, and all three leaf paths now use the object they already hoisted at the setInterval instead of re-reading leaseTimings(process.env).

runPromptLeaf needed one extra move: its read was inside the try that wraps selectPoolSandbox, while the interval is armed in a later block, so destructuring heartbeatMs there did not typecheck. Hoisted to a promptTimings object above the try — same shape as the other two paths, and the name is now the honest one for that path. Comment records why it is read once.

[pdettori]

Round 4 addressed in 78ab380 — the must-fix, both nits, and the suggestion, which turned out to be the right fix for the must-fix rather than an alternative to it.

must-fix (run-leaf.ts:428) — .catch(() => {}) at all three sites with the run-turn.ts form and comment. Pinned structurally (heartbeat-rejection-guard.test.ts): every voided heartbeat must carry a .catch, the count is pinned so a fourth interval has to be considered, and there is a proof-of-detectability case.

suggestion (sandbox-lease.ts:52) — done, one layer up, and this is the part worth your attention. The arm()/open() re-arm is wrong for this class, unlike its three siblings: dropMemo closes this store on purpose, and close() then connect() reopens a node-redis client (probed on the pinned 6.2.1 — isOpen: true, PONG), so an in-place re-arm resurrects a store no memo references and nothing will ever close. Instead sharedLease — and sharedRecords — now resolve the memo per command, so a captured closure re-enters as a fresh caller would. Self-healing without the orphan, and the exemption redis-errors.ts claims for these two becomes unconditional rather than caller-dependent. Your point 2 in the must-fix was the real damage here: it was not only a repeated unhandled rejection but a lease that could not be renewed for the rest of the turn, so the pod returned to the pool with a turn still running in it.

nit (redis-errors.ts:33) — "Twelve attempts". Re-deriving the count rather than patching the word exposed a worse error beside it: the ~210 ms is a reduced-bound (test-seam) probe, not the default. Measured — max=0 → 4 ms, max=1 → 105 ms, max=10 → 5537 ms, which is the 5.5 s of delays the same sentence already computed. Corrected in all four places, with which figure belongs to the seam now stated.

nit (run-leaf.ts:725) — verdictTimings; all three paths use the hoisted object at the setInterval.

Also, by the same asymmetry: RedisLeaseStore.close() re-threw on the store whose connect rejected — the commonest dropMemo trigger — and so never reached client.close(). Not a leak (that client is already closed either way, probed), but a teardown that rejects on the failure path is a trap for the next caller. Now the shape both siblings use.

Three new test files, 12 new cases, all shown failing against the pre-fix code. Full sweep green locally: make typecheck, pnpm -r test (1652 passed / 19 skipped), make lint, make fmt clean.

[cwiklik]

Round 5. All four round-4 findings are fixed, and the must-fix was fixed twice over — the three .catch()es and the underlying closure defect that comment 2 was pointing at. I traced the mechanisms rather than taking the descriptions:

• must-fix (run-leaf.ts 428/617/755) — all three are now void …heartbeat().catch(() => {}) carrying the run-turn.ts reasoning.
• The better fix landed, and declining arm()/open() is correct. In the new sharedLease, call() resolves leaseStore(url) per invocation and builds drop around that invocation's store, so: tick N rejects → dropMemo identity-matches the live memo → clears it and closes the store → tick N+1 finds leaseMemo === null and builds a healthy one. The captured closure genuinely self-heals, which is what the three cases in lease-closure-rearm.test.ts pin, including the negative one. And the reason for not re-arming in place holds: dropMemo closes this store deliberately, so reconnecting it would leave a live client in no memo and nothing to ever close it — an orphan per outage, in the file that exists to keep connections off maxclients. That asymmetry against the three siblings is real, and it is now documented at the point where redis-errors.ts claims the exemption, which is where the round-4 comment said the gap was.
• nit — "Eleven attempts" — now "twelve", and correcting it surfaced the larger error I had missed: the ~210 ms was a reduced-bound probe written up as production. The replacement is arithmetically sound — min(retries * 100, 1000) over retries 0..10 sums to exactly 5500 ms against 5537 ms measured, and the max=0 → 4 ms / max=1 → 105 ms ladder falls out of the same formula. Corrected in all four places, distinction intact.
• nit — promptTimings — now verdictTimings, and all three leaf paths use the object they already hoisted. Hoisting runPromptLeaf's above the try is the right call given the interval is armed in a later block, and the comment says why.
• RedisLeaseStore.close() (volunteered) — a genuine trap removed, and the "nothing leaks" caveat is stated accurately rather than oversold.

Two suggestions, both about the durability of the new guards rather than the code they guard. Neither blocks.

Separately, and blocking on mechanics rather than on review: this no longer merges. mergeable: false, rebaseable: false, and main has moved on harness/src/run-leaf.ts, harness/src/sandbox-lease.ts, harness/src/select-sandbox.ts and harness/test/select-sandbox.test.ts — every file this round rewrote. Worth resolving deliberately rather than mechanically: the per-command memo resolution is a small diff with load-bearing semantics, and it is the easiest thing in this PR to lose to a resolution that keeps the older-looking const store = leaseMemo.store form and its single bound drop. The three cases in lease-closure-rearm.test.ts should catch that if it happens, which is a good argument for re-running them after the merge rather than trusting a clean git status.

Also checked and clean: no .claude/ or .vscode/ paths anywhere in the PR, including this commit's ten files; the corrected timing figures are consistent across all four call sites; and the new fakes model the property that makes the bug reachable (once closed, every command rejects for good) rather than a convenient one.

Author: pdettori (MEMBER — maintainer)
Areas reviewed: TypeScript/Node (harness, session-backend, work-queue), tests
Agent/IDE config (.claude/.vscode): none
Commits: 7 commits, all signed-off: yes
CI status: passing (13/13 checks)

[cwiklik]

suggestion — the structural pin is the right instinct and the proof-of-detectability case is better than most, but the regex requires the void prefix and nothing in this repo enforces it.

There is no ESLint config in the tree at all: lint is prettier + hadolint + shellcheck + gitleaks, and root package.json has no lint tooling in devDependencies (only fmt/fmt:check). So @typescript-eslint/no-floating-promises is not what makes void appear at these sites — convention is. That leaves the most natural way to write the regression invisible to the pin:

heartbeat = setInterval(() => {
  lease.heartbeat();          // no `void`, no `.catch` — same unhandled rejection, same dead leaf
}, promptTimings.heartbeatMs);

voidedHeartbeats does not match that, so the .catch loop never sees it and the count assertion still reads 3. Both tests stay green on precisely the bug this file exists to catch — and tsc will not flag it either.

Anchoring at line start closes it without loosening anything, because await … / return … forms then cannot match, and those are the two that legitimately have a caller to handle the rejection:

Suggested change

	return body.match(/void\s+[\w.]*heartbeat\([^;]*;/g) ?? [];
	/** Every statement-position `heartbeat(...)` call, `void`ed or bare — both are fire-and-forget. */
	function voidedHeartbeats(body: string): string[] {
	// Anchored at line start, so `await`/`return` forms do not match: those have a caller. A bare
	// `lease.heartbeat();` does not, and there is no ESLint `no-floating-promises` here to force the
	// `void` marker the original pattern relied on.
	return body.match(/^[ \t]*(?:void\s+)?[\w.]*heartbeat\([^;]*;/gm) ?? [];
	}

That still matches all four current sites, so the 3-and-1 counts hold unchanged.

Second, smaller: the file list is ['run-leaf.ts', 'run-turn.ts'], so a renewal interval in a new module is uncovered — which your own note anticipates ("the next Redis-backed void need not come from a lease at all"). Reading ../src and checking every .ts would cost one readdirSync and make the pin track the hazard instead of today's two files.

[pdettori]

Both taken, and widening the scan paid for itself immediately.

The void requirement. Verified your claim rather than assuming it: no ESLint config anywhere in the tree, and root devDependencies is {"prettier": "3.9.6"} with only fmt/fmt:check scripts — so nothing enforces the marker. Confirmed the hole directly:

bare call — old regex: []            -> INVISIBLE
bare call — new regex: ["        lease.heartbeat();"]
await/return/decl/property — new regex: []

Anchored form landed as suggested.

The file list. Now readdirSync over every .ts in ../src, and it caught a false positive on its first run that two hard-coded files could not have surfaced: LeaseStore’s own interface signature matches the anchor — no async, at line start, terminated by ;.

sandbox-lease.ts: heartbeat(pod: string, holderId: string, ttlMs: number): Promise<void>;

Excluded by shape rather than filename — a ): Type; tail, which no call form has — so the next declaration added is excluded too without editing the test. My earlier probe had covered the class-method form (async heartbeat(…) {) but not the bare interface one, which is exactly the gap the directory scan closed.

The count assertion is now a map ({ 'run-leaf.ts': 3, 'run-turn.ts': 1 }) so a failure names the file that moved, and a renewal added in a new module fails it rather than passing unseen. Detectability cases now cover both unguarded forms plus all five legitimate ones (await, return, class method, property, interface signature).

[cwiklik]

suggestion — you flagged this yourself in the round-4 reply (5557 ms, "pays the real backoff because that class takes no seam"), so this is a nudge to close it rather than a new observation.

RedisLeaseStore is the one of the four long-lived clients that takes no maxReconnectAttempts, and the cost lands here: a ~5.5 s wall-clock unit test, on every CI run and every local vitest, for an assertion that is really about close()'s two lines and not about backoff timing. The generous 20 s timeout is the symptom.

resilientClientOptions(url, maxReconnectAttempts?) already has the seam and redis-resilient-client.test.ts uses it — threading an optional second constructor arg through RedisLeaseStore the way its siblings do takes this to milliseconds. It also removes the one place where a test's runtime is coupled to the reconnect ladder you just re-measured: retune the bound later and this test's duration moves with it, silently.

[pdettori]

Closed. RedisLeaseStore takes maxReconnectAttempts now, third positionally so the existing (url) and (url, now) callers are untouched, forwarded to resilientClientOptions — where an omitted argument keeps the default 10.

5557 ms → 21 ms. The test also asserts its own budget (< 2000 ms) so the seam cannot quietly stop being honoured, which is the other half of your point: without that, a future change that drops the argument restores the slow version and nothing fails. It also decouples the runtime from the ladder — the figure I just re-measured — so retuning the bound no longer moves this test.

[pdettori]

Rebased onto main (4b00eb1) — MERGEABLE. Both suggestions addressed.

Flagging this ahead of the merge, because the rebase exposed a defect rather than merely conflicting, and it changes behaviour after your approval.

What the #279 rebase surfaced

Textually the conflict was one hunk. Semantically main's runId → sessionId rename collided with M1 from round 3: the lease member must be unique per turn, so a per-turn value was now flowing into a parameter named sessionId — and selectPoolSandbox feeds that same argument to the microVM workspace_key.

That is wrong, and not just nominally. WorkspaceRoot/<workspace_key> is created on the first Exec for an unseen key and lives until an idle Reclaim (P4 design §4.4), so keying it per turn opens turn 2 of a session in an empty workspace and gives it its own standby VM pool (§4.3) — cross-turn continuity lost, standbys multiplied per turn instead of per session.

It got past round 3 and round 4 because /turn never had a workspace_key before this PR — it did not call selectPoolSandbox at all — so there was no previous behaviour for either of us to compare against. The rename is what made the collision visible by naming the parameter.

The resolution

Two identities, both real, no longer one value:

	keys	must be
sessionId	microVM workspace (workspace_key)	stable across a session's turns
opts.holderId	the lease's ZSET member	unique per concurrent holder

holderId defaults to the session id, so every leaf path is unchanged by construction — a leaf is one session executing once. acquireTurnSandbox derives the holder itself rather than accepting one, so the M1 expression cannot be got wrong at a call site again. An anonymous turn keys its workspace by its unique holder: no session means no continuity to preserve, a shared 'anon' workspace would be §2.3's cross-turn bleed, and an empty key is refused outright by microvm-worker (§3.4). turnRunId → turnLeaseHolder, descriptive per docs/glossary.md rather than the banned noun — #279 was right that the old lease runId duplicated session_id; this value is the counter-example that makes the identity real.

New pins: turn-lease-holder.test.ts (both directions — same session id across two turns, distinct holders), and a selectPoolSandbox case asserting the workspace key stays the session id when the holder differs.

Suggestions

1. Heartbeat guard — verified the ESLint claim (none in the tree; root devDeps is prettier alone) and the hole (lease.heartbeat(); was invisible to both the .catch loop and the count). Anchored form landed; scan widened to every .ts in harness/src, which on its first run caught a false positive two hard-coded files could not have shown — LeaseStore's interface signature matches the anchor. Excluded by shape (): Type;), not filename.
2. Reconnect seam — maxReconnectAttempts threaded through RedisLeaseStore, third positionally. 5557 ms → 21 ms, and the test asserts its own budget so the seam cannot silently stop being honoured.

Verification

Re-ran after the merge rather than trusting a clean git status, as you asked: lease-closure-rearm.test.ts's three cases pass, so the per-command memo resolution survived the resolution intact. make typecheck, pnpm -r test (1655 passed / 19 skipped), make lint, make fmt all clean.

One note for the record: on the first post-rebase run config-bundle's checkMemoryLinks ReDoS ratio assertion failed (4.66 vs < 3) while my container VM was still starting. It passes 3/3 in isolation and that package is untouched by this PR — pre-existing load-sensitivity in a timing ratio, not a regression here, but worth knowing it can flake on a loaded runner.