fix(python-node): seed the pool-id counter randomly so restarts don't collide

[phil-opp]

Summary

Fixes #3015.

Memory-pool shared-memory names are dora_pool_{dataflow_id}_{node_id}_{counter}, where counter comes from a process-local static (PINNED_COUNTER) seeded at 0. Both dataflow_id and node_id are stable across a crash-restart, so a restarted node re-derives the exact name its previous incarnation used.

When the old pool is still live — the sender crashes while its receiver keeps reading, so #2881's reclaim deliberately retains it — ShmemConf::create() fails on the leftover /dev/shm segment, and the daemon independently rejects the registration as a duplicate. Under restart_policy: Always this is a crash-restart loop that never recovers.

#2881 fixed when pools are reclaimed; this is the separate question of pool ids not being unique across incarnations.

Change

• Seed PINNED_COUNTER with a random u64 per process (via the standard library's OS-seeded RandomState — no new dependency) so each incarnation derives a distinct name. The counter stays a plain u64, so both existing name parsers (the writer fast path and try_doradma_read) keep working unchanged — this is the direction the issue recommends.
• Increment via wrapping_add to guard the now-possible overflow from a near-max seed.
• Add unit tests: the seed is not constant, and a large (random-seeded) counter round-trips through the reader's rsplit_once('_') + parse::<u64>() name parser (including for a node_id that itself contains underscores).

Testing

• cargo test -p dora-node-api-python pool_id_tests — 2 passed
• cargo fmt --all -- --check — clean

---

Generated by Claude Code

[trunk-io]

Merging to main in this repository is managed by Trunk.

• To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

[phil-opp]

Automated review by Claude — fully automated, no human reviewed this before posting.

Reviewed the change that seeds PINNED_COUNTER with a random per-process u64 (via RandomState) and switches the increment to wrapping_add, so a crash-restarted node no longer re-derives its previous incarnation's still-live dora_pool_{dataflow}_{node}_{counter} shmem name. I confirmed the real reader fast path (try_doradma_read) parses the counter with buffer_id.rsplit_once('_'), so a large random-seeded counter and node_ids containing underscores both round-trip correctly — the on-wire id format is unchanged. No important issues found.

(Minor, non-blocking: large_counter_round_trips_through_the_name_parser reimplements the rsplit_once('_') parse inline rather than calling the production parser, so it pins the format but wouldn't catch drift in the real parser. Not worth changing.)

---

Generated by Claude Code

[phil-opp]

The hazards check out — every parser is parse::<u64>(), a collision is detected loudly at ShmemConf::create() rather than silently corrupting, and fork behavior is no worse than before. One consequence worth noting:

With the collision gone, nothing reclaims the dead incarnation's pool. MemoryPoolManager::cleanup_orphans runs only at dataflow spawn (binaries/daemon/src/lib.rs:3805), and the daemon registry is hard-capped at 512 entries (lib.rs:4686). So with restart_policy: Always, a node that crashes repeatedly, and a receiver that never calls free_memory_pool, each incarnation now registers a fresh id and leaves the old entry plus its /dev/shm segment (data region up to 1 GiB) behind. After 512 registrations every pool registration on that daemon fails, including unrelated dataflows. Pre-fix this loop failed fast on the first restart. An owner-death reclaim, or at least a note in the issue, would cover it.

Also: neither new test can go RED (one exercises a function this PR introduces, the other re-implements the parser inline instead of calling it), and dora-node-api-python is excluded from both PR CI and nightly cargo test — so nothing in CI exercises this change at all.

[phil-opp]

Thanks — both points are fair. Pushed 0029b8e addressing them, and left the reclaim itself for you to weigh.

Pool leak with the collision gone. You're right that this converts fail-fast into fail-after-N: a repeatedly-crashing node whose receiver never frees now leaks a registry entry + /dev/shm segment per restart, and cleanup_orphans only runs at spawn against a capped registry. I've documented the trade-off in the PINNED_COUNTER doc comment and added a note on #3015, and I've not folded an owner-death reclaim into this PR — that touches MemoryPoolManager lifecycle and wants its own integration coverage, so it's better as a separate change you can scope. My read is that this is still net-positive because it fixes the common case (a transient crash under restart_policy: Always now recovers instead of crash-looping on the first restart), while the regressed case is a node that is already crash-looping — but if you'd rather hold this until the reclaim lands, that's reasonable too.

Tests. Agreed the originals were tautological. I extracted pool_shmem_name / parse_pool_counter as the single source of truth for the on-wire id format (also dedups ~7 inline copies), and the round-trip test now exercises those production functions, so a format regression in either turns it RED — including the underscore-in-node-id and non-numeric-tail cases. The structural gap you flagged is real and I can't close it here: dora-node-api-python is excluded from both PR and nightly cargo test, so none of this runs in CI. If it'd help, I'm happy to open a follow-up to add a minimal cargo test -p dora-node-api-python job (it builds against the system Python, no maturin needed) so at least these pure-logic tests get exercised.

---

Generated by Claude Code

[phil-opp]

🤖 Automated review by Claude — fully automated; not vetted by a human.

Reviewed the latest commit 0029b8e. The extraction of pool_shmem_name / parse_pool_counter is a faithful refactor of the ~7 inline format! / rsplit_once('_')+parse sites — the last-segment-as-counter logic and every consumer (try_doradma_read, the fast/slow write paths) are preserved, so the on-wire id format is unchanged. The round-trip test now exercises the real production functions (would go RED on format drift, including the underscore-in-node-id and non-numeric-tail cases), which closes the tautological-test gap noted earlier. The leak trade-off is now documented rather than fixed; that remains a deliberate follow-up as discussed. No new issues found.

---

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 #3015 no longer blocks 1.0.

This fix is not in the parked copy — #3015 is recorded in external/dora-pool/README.md as a re-entry criterion. Worth rebasing onto the external package to carry it forward.

[phil-opp]

Makes sense — happy to carry this into the parked copy. I checked #3152: external/dora-pool/python-binding/node_binding.rs is a verbatim copy with PINNED_COUNTER still seeded at 0 (line 33) and the same duplicated dora_pool_{}_{}_{} / rsplit_once('_') sites, so the port is mechanical — the same random seed + wrapping_add, plus the pool_shmem_name/parse_pool_counter extraction and round-trip tests applied there instead.

One sequencing question, since external/dora-pool only exists on pool-extract today: do you want me to

1. stack it on pool-extract now (retarget fix(python-node): seed the pool-id counter randomly so restarts don't collide #3056's base to pool-extract, re-apply the fix to node_binding.rs) so it merges together with / right after refactor(tensor-pool): move behind a generic extension seam, opt-in and outside the 1.0 guarantees #3152, or
2. wait for refactor(tensor-pool): move behind a generic extension seam, opt-in and outside the 1.0 guarantees #3152 to land on main, then port and retarget fix(python-node): seed the pool-id counter randomly so restarts don't collide #3056 to main?

I'd lean toward (2) to avoid basing on a branch that's still moving, and I'll keep #3056 as the in-tree fix in the meantime in case #3152 doesn't land — but I'll do whichever you prefer. Either way I'll flip the README re-entry row from "PR #3056 open" to the fix being present in the parked copy.

---

Generated by Claude Code

[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 is now an opt-in extension at libraries/extensions/memory-pool, outside the 1.0 guarantees (#3152), so #3015 does not block 1.0 — but the fix is still wanted. The transport code moved wholesale; this should rebase onto libraries/extensions/memory-pool/python/src/transport.rs with little change.