From 0c45da1a7567f6185311bd8767c74df9fd36a3dd Mon Sep 17 00:00:00 2001 From: fallenbagel <98979876+Fallenbagel@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:21:39 +0800 Subject: [PATCH] fix(requests): serialize requests for the same title Duplicates and overlapping seasons are already rejected, but both checks run well before the insert, so two users asking for the same title at the same moment both got through. Creation now takes a second lock keyed on the title inside the per-user one, always in that order so the two cannot deadlock. is4k is normalized at the same time. It is optional, and an undefined one binds as null in the duplicate query, so an API caller that omitted it could request the same title repeatedly. --- server/entity/MediaRequest.test.ts | 112 +++++++++++++- server/entity/MediaRequest.ts | 19 ++- server/routes/request.test.ts | 44 ++++++ server/routes/request.ts | 232 ++++++++++++++++------------- server/utils/requestLock.ts | 6 + 5 files changed, 299 insertions(+), 114 deletions(-) diff --git a/server/entity/MediaRequest.test.ts b/server/entity/MediaRequest.test.ts index 007cba51c3..27a70b7de2 100644 --- a/server/entity/MediaRequest.test.ts +++ b/server/entity/MediaRequest.test.ts @@ -4,12 +4,15 @@ import { beforeEach, describe, it, mock } from 'node:test'; import ExternalAPI from '@server/api/externalapi'; import { MediaType } from '@server/constants/media'; import { getRepository } from '@server/datasource'; +import Media from '@server/entity/Media'; import { DuplicateMediaRequestError, MediaRequest, QuotaRestrictedError, } from '@server/entity/MediaRequest'; +import SeasonRequest from '@server/entity/SeasonRequest'; import { User } from '@server/entity/User'; +import { Permission } from '@server/lib/permissions'; import { setupTestDb } from '@server/test/db'; // get is a prototype method unlike getMovie, and replaces the cache lookup too @@ -19,15 +22,16 @@ const externalApiGetMock = mock.method( }, 'get', async (endpoint: string) => { - const movieId = Number(endpoint.replace('/movie/', '')); + const tmdbId = Number(endpoint.replace(/^\/(movie|tv)\//, '')); - if (!movieId) { + if (!tmdbId) { throw new Error(`Unstubbed external endpoint: ${endpoint}`); } return { - id: movieId, + id: tmdbId, external_ids: {}, + seasons: [1, 2, 3].map((season_number) => ({ season_number })), // Skips getMovie's localized fallback call videos: { results: [{ type: 'Trailer', key: 'trailer' }] }, }; @@ -53,6 +57,13 @@ async function seedRequester(movieQuotaLimit: number): Promise { return userRepository.save(requester); } +async function createRequester( + email: string, + permissions = Permission.REQUEST +): Promise { + return getRepository(User).save(new User({ email, permissions, avatar: '' })); +} + function requestMovies(mediaIds: number[], requester: User) { return Promise.allSettled( mediaIds.map((mediaId) => @@ -96,4 +107,99 @@ describe('MediaRequest.request', () => { assert.strictEqual(await requestRepository.count(), 1); assert.strictEqual(externalApiGetMock.callCount(), 2); }); + + it('rejects a duplicate request that omits is4k', async () => { + const requestRepository = getRepository(MediaRequest); + const requester = await seedRequester(5); + const body = { mediaId: 66666, mediaType: MediaType.MOVIE }; + + await MediaRequest.request(body, requester); + + await assert.rejects( + () => MediaRequest.request(body, requester), + DuplicateMediaRequestError + ); + assert.strictEqual(await requestRepository.count(), 1); + }); + + it('rejects a concurrent duplicate request from a different user', async () => { + const requestRepository = getRepository(MediaRequest); + const requester = await seedRequester(5); + const otherRequester = await createRequester('second@seerr.dev'); + + const results = await Promise.allSettled( + [requester, otherRequester].map((user) => + MediaRequest.request( + { mediaId: 44444, mediaType: MediaType.MOVIE, is4k: false }, + user + ) + ) + ); + const rejected = rejections(results); + + assert.strictEqual(rejected.length, 1); + assert.ok(rejected[0].reason instanceof DuplicateMediaRequestError); + assert.strictEqual(await requestRepository.count(), 1); + }); + + it('gives an overlapping season to only one of two concurrent users', async () => { + const seasonRequestRepository = getRepository(SeasonRequest); + const requester = await seedRequester(5); + const otherRequester = await createRequester('second@seerr.dev'); + + const results = await Promise.allSettled( + [ + [requester, [1, 2]], + [otherRequester, [2, 3]], + ].map(([user, seasons]) => + MediaRequest.request( + { + mediaId: 55555, + mediaType: MediaType.TV, + seasons: seasons as number[], + is4k: false, + }, + user as User + ) + ) + ); + + assert.strictEqual(rejections(results).length, 0); + assert.strictEqual( + await seasonRequestRepository.count({ where: { seasonNumber: 2 } }), + 1 + ); + assert.strictEqual(await seasonRequestRepository.count(), 3); + }); + + it('creates one media row for concurrent 4k and non-4k requests', async () => { + const mediaRepository = getRepository(Media); + const requestRepository = getRepository(MediaRequest); + const requester = await seedRequester(5); + const otherRequester = await createRequester( + 'second@seerr.dev', + Permission.REQUEST_4K + ); + + const results = await Promise.allSettled( + [ + [requester, false], + [otherRequester, true], + ].map(([user, is4k]) => + MediaRequest.request( + { mediaId: 88888, mediaType: MediaType.MOVIE, is4k: is4k as boolean }, + user as User + ) + ) + ); + + assert.strictEqual(rejections(results).length, 0); + assert.strictEqual(await requestRepository.count(), 2); + assert.strictEqual( + await mediaRepository.count({ + where: { tmdbId: 88888, mediaType: MediaType.MOVIE }, + }), + 1 + ); + }); }); diff --git a/server/entity/MediaRequest.ts b/server/entity/MediaRequest.ts index 09f06a8798..937082562d 100644 --- a/server/entity/MediaRequest.ts +++ b/server/entity/MediaRequest.ts @@ -12,7 +12,11 @@ import { Permission } from '@server/lib/permissions'; import { getSettings } from '@server/lib/settings'; import logger from '@server/logger'; import { DbAwareColumn, resolveDbType } from '@server/utils/DbColumnHelper'; -import requestLock, { userKey } from '@server/utils/requestLock'; +import requestLock, { + mediaKey, + mediaLock, + userKey, +} from '@server/utils/requestLock'; import { truncate } from 'lodash'; import { AfterInsert, @@ -48,15 +52,22 @@ export class MediaRequest { user: User, options: MediaRequestOptions = {} ): Promise { + // is4k is optional, and an undefined one binds as null in the duplicate query + const body = { ...requestBody, is4k: !!requestBody.is4k }; + // Only a caller allowed to set the request user may queue on their lock const lockUserId = - requestBody.userId && + body.userId && user.hasPermission([Permission.MANAGE_USERS, Permission.MANAGE_REQUESTS]) - ? requestBody.userId + ? body.userId : user.id; + // No is4k in the key: one media row holds both statuses, so a 4k and a + // non-4k request for the same title race to create it return requestLock.dispatch(userKey(lockUserId), () => - MediaRequest.createRequest(requestBody, user, options) + mediaLock.dispatch(mediaKey(body.mediaType, body.mediaId), () => + MediaRequest.createRequest(body, user, options) + ) ); } diff --git a/server/routes/request.test.ts b/server/routes/request.test.ts index ed709fff52..cc27367237 100644 --- a/server/routes/request.test.ts +++ b/server/routes/request.test.ts @@ -459,6 +459,50 @@ describe('PUT /request/:requestId (tv)', () => { [3] ); }); + + it('gives a season to only one of two concurrent edits', async () => { + const requestRepo = getRepository(MediaRequest); + + const owner = await seedUser('admin@seerr.dev'); + const otherUser = await seedUser('demo@seerr.dev'); + + const mediaRequest = await seedTvRequest(owner, [1]); + const otherRequest = await seedTvRequest(otherUser, [3]); + + const admin = await loginAs('admin@seerr.dev', 'test1234'); + const friend = await loginAs('demo@seerr.dev', 'test1234'); + + const [adminRes, friendRes] = await Promise.all([ + admin + .put(`/request/${mediaRequest.id}`) + .send({ mediaType: MediaType.TV, seasons: [1, 2] }), + friend + .put(`/request/${otherRequest.id}`) + .send({ mediaType: MediaType.TV, seasons: [3, 2] }), + ]); + + assert.strictEqual(adminRes.status, 200); + assert.strictEqual(friendRes.status, 200); + + const saved = await requestRepo.findOneOrFail({ + where: { id: mediaRequest.id }, + }); + const otherSaved = await requestRepo.findOneOrFail({ + where: { id: otherRequest.id }, + }); + + const holders = [saved, otherSaved].filter((r) => + r.seasons.some((s) => s.seasonNumber === 2) + ); + assert.strictEqual(holders.length, 1); + + assert.deepStrictEqual( + [...saved.seasons, ...otherSaved.seasons] + .map((s) => s.seasonNumber) + .sort((a, b) => a - b), + [1, 2, 3] + ); + }); }); describe('PUT /request/:requestId (season availability)', () => { diff --git a/server/routes/request.ts b/server/routes/request.ts index 3cadab9f12..acdb90ccc9 100644 --- a/server/routes/request.ts +++ b/server/routes/request.ts @@ -25,7 +25,12 @@ import { Permission } from '@server/lib/permissions'; import { getSettings } from '@server/lib/settings'; import logger from '@server/logger'; import { isAuthenticated } from '@server/middleware/auth'; -import requestLock, { requestKey, userKey } from '@server/utils/requestLock'; +import requestLock, { + mediaKey, + mediaLock, + requestKey, + userKey, +} from '@server/utils/requestLock'; import { Router } from 'express'; const requestRoutes = Router(); @@ -558,122 +563,135 @@ requestRoutes.put<{ requestId: string }>( ); } - // Get existing media so we can work with all the requests - const media = await mediaRepository.findOneOrFail({ - where: { - tmdbId: request.media.tmdbId, - mediaType: MediaType.TV, - }, - relations: { requests: true }, - }); + // Same key as create, so an edit cannot claim a season that a new + // request is taking at the same moment + return mediaLock.dispatch( + mediaKey(MediaType.TV, request.media.tmdbId), + async () => { + // Get existing media so we can work with all the requests + const media = await mediaRepository.findOneOrFail({ + where: { + tmdbId: request.media.tmdbId, + mediaType: MediaType.TV, + }, + relations: { requests: true }, + }); - // Get all requested seasons that are not part of this request we are editing - const existingSeasons = media.requests - .filter( - (r) => - r.is4k === request.is4k && - r.id !== request.id && - r.status !== MediaRequestStatus.DECLINED && - r.status !== MediaRequestStatus.COMPLETED - ) - .reduce((seasons, r) => { - const combinedSeasons = r.seasons.map( - (season) => season.seasonNumber + // Get all requested seasons that are not part of this request we are editing + const existingSeasons = media.requests + .filter( + (r) => + r.is4k === request.is4k && + r.id !== request.id && + r.status !== MediaRequestStatus.DECLINED && + r.status !== MediaRequestStatus.COMPLETED + ) + .reduce((seasons, r) => { + const combinedSeasons = r.seasons.map( + (season) => season.seasonNumber + ); + + return [...seasons, ...combinedSeasons]; + }, [] as number[]); + + const currentSeasons = request.seasons.map( + (s) => s.seasonNumber ); - return [...seasons, ...combinedSeasons]; - }, [] as number[]); - - const currentSeasons = request.seasons.map((s) => s.seasonNumber); - - // Seasons the media already covers cannot be requested again, while - // the ones this request holds stay on it - const coveredSeasons = (media.seasons ?? []) - .filter( - (season) => - season[request.is4k ? 'status4k' : 'status'] !== - MediaStatus.UNKNOWN && - season[request.is4k ? 'status4k' : 'status'] !== - MediaStatus.DELETED - ) - .map((season) => season.seasonNumber) - .filter((sn) => !currentSeasons.includes(sn)); - - const filteredSeasons = requestedSeasons.filter( - (rs) => !existingSeasons.includes(rs) - ); - - const keptSeasons = filteredSeasons.filter((sn) => - currentSeasons.includes(sn) - ); - - const newSeasons = filteredSeasons.filter( - (sn) => - !currentSeasons.includes(sn) && !coveredSeasons.includes(sn) - ); - - const resultingSeasonCount = keptSeasons.length + newSeasons.length; - - if (resultingSeasonCount === 0) { - return next({ - status: 202, - message: 'No seasons available to request', - }); - } - - if (!request.ignoreQuota) { - const quotas = await requestUser.getQuota(); + // Seasons the media already covers cannot be requested again, while + // the ones this request holds stay on it + const coveredSeasons = (media.seasons ?? []) + .filter( + (season) => + season[request.is4k ? 'status4k' : 'status'] !== + MediaStatus.UNKNOWN && + season[request.is4k ? 'status4k' : 'status'] !== + MediaStatus.DELETED + ) + .map((season) => season.seasonNumber) + .filter((sn) => !currentSeasons.includes(sn)); + + const filteredSeasons = requestedSeasons.filter( + (rs) => !existingSeasons.includes(rs) + ); - // Only the seasons getQuota already counted for this owner are - // paid for, so the edit is charged for everything it left out - const quotaWindowStart = new Date(); - if (quotas.tv.days) { - quotaWindowStart.setDate( - quotaWindowStart.getDate() - quotas.tv.days + const keptSeasons = filteredSeasons.filter((sn) => + currentSeasons.includes(sn) ); - } - const countedAlready = - !ownerChanging && - (!quotas.tv.days || request.createdAt > quotaWindowStart); + const newSeasons = filteredSeasons.filter( + (sn) => + !currentSeasons.includes(sn) && !coveredSeasons.includes(sn) + ); - const priorSeasonCount = countedAlready - ? request.seasons.length - : 0; - const requiredSeasons = resultingSeasonCount - priorSeasonCount; + const resultingSeasonCount = + keptSeasons.length + newSeasons.length; + + if (resultingSeasonCount === 0) { + return next({ + status: 202, + message: 'No seasons available to request', + }); + } + + if (!request.ignoreQuota) { + const quotas = await requestUser.getQuota(); + + // Only the seasons getQuota already counted for this owner are + // paid for, so the edit is charged for everything it left out + const quotaWindowStart = new Date(); + if (quotas.tv.days) { + quotaWindowStart.setDate( + quotaWindowStart.getDate() - quotas.tv.days + ); + } + + const countedAlready = + !ownerChanging && + (!quotas.tv.days || request.createdAt > quotaWindowStart); + + const priorSeasonCount = countedAlready + ? request.seasons.length + : 0; + const requiredSeasons = + resultingSeasonCount - priorSeasonCount; + + if ( + quotas.tv.limit && + requiredSeasons > (quotas.tv.remaining ?? 0) + ) { + return next({ + status: 403, + message: 'Series Quota exceeded.', + }); + } + } + + request.seasons = request.seasons.filter((rs) => + keptSeasons.includes(rs.seasonNumber) + ); - if ( - quotas.tv.limit && - requiredSeasons > (quotas.tv.remaining ?? 0) - ) { - return next({ - status: 403, - message: 'Series Quota exceeded.', - }); + if (newSeasons.length > 0) { + logger.debug('Adding new seasons to request', { + label: 'Media Request', + newSeasons, + }); + request.seasons.push( + ...newSeasons.map( + (ns) => + new SeasonRequest({ + seasonNumber: ns, + status: MediaRequestStatus.PENDING, + }) + ) + ); + } + + await requestRepository.save(request); + + return res.status(200).json(request); } - } - - request.seasons = request.seasons.filter((rs) => - keptSeasons.includes(rs.seasonNumber) ); - - if (newSeasons.length > 0) { - logger.debug('Adding new seasons to request', { - label: 'Media Request', - newSeasons, - }); - request.seasons.push( - ...newSeasons.map( - (ns) => - new SeasonRequest({ - seasonNumber: ns, - status: MediaRequestStatus.PENDING, - }) - ) - ); - } - - await requestRepository.save(request); } return res.status(200).json(request); diff --git a/server/utils/requestLock.ts b/server/utils/requestLock.ts index 79d3d91568..86654673d4 100644 --- a/server/utils/requestLock.ts +++ b/server/utils/requestLock.ts @@ -1,10 +1,16 @@ +import type { MediaType } from '@server/constants/media'; import AsyncLock from '@server/utils/asyncLock'; // Keyed on user id or request id. Never dispatch from a subscriber or // transaction as a waiter would block while holding the save's connection. const requestLock = new AsyncLock(); +// keyed on media. always taken inside requestLock, never around it. +export const mediaLock = new AsyncLock(); + export const userKey = (userId: number) => `user:${userId}`; export const requestKey = (requestId: number) => `request:${requestId}`; +export const mediaKey = (mediaType: MediaType, mediaId: number) => + `${mediaType}:${mediaId}`; export default requestLock;