fix(update-notifier): cache npm-list install-method check + cheap short-circuits#755
Open
999cleo wants to merge 1 commit into
Open
fix(update-notifier): cache npm-list install-method check + cheap short-circuits#755999cleo wants to merge 1 commit into
999cleo wants to merge 1 commit into
Conversation
…rt-circuits
The `update-notifier` and `block-command-update-npm` init hooks both ran
`npm list -g byterover-cli --depth=0` on every CLI invocation. That
subprocess walks the entire global node_modules tree (200-500ms on a fast
desktop, 3-5s on slower environments like CI cold caches or Termux), and it
ran even when the hook would no-op anyway because:
* stdout is not a TTY (piped output, CI, scripts, daemons all have no
interactive prompt path);
* the command being invoked is not `update` (so
block-command-update-npm has nothing to block);
* the install method has not changed since the last invocation.
This commit:
1. Adds an on-disk cache for the npm-list result at
~/.config/byterover-cli/install-method.json. Cache key is the CLI's
install path, TTL is 7 days, malformed/missing/expired cache files
fall through to the live check transparently. Pure-stdlib (node:fs,
node:os, node:path), no new runtime deps.
2. Short-circuits the update-notifier hook on isTTY=false and on a new
BRV_SKIP_UPDATE_CHECK env var BEFORE doing any subprocess or
filesystem work, so non-interactive callers pay no startup cost.
3. Gates block-command-update-npm on opts.id === 'update' so every
other command path skips the npm check entirely.
The original `isNpmGlobalInstall(execSync)` export is preserved
unchanged for back-compat; the new cached wrapper is exposed as
`isNpmGlobalInstallCached(cliPath, execSync, ...)` and is what the
hooks use.
Measured on Termux/arm64 (worst case for npm-list subprocess cost):
before: brv --version ~11,700ms
after: brv --version ~810ms (14x faster)
Typical Linux desktop with SSD: expect roughly ~1500ms -> ~400ms.
Tests:
* 16 new tests in test/hooks/init/update-notifier.test.ts covering:
cache hit / miss / expired TTL / mismatched cliPath / malformed JSON /
missing file / explicit-false preservation / parent-dir creation /
write-failure safety / shouldRunUpdateCheck short-circuits.
* Existing tests in test/hooks/init/block-command-update-npm.test.ts
pass unchanged.
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.
Cache the
npm list -ginstall-method check so it doesn't run on every CLI invocationWhat this fixes
The
update-notifierandblock-command-update-npminit hooks both callisNpmGlobalInstall(execSync), which shells out tonpm list -g byterover-cli --depth=0on every singlebrvinvocation. That subprocess walks the entire global node_modules tree and is the single largest source of CLI startup latency.Measured on Termux/Android arm64 (the slowest environment, but the same class of overhead exists everywhere):
Tracing the
inithooks with timestamped log lines shows ~11.2s of the 11.7s total goes into the init hook chain, dominated by thenpm list -gsubprocess (~5.2s on its own here, measured independently). On a typical Linux desktop with SSD the same npm call is 200-500ms, so the real-world impact for most users is closer to a 1-2s slowdown per invocation, but it's still wasted work because:block-command-update-npmonly acts whencommandId === 'update', but pays the cost on every other command too.update-notifieralso has no path that can do anything in a non-TTY context (the interactiveconfirmprompt can't run), but it still pays the cost before checkingisTTY.Patch summary
1. On-disk cache for the install-method check
New helpers in
src/oclif/hooks/init/update-notifier.ts:getInstallMethodCachePath()— XDG-style path at~/.config/byterover-cli/install-method.json.readInstallMethodCache(cliPath, ...)— returnsboolean | undefined. Returnsundefined(cache miss) when the file is missing, malformed, expired beyond TTL, or for a different CLI install path.writeInstallMethodCache(cliPath, isNpmGlobal, ...)— best-effort write, silently no-ops on I/O failure so a broken cache never breaks a user's CLI run.isNpmGlobalInstallCached(cliPath, execSync, ...)— read-through cache wrapper around the originalisNpmGlobalInstall.Cache TTL: 7 days. Cache key: install path (so
nvmversion switches, prefix changes, etc. invalidate the cache automatically on the next run).2. Cheap short-circuits first in the
update-notifierhookThe hook now checks
isTTYandBRV_SKIP_UPDATE_CHECKbefore doing any subprocess or filesystem work. Previously the npm check ran first and the TTY check was deep insidehandleUpdateNotification, so non-interactive callers (piped output, CI, scripts, daemons) paid the full cost just to no-op.New env var
BRV_SKIP_UPDATE_CHECKis an explicit escape hatch for CI / scripted invocations that want zero startup overhead.3.
block-command-update-npmonly runs for theupdatecommandThe hook now bails immediately when
opts.id !== 'update'. Previously it calledisNpmGlobalInstallon every invocation just to maybe-block one specific command. This is a pure removal of wasted work.4. Tests
Existing tests pass unchanged. New tests added in
test/hooks/init/update-notifier.test.ts:shouldRunUpdateCheck (new short-circuits)— 4 tests coveringBRV_SKIP_UPDATE_CHECK,isTTY=false,isTTYomitted (back-compat),BRV_ENV=developmentinteracting withisTTY=true.install-method cache— 12 tests covering cache hit, cache miss, expired TTL, mismatched cliPath, malformed JSON, missing file, false-value preservation, parent-dir creation, write failure safety, and the wrapper behaviour skipping live calls on hit.Backward compatibility
isNpmGlobalInstall(uncached) is preserved with identical signature and behaviour. Any external/internal callers that depend on it continue to work.handleBlockCommandUpdateNpmandhandleUpdateNotificationsignatures are unchanged.shouldRunUpdateCheckadds an optionalisTTYarg. Existing callers without it default to the legacy behaviour (no TTY-based short-circuit), so back-compat is preserved.Measured impact
After patching, on the same Termux/arm64 box:
On a Linux desktop (extrapolating from the npm-list timing): expect
brv --versionto drop from ~1.5-2s to ~300-500ms on a warm cache.What this does NOT do
node:fs,node:os,node:path).Manual test plan