Add container restart runtime support

[beena352]

Summary of the Pull Request

Adds container restart support to the runtime service layer. Implements the backend Restart() API that stops a container, waits for it to exit, then starts it again as an atomic operation. Prevents external Stop() and Start() calls from landing between the two phases via a mutual-exclusion guard.

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

This PR brings the runtime component of container restart to the service boundary (the COM interface layer). Changes:

• WSLCContainer.cpp/.h: RestartTransaction struct to mark restart phases as atomic; Restart() method that runs stop + start with the transaction held, preventing external operations from landing between phases. Includes guards for delete-during-restart and restart-during-delete races
• wslc.idl: Restart(Signal, TimeoutSeconds, WarningCallback) COM interface method
• wslcsdk.h: New error code WSLC_E_CONTAINER_DELETED for containers deleted while a restart is in flight

The CLI command layer that consumes this API is in #41435 (depends on this PR).

Validation Steps Performed

[Copilot]

Pull request overview

This PR adds a first-class “container restart” operation to the WSLC container runtime, including concurrency semantics that treat restart as a two-phase (stop + start) transaction, and verifies expected behavior with new end-to-end tests.

Changes:

• Add IWSLCContainer::Restart(...) to the WSLC COM interface (wslc.idl) and implement it in WSLCContainer/WSLCContainerImpl.
• Introduce a restart transaction mechanism (m_restart + completion event) so external Start()/Stop() calls cannot interleave between restart phases, while Delete() can still race between phases.
• Add ContainerRestart coverage in WSLCTests.cpp, including expected behavior for --rm containers and restart-vs-stop/delete race handling.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
test/windows/WSLCTests.cpp	Adds ContainerRestart tests, including race scenarios and --rm behavior expectations.
src/windows/wslcsession/WSLCContainer.h	Adds restart-related APIs and synchronization helpers to the container implementation.
src/windows/wslcsession/WSLCContainer.cpp	Implements restart phases, restart transaction coordination, and adjusted auto-remove behavior during restart.
src/windows/service/inc/wslc.idl	Extends IWSLCContainer with Restart and adds WSLC_E_CONTAINER_MARKED_FOR_REMOVAL.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/windows/WslcSDK/wslcsdk.h:31

• The comment says error code definitions must be kept in sync with “wslcsdk.idl”, but the only wslcsdk.idl in-tree (src/windows/WslcSDK/winrt/wslcsdk.idl) is a WinRT projection and does not define these HRESULT constants. This makes the guidance misleading for future updates; either point to the correct source-of-truth file(s) or drop the reference to wslcsdk.idl.

// WSLC specific error codes
// Ensure wslc.idl and wslcsdk.idl are also updated.
#define WSLC_E_BASE (0x0600)
#define WSLC_E_IMAGE_NOT_FOUND MAKE_HRESULT(SEVERITY_ERROR, FACILITY_ITF, WSLC_E_BASE + 1)               /* 0x80040601 */

[Kevin Vega (kvega005)]

Restart shouldn't tear down and re-acquire runtime resources

Right now Restart() = StopPhase() + StartPhase(), and the stop half goes through OnStopped() → ReleaseRuntimeResources() → UnmapPorts() + UnmountVolumes(). The start half then re-runs MountVolumes() + MapPorts(). So a restart drops every host/VM-side reservation the container already legitimately owned and races to re-take it.

Concrete failure modes:

• UnmapPorts() resets VmMapping.VmPort for host mode, returning the VM port to the pool. MapPorts() then re-runs TryAllocatePort() and throws WSAEADDRINUSE / MessageWslcPortInUse if anything grabbed it in the gap. wslc restart failing with "port already in use" naming the container's own port is not a sensible outcome.
• Same for the host side: VmMapping.Unmap() releases the reservation, so any other process on the box can take the published port during the window, and MapPort() comes back ERROR_ALREADY_EXISTS/WSAEADDRINUSE.
• UnmountVolumes() + MountVolumes() round-trips every bind mount through the VM. If the host path was removed while the container was running, restart now fails on a mount error — or worse, silently re-creates the directory, because MountVolumes() honours CreateSourceIfMissing on this path too.

And because none of this is atomic, a restart that fails in the start half leaves the container Exited with its resources gone. The caller asked for a restart and got a stop.

The container's ports and mounts are host/VM-scoped, not run-scoped — they should simply survive the restart. OnStopped() should skip ReleaseRuntimeResources() when the stop is part of a restart, and the start half should skip MapPorts()/MountVolumes() correspondingly.

Use Docker's /containers/{id}/restart instead of hand-rolling the two phases

POST /containers/{id}/restart?signal=&t= takes exactly the two parameters Restart(Signal, TimeoutSeconds) already has, and it handles the "container is currently stopped" case itself — which removes the wasRunning branch, the conditional StopPhase(), and a good chunk of the reason m_restart has to exist at all.

Worth noting the plumbing for this is already here and currently dead: DockerEventTracker.h:28 declares ContainerEvent::Restart and DockerEventTracker.cpp:164 already maps Docker's "restart" action onto it — but OnEvent() only handles Start/Stop/Destroy, so the event is delivered and dropped. DockerHTTPClient just needs a RestartContainer() alongside StopContainer()/StartContainer().

Docker still emits die → start → restart for this, so OnStopped() still has to know a restart is in flight and not release resources — that part is needed either way. But the payoff is that "restart" becomes one Docker call plus one expected terminal event, which is exactly the shape StateTransition is built for.

Smaller things

• Stop timeout isn't validated when the container is stopped. ValidateStopTimeout() only runs inside StopPhase()'s m_state == Running branch, and Restart() skips StopPhase() entirely when !wasRunning. So Restart(SIGTERM, -5) returns E_INVALIDARG on a running container and S_OK on a stopped one. Validate at the top of Restart().
• Plugin rejection turns a restart into a stop. StartPhase() calls OnContainerStarted() and, on failure, stops the container and throws. Combined with OnStopped() having already fired OnContainerStopping(), a plugin-guarded container that fails the restart is left down. Is that intended, or should a rejected restart roll back?
• The restart transaction ends before Restart() returns. CommitState() clears m_restart as soon as the start phase commits Running, i.e. while Restart() is still inside AttachToTransition(). The if (m_restart == restart) guard in restartCleanup exists because of that window. It looks benign today, but it means the header comment "keeps the pair indivisible" isn't quite what the code does — another reason to let a single transition own the whole operation and complete itself.

[beena352]

Restart shouldn't tear down and re-acquire runtime resources

Right now Restart() = StopPhase() + StartPhase(), and the stop half goes through OnStopped() → ReleaseRuntimeResources() → UnmapPorts() + UnmountVolumes(). The start half then re-runs MountVolumes() + MapPorts(). So a restart drops every host/VM-side reservation the container already legitimately owned and races to re-take it.

Concrete failure modes:

• UnmapPorts() resets VmMapping.VmPort for host mode, returning the VM port to the pool. MapPorts() then re-runs TryAllocatePort() and throws WSAEADDRINUSE / MessageWslcPortInUse if anything grabbed it in the gap. wslc restart failing with "port already in use" naming the container's own port is not a sensible outcome.
• Same for the host side: VmMapping.Unmap() releases the reservation, so any other process on the box can take the published port during the window, and MapPort() comes back ERROR_ALREADY_EXISTS/WSAEADDRINUSE.
• UnmountVolumes() + MountVolumes() round-trips every bind mount through the VM. If the host path was removed while the container was running, restart now fails on a mount error — or worse, silently re-creates the directory, because MountVolumes() honours CreateSourceIfMissing on this path too.

And because none of this is atomic, a restart that fails in the start half leaves the container Exited with its resources gone. The caller asked for a restart and got a stop.

The container's ports and mounts are host/VM-scoped, not run-scoped — they should simply survive the restart. OnStopped() should skip ReleaseRuntimeResources() when the stop is part of a restart, and the start half should skip MapPorts()/MountVolumes() correspondingly.

Use Docker's /containers/{id}/restart instead of hand-rolling the two phases

POST /containers/{id}/restart?signal=&t= takes exactly the two parameters Restart(Signal, TimeoutSeconds) already has, and it handles the "container is currently stopped" case itself — which removes the wasRunning branch, the conditional StopPhase(), and a good chunk of the reason m_restart has to exist at all.

Worth noting the plumbing for this is already here and currently dead: DockerEventTracker.h:28 declares ContainerEvent::Restart and DockerEventTracker.cpp:164 already maps Docker's "restart" action onto it — but OnEvent() only handles Start/Stop/Destroy, so the event is delivered and dropped. DockerHTTPClient just needs a RestartContainer() alongside StopContainer()/StartContainer().

Docker still emits die → start → restart for this, so OnStopped() still has to know a restart is in flight and not release resources — that part is needed either way. But the payoff is that "restart" becomes one Docker call plus one expected terminal event, which is exactly the shape StateTransition is built for.

Smaller things

• Stop timeout isn't validated when the container is stopped. ValidateStopTimeout() only runs inside StopPhase()'s m_state == Running branch, and Restart() skips StopPhase() entirely when !wasRunning. So Restart(SIGTERM, -5) returns E_INVALIDARG on a running container and S_OK on a stopped one. Validate at the top of Restart().
• Plugin rejection turns a restart into a stop. StartPhase() calls OnContainerStarted() and, on failure, stops the container and throws. Combined with OnStopped() having already fired OnContainerStopping(), a plugin-guarded container that fails the restart is left down. Is that intended, or should a rejected restart roll back?
• The restart transaction ends before Restart() returns. CommitState() clears m_restart as soon as the start phase commits Running, i.e. while Restart() is still inside AttachToTransition(). The if (m_restart == restart) guard in restartCleanup exists because of that window. It looks benign today, but it means the header comment "keeps the pair indivisible" isn't quite what the code does — another reason to let a single transition own the whole operation and complete itself.

You're right about the resources, that's a real bug. OnStopped() always calls ReleaseRuntimeResources() (:1512), and then the start half turns around and re-runs MountVolumes() (:1122) and MapPorts() (:1125). So a restart gives up reservations the container already owns and then races to get them back. Both failure modes are real, in host mode UnmapPorts() resets VmPort, so MapPorts() goes back through TryAllocatePort() and can throw WSAEADDRINUSE naming the container's own port (:2887), and on the host side Unmap()/MapPort() can come back ERROR_ALREADY_EXISTS in any mode. MountVolumes() hits CreateDirectoryDeepNoThrow when CreateSourceIfMissing is set (:443), so a bind source someone deleted while the container was running quietly comes back.

Worth noting moby avoids this deliberately, containerRestart wraps the stop+start in daemon.Mount(container) with a defer daemon.Unmount(container), commented "Avoid unnecessarily unmounting and then directly mounting the container when the container stops and then starts again." Same idea, they just hold it at a different layer.

The fix is to skip ReleaseRuntimeResources() when a restart is in flight and skip the matching re-acquire on the way back up. Just that one call though, ReleaseProcesses() and CommitState(Exited) still need to run.

The timeout is a real bug too. ValidateStopTimeout() lives inside the Running branch (:1376), and restart skips the stop phase entirely when the container isn't running. So a bad timeout gets rejected on a running container and accepted on a stopped one. I'll move the check to the top of Restart().

On the plugin rejection leaving the container down, Start() already behaves that way today, both go through the same StartPhase() (:1025 and :1483), so restart isn't introducing it. Happy to fix it, but I'd rather do that separately unless you feel strongly.

One thing worth pinning down: as you noted, the teardown skip is needed either way. Under /restart docker still emits die then start, so OnEvent() still sees the die and OnStopped() still releases, and there'd be no StartPhase() to re-acquire, so the container would come back up with its ports unmapped. So the resource fix is independent of the /restart question and I'll land it regardless of which way that one goes.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

[Copilot]

🟡 Changes recommended

The PR description references a different new error code name than what the implementation actually adds, and the mismatch should be reconciled to avoid an incorrect contract being communicated.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The restart API and transaction semantics are implemented consistently across COM/SDK/docs and are covered by targeted tests, with only minor documentation wording feedback.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite