fix(downloads): make retry-import actually requeue the import - #917
fix(downloads): make retry-import actually requeue the import#917m4bard wants to merge 1 commit into
Conversation
9ff711e to
eb553b9
Compare
|
Amended and force pushed, now
That state is this endpoint's own doing. Before the change in this PR it cleared the block and set They are also worse off than blocked ones, which I had not appreciated when I opened this:
So a download the old endpoint touched is not merely unimportable. The book stops being searched as well, silently, because something looks like it is already downloading it. A blocked download at least leaves the book eligible. The active job check is what keeps this narrow. An import genuinely in flight has a pending or processing job and is still refused, so this is not "accept anything that is not blocked". Two tests cover it, and the second is there so the pair cannot both pass against a version that simply accepted every Nothing else in the PR changed. Worked through with Claude Code at my direction. The claims above were checked by running them rather than by reading, and I reviewed this before posting. |
POST /api/v1/downloads/{id}/retry-import returned 200 with "Import retry
queued" and logged "Reset blocked import ... back to ImportPending", but
queued nothing. It called download.Unblock() and persisted the row. That
clears ImportBlockReason and sets the status to ImportPending, and nothing
anywhere watches either field.
The only thing that imports a download is a DownloadProcessingJob, and in
production only two places create one: DirectDownloadProcessor, for DDL, and
DownloadMonitorService.OnDownloadCompleted. The second is reached from
TriggerCallbacks, which switches on (previous.Status, current.Status) and
returns immediately when they are equal. It is edge-triggered on the download
client reporting completion. For a blocked import that edge is in the past and
will not happen again, so the download sat in ImportPending forever while the
endpoint reported success.
The Retry button in the downloads view was a stub on the same feature:
const retryDownload = async () => {
toast.info('Coming Soon', 'Retry functionality will be implemented soon')
}
It never called retry-import, and it rendered on status === 'Failed', which the
endpoint rejects with 400 because it requires ImportBlocked. Wiring the stub
straight to the endpoint would not have helped; the button could not appear for
a state the endpoint accepts.
Adds IDownloadProcessingJobService.RequeueAsync. It is separate from
EnqueueAsync because that one is driven by the client reporting completion and
so requires the download to be Completed, while a retry arrives later, once the
download is already ImportPending. Requeue reuses the download's newest job so
its processing log survives, which is the record of why the earlier attempts
failed, and resets the retry budget because an operator asking for another
attempt has usually changed something the job depends on. If retention has
already deleted the job, a fresh one is created.
The controller queues before it persists the unblock, so a failure leaves the
download blocked rather than in a new silent limbo.
The existing controller test asserted the status transition only, so it passed
against an endpoint that did nothing. It stays as it is and a second test now
asserts that a job is actually queued.
Verified by reverting each production change in turn and confirming the matching
test fails: the controller requeue, the job reuse, the retry-budget reset, and
both halves of the frontend change.
It also accepts a download already stranded in ImportPending with no active job.
Before this endpoint queued anything it still cleared the block and set
ImportPending, so anyone who called the old version is left with downloads that
nothing will ever pick up. Those are worse off than blocked ones:
AutomaticSearchService and DownloadDuplicateGuard both count ImportPending as an
active download and skip the book, while neither counts ImportBlocked, so the
book is never re-searched either. Refusing them would leave every existing
victim stranded for good.
The active-job check is what keeps that narrow. An import genuinely in flight
has a pending or processing job and is still refused, so this is not "accept
anything that is not blocked".
It also refuses, with a conflict rather than a constraint violation, when another
download for the same audiobook is already active. EfDownloadRepository derives
ActiveAudiobookDeduplicationKey from the audiobook id for active statuses and a
filtered unique index enforces one at a time. ImportPending is active and
ImportBlocked is not, so unblocking collided with any live sibling and surfaced
as a SQLite constraint violation from inside SaveChanges, which tells the caller
nothing about what to do.
A terminal sibling holds no key and does not block the retry, which has its own
test: without it the check would refuse every book that had ever failed twice.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YEVQ7qDJLk5196MFeggWuA
eb553b9 to
1b24f67
Compare
POST /api/v1/downloads/{id}/retry-importanswers200with"Import retry queued"and logsReset blocked import ... back to ImportPending. It queues nothing.I hit this trying to retry a blocked import. The call succeeded and the status changed, but no
DownloadProcessingJobsrow ever appeared for that download. I gave it fifteen minutes, then restarted the container in case a fresh processor would pick it up, and it stayed where it was.Why
RetryBlockedImportcallsdownload.Unblock()and saves the row. That clearsImportBlockReasonand sets the status toImportPending. Nothing watches either field.The only thing that imports a download is a
DownloadProcessingJob, and two places in production create one:DirectDownloadProcessorfor DDL, andDownloadMonitorService.OnDownloadCompleted. The second is reached fromTriggerCallbacks, which switches on(previous.Status, current.Status)and returns as soon as they match:So the enqueue is edge-triggered on the download client reporting completion. For an import that is already blocked, that edge is in the past and will not come round again. The row sits in
ImportPendingwhile the endpoint reports success.The button
The Retry button in the downloads view is the same feature approached from the other side:
It never called the endpoint. It also rendered on
download.status === 'Failed', and the endpoint answers400to anything that is notImportBlocked, so pointing the stub at the endpoint would not have been enough by itself. The button could not appear for a state the endpoint accepts.Between the two, there is currently no working way to retry a blocked import by any route. #890 is someone sitting in that state.
What this changes
Adds
IDownloadProcessingJobService.RequeueAsync. I made it separate fromEnqueueAsyncrather than loosening that one, because they are driven by different things:EnqueueAsyncruns off the client reporting completion and so requires the download to beCompleted, while a retry arrives later, once the download is alreadyImportPending.RequeueAsyncreturns an existing active job if there is one. Failing that it reuses the download's newest job, and failing that it creates a fresh one, since retention deletes terminal jobs after a week. Reusing keeps the processing log, which is the only record of why the earlier attempts failed. It does reset the retry budget, on the grounds that someone asking for another attempt has usually changed something the job depends on. The recent-completion cooldown thatEnqueueAsyncapplies is deliberately left off, since silently doing nothing is the behaviour being fixed.The controller queues before it persists the unblock. If the queue call fails, the download stays blocked rather than landing in a state that reads as retrying and never is.
The button now calls the endpoint, and is offered on
ImportBlocked.Tests
RetryBlockedImport_ImportBlocked_TransitionsToImportPendingasserted the status transition and nothing else, so it passed against an endpoint that did no work. I left it alone and added a second test beside it that asserts a job is actually queued.Five tests added on the backend and two on the frontend. I checked that each one is capable of failing, by reverting the production changes one at a time: the controller requeue, the job reuse, the retry-budget reset, and both halves of the frontend change. Each time the matching test failed and the rest stayed green.
Full suite passes.
Worked through with Claude Code at my direction. The claims above were checked by running them rather than by reading, and I reviewed this before posting.