add SchedulerPauseModel: persist pauses across restarts (#2360, phase 1) - #2532
add SchedulerPauseModel: persist pauses across restarts (#2360, phase 1)#2532ebuzerdrmz44 wants to merge 3 commits into
Conversation
m3nu
left a comment
There was a problem hiding this comment.
Good shape, and the three calls you asked me to check all hold up.
Own table rather than a column on BackupProfileModel is right — profile_export.py:56 serialises with model_to_dict(profile) at the default recurse=True, so a paused_until column would travel into every exported profile and land as a stale pause on whoever imports it. Own table rather than a JobModel row is right too: a live pause isn't an outcome, and the store holds outcomes. And shipping the label here rather than in Phase D is the correct call — paused() had no production caller before this, so persisting the state alone would have bought nothing observable.
Two details worth calling out: list(SchedulerPauseModel.select()) materialises the cursor before _clear_pause deletes inside the loop, and _mark_paused writing a timers entry with no 'qtt' key is safe because next_job's is_scheduled short-circuits on type first — same as the existing TOO_FAR_AHEAD and NO_PREVIOUS_BACKUP entries.
One real bug, then smaller things.
Required
1. profile_delete_action uses the wrong cleanup, and can start a backup on the profile it is about to delete.
self.app.scheduler.unpause(to_delete_id) # Drop a pause the deleted id could outlive
to_delete.delete_instance(recursive=True)unpause() ends with set_timer_for_profile(profile_id) (scheduler.py:188), and that method's catch-up branch calls create_backup(profile_id) when schedule_make_up_missed is set and the next occurrence has passed. The profile still exists at that point. So deleting a paused profile that has catch-up enabled and an overdue schedule kicks off a real borg create against it, and then deletes the profile, its sources and its repo link out from under the running job.
That combination isn't exotic: a pause is set on every skipped run, and an overdue next occurrence is usually why the profile is paused. This is a user cleaning up a profile that keeps failing.
_clear_pause(to_delete_id) is what you want — it stops the timer, drops both the self.pauses and timers entries, deletes the row, and does not reschedule. Please make it public, since it now has a caller outside the class. Keep the ordering you have; delete_instance(recursive=True) would remove the row anyway, it's the in-memory entry that matters for SQLite id reuse.
test_deleting_a_paused_profile_clears_the_pause stays valid with that fix, but right now it passes for the wrong reason — the Default fixture never reaches the catch-up branch. Please extend it: enable schedule_make_up_missed, put an overdue EventLogModel create row behind it, and assert BorgCreateJob.prepare is never called.
Cheap, should fix here
2. Match _record_skip's best-effort handling. _set_pause runs SchedulerPauseModel.replace(...).execute() on the GUI thread with no try. #2530 just landed the symmetric write and wraps it in except pw.PeeweeException, precisely because a worker thread can be holding the SQLite write lock (WAL, and no timeout pragma is set, so pysqlite's 5s default applies). Here the exception escapes pause() into create_backup() and then into a Qt slot with no handler. A pause that fails to persist should degrade to the old in-memory behaviour, not take the scheduler down.
3. Record the pause as a skip. You flagged this as belonging to whichever of the two landed second. #2530 is in, so _record_skip is on master now — add the fourth call site here.
Worth settling
4. The 2**31 - 1 clamp in _set_pause belongs to Phase C's overflow PR. Defensible here, since a paused_until read back from the DB after a clock change is exactly the untrusted input that overflows. But note it in the description so the overflow PR doesn't collide, or pull it out and let that PR handle both sites at once.
5. Should a successful manual backup unpause? Yes. unpause only fires from notify() on a scheduled run, and manual backups never reach scheduler.notify. I'd rather not widen this PR for it though — set_timer_for_profile gets restructured in the Phase B split, so do it there.
6. reload_all_timers clearing the label while a remote repo's network is down: agreed, leave it, Phase B covers it.
7. Noting, not asking: a pause on a profile switched to manual lingers in self.pauses and in the table, which test_paused_manual_profile_reports_unscheduled documents. Pauses are capped at 60 min so it self-corrects, and I'm fine with that.
Rebase
#2530 landed, so this is conflicting now. All of it is adjacent-insert and trivial: the vorta.store.models import in scheduler.py, the model definition before class SchemaVersion in models.py, the create_tables list in connection.py, and the import block in test_scheduler.py. Your test additions sit at a different offset from #2530's, so those two shouldn't collide.
This one unblocks Phase B, so I'll turn the next round around quickly. #2537 is next in my queue — it only overlaps you in VortaScheduler.__init__, so whichever of the two goes second picks up a small manual rebase there.
94dc0f5 to
8e42dac
Compare
|
All in as of 8e42dac, rebased onto master. |
m3nu
left a comment
There was a problem hiding this comment.
Points 1 and 2 are properly done, and 2 went further than I asked in the right direction.
clear_pause is the correct fix and all four internal callers moved over cleanly. profile_delete_action no longer reschedules a profile it's about to delete. Wrapping clear_pause's DELETE as well as _set_pause's REPLACE was the right instinct — both are GUI-thread writes and neither should be able to take the scheduler down.
On point 3 — I was wrong to ask for it here. Please take it back out.
The notify() site is genuinely the fourth pause() call site, so you implemented what I asked. But look at what the row means. Every JobModel row #2530 writes is for a run that never reached borg and therefore has no EventLogModel row — that's the whole "records intent, including the runs that never produce a log line" split. A backup that ran and failed has already written an EventLogModel row with its returncode by the time notify() fires. So this is the first JobModel row that duplicates an EventLogModel row, and it's written with event_log = None — the Phase D view would show two unlinked records of one failure.
Linking it here isn't the fix either: log_entry is local to borg_job.run and never reaches result, so exposing it is plumbing that belongs to PR6, which you already scoped as "wires the intent to result link". So drop the _record_skip call and the msg['trigger'] = trigger line, and let PR6 own records for runs that actually executed.
Nothing is lost by dropping it. The pause is already fully represented by SchedulerPauseModel plus the PAUSED status this PR adds, which was the point of the PR. Keep test_failed_backup_records_the_pause in your back pocket — it's most of the test you'll want in PR6.
One more on the regression test
test_deleting_a_paused_profile_clears_the_pause is network-dependent as written. You give the new profile the Default fixture's repo, whose url is i0fi93@i593.repo.borgbase.com:repo, so is_remote_repo() is true, needs_network is true, and the catch-up branch is gated on self._net_up. On a runner with network it exercises the bug properly; on a developer machine that's offline it passes whether or not the fix is present. Set qapp.scheduler._net_up = True before the click. Worth the one line for a test guarding a start-a-backup-on-a-profile-being-deleted bug.
Everything else
Items 4 to 7 from last round need no action — the description covers the clamp, and I'm happy deferring the manual-unpause and reload_all_timers label items to Phase B.
Drop the notify() hunk, pin the network in that test, and I'll approve. Heads up that I just approved #2537, so this will pick up a small manual rebase in VortaScheduler.__init__ where the two of you overlap.
…he pause after a failed backup
…work in the profile deletion test
8e42dac to
cb19e6c
Compare
|
Both in as of Also I kept the test for PR6. Also, following your call on not materialising scheduled rows, pending jobs aren't in the table at all, so PR9 has to read them off the in-memory timers and merge those with |
Description
When a scheduled backup can't run (repo busy, network gone), the scheduler pauses that profile for up to an hour instead of retrying in a loop. Two problems with that. The schedule page reads "None scheduled" the whole time, so the user has no idea why backups stopped or when they resume. And the pause only lives in memory, so restarting Vorta throws the backoff away and goes straight back at whatever was broken. #2360 lists the second one under the jobs store.
What's in it
SchedulerPauseModel: the profile FK as primary key pluspaused_until, one row per profile._set_pause/_clear_pause/_restore_pauses(), the last dropping anything that expired while Vorta was closedScheduleStatusType.PAUSEDand a "Paused until 14:32" label.Independent of #2530, so it sits on master and the two can land in either order. No migration.
Completes Phase A of the renewed plan.
Calls I'd like you to check
BackupProfileModel.profile_export.pyserialises withmodel_to_dict, so a column would export a stale pause to whoever imports the profile.JobModelrow. You asked for only skipped/interrupted/completed in the store, and a live pause is none of those. Cheap to change now, awkward after a release.paused()had no production caller before this, so persisting the state alone would have bought nothing. Lifts back out if you'd rather it waited for the Jobs view.Known gaps
_record_skipdoesn't cover, and it needs both branches in one tree, so it goes in whichever lands second.unpause()only fires from a successful scheduled backup, and manual backups never connectscheduler.notify. Want a successful manual run to unpause, or an action next to the label?reload_all_timersclears the label back to "None scheduled" even though the pause is still in effect. It comes back on the next reload, andset_timer_for_profilegets restructured in the Phase B split anyway, so I left it.