From fecd7cfff5e2b092c77c9df0883f09701d649d98 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Sat, 29 Aug 2026 16:59:05 +0200 Subject: [PATCH] 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 | 22 +++- client/proto/daemon.proto | 11 +- client/server/server.go | 5 +- client/ui/services/approval.go | 16 ++- client/vnc/server/agent_handshake.go | 6 +- client/vnc/server/capture_dxgi_windows.go | 29 +++-- client/vnc/server/capture_windows.go | 11 ++ client/vnc/server/capture_x11.go | 111 ++++++++++++++++++- client/vnc/server/capture_x11_select_test.go | 94 ++++++++++++++++ client/vnc/server/server.go | 99 ++++++++++++----- client/vnc/server/server_darwin.go | 6 +- client/vnc/server/server_test.go | 67 +++++++++++ client/vnc/server/server_windows.go | 6 +- client/vnc/server/session.go | 13 ++- client/vnc/server/session_cursor.go | 45 +++++--- client/vnc/server/shutdown_state_freebsd.go | 14 +++ client/vnc/server/shutdown_state_other.go | 6 - 17 files changed, 476 insertions(+), 85 deletions(-) create mode 100644 client/vnc/server/capture_x11_select_test.go create mode 100644 client/vnc/server/shutdown_state_freebsd.go diff --git a/client/proto/daemon.pb.go b/client/proto/daemon.pb.go index 336d279ac..c60f0e3c6 100644 --- a/client/proto/daemon.pb.go +++ b/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" + diff --git a/client/proto/daemon.proto b/client/proto/daemon.proto index c38ec6547..291721ea5 100644 --- a/client/proto/daemon.proto +++ b/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; +} diff --git a/client/server/server.go b/client/server/server.go index 0649d988c..3e5e77dc4 100644 --- a/client/server/server.go +++ b/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 { diff --git a/client/ui/services/approval.go b/client/ui/services/approval.go index 0dbf648af..05348681a 100644 --- a/client/ui/services/approval.go +++ b/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 } diff --git a/client/vnc/server/agent_handshake.go b/client/vnc/server/agent_handshake.go index f9314798d..bd6374b92 100644 --- a/client/vnc/server/agent_handshake.go +++ b/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 diff --git a/client/vnc/server/capture_dxgi_windows.go b/client/vnc/server/capture_dxgi_windows.go index 994f60549..8f3db15c2 100644 --- a/client/vnc/server/capture_dxgi_windows.go +++ b/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 diff --git a/client/vnc/server/capture_windows.go b/client/vnc/server/capture_windows.go index 11600bded..56881dab2 100644 --- a/client/vnc/server/capture_windows.go +++ b/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() diff --git a/client/vnc/server/capture_x11.go b/client/vnc/server/capture_x11.go index cb28201d7..ac6b287af 100644 --- a/client/vnc/server/capture_x11.go +++ b/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 diff --git a/client/vnc/server/capture_x11_select_test.go b/client/vnc/server/capture_x11_select_test.go new file mode 100644 index 000000000..eba094692 --- /dev/null +++ b/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"})) +} diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index faaaca7d4..b3472e983 100644 --- a/client/vnc/server/server.go +++ b/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) diff --git a/client/vnc/server/server_darwin.go b/client/vnc/server/server_darwin.go index f3254d125..2bb85bf23 100644 --- a/client/vnc/server/server_darwin.go +++ b/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) diff --git a/client/vnc/server/server_test.go b/client/vnc/server/server_test.go index f9f642d9b..4e4bd93a3 100644 --- a/client/vnc/server/server_test.go +++ b/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") + } +} diff --git a/client/vnc/server/server_windows.go b/client/vnc/server/server_windows.go index 28a16533c..fc0a092e0 100644 --- a/client/vnc/server/server_windows.go +++ b/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) diff --git a/client/vnc/server/session.go b/client/vnc/server/session.go index 0578cd878..e34f59a8f 100644 --- a/client/vnc/server/session.go +++ b/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 diff --git a/client/vnc/server/session_cursor.go b/client/vnc/server/session_cursor.go index c592295bf..0971d8673 100644 --- a/client/vnc/server/session_cursor.go +++ b/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 diff --git a/client/vnc/server/shutdown_state_freebsd.go b/client/vnc/server/shutdown_state_freebsd.go new file mode 100644 index 000000000..0b7a84975 --- /dev/null +++ b/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{} +} diff --git a/client/vnc/server/shutdown_state_other.go b/client/vnc/server/shutdown_state_other.go index dbeb2fca5..db3bd4c91 100644 --- a/client/vnc/server/shutdown_state_other.go +++ b/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{} -}