Fix race condition between fetch timeout and Express response timeout

src/ExpressSetup.ts

@@ -5,12 +5,7 @@ import express, { Express } from "express";
 import * as Http from "node:http";
 import { ThumbnailApi } from "./ThumbnailApi";
 import { Server } from "node:http";
-
-// How long we wait for a request from a socket
-const REQUEST_TIMEOUT = 3000; // 3 secs
-
-// How long we wait on piping a response before we give up
-const RESPONSE_TIMEOUT = 10000; // 10 seconds
+import { REQUEST_TIMEOUT_MS, RESPONSE_TIMEOUT_MS } from "./timeoutConfig";
 
 export class ExpressSetup {
   app: Express;
@@ -77,7 +72,7 @@ export class ExpressSetup {
   }
 
   setRequestTimeout(server: Http.Server) {
-    server.requestTimeout = REQUEST_TIMEOUT;
+    server.requestTimeout = REQUEST_TIMEOUT_MS;
   }
 
   server(port: number): Http.Server {
@@ -90,9 +85,15 @@ export class ExpressSetup {
     // next two methods are like this to make
     // eslint happy about the async get handler
     const handler = async (req: express.Request, res: express.Response) => {
-      res.setTimeout(RESPONSE_TIMEOUT, () => {
-        res.status(504);
-        res.send("Gateway Timeout");
+      res.setTimeout(RESPONSE_TIMEOUT_MS, () => {
+        if (res.writableEnded) {
+          return;
+        }
+        if (res.headersSent) {
+          res.destroy();
+          return;
+        }
+        res.status(504).send("Gateway Timeout");
       });
       try {
         await thumbnailApi.handle(req, res);

src/ResponseHelper.ts

@@ -2,11 +2,12 @@ import { Readable } from "stream";
 import { pipeline } from "stream/promises";
 import express from "express";
 import { getLogger } from "./logger";
+import { FETCH_TIMEOUT_MS } from "./timeoutConfig";
 
 const logger = getLogger();
 
 export class ResponseHelper {
-  FETCH_TIMEOUT = 10 * 1000; // 10 seconds;
+  FETCH_TIMEOUT = FETCH_TIMEOUT_MS;
 
   async pipe(body: ReadableStream, expressResponse: express.Response): Promise<void> {
     try {
@@ -99,7 +100,7 @@ export class ResponseHelper {
   // whereas s3 responses should live there for a long time
   // see LONG_CACHE_TIME and SHORT_CACHE_TIME, above
   getCacheHeaders(seconds: number): Map<string, string> {
-    const now = new Date().getTime();
+    const now = Date.now();
     const expirationDateString = new Date(now + 1000 * seconds).toUTCString();
     const cacheControl = `public, max-age=${String(seconds)}`;
     return new Map([

src/timeoutConfig.ts

@@ -0,0 +1,17 @@
+// REQUEST_TIMEOUT: how long we wait for a request from a socket
+export const REQUEST_TIMEOUT_MS = 3_000;
+
+// RESPONSE_TIMEOUT: how long before Express gives up on the full response
+export const RESPONSE_TIMEOUT_MS = 10_000;
+
+// FETCH_TIMEOUT: how long to wait on an upstream image fetch.
+// Must be well under RESPONSE_TIMEOUT_MS so that AbortSignal fires and
+// sendError(404) completes before res.setTimeout sends a 504,
+// preventing ERR_HTTP_HEADERS_SENT.
+export const FETCH_TIMEOUT_MS = 5_000;
+
+if (FETCH_TIMEOUT_MS >= RESPONSE_TIMEOUT_MS) {
+  throw new Error(
+    `Invalid timeout config: FETCH_TIMEOUT_MS (${FETCH_TIMEOUT_MS}) must be less than RESPONSE_TIMEOUT_MS (${RESPONSE_TIMEOUT_MS})`,
+  );
+}