fix: Bug - Sending files between users doesn't really work (chunk-time size re-gate)
Remove the leftover MAX_AUTO_SAVE_SIZE_BYTES guard from handleFileChunk's in-memory path. The request gate (canReceiveAttachment) already admits 10-50 MB generic files for in-memory receive on stores without disk streaming (browser), but the chunk handler silently dropped every chunk of such files: no ack was sent, the sender's waitForAck timed out, and the receiver's GUI never changed. Receive admission is now decided once, at request time. Adds a two-browser regression e2e that sends an 11 MB generic file and asserts Request -> progress -> Download. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -107,7 +107,7 @@ Concurrent triggers (file-announce, message sync, peer connect) can race to requ
|
||||
|
||||
- **Requester:** `requestFromAnyPeer` marks the request pending *synchronously* before any async work, so the manager's `hasPendingRequest` gate closes the double-request race window.
|
||||
- **Sender:** `handleFileRequest` / `fulfillRequestWithFile` track active outbound streams per `(messageId, fileId, peerId)` and ignore duplicate requests while a stream is in flight. A fresh `file-request` clears any earlier `file-cancel` marker from that peer.
|
||||
- **Receiver:** chunk buffers are dense (`Array.from({ length: total })`, never sparse `new Array(total)`); a chunk index that is already buffered is ignored entirely and never counts toward `receivedBytes`; a transfer finalizes only when *every* chunk index is present — byte counters are never a substitute for chunk completeness. Assembly state is released only after the attachment is marked `available`, and chunks arriving for an already-available attachment are dropped. Files **≤ `MAX_AUTO_SAVE_SIZE_BYTES` (10 MB)** assemble in memory (parallel chunk receive, immediate `file-chunk-ack`) and are persisted after completion via `shouldPersistDownloadedAttachment`. **Oversized** persistable downloads (`> 10 MB`) append directly to disk when the store supports streaming (`canStreamToDisk`) — metadata `filePath` does not force an in-memory fallback. Disk-streamed receives decode each chunk once, append bytes through Electron IPC (`append-file-bytes`), and acknowledge the sender with `file-chunk-ack` so only one chunk is in flight at a time (preventing unbounded base64 retention in the renderer). Completed **images** ≤ 10 MB get an immediate `objectUrl` blob; oversized images stay on `savedPath` until inline display hydration runs on demand. Completed **audio/video** immediately resolve a playable URL via `attachmentStorage.getFileUrl(savedPath)` (Electron/Capacitor) or `ensureInlineDisplayObjectUrl` in the browser.
|
||||
- **Receiver:** chunk buffers are dense (`Array.from({ length: total })`, never sparse `new Array(total)`); a chunk index that is already buffered is ignored entirely and never counts toward `receivedBytes`; a transfer finalizes only when *every* chunk index is present — byte counters are never a substitute for chunk completeness. Assembly state is released only after the attachment is marked `available`, and chunks arriving for an already-available attachment are dropped. Files **≤ `MAX_AUTO_SAVE_SIZE_BYTES` (10 MB)** assemble in memory (parallel chunk receive, immediate `file-chunk-ack`) and are persisted after completion via `shouldPersistDownloadedAttachment`. **Oversized** persistable downloads (`> 10 MB`) append directly to disk when the store supports streaming (`canStreamToDisk`) — metadata `filePath` does not force an in-memory fallback. On stores that cannot stream (browser), oversized files the store can still persist (≤ 50 MB) assemble in memory instead. Whether a file can be received at all is decided once, at request time, by `canReceiveAttachment` — `handleFileChunk` must not re-gate on a stricter size cap, or it silently drops chunks the request gate already admitted (the receiver never acks, the sender's ack wait times out, and the download stalls at 0 bytes with no error). Disk-streamed receives decode each chunk once, append bytes through Electron IPC (`append-file-bytes`), and acknowledge the sender with `file-chunk-ack` so only one chunk is in flight at a time (preventing unbounded base64 retention in the renderer). Completed **images** ≤ 10 MB get an immediate `objectUrl` blob; oversized images stay on `savedPath` until inline display hydration runs on demand. Completed **audio/video** immediately resolve a playable URL via `attachmentStorage.getFileUrl(savedPath)` (Electron/Capacitor) or `ensureInlineDisplayObjectUrl` in the browser.
|
||||
- **Sender:** after each `file-chunk` the transport awaits the matching `file-chunk-ack` before sending the next chunk, in addition to data-channel bufferedAmount back-pressure.
|
||||
|
||||
### Failure handling
|
||||
@@ -214,3 +214,7 @@ Image inline previews on Electron/desktop use renderer `blob:` URLs rebuilt from
|
||||
- **Serving** is unaffected: peers still download from `savedPath` / `filePath`; blob URLs are display-only.
|
||||
|
||||
While a revoked image waits to rehydrate, chat renders the existing image-grid spinner skeleton (`isAttachmentPendingInlineHydration`).
|
||||
|
||||
## Cross-context feature docs
|
||||
|
||||
- [`agents-docs/features/attachments.md`](../../../../../agents-docs/features/attachments.md)
|
||||
|
||||
+30
@@ -629,6 +629,36 @@ describe('AttachmentTransferService', () => {
|
||||
expect(webrtc.sendToPeer).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('assembles generic files above the auto-save cap in memory when the store cannot stream but can persist them', async () => {
|
||||
// Browser receiver: no disk streaming, persistable up to 50 MB. The request
|
||||
// gate admits a 20 MB file for in-memory receive, so the chunk handler must
|
||||
// accept its chunks instead of dropping them with a file-too-large error.
|
||||
attachmentStorage.canStreamToDisk.mockReturnValue(false);
|
||||
attachmentStorage.canPersistSize.mockImplementation((bytes: number) => bytes <= 50 * 1024 * 1024);
|
||||
|
||||
const service = createService();
|
||||
const attachment = registerIncomingGenericFile(20 * 1024 * 1024);
|
||||
|
||||
service.handleFileChunk(chunkPayload(0, 2, [
|
||||
1,
|
||||
2,
|
||||
3
|
||||
]));
|
||||
|
||||
expect(attachment.requestError).toBeUndefined();
|
||||
expect(attachment.receivedBytes).toBe(3);
|
||||
|
||||
service.handleFileChunk(chunkPayload(1, 2, [
|
||||
4,
|
||||
5,
|
||||
6
|
||||
]));
|
||||
|
||||
await vi.waitFor(() => expect(attachment.available).toBe(true));
|
||||
|
||||
expect(attachment.objectUrl).toMatch(/^blob:/);
|
||||
});
|
||||
|
||||
it('assembles browser-sized generic files in memory when streaming is unavailable', async () => {
|
||||
attachmentStorage.canStreamToDisk.mockReturnValue(false);
|
||||
attachmentStorage.canPersistSize.mockImplementation((bytes: number) => bytes <= 50 * 1024 * 1024);
|
||||
|
||||
+5
-6
@@ -425,12 +425,11 @@ export class AttachmentTransferService {
|
||||
return;
|
||||
}
|
||||
|
||||
if (attachment.size > MAX_AUTO_SAVE_SIZE_BYTES) {
|
||||
attachment.requestError = this.appI18n.instant(ATTACHMENT_FILE_TOO_LARGE_KEY);
|
||||
this.runtimeStore.touch();
|
||||
return;
|
||||
}
|
||||
|
||||
// Reaching here means canReceiveAttachment passed and disk streaming is not
|
||||
// used, so the in-memory path is the agreed receive strategy - including
|
||||
// above-auto-save-cap files on stores that cannot stream but can persist
|
||||
// them (browser). A stricter size guard here would silently drop chunks the
|
||||
// request gate already admitted.
|
||||
const decodedBytes = this.transport.decodeBase64(data);
|
||||
const assemblyKey = `${messageId}:${fileId}`;
|
||||
const requestKey = this.buildRequestKey(messageId, fileId);
|
||||
|
||||
Reference in New Issue
Block a user