From dcb58741af126073f967c9f4758a5b3d345f146d Mon Sep 17 00:00:00 2001
From: Tomas Virgl <739690+tvi@users.noreply.github.com>
Date: Sat, 9 May 2026 17:00:03 -0700
Subject: [PATCH] fix(orchestrator): Bug fix in pkg/nfsproxy/recovery
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

       `pkg/nfsproxy/recovery` had a real defect that surfaced on every
       platform but was masked because the package's tests were the only
       thing exercising the panic path; on darwin the test harness ran but
       still crashed with `panic: Mount [recovered, repanicked]`.

       Bug fix in pkg/nfsproxy/recovery
       `Handler.Mount`, `Change`, `ToHandle` and `HandleLimit` deferred a
       *method* `h.tryRecovery(ctx, name)` whose body simply called the
       package-level `tryRecovery(ctx, name)`. Because `recover()` only
       returns non-nil when invoked directly inside the deferred function
       frame, the indirection meant `recover()` always returned nil and
       panics propagated out of `Handler.Mount` & friends — defeating the
       whole point of the recovery wrapper.

       `TestHandler_Mount_Panic_NoCrash` was written to assert exactly this
       property and consequently failed on every platform.

       Replaced the pass-through method with a direct
       `defer tryRecovery(ctx, name)` call at every call site and removed
       the now-redundant `(Handler).tryRecovery` method. Added a comment to
       the package-level `tryRecovery` explaining why it must be called via
       `defer` directly.
---
 .../orchestrator/pkg/nfsproxy/recovery/main.go  | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/packages/orchestrator/pkg/nfsproxy/recovery/main.go b/packages/orchestrator/pkg/nfsproxy/recovery/main.go
index 01f78704b7..27cfc0ba2d 100644
--- a/packages/orchestrator/pkg/nfsproxy/recovery/main.go
+++ b/packages/orchestrator/pkg/nfsproxy/recovery/main.go
@@ -25,7 +25,7 @@ func WrapWithRecovery(ctx context.Context, h nfs.Handler) *Handler {
 }
 
 func (h Handler) Mount(ctx context.Context, conn net.Conn, request nfs.MountRequest) (nfs.MountStatus, billy.Filesystem, []nfs.AuthFlavor) {
-	defer h.tryRecovery(ctx, "Mount")
+	defer tryRecovery(ctx, "Mount")
 	s, fs, auth := h.inner.Mount(ctx, conn, request)
 	fs = wrapFS(ctx, fs)
 
@@ -33,7 +33,7 @@ func (h Handler) Mount(ctx context.Context, conn net.Conn, request nfs.MountRequ
 }
 
 func (h Handler) Change(ctx context.Context, filesystem billy.Filesystem) billy.Change {
-	defer h.tryRecovery(ctx, "Change")
+	defer tryRecovery(ctx, "Change")
 	c := h.inner.Change(ctx, filesystem)
 
 	return wrapChange(ctx, c)
@@ -46,7 +46,7 @@ func (h Handler) FSStat(ctx context.Context, filesystem billy.Filesystem, stat *
 }
 
 func (h Handler) ToHandle(ctx context.Context, fs billy.Filesystem, path []string) []byte {
-	defer h.tryRecovery(ctx, "ToHandle")
+	defer tryRecovery(ctx, "ToHandle")
 
 	return h.inner.ToHandle(ctx, fs, path)
 }
@@ -64,17 +64,16 @@ func (h Handler) InvalidateHandle(ctx context.Context, filesystem billy.Filesyst
 }
 
 func (h Handler) HandleLimit() int {
-	defer h.tryRecovery(h.ctx, "HandleLimit")
+	defer tryRecovery(h.ctx, "HandleLimit")
 
 	return h.inner.HandleLimit()
 }
 
-func (h Handler) tryRecovery(ctx context.Context, name string) {
-	tryRecovery(ctx, name)
-}
-
+// tryRecovery must be called via `defer` directly so that recover() runs in
+// the deferred function frame. Nesting it inside another helper would cause
+// recover() to return nil and the panic would propagate.
 func tryRecovery(ctx context.Context, name string) {
-	if r := recover(); r != nil { //nolint:revive // tryRecovery is always called via defer
+	if r := recover(); r != nil { //nolint:revive // always called via defer
 		logger.L().Error(ctx, fmt.Sprintf("panic in %q nfs handler", name),
 			zap.Any("panic", r),
 			zap.Stack("stack"),