fix(vpcd): RFC 2131 §4.1 DORA exponential backoff (mulga-siv-39)

DHCPManager.Acquire previously did one DISCOVER and gave up after 15s because the outer ctx.WithTimeout matched the per-attempt timeout — nclient4's WithRetry(3) could never fire. Real DHCP servers (consumer routers, ISC dhcpd, dnsmasq) silently drop OFFERs under load; the spec mandates retransmission with exponential backoff, and a single lost DISCOVER was killing legitimate boots. Move retransmission into DHCPManager.acquireWithBackoff with a 4s/8s/16s/32s schedule (±1s jitter, ~60s worst case). NClient4Client becomes single-attempt; manager owns retry policy. Default acquireTimeout lifts from 15s to 90s (sum of schedule + slack).
2026-10-19 10:52:04 +00:00 · 2026-04-30 23:01:05 +10:00
parent 714898022e
commit efe054e4e4
4 changed files with 237 additions and 20 deletions
@@ -13,22 +13,27 @@ import (
 // github.com/insomniacslk/dhcp/dhcpv4/nclient4. Each Acquire/Renew/Release
 // opens an AF_PACKET socket on the target bridge for the duration of the
 // handshake and closes it when done — no long-lived per-lease process.
+//
+// Retransmission lives one layer up in DHCPManager.acquireWithBackoff
+// (RFC 2131 §4.1). This client always does exactly one DISCOVER attempt;
+// the per-attempt window is whatever the caller's context allows.
 type NClient4Client struct {
 	timeout time.Duration
-	retry   int
 }

 var _ Client = (*NClient4Client)(nil)

-// NewNClient4 creates an NClient4Client with sensible DORA defaults.
-func NewNClient4(timeout time.Duration, retry int) *NClient4Client {
+// NewNClient4 creates an NClient4Client. timeout is the nclient4-internal
+// socket read deadline used as a safety net when the caller supplies no
+// context deadline; in production DHCPManager always wraps each call in a
+// per-attempt ctx so this is rarely hit. The retry argument is accepted
+// for backwards compat and ignored — DHCPManager.acquireWithBackoff owns
+// retransmission.
+func NewNClient4(timeout time.Duration, _ int) *NClient4Client {
 	if timeout <= 0 {
-		timeout = 15 * time.Second
+		timeout = 60 * time.Second
 	}
-	if retry <= 0 {
-		retry = 3
-	}
-	return &NClient4Client{timeout: timeout, retry: retry}
+	return &NClient4Client{timeout: timeout}
 }

 func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Lease, error) {
@@ -42,7 +47,9 @@ func (c *NClient4Client) Acquire(ctx context.Context, req AcquireRequest) (*Leas
 	client, err := nclient4.New(req.Bridge,
 		nclient4.WithHWAddr(req.HWAddr),
 		nclient4.WithTimeout(c.timeout),
-		nclient4.WithRetry(c.retry),
+		// Single attempt per call — DHCPManager.acquireWithBackoff drives
+		// retransmission per RFC 2131 §4.1.
+		nclient4.WithRetry(1),
 	)
 	if err != nil {
 		return nil, fmt.Errorf("open nclient4 on %s: %w", req.Bridge, err)
@@ -68,7 +75,7 @@ func (c *NClient4Client) Renew(ctx context.Context, lease *Lease) (*Lease, error
 	client, err := nclient4.New(lease.Bridge,
 		nclient4.WithHWAddr(lease.HWAddr),
 		nclient4.WithTimeout(c.timeout),
-		nclient4.WithRetry(c.retry),
+		nclient4.WithRetry(1),
 	)
 	if err != nil {
 		return nil, fmt.Errorf("open nclient4 on %s for renew: %w", lease.Bridge, err)
@@ -32,10 +32,17 @@ type DHCPManager struct {
 	// HWAddr unset. Defaults to generateMAC; tests override.
 	macForClientID func(clientID string) net.HardwareAddr

-	// acquireTimeout bounds a single DORA (Acquire) / Renew / Release
-	// handshake. Independent of the RPC reply timeout on the handler side.
+	// acquireTimeout is the wallclock budget for a full Acquire — sum of
+	// every retransmit attempt in acquireBackoff plus slack. Renew/Release
+	// reuse this as their single-attempt deadline.
 	acquireTimeout time.Duration

+	// acquireBackoff drives the per-DISCOVER timeouts used by
+	// acquireWithBackoff (RFC 2131 §4.1 retransmission). Each entry is
+	// the wait-for-OFFER window for one attempt; the next entry doubles.
+	// Default: 4s, 8s, 16s, 32s. ±1s jitter applied per attempt.
+	acquireBackoff []time.Duration
+
 	// jitterFraction is the ± range applied to renewal sleeps, expressed
 	// as a fraction of T1 (0.1 = ±10%).
 	jitterFraction float64
@@ -69,11 +76,19 @@ func WithDHCPMACFunc(fn func(clientID string) net.HardwareAddr) DHCPManagerOptio
 	return func(m *DHCPManager) { m.macForClientID = fn }
 }

-// WithDHCPAcquireTimeout overrides the per-handshake DORA timeout.
+// WithDHCPAcquireTimeout overrides the wallclock budget for a full Acquire
+// (sum of every retransmit attempt + slack). Renew/Release reuse it.
 func WithDHCPAcquireTimeout(d time.Duration) DHCPManagerOption {
 	return func(m *DHCPManager) { m.acquireTimeout = d }
 }

+// WithDHCPAcquireBackoff overrides the per-DISCOVER timeout schedule used
+// by acquireWithBackoff. Tests use a tight schedule; production keeps the
+// RFC 2131 §4.1 default (4s, 8s, 16s, 32s with ±1s jitter).
+func WithDHCPAcquireBackoff(b []time.Duration) DHCPManagerOption {
+	return func(m *DHCPManager) { m.acquireBackoff = b }
+}
+
 // WithDHCPJitterFraction overrides the ±fraction applied to renewal sleeps.
 func WithDHCPJitterFraction(f float64) DHCPManagerOption {
 	return func(m *DHCPManager) { m.jitterFraction = f }
@@ -108,10 +123,18 @@ func NewDHCPManager(nc *nats.Conn, js nats.JetStreamContext, client dhcp.Client,

 	ctx, cancel := context.WithCancel(context.Background())
 	m := &DHCPManager{
-		client:         client,
-		kv:             kv,
-		nc:             nc,
-		acquireTimeout: 15 * time.Second,
+		client: client,
+		kv:     kv,
+		nc:     nc,
+		// 90s = sum of default acquireBackoff (4+8+16+32 = 60s) + slack for
+		// socket open/close on each attempt and the final ACK round-trip.
+		acquireTimeout: 90 * time.Second,
+		acquireBackoff: []time.Duration{
+			4 * time.Second,
+			8 * time.Second,
+			16 * time.Second,
+			32 * time.Second,
+		},
 		jitterFraction: 0.1,
 		rng:            rand.New(rand.NewPCG(uint64(time.Now().UnixNano()), 0xdeadbeef)), //nolint:gosec // non-cryptographic jitter
 		leases:         map[string]*managedLease{},
@@ -245,7 +268,7 @@ func (m *DHCPManager) handleAcquire(msg *nats.Msg) {
 	defer m.endAcquire(req.Bridge)
 	start := time.Now()

-	lease, err := m.client.Acquire(ctx, dhcp.AcquireRequest{
+	lease, err := m.acquireWithBackoff(ctx, dhcp.AcquireRequest{
 		Bridge:      req.Bridge,
 		ClientID:    req.ClientID,
 		Hostname:    req.Hostname,
@@ -392,6 +415,73 @@ func (m *DHCPManager) renewLoop(ctx context.Context, tracker *managedLease) {
 	}
 }

+// acquireWithBackoff drives RFC 2131 §4.1 DISCOVER retransmission. Each
+// entry in acquireBackoff sets one attempt's per-OFFER wait window with
+// ±1s jitter; on timeout the loop tries the next entry. Returns on first
+// success, on parent ctx cancellation, or after the schedule is exhausted.
+//
+// Real DHCP servers (consumer routers, ISC dhcpd, dnsmasq) silently drop
+// OFFERs under load — without retransmission a single lost packet kills
+// the whole DORA. mulga-siv-39.
+func (m *DHCPManager) acquireWithBackoff(parent context.Context, req dhcp.AcquireRequest) (*dhcp.Lease, error) {
+	schedule := m.acquireBackoff
+	if len(schedule) == 0 {
+		// Defensive: caller passed an empty schedule via WithDHCPAcquireBackoff.
+		// Fall through to a single attempt bounded by parent ctx.
+		schedule = []time.Duration{0}
+	}
+
+	var lastErr error
+	for i, base := range schedule {
+		if err := parent.Err(); err != nil {
+			if lastErr != nil {
+				return nil, fmt.Errorf("dhcp acquire on %s (client=%s): %w (last attempt err: %v)", req.Bridge, req.ClientID, err, lastErr)
+			}
+			return nil, fmt.Errorf("dhcp acquire on %s (client=%s): %w", req.Bridge, req.ClientID, err)
+		}
+
+		attemptTimeout := m.dhcpAttemptTimeout(base)
+		var (
+			ctx    context.Context
+			cancel context.CancelFunc
+		)
+		if attemptTimeout > 0 {
+			ctx, cancel = context.WithTimeout(parent, attemptTimeout)
+		} else {
+			ctx, cancel = context.WithCancel(parent)
+		}
+
+		lease, err := m.client.Acquire(ctx, req)
+		cancel()
+		if err == nil {
+			return lease, nil
+		}
+		lastErr = err
+
+		if i < len(schedule)-1 {
+			slog.Debug("DHCP Manager: DORA retransmit",
+				"client_id", req.ClientID, "bridge", req.Bridge,
+				"attempt", i+1, "next_timeout", m.dhcpAttemptTimeout(schedule[i+1]),
+				"prev_err", err)
+		}
+	}
+	return nil, lastErr
+}
+
+// dhcpAttemptTimeout applies ±1s jitter to a base DISCOVER timeout.
+// Pass 0 to disable timeout (used when schedule is empty fallback).
+func (m *DHCPManager) dhcpAttemptTimeout(base time.Duration) time.Duration {
+	if base <= 0 {
+		return 0
+	}
+	jitter := time.Duration((m.rng.Float64()*2 - 1) * float64(time.Second))
+	out := base + jitter
+	if out <= 0 {
+		return base
+	}
+	return out
+}
+
 // tryRenew calls the backend Renew under a bounded context.
 func (m *DHCPManager) tryRenew(parent context.Context, lease *dhcp.Lease) (*dhcp.Lease, error) {
 	ctx, cancel := context.WithTimeout(parent, m.acquireTimeout)
@@ -412,6 +412,125 @@ func TestDHCPManager_EncodeDecodeLeaseRoundTrip(t *testing.T) {
 	require.Equal(t, in.RawOffer, out.RawOffer)
 }

+// --- acquireWithBackoff (mulga-siv-39, RFC 2131 §4.1) ---
+
+func newBackoffManager(t *testing.T, schedule []time.Duration, fake *dhcp.Fake) *DHCPManager {
+	t.Helper()
+	_, nc, js := testutil.StartTestJetStream(t)
+	m, err := NewDHCPManager(nc, js, fake, WithDHCPAcquireBackoff(schedule))
+	require.NoError(t, err)
+	t.Cleanup(m.Close)
+	return m
+}
+
+func TestAcquireWithBackoff_FirstAttemptSucceeds(t *testing.T) {
+	fake := dhcp.NewFake()
+	m := newBackoffManager(t, []time.Duration{10 * time.Millisecond, 10 * time.Millisecond}, fake)
+
+	lease, err := m.acquireWithBackoff(context.Background(), dhcp.AcquireRequest{
+		Bridge: "br-wan", ClientID: "eni-fast", HWAddr: net.HardwareAddr{0x02, 0, 0, 0, 0, 1},
+	})
+	require.NoError(t, err)
+	require.NotNil(t, lease)
+	require.Equal(t, 1, fake.AcquireCount(), "first-attempt success must not retransmit")
+}
+
+func TestAcquireWithBackoff_RetriesThenSucceeds(t *testing.T) {
+	fake := dhcp.NewFake()
+	var calls atomic.Int32
+	tmpl := dhcp.DefaultLeaseTemplate()
+	fake.AcquireHook = func(req dhcp.AcquireRequest) (*dhcp.Lease, error) {
+		n := calls.Add(1)
+		if n < 3 {
+			return nil, errors.New("simulated OFFER timeout")
+		}
+		return &dhcp.Lease{
+			Bridge: req.Bridge, ClientID: req.ClientID, HWAddr: req.HWAddr,
+			IP: tmpl.IP, ServerID: tmpl.ServerID, LeaseDuration: tmpl.LeaseDuration,
+			AcquiredAt: time.Now(),
+		}, nil
+	}
+	m := newBackoffManager(t, []time.Duration{
+		5 * time.Millisecond, 5 * time.Millisecond, 5 * time.Millisecond, 5 * time.Millisecond,
+	}, fake)
+
+	lease, err := m.acquireWithBackoff(context.Background(), dhcp.AcquireRequest{
+		Bridge: "br-wan", ClientID: "eni-retry", HWAddr: net.HardwareAddr{0x02, 0, 0, 0, 0, 2},
+	})
+	require.NoError(t, err)
+	require.NotNil(t, lease)
+	require.Equal(t, int32(3), calls.Load())
+}
+
+func TestAcquireWithBackoff_AllAttemptsFail(t *testing.T) {
+	fake := dhcp.NewFake()
+	fake.AcquireHook = func(dhcp.AcquireRequest) (*dhcp.Lease, error) {
+		return nil, errors.New("server silent")
+	}
+	schedule := []time.Duration{
+		2 * time.Millisecond, 2 * time.Millisecond, 2 * time.Millisecond,
+	}
+	m := newBackoffManager(t, schedule, fake)
+
+	lease, err := m.acquireWithBackoff(context.Background(), dhcp.AcquireRequest{
+		Bridge: "br-wan", ClientID: "eni-fail", HWAddr: net.HardwareAddr{0x02, 0, 0, 0, 0, 3},
+	})
+	require.Error(t, err)
+	require.Nil(t, lease)
+	require.ErrorContains(t, err, "server silent")
+	require.Equal(t, len(schedule), fake.AcquireCount(),
+		"every entry in the schedule must run before giving up")
+}
+
+func TestAcquireWithBackoff_ParentCtxCancel(t *testing.T) {
+	fake := dhcp.NewFake()
+	fake.AcquireHook = func(dhcp.AcquireRequest) (*dhcp.Lease, error) {
+		return nil, errors.New("should not be called")
+	}
+	m := newBackoffManager(t, []time.Duration{
+		time.Second, time.Second, time.Second,
+	}, fake)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	lease, err := m.acquireWithBackoff(ctx, dhcp.AcquireRequest{
+		Bridge: "br-wan", ClientID: "eni-cancel", HWAddr: net.HardwareAddr{0x02, 0, 0, 0, 0, 4},
+	})
+	require.Error(t, err)
+	require.Nil(t, lease)
+	require.ErrorIs(t, err, context.Canceled)
+	require.Equal(t, 0, fake.AcquireCount(), "cancelled parent must short-circuit before any attempt")
+}
+
+func TestDHCPAttemptTimeoutJitterBounds(t *testing.T) {
+	_, nc, js := testutil.StartTestJetStream(t)
+	m, err := NewDHCPManager(nc, js, dhcp.NewFake())
+	require.NoError(t, err)
+	t.Cleanup(m.Close)
+
+	base := 4 * time.Second
+	for i := range 200 {
+		got := m.dhcpAttemptTimeout(base)
+		if got < base-time.Second || got > base+time.Second {
+			t.Fatalf("iter %d: dhcpAttemptTimeout(%v) = %v, outside ±1s", i, base, got)
+		}
+	}
+	require.Zero(t, m.dhcpAttemptTimeout(0))
+}
+
+func TestNewDHCPManager_DefaultBackoffSchedule(t *testing.T) {
+	_, nc, js := testutil.StartTestJetStream(t)
+	m, err := NewDHCPManager(nc, js, dhcp.NewFake())
+	require.NoError(t, err)
+	t.Cleanup(m.Close)
+
+	require.Equal(t, []time.Duration{
+		4 * time.Second, 8 * time.Second, 16 * time.Second, 32 * time.Second,
+	}, m.acquireBackoff)
+	require.Equal(t, 90*time.Second, m.acquireTimeout)
+}
+
 func TestResolveHWAddr(t *testing.T) {
 	t.Run("InvalidRawMACReturnsParseError", func(t *testing.T) {
 		mac, err := resolveHWAddr("not-a-mac", "client-1", nil)
@@ -10,7 +10,6 @@ import (
 	"path/filepath"
 	"strings"
 	"syscall"
-	"time"

 	"github.com/mulgadc/spinifex/spinifex/admin"
 	"github.com/mulgadc/spinifex/spinifex/services/vpcd/dhcp"
@@ -404,7 +403,9 @@ func launchService(cfg *Config) error {
 		slog.Error("Failed to get JetStream context for DHCP manager", "err", err)
 		return err
 	}
-	dhcpManager, err := NewDHCPManager(nc, js, dhcp.NewNClient4(15*time.Second, 3))
+	// timeout/retry args are legacy no-ops (mulga-siv-39): DHCPManager
+	// owns DORA retransmission via acquireWithBackoff.
+	dhcpManager, err := NewDHCPManager(nc, js, dhcp.NewNClient4(0, 0))
 	if err != nil {
 		slog.Error("Failed to create DHCP manager", "err", err)
 		return err