fix(requests): serialize requests for the same title

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

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('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)', () => {