fix: make state retry policy injectable (#836)

go.mod

@@ -25,7 +25,7 @@ require (
 	github.com/mulgadc/bluebottle v1.17.0
 	github.com/mulgadc/northstar v1.17.0
 	github.com/mulgadc/predastore v1.17.0
-	github.com/mulgadc/viperblock v1.17.0
+	github.com/mulgadc/viperblock v1.17.1-0.20260819232234-77b4fbe47481
 	github.com/nats-io/nats-server/v2 v2.14.5
 	github.com/nats-io/nats.go v1.53.1
 	github.com/opencontainers/runtime-spec v1.3.0

go.sum

@@ -1335,6 +1335,8 @@ github.com/mulgadc/predastore v1.17.0 h1:1xMFrzQU7dBaC5Q6qiAJx5ipj/yDubjtOJY7rjp
 github.com/mulgadc/predastore v1.17.0/go.mod h1:RM3hLSVsJAfwPJnmqbmcDgfdnP/RntXe+r1GOTD+S9c=
 github.com/mulgadc/viperblock v1.17.0 h1:ybcjcBf10jA1SiUkH5PTGFUZ9ZhGMljBff7iV06Vkg8=
 github.com/mulgadc/viperblock v1.17.0/go.mod h1:XYRmXrJ+k1P5nCxxTihcBUVM/4Srxc8zkFQp+a7ALNk=
+github.com/mulgadc/viperblock v1.17.1-0.20260819232234-77b4fbe47481 h1:kWiCPxfLURj6D06CA1rsaF2LP9EpwbJyqqTy/IdCCug=
+github.com/mulgadc/viperblock v1.17.1-0.20260819232234-77b4fbe47481/go.mod h1:XYRmXrJ+k1P5nCxxTihcBUVM/4Srxc8zkFQp+a7ALNk=
 github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA=
 github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ=
 github.com/mwitkow/go-conntrack v0.0.0-20161129095857-cc309e4a2223/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U=

spinifex/services/viperblockd/provider_handlers.go

@@ -1161,7 +1161,7 @@ func snapshotVolumeEngine(ctx context.Context, cfg *Config, volumeID, snapshotID
 	if err := vb.Backend.InitCtx(ctx); err != nil {
 		return nil, fmt.Errorf("backend init: %w", err)
 	}
-	if err := loadStateWithRetry(ctx, vb, volumeID); err != nil {
+	if err := loadStateWithRetry(ctx, cfg, vb, volumeID); err != nil {
 		return nil, fmt.Errorf("load state: %w", err)
 	}
 	if err := checkSnapshotIDFree(vb, volumeID, snapshotID); err != nil {
@@ -1314,7 +1314,7 @@ func constructMountedVB(ctx context.Context, cfg *Config, volumeName string) (*v
 	}
 
 	// Retry on transient backend errors so daemon recovery doesn't tip a healthy volume into cleanup.
-	if err := loadStateWithRetry(ctx, vb, volumeName); err != nil {
+	if err := loadStateWithRetry(ctx, cfg, vb, volumeName); err != nil {
 		return nil, 0, err
 	}
 

spinifex/services/viperblockd/viperblockd.go

@@ -40,11 +40,11 @@ func endSpanWithResponseError(span trace.Span, respErr string) {
 	span.End()
 }
 
-// loadStateRetryAttempts / loadStateRetryBaseDelay tune the mount-time retry
-// loop (5 attempts at 200ms * 1.5^n ≈ 3.7s; well under the 30s NATS timeout).
+// The default mount retry budget is five attempts over about 1.6 seconds of
+// backoff. Config fields override these values for isolated tests.
 const (
-	loadStateRetryAttempts  = 5
-	loadStateRetryBaseDelay = 200 * time.Millisecond
+	defaultLoadStateRetryAttempts  = 5
+	defaultLoadStateRetryBaseDelay = 200 * time.Millisecond
 )
 
 // retryLoadState invokes loadFn with exponential backoff (delay * 3/2 each step)
@@ -75,13 +75,12 @@ func retryLoadState(volume string, attempts int, baseDelay time.Duration, sleep
 	return fmt.Errorf("LoadState exhausted %d retries: %w", attempts, err)
 }
 
-// loadStateWithRetry calls vb.LoadStateCtx with the production retry budget
-// (loadStateRetryAttempts / loadStateRetryBaseDelay). The context matters
-// beyond cancellation: the S3 backend only emits a span per request when the
-// request carries one, so the no-context variant makes this time invisible.
-func loadStateWithRetry(ctx context.Context, vb *viperblock.VB, volume string) error {
+// loadStateWithRetry keeps the caller's context on every backend request so
+// cancellation and tracing span all attempts.
+func loadStateWithRetry(ctx context.Context, cfg *Config, vb *viperblock.VB, volume string) error {
+	attempts, baseDelay := cfg.loadStateRetryPolicy()
 	load := func() error { return vb.LoadStateCtx(ctx) }
-	return retryLoadState(volume, loadStateRetryAttempts, loadStateRetryBaseDelay, time.Sleep, load)
+	return retryLoadState(volume, attempts, baseDelay, time.Sleep, load)
 }
 
 var serviceName = "viperblock"
@@ -160,11 +159,15 @@ type Config struct {
 	masterKey *masterkey.Key
 
 	// s3HTTPClient is shared by every viperblock backend this service builds.
-	// One client per volume would be one connection pool per volume, so every
-	// engine open and state read would pay its own TLS handshake.
+	// One client per volume would create a connection pool per volume.
 	s3HTTPClientOnce sync.Once
 	s3HTTPClient     *http.Client
 
+	// Per-service retry knobs avoid mutable package state in tests. Zero values
+	// select the production defaults.
+	loadStateRetryAttempts  int
+	loadStateRetryBaseDelay time.Duration
+
 	// sealVolume overrides how a detached volume is sealed to predastore.
 	// Nil means sealVolumeVB, the real seal. Tests that need a seal to FAIL
 	// inject here rather than pointing S3Host at an unreachable endpoint: the
@@ -219,6 +222,18 @@ func (cfg *Config) buildVB(ctx context.Context, volumeName string) (*viperblock.
 	return vb, cacheSize, lease, nil
 }
 
+func (cfg *Config) loadStateRetryPolicy() (int, time.Duration) {
+	attempts := cfg.loadStateRetryAttempts
+	if attempts <= 0 {
+		attempts = defaultLoadStateRetryAttempts
+	}
+	baseDelay := cfg.loadStateRetryBaseDelay
+	if baseDelay <= 0 {
+		baseDelay = defaultLoadStateRetryBaseDelay
+	}
+	return attempts, baseDelay
+}
+
 type Service struct {
 	Config *Config
 }
@@ -331,7 +346,8 @@ func readVolumeState(ctx context.Context, cfg *Config, volumeName string) (viper
 		state, rerr = vb.ReadStateCtx(ctx)
 		return rerr
 	}
-	if err := retryLoadState(volumeName, loadStateRetryAttempts, loadStateRetryBaseDelay, time.Sleep, read); err != nil {
+	attempts, baseDelay := cfg.loadStateRetryPolicy()
+	if err := retryLoadState(volumeName, attempts, baseDelay, time.Sleep, read); err != nil {
 		return viperblock.VBState{}, fmt.Errorf("read state: %w", err)
 	}
 	return state, nil
@@ -373,7 +389,7 @@ func openVolumeVB(ctx context.Context, cfg *Config, volumeName string) (*viperbl
 	if err := vb.Backend.InitCtx(ctx); err != nil {
 		return nil, nil, fmt.Errorf("backend init: %w", err)
 	}
-	if err := loadStateWithRetry(ctx, vb, volumeName); err != nil {
+	if err := loadStateWithRetry(ctx, cfg, vb, volumeName); err != nil {
 		return nil, nil, fmt.Errorf("load state: %w", err)
 	}
 

spinifex/services/viperblockd/viperblockd_test.go

@@ -1,14 +1,19 @@
 package viperblockd
 
 import (
+	"context"
 	"errors"
 	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
+	vbtypes "github.com/mulgadc/viperblock/types"
 	"github.com/mulgadc/viperblock/viperblock"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -446,6 +451,46 @@ func TestMountedVolumePortRange(t *testing.T) {
 	}
 }
 
+func TestConfigLoadStateRetryPolicy(t *testing.T) {
+	attempts, delay := (&Config{}).loadStateRetryPolicy()
+	assert.Equal(t, defaultLoadStateRetryAttempts, attempts)
+	assert.Equal(t, defaultLoadStateRetryBaseDelay, delay)
+
+	cfg := &Config{loadStateRetryAttempts: 2, loadStateRetryBaseDelay: time.Nanosecond}
+	attempts, delay = cfg.loadStateRetryPolicy()
+	assert.Equal(t, 2, attempts)
+	assert.Equal(t, time.Nanosecond, delay)
+}
+
+// TestReadVolumeState_BadRequestFailsFast covers all three retry layers with
+// the production budgets: one HTTP 400 must produce one prompt request.
+func TestReadVolumeState_BadRequestFailsFast(t *testing.T) {
+	var requests atomic.Int32
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		requests.Add(1)
+		w.WriteHeader(http.StatusBadRequest)
+	}))
+	t.Cleanup(srv.Close)
+
+	cfg := &Config{
+		S3Host:    srv.URL,
+		Bucket:    "bucket",
+		Region:    "us-east-1",
+		AccessKey: "access-key",
+		SecretKey: "secret-key",
+		BaseDir:   t.TempDir(),
+	}
+
+	start := time.Now()
+	_, err := readVolumeState(context.Background(), cfg, "vol-rejected")
+	elapsed := time.Since(start)
+
+	require.Error(t, err)
+	assert.ErrorIs(t, err, vbtypes.ErrBackendNonRetryable)
+	assert.Equal(t, int32(1), requests.Load())
+	assert.Less(t, elapsed, time.Second)
+}
+
 // TestRetryLoadState pins the retry policy: transient errors retry with backoff,
 // ErrStateNotFound fails fast, and the retry budget is honoured.
 func TestRetryLoadState(t *testing.T) {