inspector: fix Runtime.consoleAPICalled timestamp unit and emit count/time events over DevTools WS - #35736
Conversation
…/time events over the DevTools WebSocket Two fixes for the DevTools-protocol server that inspector.open() / --inspect exposes: - Runtime.consoleAPICalled.timestamp (and Runtime.exceptionThrown.timestamp) was forwarded verbatim from JSC's Console.messageAdded, which reports WallTime::secondsSinceEpoch(). CDP's Runtime.Timestamp is milliseconds since epoch, so the adapter now multiplies by 1000. A frontend that treated the old value as milliseconds rendered Jan-1970 timestamps. - console.count/countReset/time/timeLog/timeEnd never reached the inspector console agent: ConsoleObject.cpp forwarded messageWithTypeAndLevel and profile/profileEnd to inspectorController().consoleClient() but not the counter/timer entry points, so a connected DevTools saw no event at all for those calls. They are now forwarded the same way, which surfaces them as Runtime.consoleAPICalled (count as CDP type "debug", timeLog/timeEnd as "timeEnd").
|
Warning Review limit reached
Next review available in: 1 minute 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 (3)
Comment |
|
Updated 6:17 PM PT - Jul 25th, 2026
⏳ @robobun, your commit 2ae31c0 is still building in
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #35396. That PR adds Both PRs edit |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the ConsoleObject.cpp forwards are byte-for-byte the existing messageWithTypeAndLevel/profile pattern (same inspectable() guard, same arguments.copyRef() for timeLog), and the seconds→ms rescale in cdp.ts is applied to both emit sites. The inline nit about countReset coverage is worth folding in but non-blocking.
Extended reasoning...
Overview
Two fixes for the DevTools-protocol inspector server: (1) src/js/internal/inspector/cdp.ts rescales JSC's seconds-since-epoch Console.messageAdded.timestamp to CDP's milliseconds for both Runtime.consoleAPICalled and Runtime.exceptionThrown; (2) src/jsc/bindings/ConsoleObject.cpp forwards count/countReset/time/timeLog/timeEnd to inspectorController().consoleClient() so InspectorConsoleAgent emits events for them. A new subprocess test in test/js/node/inspector/inspector.test.ts opens the inspector, connects as a CDP client, and asserts both the ms-scale timestamp (bracketed by Date.now()) and the emitted event types/texts.
Security risks
None. The change only adds inspector-frontend forwarding behind the existing globalObject->inspectable() guard and does arithmetic on a timestamp. No user-controlled input parsing, no auth/crypto, no new network surface.
Level of scrutiny
Low-to-moderate. The C++ hunks are purely additive and copy the exact pattern already used in the same file for messageWithTypeAndLevel, profile, and profileEnd — same inspectable() gate, same consoleClient() fetch, and timeLog uses arguments.copyRef() before the local arguments.ptr() read just like messageWithTypeAndLevel does. InspectorConsoleAgent::count/time* maintain their own counter/timer state independent of Bun's stdout printer, which is the intended split (same as WebKit); both are invoked on every call so they stay in sync. No new lifetime, refcount, or exception-scope concerns — the inspector client calls don't run user JS. The cdp.ts change replaces message.timestamp ?? Date.now() with a typeof === "number" check plus * 1000, applied consistently at both emit sites; the Date.now() fallback is pre-existing.
Other factors
The test follows the file's established fixture pattern (subprocess, injectedScriptChildEnv, concurrent pipe drain, awaits a __done__ sentinel rather than sleeping, exact-array assertions on types/texts). The one finding is a coverage nit — countReset isn't exercised — which is valid per REVIEW.md's "every sibling entry point" rule but doesn't indicate a defect; the forward is mechanically identical to the four covered siblings. No prior reviews on the PR to reconcile with.
…orwards are load-bearing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…g string reaches DevTools JSC's InspectorConsoleAgent::logTiming puts the "label: N.NNNms" string in Console.messageAdded.text and only the caller's extra arguments in parameters. The adapter preferred parameters when present, so the timing was dropped from the Runtime.consoleAPICalled args for timeLog. Prepend the text as args[0] in that case, matching Node.
There was a problem hiding this comment.
LGTM — both earlier review notes (untested countReset forward; timeLog dropping the elapsed-time string) are addressed in 79e136a and aa8db45.
Checked: the five ConsoleObject.cpp forwards mirror the existing messageWithTypeAndLevel/profile pattern including arguments.copyRef() for timeLog; the * 1000 rescale replaces both prior message.timestamp ?? Date.now() sites so Runtime.exceptionThrown is covered too; the new args prepend spreads into a fresh array rather than mutating message.parameters. The new test brackets the timestamp with Date.now() readings, awaits the __done__ sentinel instead of sleeping, and the countTexts ["cnt: 1", "cnt: 2", "cnt: 1"] assertion makes the countReset hunk load-bearing.
Extended reasoning...
Overview
Three files: src/jsc/bindings/ConsoleObject.cpp adds inspector-client forwarding to count/countReset/time/timeLog/timeEnd (five mechanically identical hunks copying the pattern already used by messageWithTypeAndLevel and profile/profileEnd in the same file); src/js/internal/inspector/cdp.ts rescales JSC's seconds-since-epoch Console.messageAdded.timestamp to CDP's milliseconds and prepends the elapsed-time text to args for type:"timing" messages that carry extra parameters; test/js/node/inspector/inspector.test.ts gains one subprocess test that connects as a CDP client and asserts the timestamp range, event types, count texts, and timing args.
Security risks
None. This is DevTools-protocol translation that is only reachable when globalObject->inspectable() is true (a debugger is attached). No parsing of untrusted external input beyond what the adapter already handled; no auth/crypto/permissions.
Level of scrutiny
Low-to-moderate. The C++ change is purely additive and pattern-matched from adjacent code in the same file — the inspectable() guard means it is a no-op in normal execution, and arguments.copyRef() for timeLog follows the same non-consuming-Ref idiom as messageWithTypeAndLevel. The adapter change is a unit conversion plus a small conditional; the pre-existing Date.now() fallback is preserved (now only taken when timestamp is not a number). The PR description includes before/after evidence on both ASAN-debug and release builds showing the test fails on main for the right reasons (seconds-scale timestamp; missing event types).
Other factors
Two prior review rounds from me on this PR were both addressed: the test now exercises countReset (third count reads "cnt: 1", so reverting that hunk would surface as "cnt: 3"), and the adapter now prepends the timing string so timeLog events carry the elapsed time as args[0] matching Node. The two comment-cop flags on the adapter comments were reasonably rebutted — they are short protocol-translation notes consistent with the rest of cdp.ts, not workaround justifications. The test follows the file's established fixture/spawn/JSON-summary pattern, uses port: 0, drains stdout/stderr/exited concurrently, and awaits the __done__ sentinel event rather than sleeping. All threads on the PR are marked resolved and no human reviewer has outstanding comments.
There was a problem hiding this comment.
LGTM — both earlier findings addressed (countReset now load-bearing via cnt: 1 after reset; timeLog now prepends the elapsed-time text as args[0]).
Checked the final revision: the || rewrite of the type-map lookup is behavior-equivalent (all CONSOLE_TYPE_MAP values are non-empty strings); arguments.copyRef() in timeLog matches the existing messageWithTypeAndLevel pattern and leaves the original Ref valid for the Bun__ConsoleObject__timeLog call below; the fixture awaits the __done__ sentinel rather than sleeping.
Extended reasoning...
Overview
Two fixes to the DevTools-protocol path: (1) cdp.ts rescales JSC's seconds-since-epoch Console.messageAdded.timestamp to CDP's milliseconds and prepends the text timing string to args for type:"timing" messages that carry parameters; (2) ConsoleObject.cpp forwards count/countReset/time/timeLog/timeEnd to inspectorController().consoleClient(), mirroring the existing messageWithTypeAndLevel and profile/profileEnd blocks in the same file. A new test in inspector.test.ts opens the inspector, connects as a CDP client, and asserts the millisecond-bracketed timestamp plus the exact event types/texts for all five newly-forwarded calls.
Security risks
None. This is inspector/DevTools protocol translation — no auth, no external input parsing beyond JSC's own inspector messages, no new network surface.
Level of scrutiny
Low-to-medium. The C++ change is five identical 4-line hunks that copy the neighbouring messageWithTypeAndLevel pattern verbatim (inspectable() guard → consoleClient() null-check → forward). The TS change is a * 1000 and a conditional array-prepend inside a translation function that this file already owns. No lifetime, allocation, or threading changes.
Other factors
- I raised two issues on earlier revisions (missing
countResetcoverage;timeLogargs dropping the elapsed-time string). Both were fixed in 79e136a and aa8db45 and the threads are resolved. The comment-cop bot flags on the two protocol-translation comments were reasonably rebutted (the file's existing style annotates every non-obvious JSC↔CDP mapping) and resolved. - The test is load-bearing for every ConsoleObject.cpp hunk: reverting
countResetyieldscnt: 3instead ofcnt: 1; revertingtimebreaks bothtimeEndassertions; reverting the timestamp rescale fails thebeforeMs/afterMsbracket by ~3 orders of magnitude. The PR description shows both ASAN-debug and release runs passing with the fix and failing without. - Checked that the
(jscType && CONSOLE_TYPE_MAP[jscType]) || …rewrite is equivalent to the old ternary — all map values are truthy strings so the||fallthrough behaves identically. Checked thatarguments.copyRef()doesn't consume the rvalue ref, soarguments.ptr()on the next line remains valid (same asmessageWithTypeAndLevelat the top of the file).
…nied' on the freshly stripped binary)
There was a problem hiding this comment.
LGTM — both earlier review notes (countReset coverage, timeLog dropping the timing string) are addressed and all threads are resolved.
What was reviewed:
- ConsoleObject.cpp forwards for count/countReset/time/timeLog/timeEnd copy the exact
inspectable() → consoleClient()pattern already used bymessageWithTypeAndLevelandprofile/profileEndin the same file;timeLogusesarguments.copyRef()so Bun's own printer below still sees the args. - cdp.ts: seconds→ms rescale, timing-text prepend, and the
CONSOLE_TYPE_MAPternary→||rewrite (all map values are non-empty strings, so behavior is identical). - New test covers all five forwards, brackets the timestamp with
Date.now(), and awaits the__done__sentinel rather than sleeping.
Extended reasoning...
Overview
Two fixes to the DevTools-protocol path: (1) src/jsc/bindings/ConsoleObject.cpp forwards count/countReset/time/timeLog/timeEnd to inspectorController().consoleClient(), mirroring the existing messageWithTypeAndLevel/profile pattern in the same file; (2) src/js/internal/inspector/cdp.ts rescales JSC's seconds-since-epoch Console.messageAdded.timestamp to CDP's milliseconds and prepends the elapsed-time text to args for type:"timing" messages so console.timeLog events carry the duration. A new test in test/js/node/inspector/inspector.test.ts exercises the full round-trip over the WebSocket.
Security risks
None. The inspector forwarding is gated on globalObject->inspectable() (unchanged gate), and the adapter change is a pure unit conversion / array prepend on data already flowing through it. No new surface, no auth/crypto/permissions.
Level of scrutiny
Low-to-moderate. The C++ change is five copies of a 4-line block that already exists twice in the same file (messageWithTypeAndLevel, profile/profileEnd) — I checked that timeLog's arguments.copyRef() matches messageWithTypeAndLevel's handling so the subsequent arguments.ptr() read remains valid. The cdp.ts change is a small refactor plus two protocol-translation fixes; I verified the CONSOLE_TYPE_MAP[jscType] ? … : … → … || … rewrite is behavior-preserving because every map value is a non-empty string, and that the pre-existing Date.now() fallback is unchanged (JSC always sends a numeric timestamp, and the adapter runs on the debugger thread where user-land Date tampering doesn't reach).
Other factors
Both of my earlier findings on this PR were folded in: 79e136a added countReset coverage (third cnt: 1 proves the reset reached the agent), and aa8db45 added the timing-text prepend so timeLog emits ["tm: …ms", "mid"] like Node. The comment-cop bot's two flags were answered (protocol-translation notes matching the file's existing style) and resolved. The gate evidence shows the new test fails on main (timestamp ~1.78e9 and missing timing string) and passes on the PR under both ASAN-debug and release. Test follows harness conventions: tempDir, injectedScriptChildEnv, concurrent pipe drain, and awaits an observable __done__ event rather than a sleep.
…orDebugger subprocess tests the same 30s budget as their neighbours
There was a problem hiding this comment.
LGTM — both prior review notes (countReset coverage, timeLog args prepend) are addressed and all threads resolved.
What was reviewed:
- ConsoleObject.cpp: the five new
inspectable() → consoleClient()->X()forwards mirror the existingmessageWithTypeAndLevel/profilepattern;timeLogusesarguments.copyRef()so the Ref survives for Bun's own printer below. - cdp.ts: the
(jscType && CONSOLE_TYPE_MAP[jscType]) || …refactor is behaviorally identical to the old ternary (all map values are non-empty strings); the timing-args prepend builds a fresh array rather than mutatingparameters. - Test: awaits the
__done__sentinel (no sleep), drains all pipes concurrently, and every ConsoleObject.cpp hunk includingcountResetis now load-bearing via the["cnt: 1", "cnt: 2", "cnt: 1"]assertion.
Extended reasoning...
Overview
Three files: ConsoleObject.cpp gains five identical if (globalObject->inspectable()) { client->X(...) } blocks in count/countReset/time/timeLog/timeEnd, copied verbatim from the pre-existing pattern in messageWithTypeAndLevel and profile/profileEnd in the same file. cdp.ts rescales JSC's seconds-since-epoch Console.messageAdded.timestamp to CDP's milliseconds and prepends the elapsed-time text to args for type:"timing" messages so console.timeLog events carry their duration. The test file gains one new subprocess test and two 30s timeout alignments on neighbouring waitForDebugger tests.
Security risks
None. This is inspector/DevTools protocol translation for a debugging feature the user explicitly opts into via inspector.open() / --inspect. No auth, crypto, permissions, or untrusted-input parsing is touched. The Date.now() fallback and array operations in cdp.ts match the file's existing (non-primordial) style — the adapter runs on the debugger thread against JSON from JSC's own backend, and none of those calls are new to this PR.
Level of scrutiny
Low-to-moderate. The C++ change is purely additive and mechanically identical across five sites to an established pattern two functions away; the only ownership subtlety (Ref<ScriptArguments>&& in timeLog) is handled with copyRef() exactly as messageWithTypeAndLevel does. The TypeScript change is a unit conversion plus a small conditional prepend, both with explanatory comments consistent with the file's translation-note style. The gate evidence shows the new test failing on main (seconds-scale timestamp, missing events) and passing on the PR under both ASAN-debug and release.
Other factors
Both of my earlier 🟡 findings were folded in: 79e136a added countReset coverage so all five forwards are load-bearing, and aa8db45 added the timing-text prepend so timeLog events carry the elapsed time as args[0] matching Node. The comment-cop bot flags were reasonably rebutted (these are protocol-translation notes in a file full of them, not workaround justifications). The type computation refactor from ternary to || is equivalent because every CONSOLE_TYPE_MAP value is a non-empty string. The new test follows harness conventions (tempDir, bunExe, injectedScriptChildEnv, concurrent pipe drain, awaits the __done__ sentinel rather than sleeping, brackets the timestamp with Date.now() readings). A duplicate-PR bot flagged #35396, but the author correctly explained they touch different code paths (global console vs inspector.console).
|
Build 81363 is red on four The one lane that completed (windows-aarch64) built and all eight test shards passed. The gate verified fail-before/pass-after on both ASAN-debug and release. Ready for a maintainer when the build fleet recovers. |
2 participants
Two fixes for
Runtime.consoleAPICalledon the DevTools-protocol WebSocket server (inspector.open()/--inspect):Repro
Cause
ConsoleMessage::addToFrontendwritesm_timestamp.secondsSinceEpoch().value()intoConsole.messageAdded. The CDP adapter (src/js/internal/inspector/cdp.ts) forwarded it verbatim intoRuntime.consoleAPICalled.timestamp/Runtime.exceptionThrown.timestamp, but CDP'sRuntime.Timestampis defined as milliseconds since epoch. A frontend treating the value as ms renders Jan 1970.count/time*missing:ConsoleObject::messageWithTypeAndLevelandprofile/profileEndforward toglobalObject->inspectorController().consoleClient(), butcount,countReset,time,timeLogandtimeEndcalled only into Bun's own printer.InspectorConsoleAgentnever saw those calls, so noConsole.messageAddedwas emitted and the adapter had nothing to translate.Fix
cdp.ts: rescalemessage.timestampfrom seconds to milliseconds when buildingRuntime.consoleAPICalled/Runtime.exceptionThrown.ConsoleObject.cpp: forwardcount/countReset/time/timeLog/timeEndto the inspector's console client, matching the existingmessageWithTypeAndLevelpattern. JSC reports count as{type:"log", level:"debug"}(CDP typedebug) andtimeLog/timeEndas{type:"timing"}(CDP typetimeEnd); Node reportscount/timeEnd. DevTools renders all of these.Verification
test/js/node/inspector/inspector.test.tsgains a test that opens the inspector, connects as a CDP client, issuesconsole.log/count/count/time/timeLog/timeEnd, and asserts both the millisecond-scale timestamp (bracketed byDate.now()) and the emitted event types / texts. Without the fix the timestamp assertion receives ~1.78e9 and the type list is["log", "log"].[review] gate passed · iteration 3 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 2 rejected · iteration 3
evidence per changed file