Repository navigation
Fix git-metadata probe CPU storm: lock-free hex + concurrency cap - #5400
austinywang wants to merge 2 commits into
Conversation
…ee hex encoder gitIndexSnapshot formatted every 20-byte object ID via String(format: "%02x", $0), which takes a process-global locale lock (localeconv_l / os_unfair_lock) on every call. Across thousands of index entries and many workspace panels probing in parallel, that lock turned git-metadata probes into a multi-core CPU storm (#4639). A table-based encoder produces byte-identical lowercase hex, allocation-light and lock-free. Applies to the four hex sites: per-entry object ID, index checksum, index file signature, and the FNV content signature. Verified equivalent across all 256 byte values, ArraySlice/Data.suffix call shapes, and full-width UInt64; the existing index parser tests still pass unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
beginWorkspaceGitMetadataProbeAttempt spawns one detached .utility task per tracked panel with no global concurrency cap. A burst (workspace restore, the 5-minute fallback refresh, or a flurry of file-watch events) schedules one probe per panel at once; with dozens of panels that parses dozens of .git/index files in parallel and saturates every core (#4639). Add GitProbeConcurrencyLimiter (a bounded async gate that resumes waiters FIFO) in CmuxGit and route the single parse choke point, TabManager.initialWorkspaceGitMetadataSnapshot, through it capped at 4. Excess probes suspend until a slot frees instead of storming every core. Unit tests cover the cap under load, strict serialization at ceiling 1, and clamping a non-positive ceiling. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces backpressure control for concurrent git metadata probes and optimizes hex string generation in git index parsing. A new ChangesConcurrency Limiting and Hex Encoding Optimization
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)
✅ Passed checks (14 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 SummaryFixes the sidebar git-metadata CPU storm (issue #4639) with two targeted changes: a lock-free table-based hex encoder replaces
Confidence Score: 5/5Safe to merge; changes are performance-only with byte-identical observable behavior, fully covered by new tests, and do not touch auth, data models, or user-facing surfaces. Both changes are narrowly scoped: the hex encoder is a pure algorithmic substitution with identical output verified exhaustively, and the concurrency limiter's slot accounting is correct. The only known gaps — the withCheckedContinuation cancellation hole and the unused maxConcurrent property — were flagged in prior review threads and do not affect correctness under normal operation. No files require special attention beyond the items already discussed in prior review threads on GitProbeConcurrencyLimiter.swift. Important Files Changed
Sequence DiagramsequenceDiagram
participant TM as TabManager (nonisolated)
participant L as GitProbeConcurrencyLimiter (actor)
participant GMS as GitMetadataService
TM->>L: await gitProbeLimiter.run
alt slot available
L->>L: "available -= 1"
else all slots taken
L->>L: append continuation to waiters (FIFO)
L->>L: earlier probe calls release()
L->>L: waiters.removeFirst().resume()
end
L->>GMS: await workspaceMetadata(for:)
GMS-->>L: WorkspaceMetadata
L-->>TM: InitialWorkspaceGitMetadataSnapshot
L->>L: defer release()
Reviews (2): Last reviewed commit: "Cap concurrent git-metadata index parses..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func acquire() async { | ||
| if available > 0 { | ||
| available -= 1 | ||
| return | ||
| } | ||
| // Resumed by `release()`, which hands off its slot directly without | ||
| // bumping `available`, so the slot count stays balanced. | ||
| await withCheckedContinuation { waiters.append($0) } | ||
| } |
There was a problem hiding this comment.
withCheckedContinuation does not respond to Swift cooperative cancellation — a task whose Task.isCancelled flag is set while it is waiting in waiters stays parked until a slot is released and release() resumes it. When finally dequeued, body() runs to completion anyway (including the Data(contentsOf:) read inside workspaceMetadata). In a burst where several workspaces close while ~36 probes are queued, cancelled probes still absorb a slot, perform unnecessary disk I/O, and delay the legitimate probes behind them.
The fix is to wrap the suspension in withTaskCancellationHandler, removing the cancelled continuation from waiters and releasing the slot without running body().
| public actor GitProbeConcurrencyLimiter { | ||
| private let maxConcurrent: Int | ||
| private var available: Int | ||
| private var waiters: [CheckedContinuation<Void, Never>] = [] | ||
|
|
||
| /// Creates a limiter allowing at most `maxConcurrent` probes to run at once. | ||
| /// | ||
| /// - Parameter maxConcurrent: The concurrency ceiling. Values below 1 are | ||
| /// clamped to 1 so the limiter always makes forward progress. | ||
| public init(maxConcurrent: Int) { | ||
| let ceiling = max(1, maxConcurrent) | ||
| self.maxConcurrent = ceiling | ||
| self.available = ceiling | ||
| } |
There was a problem hiding this comment.
maxConcurrent is stored as an instance property but is never read after init. The live ceiling is tracked entirely by available; maxConcurrent is dead state. It can be removed.
| public actor GitProbeConcurrencyLimiter { | |
| private let maxConcurrent: Int | |
| private var available: Int | |
| private var waiters: [CheckedContinuation<Void, Never>] = [] | |
| /// Creates a limiter allowing at most `maxConcurrent` probes to run at once. | |
| /// | |
| /// - Parameter maxConcurrent: The concurrency ceiling. Values below 1 are | |
| /// clamped to 1 so the limiter always makes forward progress. | |
| public init(maxConcurrent: Int) { | |
| let ceiling = max(1, maxConcurrent) | |
| self.maxConcurrent = ceiling | |
| self.available = ceiling | |
| } | |
| public actor GitProbeConcurrencyLimiter { | |
| private var available: Int | |
| private var waiters: [CheckedContinuation<Void, Never>] = [] | |
| /// Creates a limiter allowing at most `maxConcurrent` probes to run at once. | |
| /// | |
| /// - Parameter maxConcurrent: The concurrency ceiling. Values below 1 are | |
| /// clamped to 1 so the limiter always makes forward progress. | |
| public init(maxConcurrent: Int) { | |
| self.available = max(1, maxConcurrent) | |
| } |
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@Packages/CmuxGit/Sources/CmuxGit/GitProbeConcurrencyLimiter.swift`:
- Around line 48-56: Update acquire() to support task cancellation by wrapping
the withCheckedContinuation wait in a withTaskCancellationHandler so cancelled
tasks are removed from the waiters queue; specifically, when calling
withCheckedContinuation in GitProbeConcurrencyLimiter.acquire(), register the
continuation object in waiters, and in the onCancel handler remove that specific
continuation (ensure removal and append are atomic relative to release()) so
cancelled continuations don't linger, and adjust release() if necessary to
handle the case where a continuation was removed. Use the existing symbols
acquire(), waiters, and release() to locate where to add the cancellation
handler and removal logic.
🪄 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: 00f38ac4-3485-41bc-a293-4e76daa31379
📒 Files selected for processing (5)
Packages/CmuxGit/Sources/CmuxGit/GitProbeConcurrencyLimiter.swiftPackages/CmuxGit/Sources/CmuxGit/Parsing/GitMetadataService+Index.swiftPackages/CmuxGit/Tests/CmuxGitTests/GitProbeConcurrencyLimiterTests.swiftPackages/CmuxGit/Tests/CmuxGitTests/HexEncodingTests.swiftSources/TabManager.swift
| private func acquire() async { | ||
| if available > 0 { | ||
| available -= 1 | ||
| return | ||
| } | ||
| // Resumed by `release()`, which hands off its slot directly without | ||
| // bumping `available`, so the slot count stays balanced. | ||
| await withCheckedContinuation { waiters.append($0) } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Optional enhancement: task cancellation support.
Cancelled tasks currently remain in the waiters queue until their slot is granted, potentially delaying other waiters. Consider adding withTaskCancellationHandler around the continuation to remove cancelled waiters from the queue:
await withTaskCancellationHandler {
await withCheckedContinuation { waiters.append($0) }
} onCancel: {
Task { await removeCancelledWaiter() }
}However, this adds complexity (tracking which continuation to remove, ensuring atomicity). The current simple design is acceptable given that git probe work is lightweight and short-lived.
🤖 Prompt for 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.
In `@Packages/CmuxGit/Sources/CmuxGit/GitProbeConcurrencyLimiter.swift` around
lines 48 - 56, Update acquire() to support task cancellation by wrapping the
withCheckedContinuation wait in a withTaskCancellationHandler so cancelled tasks
are removed from the waiters queue; specifically, when calling
withCheckedContinuation in GitProbeConcurrencyLimiter.acquire(), register the
continuation object in waiters, and in the onCancel handler remove that specific
continuation (ensure removal and append are atomic relative to release()) so
cancelled continuations don't linger, and adjust release() if necessary to
handle the case where a continuation was removed. Use the existing symbols
acquire(), waiters, and release() to locate where to add the cancellation
handler and removal logic.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
You’re at about 98% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/CmuxGit/Sources/CmuxGit/GitProbeConcurrencyLimiter.swift">
<violation number="1" location="Packages/CmuxGit/Sources/CmuxGit/GitProbeConcurrencyLimiter.swift:55">
P1: Handle cancellation while waiting in `acquire()`: a cancelled task currently stays queued until resumed and then still runs `body()`, which wastes a slot and can perform unnecessary git probe I/O.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| // Resumed by `release()`, which hands off its slot directly without | ||
| // bumping `available`, so the slot count stays balanced. | ||
| await withCheckedContinuation { waiters.append($0) } |
There was a problem hiding this comment.
P1: Handle cancellation while waiting in acquire(): a cancelled task currently stays queued until resumed and then still runs body(), which wastes a slot and can perform unnecessary git probe I/O.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/CmuxGit/Sources/CmuxGit/GitProbeConcurrencyLimiter.swift, line 55:
<comment>Handle cancellation while waiting in `acquire()`: a cancelled task currently stays queued until resumed and then still runs `body()`, which wastes a slot and can perform unnecessary git probe I/O.</comment>
<file context>
@@ -0,0 +1,65 @@
+ }
+ // Resumed by `release()`, which hands off its slot directly without
+ // bumping `available`, so the slot count stays balanced.
+ await withCheckedContinuation { waiters.append($0) }
+ }
+
</file context>
|
Superseded by #5402, which already implements the same two fixes for #4639 (table-based hex encoder replacing the String(format: "%02x") locale-lock hot loop, and a bounded concurrency limiter on the git-metadata probes) plus snapshot batching and coalescing. Closing this as redundant. The exhaustive all-256-byte hex equivalence test here may be worth porting, and #5402's limiter could be injected rather than a static let shared singleton. |
CodeRabbit now posts non-blocking comment reviews (request_changes_workflow=false, #5538).
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Two fixes for the sidebar git-metadata probe CPU storm that spikes the cmux app past 100% CPU and beachballs under many workspaces (#4639). Diagnosed from a live
sampleof the production app:gitTrackedChangesSnapshotandgitIndexSnapshotdominated the profile, running in parallel across ~36 panels.1. Lock-free hex encoder.
gitIndexSnapshotformatted every 20-byte object ID withString(format: "%02x", $0), which acquires a process-global locale lock (localeconv_l/os_unfair_lock) on each call. Across thousands of index entries and dozens of probes running in parallel, that lock turned into a multi-core CPU storm. The four hex sites (object ID, index checksum, file signature, FNV content signature) now use a table-based encoder with byte-identical lowercase output.2. Concurrency cap.
beginWorkspaceGitMetadataProbeAttemptspawned one detached.utilitytask per tracked panel with no global cap, so a burst (workspace restore, the 5-minute fallback refresh, a file-watch flurry) parsed dozens of.git/indexfiles at once. NewGitProbeConcurrencyLimiter(a bounded async gate that resumes waiters FIFO) wraps the single parse choke point,TabManager.initialWorkspaceGitMetadataSnapshot, capped at 4. Excess probes suspend until a slot frees instead of storming every core.Tests (CmuxGit, Swift Testing):
HexEncodingTests: the encoder matchesString(format:)across all 256 byte values, the ArraySlice/Data.suffix call shapes, and full-width UInt64. The existing index parser tests pass unchanged, confirming byte-identical output.GitProbeConcurrencyLimiterTests: never exceeds the cap under 60 concurrent tasks, strictly serializes at ceiling 1, and clamps a non-positive ceiling.Not in this PR: the
browser.eval/browser.waitmain-thread beachball (#4363) is a separate browser-automation-wide threading refactor (~35 call sites route WebView access through the sharedv2BrowserWithPanelhelper under WKWebView main-actor constraints). Tracked as a follow-up.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Performance-only changes to git metadata parsing and probe scheduling; behavior is preserved via byte-identical hex output and tests, with no auth or data-model changes.
Overview
Addresses multi-core CPU spikes from parallel sidebar git-metadata probes (issue #4639) with two targeted changes.
Index parsing no longer uses per-byte
String(format: "%02x")/%016llxwhen building object IDs, checksums, file signatures, and FNV content hashes. Those call sites now use a table-basedlowercaseHexStringso hex output stays the same without hitting the process-global locale lock under heavy parallel index reads.Probe scheduling adds a shared
GitProbeConcurrencyLimiter(FIFO async gate, minimum ceiling 1) and wrapsTabManager.initialWorkspaceGitMetadataSnapshotat 4 concurrent parses so workspace restore, fallback refresh, or watch bursts cannot run unbounded.git/indexparses across every panel.New Swift Testing suites assert the encoder matches
printffor all byte shapes used in production and that the limiter respects its cap, serializes at 1, and clamps invalid ceilings.Reviewed by Cursor Bugbot for commit e1677a7. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes CPU spikes from git-metadata probes by removing a locale-locked hex path and capping concurrent index parses. Keeps the app responsive during workspace restores and file-watch bursts.
Bug Fixes
String(format: "%02x")with a lock-free hex encoder inCmuxGitfor object IDs, index checksum, file signature, and FNV content signature. Output is byte-identical.GitProbeConcurrencyLimiterand routedTabManager.initialWorkspaceGitMetadataSnapshotthrough it (cap: 4) to avoid dozens of parallel.git/indexparses.Tests
Written for commit e1677a7. Summary will update on new commits.
Summary by CodeRabbit