Skip to content

CLI: container run and create --restart policy support - #41600

Draft
David Bennett (dkbennett) wants to merge 16 commits into
masterfrom
user/dkbennett/run-create-restart
Draft

David Bennett (dkbennett) wants to merge 16 commits into
masterfrom
user/dkbennett/run-create-restart

Conversation

@dkbennett

@dkbennett David Bennett (dkbennett) commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

Summary of the Pull Request

Adds Docker-compatible --restart policy support to wslc container create and wslc container run.

Supported policies are:

  • no
  • always
  • on-failure
  • on-failure:<max-retries>
  • unless-stopped

Docker 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

  • Closes: Link to issue #xxx
  • Communication: Discussed with core contributors
  • Tests: Added and passing
  • Localization: No unlocalized user-facing strings were added
  • Dev docs: Not required
  • Documentation updated: No external documentation change is required

Detailed Description

Typed restart-policy plumbing

The CLI parses --restart into WSLCContainerRestartPolicy, consistent with other named WSLC enums. The enum contains None, Always, OnFailure, and UnlessStopped; Invalid is used as the malformed-value sentinel during parsing and validation. The maximum retry count remains a separate integer and is valid only for OnFailure.

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:

  • Policy names are case-sensitive.
  • Retry counts are accepted only for on-failure.
  • Invalid, negative, malformed, and overflowing retry counts are rejected.
  • A non-no restart policy conflicts with --rm.
  • --restart no --rm remains 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:

  • Windows-backed bind mounts.
  • Published host port mappings and relays.
  • The session VM activity hold.

Manual stop intent is published before the Docker stop request so a racing die event 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 and State.StartedAt to identify the active run, replace the cached init-process wrapper, and publish the WSLC running state. Docker event timeNano is used when available. When it is unavailable, whole-second ordering is combined with the exact StartedAt run identity so same-second replacements remain distinguishable.

Delayed die events 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 ContainerStarted callback rejects that start and WSLC stops the container.

For a Docker policy restart:

  1. The exited run produces ContainerStopping.
  2. Docker starts the replacement according to its restart policy.
  3. After WSLC reconciles the replacement, it produces ContainerStarted with current inspect data.
  4. Callback failure is logged as a telemetry failure and does not stop the Docker-created replacement.

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(), including Created and Exited wrappers. 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.ps1 passed for the changed C++ files.
  • Full x64 Debug build passed.
  • Focused plugin restart-lifecycle test passed: 1 of 1.
    • Verified four distinct Docker runs.
    • Verified strict Start -> Stop alternation through three automatic retries and final teardown.
    • Verified plugin reentry through WSLCCreateProcess from automatic start and stop callbacks.
    • Verified a deliberate ERROR_ACCESS_DENIED from the first replacement-start callback did not stop later Docker retries.
  • Isolated retry-exhaustion and port-release test passed after the non-fast deployment setup: 1 of 1.
  • Restart-focused wildcard suite passed: 30 of 30.
Restart coverage Behavior
Policy configuration Enum propagation and Docker-compatible inspect serialization
on-failure Restarts after nonzero exit and honors maximum retries
always Restarts after successful exit
Retry exhaustion Stops after the configured attempt count and releases the published host port
Manual stop Does not create an automatic restart transaction
Explicit restart Supersedes a pending Docker policy restart correctly
Resource continuity Init process, bind mounts, published ports, and VM activity survive replacement
Plugin telemetry Ordered stop and replacement-start callbacks with best-effort failure handling
Plugin reentry Lifecycle callbacks can call WSLCCreateProcess without deadlocking
Missed events and recovery StartedAt identifies and reconciles the active Docker run
Delayed Docker events A stale stop event cannot overwrite the state of a newer running retry
--rm interaction Rejects non-no policies and permits --restart no --rm

Copilot AI lite review requested due to automatic review settings September 14, 2026 05:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 != 0 instead of whether a suffix was supplied allows always:0, no:0, and unless-stopped:0 even though retry counts are supported only for on-failure. Gate this rejection on parts.HadSeparator so 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-failure policies. It therefore accepts always:0, no:0, and unless-stopped:0; the earlier HadSeparator && !Value.empty() condition also accepts malformed empty suffixes such as always:. This contradicts the documented grammar and the stated rule that retry counts are valid only for on-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:0 as valid, but the supported grammar lists bare always and permits the :<max-retries> form only for on-failure. It conflicts with the PR's stated validation contract and would prevent tightening the parser; change it to expect ArgumentException and add cases for no:0 and 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.

Comment thread src/windows/inc/docker_schema.h
Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Comment thread src/windows/wslc/arguments/SpecParsing.cpp
Copilot AI review requested due to automatic review settings September 14, 2026 05:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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 Restarting field, but recovery still has no path for a container that is already in restart backoff when the session/VM is recreated. Open() maps ContainerState::Restarting to WslcContainerStateInvalid, and no policy transaction or activity hold is armed, so the later start event 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: and always: are accepted as valid policies. These are malformed retry specifications; enter the validation block whenever a colon was supplied so the existing retryView.empty() check rejects them.
    if (parts.HadSeparator && !parts.Value.empty())
    {

src/windows/wslc/arguments/SpecParsing.cpp:705

  • GetRestartPolicyFromString is only called for a present argument, so an explicit --restart= reaches this branch and is silently converted to no. That also bypasses the --rm conflict 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 Restarting and Running can leave the transaction armed forever if the Docker start event is lost during an event-stream reconnect: the monitor observes a running container on every poll, but WSLC remains Exited, 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 Restarting still constructs the wrapper through Open() without arming this policy transaction; DockerStateToWSLCState maps that Docker state to WslcContainerStateInvalid. The recovered container therefore has no monitor or pending-policy lifecycle, exposes an invalid WSLC state, and can hit the Delete() invalid-state assertion. Recovery must recognize Restarting as 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; unlike StartPhase, it never creates a replacement WSLCProcess/DockerContainerProcessControl. OnStopped() has already signaled the old init process as exited, while GetInitProcess() continues returning m_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 cover GetInitProcess/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

Copilot AI review requested due to automatic review settings September 14, 2026 05:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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::Restarting is now parsed, but WSLCContainerImpl::DockerStateToWSLCState() still maps it to WslcContainerStateInvalid. 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: (and always:). That contradicts the documented on-failure[:max-retries] syntax and lets malformed retry policies reach Docker; enter the parsing block whenever parts.HadSeparator is 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 default no policy. Because absent arguments already get the default through ArgMap::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, and GetInitProcess() returns the cached object, so callers receive an exited process after the first automatic restart even though container exec works. Reinstall the init-process tracking here (as StartPhase() 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 OnStopped event. If the WSL service/session is reconstructed while Docker already reports Restarting, Open() still maps that state to WslcContainerStateInvalid; no m_restart is created, so the later Start event is treated as unexpected and the recovered wrapper remains unusable. Reconcile Restarting containers 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

Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Copilot AI review requested due to automatic review settings September 14, 2026 16:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

  • OnStopped can process a user-initiated container stop before StopPhase publishes its stop transition (that transition is created only after the Docker request). For an always, unless-stopped, or nonzero on-failure policy, 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 and StartPhase rejects an immediate container start while that transaction is pending; a transient Running inspect 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

Comment thread src/windows/wslcsession/WSLCContainer.cpp
Copilot AI review requested due to automatic review settings September 14, 2026 17:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 makes container 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 use THROW_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

Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Copilot AI review requested due to automatic review settings September 14, 2026 17:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Comment thread src/windows/wslcsession/WSLCSessionRuntime.cpp
Copilot AI review requested due to automatic review settings September 14, 2026 18:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 (and on-failure with a nonzero stop exit), Docker's stop endpoint leaves State.Restarting/Running false but this expression still arms m_restart; OnStopped then retains mounts, port relays, and the activity hold, and an immediate normal container rm can 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

Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Comment thread src/windows/wslcsession/WSLCSession.cpp
Comment thread test/windows/WSLCTests.cpp Outdated
Copilot AI review requested due to automatic review settings September 16, 2026 01:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 PrepareForUnexpectedStartLockHeld fails, OnEvent(Start) still calls this reconciler with m_state left as Running and no policy transaction armed. A later successful inspect then hits this early return solely because WSLC is Running, 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) with m_initProcessStartedAt and arm reconciliation when they differ.
        if (m_state == WslcContainerStateRunning || (!dockerInspect.State.Running && !dockerInspect.State.Restarting))
        {
            return;

src/windows/wslcsession/WSLCContainer.cpp:1544

  • Kill() calls StopPhase(..., Kill=true), so this expression leaves manualStopRequest false. The following kill event is returned without changing lifecycle state, and the later die event can therefore arm m_restart for an always or failing on-failure container, causing wslc container kill to be followed by an automatic restart. Treat a user kill like a user stop here; the policy-pending branch already uses cancelPolicyRestart to 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

Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Comment thread src/windows/wslcsession/WSLCContainer.cpp
Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Copilot AI review requested due to automatic review settings September 16, 2026 18:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 a NotifyingPlugin phase and invokes the external plugin for the observed Docker run, but this implementation explicitly skips OnContainerStarted for 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 the exited state; 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 TearDownVmLockHeld can concurrently tear down the Docker client. A shared runtime lock cannot simply be acquired at this line because OnStopped already 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

Comment thread src/windows/wslcsession/WSLCContainer.cpp
Copilot AI review requested due to automatic review settings September 18, 2026 06:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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

Comment thread src/windows/inc/WslPluginApi.h Outdated
Comment thread src/windows/service/exe/PluginManager.cpp Outdated
Copilot AI review requested due to automatic review settings September 18, 2026 18:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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

Copilot AI review requested due to automatic review settings September 23, 2026 01:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 High severity

Open (4)
Resolved since last review (1)

Comment thread src/windows/wslcsession/WSLCContainer.cpp
Comment thread src/windows/wslcsession/WSLCContainer.cpp
Copilot AI review requested due to automatic review settings September 24, 2026 17:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 High severity

Open (2)
Resolved since last review (4)

Comment thread src/windows/wslcsession/WSLCContainer.cpp Outdated
Comment thread src/windows/wslcsession/WSLCContainer.cpp
Copilot AI review requested due to automatic review settings September 25, 2026 22:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Unresolved parser-contract and container-recovery findings remain.

Review effort: Lite
Findings: None

Resolved since last review (2)

Copilot AI review requested due to automatic review settings September 28, 2026 21:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants