diff --git a/server/src/common/book-cover-storage.test.ts b/server/src/common/book-cover-storage.test.ts index d62a1decf..1fa9cd88d 100644 --- a/server/src/common/book-cover-storage.test.ts +++ b/server/src/common/book-cover-storage.test.ts @@ -3,7 +3,6 @@ import { join } from 'path'; import { bookCoverDirPath, bookThumbnailPath, - findExtractedBookCoverFileName, findPreferredBookCoverFileName, isCustomBookCoverFileName, isExtractedBookCoverFileName, @@ -29,9 +28,4 @@ describe('book-cover-storage', () => { expect(findPreferredBookCoverFileName(['thumbnail.jpg', 'cover.png'])).toBe('cover.png'); expect(findPreferredBookCoverFileName(['thumbnail.jpg'])).toBeNull(); }); - - it('finds extracted cover files when present', () => { - expect(findExtractedBookCoverFileName(['cover_custom.png', 'cover_extracted.jpg'])).toBe('cover_extracted.jpg'); - expect(findExtractedBookCoverFileName(['cover_custom.png'])).toBeNull(); - }); }); diff --git a/server/src/common/book-cover-storage.ts b/server/src/common/book-cover-storage.ts index b92f9dd83..da70137c3 100644 --- a/server/src/common/book-cover-storage.ts +++ b/server/src/common/book-cover-storage.ts @@ -25,10 +25,6 @@ export function findPreferredBookCoverFileName(files: readonly string[]): string return files.find(isCustomBookCoverFileName) ?? files.find(isExtractedBookCoverFileName) ?? files.find(isLegacyBookCoverFileName) ?? null; } -export function findExtractedBookCoverFileName(files: readonly string[]): string | null { - return files.find(isExtractedBookCoverFileName) ?? null; -} - function isLegacyBookCoverFileName(fileName: string): boolean { return fileName.startsWith(COVER_LEGACY_FILE_PREFIX); } diff --git a/server/src/modules/book/book.service.test.ts b/server/src/modules/book/book.service.test.ts index 3e9b5ee06..6f5cd3adf 100644 --- a/server/src/modules/book/book.service.test.ts +++ b/server/src/modules/book/book.service.test.ts @@ -172,6 +172,7 @@ function makeService(overrides: { bookMetadataLockService?: unknown } = {}) { emitAuthorsReplaced: vi.fn(), downloadAndSaveCover: vi.fn().mockResolvedValue(undefined), refreshCoverForBook: vi.fn(), + ensureThumbnailForBook: vi.fn().mockResolvedValue(null), }; const pipeline = { run: vi.fn(), @@ -853,7 +854,7 @@ describe('BookService', () => { }); it('returns thumbnail path only when file is accessible', async () => { - const { service, bookRepo } = makeService(); + const { service, bookRepo, metadataService } = makeService(); bookRepo.findLibraryIdByBookId.mockResolvedValue(5); mockAccess.mockResolvedValue(undefined); @@ -861,6 +862,18 @@ describe('BookService', () => { mockAccess.mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })); await expect(service.getThumbnailPath(9, makeUser())).resolves.toBeNull(); + expect(metadataService.ensureThumbnailForBook).toHaveBeenCalledWith(9); + }); + + it('repairs and returns a missing thumbnail from the active cover', async () => { + const { service, bookRepo, metadataService } = makeService(); + bookRepo.findLibraryIdByBookId.mockResolvedValue(5); + mockAccess.mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })); + metadataService.ensureThumbnailForBook.mockResolvedValue('/tmp/books/covers/9/thumbnail.jpg'); + + await expect(service.getThumbnailPath(9, makeUser())).resolves.toBe('/tmp/books/covers/9/thumbnail.jpg'); + + expect(metadataService.ensureThumbnailForBook).toHaveBeenCalledWith(9); }); it('throws when thumbnail access fails for non-missing errors', async () => { diff --git a/server/src/modules/book/book.service.ts b/server/src/modules/book/book.service.ts index 95ca7d639..4ea22c3ce 100644 --- a/server/src/modules/book/book.service.ts +++ b/server/src/modules/book/book.service.ts @@ -1315,7 +1315,7 @@ export class BookService { await access(path); return path; } catch (err) { - if (this.isMissingFilesystemEntry(err)) return null; + if (this.isMissingFilesystemEntry(err)) return this.metadataService.ensureThumbnailForBook(id); const errorClass = err instanceof Error ? err.name : 'Error'; const errorMessage = sanitizeLogValue(err instanceof Error ? err.message : String(err)); const pathValue = sanitizeLogValue(path); diff --git a/server/src/modules/cover/cover.service.ts b/server/src/modules/cover/cover.service.ts index de4c78bef..e1630a51a 100644 --- a/server/src/modules/cover/cover.service.ts +++ b/server/src/modules/cover/cover.service.ts @@ -7,7 +7,7 @@ import { NodePgDatabase } from 'drizzle-orm/node-postgres'; import { mkdir, readdir, readFile, rename, unlink, writeFile } from 'fs/promises'; import { basename, join } from 'path'; -import { bookCoverDirPath, bookThumbnailPath, findExtractedBookCoverFileName } from '../../common/book-cover-storage'; +import { bookCoverDirPath, bookThumbnailPath, findPreferredBookCoverFileName, isCustomBookCoverFileName } from '../../common/book-cover-storage'; import type { RequestUser } from '../../common/types/request-user'; import { sanitizeLogValue } from '../../common/utils/log-sanitize.utils'; import { ensureSafeRemoteHost } from '../../common/utils/ssrf.utils'; @@ -167,8 +167,8 @@ export class CoverService { const dir = bookCoverDirPath(this.appDataPath, bookId); await this.deleteFilesByPrefix(dir, COVER_CUSTOM_FILE_PREFIX); - const extractedPath = await this.findExtractedCover(bookId); - if (!extractedPath) { + const fallbackPath = await this.findFallbackCover(bookId); + if (!fallbackPath) { await this.removeFileIfPresent(bookThumbnailPath(this.appDataPath, bookId)); await this.setCoverSource(bookId, null); this.logger.log( @@ -177,7 +177,7 @@ export class CoverService { return null; } - const bytes = await readFile(extractedPath); + const bytes = await readFile(fallbackPath); const thumb = await generateThumbnail(bytes); await writeFile(bookThumbnailPath(this.appDataPath, bookId), thumb); await this.setCoverSource(bookId, 'extracted'); @@ -281,10 +281,15 @@ export class CoverService { } } - private async findExtractedCover(bookId: number): Promise { + /** + * Resolves the cover left behind once the custom cover is deleted. This must use the same + * preference order the cover endpoint serves from, including legacy `cover.*` files: matching on + * extracted covers alone dropped the thumbnail for a book the endpoint still served (issue #1475). + */ + private async findFallbackCover(bookId: number): Promise { const dir = bookCoverDirPath(this.appDataPath, bookId); const files = await this.readDirIfExists(dir); - const found = findExtractedBookCoverFileName(files); + const found = findPreferredBookCoverFileName(files.filter((fileName) => !isCustomBookCoverFileName(fileName))); return found ? join(dir, found) : null; } diff --git a/server/src/modules/koreader/koreader-catalog.service.test.ts b/server/src/modules/koreader/koreader-catalog.service.test.ts index b0f1c7e4e..dece3a6d9 100644 --- a/server/src/modules/koreader/koreader-catalog.service.test.ts +++ b/server/src/modules/koreader/koreader-catalog.service.test.ts @@ -145,6 +145,7 @@ function makeService( const bookService = { getDetail: vi.fn().mockResolvedValue(makeDetail()), verifyBookAccess: vi.fn().mockResolvedValue(undefined), + getThumbnailPath: vi.fn().mockResolvedValue(join('/data', 'covers', '10', 'thumbnail.jpg')), bulkSetRating: vi.fn().mockResolvedValue(undefined), verifyFileAccess: vi.fn().mockResolvedValue({ id: 100, @@ -274,7 +275,6 @@ function makeService( getDeviceFileNamingPattern: vi.fn().mockResolvedValue(deviceOrganization), } as never, pluginService as never, - { appDataPath: '/data', bookDockPath: '/data/book-dock' }, ); return { @@ -852,7 +852,7 @@ describe('KoreaderCatalogService', () => { await service.streamThumbnail(makeUser(), 10, reply as never); - expect(bookService.verifyBookAccess).toHaveBeenCalledWith(10, expect.objectContaining({ id: 1 })); + expect(bookService.getThumbnailPath).toHaveBeenCalledWith(10, expect.objectContaining({ id: 1 })); expect(reply.header).toHaveBeenCalledWith('ETag', '"5000"'); expect(reply.type).toHaveBeenCalledWith('image/jpeg'); expect(mockCreateReadStream).toHaveBeenCalledWith(join('/data', 'covers', '10', 'thumbnail.jpg')); @@ -909,6 +909,9 @@ describe('KoreaderCatalogService', () => { mockStat.mockRejectedValueOnce(new Error('missing')); await expect(service.streamThumbnail(makeUser(), 10, makeReply() as never)).rejects.toThrow(NotFoundException); + + bookService.getThumbnailPath.mockResolvedValueOnce(null); + await expect(service.streamThumbnail(makeUser(), 10, makeReply() as never)).rejects.toThrow(NotFoundException); }); it('paginates author sections with a filter and navigation links', async () => { diff --git a/server/src/modules/koreader/koreader-catalog.service.ts b/server/src/modules/koreader/koreader-catalog.service.ts index 3bac810d8..bdcf3b01a 100644 --- a/server/src/modules/koreader/koreader-catalog.service.ts +++ b/server/src/modules/koreader/koreader-catalog.service.ts @@ -3,8 +3,7 @@ import { createReadStream } from 'fs'; import { stat } from 'fs/promises'; import { basename } from 'path'; -import { BadRequestException, Inject, Injectable, InternalServerErrorException, Logger, NotFoundException } from '@nestjs/common'; -import type { ConfigType } from '@nestjs/config'; +import { BadRequestException, Injectable, InternalServerErrorException, Logger, NotFoundException } from '@nestjs/common'; import type { FastifyReply } from 'fastify'; import { DEFAULT_KOREADER_DEVICE_PATTERN, resolveUploadPath } from '@bookorbit/types'; @@ -36,13 +35,11 @@ import type { KoreaderCatalogSort, KoreaderCatalogSortOrder, } from '@bookorbit/types'; -import { bookThumbnailPath } from '../../common/book-cover-storage'; import { MAX_OFFSET_ROWS, isOffsetWithinLimit } from '../../common/constants/pagination.constants'; import { imageContentTypeFromPath } from '../../common/image-content-type'; import type { RequestUser } from '../../common/types/request-user'; import { contentDispositionHeader } from '../../common/utils/content-disposition.utils'; import { sanitizeLogValue } from '../../common/utils/log-sanitize.utils'; -import { storageConfig } from '../../config/config'; import { BookReadService } from '../book/book-read.service'; import { BookService } from '../book/book.service'; import { BrowseCountsService } from '../browse-counts/browse-counts.service'; @@ -151,7 +148,6 @@ export class KoreaderCatalogService { private readonly appSettingsService: AppSettingsService, private readonly koreaderService: KoreaderService, private readonly pluginService: KoreaderPluginService, - @Inject(storageConfig.KEY) private readonly storage: ConfigType, ) {} getRoot(): { sections: KoreaderCatalogEntry[] } { @@ -481,8 +477,8 @@ export class KoreaderCatalogService { } async streamThumbnail(user: RequestUser, bookId: number, reply: FastifyReply, ifNoneMatch?: string): Promise { - await this.bookService.verifyBookAccess(bookId, user); - const thumbnailPath = bookThumbnailPath(this.storage.appDataPath, bookId); + const thumbnailPath = await this.bookService.getThumbnailPath(bookId, user); + if (!thumbnailPath) throw new NotFoundException('No thumbnail'); try { const { mtimeMs } = await stat(thumbnailPath); const etag = `"${Math.floor(mtimeMs)}"`; diff --git a/server/src/modules/metadata/metadata-thumbnail-repair.test.ts b/server/src/modules/metadata/metadata-thumbnail-repair.test.ts new file mode 100644 index 000000000..719be5031 --- /dev/null +++ b/server/src/modules/metadata/metadata-thumbnail-repair.test.ts @@ -0,0 +1,104 @@ +import { ConfigService } from '@nestjs/config'; +import { Test } from '@nestjs/testing'; +import { access, mkdir, mkdtemp, readFile, readdir, rm, writeFile } from 'fs/promises'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import sharp from 'sharp'; + +import { DB } from '../../db'; +import { BookMetadataLockService } from '../book-metadata-lock/book-metadata-lock.service'; +import { BookEmbedderService } from '../embedding/book-embedder.service'; +import { MetadataScoreService } from '../metadata-score/metadata-score.service'; +import { NarratorService } from '../narrator/narrator.service'; +import { ComicMetadataRepository } from './comic-metadata.repository'; +import { MetadataExtractionService } from './metadata-extraction.service'; +import { MetadataService } from './metadata.service'; + +describe('MetadataService thumbnail repair', () => { + let appDataPath: string; + let service: MetadataService; + + async function makeCover(): Promise { + return sharp({ create: { width: 800, height: 1200, channels: 3, background: '#336699' } }) + .jpeg() + .toBuffer(); + } + + beforeEach(async () => { + appDataPath = await mkdtemp(join(tmpdir(), 'bookorbit-thumbnail-repair-')); + const moduleRef = await Test.createTestingModule({ + providers: [ + MetadataService, + { provide: DB, useValue: {} }, + { provide: ConfigService, useValue: { get: () => appDataPath } }, + { provide: MetadataExtractionService, useValue: {} }, + { provide: MetadataScoreService, useValue: { calculateAndSave: vi.fn() } }, + { provide: NarratorService, useValue: {} }, + { provide: ComicMetadataRepository, useValue: {} }, + { provide: BookMetadataLockService, useValue: { isFieldLocked: vi.fn().mockResolvedValue(false) } }, + { provide: BookEmbedderService, useValue: {} }, + ], + }).compile(); + service = moduleRef.get(MetadataService); + }); + + afterEach(async () => { + await rm(appDataPath, { recursive: true, force: true }); + }); + + async function seedCoverDir(bookId: number, coverFileName: string, bytes: Buffer): Promise { + const coverDir = join(appDataPath, 'covers', String(bookId)); + await mkdir(coverDir, { recursive: true }); + await writeFile(join(coverDir, coverFileName), bytes); + return coverDir; + } + + it('reproduces and repairs a missing thumbnail beside an intact custom cover', async () => { + const coverDir = await seedCoverDir(913, 'cover_custom.jpg', await makeCover()); + const thumbnailPath = join(coverDir, 'thumbnail.jpg'); + + await expect(access(thumbnailPath)).rejects.toMatchObject({ code: 'ENOENT' }); + + await expect(service.ensureThumbnailForBook(913)).resolves.toBe(thumbnailPath); + + const metadata = await sharp(await readFile(thumbnailPath)).metadata(); + expect(metadata).toMatchObject({ format: 'jpeg', width: 400, height: 600 }); + }); + + it('repairs from a legacy cover file the cover endpoint still serves', async () => { + const coverDir = await seedCoverDir(914, 'cover.jpg', await makeCover()); + + await expect(service.ensureThumbnailForBook(914)).resolves.toBe(join(coverDir, 'thumbnail.jpg')); + }); + + it('leaves no temporary file behind once the thumbnail is published', async () => { + const coverDir = await seedCoverDir(915, 'cover_extracted.jpg', await makeCover()); + + await service.ensureThumbnailForBook(915); + + await expect(readdir(coverDir)).resolves.toEqual(expect.arrayContaining(['cover_extracted.jpg', 'thumbnail.jpg'])); + await expect(readdir(coverDir)).resolves.toHaveLength(2); + }); + + it('keeps a thumbnail written while the repair was in flight', async () => { + const coverDir = await seedCoverDir(916, 'cover_custom.jpg', await makeCover()); + const thumbnailPath = join(coverDir, 'thumbnail.jpg'); + const existing = Buffer.from('thumbnail written by a concurrent cover update'); + + const repair = service.ensureThumbnailForBook(916); + await writeFile(thumbnailPath, existing); + + await expect(repair).resolves.toBe(thumbnailPath); + await expect(readFile(thumbnailPath)).resolves.toEqual(existing); + }); + + it('resolves null instead of throwing when the cover cannot be decoded', async () => { + await seedCoverDir(917, 'cover_custom.jpg', Buffer.alloc(0)); + + await expect(service.ensureThumbnailForBook(917)).resolves.toBeNull(); + }); + + it('resolves null when the book has no cover directory at all', async () => { + await expect(service.ensureThumbnailForBook(918)).resolves.toBeNull(); + }); +}); diff --git a/server/src/modules/metadata/metadata.service.test.ts b/server/src/modules/metadata/metadata.service.test.ts index 5dfa92342..ba159236e 100644 --- a/server/src/modules/metadata/metadata.service.test.ts +++ b/server/src/modules/metadata/metadata.service.test.ts @@ -1,6 +1,9 @@ vi.mock('fs/promises', () => ({ + access: vi.fn(), + link: vi.fn(), mkdir: vi.fn(), readFile: vi.fn(), + rename: vi.fn(), writeFile: vi.fn(), readdir: vi.fn().mockResolvedValue([]), rm: vi.fn(), @@ -84,7 +87,7 @@ vi.mock('./extractors/audio.extractor', () => ({ probeAudioChapters: vi.fn().mockImplementation(() => Promise.resolve({ chapters: [], durationMs: null })), })); -import { mkdir, readFile, readdir, rm, writeFile } from 'fs/promises'; +import { access, link, mkdir, readFile, readdir, rename, rm, writeFile } from 'fs/promises'; import { Logger } from '@nestjs/common'; import { authors, bookAuthors, bookGenres, bookMetadata, books, bookTags, genres, tags } from '../../db/schema'; @@ -101,6 +104,9 @@ import { METADATA_AUTHORS_REPLACED } from './metadata-events.service'; import { MetadataService } from './metadata.service'; import { MetadataExtractionService } from './metadata-extraction.service'; +const mockAccess = access as MockedFunction; +const mockLink = link as MockedFunction; +const mockRename = rename as MockedFunction; const mockMkdir = mkdir as MockedFunction; const mockReadFile = readFile as MockedFunction; const mockWriteFile = writeFile as MockedFunction; @@ -173,6 +179,9 @@ describe('MetadataService', () => { config.get.mockReturnValue('/books'); embedder.embedBook.mockResolvedValue(undefined); + mockAccess.mockRejectedValue(Object.assign(new Error('missing'), { code: 'ENOENT' })); + mockLink.mockResolvedValue(undefined); + mockRename.mockResolvedValue(undefined); mockMkdir.mockResolvedValue(undefined); mockReadFile.mockResolvedValue(''); mockWriteFile.mockResolvedValue(undefined); @@ -242,6 +251,37 @@ describe('MetadataService', () => { ); } + function stubEpubCoverExtraction(cover: Buffer): void { + mockExtractEpubMetadata.mockResolvedValueOnce({ + title: 'Refreshable book', + subtitle: null, + description: null, + isbn10: null, + isbn13: null, + publisher: null, + publishedYear: null, + language: null, + seriesName: null, + seriesIndex: null, + authors: [], + narrators: [], + genres: [], + tags: [], + rating: null, + pageCount: null, + googleBooksId: null, + goodreadsId: null, + amazonId: null, + hardcoverId: null, + hardcoverEditionId: null, + openLibraryId: null, + ranobedbId: null, + itunesId: null, + coverBuffer: null, + }); + mockExtractEpubCover.mockResolvedValueOnce(cover); + } + it('downloadAndSaveCover writes cover/thumbnail and updates metadata when download is valid', async () => { const { db, updateSet } = makeDb(); const service = makeService(db); @@ -275,37 +315,6 @@ describe('MetadataService', () => { // The Kobo CoverImageId is versioned from coverUpdatedAt, so it must move whenever the served // image does, and stay put when it does not (issue #943). describe('coverUpdatedAt stamping', () => { - function stubEpubCoverExtraction(cover: Buffer): void { - mockExtractEpubMetadata.mockResolvedValueOnce({ - title: 'Refreshable book', - subtitle: null, - description: null, - isbn10: null, - isbn13: null, - publisher: null, - publishedYear: null, - language: null, - seriesName: null, - seriesIndex: null, - authors: [], - narrators: [], - genres: [], - tags: [], - rating: null, - pageCount: null, - googleBooksId: null, - goodreadsId: null, - amazonId: null, - hardcoverId: null, - hardcoverEditionId: null, - openLibraryId: null, - ranobedbId: null, - itunesId: null, - coverBuffer: null, - }); - mockExtractEpubCover.mockResolvedValueOnce(cover); - } - it('stamps coverUpdatedAt when an overwriting extraction replaces the cover', async () => { const { db, updateSet } = makeDb(); const service = makeService(db); @@ -399,6 +408,157 @@ describe('MetadataService', () => { expect(db.update).not.toHaveBeenCalledWith(books); }); + it('refreshCoverForBook repairs a missing thumbnail from the active custom cover', async () => { + const { db, selectLimit, updateSet } = makeDb(); + const service = makeService(db); + selectLimit.mockResolvedValue([{ coverSource: 'custom' }]); + mockReaddir.mockResolvedValue(['cover_custom.jpg', 'cover_extracted.png']); + mockReadFile.mockResolvedValue(Buffer.from('custom-cover-bytes')); + stubEpubCoverExtraction(Buffer.from('extracted-cover-bytes')); + + await expect(service.refreshCoverForBook(14, '/book.epub', 'epub')).resolves.toBe(true); + + expect(mockReadFile).toHaveBeenCalledWith('/books/covers/14/cover_custom.jpg'); + expect(mockGenerateThumbnail).toHaveBeenCalledWith(Buffer.from('custom-cover-bytes')); + expect(mockWriteFile).toHaveBeenCalledWith('/books/covers/14/thumbnail.jpg', Buffer.from('thumbnail-bytes')); + expect(updateSet).toHaveBeenCalledWith({ coverUpdatedAt: expect.any(Date) }); + expect(db.update).toHaveBeenCalledWith(books); + }); + + it('refreshCoverForBook promotes the extracted cover when the stored custom cover is missing', async () => { + const { db, selectLimit, updateSet } = makeDb(); + const service = makeService(db); + selectLimit.mockResolvedValue([{ coverSource: 'custom' }]); + mockReaddir.mockResolvedValue(['cover_extracted.jpg']); + stubEpubCoverExtraction(Buffer.from('extracted-cover-bytes')); + + await expect(service.refreshCoverForBook(16, '/book.epub', 'epub')).resolves.toBe(true); + + expect(mockWriteFile).toHaveBeenCalledWith('/books/covers/16/thumbnail.jpg', Buffer.from('thumbnail-bytes')); + expect(updateSet).toHaveBeenCalledWith({ coverSource: 'extracted', updatedAt: expect.any(Date) }); + }); + + it('refreshCoverForBook keeps the custom cover source when the cover directory reads back empty', async () => { + const { db, selectLimit, updateSet } = makeDb(); + const service = makeService(db); + selectLimit.mockResolvedValue([{ coverSource: 'custom' }]); + // An unmounted cover volume is recreated by mkdir and then lists empty, which must not be read + // as "the user deleted their custom cover". + mockReaddir.mockResolvedValue([]); + stubEpubCoverExtraction(Buffer.from('extracted-cover-bytes')); + + await expect(service.refreshCoverForBook(19, '/book.epub', 'epub')).resolves.toBe(true); + + expect(updateSet).not.toHaveBeenCalledWith({ coverSource: 'extracted', updatedAt: expect.any(Date) }); + }); + + it('ensureThumbnailForBook lazily repairs a missing thumbnail from the preferred cover', async () => { + const { db } = makeDb(); + const service = makeService(db); + mockReaddir.mockResolvedValue(['cover_extracted.png', 'cover_custom.jpg']); + mockReadFile.mockResolvedValue(Buffer.from('custom-cover-bytes')); + + await expect(service.ensureThumbnailForBook(15)).resolves.toBe('/books/covers/15/thumbnail.jpg'); + + expect(mockReadFile).toHaveBeenCalledWith('/books/covers/15/cover_custom.jpg'); + expect(mockGenerateThumbnail).toHaveBeenCalledWith(Buffer.from('custom-cover-bytes')); + + // The thumbnail is staged in the cover directory and hard-linked into place, so an interrupted + // write can never leave a truncated thumbnail.jpg that later requests would accept. + const [tempPath, tempBytes] = mockWriteFile.mock.calls[0]!; + expect(tempPath).toMatch(/^\/books\/covers\/15\/\.thumbnail-repair-.+\.tmp$/); + expect(tempBytes).toEqual(Buffer.from('thumbnail-bytes')); + expect(mockLink).toHaveBeenCalledWith(tempPath, '/books/covers/15/thumbnail.jpg'); + expect(mockRm).toHaveBeenCalledWith(tempPath, { force: true }); + }); + + it('ensureThumbnailForBook keeps a thumbnail created by a concurrent cover update', async () => { + const { db } = makeDb(); + const service = makeService(db); + mockReaddir.mockResolvedValue(['cover_custom.jpg']); + mockReadFile.mockResolvedValue(Buffer.from('custom-cover-bytes')); + mockLink.mockRejectedValue(Object.assign(new Error('already exists'), { code: 'EEXIST' })); + + await expect(service.ensureThumbnailForBook(17)).resolves.toBe('/books/covers/17/thumbnail.jpg'); + expect(mockWriteFile).not.toHaveBeenCalledWith('/books/covers/17/thumbnail.jpg', expect.anything(), expect.anything()); + }); + + it('ensureThumbnailForBook publishes by rename when the filesystem has no hard links', async () => { + const { db } = makeDb(); + const service = makeService(db); + mockReaddir.mockResolvedValue(['cover_custom.jpg']); + mockReadFile.mockResolvedValue(Buffer.from('custom-cover-bytes')); + mockLink.mockRejectedValue(Object.assign(new Error('operation not permitted'), { code: 'EPERM' })); + + await expect(service.ensureThumbnailForBook(26)).resolves.toBe('/books/covers/26/thumbnail.jpg'); + + // The bytes must reach the served path through a rename of a fully written file, never a direct + // write, so an interrupted repair cannot leave a truncated thumbnail that nothing revalidates. + const [tempPath] = mockWriteFile.mock.calls[0]!; + expect(tempPath).toMatch(/^\/books\/covers\/26\/\.thumbnail-repair-.+\.tmp$/); + expect(mockRename).toHaveBeenCalledWith(tempPath, '/books/covers/26/thumbnail.jpg'); + expect(mockWriteFile).not.toHaveBeenCalledWith('/books/covers/26/thumbnail.jpg', expect.anything(), expect.anything()); + }); + + it('ensureThumbnailForBook leaves a published thumbnail alone when hard links are unavailable', async () => { + const { db } = makeDb(); + const service = makeService(db); + mockReaddir.mockResolvedValue(['cover_custom.jpg']); + mockReadFile.mockResolvedValue(Buffer.from('custom-cover-bytes')); + mockLink.mockRejectedValue(Object.assign(new Error('operation not permitted'), { code: 'EPERM' })); + mockAccess.mockResolvedValue(undefined); + + await expect(service.ensureThumbnailForBook(27)).resolves.toBe('/books/covers/27/thumbnail.jpg'); + + expect(mockRename).not.toHaveBeenCalled(); + }); + + it('ensureThumbnailForBook resolves null instead of throwing when the cover cannot be read', async () => { + const { db } = makeDb(); + const service = makeService(db); + mockReaddir.mockResolvedValue(['cover_custom.jpg']); + mockReadFile.mockResolvedValue(Buffer.from('custom-cover-bytes')); + mockGenerateThumbnail.mockRejectedValueOnce(new Error('Input buffer contains unsupported image format')); + + await expect(service.ensureThumbnailForBook(18)).resolves.toBeNull(); + }); + + it('ensureThumbnailForBook bounds concurrent thumbnail generation', async () => { + const { db } = makeDb(); + const service = makeService(db); + let active = 0; + let peak = 0; + const releases: Array<() => void> = []; + mockReaddir.mockResolvedValue(['cover_custom.jpg']); + mockReadFile.mockImplementation( + () => + new Promise((resolve) => { + active++; + peak = Math.max(peak, active); + releases.push(() => { + active--; + resolve(Buffer.from('custom-cover-bytes')); + }); + }), + ); + + const repairs = [21, 22, 23, 24, 25].map((bookId) => service.ensureThumbnailForBook(bookId)); + await vi.waitFor(() => expect(releases).toHaveLength(4)); + + expect(peak).toBe(4); + releases.splice(0).forEach((release) => release()); + await vi.waitFor(() => expect(releases).toHaveLength(1)); + releases.splice(0).forEach((release) => release()); + + await expect(Promise.all(repairs)).resolves.toEqual([ + '/books/covers/21/thumbnail.jpg', + '/books/covers/22/thumbnail.jpg', + '/books/covers/23/thumbnail.jpg', + '/books/covers/24/thumbnail.jpg', + '/books/covers/25/thumbnail.jpg', + ]); + }); + it('downloadAndSaveCover no-ops on empty payloads and network failures', async () => { const { db } = makeDb(); const service = makeService(db); diff --git a/server/src/modules/metadata/metadata.service.ts b/server/src/modules/metadata/metadata.service.ts index 994c93b4d..74cae3230 100644 --- a/server/src/modules/metadata/metadata.service.ts +++ b/server/src/modules/metadata/metadata.service.ts @@ -2,15 +2,18 @@ import { Inject, Injectable, Logger, Optional } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { and, eq, inArray, isNull, sql } from 'drizzle-orm'; import { NodePgDatabase } from 'drizzle-orm/node-postgres'; -import { mkdir, readdir, rm, writeFile } from 'fs/promises'; +import { randomUUID } from 'crypto'; +import { access, link, mkdir, readFile, readdir, rename, rm, writeFile } from 'fs/promises'; import { join } from 'path'; import { DB } from '../../db'; import * as schema from '../../db/schema'; import { COVER_EXTRACTED_FILE_PREFIX, + COVER_THUMBNAIL_FILE_NAME, bookCoverDirPath, bookThumbnailPath, + findPreferredBookCoverFileName, isCustomBookCoverFileName, isExtractedBookCoverFileName, } from '../../common/book-cover-storage'; @@ -51,6 +54,7 @@ interface RelationMutationOptions { const MAX_RELATION_NAME_LENGTH = 200; const EXTRACTED_COVER_SOURCE = 'extracted'; +const THUMBNAIL_REPAIR_CONCURRENCY = 4; const MIN_PUBLISHED_YEAR = 1000; const MAX_PUBLISHED_YEAR = 2200; const NORMALIZED_AUTHOR_NAME_SQL = normalizeMetadataTextKeySql(authors.name); @@ -67,6 +71,9 @@ function normalizePublishedYear(year: number | null | undefined): number | null export class MetadataService { private readonly logger = new Logger(MetadataService.name); private readonly appDataPath: string; + private readonly thumbnailRepairs = new Map>(); + private readonly thumbnailRepairWaiters: Array<() => void> = []; + private activeThumbnailRepairs = 0; constructor( @Inject(DB) private readonly db: Db, @@ -260,6 +267,24 @@ export class MetadataService { return true; } + /** + * Regenerates a thumbnail that went missing beside an intact cover (issue #1475). Resolves to + * null rather than throwing, so a cover that sharp cannot read stays a 404 placeholder in the + * grid instead of failing the request. + */ + async ensureThumbnailForBook(bookId: number): Promise { + const existing = this.thumbnailRepairs.get(bookId); + if (existing) return existing; + + const pending = this.withThumbnailRepairSlot(() => this.repairMissingThumbnail(bookId)); + this.thumbnailRepairs.set(bookId, pending); + try { + return await pending; + } finally { + if (this.thumbnailRepairs.get(bookId) === pending) this.thumbnailRepairs.delete(bookId); + } + } + // ── Audio helpers ──────────────────────────────────────────────────────────── /** @@ -859,7 +884,13 @@ export class MetadataService { .from(bookMetadata) .where(eq(bookMetadata.bookId, bookId)) .limit(1); - const preserveCustom = !overwrite && currentCover?.coverSource === 'custom'; + const customCoverFileName = files.find(isCustomBookCoverFileName); + const storedCustomCoverMissing = !overwrite && currentCover?.coverSource === 'custom' && customCoverFileName === undefined; + const preserveCustom = !overwrite && currentCover?.coverSource === 'custom' && customCoverFileName !== undefined; + // An empty listing cannot tell a deleted custom cover apart from cover storage that failed to + // mount, and mkdir above recreates the directory either way. Only demote the stored source when + // some other artifact proves the directory is the real one. + const replaceMissingCustom = storedCustomCoverMissing && files.length > 0; const staleCoverFiles = files.filter( (fileName) => isExtractedBookCoverFileName(fileName) || (!preserveCustom && isCustomBookCoverFileName(fileName)), @@ -868,14 +899,21 @@ export class MetadataService { await writeFile(join(dir, `${COVER_EXTRACTED_FILE_PREFIX}${ext}`), bytes); - if (!preserveCustom) { + let thumbnailUpdated = false; + if (preserveCustom && customCoverFileName && !files.includes(COVER_THUMBNAIL_FILE_NAME)) { + const customCover = await readFile(join(dir, customCoverFileName)); + const thumbnail = await generateThumbnail(customCover); + await writeFile(bookThumbnailPath(this.appDataPath, bookId), thumbnail); + thumbnailUpdated = true; + } else if (!preserveCustom) { const thumbnail = await generateThumbnail(bytes); await writeFile(bookThumbnailPath(this.appDataPath, bookId), thumbnail); + thumbnailUpdated = true; } const now = new Date(); - if (overwrite) { + if (overwrite || replaceMissingCustom) { await this.db.update(bookMetadata).set({ coverSource: EXTRACTED_COVER_SOURCE, updatedAt: now }).where(eq(bookMetadata.bookId, bookId)); } else { await this.db @@ -884,14 +922,116 @@ export class MetadataService { .where(and(eq(bookMetadata.bookId, bookId), isNull(bookMetadata.coverSource))); } - if (!preserveCustom) { - // The extracted cover and its thumbnail were just rewritten, so the served image moved even - // when the first-writer-wins update above matched no row. + if (thumbnailUpdated) { + // A served cover artifact was rewritten, so its cache version must move even when the + // first-writer-wins update above matched no row. await this.db.update(bookMetadata).set({ coverUpdatedAt: now }).where(eq(bookMetadata.bookId, bookId)); await this.db.update(books).set({ updatedAt: now }).where(eq(books.id, bookId)); } } + private async repairMissingThumbnail(bookId: number): Promise { + const event = 'metadata.thumbnail_repair'; + const startedAt = Date.now(); + const dir = bookCoverDirPath(this.appDataPath, bookId); + const thumbnailPath = bookThumbnailPath(this.appDataPath, bookId); + + try { + const files = await readdir(dir); + if (files.includes(COVER_THUMBNAIL_FILE_NAME)) return thumbnailPath; + + const coverFileName = findPreferredBookCoverFileName(files); + if (!coverFileName) { + this.logger.debug( + `[${event}] [end] bookId=${bookId} durationMs=${Date.now() - startedAt} repaired=false coverFound=false - thumbnail repair skipped`, + ); + return null; + } + + const cover = await readFile(join(dir, coverFileName)); + const thumbnail = await generateThumbnail(cover); + await this.publishRepairedThumbnail(dir, thumbnailPath, thumbnail); + this.logger.log( + `[${event}] [end] bookId=${bookId} durationMs=${Date.now() - startedAt} repaired=true source="${sanitizeLogValue(coverFileName)}" - thumbnail repair completed`, + ); + return thumbnailPath; + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code === 'ENOENT' || code === 'ENOTDIR') { + this.logger.debug( + `[${event}] [end] bookId=${bookId} durationMs=${Date.now() - startedAt} repaired=false coverFound=false - thumbnail repair skipped`, + ); + return null; + } + const errorClass = error instanceof Error ? error.name : 'Error'; + const errorMessage = sanitizeLogValue(error instanceof Error ? error.message : String(error)); + this.logger.warn( + `[${event}] [fail] bookId=${bookId} durationMs=${Date.now() - startedAt} errorClass=${errorClass} error="${errorMessage}" - thumbnail repair failed`, + ); + return null; + } + } + + /** + * Publishes the repaired thumbnail from a fully written temporary file, so an interrupted write + * can never leave a truncated thumbnail at the served path. Nothing validates a thumbnail beyond + * its filename, so a truncated one would be permanent. + * + * A hard link is preferred because it also fails when the path already exists, leaving a cover + * write that landed first in possession of its own thumbnail. Network mounts without hard links + * fall back to rename, which is still atomic and so still cannot publish a partial file; it only + * loses the exclusive create, and losing that race costs a thumbnail that is stale by + * milliseconds rather than one that is broken forever. + */ + private async publishRepairedThumbnail(dir: string, thumbnailPath: string, bytes: Buffer): Promise { + const tempPath = join(dir, `.thumbnail-repair-${randomUUID()}.tmp`); + try { + await writeFile(tempPath, bytes); + try { + await link(tempPath, thumbnailPath); + return; + } catch (error) { + if ((error as NodeJS.ErrnoException | undefined)?.code === 'EEXIST') return; + } + + try { + await access(thumbnailPath); + return; + } catch { + // No thumbnail published yet, so this repair still has something to contribute. + } + await rename(tempPath, thumbnailPath); + } finally { + await rm(tempPath, { force: true }); + } + } + + private async withThumbnailRepairSlot(operation: () => Promise): Promise { + await this.acquireThumbnailRepairSlot(); + try { + return await operation(); + } finally { + this.releaseThumbnailRepairSlot(); + } + } + + private async acquireThumbnailRepairSlot(): Promise { + if (this.activeThumbnailRepairs < THUMBNAIL_REPAIR_CONCURRENCY) { + this.activeThumbnailRepairs++; + return; + } + await new Promise((resolve) => this.thumbnailRepairWaiters.push(resolve)); + } + + private releaseThumbnailRepairSlot(): void { + const next = this.thumbnailRepairWaiters.shift(); + if (next) { + next(); + return; + } + this.activeThumbnailRepairs--; + } + private normalizeUniqueRelationNames(values: string[]): string[] { const names = values.map((value) => { const truncated = normalizeMetadataText(value)?.substring(0, MAX_RELATION_NAME_LENGTH); diff --git a/server/src/modules/opds/opds.controller.test.ts b/server/src/modules/opds/opds.controller.test.ts index 4ed481468..a50ce09dc 100644 --- a/server/src/modules/opds/opds.controller.test.ts +++ b/server/src/modules/opds/opds.controller.test.ts @@ -55,12 +55,16 @@ function makeController() { const bookService = { resolveDownloadFilename: vi.fn().mockResolvedValue('BadTitle - Author.epub'), } as never; + const metadataService = { + ensureThumbnailForBook: vi.fn().mockResolvedValue(null), + }; return { - controller: new OpdsController(opdsService, opdsBookService, config, bookService), + controller: new OpdsController(opdsService, opdsBookService, config, bookService, metadataService as never), opdsService, opdsBookService, bookService, + metadataService, }; } @@ -319,11 +323,35 @@ describe('OpdsController', () => { expect(mockCreateReadStream).not.toHaveBeenCalled(); }); - it('throws NotFoundException when thumbnail file is missing', async () => { - const { controller } = makeController(); - mockStat.mockRejectedValue(new Error('missing thumbnail')); + it('serves a thumbnail rebuilt on demand when the file is missing beside an intact cover', async () => { + const { controller, metadataService } = makeController(); + const reply = makeReply(); + const stream = { kind: 'repaired-thumbnail-stream' }; + mockStat.mockRejectedValueOnce(Object.assign(new Error('missing thumbnail'), { code: 'ENOENT' })).mockResolvedValueOnce({ mtimeMs: 7777 }); + metadataService.ensureThumbnailForBook.mockResolvedValueOnce('/books/covers/12/thumbnail.jpg'); + mockCreateReadStream.mockReturnValue(stream); + + await controller.thumbnail(12, { userId: 1, isSuperuser: false } as never, reply); + + expect(metadataService.ensureThumbnailForBook).toHaveBeenCalledWith(12); + expect(reply.header).toHaveBeenCalledWith('ETag', '"7777"'); + expect(reply.send).toHaveBeenCalledWith(stream); + }); + + it('does not attempt a repair when the thumbnail stat fails for a reason other than a missing file', async () => { + const { controller, metadataService } = makeController(); + mockStat.mockRejectedValue(Object.assign(new Error('permission denied'), { code: 'EACCES' })); await expect(controller.thumbnail(42, { userId: 7, isSuperuser: false } as never, makeReply())).rejects.toThrow(NotFoundException); + expect(metadataService.ensureThumbnailForBook).not.toHaveBeenCalled(); + }); + + it('throws NotFoundException when thumbnail file is missing and no cover can rebuild it', async () => { + const { controller, metadataService } = makeController(); + mockStat.mockRejectedValue(Object.assign(new Error('missing thumbnail'), { code: 'ENOENT' })); + + await expect(controller.thumbnail(42, { userId: 7, isSuperuser: false } as never, makeReply())).rejects.toThrow(NotFoundException); + expect(metadataService.ensureThumbnailForBook).toHaveBeenCalledWith(42); }); it('downloads file with sanitized attachment name', async () => { diff --git a/server/src/modules/opds/opds.controller.ts b/server/src/modules/opds/opds.controller.ts index beebd4340..49c5f1953 100644 --- a/server/src/modules/opds/opds.controller.ts +++ b/server/src/modules/opds/opds.controller.ts @@ -30,6 +30,7 @@ import { OpdsUser } from './opds-user.decorator'; import { OpdsBookService } from './opds-book.service'; import { OpdsService } from './opds.service'; import { BookService } from '../book/book.service'; +import { MetadataService } from '../metadata/metadata.service'; @Controller('opds') @Public() @@ -42,6 +43,7 @@ export class OpdsController { private readonly opdsBookService: OpdsBookService, private readonly config: ConfigService, private readonly bookService: BookService, + private readonly metadataService: MetadataService, ) { this.appDataPath = this.config.get('storage.appDataPath')!; } @@ -246,20 +248,34 @@ export class OpdsController { await this.opdsBookService.validateBookAccess(bookId, user.userId, user.isSuperuser, user.contentFilters); reply.header('Cross-Origin-Resource-Policy', 'cross-origin'); const thumbnailPath = bookThumbnailPath(this.appDataPath, bookId); + let mtimeMs: number; try { - const { mtimeMs } = await stat(thumbnailPath); - const etag = `"${Math.floor(mtimeMs)}"`; - if (ifNoneMatch === etag) { - reply.status(304).send(); - return; + ({ mtimeMs } = await stat(thumbnailPath)); + } catch (error) { + // Only a missing thumbnail is repairable. Every other failure keeps the answer this route has + // always given, without spending a cover read and a resize to arrive at it. + const code = (error as NodeJS.ErrnoException | undefined)?.code; + if (code !== 'ENOENT' && code !== 'ENOTDIR') throw new NotFoundException('No thumbnail'); + + // A thumbnail can be missing beside an intact cover (issue #1475); rebuild it once on demand. + const repaired = await this.metadataService.ensureThumbnailForBook(bookId); + if (!repaired) throw new NotFoundException('No thumbnail'); + try { + ({ mtimeMs } = await stat(repaired)); + } catch { + throw new NotFoundException('No thumbnail'); } - reply.header('Cache-Control', 'no-cache'); - reply.header('ETag', etag); - reply.type('image/jpeg'); - reply.send(createReadStream(thumbnailPath)); - } catch { - throw new NotFoundException('No thumbnail'); } + + const etag = `"${Math.floor(mtimeMs)}"`; + if (ifNoneMatch === etag) { + reply.status(304).send(); + return; + } + reply.header('Cache-Control', 'no-cache'); + reply.header('ETag', etag); + reply.type('image/jpeg'); + reply.send(createReadStream(thumbnailPath)); } @Get(':bookId/download') diff --git a/server/src/modules/opds/opds.module.test.ts b/server/src/modules/opds/opds.module.test.ts index 98e5cdd8e..ae5a5a2f1 100644 --- a/server/src/modules/opds/opds.module.test.ts +++ b/server/src/modules/opds/opds.module.test.ts @@ -5,6 +5,7 @@ import { MODULE_METADATA } from '@nestjs/common/constants'; import { CommonModule } from '../../common/common.module'; import { AppSettingsModule } from '../app-settings/app-settings.module'; import { BookModule } from '../book/book.module'; +import { MetadataModule } from '../metadata/metadata.module'; import { UserModule } from '../user/user.module'; import { OpdsAuthGuard } from './opds-auth.guard'; import { OpdsBookService } from './opds-book.service'; @@ -17,7 +18,13 @@ import { OpdsUserService } from './opds-user.service'; describe('OpdsModule', () => { it('registers expected module wiring', () => { - expect(Reflect.getMetadata(MODULE_METADATA.IMPORTS, OpdsModule)).toEqual([AppSettingsModule, BookModule, UserModule, CommonModule]); + expect(Reflect.getMetadata(MODULE_METADATA.IMPORTS, OpdsModule)).toEqual([ + AppSettingsModule, + BookModule, + MetadataModule, + UserModule, + CommonModule, + ]); expect(Reflect.getMetadata(MODULE_METADATA.CONTROLLERS, OpdsModule)).toEqual([OpdsController, OpdsUserController]); expect(Reflect.getMetadata(MODULE_METADATA.PROVIDERS, OpdsModule)).toEqual([ OpdsService, diff --git a/server/src/modules/opds/opds.module.ts b/server/src/modules/opds/opds.module.ts index 95b00ff68..7390c33fc 100644 --- a/server/src/modules/opds/opds.module.ts +++ b/server/src/modules/opds/opds.module.ts @@ -2,6 +2,7 @@ import { Module } from '@nestjs/common'; import { AppSettingsModule } from '../app-settings/app-settings.module'; import { BookModule } from '../book/book.module'; +import { MetadataModule } from '../metadata/metadata.module'; import { UserModule } from '../user/user.module'; import { CommonModule } from '../../common/common.module'; import { OpdsAuthGuard } from './opds-auth.guard'; @@ -13,7 +14,7 @@ import { OpdsUserController } from './opds-user.controller'; import { OpdsUserService } from './opds-user.service'; @Module({ - imports: [AppSettingsModule, BookModule, UserModule, CommonModule], + imports: [AppSettingsModule, BookModule, MetadataModule, UserModule, CommonModule], controllers: [OpdsController, OpdsUserController], providers: [OpdsService, OpdsBookService, OpdsUserService, OpdsAuthGuard, OpdsEnabledGuard], exports: [OpdsBookService],