From f6da5ad25e1985109f6c749dd10318dcd545978c Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Thu, 10 Sep 2026 13:30:12 +0200 Subject: [PATCH] fix(video): avoid a SQLite connection deadlock in the placeholder fallback generateVideoPlaceholder() unconditionally called getThumbnailSettings(), which queries the database directly (not through any active transaction). videoProcessor.js's new placeholder fallback can run from inside processUploadedPhotos' open per-file SQLite transaction (chunked video upload) -- knex's default SQLite pool has exactly one connection, so that second, un-transacted query deadlocks against the transaction holding it, timing out after acquireConnectionTimeout (60s). Reproduced directly against an isolated SQLite db. generateVideoPlaceholder now skips the settings lookup entirely when the caller supplies explicit width/height, and the video fallback passes the same DEFAULT_THUMBNAIL_WIDTH/HEIGHT the settings lookup would have fallen back to anyway (now exported for reuse). Found by codex review. --- .../videoPlaceholderNoDbLookup.test.js | 62 +++++++++++++++++++ .../services/__tests__/videoProcessor.test.js | 9 ++- backend/src/services/imageProcessor.js | 14 ++++- backend/src/services/videoProcessor.js | 15 ++++- 4 files changed, 93 insertions(+), 7 deletions(-) create mode 100644 backend/__tests__/services/videoPlaceholderNoDbLookup.test.js 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; }