diff --git a/CHANGELOG.md b/CHANGELOG.md index 3400b3e7..4a133b0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,23 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A wiped or restarted shared computer no longer leaves refs pointing at the dead page + +Snapshots are ordered on the run of the browser that took them as well as the generation, so a +computer that is replaced cannot have its old page mistaken for its new one. That ordering was +reaching only the deployments that give each Bot its own container or sandbox, because the run was +read off the infrastructure and the deployment with one shared computer has none to read. + +Two failures followed there, and both are fixed. A snapshot still in flight when somebody pressed +Reset brought the wiped page back, and the boundary went on deciding about its elements: a rule about +"Confirm transfer" firing on a click nowhere near one, or failing to fire on one that is. And a +computer that restarted counted generations from one again, so its first snapshots were dropped as +stale and refs kept resolving against a page nobody was on until the counter climbed back past it. + +The computer now mints a run for each Bot's browser session, mints a new one when the Bot is reset, +and answers which run it is on. Nothing changes for a deployment that already reports one, and a +computer too old to answer leaves the ordering exactly where it was rather than refusing anything. + ### The framework Bot answers on 5.6-tier models, and can be told how hard to think Pointing `BOT_MODEL` at a `gpt-5.6-*` model gave a Bot that started, reported healthy, and then said diff --git a/agent-computer/src/index.ts b/agent-computer/src/index.ts index 5d7177a6..b268a1f7 100644 --- a/agent-computer/src/index.ts +++ b/agent-computer/src/index.ts @@ -9,20 +9,15 @@ import { } from "./authorisation"; import { isPlainBotId } from "./bot-id"; import { - type Control, ControlError, ControlRequestError, - createControl, NO_SECRET_PENDING, TAKE_CONTROL_FIRST, } from "./control"; import { identity } from "./identity"; import { createProfiles, numberFromEnv, VIEWPORT } from "./profiles"; -import { - type InputMessage, - type Screencast, - startScreencast, -} from "./screencast"; +import { type InputMessage, startScreencast } from "./screencast"; +import { type BotSession, createSessions } from "./sessions"; import { createShell } from "./shell"; import { isCurrentViewer } from "./viewer"; import { @@ -106,51 +101,12 @@ const TEXT_EXTRACT_LIMIT = 6000; * to resolve. Playwright's `aria-ref` engine is the runtime enforcement: it resolves a ref only against * the most recent snapshot, only while the element is still connected to the document, and it mints a * new ref if an element's role or accessible name changed, so a recycled node cannot inherit an old one. - */ -/** Per-Bot browser-control state. Profiles are isolated, but this process is not a security boundary. */ -type BotSession = { - control: Control; - /** This Bot's snapshot generation. See the note above on staleness. */ - snapshotId: number; - /** The one live screen viewer for this Bot, if a person is watching. */ - viewer?: { - socket: unknown; - cast: Screencast; - /** Stops the loop that keeps the cast pointed at whatever page the Bot is actually on. */ - follow?: ReturnType; - }; -}; - -const sessions = new Map(); - -/** - * Forget the sessions of Bots whose browsers are no longer running. - * - * The map gained an entry per Bot id this process had ever seen and lost none, so a deployment where - * every employee has a Bot accumulated one small object per employee for the life of the container. - * Small, but unbounded, which is the same shape as the browsers themselves. * - * Only entries with no live browser and nobody watching are dropped: the state is the generation - * counter and the control handover, and both belong to a running browser. A Bot whose browser has - * been closed starts a fresh session next time, which is what starting a fresh browser means. + * The counter, the run it belongs to, and the sessions holding both live in their own module: the + * rules about when a run changes are testable there, and not here, because this file launches a + * browser the moment it is imported. */ -function forgetIdleSessions(): void { - for (const [botId, session] of [...sessions.entries()]) { - if (session.viewer) continue; - if (profiles.isLive(botId)) continue; - sessions.delete(botId); - } -} - -function sessionFor(botId: string): BotSession { - const existing = sessions.get(botId); - if (existing) return existing; - const created: BotSession = { control: createControl(), snapshotId: 0 }; - sessions.set(botId, created); - // Cheap, and only ever on the path that adds one, so the map cannot grow without this running. - if (sessions.size > 32) forgetIdleSessions(); - return created; -} +const sessions = createSessions({ isLive: (botId) => profiles.isLive(botId) }); /** * Sent by the server as a header on every call. Absent means the caller does not know or does not @@ -362,7 +318,7 @@ serve({ */ websocket: { async open(ws) { - const session = sessionFor(ws.data.botId); + const session = sessions.for(ws.data.botId); try { await stopViewer(session); @@ -408,7 +364,7 @@ serve({ }, async message(ws, raw) { - const session = sessionFor(ws.data.botId); + const session = sessions.for(ws.data.botId); if (!session.viewer) return; let message: InputMessage; try { @@ -447,7 +403,7 @@ serve({ }, async close(ws) { - const session = sessionFor(ws.data.botId); + const session = sessions.for(ws.data.botId); // Only the socket that is casting. A superseded one closing after its replacement has started // would otherwise stop the new viewer; see viewer.ts. if (!isCurrentViewer(session.viewer, ws)) return; @@ -488,7 +444,7 @@ serve({ if (!isOpenPath(url.pathname) && !isPlainBotId(botId)) { return json({ error: "That is not a usable bot id." }, 400); } - const session = sessionFor(botId); + const session = sessions.for(botId); /* * The wheel, asked once for everything that acts. @@ -674,6 +630,25 @@ serve({ }); } + /** + * Which run of this Bot's browser the caller is looking at. + * + * The server orders snapshots on `(run, generation)`, and the generation alone cannot carry it: + * this process mints one at zero for every session that is new, so a restart, a redeploy, an + * eviction from the idle sweep and a reset all produce a page at generation one that looks older + * than the page the server still has. Ordering on the run as well is what lets the fresh one land + * and the dead one stop resolving. + * + * A read, and deliberately not on the acting list: a person holding the wheel must not turn every + * ref the server holds into an unanswerable question. + * + * Its own endpoint rather than a field on `/computers`, because that one reads the profile + * directory and this is asked on the path of every governed action. + */ + if (url.pathname === "/run" && request.method === "GET") { + return json({ run: session.run }); + } + /** * The computers this process holds. The shape is a list because the admin surface is a * list, and because a Bot that has a profile has a computer whether or not a browser is running @@ -708,6 +683,16 @@ serve({ await profiles.reset(botId); // Reset releases control because any previous browser session and pending secret request are gone. session.control.release(); + /* + * And a new run, because the browser this session described is gone. + * + * Nothing else here says so. The entry stays in the map and the generation counter carries on, + * so a snapshot that was in flight when the wipe landed arrives at the server carrying the same + * run and the same generation as the fresh browser's would: the server deletes its row on + * reset, the late save inserts the wiped page straight back, and every ref on it goes on + * resolving. A new run is what makes those two distinguishable at the far end. + */ + sessions.renewRun(botId); return json({ reset: true, botId }); } diff --git a/agent-computer/src/sessions.ts b/agent-computer/src/sessions.ts new file mode 100644 index 00000000..ef44cb9a --- /dev/null +++ b/agent-computer/src/sessions.ts @@ -0,0 +1,121 @@ +/** + * What this process remembers about a Bot between requests, and which run of its browser that is. + * + * ITS OWN MODULE BECAUSE THE RUN IS A DECISION, not bookkeeping. `index.ts` imports Playwright at + * module scope, so anything left in there can only be tested by launching a browser, and the rules + * below are exactly the ones with a wrong answer available: which events start a new run, and which + * leave the one in progress alone. Same reason `browser-eviction.ts` and `profile-listing.ts` are + * separate files. + * + * The generation counter orders snapshots within one run of a browser and says nothing across two. + * It is minted at zero for a session that is new, and a session is new after a container restart, a + * redeploy, an eviction from the sweep below, and a reset. None of those reach the server, so a ref + * it is still holding from the run before matches a row nothing has overwritten, and the boundary + * decides about an element on a page that no longer exists. + * + * The run carries that difference to the server, which orders on `(run, generation)` rather than on + * the generation alone. A Bot with its own container gets one from the container and a sandbox gets + * one from the moment its browser last became ready; the shared computer is one process serving + * every Bot, outliving every reset, so here the answer has to come from the session itself. + */ +import { type Control, createControl } from "./control"; +import type { Screencast } from "./screencast"; + +/** Per-Bot browser-control state. Profiles are isolated, but this process is not a security boundary. */ +export type BotSession = { + control: Control; + /** This Bot's snapshot generation. Ordered within this run, and meaningless across two. */ + snapshotId: number; + /** + * Which run of this Bot's browser this is, for the server to order snapshots across. + * + * Per Bot rather than per process: they share this container and nothing else, so a run naming the + * container would be one string for every Bot and would never change when one of their browsers + * was replaced. + */ + run: string; + /** The one live screen viewer for this Bot, if a person is watching. */ + viewer?: { + socket: unknown; + cast: Screencast; + /** Stops the loop that keeps the cast pointed at whatever page the Bot is actually on. */ + follow?: ReturnType; + }; +}; + +export type SessionsOptions = { + /** Whether this Bot has a browser right now, so the sweep keeps what belongs to a running one. */ + isLive: (botId: string) => boolean; + /** How many sessions may accumulate before the sweep runs. */ + cap?: number; + /** Injected so a test can name the run it expects rather than match a uuid. */ + mintRun?: () => string; +}; + +/** + * A factory rather than a module-level `Map`, so a test can have its own and two cannot accidentally + * share state. The same shape `createControl` uses, for the same reason. + */ +export function createSessions(options: SessionsOptions) { + const sessions = new Map(); + const cap = options.cap ?? 32; + const mintRun = options.mintRun ?? (() => crypto.randomUUID()); + + /** + * Forget the sessions of Bots whose browsers are no longer running. + * + * The map gained an entry per Bot id this process had ever seen and lost none, so a deployment + * where every employee has a Bot accumulated one small object per employee for the life of the + * container. Small, but unbounded, which is the same shape as the browsers themselves. + * + * Only entries with no live browser and nobody watching are dropped: the state is the generation + * counter, the run, and the control handover, and all three belong to a running browser. A Bot + * whose browser has been closed starts a fresh session next time, which is what starting a fresh + * browser means, and the new run is what says so. + */ + function forgetIdle(): void { + for (const [botId, session] of [...sessions.entries()]) { + if (session.viewer) continue; + if (options.isLive(botId)) continue; + sessions.delete(botId); + } + } + + function sessionFor(botId: string): BotSession { + const existing = sessions.get(botId); + if (existing) return existing; + const created: BotSession = { + control: createControl(), + snapshotId: 0, + run: mintRun(), + }; + sessions.set(botId, created); + // Cheap, and only ever on the path that adds one, so the map cannot grow without this running. + if (sessions.size > cap) forgetIdle(); + return created; + } + + return { + for: sessionFor, + + /** + * Start a new run for this Bot, because its browser session is over. + * + * A reset closes the browser and deletes the profile, and the session survives it: nothing takes + * the entry out of the map, so the generation counter carries on from where it was. Without a new + * run the server cannot tell a save that was in flight when the wipe landed from the fresh + * browser's own, and the wiped page comes back with nothing willing to refuse it. + * + * The generation is deliberately left alone. It belongs to the browser, which restarts it at zero + * on its own when a session is genuinely new, and zeroing it here would hand the server a fresh + * generation one under a fresh run for a counter that never restarted. + */ + renewRun(botId: string): string { + const existing = sessions.get(botId); + // Nothing to renew: a Bot nobody has touched yet gets a session, and its run is already new. + if (!existing) return sessionFor(botId).run; + existing.run = mintRun(); + return existing.run; + }, + }; +} diff --git a/agent-computer/tests/reset-run.test.ts b/agent-computer/tests/reset-run.test.ts new file mode 100644 index 00000000..2a3585db --- /dev/null +++ b/agent-computer/tests/reset-run.test.ts @@ -0,0 +1,130 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +/** + * The reset handler, driven over HTTP against the real process. + * + * `sessions.test.ts` covers when a run changes, and it cannot cover whether the handler asks. That + * gap is the whole of the reset half: a reset that wipes the profile and leaves the run alone reads + * as correct in every unit test and still lets a save that was in flight bring the wiped page back. + * + * So this one imports `index.ts` and talks to the port it opens. It never launches a browser, which + * is what usually makes that impossible here: `/run` reads the session and `/computers/reset` stops a + * browser that was never started and deletes a directory under a temporary root, so Chromium is + * never asked for a page. The profiles root and the workspace are pointed somewhere disposable + * before the import, because the module reads both at load. + */ + +const TOKEN = "test-computer-token"; +const PORT = 41537; +const BASE = `http://127.0.0.1:${PORT}`; + +let root = ""; + +async function run(botId: string): Promise { + const response = await fetch(`${BASE}/run`, { + headers: { + "x-openbot-bot-id": botId, + "x-openbot-computer-token": TOKEN, + }, + }); + expect(response.status).toBe(200); + return ((await response.json()) as { run?: string }).run; +} + +beforeAll(async () => { + root = await mkdtemp(join(tmpdir(), "agent-computer-reset-")); + process.env.COMPUTER_TOKEN = TOKEN; + process.env.PORT = String(PORT); + process.env.PROFILES_DIR = join(root, "profiles"); + process.env.WORKSPACE_DIR = join(root, "workspace"); + // Imported after the environment is set, because the module reads it while it loads. + await import("../src/index"); +}); + +afterAll(async () => { + await rm(root, { recursive: true, force: true }); +}); + +describe("the run this computer reports", () => { + test("is there before anything has started a browser", async () => { + expect(await run("bot-1")).toBeTruthy(); + }); + + test("is the same one on the next request", async () => { + // The control. A run minted per request would change every time and refuse every ref the server + // holds, which looks identical to a working fix from the server's side of one call. + const first = await run("bot-2"); + expect(await run("bot-2")).toBe(first as string); + }); + + test("is not shared with another Bot", async () => { + expect(await run("bot-3")).not.toBe(await run("bot-4")); + }); + + test("changes when the computer is reset", async () => { + const before = await run("bot-5"); + + const reset = await fetch(`${BASE}/computers/reset`, { + method: "POST", + headers: { + "x-openbot-bot-id": "bot-5", + "x-openbot-computer-token": TOKEN, + }, + }); + expect(reset.status).toBe(200); + + expect(await run("bot-5")).not.toBe(before as string); + }); + + test("resetting one Bot leaves another Bot's alone", async () => { + const other = await run("bot-6"); + await fetch(`${BASE}/computers/reset`, { + method: "POST", + headers: { + "x-openbot-bot-id": "bot-7", + "x-openbot-computer-token": TOKEN, + }, + }); + expect(await run("bot-6")).toBe(other as string); + }); + + test("is not told to a caller with no token", async () => { + // It names a Bot and describes this process's state, so it belongs behind the same secret as + // everything else here. Only `/health` is open. + const response = await fetch(`${BASE}/run`, { + headers: { "x-openbot-bot-id": "bot-1" }, + }); + expect(response.status).toBe(401); + }); + + test("is not told to a caller with the wrong token", async () => { + const response = await fetch(`${BASE}/run`, { + headers: { + "x-openbot-bot-id": "bot-1", + "x-openbot-computer-token": "not-the-token", + }, + }); + expect(response.status).toBe(401); + }); + + test("is still answered while a person holds the wheel", async () => { + /* + * The refusal that guards acting must not reach a read. The server asks this on the path of every + * governed action, including the ones it is about to refuse, so a 409 here would turn every ref + * it holds into an unanswerable question for as long as somebody was driving. + */ + const taken = await fetch(`${BASE}/control/take`, { + method: "POST", + headers: { + "x-openbot-bot-id": "bot-8", + "x-openbot-computer-token": TOKEN, + }, + }); + expect(taken.status).toBe(200); + + expect(await run("bot-8")).toBeTruthy(); + }); +}); diff --git a/agent-computer/tests/sessions.test.ts b/agent-computer/tests/sessions.test.ts new file mode 100644 index 00000000..2a2f1e6b --- /dev/null +++ b/agent-computer/tests/sessions.test.ts @@ -0,0 +1,151 @@ +import { describe, expect, test } from "bun:test"; +import { createSessions } from "../src/sessions"; + +/** + * Which run of a Bot's browser the server is looking at. + * + * A snapshot's generation orders snapshots within one run and says nothing across two. The counter + * lives here, in a map this process keeps, and it restarts at one whenever the session behind it is + * new: a container restart, an eviction from the idle sweep, or a redeploy. None of those tell the + * server anything, so a generation the server still holds from the run before matches a row nothing + * has overwritten, and the boundary decides about an element on a page that no longer exists. + * + * The run is what carries that difference. It is minted with the session and changes whenever the + * session does, so the server can order across two of them rather than only within one. + * + * These test the bookkeeping rather than the browser. `index.ts` imports Playwright at module scope, + * so a test that reached the handlers would drag a browser runtime in with it, which is the same + * reason `browser-eviction.ts` and `profile-listing.ts` are their own modules. + */ + +/** A mint that counts, so a test can name the run it expects rather than match a uuid. */ +function counting(): () => string { + let next = 0; + return () => `run-${++next}`; +} + +describe("the run a session carries", () => { + test("a Bot's first session is given one", () => { + const sessions = createSessions({ + isLive: () => true, + mintRun: counting(), + }); + expect(sessions.for("bot-1").run).toBe("run-1"); + }); + + test("the same session keeps it", () => { + const sessions = createSessions({ + isLive: () => true, + mintRun: counting(), + }); + sessions.for("bot-1"); + expect(sessions.for("bot-1").run).toBe("run-1"); + }); + + test("two Bots do not share one", () => { + // They share this process, and nothing else. A run that identified the container rather than the + // session would be the same string for every Bot on the one shared computer, and ordering across + // runs would never fire for any of them. + const sessions = createSessions({ + isLive: () => true, + mintRun: counting(), + }); + expect(sessions.for("bot-1").run).not.toBe(sessions.for("bot-2").run); + }); + + test("a session the idle sweep dropped comes back as a new run", () => { + /* + * The eviction path, which is the one that made the generation ambiguous in the first place. + * `forgetIdleSessions` drops a Bot with no live browser once the map is over its cap, and the + * next request mints a session counting from generation one again. Reusing the run would say + * "same run, older generation" about a browser that has been gone and back. + */ + const sessions = createSessions({ + isLive: () => false, + mintRun: counting(), + cap: 1, + }); + const first = sessions.for("bot-1").run; + // Over the cap, so the sweep runs and takes the idle Bot with it. + sessions.for("bot-2"); + expect(sessions.for("bot-1").run).not.toBe(first); + }); + + test("a Bot whose browser is running is not swept, and keeps its run", () => { + // The control. A sweep that dropped everything would pass the test above while breaking every + // ref a working Bot holds. + const sessions = createSessions({ + isLive: () => true, + mintRun: counting(), + cap: 1, + }); + const first = sessions.for("bot-1").run; + sessions.for("bot-2"); + expect(sessions.for("bot-1").run).toBe(first); + }); + + test("a Bot somebody is watching is not swept either", () => { + const sessions = createSessions({ + isLive: () => false, + mintRun: counting(), + cap: 1, + }); + const watched = sessions.for("bot-1"); + watched.viewer = { socket: {}, cast: {} as never }; + sessions.for("bot-2"); + expect(sessions.for("bot-1").run).toBe(watched.run); + }); +}); + +describe("renewing a run", () => { + test("a reset gives the Bot a new one", () => { + /* + * A reset wipes the profile and closes the browser, and the session survives it: the map is not + * touched, so the generation counter carries on from where it was. Without a new run the server + * cannot tell a save that was in flight when the wipe landed from the fresh browser's own, and + * the wiped page comes back with nothing later willing to refuse it. + */ + const sessions = createSessions({ + isLive: () => true, + mintRun: counting(), + }); + const before = sessions.for("bot-1").run; + expect(sessions.renewRun("bot-1")).not.toBe(before); + expect(sessions.for("bot-1").run).not.toBe(before); + }); + + test("it leaves the other Bots alone", () => { + const sessions = createSessions({ + isLive: () => true, + mintRun: counting(), + }); + const other = sessions.for("bot-2").run; + sessions.for("bot-1"); + sessions.renewRun("bot-1"); + expect(sessions.for("bot-2").run).toBe(other); + }); + + test("resetting a Bot that has no session yet still gives it one", () => { + // Reset is reachable before anything else has touched the Bot, and a handler that assumed a + // session was already there would answer about one it had just silently created empty. + const sessions = createSessions({ + isLive: () => true, + mintRun: counting(), + }); + expect(sessions.renewRun("bot-1")).toBe("run-1"); + }); + + test("the generation carries on, because the browser is what restarts it", () => { + // Deliberately not reset here. The counter belongs to the browser, and `sessionFor` mints it at + // zero for a session that is new; a renew that also zeroed it would make a fresh generation one + // arrive under a fresh run, which is the pair the server has no way to order. + const sessions = createSessions({ + isLive: () => true, + mintRun: counting(), + }); + const session = sessions.for("bot-1"); + session.snapshotId = 7; + sessions.renewRun("bot-1"); + expect(sessions.for("bot-1").snapshotId).toBe(7); + }); +}); diff --git a/server/src/computer/gateway.ts b/server/src/computer/gateway.ts index 21c7d985..3fdcbde4 100644 --- a/server/src/computer/gateway.ts +++ b/server/src/computer/gateway.ts @@ -342,11 +342,28 @@ export function createComputerGateway( * on the store. */ async function snapshot(botId: string): Promise { + const base = await locate(botId); + /* + * Which run this is, asked before the page is drawn rather than after it. + * + * After `locate`, because on a supervisor that is the `/ensure` that reports it. But before + * `/snapshot`, because the two answers have to describe the same browser and asking afterwards + * does not guarantee it: a computer replaced while the snapshot was being taken would have its + * dead page stamped with the run of the browser that replaced it, so every ref on that page would + * resolve against the live run and the live run's own snapshots would be refused for being older. + * + * Asked first, a replacement in that window leaves a row carrying a run that is already gone. + * Nothing resolves against it and the next snapshot supersedes it, which is the direction this is + * allowed to fail in. + */ + const run = await sessionOf(botId); const result = await transport.call( - await locate(botId), + base, botId, "/snapshot", - { method: "POST" }, + { + method: "POST", + }, ); await snapshots.save(botId, { snapshotId: result.snapshotId, @@ -354,8 +371,7 @@ export function createComputerGateway( elements: new Map( result.elements.map((element) => [element.ref, element]), ), - // Read after `locate`, which is the `/ensure` that reports it. - ...(await sessionOf(botId)), + ...run, }); return result; } @@ -401,8 +417,8 @@ export function createComputerGateway( /** * The run of the computer the action is reaching, when the provider can say. * - * Undefined means unknown, not mismatched: a provider with no sessions to report, or one that - * could not be asked, leaves the generation check exactly as it was. + * Undefined means unknown, not mismatched: a provider that could not be asked leaves the + * generation check exactly as it was, rather than refusing every ref it holds. */ session?: string, ): SnapshotElement | undefined { diff --git a/server/src/computer/provider.ts b/server/src/computer/provider.ts index 6dd30336..1044f70e 100644 --- a/server/src/computer/provider.ts +++ b/server/src/computer/provider.ts @@ -88,8 +88,14 @@ export interface ComputerProvider { * * A snapshot's generation only orders snapshots within one run: a replaced container counts from * one again, so a ref from the run before it matches a row nothing has overwritten. This is what - * tells the two apart. Optional because a deployment with one shared computer has no supervisor to - * ask, and there the comparison is skipped and behaviour is unchanged. + * tells the two apart. + * + * Every provider answers it, and where the answer comes from is the whole of the difference between + * them: a container per Bot reads it off the container, a sandbox off the moment its browser last + * became ready, and the one shared computer has to ask the computer, because that process serves + * every Bot and outlives every reset. Optional in the type only for a provider that genuinely + * cannot say; undefined then means unknown rather than mismatched, and the comparison is skipped + * exactly as it was before any of this existed. */ sessionOf?(botId: string): Promise; } @@ -165,6 +171,36 @@ export function createSharedComputerProvider( return options.baseUrl; }, + /** + * Which run of this Bot's browser is current, asked of the computer itself. + * + * NOTHING ELSE HERE KNOWS. A Bot with its own container gets a run from the container and a + * sandbox gets one from the moment its browser last became ready; this deployment is one process + * serving every Bot, and it outlives every reset, so the container says the same thing before and + * after the browser it was asked about was replaced. The computer is the only party that can tell + * a session apart from the one before it, so it is the one asked. + * + * NOT REMEMBERED, unlike the supervisor's, and the difference is `locate`: there it is an + * `/ensure` that refreshes the answer before every action, while here it is a string and makes no + * call at all. A cached run would then be the one this process first saw, for the life of the + * process, which is exactly the stale answer this exists to stop giving. + */ + async sessionOf(botId: string): Promise { + try { + const body = (await call("/run", "GET", botId)) as { + run?: unknown; + } | null; + return typeof body?.run === "string" && body.run.length > 0 + ? body.run + : undefined; + } catch { + // Unknown, not mismatched. A computer that cannot be reached, or one from before this + // endpoint existed, must not turn every ref into a refusal; the comparison goes back to + // being skipped, which is where it started. + return undefined; + } + }, + async status(botId: string): Promise { try { await call("/health", "GET", botId); diff --git a/server/src/computer/snapshot-store.ts b/server/src/computer/snapshot-store.ts index 4bf26a90..3c11a5dc 100644 --- a/server/src/computer/snapshot-store.ts +++ b/server/src/computer/snapshot-store.ts @@ -47,8 +47,9 @@ export type StoredSnapshot = { * * The generation only orders snapshots within one session. This is what tells two sessions apart, * so a ref from a computer that has since been replaced resolves to nothing instead of to whatever - * now holds that ref. Undefined where there is no supervisor to ask, which leaves the behaviour as - * it was. + * now holds that ref. Undefined only where the provider could not be asked at all, which leaves the + * behaviour as it was: a null on either side skips the comparison, so a row written during an + * outage stays unordered against the runs around it. */ session?: string; }; diff --git a/server/tests/computer-gateway.test.ts b/server/tests/computer-gateway.test.ts index e2b4c255..8c017554 100644 --- a/server/tests/computer-gateway.test.ts +++ b/server/tests/computer-gateway.test.ts @@ -7,9 +7,10 @@ import { WorkspaceRefusedError, } from "../src/computer/gateway"; import type { ActionPolicy } from "../src/computer/policy"; -import type { - ComputerLocation, - ComputerProvider, +import { + type ComputerLocation, + type ComputerProvider, + createSharedComputerProvider, } from "../src/computer/provider"; import type { SnapshotResult } from "../src/computer/schema"; import { @@ -1318,3 +1319,191 @@ describe("acting on a ref the server cannot resolve", () => { expect(calls).toEqual(["scroll"]); }); }); + +/** + * The deployment with one computer for every Bot. + * + * Every session case above supplies a provider that reports a run, and the shared computer is the one + * that had nothing to report it from: no supervisor, no container per Bot, one process that outlives + * every reset. The ordering the rest of this file exercises was therefore inert exactly there, and + * the two failures it exists to stop were both reachable. These go through the real provider rather + * than a fake, because the fake is what hid it. + */ +describe("a Bot on the one shared computer", () => { + const FRESH = { + snapshotId: 1, + url: "https://fresh.example/start", + title: "Start", + truncated: false, + elements: [{ ref: "e1", role: "link", name: "Sign in" }], + } satisfies SnapshotResult; + + /** A shared computer that answers which run each Bot's browser is on, as the real one does. */ + function sharedComputer() { + let run = "run-1"; + let live: SnapshotResult = SNAPSHOT; + let afterSnapshot: (() => void) | undefined; + const server = Bun.serve({ + port: 0, + fetch: async (request) => { + const path = new URL(request.url).pathname; + if (path === "/run") return Response.json({ run }); + if (path === "/snapshot") { + const answer = Response.json(live); + // A computer that goes away as it answers, which is the window the run has to be read before. + afterSnapshot?.(); + return answer; + } + if (path === "/computers/reset") return Response.json({ reset: true }); + if (path === "/click") + return Response.json({ + action: "click", + url: live.url, + elapsedMs: 1, + }); + return Response.json({ error: path }, { status: 404 }); + }, + }); + return { + baseUrl: `http://127.0.0.1:${server.port}`, + stop: () => server.stop(true), + /** A new browser session: a restart, an eviction, a redeploy, or a reset. */ + replaceRun: (next: string) => { + run = next; + }, + showing: (next: SnapshotResult) => { + live = next; + }, + /** Something that happens the instant the snapshot is answered, and before anything else is asked. */ + onSnapshot: (effect: () => void) => { + afterSnapshot = effect; + }, + }; + } + + function gatewayOn(computer: ReturnType) { + const snapshots = createInMemorySnapshotStore(); + const { store, rows } = fakeAudit(); + const gateway = createComputerGateway({ + provider: createSharedComputerProvider({ baseUrl: computer.baseUrl }), + auditStore: store, + policy: () => PERMISSIVE, + snapshots, + }); + return { gateway, rows, snapshots }; + } + + test("a save still in flight when the wipe landed cannot resurrect the page", async () => { + /* + * `clear` deletes the row, so a save that was already on its way finds nothing to conflict with + * and inserts unconditionally. The row is back, describing a page the reset destroyed, and the + * policy decides about its elements: a deny rule on "Submit order" fires on a click nowhere near + * one, or a rule written for the new page does not fire on one that is. + * + * The write cannot tell the two apart, and does not have to. The resurrected row carries the run + * that took it, the reset gave the Bot a new one, and the read refuses the citation. + */ + const computer = sharedComputer(); + try { + const { gateway, rows, snapshots } = gatewayOn(computer); + const taken = await gateway.snapshot("bot-1"); + + await gateway.resetComputer("bot-1", ACTOR); + expect(await snapshots.load("bot-1")).toBeUndefined(); + // The reset closed that browser, so the next one is a different run. + computer.replaceRun("run-2"); + // And now the save that was in flight when it landed, carrying the run it was taken on. + await snapshots.save("bot-1", { + snapshotId: taken.snapshotId, + url: taken.url, + elements: new Map( + taken.elements.map((element) => [element.ref, element]), + ), + session: "run-1", + }); + + const refusal = await gateway + .click("bot-1", ACTOR, { ref: "e9", snapshotId: taken.snapshotId }) + .catch((error: unknown) => error); + + expect(refusal).toBeInstanceOf(StaleSnapshotError); + expect(rows.at(-1)?.payload.element).toBe("not in the current snapshot"); + } finally { + computer.stop(); + } + }); + + test("a restarted computer's first snapshot lands instead of being read as stale", async () => { + /* + * The generation lives in a map in the computer's process, and a restart, a redeploy, or the idle + * sweep dropping the session all mint it at zero again. Nothing clears the server's row, so with + * the generation as the only ordering the fresh page looks older than the dead one and every + * snapshot is dropped until the counter climbs back past it. Refs then resolve against a page + * nobody is on. + */ + const computer = sharedComputer(); + try { + const { gateway, snapshots } = gatewayOn(computer); + await gateway.snapshot("bot-1"); + expect((await snapshots.load("bot-1"))?.snapshotId).toBe(7); + + computer.replaceRun("run-2"); + computer.showing(FRESH); + await gateway.snapshot("bot-1"); + + const held = await snapshots.load("bot-1"); + expect(held?.snapshotId).toBe(1); + expect(held?.url).toBe(FRESH.url); + } finally { + computer.stop(); + } + }); + + test("a computer replaced while a snapshot was being taken does not stamp the page with the new run", async () => { + /* + * The run is asked for on the same path as the snapshot, and the two answers have to describe the + * same browser. Asked afterwards, they need not: a container that goes away between answering + * `/snapshot` and answering this one stamps the page it drew with the run of the browser that + * replaced it, and the row then claims the live run is showing a page from the dead one. Every + * ref on it resolves, and the fresh browser's own snapshots are refused for being older, which is + * both halves of the bug back inside a smaller window. + * + * Asked first, the stamp is the run that was live when we asked. A computer replaced underneath + * leaves a row nothing will match until the next snapshot lands, which is the direction this is + * allowed to fail in. + */ + const computer = sharedComputer(); + try { + const { gateway, rows, snapshots } = gatewayOn(computer); + computer.onSnapshot(() => computer.replaceRun("run-2")); + + const taken = await gateway.snapshot("bot-1"); + expect((await snapshots.load("bot-1"))?.session).toBe("run-1"); + + const refusal = await gateway + .click("bot-1", ACTOR, { ref: "e9", snapshotId: taken.snapshotId }) + .catch((error: unknown) => error); + + expect(refusal).toBeInstanceOf(StaleSnapshotError); + expect(rows.at(-1)?.payload.element).toBe("not in the current snapshot"); + } finally { + computer.stop(); + } + }); + + test("within one run an older snapshot still does not overwrite a newer one", async () => { + // The control. Ordering across runs that also stopped ordering within one would let two replicas + // snapshotting the same computer land in whichever order Postgres saw them. + const computer = sharedComputer(); + try { + const { gateway, snapshots } = gatewayOn(computer); + await gateway.snapshot("bot-1"); + computer.showing(FRESH); + await gateway.snapshot("bot-1"); + + expect((await snapshots.load("bot-1"))?.snapshotId).toBe(7); + } finally { + computer.stop(); + } + }); +}); diff --git a/server/tests/computer-provider.test.ts b/server/tests/computer-provider.test.ts index be508c90..5485675e 100644 --- a/server/tests/computer-provider.test.ts +++ b/server/tests/computer-provider.test.ts @@ -24,6 +24,7 @@ type FakeAgentComputerHandler = { computers?: (request: Request) => Response | Promise; stop?: (request: Request) => Response | Promise; reset?: (request: Request) => Response | Promise; + run?: (request: Request) => Response | Promise; }; function serveAgentComputer( @@ -57,6 +58,11 @@ function serveAgentComputer( return Response.json({ stopped: true, wasRunning: true }); } + if (url.pathname === "/run" && request.method === "GET") { + if (handlers.run) return handlers.run(request); + return Response.json({ run: "run-1" }); + } + if (url.pathname === "/computers/reset" && request.method === "POST") { if (handlers.reset) return handlers.reset(request); const botId = request.headers.get("x-openbot-bot-id") ?? "shared"; @@ -267,6 +273,98 @@ describe("shared computer provider", () => { ]); }); + /** + * Which run of the shared computer this is. + * + * The one part of the boundary that has no infrastructure to read it off. A Bot with its own + * container gets a run from the container, and a sandbox gets one from the moment its browser last + * became ready, but every Bot here shares one container that outlives every reset, so the only + * thing that knows a browser session was replaced is the computer itself. Without an answer the + * server orders snapshots on the generation alone, which is the ordering that cannot tell a save + * left over from a wiped session apart from the fresh browser's first. + */ + test("reports the run the computer says this Bot's browser is on", async () => { + const asked: { + path: string; + botId: string | null; + token: string | null; + }[] = []; + const baseUrl = serveAgentComputer( + { + run: (request) => { + asked.push({ + path: new URL(request.url).pathname, + botId: request.headers.get("x-openbot-bot-id"), + token: request.headers.get("x-openbot-computer-token"), + }); + return Response.json({ run: "d7c0f1" }); + }, + }, + { token: "computer-secret" }, + ); + const provider = createSharedComputerProvider({ + baseUrl, + token: "computer-secret", + }); + + expect(await provider.sessionOf?.("sales")).toBe("d7c0f1"); + // Addressed and authenticated like every other call, or one Bot would be told about another's. + expect(asked).toEqual([ + { path: "/run", botId: "sales", token: "computer-secret" }, + ]); + }); + + test("each Bot gets its own, because they share the container and nothing else", async () => { + const baseUrl = serveAgentComputer({ + run: (request) => + Response.json({ + run: `run-of-${request.headers.get("x-openbot-bot-id")}`, + }), + }); + const provider = createSharedComputerProvider({ baseUrl }); + + expect(await provider.sessionOf?.("sales")).toBe("run-of-sales"); + expect(await provider.sessionOf?.("analytics")).toBe("run-of-analytics"); + }); + + test("asks again rather than remembering, because a restart is not announced", async () => { + // The supervisor caches because `/ensure` refreshes it on every action. Nothing refreshes this + // one: `locate` here is a string and makes no call, so a remembered run would say "same run" + // for the whole life of the process, which is the answer this exists to stop giving. + let run = "run-1"; + const baseUrl = serveAgentComputer({ + run: () => Response.json({ run }), + }); + const provider = createSharedComputerProvider({ baseUrl }); + + expect(await provider.sessionOf?.("sales")).toBe("run-1"); + run = "run-2"; + expect(await provider.sessionOf?.("sales")).toBe("run-2"); + }); + + test("answers undefined when the computer cannot say, rather than throwing", async () => { + // A computer from before this endpoint existed, or one that is not answering. Unknown is not + // mismatched: the comparison goes back to being skipped, which is where it started, and a + // refusal on every ref would be a far worse failure than the one being fixed. + const baseUrl = serveAgentComputer({ + run: () => Response.json({ error: "Not found." }, { status: 404 }), + }); + const provider = createSharedComputerProvider({ baseUrl }); + + // Asserted first, or a provider with no `sessionOf` at all would pass this vacuously. + expect(provider.sessionOf).toBeDefined(); + expect(await provider.sessionOf?.("sales")).toBeUndefined(); + }); + + test("answers undefined when the computer answers without one", async () => { + const baseUrl = serveAgentComputer({ run: () => Response.json({}) }); + const provider = createSharedComputerProvider({ baseUrl }); + + // Asserted first, or a provider with no `sessionOf` at all would pass this vacuously. + expect(provider.sessionOf).toBeDefined(); + expect(await provider.sessionOf?.("sales")).toBeUndefined(); + }); + test("aborts fetch that never settles with configurable timeoutMs and throws ProviderError", async () => { const baseUrl = serve(() => new Promise(() => {})); const provider = createSharedComputerProvider({ diff --git a/server/tests/shared-computer-run.integration.test.ts b/server/tests/shared-computer-run.integration.test.ts new file mode 100644 index 00000000..eab2ad44 --- /dev/null +++ b/server/tests/shared-computer-run.integration.test.ts @@ -0,0 +1,249 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import type { AuditEventInput, AuditStore } from "../src/audit"; +import { StaleSnapshotError } from "../src/computer/client"; +import { createComputerGateway } from "../src/computer/gateway"; +import type { ActionPolicy } from "../src/computer/policy"; +import { createSharedComputerProvider } from "../src/computer/provider"; +import type { SnapshotResult } from "../src/computer/schema"; +import { createSnapshotStore } from "../src/computer/snapshot-store"; +import { createDatabase } from "../src/db/client"; +import { computerSnapshot } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; + +/** + * The one shared computer, through the table rather than through a `Map`. + * + * `computer-gateway.test.ts` covers this deployment against the in-memory store, and the two stores + * are separate implementations of the same rule: one is a comparison in TypeScript and the other is + * a `setWhere` Postgres evaluates. Ordering across runs is exactly where they could disagree, and a + * deployment runs the one that is not covered by that file. + * + * Everything below the stub computer is real: the provider makes its own HTTP calls, the gateway + * resolves and governs, and the row is a row. + */ + +const database = createDatabase( + process.env.DATABASE_URL ?? + "postgres://openbot:openbot@localhost:5432/openbot", + TEST_POOL, +); + +afterEach(async () => { + await database.delete(computerSnapshot); +}); + +const PERMISSIVE: ActionPolicy = { mode: "enforce", deny: [], allow: ["true"] }; +const ACTOR = { id: "dev-local-user" }; + +const DEAD: SnapshotResult = { + snapshotId: 7, + url: "https://bank.example/transfer", + title: "Transfer", + truncated: false, + elements: [{ ref: "e9", role: "button", name: "Confirm transfer" }], +}; + +const FRESH: SnapshotResult = { + snapshotId: 1, + url: "https://fresh.example/start", + title: "Start", + truncated: false, + elements: [{ ref: "e1", role: "link", name: "Sign in" }], +}; + +/** A shared computer that answers which run each Bot's browser is on, as the real one does. */ +function sharedComputer() { + let run = "run-1"; + let live: SnapshotResult = DEAD; + const server = Bun.serve({ + port: 0, + fetch: async (request) => { + const path = new URL(request.url).pathname; + if (path === "/run") return Response.json({ run }); + if (path === "/snapshot") return Response.json(live); + if (path === "/computers/reset") return Response.json({ reset: true }); + if (path === "/click") + return Response.json({ action: "click", url: live.url, elapsedMs: 1 }); + return Response.json({ error: path }, { status: 404 }); + }, + }); + return { + baseUrl: `http://127.0.0.1:${server.port}`, + stop: () => server.stop(true), + /** A new browser session: a restart, an eviction, a redeploy, or a reset. */ + replaceRun: (next: string) => { + run = next; + }, + showing: (next: SnapshotResult) => { + live = next; + }, + }; +} + +function gatewayOn(computer: ReturnType) { + const rows: AuditEventInput[] = []; + const auditStore: AuditStore = { + insert: async (event) => void rows.push(event), + }; + const snapshots = createSnapshotStore(database); + return { + rows, + snapshots, + gateway: createComputerGateway({ + provider: createSharedComputerProvider({ baseUrl: computer.baseUrl }), + auditStore, + policy: () => PERMISSIVE, + snapshots, + }), + }; +} + +describe("a Bot on the one shared computer, against the table", () => { + test("a save still in flight when the wipe landed cannot resurrect the page", async () => { + const computer = sharedComputer(); + try { + const { gateway, rows, snapshots } = gatewayOn(computer); + const taken = await gateway.snapshot("bot-1"); + + await gateway.resetComputer("bot-1", ACTOR); + expect(await snapshots.load("bot-1")).toBeUndefined(); + computer.replaceRun("run-2"); + // The row is gone, so this inserts rather than conflicting, which is the whole of the race. + await snapshots.save("bot-1", { + snapshotId: taken.snapshotId, + url: taken.url, + elements: new Map( + taken.elements.map((element) => [element.ref, element]), + ), + session: "run-1", + }); + expect((await snapshots.load("bot-1"))?.url).toBe(DEAD.url); + + const refusal = await gateway + .click("bot-1", ACTOR, { ref: "e9", snapshotId: taken.snapshotId }) + .catch((error: unknown) => error); + + expect(refusal).toBeInstanceOf(StaleSnapshotError); + expect(rows.at(-1)?.payload.element).toBe("not in the current snapshot"); + } finally { + computer.stop(); + } + }); + + test("a restarted computer's first snapshot takes the update branch", async () => { + /* + * The `setWhere` case that only Postgres decides. The row holds generation seven, the fresh + * browser offers one, and on the generation alone `lt(7, 1)` is false and the update is skipped + * with no error to say so. The run is what makes it land. + */ + const computer = sharedComputer(); + try { + const { gateway, snapshots } = gatewayOn(computer); + await gateway.snapshot("bot-1"); + expect((await snapshots.load("bot-1"))?.snapshotId).toBe(7); + + computer.replaceRun("run-2"); + computer.showing(FRESH); + await gateway.snapshot("bot-1"); + + // Read through a second store, the way a second replica would, rather than from the one that + // wrote it. + const held = await createSnapshotStore(database).load("bot-1"); + expect(held?.snapshotId).toBe(1); + expect(held?.url).toBe(FRESH.url); + expect(held?.session).toBe("run-2"); + } finally { + computer.stop(); + } + }); + + test("and its refs resolve again, rather than the dead page's", async () => { + // What the row is for. Landing the fresh snapshot is only half the property; the other half is + // that the boundary now decides about the page somebody is actually on. + const computer = sharedComputer(); + try { + const { gateway, rows } = gatewayOn(computer); + await gateway.snapshot("bot-1"); + computer.replaceRun("run-2"); + computer.showing(FRESH); + const fresh = await gateway.snapshot("bot-1"); + + await gateway.click("bot-1", ACTOR, { + ref: "e1", + snapshotId: fresh.snapshotId, + }); + + expect(rows.at(-1)?.payload.element).toMatchObject({ name: "Sign in" }); + } finally { + computer.stop(); + } + }); + + test("within one run an older snapshot still does not overwrite a newer one", async () => { + // The control, and the property #46 established. Ordering across runs that stopped ordering + // within one would let two replicas snapshotting the same computer land in whichever order + // Postgres happened to see them. + const computer = sharedComputer(); + try { + const { gateway, snapshots } = gatewayOn(computer); + await gateway.snapshot("bot-1"); + computer.showing(FRESH); + await gateway.snapshot("bot-1"); + + expect((await snapshots.load("bot-1"))?.snapshotId).toBe(7); + expect((await snapshots.load("bot-1"))?.url).toBe(DEAD.url); + } finally { + computer.stop(); + } + }); + + test("a computer that cannot say which run it is on leaves the row unordered, not refused", async () => { + /* + * A computer from before `/run` existed. Unknown is not mismatched: the ordering goes back to the + * generation alone, which is where it started, and every ref still resolves. A fix that turned a + * missing answer into a refusal would take a working deployment down on the way to fixing this. + */ + const server = Bun.serve({ + port: 0, + fetch: async (request) => { + const path = new URL(request.url).pathname; + if (path === "/snapshot") return Response.json(DEAD); + if (path === "/click") + return Response.json({ + action: "click", + url: DEAD.url, + elapsedMs: 1, + }); + return Response.json({ error: "Not found." }, { status: 404 }); + }, + }); + try { + const rows: AuditEventInput[] = []; + const auditStore: AuditStore = { + insert: async (event) => void rows.push(event), + }; + const snapshots = createSnapshotStore(database); + const gateway = createComputerGateway({ + provider: createSharedComputerProvider({ + baseUrl: `http://127.0.0.1:${server.port}`, + }), + auditStore, + policy: () => PERMISSIVE, + snapshots, + }); + + const taken = await gateway.snapshot("bot-1"); + expect((await snapshots.load("bot-1"))?.session).toBeUndefined(); + + await gateway.click("bot-1", ACTOR, { + ref: "e9", + snapshotId: taken.snapshotId, + }); + expect(rows.at(-1)?.payload.element).toMatchObject({ + name: "Confirm transfer", + }); + } finally { + server.stop(true); + } + }); +});