perf(cache): index artifact cache entries by session (Closes #142) - #185
Conversation
|
Warning Review limit reached
Next review available in: 9 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 (1)
Comment |
859f0c5
into
harsharajkumar-273:main
Closes #142
The stated bug can't occur, but the suggested fix is worth making
The issue reports that "if multiple concurrent builds clear or populate cache entries during iteration, map deletion during iteration can cause stale byte count calculations". I don't think that's reachable:
Mapduringfor...ofis explicitly well-defined — the iterator tolerates it and simply skips removed entries.invalidateSessionis synchronous throughout, so no other build can interleave inside that loop. There is no point at which_bytesis observed half-updated.I ran a property test to check rather than assume: 40,000 randomised
set/get/invalidateSessionoperations against the current implementation, asserting after every one that_bytesequals the sum of live entry sizes.No drift, and identical final state before and after this change — so this is a refactor with no behavioural difference, not a correctness fix. I've titled it
perfrather thanfixfor that reason.What the index is actually worth
invalidateSessionscanned the whole map to find one session's entries — up toMAX_ARTIFACT_CACHE_ENTRIES(200) comparisons per call. It's called from four places: session cleanup, the cache-expiry sweep, workspace replacement, and build teardown, and it runs whether or not the session ever cached anything.That last part is the cost. A session that cached nothing still pays a full 200-entry scan:
The second row is the realistic one, and it's ~107× faster. The first row is slower, and I'd rather say so than leave it out — with an empty map the old loop is trivial while the new path still does a
Map.get. That difference is ~25ns per call and inverts as soon as the cache holds anything.Change
A
Map<string, Set<string>>from session to owned keys, maintained in three places rather than one — which is the real risk of this change, so each is deliberate:setunindexes against the entry's previoussessionIdbefore reindexing, since the same key can be rewritten under a different session._evictunindexes the entry it drops. Missing this would leave keys in the index pointing at nothing._unindexdeletes the set once empty, so_bySessiondoesn't accumulate one entry per session the process has ever seen — which would be a slower leak than the one this replaces.getis untouched; LRU reordering doesn't change ownership.The property test above asserts index consistency too — every indexed key resolves to a live entry whose
sessionIdmatches, and the index size equals the map size — across all 40,000 operations, with 0 violations.Note on ordering
This touches
buildExecutor.tsat lines 40-104. My open branches for #104 (exportZip, ~1699) and #107 (cleanup, ~1808) touch the same file well away from here; I'd land this one after those to keep their diffs clean.