feat(graph): BarrierRelief primitive for mixed conditional/unconditional fan-in#62
Merged
Conversation
A waiting/barrier node (add_waiting_edge) normally activates only once every registered predecessor has arrived. When one predecessor is only reachable via a conditional branch, and the brancher routes elsewhere, that predecessor never arrives and the barrier deadlocks forever. Add GraphBuilder::add_barrier_relief(source, relief_node, barrier_node): when `source` completes a superstep without routing to `relief_node`, route_completed registers a phantom arrival of `relief_node` at `barrier_node`, letting the barrier clear on its remaining predecessors instead of hanging. This fixes the deadlock without weakening the barrier into a plain edge, which would let a *taken* branch's data race past the merge and be silently dropped (a stale-snapshot data loss bug). Threaded through GraphBuilder -> CompiledGraph::from_parts (compiled- graph metadata; no checkpoint/serialization changes needed since barrier_arrivals already persist). Adds barrier_relief_fires_when_source_skips_relief_node covering the fix. Part of the tinyflows Eng §5 mixed fan-in deadlock fix (companion change in tinyflows/src/engine.rs).
…uperstep scheduling barrier_relief_fires_when_source_skips_relief_node covered only the direct (one-hop) case: `source --branch--> relief_node`. The check — "is relief_node freshly scheduled into next this step" — is a same- superstep proxy that breaks for a multi-hop conditional predecessor (`source --branch--> x --main--> relief_node`, x a plain pass-through): relief_node is not in next on the step source itself completes, even on the TAKEN branch, since x hasn't run x yet. The relief fired a premature phantom arrival regardless, letting the barrier clear before the real predecessor's data ever committed — reintroducing the exact data-loss bug BarrierRelief exists to prevent. Fix: resolve source's actual routing target(s) this step (reusing the existing pure self.route()) and walk them forward through the compiled graph's deterministic (single-successor) edges via a new reaches_deterministically helper, stopping at barrier_node. This answers "was the branch leading to relief_node taken" directly, correct for any chain length of plain/waiting-edge pass-throughs — not just one hop. Keeps the same-superstep next_seen check as a defensive fallback. A further conditional/command node between source and relief_node (no deterministic self.edges entry) is a second runtime decision this walk cannot resolve ahead of time and conservatively reports unreachable — documented as a residual limitation in the new helper's doc comment.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds barrier relief registration to graph building and compiled graph storage. Completion routing recognizes conditionally skipped predecessors, records phantom arrivals, and can activate mixed fan-in barriers without executing skipped relief nodes. ChangesBarrier relief support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GraphBuilder
participant CompiledGraph
participant route_completed
participant Barrier
GraphBuilder->>CompiledGraph: compile barrier relief registrations
CompiledGraph->>route_completed: provide stored relief configuration
route_completed->>route_completed: resolve source routing and reachability
route_completed->>Barrier: register phantom arrival and schedule activation
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/graph/compiled/routing.rs`:
- Around line 87-114: Cache the targets returned by the first routing pass in
the relevant activation-processing flow, keyed by activation/index, and reuse
them in the barrier-relief loop around barrier_reliefs and
reaches_deterministically instead of calling self.route again. Preserve the
existing branch-taken and relief behavior, and add a regression test verifying a
stateful or nondeterministic router is invoked only once per activation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d430cf0-d6a0-4f1f-9594-09ac056625f7
📒 Files selected for processing (6)
src/graph/builder/mod.rssrc/graph/builder/types.rssrc/graph/compiled/mod.rssrc/graph/compiled/routing.rssrc/graph/compiled/test.rssrc/graph/compiled/types.rs
Addresses @coderabbitai on PR #62 (routing.rs:87-114): the barrier-relief pass called self.route() a second time for the same activation to get its resolved targets, which would invoke a stateful/nondeterministic router closure twice for one activation. Cache each activation's resolved targets (already computed once in the main completed loop) and have the relief pass index into that cache instead of recomputing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
GraphBuilder::add_barrier_relief(source, relief_node, barrier_node), a new primitive for the durable graph's waiting/barrier (fan-in) mechanism.A waiting/barrier node ([
add_waiting_edge]) normally activates only once every registered predecessor has arrived — possibly across supersteps. That's correct for a plain diamond or parallel fan-out, but breaks for a mixed fan-in: one predecessor unconditionally reachable, another reachable only via a conditional branch. If the brancher doesn't take that branch, the conditional predecessor never arrives and the barrier deadlocks forever.The naive fix — lowering that predecessor's edge as plain instead of waiting — reintroduces a different, worse bug: when the branch IS taken, the merge can fire off the superstep snapshot before that predecessor's items commit, silently dropping them (a stale-snapshot data race).
add_barrier_relieffixes the deadlock without weakening the barrier:sourcecompletes a superstep, the executor resolvessource's actual routing target(s) this step and walks them forward through the compiled graph's deterministic (single-successor) edges to see whetherrelief_nodeis reachable beforebarrier_node(this also correctly handles a multi-hop conditional predecessor, e.g.source --branch--> x --main--> relief_node, not just a direct one-hop edge).relief_nodewas not taken), it registers a phantom arrival ofrelief_nodeatbarrier_node, letting the barrier clear on its remaining real predecessors.relief_node's real arrival, whenever it lands, so no data race is possible.Non-breaking
Purely additive:
BarrierReliefstruct andGraphBuilder::add_barrier_reliefmethod.barrier_reliefs: Arc<Vec<BarrierRelief>>field onCompiledGraph, defaulting to empty when unused — every existing caller ofGraphBuilder/CompiledGraph::from_partscompiles and behaves identically since no existing graph registers a relief.barrier_arrivalspersistence already covers the runtime side.Tests
barrier_relief_fires_when_source_skips_relief_node— direct (one-hop) case: a barrier relieved when the brancher never routes to the relief node.Context
Companion fix for a
tinyflowsengine bug (mixed/multi-hop conditional fan-in deadlock — Eng §5). ThetinyflowsPR building on this primitive follows once this merges.Summary by CodeRabbit