Repository navigation
Conversation
gnanirahulnutakki
left a comment
There was a problem hiding this comment.
Reviewed exact head 13616a0d. The per-batch semaphore approach matches #3156 and preserves the documented Python scheduler. Locally, all 83 graph tests passed and Ruff lint/format checks were clean. I found one reproducible runtime-validation issue inline; addressing it at both public construction paths should make the cap safe for Python callers.
gnanirahulnutakki
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 97c2b6e62498b98fc8d575743299bc6e0351c190.
The follow-up is focused on the reported runtime-validation gap: both GraphBuilder.set_max_concurrency and Graph.__init__ now reject bool, fractional values, zero, and negatives, with parameterized coverage for each public path. Independent validation passed:
- complete graph test module: 86 passed
- Ruff format check: clean
- Ruff lint: clean
- targeted mypy: clean
- current GitHub checks: 7 passed, 3 awaiting workflow authorization, 0 failures
The original positive-integer finding is addressed. Approved.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Assessment: Approve (with two minor, non-blocking suggestions) Clean, well-scoped port of the Review notes
Nicely aligned with the issue's scope guard and the documented Python/TS scheduling divergence. |
|
@strandly-the-agent can you review this PR? Additionally test for how this affects graph resume logic |
strandly-the-agent
left a comment
There was a problem hiding this comment.
TL;DR: The semaphore mechanics themselves are solid (verified below), but max_concurrency + session persistence reopens the #3390 fan-in resume bug — confirmed with a deterministic repro, 5/5 runs, join double-executes on resume. 🔴 blocker. Everything else I dug into (refactor equivalence, event-loop safety, the 0.1s poll loop, scope guard) checked out clean.
✅ Verified myself on 97c2b6e6: 86/86 test_graph.py pass; _find_newly_ready_nodes/_is_node_ready_with_conditions/_execute_graph/_build_node_input byte-identical to merge-base 1b671d6a; stream_node/execute_node split is control-flow-identical to base when max_concurrency is None; semaphore releases on exception/timeout/cancel via async with on every path I traced; per-batch asyncio.Semaphore construction can't leak across batches or bind to the wrong loop; the 0.1s poll loop doesn't busy-wait or exit early while tasks are queued on the semaphore.
Full analysis
🔴 Blocker: max_concurrency breaks the in-flight signal that session-resume depends on
See inline comment on graph.py:941. _compute_pending_sources's EXECUTING-based seed (graph.py:1415, added by #3390 specifically for "two concurrent entry points feeding one join") assumes every dispatched node reaches Status.EXECUTING (graph.py:1023) essentially immediately. This PR's semaphore now gates entry to that line — a node queued behind the cap stays PENDING, indistinguishable from a node that was never scheduled. Repro (ran against this branch, uv run --extra dev pytest):
two entry points {left, right} -> join, max_concurrency=1
baseline (no cap): sibling status at capture = EXECUTING, next_nodes_to_execute = ['right'] (join correctly excluded)
capped (=1): sibling status at capture = PENDING, next_nodes_to_execute = ['right', 'join'] (join wrongly marked ready)
Carrying that snapshot through an actual deserialize_state + invoke_async resume (mirroring test_graph.py's own TestResumeInFlightSibling, test_graph.py:3285-3516) shows the consequence: join.stream_async is called twice — 5/5 runs. Root cause once join lands in next_nodes_to_execute alongside right: after that batch runs, _find_newly_ready_nodes re-evaluates join's incoming edge from right (now in completed_batch) and marks it ready again for the next batch — _is_node_ready_with_conditions has no "already ran this batch" guard. GraphResult.status still reports COMPLETED, so this is silent: extra cost, and a correctness problem for any non-idempotent node.
Scope: this only affects session-resume (_compute_pending_sources's EXECUTING seed). I didn't find an equivalent dependency in the interrupt-resume path. TestResumeInFlightSibling's existing tests (test_graph.py:3285-3516) are the regression coverage for the underlying #3390 fix, but none of them are parametrized with max_concurrency set, which is how this got through.
For the reviewer: either the EXECUTING transition (or an equivalent "has an outstanding task in this batch" signal) needs to happen before/independent of the semaphore gate, or max_concurrency + SessionManager + this fan-in topology needs to be explicitly called out as unsupported until fixed.
🟡 Should-fix: graph.max_concurrency = 0 set post-construction hangs forever, silently
Both validated construction paths (GraphBuilder.set_max_concurrency, Graph.__init__) correctly reject 0. But self.max_concurrency is a plain public attribute (graph.py:562), and _execute_nodes_parallel (graph.py:859) builds asyncio.Semaphore(self.max_concurrency) from it fresh every batch with no re-validation. Setting graph.max_concurrency = 0 after construction produces a Semaphore(0), which starts locked — the next batch hangs indefinitely with no error. Confirmed: invoke_async under a 1s asyncio.wait_for times out. Low likelihood (requires bypassing both public setters) — should-fix, not a blocker.
⚪ Nit: capped batches can deadlock if a node's own logic depends on a batch-mate's progress
Expected/inherent to any bounded-worker scheduler, not a bug in this PR, but worth a one-line callout in the set_max_concurrency docs: with max_concurrency < len(batch), if any node's executor internally waits on another node in the same batch making progress (e.g. a shared external coordination primitive), the batch can deadlock, since the semaphore doesn't guarantee fairness/relative ordering. Nested Graph/Swarm executors are fine — each maintains its own independent semaphore, no shared resource.
Appendix — found while testing, pre-existing, unrelated to this PR
While building the resume repro I found a second, structurally guaranteed (non-racy) double-execution bug: a node with ≥2 real incoming edges that land in the same dispatch batch re-fires itself via _find_newly_ready_nodes after that batch completes. Reproduces with max_concurrency unset, using only unmodified base scheduler functions — filed separately as #3628, not blocking here.
strandly-the-agent
left a comment
There was a problem hiding this comment.
TL;DR — good, well-scoped port; the resume interaction opieter-aws asked about is real but is a pre-existing defect this PR widens, not one it introduces. No blockers from me. I also have to correct my own earlier review on this PR: it graded that finding 🔴 and requested changes, and that was wrong on tier, numbers, and attribution. March-77, apologies for the churn — details in the graph.py:941 thread, and I've retracted the bogus issue I filed alongside it (#3628).
On resume specifically (the ask): a node queued on the new semaphore stays PENDING and never reaches Status.EXECUTING (graph.py:1023), so _compute_pending_sources' EXECUTING-only in-flight seed (graph.py:1415) can't see it. With ≥2 independent entry points feeding one join plus a mid-batch session write, the join is persisted as ready and runs twice on resume, silently (GraphResult.status == COMPLETED). But that code is byte-identical to merge-base and reproduces on main without this PR — filed as #3633. The PR's contribution is turning it from a corner case (needs a non-suspending model) into always-fires for any executor. Interrupt-resume is clean — a slot-less node is never dropped (8/8).
✅ Verified on 97c2b6e6 (base 1b671d6a): 86/86 test_graph.py; ruff check/format + mypy clean; #3156's scope guard holds — _find_newly_ready_nodes, _is_node_ready_with_conditions, _execute_graph, _build_node_input, _compute_pending_sources, serialize_state/deserialize_state all AST-verified byte-identical to base; the stream_node/execute_node split is control-flow-identical to base when max_concurrency is None; no semaphore slot leaks on raise/timeout/cancel/nested/multi-batch; test_graph_max_concurrency_limits_parallel_nodes is genuinely enforcing (15/15, and survives 4 mutations of the impl). 🔴 Refuted and retracted: my #3628.
5 × 🟡 inline below (2 correctness, 2 API consistency, 1 test coverage). None require a code change to merge if the maintainers would rather ship and track.
Verification detail — how the resume finding was established
End-to-end through a real FileSessionManager with a hard os._exit the instant the session write lands (no cleanup, no later overwrite), then a fresh process to resume:
[write] completed=['left'] next_nodes=['right','join'] status=executing | right=pending
--- restart ---
[resume] execution_order : ['left','right','join','join']
[resume] join.stream_async : 2 call(s) <-- DOUBLE EXECUTION
Control, same topology/agents, uncapped: next_nodes=['right'] | right=executing → join ran 1 time(s).
Attribution matrix (dispatch order forced worst-case, 10 runs each):
| tree | cap | executor | bad frontier |
|---|---|---|---|
base 1b671d6a |
— | mock, no awaits | 10/10 |
| base | — | + await sleep(0) or sleep(0.005) |
0/10 |
head 97c2b6e6 |
1 | any of the three | 10/10 each |
| head | 2 (= batch width) | real await | 0/10 |
So one suspension point makes base safe — but a real Agent whose Model.stream() never awaits (cached/replayed/local model; Model is a public extension point) reproduces it on base too, which is why it's #3633 rather than a finding against this PR. Frequency under the cap is ~100% conditional on which entry point dispatches first, which is itself hash-randomized (entry_points is a set, graph.py:266/308) — ~22/30 fresh processes.
Also confirmed clean: max_concurrency correctly absent from serialize_state() (consistent with the other three construction-time limits); deserialize_state's start-over + reset_on_revisit branch identical with and without the cap.
Questions
❓ (decision, blocking-ish — for @opieter-aws, since you asked about resume) Given the finding is a pre-existing defect that this PR makes routinely reachable: ship as-is with #3633 tracked and a docs caution, or hold until #3633 is fixed? I don't think it should block March-77 either way — it's a maintainer call about the knob's readiness, not about this diff's quality.
❓ (non-blocking) The cap makes entry-point dispatch order observable run-to-run, because entry_points is a hash-randomized set while TS drains an ordered array (graph.ts:346). Acceptable long-term, or worth an ordered structure as a follow-up?
Appendix — non-blocking (⚪ nits, pre-existing, and what came back clean)
⚪ Nits
- Error message drops the repo's bracket convention:
max_concurrency={value!r} | must be…vs the housefield=<value> | message(AGENTS.md:56; cf.vended_plugins/goal/plugin.py:246). TS emits bracketedmax_concurrency=<N> | must be at least 1(graph.ts:592-593), which is the wording #3156 asked to reuse. Keep!r— it's strictly better here, distinguishing'4'/4andTrue/1— just restore the brackets (graph.py:410, 556). Graph.__init__gains aValueErrorbut has noRaises:section (docstring graph.py:531-548); AGENTS.md requires one. The new setter models it correctly.- New tests don't use the repo's
tru_/exp_assertion-pair convention (docs/TESTING.md), used 79× elsewhere in the same file. test_graph_max_concurrency_validation's regex has an unescaped.for the1.5case — are.searchwildcard. No false pass today, just fragile.- PR body: "validates at least 1" undersells the check (it also rejects
bool/fractional), and "83 passed" is now 86.
Pre-existing — filed//to file separately, not this PR's problem
- #3633 — the
_compute_pending_sourcesroot cause above. - #3628 — my false positive. Closed and retracted; please disregard.
- Three Python doc snippets are copy-paste broken:
Graph(agents={...})→TypeError(Graphtakesnodes/edges/entry_points) atsession-management.mdx:100-102,session-management.mdx:180-183,hooks.mdx:746-749. Nothing catches them —typecheck:snippetsis TS-only (site/test-snippets/tsconfig.json:16) and these are inline Python blocks. Unrelated to this PR; happy to file it. - A capped batch can deadlock if a node's executor waits on a batch-mate's progress — inherent to any bounded-worker scheduler, not a defect here. Worth knowing that
execution_timeoutdoes not rescue it (boundary-only check) whilenode_timeoutdoes.
Already covered by others — not re-raised
- The two open
github-actionssuggestions (extract the shared validation helper; document thenode_timeout-vs-slot-wait clock) both still stand; the first is also the natural home for the graph.py:859 fix below. gnanirahulnutakki's fractional/boolvalidation finding — resolved in97c2b6e6, confirmed fixed.
Process note: this touches public API but is a minimal parity port (API_BAR_RAISING.md:20 — "adding a new parameter to an infrequently used method"), so PR review is sufficient; no API meeting needed. For the paper trail the repo's labels are api/needs-review / api/review-complete (API_BAR_RAISING.md:72), neither currently applied.
One caveat on all of this: I'm an experimental agent and the resume analysis above is the part most worth a human double-check before you act on it.
|
The runtime validation, shared helper, |
|
The previous CI run exposed a timing-sensitive Windows failure in |
|
Re-checked at head Root cause (unchanged): a node is marked Reproduced at head — two entry points A/B → join C, snapshot with A completed and the co-entry-point in the state each convention produces: On resume the join runs with only one parent's result and re-fires when the second parent completes — a double-execution, and silent, since Suggested fix: mark the node (Verified via |
|
Addressed the Aug 18 resume finding in Validation:
The currently failed Python integration job is unrelated to this change: it failed because |
gnanirahulnutakki
left a comment
There was a problem hiding this comment.
Assessment: Request changes
The resume fix is still scheduled one event-loop turn too late. The exact-head differential below reproduces the silent double execution; moving the state transition before task creation eliminates it.
| if self.node_timeout is not None: | ||
| # A scheduled node is in flight even while it waits for a concurrency slot. Session | ||
| # serialization uses EXECUTING to keep queued branches in the resume frontier. | ||
| node.execution_status = Status.EXECUTING |
There was a problem hiding this comment.
Issue: This assignment runs only after each created task starts. A non-suspending first executor can finish and serialize its session before the sibling task starts, leaving that sibling PENDING. On exact head 14f0b33, the real batch scheduler reproduced this 3/3 times: serialized frontier [right, join], then deserialize plus invoke ran join twice while returning COMPLETED. The full graph module still passes 100 tests, so the current sleep-based assertion does not cover the ordering boundary.
Suggestion: Mark every node in the dispatch batch EXECUTING synchronously in _execute_nodes_parallel before calling create_task, then add a regression with a non-suspending first entry point that serializes in AfterNodeCallEvent and asserts the restored join runs once. With only that placement changed locally, the same control passed 3/3: right was EXECUTING, the frontier was [right], and join ran once.
There was a problem hiding this comment.
Ran your differential independently at this exact head, plus the two controls that decide whether this should block the PR. Your mechanism is confirmed — the assignment is inside the task coroutine, so it only runs at the task's first event-loop slice, and a non-suspending first executor completes (and serializes) before that. But the controls show the remaining window is not cap-specific: it is exactly the pre-existing #3633 window, unchanged by this PR.
15 fresh processes per cell, two entry points → join, snapshot via AfterNodeCallEvent hook, then deserialize_state + invoke_async on a fresh graph:
| cell | tree | cap | executor | bad frontier | join runs 2× |
|---|---|---|---|---|---|
| A | head 14f0b335 |
1 | non-suspending | 15/15 | 15/15 |
| B | head 14f0b335 |
none | non-suspending | 15/15 | 15/15 |
| C | merge-base 5f357aef |
none | non-suspending | 15/15 | 15/15 |
| D | head 14f0b335 |
1 | suspending (5ms) | 0/15 | 0/15 |
| E | head 14f0b335 |
none | suspending (5ms) | 0/15 | 0/15 |
| D′ | prev head 0cdaaee7 |
1 | suspending (5ms) | 15/15 | 15/15 |
A confirms your 3/3. B and C are the cells that change the conclusion: the identical failure fires with the cap removed and on merge-base main — the residual race needs an executor that reaches its first await without suspending, which is #3633's original precondition, cap or no cap. And D vs D′ shows 14f0b335 did fix what it claimed: the cap-specific widening (any suspending executor failing under cap=1) is gone at this head, 15/15 → 0/15.
So I'd argue this shouldn't hold the PR: the defect that remains is in _compute_pending_sources' seed, byte-identical to main, tracked as #3633, and the PR's docs now carry a caution pointing at it. Same call we made on the Aug-4 iteration of this exact finding.
Your suggested placement (mark the batch EXECUTING synchronously in _execute_nodes_parallel before create_task, after the interrupt filter and the runtime validation) is, per your own 3/3 control, the right fix for #3633's batch-dispatch case — I've cross-posted it there so it doesn't get lost. If March-77 wants to fold it in here as a bonus that's fine by me, but I wouldn't require it: it fixes a bug this PR no longer aggravates.
There was a problem hiding this comment.
I independently reproduced this at the current head (f518ad7) and can confirm both the finding and the fix. Two entry points → join, snapshot via AfterNodeCallEvent, then deserialize_state + invoke_async on a fresh graph, 15 runs per cell:
| cell | cap | executor | join wrongly in frontier |
|---|---|---|---|
| A | 1 | non-suspending | 15/15 |
| B | none | non-suspending | 15/15 |
| C | 1 | suspending (5ms) | 0/15 |
| D | none | suspending (5ms) | 0/15 |
So the finding reproduces (A), and — matching @strandly-the-agent's cells B/C — the residual window is not cap-specific: it fires identically with the cap removed (B), which is the pre-existing #3633 precondition. The PR's persist queued nodes as in flight change did close the cap-specific suspending case (C/D clean).
I then applied your suggested placement (mark the whole dispatch batch EXECUTING synchronously in _execute_nodes_parallel before create_task, after the interrupt filter + validation) and re-ran: all four cells went to 0/15, and the full graph module still passes (102 tests). Notably it also fixes cell B — the pre-existing batch-dispatch case — so it's a strict improvement, not just a cap-specific patch.
My recommendation: fold this in here. It's a ~2-line move, fully green, and it completes the resume-correctness logic this PR already introduced — leaving that logic defeatable by a non-suspending executor (silently, since GraphResult.status still returns COMPLETED) is an incomplete version of the fix the PR set out to add. I'd weight it Important rather than a hard merge-blocker, since — as strandly established — the residual is byte-identical to main and not a regression this PR introduces; but given the change is trivial, verified, and in the exact path being modified, doing it here is clearly better than deferring the whole batch-dispatch case to #3633. A regression with a non-suspending first entry point (as you suggested) should accompany it, since the current sleep-based tests don't pin this ordering boundary.
|
@strandly-the-agent review latest revision please |
strandly-the-agent
left a comment
There was a problem hiding this comment.
TL;DR — follow-up review of 14f0b335: all five of my prior 🟡 findings are fixed and verified, the resume widening I flagged on Aug 18 is genuinely fixed (proven by differential, 15/15 → 0/15), and gnanirahulnutakki's residual-race finding is real but is exactly pre-existing #3633 — it fires identically with the cap removed and on merge-base main. Approving.
Status of my previous findings, verified against the diff at this head:
| finding (Aug 4/18) | status |
|---|---|
queued node stays PENDING → resume double-exec (Aug 18 re-check) |
✅ fixed by 14f0b335 — cell D vs D′ below |
graph.max_concurrency = 0 post-construction hangs silently |
✅ fixed — runtime re-validation before Semaphore(...) (graph.py:867-869) + test_graph_max_concurrency_validation_after_assignment |
| constructor param should be keyword-only | ✅ fixed — * before max_concurrency (graph.py:536-537) |
set_max_concurrency(None) asymmetry |
✅ fixed — int | None, None clears (graph.py:403-418) |
docs: cap can trip execution_timeout; no cap×anything tests |
✅ fixed — graph.mdx caution + slot-wait/node_timeout note, plus release-after-failure, release-after-cancellation, queue-time, and execution-timeout interaction tests |
✅ Verified myself at 14f0b335 (merge-base 5f357aef7): 100/100 test_graph.py; ruff check + format --check clean; mypy clean (one run hit OperationalError: database is locked — a mypy cache artifact from concurrent runs, clean with a fresh --cache-dir, and merge-base checks clean too); CI at head fully green (23 success, 4 skipped, 0 failures). Error messages now use the house field=<value> \| message shape via the shared _validate_positive_int helper (graph.py:302-304), which also resolves the earlier dedup suggestion.
On the open CHANGES_REQUESTED (gnanirahulnutakki, graph.py:931): mechanism confirmed — the EXECUTING assignment is inside the task coroutine, so a non-suspending first executor can serialize before a sibling's task ever starts (my cell A: 15/15, matching their 3/3). But the controls show it is not this PR's defect anymore: the identical failure fires uncapped at head (B: 15/15) and on merge-base main (C: 15/15) — that is precisely #3633's pre-existing window, needing an executor that reaches its first await without suspending. Full matrix and detail in the thread reply. Their suggested placement (mark the batch EXECUTING before create_task) is the right fix for #3633's batch case and is cross-posted there; folding it into this PR would be a welcome bonus, not a requirement.
Differential matrix (15 fresh processes per cell)
| cell | tree | cap | executor | bad frontier | join runs 2× |
|---|---|---|---|---|---|
| A | head 14f0b335 |
1 | non-suspending | 15/15 | 15/15 |
| B | head 14f0b335 |
none | non-suspending | 15/15 | 15/15 |
| C | merge-base 5f357aef |
none | non-suspending | 15/15 | 15/15 |
| D | head 14f0b335 |
1 | suspending (5ms) | 0/15 | 0/15 |
| E | head 14f0b335 |
none | suspending (5ms) | 0/15 | 0/15 |
| D′ | prev head 0cdaaee7 |
1 | suspending (5ms) | 15/15 | 15/15 |
A=B=C ⇒ the residual is cap-independent (#3633). D vs D′ ⇒ 14f0b335 fixed the cap-specific widening this PR had introduced.
Appendix — non-blocking (2)
- ⚪ The new
graph.mdxcaution says "Limiting concurrency makes this existing behavior easier to encounter" — that was true at0cdaaee7but is stale after14f0b335: capped and uncapped now fail/pass identically (A=B, D=E). Over-warning is harmless, so fine to ship as-is; if touched again, "This issue exists independently ofmax_concurrency" would be more accurate. - ⚪ The regression assertion added in
14f0b335checks a queued node isEXECUTINGafter its task has started, so it can't catch the create_task→first-slice window (the suite passes 100/100 while cell A fails) — consistent withgnanirahulnutakki's remark. The stronger regression test belongs with the #3633 fix.
As always: I'm an experimental agent — the adjudication of a colleague's blocking finding is the part most deserving a human sanity-check, which is why the raw matrix is above.
Documentation Preview ReadyYour documentation preview has been successfully deployed! Changed pages: Updated at: 2026-08-26T18:13:49.837Z |
|
Assessment: Approve Re-reviewed at head Notes on the latest changes
Solid, well-scoped port with thorough tests and docs; the follow-up items are non-blocking. |
|
Assessment: Request changes (one Important item) Re-reviewed at head The one blocker to resolve is the resume-ordering issue @gnanirahulnutakki raised, which I independently reproduced and confirmed (details in the thread). Summary of my findings: Verification results (15 runs/cell at head f518ad7)
Recommendation: move the |
5aa4749 to
ec9bd31
Compare
Description
Adds a
max_concurrencylimit to Python Graph execution while preserving theexisting batch scheduler and OR-join dependency semantics.
GraphBuilder.set_max_concurrency()and the optionalGraphconstructor parameter.
streaming and node timeout handling.
The constructor parameter is appended to the existing signature so positional
callers remain compatible.
Related issues
Fixes #3156
Testing
uv run pytest tests/strands/multiagent/test_graph.py -q(83 passed)uv run ruff format --check src/strands/multiagent/graph.py tests/strands/multiagent/test_graph.pyuv run ruff check src/strands/multiagent/graph.py tests/strands/multiagent/test_graph.pyuv run mypy src/strands/multiagent/graph.pynpm run typecheck(site)npm run typecheck:snippets(site)npm run build(857 pages, 856 HTML files, no broken links)The broader
tests/strands/multiagentcollection requires the optionala2adependency, which was not installed in the core+dev test environment. The
complete graph test module passed.
Checklist
hatch run prepare.The targeted Python checks and full site build are listed above. The full
multi-version
hatch run preparematrix was not run locally.