fix: release conversation subject in cancel terminal update - #490
onatozmenn wants to merge 2 commits into
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
1 similar comment
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
There was a problem hiding this comment.
🟡 Changes recommended
The regression test does not exercise an actual resubmission after cancellation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request fixes cancellation so conversation subjects are released atomically, including runs without sandboxes.
Changes:
- Updates cancellation to set
subject_released_at. - Removes redundant post-update release logic.
- Adds cancellation regression tests.
File summaries
| File | Description |
|---|---|
tests/test_cancel_run.py |
Tests subject release and ordinary cancellations. |
openhands/automation/router.py |
Atomically releases subjects during cancellation. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| async def test_cancelled_subject_no_longer_blocks_resubmission( | ||
| async_client, async_session | ||
| ): | ||
| """After cancel, no unreleased run holds the subject, so a resubmission | ||
| routes to a new run instead of deduplicating against the cancelled one.""" |
Co-authored-by: openhands <openhands@all-hands.dev>
|
Good catch — the resubmission test now goes through the real routing path: it cancels a queued subject run, then calls |
HUMAN:
I ran the targeted pytest suite and confirmed all 109 tests pass, including the two new subject-release cases that fail on base and pass with the fix, with ruff checks also clean.
Why
Cancelling a run that holds a conversation subject only released it when the run had recorded a
sandbox_id(and in a second write after the terminal update). Local-mode and conversation-turn child runs often have nosandbox_id, sosubject_released_atstayed unset and a resubmitted subject kept deduplicating against the cancelled run instead of creating a replacement (#472).Summary
subject_released_atatomically in the cancel terminalUPDATE(conditional onsubject_key), independent ofsandbox_id.continue_conversation.Issue Number
Fixes #472
How to Test
python -m pytest tests/test_cancel_run.py tests/test_conversations.py tests/test_watchdog.py -q— 109 passed. The two new subject tests fail on base and pass with the fix.ruff check/ruff format --checkclean.Type