feat(worker): delay S3 object deletion instead of removing immediately - #197
Open
bbornino wants to merge 7 commits into
Open
feat(worker): delay S3 object deletion instead of removing immediately#197bbornino wants to merge 7 commits into
bbornino wants to merge 7 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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a delayed BullMQ task for S3 object deletion, registers its worker processor, and routes gist cleanup through timestamp-checked deletion scheduling. ChangesDeferred S3 cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SyncTask
participant scheduleS3ObjectDeletion
participant S3
participant BullMQ
participant deleteS3ObjectProcessor
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
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
fennifith
reviewed
Jul 25, 2026
| const { bucket, key, lastModified } = job.data; | ||
|
|
||
| if (lastModified !== undefined) { | ||
| const stillUnmodified = await s3.unmodifiedSince( |
Member
There was a problem hiding this comment.
Can we add IfMatchLastModifiedTime to the DeleteObjectCommand input instead of checking this in a separate call?
| bucket: string, | ||
| key: string, | ||
| ): Promise<void> { | ||
| const lastModified = await s3.getLastModified(bucket, key); |
Member
There was a problem hiding this comment.
Could this accept lastModified: Date as a parameter instead of fetching it itself? (this also avoids adding s3 as a dependency of the bullmq package)
Per James's review on playfulprogramming#197, the low-level job-creation function (renamed enqueueS3ObjectDeletion) now takes lastModified as a required parameter instead of calling s3.getLastModified itself - packages/bullmq no longer depends on packages/s3 at all. The undefined-handling logic (warn and skip when LastModified can't be read) moves to a new scheduleS3ObjectDeletion wrapper in apps/worker/src/utils, since that's the only place any of this was ever called from - keeping the check in one place instead of duplicating it across sync-post's three call sites and getEmbedDataFromGist's one.
2 tasks
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/worker/src/tasks/sync-post/processor.ts (1)
241-249: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftScope attachment ownership before scheduling deletion.
This lookup is filtered only by
posts.slug, so a sync on one branch can treat attachment keys belonging only to another branch as removed. SincepostAttachmentsallows oneattachmentKeyto be referenced by multiple posts, the deletion path must also verify that no reference remains before deleting; the downstream processor currently checks onlyLastModified.At minimum, scope this baseline to the synchronized branch, then recheck references before enqueueing or processing deletion jobs.
Minimum scope fix
- .where(eq(posts.slug, post)); + .where(and(eq(posts.slug, post), eq(posts.branch, ref)));🤖 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 `@apps/worker/src/tasks/sync-post/processor.ts` around lines 241 - 249, Update the existingAttachmentRecords lookup in the sync-post processor to scope the baseline query to the synchronized branch as well as posts.slug, using the branch identity already available in the sync context. Before enqueueing or processing attachment deletion jobs, recheck postAttachments references and only delete keys with no remaining references, rather than relying solely on LastModified.
🧹 Nitpick comments (1)
apps/worker/src/tasks/sync-post/processor.test.ts (1)
806-932: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep a regression assertion for delayed deletion scheduling.
This test still covers deleted and replaced attachments, but it no longer asserts that
scheduleS3ObjectDeletionis called forold-file-sha.txtandold-changed-sha.txt. A regression to immediate deletion—or to no deletion job at all—would pass.Suggested assertions
+ expect(scheduleS3ObjectDeletion).toHaveBeenCalledWith( + "example-bucket", + "posts/diffing-post/attachments/old-file-sha.txt", + ); + expect(scheduleS3ObjectDeletion).toHaveBeenCalledWith( + "example-bucket", + "posts/diffing-post/attachments/old-changed-sha.txt", + );🤖 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 `@apps/worker/src/tasks/sync-post/processor.test.ts` around lines 806 - 932, Extend the test covering attachment diffing in the processor flow to assert that scheduleS3ObjectDeletion is called for both old-file-sha.txt and old-changed-sha.txt. Keep the existing upload and database assertions unchanged, and verify deletion is scheduled rather than performed immediately.
🤖 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.
Outside diff comments:
In `@apps/worker/src/tasks/sync-post/processor.ts`:
- Around line 241-249: Update the existingAttachmentRecords lookup in the
sync-post processor to scope the baseline query to the synchronized branch as
well as posts.slug, using the branch identity already available in the sync
context. Before enqueueing or processing attachment deletion jobs, recheck
postAttachments references and only delete keys with no remaining references,
rather than relying solely on LastModified.
---
Nitpick comments:
In `@apps/worker/src/tasks/sync-post/processor.test.ts`:
- Around line 806-932: Extend the test covering attachment diffing in the
processor flow to assert that scheduleS3ObjectDeletion is called for both
old-file-sha.txt and old-changed-sha.txt. Keep the existing upload and database
assertions unchanged, and verify deletion is scheduled rather than performed
immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 514e7156-0cc1-448d-9ddb-cbe5965379e3
📒 Files selected for processing (3)
apps/worker/src/tasks/sync-post/processor.test.tsapps/worker/src/tasks/sync-post/processor.tsapps/worker/test-utils/setup.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/worker/test-utils/setup.ts
…heduleS3ObjectDeletion import Matches the existing slug+branch pattern used elsewhere in this file. The removal-scheduling call sites this import backed were dropped by playfulprogramming#190's schema restructuring; per James, that's intentional going forward since attachments are now shared/reference-counted across posts and branches, and orphan cleanup belongs to playfulprogramming#191/playfulprogramming#195's sweep instead of inline detection here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Fix: undefined lastModified could still schedule an unsafe deletion
CodeRabbit found one more issue in the previous fix: if
getLastModifiedreturnedundefined(object already gone, or a transient failure), the code still scheduled a deletion with no generation marker — and since the processor's staleness check is skipped whenlastModifiedis absent, that job would unconditionally delete whatever's at the key 24 hours later, including a brand-new legitimate upload. Fixed by returning early (with aconsole.warn) instead of scheduling an unsafe job, and tightenedlastModifiedfrom optional to required at the type level to match.Job contract split, per review
Per review feedback,
scheduleS3ObjectDeletionno longer fetcheslastModifieditself — it's split intoenqueueS3ObjectDeletion(bucket, key, lastModified)(the low-level BullMQ primitive, now living entirely inpackages/bullmqwith no dependency onpackages/s3) and a newscheduleS3ObjectDeletion(bucket, key)wrapper inapps/worker/src/utils/, which fetcheslastModifiedand calls the primitive. All four call sites just changed their import path.Also investigated and declined using
IfMatchLastModifiedTimeonDeleteObjectCommandas a suggested atomic alternative to the current two-step check-then-remove: it's restricted to directory buckets per the AWS SDK's own type docs, and live testing against Minio confirmed it's silently ignored rather than enforced there — worse than not having it. Left as an accepted residual limitation, same category as the disclosedIfUnmodifiedSincegranularity gap.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.Update: Confirmed directly with James — sync-post's inline attachment-removal detection (the previous diffing loop and 404-path S3 cleanup) was intentionally dropped during #190's schema restructuring, not lost by accident. It isn't coming back here. The new schema makes per-post inline orphan detection unsafe on its own: posts now has unique(slug, locale, branch), so the same content-addressed attachmentKey can be referenced by postAttachments rows across multiple branches of the same post. Removal detection needs to account for all live references, not just one post's current sync — that's exactly what #191/#195's periodic orphan-sweep is for, and it's already tracked there separately.
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
Summary