WebKit: use GetSystemTimePreciseAsFileTime for WallTime and QPC for MonotonicTime on Windows - #34428
WebKit: use GetSystemTimePreciseAsFileTime for WallTime and QPC for MonotonicTime on Windows#34428robobun wants to merge 8 commits into
Conversation
…onotonicTime on Windows Bumps WEBKIT_VERSION to the oven-sh/WebKit#304 preview build so bun's Windows CI exercises the new clock sources. With that change, JSC's Date.now() and bun_core::time::milli_timestamp() read the same kernel clock on every platform, so the two remaining JS-visible defaults that compared against Date.now() can read it directly: - Bun.randomUUIDv7() default timestamp now reads global.js_date_now() instead of milli_timestamp(), which is the same clock source crypto.randomUUIDv7() already uses. Both paths are now the same js_date_now() + UUID7::init; only the option validation and disableEntropyCache differ. - new File([], name).lastModified default now reads js_date_now(), which is what the File API spec says the default is ("the equivalent of Date.now()"). Both defaults also now respect setSystemTime(), matching Date.now(). Tests: a Date.now() bracketing check over Bun.randomUUIDv7 / crypto.randomUUIDv7 / File.lastModified (50k iters on Windows where the ~1ms skew used to bite ~80% of samples), and a setSystemTime() pinning check for all three.
WalkthroughChangesUUIDv7 time alignment
WebKit dependency pin
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:02 AM PT - Jul 17th, 2026
❌ @robobun, your commit 48490a4 has 100 failures in
🧪 To try this PR locally: bunx bun-pr 34428That installs a local version of the PR into your bun-34428 --bun |
…calls the same core as Bun.randomUUIDv7
Both now route entropy+UUID7::init+hex-print through
webcore::crypto::{uuid_v7_at, uuid_v7_to_hex_js}. Only the argument
validation (Bun's encoding/timestamp vs Node's options object) stays
separate.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…Crypto.rs comment UUID7::init may bump the embedded timestamp by +1ms on 12-bit counter rollover, so the test now only asserts the lower bound for the UUID paths (which is what the WebKit clock change actually fixes) and keeps full bracketing for File.lastModified, which has no counter. The Crypto.rs comment is down to 3 lines and no longer claims the embedded ts is bracketed or references the PR number.
…s lastModified-default to a bracket The blob.test.ts comment described the pre-change native-clock skew that this PR removes; drop it and assert the Date.now() bracket the default now satisfies.
There was a problem hiding this comment.
All prior feedback is addressed in 0708029 and 11736ac — the Rust/test changes look correct and I have nothing further on the code side. Not auto-approving because WEBKIT_VERSION still points at the autobuild-preview-pr-304-* tag (author-noted precondition) and the WebKit-side clock-source swap for WallTime/MonotonicTime affects every Date.now()/monotonic read on Windows, which warrants a human look at the Windows CI run before landing.
What was reviewed:
uuid_v7_at/uuid_v7_to_hex_jsextraction — behavior-preserving vs. the two prior call sites;bun_boringssl_sys::rand_bytesmatches theboringssl::rand_bytesalias used on the node side.setSystemTimetest: fresh subprocess soUUID_V7_LAST_TIMESTAMPstarts at 0 and the pinned 1.7e12 is used exactly (second call same-ms, counter+1, no rollover).blob.test.tsbracket tightening and the 3-line test comment now match the invariant the code guarantees.
Extended reasoning...
Overview
This PR bumps WEBKIT_VERSION to a preview build of oven-sh/WebKit#304 (Windows WallTime → GetSystemTimePreciseAsFileTime, MonotonicTime → QPC), and aligns three JS-visible defaults to read global.js_date_now() so they agree exactly with Date.now() and respect setSystemTime(): Bun.randomUUIDv7()'s default timestamp, crypto.randomUUIDv7() (now sharing a uuid_v7_at/uuid_v7_to_hex_js core with the Bun API), and new File(...).lastModified's default. Two subprocess tests cover the bracketing and setSystemTime behavior; blob.test.ts's stale-comment/wide-tolerance default test was tightened to a bracket.
Security risks
None. The only entropy-path change is that Bun.randomUUIDv7's cached-entropy read moved into a shared helper with an added disable_entropy_cache parameter that is hard-coded false at the Bun call site — identical bytes to before. crypto.randomUUIDv7's disableEntropyCache semantics are preserved (BoringSSL rand_bytes when true, VM cache otherwise).
Level of scrutiny
The Rust/test edits are small, mechanical, and well-covered by tests. The WebKit dependency bump is the high-scrutiny piece: it swaps the clock source underlying every Date.now(), new Date(), Intl formatting, and JSC monotonic-time read on Windows. That's a runtime-wide behavior change whose real validation is the Windows CI matrix, not local tests. It also currently pins a preview tag that the PR description says must be replaced with the merged sha before landing — a hard precondition a human needs to coordinate.
Other factors
Two prior review rounds from me were fully addressed (UUID upper-bound flake risk removed, comments condensed to ≤3 lines, stale blob.test.ts comment/tolerance updated). The stderr === "" pattern in the new subprocess tests matches the five existing tests in the file and was intentionally left as-is. No outstanding threads. Given the cross-repo dependency and the Windows-global timing impact, this should get a human sign-off after the Windows CI results are in and the WebKit PR has merged.
|
CI on 11736ac (build 74265): 285/286 lanes passed, all 34 Windows lanes green (x64, x64-baseline, aarch64; builds + every test shard). The one failure is Ready for review; |
…ce.now() on Windows Pure-JS observable of the WebKit-side clock change: performance.timeOrigin is set from the precise kernel clock at startup and performance.now() is QPC elapsed, so their sum tracks the precise clock. With Date.now() now reading the same clock, sampling Date.now() then the sum gives floor(t1) - t2 <= 0. On the previous WTF QPC-interpolated path, Date.now() ran ~0.4ms ahead and ~72% of 200k samples had Date.now() > the sum.
…cess; trim uuid_v7_at doc to 3 lines In a subprocess, performance.timeOrigin is captured milliseconds before the loop, so w32tm slew between VM init and the test cannot drift the live wall clock ahead of timeOrigin + QPC elapsed.
There was a problem hiding this comment.
52820f7 addresses both open threads from the last pass — the Date.now()/performance.now() clock test now runs in a fresh subprocess (so timeOrigin is captured immediately before the loop), and the uuid_v7_at doc comment is down to 3 lines. Nothing else new to flag beyond the inline WEBKIT_VERSION note.
Extended reasoning...
Commit 52820f7 landed after the previous review round and addresses both unresolved 05:47 threads: the Windows-only Date.now() <= timeOrigin + performance.now() test is now spawned in a subprocess with a comment explaining the w32tm-slew rationale, and the uuid_v7_at doc comment was trimmed from 4 to 3 lines. Verified both against the current diff. The only finding this run is the ephemeral autobuild-preview-pr-304-* tag in WEBKIT_VERSION, which the PR description already calls out as a pre-merge TODO; the inline comment tracks it as a review gate so it isn't merged by accident. Not approving because that swap plus the WebKit bump itself (Windows clock-source change affecting Date.now()/MonotonicTime) warrant a human sign-off.
…ce clock test origin + performance.now() at ~1.8e12 has ~0.0004ms double ULP, so the sum can round below the true value and make d > p by a fraction of a microsecond. CI hit d - p = 0.0002 on one sample. The old-path skew was ~0.4ms, so a 0.01ms threshold still separates the two cleanly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/bun/util/randomUUIDv7.test.ts (1)
255-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse module-scope imports in the child scripts.
These subprocess scripts use
require()even though they are not testing dynamic/CommonJS loading. Move the imports to the top of each embedded script, or use a fixture file if-ecannot parse static imports.As per coding guidelines, only use dynamic import or
requirewhen the test specifically covers it; otherwise use module-scope imports.Proposed fix
- const crypto = require("node:crypto"); + import * as crypto from "node:crypto"; - const { setSystemTime } = require("bun:test"); - const crypto = require("node:crypto"); + import { setSystemTime } from "bun:test"; + import * as crypto from "node:crypto";Also applies to: 288-290
🤖 Prompt for 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. In `@test/js/bun/util/randomUUIDv7.test.ts` around lines 255 - 263, Update the embedded child scripts around the crypto usage to use module-scope imports instead of require(), including the additional occurrence near the later referenced lines. Preserve the scripts’ existing behavior and use a fixture file if the -e execution mode cannot parse static imports.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/bun/util/randomUUIDv7.test.ts`:
- Around line 255-263: Update the embedded child scripts around the crypto usage
to use module-scope imports instead of require(), including the additional
occurrence near the later referenced lines. Preserve the scripts’ existing
behavior and use a fixture file if the -e execution mode cannot parse static
imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f9b0c42-92fd-47d1-8021-de46975f3820
📒 Files selected for processing (3)
src/runtime/webcore/Crypto.rstest/js/bun/util/randomUUIDv7.test.tstest/js/web/fetch/blob.test.ts
autobuild-722f2a8a1a3da159a89b35730c5460a6ef58f0af is now published (43 assets). Also pulls in the three WebKit main commits that landed between the preview base and the merge: 365cb024 (Dockerfile.windows build flags), ae5110d3 (defer termination at reifyStaticProperty call sites), 8be99556 (AsyncContextSwapScope).
There was a problem hiding this comment.
No new findings — all prior feedback (comment length, stale blob.test.ts assertion, in-process Date.now/perf.now test, preview WEBKIT_VERSION tag) is addressed, and WEBKIT_VERSION now points at the merged 40-hex main sha. Deferring to a human for the WebKit bump itself: it swaps the Windows WallTime/MonotonicTime clock sources engine-wide, which is beyond what I'll auto-approve.
What was reviewed:
uuid_v7_at/uuid_v7_to_hex_jsextraction is behavior-preserving vs. both prior call sites (entropy source,disableEntropyCache, hex rendering).js_date_now().max(0.0) as u64and theFile.lastModifieddefault swap — negative/NaN handling andsetSystemTime()interaction check out.- The three new subprocess tests: pipe draining, bracketing/rollover asymmetry, and the 0.01ms slack on the Windows-only clock test all look sound.
prebuiltUrl()/prebuiltDestDir()handle the new 40-hex sha via the existing non-autobuild-path.
Extended reasoning...
Overview
Six files: a WEBKIT_VERSION bump in scripts/build/deps/webkit.ts picking up oven-sh/WebKit#304 (Windows WTF::WallTime::now() → GetSystemTimePreciseAsFileTime, MonotonicTime::now() → raw QPC); three small Rust changes in Crypto.rs / node_crypto_binding.rs / Blob.rs swapping bun_core::time::milli_timestamp() → global.js_date_now() for Bun.randomUUIDv7()'s default timestamp and new File().lastModified's default, plus extracting the shared UUIDv7 core into uuid_v7_at() / uuid_v7_to_hex_js(); three new subprocess tests in randomUUIDv7.test.ts; and a tightened bracket assertion in blob.test.ts.
Security risks
None identified. The entropy paths are unchanged (still VM entropy cache or BoringSSL rand_bytes); the refactor moves existing code into a shared helper without changing which RNG is called or how many bytes are drawn. js_date_now() is an existing internal accessor already used by crypto.randomUUIDv7().
Level of scrutiny
The Bun-side Rust and test changes are small, well-tested, and I'd approve them on their own. The WebKit bump is the piece that warrants human sign-off: it changes the clock source for Date.now(), new Date(), Intl, and every WTF MonotonicTime/ContinuousTime consumer on Windows — engine-wide behavior whose actual implementation lives in a separate repo. That falls under REVIEW.md's Dependencies & vendoring situational section, and the last full-CI report in the thread (285/286 on build 74265) was for 11736aca; three test-touching commits and the sha swap have landed since, with build #74528 kicked off on the current HEAD but no result posted yet.
Other factors
Every inline thread from my earlier passes is resolved and reflected in the current diff: comments are ≤3 lines, the Date.now/perf.now test runs in a subprocess with 0.01ms double-precision slack, the stale blob.test.ts comment is gone with the assertion tightened to a bracket, and WEBKIT_VERSION is a permanent 40-hex main sha. The uuid_v7_at extraction removes the last duplication between the Bun and node:crypto UUIDv7 paths (the UUID7 import in node_crypto_binding.rs was correctly dropped). Nothing new to flag — deferring solely on the scope of the vendored-engine bump and pending CI on HEAD.
|
CI on 48490a4 (build 74528): the debian-13 x64-asan lanes fail ~20 tests with This is not from oven-sh/WebKit#304 (those changes are all under The preview build this PR was originally validated against ( The Windows lanes (the actual subject of this PR) and this PR's own tests are green. |
|
The x64-asan validateExceptionChecks failures in build 74528 ( Fix is oven-sh/WebKit#306: keep the |
1 participant
What does this PR do?
Bumps
WEBKIT_VERSIONto the preview build of oven-sh/WebKit#304 so Bun's Windows CI exercises the new Windows clock sources:WTF::WallTime::now()(which backsDate.now(),new Date(), Intl) now readsGetSystemTimePreciseAsFileTimeinstead of theGetSystemTimeAsFileTime+QueryPerformanceCounterinterpolation scheme that kept unsynchronized static state and could disagree with the kernel's precise clock by ~1ms.MonotonicTime::now()/ContinuousTime::now()/ContinuousApproximateTime::now()now readQueryPerformanceCounterdirectly instead of wall-clock-clamping via an unsynchronizedstatic double lastTime.With that change,
Date.now()andbun_core::time::milli_timestamp()read the same kernel clock on every platform (POSIX already did; both callclock_gettime(CLOCK_REALTIME)). That lets the two remaining JS-visible defaults that a user would compare againstDate.now()read it directly:Bun.randomUUIDv7()'s default timestamp now readsglobal.js_date_now(), the same clock sourcecrypto.randomUUIDv7()already uses. Both are nowjs_date_now()+UUID7::init; only the option validation anddisableEntropyCachediffer.new File([], name).lastModified's default now readsjs_date_now(), which is what the File API spec says the default is ("the equivalent ofDate.now()").Both defaults now also respect
setSystemTime(), matchingDate.now().Originally motivated by the Windows
test-crypto-randomuuidv7.jsflake in #32623 (assert(uuidTimestamp >= Date.now())sampled just before the native call), which this removes at the source.How did you verify your code works?
bun bd test test/js/bun/util/randomUUIDv7.test.ts(20/20 pass). Two new tests:default timestamp is bracketed by Date.now(): loops 50k on Windows / 5k elsewhere assertingbefore <= {Bun.randomUUIDv7, crypto.randomUUIDv7, File.lastModified} <= after.default timestamp respects setSystemTime(): pins the clock and checks all three read the pinned value.setSystemTime()test fails on main without thesrc/changes (Bun.randomUUIDv7()andFile.lastModifiedreturn the real clock), passes with them.Note:
WEBKIT_VERSIONpoints at aautobuild-preview-pr-*tag, which only exists while oven-sh/WebKit#304 is open. Before merging this, that PR needs to merge first andWEBKIT_VERSIONshould be set to the resulting main sha.no test proof · iteration 7 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/randomUUIDv7.test.ts