Skip to content

De-flake atomicWriteFile CONCURRENT_EDIT mtime test - #273

Merged
philcunliffe merged 1 commit into
masterfrom
fix/issue-272
Jul 7, 2026
Merged

De-flake atomicWriteFile CONCURRENT_EDIT mtime test#273
philcunliffe merged 1 commit into
masterfrom
fix/issue-272

Conversation

@philcunliffe

@philcunliffephilcunliffe commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What

De-flakes atomicWriteFile enforces expectedMtimeMs (CONCURRENT_EDIT) in test/core/util-fs-atomic.test.js, which intermittently failed on fast CI with AssertionError: Missing expected rejection (confirmed non-deterministic on PRs #259 and #266 — same commit failed one test job, passed the sibling, cleared on re-run).

Why

The stale-mtime assertion relied on the preceding atomicWriteFile('v2', …) bumping the target's on-disk mtime past the caller's captured mtimeMs. On a fast runner both writes land within the same filesystem mtime tick, so the on-disk mtime still equals expectedMtimeMs, the guard (current.mtimeMs !== expectedMtimeMs) never fires, and no CONCURRENT_EDIT rejection happens.

Reproduced faithfully by pinning the on-disk mtime to a coarse whole-ms tick and re-pinning it after the write: same-tick → guard silent → no rejection.

Fix

Simulate the concurrent edit deterministically: bump the target's on-disk mtime with fs.utimes to a value strictly distinct from expectedMtimeMs (+1000ms) before the guarded write. The CONCURRENT_EDIT rejection is now asserted independent of filesystem mtime granularity. Test-only change — production atomicWriteFile behavior is unchanged.

Verification

  • Targeted file node --test test/core/util-fs-atomic.test.js × 20: all pass.
  • npm test: 1875 pass, 0 fail, 1 skipped.
  • npm run typecheck and npm run build:types: clean (exit 0).

Note (out of scope)

The production guard has an inherent same-tick blind spot: a concurrent writer that modifies the file within the same mtime tick as the caller's read is undetectable by mtime comparison. This is the standard TOCTOU limitation of lightweight mtime-based optimistic-concurrency guards, not a regression — left unchanged here.

Fixes#272

The stale-mtime assertion relied on the preceding atomicWriteFile('v2')
landing in a later filesystem mtime tick than the caller's read. On a
fast runner both writes share one mtime tick, so the on-disk mtime still
equals expectedMtimeMs, the guard (current.mtimeMs !== expectedMtimeMs)
never fires, and assert.rejects fails with "Missing expected rejection".
Simulate the concurrent edit deterministically by bumping the on-disk
mtime with fs.utimes to a value strictly distinct from expectedMtimeMs
before the guarded write, so the CONCURRENT_EDIT rejection is asserted
independent of filesystem mtime granularity. Production atomicWriteFile
behavior is unchanged.
Fixes#272
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

🔍 neutral review — approve

Focused review, scaled to a 2-line, test-only change. The diff is exactly a guaranteed-distinct mtime bump (fs.utimes(target, mtimeMs + 1000)) inside the CONCURRENT_EDIT test's stale-mtime block, so the rejection fires independent of filesystem mtime granularity. Production atomicWriteFile is untouched; the branch diff is the single test file, nothing else.

Evidence it works: CI is green and the previously-flaky test job passed in both parallel runs on this branch (the exact split that was failing on #259/#266), and the fixer reproduced the same-tick root cause and proved determinism over 20 local iterations.

Verdict: approve. Held for a human to merge — closes #272 via Fixes #272. neutral does not merge.

Automated neutral review (focused — scaled to the change's stakes).

@philcunliffe
philcunliffe merged commit f00bf33 into masterJul 7, 2026
4 checks passed
@philcunliffe
philcunliffe deleted the fix/issue-272 branch July 7, 2026 18:22
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: atomicWriteFile enforces expectedMtimeMs (CONCURRENT_EDIT) fails on fast CI (mtime granularity)

1 participant

@philcunliffe