fix(test): derive the prune cut from recorded timestamps, not a fixed 6s - #5990
fix(test): derive the prune cut from recorded timestamps, not a fixed 6s#5990Hmbown wants to merge 2 commits into
Conversation
`prune_older_than_keeps_the_newest_and_drops_only_the_old_tail` fails
intermittently on windows-latest with
assertion `left == right` failed: only the old tail should be removed
left: 3
right: 2
The fixture builds two old snapshots, sleeps 8s, then two new ones 1.1s
apart, and cuts at a hardcoded 6s. That assumes `repo.snapshot()` is fast:
`new:0` is only ~1.2s plus one git subprocess older than prune time, so on
a loaded Windows runner that subprocess alone carries it past the 6s line
and it is pruned with the old pair.
The existing fixture guard could not catch it — it asserts on `before[0]`
and `before[2]`, and `before[1]` is the entry that drifts.
The cut is now computed from the timestamps the repo actually recorded:
aim at the midpoint of the gap between the oldest survivor and the newest
victim, which leaves ~4s of slack in both directions instead of depending
on wall-clock luck. The gap itself is asserted first, so a fixture that
collapsed says so plainly rather than failing later as a count mismatch.
Behaviour under test is unchanged: two removed, `new:1` and `new:0`
survive. No production code is touched.
cargo clippy -p codewhale-tui --lib -> 0 errors
cargo test -p codewhale-tui --lib -- prune_older_than
-> test result: ok. 3 passed; 0 failed
Found when it failed the windows leg of #5987, a PR containing zero Rust
files (TypeScript, CI config and .gitignore only), so it cannot have been
caused there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D4rk4NXwyy6wmvii9Lp84P
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ce0e407c-bf19-4a34-ab62-52fa8662bdfe) |
|
Claude finished @Hmbown's task in 1m 39s —— View job Review: fix(test) — derive prune cut from recorded timestamps
Summary: This is a test-only change to I traced the new cut derivation against
This correctly fixes the bug described: the old fixture asserted on Minor nit (non-blocking): lines 1398-1403 (untouched by this diff) still describe the old fixed cutoff — "at prune time the newest pair is ~0-1.2s old against a 6s cutoff" — which is now stale since the cutoff is derived dynamically rather than fixed at 6s. Worth a follow-up tweak so a future reader isn't misled about which cutoff is actually in effect; not a functional problem. Fix this → I was unable to execute No provider/model/route safety concerns (unrelated code path), no reuse/duplication issues, no security concerns. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The new fixture guard/cutoff computation has a couple of edge-case and intent-mismatch issues that can still yield confusing failures under clock skew or weakened timing gaps.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the snapshot::repo pruning regression test to avoid Windows CI flakes by deriving the prune cutoff from the snapshot timestamps actually recorded by git, rather than relying on a hardcoded 6-second age threshold.
Changes:
- Compute
max_agefrom the midpoint between the newest “victim” and oldest “survivor” snapshot timestamps. - Add a fixture guard asserting there is a real time gap between the “old” and “new” snapshot pairs before pruning.
File summaries
| File | Description |
|---|---|
| crates/tui/src/snapshot/repo.rs | Adjusts the pruning unit test to compute the cutoff from recorded commit timestamps to reduce timing-related flakes on slow runners. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let midpoint = victim + (survivor - victim) / 2; | ||
| let max_age = Duration::from_secs((now - midpoint).max(0) as u64); |
| assert!( | ||
| now - before[0].timestamp < 6 && now - before[2].timestamp > 6, | ||
| "fixture ages unusable for a 6s cut (newest {}s, oldest-surviving-pair {}s)", | ||
| now - before[0].timestamp, | ||
| now - before[2].timestamp | ||
| survivor - victim >= 2, | ||
| "fixture needs a real gap between the pairs (survivor {survivor}, victim {victim})" | ||
| ); |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Tightens a Windows-flaky snapshot prune test by deriving the prune age from the recorded snapshot timestamps instead of a hardcoded 6 seconds, with a guard that the two pairs are separated.
Findings
- [INFO] Prune cutoff still uses a wall-clock read before the prune call (
crates/tui/src/snapshot/repo.rs:1421)
nowis captured before the midpoint assertion, andmax_ageis computed from that value.repo.prune_older_thanlikely reads the current time internally, so any delay between the two time reads shifts the effective cutoff later by the same amount. The fixture's 8s gap leaves enough slack for this to be unlikely, but the new guard only requires a 2s gap, which would leave little margin if the fixture ever produced such a small gap.
Assessment
Test-only change is sound and addresses the root cause of the flake without touching production code. The remaining wall-clock race is minor and acceptable given the fixture timing.
Advisory review by Codewhale (codewhale review --pr 5990 --post, head 0983f67143b14b75e894c59e7d75c467c697b915). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| // three snapshots were pruned instead of two. (The old fixture guard | ||
| // could not catch it either — it checked `before[0]` and `before[2]`, | ||
| // and `before[1]` is the entry that drifts.) | ||
| let now = std::time::SystemTime::now() |
There was a problem hiding this comment.
[INFO] Prune cutoff still uses a wall-clock read before the prune call
now is captured before the midpoint assertion, and max_age is computed from that value. repo.prune_older_than likely reads the current time internally, so any delay between the two time reads shifts the effective cutoff later by the same amount. The fixture's 8s gap leaves enough slack for this to be unlikely, but the new guard only requires a 2s gap, which would leave little margin if the fixture ever produced such a small gap.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_4f9c1305-4f7d-4f50-842e-8f5686fbd630) |
|
Claude finished @Hmbown's task in 1m 38s —— View job Codewhale review — PR #5990
Scope: This sync did not change the reviewed diff — Summary: Test-only change to
Non-blocking observations (all already surfaced by prior automated reviews on this PR and still present, since the code hasn't changed since):
None of these are blocking; they're pre-existing, low-severity documentation/robustness notes on a test-only file. No reuse/duplication issues, no security concerns, no provider/model/route code touched. |
There was a problem hiding this comment.
Codewhale review
The PR replaces the hard-coded 6s prune threshold in a snapshot test with a midpoint derived from recorded timestamps, preventing slow Windows runners from pruning an extra new snapshot. The change is test-only and preserves expected surviving snapshots.
Findings
- [INFO] Fixture guard does not enforce the claimed ~4s of slack (
crates/tui/src/snapshot/repo.rs:1432)
The new assertion only requires the boundary gap to be at least 2 seconds, so the midpoint could be only 1 second from each pair if the fixture is changed. Integer-second truncation ofnowplus the small delay beforeprune_older_thancan shift the effective cutoff by up to about 1 second, so a 2-second gap would not be robust. Since the fixture sleeps 8 seconds, consider assertingsurvivor - victim >= 8to preserve the intended slack. - [INFO]
max(0)silently coerces an invalid fixture to a zero duration (crates/tui/src/snapshot/repo.rs:1436)
Ifnowis not aftermidpoint(e.g. system clock adjustment),(now - midpoint).max(0)producesDuration::ZERO, which can prune all or none of the snapshots and produce a confusing count mismatch. An explicit assertion thatnow > midpointwould fail the fixture clearly instead.
Suggestions
-
crates/tui/src/snapshot/repo.rs:1432— Require the actual 8-second sleep gap so the midpoint has the intended ~4 seconds of slack on both sides, instead of allowing a 2-second gap that leaves only 1 second of slack.survivor - victim >= 8, -
crates/tui/src/snapshot/repo.rs:1436— Fail the fixture clearly when the derived cut would be in the future rather than coercing a negative age difference to zero.assert!(now > midpoint, "fixture timestamps are in the future"); let max_age = Duration::from_secs((now - midpoint) as u64);
Assessment
The fix is a sensible test-only change that directly addresses the Windows flake. The only remaining concerns are minor fixture-guard hardening opportunities; no production behavior is affected.
Advisory review by Codewhale (codewhale review --pr 5990 --post, head 1e95daf39ee4e1e07d3a3aae95d1431f8e8a39ca). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| "fixture ages unusable for a 6s cut (newest {}s, oldest-surviving-pair {}s)", | ||
| now - before[0].timestamp, | ||
| now - before[2].timestamp | ||
| survivor - victim >= 2, |
There was a problem hiding this comment.
[INFO] Fixture guard does not enforce the claimed ~4s of slack
The new assertion only requires the boundary gap to be at least 2 seconds, so the midpoint could be only 1 second from each pair if the fixture is changed. Integer-second truncation of now plus the small delay before prune_older_than can shift the effective cutoff by up to about 1 second, so a 2-second gap would not be robust. Since the fixture sleeps 8 seconds, consider asserting survivor - victim >= 8 to preserve the intended slack.
| "fixture needs a real gap between the pairs (survivor {survivor}, victim {victim})" | ||
| ); | ||
| let midpoint = victim + (survivor - victim) / 2; | ||
| let max_age = Duration::from_secs((now - midpoint).max(0) as u64); |
There was a problem hiding this comment.
[INFO] max(0) silently coerces an invalid fixture to a zero duration
If now is not after midpoint (e.g. system clock adjustment), (now - midpoint).max(0) produces Duration::ZERO, which can prune all or none of the snapshots and produce a confusing count mismatch. An explicit assertion that now > midpoint would fail the fixture clearly instead.
| "fixture ages unusable for a 6s cut (newest {}s, oldest-surviving-pair {}s)", | ||
| now - before[0].timestamp, | ||
| now - before[2].timestamp | ||
| survivor - victim >= 2, |
There was a problem hiding this comment.
Require the actual 8-second sleep gap so the midpoint has the intended ~4 seconds of slack on both sides, instead of allowing a 2-second gap that leaves only 1 second of slack.
| survivor - victim >= 2, | |
| survivor - victim >= 8, |
| "fixture needs a real gap between the pairs (survivor {survivor}, victim {victim})" | ||
| ); | ||
| let midpoint = victim + (survivor - victim) / 2; | ||
| let max_age = Duration::from_secs((now - midpoint).max(0) as u64); |
There was a problem hiding this comment.
Fail the fixture clearly when the derived cut would be in the future rather than coercing a negative age difference to zero.
| let max_age = Duration::from_secs((now - midpoint).max(0) as u64); | |
| assert!(now > midpoint, "fixture timestamps are in the future"); | |
| let max_age = Duration::from_secs((now - midpoint) as u64); |
Make the snapshot retention fixture reject a gap shorter than its intended eight seconds and fail explicitly when its cutoff is not in the past. This clarifies fixture failures; it does not eliminate wall-clock scheduling risk or qualify Windows behavior. Document lowercase TOML row presets in both guides, distinguish the metrics cost formatter from the detailed /cost report, and resolve the two public account-entry comments to one unambiguous source-asset definition. Addresses review feedback on PRs #5990, #5973 and #5861. Source review, comment-only checks, rustfmt and git diff --check pass. No new tests; the modified snapshot regression is UNRUN under primary native-build ownership. No product runtime implementation, visual assets or changelogs changed. (cherry picked from commit 4c6f176fe7e90abb308782a22a25e31e0d341473)
No-Issue: windows-only test flake found while verifying an unrelated PR; sibling of the flake work in #5929/#5980 but not listed there.
snapshot::repo::tests::prune_older_than_keeps_the_newest_and_drops_only_the_old_tailfails intermittently onwindows-latest:Why
The fixture builds two old snapshots, sleeps 8s, then two new ones 1.1s apart, and cuts at a hardcoded 6s. That assumes
repo.snapshot()is fast —new:0is only ~1.2s plus one git subprocess older than prune time, so on a loaded Windows runner that subprocess alone carries it past the 6s line and it gets pruned with the old pair.The existing fixture guard could not catch this: it asserts on
before[0]andbefore[2], andbefore[1]is the entry that drifts.The fix
The cut is now derived from the timestamps the repo actually recorded — aim at the midpoint of the gap between the oldest survivor and the newest victim, leaving ~4s of slack in both directions instead of depending on wall-clock luck. The gap itself is asserted first, so a collapsed fixture says so plainly rather than failing later as a confusing count mismatch.
Behaviour under test is unchanged: two removed,
new:1andnew:0survive. No production code is touched.Found when it failed the windows leg of #5987 — a PR containing zero Rust files (TypeScript, CI config and
.gitignoreonly), so it cannot have been caused there.🤖 Generated with Claude Code
https://claude.ai/code/session_01D4rk4NXwyy6wmvii9Lp84P
Note
Low Risk
Only adjusts a unit test fixture; snapshot pruning behavior in production is untouched.
Overview
Fixes intermittent failures of
prune_older_than_keeps_the_newest_and_drops_only_the_old_tailon slow Windows CI, where a hardcoded 6sprune_older_thancut could prunenew:0along with the old pair whenrepo.snapshot()(git subprocess) ran long enough.The test now computes
max_agefrom listed commit timestamps: it checks the gap betweenbefore[1]andbefore[2](the boundary between new and old pairs), uses the midpoint of that gap as the cutoff, then prunes with that derived age. The old wall-clock guard onbefore[0]/before[2]is removed because it did not catch drift onbefore[1]. Expected behavior is unchanged: two removals,new:1andnew:0survive.Test-only — no production snapshot/prune logic changes.
Reviewed by Cursor Bugbot for commit 1e95daf. Bugbot is set up for automated code reviews on this repo. Configure here.