Tear down blocked task chains iteratively - #747
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Merging this PR will degrade performance by 4.43%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | loop_mod_13__monty |
397.7 µs | 416.7 µs | -4.55% |
| ❌ | loop_mod_13_limits__monty |
399.5 µs | 418.4 µs | -4.51% |
| ❌ | list_comp__monty |
282.6 µs | 295.1 µs | -4.23% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing rewitt94:fix/iterative-task-cancellation (dc501ca) with main (edeb82a)
Footnotes
-
16 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
`Scheduler::cancel_one` only mapped a blocked gather's `pending_children` back to tasks through `coroutine_to_task`, so a child that was itself a `GatherFuture` was skipped: the tasks that inner gather had spawned stayed in `Scheduler::tasks`, kept running, and on completion delivered a result to the task that had just been cancelled — panicking with `Scheduler::get_task_mut: task not found`. The walk now follows nested gathers through `queue_gather_tasks`, using a second worklist so neither deep task chains nor deep gather nesting recurses. It runs before the task is dropped, while the task still holds the `Blocked` inc_ref anchoring the tree. Reported by cubic on pydantic#747. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Scheduler::cancel_task` recursed into the tasks spawned by the gather each task was blocked on, so a chain built through coroutines — which costs no native stack to build, since the commit walk unwinds at every coroutine and leaves the depth in `Scheduler::tasks` — turned that stored depth back into frames on teardown and aborted the process. It now drains an explicit worklist, with the per-task walk moved into `cancel_one`. A task is dropped ahead of the children it queues, which is sound because each child owns an inc_ref on that same gather. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/monty/src/bytecode/vm/scheduler.rs">
<violation number="1" location="crates/monty/src/bytecode/vm/scheduler.rs:454">
P2: When a very large nested-gather tree is cancelled, `queue_gather_tasks` traverses every node without polling `ResourceTracker`. Teardown can therefore exceed the configured time limit and monopolize a worker; propagate a time-check failure or otherwise bound this traversal.
(Based on your team's feedback about native-loop time guards.)</violation>
</file>
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| // Gathers nest as a tree — a gather may only be awaited once, so the | ||
| // walk cannot revisit a node and terminates. | ||
| let mut gathers: SmallVec<[HeapId; 4]> = smallvec![root]; | ||
| while let Some(gather_id) = gathers.pop() { |
There was a problem hiding this comment.
P2: When a very large nested-gather tree is cancelled, queue_gather_tasks traverses every node without polling ResourceTracker. Teardown can therefore exceed the configured time limit and monopolize a worker; propagate a time-check failure or otherwise bound this traversal.
(Based on your team's feedback about native-loop time guards.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/bytecode/vm/scheduler.rs, line 454:
<comment>When a very large nested-gather tree is cancelled, `queue_gather_tasks` traverses every node without polling `ResourceTracker`. Teardown can therefore exceed the configured time limit and monopolize a worker; propagate a time-check failure or otherwise bound this traversal.
(Based on your team's feedback about native-loop time guards.) </comment>
<file context>
@@ -427,26 +424,51 @@ impl Scheduler {
+ // Gathers nest as a tree — a gather may only be awaited once, so the
+ // walk cannot revisit a node and terminates.
+ let mut gathers: SmallVec<[HeapId; 4]> = smallvec![root];
+ while let Some(gather_id) = gathers.pop() {
+ // Coroutine and external children land here too; only gathers have
+ // children of their own to walk.
</file context>
There was a problem hiding this comment.
Currently, Monty polls the ResourceTracker only in fallible native loops that produce a value and never in any teardown path. So there is no precedent for this.
Timeout enforcement happens at the end of the turn still.
`Scheduler::cancel_one` only mapped a blocked gather's `pending_children` back to tasks through `coroutine_to_task`, so a child that was itself a `GatherFuture` was skipped: the tasks that inner gather had spawned stayed in `Scheduler::tasks`, kept running, and on completion delivered a result to the task that had just been cancelled — panicking with `Scheduler::get_task_mut: task not found`. The walk now follows nested gathers through `queue_gather_tasks`, using a second worklist so neither deep task chains nor deep gather nesting recurses. It runs before the task is dropped, while the task still holds the `Blocked` inc_ref anchoring the tree. Reported by cubic on pydantic#747. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6c12bab to
dc501ca
Compare
| ### A failing `gather` cancels its siblings | ||
|
|
||
| When one child of a `gather` raises, every sibling still running is cancelled where it is blocked and never resumes. | ||
| That includes the tasks of any gather a sibling was itself awaiting. | ||
| CPython leaves those siblings running as tasks on the loop, so: | ||
|
|
||
| ```python | ||
| async def worker(): | ||
| for _ in range(3): | ||
| await asyncio.gather(step()) | ||
| done.append('finished') | ||
| ``` | ||
|
|
||
| appends `'finished'` under CPython after a sibling of `worker()` raises, but not under Monty. | ||
|
|
||
| External calls already passed to the host are not cancelled. | ||
| The host still resolves them and the results are discarded. |
There was a problem hiding this comment.
How hard would it be to fix this divergence?
2 participants
Before: Cancelling nested blocked tasks recursed with
cancel_taskcalling itself, overflowing the stack, aborting workers.Now: Cancellation drains a heap worklist iteratively; constant stack, no abort.
Same class as #649 and #704
Summary by cubic
Tears down blocked task chains and nested gathers iteratively to avoid stack overflows and orphaned tasks. Previously, cancellation recursed, overflowed on deep chains, and skipped tasks owned by nested gathers, causing lingering work and “Scheduler::get_task_mut: task not found” panics.
Scheduler::cancel_taskwith aSmallVecworklist andcancel_one; constant stack, and safe to drop the parent before queued children due to inc_refs.queue_gather_tasksto walk nested gathers and enqueue spawned-task children; leaves external futures anchored.gathercancels siblings (differs from CPython).Written for commit dc501ca. Summary will update on new commits.