From 1fd5674331226a1261ae41634407b5c6e7eee7d7 Mon Sep 17 00:00:00 2001 From: Zoltan Csizmadia Date: Sat, 19 Sep 2026 09:47:16 -0500 Subject: [PATCH] fix: bound a COM call, and survive one that panics 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. --- internal/wsl/com_do_windows_test.go | 133 ++++++++++++++++++++++++++++ internal/wsl/com_windows.go | 76 +++++++++++++++- 2 files changed, 205 insertions(+), 4 deletions(-) create mode 100644 internal/wsl/com_do_windows_test.go diff --git a/internal/wsl/com_do_windows_test.go b/internal/wsl/com_do_windows_test.go new file mode 100644 index 0000000..2e159ae --- /dev/null +++ b/internal/wsl/com_do_windows_test.go @@ -0,0 +1,133 @@ +//go:build windows + +package wsl + +import ( + "context" + "errors" + "strings" + "testing" + "time" +) + +// fakeApartment runs comSession's loop without COM, so do()'s behaviour can be +// tested on any Windows machine rather than only one with a live wslservice. +// The loop is byte-for-byte the shape of the real one in newCOMSession. +func fakeApartment(t *testing.T, timeout time.Duration) *comSession { + t.Helper() + s := &comSession{ + calls: make(chan func()), + stop: make(chan struct{}), + callTimeout: timeout, + } + go func() { + for { + select { + case fn := <-s.calls: + fn() + case <-s.stop: + return + } + } + }() + t.Cleanup(s.Close) + return s +} + +// A panicking call must not take the process down, must not read as success, +// and must not kill the apartment loop (#437). +// +// All three matter separately. Without the recover, a panic on the apartment +// goroutine unwinds past the loop and kills the whole supervisor — bridge and +// reconciler included. With a recover but no error, `list` would return an +// empty slice and a nil error, so a machine with distros would report having +// none. And if the loop died, every later call would block until its context +// fired, which on the supervisor's path is never. +// +// 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. +func TestCOMCallPanicIsReportedAndTheLoopSurvives(t *testing.T) { + s := fakeApartment(t, time.Second) + + err := s.do(context.Background(), func() { panic("boom") }) + if err == nil { + t.Fatal("a panicking call reported success; `list` would return no distros and no error") + } + if !strings.Contains(err.Error(), "panic") { + t.Errorf("error does not say it panicked: %v", err) + } + + // The loop has to still be there. This is the assertion that would catch + // a recover placed at the loop level instead of per call. + ran := false + if err := s.do(context.Background(), func() { ran = true }); err != nil { + t.Fatalf("the apartment loop did not survive the panic: %v", err) + } + if !ran { + t.Error("the call after the panic never ran") + } +} + +// A call that never returns must not park the caller forever (#437). +// +// This is the whole bug: the supervisor passes its process-lifetime context +// down, so before this bound there was nothing to stop `do` waiting for the +// life of the process — while tick held the reconciler's mutex. +func TestCOMCallIsBoundedEvenWithAContextThatNeverFires(t *testing.T) { + s := fakeApartment(t, 150*time.Millisecond) + + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + + start := time.Now() + // context.Background() never fires — exactly what the supervisor passes. + err := s.do(context.Background(), func() { <-release }) + took := time.Since(start) + + if err == nil { + t.Fatal("a call that never returned reported success") + } + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("err = %v, want context.DeadlineExceeded", err) + } + if took > 5*time.Second { + t.Errorf("took %v: the bound is not being applied", took) + } +} + +// The caller's own deadline must still win when it is sooner, and must be +// distinguishable — Fast.List demotes the fast path on our timeout but passes +// the caller's cancellation straight through without demoting. +func TestCallersOwnDeadlineStillWins(t *testing.T) { + s := fakeApartment(t, 30*time.Second) + + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + start := time.Now() + if err := s.do(ctx, func() { <-release }); !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("err = %v, want context.DeadlineExceeded", err) + } + if took := time.Since(start); took > 5*time.Second { + t.Errorf("took %v; the caller's shorter deadline did not win", took) + } + if ctx.Err() == nil { + t.Error("the caller's context should be the one that fired, which is how " + + "Fast.List tells the two apart") + } +} + +// An ordinary call still works, and is not slowed by any of the above. +func TestCOMCallStillRunsNormally(t *testing.T) { + s := fakeApartment(t, time.Second) + got := 0 + if err := s.do(context.Background(), func() { got = 42 }); err != nil { + t.Fatalf("do: %v", err) + } + if got != 42 { + t.Errorf("fn did not run on the apartment thread (got %d)", got) + } +} diff --git a/internal/wsl/com_windows.go b/internal/wsl/com_windows.go index c7c814e..c87e5b4 100644 --- a/internal/wsl/com_windows.go +++ b/internal/wsl/com_windows.go @@ -8,6 +8,7 @@ import ( "runtime" "sync" "syscall" + "time" "unsafe" "golang.org/x/sys/windows" @@ -108,6 +109,11 @@ type comSession struct { stop chan struct{} once sync.Once sess unsafe.Pointer + + // callTimeout bounds one call; zero means comCallTimeout. A field rather + // than a mutable package global so a test can shorten it without other + // tests in the same binary seeing the change. + callTimeout time.Duration } // newCOMSession brings up the apartment and the object, or reports why it could @@ -157,11 +163,54 @@ func newCOMSession() (*comSession, error) { return s, nil } -// do runs fn on the apartment thread and waits for it. +// comCallTimeout bounds one call on the apartment thread (#437). +// +// Every COM caller used to inherit whatever context it was handed, 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, and the tick holds the +// reconciler's mutex: every Demand() blocked, so every docker command HUNG +// rather than failing, and the stats file froze so the tray could not even +// show that the supervisor was stuck. +// +// The COM rewrite (#380) made that 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 all queue behind one unbuffered +// channel and one OS thread, so one hung call stalls all of them. +// +// A ceiling well above any healthy call — the measurement that motivated this +// path was ~65 ms — so this only ever fires on a service that has stopped +// answering, and never on one that is merely slow. +const comCallTimeout = 20 * time.Second + +// do runs fn on the apartment thread and waits for it, bounded. func (s *comSession) do(ctx context.Context, fn func()) error { + // Derived, so the caller's own cancellation still wins when it is sooner. + // Callers distinguish the two: Fast.List checks the OUTER ctx, so a + // deadline of ours reads as "the backend is unresponsive" and falls back + // to wsl.exe, while the caller's own cancellation is passed straight + // through and does not demote the fast path. + timeout := s.callTimeout + if timeout == 0 { + timeout = comCallTimeout + } + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + done := make(chan struct{}) + var panicked any + call := func() { + // Ordered deliberately. close(done) is registered FIRST so it runs + // LAST: the recover has already stored its value by the time any + // waiter is released, which is what makes reading `panicked` after + // <-done race-free. + defer close(done) + defer func() { panicked = recover() }() + fn() + } + select { - case s.calls <- func() { defer close(done); fn() }: + case s.calls <- call: case <-s.stop: return fmt.Errorf("wsl: COM session is closed") case <-ctx.Done(): @@ -169,10 +218,29 @@ func (s *comSession) do(ctx context.Context, fn func()) error { } select { case <-done: + if panicked != nil { + // Recovering keeps the apartment loop alive — without it a panic + // here takes down the whole supervisor, bridge included. But a + // recovered panic must not read as success: `list` would return + // an empty slice and a nil error, and a machine with distros + // would report having none. unsafe.Slice(arr, count) panics if + // the service ever returns count > 0 with a nil array, and hr==0 + // is the only thing standing between us and that. + return fmt.Errorf("wsl: COM call panicked: %v", panicked) + } return nil case <-ctx.Done(): - // The call is still running on the apartment thread; abandoning the - // wait is safe because fn owns everything it touches. + // The call is still running on the apartment thread and keeps writing + // the variables fn captured. That is safe ONLY because every caller + // returns without reading them on the error path -- list returns + // `nil, err` and never touches `out`. Now that this deadline can + // actually fire, that is a live constraint rather than a theoretical + // one: a future caller that salvages a partial result after a timeout + // gets a real data race on a slice header. + // + // The abandoned call also keeps the single apartment thread busy, so + // the next do() waits behind it and times out too. Bounded and + // degraded beats unbounded. return ctx.Err() } }