From 544ca632c0ce60f161620f46e3d78b4274cdfa0c Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Mon, 22 Jun 2026 17:03:14 +0000 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=A4=96=20fix(site/e2e):=20close=20moc?= =?UTF-8?q?k=20external-auth=20servers=20in=20teardown?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createServer now returns a close handle so callers can release the listener after the test. The web flow closes in afterAll; the device flow uses try/finally. This prevents leaked listeners from triggering EADDRINUSE on the next run, the long-standing root cause of the externalAuth.spec.ts flake (coder/internal#356). --- site/e2e/helpers.ts | 35 ++++++++++-- site/e2e/tests/externalAuth.spec.ts | 85 +++++++++++++++++------------ 2 files changed, 78 insertions(+), 42 deletions(-) diff --git a/site/e2e/helpers.ts b/site/e2e/helpers.ts index dc68cba15f2..eccc8aca5cd 100644 --- a/site/e2e/helpers.ts +++ b/site/e2e/helpers.ts @@ -1,5 +1,6 @@ import { type ChildProcess, exec, spawn } from "node:child_process"; import { randomUUID } from "node:crypto"; +import type { Server } from "node:http"; import net from "node:net"; import path from "node:path"; import { Duplex } from "node:stream"; @@ -865,17 +866,39 @@ export class Awaiter { } } -export const createServer = async ( - port: number, -): Promise> => { +type MockServer = { + app: ReturnType; + /** + * close stops the server and force-closes any lingering keep-alive + * connections so the port is released promptly. Callers must invoke + * this in their teardown (e.g. test.afterAll, try/finally) to avoid + * leaking the listener into the next test run, which historically + * caused EADDRINUSE flakes in this suite. + */ + close: () => Promise; +}; + +export const createServer = async (port: number): Promise => { await waitForPort(port); // Wait until the port is available - const e = express(); + const app = express(); // We need to specify the local IP address as the web server // tends to fail with IPv6 related error: // listen EADDRINUSE: address already in use :::50516 - await new Promise((r) => e.listen(port, "0.0.0.0", r)); - return e; + const server = await new Promise((resolve) => { + const s = app.listen(port, "0.0.0.0", () => resolve(s)); + }); + + return { + app, + close: () => + new Promise((resolve, reject) => { + // closeAllConnections (Node >= 18.2 / 16.17) forces lingering + // keep-alives to terminate so server.close() is bounded. + server.closeAllConnections?.(); + server.close((err) => (err ? reject(err) : resolve())); + }), + }; }; async function waitForPort( diff --git a/site/e2e/tests/externalAuth.spec.ts b/site/e2e/tests/externalAuth.spec.ts index 796dd0644e9..441eec4ec16 100644 --- a/site/e2e/tests/externalAuth.spec.ts +++ b/site/e2e/tests/externalAuth.spec.ts @@ -14,8 +14,11 @@ import { beforeCoderTest, resetExternalAuthKey } from "../hooks"; test.describe .skip("externalAuth", () => { + let closeWebServer: (() => Promise) | undefined; + test.beforeAll(async ({ baseURL }) => { - const srv = await createServer(gitAuth.webPort); + const { app: srv, close } = await createServer(gitAuth.webPort); + closeWebServer = close; // The GitHub validate endpoint returns the currently authenticated user! srv.use(gitAuth.validatePath, (_req, res) => { @@ -34,6 +37,10 @@ test.describe }); }); + test.afterAll(async () => { + await closeWebServer?.(); + }); + test.beforeEach(async ({ context, page }) => { beforeCoderTest(page); await login(page); @@ -51,43 +58,49 @@ test.describe }; // Start a server to mock the GitHub API. - const srv = await createServer(gitAuth.devicePort); - srv.use(gitAuth.validatePath, (_req, res) => { - res.write(JSON.stringify(ghUser)); - res.end(); - }); - srv.use(gitAuth.codePath, (_req, res) => { - res.write(JSON.stringify(device)); - res.end(); - }); - srv.use(gitAuth.installationsPath, (_req, res) => { - res.write(JSON.stringify(ghInstall)); - res.end(); - }); + const { app: srv, close: closeServer } = await createServer( + gitAuth.devicePort, + ); + try { + srv.use(gitAuth.validatePath, (_req, res) => { + res.write(JSON.stringify(ghUser)); + res.end(); + }); + srv.use(gitAuth.codePath, (_req, res) => { + res.write(JSON.stringify(device)); + res.end(); + }); + srv.use(gitAuth.installationsPath, (_req, res) => { + res.write(JSON.stringify(ghInstall)); + res.end(); + }); - const token = { - access_token: "", - error: "authorization_pending", - error_description: "", - }; - // First we send a result from the API that the token hasn't been - // authorized yet to ensure the UI reacts properly. - const sentPending = new Awaiter(); - srv.use(gitAuth.tokenPath, (_req, res) => { - res.write(JSON.stringify(token)); - res.end(); - sentPending.done(); - }); + const token = { + access_token: "", + error: "authorization_pending", + error_description: "", + }; + // First we send a result from the API that the token hasn't been + // authorized yet to ensure the UI reacts properly. + const sentPending = new Awaiter(); + srv.use(gitAuth.tokenPath, (_req, res) => { + res.write(JSON.stringify(token)); + res.end(); + sentPending.done(); + }); - await page.goto(`/external-auth/${gitAuth.deviceProvider}`, { - waitUntil: "domcontentloaded", - }); - await page.getByText(device.user_code).isVisible(); - await sentPending.wait(); - // Update the token to be valid and ensure the UI updates! - token.error = ""; - token.access_token = "hello-world"; - await page.waitForSelector("text=1 organization authorized"); + await page.goto(`/external-auth/${gitAuth.deviceProvider}`, { + waitUntil: "domcontentloaded", + }); + await page.getByText(device.user_code).isVisible(); + await sentPending.wait(); + // Update the token to be valid and ensure the UI updates! + token.error = ""; + token.access_token = "hello-world"; + await page.waitForSelector("text=1 organization authorized"); + } finally { + await closeServer(); + } }); test("external auth web", async ({ page }) => { From ac05271f8534786a8b666dc702265bc797aaa25d Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Wed, 24 Jun 2026 03:18:48 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=A4=96=20fix(site/e2e):=20close=20lis?= =?UTF-8?q?tener=20before=20forcing=20keep-alives?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reorders the teardown sequence so server.close() runs first, then server.closeAllConnections(). The previous order let a connection accepted between the two calls escape the force-close, keeping close() pending until the keep-alive timeout. Addresses review on #26575. --- site/e2e/helpers.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/site/e2e/helpers.ts b/site/e2e/helpers.ts index eccc8aca5cd..c72afbef55f 100644 --- a/site/e2e/helpers.ts +++ b/site/e2e/helpers.ts @@ -893,10 +893,14 @@ export const createServer = async (port: number): Promise => { app, close: () => new Promise((resolve, reject) => { - // closeAllConnections (Node >= 18.2 / 16.17) forces lingering - // keep-alives to terminate so server.close() is bounded. - server.closeAllConnections?.(); + // Call server.close() first so the listener stops accepting + // new connections, then closeAllConnections() (Node >= 18.2 / + // 16.17) drops lingering keep-alives so close() resolves + // promptly. Inverting this order races: a connection accepted + // between closeAllConnections() and close() is not force- + // closed and keeps close() pending until keep-alive timeout. server.close((err) => (err ? reject(err) : resolve())); + server.closeAllConnections?.(); }), }; }; From e29c0b970d2e48c86d691e737d534edda23e82b5 Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Mon, 29 Jun 2026 02:57:51 +0000 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=A4=96=20chore(site/e2e):=20trim=20mo?= =?UTF-8?q?ck=20server=20teardown=20comments?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- site/e2e/helpers.ts | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/site/e2e/helpers.ts b/site/e2e/helpers.ts index c72afbef55f..1acaa8cbdbe 100644 --- a/site/e2e/helpers.ts +++ b/site/e2e/helpers.ts @@ -868,13 +868,7 @@ export class Awaiter { type MockServer = { app: ReturnType; - /** - * close stops the server and force-closes any lingering keep-alive - * connections so the port is released promptly. Callers must invoke - * this in their teardown (e.g. test.afterAll, try/finally) to avoid - * leaking the listener into the next test run, which historically - * caused EADDRINUSE flakes in this suite. - */ + /** Stops the server and drops keep-alive connections. */ close: () => Promise; }; @@ -893,12 +887,7 @@ export const createServer = async (port: number): Promise => { app, close: () => new Promise((resolve, reject) => { - // Call server.close() first so the listener stops accepting - // new connections, then closeAllConnections() (Node >= 18.2 / - // 16.17) drops lingering keep-alives so close() resolves - // promptly. Inverting this order races: a connection accepted - // between closeAllConnections() and close() is not force- - // closed and keeps close() pending until keep-alive timeout. + // Order matters: stop accepting, then drop keep-alives. server.close((err) => (err ? reject(err) : resolve())); server.closeAllConnections?.(); }),