fix: use actual file mtime instead of Date.now() in FileTime tracking #9450
Conversation
- Change FileTime.read() to store actual file mtime instead of Date.now() - This fixes timing jitter issues where mtime could be ahead of Date.now() - Make FileTime.read() async and add await to all callers - Fix apply_patch logic to not read mtime of deleted/moved files - Add 25ms delay before test cleanup to allow file handles to release
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: Potential Duplicate FoundPR #6744: fix(file): resolve 1ms timestamp race in sequential file edits (#8624) This PR appears to be addressing the same issue - it's specifically about resolving a 1ms timestamp race condition in file edits, which is exactly what the current PR (9450) is fixing by using actual file mtime instead of Date.now() to avoid timing jitter issues. |
has anyone ever run the test suite on a win32 system? (these show where it applies) |
|
Sorry I don't know what you mean. There is no win64 in nodejs All modern windows systems report Did you assume I was testing with Windows 95? |
|
no. i actually meant windows 64 bit. maybe im wrong. |
It's ridiculous you assume anyone talks about a 32bit system. There is an exact 0% chance that anyone ran your test suite even once successfully on any Windows. |
2 participants
Store actual file mtime instead of Date.now()
Currently.
Date.now()is stored as file mtime, instead of the actual file mtime.Issue becomes immediately apparent on Windows due to how Windows updates file timestamps, resulting in ~1ms difference, causing flaky fails in
apply_patch.test.tsFix
FileTime.read()to store actual file mtime instead ofDate.now()Date.now()FileTime.read()asyncand addawaitto all callers