s3: abort in-flight tasks at VM teardown by request id instead of reading task.http - #38353
s3: abort in-flight tasks at VM teardown by request id instead of reading task.http#38353robobun wants to merge 1 commit into
Conversation
…ding task.http While an S3 request is in flight, the HTTP thread bitwise-overwrites the task's `http` field on every progress callback (stage_http_result / update_state). Both stop_for_vm_teardown impls ran on the JS thread in that window and read `http.async_http_id` through `http.assume_init_ref()`, a data race on the field (benign in practice because the id never changes). Give S3HttpSimpleTask the `async_http_id` field S3HttpDownloadStreamingTask already had for its cancel path, capture it in a new `schedule` on each task (which also owns the hand-off: write `http`, queue it, embedded-work count, active-handle registration), and make both teardown paths call schedule_shutdown_by_id with it. The three scheduling sites in client.rs and execute_simple_s3_request now go through `schedule`, so nothing outside the task files touches `http`. A source lint pins that: `http` may only be dereferenced in `schedule`, the HTTP-thread overwrite, and Drop. Two worker-terminate tests cover the by-id abort for each task type with two requests in flight each, since request ids start at 0 and a single request would be aborted even by an uncaptured id.
|
Warning Review limit reached
Next review available in: 7 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 (5)
Comment |
|
Status: ready for review. Reproduced by reading: on main, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| env: envWithoutProxy, | ||
| stdin: "pipe", | ||
| stdout: "pipe", | ||
| stderr: "inherit", |
There was a problem hiding this comment.
🟡 await bothInFlight.promise is only ever resolved from the server's onResponded callback — nothing wires the child's exit to reject it, so if the spawned child dies before both requests land (worker eval throws, S3Client init fails, subprocess crashes) the test hangs to the file timeout instead of failing fast. Consider racing it against proc.exited, e.g. await Promise.race([bothInFlight.promise, proc.exited.then(c => Promise.reject(new Error('child exited early with ' + c)))]). (Diagnostics-only: the test can't spuriously pass, it just times out with a worse error signal.)
Extended reasoning...
What the bug is
The new "terminating a worker with buffered S3 downloads in flight aborts them" test creates a Promise.withResolvers() pair bothInFlight, resolves it from stalledS3Server's onResponded callback once two requests have arrived, and then does a bare await bothInFlight.promise; before telling the child (over stdin) to terminate the worker. Nothing ever rejects bothInFlight — in particular the spawned child's exit is not wired to it.
REVIEW.md's test rules call this shape out directly: "Wire EVERY failure event (error, close, abort, process exit) to reject the awaited promise."
Step-by-step: how it hangs
bothInFlight = Promise.withResolvers()andresponded = 0are set up.- The stall server is started; its
datacallback doesif (++responded === 2) bothInFlight.resolve(). Nothing anywhere callsbothInFlight.reject. - The child is spawned with
await using proc = Bun.spawn(...). Theawait usingdisposer only runs when control leaves the block. - Suppose the worker's eval'd body throws (e.g.
Bun.S3Clientconstruction ors3.file(key).text()throws synchronously in a future refactor), or the child process crashes before making both HTTP requests. The child exits;proc.exitedsettles; the local server never sees a seconddataevent. - Control is parked at
await bothInFlight.promise;(line 2660). Nothing settles it.proc.exitedis not awaited yet, andawait usingdisposal cannot run because the scope has not been left. - The test hangs until the test/file timeout fires.
stderr: "inherit"may print something useful, but the failure is reported as a bare timeout with no assertion.
Why existing code doesn't prevent it
The sibling streaming-download test and the pre-existing upload test go straight to Promise.all([proc.stdout.text(), proc.exited]) with no intermediate test-side await, so an early child exit settles both and produces an immediate expect(stdout).toBe(...) mismatch. The buffered case is different because a buffered download exposes nothing observable to JS mid-flight, so the test adds an out-of-band signal (server → test) — but that signal has no failure branch.
Impact
Diagnostics-only. The test cannot pass spuriously; it can only degrade a real failure into a slow, opaque timeout instead of a fast assertion with the child's exit code. So this is nit severity.
Fix
Race the readiness signal against the child's lifecycle, e.g.:
await Promise.race([
bothInFlight.promise,
proc.exited.then(code => Promise.reject(new Error(`child exited (${code}) before both requests were in flight`))),
]);(or equivalently proc.exited.then(c => bothInFlight.reject(...)) set up before the await). Either way, an early child death now fails the test immediately with a message that names what happened.
|
Updated 2:04 AM PT - Aug 14th, 2026
✅ @robobun, your commit 748ccd161e61b22f5cfc3f6057626c2e1dda43fd passed in 🧪 To try this PR locally: bunx bun-pr 38353That installs a local version of the PR into your bun-38353 --bun |
|
Relaying a review note that landed on #38361 (stacked on this PR) but concerns this PR's test, "terminating a worker with buffered S3 downloads in flight aborts them" in test/js/node/worker_threads/worker_threads.test.ts:
await Promise.race([
bothInFlight.promise,
proc.exited.then(code => Promise.reject(new Error(`child exited (${code}) before both requests were in flight`))),
]);(REVIEW.md, "wire every failure event to reject the awaited promise".) The streaming variant does not need it: it only awaits stdout and exit. |
1 participant
Problem
AsyncHTTPinline in ahttpfield. While a request is in flight the HTTP thread bitwise-overwrites that whole field on every progress callback:S3HttpSimpleTask::stage_http_result(src/runtime/webcore/s3/simple_request.rs:409, no lock at all) andS3HttpDownloadStreamingTask::update_state(src/runtime/webcore/s3/download_stream.rs:210, under the task'smutex).stop_active_handles, also run at thebun test --isolateglobal swap) calls each registered task'sstop_for_vm_teardownon the JS thread. Registered means the request is still out on the HTTP thread, and both impls didhttp_thread().schedule_shutdown((*this).http.assume_init_ref())(simple_request.rs:483, download_stream.rs:355): a&AsyncHTTPover the field, and a read ofhttp.async_http_idthrough it, concurrent with the other thread'sptr::writeof the same bytes, with nothing ordering the two. That is a data race; it is benign natively only because the id has the same value in every copy.async_http_idsoS3DownloadStreamWrapper::on_stream_cancelledaborts by id without touchinghttp. The teardown path added later in Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 went back to readinghttp, and the simple task had no captured id to use.Fix
S3HttpSimpleTaskgets theasync_http_idfield the streaming task already had. Each task type gets aschedule(this, http)that owns the hand-off: it recordshttp.async_http_id, writeshttpinto the task, queues it, bumps the VM's embedded-work count and registers the active handle (for the streaming task it also turns on body streaming, as before). The three places that used to inline that sequence (execute_simple_s3_request,list_objects,download_stream) call it instead, so the id is captured in exactly one place per type and nothing outside the two task files toucheshttpany more.stop_for_vm_teardownimpls now set the abort flag and callschedule_shutdown_by_id((*this).async_http_id). Correct because the id is whatschedule_shutdownforwarded anyway (HttpThread::schedule_shutdownisschedule_shutdown_by_id(http.async_http_id)), it is the same value the HTTP thread's copies carry (the copy is aptr::readof the original and nothing reassigns the id), and the field is written once on the JS thread before the hand-off and never by the HTTP thread, so reading it there needs no synchronisation. The field andstop_for_vm_teardowncomments now state the ownership window; the old SAFETY comments claimedhttpwas readable there.test/internal/source-lints/s3-task-http-field.test.tspins the invariant: in src/runtime/webcore/s3/, thehttpfield may only be dereferenced (assume_init*,as_ptr,as_mut_ptr,write) insideschedule, the HTTP thread's overwrite (stage_http_result/update_state) and Drop'srelease_portable; a ratchet check requires each allowed function to still touch the field. On main it reports 8 sites, including the two teardown reads above; with this change it is clean.test/js/node/worker_threads/worker_threads.test.ts("VM teardown ordering") gains two tests, one per task type, that terminate a worker with two requests in flight against a server that sends headers plus one chunk and then stalls.terminate()can only resolve if the abort reaches the HTTP thread, since that is the only thing that makes it hand the requests back. Two requests because request ids come from a counter that starts at 0, so a task that never captured its id still aborts the process's first request by accident; the existing single-upload teardown test passes even with the capture removed, while each new test hangs when its task type's capture is removed and passes with it (details below).bun bd test; the five "VM teardown ordering" tests pass on the debug build; the credential-free S3 files (s3-stream-cancel-leak, s3-stream-error-gc, s3-connection-close, s3-list-objects) pass except s3-list-objects' "fall back to NoSuchKey" case, which times out identically on an unmodified debug build of main and is tracked separately; the whole source-lints directory passes;cargo clippy -p bun_runtime --no-depsand rustfmt are clean.release_portableintoDrop(whichever lands second renames that entry in the lint's allowlist), http: move the cross-thread state out of HttpThread and claim HTTP_THREAD on the HTTP thread #37694 renames thehttp_thread()calls these lines sit on, http: widen the per-request id to 64 bits #37437 widens the id to 64 bits (the new field follows).FetchTaskletis intentionally not touched: itshttpis aBoxwhose fields the HTTP thread updates selectively viasync_progress_from, which never writes the id, so it is a different shape.Background
AsyncHTTPinto the task'shttpfield and queues it; the HTTP threadptr::reads it into its own working copy (start_queued_task), runs the request, and on each callback the task copies that working copy back overhttpso the final state is there when the JS thread eventually reads or drops the task. Between queueing and the final callback the field is therefore the HTTP thread's.async_http_idis the per-request number the HTTP thread keys its abort tracker by.schedule_shutdown_by_idpushes it onto a queue and wakes the HTTP thread, which closes the matching socket; the request then fails withAbortedand is handed back like any other completion. Ids are handed out from a process-wide counter starting at 0, and only requests created with an abort signal (every S3 request, every fetch) get one and enter the tracker.terminate(),process.exit()) first stops every registered active handle, then waits for the HTTP thread to hand back every request the VM still has out (wait_for_embedded_work) before it closes its task queue. A request whose abort never reaches the HTTP thread therefore blocks teardown until the server ends it.Lint on main, and the negative runs behind the two worker tests
Lint against main (
git stash push -- src/ && bun bd test test/internal/source-lints/s3-task-http-field.test.ts):With the
(*this).async_http_id = http.async_http_id;line deleted fromS3HttpDownloadStreamingTask::schedule(debug build):With the same line deleted from
S3HttpSimpleTask::scheduleinstead:The pre-existing upload test passes in that second run because the upload is the process's first abortable request and so has id 0, the same value as the uncaptured field. With a single download the new tests passed the same way, which is why they use two. With both captures in place all five "VM teardown ordering" tests pass (about 3 s each on the debug build, dominated by worker startup).