US-168: tighten bulk-write caps and make event-loop stalls visible

[Hazzng]

Stacks on #190 (fix/181-test-scaffolding-error-shape) — base is that branch, not main.

Addresses the concrete, decision-free defects #168 documents alongside its headline. It does not fix the headline and #168 stays open (no closing keyword).

What this does

1. MAX_BULK_WRITE_BYTES no longer defaults 2.5x wider than the per-file cap — breaking.
It defaulted to 128 MiB against a MAX_FILE_WRITE_BYTES of 50 MiB, so the bulk route accepted 2.5x what the single-file route did; the issue measured a 120 MiB batch blocking the loop for 669 ms. It now defaults to MAX_FILE_WRITE_BYTES and is derived from it in lib/env.ts, so the two cannot drift: a batch costs no more synchronous work than the widest single write the service already accepts. A caller currently sending a bulk total between 50 MiB and 128 MiB now gets 413 PAYLOAD_TOO_LARGE where it previously got 204. Set MAX_BULK_WRITE_BYTES explicitly to keep the old ceiling. (The per-entry cap was already closed in aa60a5a; this is the total.)

2. POST /writeFiles gets the writeBodyLimit middleware it never had.
Every other write route counts the body off the stream; this one did not, so an over-declared or chunked body was JSON-parsed into memory before anything checked it, bounded only by the 256 MiB global backstop. writeBodyLimit is now parameterised by limit and mounted on the route. The wire cap is twice MAX_BULK_WRITE_BYTES, not equal to it: the content caps are on decoded bytes while JSON string escaping roughly doubles the wire size of newline-/quote-heavy text, so an equal cap would reject legal batches. It is a backstop on the parse, not a second content cap.

3. eventLoopLagSnapshot() gets a caller.
It had zero non-test callers — not a route, not /readyz, not a metrics endpoint — so nothing could poll how close a replica ran to the 2 s Redis commandTimeout. GET /readyz now carries it as an eventLoop object. The field is absent (not null, not zeroed) when the monitor was never started, so an idle loop and an unmeasured one cannot be confused. Reading does not reset the histogram. Folded in: exposing it made a type lie visible — an empty histogram reports mean as NaN, which JSON.stringify renders as null where the type promises a number.

4. A lone stall now moves something other than maxMs.
Adds p999Ms to the snapshot/log line and an event_loop_stall line at severity:"critical" when a window's maxMs crosses EVENT_LOOP_STALL_THRESHOLD_MS (default 2000 — the Redis client's commandTimeout, i.e. the point where a stall starts failing other tenants' in-flight commands). It reuses the existing heartbeat_gap severity shape; no metrics system was invented.

DEFAULT_RESOLUTION_MS stays at 20 ms deliberately — the resolution is not what hid the stalls. A 10 s window holds ~400–500 readings, so one stall is ~0.2% of the sample and lands above p99 by construction. Lowering the resolution makes that strictly worse (more readings, a smaller share each) while doubling libuv wakeups; raising it to 100 ms would let a stall reach p99 only by discarding every reading below 100 ms. Idle cost is unchanged either way: 50 timer wakeups/s, each a clock read plus one HDR record — order 1e-5 of a core.

What this does NOT do

The headline of #168 — sandbox exec has no file-size ceiling, so sed over 16 MiB blocks the loop for seconds with 62.8% of the CPU in GC — is untouched. Neither of its real cures (a size ceiling inside SqlFs, or moving bash.exec to a worker thread) is here; that is a structural decision and is out of scope for this PR. heartbeat_gap still thresholds on REDIS_EXEC_LOCK_RENEW_MS (20 s), so it still cannot fire below a 20 s stall.

Measurement — the monitor now surfaces a stall it previously hid

sed 's/a/b/g' over a 16 MiB file on the FAULT replica, 4686 ms wall. The windowed log line for that 10 s window:

{"event":"event_loop_lag","p50Ms":21,"p99Ms":21,"p999Ms":4660,"maxMs":4660,"meanMs":39,"windowMs":10000}
{"event":"event_loop_stall","severity":"critical","maxMs":4660,"p999Ms":4660,"thresholdMs":2000,"windowMs":10000}

p50/p99 sit flat at the 21 ms idle floor exactly as the issue describes; p999Ms moves and the stall line fires. Before this change the same window logged {"p50Ms":21,"p99Ms":21,"maxMs":21,"meanMs":21}-shaped lines with no stall event and no p99.9.

Cap verified live too: a 3x20 MiB bulk write (each entry under the per-file cap) →
413 {"code":"PAYLOAD_TOO_LARGE","details":["Bulk write exceeds total byte limit (52428800)"]}; a small batch still 204.

Verification

• pnpm typecheck / pnpm lint:fix clean. pnpm test:unit: 1300 passed / 4 skipped (baseline 1288/4; +12 new). Nothing pre-existing broke.
• Every new test was checked by reverting its source change in isolation and confirming failure — the bulk total, the body middleware, p999Ms, the stall line, the NaN guard and the /readyz field were each reverted separately.
• node scripts/loadtest/scenarios/concurrency.mjs — all PASS, exit 0.

Not verified

• The 128 MiB → 50 MiB change was not re-measured against a real 120 MiB batch; the 669 ms figure is quoted from the issue.
• The monitor's idle-CPU figure is an order-of-magnitude estimate from the work per sample, not a profiler measurement.
• p99.9 resolves a single outlier only while a window holds fewer than ~500 readings (HDR percentile rounding). At the default 20 ms resolution a 10 s window holds ~400–500, so it works there — but maxMs and event_loop_stall are the signals that always do, and the docs say so.

Refs #168.

---

Summary by cubic

Tightens bulk-write caps so a batch costs no more synchronous work than the widest single write, and makes event-loop stalls visible at the point they start timing out Redis commands. This addresses the concrete bulk-write and observability defects in #168; the issue’s headline exec file-size ceiling stays open.

Bulk write caps

• MAX_BULK_WRITE_BYTES now derives from MAX_FILE_WRITE_BYTES (50 MiB) instead of defaulting to 128 MiB; on POST /writeFiles, totals between 50 and 128 MiB now get 413 PAYLOAD_TOO_LARGE instead of 204. Set MAX_BULK_WRITE_BYTES explicitly to keep the old ceiling.
• The files map on POST /v1/sandboxes now uses the same per-file, total, and count caps as POST /writeFiles, so creates with 50–128 MiB of initial files also get 413; non-numeric env values no longer disable the caps.
• POST /writeFiles now enforces a streaming body cap at twice the decoded cap, since JSON string escaping inflates wire size.

Event-loop observability

• GET /readyz returns the lag histogram as eventLoop; the field is absent when the monitor never started, and reading it doesn’t reset the histogram.
• A window whose maxMs crosses EVENT_LOOP_STALL_THRESHOLD_MS (default 2000, the Redis commandTimeout) emits event_loop_stall at critical, and snapshots now include p999Ms because a lone stall stays hidden in p99.
• Empty histograms now report a finite meanMs instead of null.

The #168 headline — a file-size ceiling on sandbox exec — remains out of scope and the issue stays open.

^{Written for commit 84f200b. Summary will update on new commits.}

---

Reopened from #193: its base branch fix/131-epoch-fence was dropped when #161 merged to main. Rebased onto the new main; base is now fix/181-test-scaffolding-error-shape.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 9b2f0baa-52dc-4023-8a34-8df90774de2f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cubic-dev-ai]

6 issues found across 11 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/api/event-loop-monitor.ts">

<violation number="1" location="src/api/event-loop-monitor.ts:136">
P2: When a measured maximum is just over an integer threshold, `toMs()` rounds it down to the threshold before this comparison, so no `event_loop_stall` is emitted. Preserve or compare the raw nanosecond maximum before resetting the histogram, and round only the logged payload.</violation>
</file>

<file name="src/api/tests/unit/readyz-event-loop.test.ts">

<violation number="1" location="src/api/tests/unit/readyz-event-loop.test.ts:41">
P3: Test 2 asserts `maxMs > 0` after only 80 ms of idle time at the default 20 ms histogram resolution, but idle event-loop delay readings (poll-phase sleeps) are environment-dependent and can round to 0 ms through `toMs` (`Math.round(ns / 1e6)`). The companion `event-loop-monitor.test.ts` never asserts a nonzero `max` without forcing one via `busyLoopMs()`, so this assertion can flake on quiet hosts. Force a recorded reading by blocking synchronously before the request, matching that pattern.</violation>
</file>

<file name="src/api/tests/unit/files.body-limit.test.ts">

<violation number="1" location="src/api/tests/unit/files.body-limit.test.ts:272">
P3: The overflow tests for /writeFiles don't assert the incoming stream is cancelled, unlike the PATCH suite in this same file ("cancels the incoming stream when the body overflows", which checks `canceled === true`). The middleware aborts the source upload a single reader can't drain, and that cleanup is the point of the #168 fix for chunked/lying bodies, so lock it in for the bulk route: pass `chunked(BULK_BODY_LIMIT * 2, () => { canceled = true; })` and assert `expect(canceled).toBe(true)` alongside the 413.</violation>
</file>

<file name="src/api/tests/unit/event-loop-monitor.test.ts">

<violation number="1" location="src/api/tests/unit/event-loop-monitor.test.ts:174">
P3: This window is wall-clock sensitive: with ~109 readings (500ms warmup at 5ms resolution), p99 lands on the second-highest sample, so the test fails if any single non-injected reading ≥ 50ms occurs during the ~740ms run (a GC pause or scheduler kick on loaded CI). The semantic you want — that p99 dilutes the lone stall — is more robustly expressed relative to p999 (which is already asserted ≥ 100), rather than against an absolute 50ms floor that depends on every idle reading staying tiny.</violation>
</file>

<file name="src/api/lib/env.ts">

<violation number="1" location="src/api/lib/env.ts:63">
P1: When `MAX_BULK_WRITE_BYTES` is unset, `POST /v1/sandboxes` initial-files still enforces a 128 MiB total: `src/api/routes/sandboxes.ts:28` reads the same env var with the old `128 * 1024 * 1024` fallback. The changeset and the README/CLAUDE.md rows this PR adds claim the default is now `MAX_FILE_WRITE_BYTES` and that the two caps "cannot drift", but the create route keeps the exact 2.5x door #168 documents — no per-file cap and no body-limit middleware on that path either. Point `sandboxes.ts` at the env.ts-derived constants (as this route does) so both consumers share one default.</violation>

<violation number="2" location="src/api/lib/env.ts:75">
P2: The 2x wire cap is not just a parse backstop: it can reject legal batches below the decoded cap. The middleware counts raw stream bytes, and `JSON.stringify` expands control characters (U+0000–U+001F other than \b\t\n\f\r) to `\uXXXX` — 6 wire bytes per 1 decoded byte — while JSON keys/braces consume wire bytes that never enter the decoded total. A control-char-heavy batch at roughly one third of `MAX_BULK_WRITE_BYTES` (e.g. ~17 MiB decoded of a 50 MiB cap) exceeds the 100 MiB wire cap and gets `413 Bulk write body exceeds limit (...)` even though its decoded total is legal. This contradicts the documented 'backstop, not a second content cap' design and will mislead callers who sized by decoded bytes.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: When MAX_BULK_WRITE_BYTES is unset, POST /v1/sandboxes initial-files still enforces a 128 MiB total: src/api/routes/sandboxes.ts:28 reads the same env var with the old 128 * 1024 * 1024 fallback. The changeset and the README/CLAUDE.md rows this PR adds claim the default is now MAX_FILE_WRITE_BYTES and that the two caps "cannot drift", but the create route keeps the exact 2.5x door #168 documents — no per-file cap and no body-limit middleware on that path either. Point sandboxes.ts at the env.ts-derived constants (as this route does) so both consumers share one default.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/api/lib/env.ts, line 63:

<comment>When `MAX_BULK_WRITE_BYTES` is unset, `POST /v1/sandboxes` initial-files still enforces a 128 MiB total: `src/api/routes/sandboxes.ts:28` reads the same env var with the old `128 * 1024 * 1024` fallback. The changeset and the README/CLAUDE.md rows this PR adds claim the default is now `MAX_FILE_WRITE_BYTES` and that the two caps "cannot drift", but the create route keeps the exact 2.5x door #168 documents — no per-file cap and no body-limit middleware on that path either. Point `sandboxes.ts` at the env.ts-derived constants (as this route does) so both consumers share one default.</comment>

<file context>
@@ -50,3 +50,26 @@ if (MAX_FILE_WRITE_BYTES > DEFAULT_CONTENT_CACHE_MAX_BYTES) {
+ * {@link MAX_FILE_WRITE_BYTES} makes the batch cost no more synchronous work than the widest
+ * single write the service already accepts, and ties the two together so they cannot drift.
+ */
+export const MAX_BULK_WRITE_BYTES = positiveIntEnv(process.env.MAX_BULK_WRITE_BYTES, MAX_FILE_WRITE_BYTES);
+
+/**
</file context>

[cubic-dev-ai]

P2: When a measured maximum is just over an integer threshold, toMs() rounds it down to the threshold before this comparison, so no event_loop_stall is emitted. Preserve or compare the raw nanosecond maximum before resetting the histogram, and round only the logged payload.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/api/event-loop-monitor.ts, line 136:

<comment>When a measured maximum is just over an integer threshold, `toMs()` rounds it down to the threshold before this comparison, so no `event_loop_stall` is emitted. Preserve or compare the raw nanosecond maximum before resetting the histogram, and round only the logged payload.</comment>

<file context>
@@ -95,6 +131,20 @@ export function startEventLoopMonitor(opts: EventLoopMonitorOptions = {}): void
 		log(JSON.stringify({ event: "event_loop_lag", ...snapshot, windowMs: sampleIntervalMs }));
+		// A lone stall is invisible in a windowed percentile line nobody alerts on, so it gets its
+		// own severity-tagged event — the same shape `heartbeat_gap` uses (#168).
+		if (snapshot.maxMs > stallThresholdMs) {
+			logStall(
+				JSON.stringify({
</file context>

[cubic-dev-ai]

P2: The 2x wire cap is not just a parse backstop: it can reject legal batches below the decoded cap. The middleware counts raw stream bytes, and JSON.stringify expands control characters (U+0000–U+001F other than \b\t\n\f\r) to \uXXXX — 6 wire bytes per 1 decoded byte — while JSON keys/braces consume wire bytes that never enter the decoded total. A control-char-heavy batch at roughly one third of MAX_BULK_WRITE_BYTES (e.g. ~17 MiB decoded of a 50 MiB cap) exceeds the 100 MiB wire cap and gets 413 Bulk write body exceeds limit (...) even though its decoded total is legal. This contradicts the documented 'backstop, not a second content cap' design and will mislead callers who sized by decoded bytes.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/api/lib/env.ts, line 75:

<comment>The 2x wire cap is not just a parse backstop: it can reject legal batches below the decoded cap. The middleware counts raw stream bytes, and `JSON.stringify` expands control characters (U+0000–U+001F other than \b\t\n\f\r) to `\uXXXX` — 6 wire bytes per 1 decoded byte — while JSON keys/braces consume wire bytes that never enter the decoded total. A control-char-heavy batch at roughly one third of `MAX_BULK_WRITE_BYTES` (e.g. ~17 MiB decoded of a 50 MiB cap) exceeds the 100 MiB wire cap and gets `413 Bulk write body exceeds limit (...)` even though its decoded total is legal. This contradicts the documented 'backstop, not a second content cap' design and will mislead callers who sized by decoded bytes.</comment>

<file context>
@@ -50,3 +50,26 @@ if (MAX_FILE_WRITE_BYTES > DEFAULT_CONTENT_CACHE_MAX_BYTES) {
+ * is a backstop on the parse, not a second content cap — the decoded totals are what the route
+ * actually enforces.
+ */
+export const MAX_BULK_WRITE_BODY_BYTES = MAX_BULK_WRITE_BYTES * 2;
</file context>

[cubic-dev-ai]

P3: Test 2 asserts maxMs > 0 after only 80 ms of idle time at the default 20 ms histogram resolution, but idle event-loop delay readings (poll-phase sleeps) are environment-dependent and can round to 0 ms through toMs (Math.round(ns / 1e6)). The companion event-loop-monitor.test.ts never asserts a nonzero max without forcing one via busyLoopMs(), so this assertion can flake on quiet hosts. Force a recorded reading by blocking synchronously before the request, matching that pattern.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/api/tests/unit/readyz-event-loop.test.ts, line 41:

<comment>Test 2 asserts `maxMs > 0` after only 80 ms of idle time at the default 20 ms histogram resolution, but idle event-loop delay readings (poll-phase sleeps) are environment-dependent and can round to 0 ms through `toMs` (`Math.round(ns / 1e6)`). The companion `event-loop-monitor.test.ts` never asserts a nonzero `max` without forcing one via `busyLoopMs()`, so this assertion can flake on quiet hosts. Force a recorded reading by blocking synchronously before the request, matching that pattern.</comment>

<file context>
@@ -0,0 +1,53 @@
+
+		const body = (await (await app.request("/readyz")).json()) as { eventLoop: { maxMs: number } };
+
+		expect(body.eventLoop.maxMs).toBeGreaterThan(0);
+		expect(eventLoopLagSnapshot()?.maxMs).toBeGreaterThanOrEqual(body.eventLoop.maxMs);
+	});
</file context>

Suggested change

	expect(body.eventLoop.maxMs).toBeGreaterThan(0);
	expect(eventLoopLagSnapshot()?.maxMs).toBeGreaterThanOrEqual(body.eventLoop.maxMs);
	});
	// Negative guard: the field is absent — not null, not zeroed — when nothing is measuring, so a
	await new Promise((res) => setTimeout(res, 40));
	// Block synchronously so the histogram records a guaranteed-nonzero reading; an idle loop's
	// poll sleeps can round to 0 ms at the default 20 ms resolution.
	const end = Date.now() + 40;
	while (Date.now() < end) {}
	await new Promise((res) => setTimeout(res, 40));
	const body = (await (await app.request("/readyz")).json()) as { eventLoop: { maxMs: number } };
	expect(body.eventLoop.maxMs).toBeGreaterThan(0);

[cubic-dev-ai]

P3: The overflow tests for /writeFiles don't assert the incoming stream is cancelled, unlike the PATCH suite in this same file ("cancels the incoming stream when the body overflows", which checks canceled === true). The middleware aborts the source upload a single reader can't drain, and that cleanup is the point of the #168 fix for chunked/lying bodies, so lock it in for the bulk route: pass chunked(BULK_BODY_LIMIT * 2, () => { canceled = true; }) and assert expect(canceled).toBe(true) alongside the 413.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/api/tests/unit/files.body-limit.test.ts, line 272:

<comment>The overflow tests for /writeFiles don't assert the incoming stream is cancelled, unlike the PATCH suite in this same file ("cancels the incoming stream when the body overflows", which checks `canceled === true`). The middleware aborts the source upload a single reader can't drain, and that cleanup is the point of the #168 fix for chunked/lying bodies, so lock it in for the bulk route: pass `chunked(BULK_BODY_LIMIT * 2, () => { canceled = true; })` and assert `expect(canceled).toBe(true)` alongside the 413.</comment>

<file context>
@@ -230,3 +230,103 @@ describe("PUT raw file body limit", () => {
+		});
+	});
+
+	it("rejects an oversized body streamed without a Content-Length", async () => {
+		const app = await makeApp();
+
</file context>

[cubic-dev-ai]

P3: This window is wall-clock sensitive: with ~109 readings (500ms warmup at 5ms resolution), p99 lands on the second-highest sample, so the test fails if any single non-injected reading ≥ 50ms occurs during the ~740ms run (a GC pause or scheduler kick on loaded CI). The semantic you want — that p99 dilutes the lone stall — is more robustly expressed relative to p999 (which is already asserted ≥ 100), rather than against an absolute 50ms floor that depends on every idle reading staying tiny.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/api/tests/unit/event-loop-monitor.test.ts, line 174:

<comment>This window is wall-clock sensitive: with ~109 readings (500ms warmup at 5ms resolution), p99 lands on the second-highest sample, so the test fails if any single non-injected reading ≥ 50ms occurs during the ~740ms run (a GC pause or scheduler kick on loaded CI). The semantic you want — that p99 dilutes the lone stall — is more robustly expressed relative to p999 (which is already asserted ≥ 100), rather than against an absolute 50ms floor that depends on every idle reading staying tiny.</comment>

<file context>
@@ -135,12 +152,84 @@ describe("event-loop monitor lifecycle", () => {
+			await new Promise((res) => setTimeout(res, 40));
+			const snap = eventLoopLagSnapshot();
+			expect(snap).toBeDefined();
+			expect(snap?.p99Ms).toBeLessThan(50);
+			expect(snap?.p999Ms).toBeGreaterThanOrEqual(100);
+		} finally {
</file context>

Suggested change

	expect(snap?.p99Ms).toBeLessThan(50);
	expect(snap?.p99Ms).toBeLessThan(snap?.p999Ms as number);

[Hazzng]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d41dfcb615

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Size the wire cap for worst-case JSON escaping

When file content contains JSON control characters, the wire representation can be much larger than twice the decoded UTF-8 size: for example, each one-byte NUL becomes the six-byte sequence \u0000. With a 1,024-byte decoded cap, a valid 400-byte file of NULs produces a 2,418-byte request and is rejected by the new 2,048-byte middleware limit before the decoded-content checks run; even newline- or quote-only content at exactly the decoded cap exceeds 2× once the JSON envelope is included. The wire ceiling therefore needs to accommodate worst-case escaping and structural overhead, or otherwise avoid rejecting batches that satisfy MAX_BULK_WRITE_BYTES.

Useful? React with 👍 / 👎.