fix(core): never trust a config-cache entry read inside the file's own mtime tick - #663
Conversation
…n mtime tick
`MtimeCache` and the `detect_project` memo both validate an entry on a
`(mtime, size)` stamp. mtime resolution is the platform clock's, not the
filesystem's, and on Windows two writes separated only by a read collide on
mtime the majority of the time — measured at ~62% over 6000 trials on Windows
Server 2022/NTFS, with the smallest observed nonzero mtime delta between 0.39ms
and 1.4ms. When such a rewrite also keeps the file's length, neither half of the
stamp can tell the two versions apart and the cache serves the pre-write value.
That is the `pnpm_v11_surfaces_need_a_declared_v11_incumbent` flake: it rewrites
one `package.json` from `{"packageManager":"pnpm@10.0.0"}` to
`{"packageManager":"pnpm@11.0.0"}` — 32 bytes either way — and re-reads it, so
`pnpm_v11_surface` saw the stale pin and reported no v11 surface.
Apply git's "racily clean" rule to both caches: record the instant sampled just
before the read, and serve an entry only while the file's mtime is strictly
older than it. A stamp taken in the same tick as the observation cannot prove
the file did not change afterwards. Config already on disk when the process
started — every real `.npmrc` / `package.json` / `.yarnrc.yml`, and the whole
reason the cache exists — still hits; only a file written within the tick before
the read goes uncached, at no extra I/O.
Both caches needed it. The memo caches the parsed manifest, so a stale `Project`
reaches `pm::resolve::root_manifest` even when the manifest cache itself misses
— fixing only `MtimeCache` would have left the flake in place. Verified by
removing each guard in turn and watching its regression test serve the stale
`pnpm@10.0.0`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…not FAT32-only The comment claimed "every other file system reports nanosecond mtime so the trust is sound", scoping the same-size-same-mtime residual to FAT32's whole seconds. Measured on Windows Server 2022/NTFS while diagnosing the config-cache flake: mtime resolution is the platform CLOCK's, quantized to the timer tick at 0.39-1.4ms, and two same-size writes separated only by a read collide in ~62% of 6000 trials. Comment only — `is_fresh` keeps its hash fallback and its behavior is unchanged. Whether any aube writer actually rewrites to the same length within a tick of a snapshot is a separate question this does not answer.
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-only CI flake caused by coarse mtime resolution allowing same-size rewrites (e.g. pnpm@10.0.0 → pnpm@11.0.0) to keep an identical (mtime, size) stamp and be served stale from per-process caches. The change adopts a git-style “racily clean” rule by adding a cached_at instant and only serving cached entries when the file’s mtime is strictly older than that instant.
Changes:
- Add a
cached_at: SystemTimeguard toMtimeCacheentries and requirestamp.mtime < cached_aton cache hits. - Add the same
cached_atguard to thedetect_projectmemoization entry validation, to avoid reusing a stale parsed manifest under a stamp collision. - Add deterministic regression tests that pin mtimes to reproduce same-tick collisions reliably across platforms.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/nub-core/src/workspace/detect.rs |
Adds cached_at to the project-detection memo and validates manifest stamps with a strict “mtime older than cached_at” rule; adds a regression test reproducing same-tick rewrites. |
crates/nub-core/src/config_cache.rs |
Adds cached_at to MtimeCache entries and gates cache hits on stamp.mtime < cached_at; updates docs and adds deterministic tests for same-tick rewrites (same-size and size-changing). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //! entries also carry the instant they were cached and are trusted only once | ||
| //! the file's mtime is strictly OLDER than that instant. This is git's | ||
| //! "racily clean" rule: a stamp taken in the same tick as the observation | ||
| //! cannot prove the file did not change afterwards, so it is never trusted on | ||
| //! its own. See [`Entry::cached_at`]. |
| /// A same-length rewrite landing inside the walk's own clock tick flips no stamp | ||
| /// at all, which is what [`Entry::cached_at`] exists to catch. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
The racily-clean guard compares a filesystem timestamp against a wall-clock sample. On Windows those are different clock domains and the comparison stays true through exactly the rewrite it is meant to reject, so the flake is very likely still live. Details inline on config_cache.rs:149.
Reviewed changes — full diff of 5bd3f2b (2 files, 1 commit) plus the caches' production call sites and the flaking test at crates/nub-cli/src/pm_engine/mod.rs:4426.
MtimeCacheracy-window guard —Entrygainscached_at: SystemTime, sampled before the read; a hit now also requiresstamp.mtime < entry.cached_at(config_cache.rs:149).detect_projectmemo guard — the same rule applied to every stamped manifest inProjectCache::lookup_fresh, withcached_atthreaded throughinsert(detect.rs:69,:271,:281).- Test rework —
same_mtime_different_size_invalidatesreplaced byrewrite_inside_the_cached_mtime_tick_is_never_served_stale, a newmanifest_rewritten_inside_its_mtime_tick_is_never_served_staleindetect.rs, andset_mtime/settlehelpers duplicated into both test modules. - Doc rewrite — the module header,
Stamp, and bothEntrytypes now document the racy window as the thing that closes the no-stale-read argument.
⚠️ Nothing here was validated against a real coarse-clock platform
Both regression tests synthesize the race by writing a future mtime, which is a state the Windows clock never produces on its own. The one measurement the PR cites — 62% mtime collisions on Windows Server 2022 — characterizes the bug, not the fix. Before merging, the guard should be exercised on a real windows-2022 runner: write, read through the cache, rewrite same-length, read again, and assert the second read sees the new bytes. The repo's ci-adhoc-test skill runs exactly this shape from a branch with no PR required.
Technical details
# Validate the guard on a real Windows runner before merging
## Required outcome
- Empirical evidence, from a `windows-2022` runner, that the same-length
rewrite is no longer served stale — not a synthetic `set_modified` fixture.
## Suggested approach
- A branch-scoped probe (`ci-adhoc-test`) that, in one process:
1. prints `SystemTime::now()` and `metadata().modified()` immediately after a
write, to record the sign of the gap (this alone settles whether the guard
can ever fire);
2. runs the write / cached-read / same-length-rewrite / cached-read loop a few
thousand times and counts stale hits.
- Run it against `main` and against this branch. If the stale-hit count is
unchanged, the guard is inert and the fix needs the clock-domain correction
described on `config_cache.rs:149`.Claude Opus ｜ 𝕏
| assert_eq!(&*short, "short", "size change must invalidate the cache"); | ||
| for (first, second) in [ | ||
| ("pnpm@10.0.0", "pnpm@11.0.0"), // same length — the flake's shape | ||
| ("longer-contents", "short"), // length also changes |
There was a problem hiding this comment.
Stamp.size lost its only coverage here. Under the pinned-future mtime the cached_at guard rejects the entry before size is ever the deciding field, and no other test in the module distinguishes a length change — deleting size from Stamp leaves the suite green. Worth keeping the old rewrite-hammer loop alongside the new case.
There was a problem hiding this comment.
ℹ️ The new commit is doc-only and clean. One correction to my own previous review below; the
config_cache.rs:149finding is unaffected and still open.
Reviewed changes — delta from 5bd3f2b to cdfd1c2 (1 new commit, 1 new file).
FreshnessSnapshot::is_freshstaleness note corrected (vendor/aube/crates/aube-util/src/cache.rs:193-203) — replaces the claim that the same-mtime same-size residual is FAT32-only and that "every other file system reports nanosecond mtime", with the measured Windows Server 2022 / NTFS behavior. Comment-only;is_fresh's body is untouched and the description matches it.FreshnessSnapshothas no consumers anywhere in this tree, so the residual it now documents honestly is not reachable today and needs no fix.
ℹ️ Correcting a number in my previous review, and why it does not move the finding
I wrote "~15.6 ms system clock" on config_cache.rs:149. This commit's measurement supersedes that: 0.39–1.4 ms on Server 2022 / NTFS. The argument does not depend on the magnitude — only on the direction, that a coarse-quantized mtime lags a sub-µs GetSystemTimePreciseAsFileTime. A 1 ms quantum leaves stamp.mtime < cached_at just as true as a 15.6 ms one, so the guard still passes through the same-tick rewrite.
The commit message here is in fact the clearest statement of the problem: "mtime resolution is the platform CLOCK's". SystemTime::now() reads a different, finer clock than the one the filesystem stamps with, and that is the mismatch the guard rests on.
Claude Opus ｜ 𝕏
…t it it was a no-op The previous commit's `stamp.mtime < cached_at` never rejects anything, so it fixed nothing. `SystemTime::now()` is PRECISE (`GetSystemTimePreciseAsFileTime` on Windows, nanoseconds elsewhere) while the file mtime it was compared against is QUANTIZED to the timer tick, so for any just-written file `mtime <= now()` holds by construction. Measured both ways: 0 rejections in 2000 trials on macOS, and on Windows the guard rejected 0 of 1671 stale hits — an 83.55% stale rate identical to no guard at all, against a healthy positive control. The error was comparing across clock domains. Git does not: it compares an index entry's mtime against the index FILE's mtime, two values quantized identically. Require a granularity slop instead, so the mtime's quantum had provably closed before the caching read: serve only when `cached_at - stamp.mtime >= MTIME_GRANULARITY_SLOP`. Given mtime `M = quantize(T_write)` and a read at `R`, `M + G <= R` means the quantum `[M, M+G)` closed before `R`, so every later write lands in a later quantum and reports a different mtime — the stamp then genuinely detects it. Two seconds covers the 0.39-1.4ms measured on Windows/NTFS, the 15.6ms Windows timer tick, and FAT32's 2s. It costs nothing real: config nub reads is seconds to days old and still hits; only a file written within two seconds of the read goes uncached, falling back to the read this cache replaced. Measured: 0/2000 just-written files trusted, 2000/2000 60s-old files trusted. The tests were the deeper failure and are rebuilt. They pinned the mtime to `now() + 50ms` — a FUTURE instant no clock hands out — which made the broken guard look effective. Both now pin the collision to `now()`, where a real coarse-clock write lands, and both FAIL against the previous commit's guard, so they would have caught it. Tests asserting a cache HIT age their fixture rather than spinning for a clock tick, which is also what every real on-disk config file looks like.
There was a problem hiding this comment.
ℹ️ The clock-domain blocker is properly fixed — no new correctness issues. Two doc/test nits inline.
Reviewed changes — delta from cdfd1c2 to 573ebc3 (1 new commit), plus the boundary arithmetic of the new constant, every MtimeCache and detect_project consumer, and the tests that would break if the raised miss rate mattered.
MTIME_GRANULARITY_SLOPmakes the guard real —config_cache.rs:58introduces a 2 s constant andmtime_quantum_closed(:174), replacing the barestamp.mtime < cached_atthat the previous review showed was inert.detect.rs:272calls the same helper, so the two caches can no longer drift apart. The boundary is exactly right: with floor quantization, a write att1 >= cached_atyieldsmtime' > t1 - T >= cached_at - T >= stamp.mtime, so>= 2sstrictly excludes a collision even at FAT32's 2 s quantum.- Tests now model the real relationship — both racy tests pin the colliding stamp to
now()instead ofnow() + 50ms, andsettle()is replaced byage(), which backdates a fixture clear of the slop. Both fail ifmtime_quantum_closedis neutered totrue, so they are genuine coverage rather than theatre. - Empirical validation landed — the module doc records that the bare check rejected 0 of 2000 trials on macOS and Windows, and that the slop trusts 0/2000 just-written files while still hitting on 2000/2000 aged ones. That retires the "nothing validated against a real coarse-clock platform" concern from the first review.
- Docs rewritten around the quantum framing — the module header,
Stamp, and bothEntrytypes now explain why comparing a precise clock against a quantized timestamp needs the slop to mean anything.
Three prior threads (config_cache.rs:149, :291, detect.rs:271) are resolved. The Stamp.size coverage thread at config_cache.rs:312 is left open — the ("longer-contents", "short") row still cannot reach size, because the slop guard short-circuits before length is the deciding field.
Claude Opus ｜ 𝕏
| /// days old, so it clears the slop and still hits. Only a file written within two | ||
| /// seconds of the read goes uncached, and the fallback there is exactly the | ||
| /// uncached read this cache replaced. | ||
| const MTIME_GRANULARITY_SLOP: Duration = Duration::from_secs(2); |
There was a problem hiding this comment.
Two small overclaims in an otherwise exact comment. FAT32's 2 s quantum leaves zero margin, not margin — >= 2s is the smallest constant that still proves the guarantee there, so a future reader shouldn't read "generously sized" as slack they can spend. And the proof assumes mtime and SystemTime::now() are the same clock, which holds on every local filesystem but not on NFS/SMB, where the server stamps mtime.
Technical details
# The slop's justification is tighter than the comment says
## Affected sites
- `crates/nub-core/src/config_cache.rs:51-57` — "Two seconds covers every quantum
nub can land on with margin" and "Sized generously". Verified granularities:
NTFS/ReFS 100 ns, ext4 ns (1 s on 128-byte inodes), XFS/btrfs/APFS ns, HFS+ 1 s,
exFAT 10 ms, tmpfs ns — all with real headroom. FAT32 is 2 s
(https://learn.microsoft.com/en-us/windows/win32/sysinfo/file-times), which the
constant matches exactly rather than exceeding.
- `crates/nub-core/src/config_cache.rs:174-178` — `mtime_quantum_closed` compares a
filesystem timestamp against a local `SystemTime::now()` sample. On NFS (v3/v4)
and SMB the mtime is stamped by the *server's* clock; neither protocol bounds
client/server skew (SMB tolerates minutes for Kerberos). A server clock lagging
the client by more than the slop makes a just-written file look aged, so the
entry is trusted immediately and a same-quantum rewrite can still be served
stale.
## Required outcome
- The comment should state the FAT32 case as the tight bound it is, and name the
same-clock-domain assumption as an assumption.
## Suggested approach (optional)
- No code change. Two sentences: `>= 2s` is exactly sufficient at a 2 s quantum,
and the argument holds only where the writer's clock is the reader's clock, so
a network mount whose server clock lags by more than the slop keeps the
residual. Worth stating explicitly because it is strictly better than the
pre-PR behaviour (no guard at all) and shouldn't read as a regression.| ("pnpm@10.0.0", "pnpm@11.0.0"), // same length — the flake's shape | ||
| ("longer-contents", "short"), // length also changes | ||
| ] { | ||
| let collided = SystemTime::now(); |
There was a problem hiding this comment.
Pinning to now() gives the test a 2 s deadline it doesn't state: if more than MTIME_GRANULARITY_SLOP elapses between collided and the first get_or_read, the entry becomes trusted and the run fails on the staleness assertion below, pointing at the cache rather than at the stall. Worth asserting the budget so a wedged runner fails with a diagnostic. Same shape at detect.rs:442.
Technical details
# The racy tests fail misleadingly if the harness stalls past the slop
## Affected sites
- `crates/nub-core/src/config_cache.rs:314-332` — `collided = SystemTime::now()`,
then a write, a `set_mtime`, and the first `get_or_read` whose internal
`cached_at` must land within 2 s of `collided` for the collision to be modeled.
- `crates/nub-core/src/workspace/detect.rs:442-464` — same construction, with a
full `detect_project` walk inside the window.
## Required outcome
- A run that blows the slop budget reports *that*, instead of "a rewrite sharing
the cached mtime must not serve ...", which sends the reader after a
non-existent cache bug.
## Suggested approach (optional)
- After the first `get_or_read`, assert `collided.elapsed().unwrap() <
MTIME_GRANULARITY_SLOP` with a message naming the stall. It is not a guard
against an impossible case — it is the difference between a self-debugging
failure and one that indicts the code under test.
|
Shipped in v0.7.0: https://github.com/nubjs/nub/releases/tag/v0.7.0 |
Fixes the Windows-only flake in
pnpm_v11_surfaces_need_a_declared_v11_incumbent, which reds CI onmain.MtimeCacheand thedetect_projectmemo validate entries on a(mtime, size)stamp. File mtimes are quantized to the platform timer tick (0.39-1.4 ms measured on Windows Server 2022/NTFS), so two same-size writes separated only by a read collide on mtime 62-84% of the time and the stale value is served. The test rewritespnpm@10.0.0topnpm@11.0.0— 32 bytes either way.Both caches now require a two-second granularity slop between a file's mtime and the read that cached it, proving the mtime's quantum had closed. Measured: 0/2000 just-written files trusted, 2000/2000 aged files still hit.
The second commit corrects the first, which was a no-op.