Avoid blocking iOS surface mailbox during stream handling - #87
Avoid blocking iOS surface mailbox during stream handling#87lawrencecchen wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 5 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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: 0b1cb1d3b6
ℹ️ 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) { | ||
| self.deinitDroppedSurfaceMessage(msg); | ||
| return; |
There was a problem hiding this comment.
On iOS, when the app/surface mailbox is saturated, this drops every surface message, but several messages sent through this helper are one-shot state transitions rather than redraw hints. For example dcsCommand sends .tmux_control enter/exit events via surfaceMessageWriter; if the queue is full during a pane-output burst and the .exit event is dropped here, StreamHandler has already destroyed tmux_viewer while the embedded runtime never receives the exit callback and can remain stuck in tmux-control UI state. Please only drop messages that are safe to coalesce/rederive, or keep retrying/prioritizing critical surface events without an unbounded block.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR ports the iOS render-stall fix from the parent cmux PR into the Ghostty
Confidence Score: 4/5The change is a narrow, well-scoped iOS-only fix that correctly frees owned resources and avoids the deadlock. The non-iOS path is untouched. The deinit logic correctly covers all three WriteReq-bearing Message variants in the current union, and the iOS-only guard is compile-time checked. The main concern is the else => {} catch-all, which would silently skip cleanup for any future owned-resource variant added to Message, and clipboard_write drops happen without any log warning, making them invisible in production diagnostics. src/termio/stream_handler.zig — specifically deinitDroppedSurfaceMessage and the silent drop site in surfaceMessageWriter. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[surfaceMessageWriter called] --> B["push(msg, .instant)"]
B -->|"returns > 0 (success)"| C[Done — message queued]
B -->|"returns 0 (mailbox full)"| D{iOS?}
D -->|Yes| E[deinitDroppedSurfaceMessage]
E --> F{msg variant}
F -->|clipboard_write| G["w.req.deinit()"]
F -->|pwd_change| H["w.deinit()"]
F -->|tmux_control| I["w.data.deinit()"]
F -->|"else (no owned heap)"| J[no-op]
G --> K[return — message dropped]
H --> K
I --> K
J --> K
D -->|No — macOS/Linux| L[unlock renderer mutex]
L --> M["push(msg, .forever) — blocks until space"]
M --> N[re-lock renderer mutex]
N --> O[Done — message queued]
%%{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"}}}%%
flowchart TD
A[surfaceMessageWriter called] --> B["push(msg, .instant)"]
B -->|"returns > 0 (success)"| C[Done — message queued]
B -->|"returns 0 (mailbox full)"| D{iOS?}
D -->|Yes| E[deinitDroppedSurfaceMessage]
E --> F{msg variant}
F -->|clipboard_write| G["w.req.deinit()"]
F -->|pwd_change| H["w.deinit()"]
F -->|tmux_control| I["w.data.deinit()"]
F -->|"else (no owned heap)"| J[no-op]
G --> K[return — message dropped]
H --> K
I --> K
J --> K
D -->|No — macOS/Linux| L[unlock renderer mutex]
L --> M["push(msg, .forever) — blocks until space"]
M --> N[re-lock renderer mutex]
N --> O[Done — message queued]
Reviews (1): Last reviewed commit: "chore: refresh GhosttyKit artifact" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| switch (msg) { | ||
| .clipboard_write => |w| w.req.deinit(), | ||
| .pwd_change => |w| w.deinit(), | ||
| .tmux_control => |w| w.data.deinit(), | ||
| else => {}, | ||
| } |
There was a problem hiding this comment.
The else => {} catch-all means a future Message variant that carries heap-allocated data (another WriteReq-style field) will silently skip cleanup if it reaches the drop path. Because Zig's exhaustive switch would produce a compile error when a new variant is added, using else trades that compile-time guarantee away. If a new owned-resource variant is added to apprt.surface.Message without updating this function, the alloc case of the underlying MessageData union will never be freed, causing a leak that only manifests on iOS under mailbox pressure.
| if (comptime builtin.os.tag == .ios) { | ||
| self.deinitDroppedSurfaceMessage(msg); | ||
| return; | ||
| } |
There was a problem hiding this comment.
clipboard_write gives no user feedback
When clipboard_write is enqueued during a mailbox-saturation burst on iOS, it is silently dropped and the user's clipboard operation disappears with no error or retry. Unlike scrollbar ticks or color-change notifications (which are effectively stateless), a clipboard_write is a one-shot user action; if the surface never receives it, the user gets no indication that the write failed. Consider at minimum a log.warn at the drop site so the iOS-only silent-drop path is diagnosable in production logs.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Pins the Ghostty-side part of the cmux iOS render-stall fix.\n\nChanges:\n- On iOS, avoid falling back to a forever mailbox push when the surface mailbox is saturated.\n- Deinitialize owned payloads for dropped surface messages to avoid leaks.\n\nParent cmux PR: manaflow-ai/cmux#6543
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Avoids render stalls on iOS by not blocking when the surface mailbox is full; instead, we drop the message and deinit its owned payloads to prevent leaks. Aligns with the parent
cmuxfix: manaflow-ai/cmux#6543clipboard_write.req,pwd_change, andtmux_control.data.Written for commit 0b1cb1d. Summary will update on new commits.