Skip to content

add JobModel: persistent scheduler job store (#2360, phase 1) - #2530

Open
ebuzerdrmz44 wants to merge 6 commits into
borgbase:masterfrom
ebuzerdrmz44:refactor/scheduler-job-model
Open

add JobModel: persistent scheduler job store (#2360, phase 1)#2530
ebuzerdrmz44 wants to merge 6 commits into
borgbase:masterfrom
ebuzerdrmz44:refactor/scheduler-job-model

Conversation

@ebuzerdrmz44

@ebuzerdrmz44 ebuzerdrmz44 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Description

First PR of the scheduler refactor (#2360), split into small stacked PRs. The full breakdown is on the issue.

This adds JobModel together with its first consumer, so the table arrives with a real write path rather than as a bare schema.

JobModel records intent: what the scheduler meant to do, including the runs that never reach borg and so never produce a log line. EventLogModel stays the record of what executed, linked through the event_log FK. profile and repo_url are plain strings mirroring EventLogModel, with profile_name stored next to the id so the history still names the profile after someone deletes it.

No migration. connection.py builds tables with create_tables(), which is safe by default, so the table shows up on fresh and existing DBs alike.

What writes to it

Three call sites go through VortaScheduler._record_skip:

  • borg's prepare step fails in create_backup
  • the repo is busy with another job
  • the network is down for a catch-up run in set_timer_for_profile

A real error records status='failed'. The expected cases (no WiFi, metered connection, busy repo) record status='skipped'.

The catch-up site passes the missed occurrence as scheduled_at and writes through get_or_create. A network that stays down overnight then leaves one row per missed run instead of one per timer tick.

init_db deletes job rows older than six months, next to the existing EventLogModel sweep.

db_lock moved from vorta.borg.borg_job to vorta/store/models.py, which is why borg_job.py turns up in the diff.

Not written yet

event_log and the scheduled, running and interrupted statuses have no producer in this PR. They arrive with the run-lifecycle and crash-recovery phases in the #2360 breakdown, so there is nothing to go hunting for here.

@ebuzerdrmz44

Copy link
Copy Markdown
Contributor Author

Reworked this from the bare model into something with an actual use.

The three schema questions are decisions now. Profile and repo are plain strings like EventLogModel, so a job's history sticks around even after its profile or repo gets deleted. I kept scheduled and interrupted in the status enum but nothing writes them yet.

I also folded in the first consumer, skip-reason recording, so there's a real path through the table instead of an empty schema sitting there. The skip write goes through db_lock the same way the EventLogModel writes do.

Ready for another look whenever you get a chance.

@ebuzerdrmz44
ebuzerdrmz44 marked this pull request as ready for review July 24, 2026 22:23
@ebuzerdrmz44

ebuzerdrmz44 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

@m3nu Since this is the first of a stack, here's the full shape of #2360 so it's clearer what this PR is (and isn't) trying to do. Roughly 4 phases,10-11 small PRs ( I broke my original roadmap in #2360 to make all of it easy to review 😆 ):

Phase A : Persistence (the jobs store)

  • PR1 (this PR): the JobModel table + the first skip-reason consumer + record skip reasons at the remaining skip points (repo busy, network down, mount)
  • PR2: persist pause/resume state so it survives a restart

Phase B : Split VortaScheduler into State / Scheduling / Execution (PR4–6). The Execution one wires the job→result link and makes post-backup tasks run in order.

Phase C : Reliability (PR7–8): the QTimer 24.8-day overflow, and sleep/resume detection on non-logind systems.

Phase D : Jobs View UI (PR9–11): the Jobs page, filters, cancel/re-queue. I'm holding this whole phase until you weigh in on three open questions on the issue .

This PR is deliberately the narrow shape-approval slice: get the table right, prove it works with one consumer, then stack the rest on top once you're happy with it. Everything downstream depends on this one merging first.

@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.

Thanks for folding in the first consumer — a table with a real write path is much easier to judge than a bare schema, and the phased breakdown on #2360 is genuinely helpful.

The overall shape is defensible: JobModel records intent (including the things that never produce a log line), EventLogModel stays the record of what executed, linked through the event_log FK. Plain strings for profile/repo_url mirror EventLogModel exactly. Your no-migration claim checks out — init_db calls DB.create_tables([...]) unconditionally with safe=True, so the table appears on fresh and existing DBs alike.

Requesting changes on three points, then this is good to build on.

Required

1. Failures are being recorded as skips. In create_backup, the JobModel.create(...) sits in the outer block covering both branches of the level == 'error' check. So a genuine failure — "Conditions for backup not met", repo unreachable, source missing — gets written with status='skipped' and the error text in skip_reason. That defeats the table's stated purpose of capturing intent, and it'll poison the Phase D Jobs View from day one. Use status='failed' when level == 'error'; keep skipped for the expected WiFi/metered/busy cases.

2. No retention policy → unbounded growth. init_db purges EventLogModel rows older than six months. JobModel gets no equivalent, so on a machine with an aggressive schedule and a flaky network the table only ever grows. Please add the purge alongside the existing one in this PR — retrofitting it after users have accumulated rows is strictly worse.

3. Extract the triplicated write. Three near-identical with db_lock: JobModel.create(...) blocks. Collapse into a _record_skip(self, profile, trigger, reason, status='skipped') helper on VortaScheduler. With ~10 more PRs stacked on this, it will not stay at three.

Cheap, should fix here

4. Move db_lock out of vorta.borg.borg_job. Importing a DB-serialization lock from the borg job module into the scheduler is a layering inversion — it belongs in vorta/store/. Every PR in the stack will want it, so move it now, while there's exactly one new importer.

5. Constrain the string enums. status, job_type and trigger are free-form CharFields written with bare literals. Module-level constants (or a small JobStatus class) in models.py will stop the eight downstream PRs drifting on 'skipped' vs 'skip'.

6. Missing test for the third write site. test_scheduler.py covers the prepare-failure and repo-busy skips, but not the catchup/network-down write in set_timer_for_profile.

Worth settling before the stack builds on this schema

7. profile stores the profile id as a string. Consistent with EventLogModel, so I'm not objecting — but the rationale in your comment ("a job's history sticks around even after its profile gets deleted") isn't actually achieved: what survives is a dangling numeric id with no name. If the Phase D Jobs View is meant to render history for deleted profiles, store profile_name alongside it.

8. Dead columns. scheduled_at, event_log and the scheduled/running/interrupted statuses are never written by production code — only by the test. Fine as a phase-1 slice given the roadmap, but say so explicitly in the description so a reviewer doesn't go hunting for the writer.

9. No index. The Jobs View will filter on profile/status/created_at. Adding it now avoids a migration later.

On your original three questions: agreed on plain strings (2), and yes, a startup sweep marking leftover running jobs as interrupted is enough for crash recovery (3). For (1), your instinct is right — keep next-run derivation in set_timer_for_profile as the single source of truth, and persist only skipped/interrupted/completed. Don't materialise scheduled rows.

Minor: this PR tracks #2360, not #2361 — worth keeping the cross-links straight since the two refactors are running in parallel.

@ebuzerdrmz44

Copy link
Copy Markdown
Contributor Author

want to flag this before you look again: I moved db_lock into store/models.py like you asked, but ended up not using it in the scheduler, so borg_job is still its only importer. The lock is held across process_result(), which loops every archive on a borg list . I think grabbing it from the scheduler would be the first time we take it on the GUI thread, and a large repo sync would freeze the UI. Recording is best-effort instead, so a lost row can't affect scheduling.

Narrowing the lock in borg_job.run to just the save() call would also fix it properly. Which would you rather?

@ebuzerdrmz44

Copy link
Copy Markdown
Contributor Author

@m3nu All nine done , ready for another look.

I also deduped the catch-up skips, since a down network wrote a row per tick. And I rewrote the description.

@ebuzerdrmz44

Copy link
Copy Markdown
Contributor Author

@m3nu bumping this one since the whole #2360 stack sits behind it. All nine points are in as of 43662e8.

Still open from my side: the db_lock question. Recording is best-effort right now so a lost row can't affect scheduling. If you'd rather I narrow the lock in borg_job.run to just the save() calls, that's a small standalone PR and I'll open it.

#2532 (pause persistence) is the phase 1 sibling, also green and independent of this one, if it's easier to look at them together.

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.

2 participants