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

Copy link
Copy Markdown
Contributor

Description

First PR of the scheduler refactor (#2360), which I'm splitting into small stacked PRs (full breakdown is on the issue). This one just adds JobModel; nothing reads or writes it yet.

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

The idea: JobModel tracks a job's lifecycle and intent (scheduled, running, done, skipped, interrupted). When a job actually runs it links to the matching EventLogModel row through event_log, so I'm not duplicating start/end/returncode across two tables. EventLogModel stays the record of what executed, and JobModel adds the intent plus the things that never produce a log line, like a job that got skipped.

A few schema questions before I build on it. The rest of the plan doesn't depend on these, but this table's shape does:

  1. Store scheduled jobs as rows, or keep deriving the next run on reload like today? The issue wants pending jobs to survive restarts, but storing them risks a second source of truth against the derivation in set_timer_for_profile. I lean toward persisting skipped and interrupted jobs while leaving next-run derivation as is.
  2. FK or plain strings for profile/repo? EventLogModel uses strings, so its rows survive a profile being deleted. Same for the jobs history, or is a FK fine here?
  3. For crash recovery, is a startup sweep that marks leftover running jobs as interrupted enough, or do you want more than that?

@m3nu If you'd rather this not merge as a bare model, I can fold in the first consumer (skip-reason recording) before then.

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

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