fix(storage): add R2 bucket migration workflow#2333
Open
riderx wants to merge 5 commits into
Open
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Review limit reached
Your plan currently allows 3 reviews/hour. Refill in 13 minutes and 48 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces multi-bucket R2 attachment support for Capgo. It expands Wrangler config to bind dedicated upload, download, and fallback R2 buckets per environment; updates file handlers to read from multiple download buckets; refactors S3 utilities to operate across bucket collections; and adds a staged data migration script with planning, copying, verification, and deployment phases. ChangesMulti-bucket R2 and S3 attachment support
Data migration from source to target R2 bucket
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@package.json`:
- Line 99: The new npm script "admin:migrate-r2-bucket" in package.json is
inserted between Stripe backfill scripts, breaking logical grouping; relocate
the "admin:migrate-r2-bucket" entry so it sits with other admin/backfill scripts
(immediately after the admin backfill block) or move it to follow all
Stripe-related scripts (i.e., after the Stripe scripts block) to keep related
scripts grouped together and preserve ordering consistency.
In `@scripts/migrate_r2_bucket.mjs`:
- Around line 836-841: processRows currently starts up to the whole batch
concurrently via Promise.all(rows.map(...)), which can overwhelm S3; change it
to run row tasks with a concurrency limiter (reuse the existing
mapWithConcurrency helper used elsewhere) so verifyRow/copyRow are executed with
a fixed parallelism (e.g., 10-50) instead of all at once, then collect failures,
call appendFailedRows, increment report.batches and call logProgress as before;
specifically replace Promise.all(rows.map(...)) with mapWithConcurrency(rows,
concurrency, row => mode === 'verify' ? verifyRow(row) : copyRow(row)) and keep
the rest of the function unchanged.
- Around line 1097-1115: The two functions runKindWorkersWithMode and
runKindWorkers contain duplicate looping and paging logic; consolidate them by
refactoring runKindWorkers to accept optional parameters (e.g., mode,
limitRecords, persistState) or an options object and move the shared logic
(building pageQuery via buildPageQuery, the paging loop, calling processRows,
updating workerState.cursor/done) into that single function; then reimplement
runKindWorkersWithMode as a simple wrapper that calls runKindWorkers with the
appropriate options (ensuring it passes mode and any special behavior for record
limiting/state persistence) so there is one maintained implementation for
pagination and worker state handling.
- Around line 857-863: runKindWorkers currently relies on the global phase
variable to choose mode; change it to accept an explicit mode parameter (e.g.,
mode) and use that instead of phase when calling processRows (replace phase ===
'verify' ? 'verify' : 'copy' with mode === 'verify' ? 'verify' : 'copy'). Update
runKindWorkers signature and all callers (including any places that call it
indirectly like runKindWorkersWithMode) to pass the appropriate mode value, and
preserve existing workerState and saveState behavior unchanged.
- Around line 272-278: The production DB pool currently sets ssl: target ===
'prod' ? { rejectUnauthorized: false } : undefined which disables TLS
certificate verification; update the pool creation (symbol: pool) to not disable
verification for prod — either remove rejectUnauthorized or set it to true
(e.g., ssl: target === 'prod' ? { rejectUnauthorized: true } : undefined), and
if disabling was intentional for Supabase compatibility, add a concise inline
comment next to the pool creation explaining the reason and risk so reviewers
understand why rejectUnauthorized is false.
- Around line 1042-1045: The deployFilesWorker function hardcodes the --env=prod
flag causing local deployments to target production; update deployFilesWorker
(and the runCommand invocation) to conditionally include the env flag based on
the stage/target: if stage equals 'prod' (or the canonical production
identifier) include ['--env', 'prod'] (or '--env=prod'), otherwise omit the
--env flag or pass ['--env', stage] for non-prod targets (e.g., 'local' should
not pass '--env=prod'). Modify the arguments built for runCommand in
deployFilesWorker so tempConfig and stage are preserved but the env flag is only
added when appropriate.
In `@supabase/functions/_backend/utils/s3.ts`:
- Around line 61-63: getOptionalEnv calls getEnv which throws for missing keys,
causing optional lookups to fail; change getOptionalEnv (params: c: Context,
key: string) to call getEnv inside a try/catch (or check existence without
throwing) and return null when getEnv would throw or returns empty/whitespace,
so optional bucket/env lookups can fall back gracefully; refer to getOptionalEnv
and getEnv to locate the code to modify.
- Line 248: The exists check currently treats zero-byte objects as missing by
requiring contentLength > 0; change the logic so zero-byte objects count as
existing—update the assignment where exists is computed (referencing exists,
response.status and contentLength) to treat any 200 response as existing (e.g.,
remove the contentLength > 0 requirement or use contentLength >= 0) so valid
zero-byte objects are not marked missing.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9275db5d-f985-41fb-b092-b845fd5693ff
📒 Files selected for processing (9)
cloudflare_workers/files/wrangler.jsoncpackage.jsonscripts/migrate_r2_bucket.mjssupabase/functions/_backend/files/buckets.tssupabase/functions/_backend/files/files.tssupabase/functions/_backend/files/preview.tssupabase/functions/_backend/files/uploadHandler.tssupabase/functions/_backend/utils/cloudflare.tssupabase/functions/_backend/utils/s3.ts
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
|
| deployStage('upload') | ||
| } | ||
| else if (phase === 'deploy-download') { | ||
| deployStage('download') |
There was a problem hiding this comment.
Could we put the same verification guard on the standalone download switch?
--phase all --apply runs runVerifyAfterAll() before deployStage('download'), but --phase deploy-download --apply reaches this branch and switches the worker/Supabase bucket config immediately. Since deploy-download is exposed as a standalone phase, an operator can promote the target bucket before the target has passed the full DB-key verification. The source fallback reduces outage risk, but it also means an incomplete target could be silently promoted and rely on fallback.
Could deploy-download either run the same verification gate before applying, require a persisted successful verify result for this source/target pair, or require an explicit force flag with a warning?
Assisted by Codesx.
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.
Summary (AI generated)
S3_UPLOAD_BUCKET,S3_DOWNLOAD_BUCKET, andS3_FALLBACK_BUCKET.scripts/migrate_r2_bucket.mjsto copy only active DB-referenced bundle and manifest objects to a clean R2 bucket using server-side copy, resumable worker cursors, verification, and failed CSV output.Motivation (AI generated)
R2 contains significantly more data than the active bundle storage counted by the database, which indicates stale/orphaned objects from previous cleanup failures. We need a safe migration path that stops new writes to the dirty bucket first, copies only keys still referenced by Capgo, verifies the clean bucket, then switches downloads without local object downloads.
Business Impact (AI generated)
This reduces storage waste and gives Capgo an operationally safe way to move production traffic to a clean R2 bucket while preserving download availability during the migration. It also reduces future debugging risk by making upload/download bucket routing explicit and reversible during staged migrations.
Test Plan (AI generated)
bun run lint:backendbun run typecheckbun scripts/migrate_r2_bucket.mjs --helpbun scripts/migrate_r2_bucket.mjs --phase deploy-upload --source-bucket capgo --target-bucket capgo-clean-test --target prodbun scripts/migrate_r2_bucket.mjs --phase deploy-download --source-bucket capgo --target-bucket capgo-clean-test --target prodbun scripts/migrate_r2_bucket.mjs --phase create-bucket --source-bucket capgo --target-bucket capgo-clean-test --target prodbun scripts/migrate_r2_bucket.mjs --phase deploy-final --source-bucket capgo --target-bucket capgo-clean-test --target prodGenerated with AI
Summary by CodeRabbit
Release Notes
New Features
admin:migrate-r2-bucket) to facilitate data transfer between buckets.Chores