Fix metric stream sink deletes and move table to ingest - #1356
Merged
Merged
Conversation
The Redpanda sink was stuck retrying ALTER UPDATE on version keys, blocking HR data from reaching strain and activity analytics. Co-authored-by: Cursor <cursoragent@cursor.com>
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? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughMoves the ClickHouse Changesmetric_stream ingest schema migration
Stryker ignore pattern updates
Sequence Diagram(s)sequenceDiagram
participant MetricStreamProducer
participant clickhouse_sink as clickhouse-sink.ts
participant ingest_metric_stream as ingest.metric_stream (ReplacingMergeTree)
participant migration_0034 as migration 0034
participant legacy_table as postgres_fitness.metric_stream (legacy)
MetricStreamProducer->>clickhouse_sink: event
clickhouse_sink->>ingest_metric_stream: INSERT INTO ingest.metric_stream<br/>(ingested_at, is_deleted=0, version=0)
MetricStreamProducer->>clickhouse_sink: delete scope
clickhouse_sink->>ingest_metric_stream: INSERT INTO SELECT FROM FINAL<br/>WHERE is_deleted=0<br/>SET is_deleted=1, version=greatest(version+1, delete_version)
migration_0034->>legacy_table: SELECT count() FROM system.tables
legacy_table-->>migration_0034: count > 0
migration_0034->>ingest_metric_stream: INSERT INTO SELECT FROM legacy<br/>(_peerdb_synced_at AS ingested_at, _peerdb_is_deleted AS is_deleted, _peerdb_version AS version)
migration_0034->>legacy_table: DROP TABLE IF EXISTS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. |
The metric stream bootstrap no longer adds PeerDB metadata columns via ALTER TABLE. Co-authored-by: Cursor <cursoragent@cursor.com>
Add the ingest database to isolated ClickHouse test bootstrap objects, guard optional query in migration 0034, and tighten sink tombstone test assertions. Co-authored-by: Cursor <cursoragent@cursor.com>
Update test expectations and Biome formatting for the ingest database rename, and make migration 0034 tolerate legacy table count query mocks. Co-authored-by: Cursor <cursoragent@cursor.com>
Align integration test seeds and dbt microbatch event_time with ingest.metric_stream column names, and exclude agent skill symlinks from Stryker sandboxes. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve Stryker ignorePatterns conflicts by keeping both the stripe-projects exclusion from main and the local tool-directory ignores. Co-authored-by: Cursor <cursoragent@cursor.com>
Asherlc
enabled auto-merge (squash)
June 24, 2026 01:24
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/db/clickhouse-read-models.ts (1)
854-1034: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit this file to satisfy the 1000-line rule.
Line 1034 shows
clickhouse-read-models.tsexceeds the project limit. Move a large SQL builder, such as provider stats, into a focused module before growing this file further. As per coding guidelines, “Max 1000 lines per file.” As per path instructions, “Files must stay under 1000 lines.”🤖 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/db/clickhouse-read-models.ts` around lines 854 - 1034, The buildProviderStatsReadModelSql function is contributing to the clickhouse-read-models.ts file exceeding the 1000-line limit. Extract this function into a new dedicated file (for example, a separate module focused on provider stats SQL generation), then import and use it in clickhouse-read-models.ts. This will reduce the size of the current file and comply with the project's file size guidelines.Sources: Coding guidelines, Path instructions
packages/server/src/repositories/heart-rate-repository.ts (1)
41-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale deletion-column note.
The query now filters
is_deleted = 0, but the method comment still documents_peerdb_is_deleted = 0.Proposed fix
- * version-deduplicated (`FINAL` + `_peerdb_is_deleted = 0`) but NOT collapsed + * version-deduplicated (`FINAL` + `is_deleted = 0`) but NOT collapsed🤖 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 `@packages/server/src/repositories/heart-rate-repository.ts` around lines 41 - 48, The JSDoc comment for the heart rate repository method documents filtering by `_peerdb_is_deleted = 0`, but the actual implementation now uses `is_deleted = 0`. Update the documentation comment to reflect the current deletion-column name being used in the query, replacing the reference to `_peerdb_is_deleted` with `is_deleted` to keep the documentation accurate and consistent with the implementation.packages/server/src/repositories/provider-detail-repository.ts (1)
328-328: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winOmit
recordIdinstead of passing an empty-string sentinel.When
recordIdis absent, the SQL does not include{recordId:String}, so the params object should omit the key rather than pass"". As per coding guidelines, “No empty strings as absent values.” As per path instructions, “Prefernull/undefinedover empty strings for absent values.”Proposed fix
- recordId: recordId ?? "", + ...(recordId ? { recordId } : {}), limit, offset,Apply the same change in both parameter objects.
Also applies to: 378-378
🤖 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 `@packages/server/src/repositories/provider-detail-repository.ts` at line 328, The current implementation at line 328 uses an empty string as a sentinel value when recordId is absent, which violates the coding guideline that prefers null or undefined over empty strings for absent values. Instead of using the null coalescing operator to provide an empty string fallback, conditionally include the recordId property in the params object only when recordId has a value, otherwise omit the key entirely. Apply this same change to both parameter objects referenced in the diff at lines 328 and 378.Sources: Coding guidelines, Path instructions
src/providers/garmin/sync-step-plan.ts (1)
147-170: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not hide failures from the new metric-stream query.
If
ingest.metric_streamis missing or unhealthy, this catch logs and returns an empty date set, so sync proceeds with false state. Report the exception and let the provider sync surface it throughSyncResult.errorsinstead of swallowing it. As per coding guidelines, “Always report errors to Sentry” and “UseSyncResultto report successes and failures in provider sync. Never swallow API errors.” As per path instructions, “Never silently swallow errors — every unexpectedcatchmust callcaptureException().”🤖 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/providers/garmin/sync-step-plan.ts` around lines 147 - 170, The catch block in the metric-stream query is silently swallowing errors by logging them and returning an empty Set, which prevents failures from being properly reported. Replace the logger.warn call with a captureException() call to report the error to Sentry, and instead of returning an empty Set, rethrow the error or return a rejection so that the sync failure is properly surfaced through the SyncResult.errors mechanism for the provider sync.Sources: Coding guidelines, Path instructions
🤖 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 `@packages/server/src/repositories/heart-rate-repository.integration.test.ts`:
- Around line 15-16: Extract the duplicated SeedRow type definition and seed()
function from heart-rate-repository.integration.test.ts into a new
test-helpers.ts file in the same packages/server/src/repositories/ directory.
Move the identical SeedRow interface and seed() function setup code that is
duplicated between heart-rate-repository.integration.test.ts and
provider-detail-repository.integration.test.ts into the shared test-helpers.ts
file, then import these utilities from test-helpers.ts in both integration test
files to eliminate duplication and prevent future schema drift.
In `@packages/server/src/repositories/heart-rate-repository.ts`:
- Around line 64-67: In the heart-rate-repository.ts file, the query filtering
on ingest.metric_stream uses an incorrect type specification for the userId
parameter. Change the placeholder from {userId:String} to {userId:UUID} in the
WHERE clause to match the actual UUID column type of user_id in the
ingest.metric_stream table and maintain consistency with all other queries
reading this table.
In `@scripts/seed-review-clickhouse.ts`:
- Line 154: The buildCopyStatement function currently hardcodes metadata column
names (_peerdb_synced_at, _peerdb_is_deleted, _peerdb_version) for all table
copies, but ingest.metric_stream uses different names (ingested_at, is_deleted,
version). Refactor the ReviewRawTableCopy interface to include metadataColumns
and metadataExpressions fields. Update buildCopyStatement to use these fields
from the table copy configuration instead of hardcoding the default _peerdb_*
names. Then modify each table copy entry to specify its metadata columns:
postgres_fitness.* entries should use the default _peerdb_* names, while the
ingest.metric_stream entry should specify its custom column names (ingested_at,
is_deleted, version).
In `@src/db/clickhouse-migrations/0034_move_metric_stream_to_ingest.ts`:
- Around line 10-27: Replace the manual type checking in isLegacyTableCountRow
and the fallback-to-zero behavior in parseLegacyTableCount with Zod schema
validation. Define a Zod schema that validates the expected shape of the count
payload with a field "count" that is a string, then use this schema to parse and
validate the rows array in parseLegacyTableCount. When the payload does not
match the expected schema, throw an error instead of returning 0. Apply the same
Zod validation approach to the other affected parsing functions in the migration
file (around lines 41-42 and 75-81) to ensure all data crossing the runtime
boundary is properly validated and fails fast rather than silently continuing
with invalid data.
---
Outside diff comments:
In `@packages/server/src/repositories/heart-rate-repository.ts`:
- Around line 41-48: The JSDoc comment for the heart rate repository method
documents filtering by `_peerdb_is_deleted = 0`, but the actual implementation
now uses `is_deleted = 0`. Update the documentation comment to reflect the
current deletion-column name being used in the query, replacing the reference to
`_peerdb_is_deleted` with `is_deleted` to keep the documentation accurate and
consistent with the implementation.
In `@packages/server/src/repositories/provider-detail-repository.ts`:
- Line 328: The current implementation at line 328 uses an empty string as a
sentinel value when recordId is absent, which violates the coding guideline that
prefers null or undefined over empty strings for absent values. Instead of using
the null coalescing operator to provide an empty string fallback, conditionally
include the recordId property in the params object only when recordId has a
value, otherwise omit the key entirely. Apply this same change to both parameter
objects referenced in the diff at lines 328 and 378.
In `@src/db/clickhouse-read-models.ts`:
- Around line 854-1034: The buildProviderStatsReadModelSql function is
contributing to the clickhouse-read-models.ts file exceeding the 1000-line
limit. Extract this function into a new dedicated file (for example, a separate
module focused on provider stats SQL generation), then import and use it in
clickhouse-read-models.ts. This will reduce the size of the current file and
comply with the project's file size guidelines.
In `@src/providers/garmin/sync-step-plan.ts`:
- Around line 147-170: The catch block in the metric-stream query is silently
swallowing errors by logging them and returning an empty Set, which prevents
failures from being properly reported. Replace the logger.warn call with a
captureException() call to report the error to Sentry, and instead of returning
an empty Set, rethrow the error or return a rejection so that the sync failure
is properly surfaced through the SyncResult.errors mechanism for the provider
sync.
🪄 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: 7c2ffb23-2965-4f75-8859-670d6fd3f3e0
📒 Files selected for processing (37)
analytics/models/read_models/activity_location_sample.sqlanalytics/models/read_models/provider_stats.sqlanalytics/models/read_models/read_model_microbatch.sql.test.tsanalytics/models/sources.ymlanalytics/models/staging/sensor_scalar_sample.sqlpackages/server/src/repositories/heart-rate-repository.integration.test.tspackages/server/src/repositories/heart-rate-repository.tspackages/server/src/repositories/provider-detail-repository.integration.test.tspackages/server/src/repositories/provider-detail-repository.test.tspackages/server/src/repositories/provider-detail-repository.tspackages/server/src/routers/clickhouse-integration-test-helpers.test.tspackages/server/src/routers/clickhouse-integration-test-helpers.tspackages/server/src/routers/provider-detail.test.tsscripts/catch-up-clickhouse-metric-stream.test.tsscripts/catch-up-clickhouse-metric-stream.tsscripts/seed-review-clickhouse.test.tsscripts/seed-review-clickhouse.tssrc/db/clickhouse-activity-summary.test.tssrc/db/clickhouse-deduped-sensor.tssrc/db/clickhouse-metric-stream-bootstrap.tssrc/db/clickhouse-migrations.test.tssrc/db/clickhouse-migrations/0034_move_metric_stream_to_ingest.test.tssrc/db/clickhouse-migrations/0034_move_metric_stream_to_ingest.tssrc/db/clickhouse-migrations/custom-runs.tssrc/db/clickhouse-migrations/registry.tssrc/db/clickhouse-migrations/statement-runner.tssrc/db/clickhouse-read-models.tssrc/db/clickhouse-sql-helpers.tssrc/db/clickhouse.test.tssrc/db/clickhouse.tssrc/metric-stream/clickhouse-sink.integration.test.tssrc/metric-stream/clickhouse-sink.test.tssrc/metric-stream/clickhouse-sink.tssrc/metric-stream/clickhouse-table.tssrc/providers/garmin/sync-step-plan.tsstryker.ci.config.jsonstryker.config.json
💤 Files with no reviewable changes (1)
- src/db/clickhouse-sql-helpers.ts
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Align ClickHouse migration mocks with the new count-field Zod validation and mock ClickHouse in Garmin tests so CI does not attempt real connections. Co-authored-by: Cursor <cursoragent@cursor.com>
Accept numeric legacy table counts from ClickHouse, fix Biome import order, and bootstrap ingest.metric_stream in Garmin integration tests when CLICKHOUSE_URL is set. Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve provider-detail metric stream conflicts by using ingest.metric_stream with row_number() version dedup from main. Co-authored-by: Cursor <cursoragent@cursor.com>
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
The Redpanda metric-stream ClickHouse sink was stuck retrying
ALTER TABLE ... UPDATEon_peerdb_version, leaving ~17M backlog messages and stale heart-rate data for strain and activity HR zones. Deletes now write tombstone rows viaINSERT...SELECTwith bumpedversion, which ReplacingMergeTree can apply without mutating key columns. Raw sensor samples move frompostgres_fitness.metric_streamtoingest.metric_streamwithingested_at/is_deleted/versioncolumn names; migration0034copies legacy data and drops the old table. dbt sources, read models, bootstrap SQL, catch-up scripts, and server repositories were updated to read the new table.Test plan
clickhouse-sink.test.tsandclickhouse-migrations.test.tsread_model_microbatch.sql.test.tsmetric-stream-clickhouse-sink, confirm consumer lag drainsMade with Cursor
Summary by cubic
Fixes stuck deletes in the metric‑stream ClickHouse sink by switching to tombstone inserts and migrates the mirror to
ingest.metric_streamwith ingest metadata. This drains the backlog, restores heart‑rate analytics, and retires the legacy table.Bug Fixes
ALTER ... UPDATEdeletes with tombstoneINSERT ... SELECTthat bumpversionsoReplacingMergeTreeapplies deletes safely; tightened delete‑scope assertions and tests.ingest.metric_stream; centralizedINGEST_DATABASE/METRIC_STREAM_TABLE; updated ClickHouse waits/smoke tests, test bootstraps, and catch‑up/seed scripts for ingest columns; provider‑detail now readsingest.metric_streamwith row_number() version dedup; stabilized CI by mocking ClickHouse in Garmin tests and tolerating legacy table count mocks.ingest.metric_streamwhenCLICKHOUSE_URLis set; minor lint/import‑order fixes.Migration
ingest.metric_streamwithReplacingMergeTree(version); columns_peerdb_synced_at→ingested_at,_peerdb_is_deleted→is_deleted,_peerdb_version→version. Bootstrap creates this table directly (noALTERadd‑columns).0034_move_metric_stream_to_ingest: createsingest, copies frompostgres_fitness.metric_streamwhen present (legacy existence checked viasystem.tableswith count parsed from string or number), then drops the legacy table. Adoption: run ClickHouse migrations, redeploy/restart the metric‑stream sink, rebuild dbt sensor models.Written for commit 95d037c. Summary will update on new commits.
Summary by CodeRabbit
Release Notes