Skip to content

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

Merged
Soham Das (SohamDas2021) merged 4 commits into
mainfrom
sohamdas2021-843
Sep 28, 2026
Merged

Soham Das (SohamDas2021) merged 4 commits into
mainfrom
sohamdas2021-843

Conversation

@SohamDas2021

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

Copy link
Copy Markdown
Contributor

📖 Description

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

A failed exec corrupted its own output. The error envelope was appended to the stdout the script had already been streaming to, leaving neither valid JSON nor clean program output: PRE_FAILURE_MARKER{"error":{"code":"backend_error",...}}. It now goes to stderr on its own line, so stdout belongs solely to the script and the envelope survives a script whose own stderr ends mid-line. Non-exec phases and exec dry-run stream nothing, so their envelope stays on stdout.

A truncated exec overwrote the reason it ended. Dropping output replaced the terminal outcome: a clean run lost its exit code, and a timed-out or cancelled run lost the truncation notice entirely — callers were told the output was complete when it was not. Truncation now travels alongside the outcome, so a timeout still reports as a timeout and a truncated clean run carries its exit code in details.exitCode.

Also corrects two docs that described the old behaviour: wslc-state-aware.md presented the overwriting as intentional, and the API doc's SDK-discrimination section still sent consumers to stdout for the error envelope.

🔗 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, 83/83.
  • Both stderr-framing guarantees have E2E regression guards, and both were verified to fail against a pre-fix binary rather than passing vacuously: a command that emits output and then fails (82/83 pre-fix), and a command whose last stderr write has no trailing newline (82/83 pre-fix).
  • 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

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
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:40
@SohamDas2021
Soham Das (SohamDas2021) requested a review from a team as a code owner September 28, 2026 06:40
@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.

Copilot review overview

🟡 Changes recommended

Error-envelope routing remains inconsistent for parse failures, and raw stderr cannot safely distinguish workload output from typed errors.

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

Open (2)
What changed in this PR

Prevents WSLc state-aware exec failures from corrupting workload output while preserving truncation and terminal outcomes.

Changes:

  • Routes live-exec error envelopes to stderr.
  • Adds protocol-level truncation notices without replacing terminal outcomes.
  • Updates regression tests and lifecycle documentation.
File Description
tests/​scripts/​run_wslc_state_aware_tests.ps1 Tests stderr framing and output isolation.
tests/​scripts/​run_isolation_session_state_aware_tests.ps1 Updates exec-error envelope parsing.
src/​core/​wxc/​src/​main.rs Routes non-dry-run 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 Preserves outcomes and truncated exit codes.
src/​backends/​wslc/​common/​src/​daemon_protocol.rs Adds Truncated and bumps protocol version.
src/​backends/​wslc/​common/​src/​daemon_client.rs Tracks truncation independently of completion.
docs/​wsl/​wslc-state-aware.md Documents revised WSLc behavior.
docs/​state-aware-lifecycle/​mxc-state-aware-sandbox-api.md Updates executor stream semantics.
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 on lines +834 to +836
- Non-dry-run **exec**, `exitCode != 0` AND the final non-empty line of stderr
parses as a complete `{error: {...}}` envelope: dispatch failed; SDK surfaces
the typed error.
Comment thread src/core/wxc/src/main.rs
Comment on lines +743 to +746
if phase == "exec" && !dry_run {
StateAwareExit::ExecError(json)
} else {
StateAwareExit::Error(json)
}
};
if let Some(notice) = notice {
write_frame(pipe, &notice).await?;

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.

write_frame

won't this go away when we move away from framed output?

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.

🕐

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.

:shipit:

@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 Sep 28, 2026
@SohamDas2021
Soham Das (SohamDas2021) merged commit f8f59c0 into main Sep 28, 2026
78 of 85 checks passed
@SohamDas2021
Soham Das (SohamDas2021) deleted the sohamdas2021-843 branch September 28, 2026 21:04
@microsoft-github-policy-service microsoft-github-policy-service Bot removed the Needs-Author-Feedback Waiting for additional information or action from the issue or pull-request author. label Sep 28, 2026
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.

3 participants