Skip to content

perf(wsl): a held WSL spawn reads the running list once, reusing only its own read - #36077

Merged
nwparker merged 5 commits into
mainfrom
nwparker/wsl-hold-probe-lease
Oct 29, 2026
Merged

nwparker merged 5 commits into
mainfrom
nwparker/wsl-hold-probe-lease

Conversation

@nwparker

@nwparker nwparker commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​114 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​113
Prod 9 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​63 $\color{#cf222e}{\Huge{\mathbf{−}}}$​11 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​52

ELI5

When Orca holds a WSL terminal it shouldn't start on its own (a passive restore or main's background start), it runs wsl --list --running to check whether the distro is running. Since #35971 that check ran twice for the same spawn: once early, at the startup target, and again in the terminal provider. Now the provider's check reuses the early answer, but only that same spawn's answer, so one held spawn runs one probe and nothing older can be trusted.

Flag-off: applies to WSL bridge workspaces, which exist with ORCA_INTERNAL_WSL_HOSTS unset too. Non-WSL, SSH and WSL-host (orcad) spawns never reach this check.

User risk: low. Same hold decisions, one fewer wsl.exe per held spawn.

  • There's no time-based reuse: an earlier version used a 5 s lease, which this replaces.
  • The provider trusts only a read taken during this spawn's own startup-target hold.
  • A spawn without one reads fresh, and a stop or wake Orca notes still forces a fresh read.

What Changed

  • The startup-target hold (spawn-startup-target.ts): when it holds a spawn with a resolved WSL distro, it takes a read anchor (wslHoldReadAnchor(), performance.now() minus 1 ms of slack) and reads the running list after that anchor. It returns the anchor in its target.
  • Carrying the anchor: both preflights (ipc/spawn-preflight.ts, runtime/spawn-preflight.ts) keep it on their spawn state. Both option builders put it on the spawn's options as wslHoldReadAnchor.
  • The provider hold: holdPassiveRestoreOfStoppedWslDistro reads with startedAfter: max(opts.wslHoldReadAnchor, now - 2 s) (WSL_HOLD_READ_MAX_AGE_MS). The provider holds are the daemon adapter and the daemon-off local provider, and they pass the spawn's own options unchanged, so they reuse exactly the read made for that spawn, and only while it's under 2 s old. A daemon respawn or a same-session history lock between the two holds therefore can't stretch it.
  • No anchor: a spawn that skipped the startup-target hold reads fresh, as before.
  • Noted stop or wake: settledWslRunningReadAnchor still raises the floor past any stop or wake Orca noted, which forces a fresh read.

Why

e9-reviewer found the double probe. fix-reviewer then showed that a 5 s lease would let a stop Orca never notices (an external wsl -t or --shutdown, or WSL's idle stop) slip past a background spawn's last check. Anchoring the reuse to the spawn's own read removes the double probe without widening that window. The provider hold stays, as the only guard for spawns that skip the preflight.

Linked Issue

N/A (maintainer change; follow-up to #35971, #35938 and #35865)

Visual Proof

N/A: no UI change.

Testing

  • I manually tested these changes locally
  • Automated tests:
    • New daemon-wsl-hold-read-anchor.test.ts, driving the real running-list cache with a counting probe:
      • one probe per held spawn, both while running and while stopped;
      • no reuse across two different spawns;
      • no anchor means a fresh probe;
      • a stop noted between the two holds means a fresh probe that holds (fix-reviewer's case);
      • the spawn's own read older than the 2 s cap means a fresh probe (fix-reviewer's respawn tail).
      • Mutation checks: ignoring the anchor fails the one-probe cases, reintroducing a 5 s lease fails the no-reuse and no-anchor cases, and removing the cap fails the cap case.
    • Updated pty-spawn-wsl-passive-restore-hold.test.ts: the provider receives exactly the anchor the startup-target read used.
  • Load rule:
    • Pre-existing failures only:
    • Touched tests, each run on its own: the new anchor test, the daemon passive-restore hold, local-pty windows-shell-launch, the pty-spawn and spawn-host-env holds, runtime spawn-preflight, and wsl-background-distro-gate.
    • Also: the pty runtime and IPC suites, check:code-quality:changed, oxlint, anti-slop, the ast-grep test and gates, and the five localization verifiers.

Review

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

  • Mixed versions: nothing changes on the wire. The anchor lives only in main's spawn options.

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 is limited to held WSL spawn checks in the main process.

The PR should preserve the provider's ability to catch an external WSL stop before merging.

Findings

  1. P1 Background spawns undo WSL shutdown ▶

Summary

Reuses the startup WSL running-list read during the provider hold.

  • Held WSL spawns reuse their own recent running-list check.

Diagram

sequenceDiagram
  participant S as Startup target
  participant C as Running-list cache
  participant P as Provider
  S->>C: Read after spawn anchor
  C-->>S: Running
  Note over S,P: Spawn preparation
  P->>C: Read after same anchor, capped at two seconds
  C-->>P: Reuse qualifying answer or probe again
  Note over C,P: Recorded stop or wake forces a newer read
Loading

Reviews (4) · Last reviewed commit: "Merge branch 'main' into nwparker/wsl-ho..." · Reviewed by Greptile

The provider-level holds (daemon adapter, daemon-off local provider) accept a running-list read started within a 5 s lease, so they reuse the startup-target hold's probe instead of running wsl --list --running again. A noted stop still forces a fresh read; a read older than the lease probes again.
@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

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: 7410aa3a-931d-4db8-a325-baed2399cd35
Commits

Reviewing files that changed from the base of the PR and between c6eb855 and 78bd8a2.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
Walkthrough

The startup target captures a WSL hold read anchor and passes it to the passive-restore hold. The hold applies a startedAfter filter bounded by the configured maximum read age when an anchor is provided. The anchor then passes through IPC and runtime spawn state into provider spawn options. Tests cover shared reads, fresh reads, and provider receipt of the anchor.

Priority: Low

Merge Risk: Low · previous review 7cb06

Closely timed WSL restores can share a running-state check and miss an external stop. This narrow risk should be fixed or accepted before merging.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 33.33% 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. Documenta… 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 clearly and concisely describes the main change: reusing the WSL running-list read for a held spawn while limiting reuse to that spawn.
Description check Passed The description covers the required sections, explains the before and after behavior, gives the rationale, documents testing and known pre-existing failures, and marks visual proof as not applicable. …
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: Description check

Explanation

The description covers the required sections, explains the before and after behavior, gives the rationale, documents testing and known pre-existing failures, and marks visual proof as not applicable. The maintainer-specific Linked Issue exception is used appropriately.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% 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 6 functions across 11 files.

Resolution

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

Finishing Touches: 💡 2
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • 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.

🔇 Additional comments: 1
src/main/daemon/daemon-wsl-passive-restore-hold.ts-30-30: 1

30-30: Major · Functional Correctness · Heavy lift

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Keep a fresh final check when an unobserved stop is possible.

If WSL stops after a confirmed running probe without emitting a stop note, this five-second cutoff lets the provider hold reuse the old running result. The running-list cache accepts that result without another probe. A passive spawn can then start a stopped distro, although a fresh provider read would have blocked it. WSL idle stops are specifically identified as stops that may go unreported. Preserve the fresh final check unless a reliable stop observation can invalidate the lease. (raw.githubusercontent.com)


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: 17e3d04f-b495-4987-9eed-589bff2d9530
Commits

Reviewing files that changed from the base of the PR and between 8a4bc55 and b9248b9.

Files selected for processing: 6
  • src/main/daemon/daemon-pty-adapter-wsl-passive-restore-hold.test.ts
  • src/main/daemon/daemon-pty-session-spawn.ts
  • src/main/daemon/daemon-wsl-hold-probe-lease.test.ts
  • src/main/daemon/daemon-wsl-passive-restore-hold.ts
  • 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; 6 remain after this review.

…running read

Replaces the 5 s lease: the startup-target hold records a read anchor on the spawn's options and reads after it; the provider hold reads after the same anchor, so it reuses that spawn's read and nothing older. No anchor means a fresh read; a noted stop or wake still forces one.
@nwparker
nwparker force-pushed the nwparker/wsl-hold-probe-lease branch from b9248b9 to 1f82c28 Compare October 29, 2026 20:49
@nwparker nwparker changed the title perf(wsl): a held WSL spawn reads the running list once perf(wsl): a held WSL spawn reads the running list once, reusing only its own read Oct 29, 2026
…der hold

A daemon respawn or a same-session history lock can sit between the two holds, so the provider hold reads after max(anchor, now - 2 s).

@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: 1


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: 8288f429-9deb-4717-b5fa-df7252523991
Commits

Reviewing files that changed from the base of the PR and between b9248b9 and 7cb0673.

Files selected for processing: 11
  • src/main/daemon/daemon-wsl-hold-read-anchor.test.ts
  • src/main/daemon/daemon-wsl-passive-restore-hold.ts
  • src/main/ipc/pty/ipc/spawn-options.ts
  • src/main/ipc/pty/ipc/spawn-preflight.ts
  • src/main/ipc/pty/ipc/spawn-state.ts
  • src/main/ipc/pty/pty-spawn-wsl-passive-restore-hold.test.ts
  • src/main/ipc/pty/runtime/spawn-options.ts
  • src/main/ipc/pty/runtime/spawn-preflight.ts
  • src/main/ipc/pty/runtime/spawn-state.ts
  • src/main/ipc/pty/spawn-startup-target.ts
  • src/main/providers/pty-provider-contract.ts

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


/** The anchor a spawn's first hold reads after; later holds of the same spawn reuse that read. */
export function wslHoldReadAnchor(): number {
return performance.now() - WSL_HOLD_READ_ANCHOR_SLACK_MS

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.

Minor · Functional Correctness · Quick win

Do not let the anchor accept another spawn’s read.

If a second held spawn starts within 1 ms of the first probe, subtracting the slack places its anchor before that probe. The running-list cache can then return the first spawn’s result instead of reading again. This breaks the per-spawn read guarantee and can miss an unnoted distro stop between the spawns. Capture an anchor that excludes earlier probes, and adjust the frozen-clock tests to verify closely spaced spawns. (raw.githubusercontent.com)

Comment on lines +44 to +47
startedAfter: Math.max(
opts.wslHoldReadAnchor,
performance.now() - WSL_HOLD_READ_MAX_AGE_MS
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Background spawns undo WSL shutdown

If an external wsl --shutdown stops the distro after the startup read, without an Orca stop note, the provider can reuse the earlier running answer for up to two seconds. It then allows the background spawn to start the distro again instead of holding it. The provider previously requested a fresh read and could catch that stop.

Restricting reuse to the same spawn does not close this gap. Keep a fresh check before the provider starts WSL, or otherwise preserve the stop decision.

@nwparker
nwparker merged commit 4f18d01 into main Oct 29, 2026
53 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.

1 participant