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); + }, +);