fix(build-executor): release build log buffers on the cache-hit cleanup path (Closes #107) - #167
Conversation
|
Warning Review limit reached
Next review available in: 2 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)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Closes #107
Why the stated diagnosis no longer holds
Traced against
main@2f06e6e:cleanup()callsthis.sessions.delete(sessionId)on both exits — line 1811 (the cached-directory early return) and line 1825 (the normal path).this.sessions.set(...)sites (1023, 1064, 1095, 1132, 1178) are each followed by ascheduleCleanup(sessionId)call (1035, 1075, 1106, 1145, 1192), so every session created gets a cleanup timer.crypto.randomBytes(8).toString('hex'), which always satisfies the/^[0-9a-f]{16}$/guard at line 1785, so that early return never strands an entry either.What does leak
this.buildLogs. It is populated in_initLog(line 431) and deleted in exactly one place — line 1826, which sits after theisDirCachedearly return at 1808-1813. That branch deletessessionsand returns, sobuildLogsandbuildPromisesare never cleared on it.That branch is the cache-hit path, which is the one a long-running server takes most often when rebuilding repos it has seen before — precisely the scenario the issue describes. Each stranded entry holds up to 2000 log-line objects (bounded per entry by the
shift()at line 442, but unbounded in the number of entries) plus asubscribersSet.So the issue's diagnosis is stale, but its thesis is right: there is unbounded map growth in
BuildExecutorcleanup that grows memory on long-running instances. It'sbuildLogs, notsessions.Change
Releases both maps on the early-return branch, matching what the normal path already does:
artifactCache.invalidateSession()deliberately stays on the normal path only — invalidating it here would defeat the cache this branch exists to preserve. The on-disk directory is likewise untouched.Verification
npx tsc --noEmitinbackend/clean.buildLogs,buildPromisesandpdfBuildsaudited:pdfBuildsalready self-cleans viapromise.finally()at line 1778 andbuildPromisespartially self-cleans at 1496/1508, sobuildLogsis the only map with no other release path.Note:
npm testis red on cleanmainindependently of this branch.