fix(server): keep book requests from closing early and refusing input

Three book request defects, found while planning the Android client:

- The sweep that cancels a self-serve request nobody picked a release
  for timed it from createdAt. A self-fulfiller who took on an older
  request somebody else made inherited that clock, so a request taken
  on more than six hours after it was made was cancelled at the next
  sweep and dismissed for the person who asked for it. It is now timed
  from when the request became self-serve, the decidedAt that creating
  it self-serve and taking it on both set.
- The grab routes kept Fastify's 1 MiB body limit while their DTO takes
  a base64 .torrent of up to 4 Mi characters and the service 2 MiB
  decoded, so a .torrent over about 768 KiB was refused with a 413
  before the DTO was read. Both grab routes now declare a body limit
  that fits the DTO.
- Approving with a new library and no folder kept the request's stored
  folder, which belongs to its old library, and was refused with
  SUBMIT_FOLDER_NOT_IN_LIBRARY; nothing the approver could send cleared
  it. Rerouting without a folder now leaves the old one behind.

server/src/modules/book-request/book-request-admin.controller.ts

@@ -12,6 +12,7 @@ import {
   Post,
   Query,
 } from '@nestjs/common';
+import { RouteConfig } from '@nestjs/platform-fastify';
 import { AuditAction, AuditResource, Permission } from '@bookorbit/types';
 
 import { Auditable } from '../../common/decorators/auditable.decorator';
@@ -26,7 +27,7 @@ import { RequestSeedService } from './fulfillment/request-seed.service';
 import { RequestVerificationService } from './fulfillment/request-verification.service';
 import { BulkBookRequestsDto, BulkRejectBookRequestsDto } from './dto/bulk-book-requests.dto';
 import { DecideBookRequestDto } from './dto/decide-book-request.dto';
-import { GrabBookRequestDto } from './dto/grab-book-request.dto';
+import { GRAB_BODY_LIMIT_BYTES, GrabBookRequestDto } from './dto/grab-book-request.dto';
 import { InspectBookRequestReleaseDto } from './dto/inspect-book-request-release.dto';
 import { FulfillBookRequestDto } from './dto/fulfill-book-request.dto';
 import { ListAllBookRequestsDto } from './dto/list-book-requests.dto';
@@ -232,6 +233,7 @@ export class BookRequestAdminController {
    * .torrent by hand. All three go down the same pipeline from here.
    */
   @Post(':id/grab')
+  @RouteConfig({ bodyLimit: GRAB_BODY_LIMIT_BYTES })
   @HttpCode(HttpStatus.OK)
   @Auditable({
     action: AuditAction.BookRequestGrab,

server/src/modules/book-request/book-request-grab-route.test.ts

@@ -0,0 +1,19 @@
+import { FASTIFY_ROUTE_CONFIG_METADATA } from '@nestjs/platform-fastify/constants';
+import { describe, expect, it } from 'vitest';
+
+import { BookRequestAdminController } from './book-request-admin.controller';
+import { BookRequestSelfFulfilController } from './book-request-self-fulfil.controller';
+import { GRAB_BODY_LIMIT_BYTES, GRAB_TORRENT_BASE64_MAX_LENGTH } from './dto/grab-book-request.dto';
+
+/**
+ * Fastify's 1 MiB default refused a `.torrent` over about 768 KiB with a 413 before the DTO was
+ * read, though the DTO takes a base64 body up to 4 Mi characters.
+ */
+describe('book request grab routes', () => {
+  it('take a body as large as the largest .torrent the DTO accepts', () => {
+    expect(GRAB_BODY_LIMIT_BYTES).toBeGreaterThan(GRAB_TORRENT_BASE64_MAX_LENGTH);
+    for (const grab of [BookRequestAdminController.prototype.grab, BookRequestSelfFulfilController.prototype.grab]) {
+      expect(Reflect.getMetadata(FASTIFY_ROUTE_CONFIG_METADATA, grab)).toEqual({ bodyLimit: GRAB_BODY_LIMIT_BYTES });
+    }
+  });
+});

server/src/modules/book-request/book-request-self-fulfil.controller.ts

@@ -1,4 +1,5 @@
 import { Body, Controller, DefaultValuePipe, Get, HttpCode, HttpStatus, Param, ParseBoolPipe, ParseIntPipe, Post, Query } from '@nestjs/common';
+import { RouteConfig } from '@nestjs/platform-fastify';
 import { AuditAction, AuditResource, Permission } from '@bookorbit/types';
 
 import { Auditable } from '../../common/decorators/auditable.decorator';
@@ -11,7 +12,7 @@ import { RequestFulfillmentService } from './fulfillment/request-fulfillment.ser
 import { RequestImportService } from './fulfillment/request-import.service';
 import { RequestSeedService } from './fulfillment/request-seed.service';
 import { RequestVerificationService } from './fulfillment/request-verification.service';
-import { GrabBookRequestDto } from './dto/grab-book-request.dto';
+import { GRAB_BODY_LIMIT_BYTES, GrabBookRequestDto } from './dto/grab-book-request.dto';
 import { InspectBookRequestReleaseDto } from './dto/inspect-book-request-release.dto';
 import { SelectReleaseUnitDto } from './dto/select-release-unit.dto';
 import { SearchBookRequestReleasesDto } from './dto/search-book-request-releases.dto';
@@ -76,6 +77,7 @@ export class BookRequestSelfFulfilController {
   }
 
   @Post(':id/grab')
+  @RouteConfig({ bodyLimit: GRAB_BODY_LIMIT_BYTES })
   @HttpCode(HttpStatus.OK)
   @Auditable({
     action: AuditAction.BookRequestGrab,

server/src/modules/book-request/book-request.repository.ts

@@ -758,9 +758,14 @@ export class BookRequestRepository {
    * from. They matter because a live row holds the dedupe claim on its work, so an abandoned one
    * quietly subscribes the next person to ask for that book to a request nobody is driving.
    *
+   * Timed from when the row became self-serve, which `decidedAt` records both for a request made
+   * self-serve and for one a fulfiller took on, not from `createdAt`: an older request taken on
+   * was already past the limit and was cancelled at the next sweep under its new fulfiller.
+   *
    * Bounded, because this runs on a timer and a backlog must not turn into one enormous sweep.
    */
   async findAbandonedSelfServe(olderThan: Date, limit: number): Promise<BookRequestRow[]> {
+    const selfServeSince = sql`coalesce(${bookRequests.decidedAt}, ${bookRequests.createdAt})`;
     const withAttempt = this.db
       .select({ id: bookRequestDownloads.requestId })
       .from(bookRequestDownloads)
@@ -773,11 +778,11 @@ export class BookRequestRepository {
         and(
           eq(bookRequests.selfServe, true),
           inArray(bookRequests.status, [...ABANDONABLE_SELF_SERVE_STATUSES]),
-          lt(bookRequests.createdAt, olderThan),
+          lt(selfServeSince, olderThan),
           notExists(withAttempt),
         ),
       )
-      .orderBy(asc(bookRequests.createdAt))
+      .orderBy(asc(selfServeSince))
       .limit(limit);
   }
 

server/src/modules/book-request/book-request.service.test.ts

@@ -1266,17 +1266,40 @@ describe('BookRequestService.approve', () => {
     expect(repo.updateIf).toHaveBeenCalledWith(10, expect.anything(), expect.objectContaining({ targetLibraryId: 5 }));
   });
 
-  it('refuses a folder that does not belong to the library the approver rerouted to', async () => {
+  /**
+   * The stored folder belongs to the stored library. Rerouting with no folder named used to carry
+   * it along and be refused as not part of the new library, with nothing the approver could send
+   * to clear it.
+   */
+  it('leaves the old library folder behind when the approver reroutes without naming one', async () => {
     const { service, repo } = makeService({
       repo: {
         findById: vi.fn().mockResolvedValue(joined({ targetLibraryId: 3, targetFolderId: 8 })),
         folderBelongsToLibrary: vi.fn().mockResolvedValue(false),
       },
     });
-    await expect(service.approve(10, { targetLibraryId: 5 }, approver)).rejects.toThrow(BadRequestException);
+    await service.approve(10, { targetLibraryId: 5 }, approver);
+    expect(repo.folderBelongsToLibrary).not.toHaveBeenCalled();
+    expect(repo.updateIf).toHaveBeenCalledWith(10, expect.anything(), expect.objectContaining({ targetLibraryId: 5, targetFolderId: null }));
+  });
+
+  it('still refuses a named folder that does not belong to the library the approver rerouted to', async () => {
+    const { service, repo } = makeService({
+      repo: {
+        findById: vi.fn().mockResolvedValue(joined({ targetLibraryId: 3, targetFolderId: 8 })),
+        folderBelongsToLibrary: vi.fn().mockResolvedValue(false),
+      },
+    });
+    await expect(service.approve(10, { targetLibraryId: 5, targetFolderId: 8 }, approver)).rejects.toThrow(BadRequestException);
     expect(repo.updateIf).not.toHaveBeenCalled();
   });
 
+  it('keeps the stored folder when the approver names the same library', async () => {
+    const { service, repo } = makeService({ repo: { findById: vi.fn().mockResolvedValue(joined({ targetLibraryId: 3, targetFolderId: 8 })) } });
+    await service.approve(10, { targetLibraryId: 3 }, approver);
+    expect(repo.updateIf).toHaveBeenCalledWith(10, expect.anything(), expect.objectContaining({ targetLibraryId: 3, targetFolderId: 8 }));
+  });
+
   it('keeps a folder that does belong to the destination library', async () => {
     const { service, repo } = makeService({ repo: { findById: vi.fn().mockResolvedValue(joined({ targetLibraryId: 3, targetFolderId: 8 })) } });
     await service.approve(10, {}, approver);

server/src/modules/book-request/book-request.service.ts

@@ -1128,9 +1128,13 @@ export class BookRequestService {
       targetLibraryId = dto.targetLibraryId;
     }
 
+    // A stored folder belongs to the stored library. Rerouted elsewhere with no folder named, it
+    // would be refused as not part of the new library, and nothing the approver sends can clear it,
+    // so it is left behind and the new library's own folder is used.
+    const rerouted = targetLibraryId !== joined.request.targetLibraryId;
     // Requests made before an instance default existed carry no destination at all, and there is
     // no reason to make the approver retype one the operator has since set.
-    let carriedFolderId = dto.targetFolderId ?? joined.request.targetFolderId;
+    let carriedFolderId = dto.targetFolderId ?? (rerouted ? null : joined.request.targetFolderId);
     if (targetLibraryId === null) {
       const fallback = await this.automationSettings.resolveDestinationFor(mediaKind);
       targetLibraryId = fallback.libraryId;

server/src/modules/book-request/dto/grab-book-request.dto.ts

@@ -2,6 +2,15 @@ import { Type } from 'class-transformer';
 import { IsBase64, IsInt, IsOptional, IsString, MaxLength, Min } from 'class-validator';
 import { MAX_INDEXER_RELEASE_GUID_LENGTH } from '@bookorbit/types';
 
+/** The longest base64 `.torrent` the grab takes. The 2 MiB the service accepts is about 2.7 Mi characters. */
+export const GRAB_TORRENT_BASE64_MAX_LENGTH = 4 * 1024 * 1024;
+
+/**
+ * The grab routes' body limit. Fastify's default is 1 MiB, which refused a `.torrent` over about
+ * 768 KiB with a 413 before this DTO was read, though the DTO and the service allow far more.
+ */
+export const GRAB_BODY_LIMIT_BYTES = GRAB_TORRENT_BASE64_MAX_LENGTH + 1024 * 1024;
+
 /**
  * A grab names one of three things: a release the approver picked out of the ranked list, a
  * magnet, or a .torrent file. The service enforces "exactly one", because none of them is
@@ -33,7 +42,7 @@ export class GrabBookRequestDto {
 
   @IsOptional()
   @IsBase64()
-  @MaxLength(4 * 1024 * 1024)
+  @MaxLength(GRAB_TORRENT_BASE64_MAX_LENGTH)
   torrentFileBase64?: string;
 
   @IsOptional()