Repository navigation
Expose cmux events diagnostics - #3935
austinywang wants to merge 21 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR implements the ChangesEvent Diagnostics and Status Command
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant TerminalController
participant CmuxEventBus
participant CmuxEventLogWriter
CLI->>TerminalController: events.status RPC (reset_counters=true)
TerminalController->>CmuxEventBus: diagnosticsSnapshot(resetCounters: true)
CmuxEventBus->>CmuxEventBus: collect sequence/retained bounds
CmuxEventBus->>CmuxEventBus: snapshot active subscriptions
CmuxEventBus->>CmuxEventLogWriter: retrieve durable_log state
CmuxEventLogWriter->>CmuxEventLogWriter: return cached sizes and counters
CmuxEventLogWriter->>CmuxEventLogWriter: reset disk-only drop counter
CmuxEventBus->>CmuxEventBus: reset slow-close counter
CmuxEventBus-->>TerminalController: diagnostics dict
TerminalController-->>CLI: OK response with JSON payload
CLI->>CLI: print formatted JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning, 2 inconclusive)
✅ Passed checks (10 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 |
Greptile SummaryThis PR adds
Confidence Score: 5/5Safe to merge — all three threading and correctness issues from the prior review are resolved, and the new diagnostics path is cleanly isolated from the socket worker and main actor. The lock discipline is consistent throughout: CmuxEventBus and CmuxEventLogWriter each protect their mutable state with NSLock and release before calling out; cached log sizes are updated exclusively on the IO queue; diagnosticsSnapshot() snapshots all values under the lock then does heavier subscription mapping outside it. The nonisolated annotation on v2Bool is correct since the method reads only from its parameters. No actor isolation mistakes, blocking primitives on the wrong queue, or data races were found. No files require special attention. Important Files Changed
Reviews (17): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CLI/cmux.swift`:
- Line 9309: The help text for the "cmux events status" subcommand omits an
explanation for the "--reset-counters" flag; update the status subcommand's help
block (the "cmux events status" usage/help generation code) to add an Options
section documenting "--reset-counters" and its effect, e.g., " --reset-counters
Reset resettable diagnostic counters (slow_subscription_close_count,
dropped_disk_only_line_count)"; locate the help string or function that prints
the status subcommand help (the code that emits the usage line "cmux events
status [--reset-counters]") and insert this brief option description there so
users see it inline with other status options.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cec631cf-4e23-49bf-87db-31c0561d209e
📒 Files selected for processing (3)
CLI/cmux.swiftSources/TerminalController.swiftdocs/cli-contract.md
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CLI/CMUXCLI`+Events.swift:
- Line 136: The error currently throws CLIError with the raw arg interpolated
("Unknown events status option: \(arg)"), which can leak secrets; change the
thrown message to a generic user-facing message such as "Unknown events status
option; run --help for valid options" and avoid including arg in the message,
while optionally logging the raw arg to a debug log only. Update the throw site
where CLIError is constructed (the Unknown events status option branch in the
events/status option handling) to use the generic message and, if needed, emit
the raw arg to a non-user-facing debug/logger call instead of the exception.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70c1d226-dba9-436e-9d64-9e20d510c21c
📒 Files selected for processing (3)
CLI/CMUXCLI+Events.swiftCLI/cmux.swiftSources/TerminalController.swift
Stale CodeRabbit review: the inline reset-counters help thread is resolved and CodeRabbit confirmed it was addressed after commit b648454.
Closes #3672.
Summary
cmux events statusandevents.statusdiagnostics for event bus retention, subscriptions, durable log paths/sizes, queue depth, and disk-only drops.ack.resume.gapand add diagnostics unit coverage.Testing
Note
Medium Risk
Adds a new socket RPC and CLI subcommand that surface internal event-bus and durable-log counters; risk is mainly around concurrency/locking and correctness of the new diagnostics/counter-reset behavior.
Overview
Adds
cmux events status [--reset-counters]and a new v2 socket methodevents.statusto fetch event-bus diagnostics as JSON (retention bounds/counts, active subscriptions with pending depth/limits, and durable event-log paths/sizes/queue depth plus disk-only drop counters).Implements new internal tracking for slow-subscriber closures and refactors the durable log writer to maintain cached file sizes and separate reported vs logged drop counters, with optional counter reset. Updates capabilities/help/docs accordingly and adds unit tests covering the diagnostics payload, null sequence bounds, and reset semantics.
Reviewed by Cursor Bugbot for commit 0e80a2a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds
cmux events statusand theevents.statusv2 RPC to print event-bus diagnostics as JSON, with an option to reset counters. Covers retention window, active subscriptions, and durable log paths/sizes/queue state; advertised incapabilitiesand CLI help. Implements #3672.New Features
cmux events status [--reset-counters]andevents.status; CLI prints JSON only.boot_id,oldest_seq/latest_seq(null when empty)/next_seq, retained and active subscription counts, per-subscription names/categories/pending/max,slow_subscription_close_count, limits, and durable log enabled flag, paths, cached current/rotated sizes, queue depth/limit, and disk-only drop count.Bug Fixes
reset_countersparsing from the socket worker; align disabled durable-log field types; usenullfor empty sequence bounds.Written for commit 0e80a2a. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
cmux events statusCLI subcommand (JSON output) with--reset-counters.events.statusmade available via the socket RPC interface.Documentation
Tests