Repository navigation
test(uffd): add cross-process scaffolding for gated and async ops - #2475
Conversation
Pre-work for the upcoming REMOVE event handling PR. Lands as a runtime no-op on main: no production code changes, no new UFFD features enabled. Test-helper additions (all *_test.go): - testConfig.gated and per-op `async bool` - operationModeRemove / ServePause / ServeResume / Sleep - testHandler.servePause / serveResume hooks driven by control pipes (fd 7/8) into the helper subprocess - helper subprocess pause/resume goroutine that tears down and restarts the UFFD serve loop on demand - executeAll() async path that fans out goroutines and joins on test context - expectRemoved + checkDirtiness handling for read/write/remove ordering - executeRemove via unix.MADV_DONTNEED - unregister() helper around UFFDIO_UNREGISTER for cleanup - pageStateEntries() now drains settleRequests then takes the pageTracker RLock directly so the snapshot stays correct for future writers (e.g. REMOVE) that don't go through settleRequests operationModeRemove and the gated/async paths are exercised only by the follow-up PR; here they are dead code at runtime, kept to minimise the diff of that PR.
PR SummaryLow Risk Overview Reviewed by Cursor Bugbot for commit 38e232b. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab71d89fe7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review caught a deadlock: in GO_GATED mode, handling 'P' replaced the stop closure with one that drained `exitUffd`, but the deferred final cleanup still pointed at the original closure. If the helper exited between 'P' and 'R' (e.g. parent test failure, ctx cancel, SIGUSR1), the defer ran the original cleanup again and blocked forever on `<-exitUffd` — the channel had already been drained and the original Serve goroutine had already exited. The hung helper would in turn hang the parent's cmd.Wait(). Replace the two-variable (cleanup / stopServe) layout with a single mutex-guarded `stopFn` that is reset to a no-op after firing, and have both the gated 'P' handler and the deferred final cleanup go through the same `stopServe` wrapper. 'R' now just installs a fresh `stopFn` that drains the new goroutine.
Pull out everything that's not strictly needed for "gated pause/resume +
async ops" so PR T is the smallest possible scaffolding diff and the
follow-up REMOVE PR carries those pieces alongside the production code
that actually needs them:
- helpers_test.go: drop operationModeRemove, executeRemove, expectRemoved
and the REMOVE-aware checkDirtiness branch — none of the PR T tests
call them, and they only become meaningful once UFFD_FEATURE_EVENT_REMOVE
is set in the follow-up.
- fd_helpers_test.go: revert; drop unregister() helper. Without
UFFD_FEATURE_EVENT_REMOVE, MADV_DONTNEED never queues UFFD events, so
munmap doesn't block on un-acked events and there's nothing to
unregister around.
- cross_process_helpers_test.go:
- drop unregister(uffdFd, ...) call in late cleanup (same reason as
above).
- revert pageStateEntries() to the simple settleRequests.Lock pattern;
the rework was forward-looking for a REMOVE handler that doesn't go
through settleRequests and is dead code on main.
- keep cleanup → stopFn/stopServe restructure (mandatory for the
gated tear-down/respawn path) but restore the original
fdExit.SignalExit error handling that the previous version had
silently dropped.
- revert incidental cosmetic reformats (exitUffd placement, defer
multi-line) so the diff is purely additive in the unchanged paths.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bbdc8f9. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbdc8f921f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
removed state, and deterministic race tests
#2513
- Guard executeOperation against nil servePause/serveResume hooks when a non-gated test misuses operationModeServePause/Resume — return a clear error instead of panicking on a nil function call (cursor + codex). - Make pageStateEntries() also hold pageTracker.mu.RLock() so the snapshot stays correct for any future writer that doesn't go through settleRequests (claude — matches the PR description). - Make gated 'P' check the gateSyncFile.Write error and propagate via cancel() so a write failure doesn't leave the parent's servePause closure blocked indefinitely on Read (claude). - Make the gated command goroutine reject 'P'-when-paused and 'R'-when-running so a stray or duplicate resume can't leak an untracked Serve goroutine and break later pauses (codex).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c100bc33c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Previously executeWrite took h.mutex.Lock() but executeRead held no lock, so an async write + async read on the same page would race on memoryArea under go test -race even though the bytes happen to match (data.Slice is deterministic per offset). Switch h.mutex to sync.RWMutex; reads take RLock so concurrent reads still trigger UFFD faults in parallel, writes take Lock so they exclude both other writers and any in-flight reads. No active test in this PR mixes async read/write yet; this is preemptive against the upcoming REMOVE PR's matrix tests.
Resolve conflicts in cross_process_helpers_test.go and helpers_test.go by taking the RPC-based test harness from this branch and folding in the post-#2475 fixes from main: - helpers_test.go: keep the RPC client/conn/cmd fields plus mutex as sync.RWMutex (so executeRead can RLock and executeWrite can Lock, matching the just-landed race fix) and the nil-guards in executeOperation for ServePause/ServeResume on non-gated handlers. - cross_process_helpers_test.go: keep the net/rpc + jsonrpc harness, add the Service.startServe idempotency guard (early-return when a serve goroutine is already running) so a stray duplicate ServeResume RPC can't leak an untracked Serve goroutine, mirroring the running flag fix on main. Also add the unregister(uffdFd, ...) cleanup at the end of the cmd-wait t.Cleanup so a future REMOVE-events test doesn't hang in munmap on un-acked events. - fd_helpers_test.go: auto-merged to keep the unregister helper from main (the RPC harness now uses it).
Summary
Pre-work for the upcoming UFFD
REMOVEevent handling PR. Lands as a runtime no-op on main: no production code changes, no new UFFD features enabled. Splitting it out so the REMOVE PR is just the production logic + the tests that actually exercise it.All changes are in
*_test.gofiles inpackages/orchestrator/pkg/sandbox/uffd/userfaultfd/.What this adds
helpers_test.go— generic test API surfacetestConfig.gatedflag + per-operationasync booloperationModes:Remove,ServePause,ServeResume,SleeptestHandler.servePause/serveResumehooksexecuteAllasync path: fan out goroutines, join ont.Context()expectRemoved+ read/write/remove ordering incheckDirtinessexecuteRemoveviaunix.MADV_DONTNEEDfd_helpers_test.go— UFFD ioctl helpersunregister()wrappingUFFDIO_UNREGISTERcross_process_helpers_test.go— main↔helper plumbingtt.gated == trueServeloop on demand (drains viafdexit, then spins a fresh one)unregister(uffdFd, ...)before close in cleanuppageStateEntries()reworked to drainsettleRequestsand then take thepageTrackerRLock directly, so the snapshot stays correct for future writers that don't go throughsettleRequests(the REMOVE handler in the follow-up)Why land it separately
Remove/ServePause/ServeResume/Sleepoperations andgated/asyncpaths are not exercised by any test in this PR. They're live-but-unused — dead at runtime — and kept here only to minimise the follow-up's diff. Without them, the REMOVE PR would have to add ~200 lines of test infra alongside the actual logic.Test plan
go build ./...cleango vet ./pkg/sandbox/uffd/...cleangolangci-lint run ./pkg/sandbox/uffd/userfaultfd/...cleango test ./pkg/sandbox/uffd/userfaultfd/...passes (withvm.unprivileged_userfaultfd=1)