diff --git a/CHANGELOG.md b/CHANGELOG.md index 2854ac92b..4df3bfd3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### Resetting a Bot's computer while the Bot is acting signs it out + +On a deployment with one shared computer, which is what the published image and the Helm chart run by +default, a reset takes about two seconds to close the browser before it deletes the profile. A Bot +action that arrived in that window started a new browser from the profile about to be deleted, so the +Bot stayed signed in to everything until that browser next closed, while the reset reported success +and the audit trail recorded the saved state as deleted. A Bot's browser is no longer reopened while +it is being closed: the action waits for the reset to finish and starts signed out. Computers the +supervisor makes per Bot were not affected. + ### A long control name or value in a page snapshot is cut between characters The computer's page snapshot keeps the first 200 UTF-16 code units of each control's accessible diff --git a/agent-computer/src/profiles.ts b/agent-computer/src/profiles.ts index 53714e281..800a9a599 100644 --- a/agent-computer/src/profiles.ts +++ b/agent-computer/src/profiles.ts @@ -248,6 +248,25 @@ export function createProfiles(root: string, onClosed: BrowserClosed) { const live = new Map(); /** Launches in flight, so a cold computer is started once however many callers ask at once. */ const starting = new Map>(); + /** Closes and resets in flight, so a Bot's browser is never reopened from a profile being closed or deleted. */ + const closing = new Map>(); + + const whileClosing = async ( + botId: string, + work: () => Promise, + ): Promise => { + const { promise: done, resolve } = Promise.withResolvers(); + const held = Promise.all([closing.get(botId), done]).then(() => undefined); + closing.set(botId, held); + try { + await work(); + } finally { + resolve(); + void held.then(() => { + if (closing.get(botId) === held) closing.delete(botId); + }); + } + }; // Checked, not joined. `join(root, botId)` normalizes `..` away, so a Bot id of `../workspace` // used to resolve outside the root and `reset` would delete whatever was there. @@ -273,8 +292,10 @@ export function createProfiles(root: string, onClosed: BrowserClosed) { // 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); + await whileClosing(botId, async () => { + await settleWithin(Promise.resolve(onClosed(botId)), ANNOUNCE_BUDGET_MS); + await closeAndWait(running.context).catch(() => undefined); + }); return true; }; @@ -352,6 +373,9 @@ export function createProfiles(root: string, onClosed: BrowserClosed) { * container restarts. This turns that into one slow request instead of an outage. */ async page(botId: string): Promise { + // Only when something is closing: an unconditional await would let a stop slip in ahead of the launch. + const closingNow = closing.get(botId); + if (closingNow) await closingNow; /* * One launch at a time per Bot. Calls that arrive during a launch wait for that launch instead * of starting another browser against the same profile directory. @@ -476,8 +500,10 @@ export function createProfiles(root: string, onClosed: BrowserClosed) { */ async reset(botId: string): Promise { // Its own reason rather than borrowing stop's, so the trail says which of the two happened. - await closeOnRequest(botId, "it was reset"); - await rm(directoryFor(botId), { recursive: true, force: true }); + await whileClosing(botId, async () => { + await closeOnRequest(botId, "it was reset"); + await rm(directoryFor(botId), { recursive: true, force: true }); + }); }, /** diff --git a/agent-computer/tests/browser-close-announcement.test.ts b/agent-computer/tests/browser-close-announcement.test.ts index 71c10075a..9210cd725 100644 --- a/agent-computer/tests/browser-close-announcement.test.ts +++ b/agent-computer/tests/browser-close-announcement.test.ts @@ -133,6 +133,39 @@ describe.skipIf(!asked)( await profiles.closeAll(); }, 60_000); + test("a page asked for while a reset is closing the browser starts signed out", async () => { + process.env.COMPUTER_BROWSER_IDLE_MS = String(30 * 60_000); + const { createProfiles } = (await import( + `../src/profiles?reopening=${Date.now()}` + )) as typeof import("../src/profiles"); + const root3 = join(root, "reopening"); + const profiles = createProfiles(root3, () => undefined); + const site = "https://example.test"; + + const first = await profiles.page("keeper"); + await first.context().addCookies([ + { + name: "session", + value: "signed-in", + url: site, + expires: Math.floor(Date.now() / 1000) + 86_400, + }, + ]); + await profiles.stop("keeper"); + const reopened = await profiles.page("keeper"); + expect(await reopened.context().cookies(site)).toHaveLength(1); + + const resetting = profiles.reset("keeper"); + await new Promise((resolve) => setTimeout(resolve, 100)); + const during = await profiles.page("keeper"); + // Read at once, as an action would, since Chromium loads cookies on first use. + await during.context().cookies(site); + await resetting; + + expect(await during.context().cookies(site)).toEqual([]); + 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.