fix(delegation): preserve completed child results on parent interrupt - #48893
fix(delegation): preserve completed child results on parent interrupt#48893andyvandaric wants to merge 1 commit into
Conversation
The batch delegation interrupt handler had a race condition where a child that completed between the last _cf_wait() return and the interrupt check (or during the fabrication loop iteration) could have its result discarded and replaced with a fabricated 'interrupted' entry. The old code iterated over pending futures calling f.done() point-in-time, but this check is inherently racy: a future can transition from not-done to done between the check and the else branch that fabricates an interrupted entry. Fix: replace the racy iteration with a 2-second grace window via wait(pending, timeout=2.0, return_when=ALL_COMPLETED). This cleanly separates futures into two sets — those that completed (including during the grace window) get their real results collected, and only genuinely still-running futures are marked as interrupted. This is a minimal, targeted fix. The broader 'non-blocking delegation' feature (issue NousResearch#47302, PR NousResearch#8482) addresses the larger UX problem of parent turns freezing during delegation; this fix ensures that when an interrupt does fire, already-computed work is not silently lost. Tested with 3 new test cases covering: - Fast child completes before interrupt, slow child interrupted - All children complete within grace window after interrupt - Child completing during the grace window is collected Refs: NousResearch#45496, NousResearch#47302
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating a real race in the synchronous batch aggregation path; current main still has the point-in-time f.done() branch at tools/delegate_tool.py:2596-2623.
Problems
- The added drain at
tools/delegate_tool.py:2378-2380waits up to two seconds for work that was not complete when interruption was observed. The new test explicitly treats a child completing at 1.5s after an interrupt at 0.3s as completed (tests/tools/test_delegate_batch_interrupt_race.py:247-249,270-271). That is a cancellation-policy change, not just preservation of already-computed output. - The first two timing tests do not reproduce the original TOCTOU: current main's
FIRST_COMPLETEDwait collects children that finish at 0.1s before their 0.3s/0.5s interrupt points (tools/delegate_tool.py:2629-2649;tests/tools/test_delegate_batch_interrupt_race.py:107-109,137-140).
Suggested changes
- Decide and document whether interruption may wait for post-interrupt completions; otherwise keep the interrupt path bounded while collecting only results known complete at its cutoff.
- Add a deterministic race regression test that controls the transition between the old completion observation and fabrication.
This is an automated hermes-sweeper review.
| from concurrent.futures import wait as _cf_wait_drain, ALL_COMPLETED | ||
|
|
||
| _drain_done, _drain_pending = _cf_wait_drain( | ||
| pending, timeout=2.0, return_when=ALL_COMPLETED |
There was a problem hiding this comment.
This drain waits for work that was still running when the parent interrupt was observed. That is broader than preserving already-computed results and changes the existing prompt-interrupt behavior into a two-second grace policy. Please either make that policy explicit and intentional, or collect only work known complete at the interrupt cutoff.
|
|
||
| # Child 0: completes at 0.1s | ||
| child_0 = _make_mock_child(response="Child 0 done", delay=0.1) | ||
| # Child 1: completes at 1.5s — within the 2s grace window |
There was a problem hiding this comment.
This child completes 1.2 seconds after the test sets the interrupt, so the assertion validates the new grace-window policy rather than the original f.done() TOCTOU. Please add a deterministic test that controls the completion transition between the old observation and fabricated-result path.
What does this PR do?
Fixes a race condition in the batch delegation interrupt handler where a child that completes between the last
_cf_wait()return and the_interrupt_requestedcheck has its result discarded — replaced with a fabricated "interrupted" entry even though the work was already done.In the batch polling loop (
delegate_tool.py, ~line 2370), when_interrupt_requestedfires, the old code iterates overpendingfutures callingf.done()point-in-time. This is inherently racy:elsebranch_cf_wait(timeout=0.5)return and the top-of-loop interrupt check are never collected through the normaldonepathThe fix replaces the racy iteration with a 2-second grace window via
wait(pending, timeout=2.0, return_when=ALL_COMPLETED), cleanly separating futures into completed (collect real results) vs still-running (fabricate as interrupted).Related Issue
This PR is complementary — it fixes the specific race where already-computed results are lost on interrupt, without requiring the larger async delegation redesign from #8482.
Type of Change
Changes Made
tools/delegate_tool.pybatch interrupt handler: replaced racyf.done()iteration withconcurrent.futures.wait(pending, timeout=2.0, return_when=ALL_COMPLETED)grace window that cleanly separates done vs still-pending futures.tests/tools/test_delegate_batch_interrupt_race.pywith 3 test cases covering the race condition scenarios.How to Test
python -m pytest tests/tools/test_delegate_batch_interrupt_race.py -vpython -m pytest tests/tools/test_delegate.py -vconcurrent_log_handlerin dev env, unrelated to this change).python -m pytest tests/tools/test_delegate_subagent_timeout_diagnostic.py tests/tools/test_delegate_toolset_scope.py tests/tools/test_delegate_composite_toolsets.py -vFull-suite note:
pytest tests/ -qdoes not fully complete on this local environment due to 9 pre-existing failures from missingconcurrent_log_handlermodule (required by tests that instantiate realAIAgentwithout mocking). All tests touching the delegation interrupt path pass. The focused touched-path suites above cover the change surface.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — all touched-path tests pass; 9 pre-existing failures from missingconcurrent_log_handlerare unrelated (see Full-suite note above)Documentation & Housekeeping
docs/, docstrings) — N/Acli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/AScreenshots / Logs
N/A. This is a concurrency correctness fix with unit test verification.