fix(daemon): reclaim memory pools once no live node can reach them - #3014
Conversation
|
😎 Merged successfully - details. |
|
🤖 Automated review by Claude — fully automated review, no human in the loop; please verify before acting. I reviewed the current diff. No issues found. What I traced:
Generated by Claude Code |
|
@phil-opp the Trunk merge queue failed for this PR. See the Trunk merge-status comment for details. Posted as a new comment so GitHub sends an email — Trunk's sticky comment is edited in place and won't trigger a notification. |
|
@phil-opp the Trunk merge queue failed for this PR. See the Trunk merge-status comment for details. Posted as a new comment so GitHub sends an email — Trunk's sticky comment is edited in place and won't trigger a notification. |
…2881) Pools registered by a node stayed in the daemon's table until the daemon itself exited: neither `RemoveNode` nor a node crash released them, and `finish_dataflow` did not either, so a long-running daemon (`dora up`) accumulated table entries and /dev/shm segments across dataflows. Eagerly freeing a node's pools when it goes away is not correct, as pointed out on the issue: a pool deliberately outlives its registrar — the normal lifecycle is that a sender registers a pool and a *receiver* reads and frees it, possibly well after the sender exited. So reclaim by reachability instead. Each entry now records who can still reach it: the nodes that have opened it (`touched_by`) plus the registrar's transitive downstream consumers as of registration time, which can still learn the pool id from a message already in flight. A pool is released once none of those is running locally, which every path that ends an incarnation now checks — `RemoveNode`, `ReplaceNode`, and a crash, clean exit or restart. `finish_dataflow` releases the rest unconditionally, since a finished dataflow has no node left to serve. Remote nodes never count as live: pools are host-local shared memory and every daemon keeps its own table, so a consumer on another daemon can never open this one's segments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`cleanup_orphans` unlinked every `dora_pool_{dataflow_id}_*` segment on the
host. `/dev/shm` is host-wide but the name carries only the dataflow id, and
both call sites are per-daemon: with two daemons serving one dataflow on one
machine, the spawn-time sweep races the peer's spawn and the finish-time one
runs while the peer may still be going — so a finishing daemon destroyed the
peer's live segments and left its consumers with ENOENT.
Sweep only the segments of nodes this daemon owns. `RunningDataflow` records
them (`spawn_nodes` plus anything `AddNode` brings later), and the node id is
recoverable from the segment name, whose last component is always the
counter. A name that does not fit the pattern is left alone: leaving an
orphan is recoverable, unlinking a peer's live segment is not.
The finish-time release moves into a free function so it can be tested
without a whole `Daemon` — covering both that it drops the table entries a
finished dataflow still holds, and that its sweep spares a co-located
daemon's segments.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`potential_readers` was fixed at registration time, which is stale the moment the graph changes. `dora graph connect S/out C/in` after `S` registered a pool gives `C` a path to it, but `S` is in its own reader set and reclamation fires on its exit — so the segment was unlinked while `C` still had the id in flight. Before pools were reclaimed at all that transfer worked, so this was a regression introduced with the reclamation itself. `AddMapping` and `AddNode` — the two paths that add an edge to a running dataflow — now widen the reader set of every pool the edge's source can reach by the target's downstream closure. Recomputing the whole set at reclaim time is not possible (`RemoveNode` tears the mappings down first), but updating where edges are created covers the same ground. The closure itself was also short: it walks `mappings`, which holds only edges this daemon delivers, so a `local -> remote -> local` chain stopped at the first hop and lost the local node at the end — while a pool id travels the whole chain. Remote edges are recorded at spawn and followed. A remote node never counts as live, so following through one only widens what the closure can reach. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a8d2326 to
5a5712e
Compare
|
Rebased onto main and addressed all three points. The finish-time sweep crossing daemon boundaries. Correct, and the spawn-time The "known limitation" not covering its own case. Also correct, and it was a While in there: Test coverage of the finish path. The finish-time release is a free function now, Two residual gaps, both erring toward retention rather than premature release:
|
|
🤖 Automated review by Claude — fully automated, no human reviewed this; please verify before acting. The two commits since my last review address both issues raised on 2026-08-11, and I traced them against the diff:
No new issues. The two residual gaps you documented (runtime Generated by Claude Code |
Extracts the pinned/CUDA memory-pool transport (#2168, #2386, #2619) into external/dora-pool, staged for lifting into its own repository. dora 1.0 ships no pool API: the four Python methods, the three DaemonRequest/two DaemonReply variants, the daemon-side registry and the node-api plumbing are all removed. No seam is left behind — #1872 declined to commit to this architecture, so dora should not ship a socket moulded to its shape. The parked copy is the post-#3014 code (pool reclamation, #2881), so it carries that fix rather than the pre-fix state. external/dora-pool/README.md leads with a seam contract: a budget for any future reinstatement (<200 lines of dora, no new unsafe), a table of what may never return in-tree, and re-entry criteria — #1872's five unanswered design questions, the open correctness bugs, and a GPU CI story. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extracts the pinned/CUDA memory-pool transport (#2168, #2386, #2619) into external/dora-pool, staged for lifting into its own repository. dora 1.0 ships no pool API: the four Python methods, the three DaemonRequest/two DaemonReply variants, the daemon-side registry and the node-api plumbing are all removed. No seam is left behind — #1872 declined to commit to this architecture, so dora should not ship a socket moulded to its shape. The parked copy is the post-#3014 code (pool reclamation, #2881), so it carries that fix rather than the pre-fix state. external/dora-pool/README.md leads with a seam contract: a budget for any future reinstatement (<200 lines of dora, no new unsafe), a table of what may never return in-tree, and re-entry criteria — #1872's five unanswered design questions, the open correctness bugs, and a GPU CI story. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nd outside the 1.0 guarantees (#3152) * refactor: park the memory-pool transport out of the tree Extracts the pinned/CUDA memory-pool transport (#2168, #2386, #2619) into external/dora-pool, staged for lifting into its own repository. dora 1.0 ships no pool API: the four Python methods, the three DaemonRequest/two DaemonReply variants, the daemon-side registry and the node-api plumbing are all removed. No seam is left behind — #1872 declined to commit to this architecture, so dora should not ship a socket moulded to its shape. The parked copy is the post-#3014 code (pool reclamation, #2881), so it carries that fix rather than the pre-fix state. external/dora-pool/README.md leads with a seam contract: a budget for any future reinstatement (<200 lines of dora, no new unsafe), a table of what may never return in-tree, and re-entry criteria — #1872's five unanswered design questions, the open correctness bugs, and a GPU CI story. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(extensions): add a generic extension channel for out-of-tree transports A transport that lives outside dora still needs one thing only the daemon can provide: reclamation after a crash. A node that dies cannot withdraw the descriptor it published, so its readers keep mappings to memory nobody owns (#2881 is that failure mode with a real transport attached). Adds a dataflow-scoped table of opaque byte values whose lifetime the daemon brokers — store / load / drop, plus a drained notification when a key goes away. dora never interprets the namespace, key or value. Deliberately generic rather than shaped around any one transport: naming the protocol variants after the memory pool would freeze that architecture into dora, which #1872 explicitly declined to do. A second extension needs no change here at all. Guarantees: only the storing node may overwrite a key; entries are scoped per dataflow and per namespace; every node that stored or read a key is notified when it is dropped; a dropped key is reclaimed on owner exit and on dataflow finish; dropping an absent key is a no-op so retries are safe. Bounded at 8192 entries per dataflow and 4096 pending notifications per process. This is a control plane for descriptors, not a data plane — values are copied through the daemon. See docs/extensions.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(tensor-pool): reinstate the transport as an opt-in extension Keeps the feature and its Python API surface, but out of dora's core: it lives at libraries/extensions/tensor-pool (beside ros2-bridge, same python/ subcrate shape) and reaches dora only through the generic extension channel. Opt-in and off by default, two independent flags: maturin develop -m apis/python/node/Cargo.toml --features tensor-pool cargo build -p dora-daemon --features tensor-pool A default build neither compiles nor exposes it — the receive-path drain becomes an empty no-op and dora's core keeps zero pool vocabulary. NOT covered by the 1.0 compatibility guarantees, stated in the README, the crate description, the module header, every pymethod docstring and the .pyi stubs, along with the open defects (#3015, #2935, #2890). Named tensor-pool, not memory-pool: dora already has an unrelated shared_memory_pool_size descriptor key (and DORA_NODE_SHM_POOL_SIZE) for the Zenoh SHM buffer pool, and one name for both was a persistent source of confusion. 'gpu-' would have been the other obvious fix but is inaccurate — the CPU path works without CUDA and is the only one with CI coverage. This renames the four Python methods (register_memory_pool -> register_tensor_pool and likewise for write/read/free), the crates, the feature flags and the example env keys: a user-visible break, permissible because the feature sits outside the 1.0 guarantees, and cheaper now than later. The six former daemon calls now go through extension_store / extension_load / extension_drop / drain_dropped_extension_keys, with the descriptor encoded as JSON dora never parses (python/src/seam.rs). The unsafe pointer arithmetic, the seqlock and the embedded libcudart bindings stay on the extension's side. Returning the crate to the workspace put it under -D warnings for the first time, which surfaced three latent bugs: a deprecated downcast_into, a dead initializer in the device-to-host copy path, and seqlock_begin_write with no callers at all (every write path uses begin_if_even). All fixed. Known gap: the smoke tests need a feature-built wheel plus torch, so they ship as smoke-tests.rs.example rather than a cargo target that cannot compile. The extension has 53 unit tests but no in-tree end-to-end coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes #2881.
Pools registered by a node stayed in the daemon's table until the daemon itself
exited: neither
RemoveNodenor a node crash released them, andfinish_dataflowdid not either — so a long-running daemon (
dora up) accumulated table entries and/dev/shmsegments across dataflows, walking toward the 512-entry pool cap.Eagerly freeing a node's pools when it goes away would be wrong, as @SaitejaKommi
pointed out on the issue: a pool deliberately outlives its registrar. The normal
lifecycle is that a sender registers a pool and a receiver reads and frees it,
possibly well after the sender exited.
So reclaim by reachability instead. Each entry records who can still reach it:
touched_by— nodes that have opened the pool, andpotential_readers— the registrar's transitive downstream consumers as ofregistration time, which can still learn the pool id from a message already in
flight (the daemon cannot see pool ids inside payloads, so the whole downstream
closure counts).
A pool is released once none of those is still running locally. Every path that ends
an incarnation now checks:
RemoveNode,ReplaceNode(whose outgoing exit event thegeneration guard drops), and a crash, clean exit or restart.
finish_dataflowreleases the remainder unconditionally — a finished dataflow has no node left to
serve, and its routing, channels and listeners are discarded in the same breath.
Nodes on other daemons never count as live: pools are host-local shared memory and
each daemon keeps its own table, so a remote consumer can never open this one's
segments.
Because a reclaimed pool has no live node in
touched_by, there is nobody to send aFreeMemoryPoolnotification to — unlikefree_memory_pool, which must tell theother holders to drop their per-process buffers.
finish_dataflowalso sweeps the/dev/shmsegments no table entry covers — a nodethat died between creating a segment and registering it. That sweep is now scoped to
the nodes of this daemon, which it was not before:
/dev/shmis host-wide, thesegment name embeds only the dataflow id, and
finish_dataflowis a per-daemonfinish. With two daemons serving one dataflow on one machine (a
machine:split),whichever finished first would have unlinked the other's live segments, leaving a
consumer over there with
ENOENT.RunningDataflowtracks its local nodes for this,and the pre-existing spawn-time sweep — same hazard, narrower window — is scoped the
same way.
register_memory_poolgains apotential_readersparameter,MemoryPoolEntryafield, and
cleanup_orphansanis_local_nodepredicate, so this is a breakingchange to the
dora-memory-poolcrate surface. Its only consumer isdora-daemon.Reachability cannot be a registration-time snapshot, because a running dataflow can
be rewired.
dora graph connect S/out C/inafterSregistered a pool givesCapath to it, and
Sis in its own reader set, so reclamation onS's exit would haveunlinked the segment while
Cstill had the id in flight.AddMappingandAddNodetherefore widen the reader set of every pool the edge's source can reach, by the new
target's downstream closure. Recomputing the whole set at reclaim time is not an
option —
RemoveNodetears the node's mappings down first — but widening on the twopaths that create edges covers the same ground.
The closure also follows edges whose receiver is on another daemon.
mappingsholdsonly what this daemon delivers, so a
local -> remote -> localchain used to stop atthe first hop and drop the local node at its end, even though a pool id can travel
the whole way. A remote node never counts as live, so following through one costs
nothing.
Tests: reachability rules and dataflow-scoped cleanup in
dora-memory-pool; thedownstream closure (transitive, cycle-safe) on
RunningDataflow; and the daemonwiring — a pool survives an exited sender while its consumer runs, and is reclaimed
once the last consumer is gone, when the exiting node is its only reference, and
regardless of unrelated long-lived nodes. The finish path is covered too: it releases
a pool a live consumer could still reach, and its
/dev/shmsweep unlinks thisdaemon's orphan while leaving a co-located daemon's segment alone. Plus the two
reachability gaps: a consumer connected after registration keeps the pool alive until
it too is gone, and the closure reaches a local node whose only path runs through
another daemon.