Repository navigation
fix(swarm): correct crash-restart resume for handoff and completed swarms - #3391
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Assessment: Approve Clean, well-scoped fix for the two crash-restart resume defects. The change correctly mirrors the already-correct Review notes
Nicely done — the manual end-to-end crash simulation described in the PR body is exactly the kind of verification that builds confidence in a resume fix. |
|
@strandly-the-agent review pls |
strandly-the-agent
left a comment
There was a problem hiding this comment.
🔴 Changes requested — checkpoint restore can still replay/skip work, and one reachable reset path raises KeyError.
✅ Reviewed exact head e13b2bed against merge base 3966e703; no public API change.
✅ Latest completed CI is green across Python 3.10–3.14, lint, docs, and integration.
✅ Independent exact-head adversarial matrix reproduced the findings twice identically; Swarm 39/39, changed-file Ruff, mypy (223 files), and git diff --check passed.
Review breakdown
Three blockers remain:
- The pre-transition checkpoint cannot distinguish terminal success, failed handoff, and legacy frontier semantics, causing replay or turning failure into success.
- Empty-frontier restore installs active interrupt state before resetting every node, which can raise
KeyError. - Unconditional handoff/frontier equality cleanup drops accepted self-handoffs during crash resume.
The terminal-checkpoint test gap is folded into finding 1. I suppressed the pre-existing second-crash shared-context issue, and did not repeat the existing test-docstring suggestion. Docs were otherwise accurate.
Passes: correctness · adversarial · test quality · docs accuracy
|
@strandly-the-agent re-review with the latest fixes |
|
Assessment: Approve (re-review of The three blockers @strandly-the-agent raised were legitimate — my earlier approve missed them — and all three are now correctly resolved and independently verified. I confirmed each fix by reverting it in isolation and watching the matching regression test fail. Verification of the three blockers
Testing: 4 new tests, one per scenario; all are genuine guards (each fails when its fix is reverted). Full swarm suite 43/43, Nice turnaround addressing subtle state-machine edge cases with targeted regression coverage. |
strandly-the-agent
left a comment
There was a problem hiding this comment.
🔴 Changes requested — two exact-head crash-resume paths can still silently skip pending Swarm work.
✅ Re-reviewed exact head 04332244 against prior head e13b2bed and merge base 3966e703; no public API change.
✅ Exact-head CI is fully green; local focused run passed 45/45, changed-file Ruff passed, and git diff --check passed.
✅ The new interrupt reset, terminal checkpoint, ordinary failed handoff, and interrupted self-handoff fixes are resolved.
Review breakdown
Two blockers remain:
- A handoff to a previously visited node can be serialized with an empty frontier, so restore starts over instead of running the pending target.
- Cancellation after requesting a handoff can serialize the target even though the source did not complete, so restore skips unfinished work.
I did not reopen the accepted/documented old-writer compatibility limitation, and I suppressed fresh-run behavior after an empty frontier because that is the intended Graph-compatible contract.
Passes: correctness · adversarial · test quality
|
@strandly-the-agent re-review |
|
Assessment: Approve (re-review of Both blockers from @strandly-the-agent's second pass are correctly resolved and independently verified — I reverted each fix in isolation and confirmed the matching regression test fails. Verification of the two blockers
Testing: 44/44 swarm tests pass; Minor (non-blocking): Codecov reports 80% patch coverage (1 missing + 2 partial lines) — branch coverage on the changed conditionals; the substantive cancel/revisit paths are directly tested, so this is cosmetic. The at-least-once framing is the right call here — solid iteration on a genuinely tricky checkpoint state machine. |
strandly-the-agent
left a comment
There was a problem hiding this comment.
🔴 Changes requested — two cancellation paths can still persist inconsistent handoff state.
✅ Re-reviewed exact head c05970ee against prior 04332244; the pending-revisit fix works.
✅ Exact-head local checks: 46 focused tests; specialists also ran Swarm 44/44 and FileSessionManager 40/40; git diff --check passed.
✅ Both remaining real FileSessionManager crash/restore repros were confirmed twice.
Review breakdown
- Closing the public stream with
aclose()bypasses theCancelledErrorrollback and checkpoints target B even though source A never completed. - Direct cancellation reruns A but persists B-directed handoff content and speculative shared context, which are then injected into A's retry.
These are separate defects, though one atomic rollback helper may address both. I did not repeat resolved findings or the accepted terminal at-least-once/old-writer limitations.
|
@strandly-the-agent re-review |
|
Assessment: Approve (re-review of This revision replaces the earlier targeted cancel-clear with a more general snapshot/rollback design, and it's a clear improvement. Verified by reverting each new mechanism in isolation and watching the matching regression test fail. What changed and how I verified it
Testing: 51/51 swarm tests pass (was 44), including a new end-to-end The snapshot/rollback framing generalizes the fix cleanly across all teardown paths (cancel, stream close, failure) — nice consolidation. |
|
Assessment: Approve (re-review of The latest finding from Verification of the no-handoff regression
Test-strength gaps closed
State of the PR: 63/63 swarm tests pass (was 61), ruff and mypy clean, Codecov all modified lines covered. Settling the frontier/handoff at save time keeps paying off here — the no-handoff case is now just another branch in that one writer, and the fixed-point test locks the resume contract across crash generations. No remaining blocking concerns from my side. |
|
Assessment: Approve (re-review of This follow-on — "release the turn record when a handoff advances the frontier" — closes the last teardown window in the frontier/handoff state machine. I reproduced the gap, verified the fix, and reverse-tested it. The window it closesAfter a handoff transition advances the frontier (
State of the PR: 64/64 swarm tests pass (was 63), ruff and mypy clean, tree clean. The |
|
@strandly-the-agent review latest |
There was a problem hiding this comment.
Warning
Superseded and partly retracted — see #pullrequestreview-4823897865 for my actual position.
This review was submitted through a tooling error on my side and overstates the severity. The finding below is factually reproducible — the terminal AfterNodeCallEvent write does land status="executing" with frontier=[], and restoring exactly that write does reset and discard the finished run — but framing it as trading "a compounding-history bug for a worse one" is wrong: the baseline it compares against (87ff5ada) is the revision that wedged the swarm into FAILED with zero executions. Re-running the task is bounded and within the documented at-least-once contract, so this is a non-blocking trade-off, not changes-requested. My corrected review approves 085b4cf3 and carries the item as 🟡 with that framing.
Original text, kept for the record (severity overstated — read the correction above first)
🔴 Changes requested — the no-handoff empty-frontier fix trades a compounding-history bug for a "discard the whole completed run" bug.
✅ Re-reviewed exact head 085b4cf3 against prior head 87ff5ada and merge base 59bf1ed6.
✅ Commit 2d28c70f genuinely fixes the compounding-history blocker I raised last round — reverse-tested against 87ff5ada.
✅ Commit 085b4cf3 genuinely fixes the teardown-between-nodes handoff-drop — reverse-tested by removing the self._turn = None line, frontier regresses ['second'] → [].
🔴 New regression: a committed terminal turn now serializes next_nodes_to_execute = [], and deserialize_state treats any empty frontier as "nothing to resume" — routing to _reset_for_fresh_run(), which discards node_history, node_results, accumulated_usage/metrics and shared_context wholesale even though the run actually succeeded.
Verified correct (both commits do what they claim):
2d28c70f(swarm.py:1049-1057): a committed turn with no handoff now serializes an empty frontier instead of re-listing the just-finished node. Reverse-tested: collapsing it back to the pre-fix form makes asolo/max_iterations=2repeated-crash-at-the-same-point repro compoundnode_historyacross generations again.085b4cf3(swarm.py:891):self._turn = Noneright after the handoff transition. Confirmed a teardown between "first"'s handoff and "second" starting persistsnext_nodes_to_execute=['second']with085b4cf3, vs[]with only2d28c70fapplied (silently dropping the target).
The window: the new empty-frontier branch is reached whenever a turn commits with no handoff while completion_status is still EXECUTING — before the loop's else: completion_status = Status.COMPLETED. SessionManager wires AfterNodeCallEvent → sync_multi_agent, and that hook fires from inside that window for every node including the terminal one, so FileSessionManager writes it to disk. A real on-disk repro (A→B, B terminal, no crash injected):
write #0 (after A's AfterNodeCallEvent): status=executing node_history=['first'] next_nodes=['second']
write #1 (after B's AfterNodeCallEvent): status=executing node_history=['first','second'] next_nodes=[]
write #2 (after AfterMultiAgentInvocationEvent): status=completed node_history=['first','second'] next_nodes=[]
Restoring write #1 gives completion_status=PENDING, node_history=[], results={} — the entire swarm reruns from the initial node. The identical repro against 87ff5ada writes next_nodes=['second'] at that point, so restore there resumes at "second" while preserving history/results for "first".
Suggested direction: flipping completion_status to COMPLETED where the turn commits, instead of after the AfterNodeCallEvent hook, would make every checkpoint written in this window report status="completed" — funnelling into the documented reset-on-resume contract instead of colliding with it under a misleading "executing" label.
strandly-the-agent
left a comment
There was a problem hiding this comment.
✅ Approving 085b4cf3 — both new commits are genuine fixes, verified by my own repros. One non-blocking trade-off to decide, inline.
First, a correction of my own. The review I posted 20 minutes ago (#pullrequestreview-4823866300) went out through a tooling error on my side, and it overstates the severity: it frames the terminal-window behavior as trading "a compounding-history bug for a worse one". My measurements don't support "worse" — the baseline it compares against (87ff5ada) is the revision that wedged the swarm into FAILED. Treat this review as my position; that one is superseded.
✅ 2d28c70f (empty frontier for a committed turn with no handoff) — verified a fixed point: three successive crashes at the same no-handoff completion each persist history=['solo'] frontier=[], and a clean resume is completed / execution_count=1. At 87ff5ada the same script compounds to ['solo','solo','solo'] and then returns failed with zero node calls. My blocker is closed.
✅ 085b4cf3 (self._turn = None after the handoff transition) — verified a real guard against skipped work: tearing down at the multiagent_handoff event persists frontier=['second'] at head, but frontier=[] at 2d28c70f, where the resume then drops second entirely (history=['first'], only first re-runs).
✅ Exact head: 106 focused tests pass (Swarm 64 + file-session + multi-agent hooks), changed-file ruff check/format --check clean, mypy on swarm.py clean, git diff --check clean.
label-size; every test/lint/docs/integration job and CI Gate are green — looks like the diff outgrew its size label rather than a functional failure.
The one non-blocking item (detail inline)
The terminal node's AfterNodeCallEvent checkpoint — a real SessionManager write — now lands status="executing" with frontier=[]. Restoring exactly that write resets and discards the finished run, so a crash in the gap before the invocation-final write costs a full task re-run instead of a one-node replay. It is bounded and within your at-least-once contract (no skipped work, no corruption, no wedge), and it converges with what a completed checkpoint already does — so which side of the trade you want is your call, not a merge blocker.
The _turn lifecycle reads consistently end to end now: it is created when a turn starts, committed where the state that makes it true is durable, rolled back on every teardown path, and released the moment the finished turn stops speaking for the frontier. Nice iteration on a genuinely subtle state machine.
|
From my reading of the description, I thought swarm resume is broken, after reading the code, I found it fixes non-happy path case, dies in a specific window so can we explicitly update the description? |
|
Given TS avoids this entirely by construction, do we have a tracking issue for this? I would rather have this as temorary bridge. |
|
Assessment: Approve (re-review of Pure rename refactor —
No blocking concerns on the code. Two open maintainer questions worth resolving before merge (@JackYPCOnline)Neither is a code defect, but both seem worth closing out:
|
The underlying cause is that Python stores the resume frontier as a separate field from TS doesn’t have this problem since the handoff decision is stored inside the node’s own result, so the result is the record that the turn committed. Fixing that in Python would mean changing the persisted resume format and reworking how |
|
Thanks for the reply and I see your point, i am incline to approve. Suggesting to have Murat / Patrick to do a bar raise before merging. |
|
my 2c around the last node edge case: we don't make any idempotency promises around swarm runs, the whole point is handing the task around, so the agent might re-run multiple times. Also given the current case is crash, I am okay merging this. I'd like to solve this edge case if we can, but I wouldn't want that to block this PR. so all good from me |
Description
Fixes handoff for crashed nodes in a swarm.
When a
Swarmuses a session manager, it checkpoints after every node so a crashed run can resume fromnext_nodes_to_execute. The checkpoint fires before the execution loop applies a handoff transition, so the saved state was ambiguous:EXECUTINGwith a pending handoff looks identical whether the node committed that handoff or merely inherited it from an earlier turn. Both sides of the contract tried to recover, and every inference had a case it got wrong: a finished node re-ran, a failed run resumed as a success, a cancelled node's speculative handoff was committed, and a pending self-handoff was dropped on the second restart.This replaces inference with a recorded outcome. Each node turn carries a
_InflightTurnwhose outcome the loop sets where the answer is actually known. A persisted handoff always means its target still owes a turn. The writer settles that at save time. A handoff the resume frontier already carries is not recorded, and a committed turn with no handoff persists an empty frontier. This makes restore honor the payload literally and the reader-side equality check is deleted rather than replaced.Two consequences worth noting:
multiagent_node_stopnow seenode_historyalready containing that node, because a completed turn commits before its terminal event is forwarded. A teardown at that yield must not roll back work already recorded.Callouts
A no-handoff completion replays the whole task rather than one node. An empty frontier resets for a fresh run, so a crash at the final node re-runs the task from the entry point instead of re-running that node. The alternative (record the node as done only after the loop) preserves partial progress but introduces some complexity where a stream closed on a succeeded node's terminal event rolls back its handoff and shared context. Both solutions are at-least-once and neither skips work.
The root cause of this callout is that Python lets a node hand off to itself while the TS doesn't support this. That divergence is what makes a frontier/target equality ambiguous, and closing it would make the ambiguity unrepresentable rather than merely handled. Changing that behaviour is a bigger change that is breaking for sessions where nodes have done a self-handoff.
Related Issues
Needed for multi-agent snapshot session manager
Documentation PR
N/A
Type of Change
Bug fix
Testing
Ran
hatch test(full unit suite) plus the multiagent and session suites,ruff check,ruff format --check, andmypyonswarm.py.hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.