Skip to content

Commit deec8d4

Browse files
authored
perf(seo): send ETags on edge cached pages and answer 304 (#636)
Edge cached HTML carried no validator, so every recrawl downloaded every page again. The Nuxt SEO crawler saw no unchanged page in seven days, and Workers Cache replaced each stale page with a full render. A page under an edgeCache rule now gets a weak ETag from the build id and a hash of its HTML. Sentry's per-render trace meta tags are left out of the hash. A request that sends the ETag back gets an empty 304 that keeps the route's cache headers. Requests with credentials and pages the edge may not store get no ETag. No page sends Last-Modified. A Skill page shows stars, ages and related Skills, so no one date bounds it, and a cache would answer If-Modified-Since with a wrong 304. The post-deploy edge cache check now proves the 304, and proves that a conditional request on a cold entry never leaves a 304 for the next visitor.
1 parent 6583a6a commit deec8d4

7 files changed

Lines changed: 420 additions & 7 deletions

File tree

‎.github/workflows/deploy-cloudflare.yml‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -160,9 +160,10 @@ jobs:
160160
# Workers Cache stores `/skills/trending` (see routeRules in
161161
# nuxt.config.ts). This proves a repeat visit is a hit, and that no stored
162162
# response sets a cookie, hides Markdown from an agent, or carries a
163-
# session. It fails the job without a rollback: the rollback smoke
164-
# re-checks the previous version, and a version from before the rule
165-
# could never pass these checks.
163+
# session. It also proves a stored page answers its ETag with a 304, and
164+
# that a 304 never replaces the stored page. It fails the job without a
165+
# rollback: the rollback smoke re-checks the previous version, and a
166+
# version from before the rule could never pass these checks.
166167
- name: Check the trending edge cache
167168
run: pnpm production:edge-cache
168169

‎docs/arch/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ See [ADR-0007](../adr/0007-contract-in-the-cli-repository.md).
7373
- **Public pages render signed out**: the server never reads the session for a public page. The server render must not branch on User-Agent either, because `Vary` does not name it. Only then can one stored copy serve every visitor. A page that calls `useBotDetection()` on the server needs that call removed before it gets an edge cache rule. The browser loads the session after hydration. `useAuth().state` is `pending`, `anonymous`, or `signed-in`, and an auth-dependent control renders a same-size placeholder while it is `pending`. A page that needs the session on the server uses the `session` or `auth` route middleware, which also marks the response `private, no-store`.
7474
- **Session reads**: server code reads the session with `readUserSession()`. It opens a session only when the request carries one, because h3 sets a new session cookie on every other read. ESLint bans a direct `getUserSession()`.
7575
- **Markdown negotiation**: `shared/content-negotiation.ts` decides HTML or a 307 to the `.md` URL from `Accept` and `Sec-Fetch-Dest` only, and every page response names both in `Vary`.
76+
- **HTML validators**: `shared/html-validators.ts` gives a page under an `edgeCache` rule a weak ETag: the build id plus a hash of the HTML. A request that sends it back gets an empty 304. A request with a cookie or an `Authorization` header gets no ETag. No page sends `Last-Modified`, because no one date bounds what a page renders.
7677

7778
## URL canonicals
7879

‎scripts/lib/edge-cache-check.ts‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
* 6. Every board is stored under its own key: boards that differ only by query
1717
* string render a different title or canonical URL. The bare path and the
1818
* default range are one board, so they may share both.
19+
* 7. A stored page carries an ETag, and a request that sends it back gets a
20+
* 304. A 304 must never be stored as the page: after a conditional request
21+
* on a cold entry, the next plain request still gets the whole page.
1922
*/
2023

2124
import { varyKeysEdgeCache } from '../../shared/content-negotiation'
@@ -46,7 +49,7 @@ const SERVED_FROM_CACHE = new Set(['HIT', 'STALE', 'UPDATING'])
4649

4750
export type EdgeCacheFetch = (input: string, init: RequestInit) => Promise<Response>
4851

49-
export type EdgeCacheRequest = 'anonymous' | 'markdown' | 'session-cookie'
52+
export type EdgeCacheRequest = 'anonymous' | 'markdown' | 'session-cookie' | 'revalidation'
5053

5154
export type EdgeCacheFailure
5255
= | { _tag: 'network-error', path: string, request: EdgeCacheRequest, message: string }
@@ -58,6 +61,9 @@ export type EdgeCacheFailure
5861
| { _tag: 'rendered-for-session', path: string, reason: string }
5962
| { _tag: 'www-redirect', path: string, status: number | null, location: string | null, message?: string }
6063
| { _tag: 'shared-cache-entry', paths: string[], identity: string | null }
64+
| { _tag: 'no-etag', path: string }
65+
| { _tag: 'not-revalidated', path: string, status: number }
66+
| { _tag: 'stored-not-modified', path: string, status: number }
6167

6268
export type EdgeCacheCheckResult
6369
= | { _tag: 'passed', checks: Array<{ path: string, cacheStatuses: string[] }> }
@@ -77,6 +83,8 @@ export interface EdgeCacheCheckDependencies {
7783
*/
7884
hitAttempts?: number
7985
retryDelayMs?: number
86+
/** Makes the cold cache entry for the revalidation check. Defaults to a random UUID. */
87+
nonce?: string
8088
}
8189

8290
type Fetched
@@ -227,6 +235,42 @@ async function checkSessionCookie(path: string, url: string, fetch: EdgeCacheFet
227235
return null
228236
}
229237

238+
/**
239+
* Runs after the anonymous check, so the entry is warm. Workers Cache stores
240+
* one variant per `Accept` value, so an `Accept` that names the nonce reaches
241+
* a cold entry that still renders HTML.
242+
*/
243+
async function checkRevalidation(path: string, url: string, fetch: EdgeCacheFetch, nonce: string): Promise<EdgeCacheFailure | null> {
244+
const networkError = (message: string): EdgeCacheFailure => ({ _tag: 'network-error', path, request: 'revalidation', message })
245+
246+
const stored = await request(fetch, url, BROWSER_HEADERS)
247+
if (stored._tag === 'error')
248+
return networkError(stored.message)
249+
if (stored.response.status !== 200)
250+
return { _tag: 'status', path, request: 'revalidation', expected: 200, actual: stored.response.status }
251+
const etag = stored.response.headers.get('etag')
252+
if (!etag)
253+
return { _tag: 'no-etag', path }
254+
255+
const conditional = await request(fetch, url, { ...BROWSER_HEADERS, 'if-none-match': etag })
256+
if (conditional._tag === 'error')
257+
return networkError(conditional.message)
258+
if (conditional.response.status !== 304)
259+
return { _tag: 'not-revalidated', path, status: conditional.response.status }
260+
261+
// Either a 200 or a 304 is right here. The plain request after it is the check.
262+
const cold = { ...BROWSER_HEADERS, accept: `${BROWSER_HEADERS.accept},application/x-edge-check;q=0.1;n=${nonce}` }
263+
const coldConditional = await request(fetch, url, { ...cold, 'if-none-match': etag })
264+
if (coldConditional._tag === 'error')
265+
return networkError(coldConditional.message)
266+
const next = await request(fetch, url, cold)
267+
if (next._tag === 'error')
268+
return networkError(next.message)
269+
if (next.response.status !== 200 || !next.body.includes('</html>'))
270+
return { _tag: 'stored-not-modified', path, status: next.response.status }
271+
return null
272+
}
273+
230274
function defaultWwwBaseUrl(baseUrl: string): string | null {
231275
const { protocol, hostname } = new URL(baseUrl)
232276
return hostname === 'skilld.dev' ? `${protocol}//www.${hostname}` : null
@@ -251,6 +295,7 @@ export async function checkEdgeCache(dependencies: EdgeCacheCheckDependencies):
251295
const retryDelayMs = Math.max(0, Math.floor(dependencies.retryDelayMs ?? 5_000))
252296
const paths = dependencies.paths ?? EDGE_CACHED_PATHS
253297
const wwwBaseUrl = dependencies.wwwBaseUrl === undefined ? defaultWwwBaseUrl(dependencies.baseUrl) : dependencies.wwwBaseUrl
298+
const nonce = dependencies.nonce ?? crypto.randomUUID()
254299

255300
const failures: EdgeCacheFailure[] = []
256301
const checks: Array<{ path: string, cacheStatuses: string[] }> = []
@@ -264,6 +309,9 @@ export async function checkEdgeCache(dependencies: EdgeCacheCheckDependencies):
264309
else {
265310
checks.push({ path, cacheStatuses: anonymous.cacheStatuses })
266311
identities.set(anonymous.identity, [...identities.get(anonymous.identity) ?? [], path])
312+
const revalidation = await checkRevalidation(path, url, fetch, nonce)
313+
if (revalidation)
314+
failures.push(revalidation)
267315
}
268316

269317
// After the anonymous loop, so the apex entry is warm.

‎server/plugins/html-validators.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
import { getRequestHeader, getResponseHeader, setResponseHeader } from 'h3'
2+
import { useRuntimeConfig } from 'nitropack/runtime'
3+
import { decideHtmlValidators } from '#shared/html-validators'
4+
5+
/**
6+
* Give an edge cached page an ETag, and answer a request that already holds
7+
* the page with an empty 304. `shared/html-validators.ts` decides both.
8+
*
9+
* The 304 keeps the route's cache headers, so a revalidating Workers Cache
10+
* entry stays fresh for the route's lifetime again.
11+
*/
12+
export default defineNitroPlugin((nitroApp) => {
13+
nitroApp.hooks.hook('render:response', async (response, { event }) => {
14+
const header = (value: string | number | string[] | undefined): string | null =>
15+
value === undefined ? null : Array.isArray(value) ? value.join(', ') : String(value)
16+
const decision = await decideHtmlValidators({
17+
buildId: useRuntimeConfig().app.buildId,
18+
method: event.method,
19+
status: response.statusCode ?? 200,
20+
body: response.body,
21+
request: { get: name => header(getRequestHeader(event, name)) },
22+
response: { get: name => header(getResponseHeader(event, name)) },
23+
})
24+
if (decision._tag === 'skip')
25+
return
26+
27+
setResponseHeader(event, 'etag', decision.etag)
28+
if (decision._tag === 'not-modified') {
29+
response.statusCode = 304
30+
response.statusMessage = 'Not Modified'
31+
response.body = ''
32+
}
33+
})
34+
})

‎shared/html-validators.ts‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
/**
2+
* ETags for the HTML pages Workers Cache stores, and the 304 that answers one.
3+
*
4+
* Without a validator, every recrawl downloads every page again, and Workers
5+
* Cache replaces a stale page with a full render. With one, a crawler that
6+
* sends `If-None-Match` gets a 304, and a revalidating cache keeps its body.
7+
*
8+
* The ETag is weak: a weak ETag promises the same page, not the same bytes.
9+
* Cloudflare compresses responses and may rewrite a strong ETag. Each render
10+
* also carries a fresh Sentry trace id, which is not page content.
11+
*
12+
* There is no `Last-Modified`. A Skill page shows stars, ages and related
13+
* Skills, so no single date bounds what it renders. A date that misses a
14+
* change makes a cache answer `If-Modified-Since` with a wrong 304. A request
15+
* that sends only `If-Modified-Since` gets the full page.
16+
*/
17+
18+
export interface HeaderReader {
19+
get: (name: string) => string | null
20+
}
21+
22+
export interface RenderedPage {
23+
/** The Nuxt build id. A deploy changes it, so no ETag survives a deploy. */
24+
buildId: string
25+
method: string
26+
status: number
27+
/** A string for a whole rendered document. */
28+
body: unknown
29+
request: HeaderReader
30+
response: HeaderReader
31+
}
32+
33+
export type HtmlValidatorSkipReason = 'method' | 'status' | 'not-html' | 'credentials' | 'sets-cookie' | 'not-shared'
34+
35+
export type HtmlValidatorDecision
36+
= | { _tag: 'skip', reason: HtmlValidatorSkipReason }
37+
| { _tag: 'send', etag: string }
38+
| { _tag: 'not-modified', etag: string }
39+
40+
/**
41+
* The request headers `@harlan-zw/nuxt-cloudflare` treats as credentials.
42+
* A page rendered for a request with one is never stored, so it gets no ETag.
43+
*/
44+
const CREDENTIAL_HEADERS = ['cookie', 'authorization', 'proxy-authorization', 'cf-access-jwt-assertion', 'cf-access-client-id', 'x-api-key']
45+
46+
const HTML_TYPE_RE = /^(?:text\/html|application\/xhtml\+xml)\s*(?:;|$)/i
47+
48+
/** Sentry writes these into every render with a new trace id. */
49+
const PER_RENDER_META_RE = /<meta name="(?:sentry-trace|baggage)" content="[^"]*"\s*\/?>/g
50+
51+
const ENTITY_TAG_RE = /(?:W\/)?"[^"]*"/g
52+
53+
export async function decideHtmlValidators(page: RenderedPage): Promise<HtmlValidatorDecision> {
54+
if (page.method !== 'GET' && page.method !== 'HEAD')
55+
return { _tag: 'skip', reason: 'method' }
56+
if (page.status !== 200)
57+
return { _tag: 'skip', reason: 'status' }
58+
if (typeof page.body !== 'string' || !HTML_TYPE_RE.test(page.response.get('content-type') ?? ''))
59+
return { _tag: 'skip', reason: 'not-html' }
60+
if (CREDENTIAL_HEADERS.some(name => page.request.get(name)))
61+
return { _tag: 'skip', reason: 'credentials' }
62+
if (page.response.get('set-cookie'))
63+
return { _tag: 'skip', reason: 'sets-cookie' }
64+
if (!sharedCacheMayStore(page.response.get('cloudflare-cdn-cache-control')))
65+
return { _tag: 'skip', reason: 'not-shared' }
66+
67+
const etag = `W/"${page.buildId}-${await contentHash(page.body.replace(PER_RENDER_META_RE, ''))}"`
68+
return ifNoneMatchHits(page.request.get('if-none-match'), etag)
69+
? { _tag: 'not-modified', etag }
70+
: { _tag: 'send', etag }
71+
}
72+
73+
/**
74+
* True when the edge policy lets Workers Cache store the page for one visitor
75+
* and serve it to the next. Only such a page is the same for everyone.
76+
*/
77+
function sharedCacheMayStore(cacheControl: string | null): boolean {
78+
let lifetime = 0
79+
for (const directive of (cacheControl ?? '').toLowerCase().split(',')) {
80+
const [name = '', value] = directive.trim().split('=', 2)
81+
// `private="set-cookie"` keeps one header private, never the page.
82+
if (name === 'no-store' || (name === 'private' && value === undefined))
83+
return false
84+
if (name === 'max-age' || name === 's-maxage')
85+
lifetime = Math.max(lifetime, Number(value) || 0)
86+
}
87+
return lifetime > 0
88+
}
89+
90+
/** Weak comparison (RFC 9110 section 8.8.3.2), which `If-None-Match` uses. */
91+
function ifNoneMatchHits(header: string | null, etag: string): boolean {
92+
if (!header)
93+
return false
94+
if (header.trim() === '*')
95+
return true
96+
const opaque = etag.replace(/^W\//, '')
97+
return (header.match(ENTITY_TAG_RE) ?? []).some(tag => tag.replace(/^W\//, '') === opaque)
98+
}
99+
100+
async function contentHash(html: string): Promise<string> {
101+
const digest = await crypto.subtle.digest('SHA-256', new TextEncoder().encode(html))
102+
return Array.from(new Uint8Array(digest).slice(0, 16), byte => byte.toString(16).padStart(2, '0')).join('')
103+
}

‎test/unit/edge-cache-check.test.ts‎

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,22 +23,28 @@ interface Answer {
2323
body?: string
2424
}
2525

26+
const STORED_ETAG = 'W/"build-1-stored"'
27+
2628
function response(answer: Answer, path = '/skills/trending'): Response {
2729
const headers = new Headers({
2830
'content-type': 'text/html;charset=utf-8',
2931
'vary': 'Accept, Sec-Fetch-Dest, Host',
32+
...((answer.status ?? 200) === 200 ? { etag: STORED_ETAG } : {}),
3033
...answer.headers,
3134
})
3235
for (const cookie of answer.cookies ?? [])
3336
headers.append('set-cookie', cookie)
34-
return new Response(answer.body ?? page(null, false, path), { status: answer.status ?? 200, headers })
37+
const status = answer.status ?? 200
38+
// A 304 has no body, and the Response constructor refuses one.
39+
return new Response(status === 304 ? null : answer.body ?? page(null, false, path), { status, headers })
3540
}
3641

3742
/**
3843
* A fake edge. Every anonymous request after the first to a path is a hit,
39-
* unless a test overrides one kind of request.
44+
* and a request that sends the stored ETag back gets a 304, unless a test
45+
* overrides one kind of request.
4046
*/
41-
function edge(overrides: Partial<Record<'anonymous' | 'markdown' | 'session' | 'www', (path: string, seen: number) => Answer>> = {}): EdgeCacheFetch {
47+
function edge(overrides: Partial<Record<'anonymous' | 'markdown' | 'session' | 'www' | 'conditional', (path: string, seen: number) => Answer>> = {}): EdgeCacheFetch {
4248
const seen = new Map<string, number>()
4349
return async (url, init) => {
4450
const { pathname, search } = new URL(url)
@@ -63,6 +69,13 @@ function edge(overrides: Partial<Record<'anonymous' | 'markdown' | 'session' | '
6369
headers: { 'cf-cache-status': 'HIT' },
6470
}, path)
6571
}
72+
if (headers.get('if-none-match')) {
73+
return response(overrides.conditional?.(path, 0) ?? {
74+
status: 304,
75+
headers: { etag: STORED_ETAG },
76+
body: '',
77+
}, path)
78+
}
6679
const count = (seen.get(path) ?? 0) + 1
6780
seen.set(path, count)
6881
return response(overrides.anonymous?.(path, count) ?? {
@@ -291,6 +304,50 @@ describe('checkEdgeCache', () => {
291304
})
292305
})
293306

307+
it('sends the stored page\'s ETag back and expects a 304', async () => {
308+
const conditional: Array<string | null> = []
309+
const inner = edge()
310+
const result = await run(async (url, init) => {
311+
const headers = new Headers(init.headers)
312+
if (headers.get('if-none-match'))
313+
conditional.push(headers.get('if-none-match'))
314+
return inner(url, init)
315+
})
316+
317+
expect(result._tag).toBe('passed')
318+
expect(conditional).toEqual([STORED_ETAG, STORED_ETAG])
319+
})
320+
321+
it('fails when a stored page carries no ETag', async () => {
322+
const result = await run(edge({
323+
anonymous: (_path, count) => ({ headers: { 'cf-cache-status': count === 1 ? 'MISS' : 'HIT', 'etag': '' } }),
324+
}))
325+
326+
expect(result).toMatchObject({ _tag: 'failed', failures: [{ _tag: 'no-etag', path: '/skills/trending' }] })
327+
})
328+
329+
it('fails when the edge answers a matching ETag with the whole page', async () => {
330+
const result = await run(edge({ conditional: () => ({ headers: { 'cf-cache-status': 'HIT' } }) }))
331+
332+
expect(result).toMatchObject({ _tag: 'failed', failures: [{ _tag: 'not-revalidated', path: '/skills/trending', status: 200 }] })
333+
})
334+
335+
it('fails when a conditional request on a cold entry leaves a 304 for the next visitor', async () => {
336+
const inner = edge()
337+
const storedNotModified = new Set<string>()
338+
const result = await run(async (url, init) => {
339+
const headers = new Headers(init.headers)
340+
const variant = headers.get('accept') ?? ''
341+
if (headers.get('if-none-match') && variant.includes('x-edge-check'))
342+
storedNotModified.add(variant)
343+
if (!headers.get('if-none-match') && storedNotModified.has(variant))
344+
return response({ status: 304, body: '' })
345+
return inner(url, init)
346+
})
347+
348+
expect(result).toMatchObject({ _tag: 'failed', failures: [{ _tag: 'stored-not-modified', path: '/skills/trending', status: 304 }] })
349+
})
350+
294351
it('reports a network error instead of throwing', async () => {
295352
const result = await run(async () => {
296353
throw new Error('connect ECONNREFUSED')

0 commit comments

Comments
 (0)