fetch/S3: replace ResumableSink with proper JSSinks - #36087
Code review found 1 important issue
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/runtime/webcore/ResumableSink.rs:394-402 |
controller.error() with no argument clean-closes instead of aborting |
| 🟡 Nit | src/jsc/bindings/webcore/streams/BunStreamSource.cpp:1571-1575 |
Destructured controller methods throw on the direct-ResumableSink path |
| 🟡 Nit | src/jsc/bindings/webcore/streams/BunStreamSource.cpp:1553-1560 |
resumableSetupDirect drops the underlyingSource.close() lifecycle hook |
| 🟡 Nit | test/js/web/fetch/fetch.test.ts:2540 |
Per-test 15000 timeout still present |
Annotations
Check failure on line 402 in src/runtime/webcore/ResumableSink.rs
claude / Claude Code Review
controller.error() with no argument clean-closes instead of aborting
After 67a25c5 remapped `error` → `jsEnd` (and `end`/`close` → `jsClose`), `js_end` is reachable **only** via `controller.error(...)` — but it still branches on `args.len() > 0`, a leftover from when it also served zero-arg `end()`. So `controller.error()` with no argument yields `err = None` → `on_end(context, None)` → `FetchTasklet::write_end_request(None)` → sends the terminating chunk and cleanly finishes the upload, whereas pre-PR `boundDirectError` read `callFrame->argument(1)` (= `jsUndefi
Check warning on line 1575 in src/jsc/bindings/webcore/streams/BunStreamSource.cpp
claude / Claude Code Review
Destructured controller methods throw on the direct-ResumableSink path
The pre-PR path handed `pull()` a `JSDirectStreamController`, which installs `write/end/close/flush/error` as **own `JSBoundFunction` properties** (see `installDirectControllerMethods`), so `const {write, close} = c; write('hi'); close()` worked. Here `pull()` now receives the ResumableSink, whose methods are prototype entries with the default `invalidThisBehavior: Throw` — a destructured call reaches the codegen thunk with `thisValue()===undefined`, throws `createInvalidThisError`, the pull rej
Check warning on line 1560 in src/jsc/bindings/webcore/streams/BunStreamSource.cpp
claude / Claude Code Review
resumableSetupDirect drops the underlyingSource.close() lifecycle hook
`resumableSetupDirect` reads only `pull` and wires only `cancel` from the underlying source, so a hand-written `{type:'direct', pull, close}` fetch/S3 body no longer sees its `underlyingSource.close(reason)` hook fire on `controller.close()/end()/error()` — pre-PR the materialized `JSDirectStreamController` invoked it via `callUnderlyingSourceClose` on both `onClose` and `handleError`. The async-iterator wrapper is coincidentally masked (its `asyncIterFinishSuccess` independently returns the ite
Check warning on line 2540 in test/js/web/fetch/fetch.test.ts
claude / Claude Code Review
Per-test 15000 timeout still present
The explicit `15000` per-test timeout violates `test/CLAUDE.md` ("**CRITICAL**: Do not set a timeout on tests. Bun already has timeouts.") and is redundant — the body already self-bounds via `Promise.race([proc.exited, sleep(isDebug ? 8000 : 2500)])` + `proc.kill(9)`, so it completes in ≤~8s regardless. The earlier CodeRabbit thread was marked "✅ Addressed in commit 4c9c022" but the `}, 15000);` is still present — drop the second `it()` argument.