Skip to content

Fix port conflict error message and cleanup on failure #40102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
beena352 wants to merge 1 commit into
base: feature/wsl-for-apps
Choose a base branch
from
+23 −12
Draft

Fix port conflict error message and cleanup on failure #40102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/windows/wslc/services/ContainerService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,14 +281,16 @@ int ContainerService::Run(Session& session, const std::string& image, ContainerO
{
// Create the container
auto runningContainer = CreateInternal(session, image, runOptions);
runningContainer.SetDeleteOnClose(false);
auto& container = runningContainer.Get();

// 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);

Comment on lines 286 to +293
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
// Handle attach if requested
if (WI_IsFlagSet(startFlags, WSLCContainerStartFlagsAttach))
{
Expand Down
31 changes: 20 additions & 11 deletions src/windows/wslcsession/WSLCContainer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1274,12 +1274,10 @@ std::unique_ptr<WSLCContainerImpl> WSLCContainerImpl::Open(
{
auto allocation = virtualMachine.TryAllocatePort(e.VmPort, e.Family, e.Protocol);

THROW_HR_IF_MSG(
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);
Comment on lines +1277 to +1280
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

inserted.VmMapping.AssignVmPort(allocation);
}
Expand Down Expand Up @@ -1443,20 +1441,31 @@ void WSLCContainerImpl::MapPorts()
auto allocatedPort =
m_virtualMachine.TryAllocatePort(e.ContainerPort, e.VmMapping.BindAddress.si_family, e.VmMapping.Protocol);

THROW_HR_IF_MSG(
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);
Comment on lines +1444 to +1447
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.

e.VmMapping.AssignVmPort(allocatedPort);

allocatedPorts.emplace(e.ContainerPort, allocatedPort);
}
}

m_virtualMachine.MapPort(e.VmMapping);
try
{
m_virtualMachine.MapPort(e.VmMapping);
}
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));
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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));

Copilot uses AI. Check for mistakes.
}
Comment on lines +1459 to +1466
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
throw;
}
}
}

Expand Down