fix(python): bound FREED_POOL_IDS to stop unbounded memory growth in …

[CapThunder19]

Summary

FREED_POOL_IDS (a process-global set in apis/python/node/src/lib.rs) tracks freed GPU memory-pool buffer IDs so the DORADMA fast path can detect read-after-free. Every free_memory_pool call inserted into this set, but nothing ever removed entries in normal operation the set grew for the entire lifetime of the process.

Because buffer_id is derived from a monotonic per-process counter that never repeats, a node doing register write free once per frame (the exact zero-copy GPU streaming use case this feature targets, e.g. a camera node at 30-60Hz) added one permanent entry per frame, with no upper bound. At 30fps that's ~2.6M entries/day an unbounded memory leak in exactly the long-running production pipelines dora is meant to support.

This PR bounds FREED_POOL_IDS with a small FIFO-eviction wrapper (FreedPoolIds), capped at FREED_POOL_IDS_CAP (4096) entries, since only recently-freed buffers are relevant for read-after-free detection.

Root cause

• apis/python/node/src/lib.rs, previously FREED_POOL_IDS: LazyLock<Mutex<HashSet<String>>> insert-only, no eviction.
• The one removal path (clearing the tombstone on a successful fast-path read) only fires when a receiver re-reads the exact same buffer_id, which requires the same node_id + counter pair effectively impossible during normal streaming since the counter never repeats.
• Every other per-pool cache in free_memory_pool (PINNED_POOL, TRANSIT_META, RECV_GPU_VA, RECV_CPU_SHMEM, GPU_BUF_SIZES) is explicitly cleaned up with comments calling out leak-avoidance this one was missed.

Fix

Replaced the plain HashSet<String> with FreedPoolIds, a small struct pairing a HashSet<String> (O(1) membership) with a VecDeque<String> (insertion order), evicting the oldest entry once the set exceeds FREED_POOL_IDS_CAP. No new dependencies, no protocol/API changes scoped entirely to this one file.

Testing

Added freed_pool_ids_tests (unit tests, co-located with the struct, matching this file's existing seqlock_tests convention):

• insert_past_cap_does_not_grow_unbounded regression test for the leak: inserting 4x the cap keeps len() bounded at exactly the cap.
• insert_past_cap_evicts_oldest_first confirms FIFO eviction (oldest gone, newest retained).
• duplicate_insert_is_idempotent re-inserting a tracked ID doesn't inflate the count or eviction queue.
• remove_drops_membership_and_order_entry covers the existing tombstone-clear path and re-insertion after removal.

#3032

[trunk-io]

😎 Merged successfully - details.

[phil-opp]

Automated review — no issues found.

I reviewed the bounded-tracking change. The FreedPoolIds wrapper preserves the exact method surface (insert / contains / remove / len) used by all four call sites, so the substitution is transparent and no caller depended on the old HashSet::insert bool return. Eviction is safe given buffer IDs are made unique per registration (counter suffix): an evicted old tombstone can never be re-queried by a later registration, so FIFO eviction of the oldest entries cannot cause a false-negative read-after-free on any live path. The four tests are meaningful — in particular remove_drops_membership_and_order_entry guards the stale-order-queue bug that a naive remove would introduce, and insert_past_cap_does_not_grow_unbounded is a real regression test for the original leak.

Minor observation only (not blocking): remove's order.retain(...) is O(n), but it's gated on the ID actually being present, so it's rare and off the hot path.

---

🤖 Fully automated review by Claude (Claude Code) — no human has reviewed these findings. Please verify before relying on them.

Generated by Claude Code

---

Generated by Claude Code

[phil-opp]

The eviction policy is sound and neither obvious hazard materialises: FREED_POOL_IDS is a read-after-free tombstone set rather than a free-list, so evicting an entry cannot cause a double free, and an evicted tombstone degrades to the same user-visible warn_missing_memory_pool path (the write fast path uses ShmemConf::open, not create). remove's O(n) order.retain is gated behind set.remove(id) returning true, so the hot call stays O(1). Two notes:

1. The four regression tests never execute. dora-node-api-python is excluded from cargo test in .github/workflows/ci.yml:229 and nightly.yml:1192/1833, and from clippy at ci.yml:144. The only job touching the crate is cargo check --all, which does not build #[cfg(test)] modules — so freed_pool_ids_tests is neither compiled nor run anywhere, and a compile error in it would ship. Pre-existing (the file's seqlock_tests are in the same position), but it means the regression test carries no enforcement.

2. FREED_POOL_IDS_CAP is one global budget shared across every peer the process reads from, so a 60Hz sender evicts a 1Hz sender's tombstones in about 68s. Consequence is benign per the above, but the doc comment implies a per-stream recency window it does not actually provide.

No collision with #3056 beyond a trivial textual one (adjacent lines); semantically #3056 strengthens this. No interaction with #3014.

[github-actions]

@CapThunder19 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]

🤖 Fully automated review by Claude — this review was generated end-to-end by an automated agent with no human vetting. Treat it accordingly.

Re-checked at 09644ae, the one docs-only commit since the last review: the FREED_POOL_IDS doc now states the cap is a single cross-peer budget rather than a per-stream recency window, closing the doc/behaviour mismatch noted earlier. No code change, and the FreedPoolIds logic still holds up — set/order stay in sync on insert (one eviction per insert, guarded by the set.insert bool) and on remove, and evicting a read-after-free tombstone only degrades a stale fast-path read to the existing warn_missing_memory_pool/daemon fallback, never a false-negative on a live buffer id (ids are counter-unique and never reused).

The one point still open is structural and pre-existing: dora-node-api-python is excluded from cargo test in ci.yml/nightly.yml, and cargo check --all doesn't build #[cfg(test)] modules, so freed_pool_ids_tests is neither compiled nor run in CI — a compile error in it would ship. Not introduced by this PR, but it means these regression tests currently carry no enforcement. No new issues found.

---

Generated by Claude Code