From 407b42dfb62c7c253cff5bee630a3ca710657b61 Mon Sep 17 00:00:00 2001 From: eleith Date: Tue, 6 Oct 2026 08:51:25 -0700 Subject: [PATCH 1/2] [worker] propagate transform failures and continue healthy pieces --- .../publish-transform-failure-handling.md | 5 + .../worker/src/steps/assets-generate.test.ts | 135 +++++++++++++++++- .../web/worker/src/steps/assets-generate.ts | 88 +++++++----- .../web/worker/src/transforms/image.test.ts | 87 +++++++++-- packages/web/worker/src/transforms/image.ts | 4 +- .../web/worker/src/transforms/runner.test.ts | 73 +++++++--- packages/web/worker/src/transforms/runner.ts | 2 +- .../src/transforms/utils/variants.test.ts | 40 ++++-- .../worker/src/transforms/utils/variants.ts | 1 + 9 files changed, 350 insertions(+), 85 deletions(-) create mode 100644 .changeset/publish-transform-failure-handling.md diff --git a/.changeset/publish-transform-failure-handling.md b/.changeset/publish-transform-failure-handling.md new file mode 100644 index 00000000..49dd8363 --- /dev/null +++ b/.changeset/publish-transform-failure-handling.md @@ -0,0 +1,5 @@ +--- +"@luzzle/web.worker": patch +--- + +Propagate transform and image-preparation errors instead of treating them as successful empty output. Wait for image variant writes to settle before reporting failure, skip asset-record persistence when production fails, collect failed-piece details and continue processing healthy pieces. Previously written files and metadata are not rolled back. diff --git a/packages/web/worker/src/steps/assets-generate.test.ts b/packages/web/worker/src/steps/assets-generate.test.ts index e46f78de..36da555c 100644 --- a/packages/web/worker/src/steps/assets-generate.test.ts +++ b/packages/web/worker/src/steps/assets-generate.test.ts @@ -1,5 +1,6 @@ import { describe, test, expect, vi, beforeEach, afterEach } from 'vitest' import type { Kysely } from 'kysely' +import { sql } from 'kysely' import type * as luzzleCore from '@luzzle/core' import type { LuzzleTables } from '@luzzle/core' import { Pieces, StorageFileSystem } from '@luzzle/core' @@ -113,8 +114,10 @@ describe('assetsGenerateStep', () => { await seedPiece({ id: 'a', file_path: 'books/a.md' }) await seedPiece({ id: 'b', file_path: 'books/b.md' }) - await assetsGenerateStep.run({ filePaths: ['books/a.md', 'books/b.md'] }, ctx) + const result = await assetsGenerateStep.run({ filePaths: ['books/a.md', 'books/b.md'] }, ctx) + expect(result).toEqual({ status: 'completed', value: { failedPieces: [] } }) + expect(ctx.logger.warn).not.toHaveBeenCalled() expect(mocks.runTransformsForPiece).toHaveBeenCalledTimes(2) const paths = mocks.runTransformsForPiece.mock.calls.map((c) => c[1].file_path).sort() expect(paths).toEqual(['books/a.md', 'books/b.md']) @@ -168,8 +171,136 @@ describe('assetsGenerateStep', () => { test('short-circuits and skips cleanup when filePaths is empty', async () => { await seedPiece() - await assetsGenerateStep.run({ filePaths: [] }, ctx) + const result = await assetsGenerateStep.run({ filePaths: [] }, ctx) + expect(result).toEqual({ status: 'completed', value: { failedPieces: [] } }) expect(mocks.runTransformsForPiece).not.toHaveBeenCalled() expect(mocks.cleanupAllTransforms).not.toHaveBeenCalled() }) + + test('continues after the first piece rejects', async () => { + await seedPiece({ id: 'a', file_path: 'books/a.md' }) + await seedPiece({ id: 'b', file_path: 'books/b.md' }) + mocks.runTransformsForPiece.mockRejectedValueOnce(new Error('piece processing failed')) + + const result = await assetsGenerateStep.run({ filePaths: ['books/a.md', 'books/b.md'] }, ctx) + + expect(mocks.runTransformsForPiece.mock.calls.map((call) => call[1].file_path)).toEqual([ + 'books/a.md', + 'books/b.md', + ]) + expect(result).toEqual({ + status: 'completed', + value: { failedPieces: [{ filePath: 'books/a.md', message: 'piece processing failed' }] }, + }) + expect(ctx.logger.error).toHaveBeenCalledWith('assets.generate failed for books/a.md', { + message: 'piece processing failed', + }) + expect(ctx.logger.warn).toHaveBeenCalledWith('assets.generate complete with failures', { + failedCount: 1, + count: 2, + }) + expect(ctx.logger.info).not.toHaveBeenCalledWith('assets.generate complete') + expect(mocks.cleanupAllTransforms).toHaveBeenCalledOnce() + expect(mocks.cleanupAllTransforms.mock.invocationCallOrder[0]).toBeGreaterThan( + mocks.runTransformsForPiece.mock.invocationCallOrder[1] + ) + }) + + test('continues after per-piece asset preparation fails', async () => { + await seedPiece({ id: 'a', file_path: 'books/a.md' }) + await seedPiece({ id: 'b', file_path: 'books/b.md' }) + mocks.buildAssetMaps.mockImplementationOnce(() => { + throw new Error('asset map failed') + }) + + const result = await assetsGenerateStep.run({ filePaths: ['books/a.md', 'books/b.md'] }, ctx) + + expect(result).toEqual({ + status: 'completed', + value: { failedPieces: [{ filePath: 'books/a.md', message: 'asset map failed' }] }, + }) + expect(mocks.buildAssetMaps).toHaveBeenCalledTimes(2) + expect(mocks.runTransformsForPiece).toHaveBeenCalledOnce() + expect(mocks.runTransformsForPiece.mock.calls[0][1].file_path).toBe('books/b.md') + expect(mocks.cleanupAllTransforms).toHaveBeenCalledOnce() + }) + + test.each([ + { thrown: 'string failure', message: 'string failure' }, + { thrown: null, message: 'null' }, + { thrown: undefined, message: 'undefined' }, + { thrown: { reason: 'failure' }, message: '[object Object]' }, + ])('normalizes non-Error rejection $message', async ({ thrown, message }) => { + await seedPiece() + mocks.runTransformsForPiece.mockRejectedValueOnce(thrown) + + const result = await assetsGenerateStep.run({ filePaths: ['books/great.md'] }, ctx) + + expect(result).toEqual({ + status: 'completed', + value: { failedPieces: [{ filePath: 'books/great.md', message }] }, + }) + expect(JSON.parse(JSON.stringify(result))).toEqual(result) + expect(ctx.logger.error).toHaveBeenCalledWith('assets.generate failed for books/great.md', { + message, + }) + expect(ctx.logger.warn).toHaveBeenCalledWith('assets.generate complete with failures', { + failedCount: 1, + count: 1, + }) + expect(mocks.cleanupAllTransforms).toHaveBeenCalledOnce() + }) + + test('collects every failed piece', async () => { + await seedPiece({ id: 'a', file_path: 'books/a.md' }) + await seedPiece({ id: 'b', file_path: 'books/b.md' }) + mocks.runTransformsForPiece + .mockRejectedValueOnce(new Error('first failure')) + .mockRejectedValueOnce(new Error('second failure')) + + const result = await assetsGenerateStep.run({ filePaths: ['books/a.md', 'books/b.md'] }, ctx) + + expect(result).toEqual({ + status: 'completed', + value: { + failedPieces: [ + { filePath: 'books/a.md', message: 'first failure' }, + { filePath: 'books/b.md', message: 'second failure' }, + ], + }, + }) + expect(ctx.logger.warn).toHaveBeenCalledWith('assets.generate complete with failures', { + failedCount: 2, + count: 2, + }) + }) + + test('propagates global selection failures and still cleans up', async () => { + await sql`DROP TABLE web_pieces`.execute(db) + + await expect( + assetsGenerateStep.run({ filePaths: ['books/great.md'] }, ctx) + ).rejects.toThrow('no such table: web_pieces') + + expect(mocks.runTransformsForPiece).not.toHaveBeenCalled() + expect(mocks.cleanupAllTransforms).toHaveBeenCalledOnce() + expect(ctx.logger.warn).not.toHaveBeenCalled() + expect(ctx.logger.info).not.toHaveBeenCalledWith('assets.generate complete') + }) + + test.each([false, true])('propagates cleanup failures (piece failed: %s)', async (pieceFailed) => { + await seedPiece() + if (pieceFailed) mocks.runTransformsForPiece.mockRejectedValueOnce(new Error('piece failed')) + const error = new Error('cleanup failed') + mocks.cleanupAllTransforms.mockRejectedValueOnce(error) + + await expect( + assetsGenerateStep.run({ filePaths: ['books/great.md'] }, ctx) + ).rejects.toBe(error) + + expect(mocks.runTransformsForPiece).toHaveBeenCalledOnce() + expect(mocks.cleanupAllTransforms).toHaveBeenCalledOnce() + expect(ctx.logger.warn).not.toHaveBeenCalled() + expect(ctx.logger.info).not.toHaveBeenCalledWith('assets.generate complete') + }) }) diff --git a/packages/web/worker/src/steps/assets-generate.ts b/packages/web/worker/src/steps/assets-generate.ts index fefa21b7..849b4030 100644 --- a/packages/web/worker/src/steps/assets-generate.ts +++ b/packages/web/worker/src/steps/assets-generate.ts @@ -10,56 +10,78 @@ export interface AssetsGenerateInput { filePaths: string[] } +export interface AssetsGenerateResult { + failedPieces: Array<{ filePath: string; message: string }> +} + type FullDb = Kysely -export const assetsGenerateStep: Step = { +export const assetsGenerateStep: Step = { name: 'assets.generate', - async run({ filePaths }, ctx): Promise> { + async run({ filePaths }, ctx): Promise> { const { db, config, logger } = ctx const fullDb = db as unknown as FullDb logger.info('assets.generate starting', { count: filePaths.length }) + const failedPieces: AssetsGenerateResult['failedPieces'] = [] if (filePaths.length === 0) { logger.info('assets.generate complete') - return completed(undefined) + return completed({ failedPieces }) } - const storage = new StorageFileSystem(config.storage.root) - const pieces = new Pieces(storage) - const outDir = config.paths.assets + let pieceCount = 0 + try { + const storage = new StorageFileSystem(config.storage.root) + const pieces = new Pieces(storage) + const outDir = config.paths.assets - const webPieces = await fullDb - .selectFrom('web_pieces') - .selectAll() - .where('file_path', 'in', filePaths) - .execute() + const webPieces = await fullDb + .selectFrom('web_pieces') + .selectAll() + .where('file_path', 'in', filePaths) + .execute() + pieceCount = webPieces.length - for (const webPiece of webPieces) { - const item = await fullDb - .selectFrom('pieces_items') - .select('assets_json_array') - .where('file_path', '=', webPiece.file_path) - .executeTakeFirst() + for (const webPiece of webPieces) { + try { + const item = await fullDb + .selectFrom('pieces_items') + .select('assets_json_array') + .where('file_path', '=', webPiece.file_path) + .executeTakeFirst() - const { keyToPath } = buildAssetMaps(item?.assets_json_array, config.assets.salt) + const { keyToPath } = buildAssetMaps(item?.assets_json_array, config.assets.salt) - logger.info(`assets.generate running transforms: ${webPiece.file_path}`) - await runTransformsForPiece( - fullDb as unknown as Kysely, - webPiece, - config, - outDir, - pieces, - {}, - keyToPath, - logger - ) + logger.info(`assets.generate running transforms: ${webPiece.file_path}`) + await runTransformsForPiece( + fullDb as unknown as Kysely, + webPiece, + config, + outDir, + pieces, + {}, + keyToPath, + logger + ) + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + failedPieces.push({ filePath: webPiece.file_path, message }) + logger.error(`assets.generate failed for ${webPiece.file_path}`, { message }) + } + } + } finally { + await cleanupAllTransforms() } - await cleanupAllTransforms() - - logger.info('assets.generate complete') - return completed(undefined) + if (failedPieces.length > 0) { + logger.warn('assets.generate complete with failures', { + failedCount: failedPieces.length, + count: pieceCount, + }) + } else { + logger.info('assets.generate complete') + } + return completed({ failedPieces }) }, } diff --git a/packages/web/worker/src/transforms/image.test.ts b/packages/web/worker/src/transforms/image.test.ts index c684824d..91c84f2c 100644 --- a/packages/web/worker/src/transforms/image.test.ts +++ b/packages/web/worker/src/transforms/image.test.ts @@ -1,12 +1,7 @@ import { describe, test, expect, vi, afterEach } from 'vitest' import { run } from './image.js' import { mkdir, writeFile } from 'fs/promises' -import { - getAssetPath, - getAssetDir, - getImageAssetPath, - ASSET_SIZES, -} from '../assets/paths.js' +import { getAssetPath, getAssetDir, getImageAssetPath, ASSET_SIZES } from '../assets/paths.js' import { type Config } from '@luzzle/web.config' import { generateVariantJobs } from './utils/variants.js' import type { Pieces } from '@luzzle/core' @@ -70,7 +65,7 @@ describe('transforms/image', () => { outDir: '/out', pieces: mockPieces, assetKeyToPath: emptyMap, - logger: makeLogger() + logger: makeLogger(), }) expect(mockPieces.getPieceAsset).not.toHaveBeenCalled() @@ -89,7 +84,7 @@ describe('transforms/image', () => { outDir: '/out', pieces: mockPieces, assetKeyToPath: new Map(), - logger: makeLogger() + logger: makeLogger(), }) expect(mockPieces.getPieceAsset).not.toHaveBeenCalled() @@ -107,7 +102,7 @@ describe('transforms/image', () => { outDir: '/out', pieces: mockPieces, assetKeyToPath: emptyMap, - logger: makeLogger() + logger: makeLogger(), }) expect(mockPieces.getPieceAsset).not.toHaveBeenCalled() @@ -126,7 +121,7 @@ describe('transforms/image', () => { outDir: '/out', pieces: mockPieces, assetKeyToPath: new Map([['key', 'file.pdf']]), - logger: makeLogger() + logger: makeLogger(), }) ).rejects.toThrow('non-image file') }) @@ -154,7 +149,7 @@ describe('transforms/image', () => { outDir: '/out', pieces: mockPieces, assetKeyToPath: new Map([['key', 'photo.jpg']]), - logger: makeLogger() + logger: makeLogger(), }) expect(mocks.mkdir).toHaveBeenCalledWith('/out/books/key', { recursive: true }) @@ -187,7 +182,7 @@ describe('transforms/image', () => { outDir: '/out', pieces: mockPieces, assetKeyToPath: new Map([['key', 'photo.jpg']]), - logger: makeLogger() + logger: makeLogger(), }) ).rejects.toThrow('read error') }) @@ -215,7 +210,7 @@ describe('transforms/image', () => { outDir: '/out', pieces: mockPieces, assetKeyToPath: new Map([['key', 'photo.jpg']]), - logger: makeLogger() + logger: makeLogger(), }) expect(records).toEqual( @@ -237,11 +232,73 @@ describe('transforms/image', () => { outDir: '/out', pieces: mockPieces, assetKeyToPath: new Map([['key', 'photo']]), - logger: makeLogger() + logger: makeLogger(), }) ).rejects.toThrow('non-image file') }) + test('waits for remaining variant writes before reporting a variant failure', async () => { + const error = new Error('first variant failed') + let failFast!: (reason: Error) => void + let finishSlow!: () => void + let startedSlow!: () => void + const fast = new Promise((_, reject) => { + failFast = reject + }) + const slow = new Promise((resolve) => { + finishSlow = resolve + }) + const started = new Promise((resolve) => { + startedSlow = resolve + }) + mocks.getAssetDir.mockReturnValue('books/key') + mocks.getAssetPath.mockReturnValue('books/key/photo.jpg') + mocks.getImageAssetPath.mockReturnValue('books/key/photo.variant.jpg') + mocks.generateVariantJobs.mockResolvedValue([ + { sharp: { toFile: vi.fn(() => fast) } as unknown as Sharp, width: 125, format: 'jpg' }, + { + sharp: { + toFile: vi.fn(() => { + startedSlow() + return slow + }), + } as unknown as Sharp, + width: 250, + format: 'jpg', + }, + ]) + const pieces = { + getPieceAsset: vi.fn().mockResolvedValue(Buffer.from('image_data')), + } as unknown as Pieces + let settled = false + const outcome = run({ + webPiece: makeWebPiece('{"image":"key"}'), + config: makeConfig(['image']), + outDir: '/out', + pieces, + assetKeyToPath: new Map([['key', 'photo.jpg']]), + logger: makeLogger(), + }).then( + () => { + settled = true + return null + }, + (reason) => { + settled = true + return reason + } + ) + try { + await started + failFast(error) + await new Promise((resolve) => setImmediate(resolve)) + expect(settled).toBe(false) + } finally { + finishSlow() + } + expect(await outcome).toBe(error) + }) + test('throws on variant toFile error', async () => { const mockPieces = { getPieceAsset: vi.fn().mockResolvedValue(Buffer.from('image_data')), @@ -265,7 +322,7 @@ describe('transforms/image', () => { outDir: '/out', pieces: mockPieces, assetKeyToPath: new Map([['key', 'photo.jpg']]), - logger: makeLogger() + logger: makeLogger(), }) ).rejects.toThrow('toFile error') }) diff --git a/packages/web/worker/src/transforms/image.ts b/packages/web/worker/src/transforms/image.ts index c983d4ab..29ddfc94 100644 --- a/packages/web/worker/src/transforms/image.ts +++ b/packages/web/worker/src/transforms/image.ts @@ -95,7 +95,9 @@ export async function run({ }) }) - await Promise.all(toFileJobs) + const results = await Promise.allSettled(toFileJobs) + const failed = results.find((result) => result.status === 'rejected') + if (failed) throw failed.reason } } diff --git a/packages/web/worker/src/transforms/runner.test.ts b/packages/web/worker/src/transforms/runner.test.ts index 7968b26e..d27e9269 100644 --- a/packages/web/worker/src/transforms/runner.test.ts +++ b/packages/web/worker/src/transforms/runner.test.ts @@ -174,10 +174,12 @@ describe('transforms/runner', () => { logger ) - expect(logger.info).toHaveBeenCalledWith('transform.palette generated content of application/json') + expect(logger.info).toHaveBeenCalledWith( + 'transform.palette generated content of application/json' + ) }) - test('logs error and continues when transform throws', async () => { + test('logs and propagates a transform failure instead of producing empty records', async () => { transforms.attachment.run.mockRejectedValueOnce(new Error('boom')) await expect( @@ -191,7 +193,7 @@ describe('transforms/runner', () => { new Map(), logger ) - ).resolves.not.toThrow() + ).rejects.toThrow('boom') expect(logger.error).toHaveBeenCalledWith( 'transform.attachment error for book.md', @@ -199,6 +201,43 @@ describe('transforms/runner', () => { ) }) + test('retains existing asset rows when a later transform fails after earlier production succeeds', async () => { + const oldRecords = [ + { + piece_file_path: webPiece.file_path, + piece_key: webPiece.key, + asset_key: 'old-attachment', + transformation: 'attachment', + asset_path: 'old.pdf', + mime_type: 'application/pdf', + }, + { + piece_file_path: webPiece.file_path, + piece_key: webPiece.key, + asset_key: 'old-palette', + transformation: 'palette', + content: 'old palette', + mime_type: 'application/json', + }, + ] + await db.insertInto('web_pieces_assets').values(oldRecords).execute() + transforms.attachment.run.mockResolvedValueOnce([ + { transformation: 'attachment', asset_path: 'new.pdf', mime_type: 'application/pdf' }, + ]) + transforms.palette.run.mockRejectedValueOnce(new Error('palette failed')) + + await expect( + runTransformsForPiece(db, webPiece, config, '/out', {} as Pieces, {}, new Map(), logger) + ).rejects.toThrow('palette failed') + const assets = await db + .selectFrom('web_pieces_assets') + .selectAll() + .orderBy('asset_key') + .execute() + expect(assets).toMatchObject(oldRecords) + expect(transforms.opengraph.run).not.toHaveBeenCalled() + }) + test('does nothing when typeFilter matches no transform', async () => { await runTransformsForPiece( db, @@ -243,25 +282,15 @@ describe('transforms/produceTransformsForPiece', () => { expect(assets).toHaveLength(0) }) - test('records a failed transform as empty and keeps going', async () => { - transforms.attachment.run.mockRejectedValueOnce(new Error('boom')) - transforms.palette.run.mockResolvedValueOnce([]) + test('rejects failed production without returning a successful result or running later transforms', async () => { + const error = new Error('boom') + transforms.attachment.run.mockRejectedValueOnce(error) - const produced = await produceTransformsForPiece( - webPiece, - config, - '/out', - {} as Pieces, - {}, - new Map(), - logger - ) - - expect(produced).toEqual([ - { name: 'attachment', records: [] }, - { name: 'palette', records: [] }, - { name: 'opengraph', records: [] }, - ]) + await expect( + produceTransformsForPiece(webPiece, config, '/out', {} as Pieces, {}, new Map(), logger) + ).rejects.toBe(error) + expect(transforms.palette.run).not.toHaveBeenCalled() + expect(transforms.opengraph.run).not.toHaveBeenCalled() expect(logger.error).toHaveBeenCalledWith( 'transform.attachment error for book.md', expect.objectContaining({ error: 'boom' }) @@ -307,7 +336,7 @@ describe('transforms/persistTransforms', () => { expect(assets[0]).toMatchObject({ content: 'fresh', asset_key: 'asset-key' }) }) - test('still deletes when produced records are empty (failed transform)', async () => { + test('still clears prior records after successful production of an empty result', async () => { await db .insertInto('web_pieces_assets') .values({ diff --git a/packages/web/worker/src/transforms/runner.ts b/packages/web/worker/src/transforms/runner.ts index 294b61e9..a05c008c 100644 --- a/packages/web/worker/src/transforms/runner.ts +++ b/packages/web/worker/src/transforms/runner.ts @@ -68,7 +68,7 @@ export async function produceTransformsForPiece( logger.error(`transform.${name} error for ${webPiece.file_path}`, { error: error instanceof Error ? error.message : String(error), }) - produced.push({ name, records: [] }) + throw error } } diff --git a/packages/web/worker/src/transforms/utils/variants.test.ts b/packages/web/worker/src/transforms/utils/variants.test.ts index 2c298593..ad077e6a 100644 --- a/packages/web/worker/src/transforms/utils/variants.test.ts +++ b/packages/web/worker/src/transforms/utils/variants.test.ts @@ -11,25 +11,43 @@ describe('generateVariantJobs', () => { vi.clearAllMocks() }) - test('should return an empty array if getPieceAsset fails', async () => { + test('propagates asset-read failure instead of returning successful zero jobs', async () => { const mockPieces = { getPieceAsset: vi.fn().mockRejectedValue(new Error('test error')), } as unknown as Pieces const logger = makeLogger() - const jobs = await generateVariantJobs( - 'path/to/file.jpg', - 'image.jpg', - mockPieces, - [100], - ['avif', 'jpg'], - logger - ) - - expect(jobs).toEqual([]) + await expect( + generateVariantJobs( + 'path/to/file.jpg', + 'image.jpg', + mockPieces, + [100], + ['avif', 'jpg'], + logger + ) + ).rejects.toThrow('test error') expect(logger.error).toHaveBeenCalledOnce() }) + test('propagates a Sharp preparation error rather than returning partial jobs', async () => { + const error = new Error('cannot prepare image') + vi.mocked(Sharp).mockImplementationOnce(() => { + throw error + }) + const mockPieces = { + getPieceAsset: vi.fn().mockResolvedValue('asset_content'), + } as unknown as Pieces + const logger = makeLogger() + await expect( + generateVariantJobs('book.md', 'image.jpg', mockPieces, [100], ['avif', 'jpg'], logger) + ).rejects.toBe(error) + expect(logger.error).toHaveBeenCalledWith( + 'error generating variant jobs for book.md asset at image.jpg', + expect.objectContaining({ error: error.message }) + ) + }) + test('should generate variant jobs for an image asset', async () => { const mockPieces = { getPieceAsset: vi.fn(() => 'asset_content') } as unknown as Pieces const logger = makeLogger() diff --git a/packages/web/worker/src/transforms/utils/variants.ts b/packages/web/worker/src/transforms/utils/variants.ts index 4d08447d..1e05a88b 100644 --- a/packages/web/worker/src/transforms/utils/variants.ts +++ b/packages/web/worker/src/transforms/utils/variants.ts @@ -39,6 +39,7 @@ async function generateVariantJobs( logger.error(`error generating variant jobs for ${filePath} asset at ${asset}`, { error: error instanceof Error ? error.message : String(error), }) + throw error } return jobs -- 2.54.0 From 95106366ddd2fa6db3c906673ba6f3c57de8c0ba Mon Sep 17 00:00:00 2001 From: eleith Date: Tue, 6 Oct 2026 12:31:35 -0700 Subject: [PATCH 2/2] [web] report partial publish failures explicitly --- .../publish-partial-failure-reporting.md | 6 + .../src/lib/server/workflow/publish.test.ts | 40 ++- apps/web/src/lib/server/workflow/publish.ts | 35 ++- .../src/lib/server/workflow/stream.test.ts | 53 +++- apps/web/src/lib/server/workflow/stream.ts | 43 +++- .../src/routes/admin/publish/+page.server.ts | 18 +- .../web/src/routes/admin/publish/+page.svelte | 119 ++++++--- .../routes/admin/publish/page.server.test.ts | 13 + .../web/src/routes/admin/publish/page.test.ts | 82 ++++++ .../src/routes/admin/publish/report.test.ts | 40 +++ apps/web/src/routes/admin/publish/report.ts | 30 +++ packages/web/jobs/src/index.ts | 2 + packages/web/jobs/src/specs.ts | 3 +- packages/web/jobs/src/types.ts | 11 +- packages/web/worker/README.md | 4 +- .../web/worker/src/workflows/publish.test.ts | 239 ++++++++++++++++-- packages/web/worker/src/workflows/publish.ts | 16 +- 17 files changed, 681 insertions(+), 73 deletions(-) create mode 100644 .changeset/publish-partial-failure-reporting.md create mode 100644 apps/web/src/routes/admin/publish/page.test.ts create mode 100644 apps/web/src/routes/admin/publish/report.test.ts create mode 100644 apps/web/src/routes/admin/publish/report.ts diff --git a/.changeset/publish-partial-failure-reporting.md b/.changeset/publish-partial-failure-reporting.md new file mode 100644 index 00000000..d67db90b --- /dev/null +++ b/.changeset/publish-partial-failure-reporting.md @@ -0,0 +1,6 @@ +--- +"@luzzle/web": patch +"@luzzle/web.worker": patch +--- + +Include failed-piece details in publish results while continuing healthy work and CDN/cache phases. Report completed batches with asset failures as partial in progress streams and show an explicit error summary instead of Published successfully, including on reload. Preserve ordinary success and legacy outcomes; encode piece links correctly. diff --git a/apps/web/src/lib/server/workflow/publish.test.ts b/apps/web/src/lib/server/workflow/publish.test.ts index 03c6d4ce..9ea69992 100644 --- a/apps/web/src/lib/server/workflow/publish.test.ts +++ b/apps/web/src/lib/server/workflow/publish.test.ts @@ -8,7 +8,12 @@ import { type WorkflowQueue } from './publish.fixture.js' import { getLatestWorkflowRun, getWorkflowRun, type WorkflowRunRow } from '@luzzle/web.jobs' -import { findInFlightPublishRun, validateAuditForPublish, parsePiecesDiff } from './publish.js' +import { + findInFlightPublishRun, + validateAuditForPublish, + parsePiecesDiff, + parsePublishFailures +} from './publish.js' vi.mock('@luzzle/web.jobs', () => ({ getLatestWorkflowRun: vi.fn(), @@ -240,6 +245,39 @@ describe('validateAuditForPublish', () => { }) }) +describe('parsePublishFailures', () => { + const failure = { filePath: 'failed.book.md', message: 'missing attachment' } + test('retains explicit failed-piece details from completed publish output', () => { + expect(parsePublishFailures(JSON.stringify({ failedPieces: [failure] }))).toEqual([failure]) + }) + test.each([ + null, + '', + 'invalid JSON', + 'null', + '"ok"', + '{}', + '{"failedPieces":null}', + '{"failedPieces":{}}' + ])('handles legacy and missing failure reports: %s', (output) => { + expect(parsePublishFailures(output)).toEqual([]) + }) + test('keeps valid failure entries without exposing extra fields or malformed entries', () => { + expect( + parsePublishFailures( + JSON.stringify({ + failedPieces: [ + { ...failure, extra: 'not a report field' }, + null, + { filePath: 42, message: 'bad' }, + { filePath: 'bad', message: null } + ] + }) + ) + ).toEqual([failure]) + }) +}) + describe('parsePiecesDiff', () => { const validDiff = { schemas: { added: ['blog'], updated: [], pruned: ['old'] }, diff --git a/apps/web/src/lib/server/workflow/publish.ts b/apps/web/src/lib/server/workflow/publish.ts index ba3f5b25..55b6fd29 100644 --- a/apps/web/src/lib/server/workflow/publish.ts +++ b/apps/web/src/lib/server/workflow/publish.ts @@ -1,5 +1,5 @@ import type { BackendSqlite } from 'openworkflow/sqlite' -import { getLatestWorkflowRun, getWorkflowRun } from '@luzzle/web.jobs' +import { getLatestWorkflowRun, getWorkflowRun, type PublishPieceFailure } from '@luzzle/web.jobs' import type { PiecesDiff } from '@luzzle/core' const COMPLETED_STATES = new Set(['completed', 'succeeded']) @@ -65,6 +65,39 @@ export async function validateAuditForPublish( return { ok: true } } +export function parsePublishFailures(output: string | null): PublishPieceFailure[] { + if (!output) return [] + + let parsed: unknown + try { + parsed = JSON.parse(output) + } catch { + return [] + } + + if (typeof parsed !== 'object') return [] + if (parsed === null) return [] + + const entries = (parsed as Record).failedPieces + if (!Array.isArray(entries)) return [] + + const failedPieces: PublishPieceFailure[] = [] + for (const entry of entries) { + if (typeof entry !== 'object') continue + if (entry === null) continue + + const failure = entry as Record + if (typeof failure.filePath !== 'string') continue + if (typeof failure.message !== 'string') continue + + failedPieces.push({ + filePath: failure.filePath, + message: failure.message + }) + } + return failedPieces +} + function isStringArray(value: unknown): value is string[] { return Array.isArray(value) && value.every((item) => typeof item === 'string') } diff --git a/apps/web/src/lib/server/workflow/stream.test.ts b/apps/web/src/lib/server/workflow/stream.test.ts index 75a07cd8..7f26a861 100644 --- a/apps/web/src/lib/server/workflow/stream.test.ts +++ b/apps/web/src/lib/server/workflow/stream.test.ts @@ -124,11 +124,11 @@ describe('streamJobProgress', () => { 'x-accel-buffering': 'no' }) expect(await readChunks(response)).toEqual([ - 'event: state\ndata: {"state":"completed","result":"ok","errors":null}\n\n', + 'event: state\ndata: {"state":"completed","result":"ok","errors":null,"workflowName":"Publish"}\n\n', phaseFrame, 'event: log\ndata: [{"job_id":"job-1","phase":"sync","line_number":3,"message":"café\\n🧩"}]\n\n', 'event: cursor\nid: {"sync":3,"other":8}\ndata: {"sync":3,"other":8}\n\n', - 'event: done\ndata: {"state":"completed","result":"ok","errors":null}\n\n' + 'event: done\ndata: {"state":"completed","result":"ok","errors":null,"workflowName":"Publish"}\n\n' ]) expect(mocks.selectFrom).toHaveBeenCalledWith('job_progress_logs') expect(mocks.query.where.mock.calls).toEqual([ @@ -176,7 +176,7 @@ describe('streamJobProgress', () => { 'event: done' ]) expect(chunks[0]).toBe( - 'event: state\ndata: {"state":"running","result":null,"errors":null}\n\n' + 'event: state\ndata: {"state":"running","result":null,"errors":null,"workflowName":"Publish"}\n\n' ) }) @@ -188,7 +188,7 @@ describe('streamJobProgress', () => { mocks.query.execute.mockReturnValueOnce(pending.promise) const reader = openStream().body!.getReader() expect(await readFrame(reader)).toBe( - 'event: state\ndata: {"state":"completed","result":"ok","errors":null}\n\n' + 'event: state\ndata: {"state":"completed","result":"ok","errors":null,"workflowName":"Publish"}\n\n' ) expect(await readFrame(reader)).toBe(phaseFrame) const enqueue = vi.spyOn(ReadableStreamDefaultController.prototype, 'enqueue') @@ -206,7 +206,7 @@ describe('streamJobProgress', () => { pending.resolve([]) expect(await readFrame(reader)).toBe( - 'event: done\ndata: {"state":"completed","result":"ok","errors":null}\n\n' + 'event: done\ndata: {"state":"completed","result":"ok","errors":null,"workflowName":"Publish"}\n\n' ) expect(await reader.read()).toEqual({ done: true, value: undefined }) expect(vi.getTimerCount()).toBe(0) @@ -399,7 +399,7 @@ describe('streamJobProgress', () => { ])('preserves terminal state mapping for %s', async (status, state, result) => { mocks.getWorkflowRun.mockReturnValue({ ...run, status, error: 'detail' }) const chunks = await readChunks(openStream({ jobClass: ['PublishAudit', 'Publish'] })) - const data = `{"state":"${state}","result":${result},"errors":["detail"]}` + const data = `{"state":"${state}","result":${result},"errors":["detail"],"workflowName":"Publish"}` expect(chunks).toEqual([ `event: state\ndata: ${data}\n\n`, phaseFrame, @@ -407,6 +407,47 @@ describe('streamJobProgress', () => { ]) }) + test('reports a completed publish with failed pieces as partial, never as ok', async () => { + const failedPieces = [{ filePath: 'failed.book.md', message: 'missing attachment' }] + mocks.getWorkflowRun.mockResolvedValue({ ...run, output: JSON.stringify({ failedPieces }) }) + const chunks = await readChunks(openStream()) + const outcome = JSON.stringify({ + state: 'completed', + result: 'partial', + errors: null, + workflowName: 'Publish', + failedPieces + }) + expect(chunks).toEqual([ + `event: state\ndata: ${outcome}\n\n`, + phaseFrame, + `event: done\ndata: ${outcome}\n\n` + ]) + expect(chunks.join('')).not.toContain('"result":"ok"') + }) + + test.each(['Preview', 'PublishAudit'])( + 'keeps non-publish completion semantics unchanged for %s', + async (workflow_name) => { + mocks.getWorkflowRun.mockResolvedValue({ + ...run, + workflow_name, + output: JSON.stringify({ + failedPieces: [{ filePath: 'ignored.book.md', message: 'not a publish result' }] + }) + }) + const chunks = await readChunks(openStream({ jobClass: workflow_name })) + const outcome = { + state: 'completed', + result: 'ok', + errors: null, + ...(workflow_name === 'PublishAudit' ? { workflowName: workflow_name } : {}) + } + expect(chunks[0]).toBe(`event: state\ndata: ${JSON.stringify(outcome)}\n\n`) + expect(chunks.join('')).not.toContain('failedPieces') + } + ) + test('closes with the existing error when the job is missing', async () => { mocks.getWorkflowRun.mockReturnValue(null) expect(await readChunks(openStream())).toEqual([ diff --git a/apps/web/src/lib/server/workflow/stream.ts b/apps/web/src/lib/server/workflow/stream.ts index f2c549da..837ea5e5 100644 --- a/apps/web/src/lib/server/workflow/stream.ts +++ b/apps/web/src/lib/server/workflow/stream.ts @@ -2,7 +2,8 @@ import { db, type JobProgressRow } from '$lib/server/database/index.js' import { createEventStream } from '../sse.js' import { WORKFLOW_POLL_INTERVAL_MS } from '../constants.js' import { getOpenWorkflowBackend } from './index.js' -import { getWorkflowRun, getStepAttempts } from '@luzzle/web.jobs' +import { getWorkflowRun, getStepAttempts, type PublishPieceFailure } from '@luzzle/web.jobs' +import { parsePublishFailures } from './publish.js' const TERMINAL_STATES = new Set(['completed', 'failed', 'canceled']) @@ -52,6 +53,14 @@ function fetchNewLogs(jobId: string, phase: string, afterLine: number) { type Emit = (event: string, data: unknown, id?: string) => void +type JobOutcome = { + state: string + result: unknown + errors: unknown + workflowName?: string + failedPieces?: PublishPieceFailure[] +} + async function pollOnce( jobId: string, jobClass: string | string[], @@ -62,6 +71,7 @@ async function pollOnce( try { let job: { class: string; state: string; result: unknown; errors: unknown } | null = null let runId: string | null = null + let failedPieces: PublishPieceFailure[] = [] // Query OpenWorkflow try { @@ -74,11 +84,23 @@ async function pollOnce( if (run.status === 'failed') state = 'failed' if (run.status === 'canceled') state = 'canceled' + let result: unknown = null + if (state === 'completed') { + result = 'ok' + if (run.workflow_name === 'Publish') { + failedPieces = parsePublishFailures(run.output) + if (failedPieces.length > 0) result = 'partial' + } + } + + let errors: unknown = null + if (run.error) errors = [run.error] + job = { class: run.workflow_name, state, - result: state === 'completed' ? 'ok' : null, - errors: run.error ? [run.error] : null + result, + errors } runId = run.id } @@ -97,7 +119,18 @@ async function pollOnce( return true } - emit('state', { state: job.state, result: job.result, errors: job.errors }) + const outcome: JobOutcome = { + state: job.state, + result: job.result, + errors: job.errors + } + if (job.class === 'Publish' || job.class === 'PublishAudit') { + outcome.workflowName = job.class + } + if (failedPieces.length > 0) { + outcome.failedPieces = failedPieces + } + emit('state', outcome) let phases: JobProgressRow[] = [] if (runId) { @@ -145,7 +178,7 @@ async function pollOnce( } if (TERMINAL_STATES.has(job.state)) { - emit('done', { state: job.state, result: job.result, errors: job.errors }) + emit('done', outcome) return true } return false diff --git a/apps/web/src/routes/admin/publish/+page.server.ts b/apps/web/src/routes/admin/publish/+page.server.ts index 349f0635..d43341c0 100644 --- a/apps/web/src/routes/admin/publish/+page.server.ts +++ b/apps/web/src/routes/admin/publish/+page.server.ts @@ -1,8 +1,13 @@ import { config } from '$lib/server/config' import { db, type JobProgressRow, type JobProgressLogsRow } from '$lib/server/database/index.js' import { getOpenWorkflowBackend } from '$lib/server/workflow/index.js' -import { parsePiecesDiff } from '$lib/server/workflow/publish.js' -import { getLatestWorkflowRun, getStepAttempts, type WorkflowRunRow } from '@luzzle/web.jobs' +import { parsePiecesDiff, parsePublishFailures } from '$lib/server/workflow/publish.js' +import { + getLatestWorkflowRun, + getStepAttempts, + type WorkflowRunRow, + type PublishPieceFailure +} from '@luzzle/web.jobs' import type { PiecesDiff } from '@luzzle/core' import type { PageServerLoad } from './$types' @@ -13,6 +18,7 @@ export type RunView = { phases: JobProgressRow[] logs: JobProgressLogsRow[] diff: PiecesDiff | null + failedPieces: PublishPieceFailure[] } function mapState(status: string): string { @@ -57,13 +63,19 @@ async function buildRunView(run: WorkflowRunRow | null): Promise console.error('Failed to load run progress in publish loader:', err) } + let failedPieces: PublishPieceFailure[] = [] + if (run.workflow_name === 'Publish') { + failedPieces = parsePublishFailures(run.output) + } + return { jobId: run.id, state: mapState(run.status), errors: run.error ? [run.error] : null, phases, logs, - diff: parsePiecesDiff(run.output) + diff: parsePiecesDiff(run.output), + failedPieces } } diff --git a/apps/web/src/routes/admin/publish/+page.svelte b/apps/web/src/routes/admin/publish/+page.svelte index bf908f8b..667dae3d 100644 --- a/apps/web/src/routes/admin/publish/+page.svelte +++ b/apps/web/src/routes/admin/publish/+page.svelte @@ -15,13 +15,14 @@ import LockSimpleOpenFill from 'virtual:icons/ph/lock-simple-open-fill' import type { PiecesDiff } from '@luzzle/core' + import type { PublishPieceFailure } from '@luzzle/web.jobs' import type { PageData } from './$types' import type { RunView } from './+page.server' + import { editorHref, liveHref, getRunKind, latestPhases, type ActiveKind } from './report.js' let { data }: { data: PageData } = $props() type RunStatus = 'idle' | 'enqueued' | 'running' | 'completed' | 'failed' - type ActiveKind = 'audit' | 'publish' type PhaseProgress = { phase: string @@ -71,6 +72,9 @@ run && (run.state === 'running' || run.state === 'waiting') if (inFlight(data.publish)) return { kind: 'publish', run: data.publish as RunView } if (inFlight(data.audit)) return { kind: 'audit', run: data.audit as RunView } + if (data.publish?.state === 'completed' && data.publish.failedPieces?.length) { + return { kind: 'publish', run: data.publish } + } return null } const initial = pickInitial() @@ -83,11 +87,21 @@ let logs = $state>(groupLogs((initial?.run.logs as PhaseLog[]) ?? [])) let scrollLocks = $state>({}) let syncRemote = $state(false) + let streamedFailures = $state([]) let eventSource: EventSource | null = null let auditDiff = $derived(data.audit?.jobId === jobId ? data.audit.diff : null) let publishDiff = $derived(data.publish?.jobId === jobId ? data.publish.diff : null) + let publishFailures = $derived.by(() => { + if (activeKind !== 'publish') return [] + if (streamedFailures.length > 0) return streamedFailures + + const storedPublish = data.publish + if (!storedPublish) return [] + if (storedPublish.jobId !== jobId) return [] + return storedPublish.failedPieces ?? [] + }) let auditRunId = $derived(data.audit?.jobId ?? '') // Only treat an audit as live "pending changes" if it ran in this session // (or was resumed in-flight). A stored audit from a past visit is stale — @@ -96,20 +110,6 @@ let auditedThisSession = $derived(activeKind === 'audit' && status === 'completed') let busy = $derived(status === 'running' || status === 'enqueued') - let reportTitle = $derived.by(() => { - if (status === 'idle') { - return 'Publish changes' - } - if (status === 'enqueued' || status === 'running') { - return activeKind === 'publish' ? 'Publishing...' : 'Checking for changes...' - } - if (status === 'completed') { - return activeKind === 'publish' ? 'Published successfully' : 'Pending changes' - } - // status === 'failed' - return activeKind === 'publish' ? 'Publish failed' : 'Check failed' - }) - function hasChanges(diff: PiecesDiff): boolean { return [ diff.schemas.added, @@ -124,12 +124,6 @@ // Pending changes link to the editor; published changes redirect to the live // piece (the /live route resolves the file path to its public URL on click). // Pruned pieces have no target — rendered as plain text. - function editorHref(file: string): string { - return `/admin/piece/${file}/source` - } - function liveHref(file: string): string { - return `/admin/piece/${file}/live` - } function noHref(): null { return null } @@ -156,6 +150,7 @@ function resetLive() { errorMessage = '' + streamedFailures = [] jobId = '' phases = [] logs = {} @@ -169,6 +164,7 @@ eventSource.addEventListener('state', (e) => { const payload = JSON.parse(e.data) + activeKind = getRunKind(payload.workflowName, activeKind) if (payload.state === 'running' || payload.state === 'claimed') status = 'running' else if (payload.state === 'waiting') status = 'enqueued' }) @@ -197,7 +193,14 @@ eventSource.addEventListener('done', async (e) => { const payload = JSON.parse(e.data) - const finalStatus = payload.state === 'completed' ? 'completed' : 'failed' + activeKind = getRunKind(payload.workflowName, activeKind) + let finalStatus: RunStatus = 'failed' + if (payload.state === 'completed') finalStatus = 'completed' + + streamedFailures = [] + if (Array.isArray(payload.failedPieces)) { + streamedFailures = payload.failedPieces + } if (payload.errors && payload.errors.length > 0) { errorMessage = payload.errors[0]?.message || 'Unknown error' } @@ -268,8 +271,9 @@ } onMount(() => { - if (initial) startWatching(initial.run.jobId) - else if (page.url.searchParams.has('check')) getChanges() + if (initial && (initial.run.state === 'running' || initial.run.state === 'waiting')) { + startWatching(initial.run.jobId) + } else if (page.url.searchParams.has('check')) getChanges() }) onDestroy(() => { @@ -284,9 +288,9 @@ const end = last.finished_at || Date.now() return end - first.started_at }) - let completedStages = $derived(phases.filter((p) => p.status === 'completed').length) - let totalStages = $derived(phases.length) - let activeLabel = $derived(activeKind === 'publish' ? 'Publishing' : 'Checking for changes') + let timelinePhases = $derived(latestPhases(phases)) + let completedStages = $derived(timelinePhases.filter((p) => p.status === 'completed').length) + let totalStages = $derived(timelinePhases.length)
@@ -303,7 +307,31 @@
-

{reportTitle}

+

+ {#if status === 'idle'} + Publish changes + {:else if busy} + {#if activeKind === 'publish'} + Publishing... + {:else} + Checking for changes... + {/if} + {:else if status === 'completed'} + {#if activeKind === 'publish'} + {#if publishFailures.length > 0} + Publish finished with errors + {:else} + Published successfully + {/if} + {:else} + Pending changes + {/if} + {:else if activeKind === 'publish'} + Publish failed + {:else} + Check failed + {/if} +

{#if phases.length > 0 && !busy} {#if status === 'completed' && activeKind === 'publish'} @@ -328,6 +356,24 @@
{/if} {:else if status === 'completed'} + {#if activeKind === 'publish' && publishFailures.length > 0} + + {/if} {#if activeKind === 'publish' && publishDiff} {@render changeList(publishDiff, liveHref)} {:else if activeKind === 'audit' && auditDiff} @@ -386,13 +432,17 @@
Enqueued
Waiting for worker…
{:else if status === 'running'} -
{activeLabel}
+
+ {#if activeKind === 'publish'}Publishing{:else}Checking for changes{/if} +
{completedStages}/{totalStages} stages · {formatDuration(overallDuration)} elapsed
{:else} -
{activeLabel} failed
+
+ {#if activeKind === 'publish'}Publishing failed{:else}Checking for changes failed{/if} +
{completedStages}/{totalStages} stages · {formatDuration(overallDuration)}
@@ -404,7 +454,7 @@ {#if phases.length > 0}
- {#each phases as phase (phase.phase)} + {#each timelinePhases as phase (phase.phase)} {@const hasLogs = (logs[phase.phase]?.length ?? 0) > 0}
@@ -531,8 +581,13 @@ {/each} {#each pieces as file (file)} {@const href = hrefFor(file)} + {@const assetFailure = publishFailures.find((failure) => failure.filePath === file)}
  • - {#if href} + {#if assetFailure} + {file} + — asset generation failed + {:else if href} {file} {:else} {file} diff --git a/apps/web/src/routes/admin/publish/page.server.test.ts b/apps/web/src/routes/admin/publish/page.server.test.ts index 9b80a255..b10b4187 100644 --- a/apps/web/src/routes/admin/publish/page.server.test.ts +++ b/apps/web/src/routes/admin/publish/page.server.test.ts @@ -63,6 +63,7 @@ test('awaits SDK-backed run and step reads while preserving publish page data', errors: null, logs: [], diff, + failedPieces: [], phases: [ { job_id: `${name}-id`, @@ -79,6 +80,18 @@ test('awaits SDK-backed run and step reads while preserving publish page data', expect(query.where).toHaveBeenCalledWith('job_id', '=', 'Publish-id') }) +test('preserves partial publish failures alongside the source diff without treating audit as partial', async () => { + const failedPieces = [{ filePath: 'failed.book.md', message: 'missing attachment' }] + vi.mocked(getLatestWorkflowRun).mockImplementation(async (_backend, name) => ({ + ...run(name), + output: JSON.stringify({ ...diff, failedPieces }) + })) + const result = await load({} as Parameters[0]) + if (!result) throw new Error('Expected publish page data') + expect(result.publish).toMatchObject({ state: 'completed', diff, failedPieces }) + expect(result.audit).toMatchObject({ state: 'completed', diff, failedPieces: [] }) +}) + test('keeps absent run views null', async () => { vi.mocked(getLatestWorkflowRun).mockResolvedValue(null) const result = await load({} as Parameters[0]) diff --git a/apps/web/src/routes/admin/publish/page.test.ts b/apps/web/src/routes/admin/publish/page.test.ts new file mode 100644 index 00000000..a8f6679d --- /dev/null +++ b/apps/web/src/routes/admin/publish/page.test.ts @@ -0,0 +1,82 @@ +import { expect, test, vi } from 'vitest' +import { render } from 'svelte/server' +import type { ComponentProps } from 'svelte' +import Page from './+page.svelte' +import type { RunView } from './+page.server.js' + +vi.mock('$app/navigation', () => ({ invalidateAll: vi.fn() })) +vi.mock('$app/state', () => ({ page: { url: new URL('http://localhost/admin/publish') } })) + +function html( + failedPieces = [{ filePath: 'failed.book.md', message: 'missing attachment' }], + phases: RunView['phases'] = [] +) { + const data = { + meta: { title: 'Publish' }, + config: { + url: { app: 'http://localhost', app_assets: '', luzzle_assets: '' }, + content: { text: { title: 'Test', description: '' } } + }, + audit: null, + publish: { + jobId: 'publish-id', + state: 'completed', + errors: null, + phases, + logs: [], + failedPieces, + diff: { + schemas: { added: [], updated: [], pruned: [] }, + pieces: { added: ['failed.book.md', 'healthy.book.md'], updated: [], pruned: [] } + } + } + } + return render(Page, { props: { data } as ComponentProps }).body +} + +test('a completed partial publish stays visible after reload without an all-success claim', () => { + const output = html() + expect(output).toContain('Publish finished with errors') + expect(output).toContain('missing attachment') + expect(output).toContain('asset generation failed') + expect(output).toContain('/admin/piece/failed.book.md/source') + expect(output).not.toContain('/admin/piece/failed.book.md/live') + expect(output).toContain('/admin/piece/healthy.book.md/live') + expect(output).not.toContain('Published successfully') + expect(output).toContain('role="alert"') +}) + +test('failed-piece editor links encode filenames with URL delimiters', () => { + const output = html([{ filePath: 'books/chapter#1?%.book.md', message: 'missing image' }]) + expect(output).toContain('/admin/piece/books/chapter%231%3F%25.book.md/source') +}) + +test('partial reports retain one timeline row per retried phase and count stages correctly', () => { + const phase = { + job_id: 'publish-id', + phase: 'cdn.sync', + started_at: 1000, + finished_at: 2000, + message: null + } + const output = html(undefined, [ + { ...phase, status: 'failed' }, + { ...phase, status: 'completed', started_at: 2000, finished_at: 3000 } + ]) + expect(output).toContain('Publish finished with errors') + expect(output.match(/class="phase-name(?:\s[^"]*)?"/g)).toHaveLength(1) + expect(output.replace(//g, '')).toContain('1 stages') +}) + +test('ordinary completed history retains the existing idle publish screen', () => { + const output = html([]) + expect(output).toContain('Publish changes') + expect(output).not.toContain('Publish finished with errors') + expect(output).not.toContain('Asset generation failed') +}) + +test('failed-piece messages render as text rather than executable markup', () => { + const output = html([{ filePath: 'failed.book.md', message: '' }]) + expect(output).not.toContain('') + expect(output).toContain('<script') +}) diff --git a/apps/web/src/routes/admin/publish/report.test.ts b/apps/web/src/routes/admin/publish/report.test.ts new file mode 100644 index 00000000..ec5f49da --- /dev/null +++ b/apps/web/src/routes/admin/publish/report.test.ts @@ -0,0 +1,40 @@ +import { expect, test } from 'vitest' +import { getRunKind, editorHref, liveHref, latestPhases } from './report.js' + +test('the actual workflow overrides the action used to attach to an existing run', () => { + expect(getRunKind('Publish', 'audit')).toBe('publish') + expect(getRunKind('PublishAudit', 'publish')).toBe('audit') + expect(getRunKind('Publish', null)).toBe('publish') +}) + +test('legacy stream frames without workflow metadata retain their existing kind', () => { + expect(getRunKind(undefined, 'audit')).toBe('audit') + expect(getRunKind('unknown', 'publish')).toBe('publish') +}) + +test('retry attempts collapse to the latest state per phase without changing phase order', () => { + const first = { phase: 'assets.generate', status: 'completed' } + const failed = { phase: 'cdn.sync', status: 'failed' } + const retry = { phase: 'cdn.sync', status: 'completed' } + const cache = { phase: 'cache.purge', status: 'completed' } + const rows = [first, failed, retry, cache] + expect(latestPhases(rows)).toEqual([first, retry, cache]) + expect(rows).toEqual([first, failed, retry, cache]) + expect(latestPhases([])).toEqual([]) +}) + +test.each([ + 'books/chapter#1.book.md', + 'books/name?query%.book.md', + 'folder with spaces/a book.book.md' +])('piece links preserve valid special filename characters: %s', (file) => { + const encoded = file.split('/').map(encodeURIComponent).join('/') + expect(editorHref(file)).toBe(`/admin/piece/${encoded}/source`) + expect(liveHref(file)).toBe(`/admin/piece/${encoded}/live`) + for (const href of [editorHref(file), liveHref(file)]) { + const url = new URL(href, 'http://localhost') + expect(url.search).toBe('') + expect(url.hash).toBe('') + expect(decodeURIComponent(url.pathname)).toContain(file) + } +}) diff --git a/apps/web/src/routes/admin/publish/report.ts b/apps/web/src/routes/admin/publish/report.ts new file mode 100644 index 00000000..c680792c --- /dev/null +++ b/apps/web/src/routes/admin/publish/report.ts @@ -0,0 +1,30 @@ +export type ActiveKind = 'audit' | 'publish' + +export function getRunKind(workflowName: unknown, fallback: ActiveKind | null): ActiveKind | null { + if (workflowName === 'Publish') return 'publish' + if (workflowName === 'PublishAudit') return 'audit' + return fallback +} + +export function latestPhases(rows: T[]): T[] { + const latestByPhase = new Map() + for (const row of rows) { + latestByPhase.set(row.phase, row) + } + return Array.from(latestByPhase.values()) +} + +function piecePath(file: string): string { + return file + .split('/') + .map((segment) => encodeURIComponent(segment)) + .join('/') +} + +export function editorHref(file: string): string { + return `/admin/piece/${piecePath(file)}/source` +} + +export function liveHref(file: string): string { + return `/admin/piece/${piecePath(file)}/live` +} diff --git a/packages/web/jobs/src/index.ts b/packages/web/jobs/src/index.ts index f9fd5730..c476d531 100644 --- a/packages/web/jobs/src/index.ts +++ b/packages/web/jobs/src/index.ts @@ -17,6 +17,8 @@ export { export type { AssetRecord, PublishPayload, + PublishPieceFailure, + PublishResult, PublishAuditPayload, PreviewPayload, PreviewAsset, diff --git a/packages/web/jobs/src/specs.ts b/packages/web/jobs/src/specs.ts index 8c882771..52b379c4 100644 --- a/packages/web/jobs/src/specs.ts +++ b/packages/web/jobs/src/specs.ts @@ -6,6 +6,7 @@ import type { PreviewPayload, PreviewResult, PublishPayload, + PublishResult, PublishAuditPayload, TestConnectivityPayload, TestConnectivityResult, @@ -22,7 +23,7 @@ export const previewSpec = defineWorkflowSpec({ name: 'Preview', }) -export const publishSpec = defineWorkflowSpec({ +export const publishSpec = defineWorkflowSpec({ name: 'Publish', }) diff --git a/packages/web/jobs/src/types.ts b/packages/web/jobs/src/types.ts index 316fea49..4b3e81d1 100644 --- a/packages/web/jobs/src/types.ts +++ b/packages/web/jobs/src/types.ts @@ -1,4 +1,4 @@ -import type { PieceFrontmatter } from '@luzzle/core' +import type { PieceFrontmatter, PiecesDiff } from '@luzzle/core' export interface AssetRecord { piece_asset_path?: string | null @@ -14,6 +14,15 @@ export interface PublishPayload { bisync?: boolean } +export interface PublishPieceFailure { + filePath: string + message: string +} + +export interface PublishResult extends PiecesDiff { + failedPieces: PublishPieceFailure[] +} + export interface PublishAuditPayload { bisync?: boolean } diff --git a/packages/web/worker/README.md b/packages/web/worker/README.md index 10ac2cff..85addd24 100644 --- a/packages/web/worker/README.md +++ b/packages/web/worker/README.md @@ -1,7 +1,7 @@ # @luzzle/web.worker 👷 The sidecar background worker service for the Luzzle web ecosystem. The worker -pulls asynchronous job payloads off a shared SQLite queue (powered by Sidequest) +pulls asynchronous job payloads off a shared SQLite queue (powered by OpenWorkflow) and processes resource-heavy tasks off the main web thread. ## Why it exists 🤖 @@ -29,7 +29,7 @@ The worker registers and processes the following job types: from/to remotes, parse changes, build CDN asset variants, and sync CDN. 2. **`preview`** — Performs hot-path parsing of piece modifications in the editor, returning on-the-fly syntax-highlighted HTML and asset preview URLs. -3. **`job-progress-purge`** — Clean-up job to purge completed/failed Sidequest +3. **`job-progress-purge`** — Clean-up job to purge completed/failed workflow job logs based on a retention schedule. --- diff --git a/packages/web/worker/src/workflows/publish.test.ts b/packages/web/worker/src/workflows/publish.test.ts index 205706cf..4ef62e10 100644 --- a/packages/web/worker/src/workflows/publish.test.ts +++ b/packages/web/worker/src/workflows/publish.test.ts @@ -1,27 +1,230 @@ -import { describe, test, expect, beforeEach } from 'vitest' -import { registerPublishWorkflow } from './publish.js' -import { initOpenWorkflow, getOpenWorkflow } from '@luzzle/web.jobs' +import { describe, test, expect, vi, beforeEach } from 'vitest' +import type { Workflow } from 'openworkflow' +import type { PiecesDiff } from '@luzzle/core' +import type { PublishPayload, PublishResult } from '@luzzle/web.jobs' import { publishSpec } from '@luzzle/web.jobs/specs' +import type { StepResult } from '../core/step.js' +import type { WorkerContext } from '../services/context.js' +import { registerPublishWorkflow } from './publish.js' +import { emptyPiecesDiff } from './pieces-diff.js' + +type PublishWorkflow = Workflow['fn'] +type WorkflowStep = Parameters[0]['step'] + +const mocks = vi.hoisted(() => { + const jobStep = (name: string) => ({ + name, + run: vi.fn<(input: unknown) => Promise>>(), + }) + return { + implementWorkflow: vi.fn<(spec: typeof publishSpec, fn: PublishWorkflow) => void>(), + logger: { info: vi.fn(), warn: vi.fn() }, + progress: { start: vi.fn(), complete: vi.fn(), skip: vi.fn(), fail: vi.fn() }, + archiveSync: jobStep('archive.sync'), + luzzleSync: jobStep('luzzle.sync'), + webSync: jobStep('web.sync'), + assetsGenerate: jobStep('assets.generate'), + cdnSync: jobStep('cdn.sync'), + cachePurge: jobStep('cache.purge'), + } +}) + +vi.mock('@luzzle/web.jobs', () => ({ + getOpenWorkflow: () => ({ implementWorkflow: mocks.implementWorkflow }), +})) +vi.mock('../services/context.js', () => ({ + getWorkerContext: () => ({ logger: mocks.logger, db: {} }), + createPhaseContext: (ctx: WorkerContext) => ctx, +})) +vi.mock('../core/job-progress.js', () => ({ + JobProgress: class { + constructor() { + return mocks.progress + } + }, +})) +vi.mock('../steps/archive-sync.js', () => ({ archiveSyncStep: mocks.archiveSync })) +vi.mock('../steps/luzzle-sync.js', () => ({ luzzleSyncStep: mocks.luzzleSync })) +vi.mock('../steps/web-sync/index.js', () => ({ webSyncStep: mocks.webSync })) +vi.mock('../steps/assets-generate.js', () => ({ assetsGenerateStep: mocks.assetsGenerate })) +vi.mock('../steps/cdn-sync.js', () => ({ cdnSyncStep: mocks.cdnSync })) +vi.mock('../steps/cache-purge.js', () => ({ cachePurgeStep: mocks.cachePurge })) + +const summary: PiecesDiff = { + schemas: { added: ['books'], updated: [], pruned: ['old'] }, + pieces: { + added: ['healthy.books.md'], + updated: ['broken.books.md'], + pruned: ['removed.books.md'], + }, +} + +// Exercise the app's registered callback and real runProgressPhase helper, without +// the SDK's private registry or any filesystem/transform/CDN side effects. +async function runPublish(input: PublishPayload = {}, checkpoints = new Map()) { + registerPublishWorkflow() + expect(mocks.implementWorkflow).toHaveBeenCalledOnce() + const [spec, callback] = mocks.implementWorkflow.mock.calls[0]! + expect(spec).toBe(publishSpec) + + const run = vi.fn( + async ( + { name }: { name: string }, + fn: () => Promise | Output | undefined + ): Promise => { + if (checkpoints.has(name)) return checkpoints.get(name) as Output + return (await fn()) as Output + } + ) + const result = await callback({ + input, + step: { run } as unknown as WorkflowStep, + version: null, + run: { + id: 'publish-1', + workflowName: publishSpec.name, + createdAt: new Date(), + startedAt: null, + }, + }) + return { result, phases: run.mock.calls.map(([config]) => config.name) } +} + +function expectHealthyPhasesComplete() { + expect(mocks.cdnSync.run).toHaveBeenCalledOnce() + expect(mocks.cachePurge.run).toHaveBeenCalledOnce() + expect(mocks.progress.complete).toHaveBeenCalledWith('publish-1', 'cdn.sync') + expect(mocks.progress.complete).toHaveBeenCalledWith('publish-1', 'cache.purge') + expect(mocks.progress.fail).not.toHaveBeenCalled() +} + +function expectNormalCompletion() { + expect(mocks.logger.info).toHaveBeenCalledWith('openworkflow publish complete', { + jobId: 'publish-1', + }) + expect(mocks.logger.warn).not.toHaveBeenCalled() +} describe('workflows/publish', () => { beforeEach(() => { - initOpenWorkflow({ dbPath: ':memory:' }) + vi.resetAllMocks() + for (const jobStep of [ + mocks.archiveSync, + mocks.luzzleSync, + mocks.webSync, + mocks.assetsGenerate, + mocks.cdnSync, + mocks.cachePurge, + ]) { + jobStep.run.mockResolvedValue({ status: 'completed', value: undefined }) + } + mocks.luzzleSync.run.mockResolvedValue({ status: 'completed', value: summary }) + mocks.assetsGenerate.run.mockResolvedValue({ status: 'completed', value: { failedPieces: [] } }) }) - test('registers workflow implementation successfully', () => { - registerPublishWorkflow() - const ow = getOpenWorkflow() + test('reports failed pieces in JSON while publishing healthy siblings and retaining the source diff', async () => { + const failedPieces = [{ filePath: 'broken.books.md', message: 'markdown transform failed' }] + mocks.assetsGenerate.run.mockResolvedValue({ status: 'completed', value: { failedPieces } }) - // Verify the workflow is correctly registered under its name - const registered = ( - ow as unknown as { - registry: { - get: (name: string) => { spec: { name: string }; fn: (...args: unknown[]) => unknown } - } - } - ).registry.get(publishSpec.name) - expect(registered).toBeDefined() - expect(registered.spec.name).toBe(publishSpec.name) - expect(typeof registered.fn).toBe('function') + const { result, phases } = await runPublish() + + expect(phases).toEqual([ + 'luzzle.sync', + 'web.sync', + 'assets.generate', + 'cdn.sync', + 'cache.purge', + ]) + expect(mocks.webSync.run).toHaveBeenCalledWith( + { filePaths: ['healthy.books.md', 'broken.books.md'] }, + expect.anything() + ) + expect(mocks.assetsGenerate.run).toHaveBeenCalledWith( + { filePaths: ['healthy.books.md', 'broken.books.md'] }, + expect.anything() + ) + expect(JSON.parse(JSON.stringify(result))).toEqual({ ...summary, failedPieces }) + expect(result.pieces.updated).toContain('broken.books.md') + expect(summary).not.toHaveProperty('failedPieces') + expect(mocks.progress.complete).toHaveBeenCalledWith('publish-1', 'assets.generate') + expectHealthyPhasesComplete() + expect(mocks.logger.warn).toHaveBeenCalledWith('openworkflow publish complete with failures', { + jobId: 'publish-1', + failedCount: 1, + }) + expect(mocks.logger.info).not.toHaveBeenCalledWith( + 'openworkflow publish complete', + expect.anything() + ) + }) + + test.each(['cdn.sync', 'cache.purge'])( + 'keeps a global %s rejection as a real publish failure', + async (phase) => { + const error = new Error(`${phase} failed`) + const jobStep = phase === 'cdn.sync' ? mocks.cdnSync : mocks.cachePurge + jobStep.run.mockRejectedValueOnce(error) + await expect(runPublish()).rejects.toBe(error) + expect(mocks.progress.fail).toHaveBeenCalledWith('publish-1', phase, error) + if (phase === 'cdn.sync') expect(mocks.cachePurge.run).not.toHaveBeenCalled() + expect(mocks.logger.info).not.toHaveBeenCalledWith('openworkflow publish complete', expect.anything()) + expect(mocks.logger.warn).not.toHaveBeenCalled() + } + ) + + test('returns successful reports and keeps the normal completion log', async () => { + const { result } = await runPublish() + expect(result).toEqual({ ...summary, failedPieces: [] }) + expect(mocks.archiveSync.run).not.toHaveBeenCalled() + expectHealthyPhasesComplete() + expectNormalCompletion() + }) + + test('runs optional archive bisync before the remaining publish phases', async () => { + const { result, phases } = await runPublish({ bisync: true }) + expect(phases).toEqual([ + 'archive.sync', + 'luzzle.sync', + 'web.sync', + 'assets.generate', + 'cdn.sync', + 'cache.purge', + ]) + expect(result).toEqual({ ...summary, failedPieces: [] }) + expectHealthyPhasesComplete() + expectNormalCompletion() + }) + + test('returns an empty diff and report for an empty/older luzzle checkpoint', async () => { + const { result } = await runPublish({}, new Map([['luzzle.sync', undefined]])) + expect(result).toEqual({ ...emptyPiecesDiff(), failedPieces: [] }) + expect(mocks.luzzleSync.run).not.toHaveBeenCalled() + expect(mocks.webSync.run).toHaveBeenCalledWith({ filePaths: [] }, expect.anything()) + expect(mocks.assetsGenerate.run).toHaveBeenCalledWith({ filePaths: [] }, expect.anything()) + expectHealthyPhasesComplete() + expectNormalCompletion() + }) + + test.each([undefined, null, {}, { failedPieces: [] }])( + 'accepts an empty/older assets checkpoint (%j)', + async (checkpoint) => { + const { result } = await runPublish({}, new Map([['assets.generate', checkpoint]])) + expect(result).toEqual({ ...summary, failedPieces: [] }) + expect(mocks.assetsGenerate.run).not.toHaveBeenCalled() + expectHealthyPhasesComplete() + expectNormalCompletion() + } + ) + + test('preserves failed piece reports from a durable assets checkpoint', async () => { + const failedPieces = [{ filePath: 'broken.books.md', message: 'cached failure' }] + const { result } = await runPublish({}, new Map([['assets.generate', { failedPieces }]])) + expect(result).toEqual({ ...summary, failedPieces }) + expect(mocks.assetsGenerate.run).not.toHaveBeenCalled() + expectHealthyPhasesComplete() + expect(mocks.logger.warn).toHaveBeenCalledWith('openworkflow publish complete with failures', { + jobId: 'publish-1', + failedCount: 1, + }) }) }) diff --git a/packages/web/worker/src/workflows/publish.ts b/packages/web/worker/src/workflows/publish.ts index ca0ee857..8f237c6d 100644 --- a/packages/web/worker/src/workflows/publish.ts +++ b/packages/web/worker/src/workflows/publish.ts @@ -31,11 +31,21 @@ export function registerPublishWorkflow(): void { const changedPaths = [...(summary?.pieces.added ?? []), ...(summary?.pieces.updated ?? [])] await runProgressPhase(step, ctx, jobId, progress, webSyncStep, { filePaths: changedPaths }) - await runProgressPhase(step, ctx, jobId, progress, assetsGenerateStep, { filePaths: changedPaths }) + const assetsReport = await runProgressPhase(step, ctx, jobId, progress, assetsGenerateStep, { + filePaths: changedPaths, + }) await runProgressPhase(step, ctx, jobId, progress, cdnSyncStep, undefined) await runProgressPhase(step, ctx, jobId, progress, cachePurgeStep, undefined) - logger.info('openworkflow publish complete', { jobId }) - return summary ?? emptyPiecesDiff() + const failedPieces = assetsReport?.failedPieces ?? [] + if (failedPieces.length > 0) { + logger.warn('openworkflow publish complete with failures', { + jobId, + failedCount: failedPieces.length, + }) + } else { + logger.info('openworkflow publish complete', { jobId }) + } + return { ...(summary ?? emptyPiecesDiff()), failedPieces } }) } -- 2.54.0