Repository navigation
[Bubblewrap] Fork bwrap from a process-lifetime thread - #1526
Open
Keith Mattix II (keithmattix) wants to merge 2 commits into
Open
Keith Mattix II (keithmattix) wants to merge 2 commits into
Keith Mattix II (keithmattix) wants to merge 2 commits into
Conversation
`bwrap --die-with-parent` arms PR_SET_PDEATHSIG in the launcher, and Linux delivers it when the *thread* that forked bwrap exits, not the process. `v1::spawn` forked bwrap on the caller's thread, so a sandbox started from a pooled or short-lived thread (Tokio `spawn_blocking`, a .NET thread-pool thread, a scoped worker) and waited on elsewhere was SIGKILLed when that thread was retired, surfacing as `Exited(-1)` with truncated output. The proxy (`runtimeConfig.networkProxy`) path hit it deterministically because spawn blocks until bwrap has armed the signal. Route the bwrap spawn through a dedicated thread that lives for the process lifetime, so the sandbox still dies with the embedding process but no longer with an arbitrary caller thread. Adds a regression test that spawns on a thread which exits while the workload is still running. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Author
|
@microsoft-github-policy-service agree company="Solo.io" |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The permanent native thread needs human validation of embedding and shared-library lifetime behavior across SDKs.
No open issues 🎉
What changed in this PR
Fixes #1525 by launching Bubblewrap from a process-lifetime thread, preventing sandbox termination when the caller’s thread exits.
Changes:
- Routes launches through a dedicated thread while preserving synchronous results.
- Adds launch and caller-thread-exit regression tests.
- Documents the parent-death signal behavior.
| File | Description |
|---|---|
| src/mxc-sdk/tests/streaming_bubblewrap.rs | Tests survival after the spawning thread exits. |
| src/mxc-sdk/src/backends/bubblewrap/common/mod.rs | Registers the Linux-only spawner module. |
| src/mxc-sdk/src/backends/bubblewrap/common/lifetime_spawner.rs | Implements the dedicated launch thread and unit tests. |
| src/mxc-sdk/src/backends/bubblewrap/common/bwrap_runner.rs | Routes Bubblewrap launches through the spawner. |
| docs/backends/bwrap/bubblewrap-backend.md | Explains thread-independent sandbox lifetime. |
Review effort: Balanced
bwrap --die-with-parent ties a sandbox to the thread that forked bwrap, which the Rust SDK fixed by spawning from a process-lifetime thread. Pin the same behavior in the other SDKs: - .NET: MxcContainer.Spawn on a thread that exits while the workload runs. It fails against the native library without the fix. - Node: the public spawn from a worker_threads Worker that is terminated while the workload runs. Node's spawn runs on the libuv pool, so this guards against moving it onto the calling thread. Both gate on a usable Bubblewrap; MXC_BWRAP_TESTS_REQUIRE_EXECUTION turns the skip into a failure, as for the Rust suites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This branch has not been deployed
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.
📖 Description
bwrap --die-with-parentarms PR_SET_PDEATHSIG in the launcher, and Linux delivers it when the thread that forked bwrap exits, not the process.v1::spawnforked bwrap on the caller's thread, so a sandbox started from a pooled or short-lived thread (Tokiospawn_blocking, a .NET thread-pool thread, a scoped worker) and waited on elsewhere was SIGKILLed when that thread was retired, surfacing asExited(-1)with truncated output. The proxy (runtimeConfig.networkProxy) path hit it deterministically because spawn blocks until bwrap has armed the signal.Route the bwrap spawn through a dedicated thread that lives for the process lifetime, so the sandbox still dies with the embedding process but no longer with an arbitrary caller thread. Adds a regression test that spawns on a thread which exits while the workload is still running.
🔗 References
Resolves #1525
🔍 Validation
Added automated test coverage and also tested manually by enabling network proxy.
✅ Checklist
Cargo.lock, thedependency-feed-checkcheck passes (see pull request builds)📋 Issue Type
GitHub Actions runs the PR validation build automatically. The ADO pipeline
(
MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity with the GitHubActions build; it runs on merge to
main, and Microsoft reviewers with write access can trigger iton a PR with
/azp run. See pull request builds.If the
dependency-feed-checkcheck fails on a new dependency, the crate must be added tothe feed before the PR can pass. See pull request builds
for the steps.
Microsoft Reviewers: Open in CodeFlow