Repository navigation
fix(scheduler): pause windows during disk holds, unlimited label, resume across restart (WI-2033) - #249
Conversation
|
Review of e491a71: 1 blocker (a restart undoes the overlap fix), plus 2 should-fix items. GitHub Scope. This branch is #245's head (07e866b, already reviewed) plus one commit, e491a71. Landing #249 supersedes #245. There are no Checks run locally:
What e491a71 gets right
Blocker
Should fix
Nit
|
…ependent Address the #249 review: a restart undid the disk/schedule overlap, a timed pause expiring lifted every hold, and one of two overlapping windows closing re-paused a manual resume. Blocker — restart dropped the schedule hold during a disk hold. Holds were persisted as one ranked owner string, so a disk hold overlapping a window restored only one of them; the guard lifting its own hold then resumed downloads through the rest of the window. Each hold is now stored as its own flag, and the window bookkeeping as its own keys; the owner string is kept only to read records written before this change. Should-fix — a timed pause expiring lifted every hold. `pause_for`'s timer called `resume_all`, which cleared the user, disk and schedule holds. It now lifts only the user's, so a window that opened underneath the timed pause takes effect and a disk hold stays. An indefinite `Pause All` still takes over the windows it covers, as before. Should-fix — overlapping windows re-paused on any change. The occurrence key was the joined set of open rules, so one of two windows closing read as a new window. A decision now carries the occurrences individually, and only an occurrence not seen before is a new window. A manual resume records the occurrences it overrode, so a settings save or the next tick cannot restore them; the next window still pauses. The `saved` flag on `apply_schedule` is gone: the occurrence bookkeeping already distinguishes the same window from a new one, and keeping it meant a first save never admitted its own window. Tests: a restart during a disk hold keeps the overlapping window (reproduced against the previous commit before fixing), a timed pause expiring pauses into an open window and leaves a disk hold, one of two overlapping windows closing does not re-pause (queue-manager and API level), a second pause cancels the first timer's auto-resume, and the evaluator reports per-rule occurrences. Refs WI-2033
|
Re-review of 213f732: the three reported items are fixed, but 2 new blockers: a regression in the restart handling, and CI is red against current Checks run locally on the PR head alone:
GitHub, which builds the merge ref with The three items from my last review: fixed
Dropping the Blockers
Nit
|
…1721) Add a download scheduler: rules name weekdays and a window and either pause downloads, cap the speed or take servers out of rotation. Windows may cross midnight and belong to the day they start on. - `nzb-core::schedule` evaluates rules as a pure function of wall-clock time, so window edges and midnight are tested without a clock or a queue. - `nzb-web` applies the decision every minute boundary and immediately on a config save. Pause ownership is tracked and persisted, so the schedule only lifts a pause it applied; a user pause, a timed pause or a disk-space hold survives. Speed caps and server exclusions are overlays: the configured limit and `enabled` flags are never rewritten. - `GET/PUT /api/config/schedule` validates every rule before saving, assigns ids and returns what is active now. - Settings gains a Scheduler tab (enable toggle, rule list, day/window/action editors, live status); the queue shows "paused · schedule" and the reason survives a reload. - Config example and CONFIGURATION.md document the section, including TZ. Tests: 13 evaluator unit tests, monitor and queue-manager ownership tests, 8 API integration tests, settings and queue component specs, and Playwright journeys for add/save/reload/remove and validation.
The scheduler tests asserted the stored schedule cap and the configured limit, but never the value the downloader actually reads. A cap that was recorded and never applied would have passed. Assert `get_speed_limit()` through a cap being applied, lifted when the window ends, and overridden by an explicit 0 (unlimited). Also cover a disabled schedule releasing a cap and a server exclusion it had applied, since `apply_schedule` substitutes an idle decision in that path. Refs WI-1721
Two behaviour blockers from the review of #245. A manual Resume inside a pause window was undone within a minute, because apply_schedule was level-triggered and re-paused on every tick. The pause is now edge-triggered: the window pauses when it opens and leaves a manual resume alone until the next window. Ownership still decides who may lift it, and a window restored after a restart still lifts its own pause when it ends. A speed_limit_bps = 0 rule promised unlimited but evaluate() emitted None, which apply_effective_speed_limit reads as "no scheduled cap, use the configured general.speed_limit_bps". It now emits Some(0), so an unlimited window overrides both the configured limit and any lower cap. Tests: evaluator distinguishes unlimited from no-cap-rule; queue manager covers manual resume holding inside a window, a window opening while the user is paused, restart inside a window, and disabling the schedule; API tests cover the same two bugs end to end. Docs describe the edge-triggered pause and the 0-cap precedence. Refs WI-1721
…llowed A pause window that opened while the disk-space guard held the queue never took effect: the pause had a single owner, so the guard lifting its hold resumed downloads straight through the window. A manual resume inside a window was also lost on restart, because the queue could not tell "the same window, ticked again" from "a newly opened window". - Replace the single pause owner with additive holds (user, disk, schedule). The queue is paused while any hold is set; each lifts only its own, so a window that opens during a disk hold keeps downloads paused once the hold lifts, and a window ending never lifts a user pause or a disk hold. - Give the evaluator's decision an occurrence key (`rule id@window start date` for each open pause rule, sorted) and persist it. The schedule pauses only on a new occurrence, so a restart inside an open window does not re-pause what the user resumed, while the next window pauses again. - Distinguish a settings save from a monitor tick when applying a decision, so saving inside a window adopts the open window instead of re-opening it. - Report `paused` from the API whenever an open window is keeping the queue paused, including when a disk hold overlaps it. - Render an unlimited window (`0`) as "unlimited" in settings instead of "cap 0 B/s", which read as a cap that stops every download. - Keep cancelling a pending timed resume on every pause, as before: a second pause must not inherit the previous one's deadline. Adds evaluator, queue-manager and API tests for the overlap, the restart and the occurrence key, and a frontend test for the unlimited cap label. Refs WI-2033
…ependent Address the #249 review: a restart undid the disk/schedule overlap, a timed pause expiring lifted every hold, and one of two overlapping windows closing re-paused a manual resume. Blocker — restart dropped the schedule hold during a disk hold. Holds were persisted as one ranked owner string, so a disk hold overlapping a window restored only one of them; the guard lifting its own hold then resumed downloads through the rest of the window. Each hold is now stored as its own flag, and the window bookkeeping as its own keys; the owner string is kept only to read records written before this change. Should-fix — a timed pause expiring lifted every hold. `pause_for`'s timer called `resume_all`, which cleared the user, disk and schedule holds. It now lifts only the user's, so a window that opened underneath the timed pause takes effect and a disk hold stays. An indefinite `Pause All` still takes over the windows it covers, as before. Should-fix — overlapping windows re-paused on any change. The occurrence key was the joined set of open rules, so one of two windows closing read as a new window. A decision now carries the occurrences individually, and only an occurrence not seen before is a new window. A manual resume records the occurrences it overrode, so a settings save or the next tick cannot restore them; the next window still pauses. The `saved` flag on `apply_schedule` is gone: the occurrence bookkeeping already distinguishes the same window from a new one, and keeping it meant a first save never admitted its own window. Tests: a restart during a disk hold keeps the overlapping window (reproduced against the previous commit before fixing), a timed pause expiring pauses into an open window and leaves a disk hold, one of two overlapping windows closing does not re-pause (queue-manager and API level), a second pause cancels the first timer's auto-resume, and the evaluator reports per-rule occurrences. Refs WI-2033
… toast service Two re-review blockers on #249. A manual Resume inside a window did not survive a real restart: the window lists were only loaded when `globally_paused` was true, so after a resume the first tick read the still-open occurrence as newly opened and paused again. The lists (`open_windows`, `triggered_windows`, `resumed_windows`, `user_paused_over`) are now always restored; only the live holds are gated on `was_paused`. The restart tests built their "restarted" manager by cloning the `Arc`, so the in-memory holds and an in-memory database were shared and no persisted state was ever read. They now use a file-backed database and a fresh `QueueManager`, which is what makes the blocker reproducible; the manual-resume test fails on the previous restore logic and passes now. The schedule settings form still called `this.snack`, which #248 replaced with `this.toast` (`ToastService`). The bundle failed on the merged tree; the three calls now go through `this.toast` and the spec asserts on it. Also guards `expire_user_hold` against pausing an already-paused queue, as `apply_schedule_hold` does.
213f732 to
f51caf2
Compare
restore_from_db published every hold setting, and the schedule monitor's startup tick did the same even when its decision changed nothing. A fresh database therefore gained the default pause rows on first start, which collided with the e2e seed's own globally_paused row. Only the in-memory gate is published now on load, and a tick whose holds are unchanged writes nothing. Both paths have a regression test.
|
Re-review of d2d6bdf: no blockers. Both blockers from my last review are fixed and verified, and CI is green. GitHub passes
No Blocker 1: a manual Resume survives a real restart. Fixed.f51caf2 restores the window bookkeeping ( I re-ran my own independent real-restart scratch tests on the merged tree. These are fresh managers over a file database, not taken from the PR:
Blocker 2: CI against
|
WI-2033: the follow-ups from the #245 review. Written by a DeepSeek worker.
🤖 Generated with Claude Code