fix(server): Keep the idle limit on tunnels and proxied connections

README.md

@@ -760,12 +760,12 @@ To see how well the cache works:
 - Request headers that do not come in 10 s get HTTP 408, and a request
   whose body does not come in 300 s too. This is also true in a tunnel
   and with `proxyProtocol`.
-- A `CONNECT` tunnel that sends no byte for 10 s after the `200` closes. A
-  tunnel that sends and gets no byte for 10 minutes while a request waits
-  for its response closes. Between two requests, a tunnel closes after
-  about 5 s without a new request (the `keepAliveTimeout` of Node), and
-  the client opens a new one. A gap: in a tunnel without TLS, Node removes
-  the 10-minute limit after the first request.
+- A `CONNECT` tunnel that sends no byte for 10 s after the `200` closes.
+- A connection or tunnel that sends and gets no byte for 10 minutes
+  closes, also while a request waits for its response. This is true for
+  each request, with or without TLS. Between two requests, a connection
+  closes after 6 s without a new request (the `keepAliveTimeout` of Node,
+  5 s, and 1 s that Node adds), and the client opens a new one.
 - The credentials that a run learns and the values that the sessions of
   a run without the token remember have limits. See [Config](#config) and
   [Changing values](#changing-values).

packages/service/src/server.ts

@@ -70,11 +70,12 @@ const MAX_BODY_BYTES = 64 * 1024 * 1024;
 /** A tunnel that sends no byte this long after its `200` closes. */
 const TUNNEL_FIRST_BYTE_MS = 10_000;
 /**
- * A tunnel that sends and gets no byte this long closes. A live model
- * call can take minutes before its first byte, so this is long. Between
- * two requests, Node closes the tunnel sooner (`keepAliveTimeout`, 5 s).
+ * A connection or tunnel that sends and gets no byte this long closes. A
+ * live model call can take minutes before its first byte, so this is long.
+ * Between two requests, Node closes the connection sooner
+ * (`keepAliveTimeout`, 5 s, and 1 s that Node adds).
  */
-const TUNNEL_IDLE_MS = 10 * 60 * 1000;
+const IDLE_MS = 10 * 60 * 1000;
 /**
  * The HTTP servers of the proxy. Headers that do not come in 10 s get
  * HTTP 408 from Node. A whole request, with its body, has Node's default
@@ -520,6 +521,11 @@ export async function listenProxy(
     }
     serveProxied(tunnel, incoming, outgoing, tunnel.origin);
   });
+  // Between two requests, Node sets the time limit of a socket to its
+  // `keepAliveTimeout`, and at the next request back to `timeout`. With
+  // the default `timeout` of 0, a later request in a tunnel without TLS
+  // had no idle limit. A TLS tunnel also keeps the limit of its raw socket.
+  tunnelServer.timeout = IDLE_MS;
   const server = http.createServer(HTTP_OPTIONS, (incoming, outgoing) => {
     // A request to the proxy itself has a path. A proxied plain HTTP
     // request has an absolute URL.
@@ -550,6 +556,10 @@ export async function listenProxy(
   const listener = options.proxyProtocol
     ? net.createServer((socket) => acceptProxied(socket, server))
     : server;
+  // Node's `requestTimeout` stops when a request is read. Without this, a
+  // request that waits on a hung upstream, or a client that does not read
+  // its response, holds the connection with no limit.
+  server.timeout = IDLE_MS;
   listener.on("connection", (socket: Socket) => {
     sockets.add(socket);
     socket.on("close", () => sockets.delete(socket));
@@ -597,7 +607,7 @@ export async function listenProxy(
       }
       socket.write("HTTP/1.1 200 Connection Established\r\n\r\n");
       // A tunnel that a client leaves open must not stay open forever.
-      socket.setTimeout(TUNNEL_IDLE_MS, () => socket.destroy());
+      socket.setTimeout(IDLE_MS, () => socket.destroy());
       const open = async (first: Buffer) => {
         socket.unshift(first);
         // A TLS handshake starts with byte 0x16. Anything else is plain HTTP.

packages/service/tests/service.test.ts

@@ -1450,6 +1450,59 @@ describe("limits of the service", () => {
       ]),
     ).resolves.toBe("closed");
   });
+  it.each(["a tunnel without TLS", "an absolute-form connection"])(
+    "keeps the idle limit of %s after its first request",
+    async (kind) => {
+      // Between two requests, Node sets the time limit of a socket to its
+      // keep-alive time, and at the next request to the `timeout` of the
+      // server. Before, that was 0, so a later request had no idle limit,
+      // also after Node read all of it (`requestTimeout` stops there).
+      const run = await startRun(undefined, "replay");
+      const host = new URL(origin).host;
+      let socket: ReturnType<typeof connect>;
+      let post: string;
+      if (kind === "a tunnel without TLS") {
+        socket = await openTunnel(service.url, run.url);
+        post = `POST /v1/messages HTTP/1.1\r\nhost: ${host}\r\ncontent-length: 2\r\n\r\n`;
+      } else {
+        const { username, password } = new URL(run.url);
+        const credentials = Buffer.from(
+          `${decodeURIComponent(username)}:${decodeURIComponent(password)}`,
+        ).toString("base64");
+        socket = connect(Number(new URL(service.url).port), "127.0.0.1");
+        onTestFinished(() => {
+          socket.destroy();
+        });
+        await once(socket, "connect");
+        post = `POST ${origin}/v1/messages HTTP/1.1\r\nhost: ${host}\r\nproxy-authorization: Basic ${credentials}\r\ncontent-length: 2\r\n\r\n`;
+      }
+      // The service has its end of the connection in a socket of the same
+      // class.
+      const socketClass = Object.getPrototypeOf(socket) as typeof socket;
+      const setTimeoutSpy = vi.spyOn(socketClass, "setTimeout");
+      onTestFinished(() => setTimeoutSpy.mockRestore());
+      const port = Number(new URL(service.url).port);
+      /** The time limits that the service set on its end of the connection. */
+      const limits = () =>
+        setTimeoutSpy.mock.calls.flatMap(([ms], n) => {
+          const peer = setTimeoutSpy.mock.contexts[n] as typeof socket;
+          const ours =
+            peer.localPort === port && peer.remotePort === socket.localPort;
+          return ours ? [ms] : [];
+        });
+      socket.write(`${post}{}`);
+      const [first] = (await once(socket, "data")) as [Buffer];
+      expect(first.toString()).toMatch(/^HTTP\/1.1 412 /);
+      // Node waits 5 s and 1 s more for the next request.
+      await until(() => limits().at(-1) === 6000);
+      const before = limits().length;
+      // A second request whose body does not come.
+      socket.write(`${post}{`);
+      await until(() => limits().length > before);
+      expect(limits().at(-1)).toBe(10 * 60 * 1000);
+    },
+  );
+
   it("times out slow headers, also with proxyProtocol and in a tunnel", async () => {
     // With `proxyProtocol`, the HTTP servers get their connections from a
     // TCP server, and Node did not check their time limits.