Skip to content

Improve errors returned from WSLCContainer#40147

Open
OneBlue wants to merge 5 commits intofeature/wsl-for-appsfrom
user/oneblue/state-errors
Open

Improve errors returned from WSLCContainer#40147
OneBlue wants to merge 5 commits intofeature/wsl-for-appsfrom
user/oneblue/state-errors

Conversation

@OneBlue
Copy link
Copy Markdown
Collaborator

@OneBlue OneBlue commented Apr 10, 2026

Summary of the Pull Request

This change introduces proper error messages and specialized error codes for errors coming out of WSLCContainers.

The specialized error codes allow wslc.exe to ignore some specific errors (like starting a container that's already running) while still allowing the SDK to return errors for invalid container state changes

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Copilot AI review requested due to automatic review settings April 10, 2026 01:18
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves WSLC container error handling by introducing container-specific HRESULTs and localized user-facing messages, enabling the CLI to treat certain state-related errors as non-fatal while preserving strict error semantics for the SDK.

Changes:

  • Add new container-state HRESULTs (WSLC_E_CONTAINER_NOT_RUNNING, WSLC_E_CONTAINER_IS_RUNNING) and wire them into error-string mapping.
  • Update WSLCContainer operations to throw localized “user error” messages for invalid state transitions.
  • Expand unit/e2e tests to validate new error codes/messages and CLI “no-op” behavior for already-stopped/started containers.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
test/windows/WSLCTests.cpp Updates assertions to expect new HRESULTs and validates COM error messages.
test/windows/wslc/e2e/WSLCE2EContainerStopTests.cpp Adds e2e coverage for container stop on an already-stopped container.
test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp Adds e2e coverage for container start on an already-running container.
src/windows/wslcsession/WSLCContainer.cpp Emits new container-state HRESULTs and localized user messages from container APIs.
src/windows/wslc/services/ContainerService.cpp Treats “already running/not running” as acceptable for CLI start/stop.
src/windows/service/inc/wslc.idl Defines the new WSLC container-state HRESULTs.
src/windows/common/wslutil.cpp Adds the new HRESULTs to common error string mapping.
src/shared/inc/defs.h Introduces THROW_IF_FAILED_EXCEPT helper for selective HRESULT ignoring.
localization/strings/en-US/Resources.resw Adds localized strings for the new container-state user error messages.

@OneBlue OneBlue marked this pull request as ready for review April 10, 2026 02:33
@OneBlue OneBlue requested a review from a team as a code owner April 10, 2026 02:33
Copilot AI review requested due to automatic review settings April 10, 2026 02:33
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comment on lines +23 to +26
auto _result = (result); \
if (FAILED(_result) && _result != (accepted)) \
{ \
THROW_HR(_result); \
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

THROW_IF_FAILED_EXCEPT suppresses an HRESULT but does not clear COM IErrorInfo that may have been set by the failed call. If this macro is used around COM calls (as in ContainerService), the thread can retain a stale error description/source and accidentally surface it for a later, unrelated failure. Consider explicitly clearing error info (e.g., SetErrorInfo(0, nullptr) or consuming it via GetErrorInfo) when _result == accepted, or documenting/encoding that this macro is COM-safe only when it clears error info.

Suggested change
auto _result = (result); \
if (FAILED(_result) && _result != (accepted)) \
{ \
THROW_HR(_result); \
const auto _result = (result); \
const auto _accepted = (accepted); \
if (FAILED(_result)) \
{ \
if (_result == _accepted) \
{ \
::SetErrorInfo(0, nullptr); \
} \
else \
{ \
THROW_HR(_result); \
} \

Copilot uses AI. Check for mistakes.
Comment on lines +660 to +662
THROW_HR_WITH_USER_ERROR_MSG(
WSLC_E_CONTAINER_NOT_RUNNING,
Localization::MessageWslcContainerNotRunning(m_id),
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

THROW_HR_WITH_USER_ERROR_MSG always inserts an extra ". " between the user-facing message and the internal detail. Since MessageWslcContainerNotRunning already ends with a period, the resulting internal exception text becomes "...not running.. Cannot stop..." (double period) which is noisy in logs/diagnostics. Consider removing trailing punctuation from the localized string, or avoid the auto-inserted period by using a non-suffixed user message for the _MSG form.

Suggested change
THROW_HR_WITH_USER_ERROR_MSG(
WSLC_E_CONTAINER_NOT_RUNNING,
Localization::MessageWslcContainerNotRunning(m_id),
auto userMessage = Localization::MessageWslcContainerNotRunning(m_id);
if (!userMessage.empty() && userMessage.back() == L'.')
{
userMessage.pop_back();
}
THROW_HR_WITH_USER_ERROR_MSG(
WSLC_E_CONTAINER_NOT_RUNNING,
userMessage,

Copilot uses AI. Check for mistakes.
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