test: stop rewinding monotonic instants in tmux/git tests - #354
Merged
Merged
Conversation
`Instant - Duration` panics where `Instant` is an unsigned tick counter (Windows QPC) and the rewind exceeds system uptime. The tmux stale tests rewound `Instant::now()` by an hour and the git quarantine test rewound a probe instant by a second, so a full `cargo test --bin clawhip` on a freshly booted Windows host panicked in unrelated assertions. Linux stores `Instant` as a signed `Timespec` and does not panic, which is why CI never caught it (the Windows job only runs `issue_317` and `config_backup_cleanup`). Advance the observation instant instead: pin `last_change` and pass `last_change + Duration::from_secs(3600)` as the current instant to `should_emit_stale`. This is semantically identical -- the function only compares the two instants -- and also removes the incidental second `Instant::now()` call. The git probe now uses an explicit `checked_sub` with a stated precondition. Clears all four `clippy::unchecked_duration_subtraction` sites. Fixes #353
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Fixes #353.
What
Four tests rewound a monotonic clock (
Instant::now() - Duration::from_secs(3600)in the tmux stale tests,probe_at - Duration::from_secs(1)in the git quarantine test). They now advance the observation instant instead of rewinding "now", and the git probe uses an explicitchecked_subwith a stated precondition.Why
Instant - Durationischecked_sub(..).expect("overflow when subtracting duration from instant"), and overflow is platform-dependent:Instantas a signedTimespec, so the rewind is representable — verified on a host with 3_820_556s uptime, rewinding byuptime + 3600sdoes not panic.Instantas an unsigned QPC tick count, so the subtraction panics whenever the rewind exceeds system uptime.Result: a Windows contributor running the full
cargo test --bin clawhipon a machine booted less than an hour ago panics instale_minutes_zero_disables_stale_detection,stale_minutes_nonzero_still_emits, andpane_dead_suppresses_stale_alertfor reasons unrelated to what those tests assert. Linux CI never caught it, and the Windows CI job only runsissue_317andconfig_backup_cleanup.should_emit_staleonly compareslast_changeagainst the supplied instant, so pinninglast_changeand passinglast_change + 3600sis semantically identical while being uptime-independent. It also removes an incidental secondInstant::now()call per test.Verification
cargo test --bin clawhip→ 1101 passed, 0 failedcargo fmt --check→ cleancargo clippy --all-targets -- -D warnings→ clean;clippy::unchecked_duration_subtractionnow reports 0 sites (was 4)Base
dev@7bad9d24df18984a2e68612862246ce7263eae19.—
[repo owner's gaebal-gajae (clawdbot) 🦞]