fix(web): Workers appends header values, so no-cache defeated every immutable asset - #334
Merged
Merged
Conversation
…mmutable asset
A regression from my own migration to Workers Static Assets, found by measuring
the live site rather than by any test failing.
web/public/_headers set `Cache-Control: no-cache` on /* and relied on the later,
more specific rules replacing it. Its own comment said so: "a later rule
overrides an earlier one for the same header name, which is why the cache rules
come after the catch-all." That was TRUE FOR PAGES. Workers Static Assets
CONCATENATES, so the live site served every content-hashed asset as
Cache-Control: no-cache, public, max-age=31536000, immutable
and RFC 9111 gives `no-cache` the last word: revalidate before use. Confirmed
against the running site -- /_astro/*.css and /fonts/*.woff2 both carry the
joined value.
Measured cost: 11 of 11 assets returning 304 on a repeat navigation, and the
stylesheet -- the only render-blocking resource -- costing about 588ms in front
of first paint on every internal navigation over Slow 4G. Nothing failed.
Nothing looked wrong. It was merely slower than it had been, in a way only a
measurement would show, which is why the migration passed review.
The security headers stay on /* because they belong on everything.
Cache-Control is now stated once per path class, with the HTML rule scoped to
the HTML instead of to /*.
TestNoTwoHeaderRulesSetTheSameHeader enforces the narrow mechanical rule that
follows: no rule may set a header that /* also sets, because on Workers the
specific rule does not win, it joins. Disjoint paths setting the same header are
fine -- /fonts/* and /shots/* never match one request -- so the guard checks for
a catch-all rather than for duplication. Proven able to fail by restoring
`Cache-Control: no-cache` to the /* block.
The comment that misled was not wrong when it was written. It documented Pages
semantics accurately and I changed the platform underneath it.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a performance regression in the web deploy by restructuring Cloudflare _headers so that Cache-Control is never unintentionally concatenated onto immutable, content-hashed assets under Workers Static Assets (where header values append rather than override).
Changes:
- Updates
web/public/_headersto scopeCache-Control: no-cacheto HTML routes only, while keeping long-lived caching for content-hashed/static assets. - Adds a Go guard test that fails if the catch-all
/*rule sets any header that is also set by a more specific rule (preventing reintroduction of the concatenation bug).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| web/public/_headers | Rescopes cache-control rules to avoid Workers header concatenation defeating immutable caching. |
| internal/testenv/pagesdeploy_test.go | Adds a regression guard to prevent /* from setting headers that are also set elsewhere in _headers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+31
to
+33
| # So no rule may set a header that a later rule also sets. The security headers | ||
| # stay on /* because they belong on everything; Cache-Control is now stated once | ||
| # per path class, and the HTML rule is scoped to the HTML rather than to /*. |
Comment on lines
+555
to
+561
| // The rule this enforces is narrow and mechanical: no two rules may set the same | ||
| // header, because on Workers the second does not win -- it joins. | ||
| // | ||
| // Proven able to fail against the committed tree by adding `Cache-Control: | ||
| // no-cache` back to the `/*` block: this reports /_astro/* and /* both setting | ||
| // Cache-Control. | ||
| func TestNoTwoHeaderRulesSetTheSameHeader(t *testing.T) { |
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.
A regression from my own Workers migration (#332), found by measuring the live site — no test failed.
web/public/_headerssetCache-Control: no-cacheon/*and relied on later rules replacing it. Its comment said so: "a later rule overrides an earlier one for the same header name." That was true for Pages. Workers Static Assets concatenates:RFC 9111 gives
no-cachethe last word. Every immutable asset revalidated on every navigation.Measured cost: 11 of 11 assets returning 304 on repeat navigation; the render-blocking stylesheet costing ~588 ms in front of first paint on each internal navigation over Slow 4G.
Nothing failed. Nothing looked wrong. It was merely slower — which is why the migration passed review.
The guard enforces the mechanical rule that follows: no rule may set a header
/*also sets, because the specific rule doesn't win, it joins. Disjoint paths setting the same header are fine (/fonts/*and/shots/*never match one request), so it checks for a catch-all rather than duplication. Proven able to fail by restoring the old line.The comment that misled wasn't wrong when written. It documented Pages accurately, and I changed the platform underneath it.