From fe4b12d5adf9405683f3b4115ad90e1cb2b0f9ab Mon Sep 17 00:00:00 2001 From: LMZ Date: Tue, 21 Apr 2026 11:53:33 +0800 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20two-phase=20ps=20sampling=20?= =?UTF-8?q?=E2=80=94=20drop=20comm=3D=20from=20primary=20parse=20to=20avoi?= =?UTF-8?q?d=20space-sensitive=20field=20splitting=20(issue=20#29)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1: parse pid/cpu/rss via regex from `ps -axo pid=,%cpu=,rss=` (no comm=) Phase 2: fetch `ps -p -o comm=` concurrently for top-priority PIDs only Covered by 6 regression tests in Tests/ProcessBarMonitorTests/ --- Package.swift | 5 + .../ProcessSnapshotProvider.swift | 121 ++++++++++++++---- .../ProcessSnapshotProviderTests.swift | 115 +++++++++++++++++ 3 files changed, 219 insertions(+), 22 deletions(-) create mode 100644 Tests/ProcessBarMonitorTests/ProcessSnapshotProviderTests.swift diff --git a/Package.swift b/Package.swift index 2cf37e3..3324d58 100644 --- a/Package.swift +++ b/Package.swift @@ -23,6 +23,11 @@ let package = Package( resources: [ .process("Resources") ] + ), + .testTarget( + name: "ProcessBarMonitorTests", + dependencies: ["ProcessBarMonitor"], + path: "Tests/ProcessBarMonitorTests" ) ] ) diff --git a/Sources/ProcessBarMonitor/ProcessSnapshotProvider.swift b/Sources/ProcessBarMonitor/ProcessSnapshotProvider.swift index 3071a90..b041c8e 100644 --- a/Sources/ProcessBarMonitor/ProcessSnapshotProvider.swift +++ b/Sources/ProcessBarMonitor/ProcessSnapshotProvider.swift @@ -35,14 +35,14 @@ actor ProcessSnapshotProvider { } private let psPath = "/bin/ps" - private let psArguments = ["-axo", "pid=,comm=,%cpu=,rss="] + private let psArguments = ["-axo", "pid=,%cpu=,rss="] private let metadataRefreshInterval: TimeInterval = 30 private let maxMetadataLookupsPerSnapshot = 48 private let maxConcurrentMetadataLookups = 8 - private struct RawProcess: Sendable { + struct RawProcess: Sendable { let pid: Int - let command: String + let command: String? let rawCPUPercent: Double let memoryMB: Double } @@ -88,10 +88,27 @@ actor ProcessSnapshotProvider { } let prioritizedPIDs = prioritizedMetadataPIDs(from: rawProcesses) + // Phase 2: fetch command names for top PIDs (avoids the space-in-comm parsing problem) + let topPIDs = Array(prioritizedPIDs) + let fetchedCommands = await fetchCommands(for: topPIDs) + + // Fill commands back into rawProcesses so aggregate() / metadata have them + var rawProcessesWithCommand = rawProcesses + for i in rawProcessesWithCommand.indices { + if let cmd = fetchedCommands[rawProcessesWithCommand[i].pid] { + rawProcessesWithCommand[i] = RawProcess( + pid: rawProcessesWithCommand[i].pid, + command: cmd, + rawCPUPercent: rawProcessesWithCommand[i].rawCPUPercent, + memoryMB: rawProcessesWithCommand[i].memoryMB + ) + } + } + // Resolve metadata off the main thread with bounded concurrency let now = Date() let resolved = await resolveMetadataBatch( - rawProcesses: rawProcesses, + rawProcesses: rawProcessesWithCommand, prioritizedPIDs: prioritizedPIDs, now: now, maxConcurrent: maxConcurrentMetadataLookups @@ -101,8 +118,8 @@ actor ProcessSnapshotProvider { metadataCache[pid] = CachedMetadata(metadata: metadata, updatedAt: now) } - pruneMetadataCache(validPIDs: Set(rawProcesses.map(\.pid))) - return aggregate(rawProcesses) + pruneMetadataCache(validPIDs: Set(rawProcessesWithCommand.map(\.pid))) + return aggregate(rawProcessesWithCommand) } /// Resolves metadata for prioritized PIDs in parallel via TaskGroup with bounded concurrency. @@ -149,38 +166,49 @@ actor ProcessSnapshotProvider { /// Performs NSRunningApplication lookup off the main thread via Task.detached. /// Called from background tasks spawned by the TaskGroup in resolveMetadataBatch. - private static func resolveMetadataSync(pid: Int, command: String) -> AppMetadata { + private static func resolveMetadataSync(pid: Int, command: String?) -> AppMetadata { + let cmd = command ?? "" if let runningApp = NSRunningApplication(processIdentifier: pid_t(pid)) { - let appName = runningApp.localizedName ?? fallbackAppName(for: command) + let appName = runningApp.localizedName ?? fallbackAppName(for: cmd) return AppMetadata( appName: appName, bundleIdentifier: runningApp.bundleIdentifier, - commandKey: command + commandKey: cmd ) } return AppMetadata( - appName: fallbackAppName(for: command), + appName: fallbackAppName(for: cmd), bundleIdentifier: nil, - commandKey: command + commandKey: cmd ) } - private func parsePSOutput(_ raw: String) -> [RawProcess] { + // Regex: pid (digits), cpu (float, may be negative), rss (digits) — reliable, no spaces in these fields + private nonisolated static let psStatRegex = try! NSRegularExpression( + pattern: #"^\s*(\d+)\s+(-?[\d.]+)\s+(\d+)"#, + options: [] + ) + + nonisolated func parsePSOutput(_ raw: String) -> [RawProcess] { raw .split(whereSeparator: { $0.isNewline }) .compactMap { line -> RawProcess? in - let parts = line.split(separator: " ", maxSplits: 3, omittingEmptySubsequences: true) - guard parts.count == 4, - let pid = Int(parts[0]), - let cpu = Double(parts[2]), - let rssKB = Double(parts[3]) else { + let lineStr = String(line) + let range = NSRange(lineStr.startIndex..., in: lineStr) + guard let match = Self.psStatRegex.firstMatch(in: lineStr, options: [], range: range), + let pidRange = Range(match.range(at: 1), in: lineStr), + let cpuRange = Range(match.range(at: 2), in: lineStr), + let rssRange = Range(match.range(at: 3), in: lineStr), + let pid = Int(lineStr[pidRange]), + let cpu = Double(lineStr[cpuRange]), + let rssKB = Double(lineStr[rssRange]) else { return nil } return RawProcess( pid: pid, - command: String(parts[1]), + command: nil, rawCPUPercent: max(cpu, 0), memoryMB: rssKB / 1024 ) @@ -212,13 +240,14 @@ actor ProcessSnapshotProvider { var aggregates: [String: Aggregate] = [:] for raw in rawProcesses { + let command = raw.command ?? metadataCache[raw.pid]?.metadata.commandKey ?? "" let metadata = metadataCache[raw.pid]?.metadata ?? AppMetadata( - appName: Self.fallbackAppName(for: raw.command), + appName: Self.fallbackAppName(for: command), bundleIdentifier: nil, - commandKey: raw.command + commandKey: command ) let key = aggregateKey(for: metadata) - let child = ProcessChildStat(pid: raw.pid, command: raw.command, cpuPercent: raw.rawCPUPercent, memoryMB: raw.memoryMB) + let child = ProcessChildStat(pid: raw.pid, command: command, cpuPercent: raw.rawCPUPercent, memoryMB: raw.memoryMB) if var existing = aggregates[key] { existing.cpuPercent += raw.rawCPUPercent @@ -230,7 +259,7 @@ actor ProcessSnapshotProvider { } else { aggregates[key] = Aggregate( pid: raw.pid, - command: raw.command, + command: command, appName: metadata.appName, bundleIdentifier: metadata.bundleIdentifier, cpuPercent: raw.rawCPUPercent, @@ -268,6 +297,54 @@ actor ProcessSnapshotProvider { return "command:\(metadata.commandKey)" } + /// Fetches the executable name for a single PID via `ps -p -o comm=`. + /// Uses the basename-safe `comm=` format which avoids spaces in the path issue. + private static func fetchCommand(for pid: Int) -> String? { + let process = Process() + process.executableURL = URL(fileURLWithPath: "/bin/ps") + process.arguments = ["-p", "\(pid)", "-o", "comm="] + + let output = Pipe() + process.standardOutput = output + process.standardError = FileHandle.nullDevice + + do { + try process.run() + } catch { + return nil + } + + process.waitUntilExit() + guard process.terminationStatus == 0 else { return nil } + + let data = output.fileHandleForReading.readDataToEndOfFile() + guard let raw = String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines), + !raw.isEmpty else { + return nil + } + + return raw + } + + /// Fetches commands for a batch of PIDs concurrently. + private func fetchCommands(for pids: [Int]) async -> [Int: String] { + await withTaskGroup(of: (Int, String?).self) { group in + for pid in pids { + group.addTask { + (pid, Self.fetchCommand(for: pid)) + } + } + + var results: [Int: String] = [:] + for await (pid, command) in group { + if let command { + results[pid] = command + } + } + return results + } + } + private static func fallbackAppName(for command: String) -> String { let last = URL(fileURLWithPath: command).lastPathComponent return last.isEmpty ? command : last diff --git a/Tests/ProcessBarMonitorTests/ProcessSnapshotProviderTests.swift b/Tests/ProcessBarMonitorTests/ProcessSnapshotProviderTests.swift new file mode 100644 index 0000000..faff80f --- /dev/null +++ b/Tests/ProcessBarMonitorTests/ProcessSnapshotProviderTests.swift @@ -0,0 +1,115 @@ +import XCTest +@testable import ProcessBarMonitor + +/// Regression tests for ps output parsing (issue #29). +/// The fix uses regex to reliably extract pid/cpu/rss regardless of spaces in the command field. +final class ProcessSnapshotProviderTests: XCTestCase { + + // MARK: - parsePSOutput Regression Tests + + /// Normal ps output: pid, cpu, rss separated by spaces. + func testParsePSOutput_normal() async throws { + let raw = """ + 1 0.2 14288 + 323 0.8 13920 + 325 0.0 4032 + """ + + let provider = ProcessSnapshotProvider.shared + let results = await provider.parsePSOutput(raw) + + XCTAssertEqual(results.count, 3) + + XCTAssertEqual(results[0].pid, 1) + XCTAssertEqual(results[0].rawCPUPercent, 0.2) + XCTAssertEqual(results[0].memoryMB, 14288.0 / 1024, accuracy: 0.001) + + XCTAssertEqual(results[1].pid, 323) + XCTAssertEqual(results[1].rawCPUPercent, 0.8) + XCTAssertEqual(results[1].memoryMB, 13920.0 / 1024, accuracy: 0.001) + + XCTAssertEqual(results[2].pid, 325) + XCTAssertEqual(results[2].rawCPUPercent, 0.0) + XCTAssertEqual(results[2].memoryMB, 4032.0 / 1024, accuracy: 0.001) + } + + /// Extra whitespace between fields must not break parsing. + func testParsePSOutput_extraWhitespace() async throws { + let raw = """ + 1 0.2 14288 + 323 0.8 13920 + """ + + let provider = ProcessSnapshotProvider.shared + let results = await provider.parsePSOutput(raw) + + XCTAssertEqual(results.count, 2) + XCTAssertEqual(results[0].pid, 1) + XCTAssertEqual(results[1].pid, 323) + } + + /// Lines with invalid numeric fields must be skipped and not affect valid lines. + func testParsePSOutput_invalidLinesSkipped() async throws { + let raw = """ + 1 0.2 14288 + not-a-pid 0.5 5000 + 323 0.8 13920 + 456 NaN 9999 + 789 1.0 abc + 999 2.0 12345 + """ + + let provider = ProcessSnapshotProvider.shared + let results = await provider.parsePSOutput(raw) + + // Only the two valid lines should be returned + XCTAssertEqual(results.count, 3) + XCTAssertEqual(results[0].pid, 1) + XCTAssertEqual(results[1].pid, 323) + XCTAssertEqual(results[2].pid, 999) + } + + /// All invalid lines → no crash, empty result. + func testParsePSOutput_allInvalid() async throws { + let raw = """ + invalid line here + another bad line + """ + + let provider = ProcessSnapshotProvider.shared + let results = await provider.parsePSOutput(raw) + + XCTAssertEqual(results.count, 0) + } + + /// Negative CPU values must be normalised to 0. + func testParsePSOutput_negativeCPUNormalised() async throws { + let raw = """ + 1 -0.5 14288 + 2 0.0 5000 + """ + + let provider = ProcessSnapshotProvider.shared + let results = await provider.parsePSOutput(raw) + + XCTAssertEqual(results.count, 2) + XCTAssertEqual(results[0].rawCPUPercent, 0.0) // negative → 0 + XCTAssertEqual(results[1].rawCPUPercent, 0.0) + } + + /// Large whitespace between fields (tab-like spacing from ps output variations). + func testParsePSOutput_variableSpacing() async throws { + let raw = "100\t\t0.1\t\t20000\n200\t\t1.2\t\t40000\n" + + let provider = ProcessSnapshotProvider.shared + let results = await provider.parsePSOutput(raw) + + XCTAssertEqual(results.count, 2) + XCTAssertEqual(results[0].pid, 100) + XCTAssertEqual(results[0].rawCPUPercent, 0.1) + XCTAssertEqual(results[0].memoryMB, 20000.0 / 1024, accuracy: 0.001) + XCTAssertEqual(results[1].pid, 200) + XCTAssertEqual(results[1].rawCPUPercent, 1.2) + XCTAssertEqual(results[1].memoryMB, 40000.0 / 1024, accuracy: 0.001) + } +} From 091f07cbafbbbb7104619b68ae2b2a241ba67504 Mon Sep 17 00:00:00 2001 From: LMZ Date: Tue, 21 Apr 2026 11:59:29 +0800 Subject: [PATCH 2/2] fix: prevent empty-key aggregation collision for processes with unavailable command (PR #38 review fix) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pid-based fallback key ("pid:") is now used when command is nil and no metadata cache entry exists, instead of the previous "" fallback that caused unrelated processes to collapse into one aggregate. Also adds: - aggregateKeyForTest() test helper (static, nonisolated) - 4 new aggregation regression tests: • non-prioritized nil-command processes each get unique key • prioritized PID with failed fetchCommand avoids empty-key collision • resolved commands still group correctly by command string • spaced command paths resolve correctly without pid fallback Fixes review blocking issue on PR #38. --- .../ProcessSnapshotProvider.swift | 23 ++++- .../ProcessSnapshotProviderTests.swift | 90 +++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) diff --git a/Sources/ProcessBarMonitor/ProcessSnapshotProvider.swift b/Sources/ProcessBarMonitor/ProcessSnapshotProvider.swift index b041c8e..58048b7 100644 --- a/Sources/ProcessBarMonitor/ProcessSnapshotProvider.swift +++ b/Sources/ProcessBarMonitor/ProcessSnapshotProvider.swift @@ -240,7 +240,10 @@ actor ProcessSnapshotProvider { var aggregates: [String: Aggregate] = [:] for raw in rawProcesses { - let command = raw.command ?? metadataCache[raw.pid]?.metadata.commandKey ?? "" + // Fallback chain: fetched comm → cached commandKey → pid-based key. + // The pid-based key prevents unrelated processes from collapsing into + // the same empty-string aggregate when neither phase 2 fetch nor cache hit. + let command = raw.command ?? metadataCache[raw.pid]?.metadata.commandKey ?? "pid:\(raw.pid)" let metadata = metadataCache[raw.pid]?.metadata ?? AppMetadata( appName: Self.fallbackAppName(for: command), bundleIdentifier: nil, @@ -297,6 +300,24 @@ actor ProcessSnapshotProvider { return "command:\(metadata.commandKey)" } + // MARK: - Test helpers (nonisolated, safe to call from tests) + + /// Test-only: simulates the command fallback chain without needing actor state. + /// - Parameters mirror what `aggregate()` sees per RawProcess entry. + static func aggregateKeyForTest(pid: Int, command: String?, cachedMetadata: AppMetadata?) -> String { + let resolvedCommand = command ?? cachedMetadata?.commandKey ?? "pid:\(pid)" + let metadata = cachedMetadata ?? AppMetadata( + appName: fallbackAppName(for: resolvedCommand), + bundleIdentifier: nil, + commandKey: resolvedCommand + ) + // Replicate aggregateKey logic + if let bundleIdentifier = metadata.bundleIdentifier { + return "bundle:\(bundleIdentifier)" + } + return "command:\(metadata.commandKey)" + } + /// Fetches the executable name for a single PID via `ps -p -o comm=`. /// Uses the basename-safe `comm=` format which avoids spaces in the path issue. private static func fetchCommand(for pid: Int) -> String? { diff --git a/Tests/ProcessBarMonitorTests/ProcessSnapshotProviderTests.swift b/Tests/ProcessBarMonitorTests/ProcessSnapshotProviderTests.swift index faff80f..2455dac 100644 --- a/Tests/ProcessBarMonitorTests/ProcessSnapshotProviderTests.swift +++ b/Tests/ProcessBarMonitorTests/ProcessSnapshotProviderTests.swift @@ -112,4 +112,94 @@ final class ProcessSnapshotProviderTests: XCTestCase { XCTAssertEqual(results[1].rawCPUPercent, 1.2) XCTAssertEqual(results[1].memoryMB, 40000.0 / 1024, accuracy: 0.001) } + + // MARK: - Aggregation Regression Tests (issue #29) + + /// Multiple non-prioritized processes with nil command and no cached metadata + /// must NOT collapse into the same empty-key aggregate. + /// Each must receive a distinct grouping key via the pid-based fallback. + func testAggregate_noCommandNoCache_oneProcessPerBucket() async throws { + // Simulate three unrelated processes with no command resolved and no cache entry. + // Each must get a unique aggregate key — none should collide. + let p1 = ProcessSnapshotProvider.aggregateKeyForTest(pid: 100, command: nil, cachedMetadata: nil) + let p2 = ProcessSnapshotProvider.aggregateKeyForTest(pid: 200, command: nil, cachedMetadata: nil) + let p3 = ProcessSnapshotProvider.aggregateKeyForTest(pid: 300, command: nil, cachedMetadata: nil) + + XCTAssertNotEqual(p1, p2) + XCTAssertNotEqual(p2, p3) + XCTAssertNotEqual(p1, p3) + + // Each key must contain the pid so it's stable and unique per process + XCTAssertEqual(p1, "command:pid:100") + XCTAssertEqual(p2, "command:pid:200") + XCTAssertEqual(p3, "command:pid:300") + } + + /// A prioritized PID whose fetchCommand fails and has no cache entry + /// must not be grouped with unrelated nil-command processes. + func testAggregate_prioritizedPIDCommandFetchFails_noCollisionWithOtherProcesses() async throws { + // Two different PIDs, both with nil command and nil cache. + // The original bug would give both the same "" key — fixed by pid-based fallback. + let keyPID1 = ProcessSnapshotProvider.aggregateKeyForTest(pid: 1, command: nil, cachedMetadata: nil) + let keyPID2 = ProcessSnapshotProvider.aggregateKeyForTest(pid: 2, command: nil, cachedMetadata: nil) + + XCTAssertNotEqual(keyPID1, keyPID2, + "Two unrelated processes with unavailable command must not share an aggregate key") + + // Ensure the key does NOT look like the old buggy empty-string fallback + XCTAssertFalse(keyPID1 == "command:" || keyPID1.isEmpty, + "Aggregate key must never be the empty string") + XCTAssertFalse(keyPID2 == "command:" || keyPID2.isEmpty, + "Aggregate key must never be the empty string") + } + + /// Processes whose command is successfully resolved must still group correctly + /// by their resolved command key (not pid-based fallback). + func testAggregate_resolvedCommand_oneAggregatePerCommand() async throws { + // Two PIDs with the same resolved command → same aggregate (same app, different threads/processes) + let keyA = ProcessSnapshotProvider.aggregateKeyForTest( + pid: 999, + command: "/usr/libexec/cups/scheduler", + cachedMetadata: nil + ) + let keyB = ProcessSnapshotProvider.aggregateKeyForTest( + pid: 888, + command: "/usr/libexec/cups/scheduler", + cachedMetadata: nil + ) + + XCTAssertEqual(keyA, keyB, "Same resolved command → same aggregate key") + + // And they must differ from a third distinct process + let keyC = ProcessSnapshotProvider.aggregateKeyForTest( + pid: 777, + command: "/usr/sbin/syslogd", + cachedMetadata: nil + ) + XCTAssertNotEqual(keyA, keyC) + } + + /// Parsing with spaces in command paths: the two-phase approach resolves + /// commands via `ps -p -o comm=` which uses comm= (no spaces). + /// This test verifies the command fallback chain produces correct distinct keys + /// when the same "spaced path" process appears multiple times. + func testAggregate_spacedCommandPath_resolvedCorrectly() async throws { + let spacedCommand = "/Application Utilities/Some App.app/Contents/MacOS/Helper Tool" + + let key1 = ProcessSnapshotProvider.aggregateKeyForTest(pid: 1, command: spacedCommand, cachedMetadata: nil) + let key2 = ProcessSnapshotProvider.aggregateKeyForTest(pid: 2, command: spacedCommand, cachedMetadata: nil) + + // Same command string → same aggregate (they are the same app) + XCTAssertEqual(key1, key2) + XCTAssertFalse(key1.hasPrefix("command:pid:"), + "A resolved command should NOT fall back to pid-based key") + + // Distinct command → distinct key + let keyOther = ProcessSnapshotProvider.aggregateKeyForTest( + pid: 3, + command: "/Another App/script with spaces", + cachedMetadata: nil + ) + XCTAssertNotEqual(key1, keyOther) + } }