diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index ed013249e..81467e76f 100644 --- a/client/vnc/server/server.go +++ b/client/vnc/server/server.go @@ -12,6 +12,7 @@ import ( "io" "net" "net/netip" + "slices" "sync" "syscall" "time" @@ -1295,13 +1296,26 @@ func approvalRejection(err error) (string, string) { } // acceptRetryable reports whether an Accept error can plausibly clear on its -// own. Running out of file descriptors can, once open connections close, and an -// aborted handshake concerns one client rather than the listener. Anything else -// will fail the same way on every call, and retrying it is not recovery but a -// livelock: Android has been seen returning EINVAL from accept4 for the life of -// an otherwise healthy listening socket. +// own: a pending connection that died before it could be handed over, or a +// resource shortage that eases as open connections close and memory frees. +// Both concern one attempt rather than the listening socket. +// +// The set is an allowlist rather than a denylist of fatal errors, and it +// deliberately does not use net.Error.Temporary: an error that recurs on every +// call, such as EINVAL from a listening socket whose interface went away, is +// reported as temporary there and would turn the loop into a livelock instead +// of a recovery. func acceptRetryable(err error) bool { - return errors.Is(err, syscall.ECONNABORTED) || - errors.Is(err, syscall.EMFILE) || - errors.Is(err, syscall.ENFILE) + retryable := []syscall.Errno{ + syscall.ECONNABORTED, // the pending connection was reset during the handshake + syscall.ECONNRESET, // same, reported differently by some stacks + syscall.ETIMEDOUT, // the pending connection timed out before being handed over + syscall.EMFILE, // per-process descriptor limit + syscall.ENFILE, // system-wide descriptor limit + syscall.ENOBUFS, // socket buffer memory exhausted + syscall.ENOMEM, // kernel memory exhausted + } + return slices.ContainsFunc(retryable, func(errno syscall.Errno) bool { + return errors.Is(err, errno) + }) } diff --git a/client/vnc/server/server_test.go b/client/vnc/server/server_test.go index a703730c0..9fcad39ef 100644 --- a/client/vnc/server/server_test.go +++ b/client/vnc/server/server_test.go @@ -15,6 +15,7 @@ import ( "net/netip" "strings" "sync/atomic" + "syscall" "testing" "time" @@ -719,3 +720,32 @@ func TestCloseAdmission_HandlerOutlivingDrain(t *testing.T) { t.Fatal("drain signal never fired after restart") } } + +func TestAcceptRetryable(t *testing.T) { + tests := []struct { + name string + err error + retryable bool + }{ + {"aborted handshake", syscall.ECONNABORTED, true}, + {"reset pending connection", syscall.ECONNRESET, true}, + {"timed out pending connection", syscall.ETIMEDOUT, true}, + {"process fd limit", syscall.EMFILE, true}, + {"system fd limit", syscall.ENFILE, true}, + {"no socket buffers", syscall.ENOBUFS, true}, + {"out of kernel memory", syscall.ENOMEM, true}, + // A retryable errno reached through the layers Accept actually wraps it in. + {"wrapped in OpError", &net.OpError{Op: "accept", Err: syscall.ECONNABORTED}, true}, + // EINVAL persists for the life of a socket whose interface is gone, so + // retrying it livelocks. net.Error.Temporary would call it temporary. + {"invalid listening socket", syscall.EINVAL, false}, + {"listener closed", net.ErrClosed, false}, + {"unrelated error", errors.New("boom"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.retryable, acceptRetryable(tt.err)) + }) + } +}