Conversation
to_js_time masked the millisecond count to 52 bits and stored it as u64, so a negative mtime wrapped to about 2^52 (year 144673). JSTimeType is now i64 and to_js_time truncates toward zero without a mask, so Bun.file(path).lastModified equals fs.statSync(path).mtime.getTime().
|
Warning Review limit reached
On-demand reviews are free for the next 13 days. After that, they cost $0.25 per reviewed file. Or wait 56 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 7:12 PM PT - Sep 7th, 2026
❌ @Jarred-Sumner, your commit b7bae1f has 5 failures in
🧪 To try this PR locally: bunx bun-pr 41748That installs a local version of the PR into your bun-41748 --bun |
|
Status: ready for review. Reproduced on Linux with bun 1.4.3: touch -d @-315619200 neg.js
bun -e 'console.log(Bun.file("neg.js").lastModified, require("fs").statSync("neg.js").mtimeMs)'
# before: 4503284008170496 -315619200000
# after: -315619200000 -315619200000The new test in The current head 6d1267c only moves the Windows widening into the shared |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused fix with a clear mechanism and both stat paths now sharing one helper.
What was reviewed:
to_js_timerewrite: i128 total-ns then truncate-toward-zero matchesnew Date(mtimeMs).getTime(); checked positive inputs give identical results to the old formula.INIT_TIMESTAMPsentinel: same numeric value as before, so the== INIT_TIMESTAMPguard andas f64fallback inget_last_modifiedare unaffected.- Windows
sec as i32 as u32 as i64cast matchesStat.rstimespec_parts;stat_mtimealready sign-extends from i32, so the round-trip is lossless. - Test uses
tempDir/isWindowsfrom harness, exercises both the lazy-stat and fstat-during-read paths, and gates the negative-value assertion off Windows correctly.
Extended reasoning...
Overview
This PR fixes Bun.file(path).lastModified for files with mtimes before the Unix epoch. The root cause was JSTimeType being u64 (a port of Zig's u52) with a 52-bit mask in to_js_time, which wrapped negative millisecond values into huge positives. The fix changes JSTimeType to i64, rewrites to_js_time to compute total nanoseconds in i128 and truncate toward zero (matching new Date(mtimeMs).getTime() semantics), and consolidates the three stat→mtime call sites (resolve_file_stat, POSIX ReadFile, Windows ReadFileUV) into one shared stat_to_js_mtime helper. On Windows the helper reinterprets the 32-bit uv_timespec_t.sec as unsigned to match what node:fs's timespec_parts already does, so lastModified agrees with fs.statSync().mtimeMs on every platform.
Security risks
None. This is pure arithmetic on stat timestamps with no parsing of untrusted input, no allocation, no syscall changes, and no new code paths reachable from user input beyond what already existed. The i128 intermediate and explicit clamp to i64 range make overflow impossible.
Level of scrutiny
Low-to-medium. The change is small (~40 lines net), mechanically verifiable, and the PR description traces the exact wrong value (4503284008170496) to the specific mask. I verified the sentinel value is numerically preserved (so the == INIT_TIMESTAMP lazy-stat guard and the as f64 fallback in get_last_modified still work), that the new formula gives identical results to the old one for non-negative inputs, and that the Windows cast chain matches the existing timespec_parts in src/runtime/node/Stat.rs. All call sites of to_js_time, JSTimeType, and INIT_TIMESTAMP were grepped — nothing else depends on the old unsigned semantics.
Other factors
The test is added to the correct existing file (bun-file.test.ts), uses harness helpers (tempDir, isWindows), covers both the lazy-stat and fstat-during-read code paths, and correctly gates the "actually negative" assertion off Windows where Node's own unsigned-seconds behavior means pre-1970 dates wrap. No CODEOWNERS cover the touched paths. The consolidation of three inline to_js_time calls into one helper is exactly what REVIEW.md asks for ("fix the whole class"; "the second time a block appears, extract a named helper"). Bug hunter ran to dry_streak with no findings.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused signedness fix that routes all three lastModified stat sites through the same timespec_parts helper node:fs already uses.
Checked every consumer of JSTimeType/INIT_TIMESTAMP — the sentinel value is unchanged and all readers go through as f64 or equality, so the u64→i64 flip is transparent.
Traced the Windows path: stat_mtime sign-extends the 32-bit uv_timespec_t.sec to i64, then timespec_parts narrows back to i32 before the unsigned reinterpret, so both pre-1970 and post-2038 dates match fs.Stats.mtimeMs.
Confirmed to_js_time's i128 total-ns division truncates toward zero (matching Date#getTime()) rather than the old per-part floor.
Test covers both the lazy-stat and fstat-during-read paths and branches the raw-date assertion on isWindows only.
Extended reasoning...
Overview
The PR fixes Bun.file().lastModified for files with mtime before the Unix epoch. The old to_js_time cast a signed millisecond count to u64 and masked to 52 bits, wrapping negatives to huge positives. The fix changes JSTimeType to signed i64, computes total nanoseconds in i128, divides to ms (truncating toward zero), and clamps. The private timespec_parts helper in Stat.rs (which already encodes Node's Windows unsigned-32-bit-seconds semantics for fs.Stats) is lifted to pub(crate) and reused by a rewritten stat_to_js_mtime, and both open-coded call sites in read_file.rs (POSIX ReadFile::do_read_loop and Windows ReadFileUV::on_file_open) now route through it. A new test in bun-file.test.ts utimes a file to 1960 and asserts lastModified equals statSync().mtime.getTime() on both code paths.
Security risks
None. This is arithmetic on stat timestamps for a read-only property; no parsing of untrusted input, no auth, no allocation sizing.
Level of scrutiny
Moderate — cross-platform #[cfg]-gated code and a type change with multiple consumers. I grep'd every use of JSTimeType, INIT_TIMESTAMP, and to_js_time: the sentinel keeps its numeric value (1<<52)-1, and all readers either compare for equality (Blob.rs:2080) or cast to f64 (Blob.rs:2085, Blob.rs:2094), so signedness is transparent. On Windows, bun_sys::stat_mtime sign-extends the 32-bit libuv sec to i64; timespec_parts then narrows to i32 before the as u32 reinterpret, so the round-trip preserves Node's wrap for both pre-1970 (→ far future) and post-2038 (→ correct positive) dates. The i128 total-ns approach fixes the sub-ms rounding edge the PR notes (old code floored the nsec term separately).
Other factors
The change follows REVIEW.md's "fix the whole class" — all three stat→mtime sites now share one helper that inherits node:fs's already-vetted platform widening, and the dead PosixStat::init(&stat).mtime() route is removed. The test is in the correct existing file, uses tempDir from harness with using, exercises both the lazy-stat and fstat-during-read paths, and narrows the platform carve-out to a single assertion rather than skipping. Two commits landed after the prior review addressing bot feedback (the current diff reflects the consolidation into timespec_parts rather than a duplicated cfg split). No outstanding CHANGES_REQUESTED reviews.
This branch has not been deployed
2 participants
Problem
Bun.file(path).lastModifiedwraps modulo 2^52 for a file dated before 1970. A file with mtime 1960-01-01 reports4503284008170496(year 144673).fs.statSync(path).mtimeMsreports the correct-315619200000, as Node does.to_js_time(src/jsc/lib.rs:1548). It computed the millisecond count, cast it tou64, and masked it to 52 bits, becauseJSTimeTypewasu64(a port of Zig'su52). A negative count lost its sign.Fix
JSTimeTypeis nowi64.to_js_timecomputes ini128, truncates toward zero (likenew Date(mtimeMs).getTime()), and saturates toi64. TheINIT_TIMESTAMPsentinel keeps its value, so nothing else that reads it changes.resolve_file_stat,ReadFile,ReadFileUV) now share one helper,stat_to_js_mtime(&Stat). It widens the timespec through the sametimespec_partsthatnode:fsuses (hoisted to a free function inStat.rs). On Windows that reads the 32-bituv_timespec_t.secas unsigned, as Node does. SolastModifiedagrees withfs.statSync().mtimeMson every platform, and a 2040 mtime on Windows no longer wraps negative.test/js/bun/util/bun-file.test.ts(new test fails on stock bun on Linux and Windows, passes with the fix on both, and passed on macOS in CI). Also ranblob.test.ts,globals.test.js,structured-clone-blob-file.test.ts,bun-serve-file.test.ts,bun-file-read.test.ts,test/js/node/fs/fs.test.ts.Background
Bun.file()blob is backed by awebcore::Filestore. Itslast_modifiedfield starts at theINIT_TIMESTAMPsentinel (2^52 - 1) and is filled lazily: bystaton the firstlastModifiedread, or by thefstatthat runs during a read.bun_sys::Statis libuv'suv_stat_t, whose timespec seconds are a Clong(32 bits). Node reads that field asunsigned longto push the overflow from 2038 to 2106, at the cost of pre-1970 dates. Bun'snode:fsalready does the same.Notes
touch -d @-315619200 neg.js && bun -e 'console.log(Bun.file("neg.js").lastModified, require("fs").statSync("neg.js").mtimeMs)'printed4503284008170496 -315619200000. With the fix:-315619200000 -315619200000.utimes -0.4996giveslastModified === -499, equal tostat.mtime.getTime(). The old formula floored the nanosecond part separately and would give-500.utimesdate,mtime.getTime(),stat.mtime.getTime(), lazylastModified, after-readlastModified):1960-01-01 -315619200000 3979348096000 3979348096000 3979348096000,2040-06-01 2222121600000 2222121600000 2222121600000 2222121600000. Before the fix both dates gave a value near 2^52.Bun.servestatic file routes still send noLast-Modifiedheader for a pre-1970 file.StatHash::hash(src/resolver/fs/stat_hash.rs) clamps a negative mtime to 0 on purpose and omits the header. That is a separate decision.Blobstill return the sentinel fromlastModified. fix(BunFile): lastModified returns 0 for missing files instead of 2^52-1 sentinel #33652 covers that.bun-write.test.jshas five tests that time out at 5 s under the debug ASAN build in this container, with and without this diff.no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/bun-file.test.ts