From ffae3560c423ab221d991885639612549b057b12 Mon Sep 17 00:00:00 2001 From: Hermes Date: Fri, 2 Oct 2026 10:42:07 +0200 Subject: [PATCH] fix: resolve reply-bearing X11 liveness cookies with Reply (#2) --- internal/display/liveness_test.go | 145 ++++++++++++++++++++++++++++++ internal/display/x11.go | 61 +++++++++++-- 2 files changed, 200 insertions(+), 6 deletions(-) create mode 100644 internal/display/liveness_test.go diff --git a/internal/display/liveness_test.go b/internal/display/liveness_test.go new file mode 100644 index 0000000..df0d20b --- /dev/null +++ b/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) + } +} diff --git a/internal/display/x11.go b/internal/display/x11.go index 9d79f68..7215599 100644 --- a/internal/display/x11.go +++ b/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. - if err := xproto.GetInputFocus(conn).Check(); err != nil { + // Round trip confirms the window is mapped and the server is alive; + // 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. - if err := xproto.GetInputFocus(s.conn).Check(); err != nil { + // incoming events; the reply-bearing cookie is resolved with Reply + // (see connLivenessProbe). + if err := s.probe(); err != nil { return false, err } select { -- 2.54.0