Skip to content

[WSLC] Stop state-aware exec failures from corrupting the output they report on - #1307

Closed
Soham Das (SohamDas2021) wants to merge 4 commits into
mainfrom
sohamdas2021-jubilant-pancake
Closed

Soham Das (SohamDas2021) wants to merge 4 commits into
mainfrom
sohamdas2021-jubilant-pancake

Conversation

@SohamDas2021

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

Copy link
Copy Markdown
Contributor

📖 Description

Two ways a state-aware exec could report something untrue about a run it had already finished.

A failed exec corrupted its own output. exec streams the script's stdout as it arrives. When the run then failed — a timeout, a daemon fault, output truncation — the executor appended its JSON error envelope to that same stdout. The result was neither valid JSON nor clean program output: PRE_FAILURE_MARKER{"error":{"code":"backend_error",...}}. The envelope now goes to stderr for a live exec, leaving stdout owned solely by the script. Non-exec phases and exec dry-run are unchanged, because they stream nothing and their envelope is the only thing on stdout.

A truncated exec overwrote the reason it ended. When a caller does not drain output fast enough, the daemon drops bytes and latches a truncation flag. That flag then replaced the terminal outcome: a clean run lost the exit code it had produced, and a run that timed out or was cancelled lost the truncation notice entirely — the caller was told the output was complete when it was not. Truncation now travels alongside the outcome as its own frame, so a timeout still reports as a timeout, and a truncated clean run still fails the call but carries its exit code in details.exitCode.

Also corrects docs/wsl/wslc-state-aware.md, which documented the old overwriting behaviour as intentional.

🔗 References

Addresses defect 3 of #843. The truncation defects were surfaced while analysing it; the second one became reachable on every terminal variant when #1252 added TimedOut and Cancelled.

Of #843's four defects: 1 is closed (#1187, #954), 2's output half is closed (#1252) and its stdin half is blocked on the WSLc SDK surfacing WSLCProcessFlagsStdin (#1258), 4 is closed (#1167 → #1240). This is 3.

🔍 Validation

  • cargo fmt --all -- --check and cargo clippy --all-targets --features wslc -- -D warnings clean across wslc_common, wxc_wslc_daemon, wxc, and mxc_engine.
  • cargo test --workspace, plus new unit tests at each seam: the stream-selection branch in the executor, the daemon's frame sequence for every terminal variant, and the exit code surviving a truncation failure.
  • On a real WSL2 host: run_wslc_state_aware_tests.ps1, 82/82.
  • The stderr-framing fix has an E2E regression guard: a command that emits output and then fails asserts the marker is on stdout, no envelope fragment is on stdout, and the typed error is on stderr. Against the pre-fix binary that suite reports 3 failures and captures the corruption verbatim.
  • Truncation has no E2E coverage — provoking it means out-racing the daemon's bounded queue, which is inherently racy from PowerShell. It is covered by unit tests against in-memory duplex pipes only, so that behaviour has not been exercised against a real container.

PROTOCOL_VERSION goes 5 → 6 for the new frame. The persistent per-user daemon is refused by a newer client until it restarts, which is the existing exact-equality check working as intended.

✅ Checklist

📋 Issue Type

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

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2351959c-fa0c-4666-9f47-54023bc32f03
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2351959c-fa0c-4666-9f47-54023bc32f03
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2351959c-fa0c-4666-9f47-54023bc32f03
Copilot AI balanced review requested due to automatic review settings September 28, 2026 06:06
@SohamDas2021
Soham Das (SohamDas2021) requested a review from a team as a code owner September 28, 2026 06:06
@azure-pipelines

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

@SohamDas2021 Soham Das (SohamDas2021) changed the title Stop state-aware exec failures from corrupting the output they report on [WSLC] Stop state-aware exec failures from corrupting the output they report on Sep 28, 2026

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.

Copilot review overview

🟡 Changes recommended

Some stderr envelopes can remain unparsable, and several terminal paths still discard truncation information.

Review effort: Balanced
Findings: 2 Medium severity · 2 Low severity

Open (4)
What changed in this PR

Fixes state-aware exec error framing and preserves truncation alongside terminal outcomes.

Changes:

  • Routes live exec error envelopes to stderr.
  • Adds a daemon truncation frame without replacing terminal outcomes.
  • Updates tests and lifecycle documentation.
File Description
tests/​scripts/​run_wslc_state_aware_tests.ps1 Tests stderr envelopes and clean stdout.
tests/​scripts/​run_isolation_session_state_aware_tests.ps1 Parses exec failures from stderr.
src/​core/​wxc/​src/​main.rs Routes live exec errors to stderr.
src/​backends/​wslc/​daemon/​src/​control_server.rs Emits truncation before terminal frames.
src/​backends/​wslc/​common/​src/​state_aware.rs Maps truncation into caller-visible outcomes.
src/​backends/​wslc/​common/​src/​daemon_protocol.rs Adds the truncation frame and bumps protocol version.
src/​backends/​wslc/​common/​src/​daemon_client.rs Tracks truncation during streaming exec.
docs/​wsl/​wslc-state-aware.md Documents updated WSLc output semantics.
docs/​state-aware-lifecycle/​mxc-state-aware-sandbox-api.md Updates executor stream conventions.
docs/​isolation-session/​state-aware-rust.md Documents stderr error envelopes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/backends/wslc/common/src/daemon_client.rs
Comment thread src/core/wxc/src/main.rs
Comment thread docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md
Comment thread docs/wsl/wslc-state-aware.md Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2351959c-fa0c-4666-9f47-54023bc32f03
Copilot AI review requested due to automatic review settings September 28, 2026 06:31
@SohamDas2021
Soham Das (SohamDas2021) deleted the sohamdas2021-jubilant-pancake branch September 28, 2026 06: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.

Copilot review overview

🟡 Changes recommended

Piped timeout/cancellation still hides truncation, while stderr content remains ambiguous with script-generated output.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (4)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Preserve truncation status for timeout and cancellation outcomes

src/​backends/​wslc/​common/​src/​state_aware.rs:372

The combined truncation flag is ignored when the daemon reports TimedOut or Cancelled. If either bounded queue overflowed before one of those terminals, the piped SDK caller still receives an ordinary timeout or Exited(-1) and has no indication that stdout/stderr is incomplete—the same false completeness this change is intended to eliminate. Preserve both facts in the waiter result (for example with a completion type that can carry the terminal outcome plus truncation) rather than dropping truncated for these arms.

Comment thread src/core/wxc/src/main.rs
Comment on lines +702 to +705
StateAwareExit::ExecError(json) => {
// The script's own stderr may not end in a newline, and gluing the
// envelope onto those bytes would leave it unparseable.
eprintln!("\n{}", json);
Comment on lines +770 to +775
| Exec, dispatch failed | Script's stdout, up to the point of failure | Single JSON envelope (`{error}`), after the script's stderr and any buffered MXC diagnostics |

`stdout` is authoritative: for non-exec phases it carries exactly one envelope; for exec
it carries either the script's output (success) or exactly one envelope (failure).
`stderr` is informational. MXC routes its diagnostic logger output to `stderr` in
state-aware mode so `stdout` remains parseable without sentinels. (One-shot dispatch
keeps its existing `stdout` logger behaviour — the stricter routing applies to
state-aware only.)
`stdout` is authoritative: for non-exec phases and exec dry-run it carries exactly one
envelope; for a non-dry-run exec it carries the script's output and nothing else. An exec
can fail after the script has already streamed output, so its error envelope goes to
`stderr` — appending it to `stdout` would leave neither the script's output nor the
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.

2 participants