Repository navigation
Conversation
0357d80 to
a41781a
Compare
|
Thanks for this. Off by default, the 1. Take the operation lock instead of checking it. A related case: the lock goes stale after 10 minutes. With the lock held only by sub-steps, a stream match that runs past that lets the next scheduled run start while the first is still going. 2. The default time zone comes out as UTC on a normal install. 3. No database write on every action. Smaller points:
|
|
Thanks for the detailed review on this one too. #32 came together quickly because once you gave the exact repro, the fix was narrow — call the same helper from the one other place that needed it. This one's a different scale: the locking semantics especially deserve more than a quick pass, so I want to actually sit with your owner-token suggestion rather than rush something that only looks right. Planning to spend some time on it this weekend and have an update early next week. |
Adds settings under a new "Automation" section (Automatic Full Sync: Off/Daily/Weekly/Monthly/Custom, Sync Time, Sync Day, Sync Day Of Month, Custom Cron Expression, Time Zone) that register a django-celery-beat periodic task reusing the exact manual Full Sync path (groups -> channels -> stream matching -> EPG matching -> logo assignment). Per the issue discussion and this PR's review: - The periodic task is routed to Dispatcharr's "dvr" Celery queue (set on the PeriodicTask row itself, not on the Celery task decorator), not the default queue a plugin task silently never runs on. - A scheduled run acquires the operation lock itself, as the owner for the whole run, and skips without raising if a different Plugin instance (a manual action) already holds it. _do_full_sync's own groups, channels, stream match, EPG match and logo assignment steps each reacquire that same lock on this same Plugin instance and succeed - reentrant, via an owner token _acquire_lock stores in the lock file - instead of finding it already held and rejecting themselves. An earlier version of this only peeked at the lock without holding it, which let a manual action start mid-run and touch the same groups/channels rows (neither of which took a lock of their own before this), and let the lock go stale and a second scheduled run start while the first was still going if any one sub-step ran past the timeout. A reentrant reacquire now refreshes the lock's timestamp, closing that gap too. - Off by default. The last scheduled run's outcome (ok / error / skipped, with a timestamp and message) is recorded to its own state file and shown as a row in both Validate Settings and Plugin Status. - Time Zone is explicit rather than inferred (tested live: a schedule created without one fired on UTC instead of the box's actual local time, silently missing the intended run), but its default is pre-filled from Dispatcharr's own configured system time zone (core.models.CoreSettings.get_system_time_zone(), the same source progress_status.py already uses for display), not Django's settings.TIME_ZONE - Dispatcharr pins that to UTC regardless of the admin-configured zone, so checking it first always returned a valid but wrong default. - Monthly (a specific day of month) and Custom (a raw 5-field cron expression, validated with celery's own crontab parser before being saved) cover schedules the Daily/Weekly presets cannot express. - The scheduled task logs through Dispatcharr's own "apps.plugins.loader" logger - the same one manual actions use via context["logger"] - so a scheduled run's log lines look and land identically to a manual Full Sync's, rather than under a separate, easy-to-miss logger name. Dispatcharr has no "settings changed" hook for plugins, so there is no single moment to apply a schedule edit. Reconciling the periodic task (and the settings snapshot its Celery task runs with, since a Celery task has no other way to read "whatever is currently saved") happens at the top of every action dispatch instead. That previously meant a database write on every single action, including read-only ones like Plugin Status; it now hashes what it is about to write and skips the query entirely when nothing relevant has changed since the last reconcile, while still refreshing the settings snapshot on an unrelated settings change (e.g. a different lineup file) rather than letting it go stale. The actual scheduled-run logic lives in a plain Plugin method (run_scheduled_full_sync) rather than inline in the @shared_task function, so it stays unit-testable in an environment without Celery installed - the same reason the module-level task registration is wrapped to fail soft if Celery/django-celery-beat aren't present. Verified live end-to-end against a real install (Dispatcharr 0.31.0, django-celery-beat installed, dvr queue worker running): the schedule registers, Beat picks it up on the correct time zone, the task fires on the dvr queue, a completed run produces the same CSV/report output as a manual Full Sync, and no self-rejection warnings appear during a run that previously produced exactly three of them. Fixes PiratesIRC#31
a41781a to
e9878b3
Compare
|
Went with your owner-token idea for the locking - run_scheduled_full_sync grabs the lock itself now for the whole run, and the groups/channels/stream/EPG/logo steps just reuse it instead of fighting over it. Turns out groups and channels weren't taking a lock at all before, so that's fixed too. Reacquiring also bumps the timestamp so a slow step doesn't make the lock look stale partway through. Timezone now pulls from CoreSettings.get_system_time_zone() like progress_status.py already does, instead of Django's TIME_ZONE which was always coming back UTC. And the reconcile function hashes what it's about to write so it skips the DB hit when nothing's actually changed, but still updates the snapshot if something unrelated changes so it doesn't go stale. Also cleaned up the smaller stuff you flagged (queue comment, contractions, status line length). Tests and gates all still green. Let me know if anything looks off. |
Summary
DISPATCHARR_TIME_ZONE/ DjangoTIME_ZONEso a fresh install doesn't need to already know it).django-celery-beatPeriodicTaskthat fires the exact same Full Sync path as the manual button (groups -> channels -> stream matching -> EPG matching -> logo assignment).PeriodicTaskrow is created withqueue="dvr", per the issue discussion - a plugin task left on Dispatcharr's defaultceleryqueue is registered and Beat's run counter still increments, but a worker never picks it up._do_full_syncin its own operation-lock acquisition._do_full_sync's own sub-steps (stream match, EPG match, logo assignment) already take that same lock around their own work, exactly like the manual Full Sync button. An outer acquisition holds the lock across all of them, so each sub-step sees it already held by this same run and rejects itself - measured live as exactly three "Operation locked" warnings from one single invocation, with no sync ever completing.run_scheduled_full_syncnow only peeks (read-only, never acquires) before deciding whether to start at all, then calls_do_full_syncdirectly and unwrapped, same as the manual path.apps.plugins.loaderlogger - the same one manual actions receive viacontext["logger"]- so a scheduled run's log output looks and lands identically to a manual Full Sync's, rather than under a separate, easy-to-miss logger name.ok/error/skipped, with a timestamp and message) is written to its own state file and surfaced as a row in both Validate Settings and Plugin Status.Design note worth a look
Dispatcharr's plugin API has no "settings changed" hook, so there's no single moment to apply a schedule edit. I reconcile the
PeriodicTask(and the settings snapshot its Celery task runs with - a bare Celery task has no other way to read "whatever is currently saved in the UI") at the top of every action dispatch (run()) instead of requiring a dedicated "Apply Schedule" button. It's a single cheap idempotent query per action. Happy to switch to an explicit button if you'd rather not have every action touch this.I did not change
OPERATION_LOCK_TIMEOUT_MINUTES(currently 10 minutes) - your issue reply flagged it might need raising or refreshing for a sync that runs long. Observed Full Sync duration on a 227-channel lineup is ~20s, well under that, so I left it as-is rather than guess.Test plan
pytest tests/- 1128 passed (tests/test_scheduled_full_sync.py: queue routing, off-by-default, time/day/day-of-month/cron validation, the lock-reentrancy regression specifically, status-row visibility, timezone auto-detection, and that a broken schedule reconciliation never blocks the actual requested action).github/scripts/validate_plugin.py.github/scripts/check_core_parity.py/check_client_parity.py/check_matcher_golden.py(untouched)scripts/check_version_sync.pyruff check . --select E9,F63,F7,F82python -m py_compile Lineuparr/*.pydvrqueue worker running): the schedule registers, Beat picks it up and fires it at the correct configured time zone, the task runs on thedvrqueue, the lock fix resolved the real reentrancy bug described above, and a completed scheduled run produced the same CSV/report output as a manual Full Sync, logged identically viaapps.plugins.loader.Neither
celerynordjango-celery-beatare dependencies of this plugin's own test suite (they're Dispatcharr's), so the scheduling logic is unit-tested against fakes fordjango_celery_beat.modelsandcelery.schedules.crontab; the live verification above is what confirms the real integration.Opening as a draft for your read on the reconcile-on-every-action design and the lock-timeout question above.