Skip to content

Bound filesystem policy probes during sandbox startup - #1604

Open
Soham Das (SohamDas2021) wants to merge 4 commits into
mainfrom
sohamdas2021-bubblewrap-launch-io-hang-investigation
Open

Soham Das (SohamDas2021) wants to merge 4 commits into
mainfrom
sohamdas2021-bubblewrap-launch-io-hang-investigation

Conversation

@SohamDas2021

@SohamDas2021 Soham Das (SohamDas2021) commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

📖 Description

Prevents unresponsive filesystem-policy paths from hanging sandbox startup. Probes now fail closed after five seconds, with a 16-worker cap, preserved Windows caller identity, and native-library lifetime protection.

Covers Bubblewrap, LXC, WSLC, and shared parser checks. The deadline is per probe, not total startup. Retires the outdated June 2026 roadmap; no public API or schema changes.

🔗 References

ADO 63911175; original report: #604, closed after moving tracking to ADO.

No separate design/API spec: this fixes bounded I/O in existing launch checks without adding an API, schema field, or backend.

🔍 Validation

Passed: 270 targeted Windows tests, 213 Ubuntu/WSL tests, native-library unload regression on both, formatting, and strict Clippy. Native macOS and full backend E2E validation remain for CI; live unreachable NFS/FUSE scenarios were not run.

Exact commands and results (from src)
Platform Command Result
Windows cargo test --locked --quiet -p mxc-sdk --lib --features wslc,isolation_session -- filesystem_ policy_mapping owned_thread config_parser module_lifetime backend_modules::wslc::common::sandbox::tests 270 passed
Ubuntu/WSL cargo test --locked --quiet -p mxc-sdk --lib -- filesystem_ bwrap_runner policy_mapping config_parser module_lifetime 213 passed
Both cargo build --locked --quiet -p mxc-sdk --example module_lifetime_fixture Passed
Both cargo test --locked --quiet -p mxc-sdk --test module_lifetime 1 passed per host
Windows cargo fmt --all -- --check Passed
Windows cargo clippy --locked --quiet -p mxc-sdk --lib --tests --examples --features wslc,isolation_session -- -D warnings Passed
Ubuntu/WSL cargo clippy --locked --quiet -p mxc-sdk --lib --tests --examples -- -D warnings Passed

✅ 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)

Cargo.lock is unchanged. CLA status is for the author to confirm; feature-specific and instruction-update items are not applicable.

📋 Issue Type

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

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7a12fef6-a3b7-47c5-b5b7-7cefee7dc7c8
Copilot AI balanced review requested due to automatic review settings October 28, 2026 18:44
@SohamDas2021
Soham Das (SohamDas2021) requested a review from a team as a code owner October 28, 2026 18:44
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

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

Regression coverage is incomplete: Linux CI skips the unload fixture and test, while stalled policy paths lack end-to-end SDK/CLI checks for bounded rejection without workload execution, including WSLC provisioning.

2 open issues
What changed in this PR

Bounds filesystem-policy checks in MXC’s shared parser and Bubblewrap, LXC, and WSLC startup paths, addressing hangs caused by unresponsive filesystems without changing public APIs or schemas.

Changes:

  • Adds five-second per-probe deadlines and a shared 16-worker limit, rejecting startup on timeout or worker failure.
  • Preserves Windows caller identity and protects detached workers from native-library unloading.
  • Adds timeout and library-lifetime tests, documents behavior and limitations, and retires the outdated roadmap.
File Description
src/​mxc-sdk/​tests/​module_lifetime.rs Adds subprocess-based library-unload regression coverage.
src/​mxc-sdk/​tests/​fixtures/​module_lifetime.rs Provides the detached-worker library fixture.
src/​mxc-sdk/​src/​core/​mxc_common/​process_util.rs Shares Windows impersonation-token duplication.
src/​mxc-sdk/​src/​core/​mxc_common/​module_lifetime.rs Adds cross-platform native-module lifetime protection.
src/​mxc-sdk/​src/​core/​mxc_common/​mod.rs Registers the new internal modules.
src/​mxc-sdk/​src/​core/​mxc_common/​filesystem_probe.rs Implements bounded workers, deadlines, and identity handling.
src/​mxc-sdk/​src/​core/​mxc_common/​filesystem_object.rs Bounds filesystem-object identity checks.
src/​mxc-sdk/​src/​core/​mxc_common/​filesystem_access.rs Bounds delegation checks.
src/​mxc-sdk/​src/​core/​mxc_common/​config_parser.rs Bounds existence probes and propagates probe failures.
src/​mxc-sdk/​src/​backends/​wslc/​common/​sandbox.rs Reuses shared module protection for stream pumps.
src/​mxc-sdk/​src/​backends/​wslc/​common/​policy_mapping.rs Bounds denied-path overlap resolution.
src/​mxc-sdk/​src/​backends/​lxc/​common/​filesystem_mounts.rs Bounds mount-path resolution and mask classification.
src/​mxc-sdk/​src/​backends/​isolation_session/​common/​owned_thread.rs Reuses shared token duplication.
src/​mxc-sdk/​src/​backends/​bubblewrap/​common/​bwrap_runner.rs Bounds denied-path resolution and classification.
src/​mxc-sdk/​Cargo.toml Registers the library fixture and test dependency.
docs/​schema.md Documents deadlines, failure behavior, and resource limits.
docs/​development/​README.md Removes the retired roadmap link.
docs/​development/​plans/​linux-wsl-roadmap-june-2026.md Deletes the outdated roadmap.
docs/​development/​build-and-test/​pull-requests.md Documents the library-unload regression commands.
docs/​backends/​wslc/​wsl-container-getting-started.md Explains provisioning probe deadlines.
docs/​backends/​lxc/​lxc-backend.md Links filesystem timeout guidance.
docs/​backends/​bwrap/​bubblewrap-backend.md Links filesystem timeout guidance.

Review effort: Balanced

Comment thread src/mxc-sdk/src/core/mxc_common/filesystem_probe.rs
Comment thread src/mxc-sdk/tests/module_lifetime.rs
Copilot AI balanced review requested due to automatic review settings October 28, 2026 20:51

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

Runtime validation on macOS and a genuinely stalled filesystem remains missing, leaving detached-worker safety across native-library unloads and full backend startup unconfirmed.

2 open issues

Review effort: Balanced

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7a12fef6-a3b7-47c5-b5b7-7cefee7dc7c8
@SohamDas2021
Soham Das (SohamDas2021) force-pushed the sohamdas2021-bubblewrap-launch-io-hang-investigation branch from 43541cb to c904d49 Compare October 28, 2026 21:49
Copilot AI balanced review requested due to automatic review settings October 28, 2026 21:49

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

SDK/FFI unload safety needs further confirmation because native macOS validation is pending and reported testing excludes live unreachable NFS/FUSE paths and full backend E2E.

2 open issues

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.

Summary

Reviewed this revision across 11 axes. The two retained findings concern regression coverage for behavior introduced by this PR, not confirmed runtime or containment failures. Please address the High backend-probe coverage gap before merging.

Source inspection found that the changed callers propagate probe errors and that FilesystemProbe::submit pins its containing module before spawning a worker. Independent Windows runs during this review passed:

  • cargo test --locked --offline -p mxc-sdk --lib filesystem_probe::tests -- --nocapture: 9 passed.
  • cargo build --locked --offline -p mxc-sdk --example module_lifetime_fixture: passed.
  • cargo test --locked --offline -p mxc-sdk --test module_lifetime: 1 passed.
  • cargo test --locked --offline -p mxc-sdk --lib --features wslc,isolation_session filesystem_probe::launch_tests:: -- --ignored: 2 passed.

Commands ran from src. Linux runtime verification was blocked by local WSL access denial; native macOS and full backend E2E were not run. A separate CI-script test attempt was blocked in setup by local sandbox access to toolchain settings.

The head is unchanged from the reviewed revision. The branch is behind main; attribution was rechecked against the merge-base diff displayed by GitHub. Both inline anchors are added lines, and the local diff matches GitHub after ignoring index hash abbreviation lengths.

Verified pre-existing - not attributed to this PR

None among the two retained findings. Both were verified as introduced_by_change; neither treats a pre-existing runtime bug as an additional merge condition.

Comment thread src/mxc-sdk/src/core/mxc_common/filesystem_probe_launch_tests.rs Outdated
Comment thread src/mxc-sdk/tests/fixtures/module_lifetime.rs Outdated
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Needs-Author-Feedback Waiting for additional information or action from the issue or pull-request author. label Oct 29, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7a12fef6-a3b7-47c5-b5b7-7cefee7dc7c8
Copilot AI balanced review requested due to automatic review settings October 29, 2026 17:47

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

Library-unload safety and Windows caller impersonation changes require human review of outstanding native macOS and backend E2E CI results.

No open issues 🎉

2 resolved since last review

Review effort: Balanced

Preserve bounded filesystem probes and regression coverage alongside upstream policy and host error classification.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 7a12fef6-a3b7-47c5-b5b7-7cefee7dc7c8
Copilot AI balanced review requested due to automatic review settings October 29, 2026 19:35

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

Detached-worker lifetime and native-library unload safety depend on platform-specific behavior, with macOS unload and full LXC/WSLC backend validation still pending.

No open issues 🎉

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs-Author-Feedback Waiting for additional information or action from the issue or pull-request author.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants