fix(server): fixed scheduling bug #4732
Conversation
This was a very stupid bug introduced in kopia#4682 Fixes kopia#4729
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a scheduling bug in the sleepable timer implementation that was incorrectly limiting long duration timers to a maximum sleep time instead of allowing them to wait for their full duration. The fix ensures timers continue looping until they reach the actual target time rather than being capped prematurely.
- Changed timer logic to continue looping until the actual target time is reached instead of capping at maxSleepTime
- Updated test cases to verify timers wait for their full specified duration
- Adjusted test timing tolerances and expectations to match the corrected behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/sleepable/sleepable_timer.go | Fixed timer logic to continue looping instead of closing channel immediately when intermediate timer triggers |
| internal/sleepable/sleepable_timer_test.go | Updated test expectations to verify full duration waits and adjusted timing tolerances |
| target := start.Add(100 * time.Millisecond) // Use a shorter duration for testing | ||
| timer := NewTimer(clock.Now, target) | ||
| select { | ||
| case <-timer.C: | ||
| elapsed := clock.Now().Sub(start) | ||
| if elapsed < testMaxSleepTime-5*time.Millisecond || elapsed > testMaxSleepTime+50*time.Millisecond { | ||
| t.Errorf("very long timer triggered at wrong time: expected ~%v, got %v", testMaxSleepTime, elapsed) | ||
| // Allow tolerance for timing variations | ||
| if elapsed < 100*time.Millisecond-20*time.Millisecond || elapsed > 100*time.Millisecond+50*time.Millisecond { | ||
| t.Errorf("very long timer triggered at wrong time: expected ~%v, got %v", 100*time.Millisecond, elapsed) |
There was a problem hiding this comment.
The hardcoded duration values (100time.Millisecond, 20time.Millisecond, 50*time.Millisecond) are duplicated from line 178 and create magic numbers. Consider extracting these into named constants or variables for better maintainability.
| target := start.Add(100 * time.Millisecond) // Use a shorter duration for testing | ||
| timer := NewTimer(clock.Now, target) | ||
| select { | ||
| case <-timer.C: | ||
| elapsed := clock.Now().Sub(start) | ||
| if elapsed < testMaxSleepTime-5*time.Millisecond || elapsed > testMaxSleepTime+50*time.Millisecond { | ||
| t.Errorf("very long timer triggered at wrong time: expected ~%v, got %v", testMaxSleepTime, elapsed) | ||
| // Allow tolerance for timing variations | ||
| if elapsed < 100*time.Millisecond-20*time.Millisecond || elapsed > 100*time.Millisecond+50*time.Millisecond { | ||
| t.Errorf("very long timer triggered at wrong time: expected ~%v, got %v", 100*time.Millisecond, elapsed) |
There was a problem hiding this comment.
Another instance of the hardcoded 100*time.Millisecond magic number that should be extracted into a variable to match the duration used in the test setup.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4732 +/- ##
==========================================
+ Coverage 75.86% 76.38% +0.51%
==========================================
Files 470 530 +60
Lines 37301 40467 +3166
==========================================
+ Hits 28299 30911 +2612
- Misses 7071 7510 +439
- Partials 1931 2046 +115 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| tolerance = 20 * time.Millisecond | ||
| } | ||
|
|
||
| if elapsed < tt.duration-tolerance || elapsed > tt.duration+tolerance { |
There was a problem hiding this comment.
is this going to lead to flaky tests?
There was a problem hiding this comment.
we can adjust it if ends up being flaky
| t.Run("very long duration", func(t *testing.T) { | ||
| start := clock.Now() | ||
| target := start.Add(24 * time.Hour) | ||
| target := start.Add(100 * time.Millisecond) // Use a shorter duration for testing |
There was a problem hiding this comment.
The subtest name no longer describes the case being tested with the new parameters.
| if elapsed < testMaxSleepTime-5*time.Millisecond || elapsed > testMaxSleepTime+50*time.Millisecond { | ||
| t.Errorf("very long timer triggered at wrong time: expected ~%v, got %v", testMaxSleepTime, elapsed) | ||
| // Allow tolerance for timing variations | ||
| if elapsed < 100*time.Millisecond-20*time.Millisecond || elapsed > 100*time.Millisecond+50*time.Millisecond { |
There was a problem hiding this comment.
It'd be good to check the tolerance in terms of the "target duration", that is, what is being added to start.Add above.
|
Thanks @jkowalski! 🏆 |
Uh oh!
There was an error while loading. Please reload this page.