Repository navigation
FIX: Start queued scenario run when handoff retry finds it already IN_PROGRESS - #4038
Muhammad Waqar Anwar (WaqarAnwar) wants to merge 8 commits into
Conversation
…_PROGRESS If the IN_PROGRESS update for a queued run committed but the call still raised, the handoff retry saw a non-QUEUED row and dropped the run, leaving it IN_PROGRESS with nothing executing it. The retry now starts a run whose row carries the start time this scheduler saved for it. Cancelling a queued run in that window now also cancels the IN_PROGRESS row. Closes microsoft#4036 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the scheduler retry-recovery and cancellation changes.
No open issues 🎉
What changed in this PR
Fixes #4036 by allowing the scenario scheduler to start a queued run when its earlier IN_PROGRESS update committed but reported a failure.
Changes:
- Recognizes a persisted start timestamp belonging to the current scheduler and launches the run without repeating the database update.
- Extracts task launch logic and allows cancellation during the handoff retry window.
- Adds mocked regression tests for retry recovery, mismatched start timestamps, and cancellation.
| File | Description |
|---|---|
tests/unit/backend/test_scenario_run_service.py |
Adds regression coverage for handoff retry recovery and cancellation. |
pyrit/backend/services/scenario_run_service.py |
Recovers committed starts on retry and handles cancellation before task launch. |
Review effort: Balanced
| return ( | ||
| run.started_at is not None | ||
| and persisted.scenario_run_state == ScenarioRunState.IN_PROGRESS | ||
| and persisted_started_at == run.started_at.isoformat() |
There was a problem hiding this comment.
raw == isoformat() misses when metadata comes back as a datetime (or Z vs +00:00). _load_started_at already handles that:
persisted_started_at: object = persisted.metadata.get(SCENARIO_RUN_STARTED_AT_METADATA_KEY)
return (
run.started_at is not None
and persisted.scenario_run_state == ScenarioRunState.IN_PROGRESS
- and persisted_started_at == run.started_at.isoformat()
+ and ScenarioRunService._load_started_at(scenario_result=persisted) == run.started_at
)There was a problem hiding this comment.
Good catch, thanks. The value is written by us via .isoformat() and comes back from JSON metadata, so it should round-trip as the same string in practice. But comparing strings is fragile, and reusing _load_started_at is cleaner. Switched to comparing the parsed datetime and removed the unused variable. I also added a test where the stored timestamp uses Z instead of +00:00.
…handoff-stuck-in-progress
Use _load_started_at so a saved start time written with a Z suffix or returned as a datetime still matches the run's start on handoff retry.
…aqarAnwar/PyRIT into fix/scenario-handoff-stuck-in-progress
Description
Closes #4036.
If the
IN_PROGRESSupdate for a queued scenario run commits but the call still raises, the handoff retry sees a non-QUEUEDrow and drops the run. The row staysIN_PROGRESSwith nothing executing it, and startup reconciliation only coversSQLiteMemory. This PR makes the retry start the run instead._handoff_scheduler_asyncnow recognizes anIN_PROGRESSrow whose saved start time matches the one this scheduler wrote for the run, and starts the run without writing to the database again. Other unexpected states are still dropped from the queue._launch_scheduled_run_lockedso the retry can start a run without repeating the state update.IN_PROGRESS. Before, the update did nothing, the cancel raisedValueError, and the run had already left the queue, so it stayed stuck. A queued run has no task yet, so there is nothing else to stop.The failure is simulated with a mocked database. It hasn't been observed in production or tried against a real Azure SQL connection failure.