fix(node:perf_hooks): report nodeTiming milestones as offsets from timeOrigin - #36518
fix(node:perf_hooks): report nodeTiming milestones as offsets from timeOrigin#36518robobun wants to merge 4 commits into
Conversation
…meOrigin performance.nodeTiming reported absolute epoch timestamps: the startup milestones were set to performance.timeOrigin, and startTime returned nodeStart. Node measures all of them in milliseconds from timeOrigin, and a "node" entry is the timeOrigin reference, so its startTime is 0. Also define the name/entryType/startTime/duration accessors as own enumerable properties of the nodeTiming object, matching Node, using null-prototype descriptors so prototype pollution cannot inject extra descriptor keys. Fixes #23041 Co-authored-by: 0xfandom <kashyapshivank01@gmail.com>
|
Updated 1:31 AM PT - Jul 31st, 2026
✅ @robobun, your commit d074a22ce9028cd7119c3bb86f17827a6f67df46 passed in 🧪 To try this PR locally: bunx bun-pr 36518That installs a local version of the PR into your bun-36518 --bun |
Walkthrough
ChangesPerformance node timing
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/js/node/perf_hooks/perf_hooks.test.ts`:
- Around line 14-52: Strengthen the nodeTiming coverage in the existing test by
asserting each listed milestone is exactly 0, not merely a number below the
epoch threshold. Replace the Object.keys check with own-property descriptor
checks for every nodeTiming field, requiring enumerable getter accessors. Add a
fresh bunExe() child test that sets Object.prototype.value before requiring
perf_hooks and verifies nodeTiming initializes successfully, covering
null-prototype accessor descriptor creation.
🪄 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: 51d78611-4273-430f-9462-b472ad1c634f
📒 Files selected for processing (2)
src/js/node/perf_hooks.tstest/js/node/perf_hooks/perf_hooks.test.ts
|
This PR may be a duplicate of:
🤖 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 — small, well-scoped Node compat fix with a test that fails on the old behavior.
What was reviewed:
- Verified the milestone values (
0) andstartTime(0) match Node'slib/internal/perf/nodetiming.jssemantics (offsets fromtimeOrigin, not epoch ms). - Checked the own-property descriptor block: null-prototype outer + inner descriptors, consistent with the file's existing
definePropertiescalls;toJSON()still reads through correctly since the own getters shadow the prototype getters with identical values. - Confirmed no other tests in
test/assert on the oldnodeTimingvalues (test-nodetiming-uvmetricsinfo.jsonly touchesuvMetricsInfo;test-trace-events-bootstrap.jsonly lists milestone names). loopStart/idleTimemoving from1→0is a stub-value change with no downstream consumers.
Extended reasoning...
Overview
This PR touches two files: src/js/node/perf_hooks.ts (the node:perf_hooks compat module) and its test file. It fixes performance.nodeTiming to report startup milestones as offsets from timeOrigin (Node semantics) instead of absolute epoch timestamps, and adds own enumerable accessors for name/entryType/startTime/duration so Object.keys(nodeTiming) matches Node.
Security risks
None. This is a JS-only change to stub values in a Node compat module. The new descriptor object uses __proto__: null on both the outer object and each descriptor, following the file's existing prototype-pollution-safe pattern. No user input is parsed; no native code is touched.
Level of scrutiny
Low. nodeTiming in Bun is a stub — the previous values (performance.timeOrigin, 1) were already placeholders, and this PR just makes them the correct placeholders per Node's contract. The change is mechanical: replace epoch values with 0, hardcode startTime to 0, and mirror Node's own-property shape. The linked issue (#23041) and the OpenTelemetry use case make the motivation clear.
Other factors
- The new test asserts
startTime === 0, milestones< 1e12(i.e., not epoch-scale),loopExit === -1, own-property enumerability, and the fulltoJSON()shape — good coverage for a stub fix. - The PR description confirms the test fails without the src change (
Received: 1785472769724.3042), satisfying the "prove the test fails for the right reason" requirement. - I grepped for other test-suite consumers of
nodeTimingvalues; none depend on the old behavior. - The
durationown-getter closes over the globalperformance(not a captured local), but this is consistent with the pre-existing prototype getter on the same class, so it's not a regression introduced here.
…ties name/entryType/startTime are own data properties (writable:false) in Node, not accessor descriptors; only duration is a getter. Tighten the test to assert the exact descriptor shape, assert duration > 0 (a live reading, not just a number), and add coverage for Object.prototype.value pollution at module load. Co-authored-by: 0xfandom <kashyapshivank01@gmail.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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/js/node/perf_hooks/perf_hooks.test.ts`:
- Around line 44-53: Strengthen the live duration accessor assertions around nt
by requiring the property descriptor’s set field to be undefined. Capture the
initial positive json.duration value, await one macrotask, then obtain a later
duration reading and assert it is greater than the first, preserving the
existing toJSON verification.
🪄 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: 4c9c9bc1-804b-4b0f-9b53-c2cbe7dcbfef
📒 Files selected for processing (2)
src/js/node/perf_hooks.tstest/js/node/perf_hooks/perf_hooks.test.ts
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix for nodeTiming stub values.
What was reviewed:
- Milestone values switched from epoch
timeOriginto0offsets,startTimehardcoded to0— matches Node'slib/internal/perf/nodetiming.jscontract. - Own-property descriptor shape (
name/entryType/startTimeas data props,durationas getter) with__proto__: nullon each descriptor — verified the accessor descriptor fordurationwould otherwise collide with a pollutedObject.prototype.value, and the subprocess test covers exactly that. - Checked that the module-level descriptor constant is safe to reuse (singleton
nodeTiming), and that the now-shadowed prototype getters are harmless leftovers kept for the exportedPerformanceNodeTimingclass. loopStart/idleTimemoving1 → 0is still a placeholder but no less correct than before;loopExitstays-1.
Extended reasoning...
Overview
Two files touched: src/js/node/perf_hooks.ts (the built-in node:perf_hooks module) and its test file. The runtime change is confined to createPerformanceNodeTiming() and the prototype startTime getter: milestone fields (nodeStart, v8Start, environment, bootstrapComplete, loopStart, idleTime) are now initialized to 0 instead of performance.timeOrigin / 1, startTime returns 0, and four own enumerable properties (name, entryType, startTime as data; duration as a getter) are defined on the instance via a module-level null-prototype descriptor bag to match Node's own-property shape. No native code, no new API surface.
Security risks
None. This is a pure JS stub-value correction inside a Node compat module. The only security-adjacent aspect is the __proto__: null on each descriptor, which is defensive hardening against prototype pollution — the same pattern already used throughout this file for every other defineProperty/defineProperties call.
Level of scrutiny
Low. nodeTiming in Bun is a stub (Bun does not yet record real startup milestones), so the only observable contract is the shape and scale of the numbers. The previous values (~1.7e12 epoch ms) broke OpenTelemetry's bootstrap-span computation per #23041; 0 is the correct offset-from-timeOrigin placeholder and matches what Node returns for startTime. The own-property descriptor addition is verified against Node v26.3.0's actual descriptors in the PR description, and the prototype getters that are now shadowed remain in place only because PerformanceNodeTiming is (Bun-only) exported as a class.
Other factors
- Two new tests added to the existing test file (correct location per repo conventions): one asserts the offset semantics, descriptor kinds, and
toJSON()output; the other spawns a subprocess that pollutesObject.prototype.valuebefore requiring the module to prove the null-prototype guard on thedurationaccessor descriptor is load-bearing. Both follow harness conventions (bunExe/bunEnv, concurrent pipe drain, exit-code asserted last). - All CodeRabbit and comment-cop threads are resolved; the author trimmed comments per the bot and justified declining the
Bun.sleep(0)suggestion. - The duplicate-PR bot flags #35390 ("real nodeTiming"), but this stub fix is forward-compatible with that work — it just makes the placeholder values correct in the meantime.
- No CODEOWNERS entry for
src/js/node/perf_hooks.ts.
|
Closing as a duplicate of #32481 by @0xfandom, which predates this PR, applies cleanly to current main and carries its own regression test. The only delta here (data-property descriptors for |
1 participant
Adopts #32481 by @0xfandom.
Repro
Cause
createPerformanceNodeTiming()insrc/js/node/perf_hooks.tssetbootstrapComplete/environment/nodeStart/v8Starttoperformance.timeOrigin(an absolute epoch timestamp), andget startTime()returnedthis.nodeStart. Node measures every startup milestone in milliseconds fromtimeOrigin, and a"node"entry is the timeOrigin reference, so itsstartTimeis0.Fix
timeOrigin(0; Bun does not record the individual startup milestones, so they are reported relative to process start).loopExitstays-1.startTimereturns0.name/entryType/startTimeare defined as own enumerable data properties (writable:false) anddurationas an own enumerable getter, matching Node'slib/internal/perf/nodetiming.js, soObject.keys(performance.nodeTiming)matches Node. Null-prototype descriptors so prototype pollution before the module loads cannot inject extra descriptor keys.Why this is correct
Node's
lib/internal/perf/nodetiming.jsdefinesname/entryType/startTimeas own data properties withvalue:andduration/milestones as own getters, withstartTime: 0hardcoded and each milestone computed as(ns - timeOrigin) / 1e6. Verified against Node v26.3.0:Object.getOwnPropertyDescriptor(nt, 'startTime')is{value: 0, writable: false, enumerable: true, configurable: true}. The OpenTelemetry bootstrap-span use case in #23041 depends on these being offsets, not epoch values.Verification
Without the
src/change, the new tests fail:Fixes #23041. Supersedes #32481 (same patch, brought into a repo branch so CI can run, plus the descriptor-kind correction).
[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file