Fix Windows TUI frame freeze by always draining staging textures#116
Fix Windows TUI frame freeze by always draining staging textures#116
Conversation
…TUIs The Windows D3D11 renderer's mailbox-style staging ring only drained a completed readback when the VT had quiesced (`needs_draw=false`). With continuously streaming TUIs (codex, watch, top, btop) every prepaint sees a fresh VT generation, so the path always submitted and returned `Pending` without ever mapping the in-flight slot. The on-screen image froze; clicking flipped `prefer_latest=true`, the `block_drain` branch fired, and the catch-up frame made the cursor appear to jump several rows. Always compute `drain_target` and, when a prior submit's readback is ready while we're submitting a fresher copy, present it instead of returning `Pending`. The screen now lags the live VT by at most one frame instead of freezing indefinitely. Windows-only — macOS uses embedded libghostty's Metal NSView and doesn't go through this code. Fixes #114
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/con-ghostty/src/windows/render/mod.rs`:
- Around line 521-551: The branch that returns RenderOutcome::Rendered from a
prior readback (in the drained-handling block using
self.frame_from_readback(readback)) can satisfy low-latency gating with a stale
frame; modify RenderOutcome to carry a freshness/generation indicator (e.g. add
a boolean like presented_generation_matches_target or an Optional<generation> on
the Rendered variant) and set that field when constructing the outcome in this
block (compare the readback’s generation against the current snapshot or
snapshot.generation). Update callers (notably the logic in host_view.rs that
clears low_latency_generation_target on any non-Pending outcome) to only clear
the target when the RenderOutcome indicates the presented frame actually matches
the target generation. Ensure frame_from_readback or the readback struct exposes
the generation value used for the comparison so the drained branch can set the
freshness bit appropriately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bdbd50e3-cc6b-4f0e-8190-f616e0fa94ca
📒 Files selected for processing (2)
crates/con-ghostty/src/windows/render/mod.rspostmortem/2026-05-03-windows-codex-frame-freeze.md
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…under prefer_latest Address PR #116 review: - Cursor Bugbot (HIGH): the previous commit also removed the `!needs_draw` gate from `discard_oldest_in_flight()`. With 2 slots in flight and `needs_draw=true`, that discards the OLDEST slot (most likely GPU-ready) and forces the drain onto the NEWER slot (less likely ready). On GPUs where copies span >1 prepaint, this loops without progress and re-creates the freeze. Restore the gate; only `drain_target` itself stays ungated. - CodeRabbit (MAJOR): the new "drained_during_submit" shortcut could return `Rendered` with a frame older than the VT generation the user is waiting for. `host_view` clears `low_latency_generation_target` on any non-Pending outcome, so a paste/key handshake would be satisfied by a stale frame. Skip the shortcut when `prefer_latest=true` — that means a user-driven block_drain already tried; if it didn't fire, owe the caller Pending so target tracking stays correct. Postmortem updated to document both guards.
|
Bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 77a6ccb. Configure here.
…ilbox bursts Decouple presentation rate from VT update rate so mailbox-for-bursts and forward-progress-for-streaming fall out of one rule. The renderer now stamps `last_presented_at` on every `Rendered` outcome. The continuous-output drained-during-submit shortcut only fires when at least `MIN_FALLBACK_PRESENT_INTERVAL` (16 ms = one 60 Hz vsync) has elapsed since the last present. This keeps: - Short bursts (`ls`, `dir`, `clear`) on the established mailbox snap-to-final path (the fallback never fires inside one interval), preserving the 2026-04-26 windows-command-render-latency tuning. - Sustained TUIs (codex, top, watch, btop) at 60 Hz updates with bounded image-handoff cost — no faster than GPUI would composite anyway, since each present pays a full-frame Map + memcpy + bgra_frame_to_image + sprite-atlas upload. - User-driven echo / paste / click on the unchanged `block_drain` fast path; the `!prefer_latest` gate keeps the cap from interfering with `host_view`'s low-latency generation-target handshake. Postmortem updated with the rate-cap design and references to the prior Windows perf work.
|
Bugbot run |
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5764154. Configure here.
…e machine After the continuous-output fallback + 60 Hz cap landed, the doc on `Renderer::render` and the postmortem still described the old two-mode behavior. Rewrite both so the contract a reader sees matches what the code does: - The doc enumerates the four `RenderOutcome` decisions in priority order (user-driven block_drain → continuous-output fallback → quiet-VT catch-up → pending → idle), names every input the function reads, and calls out the mailbox property at the submit level vs. the oldest-first preference at the drain side. - The postmortem drops a stale paragraph that contradicted the new rate-cap design, adds a section on how the fallback composes with `host_view`'s 750 ms `LOW_LATENCY_BURST_WINDOW` (the two timelines partition cleanly with no overlap), and sharpens the "what we learned" bullets around the conflated-concern diagnosis. No behavior change.
📝 WalkthroughWalkthroughThe Windows renderer's ChangesWindows Renderer Presentation & Drain Selection
Sequence DiagramsequenceDiagram
actor Caller
participant Renderer
participant Ring as D3D11 Staging Ring
participant Readback
participant Clock as Wall-Clock
Note over Renderer: Before: drain_target gated by needs_draw
Caller->>Renderer: render(needs_draw=true)
Renderer->>Ring: oldest_in_flight() [NOW always selected]
Ring-->>Renderer: drain_target
Renderer->>Readback: Try drain oldest
alt Drained & !prefer_latest & presentation_due()
Renderer->>Clock: mark_presented(now)
Renderer-->>Caller: RenderOutcome::Rendered [continuous fallback]
Note over Renderer: New path: return cached drained frame instead of Pending
else Not due or prefer_latest
Renderer-->>Caller: RenderOutcome::Pending
end
Caller->>Renderer: render(needs_draw=false)
Renderer->>Ring: oldest_in_flight()
Ring-->>Renderer: drain_target
alt in_flight_before > 1
Renderer->>Ring: Discard oldest [still gated by !needs_draw]
end
Renderer->>Readback: Try drain oldest
alt Drained
Renderer->>Clock: mark_presented(now)
Renderer-->>Caller: RenderOutcome::Rendered
else Not drained
Renderer-->>Caller: RenderOutcome::Pending
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a3b7647-0f03-4286-bc45-3fbab3101aaf
📒 Files selected for processing (2)
crates/con-ghostty/src/windows/render/mod.rspostmortem/2026-05-03-windows-codex-frame-freeze.md
| if !prefer_latest | ||
| && self.presentation_due() | ||
| && let Some(readback) = drained | ||
| { | ||
| let outcome = RenderOutcome::Rendered(self.frame_from_readback(readback)); | ||
| self.mark_presented(); | ||
| log_render_profile( | ||
| prof_started, | ||
| snapshot, | ||
| prefer_latest, | ||
| needs_draw, | ||
| in_flight_before_submit, | ||
| drain_target, | ||
| true, | ||
| backlog, | ||
| submitted, | ||
| draw_ms, | ||
| submit_ms, | ||
| drain_ms, | ||
| block_drain_ms, | ||
| "rendered", | ||
| "drained_during_submit", | ||
| ); | ||
| return Ok(outcome); |
There was a problem hiding this comment.
Preserve a follow-up prepaint after drained_during_submit.
This branch still leaves the freshly submitted copy in flight, but GhosttyView::render only self-schedules another prepaint on RenderOutcome::Pending (crates/con-ghostty/src/windows/host_view.rs:228-268). If VT output stops right after this fallback fires, we publish the older drained frame here and never get the retry needed to drain/present the just-submitted frame, so the terminal can stay one frame stale until some unrelated wake arrives.
Please carry a “more work still in flight” signal in RenderOutcome (or add a dedicated variant) so the caller can still cx.notify() after this path.
Possible shape
pub enum RenderOutcome {
Unchanged,
- Rendered(FrameBgra),
+ Rendered {
+ frame: FrameBgra,
+ needs_followup_prepaint: bool,
+ },
Pending,
}Then set needs_followup_prepaint: true for drained_during_submit, and have host_view schedule another prepaint when that flag is set.
2 participants
Summary
Fixes issue #114 where codex's TUI output on Windows would freeze and stop refreshing unless the user pressed Enter or clicked the terminal. The root cause was a mailbox semantics bug in the D3D11 staging texture ring buffer that prevented older readbacks from being drained when continuous VT output kept
needs_drawtrue on every prepaint cycle.Changes
Always drain the oldest in-flight staging texture slot regardless of
needs_drawstate. The previous logic only drained whenneeds_drawwas false, which caused the pipeline to stall under continuous output.Return drained frames during submit when available instead of always returning
Pendingafter a fresh submit. When a prior readback completes while we're submitting a new frame, present that drained frame immediately rather than waiting for the next prepaint.Updated comments to explain the continuous-output fallback path and why draining must happen in both
needs_drawcases.Implementation Details
The fix changes the drain logic in
Renderer::render:Removed the
(!needs_draw)guard fromdrain_targetcomputation — now always attempt to drain the oldest in-flight slot via non-blockingtry_drain.When
needs_drawis true and adrainedreadback is available, returnRenderOutcome::Rendered(...)with that frame instead ofPending. This ensures forward progress on the displayed image even when the VT is continuously updating.The just-submitted fresh copy remains in flight and will be picked up by the next prepaint, so pipeline depth is unchanged.
This preserves the original "no slideshow during bursts" intent for short command output (
ls,dir,clear) while fixing the indefinite freeze for long-running TUIs (codex spinner,watch,top,btop, streaming output).The changes are Windows-only and gated by
#[cfg(target_os = "windows")].https://claude.ai/code/session_01T6fGh6NZDctPMvVE2RAwPp
Note
Medium Risk
Changes Windows D3D11 render/present scheduling and staging-ring draining logic; could affect frame cadence or introduce subtle latency/perf regressions, but is OS-scoped and guarded with rate limits.
Overview
Fixes a Windows-only bug where continuously updating TUIs could freeze because the staging-texture ring was only drained when
needs_drawbecame false.Renderer::rendernow always attempts to non-blocking drain the oldest in-flight staging slot, and adds a bounded fallback that can present a just-drained prior frame during a fresh submit whenprefer_latestis false and a 60HzMIN_FALLBACK_PRESENT_INTERVALgate allows it. AllRenderedpaths stamplast_presented_atto enforce the cap.Adds a postmortem document (
postmortem/2026-05-03-windows-codex-frame-freeze.md) describing the issue, root cause, and rationale for the new behavior.Reviewed by Cursor Bugbot for commit 5764154. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Documentation