diff --git a/worker/src/env.ts b/worker/src/env.ts new file mode 100644 index 000000000..4cce2f0b8 --- /dev/null +++ b/worker/src/env.ts @@ -0,0 +1,68 @@ +import { randomUUID } from "node:crypto"; + +/** + * What the worker needs from its environment, parsed and ready to use. + * + * `serverInternalUrl` never carries a trailing slash, so `routineRunUrl` cannot + * produce the double-slash `//internal/routines/run` that a `SERVER_INTERNAL_URL` + * with a trailing slash used to build — a 404 the sweep only reported as "the server + * answered 404 rather than 202". `owner` falls back to a random suffix whenever + * `HOSTNAME` is absent, empty or whitespace-only, so two workers never share a lease + * name the way `routines/` alone would. + */ +export type WorkerEnv = { + workerSharedSecret: string; + serverInternalUrl: string; + databaseUrl: string; + owner: string; +}; + +/** + * Read and validate the worker's three settings, failing fast and loudly. + * + * Whitespace-only values are refused exactly like unset ones: the old `if (!value)` + * guards let `" "` through, and the loop then failed on every tick — `fetch` to + * `" /internal/..."`, `createDatabase(" ")` on the first query — logging + * `routine-sweep-tick-failed` forever instead of saying at boot what was misconfigured. + */ +export function loadWorkerEnv( + environment: Record = process.env, + generateId: () => string = () => randomUUID().slice(0, 8), +): WorkerEnv { + const workerSharedSecret = environment.WORKER_SHARED_SECRET?.trim(); + if (!workerSharedSecret) { + throw new Error( + "WORKER_SHARED_SECRET is not set, so this worker cannot authenticate itself to /internal/routines/run and no routine could be fired.", + ); + } + + const rawServerUrl = environment.SERVER_INTERNAL_URL?.trim(); + if (!rawServerUrl) { + throw new Error( + "SERVER_INTERNAL_URL is not set, so this worker does not know where to hand a routine run.", + ); + } + const serverInternalUrl = rawServerUrl.replace(/\/+$/, ""); + if (!serverInternalUrl) { + throw new Error( + "SERVER_INTERNAL_URL is not set, so this worker does not know where to hand a routine run.", + ); + } + + const databaseUrl = environment.DATABASE_URL?.trim(); + if (!databaseUrl) { + throw new Error( + "DATABASE_URL is not set, so this worker has no database to read routines from or claim them in.", + ); + } + + const host = environment.HOSTNAME?.trim(); + const owner = `routines/${host || generateId()}`; + + return { workerSharedSecret, serverInternalUrl, databaseUrl, owner }; +} + +/** Where a claimed run is handed to the server. Built on the normalised base URL. */ +export function routineRunUrl(serverInternalUrl: string): string { + return `${serverInternalUrl}/internal/routines/run`; +} diff --git a/worker/src/index.ts b/worker/src/index.ts index 38e7dfa84..79b9325ad 100644 --- a/worker/src/index.ts +++ b/worker/src/index.ts @@ -17,7 +17,6 @@ * tries again on the next tick. So every phase below gets its own try/catch, and nothing here ever * lets a phase's error reach the top and take the process down. */ -import { randomUUID } from "node:crypto"; import { createDatabase } from "../../server/src/db/client"; import { createRoutineStore } from "../../server/src/routines/store"; import { @@ -27,64 +26,35 @@ import { type RoutineSweepOptions, } from "../../server/src/routines/sweep"; import { createWorkQueue } from "../../server/src/work/queue"; +import { loadWorkerEnv, routineRunUrl } from "./env"; import { workerStatus } from "./status"; console.info(`OpenBot worker status: ${workerStatus().status}`); /* - * Refused up front, for the reason `fire-routines.ts` refuses up front: a loop that started anyway - * would open a run row for every routine it offers itself and collect a 401 on every dispatch, - * forever, with the only evidence a line in the server's audit trail. Said once, loudly, before the - * first tick, is the difference between a worker that failed to start and a deployment where - * routines quietly do nothing. - */ -const workerSharedSecret = process.env.WORKER_SHARED_SECRET; -if (!workerSharedSecret) { - throw new Error( - "WORKER_SHARED_SECRET is not set, so this worker cannot authenticate itself to /internal/routines/run and no routine could be fired.", - ); -} - -/* - * Read from the environment rather than from `DeploymentConfig`/`loadConfig`, and deliberately so. + * The worker's three settings, parsed and validated in one place (`./env`). * - * `loadConfig` demands the whole server deployment's configuration — Intelligence credentials, key - * encryption, auth — because it answers "what can this deployment do". This process is handed exactly - * three settings by `scripts/start.sh` (`DATABASE_URL`, `SERVER_INTERNAL_URL`, - * `WORKER_SHARED_SECRET`); calling `loadConfig(process.env)` here would refuse to start over - * settings this loop has no opinion about and does not need. Where this process can reach its own API - * server is a fact about where this process runs, same as `fire-routines.ts` argues for - * `SERVER_INTERNAL_URL` alone — this file extends that reasoning to the secret and the database too. - */ -const serverInternalUrl = process.env.SERVER_INTERNAL_URL; -if (!serverInternalUrl) { - throw new Error( - "SERVER_INTERNAL_URL is not set, so this worker does not know where to hand a routine run.", - ); -} - -/* - * Refused for the same reason as the two checks above: a loop that started anyway would hand - * `createDatabase` an empty connection string and fail on the first query with no indication of - * what was actually missing. + * Refused up front, for the reason `fire-routines.ts` refuses up front: a loop that + * started anyway would open a run row for every routine it offers itself and collect + * a 401 on every dispatch, forever, with the only evidence a line in the server's + * audit trail. Said once, loudly, before the first tick, is the difference between a + * worker that failed to start and a deployment where routines quietly do nothing. + * + * Read from the environment rather than from `DeploymentConfig`/`loadConfig`, and + * deliberately so. `loadConfig` demands the whole server deployment's configuration — + * Intelligence credentials, key encryption, auth — because it answers "what can this + * deployment do". This process is handed exactly three settings by `scripts/start.sh` + * (`DATABASE_URL`, `SERVER_INTERNAL_URL`, `WORKER_SHARED_SECRET`); calling + * `loadConfig(process.env)` here would refuse to start over settings this loop has no + * opinion about and does not need. */ -const databaseUrl = process.env.DATABASE_URL; -if (!databaseUrl) { - throw new Error( - "DATABASE_URL is not set, so this worker has no database to read routines from or claim them in.", - ); -} +const { workerSharedSecret, serverInternalUrl, databaseUrl, owner } = + loadWorkerEnv(); const database = createDatabase(databaseUrl); const queue = createWorkQueue(database); const routineStore = createRoutineStore(database); -// A name for the lease, so a stuck claim can be traced back to the process that took it. Mirrors -// `fire-routines.ts`: `HOSTNAME` is not set by bash, so without the random fallback every worker -// started by `scripts/start.sh` would share the owner "routines/laptop" and `ours()` could no -// longer tell one worker's lease apart from another's. -const owner = `routines/${process.env.HOSTNAME ?? randomUUID().slice(0, 8)}`; - /** * Hand one opened run to the server, which owns everything about running it. * @@ -94,7 +64,7 @@ const owner = `routines/${process.env.HOSTNAME ?? randomUUID().slice(0, 8)}`; * `last_error` needs. */ async function dispatch(routineRunId: string): Promise { - const response = await fetch(`${serverInternalUrl}/internal/routines/run`, { + const response = await fetch(routineRunUrl(serverInternalUrl), { method: "POST", headers: { authorization: `Bearer ${workerSharedSecret}`, diff --git a/worker/tests/env.test.ts b/worker/tests/env.test.ts new file mode 100644 index 000000000..95484069a --- /dev/null +++ b/worker/tests/env.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, test } from "bun:test"; +import { loadWorkerEnv, routineRunUrl } from "../src/env"; + +const base = () => ({ + WORKER_SHARED_SECRET: "secret", + SERVER_INTERNAL_URL: "http://server:3001", + DATABASE_URL: "postgres://localhost:5432/openbot", + HOSTNAME: "laptop", +}); + +describe("worker env", () => { + test("parses a complete environment", () => { + expect(loadWorkerEnv(base())).toEqual({ + workerSharedSecret: "secret", + serverInternalUrl: "http://server:3001", + databaseUrl: "postgres://localhost:5432/openbot", + owner: "routines/laptop", + }); + }); + + test.each(["WORKER_SHARED_SECRET", "SERVER_INTERNAL_URL", "DATABASE_URL"])( + "refuses an unset %s", + (name) => { + const env = base(); + delete env[name as keyof typeof env]; + expect(() => loadWorkerEnv(env)).toThrow("is not set"); + }, + ); + + test.each(["WORKER_SHARED_SECRET", "SERVER_INTERNAL_URL", "DATABASE_URL"])( + "refuses a whitespace-only %s like an unset one", + (name) => { + expect(() => loadWorkerEnv({ ...base(), [name]: " " })).toThrow( + "is not set", + ); + }, + ); + + test("trims padded values", () => { + const env = loadWorkerEnv({ + ...base(), + WORKER_SHARED_SECRET: " secret ", + DATABASE_URL: " postgres://localhost:5432/openbot ", + }); + expect(env.workerSharedSecret).toBe("secret"); + expect(env.databaseUrl).toBe("postgres://localhost:5432/openbot"); + }); + + test.each([ + ["http://server:3001/", "http://server:3001"], + ["http://server:3001///", "http://server:3001"], + ])("strips trailing slashes from %p", (raw, normalised) => { + expect( + loadWorkerEnv({ ...base(), SERVER_INTERNAL_URL: raw }).serverInternalUrl, + ).toBe(normalised); + }); + + test("refuses a URL that is only slashes", () => { + expect(() => + loadWorkerEnv({ ...base(), SERVER_INTERNAL_URL: "///" }), + ).toThrow("is not set"); + }); + + test("falls back to a generated id without a hostname", () => { + const without = base(); + delete without.HOSTNAME; + expect(loadWorkerEnv(without, () => "abc123").owner).toBe( + "routines/abc123", + ); + }); + + test.each(["", " "])( + "falls back to a generated id for HOSTNAME=%p", + (hostname) => { + expect( + loadWorkerEnv({ ...base(), HOSTNAME: hostname }, () => "abc123").owner, + ).toBe("routines/abc123"); + }, + ); + + test("trims the hostname", () => { + expect(loadWorkerEnv({ ...base(), HOSTNAME: " laptop " }).owner).toBe( + "routines/laptop", + ); + }); +}); + +describe("routineRunUrl", () => { + test("joins the run path onto the base URL", () => { + expect(routineRunUrl("http://server:3001")).toBe( + "http://server:3001/internal/routines/run", + ); + }); + + test("a trailing-slash base normalises to a single-slash run URL", () => { + const env = loadWorkerEnv({ + ...base(), + SERVER_INTERNAL_URL: "http://server:3001/", + }); + expect(routineRunUrl(env.serverInternalUrl)).toBe( + "http://server:3001/internal/routines/run", + ); + }); +});