Skip to content

fix: bound a COM call, and survive one that panics - #440

Merged
zcsizmadia merged 1 commit into
mainfrom
fix/com-call-bounded
Sep 19, 2026
Merged

zcsizmadia merged 1 commit into
mainfrom
fix/com-call-bounded

Conversation

@zcsizmadia

Copy link
Copy Markdown
Collaborator

Partially addresses #437 — deliberately not all of it, see below.

comSession.do inherited whatever context its caller handed down, and the supervisor hands down its process-lifetime context, which never fires. A wslservice that stopped answering parked the health tick forever — and tick holds the reconciler's mutex, so that is not one stalled call:

  • every Demand() blocks, so every docker command hangs rather than failing
  • the stats file freezes, so the tray cannot even show that the supervisor is stuck
  • the poke loop never runs again

The COM rewrite (#380) made this worse, not better. Callers used to be separate processes shelling out to wsl.exe and could not affect one another; now List, Terminate and doctor all queue behind one unbuffered channel and one OS thread.

The bound

do() derives a bounded context. 20 s is far above any healthy call — the measurement that motivated this whole path was ~65 ms — so it fires only on a service that has stopped answering, never on a slow one.

Deriving (rather than replacing) keeps the caller's own cancellation winning when it is sooner, which matters because Fast.List tells the two apart: our deadline demotes the fast path and falls back to wsl.exe; the caller's own cancellation passes through without demoting.

The panic

Three separate things needed handling, and getting any one wrong is its own bug:

  • without a recover, a panic on the apartment goroutine unwinds past the loop and kills the whole supervisor, bridge included
  • a recover that reported success would be worse than the panic: list would return an empty slice and a nil error, so a machine with distros reports having none
  • the recover is per call, not at the loop level, so the loop survives and later calls still work

Real trigger: unsafe.Slice(arr, count) panics if the service returns count > 0 with a nil array, and hr == 0 is the only guard.

What is NOT fixed, and why

tick still holds s.mu across the probe. Two reasons that is not in this change:

  1. A tick-level deadline would be actively dangerous. Engine.Running returns a bool, so a timeout reads as "the engine is down" and tick would try to start an engine that is probably already running. That needs an "unknown" third answer first.
  2. Moving the probe out of the mutex changes the reconciler's invariantsDemand could run between the probe and the decision, so tick would act on a stale reading. That deserves its own change with its own reasoning, not a rider on this one.

The COM bound is the part that is safe in isolation, because a failed list degrades to the wsl.exe path rather than to a wrong answer.

Verification

Both fixes have a negative control, run both ways:

control result
bound removed test times out after 45 s
panic reported as success a panicking call reported success

Tests drive do() against a fake apartment loop of the same shape as the real one, so they run on any Windows machine rather than only one with a live wslservice. The timeout is a field with zero-means-default rather than a mutable package global, so shortening it in one test cannot leak into another in the same binary.

Partially addresses #437.

comSession.do inherited whatever context its caller handed down, and
the supervisor hands down its PROCESS-LIFETIME context, which never
fires. So a wslservice that stopped answering -- a service restart,
`wsl --update`, a wedged VM -- parked the health tick forever. tick
holds the reconciler's mutex, so that is not one stalled call: every
Demand() blocks, meaning every docker command HANGS rather than
failing, and the stats file freezes so the tray cannot even show that
the supervisor is stuck.

The COM rewrite (#380) made this worse rather than better. Callers used
to be separate processes shelling out to wsl.exe and could not affect
each other; now List, Terminate and doctor queue behind one unbuffered
channel and one OS thread, so one hung call stalls all of them.

do() now derives a bounded context. 20s is far above any healthy call
-- the measurement that motivated this path was ~65ms -- so it fires
only on a service that has stopped answering, never on a slow one. The
derivation keeps the caller's own cancellation winning when it is
sooner, which matters because Fast.List tells the two apart: our
deadline demotes the fast path and falls back to wsl.exe, the caller's
own cancellation passes through untouched.

A panicking call is now recovered and reported. Three separate things
needed it:

  - without a recover, a panic on the apartment goroutine unwinds past
    the loop and kills the whole supervisor, bridge included
  - a recover that reported SUCCESS would be worse than the panic:
    list would return an empty slice and a nil error, so a machine
    with distros would report having none
  - the recover is per call, not at the loop level, so the loop
    survives and later calls still work

There is a real trigger: unsafe.Slice(arr, count) panics if the service
returns count > 0 with a nil array, and hr == 0 is the only guard.

What is NOT fixed, and why
--------------------------
tick still holds s.mu across the probe. Two reasons this is not in the
same change:

  - A tick-level deadline would be actively dangerous. Engine.Running
    returns a bool, so a timeout reads as "the engine is down" and tick
    would try to START an engine that is probably running. That needs
    an "unknown" third answer first.
  - Moving the probe out of the mutex changes the reconciler's
    invariants -- Demand could run between the probe and the decision,
    so tick would act on a stale reading. That deserves its own change
    with its own reasoning, not a rider on this one.

The COM bound is the part that is safe in isolation, because a failed
list degrades to the wsl.exe path rather than to a wrong answer.

Verification
------------
Both fixes have a negative control, run both ways:

  bound removed            -> the test times out after 45s
  panic reported as success -> "a panicking call reported success"

Tests drive do() against a fake apartment loop of the same shape as the
real one, so they run on any Windows machine rather than only one with
a live wslservice. The timeout is a field with a zero-means-default
rather than a mutable package global, so shortening it in one test
cannot leak into another in the same binary.
@zcsizmadia
zcsizmadia merged commit 936ee9c into main Sep 19, 2026
5 checks passed
@zcsizmadia
zcsizmadia deleted the fix/com-call-bounded branch September 19, 2026 14:52
zcsizmadia added a commit that referenced this pull request Sep 19, 2026
…#441)

Closes #437. #440 bounded the COM call; this is the rest.

Engine.Running returns (bool, error)
------------------------------------
Both implementations used to collapse a failed probe into false, and
the reconciler's response to false is Engine.Start. So a wedged
wslservice, a WSL update mid-flight or any transient probe failure read
as "the engine is down" and provoked a start of an engine that was
almost certainly running.

That is also why the bound could not simply be added to tick: bounding
a two-valued Running converts "slow" into "down", which is the same bug
with a timer attached. The third answer had to come first.

(false, nil) now means "definitely not running". Anything the probe
could not determine is an error, and tick does nothing at all that
round rather than guess -- lastUp keeps the last reading worth
trusting, and the next tick asks again.

Provisioner keeps both shapes. EngineRunning stays for the waitFor
polls, where "cannot tell" and "not yet" both correctly mean keep
waiting; EngineRunningErr is for the caller whose next move is to start
something. Same probe, the error no longer discarded on the path that
needs it.

The probe runs outside s.mu
---------------------------
tick held the mutex across Engine.Running with the supervisor's
process-lifetime context, so a service that stopped answering parked
the reconciler INSIDE the lock: every Demand() blocked, meaning every
docker connection hung rather than failing; LifecycleSnapshot froze, so
the tray could not even show the supervisor was stuck; and the poke
loop never ran again.

Now: probe outside, decide inside, with a 60s ceiling on the probe
itself. Generous on purpose -- a cold `wsl.exe --list` on a loaded
machine is not fast, and this is a ceiling for a probe that has stopped
answering, not a latency target.

Moving it out opens a window where a Demand can cold-start the engine
between the probe and the decision, leaving tick holding a reading that
no longer describes the machine. startGen closes it: tick captures the
counter before probing and gives up the round if it moved. Start is
idempotent so the stale case was survivable anyway, but "engine is
down; starting it" logged about an engine somebody just started is how
an operator stops believing the log.

Verification
------------
Three negative controls, each run both ways:

  probe error ignored     -> Start called 41 times in 400ms against a
                             RUNNING engine
  probe back inside mu    -> "Demand blocked behind a probe in flight"
  (from #440) bound gone  -> the COM test times out

Plus TestDefiniteDownStillStartsTheEngine, so the fix cannot have
bought safety by quietly disabling the supervisor.
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