fix(scheduler): stop a failed queue row blocking an automatic re-grab (#2710)

[vavallee]

Summary

Automatic search checked only whether the queue held a row for the release's GUID, so any row stopped the grab, including one that had failed months earlier. The reporter had twenty five rows failed on a loopback URL refusal from before BINDERY_DOWNLOAD_ALLOW_LOOPBACK was set and on indexer 429 and 500 responses from one bad afternoon, and every automatic re grab of those releases was dropped, silently, after the "auto-grabbing book" line had already been logged. This makes a finished attempt stop blocking, reuses its row when the grab goes ahead, and makes every skip say why. Closes #2710.

Which states block, and why

The manual grab has decided this correctly since #1955. The predicate moves into models (DownloadState.IsDeadForRegrab, Download.BlocksRegrab), api.regrabbable delegates to it, and the scheduler now shares it, so the two paths cannot drift.

State	Blocks a re grab	Why
grabbed, downloading, completed, importPending, importing	yes	live work, re grabbing duplicates it
importExternal, importHeld	yes	deliberately non terminal, the files are still on their way in
importFailed	yes	the scanner is still spending its retry budget on that row, a re grab would race it
imported with its book	yes	you already have the book, this is what the state is for
imported with no book (orphaned import)	no	#2289, unchanged, reused whatever its age
failed	no (after a cooldown)	a finished attempt, usually for a cause that is already gone
importBlocked	no (after a cooldown)	terminal to every automatic path since #1955, otherwise pins the GUID forever

The cooldown, and the blocklist question

A dead row is only reused once its last activity is at least six hours old. That is the interesting judgement call here, so the reasoning in full:

• Only the stall handler blocklists. markDownloadFailed (a torrent erroring in the client, a SABnzbd failure) does not, and the send failure path in SearchAndGrabBook does not either. So the blocklist does not cap the general case, and without a bound the sweep would re grab a client side failure on every pass, which can be hourly (search.interval minimum is 1h, default 12h).
• Six hours sits between the two: the ordinary next sweep of that book retries the release, and the tightest configured cadence still cannot hammer the download client with it.
• A person clicking Grab is not bound by it. RetryFailed is untouched and still reuses a row that failed seconds ago.

Combined behaviour after this change, for a release that keeps failing:

• Stalls in the client → the existing stall path marks it failed, blocklists it and re searches. BlocklistedSpec then rejects it at decision time, so it is never picked again and the cooldown never comes into it. Unchanged.
• Fails at the client or at send, repeatedly → retried at most once per six hours, each attempt visible in the log and in the queue row's error message, until a better release outranks it or the user blocklists it by hand. Not blocklisted automatically. Auto blocklisting after N failures would be a bigger behaviour change than this bug warrants, and the reporter's whole complaint is a release poisoned by one transient failure, so I have deliberately not added it. Worth a separate issue if the rate turns out to matter.

Reuse rather than a second row

RetryOrphanedImport becomes RetryDeadForAutoGrab(ctx, d, idleBefore) and carries both conditions in SQL: dead and idle past the cutoff, or an orphaned import. It reuses regrabClaimSQL, so the row keeps its id and guid and every per grab column is rewritten or reset (error_message, import_path, sabnzbd_nzo_id, torrent_id, owner_user_id, added_at, grabbed_at, completed_at, imported_at, import_retry_count). The queue therefore shows one row describing this grab, and the history event written after the send describes this grab alone. downloads.guid is UNIQUE, so reuse is also the only shape that works without deleting the old row.

The cooldown is in the SQL rather than only in the caller for the same reason the states are: between the scheduler reading the row and claiming it, a manual grab can claim it and fail, leaving a row that died seconds ago where the scheduler saw one that died months ago. downloads has no updated_at, so "last activity" is COALESCE(completed_at, grabbed_at, added_at), mirrored in Go by Download.LastActivityAt. Times are stored as RFC3339 UTC (verified against modernc/sqlite: 2026-09-19T03:11:58.643383413Z), so a bound time.Time compares correctly; julianday() returns NULL on that format, so it is deliberately not used.

Making the skip visible

The existing end of run line is slog.Info("book search finished", ..., "outcome", ...). It has no GUID and no download id, so the outcome alone cannot answer "which row held it back". Both are now emitted:

• outcome becomes already grabbed (downloading) / failed too recently (failed) instead of a flat already grabbed.
• A new INFO line per skip: skipping a release the queue still holds with book, release, guid, existing_download_id, existing_status, reason. INFO rather than DEBUG because it fires at most once per book per format per sweep, and it is the line the reporter went looking for and did not find.
• The lost claim race also logs now, with its own outcome (claimed by another grab) rather than reusing "already grabbed".

Fail before evidence

Base is d2bec3b0 (origin/main at branch time). Each run below puts the base source back under the new tests.

internal/scheduler/scheduler.go reverted to base (with a shim restoring the base RetryOrphanedImport), new tests unchanged:

--- FAIL: TestSearchAndGrabFormat_ReusesStaleFailedRow/failed
    #2710: a release whose only row failed months ago must be grabbed again; the client got 0 requests
--- FAIL: TestSearchAndGrabFormat_ReusesStaleFailedRow/importBlocked
    #2710: a release whose only row failed months ago must be grabbed again; the client got 0 requests
--- FAIL: TestSearchAndGrabFormat_StillSkipsKnownRelease/failed_seconds_ago
    the skip must be visible in the logs: "g-2289" missing from
    the skip must be visible in the logs: "failed" missing from
    the skip must be visible in the logs: "failed too recently" missing from
    the search outcome must name the reason and the blocking status
--- FAIL: TestSearchAndGrabFormat_StillSkipsKnownRelease/imported_with_its_book
    the skip must be visible in the logs: "g-2289" missing from
    the skip must be visible in the logs: "imported" missing from
    the search outcome must name the reason and the blocking status

(the other three StillSkipsKnownRelease cases fail the same way on the log assertions; they already refused the grab on base, which is the point of keeping them)

internal/db/downloads.go claim reverted to the base RetryOrphanedImport:

--- FAIL: TestDownloadRepoRetryDeadForAutoGrabConditions
    db_test.go:1681: #2710: a row that failed months ago must not block the scheduler's re-grab

What the new tests cover: a stale failed and a stale importBlocked row are re grabbed and the reused row is coherent (id, book, owner, title, url, cleared error and import path, new nzo id, status downloading, exactly one row); a row that died seconds ago, an in flight row, an importFailed row and an imported row with its book are all still refused and left untouched; the orphaned import path still passes unchanged (TestSearchAndGrabFormat_ReusesOrphanedImport, untouched); every skip is asserted present in the logs with GUID, status and reason; at the repo layer, an importBlocked row added months ago but completed_at a minute ago is still inside the cooldown, and RetryFailed still accepts a row that failed seconds ago for the manual grab.

Security

No new endpoint, no new setting, no new dependency. The one thing worth naming is tenancy (#1457): the reused row is claimed with owner_user_id = book.OwnerUserID, exactly as RetryOrphanedImport did, so a dead row left by one user and re grabbed for another user's book moves to that book's owner rather than staying in the first user's queue. Asserted in TestSearchAndGrabFormat_ReusesStaleFailedRow. The manual grab's gate is unchanged in behaviour; regrabbable now delegates to models.Download.BlocksRegrab, which is the same predicate it computed inline.

Performance

No new query. The GUID lookup was already there, and the claim is the same single UPDATE with two extra conditions in its WHERE. No new index needed: the claim is keyed on id.

Checklist

• Tests added or updated
• Doc-update gate cleared (docs/Troubleshooting-Wiki.md gains a section on the automatic search side of this; godoc on every touched function)
• Wiki pages updated if user-facing behaviour changed
• Changelog fragment (changelog.d/2710-failed-row-blocks-regrab.md, credits ccarpinteri)

Test plan

• go build ./..., go vet ./...
• go test ./internal/scheduler/... ./internal/api/... ./internal/db/... ./internal/models/... ./internal/importer/...
• go test -race ./internal/scheduler/ ./internal/db/ ./internal/models/ on the touched tests
• cd web && npm run build (no frontend change in this PR)

🤖 Generated with Claude Code

https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9

[codecov]

Codecov Report

❌ Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/scheduler/scheduler.go	76.92%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!