Give every process its own lease name, so two on one machine cannot both hold an item

CHANGELOG.md

@@ -18,6 +18,18 @@ and is safe to rerun. It kills a port holder only once that process has identifi
 OpenBot, so an unrelated process on 3010 is named and left alone rather than killed. Nothing is
 deleted: the database, the Bots' files and their browser profiles are volumes. `--keep-computers`
 leaves the browsers signed in.
+### Two workers on one machine can no longer fire the same routine twice
+
+Every process that claims work from the shared queue named itself after its hostname, and the queue
+tells two claimants apart by that name alone. Two processes on one machine therefore had the same
+name, so the lease meant nothing between them: one whose lease had lapsed was still told the item was
+its own, and both went on to dispatch it. A routine fired that way opens two runs and sends the same
+scheduled message twice. Nothing stops two workers running on one machine — the worker binds no port,
+and `scripts/start.sh` looks for a process it does not start the way `bun run dev` does. The name now
+always carries a random suffix, so a second process is a different claimant. It also keeps the
+hostname, so a stuck claim still traces back to the machine holding it, and a blank `HOSTNAME` is no
+longer read as a name — which had made every replica in a deployment share one.
+
 ### The desktop app writes its `.env` readable only by its owner
 
 The desktop `.env` holds `KEY_ENCRYPTION_KEY` and every minted token, and those are now long-lived:

server/scripts/cull-idle-computers.ts

@@ -10,7 +10,6 @@
  * reported and left for the next sweep, because a computer still running costs money rather than
  * losing anything, and a failing CronJob that pages somebody at 3am should mean something worse.
  */
-import { randomUUID } from "node:crypto";
 import { createComputerProvider } from "../src/computer/provider";
 import { loadConfig } from "../src/config";
 import { createDatabase } from "../src/db/client";
@@ -20,6 +19,7 @@ import {
   suspendClaimedComputers,
 } from "../src/work/culler";
 import { createWorkQueue } from "../src/work/queue";
+import { workOwner } from "../../shared/work-owner";
 
 const config = loadConfig(process.env);
 if (!config.computer) {
@@ -38,7 +38,7 @@ const queue = createWorkQueue(database);
 const provider = createComputerProvider(config.computer);
 
 // A name for the lease, so a stuck claim can be traced back to the pod that took it.
-const owner = `culler/${process.env.HOSTNAME ?? randomUUID().slice(0, 8)}`;
+const owner = workOwner("culler");
 
 try {
   const options = {

server/scripts/fire-routines.ts

@@ -12,7 +12,6 @@
  * routine that fires a minute late has lost nothing, and a failing CronJob that pages somebody at 3am
  * should mean something worse than that.
  */
-import { randomUUID } from "node:crypto";
 import { loadConfig } from "../src/config";
 import { createDatabase } from "../src/db/client";
 import { createRoutineStore } from "../src/routines/store";
@@ -22,6 +21,7 @@ import {
   ROUTINE_FIRE_KIND,
 } from "../src/routines/sweep";
 import { createWorkQueue } from "../src/work/queue";
+import { workOwner } from "../../shared/work-owner";
 
 const config = loadConfig(process.env);
 
@@ -60,7 +60,7 @@ const queue = createWorkQueue(database);
 const routineStore = createRoutineStore(database);
 
 // A name for the lease, so a stuck claim can be traced back to the pod that took it.
-const owner = `routines/${process.env.HOSTNAME ?? randomUUID().slice(0, 8)}`;
+const owner = workOwner("routines");
 
 /**
  * Hand one opened run to the server, which owns everything about running it.

server/src/index.ts

@@ -97,6 +97,7 @@ import {
   startWorkOfferedListener,
   type WorkOfferedListener,
 } from "./work/queue";
+import { workOwner } from "../../shared/work-owner";
 
 /**
  * Who is asking, for a CopilotKit request.
@@ -893,7 +894,7 @@ let workOfferedListener: WorkOfferedListener | undefined;
 if (config.handoff.maxDepth > 0 && config.handoff.maxPerRun > 0) {
   const runner = createHandoffRunner({
     queue: createWorkQueue(database),
-    owner: `handoff/${process.env.HOSTNAME ?? randomUUID().slice(0, 8)}`,
+    owner: workOwner("handoff"),
     auditStore: bootAuditStore,
     /*
      * The signed statement of the run the addressed Bot is about to start, carrying how deep the
@@ -1044,7 +1045,7 @@ if (config.handoff.maxDepth > 0 && config.handoff.maxPerRun > 0) {
  */
 const reaper = createHandoffRunner({
   queue: createWorkQueue(database),
-  owner: `reaper/${process.env.HOSTNAME ?? randomUUID().slice(0, 8)}`,
+  owner: workOwner("reaper"),
   sign: () => "",
   auditStore: bootAuditStore,
   // Never called: `reap` deletes rows by age and claims nothing.
@@ -1083,7 +1084,7 @@ const channelSummaries = {
     model: tenantPackage.model.defaultModel,
     resolveApiKey: resolveRuntimeModelApiKey,
   }),
-  owner: `summariser/${process.env.HOSTNAME ?? randomUUID().slice(0, 8)}`,
+  owner: workOwner("summariser"),
 };
 repeatAfterEach(async () => {
   try {

shared/work-owner.test.ts

@@ -0,0 +1,32 @@
+import { describe, expect, test } from "bun:test";
+import { workOwner } from "./work-owner";
+
+describe("who a process says it is when it takes a lease", () => {
+  test("two processes on one host never share an owner", () => {
+    const environment = { HOSTNAME: "laptop" };
+    expect(workOwner("routines", environment)).not.toBe(
+      workOwner("routines", environment),
+    );
+  });
+
+  test("a blank HOSTNAME is not a name, and never becomes the whole owner", () => {
+    for (const HOSTNAME of ["", "   ", undefined]) {
+      const owner = workOwner("handoff", { HOSTNAME });
+      expect(owner).not.toBe("handoff/");
+      expect(owner.startsWith("handoff/")).toBe(true);
+      expect(owner.length).toBeGreaterThan("handoff/".length);
+    }
+  });
+
+  test("keeps the host in the name, so a stuck claim still traces back to it", () => {
+    expect(workOwner("culler", { HOSTNAME: "pod-7" })).toMatch(
+      /^culler\/pod-7-[0-9a-f]{8}$/,
+    );
+  });
+
+  test("names the role it was asked for", () => {
+    expect(workOwner("summariser", { HOSTNAME: "h" })).toStartWith(
+      "summariser/",
+    );
+  });
+});

shared/work-owner.ts

@@ -0,0 +1,10 @@
+import { randomUUID } from "node:crypto";
+
+export function workOwner(
+  role: string,
+  environment: Record<string, string | undefined> = process.env,
+): string {
+  const host = environment.HOSTNAME?.trim();
+  const suffix = randomUUID().slice(0, 8);
+  return host ? `${role}/${host}-${suffix}` : `${role}/${suffix}`;
+}

worker/src/env.ts

@@ -1,14 +1,13 @@
-import { randomUUID } from "node:crypto";
+import { workOwner } from "../../shared/work-owner";
 
 /**
  * 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.
+ * answered 404 rather than 202". `owner` always carries a random suffix, so two
+ * workers on one host never share a lease name; see `shared/work-owner.ts`.
  */
 export type WorkerEnv = {
   workerSharedSecret: string;
@@ -27,7 +26,6 @@ export type WorkerEnv = {
  */
 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) {
@@ -66,8 +64,7 @@ export function loadWorkerEnv(
     );
   }
 
-  const host = environment.HOSTNAME?.trim();
-  const owner = `routines/${host || generateId()}`;
+  const owner = workOwner("routines", environment);
 
   return { workerSharedSecret, serverInternalUrl, databaseUrl, owner };
 }

worker/tests/env.test.ts

@@ -10,12 +10,13 @@ const base = () => ({
 
 describe("worker env", () => {
   test("parses a complete environment", () => {
-    expect(loadWorkerEnv(base())).toEqual({
+    const { owner, ...rest } = loadWorkerEnv(base());
+    expect(rest).toEqual({
       workerSharedSecret: "secret",
       serverInternalUrl: "http://server:3001",
       databaseUrl: "postgres://localhost:5432/openbot",
-      owner: "routines/laptop",
     });
+    expect(owner).toMatch(/^routines\/laptop-[0-9a-f]{8}$/);
   });
 
   test.each(["WORKER_SHARED_SECRET", "SERVER_INTERNAL_URL", "DATABASE_URL"])(
@@ -61,28 +62,27 @@ describe("worker env", () => {
     ).toThrow("is not set");
   });
 
-  test("falls back to a generated id without a hostname", () => {
+  test("names itself without a hostname, and never as the bare role", () => {
     const without = base();
     delete without.HOSTNAME;
-    expect(loadWorkerEnv(without, () => "abc123").owner).toBe(
-      "routines/abc123",
-    );
+    expect(loadWorkerEnv(without).owner).toMatch(/^routines\/[0-9a-f]{8}$/);
   });
 
-  test.each(["", "   "])(
-    "falls back to a generated id for HOSTNAME=%p",
-    (hostname) => {
-      expect(
-        loadWorkerEnv({ ...base(), HOSTNAME: hostname }, () => "abc123").owner,
-      ).toBe("routines/abc123");
-    },
-  );
+  test.each(["", "   "])("does not read HOSTNAME=%p as a name", (hostname) => {
+    const owner = loadWorkerEnv({ ...base(), HOSTNAME: hostname }).owner;
+    expect(owner).not.toBe("routines/");
+    expect(owner).toMatch(/^routines\/[0-9a-f]{8}$/);
+  });
 
   test("trims the hostname", () => {
-    expect(loadWorkerEnv({ ...base(), HOSTNAME: "  laptop  " }).owner).toBe(
-      "routines/laptop",
+    expect(loadWorkerEnv({ ...base(), HOSTNAME: "  laptop  " }).owner).toMatch(
+      /^routines\/laptop-[0-9a-f]{8}$/,
     );
   });
+
+  test("two workers on one host never share an owner", () => {
+    expect(loadWorkerEnv(base()).owner).not.toBe(loadWorkerEnv(base()).owner);
+  });
 });
 
 describe("routineRunUrl", () => {