memory-pool: cross-process `FreeMemoryPool` cleanup can be silently skipped (full-channel drop + read-with-free path)

[phil-opp]

This issue was created by a scheduled, automated Claude code-review check that looks at recently merged PRs. The findings were verified against the source, but please double-check before acting on them.

Summary

PR #2619 (feat(memory-pool): GPU-to-GPU adaptive transport…, commit e99ba21) added cross-process cleanup: when a node calls free_memory_pool, the daemon broadcasts NodeEvent::FreeMemoryPool to every other node that registered or read the pool, and each recipient releases its per-process GPU/host/IPC/shmem resources.

Two paths defeat that guarantee and reintroduce the per-process leak the feature is meant to prevent.

Bug 1 — broadcast is silently dropped when the receiver's event channel is full

binaries/daemon/src/lib.rs, FreePinnedMemory handler (~line 4025):

for (node, channel) in &dataflow.subscribe_channels {
    if touched.contains(node.as_ref())
        && node.as_ref() != node_id.as_ref()
        && let Err(e) = send_with_timestamp(channel, event.clone(), &self.clock)
    {
        tracing::warn!(/* failed to deliver FreeMemoryPool */);
    }
}

send_with_timestamp (binaries/daemon/src/event_types.rs:213) returns Ok(false) — not Err — when the channel is full (it try_sends and logs "CRITICAL: control event dropped despite headroom reservation"). The loop above only inspects the Err (channel-closed) case, so an Ok(false) drop is neither retried nor surfaced by this site. When it happens, the recipient never runs process_pending_memory_pool_frees, and its GPU buffer / pinned+transit host buffer / IPC handle / shmem mapping for that pool leak for the life of the process.

Note the sibling send_reload loop (~line 4060) explicitly handles the Ok(false) "event dropped (channel full)" case; this new site does not.

Suggested fix: match on the bool, e.g. warn (and/or retry) on Ok(false) as well as Err.

Bug 2 — the read-with-free path frees the pool but never broadcasts

binaries/daemon/src/lib.rs, ReadPinnedMemory { free: true } handler (~line 3972):

if free
    && let Err(err) = self.memory_pool.free_memory_pool(&id, node_id.as_ref())
{
    tracing::warn!("Failed to free memory pool {} after reading: {}", …);
}

free_memory_pool was changed by this PR to return (metadata, touched_by), and the new FreePinnedMemory handler uses touched_by to broadcast the cleanup. But this read-and-free path discards the Ok((metadata, touched)) tuple and performs no broadcast. So freeing a pool through ReadPinnedMemory { free: true } releases the daemon table entry and unlinks the shmem, yet leaves every other touched process holding its per-process resources — the exact leak the PR set out to fix, reintroduced via the other free entry point.

read_pinned_memory(id, free=true) is a public Rust node API (apis/rust/node/src/node/mod.rs:2358). The Python API currently always passes free=false, so the Python feature does not trigger this today, but the two free entry points are inconsistent.

Suggested fix: factor the "broadcast to touched nodes (excluding initiator)" logic into a shared helper and call it from both the FreePinnedMemory handler and the ReadPinnedMemory { free: true } path.

Related lower-severity observations (same subsystem)

• PENDING_FREES never drained for non-Python nodes. apis/rust/node/src/event_stream/thread.rs always intercepts FreeMemoryPool and pushes into the process-global PENDING_FREES set, but drain_freed_pools() is only ever called by the Python node API — there is no Rust-side caller. A pure-Rust node that reads a pool will accumulate entries that are never drained (bounded by distinct pool count; strings only). It is effectively dead wiring for the Rust API.
• Cleanup is only drained at the top of next() (apis/python/node/src/lib.rs:1259). A receiver that finishes its input loop (e.g. the sender frees the pool as its last action and no further inputs arrive) never drains the pending free, so its GPU/transit buffers are held until the process exits — a timeliness gap for scarce VRAM rather than a hard leak.

This is distinct from #2881 (daemon-side pool tracking not cleaned on RemoveNode); the paths above concern the new explicit-free broadcast added in #2619.

[phil-opp]

The two main bugs here look already resolved on main.

Bug 1 (full-channel Ok(false) drop): the broadcast now lives in notify_memory_pool_freed (binaries/daemon/src/lib.rs:776), which matches on the bool and records the Ok(false) full-channel case as undelivered (explicitly citing #2935 at lib.rs:798-803); the caller surfaces those nodes via tracing::error!.

Bug 2 (read-with-free path doesn't broadcast): both free entry points now route through a shared free_pool_and_notify helper (lib.rs:739) — the FreePinnedMemory handler (lib.rs:4873) and the ReadPinnedMemory { free: true } path (lib.rs:4834) — so freeing via either point notifies every touched node. The helper's doc comment calls out #2935 directly.

The two "related lower-severity observations" (PENDING_FREES never drained for pure-Rust nodes; cleanup only drained at the top of next()) are separate timeliness/dead-wiring notes rather than the hard cross-process leak; I'll leave those for maintainer judgment. The core cross-process leak described here no longer reproduces, so this can likely be closed.

---

Generated by Claude Code

[phil-opp]

Removed from the 1.0 milestone: the memory-pool transport is being parked out of the tree in #3152, so this no longer blocks 1.0.

The code is preserved under external/dora-pool, staged for lifting into its own repository. This bug is recorded in its README as a re-entry criterion — it must be fixed before the transport comes back. Not closing it: the defect is real and still unfixed.

[phil-opp]

Correction to my earlier note on this issue. The memory-pool transport is not being moved out of the repository after all.

It now lives at libraries/extensions/memory-pool as an opt-in feature (--features memory-pool, off by default) that is explicitly outside the 1.0 compatibility guarantees — see #3152. It reaches dora only through a new generic extension channel, so none of it is in dora's stable surface.

This stays off the 1.0 milestone for that reason, and stays open: the defect is real and unfixed. It is named in the extension's README as a known open defect.