Conversation
PR A of the Sandbox2 clean rebuild plan (see docs/projects/mlcpp-sandbox2-pr2873 in elastic/elastic-workspace, and the frozen #2873 for prior art). Reconstructs the dependency/build layer from current main, replacing #2873's inline file(WRITE)/string(REGEX REPLACE) source rewrites with checked-in, version-pinned patches that fail configure loudly on upstream drift. - 3rd_party/CMakeLists.txt: FetchContent sandboxed-api v20241008 on Linux, apply 4 patches via `git apply` with FATAL_ERROR on --check failure (idempotent across reconfigure via a reverse-check pre-test) - 3rd_party/patches/sandboxed-api/: the 4 patches (disable vendored gtest, stop -fno-exceptions propagating into ml-cpp targets, make Python3 optional, link zlib + static libstdc++/libgcc into the forkserver binary), each verified against a real shallow clone of the pinned tag - 3rd_party/licenses/{abseil,sandbox2}-*: license/attribution files - lib/sandbox/: dormant MlSandbox target (CMlSandboxAvailability query only - no policy/spawner/diagnostics, no controller wiring; those land in later PRs) plus a Linux-only forkserver runtime smoke test against a statically-linked, dependency-free payload binary Verified on Linux x86_64 (GCP devbox): configure reaches "Sandbox2 enabled: using sandbox2::sandbox2"; ml_test_sandbox passes under both a normal and a -DCMAKE_UNITY_BUILD=ON configure, including the forkserver smoke test actually forking/execing/reaping the payload. Not yet verified: aarch64, macOS, Windows non-Sandbox2 builds, license/Snyk scan.
valeriy42
changed the title
Root cause confirmed from the actual Buildkite log (fetched via `estc
buildkite fetch-logs`, not reproducible via a bare `docker run` since
that omits the `--build-tests` flag CI actually passes to
docker_entrypoint.sh): the CI build image
(docker.elastic.co/ml-dev/ml-linux-build) has no static libc/libm
archives, so the statically-linked smoke payload from the previous fix
fails to link there ("ld: cannot find -lm/-lc") on Linux x86_64,
x86_64-debug, and aarch64 - even though it linked fine on a personal
devbox that happened to have glibc-static installed.
Switch the payload back to dynamic linking (the default) and have the
smoke test call PolicyBuilder::AddLibrariesForBinary() - the actual
mechanism Sandbox2 provides for this exact case - instead of routing
around it with a static binary.
valeriy42
added this pull request to stack #3183
September 9, 2026 11:51
Comments referenced an internal planning document/plan-slice naming scheme (docs/projects/..., "PR A") from outside this repository, not resolvable by anyone reading ml-cpp alone. Reworded to be self-contained. Also corrected an inaccurate claim that ml_test_sandbox re-verifies patches against a fresh clone at runtime - it doesn't; only the configure-time git-apply check does that.
valeriy42
marked this pull request as ready for review
September 9, 2026 13:54
|
Pinging @elastic/ml-core (Team:ML) |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A critical compile-definition issue and a moderate Sandbox2 option issue remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a dormant Linux Sandbox2/Abseil dependency foundation with pinned patches, build wiring, availability APIs, and forkserver smoke coverage.
Changes:
- Fetches Sandbox2 and applies four checked-in patches.
- Adds dormant
MlSandboxtargets and platform wiring. - Adds smoke tests, payloads, and dependency license metadata.
File summaries
| File | Summary |
|---|---|
test/CMakeLists.txt |
Registers sandbox tests. |
lib/sandbox/unittest/payloads/sandbox_smoke_payload.cc |
Provides the smoke-test payload. |
lib/sandbox/unittest/Main.cc |
Adds the test runner entry point. |
lib/sandbox/unittest/CSandboxForkserverSmokeTest.cc |
Tests forkserver execution. |
lib/sandbox/unittest/CMlSandboxAvailabilityTest.cc |
Tests sandbox availability. |
lib/sandbox/unittest/CMakeLists.txt |
Configures sandbox tests and payload; compile-definition quoting requires correction. |
lib/sandbox/CMlSandboxAvailability.cc |
Implements availability detection. |
lib/sandbox/CMakeLists.txt |
Defines the dormant library. |
lib/CMakeLists.txt |
Registers the sandbox library. |
include/sandbox/CMlSandboxAvailability.h |
Declares the availability API. |
3rd_party/patches/sandboxed-api/README.md |
Documents patch maintenance. |
3rd_party/patches/sandboxed-api/0004-forkserver-zlib-static-libstdcxx.patch |
Adjusts forkserver linking. |
3rd_party/patches/sandboxed-api/0003-python3-optional.patch |
Makes Python optional. |
3rd_party/patches/sandboxed-api/0002-no-fno-exceptions-propagation.patch |
Prevents exception flag propagation. |
3rd_party/patches/sandboxed-api/0001-abseil-cpp-disable-gtest.patch |
Disables vendored Abseil gtest helpers. |
3rd_party/licenses/sandbox2-NOTICE.txt |
Adds Sandbox2 notice metadata. |
3rd_party/licenses/sandbox2-LICENSE.txt |
Adds Sandbox2 license metadata. |
3rd_party/licenses/sandbox2-INFO.csv |
Adds Sandbox2 dependency metadata. |
3rd_party/licenses/abseil-NOTICE.txt |
Adds Abseil notice metadata. |
3rd_party/licenses/abseil-LICENSE.txt |
Adds Abseil license metadata. |
3rd_party/licenses/abseil-INFO.csv |
Adds Abseil metadata; nested revision does not match the fetched source. |
3rd_party/CMakeLists.txt |
Fetches and patches Sandbox2; SAPI option names require correction. |
Review details
Suppressed comments (1)
3rd_party/CMakeLists.txt:72
- These are not option names in the pinned v20241008 SAPI; its options are
SAPI_BUILD_EXAMPLESandSAPI_BUILD_TESTING. These assignments are therefore no-ops, so a configure that enablesSAPI_BUILD_EXAMPLEScan still add the upstream examples (and the intended test switch is not controlled). Set the actual option names or remove these entries.
set(SAPI_ENABLE_EXAMPLES OFF CACHE INTERNAL "" FORCE)
set(SAPI_ENABLE_TESTS OFF CACHE INTERNAL "" FORCE)
- Files reviewed: 20/22 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if(TARGET sandbox2_smoke_payload) | ||
| add_dependencies(ml_test_sandbox sandbox2_smoke_payload) | ||
| target_compile_definitions(ml_test_sandbox PRIVATE | ||
| ML_SANDBOX2_SMOKE_PAYLOAD="$<TARGET_FILE:sandbox2_smoke_payload>" |
| @@ -0,0 +1,2 @@ | |||
| name,version,revision,url,license,copyright,sourceURL | |||
| abseil-cpp,20240722.1,,https://abseil.io,Apache License 2.0,,https://github.com/abseil/abseil-cpp | |||
edsavage
reviewed
Sep 15, 2026
| FetchContent_Declare( | ||
| sandboxed-api | ||
| GIT_REPOSITORY https://github.com/google/sandboxed-api.git | ||
| GIT_TAG ${ML_SANDBOXED_API_TAG} |
Contributor
There was a problem hiding this comment.
I think we should pin the tag to its actual commit SHA here, rather than use a (mutable) tag string, to guarantee we get exactly what we want.
edsavage
approved these changes
Sep 15, 2026
edsavage
left a comment
edsavage
left a comment
Contributor
There was a problem hiding this comment.
LGTM, just the one minor comment.
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
Reconstructs the dormant dependency/build foundation for Sandbox2 from
current
main, as a clean first slice ahead of the sandbox policy,spawner, and controller-routing changes that land in follow-up PRs. No
controller or
pytorch_inferencerouting changes in this PR — Sandbox2is not selectable from any production code path yet.
Frozen PR #2873 attempted this feature in one large branch; this PR
takes just its dependency/build layer and replaces its inline
file(WRITE)/string(REGEX REPLACE)source rewrites with checked-in,version-pinned patches that fail the configure step loudly instead of
silently no-op'ing on upstream drift.
3rd_party/CMakeLists.txt: FetchContentsandboxed-apiv20241008onLinux, applying 4 checked-in patches via
git apply(fails configureloudly on upstream drift, idempotent across reconfigure).
3rd_party/patches/sandboxed-api/: the 4 patches (disable vendoredgtest, stop
-fno-exceptionspropagating into ml-cpp targets, makePython3 optional, link zlib + static libstdc++/libgcc into the
forkserver binary).
3rd_party/licenses/{abseil,sandbox2}-*: license/attribution files.lib/sandbox/: dormantMlSandboxtarget (CMlSandboxAvailabilityquery only — no policy/spawner/diagnostics) plus a Linux-only
forkserver runtime smoke test that forks/execs/reaps a
dynamically-linked payload via
PolicyBuilder::AddLibrariesForBinary().Verified locally (Linux x86_64, both a plain configure and
-DCMAKE_UNITY_BUILD=ON) before pushing, and green on this repo's ownLinux/macOS/Windows CI, license/security scans, and Java integration
suites.
Stack created with GitHub Stacks CLI • Give Feedback 💬