Give Node.Run real cancellation instead of only abandoning stragglers at the deadline - #5
Merged
Merged
Conversation
…instead of only abandoning stragglers
Previously, when a graph never terminated on its own (a genuinely
unbounded loop, or a hub like Constant that re-emits forever by
design), runAll's only option at the deadline was to abandon the still
-running node goroutines outright -- they'd keep running in the
background, touching whatever they touch (package-global TraceLevel,
RunTime, etc.) for as long as the process lives. That's what caused
flowgraph's TestDotNaming/TestChain race: a straggler goroutine from a
Constant->Sink graph reading TraceLevel while the next sequential test
wrote it.
Now:
- runAll creates one `quit` channel per graph, shared by every node.
- Node.Init appends it as one more reflect.SelectCase, so RecvOne's
blocking select wakes on it exactly like it wakes on real data/acks.
RecvOne treats quitCase specially -- return false (same exit path as
a closed data channel) without logging it as a bad receive.
- Edge.SendAck / Edge.SendData -- the two places a node blocks on a
raw channel send outside of RecvOne's select -- now race that send
against the same quit channel, so a node caught mid-send at
shutdown doesn't just hang there either.
- At the RunTime deadline, runAll closes quit (asking every node to
stop) and gives them a short bounded grace period (quitGrace,
100ms) to actually exit before falling back to the old abandon-at
-the-deadline behavior.
A node with a genuinely unbounded fire loop that never reaches
RecvOne or a blocking send (spinning entirely inside Fire/RdyAll) still
won't notice quit and still gets abandoned -- unchanged from before
this mechanism existed. In practice, with the default ChannelSize=1,
every node in this codebase's normal usage cycles through RecvOne or a
blocking send regularly (backpressure forces it), so this closes the
gap for the common case without claiming to solve the general one.
Verified:
- Standalone probe (Constant->Sink, matching TestDotNaming's graph):
0 leaked goroutines after Run() returns, vs. leaking both node
goroutines indefinitely before this change.
- flowgraph's full test suite: all 24 subtests still pass.
- flowgraph's TestChain/TestDotNaming race, run 5x under -race: 0
warnings, was reliably reproducible before.
- flowgraph's full suite under -race, run 2x: 6 warnings both times
(down from 11-16, noisy, before) -- all 6 trace to two things this
change was never meant to fix: the pre-existing flowgraph.go
waitRdy/RdyCnt race (flagged since before any of this work started),
and a newly-visible race in TestDuckPondB's steerDuck test fixture
(shared state written by one live node while read by another,
within the same still-running graph -- not a shutdown straggler).
3 tasks
… deps func_decrypt.go already needed Pre-existing gap, unrelated to this branch's work -- picked up incidentally while verifying builds here. func_decrypt.go has always imported golang.org/x/crypto/nacl/box without go.mod declaring it, so a standalone go build ./... failed even on master.
…cvOne-only The prior commit made every SendAck/SendData call select-aware against quit, on the reasoning that a node caught mid-send when its peer already exited would otherwise hang forever. That's true, but wrong to guard against: RdyAll()/DstRdy() already proves room exists (via RdyCnt, counted from acks already received) before Fire() is ever called, so the bare sends here were never speculative -- they were already certified safe by the ack protocol before this commit added a redundant check. Benchmarked at ~28-48ns/op vs bare's ~20ns/op (measured, see PR discussion) -- a real, if small, tax added to every send in every graph, permanently, to guard a case that only exists in the one-shot teardown window. A node stuck mid-send when its peer has already exited isn't a bug to fix -- a torn-down flowgraph's in-flight state was never meant to survive the teardown, the same way a register's contents don't survive a power cut. RecvOne's quit case already handles the case that matters: a node actively cycling (repeatedly touching package-globals like TraceLevel on every pass) stops as soon as it's idle-waiting and notices quit. A node that gets stuck mid-send instead is frozen, not cycling -- it already did whatever it was going to do before blocking, and won't touch shared state again. Verified this is enough on its own: flowgraph's TestChain/TestDotNaming race, run 10x under -race with this revert in place: 0 warnings, all 10. A standalone goroutine-count probe (Constant->Sink, matching TestDotNaming's graph) shows leaking is actually the common case here (6/8 runs) given how tight a 2-node ChannelSize=1 cycle is -- yet the race stays gone, because the leaked goroutines are inert, not actively touching anything by the time they're stuck.
…/v0.8.0), not the latest go get without a version pin grabbed the latest x/crypto/x/sys, which require a newer Go toolchain than this module's go 1.20 to even compile their own source -- broke CI. Pinned to the same versions flowgraph already uses successfully.
…vOne Makes the invariant that this branch's whole design turns on discoverable from the code itself, not just from having built the ack-counting protocol and learned it firsthand: RdyCnt/DstRdy prove room exists, from acks already received, before Fire ever runs -- so SendData/SendAck's sends were never speculative, and wrapping them in select re-derives at runtime (on every call, at real measured cost) what this counter already decided once, for free. Also documents why quit is RecvOne-only: a node stuck mid-send after its peer already exited isn't a leak worth chasing -- a torn-down flowgraph's in-flight state was never meant to survive teardown. What matters is stopping a node that's actively cycling (touching package state like TraceLevel on every pass), which RecvOne's quit case already does, for free, since select was in that path before quit existed.
vectaport
marked this pull request as ready for review
July 18, 2026 18:06
Greptile SummaryThis PR adds real cancellation to
Confidence Score: 5/5Safe to merge — the quit mechanism is correctly wired, quitCase indices stay stable because cases are nulled rather than removed, and the bare SendData/SendAck sends are sound given RdyCnt's pre-proven capacity guarantee. The core change is narrow and well-contained: a single channel appended to each node's existing select, detected by a stable index, closed once at deadline. The SendData/SendAck reasoning is correct and thoroughly documented. No shared mutable state is introduced; quit is local to each runAll invocation. No files require special attention. The only subtle point worth a second look is the now-unreachable len(n.cases) == 0 guard in RecvOne (node.go line 592), but it has no effect on any currently exercised code path. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant runAll
participant Node
participant wg
runAll->>Node: assign quit chan, call Init
Note over Node: quit appended as last SelectCase
runAll->>wg: Add(len(nodes))
runAll->>Node: launch goroutines
alt graph terminates naturally
Node->>wg: Done x N
wg->>runAll: done channel closes
runAll->>runAll: return normally
else RunTime deadline fires
runAll->>Node: close(quit)
Node->>Node: reflect.Select picks quitCase
Node->>Node: RecvOne returns false
Node->>wg: Done
alt exits within quitGrace 100ms
wg->>runAll: done channel closes
else grace expires
runAll->>runAll: abandon stragglers
end
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant runAll
participant Node
participant wg
runAll->>Node: assign quit chan, call Init
Note over Node: quit appended as last SelectCase
runAll->>wg: Add(len(nodes))
runAll->>Node: launch goroutines
alt graph terminates naturally
Node->>wg: Done x N
wg->>runAll: done channel closes
runAll->>runAll: return normally
else RunTime deadline fires
runAll->>Node: close(quit)
Node->>Node: reflect.Select picks quitCase
Node->>Node: RecvOne returns false
Node->>wg: Done
alt exits within quitGrace 100ms
wg->>runAll: done channel closes
else grace expires
runAll->>runAll: abandon stragglers
end
end
Reviews (2): Last reviewed commit: "update year on LICENSE" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+3
to
+4
| golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= | ||
| golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= |
There was a problem hiding this comment.
go.sum contains checksums for golang.org/x/crypto v0.54.0 and golang.org/x/sys v0.47.0, but go.mod only requires v0.9.0 and v0.8.0 respectively. These appear to have leaked in from testing with a flowgraph replace directive. While harmless for correctness (Go always picks the minimum required version), the file should be reconciled with go mod tidy before merge to keep the module graph clean.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+598
to
+612
| if i == n.quitCase { | ||
| // quit closed by runAll asking this Node's Run to return -- expected | ||
| // shutdown, not a bad receive. | ||
| // | ||
| // This is the only place quit is checked -- SendData/SendAck stay | ||
| // bare (see their comments). A node caught mid-send when its peer | ||
| // has already exited just stays blocked; that's fine, not a leak | ||
| // worth chasing. A torn-down flowgraph's in-flight state was never | ||
| // meant to survive teardown, any more than a register's contents | ||
| // survive a power cut -- what matters is that a node actively | ||
| // cycling (touching package state on every pass, e.g. TraceLevel) | ||
| // stops promptly, which happens here, for free, since select was | ||
| // already in this path before quit existed. | ||
| return false | ||
| } |
There was a problem hiding this comment.
The PR description states: "Edge.SendAck/Edge.SendData — the two places a node blocks on a raw channel send outside of RecvOne's select — now race that send against the same quit channel too." In the actual code, these sends are deliberately kept bare (no select against quit), as the comment here confirms: "This is the only place quit is checked — SendData/SendAck stay bare."
The implementation is functionally correct (bare sends are safe because DstRdy proves capacity before Fire runs), but the description will mislead anyone reading it alongside the code. The description should be updated to reflect that only RecvOne watches quit, and that nodes blocked in mid-send after their peer has already exited are abandoned at the grace-period boundary rather than interrupted.
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
runAllpreviously had no way to tell a still-running graph's node goroutines to stop when theRunTimedeadline fired -- it just returned and let them keep running in the background for as long as the process lived. That's what caused theTestChain/TestDotNamingrace fixed in #4: a straggler goroutine from aConstant->Sinkgraph (which never terminates on its own, by design) reading the package-globalTraceLevelwhile the next sequential test wrote it. #4 fixed the terminating-graph case (wg.Wait()racing the deadline); this fixes the non-terminating case.runAllcreates onequitchannel per graph, shared by every node.Node.Initappends it as one morereflect.SelectCaseonton.cases-- the same dynamically-sized case listRecvOnealready builds and blocks on for every source-data and dest-ack edge.RecvOnedoesn't get a new wait mechanism;quitjust becomes one more thing the select it was already making was watching for.RecvOnetreats that case specially -- returnfalse(the same exit path as a closed data channel) without logging it as a bad receive.runAllclosesquit(asking every node to stop) and gives them a short bounded grace period (quitGrace, 100ms) to actually exit before falling back to the old abandon-at-the-deadline behavior.Edge.SendData/Edge.SendAckstay exactly as they were: bare, unconditional channel sends, noselect, noquitawareness. This was not the original shape of this PR -- see "What this PR tried and reverted" below.A node with a genuinely unbounded fire loop that never reaches
RecvOne(spinning entirely insideFire/RdyAll) still won't noticequitand still gets abandoned -- unchanged from before this existed (seeexamples/loop*.go). In practice, with the defaultChannelSize=1, every node in this codebase's normal usage cycles throughRecvOneregularly (backpressure forces it), so this closes the gap for the common case.Why SendData/SendAck stay bare -- what this PR tried and reverted
The first version of this PR wrapped every
SendData/SendAckchannel send in aselectagainstquittoo, reasoning that a node caught mid-send when its peer already exited would otherwise hang forever. That reasoning was backwards.RdyAll/DstRdyalready prove room exists -- viaRdyCnt, counted from acks already received -- beforeFireis ever called. So these sends were never speculative; they're certified safe by the ack protocol before this PR ever gets involved. Wrapping them inselectre-checked, at real cost on every single send in every graph, something the protocol had already decided once, for free. Benchmarked: bare send ~20ns/op vs. a non-blocking-select-then-fallback fast path ~28ns/op vs. an unconditional 2-case blocking select ~48ns/op -- a permanent tax to guard a case (peer already gone) that only exists in the one-shot teardown window, not in normal operation.The actual resolution: a node stuck mid-send after its peer has already exited isn't a leak worth chasing. A torn-down flowgraph's in-flight state was never meant to survive teardown, any more than a register's contents survive a power cut. What matters is stopping a node that's actively cycling -- repeatedly touching package-global state like
TraceLevelon every pass -- andRecvOne'squitcase already does exactly that, for free, since it's not adding a wait point that wasn't already there. A node frozen mid-send isn't cycling; it already did whatever it was going to do before blocking, and won't touch shared state again.RdyCnt,DstRdy, and both send sites now have comments documenting this so it doesn't need rediscovering.Verified
Constant->Sink, matchingTestDotNaming's graph): with bare sends restored, leaking is actually the common case for this specific tight 2-node/ChannelSize=1cycle (6/8 probe runs left 2 goroutines parked on a blocked send) -- yet:TestChain/TestDotNamingrace, run 10x under-race: 0 warnings, all 10 -- confirming the leaked-but-frozen goroutines aren't the ones that were ever racing.-race: races trace only to two things this PR was never meant to fix -- the pre-existingflowgraph.gowaitRdy/RdyCntrace (flagged since before this work started), and a newly-visible race inTestDuckPondB'ssteerDucktest fixture (a genuinely different, live-execution race between two still-running nodes in the same graph, not a shutdown straggler).Test plan
go build ./go vet .cleango.modalso fixes a pre-existing missing indirect dep (func_decrypt.goneededgolang.org/x/crypto/nacl/box), pinned to Go-1.20-compatible versionsgo.modreplace directive and pinned-commit CI, as aboveNode/Edgemechanics. flowgraph#5 tracks this branch's commits for CI verification; once merged, flowgraph'sgo.modshould get bumped to master as a follow-up.