refactor(tasks): schedule with RQ instead of rq-scheduler - #4274
Conversation
Every scan request has been refused with "A scan is already in progress" on instances whose rq-scheduler cannot make progress, and waiting does not help because nothing is running to wait for. The scheduler reads a job's function name before it takes the job out of its registry, so one job left behind by an older version, holding an argument that no longer deserializes, crashes it on every poll and stays there for the next one to trip on. Nothing scheduled runs again, and the watcher's delayed rescans pile up behind it. `_get_queued_scan_jobs` counted those as scans waiting to start, so the guard added in 99e214a refused every manual scan for as long as the scheduler stayed broken. Clear jobs the scheduler cannot read at startup, before it polls them again, and split scan discovery so the guard consults only what sits on a worker queue. Delayed scans still block nothing: the scheduler is the only thing that releases them, so one that is down must not be able to refuse scans. A recovered scheduler would release its whole backlog at once and run the same library scan over and over, so delayed scans more than an hour past due go too. Stopping a scan now drops delayed scans out of the scheduler's registry rather than only cancelling the job, which left the id in the registry for the scheduler to queue anyway once the delay was up. A worker killed mid-scan points at a job that can already be gone, which raised NoSuchJobError out of the socket handler with no reply to the client, and out of GET /tasks/status as a 500. Reading a job's status has the same problem once its hash expires, so it goes through a wrapper. Finally, name the scan in the way when refusing, and say when it is stopping rather than running, so the message is actionable. Fixes rommapp#4186 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Greptile SummaryThe PR replaces rq-scheduler with RQ 2.11's native cron and delayed-job scheduling, centralizes task lookup in a registry, and updates scan discovery, watcher deduplication, startup cleanup, process supervision, tests, and documentation.
Confidence Score: 3/5This PR should not merge until the task-name override is fixed because it bypasses the endpoint's restrictions on which registered tasks may be run manually. The endpoint validates the route-selected task but enqueues a user-overridable name that the worker resolves against the full registry, allowing hidden or non-manual tasks to execute. Files Needing Attention: backend/endpoints/tasks.py, backend/tasks/tasks.py, backend/tasks/registry.py
|
| Filename | Overview |
|---|---|
| backend/endpoints/tasks.py | Replaces local task lists with the shared registry and name-based dispatch, but permits body kwargs to override the task name validated by the endpoint. |
| backend/tasks/tasks.py | Adds the common name-based task runner and removes rq-scheduler lifecycle methods. |
| backend/tasks/registry.py | Defines stable scheduled and manual task catalogs shared by cron and API execution. |
| backend/tasks/cron_config.py | Registers enabled tasks with cron strings through RQ's native cron configuration. |
| backend/endpoints/sockets/scan.py | Reworks scan discovery across workers, queues, and the native scheduled-job registry and adds stale delayed-scan cleanup. |
| backend/watcher.py | Moves delayed scans to Queue.enqueue_in and deduplicates them using keyword payload scopes. |
| backend/startup.py | Removes periodic-task initialization and cleans stale native scans plus legacy rq-scheduler state. |
| docker/init_scripts/init | Replaces the rq-scheduler process with rq cron and enables delayed-job scheduling in the worker. |
| entrypoint.sh | Updates the development process topology to run rq cron and a scheduler-enabled worker. |
| pyproject.toml | Removes the rq-scheduler dependency and raises RQ to the native-cron-capable 2.11 series. |
| uv.lock | Resolves RQ 2.11 and removes rq-scheduler and its obsolete transitive dependency. |
Prompt To Fix All With AI
### Issue 1
backend/endpoints/tasks.py:325
**Task name validation is bypassed**
When an authorized caller includes `name` in the request body, the expansion overwrites the route-selected task name, so `run_task_by_name` can execute hidden or non-manually-runnable tasks without their endpoint restrictions. **How this was verified:** The body kwargs are expanded after the trusted name, and the worker resolves the resulting value against the unrestricted task registry.
```suggestion
kwargs={**(task_kwargs or {}), "name": task_name},
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "refactor(tasks): schedule with RQ instea..." | Re-trigger Greptile
| task_instance.run, | ||
| kwargs=task_kwargs or {}, | ||
| run_task_by_name, | ||
| kwargs={"name": task_name, **(task_kwargs or {})}, |
There was a problem hiding this comment.
Task name validation is bypassed
When an authorized caller includes name in the request body, the expansion overwrites the route-selected task name, so run_task_by_name can execute hidden or non-manually-runnable tasks without their endpoint restrictions. How this was verified: The body kwargs are expanded after the trusted name, and the worker resolves the resulting value against the unrestricted task registry.
| kwargs={"name": task_name, **(task_kwargs or {})}, | |
| kwargs={**(task_kwargs or {}), "name": task_name}, |
Knowledge Base Used: Tasks and Scheduler
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/endpoints/tasks.py
Line: 325
Comment:
**Task name validation is bypassed**
When an authorized caller includes `name` in the request body, the expansion overwrites the route-selected task name, so `run_task_by_name` can execute hidden or non-manually-runnable tasks without their endpoint restrictions. **How this was verified:** The body kwargs are expanded after the trusted name, and the worker resolves the resulting value against the unrestricted task registry.
```suggestion
kwargs={**(task_kwargs or {}), "name": task_name},
```
**Knowledge Base Used:** [Tasks and Scheduler](https://app.greptile.com/romm/-/custom-context/knowledge-base/rommapp/romm/-/docs/tasks-and-scheduler.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in bce4a3c — valid, and the impact was as described: the endpoint validated the path name, then the worker resolved whatever name the payload ended up holding, so {"name": "sync_push_pull"} in the body reached a task that is neither surfaced by the API nor manually runnable.
I nested the caller's arguments instead of reordering the spread:
kwargs={"name": task_name, "task_kwargs": task_kwargs or {}},{**(task_kwargs or {}), "name": task_name} also closes it, but the collision then depends on key order surviving future edits, and it silently drops a legitimate name argument to a task's own run(). Nesting makes it structurally impossible. run_task_by_name takes task_kwargs: dict | None and forwards it.
Covered at both levels: test_forwarded_arguments_cannot_name_another_task at the runner, and test_body_cannot_override_the_task_name asserting the enqueued payload keeps the route's name.
There was a problem hiding this comment.
Pull request overview
Refactors background scheduling from rq-scheduler to RQ 2.11 using rq cron, worker-managed delayed jobs, and name-based task dispatch.
Changes:
- Replaces the scheduler dependency and updates startup/container wiring.
- Adds a centralized task registry and cron configuration.
- Reworks watcher scans, deduplication, cancellation, legacy cleanup, documentation, and tests.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Review summary |
|---|---|
uv.lock |
Updates RQ dependencies and removes rq-scheduler. |
pyproject.toml |
Replaces the scheduler dependency with RQ 2.11. |
entrypoint.sh |
Starts RQ cron and scheduler-enabled workers. Critical (1 vote): ${REDIS_SSL:+s} treats false as TLS, causing cron to use rediss:// for plaintext Redis. |
docs/BACKEND_ARCHITECTURE.md |
Documents the new scheduling architecture. |
docker/init_scripts/init |
Updates production process lifecycle management. Critical (1 vote): ${REDIS_SSL:+s} treats false as TLS, causing cron to use rediss:// for plaintext Redis. |
backend/watcher.py |
Uses delayed RQ jobs and payload-based scan deduplication. |
backend/tests/tasks/test_tasks.py |
Updates task lifecycle and runner tests. |
backend/tests/tasks/test_scan_library.py |
Updates scan task tests. |
backend/tests/tasks/test_registry.py |
Tests task registry resolution and uniqueness. |
backend/tests/tasks/test_prevent_requeue.py |
Removes obsolete scheduler tests. |
backend/tests/tasks/test_cron_config.py |
Tests cron registration behavior. |
backend/tests/tasks/test_cleanup_zip_cache.py |
Updates cleanup task tests. |
backend/tests/tasks/test_cleanup_orphaned_resources.py |
Removes obsolete scheduling lifecycle tests. |
backend/tests/endpoints/test_tasks.py |
Updates task endpoint tests for registry-based dispatch. |
backend/tests/endpoints/sockets/test_scan.py |
Tests revised scan discovery and cancellation. |
backend/tasks/tasks.py |
Adds name-based task execution. |
backend/tasks/scheduled/scan_library.py |
Removes scheduler lifecycle handling. |
backend/tasks/scheduled/cleanup_zip_cache.py |
Removes self-unscheduling behavior. |
backend/tasks/scheduled/cleanup_upload_tmp.py |
Removes self-unscheduling behavior. |
backend/tasks/scheduled/cleanup_orphaned_resources.py |
Removes custom scheduling initialization. |
backend/tasks/scheduled/cleanup_netplay.py |
Removes self-unscheduling behavior. |
backend/tasks/registry.py |
Defines scheduled and manual task catalogs. |
backend/tasks/cron_config.py |
Registers enabled cron tasks. |
backend/startup.py |
Cleans legacy scheduler state and stale scans. Moderate (4 votes): an empty old sorted set causes an early return that skips deleting legacy scheduler keys. |
backend/handler/redis_handler.py |
Adds safe job status and payload helpers. |
backend/exceptions/task_exceptions.py |
Adds unknown-task error handling. |
backend/endpoints/tasks.py |
Uses the registry and stable task-name payloads. Moderate (4 votes): merging the request body after name allows overriding a visible task with hidden cleanup_netplay, bypassing visibility and manual-run checks. |
backend/endpoints/sockets/scan.py |
Reworks scan discovery and delayed-job handling. |
backend/endpoints/responses/__init__.py |
Uses RQ job status types. |
Suppressed comments (18)
backend/endpoints/sockets/scan.py:198
ScheduledJobRegistry.get_scheduled_time()can returnNoneif the registry entry is released or cancelled between_get_scheduled_scan_jobs()and this lookup. Comparing that value withcutoffaborts startup's stale-scan sweep, leaving the remaining backlog untouched. Treat a missing timestamp as a raced-away job and continue.
if scheduled_at > cutoff:
backend/endpoints/tasks.py:205
- This catch only protects the worker-pointer branch. Queue and finished/failed entries still flow through
_build_task_status_response, which callsjob.get_status()directly; an expired job hash raisesInvalidJobOperation, so/api/tasks/statuscan still return 500. Useget_job_statusin the common response and skip or represent jobs whose status isNone.
try:
current_job = worker.get_current_job()
except NoSuchJobError:
continue
backend/handler/redis_handler.py:106
- A queue entry can disappear between
get_jobs()and this property access. RQ raisesInvalidJobOperation/NoSuchJobErrorfor that case, notDeserializationError, so the watcher can abort filesystem-change processing instead of skipping the stale job. Treat missing jobs like other unreadable payloads.
except DeserializationError:
backend/handler/redis_handler.py:92
- This wrapper is not used by
_build_task_status_responseinendpoints/tasks.py, which still callsjob.get_status()directly. If a queue or finished job expires between enumeration and response construction,GET /api/tasks/statuscan still raiseInvalidJobOperationand return 500, so the expired-job handling does not cover the status endpoint. Use this helper in the response path and skip jobs whose status is gone.
try:
return job.get_status()
except InvalidJobOperation:
return None
backend/startup.py:187
- The new payload invariant does not cover bootstrap jobs:
_enqueue_convert_images_to_webp()still enqueues the boundconvert_images_to_webp_task.runmethod, and the recompute helper above does the same, so RQ still pickles task instances. A deployment can leave one of these jobs in Redis and hit the upgrade/deserialization failure this refactor is intended to remove. Route these bootstrap jobs throughrun_task_by_namewith their registry keys too.
_enqueue_convert_images_to_webp()
backend/startup.py:154
- The orphan check only consults queue lists. A periodic
rq-schedulerjob is re-added torq:scheduler:scheduled_jobsfor its next run, but once a worker starts it, its ID is removed from the queue list. During a rolling restart this code can therefore classify that active job as an orphan and delete its hash while it is executing. Include worker orStartedJobRegistryIDs before deleting legacy hashes.
queued = set()
for queue in (high_prio_queue, default_queue, low_prio_queue):
queued.update(queue.get_job_ids())
orphans = legacy_job_ids - queued
backend/tasks/tasks.py:19
- This claim is not true for startup backfills:
startup.pystill enqueuesrecompute_save_content_hashes_task.runandconvert_images_to_webp_task.runas bound methods. Those payloads pickle task instances, so these jobs can still hit the unreadable-payload upgrade failure that this runner is meant to eliminate. Route both startup enqueues throughrun_task_by_namewith their registry names, and update the startup tests accordingly.
Every scheduled and manually triggered task is enqueued through here, so a
job payload holds a name rather than a pickled task, and nothing in Redis
depends on where the code that runs it lives.
backend/watcher.py:160
- The new watcher deduplication path is not covered by tests: there is no test for a pending delayed job whose
platform_idsare stored inkwargs, nor for suppressing a second change for the same platform. A regression to the oldjob.args[0]lookup would therefore pass the current suite even though it would recreate duplicate rescans. Add focused tests for full-library and per-platform pending scopes.
pending_scopes = [
kwargs.get("platform_ids") or []
for job in get_pending_scan_jobs()
if (kwargs := get_job_kwargs(job)) is not None
]
docker/init_scripts/init:249
rq crontreats positional arguments as queue names, not Python modules to import. Withtasks.cron_configin this position, the cron process never executes the registration module, so no periodic tasks are registered. Pass the module through the CLI's config option instead.
tasks.cron_config &
docker/init_scripts/init:228
- The URL embeds raw Redis credentials, so a password containing
@,:,/, or%is parsed as URI syntax instead of as the credential. The new cron process then connects with the wrong host or password. Percent-encode the username and password before constructing the URL, in both entrypoints.
echo "redis${REDIS_SSL:+s}://${REDIS_USERNAME-}:${REDIS_PASSWORD}@${REDIS_HOST:-127.0.0.1}:${REDIS_PORT:-6379}/${REDIS_DB:-0}"
docker/init_scripts/init:248
rq cronuses the--logging_leveloption spelling, so this new command exits with an unknown-option error before loadingtasks.cron_config. The watchdog then repeatedly restarts it without ever running periodic tasks.
--logging-level "${LOGLEVEL}" \
docker/init_scripts/init:249
- The schedule is process-local, so this production startup path also launches one independent cron scheduler per RomM container. With multiple replicas, each periodic task executes once per replica instead of once per schedule; cron needs singleton deployment or distributed ownership.
rq cron \
--path /backend \
--logging-level "${LOGLEVEL}" \
tasks.cron_config &
docker/init_scripts/init:274
- The stale sweep runs only from
startup.py, but this worker is independently restarted bywatchdog_process_pid. If it is down for more thanSTALE_SCHEDULED_SCAN_AGEwhile the watcher keeps enqueueing, RQ promotes every overdue low-queue job as soon as this worker starts, bypassing the sweep and recreating the backlog storm. Rundrop_stale_scheduled_scansbefore each scheduler-enabled worker start, or move the purge into the worker startup path.
--with-scheduler \
entrypoint.sh:67
rq crontreats positional arguments as queue names, not Python modules to import. Withtasks.cron_configin this position, the cron process never executes the registration module, so no periodic tasks are registered. Pass the module through the CLI's config option instead.
tasks.cron_config &
entrypoint.sh:63
- The URL passed to the new cron process embeds raw Redis credentials, so passwords containing
@,:,/, or%are parsed as URI delimiters and authentication fails. Percent-encode the username and password before constructingREDIS_URL, consistently with the other entrypoint.
RQ_REDIS_URL="${REDIS_URL}" \
entrypoint.sh:66
rq cronuses the--logging_leveloption spelling (the worker invocation below already uses that form). With--logging-level, the CLI exits with an unknown-option error, so this process never starts and no periodic tasks are registered.
--logging-level "${LOGLEVEL:-INFO}" \
entrypoint.sh:67
- The schedule is process-local, so starting
rq cronunconditionally here makes every horizontally scaled RomM container register and execute the same periodic tasks. Run cron as a singleton or add explicit distributed ownership, otherwise schedules such as cleanup and sync will run once per replica.
rq cron \
--path /app/backend \
--logging-level "${LOGLEVEL:-INFO}" \
tasks.cron_config &
entrypoint.sh:83
- In this entrypoint,
main.pyis launched in the background before this scheduler-enabled worker starts. On a restart with overdue RQ delayed scans, the worker can release them beforestartup.pyreachesdrop_stale_scheduled_scans(), making the stale-backlog protection timing-dependent. Start the worker after startup cleanup has completed in this entrypoint, as the production init script already does.
--with-scheduler \
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| task_instance.run, | ||
| kwargs=task_kwargs or {}, | ||
| run_task_by_name, | ||
| kwargs={"name": task_name, **(task_kwargs or {})}, |
There was a problem hiding this comment.
Fixed in bce4a3c. Same finding as the review above, and your cleanup_netplay example is exactly right — it is in the registry for cron but absent from VISIBLE_SCHEDULED_TASKS, so it was reachable that way.
I took the "reject the reserved key" half of your suggestion rather than "keep the route name last": the caller's arguments now sit under a task_kwargs key, so a body cannot collide with the task name at all, instead of relying on ordering.
| legacy_job_ids = { | ||
| job_id.decode() | ||
| for job_id in redis_client.zrange(LEGACY_SCHEDULED_JOBS_KEY, 0, -1) | ||
| } | ||
| if not legacy_job_ids: | ||
| return | ||
|
|
||
| # A cron job the old scheduler had already queued lives in both places, | ||
| # and it still has to run, so only the orphans are deleted. | ||
| queued = set() | ||
| for queue in (high_prio_queue, default_queue, low_prio_queue): | ||
| queued.update(queue.get_job_ids()) | ||
|
|
||
| orphans = legacy_job_ids - queued | ||
| if orphans: | ||
| redis_client.delete(*(f"rq:job:{job_id}" for job_id in orphans)) | ||
|
|
||
| redis_client.delete(*LEGACY_SCHEDULER_KEYS) | ||
| for key in redis_client.scan_iter("rq:scheduler_instance:*"): | ||
| redis_client.delete(key) |
There was a problem hiding this comment.
Valid, fixed. The early return did skip the key deletion, so an old scheduler that registered itself without ever holding a delayed job left rq:scheduler_lock and rq:scheduler_instance:* behind.
Now only the orphan-job calculation is guarded, and the keys go unconditionally:
if legacy_job_ids:
... # delete only hashes no queue still references
redis_client.delete(*LEGACY_SCHEDULER_KEYS)
for key in redis_client.scan_iter("rq:scheduler_instance:*"):
redis_client.delete(key)For the record on severity: both of those keys carry a TTL of the scheduler's poll interval plus ten seconds, so they expire on their own about a minute after the old process stops, and rq:scheduler appears never to be written by rq-scheduler at all — its instances key off rq:scheduler_instance:<name>. So this was untidy rather than harmful. Three tests added, including one asserting the keys are removed when the registry is empty.
| # --url, which would put it on a world-readable command line. | ||
| build_redis_url() { | ||
| if [[ -n ${REDIS_PASSWORD-} ]]; then | ||
| echo "redis${REDIS_SSL:+s}://${REDIS_USERNAME-}:${REDIS_PASSWORD}@${REDIS_HOST:-127.0.0.1}:${REDIS_PORT:-6379}/${REDIS_DB:-0}" |
There was a problem hiding this comment.
Valid, and worse than "the new cron path" — it predates this PR. ${REDIS_SSL:+s} has been choosing the scheme for the RQ worker since dd6669e, and env.template ships REDIS_SSL=false, so anyone following the documented value has had the worker pointed at rediss:// while the app itself parsed the same variable correctly through safe_str_to_bool and used redis://. Adding cron just put a second process behind the same bug.
Fixed for both processes by parsing the value, matching Python's truthy set (1, true, yes, on, case-insensitive):
ssl="$(printf '%s' "${REDIS_SSL-}" | tr '[:upper:]' '[:lower:]')"
case "${ssl}" in
1 | true | yes | on) echo "rediss" ;;
*) echo "redis" ;;
esacVerified across false, 0, true, 1, TRUE, yes, empty and unset. tr rather than ${ssl,,} so it does not depend on bash 4.
| # The URL carries the password, so it goes through RQ_REDIS_URL rather than | ||
| # --url, which would put it on a world-readable command line. | ||
| PYTHONPATH="/app/backend:${PYTHONPATH-}" \ | ||
| RQ_REDIS_URL="${REDIS_URL}" \ |
There was a problem hiding this comment.
Valid, and worse than "the new cron path" — it predates this PR. ${REDIS_SSL:+s} has been choosing the scheme for the RQ worker since dd6669e, and env.template ships REDIS_SSL=false, so anyone following the documented value has had the worker pointed at rediss:// while the app itself parsed the same variable correctly through safe_str_to_bool and used redis://. Adding cron just put a second process behind the same bug.
Fixed for both processes by parsing the value, matching Python's truthy set (1, true, yes, on, case-insensitive):
ssl="$(printf '%s' "${REDIS_SSL-}" | tr '[:upper:]' '[:lower:]')"
case "${ssl}" in
1 | true | yes | on) echo "rediss" ;;
*) echo "redis" ;;
esacVerified across false, 0, true, 1, TRUE, yes, empty and unset. tr rather than ${ssl,,} so it does not depend on bash 4.
In this file the scheme is resolved once into REDIS_SCHEME before the URL branches, so the worker and the cron process both get it.
The lockfile sat on 2.9.0 while the constraint allowed anything under 3.0, so the floor now names the version the code is tested against. 2.11 carries what the scheduler work ahead needs: RQScheduler acquires and refreshes its lock before enqueueing, each CronJob has a name and keeps the ids of the jobs it created, and calling create_cron() twice no longer duplicates jobs. 2.10 added webhook notifications and a stable scheduler identity, and 2.9.1 covers redis-py >= 8. 2.11.0 was published inside the rolling 7-day window, so it needs a per-package exclusion until 2026-08-24, when the window reaches it and the entry can go. The constraint stays under 3.0 deliberately. That release moves get_current_job() to contextvars, reworks dependency handling behind a ReadyJobRegistry, and changes the Worker.handle_job_success() signature, which RomMWorker subclasses around. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rq-scheduler has one release since 2023, 109 open issues, and RomM ran a fork of it for Redis username and SSL options that upstream never took. Everything it was here for now ships with RQ. Its two failure modes were the reason for the workarounds in 9e9f59c. It reads a job's function name before taking the job out of its registry, so one unreadable job stalls it for good; RQ's scheduler removes every id it looked at whether or not the job could be read. And cancelling a delayed job needed both `Scheduler.cancel` and `Job.cancel`, because the first left the status alone and the second left the registry entry, which the scheduler then queued anyway; `Job.cancel` on its own now clears the scheduled registry. Delayed jobs, which is how the watcher defers a rescan, go onto the queue with `Queue.enqueue_in` and are released by the worker running with `--with-scheduler`. Periodic tasks are declared in `tasks/cron_config.py` and loaded by an `rq cron` process, replacing `rqscheduler`. A task is registered only when it is enabled and has a cron string, so the schedule-then-unschedule dance around the env toggles is gone, and with it `PeriodicTask.init/schedule/unschedule`. Jobs are enqueued by task name through `run_task_by_name`, resolved against a catalog in `tasks/registry.py`. Nothing pickles a task instance into Redis any more, which is what made a payload unreadable across upgrades in the first place. The catalog also gives the tasks endpoint one home for what a task is, rather than a list of its own. Scan discovery keys off the type a job carries rather than the scheduled rescan's function name, since every task now shares one entry point. That also drops SCAN_LIBRARY_TASK_FUNC, which only existed to name a function across an import cycle. The watcher's dedupe worked on `job.args[0]` while scans are enqueued with keyword arguments, so it never matched and every filesystem change scheduled another scan. It reads `platform_ids` from the payload now, and deliberately ignores a scan already running: that one may have walked past the folder that just changed. Startup clears what the old scheduler left in Redis, once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Spreading the caller's arguments alongside the task name let a body set `name` and redirect the run, so an authorized caller could reach a task the route had just refused: one the API does not surface, or one whose `manual_run` is off. The endpoint checked the name from the path, and the worker then resolved whatever name the payload ended up holding. Nest the caller's arguments under `task_kwargs` instead of spreading them. Ordering the spread so the trusted name wins would also close the hole, but nesting makes the collision impossible rather than order-dependent, and leaves a task free to take an argument called name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
885ef0d to
bce4a3c
Compare
…m ids A scan can be scoped by platform id, by folder slug for a directory with no database row, or by rom. Treating anything without platform ids as a scan of the whole library meant a queued slug-scoped or rom-scoped scan suppressed every rescan the watcher would otherwise schedule. Summarise what the pending scans cover instead, and match a changed folder against the slugs as well as the ids. A scan whose payload cannot be read is left out: its scope is unknowable, and a duplicate scan costs less than a rescan that never happens. Adds the tests this file never had, plus tests for the job accessors that tolerate an unreadable payload. Four of them fail against the previous scope check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both startup backfills asked whether their job id existed and then enqueued it, which two instances starting together can both get past. RQ does the check, the save and the push in one Lua call and raises DuplicateJobError, so the race closes and the pre-check goes. While here, enqueue them by name like every other task, so neither payload carries a pickled task instance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shell built the connection URL with `redis${REDIS_SSL:+s}`, which
only asks whether the variable is set. `env.template` ships
`REDIS_SSL=false`, so following the documented value pointed the worker
at `rediss://` while the app parsed the same variable through
`safe_str_to_bool` and connected in plaintext. Adding the cron process
put a second process behind it.
Parse the value in both entrypoints, matching the truthy set the app
accepts.
Also clear the old scheduler's lock and instance keys even when its
registry held no jobs: a scheduler that never queued anything still
registered itself, and only the orphan-job work needs the registry.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Description
Fixes #4186
Drops
rq-schedulerand schedules with RQ itself. The library has one release since 2023 (v0.14, Oct 2024), 109 open issues, and RomM ran a fork of it for Redis username/SSL options upstream never took. Everything it was here for now ships with RQ.Both of its failure modes are why #4272 needed workarounds:
This is also what fixes #4186. On an affected instance the scheduler is stalled on a job it cannot read, so the watcher's rescans pile up in its registry, and the scan guard added in 99e214a counted those as scans waiting to start and refused every manual scan. Here the scan guard consults only what is queued on a worker, and startup clears the legacy registry outright, so both halves of that are gone.
Scheduler.enqueue_joblogs the job's function name — which unpickles the payload — before removing it from the registry, so one bad job crashes it on every poll forever. That is the outage in [Bug] Scan already in progress, ignoring request #4186. RQ's scheduler removes every id it looked at whether or not the job could be read, so this cannot happen (verified below).Scheduler.cancelleft the status alone,Job.cancelleft the registry entry, and the scheduler then queued the "cancelled" job anyway.Job.cancelalone now clears the scheduled registry.What changes:
Queue.enqueue_inand are released by the worker running--with-scheduler. One fewer process.tasks/cron_config.pyand loaded by anrq cronprocess replacingrqscheduler. A task is registered only when it is enabled and has a cron string, so the schedule-then-unschedule dance around theENABLE_SCHEDULED_*toggles is gone, and with itPeriodicTask.init/schedule/unscheduleand theunschedule()call each task made against itself.run_task_by_name, resolved against a catalog intasks/registry.py. Nothing pickles a task instance into Redis any more — which is what made a payload unreadable across an upgrade in the first place. Manual runs go through the same path.SCAN_LIBRARY_TASK_FUNC, which existed only to name a function across an import cycle.job.args[0]while scans are enqueued with keyword arguments only, so it never matched and every filesystem change scheduled another scan. It now summarises what the pending scans cover — by platform id, by folder slug, and by rom — and matches a changed folder against the slugs as well as the ids, so a queued slug-scoped or rom-scoped scan no longer reads as a full-library rescan and suppresses everything. A scan whose payload cannot be read is left out: its scope is unknowable, and a duplicate scan costs less than a rescan that never happens. It also deliberately ignores a scan already running, which may have walked past the folder that just changed.unique=Trueinstead of asking whether their job id exists and then enqueueing it, which two instances starting together can both get past. RQ does the check, the save and the push in one Lua call and raisesDuplicateJobError. They are enqueued by name too, so no payload in the codebase carries a pickled task any more.Net −496 lines.
The rq bump, folded in from #4273
uv.locksat on rq 2.9.0 whilepyproject.tomlallowed anything under 3.0, so the floor now names 2.11 — the version this was written and tested against.To be precise about why: the migration runs on 2.9, which is what was already locked. Every API it uses exists there. 2.11 is what it benefits from —
RQScheduleracquires and refreshes its lock before enqueueing, which matters once a worker rather than a separate process releases delayed jobs, and eachCronJobgained a name plus the ids of the jobs it created, which is the introspection that moving cron out of Redis would otherwise cost. 2.10 added a stable scheduler identity, and 2.9.1 coversredis-py>= 8.The constraint stays under 3.0 deliberately: that release moves
get_current_job()tocontextvars, reworks dependencies behind aReadyJobRegistry, and changes theWorker.handle_job_success()signature, whichRomMWorkersubclasses around.2.11.0 was published on 2026-08-17, so it is inside the rolling 7-day
exclude-newerwindow until 2026-08-24 and needs a per-package exclusion to resolve before then. That entry is dated and can be deleted from tomorrow onward — say the word and I will strip it rather than leave it for someone to notice.Behaviour worth reviewing
rq cronprocess rather than in Redis, so a missed run during downtime is skipped rather than fired late, andcron_job.get_job_ids()(new in 2.11) is where run history lives.tasks/registry.pyholds every scheduled task, including the four that were never surfaced by the API (cleanup_netplay,cleanup_upload_tmp,sync_retroachievements_progress,sync_push_pull).GET /api/tasksstill lists the same six as before, viaVISIBLE_SCHEDULED_TASKS— say the word if they should all be visible now.startup.pyimportsdrop_stale_scheduled_scansfromendpoints.sockets.scan, which has the layering backwards. It is not a new coupling — that module is already in startup's import graph viatasks.scheduled.scan_library— and fixing it properly means moving scan job discovery, orscan_platformsitself, out of the endpoints layer. Left as follow-up rather than growing this diff.PeriodicTask.funcis no longer used at runtime, only asserted on bytest_task_func_paths.py. Left in place to keep this diff focused; it can go in a follow-up.Checklist
Review findings addressed
REDIS_SSLwas read in the shell asredis${REDIS_SSL:+s}, which only asks whether the variable is set, whileenv.templateshipsREDIS_SSL=false. Following the documented value pointed the RQ worker atrediss://where the app itself parsed the same variable correctly and usedredis://. That predates this PR (since dd6669e), but adding the cron process put a second process behind it, so both entrypoints now parse the value against the truthy set the app accepts.rq:scheduler_lockandrq:scheduler_instance:*. Only the orphan-job work is guarded now.nameand redirect a manual run to a task the route had refused (one not surfaced by the API, or withmanual_runoff). The caller's arguments are now nested undertask_kwargsrather than spread, so the collision is impossible rather than order-dependent, and a task stays free to take an argument calledname. Covered by a test at the runner and at the endpoint.Testing
Full backend suite green (3052 passed, 2 skipped),
trunk fmt && trunk checkclean.watcher.pygets the test file it never had (19 tests over scope summarising and the dedupe; four of them fail against the previous scope check), as do the job accessors that tolerate an unreadable payload. New tests also cover the cron config (what it registers, and that it matches the real catalog), the registry,run_task_by_name, and the rewritten scan discovery; the tests for the removedrq-schedulerlifecycle are gone.Also driven against a real Redis with the real
CronScheduler, since none of that is exercised by mocks:rq cronloadstasks.cron_configand registers exactly the enabled tasks with a cron string, every one asrun_task_by_namewith no pickled task in the payload.ScheduledJobRegistry, reads as scheduled but not queued (so it cannot refuse a manual scan), is visible to the watcher as pending, and itsplatform_idsare readable for the dedupe.job.cancel()alone clears the registry entry.RQScheduler.enqueue_scheduled_jobs()releases a due job onto the queue and out of the registry.unique=Truerefuses a second enqueue of the same job id withDuplicateJobErrorand leaves the queue at depth 1, where the same pair without it enqueues twice.Not exercised locally: the container entrypoints.
docker/init_scripts/initandentrypoint.shwere rewritten to startrq cron(with a pid file for the watchdog, sincerq cronhas no--pid) and to pass--with-schedulerto the worker; both need a Docker run before merge.AI assistance disclosure
This change was written with AI assistance (Claude Opus 5 via Claude Code). The design, the code, the tests, and this description were AI-generated, then reviewed and verified by me: I read RQ's scheduler, cron, registry, and job sources to confirm the semantics claimed here, ran the suite and linters, and ran the real-Redis verification above.