Repository navigation
fs: preserve negative utimes timestamps - #66634
guybedford wants to merge 2 commits into
Conversation
`toUnixTimestamp()` silently replaced negative numeric timestamps with the current time, while negative strings and Dates were passed through as pre-epoch times. On Windows, stat times were additionally reinterpreted as unsigned so that pre-epoch times read back as post-2038 times. Treat negative times as pre-epoch on all platforms, consistent with POSIX, at the cost of post-2038 times on Windows which libuv is unable to represent. Fixes: nodejs#64597 Refs: nodejs#64627 Assisted-by: OpenCode Signed-off-by: Guy Bedford <guybedford@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #66634 +/- ##
==========================================
- Coverage 92.78% 90.51% -2.28%
==========================================
Files 422 797 +375
Lines 193692 278189 +84497
Branches 29881 53458 +23577
==========================================
+ Hits 179718 251801 +72083
- Misses 13645 16766 +3121
- Partials 329 9622 +9293
🚀 New features to boost your workflow:
|
|
@aduh95 would value your review on this one given it's a major change. Otherwise another @nodejs/tsc review would be appreciated. |
LiviaMedeiros
left a comment
There was a problem hiding this comment.
LGTM with changelog entries added to the docs.
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3798/
Assisted-by: Claude Code Signed-off-by: Guy Bedford <guybedford@gmail.com>
|
CITGM failures seem unrelated to this change - seems like the suite might not be passing on main. |
The baseline of citgm is failing on lots of tests and a lot of the failures in https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3798/ will be unrelated. I've been working with package owners the last few weeks to clean things up. Only a strict comparison before / after the PR will give interpretable results. Since this PR is semver-major, there should be plenty of time during the 27-Alpha to sort this out (hopefully)! |
This is the semver-major alternative to #64627, fixes #64597.
toUnixTimestamp()silently replaced negative numeric timestamps with the current time, while negative strings andDates were passed through as pre-epoch times:Removing the clamp exposed that Windows reinterprets stat
tv_secasunsigned long, so that pre-epoch times read back as post-2038 times (viastatic_cast<unsigned long>inFillStatsArray). This was a Node.js decision from #43714 to work around libuv's 32-bitlong tv_secon Windows, which is what thetest-fs-utimes-y2K38.jsWindowsoverflow_mtimecase covered. As discussed in #64627, this PR treats negative times as pre-epoch on all platforms, consistent with POSIX:toUnixTimestamp()no longer replaces negative numbers with the current time.2038-01-19T03:14:07Zare no longer representable on Windows until libuv gains 64-bit time support.atime/mtimerules and the Windows time range.Test changes:
test-fs-utimes.jsnow computes the expected mtime independently offs._toUnixTimestamp()(previously the oracle was the code under test, so the negative case always passed), uses an absolute difference, and adds negative number and pre-epochDatecases. These are skipped on AIX, which rejects pre-epoch times withEINVAL.test-fs-utimes-y2K38.jsskips the Y2K38 round trip on Windows and drops theoverflow_mtimecase, keeping the Windows precision check.A CITGM run is suggested per the discussion in #64627.