Skip to content

Restart Chromium when a headful display change only changes the refresh rate - #497

Merged
masnwilliams merged 3 commits into
mainfrom
hypeship/restart-chromium-on-refresh-change
Nov 6, 2026
Merged

masnwilliams merged 3 commits into
mainfrom
hypeship/restart-chromium-on-refresh-change

Conversation

@masnwilliams

@masnwilliams masnwilliams commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Summary

On headful images, a PATCH /display (or a /chromium/configure display 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 responds refresh_rate: 60 and xrandr reports 1920x1080 59.96*, but requestAnimationFrame keeps 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 DisplayChangeNotifier only flags DISPLAY_METRIC_REFRESH_RATE on 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 with restartChromiumAndWait (which waits for DevTools to be ready) only when the realized X root size is unchanged and the refresh rate changed. The decision lives in chromiumRestartForDisplayChange:

change restart?
rate only, same size (25→60 or 60→25) yes
size only no (#262 fast path kept)
size and rate no; Chromium picks up the new rate with the size change
no change no (unchanged early return)
rate only, restart_chromium: false no; logs a warning that Chromium keeps its previous rate
  • Sizes are compared after applyResolutionAndConverge, because libxcvt rounding can map a different requested size onto the current one.
  • When Chromium is restarted, the CDP maximize re-assert is skipped (Chromium relaunches maximized). A failed restart returns a 500.
  • restart_chromium unset or true allows the restart. An explicit false opts out, which is the only way to keep the old no-restart behavior for a rate-only change.

/chromium/configure needs no change:

  • The live path reaches patchDisplayLocked, so create, pool acquire and browser update get the fix, with at most one restart per configure.
  • The restart path already applies the display while Chromium is stopped.

Cost

A rate-only change now takes about 1s inside the VM: PATCH /display measured 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

  • Unit tests: TestChromiumRestartForDisplayChange covers rate up, rate down, size only, size and rate, no change, and both restart_chromium values. go test ./cmd/api/... passes.
  • Live A/B: swapped only the kernel-images-api binary inside 3 headful browsers, built from the current release source with and without this patch, then drove the public browser update API. Values are the requestAnimationFrame median and whether Chromium restarted (PID change):
step without patch with patch
start 1920x1080@25 40ms 40ms
rate only → 1920x1080@60 40ms, no restart 16.7ms, restarted
rate only → 1920x1080@25 40ms (by luck: Chromium never left 25) 40ms, restarted
size only → 1280x800@25 40.1ms, no restart 40.1ms, no restart
size and rate → 1920x1080@60 16.7ms, no restart 16.7ms, no restart
no change 16.7ms 16.7ms, no restart
rate only → @25 with restart_chromium: false (in-VM call) — stays 16.7ms, no restart, warning logged
  • Not done: I didn't build the full image locally. The live A/B ran the real release image with only this binary changed.

Notes

The OpenAPI description of restart_chromium ("restart after resolution change ... default false for headful") is left as is. Updating it means regenerating oapi.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 via restartChromiumAndWait so 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: false skips 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 chromiumRestartForDisplayChange with comments explaining Linux/Chromium behavior, and TestChromiumRestartForDisplayChange covering 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 change

Reviewed by Cursor Bugbot for commit d809fab. Bugbot is set up for automated code reviews on this repo. Configure here.

@masnwilliams
masnwilliams merged commit 5835ee7 into main Nov 6, 2026
15 of 16 checks passed
@masnwilliams
masnwilliams deleted the hypeship/restart-chromium-on-refresh-change branch November 6, 2026 17:02
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants