From 7b2239f9935781daa16ab18d062b519b91d4ab77 Mon Sep 17 00:00:00 2001 From: Aniruddha Adak Date: Sun, 4 Oct 2026 17:06:22 +0530 Subject: [PATCH] fix(browser): a stopped turn is not a broken browser openOwned records the session as broken on any failure: } catch (error) { await this.save(owner, { ...value, url: target, status: "error", ... }, id); throw error; } One of those failures is the person asking for the turn to end. request reports a cancelled request by throwing the signal's own abort, so a Stop that lands while the worker is still opening the session wrote "error" to a browser whose profile, cookies and storage are all fine. That status is not cosmetic. The app reads it as "Needs attention", and it is what hides the preview behind a fallback and offers "Reconnect browser" instead of "Take control" - so stopping a turn left the session demanding a reconnect the person did not need, and the error survived a restart because it is persisted. Only the caller's own abort is treated as a Stop. The 45s request timeout is a real failure and leaves the signal untouched, so it still records, and the new test for that side is what keeps this from becoming "never record an error". browserFixture's handler is now awaited so a test can model a worker that has not answered yet, which is the state a Stop actually arrives into. --- apps/server/src/browser.ts | 18 ++++++++++----- tests/browser.test.ts | 46 ++++++++++++++++++++++++++++++++++++++ tests/helpers/browser.ts | 9 ++++++-- 3 files changed, 66 insertions(+), 7 deletions(-) diff --git a/apps/server/src/browser.ts b/apps/server/src/browser.ts index 5675d67b3..1de55d87c 100644 --- a/apps/server/src/browser.ts +++ b/apps/server/src/browser.ts @@ -136,11 +136,19 @@ export class BrowserService { const response = await this.request("/sessions", { id, url: target }, signal); return await this.save(owner, await response.json(), id); } catch (error) { - await this.save( - owner, - { ...value, url: target, status: "error", updatedAt: new Date().toISOString() }, - id, - ); + // A Stop is not a broken session. The person asked for the turn to end, the profile and its + // cookies are intact, and `request` reports a cancelled request by throwing the signal's own + // abort - so writing "error" here is what puts "Needs attention" and "Reconnect browser" in + // front of somebody whose browser is working, and hides the preview behind a status they did + // not cause. Only the caller's own abort counts: the 45s request timeout is a real failure and + // leaves the signal untouched, so it still records. + if (!signal?.aborted) { + await this.save( + owner, + { ...value, url: target, status: "error", updatedAt: new Date().toISOString() }, + id, + ); + } throw error; } } diff --git a/tests/browser.test.ts b/tests/browser.test.ts index 7308ae05d..c4773fc9e 100644 --- a/tests/browser.test.ts +++ b/tests/browser.test.ts @@ -263,6 +263,52 @@ test("cancelled chat browser requests do not start navigation or a follow-up rea assert.equal((await db.list("owner", "browsers")).length, 1); }); +test("stopping a turn does not leave the browser marked as needing attention", async (t) => { + const controller = new AbortController(); + let arrived = 0; + const { db, service } = await browserFixture(t, async () => { + arrived += 1; + // The worker has the request and has not answered, which is the state a Stop arrives into. + return new Promise(() => {}); + }); + + const stopped = service.observeForThread( + "owner", + "chat-thread", + savedSession.url, + controller.signal, + ); + // Abort only once the request is genuinely in flight, so this exercises the cancellation path + // rather than the pre-flight check at the top of observeForThread. + while (arrived === 0) await new Promise((resolve) => setTimeout(resolve, 5)); + controller.abort(); + + await assert.rejects(stopped, { name: "AbortError" }); + + const [stored] = await db.list("owner", "browsers"); + // The turn was stopped, not broken. "error" is what the app reads as "Needs attention", hides the + // preview behind, and offers "Reconnect browser" for - none of which a person needs after asking + // for a turn to end, and none of which their profile stopped being true for. + assert.equal(stored?.status, "idle"); +}); + +test("a worker that genuinely fails a turn still marks the browser as needing attention", async (t) => { + // The other half of the rule: only the caller's own Stop is silent. A failure the worker actually + // reported must keep recording, or this would hide a browser that really is broken. + const { db, service } = await browserFixture(t, () => ({ + status: 500, + data: { error: { message: "The browser could not be launched." } }, + })); + + await assert.rejects( + service.observeForThread("owner", "chat-thread", savedSession.url), + /could not be launched/, + ); + + const [stored] = await db.list("owner", "browsers"); + assert.equal(stored?.status, "error"); +}); + test("browser read fails on missing page text instead of inventing observation content", async (t) => { const { db, service } = await browserFixture(t, () => ({ data: { url: savedSession.url, title: savedSession.title }, diff --git a/tests/helpers/browser.ts b/tests/helpers/browser.ts index 41292879e..e15cdb6f2 100644 --- a/tests/helpers/browser.ts +++ b/tests/helpers/browser.ts @@ -13,13 +13,18 @@ import { Files } from "../../apps/server/src/files.ts"; export async function browserFixture( t: TestContext, - handle: (path: string, body: Record) => { status?: number; data: unknown }, + handle: ( + path: string, + body: Record, + ) => { status?: number; data: unknown } | Promise<{ status?: number; data: unknown }>, ) { const server = createServer(async (request, response) => { const chunks = []; for await (const chunk of request) chunks.push(Buffer.from(chunk)); const body = chunks.length ? JSON.parse(Buffer.concat(chunks).toString()) : {}; - const result = handle(request.url ?? "", body); + // Awaited so a handler can model a worker that has not answered yet, which is the state a + // Stop arrives into. + const result = await handle(request.url ?? "", body); response.writeHead(result.status ?? 200, { "content-type": "application/json" }); response.end(JSON.stringify(result.data)); });