fix(browser): stage agent downloads synchronously so Electron never opens a Save dialog

The save path was set only after an asynchronous non-conflicting-name lookup.
When Electron's own path step won that race it opened a native Save dialog, so
an agent download stalled forever at full size (and a user would see a
surprise dialog). Downloads now write to a hidden staging file set during
will-download and are renamed to the allocated name when they complete.

Author: waleedlatif1

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

@@ -1,6 +1,6 @@
-import { mkdtempSync, writeFileSync } from 'node:fs'
+import { existsSync, mkdirSync, mkdtempSync, readdirSync, writeFileSync } from 'node:fs'
 import { tmpdir } from 'node:os'
-import { join } from 'node:path'
+import { basename, dirname, join } from 'node:path'
 import type { MenuItemConstructorOptions, WebContents } from 'electron'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
@@ -304,11 +304,32 @@ function mockDownloadItem({
       const handler = item.once.mock.calls.find(([eventName]) => eventName === 'done')?.[1] as
         | ((event: unknown, nextState: MockDownloadDoneState) => void)
         | undefined
+      const savePath = item.setSavePath.mock.calls.at(-1)?.[0] as string | undefined
+      if (state === 'completed' && savePath) writeFileSync(savePath, filename)
       handler?.({}, state)
     },
   }
 }
 
+/** Asserts Electron was only ever given the hidden staging file in `directory`. */
+function expectOnlyStagingSavePath(
+  item: { setSavePath: ReturnType<typeof vi.fn> },
+  directory: string
+) {
+  expect(item.setSavePath).toHaveBeenCalledOnce()
+  const savePath = item.setSavePath.mock.calls[0]?.[0] as string
+  expect(dirname(savePath)).toBe(directory)
+  expect(basename(savePath)).toMatch(/^\.sim-download-/)
+  return savePath
+}
+
+/** Visible files in a download directory, excluding in-flight staging files. */
+function finishedDownloadFiles(directory: string): string[] {
+  return readdirSync(directory)
+    .filter((name) => !name.startsWith('.'))
+    .sort()
+}
+
 function startMockDownload(contents: MockView['webContents'], download: MockDownloadHarness): void {
   const webSession = contents.session as typeof contents.session & {
     on: ReturnType<typeof vi.fn>
@@ -3357,7 +3378,7 @@ describe('browser-agent session', () => {
     expect(item.resume).not.toHaveBeenCalled()
     await vi.waitFor(() => expect(item.resume).toHaveBeenCalledOnce())
     expect(item.cancel).not.toHaveBeenCalled()
-    expect(item.setSavePath).toHaveBeenCalledWith(join(directory, 'report.csv'))
+    const stagingPath = expectOnlyStagingSavePath(item, directory)
     expect(item.once).toHaveBeenCalledWith('done', expect.any(Function))
     expect(onDownloadsChanged).toHaveBeenLastCalledWith({
       scopeId: 'chat-test',
@@ -3375,13 +3396,17 @@ describe('browser-agent session', () => {
     const done = item.once.mock.calls.find(([eventName]) => eventName === 'done')?.[1] as
       | ((event: unknown, state: 'completed') => void)
       | undefined
-    writeFileSync(join(directory, 'report.csv'), 'report')
+    writeFileSync(stagingPath, 'report')
     done?.({}, 'completed')
 
-    expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({
-      filename: 'report.csv',
-      state: 'completed',
-    })
+    await vi.waitFor(() =>
+      expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({
+        filename: 'report.csv',
+        state: 'completed',
+      })
+    )
+    expect(existsSync(stagingPath)).toBe(false)
+    expect(finishedDownloadFiles(directory)).toEqual(['report.csv'])
     expect(snapshots.get('chat-test')?.downloads[0]).toMatchObject({
       filename: 'report.csv',
       state: 'completed',
@@ -3410,6 +3435,131 @@ describe('browser-agent session', () => {
     })
   })
 
+  it('sets a staging save path before will-download returns so Electron never prompts', () => {
+    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: 'report.csv', totalBytes: 100 })
+
+    startMockDownload(contents, download)
+
+    const stagingPath = expectOnlyStagingSavePath(download.item, directory)
+    expect(stagingPath).not.toBe(join(directory, 'report.csv'))
+    expect(download.item.cancel).not.toHaveBeenCalled()
+  })
+
+  it('waits for a slow filename allocation before publishing the completed file', async () => {
+    const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
+    const { persistence, snapshots } = memoryBrowserPersistence()
+    const pathProbe = deferred<boolean>()
+    session = freshSession(win, {}, persistence, {
+      getDirectory: () => directory,
+      getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER,
+      pathExists: () => pathProbe.promise,
+    })
+    const contents = (session.ensureTab().view as unknown as MockView).webContents
+    const download = mockDownloadItem({ filename: 'slow.bin', totalBytes: 100 })
+
+    startMockDownload(contents, download)
+    const stagingPath = expectOnlyStagingSavePath(download.item, directory)
+    download.emitDone('completed')
+    await Promise.resolve()
+
+    expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({
+      filename: 'slow.bin',
+      state: 'progressing',
+    })
+    expect(existsSync(stagingPath)).toBe(true)
+
+    pathProbe.resolve(false)
+
+    await vi.waitFor(() =>
+      expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({
+        filename: 'slow.bin',
+        state: 'completed',
+      })
+    )
+    expect(existsSync(stagingPath)).toBe(false)
+    expect(finishedDownloadFiles(directory)).toEqual(['slow.bin'])
+    const { id } = session.getBrowserDownloadsState('chat-test').downloads[0]
+    expect(session.completedBrowserDownload('chat-test', id)).toEqual({
+      filename: 'slow.bin',
+      savePath: join(directory, 'slow.bin'),
+    })
+    expect(snapshots.get('chat-test')?.downloads[0]).toMatchObject({
+      state: 'completed',
+      savePath: join(directory, 'slow.bin'),
+    })
+  })
+
+  it('interrupts a completed download whose staging file cannot be moved into place', async () => {
+    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: 'blocked.bin', totalBytes: 100 })
+
+    startMockDownload(contents, download)
+    const stagingPath = expectOnlyStagingSavePath(download.item, directory)
+    await vi.waitFor(() => expect(download.item.resume).toHaveBeenCalledOnce())
+    mkdirSync(join(directory, 'blocked.bin'))
+    download.emitDone('completed')
+
+    await vi.waitFor(() =>
+      expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({
+        filename: 'blocked.bin',
+        state: 'interrupted',
+      })
+    )
+    await vi.waitFor(() => expect(existsSync(stagingPath)).toBe(false))
+    const { id } = session.getBrowserDownloadsState('chat-test').downloads[0]
+    expect(session.completedBrowserDownload('chat-test', id)).toBeNull()
+
+    vi.mocked(Menu.buildFromTemplate).mockClear()
+    session.showBrowserDownloadsMenu('chat-test', win, { x: 10, y: 20 })
+    const template = vi.mocked(Menu.buildFromTemplate).mock.calls[0]?.[0] as
+      | MenuItemConstructorOptions[]
+      | undefined
+    expect(template?.[0]?.sublabel).toContain(
+      'the finished download could not be moved to its destination'
+    )
+  })
+
+  it('removes the staging file when a download is cancelled or interrupted', async () => {
+    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 cancelled = mockDownloadItem({ filename: 'cancelled.bin', totalBytes: 100 })
+    const interrupted = mockDownloadItem({ filename: 'interrupted.bin', totalBytes: 100 })
+
+    startMockDownload(contents, cancelled)
+    startMockDownload(contents, interrupted)
+    await vi.waitFor(() => expect(cancelled.item.resume).toHaveBeenCalledOnce())
+    await vi.waitFor(() => expect(interrupted.item.resume).toHaveBeenCalledOnce())
+    const stagingPaths = [
+      expectOnlyStagingSavePath(cancelled.item, directory),
+      expectOnlyStagingSavePath(interrupted.item, directory),
+    ]
+    for (const stagingPath of stagingPaths) writeFileSync(stagingPath, 'partial')
+
+    cancelled.emitDone('cancelled')
+    interrupted.emitDone('interrupted')
+
+    await vi.waitFor(() => expect(readdirSync(directory)).toEqual([]))
+    expect(session.getBrowserDownloadsState('chat-test').downloads).toEqual([
+      expect.objectContaining({ filename: 'interrupted.bin', state: 'interrupted' }),
+      expect.objectContaining({ filename: 'cancelled.bin', state: 'cancelled' }),
+    ])
+  })
+
   it('does not let a pre-allocation progress event consume the admission probe', async () => {
     const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
     const getFreeDiskBytes = vi.fn(() => Number.MAX_SAFE_INTEGER)
@@ -3610,19 +3760,18 @@ describe('browser-agent session', () => {
       firstProbe.resolve(false)
       secondProbe.reject(new Error('late path lookup failure'))
       await vi.advanceTimersByTimeAsync(0)
-      expect(first.item.setSavePath).not.toHaveBeenCalled()
-      expect(second.item.setSavePath).not.toHaveBeenCalled()
+      expectOnlyStagingSavePath(first.item, directory)
+      expectOnlyStagingSavePath(second.item, directory)
       expect(first.item.resume).not.toHaveBeenCalled()
       expect(second.item.resume).not.toHaveBeenCalled()
 
       first.emitDone('cancelled')
       second.emitDone('cancelled')
       const replacement = mockDownloadItem({ filename: 'hung-path.bin', totalBytes: 100 })
       startMockDownload(contents, replacement)
-      await vi.waitFor(() =>
-        expect(replacement.item.setSavePath).toHaveBeenCalledWith(join(directory, 'hung-path.bin'))
-      )
       await vi.waitFor(() => expect(replacement.item.resume).toHaveBeenCalledOnce())
+      replacement.emitDone('completed')
+      await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toEqual(['hung-path.bin']))
     } finally {
       vi.useRealTimers()
     }
@@ -3694,7 +3843,7 @@ describe('browser-agent session', () => {
     startMockDownload(contents, download)
 
     await vi.waitFor(() => expect(download.item.cancel).toHaveBeenCalledOnce())
-    expect(download.item.setSavePath).not.toHaveBeenCalled()
+    expectOnlyStagingSavePath(download.item, notDirectory)
     expect(download.item.resume).not.toHaveBeenCalled()
     expect(session.getBrowserDownloadsState('chat-test').downloads[0]).toMatchObject({
       filename: 'cannot-save.bin',
@@ -3735,7 +3884,6 @@ describe('browser-agent session', () => {
 
     startMockDownload(contents, download)
     await vi.waitFor(() => expect(download.item.resume).toHaveBeenCalledOnce())
-    const firstSavePath = download.item.setSavePath.mock.calls[0]?.[0]
     download.setReceivedBytes(2 * 1024 ** 3 + 1)
     download.emitUpdated()
 
@@ -3750,7 +3898,9 @@ describe('browser-agent session', () => {
     const replacement = mockDownloadItem({ filename: 'stream.bin', totalBytes: 100 })
     startMockDownload(contents, replacement)
     expect(replacement.item.cancel).not.toHaveBeenCalled()
-    await vi.waitFor(() => expect(replacement.item.setSavePath).toHaveBeenCalledWith(firstSavePath))
+    await vi.waitFor(() => expect(replacement.item.resume).toHaveBeenCalledOnce())
+    replacement.emitDone('completed')
+    await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toEqual(['stream.bin']))
   })
 
   it('throttles free-disk checks while stopping promptly after the interval', async () => {
@@ -3867,9 +4017,11 @@ describe('browser-agent session', () => {
       .mockReturnValueOnce(firstProbe.promise)
       .mockReturnValueOnce(secondProbe.promise)
       .mockReturnValue(Number.MAX_SAFE_INTEGER)
+    const pathExists = vi.fn((_path: string) => false)
     session = freshSession(win, {}, persistence, {
       getDirectory: () => directory,
       getFreeDiskBytes,
+      pathExists,
     })
     const contents = (session.ensureTab().view as unknown as MockView).webContents
     const first = mockDownloadItem({ filename: 'same-name.bin', totalBytes: 100 })
@@ -3878,12 +4030,11 @@ describe('browser-agent session', () => {
     startMockDownload(contents, first)
     startMockDownload(contents, second)
     await vi.waitFor(() => expect(getFreeDiskBytes).toHaveBeenCalledTimes(2))
-    const allocatedPaths = [
-      first.item.setSavePath.mock.calls[0]?.[0],
-      second.item.setSavePath.mock.calls[0]?.[0],
-    ]
-    expect(new Set(allocatedPaths).size).toBe(2)
-    expect(allocatedPaths).toContain(join(directory, 'same-name.bin'))
+    expect(pathExists.mock.calls.map(([candidate]) => candidate)).toEqual([
+      join(directory, 'same-name.bin'),
+      join(directory, 'same-name.bin'),
+      expect.stringMatching(/same-name \(.+\)\.bin$/),
+    ])
 
     await session.clearProfileStorage()
 
@@ -3903,18 +4054,24 @@ describe('browser-agent session', () => {
     const nextContents = (session.ensureTab().view as unknown as MockView).webContents
     const replacement = mockDownloadItem({ filename: 'same-name.bin', totalBytes: 100 })
     startMockDownload(nextContents, replacement)
-    await vi.waitFor(() =>
-      expect(replacement.item.setSavePath).toHaveBeenCalledWith(join(directory, 'same-name.bin'))
-    )
     await vi.waitFor(() => expect(replacement.item.resume).toHaveBeenCalledOnce())
     expect(replacement.item.cancel).not.toHaveBeenCalled()
 
     first.emitDone('completed')
     second.emitDone('cancelled')
     const concurrent = mockDownloadItem({ filename: 'same-name.bin', totalBytes: 100 })
     startMockDownload(nextContents, concurrent)
-    await vi.waitFor(() => expect(concurrent.item.setSavePath).toHaveBeenCalledOnce())
-    expect(concurrent.item.setSavePath).not.toHaveBeenCalledWith(join(directory, 'same-name.bin'))
+    await vi.waitFor(() => expect(concurrent.item.resume).toHaveBeenCalledOnce())
+    concurrent.emitDone('completed')
+    await vi.waitFor(() =>
+      expect(finishedDownloadFiles(directory)).toEqual([
+        expect.stringMatching(/^same-name \(.+\)\.bin$/),
+      ])
+    )
+    replacement.emitDone('completed')
+    await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toHaveLength(2))
+    expect(finishedDownloadFiles(directory)).toContain('same-name.bin')
+    await vi.waitFor(() => expect(readdirSync(directory)).toHaveLength(2))
   })
 
   it('does not reserve a late filename after profile teardown starts', async () => {
@@ -3931,7 +4088,7 @@ describe('browser-agent session', () => {
     await Promise.resolve()
 
     expect(download.item.cancel).toHaveBeenCalledOnce()
-    expect(download.item.setSavePath).not.toHaveBeenCalled()
+    expectOnlyStagingSavePath(download.item, directory)
     expect(download.item.resume).not.toHaveBeenCalled()
   })
 
@@ -3970,15 +4127,21 @@ describe('browser-agent session', () => {
     queueMicrotask(() => session.disposeBrowserScope('chat-first'))
     secondPathProbe.resolve(false)
 
-    await vi.waitFor(() =>
-      expect(second.item.setSavePath).toHaveBeenCalledWith(join(directory, 'shared.bin'))
-    )
-    expect(first.item.setSavePath).not.toHaveBeenCalled()
+    await vi.waitFor(() => expect(second.item.resume).toHaveBeenCalledOnce())
+    expectOnlyStagingSavePath(first.item, directory)
+    expect(first.item.resume).not.toHaveBeenCalled()
 
     const third = mockDownloadItem({ filename: 'shared.bin', totalBytes: 100 })
     startMockDownload(thirdContents, third)
-    await vi.waitFor(() => expect(third.item.setSavePath).toHaveBeenCalledOnce())
-    expect(third.item.setSavePath).not.toHaveBeenCalledWith(join(directory, 'shared.bin'))
+    await vi.waitFor(() => expect(third.item.resume).toHaveBeenCalledOnce())
+    third.emitDone('completed')
+    await vi.waitFor(() =>
+      expect(finishedDownloadFiles(directory)).toEqual([
+        expect.stringMatching(/^shared \(.+\)\.bin$/),
+      ])
+    )
+    second.emitDone('completed')
+    await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toContain('shared.bin'))
   })
 
   it('cancels only the disposed scope and ignores its late download callbacks', async () => {
@@ -4017,7 +4180,7 @@ describe('browser-agent session', () => {
     disposedDownload.emitUpdated()
     disposedDownload.emitDone('cancelled')
 
-    expect(disposedDownload.item.setSavePath).not.toHaveBeenCalled()
+    expectOnlyStagingSavePath(disposedDownload.item, directory)
     expect(disposedDownload.item.resume).not.toHaveBeenCalled()
     expect(session.getBrowserDownloadsState('chat-disposed').downloads).toEqual([])
     expect(onDownloadsChanged).not.toHaveBeenCalledWith(
@@ -4101,10 +4264,11 @@ describe('browser-agent session', () => {
     expect(rejected.item.cancel).toHaveBeenCalledOnce()
 
     first.emitDone('completed')
+    await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toEqual(['first.txt']))
     const replacement = mockDownloadItem({ filename: 'fourth.txt', totalBytes: 100 })
     startMockDownload(contents, replacement)
     expect(replacement.item.cancel).not.toHaveBeenCalled()
-    await vi.waitFor(() => expect(replacement.item.setSavePath).toHaveBeenCalledOnce())
+    await vi.waitFor(() => expect(replacement.item.resume).toHaveBeenCalledOnce())
   })
 
   it('bounds active browser downloads across tasks', () => {