Avoid blocking iOS surface mailbox writes#90
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughModifies StreamHandler.surfaceMessageWriter to add iOS-specific handling when a message push with ChangesSurface Message Drop Handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant StreamHandler
participant SurfaceMailbox
Renderer->>StreamHandler: surfaceMessageWriter(message)
StreamHandler->>SurfaceMailbox: push(message, .instant)
SurfaceMailbox-->>StreamHandler: push failed (mailbox full)
alt iOS
StreamHandler->>StreamHandler: deinitDroppedSurfaceMessage(message)
StreamHandler-->>Renderer: return (message dropped)
else non-iOS
StreamHandler->>StreamHandler: unlock renderer mutex
StreamHandler->>SurfaceMailbox: push(message, .forever)
SurfaceMailbox-->>StreamHandler: delivered
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95fa65e65b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if (comptime builtin.os.tag == .ios) { | ||
| var dropped = msg; | ||
| deinitDroppedSurfaceMessage(&dropped); | ||
| return; |
There was a problem hiding this comment.
This iOS full-mailbox path drops every surfaceMessageWriter message, but some of those are request/response protocol messages rather than lossy UI notifications: clipboardContents('?') sends .clipboard_read, and sendSizeReport(.csi_21_t) sends .report_title, both of which rely on the app thread to write a reply back to the PTY (for OSC 52, Surface.completeClipboardReadOSC52 even replies for empty clipboard data because the client is expecting one). When the app mailbox is saturated on iOS, these requests now disappear with no fallback response, so shell programs querying the clipboard or title can hang or time out; please special-case response-producing messages instead of dropping them wholesale.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/termio/stream_handler.zig">
<violation number="1" location="src/termio/stream_handler.zig:148">
P2: This drops protocol response requests such as clipboard reads and title reports when the iOS mailbox is full. Limit the nonblocking drop path to truly lossy UI side-effect messages, otherwise OSC/title queries can silently never receive a response.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // mailbox invariant. | ||
| if (comptime builtin.os.tag == .ios) { | ||
| var dropped = msg; | ||
| deinitDroppedSurfaceMessage(&dropped); |
There was a problem hiding this comment.
P2: This drops protocol response requests such as clipboard reads and title reports when the iOS mailbox is full. Limit the nonblocking drop path to truly lossy UI side-effect messages, otherwise OSC/title queries can silently never receive a response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/termio/stream_handler.zig, line 148:
<comment>This drops protocol response requests such as clipboard reads and title reports when the iOS mailbox is full. Limit the nonblocking drop path to truly lossy UI side-effect messages, otherwise OSC/title queries can silently never receive a response.</comment>
<file context>
@@ -130,12 +130,39 @@ pub const StreamHandler = struct {
+ // mailbox invariant.
+ if (comptime builtin.os.tag == .ios) {
+ var dropped = msg;
+ deinitDroppedSurfaceMessage(&dropped);
+ return;
+ }
</file context>
Greptile SummaryThis PR adds an iOS-specific non-blocking fast-path in
Confidence Score: 4/5Safe to merge for the render-liveness fix; the trade-off of dropping messages instead of blocking is deliberate and well-documented. The logic is sound and the three heap-owning variants are correctly cleaned up. The src/termio/stream_handler.zig — specifically the Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant VT as VT Stream
participant SH as StreamHandler
participant MB as App/Surface Mailbox
participant Tick as ghostty_app_tick
VT->>SH: emit surface message
SH->>MB: push(msg, .instant)
alt instant push succeeds
MB-->>SH: "returns > 0"
Note over SH: message queued
else mailbox full
MB-->>SH: returns 0
alt iOS comptime
Note over SH: deinitDroppedSurfaceMessage()
SH-->>VT: return non-blocking
else non-iOS
Note over SH: unlock renderer mutex
SH->>MB: push(msg, .forever)
MB-->>SH: space available
Note over SH: re-lock renderer mutex
end
end
Tick->>MB: drain messages
MB-->>Tick: surface messages delivered
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant VT as VT Stream
participant SH as StreamHandler
participant MB as App/Surface Mailbox
participant Tick as ghostty_app_tick
VT->>SH: emit surface message
SH->>MB: push(msg, .instant)
alt instant push succeeds
MB-->>SH: "returns > 0"
Note over SH: message queued
else mailbox full
MB-->>SH: returns 0
alt iOS comptime
Note over SH: deinitDroppedSurfaceMessage()
SH-->>VT: return non-blocking
else non-iOS
Note over SH: unlock renderer mutex
SH->>MB: push(msg, .forever)
MB-->>SH: space available
Note over SH: re-lock renderer mutex
end
end
Tick->>MB: drain messages
MB-->>Tick: surface messages delivered
Reviews (1): Last reviewed commit: "Avoid blocking iOS surface mailbox write..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn deinitDroppedSurfaceMessage(msg: *apprt.surface.Message) void { | ||
| switch (msg.*) { | ||
| .pwd_change => |*w| w.deinit(), | ||
| .clipboard_write => |*w| w.req.deinit(), | ||
| .tmux_control => |*v| v.data.deinit(), | ||
| else => {}, | ||
| } | ||
| } |
There was a problem hiding this comment.
else => {} catch-all silently no-ops for any apprt.surface.Message variant not explicitly listed. If a new heap-owning variant is added to the union and routed through surfaceMessageWriter, the iOS drop path will leak its allocation with no compile-time warning. Exhaustively listing the non-owning arms forces the compiler to flag new variants that need to be audited here.
| fn deinitDroppedSurfaceMessage(msg: *apprt.surface.Message) void { | |
| switch (msg.*) { | |
| .pwd_change => |*w| w.deinit(), | |
| .clipboard_write => |*w| w.req.deinit(), | |
| .tmux_control => |*v| v.data.deinit(), | |
| else => {}, | |
| } | |
| } | |
| fn deinitDroppedSurfaceMessage(msg: *apprt.surface.Message) void { | |
| switch (msg.*) { | |
| .pwd_change => |*w| w.deinit(), | |
| .clipboard_write => |*w| w.req.deinit(), | |
| .tmux_control => |*v| v.data.deinit(), | |
| // All remaining variants are plain value types (no heap ownership). | |
| // If a new heap-owning variant is added to Message, add it above and | |
| // call its deinit; the exhaustive match will ensure it isn't missed. | |
| .set_title, | |
| .report_title, | |
| .set_mouse_shape, | |
| .clipboard_read, | |
| .change_config, | |
| .close, | |
| .child_exited, | |
| .desktop_notification, | |
| .renderer_health, | |
| .present_surface, | |
| .password_input, | |
| .color_change, | |
| .selection_scroll_tick, | |
| .ring_bell, | |
| .progress_report, | |
| .start_command, | |
| .stop_command, | |
| .scrollbar, | |
| .search_total, | |
| .search_selected, | |
| => {}, | |
| } | |
| } |
| if (comptime builtin.os.tag == .ios) { | ||
| var dropped = msg; | ||
| deinitDroppedSurfaceMessage(&dropped); | ||
| return; | ||
| } |
There was a problem hiding this comment.
clipboard_read and report_title (sent via surfaceMessageWriter at lines 1164 and 1562) are not one-way UI side-effects — they are request-response sequences where the surface is expected to write data back to the PTY. When these messages are dropped under saturation, the remote terminal application (e.g. a shell using OSC 52 ? to read the clipboard, or an app issuing CSI 21 t to query the window title) will receive no response and may block indefinitely or time out visibly. Worth documenting the known behavioral gap, or gating the drop on variant type.
1 participant
Summary
Verification
zig build -Demit-xcframework=true -Dxcframework-target=universal -Doptimize=ReleaseFastfrom cmux.testScrollForwardingDoesNotOutliveLocalRenderLivenesspassed after the parent cmux harness reproduced the pre-fix busy snapshot state.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Keep iOS surface output nonblocking when the app/surface mailbox is saturated to avoid parking the surface queue behind
ghostty_app_tick. This preserves render and snapshot liveness on a busy main thread.pwd_change,clipboard_write, andtmux_control.Written for commit 95fa65e. Summary will update on new commits.
Summary by CodeRabbit