Add support for wslc container restart command#41005
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new wslc container restart subcommand that mirrors Docker CLI semantics by stopping (with configurable --signal/--time) and then starting a container, with end-to-end wiring from CLI → service layer → COM interface → session implementation, plus unit/E2E coverage and localized strings.
Changes:
- Add a new CLI command
wslc container restartand route it throughContainerTasksandContainerService. - Extend the internal
IWSLCContainerCOM interface withRestart(...)and implement restart logic inWSLCContainerImpl. - Add unit and E2E tests, update container subcommand help expectations, and add localization strings for help + user-facing errors.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/WSLCTests.cpp | Adds unit tests for container restart behaviors and error cases. |
| test/windows/wslc/e2e/WSLCE2EContainerTests.cpp | Updates container command help to include the new restart subcommand. |
| test/windows/wslc/e2e/WSLCE2EContainerRestartTests.cpp | New E2E tests validating container restart CLI behavior and help output. |
| src/windows/wslcsession/WSLCContainer.h | Adds restart APIs on WSLCContainerImpl and COM WSLCContainer. |
| src/windows/wslcsession/WSLCContainer.cpp | Implements restart logic and exposes it via COM with warning callback context. |
| src/windows/wslc/tasks/ContainerTasks.h | Declares RestartContainers task. |
| src/windows/wslc/tasks/ContainerTasks.cpp | Implements CLI task parsing/loop for restarting one or more containers. |
| src/windows/wslc/services/ContainerService.h | Adds ContainerService::Restart declaration. |
| src/windows/wslc/services/ContainerService.cpp | Implements Restart by opening the container and calling COM Restart. |
| src/windows/wslc/commands/ContainerRestartCommand.cpp | New command implementation for argument wiring + execution pipeline. |
| src/windows/wslc/commands/ContainerCommand.h | Declares the ContainerRestartCommand type. |
| src/windows/wslc/commands/ContainerCommand.cpp | Registers restart as a container subcommand. |
| src/windows/service/inc/wslc.idl | Extends IWSLCContainer with the Restart method. |
| localization/strings/en-US/Resources.resw | Adds localized strings for restart command descriptions and --rm refusal message. |
| std::vector<Argument> ContainerRestartCommand::GetArguments() const | ||
| { | ||
| return { | ||
| Argument::Create(ArgType::ContainerId, std::nullopt, NO_LIMIT), |
There was a problem hiding this comment.
Intentionally matches ContainerStopCommand's pattern, restart is the lifecycle sibling of stop (same -s/-t flags, same semantics). If we change restart to require the arg, we should also change stop for consistency, which is a separate discussion.
There was a problem hiding this comment.
Agree we should change those too, this one should probably have required arg true for container-id. I can put out a quick PR to change those to required so they are consistent with Docker.
There was a problem hiding this comment.
PR for fixing stop: #41108
So you can change this one too.
OneBlue
left a comment
There was a problem hiding this comment.
Unfortunately I'm not sure that Restart() can be easily implemented with our existing locking story.
We have a work item to rethink the container state machine & locking logic to allow concurrent Stop() & Kill() calls, I think we're unfortunately going to have for that change before we can implement restart
| } | ||
|
|
||
| // Refuse --rm before stopping: Stop -> OnStopped would delete the container. | ||
| THROW_HR_WITH_USER_ERROR_IF( |
There was a problem hiding this comment.
I don't think this is the behavior we want: Docker supports restarting containers when --rm is set.
| snapshotFlags = m_containerFlags; | ||
| } | ||
|
|
||
| // Refuse --rm before stopping: Stop -> OnStopped would delete the container. |
There was a problem hiding this comment.
Locking wise, I don't think that we can drop the lock here unfortunately, otherwise another stop / start operation could arrive before we complete, which would leak to weird states
Marking this PR as Draft and blocked by Deliverable 62987921 (“[WSL] [Containers] Rethink the WSLCContainer state machine”). |
dkbennett
left a comment
There was a problem hiding this comment.
Command implementation looks good, some minor things. Main thing to draw to attention is the change in how help is validated across the CLI from the recent help change. All other commands have been updated for it, so this one could just mimic what other container commands are now doing with regards to help tests.
| const std::wstring WslcContainerName2 = L"wslc-test-container-2"; | ||
| const TestImage& DebianImage = DebianTestImage(); | ||
|
|
||
| std::wstring GetHelpMessage() const |
There was a problem hiding this comment.
All of the help test validation has changed since this PR was created. It was changed for the better - you don't need to have precise help output validation anymore, so some bits can be cut from this PR.
| THROW_IF_FAILED_EXCEPT(container->Stop(options.Signal, options.Timeout), WSLC_E_CONTAINER_NOT_RUNNING); | ||
| } | ||
|
|
||
| void ContainerService::Restart(Session& session, const std::string& id, StopContainerOptions options) |
There was a problem hiding this comment.
minor nit: The ContainerService seems to have lost its alphabetical function ordering, would recommend moving the function (and header) to be as best in alphabetical as you can make it, at least as close as you can make it to other stuff, like before a cluster of S's or what similar.
| return Localization::WSLCCLI_ContainerRestartLongDesc(); | ||
| } | ||
|
|
||
| // clang-format off |
There was a problem hiding this comment.
nit: we try to avoid the clang-format off now. Other commands will put empty comments as line breaks in the context chain in ExecuteInternal instead.
See: ContainerStatsCommand.cpp as an example.
We just do this for readability of the chain and this is a nice compromise for having that readability while keeping the clang formatter enabled.
| std::vector<Argument> ContainerRestartCommand::GetArguments() const | ||
| { | ||
| return { | ||
| Argument::Create(ArgType::ContainerId, std::nullopt, NO_LIMIT), |
There was a problem hiding this comment.
Agree we should change those too, this one should probably have required arg true for container-id. I can put out a quick PR to change those to required so they are consistent with Docker.
Summary of the Pull Request
Adds wslc container restart which stops and then starts a container, matching Docker CLI semantics. Supports
--signaland--timeoptions (defaults: SIGTERM, 5s timeout).PR Checklist
Detailed Description of the Pull Request / Additional comments
Behavior:
--rmcontainer: refused withERROR_INVALID_STATE— Stop would trigger auto-delete viaOnStopped, destroying the container.WSLC_E_CONTAINER_NOT_FOUND.Implementation path:
ContainerRestartCommand→ContainerTasks::RestartContainers→ContainerService::Restart→IWSLCContainer::Restart→WSLCContainerImpl::Restart.N.B. Lock design:
WSLCContainerImpl::Restartsnapshots state/flags under a shared lock, then releases before callingStopandStart(which each take the lock exclusively). This is intentional -m_lockis a non-recursivewil::srwlock. If the container self-exits between the snapshot and Stop, Stop no-ops on the Exited state.Follow-up: To achieve full Docker parity for
--rmcontainers (Docker allows restart on--rmby suppressing auto-remove during the restart window), a follow-up PR will add a member flagm_restartInProgressguarded bywil::scope_exitand checked inOnStopped. This closes the race where a natural container exit between Stop and Start would trigger auto-delete. Shipping the--rmguard (refuse) now is the safe default; the member-flag approach needs its own review and dedicated tests for the self-exit race.Validation Steps Performed
Unit tests (
test/windows/WSLCTests.cpp—ContainerRestart):--rmcontainer → refused withERROR_INVALID_STATEand exact localized messageE2E tests (
test/windows/wslc/e2e/WSLCE2EContainerRestartTests.cpp):--helpoutput--rmcontainer refused with exact error + HRESULTManual validation: