From 4d58fbdfc6f1b72ce84b42d74bbbd5aac9ae1dc4 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Wed, 25 Mar 2026 03:28:20 -0600 Subject: [PATCH] feat: reactive ComicVine throttling with precise window-based cooldown - replace fixed RateLimiter with passive timestamp tracker exposing windowResetMs() - record throttle in ProviderThrottleTracker on 420 with exact time until hourly window resets - inject ProviderThrottleTracker into ComicVineProvider; catch ProviderThrottleError in search and lookupById - add unit tests for comicvine.client and comicvine.provider (49 tests) --- .../comicvine/comicvine.client.test.ts | 348 +++++++++++++++++ .../providers/comicvine/comicvine.client.ts | 21 +- .../comicvine/comicvine.provider.test.ts | 360 ++++++++++++++++++ .../providers/comicvine/comicvine.provider.ts | 39 +- 4 files changed, 757 insertions(+), 11 deletions(-) create mode 100644 server/src/modules/metadata-fetch/providers/comicvine/comicvine.client.test.ts create mode 100644 server/src/modules/metadata-fetch/providers/comicvine/comicvine.provider.test.ts diff --git a/server/src/modules/metadata-fetch/providers/comicvine/comicvine.client.test.ts b/server/src/modules/metadata-fetch/providers/comicvine/comicvine.client.test.ts new file mode 100644 index 000000000..10253c088 --- /dev/null +++ b/server/src/modules/metadata-fetch/providers/comicvine/comicvine.client.test.ts @@ -0,0 +1,348 @@ +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; + +import * as fetchWithThrottleModule from '../../fetch-with-throttle'; +import { ProviderThrottleError } from '../../provider-throttle.error'; +import { ComicVineClient } from './comicvine.client'; + +vi.mock('../../fetch-with-throttle', () => ({ + fetchWithThrottle: vi.fn(), +})); + +const HOUR_MS = 3_600_000; + +function makeOkResponse(results: unknown, isArray = true): Response { + return { + ok: true, + status: 200, + json: () => + Promise.resolve({ + status_code: 1, + error: 'OK', + results: isArray ? results : results, + }), + } as Response; +} + +function makeErrorResponse(status: number): Response { + return { ok: false, status } as Response; +} + +function make420Response(): Response { + return { ok: false, status: 420 } as Response; +} + +describe('ComicVineClient', () => { + let client: ComicVineClient; + let mockFetch: ReturnType; + + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date('2026-04-11T10:00:00.000Z')); + client = new ComicVineClient(); + mockFetch = vi.mocked(fetchWithThrottleModule.fetchWithThrottle); + mockFetch.mockClear(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + // --------------------------------------------------------------------------- + // RateLimiter / velocity guard + // --------------------------------------------------------------------------- + + describe('velocity guard', () => { + it('allows the first request immediately without delay', async () => { + mockFetch.mockResolvedValue(makeOkResponse([])); + const before = Date.now(); + const p = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + await p; + expect(Date.now() - before).toBe(0); + }); + + it('spaces consecutive requests 1 second apart', async () => { + mockFetch.mockResolvedValue(makeOkResponse([])); + + // Start and fully settle the first request so nextAllowedTime is set + const p1 = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + await p1; + expect(mockFetch).toHaveBeenCalledTimes(1); + + // Second request should wait 1 second before firing + const p2 = client.searchIssues('Batman', 'key'); + await vi.advanceTimersByTimeAsync(999); + expect(mockFetch).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(1); + expect(mockFetch).toHaveBeenCalledTimes(2); + + await p2; + }); + }); + + // --------------------------------------------------------------------------- + // windowResetMs + // --------------------------------------------------------------------------- + + describe('windowResetMs', () => { + it('returns 0 when no requests have been made', () => { + expect(client.windowResetMs()).toBe(0); + }); + + it('returns time until oldest request exits the 1-hour window', async () => { + mockFetch.mockResolvedValue(makeOkResponse([])); + const p = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + await p; + + // Oldest request was at t=0; window resets at t+1h + expect(client.windowResetMs()).toBe(HOUR_MS); + }); + + it('decreases as time passes', async () => { + mockFetch.mockResolvedValue(makeOkResponse([])); + const p = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + await p; + + vi.advanceTimersByTime(30 * 60 * 1000); // 30 minutes pass + expect(client.windowResetMs()).toBe(30 * 60 * 1000); + }); + + it('returns 0 after the window has fully elapsed', async () => { + mockFetch.mockResolvedValue(makeOkResponse([])); + const p = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + await p; + + vi.advanceTimersByTime(HOUR_MS + 1); + expect(client.windowResetMs()).toBe(0); + }); + + it('reflects the oldest in-window timestamp when multiple requests exist', async () => { + mockFetch.mockResolvedValue(makeOkResponse([])); + + // Two requests 10 minutes apart + const p1 = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + await p1; + + vi.advanceTimersByTime(10 * 60 * 1000); + + const p2 = client.searchIssues('Batman', 'key'); + await vi.runAllTimersAsync(); + await p2; + + // Oldest is the first one — window resets 1h after that + expect(client.windowResetMs()).toBe(50 * 60 * 1000); + }); + + it('evicts expired timestamps and returns 0 when all have exited the window', async () => { + mockFetch.mockResolvedValue(makeOkResponse([])); + + const p = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + await p; + + vi.advanceTimersByTime(HOUR_MS + 1000); + expect(client.windowResetMs()).toBe(0); + }); + }); + + // --------------------------------------------------------------------------- + // searchVolumes + // --------------------------------------------------------------------------- + + describe('searchVolumes', () => { + it('returns mapped volumes on success', async () => { + const volumes = [{ id: 1, name: 'Batman' }]; + mockFetch.mockResolvedValue(makeOkResponse(volumes)); + + const p = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + const result = await p; + + expect(result).toEqual(volumes); + }); + + it('returns cached result without making a new request', async () => { + const volumes = [{ id: 1, name: 'Batman' }]; + mockFetch.mockResolvedValue(makeOkResponse(volumes)); + + const p1 = client.searchVolumes('batman', 'key'); + await vi.runAllTimersAsync(); + await p1; + + const callCount = mockFetch.mock.calls.length; + + const p2 = client.searchVolumes('Batman', 'key'); // different casing + await vi.runAllTimersAsync(); + const result = await p2; + + expect(mockFetch).toHaveBeenCalledTimes(callCount); // no new request + expect(result).toEqual(volumes); + }); + + it('bypasses cache after TTL expires', async () => { + mockFetch.mockResolvedValue(makeOkResponse([{ id: 1, name: 'Batman' }])); + + const p1 = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + await p1; + + vi.advanceTimersByTime(10 * 60 * 1000 + 1); // past 10-min TTL + + mockFetch.mockResolvedValue(makeOkResponse([{ id: 2, name: 'Batman Vol 2' }])); + const p2 = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + const result = await p2; + + expect(result).toEqual([{ id: 2, name: 'Batman Vol 2' }]); + }); + + it('returns empty array on non-ok response', async () => { + mockFetch.mockResolvedValue(makeErrorResponse(500)); + + const p = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + + expect(await p).toEqual([]); + }); + + it('returns empty array when API status_code is not 1', async () => { + mockFetch.mockResolvedValue({ + ok: true, + status: 200, + json: () => Promise.resolve({ status_code: 100, error: 'Invalid API Key', results: null }), + } as Response); + + const p = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + + expect(await p).toEqual([]); + }); + + it('throws ProviderThrottleError on 420', async () => { + mockFetch.mockResolvedValue(make420Response()); + + const p = client.searchVolumes('Batman', 'key'); + const assertion = expect(p).rejects.toThrow(ProviderThrottleError); + await vi.runAllTimersAsync(); + await assertion; + }); + + it('returns empty array on network error', async () => { + mockFetch.mockRejectedValue(new Error('Network error')); + + const p = client.searchVolumes('Batman', 'key'); + await vi.runAllTimersAsync(); + + expect(await p).toEqual([]); + }); + }); + + // --------------------------------------------------------------------------- + // searchIssuesInVolume + // --------------------------------------------------------------------------- + + describe('searchIssuesInVolume', () => { + it('returns issues on success', async () => { + const issues = [{ id: 10, issue_number: '1' }]; + mockFetch.mockResolvedValue(makeOkResponse(issues)); + + const p = client.searchIssuesInVolume(1, '1', 'key'); + await vi.runAllTimersAsync(); + + expect(await p).toEqual(issues); + }); + + it('returns empty array on non-ok response', async () => { + mockFetch.mockResolvedValue(makeErrorResponse(503)); + + const p = client.searchIssuesInVolume(1, '1', 'key'); + await vi.runAllTimersAsync(); + + expect(await p).toEqual([]); + }); + + it('throws ProviderThrottleError on 420', async () => { + mockFetch.mockResolvedValue(make420Response()); + + const p = client.searchIssuesInVolume(1, '1', 'key'); + const assertion = expect(p).rejects.toThrow(ProviderThrottleError); + await vi.runAllTimersAsync(); + await assertion; + }); + }); + + // --------------------------------------------------------------------------- + // searchIssues + // --------------------------------------------------------------------------- + + describe('searchIssues', () => { + it('returns issues on success', async () => { + const issues = [{ id: 20, name: 'Batman #1' }]; + mockFetch.mockResolvedValue(makeOkResponse(issues)); + + const p = client.searchIssues('Batman #1', 'key'); + await vi.runAllTimersAsync(); + + expect(await p).toEqual(issues); + }); + + it('throws ProviderThrottleError on 420', async () => { + mockFetch.mockResolvedValue(make420Response()); + + const p = client.searchIssues('Batman', 'key'); + const assertion = expect(p).rejects.toThrow(ProviderThrottleError); + await vi.runAllTimersAsync(); + await assertion; + }); + }); + + // --------------------------------------------------------------------------- + // getIssueById + // --------------------------------------------------------------------------- + + describe('getIssueById', () => { + it('returns the issue on success', async () => { + const issue = { id: 999, name: 'Batman #1' }; + mockFetch.mockResolvedValue(makeOkResponse(issue, false)); + + const p = client.getIssueById('999', 'key'); + await vi.runAllTimersAsync(); + + expect(await p).toEqual(issue); + }); + + it('returns null on non-ok response', async () => { + mockFetch.mockResolvedValue(makeErrorResponse(404)); + + const p = client.getIssueById('999', 'key'); + await vi.runAllTimersAsync(); + + expect(await p).toBeNull(); + }); + + it('throws ProviderThrottleError on 420', async () => { + mockFetch.mockResolvedValue(make420Response()); + + const p = client.getIssueById('999', 'key'); + const assertion = expect(p).rejects.toThrow(ProviderThrottleError); + await vi.runAllTimersAsync(); + await assertion; + }); + + it('returns null on network error', async () => { + mockFetch.mockRejectedValue(new Error('timeout')); + + const p = client.getIssueById('999', 'key'); + await vi.runAllTimersAsync(); + + expect(await p).toBeNull(); + }); + }); +}); diff --git a/server/src/modules/metadata-fetch/providers/comicvine/comicvine.client.ts b/server/src/modules/metadata-fetch/providers/comicvine/comicvine.client.ts index 40487867f..a207147f0 100644 --- a/server/src/modules/metadata-fetch/providers/comicvine/comicvine.client.ts +++ b/server/src/modules/metadata-fetch/providers/comicvine/comicvine.client.ts @@ -6,7 +6,8 @@ import { ComicVineApiResponse, ComicVineIssue, ComicVineVolume } from './comicvi const BASE_URL = 'https://comicvine.gamespot.com/api'; const REQUEST_TIMEOUT_MS = 15_000; -const RATE_LIMIT_DELAY_MS = 1_000; +const VELOCITY_GUARD_MS = 1_000; +const HOURLY_WINDOW_MS = 3_600_000; const VOLUME_CACHE_TTL_MS = 10 * 60 * 1_000; const VOLUME_CACHE_MAX_SIZE = 50; @@ -25,16 +26,28 @@ interface CachedVolumes { class RateLimiter { private nextAllowedTime = 0; + private readonly timestamps: number[] = []; async throttle(): Promise { const now = Date.now(); const scheduled = Math.max(now, this.nextAllowedTime); - this.nextAllowedTime = scheduled + RATE_LIMIT_DELAY_MS; + this.nextAllowedTime = scheduled + VELOCITY_GUARD_MS; + this.timestamps.push(scheduled); const wait = scheduled - now; if (wait > 0) { await new Promise((resolve) => setTimeout(resolve, wait)); } } + + timeUntilWindowResetMs(): number { + const now = Date.now(); + const windowStart = now - HOURLY_WINDOW_MS; + while (this.timestamps.length > 0 && this.timestamps[0] < windowStart) { + this.timestamps.shift(); + } + if (this.timestamps.length === 0) return 0; + return Math.max(0, this.timestamps[0] + HOURLY_WINDOW_MS - now); + } } @Injectable() @@ -43,6 +56,10 @@ export class ComicVineClient { private readonly rateLimiter = new RateLimiter(); private readonly volumeCache = new Map(); + windowResetMs(): number { + return this.rateLimiter.timeUntilWindowResetMs(); + } + async searchVolumes(seriesName: string, apiKey: string): Promise { const cacheKey = seriesName.toLowerCase().trim(); const cached = this.volumeCache.get(cacheKey); diff --git a/server/src/modules/metadata-fetch/providers/comicvine/comicvine.provider.test.ts b/server/src/modules/metadata-fetch/providers/comicvine/comicvine.provider.test.ts new file mode 100644 index 000000000..8fd3dceb4 --- /dev/null +++ b/server/src/modules/metadata-fetch/providers/comicvine/comicvine.provider.test.ts @@ -0,0 +1,360 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { MetadataProviderKey, ProviderConfigurations } from '@projectx/types'; +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; + +import { ProviderConfigService } from '../../../metadata-preferences/provider-config.service'; +import { ProviderThrottleError } from '../../provider-throttle.error'; +import { ProviderThrottleTracker } from '../../provider-throttle.tracker'; +import { ComicVineClient } from './comicvine.client'; +import { ComicVineProvider } from './comicvine.provider'; + +const mockConfig: ProviderConfigurations = { + google: { enabled: false, apiKey: '' }, + amazon: { enabled: false, domain: 'amazon.com', cookie: '' }, + goodreads: { enabled: false }, + hardcover: { enabled: false, apiKey: '' }, + openLibrary: { enabled: false }, + itunes: { enabled: false, coverResolution: 'high' }, + audible: { enabled: false, domain: 'com' }, + audnexus: { enabled: false }, + comicvine: { enabled: true, apiKey: 'test-key' }, +}; + +const mockVolume = { + id: 1, + name: 'Batman', + start_year: '2016', + count_of_issues: 50, +}; + +const mockIssue = { + id: 100, + name: 'Batman #1', + issue_number: '1', + volume: { id: 1, name: 'Batman' }, + person_credits: [{ id: 1, name: 'Tom King', role: 'writer' }], + character_credits: [], + team_credits: [], + story_arc_credits: [], + location_credits: [], +}; + +describe('ComicVineProvider', () => { + let provider: ComicVineProvider; + let client: ComicVineClient; + let providerConfig: ProviderConfigService; + let throttleTracker: ProviderThrottleTracker; + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [ + ComicVineProvider, + { + provide: ComicVineClient, + useValue: { + searchVolumes: vi.fn().mockResolvedValue([]), + searchIssuesInVolume: vi.fn().mockResolvedValue([]), + searchIssues: vi.fn().mockResolvedValue([]), + getIssueById: vi.fn().mockResolvedValue(null), + windowResetMs: vi.fn().mockReturnValue(0), + }, + }, + { + provide: ProviderConfigService, + useValue: { + getConfig: vi.fn().mockResolvedValue(mockConfig), + }, + }, + { + provide: ProviderThrottleTracker, + useValue: { + record: vi.fn(), + isThrottled: vi.fn().mockReturnValue(false), + clearOnSuccess: vi.fn(), + }, + }, + ], + }).compile(); + + provider = module.get(ComicVineProvider); + client = module.get(ComicVineClient); + providerConfig = module.get(ProviderConfigService); + throttleTracker = module.get(ProviderThrottleTracker); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + // --------------------------------------------------------------------------- + // search — guard clauses + // --------------------------------------------------------------------------- + + describe('search - disabled / missing config', () => { + it('returns empty array when provider is disabled', async () => { + vi.spyOn(providerConfig, 'getConfig').mockResolvedValue({ + ...mockConfig, + comicvine: { enabled: false, apiKey: 'test-key' }, + }); + + expect(await provider.search({ title: 'Batman #1' })).toEqual([]); + expect(client.searchVolumes).not.toHaveBeenCalled(); + expect(client.searchIssues).not.toHaveBeenCalled(); + }); + + it('returns empty array when apiKey is missing', async () => { + vi.spyOn(providerConfig, 'getConfig').mockResolvedValue({ + ...mockConfig, + comicvine: { enabled: true, apiKey: '' }, + }); + + expect(await provider.search({ title: 'Batman #1' })).toEqual([]); + expect(client.searchVolumes).not.toHaveBeenCalled(); + }); + + it('returns empty array when title is missing', async () => { + expect(await provider.search({})).toEqual([]); + expect(client.searchVolumes).not.toHaveBeenCalled(); + expect(client.searchIssues).not.toHaveBeenCalled(); + }); + }); + + // --------------------------------------------------------------------------- + // search — structured path (title matches "Series #N" pattern) + // --------------------------------------------------------------------------- + + describe('search - structured path', () => { + it('calls searchVolumes then searchIssuesInVolume for a parsed title', async () => { + vi.spyOn(client, 'searchVolumes').mockResolvedValue([mockVolume]); + vi.spyOn(client, 'searchIssuesInVolume').mockResolvedValue([mockIssue]); + + const results = await provider.search({ title: 'Batman #1' }); + + expect(client.searchVolumes).toHaveBeenCalledWith('Batman', 'test-key'); + expect(client.searchIssuesInVolume).toHaveBeenCalledWith(mockVolume.id, '1', 'test-key'); + expect(results).toHaveLength(1); + }); + + it('tries volumes sorted by start_year descending', async () => { + const older = { ...mockVolume, id: 1, start_year: '1990' }; + const newer = { ...mockVolume, id: 2, start_year: '2016' }; + vi.spyOn(client, 'searchVolumes').mockResolvedValue([older, newer]); + vi.spyOn(client, 'searchIssuesInVolume').mockResolvedValue([mockIssue]); + + await provider.search({ title: 'Batman #1' }); + + expect(client.searchIssuesInVolume).toHaveBeenNthCalledWith(1, newer.id, '1', 'test-key'); + }); + + it('returns empty array when no volumes are found', async () => { + vi.spyOn(client, 'searchVolumes').mockResolvedValue([]); + + expect(await provider.search({ title: 'Batman #1' })).toEqual([]); + expect(client.searchIssuesInVolume).not.toHaveBeenCalled(); + }); + + it('tries next volume when first has no matching issues', async () => { + const v1 = { ...mockVolume, id: 1 }; + const v2 = { ...mockVolume, id: 2 }; + vi.spyOn(client, 'searchVolumes').mockResolvedValue([v1, v2]); + vi.spyOn(client, 'searchIssuesInVolume').mockResolvedValueOnce([]).mockResolvedValueOnce([mockIssue]); + + const results = await provider.search({ title: 'Batman #1' }); + + expect(client.searchIssuesInVolume).toHaveBeenCalledTimes(2); + expect(results).toHaveLength(1); + }); + + it('returns empty array when no volume has matching issues', async () => { + vi.spyOn(client, 'searchVolumes').mockResolvedValue([mockVolume]); + vi.spyOn(client, 'searchIssuesInVolume').mockResolvedValue([]); + + expect(await provider.search({ title: 'Batman #1' })).toEqual([]); + }); + + it('fetches issue detail when issue has no credits', async () => { + const issueNoCredits = { + ...mockIssue, + person_credits: [], + character_credits: [], + team_credits: [], + story_arc_credits: [], + location_credits: [], + }; + const issueWithCredits = { ...mockIssue }; + vi.spyOn(client, 'searchVolumes').mockResolvedValue([mockVolume]); + vi.spyOn(client, 'searchIssuesInVolume').mockResolvedValue([issueNoCredits]); + vi.spyOn(client, 'getIssueById').mockResolvedValue(issueWithCredits); + + await provider.search({ title: 'Batman #1' }); + + expect(client.getIssueById).toHaveBeenCalledWith(String(issueNoCredits.id), 'test-key'); + }); + + it('skips detail fetch when issue already has credits', async () => { + vi.spyOn(client, 'searchVolumes').mockResolvedValue([mockVolume]); + vi.spyOn(client, 'searchIssuesInVolume').mockResolvedValue([mockIssue]); + + await provider.search({ title: 'Batman #1' }); + + expect(client.getIssueById).not.toHaveBeenCalled(); + }); + + it('falls back to list-endpoint issue if detail fetch returns null', async () => { + const issueNoCredits = { + ...mockIssue, + person_credits: [], + character_credits: [], + team_credits: [], + story_arc_credits: [], + location_credits: [], + }; + vi.spyOn(client, 'searchVolumes').mockResolvedValue([mockVolume]); + vi.spyOn(client, 'searchIssuesInVolume').mockResolvedValue([issueNoCredits]); + vi.spyOn(client, 'getIssueById').mockResolvedValue(null); + + const results = await provider.search({ title: 'Batman #1' }); + + expect(results).toHaveLength(1); + }); + }); + + // --------------------------------------------------------------------------- + // search — general path (title does not match pattern) + // --------------------------------------------------------------------------- + + describe('search - general path', () => { + it('calls searchIssues when title does not match the issue pattern', async () => { + vi.spyOn(client, 'searchIssues').mockResolvedValue([mockIssue]); + + await provider.search({ title: 'Batman' }); + + expect(client.searchIssues).toHaveBeenCalledWith('Batman', 'test-key'); + expect(client.searchVolumes).not.toHaveBeenCalled(); + }); + + it('returns empty array when searchIssues returns nothing', async () => { + vi.spyOn(client, 'searchIssues').mockResolvedValue([]); + + expect(await provider.search({ title: 'Unknown Comic' })).toEqual([]); + }); + }); + + // --------------------------------------------------------------------------- + // search — throttle handling + // --------------------------------------------------------------------------- + + describe('search - throttle handling', () => { + it('returns empty array and records throttle when client throws ProviderThrottleError', async () => { + vi.spyOn(client, 'searchVolumes').mockRejectedValue(new ProviderThrottleError()); + vi.spyOn(client, 'windowResetMs').mockReturnValue(0); + + const result = await provider.search({ title: 'Batman #1' }); + + expect(result).toEqual([]); + expect(throttleTracker.record).toHaveBeenCalledWith(MetadataProviderKey.COMICVINE, undefined); + }); + + it('passes precise retryAfterSeconds derived from windowResetMs to the tracker', async () => { + vi.spyOn(client, 'searchVolumes').mockRejectedValue(new ProviderThrottleError()); + vi.spyOn(client, 'windowResetMs').mockReturnValue(62_000); // 62 seconds remaining + + await provider.search({ title: 'Batman #1' }); + + expect(throttleTracker.record).toHaveBeenCalledWith(MetadataProviderKey.COMICVINE, 62); + }); + + it('ceils fractional seconds when computing retryAfterSeconds', async () => { + vi.spyOn(client, 'searchIssues').mockRejectedValue(new ProviderThrottleError()); + vi.spyOn(client, 'windowResetMs').mockReturnValue(61_001); // 61.001 seconds + + await provider.search({ title: 'Batman' }); + + expect(throttleTracker.record).toHaveBeenCalledWith(MetadataProviderKey.COMICVINE, 62); + }); + + it('passes undefined retryAfterSeconds when windowResetMs returns 0', async () => { + vi.spyOn(client, 'searchIssues').mockRejectedValue(new ProviderThrottleError()); + vi.spyOn(client, 'windowResetMs').mockReturnValue(0); + + await provider.search({ title: 'Batman' }); + + expect(throttleTracker.record).toHaveBeenCalledWith(MetadataProviderKey.COMICVINE, undefined); + }); + + it('rethrows non-throttle errors from search', async () => { + vi.spyOn(client, 'searchVolumes').mockRejectedValue(new Error('Unexpected')); + + await expect(provider.search({ title: 'Batman #1' })).rejects.toThrow('Unexpected'); + expect(throttleTracker.record).not.toHaveBeenCalled(); + }); + + it('throttle on general path also records and returns empty array', async () => { + vi.spyOn(client, 'searchIssues').mockRejectedValue(new ProviderThrottleError()); + vi.spyOn(client, 'windowResetMs').mockReturnValue(120_000); + + const result = await provider.search({ title: 'Batman' }); + + expect(result).toEqual([]); + expect(throttleTracker.record).toHaveBeenCalledWith(MetadataProviderKey.COMICVINE, 120); + }); + }); + + // --------------------------------------------------------------------------- + // lookupById + // --------------------------------------------------------------------------- + + describe('lookupById', () => { + it('returns null when disabled', async () => { + vi.spyOn(providerConfig, 'getConfig').mockResolvedValue({ + ...mockConfig, + comicvine: { enabled: false, apiKey: 'test-key' }, + }); + + expect(await provider.lookupById('100')).toBeNull(); + expect(client.getIssueById).not.toHaveBeenCalled(); + }); + + it('returns null when apiKey is missing', async () => { + vi.spyOn(providerConfig, 'getConfig').mockResolvedValue({ + ...mockConfig, + comicvine: { enabled: true, apiKey: '' }, + }); + + expect(await provider.lookupById('100')).toBeNull(); + expect(client.getIssueById).not.toHaveBeenCalled(); + }); + + it('returns mapped candidate when issue is found', async () => { + vi.spyOn(client, 'getIssueById').mockResolvedValue(mockIssue); + + const result = await provider.lookupById('100'); + + expect(client.getIssueById).toHaveBeenCalledWith('100', 'test-key'); + expect(result).not.toBeNull(); + }); + + it('returns null when issue is not found', async () => { + vi.spyOn(client, 'getIssueById').mockResolvedValue(null); + + expect(await provider.lookupById('999')).toBeNull(); + }); + + it('returns null and records throttle on ProviderThrottleError', async () => { + vi.spyOn(client, 'getIssueById').mockRejectedValue(new ProviderThrottleError()); + vi.spyOn(client, 'windowResetMs').mockReturnValue(3_600_000); + + const result = await provider.lookupById('100'); + + expect(result).toBeNull(); + expect(throttleTracker.record).toHaveBeenCalledWith(MetadataProviderKey.COMICVINE, 3600); + }); + + it('rethrows non-throttle errors', async () => { + vi.spyOn(client, 'getIssueById').mockRejectedValue(new Error('DB error')); + + await expect(provider.lookupById('100')).rejects.toThrow('DB error'); + expect(throttleTracker.record).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/server/src/modules/metadata-fetch/providers/comicvine/comicvine.provider.ts b/server/src/modules/metadata-fetch/providers/comicvine/comicvine.provider.ts index c3432ab6a..7defd11f0 100644 --- a/server/src/modules/metadata-fetch/providers/comicvine/comicvine.provider.ts +++ b/server/src/modules/metadata-fetch/providers/comicvine/comicvine.provider.ts @@ -2,6 +2,8 @@ import { Injectable, Logger } from '@nestjs/common'; import { MetadataCandidate, MetadataProviderKey } from '@projectx/types'; import { ProviderConfigService } from '../../../metadata-preferences/provider-config.service'; +import { ProviderThrottleTracker } from '../../provider-throttle.tracker'; +import { ProviderThrottleError } from '../../provider-throttle.error'; import { IdentifiableProvider } from '../metadata-provider'; import { MetadataSearchParams } from '../metadata-search-params'; import { ComicVineClient } from './comicvine.client'; @@ -46,6 +48,7 @@ export class ComicVineProvider implements IdentifiableProvider { constructor( private readonly client: ComicVineClient, private readonly providerConfig: ProviderConfigService, + private readonly throttleTracker: ProviderThrottleTracker, ) {} async search(params: MetadataSearchParams): Promise { @@ -56,23 +59,41 @@ export class ComicVineProvider implements IdentifiableProvider { } if (!params.title) return []; - const maxCandidates = normalizeMaxCandidates(params.maxCandidatesPerProvider); - const parsed = parseIssueTitle(params.title); - if (parsed) { - return this.structuredSearch(parsed.seriesName, parsed.issueNumber, apiKey, maxCandidates); + try { + const maxCandidates = normalizeMaxCandidates(params.maxCandidatesPerProvider); + const parsed = parseIssueTitle(params.title); + return parsed + ? await this.structuredSearch(parsed.seriesName, parsed.issueNumber, apiKey, maxCandidates) + : await this.generalSearch(params.title, apiKey, maxCandidates); + } catch (err) { + if (err instanceof ProviderThrottleError) { + this.recordThrottle(); + return []; + } + throw err; } - - return this.generalSearch(params.title, apiKey, maxCandidates); } async lookupById(providerId: string): Promise { const { enabled, apiKey } = await this.providerConfig.getConfig().then((c) => c.comicvine); if (!enabled || !apiKey) return null; - const issue = await this.client.getIssueById(providerId, apiKey); - if (!issue) return null; + try { + const issue = await this.client.getIssueById(providerId, apiKey); + return issue ? mapIssueToCandidate(issue) : null; + } catch (err) { + if (err instanceof ProviderThrottleError) { + this.recordThrottle(); + return null; + } + throw err; + } + } - return mapIssueToCandidate(issue); + private recordThrottle(): void { + const waitMs = this.client.windowResetMs(); + const retryAfterSeconds = waitMs > 0 ? Math.ceil(waitMs / 1000) : undefined; + this.throttleTracker.record(MetadataProviderKey.COMICVINE, retryAfterSeconds); } private async structuredSearch(seriesName: string, issueNumber: string, apiKey: string, maxCandidates: number): Promise {