[server] Fix repeated provider data deletion scans and progress

[Asherlc]

Summary

Repeated provider deletions paginated every historical metric-stream key even when its latest ClickHouse version was already tombstoned, causing hours of full-projection scans and an unchanged UI message.

This change selects the newest (generation, id) version before filtering deleted rows, records examined-row counts, and publishes live progress rendered by web and mobile.

It adds unit and real-ClickHouse regression coverage and updates the deletion runbook and production incident baseline.

Validation: 6 processor unit tests, 4 ClickHouse integration tests, 197 surrounding tests, full lint, and root/server typechecks pass; the broader changed run passed 6,288 tests with four unrelated existing FIT-import integration failures.

---

Summary by cubic

Fixes repeated provider deletions that rescanned tombstoned metric streams by paging only live candidates and showing clear progress. Adds a live-candidate projection, upgrades ClickHouse to 26.6.1.1193 for bounded reads, and switches secret scans to checksum-pinned local gitleaks.

• Bug Fixes

  • Cursor reads only physical live rows via by_provider_live_generation, then validates each key’s latest version with by_provider_generation; skips already-deleted keys and keeps 1,000-key pages under ~16k reads.
  • Readiness requires both projections; the job fails early with materialization guidance.
  • Progress/checkpoints persist examinedRows and report “Checked X metric stream rows; deleted Y...”.

• Migration

  • New migration 0048_provider_live_generation_projection. Materialize both by_provider_generation and by_provider_live_generation on existing parts before redriving deletions.
  • Pin ClickHouse to 26.6.1.1193 in CI and production so ordered projection reads stop at the batch limit (26.3 scanned the full range).

^{Written for commit d255a5e. Summary will update on new commits.}

Summary by CodeRabbit

• Bug Fixes

  • Improved provider data deletion processing to skip metric stream records already marked as deleted.
  • Prevented repeated deletion runs from rescanning previously tombstoned records.
  • Added clearer progress reporting, including rows examined and rows deleted.
  • Improved checkpoint details so interrupted jobs can resume with more precise progress information.

• Documentation

  • Updated incident records and the deletion runbook with revised pagination, checkpoint, and troubleshooting guidance.

• Tests

  • Added coverage for repeated deletion runs and updated progress validation scenarios.

[coderabbitai]

📝 Walkthrough

Walkthrough

The provider deletion job now selects latest metric-stream versions before excluding tombstones, tracks examined rows in durable checkpoints, reports richer progress, and adds regression coverage and operational documentation.

Changes

Provider deletion processing

Layer / File(s)	Summary
Latest-version cursor selection src/jobs/queues.ts, src/jobs/process-provider-data-deletion-job.ts, src/jobs/process-provider-data-deletion-job.test.ts	The cursor selects the latest version per metric-stream key, excludes deleted source rows, and validates the updated query behavior.
Checkpoint and progress tracking src/jobs/process-provider-data-deletion-job.ts, src/jobs/process-provider-data-deletion-job.test.ts	Checkpoints now track examinedRows, and progress reports examined and deleted row counts.
Regression validation and operations documentation packages/server/src/jobs/process-provider-data-deletion-job.integration.test.ts, docs/provider-data-deletion-runbook.md, docs/production-incident-baseline.md	Integration coverage verifies repeated deletion skips already-deleted keys, while documentation records the cursor, checkpoint, and incident behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• Asherlc/dofek#1677: Updates the provider-deletion durable/outbox workflow and checkpoint semantics.
• Asherlc/dofek#1678: Modifies the same ClickHouse pagination and projection-based tombstoning flow.
• Asherlc/dofek#1679: Updates progress and checkpoint reporting for the provider deletion job.

Suggested labels: area/server, type/bug

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is imperative, under 70 characters, properly prefixed, and accurately summarizes the server-side deletion scan and progress fix.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

Storybook previews for 5c928a55 are ready:

• Web Storybook
• Mobile Storybook

This comment updates automatically on each PR push.

[codereviewbot-ai]

[Review Note] File: src/jobs/process-provider-data-deletion-job.ts Line: 284

🛠️ Filter Deleted Rows in TypeScript

Filter out the already-deleted rows in TypeScript before calling tombstoneMetricStreamBatch. This ensures we only tombstone non-deleted rows, while still advancing the checkpoint correctly past all examined rows (including deleted ones).

  let checkpoint = job.data.checkpoint;
  while (true) {
    const rows = await loadNextMetricStreamBatch(clickHouseClient, job.data, checkpoint);
    if (rows.length === 0) break;

    const rowsToDelete = rows.filter((row) => row.source_is_deleted === 0);
    const deletedRows = rowsToDelete.length > 0
      ? await tombstoneMetricStreamBatch(clickHouseClient, job.data, rowsToDelete)
      : 0;
    const lastRow = rows.at(-1);

[codereviewbot-ai]

[Review Note] File: src/jobs/process-provider-data-deletion-job.ts Line: 15

🚨 Critical Performance Issue: $O(N^2)$ Scan Bug

By wrapping the query in a subquery without a LIMIT inside the subquery, ClickHouse is forced to scan and group all unique (generation, id) pairs matching the WHERE clause (which can be millions of rows for a large user/provider) before filtering them in the outer query and applying LIMIT {batch_size}.

In a paginated loop, this results in an $O(N^2)$ scan:

• Batch 1 scans $N$ rows.
• Batch 2 scans $N - \text{batch_size}$ rows.
• Batch 3 scans $N - 2 \times \text{batch_size}$ rows.

For a user with 10,000,000 rows, this will scan over 50 billion rows in total, causing severe performance degradation, timeouts, or database crashes.

Solution

Instead of a subquery, we can query the table directly with LIMIT {batch_size} and include is_deleted AS source_is_deleted in the selected columns. We then filter out the deleted rows in TypeScript. This keeps the query extremely fast and bounded to batch_size on every iteration, while still advancing the checkpoint correctly past both deleted and non-deleted rows.

Let's update the schema first to include source_is_deleted:

const metricStreamCursorRowsSchema = z.array(
  z.object({
    generation: z.coerce.number().int().nonnegative(),
    id: z.uuid(),
    source_is_deleted: z.coerce.number().int(),
  }),
);

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[sourcery-ai]

Sorry @Asherlc, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[codereviewbot-ai]

Review Summary

The changes in this Pull Request are correct, robust, and well-tested.

💡 Note on the Subquery and Correctness

The subquery introduced in loadNextMetricStreamBatch is absolutely necessary for correctness:

1. In ClickHouse, the WHERE clause is executed before LIMIT BY.
2. If we were to filter is_deleted = 0 directly in the main query without a subquery (as suggested by the previous bot comment), ClickHouse would filter out the latest deleted version first, and then LIMIT 1 BY would select an older, non-deleted version of the key. This would incorrectly return the key as active and cause it to be paginated and tombstoned again.
3. By using the subquery, we first find the absolute latest version of each key (LIMIT 1 BY on the ordered rows), and then filter out those that are already deleted (WHERE source_is_deleted = 0). This perfectly matches the behavior verified in the new integration test: "does not paginate a metric stream key whose latest version is already deleted".

The performance is well-optimized since the query utilizes the covering projection by_provider_generation with a matching sorting key prefix.

LGTM! 🚀

---

🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[codereviewbot-ai]

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

[Asherlc]

Addressed the remaining PR-level review items.

• CodeRabbit title warning: updated the PR title to [server] Fix repeated provider data deletion scans and progress.
• Not applying the TypeScript-filtering suggestions from [server] Fix repeated provider data deletion scans and progress #1689 (comment) and [server] Fix repeated provider data deletion scans and progress #1689 (comment).
• Reason: returning deleted keys to TypeScript would force the worker to paginate and persist checkpoints for every tombstoned key, recreating the repeated-deletion production issue. Limiting before filtering can also stop before later active keys are found.
• Evidence: packages/server/src/jobs/process-provider-data-deletion-job.integration.test.ts requires a second deletion to finish without any updateData call. The later review summary at [server] Fix repeated provider data deletion scans and progress #1689 (comment) also confirms that latest-version selection must precede tombstone filtering.

[codereviewbot-ai]

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[codereviewbot-ai]

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[codereviewbot-ai]

LGTM! 🚀

This is an exceptionally well-designed and thoroughly tested Pull Request.

Key Highlights:

1. Elegant Index/Projection Optimization: Introducing the by_provider_live_generation projection with is_deleted in the sorting key—combined with the is_deleted = 0 filter—allows ClickHouse to perform a highly efficient range scan directly on live rows. This perfectly resolves the production issue of paginating over tombstoned keys.
2. Keyset Pagination Optimization: Rewriting the tuple comparison tuple(generation, id) > tuple(...) to individual column comparisons (generation > last_generation OR (generation = last_generation AND id > last_id)) is a great ClickHouse-specific optimization that allows the query planner to utilize the projection index efficiently.
3. Robust Integration Tests: The integration tests are excellent, especially the test verifying that the query reads a bounded live-candidate range by asserting on system.query_log's read_rows.

No issues found. Great job!

---

🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.