Repository navigation
s3: keep response metadata across progress callbacks - #33501
Conversation
The HTTP client hands `HTTPResponseMetadata` to a task exactly once, on the first progress callback that carries response headers (`to_result()` does `state.cloned_metadata.take()`). A response body delimited by connection close (no Content-Length, no Transfer-Encoding) reports progress once with the headers and again at EOF, and the second callback carries no metadata. `S3HttpSimpleTask::http_callback` overwrote `this.result` wholesale on every callback, dropping the stored metadata, so `on_response` found `None` and unwrapped it, aborting the process on a legal HTTP/1.1 200. Carry the earlier metadata across the assignment, matching what `install::NetworkTask::notify` and `FetchTasklet::callback` already do.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 2:08 PM PT - Jul 6th, 2026
❌ @robobun, your commit d949f7a has some failures in 🧪 To try this PR locally: bunx bun-pr 33501That installs a local version of the PR into your bun-33501 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal carry-forward matching the sibling FetchTasklet/NetworkTask idiom, with solid multi-callback test coverage.
Extended reasoning...
Overview
This PR fixes a server-triggerable process abort in Bun.S3Client when receiving a close-delimited HTTP/1.1 response body (200 with no Content-Length, no Transfer-Encoding, body ends at FIN). The Rust change is 4 effective lines in S3HttpSimpleTask::http_callback (src/runtime/webcore/s3/simple_request.rs): .take() the previously-stored metadata before the wholesale this.result = ... assignment, then restore it if the incoming result carries none. The test file adds three new concurrent cases (.text(), .bytes(), 256KB body) and hoists the existing envWithoutProxy object into a shared const.
Security risks
None. The change is purely additive state preservation in an internal callback — no new parsing, no new inputs, no auth/crypto surface. It removes a remote-triggerable abort (a well-formed 200 from any S3-compatible endpoint could SIGABRT the process), so the security posture strictly improves.
Level of scrutiny
Low-to-medium. While http_callback runs on the HTTP thread and the surrounding function contains unsafe, the added lines themselves are safe Rust operating on an owned Option field. I verified the fix is byte-for-byte the same idiom already used by the two sibling HTTPClientResultCallback consumers: FetchTasklet.rs:2337 (prev_metadata = task_ref.result.metadata.take() → .or(prev_metadata)) and install/NetworkTask.rs:315 (saved_metadata = this.response.metadata.take()). This PR simply brings the S3 path in line with the established pattern; it is not a novel design.
Other factors
The tests follow harness conventions precisely: tempDir, bunExe(), {...bunEnv, ...}, server.listen(0, ...), concurrent Promise.all pipe drain, combined {stdout, exitCode} assertion before checking signalCode, and test.concurrent.each for the variant matrix. The 256KB body case ensures metadata survives many progress callbacks, not just two. The envWithoutProxy extraction is a pure dedup of code that already existed in the file. No CODEOWNERS cover these paths, no outstanding reviewer comments, and the bug-hunting system found nothing. The PR description also correctly scopes out the related AsyncHTTP::send_sync issue as needing a different fix shape.
|
On the The reasoning, for whoever picks this up: #19076 reports a catchable bun.assert(this.result.metadata != null); // debug-only, compiled out in release
const response = this.result.metadata.?.response; // null unwrap = UB under ReleaseFastWith The alternative path to that identical message is That fits the reporter's own observation that their standalone reproduction works while the production cluster does not: close-delimited framing (no What would confirm it: a |
There was a problem hiding this comment.
LGTM — this mirrors the exact metadata-preservation idiom already used in install::NetworkTask::notify and FetchTasklet::callback, fixing a process-abort on close-delimited S3 responses.
Extended reasoning...
Overview
This PR is a 4-line fix in src/runtime/webcore/s3/simple_request.rs (S3HttpSimpleTask::http_callback) that preserves result.metadata across successive HTTP progress callbacks, plus ~100 lines of new regression tests in test/js/bun/s3/s3-connection-close.test.ts. The bug was that a close-delimited HTTP/1.1 200 response (no Content-Length, no Transfer-Encoding) delivers headers on an early callback and completion on a later one with metadata: None; the wholesale this.result = result assignment dropped the earlier metadata, and on_response then panicked on .unwrap(), aborting the whole process.
Security risks
None introduced. The change actually removes a DoS vector: previously any server Bun's S3 client connected to could abort the process with a well-formed 200. The fix touches no auth, crypto, signing, or input-validation logic — it only carries an already-owned Option<HTTPResponseMetadata> forward across an assignment.
Level of scrutiny
Low-to-moderate. While http_callback runs on the HTTP thread and sits next to unsafe lifetime-extension and MaybeUninit writes, none of that is touched. The added lines are plain safe Rust (Option::take() → assign → restore-if-none) and are byte-for-byte identical to the established pattern in src/install/NetworkTask.rs:315-323 and structurally the same as FetchTasklet.rs:2337-2361. There is no new ownership hazard: .take() moves the value out before the old this.result is dropped, and if the new result already carries metadata, previous_metadata simply drops at scope exit.
Other factors
- The PR description is unusually thorough: it traces the root cause through
to_result()/state.cloned_metadata.take(), cites the sibling consumers that already handle this, argues why the remainingunwrap()is now a provable invariant, and explicitly scopes outAsyncHTTP::send_sync(a related-but-distinct issue) with justification. - Tests follow repo conventions exactly (
tempDir,bunEnvspread, concurrent pipe drain viaPromise.all,test.concurrent.each, hermetic in-process TCP server, assertions on stdout beforesignalCode). The 256KB variant covers the multi-callback case, not just the two-callback minimum. - The existing
envWithoutProxyrefactor is a pure de-duplication of an inline object literal already in the file. - The bug-hunting system found no issues.
CI status: the diff is green; the red lane is Buildkite infrastructureThis needs a maintainer to merge past a red Build #69106 (
Build #69020 ( The test added here ran and passed on every lane that scheduled it: Across both builds, zero failures mention I've used my one re-roll, so I'm not pushing another |
## Problem `test/cli/install/bun-create.test.ts` crashed in CI (build 74166, Ubuntu 25.04 aarch64) during the api.github.com outage on 2026-07-16: ``` panic: called `Option::unwrap()` on a `None` value ``` The trigger is a close-delimited HTTP/1.1 body (no `Content-Length`, no `Transfer-Encoding: chunked`, body ends at FIN). `handle_response_body_from_multiple_packets` enters its processing block on every packet for such a body (the `content_length.is_none()` term) and returns `true` whenever the output buffer is non-empty, so `progress_update` fires once per packet regardless of whether the consumer asked for streaming. `to_result()` attaches the response metadata only to the first of those callbacks. Every non-streaming `HTTPClientResultCallback` consumer then has to be taught the same stash-and-reassemble dance. #33501 did it for `S3HttpSimpleTask`; `NetworkTask::notify` already does it; `send_sync` and `RemoteImageDownload::on_done` do not, so a close-delimited body split across packets either hands them a truncated body, panics `send_sync` on the missing metadata, or writes to state they have already freed. ## Fix Gate the per-packet progress return on the same streaming signals that already select per-packet delivery everywhere else: ```rust return Ok(is_done || (processed && is_streaming)); ``` where `is_streaming` is `ResponseBodyStreaming || body_receive_mode.is_some()`. The block is still entered per packet so incremental decompression keeps running; only the `progress_update` trigger is suppressed. Non-streaming consumers now receive exactly one terminal callback with `metadata: Some` (because `cloned_metadata` is still held when `on_close` drives the terminal `progress_update`). Streaming consumers are unchanged: `fetch` wires `body_receive_mode` via `to_with_backpressure()`, `NetworkTask` sets `response_body_streaming`, and the S3 download stream calls `enable_response_body_streaming()`. In `send_sync` the invariant is now asserted (`debug_assert!(!result.has_more)`), and the `metadata.unwrap()` becomes a `ConnectionClosed` error so a future regression surfaces as a catchable CLI error rather than a process abort. `RemoteImageDownload::on_done` is covered by the same change with no edits needed. `create_command::fetch_all` replaces its chained `.unwrap()`s on registry-supplied `version`/`description` with optional lookups that default to empty (display-only values). ## Test A hermetic test in `bun-create.test.ts` points `GITHUB_API_DOMAIN` at a local TLS server that serves a valid gzip tar with `content-type: application/x-gzip`, no `Content-Length`, and the body dripped across twelve `setImmediate`-separated writes with Nagle disabled. On main this fails with a truncated-body `ZlibError` (or the unwrap panic depending on scheduling); with the fix the full tarball is received and extracted. ``` # 30 iterations on the unfixed build: 0 pass / 30 fail USE_SYSTEM_BUN=1 bun test test/cli/install/bun-create.test.ts -t close-delimited bun bd test test/cli/install/bun-create.test.ts # 15 pass bun bd test test/cli/install/bun-upgrade.test.ts # 8 pass bun bd test test/cli/install/bun-audit.test.ts # 16 pass bun bd test test/js/bun/s3/s3-connection-close.test.ts # 4 pass # close-delimited fetch() smoke: buffered body intact, stream still per-chunk status: 200 body: "Hello, close-delimited World!" stream chunks: 10 body: "Hello, close-delimited World!" ``` --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
What
Bun.S3Clientaborts the whole process (Rust panic →SIGABRT, uncatchable from JS) on any successful response whose body is close-delimited: a plain HTTP/1.1200with noContent-Length, noTransfer-Encoding, andConnection: close+ FIN. That is a legal response (RFC 9112 §6.3, rule 8) and exactly what HTTP/1.0 upstreams and framing-stripping reverse proxies in front of S3-compatible services emit, so a server Bun connects to can take the process down with a well-formed200.…and under a debug build, the assertion that encodes the violated assumption:
Cause
HTTPClient::to_result()handsHTTPResponseMetadatato a task exactly once, viastate.cloned_metadata.take()— on the first progress callback that carries response headers.When the body is close-delimited,
content_lengthisNone, sohandle_response_body_from_multiple_packetsreports progress on every read. The headers therefore ship on an early callback (has_more = true,metadata = Some), and the completing callback arrives separately at EOF carryingmetadata: None.S3HttpSimpleTask::http_callbackoverwrotethis.resultwholesale on every callback, so that assignment dropped the stored metadata.on_responsethen didthis.result.metadata.as_ref().unwrap()and aborted.Responses with a
Content-LengthorTransfer-Encoding: chunkeddeliver metadata and completion on the same callback, which is why this only ever showed up on close-delimited bodies.Fix
Carry the earlier metadata across the assignment. This is the idiom the sibling consumers of
HTTPClientResultCallbackalready use —install::NetworkTask::notifyandFetchTasklet::callbackboth preserve metadata across callbacks, ands3::download_streamlatches the status code instead; onlys3::simple_requestdropped it.With metadata preserved, the
debug_assert!+unwrap()inon_responsebecome a provable invariant for this path: for S3 theCertErrorssignal is never enabled, socertificate_infois alwaysNoneand cannot pre-empt metadata into_result(); and a terminal success dispatch requiresstate.is_done(), which is only ever reachable downstream ofclone_metadata().Verification
Covers
.text(),.bytes(), and a 256KB body that spans several progress updates so the headers land many callbacks behind the completion. All three abort with exit 134 on the unfixed build and pass on the fixed one; the large body comes back byte-intact, which is what proves the metadata survives repeated callbacks rather than just the two-callback case.Not fixed here
AsyncHTTP::send_synchas a related problem with the same trigger:SingleHTTPChannel::write_itemreplaces the stored result on each callback, andsend_syncdoes a real (non-debug)result.metadata.unwrap(). A close-delimited response can therefore panic it in release, or unblock the reader on the partial first result. It needs completion-gating rather than a carry-forward, has its own copy-back ordering hazard aroundreal.response_headers, and its callers (bun create,bun publish,bun pm view,bun audit) only ever talk to registries that frame their responses, so it is left for a separate change.The
list()path is affected by this crash too, but its parsed result is separately truncated by a scan-window bug already fixed in #33497, solist()is not asserted on here.