fs: use signed types for stat data

[LiviaMedeiros]

Fixes: #43707, #32369

On Windows, right now we have to choose between pre-1970 and post-2038 dates. Post-2038 seems to be preferable so the fix is not fully applied on this platform.

Observations on different platforms

Casting to unsigned long, local test:

• expected 355, got 354.999
• EINVAL on -10000000000000
• EINVAL on 10000000000000

Test results:

• https://ci.nodejs.org/job/node-stress-single-test/352/
• https://ci.nodejs.org/job/node-stress-single-test/353/

Casting to double, initial test:

• aix71-ppc64: EINVAL
• osx11: expected 10000000000000 got 9223372036854.775
• rhel8-x64: expected 10000000000000 got 2147483647000

Casting to double or long, verbose test:

• aix71-ppc64/ppc64: rounds down to seconds, EINVALs on negative numbers

• alpine-last-latest-x64, alpine-latest-x64, centos7-ppcle, debian10-x64, rhel7-s390x, ubuntu1804-64: good results, having rounding errors on JS layer and EINVALs on {MIN,MAX}_SAFE_INTEGER

• fedora-last-latest-x64, fedora-latest-x64, rhel8-s390x, rhel8-x64: -10000000000000 gets rounded to -2147483648000, 100000000000000 gets rounded to 15032385535000

• freebsd12-x64: messed up atime and mtime?

• osx1015: never EINVALs, caps at about 9223372036855

• osx11: defaults to 0 on {under,over}flow

• smartos18-64, smartos20-64: EOVERFLOW before EINVAL

• ubuntu2004-arm64: never EINVALs, defaults to 0 instead

• win*: not reported due to node-stress-single-test is broken on win build#2991

[bnoordhuis]

This may bring back #12607, #13255 and/or the infamous #12115. They are why the cast to unsigned long was introduced, IIRC.

[LiviaMedeiros]

Is there a repro for that? I can understand why cast is needed for native JS numbers but not when it's transferred as Float64s. If the bug is platform dependent, CI should catch that.

I've tried to put numbers from #13255 in test-fs-stat-date.mjs on amd64 and got following results:

// both PR and v18.4.0:
+ 1713037251359.999
- 1713037251360

// only v18.4.0, PR passes successfully:
+ 1.8446744071562068e+22
- -2147483648000

[LiviaMedeiros]

After reading those fully:

• src: round nsec instead of truncating stat times #12607 mainly has discussion about Date rounding which is a bit hacky and relied on non-negative numbers. I adjusted that part although it can be refactored into something else.
• utimes precision errors #13255 as mentioned above, I can reproduce precision loss from this issue on v18.4.0. On top of that, I can reproduce much worse results on 32bit build. I don't see how cast to unsigned long helps.
• Node occasionally gives multiple files/folders the same inode #12115 is unrelated: it's about st_ino collisions which are not affected by this particular cast.

Overall it seems that precision loss is inevitable and accepted, and the solution is to simply use {bigint:true} whenever we deal with very precise and/or very large dates.

I've ran a dumb test to see precision regression between long and double and didn't see any difference (however, I'll definitely have to exclude borderline values from proper test 😅).
https://ci.nodejs.org/job/node-stress-single-test/352/ - double
https://ci.nodejs.org/job/node-stress-single-test/353/ - long

Of course if I'm missing something else, it can be reverted to long. The only reason why I prefer double is that these values will end up being interptered as 64-bit floats on JS side.

[LiviaMedeiros]

You were right, it did indeed bring back "overflow" from #13255 on win32.
Adjusted the code so it only affects other platforms now.

Any chance for this long to become longer? I'd expect int64_t or at least time_t but not sure how breaking it would be.
https://github.com/libuv/libuv/blob/c4d73c306b87e32acba3a77f933da72ac5604659/src/win/fs.c#L100

[bnoordhuis]

Breaking, I'm afraid. long was the wrong choice even back in 2013¹, when uv_timespec_t was introduced, but libuv can't change that without breaking ABI. How problematic is this for you?

¹ Gah, I even pointed it out but this particular instance slipped through.

[LiviaMedeiros]

It leaves us with a choice on windows: we either support negative half of int32, or upper half of uint32. Y2038 is pretty close and we have an explicit test that it works:

node/test/parallel/test-fs-utimes-y2K38.js

Lines 45 to 48 in acd2a32

	const Y2K38_mtime = 2 ** 31;
	fs.utimesSync(path, Y2K38_mtime, Y2K38_mtime);
	const Y2K38_stats = fs.statSync(path);
	assert.strictEqual(Y2K38_stats.mtime.getTime() / 1000, Y2K38_mtime);

In current state of this PR, it fixes the issue for all platforms except windows, which is IMO the best solution for now.

On a larger scale, if Node.js will be ready to migrate to newer ABI, I'd like to see more appropriate types. In JS part, I think it would be reasonable to drop Date quirks in favor of Temporal: this will mean switching from ms to ns as "default" units, making bigints more necessary and double/Float64 less accurate.

[bnoordhuis]

I completely forgot but I tried fixing pretty much the exact same issue in pretty much the exact same way in #32408... and looks like I ran into pretty much the exact same issues as you do now. 🤦

You can add a Fixes: #32369 if/when this gets merged. That issue got closed by accident because I added a Fixes: tag to the libuv commit. Again 🤦

[LiviaMedeiros]

Huh, I guess searching for previous issues could save quite a time here. 😅

[bnoordhuis]

Test failure looks related, I'm afraid:

AssertionError [ERR_ASSERTION]: expected 10000000000000 ± 2.220446049250313e-16, got 9223372036854.775
    at closeEnough (file:///Users/runner/work/node/node/test/parallel/test-fs-stat-date.mjs:18:10)
    at runTest (file:///Users/runner/work/node/node/test/parallel/test-fs-stat-date.mjs:27:3)
    at async file:///Users/runner/work/node/node/test/parallel/test-fs-stat-date.mjs:62:3 {

[LiviaMedeiros]

Sure, there are lots of different failures on borderline values. 😅
I'd really want to test them because it is where slightest regression would be caught first, but it seems to be futile to keep it cross-platform, probably allowlists of process.arch & process.platform wouldn't be robust nor helpful.
Aiming on non-flaky green CI right now.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/45208/

[LiviaMedeiros]

https://ci.nodejs.org/job/node-test-binary-windows-js-suites/15529/RUN_SUBSET=1,nodes=win10-COMPILED_BY-vs2019/testReport/junit/(root)/test/parallel_test_fs_utimes_y2K38/

node:assert:124
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ -2147483648
- 2147483648
    at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-fs-utimes-y2K38.js:48:8)
    at Module._compile (node:internal/modules/cjs/loader:1120:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1174:10)
    at Module.load (node:internal/modules/cjs/loader:998:32)
    at Module._load (node:internal/modules/cjs/loader:839:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: -2147483648,
  expected: 2147483648,
  operator: 'strictEqual'
}

Node.js v19.0.0-pre

node/test/parallel/test-fs-utimes-y2K38.js

Lines 57 to 65 in acd2a32

	// test Y2K38 for windows
	// This value if treaded as a `signed long` gets converted to -2135622133469.
	// POSIX systems stores timestamps in {long t_sec, long t_usec}.
	// NTFS stores times in nanoseconds in a single `uint64_t`, so when libuv
	// calculates (long)`uv_timespec_t.tv_sec` we get 2's complement.
	const overflow_mtime = 2159345162531;
	fs.utimesSync(path, overflow_mtime / 1000, overflow_mtime / 1000);
	const overflow_stats = fs.statSync(path);
	assert.strictEqual(overflow_stats.mtime.getTime(), overflow_mtime);

Should this be kept? It is kept.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/45226/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/45237/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/45278/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/45280/

[targos]

It seems that this doesn't work on arm-12+ on the v16.x-staging branch.
See #44070 / https://ci.nodejs.org/job/node-test-binary-arm-12+/15463/#showFailuresLink

node:internal/process/esm_loader:97
    internalBinding('errors').triggerUncaughtException(
                              ^

AssertionError [ERR_ASSERTION]: expected -40691 ± 1.0000000000000002, got 0
    at closeEnough (file:///home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-fs-stat-date.mjs:26:10)
    at runTest (file:///home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-fs-stat-date.mjs:40:3)
    at async file:///home/iojs/build/workspace/node-test-binary-arm/test/parallel/test-fs-stat-date.mjs:70:5 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: '=='
}

[targos]

Will have to land with #44174

[juanarbol]

As mentioned in #44174 (comment) this commit is a bit problematic in the v16.x release branch. I will mark this as dont-land-on-v16.x.