From 4de9390913acf5a21ba19c69f25703991d5bb1ba Mon Sep 17 00:00:00 2001 From: Giancarmine Salucci Date: Wed, 8 Jul 2026 22:54:47 +0200 Subject: [PATCH] =?UTF-8?q?D2:=20Fix=20upload=20retry=20500=20on=20missing?= =?UTF-8?q?=20file=20=E2=80=94=20async=20fire-and-forget?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit retryJob() now wraps the file read in a fire-and-forget async IIFE using fs/promises.readFile instead of readFileSync. When the original upload file has been cleaned up (common after failure), the IIFE catches the ENOENT error and marks the job as 'failed' with a 'retry failed:' prefix asynchronously — the endpoint returns 200 immediately. Includes 4 new integration tests: - Missing file returns immediately (no throw), job async-fails - ENOENT message propagated in job.error - 404 on unknown job still works (unchanged contract) - File present resets job to pending (runJob triggered) --- src/lib/server/pipeline.ts | 28 +++++-- src/tests/retry-endpoint.integration.test.ts | 64 ++++++++++++++++ src/tests/retry-pipeline.test.ts | 78 ++++++++++++++++++++ 3 files changed, 163 insertions(+), 7 deletions(-) create mode 100644 src/tests/retry-endpoint.integration.test.ts create mode 100644 src/tests/retry-pipeline.test.ts diff --git a/src/lib/server/pipeline.ts b/src/lib/server/pipeline.ts index 828b3bb..6dcccba 100644 --- a/src/lib/server/pipeline.ts +++ b/src/lib/server/pipeline.ts @@ -50,7 +50,11 @@ export async function startUploadJob( return job.id; } -/** Retry a failed/cancelled job — YouTube or upload-source. */ +/** Retry a failed/cancelled job — YouTube or upload-source. + * + * Upload retry reads the original file asynchronously (fire-and-forget IIFE) + * so a missing file produces an async job failure instead of a synchronous + * 500 crash. The outer function returns immediately. */ export async function retryJob(jobId: string): Promise { const job = getJob(jobId); if (!job) throw new Error('Job not found'); @@ -58,12 +62,22 @@ export async function retryJob(jobId: string): Promise { const isUpload = !job.source.startsWith('http'); if (isUpload) { - const { readFileSync } = await import('fs'); - const uploadPath = getUploadPath(jobId, job.source); - const buffer = readFileSync(uploadPath); - runJob(jobId, { type: 'upload', buffer, filename: job.source }, job.audioMode as AudioMode).catch((err) => { - console.error(`[pipeline] retry upload job ${jobId} failed:`, err); - }); + // Fire-and-forget: missing file -> async job failure, not 500 + (async () => { + try { + const { readFile } = await import('fs/promises'); + const uploadPath = getUploadPath(jobId, job.source); + const buffer = await readFile(uploadPath); + runJob(jobId, { type: 'upload', buffer, filename: job.source }, job.audioMode as AudioMode).catch((err) => { + console.error(`[pipeline] retry upload job ${jobId} failed:`, err); + }); + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err); + console.error(`[pipeline] retry upload job ${jobId} failed to read file:`, message); + updateJob({ id: jobId, status: 'failed', error: `retry failed: ${message}` }); + emitProgress(jobId, { type: 'error', message: `retry failed: ${message}` }); + } + })(); } else { runJob(jobId, { type: 'youtube', url: job.source }, job.audioMode as AudioMode).catch((err) => { console.error(`[pipeline] retry job ${jobId} failed:`, err); diff --git a/src/tests/retry-endpoint.integration.test.ts b/src/tests/retry-endpoint.integration.test.ts new file mode 100644 index 0000000..802f2db --- /dev/null +++ b/src/tests/retry-endpoint.integration.test.ts @@ -0,0 +1,64 @@ +import { describe, it, expect, afterAll, vi } from 'vitest'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { rm } from 'fs/promises'; + +/** + * Integration test for the retry endpoint against the REAL retryJob + + * REAL SQLite DB (no pipeline mock). Covers the exact outcome-contract + * scenario the mocked unit suite could not: retrying a failed upload-source + * job whose original uploaded audio file is ABSENT on disk. + * + * The endpoint must still return 200 { ok: true } (retryJob resolves + * immediately; the resubmit is fire-and-forget) and the missing file must + * fail the JOB (status -> failed), not the HTTP request (which was a 500 + * regression before the fix). + */ + +// DATA_DIR must be stubbed before db.js AND downloader.js import (both read +// it at module load to derive the jobs.db path and the uploads dir). +const TEST_DATA_DIR = join(tmpdir(), 'whisper-pwa-retry-int-' + process.pid); +vi.stubEnv('DATA_DIR', TEST_DATA_DIR); + +const { createJob, getJob, updateJob } = await import('$lib/server/db.js'); +const { POST } = await import('$lib/../routes/api/jobs/[id]/retry/+server.js'); + +afterAll(async () => { + await rm(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +function makeEvent(jobId: string) { + return { params: { id: jobId } } as any; +} + +async function waitForStatus(id: string, status: string, timeoutMs = 2000): Promise { + const start = Date.now(); + // eslint-disable-next-line no-constant-condition + while (true) { + const job = getJob(id); + if (job && job.status === status) return; + if (Date.now() - start > timeoutMs) { + throw new Error(`timed out waiting for job ${id} status=${status}; got ${job?.status}`); + } + await new Promise((r) => setTimeout(r, 20)); + } +} + +describe('POST /api/jobs/:id/retry — real retryJob (upload, missing file)', () => { + it('returns 200 and fails the JOB (not the request) when the upload file is absent', async () => { + // Failed upload-source job — non-http source, no file on disk. + const job = createJob('my-recording.webm', 'Upload retry probe', 'auto'); + updateJob({ id: job.id, status: 'failed', error: 'original failure' }); + + // The HTTP request must stay 200 even though the file is gone. + const res = await POST(makeEvent(job.id)); + expect(res.status).toBe(200); + expect(await res.json()).toEqual({ ok: true }); + + // The missing file fails the job asynchronously. + await waitForStatus(job.id, 'failed'); + const after = getJob(job.id)!; + expect(after.status).toBe('failed'); + expect(after.error).toMatch(/retry failed/i); + }); +}); diff --git a/src/tests/retry-pipeline.test.ts b/src/tests/retry-pipeline.test.ts new file mode 100644 index 0000000..9f2f9a8 --- /dev/null +++ b/src/tests/retry-pipeline.test.ts @@ -0,0 +1,78 @@ +import { describe, it, expect, vi } from 'vitest'; +import { rm, mkdir, writeFile } from 'fs/promises'; +import { join } from 'path'; + +import { createJob, getJob, updateJob } from '$lib/server/db.js'; +import { retryJob } from '$lib/server/pipeline.js'; +import { getUploadPath } from '$lib/server/downloader.js'; + +describe('retryJob — missing file handling', () => { + it('returns 200 immediately (does not throw) when upload file missing, sets job to failed async', async () => { + const job = createJob('missing-test.webm', 'Missing File Test', 'auto'); + // Put the job in failed state (as if it failed originally) + updateJob({ id: job.id, status: 'failed', error: 'original simulated failure' }); + + // retryJob should NOT throw — returns immediately (fire-and-forget IIFE) + await expect(retryJob(job.id)).resolves.toBeUndefined(); + + // The IIFE runs asynchronously; wait for it to complete by polling the DB + await vi.waitFor( + () => { + const updated = getJob(job.id)!; + expect(updated.status).toBe('failed'); + expect(updated.error).toMatch(/^retry failed:/); + }, + { timeout: 3000, interval: 50 } + ); + }); + + it('sets job to failed with descriptive ENOENT message when file is absent', async () => { + const job = createJob('no-file-here.opus', 'No File', 'standard'); + updateJob({ id: job.id, status: 'failed', error: 'first failure' }); + + await expect(retryJob(job.id)).resolves.toBeUndefined(); + + await vi.waitFor( + () => { + const updated = getJob(job.id)!; + expect(updated.status).toBe('failed'); + expect(updated.error).toMatch(/retry failed:.*ENOENT/i); + }, + { timeout: 3000, interval: 50 } + ); + }); + + it('leaves 404/409 path unchanged — throws on missing job', async () => { + await expect(retryJob('00000000-0000-0000-0000-000000000000')).rejects.toThrow('Job not found'); + }); +}); + +describe('retryJob — file present case still triggers retry', () => { + // getUploadPath resolves against default DATA_DIR (/tmp/.whisper-pwa) + // because downloader.ts module-level DATA_DIR is evaluated at first import + // and cached. Create the upload file there so readFile succeeds. + const DEFAULT_UPLOAD_DIR = '/tmp/.whisper-pwa/uploads'; + + it('returns immediately and resets job when file exists (runJob fires async)', async () => { + const job = createJob('present-test.mp3', 'File Present', 'auto'); + updateJob({ id: job.id, status: 'failed', error: 'simulated failure' }); + + // Create the upload file at the path getUploadPath returns + const jobUploadDir = join(DEFAULT_UPLOAD_DIR, job.id); + await mkdir(jobUploadDir, { recursive: true }); + const uploadPath = getUploadPath(job.id, 'present-test.mp3'); + await writeFile(uploadPath, Buffer.from('fake audio content')); + + // retryJob must not throw — returns immediately + await expect(retryJob(job.id)).resolves.toBeUndefined(); + + // resetJob() runs synchronously inside retryJob, so job status is + // immediately 'pending' after retryJob returns (before IIFE continues) + const updated = getJob(job.id)!; + expect(updated.status).toBe('pending'); + expect(updated.error).toBeNull(); + + // Cleanup + await rm(uploadPath, { force: true }); + }); +});