diff --git a/backend/__tests__/services/videoPlaceholderNoDbLookup.test.js b/backend/__tests__/services/videoPlaceholderNoDbLookup.test.js new file mode 100644 index 00000000..78efac31 --- /dev/null +++ b/backend/__tests__/services/videoPlaceholderNoDbLookup.test.js @@ -0,0 +1,62 @@ +/** + * generateVideoPlaceholder() must not touch the database when the caller + * already supplies width/height (videoProcessor.js's thumbnail-generation + * fallback does exactly this). + * + * Why it matters: processUploadedPhotos() (chunked video upload) holds a + * per-file SQLite transaction open across thumbnail generation. SQLite's + * knex pool defaults to a single connection, so any second, un-transacted + * db() query made while that transaction is open blocks until + * acquireConnectionTimeout (60s in production) — verified directly against + * an isolated SQLite db (codex review of #1371/#1372). Passing explicit + * dimensions must skip getThumbnailSettings()'s db() call entirely, not + * just tolerate its failure. + */ + +const path = require('path'); +const fs = require('fs').promises; +const os = require('os'); + +const mockDbSpy = jest.fn(() => { + throw new Error('db() must not be called when width/height are supplied'); +}); +jest.mock('../../src/database/db', () => ({ db: (...args) => mockDbSpy(...args) })); + +const storageModule = require('../../src/services/storage'); +const LocalFsStorage = require('../../src/services/storage/LocalFsStorage'); + +describe('generateVideoPlaceholder skips the settings DB lookup given explicit dimensions', () => { + let storage; + let root; + let imageProcessor; + + beforeAll(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-vidplaceholder-')); + storage = new LocalFsStorage({ root }); + await storage.init(); + storageModule.setStorageForTesting(storage); + imageProcessor = require('../../src/services/imageProcessor'); + }, 30000); + + afterAll(async () => { + storageModule.resetStorage(); + await fs.rm(root, { recursive: true, force: true }).catch(() => {}); + }); + + afterEach(() => mockDbSpy.mockClear()); + + it('never calls db() when width/height are provided', async () => { + const key = await imageProcessor.generateVideoPlaceholder('demo.mp4', { width: 300, height: 300 }); + + expect(key).toBe('thumbnails/thumb_demo.jpg'); + expect(await storage.exists(key)).toBe(true); + expect(mockDbSpy).not.toHaveBeenCalled(); + }); + + it('falls through to defaults (not a throw) when db() fails and no dimensions were given', async () => { + const key = await imageProcessor.generateVideoPlaceholder('demo2.mp4'); + + expect(key).toBe('thumbnails/thumb_demo2.jpg'); + expect(mockDbSpy).toHaveBeenCalled(); + }); +}); diff --git a/backend/src/services/__tests__/videoProcessor.test.js b/backend/src/services/__tests__/videoProcessor.test.js index cf650a2b..ec88190c 100644 --- a/backend/src/services/__tests__/videoProcessor.test.js +++ b/backend/src/services/__tests__/videoProcessor.test.js @@ -4,7 +4,9 @@ jest.mock('../storage', () => ({ getStorage: jest.fn() })); jest.mock('../imageProcessor', () => ({ - generateVideoPlaceholder: jest.fn() + generateVideoPlaceholder: jest.fn(), + DEFAULT_THUMBNAIL_WIDTH: 300, + DEFAULT_THUMBNAIL_HEIGHT: 300 })); const ffmpeg = require('fluent-ffmpeg'); @@ -98,7 +100,10 @@ describe('processUploadedVideo degrades gracefully instead of rejecting the whol expect(result.metadata).toEqual(expect.objectContaining({ duration: 5, videoCodec: 'h264' })); // thumbnailKey is always thumbnails/thumb_.jpg — strip the prefix // back to a filename so generateVideoPlaceholder recomputes the same key. - expect(generateVideoPlaceholder).toHaveBeenCalledWith('wedding_001.jpg'); + // Explicit width/height so generateVideoPlaceholder skips its DB-backed + // settings lookup — this can run inside an open per-file SQLite + // transaction (chunked video upload), where that lookup deadlocks. + expect(generateVideoPlaceholder).toHaveBeenCalledWith('wedding_001.jpg', { width: 300, height: 300 }); expect(result.thumbnailKey).toBe('thumbnails/thumb_placeholder.jpg'); expect(storage.putFromFile).not.toHaveBeenCalled(); }); diff --git a/backend/src/services/imageProcessor.js b/backend/src/services/imageProcessor.js index 9ba0b554..9485b403 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -369,9 +369,15 @@ async function generateVideoPlaceholder(originalFilename, options = {}) { const thumbnailRelKey = path.posix.join('thumbnails', thumbnailFilename); const storage = getStorage(); - const settings = await getThumbnailSettings(); - const width = settings.width || DEFAULT_THUMBNAIL_WIDTH; - const height = settings.height || DEFAULT_THUMBNAIL_HEIGHT; + // Skip the settings lookup when the caller already supplies dimensions. + // This can run from inside an open per-file SQLite transaction (chunked + // video upload's fallback path in videoProcessor.js) — a second, + // un-transacted db() query for settings there deadlocks against SQLite's + // single-connection pool until acquireConnectionTimeout (60s), reproduced + // directly against an isolated SQLite db (codex review of #1371/#1372). + const settings = (options.width && options.height) ? {} : await getThumbnailSettings(); + const width = options.width || settings.width || DEFAULT_THUMBNAIL_WIDTH; + const height = options.height || settings.height || DEFAULT_THUMBNAIL_HEIGHT; if (options.regenerate) { await storage.delete(thumbnailRelKey).catch(() => {}); @@ -864,4 +870,6 @@ module.exports = { ensurePreviewImage, extractCaptureDate, withLocalCopy, + DEFAULT_THUMBNAIL_WIDTH, + DEFAULT_THUMBNAIL_HEIGHT, }; diff --git a/backend/src/services/videoProcessor.js b/backend/src/services/videoProcessor.js index 803e351f..21182532 100644 --- a/backend/src/services/videoProcessor.js +++ b/backend/src/services/videoProcessor.js @@ -190,9 +190,20 @@ async function processUploadedVideo(videoPath, thumbnailKey, options = {}) { // a filename so generateVideoPlaceholder recomputes this exact same key. if (!generatedThumbnailKey) { try { - const { generateVideoPlaceholder } = require('./imageProcessor'); + const { + generateVideoPlaceholder, + DEFAULT_THUMBNAIL_WIDTH, + DEFAULT_THUMBNAIL_HEIGHT + } = require('./imageProcessor'); const placeholderFilename = path.basename(thumbnailKey).replace(/^thumb_/, ''); - const placeholderKey = await generateVideoPlaceholder(placeholderFilename); + // Explicit width/height make generateVideoPlaceholder skip its + // configured-thumbnail-size DB lookup (see its own comment) — this + // call can run from inside processUploadedPhotos' open per-file + // SQLite transaction, where that lookup would otherwise deadlock. + const placeholderKey = await generateVideoPlaceholder(placeholderFilename, { + width: DEFAULT_THUMBNAIL_WIDTH, + height: DEFAULT_THUMBNAIL_HEIGHT + }); if (placeholderKey) { generatedThumbnailKey = placeholderKey; }