fix: Bug - Sending files between users doesn't really work
Stream oversized generic attachments to disk instead of silently dropping chunks, avoid loading completed file downloads into renderer memory, and surface a clear error when the browser client cannot receive a file. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -8,7 +8,8 @@
|
|||||||
"chunksOutOfOrder": "Received media chunks out of order. Retry the download.",
|
"chunksOutOfOrder": "Received media chunks out of order. Retry the download.",
|
||||||
"writeDownloadFailed": "Could not write media download to disk.",
|
"writeDownloadFailed": "Could not write media download to disk.",
|
||||||
"openDownloadFailed": "Could not open completed media download from disk.",
|
"openDownloadFailed": "Could not open completed media download from disk.",
|
||||||
"downloadFailed": "Media download failed. Retry the download."
|
"downloadFailed": "Media download failed. Retry the download.",
|
||||||
|
"fileTooLarge": "This file is too large to download in this client. Use the desktop app or ask the sender to share a smaller file."
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -36,7 +36,8 @@
|
|||||||
"chunksOutOfOrder": "Received media chunks out of order. Retry the download.",
|
"chunksOutOfOrder": "Received media chunks out of order. Retry the download.",
|
||||||
"writeDownloadFailed": "Could not write media download to disk.",
|
"writeDownloadFailed": "Could not write media download to disk.",
|
||||||
"openDownloadFailed": "Could not open completed media download from disk.",
|
"openDownloadFailed": "Could not open completed media download from disk.",
|
||||||
"downloadFailed": "Media download failed. Retry the download."
|
"downloadFailed": "Media download failed. Retry the download.",
|
||||||
|
"fileTooLarge": "This file is too large to download in this client. Use the desktop app or ask the sender to share a smaller file."
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
"auth": {
|
"auth": {
|
||||||
|
|||||||
@@ -364,6 +364,23 @@ describe('AttachmentTransferService', () => {
|
|||||||
return attachment;
|
return attachment;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function registerIncomingGenericFile(size: number): Attachment {
|
||||||
|
const attachment: Attachment = {
|
||||||
|
id: FILE_ID,
|
||||||
|
messageId: MESSAGE_ID,
|
||||||
|
filename: 'archive.zip',
|
||||||
|
size,
|
||||||
|
mime: 'application/zip',
|
||||||
|
isImage: false,
|
||||||
|
uploaderPeerId: PEER_ID,
|
||||||
|
available: false,
|
||||||
|
receivedBytes: 0
|
||||||
|
};
|
||||||
|
|
||||||
|
runtimeStore.setAttachmentsForMessage(MESSAGE_ID, [attachment]);
|
||||||
|
return attachment;
|
||||||
|
}
|
||||||
|
|
||||||
it('streams playable media to disk when the store supports streaming', async () => {
|
it('streams playable media to disk when the store supports streaming', async () => {
|
||||||
attachmentStorage.canStreamToDisk.mockReturnValue(true);
|
attachmentStorage.canStreamToDisk.mockReturnValue(true);
|
||||||
|
|
||||||
@@ -409,4 +426,57 @@ describe('AttachmentTransferService', () => {
|
|||||||
|
|
||||||
expect(service.hasPendingRequest(MESSAGE_ID, FILE_ID)).toBe(true);
|
expect(service.hasPendingRequest(MESSAGE_ID, FILE_ID)).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('streams oversized generic files to disk when the store supports streaming', async () => {
|
||||||
|
attachmentStorage.canStreamToDisk.mockReturnValue(true);
|
||||||
|
attachmentStorage.canPersistSize.mockImplementation((bytes: number) => bytes <= 256 * 1024 * 1024);
|
||||||
|
|
||||||
|
const service = createService();
|
||||||
|
const attachment = registerIncomingGenericFile(12 * 1024 * 1024);
|
||||||
|
|
||||||
|
service.handleFileChunk(chunkPayload(0, 1, [
|
||||||
|
1,
|
||||||
|
2,
|
||||||
|
3
|
||||||
|
]));
|
||||||
|
|
||||||
|
await vi.waitFor(() => expect(attachment.available).toBe(true));
|
||||||
|
|
||||||
|
expect(attachmentStorage.createWritableFile).toHaveBeenCalled();
|
||||||
|
expect(attachmentStorage.appendBase64).toHaveBeenCalled();
|
||||||
|
expect(persistence.ensureInlineDisplayObjectUrl).not.toHaveBeenCalled();
|
||||||
|
expect(persistence.saveFileToDisk).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('rejects oversized browser downloads before requesting peers', async () => {
|
||||||
|
attachmentStorage.canStreamToDisk.mockReturnValue(false);
|
||||||
|
attachmentStorage.canPersistSize.mockImplementation((bytes: number) => bytes <= 50 * 1024 * 1024);
|
||||||
|
|
||||||
|
const service = createService();
|
||||||
|
const attachment = registerIncomingGenericFile(200 * 1024 * 1024);
|
||||||
|
|
||||||
|
await service.requestFromAnyPeer(MESSAGE_ID, attachment);
|
||||||
|
|
||||||
|
expect(attachment.requestError).toBe('attachment.errors.fileTooLarge');
|
||||||
|
expect(webrtc.sendToPeer).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
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);
|
||||||
|
|
||||||
|
const service = createService();
|
||||||
|
const attachment = registerIncomingGenericFile(3);
|
||||||
|
|
||||||
|
service.handleFileChunk(chunkPayload(0, 1, [
|
||||||
|
1,
|
||||||
|
2,
|
||||||
|
3
|
||||||
|
]));
|
||||||
|
|
||||||
|
await vi.waitFor(() => expect(attachment.available).toBe(true));
|
||||||
|
|
||||||
|
expect(attachmentStorage.appendBase64).not.toHaveBeenCalled();
|
||||||
|
expect(persistence.saveFileToDisk).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -9,13 +9,20 @@ import { AttachmentStorageService } from '../../infrastructure/services/attachme
|
|||||||
import { MAX_AUTO_SAVE_SIZE_BYTES } from '../../domain/constants/attachment.constants';
|
import { MAX_AUTO_SAVE_SIZE_BYTES } from '../../domain/constants/attachment.constants';
|
||||||
import { isImageAttachment, resolvePublishAttachmentIsImage } from '../../domain/logic/attachment-image.rules';
|
import { isImageAttachment, resolvePublishAttachmentIsImage } from '../../domain/logic/attachment-image.rules';
|
||||||
import { isSharingFromThisDevice } from '../../domain/logic/attachment-sharing.rules';
|
import { isSharingFromThisDevice } from '../../domain/logic/attachment-sharing.rules';
|
||||||
import { shouldCopyUploaderMediaToAppData, shouldPersistDownloadedAttachment } from '../../domain/logic/attachment.logic';
|
import {
|
||||||
|
canReceiveAttachment,
|
||||||
|
isAttachmentMedia,
|
||||||
|
shouldCopyUploaderMediaToAppData,
|
||||||
|
shouldPersistDownloadedAttachment,
|
||||||
|
shouldStreamAttachmentReceiveToDisk
|
||||||
|
} from '../../domain/logic/attachment.logic';
|
||||||
import type { Attachment, AttachmentMeta } from '../../domain/models/attachment.model';
|
import type { Attachment, AttachmentMeta } from '../../domain/models/attachment.model';
|
||||||
import {
|
import {
|
||||||
ATTACHMENT_TRANSFER_EWMA_CURRENT_WEIGHT,
|
ATTACHMENT_TRANSFER_EWMA_CURRENT_WEIGHT,
|
||||||
ATTACHMENT_TRANSFER_EWMA_PREVIOUS_WEIGHT,
|
ATTACHMENT_TRANSFER_EWMA_PREVIOUS_WEIGHT,
|
||||||
DEFAULT_ATTACHMENT_MIME_TYPE,
|
DEFAULT_ATTACHMENT_MIME_TYPE,
|
||||||
ATTACHMENT_DOWNLOAD_FAILED_KEY,
|
ATTACHMENT_DOWNLOAD_FAILED_KEY,
|
||||||
|
ATTACHMENT_FILE_TOO_LARGE_KEY,
|
||||||
ATTACHMENT_CHUNKS_OUT_OF_ORDER_KEY,
|
ATTACHMENT_CHUNKS_OUT_OF_ORDER_KEY,
|
||||||
ATTACHMENT_OPEN_DOWNLOAD_FAILED_KEY,
|
ATTACHMENT_OPEN_DOWNLOAD_FAILED_KEY,
|
||||||
ATTACHMENT_PREPARE_DOWNLOAD_FAILED_KEY,
|
ATTACHMENT_PREPARE_DOWNLOAD_FAILED_KEY,
|
||||||
@@ -188,6 +195,13 @@ export class AttachmentTransferService {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (!canReceiveAttachment(attachment, this.receiveCapabilities())) {
|
||||||
|
this.runtimeStore.deletePendingRequest(requestKey);
|
||||||
|
attachment.requestError = this.appI18n.instant(ATTACHMENT_FILE_TOO_LARGE_KEY);
|
||||||
|
this.runtimeStore.touch();
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
if (clearedRequestError)
|
if (clearedRequestError)
|
||||||
this.runtimeStore.touch();
|
this.runtimeStore.touch();
|
||||||
|
|
||||||
@@ -344,7 +358,9 @@ export class AttachmentTransferService {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!this.shouldReceiveToDisk(attachment) && attachment.size > MAX_AUTO_SAVE_SIZE_BYTES) {
|
if (!canReceiveAttachment(attachment, this.receiveCapabilities())) {
|
||||||
|
attachment.requestError = this.appI18n.instant(ATTACHMENT_FILE_TOO_LARGE_KEY);
|
||||||
|
this.runtimeStore.touch();
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -784,10 +800,14 @@ export class AttachmentTransferService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private shouldReceiveToDisk(attachment: Attachment): boolean {
|
private shouldReceiveToDisk(attachment: Attachment): boolean {
|
||||||
return this.isPlayableMedia(attachment) &&
|
return shouldStreamAttachmentReceiveToDisk(attachment, this.receiveCapabilities());
|
||||||
!attachment.filePath &&
|
}
|
||||||
this.attachmentStorage.canStreamToDisk() &&
|
|
||||||
this.attachmentStorage.canPersistSize(attachment.size);
|
private receiveCapabilities() {
|
||||||
|
return {
|
||||||
|
canStreamToDisk: this.attachmentStorage.canStreamToDisk(),
|
||||||
|
canPersistSize: (bytes: number) => this.attachmentStorage.canPersistSize(bytes)
|
||||||
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
private enqueueDiskFileChunk(
|
private enqueueDiskFileChunk(
|
||||||
@@ -851,6 +871,14 @@ export class AttachmentTransferService {
|
|||||||
|
|
||||||
attachment.savedPath = assembly.path;
|
attachment.savedPath = assembly.path;
|
||||||
|
|
||||||
|
if (!isAttachmentMedia(attachment)) {
|
||||||
|
attachment.available = true;
|
||||||
|
this.diskReceiveAssemblies.delete(assemblyKey);
|
||||||
|
this.runtimeStore.touch();
|
||||||
|
void this.persistence.persistAttachmentMeta(attachment);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
const restoredForDisplay = await this.persistence.ensureInlineDisplayObjectUrl(attachment);
|
const restoredForDisplay = await this.persistence.ensureInlineDisplayObjectUrl(attachment);
|
||||||
|
|
||||||
if (!restoredForDisplay) {
|
if (!restoredForDisplay) {
|
||||||
|
|||||||
@@ -22,3 +22,4 @@ export const ATTACHMENT_CHUNKS_OUT_OF_ORDER_KEY = 'attachment.errors.chunksOutOf
|
|||||||
export const ATTACHMENT_WRITE_DOWNLOAD_FAILED_KEY = 'attachment.errors.writeDownloadFailed';
|
export const ATTACHMENT_WRITE_DOWNLOAD_FAILED_KEY = 'attachment.errors.writeDownloadFailed';
|
||||||
export const ATTACHMENT_OPEN_DOWNLOAD_FAILED_KEY = 'attachment.errors.openDownloadFailed';
|
export const ATTACHMENT_OPEN_DOWNLOAD_FAILED_KEY = 'attachment.errors.openDownloadFailed';
|
||||||
export const ATTACHMENT_DOWNLOAD_FAILED_KEY = 'attachment.errors.downloadFailed';
|
export const ATTACHMENT_DOWNLOAD_FAILED_KEY = 'attachment.errors.downloadFailed';
|
||||||
|
export const ATTACHMENT_FILE_TOO_LARGE_KEY = 'attachment.errors.fileTooLarge';
|
||||||
|
|||||||
@@ -1,7 +1,9 @@
|
|||||||
import {
|
import {
|
||||||
getWatchedAttachmentRoomIdFromUrl,
|
getWatchedAttachmentRoomIdFromUrl,
|
||||||
isDirectMessageAttachmentRoomId,
|
isDirectMessageAttachmentRoomId,
|
||||||
shouldCopyUploaderMediaToAppData
|
shouldCopyUploaderMediaToAppData,
|
||||||
|
shouldStreamAttachmentReceiveToDisk,
|
||||||
|
canReceiveAttachment
|
||||||
} from './attachment.logic';
|
} from './attachment.logic';
|
||||||
|
|
||||||
describe('attachment logic', () => {
|
describe('attachment logic', () => {
|
||||||
@@ -44,4 +46,36 @@ describe('attachment logic', () => {
|
|||||||
mime: 'video/mp4'
|
mime: 'video/mp4'
|
||||||
}, undefined, true)).toBe(false);
|
}, undefined, true)).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('streams oversized generic files to disk when the store supports it', () => {
|
||||||
|
const capabilities = {
|
||||||
|
canStreamToDisk: true,
|
||||||
|
canPersistSize: (bytes: number) => bytes <= 256 * 1024 * 1024
|
||||||
|
};
|
||||||
|
|
||||||
|
expect(shouldStreamAttachmentReceiveToDisk({
|
||||||
|
size: 200 * 1024 * 1024,
|
||||||
|
mime: 'application/zip',
|
||||||
|
filePath: undefined
|
||||||
|
}, capabilities)).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('receives browser-sized files in memory when disk streaming is unavailable', () => {
|
||||||
|
const browserCapabilities = {
|
||||||
|
canStreamToDisk: false,
|
||||||
|
canPersistSize: (bytes: number) => bytes <= 50 * 1024 * 1024
|
||||||
|
};
|
||||||
|
|
||||||
|
expect(canReceiveAttachment({
|
||||||
|
size: 20 * 1024 * 1024,
|
||||||
|
mime: 'application/zip',
|
||||||
|
filePath: undefined
|
||||||
|
}, browserCapabilities)).toBe(true);
|
||||||
|
|
||||||
|
expect(canReceiveAttachment({
|
||||||
|
size: 200 * 1024 * 1024,
|
||||||
|
mime: 'application/zip',
|
||||||
|
filePath: undefined
|
||||||
|
}, browserCapabilities)).toBe(false);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -50,6 +50,49 @@ export function isDirectMessageAttachmentRoomId(roomId: string | null | undefine
|
|||||||
return !!roomId && roomId.startsWith(DIRECT_MESSAGE_ATTACHMENT_STORAGE_PREFIX);
|
return !!roomId && roomId.startsWith(DIRECT_MESSAGE_ATTACHMENT_STORAGE_PREFIX);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export interface AttachmentReceiveCapabilities {
|
||||||
|
canStreamToDisk: boolean;
|
||||||
|
canPersistSize: (bytes: number) => boolean;
|
||||||
|
}
|
||||||
|
|
||||||
|
export function shouldStreamAttachmentReceiveToDisk(
|
||||||
|
attachment: Pick<Attachment, 'size' | 'mime' | 'filePath'>,
|
||||||
|
capabilities: AttachmentReceiveCapabilities
|
||||||
|
): boolean {
|
||||||
|
if (attachment.filePath?.trim()) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!capabilities.canStreamToDisk || !capabilities.canPersistSize(attachment.size)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (attachment.size > MAX_AUTO_SAVE_SIZE_BYTES) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
return isAttachmentMedia(attachment);
|
||||||
|
}
|
||||||
|
|
||||||
|
export function canReceiveAttachmentInMemory(
|
||||||
|
attachment: Pick<Attachment, 'size'>,
|
||||||
|
capabilities: AttachmentReceiveCapabilities
|
||||||
|
): boolean {
|
||||||
|
if (attachment.size <= MAX_AUTO_SAVE_SIZE_BYTES) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
return !capabilities.canStreamToDisk && capabilities.canPersistSize(attachment.size);
|
||||||
|
}
|
||||||
|
|
||||||
|
export function canReceiveAttachment(
|
||||||
|
attachment: Pick<Attachment, 'size' | 'mime' | 'filePath'>,
|
||||||
|
capabilities: AttachmentReceiveCapabilities
|
||||||
|
): boolean {
|
||||||
|
return shouldStreamAttachmentReceiveToDisk(attachment, capabilities)
|
||||||
|
|| canReceiveAttachmentInMemory(attachment, capabilities);
|
||||||
|
}
|
||||||
|
|
||||||
function decodeUrlSegment(value: string): string {
|
function decodeUrlSegment(value: string): string {
|
||||||
try {
|
try {
|
||||||
return decodeURIComponent(value);
|
return decodeURIComponent(value);
|
||||||
|
|||||||
Reference in New Issue
Block a user