Skip to content

split VortaScheduler into state and scheduling components (#2360, goal 2) - #2558

Merged
m3nu merged 1 commit into
borgbase:masterfrom
ebuzerdrmz44:refactor/scheduler-state-scheduling
Sep 14, 2026
Merged

m3nu merged 1 commit into
borgbase:masterfrom
ebuzerdrmz44:refactor/scheduler-state-scheduling

Conversation

@ebuzerdrmz44

Copy link
Copy Markdown
Contributor

Description

VortaScheduler becomes a package. This PR takes two of the three components goal 2 asks for: State (pauses, recorded outcomes, resume detection) and Scheduling (next run times, timers, network gating). Execution stays on the facade and moves in the next PR.

src/vorta/scheduler.py was 743 lines. It is now src/vorta/scheduler/

file holds
scheduler/__init__.py VortaScheduler: both signals, tr(), the Qt slots, create_backup / notify / post_backup_tasks, delegation, and re-exports
scheduler/scheduling.py SchedulerTimers: timers, lock, arm_deadline_timer and its constants, arm_profile, reload_all_timers, next_job, next_job_for_profile, pending_jobs, mark_paused, remove_job, network gating, the 15 minute reschedule timer
scheduler/state.py SchedulerState: pause / unpause / clear_pause / paused / restore_pauses, record_skip, and resume detection

Every method moves as it is, apart from the lock ownership change described below. Nothing touches the schema.

The facade stays the QObject. It keeps both signals, since schedule_page.py and jobs_page.py connect on the scheduler object, and it keeps tr() so the translation scope is still VortaScheduler. The Qt slots stay on it as well and delegate one line inward, because QDBus wants a QObject receiver for the logind connection.

The public surface is unchanged. timers, pauses, lock, wake_timer, net_status and _net_up are still reachable as properties, which keeps the test diff down to the seam. Otherwise it would be thirty renamed attribute accesses.

What I left out:

  • Execution, which goes in the next PR: create_backup, notify, post_backup_tasks, the atomic submit, the running / completed / interrupted transitions and the event_log link.
  • The crash-recovery sweep. Your 2026-07-25 call put it in this phase, but nothing writes Status.RUNNING yet, so a sweep finds nothing and its test has to fabricate a row no production path creates. I would rather it landed with the lifecycle transitions that give it something to find.
  • Tidying of the moved code. arm_profile's body and the two EventLogModel last-run queries move as they are.

Related Issue

#2360, goal 2. Cut from #2538, so that one has to merge first. I will rebase after that.

Likely also fixes #2533, see below.

Motivation and Context

The issue asks for the 743 line VortaScheduler to be split into Scheduling, Execution and State. This takes the first two.

It also carries the one behaviour change in the PR, which is lock ownership. Today set_timer_for_profile holds self.lock across its whole body. At the catch-up branch it calls lock.release(), then create_backup(...), then reacquires in a finally, and create_backup takes the same non-reentrant lock again.

After this PR:

  • SchedulerTimers.arm_profile returns the profile that has a missed run to catch up on, instead of submitting it inline. The facade submits once the lock is free.
  • The release() / acquire() pair is gone. It was only there to survive that inline call.
  • create_backup drops the lock and keeps a _submitting set of profile ids, returning early on re-entry.

That guard does real work. BorgCreateJob.prepare() reaches the keyring at borg_job.py:159, which can spin a nested event loop through KWallet's dialog, and nothing marks the profile busy until add_job runs after prepare() returns. Under the old lock, a 15 minute qt_timer tick landing in that window froze the GUI thread. Drop the lock and leave it at that, and you get a second BorgCreateJob queued for the same repo. The set turns both cases into an early return.

I hit that freeze while working on this split, and going back through the tracker afterwards it looks like #2533 is the same bug.

lock is now taken only in arm_profile. It still serialises timer arming, and it no longer excludes submission. Narrowing it further belongs with the atomic submit in the Execution PR.

How Has This Been Tested?

Unit tests,but Unit tests do not cover the deadlock, so I checked it by hand on Linux. An interval schedule with a last run far enough back to trigger catch-up starts the backup and re-arms instead of freezing. A forced failure pauses the profile, the Schedule page shows "Paused until X", and the Jobs page shows the paused row with the skip record. The pause survives a restart from SchedulerPauseModel.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the CONTRIBUTING guide.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@ebuzerdrmz44 ebuzerdrmz44 mentioned this pull request Sep 11, 2026
2 tasks
@ebuzerdrmz44

Copy link
Copy Markdown
Contributor Author

Hi @m3nu, just checking in on this one when you get a chance.

it is stacked on #2538 , and this one has the change I think fixes the scheduler lock hang ntninja traced in #2367, so it would be nice to get this in first if it looks okay to you.

@m3nu
m3nu force-pushed the refactor/scheduler-state-scheduling branch from 26dff3d to 6fad94a Compare September 14, 2026 17:06

@m3nu m3nu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the split and the lock change against current master.

  • Re-exports cover every external import site (application.py, schedule_page.py, jobs_table_model.py, tests); packages.find picks up the new subpackage, so nothing to do for packaging.
  • The lock change is right. create_backup is only ever entered from the GUI thread (timer callbacks and the catch-up path), so lock never gave it cross-thread protection — it only enabled the self-deadlock in ntninja's trace on #2367 (prepare() → pre-command processEvents()repo_changedset_timer_for_profilelock.acquire). Returning the catch-up profile out of arm_profile and guarding re-entry with _submitting closes that, and every catch-up path still re-arms via backup_finished_event or pausemark_paused.
  • The three new tests pin exactly those paths.

I rebased the branch onto master after squashing #2538 (only your commit is left, 6fad94af); CI is green on it. The property shims for _net_up / lock / pauses can go in the Execution PR once the tests are moved to the new seams.

@m3nu
m3nu merged commit fcccbbb into borgbase:master Sep 14, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UI deadlock after a missed remote backup fails with a pre-backup command

2 participants