Skip to content

fix(test): derive the prune cut from recorded timestamps, not a fixed 6s #5990

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Hmbown wants to merge 2 commits into from
+19 −9
Closed

fix(test): derive the prune cut from recorded timestamps, not a fixed 6s #5990

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0983f67
fix(test): derive the prune cut from recorded timestamps, not a fixed 6s
Sep 7, 2026
1e95daf
Merge branch 'main' into fix/snapshot-prune-windows-flake
Hmbown Sep 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions crates/tui/src/snapshot/repo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1411,22 +1411,32 @@ mod tests {
}
let before = repo.list(usize::MAX).unwrap();
assert_eq!(before.len(), 4);
// Guard the fixture itself: if load skewed the timestamps so the cut
// would not fall between the pairs, say so instead of failing later
// with a confusing count mismatch.
// Derive the cut from the timestamps actually recorded rather than a
// fixed 6s. A fixed cut 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 pushed it past 6s and
// 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()

@codewhale-agent codewhale-agent Bot Sep 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

.duration_since(std::time::UNIX_EPOCH)
.unwrap()
.as_secs() as i64;
// Newest-first: [new:1, new:0, old:1, old:0]. The cut must land
// strictly between the pairs, so aim at the midpoint of the 8s gap —
// that leaves ~4s of slack against clock drift and a slow runner in
// both directions.
let survivor = before[1].timestamp;
let victim = before[2].timestamp;
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,

@codewhale-agent codewhale-agent Bot Sep 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@codewhale-agent codewhale-agent Bot Sep 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
survivor - victim >= 2,
survivor - victim >= 8,

"fixture needs a real gap between the pairs (survivor {survivor}, victim {victim})"
);
Comment on lines 1431 to 1434
let midpoint = victim + (survivor - victim) / 2;
let max_age = Duration::from_secs((now - midpoint).max(0) as u64);
Comment on lines +1435 to +1436

@codewhale-agent codewhale-agent Bot Sep 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@codewhale-agent codewhale-agent Bot Sep 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fail the fixture clearly when the derived cut would be in the future rather than coercing a negative age difference to zero.

Suggested change
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);


// Cut 6s back: the two old snapshots drop, the two new ones survive.
let removed = repo.prune_older_than(Duration::from_secs(6)).unwrap();
// The two old snapshots drop, the two new ones survive.
let removed = repo.prune_older_than(max_age).unwrap();
assert_eq!(removed, 2, "only the old tail should be removed");

let remaining = repo.list(usize::MAX).unwrap();
Expand Down
Loading