fix(langgraph): gate pure graph concurrent node dispatch on per-node parallel flag #117
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Blocks
#97 Pure-graph parallel dispatch runs all next-nodes concurrently whenever len > 1, ignoring each node's
parallel flag
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!117
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bugfix/m1-pure-graph-parallel-node-gate"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Fixes issue #97: all three concurrent-dispatch sites in
PureLangGraph(execute()'s_execute_from_node(), and both branches ofexecute_stream()) fired every candidate next-node concurrently viaasyncio.gather/asyncio.create_taskwhenever the graph-levelparallel_execution: true(the default) and there were 2+ candidates, without ever consulting each node's ownparallelflag. This violated Actor Configuration Standard §6.7 and §12.3.Fixed by adding a shared
PureLangGraph._partition_by_parallel_flag()helper used at all three sites: the parallel-eligible subset is gathered concurrently and joined first (§6.7 steps 2-3), the remainder runs sequentially afterward (§6.7 step 4), and theparallel_execution: falseoverride is unaffected. As a side effect,execute_stream()no longer silently drops the token output of every fan-out sibling except the last-completing one.The three pre-existing TDD regression scenarios (issue #98,
@tdd_issue_97) now pass with@tdd_expected_failremoved — confirmed via the project'sTddExpectedFailPolicyhook flagging them as unexpectedly-passing before the tag was dropped. Five additional Behave scenarios cover all-parallel, mixed parallel/non-parallel, andparallel_execution: false, at both entrypoints. A new Robot integration test drives three real sibling agents with non-monotonic delays and verifies declaration-order completion (verified to genuinely fail against the pre-fix code). A new ASV benchmark tracks dispatch cost for the two flag configurations.Closes #97
Test plan
nox -s lint— greennox -s format -- --check— greennox -s typecheck— green (Pyright strict, 0 errors)nox -s security_scan— greennox -s dead_code— greennox -s unit_tests— green (148 features, 0 failures, including 8 scenarios inpure_graph_parallel_node_gate.feature)nox -s integration_tests— green (341/341, including the new Robot test)nox -s coverage_report— see CI for the final number on this branchnox -s benchmark— new benchmark runs cleanly (PureGraphParallelDispatchBenchmark)parallelflagPR Review: !117 (Ticket #97)
Verdict: Request Changes
The implementation correctly gates pure-graph concurrent dispatch on each node's
parallelflag and covers all acceptance criteria with Behave, Robot, and ASV tests. However, the latest CI run failed the requiredcoverageandbenchmarkjobs, so the branch cannot be merged until those gates are green.Critical Issues
None.
Major Issues
Required
coverageCI job failed (appears to time out)coverageslipcoverwrapped around Behave (nox > python -m slipcover ... -m behave -q --no-capture features/) with no coverage report or threshold check emitted. This prevents verifying the required ≥97% coverage gate. Note that the plainunit_testsjob (Behave without slipcover) passed, so the tests themselves are not hanging; the failure is specific to the coverage session.nox -s coverage_reportlocally and inspect whether the session completes and meets the threshold. If it is a runner timeout, reduce suite runtime or request a timeout increase. If coverage actually dropped below 97%, add tests for the newly uncovered paths (e.g., the new_partition_by_parallel_flagbranches, exception/cancellation paths in the sequential subset, or theparallel_execution: falsepath).Required
benchmarkCI job failed (timed out)benchmarkPureGraphParallelDispatchBenchmarkmethods themselves completed successfully and quickly (~2 ms each), but the overall benchmark session did not finish, so the required benchmark gate is red.nox -s benchmark/nox -s benchmark_regressionlocally end-to-end. If the suite is simply too large for the runner's time limit, reduce the cost of the new benchmarks (fewer siblings/shorter warm-up) or work with infra to extend the runner timeout. Do not rely on "new benchmark runs cleanly" if the full ASV matrix cannot complete in CI.Minor Issues
src/cleveractors/langgraph/pure_graph.py, lines ~1920-1939 and ~2040-2057parallel_subset,_collect_stream_tokensbuffers all tokens and onlyresults[-1]is yielded. The PR description correctly notes that non-parallel siblings are now streamed sequentially (so their tokens are preserved), but parallel-marked siblings continue to lose all tokens except those from the last-completing branch. This is pre-existing behavior and is documented inexecute_stream, but it is a genuine limitation of the current streaming fan-out semantics.parallel: trueon a node currently implies buffered, last-branch-only token delivery.Nits
None.
Summary
The core fix is sound:
_partition_by_parallel_flagis applied consistently at all three dispatch sites (_execute_from_nodeand bothexecute_streambranches), preservingparallel_execution: falseand correctly running non-parallel-marked siblings sequentially. The test matrix (regression scenarios, all-parallel, mixed, and override cases across bothexecuteandexecute_stream) matches the acceptance criteria, and the Robot integration test plus ASV benchmark are welcome additions. Once thecoverageandbenchmarkCI jobs are passing, this should be good to merge.PR Review Update: !117 (Ticket #97)
Verdict: Approve
The previously failing
coveragejob is now green (CI / coverage succeeded in 6m41s), andstatus-checkis passing. As noted, thebenchmarkjob is not a required gate. With the required CI gates green, the remaining concerns are minor and do not block merge.Critical Issues
None.
Major Issues
None.
Minor Issues
src/cleveractors/langgraph/pure_graph.py, lines ~1920-1939 and ~2040-2057parallel_subset,_collect_stream_tokensbuffers all tokens and onlyresults[-1]is yielded. The PR correctly preserves tokens for non-parallel siblings by streaming them sequentially, but parallel-marked siblings continue to lose all tokens except those from the last-completing branch. This is pre-existing, documented behavior inexecute_stream, but it remains a limitation of the current streaming fan-out semantics.parallel: trueon a node currently implies buffered, last-branch-only token delivery.Nits
None.
Summary
The fix correctly implements Actor Configuration Standard §6.7/§12.3:
_partition_by_parallel_flagis applied at all three dispatch sites, theparallel_execution: falseoverride is preserved, and the acceptance criteria are covered by Behave regression scenarios, additional flag-combination scenarios, a Robot integration test, and an ASV benchmark. With the required CI gates now green, this PR is approved for merge.In review issuecomment 322041, Minor Issue 1, "Streaming still drops tokens for parallel-marked siblings", this is actually a Major issue and must be fixed!
d8dc9d48d16479902d50Response to review of !117 (Ticket #97)
Thanks for both review passes, @hurui200320, and for the escalation, @CoreRasurae.
Addressed
Minor Issue 1 from the Approve review — "Streaming still drops tokens for parallel-marked siblings" (reclassified to Major in the follow-up comment) — FIXED
execute_stream()dispatch sites (_stream_from_node's intermediate-AGENT branch and its non-AGENT branch,pure_graph.py), theparallel_execution: truesubset was collected via_collect_stream_tokens()intoresults = await asyncio.gather(*tasks), but onlyresults[-1](the last-declared branch, not even the last-completing one) was ever yielded to the caller. Every other concurrent sibling's tokens were silently discarded._partition_by_parallel_flag()in the original commit). However, §6.9 requires pure-graph mode's "externally observable semantics" to be identical to mixed-mode operation, and in mixed mode (§6.10) each node wires into its own internal stream, so parallel siblings' outputs are independently observable rather than collapsed into one. The old behavior broke that equivalence. No ADR indocs/adr/governs this — it's a straightforward bug fix aligning code to already-normative spec intent, not a new architectural decision, so no ADR was required.asyncio.gatherpreserves input task order regardless of completion order), before the sequential subset streams afterward. Verified the sole downstream consumer (runtime_dispatch.py'sasync for token in graph.execute_stream(...): yield token) has no single-token assumption, so emitting more tokens is safe.pure_graph_parallel_node_gate.feature) — one per streaming dispatch site (non-agent trigger, agent trigger) — asserting the streamed token list includes output from both parallel siblings, not just one. Coverage run scoped to this feature confirms the new/changed lines are exercised; the only nearby uncovered lines are the pre-existingexcept BaseExceptiontask-cancellation branch, unrelated to this change and not a regression.execute()path's return-value semantics (result = results[-1] if results else output_message) were intentionally left as-is. That method has a single-string return contract used to continue the (necessarily single) execution path forward — it isn't a fan-out-to-external-consumer API likeexecute_stream(), neither review flagged it, and altering it isn't required by §6.7/§12.3's "results joined before continuing" language, which is about scheduling, not the shape of the joined value.Not actioned, with justification
Verification performed (all via
nox, per project convention — nothing invoked directly)nox -s lint— greennox -s typecheck— green (Pyright strict, 0 errors)nox -s security_scan— green (bandit + semgrep, 0 findings)nox -s dead_code— green (vulture)nox -s unit_tests -- features/pure_graph_parallel_node_gate.feature— 10/10 scenarios pass (8 original + 2 new)nox -s coverage_report -- features/pure_graph_parallel_node_gate.feature— confirms the fix's new lines are covered (scoped to this feature's tests, per the delta relevant to this change rather than the full-suite gate)Amended into the existing commit (single commit on this branch) and pushed:
6479902d50f0cda68ae83d52e270d0f29f09b8a4. CHANGELOG entry updated in place to describe the final behavior (still unreleased/in-progress work, so no separate "fix" entry was added).Ready for re-review.