fix: ensure background work is finished when response has 3xx or 5xx status code (#2742)

* test: add test for ISR case that always return same body

* fix: ensure background work is finished when response has 3xx or 5xx status code

Author: pieh

src/run/handlers/server.ts

@@ -113,34 +113,31 @@ export default async (request: Request) => {
     setVaryHeaders(response.headers, request, nextConfig)
     setCacheStatusHeader(response.headers)
 
-    // Temporary workaround for an issue where sending a response with an empty
-    // body causes an unhandled error. This doesn't catch everything, but redirects are the
-    // most common case of sending empty bodies. We can't check it directly because these are streams.
-    // The side effect is that responses which do contain data will not be streamed to the client,
-    // but that's fine for redirects.
-    // TODO: Remove once a fix has been rolled out.
-    if ((response.status > 300 && response.status < 400) || response.status >= 500) {
-      const body = await response.text()
-      return new Response(body || null, response)
+    async function waitForBackgroundWork() {
+      // it's important to keep the stream open until the next handler has finished
+      await nextHandlerPromise
+
+      // Next.js relies on `close` event emitted by response to trigger running callback variant of `next/after`
+      // however @fastly/http-compute-js never actually emits that event - so we have to emit it ourselves,
+      // otherwise Next would never run the callback variant of `next/after`
+      res.emit('close')
+
+      // We have to keep response stream open until tracked background promises that are don't use `context.waitUntil`
+      // are resolved. If `context.waitUntil` is available, `requestContext.backgroundWorkPromise` will be empty
+      // resolved promised and so awaiting it is no-op
+      await requestContext.backgroundWorkPromise
     }
 
     const keepOpenUntilNextFullyRendered = new TransformStream({
       async flush() {
-        // it's important to keep the stream open until the next handler has finished
-        await nextHandlerPromise
-
-        // Next.js relies on `close` event emitted by response to trigger running callback variant of `next/after`
-        // however @fastly/http-compute-js never actually emits that event - so we have to emit it ourselves,
-        // otherwise Next would never run the callback variant of `next/after`
-        res.emit('close')
-
-        // We have to keep response stream open until tracked background promises that are don't use `context.waitUntil`
-        // are resolved. If `context.waitUntil` is available, `requestContext.backgroundWorkPromise` will be empty
-        // resolved promised and so awaiting it is no-op
-        await requestContext.backgroundWorkPromise
+        await waitForBackgroundWork()
       },
     })
 
+    if (!response.body) {
+      await waitForBackgroundWork()
+    }
+
     return new Response(response.body?.pipeThrough(keepOpenUntilNextFullyRendered), response)
   })
 }

tests/e2e/page-router.test.ts

@@ -494,6 +494,45 @@ test.describe('Simple Page Router (no basePath, no i18n)', () => {
       '.env.production.local': 'defined in .env.production.local',
     })
   })
+
+  test('ISR pages that are the same after regeneration execute background getStaticProps uninterrupted', async ({
+    page,
+    pageRouter,
+  }) => {
+    const slug = Date.now()
+
+    await page.goto(new URL(`always-the-same-body/${slug}`, pageRouter.url).href)
+
+    await new Promise((resolve) => setTimeout(resolve, 15_000))
+
+    await page.goto(new URL(`always-the-same-body/${slug}`, pageRouter.url).href)
+
+    await new Promise((resolve) => setTimeout(resolve, 15_000))
+
+    await page.goto(new URL(`always-the-same-body/${slug}`, pageRouter.url).href)
+
+    await new Promise((resolve) => setTimeout(resolve, 15_000))
+
+    // keep lambda executing to allow for background getStaticProps to finish in case background work execution was suspended
+    await fetch(new URL(`api/sleep-5`, pageRouter.url).href)
+
+    const response = await fetch(new URL(`read-static-props-blobs/${slug}`, pageRouter.url).href)
+    expect(response.ok, 'response for stored data status should not fail').toBe(true)
+
+    const data = await response.json()
+
+    expect(typeof data.start, 'timestamp of getStaticProps start should be a number').toEqual(
+      'number',
+    )
+    expect(typeof data.end, 'timestamp of getStaticProps end should be a number').toEqual('number')
+
+    // duration should be around 5s overall, due to 5s timeout, but this is not exact so let's be generous and allow 10 seconds
+    // which is still less than 15 seconds between requests
+    expect(
+      data.end - data.start,
+      'getStaticProps duration should not be longer than 10 seconds',
+    ).toBeLessThan(10_000)
+  })
 })
 
 test.describe('Page Router with basePath and i18n', () => {

tests/fixtures/page-router/netlify/functions/read-static-props-blobs.ts

@@ -0,0 +1,27 @@
+import { getDeployStore } from '@netlify/blobs'
+import { Context } from '@netlify/functions'
+
+function numberOrNull(value: string | null) {
+  if (!value) {
+    return null
+  }
+
+  const maybeNumber = parseInt(value)
+  return isNaN(maybeNumber) ? null : maybeNumber
+}
+
+// intentionally using Netlify Function to not hit Next.js server handler function instance
+// to avoid potentially resuming suspended execution
+export default async function handler(_request: Request, context: Context) {
+  const slug = context.params['slug']
+
+  const store = getDeployStore({ name: 'get-static-props-tracker', consistency: 'strong' })
+
+  const [start, end] = await Promise.all([store.get(`${slug}-start`), store.get(`${slug}-end`)])
+
+  return Response.json({ slug, start: numberOrNull(start), end: numberOrNull(end) })
+}
+
+export const config = {
+  path: '/read-static-props-blobs/:slug',
+}

tests/fixtures/page-router/package.json

@@ -8,6 +8,7 @@
     "build": "next build"
   },
   "dependencies": {
+    "@netlify/blobs": "^8.1.0",
     "@netlify/functions": "^2.7.0",
     "next": "latest",
     "react": "18.2.0",

tests/fixtures/page-router/pages/always-the-same-body/[slug].js

@@ -0,0 +1,50 @@
+import { getDeployStore } from '@netlify/blobs'
+
+const Show = ({ slug }) => {
+  // ensure that the content is stable to trigger 304 responses
+  return <pre>{slug}</pre>
+}
+
+/** @type {import('next').getStaticPaths} */
+export async function getStaticPaths() {
+  return {
+    paths: [],
+    fallback: 'blocking',
+  }
+}
+
+/** @type {import('next').GetStaticProps} */
+export async function getStaticProps({ params }) {
+  const store = getDeployStore({ name: 'get-static-props-tracker', consistency: 'strong' })
+
+  const start = Date.now()
+
+  console.log(`[timestamp] ${params.slug} getStaticProps start`)
+
+  const storeStartPromise = store.set(`${params.slug}-start`, start).then(() => {
+    console.log(`[timestamp] ${params.slug} getStaticProps start stored`)
+  })
+
+  // simulate a long running operation
+  await new Promise((resolve) => setTimeout(resolve, 5000))
+
+  const storeEndPromise = store.set(`${params.slug}-end`, Date.now()).then(() => {
+    console.log(`[timestamp] ${params.slug} getStaticProps end stored`)
+  })
+
+  console.log(
+    `[timestamp] ${params.slug} getStaticProps end (duration: ${(Date.now() - start) / 1000}s)`,
+  )
+
+  await Promise.all([storeStartPromise, storeEndPromise])
+
+  // ensure that the data is stable and always the same to trigger 304 responses
+  return {
+    props: {
+      slug: params.slug,
+    },
+    revalidate: 5,
+  }
+}
+
+export default Show

tests/fixtures/page-router/pages/api/sleep-5.js

@@ -0,0 +1,5 @@
+export default async function handler(req, res) {
+  await new Promise((resolve) => setTimeout(resolve, 5000))
+
+  res.json({ message: 'ok' })
+}