diff --git a/server/cmd/api/api/chromium_configure.go b/server/cmd/api/api/chromium_configure.go index 83c7d6c23..1e6185978 100644 --- a/server/cmd/api/api/chromium_configure.go +++ b/server/cmd/api/api/chromium_configure.go @@ -17,6 +17,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/policy" "github.com/kernel/kernel-images/server/lib/zstdutil" @@ -842,6 +843,9 @@ func chromiumDisplayApplyWhileStopped(ctx context.Context, s *ApiService, plan * return nil } _, _, err := s.applyResolutionAndConverge(ctx, w, h, rr, s.isNekoEnabled()) + if errors.Is(err, nekoclient.ErrScreenConfigurationRejected) { + return cfg400(unsupportedDisplayMessage(w, h, rr)) + } if err != nil { return cfg500ConfigureStep(chromiumConfigureStepDisplay, err.Error()) } diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go index 060d318a2..6e2615902 100644 --- a/server/cmd/api/api/display.go +++ b/server/cmd/api/api/display.go @@ -15,6 +15,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" @@ -184,6 +185,9 @@ func (s *ApiService) patchDisplayLocked(ctx context.Context, req oapi.PatchDispl if err != nil { log.Error("failed to change resolution", "error", err) + if errors.Is(err, nekoclient.ErrScreenConfigurationRejected) { + return oapi.PatchDisplay400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: unsupportedDisplayMessage(width, height, refreshRate)}}, nil + } return oapi.PatchDisplay500JSONResponse{ InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{ Message: fmt.Sprintf("failed to change resolution: %s", err.Error()), @@ -946,3 +950,9 @@ func (s *ApiService) setResolutionViaNeko(ctx context.Context, width, height, re log.Info("successfully changed resolution via Neko API", "width", width, "height", height, "refresh_rate", refreshRate) return nil } + +// unsupportedDisplayMessage is the client error for a display configuration +// the X server refused to set. +func unsupportedDisplayMessage(width, height, refreshRate int) string { + return fmt.Sprintf("the display cannot be set to %dx%d at %d Hz", width, height, refreshRate) +} diff --git a/server/cmd/api/api/display_test.go b/server/cmd/api/api/display_test.go index f0eb93f4c..851d64c3f 100644 --- a/server/cmd/api/api/display_test.go +++ b/server/cmd/api/api/display_test.go @@ -6,6 +6,8 @@ import ( "fmt" "io" "log/slog" + "net/http" + "net/http/httptest" "os" "path/filepath" "testing" @@ -13,6 +15,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" "github.com/kernel/kernel-images/server/lib/scaletozero" @@ -611,3 +614,79 @@ func TestChromiumRunPatchDisplayDoesNotRelockChromiumConfig(t *testing.T) { t.Fatal("chromiumRunPatchDisplay tried to reacquire chromiumConfigMu") } } + +// newRejectingNekoDisplay points the service at a headful display whose Neko +// answers every screen change with screenStatus, and whose xrandr reports +// 1920x1080 at 25 Hz. +func newRejectingNekoDisplay(t *testing.T, screenStatus int) *ApiService { + t.Helper() + + neko := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/login": + _, _ = w.Write([]byte(`{"token":"t"}`)) + case "/api/room/screen": + w.WriteHeader(screenStatus) + _, _ = w.Write([]byte(`{"code":422,"message":"cannot set screen size"}`)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(neko.Close) + + binDir := t.TempDir() + xrandr := "#!/bin/sh\necho ' 1920x1080_25.00 25.00*'\n" + require.NoError(t, os.WriteFile(filepath.Join(binDir, "xrandr"), []byte(xrandr), 0o755)) + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("ENABLE_WEBRTC", "true") + + origConf := xvfbSupervisorConf + xvfbSupervisorConf = filepath.Join(t.TempDir(), "absent.conf") + t.Cleanup(func() { xvfbSupervisorConf = origConf }) + + client, err := nekoclient.NewAuthClient(neko.URL, "admin", "admin") + require.NoError(t, err) + svc := newTestServiceWithFactory(t, newMockRecordManager(), newMockFactory()) + svc.nekoAuthClient = client + return svc +} + +func TestPatchDisplayNekoRejection(t *testing.T) { + width, height := 1008, 384 + rate := oapi.PatchDisplayRequestRefreshRate(60) + requireIdle := false + req := oapi.PatchDisplayRequestObject{Body: &oapi.PatchDisplayJSONRequestBody{Width: &width, Height: &height, RefreshRate: &rate, RequireIdle: &requireIdle}} + + t.Run("422 is a client error", func(t *testing.T) { + svc := newRejectingNekoDisplay(t, http.StatusUnprocessableEntity) + resp, err := svc.PatchDisplay(context.Background(), req) + require.NoError(t, err) + require.IsType(t, oapi.PatchDisplay400JSONResponse{}, resp) + assert.Equal(t, "the display cannot be set to 1008x384 at 60 Hz", resp.(oapi.PatchDisplay400JSONResponse).Message) + }) + + t.Run("500 stays a server error", func(t *testing.T) { + svc := newRejectingNekoDisplay(t, http.StatusInternalServerError) + resp, err := svc.PatchDisplay(context.Background(), req) + require.NoError(t, err) + require.IsType(t, oapi.PatchDisplay500JSONResponse{}, resp) + }) +} + +func TestChromiumDisplayApplyWhileStoppedNekoRejection(t *testing.T) { + plan := &chromiumDisplayPlan{width: 1008, height: 384, refreshRate: 60} + + t.Run("422 is a client error", func(t *testing.T) { + svc := newRejectingNekoDisplay(t, http.StatusUnprocessableEntity) + resp := chromiumDisplayApplyWhileStopped(context.Background(), svc, plan) + require.IsType(t, oapi.ChromiumConfigure400JSONResponse{}, resp) + assert.Equal(t, "the display cannot be set to 1008x384 at 60 Hz", resp.(oapi.ChromiumConfigure400JSONResponse).Message) + }) + + t.Run("500 stays a server error", func(t *testing.T) { + svc := newRejectingNekoDisplay(t, http.StatusInternalServerError) + resp := chromiumDisplayApplyWhileStopped(context.Background(), svc, plan) + require.IsType(t, oapi.ChromiumConfigure500JSONResponse{}, resp) + }) +} diff --git a/server/lib/nekoclient/client.go b/server/lib/nekoclient/client.go index 514b2b4b4..82f1c4fdb 100644 --- a/server/lib/nekoclient/client.go +++ b/server/lib/nekoclient/client.go @@ -2,6 +2,7 @@ package nekoclient import ( "context" + "errors" "fmt" "net/http" "sync" @@ -9,6 +10,11 @@ import ( nekooapi "github.com/m1k1o/neko/server/lib/oapi" ) +// ErrScreenConfigurationRejected is returned when Neko answers a screen +// configuration change with 422, which it does when the X server refuses the +// requested size or refresh rate. Retrying the same request cannot succeed. +var ErrScreenConfigurationRejected = errors.New("screen configuration rejected") + // AuthClient wraps the Neko OpenAPI client and handles authentication automatically. // It manages token caching and refresh on 401 responses. type AuthClient struct { @@ -157,6 +163,9 @@ func (c *AuthClient) ScreenConfigurationChange(ctx context.Context, config nekoo } } + if resp.StatusCode() == http.StatusUnprocessableEntity { + return fmt.Errorf("%w: screen configuration API returned status %d: %s", ErrScreenConfigurationRejected, resp.StatusCode(), string(resp.Body)) + } if resp.StatusCode() != http.StatusOK && resp.StatusCode() != http.StatusNoContent { return fmt.Errorf("screen configuration API returned status %d: %s", resp.StatusCode(), string(resp.Body)) } diff --git a/server/lib/nekoclient/client_test.go b/server/lib/nekoclient/client_test.go new file mode 100644 index 000000000..f189d9284 --- /dev/null +++ b/server/lib/nekoclient/client_test.go @@ -0,0 +1,56 @@ +package nekoclient + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + + nekooapi "github.com/m1k1o/neko/server/lib/oapi" +) + +func TestScreenConfigurationChangeStatus(t *testing.T) { + cases := []struct { + name string + status int + wantErr bool + wantRejected bool + }{ + {name: "ok", status: http.StatusOK}, + {name: "unprocessable", status: http.StatusUnprocessableEntity, wantErr: true, wantRejected: true}, + {name: "server error", status: http.StatusInternalServerError, wantErr: true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/api/login": + _, _ = w.Write([]byte(`{"token":"t"}`)) + case "/api/room/screen": + w.WriteHeader(tc.status) + _, _ = w.Write([]byte(`{}`)) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + client, err := NewAuthClient(srv.URL, "admin", "admin") + if err != nil { + t.Fatal(err) + } + width, height, rate := 1008, 384, 60 + err = client.ScreenConfigurationChange(context.Background(), nekooapi.ScreenConfiguration{Width: &width, Height: &height, Rate: &rate}) + + if (err != nil) != tc.wantErr { + t.Fatalf("got error %v, want error %v", err, tc.wantErr) + } + if got := errors.Is(err, ErrScreenConfigurationRejected); got != tc.wantRejected { + t.Fatalf("errors.Is(err, ErrScreenConfigurationRejected) = %v, want %v (err: %v)", got, tc.wantRejected, err) + } + }) + } +}