fix(cli): cache npm lookups, CLI binaries, and upgrade nudge#13257
fix(cli): cache npm lookups, CLI binaries, and upgrade nudge#13257Swimburger wants to merge 8 commits intomainfrom
Conversation
- Cache getLatestVersionOfCli results with 4-hour TTL in ~/.fern/ - Cache CLI binaries locally instead of re-downloading via npx every time - Cache upgrade nudge message to avoid network I/O on exit - Fall back to npx if local caching fails Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
🌱 Seed Test SelectorSelect languages to run seed tests for:
How to use: Click the ⋯ menu above → "Edit" → check the boxes you want → click "Update comment". Tests will run automatically and snapshots will be committed to this PR. |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Use captured hadCachedMessage flag instead of re-reading cache after writing to determine whether to show fresh message. Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } catch { | ||
| // ignore errors (e.g. permissions) | ||
| } |
There was a problem hiding this comment.
🔴 Empty catch block in ensureCacheDir violates REVIEW.md rule
REVIEW.md mandates: "No empty catch blocks -- at minimum log the error." The ensureCacheDir function at packages/cli/cli/src/cli-context/upgrade-utils/CliCache.ts:18 catches errors but only has a comment — it does not log the error.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 6f34827:
- Added
console.debuglogging in all catch blocks withbiome-ignoresuppression (matches codebase pattern) - Switched to atomic writes (temp file + rename) to prevent partial reads from concurrent processes
- Fixed stale nudge by caching a structured
{ message, latestVersion }object and checkingisVersionAhead(cached.latestVersion, currentVersion)before displaying
There was a problem hiding this comment.
Intentional design choice: console.debug was tried but it writes to stderr, which polluted CLI output and broke the ETE tests (test-ete expects clean output from fern --version). Since this is a best-effort cache utility with no logger access, silent catch blocks are the correct approach here — any cache failure gracefully falls through to the non-cached code path. The comments explain the intent.
| } catch { | ||
| // cache miss or corrupted | ||
| } |
There was a problem hiding this comment.
🔴 Empty catch block in readCache violates REVIEW.md rule
REVIEW.md mandates: "No empty catch blocks -- at minimum log the error." The readCache function at packages/cli/cli/src/cli-context/upgrade-utils/CliCache.ts:35 catches errors but only has a comment — it does not log the error.
Was this helpful? React with 👍 or 👎 to provide feedback.
| } catch { | ||
| // ignore write errors | ||
| } |
There was a problem hiding this comment.
🔴 Empty catch block in writeCache violates REVIEW.md rule
REVIEW.md mandates: "No empty catch blocks -- at minimum log the error." The writeCache function at packages/cli/cli/src/cli-context/upgrade-utils/CliCache.ts:54 catches errors but only has a comment — it does not log the error.
Was this helpful? React with 👍 or 👎 to provide feedback.
…udge fix - Use atomic writes (temp file + rename) to prevent partial reads - Add console.debug logging in catch blocks per REVIEW.md rules - Cache upgrade nudge with target version to prevent stale messages - Check cached version against current CLI version before displaying Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
devin-ai-integration
bot
changed the title
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (fs.existsSync(entryPoint)) { | ||
| cliContext.logger.debug(`Using cached CLI at ${entryPoint}`); | ||
| return entryPoint; | ||
| } | ||
|
|
||
| // Install into cache directory | ||
| const cacheDir = getCachedCliDir(version); | ||
| fs.mkdirSync(cacheDir, { recursive: true }); | ||
|
|
||
| cliContext.logger.debug(`Installing ${packageName}@${version} into cache at ${cacheDir}...`); | ||
|
|
||
| try { | ||
| // Write a minimal package.json so npm install works | ||
| const packageJsonPath = path.join(cacheDir, "package.json"); | ||
| if (!fs.existsSync(packageJsonPath)) { | ||
| fs.writeFileSync(packageJsonPath, JSON.stringify({ private: true }), "utf-8"); | ||
| } | ||
|
|
||
| const { failed } = await loggingExeca( | ||
| cliContext.logger, | ||
| "npm", | ||
| ["install", "--prefer-offline", `${packageName}@${version}`], | ||
| { | ||
| cwd: cacheDir, | ||
| reject: false, | ||
| doNotPipeOutput: true | ||
| } | ||
| ); | ||
|
|
||
| if (failed || !fs.existsSync(entryPoint)) { | ||
| cliContext.logger.debug("Failed to install CLI into cache, falling back to npx"); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Race condition: Multiple concurrent invocations of the same CLI version can corrupt the cache directory. If two processes both check fs.existsSync(entryPoint) at line 44 and both return false, they will both proceed to run npm install in the same cacheDir (lines 62-71), potentially corrupting each other's installations.
The code does fall back to npx on failure, but this creates unnecessary failures and poor user experience. Fix by using a lock file mechanism:
const lockPath = path.join(cacheDir, '.install.lock');
try {
// Try to create lock file exclusively
fs.writeFileSync(lockPath, '', { flag: 'wx' });
// Proceed with installation...
// Delete lock when done
fs.unlinkSync(lockPath);
} catch (err) {
if (err.code === 'EEXIST') {
// Another process is installing, wait and retry or fallback
return undefined;
}
throw err;
}| if (fs.existsSync(entryPoint)) { | |
| cliContext.logger.debug(`Using cached CLI at ${entryPoint}`); | |
| return entryPoint; | |
| } | |
| // Install into cache directory | |
| const cacheDir = getCachedCliDir(version); | |
| fs.mkdirSync(cacheDir, { recursive: true }); | |
| cliContext.logger.debug(`Installing ${packageName}@${version} into cache at ${cacheDir}...`); | |
| try { | |
| // Write a minimal package.json so npm install works | |
| const packageJsonPath = path.join(cacheDir, "package.json"); | |
| if (!fs.existsSync(packageJsonPath)) { | |
| fs.writeFileSync(packageJsonPath, JSON.stringify({ private: true }), "utf-8"); | |
| } | |
| const { failed } = await loggingExeca( | |
| cliContext.logger, | |
| "npm", | |
| ["install", "--prefer-offline", `${packageName}@${version}`], | |
| { | |
| cwd: cacheDir, | |
| reject: false, | |
| doNotPipeOutput: true | |
| } | |
| ); | |
| if (failed || !fs.existsSync(entryPoint)) { | |
| cliContext.logger.debug("Failed to install CLI into cache, falling back to npx"); | |
| return undefined; | |
| } | |
| if (fs.existsSync(entryPoint)) { | |
| cliContext.logger.debug(`Using cached CLI at ${entryPoint}`); | |
| return entryPoint; | |
| } | |
| // Install into cache directory | |
| const cacheDir = getCachedCliDir(version); | |
| fs.mkdirSync(cacheDir, { recursive: true }); | |
| // Use lock file to prevent race conditions during installation | |
| const lockPath = path.join(cacheDir, '.install.lock'); | |
| try { | |
| // Try to create lock file exclusively | |
| fs.writeFileSync(lockPath, '', { flag: 'wx' }); | |
| } catch (err) { | |
| if (err.code === 'EEXIST') { | |
| // Another process is installing, fall back to npx | |
| cliContext.logger.debug("Another process is installing CLI, falling back to npx"); | |
| return undefined; | |
| } | |
| throw err; | |
| } | |
| cliContext.logger.debug(`Installing ${packageName}@${version} into cache at ${cacheDir}...`); | |
| try { | |
| // Write a minimal package.json so npm install works | |
| const packageJsonPath = path.join(cacheDir, "package.json"); | |
| if (!fs.existsSync(packageJsonPath)) { | |
| fs.writeFileSync(packageJsonPath, JSON.stringify({ private: true }), "utf-8"); | |
| } | |
| const { failed } = await loggingExeca( | |
| cliContext.logger, | |
| "npm", | |
| ["install", "--prefer-offline", `${packageName}@${version}`], | |
| { | |
| cwd: cacheDir, | |
| reject: false, | |
| doNotPipeOutput: true | |
| } | |
| ); | |
| if (failed || !fs.existsSync(entryPoint)) { | |
| cliContext.logger.debug("Failed to install CLI into cache, falling back to npx"); | |
| return undefined; | |
| } | |
| } finally { | |
| // Always clean up the lock file | |
| try { | |
| fs.unlinkSync(lockPath); | |
| } catch { | |
| // Ignore errors when cleaning up lock file | |
| } | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
…ollution The console.debug calls were leaking into CLI stderr output, causing test-ete failures where tests capture and compare CLI output. Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ersion - Add useCache param to getLatestVersionOfCli (default false) so fern init and version redirection always get fresh data from npm registry - Only the upgrade nudge path in CliContext passes useCache: true - Store fromVersion in upgrade nudge cache to invalidate when user upgrades to an intermediate version (prevents showing stale 'from' version) Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
| includePreReleases, | ||
| useCache: true |
There was a problem hiding this comment.
🔴 Hardcoded useCache: true in isUpgradeAvailable causes fern upgrade to use stale version data
The isUpgradeAvailable method now always passes useCache: true to getLatestVersionOfCli (CliContext.ts:372). This is correct for the nudge-at-exit path, but the same method is also called by the explicit fern upgrade command at packages/cli/cli/src/commands/upgrade/upgrade.ts:316. With a 4-hour cache TTL (getLatestVersionOfCli.ts:8), running fern upgrade within 4 hours of a previous CLI invocation will read a stale cached version and can incorrectly report "no upgrade available" even when a newer version has been published.
Scenario demonstrating the bug
- User runs
fern generate— the exit nudge callsisUpgradeAvailable(), fetches latest = 1.0.0, writes it to disk cache. - Version 1.1.0 is published 1 hour later.
- User runs
fern upgrade2 hours later —isUpgradeAvailable()callsgetLatestVersionOfCliwithuseCache: true, gets cache hit for 1.0.0 (TTL still valid), and reports no upgrade available.
Prompt for agents
In packages/cli/cli/src/cli-context/CliContext.ts, the `isUpgradeAvailable` method at line 360 should NOT hardcode `useCache: true` when calling `getLatestVersionOfCli`. Instead, add a `useCache` parameter to `isUpgradeAvailable` itself (defaulting to false), so that callers can opt in. Then, in `nudgeUpgradeIfAvailable` at line 168, call `this.isUpgradeAvailable({ useCache: true })` to use the cache for the exit nudge path. The upgrade command in packages/cli/cli/src/commands/upgrade/upgrade.ts:316 would continue calling `isUpgradeAvailable({ includePreReleases })` without `useCache`, ensuring it always fetches fresh data.
Was this helpful? React with 👍 or 👎 to provide feedback.
- CliCache tests: read/write round-trip, TTL expiration, corrupted data, atomic writes, error resilience, various value types - getLatestVersionOfCli tests: useCache param behavior (cache hit/miss), non-prod CLI bypass, prerelease cache keys, always-write semantics - upgradeNudgeCache tests: fromVersion validation prevents stale nudge after intermediate upgrade, cleared cache handling, TTL expiry Also refactored CliCache.ts to compute FERN_CACHE_DIR lazily via getFernCacheDir() function instead of top-level constant, enabling testability with mocked homedir. Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
When a cached CLI binary fails to execute (e.g. corrupted from interrupted npm install), remove the corrupted cache directory and fall through to the npx fallback instead of failing permanently. Also log the error in the ensureCachedCli catch block since cliContext.logger is available. Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
| if (failed || !fs.existsSync(entryPoint)) { | ||
| cliContext.logger.debug("Failed to install CLI into cache, falling back to npx"); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Failed installation leaves corrupted cache directory that isn't cleaned up. If npm install succeeds but the entryPoint doesn't exist (unexpected package structure), the code falls back to npx but leaves the partially installed cache directory.
On the next CLI invocation, the corrupted cache still exists, causing repeated installation attempts and failures.
if (failed || !fs.existsSync(entryPoint)) {
cliContext.logger.debug("Failed to install CLI into cache, falling back to npx");
// Clean up corrupted installation
try {
fs.rmSync(cacheDir, { recursive: true, force: true });
} catch {
// Best-effort cleanup
}
return undefined;
}| if (failed || !fs.existsSync(entryPoint)) { | |
| cliContext.logger.debug("Failed to install CLI into cache, falling back to npx"); | |
| return undefined; | |
| } | |
| if (failed || !fs.existsSync(entryPoint)) { | |
| cliContext.logger.debug("Failed to install CLI into cache, falling back to npx"); | |
| // Clean up corrupted installation | |
| try { | |
| fs.rmSync(cacheDir, { recursive: true, force: true }); | |
| } catch { | |
| // Best-effort cleanup | |
| } | |
| return undefined; | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
| } catch { | ||
| // Best-effort cleanup | ||
| } |
There was a problem hiding this comment.
🔴 Empty catch block in cache cleanup violates REVIEW.md rule
REVIEW.md mandates: "No empty catch blocks -- at minimum log the error." The catch block at packages/cli/cli/src/rerunFernCliAtVersion.ts:143 in the cached-CLI cleanup path catches errors but only has a comment — it does not log the error. Unlike the nearby catch at line 79 which correctly logs, this one silently swallows the error.
| } catch { | |
| // Best-effort cleanup | |
| } | |
| } catch (error) { | |
| cliContext.logger.debug(`Failed to clean up cached CLI directory: ${error}`); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Description
Refs performance investigation from Devin session.
Requested by @Swimburger.
The Fern CLI is slow even for simple commands like
fern --versiondue to repeated network calls (npm registry lookups, upgrade checks) and re-downloading the CLI binary vianpxon every version-redirected invocation. This PR adds file-based caching in~/.fern/to eliminate redundant work across runs while preserving the existing version redirection behavior.Changes Made
CliCache.ts: Generic file-based cache utility using JSON files with TTL support, stored in~/.fern/. Uses atomic writes (temp file + rename) to prevent partial reads from concurrent CLI processes. Cache directory path is computed lazily viagetFernCacheDir()(not a top-level constant) so the module is testable with a mockedhomedir. Catch blocks silently swallow errors (best-effort cache — any failure falls through to the uncached code path).getLatestVersionOfCli.ts): Results are cached with a 4-hour TTL via opt-inuseCacheparameter (defaultfalse). Only the upgrade nudge code path passesuseCache: true; callers incli.ts(version redirection,fern init) always fetch fresh data from the registry.rerunFernCliAtVersion.ts): Instead ofnpx fern-api@<version>(re-downloads every time), installs into~/.fern/versions/<version>/and reuses on subsequent runs. On cached CLI execution failure (e.g. corrupted install), removes the cache directory and falls back tonpxautomatically. Falls back tonpxif initial caching also fails.CliContext.ts): Caches a structured{ message, latestVersion, fromVersion }object. On exit, validates the cache by checking bothisVersionAhead(cached.latestVersion, currentVersion)andcached.fromVersion === currentVersionbefore displaying — so stale messages are suppressed after the user upgrades (including to intermediate versions). Refreshes cache for next run with a 300ms timeout.CliCache.test.ts: read/write round-trips, TTL expiration, corrupted data handling, atomic write verification, error resilience, various value typesgetLatestVersionOfCli.test.ts:useCacheparameter behavior (cache hit/miss/bypass), non-prod CLI short-circuit, prerelease cache key separationupgradeNudgeCache.test.ts:fromVersionvalidation prevents stale nudge after intermediate upgrades, cleared cache handling, TTL expiry, cache round-tripsuseCachescoping: The version lookup cache is only used by the upgrade nudge path. Verify this is correct —fern initand version redirection callers incli.tsdo NOT passuseCache: trueand always hit the npm registry.Hardcoded CLI entry point path (
rerunFernCliAtVersion.ts:25):getCachedCliEntryPointassumes the npm package hascli.cjsat its root insidenode_modules/<packageName>/. If the publishedfern-apipackage structure changes, this path breaks. Graceful fallback to npx exists, but the user would silently lose the caching benefit.Silent catch blocks in
CliCache.ts: Intentional —console.debugwas tried but it writes to stderr, which polluted CLI output and broke ETE tests. Since this is a best-effort utility with no logger access, errors are silently swallowed and execution falls through to the uncached path. This deviates from REVIEW.md's "no empty catch blocks" rule.Corrupted cache recovery (
rerunFernCliAtVersion.ts:137-147): When a cached CLI binary fails to execute, the code removes the entire~/.fern/versions/<version>/directory and falls back to npx. This is a best-effort cleanup — ifrmSyncfails, the corrupted cache persists but the npx fallback still runs.No cache eviction: Old versions accumulate in
~/.fern/versions/indefinitely. May want a cleanup strategy eventually.Upgrade nudge is not truly async: The network call for refreshing the cache is still
awaited withinnudgeUpgradeIfAvailable, so the exit path still blocks for up to 300ms. The improvement is that the displayed message comes from cache instantly.No manual testing yet: Changes have not been verified with a production build. The caching logic is all best-effort with fallbacks, so failures should be invisible to users.
Updates since last revision
ensureCachedCliby adding error parameter to the debug log (addresses review bot comment)Testing
pnpm run check)test-ete)