From 84185e1b68f7d93166a66d5143fc561d6dbdc421 Mon Sep 17 00:00:00 2001 From: Lucas Santos Date: Sun, 8 Nov 2026 21:13:22 +0100 Subject: [PATCH 01/18] feat: retry failed downloads with backoff, then remove them --- .../src/__tests__/downloads-service.test.ts | 3 + .../__tests__/failed-download-reaper.test.ts | 115 +++++++++++++ apps/backend/src/index.ts | 23 +++ apps/backend/src/lib/config.ts | 12 ++ .../src/modules/config/config.router.ts | 5 +- .../src/modules/config/config.service.ts | 5 +- .../downloads/failed-download-reaper.ts | 153 ++++++++++++++++++ apps/ui/app/components/DownloadsSection.vue | 38 ++++- apps/ui/app/types/management.ts | 3 + website/guide/management-ui.md | 4 +- website/reference/configuration.md | 59 ++++++- 11 files changed, 410 insertions(+), 10 deletions(-) create mode 100644 apps/backend/src/__tests__/failed-download-reaper.test.ts create mode 100644 apps/backend/src/modules/downloads/failed-download-reaper.ts diff --git a/apps/backend/src/__tests__/downloads-service.test.ts b/apps/backend/src/__tests__/downloads-service.test.ts index 6edadb4..dc016fe 100644 --- a/apps/backend/src/__tests__/downloads-service.test.ts +++ b/apps/backend/src/__tests__/downloads-service.test.ts @@ -46,6 +46,9 @@ function downloadsConfig(overrides: Partial> = {}) { manualImportBackoffBaseMs: 60_000, manualImportBackoffMaxMs: 1_800_000, unlinkImportedFiles: false, + removeFailedDownloads: false, + failedDownloadRetries: 3, + failedRetryBaseDelayMs: 60_000, ...overrides, } } diff --git a/apps/backend/src/__tests__/failed-download-reaper.test.ts b/apps/backend/src/__tests__/failed-download-reaper.test.ts new file mode 100644 index 0000000..da39432 --- /dev/null +++ b/apps/backend/src/__tests__/failed-download-reaper.test.ts @@ -0,0 +1,115 @@ +import type { DownloadRecord } from '../modules/downloads/downloads.repository' +import { expect, test } from 'bun:test' +import { FailedDownloadReaper } from '../modules/downloads/failed-download-reaper' + +function failedRow(overrides: Partial = {}): DownloadRecord { + return { + id: 1, + filename: 'Movie.mkv', + status: 'failed', + errorCode: 'PEER_UNREACHABLE', + operationFailed: true, + lastOperation: 'transfer', + importMode: null, + importTarget: null, + ...overrides, + } as DownloadRecord +} + +function setup(rows: DownloadRecord[], options: { enabled?: boolean, retryFails?: boolean } = {}) { + const clock = { now: 0 } + const calls: string[] = [] + const reaper = new FailedDownloadReaper({ + enabled: () => options.enabled ?? true, + list: () => rows, + retry: async (id) => { + calls.push(`retry:${id}`) + if (options.retryFails) + throw new Error('nope') + }, + remove: async (id) => { + calls.push(`remove:${id}`) + rows.splice(rows.findIndex(row => row.id === id), 1) + }, + maxRetries: 3, + baseDelayMs: 100, + maxDelayMs: 250, + now: () => clock.now, + }) + return { reaper, clock, calls } +} + +test('retries with backoff, then removes the download once retries are spent', async () => { + const rows = [ + failedRow(), + ] + const { reaper, clock, calls } = setup(rows) + + // Delays: 100, 200, then 400 capped to 250. Every retry fails again (row stays failed). + for (const at of [ + 0, + 99, + 100, + 100, + 299, + 300, + 300, + 549, + 550, + 550, + ]) + await (clock.now = at, reaper.tick()) + + expect(calls).toEqual([ + 'retry:1', + 'retry:1', + 'retry:1', + 'remove:1', + ]) + expect(rows).toHaveLength(0) +}) + +test('a retry that throws still counts toward the limit', async () => { + const rows = [ + failedRow(), + ] + const { reaper, clock, calls } = setup(rows, { retryFails: true }) + for (let at = 0; at <= 2000; at += 50) + await (clock.now = at, reaper.tick()) + expect(calls).toEqual([ + 'retry:1', + 'retry:1', + 'retry:1', + 'remove:1', + ]) +}) + +test('removes a failure that cannot be retried right away', async () => { + const rows = [ + failedRow({ lastOperation: 'import', importMode: null }), + ] + const { reaper, calls } = setup(rows) + await reaper.tick() + expect(calls).toEqual(['remove:1']) +}) + +test('leaves user cancels and other statuses alone', async () => { + const rows = [ + failedRow({ id: 1, errorCode: 'CANCELLED' }), + failedRow({ id: 2, status: 'downloading' }), + failedRow({ id: 3, status: 'imported' }), + ] + const { reaper, clock, calls } = setup(rows) + for (const at of [0, 1000, 5000]) + await (clock.now = at, reaper.tick()) + expect(calls).toEqual([]) +}) + +test('does nothing while disabled', async () => { + const rows = [ + failedRow({ lastOperation: 'import' }), + ] + const { reaper, calls } = setup(rows, { enabled: false }) + await reaper.tick() + expect(calls).toEqual([]) +}) diff --git a/apps/backend/src/index.ts b/apps/backend/src/index.ts index 7fabe33..5867120 100644 --- a/apps/backend/src/index.ts +++ b/apps/backend/src/index.ts @@ -16,8 +16,10 @@ import { CatalogSyncService } from './modules/catalog/catalog-sync.service' import { OwnCatalogRepository } from './modules/catalog/own-catalog.repository' import { TmdbMetadataRepository } from './modules/catalog/tmdb-metadata.repository' import { ConfigService } from './modules/config/config.service' +import { DownloadsManagementController } from './modules/downloads/downloads.controller' import { DownloadsRepository } from './modules/downloads/downloads.repository' import { DownloadsService } from './modules/downloads/downloads.service' +import { FailedDownloadReaper } from './modules/downloads/failed-download-reaper' import { ImportWatcher } from './modules/downloads/import-watcher' import { ManagedKeysRepository } from './modules/managed-keys/managed-keys.repository' import { ManagedApiKeys } from './modules/managed-keys/managed-keys.service' @@ -120,6 +122,23 @@ const importWatcher = downloadsConfig }) : undefined +// Retries failed downloads with backoff, then deletes the ones that keep failing. +// Routed through the management controller so transfers and imports each retry the +// way the ***Retry*** button does. +const downloadsController = new DownloadsManagementController(downloadsRepository, downloadsService, importWatcher) +const failedDownloadReaper = downloadsConfig + ? new FailedDownloadReaper({ + // Read per tick, like unlinkImportedFiles, so the UI toggle applies live. + enabled: () => configService?.getRawDownloads()?.removeFailedDownloads ?? downloadsConfig.removeFailedDownloads, + list: () => downloadsRepository.list(), + retry: id => downloadsController.retry(id), + remove: id => downloadsController.delete(id), + maxRetries: downloadsConfig.failedDownloadRetries, + baseDelayMs: downloadsConfig.failedRetryBaseDelayMs, + maxDelayMs: downloadsConfig.retryMaxDelayMs, + }) + : undefined + function startManagementServer() { if (!envs.MANAGEMENT_KEY) return undefined @@ -185,6 +204,8 @@ for (const dest of registrable) { managedApiKeys.prune(registrable.map(d => d.id)) importWatcher?.start() +if (downloadsConfig) + failedDownloadReaper?.start(downloadsConfig.importPollIntervalMs) // Re-drive interrupted downloads from a prior run. if (config.downloads && downloadsService) { @@ -204,6 +225,7 @@ else { process.on('SIGINT', async () => { logger.info('SIGINT received, exiting') importWatcher?.stop() + failedDownloadReaper?.stop() catalogSync.stop() database.close() server.stop() @@ -215,6 +237,7 @@ process.on('SIGINT', async () => { process.on('SIGTERM', async () => { logger.info('SIGTERM received, exiting') importWatcher?.stop() + failedDownloadReaper?.stop() catalogSync.stop() database.close() server.stop() diff --git a/apps/backend/src/lib/config.ts b/apps/backend/src/lib/config.ts index 176a83f..7c3cbfc 100644 --- a/apps/backend/src/lib/config.ts +++ b/apps/backend/src/lib/config.ts @@ -276,6 +276,15 @@ export const DownloadsConfig = z.object({ // library's link) or copied it. Enabling this unlinks jack's directory entry — // never a recursive delete — right after a row flips to `imported`. unlinkImportedFiles: z.boolean().default(false), + // When enabled, a download that ends up `failed` (a transfer or import whose own + // retries above are spent, or a permanent error) is retried as a whole up to + // `failedDownloadRetries` times, backing off exponentially from + // `failedRetryBaseDelayMs` (capped at `retryMaxDelayMs`). If it still fails, its + // row and any partial or completed file are deleted. User cancels are left alone. + removeFailedDownloads: z.boolean().default(false), + // At least 3: network errors are often only reported once per attempt. + failedDownloadRetries: z.number().int().min(3).default(3), + failedRetryBaseDelayMs: z.number().int().min(0).default(60_000), }) export type DownloadsConfig = z.infer @@ -300,6 +309,9 @@ export const RawDownloadsConfig = z.object({ manualImportBackoffBaseMs: z.number().int().min(0).nullish(), manualImportBackoffMaxMs: z.number().int().min(0).nullish(), unlinkImportedFiles: z.boolean().nullish(), + removeFailedDownloads: z.boolean().nullish(), + failedDownloadRetries: z.number().int().min(3).nullish(), + failedRetryBaseDelayMs: z.number().int().min(0).nullish(), }) export type RawDownloadsConfig = z.infer diff --git a/apps/backend/src/modules/config/config.router.ts b/apps/backend/src/modules/config/config.router.ts index 2eb18c0..ed7abe0 100644 --- a/apps/backend/src/modules/config/config.router.ts +++ b/apps/backend/src/modules/config/config.router.ts @@ -96,8 +96,9 @@ export function getConfigRouter(controller: ConfigController) { }) // Partial patch: the body is merged onto the stored downloads block. Every knob - // but `unlinkImportedFiles` is captured at boot, so those land on next restart. - app.patch('/downloads', configDoc('Update the downloads block', 'Merges the given fields into the stored downloads config. `unlinkImportedFiles` applies immediately; the other values take effect on next boot.'), zValidator('json', RawDownloadsConfig), async (c) => { + // but `unlinkImportedFiles` and `removeFailedDownloads` is captured at boot, so + // those take effect on next restart. + app.patch('/downloads', configDoc('Update the downloads block', 'Merges the given fields into the stored downloads config. `unlinkImportedFiles` and `removeFailedDownloads` apply immediately; the other values take effect on next boot.'), zValidator('json', RawDownloadsConfig), async (c) => { return c.json(await controller.updateDownloads(c.req.valid('json'))) }) diff --git a/apps/backend/src/modules/config/config.service.ts b/apps/backend/src/modules/config/config.service.ts index 70609b3..3d14dc4 100644 --- a/apps/backend/src/modules/config/config.service.ts +++ b/apps/backend/src/modules/config/config.service.ts @@ -376,8 +376,9 @@ export class ConfigService { * this is what the UI's empty field means. `undefined` (absent) leaves the stored * value alone. * - * Only `unlinkImportedFiles` takes effect immediately (the import watcher reads it - * per import); the rest are captured at boot and land on the next restart. + * Only `unlinkImportedFiles` and `removeFailedDownloads` take effect immediately + * (the import watcher and the failed-download reaper read them as they run); the + * rest are captured at boot and apply on the next restart. */ async updateDownloads(input: unknown): Promise { const patch = RawDownloadsConfig.parse(input) diff --git a/apps/backend/src/modules/downloads/failed-download-reaper.ts b/apps/backend/src/modules/downloads/failed-download-reaper.ts new file mode 100644 index 0000000..a39977d --- /dev/null +++ b/apps/backend/src/modules/downloads/failed-download-reaper.ts @@ -0,0 +1,153 @@ +import type { DownloadRecord } from './downloads.repository' +import { logger } from '../../logger' + +export interface FailedDownloadReaperOptions { + /** Read every tick, so toggling `downloads.removeFailedDownloads` applies live. */ + enabled: () => boolean + list: () => DownloadRecord[] + /** Repeats the row's last failed operation (transfer or import). */ + retry: (id: number) => Promise + /** Deletes the row and its unshared partial or completed files. */ + remove: (id: number) => Promise + maxRetries: number + baseDelayMs: number + maxDelayMs: number + /** Injectable for tests. */ + now?: () => number +} + +/** Same rule the UI uses to offer ***Retry***: only these failures can be repeated. */ +function canRetry(row: DownloadRecord): boolean { + if (!row.operationFailed) + return false + if (row.lastOperation === 'transfer') + return true + return row.lastOperation === 'import' && row.importMode === 'jack_manual' && row.importTarget != null +} + +/** + * Retries failed downloads with exponential backoff and deletes the ones that keep + * failing. Each tick, a failed row either waits out its backoff, gets retried, or + * (retries spent, or not retryable) is removed with its files. + * + * Retry counts live in memory: a restart starts every row's count over. + */ +export class FailedDownloadReaper { + // `nextAt` is unset right after a retry fires; the next tick that sees the row + // failed again schedules the following retry from that moment. + private readonly state = new Map() + private timer?: ReturnType + private tickInFlight?: Promise + + constructor(private readonly options: FailedDownloadReaperOptions) {} + + private delayFor(retries: number): number { + return Math.min(this.options.maxDelayMs, this.options.baseDelayMs * 2 ** retries) + } + + start(intervalMs: number): void { + if (this.timer) + return + this.timer = setInterval(() => { + void this.tick().catch((err) => { + logger.warn({ + error: err instanceof Error + ? err.message + : String(err), + }, 'Failed download reaper tick failed') + }) + }, intervalMs) + } + + stop(): void { + clearInterval(this.timer) + this.timer = undefined + } + + tick(): Promise { + // One pass at a time: a slow delete must not overlap the next interval. + this.tickInFlight ??= this.run().finally(() => { + this.tickInFlight = undefined + }) + return this.tickInFlight + } + + private async run(): Promise { + if (!this.options.enabled()) + return + const now = (this.options.now ?? Date.now)() + const rows = this.options.list() + + // Forget rows that are gone or finished, so their ids don't keep stale counts. + const live = new Map(rows.map(row => [row.id, row])) + for (const id of this.state.keys()) { + const status = live.get(id)?.status + if (status === undefined || status === 'imported') + this.state.delete(id) + } + + for (const row of rows) { + // A user cancel keeps its .part on purpose; leave it for them. + if (row.status !== 'failed' || row.errorCode === 'CANCELLED') + continue + + const entry = this.state.get(row.id) ?? { retries: 0 } + this.state.set(row.id, entry) + + if (entry.nextAt === undefined) { + if (entry.retries >= this.options.maxRetries || !canRetry(row)) { + await this.removeRow(row, entry.retries) + continue + } + entry.nextAt = now + this.delayFor(entry.retries) + continue + } + if (now < entry.nextAt) + continue + + entry.retries++ + entry.nextAt = undefined + try { + await this.options.retry(row.id) + logger.info({ + id: row.id, + filename: row.filename, + retry: entry.retries, + maxRetries: this.options.maxRetries, + }, 'Retrying failed download') + } + catch (err) { + // Counts as a spent retry; the next tick schedules the following one. + logger.warn({ + id: row.id, + retry: entry.retries, + error: err instanceof Error + ? err.message + : String(err), + }, 'Could not retry failed download') + } + } + } + + private async removeRow(row: DownloadRecord, retries: number): Promise { + try { + await this.options.remove(row.id) + this.state.delete(row.id) + logger.info({ + id: row.id, + filename: row.filename, + retries, + error_code: row.errorCode, + }, 'Removed failed download') + } + catch (err) { + // State is kept, so the next tick tries the removal again. + logger.warn({ + id: row.id, + error: err instanceof Error + ? err.message + : String(err), + }, 'Could not remove failed download') + } + } +} diff --git a/apps/ui/app/components/DownloadsSection.vue b/apps/ui/app/components/DownloadsSection.vue index 7c178c7..140e574 100644 --- a/apps/ui/app/components/DownloadsSection.vue +++ b/apps/ui/app/components/DownloadsSection.vue @@ -27,6 +27,9 @@ function blankState() { manualImportBackoffBaseMs: null as number | null, manualImportBackoffMaxMs: null as number | null, unlinkImportedFiles: false, + removeFailedDownloads: false, + failedDownloadRetries: null as number | null, + failedRetryBaseDelayMs: null as number | null, } } @@ -99,6 +102,9 @@ async function save() { manualImportBackoffMaxMs: state.manualImportBackoffMaxMs, // Off is the default, so write it as an absent key rather than an explicit false. unlinkImportedFiles: state.unlinkImportedFiles ? true : null, + removeFailedDownloads: state.removeFailedDownloads ? true : null, + failedDownloadRetries: state.failedDownloadRetries, + failedRetryBaseDelayMs: state.failedRetryBaseDelayMs, } const creating = !downloads.value try { @@ -133,6 +139,8 @@ const transferKeys = [ 'idleTimeoutMs', 'retryBaseDelayMs', 'retryMaxDelayMs', + 'failedDownloadRetries', + 'failedRetryBaseDelayMs', ] as const const importKeys = [ 'importPollIntervalMs', @@ -151,6 +159,8 @@ const DEFAULTS: Record = { maxManualImportAttempts: 6, manualImportBackoffBaseMs: 60_000, manualImportBackoffMaxMs: 1_800_000, + failedDownloadRetries: 3, + failedRetryBaseDelayMs: 60_000, } function customCount(keys: readonly string[]) { @@ -218,6 +228,14 @@ const importOpen = ref(false) > + + + + +
+ {{ selected.length }} selected + + +
+