fix: bound debounce() recursion in autoupdate flow#1337
Open
BadassBison wants to merge 1 commit into
Open
Conversation
The previous debounce() in update.ts was a recursive sleep loop:
const debounce = async (cacheDir) => {
const m = await mtime(lastrunfile)
m.setHours(m.getHours() + 1)
if (m > new Date()) {
await wait(60 * 1000)
return debounce(cacheDir) // recurse
}
ux.stdout('time to update')
}
The recursion exits only when "lastrun" mtime is more than 1 hour
stale. But every CLI invocation touches "lastrun" before debounce()
checks it (hooks/init.ts:62), so while the user is actively running
CLI commands, "lastrun + 1hr" keeps shifting forward and the
recursion never terminates. Each spawned autoupdate child stays
pinned as a full node process for the entire active session, writing
"waiting until ..." to autoupdate.log every minute.
This is the root cause of oclif#1277 (the autoupdate.log growth symptom —
reported as 11 GB on another user's Mac). On my own machine the same
log accumulated 7.1 MB before the pinned children were SIGTERMed
externally.
Resolution: convert the recursion to a bounded iterative loop capped
at 6 hours of wall-clock wait. If the user is still active after
that, abandon this autoupdate run; the next CLI invocation will fire
a fresh one when it is again needed. Also dedupes the "waiting
until ..." stdout to a single line per child (the rest go to debug
output), which slows log growth further even within a single run.
All 9 existing tests continue to pass.
|
Thanks for the contribution! Before we can merge this, we need @BadassBison to sign the Salesforce Inc. Contributor License Agreement. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1277
The
debounce()function insrc/update.ts:353-372was a recursive sleep loop with no termination bound. While the CLI user is active, the recursion never exits — each spawned autoupdate child stays pinned as a full node process and writes one log line per minute, forever. This is the root cause of the unboundedautoupdate.loggrowth in #1277.Bug
Before:
The recursion exits only when
lastrunmtime is more than 1 hour stale. But every CLI invocation toucheslastrunbeforedebouncechecks it (src/hooks/init.ts:62), so while the user is active,lastrun + 1hrkeeps shifting forward and the recursion never terminates.(Note: the
let output = falsedeclaration on the first line is actually unused for cross-iteration deduplication — it's local to each recursive call, so it resets tofalseon every recursion. That's a separate small bug; the new implementation actually achieves the deduplication it was trying to.)After:
Three changes from the original:
MAX_DEBOUNCE_WAIT_MS(6 hours), the run is abandoned. The next CLI invocation will fire a fresh autoupdate when it's again needed.announcedis moved to an outer scope so theif (announced) debug(msg) else { ux.stdout(msg); announced = true }dedupe actually fires — previously each recursive call had its ownoutput = false, so every iteration printed to stdout.Direct evidence the mechanism fires in the wild
autoupdate.loggrows indefinitely #1277) —autoupdate.loggrew to 11 GB on @martin-helmich's machine.~/Library/Caches/<cli>/autoupdate.logis 7.1 MB of repeatedwaiting until 2026-05-26T00:32:00.650Z to updatelines, ending in SIGTERM messages where pinned children were eventually reaped externally.How to verify
Hard to demonstrate quickly (the leak accumulates over weeks of normal use), but on any long-lived CLI install:
You should see at least one pinned child process. With this PR, after at most 6 hours, that child exits on its own and the next CLI invocation can spawn a fresh autoupdate without competing with a stuck predecessor.
Test plan
yarn test).yarn lintclean.Open questions for reviewers
MAX_DEBOUNCE_WAIT_MS? Long enough to wait through typical work sessions; short enough that a stuck child doesn't pin memory for days. Tunable.autoupdate.logfiles? The boundeddebounce()alone vastly slows the growth (at most ~360 lines per autoupdate run instead of unbounded), but doesn't address existing multi-GB files. I'd be inclined to leave log rotation as a separate follow-up — happy to take that on too if desired.Companion bug
A related-but-independent bug in
src/hooks/init.ts(race window betweenautoupdateNeeded()andwriteFile) can spawn multiple pinned children per debounce-window event, amplifying this leak. Filed and fixed separately as #1335 (issue) and #1336 (PR).