diff --git a/docs/adrs/0024-sandbox-transport-remote-exec.md b/docs/adrs/0024-sandbox-transport-remote-exec.md index 0e135e58..4afeabfe 100644 --- a/docs/adrs/0024-sandbox-transport-remote-exec.md +++ b/docs/adrs/0024-sandbox-transport-remote-exec.md @@ -217,6 +217,60 @@ 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 +`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 +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..7f176f41 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,43 @@ 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**, 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 + 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..b79cce4a 100644 --- a/packages/k8s-sandbox/src/transport.ts +++ b/packages/k8s-sandbox/src/transport.ts @@ -75,6 +75,76 @@ 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]'; +/** + * 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 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 + * `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 + * 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. + * + * 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. + */ +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..07154097 --- /dev/null +++ b/packages/k8s-sandbox/test/message-size-coupling.test.ts @@ -0,0 +1,94 @@ +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; +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'); +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 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. + // + // `[^)]*` 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.', + ); + } + }); +}); 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, + }), + } +}