fix(engine): resolve mixed fan-in deadlock via per-predecessor barrier relief#13
Conversation
… diagnostics, ordering hint
… §5) is_conditional_join made a binary all-plain-or-all-waiting decision for fan-in nodes: a merge was only treated as a "conditional join" (plain edges) when EVERY predecessor was reachable from a single common brancher's distinct ports. A MIXED fan-in — one unconditionally-reachable predecessor plus one reachable only via a conditional branch — fell through to the all-waiting barrier, so the untaken branch's predecessor never arrived and the merge deadlocked forever. The naive fix (lower the conditional predecessor's edge as plain) causes silent data loss instead: when that branch IS taken, the merge fires off the superstep snapshot before the conditional predecessor's items commit. Fix: every fan-in edge is now lowered as a waiting edge unconditionally (removes is_conditional_join and the plain/waiting branch). Two new helpers classify predecessors instead of the join as a whole: - has_unconditional_path: reachable from the trigger via "main"-port edges only (so it always runs, whether that's a plain pass-through or a parallel fan-out). - conditional_predecessors: a fan-in node's direct predecessors that are NOT unconditionally reachable — i.e. sit behind a branch port. For each conditional predecessor, find_conditional_brancher locates the brancher whose port leads to it, and the lowering registers tinyagents::GraphBuilder::add_barrier_relief(brancher, predecessor, merge) so the barrier can clear on its remaining predecessors when that branch goes untaken, without ever weakening it into a plain edge. Adds tests/eng5_barrier_relief_e2e.rs: the mixed fan-in repro (both untaken- and taken-branch directions, the latter guarding against data loss), plus pure-conditional-join and pure-unconditional-fan-in regressions. Depends on tinyagents' new add_barrier_relief primitive (companion change in the tinyagents submodule/crate, not yet published — this change is not yet buildable against a released tinyagents version).
Extends the eng5_barrier_relief_e2e coverage with a graph where the conditional predecessor sits TWO hops behind the brancher (cond--true-->x--main-->a, x a plain pass-through) instead of one, per the flagged gap: the previous same-superstep relief check fired a premature phantom arrival for `a` on `cond`'s own completion — before `x` (let alone `a`) had run — even on the taken branch, silently dropping `a`'s item from the merge. Confirmed multi_hop_conditional_taken_no_data_loss fails against the prior tinyagents logic (m ends up with only c's item) and passes against the resolved-routing-target fix (tinyagents commit "fix(graph): key barrier relief on resolved routing target, not same-superstep scheduling"). multi_hop_conditional_not_taken_completes pins the untaken-branch direction stays deadlock-free too.
The mixed fan-in fix (previous commit) depends on tinyagents's new add_barrier_relief primitive, published as tinyagents 2.1.0 (crates.io) — tinyagents's own major-version line moved to 2.x independently of this work, so this also crosses that boundary from the "1.2" this crate previously required. Verified tinyflows compiles and its full test suite (340 lib tests, all integration files, 12 doctests) stays green against the real published 2.1.0 — no other breaking changes between the two lines affect this crate.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Comment |
# Conflicts: # Cargo.lock
2 participants
Summary
Fixes a deadlock (Eng §5) in a mixed fan-in: a merge node with one predecessor unconditionally reachable and another reachable only via a conditional branch that this run doesn't take.
is_conditional_joinmade a binary all-plain-or-all-waiting decision for fan-in nodes: a merge was only treated as a "conditional join" (plain edges) when EVERY predecessor was reachable from a single common brancher's distinct ports. A mixed fan-in fell through to the all-waiting barrier, so the untaken branch's predecessor never arrived and the merge deadlocked forever.The naive fix (lower the conditional predecessor's edge as plain instead of waiting) causes silent data loss instead: when that branch IS taken, the merge fires off the superstep snapshot before the conditional predecessor's items commit.
Fix
Every fan-in edge is now lowered as a waiting edge unconditionally (removes
is_conditional_joinand the plain/waiting branch). Two new helpers classify predecessors instead of the join as a whole:has_unconditional_path— reachable from the trigger via"main"-port edges only (so it always runs, whether that's a plain pass-through or a parallel fan-out).conditional_predecessors— a fan-in node's direct predecessors that are NOT unconditionally reachable, i.e. sit behind a branch port.For each conditional predecessor,
find_conditional_brancherlocates the brancher whose port leads to it, and the lowering registerstinyagents::GraphBuilder::add_barrier_relief(brancher, predecessor, merge)(new intinyagents2.1.0, tinyhumansai/tinyagents#62) so the barrier can clear on its remaining predecessors when that branch goes untaken, without ever weakening it into a plain edge — including for a multi-hop conditional predecessor (brancher --branch--> x --main--> predecessor), whichadd_barrier_reliefresolves correctly by keying on the brancher's actual resolved routing target rather than same-superstep scheduling.Dependency
Depends on
tinyagents's newadd_barrier_reliefprimitive, published astinyagents2.1.0 on crates.io (tinyhumansai/tinyagents#62, merged and released). This crate'stinyagentsrequirement moves from"1.2"to"2.1"— that also happens to crosstinyagents's own independent 1.x→2.x major bump (unrelated to this fix); verified the full test suite compiles and passes against the real published 2.1.0 with no other breakage from that jump.Tests
New
tests/eng5_barrier_relief_e2e.rs:mixed_fan_in_conditional_not_taken_completes/mixed_fan_in_conditional_taken_includes_both_items— the direct (one-hop) repro, both directions; the "taken" test is the data-loss guard.multi_hop_conditional_taken_no_data_loss/multi_hop_conditional_not_taken_completes— the same repro with the conditional predecessor two hops behind the brancher instead of one.pure_conditional_join_regression/pure_unconditional_fan_in_regression— pin the two cases the oldis_conditional_joinspecial-cased, now handled uniformly.Full existing suite green:
tests/parallel_e2e.rs,tests/hardening_branching_e2e.rs(incl.bug4a_untaken_port_data_does_not_leak_into_fan_in),tests/branching_e2e.rs, all other integration tests, engine.rs inline tests, doctests — 340 lib tests + all integration files + 12 doctests, 0 failures.