Refuse an ambiguous X display and settle the approval timeout race under one claim

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

client/internal/approval/broker.go

@@ -128,19 +128,27 @@ type Decision struct {
 	ViewOnly bool
 }
 
+// pendingRequest is one in-flight prompt. resolved records that somebody has
+// already claimed it, so the user's decision and the caller giving up race for
+// the same entry under Broker.mu and exactly one of them wins.
+type pendingRequest struct {
+	resp     chan Decision
+	resolved bool
+}
+
 // Broker holds in-flight approval requests keyed by request ID.
 type Broker struct {
 	pub EventPublisher
 
 	mu      sync.Mutex
-	pending map[string]chan Decision
+	pending map[string]*pendingRequest
 }
 
 // New returns a broker that publishes prompts via pub.
 func New(pub EventPublisher) *Broker {
 	return &Broker{
 		pub:     pub,
-		pending: make(map[string]chan Decision),
+		pending: make(map[string]*pendingRequest),
 	}
 }
 
@@ -161,7 +169,7 @@ func (b *Broker) Request(ctx context.Context, p Prompt) (Decision, error) {
 	resp := make(chan Decision, 1)
 
 	b.mu.Lock()
-	b.pending[id] = resp
+	b.pending[id] = &pendingRequest{resp: resp}
 	b.mu.Unlock()
 
 	defer b.dropPending(id)
@@ -195,17 +203,61 @@ func (b *Broker) Request(ctx context.Context, p Prompt) (Decision, error) {
 
 	select {
 	case d := <-resp:
-		if !d.Accept {
-			return zero, ErrDenied
-		}
-		return d, nil
+		return decisionResult(d)
 	case <-timer.C:
+		if d, answered := b.giveUp(id, resp); answered {
+			return decisionResult(d)
+		}
 		return zero, ErrTimeout
 	case <-ctx.Done():
+		if d, answered := b.giveUp(id, resp); answered {
+			return decisionResult(d)
+		}
 		return zero, ctx.Err()
 	}
 }
 
+// giveUp abandons the request and reports whether the user's decision landed
+// first, in which case it is returned and must be honoured.
+//
+// Respond and this path claim the same entry under the same lock, so exactly
+// one of them wins. Without that claim, a click arriving as the timer fires
+// would be told it matched a live prompt while this caller had already denied
+// the connection: the user would see their accept confirmed and the session
+// dropped anyway.
+func (b *Broker) giveUp(id string, resp <-chan Decision) (Decision, bool) {
+	if b.claim(id) {
+		return Decision{}, false
+	}
+	// Respond claimed the entry and sent while still holding the lock, so the
+	// decision is already buffered and this receive cannot block.
+	return <-resp, true
+}
+
+// claim marks the request resolved so nothing else can take it, reporting
+// whether the caller got there first. Callers must not already hold b.mu.
+func (b *Broker) claim(id string) bool {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+
+	p, ok := b.pending[id]
+	if !ok || p.resolved {
+		return false
+	}
+	p.resolved = true
+	delete(b.pending, id)
+	return true
+}
+
+// decisionResult maps a decision the user actually made onto the Request
+// contract: a deny is an error, an accept carries the view-only flag back.
+func decisionResult(d Decision) (Decision, error) {
+	if !d.Accept {
+		return Decision{}, ErrDenied
+	}
+	return d, nil
+}
+
 // Respond delivers the user's decision for id. Returns true when a pending
 // request matched and was woken, false when id was unknown or already done.
 func (b *Broker) Respond(id string, d Decision) bool {
@@ -213,18 +265,18 @@ func (b *Broker) Respond(id string, d Decision) bool {
 		return false
 	}
 	b.mu.Lock()
-	ch, ok := b.pending[id]
-	if ok {
-		delete(b.pending, id)
-	}
-	b.mu.Unlock()
-	if !ok {
+	defer b.mu.Unlock()
+
+	p, ok := b.pending[id]
+	if !ok || p.resolved {
 		return false
 	}
-	select {
-	case ch <- d:
-	default:
-	}
+	p.resolved = true
+	delete(b.pending, id)
+	// The channel is buffered and claimed exactly once, so this never blocks.
+	// Sent under the lock so a Request that loses the claim race finds the
+	// decision already waiting instead of racing this send.
+	p.resp <- d
 	return true
 }
 

client/internal/approval/broker_test.go

@@ -458,3 +458,77 @@ func TestRequestViewOnly(t *testing.T) {
 		t.Fatal("view-only request did not resolve")
 	}
 }
+
+// TestGiveUpAndRespondAreMutuallyExclusive pins the claim that makes
+// RespondApprovalResponse.matched truthful: for one request, exactly one of
+// "the user answered" and "the caller gave up" wins, in either arrival order.
+// Without it a click landing as the timer fires is told it matched a live
+// prompt while the connection has already been denied.
+func TestGiveUpAndRespondAreMutuallyExclusive(t *testing.T) {
+	t.Run("caller gives up first", func(t *testing.T) {
+		b := New(&fakePublisher{subscribers: true})
+		resp := make(chan Decision, 1)
+		b.pending["req"] = &pendingRequest{resp: resp}
+
+		d, answered := b.giveUp("req", resp)
+		assert.False(t, answered, "nothing had answered yet")
+		assert.False(t, d.Accept)
+		assert.False(t, b.Respond("req", Decision{Accept: true}),
+			"a response arriving after the caller gave up must not report a match")
+	})
+
+	t.Run("user answers first", func(t *testing.T) {
+		b := New(&fakePublisher{subscribers: true})
+		resp := make(chan Decision, 1)
+		b.pending["req"] = &pendingRequest{resp: resp}
+
+		require.True(t, b.Respond("req", Decision{Accept: true, ViewOnly: true}))
+
+		d, answered := b.giveUp("req", resp)
+		require.True(t, answered, "giving up after the user answered must surface their decision")
+		assert.True(t, d.Accept)
+		assert.True(t, d.ViewOnly, "the view-only grant must survive the race")
+	})
+}
+
+// TestRespondRacingTimeoutIsConsistent aims a Respond at the deadline and
+// requires the reported match to agree with the outcome the caller saw. A
+// stress check on the invariant above rather than a reproduction: the losing
+// window is a few instructions wide, so this does not reliably fail without the
+// claim, but it does catch an outcome pair that should never occur.
+func TestRespondRacingTimeoutIsConsistent(t *testing.T) {
+	defaultTimeout(t, 20*time.Millisecond)
+	defer defaultTimeout(t, DefaultTimeout)
+
+	for i := 0; i < 50; i++ {
+		pub := &fakePublisher{subscribers: true}
+		b := New(pub)
+
+		done := make(chan error, 1)
+		go func() {
+			done <- requestErr(b, context.Background(), Prompt{Kind: KindVNC})
+		}()
+		id := waitForRequestID(t, pub)
+
+		matchedCh := make(chan bool, 1)
+		go func() {
+			// Aim at the deadline so the claim lands on either side of it.
+			time.Sleep(20 * time.Millisecond)
+			matchedCh <- b.Respond(id, Decision{Accept: true})
+		}()
+
+		var err error
+		select {
+		case err = <-done:
+		case <-time.After(2 * time.Second):
+			t.Fatal("prompt neither resolved nor timed out")
+		}
+		matched := <-matchedCh
+
+		if matched {
+			require.NoErrorf(t, err, "iteration %d: Respond reported the prompt matched, so the accept must be honoured", i)
+			continue
+		}
+		require.ErrorIsf(t, err, ErrTimeout, "iteration %d: Respond reported no match, so the request must have timed out", i)
+	}
+}

client/proto/daemon.pb.go

@@ -7271,9 +7271,10 @@ func (x *RespondApprovalRequest) GetViewOnly() bool {
 type RespondApprovalResponse struct {
 	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.
+	// False covers three cases and does not distinguish them: request_id was
+	// never known, the prompt had already been answered, or it had expired and
+	// the connection was denied. In all three this call changed nothing, so the
+	// UI must not report the outcome the user picked as having taken effect.
 	Matched       bool `protobuf:"varint,1,opt,name=matched,proto3" json:"matched,omitempty"`
 	unknownFields protoimpl.UnknownFields
 	sizeCache     protoimpl.SizeCache

client/proto/daemon.proto

@@ -1127,8 +1127,9 @@ message RespondApprovalRequest {
 
 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.
+  // False covers three cases and does not distinguish them: request_id was
+  // never known, the prompt had already been answered, or it had expired and
+  // the connection was denied. In all three this call changed nothing, so the
+  // UI must not report the outcome the user picked as having taken effect.
   bool matched = 1;
 }

client/ui/services/approval.go

@@ -38,11 +38,12 @@ func (a *Approval) Respond(ctx context.Context, requestID string, accept, viewOn
 		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)
+		// Not an error to the caller: the dialog closes either way. The daemon
+		// does not say which of "already answered", "expired" or "never known"
+		// applies, so neither does this line; it exists so a report of "I
+		// clicked and nothing happened" has a record that the click reached a
+		// prompt that was no longer waiting.
+		log.Infof("approval %s was no longer pending; this response changed nothing", requestID)
 	}
 	return nil
 }

client/vnc/server/capture_x11.go

@@ -65,15 +65,32 @@ func detectX11Display() {
 		return
 	}
 
-	// Try /proc first (Linux), then ps fallback (FreeBSD and others).
-	if detectX11FromProc() {
+	// Try /proc first (Linux), then the socket scan (FreeBSD and others).
+	switch detectX11FromProc() {
+	case x11Detected:
 		return
-	}
-	if detectX11FromSockets() {
+	case x11Ambiguous:
+		// /proc listed X servers and none of them could be shown to be the one
+		// on the console. Falling through to the socket scan would only make
+		// the same guess with less information, so stop here: no capture at all
+		// beats capturing a session that may not be the caller's.
 		return
+	case x11NotFound:
 	}
+	detectX11FromSockets()
 }
 
+// x11Detection is the outcome of one display-detection pass. Ambiguous is
+// deliberately distinct from NotFound: it means candidates existed and were
+// refused, which must not be retried by a less discriminating fallback.
+type x11Detection int
+
+const (
+	x11NotFound x11Detection = iota
+	x11Detected
+	x11Ambiguous
+)
+
 // 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 {
@@ -94,10 +111,10 @@ type xorgCandidate struct {
 // 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 {
+func detectX11FromProc() x11Detection {
 	entries, err := os.ReadDir("/proc")
 	if err != nil {
-		return false
+		return x11NotFound
 	}
 
 	var candidates []xorgCandidate
@@ -117,40 +134,69 @@ func detectX11FromProc() bool {
 		candidates = append(candidates, xorgCandidate{display: display, auth: auth, vt: parseXorgVT(args)})
 	}
 
-	best, ok := pickXorgCandidate(candidates, activeVT())
-	if !ok {
-		return false
+	best, outcome := pickXorgCandidate(candidates, activeVT())
+	if outcome != x11Detected {
+		return outcome
 	}
 	setDisplayEnv(best.display, best.auth)
-	return true
+	return x11Detected
 }
 
-// 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) {
+// pickXorgCandidate chooses which X server to attach to.
+//
+// The one on the active VT wins outright. Failing that the choice has to be
+// unambiguous, because guessing wrong means handing a remote user another local
+// user's screen and keyboard: a single candidate is taken, and anything else is
+// refused. The lowest display number only breaks ties among servers that all
+// record no VT, where there is no active-session signal to go on at all.
+func pickXorgCandidate(candidates []xorgCandidate, activeVT int) (xorgCandidate, x11Detection) {
 	if len(candidates) == 0 {
-		return xorgCandidate{}, false
+		return xorgCandidate{}, x11NotFound
 	}
 	if activeVT > 0 {
 		for _, c := range candidates {
 			if c.vt == activeVT {
-				return c, true
+				return c, x11Detected
 			}
 		}
 	}
+	if len(candidates) == 1 {
+		return candidates[0], x11Detected
+	}
 
-	best := candidates[0]
-	for _, c := range candidates[1:] {
+	// Several servers and no way to tell which is on the console. One that
+	// records a VT is on some seat other than the active one, so it is a
+	// session this connection has no business showing.
+	var vtless []xorgCandidate
+	for _, c := range candidates {
+		if c.vt < 0 {
+			vtless = append(vtless, c)
+		}
+	}
+	if len(vtless) == 0 {
+		log.Warnf("found %d X servers, all on virtual terminals and none of them the active one (%s); not attaching to any",
+			len(candidates), describeActiveVT(activeVT))
+		return xorgCandidate{}, x11Ambiguous
+	}
+
+	best := vtless[0]
+	for _, c := range vtless[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)
+	log.Warnf("found %d X servers and none on the active VT (%s); attaching to the lowest VT-less display %s",
+		len(candidates), describeActiveVT(activeVT), best.display)
+	return best, x11Detected
+}
+
+// describeActiveVT renders the active VT for a log line, keeping "we could not
+// read it" distinct from a VT number that simply matched nothing.
+func describeActiveVT(vt int) string {
+	if vt <= 0 {
+		return "unknown"
 	}
-	return best, true
+	return "tty" + strconv.Itoa(vt)
 }
 
 // activeVT reports the virtual terminal currently on the console, or -1 when

client/vnc/server/capture_x11_select_test.go

@@ -11,25 +11,26 @@ import (
 
 // 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.
+// another local user's screen and delivers their input to it, so the active VT
+// wins whenever it is known and an unresolvable choice is refused outright.
 func TestPickXorgCandidate(t *testing.T) {
 	tests := []struct {
-		name       string
-		candidates []xorgCandidate
-		activeVT   int
-		want       string
-		wantOK     bool
+		name        string
+		candidates  []xorgCandidate
+		activeVT    int
+		want        string
+		wantOutcome x11Detection
 	}{
 		{
-			name:   "no X server found",
-			wantOK: false,
+			name:        "no X server found",
+			wantOutcome: x11NotFound,
 		},
 		{
-			name:       "single server is used whatever its VT",
-			candidates: []xorgCandidate{{display: ":3", vt: 9}},
-			activeVT:   2,
-			want:       ":3",
-			wantOK:     true,
+			name:        "single server is used whatever its VT",
+			candidates:  []xorgCandidate{{display: ":3", vt: 9}},
+			activeVT:    2,
+			want:        ":3",
+			wantOutcome: x11Detected,
 		},
 		{
 			name: "active VT wins over the lower display number",
@@ -37,19 +38,37 @@ func TestPickXorgCandidate(t *testing.T) {
 				{display: ":0", vt: 2},
 				{display: ":1", vt: 7},
 			},
-			activeVT: 7,
-			want:     ":1",
-			wantOK:   true,
+			activeVT:    7,
+			want:        ":1",
+			wantOutcome: x11Detected,
 		},
 		{
-			name: "unknown active VT falls back to the lowest display",
+			name: "several servers on other VTs are refused, not guessed at",
 			candidates: []xorgCandidate{
-				{display: ":1", vt: 7},
 				{display: ":0", vt: 2},
+				{display: ":1", vt: 3},
 			},
-			activeVT: -1,
-			want:     ":0",
-			wantOK:   true,
+			activeVT:    7,
+			wantOutcome: x11Ambiguous,
+		},
+		{
+			name: "unknown active VT does not license a guess between seats",
+			candidates: []xorgCandidate{
+				{display: ":0", vt: 2},
+				{display: ":1", vt: 3},
+			},
+			activeVT:    -1,
+			wantOutcome: x11Ambiguous,
+		},
+		{
+			name: "a VT-less server is preferred over one on an inactive VT",
+			candidates: []xorgCandidate{
+				{display: ":0", vt: 2},
+				{display: ":99", vt: -1},
+			},
+			activeVT:    7,
+			want:        ":99",
+			wantOutcome: x11Detected,
 		},
 		{
 			name: "display numbers compare numerically, not lexically",
@@ -57,9 +76,9 @@ func TestPickXorgCandidate(t *testing.T) {
 				{display: ":10", vt: -1},
 				{display: ":2", vt: -1},
 			},
-			activeVT: -1,
-			want:     ":2",
-			wantOK:   true,
+			activeVT:    -1,
+			want:        ":2",
+			wantOutcome: x11Detected,
 		},
 		{
 			name: "an unparseable display is only picked when it is alone",
@@ -67,17 +86,18 @@ func TestPickXorgCandidate(t *testing.T) {
 				{display: ":bogus", vt: -1},
 				{display: ":4", vt: -1},
 			},
-			activeVT: -1,
-			want:     ":4",
-			wantOK:   true,
+			activeVT:    -1,
+			want:        ":4",
+			wantOutcome: x11Detected,
 		},
 	}
 
 	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 {
+			got, outcome := pickXorgCandidate(tc.candidates, tc.activeVT)
+			require.Equal(t, tc.wantOutcome, outcome)
+			if tc.wantOutcome != x11Detected {
+				assert.Empty(t, got.display, "a refused choice must not leak a display")
 				return
 			}
 			assert.Equal(t, tc.want, got.display)
@@ -92,3 +112,11 @@ func TestParseXorgVT(t *testing.T) {
 	assert.Equal(t, -1, parseXorgVT([]string{"/usr/bin/Xvfb", ":99", "-screen", "0", "1920x1080x24"}))
 	assert.Equal(t, -1, parseXorgVT([]string{"/usr/lib/Xorg", ":0", "vtconsole"}))
 }
+
+// TestDescribeActiveVT keeps "we could not read the active VT" legible in the
+// log line, rather than printing a -1 that reads like a real terminal.
+func TestDescribeActiveVT(t *testing.T) {
+	assert.Equal(t, "unknown", describeActiveVT(-1))
+	assert.Equal(t, "unknown", describeActiveVT(0))
+	assert.Equal(t, "tty7", describeActiveVT(7))
+}

client/vnc/server/session_cursor.go

@@ -30,13 +30,17 @@ 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.logCursorSkip("no cursor rect: client_requested=%v source_failed=%v compositing=%v",
+		// The three flags form the key so a client that negotiates the encoding
+		// mid-session, or a source that starts failing, still gets a line: eight
+		// combinations at most.
+		s.logCursorSkip(fmt.Sprintf("state:%t%t%t", supported, failed, composite),
+			"no cursor rect: client_requested=%v source_failed=%v compositing=%v",
 			supported, failed, composite)
 		return nil
 	}
 	src, ok := s.capturer.(cursorSource)
 	if !ok {
-		s.logCursorSkip("no cursor rect: capturer %T reports no cursor source", s.capturer)
+		s.logCursorSkip("no-source", "no cursor rect: capturer %T reports no cursor source", s.capturer)
 		return nil
 	}
 	img, hotX, hotY, serial, err := src.Cursor()
@@ -48,7 +52,7 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte {
 		return nil
 	}
 	if img == nil {
-		s.logCursorSkip("no cursor rect: capturer returned no sprite")
+		s.logCursorSkip("no-sprite", "no cursor rect: capturer returned no sprite")
 		return nil
 	}
 	if serial == lastSerial {
@@ -57,7 +61,10 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte {
 	buf := encodeCursorPseudoRect(img, hotX, hotY, pf)
 	if buf == nil {
 		b := img.Bounds()
-		s.logCursorSkip("no cursor rect: sprite %dx%d stride=%d pix=%d could not be encoded",
+		// Fixed key, varying detail: a source returning junk dimensions must not
+		// be able to mint a new throttle entry per frame.
+		s.logCursorSkip("encode-failed",
+			"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
 	}
@@ -83,25 +90,28 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte {
 }
 
 // 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...)
-
+// reason. 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.
+//
+// reason keys the throttle and must come from a fixed set, never from the
+// message: a cursor source handing back varying junk dimensions would otherwise
+// mint a new key per frame, growing the map for the life of the session and
+// logging every one of them.
+func (s *session) logCursorSkip(reason, format string, args ...any) {
 	s.cursorSkipMu.Lock()
 	if s.cursorSkipSeen == nil {
 		s.cursorSkipSeen = make(map[string]struct{})
 	}
-	_, seen := s.cursorSkipSeen[msg]
-	s.cursorSkipSeen[msg] = struct{}{}
+	_, seen := s.cursorSkipSeen[reason]
+	s.cursorSkipSeen[reason] = struct{}{}
 	s.cursorSkipMu.Unlock()
 
 	if seen {
 		return
 	}
-	s.log.Debug(msg)
+	s.log.Debugf(format, args...)
 }
 
 // maxCursorDim caps the cursor sprite size we'll encode. Real platform