test(macos): subprocess soak + perf baselines, signpost instrumentation, chart identity fix#494
Conversation
…on, chart identity fix Add a SubprocessSoakTests suite that drives the real SystemBurnRunner Process/Pipe/timeout machinery against a fake burn script and asserts open file descriptors and phys_footprint stay flat across 200 spawns, that the timeout path reaps a hung child and reclaims its fds, and that concurrent callers serialize without pile-up. To enable this without spawning the real binary, SystemBurnRunner gains init(explicitBinaryURL:timeout:): an explicit URL is exec'd directly (like the bundled helper) and the capture timeout is injectable (default 30s preserves production behavior). Add PerformanceTests establishing clock+memory baselines for the hot paths: BurndownBuilder.build over 10k samples, BurnLedger.timeseries parsing 5k buckets, and UsageHistoryStore.record's full-rewrite cost against a 50k-sample store. These have no stored baselines so they never fail CI on timing. Add os_signpost instrumentation (subsystem com.agentworkforce.burn): a "refresh" category around the live/usage refresh loops and loadSpend, and a "subprocess" category around each capture with the first CLI arg as the interval message, so slowness can be profiled in Instruments. Fix chart identity churn: LiveBurnSample identified by its bucket date instead of a fresh UUID per refresh, so Swift Charts diffs marks instead of rebuilding the whole chart every ~3s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KJPzcAQdxufCWMnap8vTTL
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSystemBurnRunner is extended to accept an injectable binary URL and capture timeout, with label propagation through subprocess capture and timeout enforcement. A new Signposts enum adds os_signpost instrumentation to refresh and load-spend flows. New PerformanceTests and SubprocessSoakTests validate benchmark timing and subprocess resource stability. ChangesInjectable burn runner, signposts, and test coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces performance profiling and testing enhancements for the macOS Burn application. Key changes include the addition of lightweight os_signpost instrumentation to profile hot paths, stability improvements to the live-burn chart by using stable dates as identifiers instead of dynamic UUIDs, and the introduction of performance and subprocess soak tests. Feedback focuses on avoiding blocking synchronous calls within Swift Concurrency's cooperative thread pool in SystemBurnRunner to prevent thread starvation, as well as updating deprecated Task.sleep(nanoseconds:) calls in tests to the modern Task.sleep(for:) API.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| private func capture(label: String, _ configure: (Process) -> Void) -> String? { | ||
| let signpostID = Signposts.subprocess.makeSignpostID() | ||
| let interval = Signposts.subprocess.beginInterval( | ||
| "capture", id: signpostID, "\(label, privacy: .public)" | ||
| ) | ||
| defer { Signposts.subprocess.endInterval("capture", interval) } |
There was a problem hiding this comment.
The capture method is synchronous and uses group.wait(timeout:) and usleep to block the calling thread until the subprocess exits or times out. Because capture is called from the async actor method run(_:), it executes on a thread from Swift Concurrency's cooperative thread pool.\n\nBlocking a cooperative thread violates the Swift Concurrency runtime contract (which requires that threads always make forward progress) and can lead to thread starvation, UI hangs, or deadlocks.\n\nConsider making capture asynchronous and wrapping the blocking Process execution inside withCheckedContinuation, running the blocking work on a non-cooperative background queue (e.g., DispatchQueue.global(qos: .utility)). To maintain the serialization of subprocesses without blocking the actor's thread, you can use a task-chaining pattern on the actor.
|
|
||
| // The child is SIGTERM'd (then SIGKILL'd); give the background reader a | ||
| // beat to hit EOF and release the pipe fds. | ||
| try await Task.sleep(nanoseconds: 1_000_000_000) |
There was a problem hiding this comment.
Use the modern, non-deprecated Task.sleep(for:) API instead of Task.sleep(nanoseconds:) for better readability and type safety.
| try await Task.sleep(nanoseconds: 1_000_000_000) | |
| try await Task.sleep(for: .seconds(1)) |
| XCTAssertTrue(results.allSatisfy { $0 != nil }, "every concurrent run should complete with output") | ||
|
|
||
| // Let any just-finished captures release their pipes before snapshotting. | ||
| try await Task.sleep(nanoseconds: 300_000_000) |
There was a problem hiding this comment.
Use the modern, non-deprecated Task.sleep(for:) API instead of Task.sleep(nanoseconds:) for better readability and type safety.
| try await Task.sleep(nanoseconds: 300_000_000) | |
| try await Task.sleep(for: .milliseconds(300)) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…r-spawn fd leak Address review findings on the soak/perf PR: - capture() no longer blocks a Swift Concurrency cooperative-pool thread for up to captureTimeout (30s prod). The blocking Process/Pipe body moves into a static blockingCapture(timeout:_:) that runs on a serial utility DispatchQueue; capture() awaits a continuation resumed from that queue. The serial queue preserves the one-subprocess-at-a-time invariant the blocking actor used to provide (guarded by testConcurrentCallersSerializeWithoutPileup) while freeing the pool during the child's run. loginShell/resolveTool become async accordingly; resolveTool's PATH probe is reentrancy-tolerant (two concurrent first calls converge on the same cached value). - Plug the per-spawn fd leak the soak test caught in CI (~2 fds per run, 398 over 200 runs): parent-side Pipe/FileHandle objects are Obj-C autoreleased and GCD pools drain lazily, so descriptors lingered long after each child exited. blockingCapture now explicitly close()s the read handles after each drain (and defensively on the timeout path, after a bounded re-wait) and wraps the whole body in autoreleasepool so remaining objects drain per call. With a spawn every 3s on the live tab this was a real production leak. Soak-test thresholds unchanged (<10 fds, <20 MB). - SubprocessSoakTests: use Task.sleep(for:) Duration API. - PerformanceTests: the history-record benchmark advances a deterministic clock 61s per iteration so no iteration hits the store's <1s duplicate- collapse path; reset moved further out so pruning never kicks in mid-measure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KJPzcAQdxufCWMnap8vTTL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/macos/Tests/BurnTests/SubprocessSoakTests.swift (1)
66-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSleep script relies on undocumented shell exec-optimization; may defeat the timeout/kill test.
process.terminate()/SIGKILLinSystemBurnRunner.capture()only signal the immediate child's PID. This script assumes/bin/shimplicitly execs the trailingsleep 60into the same process (no fork). That optimization exists in bash but is explicitly undocumented behavior, not a portable guarantee across shell implementations/versions.If it doesn't apply in some environment,
shforks a child forsleep; killing the immediateshPID leaves the orphanedsleepprocess holding the pipe's write end open, so the backgroundreadDataToEndOfFile()incapture()won't see EOF until the orphan exits ~60s later — well past the 1s buffer intestTimeoutPathReapsProcessAndFDs(Lines 118-121), risking a flaky/hanging fd measurement there.Use
execto guarantee a single-PID replacement regardless of shell implementation.🔧 Proposed fix
private func makeSleepScript() throws -> URL { try makeScript( """ #!/bin/sh - sleep 60 + exec sleep 60 """ ) }🤖 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 `@apps/macos/Tests/BurnTests/SubprocessSoakTests.swift` around lines 66 - 74, The hang test helper in makeSleepScript relies on shell-specific exec behavior, which can leave an orphaned sleep process and break the timeout/kill path. Update the script generated by makeSleepScript so the shell explicitly replaces itself with the long-running command, ensuring SystemBurnRunner.capture() only has one PID to terminate and testTimeoutPathReapsProcessAndFDs can reliably observe EOF within the expected buffer.
🤖 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.
Nitpick comments:
In `@apps/macos/Tests/BurnTests/SubprocessSoakTests.swift`:
- Around line 66-74: The hang test helper in makeSleepScript relies on
shell-specific exec behavior, which can leave an orphaned sleep process and
break the timeout/kill path. Update the script generated by makeSleepScript so
the shell explicitly replaces itself with the long-running command, ensuring
SystemBurnRunner.capture() only has one PID to terminate and
testTimeoutPathReapsProcessAndFDs can reliably observe EOF within the expected
buffer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 69fc2de6-9a7b-4139-be93-f18fbff73f9a
📒 Files selected for processing (6)
apps/macos/Sources/Burn/BurnLedger.swiftapps/macos/Sources/Burn/LiveBurnViewModel.swiftapps/macos/Sources/Burn/Signposts.swiftapps/macos/Sources/Burn/UsageViewModel.swiftapps/macos/Tests/BurnTests/PerformanceTests.swiftapps/macos/Tests/BurnTests/SubprocessSoakTests.swift
Without exec, some shells fork for the trailing command; SIGTERM/SIGKILL of the immediate child would leave an orphaned sleep holding the pipe write end open past the test's fd-measurement window.
2 participants
Targets the reported BurnOSX slowness + memory leaks with empirical leak/perf tests, profiling hooks, and perf fixes. Originally stacked on #491; retargeted to
mainafter that merged.No Swift toolchain in the authoring environment — Swift here is written conservatively and verified by the
macos-app-testsworkflow (macos-14 runner).The soak test caught a real leak
The first CI run of
testRepeatedRunsDoNotLeakFDsOrMemoryfailed with "open fd count grew by 398 over 200 runs" — ~2 fds leaked per subprocess spawn. Parent-sidePipe/FileHandleobjects are Obj-C autoreleased, and on GCD/cooperative threads the pool drains lazily, so each run's descriptors lingered long after the child exited. With a spawn every ~3s on the live tab, this is a real production leak mechanism, not test noise.Fix (in
blockingCapture): explicitlyclose()the parent-side read handles after each drain (and defensively on the timeout path, after a bounded re-wait so we never close under an in-flight legacy read), and wrap the whole blocking body inautoreleasepoolso remaining autoreleased objects drain per call. The test thresholds were left untouched (<10 fds, <20 MB) — the production code must be leak-free without the caller's help.Cooperative-pool fix (review finding)
captureused to runDispatchGroup.wait+usleepinside the actor method — i.e. on a Swift Concurrency cooperative-pool thread, blocking it for up tocaptureTimeout(30s prod). With the live tab polling every 3s across providers plus spend queries, that can starve the pool (a plausible slowness cause). Now:static blockingCapture(timeout:_:)(no actor state).DispatchQueue("com.agentworkforce.burn.subprocess", qos: .utility);captureisasyncand awaits a checked continuation resumed from that queue. The serial queue preserves the one-subprocess-at-a-time invariant (guarded bytestConcurrentCallersSerializeWithoutPileup) while the cooperative thread is released for the child's whole run.loginShell/resolveToolbecameasync; the actor is reentrant at the await, so two concurrent first calls may both run the PATH probe — benign, both converge on the same cached value (commented in code).What each test guards
SubprocessSoakTests.swift— leaks in the subprocess machineryDrives the real
SystemBurnRunnerProcess/Pipe/timeout code against a temp fake-burnshell script; measures open fds via/dev/fdand memory viaphys_footprint(TASK_VM_INFO).testRepeatedRunsDoNotLeakFDsOrMemory— 10 warmup runs, then 200 measuredrun(["summary","--json"]). Asserts fd delta < 10 and footprint delta < 20 MB. (Already earned its keep — see above.)testTimeoutPathReapsProcessAndFDs— asleep 60script with an injected 1s timeout (production 30s is too slow for CI). Assertsrun()returnsniland fds return to baseline after the SIGTERM→SIGKILL reap.testConcurrentCallersSerializeWithoutPileup— 10 concurrentruncalls via a task group; all must complete with output and fd/footprint deltas stay bounded (guards the serialization invariant that prevents subprocess pile-up).PerformanceTests.swift— hot-path baselinesmeasure(metrics: [XCTClockMetric(), XCTMemoryMetric()])over:BurndownBuilder.buildwith a 10,000-sample series.BurnLedger.timeseriesparsing a 5,000-bucket payload via aFakeRunner.UsageHistoryStore.recordagainst a store pre-seeded with 50 series × 1,000 samples (the known full-cache-rewrite-per-record cost). Each iteration advances a deterministic clock by 61s so no iteration hits the store's <1s duplicate-collapse path — every measured call does the same append + full-rewrite work.No stored baselines, so these never fail CI on timing — they establish measurements in the test report and catch crashes/regressions.
Signposts — profiling slowness in Instruments
Sources/Burn/Signposts.swiftexposes oneOSSignposterper category under subsystemcom.agentworkforce.burn:refresh— intervals aroundLiveBurnViewModel.refresh(),UsageViewModel.refresh(force:), andloadSpend.subprocess— an interval around every capture (queue wait + child run), with the first CLI arg (e.g.summary) as the interval message.To profile:
then filter the os_signpost lane by subsystem
com.agentworkforce.burn(categoriesrefresh,subprocess).Chart identity fix
LiveBurnSamplewasIdentifiablevialet id = UUID(), so every ~3s refresh minted brand-new identities for all samples, forcing Swift Charts to rebuild the entire chart instead of diffing. Changed tovar id: Date { date }. Series are stored per-provider and each chartForEachiterates a single provider's array (verified inLiveBurnView.swift), where bucket dates are unique, so per-series date identity suffices.Production changes
SystemBurnRunner.init(explicitBinaryURL: URL? = nil, timeout: TimeInterval = 30)— explicit URL short-circuits resolution to direct exec (like the bundled case, no login shell); injectable timeout. Defaults reproduce prior behavior.captureasync on a serial exec queue + staticblockingCapture(above); explicit handle closes + per-callautoreleasepool(the fd-leak fix).loadSpend, andcapture.LiveBurnSampleidentity change.🤖 Generated with Claude Code
https://claude.ai/code/session_01KJPzcAQdxufCWMnap8vTTL