Repository: ROpdebee/mb-userscripts (ts)

Issue:
[Enhanced Cover Art Uploads] Rate limit errors
There's a new time quota for batch fetching of cover art from Discogs repository, returning 429 error when exceeded. This results in incomplete artwork for extensive releases or booklets making it difficult to post add the missing images.

![screen](https://github.com/ROpdebee/mb-userscripts/assets/4992576/fe077a76-4888-4f7e-b619-e551ce82fa58)

I'm suggesting to check for response codes from Discogs and loop retry with some delay until 200 received as it's not certain what their time rate is.

Pull request:
fix(ecau): retry loading image contents on certain HTTP errors
Discogs has started to rate-limit image requests. Adding a retry-loop with exponential backoff on 429/5xx HTTP errors should alleviate most of these issues, and will work for other providers too.

Manually tested on a release with 47 Discogs images. 5 retries is not enough, but 10 seems to work.

Fixes #668.

Patch:
diff --git a/src/mb_enhanced_cover_art_uploads/fetch.ts b/src/mb_enhanced_cover_art_uploads/fetch.ts
index 9e468b01..24c690f8 100644
--- a/src/mb_enhanced_cover_art_uploads/fetch.ts
+++ b/src/mb_enhanced_cover_art_uploads/fetch.ts
@@ -1,9 +1,12 @@
-import type { FetchProgress} from '@lib/util/xhr';
+import pRetry from 'p-retry';
+
+import type { FetchProgress } from '@lib/util/xhr';
 import { getFromPageContext } from '@lib/compat';
 import { LOGGER } from '@lib/logging/logger';
 import { ArtworkTypeIDs } from '@lib/MB/CoverArt';
 import { enumerate } from '@lib/util/array';
 import { urlBasename } from '@lib/util/urls';
+import { HTTPResponseError } from '@lib/util/xhr';
 import { gmxhr } from '@lib/util/xhr';
 
 import type { CoverArtProvider } from './providers/base';
@@ -204,11 +207,27 @@ export class ImageFetcher {
     }
 
     private async fetchImageContents(url: URL, fileName: string, id: number, headers: Record<string, string>): Promise<ImageContents> {
-        const resp = await gmxhr(url, {
+        const xhrOptions = {
             responseType: 'blob',
             headers: headers,
             progressCb: this.hooks.onFetchProgress?.bind(this.hooks, id, url),
+        } as const;
+
+        // Need to retry image loading because some services, like Discogs, may
+        // return 429 HTTP errors.
+        // TODO: Copied from seeding/atisket/dimensions.ts, should be put in lib.
+        const resp = await pRetry(() => gmxhr(url, xhrOptions), {
+            retries: 10,
+            onFailedAttempt: (err) => {
+                // Don't retry on 4xx status codes except for 429. Anything below 400 doesn't throw a HTTPResponseError.
+                if (!(err instanceof HTTPResponseError) || (err.statusCode < 500 && err.statusCode !== 429)) {
+                    throw err;
+                }
+
+                LOGGER.info(`Failed to retrieve image contents after ${err.attemptNumber} attempt(s): ${err.message}. Retrying (${err.retriesLeft} attempt(s) left)…`);
+            },
         });
+
         // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
         if (resp.finalUrl === undefined) {
             LOGGER.warn(`Could not detect if URL ${url.href} caused a redirect`);


Test patch:
diff --git a/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts b/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts
index e9b6fa78..4bddfe9a 100644
--- a/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts
+++ b/tests/unit/mb_enhanced_cover_art_uploads/fetch.test.ts
@@ -1,15 +1,19 @@
+import type { FailedAttemptError } from 'p-retry';
+import pRetry from 'p-retry';
+
 import type { MaximisedImage } from '@src/mb_enhanced_cover_art_uploads/maximise';
 import type { ImageContents, QueuedImage } from '@src/mb_enhanced_cover_art_uploads/types';
 import { ArtworkTypeIDs } from '@lib/MB/CoverArt';
-import { gmxhr, NetworkError } from '@lib/util/xhr';
+import { gmxhr, HTTPResponseError, NetworkError } from '@lib/util/xhr';
 import { ImageFetcher } from '@src/mb_enhanced_cover_art_uploads/fetch';
 import { enqueueImage } from '@src/mb_enhanced_cover_art_uploads/form';
 import { getMaximisedCandidates } from '@src/mb_enhanced_cover_art_uploads/maximise';
 import { getProvider, getProviderByDomain } from '@src/mb_enhanced_cover_art_uploads/providers';
 import { CoverArtProvider } from '@src/mb_enhanced_cover_art_uploads/providers/base';
 
-import { createCoverArt, createImageFile, createXhrResponse } from './test-utils/dummy-data';
+import { createCoverArt, createHttpError, createImageFile, createXhrResponse } from './test-utils/dummy-data';
 
+jest.mock('p-retry');
 jest.mock('@lib/util/xhr');
 // We need to provide a mock factory, because for some reason, either jest or
 // rewire is not recognising the generator, leading to `getMaximisedCandidates`
@@ -20,6 +24,7 @@ jest.mock<{ getMaximisedCandidates: typeof getMaximisedCandidates }>('@src/mb_en
 jest.mock('@src/mb_enhanced_cover_art_uploads/providers');
 jest.mock('@src/mb_enhanced_cover_art_uploads/form');
 
+const mockpRetry = pRetry as jest.MockedFunction<typeof pRetry>;
 const mockXhr = gmxhr as jest.MockedFunction<typeof gmxhr>;
 const mockGetMaximisedCandidates = getMaximisedCandidates as jest.MockedFunction<typeof getMaximisedCandidates>;
 const mockGetProvider = getProvider as jest.MockedFunction<typeof getProvider>;
@@ -77,6 +82,11 @@ function disableDummyFetch(mock: FetchImageContentsSpy): void {
     mock.mockRestore();
 }
 
+beforeAll(() => {
+    // Mock p-retry so that it doesn't retry on image content fetching failure.
+    mockpRetry.mockImplementation(((fn) => (fn(0))) as typeof pRetry);
+});
+
 beforeEach(() => {
     mockEnqueueImage.mockClear();
     mockFindImages.mockReset();
@@ -88,17 +98,54 @@ beforeEach(() => {
 
 describe('fetching image contents', () => {
     let fetchImageContents: typeof ImageFetcher.prototype['fetchImageContents'];
+    // Used to register when a retry occurs in the p-retry mock, so we can verify in the tests.
+    const onRetry = jest.fn();
+
+    beforeAll(() => {
+        // Mock retrying behaviour so we can test it.
+        mockpRetry.mockImplementation(async (fn, options) => {
+            try {
+                // eslint-disable-next-line @typescript-eslint/return-await
+                return await fn(0);
+            } catch (err) {
+                Object.defineProperties(err, {
+                    attemptNumber: { value: 1, writable: false },
+                    retriesLeft: { value: 1, writable: false },
+                });
+                await options?.onFailedAttempt?.(err as FailedAttemptError);
+                // Call the onRetry mock so we can verify when a retry would've
+                // occurred.
+                onRetry();
+                return fn(1);
+            }
+        });
+    });
 
     beforeEach(() => {
         const fetcher = new ImageFetcher(hooks);
         fetchImageContents = fetcher['fetchImageContents'].bind(fetcher);
+        onRetry.mockClear();
+    });
+
+    afterAll(() => {
+        mockpRetry.mockImplementation((fn) => Promise.resolve(fn(0)));
     });
 
-    it('rejects on HTTP error', async () => {
+    it('rejects on network error', async () => {
         mockXhr.mockRejectedValueOnce(new NetworkError(new URL('https://example.com')));
 
         await expect(fetchImageContents(new URL('https://example.com/broken'), 'test.jpg', 0, {}))
             .rejects.toBeInstanceOf(NetworkError);
+        expect(onRetry).not.toHaveBeenCalled();
+    });
+
+    it('rejects on HTTP 404 error', async () => {
+        mockXhr.mockRejectedValue(createHttpError(createXhrResponse({ status: 404 })));
+
+        const result = fetchImageContents(new URL('https://example.com/broken'), 'test.jpg', 0, {});
+
+        await expect(result).rejects.toBeInstanceOf(HTTPResponseError);
+        expect(onRetry).not.toHaveBeenCalled();
     });
 
     it('rejects on text response', async () => {
@@ -167,6 +214,45 @@ describe('fetching image contents', () => {
             });
     });
 
+    it('retries on 429 response', async () => {
+        mockXhr.mockRejectedValueOnce(createHttpError(createXhrResponse({
+            status: 429,
+        })));
+        mockXhr.mockResolvedValueOnce(createXhrResponse({
+            finalUrl: 'https://example.com/working',
+            response: new Blob([Uint32Array.from([0x474E5089, 0xDEADBEEF])]),
+        }));
+
+        await expect(fetchImageContents(new URL('https://example.com/working'), 'test.jpg', 0, {}))
+            .resolves.toMatchObject({
+                file: {
+                    type: 'image/png',
+                    name: 'test.0.png',
+                },
+                requestedUrl: {
+                    href: 'https://example.com/working',
+                },
+                fetchedUrl: {
+                    href: 'https://example.com/working',
+                },
+                wasRedirected: false,
+            });
+        expect(onRetry).toHaveBeenCalledOnce();
+    });
+
+    it('rejects on too many 429 responses', async () => {
+        mockXhr.mockRejectedValueOnce(createHttpError(createXhrResponse({
+            status: 429,
+        })));
+        mockXhr.mockRejectedValueOnce(createHttpError(createXhrResponse({
+            status: 429,
+        })));
+
+        await expect(fetchImageContents(new URL('https://example.com/working'), 'test.jpg', 0, {}))
+            .rejects.toBeInstanceOf(HTTPResponseError);
+        expect(onRetry).toHaveBeenCalledOnce();
+    });
+
     it('retains redirection information', async () => {
         mockXhr.mockResolvedValueOnce(createXhrResponse({
             finalUrl: 'https://example.com/redirected',
diff --git a/tests/unit/mb_enhanced_cover_art_uploads/test-utils/dummy-data.ts b/tests/unit/mb_enhanced_cover_art_uploads/test-utils/dummy-data.ts
index 514cd546..69529de2 100644
--- a/tests/unit/mb_enhanced_cover_art_uploads/test-utils/dummy-data.ts
+++ b/tests/unit/mb_enhanced_cover_art_uploads/test-utils/dummy-data.ts
@@ -1,6 +1,7 @@
 // Abstractions to create dummy data
 
 import type { CoverArt, FetchedImage } from '@src/mb_enhanced_cover_art_uploads/types';
+import { HTTPResponseError } from '@lib/util/xhr';
 
 export interface DummyImageData {
     blob?: Blob;
@@ -90,3 +91,16 @@ export function createXhrResponse(response?: Partial<GM.Response<never>>): GM.Re
         responseXML: response.responseXML ?? false,
     };
 }
+
+export function createHttpError(response?: Partial<GM.Response<never>>): HTTPResponseError {
+    const xhrResponse = createXhrResponse(response);
+    const err = new HTTPResponseError(xhrResponse.finalUrl, xhrResponse);
+    // If gmxhr is mocked, the HTTP errors are too, so we need to define these
+    // properties manually.
+    Object.defineProperties(err, {
+        response: { value: xhrResponse },
+        statusCode: { value: xhrResponse.status },
+        statusText: { value: xhrResponse.statusText },
+    });
+    return err;
+}