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..7a0c4708 --- /dev/null +++ b/supervisor/tests/docker.integration.test.ts @@ -0,0 +1,158 @@ +import { afterEach, describe, expect, test } from "bun:test"; +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. + * + * 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 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"; +const result = namesFor(BOT); +if (!result.ok) throw new Error(result.reason); +const names = result.names; + +async function remove(container: string) { + await withDocker() + .docker.getContainer(container) + .remove({ force: true }) + .catch(() => undefined); +} + +async function plant(labels: Record, health?: boolean) { + await remove(names.container); + await withDocker().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(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, + // and an adopted container receives the deployment's computer token. + await plant({ "someone.else": "true" }); + + await expect( + withDocker().supervisor.ensure(names, { image: IMAGE, environment: [] }), + ).rejects.toBeInstanceOf(withDocker().supervisor.NameHeldError); + + const info = await withDocker() + .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 withDocker().supervisor.ensure(names, { + image: IMAGE, + environment: [], + }); + + expect(state.status).toBe("running"); + }, 90_000); +}); + +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 + // error back. + await plant(OURS); + + await expect( + withDocker().supervisor.ensure(names, { + image: IMAGE, + environment: [], + readyTimeoutMs: 3_000, + }), + ).rejects.toBeInstanceOf(withDocker().supervisor.ComputerNotAnsweringError); + }, 90_000); +});