Skip to content

fix(session queue): count only rows actually transitioned by a bulk cancel - #9387

Merged
lstein merged 4 commits into
invoke-ai:mainfrom
lstein:lstein/fix/multigpu-bulk-cancel-count
Aug 12, 2026
Merged

fix(session queue): count only rows actually transitioned by a bulk cancel#9387
lstein merged 4 commits into
invoke-ai:mainfrom
lstein:lstein/fix/multigpu-bulk-cancel-count

Conversation

@lstein

@lstein lstein commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #9263, addressing the duplicate bulk-cancel counting deferred in @JPPhoto's 2026-07-25 review:

_cancel_in_progress_matching() counts an item whenever _set_queue_item_status() returns an object whose status is canceled. Two concurrent bulk cancellations can both select the same in-progress row; the first transitions it, while the second receives the already-terminal canceled row and counts it again despite making no transition.

  • The terminal guard is now part of the cancel UPDATE statement itself: _transition_queue_item_status() performs UPDATE ... WHERE item_id = ? AND status NOT IN ('completed','failed','canceled') and reports via cursor.rowcount whether THIS call performed the transition. _set_queue_item_status() is a thin wrapper preserving its existing contract.
  • _cancel_in_progress_matching() counts only actual transitions, so two racing bulk requests each report a disjoint set of canceled items.
  • Side benefit: the previous SELECT-then-UPDATE window in _set_queue_item_status (a row could turn terminal between the read and the write) is gone.

Test

test_concurrent_bulk_cancels_count_each_item_once uses a barrier to hold two cancel_by_batch_ids requests after their item-id SELECTs (both have selected the same two in-progress rows) and asserts the combined canceled count is exactly 2 — under the old code it was 4. Verified the test fails against the previous counting logic.

Merge order

Stacked on #9263 — this branch contains the multi-GPU branch's commits. Draft until #9263 merges; will then rebase onto main and mark ready for review.

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)
  • Updated What's New copy (if doing a release after this PR)

🤖 Generated with Claude Code

@github-actions github-actions Bot added api python PRs that change python files invocations PRs that change invocations backend PRs that change backend files services PRs that change app services frontend PRs that change frontend files python-tests PRs that change python tests docs PRs that change docs labels Jul 27, 2026
@lstein lstein added the 6.14.1 label Jul 28, 2026
@lstein lstein moved this to 6.14.1: Bug fixes to 6.14.0 in Invoke - Community Roadmap Jul 28, 2026
@lstein
lstein force-pushed the lstein/fix/multigpu-bulk-cancel-count branch from e9ee846 to 4e33017 Compare July 29, 2026 22:39
@lstein

lstein commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto the current #9263 head (272a2ef2ea). The rebase crossed #9355's single-transaction restructure of _set_queue_item_status, so the fix was re-implemented on the new shape: the terminal guard + UPDATE were already atomic in one transaction; what was missing was exposing WHO transitioned the row. _transition_queue_item_status now returns that bool and the bulk counter uses it. A fresh-context adversarial review of the resolution found no behavioral regressions across all seven callers; the docstring now records that the atomicity comes from SqliteDatabase's process-wide lock, not the SQL. 88 session-queue tests green, barrier test 15/15.

…ancel

Two concurrent bulk cancellations could select the same in-progress row;
the first canceled it and the second, seeing the already-terminal row
returned by _set_queue_item_status(), counted it again — both responses
reporting that they canceled the same item.

The terminal guard and the UPDATE already ran atomically (a single
transaction under SqliteDatabase's process-wide lock); what was missing
was exposing WHO performed the transition. _set_queue_item_status() now
delegates to _transition_queue_item_status(), which additionally returns
whether THIS call moved the row, and _cancel_in_progress_matching()
counts only actual transitions — so exactly one racing caller observes
itself as the canceller. The docstring records that the atomicity comes
from the database lock, not the SQL, so a future move to per-thread
connections doesn't silently reintroduce the double count.

Includes a two-thread barrier test (in-memory SQLite) asserting each
item is counted exactly once across concurrent bulk cancels; the
pre-fix counter reports 4 for 2 items under the same interleaving.

Follow-up to PR invoke-ai#9263 (JPPhoto review, 2026-07-25).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@lstein
lstein force-pushed the lstein/fix/multigpu-bulk-cancel-count branch from 4e33017 to 61a7de7 Compare July 30, 2026 01:17
@lstein
lstein marked this pull request as ready for review July 30, 2026 01:42

@JPPhoto JPPhoto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved!

You might want to consider guarded SQL UPDATE plus cursor.rowcount; the current guarantee relies on shared SqliteDatabase._lock.

@lstein
lstein enabled auto-merge (squash) August 12, 2026 17:40
@lstein
lstein merged commit 5d00f03 into invoke-ai:main Aug 12, 2026
17 checks passed
@lstein
lstein deleted the lstein/fix/multigpu-bulk-cancel-count branch August 12, 2026 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.14.1 api backend PRs that change backend files docs PRs that change docs frontend PRs that change frontend files invocations PRs that change invocations python PRs that change python files python-tests PRs that change python tests services PRs that change app services

Projects

Status: 6.14.1: Bug fixes to 6.14.0

Development

Successfully merging this pull request may close these issues.

2 participants