feat(worker): validate env up front and normalise server URL

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<string, string | undefined> = 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`;
+}

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<void> {
-  const response = await fetch(`${serverInternalUrl}/internal/routines/run`, {
+  const response = await fetch(routineRunUrl(serverInternalUrl), {
     method: "POST",
     headers: {
       authorization: `Bearer ${workerSharedSecret}`,

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",
+    );
+  });
+});