fix: handle 304 responses from functions (#373)

* test: add test for function returning 304 response

* test: run tests in dev dir

* fix: maintain bodyless responses

* test: enable watch mode in tests that test modifying functions

* chore: update CODEOWNERS to match all nested paths (#378)

* refactor: prefer early bails, use Set for more performant lookups

Author: pieh

packages/functions/dev/main.test.ts

@@ -28,6 +28,7 @@ describe('Functions with the v2 API syntax', () => {
       settings: {},
       timeouts: {},
       userFunctionsPath: 'netlify/functions',
+      watch: true,
     })
 
     const req1 = new Request('https://site.netlify/.netlify/functions/foo')
@@ -114,6 +115,39 @@ describe('Functions with the v2 API syntax', () => {
     expect(thirdChunk.done).toBeTruthy()
   })
 
+  test('Handles bodyless responses', async () => {
+    const source = `
+      export default async () =>
+        new Response(null,
+          {
+            status: 304,
+          },
+        )
+
+      export const config = { path: '/bodyless-response' }
+    `
+    const fixture = new Fixture().withFile('netlify/functions/bodyless-response.mjs', source)
+
+    const directory = await fixture.create()
+    const destPath = join(directory, 'functions-serve')
+    const functions = new FunctionsHandler({
+      accountId: 'account-123',
+      config: {},
+      destPath,
+      geolocation: {},
+      projectRoot: directory,
+      settings: {},
+      timeouts: {},
+      userFunctionsPath: 'netlify/functions',
+    })
+
+    const req = new Request('https://site.netlify/bodyless-response')
+    const match = await functions.match(req, destPath)
+    expect(match).not.toBeUndefined()
+    const res = await match!.handle(req)
+    expect(res.status).toBe(304)
+  })
+
   test('Returns a `preferStatic` property', async () => {
     const fixture = new Fixture().withFile(
       'netlify/functions/hello.mjs',
@@ -139,6 +173,7 @@ describe('Functions with the v2 API syntax', () => {
       settings: {},
       timeouts: {},
       userFunctionsPath: 'netlify/functions',
+      watch: true,
     })
 
     const req1 = new Request('https://site.netlify/hello')

packages/functions/dev/runtimes/nodejs/worker.js

@@ -10,6 +10,9 @@ import { isStream } from 'is-stream'
 import lambdaLocal from 'lambda-local'
 import sourceMapSupport from 'source-map-support'
 
+// https://github.com/nodejs/undici/blob/a36e299d544863c5ade17d4090181be894366024/lib/web/fetch/constants.js#L6
+const nullBodyStatus = new Set([101, 204, 205, 304])
+
 /**
  * @typedef HandlerResponse
  * @type {import('../../../src/function/handler_response.js').HandlerResponse}
@@ -46,17 +49,33 @@ const invocationResult = /** @type {HandlerResponse} */ (
   })
 )
 
-/** @type {number | undefined} */
-let streamPort
+/**
+ * When the result body is a stream and result status code allow to have a body,
+ * open up a http server that proxies back to the main thread and resolve with server port.
+ * Otherwise, resolve with undefined.
+ *
+ * @param {HandlerResponse} invocationResult
+ * @returns {Promise<number | undefined>}
+ */
+async function getStreamPortForStreamingResponse(invocationResult) {
+  // if we don't have result or result's body is not a stream, we do not need a stream port
+  if (!invocationResult || !isStream(invocationResult.body)) {
+    return undefined
+  }
 
-// When the result body is a StreamResponse
-// we open up a http server that proxies back to the main thread.
-if (invocationResult && isStream(invocationResult.body)) {
   const { body } = invocationResult
 
   delete invocationResult.body
 
-  await new Promise((resolve, reject) => {
+  // For streaming responses, lambda-local always returns a result with body stream.
+  // We need to discard it if result's status code does not allow response to have a body.
+  const shouldNotHaveABody = nullBodyStatus.has(invocationResult.statusCode)
+  if (shouldNotHaveABody) {
+    return undefined
+  }
+
+  // create a server that will proxy the body stream back to the main thread
+  return await new Promise((resolve, reject) => {
     const server = createServer((socket) => {
       body.pipe(socket).on('end', () => server.close())
     })
@@ -66,15 +85,19 @@ if (invocationResult && isStream(invocationResult.body)) {
     server.listen({ port: 0, host: 'localhost' }, () => {
       const address = server.address()
 
+      /** @type {number | undefined} */
+      let streamPort
       if (address && typeof address !== 'string') {
         streamPort = address.port
       }
 
-      resolve(undefined)
+      resolve(streamPort)
     })
   })
 }
 
+const streamPort = await getStreamPortForStreamingResponse(invocationResult)
+
 if (parentPort) {
   /** @type {WorkerResult} */
   const message = { ...invocationResult, streamPort }

packages/functions/vitest.config.ts

@@ -5,7 +5,7 @@ export default defineConfig({
     target: 'esnext',
   },
   test: {
-    include: ['src/**/*.test.ts'],
+    include: ['(src|dev)/**/*.test.ts'],
     testTimeout: 30_000,
   },
 })