fix(server): honor static freshness validators - #2715
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 |
ba20d9b to
b66e664
Compare
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the static-freshness / conditional-request work. This is a high-quality change: the RFC 9110 precondition ordering (If-Match → If-Unmodified-Since → If-None-Match → If-Modified-Since), weak-vs-strong comparison rules, whole-second date precision, and byte-range handling all look correct and are backed by thorough unit tests. Dev/prod parity is maintained across the Vite dev middleware, the cached fast path, the filesystem fallback in prod-server.ts, and the Worker asset path. A few small observations below — none blocking.
|
Review posted successfully. SummaryI reviewed PR #2715 ( Verdict: high quality, no blocking issues. What I verified:
Three minor, non-blocking observations posted inline:
|
90ed0e3 to
d70435b
Compare
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the static freshness/conditional-request work end to end. This is a high-quality change: the RFC 9110 §13.2.2 precondition ordering (If-Match → If-Unmodified-Since → If-None-Match → If-Modified-Since), weak-vs-strong entity-tag comparison, whole-second HTTP-date precision, the three HTTP-date wire formats (including the RFC 850 50-year sliding window with weekday/calendar validation), and single byte-range handling all look correct and are backed by thorough unit + integration tests. Dev/prod parity is maintained across the Vite dev middleware, the cached fast path, the filesystem slow path, and the Worker asset path. I ran the three new unit files locally (41 tests) — all green.
No blocking issues. A few small observations below; the earlier bot review already flagged the createStaticAssetRequest signal carry-over, the resolveStaticValidators last-writer-wins, and the no-cache regex, so I won't repeat those.
|
Review posted successfully. SummaryI reviewed PR #2715 ( Verdict: high quality, no blocking issues. What I verified:
Three minor, non-blocking observations posted inline (deliberately avoiding the three already raised by the earlier bot review):
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the static freshness / conditional-request work in full — http-conditional.ts, http-date.ts, http-range.ts, the prod-server.ts fast/slow-path rewrite, static-file-cache.ts, and worker-utils.ts. I re-ran the new unit suites locally (http-range 12, http-date 3, http-conditional 26, serve-static 62 — all green).
Verdict: high quality, no blocking issues. The RFC 9110 §13.2.2 precondition ordering, strong-vs-weak entity-tag comparison, whole-second HTTP-date handling (all three wire formats + RFC 850 sliding window), and single byte-range handling all look correct and are well-covered by tests. Dev/prod parity is maintained across the cached fast path, the filesystem slow path, and the Worker asset path.
Two new observations below (the earlier bot reviews already covered the signal carry-over, resolveStaticValidators last-writer-wins, the no-cache regex, the fast/slow 416 Content-Type divergence, and the redundant * check, so I won't repeat those). Both are behavioral/parity notes, not defects.
|
Review posted successfully. SummaryI reviewed PR #2715 ( Verdict: high quality, no blocking issues. What I verified:
Two new observations posted inline (deliberately avoiding the five already raised by earlier bot reviews):
Both are behavioral/parity notes, not defects. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the static freshness / conditional-request work end to end — http-conditional.ts, http-date.ts, http-range.ts, the prod-server.ts fast/slow-path rewrite, static-file-cache.ts, worker-utils.ts, and the createStaticAssetRequest wiring in app-router-entry.ts.
Verdict: high quality, no blocking issues. RFC 9110 §13.2.2 precondition ordering (If-Match → If-Unmodified-Since → If-None-Match → If-Modified-Since), the 412-vs-304 split for unsafe methods, strong-vs-weak entity-tag comparison, whole-second HTTP-date precision across all three wire formats (including the RFC 850 50-year sliding window with weekday/calendar validation), and single byte-range handling (suffix ranges, safe-integer overflow, 416, inclusive end for both subarray and createReadStream) all look correct and are backed by thorough unit + integration tests. Dev/prod parity is maintained across the cached fast path, the filesystem slow path, and the Worker ASSETS path.
The prior bot reviews already covered the notable observations (signal carry-over, resolveStaticValidators last-writer-wins, the no-cache regex, fast/slow 416 Content-Type divergence, the redundant * check, the always-weak-ETag If-Range trade-off, and the Worker 206 status-override edge). I confirmed all of those and won't repeat them. One new parity note below — not a defect.
|
Review posted successfully. SummaryI reviewed PR #2715 ( Verdict: high quality, no blocking issues. What I verified:
I posted one new parity note (not a defect): the identity/full response advertises |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the static-freshness/conditional-request implementation end to end (http-conditional.ts, http-date.ts, http-range.ts, the prod-server.ts fast/slow rewrite, static-file-cache.ts, worker-utils.ts) and ran the three new unit suites locally (41 tests, all green).
Verdict: high quality, no blocking issues. RFC 9110 §13.2.2 precondition ordering, strong-vs-weak entity-tag comparison, whole-second HTTP-date parsing across all three wire formats (incl. the RFC 850 50-year sliding window with weekday/calendar validation), and single byte-range handling (suffix ranges, safe-integer overflow, 416, inclusive end for both subarray and createReadStream, gated on GET + responseStatus === 200 + strong If-Range) all look correct. Dev/prod parity is maintained across the cached fast path, the filesystem slow path, and the Worker ASSETS path.
I confirmed the observations earlier reviews already covered (signal carry-over, resolveStaticValidators last-writer-wins, the no-cache regex, the always-weak-ETag If-Range trade-off, the Worker 206 status-override edge, and the Accept-Ranges on compressed responses) and won't repeat them. One new, minor parity note below — not a defect.
|
Review posted successfully. SummaryI reviewed PR #2715 ( Verdict: high quality, no blocking issues. What I verified:
One new, non-blocking observation posted inline:
I deliberately confirmed and did not repeat the observations earlier bot reviews already raised (signal carry-over, |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the static-freshness / conditional-request implementation end to end — http-conditional.ts, http-date.ts, http-range.ts, the prod-server.ts fast/slow-path rewrite, static-file-cache.ts, worker-utils.ts, and the createStaticAssetRequest wiring in app-router-entry.ts.
Verdict: high quality, no blocking issues. The RFC 9110 §13.2.2 precondition ordering (If-Match → If-Unmodified-Since → If-None-Match → If-Modified-Since), the 412-vs-304 split for unsafe methods, strong-vs-weak entity-tag comparison, whole-second HTTP-date handling across all three wire formats (including the RFC 850 50-year sliding window with weekday/calendar validation), and single byte-range handling (suffix ranges, safe-integer overflow, 416, inclusive end for both subarray and createReadStream, gated on GET + responseStatus === 200 + strong If-Range) all look correct and are backed by thorough tests. HEAD/Range is correctly ignored via requestRange = req.method === "GET" ? …. Dev/prod parity is maintained across the cached fast path, the filesystem slow path, and the Worker ASSETS path. I confirmed the 412 fast/slow-path Content-Type/Accept-Ranges alignment from 1dee6f8 is in place.
I confirmed the observations earlier reviews already raised (signal carry-over, resolveStaticValidators last-writer-wins, the no-cache regex, the always-weak-ETag If-Range trade-off, the Worker 206 status-override edge, Accept-Ranges on compressed responses, and the 412 alignment) and won't repeat them. One new parity note below — not a defect.
|
Review posted successfully. SummaryI reviewed PR #2715 ( Verdict: high quality, no blocking issues. What I verified:
One new parity note posted inline (not a defect): the slow-path 304 still emits the full I deliberately did not repeat the seven observations earlier bot reviews already raised. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the static-freshness / conditional-request work end to end — http-conditional.ts, http-date.ts, http-range.ts, the prod-server.ts fast/slow-path rewrite, static-file-cache.ts, worker-utils.ts, and the createStaticAssetRequest wiring in app-router-entry.ts. Ran the three new unit suites locally (41 tests, all green).
Verdict: high quality, no blocking issues.
What I verified:
- Precondition ordering (
http-conditional.ts:127) matches RFC 9110 §13.2.2 (If-Match → If-Unmodified-Since → If-None-Match → If-Modified-Since), the 412-vs-304 split for unsafe methods, andno-cacheforcing full revalidation. - Entity-tag comparison — strong for If-Match/If-Range, weak for If-None-Match; correct
*wildcard, comma-in-opaque-tag, backslash-as-literal, and malformed-list rejection. - HTTP-date parsing — all three wire formats with weekday/calendar validation and the RFC 850 50-year sliding window.
- Byte ranges — suffix ranges, safe-integer overflow → unsatisfiable, 416, inclusive
endfor both buffersubarray(start, end+1)andcreateReadStream({start, end}), gated on GET +responseStatus === 200+ strong If-Range. HEAD/Range correctly ignored viarequestRange = req.method === "GET" ? …. - Worker path —
assetResponse.status !== 206guard on the status override is correct, and 416 (a 4xx, so!ok) is preserved rather than masked. - Fast/slow-path header parity — I confirmed the 412 (
baseHeadersvsnotModifiedHeaders+ Content-Type + Accept-Ranges), 416, and 304 (trimmednotModifiedBaseHeaderson the slow path) divergences flagged in earlier review rounds are all now aligned in the follow-up commits (a361cfa,1dee6f8,9734c04).
One new, non-blocking observation posted inline — a theoretical RFC 850 leap-year edge case. Everything else in this PR is solid and thoroughly tested.
|
Review posted successfully. SummaryI reviewed PR #2715 ( Verdict: high quality, no blocking issues. What I verified:
I posted one new non-blocking observation inline: a theoretical RFC 850 leap-year century-boundary edge case in The seven observations from prior bot review rounds (signal carry-over, last-writer-wins validators, the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the static-freshness / conditional-request work end to end and ran the new unit suites locally (http-range 12, http-date 4, http-conditional 26, serve-static 62 — all green).
Verdict: high quality, no blocking issues.
What I verified:
- Precondition ordering (
http-conditional.ts:127) matches RFC 9110 §13.2.2 (If-Match → If-Unmodified-Since → If-None-Match → If-Modified-Since), the 412-vs-304 split for unsafe methods, and requestCache-Control: no-cacheforcing full revalidation (matches thesend/freshstack Next.js uses). - Entity-tag comparison — strong for If-Match/If-Range, weak for If-None-Match; correct
*wildcard, comma-in-opaque-tag, backslash-as-literal, and malformed-list rejection. - HTTP-date parsing — all three wire formats with weekday/calendar validation, the RFC 850 50-year sliding window, and lenient (safe) asctime day parsing.
- Byte ranges — suffix ranges, safe-integer overflow → unsatisfiable, 416, inclusive
endfor both buffersubarray(start, end+1)andcreateReadStream({start, end}), gated on GET +responseStatus === 200+ strong If-Range. HEAD/Range correctly ignored viarequestRange = req.method === "GET" ? …. - Range/omitBody/HEAD interaction — the 206/416 paths never run for no-body statuses (they gate on 200) and never run for HEAD (undefined
requestRange), so no missingomitBodyguard. - Worker path — the
assetResponse.status !== 206override guard is correct; 416 (a 4xx) is preserved via!ok.mergeVaryHeaderreturns a copy (no shared-object mutation). - Fast/slow-path header parity — the 412/416/304 divergences flagged in prior rounds are aligned in the follow-up commits.
The observations from prior review rounds (signal carry-over, resolveStaticValidators last-writer-wins, the no-cache regex, the always-weak-ETag If-Range trade-off, the Worker 206 status-override edge, Accept-Ranges on compressed responses, the header-parity nits, and the RFC 850 leap-day edge) I confirmed and deliberately did not repeat. One small new note below — not a defect.
|
Review posted successfully. SummaryI reviewed PR #2715 ( Verdict: high quality, no blocking issues. What I verified:
I posted one new, non-blocking note: the asctime branch leniently accepts a zero-padded two-digit day (RFC 9110 asctime space-pads), which is safe since it only parses incoming validators and never emits one — flagged purely so it isn't mistaken for strict conformance. I deliberately confirmed and did not repeat the ~eight observations from the prior review rounds. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Reviewed the full implementation end to end (http-conditional.ts, http-date.ts, http-range.ts, the prod-server.ts fast/slow-path rewrite, static-file-cache.ts, worker-utils.ts, and the createStaticAssetRequest wiring in app-router-entry.ts) and re-ran the three new unit suites locally (42 tests, all green).
Verdict: high quality, no blocking issues. The RFC 9110 §13.2.2 precondition ordering, weak/strong entity-tag comparison, three HTTP-date wire formats with the RFC 850 sliding window, and single byte-range handling (suffix, safe-integer overflow, 416, inclusive end, gated on GET/HEAD + status 200 + strong If-Range) all look correct and are thoroughly tested. Fast/slow/Worker parity is maintained.
One clarification for the record on the newest commit (match weak static etags for If-Match): treating W/"asset" as matching "asset" for If-Match diverges from RFC 9110 §13.1.1's strong-comparison requirement, but it correctly matches Next.js's send stack, whose isPreconditionFailure compares match === etag || match === 'W/'+etag || 'W/'+match === etag. So the divergence is deliberate and parity-correct — good call, worth a one-line comment citing send so it isn't "fixed" later.
Three small, non-blocking observations below. The many items raised across the prior review rounds (signal carry-over, last-writer-wins validators, the no-cache regex, always-weak-ETag If-Range, Worker 206 override, Accept-Ranges on compressed responses, the 412/416/304 header-parity nits, and the RFC 850 leap-day edge) I confirmed and won't repeat.
|
Review posted successfully. SummaryI reviewed PR #2715 ( Verdict: high quality, no blocking issues. What I verified:
I posted three small, non-blocking observations that hadn't been raised in prior rounds:
I confirmed and deliberately did not repeat the ~10 observations from the earlier review rounds, which are all either resolved in follow-up commits or non-defects. |
Summary
If-Match,If-Unmodified-Since, weakIf-None-Match, and strict HTTP-date handlingCache-Control: no-cachebytesranges with correct206and416responsesIf-Unmodified-SinceBranch refresh
Rebuilt directly on current
mainatc9a4a843c, which contains the final merged #2710 implementation. The stale #2710 merge ancestry and the discarded dev public-file ETag index are gone. The range behavior previously stacked from #2711 is included in this single clean commit.Next.js parity
Next.js static serving delegates freshness and range evaluation to its send/fresh stack. This change matches the observable validator precedence and freshness behavior while preserving vinext byte-range and multi-runtime behavior.
Validation
vp run vinext#buildpassed