From c4a0c55e71fecb3d33fa158fe3829d2f4a09a0b3 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Wed, 1 Apr 2026 05:25:00 -0600 Subject: [PATCH] fix(metadata): harden write pipeline with atomic writes and ordering --- server/.env.example | 4 + server/src/app.module.ts | 4 +- server/src/config/config.ts | 13 + server/src/config/env.validation.ts | 2 + .../file-write-settings.service.test.ts | 4 +- .../file-write/file-write-settings.service.ts | 16 +- .../file-write/file-write.constants.ts | 74 +++++ .../file-write/file-write.repository.test.ts | 10 +- .../file-write/file-write.repository.ts | 23 +- .../file-write/file-write.service.test.ts | 69 +++- .../modules/file-write/file-write.service.ts | 311 ++++++++++++------ .../file-write/format-writer.registry.test.ts | 6 +- .../file-write/format-writer.registry.ts | 10 +- .../formats/cbx/cb7-format-writer.ts | 101 ++++-- .../file-write/formats/cbx/cbz-zip-patcher.ts | 9 +- .../formats/cbx/comic-info-builder.ts | 41 ++- .../formats/epub/epub-cover-handler.ts | 2 +- .../formats/epub/epub-format-writer.ts | 2 +- .../formats/epub/epub-opf-builder.test.ts | 26 +- .../formats/epub/epub-opf-builder.ts | 27 +- .../formats/epub/epub-zip-patcher.ts | 9 +- .../formats/pdf/pdf-format-writer.ts | 12 +- .../file-write/formats/pdf/pdf-xmp-builder.ts | 26 +- .../shared/atomic-file-replace.test.ts | 56 ++++ .../formats/shared/atomic-file-replace.ts | 29 ++ .../modules/library/library.service.test.ts | 6 +- server/src/modules/library/library.service.ts | 2 +- 27 files changed, 630 insertions(+), 264 deletions(-) create mode 100644 server/src/modules/file-write/file-write.constants.ts create mode 100644 server/src/modules/file-write/formats/shared/atomic-file-replace.test.ts create mode 100644 server/src/modules/file-write/formats/shared/atomic-file-replace.ts diff --git a/server/.env.example b/server/.env.example index 7d8b32fb7..36df13736 100644 --- a/server/.env.example +++ b/server/.env.example @@ -22,6 +22,10 @@ BOOKS_PATH=../local/data # Defaults to {BOOKS_PATH}/staging if unset. # STAGING_PATH=../local/staging +# File write scheduler and concurrency tuning (optional) +# FILE_WRITE_DEBOUNCE_MS=3000 +# FILE_WRITE_MAX_CONCURRENT_WRITES=2 + # OIDC (optional — configure via admin settings UI; these are just for reference) # OIDC config is stored in the database (app_settings table, key: oidc_config) diff --git a/server/src/app.module.ts b/server/src/app.module.ts index db6c09adb..b3cbe36dd 100644 --- a/server/src/app.module.ts +++ b/server/src/app.module.ts @@ -4,7 +4,7 @@ import { APP_GUARD, APP_INTERCEPTOR } from '@nestjs/core'; import { ScheduleModule } from '@nestjs/schedule'; import { LoggerModule } from 'nestjs-pino'; -import { appConfig, authConfig, dbConfig, emailConfig, storageConfig } from './config/config'; +import { appConfig, authConfig, dbConfig, emailConfig, fileWriteConfig, storageConfig } from './config/config'; import { validateEnv } from './config/env.validation'; import { loggerConfig } from './common/logger.config'; import { CommonModule } from './common/common.module'; @@ -57,7 +57,7 @@ import { AuditModule } from './modules/audit/audit.module'; ConfigModule.forRoot({ isGlobal: true, validate: validateEnv, - load: [appConfig, dbConfig, authConfig, storageConfig, emailConfig], + load: [appConfig, dbConfig, authConfig, storageConfig, fileWriteConfig, emailConfig], }), ScheduleModule.forRoot(), DbModule, diff --git a/server/src/config/config.ts b/server/src/config/config.ts index 6198707c4..b4f9fcf2c 100644 --- a/server/src/config/config.ts +++ b/server/src/config/config.ts @@ -21,6 +21,19 @@ export const storageConfig = registerAs('storage', () => ({ stagingPath: process.env.STAGING_PATH, })); +export const fileWriteConfig = registerAs('fileWrite', () => ({ + debounceMs: parsePositiveInteger(process.env.FILE_WRITE_DEBOUNCE_MS, 3_000), + maxConcurrentWrites: parsePositiveInteger(process.env.FILE_WRITE_MAX_CONCURRENT_WRITES, 2), +})); + export const emailConfig = registerAs('email', () => ({ encryptionKey: process.env.EMAIL_ENCRYPTION_KEY ?? '', })); + +function parsePositiveInteger(value: string | undefined, fallback: number): number { + const parsed = Number(value); + if (!Number.isFinite(parsed) || parsed < 1) { + return fallback; + } + return Math.floor(parsed); +} diff --git a/server/src/config/env.validation.ts b/server/src/config/env.validation.ts index 5d44d251a..6422ba308 100644 --- a/server/src/config/env.validation.ts +++ b/server/src/config/env.validation.ts @@ -15,6 +15,8 @@ const envSchema = z.object({ SETUP_BOOTSTRAP_TOKEN: z.string().optional(), BOOKS_PATH: z.string().default('/data'), STAGING_PATH: z.string().optional(), + FILE_WRITE_DEBOUNCE_MS: z.coerce.number().int().positive().optional(), + FILE_WRITE_MAX_CONCURRENT_WRITES: z.coerce.number().int().positive().optional(), CLIENT_URL: z.string().url().optional(), APP_URL: z.string().url().default('http://localhost:5173'), EMAIL_ENCRYPTION_KEY: z.string().optional(), diff --git a/server/src/modules/file-write/file-write-settings.service.test.ts b/server/src/modules/file-write/file-write-settings.service.test.ts index f2d7fb3d5..aabe9ea29 100644 --- a/server/src/modules/file-write/file-write-settings.service.test.ts +++ b/server/src/modules/file-write/file-write-settings.service.test.ts @@ -81,13 +81,13 @@ describe('FileWriteSettingsService', () => { expect(db.__chains.insertChain.onConflictDoUpdate).toHaveBeenCalledTimes(1); }); - it('resolve currently delegates to global settings', async () => { + it('resolveForLibrary currently delegates to global settings', async () => { const db = makeDb(); db.__chains.findFirst.mockResolvedValue({ value: JSON.stringify({ enabled: true }) }); const service = new FileWriteSettingsService(db as never); - const result = await service.resolve(999); + const result = await service.resolveForLibrary(999); expect(result.enabled).toBe(true); }); }); diff --git a/server/src/modules/file-write/file-write-settings.service.ts b/server/src/modules/file-write/file-write-settings.service.ts index c28a900fa..1125a78b9 100644 --- a/server/src/modules/file-write/file-write-settings.service.ts +++ b/server/src/modules/file-write/file-write-settings.service.ts @@ -1,4 +1,4 @@ -import { Inject, Injectable } from '@nestjs/common'; +import { Inject, Injectable, Logger } from '@nestjs/common'; import { eq } from 'drizzle-orm'; import { NodePgDatabase } from 'drizzle-orm/node-postgres'; @@ -13,11 +13,12 @@ type Db = NodePgDatabase; @Injectable() export class FileWriteSettingsService { + private readonly logger = new Logger(FileWriteSettingsService.name); + constructor(@Inject(DB) private readonly db: Db) {} - // libraryId reserved for Phase 2 per-library overrides - // eslint-disable-next-line @typescript-eslint/no-unused-vars - async resolve(_libraryId: number): Promise { + async resolveForLibrary(libraryId: number): Promise { + void libraryId; return this.getGlobal(); } @@ -29,7 +30,12 @@ export class FileWriteSettingsService { try { const stored = JSON.parse(row.value) as Partial; return mergeSettings(DEFAULT_FILE_WRITE_SETTINGS, stored); - } catch { + } catch (error) { + const errorClass = error instanceof Error ? error.name : 'Error'; + const errorMessage = (error instanceof Error ? error.message : String(error)).replace(/"/g, '\\"'); + this.logger.warn( + `[file_write.settings] [fail] key=${FILE_WRITE_SETTINGS_KEY} errorClass=${errorClass} error="${errorMessage}" - failed to parse file write settings, using defaults`, + ); return { ...DEFAULT_FILE_WRITE_SETTINGS }; } } diff --git a/server/src/modules/file-write/file-write.constants.ts b/server/src/modules/file-write/file-write.constants.ts new file mode 100644 index 000000000..2c0e0930e --- /dev/null +++ b/server/src/modules/file-write/file-write.constants.ts @@ -0,0 +1,74 @@ +import type { BookWritePayloadKey } from './interfaces/book-write-payload.interface'; + +export const FORMAT_EPUB = 'epub'; +export const FORMAT_PDF = 'pdf'; +export const FORMAT_CBZ = 'cbz'; +export const FORMAT_CB7 = 'cb7'; + +export const CBX_FORMATS = [FORMAT_CBZ, FORMAT_CB7] as const; + +export const BOOK_WRITE_FIELD_KEYS: readonly BookWritePayloadKey[] = [ + 'title', + 'subtitle', + 'description', + 'publisher', + 'publishedYear', + 'language', + 'pageCount', + 'seriesName', + 'seriesIndex', + 'isbn10', + 'isbn13', + 'rating', + 'authors', + 'genres', + 'tags', + 'googleBooksId', + 'goodreadsId', + 'amazonId', + 'hardcoverId', + 'openLibraryId', + 'itunesId', + 'coverBytes', +]; + +export function createBookWriteFieldMask(): Set { + return new Set(BOOK_WRITE_FIELD_KEYS); +} + +export const COMIC_INFO_PROVIDER_ID_KEYS = [ + 'goodreadsId', + 'amazonId', + 'hardcoverId', + 'googleBooksId', + 'openLibraryId', +] as const satisfies readonly BookWritePayloadKey[]; + +export const COMIC_INFO_MANAGED_NOTES_KEYS = [ + 'subtitle', + 'isbn10', + 'goodreadsId', + 'amazonId', + 'hardcoverId', + 'googleBooksId', + 'openLibraryId', +] as const satisfies readonly BookWritePayloadKey[]; + +type ComicInfoProviderKey = (typeof COMIC_INFO_PROVIDER_ID_KEYS)[number]; + +export const COMIC_INFO_PROVIDER_WEB_URL_BUILDERS: Record string> = { + goodreadsId: (id: string) => `https://www.goodreads.com/book/show/${id}`, + amazonId: (id: string) => `https://www.amazon.com/dp/${id}`, + hardcoverId: (id: string) => `https://hardcover.app/books/${id}`, + googleBooksId: (id: string) => `https://books.google.com/books?id=${id}`, + openLibraryId: (id: string) => `https://openlibrary.org/works/${id}`, +}; + +export const EPUB_PROVIDER_IDENTIFIER_PREFIXES = { + goodreadsId: 'urn:goodreads:', + amazonId: 'urn:amazon:', + hardcoverId: 'urn:hardcover:', + googleBooksId: 'urn:google:', + openLibraryId: 'urn:openlibrary:', + itunesId: 'urn:itunes:', +} as const satisfies Partial>; diff --git a/server/src/modules/file-write/file-write.repository.test.ts b/server/src/modules/file-write/file-write.repository.test.ts index 40db8c7d6..c743a6fb5 100644 --- a/server/src/modules/file-write/file-write.repository.test.ts +++ b/server/src/modules/file-write/file-write.repository.test.ts @@ -66,12 +66,14 @@ describe('FileWriteRepository', () => { const genreChain = { from: vi.fn().mockReturnThis(), innerJoin: vi.fn().mockReturnThis(), - where: vi.fn().mockResolvedValue([{ name: 'Sci-Fi' }]), + where: vi.fn().mockReturnThis(), + orderBy: vi.fn().mockResolvedValue([{ name: 'Sci-Fi' }]), }; const tagChain = { from: vi.fn().mockReturnThis(), innerJoin: vi.fn().mockReturnThis(), - where: vi.fn().mockResolvedValue([{ name: 'Classic' }]), + where: vi.fn().mockReturnThis(), + orderBy: vi.fn().mockResolvedValue([{ name: 'Classic' }]), }; const db = { @@ -132,7 +134,7 @@ describe('FileWriteRepository', () => { id: 8, format: 'epub', status: 'success', - fieldsWritten: null, + fieldsWritten: ['title', 123, 'genres'], triggeredBy: 'auto', writtenAt: rowDate, durationMs: 20, @@ -149,7 +151,7 @@ describe('FileWriteRepository', () => { id: 8, format: 'epub', status: 'success', - fieldsWritten: [], + fieldsWritten: ['title', 'genres'], triggeredBy: 'auto', writtenAt: rowDate.toISOString(), durationMs: 20, diff --git a/server/src/modules/file-write/file-write.repository.ts b/server/src/modules/file-write/file-write.repository.ts index 9b365d747..2041e449d 100644 --- a/server/src/modules/file-write/file-write.repository.ts +++ b/server/src/modules/file-write/file-write.repository.ts @@ -1,5 +1,5 @@ import { Inject, Injectable } from '@nestjs/common'; -import { and, desc, eq, ne } from 'drizzle-orm'; +import { and, asc, desc, eq, ne } from 'drizzle-orm'; import { NodePgDatabase } from 'drizzle-orm/node-postgres'; import type { WriteResult, WriteLogEntry } from '@projectx/types'; @@ -29,7 +29,7 @@ export class FileWriteRepository { return row ?? null; } - async findNonMissingBookFilesByLibrary(libraryId: number) { + async findNonMissingPrimaryFilesByLibrary(libraryId: number) { return this.db .select({ bookId: books.id, @@ -58,8 +58,14 @@ export class FileWriteRepository { .select({ name: genres.name }) .from(bookGenres) .innerJoin(genres, eq(genres.id, bookGenres.genreId)) - .where(eq(bookGenres.bookId, bookId)), - this.db.select({ name: tags.name }).from(bookTags).innerJoin(tags, eq(tags.id, bookTags.tagId)).where(eq(bookTags.bookId, bookId)), + .where(eq(bookGenres.bookId, bookId)) + .orderBy(asc(genres.name)), + this.db + .select({ name: tags.name }) + .from(bookTags) + .innerJoin(tags, eq(tags.id, bookTags.tagId)) + .where(eq(bookTags.bookId, bookId)) + .orderBy(asc(tags.name)), ]); return { @@ -119,7 +125,7 @@ export class FileWriteRepository { id: r.id, format: r.format, status: r.status, - fieldsWritten: (r.fieldsWritten as string[]) ?? [], + fieldsWritten: normalizeFieldsWritten(r.fieldsWritten), triggeredBy: r.triggeredBy, writtenAt: r.writtenAt.toISOString(), durationMs: r.durationMs, @@ -127,3 +133,10 @@ export class FileWriteRepository { })); } } + +function normalizeFieldsWritten(value: unknown): string[] { + if (!Array.isArray(value)) { + return []; + } + return value.filter((item): item is string => typeof item === 'string'); +} diff --git a/server/src/modules/file-write/file-write.service.test.ts b/server/src/modules/file-write/file-write.service.test.ts index 6a56c44e5..528263b08 100644 --- a/server/src/modules/file-write/file-write.service.test.ts +++ b/server/src/modules/file-write/file-write.service.test.ts @@ -18,7 +18,7 @@ const mockReaddir = readdir as MockedFunction; const mockReadFile = readFile as MockedFunction; describe('FileWriteService', () => { - function makeService() { + function makeService(configValues: Record = {}) { const fileWriteRepo = { findPrimaryFileForBook: vi.fn(), loadPayload: vi.fn(), @@ -26,7 +26,7 @@ describe('FileWriteService', () => { setLastWrittenAt: vi.fn().mockResolvedValue(undefined), }; const settingsService = { - resolve: vi.fn().mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: true }), + resolveForLibrary: vi.fn().mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: true }), }; const writer = { write: vi.fn(), @@ -39,7 +39,7 @@ describe('FileWriteService', () => { withLock: vi.fn().mockImplementation(async (_path: string, fn: () => Promise) => fn()), }; const config = { - get: vi.fn().mockImplementation((key: string) => (key === 'storage.booksPath' ? '/books' : undefined)), + get: vi.fn().mockImplementation((key: string) => (key === 'storage.booksPath' ? '/books' : configValues[key])), } as unknown as ConfigService; const service = new FileWriteService(fileWriteRepo as never, settingsService as never, registry as never, lockService as never, config); @@ -100,7 +100,7 @@ describe('FileWriteService', () => { sizeBytes: 10, libraryId: 2, }); - settingsService.resolve.mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: false }); + settingsService.resolveForLibrary.mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: false }); const result = await service.writeToFile(10, 'auto'); @@ -117,7 +117,7 @@ describe('FileWriteService', () => { sizeBytes: 500, libraryId: 2, }); - settingsService.resolve.mockResolvedValue({ + settingsService.resolveForLibrary.mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: true, pdf: { enabled: true, maxFileSizeBytes: 100 }, @@ -146,7 +146,7 @@ describe('FileWriteService', () => { libraryId: 2, }); fileWriteRepo.loadPayload.mockResolvedValue({ title: 'Dune', authors: [{ name: 'Frank Herbert', sortName: null }] }); - settingsService.resolve.mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: true, writeCover: true }); + settingsService.resolveForLibrary.mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: true, writeCover: true }); const coverBytes = Buffer.from('cover'); mockReaddir.mockResolvedValue(['cover_extracted.jpg', 'cover_custom.png'] as never); @@ -179,7 +179,7 @@ describe('FileWriteService', () => { libraryId: 2, }); fileWriteRepo.loadPayload.mockResolvedValue({ title: 'Dune' }); - settingsService.resolve.mockResolvedValue({ + settingsService.resolveForLibrary.mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: true, cbx: { enabled: true, formats: ['cbz', 'cb7'], maxFileSizeBytes: 1_000_000 }, @@ -209,7 +209,7 @@ describe('FileWriteService', () => { libraryId: 2, }); fileWriteRepo.loadPayload.mockResolvedValue({ title: 'Dune' }); - settingsService.resolve.mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: false, writeCover: true }); + settingsService.resolveForLibrary.mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: false, writeCover: true }); writer.write.mockResolvedValue({ status: 'skipped', fieldsWritten: ['title'], durationMs: 0, reason: 'dry-run' }); const result = await service.writeToFile(5, 'auto', undefined, true); @@ -250,4 +250,57 @@ describe('FileWriteService', () => { expect(spy).toHaveBeenCalledTimes(2); vi.useRealTimers(); }); + + it('uses configured debounce duration', async () => { + vi.useFakeTimers(); + const { service } = makeService({ 'fileWrite.debounceMs': 1_000 }); + const spy = vi.spyOn(service, 'writeToFile').mockResolvedValue({ status: 'success', fieldsWritten: [], durationMs: 1 }); + + service.scheduleWrite(21, 'auto'); + vi.advanceTimersByTime(999); + expect(spy).not.toHaveBeenCalled(); + + vi.advanceTimersByTime(1); + await Promise.resolve(); + expect(spy).toHaveBeenCalledTimes(1); + vi.useRealTimers(); + }); + + it('limits concurrent writes using configured max write slots', async () => { + const { service, fileWriteRepo, settingsService, writer } = makeService({ 'fileWrite.maxConcurrentWrites': 1 }); + fileWriteRepo.findPrimaryFileForBook.mockImplementation((bookId: number) => ({ + id: bookId, + absolutePath: `/books/${bookId}.epub`, + format: 'epub', + sizeBytes: 40, + libraryId: 2, + })); + fileWriteRepo.loadPayload.mockResolvedValue({ title: 'Queued write' }); + settingsService.resolveForLibrary.mockResolvedValue({ ...DEFAULT_FILE_WRITE_SETTINGS, enabled: true, writeCover: false }); + + const order: string[] = []; + let releaseFirst!: () => void; + const firstGate = new Promise((resolve) => { + releaseFirst = resolve; + }); + + writer.write.mockImplementation(async (path: string) => { + order.push(`start:${path}`); + if (path === '/books/1.epub') { + await firstGate; + } + order.push(`end:${path}`); + return { status: 'success', fieldsWritten: ['title'], durationMs: 1 }; + }); + + const first = service.writeToFile(1, 'auto'); + const second = service.writeToFile(2, 'auto'); + + await vi.waitFor(() => expect(writer.write).toHaveBeenCalledTimes(1)); + + releaseFirst(); + await Promise.all([first, second]); + + expect(order).toEqual(['start:/books/1.epub', 'end:/books/1.epub', 'start:/books/2.epub', 'end:/books/2.epub']); + }); }); diff --git a/server/src/modules/file-write/file-write.service.ts b/server/src/modules/file-write/file-write.service.ts index db564e206..d766e65f3 100644 --- a/server/src/modules/file-write/file-write.service.ts +++ b/server/src/modules/file-write/file-write.service.ts @@ -4,42 +4,29 @@ import { readdir, readFile } from 'fs/promises'; import { join } from 'path'; import type { WriteResult, GlobalFileWriteSettings } from '@projectx/types'; +import { CBX_FORMATS, FORMAT_CB7, FORMAT_CBZ, FORMAT_EPUB, FORMAT_PDF, createBookWriteFieldMask } from './file-write.constants'; import { FileLockService } from './file-lock.service'; import { FileWriteRepository } from './file-write.repository'; import { FileWriteSettingsService } from './file-write-settings.service'; import { FormatWriterRegistry } from './format-writer.registry'; -import type { BookWritePayload, BookWritePayloadKey } from './interfaces/book-write-payload.interface'; +import type { BookWritePayload } from './interfaces/book-write-payload.interface'; -const ALL_FIELDS = new Set([ - 'title', - 'subtitle', - 'description', - 'publisher', - 'publishedYear', - 'language', - 'pageCount', - 'seriesName', - 'seriesIndex', - 'isbn10', - 'isbn13', - 'rating', - 'authors', - 'genres', - 'tags', - 'googleBooksId', - 'goodreadsId', - 'amazonId', - 'hardcoverId', - 'openLibraryId', - 'itunesId', - 'coverBytes', -]); +const FILE_WRITE_EVENT = 'file_write.write'; +const FILE_WRITE_SCHEDULE_EVENT = 'file_write.schedule'; +const FILE_WRITE_COVER_EVENT = 'file_write.cover_load'; +const UNKNOWN_FORMAT = 'unknown'; +const DEFAULT_WRITE_DEBOUNCE_MS = 3_000; +const DEFAULT_MAX_CONCURRENT_WRITES = 2; @Injectable() export class FileWriteService implements OnModuleDestroy { private readonly logger = new Logger(FileWriteService.name); private readonly booksPath: string; + private readonly debounceMs: number; + private readonly maxConcurrentWrites: number; private readonly debounceMap = new Map(); + private readonly writeQueue: Array<() => void> = []; + private activeWrites = 0; constructor( private readonly fileWriteRepo: FileWriteRepository, @@ -49,137 +36,255 @@ export class FileWriteService implements OnModuleDestroy { private readonly config: ConfigService, ) { this.booksPath = this.config.get('storage.booksPath')!; + this.debounceMs = resolvePositiveInteger(this.config.get('fileWrite.debounceMs'), DEFAULT_WRITE_DEBOUNCE_MS); + this.maxConcurrentWrites = resolvePositiveInteger(this.config.get('fileWrite.maxConcurrentWrites'), DEFAULT_MAX_CONCURRENT_WRITES); } scheduleWrite(bookId: number, triggeredBy: 'auto' | 'sync', userId?: number): void { const existing = this.debounceMap.get(bookId); if (existing) clearTimeout(existing); + + this.logger.debug( + `[${FILE_WRITE_SCHEDULE_EVENT}] [start] bookId=${bookId} triggeredBy=${triggeredBy} userId=${formatUserId(userId)} debounceMs=${this.debounceMs} - scheduled file write queued`, + ); + const timer = setTimeout(() => { this.debounceMap.delete(bookId); - this.logger.debug(`Scheduled write firing for book ${bookId} (${triggeredBy})`); - this.writeToFile(bookId, triggeredBy, userId).catch((err: Error) => - this.logger.warn(`Background write failed for book ${bookId}: ${err.message}`), + this.logger.debug( + `[${FILE_WRITE_SCHEDULE_EVENT}] [end] bookId=${bookId} triggeredBy=${triggeredBy} userId=${formatUserId(userId)} - scheduled file write fired`, ); - }, 3_000); + this.writeToFile(bookId, triggeredBy, userId).catch((err: Error) => + this.logger.warn( + `[${FILE_WRITE_SCHEDULE_EVENT}] [fail] bookId=${bookId} triggeredBy=${triggeredBy} userId=${formatUserId(userId)} errorClass=${err.name} error="${sanitizeErrorMessage(err.message)}" - scheduled file write failed`, + ), + ); + }, this.debounceMs); this.debounceMap.set(bookId, timer); } onModuleDestroy(): void { for (const timer of this.debounceMap.values()) clearTimeout(timer); this.debounceMap.clear(); + for (const release of this.writeQueue) { + release(); + } + this.writeQueue.length = 0; } async writeToFile(bookId: number, triggeredBy: 'auto' | 'sync', userId?: number, dryRun = false): Promise { - const file = await this.fileWriteRepo.findPrimaryFileForBook(bookId); - if (!file) { - return { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'no primary file' }; - } + await this.acquireWriteSlot(); - const format = (file.format ?? '').toLowerCase(); - if (!this.registry.supports(format)) { - const result: WriteResult = { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'format not supported' }; - this.logger.debug(`Write skipped for book ${bookId}: format not supported (${format || 'unknown'})`); - if (triggeredBy === 'sync') { - await this.fileWriteRepo.insertLog({ bookId, bookFileId: file.id, userId: userId ?? null, format: format || 'unknown', result, triggeredBy }); - } - return result; - } + const startedAt = Date.now(); + this.logger.log( + `[${FILE_WRITE_EVENT}] [start] bookId=${bookId} triggeredBy=${triggeredBy} userId=${formatUserId(userId)} dryRun=${dryRun} - file write started`, + ); - const settings = await this.settingsService.resolve(file.libraryId); - if (!settings.enabled && !dryRun) { - this.logger.debug(`Write skipped for book ${bookId}: file write is disabled`); - return { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'disabled' }; - } - - const formatSettings = resolveFormatSettings(settings, format); - if (!formatSettings.enabled) { - const result: WriteResult = { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'format disabled' }; - this.logger.debug(`Write skipped for book ${bookId}: format disabled (${format})`); - if (triggeredBy === 'sync') { - await this.fileWriteRepo.insertLog({ bookId, bookFileId: file.id, userId: userId ?? null, format, result, triggeredBy }); - } - return result; - } - - const sizeBytes = file.sizeBytes ?? 0; - if (sizeBytes > formatSettings.maxFileSizeBytes) { - const result: WriteResult = { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'file exceeds size limit' }; - this.logger.debug(`Write skipped for book ${bookId}: file size ${sizeBytes} exceeds limit ${formatSettings.maxFileSizeBytes}`); - if (triggeredBy === 'sync') { - await this.fileWriteRepo.insertLog({ bookId, bookFileId: file.id, userId: userId ?? null, format, result, triggeredBy }); - } - return result; - } - - const rawPayload = await this.fileWriteRepo.loadPayload(bookId); - if (!rawPayload) { - return { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'no metadata' }; - } - - const payload: BookWritePayload = { ...rawPayload }; - - if (settings.writeCover && !dryRun) { - payload.coverBytes = await this.loadCoverBytes(bookId); - } - - const writer = this.registry.get(format)!; - - let result: WriteResult; try { - result = await this.lockService.withLock(file.absolutePath, () => writer.write(file.absolutePath, payload, { fieldMask: ALL_FIELDS, dryRun })); - } catch (err) { - result = { status: 'failed', fieldsWritten: [], durationMs: 0, reason: (err as Error).message }; - this.logger.warn(`Write failed for book ${bookId}: ${(err as Error).message}`); - await this.fileWriteRepo.insertLog({ bookId, bookFileId: file.id, userId: userId ?? null, format, result, triggeredBy }); - return result; - } + const file = await this.fileWriteRepo.findPrimaryFileForBook(bookId); + if (!file) { + const result: WriteResult = { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'no primary file' }; + this.logWriteEnd(bookId, UNKNOWN_FORMAT, triggeredBy, userId, dryRun, startedAt, result); + return result; + } - await this.fileWriteRepo.insertLog({ bookId, bookFileId: file.id, userId: userId ?? null, format, result, triggeredBy }); - if (result.status === 'success') { - await this.fileWriteRepo.setLastWrittenAt(bookId, new Date()); - this.logger.log(`Wrote metadata to file for book ${bookId} (${format}, ${result.fieldsWritten.length} fields, ${result.durationMs}ms)`); + const format = (file.format ?? '').toLowerCase(); + if (!this.registry.supports(format)) { + const result: WriteResult = { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'format not supported' }; + if (triggeredBy === 'sync') { + await this.fileWriteRepo.insertLog({ + bookId, + bookFileId: file.id, + userId: userId ?? null, + format: format || UNKNOWN_FORMAT, + result, + triggeredBy, + }); + } + this.logWriteEnd(bookId, format || UNKNOWN_FORMAT, triggeredBy, userId, dryRun, startedAt, result); + return result; + } + + const settings = await this.settingsService.resolveForLibrary(file.libraryId); + if (!settings.enabled && !dryRun) { + const result: WriteResult = { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'disabled' }; + this.logWriteEnd(bookId, format || UNKNOWN_FORMAT, triggeredBy, userId, dryRun, startedAt, result); + return result; + } + + const formatSettings = resolveFormatSettings(settings, format); + if (!formatSettings.enabled) { + const result: WriteResult = { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'format disabled' }; + if (triggeredBy === 'sync') { + await this.fileWriteRepo.insertLog({ bookId, bookFileId: file.id, userId: userId ?? null, format, result, triggeredBy }); + } + this.logWriteEnd(bookId, format, triggeredBy, userId, dryRun, startedAt, result); + return result; + } + + const sizeBytes = file.sizeBytes ?? 0; + if (sizeBytes > formatSettings.maxFileSizeBytes) { + const result: WriteResult = { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'file exceeds size limit' }; + if (triggeredBy === 'sync') { + await this.fileWriteRepo.insertLog({ bookId, bookFileId: file.id, userId: userId ?? null, format, result, triggeredBy }); + } + this.logWriteEnd(bookId, format, triggeredBy, userId, dryRun, startedAt, result); + return result; + } + + const rawPayload = await this.fileWriteRepo.loadPayload(bookId); + if (!rawPayload) { + const result: WriteResult = { status: 'skipped', fieldsWritten: [], durationMs: 0, reason: 'no metadata' }; + this.logWriteEnd(bookId, format, triggeredBy, userId, dryRun, startedAt, result); + return result; + } + + const payload: BookWritePayload = { ...rawPayload }; + + if (settings.writeCover && !dryRun) { + payload.coverBytes = await this.loadCoverBytes(bookId); + } + + const writer = this.registry.get(format)!; + + let result: WriteResult; + try { + result = await this.lockService.withLock(file.absolutePath, () => + writer.write(file.absolutePath, payload, { fieldMask: createBookWriteFieldMask(), dryRun }), + ); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + result = { status: 'failed', fieldsWritten: [], durationMs: 0, reason }; + this.logWriteFail(bookId, format, triggeredBy, userId, dryRun, startedAt, error); + await this.fileWriteRepo.insertLog({ bookId, bookFileId: file.id, userId: userId ?? null, format, result, triggeredBy }); + return result; + } + + await this.fileWriteRepo.insertLog({ bookId, bookFileId: file.id, userId: userId ?? null, format, result, triggeredBy }); + if (result.status === 'success') { + await this.fileWriteRepo.setLastWrittenAt(bookId, new Date()); + } + this.logWriteEnd(bookId, format, triggeredBy, userId, dryRun, startedAt, result); + return result; + } finally { + this.releaseWriteSlot(); } - return result; } findWriteLog(bookId: number, limit = 20) { return this.fileWriteRepo.findWriteLog(bookId, limit); } - findNonMissingBookFilesByLibrary(libraryId: number) { - return this.fileWriteRepo.findNonMissingBookFilesByLibrary(libraryId); + findNonMissingPrimaryFilesByLibrary(libraryId: number) { + return this.fileWriteRepo.findNonMissingPrimaryFilesByLibrary(libraryId); } resolveSettings(libraryId: number) { - return this.settingsService.resolve(libraryId); + return this.settingsService.resolveForLibrary(libraryId); } private async loadCoverBytes(bookId: number): Promise { + const startedAt = Date.now(); const dir = join(this.booksPath, 'covers', String(bookId)); try { const files = await readdir(dir); const cover = files.find((f) => f.startsWith('cover_custom.')) ?? files.find((f) => f.startsWith('cover_extracted.')); if (!cover) return null; return readFile(join(dir, cover)); - } catch { + } catch (error) { + const errorClass = error instanceof Error ? error.name : 'Error'; + const errorMessage = sanitizeErrorMessage(error instanceof Error ? error.message : String(error)); + this.logger.debug( + `[${FILE_WRITE_COVER_EVENT}] [fail] bookId=${bookId} durationMs=${Date.now() - startedAt} errorClass=${errorClass} error="${errorMessage}" - cover bytes unavailable`, + ); return null; } } + + private logWriteEnd( + bookId: number, + format: string, + triggeredBy: 'auto' | 'sync', + userId: number | undefined, + dryRun: boolean, + startedAt: number, + result: WriteResult, + ): void { + const reasonPart = result.reason ? ` reason="${sanitizeErrorMessage(result.reason)}"` : ''; + this.logger.log( + `[${FILE_WRITE_EVENT}] [end] bookId=${bookId} format=${format || UNKNOWN_FORMAT} triggeredBy=${triggeredBy} userId=${formatUserId(userId)} dryRun=${dryRun} durationMs=${Date.now() - startedAt} status=${result.status} fieldsWritten=${result.fieldsWritten.length}${reasonPart} - file write completed`, + ); + } + + private logWriteFail( + bookId: number, + format: string, + triggeredBy: 'auto' | 'sync', + userId: number | undefined, + dryRun: boolean, + startedAt: number, + error: unknown, + ): void { + const errorClass = error instanceof Error ? error.name : 'Error'; + const errorMessage = sanitizeErrorMessage(error instanceof Error ? error.message : String(error)); + this.logger.warn( + `[${FILE_WRITE_EVENT}] [fail] bookId=${bookId} format=${format || UNKNOWN_FORMAT} triggeredBy=${triggeredBy} userId=${formatUserId(userId)} dryRun=${dryRun} durationMs=${Date.now() - startedAt} errorClass=${errorClass} error="${errorMessage}" - file write failed`, + ); + } + + private async acquireWriteSlot(): Promise { + if (this.activeWrites < this.maxConcurrentWrites) { + this.activeWrites++; + return; + } + + await new Promise((resolve) => { + this.writeQueue.push(resolve); + }); + this.activeWrites++; + } + + private releaseWriteSlot(): void { + this.activeWrites = Math.max(this.activeWrites - 1, 0); + const next = this.writeQueue.shift(); + if (next) { + next(); + } + } } function resolveFormatSettings(settings: GlobalFileWriteSettings, format: string): { enabled: boolean; maxFileSizeBytes: number } { switch (format) { - case 'epub': + case FORMAT_EPUB: return settings.epub; - case 'pdf': + case FORMAT_PDF: return settings.pdf; - case 'cbz': - case 'cb7': + case FORMAT_CBZ: + case FORMAT_CB7: return { - enabled: settings.cbx.enabled && (settings.cbx.formats as string[]).includes(format), + enabled: settings.cbx.enabled && isCbxFormat(format) && settings.cbx.formats.includes(format), maxFileSizeBytes: settings.cbx.maxFileSizeBytes, }; default: return { enabled: false, maxFileSizeBytes: 0 }; } } + +function isCbxFormat(format: string): format is (typeof CBX_FORMATS)[number] { + return (CBX_FORMATS as readonly string[]).includes(format); +} + +function resolvePositiveInteger(value: unknown, fallback: number): number { + const numeric = typeof value === 'number' ? value : Number(value); + if (!Number.isFinite(numeric) || numeric < 1) { + return fallback; + } + return Math.floor(numeric); +} + +function formatUserId(userId: number | undefined): string { + return userId == null ? 'null' : String(userId); +} + +function sanitizeErrorMessage(message: string): string { + return message.replace(/"/g, '\\"'); +} diff --git a/server/src/modules/file-write/format-writer.registry.test.ts b/server/src/modules/file-write/format-writer.registry.test.ts index 12cda8f8f..5d3ef3c67 100644 --- a/server/src/modules/file-write/format-writer.registry.test.ts +++ b/server/src/modules/file-write/format-writer.registry.test.ts @@ -13,12 +13,10 @@ describe('FormatWriterRegistry', () => { expect(registry.get('unknown')).toBeUndefined(); }); - it('keeps the last writer when duplicate format keys exist', () => { + it('throws when duplicate format keys are registered', () => { const first = { format: 'cbz', write: vi.fn() }; const second = { format: 'CBZ', write: vi.fn() }; - const registry = new FormatWriterRegistry([first, second] as never); - - expect(registry.get('cbz')).toBe(second); + expect(() => new FormatWriterRegistry([first, second] as never)).toThrow('Duplicate format writer registered: cbz'); }); }); diff --git a/server/src/modules/file-write/format-writer.registry.ts b/server/src/modules/file-write/format-writer.registry.ts index cf34f2206..79b43f369 100644 --- a/server/src/modules/file-write/format-writer.registry.ts +++ b/server/src/modules/file-write/format-writer.registry.ts @@ -8,7 +8,15 @@ export class FormatWriterRegistry { private readonly map: Map; constructor(@Inject(FORMAT_WRITERS) writers: FormatWriter[]) { - this.map = new Map((writers ?? []).map((w) => [w.format.toLowerCase(), w])); + this.map = new Map(); + + for (const writer of writers ?? []) { + const key = writer.format.toLowerCase(); + if (this.map.has(key)) { + throw new Error(`Duplicate format writer registered: ${key}`); + } + this.map.set(key, writer); + } } get(format: string): FormatWriter | undefined { diff --git a/server/src/modules/file-write/formats/cbx/cb7-format-writer.ts b/server/src/modules/file-write/formats/cbx/cb7-format-writer.ts index c491ba438..cbe87327a 100644 --- a/server/src/modules/file-write/formats/cbx/cb7-format-writer.ts +++ b/server/src/modules/file-write/formats/cbx/cb7-format-writer.ts @@ -1,5 +1,5 @@ import { Injectable } from '@nestjs/common'; -import { readFile, rename, unlink, writeFile } from 'fs/promises'; +import { readFile, writeFile } from 'fs/promises'; import { dirname, join } from 'path'; import { randomUUID } from 'crypto'; @@ -8,10 +8,11 @@ import { getSevenZip, type SevenZipFS } from '../../../../common/sevenzip'; import type { BookWritePayload } from '../../interfaces/book-write-payload.interface'; import type { FormatWriter } from '../../interfaces/format-writer.interface'; import type { FormatWriteOptions } from '../../interfaces/format-write-options.interface'; +import { replaceFileAtomically } from '../shared/atomic-file-replace'; import { resolveFieldsWritten } from '../shared/resolve-fields-written'; import { buildComicInfoXml } from './comic-info-builder'; -// All WASM FS ops are synchronous — Node's single-threaded event loop guarantees +// All WASM FS ops are synchronous - Node's single-threaded event loop guarantees // that synchronous blocks from concurrent writes never interleave with each other. // The archive path uses a unique ID to avoid VFS collisions across concurrent writes. // The XML path is shared but safe: it is only used within synchronous blocks and @@ -52,8 +53,11 @@ export class Cb7FormatWriter implements FormatWriter { sz.callMain(['e', vfsArchivePath, `-o${vfsExtractDir}`, 'ComicInfo.xml', '-y']); const raw = sz.FS.readFile(`${vfsExtractDir}/ComicInfo.xml`); existingXml = Buffer.from(raw).toString('utf-8'); - } catch { - // No ComicInfo.xml in archive — start fresh + } catch (error) { + if (!isMissingComicInfoError(error)) { + throw error; + } + // No ComicInfo.xml in archive - start fresh } const xml = buildComicInfoXml(existingXml, payload, fieldMask); @@ -61,8 +65,11 @@ export class Cb7FormatWriter implements FormatWriter { try { sz.FS.unlink(VFS_XML_PATH); - } catch { - // Not present — expected on first write + } catch (error) { + if (!isMissingVfsPathError(error)) { + throw error; + } + // Not present - expected on first write } const fd2 = sz.FS.open(VFS_XML_PATH, 'w+'); sz.FS.write(fd2, xmlBytes, 0, xmlBytes.length); @@ -79,13 +86,8 @@ export class Cb7FormatWriter implements FormatWriter { // --- synchronous block end --- const tmpPath = join(dirname(filePath), `.cbx-write-${uid}`); - await writeFile(tmpPath, Buffer.from(modifiedBytes)); - try { - await rename(tmpPath, filePath); - } catch (err) { - await unlink(tmpPath).catch(() => {}); - throw err; - } + await writeFile(tmpPath, modifiedBytes); + await replaceFileAtomically(tmpPath, filePath); return { status: 'success', fieldsWritten, durationMs: Date.now() - start }; } finally { @@ -95,27 +97,62 @@ export class Cb7FormatWriter implements FormatWriter { } function cleanupVfs(vfs: SevenZipFS, archivePath: string, extractDir: string): void { - try { - vfs.unlink(VFS_XML_PATH); - } catch { - // Already deleted by a concurrent write's cleanup — that's fine + unlinkIfExists(vfs, VFS_XML_PATH); + unlinkIfExists(vfs, archivePath); + + const files = readDirIfExists(vfs, extractDir); + if (!files) return; + + for (const f of files) { + if (f === '.' || f === '..') continue; + unlinkIfExists(vfs, `${extractDir}/${f}`); } + try { - vfs.unlink(archivePath); - } catch { - /* ignore */ - } - try { - for (const f of vfs.readdir(extractDir)) { - if (f === '.' || f === '..') continue; - try { - vfs.unlink(`${extractDir}/${f}`); - } catch { - /* ignore */ - } - } vfs.rmdir(extractDir); - } catch { - /* ignore */ + } catch (error) { + if (!isMissingVfsPathError(error)) { + throw error; + } } } + +function unlinkIfExists(vfs: SevenZipFS, path: string): void { + try { + vfs.unlink(path); + } catch (error) { + if (!isMissingVfsPathError(error)) { + throw error; + } + } +} + +function readDirIfExists(vfs: SevenZipFS, path: string): string[] | null { + try { + return vfs.readdir(path); + } catch (error) { + if (isMissingVfsPathError(error)) { + return null; + } + throw error; + } +} + +function isMissingComicInfoError(error: unknown): boolean { + const message = normalizeErrorMessage(error); + return isMissingVfsPathError(error) || message.includes('no files to process'); +} + +function isMissingVfsPathError(error: unknown): boolean { + const message = normalizeErrorMessage(error); + return ( + message.includes('no such file') || message.includes('not found') || message.includes('does not exist') || message.includes('path not found') + ); +} + +function normalizeErrorMessage(error: unknown): string { + if (error instanceof Error) { + return error.message.toLowerCase(); + } + return String(error).toLowerCase(); +} diff --git a/server/src/modules/file-write/formats/cbx/cbz-zip-patcher.ts b/server/src/modules/file-write/formats/cbx/cbz-zip-patcher.ts index b8fec78de..0ab6dace4 100644 --- a/server/src/modules/file-write/formats/cbx/cbz-zip-patcher.ts +++ b/server/src/modules/file-write/formats/cbx/cbz-zip-patcher.ts @@ -1,9 +1,9 @@ -import * as fs from 'fs/promises'; import { createWriteStream } from 'fs'; import { dirname, join } from 'path'; import { randomUUID } from 'crypto'; import * as unzipper from 'unzipper'; import archiver from 'archiver'; +import { replaceFileAtomically } from '../shared/atomic-file-replace'; function isComicInfoEntry(entryPath: string): boolean { const normalized = entryPath.replace(/\\/g, '/').toLowerCase(); @@ -41,10 +41,5 @@ export async function writeComicInfoToZip(filePath: string, xmlContent: string): void archive.finalize(); }); - try { - await fs.rename(tmpPath, filePath); - } catch (err) { - await fs.unlink(tmpPath).catch(() => {}); - throw err; - } + await replaceFileAtomically(tmpPath, filePath); } diff --git a/server/src/modules/file-write/formats/cbx/comic-info-builder.ts b/server/src/modules/file-write/formats/cbx/comic-info-builder.ts index af6ed7b35..a64225e15 100644 --- a/server/src/modules/file-write/formats/cbx/comic-info-builder.ts +++ b/server/src/modules/file-write/formats/cbx/comic-info-builder.ts @@ -1,6 +1,7 @@ import { XMLParser, XMLBuilder } from 'fast-xml-parser'; import type { BookWritePayload, BookWritePayloadKey } from '../../interfaces/book-write-payload.interface'; +import { COMIC_INFO_MANAGED_NOTES_KEYS, COMIC_INFO_PROVIDER_ID_KEYS, COMIC_INFO_PROVIDER_WEB_URL_BUILDERS } from '../../file-write.constants'; type ComicInfoObject = Record; @@ -23,7 +24,10 @@ function parseComicInfoXml(xml: string): ComicInfoObject { try { const parsed = PARSER.parse(xml) as { ComicInfo?: ComicInfoObject }; return parsed.ComicInfo ?? {}; - } catch { + } catch (error) { + if (!(error instanceof Error)) { + throw error; + } return {}; } } @@ -45,7 +49,7 @@ export function buildComicInfoXml(existingXml: string | null, payload: BookWrite if (fieldMask.has('rating') && payload.rating != null) info['CommunityRating'] = formatRating(payload.rating); if (fieldMask.has('isbn13') && payload.isbn13 != null) info['GTIN'] = payload.isbn13; - const hasProviderSelection = PROVIDER_ID_KEYS.some((key) => fieldMask.has(key)); + const hasProviderSelection = COMIC_INFO_PROVIDER_ID_KEYS.some((key) => fieldMask.has(key)); if (hasProviderSelection) { const webUrl = resolveWebUrl(payload, fieldMask); if (webUrl != null) { @@ -55,7 +59,7 @@ export function buildComicInfoXml(existingXml: string | null, payload: BookWrite } } - const hasManagedNotesSelection = MANAGED_NOTES_KEYS.some((key) => fieldMask.has(key)); + const hasManagedNotesSelection = COMIC_INFO_MANAGED_NOTES_KEYS.some((key) => fieldMask.has(key)); if (hasManagedNotesSelection) { const existingNotes = typeof info['Notes'] === 'string' ? info['Notes'] : null; const notes = buildNotes(existingNotes, payload, fieldMask); @@ -95,22 +99,20 @@ function formatRating(val: number): string { return Math.min(5.0, Math.max(0.0, val / 2.0)).toFixed(1); } -const PROVIDER_ID_KEYS: BookWritePayloadKey[] = ['goodreadsId', 'amazonId', 'hardcoverId', 'googleBooksId', 'openLibraryId']; -const MANAGED_NOTES_KEYS: BookWritePayloadKey[] = ['subtitle', 'isbn10', 'goodreadsId', 'amazonId', 'hardcoverId', 'googleBooksId', 'openLibraryId']; - function resolveWebUrl(payload: BookWritePayload, fieldMask: Set): string | null { - if (fieldMask.has('goodreadsId') && payload.goodreadsId) return `https://www.goodreads.com/book/show/${payload.goodreadsId}`; - if (fieldMask.has('amazonId') && payload.amazonId) return `https://www.amazon.com/dp/${payload.amazonId}`; - if (fieldMask.has('hardcoverId') && payload.hardcoverId) return `https://hardcover.app/books/${payload.hardcoverId}`; - if (fieldMask.has('googleBooksId') && payload.googleBooksId) return `https://books.google.com/books?id=${payload.googleBooksId}`; - if (fieldMask.has('openLibraryId') && payload.openLibraryId) return `https://openlibrary.org/works/${payload.openLibraryId}`; + for (const key of COMIC_INFO_PROVIDER_ID_KEYS) { + if (!fieldMask.has(key)) continue; + const value = payload[key]; + if (typeof value !== 'string' || value === '') continue; + return COMIC_INFO_PROVIDER_WEB_URL_BUILDERS[key](value); + } return null; } function buildNotes(existing: string | null, payload: BookWritePayload, fieldMask: Set): string | null { const lines: string[] = []; const existingManaged = new Map(); - const managedKeys = new Set(MANAGED_NOTES_KEYS.map((k) => String(k))); + const managedKeys = new Set(COMIC_INFO_MANAGED_NOTES_KEYS.map((k) => String(k))); if (existing) { for (const line of existing.split('\n')) { @@ -131,15 +133,7 @@ function buildNotes(existing: string | null, payload: BookWritePayload, fieldMas } } - const ids: [BookWritePayloadKey, string | null | undefined][] = [ - ['subtitle', payload.subtitle], - ['isbn10', payload.isbn10], - ['goodreadsId', payload.goodreadsId], - ['amazonId', payload.amazonId], - ['hardcoverId', payload.hardcoverId], - ['googleBooksId', payload.googleBooksId], - ['openLibraryId', payload.openLibraryId], - ]; + const ids: [BookWritePayloadKey, string | null][] = COMIC_INFO_MANAGED_NOTES_KEYS.map((key) => [key, resolveManagedTextField(payload, key)]); for (const [key, val] of ids) { if (fieldMask.has(key)) { @@ -155,3 +149,8 @@ function buildNotes(existing: string | null, payload: BookWritePayload, fieldMas return lines.length > 0 ? lines.join('\n') : null; } + +function resolveManagedTextField(payload: BookWritePayload, key: BookWritePayloadKey): string | null { + const value = payload[key]; + return typeof value === 'string' ? value : null; +} diff --git a/server/src/modules/file-write/formats/epub/epub-cover-handler.ts b/server/src/modules/file-write/formats/epub/epub-cover-handler.ts index 9fef15805..a3b3f3246 100644 --- a/server/src/modules/file-write/formats/epub/epub-cover-handler.ts +++ b/server/src/modules/file-write/formats/epub/epub-cover-handler.ts @@ -82,7 +82,7 @@ export function locate(opfXml: string, opfDir: string): CoverSlot | null { } } - // Strategy 1: meta[name="cover"] → manifest item by id (EPUB2 — what Calibre/readers follow) + // Strategy 1: meta[name="cover"] -> manifest item by id (EPUB2 - what Calibre/readers follow) const coverMeta = metaMetas.find((m) => attr(m, '@_name').toLowerCase() === 'cover'); if (coverMeta) { const coverId = attr(coverMeta, '@_content'); diff --git a/server/src/modules/file-write/formats/epub/epub-format-writer.ts b/server/src/modules/file-write/formats/epub/epub-format-writer.ts index 0d723e0cd..9789fe490 100644 --- a/server/src/modules/file-write/formats/epub/epub-format-writer.ts +++ b/server/src/modules/file-write/formats/epub/epub-format-writer.ts @@ -18,7 +18,7 @@ export class EpubFormatWriter implements FormatWriter { const { opfPath, opfDir } = await locateOpf(filePath); const opfXml = await EpubZipPatcher.readEntry(filePath, opfPath); - const { newOpfXml, fieldsWritten } = buildOpf(opfXml, payload, options); + const { newOpfXml, fieldsWritten } = buildOpf(opfXml, payload); const patches = new Map([[opfPath, Buffer.from(newOpfXml)]]); diff --git a/server/src/modules/file-write/formats/epub/epub-opf-builder.test.ts b/server/src/modules/file-write/formats/epub/epub-opf-builder.test.ts index 4282618e7..c9bebe91c 100644 --- a/server/src/modules/file-write/formats/epub/epub-opf-builder.test.ts +++ b/server/src/modules/file-write/formats/epub/epub-opf-builder.test.ts @@ -24,7 +24,7 @@ describe('epub-opf-builder', () => { it('throws on unsupported package version', () => { const opf = ``; - expect(() => build(opf, { title: 'Book' }, { fieldMask: new Set(['title']), dryRun: false })).toThrow('Unsupported EPUB version: "1.0"'); + expect(() => build(opf, { title: 'Book' })).toThrow('Unsupported EPUB version: "1.0"'); }); it('rewrites metadata for EPUB3, preserves UID, and appends required namespace prefix', () => { @@ -40,19 +40,15 @@ describe('epub-opf-builder', () => { `; - const result = build( - opf, - { - title: 'New Title', - subtitle: 'Sub', - authors: [{ name: 'Frank Herbert', sortName: 'Herbert, Frank' }], - genres: ['Sci-Fi'], - seriesName: 'Dune', - seriesIndex: 1, - tags: ['Classic'], - }, - { fieldMask: new Set(['title']), dryRun: false }, - ); + const result = build(opf, { + title: 'New Title', + subtitle: 'Sub', + authors: [{ name: 'Frank Herbert', sortName: 'Herbert, Frank' }], + genres: ['Sci-Fi'], + seriesName: 'Dune', + seriesIndex: 1, + tags: ['Classic'], + }); expect(result.newOpfXml).toContain('projectx: https://projectx.app/metadata/1.0/'); expect(result.newOpfXml).toContain('urn:uuid:abc'); @@ -76,7 +72,7 @@ describe('epub-opf-builder', () => { `; - const result = build(opf, { title: 'Replacement' }, { fieldMask: new Set(['title']), dryRun: false }); + const result = build(opf, { title: 'Replacement' }); expect(result.newOpfXml).toContain(''); expect(result.newOpfXml).toContain('Replacement'); diff --git a/server/src/modules/file-write/formats/epub/epub-opf-builder.ts b/server/src/modules/file-write/formats/epub/epub-opf-builder.ts index 73a22af48..7805e4550 100644 --- a/server/src/modules/file-write/formats/epub/epub-opf-builder.ts +++ b/server/src/modules/file-write/formats/epub/epub-opf-builder.ts @@ -1,6 +1,6 @@ import { XMLParser, XMLBuilder } from 'fast-xml-parser'; import type { BookWritePayload } from '../../interfaces/book-write-payload.interface'; -import type { FormatWriteOptions } from '../../interfaces/format-write-options.interface'; +import { EPUB_PROVIDER_IDENTIFIER_PREFIXES } from '../../file-write.constants'; import { PROJECTX_NS_PREFIX as APP_WRITE_NAMESPACE, PROJECTX_NS_URI as APP_NS_URI } from '../shared/projectx-ns'; import { resolveFieldsWritten } from '../shared/resolve-fields-written'; @@ -264,20 +264,11 @@ function buildFreshMetadata(payload: BookWritePayload, epubVersion: 3 | 2, uidNo } } - // Provider identifiers — plain dc:identifier with urn: text, no scheme/id attributes (matches Booklore) - const providerMappings: Array<{ field: keyof BookWritePayload; urnPrefix: string }> = [ - { field: 'goodreadsId', urnPrefix: 'urn:goodreads:' }, - { field: 'amazonId', urnPrefix: 'urn:amazon:' }, - { field: 'hardcoverId', urnPrefix: 'urn:hardcover:' }, - { field: 'googleBooksId', urnPrefix: 'urn:google:' }, - { field: 'openLibraryId', urnPrefix: 'urn:openlibrary:' }, - { field: 'itunesId', urnPrefix: 'urn:itunes:' }, - ]; - - for (const { field, urnPrefix } of providerMappings) { - const value = payload[field] as string | null | undefined; - if (value != null) { - nodes.push(makeTextNode('dc:identifier', `${urnPrefix}${value}`)); + // Provider identifiers - plain dc:identifier with urn: text, no scheme/id attributes (matches Booklore) + for (const field of EPUB_PROVIDER_IDENTIFIER_KEYS) { + const value = payload[field]; + if (typeof value === 'string' && value !== '') { + nodes.push(makeTextNode('dc:identifier', `${EPUB_PROVIDER_IDENTIFIER_PREFIXES[field]}${value}`)); } } @@ -339,8 +330,7 @@ function getPkgContentAll(pkgNode: OrderedNode): OrderedNode[] { return pkgNode['package'] as OrderedNode[]; } -// eslint-disable-next-line @typescript-eslint/no-unused-vars -export function build(opfXml: string, payload: BookWritePayload, _options: FormatWriteOptions): { newOpfXml: string; fieldsWritten: string[] } { +export function build(opfXml: string, payload: BookWritePayload): { newOpfXml: string; fieldsWritten: string[] } { const parsed = writerParser.parse(opfXml) as OrderedNode[]; const pkgResult = getPackageNode(parsed); if (!pkgResult) throw new Error('Cannot find element in OPF'); @@ -371,3 +361,6 @@ export function build(opfXml: string, payload: BookWritePayload, _options: Forma return { newOpfXml, fieldsWritten: resolveFieldsWritten(payload) }; } + +type EpubProviderIdentifierKey = keyof typeof EPUB_PROVIDER_IDENTIFIER_PREFIXES; +const EPUB_PROVIDER_IDENTIFIER_KEYS = Object.keys(EPUB_PROVIDER_IDENTIFIER_PREFIXES) as EpubProviderIdentifierKey[]; diff --git a/server/src/modules/file-write/formats/epub/epub-zip-patcher.ts b/server/src/modules/file-write/formats/epub/epub-zip-patcher.ts index 81aac1cf4..d9c49749b 100644 --- a/server/src/modules/file-write/formats/epub/epub-zip-patcher.ts +++ b/server/src/modules/file-write/formats/epub/epub-zip-patcher.ts @@ -1,7 +1,7 @@ -import * as fs from 'fs/promises'; import { createWriteStream } from 'fs'; import * as unzipper from 'unzipper'; import archiver from 'archiver'; +import { replaceFileAtomically } from '../shared/atomic-file-replace'; export async function readEntry(filePath: string, entryPath: string): Promise { const zip = await unzipper.Open.file(filePath); @@ -39,10 +39,5 @@ export async function patch(filePath: string, patches: Map): Pro void archive.finalize(); }); - try { - await fs.rename(tmpPath, filePath); - } catch (err) { - await fs.unlink(tmpPath).catch(() => {}); - throw err; - } + await replaceFileAtomically(tmpPath, filePath); } diff --git a/server/src/modules/file-write/formats/pdf/pdf-format-writer.ts b/server/src/modules/file-write/formats/pdf/pdf-format-writer.ts index 7322f29df..b535872ad 100644 --- a/server/src/modules/file-write/formats/pdf/pdf-format-writer.ts +++ b/server/src/modules/file-write/formats/pdf/pdf-format-writer.ts @@ -1,5 +1,5 @@ import { Injectable } from '@nestjs/common'; -import { readFile, rename, unlink, writeFile } from 'fs/promises'; +import { readFile, writeFile } from 'fs/promises'; import { dirname, join } from 'path'; import { randomUUID } from 'crypto'; import { PDFDocument, PDFName } from 'pdf-lib'; @@ -8,6 +8,7 @@ import type { WriteResult } from '@projectx/types'; import type { BookWritePayload, BookWritePayloadKey } from '../../interfaces/book-write-payload.interface'; import type { FormatWriter } from '../../interfaces/format-writer.interface'; import type { FormatWriteOptions } from '../../interfaces/format-write-options.interface'; +import { replaceFileAtomically } from '../shared/atomic-file-replace'; import { PROJECTX_NS_PREFIX } from '../shared/projectx-ns'; import { resolveFieldsWritten } from '../shared/resolve-fields-written'; import { buildXmp } from './pdf-xmp-builder'; @@ -34,13 +35,8 @@ export class PdfFormatWriter implements FormatWriter { const savedBytes = await pdfDoc.save(); const tempPath = join(dirname(filePath), `.tmp-${randomUUID()}.pdf`); - try { - await writeFile(tempPath, savedBytes); - await rename(tempPath, filePath); - } catch (err) { - await unlink(tempPath).catch(() => {}); - throw err; - } + await writeFile(tempPath, savedBytes); + await replaceFileAtomically(tempPath, filePath); return { status: 'success', fieldsWritten, durationMs: Date.now() - start }; } diff --git a/server/src/modules/file-write/formats/pdf/pdf-xmp-builder.ts b/server/src/modules/file-write/formats/pdf/pdf-xmp-builder.ts index cb78a29d1..468fed61f 100644 --- a/server/src/modules/file-write/formats/pdf/pdf-xmp-builder.ts +++ b/server/src/modules/file-write/formats/pdf/pdf-xmp-builder.ts @@ -1,4 +1,5 @@ import type { BookWritePayload, BookWritePayloadKey } from '../../interfaces/book-write-payload.interface'; +import { EPUB_PROVIDER_IDENTIFIER_PREFIXES } from '../../file-write.constants'; import { PROJECTX_NS_PREFIX, PROJECTX_NS_URI } from '../shared/projectx-ns'; function escapeXml(value: string): string { @@ -85,23 +86,11 @@ export function buildXmp(payload: BookWritePayload, fieldMask: Set', ].join('\n'); } + +type ProviderIdKey = keyof typeof EPUB_PROVIDER_IDENTIFIER_PREFIXES; +const PROVIDER_ID_KEYS = Object.keys(EPUB_PROVIDER_IDENTIFIER_PREFIXES) as ProviderIdKey[]; diff --git a/server/src/modules/file-write/formats/shared/atomic-file-replace.test.ts b/server/src/modules/file-write/formats/shared/atomic-file-replace.test.ts new file mode 100644 index 000000000..183e32334 --- /dev/null +++ b/server/src/modules/file-write/formats/shared/atomic-file-replace.test.ts @@ -0,0 +1,56 @@ +import { rename, unlink } from 'fs/promises'; +import type { MockedFunction } from 'vitest'; + +import { replaceFileAtomically } from './atomic-file-replace'; + +vi.mock('fs/promises', async () => { + const actual = await vi.importActual('fs/promises'); + return { + ...actual, + rename: vi.fn(), + unlink: vi.fn(), + }; +}); + +const mockRename = rename as MockedFunction; +const mockUnlink = unlink as MockedFunction; + +describe('replaceFileAtomically', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('renames file when target replacement succeeds', async () => { + mockRename.mockResolvedValue(undefined); + + await expect(replaceFileAtomically('/tmp/file.tmp', '/tmp/file.epub')).resolves.toBeUndefined(); + + expect(mockRename).toHaveBeenCalledWith('/tmp/file.tmp', '/tmp/file.epub'); + expect(mockUnlink).not.toHaveBeenCalled(); + }); + + it('cleans temp path and rethrows rename error when rename fails', async () => { + mockRename.mockRejectedValue(new Error('cross-device link')); + mockUnlink.mockResolvedValue(undefined); + + await expect(replaceFileAtomically('/tmp/file.tmp', '/tmp/file.epub')).rejects.toThrow('cross-device link'); + expect(mockUnlink).toHaveBeenCalledWith('/tmp/file.tmp'); + }); + + it('ignores ENOENT while cleaning temp path after rename failure', async () => { + const renameError = new Error('permission denied'); + const cleanupError = Object.assign(new Error('not found'), { code: 'ENOENT' }); + + mockRename.mockRejectedValue(renameError); + mockUnlink.mockRejectedValue(cleanupError); + + await expect(replaceFileAtomically('/tmp/file.tmp', '/tmp/file.epub')).rejects.toThrow('permission denied'); + }); + + it('throws aggregate error when rename and cleanup both fail', async () => { + mockRename.mockRejectedValue(new Error('rename failed')); + mockUnlink.mockRejectedValue(Object.assign(new Error('cleanup failed'), { code: 'EACCES' })); + + await expect(replaceFileAtomically('/tmp/file.tmp', '/tmp/file.epub')).rejects.toThrow('Failed to replace file atomically'); + }); +}); diff --git a/server/src/modules/file-write/formats/shared/atomic-file-replace.ts b/server/src/modules/file-write/formats/shared/atomic-file-replace.ts new file mode 100644 index 000000000..1dac16f50 --- /dev/null +++ b/server/src/modules/file-write/formats/shared/atomic-file-replace.ts @@ -0,0 +1,29 @@ +import { rename, unlink } from 'fs/promises'; + +export async function replaceFileAtomically(tempPath: string, targetPath: string): Promise { + try { + await rename(tempPath, targetPath); + } catch (renameError) { + try { + await unlink(tempPath); + } catch (cleanupError) { + if (!isErrnoCode(cleanupError, 'ENOENT')) { + const renameCause = toError(renameError); + const cleanupCause = toError(cleanupError); + throw new Error( + `Failed to replace file atomically tempPath=${tempPath} targetPath=${targetPath} renameError="${renameCause.message}" cleanupError="${cleanupCause.message}"`, + { cause: cleanupError }, + ); + } + } + throw renameError; + } +} + +function toError(error: unknown): Error { + return error instanceof Error ? error : new Error(String(error)); +} + +function isErrnoCode(error: unknown, code: string): error is NodeJS.ErrnoException { + return typeof error === 'object' && error !== null && 'code' in error && (error as NodeJS.ErrnoException).code === code; +} diff --git a/server/src/modules/library/library.service.test.ts b/server/src/modules/library/library.service.test.ts index cf2014dd8..47be77318 100644 --- a/server/src/modules/library/library.service.test.ts +++ b/server/src/modules/library/library.service.test.ts @@ -57,7 +57,7 @@ describe('LibraryService', () => { const fileWatcherService = { startWatcher: vi.fn(), stopWatcher: vi.fn() }; const fileWriteService = { resolveSettings: vi.fn(), - findNonMissingBookFilesByLibrary: vi.fn(), + findNonMissingPrimaryFilesByLibrary: vi.fn(), writeToFile: vi.fn(), }; @@ -283,7 +283,7 @@ describe('LibraryService', () => { it('writeMetadataToFiles emits progress and returns summary counters', async () => { fileWriteService.resolveSettings.mockResolvedValue({ enabled: true }); - fileWriteService.findNonMissingBookFilesByLibrary.mockResolvedValue([{ bookId: 1 }, { bookId: 2 }, { bookId: 3 }]); + fileWriteService.findNonMissingPrimaryFilesByLibrary.mockResolvedValue([{ bookId: 1 }, { bookId: 2 }, { bookId: 3 }]); fileWriteService.writeToFile .mockResolvedValueOnce({ status: 'success', fieldsWritten: [], durationMs: 1 }) .mockResolvedValueOnce({ status: 'failed', fieldsWritten: [], durationMs: 1, reason: 'write failed' }) @@ -300,7 +300,7 @@ describe('LibraryService', () => { it('writeMetadataToFiles stops when cancellation is requested', async () => { fileWriteService.resolveSettings.mockResolvedValue({ enabled: true }); - fileWriteService.findNonMissingBookFilesByLibrary.mockResolvedValue([{ bookId: 1 }, { bookId: 2 }]); + fileWriteService.findNonMissingPrimaryFilesByLibrary.mockResolvedValue([{ bookId: 1 }, { bookId: 2 }]); fileWriteService.writeToFile.mockResolvedValue({ status: 'success', fieldsWritten: [], durationMs: 1 }); let isCancelled = false; diff --git a/server/src/modules/library/library.service.ts b/server/src/modules/library/library.service.ts index b1d62f270..569af1a44 100644 --- a/server/src/modules/library/library.service.ts +++ b/server/src/modules/library/library.service.ts @@ -287,7 +287,7 @@ export class LibraryService { } } - const rows = await this.fileWriteService.findNonMissingBookFilesByLibrary(libraryId); + const rows = await this.fileWriteService.findNonMissingPrimaryFilesByLibrary(libraryId); const onProgress = options.onProgress ?? (() => undefined); const isCancelled = options.isCancelled ?? (() => false);