[Soham Das (SohamDas2021)]

📖 Description

Fixes the two correctness items left open when #1564 was approved.

CLI: stdin is closed only after the command is known to be stopped. If reading wxc-exec's stdin fails while a WSLc command runs, the executor stops the container and now waits for the command to exit before closing the command's stdin. Previously the result of the stop was ignored, so a command that was still running could read the cut-short input as complete, for example cat > staged && mv staged live publishing a truncated file.

• If the stop can't be confirmed, the command's stdin stays open until the container is torn down. The run fails with a message saying the command could not be confirmed stopped.
• A run with no timeout no longer hangs in that case.
• When a run times out, stdin is no longer closed before the command is known to be gone, so a command that ignores SIGTERM can't read early end-of-file either.

.NET: waits stay responsive while a finished process is cleaned up. On WSLc, a command that exits leaving a lot of stdin unread is reported about a minute late, and MxcProcess spends that time in cleanup. During it:

• cancelling WaitAsync now returns at once, and a later wait returns the result;
• Kill returns at once without doing anything;
• Dispose returns at once, and the process is released when cleanup finishes, before the wait returns;
• Warnings returns the warnings from before cleanup started, and OutputMetadata is null until cleanup finishes.

Before, all of these waited for the cleanup; #1564 fixed only Kill. The native API is unchanged.

🔗 References

Follow-up to #1564 (review findings 2 and 4). Finding 6 only concerned the #1564 description, which is corrected there.

🔍 Validation

Windows with WSL 3.0.4 and WSLc SDK 3.0.3 (vendored; the MxcDependencies feed returned 401).

Command	Result
cargo fmt --all -- --check	Passed
cargo clippy --workspace --all-targets -- -D warnings	Passed
cargo clippy -p mxc-sdk -p mxc_ffi --features wslc --all-targets -- -D warnings	Passed
cargo test -p mxc-sdk --features wslc --lib	Passed: 2835 passed, 0 failed
.NET suite (Microsoft.Mxc.Sdk.Tests, built with -p:MxcWithWslc=true)	Passed: 373 passed, 0 failed, 49 skipped (live-host tests)
Same, with MXC_ENABLE_WSLC_TESTS=1, class MxcContainerWslcE2ETests	Passed: 11/11
tests\scripts\run_wslc_all_tests.ps1 -Debug -SkipSetup	Passed: create-and-run 49/49 (9 stdin cases), container lifecycle 139/139
Node wslc-e2e.test.js with MXC_ENABLE_WSLC_TESTS=1	Passed: 8/8

New tests:

• Live .NET: cancelling WaitAsync, and calling Warnings and Dispose, while a wait is in the late-exit cleanup. Both tests fail without the MxcProcess change and pass with it. They are in the class the scheduled WSLc validation already runs.
• Rust unit: a forwarding failure whose stop can't be confirmed keeps the command's stdin open until teardown, and the run's wait ends on either the process exiting or the forwarding failure.

Not covered live: a failed or unconfirmed container stop, which can't be triggered on a real host; the unit tests cover it. The Node SDK unit tests were not run, since nothing in the Node SDK changed.

✅ Checklist

• Signed the Contributor License Agreement
• Linked to an issue
• 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

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[azure-pipelines]

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

[Copilot]

🟡 Changes recommended

Unresolved stdin error paths, a V1 disposal compatibility break, timeout misreporting, and regression-test gaps prevent approval.

6 open issues

• Preserve synchronous disposal behavior for V1 · New
• Confirm session termination before releasing stdin · New
• Retain stdin until shutdown resources are prepared · New
• Add an independent deadline to exit polling · New
• Await discarded tasks to observe cleanup failures · New
• Recheck terminal status before reporting timeout · New

_{Review effort: Balanced}

[Soham Das (SohamDas2021)]

This is intended: finding 4 asked that Dispose not block during WSLc cleanup, which can take about a minute. Releasing the handle and session still happens before the wait returns, and MxcProcessCleanupTests covers it.

[Soham Das (SohamDas2021)]

Stdin now stays open until teardown whenever the command isn't confirmed gone. The remaining case needs the stop and the session termination to both fail, and wxc-exec exits right after, which closes the handle anyway.

[Copilot]

🟡 Changes recommended

Forwarding-setup failures can still deliver premature EOF, and timeout ordering lacks a functional regression that detects it.

3 open issues

• Quiesce closes stdin before confirming command termination · New
• Confirm session termination before releasing stdin
• Preserve synchronous disposal behavior for V1

1 previously missed issue

In code that hasn't changed since last review

Add regression for premature stdin EOF after timeout

src/​mxc-sdk/​src/​backends/​wslc/​common/​wsl_container_runner.rs:714

[P2] The timeout stdin-close ordering fix lacks a regression through the CLI/backend flow; the existing cat timeout case in tests/scripts/run_wslc_all_tests.ps1:628-637 checks only the timeout result and would still pass with premature EOF. Extend that scheduled suite with a command that ignores SIGTERM and writes a persistent marker only after EOF, keeping host stdin open through the timeout. Assert that the run times out without writing the marker, so restoring the unconditional close_stdin() fails the test.

4 resolved since last review

• Retain stdin until shutdown resources are prepared
• Recheck terminal status before reporting timeout
• Await discarded tasks to observe cleanup failures
• Add an independent deadline to exit polling

_{Review effort: Balanced}

[Gudge (MGudgin)]

Summary

Requesting changes for findings 1 and 2: deferred disposal can prevent an already-completed .NET V1 wait from returning, and newly added forwarding-setup failures can still deliver EOF before the command is confirmed stopped. The other seven findings are non-blocking coverage, diagnostic, or documentation issues.

Reviewed 67f22ed25df442da9daa0d7acf1ffc0ff4fb8ab5. The branch is behind the current main tip (1dd531d6a26328b1581960808099c2c969e12f10); attribution uses the PR's three-dot comparison from merge base 48e09207a604baad44a1bd7396859fdb3b98115c. The local patch matches GitHub's changed lines and hunk coordinates; only C# function-name labels in hunk headings differ.

Verified behavior and limits

• A controlled harness compiled the actual base and head MxcProcess implementations with native stubs. In both a one-worker-pool scenario and a deliberately held-output scenario, the base wait completed after native cleanup was released; the head wait remained pending until worker capacity or output was released. This establishes the managed scheduling/completion dependency, not a live WSLc or IsolationSession reproduction.
• The ordinary forwarding-failure path retains stdin after an unconfirmed stop, and the explicit timeout close now checks exit confirmation. The exception is the newly expanded setup-error path identified in finding 2. Native-handle users are guarded during finishing; no independent double-release defect was confirmed.
• The existing dispatcher already includes MxcContainerWslcE2ETests in the WSLc C# suite (scripts/ci/run_backend_validation_tests.ps1:332). The class grows from 9 to 11 Facts; the runner grows from 38 to 40 Rust tests. Findings 3, 7 and 8 concern missing interactions, not absent test discovery or removed tests. Live backend suites were not rerun for this review; the PR's live-host results remain author-provided evidence.

Finding 5 - Medium: narrow the timeout stdin guarantee

Attribution: claim_mismatch - the remaining forwarding behavior predates this PR, but the description newly claims: "When a run times out, stdin is no longer closed before the command is known to be gone".

After TimedOutUnconfirmed, run_internal calls forwarding.end_run() before container/session teardown (src/mxc-sdk/src/backends/wslc/common/wsl_container_runner.rs:1421-1423). If the forwarder still owns the input pipe, ended.open_stdin is empty. A host-input read failure during the remaining teardown sees run_over == true and drops that pipe without stop confirmation (:1515-1530). A genuine host EOF is legitimate completion and is not the defect alleged here.

Either retain/freeze forwarding ownership until confirmed exit or session teardown on this outcome, or narrow the description to the explicit close ordering actually fixed. This claim-only finding is not a merge condition by itself.

Attribution ledger

Numbers match the inline comments and the claim-only finding above.

#	Severity	Final attribution	Disposition
1	High; SDK breaking change, P0/critical compatibility alert	newly_exposed_by_change	Kept
2	High	newly_exposed_by_change	Kept
3	Medium	introduced_by_change	Kept; coverage gap
4	Medium	introduced_by_change	Kept
5	Medium	claim_mismatch	Rescoped to description; non-blocking
6	Medium	introduced_by_change	Kept
7	Medium	introduced_by_change	Kept; coverage gap
8	Medium	introduced_by_change	Kept; coverage gap
9	Medium	introduced_by_change	Kept; documentation gap

The P0/critical designation for finding 1 is MXC's published-SDK compatibility alert, not a critical security-vulnerability rating.

Verified pre-existing - not charged to this PR

• StartedContainer::quiesce already requested SIGKILL and canceled stdin before checking exit at merge-base lines 1799-1806. Only the PR's new post-start event-setup failure routes into it are charged in finding 2; unchanged abandonment paths are not separate blockers.
• Stream-closer access already waited behind the native wait's control lock in the base implementation. Its continued blocking is not filed as a new runtime defect.
• Losing the managed timeout indication after a failed native wait and retry was already possible in the base WaitAfterDeadline path, so it is not filed as a regression here.

Supporting files src/mxc-sdk/src/core/mxc_common/exec_stream.rs, src/mxc-sdk/src/core/mxc_common/process_util.rs, tests/scripts/run_wslc_all_tests.ps1, and scripts/ci/run_backend_validation_tests.ps1 were byte-compared and are identical between the effective PR base and head. Findings use them only to establish newly exposed behavior or existing coverage, not to charge unrelated changes.

[Copilot]

🟡 Changes recommended

V1 Dispose now returns before releasing native resources, breaking existing ownership guarantees and risking image-store conflicts, while failed teardown can also close stdin before command completion is confirmed.

4 open issues

• Quiesce closes stdin before confirming command termination
• Confirm session termination before releasing stdin
• Preserve synchronous disposal behavior for V1
• Retain stdin when container teardown or session termination fails · New

_{Review effort: Balanced}

[Soham Das (SohamDas2021)]

Same as the drop(started) thread: this path only runs in wxc-exec, which exits immediately afterwards. Keeping the writer would only delay the end-of-file by moments, so we're leaving this as an accepted limit.

[Gudge (MGudgin)]

Follow-up verification

Approved at e6490cb537618ef171a3ed7856992e6dfb554143, superseding my earlier request for changes. Both original runtime blockers are corrected: .NET asynchronous waits no longer block a worker or depend on joining descendant-held output during deferred disposal, and forwarding-setup cleanup checks exit confirmation before canceling stdin and releases the session owner before dropping the retained pipe.

Findings 1 and 3-9 are addressed. Finding 2 is partially addressed only in regression coverage: its runtime ordering is fixed, but a test injecting a forwarding-setup failure through StartedContainer::quiesce is still absent. This is a non-blocking Medium coverage gap, not an outstanding High runtime defect. Its attribution remains newly_exposed_by_change. The other original attributions are unchanged; finding 5's claim_mismatch is resolved by the new post-run forwarding-error retention behavior. Previously excluded pre-existing stream-closer blocking and timeout-retry behavior are not merge conditions.

Verification

• Local cargo test -p mxc-sdk --features wslc --lib -- wsl_container_runner::tests: passed, 42 tests. This includes the production abort-signal wiring, no-timeout wait, confirmed-stop counterpart, and post-run stdin retention regressions.
• Exact-head Windows CI run 40943528527, job 123692794216: dotnet test --solution Microsoft.Mxc.Sdk.slnx --no-build passed with 342 succeeded, 0 failed, 57 skipped. The optional-backend build's dotnet test Microsoft.Mxc.Sdk.Tests/Microsoft.Mxc.Sdk.Tests.csproj --no-build --no-restore -p:MxcWithWslc=true -p:MxcWithIsolationSession=true reported the same results. The new unconditional cleanup tests use native wait-entry/release controls rather than depending on WSL's late-exit delay.
• Local execution of dotnet run --project Microsoft.Mxc.Sdk.Tests -- -class '*MxcProcessCleanupTests*' was blocked during restore by sandbox-denied NuGet vulnerability-data access (NU1900); no check was bypassed. Live WSLc tests were not run locally because this host has WSL 2.7.14.0, below the documented 3.0.3 minimum. The new live timeout scenarios are wired into the existing scheduled suites; this approval does not claim they ran here.

The broader simultaneous process-stop and session-termination failure remains an explicitly acknowledged author-accepted limitation, not a verified successful teardown or a separately tracked fix.