Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,23 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.

## Unreleased

### A wiped or restarted shared computer no longer leaves refs pointing at the dead page

Snapshots are ordered on the run of the browser that took them as well as the generation, so a
computer that is replaced cannot have its old page mistaken for its new one. That ordering was
reaching only the deployments that give each Bot its own container or sandbox, because the run was
read off the infrastructure and the deployment with one shared computer has none to read.

Two failures followed there, and both are fixed. A snapshot still in flight when somebody pressed
Reset brought the wiped page back, and the boundary went on deciding about its elements: a rule about
"Confirm transfer" firing on a click nowhere near one, or failing to fire on one that is. And a
computer that restarted counted generations from one again, so its first snapshots were dropped as
stale and refs kept resolving against a page nobody was on until the counter climbed back past it.

The computer now mints a run for each Bot's browser session, mints a new one when the Bot is reset,
and answers which run it is on. Nothing changes for a deployment that already reports one, and a
computer too old to answer leaves the ordering exactly where it was rather than refusing anything.

### Embedded PostgreSQL initialises on a platform volume, and says so when it cannot

`EMBEDDED_POSTGRES=on` could not create its cluster on a platform whose persistent volume is an ext4
Expand Down
106 changes: 41 additions & 65 deletions agent-computer/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,16 @@ import {
} from "./authorisation";
import { isPlainBotId } from "./bot-id";
import {
type Control,
ControlError,
ControlRequestError,
createControl,
NO_SECRET_PENDING,
TAKE_CONTROL_FIRST,
} from "./control";
import { identity } from "./identity";
import { createProfiles, numberFromEnv, VIEWPORT } from "./profiles";
import { type InputMessage, startScreencast } from "./screencast";
import { type BotSession, createSessions } from "./sessions";
import { createShell } from "./shell";
import { createViewerSlot, type ViewerSlot } from "./viewer";
import {
createWorkspace,
WorkspaceFileError,
Expand Down Expand Up @@ -102,63 +100,12 @@ const TEXT_EXTRACT_LIMIT = 6000;
* to resolve. Playwright's `aria-ref` engine is the runtime enforcement: it resolves a ref only against
* the most recent snapshot, only while the element is still connected to the document, and it mints a
* new ref if an element's role or accessible name changed, so a recycled node cannot inherit an old one.
*/
/** Per-Bot browser-control state. Profiles are isolated, but this process is not a security boundary. */
type BotSession = {
control: Control;
/** This Bot's snapshot generation. See the note above on staleness. */
snapshotId: number;
/** The page this Bot was last handed, so a change of page can retire its refs. */
livePage?: Page;
/**
* This Bot's live screen, and who owns it.
*
* Always present, because the slot is the answer to "is anybody watching" as well as the holder of
* whoever is. An empty slot is a Bot nobody is watching; there is no second way to say that.
*/
viewer: ViewerSlot;
};

const sessions = new Map<string, BotSession>();

/**
* Forget the sessions of Bots whose browsers are no longer running.
*
* The map gained an entry per Bot id this process had ever seen and lost none, so a deployment where
* every employee has a Bot accumulated one small object per employee for the life of the container.
* Small, but unbounded, which is the same shape as the browsers themselves.
*
* Only entries with no live browser and nobody watching are dropped: the state is the generation
* counter and the control handover, and both belong to a running browser. A Bot whose browser has
* been closed starts a fresh session next time, which is what starting a fresh browser means.
*
* "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.
* The counter, the run it belongs to, and the sessions holding both live in their own module: the
* rules about when a run changes are testable there, and not here, because this file launches a
* browser the moment it is imported.
*/
function forgetIdleSessions(): void {
for (const [botId, session] of [...sessions.entries()]) {
if (session.viewer.occupied()) continue;
if (profiles.isLive(botId)) continue;
sessions.delete(botId);
}
}

function sessionFor(botId: string): BotSession {
const existing = sessions.get(botId);
if (existing) return existing;
const created: BotSession = {
control: createControl(),
snapshotId: 0,
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();
return created;
}
const sessions = createSessions({ isLive: (botId) => profiles.isLive(botId) });

/**
* Sent by the server as a header on every call. Absent means the caller does not know or does not
Expand Down Expand Up @@ -204,7 +151,7 @@ const workspace = createWorkspace(process.env.WORKSPACE_DIR ?? "/workspace");
* 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
* `sessions.get`, never `sessions.for`: 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.
*/
Expand Down Expand Up @@ -234,7 +181,7 @@ const DEFAULT_BOT_ID = (() => {
})();

async function currentPage(botId: string): Promise<Page> {
const session = sessionFor(botId);
const session = sessions.for(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.
Expand Down Expand Up @@ -390,7 +337,7 @@ serve<StreamData>({
*/
websocket: {
async open(ws) {
const session = sessionFor(ws.data.botId);
const session = sessions.for(ws.data.botId);
/*
* Claimed before anything is awaited, and that order is the fix.
*
Expand Down Expand Up @@ -442,7 +389,7 @@ serve<StreamData>({
/*
* 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
* of the process, and the sweep in sessions.ts could never take 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);
Expand All @@ -457,7 +404,7 @@ serve<StreamData>({
},

async message(ws, raw) {
const session = sessionFor(ws.data.botId);
const session = sessions.for(ws.data.botId);
/*
* Whose screen this is, asked before anything is done with the input.
*
Expand Down Expand Up @@ -526,7 +473,7 @@ serve<StreamData>({
// Names the socket, so it can only ever give up its own screen. A superseded socket closing
// after its replacement has started releases nothing; see viewer.ts.
//
// `get`, not `sessionFor`: a socket closing for a Bot with no session has nothing to release,
// `get`, not `for`: 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);
Expand Down Expand Up @@ -566,7 +513,7 @@ serve<StreamData>({
if (!isOpenPath(url.pathname) && !isPlainBotId(botId)) {
return json({ error: "That is not a usable bot id." }, 400);
}
const session = sessionFor(botId);
const session = sessions.for(botId);

/*
* The wheel, asked once for everything that acts.
Expand Down Expand Up @@ -752,6 +699,25 @@ serve<StreamData>({
});
}

/**
* Which run of this Bot's browser the caller is looking at.
*
* The server orders snapshots on `(run, generation)`, and the generation alone cannot carry it:
* this process mints one at zero for every session that is new, so a restart, a redeploy, an
* eviction from the idle sweep and a reset all produce a page at generation one that looks older
* than the page the server still has. Ordering on the run as well is what lets the fresh one land
* and the dead one stop resolving.
*
* A read, and deliberately not on the acting list: a person holding the wheel must not turn every
* ref the server holds into an unanswerable question.
*
* Its own endpoint rather than a field on `/computers`, because that one reads the profile
* directory and this is asked on the path of every governed action.
*/
if (url.pathname === "/run" && request.method === "GET") {
return json({ run: session.run });
}

/**
* The computers this process holds. The shape is a list because the admin surface is a
* list, and because a Bot that has a profile has a computer whether or not a browser is running
Expand Down Expand Up @@ -786,6 +752,16 @@ serve<StreamData>({
await profiles.reset(botId);
// Reset releases control because any previous browser session and pending secret request are gone.
session.control.release();
/*
* And a new run, because the browser this session described is gone.
*
* Nothing else here says so. The entry stays in the map and the generation counter carries on,
* so a snapshot that was in flight when the wipe landed arrives at the server carrying the same
* run and the same generation as the fresh browser's would: the server deletes its row on
* reset, the late save inserts the wiped page straight back, and every ref on it goes on
* resolving. A new run is what makes those two distinguishable at the far end.
*/
sessions.renewRun(botId);
return json({ reset: true, botId });
}

Expand Down
143 changes: 143 additions & 0 deletions agent-computer/src/sessions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
/**
* What this process remembers about a Bot between requests, and which run of its browser that is.
*
* ITS OWN MODULE BECAUSE THE RUN IS A DECISION, not bookkeeping. `index.ts` imports Playwright at
* module scope, so anything left in there can only be tested by launching a browser, and the rules
* below are exactly the ones with a wrong answer available: which events start a new run, and which
* leave the one in progress alone. Same reason `browser-eviction.ts` and `profile-listing.ts` are
* separate files. `Page` comes in as a type only, which is erased at runtime, so this module and its
* tests stay free of the browser.
*
* The generation counter orders snapshots within one run of a browser and says nothing across two.
* It is minted at zero for a session that is new, and a session is new after a container restart, a
* redeploy, an eviction from the sweep below, and a reset. None of those reach the server, so a ref
* it is still holding from the run before matches a row nothing has overwritten, and the boundary
* decides about an element on a page that no longer exists.
*
* The run carries that difference to the server, which orders on `(run, generation)` rather than on
* the generation alone. A Bot with its own container gets one from the container and a sandbox gets
* one from the moment its browser last became ready; the shared computer is one process serving
* every Bot, outliving every reset, so here the answer has to come from the session itself.
*/
import type { Page } from "playwright";
import { type Control, createControl } from "./control";
import { createViewerSlot, type ViewerSlot } from "./viewer";

/** Per-Bot browser-control state. Profiles are isolated, but this process is not a security boundary. */
export type BotSession = {
control: Control;
/** This Bot's snapshot generation. Ordered within this run, and meaningless across two. */
snapshotId: number;
/**
* Which run of this Bot's browser this is, for the server to order snapshots across.
*
* Per Bot rather than per process: they share this container and nothing else, so a run naming the
* container would be one string for every Bot and would never change when one of their browsers
* was replaced.
*/
run: string;
/** The page this Bot was last handed, so a change of page can retire its refs. */
livePage?: Page;
/**
* This Bot's live screen, and who owns it.
*
* Always present, because the slot is the answer to "is anybody watching" as well as the holder of
* whoever is. An empty slot is a Bot nobody is watching; there is no second way to say that.
*/
viewer: ViewerSlot;
};

export type SessionsOptions = {
/** Whether this Bot has a browser right now, so the sweep keeps what belongs to a running one. */
isLive: (botId: string) => boolean;
/** How many sessions may accumulate before the sweep runs. */
cap?: number;
/** Injected so a test can name the run it expects rather than match a uuid. */
mintRun?: () => string;
};

/**
* A factory rather than a module-level `Map`, so a test can have its own and two cannot accidentally
* share state. The same shape `createControl` uses, for the same reason.
*/
export function createSessions(options: SessionsOptions) {
const sessions = new Map<string, BotSession>();
const cap = options.cap ?? 32;
const mintRun = options.mintRun ?? (() => crypto.randomUUID());

/**
* Forget the sessions of Bots whose browsers are no longer running.
*
* The map gained an entry per Bot id this process had ever seen and lost none, so a deployment
* where every employee has a Bot accumulated one small object per employee for the life of the
* container. Small, but unbounded, which is the same shape as the browsers themselves.
*
* Only entries with no live browser and nobody watching are dropped: the state is the generation
* counter, the run, and the control handover, and all three belong to a running browser. A Bot
* whose browser has been closed starts a fresh session next time, which is what starting a fresh
* browser means, and the new run is what says so.
*
* "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 forgetIdle(): void {
for (const [botId, session] of [...sessions.entries()]) {
if (session.viewer.occupied()) continue;
if (options.isLive(botId)) continue;
sessions.delete(botId);
}
}

function sessionFor(botId: string): BotSession {
const existing = sessions.get(botId);
if (existing) return existing;
const created: BotSession = {
control: createControl(),
snapshotId: 0,
run: mintRun(),
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 > cap) forgetIdle();
return created;
}

return {
for: sessionFor,

/**
* The session this Bot already has, or nothing.
*
* Separate from `for` because the teardown paths need to ask without answering. A browser
* closing, or a socket closing for a Bot with no session, has nothing to take down, and creating
* an entry there would grow the map on exactly the paths it is meant to shrink on.
*/
get(botId: string): BotSession | undefined {
return sessions.get(botId);
},

/**
* Start a new run for this Bot, because its browser session is over.
*
* A reset closes the browser and deletes the profile, and the session survives it: nothing takes
* the entry out of the map, so the generation counter carries on from where it was. Without a new
* run the server cannot tell a save that was in flight when the wipe landed from the fresh
* browser's own, and the wiped page comes back with nothing willing to refuse it.
*
* The generation is deliberately left alone. It belongs to the browser, which restarts it at zero
* on its own when a session is genuinely new, and zeroing it here would hand the server a fresh
* generation one under a fresh run for a counter that never restarted.
*/
renewRun(botId: string): string {
const existing = sessions.get(botId);
// Nothing to renew: a Bot nobody has touched yet gets a session, and its run is already new.
if (!existing) return sessionFor(botId).run;
existing.run = mintRun();
return existing.run;
},
};
}
Loading