Skip to content

fix(scan): stop refusing scans a stalled scheduler left pending - #4272

Closed
sdornan wants to merge 1 commit into
rommapp:masterfrom
sdornan:claude/github-issue-4186-1faa59
Closed

fix(scan): stop refusing scans a stalled scheduler left pending#4272
sdornan wants to merge 1 commit into
rommapp:masterfrom
sdornan:claude/github-issue-4186-1faa59

Conversation

@sdornan

@sdornan sdornan commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Description

Scans have been refused with "Scan failed: A scan is already in progress" on 5.1.0, with Scan already in progress, ignoring request in the logs and no scan ever appearing. Waiting does not help, because nothing is running to wait for.

The cause is a stalled rq-scheduler. It 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 (the 'unidentified' is not a valid ScanType traceback in #4186, from a delayed job created before ScanType was refactored in 9fa15d2). Nothing scheduled runs again, and every watcher rescan piles up in the registry behind it. _get_queued_scan_jobs counted those as scans waiting to start, so the concurrency guard added in 99e214a refused every manual scan for as long as the scheduler stayed broken. That matches all three reports in the issue: watcher off makes scans work again, and clearing Redis fixes it.

What this changes:

  • Clear jobs the scheduler cannot read at startup, before it polls them again, so an affected instance recovers on upgrade instead of needing Redis wiped by hand.
  • Split scan discovery, so the guard consults only scans sitting on a worker queue. Delayed scans block nothing now: the scheduler is the only thing that releases them, so a scheduler that is down must not be able to refuse scans.
  • Drop delayed scans more than an hour past due at startup. A recovered scheduler would otherwise release its whole backlog at once and run the same library scan over and over. The standing cron entry is left alone, since it reschedules itself.
  • Stopping a scan now drops delayed scans out of the scheduler's registry, not just cancels the job. Cancelling alone left the id in the registry, and the scheduler queued it anyway once the delay was up, running the scan that was just stopped.
  • Tolerate jobs that are already gone. A worker killed mid-scan points at a job that may no longer exist, 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 alongside get_job_func_name.
  • Say what is in the way when refusing: "Quick Scan is already running", "… is already queued", or "… is still stopping, try again in a moment".

No API or schema change, so no frontend type regeneration. v2 already reconciles with a running scan via /tasks/status; extending that to a refusal is a separate UI change.

Related defect found while investigating, not fixed here: the watcher's own dedupe in get_pending_scan_jobs never matches, because it filters on job.args[0] while the watcher enqueues with keyword arguments only. That is why the backlog grows one job per filesystem change.

Checklist

  • I've tested the changes locally
  • I've updated relevant comments
  • I've assigned reviewers for this PR
  • I've added unit tests that cover the changes

Testing

Full backend suite green (3015 passed, 2 skipped), trunk fmt && trunk check clean.

New unit tests cover the guard (a scheduled watcher scan, a cancelled queued scan, a job whose status is gone, and a worker holding a job that no longer exists all stop blocking), the refusal messages, dropping delayed scans out of the scheduler on stop, the unreadable-job purge, and the stale-backlog cleanup.

Verified against a real Redis and a real rq-scheduler registry as well, since mocks cannot exercise the rq-scheduler interaction: a job with unreadable data does crash enqueue_jobs() with DeserializationError and remains in the registry; the purge clears it and the next poll succeeds; a three-scan backlog is dropped while a scan still waiting out its delay survives; and a delayed scan contributes nothing to the queued set the guard checks.

AI assistance disclosure

This change was written with AI assistance (Claude Opus 5 via Claude Code). The issue diagnosis, the fix, the tests, and this description were AI-generated, then reviewed and verified by me: I confirmed the rq-scheduler and RQ behaviour in their sources, ran the test suite and linters, and ran the real-Redis verification described above.

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>
Copilot AI lite review requested due to automatic review settings August 23, 2026 19:13
@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes scan concurrency checks ignore delayed scheduler entries while adding recovery and cancellation handling for stale, unreadable, expired, and missing RQ jobs.

  • Separates queued scan discovery from delayed scheduler entries and improves scan-refusal messages.
  • Removes delayed scans when stopping and purges stale or unreadable scheduler entries during startup.
  • Tolerates missing worker jobs and expired job status records in scan and task-status discovery.
  • Adds focused tests for concurrency checks, cancellation, cleanup, and missing-job behavior.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code failure identified.

The updated discovery and cleanup paths consistently guard missing or expired RQ state, preserve standing cron entries, and distinguish delayed jobs from scans that are actually queued or running.

Important Files Changed

Filename Overview
backend/endpoints/sockets/scan.py Separates queued and scheduled scans, adds scheduler cleanup/cancellation helpers, handles missing jobs, and reports more specific concurrency refusal messages.
backend/endpoints/tasks.py Prevents stale worker registrations referencing expired jobs from failing the task-status endpoint.
backend/handler/redis_handler.py Adds a guarded job-status accessor for RQ jobs whose Redis state has expired.
backend/startup.py Runs best-effort scheduler-registry recovery before initializing periodic tasks.
backend/tasks/tasks.py Removes scheduler entries whose job payload can no longer expose a function name.
backend/tests/endpoints/sockets/test_scan.py Adds coverage for delayed, cancelled, missing, stale, and stopping scan-job states.
backend/tests/tasks/test_tasks.py Adds unit coverage for retaining readable jobs and cancelling unreadable scheduler entries.

Reviews (1): Last reviewed commit: "fix(scan): stop refusing scans a stalled..." | Re-trigger Greptile

Copilot AI 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.

Pull request overview

Fixes scan concurrency false-positives caused by rq-scheduler registries getting stuck (for example due to unreadable legacy jobs), so manual scans are no longer refused when nothing is actually running, and so stalled scheduler backlogs can self-heal on startup.

Changes:

  • Add startup-time cleanup for unreadable scheduled jobs, plus stale delayed scan backlog pruning.
  • Refine scan job discovery to distinguish running vs queued vs scheduled jobs, and make the refusal message report what is blocking (running, queued, or stopping).
  • Harden task and scan status querying against RQ edge cases where jobs referenced by workers/registries no longer exist.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
backend/tasks/tasks.py Adds scheduler-registry cleanup for unreadable scheduled jobs.
backend/startup.py Runs scheduler cleanup during startup before scheduled tasks initialize.
backend/handler/redis_handler.py Adds safe wrapper for job status retrieval when job hashes expire.
backend/endpoints/sockets/scan.py Splits queued vs scheduled scan discovery, improves stop behavior, and improves refusal messaging.
backend/endpoints/tasks.py Avoids 500s when workers reference jobs that no longer exist.
backend/tests/endpoints/sockets/test_scan.py Adds unit coverage for new scan discovery/stop/refusal behavior and stale-scan pruning.
backend/tests/tasks/test_tasks.py Adds unit coverage for unreadable scheduled job cleanup.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread backend/startup.py
Comment on lines +19 to 20
from endpoints.sockets.scan import drop_stale_scheduled_scans
from handler.database import db_save_handler
@sdornan

sdornan commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #4274, which replaces rq-scheduler with RQ's own cron and delayed-job scheduling.

Everything this PR fixed is carried there: the NoSuchJobError guards on the socket handler and /tasks/status, the safe job-status read, scheduled scans no longer counting as scans in flight, the refusal message naming what is in the way, and the stale-backlog sweep. The two workarounds it needed are deleted instead of kept — RQ's scheduler cannot stall on a job it cannot read, and Job.cancel() alone clears the scheduled registry.

Fixes #4186 has moved to #4274.

@sdornan sdornan closed this Aug 23, 2026
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