Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/CmuxTopProcessEnumeration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ nonisolated extension CmuxTopProcessSnapshot {
let path = includeProcessDetails ? processPath(pid: pid) : nil
let rawTTY = Int64(bsdInfo.e_tdev)
let ttyDevice = rawTTY > 0 ? rawTTY : nil
let cmuxScope = cachedCMUXScope(for: pid, cacheKey: cacheKey)
let cmuxScope = cachedCMUXScope(for: pid, cacheKey: cacheKey, nowNanoseconds: sampledAtNanoseconds)
let rawProcessGroupID = Int(bsdInfo.pbi_pgid)
let processGroupID = rawProcessGroupID > 0 ? rawProcessGroupID : nil
let rawTerminalProcessGroupID = Int(bsdInfo.e_tpgid)
Expand Down
2 changes: 1 addition & 1 deletion Sources/CmuxTopSnapshot.swift
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ nonisolated struct CmuxTopProcessInfo: Sendable {
}
}

nonisolated struct CmuxTopProcessScope: Sendable {
nonisolated struct CmuxTopProcessScope: Sendable, Equatable {
let workspaceID: UUID?
let surfaceID: UUID?
let attributionReason: String
Expand Down
121 changes: 100 additions & 21 deletions Sources/CmuxTopSnapshotScopeCache.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,32 @@ nonisolated struct CmuxTopProcessScopeCacheKey: Hashable {
}

private nonisolated struct CmuxTopProcessScopeCacheValue {
let scope: CmuxTopProcessScope
// 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
Comment on lines +12 to +19

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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

}

// How long a "no cmux scope" result stays cached before the process is probed
// again. The scope is derived from argv/environment, which an `exec` can change
// without changing the pid or process start time (the cache key), so a process
// first sampled in its fork-before-exec window, or one that execs into a
// `cmux hooks … monitor` later, must be re-probed eventually or it would never
// be attributed. The TTL bounds that attribution latency while still collapsing
// the per-poll sysctl storm for the steady-state majority of non-cmux processes.
private nonisolated let cmuxTopNegativeScopeTTLNanoseconds: UInt64 = 15 * 1_000_000_000

// Result of probing a single process for its cmux scope. `resolved` means the
// probe completed (the scope may legitimately be absent) and is safe to cache.
// `unavailable` means a transient failure (process exited mid-probe, pid reuse,
// or a failed sysctl) and must NOT be cached so the next poll retries.
nonisolated enum CmuxTopProcessScopeProbeResult: Equatable {
case resolved(CmuxTopProcessScope?)
case unavailable
}

// CmuxTopProcessSnapshot.capture is intentionally synchronous because it backs
Expand Down Expand Up @@ -40,21 +65,50 @@ nonisolated extension CmuxTopProcessSnapshot {

static func cachedCMUXScope(
for pid: Int,
cacheKey: CmuxTopProcessScopeCacheKey
cacheKey: CmuxTopProcessScopeCacheKey,
nowNanoseconds: UInt64,
probe: (Int, CmuxTopProcessScopeCacheKey) -> CmuxTopProcessScopeProbeResult = CmuxTopProcessSnapshot.cmuxScopeProbe
) -> CmuxTopProcessScope? {
if let cached = cmuxTopScopeCache.withLock({ cache in cache[cacheKey] }) {
return cached.scope
if let scope = cached.scope {
// Positive results never expire: a discovered cmux scope is stable.
return scope
}
if nowNanoseconds < cached.negativeExpiresAtNanos {
// Negative result still within its TTL: honor the cached miss.
return nil
}
// Negative TTL expired: fall through and re-probe in case the process
// execed into a cmux-scoped command since it was last sampled.
}

guard let scope = cmuxScope(for: pid, expectedCacheKey: cacheKey) else {
switch probe(pid, cacheKey) {
case .resolved(let scope):
// Cache the resolved result. Positive scopes are kept indefinitely;
// negative scopes are kept only until the TTL elapses so a later exec
// is eventually attributed. The key is pruned to live pids each
// capture, so a recycled pid gets a fresh key.
//
// capture() runs concurrently (async task-manager sampling and sync
// system.top socket handling), and the probe happened outside the
// lock. Never let a stale negative from an older capture clobber a
// positive scope a newer capture already discovered for the same
// process: if we probed nil but a positive entry now exists, keep and
// return it.
return cmuxTopScopeCache.withLock { cache -> CmuxTopProcessScope? in
if scope == nil, let existing = cache[cacheKey], let existingScope = existing.scope {
return existingScope
}
cache[cacheKey] = CmuxTopProcessScopeCacheValue(
scope: scope,
negativeExpiresAtNanos: nowNanoseconds &+ cmuxTopNegativeScopeTTLNanoseconds
)
return scope
}
case .unavailable:
// Transient failure: do not cache, retry on the next poll.
return nil
}

cmuxTopScopeCache.withLock { cache in
cache[cacheKey] = CmuxTopProcessScopeCacheValue(scope: scope)
}

return scope
}

static func pruneCMUXScopeCache(activeKeys: Set<CmuxTopProcessScopeCacheKey>) {
Expand All @@ -63,33 +117,58 @@ nonisolated extension CmuxTopProcessSnapshot {
}
}

private static func cmuxScope(
// Probes a single process for its cmux scope via sysctl.
//
// `KERN_PROCARGS2` can fail two very different ways: transiently, because the
// process exited mid-probe or its pid was reused (then `kinfoProc` no longer
// matches the expected start-time key), or permanently, because the process
// belongs to another user / is protected (the kernel denies procargs for the
// process's whole life). We must cache the permanent case as a definitive
// "no readable cmux scope" — those processes are not cmux-scoped and stay in
// `activeKeys`, so leaving them uncached would re-run this sysctl fan-out on
// every poll. We must NOT cache the transient case, so a process that is
// simply mid-exec is retried.
//
// The discriminator is liveness: if a procargs read fails but the process is
// still alive with the same start time, the failure is a permanent denial and
// resolves to nil; otherwise it raced with exit/reuse and is `.unavailable`.
static func cmuxScopeProbe(
for pid: Int,
expectedCacheKey: CmuxTopProcessScopeCacheKey
) -> CmuxTopProcessScope? {
guard let currentProcess = kinfoProc(for: pid),
scopeCacheKey(from: currentProcess) == expectedCacheKey else {
return nil
) -> CmuxTopProcessScopeProbeResult {
guard processMatchesKey(pid, expectedCacheKey) else {
return .unavailable
}

var mib: [Int32] = [CTL_KERN, KERN_PROCARGS2, Int32(pid)]
var size: size_t = 0
guard sysctl(&mib, u_int(mib.count), nil, &size, nil, 0) == 0,
size > MemoryLayout<Int32>.size else {
return nil
// Sizing failed: permanent denial if still alive, else exit race.
return processMatchesKey(pid, expectedCacheKey) ? .resolved(nil) : .unavailable
}

var buffer = [UInt8](repeating: 0, count: size)
let success = buffer.withUnsafeMutableBytes { rawBuffer in
sysctl(&mib, u_int(mib.count), rawBuffer.baseAddress, &size, nil, 0) == 0
}
guard success else { return nil }
guard let currentProcess = kinfoProc(for: pid),
scopeCacheKey(from: currentProcess) == expectedCacheKey else {
return nil
guard success else {
return processMatchesKey(pid, expectedCacheKey) ? .resolved(nil) : .unavailable
}
guard processMatchesKey(pid, expectedCacheKey) else {
return .unavailable
}

return cmuxScope(fromKernProcArgs: Array(buffer.prefix(Int(size))))
return .resolved(cmuxScope(fromKernProcArgs: Array(buffer.prefix(Int(size)))))
}

// True when `pid` is still the same process (same start time) as the key.
private static func processMatchesKey(
_ pid: Int,
_ expectedCacheKey: CmuxTopProcessScopeCacheKey
) -> Bool {
guard let process = kinfoProc(for: pid) else { return false }
return scopeCacheKey(from: process) == expectedCacheKey
}

static func cmuxScope(fromKernProcArgs bytes: [UInt8]) -> CmuxTopProcessScope? {
Expand Down
4 changes: 4 additions & 0 deletions cmux.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@
C7A512000000000000000002 /* CmuxTopProcessSnapshotCache.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7A512000000000000000001 /* CmuxTopProcessSnapshotCache.swift */; };
C7A501000000000000000002 /* CmuxTopSnapshot.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7A501000000000000000001 /* CmuxTopSnapshot.swift */; };
C7A508000000000000000002 /* CmuxTopSnapshotScopeCache.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7A508000000000000000001 /* CmuxTopSnapshotScopeCache.swift */; };
C7A5090000000000000005A2 /* CmuxTopSnapshotScopeCacheTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7A5090000000000000005A1 /* CmuxTopSnapshotScopeCacheTests.swift */; };
C7A509000000000000000002 /* CmuxTopSnapshotScopeTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C7A509000000000000000001 /* CmuxTopSnapshotScopeTests.swift */; };
CDFEED0300000000CDFEED03 /* CmuxUpdater in Frameworks */ = {isa = PBXBuildFile; productRef = CDFEED0200000000CDFEED02 /* CmuxUpdater */; };
CDFEED0600000000CDFEED06 /* CmuxUpdaterUI in Frameworks */ = {isa = PBXBuildFile; productRef = CDFEED0500000000CDFEED05 /* CmuxUpdaterUI */; };
Expand Down Expand Up @@ -979,6 +980,7 @@
C7A512000000000000000001 /* CmuxTopProcessSnapshotCache.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CmuxTopProcessSnapshotCache.swift; sourceTree = "<group>"; };
C7A501000000000000000001 /* CmuxTopSnapshot.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CmuxTopSnapshot.swift; sourceTree = "<group>"; };
C7A508000000000000000001 /* CmuxTopSnapshotScopeCache.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CmuxTopSnapshotScopeCache.swift; sourceTree = "<group>"; };
C7A5090000000000000005A1 /* CmuxTopSnapshotScopeCacheTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CmuxTopSnapshotScopeCacheTests.swift; sourceTree = "<group>"; };
C7A509000000000000000001 /* CmuxTopSnapshotScopeTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CmuxTopSnapshotScopeTests.swift; sourceTree = "<group>"; };
7E7E6EF344A568AC7FEE3715 /* cmuxUITests.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = cmuxUITests.xctest; sourceTree = BUILT_PRODUCTS_DIR; };
D7AB0000000000000000000A /* CmuxWebView+MoveTabToNewWorkspace.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Panels/CmuxWebView+MoveTabToNewWorkspace.swift"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -2229,6 +2231,7 @@
C7A507000000000000000001 /* TaskManagerResourcesTests.swift */,
C7A507000000000000004530 /* TaskManagerViewSnapshotBoundaryTests.swift */,
C7A509000000000000000001 /* CmuxTopSnapshotScopeTests.swift */,
C7A5090000000000000005A1 /* CmuxTopSnapshotScopeCacheTests.swift */,
C7A50C000000000000000001 /* CmuxTopProcessCPUTests.swift */,
D7AB34300000000000000006 /* SidebarWorkspaceDropPlannerTests.swift */,
D7AB34300000000000000106 /* SidebarTabDropIndicatorPredicateTests.swift */,
Expand Down Expand Up @@ -3213,6 +3216,7 @@
C0DE31390000000000000101 /* CMUXOpenCommandTests.swift in Sources */,
C3677001000000000000001 /* CmuxSSHURLRequestTests.swift in Sources */,
C7A50C000000000000000002 /* CmuxTopProcessCPUTests.swift in Sources */,
C7A5090000000000000005A2 /* CmuxTopSnapshotScopeCacheTests.swift in Sources */,
C7A509000000000000000002 /* CmuxTopSnapshotScopeTests.swift in Sources */,
D0B1000CA1B2C3D4E5F60001 /* CmuxWebViewDragRoutingTests.swift in Sources */,
7D5DA4DC51454ECDBF9AC978 /* CmuxWebViewMouseNavigationButtonTests.swift in Sources */,
Expand Down
Loading
Loading