diff --git a/apps/extension/src/long-screenshot/background.test.ts b/apps/extension/src/long-screenshot/background.test.ts index e204fe01e..062299786 100644 --- a/apps/extension/src/long-screenshot/background.test.ts +++ b/apps/extension/src/long-screenshot/background.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { attachLongScreenshot } from "./background"; import { capturePage } from "./capture"; import { captureManual } from "./manual"; +import { pruneScreenshots } from "./retention"; import { removeTiledScreenshot, TileWriter } from "./tiles"; import { type CaptureReply, @@ -16,6 +17,7 @@ vi.mock("./source", () => ({ openScreenshotSource: vi.fn(async () => ({ capture: vi.fn(), close: vi.fn() })), })); vi.mock("./manual", () => ({ captureManual: vi.fn() })); +vi.mock("./retention", () => ({ pruneScreenshots: vi.fn(async () => {}) })); vi.mock("./tiles", () => ({ TileWriter: vi.fn( class { @@ -115,6 +117,7 @@ beforeEach(() => { ); vi.mocked(TileWriter).mockClear(); vi.mocked(removeTiledScreenshot).mockClear(); + vi.mocked(pruneScreenshots).mockReset().mockResolvedValue(undefined); vi.mocked(captureManual) .mockReset() .mockImplementation( @@ -140,6 +143,101 @@ describe("independent screenshot jobs", () => { height: 2600, }; + it("prunes on startup while protecting the restored result and open previews", async () => { + const { api, call } = setup(completed); + const [tab] = await api.tabs.query(); + const pendingPreview = { + ...tab, + id: 9, + url: "about:blank", + pendingUrl: "chrome-extension://extension/long-screenshot.html?id=pending-preview", + }; + api.tabs.query.mockResolvedValue([ + tab, + { ...tab, id: 8, url: "chrome-extension://extension/long-screenshot.html?id=open-preview" }, + pendingPreview, + ]); + await call("status"); + const keep = vi.mocked(pruneScreenshots).mock.calls[0][0]; + expect(await keep(completed.id)).toBe(true); + expect(await keep("open-preview")).toBe(true); + expect(await keep("pending-preview")).toBe(true); + expect(await keep("unused")).toBe(false); + }); + + it("prunes again when previews close, releasing their protection", async () => { + const { api, call } = setup(); + const [tab] = await api.tabs.query(); + api.tabs.query.mockResolvedValue([ + tab, + { ...tab, id: 8, url: "chrome-extension://extension/long-screenshot.html?id=preview" }, + ]); + await call("status"); + expect(await vi.mocked(pruneScreenshots).mock.calls.at(-1)![0]("preview")).toBe(true); + api.tabs.query.mockResolvedValue([tab]); + api.tabs.onRemoved.emit(8); + await vi.waitFor(() => expect(pruneScreenshots).toHaveBeenCalledTimes(2)); + expect(await vi.mocked(pruneScreenshots).mock.calls.at(-1)![0]("preview")).toBe(false); + }); + + it("protects previews opened during a sweep and their loaded document during pending navigation", async () => { + const { api, call } = setup(); + await call("status"); + const keep = vi.mocked(pruneScreenshots).mock.calls[0][0]; + expect(await keep("late-preview")).toBe(false); + const [tab] = await api.tabs.query(); + const preview = { + ...tab, + id: 8, + url: "chrome-extension://extension/long-screenshot.html?id=late-preview", + pendingUrl: "https://example.com/next", + }; + api.tabs.query.mockResolvedValue([tab, preview]); + expect(await keep("late-preview")).toBe(true); + api.tabs.query.mockResolvedValue([tab]); + api.tabs.onUpdated.emit(8, { url: "https://example.com/next" }); + await vi.waitFor(() => expect(pruneScreenshots).toHaveBeenCalledTimes(2)); + expect(await vi.mocked(pruneScreenshots).mock.calls.at(-1)![0]("late-preview")).toBe(false); + }); + + it("protects a preview still being created while a new capture replaces the result", async () => { + const { api, call } = setup(completed); + await call("status"); + let created!: () => void; + api.tabs.create.mockImplementationOnce( + () => + new Promise((resolve) => { + created = () => resolve({}); + }), + ); + const opening = call("preview", { id: completed.id }); + await vi.waitFor(() => expect(api.tabs.create).toHaveBeenCalledOnce()); + await call("start"); + await vi.waitFor(() => expect(capturePage).toHaveBeenCalledOnce()); + const keep = vi.mocked(pruneScreenshots).mock.calls.at(-1)![0]; + expect(await keep(completed.id)).toBe(true); + created(); + await opening; + expect(await keep(completed.id)).toBe(false); + resolveCapture({ width: 0, height: 0, blob: new Blob() }); + await vi.waitFor(async () => + expect(await call("status")).toMatchObject({ state: { phase: "error" } }), + ); + }); + + it("keeps captures usable when retention storage temporarily fails", async () => { + vi.mocked(pruneScreenshots).mockRejectedValue(new Error("Storage busy")); + vi.mocked(capturePage).mockImplementationOnce(async (deps) => { + await deps.write({} as ImageBitmap, 800, 0, 0, 512); + return { width: 800, height: 512 }; + }); + const { api, call } = setup(); + await call("start"); + await vi.waitFor(() => expect(api.tabs.create).toHaveBeenCalledOnce()); + expect(await call("status")).toMatchObject({ state: { phase: "complete" } }); + expect(pruneScreenshots).toHaveBeenCalledTimes(3); + }); + it("retains the result when the popup reopens on its source page or its own preview", async () => { const { api, call } = setup(completed); expect(await call("status")).toMatchObject({ state: completed }); diff --git a/apps/extension/src/long-screenshot/background.ts b/apps/extension/src/long-screenshot/background.ts index a18eedc65..9125ce8a4 100644 --- a/apps/extension/src/long-screenshot/background.ts +++ b/apps/extension/src/long-screenshot/background.ts @@ -2,6 +2,7 @@ import { i18n } from "@browser-skill/i18n"; import { capturePage } from "./capture"; import { captureManual } from "./manual"; import { createPageClient } from "./page-client"; +import { pruneScreenshots } from "./retention"; import { openScreenshotSource } from "./source"; import { readTiledScreenshot, removeTiledScreenshot, TileWriter } from "./tiles"; import { @@ -26,8 +27,9 @@ export function attachLongScreenshot(options: { isTabBusy(tabId: number): boolea page?: (command: PageCommand) => Promise; }; let running: Job | null = null; - let openingPreviews = 0; + const openingPreviews = new Map(); let writes = Promise.resolve(); + let pruning = Promise.resolve(); const ready = chrome.storage.session .get(LONG_SCREENSHOT_STATE) .then(async (stored) => { @@ -48,7 +50,34 @@ export function attachLongScreenshot(options: { isTabBusy(tabId: number): boolea ); } }) - .catch(() => {}); + .catch(() => {}) + .then(() => prune()); + function prune() { + pruning = pruning + .then(async () => { + const previewUrl = chrome.runtime.getURL("/long-screenshot.html"); + const inUse = (id: string) => + id === running?.id || id === state?.id || openingPreviews.has(id); + await pruneScreenshots(async (id) => { + if (inUse(id)) return true; + // Disk reads can overlap new previews. Revalidate immediately before + // deletion, including the document still visible during navigation. + const tabs = await chrome.tabs.query({}); + return ( + inUse(id) || + tabs.some((tab) => + [tab.url, tab.pendingUrl].some( + (url) => + url?.startsWith(`${previewUrl}?`) && new URL(url).searchParams.get("id") === id, + ), + ) + ); + }); + }) + // Retention is best effort; storage recovery retries on the next lifecycle event. + .catch(() => {}); + return pruning; + } function publish(next: CaptureState | null) { state = next; writes = writes @@ -57,14 +86,17 @@ export function attachLongScreenshot(options: { isTabBusy(tabId: number): boolea } function clearResult() { // Active jobs own their cancellation and partial-result lifecycle. - if (state && !running && !openingPreviews && !isCapturing(state)) publish(null); + if (state && !running && !openingPreviews.size && !isCapturing(state)) { + publish(null); + void prune(); + } } async function clearResultOnPageChange() { await ready; const previous = state; - if (!previous || running || openingPreviews || isCapturing(previous)) return; + if (!previous || running || openingPreviews.size || isCapturing(previous)) return; const [tab] = await chrome.tabs.query({ active: true, lastFocusedWindow: true }); - if (state !== previous || running || openingPreviews || !tab) return; + if (state !== previous || running || openingPreviews.size || !tab) return; const pageUrl = tab.pendingUrl ?? tab.url; if (!pageUrl) return; if (tab.id === previous.tabId && (!previous.pageUrl || pageUrl === previous.pageUrl)) return; @@ -89,6 +121,7 @@ export function attachLongScreenshot(options: { isTabBusy(tabId: number): boolea void ready.then(() => { if (state?.tabId === tabId && info.url !== state.pageUrl) clearResult(); else pageChanged(); + void prune(); }); }); chrome.webNavigation.onCommitted.addListener((info) => { @@ -101,6 +134,7 @@ export function attachLongScreenshot(options: { isTabBusy(tabId: number): boolea chrome.tabs.onRemoved.addListener((tabId) => { void ready.then(() => { if (state?.tabId === tabId) clearResult(); + void prune(); }); }); async function preview(id: string) { @@ -109,13 +143,15 @@ export function attachLongScreenshot(options: { isTabBusy(tabId: number): boolea ? `¬ice=${encodeURIComponent(state.notice)}` : ""; // Chrome activates a newly created tab before its preview URL commits. - openingPreviews++; + openingPreviews.set(id, (openingPreviews.get(id) ?? 0) + 1); try { await chrome.tabs.create({ url: chrome.runtime.getURL(`/long-screenshot.html?id=${encodeURIComponent(id)}${notice}`), }); } finally { - openingPreviews--; + const remaining = openingPreviews.get(id)! - 1; + if (remaining) openingPreviews.set(id, remaining); + else openingPreviews.delete(id); } } async function start(requested: "auto" | "manual" | "visible" = "auto") { @@ -194,6 +230,8 @@ export function attachLongScreenshot(options: { isTabBusy(tabId: number): boolea void chrome.runtime.getPlatformInfo().catch(() => {}); }, 20_000); try { + await prune(); + controller.signal.throwIfAborted(); const mode = requested; if (mode === "auto") await client.prepare(); await checkTab(); @@ -306,6 +344,7 @@ export function attachLongScreenshot(options: { isTabBusy(tabId: number): boolea chrome.webNavigation.onBeforeNavigate.removeListener(navigated); chrome.webNavigation.onCommitted.removeListener(navigated); if (running?.id === id) running = null; + void prune(); } if (complete) await preview(id).catch(() => {}); } diff --git a/apps/extension/src/long-screenshot/exports.test.ts b/apps/extension/src/long-screenshot/exports.test.ts index 1b444bbc5..812e62570 100644 --- a/apps/extension/src/long-screenshot/exports.test.ts +++ b/apps/extension/src/long-screenshot/exports.test.ts @@ -3,7 +3,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { SCREENSHOT_CHUNK_BYTES, ScreenshotExports } from "./exports"; import { removeTiledScreenshot } from "./tiles"; -vi.mock("./tiles", () => ({ removeTiledScreenshot: vi.fn(async () => {}) })); +vi.mock("./tiles", async (importOriginal) => ({ + ...(await importOriginal()), + removeTiledScreenshot: vi.fn(async () => {}), +})); let store: ScreenshotExports; let live: Set; diff --git a/apps/extension/src/long-screenshot/exports.ts b/apps/extension/src/long-screenshot/exports.ts index 774b4720b..f6d5eae8e 100644 --- a/apps/extension/src/long-screenshot/exports.ts +++ b/apps/extension/src/long-screenshot/exports.ts @@ -5,7 +5,7 @@ import type { ScreenshotReleaseParams, ScreenshotReleaseResult, } from "@/transport/types"; -import { removeTiledScreenshot } from "./tiles"; +import { removeTiledScreenshot, screenshotRoot } from "./tiles"; export const SCREENSHOT_CHUNK_BYTES = 256 * 1024; const EXPORT_IDLE_MS = 10 * 60_000; @@ -38,10 +38,9 @@ export class ScreenshotExports { } private async removeOrphans() { - const root = await navigator.storage.getDirectory(); let directory: FileSystemDirectoryHandle; try { - directory = await root.getDirectoryHandle("long-screenshots"); + directory = await screenshotRoot(); } catch (error) { if (error instanceof DOMException && error.name === "NotFoundError") return; throw error; diff --git a/apps/extension/src/long-screenshot/long-screenshot.browser.test.ts b/apps/extension/src/long-screenshot/long-screenshot.browser.test.ts index 4b47f0e55..5a38978a0 100644 --- a/apps/extension/src/long-screenshot/long-screenshot.browser.test.ts +++ b/apps/extension/src/long-screenshot/long-screenshot.browser.test.ts @@ -22,6 +22,7 @@ const extensionPath = process.env.BSK_LONG_SCREENSHOT_EXTENSION ?? path.resolve( async function withHarness( run: (h: Awaited>) => Promise, scale = 1, + prepareCapture = true, ) { const server = createServer((req, res) => { res.setHeader("Content-Type", "text/html"); @@ -51,7 +52,9 @@ async function withHarness( await new Promise((resolve) => server.close(() => resolve())); } async function runHarness(send: Send) { - await run(await harness(send, `http://127.0.0.1:${port}`)); + await run( + await harness(send, `http://127.0.0.1:${port}`, undefined, undefined, prepareCapture), + ); } } @@ -60,6 +63,7 @@ async function harness( baseUrl: string, existingPage?: { targetId: string; sessionId: string }, knownOrigin?: string, + prepareCapture = true, ) { const targets = () => send<{ targetInfos: { type: string; url: string; targetId: string }[] }>("Target.getTargets"); @@ -145,7 +149,8 @@ async function harness( ); await new Promise((resolve) => setTimeout(resolve, 500)); await send("Page.enable", {}, page.sessionId); - await send("Page.captureScreenshot", { format: "png", fromSurface: true }, page.sessionId); + if (prepareCapture) + await send("Page.captureScreenshot", { format: "png", fromSurface: true }, page.sessionId); const request = (action: string, extra: object = {}) => evaluate( popup.sessionId, @@ -223,6 +228,55 @@ async function harness( describe.skipIf(!process.env.BSK_LONG_SCREENSHOT_CHROME)( "full-page screenshot in a real extension", () => { + it("prunes stored artifacts while retaining open previews and Agent exports", async () => { + await withHarness( + async (h) => { + const id = (index: number) => + `00000000-0000-4000-8000-${String(index).padStart(12, "0")}`; + const preview = await h.create(`${h.origin}/long-screenshot.html?id=${id(7)}`, true); + await h.poll(() => h.evaluate(preview.sessionId, "document.readyState === 'complete'")); + await h.evaluate( + h.popup.sessionId, + `(async () => { + const root = await (await navigator.storage.getDirectory()).getDirectoryHandle('long-screenshots', {create: true}); + for (let index = 1; index <= 8; index++) { + const id = '00000000-0000-4000-8000-' + String(index).padStart(12, '0'); + const directory = await root.getDirectoryHandle(id, {create: true}); + const manifest = await (await directory.getFileHandle('index.json', {create: true})).createWritable(); + await manifest.write(JSON.stringify({id, createdAt: index >= 7 ? 1 : Date.now() - index * 1000})); + await manifest.close(); + const image = await (await directory.getFileHandle('0.png', {create: true})).createWritable(); + await image.write('retained artifact bytes'); + await image.close(); + } + await root.getDirectoryHandle('agent-retained', {create: true}); + })()`, + ); + const stored = () => + h.evaluate( + h.popup.sessionId, + `(async () => { + const root = await (await navigator.storage.getDirectory()).getDirectoryHandle('long-screenshots'); + const ids = []; + for await (const id of root.keys()) ids.push(id); + return ids.sort(); + })()`, + ); + const trigger = await h.create("about:blank", true); + await h.send("Target.closeTarget", { targetId: trigger.targetId }); + await expect + .poll(stored, { timeout: 10000 }) + .toEqual([...[1, 2, 3, 4, 5, 7].map(id), "agent-retained"]); + await h.send("Target.closeTarget", { targetId: preview.targetId }); + await expect + .poll(stored, { timeout: 10000 }) + .toEqual([...[1, 2, 3, 4, 5].map(id), "agent-retained"]); + }, + 1, + false, + ); + }, 30000); + it("clears the completed popup result on another tab while keeping its open preview usable", async () => { await withHarness(async (h) => { await h.startFromPopup(); diff --git a/apps/extension/src/long-screenshot/retention.test.ts b/apps/extension/src/long-screenshot/retention.test.ts new file mode 100644 index 000000000..502df800d --- /dev/null +++ b/apps/extension/src/long-screenshot/retention.test.ts @@ -0,0 +1,162 @@ +import { afterEach, beforeEach, describe, expect, it, type Mock, vi } from "vitest"; +import { pruneScreenshots, SCREENSHOT_HISTORY_AGE_MS, SCREENSHOT_HISTORY_LIMIT } from "./retention"; + +const now = 2 * SCREENSHOT_HISTORY_AGE_MS; +const id = (index: number) => `00000000-0000-4000-8000-${String(index).padStart(12, "0")}`; +type Entry = { manifest?: string; kind?: string; readError?: Error; deleteError?: Error }; +let files: Map; +let removed: string[]; +let remove: Mock<(key: string, options: { recursive: boolean }) => Promise>; +let getRoot: Mock<() => Promise>>; + +function directory() { + return { + async *entries() { + for (const [key, entry] of files) yield [key, { kind: entry.kind ?? "directory" }]; + }, + removeEntry: remove, + getDirectoryHandle: async (key: string) => ({ + getFileHandle: async () => ({ + getFile: async () => { + const entry = files.get(key); + if (entry?.readError) throw entry.readError; + if (entry?.manifest === undefined) throw new DOMException("Missing", "NotFoundError"); + return new Blob([entry.manifest]); + }, + }), + }), + }; +} + +beforeEach(() => { + files = new Map(); + removed = []; + remove = vi.fn(async (key: string, options: { recursive: boolean }) => { + expect(options.recursive).toBe(true); + const error = files.get(key)?.deleteError; + if (error) throw error; + files.delete(key); + removed.push(key); + }); + getRoot = vi.fn(async () => directory()); + vi.stubGlobal("navigator", { + storage: { getDirectory: async () => ({ getDirectoryHandle: getRoot }) }, + }); +}); +afterEach(() => vi.unstubAllGlobals()); + +function capture(index: number, createdAt = now - index) { + files.set(id(index), { manifest: JSON.stringify({ id: id(index), createdAt }) }); + return id(index); +} + +describe("popup screenshot retention", () => { + it("keeps only the newest five captures and removes entire old artifact directories", async () => { + for (const index of [8, 2, 5, 1, 6, 3, 7, 4]) capture(index); + await pruneScreenshots(() => false, now); + expect(files.size).toBe(SCREENSHOT_HISTORY_LIMIT); + expect([...files.keys()].sort()).toEqual([1, 2, 3, 4, 5].map(id)); + expect(removed).toEqual([6, 7, 8].map(id)); + }); + + it("expires captures at the age limit even below the count limit", async () => { + capture(1, now - SCREENSHOT_HISTORY_AGE_MS + 1); + capture(2, now - SCREENSHOT_HISTORY_AGE_MS); + await pruneScreenshots(() => false, now); + expect([...files.keys()]).toEqual([id(1)]); + }); + + it("protects open previews and active captures, then reclaims them after release", async () => { + const protectedIds = new Set([capture(1, 1), capture(2, 2)]); + files.set(id(3), {}); // Active capture has not committed its first manifest yet. + protectedIds.add(id(3)); + for (let index = 4; index <= 9; index++) capture(index); + await pruneScreenshots((key) => protectedIds.has(key), now); + expect([...files.keys()].sort()).toEqual([1, 2, 3, 4, 5, 6, 7, 8].map(id)); + protectedIds.clear(); + await pruneScreenshots(() => false, now); + expect([...files.keys()]).toEqual([4, 5, 6, 7, 8].map(id)); + }); + + it("never evicts protected previews even when they exceed the history limit", async () => { + const protectedIds = new Set(Array.from({ length: 8 }, (_, index) => capture(index, 1))); + await pruneScreenshots((key) => protectedIds.has(key), now); + expect(files.size).toBe(8); + expect(remove).not.toHaveBeenCalled(); + }); + + it("rechecks ownership when a capture starts during storage reads", async () => { + capture(1, 1); + let active = false; + const getDirectoryHandle = (await getRoot()).getDirectoryHandle; + getRoot.mockResolvedValueOnce({ + async *entries() { + yield [id(1), { kind: "directory" }]; + active = true; + }, + getDirectoryHandle, + removeEntry: remove, + }); + await pruneScreenshots(() => active, now); + expect(remove).not.toHaveBeenCalled(); + }); + + it("removes unprotected crash leftovers and invalid manifests", async () => { + files.set(id(1), {}); + files.set(id(2), { manifest: "{broken" }); + files.set(id(3), { manifest: '{"createdAt":"yesterday"}' }); + await pruneScreenshots(() => false, now); + expect(files.size).toBe(0); + }); + + it("awaits current preview ownership before deleting an expired artifact", async () => { + capture(1, 1); + let protect!: (value: boolean) => void; + const keep = vi.fn( + () => + new Promise((resolve) => { + protect = resolve; + }), + ); + const sweep = pruneScreenshots(keep, now); + await vi.waitFor(() => expect(keep).toHaveBeenCalledWith(id(1))); + expect(remove).not.toHaveBeenCalled(); + protect(true); + await sweep; + expect(files.has(id(1))).toBe(true); + }); + + it("does not touch Agent exports, unrelated directories or files", async () => { + files.set("agent-orphan", {}); + files.set("unrelated", {}); + files.set(id(1), { kind: "file" }); + await pruneScreenshots(() => false, now); + expect(files.size).toBe(3); + expect(remove).not.toHaveBeenCalled(); + }); + + it("leaves unreadable captures intact instead of treating storage errors as corruption", async () => { + files.set(id(1), { readError: new DOMException("Storage unavailable", "UnknownError") }); + capture(2, 1); + await pruneScreenshots(() => false, now); + expect([...files.keys()]).toEqual([id(1)]); + }); + + it("continues past a failed deletion and can retry it on the next sweep", async () => { + capture(1, 1); + capture(2, 2); + files.get(id(2))!.deleteError = new DOMException("Busy", "NoModificationAllowedError"); + await expect(pruneScreenshots(() => false, now)).rejects.toThrow("Busy"); + expect([...files.keys()]).toEqual([id(2)]); + files.get(id(2))!.deleteError = undefined; + await pruneScreenshots(() => false, now); + expect(files.size).toBe(0); + }); + + it("accepts a missing root but surfaces other storage failures", async () => { + getRoot.mockRejectedValueOnce(new DOMException("Missing", "NotFoundError")); + await expect(pruneScreenshots(() => false, now)).resolves.toBeUndefined(); + getRoot.mockRejectedValueOnce(new DOMException("Storage unavailable", "UnknownError")); + await expect(pruneScreenshots(() => false, now)).rejects.toThrow("Storage unavailable"); + }); +}); diff --git a/apps/extension/src/long-screenshot/retention.ts b/apps/extension/src/long-screenshot/retention.ts new file mode 100644 index 000000000..75f1e8c6a --- /dev/null +++ b/apps/extension/src/long-screenshot/retention.ts @@ -0,0 +1,47 @@ +import { readTiledScreenshot, screenshotRoot } from "./tiles"; + +export const SCREENSHOT_HISTORY_LIMIT = 5; +export const SCREENSHOT_HISTORY_AGE_MS = 24 * 60 * 60 * 1000; +const POPUP_ID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; + +/** Keep recent popup captures and exempt any still in use. Agent exports have a + * separate lifecycle. Query current ownership only when about to delete a directory. */ +export async function pruneScreenshots( + keep: (id: string) => boolean | Promise, + now = Date.now(), +) { + let root: FileSystemDirectoryHandle; + try { + root = await screenshotRoot(); + } catch (error) { + if (error instanceof DOMException && error.name === "NotFoundError") return; + throw error; + } + const entries: { id: string; createdAt: number }[] = []; + for await (const [id, handle] of root.entries()) { + if (handle.kind !== "directory" || !POPUP_ID.test(id)) continue; + try { + const shot = await readTiledScreenshot(id); + entries.push({ id, createdAt: Number.isFinite(shot?.createdAt) ? shot!.createdAt : 0 }); + } catch (error) { + if (error instanceof SyntaxError) entries.push({ id, createdAt: 0 }); + // A transient storage error is not evidence that a capture is corrupt. + } + } + entries.sort((a, b) => b.createdAt - a.createdAt || a.id.localeCompare(b.id)); + let retained = 0; + let failure: unknown; + for (const { id, createdAt } of entries) { + if (createdAt > now - SCREENSHOT_HISTORY_AGE_MS && retained < SCREENSHOT_HISTORY_LIMIT) { + retained++; + continue; + } + try { + if (await keep(id)) continue; + await root.removeEntry(id, { recursive: true }); + } catch (error) { + if (!(error instanceof DOMException && error.name === "NotFoundError")) failure ??= error; + } + } + if (failure) throw failure; +} diff --git a/apps/extension/src/long-screenshot/storage.ts b/apps/extension/src/long-screenshot/storage.ts index 857947362..f540ddee9 100644 --- a/apps/extension/src/long-screenshot/storage.ts +++ b/apps/extension/src/long-screenshot/storage.ts @@ -9,7 +9,6 @@ export interface SavedScreenshot { const DATABASE = "bsk-long-screenshots"; const STORE = "screenshots"; -const MAX_AGE = 24 * 60 * 60 * 1000; async function openDatabase(): Promise { return new Promise((resolve, reject) => { @@ -24,31 +23,6 @@ async function openDatabase(): Promise { }); } -export async function saveScreenshot(value: SavedScreenshot) { - const db = await openDatabase(); - try { - await new Promise((resolve, reject) => { - const transaction = db.transaction(STORE, "readwrite"); - const store = transaction.objectStore(STORE); - let retained = 0; - const cursor = store.index("createdAt").openCursor(null, "prev"); - cursor.onsuccess = () => { - const entry = cursor.result; - if (!entry) return; - if ((entry.value as SavedScreenshot).createdAt < Date.now() - MAX_AGE || retained++ >= 4) - entry.delete(); - entry.continue(); - }; - store.put(value); - transaction.oncomplete = () => resolve(); - transaction.onerror = () => reject(transaction.error); - transaction.onabort = () => reject(transaction.error); - }); - } finally { - db.close(); - } -} - export async function readScreenshot(id: string): Promise { const db = await openDatabase(); try { diff --git a/apps/extension/src/long-screenshot/tiles.ts b/apps/extension/src/long-screenshot/tiles.ts index ee065432d..9f9e1217b 100644 --- a/apps/extension/src/long-screenshot/tiles.ts +++ b/apps/extension/src/long-screenshot/tiles.ts @@ -15,9 +15,13 @@ export interface TiledScreenshot { notice?: CaptureError; } +export async function screenshotRoot(create = false) { + return (await navigator.storage.getDirectory()).getDirectoryHandle(ROOT, { create }); +} + export async function screenshotDirectory(id: string, create = false) { if (!/^[a-zA-Z0-9-]{1,100}$/.test(id)) throw new Error("Invalid screenshot id"); - const root = await (await navigator.storage.getDirectory()).getDirectoryHandle(ROOT, { create }); + const root = await screenshotRoot(create); return root.getDirectoryHandle(id, { create }); } async function writeFile(directory: FileSystemDirectoryHandle, name: string, data: Blob | string) { @@ -42,7 +46,7 @@ export async function readTiledScreenshot(id: string): Promise + (["commit", "domcontentloaded", "load", "networkidle"] as const).map((phase) => ({ + operation, + phase, + })), + ), +)("recovers same-document $operation at the $phase checkpoint", async ({ operation, phase }) => { + const f = await fixture(); + const same = { ...f.main, documentId: "old-document" }; + f.deps.browserNavigation.getFrame.mockReset().mockResolvedValue({ documentId: same.documentId }); + const action = async () => { + f.state.blocked = false; + f.events.onReferenceFragmentUpdated.fire(same); + }; + f.deps.browserNavigation.update.mockImplementation(action); + f.deps.browserNavigation.goBack.mockImplementation(action); + f.deps.browserNavigation.goForward.mockImplementation(action); + const params = { session_id: "test", wait_until: phase }; + const result = + operation === "navigate" + ? await handleNavigate(f.manager, { ...params, url }, f.deps) + : operation === "back" + ? await handleNavigateBack(f.manager, params, f.deps) + : await handleNavigateForward(f.manager, params, f.deps); + expect(result).toMatchObject({ reached: phase, final_url: url }); + f.expectCleanedUp(); +}); + +it.each([ + "different-document", + "child-frame", + "wrong-url", + "reload", +])("rejects %s same-document evidence in browser recovery", async (kind) => { + const f = await fixture(); + f.deps.defaultTimeoutMs = 20; + f.deps.browserNavigation.getFrame.mockReset().mockResolvedValue({ documentId: "old-document" }); + const action = async () => { + f.state.blocked = false; + f.events.onReferenceFragmentUpdated.fire({ + ...f.main, + documentId: kind === "different-document" ? "other-document" : "old-document", + frameId: kind === "child-frame" ? 1 : 0, + url: kind === "wrong-url" ? `${url}#unrelated` : url, + }); + }; + f.deps.browserNavigation.update.mockImplementation(action); + f.deps.browserNavigation.reload.mockImplementation(action); + const result = + kind === "reload" + ? await handleReload(f.manager, { session_id: "test" }, f.deps) + : await handleNavigate(f.manager, { session_id: "test", url }, f.deps); + expect(result).toMatchObject({ reached: "timeout" }); + f.expectCleanedUp(); +}); + +it("keeps waiting for load when a recovered same-document page is still loading", async () => { + const f = await fixture(); + f.state.emitIdle = false; + f.deps.browserNavigation.getFrame.mockReset().mockResolvedValue({ documentId: "old-document" }); + f.deps.browserNavigation.update.mockImplementation(async () => { + f.state.blocked = false; + f.events.onHistoryStateUpdated.fire({ ...f.main, documentId: "old-document" }); + }); + let settled = false; + const work = handleNavigate(f.manager, { session_id: "test", url }, f.deps).then((result) => { + settled = true; + return result; + }); + await vi.waitFor(() => expect(f.cdpEvents.listeners.size).toBe(1)); + expect(settled).toBe(false); + f.cdpEvents.fire({ tabId: 4 }, "Page.lifecycleEvent", { + frameId: "main", + loaderId: "new-loader", + name: "load", + }); + expect(await work).toMatchObject({ reached: "load" }); + f.expectCleanedUp(); +}); + +it("cancels same-document browser recovery while awaiting lifecycle evidence", async () => { + const f = await fixture(); + const abort = new AbortController(); + f.state.emitIdle = false; + f.deps.browserNavigation.getFrame.mockReset().mockResolvedValue({ documentId: "old-document" }); + f.deps.browserNavigation.update.mockImplementation(async () => { + f.state.blocked = false; + f.events.onReferenceFragmentUpdated.fire({ ...f.main, documentId: "old-document" }); + }); + const work = handleNavigate( + f.manager, + { session_id: "test", url }, + { ...f.deps, signal: abort.signal }, + ); + await vi.waitFor(() => expect(f.cdpEvents.listeners.size).toBe(1)); + abort.abort(); + expect(await work).toMatchObject({ code: "cancelled" }); + f.expectCleanedUp(); +}); + +it("follows a new document that replaces a same-document browser handoff", async () => { + const f = await fixture(); + f.state.emitIdle = false; + let current = "old-document"; + f.deps.browserNavigation.getFrame + .mockReset() + .mockImplementation(async () => ({ documentId: current })); + f.deps.browserNavigation.update.mockImplementation(async () => { + f.state.blocked = false; + f.events.onReferenceFragmentUpdated.fire({ ...f.main, documentId: "old-document" }); + }); + let settled = false; + const work = handleNavigate(f.manager, { session_id: "test", url }, f.deps).then((result) => { + settled = true; + return result; + }); + await vi.waitFor(() => expect(f.cdpEvents.listeners.size).toBe(1)); + const next = { ...f.main, documentId: "successor", url: `${url}/next` }; + f.events.onBeforeNavigate.fire(next); + current = "successor"; + f.events.onCommitted.fire(next); + f.events.onCompleted.fire({ ...f.main, documentId: "old-document" }); + await vi.waitFor(() => expect(f.cdpEvents.listeners.size).toBe(0)); + expect(settled).toBe(false); + f.events.onCompleted.fire(next); + expect(await work).toMatchObject({ reached: "load", final_url: next.url }); + f.expectCleanedUp(); +}); diff --git a/apps/extension/src/tools/__tests__/navigation-same-document.browser.test.ts b/apps/extension/src/tools/__tests__/navigation-same-document.browser.test.ts new file mode 100644 index 000000000..98f492d47 --- /dev/null +++ b/apps/extension/src/tools/__tests__/navigation-same-document.browser.test.ts @@ -0,0 +1,137 @@ +// @vitest-environment node +// Opt in with BSK_NAV_CHROME. The shared harness owns its browser and profile. +import { createServer } from "node:http"; +import { describe, expect, it } from "vitest"; +import { SessionManager } from "@/session-manager/manager"; +import { handleNavigate, handleNavigateBack, handleNavigateForward } from "../navigation"; +import type { CdpRunner } from "../shared"; + +type Send = >( + method: string, + params?: object, + sessionId?: string, +) => Promise; +type Listener = Parameters>[0]; +interface CdpEvent { + sessionId?: string; + method: string; + params?: object; +} +interface FrameTree { + frameTree: { frame: { id: string; loaderId: string } }; +} + +describe.skipIf(!process.env.BSK_NAV_CHROME)("real browser same-document navigation", () => { + it("reuses the document for navigate, back and forward at every lifecycle checkpoint", async () => { + const server = createServer((_request, response) => { + response.setHeader("Content-Type", "text/html"); + response.end("Navigation fixture
Same document
"); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const url = `http://127.0.0.1:${(server.address() as { port: number }).port}/`; + let sessionId: string | undefined; + const listeners = new Set(); + try { + const { withChrome } = await import( + new URL( + "../../../../../evals/browser/cases/regression/snapshot-coordinates/chrome.mjs", + import.meta.url, + ).href + ); + await withChrome( + { + executable: process.env.BSK_NAV_CHROME, + deviceScale: 1, + zoom: 1, + startupTimeout: 30_000, + onEvent: (event: CdpEvent) => { + if (sessionId && event.sessionId === sessionId) + for (const listener of [...listeners]) + listener({ tabId: 4 }, event.method, event.params); + }, + }, + async (send: Send) => { + const { targetId } = await send<{ targetId: string }>("Target.createTarget", { url }); + ({ sessionId } = await send<{ sessionId: string }>("Target.attachToTarget", { + targetId, + flatten: true, + })); + const evaluate = async (expression: string): Promise => { + const reply = await send<{ result: { value: T }; exceptionDetails?: unknown }>( + "Runtime.evaluate", + { expression, returnByValue: true }, + sessionId, + ); + expect(reply.exceptionDetails).toBeUndefined(); + return reply.result.value; + }; + const ready = () => + evaluate( + "document.readyState === 'complete' && document.title === 'Navigation fixture'", + ); + for (let attempt = 0; attempt < 100 && !(await ready()); attempt += 1) + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(await ready(), "the initial document must finish loading").toBe(true); + const initial = await send("Page.getFrameTree", {}, sessionId); + const manager = new SessionManager({ + agentWindow: { + create: async () => ({ windowId: 100, initialTabIds: [4] }), + ensureActiveTab: async () => 4, + remove: async () => {}, + }, + }); + await manager.start("navigation"); + const cdp: CdpRunner = { + send: (_tabId: number, method: string, params?: object) => + send(method, params, sessionId), + onEvent: (listener) => { + listeners.add(listener); + return { + dispose: () => { + listeners.delete(listener); + }, + }; + }, + }; + const tab = async () => + ({ + id: 4, + windowId: 100, + active: true, + url: await evaluate("location.href"), + }) as chrome.tabs.Tab; + const deps = { + cdp, + tabsApi: { get: tab, query: async () => [await tab()] }, + defaultTimeoutMs: 5_000, + }; + for (const phase of ["commit", "domcontentloaded", "load", "networkidle"] as const) { + const previousUrl = await evaluate("location.href"); + const destination = `${url}#${phase}`; + const params = { session_id: "navigation", wait_until: phase }; + for (const operation of ["navigate", "back", "forward"] as const) { + const result = + operation === "navigate" + ? await handleNavigate(manager, { ...params, url: destination }, deps) + : operation === "back" + ? await handleNavigateBack(manager, params, deps) + : await handleNavigateForward(manager, params, deps); + expect(result, `${operation}/${phase}`).toMatchObject({ + reached: phase, + final_url: operation === "back" ? previousUrl : destination, + }); + const current = await send("Page.getFrameTree", {}, sessionId); + expect(current.frameTree.frame.loaderId).toBe(initial.frameTree.frame.loaderId); + expect(listeners.size).toBe(0); + } + } + }, + ); + } finally { + listeners.clear(); + await new Promise((resolve, reject) => + server.close((error) => (error ? reject(error) : resolve())), + ); + } + }, 60_000); +}); diff --git a/apps/extension/src/tools/__tests__/navigation-same-document.test.ts b/apps/extension/src/tools/__tests__/navigation-same-document.test.ts new file mode 100644 index 000000000..2434826da --- /dev/null +++ b/apps/extension/src/tools/__tests__/navigation-same-document.test.ts @@ -0,0 +1,226 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { SessionManager } from "@/session-manager/manager"; +import type { WaitUntil } from "@/transport/types"; +import { + handleNavigate, + handleNavigateBack, + handleNavigateForward, + handleReload, +} from "../navigation"; +import type { CdpRunner } from "../shared"; + +const destination = "https://example.test/page#two"; +type Listener = Parameters>[0]; +type Operation = "navigate" | "back" | "forward" | "reload"; + +async function fixture() { + const manager = new SessionManager({ + agentWindow: { + create: async () => ({ windowId: 100, initialTabIds: [4] }), + ensureActiveTab: async () => 4, + remove: async () => {}, + }, + }); + await manager.start("task"); + const listeners = new Set(); + const state = { + loader: "original", + navigateLoader: undefined as string | undefined, + readyState: "complete", + replay: ["commit", "DOMContentLoaded", "load", "networkIdle"], + action: () => {}, + }; + const fire = (method: string, params: object, tabId = 4) => { + for (const listener of [...listeners]) listener({ tabId }, method, params); + }; + const sameDocument = (url = destination, frameId = "main") => + fire("Page.navigatedWithinDocument", { frameId, url, navigationType: "fragment" }); + const lifecycle = (name: string, loaderId = state.loader, frameId = "main") => + fire("Page.lifecycleEvent", { name, frameId, loaderId }); + const send = vi.fn(async (_tabId: number, method: string) => { + if (method === "Runtime.evaluate") return { result: { value: state.readyState } }; + if (method === "Page.getFrameTree") + return { frameTree: { frame: { id: "main", loaderId: state.loader } } }; + if (method === "Page.getNavigationHistory") + return { + currentIndex: 1, + entries: [ + { id: 1, url: destination }, + { id: 2, url: "https://example.test/page#one" }, + { id: 3, url: destination }, + ], + }; + if (["Page.navigate", "Page.navigateToHistoryEntry", "Page.reload"].includes(method)) { + state.action(); + return { frameId: "main", loaderId: state.navigateLoader }; + } + if (method === "Page.setLifecycleEventsEnabled") + for (const name of state.replay) lifecycle(name); + return {}; + }); + const cdp: CdpRunner = { + send: send as CdpRunner["send"], + onEvent: (listener) => { + listeners.add(listener); + return { dispose: () => listeners.delete(listener) }; + }, + }; + const tab = { id: 4, windowId: 100, active: true, url: destination } as chrome.tabs.Tab; + const run = (operation: Operation, wait_until: WaitUntil, signal?: AbortSignal) => { + const deps = { + cdp, + tabsApi: { get: async () => tab, query: async () => [tab] }, + defaultTimeoutMs: 100, + signal, + }; + const params = { session_id: "task", wait_until }; + switch (operation) { + case "navigate": + return handleNavigate(manager, { ...params, url: destination }, deps); + case "back": + return handleNavigateBack(manager, params, deps); + case "forward": + return handleNavigateForward(manager, params, deps); + case "reload": + return handleReload(manager, params, deps); + } + }; + return { state, run, fire, sameDocument, lifecycle, send, listeners }; +} + +beforeEach(() => vi.useFakeTimers()); +afterEach(() => vi.useRealTimers()); + +describe("same-document navigation", () => { + it.each( + (["navigate", "back", "forward"] as const).flatMap((operation) => + (["commit", "domcontentloaded", "load", "networkidle"] as const).flatMap((phase) => + [false, true].map((late) => ({ operation, phase, late })), + ), + ), + )("reuses the document for $operation/$phase (event after reply: $late)", async ({ + operation, + phase, + late, + }) => { + const f = await fixture(); + f.state.action = () => { + if (late) setTimeout(() => f.sameDocument(), 1); + else f.sameDocument(); + }; + const work = f.run(operation, phase); + await vi.advanceTimersByTimeAsync(2); + expect(await work).toMatchObject({ reached: phase }); + expect(f.listeners.size).toBe(0); + }); + + it.each([ + "wrong-url", + "child-frame", + "different-tab", + "no-event", + ])("does not accept %s as navigation completion", async (kind) => { + const f = await fixture(); + f.state.action = () => { + if (kind === "wrong-url") f.sameDocument("https://example.test/page#unrelated"); + if (kind === "child-frame") f.sameDocument(destination, "child"); + if (kind === "different-tab") + f.fire("Page.navigatedWithinDocument", { frameId: "main", url: destination }, 9); + }; + const work = f.run("navigate", "load"); + await vi.advanceTimersByTimeAsync(100); + expect(await work).toMatchObject({ reached: "timeout" }); + expect(f.listeners.size).toBe(0); + }); + + it("keeps waiting for real network-idle evidence on the retained loader", async () => { + const f = await fixture(); + f.state.replay = ["commit", "DOMContentLoaded", "load"]; + f.state.action = () => f.sameDocument(); + let settled = false; + const work = f.run("navigate", "networkidle").then((result) => { + settled = true; + return result; + }); + await vi.advanceTimersByTimeAsync(1); + expect(settled).toBe(false); + f.lifecycle("networkIdle", "other-loader"); + f.lifecycle("networkIdle", "original", "child"); + await vi.advanceTimersByTimeAsync(1); + expect(settled).toBe(false); + f.lifecycle("networkIdle"); + expect(await work).toMatchObject({ reached: "networkidle" }); + }); + + it("does not accept a same-document event when navigate reports a new loader", async () => { + const f = await fixture(); + f.state.navigateLoader = "new-loader"; + f.state.action = () => f.sameDocument(); + const work = f.run("navigate", "load"); + await vi.advanceTimersByTimeAsync(100); + expect(await work).toMatchObject({ reached: "timeout" }); + }); + + it("does not let an unrelated URL change complete a reload", async () => { + const f = await fixture(); + f.state.action = () => f.sameDocument(); + const work = f.run("reload", "load"); + await vi.advanceTimersByTimeAsync(100); + expect(await work).toMatchObject({ reached: "timeout" }); + }); + + it("follows a cross-document successor after a same-document navigation", async () => { + const f = await fixture(); + f.state.replay = []; + f.state.action = () => f.sameDocument(); + const work = f.run("navigate", "networkidle"); + let settled = false; + void work.then(() => { + settled = true; + }); + await vi.advanceTimersByTimeAsync(1); + f.fire("Page.frameRequestedNavigation", { frameId: "main", disposition: "currentTab" }); + f.state.loader = "successor"; + f.fire("Page.frameNavigated", { frame: { id: "main", loaderId: "successor" } }); + f.lifecycle("networkIdle", "original"); + await vi.advanceTimersByTimeAsync(1); + expect(settled).toBe(false); + f.lifecycle("networkIdle"); + expect(await work).toMatchObject({ reached: "networkidle" }); + }); + + it.each([ + "request-intent", + "network-request", + ])("does not revive a same-document event superseded by %s before navigate replies", async (cause) => { + const f = await fixture(); + f.state.action = () => { + f.sameDocument(); + if (cause === "request-intent") { + f.fire("Page.frameRequestedNavigation", { frameId: "main", disposition: "currentTab" }); + } else { + f.fire("Network.requestWillBeSent", { + frameId: "main", + loaderId: "successor", + type: "Document", + requestId: "next", + }); + } + }; + const work = f.run("navigate", "load"); + await vi.advanceTimersByTimeAsync(100); + expect(await work).toMatchObject({ reached: "timeout" }); + }); + + it("cancels a retained-document wait and removes its event listener", async () => { + const f = await fixture(); + const abort = new AbortController(); + f.state.replay = []; + f.state.action = () => f.sameDocument(); + const work = f.run("navigate", "networkidle", abort.signal); + await vi.advanceTimersByTimeAsync(1); + abort.abort(); + expect(await work).toMatchObject({ code: "cancelled" }); + expect(f.listeners.size).toBe(0); + }); +}); diff --git a/apps/extension/src/tools/browser-navigation.ts b/apps/extension/src/tools/browser-navigation.ts index 946a38b43..33edf97a0 100644 --- a/apps/extension/src/tools/browser-navigation.ts +++ b/apps/extension/src/tools/browser-navigation.ts @@ -73,13 +73,20 @@ export async function navigateWithBrowserApi( waitUntil: WaitUntil, timeoutMs: number, signal?: AbortSignal, - afterCommit?: (event: NavigationEvent, signal: AbortSignal) => Promise, + afterCommit?: ( + event: NavigationEvent, + signal: AbortSignal, + sameDocument: boolean, + ) => Promise, requestedUrl?: string, + allowSameDocument = false, ): Promise { if (signal?.aborted) return { reached: "cancelled" }; if (timeoutMs <= 0) return { reached: "timeout" }; const controller = new AbortController(); let started = false; + let actionStarted = false; + let departingDocumentId: string | undefined; const document = new NavigationDocument(); let handoff = new AbortController(); let acceptedCommit = false; @@ -133,24 +140,14 @@ export async function navigateWithBrowserApi( pendingUrl = details.url; navigationRevision += 1; }; - const onCommitted = (details: NavigationEvent) => { - if (!isMainFrame(details) || settled || document.isRetired(details.documentId)) return; - if ( - !acceptedCommit && - (!started || - (!matchesStart(details) && !details.transitionQualifiers?.includes("server_redirect"))) - ) { - document.retire(details.documentId); - return; - } - if (!document.commit(details.documentId)) return; + const prepareDocument = (details: NavigationEvent, sameDocument = false) => { acceptedCommit = true; pendingUrl = undefined; navigationRevision += 1; resetReadiness(); committedUrl = details.url; lastLifecycle = "commit"; - matched = waitUntil === "commit" || waitUntil === "networkidle"; + matched = sameDocument || waitUntil === "commit" || waitUntil === "networkidle"; const version = document.version; const signal = handoff.signal; // A successor invalidates this handoff without ending the operation. @@ -158,7 +155,7 @@ export async function navigateWithBrowserApi( void Promise.resolve() .then(() => { signal.throwIfAborted(); - return afterCommit?.(details, signal); + return afterCommit?.(details, signal, sameDocument); }) .then( (ready) => { @@ -176,6 +173,19 @@ export async function navigateWithBrowserApi( }, ); }; + const onCommitted = (details: NavigationEvent) => { + if (!isMainFrame(details) || settled || document.isRetired(details.documentId)) return; + if ( + !acceptedCommit && + (!started || + (!matchesStart(details) && !details.transitionQualifiers?.includes("server_redirect"))) + ) { + document.retire(details.documentId); + return; + } + if (!document.commit(details.documentId)) return; + prepareDocument(details); + }; const onDOMContentLoaded = (details: NavigationEvent) => { if (!isDocument(details)) return; lastLifecycle = "DOMContentLoaded"; @@ -209,6 +219,20 @@ export async function navigateWithBrowserApi( } }; const onSameDocument = (details: NavigationEvent) => { + if (!isMainFrame(details) || settled) return; + if ( + !acceptedCommit && + allowSameDocument && + afterCommit && + actionStarted && + details.documentId === departingDocumentId && + matchesStart(details) && + document.reuseInitial(details.documentId) + ) { + started = true; + prepareDocument(details, true); + return; + } if (isDocument(details) && document.pending) void resumeCurrentDocument(); }; const onError = (details: NavigationEvent) => { @@ -253,8 +277,10 @@ export async function navigateWithBrowserApi( void Promise.resolve() .then(async () => { const departing = await api.getFrame(tabId); - document.retire(departing?.documentId); + departingDocumentId = departing?.documentId; + document.retire(departingDocumentId); controller.signal.throwIfAborted(); + actionStarted = true; return action(); }) .then( diff --git a/apps/extension/src/tools/navigation-document.ts b/apps/extension/src/tools/navigation-document.ts index e87aae1ce..cf7230cc6 100644 --- a/apps/extension/src/tools/navigation-document.ts +++ b/apps/extension/src/tools/navigation-document.ts @@ -31,6 +31,13 @@ export class NavigationDocument { return true; } + /** A matching same-document navigation keeps the operation's departing document. */ + reuseInitial(id: string | undefined): boolean { + if (!id || this.id || !this.isRetired(id)) return false; + this.retired.delete(id); + return this.commit(id); + } + isCurrent(version: number): boolean { return this.version === version; } diff --git a/apps/extension/src/tools/navigation.ts b/apps/extension/src/tools/navigation.ts index 2a319d347..8da6e7b62 100644 --- a/apps/extension/src/tools/navigation.ts +++ b/apps/extension/src/tools/navigation.ts @@ -212,6 +212,7 @@ export interface LifecycleWaitGuard { loaderId?: string | (() => string | null | undefined); beforeLoaderId?: string | null; followNavigations?: boolean; + sameDocument?: { frameId: string | null; url: string; allow?: () => boolean }; } function currentLoaderId(guard: LifecycleWaitGuard | undefined): string { @@ -256,6 +257,7 @@ function startLifecycleWait( const pendingRequests = new Set(); let navigationRevision = 0; let pendingLifecycle: { name: string; frameId?: string; loaderId?: string } | null = null; + let pendingSameDocument: { frameId: string; revision: number } | undefined; let listenerSub: { dispose(): void } | null = null; let timer: ReturnType | null = null; let dialogBudgetUsed = false; @@ -328,7 +330,34 @@ function startLifecycleWait( finish({ reached: "match", lastLifecycle: pendingLifecycle.name }); } }; - refresh = maybeFinishPending; + const acceptSameDocument = () => { + if ( + settled || + !pendingSameDocument || + pendingSameDocument.frameId !== currentFrameId() || + pendingSameDocument.revision !== navigationRevision || + guard?.sameDocument?.allow?.() === false || + pendingRequests.size || + !document.reuseInitial(guard?.beforeLoaderId ?? undefined) + ) + return; + pendingSameDocument = undefined; + navigationRevision += 1; + noteRelevantLifecycle("commit"); + if (targetName === "commit") { + finish({ reached: "match", lastLifecycle: "commit" }); + return; + } + // Chrome replays this document's recorded phases, including network idle. + // A reused document will not emit a new load event just for a URL change. + void cdp + .send(expectedTabId, "Page.setLifecycleEventsEnabled", { enabled: true }) + .catch(() => {}); + }; + refresh = () => { + acceptSameDocument(); + maybeFinishPending(); + }; tryProbe = async (mode: LifecycleProbeMode, beforeReadyState: string | null = null) => { if (settled) return; @@ -404,8 +433,8 @@ function startLifecycleWait( if (method === "Page.frameRequestedNavigation") { const p = params as { frameId?: string; disposition?: string }; if ( - currentFrameId() && - p.frameId === currentFrameId() && + p.frameId && + p.frameId === (currentFrameId() || guard.sameDocument?.frameId) && (!p.disposition || p.disposition === "currentTab") ) { document.begin(); @@ -420,12 +449,14 @@ function startLifecycleWait( loaderId?: string; type?: string; }; + const activeLoader = + document.id ?? (pendingSameDocument ? guard.beforeLoaderId : undefined); if ( p.type === "Document" && - p.frameId === currentFrameId() && + p.frameId === (currentFrameId() || guard.sameDocument?.frameId) && p.requestId && - document.id && - p.loaderId !== document.id && + activeLoader && + p.loaderId !== activeLoader && !document.isRetired(p.loaderId) ) { document.begin(); @@ -446,7 +477,16 @@ function startLifecycleWait( method === "Page.frameStoppedLoading" || method === "Page.navigatedWithinDocument" ) { - const p = params as { frameId?: string }; + const p = params as { frameId?: string; url?: string }; + if ( + method === "Page.navigatedWithinDocument" && + p.frameId && + p.frameId === guard.sameDocument?.frameId && + p.url === guard.sameDocument?.url + ) { + pendingSameDocument = { frameId: p.frameId, revision: navigationRevision }; + acceptSameDocument(); + } if (p.frameId === currentFrameId()) void reconcilePending(); return; } @@ -701,6 +741,7 @@ async function recoverBrowserNavigation( waitUntil: WaitUntil, timeoutMs: number, requestedUrl?: string, + allowSameDocument = false, ): Promise { const tabId = target.tabId; const controlled = isAgentControlledTab(ctx, tabId); @@ -727,7 +768,7 @@ async function recoverBrowserNavigation( waitUntil, timeoutMs, abort.signal, - async (event, signal) => { + async (event, signal, sameDocument) => { // Chrome metadata may advance before its corresponding event arrives. // An obsolete handoff waits for that event; it is not a navigation error. const isCurrent = async () => { @@ -745,7 +786,7 @@ async function recoverBrowserNavigation( if (!(await isCurrent())) return false; await deps.cdp.send(tabId, "Page.enable", {}); if (!(await isCurrent())) return false; - if (waitUntil === "networkidle") { + if (waitUntil === "networkidle" || (sameDocument && waitUntil !== "commit")) { const frame = await readMainFrameInfo(deps.cdp, tabId); if (!(await isCurrent())) return false; if (!frame.frameId) throw new Error("No main frame after navigation"); @@ -756,7 +797,7 @@ async function recoverBrowserNavigation( deps.cdp, tabId, frame.frameId, - "networkIdle", + cdpLifecycleName(waitUntil), remaining, waiting.signal, { loaderId: frame.loaderId ?? undefined }, @@ -778,6 +819,7 @@ async function recoverBrowserNavigation( } }, requestedUrl, + allowSameDocument, ); if (outcome.reached === "failed") return outcome.error; if (outcome.reached === "cancelled" || abort.signal.aborted) @@ -838,6 +880,7 @@ export async function handleNavigate( waitUntil, timeoutMs, params.url, + true, ); if (isRpcError(recovered)) return recovered; return attachDialogs(deps.cdp, target.tabId, dialogCursor, { ...recovered, url: params.url }); @@ -845,8 +888,10 @@ export async function handleNavigate( const expected = cdpLifecycleName(waitUntil); const beforeReadyState = await probeMainFrameReadyState(deps.cdp, target.tabId); const beforeFrame = await readMainFrameInfo(deps.cdp, target.tabId); + const sameDocumentUrl = new URL(params.url).href; let frameId = ""; let loaderId = ""; + let sameDocumentNavigation = false; const waitAbort = linkedAbortSignal(deps.signal); const wait = startLifecycleWait( deps.cdp, @@ -859,6 +904,11 @@ export async function handleNavigate( loaderId: () => loaderId, beforeLoaderId: beforeFrame.loaderId, followNavigations: true, + sameDocument: { + frameId: beforeFrame.frameId, + url: sameDocumentUrl, + allow: () => sameDocumentNavigation, + }, }, ); const waitPromise = wait.promise; @@ -894,6 +944,7 @@ export async function handleNavigate( } frameId = nav.frameId ?? ""; loaderId = nav.loaderId ?? ""; + sameDocumentNavigation = !nav.loaderId && !nav.errorText; wait.refresh(); if (nav.errorText) { waitAbort.abort(); @@ -983,6 +1034,8 @@ async function handleHistory( (api) => (direction === "back" ? api.goBack(target.tabId) : api.goForward(target.tabId)), waitUntil, timeoutMs, + undefined, + true, ); if (isRpcError(recovered)) return recovered; return attachDialogs(deps.cdp, target.tabId, dialogCursor, { @@ -1021,7 +1074,11 @@ async function handleHistory( expected, timeoutMs, waitAbort.signal, - { beforeLoaderId: beforeFrame.loaderId, followNavigations: true }, + { + beforeLoaderId: beforeFrame.loaderId, + followNavigations: true, + sameDocument: { frameId: beforeFrame.frameId, url: targetEntry.url }, + }, ); const waitPromise = wait.promise; try { diff --git a/apps/extension/src/tools/vom/__tests__/semantic-resolve.test.ts b/apps/extension/src/tools/vom/__tests__/semantic-resolve.test.ts new file mode 100644 index 000000000..f63e7cabf --- /dev/null +++ b/apps/extension/src/tools/vom/__tests__/semantic-resolve.test.ts @@ -0,0 +1,121 @@ +import { describe, expect, it, vi } from "vitest"; +import type { CapturedNode } from "../capture"; +import { buildSemanticGraph, resolveSemanticGraph, type SemanticAxNode } from "../semantic-graph"; + +function dom(id: number, tag: string, textContent?: string): CapturedNode { + return { + backendNodeId: id, + parentBackendNodeId: id === 1 ? null : 1, + tag, + attrs: {}, + textContent, + rect: { x: 0, y: 0, w: 100, h: 20 }, + paintOrder: id, + position: "static", + pointerEvents: "auto", + }; +} + +function graph(domNodes: CapturedNode[], axNodes: SemanticAxNode[] = []) { + return buildSemanticGraph({ + viewport: { width: 800, height: 600 }, + rootFrameId: "main", + documents: [ + { frameId: "main", contextScopeId: "main", target: { tabId: 1 }, domNodes, axNodes }, + ], + }); +} + +function byBackend(resolved: ReturnType, id: number) { + return [...resolved.nodes.values()].find((node) => node.backendNodeId === id)?.vom; +} + +describe("semantic sibling context", () => { + it.each( + [512, 2048].flatMap((count) => [false, true].map((ax) => ({ count, ax }))), + )("bounds sibling searches linearly for $count nodes (AX: $ax)", ({ count, ax }) => { + const domNodes = [ + dom(1, "section"), + ...Array.from({ length: count }, (_, i) => + dom(i + 2, i % 4 === 3 ? "input" : "div", i % 4 === 3 ? undefined : `Row ${i}`), + ), + ]; + const axNodes: SemanticAxNode[] = ax + ? [ + { + nodeId: "root", + backendDOMNodeId: 1, + childIds: domNodes + .slice(1) + .map((node) => String(node.backendNodeId)) + .reverse(), + }, + ...domNodes.slice(1).map((node) => ({ + nodeId: String(node.backendNodeId), + parentId: "root", + backendDOMNodeId: node.backendNodeId, + role: { type: "role", value: node.tag === "input" ? "textbox" : "StaticText" }, + ...(node.textContent + ? { name: { type: "computedString", value: node.textContent } } + : {}), + })), + ] + : []; + const input = graph(domNodes, axNodes); + const original = Array.prototype.indexOf; + let searchedSlots = 0; + const search = vi.spyOn(Array.prototype, "indexOf").mockImplementation(function ( + this: unknown[], + value: unknown, + fromIndex?: number, + ) { + searchedSlots += this.length; + return original.call(this, value, fromIndex); + }); + let resolved: ReturnType; + try { + resolved = resolveSemanticGraph(input); + } finally { + search.mockRestore(); + } + expect(searchedSlots).toBeLessThanOrEqual(count * 16); + expect(resolved.nodes.size).toBe(count + 1); + expect(byBackend(resolved, 5)?.name).toBe(ax ? "Row 4" : "Row 2"); + expect(byBackend(resolved, 2)?.nearbyText).toBe("Row 1 Row 2"); + }); + + it("uses AX child order for preceding labels and DOM order for nearby text", () => { + const resolved = resolveSemanticGraph( + graph( + [dom(1, "section"), dom(2, "div", "DOM first"), dom(3, "input"), dom(4, "div", "DOM last")], + [ + { nodeId: "root", backendDOMNodeId: 1, childIds: ["label", "field", "tail"] }, + { + nodeId: "field", + parentId: "root", + backendDOMNodeId: 3, + role: { type: "role", value: "textbox" }, + }, + { + nodeId: "tail", + parentId: "root", + backendDOMNodeId: 2, + role: { type: "role", value: "StaticText" }, + name: { type: "computedString", value: "AX tail" }, + }, + { + nodeId: "label", + parentId: "root", + backendDOMNodeId: 4, + role: { type: "role", value: "StaticText" }, + name: { type: "computedString", value: "AX label" }, + }, + ], + ), + ); + expect(byBackend(resolved, 3)).toMatchObject({ + name: "AX label", + nearbyText: "DOM first DOM last", + }); + }); +}); diff --git a/apps/extension/src/tools/vom/semantic-graph/resolve.ts b/apps/extension/src/tools/vom/semantic-graph/resolve.ts index 81e7a7dde..7ff9ea5dd 100644 --- a/apps/extension/src/tools/vom/semantic-graph/resolve.ts +++ b/apps/extension/src/tools/vom/semantic-graph/resolve.ts @@ -178,6 +178,8 @@ function externalHrefHost( interface ResolveIndexes { domChildren: Map; axChildren: Map; + domSiblingIndex: Map; + axSiblingIndex: Map; domIdIndex: Map; labelForIndex: Map; domText: (nodeId: SemanticNodeId) => string | undefined; @@ -188,11 +190,14 @@ interface ResolveIndexes { function buildIndexes(graph: SemanticGraph): ResolveIndexes { const domChildren = new Map(); const axChildren = new Map(); + const domSiblingIndex = new Map(); + const axSiblingIndex = new Map(); const domIdIndex = new Map(); const labelForIndex = new Map(); for (const node of graph.nodes.values()) { if (node.domParentId) { const children = domChildren.get(node.domParentId) ?? []; + domSiblingIndex.set(node.id, children.length); children.push(node.id); domChildren.set(node.domParentId, children); } @@ -223,6 +228,9 @@ function buildIndexes(graph: SemanticGraph): ResolveIndexes { (bIndex < 0 ? Number.MAX_SAFE_INTEGER : bIndex) ); }); + for (let index = 0; index < children.length; index += 1) { + axSiblingIndex.set(children[index], index); + } } const textMemo = new Map(); @@ -305,6 +313,8 @@ function buildIndexes(graph: SemanticGraph): ResolveIndexes { return { domChildren, axChildren, + domSiblingIndex, + axSiblingIndex, domIdIndex, labelForIndex, domText, @@ -357,7 +367,7 @@ function precedingAxText( ): string | undefined { if (!node.axParentId) return undefined; const siblings = indexes.axChildren.get(node.axParentId) ?? []; - const index = siblings.indexOf(node.id); + const index = indexes.axSiblingIndex.get(node.id) ?? -1; for (let offset = 1; offset <= 4 && index - offset >= 0; offset += 1) { const sibling = graph.nodes.get(siblings[index - offset]); const role = normalizeRole(axValue(sibling?.ax?.role)) ?? ""; @@ -379,7 +389,7 @@ function precedingDomText( ): string | undefined { if (!node.domParentId) return undefined; const siblings = indexes.domChildren.get(node.domParentId) ?? []; - const index = siblings.indexOf(node.id); + const index = indexes.domSiblingIndex.get(node.id) ?? -1; for (let offset = 1; offset <= 4 && index - offset >= 0; offset += 1) { const sibling = graph.nodes.get(siblings[index - offset]); const siblingRole = sibling @@ -453,7 +463,7 @@ function nearbyText( ): string | undefined { if (!node.domParentId) return undefined; const siblings = indexes.domChildren.get(node.domParentId) ?? []; - const index = siblings.indexOf(node.id); + const index = indexes.domSiblingIndex.get(node.id) ?? -1; if (index < 0) return undefined; const texts = siblings .slice(Math.max(0, index - 3), index + 4) @@ -487,6 +497,7 @@ export function resolveSemanticGraph( const frame = graph.frames.get(node.frameId); const tag = node.dom?.tag.toLowerCase() ?? ""; const inForm = node.dom?.inForm; + const nearby = nearbyText(node, graph, indexes); const vom: ResolvedSemanticNode["vom"] = { tag, rect: node.dom?.rect ?? null, @@ -508,7 +519,7 @@ export function resolveSemanticGraph( : {}), attrs, ...(clean(node.dom?.textContent) ? { text: clean(node.dom?.textContent) } : {}), - ...(nearbyText(node, graph, indexes) ? { nearbyText: nearbyText(node, graph, indexes) } : {}), + ...(nearby ? { nearbyText: nearby } : {}), inputState: inputState(node, role, sourceValue, isSensitive), sensitive: isSensitive, // An available AX result is authoritative, including ignored/nonmodal nodes. diff --git a/crates/bsk-cli/src/cli/business_rpc.rs b/crates/bsk-cli/src/cli/business_rpc.rs index 0932e9fe1..89323a020 100644 --- a/crates/bsk-cli/src/cli/business_rpc.rs +++ b/crates/bsk-cli/src/cli/business_rpc.rs @@ -38,6 +38,11 @@ pub const CANCEL_RESPONSE_GRACE: Duration = Duration::from_secs(2); /// the original call's timeout because cancel must answer promptly. const CANCEL_FRAME_TIMEOUT: Duration = Duration::from_secs(2); +/// Keep the IPC connection alive long enough to receive the tool's timeout. +pub(crate) fn ipc_timeout(timeout_ms: u32) -> Duration { + Duration::from_millis(u64::from(timeout_ms) + 15_000) +} + static OBSERVATION_DEPENDENCIES: std::sync::OnceLock> = std::sync::OnceLock::new(); static OBSERVATION_CONTEXT: std::sync::OnceLock> = std::sync::OnceLock::new(); @@ -291,9 +296,16 @@ async fn wait_for_sigint() -> anyhow::Result<()> { } #[cfg(test)] -mod context_tests { - use super::with_context; +mod tests { + use super::{ipc_timeout, with_context}; use serde_json::json; + use std::time::Duration; + + #[test] + fn ipc_timeout_preserves_grace_at_the_largest_tool_timeout() { + assert_eq!(ipc_timeout(60_000), Duration::from_secs(75)); + assert_eq!(ipc_timeout(u32::MAX).as_millis(), u32::MAX as u128 + 15_000); + } #[test] fn context_is_explicit_and_does_not_change_action_parameters() { diff --git a/crates/bsk-cli/src/cli/evaluate.rs b/crates/bsk-cli/src/cli/evaluate.rs index ee0cd9b2a..4d108bfe9 100644 --- a/crates/bsk-cli/src/cli/evaluate.rs +++ b/crates/bsk-cli/src/cli/evaluate.rs @@ -12,13 +12,13 @@ //! normal `CliError` path with the usual non-zero exit codes. use std::path::PathBuf; -use std::time::Duration; use anyhow::Context; use bsk_protocol::Method; use bsk_protocol::tools::{EvaluateParams, EvaluateResult}; use clap::Args; +use crate::cli::business_rpc::ipc_timeout; use crate::cli::dialogs::print_dialog_summaries; use crate::cli::ensure_daemon::ensure_daemon; use crate::cli::error::{CliError, Format}; @@ -83,12 +83,6 @@ fn call( ) } -fn ipc_timeout(timeout_ms: u32) -> Duration { - Duration::from_millis(u64::from(timeout_ms)) - .checked_add(Duration::from_secs(15)) - .unwrap_or(Duration::from_secs(u64::from(timeout_ms / 1_000) + 15)) -} - fn render(reply: &EvaluateResult, format: Format) -> Result<(), CliError> { match format { Format::Json => { diff --git a/crates/bsk-cli/src/cli/human_loop.rs b/crates/bsk-cli/src/cli/human_loop.rs index 934968baa..b488a84cd 100644 --- a/crates/bsk-cli/src/cli/human_loop.rs +++ b/crates/bsk-cli/src/cli/human_loop.rs @@ -3,7 +3,6 @@ //! the browser overlay or the call times out. use std::path::PathBuf; -use std::time::Duration; use anyhow::Context; use bsk_protocol::Method; @@ -12,6 +11,7 @@ use bsk_protocol::tools::{ }; use clap::Args; +use crate::cli::business_rpc::ipc_timeout; use crate::cli::ensure_daemon::ensure_daemon; use crate::cli::error::{CliError, Format}; use crate::cli::navigate::parse_timeout_ms; @@ -143,14 +143,6 @@ fn render(reply: &RequestHelpResult, format: Format) -> Result<(), CliError> { Ok(()) } -/// IPC budget: requested timeout + 15s slack so the client does not -/// tear the connection down before the daemon's (long) wait resolves. -fn ipc_timeout(timeout_ms: u32) -> Duration { - Duration::from_millis(u64::from(timeout_ms)) - .checked_add(Duration::from_secs(15)) - .unwrap_or(Duration::from_secs(u64::from(timeout_ms / 1_000) + 15)) -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/bsk-cli/src/cli/interaction.rs b/crates/bsk-cli/src/cli/interaction.rs index f206f523e..6ca7a3722 100644 --- a/crates/bsk-cli/src/cli/interaction.rs +++ b/crates/bsk-cli/src/cli/interaction.rs @@ -6,7 +6,6 @@ //! the value is ambiguous. use std::path::PathBuf; -use std::time::Duration; use anyhow::Context; use bsk_protocol::Method; @@ -17,6 +16,7 @@ use bsk_protocol::tools::{ }; use clap::{Args, ValueEnum}; +use crate::cli::business_rpc::ipc_timeout; use crate::cli::dialogs::print_dialog_summaries; use crate::cli::ensure_daemon::ensure_daemon; use crate::cli::error::{CliError, Format}; @@ -678,19 +678,7 @@ where P: serde::Serialize + Send + 'static, R: serde::de::DeserializeOwned + Send + 'static, { - crate::cli::business_rpc::call::( - sock, - id, - method, - Some(params), - interaction_ipc_timeout(timeout_ms), - ) -} - -fn interaction_ipc_timeout(timeout_ms: u32) -> Duration { - Duration::from_millis(u64::from(timeout_ms)) - .checked_add(Duration::from_secs(15)) - .unwrap_or(Duration::from_secs(u64::from(timeout_ms / 1_000) + 15)) + crate::cli::business_rpc::call::(sock, id, method, Some(params), ipc_timeout(timeout_ms)) } #[cfg(test)] @@ -733,14 +721,6 @@ mod tests { assert_eq!(s.as_deref(), Some(".btn")); } - #[test] - fn interaction_ipc_timeout_tracks_user_timeout_with_grace() { - assert_eq!( - interaction_ipc_timeout(60_000), - Duration::from_secs(60) + Duration::from_secs(15) - ); - } - #[test] fn fill_clap_accepts_target_positional_with_value_flag() { use clap::Parser; diff --git a/crates/bsk-cli/src/cli/logs.rs b/crates/bsk-cli/src/cli/logs.rs index 013f544dd..75c160d3b 100644 --- a/crates/bsk-cli/src/cli/logs.rs +++ b/crates/bsk-cli/src/cli/logs.rs @@ -73,20 +73,82 @@ pub fn run(args: LogsArgs) -> Result<()> { } fn print_tail_lines(path: &Path, n: usize, w: &mut W) -> Result { - let file = File::open(path).with_context(|| format!("open {}", path.display()))?; - let len = file.metadata()?.len(); - let mut reader = BufReader::new(file); - let mut buf: Vec = Vec::new(); - reader.read_to_end(&mut buf)?; - - let text = String::from_utf8_lossy(&buf); - let mut lines: Vec<&str> = text.lines().collect(); - let start = lines.len().saturating_sub(n); - lines.drain(0..start); - for line in lines { - writeln!(w, "{line}")?; - } - Ok(len) + let mut file = File::open(path).with_context(|| format!("open {}", path.display()))?; + let (start, len) = tail_snapshot(&mut file, n)?; + file.seek(SeekFrom::Start(start))?; + // Appends after this snapshot belong to the following poll, including + // appends that arrive while the initial output is being written. + let mut reader = BufReader::new(file.take(len - start)); + let mut line = Vec::new(); + let mut offset = start; + loop { + let count = reader.read_until(b'\n', &mut line)?; + if count == 0 { + break; + } + offset += count as u64; + if line.last() == Some(&b'\n') { + line.pop(); + if line.last() == Some(&b'\r') { + line.pop(); + } + } + writeln!(w, "{}", String::from_utf8_lossy(&line))?; + line.clear(); + } + w.flush()?; + Ok(offset) +} + +fn tail_snapshot(reader: &mut (impl Read + Seek), n: usize) -> std::io::Result<(u64, u64)> { + for _ in 0..3 { + let len = reader.seek(SeekFrom::End(0))?; + match tail_start(reader, len, n) { + Ok(start) => return Ok((start, len)), + Err(error) if error.kind() == std::io::ErrorKind::UnexpectedEof => continue, + Err(error) => return Err(error), + } + } + // A repeatedly truncated file has no stable tail yet. Leave its contents + // for the next follow poll, starting at zero so no records are skipped. + Ok((0, 0)) +} + +fn tail_start(reader: &mut (impl Read + Seek), len: u64, n: usize) -> std::io::Result { + if n == 0 { + return Ok(len); + } + let mut remaining = n; + let mut end = len; + let mut chunk = [0u8; 8192]; + while end > 0 { + let start = end.saturating_sub(chunk.len() as u64); + let count = (end - start) as usize; + reader.seek(SeekFrom::Start(start))?; + reader.read_exact(&mut chunk[..count])?; + for index in (0..count).rev() { + let position = start + index as u64; + if chunk[index] == b'\n' && position != len - 1 { + remaining -= 1; + if remaining == 0 { + return Ok(position + 1); + } + } + } + end = start; + } + Ok(0) +} + +fn print_appended(mut file: File, mut offset: u64, w: &mut W) -> Result { + if file.metadata()?.len() < offset { + // A truncated file starts a new stream at byte zero. + offset = 0; + } + file.seek(SeekFrom::Start(offset))?; + let copied = std::io::copy(&mut file, w)?; + w.flush()?; + Ok(offset + copied) } fn follow_file(path: &Path, mut offset: u64, w: &mut W) -> Result<()> { @@ -99,26 +161,258 @@ fn follow_file(path: &Path, mut offset: u64, w: &mut W) -> Result<()> offset = 0; } } - let mut file = match File::open(¤t) { + let file = match File::open(¤t) { Ok(f) => f, Err(_) => continue, }; - let len = file.metadata()?.len(); - if len < offset { - // File truncated or rotated; rewind. - offset = 0; + offset = print_appended(file, offset, w)?; + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::fs::OpenOptions; + use std::io::{self, Cursor}; + + #[test] + fn tail_handles_empty_files_line_endings_and_unicode() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("daemon.log"); + for (input, n, expected) in [ + ("", 1, ""), + ("first\nsecond\n", 0, ""), + ("first\nsecond\n", 1, "second\n"), + ("first\nsecond", 1, "second\n"), + ("first\nsecond\n", usize::MAX, "first\nsecond\n"), + ("first\n\n", 1, "\n"), + ("\n", 1, "\n"), + ("first\r\nsecond\r\n", 1, "second\n"), + ("first\r\nsecond\r", 1, "second\r\n"), + ("first\nδΈ­ζ–‡ πŸ¦€\n", 1, "δΈ­ζ–‡ πŸ¦€\n"), + ] { + std::fs::write(&path, input).unwrap(); + let mut output = Vec::new(); + let offset = print_tail_lines(&path, n, &mut output).unwrap(); + assert_eq!(output, expected.as_bytes(), "input={input:?}, n={n}"); + assert_eq!(offset, input.len() as u64); } - if len == offset { - continue; + } + + #[test] + fn tail_finds_boundaries_across_chunks_without_splitting_unicode() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("daemon.log"); + let expected = format!("{}\nlast\n", "η•Œ".repeat(5000)); + std::fs::write(&path, format!("{}\n{expected}", "prefix".repeat(2000))).unwrap(); + let mut output = Vec::new(); + print_tail_lines(&path, 2, &mut output).unwrap(); + assert_eq!(output, expected.as_bytes()); + } + + #[test] + fn tail_reads_only_a_small_suffix_when_few_lines_are_requested() { + struct CountingReader { + cursor: Cursor>, + bytes_read: usize, + } + impl Read for CountingReader { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + let count = self.cursor.read(buf)?; + self.bytes_read += count; + Ok(count) + } + } + impl Seek for CountingReader { + fn seek(&mut self, position: SeekFrom) -> io::Result { + self.cursor.seek(position) + } + } + let contents = "old line\n".repeat(200_000) + "last line\n"; + let len = contents.len() as u64; + let mut reader = CountingReader { + cursor: Cursor::new(contents.into_bytes()), + bytes_read: 0, + }; + assert_eq!(tail_start(&mut reader, len, 0).unwrap(), len); + assert_eq!(reader.bytes_read, 0); + assert_eq!(tail_start(&mut reader, len, 1).unwrap(), len - 10); + assert!(reader.bytes_read <= 16 * 1024); + } + + struct ShrinkingReader { + file: File, + path: PathBuf, + reads: usize, + repeatedly: bool, + } + + impl Read for ShrinkingReader { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + self.reads += 1; + if self.repeatedly { + self.file + .set_len(self.file.metadata()?.len().saturating_sub(1))?; + } else if self.reads == 2 { + std::fs::write(&self.path, b"new\n")?; + } + self.file.read(buf) + } + } + + impl Seek for ShrinkingReader { + fn seek(&mut self, position: SeekFrom) -> io::Result { + self.file.seek(position) + } + } + + #[test] + fn tail_resnapshots_when_truncated_during_the_backward_scan() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("daemon.log"); + std::fs::write(&path, "old".repeat(10_000)).unwrap(); + let mut reader = ShrinkingReader { + file: File::open(&path).unwrap(), + path, + reads: 0, + repeatedly: false, + }; + let (start, len) = tail_snapshot(&mut reader, 1).unwrap(); + assert_eq!((start, len), (0, 4)); + let mut output = Vec::new(); + let offset = print_appended(reader.file, start, &mut output).unwrap(); + assert_eq!(output, b"new\n"); + assert_eq!(offset, len); + } + + #[test] + fn repeated_truncation_defers_to_following_without_skipping_content() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("daemon.log"); + std::fs::write(&path, b"continually shrinking").unwrap(); + let mut reader = ShrinkingReader { + file: OpenOptions::new() + .read(true) + .write(true) + .open(&path) + .unwrap(), + path: path.clone(), + reads: 0, + repeatedly: true, + }; + assert_eq!(tail_snapshot(&mut reader, 1).unwrap(), (0, 0)); + assert!( + reader.reads <= 8, + "the initial scan must not retry indefinitely" + ); + let mut output = Vec::new(); + print_appended(reader.file, 0, &mut output).unwrap(); + assert!(!output.is_empty()); + assert_eq!(output, std::fs::read(path).unwrap()); + } + + struct AppendingWriter { + path: PathBuf, + output: Vec, + appended: bool, + } + + impl Write for AppendingWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + self.output.extend_from_slice(buf); + if !self.appended { + self.appended = true; + OpenOptions::new() + .append(true) + .open(&self.path)? + .write_all(b"second\n")?; + } + Ok(buf.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } + + #[test] + fn initial_tail_leaves_concurrent_appends_for_the_next_poll() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("daemon.log"); + std::fs::write(&path, b"first\n").unwrap(); + let mut writer = AppendingWriter { + path: path.clone(), + output: Vec::new(), + appended: false, + }; + let offset = print_tail_lines(&path, 1, &mut writer).unwrap(); + assert_eq!(writer.output, b"first\n"); + let offset = print_appended(File::open(&path).unwrap(), offset, &mut writer).unwrap(); + print_appended(File::open(&path).unwrap(), offset, &mut writer).unwrap(); + assert_eq!(writer.output, b"first\nsecond\n"); + } + + #[test] + fn following_advances_past_bytes_appended_during_output() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("daemon.log"); + std::fs::write(&path, b"first\n").unwrap(); + let mut writer = AppendingWriter { + path: path.clone(), + output: Vec::new(), + appended: false, + }; + let offset = print_appended(File::open(&path).unwrap(), 0, &mut writer).unwrap(); + assert_eq!(offset, b"first\nsecond\n".len() as u64); + print_appended(File::open(&path).unwrap(), offset, &mut writer).unwrap(); + assert_eq!(writer.output, b"first\nsecond\n"); + } + + #[test] + fn following_rewinds_a_truncated_file() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("daemon.log"); + std::fs::write(&path, b"old first\nold second\n").unwrap(); + let mut output = Vec::new(); + let offset = print_tail_lines(&path, 0, &mut output).unwrap(); + std::fs::write(&path, b"new\n").unwrap(); + let offset = print_appended(File::open(&path).unwrap(), offset, &mut output).unwrap(); + assert_eq!(offset, 4); + assert_eq!(output, b"new\n"); + } + + #[test] + fn output_write_and_flush_errors_end_log_reading() { + struct FailingWriter { + fail_on_flush: bool, + } + impl Write for FailingWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + if self.fail_on_flush { + Ok(buf.len()) + } else { + Err(io::ErrorKind::BrokenPipe.into()) + } + } + fn flush(&mut self) -> io::Result<()> { + Err(io::ErrorKind::BrokenPipe.into()) + } } - file.seek(SeekFrom::Start(offset))?; - let mut reader = BufReader::new(file); - let mut line = String::new(); - while reader.read_line(&mut line)? > 0 { - let _ = w.write_all(line.as_bytes()); - line.clear(); + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("daemon.log"); + std::fs::write(&path, b"first\n").unwrap(); + for fail_on_flush in [false, true] { + let mut writer = FailingWriter { fail_on_flush }; + let error = print_tail_lines(&path, 1, &mut writer).unwrap_err(); + assert_eq!( + error.downcast_ref::().unwrap().kind(), + io::ErrorKind::BrokenPipe + ); + let error = print_appended(File::open(&path).unwrap(), 0, &mut writer).unwrap_err(); + assert_eq!( + error.downcast_ref::().unwrap().kind(), + io::ErrorKind::BrokenPipe + ); } - offset = len; - let _ = w.flush(); } } diff --git a/crates/bsk-cli/src/cli/navigate.rs b/crates/bsk-cli/src/cli/navigate.rs index d95e5e9b7..92780c4a8 100644 --- a/crates/bsk-cli/src/cli/navigate.rs +++ b/crates/bsk-cli/src/cli/navigate.rs @@ -5,7 +5,6 @@ //! emits the full bsk-protocol payload. use std::path::PathBuf; -use std::time::Duration; use anyhow::Context; use bsk_protocol::Method; @@ -16,6 +15,7 @@ use bsk_protocol::tools::{ use clap::{Args, Subcommand, ValueEnum}; use serde::Serialize; +use crate::cli::business_rpc::ipc_timeout; use crate::cli::dialogs::print_dialog_summaries; use crate::cli::ensure_daemon::ensure_daemon; use crate::cli::error::{CliError, Format}; @@ -257,22 +257,7 @@ where P: serde::Serialize + Send + 'static, R: serde::de::DeserializeOwned + Send + 'static, { - crate::cli::business_rpc::call::( - sock, - id, - method, - Some(params), - navigate_ipc_timeout(timeout_ms), - ) -} - -/// Use the global tool IPC timeout but extend it so a long navigation -/// (e.g. networkidle on a slow page) doesn't trip the IPC layer before -/// the daemon's own timeout fires. -fn navigate_ipc_timeout(timeout_ms: u32) -> Duration { - Duration::from_millis(u64::from(timeout_ms)) - .checked_add(Duration::from_secs(15)) - .unwrap_or(Duration::from_secs(u64::from(timeout_ms / 1_000) + 15)) + crate::cli::business_rpc::call::(sock, id, method, Some(params), ipc_timeout(timeout_ms)) } fn render_navigate(reply: &NavigateResult, format: Format) -> Result<(), CliError> { @@ -417,14 +402,6 @@ mod tests { assert_eq!(WaitUntil::Commit, CliWaitUntil::Commit.into()); } - #[test] - fn navigate_ipc_timeout_tracks_user_timeout_with_grace() { - assert_eq!( - navigate_ipc_timeout(60_000), - Duration::from_secs(60) + Duration::from_secs(15) - ); - } - fn sample_navigate(reached: &str) -> NavigateResult { NavigateResult { tab_id: 1, diff --git a/crates/bsk-cli/src/cli/record.rs b/crates/bsk-cli/src/cli/record.rs index f59145c5b..e3c522eeb 100644 --- a/crates/bsk-cli/src/cli/record.rs +++ b/crates/bsk-cli/src/cli/record.rs @@ -1,7 +1,6 @@ //! `bsk record start|stop` β€” capture user actions in the Agent Window. use std::path::{Path, PathBuf}; -use std::time::Duration; use anyhow::Context; use bsk_protocol::tools::{ @@ -15,7 +14,7 @@ use crate::cli::TOOL_IPC_TIMEOUT; mod export; -use crate::cli::business_rpc; +use crate::cli::business_rpc::{self, ipc_timeout}; use crate::cli::ensure_daemon::ensure_daemon; use crate::cli::error::{CliError, Format}; use crate::cli::record_recovery; @@ -155,7 +154,7 @@ fn dispatch_start(args: RecordStartArgs, format: Format) -> Result<(), CliError> "record-await", Method::ToolRecordAwait, Some(await_params), - record_await_ipc_timeout(RECORD_AWAIT_TIMEOUT_MS), + ipc_timeout(RECORD_AWAIT_TIMEOUT_MS), ); // Keep write/render inside a Result so `?` cannot skip session teardown. @@ -297,12 +296,6 @@ fn prepare_record_start(output: &Path) -> Result<(), CliError> { validate_record_output(output) } -fn record_await_ipc_timeout(timeout_ms: u32) -> Duration { - Duration::from_millis(u64::from(timeout_ms)) - .checked_add(Duration::from_secs(15)) - .unwrap_or(Duration::from_secs(u64::from(timeout_ms / 1_000) + 15)) -} - fn render_finish( trace: &RecordedTrace, output_dir: &PathBuf, @@ -516,8 +509,8 @@ mod tests { #[test] fn record_await_ipc_timeout_covers_long_wait() { - let got = record_await_ipc_timeout(RECORD_AWAIT_TIMEOUT_MS); - assert!(got >= Duration::from_secs(86_400)); + let got = ipc_timeout(RECORD_AWAIT_TIMEOUT_MS); + assert!(got >= std::time::Duration::from_secs(86_400)); } #[test] diff --git a/crates/bsk-cli/src/cli/scroll.rs b/crates/bsk-cli/src/cli/scroll.rs index ba79983ff..943ec8710 100644 --- a/crates/bsk-cli/src/cli/scroll.rs +++ b/crates/bsk-cli/src/cli/scroll.rs @@ -1,12 +1,11 @@ //! `bsk scroll-to` β€” bring a target element into the visible viewport. -use std::time::Duration; - use anyhow::Context; use bsk_protocol::Method; use bsk_protocol::tools::{ScrollToParams, ScrollToResult}; use clap::Args; +use crate::cli::business_rpc::ipc_timeout; use crate::cli::dialogs::print_dialog_summaries; use crate::cli::ensure_daemon::ensure_daemon; use crate::cli::error::{CliError, Format}; @@ -77,12 +76,6 @@ fn format_used_target(used_ref: Option<&str>, used_selector: Option<&str>) -> St .unwrap_or_else(|| "?".into()) } -fn ipc_timeout(timeout_ms: u32) -> Duration { - Duration::from_millis(u64::from(timeout_ms)) - .checked_add(Duration::from_secs(15)) - .unwrap_or(Duration::from_secs(u64::from(timeout_ms / 1_000) + 15)) -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/bsk-cli/src/cli/waits.rs b/crates/bsk-cli/src/cli/waits.rs index 4f9462167..06042ef7d 100644 --- a/crates/bsk-cli/src/cli/waits.rs +++ b/crates/bsk-cli/src/cli/waits.rs @@ -27,6 +27,7 @@ use bsk_protocol::tools::{ }; use clap::{Args, ValueEnum}; +use crate::cli::business_rpc::ipc_timeout; use crate::cli::dialogs::print_dialog_summaries; use crate::cli::ensure_daemon::ensure_daemon; use crate::cli::error::{CliError, Format}; @@ -314,9 +315,7 @@ fn call_wait_ms( // IPC budget: requested duration + 15s of slack so the client // doesn't tear the connection down before the daemon's sleep // resolves. - let timeout = Duration::from_millis(duration_ms) - .checked_add(Duration::from_secs(15)) - .unwrap_or(Duration::from_secs(30)); + let timeout = Duration::from_millis(duration_ms) + Duration::from_secs(15); crate::cli::business_rpc::call::( sock, "wait-ms", @@ -340,12 +339,6 @@ fn render_wait_ms(reply: &WaitMsResult, format: Format) -> Result<(), CliError> Ok(()) } -fn ipc_timeout(timeout_ms: u32) -> Duration { - Duration::from_millis(u64::from(timeout_ms)) - .checked_add(Duration::from_secs(15)) - .unwrap_or(Duration::from_secs(u64::from(timeout_ms / 1_000) + 15)) -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/bsk-cli/src/cli/wheel.rs b/crates/bsk-cli/src/cli/wheel.rs index 303bc747b..bc2327248 100644 --- a/crates/bsk-cli/src/cli/wheel.rs +++ b/crates/bsk-cli/src/cli/wheel.rs @@ -1,12 +1,11 @@ //! `bsk wheel` β€” dispatch a native Chromium mouse-wheel event. -use std::time::Duration; - use anyhow::Context; use bsk_protocol::Method; use bsk_protocol::tools::{WheelParams, WheelResult}; use clap::Args; +use crate::cli::business_rpc::ipc_timeout; use crate::cli::dialogs::print_dialog_summaries; use crate::cli::ensure_daemon::ensure_daemon; use crate::cli::error::{CliError, Format}; @@ -143,12 +142,6 @@ fn format_used_target(used_ref: Option<&str>, used_selector: Option<&str>) -> St .unwrap_or_else(|| "viewport-center".into()) } -fn ipc_timeout(timeout_ms: u32) -> Duration { - Duration::from_millis(u64::from(timeout_ms)) - .checked_add(Duration::from_secs(15)) - .unwrap_or(Duration::from_secs(u64::from(timeout_ms / 1_000) + 15)) -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/bsk-cli/src/ipc_client.rs b/crates/bsk-cli/src/ipc_client.rs index a42541cb4..4f29a0e71 100644 --- a/crates/bsk-cli/src/ipc_client.rs +++ b/crates/bsk-cli/src/ipc_client.rs @@ -7,7 +7,70 @@ //! //! On Windows the same line protocol runs over a per-user named pipe. -use bsk_protocol::RpcError; +use anyhow::{Context, Result}; +use bsk_protocol::{Frame, ResponseBody, RpcError}; +use serde::de::DeserializeOwned; + +fn decode_response(line: &str, id: &str) -> Result> { + let frame: Frame = serde_json::from_str(line).context("decode IPC response")?; + match frame { + Frame::Response(resp) => { + if resp.id != id { + return Err(anyhow::anyhow!( + "IPC response id mismatch: expected {id}, got {}", + resp.id + )); + } + match resp.body { + ResponseBody::Ok(v) => { + let value: R = serde_json::from_value(v).context("decode result")?; + Ok(Ok(value)) + } + ResponseBody::Err(e) => Ok(Err(e)), + } + } + other => Err(anyhow::anyhow!("unexpected frame from daemon: {other:?}")), + } +} + +#[cfg(test)] +mod tests { + use super::decode_response; + use bsk_protocol::{ErrorCode, PingResult}; + use serde_json::{Value, json}; + + #[test] + fn shared_decoder_preserves_typed_results_and_rpc_error_details() { + let result: PingResult = decode_response(r#"{"id":"one","result":{"pong":true}}"#, "one") + .unwrap() + .unwrap(); + assert!(result.pong); + let error = decode_response::( + r#"{"id":"one","error":{"code":"cancelled","message":"stopped","data":{"reason":"test"}}}"#, + "one", + ) + .unwrap() + .unwrap_err(); + assert_eq!(error.code, ErrorCode::Cancelled); + assert_eq!(error.message, "stopped"); + assert_eq!(error.data, Some(json!({"reason": "test"}))); + } + + #[test] + fn shared_decoder_rejects_wrong_request_ids_and_invalid_response_shapes() { + for wire in [ + r#"{"id":"other","result":{"pong":true}}"#, + r#"{"id":"one","method":"system.ping"}"#, + r#"{"id":"one","result":{"pong":"wrong type"}}"#, + "not JSON", + ] { + assert!( + decode_response::(wire, "one").is_err(), + "{wire}" + ); + } + } +} #[cfg(unix)] mod platform { @@ -15,13 +78,13 @@ mod platform { use std::time::Duration; use anyhow::{Context, Result}; - use bsk_protocol::{Frame, Method, RequestFrame, ResponseBody}; + use bsk_protocol::{Frame, Method, RequestFrame}; use serde::{Serialize, de::DeserializeOwned}; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::net::UnixStream; use tokio::time::timeout; - use crate::ipc_client::{RpcOutcome, random_id}; + use crate::ipc_client::{RpcOutcome, decode_response, random_id}; /// Connected IPC client. Holds an open UDS connection plus a buffered /// reader/writer pair. @@ -108,28 +171,6 @@ mod platform { &self.sock_path } } - - fn decode_response(line: &str, id: &str) -> Result> { - let frame: Frame = serde_json::from_str(line).context("decode IPC response")?; - match frame { - Frame::Response(resp) => { - if resp.id != id { - return Err(anyhow::anyhow!( - "IPC response id mismatch: expected {id}, got {}", - resp.id - )); - } - match resp.body { - ResponseBody::Ok(v) => { - let value: R = serde_json::from_value(v).context("decode result")?; - Ok(Ok(value)) - } - ResponseBody::Err(e) => Ok(Err(e)), - } - } - other => Err(anyhow::anyhow!("unexpected frame from daemon: {other:?}")), - } - } } #[cfg(windows)] @@ -138,14 +179,14 @@ mod platform { use std::time::Duration; use anyhow::{Context, Result}; - use bsk_protocol::{Frame, Method, RequestFrame, ResponseBody}; + use bsk_protocol::{Frame, Method, RequestFrame}; use serde::{Serialize, de::DeserializeOwned}; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::net::windows::named_pipe::{ClientOptions, NamedPipeClient}; use tokio::time::{sleep, timeout}; use windows_sys::Win32::Foundation::{ERROR_ACCESS_DENIED, ERROR_PIPE_BUSY}; - use crate::ipc_client::{RpcOutcome, random_id}; + use crate::ipc_client::{RpcOutcome, decode_response, random_id}; pub struct Client { stream: BufReader>, @@ -267,28 +308,6 @@ mod platform { &self.pipe_name } } - - fn decode_response(line: &str, id: &str) -> Result> { - let frame: Frame = serde_json::from_str(line).context("decode IPC response")?; - match frame { - Frame::Response(resp) => { - if resp.id != id { - return Err(anyhow::anyhow!( - "IPC response id mismatch: expected {id}, got {}", - resp.id - )); - } - match resp.body { - ResponseBody::Ok(v) => { - let value: R = serde_json::from_value(v).context("decode result")?; - Ok(Ok(value)) - } - ResponseBody::Err(e) => Ok(Err(e)), - } - } - other => Err(anyhow::anyhow!("unexpected frame from daemon: {other:?}")), - } - } } pub use platform::Client; diff --git a/crates/bsk-cli/tests/logs_cmd.rs b/crates/bsk-cli/tests/logs_cmd.rs index 6d98692b0..4ae75922b 100644 --- a/crates/bsk-cli/tests/logs_cmd.rs +++ b/crates/bsk-cli/tests/logs_cmd.rs @@ -4,7 +4,7 @@ #![cfg(unix)] use std::path::PathBuf; -use std::process::Command; +use std::process::{Command, Stdio}; use std::time::{Duration, Instant}; use tempfile::TempDir; @@ -83,3 +83,45 @@ fn bsk_logs_handles_empty_home_gracefully() { .expect("bsk logs"); assert!(logs.status.success()); } + +#[test] +fn following_logs_exits_when_the_output_pipe_closes() { + use std::io::{BufRead, BufReader, Write}; + + let temp = TempDir::new().unwrap(); + let path = temp.path().join("daemon.log.2026-01-01"); + std::fs::write(&path, b"ready\n").unwrap(); + let mut child = Command::new(bsk_bin()) + .args(["logs", "--follow", "-n", "1"]) + .env("BSK_HOME", temp.path()) + .env("BSK_AUTO_UPDATE", "off") + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + let mut reader = BufReader::new(child.stdout.take().unwrap()); + let mut line = String::new(); + reader.read_line(&mut line).unwrap(); + assert_eq!(line, "ready\n"); + drop(reader); + std::fs::OpenOptions::new() + .append(true) + .open(path) + .unwrap() + .write_all(b"after close\n") + .unwrap(); + + let deadline = Instant::now() + Duration::from_secs(3); + loop { + if let Some(status) = child.try_wait().unwrap() { + assert!(!status.success()); + break; + } + if Instant::now() >= deadline { + child.kill().unwrap(); + child.wait().unwrap(); + panic!("logs --follow kept running after its output pipe closed"); + } + std::thread::sleep(Duration::from_millis(10)); + } +} diff --git a/packages/dsh-plugin-browserskill/src/client/ObservationOverlay.module.css b/packages/dsh-plugin-browserskill/src/client/ObservationOverlay.module.css index c66d17a64..4d63da92d 100644 --- a/packages/dsh-plugin-browserskill/src/client/ObservationOverlay.module.css +++ b/packages/dsh-plugin-browserskill/src/client/ObservationOverlay.module.css @@ -369,10 +369,6 @@ border-color: var(--destructive); } -.strip-item[data-state="dead"] { - opacity: 0.45; -} - .strip-main { display: flex; align-items: center; diff --git a/packages/dsh-plugin-browserskill/src/client/ObservationOverlay.tsx b/packages/dsh-plugin-browserskill/src/client/ObservationOverlay.tsx index e75c07fd6..5747027eb 100644 --- a/packages/dsh-plugin-browserskill/src/client/ObservationOverlay.tsx +++ b/packages/dsh-plugin-browserskill/src/client/ObservationOverlay.tsx @@ -242,13 +242,13 @@ function StopSessionAction(props: { type ChromeState = "active" | "idle" | "error" | "reconnecting"; /** Flat status dot, specced after the BSK popup's ConnectionStatusIndicator. */ -function StatusDot({ state }: { state: ChromeState | "dead" }) { +function StatusDot({ state }: { state: ChromeState }) { const color = state === "active" ? "bg-emerald-500" : state === "error" ? "bg-red-500" - : state === "dead" || state === "reconnecting" + : state === "reconnecting" ? "bg-amber-500" : "bg-muted-foreground/40"; return ( @@ -275,7 +275,7 @@ function StripItem(props: { store.ensureThumbnail(thumbId); }, [store, thumbId]); const thumb = store.getSnapshot().displayFrames[obs.sessionId]; - const state = obs.dead === true ? "dead" : statusOf(obs); + const state = statusOf(obs); return (
{pinned ? : null} - {hover && !pinned && obs.dead !== true ? ( + {hover && !pinned ? (