diff --git a/sake/src/lib/server/application/services/HardcoverProgressSyncService.ts b/sake/src/lib/server/application/services/HardcoverProgressSyncService.ts index 587e49e..dbb67f6 100644 --- a/sake/src/lib/server/application/services/HardcoverProgressSyncService.ts +++ b/sake/src/lib/server/application/services/HardcoverProgressSyncService.ts @@ -174,7 +174,7 @@ export function describeHardcoverSyncFailure(cause: unknown): string { if (cause.kind === 'authentication') { return 'Hardcover rejected the configured API token. Update HARDCOVER_API_TOKEN, restart Sake, and retry.'; } - if (cause.kind === 'rate-limit') { + if (cause.kind === 'rate_limit') { return 'Hardcover rate limit reached. Sake will retry this job automatically.'; } if (cause.kind === 'timeout') { @@ -183,7 +183,7 @@ export function describeHardcoverSyncFailure(cause: unknown): string { if (cause.kind === 'network') { return 'Sake could not reach Hardcover. Check the server network connection; Sake will retry automatically.'; } - if (cause.kind === 'invalid-response') { + if (cause.kind === 'invalid_response') { return 'Hardcover returned an incomplete response. Sake will retry this job automatically.'; } if (cause.kind === 'configuration') { diff --git a/sake/src/lib/server/infrastructure/clients/HardcoverClient.ts b/sake/src/lib/server/infrastructure/clients/HardcoverClient.ts index 4eda206..f3cad8b 100644 --- a/sake/src/lib/server/infrastructure/clients/HardcoverClient.ts +++ b/sake/src/lib/server/infrastructure/clients/HardcoverClient.ts @@ -3,25 +3,23 @@ const UPSTREAM_TIMEOUT_MS = 30_000; const USER_AGENT = 'Sake/1.0 (+https://github.com/Sudashiii/Sake)'; const RATE_LIMIT_INTERVAL_MS = 1_000; -export type HardcoverClientErrorKind = - | 'authentication' - | 'rate-limit' - | 'upstream' - | 'graphql' - | 'timeout' - | 'network' - | 'invalid-response' - | 'mutation' - | 'configuration'; +import { + ExternalClientError, + parseExternalJson, + requestExternal, + type ExternalClientErrorKind +} from './externalClientPolicy'; -export class HardcoverClientError extends Error { +export type HardcoverClientErrorKind = ExternalClientErrorKind; + +export class HardcoverClientError extends ExternalClientError { constructor( message: string, readonly status: number, readonly isRetryable: boolean, readonly kind: HardcoverClientErrorKind = 'upstream' ) { - super(message); + super(message, status, isRetryable, kind); this.name = 'HardcoverClientError'; } } @@ -41,12 +39,10 @@ export class HardcoverClient { } this.nextAllowedAt = Date.now() + RATE_LIMIT_INTERVAL_MS; - const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), UPSTREAM_TIMEOUT_MS); try { - const response = await this.fetchFn(HARDCOVER_API_URL, { + const response = await requestExternal(this.fetchFn, HARDCOVER_API_URL, { method: 'POST', - signal: controller.signal, + timeoutMs: UPSTREAM_TIMEOUT_MS, headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${this.apiToken}`, @@ -55,25 +51,11 @@ export class HardcoverClient { body: JSON.stringify({ query, variables }) }); - if (!response.ok) { - const kind: HardcoverClientErrorKind = - response.status === 401 || response.status === 403 - ? 'authentication' - : response.status === 429 - ? 'rate-limit' - : 'upstream'; - throw new HardcoverClientError( - `Hardcover API returned HTTP ${response.status}`, - response.status, - response.status === 429 || response.status >= 500, - kind - ); - } - - const payload = (await response.json()) as { + const payload = await parseExternalJson(response, (value): value is { data?: T; errors?: Array<{ message?: string }>; - }; + } => typeof value === 'object' && value !== null && + ('data' in value || 'errors' in value)); if (payload.errors?.length) { const message = payload.errors.map((error) => error.message ?? 'Unknown GraphQL error').join('; '); throw new HardcoverClientError( @@ -84,19 +66,17 @@ export class HardcoverClient { ); } if (payload.data === undefined) { - throw new HardcoverClientError('Hardcover API returned no data', 502, true, 'invalid-response'); + throw new HardcoverClientError('Hardcover API returned no data', 502, true, 'invalid_response'); } return payload.data; } catch (cause: unknown) { if (cause instanceof HardcoverClientError) { throw cause; } - if (cause instanceof Error && cause.name === 'AbortError') { - throw new HardcoverClientError('Hardcover request timed out', 504, true, 'timeout'); + if (cause instanceof ExternalClientError) { + throw new HardcoverClientError(cause.message, cause.status, cause.isRetryable, cause.kind); } throw new HardcoverClientError('Hardcover request failed', 502, true, 'network'); - } finally { - clearTimeout(timer); } } } diff --git a/sake/src/lib/server/infrastructure/clients/ZLibraryClient.ts b/sake/src/lib/server/infrastructure/clients/ZLibraryClient.ts index 493b8f1..4765d41 100644 --- a/sake/src/lib/server/infrastructure/clients/ZLibraryClient.ts +++ b/sake/src/lib/server/infrastructure/clients/ZLibraryClient.ts @@ -5,11 +5,16 @@ import type { ZLibraryCredentials, ZLibraryPort, ZLibrarySearchRequest } from '$ import { toUrlEncoded } from '$lib/server/infrastructure/clients/toUrlEncode'; import type { ZLoginRequest } from '$lib/types/ZLibrary/Requests/ZLoginRequest'; import { apiError, apiOk, type ApiResult } from '$lib/server/http/api'; +import { + ExternalClientError, + parseExternalJson, + requestExternal +} from '$lib/server/infrastructure/clients/externalClientPolicy'; export class ZLibraryClient implements ZLibraryPort { private readonly baseUrl: string; - constructor(baseUrl: string) { + constructor(baseUrl: string, private readonly fetchFn: typeof fetch = fetch) { this.baseUrl = baseUrl; } @@ -40,7 +45,7 @@ export class ZLibraryClient implements ZLibraryPort { let fileInfo: ZBookFileResponse; try { - fileInfo = (await fileInfoResponse.value.json()) as ZBookFileResponse; + fileInfo = await parseExternalJson(fileInfoResponse.value, isZBookFileResponse); } catch (cause) { return apiError('Failed to parse download file info', 502, cause); } @@ -101,8 +106,9 @@ export class ZLibraryClient implements ZLibraryPort { private async get(path: string, credentials?: ZLibraryCredentials): Promise> { try { - const response = await fetch(this.baseUrl + path, { + const response = await requestExternal(this.fetchFn, this.baseUrl + path, { method: 'GET', + timeoutMs: 30_000, headers: this.getHeaders(credentials) }); @@ -112,14 +118,15 @@ export class ZLibraryClient implements ZLibraryPort { return apiOk(response); } catch (cause) { - return apiError('Failed to execute GET request', 502, cause); + return apiError('Failed to execute GET request', getExternalStatus(cause), cause); } } private async getAbsolute(url: string, credentials?: ZLibraryCredentials): Promise> { try { - const response = await fetch(url, { + const response = await requestExternal(this.fetchFn, url, { method: 'GET', + timeoutMs: 30_000, headers: this.getHeaders(credentials) }); @@ -129,7 +136,7 @@ export class ZLibraryClient implements ZLibraryPort { return apiOk(response); } catch (cause) { - return apiError('Failed to execute GET request', 502, cause); + return apiError('Failed to execute GET request', getExternalStatus(cause), cause); } } @@ -139,24 +146,46 @@ export class ZLibraryClient implements ZLibraryPort { credentials?: ZLibraryCredentials ): Promise> { try { - const response = await fetch(this.baseUrl + path, { + const response = await requestExternal(this.fetchFn, this.baseUrl + path, { method: 'POST', + timeoutMs: 30_000, headers: this.getHeaders(credentials), body: toUrlEncoded(data) }); - if (!response.ok) { - return apiError(`Request failed with status ${response.status}`, response.status); - } - - const parsed = (await response.json()) as T; + const parsed = await parseExternalJson(response, (value): value is T => { + if (path === ZLibraryRoutes.search) return isZSearchBookResponse(value); + if (path === ZLibraryRoutes.passwordLogin) return isZLoginResponse(value); + return typeof value === 'object' && value !== null; + }); return apiOk(parsed); } catch (cause) { - return apiError('Failed to execute POST request', 502, cause); + return apiError('Failed to execute POST request', getExternalStatus(cause), cause); } } } +function getExternalStatus(cause: unknown): number { + return cause instanceof ExternalClientError ? cause.status : 502; +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +function isZBookFileResponse(value: unknown): value is ZBookFileResponse { + if (!isRecord(value) || !isRecord(value.file)) return false; + return typeof value.success === 'number' && typeof value.file.downloadLink === 'string'; +} + +function isZSearchBookResponse(value: unknown): value is ZSearchBookResponse { + return isRecord(value) && typeof value.success === 'number' && Array.isArray(value.books); +} + +function isZLoginResponse(value: unknown): value is ZLoginResponse { + return isRecord(value) && (value.success === 0 || value.success === 1) && isRecord(value.user); +} + const ZLibraryRoutes: Record = { passwordLogin: '/eapi/user/login', profile: '/eapi/user/profile', diff --git a/sake/src/lib/server/infrastructure/clients/externalClientPolicy.ts b/sake/src/lib/server/infrastructure/clients/externalClientPolicy.ts new file mode 100644 index 0000000..4061f27 --- /dev/null +++ b/sake/src/lib/server/infrastructure/clients/externalClientPolicy.ts @@ -0,0 +1,86 @@ +export type ExternalClientErrorKind = + | 'timeout' + | 'rate_limit' + | 'authentication' + | 'invalid_response' + | 'upstream' + | 'network' + | 'graphql' + | 'mutation' + | 'configuration'; + +export class ExternalClientError extends Error { + constructor( + message: string, + readonly status: number, + readonly isRetryable: boolean, + readonly kind: ExternalClientErrorKind = 'upstream' + ) { + super(message); + this.name = 'ExternalClientError'; + } +} + +export interface ExternalRequestOptions extends RequestInit { + timeoutMs: number; +} + +export async function requestExternal( + fetchFn: typeof fetch, + input: RequestInfo | URL, + options: ExternalRequestOptions +): Promise { + const controller = new AbortController(); + const { timeoutMs, ...requestInit } = options; + const timer = setTimeout(() => controller.abort(), timeoutMs); + try { + const response = await fetchFn(input, { ...requestInit, signal: controller.signal }); + if (!response.ok) { + throw new ExternalClientError( + `External API returned HTTP ${response.status}`, + response.status, + response.status === 429 || response.status >= 500, + classifyStatus(response.status) + ); + } + return response; + } catch (cause: unknown) { + if (cause instanceof ExternalClientError) { + throw cause; + } + if (cause instanceof Error && cause.name === 'AbortError') { + throw new ExternalClientError('External request timed out', 504, true, 'timeout'); + } + throw new ExternalClientError('External request failed', 502, true, 'network'); + } finally { + clearTimeout(timer); + } +} + +export async function parseExternalJson( + response: Response, + validate: (value: unknown) => value is T, + maxBytes = 1_048_576 +): Promise { + const body = await response.text(); + if (new TextEncoder().encode(body).byteLength > maxBytes) { + throw new ExternalClientError('External API response was too large', 502, false, 'invalid_response'); + } + + let value: unknown; + try { + value = JSON.parse(body) as unknown; + } catch { + throw new ExternalClientError('External API returned invalid JSON', 502, false, 'invalid_response'); + } + if (!validate(value)) { + throw new ExternalClientError('External API returned an invalid response', 502, false, 'invalid_response'); + } + return value; +} + +function classifyStatus(status: number): ExternalClientErrorKind { + if (status === 401 || status === 403) return 'authentication'; + if (status === 429) return 'rate_limit'; + return 'upstream'; +} diff --git a/sake/src/lib/server/infrastructure/queue/downloadQueue.ts b/sake/src/lib/server/infrastructure/queue/downloadQueue.ts index e56f59b..c1ca269 100644 --- a/sake/src/lib/server/infrastructure/queue/downloadQueue.ts +++ b/sake/src/lib/server/infrastructure/queue/downloadQueue.ts @@ -15,6 +15,7 @@ import type { ZLibraryQueueTaskInput } from '$lib/server/application/ports/DownloadQueuePort'; import { randomUUID } from 'node:crypto'; +import { ExternalClientError } from '$lib/server/infrastructure/clients/externalClientPolicy'; interface BaseQueuedDownload { id: string; @@ -274,9 +275,9 @@ export class DownloadQueue { return; } - const canRetry = this.isRetryableFailure( + const canRetry = isRetryableExternalFailure( useCaseResult.error.status, - useCaseResult.error.message + useCaseResult.error.cause ); const isLastAttempt = attempt === task.maxAttempts; if (!canRetry || isLastAttempt) { @@ -369,22 +370,6 @@ export class DownloadQueue { return `${normalizedTitle}.${extension}`; } - private isRetryableFailure(statusCode: number, message: string): boolean { - if (statusCode === 429 || statusCode >= 500) { - return true; - } - - const normalized = message.toLowerCase(); - return ( - normalized.includes('terminated') || - normalized.includes('timeout') || - normalized.includes('econnreset') || - normalized.includes('network') || - normalized.includes('failed to execute get request') || - normalized.includes('failed to execute post request') - ); - } - private getRetryDelayMs(attempt: number): number { // 500ms, 1000ms, 2000ms... return 500 * 2 ** (attempt - 1); @@ -478,3 +463,10 @@ export class DownloadQueue { return copy.buffer; } } + +export function isRetryableExternalFailure(statusCode: number, cause: unknown): boolean { + if (cause instanceof ExternalClientError) { + return cause.isRetryable; + } + return statusCode === 429 || statusCode >= 500; +} diff --git a/sake/tests/contract/server/infrastructure/clients/hardcoverClient.test.ts b/sake/tests/contract/server/infrastructure/clients/hardcoverClient.test.ts index 85cbfe7..667c190 100644 --- a/sake/tests/contract/server/infrastructure/clients/hardcoverClient.test.ts +++ b/sake/tests/contract/server/infrastructure/clients/hardcoverClient.test.ts @@ -24,7 +24,7 @@ describe('HardcoverClient', () => { (error: unknown) => error instanceof HardcoverClientError && error.status === 429 && - error.kind === 'rate-limit' && + error.kind === 'rate_limit' && error.isRetryable ); }); diff --git a/sake/tests/contract/server/infrastructure/clients/zLibraryClient.test.ts b/sake/tests/contract/server/infrastructure/clients/zLibraryClient.test.ts new file mode 100644 index 0000000..13573b8 --- /dev/null +++ b/sake/tests/contract/server/infrastructure/clients/zLibraryClient.test.ts @@ -0,0 +1,44 @@ +import assert from 'node:assert/strict'; +import { describe, test } from 'node:test'; +import { ExternalClientError } from '$lib/server/infrastructure/clients/externalClientPolicy'; +import { ZLibraryClient } from '$lib/server/infrastructure/clients/ZLibraryClient'; + +describe('ZLibraryClient', () => { + test('classifies authentication responses without retrying', async () => { + const client = new ZLibraryClient('https://z.example', async () => new Response(null, { status: 401 })); + const result = await client.search({ searchText: 'book' }); + + assert.equal(result.ok, false); + if (result.ok) return; + assert.equal(result.error.status, 401); + assert.ok(result.error.cause instanceof ExternalClientError); + assert.equal(result.error.cause.kind, 'authentication'); + assert.equal(result.error.cause.isRetryable, false); + }); + + test('rejects malformed JSON as a non-retryable invalid response', async () => { + const client = new ZLibraryClient('https://z.example', async () => + new Response('{not-json', { status: 200, headers: { 'Content-Type': 'application/json' } }) + ); + const result = await client.search({ searchText: 'book' }); + + assert.equal(result.ok, false); + if (result.ok) return; + assert.ok(result.error.cause instanceof ExternalClientError); + assert.equal(result.error.cause.kind, 'invalid_response'); + assert.equal(result.error.cause.isRetryable, false); + }); + + test('classifies aborted requests as retryable timeouts', async () => { + const client = new ZLibraryClient('https://z.example', async () => { + throw new DOMException('aborted', 'AbortError'); + }); + const result = await client.search({ searchText: 'book' }); + + assert.equal(result.ok, false); + if (result.ok) return; + assert.ok(result.error.cause instanceof ExternalClientError); + assert.equal(result.error.cause.kind, 'timeout'); + assert.equal(result.error.cause.isRetryable, true); + }); +}); diff --git a/sake/tests/unit/lib/server/application/services/hardcoverProgressSyncService.test.ts b/sake/tests/unit/lib/server/application/services/hardcoverProgressSyncService.test.ts index dd07048..a7ee9c6 100644 --- a/sake/tests/unit/lib/server/application/services/hardcoverProgressSyncService.test.ts +++ b/sake/tests/unit/lib/server/application/services/hardcoverProgressSyncService.test.ts @@ -153,7 +153,7 @@ describe('HardcoverProgressSyncService', () => { ); assert.match( describeHardcoverSyncFailure( - new HardcoverClientError('Hardcover API returned HTTP 429', 429, true, 'rate-limit') + new HardcoverClientError('Hardcover API returned HTTP 429', 429, true, 'rate_limit') ), /retry.*automatically/i ); diff --git a/sake/tests/unit/lib/server/infrastructure/queue/downloadQueue.test.ts b/sake/tests/unit/lib/server/infrastructure/queue/downloadQueue.test.ts new file mode 100644 index 0000000..3735cb0 --- /dev/null +++ b/sake/tests/unit/lib/server/infrastructure/queue/downloadQueue.test.ts @@ -0,0 +1,29 @@ +import assert from 'node:assert/strict'; +import { describe, test } from 'node:test'; +import { ExternalClientError } from '$lib/server/infrastructure/clients/externalClientPolicy'; +import { isRetryableExternalFailure } from '$lib/server/infrastructure/queue/downloadQueue'; + +describe('download queue retry classification', () => { + test('uses structured external causes instead of their messages', () => { + assert.equal( + isRetryableExternalFailure( + 502, + new ExternalClientError('arbitrary message', 502, false, 'invalid_response') + ), + false + ); + assert.equal( + isRetryableExternalFailure( + 400, + new ExternalClientError('arbitrary message', 504, true, 'timeout') + ), + true + ); + }); + + test('retains status-based retries for unstructured upstream failures', () => { + assert.equal(isRetryableExternalFailure(429, undefined), true); + assert.equal(isRetryableExternalFailure(503, undefined), true); + assert.equal(isRetryableExternalFailure(400, new Error('timeout')), false); + }); +});