From b0b7820d497f614dfffd3589c5a9ca47e06d0f7f Mon Sep 17 00:00:00 2001 From: David McKay Date: Fri, 21 Aug 2026 21:57:01 -0700 Subject: [PATCH] Make an upgrade reach a Bot's computer A computer is a container the supervisor makes, and `ensure` reused any container with the right name whatever image 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 compose did not make them, so even a full teardown left them behind. That is worse than stale code. agent-computer is the browser, the workspace and the confinement around both, so a fix to any of them silently did not apply to a Bot that already had a computer. Found by rebuilding every image, tearing the stack down, starting it back up, and watching a Bot answer with in-memory state from an hour earlier: a handover prompt about a page from a previous conversation, offered on a new one. The prompt was the symptom; the container was four hours old on an image two builds behind. A computer built from a different image is now replaced on next use. Compared by resolved id rather than by tag, because both sides are the same tag and the question is whether it has moved. An image that cannot be inspected counts as current: destroying a working browser over a failed inspect is a worse answer than running a version behind. Replacing is safe, and that is what makes it automatic. The profile and the workspace are named volumes and are not removed, 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. That is the difference between this and reset, which is asked for deliberately and does take the profile. What is lost is what the old computer held in memory, which belongs to a run the upgrade has already ended. start.sh now also says that stopping the compose services leaves the computers running, and how to stop them. Not closed: drift in the container's configuration rather than its image. An operator tightening the confinement, a pids limit or a gVisor runtime, still does not reach a computer that already exists. --- CHANGELOG.md | 9 ++ scripts/start.sh | 3 + supervisor/src/docker.ts | 69 ++++++++++- supervisor/tests/docker.integration.test.ts | 127 ++++++++++++++++++++ 4 files changed, 206 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cc5bb43..b470f283 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/scripts/start.sh b/scripts/start.sh index d7db5814..20536095 100755 --- a/scripts/start.sh +++ b/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 diff --git a/supervisor/src/docker.ts b/supervisor/src/docker.ts index 715a2aee..048068d1 100644 --- a/supervisor/src/docker.ts +++ b/supervisor/src/docker.ts @@ -169,7 +169,7 @@ export async function listOwned(): Promise { */ 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,6 +178,9 @@ 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; @@ -185,6 +188,41 @@ async function inspectOwned( } } +/** + * 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 { + 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 { 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]) { diff --git a/supervisor/tests/docker.integration.test.ts b/supervisor/tests/docker.integration.test.ts index 7a0c4708..57653c1e 100644 --- a/supervisor/tests/docker.integration.test.ts +++ b/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 { + 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); + }, +);