display: return realized X root size instead of failing on libxcvt rounding#276
Merged
Conversation
…unding PATCH /display previously enforced strict equality between the request and the X root size after a resize. libxcvt's CVT 8-pixel grid round (e.g. 1365 → 1360) and the hard-coded FWXGA bump for 1360×768 → 1366×768 make that equality impossible to satisfy for some requests, returning 500. Callers that treat /display 500s as fatal end up tainting the browser instance on every odd-width resize. Replace the strict post-condition with a single xrandr read of the X root after neko's synchronous resize returns. Use the realized dimensions for the response body so callers' coordinate math lines up with what the X server actually rendered. Log the request-vs-realized gap for diagnosability. The e2e reproducer is flipped from asserting the 500 to asserting the 200 + realized 1366 in the response body. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
sjmiller609
approved these changes
Jun 4, 2026
|
Created a monitoring plan for this PR. What this PR does: Fixes browser VMs being permanently tainted and recycled when a display resize request uses a width libxcvt rounds to a different value (e.g., 1365px → 1366px). Previously every such Intended effect:
Risks:
|
A single xrandr read immediately after neko's resize could catch a transient — chromium running in --kiosk briefly pushes the X root to the dummy DDX's max mode (3840×2160) while mutter settles on the new screen, producing a misleading "realized" size in the response. Replace the single read with a short poll loop that returns early when either (a) xrandr reports the requested size, or (b) consecutive readings are stable for ~150ms. The deadline (10s) only triggers if the X root never converges, in which case the last observation is still returned — the path stays non-fatal. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
waitForXRootRealized would treat any three consecutive stable readings as the realized size, which silently accepts the pre-resize baseline if the X server hasn't committed the new mode yet. The response would echo back the old dimensions as 'realized' — a success-looking lie. Capture the pre-resize X root, pass it through, and exclude it from the stable-match condition. The match-the-request fast path is unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1e848b7. Configure here.
The previous commit added a guard that refused to accept the pre-resize baseline as a stable-N match, defending against a hypothetical race where the X server hadn't committed the new mode yet. In practice XSetScreenConfiguration is a synchronous X protocol round-trip, so by the time neko's call returns the server has committed — that race doesn't exist. The guard had a real cost: when a request rounds back to the current X root (idempotent re-PATCH of an odd width, or any resize whose libxcvt realization equals the current screen), the poll loop would never declare baseline-stable and would wait the full 10s before returning. Drop the guard. Stable-N now accepts any value, including the baseline. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
8c80a4a to
07268ef
Compare
…N path Why: chromium --start-maximized briefly drives xrandr to report the dummy DDX's max mode (e.g. 3840x2160) while mode-switch propagates. A naive stable-N would echo that transient into the response body. Real libxcvt rounding is <16 px; the dummy max is >1000 px off — acceptableDelta=32 sits between them.
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
PATCH /displaypreviously enforced strict equality between the request and the X root size after a resize. libxcvt's CVT 8-pixel grid round (e.g. 1365 → 1360) and the hard-coded FWXGA bump for 1360×768 → 1366×768 make that equality impossible to satisfy for some requests, returning 500./display500s asvm_unrecoverablewere tainting the browser instance on every odd-width/configurecall.Where the rounding lives
libxcvt_gen_mode_info(called by neko viaXCreateScreenMode) rounds widths down to the CVT 8-pixel grid inlib/libxcvt.c:102:Then at the end of the same function there's an FWXGA carve-out (
lib/libxcvt.c:294-298) that bumps the canonical 1360×768 CVT output back to 1366×768 to match real laptop EDIDs:So 1365×768 → 1360×768 (CVT round) → 1366×768 (FWXGA bump). Verified by linking a small probe against libxcvt 0.1.3 directly:
No amount of rounding logic on the kernel-images side can mirror this — the right contract is to read what landed, not predict it.
Why this approach over the alternatives
resolveDisplayParams: would need to mirror libxcvt's CVT-8 round and FWXGA carve-out and any future libxcvt rule. Brittle.screenConfigurationChangehandler returns the request body (HttpSuccess(w, data)), not the realized config (size) — a separate upstream bug. Worth fixing in kernel/neko but doesn't block this.Verified
chromium-headful-test:latestlocally and ran the e2e reproducer (TestDisplayResizeOddWidthHonoursLibxcvtRounding):X root verification: x root is 1366x768, want 1365x768.{"height":768,"refresh_rate":60,"width":1366}andx_root=1366x768.headful_start_maximized,headful_kiosk,headful_xorg_no_neko.headless_defaultskipped because the headless image isn't built locally (unrelated).Test plan
go build ./...go test ./cmd/api/api/... ./lib/nekoclient/...(unit tests pass)TestDisplayResizeOddWidthHonoursLibxcvtRoundingpasses against built imageTestDisplayResizeChromiumWindowheadful subtests passFollow-ups
screenConfigurationChangeto returnsizeinstead ofdataso the HTTP response carries the realized dims. Independent of this PR.🤖 Generated with Claude Code
Note
Medium Risk
Changes headful display resize success criteria and API-reported dimensions; mis-polling could return wrong sizes, but CDP maximize failures still fail the request.
Overview
PATCH /displayon the headful Xorg path no longer fails when the X server lands on a size libxcvt rounded away from the request (e.g. 1365×768 → 1366×768). StrictwaitForXRootSizeverification is replaced bywaitForXRootRealized, which polls xrandr until the root matches the request, or stays stable within 32px of it—so CVT/FWXGA rounding is accepted while transient dummy-DDX sizes (e.g. 3840×2160 during kiosk settle) are not. The handler uses those dimensions in the 200 body (with logging when they differ from the request); CDP maximize re-assert runs after that poll. Neko client docs note the API echoes the request, not realized size.Adds
TestDisplayResizeOddWidthHonoursLibxcvtRoundingfor the production 1365×768 case.Reviewed by Cursor Bugbot for commit 2050e6a. Bugbot is set up for automated code reviews on this repo. Configure here.