feat(cli): auto-download buf binary when not on PATH#13682
Conversation
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
🤖 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:
|
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit in Settings → Usage.
Once credits are available, reopen this pull request to trigger a review.
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
| try { | ||
| await mkdir(lockPath, { recursive: false }); | ||
| } catch (err) { | ||
| // Another process grabbed the lock between our rm and mkdir — wait briefly and retry | ||
| logger.debug(`Failed to re-acquire buf lock after break: ${err instanceof Error ? err.message : String(err)}`); | ||
| await new Promise((resolve) => setTimeout(resolve, LOCK_RETRY_INTERVAL_MS)); | ||
| await mkdir(lockPath, { recursive: false }); | ||
| } | ||
| return createLockReleaser(lockPath, logger); |
There was a problem hiding this comment.
Critical race condition in lock retry logic after timeout. If multiple processes hit the timeout simultaneously, they all call rm(lockPath) followed by mkdir(lockPath). The final mkdir at line 139 is not wrapped in error handling, causing an unhandled exception when another process wins the race.
Impact: The function throws unexpectedly instead of either acquiring the lock or gracefully failing, causing resolveBuf() to return undefined.
Fix: The retry logic should loop back to the main acquisition loop rather than attempting a single unguarded retry:
try {
await mkdir(lockPath, { recursive: false });
} catch (err) {
logger.debug(`Failed to re-acquire buf lock after break, retrying acquisition loop`);
// Loop back to normal acquisition with remaining time
const remainingTime = Math.max(0, LOCK_TIMEOUT_MS - (Date.now() - deadline));
if (remainingTime <= 0) {
throw new Error(`Failed to acquire buf lock after timeout and retry`);
}
// Recursive call or loop back to start
}Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Good catch — fixed in 7f76e72. Replaced the unguarded final mkdir with a bounded retry loop in both BufDownloader.ts and ProtocGenOpenAPIDownloader.ts. If another process wins the race after the stale lock is broken, we now loop with LOCK_RETRY_INTERVAL_MS until a retry deadline instead of throwing an unhandled error.
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
| await mkdir(lockPath, { recursive: false }); | ||
| } catch { | ||
| // Another process grabbed the lock between our rm and mkdir — retry with remaining time | ||
| const remaining = Math.max(0, deadline + LOCK_TIMEOUT_MS - Date.now()); |
There was a problem hiding this comment.
Incorrect calculation of remaining timeout. The code adds LOCK_TIMEOUT_MS to deadline (which is already Date.now() + LOCK_TIMEOUT_MS), resulting in double the intended timeout.
What breaks: After breaking a stale lock, the retry will wait for 2 * LOCK_TIMEOUT_MS (240 seconds) instead of the remaining time from the original deadline.
Fix:
const remaining = Math.max(0, deadline - Date.now());This correctly calculates how much time remains from the original 120-second deadline.
| const remaining = Math.max(0, deadline + LOCK_TIMEOUT_MS - Date.now()); | |
| const remaining = Math.max(0, deadline - Date.now()); |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Good catch — fixed in 9b565ef. Changed deadline + LOCK_TIMEOUT_MS - Date.now() to deadline - Date.now() in both BufDownloader.ts and ProtocGenOpenAPIDownloader.ts. The retry now correctly uses the remaining time from the original 120s deadline instead of doubling it.
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-authored-by: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Description
Refs: Follow-up to #13667 (protoc-gen-openapi auto-download)
Auto-downloads the
bufCLI binary (v1.66.1) from GitHub Releases when it is not found on PATH. This eliminates the requirement for users to pre-installbufbefore running proto-based SDK generation.Changes Made
BufDownloader.ts: Downloads buf from GitHub Releases, caches at~/.fern/bin/with versioned filenames, filesystem locking (mkdir-based), and atomic file operations (write-to-temp + rename). Follows the same pattern asProtocGenOpenAPIDownloader.ts.ensureBufCommand()inutils.ts: Extracted the buf resolution logic (check PATH viawhich, fall back to auto-download) into a standalone helper function. Both generators delegate to this shared utility, avoiding duplication.ProtobufIRGenerator.tsandProtobufOpenAPIGenerator.ts: Both generators now callensureBufResolved()at the start ofgenerateLocal(). Each generator'sensureBufResolved()is a thin wrapper that caches the result on the instance and delegates to the sharedensureBufCommand(). The resolved command (either"buf"or the full cached path) is passed through to allbufinvocations.ProtobufOpenAPIGenerator.ts: AddedensureProtocGenOpenAPIResolved()— thewhich protoc-gen-openapicheck now runs once per generation instead of once per proto file. Previously this check ran insidedoGenerateLocal()(called per file), producing 17+ repeated "not found on PATH" log lines.infolevel in bothBufDownloader.tsandProtocGenOpenAPIDownloader.ts— cache hit, download start, download complete, and version update messages are now visible without--log-level debug.utils.ts:detectAirGappedModeForProtobuf()accepts an optionalbufCommandparameter so it uses the resolved buf binary instead of assuming"buf"is on PATH.BufDownloader.tsandProtocGenOpenAPIDownloader.ts: After a stale-lock force-break, the previously unguardedmkdiris now replaced with a bounded retry loop. The retry budget isMath.max(LOCK_RETRY_INTERVAL_MS * 5, deadline - Date.now()), guaranteeing a minimum of ~1s of retries even after the original deadline expires, preventing both unhandled exceptions and dead-code retry loops.BufDownloader.test.ts: 11 unit tests (mocked fetch) + 2 real e2e tests that download the actual buf binary from GitHub Releases.ProtocGenOpenAPIDownloader.test.ts: Test assertions updated to checklogger.infoinstead oflogger.debugfor promoted messages; added missingvi.resetModules().versions.yml: Added 4.37.0 entry.url-form-encoded.json): Regenerated to includeTokenRequest/TokenResponse/get_tokentypes added on main.Supported platforms: linux/x64, linux/arm64, darwin/x64, darwin/arm64, win32/x64, win32/arm64.
Testing
pnpm run check)fern check --api csharp-grpc-proto --log-level debug— verified fresh download, cache reuse, and proto generationresolveBufreturns a full binary path (not a directory likeresolveProtocGenOpenAPI). Verify the integration in both generators correctly passes this as the command torunExecaandcreateLoggingExecutable.whichcheck moved fromdoGenerateLocal()toensureProtocGenOpenAPIResolved()uses aprotocGenOpenAPIResolvedboolean flag. If PATH were to change mid-generation (unlikely), the cached result would be stale.Duplicated→ Addressed: Extracted sharedensureBufResolved()method in both generatorsensureBufCommand()intoutils.ts. Each generator'sensureBufResolved()is now a thin wrapper (caches result + callsfailAndThrowon error).ensureBufCommandusesprocess.cwd()as cwd for thewhichcall — this should be fine sincewhichsearches PATH regardless of cwd, but differs from the previous inline code which usedprotobufGeneratorConfigPath. Worth a quick sanity check.getPlatformInfo): buf usesaarch64for Linux ARM64 butarm64for macOS ARM64. Verify this matches actual release assets.Math.max(LOCK_RETRY_INTERVAL_MS * 5, deadline - Date.now()). This guarantees a minimum ~1s retry window (5 × 200ms) even when the original deadline has already expired. Confirm this is a reasonable minimum — too short risks failing prematurely if the competing process is still downloading; too long delays the error path.whichfallback: Thecatchblock inensureBufCommand()/ensureProtocGenOpenAPIResolved()catches all errors fromwhich(not just "not found"). Same pattern as existing code but worth noting.url-form-encoded.jsonare from merging with main (addsTokenRequest/TokenResponse/get_token), not from this PR's code changes.Link to Devin session: https://app.devin.ai/sessions/9aa3a5abd77d4e069bcfade48423e986
Requested by: @Swimburger