fix(server): fixed snapshot scheduling in the event of machine goes to sleep #4682
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR replaces standard timers with a “sleepable” timer that breaks waits into capped intervals to handle machine sleep, and updates the scheduler to use this new timer.
- Introduce
sleepable.TimerwithMaxSleepTimeto cap sleep intervals. - Add comprehensive tests in
sleepable_timer_test.goto validate timer behavior (including edge cases and concurrency). - Update the scheduler (
internal/scheduler/scheduler.go) to usesleepable.NewTimerinstead oftime.NewTimer.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/sleepable/sleepable_timer.go | New sleepable timer implementation and configuration via MaxSleepTime. |
| internal/sleepable/sleepable_timer_test.go | New tests covering timer firing, stopping, concurrency, and edge cases. |
| internal/scheduler/scheduler.go | Scheduler updated to use sleepable.NewTimer and cap sleep between triggers. |
Comments suppressed due to low confidence (1)
internal/sleepable/sleepable_timer_test.go:144
- The loop
for range 10is invalid Go syntax and will not compile; use a standard indexed loop (e.g.for i := 0; i < 10; i++) or iterate over a slice.
for range 10 {
| } | ||
|
|
||
| timer = time.NewTimer(sleepTimeUntilNextTrigger) | ||
| timer = sleepable.NewTimer(s.TimeNow, targetTime) |
There was a problem hiding this comment.
Existing timer isn’t stopped before being overwritten, leaking its goroutine. Add if timer != nil { timer.Stop() } before creating the new one.
There was a problem hiding this comment.
the timer is being stopped
| start := clock.Now() | ||
| target := start.Add(tt.duration) | ||
|
|
||
| timer := NewTimer(clock.Now, target) | ||
|
|
||
| // Wait for timer to trigger | ||
| <-timer.C | ||
|
|
||
| elapsed := clock.Now().Sub(start) |
There was a problem hiding this comment.
[nitpick] These tests use the real clock and actual time.Sleep, which can be flaky; consider using a fake or simulated clock to make tests faster and more reliable.
| start := clock.Now() | |
| target := start.Add(tt.duration) | |
| timer := NewTimer(clock.Now, target) | |
| // Wait for timer to trigger | |
| <-timer.C | |
| elapsed := clock.Now().Sub(start) | |
| simulatedClock := clock.NewSimulatedClock() | |
| start := simulatedClock.Now() | |
| target := start.Add(tt.duration) | |
| timer := NewTimer(simulatedClock.Now, target) | |
| // Advance the simulated clock to trigger the timer | |
| simulatedClock.Advance(tt.duration) | |
| <-timer.C | |
| elapsed := simulatedClock.Now().Sub(start) |
There was a problem hiding this comment.
simulated clock won't work here.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4682 +/- ##
==========================================
+ Coverage 75.86% 76.39% +0.52%
==========================================
Files 470 530 +60
Lines 37301 40229 +2928
==========================================
+ Hits 28299 30732 +2433
- Misses 7071 7471 +400
- Partials 1931 2026 +95 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
julio-lopez
left a comment
There was a problem hiding this comment.
@jkowalski could you elaborate on what the root cause of the issue was?
Overall, LGTM.
Questions and minor comments inline.
Thanks.
| if elapsed > 10*time.Millisecond { | ||
| t.Errorf("zero duration timer took too long: %v", elapsed) | ||
| } |
There was a problem hiding this comment.
Can require.WithinDuration be used here?
| c.stopOnce.Do(func() { | ||
| close(c.stopChan) | ||
| }) |
See #3512 (comment) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
cb9addd to
61bdf6d
Compare
This was a very stupid bug introduced in kopia#4682
This was a very stupid bug introduced in kopia#4682 Fixes kopia#4729
Uh oh!
There was an error while loading. Please reload this page.