Fix port conflict error message and cleanup on failure#40102
Fix port conflict error message and cleanup on failure#40102beena352 wants to merge 1 commit intomicrosoft:feature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates WSLC container startup to surface a correct “port already in use” error to users (instead of a misleading “distribution already exists” message) and ensures containers are cleaned up when Start() fails during wslc container run.
Changes:
- Switch port-conflict handling to
THROW_HR_WITH_USER_ERROR(_IF)so a user-facing message can be surfaced. - Add error translation around
MapPort()to try to convert port-conflict failures into a clearer user error. - Fix
wslc container runfailure cleanup by only disabling auto-delete after a successfulStart().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/windows/wslcsession/WSLCContainer.cpp | Improves port-conflict error surfacing during port allocation/mapping for container open/start. |
| src/windows/wslc/services/ContainerService.cpp | Ensures container run doesn’t leak containers when Start() fails by keeping auto-delete enabled until after a successful start. |
| THROW_HR_WITH_USER_ERROR_IF( | ||
| HRESULT_FROM_WIN32(ERROR_ALREADY_EXISTS), | ||
| !allocation, | ||
| "Port %hu is in use, cannot open container %hs", | ||
| e.VmPort, | ||
| dockerContainer.Id.c_str()); | ||
| std::format(L"Port {} is already in use, cannot open container {}", e.VmPort, dockerContainer.Id), | ||
| !allocation); |
There was a problem hiding this comment.
The new user-facing port-conflict strings are hard-coded here. Elsewhere in the codebase, errors surfaced via THROW_HR_WITH_USER_ERROR(_IF) typically come from wsl::shared::Localization::MessageWslc* so they can be localized (e.g., this file uses MessageWslcVolumeNotFound). Consider adding a localized message helper for this port-in-use case and using it here.
| THROW_HR_WITH_USER_ERROR_IF( | ||
| HRESULT_FROM_WIN32(ERROR_ALREADY_EXISTS), | ||
| !allocatedPort, | ||
| "Port %hu is in use, cannot start container %hs", | ||
| e.ContainerPort, | ||
| m_id.c_str()); | ||
| std::format(L"Port {} is already in use, cannot start container {}", e.ContainerPort, m_id), | ||
| !allocatedPort); |
There was a problem hiding this comment.
The error message uses e.ContainerPort, but for published ports the host port can differ (e.g., -p 8080:80). This will report the wrong port number to the user. Use the Windows/host port from the mapping (e.g., e.VmMapping.HostPort()) in the message (and keep container port only if you want to show both).
| catch (const wil::ResultException& ex) | ||
| { | ||
| if (ex.GetErrorCode() == HRESULT_FROM_WIN32(ERROR_ALREADY_EXISTS)) | ||
| { | ||
| THROW_HR_WITH_USER_ERROR( | ||
| HRESULT_FROM_WIN32(ERROR_ALREADY_EXISTS), | ||
| std::format(L"Port {} is already in use, cannot start container {}", e.ContainerPort, m_id)); | ||
| } |
There was a problem hiding this comment.
MapPort() failures caused by the OS bind/listen failing will come back as HRESULT_FROM_WIN32(WSAEADDRINUSE) (and possibly WSAEACCES), not ERROR_ALREADY_EXISTS (which is used for duplicate mappings inside wslrelay). As written, the new user error message won’t trigger for the common “port already bound by another process” scenario. Handle WSAEADDRINUSE (and optionally WSAEACCES) here and surface the port-conflict user error for those HRESULTs too.
| // Start the created container | ||
| WSLCContainerStartFlags startFlags{}; | ||
| WI_SetFlagIf(startFlags, WSLCContainerStartFlagsAttach, !runOptions.Detach); | ||
| THROW_IF_FAILED(container.Start(startFlags, nullptr)); // TODO: Error message, detach keys | ||
|
|
||
| // Disable auto-delete only after successful start | ||
| runningContainer.SetDeleteOnClose(false); | ||
|
|
There was a problem hiding this comment.
This change alters observable behavior (container should be deleted when Start() fails). The repo has WSLC end-to-end tests for wslc container run (e.g. test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp), but this scenario isn’t covered. Add an e2e test that holds a host port open, runs wslc container run -p <port>:..., asserts the expected port-in-use user message, and verifies the container is not left behind.
| { | ||
| THROW_HR_WITH_USER_ERROR( | ||
| HRESULT_FROM_WIN32(ERROR_ALREADY_EXISTS), | ||
| std::format(L"Port {} is already in use, cannot start container {}", e.ContainerPort, m_id)); |
There was a problem hiding this comment.
This user error message also uses e.ContainerPort, which may differ from the host/published port (e.VmMapping.HostPort()). If the mapping is -p 8080:80, a bind failure should report 8080, not 80.
| std::format(L"Port {} is already in use, cannot start container {}", e.ContainerPort, m_id)); | |
| std::format(L"Port {} is already in use, cannot start container {}", e.VmMapping.HostPort(), m_id)); |
Summary of the Pull Request
Fix port conflict error message in wslc container run. When starting a container on a port that is already in use, users previously saw the misleading message “A distribution with the supplied name already exists.” With this fix, the correct error is shown (for example, “Port 8080 is already in use, cannot start container ”). Also clean up the container left behind when Start() fails.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed