What happens
DownloadService.SendToDownloadClientAsync hands the prepared submission to the gateway
with no cancellation token:
listenarr.application/Downloads/Submission/DownloadService.cs:337
submissionResult = await clientGateway.AddAsync(downloadClient, prepared);
IDownloadClientGateway.AddAsync declares CancellationToken ct = default
(listenarr.application/Downloads/Contracts/IDownloadClientGateway.cs:29-32), so the token
in play is None and the method has no way to ask whether anyone actually requested a
cancellation.
The next two arms are:
DownloadService.cs:343-347, catch (OperationCanceledException), which removes the
provisional Download row and rethrows.
DownloadService.cs:348-358, the general arm, which removes the row and wraps the cause
in DownloadClientSubmissionException.
An HttpClient request timeout throws TaskCanceledException, and that derives from
OperationCanceledException, so it takes the first arm. History is only written after the
client has accepted (DownloadService.cs:368-374), and the provisional row is gone, so a
grab against a download client that is down, unreachable or wedged ends with nothing
written anywhere. The caller sees a raw TaskCanceledException; the UI shows the release
simply not appearing.
I think the cost is that the next automatic search picks the same release again, with the
same result, and the user has no history entry to look at to work out why.
Reproducing it
On a generated library, with one monitored book that a configured indexer can satisfy:
- Configure a download client pointing at a host and port that accept the TCP connection
and then never answer. A listener that accepts and sleeps is enough; blackholing the
port so the connect itself hangs works too.
- Trigger a search and grab for the book, either from the UI or by letting the automatic
search cycle reach it.
- Wait out the client's HTTP timeout.
- Check Activity and History. There is no entry for the attempt, and the queue has no row
either.
Contrast with a client that answers and refuses: that path takes the general arm and at
least surfaces DownloadClientSubmissionException to the caller.
The branch
I have a branch ready: fix/submission-timeout-history, one commit
on top of a630572.
- Threads a
CancellationToken through StartDownloadAsync and both
SendToDownloadClientAsync overloads into AddAsync, defaulting to default so no
existing caller has to change.
AutomaticSearchService.cs:308 passes stoppingToken, which is the caller that actually
holds a shutdown token. The API controller
(listenarr.api/Features/Downloads/DownloadController.cs:90) keeps the default, since no
action in that controller binds a token today and making a browser disconnect cancel a
submission is a separate decision.
- Narrows the cancel arm to
catch (OperationCanceledException) when (ct.IsCancellationRequested),
which is the same filter AutomaticSearchService.cs:114 already uses around its own
cycle, and drops OperationCanceledException from the general arm's exclusion list so
everything else falls through to it.
- No change to
IDownloadClientGateway; it already took an optional token on every method.
DownloadService.cs stays at 498 lines.
Two tests, both in
tests/Features/Application/Downloads/Submission/DownloadServiceTests.cs:
- the gateway throws
TaskCanceledException while nothing has requested cancellation, and
the call must surface DownloadClientSubmissionException wrapping it;
- the gateway throws a cancellation carrying a token that genuinely is cancelled, and the
call must still propagate it untouched, with no failure recorded.
Reverting the catch narrowing fails the first and leaves the second passing. Full backend
suite green on the branch.
How this composes with #882
On canary alone the observable change is narrow: a timeout takes the general arm instead of
the cancel arm, so the caller gets DownloadClientSubmissionException rather than a bare
TaskCanceledException, and the API returns 502 rather than an unhandled cancellation. No
history row is written either way, because canary writes nothing on the failure path at
all. That is why the test asserts on the exception type rather than on a history entry.
#882 is what makes the general arm write a DownloadFailed event. With both in, a timed
out grab reaches that arm and does get a history row, which is the outcome worth having.
Its commit message says cancellation is left alone because a shutdown is not a release
failure, and I agree with that as stated; the point here is only that today the code cannot
tell a shutdown from a timeout, because our own token never reaches the call.
The two branches touch the same two catch arms in the same method, so they will conflict
textually. #882 first is cleaner: it moves RemoveProvisionalDownloadAsync out into
DownloadSubmissionFailureHandler and changes both arms, and this branch is then a two
line edit to the filters on top of that shape. The reverse order makes #882 re-resolve the
same hunks. #918 and #943 also touch DownloadService.cs but in other methods.
Open question
Whether the API path should pass HttpContext.RequestAborted. It would make a browser that
navigates away mid-submission count as a real cancellation rather than a failure, which
seems right, but it is a behaviour change on an endpoint that binds no token today and it
is not needed for the defect above, so the branch leaves it out.
Disclosure: drafted with Claude Code at my direction; I read the cited code at the stated commit and reviewed this before posting.
What happens
DownloadService.SendToDownloadClientAsynchands the prepared submission to the gatewaywith no cancellation token:
listenarr.application/Downloads/Submission/DownloadService.cs:337submissionResult = await clientGateway.AddAsync(downloadClient, prepared);IDownloadClientGateway.AddAsyncdeclaresCancellationToken ct = default(
listenarr.application/Downloads/Contracts/IDownloadClientGateway.cs:29-32), so the tokenin play is
Noneand the method has no way to ask whether anyone actually requested acancellation.
The next two arms are:
DownloadService.cs:343-347,catch (OperationCanceledException), which removes theprovisional
Downloadrow and rethrows.DownloadService.cs:348-358, the general arm, which removes the row and wraps the causein
DownloadClientSubmissionException.An
HttpClientrequest timeout throwsTaskCanceledException, and that derives fromOperationCanceledException, so it takes the first arm. History is only written after theclient has accepted (
DownloadService.cs:368-374), and the provisional row is gone, so agrab against a download client that is down, unreachable or wedged ends with nothing
written anywhere. The caller sees a raw
TaskCanceledException; the UI shows the releasesimply not appearing.
I think the cost is that the next automatic search picks the same release again, with the
same result, and the user has no history entry to look at to work out why.
Reproducing it
On a generated library, with one monitored book that a configured indexer can satisfy:
and then never answer. A listener that accepts and sleeps is enough; blackholing the
port so the connect itself hangs works too.
search cycle reach it.
either.
Contrast with a client that answers and refuses: that path takes the general arm and at
least surfaces
DownloadClientSubmissionExceptionto the caller.The branch
I have a branch ready:
fix/submission-timeout-history, one commiton top of a630572.
CancellationTokenthroughStartDownloadAsyncand bothSendToDownloadClientAsyncoverloads intoAddAsync, defaulting todefaultso noexisting caller has to change.
AutomaticSearchService.cs:308passesstoppingToken, which is the caller that actuallyholds a shutdown token. The API controller
(
listenarr.api/Features/Downloads/DownloadController.cs:90) keeps the default, since noaction in that controller binds a token today and making a browser disconnect cancel a
submission is a separate decision.
catch (OperationCanceledException) when (ct.IsCancellationRequested),which is the same filter
AutomaticSearchService.cs:114already uses around its owncycle, and drops
OperationCanceledExceptionfrom the general arm's exclusion list soeverything else falls through to it.
IDownloadClientGateway; it already took an optional token on every method.DownloadService.csstays at 498 lines.Two tests, both in
tests/Features/Application/Downloads/Submission/DownloadServiceTests.cs:TaskCanceledExceptionwhile nothing has requested cancellation, andthe call must surface
DownloadClientSubmissionExceptionwrapping it;call must still propagate it untouched, with no failure recorded.
Reverting the catch narrowing fails the first and leaves the second passing. Full backend
suite green on the branch.
How this composes with #882
On canary alone the observable change is narrow: a timeout takes the general arm instead of
the cancel arm, so the caller gets
DownloadClientSubmissionExceptionrather than a bareTaskCanceledException, and the API returns 502 rather than an unhandled cancellation. Nohistory row is written either way, because canary writes nothing on the failure path at
all. That is why the test asserts on the exception type rather than on a history entry.
#882 is what makes the general arm write a
DownloadFailedevent. With both in, a timedout grab reaches that arm and does get a history row, which is the outcome worth having.
Its commit message says cancellation is left alone because a shutdown is not a release
failure, and I agree with that as stated; the point here is only that today the code cannot
tell a shutdown from a timeout, because our own token never reaches the call.
The two branches touch the same two catch arms in the same method, so they will conflict
textually. #882 first is cleaner: it moves
RemoveProvisionalDownloadAsyncout intoDownloadSubmissionFailureHandlerand changes both arms, and this branch is then a twoline edit to the filters on top of that shape. The reverse order makes #882 re-resolve the
same hunks. #918 and #943 also touch
DownloadService.csbut in other methods.Open question
Whether the API path should pass
HttpContext.RequestAborted. It would make a browser thatnavigates away mid-submission count as a real cancellation rather than a failure, which
seems right, but it is a behaviour change on an endpoint that binds no token today and it
is not needed for the defect above, so the branch leaves it out.
Disclosure: drafted with Claude Code at my direction; I read the cited code at the stated commit and reviewed this before posting.