Repository navigation
fix: prune scoped metric deletes and enforce sample identity - #3235
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 16 minutes. View limit details
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. |
Reviewer's GuideData structuresRemove caller-supplied IDs from the metric-stream row input and always derive the deterministic sample identity for newly published events, while preserving historical wire-event parsing.
APIRemove the optional ID from the writer insert interface so new callers cannot supply an identity that bypasses deterministic ID derivation.
ImplementationDiscover deletion candidates by immutable identity tuples, resolve complete versions, and then apply mutable scope predicates to each latest row without adding runtime tuning or changing acknowledgement behavior.
ConfigurationRestrict the Docker
DocumentationRecord the completed history-only recovery, its bounded health observations, and explicit limits on what those observations establish.
Preserve the deletion-candidate identity audit and its frozen query as historical evidence, not as authorization to rerun it.
Retain the frozen comparison and summary documenting the historical identity precondition and retained-version audit.
Preserve the local candidate-selection qualification and implementation verification with their stated assumptions and scope limits.
Document the extraction from the superseded draft, dispositions for its 728 paths, sibling ownership boundaries, deferred experiments, and review follow-up.
TestsExercise pruning and scoped-deletion semantics on ClickHouse, including mutable corrections, mixed scopes, timestamp precision, and acknowledgement behavior.
Update sink integration fixtures and regressions to retain historical wire IDs while checking latest-row behavior and deletion chunk boundaries.
Align sink query assertions with immutable identity tuple membership and latest-row scope checks.
Test deterministic publication identity, rejection of caller-provided IDs, and continued preservation of historical wire identity and timestamp precision.
Remove caller-supplied IDs from new consumer fixtures while leaving historical wire-ID coverage intact.
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Storybook previews for This comment updates automatically on each PR push. |
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Mergestorm reviews this repository's pull requestsEvery PR gets AI code review that flags real bugs before they merge. This account's 30 free credits for this monthly window are used, so reviews are paused at The Starter plan includes 120 credits a month. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3235 +/- ##
==========================================
- Coverage 97.41% 97.41% -0.01%
==========================================
Files 1124 1124
Lines 61626 61625 -1
Branches 19092 19091 -1
==========================================
- Hits 60032 60031 -1
Misses 1548 1548
Partials 46 46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/metric-stream/clickhouse-sink.ts" line_range="679-683" />
<code_context>
- )
- WHERE ${correlatedLatestPredicates}
+ WHERE (metric_stream_row.id, metric_stream_row.user_id,
+ metric_stream_row.provider_id, metric_stream_row.recorded_at) IN (
+ SELECT id, user_id, provider_id, recorded_at FROM (
+ ${scopedCandidates}
+ )
+ )
+ GROUP BY metric_stream_row.id
</code_context>
<issue_to_address>
**Out-of-scope samples get deleted**
When a retained wire ID has versions with different user, provider, or recorded-time values, and an older version matches the delete scope while the latest does not, `scopedCandidates` limits the versions passed to `argMax`, so it can select the older matching row and exclude the actual latest row. The sink writes a tombstone based on that stale row, and the caller sees the current out-of-scope sample deleted.
Run `argMax` across every version of each candidate ID before applying the delete-scope predicate to the latest row.
Also at `src/metric-stream/clickhouse-sink.integration.test.ts:373`.
</issue_to_address>
### Comment 2
<location path="src/metric-stream/clickhouse-sink.ts" line_range="684" />
<code_context>
+ )
+ GROUP BY metric_stream_row.id
)
- WHERE latest_row.14 = 0
+ WHERE (${latestScopePredicate})
+ AND latest_row.14 = 0
</code_context>
<issue_to_address>
**Unrelated sample versions get deleted**
When a batch contains multiple scopes and a sample's older mutable fields match one scope while its latest row matches another, the sink can retain a sample through one candidate scope, then accept its latest row through a different scope because `latestScopePredicate` is an uncorrelated OR. It writes a tombstone even though no single scope matched both versions, so the caller sees data deleted unexpectedly.
Preserve the association between each candidate scope and its latest-row predicate when resolving identities.
</issue_to_address>
### Comment 3
<location path=".github/workflows/build-docker.yml" line_range="42" />
<code_context>
tags: |
type=sha,value=${{ github.sha }}
- type=raw,value=latest
+ type=raw,value=latest,enable={{is_default_branch}}
- name: Compute web asset base URL
id: web-assets
</code_context>
<issue_to_address>
**Non-default builds replace latest**
When an ML or Deploy build runs from a non-default branch, including through manual dispatch, the updated `build-docker.yml` tag is conditional, but the ML and Deploy metadata entries still emit an unconditional `latest` tag. Those builds overwrite the shared tag, so users pulling `latest` receive the non-default-branch image.
Make the `latest` tag conditional on the default branch in every build workflow and metadata entry.
</issue_to_address>
### Comment 4
<location path="src/metric-stream/clickhouse-sink.ts" line_range="603" />
<code_context>
+ (conditions) => `SELECT candidate_row.id AS id, candidate_row.user_id AS user_id,
+ candidate_row.provider_id AS provider_id, candidate_row.recorded_at AS recorded_at
FROM ${METRIC_STREAM_TABLE} AS candidate_row
WHERE ${conditions.join(" AND ")}
- GROUP BY candidate_row.id`,
+ GROUP BY candidate_row.id, candidate_row.user_id,
</code_context>
<issue_to_address>
**Activity-only deletes scan broadly**
When a deletion scope has `activityId` but no `userId`, `clickHouseDeleteScopeConditions` permits scopes containing only `activityId`, so this candidate query filters on `activity_id` without the leading `user_id` key. The table is ordered by `(user_id, activity_id, channel, recorded_at, id)`, so ClickHouse cannot prune by a primary-key prefix for that scope and reads broadly across the table, risking another slow delete.
Add an access path suitable for activity-only candidate lookup, or require a leading-key scope before running this query.
</issue_to_address>
### Comment 5
<location path="src/metric-stream/clickhouse-sink.integration.test.ts" line_range="140-217" />
<code_context>
await client.close?.();
});
+ it("prunes unrelated identities while applying a scoped deletion", async () => {
+ const userId = randomUUID();
+ const otherUserId = randomUUID();
+ const activityId = randomUUID();
+ const queryId = randomUUID();
+ const fixtureRows = 65_536;
+ try {
+ await client.command({
+ query: `INSERT INTO ${METRIC_STREAM_TABLE}
+ (id, user_id, activity_id, provider_id, recorded_at, channel, scalar)
+ SELECT reinterpretAsUUID(MD5(concat(toString({userId:UUID}), toString(number)))),
+ if(number % 16 = 0, {userId:UUID}, {otherUserId:UUID}),
+ {activityId:UUID}, 'identity-pruning-test',
+ toDateTime64('2026-06-08 12:00:00', 6), 'heart_rate', 72
+ FROM numbers(${fixtureRows})`,
+ query_params: { userId, otherUserId, activityId },
+ });
+ await markMetricStreamScopesDeletedInClickHouse(
+ {
</code_context>
<issue_to_address>
**Integration test file exceeds limit**
The added integration scenarios leave `clickhouse-sink.integration.test.ts` at 1,131 lines, exceeding the repository’s 1,000-line ceiling and leaving the sink coverage in an oversized test file.
Split the added integration scenarios into focused `*.integration.test.ts` files so each remains below 1,000 lines.
Also at `src/metric-stream/clickhouse-sink.integration.test.ts:441-599`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and if the identity tuple or scope matching is wrong, a deletion event could write tombstones for metric rows that should remain live. Reverting stops the new selection logic but does not remove tombstones already written; affected rows would need explicit repair.
Blocking findings: src/metric-stream/clickhouse-sink.ts:683, src/metric-stream/clickhouse-sink.ts:684, .github/workflows/build-docker.yml:42, src/metric-stream/clickhouse-sink.ts:603, src/metric-stream/clickhouse-sink.integration.test.ts:217
|
🤖 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. |
|
🤖 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. |
|
Addressed all five items in Sourcery review, with individual evidence replies in each thread. a04b1dcc8 guards the remaining three latest-tag entries, splits the integration file below the line limit, and documents the activity-only candidate-scan boundary. The approved immutable identity contract and same-revision scope-union semantics are retained; their counterexamples and requested strategy changes are explained in the corresponding threads and extraction record. All 20 integration tests pass after the split on ClickHouse26.6.1.1193, retries disabled. Root typecheck, Biome and diff checks pass. CI on the new head is pending; previous head passed all103 checks. No production changes. |
Scoped metric-stream deletion currently joins candidate IDs against unrelated physical versions across the table. Resolve complete versions through immutable
(id,user_id,provider_id,recorded_at)membership, then recheck mutable fields on the latest full row. Enforce the existing deterministic ID at new-publication ingress while retaining historical wire IDs and main’s Redpanda contracts.Also restrict Docker
latesttagging to the default branch. Preserve curated history-recovery/identity evidence and provide an explicit disposition for all 728 frozen paths from superseded #3231. Broker infrastructure belongs to #3233; account erasure and required analytics/schema dependencies belong to #3234. Deferred transport/marker-v2 prototypes remain at the immutable source rather than being imported wholesale.Validation:
The fixture proves pruning and semantic regressions, not a universal production scan bound. Historical receipts are dated observations with their original source revisions. No timeout/retry or optimizer tuning was added.
Summary by cubic
Fixes scoped metric-stream deletion so it no longer joins candidate IDs against unrelated physical versions across the table. The sink now resolves complete versions through immutable
(id,user_id,provider_id,recorded_at)membership before rechecking mutable scope fields on the latest row. Also enforces the deterministic ID at publication ingress while retaining historical wire IDs, and restricts Dockerlatesttagging to the default branch.Written for commit a04b1dc. Summary will update on new commits.
Summary by Sourcery
Correct scoped metric-stream deletion identity handling and publication ID validation while preserving historical compatibility and tightening release evidence.
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: