From d59d13c4a3e3e9bbedf90cf99f16e26cfb668a79 Mon Sep 17 00:00:00 2001
From: Tomas Srnka <tomas.srnka@gmail.com>
Date: Sat, 1 Aug 2026 08:31:27 +0200
Subject: [PATCH] fix(orchestrator): abort the NBD release backoff on context
 cancellation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The ReleaseDevice retry loop only checked the context at the top of the
loop and then slept uninterruptibly between attempts. Both callers that
pass WithInfiniteRetry — DirectPathMount.Close on every sandbox stop and
DevicePool.Close on pool teardown — therefore kept retrying a stuck device
for a full backoff interval after their context was already cancelled, and
the WithTimeout deadline Close relies on could only take effect one
interval late.

Wait on the context alongside the backoff timer, matching the pattern the
connect retry loop in path_direct.go already uses, and name the interval.
---
 packages/orchestrator/pkg/sandbox/nbd/pool.go | 12 ++-
 .../pkg/sandbox/nbd/pool_release_test.go      | 82 +++++++++++++++++++
 2 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 packages/orchestrator/pkg/sandbox/nbd/pool_release_test.go

diff --git a/packages/orchestrator/pkg/sandbox/nbd/pool.go b/packages/orchestrator/pkg/sandbox/nbd/pool.go
index ba6e3e1198..2d11ff561c 100644
--- a/packages/orchestrator/pkg/sandbox/nbd/pool.go
+++ b/packages/orchestrator/pkg/sandbox/nbd/pool.go
@@ -26,6 +26,9 @@ const (
 	waitOnNBDError                = 50 * time.Millisecond
 	devicePoolCloseReleaseTimeout = 10 * time.Minute
 	sysBlockDir                   = "/sys/block"
+	// releaseRetryDelay is how long ReleaseDevice waits before retrying a device
+	// that is still in use.
+	releaseRetryDelay = 500 * time.Millisecond
 )
 
 var (
@@ -366,7 +369,14 @@ func (d *DevicePool) ReleaseDevice(ctx context.Context, idx DeviceSlot, opts ...
 			logger.L().Error(ctx, "error releasing device", zap.Int("attempt", attempt), zap.Error(err))
 		}
 
-		time.Sleep(500 * time.Millisecond)
+		// Wait on the context too: Close bounds a stuck device with WithTimeout,
+		// and shutdown cancels the context outright. An uninterruptible sleep
+		// here would make every remaining attempt outlive both.
+		select {
+		case <-ctx.Done():
+			return ctx.Err()
+		case <-time.After(releaseRetryDelay):
+		}
 	}
 }
 
diff --git a/packages/orchestrator/pkg/sandbox/nbd/pool_release_test.go b/packages/orchestrator/pkg/sandbox/nbd/pool_release_test.go
new file mode 100644
index 0000000000..114de95680
--- /dev/null
+++ b/packages/orchestrator/pkg/sandbox/nbd/pool_release_test.go
@@ -0,0 +1,82 @@
+//go:build linux
+
+package nbd
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/bits-and-blooms/bitset"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// unreachableSlot is an index no NBD device can have, so isDeviceFree always
+// fails to read /sys/block/nbd<idx>/size and release() keeps returning an
+// error. That drives the retry loop deterministically, whether or not the nbd
+// module is loaded on the machine running the test.
+const unreachableSlot = DeviceSlot(1 << 20)
+
+func retryingPool() *DevicePool {
+	return &DevicePool{
+		done:      make(chan struct{}),
+		usedSlots: bitset.New(16),
+		slots:     make(chan DeviceSlot, 1),
+	}
+}
+
+// A cancelled context must abort the release retry loop while it is backing
+// off, not only when it happens to be at the top of the loop. Otherwise every
+// stuck device adds a full backoff interval to orchestrator shutdown.
+func TestReleaseDeviceAbortsBackoffOnContextCancel(t *testing.T) {
+	t.Parallel()
+
+	pool := retryingPool()
+
+	ctx, cancel := context.WithCancel(t.Context())
+	defer cancel()
+
+	// Cancel once the first failed attempt has entered the backoff.
+	time.AfterFunc(releaseRetryDelay/10, cancel)
+
+	start := time.Now()
+	err := pool.ReleaseDevice(ctx, unreachableSlot, WithInfiniteRetry())
+	elapsed := time.Since(start)
+
+	require.ErrorIs(t, err, context.Canceled)
+	assert.Less(t, elapsed, releaseRetryDelay,
+		"ReleaseDevice slept through the cancellation instead of aborting the backoff")
+}
+
+// The same applies to the deadline installed by WithTimeout, which Close relies
+// on to bound how long a single stuck device can hold up the pool.
+func TestReleaseDeviceAbortsBackoffOnTimeout(t *testing.T) {
+	t.Parallel()
+
+	pool := retryingPool()
+
+	start := time.Now()
+	err := pool.ReleaseDevice(t.Context(), unreachableSlot,
+		WithInfiniteRetry(),
+		WithTimeout(releaseRetryDelay/10),
+	)
+	elapsed := time.Since(start)
+
+	require.ErrorIs(t, err, context.DeadlineExceeded)
+	assert.Less(t, elapsed, releaseRetryDelay,
+		"ReleaseDevice slept past its own deadline")
+}
+
+// Without infinite retry the first failure is returned immediately, so the
+// backoff change must not alter that path.
+func TestReleaseDeviceReturnsFirstErrorWithoutInfiniteRetry(t *testing.T) {
+	t.Parallel()
+
+	pool := retryingPool()
+
+	err := pool.ReleaseDevice(t.Context(), unreachableSlot)
+
+	require.Error(t, err)
+	assert.NotErrorIs(t, err, context.Canceled)
+}