Repository navigation
Reduce UI hot paths with many resumed Codex sessions - #4103
Closed
austinywang wants to merge 30 commits into
Closed
austinywang wants to merge 30 commits into
austinywang wants to merge 30 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ine-ui-lag-many-codex
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds owner-PID lifecycle handling and unresolved-transcript timeout to the Codex monitor, precomputes window-move snapshots for the workspace sidebar and flows them into rows/menus, refactors sidebar socket commands to use scheduled mutations with log deduplication, and updates a vendor submodule. ChangesOwner Process Lifecycle Monitoring for Codex Monitor
Precomputed Window Move Target Snapshots in Sidebar
Sidebar Socket Command Refactoring
Dependency Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Around line 16793-16799: The owner-PID parsing silently accepts non-positive
or sentinel values and codexMonitorOwnerProcessIsGone(_: ) treats them as “owner
always alive”; modify the parse/validation where
optionValue(...).flatMap(Int.init) is used to reject or warn on invalid owner
PIDs (<= 1) instead of passing them through, and add an explicit check/log when
a non-sane PID is provided so the process either fails closed or at minimum
emits a breadcrumb; keep codexMonitorOwnerProcessIsGone(ownerPID:) logic for
valid PIDs (>1) only and ensure any call-sites handle a nil/invalid parse by
returning an error or logging rather than relying on the guard to silently
disable owner tracking.
- Around line 16801-16808: The env override in
codexMonitorUnresolvedTranscriptTimeout currently clamps the parsed value to
Self.codexMonitorUnresolvedTranscriptTimeoutSeconds which prevents increasing
the timeout via CMUX_CODEX_MONITOR_UNRESOLVED_TRANSCRIPT_TIMEOUT_SECONDS; change
the logic to only enforce a sensible minimum (e.g., max(parsed, 0.05)) and
remove the upper clamp, or if a hard upper bound is required use a clearly named
constant (e.g., codexMonitorUnresolvedTranscriptTimeoutMaxSeconds or the
existing deadline value) and apply min(parsed,
codexMonitorUnresolvedTranscriptTimeoutMaxSeconds); additionally emit a
warning/log when an env-provided value is clipped so operators aren’t surprised.
🪄 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: be9da52e-ccb5-4c45-9a08-fcfdcda77eac
📒 Files selected for processing (6)
CLI/cmux.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/TerminalController.swifttests/test_codex_feed_hooks.pyvendor/bonsplit
…ine-ui-lag-many-codex
Contributor
Greptile SummaryThis PR reduces UI hot-path work when many Codex sessions are active and hardens the Codex monitor lifecycle. It addresses the lag reported in #4101 by removing avoidable app-wide scans from SwiftUI render work and tightening monitor process cleanup.
Confidence Score: 5/5Safe to merge — all identified issues from prior rounds are resolved, and the current diff introduces no new correctness problems. The Codex monitor changes are correct: owner-PID polling with a 1-second wait cap ensures prompt exit, NSCondition eliminates the missed-signal window that DispatchSemaphore had, and the unresolved-transcript timeout prevents indefinite orphans. The windowLabelsById implementation confirms labels are purely positional (index + referenceWindowId), so the fingerprint fully covers all label-affecting properties and the snapshot cache cannot go stale between notifications. The TerminalMutationBus pending queue is protected by a lock and mutations are enqueued synchronously, so drainPendingMutationsSynchronously called inside v2MainSync will always see all in-flight log/set_progress/clear_progress mutations before clear_log/list_log act — the ordering race from the earlier review is resolved. All 19 catalog locales are present for the new error string. No files require special attention. Important Files Changed
Reviews (23): Last reviewed commit: "merge: resolve latest main conflicts" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Sources/AppDelegate.swift`:
- Around line 4013-4030: The cache for windowMoveTargetSnapshots is only keyed
by referenceWindowId (WindowMoveTargetSnapshotCacheKey) but the produced
snapshots depend on window key/visible state via orderedMainWindowSummaries and
windowLabelsById, so update the caching strategy to include those determinants
(e.g., include a visibility/key-derived token or timestamp in
WindowMoveTargetSnapshotCacheKey) or explicitly invalidate entries when a
window's key/visible state changes (triggered alongside
mainWindowContextsDidChange); modify WindowMoveTargetSnapshotCacheKey and the
code paths that update windowMoveTargetSnapshotCache so cache keys reflect
visibility/key changes or ensure invalidation whenever
orderedMainWindowSummaries/windowLabelsById inputs change.
In `@tests/test_codex_feed_hooks.py`:
- Around line 546-550: The test currently treats a zero exit code as failure;
invert the check so the test expects the monitor to reject invalid --owner-pid
by exiting non‑zero. Change the condition on the result variable
(result.returncode) in the failing branch used by the block that raises
AssertionError (the f"hooks codex monitor failed..." message) so that it raises
when returncode == 0 instead of when returncode != 0, ensuring the test asserts
that an invalid owner PID is rejected early.
🪄 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: 53222dd4-1f91-493d-ba27-df54cfbfb8fc
📒 Files selected for processing (4)
CLI/cmux.swiftSources/AppDelegate.swiftSources/ContentView.swifttests/test_codex_feed_hooks.py
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@Sources/AppDelegate.swift`:
- Around line 846-847: The snapshot cache (windowMoveTargetSnapshotCache keyed
by WindowMoveTargetSnapshotCacheKey holding [WindowMoveTargetSnapshot]) is
unbounded because orderingFingerprint changes create new keys and old arrays
accumulate; change the cache to be bounded by replacing the per-key array with a
single snapshot or a fixed-size/LRU container (e.g., use NSCache with countLimit
or store WindowMoveTargetSnapshot instead of an array) and ensure entries are
pruned when mainWindowContexts changes; update all usages that read/write
windowMoveTargetSnapshotCache and any code interacting with
WindowMoveTargetSnapshotCacheKey, WindowMoveTargetSnapshot, and
mainWindowContexts to use the new replace-on-reference semantics so old
snapshots are dropped instead of retained.
In `@tests/test_codex_feed_hooks.py`:
- Line 495: Increase the exit timeout for the secondary PID-scan check to avoid
flakes: update the call to wait_for_monitor_pids(session_id, present=False,
timeout=1) to use a longer timeout (e.g., timeout=5) consistent with other
sibling calls and with the monitor.communicate(timeout=5) usage; this change
touches the invocation of wait_for_monitor_pids in the test and ensures the PID
scan has sufficient time on slow CI.
- Around line 466-478: This test uses time.time() to compute deadline and check
while time.time() < deadline which can be affected by wall-clock adjustments;
change both uses of time.time() in this snippet to time.monotonic() when
computing deadline and in the loop condition so the bounded wait uses a
monotonic clock (leave monitor.poll(), monitor.communicate(), fake.frames and
the existing break/AssertionError logic unchanged).
- Around line 423-463: The test spawns the owner sleep process (owner =
subprocess.Popen([...])) before the with FakeCmuxSocket(...) context and before
the try/finally that ensures cleanup, so if FakeCmuxSocket.__enter__ or the
monitor subprocess.Popen raises the owner can be leaked; move the owner creation
so it occurs after entering the FakeCmuxSocket context and inside the try that
contains the monitor Popen (or wrap the owner creation in its own try/finally)
so that the owner subprocess is always terminated in the existing finally block
that references owner. Ensure references to the owner variable remain valid for
the finally cleanup and that no early-raise path can leave the sleep process
running.
- Around line 521-552: Wrap the subprocess.run(...) call (the one assigning
result) in a try/except catching subprocess.TimeoutExpired and re-raise a
clearer AssertionError or RuntimeError that indicates the monitor timed out
(e.g., "hooks codex monitor timed out; likely still looping when invalid
--owner-pid was passed") so regressions produce an actionable failure; keep the
subsequent checks (result.returncode and fake.frames inspection) unchanged.
While editing the test, add additional invalid --owner-pid inputs (e.g., "0",
"-1", "abc") as separate subtests or parameterized cases to exercise
non-numeric/negative boundaries in addition to "1". Ensure references remain to
the existing subprocess.run invocation and the fake.frames check.
🪄 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: 251835f2-21d3-444e-b6f8-642db1f3021f
📒 Files selected for processing (3)
CLI/cmux.swiftSources/AppDelegate.swifttests/test_codex_feed_hooks.py
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c7a2816. Configure here.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
austinywang
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
May 13, 2026 14:09
Stale CodeRabbit change-request review from an older commit; findings were fixed, replied to, and latest CodeRabbit status is passing on 70f3042.
Contributor
Author
|
Addressed the Greptile async/sync log ordering finding in 522073f. |
…ine-ui-lag-many-codex
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
…ine-ui-lag-many-codex
lawrencecchen
added
the
stale-revisit
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
3 participants
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.
Addresses #4101.
Local repro evidence
User requested a local dev-build repro to inspect the lag directly. I used a tagged debug build:
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-4101-whole-machine-ui-lag-many-codex --launchBuild path:
/Users/austinwang/Library/Developer/Xcode/DerivedData/cmux-issue-4101-whole-machine-ui-lag-many-codex/Build/Products/Debug/cmux DEV issue-4101-whole-machine-ui-lag-many-codex.appSynthetic load approximated the report: 2 cmux windows, 14 workspaces, 68 surfaces, 10 Codex-like restore/session rows, and repeated progress/log/socket churn.
Baseline artifacts:
/tmp/issue-4101-local-repro/load-summary.json/tmp/issue-4101-local-repro/diagnostics-20260513121835/tmp/issue-4101-local-repro/latency-measurements.jsonPost-change artifacts:
/tmp/issue-4101-local-repro/post-sidebar-cache-diagnostics-20260513054346-state/tmp/issue-4101-local-repro/post-sidebar-cache-load-20260513054431/tmp/issue-4101-local-repro/final-local-load-20260513054846/diagnostics-20260513054921/tmp/issue-4101-local-repro/final-local-load-20260513054846/diagnostics-20260513054921-sample-cmux/tmp/issue-4101-local-repro/final-local-load-20260513054846/diagnostics-20260513054921-stateThe baseline sample had main-thread SwiftUI/AppKit layout work repeatedly walking cross-window move-menu state from
TabBarView.bodyand sidebar row context-menu bodies throughWorkspace.bonsplitTabMoveDestinations,AppDelegate.workspaceMoveTargets,AppDelegate.windowMoveTargets,windowLabelsById, andlistMainWindowSummaries.After the patch, the same sample counters dropped to zero for the expensive cross-window scans:
workspaceMoveTargets: 0bonsplitTabMoveDestinations: 0windowMoveTargets: 0windowLabelsById: 0The final load sample still shows real rendering cost in
TabBarView/Core Animation while terminal output is active, so this PR is intentionally the minimum architectural cut that removes the repro's avoidable app-wide scans and long-lived monitor/telemetry pressure.Changes
#3927 cross-reference
Ported/re-authored selected pieces from the direction in #3927:
Not ported in this minimum cut:
__hot-pathpooled socket subcommand.ProcessScannerreplacement forps/lsofsubprocesses.Dependency
Bonsplit
mainis protected, so the submodule commit is pushed to the same-org branch and opened as:manaflow-ai/bonsplit#122
The cmux submodule pointer references that remote-reachable Bonsplit commit.
Verification
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-4101-whole-machine-ui-lag-many-codex --launch.sample,top,vmmap,cmux top --all --processes, and terminal state artifacts above.Note
Medium Risk
Changes hot SwiftUI sidebar paths and Codex monitor child lifecycle; log/progress ordering now depends on synchronous mutation draining, though covered by a new test.
Overview
Targets #4101 UI lag under many resumed Codex sessions by cutting repeated cross-window work in the sidebar and tightening Codex monitor lifetimes.
Codex transcript monitor gains optional
--owner-pid(passed from hook start), exits when that process is gone (1s poll) or when a transcript stays missing past a configurable timeout (default 5m), validates owner PIDs with a localized CLI error, and uses anNSCondition-based waiter instead of a semaphore for file-change waits. CI runstest_codex_feed_hooks.pywith new owner-PID regression cases.Sidebar / socket path: workspace “move to window” menus use cached
WindowMoveTargetSnapshotlists (invalidated when main window contexts change) instead of rebuilding fullwindowMoveTargetsinside every row.log,set_progress, andclear_progressgo through the async sidebar mutation bus with no-op guards; identical log lines within 5s are skipped.list_log/clear_logsynchronously drain pending mutations so reads see queued writes; a unit test locks that ordering.Test harness: the
--versionmemory guard drops the 40k junk-app fixture for a FIFO “trap” sibling plist, usesresourceRSS and process-group kill on timeout.Reviewed by Cursor Bugbot for commit 0083a9d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Reduces UI lag when many Codex sessions resume by caching window move-target snapshots and building move menus only on demand. Hardens the Codex monitor with owner-PID lifecycle, strict validation, bounded waits, and reliable file-change waiting; addresses #4101.
Performance
.mainWindowContextsDidChange.log/set_progress/clear_progressthrough the async mutation path; dedupe identical log entries within 5s.list_log/clear_log; unit test verifies ordering.Bug Fixes
--owner-pidto Codex monitor; poll ~1s and exit when owner dies; reject invalid PIDs with a localized error; bound missing-transcript waits viaCMUX_CODEX_MONITOR_UNRESOLVED_TRANSCRIPT_TIMEOUT_SECONDS(default 5m).NSConditionwaiter to prevent missed signals.resourceRSS, and process-group kill on timeout.Written for commit 0083a9d. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Chores