From 76ec0450509daa3b1d90ee96bb21c593da16ba23 Mon Sep 17 00:00:00 2001
From: AdaAibaby <8872412+adababys@users.noreply.github.com>
Date: Thu, 22 Oct 2026 12:19:38 +0800
Subject: [PATCH] fix(uffd): log serve-loop failures and drop the stale handle
 TODO

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 #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 #3131.
---
 packages/orchestrator/pkg/sandbox/sandbox.go  |  5 +-
 .../sandbox/uffd/handle_failure_log_test.go   | 46 +++++++++++++++++++
 .../orchestrator/pkg/sandbox/uffd/uffd.go     |  9 +++-
 3 files changed, 58 insertions(+), 2 deletions(-)
 create mode 100644 packages/orchestrator/pkg/sandbox/uffd/handle_failure_log_test.go

diff --git a/packages/orchestrator/pkg/sandbox/sandbox.go b/packages/orchestrator/pkg/sandbox/sandbox.go
index 02e351b627..f031962d38 100644
--- a/packages/orchestrator/pkg/sandbox/sandbox.go
+++ b/packages/orchestrator/pkg/sandbox/sandbox.go
@@ -2116,7 +2116,10 @@ func (f *Factory) ResumeSandbox(
 		ctx, span := tracer.Start(execCtx, "sandbox-exit-wait")
 		defer span.End()
 
-		// Wait for either uffd or fc process to exit
+		// Wait for either uffd or fc process to exit. A userfaultfd serve-loop
+		// failure completes fcUffd.Exit(), so on this resume path a dead UFFD
+		// handler is turned into a sandbox stop here rather than leaving a VM
+		// with no page-fault handler.
 		select {
 		case <-fcUffd.Exit().Done():
 		case <-fcHandle.Exit.Done():
diff --git a/packages/orchestrator/pkg/sandbox/uffd/handle_failure_log_test.go b/packages/orchestrator/pkg/sandbox/uffd/handle_failure_log_test.go
new file mode 100644
index 0000000000..ed37ed6691
--- /dev/null
+++ b/packages/orchestrator/pkg/sandbox/uffd/handle_failure_log_test.go
@@ -0,0 +1,46 @@
+//go:build linux
+
+package uffd
+
+import (
+	"context"
+	"net"
+	"os"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestServeLoop_HandleFailureSetsExitError asserts the invariant the serve
+// loop ERROR log depends on: a genuine handle failure (here, a malformed
+// handshake) completes u.Exit() with a non-nil error. That same non-nil
+// exitErr is what gates the "uffd serve loop failed" log and what the
+// resume-path lifecycle watcher turns into a sandbox stop.
+func TestServeLoop_HandleFailureSetsExitError(t *testing.T) {
+	t.Parallel()
+	u := startHandshakeTest(t)
+
+	conn, err := net.DialUnix("unix", nil, &net.UnixAddr{Name: u.socketPath, Net: "unix"})
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = conn.Close() })
+
+	file, err := os.CreateTemp(t.TempDir(), "backing")
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = file.Close() })
+
+	// A malformed mapping body makes handle() fail (not an expected stop-time
+	// close), so exitErr stays non-nil.
+	_, _, err = conn.WriteMsgUnix([]byte("malformed mapping"), syscall.UnixRights(int(file.Fd())), nil)
+	require.NoError(t, err)
+
+	ctx, cancel := context.WithTimeout(t.Context(), time.Second)
+	defer cancel()
+	select {
+	case <-u.Exit().Done():
+	case <-ctx.Done():
+		t.Fatal("handle failure did not complete uffd exit")
+	}
+	require.Error(t, u.Exit().Error(), "a genuine handle failure must surface a non-nil exit error")
+}
diff --git a/packages/orchestrator/pkg/sandbox/uffd/uffd.go b/packages/orchestrator/pkg/sandbox/uffd/uffd.go
index bfa2a9afb5..6f7be32887 100644
--- a/packages/orchestrator/pkg/sandbox/uffd/uffd.go
+++ b/packages/orchestrator/pkg/sandbox/uffd/uffd.go
@@ -129,7 +129,6 @@ func (u *Uffd) Start(ctx context.Context) error {
 		ctx, span := tracer.Start(ctx, "serve uffd")
 		defer span.End()
 
-		// TODO: If the handle function fails, we should kill the sandbox
 		handleErr := u.handle(ctx, fdExit)
 
 		// If handle failed before setting the handler value, set an error to unblock
@@ -145,6 +144,14 @@ func (u *Uffd) Start(ctx context.Context) error {
 			exitErr = nil
 		}
 
+		// A non-nil exitErr here is a genuine userfaultfd serve failure (not an
+		// expected close during stop). Surface it: without this the failure was
+		// invisible in production, and it propagates through u.exit below, which
+		// the resume-path lifecycle watcher turns into a sandbox stop.
+		if exitErr != nil {
+			u.logger.Error(ctx, "uffd serve loop failed", zap.String("socket_path", u.socketPath), zap.Error(exitErr))
+		}
+
 		closeErr := u.lis.Close()
 		if errors.Is(closeErr, net.ErrClosed) {
 			closeErr = nil