Skip to content

Bill a slow present to the compositor, not to the app - #508

Open
samoylenkodmitry wants to merge 1 commit into
mainfrom
claude/stoic-franklin-d8ce3d
Open

Bill a slow present to the compositor, not to the app#508
samoylenkodmitry wants to merge 1 commit into
mainfrom
claude/stoic-franklin-d8ce3d

Conversation

@samoylenkodmitry

Copy link
Copy Markdown
Owner

robot_idle_fps_after_tab_walk failed on a loaded host (load_1m 24 on a 10-core machine) with:

wait_for_idle: timed out after 1280577 iterations; needs_update=false,
needs_redraw=true, has_animations=false, waiting_for_present=true

then passed in isolation and passed again in a full 121-test suite run on the same commit (a1f64e5).

The defect

The budget was already wall-clock (ROBOT_IDLE_TIMEOUT = 10s); iterations is a diagnostic, and enters the decision only as a floor that delays failure on a starved host. 1.28M turns inside that budget is a loop with all the CPU it asked for — an outstanding idle wait pins the event loop at ControlFlow::Poll — so the floor did not apply and the wait was reported as AppNotConverging.

But its own diagnostics say composition had converged: needs_update=false, has_animations=false. The only outstanding work was waiting_for_present — a frame the compositor had not handed back. The application was failed for a wait it does not own. The codebase already models compositor refusal separately (ROBOT_PRESENT_WAIT_TIMEOUT, unpresentable_frames_since_present); the idle path never consulted it.

The fix

The two waits have different owners and now have different budgets. A settled composition waiting on a present gets its own clock, armed on the turn it becomes the sole blocker, bounded by ROBOT_IDLE_PRESENT_TIMEOUT = 30s and reported as SurfaceNotPresenting against the surface — naming refused frames, target generation and current one, as the standalone present wait already does.

30s keeps that under run_robot_test.sh's generic 60s per-example timeout, so a window that truly never presents still reports a diagnostic instead of dying as an opaque exit 124; the 60s starvation ceiling would have collided with it. AppNotConverging keeps the soft budget and every diagnostic field it carried. Headless runs are unaffected — they never arm a present target.

Three tests. Verified against the unfixed code: the two new ones fail with exactly the reported misattribution (AppNotConverging where SurfaceNotPresenting/None belongs). The third pins that an app still churning with a present outstanding keeps being blamed, so the exemption cannot over-correct.

cranpose-testing

The headless wait_for_idle deliberately does not get the same treatment. That pump is synchronous work against an in-memory renderer with nothing to block on, so what a healthy app needs does not move with host load; timing it would decide headless results by host load — the flakiness this PR removes from the desktop side.

Its defect was the opposite: exhausting the budget returned quietly and handed back a half-settled tree. It now has a named budget, the reason it is counted in turns, and a loud failure with a diagnostic. pump_robot_until_stable was a copy of the same loop with per-call-site bounds of 40 and 60; instrumented measurement says every call settles on turn 1, so it goes and its callers use wait_for_idle.

Also

debug_toggles' non-UTF-8 fixture spelled a tmpfs path, which the workspace tmpfs hygiene gate reads as a test writing to /tmpjust test was red on main before this branch (introduced by #495). The fixture is about byte preservation; the directory is arbitrary.

Known, not addressed here

  • While parked on a present the loop spins at Poll, competing with the compositor it waits for. That is the mechanism behind the failure, but polling across "asking for a frame and being handed it" is documented as required or measured FPS clamps to vsync — changing it would break the frame-rate contracts this test family exists to check.
  • RobotTestRule::wait_for_idle loops on needs_redraw() (stale pixels + renderer warm-up), not pending work, so it is effectively "update once and return" — all 35 calls measured settle at turn 1. Fixing that means switching to frame_schedule() plus a shared animation-loop exit, and changes what every headless test observes. Filed separately; the misleading doc is corrected here.

Local gates: just fmt-check, just test, just clippy all clean.

🤖 Generated with Claude Code

robot_idle_fps_after_tab_walk failed on a loaded host with "timed out
after 1280577 iterations; needs_update=false, needs_redraw=true,
has_animations=false, waiting_for_present=true", then passed in
isolation and passed again in a full suite run on the same commit.

1.28M turns inside the 10s budget is a loop with all the CPU it asked
for -- an outstanding idle wait pins the event loop at ControlFlow::Poll
-- so the starvation floor did not apply and the wait was reported as an
application defect. But its own diagnostics say composition had
converged: no pending update, no animation. The only outstanding work
was a frame the compositor had not handed back. The application was
failed for a wait it does not own.

The two waits have different owners and now have different budgets. A
settled composition waiting on a present gets a clock of its own, armed
on the turn it becomes the sole blocker, bounded by
ROBOT_IDLE_PRESENT_TIMEOUT and reported as SurfaceNotPresenting against
the surface -- naming the refused frames, the target generation and the
current one, the way the standalone present wait already does. 30s keeps
that under run_robot_test.sh's generic per-example timeout, so a window
that truly never presents still reports a diagnostic instead of dying as
an opaque exit 124; the 60s starvation ceiling would have collided with
it. AppNotConverging keeps the soft budget and keeps every diagnostic
field it carried. Headless runs are unaffected: they never arm a present
target.

cranpose-testing's headless wait_for_idle does not get the same
treatment. That pump is synchronous work against an in-memory renderer
with nothing to block on, so what a healthy app needs does not move with
host load, and timing it would decide headless results by host load --
the flakiness this commit removes from the desktop side. Its defect was
the opposite one: exhausting the budget returned quietly and handed back
a half-settled tree. It now has a named budget, the reason it is counted
in turns, and a loud failure. pump_robot_until_stable was a copy of the
same loop with per-call-site bounds of 40 and 60; measurement says every
call settles on turn 1, so it goes and its callers use wait_for_idle.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant