Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 3 additions & 0 deletions agent-computer/src/challenge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ export async function detectChallenge(
result.reason,
options.toolCallId,
result.kind,
// The challenge's own address, so a browser that restarts before anyone takes control is sent
// back to it rather than handing the person a blank page with nothing to solve.
page.url(),
).request;
if (!request)
throw new Error("The challenge handoff did not produce a request.");
Expand Down
4 changes: 4 additions & 0 deletions agent-computer/src/control-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ const statuses = new Set([
"interrupted",
]);
const sources = new Set(["model", "manual", "cloudflare", "visible-challenge"]);
/** The longest page address a request keeps. Longer ones are not kept rather than cut. */
export const MAX_REQUEST_URL = 2048;
const record = (v: unknown): v is Record<string, unknown> =>
!!v && typeof v === "object" && !Array.isArray(v);
const timestamp = (v: unknown): v is string =>
Expand All @@ -60,6 +62,8 @@ function validRequest(v: unknown): v is HandoffRequest {
(v.expiresAt === undefined || timestamp(v.expiresAt)) &&
(v.finishedAt === undefined || timestamp(v.finishedAt)) &&
(v.interruption === undefined || typeof v.interruption === "string") &&
(v.url === undefined ||
(typeof v.url === "string" && v.url.length <= MAX_REQUEST_URL)) &&
(v.status !== "waiting" || timestamp(v.expiresAt)) &&
(v.status === "waiting" || v.expiresAt === undefined) &&
(v.status === "waiting" || v.status === "taken"
Expand Down
61 changes: 60 additions & 1 deletion agent-computer/src/control.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import type {
HandoffSource,
} from "../../shared/computer-control";
import { createActionBarrier } from "./action-barrier";
import type { ControlStore, StoredControl } from "./control-store";
import {
type ControlStore,
MAX_REQUEST_URL,
type StoredControl,
} from "./control-store";
export type ControlState = ComputerControlState;
export class ControlError extends Error {
constructor(message: string) {
Expand Down Expand Up @@ -40,6 +44,26 @@ type Options = {
drainTimeoutMs?: number;
};

/**
* A page worth sending a blank browser back to: a real http(s) address, short enough to keep.
*
* Cloudflare's `__cf_chl_*` parameters are dropped. They are one-time tokens for the challenge that
* was on screen, which a restart has already lost, so the page kept is the one the Bot was opening.
*/
export function pageWorthKeeping(url: unknown): string | undefined {
if (typeof url !== "string" || url.length > MAX_REQUEST_URL) return undefined;
try {
const parsed = new URL(url);
if (parsed.protocol !== "http:" && parsed.protocol !== "https:")
return undefined;
for (const name of [...parsed.searchParams.keys()])
if (name.startsWith("__cf_chl_")) parsed.searchParams.delete(name);
return parsed.toString();
} catch {
return undefined;
}
}

/** Request identity is durable; secret values never enter this state machine. */
export function createControl(
now: () => string = () => new Date().toISOString(),
Expand Down Expand Up @@ -191,6 +215,21 @@ export function createControl(
}
syncAdmission();

/**
* The page of the most recent request, when that request was interrupted recently.
*
* Only the most recent one: a request that finished, was cancelled or expired after an
* interruption means the person or the Bot moved on, and an older page must not come back.
*/
function interruptedPage(at: string): string | undefined {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/computers/stop and /computers/reset both call interrupt(), so they leave the request interrupted, the same status a process restart uses. That means a help request from a blank browser within 10 minutes of a reset inherits the old page and reopens it. I checked: after requestHelp(..., "https://bank.example/reset?token=secret") → interrupt("The computer was reset; …") → requestHelp("other", "b", "model", "about:blank"), the new request has the old URL.

Reset is meant to forget everything, so it shouldn't bring that page back, and stop is probably a deliberate "move on" too. Could inheritance be limited to the process-restart interruption? For example, mark that interruption, or clear url in the stop and reset routes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fe9622e. Only a restart (the process or the browser coming back up) marks a request as keeping its page now, through a keepsPage flag; the stop and reset routes clear it, including when the browser close reaches renewRun first. Your repro is a test in tests/control-handoff-page.test.ts (red on bf70aab, green now), alongside stop or reset after a browser close.

const latest = data.requests.at(-1);
if (latest?.status !== "interrupted" || !latest.url || !latest.finishedAt)
return undefined;
return Date.parse(at) - Date.parse(latest.finishedAt) <= HELP_REQUEST_TTL_MS
? latest.url
: undefined;
}

function rememberAlias(toolCallId: string, requestId: string) {
Object.defineProperty(data.aliases, toolCallId, {
value: requestId,
Expand All @@ -206,6 +245,7 @@ export function createControl(
reason: unknown,
toolCallId?: string,
source: HandoffSource = "model",
pageUrl?: string,
): ControlState {
expire();
if (
Expand All @@ -230,6 +270,13 @@ export function createControl(
}
barrier.close(); // Before any persistence or awaited browser operation.
const at = now();
/*
* The page this request is about. A browser that restarted comes back blank, so a request
* raised on a blank page inherits the page of the request a restart interrupted, if that was
* recent: the Bot asking again after "the computer process restarted" is still asking about
* the page it lost, and the person it hands control to needs that page in front of them.
*/
const url = pageWorthKeeping(pageUrl) ?? interruptedPage(at);
const request: HandoffRequest = {
id: crypto.randomUUID(),
toolCallId,
Expand All @@ -242,6 +289,7 @@ export function createControl(
createdAt: at,
updatedAt: at,
expiresAt: new Date(Date.parse(at) + HELP_REQUEST_TTL_MS).toISOString(),
...(url ? { url } : {}),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url stays on the request after it finishes (completed, cancelled, expired), so it's saved to .control/<bot>.json for as long as the record is kept (30 days, up to 33 records). Page addresses can carry one-time tokens (?code=, reset links, magic links). A cancelled request in my probe still had ?code=abc on it.

The address is only needed while the request is live, or when it was interrupted and is still inside the TTL. Could finish() delete request.url for every status except interrupted? interruptedPage could also drop it once it's past the TTL.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fe9622e. Every ending except a restart now deletes url in finish(), a restart's page is dropped once the 10-minute window passes, and records written before this change are cleaned when they are read. Tests cover cancelled, completed and expired requests, the past-window case and the old-record cleanup.

};
data.currentRequestId = request.id;
data.requests.push(request);
Expand Down Expand Up @@ -309,6 +357,17 @@ export function createControl(
return get();
},
interrupt,
/**
* Where a blank browser should be sent before a person works the current handoff.
*
* The live request's page, while one is waiting or held. Nothing otherwise: once the request
* is over, whatever the browser shows is the Bot's business again.
*/
pageToRestore(): string | undefined {
expire();
const request = current();
return active(request) ? request?.url : undefined;
},
snapshotTaken() {
if (
data.holder === "bot" &&
Expand Down
112 changes: 90 additions & 22 deletions agent-computer/src/egress.ts
Original file line number Diff line number Diff line change
Expand Up @@ -580,29 +580,105 @@ async function decide(
}

/**
* Connection options that reach a name through the addresses already checked, and nothing else.
* The checked addresses in the order they are tried: IPv4 first, then IPv6, each in DNS order.
*
* Every address the name resolved to passed the deny check, so any of them may be tried. Node's own
* Happy Eyeballs (`autoSelectFamily`, RFC 8305) then tries them in turn, so an AAAA answer on a
* computer with no IPv6 route falls back to the A record instead of failing. The `lookup` answers
* from the checked list without asking DNS, which is what keeps the pin: the name is never resolved
* a second time. A literal address needs neither.
* IPv4 first because a container network is the usual place this runs, and many have no IPv6 route:
* an AAAA answer tried first there costs a full attempt timeout on every connection.
*/
export function connectOrder(addresses: readonly string[]): string[] {
return [
...addresses.filter((address) => isIP(address) !== 6),
...addresses.filter((address) => isIP(address) === 6),
];
}

/** How long one address may take to accept before the next checked address is tried. */
export const CONNECT_ATTEMPT_TIMEOUT_MS = 3_000;

/**
* Connect to a name through the addresses already checked, one at a time, and nothing else.
*
* Every address the name resolved to passed the deny check, so any of them may be tried; a literal
* address is the only one tried. The name is never resolved a second time, which is what keeps the
* pin. Each attempt connects to one literal address, so no lookup runs and no runtime fallback logic
* is involved: this loop is the fallback.
*
* NOT `autoSelectFamily` WITH THE CHECKED LIST, WHICH THIS USED TO BE. Bun's `node:net` walks a list
* of addresses with a timer that can fire after the connection it belongs to has been torn down, and
* then throws `TypeError: null is not an object (evaluating 'context')` from
* `internalConnectMultipleTimeout` (oven-sh/bun#25633, still reproducing on 1.3.13 and seen here on
* 1.3.14). The throw is in a timer, so nothing catches it and the computer process exits. A
* Cloudflare-fronted site resolves to several addresses and a challenge page opens and abandons many
* connections at once, so opening one in a Bot's browser restarted the computer and lost the page the
* person was being asked to solve.
*/
export function connectInTurn(
target: { host: string; port: number; addresses?: readonly string[] },
onReady: (socket: Socket) => void,
onError: (error: Error) => void,
attemptTimeoutMs = CONNECT_ATTEMPT_TIMEOUT_MS,
): void {
const queue = target.addresses?.length
? connectOrder(target.addresses)
: [target.host];
let lastError: Error | undefined;
const attempt = (index: number) => {
const address = queue[index];
if (address === undefined) {
onError(lastError ?? new Error(`Could not connect to ${target.host}.`));
return;
}
let settled = false;
const socket = connect({ host: address, port: target.port });
const moveOn = (error: Error) => {
if (settled) return;
settled = true;
clearTimeout(timer);
lastError = error;
socket.destroy();
attempt(index + 1);
};
const timer = setTimeout(
() =>
moveOn(new Error(`${address}:${target.port} did not answer in time.`)),
attemptTimeoutMs,
);
socket.once("error", moveOn);
socket.once("connect", () => {
if (settled) return;
settled = true;
clearTimeout(timer);
socket.off("error", moveOn);
// From here an error belongs to the caller, exactly as it did when it registered its own.
socket.once("error", onError);
onReady(socket);
});
};
attempt(0);
}

/**
* Request options that reach a name through one checked address, without a second lookup.
*
* For plain HTTP, where the request library owns the socket. One address rather than the list, for
* the reason given on {@link connectInTurn}: the list is what sends Bun down its crashing path. The
* first IPv4 address is chosen, so a computer with no IPv6 route still connects.
*/
function pinnedTo(addresses: readonly string[] | undefined) {
if (!addresses?.length) return {};
const entries = addresses.map((address) => ({
address,
family: isIP(address) === 6 ? 6 : 4,
}));
const [address] = connectOrder(addresses);
if (address === undefined) return {};
const family = isIP(address) === 6 ? 6 : 4;
return {
autoSelectFamily: true,
autoSelectFamily: false,
lookup: (
_host: string,
options: { all?: boolean } | number | undefined,
callback: (...args: unknown[]) => void,
) => {
if (typeof options === "object" && options?.all) callback(null, entries);
else callback(null, entries[0]?.address, entries[0]?.family);
if (typeof options === "object" && options?.all)
callback(null, [{ address, family }]);
else callback(null, address, family);
},
};
}
Expand Down Expand Up @@ -654,15 +730,7 @@ function tunnel(
onError: (error: Error) => void,
) {
if (!upstream) {
const socket = connect(
{
host: target.host,
port: target.port,
...(pinnedTo(target.addresses) as object),
},
() => onReady(socket),
);
socket.once("error", onError);
connectInTurn(target, onReady, onError);
return;
}
const via = upstreamAddress(upstream);
Expand Down
81 changes: 77 additions & 4 deletions agent-computer/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { mkdir, rm, writeFile } from "node:fs/promises";
import { setDefaultAutoSelectFamily } from "node:net";
import { join } from "node:path";
import { serve } from "bun";
import type { Page } from "playwright";
Expand Down Expand Up @@ -105,6 +106,14 @@ if (!COMPUTER_TOKEN) {
process.exit(1);
}

/*
* No multi-address connect anywhere in this process. Bun turns it on by default, and its timer can
* throw from `internalConnectMultipleTimeout` after the connection is gone, which exits the process
* (oven-sh/bun#25633). The egress filter already connects one checked address at a time (see
* `connectInTurn`); this covers every other connect here, such as reaching an upstream proxy by name.
*/
setDefaultAutoSelectFamily(false);

const RUNTIME = browserRuntimeFromEnv(process.env);
const BROWSER_MODE = RUNTIME.mode;
const VIRTUAL_DISPLAY = await startVirtualDisplay(
Expand Down Expand Up @@ -305,6 +314,58 @@ async function currentPage(
return page;
}

/** A browser that has not been sent anywhere, which is what a restarted one shows. */
function isBlankPage(url: string): boolean {
return (
url === "" ||
url === "about:blank" ||
url.startsWith("chrome://newtab") ||
url.startsWith("chrome://new-tab-page")
);
}

/**
* Put the page a handoff is about back in front of the person, when the browser came back blank.
*
* A browser restarts underneath a waiting request more often than it should: Chromium crashing, the
* idle sweep or the cap closing it, the whole computer restarting. Each of those relaunches onto
* `about:blank`, and before this a person who took control was handed a blank screen with nothing to
* solve while the Bot waited for them to solve it. The request carries the page it was raised on, so
* the computer opens it again first. Through the same navigation the Bot uses, so the egress filter
* and the page-access rule still decide whether it may load.
*
* Only a blank browser is touched. A person or a site that moved the page somewhere else meant to.
* Best effort: a page that will not load is logged and the handoff goes ahead, because a person in
* front of a blank browser can still type the address, and a refused handoff helps nobody.
*/
async function restoreHandoffPage(botId: string): Promise<void> {
const session = sessions.for(botId);
const want = session.control.pageToRestore();
if (!want) return;
try {
const target = await currentPage(botId);
if (!isBlankPage(target.url())) return;
await navigateWebPage(target, want, RUNTIME.backend, NAVIGATION_TIMEOUT_MS);
// A new document, so every ref handed out before it is retired, as after any navigation.
session.snapshotId += 1;
console.info(
JSON.stringify({
type: "computer-handoff-page-restored",
botId,
origin: new URL(want).origin,
}),
);
} catch (error) {
console.info(
JSON.stringify({
type: "computer-handoff-page-not-restored",
botId,
reason: error instanceof Error ? error.message : String(error),
}),
);
}
}

/**
* The page as text, the way a reader sees it.
*
Expand Down Expand Up @@ -750,9 +811,17 @@ serve<StreamData>({
typeof body.toolCallId !== "string"
)
return json({ error: "toolCallId must be a string." }, 400);
return json(
session.control.requestHelp(body?.reason, body?.toolCallId),
// The page the Bot is stuck on, recorded with the request. A browser that will not start is
// no reason to refuse the request: the person is the one who can see why.
const stuckOn = await currentPage(botId).catch(() => undefined);
const state = session.control.requestHelp(
body?.reason,
body?.toolCallId,
"model",
stuckOn?.url(),
);
await restoreHandoffPage(botId);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the one on /control/take) waits for restoreHandoffPage, which waits up to NAVIGATION_TIMEOUT_MS (30s by default), plus a cold browser launch if one isn't running. That delays the Bot's help call and the person's "Take control" click, and gets close to the server client's 45s timeout.

The restore is best effort and its result doesn't change the response, so could it run without being awaited (void restoreHandoffPage(botId), which already catches its own errors), or with a shorter timeout?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fe9622e. Help and Take control both start the restore in the background and answer straight away. Restores run one at a time per Bot and give up if the person has sent input or the request has ended by the time the browser is ready. A new real-browser test (tests/handoff-page-restore.test.ts, now in CI's real-browser job) shows both calls under 2 s, against about 4 s before, with the restored page still on screen.

return json(state);
}

// The Bot asking for one value it must not be told. It has already focused the field.
Expand Down Expand Up @@ -872,8 +941,12 @@ serve<StreamData>({
} | null;
if (typeof body?.requestId !== "string" || !body.requestId.trim())
return json({ error: "A requestId is required." }, 400);
if (url.pathname === "/control/take")
return json(await session.control.take(body.requestId));
if (url.pathname === "/control/take") {
const taken = await session.control.take(body.requestId);
// The browser may have restarted while the request waited; give the person the page.
await restoreHandoffPage(botId);
return json(taken);
}
if (url.pathname === "/control/cancel")
return json(session.control.cancel(body.requestId));
return json(session.control.release(body.requestId));
Expand Down
Loading
Loading