From 646ef7e34415e9f267badb4b90866c6931d0484e Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:05:00 -0400 Subject: [PATCH 1/4] Let a cloud image be copied under a new id on the server A placed image's id is also its image's id, so copying one to another page of a cloud strategy needs a row of its own. images:copyAsset gives the copy's id a row pointing at the same stored bytes, as a duplicated strategy's images do, taking over a placeholder if the copy's content landed first. Additive: no client calls it yet. Co-Authored-By: Claude Opus 5.5 --- convex/function_spec.json | 53 ++++ convex/imageCopy.test.ts | 295 ++++++++++++++++++++ convex/images.ts | 55 ++++ lib/collab/generated/convex_models.dart | 40 +++ lib/collab/generated/icarus_convex_api.dart | 25 ++ 5 files changed, 468 insertions(+) create mode 100644 convex/imageCopy.test.ts diff --git a/convex/function_spec.json b/convex/function_spec.json index 93232fbf..b56f025c 100644 --- a/convex/function_spec.json +++ b/convex/function_spec.json @@ -40101,6 +40101,59 @@ "kind": "public" } }, + { + "args": { + "type": "object", + "value": { + "clientProtocolVersion": { + "fieldType": { + "type": "number" + }, + "optional": false + }, + "sourceAssetPublicId": { + "fieldType": { + "type": "string" + }, + "optional": false + }, + "strategyPublicId": { + "fieldType": { + "type": "string" + }, + "optional": false + }, + "targetAssetPublicId": { + "fieldType": { + "type": "string" + }, + "optional": false + } + } + }, + "functionType": "Mutation", + "identifier": "images.js:copyAsset", + "returns": { + "type": "union", + "value": [ + { + "type": "literal", + "value": "copied" + }, + { + "type": "literal", + "value": "uploading" + }, + { + "type": "literal", + "value": "unavailable" + } + ] + }, + "visibility": { + "kind": "public" + } + }, { "args": { "type": "object", diff --git a/convex/imageCopy.test.ts b/convex/imageCopy.test.ts new file mode 100644 index 00000000..a8ba8db2 --- /dev/null +++ b/convex/imageCopy.test.ts @@ -0,0 +1,295 @@ +import { + convexTest, + type TestConvexForDataModel, + type TestConvexForDataModelAndIdentity, +} from "convex-test"; +import { makeFunctionReference } from "convex/server"; +import { afterEach, beforeAll, describe, expect, test, vi } from "vitest"; +import type { DataModel } from "./_generated/dataModel"; +import { markAssetReferencesReady } from "./lib/assetReferences"; +import { CURRENT_CLOUD_PROTOCOL_VERSION } from "./lib/cloudProtocol"; +import schema from "./schema"; +import { modules } from "./test.setup"; + +const ensureCurrentUser = makeFunctionReference<"mutation">( + "users:ensureCurrentUser", +); +const createStrategy = makeFunctionReference<"mutation">( + "strategies:createWithInitialPage", +); +const deleteStrategy = makeFunctionReference<"mutation">("strategies:delete"); +const addPage = makeFunctionReference<"mutation">("pages:add"); +const applyBatch = makeFunctionReference<"mutation">("ops:applyBatch"); +const purgeOldTombstones = makeFunctionReference<"mutation">( + "maintenance:purgeOldTombstones", +); +const listImages = makeFunctionReference<"query">("images:listForStrategy"); +const copyAsset = makeFunctionReference<"mutation">("images:copyAsset"); + +type Harness = TestConvexForDataModel; +type RootHarness = TestConvexForDataModelAndIdentity; + +const protocol = { clientProtocolVersion: CURRENT_CLOUD_PROTOCOL_VERSION }; +const strategyPublicId = "copy-strategy"; +const firstPage = "copy-page-1"; +const secondPage = "copy-page-2"; +const original = "placed-image"; +const copy = `placed-image~cp1~6f1c2d0e-3b4a-4c5d-8e9f-0a1b2c3d4e5f`; +const settings = { agentSize: 40, abilitySize: 30, useNeutralTeamColors: true }; + +function identity(subject: string) { + return { + issuer: "https://image-copy.test", + subject, + tokenIdentifier: `image-copy|${subject}`, + name: `User ${subject}`, + }; +} + +/// A two-page strategy whose first page shows a placed image, with the +/// image's upload [status] ("active" once its bytes landed). +async function createHarness(status: "active" | "pending" = "active"): Promise<{ + t: RootHarness; + owner: Harness; + other: Harness; +}> { + const t = convexTest(schema, modules); + await t.run(markAssetReferencesReady); + const owner = t.withIdentity(identity("owner")); + const other = t.withIdentity(identity("other")); + await owner.mutation(ensureCurrentUser, protocol); + await other.mutation(ensureCurrentUser, protocol); + await owner.mutation(createStrategy, { + ...protocol, + publicId: strategyPublicId, + name: "Bind B split", + mapData: "bind", + initialPagePublicId: firstPage, + initialPageName: "Setup", + initialPageIsAutoNamed: false, + initialPageIsAttack: true, + initialPageSettings: settings, + }); + await owner.mutation(addPage, { + ...protocol, + strategyPublicId, + pagePublicId: secondPage, + name: "Hit", + sortIndex: 1, + isAttack: true, + expectedRevision: 0, + }); + await t.run(async (ctx) => { + const strategy = await ctx.db + .query("strategies") + .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) + .unique(); + const now = Date.now(); + await ctx.db.insert("imageAssets", { + publicId: original, + provider: "r2", + strategyId: strategy!._id, + uploadAttemptPublicId: "placed-image-attempt", + objectKey: `strategies/${strategyPublicId}/${original}.png`, + uploadStatus: status, + fileExtension: ".png", + mimeType: "image/png", + width: 64, + height: 32, + byteSize: 100, + ...(status === "active" ? { uploadedAt: now } : {}), + createdAt: now, + updatedAt: now, + }); + }); + await addImage(owner, original, firstPage); + return { t, owner, other }; +} + +async function addImage(user: Harness, id: string, pagePublicId: string) { + await user.mutation(applyBatch, { + ...protocol, + strategyPublicId, + clientId: `add-${id}`, + ops: [ + { + opId: `add-${id}`, + type: "element.add", + elementPublicId: id, + pagePublicId, + payload: { + kind: "image", + payloadVersion: 1, + data: { id, elementType: "image", scale: 2 }, + }, + sortIndex: 1, + }, + ], + }); +} + +async function copyImage(user: Harness) { + return await user.mutation(copyAsset, { + ...protocol, + strategyPublicId, + sourceAssetPublicId: original, + targetAssetPublicId: copy, + }); +} + +type ImageRow = { publicId: string; uploadStatus: string; url: string | null }; + +async function images(user: Harness) { + const rows = (await user.query(listImages, { strategyPublicId })) as ImageRow[]; + return Object.fromEntries( + rows.map((row) => [row.publicId, [row.uploadStatus, row.url]]), + ); +} + +async function rowsFor(t: RootHarness, publicId: string) { + return await t.run(async (ctx) => + (await ctx.db.query("imageAssets").collect()).filter( + (row) => row.publicId === publicId, + ), + ); +} + +const url = `https://media.copy.test/strategies/${strategyPublicId}/${original}.png`; + +beforeAll(() => { + process.env.R2_ACCOUNT_ID = "copy-account"; + process.env.R2_BUCKET = "copy-bucket"; + process.env.R2_ACCESS_KEY_ID = "copy-access-key"; + process.env.R2_SECRET_ACCESS_KEY = "copy-secret"; + process.env.R2_PUBLIC_BASE_URL = "https://media.copy.test"; + process.env.R2_S3_ENDPOINT = "https://copy.r2.test"; +}); + +afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); +}); + +describe("images:copyAsset", () => { + test("a copied image shows the original's picture under its own id", async () => { + const { owner } = await createHarness(); + + expect(await copyImage(owner)).toBe("copied"); + await addImage(owner, copy, secondPage); + + expect(await images(owner)).toEqual({ + [original]: ["active", url], + [copy]: ["active", url], + }); + }); + + test("a copy whose content landed first takes over its placeholder", async () => { + const { t, owner } = await createHarness(); + await addImage(owner, copy, secondPage); + expect((await images(owner))[copy]).toEqual(["pending", null]); + + expect(await copyImage(owner)).toBe("copied"); + + expect((await images(owner))[copy]).toEqual(["active", url]); + expect((await rowsFor(t, copy)).map((row) => row.uploadStatus)).toEqual([ + "active", + ]); + }); + + test("copying again changes nothing", async () => { + const { t, owner } = await createHarness(); + await copyImage(owner); + + expect(await copyImage(owner)).toBe("copied"); + expect(await rowsFor(t, copy)).toHaveLength(1); + }); + + test("an image still uploading has nothing to copy yet", async () => { + const { t, owner } = await createHarness("pending"); + await addImage(owner, copy, secondPage); + + expect(await copyImage(owner)).toBe("uploading"); + // The copy's placeholder waits, as for any image on its way. + expect((await rowsFor(t, copy)).map((row) => row.uploadStatus)).toEqual([ + "pending", + ]); + }); + + test("an image the strategy cannot show is unavailable", async () => { + const { owner } = await createHarness(); + + expect( + await owner.mutation(copyAsset, { + ...protocol, + strategyPublicId, + sourceAssetPublicId: "missing-image", + targetAssetPublicId: `missing-image~cp1~6f1c2d0e-3b4a-4c5d-8e9f-0a1b2c3d4e5f`, + }), + ).toBe("unavailable"); + }); + + test("only an editor of the strategy can copy, and never onto itself", async () => { + const { owner, other } = await createHarness(); + + await expect(copyImage(other)).rejects.toThrow(); + await expect( + owner.mutation(copyAsset, { + ...protocol, + strategyPublicId, + sourceAssetPublicId: original, + targetAssetPublicId: original, + }), + ).rejects.toThrow(); + }); + + test("deleting the original keeps the copy's bytes; deleting both frees them", async () => { + vi.useFakeTimers(); + const fetchMock = vi.fn( + async (_input: RequestInfo | URL, _init?: RequestInit) => + new Response(null, { status: 204 }), + ); + vi.stubGlobal("fetch", fetchMock); + const { t, owner } = await createHarness(); + await copyImage(owner); + await addImage(owner, copy, secondPage); + + await owner.mutation(applyBatch, { + ...protocol, + strategyPublicId, + clientId: "remove-original", + ops: [ + { + opId: "remove-original", + type: "element.delete", + elementPublicId: original, + pagePublicId: firstPage, + expectedElementRevision: 1, + }, + ], + }); + vi.setSystemTime(Date.now() + 31 * 24 * 60 * 60 * 1000); + await t.mutation(purgeOldTombstones, {}); + await t.finishAllScheduledFunctions(vi.runAllTimers); + + expect(fetchMock).not.toHaveBeenCalled(); + expect((await images(owner))[copy]).toEqual(["active", url]); + + const revision = await t.run(async (ctx) => { + const strategy = await ctx.db + .query("strategies") + .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) + .unique(); + return strategy!.revision; + }); + await owner.mutation(deleteStrategy, { + ...protocol, + strategyPublicId, + expectedRevision: revision, + }); + await t.finishAllScheduledFunctions(vi.runAllTimers); + + expect( + fetchMock.mock.calls.map((call) => new URL(String(call[0])).pathname), + ).toEqual([`/copy-bucket/strategies/${strategyPublicId}/${original}.png`]); + }); +}); diff --git a/convex/images.ts b/convex/images.ts index 945b30ec..48aaa523 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -7,6 +7,7 @@ import { isAssetReferenced, } from "./lib/assetReferences"; import { + copyActiveAssetToStrategy, getActiveAssetForStrategy, inferFileExtension, getViewerAssetForStrategy, @@ -23,6 +24,7 @@ import { internalAction, internalMutation, internalQuery, + mutation, query, type MutationCtx, type QueryCtx, @@ -383,6 +385,59 @@ export const createR2UploadIntent = internalMutation({ }, }); +/// Gives image `targetAssetPublicId` the picture the strategy already shows +/// as `sourceAssetPublicId`, for a placed image copied to another page: a +/// placed image's id is also its image's id, so the copy needs a row of its +/// own. The row points at the same stored bytes, as a duplicated strategy's +/// images do (`copyActiveAssetToStrategy`), so nothing is uploaded twice. +/// +/// The copy's content may reach the server first and leave a placeholder; +/// the copied row replaces it. Copying again is harmless. "uploading" means +/// the source's upload has not finished, so there is nothing to copy yet; +/// "unavailable" means the strategy cannot show the source either. +export const copyAsset = mutation({ + args: { + ...cloudProtocolArgs, + strategyPublicId: v.string(), + sourceAssetPublicId: v.string(), + targetAssetPublicId: v.string(), + }, + returns: v.union( + v.literal("copied"), + v.literal("uploading"), + v.literal("unavailable"), + ), + handler: async (ctx, args) => { + assertSupportedCloudProtocol(args.clientProtocolVersion); + if (args.sourceAssetPublicId === args.targetAssetPublicId) { + throw invalidPayloadError("An image cannot be copied onto itself."); + } + const strategy = await getStrategyByPublicId(ctx, args.strategyPublicId); + const { user } = await assertStrategyRole(ctx, strategy, "editor"); + + const target = await getViewerAssetForStrategy( + ctx, + strategy._id, + args.targetAssetPublicId, + ); + if (target !== null && !isUploadPlaceholder(target)) { + return inferUploadStatus(target) === "active" ? "copied" : "uploading"; + } + const copied = await copyActiveAssetToStrategy(ctx, { + sourceStrategyId: strategy._id, + sourceAssetPublicId: args.sourceAssetPublicId, + targetStrategyId: strategy._id, + targetAssetPublicId: args.targetAssetPublicId, + userId: user._id, + now: Date.now(), + }); + if (copied === "copied" && target !== null) { + await ctx.db.delete(target._id); + } + return copied; + }, +}); + export const completeUpload = action({ args: { ...cloudProtocolArgs, diff --git a/lib/collab/generated/convex_models.dart b/lib/collab/generated/convex_models.dart index 8d05c6ab..9b121823 100644 --- a/lib/collab/generated/convex_models.dart +++ b/lib/collab/generated/convex_models.dart @@ -221,6 +221,25 @@ enum ImagesCompleteUploadArgsProvider { } } +enum ImagesCopyAssetResult { + copied('copied'), + unavailable('unavailable'), + uploading('uploading'); + + const ImagesCopyAssetResult(this.wireName); + final String wireName; + + static ImagesCopyAssetResult fromWireName(String wireName, String path) { + for (final value in values) { + if (value.wireName == wireName) return value; + } + throw ConvexDecodingException( + path, + 'unknown ImagesCopyAssetResult $wireName', + ); + } +} + enum ImagesGenerateUploadUrlResultProvider { r2('r2'); @@ -5410,6 +5429,27 @@ ImagesCompleteUploadResult decodeImagesCompleteUploadResult( 'images.js:completeUpload.returns', ); +ConvexObject encodeImagesCopyAssetArgs({ + required double clientProtocolVersion, + required String sourceAssetPublicId, + required String strategyPublicId, + required String targetAssetPublicId, +}) => ConvexObject({ + 'clientProtocolVersion': _encodeNumber( + clientProtocolVersion, + 'images.js:copyAsset.args.clientProtocolVersion', + ), + 'sourceAssetPublicId': ConvexString(sourceAssetPublicId), + 'strategyPublicId': ConvexString(strategyPublicId), + 'targetAssetPublicId': ConvexString(targetAssetPublicId), +}); + +ImagesCopyAssetResult decodeImagesCopyAssetResult(ConvexValue value) => + ImagesCopyAssetResult.fromWireName( + _decodeString(value, 'images.js:copyAsset.returns'), + 'images.js:copyAsset.returns', + ); + ConvexObject encodeImagesDeleteAssetRefArgs({ required String assetPublicId, required double clientProtocolVersion, diff --git a/lib/collab/generated/icarus_convex_api.dart b/lib/collab/generated/icarus_convex_api.dart index a4facdd0..12597b9f 100644 --- a/lib/collab/generated/icarus_convex_api.dart +++ b/lib/collab/generated/icarus_convex_api.dart @@ -408,6 +408,12 @@ abstract interface class ImagesModule { ConvexOptional uploadId = const ConvexOptional.absent(), ConvexOptional width = const ConvexOptional.absent(), }); + Future copyAsset({ + required double clientProtocolVersion, + required String sourceAssetPublicId, + required String strategyPublicId, + required String targetAssetPublicId, + }); Future deleteAssetRef({ required String assetPublicId, required double clientProtocolVersion, @@ -478,6 +484,25 @@ final class _ImagesModule implements ImagesModule { ); } + @override + Future copyAsset({ + required double clientProtocolVersion, + required String sourceAssetPublicId, + required String strategyPublicId, + required String targetAssetPublicId, + }) { + final args = encodeImagesCopyAssetArgs( + clientProtocolVersion: clientProtocolVersion, + sourceAssetPublicId: sourceAssetPublicId, + strategyPublicId: strategyPublicId, + targetAssetPublicId: targetAssetPublicId, + ); + return _invoke( + () => _transport.mutation('images:copyAsset', args), + decodeImagesCopyAssetResult, + ); + } + @override Future deleteAssetRef({ required String assetPublicId, From a692d7c2ff47fcae013eb24cf8b8e8f728501a0e Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:31:24 -0400 Subject: [PATCH 2/4] Replace a failed upload with a copy; reclaim copies nothing uses Astra's review: a copied image whose content never reaches the server kept its row, and so its bytes, for good. A day after each copy, the copy now goes through the reclaim check, which removes it only if nothing shows it. A failed upload under the copy's id no longer reads as one still on its way: the copy replaces it. Tests cover both, and a viewer collaborator being refused. Co-Authored-By: Claude Opus 5.5 --- convex/function_spec.json | 26 ++++++++++++++ convex/imageCopy.test.ts | 73 +++++++++++++++++++++++++++++++++++++++ convex/images.ts | 40 ++++++++++++++++++--- 3 files changed, 135 insertions(+), 4 deletions(-) diff --git a/convex/function_spec.json b/convex/function_spec.json index b56f025c..2ce6921e 100644 --- a/convex/function_spec.json +++ b/convex/function_spec.json @@ -41018,6 +41018,32 @@ "kind": "internal" } }, + { + "args": { + "type": "object", + "value": { + "assetPublicId": { + "fieldType": { + "type": "string" + }, + "optional": false + }, + "strategyId": { + "fieldType": { + "tableName": "strategies", + "type": "id" + }, + "optional": false + } + } + }, + "functionType": "Mutation", + "identifier": "images.js:reclaimCopiedAssetIfUnused", + "returns": null, + "visibility": { + "kind": "internal" + } + }, { "args": { "type": "object", diff --git a/convex/imageCopy.test.ts b/convex/imageCopy.test.ts index a8ba8db2..10a85278 100644 --- a/convex/imageCopy.test.ts +++ b/convex/imageCopy.test.ts @@ -25,6 +25,8 @@ const purgeOldTombstones = makeFunctionReference<"mutation">( ); const listImages = makeFunctionReference<"query">("images:listForStrategy"); const copyAsset = makeFunctionReference<"mutation">("images:copyAsset"); +const createShare = makeFunctionReference<"mutation">("shares:create"); +const redeemShare = makeFunctionReference<"mutation">("shares:redeem"); type Harness = TestConvexForDataModel; type RootHarness = TestConvexForDataModelAndIdentity; @@ -204,6 +206,62 @@ describe("images:copyAsset", () => { expect(await rowsFor(t, copy)).toHaveLength(1); }); + test("a copy replaces a failed upload under its id", async () => { + const { t, owner } = await createHarness(); + await t.run(async (ctx) => { + const strategy = await ctx.db + .query("strategies") + .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) + .unique(); + const now = Date.now(); + await ctx.db.insert("imageAssets", { + publicId: copy, + provider: "r2", + strategyId: strategy!._id, + uploadAttemptPublicId: "failed-attempt", + objectKey: `strategies/${strategyPublicId}/failed.png`, + uploadStatus: "failed", + fileExtension: ".png", + mimeType: "image/png", + createdAt: now, + updatedAt: now, + }); + }); + await addImage(owner, copy, secondPage); + + expect(await copyImage(owner)).toBe("copied"); + expect((await images(owner))[copy]).toEqual(["active", url]); + }); + + test("a copy nothing shows is removed a day later; one in use stays", async () => { + vi.useFakeTimers(); + const fetchMock = vi.fn(async () => new Response(null, { status: 204 })); + vi.stubGlobal("fetch", fetchMock); + const { t, owner } = await createHarness(); + const unused = `placed-image~cp1~0b1c2d3e-4f50-4a6b-8c7d-9e0f1a2b3c4d`; + await copyImage(owner); + await addImage(owner, copy, secondPage); + await owner.mutation(copyAsset, { + ...protocol, + strategyPublicId, + sourceAssetPublicId: original, + targetAssetPublicId: unused, + }); + + vi.setSystemTime(Date.now() + 25 * 60 * 60 * 1000); + await t.finishAllScheduledFunctions(vi.runAllTimers); + + expect((await rowsFor(t, unused)).map((row) => row.uploadStatus)).toEqual( + [], + ); + // Its bytes are the original's, so they stay. + expect(fetchMock).not.toHaveBeenCalled(); + expect(await images(owner)).toEqual({ + [original]: ["active", url], + [copy]: ["active", url], + }); + }); + test("an image still uploading has nothing to copy yet", async () => { const { t, owner } = await createHarness("pending"); await addImage(owner, copy, secondPage); @@ -232,6 +290,21 @@ describe("images:copyAsset", () => { const { owner, other } = await createHarness(); await expect(copyImage(other)).rejects.toThrow(); + // A collaborator who may only view cannot copy; an editor can. + for (const role of ["viewer", "editor"] as const) { + await owner.mutation(createShare, { + ...protocol, + targetType: "strategy", + targetPublicId: strategyPublicId, + token: `as-${role}`, + role, + }); + } + await other.mutation(redeemShare, { ...protocol, token: "as-viewer" }); + await expect(copyImage(other)).rejects.toThrow(); + await other.mutation(redeemShare, { ...protocol, token: "as-editor" }); + expect(await copyImage(other)).toBe("copied"); + await expect( owner.mutation(copyAsset, { ...protocol, diff --git a/convex/images.ts b/convex/images.ts index 48aaa523..b8713d4d 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -88,6 +88,9 @@ export const markDeletedStrategyImageAssetsRef = makeFunctionReference<"mutation">("images:markDeletedStrategyImageAssets"); export const processAssetReclaimCandidatesRef = makeFunctionReference<"mutation">("images:processAssetReclaimCandidates"); +const reclaimCopiedAssetIfUnusedRef = makeFunctionReference<"mutation">( + "images:reclaimCopiedAssetIfUnused", +); const markDeletedPageImageAssetsRef = makeFunctionReference<"mutation">( "images:markDeletedPageImageAssets", ); @@ -392,9 +395,14 @@ export const createR2UploadIntent = internalMutation({ /// images do (`copyActiveAssetToStrategy`), so nothing is uploaded twice. /// /// The copy's content may reach the server first and leave a placeholder; -/// the copied row replaces it. Copying again is harmless. "uploading" means -/// the source's upload has not finished, so there is nothing to copy yet; +/// the copied row replaces it, as it replaces a failed upload's. Copying +/// again is harmless. "uploading" means an upload is still on its way (the +/// source's, so there is nothing to copy yet, or one under the target's id); /// "unavailable" means the strategy cannot show the source either. +/// +/// Content that never arrives (its page deleted first, say) would leave the +/// row unused for good, so a day later the copy goes through the reclaim +/// check, which removes it only if nothing shows it. export const copyAsset = mutation({ args: { ...cloudProtocolArgs, @@ -421,7 +429,12 @@ export const copyAsset = mutation({ args.targetAssetPublicId, ); if (target !== null && !isUploadPlaceholder(target)) { - return inferUploadStatus(target) === "active" ? "copied" : "uploading"; + const status = inferUploadStatus(target); + if (status === "active") return "copied"; + if (status === "pending") return "uploading"; + // A failed upload under the target's id: the copied row, being newer, + // is the one readers see, and the stale-upload sweep removes the + // failed one. } const copied = await copyActiveAssetToStrategy(ctx, { sourceStrategyId: strategy._id, @@ -431,13 +444,32 @@ export const copyAsset = mutation({ userId: user._id, now: Date.now(), }); - if (copied === "copied" && target !== null) { + if (copied !== "copied") return copied; + if (target !== null && isUploadPlaceholder(target)) { await ctx.db.delete(target._id); } + await ctx.scheduler.runAfter( + staleUploadAgeMs, + reclaimCopiedAssetIfUnusedRef, + { strategyId: strategy._id, assetPublicId: args.targetAssetPublicId }, + ); return copied; }, }); +/// Puts a copied image through the reclaim check a day after the copy (see +/// [copyAsset]): it is removed only if no content, live or restorable, +/// shows it. +export const reclaimCopiedAssetIfUnused = internalMutation({ + args: { + strategyId: v.id("strategies"), + assetPublicId: v.string(), + }, + handler: async (ctx, args) => { + await queueAssetReclaim(ctx, args.strategyId, [args.assetPublicId]); + }, +}); + export const completeUpload = action({ args: { ...cloudProtocolArgs, From f49e0c3297ae1173de609c6d78d82ef9ab267dac Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:37:45 -0400 Subject: [PATCH 3/4] Keep a copied image's row until something that showed it goes Astra's review of the app side: a copy can wait in a device's outbox for days and still lands needing its picture, so the day-later reclaim of unused copies broke copies delivered late. Copied rows now go as any image does, once content that showed it is purged or with the strategy; a copy whose content never arrives keeps its row until then. Co-Authored-By: Claude Opus 5.5 --- convex/function_spec.json | 26 -------------------------- convex/imageCopy.test.ts | 23 +++++------------------ convex/images.ts | 28 ++++------------------------ 3 files changed, 9 insertions(+), 68 deletions(-) diff --git a/convex/function_spec.json b/convex/function_spec.json index 2ce6921e..b56f025c 100644 --- a/convex/function_spec.json +++ b/convex/function_spec.json @@ -41018,32 +41018,6 @@ "kind": "internal" } }, - { - "args": { - "type": "object", - "value": { - "assetPublicId": { - "fieldType": { - "type": "string" - }, - "optional": false - }, - "strategyId": { - "fieldType": { - "tableName": "strategies", - "type": "id" - }, - "optional": false - } - } - }, - "functionType": "Mutation", - "identifier": "images.js:reclaimCopiedAssetIfUnused", - "returns": null, - "visibility": { - "kind": "internal" - } - }, { "args": { "type": "object", diff --git a/convex/imageCopy.test.ts b/convex/imageCopy.test.ts index 10a85278..7ce8569c 100644 --- a/convex/imageCopy.test.ts +++ b/convex/imageCopy.test.ts @@ -233,33 +233,20 @@ describe("images:copyAsset", () => { expect((await images(owner))[copy]).toEqual(["active", url]); }); - test("a copy nothing shows is removed a day later; one in use stays", async () => { + test("a copy whose content arrives days later still has its picture", async () => { vi.useFakeTimers(); const fetchMock = vi.fn(async () => new Response(null, { status: 204 })); vi.stubGlobal("fetch", fetchMock); const { t, owner } = await createHarness(); - const unused = `placed-image~cp1~0b1c2d3e-4f50-4a6b-8c7d-9e0f1a2b3c4d`; await copyImage(owner); - await addImage(owner, copy, secondPage); - await owner.mutation(copyAsset, { - ...protocol, - strategyPublicId, - sourceAssetPublicId: original, - targetAssetPublicId: unused, - }); - vi.setSystemTime(Date.now() + 25 * 60 * 60 * 1000); + // The copy waited in a device's outbox, offline, for three days. + vi.setSystemTime(Date.now() + 3 * 24 * 60 * 60 * 1000); await t.finishAllScheduledFunctions(vi.runAllTimers); + await addImage(owner, copy, secondPage); - expect((await rowsFor(t, unused)).map((row) => row.uploadStatus)).toEqual( - [], - ); - // Its bytes are the original's, so they stay. + expect((await images(owner))[copy]).toEqual(["active", url]); expect(fetchMock).not.toHaveBeenCalled(); - expect(await images(owner)).toEqual({ - [original]: ["active", url], - [copy]: ["active", url], - }); }); test("an image still uploading has nothing to copy yet", async () => { diff --git a/convex/images.ts b/convex/images.ts index b8713d4d..986144b3 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -88,9 +88,6 @@ export const markDeletedStrategyImageAssetsRef = makeFunctionReference<"mutation">("images:markDeletedStrategyImageAssets"); export const processAssetReclaimCandidatesRef = makeFunctionReference<"mutation">("images:processAssetReclaimCandidates"); -const reclaimCopiedAssetIfUnusedRef = makeFunctionReference<"mutation">( - "images:reclaimCopiedAssetIfUnused", -); const markDeletedPageImageAssetsRef = makeFunctionReference<"mutation">( "images:markDeletedPageImageAssets", ); @@ -400,9 +397,10 @@ export const createR2UploadIntent = internalMutation({ /// source's, so there is nothing to copy yet, or one under the target's id); /// "unavailable" means the strategy cannot show the source either. /// -/// Content that never arrives (its page deleted first, say) would leave the -/// row unused for good, so a day later the copy goes through the reclaim -/// check, which removes it only if nothing shows it. +/// The row is not swept on a timer: the copy's content may wait in a +/// device's outbox for days and still needs it. It goes as any image does, +/// once content that showed it is purged, or with the strategy; a copy whose +/// content never arrives keeps its row until then. export const copyAsset = mutation({ args: { ...cloudProtocolArgs, @@ -448,28 +446,10 @@ export const copyAsset = mutation({ if (target !== null && isUploadPlaceholder(target)) { await ctx.db.delete(target._id); } - await ctx.scheduler.runAfter( - staleUploadAgeMs, - reclaimCopiedAssetIfUnusedRef, - { strategyId: strategy._id, assetPublicId: args.targetAssetPublicId }, - ); return copied; }, }); -/// Puts a copied image through the reclaim check a day after the copy (see -/// [copyAsset]): it is removed only if no content, live or restorable, -/// shows it. -export const reclaimCopiedAssetIfUnused = internalMutation({ - args: { - strategyId: v.id("strategies"), - assetPublicId: v.string(), - }, - handler: async (ctx, args) => { - await queueAssetReclaim(ctx, args.strategyId, [args.assetPublicId]); - }, -}); - export const completeUpload = action({ args: { ...cloudProtocolArgs, From f3922cb5c8746368cf3e089d677a2edef2570fbf Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Sat, 10 Oct 2026 12:42:28 -0400 Subject: [PATCH 4/4] Refuse a copy onto an id that already shows another image CodeRabbit's review: an active row under the target's id counted as the copy whatever it showed. It now counts only if it has the source's stored bytes; otherwise the copy is refused. Co-Authored-By: Claude Opus 5.5 --- convex/imageCopy.test.ts | 29 +++++++++++++++++++++++++++++ convex/images.ts | 18 +++++++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/convex/imageCopy.test.ts b/convex/imageCopy.test.ts index 7ce8569c..18bbbd8a 100644 --- a/convex/imageCopy.test.ts +++ b/convex/imageCopy.test.ts @@ -206,6 +206,35 @@ describe("images:copyAsset", () => { expect(await rowsFor(t, copy)).toHaveLength(1); }); + test("an id that already shows another image is refused", async () => { + const { t, owner } = await createHarness(); + await t.run(async (ctx) => { + const strategy = await ctx.db + .query("strategies") + .withIndex("by_publicId", (q) => q.eq("publicId", strategyPublicId)) + .unique(); + const now = Date.now(); + await ctx.db.insert("imageAssets", { + publicId: copy, + provider: "r2", + strategyId: strategy!._id, + uploadAttemptPublicId: "other-attempt", + objectKey: `strategies/${strategyPublicId}/other.png`, + uploadStatus: "active", + fileExtension: ".png", + mimeType: "image/png", + uploadedAt: now, + createdAt: now, + updatedAt: now, + }); + }); + + await expect(copyImage(owner)).rejects.toThrow(); + expect((await rowsFor(t, copy)).map((row) => row.objectKey)).toEqual([ + `strategies/${strategyPublicId}/other.png`, + ]); + }); + test("a copy replaces a failed upload under its id", async () => { const { t, owner } = await createHarness(); await t.run(async (ctx) => { diff --git a/convex/images.ts b/convex/images.ts index 986144b3..1438a1ae 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -428,7 +428,23 @@ export const copyAsset = mutation({ ); if (target !== null && !isUploadPlaceholder(target)) { const status = inferUploadStatus(target); - if (status === "active") return "copied"; + if (status === "active") { + // Copied before, or another image entirely: only a row with the + // source's bytes is this copy. + const source = await getActiveAssetForStrategy( + ctx, + strategy._id, + args.sourceAssetPublicId, + ); + if ( + source !== null && + (source.objectKey !== target.objectKey || + source.storageId !== target.storageId) + ) { + throw conflictError("That image id already shows another image."); + } + return "copied"; + } if (status === "pending") return "uploading"; // A failed upload under the target's id: the copied row, being newer, // is the one readers see, and the stale-upload sweep removes the