fix(orchestrator): bug fix in pkg/nfsproxy/recovery (#2606)

`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.

Author: tvi

packages/orchestrator/pkg/nfsproxy/recovery/main.go

@@ -25,15 +25,15 @@ 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)
 
 	return s, fs, auth
 }
 
 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"),