From b30d2cd9946cee785e2ff0ac25cc88c2546595c7 Mon Sep 17 00:00:00 2001 From: Ned Cutler Date: Sat, 11 Jul 2026 13:26:52 -0400 Subject: [PATCH 1/3] Prevent orphaned photo uploads --- .gitignore | 1 + lib/photo-upload.test.ts | 81 ++++++++++++-- lib/photo-upload.ts | 138 ++++++++++++++++-------- scripts/purge-orphan-objects.mjs | 175 +++++++++++++++++++++++++++++++ 4 files changed, 343 insertions(+), 52 deletions(-) create mode 100644 scripts/purge-orphan-objects.mjs diff --git a/.gitignore b/.gitignore index 16b6dfe..884c098 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,7 @@ npm-debug.log* # Playwright MCP debug artifacts .playwright-mcp/ .dedupe-report.json +.orphan-report.json # Playwright e2e .next-e2e diff --git a/lib/photo-upload.test.ts b/lib/photo-upload.test.ts index d8e93a1..4488abb 100644 --- a/lib/photo-upload.test.ts +++ b/lib/photo-upload.test.ts @@ -43,16 +43,18 @@ function existingPhoto(contentHash: string): Photo { }; } -function fakeSupabase(options: { failUploadFor?: string[]; clashHashes?: string[]; insertErrorMessage?: string } = {}) { +function fakeSupabase(options: { failUploadFor?: string[]; clashHashes?: string[]; legacyClashHashes?: string[]; clashError?: string; insertErrorMessage?: string; insertThrows?: string } = {}) { const uploaded: string[] = []; + const upserted: boolean[] = []; const removed: string[] = []; const inserted: Array> = []; const client = { storage: { from: () => ({ - upload: async (path: string, file: File) => { + upload: async (path: string, file: File, uploadOptions?: { upsert?: boolean }) => { if (options.failUploadFor?.some((name) => file.name.startsWith(name))) return { error: { message: "storage exploded" } }; uploaded.push(path); + upserted.push(Boolean(uploadOptions?.upsert)); return { error: null }; }, remove: async (paths: string[]) => { @@ -64,11 +66,21 @@ function fakeSupabase(options: { failUploadFor?: string[]; clashHashes?: string[ from: () => ({ select: () => ({ eq: () => ({ - in: async () => ({ data: (options.clashHashes ?? []).map((hash) => ({ content_hash: hash })) }), + in: async () => ({ + data: options.clashError + ? null + : [...(options.clashHashes ?? []), ...(options.legacyClashHashes ?? [])].map((hash) => ({ + content_hash: hash, + image_path: options.legacyClashHashes?.includes(hash) ? `lofoten-2026/legacy-${hash}.mp4` : `lofoten-2026/${hash}.mp4`, + thumbnail_path: null, + })), + error: options.clashError ? { message: options.clashError } : null, + }), }), }), insert: (rows: Array>) => ({ select: async () => { + if (options.insertThrows) throw new Error(options.insertThrows); if (options.insertErrorMessage) return { data: null, error: { message: options.insertErrorMessage } }; inserted.push(...rows); const returned = rows.map((row, index) => ({ ...row, id: `row-${index}` })); @@ -77,7 +89,7 @@ function fakeSupabase(options: { failUploadFor?: string[]; clashHashes?: string[ }), }), } as unknown as SupabaseClient; - return { client, uploaded, removed, inserted }; + return { client, uploaded, upserted, removed, inserted }; } const trip = { id: "trip-1", slug: "lofoten-2026" }; @@ -137,16 +149,73 @@ describe("uploadPhotoBatch", () => { expect(inserted).toHaveLength(1); }); - it("removes storage objects for hashes someone else uploaded mid-batch", async () => { + // New rows are content-addressed, so a racing upload of an already-stored hash + // lands on the live row's exact key. Removing it would strand that row. + it("leaves storage alone for hashes someone else uploaded mid-batch", async () => { const { client, removed, inserted } = fakeSupabase({ clashHashes: ["hash-a"] }); const { result } = await batch(client, [input({ clientId: "a" }), input({ clientId: "b" })]); expect(result.failedClientIds).toEqual(["a"]); expect(result.savedClientIds).toEqual(["b"]); - expect(removed).toHaveLength(1); + expect(removed).toEqual([]); expect(inserted).toHaveLength(1); }); + it("removes an unused hash-path upload when the clashing row has a legacy UUID path", async () => { + const { client, removed, inserted } = fakeSupabase({ legacyClashHashes: ["hash-a"] }); + const { result } = await batch(client, [input({ clientId: "a" }), input({ clientId: "b" })]); + + expect(result.failedClientIds).toEqual(["a"]); + expect(result.savedClientIds).toEqual(["b"]); + expect(removed).toEqual(["lofoten-2026/hash-a.mp4"]); + expect(inserted).toHaveLength(1); + }); + + it("does not clean up live objects when the clash check fails", async () => { + const { client, removed } = fakeSupabase({ clashError: "clash lookup failed" }); + const { result } = await batch(client, [input({ clientId: "a" })]); + + expect(result.insertErrorMessage).toBe("clash lookup failed"); + expect(result.failedClientIds).toEqual(["a"]); + expect(result.inserted).toBe(false); + expect(removed).toEqual([]); + }); + + it("keys objects by content hash and upserts, so a retry overwrites instead of orphaning", async () => { + const first = fakeSupabase(); + await batch(first.client, [input({ clientId: "a" })]); + // Same file, second attempt: a batch that died before its insert last time. + const second = fakeSupabase(); + await batch(second.client, [input({ clientId: "a" })]); + + expect(first.uploaded).toEqual(["lofoten-2026/hash-a.mp4"]); + expect(second.uploaded).toEqual(first.uploaded); + expect(first.upserted).toEqual([true]); + }); + + it("rolls back only the fresh objects when the insert fails", async () => { + const { client, removed } = fakeSupabase({ clashHashes: ["hash-a"], insertErrorMessage: "insert exploded" }); + const { result } = await batch(client, [input({ clientId: "a" }), input({ clientId: "b" })]); + + expect(result.insertErrorMessage).toBe("insert exploded"); + // b's object is unreferenced and goes; a's is the existing row's own object. + expect(removed).toEqual(["lofoten-2026/hash-b.mp4"]); + expect(result.savedClientIds).toEqual([]); + }); + + it("leaves uploads in place when the insert throws", async () => { + const { client, removed, uploaded } = fakeSupabase({ insertThrows: "network died" }); + const { result } = await batch(client, [input({ clientId: "a" })]); + + expect(result.insertErrorMessage).toBe("network died"); + expect(result.failedClientIds).toEqual(["a"]); + expect(result.inserted).toBe(false); + // Which hashes gained a row is unknown here, so blind cleanup could delete a + // live object. The content-addressed key is reused by the retry instead. + expect(uploaded).toEqual(["lofoten-2026/hash-a.mp4"]); + expect(removed).toEqual([]); + }); + it("rolls back every uploaded object when the insert fails", async () => { const { client, removed, inserted } = fakeSupabase({ insertErrorMessage: "unique constraint" }); const { result } = await batch(client, [input({ clientId: "a" }), input({ clientId: "b" })]); diff --git a/lib/photo-upload.ts b/lib/photo-upload.ts index a0d71dd..25b116e 100644 --- a/lib/photo-upload.ts +++ b/lib/photo-upload.ts @@ -59,8 +59,9 @@ type PendingRow = { * thumbnail concurrently per item (bounded by `concurrency`), re-check * content hashes against the database (someone else may have uploaded the * same photo mid-batch), then insert the surviving rows. Storage objects are - * removed again whenever their row cannot be inserted, so a failure never - * leaves orphaned files behind. + * removed again when it is safe to prove that their row was not inserted. + * If the database outcome is unknown, content-addressed paths let a retry + * reuse the same objects instead of creating another abandoned copy. * * Pure orchestration over the injected client — UI state (error banners, * reloads, panel close) stays with the caller, driven by the outcome. @@ -78,13 +79,23 @@ export async function uploadPhotoBatch(options: { const rows: PendingRow[] = []; const failures: string[] = []; const warnings: string[] = []; - const uploadedPaths: string[] = []; const savedClientIds: string[] = []; const failedClientIds: string[] = []; let insertErrorMessage: string | null = null; let inserted = false; let insertedRows: Photo[] = []; + const storagePathsOf = (pending: PendingRow[]) => + pending.flatMap((row) => [row.image_path, row.thumbnail_path].filter((path): path is string => Boolean(path))); + + // A cleanup that silently fails is how the bucket filled up with unreachable + // objects in the first place, so a failed removal is reported, not dropped. + const removeObjects = async (paths: string[], label: string) => { + if (paths.length === 0) return; + const { error } = await supabase.storage.from(PHOTO_BUCKET).remove(paths); + if (error) warnings.push(`${label}: storage cleanup failed (${error.message})`); + }; + const { uploads: uploadCandidates, duplicates } = partitionDuplicatePhotos( inputs.map((input) => ({ input, contentHash: input.contentHash, mediaType: input.mediaType, takenAt: input.exif?.takenAt ?? null, coordinate: input.coordinate })), existingPhotos, @@ -98,30 +109,32 @@ export async function uploadPhotoBatch(options: { await mapWithConcurrency(uploadCandidates.map((candidate) => candidate.input), concurrency, async (input) => { const prepared = await prepareMediaFiles(input.file); const extension = storageFileExtension(prepared.imageFile); - const path = `${trip.slug}/${crypto.randomUUID()}.${extension}`; - const thumbnailPath = prepared.thumbnailFile ? `${trip.slug}/thumbs/${crypto.randomUUID()}.jpg` : null; + // Content-addressed, so an upload is idempotent: a retry of a batch that + // died before its insert lands on the same key and overwrites the object + // instead of stranding it under a fresh uuid. `(trip_id, content_hash)` is + // already unique in the database, so one key here is one row there. + const path = `${trip.slug}/${input.contentHash}.${extension}`; + const thumbnailPath = prepared.thumbnailFile ? `${trip.slug}/thumbs/${input.contentHash}.jpg` : null; // The thumbnail never depends on the image upload, so both go up // together instead of back to back. const [imageUpload, thumbnailUpload] = await Promise.all([ - supabase.storage.from(PHOTO_BUCKET).upload(path, prepared.imageFile, { cacheControl: IMMUTABLE_CACHE_SECONDS, upsert: false, contentType: prepared.imageFile.type || undefined }), + supabase.storage.from(PHOTO_BUCKET).upload(path, prepared.imageFile, { cacheControl: IMMUTABLE_CACHE_SECONDS, upsert: true, contentType: prepared.imageFile.type || undefined }), prepared.thumbnailFile && thumbnailPath - ? supabase.storage.from(PHOTO_BUCKET).upload(thumbnailPath, prepared.thumbnailFile, { cacheControl: IMMUTABLE_CACHE_SECONDS, upsert: false, contentType: prepared.thumbnailFile.type }) + ? supabase.storage.from(PHOTO_BUCKET).upload(thumbnailPath, prepared.thumbnailFile, { cacheControl: IMMUTABLE_CACHE_SECONDS, upsert: true, contentType: prepared.thumbnailFile.type }) : Promise.resolve(null), ]); if (imageUpload.error) { - if (thumbnailPath && thumbnailUpload && !thumbnailUpload.error) await supabase.storage.from(PHOTO_BUCKET).remove([thumbnailPath]); + if (thumbnailPath && thumbnailUpload && !thumbnailUpload.error) await removeObjects([thumbnailPath], input.file.name); failures.push(`${input.file.name}: ${imageUpload.error.message}`); failedClientIds.push(input.clientId); onItemComplete(); return; } - uploadedPaths.push(path); let thumbnailStoragePath: string | null = null; if (thumbnailUpload) { if (thumbnailUpload.error) { warnings.push(`${input.file.name}: thumbnail skipped`); } else if (thumbnailPath) { - uploadedPaths.push(thumbnailPath); thumbnailStoragePath = thumbnailPath; } } @@ -144,43 +157,76 @@ export async function uploadPhotoBatch(options: { }); if (rows.length > 0) { - // Re-check hashes against the database rather than local state, so a - // photo someone else uploaded mid-batch is skipped instead of failing - // the whole insert on the unique index. - const { data: clashData } = await supabase.from("photos").select("content_hash").eq("trip_id", trip.id).in("content_hash", rows.map((row) => row.content_hash)); - const clashes = new Set(((clashData ?? []) as Array<{ content_hash: string }>).map((row) => row.content_hash)); - const clashedRows = rows.filter((row) => clashes.has(row.content_hash)); - const freshRows = rows.filter((row) => !clashes.has(row.content_hash)); - if (clashedRows.length > 0) { - await supabase.storage.from(PHOTO_BUCKET).remove(clashedRows.flatMap((row) => [row.image_path, row.thumbnail_path].filter((rowPath): rowPath is string => Boolean(rowPath)))); - failures.push(`${clashedRows.length} media item${clashedRows.length === 1 ? "" : "s"} already uploaded by someone else, skipped`); - failedClientIds.push(...clashedRows.map((row) => row.client_id)); - } - const insertRows = freshRows.map((row) => ({ - trip_id: row.trip_id, - day_id: row.day_id, - uploader_name: row.uploader_name, - content_hash: row.content_hash, - media_type: row.media_type, - image_path: row.image_path, - thumbnail_path: row.thumbnail_path, - lat: row.lat, - lng: row.lng, - taken_at: row.taken_at, - caption: row.caption, - exif_found: row.exif_found, - })); - if (insertRows.length > 0) { - const { data: returnedRows, error: insertError } = await supabase.from("photos").insert(insertRows).select(); - if (insertError) { - if (uploadedPaths.length > 0) await supabase.storage.from(PHOTO_BUCKET).remove(uploadedPaths); - insertErrorMessage = insertError.message; - failedClientIds.push(...freshRows.map((row) => row.client_id)); - } else { - savedClientIds.push(...freshRows.map((row) => row.client_id)); - inserted = true; - insertedRows = (returnedRows ?? []) as Photo[]; + try { + // Re-check hashes against the database rather than local state, so a + // photo someone else uploaded mid-batch is skipped instead of failing + // the whole insert on the unique index. + const { data: clashData, error: clashError } = await supabase + .from("photos") + .select("content_hash,image_path,thumbnail_path") + .eq("trip_id", trip.id) + .in("content_hash", rows.map((row) => row.content_hash)); + if (clashError) throw clashError; + const clashes = new Map( + ((clashData ?? []) as Array<{ content_hash: string; image_path: string; thumbnail_path: string | null }>).map((row) => [row.content_hash, row]), + ); + const clashedRows = rows.filter((row) => clashes.has(row.content_hash)); + const freshRows = rows.filter((row) => !clashes.has(row.content_hash)); + if (clashedRows.length > 0) { + // New rows already use hash paths, so a racing upload normally lands on + // the live row's own objects and must not remove them. Legacy rows still + // use UUID paths, though; in that case the hash-path uploads are unused + // and can be removed without touching the legacy row's objects. + const unusedClashPaths = clashedRows.flatMap((row) => { + const existing = clashes.get(row.content_hash)!; + return [row.image_path, row.thumbnail_path].filter( + (path): path is string => Boolean(path) && path !== existing.image_path && path !== existing.thumbnail_path, + ); + }); + await removeObjects(unusedClashPaths, "duplicate cleanup"); + failures.push(`${clashedRows.length} media item${clashedRows.length === 1 ? "" : "s"} already uploaded by someone else, skipped`); + failedClientIds.push(...clashedRows.map((row) => row.client_id)); + } + const insertRows = freshRows.map((row) => ({ + trip_id: row.trip_id, + day_id: row.day_id, + uploader_name: row.uploader_name, + content_hash: row.content_hash, + media_type: row.media_type, + image_path: row.image_path, + thumbnail_path: row.thumbnail_path, + lat: row.lat, + lng: row.lng, + taken_at: row.taken_at, + caption: row.caption, + exif_found: row.exif_found, + })); + if (insertRows.length > 0) { + const { data: returnedRows, error: insertError } = await supabase.from("photos").insert(insertRows).select(); + if (insertError) { + // Only the fresh rows' objects: their hashes have no row, so nothing + // references those paths. The clashed ones belong to rows that exist. + await removeObjects(storagePathsOf(freshRows), "rollback"); + insertErrorMessage = insertError.message; + failedClientIds.push(...freshRows.map((row) => row.client_id)); + } else { + savedClientIds.push(...freshRows.map((row) => row.client_id)); + inserted = true; + insertedRows = (returnedRows ?? []) as Photo[]; + } } + } catch (error) { + // The clash check or the insert threw, so which hashes have rows is now + // unknown and blind cleanup could delete a live object. The uploads are + // content-addressed, so they sit on the keys a retry of this same batch + // will overwrite -- abandoning them costs one object each, not one per + // attempt. scripts/purge-orphan-objects.mjs sweeps any never retried. + insertErrorMessage = error instanceof Error + ? error.message + : typeof error === "object" && error !== null && "message" in error && typeof error.message === "string" + ? error.message + : "Could not save uploaded media."; + failedClientIds.push(...rows.map((row) => row.client_id)); } } diff --git a/scripts/purge-orphan-objects.mjs b/scripts/purge-orphan-objects.mjs new file mode 100644 index 0000000..454cd58 --- /dev/null +++ b/scripts/purge-orphan-objects.mjs @@ -0,0 +1,175 @@ +// Delete storage objects in the photo bucket that no photos row references. +// +// A failed upload batch leaves its already-uploaded objects behind: the path is +// a fresh uuid per attempt, so a retry uploads a second copy rather than +// overwriting the first, and nothing points at the abandoned one. Those orphans +// are unreachable from the app but still count against the storage quota. +// +// An object is an orphan only if its path appears in neither image_path nor +// thumbnail_path of any row. The live set is never touched, and objects younger +// than MIN_AGE_HOURS are always spared -- an upload whose row insert has not +// landed yet is indistinguishable from an orphan, so age is what separates them. +// +// Usage: +// node scripts/purge-orphan-objects.mjs # read-only: list, diff, report +// node scripts/purge-orphan-objects.mjs --apply # delete the orphans +// +// Read-only mode needs only the anon key (rows and the bucket are both publicly +// readable). Apply mode needs SUPABASE_SERVICE_ROLE_KEY because object deletes +// are member-gated by RLS. The orphan manifest is written either way. + +import { readFileSync, writeFileSync } from "node:fs"; + +const REPORT_PATH = new URL("../.orphan-report.json", import.meta.url).pathname; +const PHOTO_BUCKET = "trip-photos"; +const DELETE_BATCH = 100; +// An in-flight upload looks exactly like an orphan until its row is inserted. +const MIN_AGE_HOURS = 1; + +function readEnv(name) { + if (process.env[name]) return process.env[name]; + for (const file of [".env.local", ".env"]) { + try { + const line = readFileSync(new URL(`../${file}`, import.meta.url), "utf8") + .split("\n") + .find((entry) => entry.startsWith(`${name}=`)); + if (line) return line.slice(name.length + 1).trim(); + } catch { + // missing env file -- keep looking + } + } + return null; +} + +const SUPABASE_URL = readEnv("NEXT_PUBLIC_SUPABASE_URL"); +const ANON_KEY = readEnv("NEXT_PUBLIC_SUPABASE_ANON_KEY"); +const SERVICE_KEY = process.env.SUPABASE_SERVICE_ROLE_KEY ?? null; +const apply = process.argv.includes("--apply"); + +if (!SUPABASE_URL || !ANON_KEY) { + console.error("Missing NEXT_PUBLIC_SUPABASE_URL / NEXT_PUBLIC_SUPABASE_ANON_KEY."); + process.exit(1); +} +if (apply && !SERVICE_KEY) { + console.error("--apply needs SUPABASE_SERVICE_ROLE_KEY in the environment."); + process.exit(1); +} + +const writeKey = SERVICE_KEY ?? ANON_KEY; +const restHeaders = (key) => ({ apikey: key, Authorization: `Bearer ${key}`, "Content-Type": "application/json" }); + +async function rest(path, { method = "GET", key = ANON_KEY, body } = {}) { + const response = await fetch(`${SUPABASE_URL}${path}`, { + method, + headers: { ...restHeaders(key), ...(method === "DELETE" ? { Prefer: "return=minimal" } : {}) }, + body: body ? JSON.stringify(body) : undefined, + }); + if (!response.ok) throw new Error(`${method} ${path} -> ${response.status}: ${await response.text()}`); + return response.status === 204 ? null : response.json(); +} + +/** Every path any row points at, across both columns. */ +async function fetchReferencedPaths() { + const referenced = new Set(); + for (let from = 0; ; from += 1000) { + const page = await rest(`/rest/v1/photos?select=image_path,thumbnail_path&limit=1000&offset=${from}`); + for (const row of page) { + if (row.image_path) referenced.add(row.image_path); + if (row.thumbnail_path) referenced.add(row.thumbnail_path); + } + if (page.length < 1000) break; + } + return referenced; +} + +// The list API returns one level at a time: entries with metadata are objects, +// entries without are folders to descend into. +async function listObjects(prefix = "") { + const objects = []; + for (let from = 0; ; from += 1000) { + const page = await rest(`/storage/v1/object/list/${PHOTO_BUCKET}`, { + method: "POST", + body: { prefix, limit: 1000, offset: from }, + }); + for (const entry of page) { + const path = prefix ? `${prefix}/${entry.name}` : entry.name; + if (entry.metadata?.size === undefined || entry.metadata === null) objects.push(...await listObjects(path)); + else objects.push({ path, size: entry.metadata.size, createdAt: entry.created_at ?? null }); + } + if (page.length < 1000) break; + } + return objects; +} + +function partitionOrphans(objects, referenced, now) { + const cutoff = now - MIN_AGE_HOURS * 3600 * 1000; + const live = []; + const orphans = []; + const tooNew = []; + for (const object of objects) { + if (referenced.has(object.path)) live.push(object); + else if (object.createdAt && Date.parse(object.createdAt) > cutoff) tooNew.push(object); + else orphans.push(object); + } + return { live, orphans, tooNew }; +} + +const mb = (bytes) => (bytes / 1024 / 1024).toFixed(1); +const sum = (objects) => objects.reduce((total, object) => total + object.size, 0); + +const referenced = await fetchReferencedPaths(); +console.log(`Rows reference ${referenced.size} distinct paths. Listing bucket...`); + +const objects = await listObjects(); +const { live, orphans, tooNew } = partitionOrphans(objects, referenced, Date.now()); +const missing = [...referenced].filter((path) => !objects.some((object) => object.path === path)); + +writeFileSync(REPORT_PATH, JSON.stringify({ + totals: { + referencedPaths: referenced.size, + objects: objects.length, + live: live.length, + liveBytes: sum(live), + orphans: orphans.length, + orphanBytes: sum(orphans), + sparedAsTooNew: tooNew.length, + referencedButMissing: missing.length, + }, + orphans: orphans.map((object) => object.path), + missing, +}, null, 2)); + +console.log(`\n live (referenced) ${String(live.length).padStart(5)} objects ${mb(sum(live)).padStart(8)} MB`); +console.log(` orphans ${String(orphans.length).padStart(5)} objects ${mb(sum(orphans)).padStart(8)} MB <- reclaimable`); +if (tooNew.length > 0) console.log(` spared (< ${MIN_AGE_HOURS}h old) ${String(tooNew.length).padStart(5)} objects ${mb(sum(tooNew)).padStart(8)} MB`); +if (missing.length > 0) console.log(`\n WARNING: ${missing.length} referenced paths have no object -- broken rows, not touched.`); +console.log(`\n bucket after purge: ${(sum(live) / 1024 / 1024 / 1024).toFixed(2)} GB`); +console.log(`\nManifest: ${REPORT_PATH}`); + +if (!apply) { + console.log("\nDry run complete. Re-run with --apply and SUPABASE_SERVICE_ROLE_KEY to delete the orphans."); + process.exit(0); +} + +if (orphans.length === 0) { + console.log("\nNothing to delete."); + process.exit(0); +} + +// Re-read the rows rather than trusting the set from the top of this run: a +// photo saved while we were listing would otherwise be deleted out from under +// its own row. +console.log("\nRe-checking rows before deleting..."); +const referencedNow = await fetchReferencedPaths(); +const stale = orphans.filter((object) => referencedNow.has(object.path)); +const confirmed = orphans.filter((object) => !referencedNow.has(object.path)); +if (stale.length > 0) console.log(` ${stale.length} object(s) picked up a row since listing -- sparing them.`); + +let deleted = 0; +for (let from = 0; from < confirmed.length; from += DELETE_BATCH) { + const batch = confirmed.slice(from, from + DELETE_BATCH).map((object) => object.path); + await rest(`/storage/v1/object/${PHOTO_BUCKET}`, { method: "DELETE", key: writeKey, body: { prefixes: batch } }); + deleted += batch.length; + console.log(` deleted ${deleted}/${confirmed.length}`); +} +console.log(`\nDeleted ${deleted} orphaned objects, reclaiming ${mb(sum(confirmed))} MB.`); From 8834825c5211334c2a5235d973c1721931123916 Mon Sep 17 00:00:00 2001 From: Ned Cutler Date: Sat, 11 Jul 2026 13:35:49 -0400 Subject: [PATCH 2/3] Re-check references before insert-failure rollback A unique-violation insert usually means a racer inserted one of the batch's hashes after the clash check, and that row references the same content-addressed paths this batch uploaded. Blindly rolling back the fresh rows' objects would delete the racer's live objects, so re-query which hashes gained rows and remove only unreferenced paths; if the re-check itself fails, leave the objects for the retry/purge to handle. Also key the purge script's min-age guard on updated_at: a retried upload upserts onto its old key without resetting created_at, so a just-re-uploaded object awaiting its row insert must count as new. --- lib/photo-upload.test.ts | 52 +++++++++++++++++++++++++------- lib/photo-upload.ts | 22 ++++++++++++-- scripts/purge-orphan-objects.mjs | 7 +++-- 3 files changed, 65 insertions(+), 16 deletions(-) diff --git a/lib/photo-upload.test.ts b/lib/photo-upload.test.ts index 4488abb..7113367 100644 --- a/lib/photo-upload.test.ts +++ b/lib/photo-upload.test.ts @@ -43,8 +43,9 @@ function existingPhoto(contentHash: string): Photo { }; } -function fakeSupabase(options: { failUploadFor?: string[]; clashHashes?: string[]; legacyClashHashes?: string[]; clashError?: string; insertErrorMessage?: string; insertThrows?: string } = {}) { +function fakeSupabase(options: { failUploadFor?: string[]; clashHashes?: string[]; legacyClashHashes?: string[]; lateClashHashes?: string[]; clashError?: string; recheckError?: string; insertErrorMessage?: string; insertThrows?: string } = {}) { const uploaded: string[] = []; + let clashQueries = 0; const upserted: boolean[] = []; const removed: string[] = []; const inserted: Array> = []; @@ -66,16 +67,26 @@ function fakeSupabase(options: { failUploadFor?: string[]; clashHashes?: string[ from: () => ({ select: () => ({ eq: () => ({ - in: async () => ({ - data: options.clashError - ? null - : [...(options.clashHashes ?? []), ...(options.legacyClashHashes ?? [])].map((hash) => ({ - content_hash: hash, - image_path: options.legacyClashHashes?.includes(hash) ? `lofoten-2026/legacy-${hash}.mp4` : `lofoten-2026/${hash}.mp4`, - thumbnail_path: null, - })), - error: options.clashError ? { message: options.clashError } : null, - }), + in: async () => { + clashQueries += 1; + const errorMessage = clashQueries === 1 ? options.clashError : options.recheckError; + if (errorMessage) return { data: null, error: { message: errorMessage } }; + const hashes = [ + ...(options.clashHashes ?? []), + ...(options.legacyClashHashes ?? []), + // Rows a racer inserted between the clash check and the insert: + // visible only from the second (post-failure) query onward. + ...(clashQueries > 1 ? options.lateClashHashes ?? [] : []), + ]; + return { + data: hashes.map((hash) => ({ + content_hash: hash, + image_path: options.legacyClashHashes?.includes(hash) ? `lofoten-2026/legacy-${hash}.mp4` : `lofoten-2026/${hash}.mp4`, + thumbnail_path: null, + })), + error: null, + }; + }, }), }), insert: (rows: Array>) => ({ @@ -203,6 +214,25 @@ describe("uploadPhotoBatch", () => { expect(result.savedClientIds).toEqual([]); }); + it("spares objects a racing insert claimed when the unique index rejects the batch", async () => { + const { client, removed } = fakeSupabase({ lateClashHashes: ["hash-a"], insertErrorMessage: "unique constraint" }); + const { result } = await batch(client, [input({ clientId: "a" }), input({ clientId: "b" })]); + + expect(result.insertErrorMessage).toBe("unique constraint"); + // hash-a gained a row mid-window, and that row references our own + // content-addressed objects; only hash-b's object is truly unreferenced. + expect(removed).toEqual(["lofoten-2026/hash-b.mp4"]); + }); + + it("skips rollback cleanup when the post-failure re-check fails", async () => { + const { client, removed } = fakeSupabase({ insertErrorMessage: "insert exploded", recheckError: "recheck down" }); + const { result } = await batch(client, [input({ clientId: "a" })]); + + expect(result.insertErrorMessage).toBe("insert exploded"); + expect(removed).toEqual([]); + expect(result.warnings.some((warning) => warning.includes("recheck down"))).toBe(true); + }); + it("leaves uploads in place when the insert throws", async () => { const { client, removed, uploaded } = fakeSupabase({ insertThrows: "network died" }); const { result } = await batch(client, [input({ clientId: "a" })]); diff --git a/lib/photo-upload.ts b/lib/photo-upload.ts index 25b116e..395d3ce 100644 --- a/lib/photo-upload.ts +++ b/lib/photo-upload.ts @@ -204,9 +204,25 @@ export async function uploadPhotoBatch(options: { if (insertRows.length > 0) { const { data: returnedRows, error: insertError } = await supabase.from("photos").insert(insertRows).select(); if (insertError) { - // Only the fresh rows' objects: their hashes have no row, so nothing - // references those paths. The clashed ones belong to rows that exist. - await removeObjects(storagePathsOf(freshRows), "rollback"); + // The likely insert error is the unique index: a racer inserted one + // of these hashes after the clash check above, and that row now + // references our content-addressed paths. Re-check which hashes + // gained rows and remove only the paths nothing references. + const { data: recheckData, error: recheckError } = await supabase + .from("photos") + .select("content_hash,image_path,thumbnail_path") + .eq("trip_id", trip.id) + .in("content_hash", freshRows.map((row) => row.content_hash)); + if (recheckError) { + warnings.push(`rollback: skipped storage cleanup, could not confirm the objects are unreferenced (${recheckError.message})`); + } else { + const referencedPaths = new Set( + ((recheckData ?? []) as Array<{ image_path: string; thumbnail_path: string | null }>).flatMap((row) => + [row.image_path, row.thumbnail_path].filter((path): path is string => Boolean(path)), + ), + ); + await removeObjects(storagePathsOf(freshRows).filter((path) => !referencedPaths.has(path)), "rollback"); + } insertErrorMessage = insertError.message; failedClientIds.push(...freshRows.map((row) => row.client_id)); } else { diff --git a/scripts/purge-orphan-objects.mjs b/scripts/purge-orphan-objects.mjs index 454cd58..dc723c0 100644 --- a/scripts/purge-orphan-objects.mjs +++ b/scripts/purge-orphan-objects.mjs @@ -94,7 +94,10 @@ async function listObjects(prefix = "") { for (const entry of page) { const path = prefix ? `${prefix}/${entry.name}` : entry.name; if (entry.metadata?.size === undefined || entry.metadata === null) objects.push(...await listObjects(path)); - else objects.push({ path, size: entry.metadata.size, createdAt: entry.created_at ?? null }); + // A retried upload upserts onto its old key, which refreshes updated_at + // but not created_at -- judge age by the newest write so an object that + // was just re-uploaded (insert still pending) counts as new. + else objects.push({ path, size: entry.metadata.size, lastWriteAt: entry.updated_at ?? entry.created_at ?? null }); } if (page.length < 1000) break; } @@ -108,7 +111,7 @@ function partitionOrphans(objects, referenced, now) { const tooNew = []; for (const object of objects) { if (referenced.has(object.path)) live.push(object); - else if (object.createdAt && Date.parse(object.createdAt) > cutoff) tooNew.push(object); + else if (object.lastWriteAt && Date.parse(object.lastWriteAt) > cutoff) tooNew.push(object); else orphans.push(object); } return { live, orphans, tooNew }; From 6e75183f49c63bdb96c7c2c1b2449ac3b6aa836c Mon Sep 17 00:00:00 2001 From: Ned Cutler Date: Sat, 11 Jul 2026 13:45:22 -0400 Subject: [PATCH 3/3] Harden orphan prevention review fixes --- lib/photo-upload.test.ts | 15 ++++++-- lib/photo-upload.ts | 28 ++++++++++----- scripts/purge-orphan-objects.mjs | 61 +++++++++----------------------- 3 files changed, 49 insertions(+), 55 deletions(-) diff --git a/lib/photo-upload.test.ts b/lib/photo-upload.test.ts index 7113367..f898f90 100644 --- a/lib/photo-upload.test.ts +++ b/lib/photo-upload.test.ts @@ -43,7 +43,7 @@ function existingPhoto(contentHash: string): Photo { }; } -function fakeSupabase(options: { failUploadFor?: string[]; clashHashes?: string[]; legacyClashHashes?: string[]; lateClashHashes?: string[]; clashError?: string; recheckError?: string; insertErrorMessage?: string; insertThrows?: string } = {}) { +function fakeSupabase(options: { failUploadFor?: string[]; existingUploadFor?: string[]; clashHashes?: string[]; legacyClashHashes?: string[]; lateClashHashes?: string[]; clashError?: string; recheckError?: string; insertErrorMessage?: string; insertThrows?: string } = {}) { const uploaded: string[] = []; let clashQueries = 0; const upserted: boolean[] = []; @@ -54,6 +54,7 @@ function fakeSupabase(options: { failUploadFor?: string[]; clashHashes?: string[ from: () => ({ upload: async (path: string, file: File, uploadOptions?: { upsert?: boolean }) => { if (options.failUploadFor?.some((name) => file.name.startsWith(name))) return { error: { message: "storage exploded" } }; + if (options.existingUploadFor?.some((name) => file.name.startsWith(name))) return { error: { message: "The resource already exists", statusCode: "409" } }; uploaded.push(path); upserted.push(Boolean(uploadOptions?.upsert)); return { error: null }; @@ -192,7 +193,7 @@ describe("uploadPhotoBatch", () => { expect(removed).toEqual([]); }); - it("keys objects by content hash and upserts, so a retry overwrites instead of orphaning", async () => { + it("keys objects by content hash without overwriting immutable objects", async () => { const first = fakeSupabase(); await batch(first.client, [input({ clientId: "a" })]); // Same file, second attempt: a batch that died before its insert last time. @@ -201,7 +202,15 @@ describe("uploadPhotoBatch", () => { expect(first.uploaded).toEqual(["lofoten-2026/hash-a.mp4"]); expect(second.uploaded).toEqual(first.uploaded); - expect(first.upserted).toEqual([true]); + expect(first.upserted).toEqual([false]); + }); + + it("reuses an immutable object left by an earlier unknown-outcome attempt", async () => { + const { client, inserted } = fakeSupabase({ existingUploadFor: ["a.jpg"] }); + const { result } = await batch(client, [input({ clientId: "a" })]); + + expect(result.savedClientIds).toEqual(["a"]); + expect(inserted).toHaveLength(1); }); it("rolls back only the fresh objects when the insert fails", async () => { diff --git a/lib/photo-upload.ts b/lib/photo-upload.ts index 395d3ce..6058cbb 100644 --- a/lib/photo-upload.ts +++ b/lib/photo-upload.ts @@ -96,6 +96,15 @@ export async function uploadPhotoBatch(options: { if (error) warnings.push(`${label}: storage cleanup failed (${error.message})`); }; + // Hash paths are immutable. A retry may find the object left by an earlier + // attempt whose database outcome was unknown; that is success, not a reason + // to overwrite bytes already cached under this key. + const isAlreadyStored = (error: unknown) => { + if (!error || typeof error !== "object") return false; + const value = error as { statusCode?: string | number; status?: string | number; message?: string }; + return Number(value.statusCode ?? value.status) === 409 || /already exists|duplicate/i.test(value.message ?? ""); + }; + const { uploads: uploadCandidates, duplicates } = partitionDuplicatePhotos( inputs.map((input) => ({ input, contentHash: input.contentHash, mediaType: input.mediaType, takenAt: input.exif?.takenAt ?? null, coordinate: input.coordinate })), existingPhotos, @@ -110,21 +119,23 @@ export async function uploadPhotoBatch(options: { const prepared = await prepareMediaFiles(input.file); const extension = storageFileExtension(prepared.imageFile); // Content-addressed, so an upload is idempotent: a retry of a batch that - // died before its insert lands on the same key and overwrites the object - // instead of stranding it under a fresh uuid. `(trip_id, content_hash)` is + // died before its insert lands on the same immutable key and reuses the + // existing object instead of stranding a fresh uuid. `(trip_id, content_hash)` is // already unique in the database, so one key here is one row there. const path = `${trip.slug}/${input.contentHash}.${extension}`; const thumbnailPath = prepared.thumbnailFile ? `${trip.slug}/thumbs/${input.contentHash}.jpg` : null; // The thumbnail never depends on the image upload, so both go up // together instead of back to back. const [imageUpload, thumbnailUpload] = await Promise.all([ - supabase.storage.from(PHOTO_BUCKET).upload(path, prepared.imageFile, { cacheControl: IMMUTABLE_CACHE_SECONDS, upsert: true, contentType: prepared.imageFile.type || undefined }), + supabase.storage.from(PHOTO_BUCKET).upload(path, prepared.imageFile, { cacheControl: IMMUTABLE_CACHE_SECONDS, upsert: false, contentType: prepared.imageFile.type || undefined }), prepared.thumbnailFile && thumbnailPath - ? supabase.storage.from(PHOTO_BUCKET).upload(thumbnailPath, prepared.thumbnailFile, { cacheControl: IMMUTABLE_CACHE_SECONDS, upsert: true, contentType: prepared.thumbnailFile.type }) + ? supabase.storage.from(PHOTO_BUCKET).upload(thumbnailPath, prepared.thumbnailFile, { cacheControl: IMMUTABLE_CACHE_SECONDS, upsert: false, contentType: prepared.thumbnailFile.type }) : Promise.resolve(null), ]); - if (imageUpload.error) { - if (thumbnailPath && thumbnailUpload && !thumbnailUpload.error) await removeObjects([thumbnailPath], input.file.name); + if (imageUpload.error && !isAlreadyStored(imageUpload.error)) { + if (thumbnailPath && thumbnailUpload && !thumbnailUpload.error) { + warnings.push(`${input.file.name}: thumbnail retained for a safe retry`); + } failures.push(`${input.file.name}: ${imageUpload.error.message}`); failedClientIds.push(input.clientId); onItemComplete(); @@ -132,7 +143,7 @@ export async function uploadPhotoBatch(options: { } let thumbnailStoragePath: string | null = null; if (thumbnailUpload) { - if (thumbnailUpload.error) { + if (thumbnailUpload.error && !isAlreadyStored(thumbnailUpload.error)) { warnings.push(`${input.file.name}: thumbnail skipped`); } else if (thumbnailPath) { thumbnailStoragePath = thumbnailPath; @@ -242,7 +253,8 @@ export async function uploadPhotoBatch(options: { : typeof error === "object" && error !== null && "message" in error && typeof error.message === "string" ? error.message : "Could not save uploaded media."; - failedClientIds.push(...rows.map((row) => row.client_id)); + const alreadyFailed = new Set(failedClientIds); + failedClientIds.push(...rows.map((row) => row.client_id).filter((clientId) => !alreadyFailed.has(clientId))); } } diff --git a/scripts/purge-orphan-objects.mjs b/scripts/purge-orphan-objects.mjs index dc723c0..0d34f59 100644 --- a/scripts/purge-orphan-objects.mjs +++ b/scripts/purge-orphan-objects.mjs @@ -1,9 +1,9 @@ // Delete storage objects in the photo bucket that no photos row references. // -// A failed upload batch leaves its already-uploaded objects behind: the path is -// a fresh uuid per attempt, so a retry uploads a second copy rather than -// overwriting the first, and nothing points at the abandoned one. Those orphans -// are unreachable from the app but still count against the storage quota. +// Legacy failed batches can leave UUID-path objects behind. Content-addressed +// uploads can also leave one stable object behind when a database outcome is +// unknown. Those objects are unreachable from the app but still count against +// the storage quota. // // An object is an orphan only if its path appears in neither image_path nor // thumbnail_path of any row. The live set is never touched, and objects younger @@ -12,17 +12,15 @@ // // Usage: // node scripts/purge-orphan-objects.mjs # read-only: list, diff, report -// node scripts/purge-orphan-objects.mjs --apply # delete the orphans -// -// Read-only mode needs only the anon key (rows and the bucket are both publicly -// readable). Apply mode needs SUPABASE_SERVICE_ROLE_KEY because object deletes -// are member-gated by RLS. The orphan manifest is written either way. +// Deletion is deliberately not automated here: a reference can be created +// between any client-side confirmation and delete. Safe deletion needs a +// server-side maintenance protocol shared with uploads. The manifest is an +// inventory for inspection until that protocol exists. import { readFileSync, writeFileSync } from "node:fs"; const REPORT_PATH = new URL("../.orphan-report.json", import.meta.url).pathname; const PHOTO_BUCKET = "trip-photos"; -const DELETE_BATCH = 100; // An in-flight upload looks exactly like an orphan until its row is inserted. const MIN_AGE_HOURS = 1; @@ -43,19 +41,17 @@ function readEnv(name) { const SUPABASE_URL = readEnv("NEXT_PUBLIC_SUPABASE_URL"); const ANON_KEY = readEnv("NEXT_PUBLIC_SUPABASE_ANON_KEY"); -const SERVICE_KEY = process.env.SUPABASE_SERVICE_ROLE_KEY ?? null; const apply = process.argv.includes("--apply"); if (!SUPABASE_URL || !ANON_KEY) { console.error("Missing NEXT_PUBLIC_SUPABASE_URL / NEXT_PUBLIC_SUPABASE_ANON_KEY."); process.exit(1); } -if (apply && !SERVICE_KEY) { - console.error("--apply needs SUPABASE_SERVICE_ROLE_KEY in the environment."); +if (apply) { + console.error("--apply is disabled: deletion is unsafe without a lock shared with uploads."); process.exit(1); } -const writeKey = SERVICE_KEY ?? ANON_KEY; const restHeaders = (key) => ({ apikey: key, Authorization: `Bearer ${key}`, "Content-Type": "application/json" }); async function rest(path, { method = "GET", key = ANON_KEY, body } = {}) { @@ -72,7 +68,7 @@ async function rest(path, { method = "GET", key = ANON_KEY, body } = {}) { async function fetchReferencedPaths() { const referenced = new Set(); for (let from = 0; ; from += 1000) { - const page = await rest(`/rest/v1/photos?select=image_path,thumbnail_path&limit=1000&offset=${from}`); + const page = await rest(`/rest/v1/photos?select=image_path,thumbnail_path&order=id.asc&limit=1000&offset=${from}`); for (const row of page) { if (row.image_path) referenced.add(row.image_path); if (row.thumbnail_path) referenced.add(row.thumbnail_path); @@ -111,8 +107,11 @@ function partitionOrphans(objects, referenced, now) { const tooNew = []; for (const object of objects) { if (referenced.has(object.path)) live.push(object); - else if (object.lastWriteAt && Date.parse(object.lastWriteAt) > cutoff) tooNew.push(object); - else orphans.push(object); + else { + const lastWriteAt = object.lastWriteAt ? Date.parse(object.lastWriteAt) : Number.NaN; + if (!Number.isFinite(lastWriteAt) || lastWriteAt > cutoff) tooNew.push(object); + else orphans.push(object); + } } return { live, orphans, tooNew }; } @@ -149,30 +148,4 @@ if (missing.length > 0) console.log(`\n WARNING: ${missing.length} referenced p console.log(`\n bucket after purge: ${(sum(live) / 1024 / 1024 / 1024).toFixed(2)} GB`); console.log(`\nManifest: ${REPORT_PATH}`); -if (!apply) { - console.log("\nDry run complete. Re-run with --apply and SUPABASE_SERVICE_ROLE_KEY to delete the orphans."); - process.exit(0); -} - -if (orphans.length === 0) { - console.log("\nNothing to delete."); - process.exit(0); -} - -// Re-read the rows rather than trusting the set from the top of this run: a -// photo saved while we were listing would otherwise be deleted out from under -// its own row. -console.log("\nRe-checking rows before deleting..."); -const referencedNow = await fetchReferencedPaths(); -const stale = orphans.filter((object) => referencedNow.has(object.path)); -const confirmed = orphans.filter((object) => !referencedNow.has(object.path)); -if (stale.length > 0) console.log(` ${stale.length} object(s) picked up a row since listing -- sparing them.`); - -let deleted = 0; -for (let from = 0; from < confirmed.length; from += DELETE_BATCH) { - const batch = confirmed.slice(from, from + DELETE_BATCH).map((object) => object.path); - await rest(`/storage/v1/object/${PHOTO_BUCKET}`, { method: "DELETE", key: writeKey, body: { prefixes: batch } }); - deleted += batch.length; - console.log(` deleted ${deleted}/${confirmed.length}`); -} -console.log(`\nDeleted ${deleted} orphaned objects, reclaiming ${mb(sum(confirmed))} MB.`); +console.log("\nInventory complete. Automatic deletion is disabled until uploads and cleanup share a server-side lock.");