Skip to content

[Bubblewrap] Avoid redundant namespace probe on launch - #1618

Merged
Carlos Alexandro Becker (caarlos0) merged 4 commits into
mainfrom
bwrap-probe-ns
Oct 30, 2026
Merged

Carlos Alexandro Becker (caarlos0) merged 4 commits into
mainfrom
bwrap-probe-ns

Conversation

@caarlos0

@caarlos0 Carlos Alexandro Becker (caarlos0) commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

📖 Description

Avoid allocating disposable user and network namespaces on Bubblewrap proxy and firewall launches. Actual sandbox setup establishes namespace availability; standalone validation and capability discovery retain their probes.

Preserve startup diagnostics and add failure-path and concurrent-launch coverage. This is an internal performance change with no schema, policy, or public API changes, so no design spec is required. Launch latency has not yet been measured.

🔗 References

Fixes #1617

🔍 Validation

  • cargo test --manifest-path src/Cargo.toml -p mxc-sdk --lib --offline mxc_common::sandbox_process::runner_tests — passed (9 tests, macOS).
  • cargo fmt --manifest-path src/Cargo.toml --all -- --check — passed.
  • shellcheck tests/scripts/run_bwrap_namespace_startup_test.sh — passed.
  • bash -n tests/scripts/run_bwrap_namespace_startup_test.sh tests/scripts/run_bwrap_all_tests.sh — passed.
  • CI at 85eef27f, run 41050887992: all required checks passed, including Linux x64/arm64, macOS arm64, Windows x64/arm64, and SDK suites.
  • From src/, cargo test --locked --release --target x86_64-unknown-linux-gnu -p mxc-sdk --test streaming_bubblewrap and cargo test --locked --release --target aarch64-unknown-linux-gnu -p mxc-sdk --test streaming_bubblewrap — both passed (16 tests each), including namespace-startup diagnostics through both spawn and spawn_with_pty.
  • From src/, cargo test --locked --release --target x86_64-unknown-linux-gnu -p mxc-sdk --lib 'core_modules::mxc_pty::' and cargo test --locked --release --target aarch64-unknown-linux-gnu -p mxc-sdk --lib 'core_modules::mxc_pty::' — both passed (47 tests each), including bounded nonblocking startup-output capture.
  • cargo check --manifest-path src/Cargo.toml -p mxc-sdk --lib --target x86_64-unknown-linux-gnu --offline — failed: Linux Rust target not installed.
  • cargo check --manifest-path src/Cargo.toml -p mxc-sdk --lib --offline — blocked by the local sandbox's dependency-metadata write policy.
  • Local PTY diagnostic unit test — failed during PTY allocation with ERANGE; the Linux CI test passed on both architectures.
  • bash tests/scripts/run_bwrap_namespace_startup_test.sh — not run: no Linux lxc-exec binary on this macOS host. The scheduled failure-path, cleanup, and concurrent-launch script and before/after latency measurements remain pending.

The new Bubblewrap functional script is wired into the scheduled backend suite. The SDK startup-diagnostic test is in the Linux streaming suite.

✅ Checklist

  • Signed the Contributor License Agreement
  • Linked to an issue
  • For features or changes requiring a spec: linked the issue with maintainer-applied feature-accepted
  • For implementation: linked the required approved and merged design/API spec, or explained why no spec is required
  • Feature issue remains open unless this PR completes all agreed implementation, validation, and documentation
  • Updated documentation (if applicable)
  • Updated Copilot instructions (if build, architecture, or conventions changed)
  • If this PR changes Cargo.lock, the dependency-feed-check check passes (see pull request builds)

📋 Issue Type

  • Bug fix
  • Feature
  • Task
Microsoft Reviewers: Open in CodeFlow

Use actual sandbox setup to check namespace availability while retaining standalone validation and capability probes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7f101bc6-5cd9-407a-94b5-ce6607786613
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 29, 2026 19:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The Linux-only startup and cleanup changes were neither compiled nor exercised on Linux, leaving fail-closed behavior, bounded startup, process cleanup, diagnostics, and concurrent launches unverified.

No open issues 🎉

What changed in this PR

Addresses #1617 by removing redundant disposable namespace probes from Bubblewrap launches while retaining probes for standalone validation and capability discovery. Actual sandbox setup determines namespace availability.

Changes:

  • Separates cached tool checks from namespace-allocation probes.
  • Preserves startup diagnostics and adds namespace-policy guidance.
  • Adds failure-path, concurrent-launch, and streaming SDK coverage, with the functional script included in the Bubblewrap suite.

Linux runtime validation and before/after latency measurements remain pending.

File Description
tests/​scripts/​run_bwrap_namespace_startup_test.sh Adds launch, validation, failure-path, cleanup, and concurrency scenarios.
tests/​scripts/​run_bwrap_all_tests.sh Includes namespace startup tests in the backend suite.
src/​mxc-sdk/​tests/​streaming_bubblewrap.rs Tests namespace startup diagnostics through the streaming SDK.
src/​mxc-sdk/​src/​core/​mxc_common/​sandbox_process.rs Clarifies that actual setup may enforce environmental checks.
src/​mxc-sdk/​src/​backends/​bubblewrap/​common/​proxy_network.rs Separates namespace probes from tool checks and captures startup stderr.
src/​mxc-sdk/​src/​backends/​bubblewrap/​common/​bwrap_runner.rs Retains validation probes and enriches launch failure diagnostics.
docs/​backends/​bwrap/​bubblewrap-backend.md Documents launch checks versus standalone namespace probing.

Review effort: Balanced

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7f101bc6-5cd9-407a-94b5-ce6607786613
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 29, 2026 19:51

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No high-confidence correctness or compatibility findings in c19e80c. Both independent checks also found no medium-or-higher defects.

Verification gaps: Linux startup, cleanup, and concurrent-launch behavior were not exercised in this review. Before/after launch latency remains unmeasured. Shell syntax, ShellCheck, and diff checks passed; no local build was run. This is not runtime verification of the change.

– Copilot, on behalf of Carlos

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The PTY startup path loses Bubblewrap’s underlying error output, leaving callers with generic guidance; bounded PTY diagnostics and startup-failure test coverage are still needed.

1 open issue

Review effort: Balanced

Capture bounded available PTY output when namespace startup fails and cover both public streaming spawn APIs in the failure fixture.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7f101bc6-5cd9-407a-94b5-ce6607786613
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 30, 2026 11:45

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One blocking P2 finding in the new PTY test. This review supersedes #1618 (review). The earlier PTY diagnostic issue is addressed by the new output collection path and public API fixture.

Verification gaps: no local builds or runtime tests were run. Host load exceeded the local test limit. Linux startup/cleanup behavior and before/after latency remain unverified; the current macOS CI test step is still running. Both independent checks completed; the macOS ordering defect below was confirmed against the existing PTY drain helper and macOS test discovery.

– Copilot, on behalf of Carlos

Comment thread src/mxc-sdk/src/core/mxc_pty/mod.rs Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The macOS PTY diagnostic waits for the child before draining output, which can block the child’s terminal close and cause the test to hit its 20-second deadline.

1 open issue
1 resolved since last review

Review effort: Balanced

Comment thread src/mxc-sdk/src/core/mxc_pty/mod.rs Resolved
Match the diagnostic helper's only production caller and avoid waiting for a Darwin PTY child before draining its output. The existing Linux PTY test step retains the bounded-read assertions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7f101bc6-5cd9-407a-94b5-ce6607786613
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 30, 2026 11:49

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new findings in 85eef27. The Linux-only helper and test gates address the macOS PTY test failure; both independent checks found no further defects. This supersedes #1618 (review) (GitHub does not permit dismissing COMMENT reviews).

Verification gaps remain: no local builds or runtime tests were run. Linux startup, cleanup, and concurrent launches, and before/after latency remain unverified. The incremental diff check passed.

– Copilot, on behalf of Carlos

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Linux runtime validation is still pending for namespace startup, fail-closed cleanup, and PTY diagnostics, with Linux CI in progress and local testing limited to macOS.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@MGudgin Gudge (MGudgin) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved following an eight-axis static review of 85eef27. No substantiated High or Medium findings remained after consolidation. Request validation, actual namespace enforcement, bounded startup diagnostics, and regression-test/CI wiring were inspected. Linux runtime tests and latency measurements were not independently run during this review.

@caarlos0
Carlos Alexandro Becker (caarlos0) merged commit 6cc8765 into main Oct 30, 2026
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bubblewrap] Avoid redundant namespace allocation during launch validation

4 participants