Skip to content

fix(queue): first terminal status wins, so a late COMPLETE cannot overwrite a cancel - #1429

Open
splitsec2 wants to merge 1 commit into
calibrain:mainfrom
splitsec2:fix/terminal-status-race
Open

splitsec2 wants to merge 1 commit into
calibrain:mainfrom
splitsec2:fix/terminal-status-race

Conversation

@splitsec2

@splitsec2 splitsec2 commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Kept small on purpose so it's easy to review. I have a few more small fixes like this from going back over recently merged PRs. Would you rather get them as separate PRs like this one, or combined into one?

#1361 made cancel_download write under the queue lock, which stops a cancel from overwriting a download that finished first. The other direction is still open. The worker checks cancel_flag.is_set() and then calls update_status(COMPLETE) a few lines later in _process_single_download. If a cancel lands between those two lines the task goes CANCELLED, then the worker writes COMPLETE over it. _apply_status_locked allows terminal to terminal moves, so the terminal hook also fires a second time and records two events for one task.

This makes the first terminal status win: _apply_status_locked ignores a different terminal status once the task already has one. Requeueing from a terminal status still works because QUEUED isn't terminal.

I replaced the existing race test. It patched queue.update_status, which cancel_download no longer calls, so it never exercised the path it names, and its last assertion only looked at the final hook event. The new tests call the queue directly: complete then cancel is refused, cancel then complete stays cancelled with one hook event, error then complete stays error, and a terminal task can still be requeued. The two late-write tests fail on current main.

Ruff and basedpyright are clean. tests/core and tests/download pass apart from 4 seleniumbase import failures in my environment that also fail on main.

The one red check is the existing clock-dependent test test_a_queued_torrent_asks_for_a_grace_once, which fails on any runner that booted recently. It is fixed in #1426 and is not caused by this change.

@splitsec2

Copy link
Copy Markdown
Contributor Author

@calibrain unrelated to this PR: could you turn on private vulnerability reporting for the repo (Settings, Code security, Private vulnerability reporting)? It's off right now and there's no SECURITY.md, and I have a security issue to report that shouldn't go in a public issue. I'll file it there once it's on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant