Repository navigation
Fix shared-memory ownership across Windows process handoff - #146
Merged
Merged
Conversation
tavateva
approved these changes
Oct 17, 2026
tavateva
left a comment
There was a problem hiding this comment.
Review of 87d8a41, graded against #145.
Verdict: approve. The fix targets the right owner. Allocation now happens in the manager server, which holds a live handle until the existing remove_volume release point or run shutdown. The names-only tracker and _termed_mem collapse into one owning registry that list_segments, shutdown, owner-death cleanup and explicit release all read. The diagnosis that the old client-side shutdown() override was shadowed by BaseManager.start()'s finalizer is correct.
Verification
- Full Linux suite: 253 passed.
test_shared_memory_lifecycle.py: 17/17, also with-W error::ResourceWarning. - Real Linux slice → backprojection through the live manager. This was the benchmark harness at 128³ with 3 pairs, used here as a smoke test, not for its timing figures.
- Both operations completed and were pixel-equivalent across all runs.
- The output pixel digests (
7d7fabc3…,e1ddd105…) and the source digest are identical to a run of the same fixture on the pre-change pipeline (#140's head). - No leaked-segment warnings appeared in any run's stderr, and
/dev/shmwas empty afterwards.
- Relied on: the native Windows/macOS CI matrix and the author's Windows
WinError 2reproduction against unmodifiedmain.
Non-blocking
- The closing keyword closes #145 with two acceptance items open. The body defers #145's real-data Linux/Windows slicing → backprojection validation and item 4 (integration with #139's observation facility) until the #144 rebase. Merging with
Closes #145closes the issue, and those requirements then live only in this PR body.- Minimum response: move both items onto #139, or a follow-up, and confirm they landed there before merge. Alternatively, change the keyword to
Part of #145.
- Minimum response: move both items onto #139, or a follow-up, and confirm they landed there before merge. Alternatively, change the keyword to
- This registry is the natural fix for the #144 Linux accounting gap. On Linux the server's handles are mapped but never touched, so they add nothing to PSS. Queued, untouched chunks therefore stay invisible to
process_tree_pss_bytesafter the rebase, as in the #144 early review. But_OwnedSharedMemoryTrackeris now the single authoritative list of live operation segments and their sizes, which is exactly what a USS-plus-live-segments metric needs. Minimum response: none here. Noted for the #144 rebase. - CPython internals are declared and pinned.
_run_servermirrorsBaseManager._run_server,dispatchis used directly, and the server'sshared_memory_contextis replaced. All of this is declared and covered by the 3.11/3.12 native matrix, andpyprojectcaps Python below 3.13. Any future Python cap bump should re-check these three seams.
5 of 8 tasks
This was referenced Oct 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A Windows download worker can close its last shared-memory handle before the parent deserializes its result. The allocation then disappears and opening its name raises
WinError 2, surfaced asBrokenProcessPool. The manager now owns a live handle before returning an allocation name and keeps it until the existing per-volume release point or run shutdown.Closes #145. Based directly on main (
97c4d4d), independently of #139 / PR #144.Behavior
ShareableListregistration acquires a server handle synchronously too.shutdown()override was shadowed byBaseManager.start()'s finalizer. Use the parent-process sentinel for owner-death cleanup on Windows/POSIX and retain lftomo's POSIX process-group detachment.Validation
mem.py: after producer exit,pickle.loads()raisesFileNotFoundError: [WinError 2]. It passes with this change. No scheduling delay is used to make the handoff succeed.8d92bc3): Linux, Windows, and macOS, each with Python 3.11 and 3.12. The first macOS run caught overlong explicit names in two test fixtures; those names were shortened without changing production code. The separate push coverage workflow also passed.git diff --checkpassed. No new warnings observed.As requested, observer integration and real-data Linux/Windows slicing/backprojection and RAM-overhead validation remain open acceptance items in #139, under “Acceptance carried forward from #145 / PR #146”. Issue #145 marks both requirements as transferred, so they remain tracked after this PR closes it. They follow the memory-reporting branch rebase onto this change. That rebase must attach the existing observer to server-owned allocation/registration/release, preserve constructor observation for deserialized attachments, and remove the redundant
__setstate__observation path. The separate mapping-manifest publication failure remains work for #139. This PR does not claim scientific-pipeline or RAM-collector acceptance from lifecycle tests alone.Shared-basis audit
SharedNPArray,SharedNPManager,VolumeCache.remove_volume, and the slicing completion callback; lftomolftomo/util/mem.pyat8d4c884e9f42ad913a38f6592f85ba9419f7a0f1; CPythonSharedMemoryManager,SharedMemoryServer, tracker, and manager startup/finalization.SharedMemoryServer.shared_memory_contextnow owns each live handle and its run-scoped/termed release policy. Previously CPython tracked run-scoped names and_termed_memseparately tracked termed names without owning handles. The global list, getter, list proxy registration, client proxy state, duplicate allocation path, and ineffective client shutdown loop are removed.list_segments, normal shutdown, owner-death cleanup, and explicit removal all derive from the single registry.SharedNPArraycontinues to own only its process-local mapping, shape/views, and direct-creator unlink flag. Default whole-object serialization previously carried that flag and potentially cached pixels. The imported__reduce__converts once to a name/shape/dtype/view boundary adapter, reconstructed as a non-owning attachment; it is not a second lifetime registry.VolumeCache.remove_volume()and its existing completion callback retain the decision about when a termed buffer is no longer needed. The server atomically validates and executes release. No new buffer-retention schedule, sleep, retry, or duplicate completion state is introduced.The durable contract and integration notes are in
documentation/docs/development/shared-memory-lifecycle.md.Review follow-up
_run_server/startup/finalization,dispatch, andSharedMemoryServer.shared_memory_context, with the requirement to extend native lifecycle CI before supporting another Python version.Completed CPython compatibility audit
ResourceWarningpromoted to an error and empty stderr. The probes coverfork,spawn, andforkserver, initializer argument forwarding, the installed shutdown finalizer, and both RPC/public error behavior.SharedMemory(track=True)from 3.13, child-finalizer relocation in 3.13, and Linux'sforkserverdefault from 3.14. The existing handoff and owner-death regressions now cover every available start method rather than onlyspawn.>=3.11,<3.13and the supported CI version matrix are unchanged.documentation/docs/development/shared-memory-lifecycle.md, under “Compatibility audit (2026-10-17)”. The expanded native CI matrix passed on Python 3.11/3.12 for all three platforms, with no warnings in the lifecycle test summaries. The subsequent audit-results commit changes documentation only.