Repository navigation
Conversation
The BigIntStats constructor and the date getters of Stats and BigIntStats picked a conversion from the class of the object: - `new BigIntStats(...)` divided `BigInt.asIntN(64, ns)` as a double and stored the four `*Ms` fields as Numbers, which its own date getters reject. - The date getters used ToNumber for Stats and a ToBigInt cut to 64 bits for BigIntStats. A BigInt `*Ms` at or past 2n ** 63n wrapped to a valid date, and a value of the other numeric type threw. Node.js divides with the JS `/` operator in the constructor and converts with `Number(ms)` in one getter that both classes share. Do the same: - `divideNsIntoMs` applies `jsDiv` with `1000000n` for both constructor paths. - One getter per date field serves both prototypes. It converts `*Ms` with the existing `IDLLooseDoubleOrBigInt` converter, behind an inline path for a Number and for a BigInt that a double holds exactly. The getters still do not apply Node's `Math.round`, so the date of a real file does not change. test-fs-watchfile-bigint.js gets its upstream assertion back: it compares the stats of a missing file with `new BigIntStats(0n, ...)`. This replaces #42563 and #42564.
|
Updated 6:41 PM PT - Oct 28th, 2026
❌ @robobun, your commit 2045e05 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 47233That installs a local version of the PR into your bun-47233 --bun |
|
Status: the change and the tests are pushed. This PR replaces #42563 and #42564. Reproduction: the script in the Notes of the PR body, run with a release build of main on Linux x64.
Proof for the tests ( CI (build 134618) passed: 190 of 195 jobs pass, the Windows and macOS lanes included. The other 5 are Open question for a maintainer: should the date getters also apply Node's |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
BigInt timestamp construction src/jsc/bindings/NodeFSStatBinding.cpp, test/js/node/fs/fs-stats-constructor.test.ts, test/js/node/test/parallel/test-fs-watchfile-bigint.js |
BigIntStats construction divides nanosecond timestamps by one million using BigInt division. Tests cover constructor forms, conversion behavior, and watchfile callback stats. |
Shared Stats date getters src/jsc/bindings/NodeFSStatBinding.cpp, test/js/node/fs/fs-stats-constructor.test.ts |
Date getters now recognize both Stats structures and use shared conversion logic. Tests cover conversion behavior and shared prototype accessors. |
Suggested reviewers: jarred-sumner, cirospaciari
Priority:
Merge Risk: 
2045e
-
No blocking issue is established. Merge after normal build and test checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title "node:fs: convert Stats times by the value, not by the class" matches the main change. The objectives say both constructor paths now use value-based BigInt division, and that date getters ar… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description provides a detailed explanation of the problem, fix, scope, trade-offs, and verification results. It does not use the template headings exactly, but it includes the required informatio… Full details: Title check
-
Explanation
The title "node:fs: convert Stats times by the value, not by the class" matches the main change. The objectives say both constructor paths now use value-based BigInt division, and that date getters are shared and convert *Ms by value (IDLLooseDoubleOrBigInt) instead of selecting the conversion by class. The title is specific, concise, and follows the node:fs prefix convention. It is 59 characters, which is within a normal length.
Full details: Description check
-
Explanation
The description provides a detailed explanation of the problem, fix, scope, trade-offs, and verification results. It does not use the template headings exactly, but it includes the required information for what the PR does and how the changes were verified.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the shared getter's getDirect fast path: both class structures are built with atimeMs..birthtimeMs at offsets 10-13 (src/jsc/bindings/NodeFSStatBinding.cpp:562-569 and :591-598), so hasStatsStructure accepting either structure is sound. The isFile()/mode methods still convert by class rather than by value — pre-existing and outside this PR's scope. getStructure<> is still used by the native and constructor paths, so no helper went dead.
Extended reasoning...
The change reworks the native node:fs Stats/BigIntStats binding so that BigIntStats constructor *Ms fields are BigInt-divided via jsDiv and the four date getters are shared across both prototypes, converting by value type; it touches no security-sensitive surface. The one posted finding is a pre-existing truncate-vs-round divergence from Node that the author already documents as a maintainer decision, and the behavioral change to a node compat surface in C++ warrants a human look, so this is a ruled-out note rather than an approval.
Problem
new BigIntStats(...)stores its*Msfields as Numbers (2.5, Node.js2n). Its date getters throwTypeError: Invalid argument type in ToBigInt operation.BigIntStatsdate getters cut*Msto 64 bits: a file dated18446744073709552s readsmtime.getTime() === 384, notNaN. The other numeric type throws.src/jsc/bindings/NodeFSStatBinding.cpp:237,:801,:884). No user reported it.Fix
divideNsIntoMs: JS/with1000000n, as in Node.js.*MslikeNumber(ms)(IDLLooseDoubleOrBigInt). No other file's date changes (0 of 17,820).test/js/node/fs/fs-stats-constructor.test.ts(16 new tests fail on main) and the restored upstreamtest-fs-watchfile-bigint.js.Background
BigIntStatsis the unexported class offs.stat(path, { bigint: true })results.Datefrom*Ms.Downsides
*Msfraction is 0.5 or more. The round is one ready commit (costs in Notes) that moves dates of existing files. A maintainer must decide.Statsruns 3 to 8 more instructions.new BigIntStats(...)retains 4 BigInt cells and runs about 1,045 more.Notes
Repro (Linux, line 3 needs a tmpfs):
typeof atimeMs, valuenumber 2.5bigint 2bigint 2s.atime1970-01-01T00:00:00.002Z1970-01-01T00:00:00.002Z{ bigint: true }384NaNNaNctimeMsonStats55The 62 values that the new tests assert differ from Node.js v26.3.0 in 37 values on main and in 0 with this PR. None of them is a fraction. A fractional
*Msstill differs from Node.js (the round, below).Who can reach the changed behaviour: a program that constructs
BigIntStatsby hand, a file dated past 2^63 ms on a filesystem with 64-bit times, or a program that stores the other numeric type in*Ms. Nothing in this repository calls the constructor.Measurements (Linux x64, release builds of main 8d5672c and of this change on it, before the rebase over the WebKit upgrade)
User-space instructions per iteration, JIT off, N = 200,000, medians of interleaved runs. Runs of one binary differ by about 10.
statSync(f), no date readstatSync(f, { bigint: true }), no date readstatSync(f).mtimestatSync(f), thenmtimeandctimestatSync(f), then all four datesstatSync(f, { bigint: true }).mtimestatSync(f, { bigint: true }), then all four datesnew BigIntStats(...)The later date reads cost more on
Statsbecause the shared getter compares two structures before it reads*Msby name.new BigIntStats(...)wall time, JIT on, 2,000,000 calls, 5 interleaved runs: 504 to 664 ns per call on main, 651 to 960 ns with this PR.Release text: 138,010,898 to 138,007,314 bytes (3,584 fewer). Date getters: 9,704 bytes in 8 functions to 4,768 bytes in 4. The two
BigIntStatsconstructor paths: 7,510 to 8,358 bytes withdivideNsIntoMs.Bun__createJSStatsObject,Bun__createJSBigIntStatsObject, theStatsconstructor paths, the date setter and the 14 mode methods: 0 instructions differ (19 functions, addresses normalized).new BigIntStats(...): 4 BigInt cells retained per call, plus the divisor. Main: 0.A Number stat and a date read create 0
BigIntStatsclass objects, as on main.Real files, main against this PR: 0 of 17,820 differ in any date of either class (
/usr/include,/usr/lib/x86_64-linux-gnu,src/,test/js/node,node_modules, up to 5,000 files each).Exception-check mode (
BUN_JSC_validateExceptionChecks=1): 0 unchecked exceptions.The round, left out
Node.js:
new Date(MathRound(Number(ms)))(dateFromMs). Bun truncates. The change is one line plus tests, commit eec912d on branchrobobun/2cfe25fb/fs-stats-date-round:A date moves by +1 ms when the
*Msfraction is 0.5 or more. How many files that is depends on how they were written. On this host (Linux 6.1, ext4):mtimedate would move/usr/include,/usr/lib/x86_64-linux-gnu(whole-second times)node_modulessrc/of this checkouttest/js/nodeof this checkoutLinux before 6.13 stamps new files from a coarse clock whose sub-millisecond phase is constant for hours, so one write burst moves together.
Measured with a build of that commit, on 2,400 files generated with a uniformly random sub-millisecond
mtime:mtime.getTime()differs fromMath.trunc(mtimeMs)Bun.file(f).lastModified === fs.statSync(f).mtime.getTime()statSync(f).mtimediffers fromstatSync(f, { bigint: true }).mtimeutimesSync(dest, s.atime, s.mtime)is newer than its sourcefs.cpSync(src, dest, { preserveTimestamps: true })is newer than its sourceetag1.8.1 tag (express.static,send) differs from the tag Node.js issuedutimesSync(f, ms / 1000)thenmtime.getTime() !== ms, 1,000 values in 2004-10cp. With it,cpSyncwithpreserveTimestampsdoes not depend on the dates.lastModifiedmatchesfs.statSync().mtime.getTime(). With the round that holds only for whole-millisecond times.Not changed, still differs from Node.js
Bun.file().lastModifiedstays truncated. It is the File API field, and Chromium (TimeDelta::InMilliseconds()) and WebKit (millisecondsAs<int64_t>()) truncate it for a file on disk. It still reads384for the far-future file above. Bun.file: keep the sign of lastModified for files dated before 1970 #41748 rewrites that conversion.is*()mode methods (NodeFSStatBinding.cpp:142) keep one function per class and readmodeby the class. Node.js also picks there by the class of the receiver.Stats(...)andBigIntStats(...)withoutnewreturn an object. Node.js throws.getStructure<isBigInt>ingetDateFieldand has to move tohasStatsStructureafter this PR.*Instantaccessors and the 18-argumentStatsof Node.js v26.2 (docs/runtime/nodejs-compat.mdx:44).dev, ino, mode, ..., Node.js:dev, mode, nlink, ...).Windows and macOS: not run locally. CI build 134618 passes on the Windows and macOS lanes. The changed code has no platform branch.
Suites run with the debug build, after the rebase:
fs-stats-constructor.test.ts(also in exception-check mode), and fromtest/js/node/test/parallel/:test-fs-watchfile-bigint.js,test-fs-watchfile.js,test-fs-stat.js,test-fs-stat-bigint.js,test-fs-stat-date.mjs,test-fs-utimes.js. Before the rebase, with the debug build:fs-stats-truncate.test.ts,fs-birthtime-linux.test.ts,cp.test.ts,vm.test.ts -t newTarget,test-fs-stat-abort-test.js(under the test runner),test-fs-statfs.js,test-fs-utimes-y2K38.js,test-fs-watchfile-ref-unref.js,test-fs-promises.js,test-fs-promises-file-handle-stat.js,test-worker-fs-stat-watcher.js,test-fs-cp-async-preserve-timestamps.mjs. With the release build:fs.test.ts(629 pass),bun-file.test.ts, and the 101 ahead-of-time programs whose path containsstats.fs.test.tson the debug build: 4 recursivereaddirtests exceed their 10 s limit in this container. They pass on the release build.tscontest/: no errors.Same file, other open PRs: #42518, #42558, #46832, #46388. None changes a conversion.
Self-review, the 3 concerns addressed in part: the package effects of the round list only the
etagrows that were measured again here, the instruction counts are JIT-off only, and a checked-in differential test against Node.js is left for a follow-up.