diff --git a/src/app/utils/matrix.ts b/src/app/utils/matrix.ts index ce1497309..1471325fd 100644 --- a/src/app/utils/matrix.ts +++ b/src/app/utils/matrix.ts @@ -171,12 +171,14 @@ const matrixErrorFromUnknown = (e: unknown): MatrixError => { // HTTP statuses that should not be retried — client errors are deterministic // (e.g. 413 payload too large, 400 bad request, 401/403 auth) and won't succeed on retry. -const isRetryableUploadError = (e: unknown): boolean => { - // A user-cancelled / aborted upload must never be retried. matrix-js-sdk's - // mx.cancelUpload() rejects the upload with a DOMException named "AbortError"; - // without this guard the retry loop would resurrect an upload the user just - // cancelled. - if ((e as { name?: unknown } | null | undefined)?.name === 'AbortError') return false; +const isRetryableUploadError = (e: unknown, cancelled: boolean): boolean => { + // A user-cancelled upload must never be retried. BUT matrix-js-sdk rejects + // with a DOMException named "AbortError" for BOTH mx.cancelUpload() and any + // XHR that ends with status 0 — a dropped connection, going offline, DNS — + // ("mimic fetch API", http-api/index.ts). Those are exactly the transient + // failures this retry loop exists for, so decide by OUR cancel signal, not + // by the error's name (Gitea #172). + if ((e as { name?: unknown } | null | undefined)?.name === 'AbortError') return !cancelled; if (e instanceof MatrixError) { const status = e.httpStatus; // No status => network/transport failure (transient): retry. @@ -249,7 +251,7 @@ export const uploadContent = async ( } catch (e: unknown) { lastError = matrixErrorFromUnknown(e); - if (retryCount === UPLOAD_MAX_RETRY_COUNT || !isRetryableUploadError(e)) { + if (retryCount === UPLOAD_MAX_RETRY_COUNT || !isRetryableUploadError(e, !!signal?.aborted)) { onError(lastError); return; } diff --git a/src/app/utils/uploadRetry.test.ts b/src/app/utils/uploadRetry.test.ts new file mode 100644 index 000000000..689764aa1 --- /dev/null +++ b/src/app/utils/uploadRetry.test.ts @@ -0,0 +1,59 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { MatrixClient, MatrixError } from 'matrix-js-sdk'; +import { uploadContent } from './matrix'; + +// Drives `uploadContent` with a fake client whose uploads fail per `plan` +// (one entry per attempt: an Error to reject with, or 'ok'). Back-off sleeps +// are real timers, so failures are kept to 1-2 retries per test. +const run = async (plan: Array, cancelAfterAttempt?: number) => { + let attempt = 0; + const controller = new AbortController(); + const mx = { + uploadContent: () => { + attempt += 1; + const step = plan[attempt - 1]; + if (cancelAfterAttempt === attempt) controller.abort(); + return step === 'ok' ? Promise.resolve({ content_uri: 'mxc://x/y' }) : Promise.reject(step); + }, + } as unknown as MatrixClient; + let outcome: { mxc?: string; error?: MatrixError } = {}; + await uploadContent(mx, new Blob(['x']) as unknown as File, { + onProgress: () => undefined, + onSuccess: (mxc) => { + outcome = { mxc }; + }, + onError: (error) => { + outcome = { error }; + }, + signal: controller.signal, + }); + return { attempts: attempt, ...outcome }; +}; + +const abortError = () => new DOMException('request failed', 'AbortError'); + +test('a dropped connection (SDK reports it as AbortError) is retried', async () => { + const r = await run([abortError(), 'ok']); + assert.equal(r.attempts, 2); + assert.equal(r.mxc, 'mxc://x/y'); +}); + +test('a user cancel (our signal aborted) is NOT retried even though it is also an AbortError', async () => { + const r = await run([abortError(), 'ok'], 1); + assert.equal(r.attempts, 1); + assert.equal(r.mxc, undefined); + assert.ok(r.error); +}); + +test('5xx is retried, 4xx fails fast', async () => { + const server = new MatrixError({ errcode: 'M_UNKNOWN', error: 'boom' }, 502); + const r = await run([server, 'ok']); + assert.equal(r.attempts, 2); + assert.equal(r.mxc, 'mxc://x/y'); + + const tooLarge = new MatrixError({ errcode: 'M_TOO_LARGE', error: 'big' }, 413); + const s = await run([tooLarge, 'ok']); + assert.equal(s.attempts, 1); + assert.equal(s.error?.httpStatus, 413); +});