Skip to content

FIX Drain nested scoring and batch tasks on failure - #2856

Merged
Roman Lutz (romanlutz) merged 8 commits into
microsoft:mainfrom
biefan:fix/concurrent-scoring-lifetimes
Oct 8, 2026
Merged

Roman Lutz (romanlutz) merged 8 commits into
microsoft:mainfrom
biefan:fix/concurrent-scoring-lifetimes

Conversation

@biefan

@biefan biefan (biefan) commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Description

A failed scoring call can return while nested scorers or other items in its batch still run. This allows old work to overlap a retry or persist scores after the caller has reported failure.

This change shares task cleanup across the existing response-level protection and the remaining concurrency boundaries:

Entry point Work drained before failure returns
Scorer.score_with_scorers_async() Independently persisted scoring roots
TrueFalseCompositeScorer Nested child scorers
Multi-piece MessageScorer Piece-level scoring tasks
score_batch_async() / batch_task_async() Other items in the current batch

The helper shields the initial gather and explicitly owns cancellation. It cancels unfinished children only if they are not already processing cancellation, then shields and drains both the original gather and every child. A slow asynchronous cleanup therefore receives only one cancellation request, even if the caller cancels repeatedly. Further caller cancellation propagates after cleanup; sibling cleanup errors do not replace the primary failure.

Waiting for the original gather also matters when children finish in the same event-loop turn as caller cancellation: their completion callbacks may still be queued. Reading the original gather's exception before those callbacks run raises InvalidStateError and replaces the intended CancelledError. The shared drain now waits for and consumes that result safely.

Successful result ordering, completed score persistence, batch sizes, and rate-limit validation are preserved. Later batches do not start after a failure. Completed requests and scores are not rolled back.

The branch includes current main and the SQLite cancellation cleanup from #2982. That fix is required when cancelling a score-validation SELECT leaves a native cursor holding a writer lock and the attack subsequently tries to persist its error result.

Tests and Documentation

  • SQLite-backed public-API regressions cover four scoring entry points with ordinary failure and child cancellation. They verify cleanup before return, no late scoring, and no late persistence using a filtered get_scores_async(score_type="true_false", include_intermediate=True) query.
  • Event-controlled batch tests cover caller and child cancellation, slow cleanup, repeated cancellation, no remaining children, and no later batch. Two additional cases cover children completing or failing immediately before caller cancellation; both reproduced InvalidStateError before the fix and now preserve CancelledError.
  • The deterministic attack regression from FIX SQLite cancellation cleanup #2982 reproduced database table is locked on the previous PR head, bb29c8e, in a separate checkout. It passes with the updated branch, preserving the original scorer failure and persisting the error result.
  • Targeted SQLite, scoring transport, batch, and nested-scoring tests: 110 passed on Python 3.13.12 with all optional dependencies installed.
  • Full make unit-test run on Python 3.13.12 with all optional dependencies: 24,346 passed, 11 skipped, 0 failed.
  • All applicable pre-commit checks for the PR's files passed, including repository-wide ty check pyrit with all optional dependencies installed.
uv run --extra all pytest -q tests/unit/memory/test_sqlite_cancellation.py tests/unit/executor/attack/test_scoring_expectation_transport.py tests/unit/prompt_target/test_batch_helper.py tests/unit/score/test_concurrent_scoring_lifetime.py --tb=short
make unit-test CMD='uv run --extra all -m'

The common helper and root/batch docstrings document the cleanup contract. No notebooks changed, and validation used mocked targets and local SQLite rather than live model endpoints or SQL Server.

Comment thread pyrit/common/task_utils.py Outdated
Comment thread tests/unit/score/test_concurrent_scoring_lifetime.py Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

# Conflicts:
#	pyrit/score/message_scorer.py
#	pyrit/score/true_false/true_false_composite_scorer.py
auto-merge was automatically disabled October 7, 2026 04:14

Head branch was pushed to by a user without write access

Roman Lutz (romanlutz) and others added 2 commits October 7, 2026 13:35
Update the branch for the required up-to-date check before merging PR microsoft#2856.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@romanlutz
Roman Lutz (romanlutz) merged commit 1e0cf53 into microsoft:main Oct 8, 2026
51 checks passed
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.

2 participants