mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 00:03:52 +00:00
fix(browser): report creation-order indices and target the newest active claim
Round-five Opus findings (both reviewers converged independently): P1 — 'tab show', 'tab current' and 'tab switch --page' reported the bridge's registration index, which any park+wake permutes (unregister deletes the map key, re-register appends). The listing speaks creation order, so the reported number named a different tab than the listing — and chaining it into '--index' closed the wrong tab. Every index a headless caller sees now comes from the same creation-ordered listing. P1 — the strict parked-active claim let one flag exist per scope, but the scope-less implicit target still scanned broadly and returned the FIRST flagged page in creation order — the oldest scope's page, not the page that actually held the most recent active claim. It now prefers the most recently used flagged page, matching the bridge's global-pointer semantics; scoped queries see at most one flag and are unchanged. P2 — closeTab's promotion gate called getActivePageId, which resolves through resolveActiveTab and reassigns the bridge's active pointers as a side effect (the exact mutation this PR's own comment warns against), and its undefined scope meant 'anywhere' while the promotion is strict. The gate now reads the page book: promote only when the closed page's scope holds no resident page. P2 — an idle/grace window past 2^31-1ms (reachable via the documented 'set it very high' advice, e.g. 1e10) overflowed setTimeout, which Node clamps to 1ms — a permanent spin. The armed delay is now capped at the setTimeout ceiling; re-checking at the horizon is harmless. Invalid knob values also warn instead of falling back silently. P2 — the docs sold the download veto as a guarantee; reworded to what ships: a decision-time veto, with downloads that begin during teardown still cancelled, exactly as if the tab had been closed at that instant.
This commit is contained in:
@@ -293,8 +293,10 @@ storage live in the profile partition and survive, but an unsubmitted form or
|
||||
an SPA's in-memory state does not. A page is never parked while a paired
|
||||
client is streaming it, while a command against it is in flight, while its
|
||||
initial load is still running, while it is waiting on a certificate decision,
|
||||
while a wake is still rebuilding it, or while it is writing an active
|
||||
download — parking cancels a download, so an active one always vetoes it. A stream ending counts as use, so a viewer who closes the pane gets
|
||||
or while a wake is still rebuilding it. An active download vetoes the park
|
||||
decision — parking cancels downloads — but a download that begins during the
|
||||
teardown itself is still cancelled, exactly as if the tab had been closed at
|
||||
that instant. A stream ending counts as use, so a viewer who closes the pane gets
|
||||
the full idle window before the page parks, even if they only watched. A navigation that has still not finished by the load timeout is
|
||||
deliberately reclaimable — otherwise one stalled page per create could hold a
|
||||
renderer forever, which is the failure this exists to prevent; waking it simply
|
||||
|
||||
@@ -817,6 +817,35 @@ describe('OffscreenBrowserBackend reclamation', () => {
|
||||
.map((page) => page.browserPageId)
|
||||
.sort()
|
||||
).toEqual(['g', 'w'])
|
||||
// Why: with one flag per scope, a scope-less implicit command must target
|
||||
// the page that held the MOST RECENT active claim — not the oldest scope's
|
||||
// page just because it comes first in creation order.
|
||||
expect(h.backend.getParkedPageIdForImplicitTarget()).toBe('g')
|
||||
})
|
||||
|
||||
it('promotes for a scope even while another worktree has a resident page', async () => {
|
||||
// Why: the promotion gate is per scope. A live tab elsewhere on the host
|
||||
// says nothing about this worktree's selection.
|
||||
const h = createHarness({ activePageId: 'holder' })
|
||||
await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' })
|
||||
h.clock.value += 1_000
|
||||
await h.backend.createTab({
|
||||
url: 'https://holder',
|
||||
browserPageId: 'holder',
|
||||
worktreeId: 'wt-1'
|
||||
})
|
||||
h.clock.value += 1_000
|
||||
await h.backend.createTab({ url: 'https://other', browserPageId: 'other', worktreeId: 'wt-2' })
|
||||
h.clock.value += 120_000
|
||||
await h.backend.wakeTab('other')
|
||||
await h.backend.reclaimIdlePages()
|
||||
h.activePageId = undefined
|
||||
|
||||
await h.backend.closeTab('holder')
|
||||
|
||||
expect(
|
||||
h.backend.listParkedPages('wt-1').map((page) => [page.browserPageId, page.active === true])
|
||||
).toEqual([['a', true]])
|
||||
})
|
||||
|
||||
it('targets the page that was active, not merely the most recently used', async () => {
|
||||
|
||||
@@ -120,7 +120,12 @@ export class OffscreenBrowserBackend implements BrowserBackend {
|
||||
// either the parked-active flag, or a live pointer whose teardown found no
|
||||
// other live tab to promote. A worktree with open pages and no active tab
|
||||
// breaks the one-selected-tab assumption every paired client renders on.
|
||||
if (page && !this.options.getAgentBrowserBridge?.()?.getActivePageId(page.worktreeId)) {
|
||||
// Gate on the book, not the bridge: getActivePageId resolves through
|
||||
// resolveActiveTab, which reassigns the active pointers as a side effect,
|
||||
// and its undefined scope means "anywhere" while the promotion is strict.
|
||||
// A scope holding a resident page needs no promotion — the live listing
|
||||
// marks one of its tabs active itself.
|
||||
if (page && !this.pages.resident().some((open) => open.worktreeId === page.worktreeId)) {
|
||||
this.pages.promoteParkedActive(page.worktreeId)
|
||||
}
|
||||
// Why: closing changes both residency and rank, and with the last page gone
|
||||
|
||||
@@ -104,16 +104,22 @@ export class OffscreenBrowserOpenPages {
|
||||
* tie when none is recorded.
|
||||
*/
|
||||
parkedIdForImplicitTarget(worktreeId?: string): string | null {
|
||||
// Why MRU among the flagged: the claim is strict per scope, so a scoped
|
||||
// query sees at most one flag — but the scope-less query spans every
|
||||
// worktree and can see one flag per scope. Recency picks the page that
|
||||
// actually held the most recent active claim, matching the bridge's own
|
||||
// global-pointer semantics; creation order would pick the oldest scope.
|
||||
let flagged: OffscreenBrowserPage | null = null
|
||||
let best: OffscreenBrowserPage | null = null
|
||||
for (const page of this.parked(worktreeId)) {
|
||||
if (page.activeWhenParked) {
|
||||
return page.browserPageId
|
||||
if (page.activeWhenParked && (!flagged || page.lastActivityAt > flagged.lastActivityAt)) {
|
||||
flagged = page
|
||||
}
|
||||
if (!best || page.lastActivityAt > best.lastActivityAt) {
|
||||
best = page
|
||||
}
|
||||
}
|
||||
return best?.browserPageId ?? null
|
||||
return (flagged ?? best)?.browserPageId ?? null
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -36,7 +36,13 @@ function readPositiveIntEnv(name: string, fallback: number): number {
|
||||
// Number() consumes the whole string, so '1e9' parses fully and trailing
|
||||
// garbage falls back instead of truncating.
|
||||
const parsed = Number(raw)
|
||||
return Number.isInteger(parsed) && parsed >= 0 ? parsed : fallback
|
||||
if (!Number.isInteger(parsed) || parsed < 0) {
|
||||
// Why the warning: parseInt used to truncate '60000ms' to 60000; falling
|
||||
// back silently would change such a knob's behavior with no signal.
|
||||
console.warn(`[offscreen-browser] ignoring invalid ${name}=${raw}`)
|
||||
return fallback
|
||||
}
|
||||
return parsed
|
||||
}
|
||||
|
||||
export function readOffscreenBrowserRetentionBudget(): BrowserRetentionBudget {
|
||||
|
||||
@@ -38,6 +38,7 @@ function createReclaimer(
|
||||
pages?: OffscreenBrowserPage[]
|
||||
pinned?: readonly string[]
|
||||
park?: (browserPageId: string, live: Map<string, OffscreenBrowserPage>) => Promise<void>
|
||||
budget?: BrowserRetentionBudget
|
||||
} = {}
|
||||
) {
|
||||
const live = new Map<string, OffscreenBrowserPage>(
|
||||
@@ -51,7 +52,7 @@ function createReclaimer(
|
||||
} as unknown as OffscreenBrowserOpenPages
|
||||
const reclaimer = new OffscreenBrowserPageReclaimer({
|
||||
pages,
|
||||
budget: BUDGET,
|
||||
budget: args.budget ?? BUDGET,
|
||||
isReleasing: () => false,
|
||||
isWaking: () => false,
|
||||
hasCertificateChallenge: () => false,
|
||||
@@ -201,6 +202,23 @@ describe('OffscreenBrowserPageReclaimer scheduling', () => {
|
||||
expect(attempts).toBeLessThanOrEqual(11)
|
||||
})
|
||||
|
||||
it('caps the armed delay below the setTimeout ceiling', () => {
|
||||
// Why: Node clamps a delay past 2^31-1ms to 1ms — a documented "set the
|
||||
// idle window very high" configuration would otherwise spin the sweep at
|
||||
// 1ms forever on an idle host.
|
||||
const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout')
|
||||
const { reclaimer } = createReclaimer({
|
||||
pages: [page('a', 1_000_000)],
|
||||
budget: { ...BUDGET, idleMs: 100_000_000_000 }
|
||||
})
|
||||
reclaimer.reschedule()
|
||||
|
||||
expect(reclaimer.isScheduled).toBe(true)
|
||||
const delays = setTimeoutSpy.mock.calls.map((call) => call[1] as number)
|
||||
expect(delays.length).toBeGreaterThan(0)
|
||||
expect(Math.max(...delays)).toBeLessThanOrEqual(2_147_483_647)
|
||||
})
|
||||
|
||||
it('arms nothing when stop lands while a sweep is in flight', async () => {
|
||||
// Why: stop() clears sweepInFlight, so without a terminal flag the sweep's
|
||||
// own re-arm no longer sees itself in flight and schedules a fresh check
|
||||
|
||||
@@ -17,6 +17,9 @@ import { nextOffscreenBrowserReclaimCheckAt } from './offscreen-browser-page-rec
|
||||
// timer for the moment its own answer could next change. There is deliberately
|
||||
// no recurring sweep: a host doing nothing holds no timer at all.
|
||||
|
||||
/** Node's setTimeout ceiling; longer delays are clamped by Node to 1ms. */
|
||||
const MAX_RECLAIM_TIMER_DELAY_MS = 2_147_483_647
|
||||
|
||||
export type OffscreenBrowserReclaimerDeps = {
|
||||
pages: OffscreenBrowserOpenPages
|
||||
budget: BrowserRetentionBudget
|
||||
@@ -79,8 +82,13 @@ export class OffscreenBrowserPageReclaimer {
|
||||
}
|
||||
// Why the floor: a deadline in the past that the last sweep could not act on
|
||||
// would re-arm at 0ms, and a timer that re-arms itself instantly is a pegged
|
||||
// CPU on an otherwise idle server.
|
||||
const delay = Math.max(0, at - now, this.backoffUntil - now)
|
||||
// CPU on an otherwise idle server. Why the ceiling: Node clamps a setTimeout
|
||||
// beyond 2^31-1ms to 1ms — the same spin, reachable by configuring the idle
|
||||
// window past ~24.8 days. Re-checking at the horizon is harmless.
|
||||
const delay = Math.min(
|
||||
Math.max(0, at - now, this.backoffUntil - now),
|
||||
MAX_RECLAIM_TIMER_DELAY_MS
|
||||
)
|
||||
this.timer = setTimeout(() => {
|
||||
this.timer = null
|
||||
this.runSweep()
|
||||
|
||||
@@ -171,6 +171,22 @@ describe('headless parked-page targeting', () => {
|
||||
])
|
||||
})
|
||||
|
||||
it('reports listing indices, not bridge registration order, from show and switch', async () => {
|
||||
// Why: a park+wake permutes the bridge's registration order. The index a
|
||||
// caller gets back must name the same tab in the listing they read, or
|
||||
// chaining it into --index addresses a different tab.
|
||||
const { RuntimeBrowserCommands } = await import('./orca-runtime-browser')
|
||||
const fake = createFakeHeadlessHost(['c', 'a'], [], ['a', 'c'])
|
||||
const commands = new RuntimeBrowserCommands(fake.host)
|
||||
|
||||
const current = await commands.browserTabCurrent({ worktree: 'id:wt-1' })
|
||||
expect(current.tab.browserPageId).toBe('c')
|
||||
expect(current.tab.index).toBe(1)
|
||||
|
||||
const switched = await commands.browserTabSwitch({ page: 'c', worktree: 'id:wt-1' })
|
||||
expect(switched).toMatchObject({ browserPageId: 'c', switched: 1 })
|
||||
})
|
||||
|
||||
it('switches to the page the listed index named, not one an implicit wake promoted', async () => {
|
||||
// Why: with every page parked, resolving the worktree first wakes the most
|
||||
// recently used one, which makes it live and moves it to listing index 0.
|
||||
|
||||
@@ -655,6 +655,28 @@ export class RuntimeBrowserCommands {
|
||||
// it into opening yet another one, which is the leak this fixes. Every
|
||||
// index-addressed command resolves through this same list so `tab list`
|
||||
// indices and `--index` never disagree.
|
||||
/** Headless serve with the parked-aware, creation-ordered listing. */
|
||||
private isHeadlessOffscreenListing(): boolean {
|
||||
return (
|
||||
this.host.getOffscreenBrowserBackend()?.listOpenPageIds !== undefined &&
|
||||
!this.host.getAvailableAuthoritativeWindow()
|
||||
)
|
||||
}
|
||||
|
||||
/** The page's position in the creation-ordered listing; null on desktop. */
|
||||
private headlessListedTabIndex(
|
||||
worktreeId: string | undefined,
|
||||
browserPageId: string
|
||||
): number | null {
|
||||
if (!this.isHeadlessOffscreenListing()) {
|
||||
return null
|
||||
}
|
||||
const index = this.listBrowserTabsIncludingParked(worktreeId).findIndex(
|
||||
(tab) => tab.browserPageId === browserPageId
|
||||
)
|
||||
return index === -1 ? null : index
|
||||
}
|
||||
|
||||
private listBrowserTabsIncludingParked(
|
||||
worktreeId: string | undefined
|
||||
): BrowserTabListResult['tabs'] {
|
||||
@@ -770,12 +792,13 @@ export class RuntimeBrowserCommands {
|
||||
target.worktreeId,
|
||||
target.browserPageId
|
||||
)
|
||||
// Why (STA-4341): waking the target makes it live, which renumbers the
|
||||
// bridge's own listing. Report the index the caller passed — the one the
|
||||
// listing they read actually names — rather than a position that describes
|
||||
// neither their request nor what they were looking at.
|
||||
const result =
|
||||
indexed && params.index !== undefined ? { ...switched, switched: params.index } : switched
|
||||
// Why (STA-4341): the bridge computes `switched` over its registration
|
||||
// order, which any park+wake permutes. Report the target's position in the
|
||||
// creation-ordered listing instead — the same number `tab list` prints and
|
||||
// `--index` resolves against — or a caller chaining the reported number
|
||||
// into `--index` addresses a different tab than the one it switched to.
|
||||
const listedIndex = this.headlessListedTabIndex(target.worktreeId, switched.browserPageId)
|
||||
const result = listedIndex === null ? switched : { ...switched, switched: listedIndex }
|
||||
if (params.focus) {
|
||||
// Why: scope focus to the tab's owning worktree; the renderer never yanks the user across worktrees on this signal (see focusBrowserTabInWorktree).
|
||||
const worktreeId =
|
||||
@@ -1874,17 +1897,30 @@ export class RuntimeBrowserCommands {
|
||||
explicitWorktreeId?: string
|
||||
): BrowserTabListResult['tabs'][number] {
|
||||
const worktreeId = explicitWorktreeId ?? browserManager.getWorktreeIdForTab(browserPageId)
|
||||
const tab = this.requireAgentBrowserBridge()
|
||||
.tabList(worktreeId)
|
||||
.tabs.find((entry) => entry.browserPageId === browserPageId)
|
||||
if (!tab) {
|
||||
const scope = worktreeId ? ' in this worktree' : ''
|
||||
throw new BrowserError(
|
||||
'browser_tab_not_found',
|
||||
`Browser page ${browserPageId} was not found${scope}`
|
||||
// Why (STA-4341): the bridge numbers tabs by registration order, which any
|
||||
// park+wake permutes. Every index a headless caller sees must come from the
|
||||
// creation-ordered listing `tab list` prints, or `tab show`/`tab current`
|
||||
// report a number that names a different tab in that listing.
|
||||
if (this.isHeadlessOffscreenListing()) {
|
||||
const tab = this.listBrowserTabsIncludingParked(worktreeId).find(
|
||||
(entry) => entry.browserPageId === browserPageId
|
||||
)
|
||||
if (tab) {
|
||||
return tab
|
||||
}
|
||||
} else {
|
||||
const tab = this.requireAgentBrowserBridge()
|
||||
.tabList(worktreeId)
|
||||
.tabs.find((entry) => entry.browserPageId === browserPageId)
|
||||
if (tab) {
|
||||
return this.enrichBrowserTabInfo(tab)
|
||||
}
|
||||
}
|
||||
return this.enrichBrowserTabInfo(tab)
|
||||
const scope = worktreeId ? ' in this worktree' : ''
|
||||
throw new BrowserError(
|
||||
'browser_tab_not_found',
|
||||
`Browser page ${browserPageId} was not found${scope}`
|
||||
)
|
||||
}
|
||||
|
||||
// Why: headless serve path — the offscreen backend registers synchronously, so there is no webview-mount wait.
|
||||
|
||||
Reference in New Issue
Block a user