fix(orchestrator): abort the NBD release backoff on context cancellation - #3495
Conversation
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.
PR SummaryLow Risk Overview That matters for sandbox stop and pool teardown paths that call Reviewed by Cursor Bugbot for commit d59d13c. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
LGTM — a correct, minimal fix that swaps an uninterruptible time.Sleep for a select on ctx.Done()/time.After, so the backoff aborts immediately on cancellation or timeout instead of after up to 500ms.
Extended reasoning...
Overview
The change is confined to packages/orchestrator/pkg/sandbox/nbd/pool.go's ReleaseDevice retry loop, plus a new test file (pool_release_test.go) exercising the fix. The old code called time.Sleep(500 * time.Millisecond) between retry attempts, which cannot observe context cancellation or a WithTimeout deadline mid-sleep. The fix replaces it with select { case <-ctx.Done(): return ctx.Err(); case <-time.After(releaseRetryDelay): }, which is the standard idiomatic pattern for an interruptible delay in Go.
Security risks
None. This is a timing/shutdown-latency fix in device-release bookkeeping; it does not touch auth, data handling, or any externally reachable surface.
Level of scrutiny
Low-to-moderate scrutiny is appropriate: the change is small (a single loop body edit plus a new constant), touches only the internal retry/backoff mechanics of NBD device release used during sandbox stop and orchestrator shutdown, and does not alter the non-retry code path (pinned by the pre-existing/added TestReleaseDeviceReturnsFirstErrorWithoutInfiniteRetry). I traced through the new tests (retryingPool helper, unreachableSlot forcing a deterministic release failure) and confirmed the assertions match the described repro: cancellation aborts before the full 500ms backoff via context.Canceled, WithTimeout aborts via context.DeadlineExceeded, and the non-infinite-retry path still returns the first error immediately without wrapping it as cancellation.
Other factors
The PR includes tests that fail on unmodified main and pass after the fix, per the author's description, and Codecov reports full coverage of the changed lines. The change is consistent with how Populate's retry loop in the same file already uses time.Sleep only for non-context-sensitive paths, so this doesn't introduce a new pattern, just fixes an inconsistency in one call site. No outstanding review comments from bugbot/codecov need addressing, and there are no prior reviews from me on this PR.
…ion (#3495) Fixes #3347 ## What was broken `DevicePool.ReleaseDevice` kept retrying a stuck NBD device for a full 500ms after its context was already cancelled. With `WithInfiniteRetry()` — used by `DirectPathMount.Close` on every sandbox stop and by `DevicePool.Close` on teardown — each remaining attempt added another 500ms to orchestrator shutdown, and the `WithTimeout` deadline `Close` depends on could only take effect one interval late. ## Root cause The retry loop checked `ctx.Done()` only at the top and then called `time.Sleep(500 * time.Millisecond)`, which is not interruptible. Cancellation was invisible for the whole backoff. ## Reproduction Added tests that drive the retry loop against a device index no NBD device can have, so the free-check fails deterministically whether or not the nbd module is loaded. On unmodified `main` both cancellation tests fail: ``` "510.111732ms" is not less than "500ms" ReleaseDevice slept through the cancellation instead of aborting the backoff "509.958285ms" is not less than "500ms" ReleaseDevice slept past its own deadline ``` `TestReleaseDeviceReturnsFirstErrorWithoutInfiniteRetry` passes before and after, pinning the non-retrying path. ## Verification All three tests pass after the fix (`-count=5`, no flakes); `gofmt`, `go vet` and `golangci-lint v2.11.4` are clean. The pre-existing `TestPathDirect_*` / `TestSlowBackend_*` cases in this package need the nbd kernel module and fail identically on unmodified `main` in my container, so they are unchanged by this PR and I am relying on CI's `infra-tests` runner to exercise them.
Fixes #3347
What was broken
DevicePool.ReleaseDevicekept retrying a stuck NBD device for a full 500ms after its context was already cancelled. WithWithInfiniteRetry()— used byDirectPathMount.Closeon every sandbox stop and byDevicePool.Closeon teardown — each remaining attempt added another 500ms to orchestrator shutdown, and theWithTimeoutdeadlineClosedepends on could only take effect one interval late.Root cause
The retry loop checked
ctx.Done()only at the top and then calledtime.Sleep(500 * time.Millisecond), which is not interruptible. Cancellation was invisible for the whole backoff.Reproduction
Added tests that drive the retry loop against a device index no NBD device can have, so the free-check fails deterministically whether or not the nbd module is loaded. On unmodified
mainboth cancellation tests fail:TestReleaseDeviceReturnsFirstErrorWithoutInfiniteRetrypasses before and after, pinning the non-retrying path.Verification
All three tests pass after the fix (
-count=5, no flakes);gofmt,go vetandgolangci-lint v2.11.4are clean.The pre-existing
TestPathDirect_*/TestSlowBackend_*cases in this package need the nbd kernel module and fail identically on unmodifiedmainin my container, so they are unchanged by this PR and I am relying on CI'sinfra-testsrunner to exercise them.