From e16e544a61c1b3ac694721b56e2e3bf143736d21 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Tue, 8 Sep 2026 09:50:50 -0400 Subject: [PATCH 1/2] fix(transport): derive the gRPC message ceiling from the output cap (#173 item 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #180/#182/#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) Signed-off-by: Paolo Dettori --- .../0024-sandbox-transport-remote-exec.md | 39 +++++++++++++ ...026-07-08-sandbox-transport-grpc-design.md | 24 ++++++++ packages/k8s-sandbox/src/index.ts | 3 + packages/k8s-sandbox/src/transport.ts | 33 +++++++++++ .../test/message-size-coupling.test.ts | 55 ++++++++++++++++++ packages/sandbox-relay/src/main.ts | 9 ++- .../sandbox-relay/test/main-wiring.test.ts | 14 +++++ remote-worker/cmd/worker/main.go | 19 ++----- .../internal/session/contract_test.go | 53 ++++++++++++++++- remote-worker/internal/session/dial.go | 57 +++++++++++++++++++ 10 files changed, 290 insertions(+), 16 deletions(-) create mode 100644 packages/k8s-sandbox/test/message-size-coupling.test.ts create mode 100644 remote-worker/internal/session/dial.go diff --git a/docs/adrs/0024-sandbox-transport-remote-exec.md b/docs/adrs/0024-sandbox-transport-remote-exec.md index 0e135e58..35f645b3 100644 --- a/docs/adrs/0024-sandbox-transport-remote-exec.md +++ b/docs/adrs/0024-sandbox-transport-remote-exec.md @@ -217,6 +217,45 @@ transport.ts` hardcodes `streaming: true` — so a scripted `End{truncated: true runner's accounting and the session's frame. Equality of the two caps is still pinned, but it now pins memory parity rather than detectability, and the test says so. +### 2026-09-08 — the gRPC message ceiling is derived from the output cap (#173 item 2) + +`MaxCallRecvMsgSize` was unconfigured, so both ends sat at gRPC's 4 MiB receive default. + +**The reported severity was wrong, and tracing it changed the fix.** The item claimed an +oversized write "kills the whole stream rather than one exec, triggering a reconnect". It does +not: `ExecRequest` is `{sandbox_id, exec}` and the relay 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 was a read/write asymmetry the item never mentioned.** Read is capped at +`DEFAULT_OUTPUT_CAP` (8 MiB); a write costs 4/3 of the file 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. `KubectlTransport` has no such ceiling, so the same write succeeded +on the pod path — the divergence class of the three revisions above. + +**Decided:** 16 MiB on both receive limits, **derived** as `DEFAULT_OUTPUT_CAP × 4/3` plus +command and framing headroom, so write capacity ≥ read capacity by construction. Pinned equal +across the language boundary by `message-size-coupling.test.ts`, which asserts the derivation +too, so raising the output cap alone cannot restore the asymmetry. Send limits were already +unlimited on both implementations and are untouched. + +**Rejected:** raising only the relay, the hop that visibly rejects today. It 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. Hence an equality between the two +limits, not a floor, and `TestContractAcceptsAnOversizedWritePayload` fails if the worker is +left at the default. + +**Rejected:** treating "needs a decision on both ends plus a documented max write size" as a +prerequisite. That framing kept the item untouched for a month, but once the asymmetry is the +frame the number follows from `DEFAULT_OUTPUT_CAP` and needs no cross-team agreement. + +**Consequence:** the worker's dial options moved into `session.DialOptions` so the contract +tests dial exactly as `main.go` does. An option that production does not apply is +indistinguishable from an absent one, and the coupling test asserts `main.go` reaches the wire +through that function. + --- _Assisted-By: Claude (Anthropic AI) _ diff --git a/docs/specs/2026-07-08-sandbox-transport-grpc-design.md b/docs/specs/2026-07-08-sandbox-transport-grpc-design.md index 83444679..825aa5a8 100644 --- a/docs/specs/2026-07-08-sandbox-transport-grpc-design.md +++ b/docs/specs/2026-07-08-sandbox-transport-grpc-design.md @@ -408,6 +408,30 @@ The frame _semantics_ are carried from the superseded design verbatim — only t seam a second truncation concept for bytes it does not return would buy nothing, and the Go worker logs the event instead. +- **Message-size ceiling, and why it is derived rather than chosen.** gRPC defaults every + receive limit to 4 MiB on both implementations (grpc-js + `DEFAULT_MAX_RECEIVE_MESSAGE_LENGTH`, grpc-go `defaultClient/ServerMaxReceiveMessageSize`); + send limits default to unlimited. 4 MiB is **smaller than this contract's own write path + needs.** A write travels as base64 in `Exec.stdin`, inflating the file by 4/3, so the + largest _readable_ file — `DEFAULT_OUTPUT_CAP`, 8 MiB — becomes a ~10.7 MiB `Exec`. Left at + the default, every file between ~3 MiB and 8 MiB was **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 such ceiling, making + this one more divergence decided by which backend was leased. + + The ceiling is therefore **16 MiB**, covering `DEFAULT_OUTPUT_CAP × 4/3` plus the command + string and framing, so write capacity ≥ read capacity by construction. It is + `MAX_EXEC_MESSAGE_BYTES` (`transport.ts`) and `session.MaxRecvMsgBytes` (`dial.go`), pinned + equal by `message-size-coupling.test.ts` — which also asserts the derivation, so raising the + output cap alone cannot silently restore the asymmetry. + + **Both ends must move together, and the worker's must not be lower.** The relay's ingress + limit rejects an oversized `ExecRequest` with `RESOURCE_EXHAUSTED`, and that failure is + contained to one exec. A relay that accepts more than the worker forwards a payload the + worker then refuses **on the Attach stream**, whose death takes every concurrent and queued + exec with it and forces a re-dial — strictly worse than both sitting at the default. That + ordering is why the limits are pinned as an equality rather than as a floor. + - **Abort/end races.** A late `End` for an aborted `req_id` is dropped; an `Abort` for an already-ended `req_id` is a no-op. diff --git a/packages/k8s-sandbox/src/index.ts b/packages/k8s-sandbox/src/index.ts index 48fedf9b..a60d58e6 100644 --- a/packages/k8s-sandbox/src/index.ts +++ b/packages/k8s-sandbox/src/index.ts @@ -2,6 +2,9 @@ export { k8sSandboxExtension } from './extension.js'; export { resolveConfig, type K8sSandboxConfig } from './config.js'; export { buildKubectlArgs, KubectlTransport, type ExecInPod, type ExecResult } from './exec.js'; export type { SandboxTransport } from './transport.js'; +// Exported for the relay, which must set its ingress limit to the SAME value the Go +// worker uses — see MAX_EXEC_MESSAGE_BYTES on why the two cannot move independently. +export { MAX_EXEC_MESSAGE_BYTES } from './transport.js'; export { buildPersistentKubectlArgs, persistentExecInPod } from './persistent-exec.js'; export { buildSelectorArgs, diff --git a/packages/k8s-sandbox/src/transport.ts b/packages/k8s-sandbox/src/transport.ts index b8b125e5..db4ec70e 100644 --- a/packages/k8s-sandbox/src/transport.ts +++ b/packages/k8s-sandbox/src/transport.ts @@ -75,6 +75,39 @@ export const DEFAULT_OUTPUT_CAP = 8 * 1024 * 1024; // 8 MiB /** Appended to returned stdout when the cap trips, so Pi sees the truncation. */ export const OUTPUT_TRUNCATED_MARKER = '\n[output truncated]'; +/** + * BASE64_INFLATION is what a write costs on the wire. `createPodWriteOps` sends file + * content as base64 in `Exec.stdin` (operations.ts), and base64 encodes 3 bytes as 4. + */ +export const BASE64_INFLATION = 4 / 3; + +/** + * MAX_EXEC_MESSAGE_BYTES raises gRPC's 4 MiB default receive limit, which is smaller + * than this contract's own write path needs (#173 item 2). + * + * THE DERIVATION, because the number must not be arbitrary. The largest readable file + * is DEFAULT_OUTPUT_CAP, and writing it back costs DEFAULT_OUTPUT_CAP × + * BASE64_INFLATION ≈ 10.7 MiB of `Exec`. At the 4 MiB default, every file between + * ~3 MiB and 8 MiB was READABLE BUT NOT WRITABLE — and Pi's Edit composes read with + * write, so editing one succeeded at reading and then failed. 16 MiB covers the + * inflated cap with room for the command string and protobuf framing, making write + * capacity >= read capacity by construction. `KubectlTransport` pipes base64 through + * `kubectl exec` stdin with no such ceiling, so this also removes a divergence where + * the same write succeeded or failed depending on which backend was leased. + * + * BOTH ENDS MUST MOVE TOGETHER, and the worker's limit must be at least this one. + * The relay's ingress is what rejects an oversized `ExecRequest` today, and that + * rejection is contained to a single exec. Raising the relay alone would forward the + * payload and move the rejection onto the worker's Attach stream, whose death takes + * every concurrent and queued exec with it. The Go side is + * `session.MaxRecvMsgBytes`, pinned to this value by + * test/message-size-coupling.test.ts — change one and change the other. + * + * Send limits need no change: grpc-js defaults max_send_message_length to -1 and + * grpc-go defaults MaxCallSendMsgSize to MaxInt32, both effectively unlimited. + */ +export const MAX_EXEC_MESSAGE_BYTES = 16 * 1024 * 1024; // 16 MiB + /** * Ceiling on one exec when the caller names no `timeout` (spec §3; issue #182). Shared by * ALL THREE implementations, for the same reason DEFAULT_OUTPUT_CAP is: an exec that diff --git a/packages/k8s-sandbox/test/message-size-coupling.test.ts b/packages/k8s-sandbox/test/message-size-coupling.test.ts new file mode 100644 index 00000000..17a07ae1 --- /dev/null +++ b/packages/k8s-sandbox/test/message-size-coupling.test.ts @@ -0,0 +1,55 @@ +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { BASE64_INFLATION, DEFAULT_OUTPUT_CAP, MAX_EXEC_MESSAGE_BYTES } from '../src/transport.js'; + +const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '../../..'); +const DIAL_GO = resolve(REPO_ROOT, 'remote-worker/internal/session/dial.go'); +const MAIN_GO = resolve(REPO_ROOT, 'remote-worker/cmd/worker/main.go'); + +/** + * Loud-throw reader, same shape as output-cap-coupling's: a renamed or reformatted Go + * constant must fail the extraction rather than limp through as NaN and pass the + * comparison by accident. + */ +const readMaxRecvMsgBytes = (): number => { + const src = readFileSync(DIAL_GO, 'utf8'); + const match = /MaxRecvMsgBytes = (\d+) \* 1024 \* 1024/.exec(src); + if (!match) { + throw new Error( + 'could not find `MaxRecvMsgBytes = N * 1024 * 1024` in dial.go — constant renamed or reformatted?', + ); + } + return Number(match[1]) * 1024 * 1024; +}; + +describe('gRPC message-size limit is pinned across the language boundary', () => { + it("MAX_EXEC_MESSAGE_BYTES equals the Go worker's MaxRecvMsgBytes", () => { + // The two ends must move TOGETHER. The relay's ingress limit rejects an oversized + // ExecRequest and contains the failure to one exec; if the worker's limit were + // lower, the relay would forward a payload the worker then refuses — on the Attach + // stream, whose death takes every concurrent and queued exec with it and forces a + // re-dial. A worker limit BELOW the relay's is therefore strictly worse than both + // being at the 4 MiB default, which is why this is an equality and not a floor. + expect(MAX_EXEC_MESSAGE_BYTES).toBe(readMaxRecvMsgBytes()); + }); + + it('leaves room to write back the largest file the read path can return', () => { + // The defect this closes: read is capped at DEFAULT_OUTPUT_CAP (8 MiB) while a + // write costs 4/3 of the file in Exec.stdin, so at gRPC's 4 MiB default a file + // 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); + }); + + it('is actually applied by the worker, not merely defined', () => { + // 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( + /grpc\.NewClient\(\s*relayAddr,\s*session\.DialOptions\(/, + ); + }); +}); diff --git a/packages/sandbox-relay/src/main.ts b/packages/sandbox-relay/src/main.ts index 66f79cee..9d263293 100644 --- a/packages/sandbox-relay/src/main.ts +++ b/packages/sandbox-relay/src/main.ts @@ -17,13 +17,20 @@ import { type ExecEvent, type AbortRequest, type AbortResponse, + MAX_EXEC_MESSAGE_BYTES, } from '@sh/k8s-sandbox'; import { RedisRecordStore } from '@sh/harness'; import { createRelay, type RelayDeps, type AttachStream } from './relay.js'; export function buildServer(deps: RelayDeps): { server: Server } { const relay = createRelay(deps); - const server = new Server(); + // Raise the ingress limit above gRPC's 4 MiB default. This is the hop that rejects an + // oversized write today: the harness's ExecRequest carries base64 stdin at 4/3 of the + // file, so a file the read path can return (DEFAULT_OUTPUT_CAP, 8 MiB) needs ~10.7 MiB + // 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 }); const workerImpl: SandboxWorkerServer = { // AttachStream types metadata.get() as returning string[]; grpc-js's real diff --git a/packages/sandbox-relay/test/main-wiring.test.ts b/packages/sandbox-relay/test/main-wiring.test.ts index c569d5f3..06dedddd 100644 --- a/packages/sandbox-relay/test/main-wiring.test.ts +++ b/packages/sandbox-relay/test/main-wiring.test.ts @@ -2,6 +2,7 @@ import { EventEmitter } from 'node:events'; import { describe, expect, it, vi } from 'vitest'; import { buildServer } from '../src/main.js'; import type { RecordStore } from '@sh/harness'; +import { MAX_EXEC_MESSAGE_BYTES } from '@sh/k8s-sandbox'; const records: RecordStore = { put: async () => {}, remove: async () => {}, list: async () => [] }; @@ -15,6 +16,19 @@ function getHandler(server: unknown, path: string): (call: unknown) => unknown { } describe('relay server wiring', () => { + // #173 item 2. This is the limit that actually rejects an oversized write today: the + // harness sends an ExecRequest whose base64 stdin is 4/3 of the file, and gRPC's + // default ingress ceiling is 4 MiB — so a file the read path can return (up to + // DEFAULT_OUTPUT_CAP, 8 MiB) could not be written back. It must equal the worker's + // MaxRecvMsgBytes, not merely be raised: a relay that accepts MORE than the worker + // forwards a payload the worker then refuses on the Attach stream, killing every + // concurrent exec on it. message-size-coupling.test.ts pins the equality. + it('raises the ingress message limit to the shared MAX_EXEC_MESSAGE_BYTES', () => { + const { server } = buildServer({ records, validateToken: () => true }); + const options = (server as unknown as { options: Record }).options; + expect(options['grpc.max_receive_message_length']).toBe(MAX_EXEC_MESSAGE_BYTES); + }); + it('registers both gRPC services', () => { const { server } = buildServer({ records, validateToken: () => true }); // grpc-js Server keeps registered handlers in a private `handlers` Map keyed diff --git a/remote-worker/cmd/worker/main.go b/remote-worker/cmd/worker/main.go index 117d0a74..281e35df 100644 --- a/remote-worker/cmd/worker/main.go +++ b/remote-worker/cmd/worker/main.go @@ -24,7 +24,6 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/credentials" "google.golang.org/grpc/credentials/insecure" - "google.golang.org/grpc/keepalive" "google.golang.org/grpc/metadata" pb "github.com/kagenti/serverless-harness/gen/go/sandbox/v1" @@ -126,19 +125,11 @@ func main() { // alive and registered while answering nothing. These params bound that to // ~40s (Time + Timeout) instead of TCP retransmit exhaustion. // - // PermitWithoutStream: false is deliberate and safe here. The Attach stream is - // open for the entire time liveness matters, so pinging only while a stream is - // active loses nothing — and the relay (packages/sandbox-relay) configures no - // keepalive enforcement policy at all, so this cannot trip a server-side - // GOAWAY ENHANCE_YOUR_CALM. - conn, err := grpc.NewClient(relayAddr, - grpc.WithTransportCredentials(creds), - grpc.WithKeepaliveParams(keepalive.ClientParameters{ - Time: 30 * time.Second, - Timeout: 10 * time.Second, - PermitWithoutStream: false, - }), - ) + // The params themselves, and the raised receive limit that travels with them, now + // live in session.DialOptions so the contract tests dial exactly as production + // does — an unapplied option is indistinguishable from an absent one otherwise + // (#173 item 2). + conn, err := grpc.NewClient(relayAddr, session.DialOptions(creds)...) if err != nil { log.Fatalf("dial %s: %v", relayAddr, err) } diff --git a/remote-worker/internal/session/contract_test.go b/remote-worker/internal/session/contract_test.go index 1e6b2f64..71659c25 100644 --- a/remote-worker/internal/session/contract_test.go +++ b/remote-worker/internal/session/contract_test.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "os" "os/exec" + "strconv" "strings" "testing" "time" @@ -60,7 +61,11 @@ func (h *harness) attach(t *testing.T) *relaytest.Conn { // stream's own context is cancelled. func (h *harness) attachCancellable(t *testing.T) (*relaytest.Conn, context.CancelFunc, <-chan error) { t.Helper() - cc, err := grpc.NewClient(h.relay.Addr, grpc.WithTransportCredentials(insecure.NewCredentials())) + // session.DialOptions, not a hand-rolled option list: these tests are the only + // place the worker's dial configuration is exercised, so building their own would + // leave "does main.go actually apply it?" unanswered — and an unapplied option is + // indistinguishable from an absent one (#173 item 2). + cc, err := grpc.NewClient(h.relay.Addr, session.DialOptions(insecure.NewCredentials())...) if err != nil { t.Fatalf("dial %s: %v", h.relay.Addr, err) } @@ -497,6 +502,52 @@ func TestContractNonStreamingBuffersThenChunksAtExit(t *testing.T) { } } +// #173 item 2. A write travels as base64 in Exec.stdin, which inflates the payload by +// 4/3 — so an 8 MiB file (readable, since DEFAULT_OUTPUT_CAP is 8 MiB) becomes an +// ~10.7 MiB Exec, far past gRPC's 4 MiB default receive limit. The harness's read path +// can therefore fetch files this path cannot write back. +// +// THE ORDER OF THE TWO FIXES MATTERS, which is what this test guards. Raising only the +// relay's ingress limit would let the oversized ExecRequest through and move the +// rejection one hop later, onto the worker's Attach stream — and THAT kills the whole +// connection, taking every concurrent and queued exec with it. It is the failure item 2 +// originally described, so fixing the relay alone would have created the bug the item +// wrongly claimed already existed. The worker's receive limit must be at least the +// relay's, and this test fails if the worker is left at the 4 MiB default. +func TestContractAcceptsAnOversizedWritePayload(t *testing.T) { + h := newHarness(t) + conn := h.attach(t) + + // Just past the 4 MiB default once base64-encoded, exactly as writeFile encodes it + // (operations.ts) — the real shape of the payload, not a synthetic blob. + content := make([]byte, 4*1024*1024) + for i := range content { + content[i] = byte('a' + i%26) + } + b64 := base64.StdEncoding.EncodeToString(content) + if len(b64) <= 4*1024*1024 { + t.Fatalf("payload is %d bytes, which does not cross the 4 MiB default it exists to cross", len(b64)) + } + + const reqID = 900 + conn.SendExec(t, &pb.Exec{ + ReqId: reqID, + Command: "base64 -d | wc -c", + Stdin: []byte(b64), + Streaming: false, + }) + + stdout, _, terminal := conn.Collect(t, reqID) + if terminal.GetEnd() == nil || terminal.GetEnd().GetExitCode() != 0 { + t.Fatalf("terminal = %+v, want End{exit_code:0}: the oversized Exec did not survive the stream", terminal) + } + // The command decodes what it received and counts the bytes, so this proves the + // payload arrived WHOLE rather than merely that the stream stayed up. + if got := strings.TrimSpace(string(stdout)); got != strconv.Itoa(len(content)) { + t.Errorf("worker decoded %s bytes, want %d: the payload was truncated in transit", got, len(content)) + } +} + // Serve must RETURN when the context that created the Attach stream is cancelled: // the shutdown path main.go depends on, where SIGTERM cancels attachCtx. Note what // actually does the work — recvLoop has no select on ctx, so cancellation reaches diff --git a/remote-worker/internal/session/dial.go b/remote-worker/internal/session/dial.go new file mode 100644 index 00000000..b2c5ea11 --- /dev/null +++ b/remote-worker/internal/session/dial.go @@ -0,0 +1,57 @@ +package session + +import ( + "time" + + "google.golang.org/grpc" + "google.golang.org/grpc/credentials" + "google.golang.org/grpc/keepalive" +) + +// MaxRecvMsgBytes raises gRPC's 4 MiB default receive limit, which is too small for +// this contract's own write path (#173 item 2). +// +// THE DERIVATION, because the number must not be arbitrary. A write travels as base64 +// in Exec.stdin (operations.ts encodes it), and base64 inflates by 4/3. The read path +// is capped at DEFAULT_OUTPUT_CAP = 8 MiB, so the largest readable file becomes an +// 8 MiB × 4/3 ≈ 10.7 MiB Exec. At the 4 MiB default, every file between ~3 MiB and +// 8 MiB was READABLE BUT NOT WRITABLE — and Pi's Edit composes read with write, so +// editing such a file succeeded at reading and then failed. 16 MiB covers the +// inflated read cap with room for the command string and protobuf framing, so write +// capacity is >= read capacity by construction. +// +// BOTH ENDS MUST MOVE TOGETHER, and the worker's must be at least the relay's. The +// relay's ingress limit is what rejects an oversized ExecRequest today, and that +// rejection is contained to one exec. Raising the relay alone would forward the +// payload and move the rejection here — onto the Attach stream, whose death takes +// every concurrent and queued exec with it and forces main.go to re-dial. The +// coupling is pinned across the language boundary by +// packages/k8s-sandbox/test/message-size-coupling.test.ts and behaviourally by +// TestContractAcceptsAnOversizedWritePayload. +// +// Send limits need no change: grpc-go defaults MaxCallSendMsgSize to MaxInt32 and +// grpc-js defaults max_send_message_length to -1, both effectively unlimited. +const MaxRecvMsgBytes = 16 * 1024 * 1024 + +// DialOptions returns the options every worker connection to the relay must use. +// It exists so tests exercise the SAME configuration production dials with: the +// receive limit above is only meaningful if it is actually applied, and a test that +// built its own option list would prove nothing about main.go. +func DialOptions(creds credentials.TransportCredentials) []grpc.DialOption { + return []grpc.DialOption{ + grpc.WithTransportCredentials(creds), + // Raise the receive ceiling for ServerFrames carrying a large Exec.stdin. This + // is the relay->worker half of the pair described on MaxRecvMsgBytes. + grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(MaxRecvMsgBytes)), + // PermitWithoutStream: false is deliberate and safe here. The Attach stream is + // open for the entire time liveness matters, so pinging only while a stream is + // active loses nothing — and the relay (packages/sandbox-relay) configures no + // keepalive enforcement policy at all, so this cannot trip a server-side + // GOAWAY ENHANCE_YOUR_CALM. + grpc.WithKeepaliveParams(keepalive.ClientParameters{ + Time: 30 * time.Second, + Timeout: 10 * time.Second, + PermitWithoutStream: false, + }), + } +} From 01e91d047529a9fa0faac6ce708b7da454576ac9 Mon Sep 17 00:00:00 2001 From: Paolo Dettori Date: Tue, 8 Sep 2026 10:52:25 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix(transport):=20address=20PR=20#233=20rev?= =?UTF-8?q?iew=20=E2=80=94=20exact=20base64=20floor,=20budget=20note,=20ro?= =?UTF-8?q?bust=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Signed-off-by: Paolo Dettori --- .../0024-sandbox-transport-remote-exec.md | 25 ++++++-- ...026-07-08-sandbox-transport-grpc-design.md | 23 ++++++-- packages/k8s-sandbox/src/transport.ts | 59 +++++++++++++++---- .../test/message-size-coupling.test.ts | 59 +++++++++++++++---- 4 files changed, 135 insertions(+), 31 deletions(-) diff --git a/docs/adrs/0024-sandbox-transport-remote-exec.md b/docs/adrs/0024-sandbox-transport-remote-exec.md index 35f645b3..4afeabfe 100644 --- a/docs/adrs/0024-sandbox-transport-remote-exec.md +++ b/docs/adrs/0024-sandbox-transport-remote-exec.md @@ -234,11 +234,26 @@ write ceiling. Files between ~3 and 8 MiB were **readable but not writable**, an composes read with write. `KubectlTransport` has no such ceiling, so the same write succeeded on the pod path — the divergence class of the three revisions above. -**Decided:** 16 MiB on both receive limits, **derived** as `DEFAULT_OUTPUT_CAP × 4/3` plus -command and framing headroom, so write capacity ≥ read capacity by construction. Pinned equal -across the language boundary by `message-size-coupling.test.ts`, which asserts the derivation -too, so raising the output cap alone cannot restore the asymmetry. Send limits were already -unlimited on both implementations and are untouched. +**Decided:** 16 MiB on both receive limits, **derived** as +`base64EncodedLength(DEFAULT_OUTPUT_CAP) + EXEC_FRAMING_HEADROOM` (11,184,812 + 65,536), so +write capacity ≥ read capacity by construction. Pinned equal across the language boundary by +`message-size-coupling.test.ts`, which asserts the derivation too, so raising the output cap +alone cannot restore the asymmetry. Send limits were already unlimited on both implementations +and are untouched. + +The floor uses the **exact** encoded length `4·⌈n/3⌉` rather than the ×4/3 ratio. Caught in +review: the ratio yields 11,184,810.67 against a real base64 length of 11,184,812, so a bound +written against it sat 1.33 bytes _below_ the smallest payload it existed to admit and would +have certified a ceiling of 11,184,811 that cannot carry an 8 MiB file. **A guard derived from +an approximation can fail at exactly the boundary it was written for.** + +**Memory budget**, stated because moving a ceiling in this repo comes with one (`BufferCap` +gives `2 × MaxConcurrent × BufferCap`; the 2026-08-28 revision pins the caps equal for memory +parity). Worst-case ingress buffering rises 4×, as +`concurrently decoding ExecRequests × MAX_EXEC_MESSAGE_BYTES`. It is a transient rather than a +residency — the relay forwards `exec` and keeps no copy — and nothing in the contract bounds +the count, so it sizes the relay rather than proving a bound. One such request is a quarter of +the worker's 64 MiB budget, and only a write near the read cap reaches it. **Rejected:** raising only the relay, the hop that visibly rejects today. It would forward the payload and move the rejection onto the worker's Attach stream — killing every concurrent and diff --git a/docs/specs/2026-07-08-sandbox-transport-grpc-design.md b/docs/specs/2026-07-08-sandbox-transport-grpc-design.md index 825aa5a8..7f176f41 100644 --- a/docs/specs/2026-07-08-sandbox-transport-grpc-design.md +++ b/docs/specs/2026-07-08-sandbox-transport-grpc-design.md @@ -419,11 +419,24 @@ The frame _semantics_ are carried from the superseded design verbatim — only t `KubectlTransport` pipes base64 through `kubectl exec` stdin with no such ceiling, making this one more divergence decided by which backend was leased. - The ceiling is therefore **16 MiB**, covering `DEFAULT_OUTPUT_CAP × 4/3` plus the command - string and framing, so write capacity ≥ read capacity by construction. It is - `MAX_EXEC_MESSAGE_BYTES` (`transport.ts`) and `session.MaxRecvMsgBytes` (`dial.go`), pinned - equal by `message-size-coupling.test.ts` — which also asserts the derivation, so raising the - output cap alone cannot silently restore the asymmetry. + The ceiling is therefore **16 MiB**, clearing + `base64EncodedLength(DEFAULT_OUTPUT_CAP) + EXEC_FRAMING_HEADROOM` = 11,184,812 + 65,536, so + write capacity ≥ read capacity by construction. It is `MAX_EXEC_MESSAGE_BYTES` + (`transport.ts`) and `session.MaxRecvMsgBytes` (`dial.go`), pinned equal by + `message-size-coupling.test.ts` — which also asserts the derivation, so raising the output + cap alone cannot silently restore the asymmetry. The floor uses the **exact** encoded + length `4·⌈n/3⌉`, not the ×4/3 ratio: the ratio gives 11,184,810.67, which sits _below_ the + real payload and would admit a ceiling too small to carry an 8 MiB file. + + **Memory budget.** Raising a receive limit raises worst-case ingress buffering with it, 4× + here, so it is stated as a formula the way `BufferCap` states its own: + `concurrently decoding ExecRequests × MAX_EXEC_MESSAGE_BYTES`. It is a **transient, not a + residency** — the relay 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. Nothing in the contract bounds that count (the relay is single-replica and serves + every harness replica), so this is a ceiling to size the relay against rather than a proof. + For scale, one such request costs a quarter of the worker's own 64 MiB + (`2 × MaxConcurrent × BufferCap`) budget, and only a write near the read cap reaches it. **Both ends must move together, and the worker's must not be lower.** The relay's ingress limit rejects an oversized `ExecRequest` with `RESOURCE_EXHAUSTED`, and that failure is diff --git a/packages/k8s-sandbox/src/transport.ts b/packages/k8s-sandbox/src/transport.ts index db4ec70e..b79cce4a 100644 --- a/packages/k8s-sandbox/src/transport.ts +++ b/packages/k8s-sandbox/src/transport.ts @@ -76,24 +76,48 @@ export const DEFAULT_OUTPUT_CAP = 8 * 1024 * 1024; // 8 MiB export const OUTPUT_TRUNCATED_MARKER = '\n[output truncated]'; /** - * BASE64_INFLATION is what a write costs on the wire. `createPodWriteOps` sends file - * content as base64 in `Exec.stdin` (operations.ts), and base64 encodes 3 bytes as 4. + * base64EncodedLength is what a write costs on the wire, EXACTLY. `createPodWriteOps` + * sends file content as base64 in `Exec.stdin` (operations.ts), and base64 emits 4 + * bytes per 3 consumed, padding the final group — so the length is `4·⌈n/3⌉`, which is + * up to 2 bytes more than the ×4/3 ratio suggests. + * + * The ratio is fine for prose and wrong for a bound: at DEFAULT_OUTPUT_CAP the two + * differ by 1.33 bytes, and 8 MiB × 4/3 = 11184810.67 sits BELOW the real 11184812. A + * guard written against the ratio therefore admits a ceiling that cannot actually carry + * the largest readable file — it fails to certify the one property it exists for, at + * exactly its own boundary. Use this instead of multiplying. + */ +export const base64EncodedLength = (bytes: number): number => 4 * Math.ceil(bytes / 3); + +/** + * EXEC_FRAMING_HEADROOM is what an `Exec` costs BEYOND its base64 stdin: the command + * string, the protobuf field tags and length prefixes, and `ExecRequest`'s `sandbox_id`. + * + * Measured, not guessed: a 4 MiB write arrived as 5592440 bytes against a base64 payload + * of 5592408 — a delta of **32 bytes**. 64 KiB is three orders of magnitude above that, + * which is deliberate: the command string is the only unbounded term (`base64 -d > ` + * today, a few dozen bytes) and nothing in the contract caps it. */ -export const BASE64_INFLATION = 4 / 3; +export const EXEC_FRAMING_HEADROOM = 64 * 1024; /** * MAX_EXEC_MESSAGE_BYTES raises gRPC's 4 MiB default receive limit, which is smaller * than this contract's own write path needs (#173 item 2). * * THE DERIVATION, because the number must not be arbitrary. The largest readable file - * is DEFAULT_OUTPUT_CAP, and writing it back costs DEFAULT_OUTPUT_CAP × - * BASE64_INFLATION ≈ 10.7 MiB of `Exec`. At the 4 MiB default, every file between - * ~3 MiB and 8 MiB was READABLE BUT NOT WRITABLE — and Pi's Edit composes read with - * write, so editing one succeeded at reading and then failed. 16 MiB covers the - * inflated cap with room for the command string and protobuf framing, making write - * capacity >= read capacity by construction. `KubectlTransport` pipes base64 through - * `kubectl exec` stdin with no such ceiling, so this also removes a divergence where - * the same write succeeded or failed depending on which backend was leased. + * is DEFAULT_OUTPUT_CAP, and writing it back costs + * `base64EncodedLength(DEFAULT_OUTPUT_CAP) + EXEC_FRAMING_HEADROOM` = 11184812 + 65536 + * ≈ 10.7 MiB of `Exec`. At the 4 MiB default, every file between ~3 MiB and 8 MiB was + * READABLE BUT NOT WRITABLE — and Pi's Edit composes read with write, so editing one + * succeeded at reading and then failed. 16 MiB clears that floor with ~5 MiB to spare, + * making write capacity >= read capacity by construction. `KubectlTransport` pipes + * base64 through `kubectl exec` stdin with no such ceiling, so this also removes a + * divergence where the same write succeeded or failed depending on which backend was + * leased. + * + * The floor is asserted against the EXACT encoded length, not the ×4/3 ratio — see + * base64EncodedLength for why the ratio cannot certify this property at its own + * boundary. * * BOTH ENDS MUST MOVE TOGETHER, and the worker's limit must be at least this one. * The relay's ingress is what rejects an oversized `ExecRequest` today, and that @@ -103,6 +127,19 @@ export const BASE64_INFLATION = 4 / 3; * `session.MaxRecvMsgBytes`, pinned to this value by * test/message-size-coupling.test.ts — change one and change the other. * + * MEMORY BUDGET — raising a receive limit raises worst-case ingress buffering with it, + * 4x here, so state it the way `BufferCap` states its own (runner.go): + * + * concurrently decoding ExecRequests × MAX_EXEC_MESSAGE_BYTES + * + * It is a TRANSIENT, not a residency: the relay forwards `exec` to the Attach stream and + * keeps no copy (relay.ts routeExec), so the peak is however many oversized requests are + * mid-decode at once rather than however many execs are in flight. Nothing in the + * contract bounds that count — the relay is single-replica and serves every harness + * replica — so this is a ceiling to size the relay's limits against, not a proof. For + * scale: one such request costs a quarter of the worker's own 64 MiB + * (2 × MaxConcurrent × BufferCap) budget, and only a write near the read cap reaches it. + * * Send limits need no change: grpc-js defaults max_send_message_length to -1 and * grpc-go defaults MaxCallSendMsgSize to MaxInt32, both effectively unlimited. */ diff --git a/packages/k8s-sandbox/test/message-size-coupling.test.ts b/packages/k8s-sandbox/test/message-size-coupling.test.ts index 17a07ae1..07154097 100644 --- a/packages/k8s-sandbox/test/message-size-coupling.test.ts +++ b/packages/k8s-sandbox/test/message-size-coupling.test.ts @@ -2,7 +2,12 @@ import { readFileSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; import { dirname, resolve } from 'node:path'; import { describe, expect, it } from 'vitest'; -import { BASE64_INFLATION, DEFAULT_OUTPUT_CAP, MAX_EXEC_MESSAGE_BYTES } from '../src/transport.js'; +import { + base64EncodedLength, + DEFAULT_OUTPUT_CAP, + EXEC_FRAMING_HEADROOM, + MAX_EXEC_MESSAGE_BYTES, +} from '../src/transport.js'; const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '../../..'); const DIAL_GO = resolve(REPO_ROOT, 'remote-worker/internal/session/dial.go'); @@ -36,20 +41,54 @@ describe('gRPC message-size limit is pinned across the language boundary', () => }); it('leaves room to write back the largest file the read path can return', () => { - // The defect this closes: read is capped at DEFAULT_OUTPUT_CAP (8 MiB) while a - // write costs 4/3 of the file in Exec.stdin, so at gRPC's 4 MiB default a file - // 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); + // The defect this closes: read is capped at DEFAULT_OUTPUT_CAP (8 MiB) while a write + // costs base64 of the file in Exec.stdin, so at gRPC's 4 MiB default a file 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. + // + // Two things this bound gets right that `DEFAULT_OUTPUT_CAP * 4/3` did not. The + // encoded length is EXACT: 4·⌈n/3⌉ is 11184812 where the ratio gives 11184810.67, so + // the ratio's bound sat 1.33 bytes BELOW the smallest payload it exists to admit and + // would have passed a ceiling of 11184811 that cannot carry an 8 MiB file. And the + // headroom term is asserted rather than merely claimed in prose: without it, a + // ceiling with zero room for the command string or protobuf framing satisfies the + // floor while contradicting the constant's own documented derivation. + expect(MAX_EXEC_MESSAGE_BYTES).toBeGreaterThanOrEqual( + base64EncodedLength(DEFAULT_OUTPUT_CAP) + EXEC_FRAMING_HEADROOM, + ); + }); + + it('encodes lengths the way base64 actually does, padding included', () => { + // base64EncodedLength is load-bearing for the floor above, so pin it directly rather + // than trusting the formula: Node's own encoder is the oracle. + for (const n of [0, 1, 2, 3, 4, 3000, DEFAULT_OUTPUT_CAP]) { + expect(base64EncodedLength(n)).toBe(Buffer.alloc(n).toString('base64').length); + } }); it('is actually applied by the worker, not merely defined', () => { // 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( - /grpc\.NewClient\(\s*relayAddr,\s*session\.DialOptions\(/, - ); + // + // `[^)]*` for the address argument, NOT the literal `relayAddr`: pinning a local + // variable name would make renaming it report "the option is not applied" while the + // option is in fact applied — a false negative on the one assertion whose entire job + // is telling applied from merely defined. The requirement is that the DialOptions + // call sits inside NewClient's arguments; what the address is called is not this + // test's business. A bare /session\.DialOptions\(/ would be too loose in the other + // direction, since main.go's own comment names the function. + const mainGo = readFileSync(MAIN_GO, 'utf8'); + if (!/grpc\.NewClient\([^)]*session\.DialOptions\(/.test(mainGo)) { + // Loud throw naming the cause, like readMaxRecvMsgBytes above: a bare toMatch + // failure here reads as "the limit is not applied" when the likelier truth is that + // the dial was restructured and this assertion needs re-aiming. + throw new Error( + '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.', + ); + } }); });