fix(browser): close cancellation and teardown races in holds and downloads

- An already-aborted click presses nothing, and a held click marks its outcome
  pending before dispatch, so cancelling mid-hold reports an unknown outcome
  with doNotRetry instead of an error that invites a retry.
- The disablePortal exemption resets when a visibility walk crosses from an
  iframe into its host page, where the host's own aria-hidden applies.
- Teardown keeps a download's name reserved until its placeholder claim
  settles, so a newer download cannot be handed a name the claim then takes.
- Downloads are paused before the staging path is set, as on staging, so a
  pause failure leaves no staging file behind.

Author: waleedlatif1

apps/desktop/src/main/browser-agent/cdp.test.ts

@@ -276,6 +276,17 @@ describe('browser-agent CDP instrumentation', () => {
     }
   })
 
+  it('presses nothing when its click was aborted before dispatch', async () => {
+    const contents = new WebContentsView().webContents
+    const controller = new AbortController()
+    controller.abort()
+
+    await expect(
+      clickAt(contents, 5, 6, false, PRIMARY_CLICK, controller.signal)
+    ).rejects.toMatchObject({ name: 'AbortError' })
+    expect(contents.debugger.sendCommand).not.toHaveBeenCalled()
+  })
+
   it('releases a held button as soon as its click is aborted', async () => {
     const contents = new WebContentsView().webContents
     const types = () =>

apps/desktop/src/main/browser-agent/cdp.ts

@@ -984,9 +984,10 @@ export function clearAgentContextMenu(contents: WebContents): void {
 }
 
 /**
- * Clicks at viewport coordinates. `signal` ends a press-and-hold early: the click rejects with the
- * abort reason and the button is released at once, so a cancelled or timed-out click cannot stay
- * held into the next action. That release can still activate the control under the pointer.
+ * Clicks at viewport coordinates. An already-aborted `signal` rejects before anything is pressed.
+ * During a press-and-hold it ends the hold early: the click rejects with the abort reason and the
+ * button is released at once, so a cancelled or timed-out click cannot stay held into the next
+ * action. That release can still activate the control under the pointer.
  */
 export async function clickAt(
   contents: WebContents,
@@ -997,6 +998,7 @@ export async function clickAt(
   signal?: AbortSignal
 ): Promise<void> {
   if (moveBeforePress) await moveMouse(contents, x, y)
+  signal?.throwIfAborted()
   const { button, clickCount, modifiers, holdMs } = click
   const buttons = BUTTON_MASKS[button]
   let pressed = false

apps/desktop/src/main/browser-agent/driver.test.ts

@@ -3603,6 +3603,34 @@ describe('credential protection', () => {
     })
   })
 
+  it('reports a press-and-hold cancelled mid-hold as an unknown outcome', async () => {
+    const contents = await openPage()
+    respondWith(contents, {})
+
+    const pending = driver.executeTool(
+      'chat-test',
+      'browser_click',
+      { elementId: 0, holdMs: 5_000 },
+      'hold-call'
+    )
+    await vi.waitFor(() =>
+      expect(
+        cdpCalls(contents, 'Input.dispatchMouseEvent').some(
+          ([, params]) => toRecord(params).type === 'mousePressed'
+        )
+      ).toBe(true)
+    )
+    driver.cancelTool('chat-test', 'hold-call')
+
+    await expect(pending).resolves.toMatchObject({
+      ok: true,
+      result: { outcomeUnknown: true, doNotRetry: true },
+    })
+    expect(
+      cdpCalls(contents, 'Input.dispatchMouseEvent').map(([, params]) => toRecord(params).type)
+    ).toContain('mouseReleased')
+  })
+
   it('rejects batches that name non-action tools or observe per action', async () => {
     await openPage()
 

apps/desktop/src/main/browser-agent/driver.ts

@@ -3223,6 +3223,9 @@ async function executeToolInner(
         try {
           assertCurrentExecution()
           assertElementActionCurrent(contents, elementId, target)
+          // A hold keeps the press in flight for seconds; cancelling it mid-gesture must read as
+          // an outcome that may have acted, never as a click that did not start.
+          if (click.holdMs > 0) onActionOutcome?.({ status: 'pending' })
           await cdp.clickAt(contents, x, y, false, click, signal)
           trusted = true
           activation = 'native-pointer'
@@ -3263,6 +3266,7 @@ async function executeToolInner(
           try {
             assertCurrentExecution()
             assertElementActionCurrent(contents, elementId, target)
+            if (click.holdMs > 0) onActionOutcome?.({ status: 'pending' })
             await cdp.clickAt(contents, finalTopPoint.x, finalTopPoint.y, false, click, signal)
             trusted = true
             activation = 'native-pointer'
@@ -4641,6 +4645,7 @@ async function executeToolInner(
       const beforeElement = await activeElementState(contents)
       assertCurrentExecution()
       assertActiveContents(contents, clickNavigationEpoch)
+      if (click.holdMs > 0) onActionOutcome?.({ status: 'pending' })
       try {
         await cdp.clickAt(contents, x, y, true, click, signal)
       } catch (error) {

apps/desktop/src/main/browser-agent/page-functions.test.ts

@@ -2570,6 +2570,52 @@ describe('modal hidden together with its own app root', () => {
     expect(outline).not.toContain('Framed compose')
   })
 
+  it('does not carry a framed modal exemption into the host page', () => {
+    document.body.innerHTML = '<div id="app" aria-hidden="true"></div>'
+    const frame = visible(document.createElement('iframe'))
+    ;(document.getElementById('app') as HTMLElement).append(frame)
+    const inner = frame.contentDocument as Document
+    inner.body.innerHTML = `
+      <div id="root" aria-hidden="true">
+        <div role="dialog" aria-modal="true" aria-label="Framed"><button>Framed send</button></div>
+      </div>`
+    for (const element of Array.from(inner.body.querySelectorAll('*'))) visible(element)
+    register(inner.querySelector('button') as HTMLButtonElement)
+
+    expect(runSerialized(clickElement, [0, false])).toMatchObject({ error: 'not-visible' })
+  })
+
+  it('does not scroll a framed modal list whose host frame is hidden', () => {
+    document.body.innerHTML = '<div id="app" aria-hidden="true"></div>'
+    const frame = visible(document.createElement('iframe'))
+    ;(document.getElementById('app') as HTMLElement).append(frame)
+    const inner = frame.contentDocument as Document
+    inner.body.innerHTML = `
+      <div id="root" aria-hidden="true">
+        <div role="dialog" aria-modal="true" aria-label="Framed">
+          <div id="list" style="overflow-y: auto"><div>row</div></div>
+        </div>
+      </div>`
+    for (const element of Array.from(inner.body.querySelectorAll('*'))) visible(element)
+    const list = inner.getElementById('list') as HTMLDivElement
+    Object.defineProperties(list, {
+      clientHeight: { configurable: true, value: 200 },
+      scrollHeight: { configurable: true, value: 1_000 },
+      scrollTop: { configurable: true, writable: true, value: 0 },
+      scrollBy: {
+        configurable: true,
+        value: ({ top }: ScrollToOptions) => {
+          list.scrollTop += top || 0
+        },
+      },
+    })
+    register(list.firstElementChild as HTMLDivElement)
+
+    scrollPage('down', 100, 0)
+
+    expect(list.scrollTop).toBe(0)
+  })
+
   it('exposes a disablePortal modal nested inside another open modal', () => {
     document.body.innerHTML = `
       <div id="__next" aria-hidden="true"><main>

apps/desktop/src/main/browser-agent/page-functions.ts

@@ -1123,6 +1123,8 @@ export function clickElement(
       if ('host' in root) current = root.host as Element
       else {
         const frame: Element | null = current.ownerDocument.defaultView?.frameElement ?? null
+        // The modal exemption belongs to one document; the host page's own aria-hidden applies.
+        aboveExemptModal = false
         current = frame ? (frame as Element) : null
       }
     }
@@ -2662,6 +2664,8 @@ export function scrollPage(direction: string, amount?: number, elementId?: numbe
         if ('host' in root) current = root.host as Element
         else {
           const frame: Element | null = current.ownerDocument.defaultView?.frameElement ?? null
+          // The modal exemption belongs to one document; the host page's own aria-hidden applies.
+          aboveExemptModal = false
           current = frame
         }
       }

apps/desktop/src/main/browser-agent/session.test.ts

@@ -4199,6 +4199,66 @@ describe('browser-agent session', () => {
     expect(download.item.resume).not.toHaveBeenCalled()
   })
 
+  it('cancels a download it cannot pause before giving it a staging file', () => {
+    const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
+    session = freshSession(win, {}, undefined, {
+      getDirectory: () => directory,
+      getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER,
+    })
+    const contents = (session.ensureTab().view as unknown as MockView).webContents
+    const download = mockDownloadItem({ filename: 'unpausable.bin', totalBytes: 100 })
+    download.item.pause.mockImplementation(() => {
+      throw new Error('pause unavailable')
+    })
+
+    startMockDownload(contents, download)
+
+    expect(download.item.cancel).toHaveBeenCalledOnce()
+    expect(download.item.setSavePath).not.toHaveBeenCalled()
+    expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({
+      filename: 'unpausable.bin',
+      state: 'interrupted',
+    })
+  })
+
+  it('keeps a torn-down download name reserved until its pending claim settles', async () => {
+    const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
+    const claims: Array<{ path: string; gate: ReturnType<typeof deferred<void>> }> = []
+    session = freshSession(win, {}, undefined, {
+      getDirectory: () => directory,
+      getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER,
+      claimFile: async (path) => {
+        const gate = deferred<void>()
+        claims.push({ path, gate })
+        await gate.promise
+        writeFileSync(path, '', { flag: 'wx' })
+      },
+    })
+    const firstContents = session.withBrowserScope(
+      'chat-first',
+      () => (session.ensureTab().view as unknown as MockView).webContents
+    )
+    const secondContents = session.withBrowserScope(
+      'chat-second',
+      () => (session.ensureTab().view as unknown as MockView).webContents
+    )
+    const first = mockDownloadItem({ filename: 'report.bin', totalBytes: 100 })
+    const second = mockDownloadItem({ filename: 'report.bin', totalBytes: 100 })
+
+    startMockDownload(firstContents, first)
+    await vi.waitFor(() => expect(claims).toHaveLength(1))
+    session.disposeBrowserScope('chat-first')
+    startMockDownload(secondContents, second)
+    await vi.waitFor(() => expect(claims).toHaveLength(2))
+
+    expect(claims[1].path).not.toBe(claims[0].path)
+    claims[0].gate.resolve()
+    claims[1].gate.resolve()
+    await vi.waitFor(() => expect(second.item.resume).toHaveBeenCalledOnce())
+    expect(second.item.cancel).not.toHaveBeenCalled()
+    await vi.waitFor(() => expect(existsSync(claims[0].path)).toBe(false))
+  })
+
   it('does not let a cancelled allocation release another download path owner', async () => {
     const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
     const firstPathProbe = deferred<boolean>()

apps/desktop/src/main/browser-agent/session.ts

@@ -144,6 +144,8 @@ export interface BrowserDownloadSettings {
   pathExists?: (path: string) => boolean | Promise<boolean>
   /** Overrides the move of a completed staging file to its final name. */
   moveFile?: (from: string, to: string) => Promise<void>
+  /** Overrides the exclusive creation of a download's destination placeholder. */
+  claimFile?: (path: string) => Promise<void>
 }
 
 export interface AgentSessionEvents {
@@ -441,6 +443,11 @@ interface ActiveBrowserDownload {
    * program choosing a name sees it taken; the completed file replaces it.
    */
   placeholderPath?: string
+  /**
+   * Set while the placeholder write is in flight. The name stays reserved in process until it
+   * settles, so teardown cannot hand the name to a newer download that the write then beats.
+   */
+  claimingDestination?: boolean
   /** Settles with the final destination, or null when allocation failed. */
   destination: Promise<string | null>
   /** Set once Electron reports the item done, so a late disk check never resumes or cancels it. */
@@ -716,7 +723,7 @@ function releaseActiveBrowserDownload(active: ActiveBrowserDownload): void {
   if (active.terminal) return
   active.terminal = true
   activeBrowserDownloads.delete(active)
-  releaseActiveBrowserDownloadPath(active)
+  if (!active.claimingDestination) releaseActiveBrowserDownloadPath(active)
   // Once Electron reports the item done, the move owns the placeholder until it settles.
   if (!active.finished) removeBrowserDownloadPlaceholder(active)
 }
@@ -756,12 +763,16 @@ function discardStagedBrowserDownload(active: ActiveBrowserDownload): void {
   removeBrowserDownloadPlaceholder(active)
 }
 
+function createEmptyFileExclusively(path: string): Promise<void> {
+  return writeFile(path, '', { flag: 'wx' })
+}
+
 /** Claims the allocated destination with an empty file; throws if anything already holds it. */
 async function claimBrowserDownloadDestination(
   active: ActiveBrowserDownload,
   savePath: string
 ): Promise<void> {
-  await writeFile(savePath, '', { flag: 'wx' })
+  await (browserDownloadSettings?.claimFile ?? createEmptyFileExclusively)(savePath)
   active.placeholderPath = savePath
 }
 
@@ -1635,23 +1646,24 @@ function configureBrowserDownloads(ses: Session): void {
       withBrowserScope(scopeId, persistBrowserSession)
       logger.warn(message, { error: getErrorMessage(error), filename })
     }
-    const stagingPath = join(directory, `.sim-download-${generateShortId()}`)
+    // Paused before the staging path is set, so a failure here leaves no file behind.
     try {
-      item.setSavePath(stagingPath)
+      item.pause()
     } catch (error) {
       failDownloadSetup(
-        'Stopped: the download destination could not be prepared safely',
-        'Could not set the staging destination for an agent browser download',
+        'Stopped: the download could not be paused for a disk-space safety check',
+        'Agent browser download could not be paused for admission',
         error
       )
       return
     }
+    const stagingPath = join(directory, `.sim-download-${generateShortId()}`)
     try {
-      item.pause()
+      item.setSavePath(stagingPath)
     } catch (error) {
       failDownloadSetup(
-        'Stopped: the download could not be paused for a disk-space safety check',
-        'Agent browser download could not be paused for admission',
+        'Stopped: the download destination could not be prepared safely',
+        'Could not set the staging destination for an agent browser download',
         error
       )
       return
@@ -1768,11 +1780,17 @@ function configureBrowserDownloads(ses: Session): void {
           publishActiveBrowserDownload(active)
           return null
         }
-        await claimBrowserDownloadDestination(active, savePath)
-        if (active.terminal || !activeBrowserDownloads.has(active)) {
-          removeBrowserDownloadPlaceholder(active)
-          return null
+        active.claimingDestination = true
+        try {
+          await claimBrowserDownloadDestination(active, savePath)
+        } finally {
+          active.claimingDestination = false
+          if (active.terminal || !activeBrowserDownloads.has(active)) {
+            removeBrowserDownloadPlaceholder(active)
+            releaseActiveBrowserDownloadPath(active, savePath)
+          }
         }
+        if (active.terminal || !activeBrowserDownloads.has(active)) return null
         checkBrowserDownloadDiskSpace(active, 'admission')
         return savePath
       })