From 3e5f29bea82b8fce7750cb53f8ad78749019243a Mon Sep 17 00:00:00 2001 From: hiroTamada <88675973+hiroTamada@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:45:04 +0000 Subject: [PATCH] Preserve display state and reject invalid screen changes --- server/cmd/api/api/chromium_configure.go | 3 + server/cmd/api/api/display.go | 95 ++++++++++------ server/cmd/api/api/display_resize_test.go | 132 ++++++++++++++++++++++ server/lib/nekoclient/client.go | 12 +- 4 files changed, 207 insertions(+), 35 deletions(-) create mode 100644 server/cmd/api/api/display_resize_test.go diff --git a/server/cmd/api/api/chromium_configure.go b/server/cmd/api/api/chromium_configure.go index 18d43c2bd..631cde43c 100644 --- a/server/cmd/api/api/chromium_configure.go +++ b/server/cmd/api/api/chromium_configure.go @@ -840,6 +840,9 @@ func chromiumDisplayApplyWhileStopped(ctx context.Context, s *ApiService, plan * } _, _, err := s.applyResolutionAndConverge(ctx, w, h, rr, s.isNekoEnabled()) if err != nil { + if isNekoScreenRejection(err) { + return cfg400(err.Error()) + } return cfg500ConfigureStep(chromiumConfigureStepDisplay, err.Error()) } return nil diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index 285ae8ffc..dd47ce497 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -3,8 +3,10 @@ package api import ( "context" "encoding/base64" + "errors" "fmt" "log/slog" + "math" "os" "os/exec" "path/filepath" @@ -14,6 +16,7 @@ import ( "github.com/kernel/kernel-images/server/lib/cdpclient" "github.com/kernel/kernel-images/server/lib/logger" + "github.com/kernel/kernel-images/server/lib/nekoclient" oapi "github.com/kernel/kernel-images/server/lib/oapi" "github.com/kernel/kernel-images/server/lib/recorder" nekooapi "github.com/m1k1o/neko/server/lib/oapi" @@ -130,6 +133,7 @@ func (s *ApiService) patchDisplayLocked(ctx context.Context, req oapi.PatchDispl "realized", fmt.Sprintf("%dx%d", realizedW, realizedH)) } width, height = realizedW, realizedH + _, _, refreshRate, _, err = s.getCurrentResolutionFromXrandr(ctx) } // Re-assert the maximized window state via CDP so mutter reflows the // window onto the new root; applyResolutionAndConverge already verified @@ -183,6 +187,9 @@ func (s *ApiService) patchDisplayLocked(ctx context.Context, req oapi.PatchDispl if err != nil { log.Error("failed to change resolution", "error", err) + if isNekoScreenRejection(err) { + return oapi.PatchDisplay400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: err.Error()}}, nil + } return oapi.PatchDisplay500JSONResponse{ InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{ Message: fmt.Sprintf("failed to change resolution: %s", err.Error()), @@ -553,6 +560,27 @@ func abs(x int) int { // relaunching Chromium in --kiosk — must go through here rather than call // setResolutionViaNeko directly. func (s *ApiService) applyResolutionAndConverge(ctx context.Context, width, height, refreshRate int, useNeko bool) (int, int, error) { + previousW, previousH, previousRate, _, err := s.getCurrentResolutionFromXrandr(ctx) + if err != nil { + return 0, 0, err + } + realizedW, realizedH, err := s.setResolutionAndConverge(ctx, width, height, refreshRate, useNeko) + if err == nil { + return realizedW, realizedH, nil + } + + // Mode creation can move the X root even when the screen change fails. + // Restoration needs the same retries as resizing: Neko can drop a reconfig. + restoreCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 35*time.Second) + defer cancel() + if _, _, restoreErr := s.setResolutionAndConverge(restoreCtx, previousW, previousH, previousRate, useNeko); restoreErr != nil { + logger.FromContext(ctx).Error("failed to restore display after resize failure", "error", restoreErr) + err = fmt.Errorf("%w; failed to restore display: %s", err, restoreErr) + } + return realizedW, realizedH, err +} + +func (s *ApiService) setResolutionAndConverge(ctx context.Context, width, height, refreshRate int, useNeko bool) (int, int, error) { log := logger.FromContext(ctx) var err error if useNeko { @@ -593,6 +621,11 @@ func (s *ApiService) applyResolutionAndConverge(ctx context.Context, width, heig return realizedW, realizedH, nil } +func isNekoScreenRejection(err error) bool { + var screenErr *nekoclient.ScreenConfigurationError + return errors.As(err, &screenErr) && screenErr.StatusCode >= 400 && screenErr.StatusCode < 500 +} + // setViewportViaCDP resizes the browser viewport using the CDP // Emulation.setDeviceMetricsOverride command. This is near-instant and does // not require restarting Chromium or Xvfb. @@ -719,7 +752,7 @@ func (s *ApiService) getCurrentResolution(ctx context.Context) (int, int, int, e // getCurrentResolutionFromXrandr queries xrandr for the current display // resolution. The fourth return value reports whether the refresh rate was -// parsed from xrandr output (true) or is the synthesized fallback (false); +// parsed from xrandr output (true) or is unavailable (false); // callers can use it to prefer a previously-recorded rate when xrandr is // silent, as Xvfb is. func (s *ApiService) getCurrentResolutionFromXrandr(ctx context.Context) (int, int, int, bool, error) { @@ -727,8 +760,7 @@ func (s *ApiService) getCurrentResolutionFromXrandr(ctx context.Context) (int, i display := s.resolveDisplayFromEnv() // Use xrandr to get current resolution - // Note: Using bash -c (not -lc) to avoid login shell overriding DISPLAY env var - cmd := exec.CommandContext(ctx, "bash", "-c", "xrandr | grep -E '\\*' | awk '{print $1}'") + cmd := exec.CommandContext(ctx, "xrandr", "--current") cmd.Env = append(os.Environ(), fmt.Sprintf("DISPLAY=%s", display)) out, err := cmd.Output() @@ -737,40 +769,35 @@ func (s *ApiService) getCurrentResolutionFromXrandr(ctx context.Context) (int, i return 0, 0, 0, false, fmt.Errorf("failed to execute xrandr command: %w", err) } - resStr := strings.TrimSpace(string(out)) - parts := strings.Split(resStr, "x") - if len(parts) != 2 { - log.Error("unexpected xrandr output format", "output", resStr) - return 0, 0, 0, false, fmt.Errorf("unexpected xrandr output format: %s", resStr) - } + return parseCurrentResolutionFromXrandr(string(out)) +} - width, err := strconv.Atoi(parts[0]) - if err != nil { - log.Error("failed to parse width", "error", err, "value", parts[0]) - return 0, 0, 0, false, fmt.Errorf("failed to parse width '%s': %w", parts[0], err) - } - - // Parse height and refresh rate (e.g., "1080_60.00" -> height=1080, rate=60) - heightStr := parts[1] - refreshRate := 60 // default when xrandr omits the _rate suffix (e.g. Xvfb) - rateFromXrandr := false - if idx := strings.Index(heightStr, "_"); idx != -1 { - rateStr := heightStr[idx+1:] - heightStr = heightStr[:idx] - // Parse the refresh rate (e.g., "60.00" -> 60) - if rateFloat, err := strconv.ParseFloat(rateStr, 64); err == nil { - refreshRate = int(rateFloat) - rateFromXrandr = true +func parseCurrentResolutionFromXrandr(out string) (int, int, int, bool, error) { + for _, line := range strings.Split(out, "\n") { + fields := strings.Fields(line) + if len(fields) < 2 { + continue + } + for _, field := range fields[1:] { + if !strings.Contains(field, "*") { + continue + } + rate, err := strconv.ParseFloat(strings.TrimRight(field, "*+"), 64) + if err != nil { + return 0, 0, 0, false, fmt.Errorf("invalid xrandr refresh rate %q: %w", field, err) + } + modeName := strings.SplitN(fields[0], "_", 2)[0] + var width, height int + if _, err := fmt.Sscanf(modeName, "%dx%d", &width, &height); err != nil { + return 0, 0, 0, false, fmt.Errorf("invalid xrandr mode %q: %w", fields[0], err) + } + if rate == 0 { + return width, height, 60, false, nil + } + return width, height, int(math.Round(rate)), true, nil } } - - height, err := strconv.Atoi(heightStr) - if err != nil { - log.Error("failed to parse height", "error", err, "value", heightStr) - return 0, 0, 0, false, fmt.Errorf("failed to parse height '%s': %w", heightStr, err) - } - - return width, height, refreshRate, rateFromXrandr, nil + return 0, 0, 0, false, fmt.Errorf("no current mode in xrandr output") } // stoppedRecordingInfo holds state captured from a recording that was stopped diff --git a/server/cmd/api/api/display_resize_test.go b/server/cmd/api/api/display_resize_test.go new file mode 100644 index 000000000..66898f99c --- /dev/null +++ b/server/cmd/api/api/display_resize_test.go @@ -0,0 +1,132 @@ +package api + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + + "github.com/kernel/kernel-images/server/lib/nekoclient" + oapi "github.com/kernel/kernel-images/server/lib/oapi" + "github.com/kernel/kernel-images/server/lib/recorder" + nekooapi "github.com/m1k1o/neko/server/lib/oapi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseCurrentResolutionFromXrandr(t *testing.T) { + for _, tc := range []struct { + name string + out string + width, height, rate int + hasRate bool + }{ + {"rounded CVT rate", " 376x480_60 59.21*+\n", 376, 480, 59, true}, + {"nearest integer", " 1280x800_60.00 59.81*\n", 1280, 800, 60, true}, + {"non-default rate", " 1920x1080_25.00 24.98*+ 60.00\n", 1920, 1080, 25, true}, + {"Xvfb", " 1280x720 0.00*\n", 1280, 720, 60, false}, + } { + t.Run(tc.name, func(t *testing.T) { + w, h, rate, hasRate, err := parseCurrentResolutionFromXrandr(tc.out) + require.NoError(t, err) + assert.Equal(t, tc.width, w) + assert.Equal(t, tc.height, h) + assert.Equal(t, tc.rate, rate) + assert.Equal(t, tc.hasRate, hasRate) + }) + } + _, _, _, _, err := parseCurrentResolutionFromXrandr("DUMMY0 connected\n") + require.Error(t, err) +} + +func TestRejectedResizeRestoresDisplay(t *testing.T) { + for _, status := range []int{http.StatusUnprocessableEntity, http.StatusBadRequest, http.StatusInternalServerError} { + for _, path := range []string{"display", "configure-live", "configure-stopped"} { + t.Run(fmt.Sprintf("%s/%d", path, status), func(t *testing.T) { + dir := t.TempDir() + state := filepath.Join(dir, "screen") + previous := " 1280x800_25.00 25.00*+\n" + require.NoError(t, os.WriteFile(state, []byte(previous), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "xrandr"), []byte("#!/bin/sh\ncat \"$TEST_XRANDR_STATE\"\n"), 0700)) + t.Setenv("TEST_XRANDR_STATE", state) + t.Setenv("PATH", dir+":"+os.Getenv("PATH")) + t.Setenv("ENABLE_WEBRTC", "true") + + var requests []nekooapi.ScreenConfiguration + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path == "/api/login" { + _, _ = w.Write([]byte(`{"token":"test-token"}`)) + return + } + var config nekooapi.ScreenConfiguration + if err := json.NewDecoder(r.Body).Decode(&config); err != nil { + w.WriteHeader(http.StatusBadRequest) + return + } + requests = append(requests, config) + if len(requests) == 1 { + _ = os.WriteFile(state, []byte(" 3840x2160_25.00 25.00*+\n"), 0600) + w.WriteHeader(status) + _, _ = w.Write([]byte(`{"message":"cannot set screen size"}`)) + return + } + if path == "configure-live" && len(requests) == 2 { + // Neko can accept a reconfig without applying it. + w.WriteHeader(http.StatusNoContent) + return + } + _ = os.WriteFile(state, []byte(previous), 0600) + w.WriteHeader(http.StatusNoContent) + })) + defer server.Close() + client, err := nekoclient.NewAuthClient(server.URL, "admin", "test") + require.NoError(t, err) + svc := &ApiService{nekoAuthClient: client, recordManager: recorder.NewFFmpegManager()} + svc.displayModeOnce.Do(func() { svc.displayModeVal = "xorg" }) + width, height, rate, idle := 379, 480, 60, false + requestRate := oapi.PatchDisplayRequestRefreshRate(rate) + body := &oapi.PatchDisplayJSONRequestBody{Width: &width, Height: &height, RefreshRate: &requestRate, RequireIdle: &idle} + ctx := context.Background() + if path == "display" { + resp, err := svc.PatchDisplay(ctx, oapi.PatchDisplayRequestObject{Body: body}) + require.NoError(t, err) + if status < 500 { + require.IsType(t, oapi.PatchDisplay400JSONResponse{}, resp) + } else { + require.IsType(t, oapi.PatchDisplay500JSONResponse{}, resp) + } + } else { + var resp oapi.ChromiumConfigureResponseObject + if path == "configure-live" { + resp = chromiumRunPatchDisplay(ctx, svc, body) + } else { + resp = chromiumDisplayApplyWhileStopped(ctx, svc, &chromiumDisplayPlan{width: width, height: height, refreshRate: rate}) + } + if status < 500 { + require.IsType(t, oapi.ChromiumConfigure400JSONResponse{}, resp) + } else { + require.IsType(t, oapi.ChromiumConfigure500JSONResponse{}, resp) + } + } + if path == "configure-live" { + require.Len(t, requests, 3) + } else { + require.Len(t, requests, 2) + } + for _, restore := range requests[1:] { + assert.Equal(t, 1280, *restore.Width) + assert.Equal(t, 800, *restore.Height) + assert.Equal(t, 25, *restore.Rate) + } + got, err := os.ReadFile(state) + require.NoError(t, err) + assert.Equal(t, previous, string(got)) + }) + } + } +} diff --git a/server/lib/nekoclient/client.go b/server/lib/nekoclient/client.go index 514b2b4b4..8e4b56ba9 100644 --- a/server/lib/nekoclient/client.go +++ b/server/lib/nekoclient/client.go @@ -117,6 +117,16 @@ func (c *AuthClient) SessionsGet(ctx context.Context) ([]nekooapi.SessionData, e return *resp.JSON200, nil } +// ScreenConfigurationError preserves the HTTP status of a rejected screen change. +type ScreenConfigurationError struct { + StatusCode int + Body string +} + +func (e *ScreenConfigurationError) Error() string { + return fmt.Sprintf("screen configuration API returned status %d: %s", e.StatusCode, e.Body) +} + // ScreenConfigurationChange changes the screen resolution via Neko API. // The HTTP response body echoes the request, not the realized // configuration (neko's screenConfigurationChange handler returns `data`, @@ -158,7 +168,7 @@ func (c *AuthClient) ScreenConfigurationChange(ctx context.Context, config nekoo } if resp.StatusCode() != http.StatusOK && resp.StatusCode() != http.StatusNoContent { - return fmt.Errorf("screen configuration API returned status %d: %s", resp.StatusCode(), string(resp.Body)) + return &ScreenConfigurationError{StatusCode: resp.StatusCode(), Body: string(resp.Body)} } return nil