Repository navigation
[ML] Reap child IPC dirs and fail closed on degraded seccomp - #3217
Merged
Merged
Conversation
|
Pinging @elastic/ml-core (Team:ML) |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate Sandbox2 IPC reaping lifecycle issues must be fixed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (4)
What changed in this PR
Adds controller-owned cleanup for child IPC directories and enables fail-closed behavior when in-process seccomp installation fails.
Changes:
- Adds PID-aware reaping across legacy and Sandbox2 lifecycles.
- Wires spawn-failure cleanup and lifecycle tests.
- Enables degraded-seccomp termination and updates documentation.
Review follow-ups:
- Critical (1 vote): Serialize IPC directory ownership checks with removal.
- Critical (1 vote): Defer failed-spawn cleanup until after child termination and reaping.
- Moderate (4 votes): Register the IPC root before monitor callbacks can observe immediate exits.
- Moderate (1 vote): Notify the reaper for every completed monitor result.
- Nit (2 votes): Update stale seccomp Doxygen.
| File | Summary |
|---|---|
lib/seccomp/unittest/CSeccompFilterBuilderTest.cc |
Verifies fail-closed seccomp behavior. |
lib/sandbox/unittest/CPytorchInferenceSandboxPolicyTest.cc |
Tests IPC directory reaping. |
lib/sandbox/CSandboxedProcessSpawner_Linux.cc |
Integrates reaping with Sandbox2 lifecycle events. |
lib/sandbox/CPytorchInferenceSandboxPolicy.cc |
Derives IPC roots from launch arguments. |
lib/sandbox/CMakeLists.txt |
Builds the reaper implementation. |
lib/sandbox/CChildIpcDirectoryReaper.cc |
Implements guarded directory cleanup. |
lib/core/unittest/CDetachedProcessSpawnerTest.cc |
Tests child-exit callbacks. |
lib/core/CDetachedProcessSpawner.cc |
Adds spawn and exit callback support. |
lib/core/CDetachedProcessSpawner_Windows.cc |
Adds cross-platform callback compatibility. |
include/seccomp/CSystemCallFilter.h |
Enables termination on degraded seccomp. |
include/sandbox/CSandboxedProcessSpawner.h |
Exposes Sandbox2 reaper wiring. |
include/sandbox/CPytorchInferenceSandboxPolicy.h |
Documents IPC directory lifecycle. |
include/sandbox/CChildIpcDirectoryReaper.h |
Declares the reaper API. |
include/core/CDetachedProcessSpawner.h |
Declares callback-enabled spawn APIs. |
docs/sandbox2_production_failure_modes.md |
Documents degraded-seccomp behavior. |
bin/pytorch_inference/Main.cc |
Uses the centralized termination setting. |
bin/controller/CProcessSpawnerRouter.h |
Stores the shared reaper. |
bin/controller/CProcessSpawnerRouter.cc |
Wires reaping across routes and failure paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
edsavage
approved these changes
Sep 24, 2026
Contributor
There was a problem hiding this comment.
LGTM overall
There's a few minor changes worth addressing before merge. The main one is a potential directory-leak race on the Sandbox2 path (noteSpawn runs after the monitor thread is launched); the rest are lower priority (blocking FS IO under the tracker mutex, a double onSpawnFailed invocation, a name-only shape guard, and the no-startup-sweep trade-off).
Base automatically changed from
feature/sandbox2-pr-f-landlock-fallback
to
main
September 25, 2026 11:35
valeriy42
force-pushed
the
feature/sandbox2-pr-g-ipc-reap-seccomp
branch
2 times, most recently
from
September 26, 2026 19:09
2ea0ce9 to
373e7c3
Compare
valeriy42
force-pushed
the
feature/sandbox2-pr-g-ipc-reap-seccomp
branch
from
October 9, 2026 12:39
373e7c3 to
67db184
Compare
…nce exits The controller removes $TMPDIR/ml-child-ipc/<deployment-id> once the bound child exits or a spawn fails without a live pid, via CChildIpcDirectoryReaper wired through the legacy tracker and Sandbox2 monitor. Closes #3214.
TERMINATE_ON_DEGRADED_SECCOMP_FAILURE is now true in CSystemCallFilter.h so legacy and Landlock pytorch_inference launches exit before model IO when the in-process filter cannot be installed. Closes #3213.
Serialize reaper ownership with directory removal, register Sandbox2 IPC bindings before the monitor waits, defer failed-handoff IPC cleanup until after kill/reap, and invoke legacy reaper callbacks outside the tracker mutex. Refresh degraded-seccomp comments to match production defaults.
valeriy42
force-pushed
the
feature/sandbox2-pr-g-ipc-reap-seccomp
branch
from
October 16, 2026 07:31
67db184 to
840d665
Compare
Drop CORE_EXPORT from CChildIpcDirectoryReaper so controller links on Windows; use /bin/sh -c true in the child-exit callback unit test.
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.
Summary
CChildIpcDirectoryReaperremoves$TMPDIR/ml-child-ipc/<deployment-id>after the bound child exits (legacy tracker or Sandbox2 monitor) or after a failed spawn with no live pid;TERMINATE_ON_DEGRADED_SECCOMP_FAILUREis true ininclude/seccomp/CSystemCallFilter.hso legacy and Landlockpytorch_inferenceexits before model IO when in-process seccomp cannot install;mode:"landlock") unchanged.closes #3214.
closes #3213.