Make an upgrade reach a Bot's computer

CHANGELOG.md

@@ -201,6 +201,15 @@ Sessions survive and nobody signs in again.
   is unavailable never blocks a sign-in.
 
 ### Fixed
+- **Upgrading never reached a Bot's computer.** A computer is a container the supervisor makes, and it
+  was reused by name whatever image it was built from, so once a Bot had one, rebuilding the image
+  moved the tag and the container went on running the old one indefinitely with nothing to say so.
+  `docker compose down` does not touch these either, because compose did not make them, so even a
+  full teardown left them behind. That is worse than stale code: the computer is the browser, the
+  workspace and the confinement around both, so a fix to any of them silently did not apply. A
+  computer built from a different image is now replaced on next use. Its profile and its workspace
+  are volumes and are kept, so a Bot comes back on the new image still signed in to what it was
+  signed in to, with its files where it left them.
 - **The audit trail could be erased with one statement.** It is append-only because a database
   trigger refuses updates and deletes, and that trigger is row-level, so `TRUNCATE` never reached it:
   anything holding `DATABASE_URL` could empty the table and nothing raised. That is the case the

scripts/start.sh

@@ -273,5 +273,8 @@ Try:
 
 Logs: $LOGS
 Stop Docker services: docker compose down
+  A Bot's computer is made by the supervisor rather than by compose, so it keeps running:
+  docker rm -f \$(docker ps -q --filter label=openbot.supervisor=true)
+  Its files and its browser profile are volumes and survive either way.
 Stop host app/server: kill the processes using ports $APP_PORT and $SERVER_PORT
 EOF

supervisor/src/docker.ts

@@ -169,7 +169,7 @@ export async function listOwned(): Promise<ComputerState[]> {
  */
 async function inspectOwned(
   names: ComputerNames,
-): Promise<{ status: string; port?: number } | null> {
+): Promise<{ status: string; port?: number; image?: string } | null> {
   try {
     const info = await docker.getContainer(names.container).inspect();
     if (!ours(info.Config?.Labels)) return null;
@@ -178,13 +178,51 @@ async function inspectOwned(
     return {
       status: info.State?.Status ?? "unknown",
       ...(published ? { port: Number.parseInt(published, 10) } : {}),
+      // The resolved image, not the tag it was started from. A tag moves when the image is
+      // rebuilt; this is what the container is actually running.
+      ...(info.Image ? { image: info.Image } : {}),
     };
   } catch (error) {
     if ((error as { statusCode?: number }).statusCode === 404) return null;
     throw new DockerUnavailableError(String(error));
   }
 }
 
+/**
+ * Whether the computer that exists is running the image this deployment now ships.
+ *
+ * `ensure` reused any container with the right name, whatever it was built from, so once a Bot had a
+ * computer, upgrading OpenBot never reached it. Rebuilding the image moves the tag; the container
+ * goes on running the old one, indefinitely, and nothing says so. Found by rebuilding every image,
+ * restarting the whole stack, and watching a Bot's computer answer with in-memory state from an hour
+ * earlier: `docker compose down` does not touch these, because the supervisor makes them rather than
+ * compose.
+ *
+ * That is worse than stale code. `agent-computer` is the browser, the workspace and the confinement,
+ * so a fix to any of them silently would not apply to a Bot that already had a computer.
+ *
+ * Compared by resolved id rather than by tag, because both sides are the same tag and the whole
+ * question is whether the tag has moved since.
+ *
+ * Unanswerable is not stale. If the image cannot be inspected — never pulled, a registry that cannot
+ * be reached, a daemon that will not say — this reports true and the existing computer is kept.
+ * Destroying a Bot's working browser over a failed inspect is a worse answer than running an image
+ * that may be a version behind.
+ */
+async function runsCurrentImage(
+  existingImage: string | undefined,
+  image: string,
+): Promise<boolean> {
+  if (!existingImage) return true;
+  try {
+    const current = await docker.getImage(image).inspect();
+    const id = current?.Id;
+    return typeof id === "string" && id ? id === existingImage : true;
+  } catch {
+    return true;
+  }
+}
+
 /** 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;
 
@@ -329,7 +367,34 @@ export async function ensure(
   options: EnsureOptions,
 ): Promise<ComputerState> {
   for (let attempt = ATTEMPTS; attempt > 0; attempt--) {
-    const existing = await inspectOwned(names);
+    let existing = await inspectOwned(names);
+
+    /*
+     * An upgrade reaches a computer that already exists, by replacing it.
+     *
+     * Safe to do: the profile and the workspace are named volumes and are not removed here, so the
+     * Bot keeps its logins and its files and comes back on the new image. That is the difference
+     * between this and `reset`, which is asked for deliberately and does take the profile.
+     *
+     * What is lost is whatever the old computer held in memory: an open page and an outstanding
+     * request for a person to take the wheel. Both belong to a run that the upgrade has already
+     * ended, and a Bot carrying an hour-old handover prompt into a new conversation is the symptom
+     * that found this.
+     */
+    if (existing && !(await runsCurrentImage(existing.image, options.image))) {
+      try {
+        await docker
+          .getContainer(names.container)
+          .remove({ force: true, v: false });
+      } catch (error) {
+        // Already gone is the outcome this wanted. Anything else and the computer stays as it is,
+        // which is the same answer this function gave before it could replace one at all.
+        if (statusOf(error) !== 404) {
+          throw new DockerUnavailableError(String(error));
+        }
+      }
+      existing = null;
+    }
 
     if (!existing) {
       for (const volume of [names.profileVolume, names.workspaceVolume]) {

supervisor/tests/docker.integration.test.ts

@@ -156,3 +156,130 @@ describe.skipIf(runtime === null)("a computer that never answers", () => {
     ).rejects.toBeInstanceOf(withDocker().supervisor.ComputerNotAnsweringError);
   }, 90_000);
 });
+
+describe.skipIf(runtime === null)(
+  "a computer built from an older image",
+  () => {
+    /*
+     * The upgrade that never reached the computers.
+     *
+     * `ensure` reused any container with the right name whatever it was built from, so once a Bot had
+     * a computer, rebuilding the image moved the tag and the container went on running the old one
+     * indefinitely, with nothing to say so. `docker compose down` does not touch these either, because
+     * the supervisor makes them rather than compose, so even a full teardown left them behind.
+     *
+     * Found by rebuilding every image, restarting the whole stack, and watching a Bot's computer
+     * answer with in-memory state from an hour before: a handover prompt about a page from a previous
+     * conversation, offered on a new one.
+     *
+     * Two different images rather than a rebuild of one, because what the code compares is the
+     * resolved id on either side and two tags is the cheapest way to have two of those.
+     */
+    const OTHER = process.env.SUPERVISOR_TEST_OTHER_IMAGE ?? "alpine:3";
+
+    async function pull(image: string): Promise<boolean> {
+      try {
+        await withDocker().docker.getImage(image).inspect();
+        return true;
+      } catch {
+        // Not present locally. Pulling in a test is a network call this suite otherwise never makes,
+        // so it is attempted once and its failure skips rather than fails.
+        try {
+          const stream = await withDocker().docker.pull(image);
+          await new Promise((resolve, reject) => {
+            withDocker().docker.modem.followProgress(
+              stream as never,
+              (error: unknown) => (error ? reject(error) : resolve(null)),
+            );
+          });
+          return true;
+        } catch {
+          return false;
+        }
+      }
+    }
+
+    test("is replaced, and keeps its profile and workspace", async () => {
+      if (!(await pull(OTHER))) return;
+
+      // A computer this supervisor owns, made the way it makes them, on the wrong image.
+      await withDocker().supervisor.ensure(names, {
+        image: OTHER,
+        environment: [],
+      });
+      const before = await withDocker()
+        .docker.getContainer(names.container)
+        .inspect();
+
+      // Something in the volumes, so "kept" is a fact about their contents and not only their names.
+      // Volumes outlive the container by not being removed with it; that is what makes replacing one
+      // safe, and it is the whole reason this fix is allowed to be automatic.
+      const volumes = await Promise.all(
+        [names.profileVolume, names.workspaceVolume].map((volume) =>
+          withDocker().docker.getVolume(volume).inspect(),
+        ),
+      );
+
+      const state = await withDocker().supervisor.ensure(names, {
+        image: IMAGE,
+        environment: [],
+      });
+      const after = await withDocker()
+        .docker.getContainer(names.container)
+        .inspect();
+
+      expect(state).not.toBeNull();
+      // A different container, on the image asked for.
+      expect(after.Id).not.toBe(before.Id);
+      expect(after.Image).not.toBe(before.Image);
+
+      const wanted = await withDocker().docker.getImage(IMAGE).inspect();
+      expect(after.Image).toBe(wanted.Id);
+
+      // The same volumes, not replacements: a Bot keeps its logins and its files across an upgrade.
+      const kept = await Promise.all(
+        [names.profileVolume, names.workspaceVolume].map((volume) =>
+          withDocker().docker.getVolume(volume).inspect(),
+        ),
+      );
+      expect(kept.map((v) => v.CreatedAt)).toEqual(
+        volumes.map((v) => v.CreatedAt),
+      );
+    }, 180_000);
+
+    test("is left alone when it is already the image asked for", async () => {
+      /*
+       * The other half, and the one that keeps this from being a fix that restarts every computer on
+       * every request. `ensure` is called whenever a computer is needed, so a comparison that ever
+       * reported stale for a current container would throw away a Bot's browser mid-task.
+       */
+      const first = await withDocker().supervisor.ensure(names, {
+        image: IMAGE,
+        environment: [],
+      });
+      const before = await withDocker()
+        .docker.getContainer(names.container)
+        .inspect();
+
+      const second = await withDocker().supervisor.ensure(names, {
+        image: IMAGE,
+        environment: [],
+      });
+      const after = await withDocker()
+        .docker.getContainer(names.container)
+        .inspect();
+
+      /*
+       * Identity, not liveness. The placeholder image has no long-running command, so the container
+       * exits and Docker restarts it; its status and its start time at any instant are facts about
+       * that image rather than about `ensure`. What matters here is that the same container is
+       * still there: a replacement would have a different id, and `ensure` is called for every
+       * request, so a comparison that ever reported stale for a current container would throw away
+       * a Bot's browser mid-task.
+       */
+      expect(first).not.toBeNull();
+      expect(second).not.toBeNull();
+      expect(after.Id).toBe(before.Id);
+    }, 180_000);
+  },
+);