fix(build): skip unhelpful precompressed variants - #2712
Conversation
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the precompression changes. The core logic is sound: variants are only emitted when they actually reduce wire bytes (accounting for negotiation-header overhead), stale variants are cleaned up on re-runs and on threshold/incompressibility transitions, and the atomic temp-then-rename write prevents partial variants from being exposed. The shared isPrecompressedVariantBeneficial helper keeps the build-time emit decision and the serve-time defense-in-depth check consistent, which is good.
A few things worth considering below. The only one I'd call more than nit-level is the orphaned .tmp file behavior on a mid-build crash.
| return false; | ||
| } | ||
|
|
||
| const temporary = `${destination}.${process.pid}.${randomUUID()}.tmp`; |
There was a problem hiding this comment.
The temp-then-rename approach is correct for atomicity, but if the process crashes between writeFile and the catch/rename (e.g. SIGKILL, ENOSPC handled elsewhere, OOM), an orphaned foo.js.<pid>.<uuid>.tmp is left in _next/static/.
That matters because the serving side (static-file-cache.ts) registers every walked file that isn't a .br/.gz/.zst/.vite entry, so a stray .tmp becomes a servable static entry at its own pathname (with application/octet-stream content-type). The name is unguessable and the dir is immutable-cached, so severity is low, but it's cruft that can accumulate across failed builds.
Two mitigations to consider: (1) skip .tmp (or the whole *.<pid>.*.tmp shape) in walkFilesWithStats/StaticFileCache, and/or (2) sweep stale *.tmp files in the target dir at the start of precompressAssets. Option 1 is the cheaper safety net.
There was a problem hiding this comment.
Fixed in 3a6168a. The startup cache now excludes only vinext’s exact .br|.gz|.zst.<pid>.<randomUUID>.tmp shape, with a regression test proving an interrupted-write artifact is not registered as a servable URL.
| if (outcomes.some(Boolean)) { | ||
| result.filesCompressed++; | ||
| result.totalOriginalBytes += content.length; | ||
| result.totalBrotliBytes += outcomes[0] ? brContent.length : content.length; |
There was a problem hiding this comment.
Minor semantics change worth confirming is intentional: filesCompressed now increments when any variant is beneficial, and totalBrotliBytes falls back to content.length when brotli specifically isn't beneficial. In the edge case where brotli is never beneficial for any file but gzip/zstd are, totalBrotliBytes === totalOriginalBytes, so the summary line in index.ts:6546 prints 0.0% smaller with brotli while still reporting a nonzero filesCompressed. Not a bug, but the label may read oddly. Consider reporting the best-variant ratio, or renaming the metric, if you want the summary to stay meaningful in that case.
| originalSize: number, | ||
| encoding: PrecompressedEncoding, | ||
| ): boolean { | ||
| return originalSize - compressedSize > RESPONSE_HEADER_OVERHEAD[encoding]; |
There was a problem hiding this comment.
The overhead model counts both Content-Encoding and a full Vary: Accept-Encoding header as pure additional cost of the variant. In practice the original response also gains Vary: Accept-Encoding whenever any variant exists (see static-file-cache.ts:184-187), so Vary isn't strictly incremental to choosing the encoded representation. Since the comment already calls this "conservative," this is fine as a heuristic — just flagging that the threshold is slightly stricter than the true wire-byte delta, which means a small band of marginally-beneficial variants gets dropped. If that's the intended bias, no change needed.
|
|
||
| it("does not emit a marginally smaller representation with larger wire overhead", async () => { | ||
| const content = deterministicHighEntropyBytes(4096); | ||
| content.copy(content, content.length - 48, 0, 48); |
There was a problem hiding this comment.
This test depends on brotli q5 output for this specific input landing in the narrow "smaller than original but within header overhead" window. It's guarded by the explicit assertions on lines 120-121, so it will fail loudly (not silently pass) if a zlib/brotli version bump shifts the output size out of that window — which is the right call. Just noting it may need a nudge to the input/mutation if brotli's behavior changes across Node versions in CI.
|
Review posted. Summary of what I found in PR #2712: Overall assessmentThe change is sound. Variants are emitted only when they reduce wire bytes (net of negotiation-header overhead), stale variants get cleaned up on re-runs and on threshold/incompressibility transitions, writes are atomic via temp-then-rename, and the shared Feedback posted (4 inline comments)
No blocking issues. |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-reviewed after the tmp-file fix landed (3a6168a). The core design is sound and the earlier orphaned-.tmp concern is now addressed: variants are emitted only when they beat the negotiation-header overhead, stale variants are swept on re-runs / below-threshold / incompressible transitions, writes are atomic via temp-then-rename, and the shared isPrecompressedVariantBeneficial helper keeps build-time emit and serve-time defense-in-depth in lockstep. Tests are thorough and fail loudly rather than silently.
A few residual observations below — none blocking.
| // A hard process exit can strand the temporary side of an atomic | ||
| // precompression write. Never register that internal artifact as a URL, | ||
| // but preserve matching user files outside the precompression target. | ||
| if (isHashed && PRECOMPRESSION_TEMP_FILE_RE.test(relativePath)) continue; |
There was a problem hiding this comment.
The tmp-artifact guard is gated on isHashed. Precompression only ever writes tmp files under the hashed assets dir, so this is correct for the normal case. But the guard is doing double duty as a security/hygiene filter, and it silently does nothing for any tmp-shaped file outside the recognized hashed layout (e.g. an unusual assetPrefix layout that isHashed doesn't classify, or a user file under public/ that happens to match the shape). Since the whole point is "never register an internal artifact as a URL," consider testing the regex unconditionally and only using isHashed to decide cache-control, not whether to register at all. As written, the coupling of the safety filter to the hashed-path heuristic is a little surprising and worth a comment clarifying the intended scope.
| if (outcomes.some(Boolean)) { | ||
| result.filesCompressed++; | ||
| result.totalOriginalBytes += content.length; | ||
| result.totalBrotliBytes += outcomes[0] ? brContent.length : content.length; |
There was a problem hiding this comment.
totalBrotliBytes now falls back to content.length when brotli specifically isn't beneficial, while filesCompressed increments if any variant is beneficial. In the edge case where brotli is never beneficial but gzip/zstd are, totalBrotliBytes === totalOriginalBytes, so the summary at index.ts:6546 reports a nonzero file count with 0.0% smaller with brotli. Not a correctness bug, but the label reads oddly. Since the metric is now "the representation Brotli negotiation would serve" rather than raw brotli bytes, consider either renaming it (e.g. totalNegotiatedBytes) or reporting the best-variant ratio so the summary stays meaningful.
| originalSize: number, | ||
| encoding: PrecompressedEncoding, | ||
| ): boolean { | ||
| return originalSize - compressedSize > RESPONSE_HEADER_OVERHEAD[encoding]; |
There was a problem hiding this comment.
The overhead model charges the variant for a full Vary: Accept-Encoding line, but the original response also gains Vary whenever any variant exists (static-file-cache.ts:194-197), so Vary isn't strictly incremental to choosing the encoded representation — only Content-Encoding is. That makes the threshold slightly stricter than the true wire-byte delta and drops a thin band of marginally-beneficial variants. The doc comment already calls this "conservative," so this is fine as an intentional bias — just flagging that the Vary term is arguably double-counted against the variant.
| const temporary = `${destination}.${process.pid}.${randomUUID()}.tmp`; | ||
| try { | ||
| await fsp.writeFile(temporary, compressed); | ||
| await fsp.rename(temporary, destination); |
There was a problem hiding this comment.
fsp.rename over an existing destination is atomic on POSIX and overwrites on Windows (libuv passes MOVEFILE_REPLACE_EXISTING), so replacing an existing beneficial variant is safe cross-platform. One thing not covered: two concurrent precompressAssets runs against the same output dir (e.g. overlapping builds) each write a unique <pid>.<uuid>.tmp, so temps won't collide, but the final rename to the shared .br/.gz/.zst target still races last-writer-wins. That's almost certainly acceptable for a build step, but if concurrent builds into one dir is ever a supported scenario it's worth a note.
|
Review posted successfully. Here's a summary of my re-review of PR #2712. Overall assessmentThe change is sound and the prior review's main concern (orphaned Feedback posted (4 inline comments, none blocking)
No blocking issues. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Independent re-review (/bigbonk). I read the full diff, ran vp test run tests/precompress.test.ts tests/static-file-cache.test.ts (52 passing), and verified the temp-file regex against real randomUUID()-based output. No blocking issues.
The design is sound:
- Variants are emitted only when savings exceed
Content-Encoding+Varyoverhead (isPrecompressedVariantBeneficial), and the same helper gates serve-time selection, so build-time emit and serve-time defense-in-depth stay in lockstep. - Atomic temp-then-rename with per-write
<pid>.<uuidv4>.tmpnames,catchcleans up the temp on failure. - Stale variants are swept on re-runs and on below-threshold / incompressible transitions (
removeCompressedVariants,removeFileIfPresent). - The interrupted-write artifact guard in
static-file-cache.tsprevents a stranded.tmpfrom becoming a servable URL, and the test confirms a matchingpublic/file is still served.
The prior review rounds already covered the substantive points (.tmp guard scope, totalBrotliBytes label, Vary double-counting, concurrent builds). I agree with all of them and won't re-post duplicates. One residual observation below that I think is the most actionable of the non-blocking items.
| return false; | ||
| } | ||
|
|
||
| const temporary = `${destination}.${process.pid}.${randomUUID()}.tmp`; |
There was a problem hiding this comment.
The serve-time guard in static-file-cache.ts now prevents a stranded .tmp from being served, which closes the security angle. But the artifacts still accumulate on disk across crashed builds: walkFiles only collects COMPRESSIBLE_EXTENSIONS, so .tmp files are never cleaned up by a subsequent precompressAssets run — only the serve-time filter hides them.
Consider a cheap sweep at the start of precompressAssets (or folding a .tmp match into the walk) so a failed build doesn't leave permanent cruft in _next/static/. Not blocking — the names are unguessable and the dir is immutable-cached — just noting the on-disk accumulation isn't self-healing.
|
Review posted successfully. Here's my summary of PR #2712. Overall assessmentThe change is sound and well-tested. I independently verified the diff, ran the two targeted test files (52 tests passing), and confirmed the temp-file regex matches actual The design holds up:
Feedback postedOne inline comment (non-blocking): the serve-time guard closes the security angle, but orphaned The prior review rounds already covered the other substantive points (guard scope, |
# Conflicts: # packages/vinext/src/server/static-file-cache.ts
|
/bigbonk review for issues |
There was a problem hiding this comment.
Independent /bigbonk re-review. I read the full diff, ran vp test run tests/precompress.test.ts tests/static-file-cache.test.ts (54 passing), and verified the temp-file regex against real randomUUID() output. No blocking issues — the change is correct and well-tested.
The design holds up:
isPrecompressedVariantBeneficialgates both build-time emit and serve-time selection, so the two decisions stay in lockstep and can't drift.- Atomic temp-then-rename with per-write
<pid>.<uuidv4>.tmpnames; thecatchcleans up the temp on failure and rethrows. - Stale variants are swept on re-runs and on below-threshold / incompressible transitions (
removeCompressedVariants,removeFileIfPresent). - The serve-time guard in
static-file-cache.tsprevents a stranded.tmpfrom becoming a servable URL while preserving a matchingpublic/file (confirmed by the new test).
The prior review rounds already covered the substantive non-blocking items (.tmp on-disk accumulation, guard scope coupled to isHashed, totalBrotliBytes label, Vary double-counting, concurrent-build races). I agree with all of them. Below I re-surface only the single item I consider most actionable and still unaddressed in code — the others remain fine as documented/intentional.
| return true; | ||
| } | ||
|
|
||
| async function removeCompressedVariants(fullPath: string): Promise<void> { |
There was a problem hiding this comment.
On-disk .tmp accumulation is not self-healing (non-blocking).
The serve-time guard in static-file-cache.ts correctly hides a stranded temp file, so the security/correctness angle is closed. But nothing ever deletes an orphaned foo.js.br.<pid>.<uuid>.tmp from disk:
walkFiles(line 121) only collectsCOMPRESSIBLE_EXTENSIONS, so.tmpfiles are skipped by the build walk entirely.removeCompressedVariantsonly removes.br/.gz/.zst(lines 211-213), never.tmp.
So after a crash between writeFile and rename, the temp lingers permanently across every subsequent build. Severity is low (unguessable name, immutable-cached dir, hidden at serve time), but the cleanup isn't self-healing. A cheap sweep at the start of precompressAssets — or folding the .tmp shape into removeCompressedVariants — would make failed builds leave no permanent cruft in _next/static/.
|
Review posted. Summary of my independent AssessmentNo blocking issues. The change is correct and well-tested. I read the full diff, ran the two targeted test files (54 passing), and verified the temp-file regex against real The design is sound:
Feedback postedOne inline comment (non-blocking): the on-disk The prior review rounds already covered the other residual items (guard scope coupled to |
2 participants
What changed
Content-EncodingandVaryresponse-field overheadWhy
Precompression previously wrote every encoding result regardless of size. High-entropy assets could therefore produce representations larger than the original, which the production server would still prefer during content negotiation. A merely smaller payload could also cost more on the wire once its required response fields were included. Reruns could retain variants that were no longer useful.
Impact
Precompressed representations reduce conservative HTTP/1 wire bytes, stale variants are cleaned up, partially written variants are not exposed to the production server, and legitimate public files remain unaffected.
Validation
vp test run tests/precompress.test.ts tests/static-file-cache.test.ts tests/serve-static.test.ts(99 tests)vp check packages/vinext/src/build/precompress.ts packages/vinext/src/server/static-file-cache.ts packages/vinext/src/utils/precompressed-variant.ts tests/precompress.test.ts tests/static-file-cache.test.ts tests/serve-static.test.tsvp run vinext#buildVINEXT_PRECOMPRESS=1 pnpm exec vinext buildintests/fixtures/pages-basic(15 assets, 45 encoded variants, 71.3% Brotli reduction)