[ML] Linear child ownership and fault-injected lifecycle for Sandbox2

[valeriy42]

Summary

Rebuilds CSandboxedProcessSpawner around an explicit lifecycle state machine (Prepared -> Launched -> IdentityCaptured -> Registered -> Monitoring -> TerminationRequested -> CleanupRequired -> Reaped/Failed), stacked on #3184.

• Non-throwing kill-and-reap guard, armed immediately after launch and PID capture, holding its own owning reference to the sandbox handle so cleanup is order-independent during unwinding.
• Four injectable seams (pidfd acquisition, registry allocation, monitor-thread launch, sandbox completion) for deterministic fault injection.
• Explicit pidfd outcome classification: a kernel lacking pidfd support (ENOSYS) is the only case that falls back to signalling the sandboxee through its owned monitor handle (SIGKILL, identity-safe, no numeric-PID lookup); every other pidfd error is treated as a resource/identity failure and fails registration outright. Numeric kill(pid, ...) does not appear anywhere in this file.
• A CAS-controlled one-shot latch replacing two-boolean coordination for the timeout-vs-completion race.
• CSandboxedProcessSpawnerLifecycleTest_Linux.cc: fault-injection coverage for every pidfd class, allocation/resource failures, stale-generation protection, PID-reuse identity binding, the CAS race, descriptor-baseline cleanup, and destructor-latency.

Notes

• E_Launched/E_CleanupRequired lifecycle states are declared but intentionally left unassigned (no natural single point without broader restructuring) — not silently dropped.
• The SIGKILL-only assumption for the owned-monitor termination path (no graceful-SIGTERM variant is buildable against the pinned Sandbox2 dependency version) was cross-checked against the vendored monitor source during review.

[valeriy42]

Devbox verification (Linux x86_64, real Sandbox2 toolchain): `ml_test_sandbox` filtered to `CSandboxedProcessSpawnerLifecycleTest_Linux` — 13/13 test cases passed, 372/372 assertions, exit code 0.

This surfaced and fixed two real defects that macOS-only syntax checks couldn't catch:

• Two BOOST_TEST_REQUIRE call sites compared non-streamable types (std::future_status, a std::map iterator), which is a hard compile error for that macro (it tries to print both operands on failure). Switched to plain BOOST_REQUIRE.
• The forkserver warm-up used to stabilize the descriptor-count baseline was wired as a BOOST_GLOBAL_FIXTURE, whose constructor runs before Boost.Test finishes its own framework setup. Forking a real process that early corrupted the framework's internal state (every test case genuinely passed, but the run still reported a spurious "Incorrect setup: no test case executed" with a non-zero exit code). Moved the warm-up to a std::call_once-guarded call from inside the normal per-case fixture instead.

Also ran clang-format via the CI's own check image and fixed several workspace-planning-doc citations that had leaked into comments/commit messages (not resolvable outside this internal effort's own tracking) — both now clean.

Buildkite (macOS/Windows legs, full cross-platform CI) still pending.

[elasticsearchmachine]

Pinging @elastic/ml-core (Team:ML)

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical lifecycle, cleanup, and sandbox path issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 1

Open (5)

• Prevent terminate during detach exception handling · New
• Mark released entry before replacement can throw · New
• Rewrite IPC paths for sandbox-mounted locations · New
• Clean up child when result awaiting throws · New
• Support pidfd-unavailable environments in test · New

What changed in this PR

Rebuilds Sandbox2 child spawning with identity-safe lifecycle tracking, pidfd cleanup, fault-injection seams, and Linux lifecycle tests.

Changes:

• Adds lifecycle state, ownership, signaling, and cleanup management.
• Adds race, failure, descriptor, and fault-injection tests.
• Wires implementation and test payloads into CMake.

File	Summary
lib/​sandbox/​unittest/​payloads/​lifecycle_signal_payload.cc	Adds the long-lived signal-observing test payload.
lib/​sandbox/​unittest/​CSandboxedProcessSpawnerLifecycleTest_Linux.cc	Adds lifecycle and fault-injection coverage. Moderate findings: handle ENOSYS in the positive-path test (2 votes) and add deterministic readiness before signaling (1 vote).
lib/​sandbox/​unittest/​CMakeLists.txt	Registers lifecycle tests and payload targets.
lib/​sandbox/​CSandboxedProcessSpawner_Linux.cc	Implements spawning, monitoring, signaling, and cleanup. Findings: critical path mapping (1), detach failure handling (3), cleanup on await exceptions (1), and descriptor replacement safety (1); moderate allocation exception handling (1) and stale completion reporting (1).
lib/​sandbox/​CMakeLists.txt	Adds the spawner implementation target.
include/​sandbox/​CSandboxedProcessSpawner.h	Defines lifecycle states, registry, latch, and injection interfaces.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[edsavage]

Looking pretty good, I just spotted one thing that I think should be fixed prior to merge

testDestructorDoesNotJoinAndReturnsUnderOneSecond asserts elapsed < 1s via steady_clock. I think this will be subject to spurious failures on a stalled CI box. The property under test here is actually binary: Either a correct destructor returns in microseconds, or a regression that joins/blocks on the monitor (parked in AwaitResult() on a never-exiting child) hangs forever.

We could rework the test as a deadlock watchdog rather than a timing check: run spawner.reset() on its own thread (moving sole ownership of the unique_ptr in to avoid a race), wait_for a largeish timeout (~30s, purely as a deadlock guard rather than a performance bound), and then assert on status being future_status::ready so we assert on completion, not on elapsed time.This matches the watchdog pattern already used in testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd. If we do go this route we should rename the test to something like testDestructorDoesNotBlockOnLiveChild.

[valeriy42]

Addressed the destructor feedback in 432babb5c: renamed to testDestructorDoesNotBlockOnLiveChild, run spawner.reset() on a worker thread with sole unique_ptr ownership, and assert future_status::ready after a 30s deadlock guard (same watchdog pattern as testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd) instead of elapsed < 1s.

Copilot lifecycle/cleanup threads are replied inline; IPC argv rewrite and ENOSYS skip left as out-of-scope / intentional positive control.

[edsavage]

Addressed the destructor feedback in 432babb5c: renamed to testDestructorDoesNotBlockOnLiveChild, run spawner.reset() on a worker thread with sole unique_ptr ownership, and assert future_status::ready after a 30s deadlock guard (same watchdog pattern as testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd) instead of elapsed < 1s.

Copilot lifecycle/cleanup threads are replied inline; IPC argv rewrite and ENOSYS skip left as out-of-scope / intentional positive control.

Thanks @valeriy42! I went looking for 432babb5c and I don't think it made it onto the PR branch, the changes aren't in the current head.

However,

• 432babb5c does exist in the repo (authored 2026-09-21 12:56Z) and it does contain the destructor rework you described: testDestructorDoesNotBlockOnLiveChild with the std::future_status::ready deadlock watchdog, plus the spawner cleanup hardening (pidfd clearing, non-terminating monitor detach, Kill/reap before dropping the sandbox).
• But the current PR head 5215cb37a still has the pre-review test testDestructorDoesNotJoinAndReturnsUnderOneSecond with BOOST_CHECK(elapsed < std::chrono::seconds(1)), so the watchdog rename/fix isn't present on the branch.
• Comparing 432babb5c against the branch heads from the 21st (1a22ab149, e79c6e55f, 5215cb37a) all report diverged, i.e. 432babb5c was never an ancestor of feature/sandbox2-pr-d-lifecycle. The branch was force-pushed three times that day (12:54 -> 13:49 -> 18:30 -> 5215cb37a), and the final rebase landed on a history without the review commit, so I suspect it got clobbered somewhere along the way or it ended up on a separate/local branch rather than the PR branch. Could you cherry-pick it onto the current head and push?

[valeriy42]

Cherry-picked the review fix onto current head 5215cb37a as ff6a40b98 (content from 432babb5c).

• Destructor test is now testDestructorDoesNotBlockOnLiveChild with the 30s future_status::ready deadlock watchdog (same pattern as testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd), not elapsed < 1s.
• Also includes the spawner cleanup hardening from that commit (pidfd clearing after close, non-terminating monitor detach path, Kill/reap before registry erase on monitor await failure).

@edsavage — should be on the PR branch now; thanks for catching the dropped commit after the rebase.

[edsavage]

LGTM