From aa3e2309266f29329bc31edeef3c6af2b3111f34 Mon Sep 17 00:00:00 2001 From: goodboy Date: Mon, 27 Apr 2026 10:51:28 -0400 Subject: [PATCH] Fix `SharedMemory` under `subint_forkserver` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the resolution described in c99d475d's `subint_forkserver_mp_shared_memory_issue.md` (now updated with the resolution post-mortem). Two-part fix that side-steps `mp.resource_tracker` entirely rather than try to make it fork-safe — turns out that's both simpler AND more correct given tractor already SC-manages allocation lifetimes. Deats, - `tractor/ipc/_mp_bs.py::disable_mantracker()`: drop the `platform.python_version_tuple()[:-1] >= ('3', '13')` branch — patches now run unconditionally: * monkey-patch `mp.resource_tracker. _resource_tracker` to a no-op `ManTracker` subclass (empty `register` / `unregister` / `ensure_running`). * return `partial(SharedMemory, track=False)` for the per-allocation opt-out. * belt + suspenders: even if something dodges the wrapper, the singleton can't talk to the inherited (broken) parent fd. - `tractor/ipc/_shm.py::open_shm_list()`: drop the 3.13+ conditional skip of the unlink-callback; install a `try_unlink()` wrapper that swallows `FileNotFoundError` (sibling-already-cleaned race in shared-key setups). Without `mp.resource_tracker` doing it for us, we own the unlink — `actor. lifetime_stack` is the right place since tractor already controls actor lifecycle. - `tests/test_shm.py`: uncomment-out `subint_forkserver` from the module-level skip- list (tests pass now). Inline comment cross-refs the two `_mp_bs` / `_shm` workarounds. - `ai/conc-anal/subint_forkserver_mp_shared_memory_ issue.md`: heavy rewrite — flips status from "open / unresolvable in tractor" to "resolved, kept as decision record". Adds Resolution section, "Why this is the right call" rationale (mp tracker is widely criticized; tractor already owns lifecycle), trade-offs (crash-leaked segments, lost mp leak warning), verification (7 passed under both `subint_forkserver` and `trio` backends), and upstream issue links (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code --- ...ubint_forkserver_mp_shared_memory_issue.md | 206 +++++++++++------- tests/test_shm.py | 4 + tractor/ipc/_mp_bs.py | 68 +++--- tractor/ipc/_shm.py | 25 ++- 4 files changed, 181 insertions(+), 122 deletions(-) diff --git a/ai/conc-anal/subint_forkserver_mp_shared_memory_issue.md b/ai/conc-anal/subint_forkserver_mp_shared_memory_issue.md index 8351aae0..030a32d0 100644 --- a/ai/conc-anal/subint_forkserver_mp_shared_memory_issue.md +++ b/ai/conc-anal/subint_forkserver_mp_shared_memory_issue.md @@ -1,24 +1,29 @@ -# `subint_forkserver` × `multiprocessing.SharedMemory`: incompatible-by-mp-design +# `subint_forkserver` × `multiprocessing.SharedMemory`: fork-inherited `resource_tracker` fd Surfaced by `tests/test_shm.py` under -`--spawn-backend=subint_forkserver`. Both test functions -fail with distinct symptoms that share one root cause: +`--spawn-backend=subint_forkserver`. Two distinct +failure modes, one root cause: **`multiprocessing.resource_tracker` is fork-without-exec -unsafe.** +unsafe** (canonical CPython class — bpo-38119, bpo-45209). + +**Status: resolved by `tractor/ipc/_mp_bs.py` + +`tractor/ipc/_shm.py` changes (see "Resolution" below). +This doc kept as the +post-mortem / decision record.** ## TL;DR `mp.shared_memory.SharedMemory` registers each shm allocation with the per-process `multiprocessing.resource_tracker` singleton. The -tracker is a daemon process started lazily, and the +tracker is a daemon process started lazily; the parent owns a unix-pipe-fd to it. When the parent forks-without-execing into a `subint_forkserver` -child, the child inherits that fd — but the fd refers -to the *parent's* tracker, which the child has no +child, the child inherits that fd — but it refers to +the *parent's* tracker, which the child has no business writing to. -Two manifestations: +Two manifestations under the original (pre-fix) code: 1. **`test_child_attaches_alot`** — child loops 1000× `attach_shm_list()`. First `mp.SharedMemory` call @@ -37,89 +42,140 @@ Two manifestations: `FileExistsError: '/shm_list'` because the leak persists across the parametrize loop and forkserver children can't `shm_open(create=True)` an existing - key. Trio backend doesn't surface this because - each subactor `exec`s a fresh interpreter → - independent resource tracker per subactor → no - inherited-fd issue, and the test's pre-existing - leak is masked by the per-process tracker reset. + key. -## Why trio backend works +Trio backend (`mp_spawn`-style) doesn't surface this: +each subactor `exec`s a fresh interpreter → +independent resource tracker per subactor → no +inherited-fd issue, and the test's pre-existing leak +gets masked by the per-process tracker reset. -Under `--spawn-backend=trio`, each subactor is born -via `python -m tractor._child` (full `execve`) → -fresh interpreter → fresh module-level globals → -`mp.resource_tracker._resource_tracker` is `None` -until first use → `mp.SharedMemory` constructs its -own tracker, talks to its own pipe-fd. No cross- -process fd inheritance. +Under `subint_forkserver`, the child is `os.fork()`'d +from a worker thread (no `exec`) → inherits parent's +`mp.resource_tracker._resource_tracker._fd` → EBADF +/ cross-talk on first `mp.SharedMemory` op. -Under `subint_forkserver`, the child is -`os.fork()`'d from a worker thread of the parent -(no `exec`) → inherits parent's -`mp.resource_tracker._resource_tracker._fd` → -EBADF / cross-talk on first `mp.SharedMemory` -operation in the child. +## Resolution -## Status +We side-step the broken upstream machinery entirely +rather than try to make it fork-safe. Two-part fix +landed (commits to follow this doc): -**Not a tractor bug.** This is the canonical -"fork-without-exec breaks `multiprocessing` -internals" class — see CPython issues: +### 1. `tractor/ipc/_mp_bs.py::disable_mantracker()` + — unconditional disable -- https://bugs.python.org/issue38119 -- https://bugs.python.org/issue45209 +The previous "3.13+ short-circuit" path used +`partial(SharedMemory, track=False)` to opt-out of +registration on 3.13+. The `track=False` switch is +necessary but not sufficient under fork: the +inherited tracker fd can still be touched indirectly +(e.g. through `_ensure_running_and_write`'s +self-check path). -Pure-`fork` start method has the same incompatibility; -that's why `mp` itself defaults to `spawn` on macOS -and `forkserver`/`spawn` on Linux post-3.14. +The fix takes both belts AND suspenders: -## Mitigation +- **Always** monkey-patch + `mp.resource_tracker._resource_tracker` to a + no-op `ManTracker` subclass whose + `register`/`unregister`/`ensure_running` are all + empty. +- **Always** wrap `SharedMemory` with + `track=False`. -`tests/test_shm.py` is module-marked with -`pytest.mark.skipon_spawn_backend('subint_forkserver', -'subint', reason=...)` pointing at this doc. +Result: the inherited tracker fd in the fork child +is still inherited (fd is a kernel object; we can't +un-inherit it across fork) but **nothing in the +shm code path will ever try to use it** — both the +tracker singleton and the per-allocation registration +are short-circuited. -Two longer-term options if we ever want shm tests under -`subint_forkserver`: +### 2. `tractor/ipc/_shm.py::open_shm_list()` + — own the cleanup -1. **Reset the inherited tracker fd in the child - prelude** — - `tractor/spawn/_subint_forkserver.py::_child_target` - already calls `_close_inherited_fds()`. We could - additionally explicitly clear - `multiprocessing.resource_tracker._resource_tracker` - so the child re-creates a fresh tracker on first - shm op. **Caveat**: this means each - forkserver-subactor spawns its own resource-tracker - daemon-process, multiplying daemon-proc count by - subactor count. mp authors deliberately avoided - this — the tracker is meant to be a per-mp-context - singleton. +Without `mp.resource_tracker`, nobody else will +unlink leaked segments at process exit. tractor +already controls actor lifecycle, so we register +unlink on the actor's lifetime stack: -2. **Stop using `multiprocessing.shared_memory`** — - migrate to `posix_ipc` directly (no resource - tracker) or finish the `hotbaud`-based ringbuf - transport that already supersedes shm in many - `tractor` IPC paths. +```python +def try_unlink(): + try: + shml.shm.unlink() + except FileNotFoundError as fne: + log.exception(...) # benign sibling-already-cleaned race -Neither is in scope for the -`subint_forkserver`-backend-lands PR; both are tracked -out as future work. +actor.lifetime_stack.callback(try_unlink) +``` -## Reproducer +The `FileNotFoundError` swallow handles the case +where a sibling actor already unlinked the same +segment (legitimate race in shared-key setups). + +## Why this is the right call + +- **mp's tracker is widely criticized.** The + in-tree comment "non-SC madness" predates this + fix and matches CPython upstream's own discomfort + (e.g. the per-context tracker design rework + discussions in bpo-43475). +- **tractor already owns process lifecycle.** We + have `actor.lifetime_stack`, `Portal.cancel_actor`, + and the IPC cancel cascade. Adding mp's tracker + on top buys nothing we can't do better ourselves. +- **Backend-uniform.** No special-casing per spawn + backend. trio (`mp_spawn`-style), `subint_forkserver`, + and the future `subint` all behave identically + — register-time no-op, exit-time unlink-via- + lifetime-stack. + +## Trade-offs / known gaps + +- **Crash-leaked segments.** If an actor segfaults + or is `SIGKILL`'d before its lifetime stack runs, + `/dev/shm/` will leak. Mitigations: + - `tractor-reap` (the new + `scripts/tractor-reap` CLI) doesn't yet sweep + `/dev/shm` — could extend it. + - Higher-level apps using shm should pin a UUID + into the key (the `'shml_'` pattern in + `test_child_attaches_alot`) so leaks are + distinct per session and easy to GC out-of-band. +- **Cross-actor unlink races.** Two actors holding + the same shm key racing on `unlink()` — handled + by the `FileNotFoundError` swallow. +- **Crashes won't show up in mp's leak warning.** + We've turned off `resource_tracker`, so the usual + `resource_tracker: There appear to be N leaked + shared_memory objects to clean up at shutdown` + warning is gone too. If we ever want it back as + a crash-detection signal, we'd need our own + equivalent (walk the actor's `_shm_list_keys` set + at root teardown, log any unfreed). + +## Verification ```sh -# fail mode 1 (EBADF on resource_tracker._fd): -./py314/bin/python -m pytest \ - tests/test_shm.py::test_child_attaches_alot \ - --spawn-backend=subint_forkserver --tb=short - -# fail mode 2 (FileExistsError on /shm_list): -./py314/bin/python -m pytest \ - tests/test_shm.py::test_parent_writer_child_reader \ +# fixed under both backends: +./py314/bin/python -m pytest tests/test_shm.py \ --spawn-backend=subint_forkserver +# 7 passed -# baseline (passes): -./py314/bin/python -m pytest \ - tests/test_shm.py --spawn-backend=trio +./py314/bin/python -m pytest tests/test_shm.py \ + --spawn-backend=trio +# 7 passed (regression check) ``` + +## References + +- CPython upstream issues: + - https://bugs.python.org/issue38119 (fork + + resource_tracker fd inheritance) + - https://bugs.python.org/issue45209 + (SharedMemory + resource_tracker) + - https://bugs.python.org/issue43475 + (per-context tracker rework discussion) +- Long-term alternative: migrate off + `multiprocessing.shared_memory` entirely to + `posix_ipc` (no tracker) or finish the + `hotbaud`-based ringbuf transport. Not blocked on + this fix — both are independently tracked. diff --git a/tests/test_shm.py b/tests/test_shm.py index 8ea43457..d6ad93f4 100644 --- a/tests/test_shm.py +++ b/tests/test_shm.py @@ -17,6 +17,10 @@ from tractor.ipc._shm import ( pytestmark = pytest.mark.skipon_spawn_backend( 'subint', # 'subint_forkserver', + # XXX we hack around this stdlib limitation by both, + # - setting `ShareMemory(track=False)` + # - overriding the `mp.ResourceTracker` nonsense in + # `.ipc._mp_bs`. reason=( 'subint: GIL-contention hanging class.\n' 'subint_forkserver: `multiprocessing.SharedMemory` ' diff --git a/tractor/ipc/_mp_bs.py b/tractor/ipc/_mp_bs.py index 462291c6..7f2092d2 100644 --- a/tractor/ipc/_mp_bs.py +++ b/tractor/ipc/_mp_bs.py @@ -17,7 +17,7 @@ Utils to tame mp non-SC madeness ''' -import platform +from functools import partial def disable_mantracker(): @@ -27,49 +27,37 @@ def disable_mantracker(): ''' from multiprocessing.shared_memory import SharedMemory + from multiprocessing import ( + resource_tracker as mantracker, + ) + # XXX ALWAYS disable the stdlib's "resource tracker"; it prevents + # fork backends and never was useful to us since we're SC + # lifetime managing all allocations. + class ManTracker(mantracker.ResourceTracker): + def register(self, name, rtype): + pass + + def unregister(self, name, rtype): + pass + + def ensure_running(self): + pass + + # "know your land and know your prey" + # https://www.dailymotion.com/video/x6ozzco + mantracker._resource_tracker = ManTracker() + mantracker.register = mantracker._resource_tracker.register + mantracker.ensure_running = mantracker._resource_tracker.ensure_running + mantracker.unregister = mantracker._resource_tracker.unregister + mantracker.getfd = mantracker._resource_tracker.getfd # 3.13+ only.. can pass `track=False` to disable # all the resource tracker bs. # https://docs.python.org/3/library/multiprocessing.shared_memory.html - if (_py_313 := ( - platform.python_version_tuple()[:-1] - >= - ('3', '13') - ) - ): - from functools import partial - return partial( - SharedMemory, - track=False, - ) - - # !TODO, once we drop 3.12- we can obvi remove all this! - else: - from multiprocessing import ( - resource_tracker as mantracker, - ) - - # Tell the "resource tracker" thing to fuck off. - class ManTracker(mantracker.ResourceTracker): - def register(self, name, rtype): - pass - - def unregister(self, name, rtype): - pass - - def ensure_running(self): - pass - - # "know your land and know your prey" - # https://www.dailymotion.com/video/x6ozzco - mantracker._resource_tracker = ManTracker() - mantracker.register = mantracker._resource_tracker.register - mantracker.ensure_running = mantracker._resource_tracker.ensure_running - mantracker.unregister = mantracker._resource_tracker.unregister - mantracker.getfd = mantracker._resource_tracker.getfd - - # use std type verbatim - shmT = SharedMemory + shmT = partial( + SharedMemory, + track=False, + ) return shmT diff --git a/tractor/ipc/_shm.py b/tractor/ipc/_shm.py index b60fafcc..f0225d70 100644 --- a/tractor/ipc/_shm.py +++ b/tractor/ipc/_shm.py @@ -929,15 +929,26 @@ def open_shm_list( # "close" attached shm on actor teardown try: actor = tractor.current_actor() - actor.lifetime_stack.callback(shml.shm.close) - # XXX on 3.13+ we don't need to call this? - # -> bc we pass `track=False` for `SharedMemeory` orr? - if ( - platform.python_version_tuple()[:-1] < ('3', '13') - ): - actor.lifetime_stack.callback(shml.shm.unlink) + # >XXX NOTE< on 3.13+ we need to call this AS WELL AS pass + # `track=False` for `mp.SharedMemeory` otherwise fork based + # backends will error out due to long lived stdlib + # limitations, + # - https://bugs.python.org/issue38119 + # - https://bugs.python.org/issue45209 + # + def try_unlink(): + try: + shml.shm.unlink() + except FileNotFoundError as fne: + log.debug( + f'ShmList already deallocated pre-actor-shutdown.\n' + f'{fne!r}\n' + ) + + actor.lifetime_stack.callback(try_unlink) + except RuntimeError: log.warning('tractor runtime not active, skipping teardown steps')