WebKit: use GetSystemTimePreciseAsFileTime for WallTime and QPC for MonotonicTime on Windows - #34428
Open
robobun wants to merge 8 commits into
Open
WebKit: use GetSystemTimePreciseAsFileTime for WallTime and QPC for MonotonicTime on Windows#34428robobun wants to merge 8 commits into
robobun wants to merge 8 commits into
Claude / Claude Code Review
completed
Jul 17, 2026 in 16m 20s
Code review found 1 important issue
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | test/js/bun/util/randomUUIDv7.test.ts:232-241 |
Bracketing test upper bound can flake via UUIDv7 counter rollover |
| 🟡 Nit | test/js/bun/util/randomUUIDv7.test.ts:250 |
New tests assert stderr is exactly empty |
| 🟡 Nit | src/runtime/webcore/Crypto.rs:250-255 |
New comment exceeds CLAUDE.md 3-line limit |
Annotations
Check failure on line 241 in test/js/bun/util/randomUUIDv7.test.ts
claude / Claude Code Review
Bracketing test upper bound can flake via UUIDv7 counter rollover
The upper-bound checks `b <= after` / `c <= after` assert an invariant `UUID7::init` does not guarantee: on 12-bit counter rollover (src/jsc/uuid.rs:132-137) it bumps the stored timestamp by +1ms, so if this loop sustains >~1024 iterations per real ms (2 UUID7 calls each) the embedded timestamp will legitimately exceed `after`. The lower bound is what oven-sh/WebKit#304 fixes — either give the UUID upper bounds a small slack (e.g. `b <= after + 2`) or keep the strict upper bound only for `File.l
Check warning on line 250 in test/js/bun/util/randomUUIDv7.test.ts
claude / Claude Code Review
New tests assert stderr is exactly empty
nit: REVIEW.md's subprocess-test guidance says to never assert stderr is exactly empty (ASAN/debug builds emit benign warnings) and to instead assert a combined `{ stdout, stderr, exitCode }` object — this applies here and at line 278. That said, the five pre-existing subprocess tests in this file use the identical pattern, so this matches local convention and the practical flake risk is low given `bunEnv` sets `BUN_DEBUG_QUIET_LOGS=1`; not blocking.
Check warning on line 255 in src/runtime/webcore/Crypto.rs
claude / Claude Code Review
New comment exceeds CLAUDE.md 3-line limit
This comment is 5 lines; CLAUDE.md rule 13 caps code comments at 3 lines max. The `oven-sh/WebKit#304` reference is also bug history, which per REVIEW.md belongs in the PR description rather than the code. It could be condensed to e.g. `// js_date_now() is exactly Date.now() (same precise clock + any setSystemTime override), so a caller bracketing with Date.now() always sees before <= ts <= after.`
Loading