fix(task): propagate interruptions to active subagents - #4108
fix(task): propagate interruptions to active subagents#4108bozhnyukAlex wants to merge 3 commits into
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
[Automatic Post]: I have assigned @enyst as a reviewer based on git blame information. Thanks in advance for the help! This comment was created by an AI agent (OpenHands) on behalf of the user. |
VascoSch92
left a comment
VascoSch92
left a comment
There was a problem hiding this comment.
Went through this carefully and the bug is definitely real. I ran the repro from #4107 on both branches and got call_count 0 on main vs 1 here, and your new test_parent_interrupt_stops_active_subagent fails on main and passes on this branch. Task suite is green for me too (68/68).
One thing I want to highlight because it's easy to miss on a skim: swapping run() for arun() isn't cosmetic, it's the part that actually makes this work. LocalConversation.interrupt() only cancels _arun_task, and that's only ever set by arun(). If the child had stayed on sync run() you'd have fallen through to pause(), which doesn't take effect until between iterations, so it would never cancel an in-flight LLM call. Nice catch.
A few comments inline, none of them blocking.
Also worth recording two things I went looking for and didn't find: nesting asyncio.run() inside a running loop isn't a problem here, since tool calls always get dispatched to a worker thread via run_in_executor; and I wondered about _tasks_lock contention while _create_task holds it, but it only measures 1-3ms because tools initialize lazily outside the lock.
Happy to approve once the description picks up the sync-parent and acompletion points.
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
VascoSch92
left a comment
VascoSch92
left a comment
There was a problem hiding this comment.
One thing I want to confirm before approving: routing subagents through arun() means they now always use LLM.acompletion(), even when the parent runs synchronously. Since acompletion() is an independent litellm path and does not delegate to completion(), any custom LLM subclass that overrides only completion() will have that override silently bypassed for subagents.
It's documented in the Notes, but should we also surface it in the changelog / release notes so downstream users with custom sync-only LLMs aren't caught out? @enyst
Otherwise LGTM
|
[Automatic Post]: It has been a while since there was any activity on this PR. @bozhnyukAlex, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
Still active and ready to merge. All review comments have been addressed, the review threads are resolved, and CI is green. |
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).
🔴 Needs improvement - The direction is right, but the cancellation handoff still loses an interrupt in one real race window and the async conversion breaks an existing customization contract.
[CRITICAL ISSUES]
- [
openhands-tools/openhands/tools/task/manager.py, Line 379] Lost interrupt between the generation check andarun()registration:_was_interrupted()is checked beforesend_message()and_run_until_finished(). IfTaskManager.interrupt()lands after that check but beforeLocalConversation.arun()sets_arun_taskand_cancel_token,conversation.interrupt()falls back topause().arun()then treatsPAUSEDas resumable and sets the child back toRUNNING, so the interrupt this PR is meant to make durable is lost. Close the handoff by registering the async run before the final generation check (or otherwise carrying a durable cancellation token into startup), and add a deterministic barrier test for this exact interval; the current tests cover interruption before_run_task()and after the child LLM call starts, but not between them. - [
openhands-tools/openhands/tools/task/manager.py, Line 430] Breaking change for sync-only custom LLM subclasses: every task subagent now goes througharun()/LLM.acompletion(). The inheritedacompletion()path calls the async LiteLLM transport directly; it does not delegate to a subclass's overriddencompletion(). Existing custom task agents that intentionally implement only the long-standing synchronous override will now bypass their implementation and may fail or call an unintended provider. The PR description acknowledges the new requirement, but documentation does not make a userspace break acceptable. Preserve the old override contract through a compatibility fallback/adapter or a deprecation path, and add a regression test with anLLMsubclass that overrides onlycompletion().
[TESTING GAPS]
- [Overall PR] Eval-sensitive task behavior: this changes tool execution and subagent orchestration, and I found no completed eval-monitor run plus human confirmation. Per this repository's review guide, a human maintainer should decide after lightweight eval coverage once the correctness issues above are fixed.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🔴 HIGH
This touches core subagent orchestration and changes which public LLM method downstream customizations execute. The remaining startup race can leak model work after an interrupt, while the compatibility break can fail existing custom agents. Recommendation: Do not auto-merge. Request review from a human SDK architect and validate task-tool behavior with lightweight evals after fixing the startup handoff and sync-only LLM compatibility.
VERDICT:
❌ Needs rework: The current implementation does not make startup cancellation fully durable and breaks existing custom LLM subclasses.
KEY INSIGHT:
Cancellation is only reliable when ownership transfers atomically from the manager's generation token to the child's registered async task.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Review: fix(task): propagate interruptions to active subagents
This PR addresses a real bug - parent interrupts were not reaching active subagents because TaskExecutor inherited the no-op ToolExecutor.interrupt(). The approach (delegate to TaskManager, run subagents through arun() so CancelledError can cancel in-flight LLM calls, and track interrupt generations for the pre-startup race) is sound and the test coverage is solid.
This review was created by an AI agent (OpenHands) on behalf of the repository reviewer.
Findings
1. Startup race window between the generation check and arun() registration (MEDIUM)
The generation check at _run_task line 379 closes the "interrupt before _run_task" race, but there is still a gap between that check and the point where LocalConversation.arun() sets _arun_task / _cancel_token (local_conversation.py lines 1916-1917). During this window, send_message() (line 384) and _ensure_agent_ready() (run via asyncio.to_thread inside arun(), which loads plugins and initializes the agent) execute. If TaskManager.interrupt() lands in this interval, conversation.interrupt() finds _arun_task is None and falls back to pause(), setting the subagent to PAUSED. When arun() then starts, its status normalization block resets PAUSED to RUNNING, and the interrupt is silently lost - the subagent proceeds to make model calls.
The existing tests cover interruption before _create_task completes (test_interrupt_before_registration_prevents_subagent_run) and after the child LLM call starts (test_parent_interrupt_stops_active_subagent), but not this intermediate window. A deterministic barrier test for the interval between the generation check and arun() registration would close the coverage gap. One possible fix: re-check _was_interrupted() inside _arun_until_finished right before (or immediately after) the first await conversation.arun(), or pre-register a cancellation token on the subagent conversation before entering arun().
2. arun() switch bypasses sync-only custom LLM subclasses (LOW-MEDIUM)
Subagents now go through arun() then Agent.astep() then amake_llm_completion() then LLM.acompletion(). The base LLM.acompletion() is a fully independent async implementation (litellm acompletion transport) and does not delegate to a subclass's overridden completion(). A custom LLM subclass that overrides only completion() - a previously valid pattern for task subagents since they used run() then step() then make_llm_completion() then completion() - will now have that override silently bypassed. TestLLM is unaffected because it overrides both methods (with acompletion() delegating to completion()). The PR description acknowledges this in the Notes section, which is good, but since the PR is classified as a non-breaking bug fix, consider whether a deprecation note or compatibility adapter is warranted for downstream users with sync-only custom LLMs.
3. close() calls interrupt() but does not evict conversations (LOW)
close() (line 497) now calls self.interrupt() before clearing _tasks. interrupt() schedules cancellation on subagent event loops via call_soon_threadsafe, but close() immediately proceeds to clear the _tasks dict without calling _evict_task() (which would pause() + close() each conversation). If a subagent's asyncio.run() is still in-flight when _tasks.clear() runs, the worker thread's _run_task finally block may attempt to evict a task that was already removed from the dict. The consequences are minor (at worst a logged warning), and this behavior is pre-existing for the non-interrupt path, but it is worth noting that interrupt() during close() is fire-and-forget - the subagent conversations are not guaranteed to be cleanly shut down before the manager is destroyed.
Positive aspects
- The interrupt generation counter is a clean design for the pre-startup race.
- Copying the active task snapshot outside the lock (lines 481-487) correctly avoids holding
_tasks_lockduringconversation.interrupt()callbacks. InterruptibleSubagentLLMin the end-to-end test is a well-constructed fixture that verifies actualCancelledErrorpropagation through the async LLM path.- Exception handling in
interrupt()(lines 489-493) prevents one failing subagent interrupt from blocking others.
Risk Assessment: MEDIUM
The PR correctly fixes a real bug and the core cancellation path (interrupt then call_soon_threadsafe(task.cancel) then CancelledError then PAUSED) works reliably once arun() has registered. The startup race window (Finding 1) is the primary concern - it is narrow but non-trivial for fresh subagents where _ensure_agent_ready performs plugin/agent initialization. The sync-only LLM compatibility change (Finding 2) is acknowledged but could surprise downstream users. Neither is blocking for merging as a bug fix, but both should be tracked.
| ) -> None: | ||
| """Run a sub-agent conversation to completion, handling confirmations.""" | ||
| conversation.run() | ||
| asyncio.run(self._arun_until_finished(task_id, conversation)) |
There was a problem hiding this comment.
Startup race window: The generation check at line 379 passes, then send_message() (384) and _run_until_finished() execute before LocalConversation.arun() sets _arun_task and _cancel_token (local_conversation.py:1916-1917). If interrupt() lands in this interval, conversation.interrupt() finds _arun_task is None and falls back to pause() then PAUSED. arun() then normalizes PAUSED to RUNNING (local_conversation.py:1948-1953), silently losing the interrupt. The subagent proceeds to make model calls despite the parent being interrupted.
The generation token closes the pre-_run_task race but not this one. Consider re-checking _was_interrupted(interrupt_generation) inside _arun_until_finished right before the first await conversation.arun(), or pre-registering a cancellation token before entering arun(). A deterministic barrier test for this interval would also close the coverage gap - the existing tests cover before-_create_task and after-LLM-start, but not the window between the generation check and arun() registration.
| self, task_id: str, conversation: LocalConversation | ||
| ) -> None: | ||
| """Run a sub-agent asynchronously so in-flight work can be interrupted.""" | ||
| await conversation.arun() |
There was a problem hiding this comment.
Sync-only LLM compatibility: Switching to arun() routes subagent LLM calls through Agent.astep() then amake_llm_completion() then LLM.acompletion(). The base LLM.acompletion() is an independent async implementation (litellm acompletion) and does not delegate to a subclass's overridden completion(). Custom LLM subclasses that override only completion() - valid before since subagents used run() then step() then completion() - will now have that override silently bypassed for task subagents. TestLLM is unaffected (it overrides both, with acompletion() delegating to completion()). The PR description acknowledges this, but since the change is classified as non-breaking, consider whether a compatibility adapter or deprecation note is warranted for downstream sync-only custom LLMs.
4 participants
HUMAN:
I reviewed the implementation. Need this thing for production use.
AGENT:
Why
Interrupting a parent
LocalConversationwhile itstasktool was running only paused the parent.TaskExecutorinherited the no-opToolExecutor.interrupt(), so the active subagent continued consuming iterations and model calls.Summary
TaskExecutor.interrupt()toTaskManager.arun()so an in-flight model request can be cancelled.Issue Number
Closes #4107.
How to Test
uv run pre-commit run --all-files --show-diff-on-failureuv run pytest tests/tools/task -quv run pytest tests/agent_server/test_openapi_discriminator.py tests/sdk/context/test_agent_context_serialization.py tests/sdk/conversation/local/test_conversation_core.py tests/sdk/conversation/local/test_state_serialization.py tests/tools/file_editor/test_memory_usage.py -quv run pytest tests/tools/terminal -q(311 passed; two environment-contaminated cases passed when rerun in isolation after cleaning a stale tmux server)End-to-end evidence:
test_parent_interrupt_stops_active_subagentstarts a real parent conversation withTaskToolSet, waits for a blocking subagent LLM call, interrupts the parent, and verifies both cancellation of the child request and the parent'sPAUSEDstate.Video/Screenshots
Not applicable; this changes runtime cancellation behavior and is covered by an end-to-end test.
Type of Change
Notes
The synchronous
TaskManager.start_task()contract is unchanged. Only the internal subagent run loop is asynchronous so SDK interruption can cancel in-flight work.Cancellation propagation applies when the parent conversation is driven through
LocalConversation.arun(). Parents using synchronousrun()retain cooperative pause semantics and cannot interrupt an in-flight subagent call.Task subagents now run through
arun()and therefore use the separateLLM.acompletion()path instead ofLLM.completion(). Custom LLM subclasses used by task subagents must support that asynchronous method.