From 590e487250c2ce051e7196196e451fa9e381c34c Mon Sep 17 00:00:00 2001 From: Myx Date: Mon, 13 Jul 2026 18:48:30 +0200 Subject: [PATCH] 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 --- agents-docs/LESSONS.md | 7 ++ .../chat/large-generic-file-transfer.spec.ts | 114 ++++++++++++++++++ toju-app/src/app/domains/attachment/README.md | 6 +- .../attachment-transfer.service.spec.ts | 30 +++++ .../services/attachment-transfer.service.ts | 11 +- 5 files changed, 161 insertions(+), 7 deletions(-) create mode 100644 e2e/tests/chat/large-generic-file-transfer.spec.ts diff --git a/agents-docs/LESSONS.md b/agents-docs/LESSONS.md index 55238b7..d0e62fb 100644 --- a/agents-docs/LESSONS.md +++ b/agents-docs/LESSONS.md @@ -25,6 +25,13 @@ Durable rules for AI agents working on this project. Read this file at session s ## Lessons +### Decide attachment receive admission once at request time; never re-gate size in the chunk handler [attachments] + +- **Trigger:** "Sending files between users doesn't really work" — a browser user clicked Request on a 10–50 MB generic file, the request gate (`canReceiveAttachment`) admitted it for in-memory receive, the sender streamed chunks, but `handleFileChunk` still had a leftover hard `size > MAX_AUTO_SAVE_SIZE_BYTES` rejection on the in-memory path, so every chunk was dropped, no ack was ever sent, the sender's `waitForAck` timed out, and the GUI never changed. +- **Rule:** `canReceiveAttachment` (request time) is the single admission decision; the chunk handler may only route between disk-streaming and in-memory assembly — any stricter size check there silently drops chunks the request gate already admitted. +- **Why:** the failure is invisible in logs-from-the-outside: the sender's per-chunk sends look like a working transfer ("packages with size 32kb") until the ack timeout, and the receiver sets `requestError` only into memory that a re-request immediately clears — the user just sees a dead Request button. +- **Example:** removed the `MAX_AUTO_SAVE_SIZE_BYTES` guard in `attachment-transfer.service.ts#handleFileChunk`; regression e2e `e2e/tests/chat/large-generic-file-transfer.spec.ts` sends an 11 MB `.bin` between two browser clients and asserts Request → progress → Download (fails on the old code, passes after). + ### Re-queue attachment auto-downloads on every message/room binding event; never trust one transport's ordering [attachments] [realtime] - **Trigger:** cross-user attachment sync e2e (`chat-message-features.spec.ts`) flaked ~50%: `file-announce` (WebRTC data channel) beat `chat-message` (signaling websocket) to the receiver, so the announce-time auto-download resolved `roomId=null`, silently gave up, and nothing ever retried — the receiver showed "Waiting for image source..." forever. A related bug: the stalled-download reset keyed only on "receivedBytes>0 && no pending request", but the pending-request marker is deleted on the *first* chunk, so any auto-download pass during an active transfer cancelled it mid-stream and the retry deadlocked against the sender's active-transfer dedupe. diff --git a/e2e/tests/chat/large-generic-file-transfer.spec.ts b/e2e/tests/chat/large-generic-file-transfer.spec.ts new file mode 100644 index 0000000..ca17e9a --- /dev/null +++ b/e2e/tests/chat/large-generic-file-transfer.spec.ts @@ -0,0 +1,114 @@ +import { test, expect } from '../../fixtures/multi-client'; +import { RegisterPage } from '../../pages/register.page'; +import { ServerSearchPage } from '../../pages/server-search.page'; +import { ChatMessagesPage } from '../../pages/chat-messages.page'; + +/** + * Regression coverage for "Sending files between users doesn't really work": + * a generic (non-media) file above the 10 MB auto-save cap sent to a browser + * receiver. The receiver clicks Request; previously the chunk handler dropped + * every incoming chunk with a silent file-too-large error, the sender's ack + * wait timed out, and the GUI never changed. + */ +const LARGE_FILE_SIZE_BYTES = 11 * 1024 * 1024; + +test.describe('Large generic file transfer', () => { + test.describe.configure({ timeout: 420_000, retries: 1 }); + + test('browser receiver can request and download a generic file above the auto-save cap', async ({ createClient }) => { + const suffix = uniqueName('largefile'); + const serverName = `Large File Server ${suffix}`; + const fileName = `${suffix}-dataset.bin`; + const caption = `Large file upload ${suffix}`; + const alice = await createClient(); + const bob = await createClient(); + const aliceMessages = new ChatMessagesPage(alice.page); + const bobMessages = new ChatMessagesPage(bob.page); + + await test.step('Alice and Bob register and meet in a server', async () => { + const aliceRegister = new RegisterPage(alice.page); + + await aliceRegister.goto(); + await aliceRegister.register(`alice_${suffix}`, 'Alice', 'TestPass123!'); + await expect(alice.page).toHaveURL(/\/dashboard/, { timeout: 15_000 }); + + const bobRegister = new RegisterPage(bob.page); + + await bobRegister.goto(); + await bobRegister.register(`bob_${suffix}`, 'Bob', 'TestPass123!'); + await expect(bob.page).toHaveURL(/\/dashboard/, { timeout: 15_000 }); + + const aliceSearch = new ServerSearchPage(alice.page); + + await aliceSearch.createServer(serverName, { description: 'Large generic file transfer coverage' }); + await expect(alice.page).toHaveURL(/\/room\//, { timeout: 15_000 }); + + const bobSearch = new ServerSearchPage(bob.page); + + await bobSearch.joinServerFromSearch(serverName); + await expect(bob.page).toHaveURL(/\/room\//, { timeout: 15_000 }); + + await aliceMessages.waitForReady(); + await bobMessages.waitForReady(); + }); + + await test.step('Alice sends an 11 MB generic file', async () => { + await attachGeneratedBinaryFile(aliceMessages, fileName, LARGE_FILE_SIZE_BYTES); + await aliceMessages.sendMessage(caption); + await expect(aliceMessages.getMessageItemByText(caption)).toBeVisible({ timeout: 30_000 }); + }); + + const bobBubble = bobMessages.getMessageItemByText(caption); + + await test.step('Bob sees the attachment card with a Request button', async () => { + await expect(bobBubble).toBeVisible({ timeout: 30_000 }); + await expect(bobBubble.getByText(fileName, { exact: false })).toBeVisible({ timeout: 30_000 }); + await expect(bobBubble.getByRole('button', { name: /request/i })).toBeVisible({ timeout: 20_000 }); + }); + + await test.step('Bob requests the file and it downloads to completion', async () => { + await bobBubble.getByRole('button', { name: /request/i }).click(); + + // The transfer must visibly progress (Cancel replaces Request) instead of + // silently stalling at 0 bytes like the original bug. + await expect(bobBubble.getByRole('button', { name: /cancel/i })).toBeVisible({ timeout: 30_000 }); + + await expect(bobBubble.getByRole('button', { name: /download/i })).toBeVisible({ timeout: 300_000 }); + await expect(bobBubble.getByText(/too large/i)).toHaveCount(0); + }); + }); +}); + +/** + * Builds the file inside the page so the multi-megabyte payload never crosses + * the CDP protocol as a base64 string. + */ +async function attachGeneratedBinaryFile( + messages: ChatMessagesPage, + fileName: string, + sizeBytes: number +): Promise { + await messages.waitForReady(); + + await messages.composerInput.evaluate((element, { name, size }) => { + const bytes = new Uint8Array(size); + + for (let index = 0; index < size; index++) { + bytes[index] = (index * 31 + 7) & 0xff; + } + + const dataTransfer = new DataTransfer(); + + dataTransfer.items.add(new File([bytes], name, { type: 'application/octet-stream' })); + element.dispatchEvent(new DragEvent('drop', { + bubbles: true, + cancelable: true, + dataTransfer + })); + }, { name: fileName, size: sizeBytes }); +} + +function uniqueName(prefix: string): string { + return `${prefix}-${Date.now()}-${Math.random().toString(36) + .slice(2, 8)}`; +} diff --git a/toju-app/src/app/domains/attachment/README.md b/toju-app/src/app/domains/attachment/README.md index b1ca74b..873f389 100644 --- a/toju-app/src/app/domains/attachment/README.md +++ b/toju-app/src/app/domains/attachment/README.md @@ -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) diff --git a/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.spec.ts b/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.spec.ts index 99ba722..bbc453f 100644 --- a/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.spec.ts +++ b/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.spec.ts @@ -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); diff --git a/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.ts b/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.ts index eb36e29..fd95c44 100644 --- a/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.ts +++ b/toju-app/src/app/domains/attachment/application/services/attachment-transfer.service.ts @@ -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);