fix(worker): reserve outbound capacity for terminal frames (#173 item 1)

[pdettori]

#173 item 1 — the top correctness item on that issue. Does not close #173: items 2, 4, 9 and 10 remain.

The bug

A busy: queue full refusal could be dropped outright, leaving its caller with no answer at all.

accept runs on the recv goroutine, which must never block — an Abort queued behind a stalled dispatch is exactly what would free the pool — so it sends through the non-blocking trySend. But every frame accept sends is terminal, refusals are never cached, and outbound was shared with the chunk stream of every running exec. Chunks win by volume, so the drop correlated with the overload that produced it.

The comment deferring the fix justified it as "survivable only because the harness timeout is dual-ended" — written when the relay ceiling was 120 s. #182 replaced that with DEFAULT_EXEC_TIMEOUT_S = 30 * 60, so the stall it excused had grown 15×, under exactly the condition that causes it. The deferral was correct when written; the value it depended on changed underneath it.

Reserved capacity, not a priority lane

outbound keeps one channel and gains TerminalReserve (QueueCap/4 = 16) slots. A chunkSlots semaphore holds chunk-class frames to QueueCap residency, so those last slots are reachable only from the recv goroutine.

Why not the remedy the old comment named ("a priority channel, or a dedicated terminal-frame forwarder"): spec §8 requires Chunk* then End per req_id. A priority lane lets a cache replay overtake the original's still-queued chunks, or a colliding refusal overtake an earlier exec's frames under the same id — the harness settles the exec on whichever arrives first and discards the real output behind it. Making that safe needs per-req_id pending tracking. Reserved capacity needs none: one FIFO channel cannot reorder.

Two details that are deliberate:

• The sender releases a slot when it takes a non-reserved frame, not after Send. The semaphore then measures residency in the channel rather than depending on Send's latency.
• Frames carry the accounting they were admitted under (outFrame.reserved) instead of it being re-derived from frame type — which would break silently the day a kind is sent from both paths.

Exhaustion is a different condition, handled as one

The reserve is a probability argument, not a proof: nothing bounds how many refusals arrive between two drains. But with chunk producers fenced off, failing to place a terminal frame no longer means "busy" — it means nothing is draining at all. So accept returns ErrEgressWedged, recvLoop propagates it, and Serve returns it for main.go to re-dial, with the dedup cache answering redeliveries (spec §5, §6.2). Seconds instead of half an hour.

A dropped cache-hit replay escalates too. It reads as harmless — "a duplicate of a frame already delivered once" — but the harness redelivers precisely because it never got the first answer.

A teardown wedge that predates this

enqueue now gives up if connCtx is done. A Send that blocks (rather than fails) parks the sender, so a producer waiting for room waited forever and wg.Wait() never reached zero — meaning Serve could never return, which is the only thing that makes main.go re-dial. TestSendFailureDoesNotWedgeTeardown misses this because a failing Send lets the sender keep draining.

Scope stated plainly: Serve still cannot return while Send is blocked forever, because teardown ends at wgSender.Wait(). In production that is bounded by gRPC's keepalive, not by this package. The new test says so rather than implying otherwise.

Tests

Three, each watched failing first and each checked to discriminate rather than merely pass:

Test	Pins
TestBusyRefusalSurvivesAChunkBacklog	The defect, plus FIFO — the refusal must arrive after the chunks queued before it
TestExhaustedReserveEndsTheSession	The session ends on its own initiative; the stream is deliberately never closed
TestBlockedSendDoesNotWedgeProducers	Verified red by making the semaphore acquire uncancellable

Worth flagging: the first test passed vacuously three times before it was made honest — a gate predicate disabled by a > 0 guard, a saturation check that fired on a 5 ms scheduling hiccup, and releasing the gate before recvLoop had consumed the flood. Each would have shipped a test that proved nothing. Saturation is now proven by chunk delivery stalling at a floor of QueueCap+1, not by a sleep.

Verification

go test -race ./... · go vet · gofmt -l · make test (whole repo) · make typecheck · make lint (9/9 hooks) — all green. No temporary fault-injection code survives (grepped).

No wire or proto change: §8 is enforced, not amended, so no spec §8 entry or ADR-0024 revision is owed. accept gains an error return, which is not #173 item 4's plumbing refactor — that stays open, and per the triage note is best folded into whichever change next touches this file.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>

[pdettori]

Verdict: APPROVE — posted as a comment because GitHub does not allow approving your own pull request.

Verified the load-bearing claims rather than taking the comments at their word.

The reserve invariant holds. A chunk-class frame acquires its chunkSlots token before it enters the channel, and the token is released when the sender dequeues it — so resident chunk frames are <= QueueCap at every instant. With cap = QueueCap+TerminalReserve, trySend can therefore only fail once >= TerminalReserve terminal frames are themselves resident and undrained. "Wedged, not busy" is a proven distinction here, not an asserted one.

The semaphore is balanced on all four paths (acquire->enqueue, acquire->abandon+hand back, ctx-done-before-acquire, sender release) — no token leak. The <-chunkSlots in the abandon path cannot block, since the caller's own admission is by construction still outstanding.

Teardown does not regress into a panic or a new deadlock. Every new blocking wait has a connCtx.Done() escape, so wg.Wait() still completes before close(outbound) and the ordering is unchanged. cancel() precedes every new return ErrEgressWedged, so no contexts leak.

The re-dial/dedup argument is real, not aspirational. main.go creates one Session outside the re-dial loop, and runOne calls cache.Put before send(frame) — so a frame abandoned during teardown is still answerable on redelivery. Serve returns the error bare, so errors.Is works and main.go treats it like any other stream end.

Two non-blocking notes inline. Single signed-off commit, all 12 checks green, no dependency/CI/config surface touched.

Assisted-By: Claude Code

[pdettori]

suggestion — this sleep is doing more work than its comment admits. If it expires before the last accept reaches its trySend, the gate is released, the sender drains, and the refusal then lands in a channel with room — i.e. the test passes against the unfixed code, which is exactly the third vacuity mode the PR description calls out.

The FIFO assertion below does not backstop it: in that racy ordering the sender has already drained >= QueueCap chunks before the refusal is enqueued, so chunksBefore >= session.QueueCap still holds and that check passes too. Every other synchronisation point in this test was deliberately converted to an observed condition; this one remains a timing bet on a loaded runner.

Options, cheapest first: give it a much larger margin and say in the comment that it is the sole vacuity guard, or expose a test-only counter of completed accept calls so the wait becomes observable like the others.

Separately: settle() (line 193) is this exact sleep, if an inline one is kept.

[pdettori]

Fixed in 26c689b — replaced with an observed condition, and you were right that the FIFO check was no backstop: in the racy ordering the sender has already drained >= QueueCap chunks, so chunksBefore >= session.QueueCap holds and that assertion passes too.

Took neither option verbatim, because a third one avoids both the timing bet and a test-only hook in the session. recvLoop is a single goroutine alternating Recv and dispatch, so ENTERING Recv for the (N+1)th time proves dispatch of the Nth frame returned. fakeStream now counts Recv entries and the test waits on recvEntries() >= pushed+1. Every synchronisation point in the test is now observed rather than timed, and the session gains nothing test-only.

Re-verified the rewritten test still catches the original defect, since a green test proves nothing on its own: with the reserve removed it fails. Worth noting it now fails AT THE BARRIER rather than at the refusal assertion — escalation ends the session mid-flood once the reserve cannot absorb the refusals — so the barrier's message names that cause explicitly, since that message is what a maintainer will actually read.

On settle(): agreed it is the same sleep. No inline sleep remains here, so nothing to share. Its other uses are #173 item 9 ("an exported test hook would let it become a condition wait"), still open — and the Recv-entry trick may be the cheaper answer there too, since it needs no exported hook at all.

[pdettori]

nit — this degrades silently rather than loudly: at QueueCap < 4 it evaluates to 0, the reserve disappears, and trySend then fails on any full buffer — turning a transient chunk backlog into a dropped connection, which is a worse failure than the one being fixed here.

Given this file already guards against a future maintainer's mistake elsewhere (the outFrame.reserved rationale exists precisely for "the day a frame kind is sent from both paths"), a one-line compile-time assertion would fit the same ethos:

// TerminalReserve must be >= 1, or the reserve silently vanishes.
var _ [TerminalReserve - 1]struct{}

[pdettori]

Added in 26c689b, with your assertion verbatim. Confirmed it actually bites rather than just looking like it does: temporarily changing the divisor to 128 fails the build with

internal/session/loop.go:51:8: invalid array length TerminalReserve - 1 (untyped int constant -1)

Also expanded the const comment to say WHY, since "reserve must be >= 1" on its own does not convey that the failure mode is worse than the bug being fixed — a vanished reserve turns every transient chunk backlog into a dropped connection.