Fix metric_stream CI failures and future-sample guards - #1083
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates Timescale chunking and indexes for fitness.metric_stream, adds a drizzle migration to enforce 1-day chunks and drop obsolete indexes, expands the runbook and incident record for a storage-pressure event, adds server-side validation to reject future-dated IMU and WHOOP BLE samples (with tests), and adds a metric_stream fallback path for activity sensor queries. ChangesMetric Stream Storage & Schema Controls
Ingest-Time Future-Timestamp Guards
Activity Sensor Store Fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens fitness.metric_stream storage behavior (chunk sizing + index set) and adds server-side ingestion guards to prevent future-dated sensor samples from creating unexpected Timescale chunk fanout.
Changes:
- Add a Timescale migration to enforce 1-day chunk intervals and drop obsolete
metric_streamindexes. - Remove stale index declarations from Drizzle schema + DBML to prevent schema drift.
- Add “future-dated sample” guards to IMU + WHOOP BLE ingestion routers, with unit tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/db/schema.ts |
Removes the stale metric_stream_provider_time_idx declaration from the Drizzle schema. |
drizzle/0011_metric_stream_storage_controls.sql |
Enforces 1-day chunk interval and drops obsolete hypertable indexes. |
docs/schema.dbml |
Removes the dropped index from schema documentation to match intended DB state. |
packages/server/src/routers/whoop-ble-sync.ts |
Adds a future-sample rejection guard and consistent server timestamp logging. |
packages/server/src/routers/whoop-ble-sync.test.ts |
Adds unit coverage for WHOOP BLE future-dated sample rejection. |
packages/server/src/routers/inertial-measurement-unit-sync.ts |
Adds a future-sample rejection guard and consistent server timestamp logging. |
packages/server/src/routers/inertial-measurement-unit-sync.test.ts |
Adds unit coverage for IMU future-dated sample rejection. |
docs/production-incident-baseline.md |
Documents the production storage-pressure incident and mitigation actions. |
docs/metric-stream-timescaledb-runbook.md |
Updates operational runbook queries and guidance for chunk interval/index changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+398
to
+417
| const trpcCaller = caller(ctx); | ||
|
|
||
| await expect( | ||
| trpcCaller.pushRealtimeData({ | ||
| deviceId: "WHOOP Strap", | ||
| samples: [ | ||
| { | ||
| timestamp: "2026-03-30T12:06:00.001Z", | ||
| rrIntervalMs: 812, | ||
| quaternionW: 1.0, | ||
| quaternionX: 0.0, | ||
| quaternionY: 0.0, | ||
| quaternionZ: 0.0, | ||
| }, | ||
| ], | ||
| }), | ||
| ).rejects.toThrow("WHOOP BLE sample timestamp is too far in the future"); | ||
|
|
||
| expect(mockDb.execute).toHaveBeenCalledTimes(0); | ||
| vi.useRealTimers(); |
Comment on lines
+43
to
+48
| function rejectFutureSamples(samples: InertialMeasurementUnitSample[], now: Date) { | ||
| const futureLimitMs = now.getTime() + MAX_FUTURE_SAMPLE_SKEW_MS; | ||
| const futureSample = samples.find((sample) => { | ||
| const sampleTimeMs = Date.parse(sample.timestamp); | ||
| return Number.isFinite(sampleTimeMs) && sampleTimeMs > futureLimitMs; | ||
| }); |
Comment on lines
114
to
119
| rejectFutureSamples(input.samples, now); | ||
| await ensureProvider(ctx.db, ctx.userId); | ||
|
|
||
| // Log timestamp range to detect stale/future data | ||
| const firstTimestamp = input.samples[0]?.timestamp; | ||
| const lastTimestamp = input.samples[input.samples.length - 1]?.timestamp; |
Comment on lines
+142
to
+154
| const execute = makeExecute(); | ||
| const caller = createCaller({ db: { execute }, userId: "user-1" }); | ||
|
|
||
| await expect( | ||
| caller.pushSamples({ | ||
| deviceId: "WHOOP Strap", | ||
| deviceType: "whoop", | ||
| samples: [makeSample({ timestamp: "2026-03-25T10:06:00.001Z" })], | ||
| }), | ||
| ).rejects.toThrow("IMU sample timestamp is too far in the future"); | ||
|
|
||
| expect(execute).toHaveBeenCalledTimes(0); | ||
| vi.useRealTimers(); |
Comment on lines
+1
to
+4
| SELECT public.set_chunk_time_interval('fitness.metric_stream', INTERVAL '1 day'); | ||
| --> statement-breakpoint | ||
| SET lock_timeout = '10s'; | ||
| --> statement-breakpoint |
Comment on lines
+8
to
+10
| const PROVIDER_ID = "whoop_ble"; | ||
| const INSERT_BATCH_SIZE = 2000; | ||
| const MAX_FUTURE_SAMPLE_SKEW_MS = 5 * 60 * 1000; |
Comment on lines
+3
to
+7
| SET lock_timeout = '10s'; | ||
| --> statement-breakpoint | ||
| DROP INDEX IF EXISTS fitness.metric_stream_provider_time_idx; | ||
| --> statement-breakpoint | ||
| DROP INDEX IF EXISTS fitness.metric_stream_recorded_at_idx; |
Comment on lines
+47
to
+52
| function rejectFutureSamples(samples: WhoopBleRealtimeDataSample[], now: Date) { | ||
| const futureLimitMs = now.getTime() + MAX_FUTURE_SAMPLE_SKEW_MS; | ||
| const futureSample = samples.find((sample) => { | ||
| const sampleTimeMs = Date.parse(sample.timestamp); | ||
| return Number.isFinite(sampleTimeMs) && sampleTimeMs > futureLimitMs; | ||
| }); |
Comment on lines
128
to
133
| rejectFutureSamples(input.samples, now); | ||
| await ensureProvider(ctx.db, ctx.userId); | ||
|
|
||
| const firstTimestamp = input.samples[0]?.timestamp; | ||
| const lastTimestamp = input.samples[input.samples.length - 1]?.timestamp; | ||
|
|
Contributor
|
Review app is ready: This environment runs on a dedicated Hetzner server for PR #1083 and updates on each push. |
Asherlc
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/repositories/activity-sensor-store.test-helper.ts`:
- Around line 214-235: The raw SQL in private method
`#activityChannelValuesFromMetricStream` returns unvalidated rows; change the call
from this.#db.execute(...) to this.#db.executeWithSchema(...) and provide a Zod
schema (e.g., z.object({ scalar: z.number() })) that matches the query shape
before mapping; validate the result with that schema and then return rows.map(r
=> r.scalar) so the runtime type is enforced.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8009257-a781-4c78-830a-7f30c1c06b11
📒 Files selected for processing (2)
drizzle/0011_metric_stream_storage_controls.sqlpackages/server/src/repositories/activity-sensor-store.test-helper.ts
✅ Files skipped from review due to trivial changes (1)
- drizzle/0011_metric_stream_storage_controls.sql
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/server/src/repositories/activity-sensor-store.test-helpers.ts (1)
403-424:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
executeWithSchemafor the newmetric_streamraw SQL helper.This new query still relies on compile-time typing only (
execute<{ scalar: number }>). Please switch toexecuteWithSchemawith a Zod row schema so runtime shape mismatches fail fast.Patch sketch
+const metricScalarRowSchema = z.object({ + scalar: z.coerce.number(), +}); async `#activityChannelValuesFromMetricStream`( window: ActivitySensorWindow, channel: string, ): Promise<number[]> { @@ - const rows = await this.#db.execute<{ scalar: number }>( - sql`SELECT scalar::real AS scalar - FROM fitness.metric_stream - WHERE user_id = ${window.userId}::uuid - AND activity_id IN (${sql.join(activityIds, sql`, `)}) - AND channel = ${channel} - AND scalar IS NOT NULL - ORDER BY recorded_at`, - ); + const rows = await executeWithSchema( + this.#db, + metricScalarRowSchema, + sql`SELECT scalar::real AS scalar + FROM fitness.metric_stream + WHERE user_id = ${window.userId}::uuid + AND activity_id IN (${sql.join(activityIds, sql`, `)}) + AND channel = ${channel} + AND scalar IS NOT NULL + ORDER BY recorded_at`, + ); return rows.map((row) => row.scalar); }As per coding guidelines
packages/server/src/repositories/**/*.ts: “PreferexecuteWithSchemafor raw SQL queries in repositories” and “Every raw SQL result MUST have a Zod schema”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/repositories/activity-sensor-store.test-helpers.ts` around lines 403 - 424, The raw SQL call inside private method `#activityChannelValuesFromMetricStream` currently uses this.#db.execute<{ scalar: number }>(...) with compile-time typing only; replace it with this.#db.executeWithSchema(...) and supply a Zod row schema (e.g., z.object({ scalar: z.number() })) so runtime row shape mismatches fail fast, import z from 'zod' if needed, and keep the rest of the SQL and the final rows.map((row) => row.scalar) logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/repositories/activity-sensor-store.test-helpers.ts`:
- Around line 372-375: The fallback path in
`#activityChannelValuesFromMetricStream` can overcount seconds because it returns
raw fitness.metric_stream rows across all window.memberActivityIds and
subsequent mapping treats each row as one second; modify the fallback query in
`#activityChannelValuesFromMetricStream` to deduplicate rows by timestamp (or
normalize into per-second buckets) per activity/member before returning values
so overlapping provider streams do not inflate zone durations; update the same
deduplication logic used by the callers of `#activityChannelValues` and any
mapping code that converts metric_stream rows to value entries (the code paths
referenced around the mapping of rows to seconds) to ensure per-timestamp
uniqueness is enforced consistently.
---
Duplicate comments:
In `@packages/server/src/repositories/activity-sensor-store.test-helpers.ts`:
- Around line 403-424: The raw SQL call inside private method
`#activityChannelValuesFromMetricStream` currently uses this.#db.execute<{ scalar:
number }>(...) with compile-time typing only; replace it with
this.#db.executeWithSchema(...) and supply a Zod row schema (e.g., z.object({
scalar: z.number() })) so runtime row shape mismatches fail fast, import z from
'zod' if needed, and keep the rest of the SQL and the final rows.map((row) =>
row.scalar) logic unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7ead870-537d-4b85-9093-ffa25b761a94
📒 Files selected for processing (4)
docs/production-incident-baseline.mddocs/schema.dbmlpackages/server/src/repositories/activity-sensor-store.test-helpers.tssrc/db/schema.ts
💤 Files with no reviewable changes (2)
- docs/schema.dbml
- src/db/schema.ts
✅ Files skipped from review due to trivial changes (1)
- docs/production-incident-baseline.md
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/server/src/repositories/activity-sensor-store.test-helpers.ts (2)
415-425:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
executeWithSchema+ Zod for this new repository raw SQL path.This new query relies on compile-time typing only. Please validate at runtime with
executeWithSchemaand a row schema before mapping scalars.Proposed fix
+const metricStreamScalarRowSchema = z.object({ + scalar: z.number(), +}); + async `#activityChannelValuesFromMetricStream`( window: ActivitySensorWindow, channel: string, ): Promise<number[]> { @@ - const rows = await this.#db.execute<{ scalar: number }>( - sql`SELECT MAX(scalar)::real AS scalar + const rows = await executeWithSchema( + this.#db, + metricStreamScalarRowSchema, + sql`SELECT MAX(scalar)::real AS scalar FROM fitness.metric_stream WHERE user_id = ${window.userId}::uuid AND activity_id IN (${sql.join(activityIdClauses, sql`, `)}) AND channel = ${channel} AND scalar IS NOT NULL GROUP BY activity_id, recorded_at ORDER BY recorded_at`, );As per coding guidelines
packages/server/src/repositories/**/*.ts: “PreferexecuteWithSchemafor raw SQL queries in repositories” and “Every raw SQL result MUST have a Zod schema”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/repositories/activity-sensor-store.test-helpers.ts` around lines 415 - 425, The raw SQL call in activity-sensor-store.test-helpers.ts uses this.#db.execute without runtime validation; replace the call to this.#db.execute with this.#db.executeWithSchema and provide a Zod schema for the result rows (e.g., a schema describing { scalar: number | null } or { scalar: number } as appropriate), validate the rows with that schema, then map over the validated rows to return the scalar values; update the variable that currently builds the query (activityIdClauses / channel / window.userId) only as needed to feed into executeWithSchema and ensure you handle nullable scalar values per the schema before returning.
416-423:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback grouping can still inflate zone seconds across member activities.
Line 422 groups by
activity_id, recorded_at, so overlapping timestamps acrosswindow.memberActivityIdsstill produce multiple rows; then Line 378 counts each row as one second. This can overcount zone duration.Proposed fix
- const rows = await this.#db.execute<{ scalar: number }>( - sql`SELECT MAX(scalar)::real AS scalar + const rows = await this.#db.execute<{ scalar: number }>( + sql`SELECT MAX(scalar)::real AS scalar FROM fitness.metric_stream WHERE user_id = ${window.userId}::uuid AND activity_id IN (${sql.join(activityIdClauses, sql`, `)}) AND channel = ${channel} AND scalar IS NOT NULL - GROUP BY activity_id, recorded_at - ORDER BY recorded_at`, + GROUP BY DATE_TRUNC('second', recorded_at) + ORDER BY DATE_TRUNC('second', recorded_at)`, );Also applies to: 376-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/repositories/activity-sensor-store.test-helpers.ts` around lines 416 - 423, The SQL fallback grouping uses GROUP BY activity_id, recorded_at which lets identical timestamps from different memberActivityIds produce multiple rows and overcount zone seconds; change the GROUP BY so rows are grouped only by recorded_at (and other non-duplicative columns like channel/user if needed) so MAX(scalar)::real is taken per timestamp across activity_id duplicates. Update the two SQL fragments that reference activityIdClauses/window.memberActivityIds/channel (the SELECT ... MAX(scalar)::real ... GROUP BY activity_id, recorded_at blocks) to remove activity_id from the GROUP BY (group by recorded_at instead) so overlapping timestamps across member activities collapse to a single row per second.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/server/src/repositories/activity-sensor-store.test-helpers.ts`:
- Around line 415-425: The raw SQL call in activity-sensor-store.test-helpers.ts
uses this.#db.execute without runtime validation; replace the call to
this.#db.execute with this.#db.executeWithSchema and provide a Zod schema for
the result rows (e.g., a schema describing { scalar: number | null } or {
scalar: number } as appropriate), validate the rows with that schema, then map
over the validated rows to return the scalar values; update the variable that
currently builds the query (activityIdClauses / channel / window.userId) only as
needed to feed into executeWithSchema and ensure you handle nullable scalar
values per the schema before returning.
- Around line 416-423: The SQL fallback grouping uses GROUP BY activity_id,
recorded_at which lets identical timestamps from different memberActivityIds
produce multiple rows and overcount zone seconds; change the GROUP BY so rows
are grouped only by recorded_at (and other non-duplicative columns like
channel/user if needed) so MAX(scalar)::real is taken per timestamp across
activity_id duplicates. Update the two SQL fragments that reference
activityIdClauses/window.memberActivityIds/channel (the SELECT ...
MAX(scalar)::real ... GROUP BY activity_id, recorded_at blocks) to remove
activity_id from the GROUP BY (group by recorded_at instead) so overlapping
timestamps across member activities collapse to a single row per second.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa3e276a-4983-4beb-8153-9459b685774c
📒 Files selected for processing (1)
packages/server/src/repositories/activity-sensor-store.test-helpers.ts
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
Fixes the current CI failures on this branch by combining storage-control migration changes with missing test-coverage handling for activity heart-rate zones.
This patch adds
drizzle/0011_metric_stream_storage_controls.sqlto enforce 1-dayfitness.metric_streamchunking and drop obsolete indexes withDROP INDEX CONCURRENTLY, and removesmetric_stream_provider_time_idxfromsrc/db/schema.tsplusdocs/schema.dbml.It also updates
PostgresTestActivitySensorStoreso hr-zone fixture generation falls back to rawfitness.metric_streamvalues when deduped samples are missing, which addresses the two integration assertions that were gettingtotalSeconds = 0.The fix adds server-side rejection for IMU and WHOOP BLE realtime samples that are more than five minutes in the future, with explicit BAD_REQUEST messages and regression tests in both router test files.
Operational notes and production impact were added to Timescale runbook and incident baseline docs to capture chunk-interval and index-removal behavior.
Summary by CodeRabbit
Bug Fixes
Performance
Documentation
Chores
Tests