detect resume from sleep without logind (#2360, phase 3) - #2537
Conversation
|
@m3nu Map of where this sits, since #2360 is now four PRs wide and this one jumps ahead of Phase B. Phase C does not depend on Phase A or B, so it does not have to wait in line. What does gate everything else: fork PRs cannot be stacked, so PR4 onwards each have to branch off a merged master.
Still open from earlier rounds, no rush on any of them:
One note from re-reading the issue against what is built, not needing a reply unless you disagree: Goal 1's "Pending/scheduled jobs (survive restarts)" and your "don't materialise scheduled rows" are compatible, as I read it. |
…ng recovers on systems without logind
f03937a to
99c5a61
Compare
m3nu
left a comment
There was a problem hiding this comment.
Rebase looks clean — re-anchored onto #2530 with the content unchanged, CI green. Reviewed against the merged master.
The core reasoning is right, and it's the part I checked hardest. CLOCK_MONOTONIC doesn't advance across suspend, so a monotonic detector would see one tick's worth of gap after an 8-hour sleep and never fire; wall clock is the only signal available without a Linux-specific CLOCK_BOOTTIME branch plus a macOS fallback. The false-positive sources you accept — NTP step, DST shift, an event loop blocked past the threshold — each cost exactly one redundant reload_all_timers(), which the periodic poll already does unconditionally. That's the correct trade.
Also good: _handle_resume resetting _last_wake_check so a logind resume doesn't leave the detector primed to fire again on the next tick, and starting wake_timer unconditionally rather than only when logind is missing. Keep it unconditional — PrepareForSleep doesn't fire for a hypervisor pause and doesn't exist on macOS at all, so the timer is a genuine backstop rather than a fallback. loginSuspendNotify having tests for the first time is a nice bonus.
Two things before I approve, both small.
1. Make the tick 5 minutes, threshold 10. Answering your question 1: the cost isn't the two datetime subtractions, it's that a 60s timer wakes the process 1440 times a day and defeats tickless idle on Linux and App Nap on macOS. What the latency buys is how soon after resume a missed backup gets picked up — and for a backup app, 5 minutes versus 1 minute is not a difference anyone notices, while 15 minutes versus 5 is. So take the 3x improvement at a fifth of the wakeups. Your existing test gaps (2 hours and 1 minute) still land on the right side of a 10-minute threshold, so this should be constants-only.
2. Stop wake_timer for every scheduler the module builds, not just these three. You spotted the hazard — the comment about qapp keeping every scheduler alive is exactly right — but the fix is applied per-test, and the other tests in this file that construct a VortaScheduler still leak one with a 2020 baseline. Once clockmock is torn down, such a timer computes a six-year gap and calls reload_all_timers() on a stale scheduler in the middle of an unrelated test. The pre-existing qt_timer has the same leak and has never bitten because 15 minutes outlives the suite; at 60s (or even 5 min) that stops being true. An autouse fixture in test_scheduler.py that stops wake_timer on construction would cover it, and Phase B is about to add a lot more scheduler tests to this file.
Answering your question 2: yes, wake_timer, checkForResume and _handle_resume go to State, not Scheduling — none of them computes a next run, they observe machine state and poke the scheduler. Carry them there in PR4.
Tiny nit, take or leave: the new log line reads "watching for clock gaps instead", but the detector runs whether or not logind is present. "as well" would be more accurate.
Push those two and I'll approve. Note that #2532 and this one overlap in VortaScheduler.__init__ — you're ahead of it right now, so if this lands first, #2532 picks up the manual rebase there rather than the other way round.
…uler the tests build
m3nu
left a comment
There was a problem hiding this comment.
All three in, approving.
5 min / 10 min threshold, and the fixture. The fixture is a better shape than what I suggested — patching __init__ catches every scheduler the module builds rather than relying on each test to remember, and stopping qapp.scheduler's timer explicitly closes the one instance the patch can't reach.
Take-or-leave, don't hold the PR for it: the fixture only applies to test_scheduler.py, so qapp.scheduler's timer is still live during earlier files — test_schedule.py runs first alphabetically and has its own clockmock, which is the same 2020-baseline setup. At a 5-minute interval the odds of a tick landing inside that file are low enough that I'm not worried, but tests/unit/conftest.py is the natural home for it whenever you next touch that file.
Description
Phase C of #2360, first of two. Fixes one of the four reliability defects the issue lists: "DBus sleep/resume detection fails silently."
At startup Vorta checks whether
org.freedesktop.login1is registered. If it is not, it logs a warning and gives up permanently. So without logind there has never been any resume detection, and after a wake every profile's timer stays stale until the 15-minute poll comes around.This adds a detector that does not need logind, making the DBus signal a fast path rather than the only path.
What's in it
wake_timerticks every 60s and only compares two timestamps. QTimer does not fire while suspended, so a gap far larger than one tick means the machine was frozen and the schedules are stale.time.monotonic().man 2 clock_gettime:CLOCK_BOOTTIMEis "identical to CLOCK_MONOTONIC, except that it also includes any time that the system is suspended". A monotonic detector would see a 60s gap after an 8-hour suspend and never fire. The cost is that an NTP step or DST shift can trip it, which buys one redundantreload_all_timers(), exactly what the poll already does unconditionally.loginSuspendNotifyandcheckForResumenow share_handle_resume(), previously inline in the former. It resets the detector's baseline so a logind resume does not also trip the timer on the next tick.warningtoinfo, since it is no longer a failure.clockmockfixture and the realtimeoutsignal.loginSuspendNotifyhad no coverage before.Independent of #2530 and #2532: branches off master.
Need help in these decisions
1. Is 60s the right tick? Each tick is two
datetimesubtractions, so the cost is the wakeup, not compute. 5 minutes still beats today by 3x, andCLOCK_BOOTTIMEwould allow a much lazier check at the price of a Linux-only branch plus a macOS fallback. One-line change either way.2. Where does this land after Phase B? I expect
wake_timer,checkForResumeand_handle_resumeto go to the State component rather than Scheduling, since none of it computes a next run. Confirm and I will carry them there in PR4.