Do not start a container this supervisor does not own

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<void> {
   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,

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;

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<typeof import("dockerode").default>;
+  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<DockerRuntime | null> {
+  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<string, string>, 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);
+});