From 2b1dffb3954f37b1db306a7155f7a8a73d0e04b4 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Mon, 28 Sep 2026 12:16:33 -0400 Subject: [PATCH 1/2] Wait for an upload's URL to expire before deleting its bytes An image deleted while its presigned PUT was still in flight lost its row before the bytes landed, so the object stayed in R2 with nothing left to reclaim it. Intents now record when their upload URL expires, and the physical-deletion claim skips an asset until that time plus a 15 minute grace for a PUT that started just before expiry. The hourly sweep picks it up afterwards. Co-Authored-By: Claude Opus 5.5 (1M context) --- convex/imageAssetLifecycle.test.ts | 48 ++++++++++++++++++++++++++++++ convex/images.ts | 22 +++++++++++++- convex/lib/r2.ts | 14 +++++---- convex/schema.ts | 3 ++ 4 files changed, 81 insertions(+), 6 deletions(-) diff --git a/convex/imageAssetLifecycle.test.ts b/convex/imageAssetLifecycle.test.ts index e381ff07..b40ac896 100644 --- a/convex/imageAssetLifecycle.test.ts +++ b/convex/imageAssetLifecycle.test.ts @@ -29,6 +29,9 @@ const sweepDeletedImageAssets = makeFunctionReference<"action">( "images:sweepDeletedImageAssets", ); const completeUpload = makeFunctionReference<"action">("images:completeUpload"); +const generateUploadUrl = makeFunctionReference<"action">( + "images:generateUploadUrl", +); const getAssetUrl = makeFunctionReference<"query">("images:getAssetUrl"); type Harness = TestConvexForDataModel; @@ -551,6 +554,51 @@ describe("image asset lifecycle", () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); + test("an upload deleted while its URL still works keeps its row until the URL dies", async () => { + vi.useFakeTimers(); + const fetchMock = mockR2Deletes(); + const { t, owner } = await createHarness(); + await seedStrategy(owner); + const upload = (await owner.action(generateUploadUrl, { + clientProtocolVersion: CURRENT_CLOUD_PROTOCOL_VERSION, + strategyPublicId, + assetPublicId: "in-flight", + mimeType: "image/png", + fileExtension: "png", + })) as { objectKey: string; expiresAt: number }; + expect(await allAssets(t)).toMatchObject([ + { uploadStatus: "pending", uploadUrlExpiresAt: upload.expiresAt }, + ]); + + // The strategy goes while the client's PUT may still be sending bytes. + await owner.mutation(deleteStrategy, { + clientProtocolVersion: CURRENT_CLOUD_PROTOCOL_VERSION, + strategyPublicId, + expectedRevision: 0, + }); + await t.finishAllScheduledFunctions(vi.runAllTimers); + expect(fetchMock).not.toHaveBeenCalled(); + expect(await allAssets(t)).toMatchObject([ + { uploadStatus: "deleted", objectKey: upload.objectKey }, + ]); + + // Still inside the grace after expiry: a PUT that started in time may + // not have finished. + vi.setSystemTime(upload.expiresAt + 14 * 60 * 1000); + await expect( + t.action(sweepDeletedImageAssets, {}), + ).resolves.toMatchObject({ deleted: 0, failed: 0 }); + + vi.setSystemTime(upload.expiresAt + 15 * 60 * 1000); + await expect( + t.action(sweepDeletedImageAssets, {}), + ).resolves.toMatchObject({ deleted: 1, failed: 0 }); + expect(await allAssets(t)).toEqual([]); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(String(fetchMock.mock.calls[0]?.[0])).toContain(upload.objectKey); + await t.finishAllScheduledFunctions(vi.runAllTimers); + }); + test("legacy reads survive while completion inserts an exact-owned replacement", async () => { const { t, owner } = await createHarness(); await seedStrategy(owner); diff --git a/convex/images.ts b/convex/images.ts index 84e66d19..7f391f4d 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -35,6 +35,7 @@ import { deleteR2Object, expectedMimeTypeForExtension, getR2Config, + getUploadUrlExpiresSeconds, headR2Object, normalizeImageExtension, presignR2PutUrl, @@ -74,6 +75,9 @@ const physicalDeletionBatch = 25; // size, so a batch stays far below Convex's per-transaction limits. const reclaimCandidateBatch = 25; const staleDeletionClaimAgeMs = 15 * 60 * 1000; +// R2 checks a presigned URL when the PUT starts, so a PUT that started just +// before expiry can still be sending bytes after it. This covers that PUT. +const uploadInFlightGraceMs = 15 * 60 * 1000; const deletionRetryDelayMs = 60 * 1000; export const markDeletedStrategyImageAssetsRef = @@ -269,6 +273,7 @@ export const generateUploadUrl = action({ uploadId: Id<"imageAssets">; uploadAttemptPublicId: string; objectKey: string; + issuedAt: number; } = await ctx.runMutation(internal.images.createR2UploadIntent, { strategyPublicId: args.strategyPublicId, @@ -281,10 +286,13 @@ export const generateUploadUrl = action({ width: args.width, height: args.height, }); + // Signed at the intent's time, so the URL expires exactly when the + // intent's uploadUrlExpiresAt says it does. const signed = await presignR2PutUrl({ config, objectKey: intent.objectKey, mimeType: validated.mimeType, + now: new Date(intent.issuedAt), }); return { @@ -337,6 +345,7 @@ export const createR2UploadIntent = internalMutation({ width: args.width, height: args.height, byteSize: args.byteSize, + uploadUrlExpiresAt: now + getUploadUrlExpiresSeconds() * 1000, updatedAt: now, }; // Content that shows this image may have landed first and left a @@ -367,6 +376,7 @@ export const createR2UploadIntent = internalMutation({ uploadId, uploadAttemptPublicId: args.uploadAttemptPublicId, objectKey: args.objectKey, + issuedAt: now, }; }, }); @@ -1052,6 +1062,10 @@ export const claimDeletedImageAssets = internalMutation({ 1, Math.min(args.limit ?? physicalDeletionBatch, physicalDeletionBatch), ); + const now = Date.now(); + // An upload deleted while its PUT may still be in flight waits for the + // URL to die. Deleting first would let the bytes land afterwards with no + // row left to reclaim them; the hourly sweep picks the asset up later. const assets = await ctx.db .query("imageAssets") .withIndex("by_uploadStatus_and_updatedAt", (q) => @@ -1061,10 +1075,16 @@ export const claimDeletedImageAssets = internalMutation({ q.and( q.neq(q.field("strategyId"), undefined), q.eq(q.field("cleanupClaimedAt"), undefined), + q.or( + q.eq(q.field("uploadUrlExpiresAt"), undefined), + q.lte( + q.field("uploadUrlExpiresAt"), + now - uploadInFlightGraceMs, + ), + ), ), ) .take(limit); - const now = Date.now(); for (const asset of assets) { await ctx.db.patch(asset._id, { cleanupClaimedAt: now }); } diff --git a/convex/lib/r2.ts b/convex/lib/r2.ts index 9f2b90df..8867a3bd 100644 --- a/convex/lib/r2.ts +++ b/convex/lib/r2.ts @@ -68,6 +68,14 @@ function optionalPositiveIntEnv( return parsed; } +export function getUploadUrlExpiresSeconds(): number { + return optionalPositiveIntEnv( + "R2_UPLOAD_URL_EXPIRES_SECONDS", + defaultUploadUrlExpiresSeconds, + { min: 1, max: 604800 }, + ); +} + export function getR2Config(): R2Config { const accountId = requiredEnv("R2_ACCOUNT_ID"); const bucket = requiredEnv("R2_BUCKET"); @@ -82,11 +90,7 @@ export function getR2Config(): R2Config { accessKeyId: requiredEnv("R2_ACCESS_KEY_ID"), secretAccessKey: requiredEnv("R2_SECRET_ACCESS_KEY"), publicBaseUrl: getR2PublicBaseUrl(), - uploadUrlExpiresSeconds: optionalPositiveIntEnv( - "R2_UPLOAD_URL_EXPIRES_SECONDS", - defaultUploadUrlExpiresSeconds, - { min: 1, max: 604800 }, - ), + uploadUrlExpiresSeconds: getUploadUrlExpiresSeconds(), maxImageBytes: optionalPositiveIntEnv( "R2_MAX_IMAGE_BYTES", defaultMaxImageBytes, diff --git a/convex/schema.ts b/convex/schema.ts index 32cfc4c2..fb5ca250 100644 --- a/convex/schema.ts +++ b/convex/schema.ts @@ -230,6 +230,9 @@ export default defineSchema({ byteSize: v.optional(v.number()), etag: v.optional(v.string()), uploadedAt: v.optional(v.number()), + // When the presigned PUT URL for this upload attempt stops working. Until + // then the bytes can still land, so they are not deleted before it. + uploadUrlExpiresAt: v.optional(v.number()), deletedAt: v.optional(v.number()), cleanupClaimedAt: v.optional(v.number()), createdAt: v.optional(v.number()), From d4759c3101ad8df929c88c883f97ca65b08a2100 Mon Sep 17 00:00:00 2001 From: Dara Adedeji Date: Mon, 28 Sep 2026 12:32:15 -0400 Subject: [PATCH 2/2] Give an in-flight upload a day, not 15 minutes, to finish A PUT that R2 accepted just before its URL expired can keep sending for as long as the connection takes: a 15 MB image at 12 KB/s needs about 20 minutes. Deleting its object after a fixed 15 minutes could still orphan the bytes. A day covers any real upload, and the wait costs little since a deleted element's image already stays for its 30-day tombstone. Co-Authored-By: Claude Opus 5.5 (1M context) --- convex/imageAssetLifecycle.test.ts | 8 ++++---- convex/images.ts | 6 ++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/convex/imageAssetLifecycle.test.ts b/convex/imageAssetLifecycle.test.ts index b40ac896..e6281f01 100644 --- a/convex/imageAssetLifecycle.test.ts +++ b/convex/imageAssetLifecycle.test.ts @@ -582,14 +582,14 @@ describe("image asset lifecycle", () => { { uploadStatus: "deleted", objectKey: upload.objectKey }, ]); - // Still inside the grace after expiry: a PUT that started in time may - // not have finished. - vi.setSystemTime(upload.expiresAt + 14 * 60 * 1000); + // Well after expiry, a PUT that started in time may still be sending + // over a slow connection. + vi.setSystemTime(upload.expiresAt + 23 * 60 * 60 * 1000); await expect( t.action(sweepDeletedImageAssets, {}), ).resolves.toMatchObject({ deleted: 0, failed: 0 }); - vi.setSystemTime(upload.expiresAt + 15 * 60 * 1000); + vi.setSystemTime(upload.expiresAt + 24 * 60 * 60 * 1000); await expect( t.action(sweepDeletedImageAssets, {}), ).resolves.toMatchObject({ deleted: 1, failed: 0 }); diff --git a/convex/images.ts b/convex/images.ts index 7f391f4d..6257c52b 100644 --- a/convex/images.ts +++ b/convex/images.ts @@ -76,8 +76,10 @@ const physicalDeletionBatch = 25; const reclaimCandidateBatch = 25; const staleDeletionClaimAgeMs = 15 * 60 * 1000; // R2 checks a presigned URL when the PUT starts, so a PUT that started just -// before expiry can still be sending bytes after it. This covers that PUT. -const uploadInFlightGraceMs = 15 * 60 * 1000; +// before expiry can still be sending bytes after it, for as long as a slow +// connection takes. A day covers any real upload, and waiting costs little: +// a deleted element's image already stays for its 30-day tombstone. +const uploadInFlightGraceMs = 24 * 60 * 60 * 1000; const deletionRetryDelayMs = 60 * 1000; export const markDeletedStrategyImageAssetsRef =