Attach to the X server on the active VT and keep a retryable DXGI frame from tearing down the capturer

Claude-Session: https://claude.ai/code/session_01QKDYfH4WKLbpNQHccpVo3P

client/proto/daemon.pb.go

@@ -7206,7 +7206,8 @@ type RespondApprovalRequest struct {
 	// when a subsystem awaits user approval for an inbound connection.
 	RequestId string `protobuf:"bytes,1,opt,name=request_id,json=requestId,proto3" json:"request_id,omitempty"`
 	// accept is true if the user approved the request, false if they
-	// denied it. A missing or unknown request_id is treated as a no-op.
+	// denied it. An unknown request_id is not an error; the response reports
+	// it as unmatched.
 	Accept bool `protobuf:"varint,2,opt,name=accept,proto3" json:"accept,omitempty"`
 	// view_only signals that the user granted the connection but withheld
 	// input control. Only meaningful when accept is true; ignored when
@@ -7268,7 +7269,12 @@ func (x *RespondApprovalRequest) GetViewOnly() bool {
 }
 
 type RespondApprovalResponse struct {
-	state         protoimpl.MessageState `protogen:"open.v1"`
+	state protoimpl.MessageState `protogen:"open.v1"`
+	// matched is true when request_id named a prompt that was still waiting.
+	// False means the prompt had already been answered, or had expired and the
+	// connection was denied: the click had no effect, and the UI should say so
+	// rather than reporting the outcome the user picked.
+	Matched       bool `protobuf:"varint,1,opt,name=matched,proto3" json:"matched,omitempty"`
 	unknownFields protoimpl.UnknownFields
 	sizeCache     protoimpl.SizeCache
 }
@@ -7303,6 +7309,13 @@ func (*RespondApprovalResponse) Descriptor() ([]byte, []int) {
 	return file_daemon_proto_rawDescGZIP(), []int{110}
 }
 
+func (x *RespondApprovalResponse) GetMatched() bool {
+	if x != nil {
+		return x.Matched
+	}
+	return false
+}
+
 type PortInfo_Range struct {
 	state         protoimpl.MessageState `protogen:"open.v1"`
 	Start         uint32                 `protobuf:"varint,1,opt,name=start,proto3" json:"start,omitempty"`
@@ -7977,8 +7990,9 @@ const file_daemon_proto_rawDesc = "" +
 	"\n" +
 	"request_id\x18\x01 \x01(\tR\trequestId\x12\x16\n" +
 	"\x06accept\x18\x02 \x01(\bR\x06accept\x12\x1b\n" +
-	"\tview_only\x18\x03 \x01(\bR\bviewOnly\"\x19\n" +
-	"\x17RespondApprovalResponse*b\n" +
+	"\tview_only\x18\x03 \x01(\bR\bviewOnly\"3\n" +
+	"\x17RespondApprovalResponse\x12\x18\n" +
+	"\amatched\x18\x01 \x01(\bR\amatched*b\n" +
 	"\bLogLevel\x12\v\n" +
 	"\aUNKNOWN\x10\x00\x12\t\n" +
 	"\x05PANIC\x10\x01\x12\t\n" +

client/proto/daemon.proto

@@ -1116,7 +1116,8 @@ message RespondApprovalRequest {
   // when a subsystem awaits user approval for an inbound connection.
   string request_id = 1;
   // accept is true if the user approved the request, false if they
-  // denied it. A missing or unknown request_id is treated as a no-op.
+  // denied it. An unknown request_id is not an error; the response reports
+  // it as unmatched.
   bool accept = 2;
   // view_only signals that the user granted the connection but withheld
   // input control. Only meaningful when accept is true; ignored when
@@ -1124,4 +1125,10 @@ message RespondApprovalRequest {
   bool view_only = 3;
 }
 
-message RespondApprovalResponse {}
+message RespondApprovalResponse {
+  // matched is true when request_id named a prompt that was still waiting.
+  // False means the prompt had already been answered, or had expired and the
+  // connection was denied: the click had no effect, and the UI should say so
+  // rather than reporting the outcome the user picked.
+  bool matched = 1;
+}

client/server/server.go

@@ -2112,10 +2112,11 @@ func (s *Server) RespondApproval(ctx context.Context, msg *proto.RespondApproval
 	if engine == nil {
 		return nil, gstatus.Errorf(codes.FailedPrecondition, "engine not running")
 	}
-	if !engine.RespondApproval(msg.GetRequestId(), msg.GetAccept(), msg.GetViewOnly()) {
+	matched := engine.RespondApproval(msg.GetRequestId(), msg.GetAccept(), msg.GetViewOnly())
+	if !matched {
 		log.Debugf("approval response for unknown request_id %s", msg.GetRequestId())
 	}
-	return &proto.RespondApprovalResponse{}, nil
+	return &proto.RespondApprovalResponse{Matched: matched}, nil
 }
 func (s *Server) runProbes(ctx context.Context, waitForProbeResult bool) {
 	if s.connectClient == nil {

client/ui/services/approval.go

@@ -5,6 +5,8 @@ package services
 import (
 	"context"
 
+	log "github.com/sirupsen/logrus"
+
 	"github.com/netbirdio/netbird/client/proto"
 )
 
@@ -27,10 +29,20 @@ func (a *Approval) Respond(ctx context.Context, requestID string, accept, viewOn
 	if err != nil {
 		return err
 	}
-	_, err = cli.RespondApproval(ctx, &proto.RespondApprovalRequest{
+	resp, err := cli.RespondApproval(ctx, &proto.RespondApprovalRequest{
 		RequestId: requestID,
 		Accept:    accept,
 		ViewOnly:  viewOnly,
 	})
-	return err
+	if err != nil {
+		return err
+	}
+	if !resp.GetMatched() {
+		// Not an error to the caller: the dialog closes either way, and the
+		// connection has already been denied by the broker's timeout. Logged so
+		// a report of "I clicked accept and it still disconnected" has a record
+		// showing the click landed after the prompt had expired.
+		log.Infof("approval %s was no longer pending; the daemon had already answered it", requestID)
+	}
+	return nil
 }

client/vnc/server/agent_handshake.go

@@ -156,9 +156,13 @@ func agentServerHandshake(conn net.Conn, token []byte) (bool, error) {
 // surfaces as a failed write (a reset or broken pipe) as often as a failed
 // read, and logging either at warning level would fill the daemon log with
 // entries for something entirely expected.
+//
+// io.ErrUnexpectedEOF is deliberately not here: that is a peer that sent part
+// of a handshake and then went away, which is an aborted or malformed
+// authentication attempt rather than a probe, and has to stay visible.
 func isProbeDisconnect(err error) bool {
 	switch {
-	case errors.Is(err, io.EOF), errors.Is(err, io.ErrUnexpectedEOF):
+	case errors.Is(err, io.EOF):
 		return true
 	case errors.Is(err, net.ErrClosed):
 		return true

client/vnc/server/capture_dxgi_windows.go

@@ -7,8 +7,6 @@ import (
 	"fmt"
 	"image"
 
-	log "github.com/sirupsen/logrus"
-
 	"github.com/kirides/go-d3d/d3d11"
 	"github.com/kirides/go-d3d/outputduplication"
 )
@@ -66,13 +64,19 @@ func newDXGICapturer() (*dxgiCapturer, error) {
 		height: h,
 	}
 
-	// Best effort: an idle desktop legitimately has no new frame to give, and
-	// waiting here for one would only delay a capturer that is otherwise ready.
-	// capture() declines to hand out img until a frame has actually arrived.
-	if err := c.dup.GetImage(c.img, firstFrameTimeoutMS); err == nil {
+	// An idle desktop legitimately has no new frame to give, and waiting here
+	// for one would only delay a capturer that is otherwise ready: capture()
+	// declines to hand out img until a frame has actually arrived. Any other
+	// failure is DXGI itself being unusable in this session, and has to reach
+	// createCapturer so it falls back to GDI instead of rebuilding a duplication
+	// that will fail the same way on every request.
+	err = c.dup.GetImage(c.img, firstFrameTimeoutMS)
+	switch {
+	case err == nil:
 		c.gotFrame = true
-	} else if !errors.Is(err, outputduplication.ErrNoImageYet) {
-		log.Debugf("first DXGI frame: %v", err)
+	case !errors.Is(err, outputduplication.ErrNoImageYet):
+		c.close()
+		return nil, fmt.Errorf("acquire first desktop frame: %w", err)
 	}
 
 	return c, nil
@@ -93,10 +97,11 @@ func (c *dxgiCapturer) capture() (*image.RGBA, error) {
 		// "No new frame" is the right answer on an idle desktop, but only once
 		// there is a frame to repeat. Before that img is all zeroes, and
 		// returning it would serve a black desktop that looks like a successful
-		// capture. Reported as an error instead: the worker retries shortly and
-		// stays on DXGI, where failing the constructor would have dropped the
-		// whole session to the much slower GDI path.
-		return nil, err
+		// capture. Reported as errFrameNotReady instead, which the worker
+		// answers without tearing the capturer down: rebuilding a D3D11 device
+		// and duplication on every request would never converge on a desktop
+		// that is idle precisely because nobody is touching it.
+		return nil, fmt.Errorf("%w: %w", errFrameNotReady, err)
 	}
 
 	// Copy into the next output buffer. The DesktopCapturer hands out the

client/vnc/server/capture_windows.go

@@ -3,6 +3,7 @@
 package server
 
 import (
+	"errors"
 	"fmt"
 	"image"
 	"runtime"
@@ -435,6 +436,11 @@ type frameCapturer interface {
 	close()
 }
 
+// errFrameNotReady is the capture error that means "healthy, but nothing to
+// hand out yet". The worker answers the request with it and keeps the
+// capturer, where any other error tears the capturer down and rebuilds it.
+var errFrameNotReady = errors.New("no frame captured yet")
+
 // captureWorker owns the worker goroutine's mutable state. Extracted into a
 // struct so the request/desktop/init logic can live on small methods and the
 // outer worker() stays a thin loop.
@@ -475,6 +481,11 @@ func (w *captureWorker) serveRequest(req captureReq) {
 		return
 	}
 	img, err := fc.capture()
+	if errors.Is(err, errFrameNotReady) {
+		log.Tracef("capture: %v", err)
+		req.reply <- captureReply{err: err}
+		return
+	}
 	if err != nil {
 		log.Debugf("capture: %v", err)
 		w.closeCapturer()

client/vnc/server/capture_x11.go

@@ -5,6 +5,7 @@ package server
 import (
 	"fmt"
 	"image"
+	"math"
 	"os"
 	"os/exec"
 	"strconv"
@@ -73,12 +74,33 @@ func detectX11Display() {
 	}
 }
 
-// detectX11FromProc scans /proc/*/cmdline for Xorg (Linux).
+// xorgCandidate is one X server found in /proc, with the pieces that decide
+// whether it is the one a remote user should be attached to.
+type xorgCandidate struct {
+	display string
+	auth    string
+	// vt is the virtual terminal the server was started on, or -1 when it
+	// records none (Xvfb, Xwayland, a nested server).
+	vt int
+}
+
+// detectX11FromProc scans /proc/*/cmdline for X servers (Linux) and attaches to
+// the one on the active virtual terminal.
+//
+// A host can be running several at once: multi-seat, a fast-user-switch that
+// left the previous session's server up, an Xvfb next to the real one. Taking
+// whichever /proc entry readdir happened to return first would attach the VNC
+// session to an arbitrary one of those, showing that user's screen and
+// delivering the remote user's input to it. /sys/class/tty/tty0/active names the
+// session actually on the console and X records its own VT in argv, so the two
+// can be matched.
 func detectX11FromProc() bool {
 	entries, err := os.ReadDir("/proc")
 	if err != nil {
 		return false
 	}
+
+	var candidates []xorgCandidate
 	for _, e := range entries {
 		if !e.IsDir() {
 			continue
@@ -87,12 +109,91 @@ func detectX11FromProc() bool {
 		if err != nil {
 			continue
 		}
-		if display, auth := parseXorgArgs(splitCmdline(cmdline)); display != "" {
-			setDisplayEnv(display, auth)
-			return true
+		args := splitCmdline(cmdline)
+		display, auth := parseXorgArgs(args)
+		if display == "" {
+			continue
+		}
+		candidates = append(candidates, xorgCandidate{display: display, auth: auth, vt: parseXorgVT(args)})
+	}
+
+	best, ok := pickXorgCandidate(candidates, activeVT())
+	if !ok {
+		return false
+	}
+	setDisplayEnv(best.display, best.auth)
+	return true
+}
+
+// pickXorgCandidate chooses which X server to attach to: the one on the active
+// VT when that is known, and otherwise the lowest display number, so the choice
+// is at least stable across runs instead of following readdir order.
+func pickXorgCandidate(candidates []xorgCandidate, activeVT int) (xorgCandidate, bool) {
+	if len(candidates) == 0 {
+		return xorgCandidate{}, false
+	}
+	if activeVT > 0 {
+		for _, c := range candidates {
+			if c.vt == activeVT {
+				return c, true
+			}
 		}
 	}
-	return false
+
+	best := candidates[0]
+	for _, c := range candidates[1:] {
+		if displayNumber(c.display) < displayNumber(best.display) {
+			best = c
+		}
+	}
+	if len(candidates) > 1 {
+		log.Warnf("found %d X servers and none on the active VT (%d); attaching to DISPLAY=%s",
+			len(candidates), activeVT, best.display)
+	}
+	return best, true
+}
+
+// activeVT reports the virtual terminal currently on the console, or -1 when
+// that cannot be read (no sysfs, a seat with no VT, FreeBSD).
+func activeVT() int {
+	data, err := os.ReadFile("/sys/class/tty/tty0/active")
+	if err != nil {
+		return -1
+	}
+	n, err := strconv.Atoi(strings.TrimPrefix(strings.TrimSpace(string(data)), "tty"))
+	if err != nil {
+		return -1
+	}
+	return n
+}
+
+// parseXorgVT extracts the VT from an X server's argv, which spells it as a
+// bare "vt7" token. Returns -1 when the server records none.
+func parseXorgVT(args []string) int {
+	for _, arg := range args {
+		if !strings.HasPrefix(arg, "vt") {
+			continue
+		}
+		if n, err := strconv.Atoi(arg[len("vt"):]); err == nil {
+			return n
+		}
+	}
+	return -1
+}
+
+// displayNumber extracts the screen-less display number from a DISPLAY value
+// (":1" or ":1.0"). An unparseable value sorts last so it is only picked when
+// nothing else is on offer.
+func displayNumber(display string) int {
+	s := strings.TrimPrefix(display, ":")
+	if dot := strings.IndexByte(s, '.'); dot >= 0 {
+		s = s[:dot]
+	}
+	n, err := strconv.Atoi(s)
+	if err != nil {
+		return math.MaxInt
+	}
+	return n
 }
 
 // detectX11FromSockets checks /tmp/.X11-unix/ for X sockets and uses ps

client/vnc/server/capture_x11_select_test.go

@@ -0,0 +1,94 @@
+//go:build (linux && !android) || freebsd
+
+package server
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestPickXorgCandidate covers which X server a service-mode capturer attaches
+// to when several are running. Picking the wrong one shows a remote user
+// another local user's screen, so the active VT wins whenever it is known.
+func TestPickXorgCandidate(t *testing.T) {
+	tests := []struct {
+		name       string
+		candidates []xorgCandidate
+		activeVT   int
+		want       string
+		wantOK     bool
+	}{
+		{
+			name:   "no X server found",
+			wantOK: false,
+		},
+		{
+			name:       "single server is used whatever its VT",
+			candidates: []xorgCandidate{{display: ":3", vt: 9}},
+			activeVT:   2,
+			want:       ":3",
+			wantOK:     true,
+		},
+		{
+			name: "active VT wins over the lower display number",
+			candidates: []xorgCandidate{
+				{display: ":0", vt: 2},
+				{display: ":1", vt: 7},
+			},
+			activeVT: 7,
+			want:     ":1",
+			wantOK:   true,
+		},
+		{
+			name: "unknown active VT falls back to the lowest display",
+			candidates: []xorgCandidate{
+				{display: ":1", vt: 7},
+				{display: ":0", vt: 2},
+			},
+			activeVT: -1,
+			want:     ":0",
+			wantOK:   true,
+		},
+		{
+			name: "display numbers compare numerically, not lexically",
+			candidates: []xorgCandidate{
+				{display: ":10", vt: -1},
+				{display: ":2", vt: -1},
+			},
+			activeVT: -1,
+			want:     ":2",
+			wantOK:   true,
+		},
+		{
+			name: "an unparseable display is only picked when it is alone",
+			candidates: []xorgCandidate{
+				{display: ":bogus", vt: -1},
+				{display: ":4", vt: -1},
+			},
+			activeVT: -1,
+			want:     ":4",
+			wantOK:   true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got, ok := pickXorgCandidate(tc.candidates, tc.activeVT)
+			require.Equal(t, tc.wantOK, ok)
+			if !tc.wantOK {
+				return
+			}
+			assert.Equal(t, tc.want, got.display)
+		})
+	}
+}
+
+// TestParseXorgVT confirms the bare "vt7" token X servers carry in argv is the
+// one read, and that nothing else in the command line is mistaken for it.
+func TestParseXorgVT(t *testing.T) {
+	assert.Equal(t, 7, parseXorgVT([]string{"/usr/lib/Xorg", ":0", "-auth", "/run/x.auth", "vt7", "-novtswitch"}))
+	assert.Equal(t, -1, parseXorgVT([]string{"/usr/bin/Xvfb", ":99", "-screen", "0", "1920x1080x24"}))
+	assert.Equal(t, -1, parseXorgVT([]string{"/usr/lib/Xorg", ":0", "vtconsole"}))
+}

client/vnc/server/server.go

@@ -183,18 +183,23 @@ type Server struct {
 	ctx            context.Context
 	cancel         context.CancelFunc
 	vmgr           virtualSessionManager
-	// handlers counts the in-flight connection handlers so Stop can wait for
-	// them before tearing down the capturer and injector they use.
+	// handlerCount counts the in-flight connection handlers so Stop can wait
+	// for them before tearing down the capturer and injector they use, and
+	// stopping records whether admission is closed.
 	//
-	// handlersMu guards both the counter's growth and stopping. A WaitGroup
-	// forbids an Add that starts from zero while a Wait is in flight, and the
-	// accept loops run in their own goroutines, so without a barrier a
-	// connection accepted just before the listener closed could Add after Wait
-	// had already returned. It cannot be s.mu: Stop holds that for its whole
-	// body, so an accept loop taking it would deadlock against the wait.
-	handlersMu sync.Mutex
-	handlers   sync.WaitGroup
-	stopping   bool
+	// handlersMu guards all three fields. It cannot be s.mu: Stop holds that
+	// for its whole body, so an accept loop taking it would deadlock against
+	// the wait. A plain counter rather than a sync.WaitGroup because Stop gives
+	// up after handlerDrainTimeout: a handler that outlives the drain is still
+	// counted when a later Start reopens admission, and a WaitGroup panics when
+	// an Add races the Wait it left behind.
+	handlersMu   sync.Mutex
+	handlerCount int
+	stopping     bool
+	// handlersDrained is closed once handlerCount reaches zero after admission
+	// closed. Recreated by closeAdmission and dropped by Start, so each
+	// stop/start cycle waits on its own signal rather than on a stale one.
+	handlersDrained chan struct{}
 	// onVirtualProcesses forwards live virtual-session process records to the
 	// daemon for crash recovery; nil when nothing is listening.
 	onVirtualProcesses func(*ShutdownState)
@@ -488,26 +493,58 @@ func (s *Server) beginHandler() bool {
 	if s.stopping {
 		return false
 	}
-	s.handlers.Add(1)
+	s.handlerCount++
 	return true
 }
 
+// endHandler retires one in-flight connection handler.
+func (s *Server) endHandler() {
+	s.handlersMu.Lock()
+	defer s.handlersMu.Unlock()
+
+	s.handlerCount--
+	if s.handlerCount == 0 {
+		s.signalDrainedLocked()
+	}
+}
+
+// closeAdmission refuses further handlers and returns the channel that closes
+// once the handlers already in flight have finished. Stop calls it before
+// touching anything else: a connection an accept loop returns mid-shutdown
+// would otherwise miss the closeActiveSessions snapshot and still be admitted,
+// starting a session over a capturer and injector about to be torn down.
+func (s *Server) closeAdmission() <-chan struct{} {
+	s.handlersMu.Lock()
+	defer s.handlersMu.Unlock()
+
+	s.stopping = true
+	if s.handlersDrained == nil {
+		s.handlersDrained = make(chan struct{})
+	}
+	if s.handlerCount == 0 {
+		s.signalDrainedLocked()
+	}
+	return s.handlersDrained
+}
+
+// signalDrainedLocked closes the drain channel at most once. Callers hold
+// handlersMu.
+func (s *Server) signalDrainedLocked() {
+	if s.handlersDrained == nil {
+		return
+	}
+	select {
+	case <-s.handlersDrained:
+	default:
+		close(s.handlersDrained)
+	}
+}
+
 // awaitHandlers waits for the in-flight connection handlers, giving up after
 // handlerDrainTimeout so a stuck one cannot hold shutdown open.
-func (s *Server) awaitHandlers() {
-	// Closes the door before waiting: past this point beginHandler refuses, so
-	// no Add can begin while Wait is running.
-	s.handlersMu.Lock()
-	s.stopping = true
-	s.handlersMu.Unlock()
-
-	done := make(chan struct{})
-	go func() {
-		s.handlers.Wait()
-		close(done)
-	}()
+func (s *Server) awaitHandlers(drained <-chan struct{}) {
 	select {
-	case <-done:
+	case <-drained:
 	case <-time.After(handlerDrainTimeout):
 		s.log.Warnf("timed out after %s waiting for VNC connection handlers to finish", handlerDrainTimeout)
 	}
@@ -689,9 +726,11 @@ func (s *Server) Start(ctx context.Context, addr netip.AddrPort, network netip.P
 	}
 
 	// Reopen the door a previous Stop closed, so a restarted server accepts
-	// handlers again.
+	// handlers again, and drop that Stop's drain signal so the next one waits
+	// on a fresh channel rather than on one already closed.
 	s.handlersMu.Lock()
 	s.stopping = false
+	s.handlersDrained = nil
 	s.handlersMu.Unlock()
 
 	s.ctx, s.cancel = context.WithCancel(ctx)
@@ -784,6 +823,10 @@ func (s *Server) Stop() error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
+	// Before anything is closed, so no connection is admitted into a teardown
+	// already in progress.
+	drained := s.closeAdmission()
+
 	if s.cancel != nil {
 		s.cancel()
 		s.cancel = nil
@@ -821,7 +864,7 @@ func (s *Server) Stop() error {
 	// session does is release the modifiers and buttons the client left held:
 	// closing the injector first would drop those and leave the host with a
 	// stuck Shift or mouse button.
-	s.awaitHandlers()
+	s.awaitHandlers(drained)
 
 	if c, ok := s.capturer.(interface{ Close() }); ok {
 		c.Close()
@@ -877,7 +920,7 @@ func (s *Server) acceptLoop(ln net.Listener) {
 			continue
 		}
 		go func(c net.Conn) {
-			defer s.handlers.Done()
+			defer s.endHandler()
 			defer s.releaseConnSlot()
 			defer s.untrackConn(c)
 			s.handleConnection(c)

client/vnc/server/server_darwin.go

@@ -65,11 +65,13 @@ func (s *Server) serviceAcceptLoop(ln net.Listener) {
 		if !s.beginHandler() {
 			s.releaseConnSlot()
 			s.untrackConn(metered)
-			_ = conn.Close()
+			// The wrapper, not the raw conn: newMetricsConn has already
+			// started a sampling goroutine that only its own Close stops.
+			_ = metered.Close()
 			continue
 		}
 		go func(c net.Conn) {
-			defer s.handlers.Done()
+			defer s.endHandler()
 			defer s.releaseConnSlot()
 			defer s.untrackConn(c)
 			s.handleServiceConnection(c, mgr)

client/vnc/server/server_test.go

@@ -577,3 +577,70 @@ func TestGateApproval_PassesPubKeyHex(t *testing.T) {
 	assert.True(t, allowed)
 	assert.Equal(t, hex.EncodeToString(pub), app.lastIn.PeerPubKey)
 }
+
+// TestCloseAdmission_RefusesAndDrains covers the shutdown barrier: once
+// admission is closed no further handler may start, and the drain signal fires
+// only after the ones already counted have finished.
+func TestCloseAdmission_RefusesAndDrains(t *testing.T) {
+	srv := &Server{log: log.WithField("test", t.Name())}
+
+	require.True(t, srv.beginHandler(), "a fresh server must admit handlers")
+	require.True(t, srv.beginHandler())
+
+	drained := srv.closeAdmission()
+	assert.False(t, srv.beginHandler(), "admission must be closed after closeAdmission")
+
+	srv.endHandler()
+	select {
+	case <-drained:
+		t.Fatal("drained before the last handler finished")
+	default:
+	}
+
+	srv.endHandler()
+	select {
+	case <-drained:
+	case <-time.After(time.Second):
+		t.Fatal("drain signal never fired")
+	}
+}
+
+// TestCloseAdmission_HandlerOutlivingDrain reproduces the restart hazard: Stop
+// gives up after handlerDrainTimeout, so a handler can still be running when a
+// later Start reopens admission. The counter must survive that (a sync.WaitGroup
+// panics on an Add racing the Wait it left behind), and the next Stop must wait
+// for both the straggler and the handlers admitted after the restart.
+func TestCloseAdmission_HandlerOutlivingDrain(t *testing.T) {
+	srv := &Server{log: log.WithField("test", t.Name())}
+
+	require.True(t, srv.beginHandler())
+	firstDrain := srv.closeAdmission()
+	select {
+	case <-firstDrain:
+		t.Fatal("drained while a handler was still in flight")
+	default:
+	}
+
+	// What Start does after a drain that timed out.
+	srv.handlersMu.Lock()
+	srv.stopping = false
+	srv.handlersDrained = nil
+	srv.handlersMu.Unlock()
+
+	require.True(t, srv.beginHandler(), "a restarted server must admit handlers again")
+
+	secondDrain := srv.closeAdmission()
+	srv.endHandler() // the straggler from before the restart
+	select {
+	case <-secondDrain:
+		t.Fatal("drained before the post-restart handler finished")
+	default:
+	}
+
+	srv.endHandler()
+	select {
+	case <-secondDrain:
+	case <-time.After(time.Second):
+		t.Fatal("drain signal never fired after restart")
+	}
+}

client/vnc/server/server_windows.go

@@ -352,11 +352,13 @@ func (s *Server) serviceAcceptLoop(ln net.Listener) {
 		if !s.beginHandler() {
 			s.releaseConnSlot()
 			s.untrackConn(metered)
-			_ = conn.Close()
+			// The wrapper, not the raw conn: newMetricsConn has already
+			// started a sampling goroutine that only its own Close stops.
+			_ = metered.Close()
 			continue
 		}
 		go func(c net.Conn) {
-			defer s.handlers.Done()
+			defer s.endHandler()
 			defer s.releaseConnSlot()
 			defer s.untrackConn(c)
 			s.handleServiceConnection(c, sm)

client/vnc/server/session.go

@@ -123,10 +123,15 @@ type session struct {
 	// compositing falls back to a no-op (capturer cannot supply a sprite
 	// or position). One line per session is enough to point at the cause.
 	cursorWarnOnce sync.Once
-	// cursorSkipOnce does the same for the Cursor pseudo-encoding path, which
-	// has several ways to decline to send a rect and used to take all of them
-	// silently.
-	cursorSkipOnce sync.Once
+	// cursorSkipMu guards cursorSkipSeen.
+	cursorSkipMu sync.Mutex
+	// cursorSkipSeen holds the cursor-skip diagnostics already emitted, so the
+	// Cursor pseudo-encoding path says each distinct reason once instead of
+	// taking all of them silently. Keyed by the formatted line rather than
+	// throttled once per session: a client can negotiate the encoding
+	// mid-session and the cursor source can start failing later, and the reason
+	// that happens to come first must not swallow the ones that follow.
+	cursorSkipSeen map[string]struct{}
 	// clientJPEGQuality and clientZlibLevel hold the 0..9 levels the client
 	// advertised via the QualityLevel / CompressLevel pseudo-encodings, or
 	// -1 when the client has not expressed a preference. Applied to the

client/vnc/server/session_cursor.go

@@ -4,6 +4,7 @@ package server
 
 import (
 	"encoding/binary"
+	"fmt"
 	"image"
 )
 
@@ -29,17 +30,13 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte {
 	// cannot produce one, or the sprite failed to encode. Say which, once per
 	// session, so the next report of a missing cursor names its own cause.
 	if !supported || failed || composite {
-		s.cursorSkipOnce.Do(func() {
-			s.log.Debugf("no cursor rect: client_requested=%v source_failed=%v compositing=%v",
-				supported, failed, composite)
-		})
+		s.logCursorSkip("no cursor rect: client_requested=%v source_failed=%v compositing=%v",
+			supported, failed, composite)
 		return nil
 	}
 	src, ok := s.capturer.(cursorSource)
 	if !ok {
-		s.cursorSkipOnce.Do(func() {
-			s.log.Debugf("no cursor rect: capturer %T reports no cursor source", s.capturer)
-		})
+		s.logCursorSkip("no cursor rect: capturer %T reports no cursor source", s.capturer)
 		return nil
 	}
 	img, hotX, hotY, serial, err := src.Cursor()
@@ -51,9 +48,7 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte {
 		return nil
 	}
 	if img == nil {
-		s.cursorSkipOnce.Do(func() {
-			s.log.Debug("no cursor rect: capturer returned no sprite")
-		})
+		s.logCursorSkip("no cursor rect: capturer returned no sprite")
 		return nil
 	}
 	if serial == lastSerial {
@@ -61,11 +56,9 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte {
 	}
 	buf := encodeCursorPseudoRect(img, hotX, hotY, pf)
 	if buf == nil {
-		s.cursorSkipOnce.Do(func() {
-			b := img.Bounds()
-			s.log.Debugf("no cursor rect: sprite %dx%d stride=%d pix=%d could not be encoded",
-				b.Dx(), b.Dy(), img.Stride, len(img.Pix))
-		})
+		b := img.Bounds()
+		s.logCursorSkip("no cursor rect: sprite %dx%d stride=%d pix=%d could not be encoded",
+			b.Dx(), b.Dy(), img.Stride, len(img.Pix))
 		return nil
 	}
 	// Re-check under the write lock so a cursor another goroutine already
@@ -89,6 +82,28 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte {
 	return buf
 }
 
+// logCursorSkip reports why this update carries no cursor rect, once per
+// distinct line. pendingCursorRect runs per framebuffer update, so an
+// unthrottled log would flood; throttling once per session instead would let
+// whichever reason came first hide every later one, and the reasons do change
+// as the client negotiates encodings and the cursor source starts failing.
+func (s *session) logCursorSkip(format string, args ...any) {
+	msg := fmt.Sprintf(format, args...)
+
+	s.cursorSkipMu.Lock()
+	if s.cursorSkipSeen == nil {
+		s.cursorSkipSeen = make(map[string]struct{})
+	}
+	_, seen := s.cursorSkipSeen[msg]
+	s.cursorSkipSeen[msg] = struct{}{}
+	s.cursorSkipMu.Unlock()
+
+	if seen {
+		return
+	}
+	s.log.Debug(msg)
+}
+
 // maxCursorDim caps the cursor sprite size we'll encode. Real platform
 // cursors are tiny (<=256×256 on every supported OS); a value past this
 // almost certainly indicates a corrupted platform-API response, and

client/vnc/server/shutdown_state_freebsd.go

@@ -0,0 +1,14 @@
+//go:build freebsd
+
+package server
+
+// describeProcess records nothing: FreeBSD runs virtual sessions but mounts no
+// procfs by default, so there is no start time to pin an identity to, and
+// without a way to tell a reused PID from the original a record would only
+// invite an unsafe kill later.
+//
+// FreeBSD-only rather than shared with the other non-Linux platforms: Windows
+// and macOS have no virtual sessions to describe, so a stub there is dead code.
+func describeProcess(_ int) sessionProcess {
+	return sessionProcess{}
+}

client/vnc/server/shutdown_state_other.go

@@ -41,9 +41,3 @@ func (s *ShutdownState) Cleanup() error {
 // sessionProcess is the placeholder identity these platforms record, which is
 // to say none.
 type sessionProcess struct{}
-
-// describeProcess records nothing: without a way to tell a reused PID from the
-// original, a record would only invite an unsafe kill later.
-func describeProcess(_ int) sessionProcess {
-	return sessionProcess{}
-}