orch: fix uffd prefault/close race condition - #2830
Merged
Merged
Conversation
PR SummaryMedium Risk Overview Close now takes the existing settleRequests write lock for the full shutdown, sets an idempotent closed flag, then closes fds; Prefault takes the read lock and returns nil without touching the fd when closed is set. A deterministic Linux regression test parks Prefault before the lock so Close can finish first. Reviewed by Cursor Bugbot for commit 64a02ef. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 6 Tests Failed:
View the full list of 7 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
Calling Close multiple times will invoke syscall.Close on the wakeupPipe file descriptors repeatedly, which can silently close unrelated active descriptors if they have been recycled by the operating system. Guarding the entire close sequence by checking and setting the closed flag under the settleRequests lock ensures idempotency and prevents accidental closure of recycled descriptors.
kalyazin
force-pushed
the
kalyazin/prefetch_race
branch
from
May 27, 2026 15:48
1cc4dcc to
b983a21
Compare
kalyazin
marked this pull request as ready for review
May 27, 2026 16:43
Prefault() and Close() could race: Close() freed the uffd fd number while a prefetcher goroutine was about to acquire settleRequests.RLock and call UFFDIO_COPY. If the OS recycled the fd to a non-uffd file between the close and the ioctl, the syscall returned ENOTTY (seen in production from 2026-04-26 onward, worsening after PR #2522 added an extra fd close per session). Fix by making Close() acquire settleRequests.Lock() before closing the fd and setting a `closed` flag. Prefault() checks the flag immediately after acquiring RLock; if set, it returns nil without touching the fd. This ensures the fd is only closed after all in-flight UFFDIO_COPY callers have released the read-lock. Also add faultPhaseBeforePrefaultRLock test hook so a regression test can deterministically park Prefault before the RLock, let Close() run, and verify the closed-check path. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
TestPrefaultConcurrentWithClose deterministically reproduces the race that caused ENOTTY/EBADF in production: a prefetcher goroutine is parked at faultPhaseBeforePrefaultRLock (before acquiring settleRequests.RLock), Close() is called to completion, and then the goroutine is released. The test asserts Prefault returns nil rather than an error from UFFDIO_COPY on a closed or recycled fd. The test is in-process (no cross-process harness needed — Prefault is called directly and short-circuits before any page-fault kernel interaction). The setup uses a real uffd fd so Close() can safely close a valid fd number. Also document BeforePrefaultRLock in the testharness Point constants so the value is named if cross-process tests ever need to park here. Signed-off-by: Nikita Kalyazin <nikita.kalyazin@e2b.dev>
kalyazin
force-pushed
the
kalyazin/prefetch_race
branch
from
May 28, 2026 08:04
b983a21 to
64a02ef
Compare
bchalios
approved these changes
May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix Prefault/Close race causing ENOTTY/EBADF
Motivation
Since 2026-04-26 the orchestrator has been logging
UFFD serve uffdio copy error: inappropriate ioctl for device(ENOTTY) andbad file descriptor(EBADF) from the UFFD prefetch path. The error rate increased sharply on 2026-05-27 after #2522 landed.The root cause is a race between the prefetcher and sandbox teardown:
Close()held no lock when it freed the uffd fd. The prefetcher runs on a non-cancellableexecCtxand has no way to observe thatClose()has run. If the OS recycled the fd number before the prefetcher'sUFFDIO_COPYioctl fired, the kernel returned ENOTTY because the fd now referred to a non-uffd file. The error was benign at the sandbox level (the sandbox was already being torn down) but noisy and misleading in logs.The bug was latent from when the prefetcher was introduced in January 2026 (#1705) but only started firing after the ubuntu24 template rebuild in April populated
Prefetch.Memorydata, causing the prefetcher to actually run.Fix
Close()now holdssettleRequests.Lock()for the entire close sequence and is idempotent:Prefault()checks the flag immediately after acquiringsettleRequests.RLock():This gives three safety guarantees:
Prefaultcaller already holdingRLockwhenClose()runs will complete itsUFFDIO_COPYagainst the still-valid fd beforeClose()can acquire the write lock.Prefaultcall that starts afterClose()returns will seeclosed == trueand return nil without touching the fd.Close()is idempotent: a second call returns immediately without touching already-freed fds, preventing accidental double-close of the wakeup pipe fds (and any unrelated fd the OS may have recycled those numbers to).settleRequestsalready existed for exactly this kind of serialisation (guarding the lookup→install→state-update sequence against REMOVE batches), so no new lock is introduced. In productionServe()drains all workers viau.wg.Wait()before returning, so by the time the deferredClose()fires the lock is always uncontended.Test
TestPrefaultConcurrentWithClosedeterministically reproduces the race using afaultPhaseBeforePrefaultRLocktest hook. The goroutine is parked before it acquiresRLock,Close()is called to completion, then the goroutine is released. Without the fix the test fails withfailed to fault page: failed uffdio copy: bad file descriptor; with the fix it returns nil.