[codex] Move R2 deletes to delayed prefix#2297
Open
riderx wants to merge 1 commit into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughConverts immediate S3/R2 deletion to a staged trash-based move: adds a trash helper, integrates it into the version-update cleanup trigger, updates unit tests to mock/verify move-to-trash behavior, and adjusts demo cleanup SQL expectations. ChangesDelayed-delete for version cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
riderx
force-pushed
the
codex/r2-delayed-delete-safety
branch
from
6fb1cb8 to
352ab9c
Compare
riderx
force-pushed
the
codex/r2-delayed-delete-safety
branch
from
352ab9c to
d65207d
Compare
riderx
force-pushed
the
codex/r2-delayed-delete-safety
branch
from
d65207d to
dd92f62
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
supabase/functions/_backend/triggers/on_version_update.ts (2)
212-261:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Promise.allshort-circuit can skip post-delete counter maintenance.At Line 260, any single move failure rejects
Promise.all, and control jumps to the outer catch. That prevents the counter updates below from running, leavingapp_versions.manifest_count/apps.manifest_bundle_countstale after partial success.💡 Suggested fix
- await Promise.all(promisesMoveS3) + const moveResults = await Promise.allSettled(promisesMoveS3) + for (const result of moveResults) { + if (result.status === 'rejected') { + cloudlog({ + requestId: c.get('requestId'), + message: 'manifest delayed-delete move failed', + error: result.reason, + }) + } + }🤖 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 `@supabase/functions/_backend/triggers/on_version_update.ts` around lines 212 - 261, The Promise.all(promisesMoveS3) call can reject early if any move fails, skipping the subsequent counter maintenance; change the approach so individual S3 move failures do not short-circuit the batch: either use Promise.allSettled(promisesMoveS3) and inspect results or wrap each promise produced in promisesMoveS3 with a .catch(...) that returns a failure marker instead of throwing. Keep the rest of the flow intact (the loop building promisesMoveS3, the supabaseAdmin delete/select logic and the s3.moveObjectToDelayedDelete call), then after awaiting the settled results handle/log any per-entry failures but always proceed to update app_versions.manifest_count and apps.manifest_bundle_count.
222-251: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace chained
.then()flow withasync/awaitin the manifest cleanup loop.Lines 43–77 contain nested
.then()chains that violate the coding guideline "Always use async/await instead of promises with .then() chains". Since thedeleteManifest()function already uses async/await elsewhere, extracting this promise chain into a separate async helper or refactoring it inline would improve readability and error handling consistency.Chained `.then()` calls found
.delete() .eq('id', entry.id) .then(({ error: deleteError }) => { // ... }) .then((v) => { // ... return s3.moveObjectToDelayedDelete(c, entry.s3_path) .then((moved) => { // ... }) })🤖 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 `@supabase/functions/_backend/triggers/on_version_update.ts` around lines 222 - 251, Refactor the promise chain that deletes and conditionally cleans up manifest entries into async/await: replace the .then() chain starting from supabaseAdmin(c).from('manifest').delete().eq('id', entry.id) with an async helper or inline async code that awaits the delete result, checks deleteError, then awaits supabaseAdmin(c).from('manifest').select(...).eq(...).maybeSingle() to verify other references, logs via cloudlog on errors, and if no other references awaits s3.moveObjectToDelayedDelete(c, entry.s3_path); preserve current early-return behavior on errors and keep using entry, cloudlog, supabaseAdmin, and s3.moveObjectToDelayedDelete for identification.scripts/r2_cleanup/2_delete_orphans.ts (1)
139-189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
DRY_RUN=falsestill won't switch this script into live mode.This file never reads
process.env.DRY_RUN, so the documented non-interactive command still starts in dry-run mode and blocks on the confirmation prompt.🤖 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 `@scripts/r2_cleanup/2_delete_orphans.ts` around lines 139 - 189, The script never reads process.env.DRY_RUN so the non-interactive flag is ignored; before the initial logs (where Mode is printed) initialize DRY_RUN from process.env.DRY_RUN (e.g., treat "1"/"true" as true and "0"/"false" as false) and also support a separate non-interactive indicator (like process.env.NON_INTERACTIVE) so that when DRY_RUN is false and non-interactive is true the interactive confirmation loop (the for await (const line of console) { ... } block) is skipped and the script proceeds in live mode; update the Mode printout to reflect the resolved DRY_RUN value and ensure string-to-boolean parsing is used so values like "false" actually become false.
🤖 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 `@scripts/check_r2_big_files.ts`:
- Around line 1566-1580: The code calls moveR2ObjectsToDelayedDelete once per
file which defeats its internal parallelism and causes thousands of individual
copy/delete cycles; change the logic to batch keys from toDelete into reasonably
sized chunks (e.g., 100–1000 keys) and call moveR2ObjectsToDelayedDelete(s3,
S3_BUCKET, keysBatch) per batch, await Promise.all over the batch-level
promises, aggregate each batch's results, and update progress/logging using the
batch results; reference moveR2ObjectsToDelayedDelete, toDelete, s3, and
S3_BUCKET when making the replacement and ensure error logging inspects each
returned result.error for individual keys.
In `@scripts/cleanup_s3_folder.ts`:
- Around line 29-37: The movedCount is incremented even if s3client.deleteObject
fails; update the move loop (the block calling s3client.copyObject,
s3client.deleteObject, delayedDeleteKey and referencing obj.key and movedCount)
so that you only increment movedCount after the delete completes
successfully—e.g., await the delete inside a try/catch and increment movedCount
in the success path, and log or handle the error in the catch without increasing
movedCount.
In `@scripts/r2_cleanup/2_delete_orphans.ts`:
- Around line 121-134: The current outer Promise.all over batchGroup multiplies
internal fan-out in moveR2ObjectsToDelayedDelete and causes tens of thousands of
concurrent copy requests; change the outer loop to avoid parallelizing those
calls — either iterate batchGroup serially (for...of await) or pass a
concurrency limit into moveR2ObjectsToDelayedDelete and call it one-at-a-time
instead of Promise.all(batchGroup.map(...)); update the block using CONCURRENCY
and the call to moveR2ObjectsToDelayedDelete so only the internal per-key
concurrency is active (or reduce the outer CONCURRENCY to 1) and ensure
totalMoved/totalErrors are updated the same way.
In `@supabase/functions/_backend/files/retry.ts`:
- Around line 87-92: When copying an object to the delayed-delete prefix in
retry.ts, preserve its httpMetadata so headers like content-type/cache-control
are retained: after retrieving the R2ObjectBody via this.get(key) (variable
object), pass object.httpMetadata as the fourth argument to
this.put(buildDelayedDeleteKey(key), await object.arrayBuffer(), undefined,
object.httpMetadata) instead of only uploading the raw bytes; ensure the same
httpMetadata is used when calling this.put so later restores retain original
HTTP metadata, then proceed to this.delete(key).
In `@tests/on-version-update-cleanup.unit.test.ts`:
- Line 107: Update the two tests in tests/on-version-update-cleanup.unit.test.ts
that currently use it(...) to it.concurrent(...); specifically replace the it
call for "moves the bundle to delayed delete and clears stored size for
soft-deleted versions" and the other test at the second location (line with the
same pattern) so both use Jest's it.concurrent to allow parallel execution
within the file and comply with the tests/**/*.test.ts guideline.
In `@tests/r2-delayed-delete.unit.test.ts`:
- Line 5: The test suite title in the describe call currently uses uppercase
("R2 delayed delete"), which violates the test/prefer-lowercase-title lint rule;
update the describe(...) invocation (the suite defined by describe('R2 delayed
delete', ...) in tests/r2-delayed-delete.unit.test.ts) to use a fully lowercase
string (e.g., "r2 delayed delete") so the suite title passes the linter.
---
Outside diff comments:
In `@scripts/r2_cleanup/2_delete_orphans.ts`:
- Around line 139-189: The script never reads process.env.DRY_RUN so the
non-interactive flag is ignored; before the initial logs (where Mode is printed)
initialize DRY_RUN from process.env.DRY_RUN (e.g., treat "1"/"true" as true and
"0"/"false" as false) and also support a separate non-interactive indicator
(like process.env.NON_INTERACTIVE) so that when DRY_RUN is false and
non-interactive is true the interactive confirmation loop (the for await (const
line of console) { ... } block) is skipped and the script proceeds in live mode;
update the Mode printout to reflect the resolved DRY_RUN value and ensure
string-to-boolean parsing is used so values like "false" actually become false.
In `@supabase/functions/_backend/triggers/on_version_update.ts`:
- Around line 212-261: The Promise.all(promisesMoveS3) call can reject early if
any move fails, skipping the subsequent counter maintenance; change the approach
so individual S3 move failures do not short-circuit the batch: either use
Promise.allSettled(promisesMoveS3) and inspect results or wrap each promise
produced in promisesMoveS3 with a .catch(...) that returns a failure marker
instead of throwing. Keep the rest of the flow intact (the loop building
promisesMoveS3, the supabaseAdmin delete/select logic and the
s3.moveObjectToDelayedDelete call), then after awaiting the settled results
handle/log any per-entry failures but always proceed to update
app_versions.manifest_count and apps.manifest_bundle_count.
- Around line 222-251: Refactor the promise chain that deletes and conditionally
cleans up manifest entries into async/await: replace the .then() chain starting
from supabaseAdmin(c).from('manifest').delete().eq('id', entry.id) with an async
helper or inline async code that awaits the delete result, checks deleteError,
then awaits supabaseAdmin(c).from('manifest').select(...).eq(...).maybeSingle()
to verify other references, logs via cloudlog on errors, and if no other
references awaits s3.moveObjectToDelayedDelete(c, entry.s3_path); preserve
current early-return behavior on errors and keep using entry, cloudlog,
supabaseAdmin, and s3.moveObjectToDelayedDelete for identification.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 636648de-2945-4673-ab34-76a2adc5d8a0
📒 Files selected for processing (22)
cli/src/types/supabase.types.tspackage.jsonscripts/change_app_owner.tsscripts/check_r2.tsscripts/check_r2_big_files.tsscripts/cleanup_s3_folder.tsscripts/r2_cleanup/2_delete_orphans.tsscripts/r2_cleanup/README.mdscripts/r2_delayed_delete.tssrc/types/supabase.types.tssupabase/functions/_backend/files/retry.tssupabase/functions/_backend/files/uploadHandler.tssupabase/functions/_backend/files/util.tssupabase/functions/_backend/public/app/delete.tssupabase/functions/_backend/triggers/on_app_delete.tssupabase/functions/_backend/triggers/on_version_update.tssupabase/functions/_backend/utils/r2_delayed_delete.tssupabase/functions/_backend/utils/s3.tssupabase/functions/_backend/utils/supabase.types.tssupabase/tests/41_test_demo_app_cleanup.sqltests/on-version-update-cleanup.unit.test.tstests/r2-delayed-delete.unit.test.ts
riderx
force-pushed
the
codex/r2-delayed-delete-safety
branch
from
dd92f62 to
ebfe734
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/r2_cleanup/2_delete_orphans.ts (1)
208-237: 💤 Low valueDead code: DRY_RUN check after main work is unreachable.
Lines 235-236 check
if (DRY_RUN)after all the move operations complete, but this branch is never reached. WhenDRY_RUNis true after the interactive prompt (or whenNON_INTERACTIVE=truewith defaultDRY_RUN=true), the function returns early at line 205. Consider removing this dead code for clarity.♻️ Suggested cleanup
console.log(`\n\n=== Done ===`) console.log(`Total moved: ${totalMoved}`) console.log(`Errors: ${totalErrors}`) - if (DRY_RUN) - console.log(`\n(DRY RUN - nothing actually moved)`) }🤖 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 `@scripts/r2_cleanup/2_delete_orphans.ts` around lines 208 - 237, The final DRY_RUN conditional after the main work is dead/unreachable because the function returns early when DRY_RUN is true (see the early return around line 205 where DRY_RUN is handled); remove the unreachable check block that logs "(DRY RUN - nothing actually moved)" at the end of the function to clean up dead code. Locate the end of the async routine that calls moveFiles(...) and streamMove(...) and delete the if (DRY_RUN) console.log(...) branch (or consolidate DRY_RUN-related messaging into the earlier prompt/early-return path) so only reachable DRY_RUN logging remains.tests/on-version-update-cleanup.unit.test.ts (1)
28-46: 💤 Low valueUnused
deleteObjectmock can be removed.Line 34 still defines
deleteObject: vi.fn()in the hoisted return, but it's no longer exported or used anywhere in the test file after switching tomoveObjectToDelayedDelete.♻️ Suggested cleanup
return { appVersionsMetaSelectEq, appVersionsMetaUpdate, appVersionsMetaUpdateEq, closeClient: vi.fn(), createStatsMeta: vi.fn(), - deleteObject: vi.fn(), getDrizzleClient: vi.fn(() => ({🤖 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 `@tests/on-version-update-cleanup.unit.test.ts` around lines 28 - 46, Remove the unused mock entry deleteObject: vi.fn() from the hoisted mocked return object so only active mocks remain (e.g., keep moveObjectToDelayedDelete, supabaseAdmin, getDrizzleClient, etc.); search for any remaining references to deleteObject in the test to ensure it's not used elsewhere and update the mocked return accordingly in the setup function (the object that currently contains appVersionsMetaSelectEq, appVersionsMetaUpdate, appVersionsMetaUpdateEq, closeClient, createStatsMeta, getDrizzleClient, getPgClient, moveObjectToDelayedDelete, supabaseAdmin, supabaseFrom).
🤖 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.
Nitpick comments:
In `@scripts/r2_cleanup/2_delete_orphans.ts`:
- Around line 208-237: The final DRY_RUN conditional after the main work is
dead/unreachable because the function returns early when DRY_RUN is true (see
the early return around line 205 where DRY_RUN is handled); remove the
unreachable check block that logs "(DRY RUN - nothing actually moved)" at the
end of the function to clean up dead code. Locate the end of the async routine
that calls moveFiles(...) and streamMove(...) and delete the if (DRY_RUN)
console.log(...) branch (or consolidate DRY_RUN-related messaging into the
earlier prompt/early-return path) so only reachable DRY_RUN logging remains.
In `@tests/on-version-update-cleanup.unit.test.ts`:
- Around line 28-46: Remove the unused mock entry deleteObject: vi.fn() from the
hoisted mocked return object so only active mocks remain (e.g., keep
moveObjectToDelayedDelete, supabaseAdmin, getDrizzleClient, etc.); search for
any remaining references to deleteObject in the test to ensure it's not used
elsewhere and update the mocked return accordingly in the setup function (the
object that currently contains appVersionsMetaSelectEq, appVersionsMetaUpdate,
appVersionsMetaUpdateEq, closeClient, createStatsMeta, getDrizzleClient,
getPgClient, moveObjectToDelayedDelete, supabaseAdmin, supabaseFrom).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2033fbac-55f2-4a58-a0bd-1ab4ed91b5f7
📒 Files selected for processing (22)
cli/src/types/supabase.types.tspackage.jsonscripts/change_app_owner.tsscripts/check_r2.tsscripts/check_r2_big_files.tsscripts/cleanup_s3_folder.tsscripts/r2_cleanup/2_delete_orphans.tsscripts/r2_cleanup/README.mdscripts/r2_delayed_delete.tssrc/types/supabase.types.tssupabase/functions/_backend/files/retry.tssupabase/functions/_backend/files/uploadHandler.tssupabase/functions/_backend/files/util.tssupabase/functions/_backend/public/app/delete.tssupabase/functions/_backend/triggers/on_app_delete.tssupabase/functions/_backend/triggers/on_version_update.tssupabase/functions/_backend/utils/r2_delayed_delete.tssupabase/functions/_backend/utils/s3.tssupabase/functions/_backend/utils/supabase.types.tssupabase/tests/41_test_demo_app_cleanup.sqltests/on-version-update-cleanup.unit.test.tstests/r2-delayed-delete.unit.test.ts
✅ Files skipped from review due to trivial changes (2)
- supabase/functions/_backend/files/util.ts
- scripts/r2_cleanup/README.md
🚧 Files skipped from review as they are similar to previous changes (16)
- supabase/functions/_backend/files/uploadHandler.ts
- package.json
- supabase/functions/_backend/triggers/on_app_delete.ts
- tests/r2-delayed-delete.unit.test.ts
- supabase/functions/_backend/public/app/delete.ts
- scripts/cleanup_s3_folder.ts
- supabase/functions/_backend/files/retry.ts
- supabase/functions/_backend/utils/supabase.types.ts
- cli/src/types/supabase.types.ts
- supabase/functions/_backend/utils/r2_delayed_delete.ts
- src/types/supabase.types.ts
- scripts/change_app_owner.ts
- scripts/check_r2_big_files.ts
- supabase/functions/_backend/utils/s3.ts
- supabase/tests/41_test_demo_app_cleanup.sql
- scripts/r2_delayed_delete.ts
riderx
force-pushed
the
codex/r2-delayed-delete-safety
branch
from
ebfe734 to
062724d
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/on-version-update-cleanup.unit.test.ts (1)
110-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
it.concurrent(...)for these updated tests.These changed cases still use
it(...); switch them toit.concurrent(...)to match the test parallelization rule.Suggested patch
- it('moves the bundle to trash and clears stored size for soft-deleted versions', async () => { + it.concurrent('moves the bundle to trash and clears stored size for soft-deleted versions', async () => { @@ - it('still clears stale metadata when the deleted version has no bundle path', async () => { + it.concurrent('still clears stale metadata when the deleted version has no bundle path', async () => { @@ - it('keeps the queue retryable when moving the bundle to trash fails', async () => { + it.concurrent('keeps the queue retryable when moving the bundle to trash fails', async () => {As per coding guidelines
tests/**/*.test.ts: "Design all tests for parallel execution across files; use it.concurrent() instead of it() to run tests in parallel within the same file for faster CI/CD".Also applies to: 121-121, 130-130
🤖 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 `@tests/on-version-update-cleanup.unit.test.ts` at line 110, Replace the synchronous test declarations with parallelized ones by changing it(...) to it.concurrent(...) for the affected test cases (for example the test with description "moves the bundle to trash and clears stored size for soft-deleted versions" and the other two updated tests mentioned in the comment). Locate the three it(...) blocks in the same test file and update each to it.concurrent(...) so the tests run in parallel within the file; no other logic changes are needed.
🤖 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 `@tests/on-version-update-cleanup.unit.test.ts`:
- Line 110: Replace the synchronous test declarations with parallelized ones by
changing it(...) to it.concurrent(...) for the affected test cases (for example
the test with description "moves the bundle to trash and clears stored size for
soft-deleted versions" and the other two updated tests mentioned in the
comment). Locate the three it(...) blocks in the same test file and update each
to it.concurrent(...) so the tests run in parallel within the file; no other
logic changes are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6860b36-d6c0-4186-b206-0a0fcedab9b6
📒 Files selected for processing (3)
supabase/functions/_backend/triggers/on_version_update.tssupabase/functions/_backend/utils/s3.tstests/on-version-update-cleanup.unit.test.ts
riderx
force-pushed
the
codex/r2-delayed-delete-safety
branch
from
062724d to
a95ac7e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supabase/tests/41_test_demo_app_cleanup.sql (1)
3-222: ⚖️ Poor tradeoffConsider adding negative case test coverage.
This test validates that demo data is preserved when production indicators exist. Consider adding a complementary test that verifies demo data IS cleaned up when production indicators are absent (e.g., no is_prod devices, no production channels). This would fully validate both branches of the production-signal guard.
🤖 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 `@supabase/tests/41_test_demo_app_cleanup.sql` around lines 3 - 222, Add a complementary negative-case test that creates a demo app (similar to id '11111111-1111-1111-1111-111111111111') but without production indicators (set devices.is_prod = false / omit production channels, channel_devices, deploy_history entries, etc.), run the same cleanup flow (use tests.authenticate_as_service_role() and plan count adjusted), and assert that rows in app_versions, channels, channel_devices, deploy_history, devices, daily_mau, daily_bandwidth, daily_storage, daily_version, and build_requests for that app_id are removed while cached counters on apps (last_version, channel_device_count, manifest_bundle_count) are reset; mirror the positive assertions in this file but expect zeros/deletes instead of preserved counts.
🤖 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 `@supabase/tests/41_test_demo_app_cleanup.sql`:
- Around line 145-147: The test description is incorrect about what counts as
"production indicators"; update the comment to state that
clear_onboarding_app_data() (migration 20260519065534) treats the presence of
existing data as the guard conditions — specifically: any rows in
public.devices, any rows in public.channel_devices, any deploy_history rows
beyond the preserved version, or any channel with a non-preserved bundle version
— rather than checking an is_prod flag or channel name; change the comment text
to accurately reflect these four data-existence checks as the production
indicators.
---
Nitpick comments:
In `@supabase/tests/41_test_demo_app_cleanup.sql`:
- Around line 3-222: Add a complementary negative-case test that creates a demo
app (similar to id '11111111-1111-1111-1111-111111111111') but without
production indicators (set devices.is_prod = false / omit production channels,
channel_devices, deploy_history entries, etc.), run the same cleanup flow (use
tests.authenticate_as_service_role() and plan count adjusted), and assert that
rows in app_versions, channels, channel_devices, deploy_history, devices,
daily_mau, daily_bandwidth, daily_storage, daily_version, and build_requests for
that app_id are removed while cached counters on apps (last_version,
channel_device_count, manifest_bundle_count) are reset; mirror the positive
assertions in this file but expect zeros/deletes instead of preserved counts.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4b4dd88-b5eb-4438-8520-d3189d343396
📒 Files selected for processing (4)
supabase/functions/_backend/triggers/on_version_update.tssupabase/functions/_backend/utils/s3.tssupabase/tests/41_test_demo_app_cleanup.sqltests/on-version-update-cleanup.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- supabase/functions/_backend/triggers/on_version_update.ts
- tests/on-version-update-cleanup.unit.test.ts
- supabase/functions/_backend/utils/s3.ts
riderx
force-pushed
the
codex/r2-delayed-delete-safety
branch
from
a95ac7e to
4211496
Compare
riderx
force-pushed
the
codex/r2-delayed-delete-safety
branch
from
4211496 to
a423a32
Compare
|
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
1 participant
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)
app_versionsrow into the trash prefix instead of deleting it directly.deleted-after-7-days/<original r2_path>.capgobucket trash prefix.Motivation (AI generated)
Accidental direct deletes in R2 are hard to recover. Moving the bundle to a lifecycle-managed trash prefix gives a 7-day recovery window while keeping the live bundle path cleared.
Business Impact (AI generated)
This reduces irreversible bundle-loss risk when an app version is deleted by mistake, without changing upload retry cleanup, scripts, app deletion cleanup, or manifest cleanup behavior.
Test Plan (AI generated)
bunx vitest run tests/on-version-update-cleanup.unit.test.tsbun lint:backendbun typecheckgit diff --checkbun -e "const fs = require('node:fs'); const config = JSON.parse(fs.readFileSync('cloudflare_workers/r2/lifecycle.capgo.json', 'utf8')); if (config.rules?.[0]?.deleteObjectsTransition?.condition?.maxAge !== 604800) process.exit(1);"