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.

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:<conversationId>`, 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:<conversationId>`, 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.
 

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) {

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);
   });
 });

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;
 }

toju-app/src/app/store/messages/messages-incoming.handlers.spec.ts

@@ -39,13 +39,44 @@ function createContext(overrides: Record<string, unknown> = {}) {
   } 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' }]

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),