fix(cli): skip npm install in protobuf IR generation when packages already installed#12326
Closed
thesandlord wants to merge 4 commits intomainfrom
Closed
fix(cli): skip npm install in protobuf IR generation when packages already installed#12326thesandlord wants to merge 4 commits intomainfrom
thesandlord wants to merge 4 commits intomainfrom
Conversation
…ready installed Co-Authored-By: Sandeep Dinesh <sandeep@buildwithfern.com>
Contributor
🤖 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:
|
Contributor
🌱 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. |
…n air-gapped mode Co-Authored-By: Sandeep Dinesh <sandeep@buildwithfern.com>
Co-Authored-By: Sandeep Dinesh <sandeep@buildwithfern.com>
Co-Authored-By: Sandeep Dinesh <sandeep@buildwithfern.com>
Contributor
|
This PR is stale because it has been open 25 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Contributor
|
This PR was closed because it has been inactive for 5 days after being marked stale. |
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.
Description
Refs: Customer report of gRPC generation hanging in air-gapped self-hosted environments.
In
ProtobufIRGenerator.setupRemainingProtobufConfig(), twonpm installcalls (npm installfor@bufbuild/protobuf+@bufbuild/protoplugin, andnpm install -g fern-api) ran unconditionally. In air-gapped environments where npm cannot reach the registry, these calls hang indefinitely — the root cause of gRPC generation freezing in self-hosted deployments.This PR uses the existing
this.isAirGappedflag as the primary guard to skip all npm installs in air-gapped mode (where the self-hosted Docker image already hasfern-apiglobally and@bufbuildpackages at/opt/buf-modules). For non-air-gapped environments, package availability is checked before installing as a secondary optimization.Additionally, the air-gapped detection timeout (
detectAirGappedModeForProtobuf) is reduced from 30s to 10s to minimize startup delay in air-gapped environments.Requested by: @thesandlord
Link to Devin run: https://app.devin.ai/sessions/a5880378995e43e5bc32210f77dcac83
Changes Made
this.isAirGappedistrue, all npm installs are skipped entirely with a debug log messageisModuleResolvable()— checks if a package exists in the working directory'snode_modulesviafs.stat(used in non-air-gapped path)isPackageInstalledGlobally()— checks if a package is globally installed vianpm list -g(used in non-air-gapped path)setupRemainingProtobufConfigskipsnpm install/npm install -g fern-apiif the packages are already presentdetectAirGappedModeForProtobuf()(utils.ts)versions.ymlwith v3.76.1 changelog entryisAirGappedis only set whendeps.length > 0: IngenerateLocal(), air-gap detection viadetectAirGappedModeForProtobufonly runs when there are buf dependencies. If a protobuf spec has no deps but the environment is air-gapped,this.isAirGappedremainsundefinedand npm install will still be attempted. In practice, gRPC specs should always have deps — but reviewers should confirm this assumption holds.isModuleResolvableon temp directory: In the non-air-gapped path, the check runs against a freshly created temp directory that won't havenode_modules, so it will almost always fall through to runningnpm install. This is acceptable behavior for non-air-gapped environments but means the check is effectively a no-op optimization. The air-gapped guard is the real fix.Docker npm wrapper interaction: The self-hosted Docker image already has an npm wrapper that intercepts
npm installcalls for@bufbuildpackages and copies from/opt/buf-modules/node_modules. If the wrapper were working correctly, the hang shouldn't occur — suggesting the wrapper may be bypassed in some scenarios (PATH ordering, permissions, etc.). This code-level fix provides defense-in-depth.10s detection timeout: The reduced timeout should be sufficient for a network connectivity check, but very slow or high-latency networks could false-positive as air-gapped. Previously this was 30s.
Testing
pnpm run check)