fix: quadratic chrome-event dedup and clean-quit races in the TUI

[metaphorics]

Summary

Adversarial E2E waves against the Linux TUI (raw PTY byte streams, paste+resize+key races, 200k-line command output, malformed RPC, signals under load, UI attach/detach churn, state bombs) surfaced four real defects, each fixed in its own commit. The clean-quit race fixes for the in-loop request path landed separately in #42 (finish_run/successful_exit), so this PR is the delta on top.

Emitter::redraw quadratic dedup. First-redraw channels get chrome.snapshot_events() merged with this pass's pending events. The merge appended every deduped event into the list it was scanning — ~N²/2 Object::eq comparisons for a batch of N msg_show events: :!seq 1 50000 took ~25s with a UI attached vs 30ms headless. Dedup is now against the immutable snapshot only, and the merged list is built only when a channel is actually uninitialized. :!seq 1 50000 now replies in ~0.16s.

Startup try_resize quit race. The one request path #42's finish_run didn't cover: a -c qall-style child can exit after attach but before the registered resize request lands, failing the TUI on transport instead of exiting cleanly. The startup try_resize error now goes through finish_run exactly like the in-loop paths.

SIGTERM wedge during synchronous RPC waits. Client::request blocks in next_message on incoming.recv() until the reply arrives, and a :!cmd<CR> input only replies after its shell command finishes — so signals.pending() at the loop top is unreachable while the wait is parked. SIGTERM during a 200k-line :!seq left the TUI alive permanently (main thread futex-parked; reader thread busy draining echo events). ShutdownSignals now registers a second, non-consumed requested flag per restore signal; Client::watch_shutdown polls it inside a 50ms recv_timeout, surfacing ClientError::Interrupted, which the run loop maps back to the existing pending() → restore → resume_default path. SIGTERM mid-flood now exits in ~0.1s.

:cq N exit code discarded. A child that exits with a status chose its own code, but finish_run wrapped every Eof in the transport diagnostic and run_interactive discarded it — the process always exited 1. finish_run now relays a code-bearing child's captured stderr verbatim, and run_interactive returns Result<ExitCode, AppError> mapping Eof/NonZeroExit codes onto the process status (mch_exit(code)). :cq 3 through the full TUI exits 3.

'readonly' never set → E45 unreachable. readfile marks 'readonly' on first load when no write bit is set (fileio.c:462-539), but BufferFlags::READONLY had no writer and the E45 gates checked only that flag — even :set readonly couldn't protect a file. Load paths (open_startup_files for argv files, load_buffer_for_switch for :e/:b on a fresh buffer, origin.is_created()) now set the buffer-local option via a mode & 0222 == 0 metadata probe, and the gates honor option OR flag. The FileIO seam has no access(W_OK) so ACL/mount-level unwritability is uncovered; the [RO] read-message suffix is also not wired — flagged for follow-up.

Testing

• cargo nextest run --workspace: 3502/3502 pass.
• Wave-3 adversarial suite 39/39 on this binary (multi-UI attach/detach churn under flood, malformed msgpack-RPC on the embed stream, SIGTSTP/SIGCONT/SIGTERM/SIGINT storm, same-batch key+paste+resize interleave, split and invalid UTF-8 across PTY writes, :cq/:cq N exit codes, readonly :x/:w!, job floods outliving quit, autocmd/redraw/register/wildmenu/redir bombs, resize-during-flood, rapid launch-quit cycles); wave-2 61/61 and wave-1 44/44 suites still green.
• Real-terminal (konsole + tmux on DISPLAY :0): :!seq 1 50000 ~0.05s to request reply, resize-storm + :wq 5/5 clean exits, SIGTERM mid-flood restores terminal and exits, :x on a 444 file reports E45 and :w! overrides.
• cargo test -p ox-tui -p ox-editor -p oxvim all green; no new clippy warnings on touched files.

Link to Devin session: https://app.devin.ai/sessions/5f07717a060d44ddafade088e73fe4fc
Open in Devin Desktop: https://app.devin.ai/desktop/session/5f07717a060d44ddafade088e73fe4fc?variant=devin
Requested by: @metaphorics

[devin-ai-integration]

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

• Disable automatic comment, CI, and merge conflict monitoring

---

Original prompt from a

@gosuda/oxvim Run fully E2E various TUI QA/QC/debug/fix/codebase-cleanup for Linux.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: b47bf7fb-65fe-4c8d-859b-a6c19ee24dd1

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 26343987-1db1-4712-9355-aafd82862b65

📥 Commits

Reviewing files that changed from the base of the PR and between 7af4dec and 88c582f.

📒 Files selected for processing (3)

• crates/ox-tui/src/client.rs
• crates/ox-tui/src/lib.rs
• crates/ox-ui/src/emitter.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Recent review details

⏰ Context from checks skipped due to timeout. (5)

• GitHub Check: Real editor PTY (Linux)
• GitHub Check: Native terminal library (Windows, not full editor)
• GitHub Check: Analyze (ruby)
• GitHub Check: Analyze (actions)
• GitHub Check: Analyze (rust)

🔇 Additional comments (3)

crates/ox-tui/src/client.rs (1)

211-220: LGTM!

crates/ox-tui/src/lib.rs (1)

422-425: LGTM!

Also applies to: 456-463, 520-521, 530-545, 549-564

crates/ox-ui/src/emitter.rs (1)

123-139: LGTM!

---

📝 Summary

Summary by CodeRabbit

• Bug Fixes
  • The terminal interface now exits cleanly when its associated process shuts down successfully, restoring the terminal and disabling mouse capture when needed.
  • Improved handling of interrupted requests so the displayed error includes the process’s exit status and available diagnostic output.
  • Initial interface updates are more consistent when multiple display channels are attached.

Walkthrough

The TUI now treats selected client EOF errors as clean shutdowns and restores terminal state during cleanup. The redraw path now builds and merges chrome events only when an attached channel is uninitialized.

Changes

TUI clean client exit

Layer / File(s)	Summary
Map broken-pipe requests to EOF crates/ox-tui/src/client.rs	A broken-pipe request write now uses the EOF error path. Other write failures still propagate unchanged.
Handle clean EOF in the run loop crates/ox-tui/src/lib.rs	Startup resize, redraw, and terminal-event errors are checked for clean EOF. The cleanup helper disables active mouse capture and restores the terminal. The run loop also has a targeted clippy::too_many_lines allowance.

Redraw chrome events

Layer / File(s)	Summary
Build initial chrome events conditionally crates/ox-ui/src/emitter.rs	The redraw path builds a chrome snapshot and merges current-pass events only when an attached channel is uninitialized. It deduplicates pending events against the snapshot.

Priority: ⬇️ Low

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 88c58

The changes improve clean TUI shutdown handling and avoid unnecessary chrome snapshot work. No actionable merge-blocking risk remains in the supplied evidence; merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 88c58

The changes improve clean-exit handling without demonstrating broader access or privileges. One shutdown edge case remains: waiting for transport readers after a broken pipe can delay terminal restoration indefinitely if inherited output pipes remain open.

Retained concerns

• Low · reliability · inferred: BrokenPipe now enters synchronous EOF cleanup before terminal restoration. Although child polling has a one-second deadline, transport-worker joins have no deadline. If a child descendant retains stdout or stderr after the child exits or is terminated, a reader can remain blocked and prevent terminal restoration. Previously, this write-failure path returned the transport error directly. The inferred exposure is limited to the local child-process lifecycle.

Security review details

Security Blast Radius

• inferred — The demonstrated lifecycle authority is over the client’s own spawned child and the current terminal session. The redraw change operates over the existing attached-channel registry. These paths do not establish tenant-wide, service-wide or environment-wide exposure.

Security Findings and Attack Paths

• inferred — The unresolved failure path requires local child-process behavior that closes RPC input while leaving an output pipe held open, potentially through a descendant. A racing request can then reach unbounded reader joins before terminal restoration. This is a conditional availability and cleanup concern; remote reachability and privilege escalation were not established.

Trust Boundaries and Controls

• observed — Request mutation remains serialized through mutable client access. Responses must match the outstanding request identity, and unsupported inbound requests are rejected. The change adds EOF cleanup to write failures without changing these protocol checks.
• inferred — Retaining duplicate pending chrome events changes multiplicity, not the selected recipients or event authority. Existing channel options and first-redraw routing remain in place. Owner-level pairing of channel removal with emitter-cache cleanup was not established, so same-identity reattachment coverage remains incomplete.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title uses the Conventional Commits fix: prefix and accurately describes both main changes: quadratic chrome-event deduplication and clean-quit race fixes.
Description check	✅ Passed	The description directly explains the two defects, the implementation changes, and the reported test results. It is clearly related to the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 70.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[devin-ai-integration]

On the retained low-severity concern (unbounded join_worker after BrokenPipe → eof_error):

The exposure predates this change and is not widened materially by it. eof_error() already ran the same unbounded joins on every EOF path — the recv loop calls it on ReaderEvent::Eof, and Client::stop() does the same — so a descendant retaining the child's stdout would stall terminal restoration on a plain :qall today, regardless of this patch. The new path only adds write-side EPIPE detection, i.e. the same child-gone state reached earlier.

Reaching the stall also requires a descendant process that inherits the embed's RPC stdout/stderr. All embed-spawned work uses Stdio::piped()/Stdio::null() (job control, :terminal ptys, system()), so no oxvim-spawned descendant holds those pipes — the holder would have to be created outside oxvim's spawn paths.

A bounded join isn't a small change: std::thread::JoinHandle has no timeout, and the reader threads own the pipe read ends so they can't be unblocked from outside — it needs a channel/loop redesign of join_worker. Happy to take that up as a follow-up if you want the shutdown path hardened across all EOF callers rather than only this new one.

[devin-ai-integration]

E2E verification on a real terminal (konsole + tmux on DISPLAY :0, target/release/oxvim @ 88c582f, keys driven via tmux into the real pty):

:!<big-output> (emitter O(N²) fix): :!seq 1 50000 completed in ~2.1s (measured to a queued :echomsg marker that can only render after the bang returns), and the TUI inserted text immediately after — previously this wedged ~25s at ~98% CPU on the embed process.

Clean-quit race (client request fix): fired a continuous tmux resize-window storm at the pane while sending iEND<Esc>:wq<CR> — 5 runs, all exited 0 and wrote the buffer every time; no embedded editor closed its RPC stream error.

Regression sanity: file open/insert/save, :messages history float, :split per-window statuslines, floating : popup, bracketed paste (\e[200~…\e[201~ → nvim_paste path), :qa! → exit 0.