diff --git a/CHANGELOG.md b/CHANGELOG.md index 85bcf0b573..358a3d5d82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,19 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. - Migration `0062_action_policy_labels` adds a `labels` column to `action_policies`, defaulting to empty. Existing boundary rules keep working and show as "Custom rule" until somebody names them. +### A computer that is starting says so, instead of showing a problem + +Opening the Computer panel while a Bot's computer was asleep or starting showed "Can't show the +screen right now", and an action during a cold start was marked "didn't work". The panel now says +the computer is starting, or is off until the Bot next uses it, and shows a problem only when it +can't be reached or has gone two and a half minutes without a picture; a live screen being +reconnected shows a small "Reconnecting…" badge. An action turned away by a cold start reads "not +done yet" and the Bot is told to try again. Looking at the panel no longer wakes a sleeping computer, +a Bot says in one sentence what it is about to do before its first computer action, and a hosted +computer's cold start answers within 45 seconds, under a load balancer's usual 60-second idle +timeout. A Docker computer still booting its browser reads as starting once the supervisor is +upgraded too; an older supervisor works as before. + ### Screens share one spacing, row and empty-state design Every configuration screen now takes its spacing from the page frame: 32px under the title, 48px diff --git a/app/src/components/channels/tool-line.tsx b/app/src/components/channels/tool-line.tsx index 91306c720f..528d1d794e 100644 --- a/app/src/components/channels/tool-line.tsx +++ b/app/src/components/channels/tool-line.tsx @@ -17,6 +17,7 @@ export function ToolLine({ running, refused, failed, + notYet, children, }: { /** Drawn before the label, small and muted. A group of steps puts its kinds here. */ @@ -30,6 +31,12 @@ export function ToolLine({ refused?: boolean; /** It was permitted and did not work. A different request might. */ failed?: boolean; + /** + * Nothing was done yet, for an ordinary reason: a computer still starting turned the call away + * before it reached anything. Muted like a success, because nothing went wrong, and labelled so it + * does not read as one. + */ + notYet?: boolean; /** Shown when the line is expanded. Without it the line is not expandable. */ children?: ReactNode; }) { @@ -51,7 +58,9 @@ export function ToolLine({ ? `Blocked: ${label}` : failed ? `${label}, didn't work` - : label} + : notYet + ? `${label}, not done yet` + : label} {detail ? {detail} : null} diff --git a/app/src/components/computer/computer-view.tsx b/app/src/components/computer/computer-view.tsx index 4fc93e71b6..b1735d5e93 100644 --- a/app/src/components/computer/computer-view.tsx +++ b/app/src/components/computer/computer-view.tsx @@ -1,6 +1,11 @@ +import { useQuery } from "@tanstack/react-query"; import { useEffect, useRef, useState } from "react"; import { createPortal } from "react-dom"; import { supplySecret } from "@/lib/computers/control"; +import { + type ComputerState, + computerStatusQueryOptions, +} from "@/lib/computers/queries"; import { readPageFrame, readScreenshot, @@ -85,6 +90,23 @@ async function preloadFrame(base64: string): Promise { } } +function capitalise(text: string): string { + return text.charAt(0).toUpperCase() + text.slice(1); +} + +/** + * How long a computer may go without a frame before that is worth telling anybody. + * + * GENEROUS ON PURPOSE. A first start pulls an image, schedules a pod and launches a browser, and a + * minute is ordinary. The panel used to call the first frameless poll a problem, so every cold start + * opened on "Can't show the screen right now… ask an admin", in front of a person whose computer was + * doing exactly what it should. Past the server's own wait for a cold start, with room to spare. + */ +const STARTUP_GRACE_MS = 150_000; + +/** How often a computer that has not drawn a frame yet is asked where it is. */ +const STATUS_POLL_MS = 2_000; + /** Identical frames in a row that mean the page has stopped changing. */ const SETTLED_FRAMES = 3; @@ -107,9 +129,22 @@ function NothingToSee({ blankBrowser, settled, page, + waiting, + owner = "the Bot's", }: { problem: string | null; blankBrowser: boolean; + /** + * Why there is no picture yet, when the reason is ordinary. + * + * `starting` is a computer on its way up, which on a first start takes about a minute; `asleep` is + * one that is off and starts on the Bot's next action. Neither is a problem and neither is drawn as + * one. A problem is only what is left after a generous wait, or a computer the server says it + * cannot reach. + */ + waiting?: "starting" | "asleep" | undefined; + /** Whose computer, possessive and lower case: "Sales's", or "the Bot's" without a name. */ + owner?: string; /** Whether this is a turn that has finished, rather than the browser as it is now. */ settled?: boolean; /** The page that turn opened, named when there is no picture of it. */ @@ -157,6 +192,15 @@ function NothingToSee({ to check its Computer. + ) : waiting === "starting" ? ( + + Starting {owner} computer. The first start takes about a minute. + + ) : waiting === "asleep" ? ( + + {capitalise(owner)} computer is off. It starts when the Bot next uses + it. + ) : blankBrowser ? ( The Bot hasn't opened a page yet. ) : ( @@ -202,6 +246,8 @@ type Props = { * side panel is not a turn and has nothing to remember. */ toolCallId?: string; + /** How long to wait for a first frame before calling it a problem. Shortened by tests. */ + startupGraceMs?: number; }; export function ComputerView({ @@ -215,11 +261,21 @@ export function ComputerView({ page, finished, toolCallId, + startupGraceMs = STARTUP_GRACE_MS, }: Props) { const [shot, setShot] = useState(null); + /** + * The last screenshot that was attempted and failed. Held, not shown: see `shownProblem` below. + */ const [problem, setProblem] = useState(null); + /** What the last poll said about a computer that is not ready, which it says without waking it. */ + const [polledState, setPolledState] = useState(null); + /** Set once a computer has gone `startupGraceMs` without a frame. */ + const [stalled, setStalled] = useState(false); const [expanded, setExpanded] = useState(false); const [streamProblem, setStreamProblem] = useState(null); + /** A dropped live stream being retried on its own. A badge, never the overlay. */ + const [reconnecting, setReconnecting] = useState(false); const [retryKey, setRetryKey] = useState(0); /** * A finished turn is history, and history is not polled. @@ -339,11 +395,29 @@ export function ComputerView({ // Always fetch at least one frame; only repeated refreshes are conditional. const tick = async () => { try { - const { frame, error } = await readScreenshot(computerId); + const { frame, error, notReady } = await readScreenshot(computerId); if (generation.current !== mine) return; - if (!frame) { + if (notReady) { + /* + * Asleep or starting, said by the server without waking the computer. Not a problem: + * nothing has failed, so the last failure (if any) is no longer the news. + */ + setPolledState(notReady.state); + setProblem(null); + /* + * A frame from before the computer went to sleep is not what it shows now, and keeping it + * would also keep the live stream mounted, which reconnects to a computer nobody woke. + */ + setShot(null); + } else if (!frame) { setProblem(error ?? "The screen isn't available right now."); + /* + * The state is unknown again, not whatever the last not-ready answer said. A stale + * "absent" would keep calling a computer that is failing "off", and an off computer is + * never timed out, so the failure would never be shown. Cleared, the status read resumes. + */ + setPolledState(null); } else { // Exact byte comparison is the settling signal. unchanged = frame.base64 === lastFrame ? unchanged + 1 : 0; @@ -353,6 +427,7 @@ export function ComputerView({ if (generation.current !== mine) return; setShot(frame); setProblem(null); + setPolledState(null); } } finally { if (generation.current === mine && shouldContinue()) { @@ -368,6 +443,38 @@ export function ComputerView({ }; }, [computerId, active, intervalMs, secretPending, settled, visualVisible]); + /* + * Where the computer is in its life, until there is a picture to show. + * + * The status route never wakes a computer, so this is safe to poll while one is starting. It is + * what the panel knows before its first screenshot has come back, and the only word on a computer + * the server cannot reach. Once a frame arrives the frame is the answer and this stops asking, and + * so it does while the screenshot poll is itself reporting the state, which it does every second. + * A failed status read is "not known yet", never a problem in itself. + */ + const status = useQuery({ + ...computerStatusQueryOptions(computerId), + enabled: !settled && visualVisible && shot === null && polledState === null, + refetchInterval: STATUS_POLL_MS, + retry: false, + }); + /** The fresher of the two answers: the poll that just ran, else the status read. */ + const computerState: ComputerState | undefined = + polledState ?? status.data?.state; + /** In a turn, an absent computer is one the Bot's action is about to start, not one left off. */ + const asleep = computerState === "absent" && !toolCallId; + /** Waiting for a first frame, for a reason that can end. A computer that is off waits for the Bot. */ + const waitingForFrame = !settled && visualVisible && shot === null && !asleep; + + useEffect(() => { + if (!waitingForFrame) { + setStalled(false); + return; + } + const timer = setTimeout(() => setStalled(true), startupGraceMs); + return () => clearTimeout(timer); + }, [waitingForFrame, startupGraceMs]); + // Input forwarding lives in LiveScreen on the socket. // Escape is bound to the window so it works regardless of overlay focus. useEffect(() => { @@ -386,6 +493,36 @@ export function ComputerView({ */ const blankBrowser = !settled && shot ? isBlankBrowser(shot) : false; + const owner = name ? `${name}'s` : "the Bot's"; + /** + * The problem worth showing, which is not the same as the last one that happened. + * + * A computer the server says it cannot reach is a problem at once. Anything else that failed is + * shown only once the wait has run past what a start can take, because before then a failed + * screenshot is far more often a browser still launching than a broken computer. + */ + const shownProblem = + computerState === "unreachable" + ? `${capitalise(owner)} computer can't be reached right now.` + : stalled + ? (problem ?? "It's taking longer than usual to start.") + : null; + /** + * The calm sentence, when there is no picture and nothing wrong. + * + * A ready computer that has simply not sent its first frame yet keeps the plain "Waiting for the + * Bot's screen…", and so does one whose state is not known yet: saying "Starting" over a computer + * that is already running would be the panel inventing a delay. A failed screenshot from a computer + * that is not known to be ready is read as one still coming up. + */ + const waiting: "starting" | "asleep" | undefined = asleep + ? "asleep" + : computerState === "starting" || + computerState === "absent" || + (problem !== null && computerState !== "ready") + ? "starting" + : undefined; + /* * Sized from the ratio, never from the payload, so the frame is identical while a screen is * loading, once it arrives, and while the browser has nothing open. A blank browser used to @@ -474,9 +611,11 @@ export function ComputerView({ {showScreen ? null : ( )} @@ -651,8 +790,23 @@ export function ComputerView({ computerId={computerId} driving={driving} onProblem={setStreamProblem} + onReconnecting={setReconnecting} retryKey={retryKey} /> + {/* + A drop being retried, said quietly in a corner. The stream retries on its + own with a backoff, and the screen under it is usually back in a second; + covering it with the Retry panel made a routine reconnect look like a dead + one. The panel below is for when the retries have run out. + */} + {reconnecting && !streamProblem ? ( + + Reconnecting… + + ) : null} {/* A live screen that ends reports why through `onProblem`, and this is the branch that is mounted when it does. Without drawing it here the message @@ -679,9 +833,11 @@ export function ComputerView({
)} diff --git a/app/src/components/computer/live-screen.tsx b/app/src/components/computer/live-screen.tsx index b4e7a38228..fe8cd543ac 100644 --- a/app/src/components/computer/live-screen.tsx +++ b/app/src/components/computer/live-screen.tsx @@ -92,6 +92,8 @@ type Props = { driving: boolean; /** Called with a human-readable reason when the stream cannot be established. */ onProblem?: (problem: string | null) => void; + /** Called while a dropped stream is being retried on its own, and again when it is back or gone. */ + onReconnecting?: (reconnecting: boolean) => void; retryKey?: number; }; @@ -105,6 +107,7 @@ export function LiveScreen({ computerId, driving, onProblem, + onReconnecting, retryKey = 0, }: Props) { const canvasRef = useRef(null); @@ -279,6 +282,7 @@ export function LiveScreen({ setConnected(ready); }, onProblem: (problem) => onProblem?.(problem), + onReconnecting: (reconnecting) => onReconnecting?.(reconnecting), onDisconnect: () => { frameSize.current = null; latestFrame.current = null; @@ -296,7 +300,7 @@ export function LiveScreen({ document.removeEventListener("visibilitychange", drawLatestFrame); socketRef.current = null; }; - }, [computerId, onProblem, retryKey]); + }, [computerId, onProblem, onReconnecting, retryKey]); useEffect(() => { const wasDriving = previouslyDriving.current; diff --git a/app/src/lib/computers/call.ts b/app/src/lib/computers/call.ts index 38357e5696..52a333ec41 100644 --- a/app/src/lib/computers/call.ts +++ b/app/src/lib/computers/call.ts @@ -94,6 +94,23 @@ export function failedComputerCall( status: number, body: Record | null, ): ToolOutcome { + /* + * A cold start that outlasted the server's wait. Said as what it is: the computer is starting, this + * call did nothing, and asking again shortly is the right next step. Kept apart from the failures + * below so the line in the transcript reads as a wait, and so the model is not handed something + * that looks like a broken computer and gives up on one seconds from ready. + */ + if (body?.starting === true) { + return { + ok: false, + starting: true, + notPerformed: true, + reason: + typeof body.error === "string" + ? body.error + : "The computer is still starting. This call was not performed. Try it again in a moment.", + }; + } return { ok: false, reason: typeof body?.error === "string" ? body.error : "That did not work.", diff --git a/app/src/lib/computers/queries.ts b/app/src/lib/computers/queries.ts index 7d8d8dcffa..8f777755f3 100644 --- a/app/src/lib/computers/queries.ts +++ b/app/src/lib/computers/queries.ts @@ -40,12 +40,46 @@ export type ActionPolicy = { export type RuleLabel = { name: string; description: string }; +/** + * Where one Bot's computer is in its life, as the server's provider reports it. + * + * `absent` is a computer that is not running: never started, or put to sleep after a quiet spell. + * Either way it starts on the Bot's next computer action, so it is not a fault. `starting` is on its + * way up. `unreachable` is the only one that is something wrong. + */ +export type ComputerState = "absent" | "starting" | "ready" | "unreachable"; + +export type ComputerStatus = { + botId: string; + state: ComputerState; + /** Set when `state` is `unreachable`. Written for an admin, so not shown on a screen anybody can open. */ + reason?: string; +}; + export const computerKeys = { all: ["computers"] as const, fleet: () => ["computers", "fleet"] as const, policy: () => ["computers", "policy"] as const, + status: (botId: string) => ["computers", "status", botId] as const, }; +/** + * One Bot's computer's state, read without waking it. + * + * The status route reads the provider's own record of the computer and never dials it, which is what + * makes it safe to poll while somebody is watching a computer start: asking for a screenshot instead + * went through the path that resumes a sleeping computer, so looking was enough to start it. + */ +export function computerStatusQueryOptions(botId: string) { + return queryOptions({ + queryKey: computerKeys.status(botId), + queryFn: (): Promise => + client(`/api/computers/${encodeURIComponent(botId)}/status`, { + fallback: "Couldn't check the computer.", + }).then((response) => response.json()), + }); +} + /** * The deployment-wide fleet route. * diff --git a/app/src/lib/computers/screen-connection.ts b/app/src/lib/computers/screen-connection.ts index f540144f12..9a716f11dd 100644 --- a/app/src/lib/computers/screen-connection.ts +++ b/app/src/lib/computers/screen-connection.ts @@ -11,6 +11,16 @@ type Options = { verifyOwnership: () => Promise; onReady: (ready: boolean) => void; onProblem: (problem: string | null) => void; + /** + * Whether a dropped stream is being retried on its own. + * + * Separate from `onProblem` on purpose. A retry in progress is routine (a deploy, a laptop waking, + * a load balancer recycling a connection) and is drawn as a small badge over the screen. A problem + * is what is left once the retries are spent, and only that gets the overlay with a Retry button. + * Both used to arrive through `onProblem`, so every brief blip covered the screen with the same + * panel a dead connection gets. + */ + onReconnecting?: (reconnecting: boolean) => void; onDisconnect: () => void; onMessage: (event: { data: unknown }) => void; schedule?: (run: () => void, delay: number) => () => void; @@ -40,12 +50,13 @@ export function connectScreen(options: Options) { options.onReady(false); const delay = BACKOFF[attempts++]; if (delay === undefined) { + options.onReconnecting?.(false); options.onProblem( "The live screen is disconnected. Retry to reconnect.", ); return; } - options.onProblem("Reconnecting to the live screen…"); + options.onReconnecting?.(true); cancelTimer = schedule(connect, delay); }; current.onopen = async () => { @@ -53,6 +64,7 @@ export function connectScreen(options: Options) { const permitted = attempts === 0 || (await options.verifyOwnership().catch(() => false)); if (!owns()) return; + options.onReconnecting?.(false); options.onProblem(null); options.onReady(permitted); }; @@ -73,6 +85,7 @@ export function connectScreen(options: Options) { ) { stopped = true; cancelTimer?.(); + options.onReconnecting?.(false); options.onDisconnect(); options.onReady(false); options.onProblem( @@ -99,6 +112,7 @@ export function connectScreen(options: Options) { stop: () => { stopped = true; cancelTimer?.(); + options.onReconnecting?.(false); options.onReady(false); socket?.close(); }, diff --git a/app/src/lib/computers/screen.ts b/app/src/lib/computers/screen.ts index d52608af90..a8117b0139 100644 --- a/app/src/lib/computers/screen.ts +++ b/app/src/lib/computers/screen.ts @@ -1,4 +1,5 @@ import { tryClient } from "@/lib/client"; +import type { ComputerState } from "./queries"; /** * One frame of a Bot's screen. @@ -16,6 +17,26 @@ export type Screenshot = { url?: string; }; +/** + * What a screenshot request can come back with. + * + * `notReady` is not an error. The server answers it for a computer that is asleep or starting, and + * does so without waking it; a computer in either state is behaving normally, so the caller draws it + * calmly and keeps polling. `error` is a screenshot that was attempted and failed. + */ +export type ScreenshotRead = { + frame?: Screenshot; + error?: string; + notReady?: { state: ComputerState; reason?: string }; +}; + +const COMPUTER_STATES: readonly ComputerState[] = [ + "absent", + "starting", + "ready", + "unreachable", +]; + /** * Read the current frame. * @@ -25,7 +46,7 @@ export type Screenshot = { */ export async function readScreenshot( computerId: string, -): Promise<{ frame?: Screenshot; error?: string }> { +): Promise { const unavailable = "The screen is not available right now."; try { const response = await tryClient(`/api/computers/${computerId}/screenshot`); @@ -35,12 +56,31 @@ export async function readScreenshot( } | null; return { error: body?.error ?? unavailable }; } - return parseScreenshot(await response.json().catch(() => null)); + const body = (await response.json().catch(() => null)) as unknown; + const notReady = notReadyOf(body); + if (notReady) return { notReady }; + return parseScreenshot(body); } catch { return { error: unavailable }; } } +/** The server's "not ready yet" answer, or nothing when the body is something else. */ +function notReadyOf(body: unknown): ScreenshotRead["notReady"] { + if (!body || typeof body !== "object" || Array.isArray(body)) return; + const { ready, state, reason } = body as { + ready?: unknown; + state?: unknown; + reason?: unknown; + }; + if (ready !== false) return; + if (!COMPUTER_STATES.includes(state as ComputerState)) return; + return { + state: state as ComputerState, + ...(typeof reason === "string" ? { reason } : {}), + }; +} + function parseScreenshot(body: unknown): { frame?: Screenshot; error?: string; diff --git a/app/src/lib/copilot/computer-tools.tsx b/app/src/lib/copilot/computer-tools.tsx index 2e6d2ccc64..aace024639 100644 --- a/app/src/lib/copilot/computer-tools.tsx +++ b/app/src/lib/copilot/computer-tools.tsx @@ -61,6 +61,8 @@ type ComputerOutcome = { refused?: boolean; reason?: string; staleRefs?: boolean; + /** The computer was still starting and the call was not performed. */ + starting?: boolean; elements?: unknown[]; element?: { role?: string; name?: string }; /** What a shell call reports back, so the line can show the output rather than only the command. */ @@ -160,6 +162,7 @@ function ActionLine({ running, refused, failed, + notYet, }: { label: string; detail?: string; @@ -168,21 +171,42 @@ function ActionLine({ refused?: boolean; /** It was permitted and did not work. A different request might. */ failed?: boolean; + /** The computer was still starting, so nothing was done yet. See `stillStarting`. */ + notYet?: boolean; }) { return ( ); } +/** What a line says about a call that waited on a computer still starting. */ +const STILL_STARTING_DETAIL = "The computer is still starting."; + /** Whether a result is an ordinary failure rather than a refusal, so the two can render differently. */ function didNotWork(outcome: ComputerOutcome): boolean { - return outcome.ok === false && outcome.refused !== true; + return ( + outcome.ok === false && + outcome.refused !== true && + outcome.starting !== true + ); +} + +/** + * Whether a call came back because the computer was still starting, and so did nothing. + * + * Not a failure, and not drawn as one. A cold start can outlast the server's wait, and when it does + * the call is turned away before anything reaches the computer. Drawn as "didn't work" in amber, it + * read as a broken computer during the one minute every first start takes. + */ +function stillStarting(outcome: ComputerOutcome): boolean { + return outcome.ok === false && outcome.starting === true; } export function ComputerTools() { @@ -362,6 +386,7 @@ export function ComputerTools() { } refused={outcomeOf(result).refused === true} failed={didNotWork(outcomeOf(result))} + notYet={stillStarting(outcomeOf(result))} /> ), }); @@ -408,6 +433,7 @@ export function ComputerTools() { } refused={outcome.refused === true} failed={didNotWork(outcome)} + notYet={stillStarting(outcome)} /> ); }, @@ -451,6 +477,7 @@ export function ComputerTools() { detail={typeof args?.key === "string" ? args.key : undefined} refused={outcomeOf(result).refused === true} failed={didNotWork(outcomeOf(result))} + notYet={stillStarting(outcomeOf(result))} /> ), }); @@ -625,6 +652,7 @@ export function ComputerTools() { } refused={outcome.refused === true} failed={didNotWork(outcome)} + notYet={stillStarting(outcome)} /> ); }, @@ -671,6 +699,7 @@ export function ComputerTools() { } refused={outcome.refused === true} failed={didNotWork(outcome)} + notYet={stillStarting(outcome)} /> ); }, @@ -748,6 +777,7 @@ export function ComputerTools() { } refused={outcome.refused === true} failed={didNotWork(outcome) || exit !== 0} + notYet={stillStarting(outcome)} > {status === "complete" ? ( ); }, @@ -860,6 +891,7 @@ export function ComputerTools() { label="Scrolled" refused={outcomeOf(result).refused === true} failed={didNotWork(outcomeOf(result))} + notYet={stillStarting(outcomeOf(result))} /> ), }); diff --git a/app/tests/computer-calm-start.test.tsx b/app/tests/computer-calm-start.test.tsx new file mode 100644 index 0000000000..a1c9c63b5a --- /dev/null +++ b/app/tests/computer-calm-start.test.tsx @@ -0,0 +1,335 @@ +import { afterAll, afterEach, beforeAll, expect, test } from "bun:test"; +import { GlobalRegistrator } from "@happy-dom/global-registrator"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { + act, + cleanup, + fireEvent, + render, + waitFor, +} from "@testing-library/react"; +import type { ReactElement } from "react"; +import { ToolLine } from "@/components/channels/tool-line"; +import { ComputerView } from "@/components/computer/computer-view"; + +test("a call turned away by a computer still starting reads as not done yet, not as a failure", () => { + const view = render( + , + ); + const line = view.getByText("Clicked, not done yet"); + expect(line.closest(".text-amber-600")).toBeNull(); + expect(line.closest(".text-destructive")).toBeNull(); + expect(view.queryByText(/didn't work/)).toBeNull(); +}); + +/** + * A computer starting in front of a person never reads as a failure. + * + * The panel used to call the first frameless screenshot a problem, so every cold start opened on + * "Can't show the screen right now… ask an admin", and every blip in the live stream covered the + * screen with a Retry panel. What a person should see instead is a sentence saying the computer is + * starting, a small "Reconnecting…" while the stream comes back, and a problem only when there + * really is one. + */ + +class SocketDouble { + static readonly OPEN = 1; + static readonly CLOSED = 3; + static all: SocketDouble[] = []; + + readyState = SocketDouble.OPEN; + onopen: (() => void) | null = null; + onmessage: ((event: { data: string }) => void) | null = null; + onerror: (() => void) | null = null; + onclose: (() => void) | null = null; + + constructor(_url: string) { + SocketDouble.all.push(this); + queueMicrotask(() => this.onopen?.()); + } + + send() {} + + close() { + this.readyState = SocketDouble.CLOSED; + } +} + +let originalFetch: typeof fetch; +let originalWebSocket: typeof WebSocket; +let originalIntersectionObserver: typeof IntersectionObserver | undefined; +// Prototypes outlive this file: bun runs every test file in one process, and a canvas stub left +// behind here breaks the next file that draws on one. +let originalGetContext: typeof HTMLCanvasElement.prototype.getContext; +let originalDecode: typeof HTMLImageElement.prototype.decode; + +beforeAll(() => { + GlobalRegistrator.register(); + originalFetch = globalThis.fetch; + originalWebSocket = globalThis.WebSocket; + originalIntersectionObserver = globalThis.IntersectionObserver; + originalGetContext = HTMLCanvasElement.prototype.getContext; + originalDecode = HTMLImageElement.prototype.decode; + globalThis.WebSocket = SocketDouble as unknown as typeof WebSocket; + // Without an observer the preview counts as on screen, which is what these cases are about. + Reflect.deleteProperty(globalThis, "IntersectionObserver"); + HTMLImageElement.prototype.decode = () => Promise.resolve(); +}); + +afterEach(() => { + cleanup(); + SocketDouble.all = []; + globalThis.fetch = originalFetch; +}); + +afterAll(() => { + globalThis.fetch = originalFetch; + globalThis.WebSocket = originalWebSocket; + if (originalIntersectionObserver) { + globalThis.IntersectionObserver = originalIntersectionObserver; + } + HTMLCanvasElement.prototype.getContext = originalGetContext; + HTMLImageElement.prototype.decode = originalDecode; + GlobalRegistrator.unregister(); +}); + +function renderWithQueries(ui: ReactElement) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + return render( + {ui}, + ); +} + +const control = { + holder: "bot", + since: new Date(0).toISOString(), + requested: false, +}; + +/** A server whose answers are given per path ending; anything else is a 404. */ +function serve(routes: Record Response>) { + globalThis.fetch = (async (input) => { + const path = String(input); + for (const [ending, answer] of Object.entries(routes)) { + if (path.endsWith(ending)) return answer(); + } + if (path.endsWith("/control")) return Response.json(control); + return new Response("not found", { status: 404 }); + }) as typeof fetch; +} + +const frame = () => + Response.json({ + base64: "AAAA", + width: 320, + height: 200, + capturedAt: new Date(0).toISOString(), + url: "https://example.com", + }); + +test("a computer that is starting says so, calmly, and shows no problem", async () => { + serve({ + "/status": () => Response.json({ botId: "sales", state: "starting" }), + "/screenshot": () => Response.json({ ready: false, state: "starting" }), + }); + + const view = renderWithQueries( + , + ); + + expect( + await view.findByText( + "Starting Sales's computer. The first start takes about a minute.", + ), + ).toBeDefined(); + expect(view.queryByText(/can't show the screen/i)).toBeNull(); + expect(view.queryByText(/error/i)).toBeNull(); +}); + +test("failed screenshots during a start are not a problem until the grace runs out", async () => { + serve({ + "/status": () => Response.json({ botId: "sales", state: "starting" }), + "/screenshot": () => + Response.json( + { error: "The computer did not respond." }, + { status: 503 }, + ), + }); + + const view = renderWithQueries( + , + ); + + await view.findByText(/^Starting Sales's computer/); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 60)); + }); + expect(view.queryByText(/can't show the screen/i)).toBeNull(); + + // Past the grace, with still no frame, it is worth saying. + expect( + await view.findByText( + "Can't show the screen right now", + {}, + { timeout: 1000 }, + ), + ).toBeDefined(); + expect(view.getByText("The computer did not respond.")).toBeDefined(); +}); + +test("a computer the server cannot reach is a problem straight away", async () => { + serve({ + "/status": () => + Response.json({ + botId: "sales", + state: "unreachable", + reason: "supervisor down", + }), + "/screenshot": () => + Response.json({ + ready: false, + state: "unreachable", + reason: "supervisor down", + }), + }); + + const view = renderWithQueries( + , + ); + + expect( + await view.findByText("Sales's computer can't be reached right now."), + ).toBeDefined(); + // The provider's own words are for an admin, not this screen. + expect(view.queryByText(/supervisor down/)).toBeNull(); +}); + +test("a computer that is off, watched from the side panel, says it starts when the Bot uses it", async () => { + serve({ + "/status": () => Response.json({ botId: "sales", state: "absent" }), + "/screenshot": () => Response.json({ ready: false, state: "absent" }), + }); + + const view = renderWithQueries( + , + ); + + expect( + await view.findByText( + "Sales's computer is off. It starts when the Bot next uses it.", + ), + ).toBeDefined(); + // Off is not stalled: waiting past the grace still shows no problem. + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 80)); + }); + expect(view.queryByText(/can't show the screen/i)).toBeNull(); +}); + +test("a live stream that drops shows a quiet badge, not the Retry panel", async () => { + HTMLCanvasElement.prototype.getContext = (() => ({ + drawImage: () => undefined, + clearRect: () => undefined, + })) as unknown as typeof HTMLCanvasElement.prototype.getContext; + serve({ "/screenshot": frame }); + + const view = renderWithQueries( + , + ); + await view.findByAltText("What the Bot is looking at"); + fireEvent.click(view.getByLabelText("Open the Bot's screen full size")); + await waitFor(() => expect(SocketDouble.all.length).toBe(1)); + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + + await act(async () => { + SocketDouble.all[0]?.onclose?.(); + }); + + expect(view.getByText("Reconnecting…")).toBeDefined(); + expect(view.queryByText("Retry")).toBeNull(); +}); + +test("a computer that was off and then fails is not called off for ever", async () => { + // The first answer says off; every one after it is a failed screenshot of a computer now up. + let shots = 0; + serve({ + "/status": () => Response.json({ botId: "sales", state: "ready" }), + "/screenshot": () => + shots++ === 0 + ? Response.json({ ready: false, state: "absent" }) + : Response.json( + { error: "The computer did not respond." }, + { status: 503 }, + ), + }); + + const view = renderWithQueries( + , + ); + + expect( + await view.findByText( + "Can't show the screen right now", + {}, + { timeout: 1000 }, + ), + ).toBeDefined(); + expect(view.queryByText(/is off/)).toBeNull(); +}); + +test("a computer that goes to sleep stops showing its last frame", async () => { + let shots = 0; + serve({ + "/status": () => Response.json({ botId: "sales", state: "absent" }), + "/screenshot": () => + shots++ === 0 + ? frame() + : Response.json({ ready: false, state: "absent" }), + }); + + const view = renderWithQueries( + , + ); + + await view.findByAltText("What the Bot is looking at"); + await waitFor(() => { + expect( + view.getByText( + "Sales's computer is off. It starts when the Bot next uses it.", + ), + ).toBeDefined(); + expect(view.queryByAltText("What the Bot is looking at")).toBeNull(); + }); +}); diff --git a/app/tests/computer-preview-visibility.test.tsx b/app/tests/computer-preview-visibility.test.tsx index c9d727a96f..b4edecea75 100644 --- a/app/tests/computer-preview-visibility.test.tsx +++ b/app/tests/computer-preview-visibility.test.tsx @@ -1,5 +1,7 @@ import { afterAll, afterEach, beforeAll, expect, test } from "bun:test"; import { GlobalRegistrator } from "@happy-dom/global-registrator"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import type { ReactElement } from "react"; import { act, cleanup, render, waitFor } from "@testing-library/react"; import { ComputerView } from "@/components/computer/computer-view"; import { LiveScreen } from "@/components/computer/live-screen"; @@ -137,6 +139,16 @@ afterAll(() => { GlobalRegistrator.unregister(); }); +/** The view reads the computer's state through a query, so it needs a client of its own per test. */ +function renderWithQueries(ui: ReactElement) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + return render( + {ui}, + ); +} + function setVisibility(state: DocumentVisibilityState) { Object.defineProperty(document, "visibilityState", { configurable: true, @@ -264,7 +276,9 @@ test("hidden computer preview pauses screenshot polling while control polling co return new Response("not found", { status: 404 }); }) as typeof fetch; - render(); + renderWithQueries( + , + ); await act(async () => { await new Promise((resolve) => setTimeout(resolve, 30)); }); @@ -297,7 +311,7 @@ test("offscreen computer preview starts screenshot polling when it intersects", return new Response("not found", { status: 404 }); }) as typeof fetch; - render( + renderWithQueries( , ); const entry = IntersectionObserverDouble.observed.at(0); @@ -341,7 +355,9 @@ test("computer preview does not decode a screenshot response that lands after th return new Response("not found", { status: 404 }); }) as typeof fetch; - render(); + renderWithQueries( + , + ); const entry = IntersectionObserverDouble.observed.at(0); expect(entry).toBeDefined(); await act(async () => { diff --git a/app/tests/computer-tool-refusals.test.ts b/app/tests/computer-tool-refusals.test.ts index 8404f7fb68..6f291f83f5 100644 --- a/app/tests/computer-tool-refusals.test.ts +++ b/app/tests/computer-tool-refusals.test.ts @@ -25,6 +25,41 @@ function serverAnswering(status: number, body: unknown) { })) as unknown as typeof fetch; } +describe("a computer still starting", () => { + /* + * A cold start that outlasted the server's wait. The model has to be told the call did nothing and + * is worth repeating; read as an ordinary failure it apologised and gave up on a computer that was + * seconds from ready. + */ + test("is reported as starting and not performed, in the server's own words", async () => { + serverAnswering(503, { + error: + "The computer is still starting. This call was not performed. Try it again in a moment.", + starting: true, + notPerformed: true, + }); + + const outcome = await callComputer("bot-1", "/click", { method: "POST" }); + + expect(outcome).toEqual({ + ok: false, + starting: true, + notPerformed: true, + reason: + "The computer is still starting. This call was not performed. Try it again in a moment.", + }); + }); + + test("an ordinary unavailable computer is not mistaken for one starting", async () => { + serverAnswering(503, { error: "The computer did not respond." }); + + const outcome = await callComputer("bot-1", "/click", { method: "POST" }); + + expect(outcome.starting).toBeUndefined(); + expect(outcome.reason).toBe("The computer did not respond."); + }); +}); + describe("a computer call the server refused", () => { test("a person holding the wheel is reported as that, not as stale refs", async () => { serverAnswering(409, { diff --git a/app/tests/screen-connection.test.ts b/app/tests/screen-connection.test.ts index 42a0a79d76..502a1ec3cf 100644 --- a/app/tests/screen-connection.test.ts +++ b/app/tests/screen-connection.test.ts @@ -55,7 +55,8 @@ test("connection loss disables input, retries boundedly, then asks for Retry", a const r = rig(); r.sockets[0]?.onopen?.(); r.sockets[0]?.onclose?.(); - expect(r.statuses.at(-2)).toBe("blocked"); + // Blocked, and nothing more: a retry in progress is not a problem (see the reconnecting test). + expect(r.statuses.at(-1)).toBe("blocked"); for (let i = 0; i < 5; i++) { r.timers[i]?.run(); r.sockets[i + 1]?.onclose?.(); @@ -107,6 +108,60 @@ test("a superseded viewer stays stopped until explicit Retry", () => { r.control.stop(); }); +/* + * A blip is a badge, not an overlay. Every brief drop used to report "Reconnecting to the live + * screen…" as a problem, and the view drew problems as a panel over the screen with a Retry button, + * so a routine reconnect looked exactly like a dead connection. + */ +test("a drop being retried reports reconnecting, and no problem until the retries run out", async () => { + const sockets: ScreenSocket[] = []; + const timers: (() => void)[] = []; + const problems: (string | null)[] = []; + const reconnecting: boolean[] = []; + connectScreen({ + open: () => { + const socket: ScreenSocket = { + onopen: null, + onclose: null, + onerror: null, + onmessage: null, + close: () => undefined, + }; + sockets.push(socket); + return socket; + }, + schedule: (run) => { + timers.push(run); + return () => undefined; + }, + verifyOwnership: async () => false, + onReady: () => undefined, + onProblem: (problem) => problems.push(problem), + onReconnecting: (value) => reconnecting.push(value), + onDisconnect: () => undefined, + onMessage: () => undefined, + }); + + await sockets[0]?.onopen?.(); + sockets[0]?.onclose?.(); + expect(reconnecting.at(-1)).toBe(true); + expect(problems.filter(Boolean)).toEqual([]); + + // Back on the first retry: the badge goes, and nothing was ever drawn over the screen. + timers[0]?.(); + await sockets[1]?.onopen?.(); + expect(reconnecting.at(-1)).toBe(false); + expect(problems.filter(Boolean)).toEqual([]); + + // Gone for good: only now is it a problem, and the badge is cleared with it. + for (let i = 1; i <= 6; i++) { + sockets[i]?.onclose?.(); + timers[i]?.(); + } + expect(reconnecting.at(-1)).toBe(false); + expect(problems.at(-1)).toContain("Retry"); +}); + test("unmount cancels a pending reconnect and ignores old socket callbacks", () => { const r = rig(); r.sockets[0]?.onclose?.(); diff --git a/server/src/computer/client.ts b/server/src/computer/client.ts index fbc8d59fe1..42fff9ea6b 100644 --- a/server/src/computer/client.ts +++ b/server/src/computer/client.ts @@ -26,6 +26,37 @@ export class ComputerStoppedError extends ComputerUnavailableError { } } +/** + * What every caller is told when a computer did not finish starting in time. + * + * Written for the model and for a person at once, because both read it: the headless tools hand it + * to the model as the result, and the routes send it to the browser, which shows it on the line. + */ +export const STILL_STARTING_MESSAGE = + "The computer is still starting. This call was not performed. Try it again in a moment."; + +/** + * The wait for a cold computer ran out before it could take the call. + * + * A NORMAL WAIT THAT RAN LONG, NOT AN OUTAGE. A computer that was asleep costs a pod schedule and a + * browser launch to wake, and the first start pulls an image as well, so a minute is ordinary and + * two is not unheard of. Reporting that as a failure put red text and "did not become ready" in + * front of somebody whose computer was, at that moment, finishing its start. + * + * A subclass of `ComputerUnavailableError`, so everything that already catches an unavailable + * computer still does. What the type adds is that every surface can say the gentle thing instead: + * the route marks the body `starting`, the headless tools answer with a result rather than a throw, + * and the browser renders it as a wait rather than a failure. The message says the call was NOT + * performed, which is true by construction: this is only thrown before anything reached the + * computer, so a retry cannot do the thing twice. + */ +export class ComputerStillStartingError extends ComputerUnavailableError { + constructor(reason: string = STILL_STARTING_MESSAGE) { + super(reason); + this.name = "ComputerStillStartingError"; + } +} + /** The requested element is not on the current page. */ export class ElementNotFoundError extends Error { constructor(reason: string) { diff --git a/server/src/computer/gateway.ts b/server/src/computer/gateway.ts index 141ac3503e..4f6c5bc15f 100644 --- a/server/src/computer/gateway.ts +++ b/server/src/computer/gateway.ts @@ -27,6 +27,7 @@ import { import { HeadlessToolSuspension } from "@openbot/toolkit/headless-tools"; import { withUntrustedNotice } from "../untrusted-content"; import { + ComputerStillStartingError, ComputerStoppedError, ComputerUnavailableError, createComputerTransport, @@ -35,6 +36,7 @@ import { import { checkComputerAddress } from "./target"; export { + ComputerStillStartingError, ComputerUnavailableError, ElementNotFoundError, HandoffRequestError, @@ -368,12 +370,22 @@ export function createComputerGateway( * has to be decided and recorded, and the attempt failing is what writes the failure row beside the * decision; throwing here would take the action off the trail entirely. So a failure answers * "unknown", which leaves the generation check where it was and leaves the address to the attempt. + * + * EXCEPT A COMPUTER THAT IS STILL STARTING, which is carried back rather than forgotten. Forgetting + * it sent the attempt to `locate` again, and that is a second full cold-start wait stacked on the + * first: a click on a sleeping computer held its request open twice as long before saying + * anything. The attempt throws the carried error instead, inside the block that writes the failure + * row, so the trail still shows a permitted action that was not performed. */ - async function locateForAction(botId: string): Promise { + async function locateForAction( + botId: string, + ): Promise<{ address?: string; starting?: ComputerStillStartingError }> { try { - return await locate(botId); - } catch { - return undefined; + return { address: await locate(botId) }; + } catch (error) { + return error instanceof ComputerStillStartingError + ? { starting: error } + : {}; } } @@ -557,7 +569,8 @@ export function createComputerGateway( * the policy has even seen it. The address that comes back is the one the attempt then uses, so * on the supervisor this costs no extra call for the actions that do need it. */ - const address = ref ? await locateForAction(botId) : undefined; + const located = ref ? await locateForAction(botId) : undefined; + const address = located?.address; const { session } = ref ? await sessionOf(botId) : { session: undefined }; const element = resolve(stored, ref, snapshotId, session); // For a navigation the relevant page is the one being opened, not the one already loaded. Using @@ -659,6 +672,7 @@ export function createComputerGateway( * was written would be a way to act without appearing on the trail. The failure row beside it is * what stops the trail claiming a permitted action was carried out when nothing was sent. */ + if (located?.starting) throw located.starting; if (ref && stored && !element) { throw new StaleSnapshotError( `${ref} is not on the page this computer is showing, so nothing can be checked against it before acting. Take a fresh snapshot and use the refs it returns.`, diff --git a/server/src/computer/headless-tools.ts b/server/src/computer/headless-tools.ts index 0036785d8c..701b9f59b3 100644 --- a/server/src/computer/headless-tools.ts +++ b/server/src/computer/headless-tools.ts @@ -8,6 +8,7 @@ import { import { type ActionActor, type ComputerGateway, + ComputerStillStartingError, HumanHasControlError, } from "./gateway"; import type { ControlState } from "./schema"; @@ -63,6 +64,22 @@ export function createHeadlessComputerTools(options: { try { return await execute(input, context); } catch (error) { + /* + * A cold start that outlasted the wait is answered, not thrown. + * + * Thrown, it reached the model as a bare error string beside every real failure, and a + * model reading "error" tends to apologise and give up on a computer that was seconds from + * ready. As a result it says the three things that decide the next step: the computer is + * starting, this call did nothing, and trying again shortly is the right move. + */ + if (error instanceof ComputerStillStartingError) { + return { + ok: false, + starting: true, + notPerformed: true, + reason: error.message, + }; + } if (error instanceof HumanHasControlError) { throw new HeadlessToolSuspension(error.message, { kind: "computer_handoff", diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index c88016e886..512e3ec8ec 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -10,6 +10,7 @@ import { type ActionActor, ActionRefusedError, type ComputerGateway, + ComputerStillStartingError, ComputerUnavailableError, ElementNotFoundError, HandoffRequestError, @@ -98,9 +99,36 @@ export function createComputerRoutes( return context.json(await gateway.status(botId)); }); + /** + * The frame a watcher polls for, and never the thing that wakes a computer. + * + * `screenshot` goes through `locate`, and `locate` resumes a suspended computer and waits for it. + * Called blind, opening the panel was enough to start a Bot's computer, every poll after that held + * a request open for the length of a cold start, and the first one to give up was drawn as "can't + * show the screen". The computer starts on the first thing a Bot does with it; looking at it is + * not that. + * + * So the status is asked first, which reads the provider's own record and touches nothing. A + * computer that is not ready answers 200 with `ready: false` and its state: an answer, not an + * error, because a computer that is asleep or starting is behaving normally and the panel draws + * it as such. Only an unreachable one is a fault, and the state says that too. + * + * There is a gap between the two calls, and a computer suspended inside it is woken by the + * screenshot. The culler suspends after a long idle, so landing in that gap needs a viewer polling + * at the exact moment of the suspend; the cost is one computer started early, never a wrong frame. + */ routes.get("/:botId/screenshot", async (context) => { + const botId = context.req.param("botId"); try { - return context.json(await gateway.screenshot(context.req.param("botId"))); + const status = await gateway.status(botId); + if (status.state !== "ready") { + return context.json({ + ready: false, + state: status.state, + ...(status.reason ? { reason: status.reason } : {}), + }); + } + return context.json(await gateway.screenshot(botId)); } catch (error) { return context.json(errorBody(error), statusFor(error)); } @@ -994,6 +1022,14 @@ function errorBody(error: unknown): Record { ...(error instanceof HandoffRequestError ? { controlRequestError: true } : {}), + /* + * A cold start that outlasted the wait. Marked so the browser can draw a wait rather than a + * failure, and tell the model the call was not performed and is worth repeating. Without the + * flag it reads as any other unavailable computer, which sends a person looking for an outage. + */ + ...(error instanceof ComputerStillStartingError + ? { starting: true, notPerformed: true } + : {}), }; } diff --git a/server/src/computer/sandbox.ts b/server/src/computer/sandbox.ts index ed9b89660c..8766ae8162 100644 --- a/server/src/computer/sandbox.ts +++ b/server/src/computer/sandbox.ts @@ -15,6 +15,7 @@ * in-cluster service account gives a token and a CA, which is what `inClusterConfig` reads. */ import { readFile } from "node:fs/promises"; +import { ComputerStillStartingError } from "./client"; import type { ComputerLocation, ComputerProvider, @@ -116,10 +117,24 @@ export type SandboxProviderOptions = { token?: string | (() => Promise); ca?: string; fetchImpl?: typeof fetch; - /** How long `locate` waits for a suspended computer to come back before giving up. */ + /** How long a computer is waited on to come back, by `update`'s restart and at most by `locate`. */ resumeTimeoutMs?: number; + /** + * How long one `locate` waits before answering that the computer is still starting. + * + * Shorter than a resume can take ON PURPOSE. `locate` runs inside a person's request, and a load + * balancer with the usual 60s idle timeout cuts a request that says nothing for longer: the browser + * got a bare gateway error, the action could still land once the pod came up, and the model was + * told it had failed. Answering before that line keeps the "not performed" true. Giving up costs + * nothing, because the resume has already been written and the pod keeps coming up; the next call + * waits again. + */ + locateWaitMs?: number; }; +/** See `locateWaitMs`. Under a 60s load balancer idle timeout, with room for the call itself. */ +const LOCATE_WAIT_MS = 45_000; + /** * The service account this pod was given, which is how it reaches the API server. * @@ -250,6 +265,10 @@ export function createSandboxComputerProvider( } { const doFetch = options.fetchImpl ?? fetch; const resumeTimeoutMs = options.resumeTimeoutMs ?? 120_000; + const locateWaitMs = Math.min( + resumeTimeoutMs, + options.locateWaitMs ?? LOCATE_WAIT_MS, + ); /** Restarts started by `update` and not yet finished, one per Bot. */ const lastRestart = new Map>(); const base = () => @@ -320,17 +339,31 @@ export function createSandboxComputerProvider( }; } - async function waitForReady(botId: string): Promise { - const deadline = Date.now() + resumeTimeoutMs; + async function waitForReady( + botId: string, + budgetMs: number = resumeTimeoutMs, + ): Promise { + const deadline = Date.now() + budgetMs; for (;;) { const sandbox = await read(botId); if (sandbox && isReady(sandbox) && sandbox.status?.serviceFQDN) { return sandbox; } if (Date.now() >= deadline) { - throw new SandboxError( - `The computer for ${botId} did not become ready within ${Math.round(resumeTimeoutMs / 1000)}s. A resume costs a pod schedule and a browser launch, so this is a real wait rather than a failure, but it has to end somewhere.`, + /* + * A resume costs a pod schedule and a browser launch, so this is a real wait rather than a + * failure, but it has to end somewhere. When it does, the caller is told the computer is + * still starting and nothing was done, which is the truth: the pod is very likely seconds + * away. The operator's version, with the Bot and the budget, goes to the log. + */ + console.warn( + JSON.stringify({ + type: "computer-sandbox-still-starting", + botId, + waitedMs: budgetMs, + }), ); + throw new ComputerStillStartingError(); } await new Promise((resolve) => setTimeout(resolve, 1_000)); } @@ -458,7 +491,7 @@ export function createSandboxComputerProvider( // A computer that is running on an older image is left alone here. It may be in the middle of // a task, and restarting it under the Bot is the person's decision, which is `update`. - const ready = await waitForReady(botId); + const ready = await waitForReady(botId, locateWaitMs); const fqdn = ready.status?.serviceFQDN; if (!fqdn) { throw new SandboxError( diff --git a/server/src/computer/supervisor.ts b/server/src/computer/supervisor.ts index 5e25f06130..3e8254adb9 100644 --- a/server/src/computer/supervisor.ts +++ b/server/src/computer/supervisor.ts @@ -13,6 +13,7 @@ * honest about being one shared computer. */ +import { ComputerStillStartingError } from "./client"; import type { ComputerLocation, ComputerProvider } from "./provider"; import type { ComputerStatus } from "./schema"; @@ -24,6 +25,8 @@ type SupervisorComputerLocation = { /** Where to reach it, decided by the supervisor rather than assembled here. */ url?: string; startedAt?: string; + /** The image's healthcheck verdict, from a supervisor new enough to report it. */ + health?: string; }; export type SupervisorOptions = { @@ -75,7 +78,18 @@ export function createDockerSupervisorProvider( */ const sessions = new Map(); - async function call(path: string, method = "POST"): Promise { + async function call( + path: string, + method = "POST", + /** + * Whether running out of time means the computer is still starting. + * + * Only for `/ensure`, which is the one call that waits on a container coming up: there a timeout + * is a slow start, and the caller is told so gently. Anywhere else it is a supervisor that has + * stopped answering, which is a real fault and keeps its own message. + */ + startingOnTimeout = false, + ): Promise { let response: Response; try { response = await doFetch(`${base}${path}`, { @@ -86,6 +100,13 @@ export function createDockerSupervisorProvider( signal: AbortSignal.timeout(timeoutMs), }); } catch (error) { + if ( + startingOnTimeout && + error instanceof Error && + error.name === "TimeoutError" + ) { + throw new ComputerStillStartingError(); + } throw new SupervisorError( `The container supervisor at ${base} could not be reached (${error instanceof Error ? error.message : String(error)}).`, ); @@ -96,8 +117,18 @@ export function createDockerSupervisorProvider( stopped?: boolean; reset?: boolean; computers?: SupervisorComputerLocation[]; + starting?: boolean; } | null; if (!response.ok) { + // The supervisor's own health wait ran out on a container that is up and still booting. The + // same slow start as a timeout here, so it gets the same gentle answer. + if ( + startingOnTimeout && + response.status === 503 && + body?.starting === true + ) { + throw new ComputerStillStartingError(); + } throw new SupervisorError( body?.error ?? `The supervisor answered ${response.status}.`, ); @@ -136,7 +167,11 @@ export function createDockerSupervisorProvider( const rawStatus = location.status.toLowerCase(); switch (rawStatus) { case "running": - return { botId, state: "ready" }; + // Running is the process; healthy is the browser inside it. Until the healthcheck passes, + // `/ensure` holds every caller, so the computer is still starting. + return location.health === "starting" + ? { botId, state: "starting" } + : { botId, state: "ready" }; case "created": case "restarting": return { botId, state: "starting" }; @@ -206,6 +241,8 @@ export function createDockerSupervisorProvider( async locate(botId: string): Promise { const state = (await call( `/computers/${encodeURIComponent(botId)}/ensure`, + "POST", + true, )) as SupervisorComputerLocation; // Recorded on every ensure, so a replaced container is visible to whatever asks next. if (state?.startedAt) sessions.set(botId, state.startedAt); diff --git a/server/tests/assertion-shape.test.ts b/server/tests/assertion-shape.test.ts index 83f13618c2..7790ddbdeb 100644 --- a/server/tests/assertion-shape.test.ts +++ b/server/tests/assertion-shape.test.ts @@ -555,21 +555,21 @@ const ALLOW: Allowed[] = [ }, { file: "server/tests/computer-supervisor.test.ts", - line: 64, + line: 65, shape: "a", finding: "NEW (round 2 did not find this)", why: 'expect(client.locate("sales")).rejects.toThrow(SupervisorError);', }, { file: "server/tests/computer-supervisor.test.ts", - line: 74, + line: 75, shape: "a", finding: "NEW (round 2 did not find this)", why: 'expect(client.locate("bad id")).rejects.toThrow( "A bot id may contain only letters.", );', }, { file: "server/tests/computer-supervisor.test.ts", - line: 88, + line: 89, shape: "a", finding: "NEW (round 2 did not find this)", why: 'expect(client.locate("sales")).rejects.toThrow(/could not be reached/);', @@ -709,7 +709,7 @@ const ALLOW: Allowed[] = [ }, { file: "server/tests/computer-sandbox.test.ts", - line: 78, + line: 79, shape: "b", finding: "NEW (round 2 did not find this)", why: 'expect( await providerWith(suspended).sessionOf?.("knowledge"), ).toBeUndefined();', diff --git a/server/tests/computer-routes.test.ts b/server/tests/computer-routes.test.ts index 3c4eae8873..705d4ad1fe 100644 --- a/server/tests/computer-routes.test.ts +++ b/server/tests/computer-routes.test.ts @@ -1,7 +1,10 @@ import { describe, expect, test } from "bun:test"; import type { MiddlewareHandler } from "hono"; import type { AppVariables, AuthenticatedActor } from "../src/auth/guards"; -import type { ComputerGateway } from "../src/computer/gateway"; +import { + type ComputerGateway, + ComputerStillStartingError, +} from "../src/computer/gateway"; import type { PolicyStore } from "../src/computer/policy-store"; import { createComputerRoutes } from "../src/computer/routes"; @@ -9,6 +12,7 @@ describe("computer routes", () => { test("gets a screenshot through the governed computer gateway", async () => { const requestedBotIds: string[] = []; const gateway = { + status: async (botId: string) => ({ botId, state: "ready" }), screenshot: async (botId: string) => { requestedBotIds.push(botId); return { image: "aGVsbG8=", mimeType: "image/png" as const }; @@ -41,6 +45,94 @@ describe("computer routes", () => { }); }); +/** + * Watching a computer must not start it, and a slow start must not read as a failure. + * + * `screenshot` resumes a suspended computer on the way, so the panel polling it once a second used + * to be what woke every Bot it was opened on, and the first poll to give up drew "can't show the + * screen" over a computer that was simply still booting. + */ +describe("a computer that is not ready yet", () => { + function routesFor(gateway: Partial) { + const requireUser: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, + ) => { + context.set("actor", { id: "user-1", email: "u@openbot.test" } as never); + await next(); + }; + return createComputerRoutes( + gateway as ComputerGateway, + {} as PolicyStore, + requireUser, + async () => true, + ); + } + + test.each(["absent", "starting"] as const)( + "answers %s as a state, without taking a screenshot", + async (state) => { + let screenshots = 0; + const routes = routesFor({ + status: async (botId: string) => ({ botId, state }), + screenshot: async () => { + screenshots++; + return { base64: "AAAA", width: 1, height: 1, capturedAt: "" }; + }, + }); + + const response = await routes.request("http://openbot.test/b/screenshot"); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ ready: false, state }); + expect(screenshots).toBe(0); + }, + ); + + test("passes an unreachable computer's reason through, still as an answer", async () => { + const routes = routesFor({ + status: async (botId: string) => ({ + botId, + state: "unreachable", + reason: "The supervisor could not be reached.", + }), + screenshot: async () => { + throw new Error("must not be called"); + }, + }); + + const response = await routes.request("http://openbot.test/b/screenshot"); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ + ready: false, + state: "unreachable", + reason: "The supervisor could not be reached.", + }); + }); + + test("an action whose cold start ran out says it was not performed, and is marked as starting", async () => { + const routes = routesFor({ + navigate: async () => { + throw new ComputerStillStartingError(); + }, + }); + + const response = await routes.request("http://openbot.test/b/navigate", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ url: "https://example.com" }), + }); + + expect(response.status).toBe(503); + const body = (await response.json()) as Record; + expect(body.starting).toBe(true); + expect(body.notPerformed).toBe(true); + expect(String(body.error)).toContain("still starting"); + expect(String(body.error)).toContain("not performed"); + }); +}); + /** * The fleet listing is the one route here that is not about the Bot in its path. * diff --git a/server/tests/computer-sandbox.test.ts b/server/tests/computer-sandbox.test.ts index 09a4c143d7..c37e5eb5ef 100644 --- a/server/tests/computer-sandbox.test.ts +++ b/server/tests/computer-sandbox.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { ComputerStillStartingError } from "../src/computer/client"; import { createSandboxComputerProvider, sandboxNameFor, @@ -107,6 +108,78 @@ describe("telling one run of a Bot's computer from the next", () => { }); }); +/** + * A cold start that runs past the wait is a wait, not a fault. + * + * It used to throw a `SandboxError` whose message explained the pod schedule to whoever was + * watching, and the route answered 500. Now it is the typed "still starting" every surface knows to + * render gently, and the call it interrupted is said to have done nothing. + */ +describe("a computer that takes longer than the wait to start", () => { + const booting = { + metadata: { name: "bot-general-abc" }, + spec: { operatingMode: "Running" }, + status: { + conditions: [ + { type: "Suspended", status: "False" }, + { type: "Ready", status: "False" }, + ], + }, + }; + + test("locate gives up with the still-starting error, not a sandbox failure", async () => { + const provider = createSandboxComputerProvider({ + namespace: "openbot", + template: { podTemplate: { spec: { containers: [] } } }, + idleAfterMs: 60_000, + apiServer: "https://kubernetes.default", + token: "t", + resumeTimeoutMs: 0, + fetchImpl: (async () => + Response.json(booting)) as unknown as typeof fetch, + }); + const warn = console.warn; + console.warn = () => undefined; + try { + const failure = await provider.locate("general").catch((error) => error); + expect(failure).toBeInstanceOf(ComputerStillStartingError); + expect(failure.message).toContain("not performed"); + } finally { + console.warn = warn; + } + }); + + test("locate answers within its own wait, well before a resume's budget runs out", async () => { + // Held under a load balancer's idle timeout, so the person's request is answered, not cut. + const provider = createSandboxComputerProvider({ + namespace: "openbot", + template: { podTemplate: { spec: { containers: [] } } }, + idleAfterMs: 60_000, + apiServer: "https://kubernetes.default", + token: "t", + resumeTimeoutMs: 600_000, + locateWaitMs: 0, + fetchImpl: (async () => + Response.json(booting)) as unknown as typeof fetch, + }); + const warn = console.warn; + console.warn = () => undefined; + try { + const failure = await provider.locate("general").catch((error) => error); + expect(failure).toBeInstanceOf(ComputerStillStartingError); + } finally { + console.warn = warn; + } + }); + + test("status calls one on its way up starting, without touching it", async () => { + const seen: string[] = []; + const status = await providerWith(booting, seen).status("general"); + expect(status.state).toBe("starting"); + expect(seen.every((call) => call.startsWith("GET "))).toBe(true); + }); +}); + describe("naming a Bot's computer in a cluster", () => { test("a bot id that is not a legal name still gets one, and a unique one", () => { // Bot ids are ours and hold anything a person typed; a resource name may not. Two ids that differ diff --git a/server/tests/computer-supervisor.test.ts b/server/tests/computer-supervisor.test.ts index 4fdf48baa1..2597baa26f 100644 --- a/server/tests/computer-supervisor.test.ts +++ b/server/tests/computer-supervisor.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { ComputerStillStartingError } from "../src/computer/client"; import type { ComputerProvider } from "../src/computer/provider"; import { createDockerSupervisorProvider, @@ -88,6 +89,59 @@ describe("locating a Bot's computer", () => { expect(client.locate("sales")).rejects.toThrow(/could not be reached/); }); + test("an ensure that runs out of time is a computer still starting, not a broken supervisor", async () => { + // `/ensure` is the call that waits on a container coming up, so its deadline is a slow start. + const client = createDockerSupervisorProvider({ + baseUrl: "http://supervisor:4300", + fetchImpl: (async () => { + throw new DOMException("The operation timed out.", "TimeoutError"); + }) as unknown as typeof fetch, + }); + const failure = await client.locate("sales").catch((error) => error); + expect(failure).toBeInstanceOf(ComputerStillStartingError); + }); + + test("a timeout on a plain read is still the supervisor's fault", async () => { + const client = createDockerSupervisorProvider({ + baseUrl: "http://supervisor:4300", + fetchImpl: (async () => { + throw new DOMException("The operation timed out.", "TimeoutError"); + }) as unknown as typeof fetch, + }); + await expect(client.stop("sales")).rejects.toThrow(SupervisorError); + }); + + test("an ensure the supervisor answers as still booting is a computer still starting", async () => { + // The supervisor's own health wait ran out on a container that is up, and it says so. + const client = createDockerSupervisorProvider({ + baseUrl: "http://supervisor:4300", + fetchImpl: (async () => + Response.json( + { + error: "The computer started but did not answer within 60000ms.", + starting: true, + }, + { status: 503 }, + )) as unknown as typeof fetch, + }); + const failure = await client.locate("sales").catch((error) => error); + expect(failure).toBeInstanceOf(ComputerStillStartingError); + }); + + test("a 503 from an unreachable Docker is still the supervisor's fault", async () => { + const client = createDockerSupervisorProvider({ + baseUrl: "http://supervisor:4300", + fetchImpl: (async () => + Response.json( + { error: "Docker is not reachable." }, + { status: 503 }, + )) as unknown as typeof fetch, + }); + const failure = await client.locate("sales").catch((error) => error); + expect(failure).toBeInstanceOf(SupervisorError); + expect(failure).not.toBeInstanceOf(ComputerStillStartingError); + }); + test("the bot id is escaped into the path", async () => { let seen = ""; const client = createDockerSupervisorProvider({ @@ -145,6 +199,28 @@ describe("Docker supervisor provider", () => { }, ); + test("a running container whose healthcheck is still starting is starting, not ready", async () => { + const provider = createDockerSupervisorProvider({ + baseUrl: "http://supervisor:4300", + fetchImpl: (async () => + Response.json({ + computers: [ + { botId: "bot", status: "running", health: "starting" }, + { botId: "other", status: "running", health: "healthy" }, + ], + })) as unknown as typeof fetch, + }); + + expect(await provider.status("bot")).toEqual({ + botId: "bot", + state: "starting", + }); + expect(await provider.status("other")).toEqual({ + botId: "other", + state: "ready", + }); + }); + test("reports missing bot as absent", async () => { const provider = createDockerSupervisorProvider({ baseUrl: "http://supervisor:4300", diff --git a/server/tests/headless-tools.test.ts b/server/tests/headless-tools.test.ts index db4a7c218d..6f71853e38 100644 --- a/server/tests/headless-tools.test.ts +++ b/server/tests/headless-tools.test.ts @@ -9,7 +9,10 @@ import { import { Observable } from "rxjs"; import { parseApprovalContinuation } from "../src/approvals/types"; import type { AuditEventInput } from "../src/audit"; -import { createComputerGateway } from "../src/computer/gateway"; +import { + ComputerStillStartingError, + createComputerGateway, +} from "../src/computer/gateway"; import * as headless from "../src/computer/headless-tools"; import { HeadlessToolsMiddleware } from "@openbot/toolkit/headless-tools"; import { createTurnRunner, type RunnerLike } from "@openbot/toolkit/run-turn"; @@ -386,6 +389,75 @@ test("scheduled computer actions carry the owner and routine into both policy an ); }); +/** + * A cold start that outlasts the wait reaches the model as an answer it can act on. + * + * Thrown, it was one more error string; the model apologised and gave up on a computer that was + * seconds from ready. And a click used to wait for the start twice, once to learn the run and again + * to send, so it is also pinned that the wait happens once. + */ +test("a computer still starting answers 'not performed, try again' rather than throwing, after one wait", async () => { + let locates = 0; + const audit: AuditEventInput[] = []; + const gateway = createComputerGateway({ + provider: { + name: "test", + isolation: "per-bot", + locate: async () => { + locates++; + throw new ComputerStillStartingError(); + }, + status: async (botId) => ({ botId, state: "starting" }), + list: async () => [], + stop: async () => ({ wasRunning: false }), + reset: async () => ({ cleared: false }), + }, + auditStore: { + insert: async (event) => { + audit.push(event); + }, + }, + policy: () => ({ mode: "enforce", allow: ["true"], deny: [] }), + fetchImpl: (async () => { + throw new Error("nothing should reach the computer"); + }) as unknown as typeof fetch, + }); + const tools = headless.createHeadlessComputerTools({ + gateway, + botId: "bot", + actor: { id: "owner", userId: "owner" }, + }); + const invoke = (name: string, args: unknown) => { + const tool = tools.find((candidate) => candidate.definition.name === name); + if (!tool) throw new Error(`Missing ${name}`); + return tool.execute(args, { + toolCallId: name, + signal: new AbortController().signal, + }); + }; + + const navigated = await invoke("computer_navigate", { + url: "https://example.com", + }); + expect(navigated).toMatchObject({ + ok: false, + starting: true, + notPerformed: true, + }); + expect(String((navigated as { reason?: string }).reason)).toContain( + "not performed", + ); + + locates = 0; + const clicked = await invoke("computer_click", { ref: "e1", snapshotId: 7 }); + expect(clicked).toMatchObject({ ok: false, starting: true }); + expect(locates).toBe(1); + // Still on the trail: allowed, then the failure that says it did not happen. + expect(audit.map((event) => event.eventType)).toContain( + "computer.action_failed", + ); +}); + test("the headless computer adapter executes shipped browser, command, and file tools through the gateway", async () => { expect(typeof headless.createHeadlessComputerTools).toBe("function"); const { gateway, received, audit } = computerHarness(); diff --git a/shared/bot-prompt.test.ts b/shared/bot-prompt.test.ts index 2f5d14f34d..453332936e 100644 --- a/shared/bot-prompt.test.ts +++ b/shared/bot-prompt.test.ts @@ -28,4 +28,11 @@ describe("COMPUTER_GUIDANCE", () => { expect(COMPUTER_GUIDANCE).toContain("Opening a page is not a web search."); expect(COMPUTER_GUIDANCE).toContain("cannot search"); }); + + test("says what it is about to do before its first computer action", () => { + // A first action can sit through a cold start; a sentence first means the wait has a reason. + expect(COMPUTER_GUIDANCE).toContain( + "Before your first computer action in a turn, say in one sentence what you are about to do and where.", + ); + }); }); diff --git a/shared/bot-prompt.ts b/shared/bot-prompt.ts index 306e97566b..abd8feeb46 100644 --- a/shared/bot-prompt.ts +++ b/shared/bot-prompt.ts @@ -65,6 +65,7 @@ const COMPUTER_GUIDANCE_LINES = [ "Some actions are refused by this deployment's policy. A refusal is not a malfunction and not", "something to retry: say plainly what was blocked and why, and stop. Do not try another route to", "the same thing.", + "Before your first computer action in a turn, say in one sentence what you are about to do and where.", "Say what you found or did in plain language, briefly.", ]; diff --git a/supervisor/src/docker.ts b/supervisor/src/docker.ts index d339054b37..5418092691 100644 --- a/supervisor/src/docker.ts +++ b/supervisor/src/docker.ts @@ -1,4 +1,5 @@ import Docker from "dockerode"; +import { type ContainerHealth, healthOf } from "./health"; import { BOT_LABEL, type ComputerNames, @@ -93,6 +94,13 @@ export type ComputerState = { port?: number; /** Where to reach it, however it is arranged. */ url?: string; + /** + * What the image's healthcheck says, when Docker reports one. + * + * `running` only means the process exists. A container still `starting` here is booting its + * browser, and `ensure` would hold the caller until it is healthy, so a status read says so. + */ + health?: ContainerHealth; }; /** @@ -197,6 +205,9 @@ export async function listOwned(): Promise { botId: container.Labels?.[BOT_LABEL] ?? "unknown", container: (container.Names?.[0] ?? "").replace(/^\//, ""), status: container.State, + ...(healthOf(container.Status) + ? { health: healthOf(container.Status) } + : {}), ...(container.Created ? { startedAt: new Date(container.Created * 1000).toISOString() } : {}), diff --git a/supervisor/src/health.ts b/supervisor/src/health.ts new file mode 100644 index 0000000000..d55a825b47 --- /dev/null +++ b/supervisor/src/health.ts @@ -0,0 +1,20 @@ +/** + * Reading a container's healthcheck from the status line Docker writes. + * + * In its own file rather than in `docker.ts` for the reason `names.ts` is: that file imports + * `dockerode` at module scope, so a test of a one-line parse would need the Docker client installed + * merely to be imported. + */ + +/** What the image's healthcheck says, when Docker reports one. */ +export type ContainerHealth = "starting" | "healthy" | "unhealthy"; + +/** The healthcheck verdict Docker writes into a container's status line, e.g. "Up 3 seconds (health: starting)". */ +export function healthOf( + status: string | undefined, +): ContainerHealth | undefined { + const match = /\((?:health: )?(starting|healthy|unhealthy)\)/.exec( + status ?? "", + ); + return match?.[1] as ContainerHealth | undefined; +} diff --git a/supervisor/src/index.ts b/supervisor/src/index.ts index e995abc30d..6478e07eed 100644 --- a/supervisor/src/index.ts +++ b/supervisor/src/index.ts @@ -124,12 +124,14 @@ app.post("/computers/:botId/ensure", async (context) => { } // Not ready is a 503 like an outage is, because the caller's next move is the same: wait and // ask again. The message is what differs, and it is the part an operator acts on. - if ( - error instanceof DockerUnavailableError || - error instanceof ComputerNotAnsweringError - ) { + if (error instanceof DockerUnavailableError) { return context.json({ error: error.message }, 503); } + // Marked `starting`, because the container is up and still booting: the server tells the Bot + // the call was not performed and is worth repeating, instead of reporting an outage. + if (error instanceof ComputerNotAnsweringError) { + return context.json({ error: error.message, starting: true }, 503); + } throw error; } }); diff --git a/supervisor/tests/health.test.ts b/supervisor/tests/health.test.ts new file mode 100644 index 0000000000..6b54ad9b1d --- /dev/null +++ b/supervisor/tests/health.test.ts @@ -0,0 +1,15 @@ +import { describe, expect, test } from "bun:test"; +import { healthOf } from "../src/health"; + +describe("reading a container's healthcheck from its status line", () => { + test.each([ + ["Up 3 seconds (health: starting)", "starting"], + ["Up 5 minutes (healthy)", "healthy"], + ["Up 1 minute (unhealthy)", "unhealthy"], + ["Up 2 hours", undefined], + ["Exited (0) 3 minutes ago", undefined], + [undefined, undefined], + ] as const)("%s", (status, expected) => { + expect(healthOf(status)).toBe(expected); + }); +});