fix(macos): bound usage history growth + debounced persistence#492
Conversation
…tests
UsageHistoryStore held every usage sample in an in-memory dict mirrored
to history.json, with three unbounded-growth / hot-path problems:
- Nil-reset series (keyed "...|none") were never pruned: pruneStaleWindows
only understood numeric reset timestamps, so Double("none") failed and the
series accumulated forever.
- Within-window series were unbounded: a weekly window sampled every ~60s is
~10k samples with nothing to thin it.
- Every record() re-encoded the entire cache to JSON and wrote it to disk
synchronously inside queue.sync, on the main-actor refresh path — a
per-minute hitch that grows with history size.
Fixes:
- Prune nil-reset series once their newest sample is >24h old; keep the
existing reset->1h rule for timestamped keys.
- Cap each series at maxSamplesPerSeries (2000): on overflow, keep the first
sample and the newest half at full resolution, decimating the older half.
- Debounce persistence: cache mutation stays synchronous, but the disk write
is a coalesced DispatchWorkItem ~2s out, written atomically. flushForTesting()
performs a pending write synchronously for deterministic tests.
Adds UsageHistoryStoreTests covering nil-reset pruning, timestamped-window
pruning, cap/decimation, duplicate collapse, debounced persistence + roundtrip,
and on-disk dict load.
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: 59 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)
✨ 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 memory-growth limits and debounced disk persistence to UsageHistoryStore, along with a comprehensive test suite. It also simplifies test assertions in BurnLedgerParsingTests and removes the watchDesired reentrancy guard from BurnLedger. The review feedback highlights two critical issues: first, removing the watchDesired flag in BurnLedger introduces actor reentrancy bugs that can leak background processes or ignore stop requests; second, performing synchronous disk writes on the shared queue can block the main thread during synchronous record() calls. The reviewer suggests restoring the reentrancy guards and offloading disk writes to a dedicated background queue.
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 var watchProcess: Process? | ||
| /// True between start and stop requests. Actor methods are reentrant across | ||
| /// awaits, so a stop can interleave while start is suspended resolving the | ||
| /// binary; start rechecks this before spawning so the stop wins. | ||
| private var watchDesired = false | ||
|
|
||
| /// Starts a background `burn ingest --watch` if one isn't already running. | ||
| /// Only runs with the bundled native helper (a login-shell child can't be | ||
| /// cleanly managed); the live chart still polls either way. | ||
| func startIngestWatch() async { | ||
| guard watchProcess == nil else { return } | ||
| watchDesired = true | ||
| guard let url = await runner.bundledBinaryURL() else { return } | ||
| guard watchDesired, watchProcess == nil else { return } | ||
| let process = Process() | ||
| process.executableURL = url | ||
| process.arguments = ["ingest", "--watch", "--quiet"] |
There was a problem hiding this comment.
Actor Reentrancy Bug
Removing the watchDesired flag and the post-await guard checks introduces a serious actor reentrancy bug.
Because Swift actor methods are reentrant, the actor can suspend at await runner.bundledBinaryURL(). During this suspension, other tasks can execute on the actor. This leads to two major issues:
- Duplicate Processes / Leaks: If
startIngestWatch()is called multiple times concurrently, they will all pass the initialwatchProcess == nilcheck, await the URL resolution, and then spawn multiple duplicate background processes. Only the last one will be stored inwatchProcess, leaking the others to run indefinitely. - Ignored Stop Requests: If
stopIngestWatch()is called whilestartIngestWatch()is suspended, the stop request will be ignored becausewatchProcessis stillnilat that moment. Once the suspension resumes, the process will be spawned anyway and left running.
We should restore the watchDesired state variable and the post-await guard checks to ensure safety.
private var watchProcess: Process?
/// True between start and stop requests. Actor methods are reentrant across
/// awaits, so a stop can interleave while start is suspended resolving the
/// binary; start rechecks this before spawning so the stop wins.
private var watchDesired = false
/// Starts a background `burn ingest --watch` if one isn't already running.
/// Only runs with the bundled native helper (a login-shell child can't be
/// cleanly managed); the live chart still polls either way.
func startIngestWatch() async {
guard watchProcess == nil else { return }
watchDesired = true
guard let url = await runner.bundledBinaryURL() else { return }
guard watchDesired, watchProcess == nil else { return }
let process = Process()
process.executableURL = url
process.arguments = ["ingest", "--watch", "--quiet"]| /// Terminates the background watch process, if running. | ||
| func stopIngestWatch() { | ||
| watchDesired = false | ||
| watchProcess?.terminate() | ||
| watchProcess = nil | ||
| } |
There was a problem hiding this comment.
Actor Reentrancy Bug (Stop Ingest)
To fully restore the reentrancy guard, we must also set watchDesired = false in stopIngestWatch() so that any pending start request is cancelled.
/// Terminates the background watch process, if running, and cancels any
/// start that is still resolving the binary.
func stopIngestWatch() {
watchDesired = false
watchProcess?.terminate()
watchProcess = nil
}| private func persist() { | ||
| guard let data = try? JSONEncoder().encode(cache) else { return } | ||
| try? data.write(to: fileURL) | ||
| try? data.write(to: fileURL, options: .atomic) | ||
| } |
There was a problem hiding this comment.
Main-Thread Blocking during Disk I/O
Although cache mutations are debounced and scheduled asynchronously on queue, the actual disk write (data.write(to:options:)) is a synchronous I/O operation performed on the same serial queue.
Because record() is called synchronously from the main actor using queue.sync, any call to record() while persist() is writing to disk will block the main thread until the disk write completes. This defeats the purpose of moving persistence off the hot path and can still cause UI hitches.
To completely eliminate main-thread blocking, we should perform the synchronous disk write on a dedicated background serial queue. We can also add a synchronously parameter to persist() so that flushForTesting() remains deterministic and synchronous.
private static let writeQueue = DispatchQueue(label: "com.agentworkforce.burn.history.write", qos: .background)
private func persist(synchronously: Bool = false) {
guard let data = try? JSONEncoder().encode(cache) else { return }
let url = fileURL
let writeBlock = {
try? data.write(to: url, options: .atomic)
}
if synchronously {
writeBlock()
} else {
Self.writeQueue.async(execute: writeBlock)
}
}| func flushForTesting() { | ||
| queue.sync { | ||
| pendingWrite?.cancel() | ||
| pendingWrite = nil | ||
| persist() | ||
| } | ||
| } |
There was a problem hiding this comment.
Synchronous Flush for Testing
Update flushForTesting() to call persist(synchronously: true) so that tests remain deterministic and wait for the disk write to complete.
| func flushForTesting() { | |
| queue.sync { | |
| pendingWrite?.cancel() | |
| pendingWrite = nil | |
| persist() | |
| } | |
| } | |
| func flushForTesting() { | |
| queue.sync { | |
| pendingWrite?.cancel() | |
| pendingWrite = nil | |
| persist(synchronously: true) | |
| } | |
| } |
| private func schedulePersist() { | ||
| pendingWrite?.cancel() | ||
| let item = DispatchWorkItem { [weak self] in | ||
| guard let self else { return } | ||
| self.pendingWrite = nil | ||
| self.persist() | ||
| } | ||
| pendingWrite = item | ||
| queue.asyncAfter(deadline: .now() + persistDebounce, execute: item) | ||
| } |
There was a problem hiding this comment.
Asynchronous Persist Scheduling
Update schedulePersist() to call persist(synchronously: false) to offload the disk write to the background queue.
| private func schedulePersist() { | |
| pendingWrite?.cancel() | |
| let item = DispatchWorkItem { [weak self] in | |
| guard let self else { return } | |
| self.pendingWrite = nil | |
| self.persist() | |
| } | |
| pendingWrite = item | |
| queue.asyncAfter(deadline: .now() + persistDebounce, execute: item) | |
| } | |
| private func schedulePersist() { | |
| pendingWrite?.cancel() | |
| let item = DispatchWorkItem { [weak self] in | |
| guard let self else { return } | |
| self.pendingWrite = nil | |
| self.persist(synchronously: false) | |
| } | |
| pendingWrite = item | |
| queue.asyncAfter(deadline: .now() + persistDebounce, execute: item) | |
| } |
![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: 743f3d2027
ℹ️ 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".
| watchDesired = true | ||
| guard let url = await runner.bundledBinaryURL() else { return } | ||
| guard watchDesired, watchProcess == nil else { return } | ||
| let process = Process() |
There was a problem hiding this comment.
When the live view is opened and then closed while runner.bundledBinaryURL() is still suspended, stopIngestWatch() can run with watchProcess == nil and return; this code then resumes and spawns burn ingest --watch anyway. That leaves the watcher running after the UI has stopped until some later stop call happens. Keep a desired flag or recheck after the await before creating the process.
Useful? React with 👍 / 👎.
|
|
||
| // Keep each series bounded so long-lived windows can't grow forever. | ||
| if samples.count > Self.maxSamplesPerSeries { | ||
| samples = decimated(samples) |
There was a problem hiding this comment.
For users who already have a bloated history.json, a single call to decimated only reduces the series to about 75% of its previous size because it keeps the whole newer half. A 10,080-sample weekly series becomes roughly 7,561 samples and is written back/returned still far above maxSamplesPerSeries, so the memory and full-file persistence cost remain high for several refreshes. Loop or sample directly to the target cap before assigning back to the cache.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Move the actual disk write off the store's serial queue onto a dedicated utility-QoS writeQueue: record() enters the store queue synchronously from the main actor, so an in-progress debounced write there could block the next refresh. Encoding stays on the store queue for thread safety; persist(synchronously:) lets flushForTesting() guarantee the file is on disk before returning (the serial writeQueue makes the sync flush wait for and supersede any in-flight async write). - Enforce maxSamplesPerSeries with a loop: one decimation pass only trims to ~75%, so an oversized legacy history (e.g. 10k samples) stayed over the cap. Add a test seeding 10,000 samples on disk and asserting one record() bounds the series while keeping the first sample and the newest data. - Nil-reset pruning now uses the series' max date instead of .last, so it stays correct if a series is ever not strictly chronological. - Align BurnLedgerParsingTests with main: hoist awaits out of XCTUnwrap autoclosures (Swift rejects async calls there) and use the withLock helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KJPzcAQdxufCWMnap8vTTL
2 participants
Stacked on #491.
UsageHistoryStore(apps/macos/Sources/Burn/UsageHistory.swift) keeps every usage sample in an in-memory dict mirrored tohistory.json, recording one sample per metric roughly every 60s. Three unbounded-growth / hot-path problems contributed to the reported BurnOSX memory growth and per-minute slowness.Bugs
1. Nil-reset series were never pruned.
key(provider:metric:)uses"none"as the trailing segment whenmetric.resetsAt == nil.pruneStaleWindowsonly handled numeric reset timestamps (Double(last)), soDouble("none")failed the guard and those series accumulated samples forever.2. Within-window series were unbounded. A weekly window sampled every ~60s is roughly
7 * 24 * 60 ≈ 10,080samples with nothing thinning them — one series per window, kept for the whole window.3. Persistence ran on the hot path. Every
record()re-encoded the entire cache to JSON and wrote it to disk synchronously insidequeue.sync, andrecordis called from the main actor during refresh. A full-file rewrite per minute whose cost grows with total history size — a main-thread hitch that gets worse over time.Fixes
1. Prune nil-reset series by staleness. In
pruneStaleWindows,…|nonekeys are dropped once their newest sample is older than 24h relative toreference. Timestamped keys keep the existing "reset > 1h ago" rule.2. Cap samples per series. New internal constant
maxSamplesPerSeries = 2_000. On append past the cap the series is decimated: the first sample (window origin) is always kept, the newest half is kept at full resolution, and the older half is thinned by dropping every second sample. Deterministic, and structured so repeated appends stay bounded without discarding recent, high-value data.3. Move persistence off the hot path. Cache mutation stays synchronous (the
queue.syncinrecordstill returns the updated series), but the disk write is now a debounced, coalescedDispatchWorkItemscheduled ~2s out on the private queue — a burst of records produces one write. Writes are now atomic (.atomic) so a crash mid-write can't corrupthistory.json. New internalflushForTesting()performs a pending write synchronously for deterministic tests. (Silent decode-failure fallback on init is unchanged — acceptable behavior.)Burndown consumption (
Burndown.swift) is unaffected: it reads samples bydate/percentage, both preserved.Tests
New
apps/macos/Tests/BurnTests/UsageHistoryStoreTests.swift, each usinginit(fileURL:)on a unique temp file with injected dates (no sleeps):flushForTesting(), then decode matches in-memory series, and a new store on the same file loads the roundtripped samplesNo Swift toolchain in the authoring environment; the
macos-app-testsworkflow verifies on macos-14.🤖 Generated with Claude Code
https://claude.ai/code/session_01KJPzcAQdxufCWMnap8vTTL
Generated by Claude Code