Skip to content

fix: wait for remote media that is still being uploaded #410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cardoso wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: wait for remote media that is still being uploaded #410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 126 additions & 0 deletions packages/federation-sdk/src/services/media.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
import { describe, expect, it, mock } from 'bun:test';

import type { ConfigService } from './config.service';
import type { FederationRequestService } from './federation-request.service';
import { MediaService, resolveDownloadTimeoutMs } from './media.service';

const buildService = (requestBinaryData: unknown) =>
new MediaService({} as ConfigService, { requestBinaryData } as unknown as FederationRequestService);

describe('MediaService.downloadFromRemoteServer', () => {
it('asks the origin to wait for an upload that is still being committed', async () => {
const calls: { endpoint: string; queryParams?: Record<string, string> }[] = [];
const requestBinaryData = mock(async (_method: string, _server: string, endpoint: string, queryParams?: Record<string, string>) => {
calls.push({ endpoint, queryParams });
throw new Error('not found');
});

await buildService(requestBinaryData)
.downloadFromRemoteServer('remote.example', 'abc')
.catch(() => undefined);

expect(calls).toHaveLength(3);
for (const call of calls) {
expect(call.queryParams?.timeout_ms).toBe('20000');
}
});

it('does not let the origin fetch the file from a third server on our behalf', async () => {
const calls: { endpoint: string; queryParams?: Record<string, string> }[] = [];
const requestBinaryData = mock(async (_method: string, _server: string, endpoint: string, queryParams?: Record<string, string>) => {
calls.push({ endpoint, queryParams });
throw new Error('not found');
});

await buildService(requestBinaryData)
.downloadFromRemoteServer('remote.example', 'abc')
.catch(() => undefined);

expect(calls[0]?.queryParams?.allow_remote).toBeUndefined();
expect(calls[1]?.queryParams?.allow_remote).toBe('false');
expect(calls[2]?.queryParams?.allow_remote).toBe('false');
});

it('falls back to the default when the configured timeout is malformed', async () => {
const previous = process.env.FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS;
process.env.FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS = '20s';

try {
const calls: (Record<string, string> | undefined)[] = [];
const requestBinaryData = mock(async (_method: string, _server: string, _endpoint: string, queryParams?: Record<string, string>) => {
calls.push(queryParams);
throw new Error('not found');
});

await buildService(requestBinaryData)
.downloadFromRemoteServer('remote.example', 'abc')
.catch(() => undefined);

expect(calls[0]?.timeout_ms).toBe('20000');
} finally {
if (previous === undefined) {
delete process.env.FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS;
} else {
process.env.FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS = previous;
}
}
});

it('honours a valid configured timeout', async () => {
const previous = process.env.FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS;
process.env.FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS = '45000';

try {
const calls: (Record<string, string> | undefined)[] = [];
const requestBinaryData = mock(async (_method: string, _server: string, _endpoint: string, queryParams?: Record<string, string>) => {
calls.push(queryParams);
throw new Error('not found');
});

await buildService(requestBinaryData)
.downloadFromRemoteServer('remote.example', 'abc')
.catch(() => undefined);

expect(calls[0]?.timeout_ms).toBe('45000');
} finally {
if (previous === undefined) {
delete process.env.FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS;
} else {
process.env.FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS = previous;
}
}
});

it('still returns the content from the first endpoint that answers', async () => {
const content = Buffer.from('file-bytes');
const requestBinaryData = mock(async () => ({ content }));

const result = await buildService(requestBinaryData).downloadFromRemoteServer('remote.example', 'abc');

expect(result).toBe(content);
expect(requestBinaryData).toHaveBeenCalledTimes(1);
});
});

describe('resolveDownloadTimeoutMs', () => {
it('defaults to 20s, matching what other homeservers ask for', () => {
expect(resolveDownloadTimeoutMs(undefined)).toBe(20_000);
expect(resolveDownloadTimeoutMs('')).toBe(20_000);
expect(resolveDownloadTimeoutMs(' ')).toBe(20_000);
});

it('caps the wait so a request cannot be held open indefinitely', () => {
expect(resolveDownloadTimeoutMs('120000')).toBe(60_000);
});

it('accepts a valid override', () => {
expect(resolveDownloadTimeoutMs('5000')).toBe(5_000);
expect(resolveDownloadTimeoutMs('0')).toBe(0);
});

it('rejects a malformed override rather than sending a nonsense timeout', () => {
for (const raw of ['20s', '1.5', '-1', 'abc', 'Infinity']) {
expect(() => resolveDownloadTimeoutMs(raw)).toThrow('Invalid FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS value');
}
});
});
59 changes: 53 additions & 6 deletions packages/federation-sdk/src/services/media.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,70 @@ import { singleton } from 'tsyringe';
import { ConfigService } from './config.service';
import { FederationRequestService } from './federation-request.service';

const DEFAULT_DOWNLOAD_TIMEOUT_MS = 20_000;
const MAX_DOWNLOAD_TIMEOUT_MS = 60_000;

@cubic-dev-ai cubic-dev-ai Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS exceeds 20 seconds, MediaService advertises a longer timeout_ms to the origin but cannot wait for it. @rocket.chat/federation-core destroys each request at its hard-coded 20-second timeout, so make that transport timeout configurable or cap this setting.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/services/media.service.ts, line 8:

<comment>When `FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS` exceeds 20 seconds, `MediaService` advertises a longer `timeout_ms` to the origin but cannot wait for it. `@rocket.chat/federation-core` destroys each request at its hard-coded 20-second timeout, so make that transport timeout configurable or cap this setting.</comment>

<file context>
@@ -4,23 +4,70 @@ import { singleton } from 'tsyringe';
 import { FederationRequestService } from './federation-request.service';
 
+const DEFAULT_DOWNLOAD_TIMEOUT_MS = 20_000;
+const MAX_DOWNLOAD_TIMEOUT_MS = 60_000;
+
+export function resolveDownloadTimeoutMs(raw: string | undefined): number {
</file context>


export function resolveDownloadTimeoutMs(raw: string | undefined): number {
if (!raw?.trim()) {
return DEFAULT_DOWNLOAD_TIMEOUT_MS;
}

const value = Number(raw);
if (!Number.isSafeInteger(value) || value < 0) {
throw new Error('Invalid FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS value');
}

return Math.min(value, MAX_DOWNLOAD_TIMEOUT_MS);

@cubic-dev-ai cubic-dev-ai Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: When the configured timeout exceeds MAX_DOWNLOAD_TIMEOUT_MS, Math.min silently caps it to 60s with no log, which is inconsistent with the invalid-value path that logs a warning and falls back. An operator who sets e.g. 120000ms will silently get 60s and may not understand why downloads still time out. Log a warning (or reject) when the value is capped.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/services/media.service.ts, line 20:

<comment>When the configured timeout exceeds MAX_DOWNLOAD_TIMEOUT_MS, `Math.min` silently caps it to 60s with no log, which is inconsistent with the invalid-value path that logs a warning and falls back. An operator who sets e.g. 120000ms will silently get 60s and may not understand why downloads still time out. Log a warning (or reject) when the value is capped.</comment>

<file context>
@@ -4,23 +4,70 @@ import { singleton } from 'tsyringe';
+		throw new Error('Invalid FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS value');
+	}
+
+	return Math.min(value, MAX_DOWNLOAD_TIMEOUT_MS);
+}
+
</file context>

}

@singleton()
export class MediaService {
private readonly logger = createLogger('MediaService');

private downloadTimeoutMs?: number;

constructor(private readonly configService: ConfigService, private readonly federationRequest: FederationRequestService) {}

private get timeoutMs(): number {
if (this.downloadTimeoutMs === undefined) {
try {
this.downloadTimeoutMs = resolveDownloadTimeoutMs(process.env.FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS);
} catch (err) {
this.downloadTimeoutMs = DEFAULT_DOWNLOAD_TIMEOUT_MS;
this.logger.warn({
msg: 'Ignoring invalid FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS, using the default',
value: process.env.FEDERATION_MEDIA_DOWNLOAD_TIMEOUT_MS,
defaultMs: DEFAULT_DOWNLOAD_TIMEOUT_MS,
err,
});
}
}

return this.downloadTimeoutMs;
}

async downloadFromRemoteServer(serverName: string, mediaId: string): Promise<Buffer | null> {
const endpoints = [
`/_matrix/federation/v1/media/download/${mediaId}`,
`/_matrix/media/v3/download/${serverName}/${mediaId}`,
`/_matrix/media/r0/download/${serverName}/${mediaId}`,
const timeoutMs = String(this.timeoutMs);

const endpoints: { path: string; queryParams: Record<string, string> }[] = [
{
path: `/_matrix/federation/v1/media/download/${mediaId}`,
queryParams: { timeout_ms: timeoutMs },
},
{
path: `/_matrix/media/v3/download/${serverName}/${mediaId}`,
queryParams: { allow_remote: 'false', timeout_ms: timeoutMs },
},
{
path: `/_matrix/media/r0/download/${serverName}/${mediaId}`,
queryParams: { allow_remote: 'false', timeout_ms: timeoutMs },
},
];

for await (const endpoint of endpoints) {
for await (const { path: endpoint, queryParams } of endpoints) {
try {
// TODO: Stream remote file downloads instead of buffering the entire file in memory.
const response = await this.federationRequest.requestBinaryData('GET', serverName, endpoint);
const response = await this.federationRequest.requestBinaryData('GET', serverName, endpoint, queryParams);

return response.content;
} catch (err) {
Expand Down
Loading