Repository navigation
Conversation
The uffd serve goroutine carried a stale TODO ("if the handle function fails,
we should kill the sandbox") and logged nothing when handle() failed, so a
userfaultfd failure was invisible in production.
- Remove the misleading TODO.
- Log the failure at ERROR, but only for a genuine serve-loop failure: the
existing stopping-time close (net.ErrClosed / context.Canceled) already
zeroes exitErr, so the log is gated on exitErr != nil and does not fire on
normal sandbox stop.
- Add an accurate comment on the resume-path lifecycle watcher noting that a
completed fcUffd.Exit() (which a serve-loop failure triggers) is turned into
a sandbox stop there, so a dead UFFD handler does not leave a VM without a
page-fault handler. Scoped to the resume path, which is the one that watches
fcUffd.Exit().Done().
Rebased, trimmed replacement for e2b-dev#3132 (envd/sandbox code was reworked since).
Adds a focused test asserting a handle failure surfaces a non-nil uffd exit
error - the condition that gates the new log. Revisits e2b-dev#3131.
AdaAibaby
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
October 22, 2026 04:20
This branch has not been deployed
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
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.
Summary
The userfaultfd (UFFD) serve goroutine carried a stale, misleading
TODO("if the handle function fails, we should kill the sandbox") and logged nothing whenhandle()failed, leaving UFFD failures invisible in production.Rebased, trimmed replacement for #3132 (the envd/sandbox and uffd code was reworked on main since). Revisits #3131.
Changes
uffd/uffd.go— remove the staleTODO; add anERRORlog for a genuine serve-loop failure. The log is gated onexitErr != nil, which the existing stopping-time check already zeroes for an expected close (net.ErrClosed/context.Canceled), so it does not fire on a normal sandbox stop — only on real failures.sandbox/sandbox.go— add an accurate comment on the resume-path lifecycle watcher: a serve-loop failure completesfcUffd.Exit(), and theselect { case <-fcUffd.Exit().Done(): ... }there turns that intosbx.Stop(), so a dead UFFD handler does not leave a VM without a page-fault handler. Scoped to the resume path (the one that actually watchesfcUffd.Exit().Done()).uffd/handle_failure_log_test.go(new) — asserts the invariant the log depends on: a genuine handle failure (malformed handshake) completesu.Exit()with a non-nil error.Scope note
Deliberately minimal, matching what was welcomed for a fresh PR: the log line and the TODO removal. The original #3132 claimed the sandbox "is already stopped" unconditionally; on current
mainthat is only true on the resume path (the create path's watcher waits onfcHandle.Exitonly), so the comment is scoped accordingly rather than restated as universal.Test
go build(uffd + sandbox),go vet,gofmt, and the fullpkg/sandbox/uffdsuite pass.