Skip to content

Commit aa3e230

Browse files
goodboygoodboy
committed
Fix SharedMemory under subint_forkserver
Implements the resolution described in c99d475'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
1 parent c99d475 commit aa3e230

4 files changed

Lines changed: 196 additions & 137 deletions

File tree

‎ai/conc-anal/subint_forkserver_mp_shared_memory_issue.md‎

Lines changed: 145 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,29 @@
1-
# `subint_forkserver` × `multiprocessing.SharedMemory`: incompatible-by-mp-design
1+
# `subint_forkserver` × `multiprocessing.SharedMemory`: fork-inherited `resource_tracker` fd
22

33
Surfaced by `tests/test_shm.py` under
4-
`--spawn-backend=subint_forkserver`. Both test functions
5-
fail with distinct symptoms that share one root cause:
4+
`--spawn-backend=subint_forkserver`. Two distinct
5+
failure modes, one root cause:
66
**`multiprocessing.resource_tracker` is fork-without-exec
7-
unsafe.**
7+
unsafe** (canonical CPython class — bpo-38119, bpo-45209).
8+
9+
**Status: resolved by `tractor/ipc/_mp_bs.py` +
10+
`tractor/ipc/_shm.py` changes (see "Resolution" below).
11+
This doc kept as the
12+
post-mortem / decision record.**
813

914
## TL;DR
1015

1116
`mp.shared_memory.SharedMemory` registers each shm
1217
allocation with the per-process
1318
`multiprocessing.resource_tracker` singleton. The
14-
tracker is a daemon process started lazily, and the
19+
tracker is a daemon process started lazily; the
1520
parent owns a unix-pipe-fd to it. When the parent
1621
forks-without-execing into a `subint_forkserver`
17-
child, the child inherits that fd — but the fd refers
18-
to the *parent's* tracker, which the child has no
22+
child, the child inherits that fd — but it refers to
23+
the *parent's* tracker, which the child has no
1924
business writing to.
2025

21-
Two manifestations:
26+
Two manifestations under the original (pre-fix) code:
2227

2328
1. **`test_child_attaches_alot`** — child loops 1000×
2429
`attach_shm_list()`. First `mp.SharedMemory` call
@@ -37,89 +42,140 @@ Two manifestations:
3742
`FileExistsError: '/shm_list'` because the leak
3843
persists across the parametrize loop and forkserver
3944
children can't `shm_open(create=True)` an existing
40-
key. Trio backend doesn't surface this because
41-
each subactor `exec`s a fresh interpreter →
42-
independent resource tracker per subactor → no
43-
inherited-fd issue, and the test's pre-existing
44-
leak is masked by the per-process tracker reset.
45-
46-
## Why trio backend works
47-
48-
Under `--spawn-backend=trio`, each subactor is born
49-
via `python -m tractor._child` (full `execve`) →
50-
fresh interpreter → fresh module-level globals →
51-
`mp.resource_tracker._resource_tracker` is `None`
52-
until first use → `mp.SharedMemory` constructs its
53-
own tracker, talks to its own pipe-fd. No cross-
54-
process fd inheritance.
55-
56-
Under `subint_forkserver`, the child is
57-
`os.fork()`'d from a worker thread of the parent
58-
(no `exec`) → inherits parent's
59-
`mp.resource_tracker._resource_tracker._fd` →
60-
EBADF / cross-talk on first `mp.SharedMemory`
61-
operation in the child.
62-
63-
## Status
64-
65-
**Not a tractor bug.** This is the canonical
66-
"fork-without-exec breaks `multiprocessing`
67-
internals" class — see CPython issues:
68-
69-
- https://bugs.python.org/issue38119
70-
- https://bugs.python.org/issue45209
71-
72-
Pure-`fork` start method has the same incompatibility;
73-
that's why `mp` itself defaults to `spawn` on macOS
74-
and `forkserver`/`spawn` on Linux post-3.14.
75-
76-
## Mitigation
77-
78-
`tests/test_shm.py` is module-marked with
79-
`pytest.mark.skipon_spawn_backend('subint_forkserver',
80-
'subint', reason=...)` pointing at this doc.
81-
82-
Two longer-term options if we ever want shm tests under
83-
`subint_forkserver`:
84-
85-
1. **Reset the inherited tracker fd in the child
86-
prelude** —
87-
`tractor/spawn/_subint_forkserver.py::_child_target`
88-
already calls `_close_inherited_fds()`. We could
89-
additionally explicitly clear
90-
`multiprocessing.resource_tracker._resource_tracker`
91-
so the child re-creates a fresh tracker on first
92-
shm op. **Caveat**: this means each
93-
forkserver-subactor spawns its own resource-tracker
94-
daemon-process, multiplying daemon-proc count by
95-
subactor count. mp authors deliberately avoided
96-
this — the tracker is meant to be a per-mp-context
97-
singleton.
98-
99-
2. **Stop using `multiprocessing.shared_memory`** —
100-
migrate to `posix_ipc` directly (no resource
101-
tracker) or finish the `hotbaud`-based ringbuf
102-
transport that already supersedes shm in many
103-
`tractor` IPC paths.
104-
105-
Neither is in scope for the
106-
`subint_forkserver`-backend-lands PR; both are tracked
107-
out as future work.
108-
109-
## Reproducer
45+
key.
46+
47+
Trio backend (`mp_spawn`-style) doesn't surface this:
48+
each subactor `exec`s a fresh interpreter →
49+
independent resource tracker per subactor → no
50+
inherited-fd issue, and the test's pre-existing leak
51+
gets masked by the per-process tracker reset.
52+
53+
Under `subint_forkserver`, the child is `os.fork()`'d
54+
from a worker thread (no `exec`) → inherits parent's
55+
`mp.resource_tracker._resource_tracker._fd` → EBADF
56+
/ cross-talk on first `mp.SharedMemory` op.
57+
58+
## Resolution
59+
60+
We side-step the broken upstream machinery entirely
61+
rather than try to make it fork-safe. Two-part fix
62+
landed (commits to follow this doc):
63+
64+
### 1. `tractor/ipc/_mp_bs.py::disable_mantracker()`
65+
— unconditional disable
66+
67+
The previous "3.13+ short-circuit" path used
68+
`partial(SharedMemory, track=False)` to opt-out of
69+
registration on 3.13+. The `track=False` switch is
70+
necessary but not sufficient under fork: the
71+
inherited tracker fd can still be touched indirectly
72+
(e.g. through `_ensure_running_and_write`'s
73+
self-check path).
74+
75+
The fix takes both belts AND suspenders:
76+
77+
- **Always** monkey-patch
78+
`mp.resource_tracker._resource_tracker` to a
79+
no-op `ManTracker` subclass whose
80+
`register`/`unregister`/`ensure_running` are all
81+
empty.
82+
- **Always** wrap `SharedMemory` with
83+
`track=False`.
84+
85+
Result: the inherited tracker fd in the fork child
86+
is still inherited (fd is a kernel object; we can't
87+
un-inherit it across fork) but **nothing in the
88+
shm code path will ever try to use it** — both the
89+
tracker singleton and the per-allocation registration
90+
are short-circuited.
91+
92+
### 2. `tractor/ipc/_shm.py::open_shm_list()`
93+
— own the cleanup
94+
95+
Without `mp.resource_tracker`, nobody else will
96+
unlink leaked segments at process exit. tractor
97+
already controls actor lifecycle, so we register
98+
unlink on the actor's lifetime stack:
99+
100+
```python
101+
def try_unlink():
102+
try:
103+
shml.shm.unlink()
104+
except FileNotFoundError as fne:
105+
log.exception(...) # benign sibling-already-cleaned race
106+
107+
actor.lifetime_stack.callback(try_unlink)
108+
```
109+
110+
The `FileNotFoundError` swallow handles the case
111+
where a sibling actor already unlinked the same
112+
segment (legitimate race in shared-key setups).
113+
114+
## Why this is the right call
115+
116+
- **mp's tracker is widely criticized.** The
117+
in-tree comment "non-SC madness" predates this
118+
fix and matches CPython upstream's own discomfort
119+
(e.g. the per-context tracker design rework
120+
discussions in bpo-43475).
121+
- **tractor already owns process lifecycle.** We
122+
have `actor.lifetime_stack`, `Portal.cancel_actor`,
123+
and the IPC cancel cascade. Adding mp's tracker
124+
on top buys nothing we can't do better ourselves.
125+
- **Backend-uniform.** No special-casing per spawn
126+
backend. trio (`mp_spawn`-style), `subint_forkserver`,
127+
and the future `subint` all behave identically
128+
— register-time no-op, exit-time unlink-via-
129+
lifetime-stack.
130+
131+
## Trade-offs / known gaps
132+
133+
- **Crash-leaked segments.** If an actor segfaults
134+
or is `SIGKILL`'d before its lifetime stack runs,
135+
`/dev/shm/<key>` will leak. Mitigations:
136+
- `tractor-reap` (the new
137+
`scripts/tractor-reap` CLI) doesn't yet sweep
138+
`/dev/shm` — could extend it.
139+
- Higher-level apps using shm should pin a UUID
140+
into the key (the `'shml_<uuid>'` pattern in
141+
`test_child_attaches_alot`) so leaks are
142+
distinct per session and easy to GC out-of-band.
143+
- **Cross-actor unlink races.** Two actors holding
144+
the same shm key racing on `unlink()` — handled
145+
by the `FileNotFoundError` swallow.
146+
- **Crashes won't show up in mp's leak warning.**
147+
We've turned off `resource_tracker`, so the usual
148+
`resource_tracker: There appear to be N leaked
149+
shared_memory objects to clean up at shutdown`
150+
warning is gone too. If we ever want it back as
151+
a crash-detection signal, we'd need our own
152+
equivalent (walk the actor's `_shm_list_keys` set
153+
at root teardown, log any unfreed).
154+
155+
## Verification
110156

111157
```sh
112-
# fail mode 1 (EBADF on resource_tracker._fd):
113-
./py314/bin/python -m pytest \
114-
tests/test_shm.py::test_child_attaches_alot \
115-
--spawn-backend=subint_forkserver --tb=short
116-
117-
# fail mode 2 (FileExistsError on /shm_list):
118-
./py314/bin/python -m pytest \
119-
tests/test_shm.py::test_parent_writer_child_reader \
158+
# fixed under both backends:
159+
./py314/bin/python -m pytest tests/test_shm.py \
120160
--spawn-backend=subint_forkserver
161+
# 7 passed
121162

122-
# baseline (passes):
123-
./py314/bin/python -m pytest \
124-
tests/test_shm.py --spawn-backend=trio
163+
./py314/bin/python -m pytest tests/test_shm.py \
164+
--spawn-backend=trio
165+
# 7 passed (regression check)
125166
```
167+
168+
## References
169+
170+
- CPython upstream issues:
171+
- https://bugs.python.org/issue38119 (fork
172+
+ resource_tracker fd inheritance)
173+
- https://bugs.python.org/issue45209
174+
(SharedMemory + resource_tracker)
175+
- https://bugs.python.org/issue43475
176+
(per-context tracker rework discussion)
177+
- Long-term alternative: migrate off
178+
`multiprocessing.shared_memory` entirely to
179+
`posix_ipc` (no tracker) or finish the
180+
`hotbaud`-based ringbuf transport. Not blocked on
181+
this fix — both are independently tracked.

‎tests/test_shm.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,10 @@
1717
pytestmark = pytest.mark.skipon_spawn_backend(
1818
'subint',
1919
# 'subint_forkserver',
20+
# XXX we hack around this stdlib limitation by both,
21+
# - setting `ShareMemory(track=False)`
22+
# - overriding the `mp.ResourceTracker` nonsense in
23+
# `.ipc._mp_bs`.
2024
reason=(
2125
'subint: GIL-contention hanging class.\n'
2226
'subint_forkserver: `multiprocessing.SharedMemory` '

‎tractor/ipc/_mp_bs.py‎

Lines changed: 29 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
Utils to tame mp non-SC madeness
1818
1919
'''
20-
import platform
20+
from functools import partial
2121

2222

2323
def disable_mantracker():
@@ -27,49 +27,37 @@ def disable_mantracker():
2727
2828
'''
2929
from multiprocessing.shared_memory import SharedMemory
30-
30+
from multiprocessing import (
31+
resource_tracker as mantracker,
32+
)
33+
34+
# XXX ALWAYS disable the stdlib's "resource tracker"; it prevents
35+
# fork backends and never was useful to us since we're SC
36+
# lifetime managing all allocations.
37+
class ManTracker(mantracker.ResourceTracker):
38+
def register(self, name, rtype):
39+
pass
40+
41+
def unregister(self, name, rtype):
42+
pass
43+
44+
def ensure_running(self):
45+
pass
46+
47+
# "know your land and know your prey"
48+
# https://www.dailymotion.com/video/x6ozzco
49+
mantracker._resource_tracker = ManTracker()
50+
mantracker.register = mantracker._resource_tracker.register
51+
mantracker.ensure_running = mantracker._resource_tracker.ensure_running
52+
mantracker.unregister = mantracker._resource_tracker.unregister
53+
mantracker.getfd = mantracker._resource_tracker.getfd
3154

3255
# 3.13+ only.. can pass `track=False` to disable
3356
# all the resource tracker bs.
3457
# https://docs.python.org/3/library/multiprocessing.shared_memory.html
35-
if (_py_313 := (
36-
platform.python_version_tuple()[:-1]
37-
>=
38-
('3', '13')
39-
)
40-
):
41-
from functools import partial
42-
return partial(
43-
SharedMemory,
44-
track=False,
45-
)
46-
47-
# !TODO, once we drop 3.12- we can obvi remove all this!
48-
else:
49-
from multiprocessing import (
50-
resource_tracker as mantracker,
51-
)
52-
53-
# Tell the "resource tracker" thing to fuck off.
54-
class ManTracker(mantracker.ResourceTracker):
55-
def register(self, name, rtype):
56-
pass
57-
58-
def unregister(self, name, rtype):
59-
pass
60-
61-
def ensure_running(self):
62-
pass
63-
64-
# "know your land and know your prey"
65-
# https://www.dailymotion.com/video/x6ozzco
66-
mantracker._resource_tracker = ManTracker()
67-
mantracker.register = mantracker._resource_tracker.register
68-
mantracker.ensure_running = mantracker._resource_tracker.ensure_running
69-
mantracker.unregister = mantracker._resource_tracker.unregister
70-
mantracker.getfd = mantracker._resource_tracker.getfd
71-
72-
# use std type verbatim
73-
shmT = SharedMemory
58+
shmT = partial(
59+
SharedMemory,
60+
track=False,
61+
)
7462

7563
return shmT

‎tractor/ipc/_shm.py‎

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -929,15 +929,26 @@ def open_shm_list(
929929
# "close" attached shm on actor teardown
930930
try:
931931
actor = tractor.current_actor()
932-
933932
actor.lifetime_stack.callback(shml.shm.close)
934933

935-
# XXX on 3.13+ we don't need to call this?
936-
# -> bc we pass `track=False` for `SharedMemeory` orr?
937-
if (
938-
platform.python_version_tuple()[:-1] < ('3', '13')
939-
):
940-
actor.lifetime_stack.callback(shml.shm.unlink)
934+
# >XXX NOTE< on 3.13+ we need to call this AS WELL AS pass
935+
# `track=False` for `mp.SharedMemeory` otherwise fork based
936+
# backends will error out due to long lived stdlib
937+
# limitations,
938+
# - https://bugs.python.org/issue38119
939+
# - https://bugs.python.org/issue45209
940+
#
941+
def try_unlink():
942+
try:
943+
shml.shm.unlink()
944+
except FileNotFoundError as fne:
945+
log.debug(
946+
f'ShmList already deallocated pre-actor-shutdown.\n'
947+
f'{fne!r}\n'
948+
)
949+
950+
actor.lifetime_stack.callback(try_unlink)
951+
941952
except RuntimeError:
942953
log.warning('tractor runtime not active, skipping teardown steps')
943954

0 commit comments

Comments
 (0)