fix: resolve reply-bearing X11 liveness cookies with Reply (#2)

internal/display/liveness_test.go

@@ -0,0 +1,145 @@
+package display
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/BurntSushi/xgb"
+	"github.com/BurntSushi/xgb/xproto"
+)
+
+func TestConfirmMappedSucceedsWhenLivenessReplySucceeds(t *testing.T) {
+	// Open's post-map liveness stage must succeed when the reply-bearing
+	// round trip succeeds, so the mapped window proceeds to a live session.
+	probes := 0
+	probe := func() error {
+		probes++
+		return nil
+	}
+
+	if err := confirmMapped(probe); err != nil {
+		t.Fatalf("confirmMapped=%v want success when the liveness reply arrives", err)
+	}
+	if probes != 1 {
+		t.Fatalf("probes=%d want exactly one liveness round trip", probes)
+	}
+}
+
+func TestConfirmMappedWrapsLivenessFailureForMappedWindowCleanup(t *testing.T) {
+	sentinel := errors.New("connection reset")
+	err := confirmMapped(func() error { return sentinel })
+	if err == nil {
+		t.Fatal("confirmMapped must propagate a failed liveness round trip")
+	}
+	if !strings.Contains(err.Error(), "map kiosk window") {
+		t.Fatalf("error %v must be wrapped as the map-kiosk-window failure so Open closes the connection", err)
+	}
+	if !errors.Is(err, sentinel) {
+		t.Fatalf("error %v must carry the underlying liveness error", err)
+	}
+}
+
+func TestX11SessionWaitEventKeepsPollingWhileLivenessRepliesSucceed(t *testing.T) {
+	probes := 0
+	session := &x11Session{
+		poll: time.Millisecond,
+		pollEvent: func() (xgb.Event, xgb.Error) {
+			return nil, nil
+		},
+		probe: func() error {
+			probes++
+			return nil
+		},
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Millisecond)
+	defer cancel()
+	_, err := session.WaitEvent(ctx)
+	if !errors.Is(err, context.DeadlineExceeded) {
+		t.Fatalf("WaitEvent=%v want clean cancellation while liveness replies succeed", err)
+	}
+	if probes == 0 {
+		t.Fatal("WaitEvent must perform liveness round trips while idle")
+	}
+}
+
+func TestX11SessionWaitEventPropagatesLivenessError(t *testing.T) {
+	sentinel := errors.New("X server went away")
+	session := &x11Session{
+		poll:      time.Millisecond,
+		pollEvent: func() (xgb.Event, xgb.Error) { return nil, nil },
+		probe:     func() error { return sentinel },
+	}
+
+	// The propagated error is what makes Run close the mapped window and
+	// reconnect, keeping the failure contained in the display loop.
+	_, err := session.WaitEvent(context.Background())
+	if !errors.Is(err, sentinel) {
+		t.Fatalf("WaitEvent=%v want the underlying liveness error", err)
+	}
+}
+
+func TestX11SessionWaitEventStillHandlesEventsBeforeLiveness(t *testing.T) {
+	session := &x11Session{
+		poll: time.Millisecond,
+		pollEvent: func() (xgb.Event, xgb.Error) {
+			return xproto.ExposeEvent{}, nil
+		},
+		probe: func() error {
+			t.Fatal("liveness must not run when an event is already handled")
+			return nil
+		},
+	}
+
+	redraw, err := session.WaitEvent(context.Background())
+	if err != nil {
+		t.Fatalf("WaitEvent=%v", err)
+	}
+	if !redraw {
+		t.Fatal("an expose event must request a redraw")
+	}
+}
+
+// TestResolveLivenessWaitsForReplyOnReplyBearingCookie is the behavioral
+// guard for the hardware-proven root cause. GetInputFocus issues a
+// reply-bearing cookie (NewCookie(true, true)); resolveLiveness must
+// consume it with Reply — meaning it blocks awaiting the server's reply —
+// never with Check, which would return the XGB reply-cookie error on every
+// call regardless of server health. A reply-bearing cookie can be built
+// without a server because xgb's NewCookie does not touch the connection.
+func TestResolveLivenessWaitsForReplyOnReplyBearingCookie(t *testing.T) {
+	conn := &xgb.Conn{}
+	cookie := xproto.GetInputFocusCookie{Cookie: conn.NewCookie(true, true)}
+
+	done := make(chan error, 1)
+	go func() { done <- resolveLiveness(cookie) }()
+	select {
+	case err := <-done:
+		t.Fatalf("resolveLiveness returned %v without any server reply; the cookie was not resolved with Reply", err)
+	case <-time.After(100 * time.Millisecond):
+		// Still waiting for the reply: Reply-resolution semantics intact.
+	}
+}
+
+// TestCheckOnReplyBearingCookieIsTheDeviceRootCause pins the exact xgb
+// behavior the fix depends on, using the real cookie API: Check on a
+// reply-bearing cookie returns the device-proven error immediately, while
+// Reply is the only resolution that waits for the server. This is the
+// behavioral contrast proving connLivenessProbe/resolveLiveness cannot
+// reintroduce the open_failed reconnect loop.
+func TestCheckOnReplyBearingCookieIsTheDeviceRootCause(t *testing.T) {
+	conn := &xgb.Conn{}
+	cookie := xproto.GetInputFocusCookie{Cookie: conn.NewCookie(true, true)}
+
+	err := cookie.Check()
+	if err == nil {
+		t.Fatal("Check on a reply-bearing cookie must fail; the device root cause would be reintroduced")
+	}
+	const deviceError = "Cannot call 'Check' on a cookie that is expecting a *reply*. Use 'Reply' instead."
+	if err.Error() != deviceError {
+		t.Fatalf("Check error=%q want the device-proven %q", err.Error(), deviceError)
+	}
+}

internal/display/x11.go

@@ -178,6 +178,44 @@ func (o X11Opener) pollInterval() time.Duration {
 	return 100 * time.Millisecond
 }
 
+// livenessRequest is one liveness round trip on a connection: it resolves
+// its request and reports only the error.
+type livenessRequest func() error
+
+// connLivenessProbe returns the liveness round trip used after mapping a
+// window and while a session waits for events. GetInputFocus carries a
+// reply, so its cookie must be resolved with Reply: calling Check on a
+// reply-bearing cookie fails at runtime with the XGB reply-cookie error
+// ("Cannot call 'Check' on a cookie that is expecting a *reply*"), which
+// killed every layer open and reconnect on the device.
+func connLivenessProbe(conn *xgb.Conn) livenessRequest {
+	return func() error {
+		return resolveLiveness(xproto.GetInputFocus(conn))
+	}
+}
+
+// resolveLiveness consumes one issued reply-bearing liveness cookie: the
+// reply is read and discarded, and only the error propagates. The cookie
+// must be resolved with Reply exactly once — Check on a reply-bearing
+// cookie returns the XGB reply-cookie error instead of ever reaching the
+// server, so a probe resolved with Check fails on every call regardless of
+// server health.
+func resolveLiveness(cookie xproto.GetInputFocusCookie) error {
+	_, err := cookie.Reply()
+	return err
+}
+
+// confirmMapped runs the post-map liveness stage of Open: a successful
+// reply proves the window is mapped and the server alive; a failure is
+// wrapped as the map-kiosk-window error so the caller closes the
+// connection and drops the mapped window.
+func confirmMapped(probe livenessRequest) error {
+	if err := probe(); err != nil {
+		return fmt.Errorf("map kiosk window: %w", err)
+	}
+	return nil
+}
+
 // Open connects to the X server and maps the kiosk window. Failure is
 // expected while the display manager is starting; the caller retries.
 func (o X11Opener) Open(ctx context.Context) (Session, error) {
@@ -239,10 +277,12 @@ func (o X11Opener) Open(ctx context.Context) (Session, error) {
 	// destroy is video end. WFB and launcher processes are never consulted.
 	xproto.ChangeWindowAttributes(conn, screen.Root,
 		xproto.CwEventMask, []uint32{xproto.EventMaskSubstructureNotify})
-	// Round trip confirms the window is mapped and the server is alive.
+	// Round trip confirms the window is mapped and the server is alive;
-	if err := xproto.GetInputFocus(conn).Check(); err != nil {
+	// the reply-bearing cookie is resolved with Reply (see
+	// connLivenessProbe).
+	if err := confirmMapped(connLivenessProbe(conn)); err != nil {
 		conn.Close()
-		return nil, fmt.Errorf("map kiosk window: %w", err)
+		return nil, err
 	}
 
 	// A layer reconnect must never cover running video: if video windows
@@ -301,6 +341,8 @@ func (o X11Opener) Open(ctx context.Context) (Session, error) {
 		markerAtom:   markerAtom,
 		role:         o.Role,
 		unicodeFont:  unicodeFont,
+		pollEvent:    conn.PollForEvent,
+		probe:        connLivenessProbe(conn),
 	}, nil
 }
 
@@ -560,6 +602,12 @@ type x11Session struct {
 	markerAtom   xproto.Atom
 	role         LayerRole
 	unicodeFont  bool
+	// pollEvent drains one pending X event; conn.PollForEvent in
+	// production. Tests script it.
+	pollEvent func() (xgb.Event, xgb.Error)
+	// probe performs one reply-bearing liveness round trip; its error ends
+	// the session. connLivenessProbe in production. Tests script it.
+	probe livenessRequest
 }
 
 func (s *x11Session) Canvas() Canvas { return s }
@@ -598,7 +646,7 @@ func (s *x11Session) WaitEvent(ctx context.Context) (bool, error) {
 	defer ticker.Stop()
 	for {
 		for {
-			event, err := s.conn.PollForEvent()
+			event, err := s.pollEvent()
 			if err != nil {
 				return false, err
 			}
@@ -613,8 +661,9 @@ func (s *x11Session) WaitEvent(ctx context.Context) (bool, error) {
 			}
 		}
 		// Round trip keeps the connection liveness visible even without
-		// incoming events.
+		// incoming events; the reply-bearing cookie is resolved with Reply
-		if err := xproto.GetInputFocus(s.conn).Check(); err != nil {
+		// (see connLivenessProbe).
+		if err := s.probe(); err != nil {
 			return false, err
 		}
 		select {