Skip to content

fix(relay): bound request bodies without cancelling mutations (#4424) - #7753

Draft
MajorTal wants to merge 5 commits into
block:mainfrom
MajorTal:codex/issue-4424-body-timeouts
Draft

MajorTal wants to merge 5 commits into
block:mainfrom
MajorTal:codex/issue-4424-body-timeouts

Conversation

@MajorTal

@MajorTal MajorTal commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #4401, split out per Carl's review there. buzz-relay bounds request bytes but not request time (#4424): valid headers followed by a withheld body parks a task per socket, pre-auth, on the API and media routers.

What changes

One principle: bound reception, never the handler. Every deadline in this PR is a body read error. No timer wraps a handler future that writes.

API router. A middleware collects the whole request body up front under a 60 s idle deadline (resets on every frame), a 300 s reception ceiling (so a client trickling one byte per idle window cannot hold a pre-auth task forever) and the existing 1 MiB limit. It answers 408 for a body that does not arrive in time and 413 for an oversized one before any handler runs. An over-limit Content-Length is refused without reading a byte, as the old limit layer did. The handler gets a buffered body; every API write handler extracts its body in full already (Bytes, or Json for the mesh demo echo), so handlers see what they saw before.

That is the fix for the P1 in #4401: ingest_event inserts, dispatch_persistent_event awaits the bounded audit queue, and only then spawns fanout. There is no deadline around that anymore. A test pins it: a handler that outlives both deadlines tenfold after a complete body still completes.

Media uploads. Same idle bound (60 s between frames, so a slow but progressing upload finishes) plus a 3600 s reception ceiling matching the Blossom auth window, which bounds how long a trickling client can hold an upload permit. The ceiling used to be a TimeoutLayer around the whole upload future; that could drop the future after the blob and sidecar were published and before the audit entry was enqueued. It is now enforced only while body frames are still being read, so once the last frame is in, the storage writes, upload record, sidecar publish and audit enqueue cannot be cancelled. A test pins that as well.

Media reads. 300 s wall deadline, answered 504. No body, no mutation; a hung storage read no longer parks a task. 504 and not 408 because nothing on that path waits on the client.

Unchanged: admin router, git policy router, SPA fallback, health listener. Header-read deadlines before routing (hyper level) remain open on #4424.

Tests

  • API guard: withheld body answers 408 at the bound and the handler never ran; trickled body answers 408 at the ceiling; complete body then slow handler completes; oversized body answers 413; declared oversize answers 413 without the body being read; a dropped stalled request never completes a handler; WebSocket upgrade through the guard survives past both deadlines.
  • Media: slow-but-progressing body completes past the idle bound; body that outlives the ceiling answers 408 mid-body; complete body then slow handler is never cancelled by the ceiling; withheld body 408; over-limit 413; stalled no-body read bounded by the read deadline with 504; legacy /media/upload alias stays under the upload guards; the body error classifier and the video-stream timeout conversion are pinned in buzz-media.
  • The stall tests are wrapped in a five second bound so a regression fails instead of hanging the suite.
  • All of these run in just test-unit now (router::tests::body_guards:: and the two buzz-media tests are added to its nextest expressions). Before, no hosted lane executed them.

Review done before this push

Block's gates and reviews, run locally on this exact head, macOS Apple Silicon:

  • just fmt-check, just clippy (workspace, -D warnings), just test-unit (cargo-nextest), cargo-deny check, just file-size-check, just security-review-check: clean.
  • Postgres lane (scripts/postgres-test-run.sh against Postgres and Redis containers): 390 passed.
  • A reviewer run with the repository's Codex security review prompt over the range: one LOW finding, the upload ceiling cancellation described above. Fixed here.
  • An adversarial review shaped like Carl's, tracing every timer against ingest_event and dispatch_persistent_event: the old P1 does not apply to this design. Its non-blocking items (API trickle, 408 for server-side read stalls, stall tests that hang instead of fail, guard tests outside every CI lane, comments citing notes that are not in the repository) are all fixed here.

Hosted Rust CI needs a maintainer to approve the workflow for this fork branch.

Stacked on #4401 (its four commits appear here until it merges).

Refs #4424

🤖 Generated with Claude Code

Signed-off-by: Tal Weiss <major.tal@gmail.com>
@github-actions

github-actions Bot commented Sep 19, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 5511b56fcf0047f9d0d4887dc75125b6933a6bbd...b2dca8fdc3cf52edd8922ccfb7c61646765a032b.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review b2dca8fdc3cf52edd8922ccfb7c61646765a032b to authorize a new review.
Any previous review applies only to its recorded range.

MajorTal and others added 4 commits September 19, 2026 14:17
Git sends the same unauthenticated four-byte flush-packet probe before
any smart HTTP POST whose body exceeds http.postBuffer — not just large
pushes. Fetch negotiations cross the 1 MiB decoded threshold when the
client's divergent local history contributes enough have lines (an
in-limit repo plus ~21k unpushed divergent branches suffices; measured
live at MAX_MANIFEST_REFS = 10_000 with divergent client history:
1.46 MiB decoded negotiation, probe fired, fetch got 401).

Parameterize the receive-pack probe middleware over the service's
(request MIME, result MIME) pair — the predicate was already otherwise
service-agnostic — and mount it on the upload-pack POST route as well.
All other predicate legs are unchanged: no Authorization /
Content-Encoding / Transfer-Encoding headers, a single exact
Content-Length of 4, and an exact 0000 body; near misses fall through
to the normal authenticated path.

Tests: the existing exactness/path-independence matrix now runs against
both services (2 path shapes + 11 near-misses each), plus a
cross-service case proving each route rejects the other service's
request MIME.

Fixes block#4423

Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
Signed-off-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
Signed-off-by: Tal Weiss <major.tal@gmail.com>
Admitting the 0000 probe means reading four unauthenticated body bytes.
Give that read its own 60 s deadline that fails closed into the normal
authenticated path, so a client that sends the probe headers and then
withholds the body cannot park a task. The wider request-time policy for
the API and media routers is a separate change (block#4424).

Co-authored-by: npub1qyvc0c5kl4gqv2fd97fsk46tu378sqgy35vc83rvgfwne90sel7s0ed67d <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Tal Weiss <major.tal@gmail.com>
The unit lane selects buzz-relay tests by explicit nextest expression, and
none of its terms matched api::git::transport::track_c_tests, so the four
compatibility-probe tests were compiled by clippy but executed by no hosted
job. They need no database or storage, so they belong in the infra-free lane.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Tal Weiss <major.tal@gmail.com>
buzz-relay bounded request bytes but never request time (block#4424): valid
headers followed by a withheld body parked a task per socket, before
authentication, on the API and media routers.

API router: a middleware collects the whole request body up front under a
60 s idle deadline, a 300 s reception ceiling and the existing 1 MiB limit,
answers 408 for a body that does not arrive in time and 413 for an
oversized one before any handler runs (an over-limit Content-Length is
refused without reading a byte), and hands a buffered body to the handler.
Handlers themselves run with no deadline: ingest_event inserts,
dispatch_persistent_event awaits the bounded audit queue and only then
spawns fanout, so a wall deadline around that future could strand a
committed event with no audit and no delivery while a retry is answered
duplicate. A test pins that a slow handler after a complete body is never
cancelled.

Media uploads: an idle body timeout (60 s between frames) so a slow but
progressing upload finishes while a withheld body fails closed with 408,
plus a 3600 s reception ceiling, matching the Blossom auth window, that
bounds how long a trickling client can hold an upload permit. Both surface
as body read errors and neither is a timer around the handler: once the
last frame is in, the storage writes, upload record, sidecar publish and
audit enqueue cannot be dropped, and a test pins that too. Media reads: a
300 s wall deadline answered 504; no body, no mutation. Body errors are
classified (reception deadline, length limit, other) so 408 and 413 are
never reported as storage failures.

The guard tests need no infrastructure and run in the unit lane.

Unchanged: admin router, git policy router, SPA fallback, health listener.
Header-read deadlines before routing remain open on block#4424.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Tal Weiss <major.tal@gmail.com>
@MajorTal
MajorTal force-pushed the codex/issue-4424-body-timeouts branch from f3382e2 to b2dca8f Compare September 19, 2026 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant