feat(worker): delay S3 object deletion instead of removing immediately#197
feat(worker): delay S3 object deletion instead of removing immediately#197bbornino wants to merge 4 commits into
Conversation
Immediate s3.remove() calls risked breaking in-flight or cached requests for a key the frontend was still pointing at. Deletion is now scheduled as a delayed BullMQ job (24h grace period) via a shared scheduleS3ObjectDeletion() helper, reusing the existing delete-object primitive rather than depending on S3-native lifecycle rules, since Tigris (prod) only supports prefix-based lifecycle filtering and can't act on object tags.
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a delayed BullMQ task for S3 object deletion, registers its worker processor, and updates post and gist synchronization cleanup paths to schedule deletion instead of removing objects immediately. ChangesDeferred S3 cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SyncTask
participant scheduleS3ObjectDeletion
participant BullMQ
participant deleteS3ObjectProcessor
participant S3
SyncTask->>scheduleS3ObjectDeletion: bucket and object key
scheduleS3ObjectDeletion->>S3: getLastModified(bucket, key)
scheduleS3ObjectDeletion->>BullMQ: enqueue delayed DELETE_S3_OBJECT job
BullMQ->>deleteS3ObjectProcessor: deliver job after grace period
deleteS3ObjectProcessor->>S3: unmodifiedSince(bucket, key, timestamp)
deleteS3ObjectProcessor->>S3: remove(bucket, key) when unchanged
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bullmq/src/tasks/delete-s3-object.ts`:
- Around line 4-25: Update scheduleS3ObjectDeletion and DeleteS3ObjectInput to
carry a per-upload/version token, and stop using the stable bucket/key-based job
ID so repeated requests are not deduplicated. Include the token in the scheduled
payload and job identity, then update the worker’s delete-s3-object processor to
compare the token with the current object version and skip stale deletions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1a34c79-6ab1-4351-be66-864919506486
📒 Files selected for processing (12)
apps/worker/src/index.tsapps/worker/src/tasks/delete-s3-object/processor.test.tsapps/worker/src/tasks/delete-s3-object/processor.tsapps/worker/src/tasks/sync-post/processor.test.tsapps/worker/src/tasks/sync-post/processor.tsapps/worker/src/tasks/url-metadata/getEmbedDataFromGist.test.tsapps/worker/src/tasks/url-metadata/getEmbedDataFromGist.tsapps/worker/test-utils/setup.tspackages/bullmq/src/queues.tspackages/bullmq/src/tasks/delete-s3-object.tspackages/bullmq/src/tasks/index.tspackages/bullmq/src/tasks/types.ts
…ressed keys CodeRabbit found that a scheduled deletion's stable job ID lets a content-addressed attachment key get deleted even after it's been legitimately re-referenced (e.g. identical content reappearing with the same sha). ETag-based staleness checking can't catch this, since identical content always produces an identical ETag whether it's the original upload or a fresh one. Capturing LastModified at scheduling time and verifying it's unchanged before deleting does catch it, since S3 bumps LastModified on every write regardless of content match - mirroring the same staleness-check pattern playfulprogramming#191/playfulprogramming#195 already established for the orphan-sweep task.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/bullmq/src/tasks/delete-s3-object.ts (1)
27-32:⚠️ Potential issue | 🔴 CriticalAvoid key-based dedup for delayed S3 deletes.
Using a stable job ID based solely on the bucket and key will cause subsequent deletion requests for the same key within the grace period to be deduplicated by BullMQ. Since the first job retains its original
lastModifiedtimestamp, the worker will later compare it against the updated object, detect a mismatch, skip the deletion, and ultimately leak the newer object version.To resolve this, append a unique identifier (such as the object timestamp) to the job ID so repeated deletions of replaced objects are queued as independent jobs.
🐛 Proposed fix
await createJob( Tasks.DELETE_S3_OBJECT, - `delete-s3-object:${bucket}:${key}`, + `delete-s3-object:${bucket}:${key}:${lastModified?.getTime() ?? Date.now()}`, { bucket, key, lastModified: lastModified?.toISOString() }, { delay: DELETE_S3_OBJECT_GRACE_PERIOD_MS }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bullmq/src/tasks/delete-s3-object.ts` around lines 27 - 32, Update the createJob call in the delete-s3-object task to make the job ID unique per object version by appending a stable unique identifier such as lastModified to the bucket/key ID. Preserve the existing payload and grace-period delay while ensuring repeated deletion requests for replaced objects are queued independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/bullmq/src/tasks/delete-s3-object.ts`:
- Around line 27-32: Update the createJob call in the delete-s3-object task to
make the job ID unique per object version by appending a stable unique
identifier such as lastModified to the bucket/key ID. Preserve the existing
payload and grace-period delay while ensuring repeated deletion requests for
replaced objects are queued independently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67c99449-f355-4d08-b1ad-5f43ca0454ae
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/worker/src/tasks/delete-s3-object/processor.test.tsapps/worker/src/tasks/delete-s3-object/processor.tsapps/worker/test-utils/setup.tspackages/bullmq/package.jsonpackages/bullmq/src/tasks/delete-s3-object.tspackages/s3/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/worker/test-utils/setup.ts
- apps/worker/src/tasks/delete-s3-object/processor.ts
BullMQ's deduplication silently drops a second queue.add() call under the same job id while the first job is still pending, keeping no record of the dropped call's data. Since the job id was a stable bucket:key string, a key that got deleted, legitimately re-added, then deleted again within the 24h grace period would have its second deletion silently absorbed into the first (now-stale) job - which then skips deleting on the safety check, leaking the object in S3 permanently. Fold the object's LastModified (or a random id when it can't be read) into the job id so each generation of a key gets its own job, while calls for an unchanged object still dedupe as before.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bullmq/src/tasks/delete-s3-object.test.ts`:
- Around line 48-55: Update the test covering an undefined last-modified value
in scheduleS3ObjectDeletion to assert that createJob is not called after
invoking the deletion twice. Remove the job ID uniqueness assertion and verify
the early-return behavior for nonexistent objects.
In `@packages/bullmq/src/tasks/delete-s3-object.ts`:
- Around line 25-38: Return early in delete-s3-object.ts when getLastModified
returns no value, and remove the crypto.randomUUID fallback because
lastModifiedIso must be defined before createJob is called. In
delete-s3-object.test.ts, update the undefined-last-modified case to assert that
the mocked createJob is not called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c57fb29-a5ce-4510-8514-9340aef662c5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/bullmq/package.jsonpackages/bullmq/src/tasks/delete-s3-object.test.tspackages/bullmq/src/tasks/delete-s3-object.tspackages/bullmq/tsconfig.jsonpackages/bullmq/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bullmq/package.json
…nown Scheduling a deletion without a LastModified left the processor with no safety check to run at execution time, so it would unconditionally delete whatever was at that key 24h later - including a legitimate new upload made during the grace period. Bail out of scheduling instead, with a warning log so a transient metadata-read failure doesn't silently leak the object. lastModified on DeleteS3ObjectInput is now required, and the random-id job ID fallback is gone along with it - every scheduled job is guaranteed to carry a real generation marker.
1 participant
Closes #188.
Problem
Several worker tasks called
s3.remove()synchronously as soon as an object was no longer needed (removed attachments insync-post, deleted files ingetEmbedDataFromGist). This risked breaking any in-flight or CDN-cached request that was still pointing at the key at the moment it disappeared.What changed
Deletion is now scheduled as a delayed BullMQ job instead of happening immediately:
delete-s3-objecttask (packages/bullmq/src/tasks/delete-s3-object.ts,apps/worker/src/tasks/delete-s3-object/processor.ts) whose processor calls the existings3.remove(bucket, key), guarded by a staleness check (see below).createJob()gained an optionalopts.delayparam so a job can be scheduled in the future instead of run immediately.scheduleS3ObjectDeletion(bucket, key)helper (exported frompackages/bullmq/src/tasks/delete-s3-object.ts, alongside the grace-period constant) builds the job ID and delay consistently. Both call sites (sync-post/processor.ts's three removal points, andgetEmbedDataFromGist.ts) import and use this same function rather than each constructing the job independently.DELETE_S3_OBJECT_GRACE_PERIOD_MS), matching the "e.g. a day" example in Create S3 lifecycle configuration #188.Why an application-level delayed job instead of native S3 lifecycle configuration
The issue suggested
PutBucketLifecycleConfigurationwith object tagging: tag an object at delete-time, then have a lifecycle rule expire objects carrying that tag. That works on real AWS S3 and on MinIO (local dev), but **Tigris(prod) only supports lifecycle-rule filtering by key prefix — it does not support tag-based filteringing
PutObjectTaggingas a standalone API call (confirmed directly byTigris: "Filtering is by key prefix only. Not by object tags... We don't have that yet.").A tag-based lifecycle rule that appeared to work in local dev would silently do nothing in production. A prefix-based rule would work uniformly across all three backends, but the current key layout doesn't have a prefix that means "pending deletion" distinct from "live object" — and moving an object to a dedicated prefix would require copying it to a new key, which doesn't solve the actual problem (the frontend needs the original key to keep resolving during
the grace period, not a new one).
Given that gap, this PR keeps the deletion mechanism entirely in application code — a delayed BullMQ emove()` after the grace period — so behavior is identical across MinIO, AWS S3, and Tigris rather than depending on lifecycle-rule capabilities that differ between them.
Fix: stale-key race found by CodeRabbit (PR #197)
CodeRabbit flagged a real race in the first version of this PR: the deletion job's ID is stable (
delete-s3-object:${bucket}:${key}), so if a key gets legitimately re-referenced while its deletion is still pending — e.g. a content-addressed attachment key (posts/{post}/attachments/{sha}{extension}) reappearing with the identical sha after its removal was already scheduled — the original delayed job still fires and deletes the object that's now actively in use again.Why
matchesEtag(the existing S3 helper) can't catch this: the affected keys are content-addres identical bytes reappearing at the same key. S3's ETag for a single-part upload is the content'sMD5, so identical content always produces an identical ETag whether it's the original upload or a fresh one. An ETag-based staleness check would report "unchanged" in precisely the case where it's now unsafe to delete — there-reference is invisible to it by construction. (Folding the etag into the job ID doesn't help eitheason: it only affects which scheduling calls collapse into one job, not what the processor should doat execution time, and for content-addressed keys the etag adds no discriminating power over the key itself.)
Why a
LastModified-based check does: S3 updates an object'sLastModifiedon every write, evenical to what was there before — a rewrite is still a rewrite. So capturingLastModifiedat scheduling time and verifying it's unchanged immediately before deleting correctly detects "this key was rewritten since I scheduled its removal," regardless of whether the new content happens to be byte-identical. This mirrors the same class of grace-period staleness check #191/#195 already established for the orphan-sweep task, so it's consistent with an existing pattern rather than a new one.Concretely:
scheduleS3ObjectDeletion()now calls a news3.getLastModified()helper and includes the result in the job data; thedelete-s3-objectprocessor calls a news3.unmodifiedSince()helper (a conditionalHeadObjectCommandwithIfUnmodifiedSince, the timestamp analog ofmatchesEtag's existingIfMat removing the object, and skips the delete if the object was rewritten in the meantime. Both call sites (sync-post,getEmbedDataFromGist) needed no changes — the capture/check is entirely internal toscheduleS3ObjectDeletion` and the processor.Known, accepted residual limitation:
IfUnmodifiedSinceis HTTP-date granularity (1-second resolution). A rewrite landing within the same second as the captured timestamp wouldn't be caught. This narrows the race window from "upto 24 hours" to "up to ~1 second," which we're treating as an accepted trade-off rather than a full c rather than claiming this eliminates the race entirely.
Out of scope — follow-up needed
#191 / PR #195 (cleanup-attachments task) is not touched in this PR. That PR isn't merged to
mainyet, so itss3.remove()call for orphaned attachments doesn't exist in this branch's tree. Action item: once #195 lands, its immediates3.remove()call needs to be swapped forscheduleS3ObjectDeletion()the same way the three call sites in this PR were, for consistency with the rest of the codebase's deletion behavior.Open questions for James
pending-deletion=true) would be purely a non-functional audit trail for humans browsing thebucket — not something that drives any actual behavior. I left it out to avoid adding a call site andng convenience nobody's asked for yet. Worth a quick gut-check on whether that trade-off is right.
Testing
pnpm run build:all— passes across all 10 NX projects (now includes a newpackages/bullmq→packages/s3workspace dependency edge, added becausescheduleS3ObjectDeletionneedss3.getLastModified).pnpm run test:unit(lint, knip, publint, sherif, vitest) — passes; worker suite is now 54 tests,delete-s3-objectprocessor's staleness-check branches (nolastModifiedcaptured, unmodified sincescheduling, rewritten since scheduling) and one covering the previously-untestedgetEmbedDataFromGistdeletion path.pnpm run prettier— clean (aside from the pre-existing, intentionally-untracked.claude/settings.local.json).LastModifiedvias3.getLastModified, confirmeds3.unmodifiedSincereturnstrueagainst the untouched object, rewrote the same key, then confirmeds3.unmodifiedSincereturnsfalseagainst the rewritten object — validating the conditionalHeadObjectCommand/IfUnmodifiedSincebehavior against actual MinIO rather than just the AWS SDK's type definitions.Summary by CodeRabbit