diff --git a/.env.example b/.env.example index 20128287..7fd6144a 100644 --- a/.env.example +++ b/.env.example @@ -9,8 +9,9 @@ KEY_ENCRYPTION_KEY=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= # AUDIT_RETENTION_DAYS=365 # Two names for one number, and they must agree when both are set. # -# The server accepts either PORT or SERVER_PORT (server/src/index.ts), preferring PORT when both -# are present and refusing to start if they disagree, so a single edit is enough. scripts/start.sh +# The server accepts either PORT or SERVER_PORT (server/src/config.ts), preferring PORT when both +# are present and refusing to start if they disagree, so a single edit is enough. An empty value +# is an unset one, so `PORT=` with SERVER_PORT set moves the server too. scripts/start.sh # and the app's Vite proxy read SERVER_PORT/APP_PORT, and docs/configuration.md documents # SERVER_PORT as the setting. Only PORT shipped here historically, so moving the server by # editing one line left the script still looking at 3001: it found whatever else was there, diff --git a/CHANGELOG.md b/CHANGELOG.md index d446e043..aeac3e60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### An empty `PORT` no longer starts the server on a port nobody asked for + +`PORT` and `SERVER_PORT` name one number, and either is meant to move the server. A `PORT` that was +declared but empty — a compose file passing a variable the host never set, or `PORT=` left in a +`.env` next to a `SERVER_PORT` that was set — was read as "set to nothing": `SERVER_PORT` was +ignored, the number parsed to `NaN`, and the server came up on an ephemeral port while everything +that polls `SERVER_PORT` reported it had never started. An empty value now counts as unset, the way +every other setting already treats it, and a value that is not a whole port number (`30o1` used to +start the server on port 30) refuses to start instead. + ### Coworkers are made in a wizard and managed in a dialog Creating a coworker is now a three-step wizard — who it is, who may see it, then where it runs, diff --git a/server/src/config.ts b/server/src/config.ts index 1c235f9c..aa9d89eb 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -132,6 +132,8 @@ export type HandoffCaps = { }; export type DeploymentConfig = { + /** The port the API listens on. Named `PORT` or `SERVER_PORT`; see `serverPort`. */ + port: number; databaseUrl: string; keyEncryptionKey: string; /** @@ -854,6 +856,44 @@ function agentStallTimeoutMs(environment: Environment): number { return milliseconds; } +/** Where the API listens when nothing says otherwise: what `.env.example` and the image ship. */ +const DEFAULT_PORT = 3001; + +/** + * The port the API listens on, from either of its two names. + * + * `PORT` and `SERVER_PORT` name one number: either moves the server, and two that disagree are + * refused at boot rather than half-applied. Read through `optional` like every other setting here, + * and that is the point. An unset variable declared in a compose file, or left as `PORT=` in a + * `.env`, arrives as an empty string rather than as absent, so `process.env.PORT ?? + * process.env.SERVER_PORT` never fell through to the second name, and `Number.parseInt("")` is + * `NaN`. Given `NaN`, `Bun.serve` binds an ephemeral port: the server came up somewhere nobody had + * asked for, `SERVER_PORT` ignored, and the script polling it reported a server that never + * started — the failure #312 set out to remove, back through the other name. + * + * A value that is not a whole port number is refused for the reason the caps above are: `30o1` + * used to start the server on port 30, and a typo has to fail where somebody is looking. + */ +function serverPort(environment: Environment): number { + const read = (name: string): number | undefined => { + const raw = optional(environment, name); + if (raw === undefined) return undefined; + const value = Number(raw); + if (!Number.isInteger(value) || value < 1 || value > 65535) { + throw new Error(`${name} must be a whole number between 1 and 65535`); + } + return value; + }; + const port = read("PORT"); + const serverPort = read("SERVER_PORT"); + if (port !== undefined && serverPort !== undefined && port !== serverPort) { + throw new Error( + `PORT (${port}) and SERVER_PORT (${serverPort}) disagree: set one or set both to the same value`, + ); + } + return port ?? serverPort ?? DEFAULT_PORT; +} + export function loadConfig( environment: Environment = process.env, ): DeploymentConfig { @@ -863,6 +903,7 @@ export function loadConfig( const workerSharedSecret = optional(environment, "WORKER_SHARED_SECRET"); return { + port: serverPort(environment), databaseUrl: required(environment, "DATABASE_URL"), keyEncryptionKey: keyEncryptionKey(environment), ...(managedAgent ? { managedAgent } : {}), diff --git a/server/src/index.ts b/server/src/index.ts index 6828cb9a..f91dd691 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -143,17 +143,9 @@ const identifyActor: IdentifyActor = async (request) => { }; const config = loadConfig(); -const rawPort = process.env.PORT ?? process.env.SERVER_PORT ?? "3001"; -if ( - process.env.PORT && - process.env.SERVER_PORT && - process.env.PORT !== process.env.SERVER_PORT -) { - throw new Error( - `PORT (${process.env.PORT}) and SERVER_PORT (${process.env.SERVER_PORT}) disagree: set one or set both to the same value`, - ); -} -const port = Number.parseInt(rawPort, 10); +// Read with the rest of the configuration, where an empty variable is an absent one. See +// `serverPort` in config.ts for what `process.env.PORT ?? …` did with `PORT=` instead. +const port = config.port; const database = createDatabase(config.databaseUrl); await initializeDevActorUser(database, config.singleUser); // The vault, built before the agent store because a customer's agent may sit behind a key and that diff --git a/server/tests/config.test.ts b/server/tests/config.test.ts index 5b1d9ce2..50b54ce5 100644 --- a/server/tests/config.test.ts +++ b/server/tests/config.test.ts @@ -394,6 +394,59 @@ describe("deployment configuration", () => { }, ); + test("listens on 3001 when neither PORT nor SERVER_PORT is set", () => { + expect(loadConfig(baseEnvironment).port).toBe(3001); + }); + + test("moves the server by either name", () => { + expect(loadConfig({ ...baseEnvironment, PORT: "3005" }).port).toBe(3005); + expect(loadConfig({ ...baseEnvironment, SERVER_PORT: "3005" }).port).toBe( + 3005, + ); + expect( + loadConfig({ ...baseEnvironment, PORT: " 3005 ", SERVER_PORT: "3005" }) + .port, + ).toBe(3005); + }); + + /* + * An unset variable declared in a compose file, or left as `PORT=` in a `.env`, arrives as an + * empty string rather than as absent. `process.env.PORT ?? process.env.SERVER_PORT` saw the empty + * string and never reached the second name, and `Number.parseInt("")` handed `Bun.serve` a NaN, + * which it answers by binding an ephemeral port nobody asked for. + */ + test("reads SERVER_PORT when PORT is declared but empty, and the other way round", () => { + expect( + loadConfig({ ...baseEnvironment, PORT: "", SERVER_PORT: "3005" }).port, + ).toBe(3005); + expect( + loadConfig({ ...baseEnvironment, PORT: "3005", SERVER_PORT: "" }).port, + ).toBe(3005); + expect( + loadConfig({ ...baseEnvironment, PORT: "", SERVER_PORT: "" }).port, + ).toBe(3001); + }); + + test("refuses to start when PORT and SERVER_PORT disagree", () => { + expect(() => + loadConfig({ ...baseEnvironment, PORT: "3001", SERVER_PORT: "3005" }), + ).toThrow("PORT (3001) and SERVER_PORT (3005) disagree"); + }); + + // `Number.parseInt("30o1")` is 30, and the server used to come up there. Refused instead, the way + // a mistyped cap is: a port has to fail at start-up, where somebody is looking. + test.each(["30o1", "three", "0", "65536", "1.5", "-1"])( + "refuses to start on PORT=%p", + (value) => { + expect(() => loadConfig({ ...baseEnvironment, PORT: value })).toThrow( + "PORT must be a whole number between 1 and 65535", + ); + expect(() => + loadConfig({ ...baseEnvironment, SERVER_PORT: value }), + ).toThrow("SERVER_PORT must be a whole number between 1 and 65535"); + }, + ); + test("configures Docker as the per-Bot computer provider", () => { const config = loadConfig({ ...baseEnvironment,