Second review pass over #1910: races, duplication, dead code

api/src/changeRequests/documentProcessing/deleteMediaCollection.ts

@@ -1,7 +1,7 @@
 import { MediaDto } from "../../dto/MediaDto";
 import { DbService } from "../../db/db.service";
 import { S3Service } from "../../s3/s3.service";
-import { isBucketRelative, toStoredMediaUrl } from "./mediaUrl";
+import { isBucketRelative, isInOurStorage } from "./mediaUrl";
 
 /**
  * Where a collection lives in its bucket, or why we will not touch it.
@@ -22,7 +22,23 @@ export type PrefixResolution = { prefix: string } | { refusal: string };
 const SESSION_ID = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
 
 /** What the encoder publishes at the root of a collection. */
-const MASTER = "/master.m3u8";
+export const MASTER = "/master.m3u8";
+
+/** Where a bucket publishes its objects, and what to call it in a warning. */
+export type Bucket = { publicUrl?: string; name?: string };
+
+export async function loadBucket(
+    bucketId: string,
+    db: DbService,
+): Promise<{ bucket: Bucket } | { error: string }> {
+    try {
+        const result = await db.getDoc(bucketId);
+        if (!result.docs?.length) return { error: `bucket ${bucketId} no longer exists` };
+        return { bucket: result.docs[0] };
+    } catch (error) {
+        return { error: error.message };
+    }
+}
 
 /**
  * Turn a published `hlsUrl` into the object prefix holding that collection.
@@ -121,32 +137,19 @@ export async function deleteMediaCollection(
         return warnings;
     }
 
-    let bucket: { publicUrl?: string; name?: string };
-    try {
-        const result = await db.getDoc(bucketId);
-        if (!result.docs?.length) {
-            warnings.push(
-                `Media files were not deleted: bucket ${bucketId} no longer exists. ` +
-                    "Please remove them on the storage provider.",
-            );
-            return warnings;
-        }
-        bucket = result.docs[0];
-    } catch (error) {
-        warnings.push(`Media files were not deleted: ${error.message}`);
+    const loaded = await loadBucket(bucketId, db);
+    if ("error" in loaded) {
+        warnings.push(
+            `Media files were not deleted: ${loaded.error}. ` +
+                "Please remove them on the storage provider.",
+        );
         return warnings;
     }
+    const bucket = loaded.bucket;
 
-    // Media hosted elsewhere — a YouTube link, a master on someone else's CDN —
-    // has nothing here to delete, and telling the operator to go and remove it
-    // "on the storage provider" would send them looking for files their bucket
-    // never held.
-    if (
-        !isBucketRelative(media.hlsUrl) &&
-        toStoredMediaUrl(media.hlsUrl, bucket.publicUrl) === media.hlsUrl
-    ) {
-        return warnings;
-    }
+    // Media hosted elsewhere has nothing here to delete, and a warning would send the
+    // operator looking for files their bucket never held.
+    if (!isInOurStorage(media.hlsUrl, [bucket.publicUrl])) return warnings;
 
     const resolved = resolveCollectionPrefix(media.hlsUrl, bucket.publicUrl);
     if ("refusal" in resolved) {

api/src/changeRequests/documentProcessing/migrateMediaCollection.ts

@@ -1,33 +1,12 @@
 import { MediaDto } from "../../dto/MediaDto";
 import { DbService } from "../../db/db.service";
 import { S3Service } from "../../s3/s3.service";
-import { resolveCollectionPrefix } from "./deleteMediaCollection";
-import { isBucketRelative, toStoredMediaUrl } from "./mediaUrl";
-
-/** What the encoder publishes at the root of a collection. */
-const MASTER = "master.m3u8";
+import { MASTER, loadBucket, resolveCollectionPrefix } from "./deleteMediaCollection";
+import { isBucketRelative, isInOurStorage } from "./mediaUrl";
 
 /** The S3 API's own ceiling on keys per delete call. */
 const DELETE_BATCH = 1000;
 
-/**
- * Where a bucket publishes its objects, and what to call it in a warning.
- */
-type Bucket = { publicUrl?: string; name?: string };
-
-async function loadBucket(
-    bucketId: string,
-    db: DbService,
-): Promise<{ bucket: Bucket } | { error: string }> {
-    try {
-        const result = await db.getDoc(bucketId);
-        if (!result.docs?.length) return { error: `bucket ${bucketId} no longer exists` };
-        return { bucket: result.docs[0] };
-    } catch (error) {
-        return { error: error.message };
-    }
-}
-
 /**
  * Move a media collection from one bucket to another, then point the document at
  * its new home.
@@ -88,17 +67,9 @@ export async function migrateMediaCollection(
         return { failed: true, warnings };
     }
 
-    // The same proof used before deleting: a prefix we cannot derive from the
-    // bucket's own public base is a collection we did not write.
-    // Media that is not in the old bucket is not ours to move: a YouTube link
-    // or a master on someone else's CDN belongs to whoever serves it, and the
-    // bucket change is about where *future* output goes. Treating that as a
-    // failed migration would revert a change the user made deliberately and
-    // warn about files that were never going anywhere.
-    const external =
-        !isBucketRelative(previousHlsUrl) &&
-        toStoredMediaUrl(previousHlsUrl, oldBucket.publicUrl) === previousHlsUrl;
-    if (external) return { failed: false, warnings };
+    // External media is not ours to move, and a bucket change is about where future
+    // output goes; calling it a failed migration would revert a deliberate change.
+    if (!isInOurStorage(previousHlsUrl, [oldBucket.publicUrl])) return { failed: false, warnings };
 
     const resolved = resolveCollectionPrefix(previousHlsUrl, oldBucket.publicUrl);
     if ("refusal" in resolved) {
@@ -141,14 +112,10 @@ export async function migrateMediaCollection(
             }
         }
 
-        // Only now is the new location real, so only now may the document name it.
-        //
-        // A relative URL already names a path inside whichever bucket the
-        // document points at, so moving buckets does not change it — which is
-        // the point of storing it that way. Only the legacy absolute form has
-        // to be rewritten.
+        // Only now is the new location real. A relative URL already names a path inside
+        // whichever bucket the document points at; only the legacy absolute form moves.
         if (!isBucketRelative(media.hlsUrl)) {
-            media.hlsUrl = `${newBucket.publicUrl.replace(/\/+$/, "")}/${prefix}/${MASTER}`;
+            media.hlsUrl = `${newBucket.publicUrl.replace(/\/+$/, "")}/${prefix}${MASTER}`;
         }
 
         // Last, and its failure is not the migration's failure: the files are in
@@ -177,11 +144,9 @@ export async function migrateMediaCollection(
         );
         return { failed: false, warnings };
     } catch (error) {
-        // Nothing was deleted and the URL was not rewritten, so the collection is
-        // still whole and still where the document says it is. Anything already
-        // copied is left in place: it is unreferenced, harmless, and overwritten by
-        // a retry — whereas deleting it on the way out of a failure risks removing
-        // objects we did not put there.
+        // Nothing was deleted and the URL was not rewritten. Copies already made are
+        // left: a retry overwrites them, and deleting on the way out risks objects we
+        // did not put there.
         warnings.push(
             `Media migration failed: ${error.message}. The files were left in ` +
                 `${oldBucket.name ?? oldBucketId}.`,

api/src/changeRequests/documentProcessing/processPostTagDto.ts

@@ -8,6 +8,7 @@ import { processMedia } from "./processMediaDto";
 import { deleteMediaCollection } from "./deleteMediaCollection";
 import { migrateMediaCollection } from "./migrateMediaCollection";
 import { isInOurStorage } from "./mediaUrl";
+import { StorageDto } from "../../dto/StorageDto";
 import {
     deleteSidecar,
     deleteSidecarsForParent,
@@ -43,22 +44,16 @@ export default async function processPostTagDto(
             warnings.push(...imageWarnings);
         }
 
-        // Media files go only when the user asked for them in the delete
-        // confirmation. Opt-in because it is irreversible and because the
-        // collection may be referenced somewhere this API cannot see; the previous
-        // document is the authority on where the files are, and the incoming one
-        // carries the intent.
+        // Opt-in from the delete confirmation: irreversible, and the collection may be
+        // referenced somewhere this API cannot see. prevDoc knows where the files are.
         if (doc.media?.deleteFiles) {
             warnings.push(
                 ...(await deleteMediaCollection(prevDoc?.media, prevDoc?.mediaBucketId, db)),
             );
         }
 
-        // Sidecars are children of this document — nothing else references them and
-        // no client holds a copy, so they go with it. Hard delete, no DeleteCmd. A
-        // failure here must not block the content delete: an orphaned sidecar is
-        // unreadable once the parent is gone (GET /sidecar 404s), so warn rather
-        // than throw, matching deleteImage's precedent.
+        // Sidecars go with their parent (hard delete, no DeleteCmd). Warn rather than
+        // block the delete, as deleteImage does: an orphan is unreadable anyway.
         try {
             await deleteSidecarsForParent(db, doc._id);
         } catch (error) {
@@ -121,32 +116,20 @@ export default async function processPostTagDto(
         delete (doc as any).image; // Remove the legacy image field
     }
 
-    // Process media
     if (doc.media) {
-        // The bucket is where the encoder was told to write, and is what a later
-        // edit of the collection has to be pointed back at — so it is required
-        // for a collection in our own storage, and meaningless for one that is
-        // not. A YouTube link, or an HLS master on someone else's CDN, has no
-        // bucket to be relative to and nothing here to migrate or delete;
-        // demanding one records a bucket that does not own anything.
-        //
-        // Asked of the configured buckets rather than assumed from the shape of
-        // the URL: an absolute URL under a bucket's public URL is ours, and is
-        // exactly the case that must not be saved without naming its bucket —
-        // stored un-relative, it breaks the moment that bucket is renamed.
+        // A collection in our own storage must name its bucket: that is how the URL is
+        // stored relative, migrated and deleted. External media has no bucket to name.
         if (doc.media.hlsUrl && !doc.mediaBucketId) {
             const buckets = await db.getDocsByType(DocType.Storage);
-            const publicUrls = buckets.docs.map((b: any) => b.publicUrl);
+            const publicUrls = buckets.docs.map((b: StorageDto) => b.publicUrl);
 
             if (isInOurStorage(doc.media.hlsUrl, publicUrls)) {
                 throw new Error("Bucket is not specified for media processing.");
             }
         }
 
-        // A bucket change has to take the files with it. `mediaBucketId` and
-        // `hlsUrl` must name the same bucket: if they diverge, the collection can no
-        // longer be resolved from the URL, and deleting the document then leaves the
-        // files behind for good.
+        // A bucket change takes the files with it: `mediaBucketId` and `hlsUrl` must
+        // name the same bucket, or a later delete cannot find the collection.
         if (prevDoc?.mediaBucketId && prevDoc.mediaBucketId !== doc.mediaBucketId) {
             const migration = await migrateMediaCollection(
                 doc.media,
@@ -165,21 +148,15 @@ export default async function processPostTagDto(
             }
         }
 
-        // A failed key store must fail the change request, not become a warning: the
-        // plaintext key exists only for the duration of this request (processMedia has
-        // already dropped it by the time the error is caught), so saving the Post with
-        // an `hlsUrl` and no `hlsKey_id` would leave an unplayable, unrecoverable
-        // collection. Let it throw — processChangeRequest has no catch here, so the CR
-        // fails and the editor still holds the key to retry.
+        // Deliberately not caught: the plaintext key exists only for this request, so a
+        // saved `hlsUrl` without its `hlsKey_id` would be unrecoverable. Failing the
+        // change request leaves the editor holding the key to retry.
         warnings.push(...(await processMedia(doc.media, doc, db)));
     }
 
-    // A key that was referenced and no longer is has been removed by the editor —
-    // whether they cleared the key field or the whole media object. Outside the
-    // `if (doc.media)` block on purpose: removing the collection drops doc.media
-    // entirely, which is the case a check inside processMedia would never see. A
-    // fresh hlsKey in the same request means replace, not delete — processMedia
-    // has already recreated the sidecar at the same id. See ADR 0019.
+    // Outside `if (doc.media)`: removing the whole media object removes the key too.
+    // A fresh `hlsKey` in the same request is a replacement processMedia has already
+    // stored at the same sidecar id (ADR 0019).
     if (prevDoc?.media?.hlsKey_id && !doc.media?.hlsKey_id && !doc.media?.hlsKey) {
         await deleteSidecar(db, doc._id, SidecarType.HlsEncryptionKey);
     }

api/src/configuration.ts

@@ -1,6 +1,3 @@
-// RateLimiterConfig shape: { enabled, freeStrikes, baseBackoffMs, maxBackoffMs, strikeDecayMs }.
-// Every endpoint-specific rate-limit config below is one of these, keyed under its own env prefix
-// and its own config namespace, so limiters never share a bucket across endpoints.
 import { RateLimiterConfig } from "./ratelimit/rateLimiter.service";
 
 export type DatabaseConfig = {

api/src/db/schemaUpgrade/v21.spec.ts

@@ -24,7 +24,7 @@ describe("v21 — legacy video field moved to media.hlsUrl", () => {
         return { _id: id, type: DocType.Post, ...(media !== undefined ? { media } : {}) };
     }
 
-    function content(id: string, parentId: string, video?: string) {
+    function content(id: string, parentId: string, video?: string): any {
         return { _id: id, type: DocType.Content, parentId, ...(video ? { video } : {}) };
     }
 
@@ -37,11 +37,26 @@ describe("v21 — legacy video field moved to media.hlsUrl", () => {
 
         expect(p.media).toEqual({ fileCollections: [], hlsUrl: "https://example.com/master.m3u8" });
         expect(c.video).toBeUndefined();
+        expect(c.parentMedia).toEqual(p.media);
         expect(upserted).toContain(p);
         expect(upserted).toContain(c);
         expect(db.setSchemaVersion).toHaveBeenCalledWith(21);
     });
 
+    it("stamps parentMedia on the siblings that never had a video of their own", async () => {
+        const p = post("post-1");
+        const withVideo = content("content-en", "post-1", "https://example.com/a.m3u8");
+        const without = content("content-fr", "post-1");
+        const { db, upserted } = mockDb(20, { [DocType.Post]: [p], [DocType.Tag]: [] }, {
+            "post-1": [withVideo, without],
+        });
+
+        await v21(db);
+
+        expect(without.parentMedia).toEqual(p.media);
+        expect(upserted).toContain(without);
+    });
+
     it("leaves an existing parent hlsUrl untouched but still clears the child's video", async () => {
         const p = post("post-1", { fileCollections: [], hlsUrl: "https://example.com/existing.m3u8" });
         const c = content("content-1", "post-1", "https://example.com/stale.m3u8");
@@ -51,6 +66,7 @@ describe("v21 — legacy video field moved to media.hlsUrl", () => {
 
         expect(p.media.hlsUrl).toBe("https://example.com/existing.m3u8");
         expect(c.video).toBeUndefined();
+        expect(c.parentMedia).toBeUndefined();
         expect(upserted).not.toContain(p);
         expect(upserted).toContain(c);
     });

api/src/db/schemaUpgrade/v21.ts

@@ -1,23 +1,14 @@
 import { DbService } from "../db.service";
 import { DocType } from "../../enums";
+import { ContentDto } from "../../dto/ContentDto";
 
 /**
- * Upgrade the database schema from version 20 to 21.
+ * Upgrade the database schema from version 20 to 21: the legacy per-language
+ * `ContentDto.video` URL moves onto the parent's `media.hlsUrl`.
  *
- * Moves the legacy per-language `ContentDto.video` URL onto the parent's
- * `media.hlsUrl` (`_contentParentDto.media`, `MediaDto.hlsUrl`). The CMS video editor
- * (`EditContentVideo.vue`) already writes exclusively to `parent.media.hlsUrl`, and the
- * app already prefers `parentMedia.hlsUrl` over `content.video` (`videoSourceFor`) — so
- * `video` is dead weight on any Content doc whose parent already has an `hlsUrl`, and a
- * stale leftover on parents that don't.
- *
- * For each Post/Tag with no `media.hlsUrl`, the first non-empty `video` found among its
- * child Content docs (across languages) is copied onto `parent.media.hlsUrl` — the
- * per-parent `media` field can only hold one collection, so this is a many-to-one
- * collapse; any other distinct value among the remaining children is logged and
- * dropped. `video` is then deleted from every child that had it, regardless of whether
- * its value was the one kept, since a per-child video field is no longer read anywhere
- * once `parentMedia.hlsUrl` exists.
+ * A parent holds one collection, so the first child's value wins and any other
+ * distinct value is logged and dropped. `video` is then cleared from every child,
+ * and `parentMedia` stamped on them as a change request would.
  */
 export default async function (db: DbService) {
     try {
@@ -44,10 +35,12 @@ export default async function (db: DbService) {
             for (const parent of parents) {
                 stats.parentsScanned++;
 
-                const { docs: children } = await db.getContentByParentId(parent._id);
-                const withVideo = (children as any[]).filter((c) => c.video);
+                const { docs } = await db.getContentByParentId(parent._id);
+                const children = docs as ContentDto[];
+                const withVideo = children.filter((c) => c.video);
                 if (!withVideo.length) continue;
 
+                let parentUpdated = false;
                 if (!parent.media?.hlsUrl) {
                     if (!parent.media) parent.media = { fileCollections: [] };
                     parent.media.hlsUrl = withVideo[0].video;
@@ -63,13 +56,24 @@ export default async function (db: DbService) {
                     parent.updatedTimeUtc = Date.now();
                     await db.upsertDoc(parent);
                     stats.parentsUpdated++;
+                    parentUpdated = true;
                 }
 
-                for (const child of withVideo) {
+                // `parentMedia` is only ever stamped by a change request, so a migrated
+                // parent's children must get it here or the app shows no video until
+                // the parent is next saved.
+                for (const child of children) {
+                    const hadVideo = Boolean(child.video);
+                    if (!hadVideo && !parentUpdated) continue;
+
                     delete child.video;
+                    if (parentUpdated) {
+                        child.parentMedia = parent.media;
+                        child.parentMediaBucketId = parent.mediaBucketId;
+                    }
                     child.updatedTimeUtc = Date.now();
                     await db.upsertDoc(child);
-                    stats.childrenCleared++;
+                    if (hadVideo) stats.childrenCleared++;
                 }
             }
         }

api/src/dto/MediaDto.ts

@@ -1,7 +1,7 @@
 import "reflect-metadata"; // https://stackoverflow.com/questions/72009995/typeerror-reflect-getmetadata-is-not-a-function
 import { IsBoolean, IsOptional, IsString, Matches } from "class-validator";
 import { Expose } from "class-transformer";
-import { Uuid } from "src/enums";
+import { Uuid } from "../enums";
 
 /**
  * Database structured Media object