Conversation
* feat: add durable sandbox epoch migration * feat: fence postgres sandbox epochs * feat(sql-fs): fence script scopes with sandbox epoch * test: add postgres fencing regressions * fix: advance fencing epoch on composite writes * fix(sql-fs): address PR #161 review comments for epoch fencing (F2-L2 #131) - Accept tx-local advances with s.version > expected; extract epochParam helper - Throw ENOENT on missing sandbox; fallback to tombstone epoch on delete - Pin epoch inside locked tx (close TOCTOU); refresh tx-local epoch per composite - Publish lastKnownEpoch only after COMMIT; throw ESTALE with 409 mapping - Document sandbox_epochs RLS exemption; split fencing tests; harden integration setup * fix(sql-fs): close fencing gaps from follow-up review (F2-L2 #131) - Baseline lastKnownEpoch on ready/reload so scopes fence fresh cache state - Take writer lock before fenced CTE snapshot in all composites - Harden integration tests (race readiness, explicit ctx.skip on superuser) - Tighten fencing unit assertions to the deleting statement * chore: add changeset for epoch fencing (F2-L2 #131) --------- Co-authored-by: trek-ai[bot] <302790941+trek-ai[bot]@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
There was a problem hiding this comment.
8 issues found across 12 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/lib/env.ts">
<violation number="1" location="src/api/lib/env.ts:95">
P2: When `MAX_EXEC_FILE_BYTES` is set to a positive fraction below 1, such as `0.5`, this call produces a zero-byte cap because `positiveIntEnv` floors after checking `n > 0`. `SqlFs` then rejects every nonempty script read and write; floor before validating positivity and fall back when the result is below 1.</violation>
<violation number="2" location="src/api/lib/env.ts:95">
P2: Raising `MAX_EXEC_FILE_BYTES` above the 8 MiB default silently reintroduces the #168 cross-tenant failure this cap exists to prevent: past ~2 s of synchronous string building, in-flight Redis commands of *other* tenants time out (commandTimeout: 2000). The docstring tells operators "Raise it only where the replica is not shared, or is not backed by Redis", but nothing enforces or even flags that — and the sibling constant in this same file, `MAX_FILE_WRITE_BYTES` (lines 88-96...), sets the precedent of emitting `console.warn` when an override steps past the safe default. Add the same warning so an override on a shared/Redis-backed replica is visible at boot instead of only in a docstring.</violation>
</file>
<file name="src/sql-fs/errors.ts">
<violation number="1" location="src/sql-fs/errors.ts:144">
P2: When the file exceeds the exec limit, the suggested shell commands cannot process it in slices because just-bash reads the whole file before those utilities can truncate or split it. Recommend only the HTTP GET route here, or provide a genuinely range-aware exec operation before advertising shell slicing.</violation>
</file>
<file name="src/sql-fs/sql-fs.ts">
<violation number="1" location="src/sql-fs/sql-fs.ts:1114">
P1: When an exec passes a large string to `writeFile` or `appendFile`, `TextEncoder.encode` allocates the entire over-limit byte buffer before this assertion. Reject an obviously oversized string before encoding, then retain the exact byte-length check to prevent the allocation and event-loop stall this cap is intended to avoid.</violation>
</file>
<file name="src/api/tests/unit/exec-file-limit.test.ts">
<violation number="1" location="src/api/tests/unit/exec-file-limit.test.ts:199">
P3: Hardcode the ceiling number instead of reusing the imported constant. The remediation-message assertion pins "8388608 bytes" as a string literal while every other assertion in this file derives from `MAX_EXEC_FILE_BYTES`; if the default in `src/api/lib/env.ts` changes, this assertion fails even though the code is correct, and under a `MAX_EXEC_FILE_BYTES` env override the big file is created at `MAX_EXEC_FILE_BYTES + 1` and trips the cap, but this assertion still demands the default number. Interpolate the constant so the wording check tracks the real limit.</violation>
</file>
<file name=".changeset/fix-168-exec-file-size-ceiling.md">
<violation number="1" location=".changeset/fix-168-exec-file-size-ceiling.md:11">
P3: The claim that a file too large for a script is "still retrievable whole with ... MCP `file_read`" is over-broad and will mislead the agent that the rest of the changeset is written for. `tools.ts` gates `file_read` at `MAX_MCP_READ_FILE_BYTES` (default 16 MiB) and returns `file_too_large` above it (`src/api/mcp/tools.ts:222,309`), so a script that hits EFBIG on a 20 MiB file cannot recover via MCP `file_read`. Only `GET .../files/*path` (no size gate in `src/api/routes/files.ts`) is unconditionally available. Qualify the sentence with the 16 MiB MCP read cap.</violation>
</file>
<file name="src/api/exec-context.ts">
<violation number="1" location="src/api/exec-context.ts:25">
P2: When a script both trips the cap and is later aborted by timeout, the recorded `exceeded` error wins over the abort outcome. `bash.exec` resolves with exit 124 on a plain timeout, so session-manager's `runExec` reaches the `ctx.exceeded !== undefined` branch (session-manager.ts:1793-1796) and throws EFBIG while rolling the script-tx back — even though the file-cap trip is not what stopped the work. A script that previously committed several writes then reads an over-cap file and hits the runtime budget loses all of it and is told the cap was the failure. This diverges from the documented plain-timeout-commits policy (audit L7), and the client cannot learn both facts. Consider recording the abort outcome as well (or checking `bash.exec`'s resolved exit/timeout before the `exceeded` marker) so the surfaced error reflects the terminal cause, or documenting this precedence in the `exceeded` doc block.</violation>
</file>
<file name="CLAUDE.md">
<violation number="1" location="CLAUDE.md:237">
P3: In the new `MAX_EXEC_FILE_BYTES` row, the phrase "and past the 2 s Redis `commandTimeout` that stall times out *other* tenants' in-flight commands" reads as if the stall's subject got elided — "that stall" has no clear antecedent in the sentence (the earlier clause names only `sed s///g`). README's parallel sentence is unambiguous: "a stall past the 2 s Redis `commandTimeout` times out **other** tenants' in-flight commands." Use the same subject-first phrasing here.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const { name, parentEntry } = this.#requireParentDir(path); | ||
|
|
||
| const bytes = typeof content === "string" ? new TextEncoder().encode(content) : content; | ||
| this.#assertExecFileSize(path, bytes.byteLength, "write"); |
There was a problem hiding this comment.
P1: When an exec passes a large string to writeFile or appendFile, TextEncoder.encode allocates the entire over-limit byte buffer before this assertion. Reject an obviously oversized string before encoding, then retain the exact byte-length check to prevent the allocation and event-loop stall this cap is intended to avoid.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/sql-fs/sql-fs.ts, line 1114:
<comment>When an exec passes a large string to `writeFile` or `appendFile`, `TextEncoder.encode` allocates the entire over-limit byte buffer before this assertion. Reject an obviously oversized string before encoding, then retain the exact byte-length check to prevent the allocation and event-loop stall this cap is intended to avoid.</comment>
<file context>
@@ -1085,6 +1111,7 @@ export class SqlFs<Tx = unknown> implements ICoherentFs, IReadOnlyScopeFs {
const { name, parentEntry } = this.#requireParentDir(path);
const bytes = typeof content === "string" ? new TextEncoder().encode(content) : content;
+ this.#assertExecFileSize(path, bytes.byteLength, "write");
const sha256 = new Uint8Array(createHash("sha256").update(bytes).digest());
const mtime = new Date();
</file context>
| * cap at all — held until `bash.exec` moves off the main thread. Raise it only where the replica | ||
| * is not shared, or is not backed by Redis. | ||
| */ | ||
| export const MAX_EXEC_FILE_BYTES = positiveIntEnv(process.env.MAX_EXEC_FILE_BYTES, 8 * 1024 * 1024); |
There was a problem hiding this comment.
P2: When MAX_EXEC_FILE_BYTES is set to a positive fraction below 1, such as 0.5, this call produces a zero-byte cap because positiveIntEnv floors after checking n > 0. SqlFs then rejects every nonempty script read and write; floor before validating positivity and fall back when the result is below 1.
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 95:
<comment>When `MAX_EXEC_FILE_BYTES` is set to a positive fraction below 1, such as `0.5`, this call produces a zero-byte cap because `positiveIntEnv` floors after checking `n > 0`. `SqlFs` then rejects every nonempty script read and write; floor before validating positivity and fall back when the result is below 1.</comment>
<file context>
@@ -73,3 +73,23 @@ export const MAX_BULK_WRITE_BYTES = positiveIntEnv(process.env.MAX_BULK_WRITE_BY
+ * cap at all — held until `bash.exec` moves off the main thread. Raise it only where the replica
+ * is not shared, or is not backed by Redis.
+ */
+export const MAX_EXEC_FILE_BYTES = positiveIntEnv(process.env.MAX_EXEC_FILE_BYTES, 8 * 1024 * 1024);
</file context>
| op === "read" ? `'${path}' is ${attemptedBytes} bytes` : `writing '${path}' would produce ${attemptedBytes} bytes`; | ||
| const remedy = | ||
| op === "read" | ||
| ? "process a slice at a time (`head -c`, `tail -c`, `split -b`, `sed -n '1,20000p'`), or pull the whole file out over HTTP with `GET .../files/{path}` (MCP `file_read`), which this limit does not apply to" |
There was a problem hiding this comment.
P2: When the file exceeds the exec limit, the suggested shell commands cannot process it in slices because just-bash reads the whole file before those utilities can truncate or split it. Recommend only the HTTP GET route here, or provide a genuinely range-aware exec operation before advertising shell slicing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/sql-fs/errors.ts, line 144:
<comment>When the file exceeds the exec limit, the suggested shell commands cannot process it in slices because just-bash reads the whole file before those utilities can truncate or split it. Recommend only the HTTP GET route here, or provide a genuinely range-aware exec operation before advertising shell slicing.</comment>
<file context>
@@ -116,6 +116,44 @@ export function createEdriverfault(cause: Error): Error {
+ op === "read" ? `'${path}' is ${attemptedBytes} bytes` : `writing '${path}' would produce ${attemptedBytes} bytes`;
+ const remedy =
+ op === "read"
+ ? "process a slice at a time (`head -c`, `tail -c`, `split -b`, `sed -n '1,20000p'`), or pull the whole file out over HTTP with `GET .../files/{path}` (MCP `file_read`), which this limit does not apply to"
+ : "write several smaller files (`split -b`), send large content in over HTTP with `PUT .../files/{path}` (MCP `file_write`), or change part of a file with `PATCH .../files/{path}` (MCP `file_edit`) instead of rewriting it whole";
+ return makeFsError(
</file context>
Suggested change
| ? "process a slice at a time (`head -c`, `tail -c`, `split -b`, `sed -n '1,20000p'`), or pull the whole file out over HTTP with `GET .../files/{path}` (MCP `file_read`), which this limit does not apply to" | |
| +\t\t\t? "pull the whole file out over HTTP with `GET .../files/{path}` (MCP `file_read`), which this limit does not apply to" |
| /** Largest file this script may read whole or produce. See MAX_EXEC_FILE_BYTES. */ | ||
| readonly maxFileBytes: number; | ||
| /** | ||
| * The first EFBIG this script tripped, recorded as well as thrown. |
There was a problem hiding this comment.
P2: When a script both trips the cap and is later aborted by timeout, the recorded exceeded error wins over the abort outcome. bash.exec resolves with exit 124 on a plain timeout, so session-manager's runExec reaches the ctx.exceeded !== undefined branch (session-manager.ts:1793-1796) and throws EFBIG while rolling the script-tx back — even though the file-cap trip is not what stopped the work. A script that previously committed several writes then reads an over-cap file and hits the runtime budget loses all of it and is told the cap was the failure. This diverges from the documented plain-timeout-commits policy (audit L7), and the client cannot learn both facts. Consider recording the abort outcome as well (or checking bash.exec's resolved exit/timeout before the exceeded marker) so the surfaced error reflects the terminal cause, or documenting this precedence in the exceeded doc block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/api/exec-context.ts, line 25:
<comment>When a script both trips the cap and is later aborted by timeout, the recorded `exceeded` error wins over the abort outcome. `bash.exec` resolves with exit 124 on a plain timeout, so session-manager's `runExec` reaches the `ctx.exceeded !== undefined` branch (session-manager.ts:1793-1796) and throws EFBIG while rolling the script-tx back — even though the file-cap trip is not what stopped the work. A script that previously committed several writes then reads an over-cap file and hits the runtime budget loses all of it and is told the cap was the failure. This diverges from the documented plain-timeout-commits policy (audit L7), and the client cannot learn both facts. Consider recording the abort outcome as well (or checking `bash.exec`'s resolved exit/timeout before the `exceeded` marker) so the surfaced error reflects the terminal cause, or documenting this precedence in the `exceeded` doc block.</comment>
<file context>
@@ -0,0 +1,43 @@
+ /** Largest file this script may read whole or produce. See MAX_EXEC_FILE_BYTES. */
+ readonly maxFileBytes: number;
+ /**
+ * The first EFBIG this script tripped, recorded as well as thrown.
+ *
+ * Throwing alone is not enough on the read side: just-bash's text utilities catch every
</file context>
| * cap at all — held until `bash.exec` moves off the main thread. Raise it only where the replica | ||
| * is not shared, or is not backed by Redis. | ||
| */ | ||
| export const MAX_EXEC_FILE_BYTES = positiveIntEnv(process.env.MAX_EXEC_FILE_BYTES, 8 * 1024 * 1024); |
There was a problem hiding this comment.
P2: Raising MAX_EXEC_FILE_BYTES above the 8 MiB default silently reintroduces the #168 cross-tenant failure this cap exists to prevent: past ~2 s of synchronous string building, in-flight Redis commands of other tenants time out (commandTimeout: 2000). The docstring tells operators "Raise it only where the replica is not shared, or is not backed by Redis", but nothing enforces or even flags that — and the sibling constant in this same file, MAX_FILE_WRITE_BYTES (lines 88-96...), sets the precedent of emitting console.warn when an override steps past the safe default. Add the same warning so an override on a shared/Redis-backed replica is visible at boot instead of only in a docstring.
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 95:
<comment>Raising `MAX_EXEC_FILE_BYTES` above the 8 MiB default silently reintroduces the #168 cross-tenant failure this cap exists to prevent: past ~2 s of synchronous string building, in-flight Redis commands of *other* tenants time out (commandTimeout: 2000). The docstring tells operators "Raise it only where the replica is not shared, or is not backed by Redis", but nothing enforces or even flags that — and the sibling constant in this same file, `MAX_FILE_WRITE_BYTES` (lines 88-96...), sets the precedent of emitting `console.warn` when an override steps past the safe default. Add the same warning so an override on a shared/Redis-backed replica is visible at boot instead of only in a docstring.</comment>
<file context>
@@ -73,3 +73,23 @@ export const MAX_BULK_WRITE_BYTES = positiveIntEnv(process.env.MAX_BULK_WRITE_BY
+ * cap at all — held until `bash.exec` moves off the main thread. Raise it only where the replica
+ * is not shared, or is not backed by Redis.
+ */
+export const MAX_EXEC_FILE_BYTES = positiveIntEnv(process.env.MAX_EXEC_FILE_BYTES, 8 * 1024 * 1024);
</file context>
Suggested change
| export const MAX_EXEC_FILE_BYTES = positiveIntEnv(process.env.MAX_EXEC_FILE_BYTES, 8 * 1024 * 1024); | |
| export const MAX_EXEC_FILE_BYTES = positiveIntEnv(process.env.MAX_EXEC_FILE_BYTES, 8 * 1024 * 1024); | |
| // Same rationale as the MAX_FILE_WRITE_BYTES warning above: an override is sometimes legitimate | |
| // (dedicated replica, no shared Redis), but must not be silent — past the 8 MiB default the #168 | |
| // stall crosses the 2 s Redis commandTimeout and breaks other tenants' in-flight commands, not | |
| // just this request's latency. | |
| if (MAX_EXEC_FILE_BYTES > 8 * 1024 * 1024) { | |
| console.warn( | |
| JSON.stringify({ | |
| event: "exec_cap_above_default", | |
| maxExecFileBytes: MAX_EXEC_FILE_BYTES, | |
| warning: | |
| "with the exec ceiling raised, a large script read/write can stall the shared event loop past the 2 s Redis commandTimeout (cross-tenant, #168); only safe where the replica is not shared and is not backed by Redis", | |
| }), | |
| ); | |
| } |
| .then(() => undefined) | ||
| .catch((e: Error) => e); | ||
|
|
||
| expect(err?.message).toContain("the per-file exec limit is 8388608 bytes"); |
There was a problem hiding this comment.
P3: Hardcode the ceiling number instead of reusing the imported constant. The remediation-message assertion pins "8388608 bytes" as a string literal while every other assertion in this file derives from MAX_EXEC_FILE_BYTES; if the default in src/api/lib/env.ts changes, this assertion fails even though the code is correct, and under a MAX_EXEC_FILE_BYTES env override the big file is created at MAX_EXEC_FILE_BYTES + 1 and trips the cap, but this assertion still demands the default number. Interpolate the constant so the wording check tracks the real limit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/api/tests/unit/exec-file-limit.test.ts, line 199:
<comment>Hardcode the ceiling number instead of reusing the imported constant. The remediation-message assertion pins "8388608 bytes" as a string literal while every other assertion in this file derives from `MAX_EXEC_FILE_BYTES`; if the default in `src/api/lib/env.ts` changes, this assertion fails even though the code is correct, and under a `MAX_EXEC_FILE_BYTES` env override the big file is created at `MAX_EXEC_FILE_BYTES + 1` and trips the cap, but this assertion still demands the default number. Interpolate the constant so the wording check tracks the real limit.</comment>
<file context>
@@ -0,0 +1,225 @@
+ .then(() => undefined)
+ .catch((e: Error) => e);
+
+ expect(err?.message).toContain("the per-file exec limit is 8388608 bytes");
+ expect(err?.message).toContain("MAX_EXEC_FILE_BYTES");
+ expect(err?.message).not.toContain("No such file or directory");
</file context>
Suggested change
| expect(err?.message).toContain("the per-file exec limit is 8388608 bytes"); | |
| expect(err?.message).toContain(`the per-file exec limit is ${MAX_EXEC_FILE_BYTES} bytes`); |
|
|
||
| Past roughly 2 s the stall stops being a latency problem and becomes a correctness problem for unrelated tenants: `src/redis/client.ts` sets `commandTimeout: 2000`, so a stall beyond it times out in-flight Redis commands belonging to other sandboxes — observed in the harness as `rw_lock_writer_release_error … Command timed out` against a sandbox that had no part in the exec. The `sed` threshold for that is ~14 MiB. 8 MiB is chosen to sit a factor of two under it rather than at it, because the per-byte cost varies by utility (`tr` is worse than `sed`, `wc` is better) and the ceiling has to hold for the worst one. | ||
|
|
||
| The ceiling applies **only inside `bash.exec`** — it is scoped by an `AsyncLocalStorage` context, so the HTTP and MCP file routes keep their own, wider caps (`MAX_FILE_WRITE_BYTES`, 50 MiB). A file too large for a script to read is still retrievable whole with `GET /v1/sandboxes/:id/files/*path` or MCP `file_read`. That asymmetry is the point: moving bytes over HTTP costs one buffer copy, rebuilding them through a shell pipeline costs seconds of GC. |
There was a problem hiding this comment.
P3: The claim that a file too large for a script is "still retrievable whole with ... MCP file_read" is over-broad and will mislead the agent that the rest of the changeset is written for. tools.ts gates file_read at MAX_MCP_READ_FILE_BYTES (default 16 MiB) and returns file_too_large above it (src/api/mcp/tools.ts:222,309), so a script that hits EFBIG on a 20 MiB file cannot recover via MCP file_read. Only GET .../files/*path (no size gate in src/api/routes/files.ts) is unconditionally available. Qualify the sentence with the 16 MiB MCP read cap.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .changeset/fix-168-exec-file-size-ceiling.md, line 11:
<comment>The claim that a file too large for a script is "still retrievable whole with ... MCP `file_read`" is over-broad and will mislead the agent that the rest of the changeset is written for. `tools.ts` gates `file_read` at `MAX_MCP_READ_FILE_BYTES` (default 16 MiB) and returns `file_too_large` above it (`src/api/mcp/tools.ts:222,309`), so a script that hits EFBIG on a 20 MiB file cannot recover via MCP `file_read`. Only `GET .../files/*path` (no size gate in `src/api/routes/files.ts`) is unconditionally available. Qualify the sentence with the 16 MiB MCP read cap.</comment>
<file context>
@@ -0,0 +1,17 @@
+
+Past roughly 2 s the stall stops being a latency problem and becomes a correctness problem for unrelated tenants: `src/redis/client.ts` sets `commandTimeout: 2000`, so a stall beyond it times out in-flight Redis commands belonging to other sandboxes — observed in the harness as `rw_lock_writer_release_error … Command timed out` against a sandbox that had no part in the exec. The `sed` threshold for that is ~14 MiB. 8 MiB is chosen to sit a factor of two under it rather than at it, because the per-byte cost varies by utility (`tr` is worse than `sed`, `wc` is better) and the ceiling has to hold for the worst one.
+
+The ceiling applies **only inside `bash.exec`** — it is scoped by an `AsyncLocalStorage` context, so the HTTP and MCP file routes keep their own, wider caps (`MAX_FILE_WRITE_BYTES`, 50 MiB). A file too large for a script to read is still retrievable whole with `GET /v1/sandboxes/:id/files/*path` or MCP `file_read`. That asymmetry is the point: moving bytes over HTTP costs one buffer copy, rebuilding them through a shell pipeline costs seconds of GC.
+
+Enforcement is O(1) on the passing path — a store lookup and an integer compare before any blob is fetched, with no extra database round trip and no allocation. An over-cap read is refused without reading the blob at all.
</file context>
Suggested change
| The ceiling applies **only inside `bash.exec`** — it is scoped by an `AsyncLocalStorage` context, so the HTTP and MCP file routes keep their own, wider caps (`MAX_FILE_WRITE_BYTES`, 50 MiB). A file too large for a script to read is still retrievable whole with `GET /v1/sandboxes/:id/files/*path` or MCP `file_read`. That asymmetry is the point: moving bytes over HTTP costs one buffer copy, rebuilding them through a shell pipeline costs seconds of GC. | |
| The ceiling applies **only inside `bash.exec`** — it is scoped by an `AsyncLocalStorage` context, so the HTTP and MCP file routes keep their own caps (`MAX_FILE_WRITE_BYTES`, 50 MiB, for writes; `MAX_MCP_READ_FILE_BYTES`, 16 MiB, for MCP `file_read`; `GET .../files/*path` is uncapped). A file too large for a script to read is still retrievable whole with `GET /v1/sandboxes/:id/files/*path`, and with MCP `file_read` up to `MAX_MCP_READ_FILE_BYTES` (16 MiB). That asymmetry is the point: moving bytes over HTTP costs one buffer copy, rebuilding them through a shell pipeline costs seconds of GC. |
| | `REDIS_PATH_SNAPSHOT_TTL_MS` | No (default: 3600000) | TTL for path snapshot entries (ms, default 1h). | | ||
| | `MAX_FILE_WRITE_BYTES` | No (default: contentCache cap, 50 MiB) | Largest single file body any write surface accepts (PUT, PATCH, MCP `file_write`). Raising it past the contentCache cap costs ~4x the memory per file for the whole `SESSION_IDLE_MS`. | | ||
| | `MAX_BULK_WRITE_BYTES` | No (default: `MAX_FILE_WRITE_BYTES`) | Largest total decoded byte count one `POST /writeFiles` batch may carry. Defaults to the per-file cap so the bulk route is not a wider door than the single write it batches (#168 — it used to default to 128 MiB against a 50 MiB per-file cap). The request body is separately capped at twice this, counted off the stream before the JSON parse, to leave headroom for JSON string escaping. | | ||
| | `MAX_EXEC_FILE_BYTES` | No (default: 8 MiB) | Largest file a sandbox `exec` script may read whole, or produce with one write. Bash text utilities rebuild strings synchronously on the main thread — `sed s///g` blocks the event loop 706 ms at 8 MiB and 2238 ms at 16 MiB, and past the 2 s Redis `commandTimeout` that stall times out *other* tenants' in-flight commands. Trips as `EFBIG` → 413, never retryable. Applies only to calls made from inside `bash.exec`; the HTTP/MCP file routes keep their own caps, so a file above this can still be fetched with `GET .../files/{path}`. Stopgap until `bash.exec` moves off the main thread (#168). | |
There was a problem hiding this comment.
P3: In the new MAX_EXEC_FILE_BYTES row, the phrase "and past the 2 s Redis commandTimeout that stall times out other tenants' in-flight commands" reads as if the stall's subject got elided — "that stall" has no clear antecedent in the sentence (the earlier clause names only sed s///g). README's parallel sentence is unambiguous: "a stall past the 2 s Redis commandTimeout times out other tenants' in-flight commands." Use the same subject-first phrasing here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLAUDE.md, line 237:
<comment>In the new `MAX_EXEC_FILE_BYTES` row, the phrase "and past the 2 s Redis `commandTimeout` that stall times out *other* tenants' in-flight commands" reads as if the stall's subject got elided — "that stall" has no clear antecedent in the sentence (the earlier clause names only `sed s///g`). README's parallel sentence is unambiguous: "a stall past the 2 s Redis `commandTimeout` times out **other** tenants' in-flight commands." Use the same subject-first phrasing here.</comment>
<file context>
@@ -234,6 +234,7 @@ const TABLE = Object.assign(Object.create(null) as Record<string, string>, {
| `REDIS_PATH_SNAPSHOT_TTL_MS` | No (default: 3600000) | TTL for path snapshot entries (ms, default 1h). |
| `MAX_FILE_WRITE_BYTES` | No (default: contentCache cap, 50 MiB) | Largest single file body any write surface accepts (PUT, PATCH, MCP `file_write`). Raising it past the contentCache cap costs ~4x the memory per file for the whole `SESSION_IDLE_MS`. |
| `MAX_BULK_WRITE_BYTES` | No (default: `MAX_FILE_WRITE_BYTES`) | Largest total decoded byte count one `POST /writeFiles` batch may carry. Defaults to the per-file cap so the bulk route is not a wider door than the single write it batches (#168 — it used to default to 128 MiB against a 50 MiB per-file cap). The request body is separately capped at twice this, counted off the stream before the JSON parse, to leave headroom for JSON string escaping. |
+| `MAX_EXEC_FILE_BYTES` | No (default: 8 MiB) | Largest file a sandbox `exec` script may read whole, or produce with one write. Bash text utilities rebuild strings synchronously on the main thread — `sed s///g` blocks the event loop 706 ms at 8 MiB and 2238 ms at 16 MiB, and past the 2 s Redis `commandTimeout` that stall times out *other* tenants' in-flight commands. Trips as `EFBIG` → 413, never retryable. Applies only to calls made from inside `bash.exec`; the HTTP/MCP file routes keep their own caps, so a file above this can still be fetched with `GET .../files/{path}`. Stopgap until `bash.exec` moves off the main thread (#168). |
| `MAX_BULK_WRITE_FILES` | No (default: 1000) | Max number of entries in one `POST /writeFiles` batch. |
| `EVENT_LOOP_MONITOR_INTERVAL_MS` | No (default: 10000) | Sampling interval (ms) for the F8 event-loop lag monitor. Each window logs `event:"event_loop_lag"` (`p50Ms`/`p99Ms`/`p999Ms`/`maxMs`/`meanMs`) then resets the histogram. Purely observational; pairs with per-heartbeat `event:"heartbeat_gap"` warn/critical events that flag a stall eating into a Redis lease (see DEVELOPER.md "Lock observability"). The live histogram is also on `GET /readyz` as `eventLoop`. |
</file context>
Suggested change
| | `MAX_EXEC_FILE_BYTES` | No (default: 8 MiB) | Largest file a sandbox `exec` script may read whole, or produce with one write. Bash text utilities rebuild strings synchronously on the main thread — `sed s///g` blocks the event loop 706 ms at 8 MiB and 2238 ms at 16 MiB, and past the 2 s Redis `commandTimeout` that stall times out *other* tenants' in-flight commands. Trips as `EFBIG` → 413, never retryable. Applies only to calls made from inside `bash.exec`; the HTTP/MCP file routes keep their own caps, so a file above this can still be fetched with `GET .../files/{path}`. Stopgap until `bash.exec` moves off the main thread (#168). | | |
| | `MAX_EXEC_FILE_BYTES` | No (default: 8 MiB) | Largest file a sandbox `exec` script may read whole, or produce with one write. Bash text utilities rebuild strings synchronously on the main thread — `sed s///g` blocks the event loop 706 ms at 8 MiB and 2238 ms at 16 MiB, and a stall past the 2 s Redis `commandTimeout` times out *other* tenants' in-flight commands. Trips as `EFBIG` → 413, never retryable. Applies only to calls made from inside `bash.exec`; the HTTP/MCP file routes keep their own caps, so a file above this can still be fetched with `GET .../files/{path}`. Stopgap until `bash.exec` moves off the main thread (#168). | |
Hazzng
added this pull request to stack #199
September 19, 2026 01:01
Owner
Author
|
@codex review |
…y lock Wrap the whole migration run in one transaction and take pg_advisory_xact_lock as its first statement, replacing the session-scoped pg_advisory_lock that a transaction pooler silently defeats. Measured on the Neon pooler: a second booter acquired the old lock in 267-335ms while it was held; it now waits 1855-1906ms for the holder to commit. US-165: drop DATABASE_DIRECT_URL from the deployment, correct its doc row. No server code ever read it; the runner no longer needs a direct connection. drizzle-kit still reads it for pnpm db:generate, so the variable stays. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion-class SQLSTATEs to 503 `app.onError` and the exec SSE error frame allowlisted the error message but emitted the raw `code`, leaking driver codes (ECONNRESET, CONNECTION_CLOSED) and Postgres SQLSTATEs to clients. Add `clientSafeErrorCode` next to `clientSafeErrorMessage` so the pair cannot be used asymmetrically, and map connection-class SQLSTATEs (08xxx, 53300, 53400, 57P03) to a retryable 503 reported as EUNAVAILABLE. Closes #174 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wire c.req.raw.signal into the /exec-sync abort controller, mirroring
/exec-sync-batch (already-aborted pre-check plus a { once: true }
listener), so an abandoned request stops holding the sandbox exec lock
for the rest of its timeout. Semantics stay abort-only — work the script
already committed stays committed, as on /exec, /exec-sync-batch and the
timeout path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ceeds the lease Move the REDIS_EXEC_LOCK_* env parsing into loadExecLockOptions(), lower the acquire-timeout default from 300s to lease + ~15s reap margin, and fail startup when the acquire window is not strictly greater than both the writer lease and the reader lease — a crashed holder is only reaped when its lease expires, so a shorter window turns recovery into a 503. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both read handlers went through withOwnedSessionOrRehydrate, so two concurrent GETs on one sandbox serialized as if each were a write. Switch the two read call sites to withOwnedSessionRead — the same path MCP file_read already uses for identical work. This does NOT let a GET overtake an in-flight writer: shared still excludes exclusive. It removes reader-reader serialization only (C=12 burst wall time 131ms -> 6ms on the load-test harness). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Data plane (blob cache, path snapshot) moves to its own ioredis connection so multi-MiB SETs can no longer head-of-line block the control plane's locks and version counter. RedisBlobCache.set gains an in-flight count/byte cap and drops rather than queues. The circuit breaker is keyed by role, and every transition now logs redis_circuit_open / redis_circuit_closed with the role — the original outage produced zero log lines. Closes #167 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add a `retryable` discriminator to every error body, give the poisoned (rolled-back) publish its own `ECOHERENCE_UNAPPLIED` code, correct the ECOHERENCE message that told clients to retry an already-committed write, and defer — rather than surface — a stranded INCR failure on a turn that mutated nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Root-anchored "comparison/**" missed the copy inside an in-repo git worktree, sweeping a duplicate src/ suite and deliberately-excluded comparison fixtures into every pnpm test:unit run. Also exclude .claude/** outright. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four test apps hand-rolled the leaky pre-#174 onError, so they no longer matched production and could not fail on a code-leak regression. Route them through one testErrorHandler built on clientSafeErrorCode, and guard both the shape and the absence of hand-rolled copies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Default MAX_BULK_WRITE_BYTES to MAX_FILE_WRITE_BYTES instead of 128 MiB, add the streaming body cap POST /writeFiles never had, expose the lag histogram on /readyz, and surface a lone stall via p99.9 and a new event_loop_stall line thresholded at the Redis commandTimeout. Refs #168 — the structural exec cap / worker-thread fix stays open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a93f5f970f
ℹ️ 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".
| if (entry.kind === INODE_KIND.DIRECTORY) throw createEisdir(path); | ||
| // Before the cache lookup on purpose: a warm cache makes the DB round trip free | ||
| // but not the megabytes of string work the caller is about to do with the bytes. | ||
| this.#assertExecFileSize(path, entry.size, "read"); |
There was a problem hiding this comment.
When a script pipes two legal 8 MiB files into sed, or expands one legal file into a much larger result, every read passes this per-entry check but just-bash then synchronously builds and processes a 16+ MiB string. For redirected output the new write check runs only after that allocation, while stdout has no write check at all. Since the commit's own measurements show 16 MiB of sed work exceeding Redis's 2-second timeout, the cross-tenant failure remains trivially reachable; enforce a byte budget at the pipeline or command-output boundary rather than only at individual filesystem calls.
Useful? React with 👍 / 👎.
Comment on lines
+143
to
+145
| op === "read" | ||
| ? "process a slice at a time (`head -c`, `tail -c`, `split -b`, `sed -n '1,20000p'`), or pull the whole file out over HTTP with `GET .../files/{path}` (MCP `file_read`), which this limit does not apply to" | ||
| : "write several smaller files (`split -b`), send large content in over HTTP with `PUT .../files/{path}` (MCP `file_write`), or change part of a file with `PATCH .../files/{path}` (MCP `file_edit`) instead of rewriting it whole"; |
There was a problem hiding this comment.
For a file above the limit, head, tail, split, and sed still read through the same IFileSystem whole-file methods, and #readBytes rejects solely from the full inode size before fetching any content. Consequently every suggested in-sandbox slicing command trips the same EFBIG instead of recovering, leaving only the HTTP/MCP advice usable; either provide a ranged-read path or stop directing callers to commands that cannot pass this check.
Useful? React with 👍 / 👎.
A backend reaped mid-transaction leaves the driver flushing a buffered write to a nulled socket from a bare setImmediate, which is a fatal uncaught exception that kills the replica and every other in-flight request on it. Recognise exactly that stack frame, log it, and fail the DB awaits it stranded with EDRIVERFAULT instead of letting them hang; everything else keeps Node's default crash. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main's epoch fence (#161) calls getSandboxEpoch on every script-scope transaction, so a fake dialect without it throws before the test reaches the fault path it is asserting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…writes Distinguish a structurally invalid version key (WRONGTYPE, "ERR value is not an integer") from a transport failure. A structurally invalid key is repaired in place with an epoch-stamped SET so the write publishes normally; every other INCR failure keeps the #186 deferral. The F7 destroy tombstone is exempt so a repair cannot resurrect a destroyed sandbox. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Document maxmemory-policy allkeys-lru as a deployment requirement alongside REDIS_URL / REDIS_DATA_URL in CLAUDE.md and README.md, and check it at boot with CONFIG GET maxmemory-policy. A non-allkeys policy logs redis_eviction_policy_unsafe at critical severity. The check is never awaited, never fails startup, and recognises a provider that forbids CONFIG GET as its own outcome. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sandbox `exec` had no file-size bound of any kind: just-bash's text utilities rebuild strings synchronously on the main thread, so `sed s///g` blocked the event loop 706 ms at 8 MiB and 2238 ms at 16 MiB with 62.8% of the CPU in GC. Past the 2 s Redis `commandTimeout` that stall times out in-flight commands belonging to other tenants, which makes it a cross-tenant correctness bug rather than a latency blip. Enforce an 8 MiB ceiling (`MAX_EXEC_FILE_BYTES`) inside SqlFs, scoped through an AsyncLocalStorage set once per `bash.exec` so the HTTP and MCP file routes keep their own, looser caps. The size comes from the pathCache, so an over-cap call costs neither a round trip nor the allocation it exists to prevent. bash swallows an FS read rejection into a phantom "No such file or directory", so the error is recorded on the exec context as well as thrown and re-raised after the script returns — rolling the script-tx back first, the same shape `readOnlyContext` already uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
main's epoch fence (#161) calls getSandboxEpoch on every script-scope transaction, so a fake dialect without it throws before the test reaches the ceiling it is asserting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Hazzng
force-pushed
the
fix/168-exec-file-size-ceiling
branch
from
September 19, 2026 01:09
a93f5f9 to
a465a65
Compare
Owner
Author
|
Superseded by a re-opened PR with the correct base. Its original base |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #168. Stacks on PR #197.
Completes #168. Its other concrete parts — bulk-write caps, the missing
writeBodyLimitonPOST /writeFiles,eventLoopLagSnapshot()wired to/readyz, andp999Ms+event_loop_stall— landed in PR #193.The decision
The repo owner chose ceiling now, worker thread later. The worker-thread migration is tracked as #198, scoped against the existing python3/js-exec worker-bridge plan (which addresses a different limit and does not help here).
What it does
MAX_EXEC_FILE_BYTES, default 8 MiB, enforced insideSqlFsand scoped tobash.execby anAsyncLocalStoragecontext. Over-cap reads and writes are refused asEFBIG→ 413.bash.execMAX_EXEC_FILE_BYTES(8 MiB)MAX_FILE_WRITE_BYTES(50 MiB), unchangedThe asymmetry is deliberate: moving bytes over HTTP costs a buffer copy; rebuilding them through a shell pipeline costs seconds of GC. A file too large for a script to read is still retrievable whole via
GET .../files/*path.Why 8 MiB
sed s///gblocks the event loop 706 ms at 8 MiB and 2238 ms at 16 MiB;tr5563 ms at 49 MiB. Past ~2 s it stops being a latency problem:commandTimeout: 2000means the stall times out in-flight Redis commands belonging to other tenants (observed asrw_lock_writer_release_erroragainst an unrelated sandbox). Thesedthreshold for that is ~14 MiB. 8 MiB sits a factor of two under it rather than at it, because per-byte cost varies by utility and the ceiling must hold for the worst.The error message is part of the deliverable
The caller is an agent that has to recover without a human, so it states the limit is deliberate and retrying is pointless, gives both numbers, names the concrete smaller calls (
head -c,split -b,sed -n '1,20000p';PATCH .../files/*pathfor writes), and names the env var for an operator reading the same line in a log. A test asserts those parts are present so a refactor cannot degrade it to a bare code.Performance
Enforcement is O(1) on the passing path — a store lookup and an integer compare before any blob is fetched. No extra database round trip, no allocation. An over-cap read never fetches the blob.
Verification
pnpm typecheckclean,pnpm lint:fixclean,pnpm test:unit1408 passed / 4 skipped (base 1371 + 37 new), nothing pre-existing broken.sql-fs.tsfails 6+ of the 16 (rejects readFile of an over-cap file with EFBIG,does not fetch the blob for an over-cap read,rejects an append whose RESULT crosses the cap, counting the existing bytes); restored, 16/16.Not verified
The implementing agent was interrupted before it reported, so the bystander-latency measurement it was asked for (probing a different sandbox and owner every 5 ms during a large
sed) was not completed, andconcurrency.mjswas not re-run on this branch by it. The orchestrator verified the gate and the revert independently. Benchmarking of the full stack againstmainis being done separately and will be reported on the stack.Summary by cubic
Closes #168. Sandbox
execpreviously had no file-size ceiling; it now rejects reads and writes above 8 MiB (MAX_EXEC_FILE_BYTES) withEFBIGmapped to 413, preventing large shell transformations from blocking other tenants' Redis commands. Scripts that process larger files will fail, while HTTP and MCP file routes keep their existing limits.Issue #168 safeguards
/writeFilesnow enforces streaming body and bulk-size limits, and/readyzexposes event-loop lag with a stall signal.EFBIGexplains why retrying will not help and suggests smaller commands such ashead -candsplit -b.bash.execoff the main thread remains the longer-term fix.Stacked reliability fixes
retryabledurability flag; driver faults and poisoned Redis version keys recover without killing or wedging the replica.maxmemory-policy allkeys-lru; startup warns when the policy is unsafe.Written for commit a465a65. Summary will update on new commits.