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

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)
+	}
+}

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,22 +163,84 @@ 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():
 		return ctx.Err()
 	}
 	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()
 	}
 }