CLI: container run and create --restart policy support - #41600
David Bennett (dkbennett) wants to merge 16 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical lifecycle/recovery and restart-policy validation issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Docker-compatible --restart policy support to wslc container create and run, including lifecycle tracking and resource preservation during automatic restarts.
Changes:
- Adds restart-policy parsing, validation, API plumbing, and inspect output.
- Extends WSLC restart transactions and monitoring for Docker policy restarts.
- Adds schema, localization, unit, CLI, and end-to-end tests.
File summaries
| File | Reviewed changes and findings |
|---|---|
test/windows/wslc/WSLCCLIExecutionUnitTests.cpp |
Tests restart-option plumbing. |
test/windows/wslc/WSLCCLIArgumentUnitTests.cpp |
Tests policy parsing. Nit (1 vote, line 217): expects always:0 to be valid; update it and add no:0 and empty-suffix cases. |
test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp |
Tests automatic restart behavior and conflicts. |
test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp |
Tests create policies and conflicts. |
test/windows/wslc/CommandLineTestCases.h |
Adds CLI acceptance cases. |
src/windows/wslcsession/WSLCSessionRuntime.h |
Declares shared-lock probing. |
src/windows/wslcsession/WSLCSessionRuntime.cpp |
Implements shared-lock probing. |
src/windows/wslcsession/WSLCContainer.h |
Defines policy restart transactions and monitoring. |
src/windows/wslcsession/WSLCContainer.cpp |
Integrates policy restart lifecycle handling. Critical (3 votes, line 1860): a lost start event can leave the transaction armed when Docker is already Running; reconcile that state. Critical (2 votes, line 1329): recovery during Restarting does not arm or reconcile the policy transaction. Critical (1 vote, line 1347): the policy-start path does not recreate the init-process wrapper. |
src/windows/wslc/tasks/ContainerTasks.cpp |
Maps CLI policies to container options. |
src/windows/wslc/services/ContainerService.cpp |
Forwards policies to the launcher. |
src/windows/wslc/services/ContainerModel.h |
Defines the typed restart policy. |
src/windows/wslc/commands/ContainerRunCommand.cpp |
Registers and validates --restart. |
src/windows/wslc/commands/ContainerCreateCommand.cpp |
Registers and validates --restart. |
src/windows/wslc/commands/ContainerCommand.h |
Updates container command declarations. |
src/windows/wslc/arguments/SpecParsing.h |
Documents the restart parser API. |
src/windows/wslc/arguments/SpecParsing.cpp |
Parses and validates restart policies. Moderate (2 votes, line 715): accepts empty suffixes such as on-failure:. Moderate (1 vote, line 736): accepts invalid forms such as always:0, no:0, and unless-stopped:0. |
src/windows/wslc/arguments/ArgumentValidation.cpp |
Caches typed restart arguments. |
src/windows/wslc/arguments/ArgumentDefinitions.h |
Defines the restart argument. |
src/windows/wslc/arguments/ArgumentConvertedTypes.h |
Adds the converted policy type. |
src/windows/service/inc/wslc.idl |
Extends container creation options. |
src/windows/inc/wslc_schema.h |
Exposes restart policy in inspect output. |
src/windows/inc/docker_schema.h |
Models Docker restart policy and state. Critical (1 vote, line 508): recovered Restarting containers map to an invalid WSLC state without restoring the policy transaction and activity hold. |
src/windows/common/WSLCContainerLauncher.h |
Declares launcher policy configuration. |
src/windows/common/WSLCContainerLauncher.cpp |
Forwards policy configuration. |
localization/strings/en-US/Resources.resw |
Adds localized CLI strings. |
Review details
Suppressed comments (3)
src/windows/wslc/arguments/SpecParsing.cpp:736
- Checking
maximumRetryCount != 0instead of whether a suffix was supplied allowsalways:0,no:0, andunless-stopped:0even though retry counts are supported only foron-failure. Gate this rejection onparts.HadSeparatorso any:<count>on the other policies is rejected.
if (parts.Key != L"on-failure" && maximumRetryCount != 0)
src/windows/wslc/arguments/SpecParsing.cpp:738
- Because the retry count defaults to zero, this check only rejects nonzero counts for non-
on-failurepolicies. It therefore acceptsalways:0,no:0, andunless-stopped:0; the earlierHadSeparator && !Value.empty()condition also accepts malformed empty suffixes such asalways:. This contradicts the documented grammar and the stated rule that retry counts are valid only foron-failure; validate separator presence and suffix syntax explicitly and add regression cases.
if (parts.Key != L"on-failure" && maximumRetryCount != 0)
{
throw ArgumentException(Localization::WSLCCLI_InvalidRestartPolicyError(argName, input));
test/windows/wslc/WSLCCLIArgumentUnitTests.cpp:217
- This assertion codifies
always:0as valid, but the supported grammar lists barealwaysand permits the:<max-retries>form only foron-failure. It conflicts with the PR's stated validation contract and would prevent tightening the parser; change it to expectArgumentExceptionand add cases forno:0and empty suffixes.
VERIFY_NO_THROW(validation::GetRestartPolicyFromString(L"always:0"));
- Files reviewed: 26/26 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Moderate issues remain in restart recovery, policy validation, missed-start reconciliation, and init-process recreation.
Review details
Suppressed comments (6)
src/windows/inc/docker_schema.h:508
- This adds the Docker
Restartingfield, but recovery still has no path for a container that is already in restart backoff when the session/VM is recreated.Open()mapsContainerState::RestartingtoWslcContainerStateInvalid, and no policy transaction or activity hold is armed, so the laterstartevent is treated as unexpected and the container can remain invalid without preserved resources. Detect and restore the policy-restart transaction during container recovery.
bool Restarting{};
src/windows/wslc/arguments/SpecParsing.cpp:716
- Because this condition skips the parser when the suffix is empty, both
on-failure:andalways:are accepted as valid policies. These are malformed retry specifications; enter the validation block whenever a colon was supplied so the existingretryView.empty()check rejects them.
if (parts.HadSeparator && !parts.Value.empty())
{
src/windows/wslc/arguments/SpecParsing.cpp:705
GetRestartPolicyFromStringis only called for a present argument, so an explicit--restart=reaches this branch and is silently converted tono. That also bypasses the--rmconflict check; reject an empty value instead of treating it as the absent-option default.
if (input.empty())
{
return {};
src/windows/wslcsession/WSLCContainer.cpp:1862
- Returning for both
RestartingandRunningcan leave the transaction armed forever if the Dockerstartevent is lost during an event-stream reconnect: the monitor observes a running container on every poll, but WSLC remainsExited, keeps the ports/mounts and VM hold, and rejects normal starts. Reconcile the running state here (including the normal start notification/state transition) rather than treating it as an unresolved restart, and add coverage for a missed start event.
if (inspect.State.Restarting || inspect.State.Running)
{
return;
src/windows/wslcsession/WSLCContainer.cpp:1769
- A service/VM recovery while Docker reports
Restartingstill constructs the wrapper throughOpen()without arming this policy transaction;DockerStateToWSLCStatemaps that Docker state toWslcContainerStateInvalid. The recovered container therefore has no monitor or pending-policy lifecycle, exposes an invalid WSLC state, and can hit theDelete()invalid-state assertion. Recovery must recognizeRestartingas a policy-pending state and initialize the transaction/monitor (or otherwise normalize it to a valid exited state).
__requires_exclusive_lock_held(m_lock) void WSLCContainerImpl::ArmPolicyRestartLockHeld()
{
WI_ASSERT(!m_restart);
auto restart = std::make_shared<RestartTransaction>(RestartSource::Policy);
StartPolicyRestartMonitor();
m_restart = std::move(restart);
UpdateActivityHoldLockHeld();
src/windows/wslcsession/WSLCContainer.cpp:1353
- The policy-start branch only commits
Running; unlikeStartPhase, it never creates a replacementWSLCProcess/DockerContainerProcessControl.OnStopped()has already signaled the old init process as exited, whileGetInitProcess()continues returningm_initProcess, so after an automatic restart clients receive a permanently exited init process even though the container is running. Recreate the init-process state as part of policy-start reconciliation and coverGetInitProcess/attach after a policy restart.
if (accepted)
{
CommitState(WslcContainerStateRunning, eventTime);
// Policy resolution also disarms the abandonment monitor, signals lifecycle
// waiters, and updates the VM activity hold.
ResolvePolicyRestartLockHeld(false);
- Files reviewed: 26/26 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate lifecycle, recovery, and argument-validation findings must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
src/windows/inc/docker_schema.h:508
ContainerState::Restartingis now parsed, butWSLCContainerImpl::DockerStateToWSLCState()still maps it toWslcContainerStateInvalid. If the session recovers while Docker is in restart backoff,RecoverExistingContainers()installs an unusable wrapper; the subsequent start event has no policy transaction to reconcile it, so the container remains invalid and its WSLC lifecycle/resources are not restored. Recovery needs an explicit Restarting reconciliation path.
bool Restarting{};
src/windows/wslc/arguments/SpecParsing.cpp:715
- An explicit separator with no value is accepted because this block is skipped for inputs such as
on-failure:(andalways:). That contradicts the documentedon-failure[:max-retries]syntax and lets malformed retry policies reach Docker; enter the parsing block wheneverparts.HadSeparatoris true so the existing empty-value check rejects them.
if (parts.HadSeparator && !parts.Value.empty())
src/windows/wslc/arguments/SpecParsing.cpp:705
- An explicit
--restart=is parsed as a present value with an empty string, but this branch returns the defaultnopolicy. Because absent arguments already get the default throughArgMap::GetValue, an empty supplied value should be rejected as an invalid policy instead of silently enabling--rm/no-restart semantics.
if (input.empty())
{
return {};
src/windows/wslcsession/WSLCContainer.cpp:1353
- This path marks the container Running after Docker's policy start, but it never creates a new
DockerContainerProcessControl/WSLCProcess.OnStopped()has already signaled the previous init process, andGetInitProcess()returns the cached object, so callers receive an exited process after the first automatic restart even thoughcontainer execworks. Reinstall the init-process tracking here (asStartPhase()does) before publishing Running.
if (accepted)
{
CommitState(WslcContainerStateRunning, eventTime);
// Policy resolution also disarms the abandonment monitor, signals lifecycle
// waiters, and updates the VM activity hold.
ResolvePolicyRestartLockHeld(false);
src/windows/wslcsession/WSLCContainer.cpp:1677
- This new policy transaction is only armed from the live
OnStoppedevent. If the WSL service/session is reconstructed while Docker already reportsRestarting,Open()still maps that state toWslcContainerStateInvalid; nom_restartis created, so the later Start event is treated as unexpected and the recovered wrapper remains unusable. ReconcileRestartingcontainers during recovery (or arm the policy transaction there) before relying on this path.
if (m_state == WslcContainerStateRunning && !transition && !m_restart)
{
try
{
const auto inspect = m_runtime.Docker().InspectContainer(m_id);
- Files reviewed: 26/26 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The inspect restart state is not populated, and explicit stops can incorrectly trigger policy-restart handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/windows/wslcsession/WSLCContainer.cpp:1641
OnStoppedcan process a user-initiatedcontainer stopbeforeStopPhasepublishes its stop transition (that transition is created only after the Docker request). For analways,unless-stopped, or nonzeroon-failurepolicy, this predicate then arms a policy transaction even though Docker intentionally leaves an explicitly stopped container exited. The container temporarily retains its ports/VM hold andStartPhaserejects an immediatecontainer startwhile that transaction is pending; a transientRunninginspect can also be mistaken for a policy restart. Record the explicit stop intent before issuing the Docker request, or otherwise exclude these exits from policy-restart detection.
policyRestartPending = inspect.State.Restarting || inspect.State.Running || restartPolicy == "always" ||
restartPolicy == "unless-stopped" || (restartPolicy == "on-failure" && exitCode != 0);
- Files reviewed: 27/27 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
An unresolved recovery deadlock and inaccurate restart-age reporting require correction.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/windows/wslcsession/WSLCContainer.cpp:1869
- The monitor's fallback path is the one used when the Docker start event was missed, but it records the WSLC transition with the observer's current time instead of Docker's authoritative
State.StartedAt. This makescontainer list/status report an inaccurate relative age for the restarted container; use the same StartedAt-to-epoch conversion as the recovery path before calling reconciliation.
container.ReconcilePolicyRestartStartedLockHeld(inspect, std::time(nullptr));
src/windows/wslcsession/WSLCContainer.cpp:1752
- This new failure branch uses a bare
if (FAILED(...)), contrary to the repository's WIL error-handling convention. Keep the diagnostic log, then useTHROW_IF_FAILED(pluginResult)so this path has the same failure semantics without introducing a separate HRESULT test.
if (FAILED(pluginResult))
{
LOG_HR_MSG(pluginResult, "Plugin rejected policy restart of container '%hs' (0x%x)", m_id.c_str(), pluginResult);
THROW_HR(pluginResult);
}
- Files reviewed: 27/27 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical stop-handling and moderate recovery-ordering findings remain.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved locking, restart-state, recovery, and test-path findings block safe approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/windows/wslcsession/WSLCContainer.cpp:1654
- These policy-name fallbacks also classify an explicit stop as a pending automatic restart. For
always/unless-stopped(andon-failurewith a nonzero stop exit), Docker's stop endpoint leavesState.Restarting/Runningfalse but this expression still armsm_restart;OnStoppedthen retains mounts, port relays, and the activity hold, and an immediate normalcontainer rmcan report the stopped container as active until the monitor clears it. Track explicit stop/kill intent through the event (or otherwise distinguish Docker's restart state) so only daemon-initiated restarts arm this transaction.
policyRestartPending = inspect.State.Restarting || inspect.State.Running || restartPolicy == "always" ||
restartPolicy == "unless-stopped" || (restartPolicy == "on-failure" && exitCode != 0);
- Files reviewed: 29/29 changed files
- Comments generated: 4
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Five unresolved findings include two critical restart-lifecycle races and missing policy-restart plugin reconciliation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/windows/wslcsession/WSLCContainer.cpp:1941
- If the Docker inspect in
PrepareForUnexpectedStartLockHeldfails,OnEvent(Start)still calls this reconciler withm_stateleft asRunningand no policy transaction armed. A later successful inspect then hits this early return solely because WSLC isRunning, so the newer Docker run is never reconciled or monitored and the cached init-process wrapper can remain stale. The guard needs to compare the Docker run (StartedAt/Restarting) withm_initProcessStartedAtand arm reconciliation when they differ.
if (m_state == WslcContainerStateRunning || (!dockerInspect.State.Running && !dockerInspect.State.Restarting))
{
return;
src/windows/wslcsession/WSLCContainer.cpp:1544
Kill()callsStopPhase(..., Kill=true), so this expression leavesmanualStopRequestfalse. The followingkillevent is returned without changing lifecycle state, and the laterdieevent can therefore armm_restartfor analwaysor failingon-failurecontainer, causingwslc container killto be followed by an automatic restart. Treat a user kill like a user stop here; the policy-pending branch already usescancelPolicyRestartto avoid marking cancellation as a new request.
const bool manualStopRequest = !Kill && !RestartPhase && !cancelPolicyRestart;
- Files reviewed: 37/37 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The critical VM teardown race and moderate retry-exhaustion cleanup issue remain unresolved, with a plugin retry-callback contract mismatch also noted.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
src/windows/inc/WslPluginApi.h:137
- The PR description says
ReconcilePolicyRestart()enters aNotifyingPluginphase and invokes the external plugin for the observed Docker run, but this implementation explicitly skipsOnContainerStartedfor policy retries and the added authorization-inheritance test expects that behavior. Please reconcile the advertised state machine and plugin-rejection validation with the shipped contract (either update the description/tests or invoke the callback for retries).
// Called when WSLC starts a container. Returning an error rejects the start and causes WSLC to stop
// the container. Docker restart-policy retries inherit the original approval and do not invoke this
// callback.
src/windows/wslcsession/WSLCContainer.cpp:1965
- When Docker exhausts a policy restart, this calls
OnFailedRestartExclusiveLockHeld(), whose implementation unconditionally force-deletes the container. That makes a normal retry exhaustion (and an externally/manual-stopped policy container) disappear instead of remaining inspectable in theexitedstate; the new retry-exhaustion test only observes it before the monitor's delete can race in. Use policy-specific failure cleanup that releases the retained resources/activity hold without invoking the explicit-restart deletion path.
ResolvePolicyRestartLockHeld(false);
[[maybe_unused]] const auto transition = OnFailedRestartExclusiveLockHeld();
src/windows/wslcsession/WSLCContainer.cpp:1777
- This new Docker inspection has the same lifetime race on the stop-event path: the callback runs without runtime protection, while
TearDownVmLockHeldcan concurrently tear down the Docker client. A shared runtime lock cannot simply be acquired at this line becauseOnStoppedalready runs under lifecycle/container locks; move the protected inspection before those locks or provide a safe fallback when runtime protection is unavailable.
dockerInspect = m_runtime.Docker().InspectContainer(m_id);
- Files reviewed: 38/38 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces broad container/session lifecycle and locking changes (including policy-restart reconciliation and VM startup barriers) that warrant careful human verification beyond automated review.
Review details
- Files reviewed: 39/39 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The change set materially alters WSLC container lifecycle reconciliation, recovery, and plugin notification ordering across multiple concurrency boundaries and needs final human review despite strong test additions.
Review details
- Files reviewed: 39/39 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical and moderate lifecycle/recovery findings remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 4
Open (4)
Release runtime lock before container-stopping callbacks · New Handle same-second replacement runs without timeNano · New The start-event path setsreconcilePolicyRestartand releases the locks before calling this… WhenStopContainerreturns HTTP 304, this decrementsm_manualStopRequestsbefore the queued…
Resolved since last review (1)
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Two critical and two moderate lifecycle issues remain, and the focused test rerun is pending.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2

Summary of the Pull Request
Adds Docker-compatible
--restartpolicy support towslc container createandwslc container run.Supported policies are:
noalwayson-failureon-failure:<max-retries>unless-stoppedDocker remains responsible for restart eligibility, retry limits, backoff, manual-stop behavior, persistence, daemon recovery, and starting replacement processes. WSLC observes those lifecycle transitions and reconciles its process wrapper, bind mounts, published ports, activity hold, inspect state, and plugin lifecycle notifications.
PR Checklist
Detailed Description
Typed restart-policy plumbing
The CLI parses
--restartintoWSLCContainerRestartPolicy, consistent with other named WSLC enums. The enum containsNone,Always,OnFailure, andUnlessStopped;Invalidis used as the malformed-value sentinel during parsing and validation. The maximum retry count remains a separate integer and is valid only forOnFailure.The typed policy flows through the CLI model, container launcher, and internal WSLC container options. It is converted to Docker's string representation only when constructing the Docker request. Docker and WSLC inspect continue to expose Docker-compatible policy names.
Validation is Docker-compatible:
on-failure.norestart policy conflicts with--rm.--restart no --rmremains valid.Docker-owned restart lifecycle
Docker policy restarts do not call the WSLC
Restart()API. WSLC therefore tracks a policy restart transaction when it observes an eligible exit and preserves resources that must survive until Docker either starts a replacement or exhausts the policy:Manual stop intent is published before the Docker stop request so a racing
dieevent is not classified as an automatic restart. Explicit stop, kill, restart, and delete operations cancel or supersede pending policy-restart handling according to their existing lifecycle semantics.Docker start events, fallback monitoring, missed-event handling, and startup recovery use the same
ReconcilePolicyRestart()path. The reconciler uses current Docker inspect state andState.StartedAtto identify the active run, replace the cached init-process wrapper, and publish the WSLC running state. Docker eventtimeNanois used when available. When it is unavailable, whole-second ordering is combined with the exactStartedAtrun identity so same-second replacements remain distinguishable.Delayed
dieevents are evaluated against Docker's current start time before WSLC mutates state. If the cached wrapper already represents the newer Docker run, the old event is ignored. If Docker represents a newer run than the wrapper, WSLC retires the old wrapper and enters the common policy reconciler.This is lifecycle reconciliation around Docker's restart state machine. WSLC does not reproduce Docker's restart-policy algorithms or maintain a separate durable retry counter.
Plugin lifecycle notifications
The current MDE plugin does not authorize per-container configuration or evaluate restart policy. MDE provisioning occurs at utility-VM startup. Container callbacks are required for logging and telemetry bookkeeping.
An explicit WSLC-directed start retains its existing behavior: a failing
ContainerStartedcallback rejects that start and WSLC stops the container.For a Docker policy restart:
ContainerStopping.ContainerStartedwith current inspect data.Lifecycle callbacks for each container are serialized and alternate in start/stop order. A stop cannot precede the first start, and two start or two stop notifications cannot be emitted consecutively for the same container.
Container lifecycle callbacks run without the runtime lock while a temporary activity hold keeps the VM available. This allows plugins to call back into WSLC, including
WSLCCreateProcess, without recursively acquiring the runtime SRW lock.Startup recovery
Every newly opened recovered container wrapper is queued for
CompleteRecovery(), includingCreatedandExitedwrappers. This clears the event-reconciliation gate only after the session releases its recovery locks.Running and restarting containers additionally enter the same policy-restart reconciler used by Docker events and the fallback monitor. Recovery uses a fresh Docker inspect result, restores WSLC runtime resources, replaces the init-process wrapper for the active Docker run, and emits the corresponding replacement-start lifecycle notification.
Validation
FormatSource.ps1passed for the changed C++ files.Start -> Stopalternation through three automatic retries and final teardown.WSLCCreateProcessfrom automatic start and stop callbacks.ERROR_ACCESS_DENIEDfrom the first replacement-start callback did not stop later Docker retries.on-failurealwaysWSLCCreateProcesswithout deadlockingStartedAtidentifies and reconciles the active Docker run--rminteractionnopolicies and permits--restart no --rm