diff --git a/CHANGELOG.md b/CHANGELOG.md index 6eb5d41b..f3338284 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,45 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A sign-in a site opens in a new window is shown, and can be clicked + +A Bot's browser was bound to the page it launched with, and to nothing the site opened afterwards. +Anything arriving in a new window or tab was invisible on the live screen and unreachable by input, +so the popup sign-ins that a person takes the wheel to complete were exactly the ones they could not +complete. Worse than invisible: a click at the place the popup's button was drawn went to the page +underneath it, so a person trying to finish a sign-in could navigate the page the Bot was working on +without seeing either result. + +The browser now follows the window the site opens, and returns to the opener when it closes, which is +what a sign-in popup does when it succeeds. A snapshot taken before the change of page is refused +afterwards with the same "take a new snapshot" it already gives after a navigation, so a stale ref +cannot act on the wrong document. + +Nothing to configure. +### Stopping a Bot's computer stops it, and the person watching is told + +A computer somebody stopped came back up on its own about a second later, and reset did the same. The +live screen kept a loop asking for the Bot's current page once a second, asking for a page is what +starts a browser, and nothing tore that loop down when the browser it was showing went away. The same +loop kept the browser marked recently used, so a Bot with somebody watching was also immune to the +idle timeout and came straight back after being closed to stay under the cap on running browsers. +Those last two never involved a request at all, so nothing on the stop path could have covered them. + +Two smaller failures went with it. A person who reconnected, leaving their old window open, could +have that old window's typing land in the page the new one was watching, with nothing said to either. +And a window closed while the browser was still starting left a screencast and its loop behind for a +connection that had already gone. + +The screen is now held per connection rather than per Bot, so closing one only ever ends its own, and +teardown hangs off the browser closing rather than off the two requests that ask for it. A viewer +whose screen ends is sent a message saying why, whether the computer stopped, was reset, or the +screen was taken over by another window. + +Nothing to configure, and no change for a deployment where nobody watches a Bot work. **The app does +not yet show that message**: it arrives at the browser and is held in state the live screen does not +read, so a person still sees the last frame until they reopen the screen. That half is tracked +separately in #287. + ## 0.0.5 ### One Bot can hand work to another, and reach a person when no Bot will do diff --git a/agent-computer/src/env.ts b/agent-computer/src/env.ts index 2cd93262..37565f58 100644 --- a/agent-computer/src/env.ts +++ b/agent-computer/src/env.ts @@ -14,3 +14,36 @@ export function numberFromEnv(name: string, fallback: number): number { const value = Number(raw); return Number.isFinite(value) && value > 0 ? value : fallback; } + +/** + * Wait for something that ought to finish, and carry on when it does not. + * + * Closing a browser means asking Chromium and a CDP session to stop, and either can decline to + * answer: a page that has already gone, a socket that is still open but dead, a renderer that is not + * coming back. None of that is a reason for the caller to stop, and the callers here are the ones + * that must not stop. A teardown that never settles otherwise pins the Bot it belongs to, blocks the + * launch of whichever Bot triggered the eviction, and on the way out holds every profile's flush + * until the container is killed instead. + * + * So the wait is bounded and the result is discarded either way, the same bargain `closeAndWait` + * already makes: better to lose the last seconds of a cast than to never close anything again. + * Rejections are swallowed for the same reason, since a failed stop and a slow one leave the caller + * with the same work to do. + */ +export async function settleWithin( + work: Promise | undefined, + budgetMs: number, +): Promise { + if (!work) return; + let timer: ReturnType | undefined; + const deadline = new Promise((resolve) => { + timer = setTimeout(resolve, budgetMs); + // Housekeeping must never be the reason the process stays up. + timer.unref?.(); + }); + try { + await Promise.race([work.catch(() => undefined), deadline]); + } finally { + if (timer) clearTimeout(timer); + } +} diff --git a/agent-computer/src/index.ts b/agent-computer/src/index.ts index 5d7177a6..341fcc34 100644 --- a/agent-computer/src/index.ts +++ b/agent-computer/src/index.ts @@ -18,13 +18,9 @@ import { } 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 { createShell } from "./shell"; -import { isCurrentViewer } from "./viewer"; +import { createViewerSlot, type ViewerSlot } from "./viewer"; import { createWorkspace, WorkspaceFileError, @@ -112,13 +108,15 @@ 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; - }; + /** The page this Bot was last handed, so a change of page can retire its refs. */ + livePage?: Page; + /** + * This Bot's live screen, and who owns it. + * + * Always present, because the slot is the answer to "is anybody watching" as well as the holder of + * whoever is. An empty slot is a Bot nobody is watching; there is no second way to say that. + */ + viewer: ViewerSlot; }; const sessions = new Map(); @@ -133,10 +131,16 @@ const sessions = new Map(); * 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. + * + * "Nobody watching" starts at the claim, not at the first frame. A viewer whose browser is still + * launching holds a claim and no cast, and reading occupancy from the cast would call that Bot idle + * for the whole cold launch: this runs on the path that adds a session, so another Bot connecting + * then would drop the control handover out from under a screen that is seconds from live. It stays + * occupied until teardown finishes, for the same reason at the other end. */ function forgetIdleSessions(): void { for (const [botId, session] of [...sessions.entries()]) { - if (session.viewer) continue; + if (session.viewer.occupied()) continue; if (profiles.isLive(botId)) continue; sessions.delete(botId); } @@ -145,7 +149,11 @@ function forgetIdleSessions(): void { function sessionFor(botId: string): BotSession { const existing = sessions.get(botId); if (existing) return existing; - const created: BotSession = { control: createControl(), snapshotId: 0 }; + const created: BotSession = { + control: createControl(), + snapshotId: 0, + viewer: createViewerSlot(), + }; 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(); @@ -187,7 +195,23 @@ const workspace = createWorkspace(process.env.WORKSPACE_DIR ?? "/workspace"); * `chromium.launch()` gives a fresh anonymous profile every time. Persistent profiles live on a * mounted volume so sign-in state survives the container. */ -const profiles = createProfiles(process.env.PROFILES_DIR ?? "/profiles"); +/** + * The browsers, and what a closing one takes with it. + * + * Every close announces itself here, whether it came from a request, the cap, or the idle sweep, and + * the live screen watching that Bot comes down with it. Hanging this off the close rather than + * calling it from the stop and reset handlers is what covers the two closes no request makes: a + * viewer that outlived one of those kept a 1Hz loop asking for a page, which starts a browser, so + * the Bot was immune to the idle timeout and came straight back after a cap eviction. + * + * `sessions.get`, never `sessionFor`: a Bot with no session has nobody watching, and inventing one + * here would put an entry in the map on the path that closes browsers, which is where the map is + * meant to shrink. + */ +const profiles = createProfiles( + process.env.PROFILES_DIR ?? "/profiles", + (botId) => sessions.get(botId)?.viewer.releaseAll(COMPUTER_STOPPED), +); // Rooted in the same workspace the file tools use, so a command and a written file see one // directory rather than two. const shell = createShell(process.env.WORKSPACE_DIR ?? "/workspace"); @@ -210,7 +234,13 @@ const DEFAULT_BOT_ID = (() => { })(); async function currentPage(botId: string): Promise { - return profiles.page(botId); + const session = sessionFor(botId); + const page = await profiles.page(botId); + // A ref names an element on the page it was taken from, so moving to a window the site opened has + // to retire the outstanding ones exactly as a navigation does, or a click lands on the wrong document. + if (session.livePage && session.livePage !== page) session.snapshotId += 1; + session.livePage = page; + return page; } /** @@ -330,19 +360,17 @@ function json(body: unknown, status = 200): Response { }); } -/** - * One live viewer at a time per Bot, so a reconnect replaces rather than stacks, and two people - * watching two different Bots do not fight over one cast. - * - * A second cast on the same page would have Chrome encoding every frame twice and both sockets acking - * independently, which stalls both. One person drives; one cast. - */ -async function stopViewer(session: BotSession): Promise { - const current = session.viewer; - session.viewer = undefined; - if (current?.follow) clearInterval(current.follow); - await current?.cast.stop(); -} +/** What a person watching is told when the browser they were watching went away. */ +const COMPUTER_STOPPED = + "This computer stopped, so the screen ended. Start it again to carry on watching."; + +/** What a person is told when their screen is still opening and they have already started typing. */ +const SCREEN_STILL_STARTING = + "This screen is still starting. Try that again in a moment."; + +/** What a person is told when they act on a screen that is no longer theirs, or no longer anything. */ +const SCREEN_NO_LONGER_LIVE = + "This screen is no longer live. Reopen it to carry on watching."; /** How often the cast checks that it is still showing the page the Bot is on. */ const FOLLOW_INTERVAL_MS = 1_000; @@ -363,15 +391,31 @@ serve({ websocket: { async open(ws) { const session = sessionFor(ws.data.botId); + /* + * Claimed before anything is awaited, and that order is the fix. + * + * Below this line the Bot's browser may have to be launched, which takes long enough for a + * client to connect and go away inside it. A close arriving in that window used to find nothing + * installed and so did nothing, while this function carried on to install a cast and a 1Hz + * interval for a socket that had already gone: no second close ever came, and the interval went + * on relaunching a browser somebody had stopped. With the claim taken first there is always + * something for that close to release, and everything below goes through the claim and is + * refused once it is gone. + */ + const claim = session.viewer.claim(ws, (reason) => { + try { + ws.send(JSON.stringify({ type: "error", error: reason })); + } catch { + // Best effort. The socket may already be gone, which is not a reason to stop tearing down. + } + }); try { - await stopViewer(session); - const send = (frame: unknown) => { // A closed socket starts a fresh cast on the next connection. try { ws.send(JSON.stringify(frame)); } catch { - void stopViewer(session); + void session.viewer.release(ws); } }; @@ -383,20 +427,25 @@ serve({ const attach = async () => { const target = await currentPage(ws.data.botId); if (target === casting) return; - const previous = session.viewer; const cast = await startScreencast(target, send); + // The claim stops a cast it refuses, so a launch that lost the screen leaks nothing. + if (!(await claim.install(cast))) return; casting = target; - session.viewer = { socket: ws, cast, follow: previous?.follow }; - // The old cast stops after the replacement is running, so the screen does not go blank. - await previous?.cast.stop().catch(() => undefined); }; await attach(); const follow = setInterval(() => { void attach().catch(() => undefined); }, FOLLOW_INTERVAL_MS); - if (session.viewer) session.viewer.follow = follow; + if (!claim.setFollow(() => clearInterval(follow))) return; } catch (error) { + /* + * Released before the socket is told, because this path is reachable in exactly the timing + * the claim exists for. A claim left behind here would keep the session occupied for the life + * of the process, and `forgetIdleSessions` could never sweep it: the unbounded growth that + * function was written to stop, reintroduced by the error path of the fix for it. + */ + await session.viewer.release(ws); ws.send( JSON.stringify({ type: "error", @@ -409,7 +458,30 @@ serve({ async message(ws, raw) { const session = sessionFor(ws.data.botId); - if (!session.viewer) return; + /* + * Whose screen this is, asked before anything is done with the input. + * + * A superseded socket used to dispatch through whatever the session held, so a replaced + * window's typing landed in the page the current viewer was watching. It heard nothing about + * it either, because the old missing-viewer check returned before reaching anything that could + * report, which is why this answers the sender rather than returning quietly. + * + * Starting and gone are told apart deliberately. Both own no cast, and answering them the same + * way tells somebody whose screen is still opening that their session ended. + */ + const standing = session.viewer.standingOf(ws); + if (standing.state !== "casting") { + ws.send( + JSON.stringify({ + type: "error", + error: + standing.state === "starting" + ? SCREEN_STILL_STARTING + : SCREEN_NO_LONGER_LIVE, + }), + ); + return; + } let message: InputMessage; try { message = JSON.parse(String(raw)) as InputMessage; @@ -420,13 +492,17 @@ serve({ // without this check, anything that could reach this port could drive the browser while a Bot // was working, which is the one thing the control state exists to prevent. // + // Owning the screen is not permission either, which is why this stands after the ownership + // question above and not instead of it: the two refuse different things, and the one asked + // first only decides whether the input has anywhere to land. + // // Refuse with an error so the surface can explain why input is ignored. if (!session.control.humanMayDrive()) { ws.send(JSON.stringify({ type: "error", error: TAKE_CONTROL_FIRST })); return; } try { - await session.viewer.cast.send(message); + await standing.cast.send(message); } catch (error) { // Reported rather than swallowed. A dispatch that fails means the person's input did nothing, // and they must not be left believing it landed. @@ -447,11 +523,13 @@ serve({ }, async close(ws) { - const session = sessionFor(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; - await stopViewer(session); + // Names the socket, so it can only ever give up its own screen. A superseded socket closing + // after its replacement has started releases nothing; see viewer.ts. + // + // `get`, not `sessionFor`: a socket closing for a Bot with no session has nothing to release, + // and creating one here would add a map entry on a teardown path, which is the direction the + // map is meant to shrink in. + await sessions.get(ws.data.botId)?.viewer.release(ws); }, }, async fetch(request, server) { diff --git a/agent-computer/src/live-page.ts b/agent-computer/src/live-page.ts new file mode 100644 index 00000000..ee18064b --- /dev/null +++ b/agent-computer/src/live-page.ts @@ -0,0 +1,16 @@ +// Its own file so a test can reach it without the Playwright import `profiles.ts` carries. + +/** Enough of a page for the choice. Keeps this file independent of what else is on one. */ +export type OpenPage = { isClosed(): boolean }; + +// The newest open page wins, so a window the site opens becomes the one being watched and driven, +// and closing it falls back to whatever is still open rather than to a page that has gone. +export function chooseLivePage( + opened: readonly T[], +): T | undefined { + for (let index = opened.length - 1; index >= 0; index -= 1) { + const page = opened[index]; + if (page && !page.isClosed()) return page; + } + return undefined; +} diff --git a/agent-computer/src/profiles.ts b/agent-computer/src/profiles.ts index 28f6547f..4e6fe26c 100644 --- a/agent-computer/src/profiles.ts +++ b/agent-computer/src/profiles.ts @@ -39,7 +39,8 @@ import { type BrowserContext, chromium, type Page } from "playwright"; import { profileDirectoryFor } from "./bot-id"; import { chooseEvictions, chooseIdle } from "./browser-eviction"; import { egressFor, egressLabel } from "./egress"; -import { numberFromEnv } from "./env"; +import { numberFromEnv, settleWithin } from "./env"; +import { chooseLivePage } from "./live-page"; import { botIdsIn } from "./profile-listing"; // Re-exported so callers that already import it from here do not change, while the test imports it @@ -120,6 +121,25 @@ console.info( */ const CLOSE_SETTLE_MS = 2_000; +/** + * How long telling a viewer its browser went away may take before the close carries on without it. + * + * The close is what has to happen; the announcement is courtesy. Unbounded, one screencast that will + * not stop would hold a browser close open, and because the cap evicts from inside another Bot's + * launch, it would hold that launch and everything queued behind it too. On the way out it would + * keep every profile from flushing until the container was killed. + */ +const ANNOUNCE_BUDGET_MS = 2_000; + +/** + * How long a stop or reset waits for a launch it is racing. + * + * Long enough for a cold Chromium start, which is what it is waiting for, and bounded so a launch + * that never finishes cannot hold a request open forever. Timing out leaves the browser running, + * which is the same answer the caller got before this waited at all. + */ +const LAUNCH_WAIT_MS = 30_000; + /** What a Bot's browser looks like from outside. */ export type BotBrowser = { botId: string; @@ -179,18 +199,36 @@ const IDLE_TIMEOUT_MS = numberFromEnv("COMPUTER_BROWSER_IDLE_MS", 30 * 60_000); /** How often the idle sweep looks. Cheap: it walks a map of at most `MAX_LIVE_BROWSERS`. */ const IDLE_SWEEP_MS = 60_000; -export function createProfiles(root: string) { +/** + * Told whenever a Bot's browser is closed and forgotten, before the close is waited on. + * + * Exists because closing a browser is not the whole of stopping a computer: anything still pointed + * at the page it was showing has to be taken down with it, and the live screen's follow loop calls + * back in for a page every second, which is a launch path. A viewer left running therefore relaunches + * whatever was just closed, so a stopped computer restarts itself and an idle one never goes away. + * + * It is announced here rather than called from the two request handlers because a browser closes + * from more places than those: the cap after a launch and the idle sweep close one without any + * request being involved. Hanging the teardown off the close itself covers those by construction, + * where remembering to add a call at each new handler does not. + * + * Awaited before the context closes, so a follow loop cannot squeeze a relaunch into the gap. + */ +export type BrowserClosed = (botId: string) => void | Promise; + +export function createProfiles(root: string, onClosed: BrowserClosed) { + type LiveBrowser = { + context: BrowserContext; + page: Page; + startedAt: string; + /** When this Bot last asked for its page. Decides what the cap and the sweep close. */ + usedAt: number; + /** Point `page` at whatever is open now. Called on every page opening and closing. */ + retarget: () => void; + }; + /** One running browser per Bot, up to {@link MAX_LIVE_BROWSERS}. */ - const live = new Map< - string, - { - context: BrowserContext; - page: Page; - startedAt: string; - /** When this Bot last asked for its page. Decides what the cap and the sweep close. */ - usedAt: number; - } - >(); + const live = new Map(); /** Launches in flight, so a cold computer is started once however many callers ask at once. */ const starting = new Map>(); @@ -205,14 +243,22 @@ export function createProfiles(root: string) { * Gracefully, so Chromium flushes the profile: the whole point of closing one is that the Bot's * logins survive and its next request starts where it left off. */ - const evict = async (botId: string, reason: string): Promise => { + const evict = async (botId: string, reason: string): Promise => { const running = live.get(botId); - if (!running) return; + if (!running) return false; live.delete(botId); console.info( JSON.stringify({ type: "computer-browser-closed", botId, reason }), ); + // Before the close, so whatever was watching this browser is taken down while it is still the + // browser being closed. A live screen surviving here relaunches it a second later. + // + // Bounded, because this now sits on the launch path: `enforceCap` evicts from inside another + // Bot's launch, so a teardown that never answers would pin that launch and every caller waiting + // on it. The close is the thing that must happen; being told about it is best effort. + await settleWithin(Promise.resolve(onClosed(botId)), ANNOUNCE_BUDGET_MS); await closeAndWait(running.context).catch(() => undefined); + return true; }; /** @@ -249,6 +295,29 @@ export function createProfiles(root: string) { // Housekeeping must not hold the process open on the way out. idleSweep.unref?.(); + /** + * Close a browser a person asked to close, including one that is still starting. + * + * `evict` only knows about browsers already in `live`, and a launch does not land there until it + * finishes. A request arriving inside that window therefore passed straight through: it answered + * "nothing was running", the launch completed a moment later, and the browser the person asked to + * close was up with the live screen still on it and the follow loop keeping it marked recently + * used, so the idle sweep would not reclaim it either. Reset was worse, deleting the profile + * directory that the finishing launch then recreated. + * + * So a request waits for the launch it is racing and closes what it produced. This is deliberately + * NOT inside `evict`: the cap evicts from inside a launch, and an `evict` that waited on `starting` + * could wait on the very launch it is running under. + */ + const closeOnRequest = async ( + botId: string, + reason: string, + ): Promise => { + // Its failure is the launch's own to report; here it only means there is nothing left to close. + await settleWithin(starting.get(botId), LAUNCH_WAIT_MS); + return evict(botId, reason); + }; + const sweepLocks = async (dir: string): Promise => { await Promise.all( SINGLETON_FILES.map((name) => @@ -274,6 +343,9 @@ export function createProfiles(root: string) { if (launching) return launching; const existing = live.get(botId); + // Asked before the page is judged, so a close that has not been delivered yet does not read as + // a browser that has gone. + existing?.retarget(); if ( existing?.context.browser()?.isConnected() && !existing.page.isClosed() @@ -285,6 +357,11 @@ export function createProfiles(root: string) { if (existing) { // Half-dead: the browser went away, or its page did. Dropped rather than repaired, because a // context whose browser has gone is not usable for anything. + // + // The one close that does not announce itself, deliberately. A replacement is launched on the + // next line and the live screen's follow loop re-attaches to it within the second, so this is + // a browser being swapped rather than one going away. Telling the viewer here would end a + // screen that is about to be fine, which is the opposite of what the announcement is for. await existing.context.close().catch(() => undefined); live.delete(botId); } @@ -309,16 +386,40 @@ export function createProfiles(root: string) { }); // Persistent contexts open with a page already; reuse it rather than leaving an extra blank tab. const page = context.pages()[0] ?? (await context.newPage()); - live.set(botId, { + const record: LiveBrowser = { context, page, startedAt: new Date().toISOString(), usedAt: Date.now(), + retarget: () => {}, + }; + record.retarget = () => { + const next = chooseLivePage(context.pages()); + if (!next || next === record.page) return; + record.page = next; + console.info( + JSON.stringify({ + type: "computer-page-changed", + botId, + url: next.url(), + }), + ); + }; + // Without this the Bot stays pinned to the page it launched with, so a sign-in the site opens + // in a new window is neither shown to the person taking the wheel nor reachable by input. + context.on("page", (opened) => { + record.retarget(); + // A popup closes itself when it succeeds, and the record must move back to the opener + // rather than leave a closed page to be read as a dead browser. + opened.on("close", () => record.retarget()); }); + page.on("close", () => record.retarget()); + live.set(botId, record); // After the new one is in the map, so the cap counts what is really running and the Bot that // just asked is the most recently used and therefore never the one closed. await enforceCap(); - return page; + // Not `page`: a window opened while the browser was starting is already the live one. + return record.page; })(); starting.set(botId, launch); @@ -332,17 +433,14 @@ export function createProfiles(root: string) { }, /** - * Close this Bot's browser without touching what it knows. + * Stop this Bot's browser, keeping what it knows. * - * Gracefully, so Chromium flushes its profile. This is what "kill" means for a Bot's computer: the - * browser stops, the login survives, and the next request starts it again where it left off. + * The same close the cap and the idle sweep make, so it goes through the same path rather than + * repeating it: a request is one more reason a browser closes, not a different kind of closing, + * and anything watching has to come down either way. */ async stop(botId: string): Promise { - const existing = live.get(botId); - if (!existing) return false; - live.delete(botId); - await closeAndWait(existing.context); - return true; + return closeOnRequest(botId, "it was stopped"); }, /** @@ -354,7 +452,8 @@ export function createProfiles(root: string) { * clean browser, which is the same path as a first ever start and so needs no second code path. */ async reset(botId: string): Promise { - await this.stop(botId); + // Its own reason rather than borrowing stop's, so the trail says which of the two happened. + await closeOnRequest(botId, "it was reset"); await rm(directoryFor(botId), { recursive: true, force: true }); }, @@ -398,9 +497,16 @@ export function createProfiles(root: string) { */ async closeAll(): Promise { clearInterval(idleSweep); - const contexts = [...live.values()]; + const entries = [...live.entries()]; live.clear(); - await Promise.all(contexts.map((c) => closeAndWait(c.context))); + // Told for each, the same as any other close. On the way out it changes nothing that survives, + // but a viewer whose socket outlives this by a moment is still owed the message. + await Promise.all( + entries.map(([botId]) => + settleWithin(Promise.resolve(onClosed(botId)), ANNOUNCE_BUDGET_MS), + ), + ); + await Promise.all(entries.map(([, c]) => closeAndWait(c.context))); }, /** How many browsers are running. For the idle sweep's own tests, and for a status reader. */ diff --git a/agent-computer/src/viewer.ts b/agent-computer/src/viewer.ts index 88470592..db43ae60 100644 --- a/agent-computer/src/viewer.ts +++ b/agent-computer/src/viewer.ts @@ -1,33 +1,240 @@ +import { settleWithin } from "./env"; +import type { Screencast } from "./screencast"; + +/** + * How long a cast gets to stop before the teardown moves on without it. + * + * Bounded because occupancy counts teardowns that are still running, and the session sweep reads + * occupancy. A `stop` that never settles would leave the entry in the set for the life of the + * process, so the Bot would read as watched forever and its session could never be swept, which is + * the unbounded growth that sweep exists to stop, arriving through the fix for it. + */ +const STOP_BUDGET_MS = 5_000; + /** - * Which socket is allowed to stop the live screen. + * Who owns a Bot's live screen, and what may still act on it. + * + * A Bot has one live screen at a time, and a second `/stream` replaces the first rather than being + * refused: a second cast on the same page would have Chrome encoding every frame twice with both + * sockets acking independently, which stalls both. One person drives, one cast. * - * A Bot's screen has one viewer, and a second `/stream` replaces the first rather than being - * refused: `open` stops whatever was casting and puts the new socket in the session. What it does - * not do is close the socket it replaced, because that socket belongs to a client that may still be - * using it. So the superseded socket closes on its own schedule, and on an ordinary reconnect, where - * a client opens the new connection before dropping the old one, that is after the replacement is - * already casting. + * What replacement does not do is close the socket it superseded, because that socket belongs to a + * client that may still be using it. So a superseded socket stays open and closes on its own + * schedule, which on an ordinary make-before-break reconnect is after the replacement is already + * casting. Anything that stops a viewer therefore has to establish that it owns the one it is + * stopping, and that is what this module exists to make unavoidable rather than remembered. * - * A `close` handler that stops the session's viewer without asking whether the closing socket is the - * one casting therefore stops the wrong viewer. The screen the person just reconnected to goes quiet, - * and their input is dropped without a word, because the input path checks for a viewer before it - * checks anything it could report. Both failures are silent; the browser is fine, the Bot is fine, - * and the person is looking at a still image. + * Ownership is held per socket, as a claim taken before the browser is asked for a page. That order + * is the load-bearing part. `open` awaits a page, which launches Chromium when nothing is running, + * and a close landing inside that window used to find nothing installed and so did nothing, while + * the launch went on to install a cast and a 1Hz follow loop for a socket that had already gone. No + * second close ever arrived. With the claim taken first there is always something to release, and + * everything the launch produces afterwards goes through that claim and is refused once it is + * revoked. * - * It lives in its own file for the reason `bot-id.ts` and `authorisation.ts` do: `index.ts` imports - * Playwright at module scope, so anything left in it needs Chrome merely to be imported by a test. - * The decision is here and the stopping stays there, the same split `browser-eviction.ts` makes. + * The refusals hand nothing back to the caller to clean up. A refused `install` stops the cast it was + * given and a refused `setFollow` cancels the loop it was given, because a caller that forgets leaks + * a screencast or a timer against a browser nobody is watching, and forgetting is exactly what this + * module is here to take off the table. For the same reason there is no way to stop a cast you do + * not own: `release` names a socket and does nothing when that socket owns nothing. + * + * It lives in its own file for the reason `browser-eviction.ts`, `authorisation.ts` and `bot-id.ts` + * do: `index.ts` imports Playwright at module scope, so anything left there needs Chrome merely to + * be imported by a test. `Screencast` comes in as a type only, which is erased at runtime, so this + * module and its tests stay free of the browser. Starting and stopping a cast stays in `index.ts`; + * the decisions are here. */ /** - * Is this socket the one currently casting? + * What a socket may do with the screen right now. * - * By identity, never by value. Two sockets are distinct objects however alike they look, and an - * equality that compared their contents would put the bug back for any pair that happened to match. + * Three answers, not two. A socket holding a claim with no cast yet is mid-launch and its screen is + * still opening; a socket holding nothing was superseded, closed, or never connected. Both own no + * cast, and collapsing them tells somebody whose screen is still starting that their session ended. + * The message is the only thing either of them gets, so it has to be the true one. */ -export function isCurrentViewer( - current: { socket: unknown } | undefined, - socket: unknown, -): boolean { - return current?.socket === socket; +export type ViewerStanding = + | { state: "casting"; cast: Screencast } + | { state: "starting" } + | { state: "gone" }; + +/** What an in-flight `open` may do with the screen it is starting. Refused once revoked. */ +export type ViewerClaim = { + /** + * Put a cast behind this claim, replacing one already there. + * + * Answers whether it was accepted. A refused cast is stopped here rather than returned, so a + * launch that lost its claim cannot leak one. The replaced cast stops only after the new one is + * installed, so the screen does not blank between the two while the Bot moves page to page. + */ + install(cast: Screencast): Promise; + /** Register the loop that keeps the cast on the Bot's current page. A refused loop is cancelled. */ + setFollow(cancel: () => void): boolean; +}; + +export type ViewerSlot = { + /** Take the screen for this socket, superseding and tearing down whoever held it. */ + claim(socket: unknown, notify: (reason: string) => void): ViewerClaim; + /** Give up the screen, if this socket is the one holding it. Its own close, so it is not told. */ + release(socket: unknown): Promise; + /** The browser went away. Tear down whoever is watching and tell them why. */ + releaseAll(reason: string): Promise; + /** What this socket may do with the screen right now. */ + standingOf(socket: unknown): ViewerStanding; + /** Is anybody watching, or about to be? What the session sweep asks. */ + occupied(): boolean; + /** Settle in-flight teardowns. A test seam; nothing on the acting path waits on this. */ + settled(): Promise; +}; + +/** What a person is told when somebody else takes the screen they were watching. */ +export const SUPERSEDED = + "This screen is now being watched somewhere else, so it stopped here."; + +type Entry = { + socket: unknown; + notify: (reason: string) => void; + cast?: Screencast; + cancelFollow?: () => void; + /** Set the moment the entry stops owning the screen, and never unset. The one source of truth. */ + revoked: boolean; +}; + +export function createViewerSlot(): ViewerSlot { + let current: Entry | undefined; + /* + * Teardowns still running. Occupancy counts them, because a browser whose cast is still stopping + * is not yet a Bot nobody is watching, and the session sweep reading otherwise would drop the + * control state out from under a screen that is mid-handover. + */ + const tearing = new Set>(); + + function track(work: Promise): void { + tearing.add(work); + void work.finally(() => tearing.delete(work)); + } + + /* + * Everything is best effort and nothing throws. The page can go away before its cast is told to + * stop, and the socket can be gone before we can tell it anything, and neither is a reason to + * leave the rest of the teardown undone: one dead page must not wedge the slot for every later + * connection. + */ + async function tearDown(entry: Entry, reason?: string): Promise { + if (reason !== undefined) { + try { + entry.notify(reason); + } catch { + // The socket is already gone. It cannot be told, and does not need to be. + } + } + if (entry.cancelFollow) { + try { + entry.cancelFollow(); + } catch { + // Cancelling a timer does not fail, but a caller's callback might. + } + entry.cancelFollow = undefined; + } + const cast = entry.cast; + entry.cast = undefined; + await settleWithin(cast?.stop(), STOP_BUDGET_MS); + } + + async function stopStray(cast: Screencast): Promise { + await settleWithin(cast.stop(), STOP_BUDGET_MS); + } + + return { + claim(socket, notify) { + const previous = current; + const entry: Entry = { socket, notify, revoked: false }; + /* + * The new claim owns the screen from here, before anything is awaited. A reconnect that + * arrives while the previous cast is still stopping must not be able to lose to it. + */ + current = entry; + if (previous) { + previous.revoked = true; + track(tearDown(previous, SUPERSEDED)); + } + + return { + async install(cast) { + if (entry.revoked) { + /* + * Tracked, not merely awaited. Occupancy and `settled` are how everything else learns + * that nothing is casting any more, and a cast stopped outside that accounting means + * both of them can answer "nothing" while Chrome is still encoding frames. + */ + const work = stopStray(cast); + track(work); + await work; + return false; + } + const replaced = entry.cast; + entry.cast = cast; + // After the replacement is running, so the screen does not go blank in between. + if (replaced) { + const work = stopStray(replaced); + track(work); + await work; + } + return true; + }, + setFollow(cancel) { + if (entry.revoked) { + try { + cancel(); + } catch { + // Same reason the teardown swallows it. + } + return false; + } + entry.cancelFollow?.(); + entry.cancelFollow = cancel; + return true; + }, + }; + }, + + async release(socket) { + const entry = current; + // Identity, never shape. Two sockets are distinct objects however alike they look, and an + // equality that compared their contents would hand the screen to any socket resembling the + // owner. This is also what makes a superseded socket's late close a no-op. + if (!entry || entry.socket !== socket) return; + current = undefined; + entry.revoked = true; + // Its own close, so nobody is told: the client already knows, and the socket is going away. + const work = tearDown(entry); + track(work); + await work; + }, + + async releaseAll(reason) { + const entry = current; + if (!entry) return; + current = undefined; + entry.revoked = true; + const work = tearDown(entry, reason); + track(work); + await work; + }, + + standingOf(socket) { + if (!current || current.socket !== socket) return { state: "gone" }; + return current.cast + ? { state: "casting", cast: current.cast } + : { state: "starting" }; + }, + + occupied() { + return current !== undefined || tearing.size > 0; + }, + + async settled() { + await Promise.all([...tearing]); + }, + }; } diff --git a/agent-computer/tests/browser-close-announcement.test.ts b/agent-computer/tests/browser-close-announcement.test.ts new file mode 100644 index 00000000..71c10075 --- /dev/null +++ b/agent-computer/tests/browser-close-announcement.test.ts @@ -0,0 +1,156 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { mkdtemp, rm, stat } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +/** + * Every way a Bot's browser closes tells whoever was watching it. + * + * `live-screen.test.ts` covers the two closes a request makes, and it cannot cover the other two: the + * cap closes a browser after somebody else's launch, and the idle sweep closes one on a timer, and + * neither is reachable by asking this process for anything. There is deliberately no endpoint that + * triggers them, so this drives `createProfiles` itself. + * + * They matter because a viewer that outlives a close keeps a loop asking for a page every second, + * and asking for a page starts a browser. A Bot with somebody watching was therefore immune to the + * idle timeout and came straight back after a cap eviction, which is the same failure the stop + * handler had, arriving by a route no handler is on. Hanging the announcement off the close itself + * is what covers these without anybody having to remember them. + * + * ASKED FOR BY NAME for the same reason as `live-screen.test.ts`: `profiles.ts` imports Playwright at + * module scope and CI does not install this directory's dependencies. + * + * bun run test:live-screen + */ + +const asked = process.env.OPENBOT_LIVE_SCREEN === "1"; + +let root = ""; +/* + * Both knobs are read when `profiles.ts` loads, so each case imports its own copy of the module and + * puts the environment back afterwards. Bun shares one module registry across every test file in a + * run and does not honour the order they are named on the command line, so a module imported without + * a fresh specifier is whatever an earlier file already loaded, and a knob left set leaks into + * whatever loads next. + */ +const IDLE = "COMPUTER_BROWSER_IDLE_MS"; +const CAP = "COMPUTER_MAX_BROWSERS"; +const before: Record = {}; + +beforeAll(async () => { + if (!asked) return; + before[IDLE] = process.env[IDLE]; + before[CAP] = process.env[CAP]; + root = await mkdtemp(join(tmpdir(), "agent-computer-closes-")); +}); + +afterAll(async () => { + if (!asked) return; + for (const name of [IDLE, CAP]) { + const original = before[name]; + if (original === undefined) delete process.env[name]; + else process.env[name] = original; + } + await rm(root, { recursive: true, force: true }); +}); + +describe.skipIf(!asked)( + "a browser closed by something nobody asked for", + () => { + test("the idle sweep tells whoever was watching", async () => { + // The shortest timeout the sweep will act on. Zero disables it, because a timeout of nothing + // means the feature is off rather than that everything is idle. + process.env.COMPUTER_BROWSER_IDLE_MS = "1"; + // Its own copy, for the reason given above the hooks. + const { createProfiles } = (await import( + `../src/profiles?idle=${Date.now()}` + )) as typeof import("../src/profiles"); + const told: string[] = []; + const profiles = createProfiles(join(root, "idle"), (botId) => { + told.push(botId); + }); + + await profiles.page("swept"); + expect(profiles.liveCount()).toBe(1); + + // Past the timeout, so the browser counts as idle rather than as just-used. + await new Promise((resolve) => setTimeout(resolve, 25)); + await profiles.sweepIdleNow(); + + expect(told).toEqual(["swept"]); + expect(profiles.liveCount()).toBe(0); + await profiles.closeAll(); + }, 60_000); + + test("a stop that lands while the browser is still starting still closes it", async () => { + // The window the request path fell through. `evict` only knows about browsers already running, + // and a launch does not land there until it finishes, so a stop arriving first answered + // "nothing was running" and left the browser up a moment later with the live screen still on + // it. Reset was worse, deleting the profile directory the finishing launch recreated. + process.env.COMPUTER_BROWSER_IDLE_MS = String(30 * 60_000); + const { createProfiles } = (await import( + `../src/profiles?racing=${Date.now()}` + )) as typeof import("../src/profiles"); + const told: string[] = []; + const profiles = createProfiles(join(root, "racing"), (botId) => { + told.push(botId); + }); + + // Deliberately not awaited: the stop is issued while the launch is still in flight. + const launching = profiles.page("racer"); + const stopped = await profiles.stop("racer"); + await launching.catch(() => undefined); + + expect(stopped).toBe(true); + expect(told).toEqual(["racer"]); + expect(profiles.liveCount()).toBe(0); + await profiles.closeAll(); + }, 60_000); + + test("a reset that lands while the browser is still starting wipes it for good", async () => { + // Reset had the same hole as stop and a worse consequence: it closed nothing, deleted the + // profile directory, and then the launch it never waited for finished and recreated the + // directory it had just wiped, leaving the Bot signed into everything it was meant to forget. + process.env.COMPUTER_BROWSER_IDLE_MS = String(30 * 60_000); + const { createProfiles } = (await import( + `../src/profiles?resetting=${Date.now()}` + )) as typeof import("../src/profiles"); + const told: string[] = []; + const root2 = join(root, "resetting"); + const profiles = createProfiles(root2, (botId) => { + told.push(botId); + }); + + // Not awaited: the reset is issued into the middle of the launch. + const launching = profiles.page("wiper"); + await profiles.reset("wiper"); + await launching.catch(() => undefined); + + expect(told).toEqual(["wiper"]); + expect(profiles.liveCount()).toBe(0); + // The directory stays gone rather than being recreated by the launch that finished after it. + await expect(stat(join(root2, "wiper"))).rejects.toThrow(); + await profiles.closeAll(); + }, 60_000); + + test("the cap tells the Bot whose browser it closed", async () => { + // One browser allowed, so the second Bot's launch is what closes the first Bot's browser. The + // person watching the first one never asked for anything and is owed the message just the same. + process.env.COMPUTER_BROWSER_IDLE_MS = String(30 * 60_000); + process.env.COMPUTER_MAX_BROWSERS = "1"; + const { createProfiles } = (await import( + `../src/profiles?cap=${Date.now()}` + )) as typeof import("../src/profiles"); + const told: string[] = []; + const profiles = createProfiles(join(root, "cap"), (botId) => { + told.push(botId); + }); + + await profiles.page("first"); + await profiles.page("second"); + + expect(told).toEqual(["first"]); + await profiles.closeAll(); + }, 60_000); + }, +); diff --git a/agent-computer/tests/follows-popup.test.ts b/agent-computer/tests/follows-popup.test.ts new file mode 100644 index 00000000..245757da --- /dev/null +++ b/agent-computer/tests/follows-popup.test.ts @@ -0,0 +1,84 @@ +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, describe, expect, test } from "bun:test"; + +/** + * Asked for by name, like the deployment journey at the repository root: it launches a real Chromium, + * which the machine running `bun test` is not required to have. The import is inside the test rather + * than at the top of the file for the same reason. + * + * cd agent-computer && bunx playwright install chromium + * OPENBOT_COMPUTER_BROWSER=1 bun test tests/follows-popup.test.ts + */ +const asked = process.env.OPENBOT_COMPUTER_BROWSER === "1"; +const LAUNCH_TIMEOUT_MS = 120_000; + +const html = (title: string, body: string) => + new Response(`${title}${body}`, { + headers: { "content-type": "text/html; charset=utf-8" }, + }); + +const site = Bun.serve({ + port: 0, + fetch(request) { + const { pathname } = new URL(request.url); + if (pathname === "/popup") return html("POPUP", "

the popup

"); + return html( + "OPENER", + ``, + ); + }, +}); +const origin = `http://127.0.0.1:${site.port}`; + +afterAll(() => { + site.stop(true); +}); + +/** Playwright delivers the opening and the closing as events, so the record moves a tick later. */ +async function settle(read: () => Promise, wanted: string) { + for (let attempt = 0; attempt < 50; attempt += 1) { + if ((await read()).includes(wanted)) return; + await new Promise((resolve) => setTimeout(resolve, 100)); + } +} + +describe.skipIf(!asked)("the page a Bot is on", () => { + test( + "follows a window the site opens, and comes back when it closes", + async () => { + const { createProfiles } = await import("../src/profiles"); + const root = await mkdtemp(join(tmpdir(), "openbot-profiles-")); + const profiles = createProfiles(root); + const bot = "popup-test"; + const url = async () => (await profiles.page(bot)).url(); + + try { + const opener = await profiles.page(bot); + await opener.goto(`${origin}/opener`, { + waitUntil: "domcontentloaded", + }); + expect(await url()).toContain("/opener"); + + await opener.click("#open"); + await settle(url, "/popup"); + // The defect this covers: the popup is open and rendering, and everything the Bot and the + // person taking the wheel can reach is still pointed at the page it launched with. + expect(await url()).toContain("/popup"); + + const popup = await profiles.page(bot); + await popup.close(); + await settle(url, "/opener"); + expect(await url()).toContain("/opener"); + // A sign-in popup closes itself when it succeeds, and the profile that just received it must + // still be there. + expect(opener.isClosed()).toBeFalse(); + } finally { + await profiles.stop(bot).catch(() => undefined); + await rm(root, { recursive: true, force: true }).catch(() => undefined); + } + }, + LAUNCH_TIMEOUT_MS, + ); +}); diff --git a/agent-computer/tests/live-page.test.ts b/agent-computer/tests/live-page.test.ts new file mode 100644 index 00000000..27cf5c94 --- /dev/null +++ b/agent-computer/tests/live-page.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, test } from "bun:test"; +import { chooseLivePage } from "../src/live-page"; + +// A popup OAuth sign-in is the case this exists for: the page a Bot was pinned to at launch is not +// the page the person taking the wheel needs to see, and the popup closes itself when it succeeds. +const page = (closed = false) => ({ isClosed: () => closed }); + +describe("choosing the page a Bot is on", () => { + test("one page is that page", () => { + const only = page(); + expect(chooseLivePage([only])).toBe(only); + }); + + test("a window the site opens becomes the live one", () => { + const opener = page(); + const popup = page(); + expect(chooseLivePage([opener, popup])).toBe(popup); + }); + + test("closing that window falls back to the opener", () => { + const opener = page(); + const popup = page(true); + expect(chooseLivePage([opener, popup])).toBe(opener); + }); + + test("a closed opener is not chosen while another page is open", () => { + const opener = page(true); + const popup = page(); + expect(chooseLivePage([opener, popup])).toBe(popup); + }); + + test("nothing open is nothing to choose", () => { + expect(chooseLivePage([page(true), page(true)])).toBeUndefined(); + expect(chooseLivePage([])).toBeUndefined(); + }); +}); diff --git a/agent-computer/tests/live-screen.test.ts b/agent-computer/tests/live-screen.test.ts new file mode 100644 index 00000000..ee306ce7 --- /dev/null +++ b/agent-computer/tests/live-screen.test.ts @@ -0,0 +1,447 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { createServer } from "node:net"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +/** + * The live screen, driven against the real process with a real browser. + * + * `viewer.test.ts` covers who owns the screen and what that lets them do, and it cannot cover + * whether the handlers ask. That gap is the whole of this bug: every failure here reads as correct + * in a unit test of the decision, because the decision was never the part that was wrong. So this + * one imports `index.ts`, opens real sockets against the port it listens on, and lets it launch + * Chromium. + * + * ASKED FOR BY NAME, like `tests/smoke/journey.test.ts`, and for a related reason. `index.ts` imports + * Playwright at module scope, `playwright` is declared only in this directory's own `package.json`, + * and CI installs the root workspaces plus the two Bots and never this one. An ungated file would + * therefore throw on import there, and `bun run test:ci` asserts a floor on the number of tests + * executed, so that failure reddens the build rather than skipping quietly. Reading the flag before + * the dynamic import below is what keeps the default suite honest on a machine where Playwright was + * never installed: + * + * bun run test:live-screen + * + * Everything here is timing against a browser that has to start, so the waits are generous. They are + * not the thing under test; what is under test is whether anything relaunches a browser nobody asked + * to start, and whether input reaches a page it does not belong to. + */ + +const asked = process.env.OPENBOT_LIVE_SCREEN === "1"; + +const TOKEN = "test-computer-token"; + +/** + * A port the operating system says is free, rather than one picked in advance. + * + * A fixed number here fails the whole file at import when anything else holds it, and reports as one + * broken test rather than as a port clash. 41641 in particular is Tailscale's default, so on a host + * running it this file could never have run at all. + */ +async function freePort(): Promise { + return new Promise((resolve, reject) => { + const probe = createServer(); + probe.on("error", reject); + probe.listen(0, "127.0.0.1", () => { + const found = (probe.address() as { port: number }).port; + probe.close(() => resolve(found)); + }); + }); +} + +let BASE = ""; +let WS = ""; + +/** + * How long "it did not come back" has to keep being true. + * + * A single check one tick later cannot tell a relaunch that never happened from one still in flight: + * `evict` reads only the browsers already running, and a launch is not one of those until it + * finishes, so both answer `wasRunning: false`. Holding the answer across several ticks and a whole + * cold start is what makes it mean the loop is gone rather than merely slow. + */ +const STAYS_STOPPED_MS = 9_000; + +/** Long enough for a close to land and the follow loop to tick once after it. */ +const AFTER_ONE_FOLLOW_TICK_MS = 1_600; +/** A cold Chromium launch here takes under two seconds; this leaves room on a slower machine. */ +const LAUNCH_MS = 8_000; + +/** A page that writes every key it receives into the body, so input that lands is readable back. */ +const TYPING_PAGE = + "data:text/html," + + encodeURIComponent( + "start", + ); + +let root = ""; +let closing: Array<() => void> = []; + +function api(path: string, botId: string, init?: RequestInit) { + return fetch(`${BASE}${path}`, { + ...init, + headers: { + "content-type": "application/json", + "x-openbot-bot-id": botId, + "x-openbot-computer-token": TOKEN, + ...(init?.headers ?? {}), + }, + }); +} + +type Frames = { + socket: WebSocket; + /** Every error the server sent this socket, in order. */ + errors: string[]; + /** + * Resolves when the connection is up, which is when the server's `open` handler starts running. + * + * The cold-launch window opens here, not when the socket is constructed. A socket closed before it + * has connected never reaches the handler at all, so nothing is ever stranded and the failure this + * file exists to catch cannot happen. Waiting for this is what puts the close inside the launch. + */ + connected: Promise; + /** Resolves once a frame of the page has arrived, which means this socket is the one casting. */ + casting: Promise; + close: () => void; +}; + +function watch(botId: string): Frames { + const socket = new WebSocket(`${WS}/stream?bot=${botId}&token=${TOKEN}`); + const errors: string[] = []; + let sawFrame = () => {}; + const casting = new Promise((resolve) => { + sawFrame = resolve; + }); + let opened = () => {}; + const connected = new Promise((resolve) => { + opened = resolve; + }); + socket.addEventListener("open", () => opened()); + socket.addEventListener("message", (event) => { + const message = JSON.parse(String(event.data)) as { + type?: string; + error?: string; + }; + if (message.type === "frame") sawFrame(); + if (message.type === "error") errors.push(message.error ?? ""); + }); + const close = () => { + try { + socket.close(); + } catch { + // Already gone. + } + }; + closing.push(close); + return { socket, errors, connected, casting, close }; +} + +const wait = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +/** Wait for something to become true, rather than sleeping a guessed amount and hoping. */ +async function until( + what: () => boolean, + budgetMs: number, + why: string, + refresh?: () => Promise, +): Promise { + const deadline = Date.now() + budgetMs; + while (Date.now() < deadline) { + await refresh?.(); + if (what()) return; + await wait(25); + } + throw new Error(`Timed out waiting for ${why}`); +} + +/** Keep asking, and fail the moment the browser comes back rather than only at the end. */ +async function staysStopped(botId: string): Promise { + const deadline = Date.now() + STAYS_STOPPED_MS; + while (Date.now() < deadline) { + if (await stopped(botId)) { + throw new Error( + "Something started the browser again after it was stopped", + ); + } + await wait(250); + } +} + +async function stopped(botId: string): Promise { + const response = await api("/computers/stop", botId, { method: "POST" }); + const body = (await response.json()) as { wasRunning?: boolean }; + return body.wasRunning === true; +} + +beforeAll(async () => { + if (!asked) return; + root = await mkdtemp(join(tmpdir(), "agent-computer-live-screen-")); + const port = await freePort(); + BASE = `http://127.0.0.1:${port}`; + WS = `ws://127.0.0.1:${port}`; + process.env.COMPUTER_TOKEN = TOKEN; + process.env.PORT = String(port); + process.env.PROFILES_DIR = join(root, "profiles"); + process.env.WORKSPACE_DIR = join(root, "workspace"); + await mkdir(join(root, "profiles"), { recursive: true }); + // After the environment is set, because the module reads it while it loads, and behind the flag, + // because this is the import that needs Playwright present. + await import("../src/index"); +}); + +afterAll(async () => { + if (!asked) return; + for (const close of closing) close(); + closing = []; + // Every browser this file started, so the rest of the suite does not inherit a stray Chromium. + for (const botId of [ + "cold-close", + "supersede", + "late-close", + "wheel", + "stop-viewer", + "reset-viewer", + "wont-launch", + "still-starting", + ]) { + await api("/computers/stop", botId, { method: "POST" }).catch( + () => undefined, + ); + } + await rm(root, { recursive: true, force: true }); + // Generous, because it is stopping real browsers: the default hook budget is shorter than a + // Chromium shutdown and the file would fail on its own cleanup rather than on anything it tested. +}, 60_000); + +describe.skipIf(!asked)( + "a socket that closes while the browser is starting", + () => { + test("leaves nothing behind that starts the browser again", async () => { + // Failure 1, and the observable is deliberately not "is there a viewer": it is whether anything + // relaunches Chromium after somebody stopped it. The orphaned follow interval called + // `currentPage` every second, which is a launch path, so a stopped computer came back up. + const botId = "cold-close"; + const viewer = watch(botId); + // Connected first, so the server's `open` is running and awaiting a page, then closed straight + // away. That is the window: closing before the connection is up never reaches the handler. + await viewer.connected; + viewer.close(); + + await wait(LAUNCH_MS); + // The launch really did produce a browser. Without this the whole case passes vacuously on a + // machine where the launch failed or is still going: nothing was cast, no follow loop existed, + // and the orphaned interval this exists to catch was never created. + expect(await stopped(botId)).toBe(true); + + await staysStopped(botId); + }, 30_000); + }, +); + +describe.skipIf(!asked)("a socket that another connection replaced", () => { + test("cannot type into the screen that replaced it, and is told so", async () => { + // Failure 3. The input handler dispatched through whatever the session held, so the replaced + // window's keys went into the page the current viewer was watching, and the sender heard nothing + // because the old check returned before reaching anything that could report. + const botId = "supersede"; + await api("/navigate", botId, { + method: "POST", + body: JSON.stringify({ url: TYPING_PAGE }), + }); + + const first = watch(botId); + await first.casting; + const second = watch(botId); + await second.casting; + + await api("/control/take", botId, { method: "POST" }); + first.socket.send(JSON.stringify({ type: "key", key: "z" })); + + // The exact refusal, not merely some error. Dispatching through a cast the sender does not own + // also fails, and fails loudly, so "an error arrived" passes just as well when the ownership + // check is gone. Only the wording separates being refused from blundering into a null. + await until( + () => first.errors.some((e) => /no longer live/i.test(e)), + 5_000, + "the replaced socket to be told its screen is no longer live", + ); + + const read = await api("/read", botId); + const { text } = (await read.json()) as { text: string }; + expect(text).not.toContain("z"); + }, 30_000); +}); + +describe.skipIf(!asked)("a superseded socket closing later", () => { + test("does not take the screen down with it", async () => { + // What #191 fixed, at the level where it can actually go wrong again. Replacement does not close + // the socket it superseded, so that socket closes on its own schedule, which on an ordinary + // make-before-break reconnect is after the replacement is already casting. A close that stopped + // whatever the slot held rather than naming its own socket would leave the person who just + // reconnected watching a still image with input going nowhere, and nothing would say so. + const botId = "late-close"; + await api("/navigate", botId, { + method: "POST", + body: JSON.stringify({ url: TYPING_PAGE }), + }); + + const first = watch(botId); + await first.casting; + const second = watch(botId); + await second.casting; + + // The replaced socket goes away now, after its replacement is live. + first.close(); + await wait(AFTER_ONE_FOLLOW_TICK_MS); + + // The survivor still owns the screen, and the proof is that its typing arrives: a cast that was + // stopped underneath it, or an ownership it quietly lost, would refuse this instead. + await api("/control/take", botId, { method: "POST" }); + second.socket.send(JSON.stringify({ type: "key", key: "k" })); + + let landed = ""; + await until( + () => landed.includes("k"), + 5_000, + "the surviving viewer's key to reach the page", + async () => { + const read = await api("/read", botId); + landed = ((await read.json()) as { text: string }).text; + }, + ); + + expect(second.errors).toEqual([]); + }, 30_000); +}); + +describe.skipIf(!asked)( + "the wheel, with the ownership check in front of it", + () => { + test("still refuses the casting socket while the Bot holds it", async () => { + // The control half of the reordering. The superseded case above runs with a person already + // holding the wheel, so the control check is passive there and a rewiring that dropped it would + // still pass. Here the socket genuinely owns the screen and nobody has taken control, which is + // the only arrangement where that check is the one doing the refusing. + const botId = "wheel"; + await api("/navigate", botId, { + method: "POST", + body: JSON.stringify({ url: TYPING_PAGE }), + }); + await api("/control/release", botId, { method: "POST" }); + + const viewer = watch(botId); + await viewer.casting; + + viewer.socket.send(JSON.stringify({ type: "key", key: "q" })); + + await until( + () => viewer.errors.length > 0, + 5_000, + "the owner to be told to take control first", + ); + expect(viewer.errors.some((e) => /control/i.test(e))).toBe(true); + + const read = await api("/read", botId); + const { text } = (await read.json()) as { text: string }; + expect(text).not.toContain("q"); + }, 30_000); + }, +); + +describe.skipIf(!asked)("stopping the computer out from under a viewer", () => { + test("takes the screen down and does not come back", async () => { + // Failure 2. Stopping released the wheel and left the viewer alone, so the follow loop's next + // tick asked for a page, which starts a browser, and the computer somebody had just stopped was + // running again a second later, refreshing its own idle timestamp every tick while it did. + const botId = "stop-viewer"; + const viewer = watch(botId); + await viewer.casting; + + expect(await stopped(botId)).toBe(true); + + await until( + () => viewer.errors.some((e) => /stopped/i.test(e)), + 5_000, + "the viewer to be told the computer stopped", + ); + + await staysStopped(botId); + }, 30_000); + + test("the same holds when the computer is reset rather than stopped", async () => { + // Reset wipes the profile as well, and had the identical hole: it released the wheel and never + // touched the viewer. Its own response carries no `wasRunning`, so the relaunch is observed + // through a following stop rather than through what reset itself answers. + const botId = "reset-viewer"; + const viewer = watch(botId); + await viewer.casting; + + await api("/computers/reset", botId, { method: "POST" }); + + await until( + () => viewer.errors.some((e) => /stopped/i.test(e)), + 5_000, + "the viewer to be told its computer went away", + ); + + await staysStopped(botId); + }, 30_000); +}); + +describe.skipIf(!asked)("typing into a screen that is still opening", () => { + test("is told the screen is starting, not that it ended", async () => { + // The reason there are three standings rather than two. A socket mid-launch owns a claim and no + // cast, exactly like a socket that was superseded, and answering both the same way tells somebody + // whose screen is seconds from live that their session is over. + const botId = "still-starting"; + const viewer = watch(botId); + // Only connected, deliberately: the browser is still starting, so no cast exists yet. + await viewer.connected; + viewer.socket.send(JSON.stringify({ type: "key", key: "s" })); + + await until( + () => viewer.errors.length > 0, + LAUNCH_MS, + "the socket to be answered while its screen is still starting", + ); + + expect(viewer.errors[0]).toMatch(/still starting/i); + }, 40_000); +}); + +describe.skipIf(!asked)("a screen whose browser will not start", () => { + test("says so and leaves nothing holding the session", async () => { + // `open`'s catch. A file sits where this Bot's profile directory would go, so Chromium cannot + // launch and `currentPage` throws while the claim is already held. The socket is told and closed, + // which is the observable part; the claim being released with it is what keeps the session + // sweepable, and `viewer.test.ts` pins that half because a leaked claim changes nothing a caller + // outside this process can see until the map has grown. + const botId = "wont-launch"; + await writeFile(join(root, "profiles", botId), "not a directory"); + + const viewer = watch(botId); + await viewer.connected; + + await until( + () => viewer.errors.length > 0, + 15_000, + "the socket to be told its screen could not be started", + ); + + // What it says is Playwright's to word, so this pins only that the socket was told why its + // screen never arrived rather than handed one of the lifecycle refusals, which would mean the + // failure had been reported as somebody else taking the screen. + expect(viewer.errors[0]).not.toMatch( + /no longer live|still starting|computer stopped/i, + ); + + // Closed by the handler rather than left open against a browser that does not exist. + await until( + () => viewer.socket.readyState === WebSocket.CLOSED, + 5_000, + "the socket to be closed after the failure", + ); + }, 40_000); +}); diff --git a/agent-computer/tests/viewer.test.ts b/agent-computer/tests/viewer.test.ts index 87f95003..73ce2b77 100644 --- a/agent-computer/tests/viewer.test.ts +++ b/agent-computer/tests/viewer.test.ts @@ -1,41 +1,540 @@ import { describe, expect, test } from "bun:test"; -import { isCurrentViewer } from "../src/viewer"; +import type { Screencast } from "../src/screencast"; +import { createViewerSlot, SUPERSEDED } from "../src/viewer"; /** - * Which socket is allowed to stop the live screen. + * Who owns the live screen, and what may act on it. * - * One viewer per Bot, and a second `/stream` replaces the first: `open` stops whatever was casting - * and puts the new socket in the session. The old socket is not closed by that, so its `close` - * arrives whenever the client gets round to it, which on an ordinary make-before-break reconnect is - * after the replacement is already running. A close that stops the current viewer without asking - * whether it owns it stops the wrong one, and the person who just reconnected gets a screen that - * never updates and input that goes nowhere. + * A Bot has one live screen, and a second `/stream` replaces the first rather than being refused. The + * replaced socket is not closed by that, so it stays open and closes whenever its client gets round + * to it, which on an ordinary make-before-break reconnect is after the replacement is already + * casting. Everything that goes wrong here goes wrong because something acted on the session's + * viewer without establishing that it owned the one it was acting on. * - * The decision rather than the stopping. Stopping a cast is Playwright's job and is not where the - * wrong answer was; `browser-eviction.ts` splits the same way and for the same reason. + * So ownership is the thing under test, and it is held per socket. A claim is taken before the + * browser is asked for a page, which is what lets a close that lands during a cold launch be honored + * at all: there is something to release even before there is a cast. Everything the launch produces + * afterwards goes through that claim and is refused once it is revoked. + * + * The decisions rather than the stopping. Starting and stopping a cast is Playwright's job and is not + * where the wrong answers were; `browser-eviction.ts` splits the same way and for the same reason. + * These tests use fake casts, so nothing here needs Chrome. */ -describe("deciding whether a closing socket stops the live screen", () => { - const socket = { id: "a" }; - const other = { id: "b" }; - test("the socket that is casting stops it", () => { - expect(isCurrentViewer({ socket }, socket)).toBe(true); +/** A cast that records what was asked of it. Enough of `Screencast` for every decision below. */ +function fakeCast(): Screencast & { stops: number; sent: unknown[] } { + const cast = { + stops: 0, + sent: [] as unknown[], + async stop() { + cast.stops += 1; + }, + async send(message: unknown) { + cast.sent.push(message); + }, + }; + return cast as Screencast & { stops: number; sent: unknown[] }; +} + +/** A cast whose stop hangs until it is let go, so a teardown can be observed mid-flight. */ +function pendingCast(): Screencast & { finish: () => void; stops: number } { + let release = () => {}; + const stopped = new Promise((resolve) => { + release = resolve; + }); + const cast = { + stops: 0, + async stop() { + cast.stops += 1; + await stopped; + }, + async send() {}, + finish: () => release(), + }; + return cast as Screencast & { finish: () => void; stops: number }; +} + +/** A cast whose stop rejects. The teardown paths must survive one. */ +function brokenCast(): Screencast { + return { + async stop() { + throw new Error("the page went away first"); + }, + async send() {}, + } as Screencast; +} + +function recorder() { + const said: string[] = []; + return { said, notify: (reason: string) => said.push(reason) }; +} + +describe("taking the live screen", () => { + test("a claim owns the screen before any cast exists", async () => { + // The whole point of claiming before the browser is asked for a page. Until this, a close during + // a cold launch found nothing to release and the launch installed a cast for a socket that had + // already gone. + const slot = createViewerSlot(); + const socket = { id: "a" }; + + slot.claim(socket, () => {}); + + expect(slot.occupied()).toBe(true); + expect(slot.standingOf(socket).state).toBe("starting"); + }); + + test("installing a cast makes that socket the one casting", async () => { + const slot = createViewerSlot(); + const socket = { id: "a" }; + const cast = fakeCast(); + + const claim = slot.claim(socket, () => {}); + expect(await claim.install(cast)).toBe(true); + + const standing = slot.standingOf(socket); + expect(standing.state).toBe("casting"); + expect(standing.state === "casting" && standing.cast).toBe(cast); + }); + + test("a socket that never claimed owns nothing", () => { + const slot = createViewerSlot(); + + expect(slot.standingOf({ id: "nobody" }).state).toBe("gone"); + expect(slot.occupied()).toBe(false); + }); + + test("identity, not shape, when asked what a socket owns", async () => { + // Two sockets are never equal by value, and comparing them that way would hand the screen to any + // socket that happened to look like the owner. + const slot = createViewerSlot(); + const claim = slot.claim({ id: "a" }, () => {}); + await claim.install(fakeCast()); + + expect(slot.standingOf({ id: "a" }).state).toBe("gone"); + }); + + test("identity, not shape, when a socket gives the screen up", async () => { + // The same rule on the release path, and it needs two sockets that look alike to catch: a + // comparison by value would let a stranger's close stop the owner's cast, which is the original + // bug wearing different clothes. Distinct objects, deliberately identical contents. + const slot = createViewerSlot(); + const owner = { id: "same" }; + const twin = { id: "same" }; + const cast = fakeCast(); + const claim = slot.claim(owner, () => {}); + await claim.install(cast); + + await slot.release(twin); + + expect(cast.stops).toBe(0); + expect(slot.standingOf(owner).state).toBe("casting"); + }); +}); + +describe("a close that lands while the browser is still starting", () => { + test("the cast the launch produces is refused and stopped, not installed", async () => { + // Failure 1. `open` awaits a page, which launches Chromium when nothing is running, and a socket + // that closes inside that window used to leave a cast and a 1Hz interval behind for a socket that + // was already gone. Nothing arrived later to stop them, and the interval went on relaunching the + // browser after somebody stopped it. + const slot = createViewerSlot(); + const socket = { id: "a" }; + const claim = slot.claim(socket, () => {}); + + await slot.release(socket); + + const late = fakeCast(); + expect(await claim.install(late)).toBe(false); + // Refused and stopped by the slot, so a caller cannot leak a cast by forgetting to. + expect(late.stops).toBe(1); + expect(slot.occupied()).toBe(false); + expect(slot.standingOf(socket).state).toBe("gone"); + }); + + test("a follow loop registered after the close is refused", async () => { + const slot = createViewerSlot(); + const socket = { id: "a" }; + const claim = slot.claim(socket, () => {}); + + await slot.release(socket); + + expect(claim.setFollow(() => {})).toBe(false); + }); + + test("the claim is released even when the launch threw", async () => { + // `open`'s catch closes the socket after sending an error frame. If nothing released the claim + // there, the slot would stay occupied forever and `forgetIdleSessions` could never sweep the + // session, which is the unbounded growth it exists to stop. + const slot = createViewerSlot(); + const socket = { id: "a" }; + slot.claim(socket, () => {}); + + await slot.release(socket); + + expect(slot.occupied()).toBe(false); + }); + + test("releasing the same socket twice is harmless", async () => { + // The catch path closes the socket, so `close` runs release a second time for the same socket. + const slot = createViewerSlot(); + const socket = { id: "a" }; + const claim = slot.claim(socket, () => {}); + const cast = fakeCast(); + await claim.install(cast); + + await slot.release(socket); + await slot.release(socket); + + expect(cast.stops).toBe(1); + expect(slot.occupied()).toBe(false); + }); + + test("a socket closing its own screen is not told about it", async () => { + // Only supersession and the browser going away are news. A client that closed its own socket + // already knows, and the socket is on its way out anyway, so telling it is at best a write to + // something that is gone. + const slot = createViewerSlot(); + const socket = { id: "a" }; + const heard = recorder(); + const claim = slot.claim(socket, heard.notify); + await claim.install(fakeCast()); + + await slot.release(socket); + + expect(heard.said).toEqual([]); + }); + + test("releasing a socket that owns nothing leaves the owner alone", async () => { + // The superseded socket's close, arriving after its replacement is already casting. Stopping the + // session's viewer without asking who owns it is what made the reconnected screen go quiet. + const slot = createViewerSlot(); + const owner = { id: "owner" }; + const stranger = { id: "stranger" }; + const cast = fakeCast(); + const claim = slot.claim(owner, () => {}); + await claim.install(cast); + + await slot.release(stranger); + + expect(cast.stops).toBe(0); + expect(slot.standingOf(owner).state).toBe("casting"); + expect(slot.occupied()).toBe(true); + }); +}); + +describe("a second connection taking over", () => { + test("the replaced cast and its follow loop are torn down", async () => { + const slot = createViewerSlot(); + const first = { id: "first" }; + const firstCast = fakeCast(); + let cancelled = 0; + + const firstClaim = slot.claim(first, () => {}); + await firstClaim.install(firstCast); + firstClaim.setFollow(() => { + cancelled += 1; + }); + + slot.claim({ id: "second" }, () => {}); + await slot.settled(); + + expect(firstCast.stops).toBe(1); + expect(cancelled).toBe(1); + }); + + test("the replaced socket is told, and is not closed by us", async () => { + // The socket belongs to a client that may still be using it, so replacement does not close it. + // Telling it is the whole reason the notify callback exists: otherwise its screen freezes on the + // last frame and its input goes nowhere without a word. + const slot = createViewerSlot(); + const heard = recorder(); + + const firstClaim = slot.claim({ id: "first" }, heard.notify); + await firstClaim.install(fakeCast()); + + slot.claim({ id: "second" }, () => {}); + await slot.settled(); + + // The exact message, not merely that something was said. This is the only thing a replaced + // viewer is ever told, and "some non-empty string" stays green if it becomes the stop message, + // the no-longer-live message, or a stray debug line. + expect(heard.said).toEqual([SUPERSEDED]); + }); + + test("the replaced claim can no longer install or follow", async () => { + // Failure 4, as the rule that removes it rather than as an observation of it. Two opens that + // interleave used to let the older one assign itself over the newer one after its awaits, leaving + // an interval nothing held. This is the module's rule under test, not the running process. + const slot = createViewerSlot(); + const firstClaim = slot.claim({ id: "first" }, () => {}); + + slot.claim({ id: "second" }, () => {}); + await slot.settled(); + + const late = fakeCast(); + expect(await firstClaim.install(late)).toBe(false); + expect(late.stops).toBe(1); + expect(firstClaim.setFollow(() => {})).toBe(false); + }); + + test("the new socket is the one casting afterwards", async () => { + const slot = createViewerSlot(); + const second = { id: "second" }; + const secondCast = fakeCast(); + + const firstClaim = slot.claim({ id: "first" }, () => {}); + await firstClaim.install(fakeCast()); + + const secondClaim = slot.claim(second, () => {}); + await secondClaim.install(secondCast); + await slot.settled(); + + const standing = slot.standingOf(second); + expect(standing.state).toBe("casting"); + expect(standing.state === "casting" && standing.cast).toBe(secondCast); }); +}); + +describe("following the page the Bot moves to", () => { + test("a replacement cast supersedes the one before it", async () => { + // The 1Hz loop re-attaches when the Bot opens a different page. The new cast has to be running + // before the old one stops, or the screen blanks between the two. + const slot = createViewerSlot(); + const socket = { id: "a" }; + const first = fakeCast(); + const second = fakeCast(); + + const claim = slot.claim(socket, () => {}); + await claim.install(first); + await claim.install(second); + + expect(first.stops).toBe(1); + expect(second.stops).toBe(0); + const standing = slot.standingOf(socket); + expect(standing.state === "casting" && standing.cast).toBe(second); + }); + + test("replacing the follow loop cancels the one it replaces", async () => { + const slot = createViewerSlot(); + let firstCancelled = 0; + const claim = slot.claim({ id: "a" }, () => {}); + await claim.install(fakeCast()); + + claim.setFollow(() => { + firstCancelled += 1; + }); + claim.setFollow(() => {}); + + expect(firstCancelled).toBe(1); + }); +}); + +describe("accounting for a cast the slot refused", () => { + test("a refused cast is stopped before the slot reports itself settled", async () => { + // Occupancy and `settled` are how the sweep and the tests learn that nothing is casting. A cast + // stopped outside that accounting lets both answer "nothing" while Chrome is still encoding, so + // the refusal has to be part of the teardown rather than beside it. + const slot = createViewerSlot(); + const socket = { id: "a" }; + const claim = slot.claim(socket, () => {}); + await slot.release(socket); + + const late = pendingCast(); + const refusal = claim.install(late); + + expect(slot.occupied()).toBe(true); + + late.finish(); + expect(await refusal).toBe(false); + await slot.settled(); + + expect(slot.occupied()).toBe(false); + }); +}); - test("a socket that was replaced stops nothing", () => { - // The bug this exists for. The old socket closes after the new one has taken over, and without - // this the new viewer is the one that gets stopped. - expect(isCurrentViewer({ socket: other }, socket)).toBe(false); +describe("the browser closing under the viewer", () => { + test("everything is torn down and the watcher is told", async () => { + // Failure 2 and the eviction paths behind it. A browser that closes while a viewer is alive gets + // relaunched a second later by the follow tick, so a stopped computer restarts itself and an + // idle one never goes away. + const slot = createViewerSlot(); + const heard = recorder(); + const cast = fakeCast(); + let cancelled = 0; + + const claim = slot.claim({ id: "a" }, heard.notify); + await claim.install(cast); + claim.setFollow(() => { + cancelled += 1; + }); + + await slot.releaseAll("the computer was stopped"); + + expect(cast.stops).toBe(1); + expect(cancelled).toBe(1); + expect(heard.said).toEqual(["the computer was stopped"]); + expect(slot.occupied()).toBe(false); }); - test("a close with no viewer at all stops nothing", () => { - // Both sockets gone, or the cast never started. There is nothing to stop and nothing to get wrong. - expect(isCurrentViewer(undefined, socket)).toBe(false); + test("a viewer still starting is torn down too", async () => { + // The claim exists before the cast does, and a browser that closes inside that window has to + // revoke it, or the launch still in flight installs a cast onto a browser that is gone. + const slot = createViewerSlot(); + const socket = { id: "a" }; + const claim = slot.claim(socket, () => {}); + + await slot.releaseAll("the computer was stopped"); + + const late = fakeCast(); + expect(await claim.install(late)).toBe(false); + expect(late.stops).toBe(1); + expect(slot.occupied()).toBe(false); + }); + + test("releasing an empty slot tells nobody and does nothing", async () => { + const slot = createViewerSlot(); + + await slot.releaseAll("the computer was stopped"); + + expect(slot.occupied()).toBe(false); + }); +}); + +describe("what a socket may do with the screen right now", () => { + test("the casting socket is handed its own cast", async () => { + const slot = createViewerSlot(); + const socket = { id: "a" }; + const cast = fakeCast(); + const claim = slot.claim(socket, () => {}); + await claim.install(cast); + + const standing = slot.standingOf(socket); + + expect(standing.state === "casting" && standing.cast).toBe(cast); + }); + + test("a socket whose screen is still starting is told apart from one that is gone", async () => { + // Both own no cast, and answering them the same way tells somebody whose screen is still opening + // that their session ended. The message is the only thing they get, so it has to be the true one. + const slot = createViewerSlot(); + const starting = { id: "starting" }; + const gone = { id: "gone" }; + slot.claim(starting, () => {}); + + expect(slot.standingOf(starting).state).toBe("starting"); + expect(slot.standingOf(gone).state).toBe("gone"); }); - test("identity, not shape", () => { - // Two sockets are never equal by value, and comparing them that way would put the bug back for - // any pair that happened to look alike. - expect(isCurrentViewer({ socket: { id: "a" } }, { id: "a" })).toBe(false); + test("a superseded socket is gone, not starting", async () => { + const slot = createViewerSlot(); + const first = { id: "first" }; + slot.claim(first, () => {}); + + slot.claim({ id: "second" }, () => {}); + await slot.settled(); + + expect(slot.standingOf(first).state).toBe("gone"); + }); +}); + +describe("the session sweep asking whether anybody is watching", () => { + test("a Bot is watched from the claim, not from the first frame", async () => { + // `forgetIdleSessions` drops sessions with nobody watching and no live browser. Reading occupancy + // from an installed cast would call a Bot unwatched for the whole cold launch, and sweeping then + // would drop the control state out from under the person who is about to be watching it. + const slot = createViewerSlot(); + slot.claim({ id: "a" }, () => {}); + + expect(slot.occupied()).toBe(true); + }); + + test("a Bot stays watched until teardown has finished", async () => { + const slot = createViewerSlot(); + const socket = { id: "a" }; + const claim = slot.claim(socket, () => {}); + await claim.install(fakeCast()); + + const releasing = slot.release(socket); + await releasing; + + expect(slot.occupied()).toBe(false); + }); + + test("a Bot is still watched while its cast is stopping", async () => { + // Occupancy has to count the teardown itself, not just whether somebody holds the slot. The + // sweep drops sessions with nobody watching and no live browser, and a session dropped while its + // cast is still stopping takes the control state with it. + const slot = createViewerSlot(); + const socket = { id: "a" }; + const cast = pendingCast(); + const claim = slot.claim(socket, () => {}); + await claim.install(cast); + + const releasing = slot.release(socket); + await Promise.resolve(); + + expect(slot.occupied()).toBe(true); + + cast.finish(); + await releasing; + + expect(slot.occupied()).toBe(false); + }); + + test("a claim taken while a teardown is in flight keeps the Bot watched", async () => { + // The reconnect that arrives before the old cast has finished stopping. Clearing occupancy when + // the older teardown completes would report nobody watching while somebody is. + const slot = createViewerSlot(); + const first = { id: "first" }; + const claim = slot.claim(first, () => {}); + await claim.install(fakeCast()); + + const releasing = slot.release(first); + slot.claim({ id: "second" }, () => {}); + await releasing; + + expect(slot.occupied()).toBe(true); + }); +}); + +describe("a cast that cannot be stopped", () => { + test("a rejecting stop does not break a takeover", async () => { + // The page can go away before the cast is told to stop. `attach` already swallows this; teardown + // has to as well, or one dead page leaves the slot wedged for every later connection. + const slot = createViewerSlot(); + const firstClaim = slot.claim({ id: "first" }, () => {}); + await firstClaim.install(brokenCast()); + + slot.claim({ id: "second" }, () => {}); + await slot.settled(); + + expect(slot.standingOf({ id: "first" }).state).toBe("gone"); + }); + + test("a rejecting stop does not break releasing the browser", async () => { + const slot = createViewerSlot(); + const socket = { id: "a" }; + const claim = slot.claim(socket, () => {}); + await claim.install(brokenCast()); + + await slot.releaseAll("the computer was stopped"); + + expect(slot.occupied()).toBe(false); + }); + + test("a notify that throws does not stop the teardown", async () => { + // The socket may already be gone when we try to tell it. Sending is best effort; the teardown is + // not. + const slot = createViewerSlot(); + const cast = fakeCast(); + const claim = slot.claim({ id: "a" }, () => { + throw new Error("that socket is closed"); + }); + await claim.install(cast); + + await slot.releaseAll("the computer was stopped"); + + expect(cast.stops).toBe(1); + expect(slot.occupied()).toBe(false); }); }); diff --git a/docs/development.md b/docs/development.md index 3c84c575..b2c92f72 100644 --- a/docs/development.md +++ b/docs/development.md @@ -119,6 +119,22 @@ suite cannot reach: server to supervisor to computer, the gateway deciding befor and the audit row landing. Point it elsewhere with `OPENBOT_API_URL`. Without a deployment it is skipped by `bun run test` and says what to start when asked for by name. +`bun run test:live-screen` is separate for a related reason and needs no deployment, only this +directory's own dependencies: + +```sh +cd agent-computer && bun install +cd .. && bun run test:live-screen +``` + +It drives the live screen against the real computer process with a real Chromium: a socket closing +while the browser is still starting, a second connection taking the screen from the first, the wheel +refusing input from the socket that owns it, and a browser closing by request or by the idle sweep. +Those need `agent-computer/src/index.ts`, which imports Playwright at module scope, and `playwright` +is declared only in `agent-computer/package.json`, which `bun install` at the root does not reach. So +without `OPENBOT_LIVE_SCREEN=1` the files skip before importing anything, which is what keeps +`bun run test` and CI working where that dependency was never installed. + ## Contribution checklist - Keep changes focused. diff --git a/package.json b/package.json index 72472d8b..84c4ee66 100644 --- a/package.json +++ b/package.json @@ -21,6 +21,7 @@ "test:ci": "bun scripts/test-ci.ts", "pretest": "bun run generate:app-config", "test:smoke": "OPENBOT_SMOKE=1 bun test tests/smoke", + "test:live-screen": "OPENBOT_LIVE_SCREEN=1 bun test agent-computer/tests/live-screen.test.ts agent-computer/tests/browser-close-announcement.test.ts", "diagram": "bun scripts/architecture-diagram.ts", "mock:knowledge": "bun scripts/mock-knowledge-mcp.ts" },