Skip to content

fix(wsl): hold a daemon-off restore of a stopped WSL distro - #35938

Merged
nwparker merged 1 commit into
mainfrom
nwparker/wsl-daemon-off-restore-hold
Oct 29, 2026
Merged

nwparker merged 1 commit into
mainfrom
nwparker/wsl-daemon-off-restore-hold

Conversation

@nwparker

@nwparker nwparker commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​43 $\color{#cf222e}{\Huge{\mathbf{−}}}$​2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​41
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4

ELI5

When Orca can't start its background terminal service, it falls back to running terminals itself. On that fallback path, reopening Orca would restart a stopped WSL distro just to restore an old WSL terminal pane, even though the user never asked for it. This PR makes the fallback path wait for the user, the same way the normal path already does.

What Changed

  • Before: If the terminal daemon failed to start (main-process-pty-startup.ts, "STARTUP FAILED — falling back to local PTYs"), or missed the 60-second startup deadline (LOCAL_PTY_STARTUP_FAIL_OPEN_TIMEOUT_MS in first-window-startup-services.ts), restored WSL panes went through the local terminal provider. That provider had no stopped-distro check, so it ran wsl.exe and booted the distro.
  • After: The local provider runs the same check the daemon does, holdPassiveRestoreOfStoppedWslDistro, once the launch plan knows the distro and before anything is spawned. A passive restore or a background start of a stopped distro is held, and the pane shows the existing stopped card with a Start button. Fresh terminals the user opens, and restores of a running distro, behave as before.

Why

This reuses the daemon's hold as it is, so both providers apply one rule and the renderer handling stays unchanged. The alternative was to stop restoring WSL panes at all when the daemon is off, but that would also drop panes for distros that are running.

Linked Issue

Internal WSL rewrite, E9 follow-up flagged in review of #35858.

Visual Proof

N/A. The stopped card is unchanged; this only routes the fallback path into it.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

local-pty-provider-windows-shell-launch.test.ts has two new cases:

  • A passive restore of a stopped distro is rejected with the hold message, and wsl.exe is never spawned. This case fails without the fix.
  • The same restore spawns wsl.exe once the distro reads as running.

typecheck:ci, oxlint and check:code-quality:changed pass.

Platforms: macOS, unit tests with Windows mocked. I haven't run it on a Windows machine.

Review

  • Flag-off: no change for users with the flag off. The hold only fires for a restore or background start on a local WSL distro when the daemon is down. On a healthy daemon this code path doesn't run.
  • User-risk: low. In the daemon-off fallback, a restored WSL pane for a stopped distro now shows the Start card instead of booting the distro. Probes: one settled running-state read per held restore, through the existing shared cache. That adds no recurring probes, so 0 per minute at steady state.

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • SSH: no change. SSH panes use the remote provider, and the hold needs a local WSL distro.
  • Folder workspaces: no change. The hold depends on the distro, not on git.
  • Other platforms: macOS and Linux never produce a WSL distro, so the hold returns early.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

@greptile-apps

greptile-apps Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Low impact] The change reaches WSL pane restores routed through the local fallback.

The PR appears safe to merge, with a non-blocking suggestion to strengthen the stopped-distro test.

Findings

  1. P2 Test misses early WSL probes ▶

Summary

The local terminal provider now checks the shared WSL hold before checking the folder or starting a terminal.

  • Restored WSL panes wait for a stopped distro when the terminal daemon is unavailable.

Reviews (1) · Last reviewed commit: "fix(wsl): hold a daemon-off restore of a..." · Reviewed by Greptile

expect(readWslDistroRunningStateMock).toHaveBeenCalledWith('Ubuntu', {
retryUnverifiable: true
})
expect(spawnMock).not.toHaveBeenCalled()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Test misses early WSL probes

The stopped-distro test only checks spawnMock, but validateWorkingDirectory can call wslUncDirectoryExistsAsync before the terminal starts. Moving the hold below that check would still pass this test, even though the folder probe could start the stopped distro. Also assert that wslUncDirectoryExistsAsyncMock was not called.

Suggested change
expect(spawnMock).not.toHaveBeenCalled()
expect(spawnMock).not.toHaveBeenCalled()
expect(wslUncDirectoryExistsAsyncMock).not.toHaveBeenCalled()

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

spawnLocalPty now awaits the passive-restore hold for the planned WSL distro before working-directory validation. It checks for cancellation after the hold. Tests verify that a stopped distro rejects without spawning a process and that a running distro proceeds to spawn wsl.exe.

Priority: Normal

Merge Risk: Moderate · up to 103da

This change prevents passive restores from starting a stopped WSL distro. It does not apply when the target distro cannot be resolved, where the default distro can still be started without the hold. Shutdown can also be delayed by up to roughly ten seconds while the distro state is probed. Close the unknown-distro gap before merging, and make the hold respect cancellation.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title names the main change: a daemon-off restore of a stopped WSL distro is now held. It follows the conventional-commit form and matches the objectives and the code summary, which adds the hold …
Description check Passed The description follows the template. It has ELI5, What Changed, Why, Linked Issue, Visual Proof, Testing, Review, Agent skill upstream boundary, Notes, and Checklist sections, and the content in each…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: a daemon-off restore of a stopped WSL distro is now held. It follows the conventional-commit form and matches the objectives and the code summary, which adds the hold to the local PTY spawn path. It is specific enough for a reviewer scanning history.

Full details: Description check

Explanation

The description follows the template. It has ELI5, What Changed, Why, Linked Issue, Visual Proof, Testing, Review, Agent skill upstream boundary, Notes, and Checklist sections, and the content in each is filled in. The Linked Issue section is weak: it names an internal rewrite and a follow-up to #35858, but it does not use a 'Fixes #' issue link. The AI Disclosure section is omitted, which is acceptable for an internal contributor. The final checklist item for pnpm lint, typecheck, test, and build is unchecked, and the Testing section says manual testing was not done and Windows was not run.

Resolution

Optional: replace the Linked Issue text with a real issue number if one exists, since outside contributors need an actual issue link. Confirm the unchecked pnpm lint, typecheck, test, and build item before merge, or note that CI will cover it.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 2


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 33149bce-7f71-4aa3-b353-d541aa4115d6
Commits

Reviewing files that changed from the base of the PR and between 8cc6878 and 103da38.

Files selected for processing: 2
  • src/main/providers/local-pty-provider-windows-shell-launch.test.ts
  • src/main/providers/local-pty-spawn.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

}
throwIfCanceled()
// Daemon-off (the daemon failed to start) a restore spawns here: same E9 hold as the daemon's.
await holdPassiveRestoreOfStoppedWslDistro(args, plan.launchWslDistro ?? undefined)

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.

Major · Functional Correctness · Heavy lift

Hold WSL launches when the target distro is unknown.

If default-distro discovery fails and no distro is cached, plan.launchWslDistro is null. The hold then returns without checking state, but the planned wsl.exe launch selects the default distro and can start it. Resolve the target before allowing a passive launch, or fail closed when a WSL target cannot be resolved. (raw.githubusercontent.com)


Minor · Stability & Availability · Quick win

Make the hold responsive to cancellation.

If shutdown cancels a spawn during the distro read, the spawn remains pending until the read and its retry finish. If the hold rejects, it also returns the hold error instead of the cancellation error. Race the shared read against cancellation and preserve cancellation on rejection. (raw.githubusercontent.com)

Based on learnings: check canceled state when an in-flight asynchronous start settles.

Source: Learnings

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.

1 participant