renderer: thread-safe tokened forced draw for offscreen capture - #181
Merged
Merged
Conversation
Add ghostty_surface_request_render_with_token: queue one forced update+draw executed on the renderer thread via the existing draw_now async, acknowledged through the render-presented callback after the exact frame's main-thread IOSurface assignment. Unlike ghostty_surface_render_now_with_token (which renders on the calling thread and requires embedder-owned renderer state, i.e. iOS external drain), this is safe from any thread while the renderer OS thread is live, and deliberately skips the occlusion visibility gate so an occluded window still renders a fresh ground-truth frame. cmux uses it for the debug-socket offscreen surface screenshot RPC.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds a C API for tokened forced rendering. Requests queue on the renderer thread, allow one pending token, bypass occlusion, and report presentation after the exact frame reaches the platform layer. It also documents Darwin locale initialization ordering. ChangesTokened rendering
Locale initialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant EmbeddedSurface
participant RendererThread
participant PlatformLayer
participant PresentationCallback
EmbeddedSurface->>RendererThread: queue tokened forced render
RendererThread->>RendererThread: consume token and rebuild frame data
RendererThread->>PlatformLayer: submit exact frame for presentation
PlatformLayer->>PresentationCallback: report presented token
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/os/open.zig`:
- Around line 197-198: Update the assertion in the relevant open-process logging
test so the blank-line-only child output requires logged to equal zero, rather
than merely being below ten; preserve the existing test setup and logging
behavior.
- Around line 130-134: Update the test “drainStderr drains blank lines without
reporting” to also assert that the reader is fully consumed by checking r.end -
r.seek equals zero after drainStderr returns, while preserving the existing
zero-count assertion.
- Around line 107-110: Normalize each line’s trailing carriage return before the
blank-line check in the opener stderr-reading flow, so CRLF-only lines are
treated as empty and skipped. Add a regression test covering blank CRLF output
and verify no diagnostic record is emitted for it.
In `@src/renderer/Thread.zig`:
- Around line 1838-1850: Prevent tokened draw requests from becoming stranded
when external drain is active: update Thread.zig lines 1838-1850 to have the
external renderer owner consume the request or reject it without retaining the
pending slot, and synchronize request acceptance with external-drain activation
in Thread.zig lines 1026-1043. Preserve the corrected acceptance result through
Surface.requestRenderWithToken in src/apprt/embedded.zig lines 1414-1429 and its
C ABI wrapper at lines 3149-3156, and document any platform or external-drain
restriction in include/ghostty.h lines 1394-1406.
🪄 Autofix
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 Plus
Run ID: b480837a-77f3-4ef3-a9fb-ef91c70e09c2
📒 Files selected for processing (5)
include/ghostty.hsrc/apprt/embedded.zigsrc/global.zigsrc/os/open.zigsrc/renderer/Thread.zig
Comment on lines
+107
to
+110
| // Blank lines contain no diagnostic information. Keep consuming them | ||
| // so the child can exit, but do not emit repeated `open stderr=` | ||
| // records if an opener writes blank output. | ||
| if (line.len == 0) continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)open\.zig$|src/os/open\.zig$' || true
echo
echo "Relevant src/os/open.zig lines:"
if [ -f src/os/open.zig ]; then
nl -ba src/os/open.zig | sed -n '80,140p'
fi
echo
echo "Search takeDelimiter and blank line handling:"
rg -n "takeDelimiter|line\.len|stderr=|Windows|crlf|\\r?\\n" -C 3 .Repository: manaflow-ai/ghostty
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant src/os/open.zig lines:"
sed -n '80,220p' src/os/open.zig | awk '{printf "%6d\t%s\n", NR+79, $0}'
echo
echo "Search takeDelimiter and blank line handling:"
rg -n "takeDelimiter|line\.len|stderr=|Windows|crlf|cr|\\\\r" -C 3 .Repository: manaflow-ai/ghostty
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant src/os/open.zig lines:"
sed -n '80,220p' src/os/open.zig | awk '{printf "%6d\t%s\n", NR+79, $0}'
echo
echo "Focused search in src/os/open.zig and tests containing open:"
rg -n "takeDelimiter|drainStderr|line\.len|stderr=|Windows|os\.open|openStderr|skipWindows|skip_windows|\\\\r|\\\\n" src/os/open.zig src test -C 3 || true
echo
echo "Behavioral probe of CRLF-delimited line in Zig-like stream logic:"
python3 - <<'PY'
import re
data = b"open stderr=test\r\nfoo\r\n\r\n"
lines = data.split(b"\n")
for raw in lines:
line = raw[:-1] if raw[-1:] == b"\r" else raw
print(repr(raw), "len", len(raw), "stripped_len", len(line), "is_blank=", len(line) == 0)
PYRepository: manaflow-ai/ghostty
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "src/os/open.zig first 240 lines:"
awk '{printf "%6d\t%s\n", NR, $0}' src/os/open.zig | sed -n '1,240p'
echo
echo "open.zig occurrences of delimiters/tests:"
rg -n "takeDelimiter|buf\.readUntilDelimiter|drainStderr|line\.len|stderr=|Windows|windows|\\\\r|\\\\n|crlf" src/os/open.zig -C 3
echo
echo "Behavior: empty CRLF vs LF line length after LF-delimited reads:"
python3 - <<'PY'
test_cases = b"a\r\n\r\n"
line_buf = bytearray()
for byte in test_cases:
line_buf.append(byte)
if byte == 10: # '\n'
raw = bytes(line_buf)[:-1] if len(line_buf) >= 1 and line_buf[-1] == 10 else bytes(line_buf)
line_buf.clear()
print(repr(raw), "line.len", len(raw), "is_zero_len", len(raw) == 0)
PYRepository: manaflow-ai/ghostty
Length of output: 14749
Normalize CRLF lines before the blank-line check.
takeDelimiter('\n') leaves \r on Windows for a line like \r\n, so a blank CRLF line has line.len == 1 and is logged. Normalize the trailing \r before line.len == 0 and add a CRLF regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/os/open.zig` around lines 107 - 110, Normalize each line’s trailing
carriage return before the blank-line check in the opener stderr-reading flow,
so CRLF-only lines are treated as empty and skipped. Add a regression test
covering blank CRLF output and verify no diagnostic record is emitted for it.
Comment on lines
+130
to
+134
| test "drainStderr drains blank lines without reporting" { | ||
| // The old `takeDelimiterExclusive` loop spun forever here: the seek position | ||
| // parks on the delimiter and every subsequent read returns an empty slice. | ||
| var r: std.Io.Reader = .fixed("\n\n\n"); | ||
| try std.testing.expectEqual(@as(usize, 3), drainStderr(&r)); | ||
| try std.testing.expectEqual(@as(usize, 0), drainStderr(&r)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert end-of-stream in the blank-line test.
The test checks only the reported count. A regression that stops after the first blank line can return 0 and still pass while leaving input unread. Also assert that r.end - r.seek is zero.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/os/open.zig` around lines 130 - 134, Update the test “drainStderr drains
blank lines without reporting” to also assert that the reader is fully consumed
by checking r.end - r.seek equals zero after drainStderr returns, while
preserving the existing zero-count assertion.
Comment on lines
+197
to
+198
| // A short burst of identical blank lines must not become a log flood. | ||
| try testing.expect(logged < 10); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert zero reported lines.
The child emits only blank lines, so logged must equal 0. The current logged < 10 assertion allows regressions that report up to nine blank lines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/os/open.zig` around lines 197 - 198, Update the assertion in the relevant
open-process logging test so the blank-line-only child output requires logged to
equal zero, rather than merely being below ten; preserve the existing test setup
and logging behavior.
Comment on lines
1838
to
+1850
| if (t.externalDrainActive()) return .rearm; | ||
|
|
||
| // cmux fork: a queued tokened draw takes this wake. It rebuilds frame data | ||
| // from current terminal state and skips the `flags.visible` gate on | ||
| // purpose (drawFrame's early-return): the whole point of the tokened path | ||
| // is ground-truth capture of a window the compositor considers occluded. | ||
| // The renderer-realized gate still applies (drawing unrealized GPU state | ||
| // is invalid); an unconsumed presentation is dropped and its callback | ||
| // never fires, which the embedder surfaces as a timeout. | ||
| if (t.takePendingDrawPresentation()) |presentation| { | ||
| drawPendingTokenedFrame(t, presentation); | ||
| return .rearm; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Reject or service tokened requests in external-drain mode.
When external_drain is active, Line 1838 returns before takePendingDrawPresentation. requestDrawWithPresentation can still return true and leave the pending slot occupied forever. The callback never receives the token, and all later requests return false.
src/renderer/Thread.zig#L1838-L1850: consume the request through the external renderer owner, or reject it without retaining the slot.src/renderer/Thread.zig#L1026-L1043: synchronize rejection with external-drain activation so an accepted request cannot become stranded.src/apprt/embedded.zig#L1414-L1429: preserve the corrected acceptance behavior inSurface.requestRenderWithToken.src/apprt/embedded.zig#L3149-L3156: preserve the corrected acceptance behavior in the C ABI wrapper.include/ghostty.h#L1394-L1406: document any platform or external-drain restriction.
📍 Affects 3 files
src/renderer/Thread.zig#L1838-L1850(this comment)src/renderer/Thread.zig#L1026-L1043src/apprt/embedded.zig#L1414-L1429src/apprt/embedded.zig#L3149-L3156include/ghostty.h#L1394-L1406
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/Thread.zig` around lines 1838 - 1850, Prevent tokened draw
requests from becoming stranded when external drain is active: update Thread.zig
lines 1838-1850 to have the external renderer owner consume the request or
reject it without retaining the pending slot, and synchronize request acceptance
with external-drain activation in Thread.zig lines 1026-1043. Preserve the
corrected acceptance result through Surface.requestRenderWithToken in
src/apprt/embedded.zig lines 1414-1429 and its C ABI wrapper at lines 3149-3156,
and document any platform or external-drain restriction in include/ghostty.h
lines 1394-1406.
…n-render # Conflicts: # src/global.zig
zig 0.16 removed std.Thread.Mutex; match the file's existing mutex idiom (lockUncancelable(global.io()) / unlock(global.io())).
…n-render # Conflicts: # src/global.zig
This was referenced Aug 12, 2026
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
1 participant
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.
Adds
ghostty_surface_request_render_with_token: queues one forced update+draw executed on the renderer thread via the existingdraw_nowasync, acknowledged through the render-presented callback after the exact frame's main-thread IOSurface assignment.Mechanism: a single-slot mutex-guarded pending
FramePresentationonrenderer.Thread;drawNowCallbackconsumes it, rebuilds frame data from current terminal state (updateFrame), and draws withdrawFrameWithPresentation(sync=true). On Metal the presentation rides the command-buffer completion and is delivered on main in the same block that assigns the frame's IOSurface to the layer, so an embedder readinglayer.contentsfrom the callback observes at least that frame.Why a new entry point:
ghostty_surface_render_now_with_tokenperforms the whole render cycle on the calling thread and is only safe when the embedder owns renderer state (iOS external-drain mode). On macOS the renderer OS thread is live, and draining the mailbox from a foreign thread mutates libxev loop timers unsafely. This API only fills the slot and ringsdraw_now, so it is safe from any thread.The forced draw deliberately skips the
flags.visibleocclusion gate (but keeps therenderer_realizedgate): its purpose is ground-truth capture of a surface whose window is occluded, on another Space, or never ordered front. cmux uses it for thedebug.surface.screenshotdebug-socket RPC (cmux PR to follow; the cmux submodule pointer bump depends on this PR merging first).Note: branch is based on the SHA cmux main currently pins (19d03fa), which is itself ahead of this fork's main via a pending PR; the extra commits below mine will disappear from this diff once that lands.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds a thread-safe API to request a tokened offscreen render on the renderer thread, enabling ground-truth screenshots even when the window is occluded. Also reduces noisy opener logs and fixes a startup locale race.
New Features
ghostty_surface_request_render_with_token(...)to queue one forced update+draw on the renderer thread.draw_nowand delivers the exact frame’s token via the render-presented callback after IOSurface assignment.Bug Fixes
drainStderr: ignore blank lines and bound repeats; tests for EOF and repeated blank output.std.Io.Mutexfor the pending presentation slot for Zig 0.16 compatibility.Written for commit a52417b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation