fix(browser): claim agent download names on disk while bytes stage

The allocated destination gets an empty placeholder created with O_EXCL, as
Firefox does, so another program picking a name sees it taken and the final
rename only ever replaces Sim's own placeholder. A name that something else
grabbed first stops the download instead of being overwritten. Teardown and
failures remove the placeholder once, so a late-settling download cannot delete
a name a newer download has claimed.

Author: waleedlatif1

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

@@ -1,4 +1,11 @@
-import { existsSync, mkdirSync, mkdtempSync, readdirSync, renameSync, writeFileSync } from 'node:fs'
+import {
+  existsSync,
+  mkdtempSync,
+  readdirSync,
+  readFileSync,
+  renameSync,
+  writeFileSync,
+} from 'node:fs'
 import { tmpdir } from 'node:os'
 import { basename, dirname, join } from 'node:path'
 import type { MenuItemConstructorOptions, WebContents } from 'electron'
@@ -3570,19 +3577,41 @@ describe('browser-agent session', () => {
     }
   })
 
+  it('never overwrites a file that takes the allocated name before the download claims it', async () => {
+    const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
+    writeFileSync(join(directory, 'taken.bin'), 'user data')
+    session = freshSession(win, {}, undefined, {
+      getDirectory: () => directory,
+      getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER,
+      pathExists: () => false,
+    })
+    const contents = (session.ensureTab().view as unknown as MockView).webContents
+    const download = mockDownloadItem({ filename: 'taken.bin', totalBytes: 100 })
+
+    startMockDownload(contents, download)
+
+    await vi.waitFor(() => expect(download.item.cancel).toHaveBeenCalledOnce())
+    expect(download.item.resume).not.toHaveBeenCalled()
+    expect(readFileSync(join(directory, 'taken.bin'), 'utf8')).toBe('user data')
+  })
+
   it('interrupts a completed download whose staging file cannot be moved into place', async () => {
     const directory = mkdtempSync(join(tmpdir(), 'sim-browser-downloads-'))
+    const moveFile = vi.fn(() =>
+      Promise.reject(Object.assign(new Error('cross-device link'), { code: 'EXDEV' }))
+    )
     session = freshSession(win, {}, undefined, {
       getDirectory: () => directory,
       getFreeDiskBytes: () => Number.MAX_SAFE_INTEGER,
+      moveFile,
     })
     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'))
+    expect(readFileSync(join(directory, 'blocked.bin'), 'utf8')).toBe('')
     download.emitDone('completed')
 
     await vi.waitFor(() =>
@@ -3591,7 +3620,9 @@ describe('browser-agent session', () => {
         state: 'interrupted',
       })
     )
-    await vi.waitFor(() => expect(existsSync(stagingPath)).toBe(false))
+    await vi.waitFor(() => expect(readdirSync(directory)).toEqual([]))
+    expect(existsSync(stagingPath)).toBe(false)
+    expect(moveFile).toHaveBeenCalledOnce()
     const { id } = session.getBrowserDownloadsState('chat-test').downloads[0]
     expect(session.completedBrowserDownload('chat-test', id)).toBeNull()
 
@@ -3970,6 +4001,7 @@ describe('browser-agent session', () => {
     })
 
     download.emitDone('cancelled')
+    await vi.waitFor(() => expect(readdirSync(directory)).toEqual([]))
     const replacement = mockDownloadItem({ filename: 'stream.bin', totalBytes: 100 })
     startMockDownload(contents, replacement)
     expect(replacement.item.cancel).not.toHaveBeenCalled()
@@ -4118,6 +4150,7 @@ describe('browser-agent session', () => {
     expect(second.item.resume).not.toHaveBeenCalled()
     expect(session.getBrowserDownloadsState('chat-test').downloads).toEqual([])
     expect(snapshots.get('chat-test')?.downloads).toEqual([])
+    await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toEqual([]))
 
     const nextContents = (session.ensureTab().view as unknown as MockView).webContents
     const replacement = mockDownloadItem({ filename: 'same-name.bin', totalBytes: 100 })
@@ -4131,14 +4164,18 @@ describe('browser-agent session', () => {
     startMockDownload(nextContents, concurrent)
     await vi.waitFor(() => expect(concurrent.item.resume).toHaveBeenCalledOnce())
     concurrent.emitDone('completed')
+    // The torn-down downloads settling late must not remove the replacement's claimed name.
     await vi.waitFor(() =>
       expect(finishedDownloadFiles(directory)).toEqual([
         expect.stringMatching(/^same-name \(.+\)\.bin$/),
+        'same-name.bin',
       ])
     )
+    expect(readFileSync(join(directory, 'same-name.bin'), 'utf8')).toBe('')
     replacement.emitDone('completed')
-    await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toHaveLength(2))
-    expect(finishedDownloadFiles(directory)).toContain('same-name.bin')
+    await vi.waitFor(() =>
+      expect(readFileSync(join(directory, 'same-name.bin'), 'utf8')).toBe('same-name.bin')
+    )
     await vi.waitFor(() =>
       expect(readdirSync(directory).filter((name) => name.startsWith('.'))).toEqual([])
     )
@@ -4208,10 +4245,13 @@ describe('browser-agent session', () => {
     await vi.waitFor(() =>
       expect(finishedDownloadFiles(directory)).toEqual([
         expect.stringMatching(/^shared \(.+\)\.bin$/),
+        'shared.bin',
       ])
     )
     second.emitDone('completed')
-    await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toContain('shared.bin'))
+    await vi.waitFor(() =>
+      expect(readFileSync(join(directory, 'shared.bin'), 'utf8')).toBe('shared.bin')
+    )
   })
 
   it('cancels only the disposed scope and ignores its late download callbacks', async () => {
@@ -4334,7 +4374,9 @@ describe('browser-agent session', () => {
     expect(rejected.item.cancel).toHaveBeenCalledOnce()
 
     first.emitDone('completed')
-    await vi.waitFor(() => expect(finishedDownloadFiles(directory)).toEqual(['first.txt']))
+    await vi.waitFor(() =>
+      expect(readFileSync(join(directory, 'first.txt'), 'utf8')).toBe('first.txt')
+    )
     const replacement = mockDownloadItem({ filename: 'fourth.txt', totalBytes: 100 })
     startMockDownload(contents, replacement)
     expect(replacement.item.cancel).not.toHaveBeenCalled()

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

@@ -1,6 +1,6 @@
 import { AsyncLocalStorage } from 'node:async_hooks'
 import { existsSync } from 'node:fs'
-import { rename, rm, statfs } from 'node:fs/promises'
+import { rename, rm, statfs, writeFile } from 'node:fs/promises'
 import { join } from 'node:path'
 import type {
   BrowserDataKind,
@@ -436,6 +436,11 @@ interface ActiveBrowserDownload {
   stagingPath: string
   /** The reserved final destination, once allocation has chosen one. */
   savePath?: string
+  /**
+   * The empty file claiming `savePath` on disk while bytes stage, as Firefox does, so another
+   * program choosing a name sees it taken; the completed file replaces it.
+   */
+  placeholderPath?: string
   /** 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. */
@@ -712,6 +717,8 @@ function releaseActiveBrowserDownload(active: ActiveBrowserDownload): void {
   active.terminal = true
   activeBrowserDownloads.delete(active)
   releaseActiveBrowserDownloadPath(active)
+  // Once Electron reports the item done, the move owns the placeholder until it settles.
+  if (!active.finished) removeBrowserDownloadPlaceholder(active)
 }
 
 function releaseActiveBrowserDownloadPath(
@@ -723,15 +730,41 @@ function releaseActiveBrowserDownloadPath(
   }
 }
 
-function discardStagedBrowserDownload(active: ActiveBrowserDownload): void {
-  void rm(active.stagingPath, { force: true }).catch((error) => {
+function removeBrowserDownloadFile(active: ActiveBrowserDownload, path: string): void {
+  void rm(path, { force: true }).catch((error) => {
     logger.warn('Could not remove a staged agent browser download', {
       error: getErrorMessage(error),
       filename: active.download.filename,
     })
   })
 }
 
+/**
+ * Removes the destination placeholder at most once: after that the name is free, and a later
+ * download may already have claimed it.
+ */
+function removeBrowserDownloadPlaceholder(active: ActiveBrowserDownload): void {
+  const { placeholderPath } = active
+  if (!placeholderPath) return
+  active.placeholderPath = undefined
+  removeBrowserDownloadFile(active, placeholderPath)
+}
+
+/** Removes a failed download's staging file and its destination placeholder. */
+function discardStagedBrowserDownload(active: ActiveBrowserDownload): void {
+  removeBrowserDownloadFile(active, active.stagingPath)
+  removeBrowserDownloadPlaceholder(active)
+}
+
+/** 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' })
+  active.placeholderPath = savePath
+}
+
 /**
  * Errors a just-written file raises while antivirus or indexing briefly holds it open (Windows);
  * Chromium retries its own final download rename on these too.
@@ -1722,7 +1755,7 @@ function configureBrowserDownloads(ses: Session): void {
         allocationExpired = true
       }
     )
-      .then((savePath) => {
+      .then(async (savePath) => {
         if (active.terminal || !activeBrowserDownloads.has(active)) {
           releaseActiveBrowserDownloadPath(active, savePath ?? undefined)
           return null
@@ -1735,6 +1768,11 @@ function configureBrowserDownloads(ses: Session): void {
           publishActiveBrowserDownload(active)
           return null
         }
+        await claimBrowserDownloadDestination(active, savePath)
+        if (active.terminal || !activeBrowserDownloads.has(active)) {
+          removeBrowserDownloadPlaceholder(active)
+          return null
+        }
         checkBrowserDownloadDiskSpace(active, 'admission')
         return savePath
       })