Merge pull request github#15301 from github/repo-sync

repo sync

Author: Octomerger

middleware/handle-errors.js

@@ -1,5 +1,9 @@
 import FailBot from '../lib/failbot.js'
 import { nextApp } from './next.js'
+import { setFastlySurrogateKey, SURROGATE_ENUMS } from './set-fastly-surrogate-key.js'
+import { cacheControlFactory } from './cache-control.js'
+
+const cacheControl = cacheControlFactory(60) // 1 minute
 
 function shouldLogException(error) {
   const IGNORED_ERRORS = [
@@ -32,10 +36,34 @@ export default async function handleError(error, req, res, next) {
   // anywhere. So this is why we log it additionally.
   // Note, not using console.error() because it's arguably handled.
   // Some tests might actually expect a 500 error.
-  if (
-    process.env.NODE_ENV === 'test' &&
-    !(req.path.startsWith('/assets') || req.path.startsWith('/_next/static'))
-  ) {
+
+  if (req.path.startsWith('/assets') || req.path.startsWith('/_next/static')) {
+    // By default, Fastly will cache 404 responses unless otherwise
+    // told not to.
+    // See https://docs.fastly.com/en/guides/how-caching-and-cdns-work#http-status-codes-cached-by-default
+    // Most of the time, that's a good thing! Especially, if bombarded
+    // for some static asset that we don't have.
+    // E.g. `ab -n 10000 https://docs.github.com/assets/doesnotexist.png`
+    // But due to potential timing issue related to how the servers start,
+    // what might happen is that a new insteance comes up that
+    // contains `<script src="/_next/static/foo.1234.css">` in the HTML.
+    // The browser then proceeds to request /_next/static/foo.1234.css
+    // but this time it could be unluckily routed to a different instance
+    // that hasn't yet been upgraded, so they get a 404. And the CDN will
+    // notice this and cache it.
+    // Setting a tiny cache gets us a good compromise. It protects us
+    // against most stamping herds on 404s (thank you CDN!) but it also
+    // clears itself if you get that unlucky timing issue with rolling
+    // instances in a new deployment.
+    // For more background see issue 1553.
+    cacheControl(res)
+    // Undo the cookie setting that CSRF sets.
+    res.removeHeader('set-cookie')
+    // Makes sure the surrogate key is NOT the manual one if it failed.
+    // This basically unsets what was assumed in the beginning of
+    // loading all the middlewares.
+    setFastlySurrogateKey(res, SURROGATE_ENUMS.DEFAULT)
+  } else if (process.env.NODE_ENV === 'test') {
     console.warn('An error occurrred in some middleware handler', error)
   }
 

tests/rendering/static-assets.js

@@ -13,18 +13,19 @@ function getNextStaticAsset(directory) {
   return path.join(root, files[0])
 }
 
-function checkCachingHeaders(res, defaultSurrogateKey = false) {
+function checkCachingHeaders(res, defaultSurrogateKey = false, minMaxAge = 60 * 60) {
   expect(res.headers['set-cookie']).toBeUndefined()
   expect(res.headers['cache-control']).toContain('public')
   const maxAgeSeconds = parseInt(res.header['cache-control'].match(/max-age=(\d+)/)[1], 10)
   // Let's not be too specific in the tests, just as long as it's testing
   // that it's a reasonably large number of seconds.
-  expect(maxAgeSeconds).toBeGreaterThanOrEqual(60 * 60)
+  expect(maxAgeSeconds).toBeGreaterThanOrEqual(minMaxAge)
   // Because it doesn't have have a unique URL
   expect(res.headers['surrogate-key']).toBe(
     defaultSurrogateKey ? SURROGATE_ENUMS.DEFAULT : SURROGATE_ENUMS.MANUAL
   )
 }
+
 describe('static assets', () => {
   it('should serve /assets/cb-* with optimal headers', async () => {
     const res = await get('/assets/cb-1234/images/site/logo.png')
@@ -52,15 +53,19 @@ describe('static assets', () => {
     const res = await get('/assets/cb-1234/never/heard/of.png')
     expect(res.statusCode).toBe(404)
     expect(res.header['content-type']).toContain('text/plain')
+    // Only a tiny amount of Cache-Control on these
+    checkCachingHeaders(res, true, 60)
   })
   it('should 404 on /assets/ with plain text', async () => {
     const res = await get('/assets/never/heard/of.png')
     expect(res.statusCode).toBe(404)
     expect(res.header['content-type']).toContain('text/plain')
+    checkCachingHeaders(res, true, 60)
   })
   it('should 404 on /_next/static/ with plain text', async () => {
     const res = await get('/_next/static/never/heard/of.css')
     expect(res.statusCode).toBe(404)
     expect(res.header['content-type']).toContain('text/plain')
+    checkCachingHeaders(res, true, 60)
   })
 })