[server] Fix ClickHouse provider deletion acknowledgements - #1676
Merged
Merged
Conversation
Provider deletion events could be consumed without writing tombstones, leaving analytics stale and retry scans saturating ClickHouse. Record applied delete event IDs after scoped tombstone inserts so refresh jobs can await durable completion.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
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. |
|
The PR successfully implements a more robust acknowledgement mechanism for metric stream deletions in ClickHouse by using a dedicated acknowledgement table instead of polling for row counts. This avoids the overhead and potential unreliability of The refactoring of I've noted one critical issue regarding backward compatibility for in-flight jobs in the queue and a performance nit regarding ClickHouse index usage. 🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Contributor
Reviewer's GuideImplements durable, versioned ClickHouse tombstoning for metric-stream deletions and introduces an acknowledgement path and ClickHouse query guardrails so provider-delete analytics jobs wait on a small receipt table instead of re-scanning the large metric_stream table, along with tests, migrations, and documentation updates. Sequence diagram for provider delete with ClickHouse deletion acknowledgementsequenceDiagram
actor User
participant ProviderDetailRouter as providerDetailRouter
participant MetricStreamWriter as replaceMetricStreamBatch
participant Redpanda as MetricStreamEventPublisher
participant ClickHouseSink as applyMetricStreamEventsToClickHouse
participant MetricStream as ingest_metric_stream
participant DeleteAck as ingest_metric_stream_delete_acknowledgement
participant JobQueue as enqueueProviderDeleteAnalyticsRefresh
participant AnalyticsJob as processActivityDeleteAnalyticsJob
participant ReadModel as waitForMetricStreamDeleteAcknowledgement
User->>ProviderDetailRouter: request provider-data-delete
ProviderDetailRouter->>MetricStreamWriter: replaceMetricStreamBatch(scope, rows, sourceType)
MetricStreamWriter->>Redpanda: replaceRows(scope, rows)
Redpanda-->>MetricStreamWriter: MetricStreamReplacementPublishResult(deleted.eventId, rows)
MetricStreamWriter-->>ProviderDetailRouter: MetricStreamReplacementReceipt(deletedEventId)
ProviderDetailRouter->>JobQueue: enqueueProviderDeleteAnalyticsRefresh(userId, providerId, deletedEventId)
Redpanda->>ClickHouseSink: MetricStreamRedpandaEvent(metric_stream_deleted, eventId)
ClickHouseSink->>ClickHouseSink: applyMetricStreamEventsToClickHouse(client, events)
ClickHouseSink->>MetricStream: markMetricStreamScopeDeletedInClickHouse(client, scope)
ClickHouseSink->>DeleteAck: acknowledgeMetricStreamDeletion(client, eventId)
JobQueue->>AnalyticsJob: processActivityDeleteAnalyticsJob(job)
AnalyticsJob->>ReadModel: waitForMetricStreamDeleteAcknowledgement(client, metricStreamDeleteEventId)
loop until acknowledgement_count > 0
ReadModel->>DeleteAck: SELECT count() AS acknowledgement_count
end
ReadModel-->>AnalyticsJob: acknowledgement observed
AnalyticsJob->>AnalyticsJob: rebuild ClickHouse models and invalidate cache
Entity-relationship diagram for metric_stream deletion acknowledgement tableerDiagram
ingest_metric_stream {
UUID id
Int64 version
UInt8 is_deleted
}
ingest_metric_stream_delete_acknowledgement {
UUID event_id
DateTime64 applied_at
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughMetric stream deletions now emit version 2 events with IDs, write ClickHouse tombstones and acknowledgement receipts, propagate those IDs through provider-delete jobs, and poll acknowledgements before analytics refresh. ClickHouse profiles also add bounded query execution and disconnect cancellation. ChangesMetric stream deletion flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderDeletion
participant MetricStreamWriter
participant Redpanda
participant ClickHouseSink
participant AnalyticsJob
ProviderDeletion->>MetricStreamWriter: replaceMetricStreamBatch
MetricStreamWriter->>Redpanda: publish version 2 deletion event
Redpanda->>ClickHouseSink: deliver eventId
ClickHouseSink->>ClickHouseSink: write tombstone and acknowledgement
AnalyticsJob->>ClickHouseSink: poll acknowledgement
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
metricStreamDeleteEventIdfield onProviderDeleteAnalyticsJobDatais required, but existing queuedprovider-delete-analytics-refreshjobs in Redis won’t have it; consider handlingundefinedinprocessActivityDeleteAnalyticsJob(e.g., falling back to the old wait logic or skipping the ClickHouse acknowledgement wait) to avoid deploy-time failures. - In
acknowledgeMetricStreamDeletion, the error message says "ClickHouse metric-stream replacement requires a command-capable client" even though it’s called from the deletion path; updating the wording to mention deletion/acknowledgement would make failures easier to interpret.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `metricStreamDeleteEventId` field on `ProviderDeleteAnalyticsJobData` is required, but existing queued `provider-delete-analytics-refresh` jobs in Redis won’t have it; consider handling `undefined` in `processActivityDeleteAnalyticsJob` (e.g., falling back to the old wait logic or skipping the ClickHouse acknowledgement wait) to avoid deploy-time failures.
- In `acknowledgeMetricStreamDeletion`, the error message says "ClickHouse metric-stream replacement requires a command-capable client" even though it’s called from the deletion path; updating the wording to mention deletion/acknowledgement would make failures easier to interpret.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
|
[Review Note] File: Nit: Wrapping (And similarly for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@src/analytics/activity-read-model-build.ts`:
- Around line 122-130: Update the query result handling in the
acknowledgement-count flow to validate the JSONEachRow response with Zod before
reading acknowledgement_count. Replace reliance on the client.query generic type
with a Zod schema that parses the returned rows, then preserve the existing
numeric fallback and Number conversion for a missing or invalid count according
to the established behavior.
In `@src/metric-stream/clickhouse-sink.integration.test.ts`:
- Around line 109-115: Replace the hardcoded table name in the acknowledgement
count query with the exported METRIC_STREAM_DELETE_ACKNOWLEDGEMENT_TABLE
constant, and update the clickhouse-table.ts import to include it alongside the
existing table constant.
In `@src/metric-stream/clickhouse-sink.ts`:
- Around line 174-200: The metric query applies scope predicates before
selecting the latest version, allowing stale scoped rows to represent an ID.
Update the query around the latest_row subquery to identify candidate IDs using
the scope conditions, select argMax for each candidate across all versions, then
apply the scope predicates to the resulting latest tuple before excluding
tombstones. Add a real ClickHouse integration fixture covering an older matching
version followed by a newer out-of-scope version, and verify deleting the old
scope preserves the newer row.
In `@src/metric-stream/events.ts`:
- Around line 108-120: Update the eventId validator in
metricStreamDeletedEventV2Schema from z.guid() to z.uuid(), preserving the
existing schema structure and strict object validation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4a3964a8-21ea-433d-bca8-e976d48768d6
📒 Files selected for processing (29)
analytics/profiles.ymldeploy/README.mddeploy/clickhouse/users.d/allow-experimental-nullable-tuple-type.xmldeploy/clickhouse/users.d/default-query-guardrails.xmldeploy/stack.ymldocs/clickhouse-metric-stream.mddocs/production-incident-baseline.mdpackages/server/src/routers/provider-detail.test.tspackages/server/src/routers/provider-detail.tsscripts/backfill-ride-with-gps-track-points.tssrc/analytics/activity-read-model-build.test.tssrc/analytics/activity-read-model-build.tssrc/db/clickhouse-metric-stream-bootstrap.tssrc/db/clickhouse-migrations/0045_metric_stream_delete_acknowledgement.tssrc/db/clickhouse-migrations/registry.test.tssrc/db/clickhouse-migrations/registry.tssrc/db/metric-stream-writer.test.tssrc/db/metric-stream-writer.tssrc/jobs/process-activity-delete-analytics-job.test.tssrc/jobs/process-activity-delete-analytics-job.tssrc/jobs/queues.test.tssrc/jobs/queues.tssrc/metric-stream/clickhouse-sink.integration.test.tssrc/metric-stream/clickhouse-sink.test.tssrc/metric-stream/clickhouse-sink.tssrc/metric-stream/clickhouse-table.tssrc/metric-stream/events.test.tssrc/metric-stream/events.tssrc/metric-stream/redpanda-producer.ts
💤 Files with no reviewable changes (1)
- deploy/clickhouse/users.d/allow-experimental-nullable-tuple-type.xml
|
🤖 Review aborted: the PR is too complex or took too long to analyze. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
[Review Note] File: Why is the acknowledgement wait only implemented for provider deletes and not for activity deletes? If an activity is deleted, its metrics are also removed from ClickHouse. If the analytics rebuild starts before those tombstones are applied, the resulting analytics might still include the deleted activity's metrics. Consider adding |
Validate deletion acknowledgements at the database boundary and tighten deletion event UUIDs.
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
🤖 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
changed the title
Owner
Author
|
Review follow-up:
|
Owner
Author
|
@CodeRabbit full rereview |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 12 minutes. |
Owner
Author
|
@CodeRabbit full rereview |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/metric-stream/clickhouse-sink.ts`:
- Around line 146-149: Update the scope condition logic around
metricStreamDeleteScopeSchema handling so externalId: null emits a ClickHouse IS
NULL predicate without binding external_id, while defined non-null values retain
the existing equality predicate and parameter. Add a minimal ClickHouse
integration test covering deletion of rows with external_id IS NULL, following
the repository’s database-semantic test requirements.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8742e89c-adc1-481d-9eaf-5d701c92ec19
📒 Files selected for processing (29)
analytics/profiles.ymldeploy/README.mddeploy/clickhouse/users.d/allow-experimental-nullable-tuple-type.xmldeploy/clickhouse/users.d/default-query-guardrails.xmldeploy/stack.ymldocs/clickhouse-metric-stream.mddocs/production-incident-baseline.mdpackages/server/src/routers/provider-detail.test.tspackages/server/src/routers/provider-detail.tsscripts/backfill-ride-with-gps-track-points.tssrc/analytics/activity-read-model-build.test.tssrc/analytics/activity-read-model-build.tssrc/db/clickhouse-metric-stream-bootstrap.tssrc/db/clickhouse-migrations/0045_metric_stream_delete_acknowledgement.tssrc/db/clickhouse-migrations/registry.test.tssrc/db/clickhouse-migrations/registry.tssrc/db/metric-stream-writer.test.tssrc/db/metric-stream-writer.tssrc/jobs/process-activity-delete-analytics-job.test.tssrc/jobs/process-activity-delete-analytics-job.tssrc/jobs/queues.test.tssrc/jobs/queues.tssrc/metric-stream/clickhouse-sink.integration.test.tssrc/metric-stream/clickhouse-sink.test.tssrc/metric-stream/clickhouse-sink.tssrc/metric-stream/clickhouse-table.tssrc/metric-stream/events.test.tssrc/metric-stream/events.tssrc/metric-stream/redpanda-producer.ts
💤 Files with no reviewable changes (1)
- deploy/clickhouse/users.d/allow-experimental-nullable-tuple-type.xml
|
The PR introduces a robust acknowledgement mechanism for metric stream deletions, effectively resolving potential race conditions and visibility issues in ClickHouse. Highlights:
The implementation is clean and follows ClickHouse best practices for handling 🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does. |
There was a problem hiding this comment.
Asherlc has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Asherlc
enabled auto-merge (squash)
July 18, 2026 05:12
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/metric-stream/clickhouse-sink.test.ts (1)
184-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the exported table name constant.
To maintain consistency and avoid hardcoding schema coordinates, use the
METRIC_STREAM_DELETE_ACKNOWLEDGEMENT_TABLEconstant instead of the raw string"ingest.metric_stream_delete_acknowledgement".♻️ Proposed refactor
expect(command).toHaveBeenNthCalledWith(2, { - query: expect.stringContaining("ingest.metric_stream_delete_acknowledgement"), + query: expect.stringContaining(METRIC_STREAM_DELETE_ACKNOWLEDGEMENT_TABLE), query_params: { event_id: deleteEvent.eventId }, });You may also need to update the import block from
./clickhouse-table.tsto includeMETRIC_STREAM_DELETE_ACKNOWLEDGEMENT_TABLEif it isn't already imported.🤖 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 `@src/metric-stream/clickhouse-sink.test.ts` around lines 184 - 190, Update the assertion for the second command in the test around firstCommandQuery to use the exported METRIC_STREAM_DELETE_ACKNOWLEDGEMENT_TABLE constant instead of the hardcoded ingest.metric_stream_delete_acknowledgement string, adding the constant to the clickhouse-table import if 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.
Outside diff comments:
In `@src/metric-stream/clickhouse-sink.test.ts`:
- Around line 184-190: Update the assertion for the second command in the test
around firstCommandQuery to use the exported
METRIC_STREAM_DELETE_ACKNOWLEDGEMENT_TABLE constant instead of the hardcoded
ingest.metric_stream_delete_acknowledgement string, adding the constant to the
clickhouse-table import if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c857ef93-9df1-409e-9466-d646f48edcb4
📒 Files selected for processing (3)
src/metric-stream/clickhouse-sink.integration.test.tssrc/metric-stream/clickhouse-sink.test.tssrc/metric-stream/clickhouse-sink.ts
This was referenced Jul 18, 2026
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
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
Validation
pnpm lintpnpm tsc --noEmit(root, server, and web)Summary by CodeRabbit
Bug Fixes
Improvements
eventId, including acknowledgement recording and event-scoped polling.Documentation
Tests