Skip to content

wslc: assert Pid > 0 in DockerExecProcessControl::SetPid#40567

Merged
benhillis merged 1 commit into
microsoft:masterfrom
benhillis:fix/wslc-setpid-assert
May 16, 2026
Merged

wslc: assert Pid > 0 in DockerExecProcessControl::SetPid#40567
benhillis merged 1 commit into
microsoft:masterfrom
benhillis:fix/wslc-setpid-assert

Conversation

@benhillis
Copy link
Copy Markdown
Member

Summary

Defense-in-depth follow-up to #40550. Adds WI_ASSERT(Pid > 0) in DockerExecProcessControl::SetPid so any future caller that bypasses the state.Pid > 0 filter in the exec polling loop is caught immediately in Debug builds.

Background

#40550 fixed a hang in WSLCContainerImpl::Exec where Docker's transient {Running=true, Pid=0} response (seen on fast runc failures, e.g. -u root:badgid) was treated as a valid running process. The polling loop now filters state.Pid > 0 before calling SetPid, so production behavior is correct.

This PR adds a hard assertion at the sink so the contract is enforced at the lowest level. Without it, a future caller refactor that drops the Pid > 0 check would silently hang WSLCProcess::Wait forever, because Docker never emits exec_die for a process that never spawned.

Change

void DockerExecProcessControl::SetPid(int Pid)
{
    std::lock_guard lock{m_lock};

    // Pid must be a real (forked) process. Docker reports Pid=0 in the brief
    // window between StartExec returning and runc actually forking the user
    // process; treating 0 as a valid PID causes the exec wait to hang forever
    // because Docker never emits exec_die for a process that never spawned
    // (see PR #40550). Callers must filter Pid > 0 themselves.
    WI_ASSERT(Pid > 0);
    WI_ASSERT(!m_pid.has_value());

    m_pid = Pid;
}

Suggested by @OneBlue in the #40550 review.

Defense-in-depth follow-up to PR microsoft#40550. The exec polling loop in
WSLCContainerImpl::Exec now correctly filters Pid > 0 before calling
SetPid, but a future caller that bypassed that check would silently
hang the process wait (because Docker never emits exec_die for a
process that never spawned). Assert at the lowest level so any such
regression fires loudly in Debug builds.

Suggested by @OneBlue in the PR microsoft#40550 review.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 15, 2026 22:36
@benhillis benhillis requested a review from a team as a code owner May 15, 2026 22:36
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a debug-only contract check in the wslc Docker exec process-control path to ensure an exec PID is always a real, forked process ID, preventing future regressions of the previously-fixed “Pid=0 treated as running” hang scenario.

Changes:

  • Add WI_ASSERT(Pid > 0) to DockerExecProcessControl::SetPid to enforce the caller contract in Debug builds.
  • Add an explanatory comment documenting Docker’s transient Pid=0 behavior and the hang failure mode.

@benhillis benhillis enabled auto-merge (squash) May 15, 2026 23:48
@benhillis benhillis merged commit 96b0912 into microsoft:master May 16, 2026
13 checks passed
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.

3 participants