From 9dd4e3f6795f2ff18090b30a5358326a35346d72 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Thu, 27 Aug 2026 23:51:15 -0500 Subject: [PATCH 1/8] Hold a Bot's live screen per socket rather than per Bot A Bot has one live screen and a second connection replaces the first, but the replaced socket is not closed by that: it stays open and closes whenever its client gets round to it, which on an ordinary reconnect is after the replacement is already casting. So anything that stops a viewer has to establish that it owns the one it is stopping, and today only the close handler asks. The input handler, the two computer handlers and the follow loop's own attach all reach for whatever the session holds. This adds the slot that makes ownership something a caller cannot skip. A claim is taken for the socket before the browser is asked for a page, which is what lets a close landing during a cold launch be honoured at all: there is something to release before there is a cast, and the cast the launch produces afterwards is refused. A refused install stops the cast it was given and a refused follow loop is cancelled, because a caller that forgets leaks a screencast or a 1Hz timer against a browser nobody is watching. Three answers rather than two when asked what a socket may do, because a claim with no cast yet is a screen still opening and not a screen that ended, and telling somebody the wrong one of those is the whole reason the notification exists. Occupancy counts a teardown that is still running, so the session sweep cannot drop a Bot mid-handover. The predicate stays for now; index.ts moves onto the slot next, and doing that in one change rather than two keeps every commit building. No Playwright: Screencast arrives as a type, which is erased, so this and its tests run without Chrome. Each rule is covered both ways, and the suite was checked against seventeen deliberate breakages of the module to confirm none of it passes vacuously. --- agent-computer/src/viewer.ts | 237 ++++++++++++- agent-computer/tests/viewer.test.ts | 524 +++++++++++++++++++++++++++- 2 files changed, 728 insertions(+), 33 deletions(-) diff --git a/agent-computer/src/viewer.ts b/agent-computer/src/viewer.ts index 88470592..2c1ed651 100644 --- a/agent-computer/src/viewer.ts +++ b/agent-computer/src/viewer.ts @@ -1,22 +1,37 @@ +import type { Screencast } from "./screencast"; + /** - * Which socket is allowed to stop the live screen. - * - * 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. - * - * 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. - * - * 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. + * 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. + * + * 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. + * + * 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. + * + * 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. */ /** @@ -24,6 +39,9 @@ * * 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. + * + * The slot below supersedes this: it answers the same question without a caller having to remember + * to ask. It stays until `index.ts` moves onto the slot, so that move is one change rather than two. */ export function isCurrentViewer( current: { socket: unknown } | undefined, @@ -31,3 +49,186 @@ export function isCurrentViewer( ): boolean { return current?.socket === socket; } + +/** + * What a socket may do with the screen right now. + * + * 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 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. For tests and shutdown, never on the acting path. */ + 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 cast?.stop().catch(() => undefined); + } + + async function stopStray(cast: Screencast): Promise { + await cast.stop().catch(() => undefined); + } + + 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) { + await stopStray(cast); + 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) await stopStray(replaced); + 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/viewer.test.ts b/agent-computer/tests/viewer.test.ts index 87f95003..341dd6d5 100644 --- a/agent-computer/tests/viewer.test.ts +++ b/agent-computer/tests/viewer.test.ts @@ -1,20 +1,477 @@ import { describe, expect, test } from "bun:test"; -import { isCurrentViewer } from "../src/viewer"; +import type { Screencast } from "../src/screencast"; +import { createViewerSlot, isCurrentViewer } 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. */ + +/** 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 } { + let release = () => {}; + const stopped = new Promise((resolve) => { + release = resolve; + }); + return { + async stop() { + await stopped; + }, + async send() {}, + finish: () => release(), + } as Screencast & { finish: () => void }; +} + +/** 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(); + + expect(heard.said).toHaveLength(1); + expect(heard.said[0]).toMatch(/./); + }); + + 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("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 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("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("deciding whether a closing socket stops the live screen", () => { + // The predicate the slot supersedes. It is still what `index.ts` asks until that moves across, so + // it stays under test until it goes. const socket = { id: "a" }; const other = { id: "b" }; @@ -23,19 +480,56 @@ describe("deciding whether a closing socket stops the live screen", () => { }); 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); }); 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("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); }); }); + +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); + }); +}); From efde1561ba35a75accff1d4f6ea5ecb62ee9bd7f Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:05:05 -0500 Subject: [PATCH 2/8] Ask which socket is acting before acting on its behalf The handlers reached for whatever the session held. Only the close handler asked whether the socket in front of it owned the screen, and the input handler, the sweep and the follow loop's own attach did not, which is three call sites of a question that was only ever answered at one. They go through the slot now, so the question is not one a caller can skip. The claim is taken before the browser is asked for a page: a close landing during a cold launch releases it, and the cast that launch produces afterwards is refused and stopped rather than installed for a socket that has gone. That was leaving a screencast and a 1Hz interval behind, and the interval called currentPage every second, which is a launch path, so a computer somebody had stopped came back up on its own. Input is answered rather than dropped. A superseded socket's typing used to dispatch through the session's cast and land in the page the current viewer was watching, and its sender heard nothing because the old check returned before reaching anything that could report. A socket that is still starting and a socket that is gone are told different things, because telling somebody whose screen is opening that it ended is a worse answer than none. The wheel is untouched and still refuses: it stands after the ownership question rather than instead of it, since the two refuse different things and owning the screen was never permission to drive it. The real-process tests are asked for by name, bun run test:live-screen, the way tests/smoke does. index.ts imports Playwright at module scope and CI installs the root workspaces and the two Bots but not this directory, so an ungated file would throw on import there and redden the build through the test-count floor. Reading the flag before the dynamic import keeps the default suite honest where Playwright was never installed, which was confirmed by running it with the package removed. Both failures were watched failing against the old handlers first: the stopped computer restarting, and the key from a replaced window arriving in the live one. The wheel case and the superseded close cannot fail that way, since both are existing behaviour, so each was confirmed to bite by breaking the check it guards and watching it go red. --- agent-computer/src/index.ts | 132 ++++++---- agent-computer/tests/live-screen.test.ts | 294 +++++++++++++++++++++++ package.json | 1 + 3 files changed, 384 insertions(+), 43 deletions(-) create mode 100644 agent-computer/tests/live-screen.test.ts diff --git a/agent-computer/src/index.ts b/agent-computer/src/index.ts index 5d7177a6..b3e3341d 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,13 @@ 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; - }; + /** + * 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 +129,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 +147,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(); @@ -330,19 +336,13 @@ 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 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 +363,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 +399,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 +430,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 +464,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 +495,9 @@ 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. + await sessionFor(ws.data.botId).viewer.release(ws); }, }, async fetch(request, server) { diff --git a/agent-computer/tests/live-screen.test.ts b/agent-computer/tests/live-screen.test.ts new file mode 100644 index 00000000..308e974c --- /dev/null +++ b/agent-computer/tests/live-screen.test.ts @@ -0,0 +1,294 @@ +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 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"; +const PORT = 41641; +const BASE = `http://127.0.0.1:${PORT}`; +const WS = `ws://127.0.0.1:${PORT}`; + +/** Comfortably past the 1Hz follow loop, so a tick that would relaunch has had its chance. */ +const PAST_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}`); +} + +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-")); + process.env.COMPUTER_TOKEN = TOKEN; + process.env.PORT = String(PORT); + process.env.PROFILES_DIR = join(root, "profiles"); + process.env.WORKSPACE_DIR = join(root, "workspace"); + // 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"]) { + 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); + await stopped(botId); + await wait(PAST_ONE_FOLLOW_TICK_MS); + + expect(await stopped(botId)).toBe(false); + }, 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(PAST_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); + }, +); diff --git a/package.json b/package.json index 72472d8b..839582d8 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", "diagram": "bun scripts/architecture-diagram.ts", "mock:knowledge": "bun scripts/mock-knowledge-mcp.ts" }, From e6b33b7606dd997649f9690ca8f7eb841a3bc505 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:14:04 -0500 Subject: [PATCH 3/8] Take the live screen down with the browser it was showing Stopping a computer released the wheel and left the viewer running, so the follow loop asked for a page a second later, which starts a browser, and the computer somebody had just stopped was up again. Reset had the same hole. Both are fixed by not putting the teardown in either handler. A browser closes from four places, and only two of them are requests: the cap closes one after somebody else's launch and the idle sweep closes one on a timer, neither with any request involved. A viewer surviving those kept the same loop asking for a page, so a Bot with somebody watching was immune to the idle timeout and came straight back after a cap eviction, which is the same failure arriving by a route no handler is on. So the close announces itself and the screen comes down with it. stop now goes through the same eviction path as the cap and the sweep, because a request is one more reason a browser closes rather than a different kind of closing. profiles still knows nothing about viewers: it says a browser went away and the module that owns the screen decides what that means. The announcement is awaited before the context closes, so a follow tick cannot fit a relaunch into the gap. The two request paths are covered against the real process. The cap and the sweep are reachable from no endpoint, and deliberately so, so they are driven through createProfiles rather than by adding a way in that only a test would use. Each observation was made both ways: the stopped computer restarting itself was watched failing first, and removing the announcement afterwards was confirmed to redden all four paths, while making stop close a browser its own way reddened the two request cases and left the other two green, which is what says the request path really does share the close rather than repeating it. --- agent-computer/src/index.ts | 22 ++++- agent-computer/src/profiles.ts | 57 +++++++++--- .../tests/browser-close-announcement.test.ts | 86 +++++++++++++++++++ agent-computer/tests/live-screen.test.ts | 52 ++++++++++- package.json | 2 +- 5 files changed, 205 insertions(+), 14 deletions(-) create mode 100644 agent-computer/tests/browser-close-announcement.test.ts diff --git a/agent-computer/src/index.ts b/agent-computer/src/index.ts index b3e3341d..0001ff5d 100644 --- a/agent-computer/src/index.ts +++ b/agent-computer/src/index.ts @@ -193,7 +193,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"); @@ -336,6 +352,10 @@ function json(body: unknown, status = 200): Response { }); } +/** 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."; diff --git a/agent-computer/src/profiles.ts b/agent-computer/src/profiles.ts index 28f6547f..fa111c45 100644 --- a/agent-computer/src/profiles.ts +++ b/agent-computer/src/profiles.ts @@ -179,7 +179,27 @@ 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 = () => {}, +) { /** One running browser per Bot, up to {@link MAX_LIVE_BROWSERS}. */ const live = new Map< string, @@ -205,14 +225,18 @@ 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. + await Promise.resolve(onClosed(botId)).catch(() => undefined); await closeAndWait(running.context).catch(() => undefined); + return true; }; /** @@ -337,12 +361,15 @@ export function createProfiles(root: string) { * 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. */ + /** + * Stop this Bot's browser, keeping what it knows. + * + * 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 evict(botId, "it was stopped"); }, /** @@ -354,7 +381,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 evict(botId, "it was reset"); await rm(directoryFor(botId), { recursive: true, force: true }); }, @@ -398,9 +426,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]) => + Promise.resolve(onClosed(botId)).catch(() => undefined), + ), + ); + 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/tests/browser-close-announcement.test.ts b/agent-computer/tests/browser-close-announcement.test.ts new file mode 100644 index 00000000..13773801 --- /dev/null +++ b/agent-computer/tests/browser-close-announcement.test.ts @@ -0,0 +1,86 @@ +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"; + +/** + * 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 = ""; + +beforeAll(async () => { + if (!asked) return; + root = await mkdtemp(join(tmpdir(), "agent-computer-closes-")); +}); + +afterAll(async () => { + if (!asked) return; + 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"; + const { createProfiles } = await 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("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"; + // A fresh module registry, because the cap and the timeout are read when the module loads. + 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/live-screen.test.ts b/agent-computer/tests/live-screen.test.ts index 308e974c..3361d542 100644 --- a/agent-computer/tests/live-screen.test.ts +++ b/agent-computer/tests/live-screen.test.ts @@ -150,7 +150,14 @@ afterAll(async () => { 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"]) { + for (const botId of [ + "cold-close", + "supersede", + "late-close", + "wheel", + "stop-viewer", + "reset-viewer", + ]) { await api("/computers/stop", botId, { method: "POST" }).catch( () => undefined, ); @@ -292,3 +299,46 @@ describe.skipIf(!asked)( }, 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 wait(PAST_ONE_FOLLOW_TICK_MS); + + // Nothing restarted it in the meantime, which is the whole observable. + expect(await stopped(botId)).toBe(false); + }, 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 wait(PAST_ONE_FOLLOW_TICK_MS); + + expect(await stopped(botId)).toBe(false); + }, 30_000); +}); diff --git a/package.json b/package.json index 839582d8..84c4ee66 100644 --- a/package.json +++ b/package.json @@ -21,7 +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", + "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" }, From 25915996aefef8d6fcd2a5a8723a2b236a5ae515 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:16:18 -0500 Subject: [PATCH 4/8] Drop the predicate the slot replaced It had one caller and the slot took that over, so it is dead now. Kept through the previous change only so index.ts moved across in one commit rather than leaving the tree unbuildable between two. Also says why one browser close stays silent. profiles announces a close so the screen watching it comes down, but the half-dead path closes a context whose browser has already gone and launches a replacement on the next line, and the follow loop re-attaches to it within the second. Telling the viewer there would end a screen that is about to be fine, which is the opposite of what the announcement is for. That is the only close of the four that does not fire it, and a reader checking coverage should not have to work out whether it was forgotten. --- agent-computer/src/profiles.ts | 5 +++++ agent-computer/src/viewer.ts | 16 ---------------- agent-computer/tests/viewer.test.ts | 25 +------------------------ 3 files changed, 6 insertions(+), 40 deletions(-) diff --git a/agent-computer/src/profiles.ts b/agent-computer/src/profiles.ts index fa111c45..af5eb795 100644 --- a/agent-computer/src/profiles.ts +++ b/agent-computer/src/profiles.ts @@ -309,6 +309,11 @@ export function createProfiles( 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); } diff --git a/agent-computer/src/viewer.ts b/agent-computer/src/viewer.ts index 2c1ed651..0aee3e14 100644 --- a/agent-computer/src/viewer.ts +++ b/agent-computer/src/viewer.ts @@ -34,22 +34,6 @@ import type { Screencast } from "./screencast"; * the decisions are here. */ -/** - * Is this socket the one currently casting? - * - * 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. - * - * The slot below supersedes this: it answers the same question without a caller having to remember - * to ask. It stays until `index.ts` moves onto the slot, so that move is one change rather than two. - */ -export function isCurrentViewer( - current: { socket: unknown } | undefined, - socket: unknown, -): boolean { - return current?.socket === socket; -} - /** * What a socket may do with the screen right now. * diff --git a/agent-computer/tests/viewer.test.ts b/agent-computer/tests/viewer.test.ts index 341dd6d5..1df97531 100644 --- a/agent-computer/tests/viewer.test.ts +++ b/agent-computer/tests/viewer.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import type { Screencast } from "../src/screencast"; -import { createViewerSlot, isCurrentViewer } from "../src/viewer"; +import { createViewerSlot } from "../src/viewer"; /** * Who owns the live screen, and what may act on it. @@ -469,29 +469,6 @@ describe("the session sweep asking whether anybody is watching", () => { }); }); -describe("deciding whether a closing socket stops the live screen", () => { - // The predicate the slot supersedes. It is still what `index.ts` asks until that moves across, so - // it stays under test until it goes. - const socket = { id: "a" }; - const other = { id: "b" }; - - test("the socket that is casting stops it", () => { - expect(isCurrentViewer({ socket }, socket)).toBe(true); - }); - - test("a socket that was replaced stops nothing", () => { - expect(isCurrentViewer({ socket: other }, socket)).toBe(false); - }); - - test("a close with no viewer at all stops nothing", () => { - expect(isCurrentViewer(undefined, socket)).toBe(false); - }); - - test("identity, not shape", () => { - expect(isCurrentViewer({ socket: { id: "a" } }, { id: "a" })).toBe(false); - }); -}); - 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 From 2feb3fc7c99384c2b2676a53a22f92accd80584f Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:20:07 -0500 Subject: [PATCH 5/8] Drive the launch that fails, not just the ones that work open's catch was the one path in this change with no coverage against the real process. A file where the profile directory would go makes Chromium refuse to start, so currentPage throws with the claim already held, which is the arrangement the catch exists for and the one the cold-launch timing makes reachable. What it pins is that the socket is told and closed rather than left waiting on a browser that will never arrive, and that the Bot is still usable afterwards. The claim being released with it stays covered by viewer.test.ts, because a leaked claim changes nothing observable from outside this process until the session map has grown, and reaching that state through the port would prove less than the unit test already does. --- agent-computer/tests/live-screen.test.ts | 35 +++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/agent-computer/tests/live-screen.test.ts b/agent-computer/tests/live-screen.test.ts index 3361d542..a22c6077 100644 --- a/agent-computer/tests/live-screen.test.ts +++ b/agent-computer/tests/live-screen.test.ts @@ -1,5 +1,5 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test"; -import { mkdtemp, rm } from "node:fs/promises"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -140,6 +140,7 @@ beforeAll(async () => { 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"); @@ -157,6 +158,7 @@ afterAll(async () => { "wheel", "stop-viewer", "reset-viewer", + "wont-launch", ]) { await api("/computers/stop", botId, { method: "POST" }).catch( () => undefined, @@ -342,3 +344,34 @@ describe.skipIf(!asked)("stopping the computer out from under a viewer", () => { expect(await stopped(botId)).toBe(false); }, 30_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", + ); + + // 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", + ); + + // The Bot is still usable afterwards: a failed launch must not wedge it. + expect(await stopped(botId)).toBe(false); + }, 40_000); +}); From 27ed03c860ff2374b19d116ababcb6c14338bf8a Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:56:15 -0500 Subject: [PATCH 6/8] Close a browser somebody asked to close, even one still starting Review found the request path falling through the mechanism the rest of this branch added. Everything hangs off closing a browser that is running, and a launch is not one of those until it finishes, so a stop or reset arriving inside a cold start answered "nothing was running" and left the browser up a moment later with the live screen still on it, its follow loop keeping it marked recently used so the idle sweep would not reclaim it either. Reset was worse: it deleted the profile directory that the finishing launch recreated, leaving the Bot signed into what it was told to forget. A request now waits for the launch it is racing. Deliberately not inside the eviction itself, because the cap evicts from inside a launch and would then be able to wait on the launch it is running under. The waits that were unbounded are bounded now. Telling a viewer its browser went away sits on another Bot's launch path, and stopping a cast decides whether a session can ever be swept, so a screencast that never answers used to pin a launch, keep a session occupied for the life of the process, and on the way out hold every profile's flush until the container was killed. Smaller things the same pass turned up: a refused cast was stopped outside the teardown accounting, so occupancy could report nothing casting while Chrome was still encoding; the close handler created a session for a Bot that had none, which the comment three lines above it argues against; the announcement callback was optional at the one call site the fix depends on; and a stale doc block described a body that had moved. Four tests were weaker than they read. The cold-close case never established that a browser had started, so it would have passed on a machine where the launch failed. "It did not come back" was one check after a fixed sleep, which cannot tell a relaunch that never happened from one still in flight, so it is held across a whole cold start now. The superseded message was asserted as any non-empty string. And the branch that tells somebody their screen is still opening, the whole reason there are three standings rather than two, had no test at all. The port was fixed at Tailscale's default, so on a host running it this file could never have started; it asks the operating system for a free one now. The announcement tests leaked their environment knobs and passed on the alphabetical order of the files, since Bun shares one module registry across a run and ignores the order they are named in. docs/development.md documents the lane beside test:smoke, including that it needs this directory's own install. --- agent-computer/src/env.ts | 33 ++++++ agent-computer/src/index.ts | 6 +- agent-computer/src/profiles.ts | 67 ++++++++--- agent-computer/src/viewer.ts | 32 +++++- .../tests/browser-close-announcement.test.ts | 76 ++++++++++++- agent-computer/tests/live-screen.test.ts | 106 +++++++++++++++--- agent-computer/tests/viewer.test.ts | 40 ++++++- docs/development.md | 16 +++ 8 files changed, 328 insertions(+), 48 deletions(-) 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 0001ff5d..8fae8102 100644 --- a/agent-computer/src/index.ts +++ b/agent-computer/src/index.ts @@ -517,7 +517,11 @@ serve({ async close(ws) { // 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. - await sessionFor(ws.data.botId).viewer.release(ws); + // + // `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/profiles.ts b/agent-computer/src/profiles.ts index af5eb795..5b92a097 100644 --- a/agent-computer/src/profiles.ts +++ b/agent-computer/src/profiles.ts @@ -39,7 +39,7 @@ 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 { botIdsIn } from "./profile-listing"; // Re-exported so callers that already import it from here do not change, while the test imports it @@ -120,6 +120,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; @@ -196,10 +215,7 @@ const IDLE_SWEEP_MS = 60_000; */ export type BrowserClosed = (botId: string) => void | Promise; -export function createProfiles( - root: string, - onClosed: BrowserClosed = () => {}, -) { +export function createProfiles(root: string, onClosed: BrowserClosed) { /** One running browser per Bot, up to {@link MAX_LIVE_BROWSERS}. */ const live = new Map< string, @@ -234,7 +250,11 @@ export function createProfiles( ); // 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. - await Promise.resolve(onClosed(botId)).catch(() => undefined); + // + // 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; }; @@ -273,6 +293,29 @@ export function createProfiles( // 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) => @@ -360,12 +403,6 @@ export function createProfiles( } }, - /** - * Close this Bot's browser without touching 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. - */ /** * Stop this Bot's browser, keeping what it knows. * @@ -374,7 +411,7 @@ export function createProfiles( * and anything watching has to come down either way. */ async stop(botId: string): Promise { - return evict(botId, "it was stopped"); + return closeOnRequest(botId, "it was stopped"); }, /** @@ -387,7 +424,7 @@ export function createProfiles( */ async reset(botId: string): Promise { // Its own reason rather than borrowing stop's, so the trail says which of the two happened. - await evict(botId, "it was reset"); + await closeOnRequest(botId, "it was reset"); await rm(directoryFor(botId), { recursive: true, force: true }); }, @@ -437,7 +474,7 @@ export function createProfiles( // but a viewer whose socket outlives this by a moment is still owed the message. await Promise.all( entries.map(([botId]) => - Promise.resolve(onClosed(botId)).catch(() => undefined), + settleWithin(Promise.resolve(onClosed(botId)), ANNOUNCE_BUDGET_MS), ), ); await Promise.all(entries.map(([, c]) => closeAndWait(c.context))); diff --git a/agent-computer/src/viewer.ts b/agent-computer/src/viewer.ts index 0aee3e14..db43ae60 100644 --- a/agent-computer/src/viewer.ts +++ b/agent-computer/src/viewer.ts @@ -1,5 +1,16 @@ +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; + /** * Who owns a Bot's live screen, and what may still act on it. * @@ -72,7 +83,7 @@ export type ViewerSlot = { standingOf(socket: unknown): ViewerStanding; /** Is anybody watching, or about to be? What the session sweep asks. */ occupied(): boolean; - /** Settle in-flight teardowns. For tests and shutdown, never on the acting path. */ + /** Settle in-flight teardowns. A test seam; nothing on the acting path waits on this. */ settled(): Promise; }; @@ -127,11 +138,11 @@ export function createViewerSlot(): ViewerSlot { } const cast = entry.cast; entry.cast = undefined; - await cast?.stop().catch(() => undefined); + await settleWithin(cast?.stop(), STOP_BUDGET_MS); } async function stopStray(cast: Screencast): Promise { - await cast.stop().catch(() => undefined); + await settleWithin(cast.stop(), STOP_BUDGET_MS); } return { @@ -151,13 +162,24 @@ export function createViewerSlot(): ViewerSlot { return { async install(cast) { if (entry.revoked) { - await stopStray(cast); + /* + * 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) await stopStray(replaced); + if (replaced) { + const work = stopStray(replaced); + track(work); + await work; + } return true; }, setFollow(cancel) { diff --git a/agent-computer/tests/browser-close-announcement.test.ts b/agent-computer/tests/browser-close-announcement.test.ts index 13773801..71c10075 100644 --- a/agent-computer/tests/browser-close-announcement.test.ts +++ b/agent-computer/tests/browser-close-announcement.test.ts @@ -1,5 +1,5 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test"; -import { mkdtemp, rm } from "node:fs/promises"; +import { mkdtemp, rm, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -26,14 +26,31 @@ import { join } from "node:path"; 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 }); }); @@ -44,7 +61,10 @@ describe.skipIf(!asked)( // 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"; - const { createProfiles } = await import("../src/profiles"); + // 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); @@ -62,12 +82,62 @@ describe.skipIf(!asked)( 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"; - // A fresh module registry, because the cap and the timeout are read when the module loads. const { createProfiles } = (await import( `../src/profiles?cap=${Date.now()}` )) as typeof import("../src/profiles"); diff --git a/agent-computer/tests/live-screen.test.ts b/agent-computer/tests/live-screen.test.ts index a22c6077..ee306ce7 100644 --- a/agent-computer/tests/live-screen.test.ts +++ b/agent-computer/tests/live-screen.test.ts @@ -1,5 +1,6 @@ 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"; @@ -30,12 +31,40 @@ import { join } from "node:path"; const asked = process.env.OPENBOT_LIVE_SCREEN === "1"; const TOKEN = "test-computer-token"; -const PORT = 41641; -const BASE = `http://127.0.0.1:${PORT}`; -const WS = `ws://127.0.0.1:${PORT}`; -/** Comfortably past the 1Hz follow loop, so a tick that would relaunch has had its chance. */ -const PAST_ONE_FOLLOW_TICK_MS = 1_600; +/** + * 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; @@ -127,6 +156,19 @@ async function until( 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 }; @@ -136,8 +178,11 @@ async function stopped(botId: string): Promise { 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.PORT = String(port); process.env.PROFILES_DIR = join(root, "profiles"); process.env.WORKSPACE_DIR = join(root, "workspace"); await mkdir(join(root, "profiles"), { recursive: true }); @@ -159,6 +204,7 @@ afterAll(async () => { "stop-viewer", "reset-viewer", "wont-launch", + "still-starting", ]) { await api("/computers/stop", botId, { method: "POST" }).catch( () => undefined, @@ -184,10 +230,12 @@ describe.skipIf(!asked)( viewer.close(); await wait(LAUNCH_MS); - await stopped(botId); - await wait(PAST_ONE_FOLLOW_TICK_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); - expect(await stopped(botId)).toBe(false); + await staysStopped(botId); }, 30_000); }, ); @@ -246,7 +294,7 @@ describe.skipIf(!asked)("a superseded socket closing later", () => { // The replaced socket goes away now, after its replacement is live. first.close(); - await wait(PAST_ONE_FOLLOW_TICK_MS); + 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. @@ -318,10 +366,8 @@ describe.skipIf(!asked)("stopping the computer out from under a viewer", () => { 5_000, "the viewer to be told the computer stopped", ); - await wait(PAST_ONE_FOLLOW_TICK_MS); - // Nothing restarted it in the meantime, which is the whole observable. - expect(await stopped(botId)).toBe(false); + await staysStopped(botId); }, 30_000); test("the same holds when the computer is reset rather than stopped", async () => { @@ -339,12 +385,32 @@ describe.skipIf(!asked)("stopping the computer out from under a viewer", () => { 5_000, "the viewer to be told its computer went away", ); - await wait(PAST_ONE_FOLLOW_TICK_MS); - expect(await stopped(botId)).toBe(false); + 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 @@ -364,14 +430,18 @@ describe.skipIf(!asked)("a screen whose browser will not start", () => { "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", ); - - // The Bot is still usable afterwards: a failed launch must not wedge it. - expect(await stopped(botId)).toBe(false); }, 40_000); }); diff --git a/agent-computer/tests/viewer.test.ts b/agent-computer/tests/viewer.test.ts index 1df97531..73ce2b77 100644 --- a/agent-computer/tests/viewer.test.ts +++ b/agent-computer/tests/viewer.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import type { Screencast } from "../src/screencast"; -import { createViewerSlot } from "../src/viewer"; +import { createViewerSlot, SUPERSEDED } from "../src/viewer"; /** * Who owns the live screen, and what may act on it. @@ -37,18 +37,21 @@ function fakeCast(): 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 } { +function pendingCast(): Screencast & { finish: () => void; stops: number } { let release = () => {}; const stopped = new Promise((resolve) => { release = resolve; }); - return { + const cast = { + stops: 0, async stop() { + cast.stops += 1; await stopped; }, async send() {}, finish: () => release(), - } as Screencast & { finish: () => void }; + }; + return cast as Screencast & { finish: () => void; stops: number }; } /** A cast whose stop rejects. The teardown paths must survive one. */ @@ -252,8 +255,10 @@ describe("a second connection taking over", () => { slot.claim({ id: "second" }, () => {}); await slot.settled(); - expect(heard.said).toHaveLength(1); - expect(heard.said[0]).toMatch(/./); + // 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 () => { @@ -324,6 +329,29 @@ describe("following the page the Bot moves to", () => { }); }); +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); + }); +}); + 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 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. From 0e73d8b432e9835fce203755840cbca049ce476d Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Fri, 28 Aug 2026 08:42:55 -0500 Subject: [PATCH 7/8] Say what changes for somebody running a Bot people watch The checklist in docs/development.md asks for this when a deployment behaves differently afterwards, and it does: a computer that was stopped stays stopped, a watched Bot stops being immune to the idle timeout and the cap, and a viewer whose screen ends is told why. It also says plainly that the app does not show that message yet, so nobody reads this and expects to see it on screen. --- CHANGELOG.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6eb5d41b..348b1a9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,30 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### 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 From 742230fe005ef3634898b69ed8c06c2c38ee4b07 Mon Sep 17 00:00:00 2001 From: Vaibhav Zope <121456155+zopeVaibhav@users.noreply.github.com> Date: Fri, 28 Aug 2026 22:23:49 +0530 Subject: [PATCH 8/8] Follow the window a site opens, and come back when it closes (#286) --- CHANGELOG.md | 16 +++++ agent-computer/src/index.ts | 10 ++- agent-computer/src/live-page.ts | 16 +++++ agent-computer/src/profiles.ts | 52 ++++++++++---- agent-computer/tests/follows-popup.test.ts | 84 ++++++++++++++++++++++ agent-computer/tests/live-page.test.ts | 36 ++++++++++ 6 files changed, 201 insertions(+), 13 deletions(-) create mode 100644 agent-computer/src/live-page.ts create mode 100644 agent-computer/tests/follows-popup.test.ts create mode 100644 agent-computer/tests/live-page.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6eb5d41b..f62c6d7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,22 @@ 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. + ## 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/index.ts b/agent-computer/src/index.ts index 5d7177a6..6e6d4ea5 100644 --- a/agent-computer/src/index.ts +++ b/agent-computer/src/index.ts @@ -112,6 +112,8 @@ type BotSession = { control: Control; /** This Bot's snapshot generation. See the note above on staleness. */ snapshotId: number; + /** The page this Bot was last handed, so a change of page can retire its refs. */ + livePage?: Page; /** The one live screen viewer for this Bot, if a person is watching. */ viewer?: { socket: unknown; @@ -210,7 +212,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; } /** 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..f4dc8cfc 100644 --- a/agent-computer/src/profiles.ts +++ b/agent-computer/src/profiles.ts @@ -40,6 +40,7 @@ import { profileDirectoryFor } from "./bot-id"; import { chooseEvictions, chooseIdle } from "./browser-eviction"; import { egressFor, egressLabel } from "./egress"; import { numberFromEnv } 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 @@ -180,17 +181,17 @@ const IDLE_TIMEOUT_MS = numberFromEnv("COMPUTER_BROWSER_IDLE_MS", 30 * 60_000); const IDLE_SWEEP_MS = 60_000; export function createProfiles(root: string) { + 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>(); @@ -274,6 +275,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() @@ -309,16 +313,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); 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(); + }); +});