inspector: distinguish backpressure and drops in the debugger WebSocket writer - #34220
inspector: distinguish backpressure and drops in the debugger WebSocket writer#34220hughescr wants to merge 6 commits into
Conversation
ws.sendText() (the uWS binding backing the inspector debugger socket) has a documented three-way return contract: -1 (BACKPRESSURE, message accepted into uWS's outbound buffer but the buffer is running high), 0 (DROPPED, message not sent at all), or a positive byte count (SUCCESS). webSocketWriter previously collapsed this with `!!`, which coerces -1 to `true` -- indistinguishable from success -- so bufferedWriter (which only requeues a message when write() returns falsy) never reacted to backpressure at all: the coercion silently lied about what sendText() actually returned. This distinguishes all three cases and adds real pacing behavior for backpressure: bufferedWriter now queues (pauses) further writes while backpressured, flushing them in order once drain() reports the connection has room again -- so a burst of events under backpressure is paced rather than blindly hammering an already-strained socket. This intentionally does NOT re-send the specific message that reported backpressure. Per uWS's own send() contract, BACKPRESSURE means that message was already accepted into the outbound buffer and will still be delivered -- unlike DROPPED (0), which means it was discarded and must be retried. Re-sending an already-accepted message would duplicate it on the wire, a worse bug than the one being fixed. Only a genuine DROPPED result is queued for retry, matching bufferedWriter's pre-existing behavior for that case. While extending drain()'s tri-state handling, also fixed an adjacent latent bug: pendingMessages was never cleared after a fully successful drain pass, which would have re-sent already-delivered messages on the next drain -- a duplicate-send bug of its own, and one now far more likely to trigger given backpressure also routes through this queue. Adds a regression test to test/cli/inspect/test-reporter.test.ts that drives the debugger socket into genuine backpressure (pausing reads before the subprocess starts emitting) and asserts every found/end event arrives exactly once and in non-decreasing id order. This is the fuller alternative to the minimal one-line coercion fix (`sendText(message) !== 0`, staged separately) -- see PR body for verification status and how to choose between the two. Claude-Session: https://claude.ai/code/session_01N8rB9z5epigmEMvKbJFuQ1
`webSocketWriter`/`bufferedWriter` (src/js/internal/debugger.ts) back the
`ws:`/`ws+unix:` debugger transport (Bun.serve's `#websocket` handler). This
tightens their contract in three ways:
- `webSocketWriter` now surfaces `ws.sendText()`'s real tri-state result
("backpressure" / "dropped" / "success") instead of coercing it with `!!`,
which collapsed BACKPRESSURE (-1, message already accepted -- do not
resend) and DROPPED (0, message never sent -- must retry) into the same
truthy/falsy signal.
- `bufferedWriter` paces (queues) further writes on backpressure without
resending the message that reported it, and requeues on a genuine drop.
- `bufferedWriter.write()` now also gates on `pendingMessages.length > 0`,
not just the `paced` flag: a "dropped" result queues a message without
setting `paced`, so a later write could otherwise reach the wire ahead of
a still-queued retry. `pendingMessages` is also fully cleared once a
drain fully succeeds, so a later no-op drain can't resend stale entries.
`ws.sendText()`'s `result === 0` is ambiguous in the underlying Rust binding
itself (DROPPED vs. a genuine zero-byte SUCCESS or a closed-socket
short-circuit); treating every 0 as "dropped" is safe here specifically
because CDP's `sendMessageToFrontend` never hands this writer an empty
string in the first place -- documented in a comment at the call site.
`bun:internal-for-testing` now exposes the real `webSocketWriter`/
`bufferedWriter` implementations (via a `testHooks` property riding on
debugger.ts's single permitted default export) so the accompanying unit
tests exercise the shipped code directly rather than a reimplementation
that could drift from it. test/cli/inspect/test-reporter.test.ts adds
scripted-writer unit tests for the pacing/requeue/ordering/drain-clearing
behavior above, plus a real `ws://` integration test proving the production
`bufferedWriter(webSocketWriter(ws))` wiring delivers every TestReporter
event exactly once and in order over an actual connection. This is not
claimed to fix any specific CI incident -- it's a correctness fix for the
writer layer found via code review, verified by tests that pin the
contract going forward.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThe inspector debugger now exposes writer test hooks, uses explicit WebSocket write outcomes, updates buffering and retry ordering, and adds unit and subprocess WebSocket integration tests. Inspector writer flow
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/inspect/test-reporter.test.ts`:
- Around line 563-564: Remove the custom 30000 timeout arguments from the
spawned child test command and the corresponding outer test timeout near the
related test setup. Rely on the repository runner’s standard timeout policy,
reducing TEST_COUNT only if needed to keep the workload within the normal
debug-build budget.
- Around line 618-637: Update the completion logic around the WebSocket message
listener and donePromise so it does not resolve immediately on the 200th
TestReporter.end event. After receiving all expected end events, send a uniquely
identified protocol command and resolve only when its matching response is
received, preserving the existing foundIds and endedIds collection while
ensuring queued duplicate deliveries are observed before assertions and socket
closure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec739b6e-9d77-41a8-bdfe-57abd91ffbab
📒 Files selected for processing (3)
src/js/internal-for-testing.tssrc/js/internal/debugger.tstest/cli/inspect/test-reporter.test.ts
Removed the child's inert --timeout override from the spawned bun test argv (the outer test()'s 30000ms timeout argument is unchanged). Resolve via a protocol barrier after the final end event -- or via socket close once all end events have been observed, since the child exits immediately and close proves the stream is complete -- rejecting fast when the socket dies with events still missing. Surface captured child stderr before the exit-code assertion, so a nonzero exit fails with the child's own error output instead of a bare exit-code mismatch.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/inspect/test-reporter.test.ts`:
- Around line 616-640: Update the message handler in the test reporter barrier
flow so the BARRIER_ID branch resolves donePromise only for protocol responses
that have no method. Preserve handling of TestReporter events first, and require
both msg.id === BARRIER_ID and an absent method before calling doneResolve.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d64ba4a-05e4-480f-b8db-6466e6a20cc0
📒 Files selected for processing (1)
test/cli/inspect/test-reporter.test.ts
Per CDP framing, a response never carries a method, so gate the barrier-resolve branch on msg.method === undefined as well as the id.
* upstream/main: (70 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/js/internal/debugger.ts
|
Housekeeping for reviewers: I've just merged latest Since I have four related-but-independent PRs open (#34219, #34220, #34221, #34222), a quick map so nobody has to reconstruct it: no two of them touch the same source file. The only overlap anywhere is test coverage — #34219, this PR, and #34221 each extend |
The sticky hasQueuedAnyDebuggerMessage gate is set on the first protocol response and never clears, so every inspected process exit paid a fixed +150ms with no completion condition. The FIFO sentinel alone (which wakes as soon as the debugger thread has called write() for every queued message) is sufficient for the bug in #34218; WebSocket-level backpressure flushing is a separate concern tracked in #34220. Test passes 5/5 with the sentinel alone.
Related: #34218 (this PR fixes the "layer (b)" writer-correctness defect described there; the exit-drain half is #34219)
What does this PR do?
Fixes a correctness bug in the inspector debugger's writer layer
(
webSocketWriter/bufferedWriterinsrc/js/internal/debugger.ts), whichbacks the
ws:/ws+unix:transport used by--inspect/--inspect-waitover a real
Bun.serveWebSocket (Debugger's#websockethandler).uWS'sws.sendText()returns a three-way contract (seepackages/bun-uws/src/WebSocket.h,WebSocket::send()'s doc comment andSendStatusenum):-1(BACKPRESSURE -- the message was accepted intouWS's outbound buffer, which is just running high),
0(DROPPED -- themessage was not sent at all), or a positive byte count (SUCCESS).
webSocketWriterpreviously collapsed this three-way result with!!,coercing
-1totrueand making backpressure indistinguishable fromsuccess. Since
bufferedWriteronly requeues on a falsy return, it neverreacted to backpressure at all -- and, had it naively resent on any falsy
value instead, it would have duplicated a message that uWS says was already
accepted.
The fix, entirely in the existing writer-layer functions (no new
files):
webSocketWriternow surfaces the real tri-state result --"success"/"backpressure"/"dropped"-- instead of coercing it.result === 0is ambiguous in the underlying Rust binding itself (it'salso what a genuine zero-length-string SUCCESS or an already-closed-socket
short-circuit would return -- see
send_status_to_js/send_textinsrc/runtime/server/ServerWebSocket.rs); treating every0as"dropped"is safe here specifically because CDP'ssendMessageToFrontendskips empty messages before they ever reach thiswriter (
src/jsc/bindings/BunDebugger.cpp:217-218), so that ambiguitycan't actually arise on this path -- documented in a comment at the call
site.
bufferedWriternow paces (queues, does not resend) further writes onbackpressure, and requeues on a genuine drop.
bufferedWriter.write()also gates onpendingMessages.length > 0, notjust the
pacedflag: a"dropped"result queues the message but doesnot set
paced, so without this a laterwrite()could reach the wiredirectly and overtake a still-queued retry -- once anything is queued,
every later write has to queue behind it to preserve order.
pendingMessagesis now fully cleared once a drain fully succeeds, so alater no-op
drain()can't resend stale entries.bun:internal-for-testinggains adebuggerInternalsexport exposing thereal
webSocketWriter/bufferedWriterimplementations (via atestHooksproperty riding on
debugger.ts's single permitted default export --builtin modules under
src/js/internal/**support onlyexport default,so a second named export isn't an option). This lets the accompanying tests
exercise the shipped code directly instead of a reimplementation in the
test file that could silently drift from it.
Not claimed: this PR does not assert it fixes any specific CI incident
or reported issue. It's a correctness fix found by inspecting the writer
layer's handling of
sendText()'s return contract, landed with tests thatpin the corrected behavior going forward.
Relationship to sibling PRs: this is one of three independent PRs
against the same base (
fix/inspector-backpressure-retry) that each touchtest/cli/inspect/test-reporter.test.ts's former"delivers every TestReporter event exactly once, in order, when write() reports WebSocket backpressure"test -- a main-thread-exit-drain fix and aTestReporterid-collision fix, both found while investigating this writerbug, but with unrelated root causes and call paths. That test exercised
none of them correctly (it used
--inspect-wait=unix:, which neverconstructs a
bufferedWriter(webSocketWriter(...))in the first place), sothis PR replaces it with tests that actually cover this fix; the other two
PRs replace the same original test with their own, different coverage.
Whichever of the three lands last will need its trailing test block
reconciled by hand against whatever's already merged, since all three
diffs touch the same lines.
How did you verify your code works?
Added two new
describeblocks totest/cli/inspect/test-reporter.test.ts,replacing the old backpressure test that didn't actually exercise this code
path:
"writer layer: bufferedWriter / webSocketWriter (unit)"-- scriptedunit tests against the real implementations (via
bun:internal-for-testing'sdebuggerInternals), covering:webSocketWritermapping all threesendText()return values to thecorrect
WriteResult.bufferedWriterpacing (not resending) after backpressure.bufferedWriterrequeuing a dropped write, and blocking a later writefrom overtaking it on the wire (the ordering-fix regression case).
pendingMessagesbeing fully cleared after a successful drain (nostale resend on a later no-op drain).
through a multi-message queue.
close()clearing the pending queue and delegating to the underlyingwriter's
close().webSocketWriter+bufferedWritercomposed together, proving adropped CDP message can never be overtaken by the next one through the
real
sendText()mapping."writer layer: ws:// integration"-- a real--inspect-wait=ws://subprocess with a genuine
WebSocketclient, proving the actualproduction wiring (
bufferedWriter(webSocketWriter(ws)), constructed byDebugger's#websockethandler) delivers everyTestReporter.found/TestReporter.endevent for 200 tests exactly once and in non-decreasingid order, with no drops or duplicates, end to end over a real socket.
Honest limitation: this integration test does not force a genuine
socket-level BACKPRESSURE or DROPPED condition. The global
WebSocketclient used here (matching this suite's other
ws://tests, e.g.test/regression/issue/21654) has no public API to pause reading theunderlying socket the way a raw
Bun.connect()client can, so uWS'soutbound buffer never has a reason to back up. Forcing a genuine DROPPED
additionally needs upward of 16MB of unacknowledged backlog -- the
debugger's
#websockethandler sets nobackpressureLimit, so uWS fallsback to its 16MB default (
WebSocketServerContext.rs'sbackpressure_limitdefault) -- which isn't a clean, fast, reliable hook tobuild a test around. Both scenarios are instead covered deterministically
by the scripted-writer unit tests in (1), which drive the real
webSocketWriter/bufferedWriterfunctions with a scripted underlyingwriter that returns
"backpressure"/"dropped"on command. This test'sjob is only to prove the real production wiring delivers correctly under
normal conditions, over an actual connection.
Ran the gate three times plus a full sweep, per this repo's
CLAUDE.mdguidance to check for flakiness:
Typechecked
src/js(cd src/js && bun x tsc --noEmit) and comparederrors attributed to the two changed files specifically against an
unmodified-base run:
internal-for-testing.tshas zero errors on bothsides, and
internal/debugger.tsactually has two fewer errors on thisbranch than on base -- the widened
onData(socket: Socket<{ ...; backend: Backend | Writer }>, ...)signature resolves two pre-existingSocket<{ backend: Backend }>vs.Socket<{ backend: Writer }>mismatchesat its call sites, rather than papering over them with a cast. (A
whole-tree error-count comparison is not a reliable signal in this repo --
several thousand errors in unrelated
node/*/internal/streams/*filesvary between runs regardless of this change -- so the comparison here is
scoped to the files actually touched.)
https://claude.ai/code/session_013DexKQwDASzoRVDFS8Jtyn