From c89ba644df1b3562e8c8f54ea0b0b33f700a3c3c Mon Sep 17 00:00:00 2001 From: Vaibhav Zope Date: Wed, 9 Sep 2026 00:41:40 +0530 Subject: [PATCH] Give every process its own lease name, so two on one machine cannot both hold an item --- CHANGELOG.md | 12 ++++++++++ server/scripts/cull-idle-computers.ts | 4 ++-- server/scripts/fire-routines.ts | 4 ++-- server/src/index.ts | 7 +++--- shared/work-owner.test.ts | 32 +++++++++++++++++++++++++++ shared/work-owner.ts | 10 +++++++++ worker/src/env.ts | 11 ++++----- worker/tests/env.test.ts | 32 +++++++++++++-------------- 8 files changed, 82 insertions(+), 30 deletions(-) create mode 100644 shared/work-owner.test.ts create mode 100644 shared/work-owner.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index daf650457..75e5882ac 100644 --- a/CHANGELOG.md +++ b/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: diff --git a/server/scripts/cull-idle-computers.ts b/server/scripts/cull-idle-computers.ts index 43074a489..c5609481d 100644 --- a/server/scripts/cull-idle-computers.ts +++ b/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 = { diff --git a/server/scripts/fire-routines.ts b/server/scripts/fire-routines.ts index 0cd5c8c98..ee2545ed8 100644 --- a/server/scripts/fire-routines.ts +++ b/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. diff --git a/server/src/index.ts b/server/src/index.ts index 1218ebc0c..260091eb6 100644 --- a/server/src/index.ts +++ b/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 { diff --git a/shared/work-owner.test.ts b/shared/work-owner.test.ts new file mode 100644 index 000000000..c8aff4940 --- /dev/null +++ b/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/", + ); + }); +}); diff --git a/shared/work-owner.ts b/shared/work-owner.ts new file mode 100644 index 000000000..e55721e7f --- /dev/null +++ b/shared/work-owner.ts @@ -0,0 +1,10 @@ +import { randomUUID } from "node:crypto"; + +export function workOwner( + role: string, + environment: Record = process.env, +): string { + const host = environment.HOSTNAME?.trim(); + const suffix = randomUUID().slice(0, 8); + return host ? `${role}/${host}-${suffix}` : `${role}/${suffix}`; +} diff --git a/worker/src/env.ts b/worker/src/env.ts index 8b0919207..1aad6e143 100644 --- a/worker/src/env.ts +++ b/worker/src/env.ts @@ -1,4 +1,4 @@ -import { randomUUID } from "node:crypto"; +import { workOwner } from "../../shared/work-owner"; /** * What the worker needs from its environment, parsed and ready to use. @@ -6,9 +6,8 @@ import { randomUUID } from "node:crypto"; * `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 = 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 }; } diff --git a/worker/tests/env.test.ts b/worker/tests/env.test.ts index 95484069a..28abdc0d3 100644 --- a/worker/tests/env.test.ts +++ b/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", () => {