Probe hosts for health instead of risking the shared connection

The health check shares each host's libvirt connection with the reconciles,
so it must not be able to take that connection away from them.

- A timed-out check no longer drops the link. The check goes through the new
  hostPool.probe, which abandons its RPCs when the context ends and leaves
  the connection alone, so an image upload or a Create running on the same
  link is not cut off. A link that really died is closed by the keepalive.
  One abandoned probe per host may stay waiting; until it returns, the next
  check reports Timeout at once (errProbeBusy).
- Waiting for a host's connection slot now ends with the caller's context.
  hostEntry used a mutex, so a check could wait out a reconcile redialing a
  dead host — minutes — past its own timeout. The slot is a one-slot channel
  and acquire bounds only the wait, never the uncontended take.
- --provider-check-timeout defaults to 0 and derives min(15s, interval).
  The flat 15s default contradicted the validation that the timeout may not
  exceed the interval, so --provider-check-interval=12s alone failed to
  start.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

internal/provider/kvm/check.go

@@ -48,10 +48,12 @@ func (p *Provider) Check(ctx context.Context) []provider.TargetStatus {
 	return out
 }
 
-// checkHost returns the RPC errors from fn untouched: call drops the
-// shared connection on anything that is not a libvirt error, and a pool
-// that exists but is stopped says nothing about the link. facts is read
-// only after call returned nil, since an abandoned fn may still write it.
+// checkHost goes through probe, so a check that runs out of time abandons
+// its RPCs rather than closing the connection the reconciles share. fn
+// returns the RPC errors untouched, since a transport error still drops the
+// link; a pool that exists but is stopped says nothing about the link, so
+// that verdict is reached afterwards. facts is read only after probe
+// returned nil, since an abandoned fn may still write it.
 func (p *Provider) checkHost(ctx context.Context, host provider.KVMHost) provider.TargetStatus {
 	ts := provider.TargetStatus{
 		Name:    host.Name,
@@ -60,7 +62,7 @@ func (p *Provider) checkHost(ctx context.Context, host provider.KVMHost) provide
 	}
 	start := time.Now()
 	var facts hostFacts
-	err := p.hosts.call(ctx, p.logger(ctx), host, func(api libvirtAPI) error {
+	err := p.hosts.probe(ctx, p.logger(ctx), host, func(api libvirtAPI) error {
 		version, err := api.ConnectGetLibVersion()
 		if err != nil {
 			return err
@@ -125,7 +127,7 @@ func checkReason(err error) string {
 		}
 	}
 	switch {
-	case errors.Is(err, context.DeadlineExceeded), errors.Is(err, context.Canceled):
+	case errors.Is(err, context.DeadlineExceeded), errors.Is(err, context.Canceled), errors.Is(err, errProbeBusy):
 		return provider.ReasonTimeout
 	case hasCode(err, libvirt.ErrNoStoragePool):
 		return reasonPoolNotFound

internal/provider/kvm/check_test.go

@@ -7,6 +7,7 @@ import (
 	"io"
 	"maps"
 	"strings"
+	"sync"
 	"sync/atomic"
 	"testing"
 	"time"
@@ -174,14 +175,18 @@ func TestCheck_hostsCheckedConcurrently(t *testing.T) {
 	}
 }
 
-func TestCheck_timeout(t *testing.T) {
+// A daemon that stopped answering times the check out, but the connection
+// the reconciles share stays up, and the next check does not stack a
+// second hung RPC on top of the first.
+func TestCheck_timeoutKeepsTheConnection(t *testing.T) {
 	t.Parallel()
 	api := newFakeAPI("default")
 	block := make(chan struct{})
 	t.Cleanup(func() { close(block) })
 	blocked := &blockingAPI{fakeAPI: api, block: block}
+	var closed atomic.Bool
 	dial := func(context.Context, provider.KVMHost) (*hostClient, error) {
-		return &hostClient{api: blocked, close: func() {}}, nil
+		return &hostClient{api: blocked, close: func() { closed.Store(true) }}, nil
 	}
 	pc := testConfig("nas1")
 	p := newWithDialer(pc.Name, *pc.KVM, dial, nil)
@@ -193,6 +198,49 @@ func TestCheck_timeout(t *testing.T) {
 	if got[0].Ready || got[0].Reason != provider.ReasonTimeout {
 		t.Errorf("Check() = %+v, want Timeout", got[0])
 	}
+	if closed.Load() {
+		t.Error("the timed-out check closed the connection the reconciles share")
+	}
+	again := p.Check(context.Background())
+	if again[0].Reason != provider.ReasonTimeout || !strings.Contains(again[0].Message, "still waiting") {
+		t.Errorf("second Check() = %+v, want an immediate Timeout while the first still waits", again[0])
+	}
+}
+
+// A reconcile redialing a dead host holds the host's slot for minutes; the
+// check must give up at its own deadline rather than wait that out.
+func TestCheck_doesNotWaitBehindAReconcileDial(t *testing.T) {
+	t.Parallel()
+	release := make(chan struct{})
+	dialing := make(chan struct{})
+	var once sync.Once
+	dial := func(context.Context, provider.KVMHost) (*hostClient, error) {
+		once.Do(func() { close(dialing) })
+		<-release
+		return nil, errHostOff
+	}
+	pc := testConfig("nas1")
+	p := newWithDialer(pc.Name, *pc.KVM, dial, nil)
+	reconcile := make(chan error, 1)
+	go func() {
+		_, err := p.Get(context.Background(), formatProviderID("nas1", testName))
+		reconcile <- err
+	}()
+	<-dialing
+	ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+	defer cancel()
+	start := time.Now()
+
+	got := p.Check(ctx)
+
+	if waited := time.Since(start); waited > time.Second {
+		t.Errorf("Check() took %s behind the reconcile's dial, want about 50ms", waited)
+	}
+	if got[0].Reason != provider.ReasonTimeout {
+		t.Errorf("Check() = %+v, want Timeout", got[0])
+	}
+	close(release)
+	<-reconcile
 }
 
 // blockingAPI hangs in ConnectGetLibVersion until block closes, like a
@@ -222,6 +270,7 @@ func TestCheckReason(t *testing.T) {
 			&dialError{stage: stageSSHHandshake, err: context.DeadlineExceeded}), reasonSSHHandshakeFailed},
 		{"deadline", context.DeadlineExceeded, provider.ReasonTimeout},
 		{"cancelled", context.Canceled, provider.ReasonTimeout},
+		{"previous check still waiting", errProbeBusy, provider.ReasonTimeout},
 		{"no pool", lvError(libvirt.ErrNoStoragePool, "no pool"), reasonPoolNotFound},
 		{"other libvirt error", lvError(libvirt.ErrInternalError, "boom"), reasonLibvirtError},
 		{"link died mid-call", io.EOF, provider.ReasonUnreachable},

internal/provider/kvm/conn.go

@@ -9,6 +9,7 @@ import (
 	"net"
 	"os"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	libvirt "github.com/digitalocean/go-libvirt"
@@ -102,9 +103,16 @@ type hostPool struct {
 	entries map[string]*hostEntry
 }
 
+// hostEntry is one host's slot in the pool. lock is a one-slot semaphore
+// rather than a mutex so that waiting for it can end with the caller's
+// context: whoever holds it may be dialing a dead host for as long as the
+// dial takes.
 type hostEntry struct {
-	mu     sync.Mutex
+	lock   chan struct{}
 	client *hostClient
+	// probing is set while a probe's fn runs, including one whose caller
+	// has already given up on it.
+	probing atomic.Bool
 }
 
 func newHostPool(dial hostDialer) *hostPool {
@@ -116,16 +124,37 @@ func (p *hostPool) entry(name string) *hostEntry {
 	defer p.mu.Unlock()
 	e, ok := p.entries[name]
 	if !ok {
-		e = &hostEntry{}
+		e = &hostEntry{lock: make(chan struct{}, 1)}
 		p.entries[name] = e
 	}
 	return e
 }
 
+// acquire takes e.lock. Only waiting is bounded by ctx: a free slot is
+// taken even when ctx has already ended, so an uncontended caller behaves
+// as it did with a mutex, and a contended one gives up with ctx's error.
+func (e *hostEntry) acquire(ctx context.Context) error {
+	select {
+	case e.lock <- struct{}{}:
+		return nil
+	default:
+	}
+	select {
+	case e.lock <- struct{}{}:
+		return nil
+	case <-ctx.Done():
+		return ctx.Err()
+	}
+}
+
+func (e *hostEntry) release() { <-e.lock }
+
 func (p *hostPool) get(ctx context.Context, log logr.Logger, host provider.KVMHost) (*hostClient, error) {
 	e := p.entry(host.Name)
-	e.mu.Lock()
-	defer e.mu.Unlock()
+	if err := e.acquire(ctx); err != nil {
+		return nil, fmt.Errorf("waiting for the connection to host %s (%s): %w", host.Name, host.Address, err)
+	}
+	defer e.release()
 	if e.client != nil {
 		if e.client.alive() {
 			return e.client, nil
@@ -149,8 +178,8 @@ func (p *hostPool) get(ctx context.Context, log logr.Logger, host provider.KVMHo
 // connection that was already replaced by a redial is left alone.
 func (p *hostPool) drop(host string, c *hostClient) {
 	e := p.entry(host)
-	e.mu.Lock()
-	defer e.mu.Unlock()
+	e.lock <- struct{}{}
+	defer e.release()
 	if e.client == c {
 		e.client = nil
 	}
@@ -173,19 +202,61 @@ func (p *hostPool) call(ctx context.Context, log logr.Logger, host provider.KVMH
 	if err != nil {
 		return err
 	}
-	api := c.api
-	if log.V(1).Enabled() {
-		api = &tracingAPI{inner: c.api, log: log}
-	}
-	err = awaitOrCancel(ctx, func() error { return fn(api) }, func() {
+	err = awaitOrCancel(ctx, func() error { return fn(traced(c, log)) }, func() {
 		log.V(1).Info("libvirt connection dropped", "reason", "context cancelled", "error", ctx.Err().Error())
 		p.drop(host.Name, c)
 	})
+	p.dropOnTransportError(ctx, log, host, c, err)
+	return err
+}
+
+// errProbeBusy refuses a probe while the previous one on the same host has
+// not come back yet.
+var errProbeBusy = errors.New("the previous check of this host is still waiting for libvirt to answer")
+
+// probe is call for the health check, which must never cost the reconciles
+// their connection: when ctx ends first, fn is abandoned and the connection
+// left alone. A link that is really dead is closed by the keepalive, which
+// ends fn too. At most one probe per host runs at a time, an abandoned one
+// included, so a daemon that stopped answering cannot pile up goroutines;
+// until it answers, further probes get errProbeBusy.
+func (p *hostPool) probe(ctx context.Context, log logr.Logger, host provider.KVMHost, fn func(api libvirtAPI) error) error {
+	log = log.WithName("libvirt")
+	e := p.entry(host.Name)
+	if !e.probing.CompareAndSwap(false, true) {
+		return errProbeBusy
+	}
+	c, err := p.get(ctx, log, host)
+	if err != nil {
+		e.probing.Store(false)
+		return err
+	}
+	err = awaitOrCancel(ctx, func() error {
+		defer e.probing.Store(false)
+		return fn(traced(c, log))
+	}, func() {
+		log.V(1).Info("libvirt probe abandoned, connection kept", "error", ctx.Err().Error())
+	})
+	p.dropOnTransportError(ctx, log, host, c, err)
+	return err
+}
+
+// traced returns c's API, logging every RPC when log has V(1) enabled.
+func traced(c *hostClient, log logr.Logger) libvirtAPI {
+	if log.V(1).Enabled() {
+		return &tracingAPI{inner: c.api, log: log}
+	}
+	return c.api
+}
+
+// dropOnTransportError drops c when err is evidence against the link:
+// neither a libvirt error, nor a verdict already classified, nor the
+// caller's own context ending.
+func (p *hostPool) dropOnTransportError(ctx context.Context, log logr.Logger, host provider.KVMHost, c *hostClient, err error) {
 	if err != nil && !isLibvirtError(err) && !isClassified(err) && !errors.Is(err, ctx.Err()) {
 		log.V(1).Info("libvirt connection dropped", "reason", "transport error", "error", err.Error())
 		p.drop(host.Name, c)
 	}
-	return err
 }
 
 // awaitOrCancel runs fn in the background and returns its result, unless ctx

internal/provider/kvm/conn_test.go

@@ -232,6 +232,97 @@ func TestHostPool_call_hostsAreIndependent(t *testing.T) {
 	}
 }
 
+// A dial to a dead host holds the host's slot for as long as it takes; a
+// caller with a deadline must not wait past it.
+func TestHostPool_get_waitingRespectsContext(t *testing.T) {
+	t.Parallel()
+	release := make(chan struct{})
+	dialing := make(chan struct{})
+	var once sync.Once
+	dial := func(context.Context, provider.KVMHost) (*hostClient, error) {
+		once.Do(func() { close(dialing) })
+		<-release
+		return nil, errors.New("ssh dial: connection timed out")
+	}
+	p := newHostPool(dial)
+	slow := make(chan error, 1)
+	go func() { slow <- p.call(context.Background(), discard, testHost, noop) }()
+	<-dialing
+	ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
+	defer cancel()
+	start := time.Now()
+
+	err := p.call(ctx, discard, testHost, noop)
+
+	if !errors.Is(err, context.DeadlineExceeded) {
+		t.Errorf("call() error = %v, want the caller's deadline", err)
+	}
+	if waited := time.Since(start); waited > time.Second {
+		t.Errorf("call() waited %s behind another caller's dial, want about 50ms", waited)
+	}
+	close(release)
+	if err := <-slow; err == nil {
+		t.Error("the blocked dial succeeded, want its error")
+	}
+}
+
+func TestHostPool_probe_abandonsWithoutClosing(t *testing.T) {
+	t.Parallel()
+	d := &fakeDialer{}
+	p := newHostPool(d.dial)
+	unblock := make(chan struct{})
+	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond)
+	defer cancel()
+
+	err := p.probe(ctx, discard, testHost, func(libvirtAPI) error {
+		<-unblock // a daemon that stopped answering
+		return nil
+	})
+
+	if !errors.Is(err, context.DeadlineExceeded) {
+		t.Fatalf("probe() error = %v, want DeadlineExceeded", err)
+	}
+	if d.clients[0].closed.Load() {
+		t.Error("probe closed the shared connection when it gave up")
+	}
+	if err := p.probe(context.Background(), discard, testHost, noop); !errors.Is(err, errProbeBusy) {
+		t.Errorf("probe() while the abandoned one still waits = %v, want errProbeBusy", err)
+	}
+	if err := p.call(context.Background(), discard, testHost, noop); err != nil {
+		t.Errorf("call() on the kept connection = %v", err)
+	}
+	close(unblock)
+	deadline := time.Now().Add(time.Second)
+	for {
+		err := p.probe(context.Background(), discard, testHost, noop)
+		if err == nil {
+			break
+		}
+		if !errors.Is(err, errProbeBusy) || time.Now().After(deadline) {
+			t.Fatalf("probe() after the abandoned one returned = %v, want nil", err)
+		}
+		time.Sleep(time.Millisecond)
+	}
+	if d.dialCount() != 1 {
+		t.Errorf("dials = %d, want 1: nothing should have redialed", d.dialCount())
+	}
+}
+
+func TestHostPool_probe_dropsConnectionOnTransportError(t *testing.T) {
+	t.Parallel()
+	d := &fakeDialer{}
+	p := newHostPool(d.dial)
+
+	err := p.probe(context.Background(), discard, testHost, func(libvirtAPI) error { return io.ErrUnexpectedEOF })
+
+	if !errors.Is(err, io.ErrUnexpectedEOF) {
+		t.Fatalf("probe() error = %v, want io.ErrUnexpectedEOF passed through", err)
+	}
+	if !d.clients[0].closed.Load() {
+		t.Error("a transport error during a probe left the connection cached")
+	}
+}
+
 // --- SSH dial against an in-process server ---
 
 func newKeyPair(t *testing.T) (ssh.Signer, []byte) {

internal/providerhealth/monitor.go

@@ -73,7 +73,8 @@ type Monitor struct {
 	// round runs at start: nothing races it, and early visibility is the
 	// point.
 	Interval time.Duration
-	// Timeout bounds each round (default 15s).
+	// Timeout bounds each round; 0 means 15s, or the interval when that is
+	// shorter.
 	Timeout time.Duration
 
 	now func() time.Time
@@ -92,31 +93,36 @@ func (m *Monitor) NeedLeaderElection() bool { return true }
 
 // ValidateSchedule checks --provider-check-interval and
 // --provider-check-timeout. The interval is 0 (off) or at least 10s, which
-// is what keeps one status write per provider per round cheap; the timeout
-// is positive and no longer than the interval.
+// is what keeps one status write per provider per round cheap. The timeout
+// is 0 (derived from the interval, see Monitor.Timeout) or positive and no
+// longer than the interval.
 func ValidateSchedule(interval, timeout time.Duration) error {
 	switch {
 	case interval == 0:
 		return nil
 	case interval < minInterval:
 		return fmt.Errorf("--provider-check-interval must be 0 (off) or at least %s, got %s", minInterval, interval)
-	case timeout <= 0 || timeout > interval:
-		return fmt.Errorf("--provider-check-timeout must be positive and at most the interval (%s), got %s", interval, timeout)
+	case timeout < 0 || timeout > interval:
+		return fmt.Errorf("--provider-check-timeout must be at most the interval (%s), got %s", interval, timeout)
 	}
 	return nil
 }
 
+func (m *Monitor) setDefaults() {
+	if m.Timeout == 0 {
+		m.Timeout = min(defaultTimeout, m.Interval)
+	}
+	if m.now == nil {
+		m.now = time.Now
+	}
+}
+
 // Start deletes the objects of providers that are no longer configured,
 // then checks every Interval until ctx ends. The providers config is read
 // once per process, so cleaning up once is enough. With checking off,
 // every object goes, so none is left claiming a stale Ready.
 func (m *Monitor) Start(ctx context.Context) error {
-	if m.Timeout == 0 {
-		m.Timeout = defaultTimeout
-	}
-	if m.now == nil {
-		m.now = time.Now
-	}
+	m.setDefaults()
 	log := logf.FromContext(ctx).WithName("provider-health")
 	ctx = logf.IntoContext(ctx, log)
 

internal/providerhealth/monitor_test.go

@@ -340,7 +340,8 @@ func TestValidateSchedule(t *testing.T) {
 		{"shortest interval", 10 * time.Second, 10 * time.Second, false},
 		{"too frequent", 5 * time.Second, time.Second, true},
 		{"negative interval", -time.Minute, time.Second, true},
-		{"no timeout", time.Minute, 0, true},
+		{"timeout left to derive, short interval", 12 * time.Second, 0, false},
+		{"negative timeout", time.Minute, -time.Second, true},
 		{"timeout longer than interval", time.Minute, 2 * time.Minute, true},
 	}
 	for _, tc := range tests {
@@ -352,3 +353,25 @@ func TestValidateSchedule(t *testing.T) {
 		})
 	}
 }
+
+func TestMonitor_setDefaults_timeout(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name                    string
+		interval, timeout, want time.Duration
+	}{
+		{"default", time.Minute, 0, 15 * time.Second},
+		{"interval shorter than the default", 12 * time.Second, 0, 12 * time.Second},
+		{"explicit", time.Minute, 20 * time.Second, 20 * time.Second},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			m := &Monitor{Interval: tc.interval, Timeout: tc.timeout}
+			m.setDefaults()
+			if m.Timeout != tc.want {
+				t.Errorf("Timeout = %s, want %s", m.Timeout, tc.want)
+			}
+		})
+	}
+}