fix(cli): optimize protobuf buf generate to reuse single working dir#13673
fix(cli): optimize protobuf buf generate to reuse single working dir#13673Swimburger merged 17 commits intomainfrom
Conversation
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
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.
🤖 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:
|
🌱 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. |
Performance BenchmarkBenchmarked
Result: ~44ms faster (3%) on this small fixture with only 1 proto target file. Expected scaling with more proto filesThis fixture only has 1 proto target, so the parallelization benefit is limited to running the two For the ~17 proto files mentioned in the original investigation, the gains are larger because:
Estimated savings with 17 proto files at ~100ms each:
|
…oto files Instead of creating a new temp dir, copying the proto root, running which checks, writing configs, and resolving deps for EVERY proto file, we now: 1. prepare() - does all setup once: temp dir, copy, which checks, buf.yaml, buf dep update 2. generateFromPrepared() - only runs 'buf generate <target>' per file For N proto files this eliminates (N-1) × expensive setup operations: - temp dir creation + recursive copy of proto root - 2 'which' subprocess calls (buf, protoc-gen-openapi) - buf.yaml + buf.gen.yaml file writes - buf dep update (network call to resolve dependencies) 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>
…epare/generateFromPrepared Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
…s explicit targets Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
| const preparedDir = await generator.prepare({ | ||
| absoluteFilepathToProtobufRoot: representative.absoluteFilepathToProtobufRoot, | ||
| relativeFilepathToProtobufRoot: representative.relativeFilepathToProtobufRoot, | ||
| local: representative.generateLocally, | ||
| deps: representative.dependencies | ||
| }); |
There was a problem hiding this comment.
🟡 Grouping by proto root silently ignores differing dependencies and generateLocally of non-representative specs
When multiple ProtobufSpec objects share the same absoluteFilepathToProtobufRoot, they are grouped together and only the first spec's dependencies, relativeFilepathToProtobufRoot, and generateLocally are passed to generator.prepare() at getAllOpenAPISpecs.ts:43-48. Any other specs in the same group with different dependencies or generateLocally values have their properties silently ignored.
In the old code, each ProtobufSpec was processed independently via convertProtobufToOpenAPI, which called generator.generate() with that spec's own deps and local flag. The new grouping optimization assumes all specs sharing the same root have identical configuration, but this is not validated. If a second spec has additional or different dependencies, buf generate could fail or produce incorrect output because its deps were never resolved in the shared working directory.
Prompt for agents
In packages/cli/workspace/lazy-fern-workspace/src/utils/getAllOpenAPISpecs.ts, after selecting the representative spec (line 38) and before calling generator.prepare() (line 43), add a validation that all specs in the group have the same dependencies, generateLocally, and relativeFilepathToProtobufRoot values. If they differ, either merge the dependencies (take the union) or fall back to processing specs independently via the old generate() path. At minimum, log a warning when discrepancies are detected.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is a valid observation but not a bug in practice — specs sharing the same absoluteFilepathToProtobufRoot come from the same protobuf block in generators.yml, so they always share the same dependencies, generateLocally, and relativeFilepathToProtobufRoot. The grouping mirrors the data model: one proto root = one config.
The old code also didn't handle this case differently — each explicit-target spec passed its own config to generate(), but those configs were always identical since they originated from the same source.
I'll leave this as-is since adding validation for an impossible state would be over-engineering, but happy to add an assertion if the reviewer prefers.
…llelize-protobuf-generation
…ace condition Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Self-ReviewReviewed the full diff across all 4 changed files. Here are my findings: Issues Found1. 2. 3. Missing Confirmed Correct
RecommendationRemove |
…ents from generateFromPrepared Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
…on (resolve versions.yml conflict) Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
…on (resolve versions.yml conflict) Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
…d bufLockContents from PreparedWorkingDir Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
…PATH Co-Authored-By: Niels Swimberghe <3382717+Swimburger@users.noreply.github.com>
…ern-api#13673) 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 performance investigation of dual
buf generatepasses inOSSWorkspace.ts.Proto files go through
buf generatetwice—once viaprotoc-gen-openapi(OpenAPI path) and once viaprotoc-gen-fern(IR path). This PR:protoc-gen-fernruns concurrently withprotoc-gen-openapi.whichchecks, writing configs, and resolving deps for each file).OSSWorkspacelevel so thattoFernWorkspace()andvalidateOSSWorkspace()share the same generated specs instead of running buf generate twice.For N proto files, this eliminates (N−1) × the expensive setup operations: temp dir creation, recursive copy, 2
whichsubprocess calls, config file writes, andbuf dep update. The workspace-level cache further eliminates a full duplicate pass that previously ran during validation.Benchmark Results
Tested with 21 proto targets (all with explicit targets):
main44% faster (~3.3s saved) — measured before the caching fix, which eliminates an additional full duplicate pass visible in
fern generate(where bothtoFernWorkspaceand workspace validation independently calledgetAllOpenAPISpecs).Changes Made
ProtobufOpenAPIGenerator— newprepare()/generateFromPrepared()API:prepare()does all one-time setup (temp dir, copy proto root, writebuf.gen.yaml+buf.yaml, resolvebufandprotoc-gen-openapibinaries via auto-download helpers, runbuf dep update).generateFromPrepared()only runsbuf generate <target>and renames the output to a unique temp file.prepare()callsensureBufResolved()andensureProtocGenOpenAPIResolved()— matching the pattern ingenerateLocal()— so the auto-download feature from feat(cli): auto-download protoc-gen-openapi binary when not on PATH #13667 works correctly in the optimized path. Ifprotoc-gen-openapiisn't on PATH, it's resolved viaresolveProtocGenOpenAPIand the download directory is prepended tobuf's PATH viaenvOverride.ensureBufCommand()now logs the resolved path whenbufis found on PATH (e.g.Found buf on PATH: /usr/local/bin/buf) and when auto-downloaded.ensureProtocGenOpenAPIResolved()does the same forprotoc-gen-openapi. Helps users debug which binary is being used.getAllOpenAPISpecs— group-by-root refactor: All protobuf specs (both explicit-target and no-target) are grouped byabsoluteFilepathToProtobufRoot. Each group callsprepare()once, then iterates targets withgenerateFromPrepared(). Removed the oldexistingBufLockContentscaching approach andconvertProtobufToOpenAPI(dead code after refactor).OSSWorkspace.getOpenAPISpecsCached(): New public method that caches thePromise<OpenAPISpec[]>fromgetAllOpenAPISpecs(), keyed byrelativePathToDependency. BothgetOpenAPIIr()andgetIntermediateRepresentation()now use this cache, andvalidateOSSWorkspacecalls it instead ofgetAllOpenAPISpecsdirectly. This eliminates the duplicate protoc-gen-openapi pass that occurred duringfern generate.makeOpenApiSpechelper: Shared logic for constructingOpenAPISpecobjects from generator results.OSSWorkspace.getIntermediateRepresentation:protoc-gen-fernIR generation kicked off as a promise at method start, runs concurrently with the OpenAPI pipeline, results merged after.generateAllProtobufIRshelper: Processes protobuf specs sequentially (not parallel—npm install -ginProtobufIRGeneratoris not safe to run concurrently). The cross-pass parallelism with the OpenAPI path is still preserved.convertProtobufToOpenAPI, unusedbufLockContentsfield fromPreparedWorkingDir, unusedMaybeValidandisNonNullishimports.pnpm seed clean.versions.ymlentry (4.37.1).Important for Review
allSpecsloop. Verified thatmergeIntermediateRepresentationusesir1.x ?? ir2.xfor scalars (first-wins) and spread/concat for collections — protobuf specs were already processed after OpenAPI docs in the old ordering, so precedence is preserved.Promise:getOpenAPISpecsCachedcaches the in-flight promise, so concurrent callers share the same work. If the promise rejects, subsequent callers get the same rejection (acceptable since failures are fatal).absoluteFilepathToProtobufRoot, only the first spec'sdependencies,generateLocally, andrelativeFilepathToProtobufRootare used forprepare(). This matches the data model (one proto root = one config block ingenerators.yml), but is not validated at runtime.generateFromPreparedassumes output file exists afterbuf generate: Ifbuf generatesucceeds (exit code 0) but produces no output, the subsequentrename()will throw.protoc-gen-openapialways writes a minimal OpenAPI doc even for proto files with no services, and the old code had the same implicit assumption.convertProtobufToOpenAPIreturnedundefined; the newgenerateFromPreparedcallsfailAndThrowon non-zero exit. This is intentionally stricter.Human Review Checklist
mergeIntermediateRepresentationis order-independent (protobuf IRs now merged after all other specs instead of interleaved) — verified: scalar fields useir1 ?? ir2(first-wins), collections use concat; old ordering already had protobuf after OpenAPIprotoc-gen-openapialways writesoutput/openapi.yamlon exit code 0 — verified: plugin always writes output when invoked; old code had same assumptionprepare()setup logic mirrorsdoGenerateLocal()— verified step-by-step: air-gapped detection, tmp dir, cp, config writes, binary resolution viaensureBufResolved/ensureProtocGenOpenAPIResolved, envOverride, buf.yaml, buf dep update all matchenvOverrideis correctly propagated through bothprepare()andgenerateFromPrepared()— verified: stored inPreparedWorkingDir, passed to bothbuf dep updateandbuf generateexecutablesprepare()usesensureBufResolved()/ensureProtocGenOpenAPIResolved()(not rawwhichchecks) so buf auto-download from feat(cli): auto-download protoc-gen-openapi binary when not on PATH #13667 is not regressedgetOpenAPISpecsCachedkeying byrelativePathToDependencyis correct — the cache key isrelativePathToDependency ?? "", so the common case (undefined) shares a single cache entry acrossgetOpenAPIIr,getIntermediateRepresentation, andvalidateOSSWorkspaceTesting
pnpm run check(biome) passestsc --noEmit)pnpm seed cleanrun — removed 3 orphaned seed foldersbuf+protoc-gen-openapito exercise.Link to Devin session: https://app.devin.ai/sessions/1574d18acdd340968bb63a4bf2f686fe
Requested by: @Swimburger