From e27fed1d40354d5b15de2f28e5679aed1b601bd6 Mon Sep 17 00:00:00 2001 From: splitsec2 <35583321+splitsec2@users.noreply.github.com> Date: Thu, 22 Oct 2026 18:19:16 -0600 Subject: [PATCH] fix(queue): a late progress update no longer reopens a finished task (#1538) #1429 made the first terminal status win, but the guard only refused terminal to terminal changes. A worker's poll that reports DOWNLOADING after a cancel still overwrote CANCELLED (the poll loop calls the status callback before it waits on the cancel flag), and the COMPLETE or ERROR that followed was then accepted as a first terminal status, so the terminal hook ran a second time. This refuses any status change away from a terminal status. `add()` and `enqueue_existing()` reset the status themselves, so retries are unaffected. The new test cancels a downloading task, sends DOWNLOADING and then COMPLETE, and expects CANCELLED and one hook call. It fails on main and passes here. Full non-e2e suite passes, ruff and basedpyright are clean. Found in a Claude pass over 1.5.0, same as my other small fixes from it. --- shelfmark/core/queue.py | 11 +++++------ tests/core/test_queue.py | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/shelfmark/core/queue.py b/shelfmark/core/queue.py index 99a69b75..1df40937 100644 --- a/shelfmark/core/queue.py +++ b/shelfmark/core/queue.py @@ -168,14 +168,13 @@ class BookQueue: hook: Callable[[str, QueueStatus, DownloadTask], None] | None = None hook_task: DownloadTask | None = None previous_status = self._status.get(book_id) - if ( - previous_status in TERMINAL_QUEUE_STATUSES - and status in TERMINAL_QUEUE_STATUSES - and previous_status != status - ): + if previous_status in TERMINAL_QUEUE_STATUSES and previous_status != status: # The first terminal status wins. A worker that checked its cancel flag just before # a cancel landed would otherwise overwrite CANCELLED with COMPLETE and fire the - # terminal hook a second time. + # terminal hook a second time. A late progress update (DOWNLOADING) is refused for + # the same reason: it would reopen the task and let the next terminal status + # through. A retry goes through add() or enqueue_existing(), which reset the + # status themselves. return None, None self._update_status(book_id, status) diff --git a/tests/core/test_queue.py b/tests/core/test_queue.py index 8ef9c661..1deaf4f4 100644 --- a/tests/core/test_queue.py +++ b/tests/core/test_queue.py @@ -88,6 +88,23 @@ def test_a_late_complete_does_not_overwrite_a_cancel(): assert terminal_events == [QueueStatus.CANCELLED] +def test_a_late_progress_update_does_not_reopen_a_cancelled_task(): + """A poll that reports DOWNLOADING after the cancel must not reopen the task. + + Without this the next COMPLETE or ERROR is accepted as a first terminal status and + the terminal hook fires a second time. + """ + queue = BookQueue() + terminal_events = _downloading_task(queue, "race-task") + + assert queue.cancel_download("race-task") is True + queue.update_status("race-task", QueueStatus.DOWNLOADING) + queue.update_status("race-task", QueueStatus.COMPLETE) + + assert queue.get_task_status("race-task") == QueueStatus.CANCELLED + assert terminal_events == [QueueStatus.CANCELLED] + + def test_a_task_that_ended_in_error_is_not_completed_afterwards(): queue = BookQueue() terminal_events = _downloading_task(queue, "race-task")