Skip to content

Commit a04b1dc

Browse files
committed
fix: address independent fixes review findings
1 parent 1144cf4 commit a04b1dc

6 files changed

Lines changed: 305 additions & 241 deletions

File tree

‎.github/workflows/build-docker-ml.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ jobs:
3939
images: ${{ steps.image-name.outputs.name }}
4040
tags: |
4141
type=sha,value=${{ github.sha }}
42-
type=raw,value=latest
42+
type=raw,value=latest,enable={{is_default_branch}}
4343
- uses: docker/build-push-action@c3c9e263c25d99ce0380d002d59b67737d91b0dc # v7.4.0
4444
with:
4545
context: packages/ml

‎.github/workflows/deploy.yml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ jobs:
9090
images: ${{ steps.tag.outputs.image }}
9191
tags: |
9292
type=sha,value=${{ github.sha }}
93-
type=raw,value=latest
93+
type=raw,value=latest,enable={{is_default_branch}}
9494
- name: Compute web asset base URL
9595
id: web-assets
9696
env:
@@ -154,7 +154,7 @@ jobs:
154154
images: ${{ steps.tag.outputs.image }}
155155
tags: |
156156
type=sha,value=${{ github.sha }}
157-
type=raw,value=latest
157+
type=raw,value=latest,enable={{is_default_branch}}
158158
- uses: docker/build-push-action@c3c9e263c25d99ce0380d002d59b67737d91b0dc # v7.4.0
159159
with:
160160
context: packages/ml

‎docs/evidence/pr-split-20261108/README.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,3 +26,13 @@ The handoff recorded RED: the old query read 77,824 rows against a required boun
2626
## Retrospective
2727

2828
The pre-existing failing regression and explicit sibling ownership kept the runtime diff small. Separating historical receipts from current instructions required the most review. For the next split, record path and hunk ownership before implementation and retain one immutable source revision. Suggested documentation improvement: add that sentence to the PR splitting guidance. Reuse `integration-tests-ready` and `verification-before-completion`; no custom validator or new skill is needed.
29+
30+
## Review follow-up
31+
32+
The [Sourcery review](https://github.com/Asherlc/dofek/pull/3235#pullrequestreview-5818332363) identified unconditional `latest` entries in the ML and deploy workflows; those now use the same default-branch guard. The identity integration scenarios are separated from the lifecycle suite to keep both below 1,000 lines.
33+
34+
The immutable tuple is an explicit precondition, backed by the frozen [incoming identity audit](../metric-stream-history-identity-audit-20261107/summary.json) and [retained deletion-candidate audit](../metric-stream-deletion-scope-audit-20261108/summary.json). Arbitrary historical movement between user/provider/time tuples is outside that contract. Wire parsers retain IDs for replay; they do not establish that such movement is supported. Mutable activity/channel/external ID corrections remain covered by executable regressions.
35+
36+
For a same-revision batch, deletion means the latest row matches any supplied scope. Every latest row satisfying a scope also satisfies that scope's candidate predicate, so losing the candidate-scope index does not introduce an extra deletion under the immutable identity contract. Retaining a historical candidate association would not change that set.
37+
38+
Activity-only candidate discovery can still scan broadly. This change removes the second global version scan; it does not promise bounded reads for all supported scopes. Requiring user IDs would break the existing activity-only contract, and adding a new stored access path without workload evidence would exceed the approved minimal fix. The code now states this limitation beside candidate discovery. No timeout, optimizer tuning, migration or scope restriction was added.
Lines changed: 290 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,290 @@
1+
import { randomUUID } from "node:crypto";
2+
import { afterAll, beforeAll, describe, expect, it } from "vitest";
3+
import { z } from "zod";
4+
import { type ClickHouseClient, createClickHouseClientFromEnv } from "../db/clickhouse.ts";
5+
import { buildClickHouseBootstrapStatementsForNativeMetricStream } from "../db/clickhouse-metric-stream-bootstrap.ts";
6+
import {
7+
type ClickHouseMetricStreamInsertClient,
8+
insertMetricStreamEventsIntoClickHouse,
9+
mapMetricStreamEventToClickHouseRow,
10+
markMetricStreamScopesDeletedInClickHouse,
11+
} from "./clickhouse-sink.ts";
12+
import {
13+
METRIC_STREAM_DELETE_ACKNOWLEDGEMENT_TABLE,
14+
METRIC_STREAM_DELETE_SCOPE_TABLE,
15+
METRIC_STREAM_TABLE,
16+
} from "./clickhouse-table.ts";
17+
import {
18+
createMetricStreamDeletedEvent,
19+
createMetricStreamEvent,
20+
type MetricStreamRowInput,
21+
metricStreamEventV2Schema,
22+
} from "./events.ts";
23+
24+
const operationRevision = "1000000000000000";
25+
function createCurrentMetricStreamEvent(
26+
row: MetricStreamRowInput & { id?: string },
27+
revision = operationRevision,
28+
) {
29+
return row.id === undefined
30+
? createMetricStreamEvent(row, revision)
31+
: metricStreamEventV2Schema.parse({ ...row, version: 2, operationRevision: revision });
32+
}
33+
const createCurrentMetricStreamDeletedEvent = (
34+
scope: Parameters<typeof createMetricStreamDeletedEvent>[0],
35+
revision = operationRevision,
36+
) => createMetricStreamDeletedEvent(scope, revision);
37+
function assertInsertCapable(
38+
client: ClickHouseClient,
39+
): asserts client is ClickHouseClient & ClickHouseMetricStreamInsertClient {
40+
if (typeof client.insert !== "function") throw new Error("ClickHouse client must support insert");
41+
}
42+
43+
describe("metric stream scoped deletion identity (integration)", () => {
44+
const client = createClickHouseClientFromEnv();
45+
assertInsertCapable(client);
46+
beforeAll(async () => {
47+
for (const statement of buildClickHouseBootstrapStatementsForNativeMetricStream(""))
48+
await client.command({ query: statement });
49+
});
50+
afterAll(async () => {
51+
await client.close?.();
52+
});
53+
it("prunes unrelated identities while applying a scoped deletion", async () => {
54+
const userId = randomUUID();
55+
const otherUserId = randomUUID();
56+
const activityId = randomUUID();
57+
const queryId = randomUUID();
58+
const fixtureRows = 65_536;
59+
try {
60+
await client.command({
61+
query: `INSERT INTO ${METRIC_STREAM_TABLE}
62+
(id, user_id, activity_id, provider_id, recorded_at, channel, scalar)
63+
SELECT reinterpretAsUUID(MD5(concat(toString({userId:UUID}), toString(number)))),
64+
if(number % 16 = 0, {userId:UUID}, {otherUserId:UUID}),
65+
{activityId:UUID}, 'identity-pruning-test',
66+
toDateTime64('2026-06-08 12:00:00', 6), 'heart_rate', 72
67+
FROM numbers(${fixtureRows})`,
68+
query_params: { userId, otherUserId, activityId },
69+
});
70+
await markMetricStreamScopesDeletedInClickHouse(
71+
{
72+
command: (options) =>
73+
client.command({
74+
...options,
75+
...(options.query.startsWith(`INSERT INTO ${METRIC_STREAM_TABLE} (`)
76+
? { query_id: queryId }
77+
: {}),
78+
}),
79+
},
80+
[createCurrentMetricStreamDeletedEvent({ userId, activityId })],
81+
);
82+
const result = await client.query({
83+
query: `SELECT user_id, countIf(is_deleted = 1) AS deleted,
84+
countIf(is_deleted = 0) AS live
85+
FROM ${METRIC_STREAM_TABLE} FINAL
86+
WHERE user_id IN {users:Array(UUID)} GROUP BY user_id`,
87+
query_params: { users: [userId, otherUserId] },
88+
format: "JSONEachRow",
89+
});
90+
const counts = z
91+
.array(
92+
z.object({
93+
user_id: z.string(),
94+
deleted: z.coerce.number(),
95+
live: z.coerce.number(),
96+
}),
97+
)
98+
.parse(await result.json());
99+
expect(counts).toEqual(
100+
expect.arrayContaining([
101+
{ user_id: userId, deleted: fixtureRows / 16, live: 0 },
102+
{ user_id: otherUserId, deleted: 0, live: (fixtureRows * 15) / 16 },
103+
]),
104+
);
105+
106+
await client.command({ query: "SYSTEM FLUSH LOGS" });
107+
const stats = await client.query({
108+
query: `SELECT read_rows FROM system.query_log
109+
WHERE query_id = {queryId:String} AND type = 'QueryFinish'`,
110+
query_params: { queryId },
111+
format: "JSONEachRow",
112+
});
113+
const reads = z
114+
.array(z.object({ read_rows: z.coerce.number().positive() }))
115+
.parse(await stats.json());
116+
expect(reads).toHaveLength(1);
117+
expect(reads[0]?.read_rows).toBeLessThan(fixtureRows / 2);
118+
} finally {
119+
await client.command({
120+
query: `ALTER TABLE ${METRIC_STREAM_TABLE} DELETE
121+
WHERE user_id IN {users:Array(UUID)} SETTINGS mutations_sync = 1`,
122+
query_params: { users: [userId, otherUserId] },
123+
});
124+
await client.command({
125+
query: `ALTER TABLE ${METRIC_STREAM_DELETE_SCOPE_TABLE} DELETE
126+
WHERE user_id = {userId:UUID} SETTINGS mutations_sync = 1`,
127+
query_params: { userId },
128+
});
129+
}
130+
});
131+
132+
it.each(["channel", "externalId", "scalar"] as const)(
133+
"resolves the latest %s correction with native timestamp precision",
134+
async (field) => {
135+
const userId = randomUUID();
136+
const event = createCurrentMetricStreamEvent({
137+
id: randomUUID(),
138+
userId,
139+
providerId: "identity-correction-test",
140+
recordedAt: "2026-06-08T12:00:00.000001Z",
141+
sourceType: "api",
142+
channel: "heart_rate",
143+
externalId: "original",
144+
scalar: 72,
145+
});
146+
const changes = { channel: "power", externalId: "moved", scalar: 81 };
147+
const latest = { ...event, [field]: changes[field] };
148+
const scope = {
149+
userId,
150+
providerId: event.providerId,
151+
channel: event.channel,
152+
externalId: event.externalId,
153+
recordedAtStart: "2026-06-08T12:00:00.000Z",
154+
recordedAtEnd: "2026-06-08T12:00:01.000Z",
155+
};
156+
await client.command({ query: `SYSTEM STOP MERGES ${METRIC_STREAM_TABLE}` });
157+
try {
158+
for (const [index, row] of [event, latest].entries()) {
159+
await client.insert({
160+
table: METRIC_STREAM_TABLE,
161+
values: [
162+
{
163+
...mapMetricStreamEventToClickHouseRow(row),
164+
version: 3,
165+
ingested_at: `2026-06-08T12:00:00.00000000${index}Z`,
166+
},
167+
],
168+
format: "JSONEachRow",
169+
clickhouse_settings: { date_time_input_format: "best_effort" },
170+
});
171+
}
172+
await markMetricStreamScopesDeletedInClickHouse(client, [
173+
createCurrentMetricStreamDeletedEvent(scope),
174+
]);
175+
const result = await client.query({
176+
query: `SELECT latest.1 AS channel, latest.2 AS external_id,
177+
latest.3 AS scalar, latest.4 AS is_deleted,
178+
toString(toUnixTimestamp64Micro(latest.5)) AS recorded_microseconds,
179+
deleted_count
180+
FROM (SELECT argMax(tuple(channel, external_id, scalar, is_deleted, recorded_at),
181+
tuple(version, ingested_at)) AS latest,
182+
countIf(is_deleted = 1) AS deleted_count
183+
FROM ${METRIC_STREAM_TABLE} WHERE id = {id:UUID})`,
184+
query_params: { id: event.id },
185+
format: "JSONEachRow",
186+
});
187+
const rows = z
188+
.array(
189+
z.object({
190+
channel: z.string(),
191+
external_id: z.string(),
192+
scalar: z.number(),
193+
is_deleted: z.number(),
194+
recorded_microseconds: z.string(),
195+
deleted_count: z.coerce.number(),
196+
}),
197+
)
198+
.parse(await result.json());
199+
expect(rows).toEqual([
200+
{
201+
channel: latest.channel,
202+
external_id: latest.externalId,
203+
scalar: latest.scalar,
204+
is_deleted: field === "scalar" ? 1 : 0,
205+
recorded_microseconds: (
206+
BigInt(Date.parse("2026-06-08T12:00:00Z")) * 1000n +
207+
1n
208+
).toString(),
209+
deleted_count: field === "scalar" ? 1 : 0,
210+
},
211+
]);
212+
} finally {
213+
await client.command({ query: `SYSTEM START MERGES ${METRIC_STREAM_TABLE}` });
214+
await client.command({
215+
query: `ALTER TABLE ${METRIC_STREAM_TABLE} DELETE
216+
WHERE user_id = {userId:UUID} SETTINGS mutations_sync = 1`,
217+
query_params: { userId },
218+
});
219+
await client.command({
220+
query: `ALTER TABLE ${METRIC_STREAM_DELETE_SCOPE_TABLE} DELETE
221+
WHERE user_id = {userId:UUID} SETTINGS mutations_sync = 1`,
222+
query_params: { userId },
223+
});
224+
}
225+
},
226+
);
227+
228+
it("applies an activity-only scope across users in a mixed scope batch", async () => {
229+
const activityId = randomUUID();
230+
const specificUser = randomUUID();
231+
const events = [0, 1].map((index) =>
232+
createCurrentMetricStreamEvent(
233+
{
234+
userId: randomUUID(),
235+
providerId: `mixed-scope-${index}`,
236+
activityId,
237+
recordedAt: "2026-06-08T12:00:00Z",
238+
externalId: "sample",
239+
sourceType: "api",
240+
channel: "heart_rate",
241+
scalar: 72,
242+
},
243+
"999999999999999",
244+
),
245+
);
246+
const deletes = [
247+
createCurrentMetricStreamDeletedEvent({ userId: specificUser, providerId: "unrelated" }),
248+
createCurrentMetricStreamDeletedEvent({ activityId }),
249+
];
250+
try {
251+
await insertMetricStreamEventsIntoClickHouse(client, events);
252+
await markMetricStreamScopesDeletedInClickHouse(client, deletes);
253+
const result = await client.query({
254+
query: `SELECT countIf(is_deleted = 1) AS deleted
255+
FROM ${METRIC_STREAM_TABLE} FINAL WHERE id IN {ids:Array(UUID)}`,
256+
query_params: { ids: events.map((event) => event.id) },
257+
format: "JSONEachRow",
258+
});
259+
expect(z.array(z.object({ deleted: z.coerce.number() })).parse(await result.json())).toEqual([
260+
{ deleted: 2 },
261+
]);
262+
const acknowledgements = await client.query({
263+
query: `SELECT count() AS count FROM ${METRIC_STREAM_DELETE_ACKNOWLEDGEMENT_TABLE} FINAL
264+
WHERE event_id IN {ids:Array(UUID)}`,
265+
query_params: { ids: deletes.map((event) => event.eventId) },
266+
format: "JSONEachRow",
267+
});
268+
expect(
269+
z.array(z.object({ count: z.coerce.number() })).parse(await acknowledgements.json()),
270+
).toEqual([{ count: 2 }]);
271+
} finally {
272+
await client.command({
273+
query: `ALTER TABLE ${METRIC_STREAM_TABLE} DELETE
274+
WHERE activity_id = {activityId:UUID} SETTINGS mutations_sync = 1`,
275+
query_params: { activityId },
276+
});
277+
await client.command({
278+
query: `ALTER TABLE ${METRIC_STREAM_DELETE_SCOPE_TABLE} DELETE
279+
WHERE activity_id = {activityId:UUID} OR user_id = {specificUser:UUID}
280+
SETTINGS mutations_sync = 1`,
281+
query_params: { activityId, specificUser },
282+
});
283+
await client.command({
284+
query: `ALTER TABLE ${METRIC_STREAM_DELETE_ACKNOWLEDGEMENT_TABLE} DELETE
285+
WHERE event_id IN {ids:Array(UUID)} SETTINGS mutations_sync = 1`,
286+
query_params: { ids: deletes.map((event) => event.eventId) },
287+
});
288+
}
289+
});
290+
});

0 commit comments

Comments
 (0)