Repository navigation
Cache negative cmux-scope results so system.top stops re-probing every process per poll - #5759
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughCaches definitive CMUX probe outcomes (including explicit nil), separates transient failures via a probe-result enum, extracts probing into cmuxScopeProbe, makes cachedCMUXScope time-aware and injectable, updates caller usage to pass timestamps, and adds tests plus Xcode wiring for cache semantics. ChangesCMUX Scope Caching
Sequence DiagramsequenceDiagram
participant ProcessEnumerator
participant CmuxTopProcessSnapshot
participant CMUXScopeCache
participant Sysctl
ProcessEnumerator->>CmuxTopProcessSnapshot: request cachedCMUXScope(pid, cacheKey, now)
CmuxTopProcessSnapshot->>CMUXScopeCache: lookup(cacheKey)
alt cache hit (resolved)
CMUXScopeCache-->>CmuxTopProcessSnapshot: return scopeOrNil
CmuxTopProcessSnapshot-->>ProcessEnumerator: return scopeOrNil
else cache miss
CmuxTopProcessSnapshot->>CmuxTopProcessSnapshot: cmuxScopeProbe(pid, cacheKey)
CmuxTopProcessSnapshot->>Sysctl: read KERN_PROC_PID and KERN_PROCARGS2
Sysctl-->>CmuxTopProcessSnapshot: procinfo/procargs
alt probe success
CmuxTopProcessSnapshot->>CMUXScopeCache: store resolved(scopeOrNil) [cache only .resolved]
CmuxTopProcessSnapshot-->>ProcessEnumerator: return scopeOrNil
else probe unavailable
CmuxTopProcessSnapshot-->>ProcessEnumerator: return nil (do not cache)
end
end
🎯 4 (Complex) | ⏱️ ~45 minutes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 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 fixes the leading on-CPU hot spot in
Confidence Score: 5/5Safe to merge. The change is narrowly scoped to the negative-result caching path, all five edge cases are covered by injectable-probe unit tests, and the concurrent nil-clobber guard is correct. The lock scope is kept intentionally minimal (dict reads/writes only; sysctls happen outside), the No files require special attention. Important Files Changed
Reviews (6): Last reviewed commit: "fix(system.top): cache negative cmux-sco..." | Re-trigger Greptile |
0feeaa7 to
d012eb7
Compare
|
Note on |
d012eb7 to
1923690
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1923690d02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard sysctl(&mib, u_int(mib.count), nil, &size, nil, 0) == 0, | ||
| size > MemoryLayout<Int32>.size else { | ||
| return nil | ||
| return .unavailable |
There was a problem hiding this comment.
When KERN_PROCARGS2 fails permanently for protected/other-user processes (for example long-lived system daemons), this path returns .unavailable, and cachedCMUXScope deliberately skips writing any cache entry for .unavailable. Those processes are therefore still re-probed on every system.top poll, preserving the sysctl storm this change is meant to eliminate for a common class of non-cmux processes; only genuinely transient cases such as ESRCH/pid reuse should retry rather than caching a negative result.
Useful? React with 👍 / 👎.
1923690 to
676ed82
Compare
![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 `@Sources/CmuxTopSnapshotScopeCache.swift`:
- Around line 12-19: The positive cache entries currently never expire which
lets an exec-changing process keep an old CmuxTopProcessScope indefinitely; add
a staleness policy for positive hits by adding a positiveExpiresAtNanos (UInt64)
timestamp to the cache entry (in the same type that currently has scope and
negativeExpiresAtNanos), set it when inserting positive scopes (e.g. use a short
TTL for scopes derived from cmux-hook-arguments but keep longer/never for truly
inherited stable sources), and update the lookup logic that currently checks
negativeExpiresAtNanos to also treat a positive entry as expired if now >
positiveExpiresAtNanos (or if the source requires revalidation), causing callers
to re-probe instead of returning the stale scope from functions/methods that
reference CmuxTopSnapshotScopeCache, scope, and negativeExpiresAtNanos.
🪄 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: 8f9790ee-c407-4c26-94d1-ff640a9bcc7a
📒 Files selected for processing (5)
Sources/CmuxTopProcessEnumeration.swiftSources/CmuxTopSnapshot.swiftSources/CmuxTopSnapshotScopeCache.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CmuxTopSnapshotScopeCacheTests.swift
| // nil means "this process was probed and has no cmux scope". A negative entry | ||
| // is honored as a hit only until `negativeExpiresAtNanos`, so a non-cmux | ||
| // process is re-probed at most once per TTL window instead of on every | ||
| // system.top poll. Positive entries never expire (`negativeExpiresAtNanos` is | ||
| // ignored when `scope != nil`): a cmux scope comes from inherited environment | ||
| // or a stable argv and does not disappear for the process lifetime. | ||
| let scope: CmuxTopProcessScope? | ||
| let negativeExpiresAtNanos: UInt64 |
There was a problem hiding this comment.
Don't keep positive scope hits forever on an exec-stable cache key.
Lines 23-26 already note that exec can change argv/environment without changing (pid, start time), but Lines 73-76 return any positive hit indefinitely. That leaves cmux-hook-arguments scopes stale if a process later execs away from the cmux hooks … monitor argv: subsequent system.top polls will keep attributing the new program to the old workspace/surface for the rest of that process lifetime. Positive entries need their own staleness policy, or at least source-specific revalidation, instead of unconditional indefinite reuse.
As per coding guidelines, cache substitutions in snapshot paths must explicitly handle stale caches, not just cold misses.
Also applies to: 72-107
🤖 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 `@Sources/CmuxTopSnapshotScopeCache.swift` around lines 12 - 19, The positive
cache entries currently never expire which lets an exec-changing process keep an
old CmuxTopProcessScope indefinitely; add a staleness policy for positive hits
by adding a positiveExpiresAtNanos (UInt64) timestamp to the cache entry (in the
same type that currently has scope and negativeExpiresAtNanos), set it when
inserting positive scopes (e.g. use a short TTL for scopes derived from
cmux-hook-arguments but keep longer/never for truly inherited stable sources),
and update the lookup logic that currently checks negativeExpiresAtNanos to also
treat a positive entry as expired if now > positiveExpiresAtNanos (or if the
source requires revalidation), causing callers to re-probe instead of returning
the stale scope from functions/methods that reference CmuxTopSnapshotScopeCache,
scope, and negativeExpiresAtNanos.
Source: Coding guidelines
676ed82 to
e3b9c5e
Compare
… probes Adds a probe-injection seam and monotonic-clock plumbing to CmuxTopProcessSnapshot.cachedCMUXScope, classifies permanent procargs denials (other-user/protected processes) as cacheable negatives vs transient exit races, and adds a new wired Swift Testing suite (CmuxTopSnapshotScopeCacheTests, serialized for the shared cache) covering: negatives cached within a TTL (not re-probed every poll), an expired negative re-probed after exec, transient failures retried, and a stale negative never clobbering a concurrently discovered positive scope. This commit keeps the buggy behavior (resolved-nil results are not cached) so the new tests fail; the fix follows in the next commit. Issue: #5756 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…p per-poll sysctl storm CmuxTopProcessSnapshot.cachedCMUXScope only cached positive scope lookups, so every process without a cmux scope (the vast majority on a busy host: system daemons, helpers, and agent processes with no CMUX_WORKSPACE_ID/CMUX_SURFACE_ID) was a permanent cache miss. Each system.top / system.memory poll then re-ran the scope probe (two sysctl(KERN_PROC_PID) + one sysctl(KERN_PROCARGS2)) for every such process. Under a heavy local agent fleet this was the single largest on-CPU consumer in a live sample of a beachballing build (hundreds of processes × 3 syscalls × poll rate). Cache the resolved nil result too, but under a 15s TTL rather than for the process lifetime. The cache key is (pid, process start time), which an `exec` does not change, and the scope is derived from argv/environment, which an exec can change. Caching the negative forever would mean a process first sampled in its fork-before-exec window (or one that execs into a `cmux hooks … monitor` later, e.g. a launchd-parented monitor) would never be attributed. The TTL bounds that attribution latency to 15s while still collapsing the steady-state per-poll storm. Positive scopes are still cached indefinitely. The probe (cmuxScopeProbe) also distinguishes a permanent KERN_PROCARGS2 denial (other-user/protected process that is still alive) — cached as a definitive "no readable cmux scope" — from a transient failure (process exited mid-probe / pid reuse), which stays uncached. Without this, every root/other-user daemon would remain a permanent cache miss and keep the sysctl fan-out unbounded. capture() runs concurrently (async task-manager sampling and sync system.top socket handling) and the probe runs outside the cache lock, so the post-probe write is conditional: a resolved-nil result never overwrites a positive scope a concurrent capture already discovered, and returns that positive instead. Drops steady-state per-poll cost from O(non-cmux processes × 3 syscalls) to roughly O(non-cmux processes × 3 syscalls / (TTL / poll interval)). Closes #5756 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e3b9c5e to
79d3456
Compare
This branch was successfully deployed
1 participant
Summary
system.topre-probed every non-cmux process with up to 3sysctlcalls on every poll, becauseCmuxTopProcessSnapshot.cachedCMUXScopeonly cached positive scope lookups. Under a heavy local agent fleet this was the single largest on-CPU consumer in a livesampleof a beachballing stable build (v0.64.14) — hundreds of processes × 3 syscalls × poll rate.This caches negative results too, with the nuance needed to stay correct:
(pid, process start time), which anexecdoes not change, while scope is derived from argv/environment, which an exec can change. A lifetime negative cache would permanently mis-attribute a process first sampled in its fork-before-exec window, or one that execs into acmux hooks … monitorlater (e.g. a launchd-parented monitor). The TTL bounds attribution latency to 15s while still collapsing the steady-state per-poll storm. Positive scopes stay cached indefinitely (inherited-env / stable-argv scope doesn't disappear).KERN_PROCARGS2fails permanently for other-user / protected processes (not just on exit races). The probe now distinguishes a permanent denial (process still alive → cache nil) from a transient exit/pid-reuse race (stays uncached). Without this, every root/other-user daemon would remain a permanent cache miss and keep the sysctl fan-out unbounded.capture()runs concurrently (async task-manager sampling and syncsystem.topsocket handling) and the probe runs outside the cache lock, so the post-probe write refuses to overwrite a positive scope a concurrent capture already discovered, and returns that positive.Drops steady-state per-poll cost from O(non-cmux processes × 3 syscalls) to roughly O(non-cmux processes × 3 syscalls ÷ (TTL ÷ poll interval)).
Testing
New wired Swift Testing suite
cmuxTests/CmuxTopSnapshotScopeCacheTests(.serialized, probe-injection + monotonic-clock seam): negatives cached within the TTL (probed once per window, not per poll), positives cached indefinitely, transient failures retried, an expired negative re-probed after exec, and a stale negative never clobbering a concurrently discovered positive. Two-commit red/green: commit 1 adds the seam + tests with the buggy behavior; commit 2 enables the TTL negative caching.Three Codex autoreview rounds drove the design: a lifetime negative cache (exec problem) → 15s TTL; an unconditional write (concurrency clobber) → conditional write; transient-only failure classification (procargs denial leak) → permanent-denial caching.
Notes
The only
workflow-guard-testsfailure is the pre-existingAgentLaunchSanitizer.swiftfile-length-budget overflow onmain(untouched here;main's own CI already fails this step). This PR's additions stay within budget in a separate wired test file.Issues