Repository navigation
mux: purge per-surface agent/notification tables on close (follow-up to #7584) - #7593
Merged
Merged
Conversation
…view findings - Purge agent_records + surface_notifications when a surface is closed or reaped (close_surface/close_surfaces), fixing an unbounded per-SurfaceId leak and stale list-agents results for dead tabs. Flagged by Greptile, Cursor Bugbot, and CodeRabbit. Adds a regression test. - Conformance fixture implemented-verbs-json gains a requires block so it skips gracefully on pre-protocol-6 servers instead of failing hard. - Sidebar unread dot now colors by notification severity, matching the tab-bar cue instead of always using notification_info. - Escape pipe chars in spec/cli.md verb table so it renders correctly.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR purges agent and notification side tables when surfaces close in mux-core, refactors the TUI sidebar's unread indicator into a severity-ranked color helper (error/warning/info), adds a requires constraint to a conformance fixture, and escapes pipe characters in CLI spec documentation tables. ChangesSurface Side-Table Cleanup
Sidebar Unread Indicator
Conformance Fixture and CLI Docs
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Mux
participant AgentRecords
participant SurfaceNotifications
Caller->>Mux: close_surface(surface_id)
Mux->>Mux: purge_surface_side_tables(surface_id)
Mux->>AgentRecords: remove entries for surface_id
Mux->>SurfaceNotifications: remove entries for surface_id
Mux->>Mux: kill surface
Mux-->>Caller: emit MuxEvent::TreeChanged
Possibly related PRs
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
Contributor
Greptile SummaryThis PR cleans up per-surface mux state and small UI/docs follow-ups.
Confidence Score: 5/5The changed flow looks mergeable after a small shutdown cleanup.
mux/crates/mux-core/src/mux.rs Important Files Changed
Reviews (1): Last reviewed commit: "mux: purge per-surface agent/notificatio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+424
to
+427
| fn purge_surface_side_tables(&self, surface: SurfaceId) { | ||
| self.agent_records.lock().unwrap().remove(&surface); | ||
| self.surface_notifications.lock().unwrap().remove(&surface); | ||
| } |
Contributor
There was a problem hiding this comment.
When shutdown() kills the remaining surfaces, it does not call this cleanup path. If the Mux is still queried during teardown, list_agents and surface_notification can still return entries for surfaces that were just killed.
This branch was successfully deployed
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.
Follow-up to #7584, which merged before its review-bot findings could be folded in.
Real bug (flagged independently by Greptile P2, Cursor Bugbot, and CodeRabbit): the new per-surface
agent_recordsandsurface_notificationsmaps are populated byreport-agent/notifybut were never purged when a surface closes or is reaped. BecauseSurfaceIdis monotonic, every closed tab leaked an entry for the daemon's whole lifetime, and unfilteredlist-agentskept returning dead surfaces as live agents. This addspurge_surface_side_tables, called fromclose_surface/close_surfaces, plus a regression test asserting both side tables drop the surface on close while sibling tabs survive.Also folds in the smaller review findings:
implemented-verbs-jsongains arequiresblock (the 8 protocol-6 verbs) so it skips gracefully on older servers instead of failing hard, matching the existingmove-tabfixture convention.notification_info.spec/cli.mdverb table so it renders as a table.cargo test -p mux-core -p mux-tuigreen (104 tests);cargo fmt --checkclean.Note
Medium Risk
Touches the surface-close path in mux-core and fixes stale agent/notification state that affected control-socket behavior; UI and doc tweaks are low risk.
Overview
Fixes a daemon-lifetime leak where
agent_recordsandsurface_notificationswere never cleared when a tab closed, solist-agentscould keep reporting dead surfaces and maps grew without bound.mux-core adds
purge_surface_side_tablesand invokes it fromclose_surfaceandclose_surfaces, with a regression test that agent and notification entries disappear while a sibling tab remains.Conformance
implemented-verbs-jsonnow declares arequires.commandslist (protocol-6 verbs) so older servers skip the fixture instead of failing.mux-tui sidebar unread dots use the highest-severity unread color (error > warning > info), aligned with the tab bar.
spec/cli.md escapes
|in verb-table cells so Markdown renders correctly.Reviewed by Cursor Bugbot for commit 2cbd7d2. Bugbot is set up for automated code reviews on this repo. Configure here.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes a leak and stale agent listings by purging per-surface
agent_recordsandsurface_notificationswhen a surface closes. Also aligns the sidebar unread dot with notification severity, updates the conformance fixture to skip on older servers, and fixes CLI spec table rendering.purge_surface_side_tablesand call it fromclose_surface/close_surfacesto remove closed surfaces from side tables, preventing unbounded growth and keepinglist-agentsaccurate; includes a regression test.implemented-verbs-jsonadds arequiresblock for protocol-6 verbs to skip gracefully on older servers; escape|inspec/cli.mdverb table for correct rendering.Written for commit 2cbd7d2. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation