Skip to content

fix(manifest): solid deleted-version cleanup (R2 trash before DB delete)#2724

Merged
riderx merged 8 commits into
mainfrom
fix/manifest-deleted-cleanup
Jul 22, 2026
Merged

fix(manifest): solid deleted-version cleanup (R2 trash before DB delete)#2724
riderx merged 8 commits into
mainfrom
fix/manifest-deleted-cleanup

Conversation

@riderx

@riderx riderx commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary (AI generated)

  • Manifest cleanup now checks R2 existence → moves to deleted-after-7-days/ (7-day lifecycle) → only then deletes the DB row, so we never lose track of objects before trash.
  • Missing R2 objects are treated as already gone and the DB row is deleted; shared delta files still referenced by other versions are kept.
  • Hardened on_version_update queue: 300s HTTP timeout, 30 retries for version queue, no silent ACK on cleanup failure, cron sweeper for stuck soft-deleted versions.
  • Added scripts/reclaim_deleted_version_manifests.ts (single path: reclaim all stuck deleted manifests with the same R2-then-DB rules).
  • Load tests cover 5000-file manifests and partial trash/DB failures.

Motivation (AI generated)

Production manifest was ~8GB / ~10M rows, with ~5.8M rows stuck on soft-deleted versions because cleanup deleted DB rows first (or failed mid-flight) and the queue archived work without recovery.

Business Impact (AI generated)

Stops unbounded Postgres/replica growth from soft-deleted bundles, keeps OTA delta objects recoverable via lifecycle trash, and makes deleted-version cleanup durable under large manifests.

Test Plan (AI generated)

  • bunx vitest run tests/on-version-update-cleanup.unit.test.ts tests/queue-consumer-message-shape.unit.test.ts
  • Apply migration sweep_deleted_version_manifests in prod
  • Deploy functions / workers with queue timeout change
  • Run bun scripts/reclaim_deleted_version_manifests.ts against prod to drain backlog
  • Confirm manifest rows on deleted=true versions drop, then VACUUM as needed

Generated with AI

Made with Cursor

Review in cubic

Summary by CodeRabbit

  • New Features
    • Added a scheduled sweep to automatically clear soft-deleted app versions that still have leftover manifest data.
    • Added a recovery script to reclaim unreferenced manifest-backed objects from storage.
  • Bug Fixes
    • Improved manifest cleanup to prevent removing shared files still referenced elsewhere.
    • Added safer storage presence checks to avoid incorrect database deletions when objects are missing.
  • Reliability
    • Increased timeout and retry budgeting for version-related background queue processing.
  • Tests
    • Expanded unit tests to verify cleanup ordering, rollback behavior, and shared-reference edge cases.

Soft-deleted versions were leaving millions of manifest rows because cleanup
deleted DB tracking first and silently ACKed failures. Move objects to the
7-day lifecycle trash only after an existence check, delete each row after
that succeeds, raise version-queue timeout/retries, add a sweeper, and reclaim
stuck deleted manifests with the same path.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0a534bb3-f734-4f6f-8c78-61183250e9e2

📥 Commits

Reviewing files that changed from the base of the PR and between 48e5bc1 and 80344bf.

📒 Files selected for processing (6)
  • read_replicate/schema_replicate.catalog.json
  • read_replicate/schema_replicate.sql
  • scripts/reclaim_deleted_version_manifests.ts
  • supabase/functions/_backend/triggers/queue_consumer.ts
  • supabase/functions/_backend/utils/s3.ts
  • tests/queue-consumer-message-shape.unit.test.ts
📝 Walkthrough

Walkthrough

Deleted version manifest cleanup now moves unreferenced objects to trash before deleting rows, finalizes state transactionally, retries failures through version queues, and adds a scheduled database sweeper plus a bounded reclamation script.

Changes

Deleted manifest reclamation

Layer / File(s) Summary
Coordinated manifest cleanup
supabase/functions/_backend/triggers/on_version_update.ts, supabase/functions/_backend/utils/s3.ts, tests/on-version-update-cleanup.unit.test.ts
Manifest objects are reference-checked and moved before database rows are deleted; final state updates verify no rows remain and tests cover ordering, rollback, and retry behavior.
Version queue retry budgeting
supabase/functions/_backend/triggers/queue_consumer.ts, tests/queue-consumer-message-shape.unit.test.ts
Version queues use a 300-second timeout and a 30-read retry budget, while other queues retain the five-read budget.
Scheduled manifest recovery
supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql
A security-definer sweeper fixes stale counters, requeues deleted versions with manifest rows, and schedules execution through cron_tasks.
Bulk manifest reclamation
scripts/reclaim_deleted_version_manifests.ts, .gitignore
A paginated Bun script cleans deleted versions with bounded concurrency, shared-object checks, trash moves, row verification, and transactional counter updates; .context/ is ignored.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant VersionQueue
  participant on_version_update
  participant ObjectStorage
  participant Database
  VersionQueue->>on_version_update: dispatch deleted version update
  on_version_update->>Database: load manifest entries
  on_version_update->>ObjectStorage: trash unreferenced objects
  ObjectStorage-->>on_version_update: move results
  on_version_update->>Database: delete manifest rows
  on_version_update->>Database: verify and commit state reset
Loading

Suggested labels: codex

Suggested reviewers: dalanir, wcaleniewolny

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and clearly matches the main change: safer deleted-version manifest cleanup with R2 trash before DB deletion.
Description check ✅ Passed The description includes Summary, Motivation, Business Impact, and Test Plan, and the missing Screenshots section is reasonable for backend changes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@codspeed-hq

codspeed-hq Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 43 untouched benchmarks
⏩ 2 skipped benchmarks1


Comparing fix/manifest-deleted-cleanup (80344bf) with main (353621b)

Open in CodSpeed

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Co-authored-by: Cursor <cursoragent@cursor.com>
@riderx
riderx marked this pull request as ready for review July 22, 2026 14:41
@cursor

cursor Bot commented Jul 22, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_5250bd66-871e-4a6a-ab59-d64b3f8d08be)

@cursor
cursor Bot requested review from Dalanir and WcaleNieWolny July 22, 2026 14:42

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Risk: high. Cursor Bugbot did not complete (check skipped; usage limit), so there is no clean automated review signal. Human review is required for this production manifest/R2 cleanup, queue, and migration change; reviewers assigned.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver External

@coderabbitai coderabbitai Bot added the codex label Jul 22, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Risk: high. Cursor Bugbot did not complete (usage limit reached), so automated review is incomplete. Human review is needed for this migration and irreversible R2/DB cleanup path.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

@riderx

riderx commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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/reclaim_deleted_version_manifests.ts`:
- Line 100: Update the pg.Client SSL configuration in the database connection
initialization to remove rejectUnauthorized: false and enforce certificate
validation. Supply the trusted production database CA through ssl.ca using the
repository’s existing CA configuration or environment symbol, preserving the
requireDbUrl(env) connection setup.

In `@supabase/functions/_backend/triggers/on_version_update.ts`:
- Around line 356-400: Serialize cleanup for each shared manifest file in the
deleteManifest flow around the manifestEntries batch, using a per-file lock or
atomic database-side claim keyed by file_hash and file_name before checking
references and moving the S3 object. Ensure concurrent workers cannot both
observe a reference and delete their rows while leaving the shared R2 object
orphaned, while preserving the existing trash-before-row-delete behavior.

In `@supabase/functions/_backend/utils/s3.ts`:
- Around line 148-155: Update moveObjectToTrash around checkIfExist so only a
definitive 404/confirmed missing-object result skips the trash copy and returns
success. Treat HEAD failures and missing metadata as indeterminate, propagate or
fail those cases, and preserve DB tracking instead of allowing
deleteManifest/deleteIt to proceed as though the object were gone.

In `@supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql`:
- Around line 58-66: Validate the candidates CTE with EXPLAIN (ANALYZE, BUFFERS)
against production-scale data and add or verify indexes supporting the deleted
app_versions join, stale-counter predicate, and manifest(app_version_id) lookup.
Preserve the existing batching behavior while ensuring the GROUP BY and LIMIT
avoid repeated full scans.
🪄 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: f1df17e0-843d-4749-a20d-7fa3c8227cf1

📥 Commits

Reviewing files that changed from the base of the PR and between 4c5c9f0 and 927a526.

📒 Files selected for processing (7)
  • scripts/reclaim_deleted_version_manifests.ts
  • supabase/functions/_backend/triggers/on_version_update.ts
  • supabase/functions/_backend/triggers/queue_consumer.ts
  • supabase/functions/_backend/utils/s3.ts
  • supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql
  • tests/on-version-update-cleanup.unit.test.ts
  • tests/queue-consumer-message-shape.unit.test.ts
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • Cap-go/capacitor-updater (manual)

Comment thread scripts/reclaim_deleted_version_manifests.ts Outdated
Comment thread supabase/functions/_backend/triggers/on_version_update.ts
Comment thread supabase/functions/_backend/utils/s3.ts Outdated
Only skip R2 trash on a definitive absent HEAD, serialize shared-hash
cleanup with advisory locks, pin reclaim-script TLS verification, and
bound the sweeper scan with deleted-version indexes.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@cursor

cursor Bot commented Jul 22, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_d41bcc30-669d-4a68-9794-fa7e513f501d)

@cursor

cursor Bot commented Jul 22, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_16b41842-72a8-49ff-83df-8696bbe25e55)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Risk: high. Cursor Bugbot did not complete (usage limit reached), so automated review is incomplete. Not approving; human review is still needed for this migration and irreversible R2/DB cleanup path. Reviewers already assigned.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Risk: high. Cursor Bugbot did not complete (check skipped; usage limit), so there is no clean automated review signal. Human review remains required for this migration and irreversible R2/DB cleanup path; Dalanir and WcaleNieWolny are already assigned.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver External

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed

Confidence score: 5/5

  • Safe to merge after the addressed issues were fixed.

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread supabase/functions/_backend/triggers/queue_consumer.ts
Comment thread scripts/reclaim_deleted_version_manifests.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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/functions/_backend/utils/s3.ts`:
- Around line 150-159: Update getObjectPresence() to apply a finite timeout to
the HEAD fetch, using AbortSignal.timeout(...) or an equivalent abort mechanism
in the fetch options. Preserve the existing response-status handling and ensure
timeout or abort errors continue through the existing catch path to the
fail-closed fallback.

In `@supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql`:
- Around line 3-9: Address the write-lock risk from creating the partial indexes
in the sweep migration: assess whether app_versions is small enough for the
current transactional builds, and if not, move these index creations to an
out-of-band deployment using concurrent index creation. Preserve the index
definitions and partial predicates, and do not add CONCURRENTLY inside the
migration transaction.
- Line 10: Add the SQL comment marker to the wrapped header text in the
migration so every line of the comment, including “that still have manifest rows
or stale counters.”, begins with `--` and the migration parses correctly.
🪄 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: 5f4eafbb-5b52-4554-aff0-0ee25f99d778

📥 Commits

Reviewing files that changed from the base of the PR and between 927a526 and 56dcb1f.

📒 Files selected for processing (6)
  • .gitignore
  • scripts/reclaim_deleted_version_manifests.ts
  • supabase/functions/_backend/triggers/on_version_update.ts
  • supabase/functions/_backend/utils/s3.ts
  • supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql
  • tests/on-version-update-cleanup.unit.test.ts
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • Cap-go/capacitor-updater (manual)

Comment thread supabase/functions/_backend/utils/s3.ts
Comment thread supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql Outdated
Co-authored-by: Cursor <cursoragent@cursor.com>
@cursor

cursor Bot commented Jul 22, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_dcff2abf-90f2-47ff-a937-26a05ce5a7bb)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Risk: high. Cursor Bugbot did not complete (check skipped; usage limit), so automated review is incomplete. Not approving this migration and irreversible R2/DB cleanup path; Dalanir and WcaleNieWolny are already assigned for human review.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver External

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Risk: high. Cursor Bugbot did not complete (check skipped; usage limit), so automated review is incomplete. Not approving; human review remains required for this migration and irreversible R2/DB cleanup path. Reviewers already assigned.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql (1)

103-146: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Do not create a dedicated cron task for this sweeper.

Lines [103-146] upsert a new public.cron_tasks entry, violating the repository rule that scheduled work must be added to process_all_cron_tasks instead. Integrate this bounded sweep into that existing dispatcher rather than registering another cron task.

🤖 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/migrations/20260722095157_sweep_deleted_version_manifests.sql`
around lines 103 - 146, Remove the dedicated cron_tasks upsert for
sweep_deleted_version_manifests. Integrate the bounded
public.sweep_deleted_version_manifests(100) call into the existing
process_all_cron_tasks dispatcher, following its established scheduling and
error-handling flow.

Source: Coding guidelines

🤖 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 `@supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql`:
- Around line 103-146: Remove the dedicated cron_tasks upsert for
sweep_deleted_version_manifests. Integrate the bounded
public.sweep_deleted_version_manifests(100) call into the existing
process_all_cron_tasks dispatcher, following its established scheduling and
error-handling flow.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7f90e147-658e-4545-bdaf-c83cf419d541

📥 Commits

Reviewing files that changed from the base of the PR and between 56dcb1f and 48e5bc1.

📒 Files selected for processing (1)
  • supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • Cap-go/capacitor-updater (manual)

Keep deleted-manifest cleanup fail-closed when R2 HEAD hangs.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cursor

cursor Bot commented Jul 22, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_8ea98be6-a20c-4575-81ae-7d43bfc4430c)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Risk: high. Cursor Bugbot did not complete (check skipped; usage limit), so automated review is incomplete. Not approving; human review remains required for this migration and irreversible R2/DB cleanup path. Reviewers already assigned.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Risk: high. Cursor Bugbot did not complete (check skipped; usage limit), so automated review is incomplete. Not approving this migration and irreversible R2/DB cleanup path; Dalanir and WcaleNieWolny are already assigned for human review.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver External

Also URL-encode reclaim CopySource so trash moves don't fail on special key chars.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cursor

cursor Bot commented Jul 22, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_f10243c2-91dd-4885-8c98-c774a9056e34)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Risk: high. Cursor Bugbot did not complete (check skipped; usage limit), so automated review is incomplete. Not approving this migration and irreversible R2/DB cleanup path; Dalanir and WcaleNieWolny are already assigned for human review.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver External

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Risk: high. Cursor Bugbot did not complete (check skipped; usage limit), so automated review is incomplete. Not approving this migration and irreversible R2/DB cleanup path; Dalanir and WcaleNieWolny are already assigned for human review.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

Co-authored-by: Cursor <cursoragent@cursor.com>
@cursor

cursor Bot commented Jul 22, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_60db77a5-c3ae-4c11-9479-9d9b8a5a301c)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Risk: high. Cursor Bugbot did not complete (check skipped; usage limit), so automated review is incomplete. Not approving this migration and irreversible R2/DB cleanup path; Dalanir and WcaleNieWolny are already assigned for human review.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver External

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Risk: high. Cursor Bugbot did not complete (check skipped; usage limit), so automated review is incomplete. Not approving this migration and irreversible R2/DB cleanup path; Dalanir and WcaleNieWolny are already assigned for human review.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Approver

@sonarqubecloud

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 issues found across 10 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name=".gitignore">

<violation number="1" location=".gitignore:117">
P3: The new broad `.context/` rule subsumes the existing `.context/visual-diff` entry. Removing the narrower entry would keep the ignore list free of redundant patterns.</violation>
</file>

<file name="scripts/reclaim_deleted_version_manifests.ts">

<violation number="1" location="scripts/reclaim_deleted_version_manifests.ts:112">
P1: Remote database connections can silently lose TLS or certificate verification when any URL component merely contains `localhost`/`127.0.0.1`. Parse the connection URL and compare its hostname exactly against explicit loopback hosts, reusing that result in both checks.</violation>
</file>

<file name="supabase/functions/_backend/utils/s3.ts">

<violation number="1" location="supabase/functions/_backend/utils/s3.ts:159">
P1: A nonexistent or misconfigured bucket also returns 404 (`NoSuchBucket`), so this branch can classify every key as absent and let cleanup delete DB tracking while the objects remain in the actual R2 bucket. Preserve the error code via an SDK/body-bearing probe, treating only `NoSuchKey` as absent and all bucket/ambiguous failures as unknown.</violation>
</file>

<file name="supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql">

<violation number="1" location="supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql:71">
P2: The batch limit bounds matches, not work: once leftover manifests are sparse, each cron run may probe every deleted `app_versions` row. A resumable keyset cursor or manifest-driven work queue would keep inspected rows bounded as the backlog drains.</violation>

<violation number="2" location="supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql:72">
P1: Deleted versions with `deleted_at IS NULL` are repeatedly queued but never enter manifest cleanup because `getDeletedVersionAction` treats them as non-deleted. Consider backfilling `deleted_at` before enqueueing these rows, or handling this legacy state explicitly.</violation>
</file>

<file name="supabase/functions/_backend/triggers/on_version_update.ts">

<violation number="1" location="supabase/functions/_backend/triggers/on_version_update.ts:361">
P1: The advisory lock, reference check, delete, and commit are not guaranteed to run on one PostgreSQL session because they use `Pool.query`, so the lock/transaction can fail to protect the delete. Reusing one pool and checking out/releasing a client per entry would pin each transaction and avoid creating thousands of pools for large manifests.</violation>

<violation number="2" location="supabase/functions/_backend/triggers/on_version_update.ts:364">
P0: Manifest cleanup now fails on every row before touching R2 because PostgreSQL rejects `chr(0)` in text with `null character not permitted`. A non-NUL unambiguous encoding such as `jsonb_build_array($1, $2)::text` preserves the composite lock key.</violation>
</file>

Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

await entryPg.query('BEGIN')
// Serialize shared-hash cleanup across concurrent deleted versions.
await entryPg.query(
`SELECT pg_advisory_xact_lock(hashtextextended($1 || chr(0) || $2, 0))`,

@cubic-dev-ai cubic-dev-ai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0: Manifest cleanup now fails on every row before touching R2 because PostgreSQL rejects chr(0) in text with null character not permitted. A non-NUL unambiguous encoding such as jsonb_build_array($1, $2)::text preserves the composite lock key.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/functions/_backend/triggers/on_version_update.ts, line 364:

<comment>Manifest cleanup now fails on every row before touching R2 because PostgreSQL rejects `chr(0)` in text with `null character not permitted`. A non-NUL unambiguous encoding such as `jsonb_build_array($1, $2)::text` preserves the composite lock key.</comment>

<file context>
@@ -356,45 +356,55 @@ async function deleteManifest(c: Context, record: Database['public']['Tables']['
+          await entryPg.query('BEGIN')
+          // Serialize shared-hash cleanup across concurrent deleted versions.
+          await entryPg.query(
+            `SELECT pg_advisory_xact_lock(hashtextextended($1 || chr(0) || $2, 0))`,
+            [entry.file_hash, entry.file_name],
+          )
</file context>
Suggested change
`SELECT pg_advisory_xact_lock(hashtextextended($1 || chr(0) || $2, 0))`,
`SELECT pg_advisory_xact_lock(hashtextextended(jsonb_build_array($1, $2)::text, 0))`,
Fix with cubic

async function main() {
const env = await loadEnv(ENV_FILE)
const databaseUrl = requireDbUrl(env)
const usesLocalDatabase = databaseUrl.includes('localhost') || databaseUrl.includes('127.0.0.1')

@cubic-dev-ai cubic-dev-ai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Remote database connections can silently lose TLS or certificate verification when any URL component merely contains localhost/127.0.0.1. Parse the connection URL and compare its hostname exactly against explicit loopback hosts, reusing that result in both checks.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/reclaim_deleted_version_manifests.ts, line 112:

<comment>Remote database connections can silently lose TLS or certificate verification when any URL component merely contains `localhost`/`127.0.0.1`. Parse the connection URL and compare its hostname exactly against explicit loopback hosts, reusing that result in both checks.</comment>

<file context>
@@ -95,9 +96,26 @@ async function moveToTrash(s3: S3Client, bucket: string, key: string) {
   const env = await loadEnv(ENV_FILE)
-  const db = new pg.Client({ connectionString: requireDbUrl(env), ssl: { rejectUnauthorized: false } })
+  const databaseUrl = requireDbUrl(env)
+  const usesLocalDatabase = databaseUrl.includes('localhost') || databaseUrl.includes('127.0.0.1')
+  const db = new pg.Client({
+    connectionString: databaseUrl,
</file context>
Fix with cubic

})
await response.body?.cancel()

if (response.status === 404)

@cubic-dev-ai cubic-dev-ai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: A nonexistent or misconfigured bucket also returns 404 (NoSuchBucket), so this branch can classify every key as absent and let cleanup delete DB tracking while the objects remain in the actual R2 bucket. Preserve the error code via an SDK/body-bearing probe, treating only NoSuchKey as absent and all bucket/ambiguous failures as unknown.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/functions/_backend/utils/s3.ts, line 159:

<comment>A nonexistent or misconfigured bucket also returns 404 (`NoSuchBucket`), so this branch can classify every key as absent and let cleanup delete DB tracking while the objects remain in the actual R2 bucket. Preserve the error code via an SDK/body-bearing probe, treating only `NoSuchKey` as absent and all bucket/ambiguous failures as unknown.</comment>

<file context>
@@ -141,18 +141,63 @@ function shouldUseSizeRangeFallback(size: number, headError: unknown): boolean {
+    })
+    await response.body?.cancel()
+
+    if (response.status === 404)
+      return 'absent'
+    if (response.status === 200)
</file context>
Fix with cubic

WITH candidates AS (
SELECT av.id
FROM public.app_versions AS av
WHERE av.deleted = true

@cubic-dev-ai cubic-dev-ai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Deleted versions with deleted_at IS NULL are repeatedly queued but never enter manifest cleanup because getDeletedVersionAction treats them as non-deleted. Consider backfilling deleted_at before enqueueing these rows, or handling this legacy state explicitly.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql, line 72:

<comment>Deleted versions with `deleted_at IS NULL` are repeatedly queued but never enter manifest cleanup because `getDeletedVersionAction` treats them as non-deleted. Consider backfilling `deleted_at` before enqueueing these rows, or handling this legacy state explicitly.</comment>

<file context>
@@ -55,14 +65,17 @@ BEGIN
-    ORDER BY m.app_version_id
+    SELECT av.id
+    FROM public.app_versions AS av
+    WHERE av.deleted = true
+      AND EXISTS (
+        SELECT 1
</file context>
Fix with cubic

await Promise.all(batch.map(async (entry) => {
const entryPg = getPgClient(c, false)
try {
await entryPg.query('BEGIN')

@cubic-dev-ai cubic-dev-ai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The advisory lock, reference check, delete, and commit are not guaranteed to run on one PostgreSQL session because they use Pool.query, so the lock/transaction can fail to protect the delete. Reusing one pool and checking out/releasing a client per entry would pin each transaction and avoid creating thousands of pools for large manifests.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/functions/_backend/triggers/on_version_update.ts, line 361:

<comment>The advisory lock, reference check, delete, and commit are not guaranteed to run on one PostgreSQL session because they use `Pool.query`, so the lock/transaction can fail to protect the delete. Reusing one pool and checking out/releasing a client per entry would pin each transaction and avoid creating thousands of pools for large manifests.</comment>

<file context>
@@ -356,45 +356,55 @@ async function deleteManifest(c: Context, record: Database['public']['Tables']['
-          }
+        const entryPg = getPgClient(c, false)
+        try {
+          await entryPg.query('BEGIN')
+          // Serialize shared-hash cleanup across concurrent deleted versions.
+          await entryPg.query(
</file context>
Fix with cubic

-- Start from deleted versions (bounded) and probe manifest via app_version_id index.
WITH candidates AS (
SELECT av.id
FROM public.app_versions AS av

@cubic-dev-ai cubic-dev-ai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The batch limit bounds matches, not work: once leftover manifests are sparse, each cron run may probe every deleted app_versions row. A resumable keyset cursor or manifest-driven work queue would keep inspected rows bounded as the backlog drains.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At supabase/migrations/20260722095157_sweep_deleted_version_manifests.sql, line 71:

<comment>The batch limit bounds matches, not work: once leftover manifests are sparse, each cron run may probe every deleted `app_versions` row. A resumable keyset cursor or manifest-driven work queue would keep inspected rows bounded as the backlog drains.</comment>

<file context>
@@ -55,14 +65,17 @@ BEGIN
-    GROUP BY m.app_version_id
-    ORDER BY m.app_version_id
+    SELECT av.id
+    FROM public.app_versions AS av
+    WHERE av.deleted = true
+      AND EXISTS (
</file context>
Fix with cubic

Comment thread .gitignore
cli-helper/npm/*/CapgoKeychainHelper.app
cli-helper/npm/*/CapgoAscKeyHelper.app
.tinbase/
.context/

@cubic-dev-ai cubic-dev-ai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The new broad .context/ rule subsumes the existing .context/visual-diff entry. Removing the narrower entry would keep the ignore list free of redundant patterns.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .gitignore, line 117:

<comment>The new broad `.context/` rule subsumes the existing `.context/visual-diff` entry. Removing the narrower entry would keep the ignore list free of redundant patterns.</comment>

<file context>
@@ -114,3 +114,4 @@ cli-helper/dist/
 cli-helper/npm/*/CapgoKeychainHelper.app
 cli-helper/npm/*/CapgoAscKeyHelper.app
 .tinbase/
+.context/
</file context>
Fix with cubic

@riderx
riderx merged commit 4141338 into main Jul 22, 2026
42 of 60 checks passed
@riderx
riderx deleted the fix/manifest-deleted-cleanup branch July 22, 2026 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant