fix: stop the download queue from deadlocking after failed jobs - #271
Merged
Conversation
`consume()` re-raised on error when `--ignore-errors` was not given. That does not just abort the job, it leaves the `while True` loop and kills the consumer task. `task_done()` still ran for the current item, but the consumer never took another one. Once every `--jobs` consumer had died this way, `await QUEUE.join()` waited forever for items nobody would pick up. With the default `-j 3` three early failures were enough; with `-j 1` a single one. The failure was also invisible: the exception sat in a dead task, and the `asyncio.gather(..., return_exceptions=True)` that would have collected it only runs after `QUEUE.join()` returns, which in this state never happens. Keep the consumer alive and move the abort decision into a `DownloadRun` object shared by all consumers. On the first failure it sets an event; consumers then stop starting jobs and only drain what is left, so `QUEUE.join()` can return. Downloads already in flight are allowed to finish rather than being cancelled mid-write. Waiting on `QUEUE.join()` alone was the deeper flaw: any consumer that ends early strands the queue. `drain_queue()` now waits on the join and the consumers together, so a worker that dies for any reason is noticed and reported instead of hanging the run. `--jobs` also rejects values below `1`, which previously queued work that no consumer would ever pick up. This also makes `--ignore-errors` mean what its help text says. Before, the flag's absence did not abort anything: one consumer died while the others carried on downloading until they died too. Finally, a run in which a job raised now ends in `AudibleCliException`, which `cli.main()` maps to exit code 2. That holds with `--ignore-errors` as well, so those failures are visible to scripts instead of being reported as success. Failures that are only logged, such as an unknown ASIN or a download rejected by its HTTP status, are not covered yet and still exit zero. Closes #235 Closes #239
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
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.
Closes #235, closes #239. Takes a first step on #256 — see the caveat below.
The bug
consume()re-raised on error when--ignore-errorswas not given:That does not just abort the job, it kills the consumer task.
task_done()still ran for the current item, but the consumer never took another one. Once every--jobsconsumer had died this way,await QUEUE.join()waited forever for items nobody would pick up.Reproduced against the real
consume()/QUEUE:-j 3, 3 failures first, then 5 good jobs-j 1, 1 failure, then 2 good jobs-j 3, 1 failure--ignore-errorsThe default is
-j 3, so three early failures were enough. That matches #235 ("a lot of Skip download and then suddenly audible hangs") and its offshoot #239.The failure was invisible too: the exception sat in a dead task, and the
asyncio.gather(..., return_exceptions=True)that would have collected it only runs afterQUEUE.join()returns — which in this state never happens.The fix
DownloadRunholds the abort event, the collected errors and the skipped count, shared by all consumers. The consumer never leaves its loop: on failure it records the error, and if--ignore-errorsis not set it sets the abort event. Consumers then stop starting jobs and only acknowledge what is left, soQUEUE.join()can return. Downloads already in flight finish rather than being cancelled mid-write.Waiting on
QUEUE.join()alone was the deeper flaw — any consumer ending early strands the queue.drain_queue()now waits on the join and the consumers together, so a worker that dies for any reason is reported instead of hanging the run.--jobsalso rejects values below1;-j 0previously queued work that no consumer would ever pick up.This also makes
--ignore-errorsmean what its help text says. Before, its absence aborted nothing: one consumer died while the others kept downloading until they died too.Exit code
A run in which a job raised now ends in
AudibleCliException, whichcli.main()maps to exit code 2 — with--ignore-errorsas well.DownloadRunand still exit zero: an unknown ASIN or title, a cover with no URL, and downloads that return a non-successStatus(DownloadError,DownloadErrorStatusCode,DownloadSizeMismatch,DownloadContentTypeMismatch). Capturing those means threading the run state through every download function and separating real failures from legitimateDestinationAlreadyExistsskips — worth its own PR, so #256 stays open. The changelog entry is worded to match what actually holds.Relation to #257
#257 also targets #256 and overlaps in
consume(), error tracking and the command epilogue, so the two should not be merged independently. It keeps the re-raise and therefore the deadlock, and itsreturn 1from the Click callback does not become the process exit status — Click discards callback return values in standalone mode. Its useful part is the additional error capture points, which are exactly what the #256 follow-up needs.Verification
A harness driving the real
consume()/QUEUE/drain_queue()— not a reimplementation:--ignore-errorsruns everything and only counts the failures-j 0and-j -1are rejected by ClickAudibleCliException→ exit 2 andRuntimeError→ exit 3 confirmed throughcli.main()ruff check src plugin_cmdsshows no new findings;cli's pre-existing C901 complexity drops from 32 to 31 because the draining moved out of it.No tests were committed — the repo still has no pytest dependency and no CI job to run them. That gap is tracked separately.