fix(transport): derive the gRPC message ceiling from the output cap (#173 item 2) by pdettori · Pull Request #233 · rossoctl/serverless-harness

pdettori · 2026-09-08T13:53:07Z

Addresses #173 item 2. No closing keyword and not linked to #173 — item 4 is in flight as #232, and PR-linking is the likely cause of the erroneous auto-closes after #229/#230. Independent of #232: different files, either can land first.

The item's severity claim is wrong, and tracing it changed the fix

Item 2 says an oversized base64 write "kills the whole stream rather than one exec, triggering a reconnect."

It doesn't. ExecRequest is {sandbox_id, exec}, and relay.ts forwards {exec: {reqId, command, stdin, timeoutS, streaming}} with no sandbox_id — so the frame the worker receives is strictly smaller than the request that carried it in. With both limits at the same 4 MiB default, the relay's ingress always trips first and the Attach stream never sees the payload. Blast radius is one exec, RESOURCE_EXHAUSTED.

The real defect is a read/write asymmetry the item never mentions

	Mechanism	Ceiling
read	`cat` → raw stdout	`DEFAULT_OUTPUT_CAP` = 8 MiB
write	content → base64 (×4/3) → `stdin`	4 MiB gRPC ⇒ ~3 MiB of file

The relay path could read a file it could not write back. Files between ~3 and 8 MiB read fine and failed to write — and createPodEditOps composes read with write, so Pi's Edit on a 4 MiB file succeeded at reading and then failed. KubectlTransport pipes base64 through kubectl exec stdin with no ceiling, so the identical write succeeded on the pod path: the backend-dependent divergence #180, #182 and #185 were each closed on.

The fix, and why the number isn't a judgement call

16 MiB on both receive limits, derived as DEFAULT_OUTPUT_CAP × BASE64_INFLATION (≈10.7 MiB) plus command and framing headroom — so write capacity ≥ read capacity by construction.

MAX_EXEC_MESSAGE_BYTES (transport.ts) and session.MaxRecvMsgBytes (dial.go) are pinned equal by message-size-coupling.test.ts, which also asserts the derivation — so raising the output cap alone cannot silently restore the asymmetry.

Send limits are untouched: grpc-js defaults max_send_message_length to -1, grpc-go defaults MaxCallSendMsgSize to MaxInt32.

Equality, not a floor — this is the load-bearing detail

Raising only the relay (the hop that visibly rejects today) would forward the payload and move the rejection onto the worker's Attach stream, whose death takes every concurrent and queued exec with it and forces a re-dial. That is the failure the item wrongly claimed already existed — fixing the relay alone would have created it.

TestContractAcceptsAnOversizedWritePayload fails if the worker is left at the default. It was watched failing with the exact error, and I checked the reason rather than accepting a timeout:

SERVE RETURNED: rpc error: code = ResourceExhausted desc =
  grpc: received message larger than max (5592440 vs. 4194304)

Serve returning is the stream death. The test then asserts the payload arrived whole — the command decodes and counts its own stdin — not merely that the stream stayed up.

"Needs a decision on both ends" was illusory

That framing kept this item untouched for a month. Once the asymmetry is the frame, the number follows from DEFAULT_OUTPUT_CAP and needs no cross-team agreement.

One structural change

The worker's dial options moved into session.DialOptions (keepalive included, comment carried with it) so the contract tests dial exactly as main.go does. An option production doesn't apply is indistinguishable from an absent one — which is why one of the three coupling assertions greps main.go for that call rather than trusting the constant's existence.

All three guards were checked to discriminate, each against a different mistake: shrinking the Go constant (equality fires), shrinking both below the read cap (derivation fires), and reverting main.go to a hand-rolled dial (application fires).

Docs

Spec §8 gains the derivation and the both-ends ordering. ADR-0024 gets an append-only Revisions entry recording the corrected severity. The issue body and a comment carry the correction too, so nobody re-scopes from the wrong claim.

Verification

go test -race ./... · go vet · gofmt · make test (whole repo) · make typecheck (0 errors) · make lint (9/9).

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

…ossoctl#173 item 2) Both ends sat at gRPC's 4 MiB receive default, which is smaller than this contract's own write path needs. THE ITEM'S SEVERITY CLAIM IS WRONG, and tracing it changed the fix. It says an oversized write "kills the whole stream rather than one exec, triggering a reconnect". It does not: ExecRequest is {sandbox_id, exec} and relay.ts forwards {exec: {...}} WITHOUT sandbox_id, so the frame the worker receives is strictly smaller than the request that carried it in. With both limits equal, the relay's ingress always trips first and the Attach stream never sees the payload — one exec fails with RESOURCE_EXHAUSTED. THE REAL DEFECT IS A READ/WRITE ASYMMETRY the item never mentions. Read is capped at DEFAULT_OUTPUT_CAP (8 MiB); a write costs 4/3 of the file as base64 in Exec.stdin, so ~3 MiB was the write ceiling. Files between ~3 and 8 MiB were READABLE BUT NOT WRITABLE, and Pi's Edit composes read with write — so editing one succeeded at reading and then failed. KubectlTransport pipes base64 through kubectl exec stdin with no ceiling, so the same write succeeded on the pod path: the backend-dependent divergence rossoctl#180/rossoctl#182/rossoctl#185 were each closed on. The ceiling is 16 MiB, DERIVED as DEFAULT_OUTPUT_CAP x 4/3 plus command and framing headroom, so write capacity >= read capacity by construction. MAX_EXEC_MESSAGE_BYTES (transport.ts) and session.MaxRecvMsgBytes (dial.go) are pinned EQUAL by message-size-coupling.test.ts, which also asserts the derivation so raising the output cap alone cannot restore the asymmetry. Equality, not a floor, and that is the load-bearing detail. Raising only the relay — the hop that visibly rejects today — would forward the payload and move the rejection onto the worker's Attach stream, killing every concurrent and queued exec and forcing a re-dial. That is the failure the item wrongly claimed already existed, so fixing the relay alone would have created it. TestContractAcceptsAnOversizedWritePayload fails if the worker is left at the default; it was watched failing with the exact error, "ResourceExhausted: received message larger than max (5592440 vs. 4194304)", and Serve returning — the stream death itself. "Needs a decision on both ends plus a documented max write size" is what kept this item untouched for a month. Once the asymmetry is the frame, the number follows from DEFAULT_OUTPUT_CAP and needs no cross-team agreement. The worker's dial options moved into session.DialOptions, keepalive included, so the contract tests dial exactly as main.go does. An option production does not apply is indistinguishable from an absent one, which is why one of the three coupling assertions greps main.go for that call rather than trusting the constant's existence. All three were checked to discriminate, each against a different mistake: shrinking the Go constant, shrinking both below the read cap, and reverting main.go to a hand-rolled dial. Send limits are untouched — grpc-js defaults max_send_message_length to -1 and grpc-go defaults MaxCallSendMsgSize to MaxInt32. Spec §8 gains the derivation and the both-ends ordering; ADR-0024 gets an append-only Revisions entry recording the corrected severity. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>

pdettori

Reviewed the derivation against source rather than taking the body's word for it, and the load-bearing claims hold:

The severity correction is right. relay.ts:132 writes {exec: {reqId, command, stdin, timeoutS, streaming}} with no sandbox_id, so the worker-bound frame really is strictly smaller than the ExecRequest that carried it — the relay's ingress does trip first, and the blast radius is one exec.
The asymmetry is real and not merely arithmetic. ChunkSize = 32 * 1024 (internal/exec/runner.go:24), and buffered non-streaming output is re-sliced to it at exit rather than emitted as one BufferCap frame (runner.go:282). So the read path never presents a frame to any receive limit, while a write presents the whole base64 payload as one — which is exactly why 8 MiB read and ~3 MiB write could coexist.
Send limits genuinely need no change. defaultExecClient constructs new SandboxExecClient(addr, credentials.createInsecure()) with no channel options (harness/src/select-sandbox.ts:57), so the harness egress rides grpc-js's -1 default.
The quoted error corroborates the framing accounting. base64(4 MiB) is 5,592,408 bytes and gRPC reported 5,592,440 — the 32-byte delta is the command plus protobuf framing, which is the same headroom term the 16 MiB figure is claimed to cover.
Both coupling regexes match their targets, and \s* tolerates a gofmt reflow of the main.go call.

Three non-blocking notes below, all on the guards rather than the fix. No must-fix findings; COMMENT rather than APPROVE only because GitHub refuses self-approval.

Assisted-By: Claude Code

pdettori · 2026-09-08T14:39:07Z

+    // between ~3 and 8 MiB was readable but not writable — and Pi's Edit composes read
+    // with write. Asserting the RELATIONSHIP rather than the literal means bumping the
+    // output cap alone cannot silently reintroduce the asymmetry.
+    expect(MAX_EXEC_MESSAGE_BYTES).toBeGreaterThan(DEFAULT_OUTPUT_CAP * BASE64_INFLATION);


suggestion — this guard is a bare floor, so it does not defend the part of the constant's doc comment that carries the argument: "16 MiB covers the inflated cap with room for the command string and protobuf framing". MAX_EXEC_MESSAGE_BYTES = 11184811 would satisfy this assertion while leaving zero headroom for either.

And the floor is very slightly too low to be the true minimum: DEFAULT_OUTPUT_CAP * BASE64_INFLATION is 11,184,810.67, whereas base64 of 8 MiB is 4*ceil(N/3) = 11,184,812 bytes. The asserted bound sits ~1.3 bytes below the smallest payload it exists to admit — invisible at 16 MiB, but it means the assertion does not actually certify "write capacity >= read capacity" at its own boundary.

Asserting the margin the comment claims would close both:

expect(MAX_EXEC_MESSAGE_BYTES).toBeGreaterThan(DEFAULT_OUTPUT_CAP * BASE64_INFLATION + 64 * 1024);

Fixed in 01e91d0 — and the arithmetic half of this is the sharpest finding in either PR. Confirmed both claims before touching anything:

DEFAULT_OUTPUT_CAP = 8388608 N * 4/3 (my bound) = 11184810.666666666 4*ceil(N/3) (real b64) = 11184812 my bound is below real by 1.333... bytes MAX = 11184811 -> passes my guard: True, actually sufficient: False

So the guard did not certify "write capacity >= read capacity" at exactly the boundary it was written for. Verified by mutation too, not just arithmetic: a ceiling of 11184811 now fails.

I fixed the root cause rather than taking the suggested + 64 * 1024 on the ratio. The ratio is the defect — adding a margin would have left a bound that is wrong by 1.33 bytes and merely far enough from the boundary to hide it. BASE64_INFLATION is gone, replaced by an exact base64EncodedLength() (4·⌈n/3⌉), and since that function is now load-bearing it is pinned against Node's own encoder over seven sizes rather than trusted as a formula.

Your headroom point stands on its own and is also fixed: EXEC_FRAMING_HEADROOM = 64 * 1024, asserted rather than claimed in prose. It is measured, not picked — the RED error showed 5592440 against a 5592408-byte payload, a 32-byte delta, the same term you spotted independently. 64 KiB is three orders of magnitude above that, which is deliberate: the command string is the only unbounded term and nothing in the contract caps it.

Mutation-checked both ways: 11184811 fails (the old guard's blind spot) and 11184812 — exact length, zero headroom — also fails, which is the case a bare floor admitted while contradicting the constant's own documented derivation.

pdettori · 2026-09-08T14:39:07Z

+  // here. MAX_EXEC_MESSAGE_BYTES is shared with the Go worker's session.MaxRecvMsgBytes
+  // and pinned equal to it — a relay that accepts more than the worker would forward a
+  // payload the worker refuses on its Attach stream, killing every exec on it (#173 item 2).
+  const server = new Server({ 'grpc.max_receive_message_length': MAX_EXEC_MESSAGE_BYTES });


suggestion — worst-case ingress buffering on the relay rises 4x here, and that consequence is not recorded anywhere in the spec §8 or ADR addition.

This repo does document these budgets when it moves a ceiling: internal/exec/runner.go:41 states its as a formula (2 streams x session.MaxConcurrent x BufferCap), and the earlier output-cap revision in ADR-0024 pins the two caps equal explicitly for "memory parity". A one-line note of the same kind on the raised ingress limit would keep that convention intact.

No correctness objection — the number is derived, and 16 MiB per in-flight ExecRequest is unremarkable next to the existing BufferCap budget. It is the absence of the note, not the size, that stands out against the rest of the change.

Fair, and the convention argument is the right one — runner.go's 2 × MaxConcurrent × BufferCap and the 2026-08-28 revision's memory-parity note are exactly the precedent. Added in 01e91d0, in three places: the constant's doc comment, spec §8, and the ADR entry.

Stated as a formula, concurrently decoding ExecRequests × MAX_EXEC_MESSAGE_BYTES, with two qualifications I did not want to leave implicit:

It is a transient, not a residency. routeExec forwards exec onto the Attach stream and keeps no copy, so the peak is however many oversized requests are mid-decode — not however many execs are in flight, which is what the BufferCap formula measures. Writing it in that form without the distinction would have overstated it.

It sizes the relay; it does not bound it. Nothing in the contract caps the count — the relay is single-replica and serves every harness replica — so I recorded it as a ceiling to size against rather than a proof. Claiming a bound I have not derived would be the same overstatement this repo has been caught on before.

For scale, and to make the 4× rise judgeable rather than just stated: one such request is a quarter of the worker's own 64 MiB budget, and only a write near the read cap reaches it.

pdettori · 2026-09-08T14:39:07Z

+    // A constant nothing dials with is indistinguishable from no constant. main.go must
+    // reach the wire through session.DialOptions, which is where the limit is attached
+    // and what the contract tests exercise.
+    expect(readFileSync(MAIN_GO, 'utf8')).toMatch(


nit — this pins main.go's local variable name relayAddr, not just the call through session.DialOptions. Renaming that variable makes the test report "is actually applied by the worker" as a failure while the option is in fact applied — a false negative on the one assertion whose whole purpose is to distinguish applied from merely defined.

readMaxRecvMsgBytes above guards against exactly this class of drift with a loud throw that names the cause; this assertion just fails a toMatch. Loosening to /grpc\.NewClient\([^)]*session\.DialOptions\(/ (or simply /session\.DialOptions\(/) still discriminates the mistake it targets — reverting main.go to a hand-rolled option list.

Right, and it is the worst failure mode for this particular assertion: a false negative on the one guard whose entire job is telling applied from merely defined. Fixed in 01e91d0 with your first form, /grpc\.NewClient\([^)]*session\.DialOptions\(/, and mutation-checked in both directions — renaming relayAddr to addr now passes, and reverting main.go to a hand-rolled option list still fails.

I did not take the simpler /session\.DialOptions\(/. It is too permissive in the other direction: main.go's own comment names the function, so that pattern is one edit away from matching prose rather than the call — a false positive on the same assertion, which is the more dangerous polarity here.

Also took the loud-throw point, since you are right that the two guards in this file were inconsistent about it. It now throws naming both candidate causes rather than just the alarming one:

main.go does not dial through session.DialOptions — either the receive limit is no longer applied in production (the defect this guards), or the dial was restructured and this assertion needs updating. Check which before "fixing" it.

That second clause is the part worth having. A bare toMatch failure reads as "production is broken" when the likelier truth is that the dial moved, and the tempting response to a scary-looking failure is to loosen the guard.

…budget note, robust guard Three non-blocking suggestions, all on the guards rather than the fix. Each verified before changing anything; the first was sharper than it looked. THE FLOOR WAS DERIVED FROM AN APPROXIMATION AND FAILED AT ITS OWN BOUNDARY. DEFAULT_OUTPUT_CAP * (4/3) is 11184810.67, but base64 of 8 MiB is 4*ceil(N/3) = 11184812 — so the asserted bound sat 1.33 bytes BELOW the smallest payload it existed to admit. Confirmed by arithmetic and then by mutation: a ceiling of 11184811 passed the old guard while being too small to carry an 8 MiB file, which means the assertion did not certify "write capacity >= read capacity" at exactly the point it was written for. BASE64_INFLATION is replaced by an exact base64EncodedLength(), pinned against Node's own encoder over seven sizes rather than trusted as a formula. The floor also now asserts the headroom the constant's doc comment CLAIMS. Before, a ceiling with zero room for the command string or protobuf framing satisfied the bound while contradicting the documented derivation. EXEC_FRAMING_HEADROOM is 64 KiB, measured rather than guessed: the RED error showed 5592440 bytes against a 5592408-byte base64 payload, a 32-byte delta, so 64 KiB is three orders of magnitude of slack on the one unbounded term (the command string). MEMORY BUDGET NOW RECORDED, in the code, spec §8 and the ADR entry. This repo states such budgets as formulas when it moves a ceiling — BufferCap gives 2 x MaxConcurrent x BufferCap, and the 2026-08-28 revision pins the caps equal for memory parity — and a 4x rise in worst-case ingress buffering had no note at all. Stated honestly as a TRANSIENT rather than a residency (the relay forwards exec and keeps no copy) and as a sizing figure rather than a proof, since nothing in the contract bounds how many requests decode at once. THE APPLIED-GUARD PINNED A LOCAL VARIABLE NAME. Matching on `relayAddr` would report "the option is not applied" if that variable were renamed, while the option was in fact applied — a false negative on the one assertion whose whole job is telling applied from merely defined. The address argument is now matched generically, and the guard throws loudly naming both possible causes, matching readMaxRecvMsgBytes's convention in the same file. Not loosened to a bare match on the function name, which would be too permissive in the other direction since main.go's own comment mentions it. All four guards re-checked by mutation: ceiling 11184811 fails, exact-length-with-no-headroom fails, renaming relayAddr still PASSES, and reverting main.go to a hand-rolled dial still fails. The first two also trip the loud-throw extractor, since the mutated Go constant loses its `* 1024 * 1024` form — that guard reporting "constant renamed or reformatted" is it working as designed. No production behaviour change: the 16 MiB value is unchanged and still clears the corrected floor by ~5 MiB. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Paolo Dettori <dettori@us.ibm.com>

pdettori commented Sep 8, 2026

View reviewed changes

pdettori merged commit b533d87 into rossoctl:main Sep 8, 2026
12 checks passed

pdettori deleted the fix/173-item2-message-size branch September 8, 2026 15:55

Navigation Menu

Uh oh!

fix(transport): derive the gRPC message ceiling from the output cap (#173 item 2) - #233

fix(transport): derive the gRPC message ceiling from the output cap (#173 item 2)#233
pdettori merged 2 commits into
rossoctl:mainfrom
pdettori:fix/173-item2-message-size

pdettori commented Sep 8, 2026

Uh oh!

pdettori left a comment

Uh oh!

pdettori Sep 8, 2026

Uh oh!

pdettori Sep 8, 2026

Uh oh!

pdettori Sep 8, 2026

Uh oh!

pdettori Sep 8, 2026

Uh oh!

pdettori Sep 8, 2026

Uh oh!

pdettori Sep 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

pdettori commented Sep 8, 2026

The item's severity claim is wrong, and tracing it changed the fix

The real defect is a read/write asymmetry the item never mentions

The fix, and why the number isn't a judgement call

Equality, not a floor — this is the load-bearing detail

"Needs a decision on both ends" was illusory

One structural change

Docs

Verification

Uh oh!

pdettori left a comment

Choose a reason for hiding this comment

Uh oh!

pdettori Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

pdettori Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

pdettori Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

pdettori Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

pdettori Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

pdettori Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant