From 497033aff0a188d04092d19d7aa68f128bbbeed0 Mon Sep 17 00:00:00 2001 From: Myx Date: Sun, 12 Jul 2026 21:26:59 +0200 Subject: [PATCH] fix: Bug - Sending files and attachment issues (cross-transport auto-download race) Re-queue attachment auto-downloads when the chat message arrives, since file-announce (WebRTC) can beat chat-message (websocket) and the announce-time pass gives up on an unknown room. Gate stalled-download resets on chunk-progress staleness so an active transfer is never cancelled mid-stream, which deadlocked the retry against the sender's active-transfer dedupe. Co-authored-by: Cursor --- agents-docs/LESSONS.md | 7 +++ toju-app/src/app/domains/attachment/README.md | 4 +- .../services/attachment-manager.service.ts | 3 +- .../attachment-autodownload.rules.spec.ts | 46 ++++++++++++++++--- .../logic/attachment-autodownload.rules.ts | 35 ++++++++++++-- .../messages-incoming.handlers.spec.ts | 35 +++++++++++++- .../messages/messages-incoming.handlers.ts | 4 ++ 7 files changed, 118 insertions(+), 16 deletions(-) diff --git a/agents-docs/LESSONS.md b/agents-docs/LESSONS.md index 940b36e..55238b7 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 +### 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. +- **Rule:** events that complete the `messageId -> roomId` binding (`chat-message` in `messages-incoming.handlers.ts`) must call `queueAutoDownloadsForMessage` again — never assume `file-announce` arrives after the message, they ride different transports; and stall detection must gate on chunk-progress staleness (`lastUpdateMs` older than `ATTACHMENT_STALLED_DOWNLOAD_THRESHOLD_MS`), never on the absence of a pending-request marker alone. +- **Why:** both halves fail silently (no error, no requestError set), so the UI just sits at 0 bytes; the flake is timing-dependent and invisible in single-client tests — only the two-client e2e with `--repeat-each` exposed it deterministically enough to fix. +- **Example:** `handleChatMessage` now calls `attachments.queueAutoDownloadsForMessage(message.id)` after `rememberMessageRoom`; `shouldResetStalledAttachmentDownload(attachment, hasPendingRequest, nowMs)` in `attachment-autodownload.rules.ts`. Verified with `npx playwright test -g "syncs image and file attachments|syncs multi-chunk" --repeat-each=4` (8/8 after, ~50% before). + ### Scope per-user UI state by user id, not by the client database [persistence] [multi-user] [custom-emoji] - **Trigger:** custom emoji "saved library" membership was a single `savedByUser` flag on the shared emoji row plus a long-lived singleton (`CustomEmojiService`) that merged state across logins — so a second account on the same client (and the Electron shared SQLite DB) inherited the first user's picker. diff --git a/toju-app/src/app/domains/attachment/README.md b/toju-app/src/app/domains/attachment/README.md index 949a8ae..b1ca74b 100644 --- a/toju-app/src/app/domains/attachment/README.md +++ b/toju-app/src/app/domains/attachment/README.md @@ -144,7 +144,9 @@ When the user navigates to a room, the manager watches the route and decides whi The decision lives in `shouldAutoRequestWhenWatched()` which calls `isAttachmentMedia()` and checks against `MAX_AUTO_SAVE_SIZE_BYTES`. -Direct-message routes (`/dm/:conversationId` and `/pm/:conversationId`) are treated as watched attachment containers named `direct-message:`, so image/video metadata announced for the visible conversation is eligible for the same automatic request path as server-room media. Auto-download work fans out with bounded concurrency (`ATTACHMENT_AUTO_DOWNLOAD_CONCURRENCY`, default 3 files at a time per watched room) so multiple pending files can progress in parallel without removing the per-file chunk-ack memory safety invariant. Stalled partial downloads with no active pending request are reset automatically before the next auto-download pass. +Direct-message routes (`/dm/:conversationId` and `/pm/:conversationId`) are treated as watched attachment containers named `direct-message:`, so image/video metadata announced for the visible conversation is eligible for the same automatic request path as server-room media. Auto-download work fans out with bounded concurrency (`ATTACHMENT_AUTO_DOWNLOAD_CONCURRENCY`, default 3 files at a time per watched room) so multiple pending files can progress in parallel without removing the per-file chunk-ack memory safety invariant. Stalled partial downloads are reset automatically before the next auto-download pass — but only when chunk progress has been quiet past `ATTACHMENT_STALLED_DOWNLOAD_THRESHOLD_MS` (`attachment-autodownload.rules.ts`). The pending-request marker is deleted on the first received chunk, so "no pending request" alone must never classify an in-flight transfer as stalled; resetting an active transfer cancels it on the sender and the retry deadlocks against the sender's per-peer active-transfer dedupe. + +Auto-download triggers must fire from *every* event that can complete the `messageId -> roomId` binding, because `file-announce` (WebRTC data channel) and `chat-message` (signaling websocket) travel on different transports and arrive in either order. When the announce arrives first, the room is still unknown and that pass gives up; the `chat-message` handler (`messages-incoming.handlers.ts`) therefore calls `rememberMessageRoom` and re-queues `queueAutoDownloadsForMessage` for the arriving message. Incoming and synced attachment metadata is normalized through `attachment-normalize.rules.ts` / `attachment-mime.rules.ts`: generic `application/octet-stream` (or empty) MIME types are inferred from the filename extension so images still group into galleries and small audio/video render as players instead of generic file cards. Display hydration (`needsAttachmentDisplayHydration`) rehydrates blob/file URLs from disk for both inline images and playable media without forcing a fresh peer download when local bytes already exist. diff --git a/toju-app/src/app/domains/attachment/application/services/attachment-manager.service.ts b/toju-app/src/app/domains/attachment/application/services/attachment-manager.service.ts index 278a4dc..8bf4c9d 100644 --- a/toju-app/src/app/domains/attachment/application/services/attachment-manager.service.ts +++ b/toju-app/src/app/domains/attachment/application/services/attachment-manager.service.ts @@ -382,7 +382,8 @@ export class AttachmentManagerService { if (shouldResetStalledAttachmentDownload( attachment, - this.transfer.hasPendingRequest(messageId, attachment.id) + this.transfer.hasPendingRequest(messageId, attachment.id), + Date.now() )) { this.transfer.cancelRequest(messageId, attachment); } else if ((attachment.receivedBytes ?? 0) > 0) { diff --git a/toju-app/src/app/domains/attachment/domain/logic/attachment-autodownload.rules.spec.ts b/toju-app/src/app/domains/attachment/domain/logic/attachment-autodownload.rules.spec.ts index 1d071eb..975c3ae 100644 --- a/toju-app/src/app/domains/attachment/domain/logic/attachment-autodownload.rules.spec.ts +++ b/toju-app/src/app/domains/attachment/domain/logic/attachment-autodownload.rules.spec.ts @@ -1,20 +1,52 @@ -import { shouldResetStalledAttachmentDownload } from './attachment-autodownload.rules'; +import { ATTACHMENT_STALLED_DOWNLOAD_THRESHOLD_MS, shouldResetStalledAttachmentDownload } from './attachment-autodownload.rules'; + +const NOW_MS = 1_750_000_000_000; describe('attachment autodownload rules', () => { - it('resets stalled partial downloads that are no longer actively transferring', () => { + it('does not reset an actively transferring download with recent chunk progress', () => { + // Regression: the pending-request marker is deleted on the first received + // chunk, so an in-flight multi-chunk transfer has receivedBytes > 0 and no + // pending request - it must NOT be treated as stalled while chunks flow. expect(shouldResetStalledAttachmentDownload({ available: false, - receivedBytes: 128 - }, false)).toBe(true); + receivedBytes: 128, + lastUpdateMs: NOW_MS - 100 + }, false, NOW_MS)).toBe(false); + }); + it('resets partial downloads with no progress past the stall threshold', () => { + expect(shouldResetStalledAttachmentDownload({ + available: false, + receivedBytes: 128, + lastUpdateMs: NOW_MS - ATTACHMENT_STALLED_DOWNLOAD_THRESHOLD_MS - 1 + }, false, NOW_MS)).toBe(true); + }); + + it('treats partial downloads without a progress timestamp as stalled', () => { expect(shouldResetStalledAttachmentDownload({ available: false, receivedBytes: 128 - }, true)).toBe(false); + }, false, NOW_MS)).toBe(true); + }); + + it('never resets downloads with a pending request or already available', () => { + expect(shouldResetStalledAttachmentDownload({ + available: false, + receivedBytes: 128, + lastUpdateMs: NOW_MS - ATTACHMENT_STALLED_DOWNLOAD_THRESHOLD_MS - 1 + }, true, NOW_MS)).toBe(false); expect(shouldResetStalledAttachmentDownload({ available: true, - receivedBytes: 128 - }, false)).toBe(false); + receivedBytes: 128, + lastUpdateMs: NOW_MS - ATTACHMENT_STALLED_DOWNLOAD_THRESHOLD_MS - 1 + }, false, NOW_MS)).toBe(false); + }); + + it('never resets downloads that have not received any bytes', () => { + expect(shouldResetStalledAttachmentDownload({ + available: false, + receivedBytes: 0 + }, false, NOW_MS)).toBe(false); }); }); diff --git a/toju-app/src/app/domains/attachment/domain/logic/attachment-autodownload.rules.ts b/toju-app/src/app/domains/attachment/domain/logic/attachment-autodownload.rules.ts index ca8784a..7bbd2f3 100644 --- a/toju-app/src/app/domains/attachment/domain/logic/attachment-autodownload.rules.ts +++ b/toju-app/src/app/domains/attachment/domain/logic/attachment-autodownload.rules.ts @@ -1,8 +1,33 @@ +/** + * How long a partial download may go without chunk progress before an + * auto-download pass treats it as stalled and resets it for a retry. + */ +export const ATTACHMENT_STALLED_DOWNLOAD_THRESHOLD_MS = 15_000; + +/** + * The pending-request marker is deleted as soon as the first chunk arrives, so + * "no pending request" does NOT mean "not transferring". An in-flight transfer + * is recognized by recent chunk progress (`lastUpdateMs`); only partials with + * no progress past the stall threshold (or with no progress timestamp at all, + * e.g. leftovers from a previous session) may be reset - resetting an active + * transfer cancels it on the sender and deadlocks the retry against the + * sender's active-transfer dedupe. + */ export function shouldResetStalledAttachmentDownload( - attachment: Pick<{ available?: boolean; receivedBytes?: number }, 'available' | 'receivedBytes'>, - hasPendingRequest: boolean + attachment: Pick< + { available?: boolean; lastUpdateMs?: number; receivedBytes?: number }, + 'available' | 'lastUpdateMs' | 'receivedBytes' + >, + hasPendingRequest: boolean, + nowMs: number ): boolean { - return !attachment.available && - (attachment.receivedBytes ?? 0) > 0 && - !hasPendingRequest; + if (attachment.available || hasPendingRequest || (attachment.receivedBytes ?? 0) === 0) { + return false; + } + + if (!attachment.lastUpdateMs) { + return true; + } + + return nowMs - attachment.lastUpdateMs > ATTACHMENT_STALLED_DOWNLOAD_THRESHOLD_MS; } diff --git a/toju-app/src/app/store/messages/messages-incoming.handlers.spec.ts b/toju-app/src/app/store/messages/messages-incoming.handlers.spec.ts index e835cd2..583e1fd 100644 --- a/toju-app/src/app/store/messages/messages-incoming.handlers.spec.ts +++ b/toju-app/src/app/store/messages/messages-incoming.handlers.spec.ts @@ -39,13 +39,44 @@ function createContext(overrides: Record = {}) { } as const; } +describe('dispatchIncomingMessage attachment auto-download race', () => { + it('queues attachment auto-downloads when the chat message arrives after its file-announce', async () => { + // file-announce can reach a receiver before the chat-message itself; the + // announce-time auto-download then fails to resolve the room and gives up, + // so the arriving message must re-queue the download. + const saveMessage = vi.fn(async () => undefined); + const rememberMessageRoom = vi.fn(); + const queueAutoDownloadsForMessage = vi.fn(); + const context = createContext({ + db: { saveMessage }, + attachments: { rememberMessageRoom, queueAutoDownloadsForMessage }, + currentUser: { id: 'user-2', oderId: 'user-2' }, + currentRoom: { id: 'room-a' }, + savedRooms: [{ id: 'room-a' }] + }); + const action = await firstValueFrom( + dispatchIncomingMessage( + { + type: 'chat-message', + message: createMessage({ id: 'message-with-attachment', roomId: 'room-a' }) + } as never, + context as never + ).pipe(defaultIfEmpty(null)) + ); + + expect(action).not.toBeNull(); + expect(rememberMessageRoom).toHaveBeenCalledWith('message-with-attachment', 'room-a'); + expect(queueAutoDownloadsForMessage).toHaveBeenCalledWith('message-with-attachment'); + }); +}); + describe('dispatchIncomingMessage multi-device sync', () => { it('accepts own messages that originated on another client instance', async () => { const saveMessage = vi.fn(async () => undefined); const rememberMessageRoom = vi.fn(); const context = createContext({ db: { saveMessage }, - attachments: { rememberMessageRoom }, + attachments: { rememberMessageRoom, queueAutoDownloadsForMessage: vi.fn() }, currentUser: { id: 'user-1', oderId: 'user-1' }, currentRoom: { id: 'room-a' }, savedRooms: [{ id: 'room-a' }], @@ -74,7 +105,7 @@ describe('dispatchIncomingMessage multi-device sync', () => { const rememberMessageRoom = vi.fn(); const context = createContext({ db: { saveMessage }, - attachments: { rememberMessageRoom }, + attachments: { rememberMessageRoom, queueAutoDownloadsForMessage: vi.fn() }, currentUser: { id: 'viewer-home', oderId: 'viewer-home' }, currentRoom: { id: 'room-a' }, savedRooms: [{ id: 'room-a' }] diff --git a/toju-app/src/app/store/messages/messages-incoming.handlers.ts b/toju-app/src/app/store/messages/messages-incoming.handlers.ts index 9845b56..c7f7651 100644 --- a/toju-app/src/app/store/messages/messages-incoming.handlers.ts +++ b/toju-app/src/app/store/messages/messages-incoming.handlers.ts @@ -382,6 +382,10 @@ function handleChatMessage( return EMPTY; attachments.rememberMessageRoom(normalizedMessage.id, normalizedMessage.roomId); + // A file-announce for this message may have arrived before the message itself; + // that announce-time auto-download could not resolve the room and gave up, so + // re-queue now that the message-to-room binding is known. + attachments.queueAutoDownloadsForMessage(normalizedMessage.id); trackBackgroundOperation( db.saveMessage(normalizedMessage),