fix(imports): retry a failed file import instead of blocking on the first attempt - #933
Open
m4bard wants to merge 1 commit into
Open
fix(imports): retry a failed file import instead of blocking on the first attempt#933m4bard wants to merge 1 commit into
m4bard wants to merge 1 commit into
Conversation
…irst attempt A download whose file import fails is marked Failed immediately, with RetryCount still 0, and the download goes to ImportBlocked. So a source file that is briefly unavailable, such as one the download client still holds open, ends the download permanently. Most failure points in ProcessJobAsync take the bounded retry path instead: source path missing, client fetch, files on disk not matching, marking the item imported, enqueuing the scan, and committing finalization all call ScheduleRetryAsync. Three do not, and this changes one of them, the per-file ImportResult failures. The catch of InvalidOperationException around the import call and the "No audio files were registered after file import" case are left alone, because it is less clear their failures are transient. Route the per-file case through ScheduleRetryAsync while attempts remain. Terminal behaviour is unchanged: once they are spent the download is blocked exactly as before, and the terminal call is still FailImportAsync rather than letting ScheduleRetryAsync exhaust itself, because FailImportAsync is the only one that records failedResults on the history entry. Import_FailedPublication_PersistsFailureContractInHistory reached its assertion through the first import failure, which is no longer terminal. It now seeds RetryCount = MaxRetries so it exercises the terminal attempt, the one that writes the FailedResults contract it checks. Its assertions are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A file import that fails once ends the download permanently, even though the job has a retry budget
it never spends.
What happens
In
DownloadProcessingJobProcessor.ProcessJobAsync, ifImportDownloadFilesAsyncreturns anyunsuccessful
ImportResult, the job goes straight toFailImportAsync.RetryCountis still 0 atthat point, and
ProcessQueueAsyncthen blocks the download. A source file that is brieflyunavailable, say one the download client still has open, is therefore treated the same as a file
that will never import.
Most failure points in that method already go through
ScheduleRetryAsync: the source path missing,the client fetch, the files on disk not matching what was expected, marking the item imported,
enqueuing the scan, and committing finalization. Three do not. I have changed one of them.
catch (InvalidOperationException)around the import call, which looks more like a state orprogramming error than something a retry would clear
ImportResultfailures, which is this changeNo audio files were registered after file import, in the case where the audiobook has noexisting files either
I left the other two as they are. I am not confident their failures are transient, and I would
rather send you one change I can argue for than three I cannot. If you want either of them routed
the same way, that is easy to add.
The change
Send the per-file failure case through
ScheduleRetryAsyncwhile attempts remain, then callFailImportAsynconce they are spent.Terminal behaviour does not change. With the budget exhausted the download still ends up
ImportBlocked, and the last call is stillFailImportAsyncrather than lettingScheduleRetryAsyncrun itself out, because
FailImportAsyncis the only one that recordsfailedResultson the historyentry. That per-file detail seems worth keeping for the attempt that gives up.
Tests
One new test,
Import_FileImportFailure_RetriesBeforeBlockingTheDownload. It fails on canary andpasses here.
One existing test needed adapting, though not in what it asserts.
Import_FailedPublication_PersistsFailureContractInHistorychecks theFailedResultscontract on thehistory entry, and it used to reach that assertion through the first import failure, which is no
longer terminal. It now seeds
RetryCount = MaxRetriesso it exercises the terminal attempt, which isthe one that writes the contract it is testing. The assertions themselves are untouched.
Where this sits
This touches
DownloadProcessingJobProcessor.cs, which you have changed three times recently, mostrecently in 76be79f. If it collides with something you have in progress, say so and I will drop it
or rework it around whatever lands. I have no attachment to this shape.
Worked through with Claude Code at my direction. The test claims above were checked by running
them, including the failure on canary with canary's own production file. The description of the
other two failure paths is source reading rather than something I exercised. I reviewed this
before posting.