Repository navigation
[Bubblewrap] Fork bwrap from a process-lifetime thread - #1526
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>
|
@microsoft-github-policy-service agree company="Solo.io" |
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>
|
Thanks Keith Mattix II (@keithmattix) for the PR! Soham Das (@SohamDas2021) can you take a look at this when you get the chance? |
|
|
||
| #[test] | ||
| fn spawn_reports_launch_errors() { | ||
| let error = spawn(Command::new("mxc-lifetime-spawner-absent-binary")) |
There was a problem hiding this comment.
A nonexistent bare executable name does not guarantee NotFound: Linux PATH lookup can return PermissionDenied when a searched directory is inaccessible.
Use an absolute nonexistent path inside a test-owned temporary directory so the test checks error propagation rather than the host’s PATH configuration.
| .expect("spawning thread"); | ||
|
|
||
| let mut rest = String::new(); | ||
| stdout |
There was a problem hiding this comment.
The requested 30-second timeout does not bound this read: Bubblewrap enforces its deadline through wait/try_wait, but proc.wait() comes afterward. If the sandbox hangs without closing stdout, can this test block indefinitely instead of reporting a timeout?
| // when the *thread* that forked bwrap exits. A caller that spawns on a | ||
| // thread-pool or short-lived thread and waits elsewhere must not have | ||
| // the sandbox killed when that thread exits. | ||
| BubblewrapHost.Require(); |
There was a problem hiding this comment.
The default .NET workflow discovers this test but neither prepares Bubblewrap nor sets MXC_BWRAP_TESTS_REQUIRE_EXECUTION, so a runner without Bubblewrap silently skips it. Add a Bubblewrap-prepared .NET invocation with strict execution and a test-specific execution check, so unrelated passing tests cannot leave this regression unexercised.
microsoft-github-policy-service
Bot
added
the
Needs-Author-Feedback
📖 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