Repository navigation
Restart Chromium when a headful display change only changes the refresh rate - #497
Merged
Merged
Conversation
sjmiller609
approved these changes
Nov 2, 2026
hiroTamada
approved these changes
Nov 4, 2026
masnwilliams
added a commit
that referenced
this pull request
Nov 6, 2026
## Summary When Neko can't set a screen configuration, it returns 422 (`cannot set screen size`). kernel-images passes that back as a 500 from both `PATCH /display` and `/configure`, so callers can't tell "this size isn't supported" apart from a server fault. The control plane only turns a 4xx from the VM into a client error; a 500 gets treated as a broken instance, retried on other instances, and then reported as a generic 500. ## Change - `nekoclient.ScreenConfigurationChange` wraps a 422 in the new `ErrScreenConfigurationRejected`. Other non-2xx statuses are unchanged. - `PATCH /display` (Xorg path) and the `/configure` display step applied while Chromium is stopped return **400** `the display cannot be set to WxH at R Hz` for that error. The live `/configure` path goes through `patchDisplayLocked`, so it gets the 400 as well. Every other failure is still a 500. This doesn't make more sizes work. The root cause, generated modes that X lists at one Hz below the requested rate, is fixed in kernel/neko#31. With that fix in, the remaining 422s are sizes the X server can't set at all, for example anything below 64x64. ## Testing - **Unit tests:** - `TestScreenConfigurationChangeStatus` covers 200, 422 and 500 from a fake Neko. - `TestPatchDisplayNekoRejection` and `TestChromiumDisplayApplyWhileStoppedNekoRejection` drive the real handlers against a fake Neko and a fake `xrandr`: a 422 becomes a 400 and a 500 stays a 500. - Both handler tests fail without the change. `go test -race` passes for every non-e2e package except `TestUpstreamManagerDetectsChromiumAndRestart` (devtoolsproxy, untouched). It failed once on a temp-dir cleanup race and passed when rerun. - **Production browser (throwaway session, hot-patched binary):** built from the deployed release source plus this change, together with the neko fix. `PATCH /browsers/{id}` to 63x400 returned 400 `viewport_configuration_failed` / `the display cannot be set to 63x400 at 60 Hz` in 0.2s. Before, it returned 500 `failed to update session`. ## Notes - **Overlap with #497:** both PRs touch `display.go`, in separate hunks. This branch merges cleanly with #497 as it stands. - **kernel-images-private sync:** `chromiumDisplayApplyWhileStopped` calls the fork-only `applyXorgDisplayResolution` there. The resolution is to keep the fork's `badReq` handling and add the `ErrScreenConfigurationRejected` check after it. I built and ran exactly that for the production test. - **Follow-up:** bump `ghcr.io/kernel/neko/base` in both Dockerfiles once kernel/neko#31 is tagged. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Behavior change is limited to HTTP status mapping for an existing Neko 422 path; no auth or data-handling changes. > > **Overview** > When Neko’s screen API returns **422** (X refused the requested mode), callers previously saw **500**, which made unsupported viewports look like a broken VM. This PR classifies that case as a **client error**. > > **`nekoclient.ScreenConfigurationChange`** now wraps HTTP 422 in **`ErrScreenConfigurationRejected`**. Other non-2xx responses are unchanged. > > **`PATCH /display`** and the Chromium **`/configure`** display step (including live configure via `patchDisplayLocked`) map that sentinel to **400** with `the display cannot be set to WxH at R Hz` via **`unsupportedDisplayMessage`**. All other resize failures still return **500**. > > Tests cover the Neko client status handling and both API paths (422 → 400, 500 → 500). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1200e1f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On headful images, a
PATCH /display(or a/chromium/configuredisplay part) that changes only the refresh rate at the current resolution switches the X mode but leaves Chromium rendering at its old rate. For example, 1920x1080@25 → 1920x1080@60 respondsrefresh_rate: 60and xrandr reports1920x1080 59.96*, butrequestAnimationFramekeeps firing every 40ms until Chromium restarts.Chromium on Linux re-reads the screen's refresh rate when the X screen size changes. A rate-only mode switch produces no display-metrics change, because
DisplayChangeNotifieronly flagsDISPLAY_METRIC_REFRESH_RATEon macOS (ui/display/display_change_notifier.cc). Before #262 the Xorg path restarted Chromium after every resize, which hid this. #262 removed that restart for size changes, which Chromium handles live, so rate-only changes regressed.Change
patchDisplayLocked(Xorg branch) now restarts Chromium withrestartChromiumAndWait(which waits for DevTools to be ready) only when the realized X root size is unchanged and the refresh rate changed. The decision lives inchromiumRestartForDisplayChange:restart_chromium: falseapplyResolutionAndConverge, because libxcvt rounding can map a different requested size onto the current one.restart_chromiumunset ortrueallows the restart. An explicitfalseopts out, which is the only way to keep the old no-restart behavior for a rate-only change./chromium/configureneeds no change:patchDisplayLocked, so create, pool acquire and browser update get the fix, with at most one restart per configure.Cost
A rate-only change now takes about 1s inside the VM:
PATCH /displaymeasured 0.63–1.71s, median 0.97s, across 3 browsers. Size-only changes stay at 8–32ms.The restart drops CDP connections and resets page state. That was already true of every headful resize before #262, but it now applies only to rate-only changes.
Testing
TestChromiumRestartForDisplayChangecovers rate up, rate down, size only, size and rate, no change, and bothrestart_chromiumvalues.go test ./cmd/api/...passes.kernel-images-apibinary inside 3 headful browsers, built from the current release source with and without this patch, then drove the public browser update API. Values are therequestAnimationFramemedian and whether Chromium restarted (PID change):restart_chromium: false(in-VM call)Notes
The OpenAPI description of
restart_chromium("restart after resolution change ... default false for headful") is left as is. Updating it means regeneratingoapi.go; I can do that in a follow-up if preferred.Note
Medium Risk
Alters headful display handling by restarting Chromium on rate-only mode switches (~1s, drops CDP/state); restart failure now fails the request. Opt-out preserves old behavior with a warning.
Overview
Headful
PATCH /display(Xorg) now detects refresh-rate-only changes (same realized width/height as before the request, different rate). In that case it restarts Chromium viarestartChromiumAndWaitso the new refresh interval is applied; size-only and size+rate changes still avoid restart and rely on CDP maximize afterward.Opt-out:
restart_chromium: falseskips the restart for rate-only changes and logs a warning that Chromium keeps rendering at the old rate until a later restart. Failed restart surfaces as an error (500 path) instead of silently succeeding.Adds
chromiumRestartForDisplayChangewith comments explaining Linux/Chromium behavior, andTestChromiumRestartForDisplayChangecovering rate up/down, size-only, combined changes, no-op, and both restart flags.Conventional commit title:
fix(api): restart Chromium on headful refresh-rate-only display changeReviewed by Cursor Bugbot for commit d809fab. Bugbot is set up for automated code reviews on this repo. Configure here.