test: give the flushed compaction loop a timeout that fits it - #54
Merged
Conversation
This test failed once on a Windows runner at the 5 s default, on a pull request that bumped two lint packages — nothing that could touch it. It runs a hundred storage flushes: twenty appends, and per iteration a checkpoint file plus its directory and a compacted file plus the directory its rename lands in. Locally that is 134 ms; on a loaded runner it passed 5 s. Raising the limit rather than shrinking the test is the point. Dropping iterations would weaken what it checks — accumulation is what repetition exposes — and dropping `fsync` would move it off the path that writes the temporaries at all. What varies here is the filesystem, not the code, which is the same reason the measured append range spans 0.47-1.49 ms between NTFS and ext4. 30 s matches what the property tests already use. Confirmed the argument is honoured rather than assumed: set to 1 ms it fails with "Test timed out in 1ms", so the limit is real and not decorative.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The failure
Node 24 / windows-latestwent red on #48 — a pull request bumpingpublintandtypescript-eslint, neither of which can reach this code:Re-running the job passed, and it is the only CI failure in the last 25 runs. So: a real flake, not a regression, and not caused by the pull request it blocked.
Why this test is slow
It is the only test in the suite that loops flushed writes. With
fsync: true, each of its 20 iterations costs five storage flushes:appendcheckpointflush: true) +syncDirectoryafter the renamecompactsyncDirectoryafter the renameA hundred flushes in total. On my Windows machine that is 134 ms; on a loaded CI runner it exceeded 5000 ms — a 37× spread. That number is a property of the host filesystem, not of the code, which is the same reason the README quotes the
fsync: trueappend cost as a range (0.47–1.49 ms) rather than a figure.Since the test body is synchronous, the timeout cannot interrupt it — it is a "this took too long" assertion measured after the fact.
The fix, and what it deliberately is not
Raised to
30_000, matching whatwal.property.test.tsalready uses.Neither cheaper option was acceptable:
fsync: true— that moves the test off the path that writes the temporaries it is checking for.Both would have been weakening a test to make CI green, which is the one thing AGENTS.md rules out.
Proof the limit is real
That the test passes proves nothing — it passed before. So I checked the argument is actually honoured, by setting it to 1 ms:
The limit is wired, not decorative. Restored to
30_000afterwards.Contract impact
None. Test-only, and no changelog entry — nothing observable changes for a consumer.