fix(requests): serialize requests for the same title (#3380)

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<User> {
   return userRepository.save(requester);
 }
 
+async function createRequester(
+  email: string,
+  permissions = Permission.REQUEST
+): Promise<User> {
+  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('[email protected]');
+
+    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('[email protected]');
+
+    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(
+      '[email protected]',
+      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
+    );
+  });
 });

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<MediaRequest> {
+    // 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)
+      )
     );
   }
 

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('[email protected]');
+    const otherUser = await seedUser('[email protected]');
+
+    const mediaRequest = await seedTvRequest(owner, [1]);
+    const otherRequest = await seedTvRequest(otherUser, [3]);
+
+    const admin = await loginAs('[email protected]', 'test1234');
+    const friend = await loginAs('[email protected]', '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)', () => {

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 },
-            });
-
-            // 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);
-
-            // 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();
-
-              // 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.',
+            // 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
+                    );
+
+                    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();
+
+                  // 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 (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);

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;