feat(worker): add cleanup task for unreferenced S3 attachments#195
feat(worker): add cleanup task for unreferenced S3 attachments#195bbornino wants to merge 3 commits into
Conversation
Sweeps S3 objects under posts/*/attachments/* on a repeatable interval and removes any with no matching post_attachments row, catching attachments orphaned by interrupted sync-post jobs.
sync-post uploads an attachment to S3 before its post_attachments row commits, so a very recent orphan-looking object may just be mid-flight. S3's list now returns lastModified alongside each key, and the cleanup task skips anything younger than a one-hour grace period.
|
Warning Review limit reached
Next review available in: 40 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 repeatable BullMQ worker that lists S3 attachments, preserves referenced or recent objects, and removes unreferenced older objects. The change includes typed task registration, paginated S3 listing, startup scheduling, tests, and updated S3 mocks. ChangesAttachment cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Worker as Worker startup
participant Queue as BullMQ queue
participant Processor as Cleanup processor
participant S3
participant DB as postAttachments
Worker->>Queue: Schedule repeatable cleanup job
Queue->>Processor: Execute CLEANUP_ATTACHMENTS
Processor->>S3: List posts/ objects
Processor->>DB: Query referenced attachment keys
Processor->>S3: Remove unreferenced old objects
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (4)
packages/s3/src/utils.ts (1)
79-107: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider streaming or batching for very large attachment sets.
list()accumulates all matching S3 objects into a single in-memory array before returning. For workspaces with many posts/attachments this could consume significant memory. If scale is a concern, exposing an async iterable or accepting a callback would let the processor stream candidates without holding the full list. Not a blocker for the current scope.🤖 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/s3/src/utils.ts` around lines 79 - 107, Consider changing list() to expose matching S3 objects incrementally through an async iterable or callback instead of accumulating every object in the objects array. Preserve pagination via continuationToken and emit each valid object as soon as it is received, while updating callers to consume the streaming interface where appropriate.apps/worker/src/tasks/cleanup-attachments/processor.ts (2)
26-31: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffLoading all
post_attachmentsrows into memory may not scale.The processor selects every
attachmentKeyfrompost_attachmentsand builds an in-memorySet. For workspaces with a large number of attachments this query transfers and holds the full key set in memory on every run. Consider scoping the query to only the candidate keys (e.g.,where attachmentKey in (...)) or streaming the comparison if the table grows large.🤖 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/cleanup-attachments/processor.ts` around lines 26 - 31, Update the referenced-key lookup in the cleanup processor to scope postAttachments rows to the current candidate attachment keys, using the query builder’s attachmentKey membership filter before constructing referencedKeys. Preserve the existing Set-based comparison while avoiding retrieval of unrelated rows.
33-38: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffSequential deletion is safe but slow for large orphan sets.
The
for...ofloop awaits eachs3.removecall one at a time. This is correct for error isolation but could be slow when many orphans exist. If throughput becomes a concern, consider bounded concurrency (e.g.,p-limitor a simple chunkedPromise.all) while preserving the fail-fast behavior on the first error.🤖 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/cleanup-attachments/processor.ts` around lines 33 - 38, Update the orphan-removal loop in the cleanup processor to delete unreferenced keys with bounded concurrency instead of awaiting each s3.remove call sequentially. Preserve fail-fast behavior by propagating the first deletion error, and retain the existing skip logic and success logging for each removed key.apps/worker/src/tasks/cleanup-attachments/processor.test.ts (1)
10-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for error propagation when
s3.removethrows.The PR objective states the job "fails the job if an error occurs," but no test verifies that a throwing
s3.removecauses the processor to reject. A test likevi.mocked(s3.remove).mockRejectedValueOnce(new Error("..."))asserting the processor rejects would lock in that contract.🤖 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/cleanup-attachments/processor.test.ts` around lines 10 - 121, Add a test alongside the existing cleanup processor tests that configures an eligible orphaned attachment and makes s3.remove reject with an Error, then assert that processor rejects with the same error. Reuse the existing S3 listing and database mocks needed to reach removal, and verify the rejection propagates without being swallowed.
🤖 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/types.ts`:
- Line 50: Update the TaskOutputs mapping for Tasks.CLEANUP_ATTACHMENTS from
object to void so it matches the cleanup processor’s Promise<void> return
contract and resolves the processor type mismatch.
---
Nitpick comments:
In `@apps/worker/src/tasks/cleanup-attachments/processor.test.ts`:
- Around line 10-121: Add a test alongside the existing cleanup processor tests
that configures an eligible orphaned attachment and makes s3.remove reject with
an Error, then assert that processor rejects with the same error. Reuse the
existing S3 listing and database mocks needed to reach removal, and verify the
rejection propagates without being swallowed.
In `@apps/worker/src/tasks/cleanup-attachments/processor.ts`:
- Around line 26-31: Update the referenced-key lookup in the cleanup processor
to scope postAttachments rows to the current candidate attachment keys, using
the query builder’s attachmentKey membership filter before constructing
referencedKeys. Preserve the existing Set-based comparison while avoiding
retrieval of unrelated rows.
- Around line 33-38: Update the orphan-removal loop in the cleanup processor to
delete unreferenced keys with bounded concurrency instead of awaiting each
s3.remove call sequentially. Preserve fail-fast behavior by propagating the
first deletion error, and retain the existing skip logic and success logging for
each removed key.
In `@packages/s3/src/utils.ts`:
- Around line 79-107: Consider changing list() to expose matching S3 objects
incrementally through an async iterable or callback instead of accumulating
every object in the objects array. Preserve pagination via continuationToken and
emit each valid object as soon as it is received, while updating callers to
consume the streaming interface where appropriate.
🪄 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: 5bffafa3-3312-4144-af8f-85464415f326
📒 Files selected for processing (6)
apps/worker/src/index.tsapps/worker/src/tasks/cleanup-attachments/processor.test.tsapps/worker/src/tasks/cleanup-attachments/processor.tsapps/worker/test-utils/setup.tspackages/bullmq/src/tasks/types.tspackages/s3/src/utils.ts
TaskOutputs[Tasks.CLEANUP_ATTACHMENTS] was typed as object but the processor returns Promise<void>, breaking the build. Also adds a test locking in that an S3 removal failure rejects the job instead of being swallowed.
1 participant
Description:
Closes #191.
Adds a
cleanup-attachmentsBullMQ task that removes S3 attachment objects with no matchingpost_attachmentsrow, sincesync-postcan't safely delete an attachment itself when multiple branches of the same post might still be using it.What changed
packages/s3/src/utils.ts: newlist(bucket, prefix)helper (paginatedListObjectsV2), returning each object's key andlastModified. Reuses the existing S3 client rather than adding new client logic.apps/worker/src/tasks/cleanup-attachments/processor.ts: lists S3 objects underposts/*/attachments/*, filters out anything younger than a one-hour grace period, diffs the remainder against the full set ofattachmentKeyvalues inpost_attachments, and removes any orphaned object. Plain sequential loop, no concurrency/batching, errors throw and fail the job — matching the existing worker-task conventions.packages/bullmq, wired up inapps/worker/src/index.tsalongside a repeatable job registration (see open question test: add initial unit tests #1 below).processor.test.tscovers: an unreferenced attachment getting removed, a referenced attachment being left alone, a no-op when there are no attachment objects, that the query has no per-post filter, and that an otherwise-unreferenced attachment inside the grace period is left alone (and never even triggers thepost_attachmentsquery).Open questions for James
Trigger mechanism. The issue doesn't specify what should kick this off. I defaulted to a BullMQ repeatable job on a fixed interval (currently once/day, registered in
apps/worker/src/index.ts), since re-registering a repeatable job with the same name/options on every worker restart is a no-op in BullMQ rather than a duplicate schedule. Open to a different trigger (e.g. adev/-style manual route, or something driven off the webhook work in Webhook to import/sync content from GitHub with the db #7) if you'd rather not have an always-on scheduled job.Grace period threshold.
sync-postuploads an attachment to S3 in Phase 3 before writing thepost_attachmentsrow in the Phase 4 transaction, so an object could briefly look orphaned mid-flight (or permanently, if the job crashes in that window). This PR ships a one-hour grace period by default — any S3 object younger than that is never considered a deletion candidate, regardless of whether apost_attachmentsrow references it yet. Wanted your read on whether one hour is the right value, or whether you'd prefer a different threshold (or a different mechanism entirely, e.g. checking job status rather than object age).Schema/scope mismatch worth flagging explicitly. The issue text describes attachments living in an
attachmentstable referenced by zero-or-manypost_attachmentsrows, but no such table exists —post_attachments(from Upload "post attachments" in sync-post task #86/PR feat: sync post attachments (images, PDFs, PowerPoints) to S3 #181) stores the S3 key inline per(postSlug, attachmentName)row, and that key is generated asposts/${post}/attachments/${sha}${extension}— i.e. namespaced per post, so it can never actually be shared across posts or branches under the current scheme. Given that, this PR implements the closest real analog: a straight S3-vs-post_attachmentsorphan sweep, catching objects left behind by interruptedsync-postjobs (see Migrate tasks to BullMQ #2). It does not implement true cross-post/cross-branch attachment sharing, since that would require changing the key scheme to be content-addressed independent of post slug — happy to scope that as separate follow-up work if that's actually the intent behind the issue.Test plan
pnpm test:unitpasses (lint, knip, publint, sherif, vitest across all projects)pnpm prettiercheck clean (aside from the pre-existing.claude/settings.local.jsonexclusion)Summary by CodeRabbit
New Features
Bug Fixes
Tests