From cda2c5a22153d40b04bb99660b4c42ecbee57361 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 21 Aug 2026 14:28:37 -0500 Subject: [PATCH 1/2] Do not start a container this supervisor does not own `ensure` treats a 409 from `createContainer` as the other request creating the same computer, and goes on to start the container by name. That is right when the container already there is ours and wrong the rest of the time: a container left by a deployment under a different namespace, made by hand, or put there by somebody who guessed the name gets started and handed back as this Bot's computer, and the server then sends the deployment's computer token to whatever is listening inside it. The file's own rule says otherwise. Ownership is checked in `inspectOwned` so that stop, reset and inspect treat a name that is not ours as absent, and this was the one path that adopted it instead. Now a 409 is followed by the same ownership check, and a name held by something else is refused with its own error rather than a Docker outage, since the daemon is answering and the thing an operator has to look at is the container. Reproduced against a real daemon before and after: a container named `openbot-computer-` carrying somebody else's label was started by `ensure` and returned as the computer; it is now refused and left alone, and a container this supervisor does own is still started. The second half is the readiness wait, which could not fail. `waitUntilAnswering` fell out of its loop at the deadline and returned, so a computer that never came up was reported ready, which is the exact outcome the function exists to prevent. It throws now, the timeout is configurable because it decides when a slow start becomes an error, and 503 still tells the caller to wait and ask again. The tests drive a real Docker socket and skip where there is not one. A stub cannot show any of this: the behaviour under test is the daemon's 409, not ours. --- supervisor/src/docker.ts | 74 ++++++++++++- supervisor/src/index.ts | 14 ++- supervisor/tests/docker.integration.test.ts | 109 ++++++++++++++++++++ 3 files changed, 192 insertions(+), 5 deletions(-) create mode 100644 supervisor/tests/docker.integration.test.ts diff --git a/supervisor/src/docker.ts b/supervisor/src/docker.ts index 7b3d551f..28f5b609 100644 --- a/supervisor/src/docker.ts +++ b/supervisor/src/docker.ts @@ -57,6 +57,38 @@ export type ComputerState = { url?: string; }; +/** + * The name this Bot's computer would have is held by a container this supervisor does not own. + * + * Separate from {@link DockerUnavailableError} because the daemon is fine and an operator sent + * looking at it would find nothing. What has to happen is that somebody looks at the container + * holding the name and decides whether it should be there. + */ +export class NameHeldError extends Error { + constructor(container: string) { + super( + `A container named ${container} already exists and does not belong to this deployment. Remove it or rename it; it will not be adopted.`, + ); + this.name = "NameHeldError"; + } +} + +/** + * The container started and the computer inside it never answered. + * + * Its own class because the two failures need different actions. Docker being unreachable is the + * supervisor's problem; this is the computer's, and the message has to say so or an operator reads + * "could not reach Docker" about a daemon that is answering. + */ +export class ComputerNotAnsweringError extends Error { + constructor(container: string, timeoutMs: number) { + super( + `The computer in ${container} started but did not answer within ${timeoutMs}ms. It is not ready, so it is not being handed out.`, + ); + this.name = "ComputerNotAnsweringError"; + } +} + export class DockerUnavailableError extends Error { constructor(cause: string) { super( @@ -146,6 +178,9 @@ async function inspectOwned( } } +/** Long enough for a cold start with a large image, short enough that a caller is not left hanging. */ +const DEFAULT_READY_TIMEOUT_MS = 60_000; + /** * Wait until the computer actually answers. * @@ -163,7 +198,7 @@ async function inspectOwned( */ async function waitUntilAnswering( container: string, - timeoutMs = 60_000, + timeoutMs: number = DEFAULT_READY_TIMEOUT_MS, ): Promise { const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { @@ -182,6 +217,16 @@ async function waitUntilAnswering( } await new Promise((resolve) => setTimeout(resolve, 250)); } + + /* + * The deadline is a failure, not an answer. + * + * Returning here reported every computer that never came up as ready, which is the exact thing + * this function exists to prevent: the caller is handed an address, sends the deployment's + * computer token to it, and gets a transport error it cannot tell from a computer that is broken + * in some other way. A wait that cannot fail is a sleep. + */ + throw new ComputerNotAnsweringError(container, timeoutMs); } export type EnsureOptions = { @@ -198,6 +243,14 @@ export type EnsureOptions = { */ runtime?: string; memoryBytes?: number; + /** + * How long a started computer is given to answer before the attempt is called a failure. + * + * Configurable because the wait now fails rather than returning, so the number decides when a slow + * start becomes an error, and a deployment pulling a large image on a cold host is not the same as + * a test that wants an answer in seconds. + */ + readyTimeoutMs?: number; pidsLimit?: number; /** * The volume holding the SPIRE agent's Workload API socket, mounted read-only into each computer @@ -298,11 +351,24 @@ export async function ensure( HostConfig: hostConfig(names, options), }); } catch (error) { - // The other request creating the same computer got there first, which is what idempotent - // means here. Its container is the one this request goes on to start. if (statusOf(error) !== 409) { throw new DockerUnavailableError(String(error)); } + /* + * Something already holds the name, and 409 does not say what. + * + * Usually it is the other request creating the same computer, which is what idempotent means + * here, and its container is the one this request goes on to start. The other case is a + * container this supervisor does not own: left by a deployment that used a different + * namespace, made by hand, or put there by somebody who guessed the name. Ownership is + * checked everywhere else precisely so that one is treated as absent, and starting it here + * on a 409 was the one path that adopted it instead: `start` names the container, not the + * container this supervisor made, and the address goes back to a server that then sends the + * deployment's computer token to whatever is listening inside it. + */ + if (!(await inspectOwned(names))) { + throw new NameHeldError(names.container); + } } } @@ -323,7 +389,7 @@ export async function ensure( } const settled = await inspectOwned(names); - await waitUntilAnswering(names.container); + await waitUntilAnswering(names.container, options.readyTimeoutMs); return { botId: names.botId, diff --git a/supervisor/src/index.ts b/supervisor/src/index.ts index 78575a09..779d19b6 100644 --- a/supervisor/src/index.ts +++ b/supervisor/src/index.ts @@ -1,9 +1,11 @@ import { serve } from "bun"; import { Hono } from "hono"; import { + ComputerNotAnsweringError, DockerUnavailableError, ensure, listOwned, + NameHeldError, reachable, reset, stop, @@ -127,7 +129,17 @@ app.post("/computers/:botId/ensure", async (context) => { : { identity: identity.reason }), }); } catch (error) { - if (error instanceof DockerUnavailableError) { + // A held name is not an outage. 409 says the conflict is with something already there, so an + // operator reads the message rather than going to look at a daemon that is working. + if (error instanceof NameHeldError) { + return context.json({ error: error.message }, 409); + } + // Not ready is a 503 like an outage is, because the caller's next move is the same: wait and + // ask again. The message is what differs, and it is the part an operator acts on. + if ( + error instanceof DockerUnavailableError || + error instanceof ComputerNotAnsweringError + ) { return context.json({ error: error.message }, 503); } throw error; diff --git a/supervisor/tests/docker.integration.test.ts b/supervisor/tests/docker.integration.test.ts new file mode 100644 index 00000000..140e8f44 --- /dev/null +++ b/supervisor/tests/docker.integration.test.ts @@ -0,0 +1,109 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import Docker from "dockerode"; +import { + ComputerNotAnsweringError, + ensure, + NameHeldError, + reachable, +} from "../src/docker"; +import { namesFor } from "../src/names"; + +/* + * The ownership rule, against a real daemon. + * + * A fake Docker cannot show what this is about. The question is what the daemon does when two + * things want one name, and the answer, a 409 from `createContainer`, is the daemon's own behaviour + * rather than something a stub would be asserting about itself. + * + * Skipped where the socket is not reachable, which is most machines and most CI runs. That makes + * this a test that has to be asked for; it is here so the rule can be re-checked rather than + * argued about. + */ +const docker = new Docker( + process.env.DOCKER_SOCKET + ? { socketPath: process.env.DOCKER_SOCKET } + : undefined, +); +const available = await reachable(); +const IMAGE = process.env.SUPERVISOR_TEST_IMAGE ?? "debian:bookworm-slim"; + +const BOT = "supervisortestbot"; +const result = namesFor(BOT); +if (!result.ok) throw new Error(result.reason); +const names = result.names; + +async function remove(container: string) { + await docker + .getContainer(container) + .remove({ force: true }) + .catch(() => undefined); +} + +async function plant(labels: Record, health?: boolean) { + await remove(names.container); + await docker.createContainer({ + name: names.container, + Image: IMAGE, + Labels: labels, + Cmd: ["sleep", "600"], + ...(health + ? {} + : { + Healthcheck: { + Test: ["CMD-SHELL", "exit 1"], + Interval: 1_000_000_000, + Retries: 1, + StartPeriod: 0, + }, + }), + }); +} + +const OURS = { + "openbot.supervisor": "true", + "openbot.namespace": "openbot", + "openbot.bot-id": BOT, +}; + +afterEach(async () => { + await remove(names.container); +}); + +describe.skipIf(!available)("a name held by somebody else", () => { + test("is refused rather than adopted, and never started", async () => { + // The container this supervisor did not make. Ownership is checked everywhere else so that a + // name collision reads as absent; starting it on a 409 was the path that adopted it instead, + // and an adopted container receives the deployment's computer token. + await plant({ "someone.else": "true" }); + + await expect( + ensure(names, { image: IMAGE, environment: [] }), + ).rejects.toBeInstanceOf(NameHeldError); + + const info = await docker.getContainer(names.container).inspect(); + expect(info.State?.Running).toBe(false); + }, 90_000); + + test("but a container this supervisor owns is still started", async () => { + // The other direction, and the reason the check is ownership rather than existence: `ensure` is + // idempotent, so the container a previous call left stopped has to come back up. + await plant(OURS, true); + + const state = await ensure(names, { image: IMAGE, environment: [] }); + + expect(state.status).toBe("running"); + }, 90_000); +}); + +describe.skipIf(!available)("a computer that never answers", () => { + test("fails instead of being handed out as ready", async () => { + // A wait that cannot fail is a sleep: every computer that never came up was reported ready, and + // the caller learned otherwise by sending it the deployment's token and getting a transport + // error back. + await plant(OURS); + + await expect( + ensure(names, { image: IMAGE, environment: [], readyTimeoutMs: 3_000 }), + ).rejects.toBeInstanceOf(ComputerNotAnsweringError); + }, 90_000); +}); From 28f7d740713e57ba7d5170a060f7807be49859ea Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:09:38 -0500 Subject: [PATCH 2/2] Resolve the Docker client when the test runs, not when the file loads `supervisor` is not one of the root workspaces, so a root `bun install` never installs `dockerode`, and `bun test` from the root walks this directory anyway. Importing the client at module scope, or `../src/docker` which holds one, therefore failed to resolve in CI and took the whole file down instead of skipping it: the suite reported an unhandled error rather than a test result. It passed locally because a `bun install` inside `supervisor/` had already put the package there, which is the case `tests/clean-checkout.test.ts` was written about. That guard did not catch this one: it filters the declared dependencies to the single name that broke before, so a new one is invisible to it. Both imports now happen inside the function that decides whether the tests can run at all, alongside the socket check that was already there. Three reasons to skip rather than fail, none of them a property of the code under test: no socket on this machine, no client installed, or a socket that will not answer. Verified both ways, because a test file that skips everywhere would have made CI green by removing the coverage. With the root install alone the three tests skip and nothing errors. With the package present and a reachable daemon they run and pass, and removing the ownership check from `ensure` still turns the foreign-container one red. --- supervisor/tests/docker.integration.test.ts | 109 ++++++++++++++------ 1 file changed, 79 insertions(+), 30 deletions(-) diff --git a/supervisor/tests/docker.integration.test.ts b/supervisor/tests/docker.integration.test.ts index 140e8f44..7a0c4708 100644 --- a/supervisor/tests/docker.integration.test.ts +++ b/supervisor/tests/docker.integration.test.ts @@ -1,30 +1,70 @@ import { afterEach, describe, expect, test } from "bun:test"; -import Docker from "dockerode"; -import { - ComputerNotAnsweringError, - ensure, - NameHeldError, - reachable, -} from "../src/docker"; +import { existsSync } from "node:fs"; import { namesFor } from "../src/names"; /* * The ownership rule, against a real daemon. * - * A fake Docker cannot show what this is about. The question is what the daemon does when two - * things want one name, and the answer, a 409 from `createContainer`, is the daemon's own behaviour - * rather than something a stub would be asserting about itself. + * A fake Docker cannot show what this is about. The question is what the daemon does when two things + * want one name, and the answer, a 409 from `createContainer`, is the daemon's own behaviour rather + * than something a stub would be asserting about itself. * - * Skipped where the socket is not reachable, which is most machines and most CI runs. That makes - * this a test that has to be asked for; it is here so the rule can be re-checked rather than - * argued about. + * NOTHING HERE IS IMPORTED AT MODULE SCOPE except `namesFor`, which has no dependencies of its own. + * `supervisor` is not one of the root workspaces, so a root `bun install` never installs `dockerode`, + * and `bun test` from the root walks this directory anyway. A static import of the client, or of + * `../src/docker` which holds one, fails to resolve there and takes the whole file down with it + * rather than skipping. So the client is resolved when a test is about to use it, and its absence is + * one of the reasons to skip, alongside there being no socket to talk to. */ -const docker = new Docker( - process.env.DOCKER_SOCKET - ? { socketPath: process.env.DOCKER_SOCKET } - : undefined, -); -const available = await reachable(); + +const SOCKET = process.env.DOCKER_SOCKET ?? "/var/run/docker.sock"; + +type DockerRuntime = { + docker: InstanceType; + supervisor: typeof import("../src/docker"); +}; + +/** + * The daemon and the module under test, or nothing. + * + * Nothing on any of the three reasons this cannot run: no socket on this machine, no client package + * installed because the root is the only thing that ran `bun install`, or a socket that will not + * answer. Each is a reason to skip rather than to fail, and none of them is a property of the code + * being tested. + */ +async function dockerRuntime(): Promise { + if (!existsSync(SOCKET)) return null; + try { + const supervisor = await import("../src/docker"); + if (!(await supervisor.reachable())) return null; + const { default: Docker } = await import("dockerode"); + return { + docker: new Docker( + process.env.DOCKER_SOCKET ? { socketPath: SOCKET } : undefined, + ), + supervisor, + }; + } catch { + return null; + } +} + +const runtime = await dockerRuntime(); + +/** + * The runtime, for a test that is only reached when there is one. + * + * Every use sits inside a `describe.skipIf(runtime === null)`, so the throw is unreachable. It is + * here to say that in the types rather than with an assertion at each of the eight call sites. + */ +function withDocker(): DockerRuntime { + if (runtime === null) { + throw new Error("This test runs only when a Docker daemon is reachable."); + } + return runtime; +} + +/** Any small image that stays up when told to sleep. Nothing here tests what is inside it. */ const IMAGE = process.env.SUPERVISOR_TEST_IMAGE ?? "debian:bookworm-slim"; const BOT = "supervisortestbot"; @@ -33,15 +73,15 @@ if (!result.ok) throw new Error(result.reason); const names = result.names; async function remove(container: string) { - await docker - .getContainer(container) + await withDocker() + .docker.getContainer(container) .remove({ force: true }) .catch(() => undefined); } async function plant(labels: Record, health?: boolean) { await remove(names.container); - await docker.createContainer({ + await withDocker().docker.createContainer({ name: names.container, Image: IMAGE, Labels: labels, @@ -69,7 +109,7 @@ afterEach(async () => { await remove(names.container); }); -describe.skipIf(!available)("a name held by somebody else", () => { +describe.skipIf(runtime === null)("a name held by somebody else", () => { test("is refused rather than adopted, and never started", async () => { // The container this supervisor did not make. Ownership is checked everywhere else so that a // name collision reads as absent; starting it on a 409 was the path that adopted it instead, @@ -77,10 +117,12 @@ describe.skipIf(!available)("a name held by somebody else", () => { await plant({ "someone.else": "true" }); await expect( - ensure(names, { image: IMAGE, environment: [] }), - ).rejects.toBeInstanceOf(NameHeldError); + withDocker().supervisor.ensure(names, { image: IMAGE, environment: [] }), + ).rejects.toBeInstanceOf(withDocker().supervisor.NameHeldError); - const info = await docker.getContainer(names.container).inspect(); + const info = await withDocker() + .docker.getContainer(names.container) + .inspect(); expect(info.State?.Running).toBe(false); }, 90_000); @@ -89,13 +131,16 @@ describe.skipIf(!available)("a name held by somebody else", () => { // idempotent, so the container a previous call left stopped has to come back up. await plant(OURS, true); - const state = await ensure(names, { image: IMAGE, environment: [] }); + const state = await withDocker().supervisor.ensure(names, { + image: IMAGE, + environment: [], + }); expect(state.status).toBe("running"); }, 90_000); }); -describe.skipIf(!available)("a computer that never answers", () => { +describe.skipIf(runtime === null)("a computer that never answers", () => { test("fails instead of being handed out as ready", async () => { // A wait that cannot fail is a sleep: every computer that never came up was reported ready, and // the caller learned otherwise by sending it the deployment's token and getting a transport @@ -103,7 +148,11 @@ describe.skipIf(!available)("a computer that never answers", () => { await plant(OURS); await expect( - ensure(names, { image: IMAGE, environment: [], readyTimeoutMs: 3_000 }), - ).rejects.toBeInstanceOf(ComputerNotAnsweringError); + withDocker().supervisor.ensure(names, { + image: IMAGE, + environment: [], + readyTimeoutMs: 3_000, + }), + ).rejects.toBeInstanceOf(withDocker().supervisor.ComputerNotAnsweringError); }, 90_000); });