Expire recovery scores after source deletion

[Asherlc]

Summary

• append tombstones when recovery input or score keys disappear
• migrate both ClickHouse recovery tables and filter deleted rows from every API consumer
• cover the full source removal lifecycle against real ClickHouse, including the dashboard response

Validation

• pnpm lint
• pnpm exec tsc --noEmit
• pnpm exec tsc --noEmit (packages/server)
• pnpm test (13,563 passed, 21 skipped)
• pnpm exec vitest run --project integration packages/server/src/services/daily-recovery-read-model.integration.test.ts --retry=0

Fixes #1771

---

Summary by cubic

Expire recovery inputs and scores when their source data is removed by writing tombstones and filtering them from all APIs. Also only append “dirty” input rows to avoid unnecessary incremental refreshes (addresses #1771).

• Bug Fixes

  • Emit is_deleted tombstones in analytics.daily_recovery_inputs and analytics.daily_recovery, preserving existing keys so deletions are captured.
  • Incremental builds reprocess only users changed since the last refresh and ignore deleted rows when scoring.
  • Append rows only when input values change (null-safe compare) to prevent advancing downstream refreshes unnecessarily.
  • Filter is_deleted = 0 across server queries (stress, recovery, dashboard, mobile) and add an end-to-end ClickHouse lifecycle test.

• Migration

  • Run 0057_daily_recovery_lifecycle to add is_deleted UInt8 DEFAULT 0 to both recovery tables.
  • After migrating, re-materialize the read models; no client changes required.

^{Written for commit 4f44f0e. Summary will update on new commits.}

[codereviewbot-ai]

🤖 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.

[cursor]

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.

[sourcery-ai]

Sorry @Asherlc, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Warning

Review limit reached

@Asherlc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 37 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 83187cbe-7989-428d-88c8-3657ea7bd2ef

📥 Commits

Reviewing files that changed from the base of the PR and between a5c6b48 and 4f44f0e.

📒 Files selected for processing (22)

• analytics/models/read_models/daily_recovery.sql
• analytics/models/read_models/daily_recovery_inputs.sql
• analytics/models/read_models/read_model_microbatch.sql.test.ts
• packages/server/src/repositories/stress-repository.test.ts
• packages/server/src/repositories/stress-repository.ts
• packages/server/src/routers/clickhouse-integration-test-models.ts
• packages/server/src/routers/clickhouse-integration-test-read-models-a.ts
• packages/server/src/routers/clickhouse-integration-test-read-models-b.ts
• packages/server/src/routers/recovery.test.ts
• packages/server/src/routers/recovery.ts
• packages/server/src/routers/stress.test.ts
• packages/server/src/services/daily-recovery-read-model.integration.test.ts
• packages/server/src/services/dashboard-overview.test.ts
• packages/server/src/services/dashboard-overview.ts
• packages/server/src/services/mobile-recovery-tab.test.ts
• packages/server/src/services/mobile-recovery-tab.ts
• packages/server/src/services/mobile-training-tab.test.ts
• packages/server/src/services/mobile-training-tab.ts
• src/db/clickhouse-migrations/0057_daily_recovery_lifecycle.test.ts
• src/db/clickhouse-migrations/0057_daily_recovery_lifecycle.ts
• src/db/clickhouse-migrations/registry.test.ts
• src/db/clickhouse-migrations/registry.ts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Expire recovery inputs/scores when their source data is deleted

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Emit tombstone rows for deleted recovery inputs/scores via an is_deleted marker.
• Update all ClickHouse-backed recovery queries to exclude deleted rows (is_deleted = 0).
• Add ClickHouse migration + end-to-end integration test covering full deletion lifecycle.

Diagram

graph TD
  MIG[["Migration 0056: add is_deleted"]] --> RM1["Read model: recovery inputs"] --> RM2["Read model: recovery scores"] --> API(["Server APIs/Repos"]) --> CLIENT["Dashboard/Mobile"]
  SRC[("CH source views") ] --> RM1
  MIG --> RM2

  subgraph Legend
    direction LR
    _mig[["Migration"]] ~~~ _db[("ClickHouse data")] ~~~ _rm["Read model"] ~~~ _svc(["Service/API"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Hard-delete/TTL instead of tombstones

• ➕ Avoids carrying deleted rows forward indefinitely
• ➕ Simplifies consumer-side filtering if deletes are truly removed
• ➖ Hard to make incremental read-model builds reliably detect missing keys
• ➖ Risk of “resurrection”/stale aggregates if upstream deletes aren’t replayed in-order
• ➖ ReplacingMergeTree patterns typically benefit from explicit lifecycle markers

2. Recompute full read-models periodically (no incremental tombstone union)

• ➕ Simpler SQL logic; deletions naturally disappear
• ➕ Lower risk of subtle incremental-state bugs
• ➖ Higher ClickHouse cost and longer build times
• ➖ Operationally harder to run frequently enough for product expectations

3. Model deletions at the source views (emit is_deleted upstream)

• ➕ Single place to decide lifecycle; downstream models are simpler
• ➕ Consistent deletion semantics across all derived models
• ➖ Requires broader changes to upstream pipelines/views
• ➖ May not be feasible if upstream sources don’t preserve deletion events

Recommendation: The PR’s tombstone approach is the best fit for incremental ClickHouse read-models: it makes deletions explicit and deterministic, and the consistent is_deleted = 0 filtering across consumers prevents stale recovery. The key-union + left-join pattern is appropriate here; alternatives either increase cost (full rebuilds) or require broader upstream refactors.

Files changed (22) +555 / -74

Bug fix (7) +108 / -66

daily_recovery.sqlAdd tombstones + incremental key union for daily recovery scores +67/-60

Add tombstones + incremental key union for daily recovery scores

• Reworks incremental logic to track changed users, union existing keys, and emit tombstone rows when a (user_id, date) score disappears. Filters upstream inputs to 'is_deleted = 0' and adds 'is_deleted' output based on missing joined score rows; also normalizes refreshed_at to UTC and enables join_use_nulls.

analytics/models/read_models/daily_recovery.sql

daily_recovery_inputs.sqlEmit tombstones for deleted recovery inputs via key union +35/-6

Emit tombstones for deleted recovery inputs via key union

• Adds incremental key preservation ('existing_keys' + 'UNION DISTINCT') so removed input keys produce rows with 'is_deleted = 1'. Sets refreshed_at to UTC and enables join_use_nulls to make left-join tombstoning behavior consistent.

analytics/models/read_models/daily_recovery_inputs.sql

stress-repository.tsFilter deleted recovery rows in stress repository query +1/-0

Filter deleted recovery rows in stress repository query

• Adds 'AND recovery_inputs.is_deleted = 0' to the ClickHouse query selecting recovery inputs for stress calculations.

packages/server/src/repositories/stress-repository.ts

recovery.tsExclude deleted recovery rows from recovery endpoints +2/-0

Exclude deleted recovery rows from recovery endpoints

• Adds 'is_deleted = 0' predicates to ClickHouse queries used by readinessScore and strainTarget so tombstoned recovery rows are not returned.

packages/server/src/routers/recovery.ts

dashboard-overview.tsFilter tombstoned recovery scores from dashboard overview query +1/-0

Filter tombstoned recovery scores from dashboard overview query

• Adds 'AND recovery.is_deleted = 0' to the dashboard overview recovery query to avoid returning deleted readiness history.

packages/server/src/services/dashboard-overview.ts

mobile-recovery-tab.tsFilter deleted recovery inputs in mobile recovery tab query +1/-0

Filter deleted recovery inputs in mobile recovery tab query

• Adds 'AND recovery_inputs.is_deleted = 0' so mobile recovery tab data ignores tombstoned recovery rows.

packages/server/src/services/mobile-recovery-tab.ts

mobile-training-tab.tsExclude tombstoned recovery rows from mobile training tab +1/-0

Exclude tombstoned recovery rows from mobile training tab

• Adds 'AND recovery.is_deleted = 0' to the ClickHouse recovery query used in training tab aggregation.

packages/server/src/services/mobile-training-tab.ts

Tests (13) +432 / -8

read_model_microbatch.sql.test.tsUpdate microbatch SQL assertions for deletion lifecycle logic +10/-4

Update microbatch SQL assertions for deletion lifecycle logic

• Adjusts read-model SQL snapshot expectations to assert presence of 'existing_keys' / 'changed_users' CTEs and 'is_deleted' computations/filters in both recovery read-models.

analytics/models/read_models/read_model_microbatch.sql.test.ts

stress-repository.test.tsAssert stress repository filters deleted recovery rows +1/-0

Assert stress repository filters deleted recovery rows

• Extends query-text expectations to include 'recovery_inputs.is_deleted = 0' when reading from analytics.daily_recovery.

packages/server/src/repositories/stress-repository.test.ts

clickhouse-integration-test-models.tsAdd is_deleted columns to ClickHouse integration test schemas +2/-0

Add is_deleted columns to ClickHouse integration test schemas

• Updates the test DDL definitions for daily_recovery_inputs and daily_recovery tables to include 'is_deleted UInt8 DEFAULT 0' so lifecycle behavior can be tested realistically.

packages/server/src/routers/clickhouse-integration-test-models.ts

clickhouse-integration-test-read-models-a.tsInclude is_deleted output in test recovery-input read model SQL +1/-0

Include is_deleted output in test recovery-input read model SQL

• Adds '0 AS is_deleted' to the test read-model select to match the new table schema for integration tests.

packages/server/src/routers/clickhouse-integration-test-read-models-a.ts

clickhouse-integration-test-read-models-b.tsFilter deleted rows in test recovery read model selector +3/-1

Filter deleted rows in test recovery read model selector

• Adds '0 AS is_deleted' to the select list and appends 'WHERE is_deleted = 0' to ensure tests exercise the new consumer-side filter behavior.

packages/server/src/routers/clickhouse-integration-test-read-models-b.ts

recovery.test.tsAssert recovery router queries exclude deleted rows +2/-0

Assert recovery router queries exclude deleted rows

• Updates router query expectations to check for 'recovery_inputs.is_deleted = 0' and 'recovery.is_deleted = 0' across readiness/strain-target flows.

packages/server/src/routers/recovery.test.ts

stress.test.tsAssert stress router uses is_deleted filter on recovery inputs +1/-0

Assert stress router uses is_deleted filter on recovery inputs

• Extends query-text assertion to require 'recovery_inputs.is_deleted = 0' when stress endpoints query analytics.daily_recovery.

packages/server/src/routers/stress.test.ts

daily-recovery-read-model.integration.test.tsEnd-to-end ClickHouse test for deletion tombstoning lifecycle +386/-0

End-to-end ClickHouse test for deletion tombstoning lifecycle

• Adds a real ClickHouse integration test that seeds minimal source data, materializes both read-model tables, validates API output, then deletes the source (TRUNCATE) and re-materializes incrementally to assert tombstones are written and API responses omit recovery.

packages/server/src/services/daily-recovery-read-model.integration.test.ts

dashboard-overview.test.tsAssert dashboard overview filters deleted recovery rows +1/-0

Assert dashboard overview filters deleted recovery rows

• Adds an expectation that dashboard recovery queries include 'recovery.is_deleted = 0'.

packages/server/src/services/dashboard-overview.test.ts

mobile-recovery-tab.test.tsAssert mobile recovery tab excludes deleted recovery inputs +1/-0

Assert mobile recovery tab excludes deleted recovery inputs

• Adds an expectation that the recovery query includes 'recovery_inputs.is_deleted = 0'.

packages/server/src/services/mobile-recovery-tab.test.ts

mobile-training-tab.test.tsAssert mobile training tab filters deleted recovery scores +1/-0

Assert mobile training tab filters deleted recovery scores

• Extends test expectations to include 'recovery.is_deleted = 0' in the recovery query used by the training tab.

packages/server/src/services/mobile-training-tab.test.ts

0056_daily_recovery_lifecycle.test.tsUnit test for recovery lifecycle migration content +20/-0

Unit test for recovery lifecycle migration content

• Adds a migration test asserting both tables are altered and 'is_deleted UInt8 DEFAULT 0' is included in each statement.

src/db/clickhouse-migrations/0056_daily_recovery_lifecycle.test.ts

registry.test.tsUpdate migration registry test for new latest migration +3/-3

Update migration registry test for new latest migration

• Updates the registry test to expect 0056 as the newest migration and to validate it targets the recovery tables.

src/db/clickhouse-migrations/registry.test.ts

Other (2) +15 / -0

0056_daily_recovery_lifecycle.tsMigration: add is_deleted to daily recovery lifecycle tables +13/-0

Migration: add is_deleted to daily recovery lifecycle tables

• Introduces migration 0056 to add 'is_deleted UInt8 DEFAULT 0' to analytics.daily_recovery_inputs and analytics.daily_recovery at stable column positions.

src/db/clickhouse-migrations/0056_daily_recovery_lifecycle.ts

registry.tsRegister daily recovery lifecycle migration (0056) +2/-0

Register daily recovery lifecycle migration (0056)

• Adds the new migration factory to the ClickHouse migrations registry so it runs in order.

src/db/clickhouse-migrations/registry.ts

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 163 rules

✅ Skills: fix-provider, write-tests, cloudflare

1. Changed-users always all ✓ Resolved 🐞 Bug ➹ Performance

Description

daily_recovery incremental scope depends on daily_recovery_inputs.refreshed_at advancing only
for truly changed users, but daily_recovery_inputs emits all existing keys every run and assigns a
new refreshed_at via refresh_clock, so changed_users will usually include every user and
daily_recovery will recompute everything each run.

Code

analytics/models/read_models/daily_recovery_inputs.sql[R131-156]

+result_keys AS (
+    SELECT
+        user_id,
+        date
+    FROM inputs_with_baselines
+    {% if is_incremental() %}
+    UNION DISTINCT
+    SELECT
+        user_id,
+        date
+    FROM existing_keys
+    {% endif %}
+),
+
refresh_clock AS (
    SELECT
        toUInt64(toUnixTimestamp64Nano(now64(9))) AS refresh_version,
-        now64(9) AS refreshed_at
+        now64(9, 'UTC') AS refreshed_at
)

SELECT
-    CAST(inputs_with_baselines.user_id, 'UUID') AS user_id,
-    CAST(inputs_with_baselines.date, 'Date') AS date,
+    CAST(result_keys.user_id, 'UUID') AS user_id,
+    CAST(result_keys.date, 'Date') AS date,
    inputs_with_baselines.hrv AS hrv,
    inputs_with_baselines.resting_hr AS resting_hr,
    inputs_with_baselines.respiratory_rate AS respiratory_rate,

Relevance

⭐⭐⭐ High

Team has accepted fixes to bound incremental append models and avoid full snapshot re-emits; this
matches that pattern.

PR-#1253

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

daily_recovery_inputs unions all previously materialized active keys into result_keys and then
cross joins refresh_clock so every emitted row gets a new refreshed_at. daily_recovery then
uses daily_recovery_inputs.refreshed_at > last_refreshed_at to derive changed_users, which will
match most/all users whenever inputs were re-emitted, causing the downstream model to reprocess
broadly each incremental run.

analytics/models/read_models/daily_recovery_inputs.sql[12-20]
analytics/models/read_models/daily_recovery_inputs.sql[131-175]
analytics/models/read_models/daily_recovery.sql[12-23]
analytics/models/read_models/daily_recovery.sql[43-47]
PR-#1253

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`daily_recovery` uses an incremental optimization that selects `changed_users` by comparing `daily_recovery_inputs.refreshed_at` against the last materialized recovery refresh time. But `daily_recovery_inputs` currently re-emits all existing (user_id, date) keys on every incremental run and cross joins a new `refresh_clock.refreshed_at`, which makes all rows appear “changed” every run.

This defeats the intended incremental behavior, increasing ClickHouse CPU/IO and causing the downstream `daily_recovery` model to process essentially the full user set each run.

## Issue Context
- `daily_recovery_inputs` currently unions `existing_keys` (all active keys) into `result_keys` and cross joins `refresh_clock`, producing a new `refreshed_at` for every key.
- `daily_recovery` computes `changed_users` by selecting distinct users whose inputs `refreshed_at` is newer than the last recovery refresh.

## Fix Focus Areas
- analytics/models/read_models/daily_recovery_inputs.sql[12-20]
- analytics/models/read_models/daily_recovery_inputs.sql[131-175]
- analytics/models/read_models/daily_recovery.sql[12-23]
- analytics/models/read_models/daily_recovery.sql[43-47]

## What to change
Implement a true incremental/dirty-key strategy for `daily_recovery_inputs` so it does **not** rewrite unchanged keys each run. For example:
1. Add a `target_state` CTE (similar to `daily_recovery`) for `daily_recovery_inputs`.
2. Build a `changed_users` / `dirty_keys` set driven by *source changes* and/or key-diffing (new keys + keys that disappeared) and restrict `result_keys` to that set.
3. Only emit tombstones for keys in-scope (e.g., keys for `changed_users`) rather than unioning all existing keys.

This keeps `daily_recovery_inputs.refreshed_at` advancing only when keys actually change, restoring the correctness of `daily_recovery.changed_users`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. CREATE TABLE outside schema/ ✗ Dismissed 📘 Rule violation ⚙ Maintainability

Description

The PR adds ClickHouse table DDL (CREATE TABLE ...) directly inside non-schema/ TypeScript
files, rather than keeping table definitions under a dedicated schema/ directory. This scatters
schema definitions across the codebase and violates the required schema/enums placement convention.

Code

packages/server/src/services/daily-recovery-read-model.integration.test.ts[R268-276]

+function createDailyMetricsTableSql(targetSchema: string): string {
+  return `CREATE TABLE ${targetSchema}.v_daily_metrics (
+    user_id UUID,
+    date Date,
+    hrv Nullable(Float32),
+    respiratory_rate_avg Nullable(Float32)
+  )
+  ENGINE = MergeTree
+  ORDER BY (user_id, date)`;

Relevance

⭐⭐ Medium

No close historical precedent found requiring ClickHouse test DDL to live under schema/; could be
allowed for fixtures.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 721925 requires DB table definitions to be placed under schema/. The added
integration test defines multiple ClickHouse tables via CREATE TABLE strings, and the router
test-models file contains (and is modified to extend) ClickHouse table column definitions, both
outside schema/.

Rule 721925: Place database schema and enums under schema/ directory
packages/server/src/services/daily-recovery-read-model.integration.test.ts[268-328]
packages/server/src/routers/clickhouse-integration-test-models.ts[137-178]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Database table definitions (ClickHouse `CREATE TABLE` DDL) are being introduced/maintained in TypeScript files outside a `schema/` directory.

## Issue Context
The compliance rule requires canonical DB table/enum definitions to live under `schema/` to keep schema auditable and consistent; embedding DDL strings in test/service/router code makes this harder.

## Fix Focus Areas
- packages/server/src/services/daily-recovery-read-model.integration.test.ts[268-328]
- packages/server/src/routers/clickhouse-integration-test-models.ts[137-178]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. buildTestRecoveryReadModelSelectSql inlines SQL ✗ Dismissed 📘 Rule violation ⚙ Maintainability

Description

The PR modifies ClickHouse read-model transformation SQL as multiline template literals in
TypeScript test helpers instead of loading the canonical .sql model files under
analytics/models/. This increases risk of SQL drift between production models and test fixtures,
violating the requirement to store transformation SQL in dedicated .sql model files.

Code

packages/server/src/routers/clickhouse-integration-test-read-models-b.ts[R394-401]

  CAST(NULL, 'Nullable(Float64)') AS resting_hr_score,
  CAST(NULL, 'Nullable(Float64)') AS sleep_score,
  CAST(NULL, 'Nullable(Float64)') AS respiratory_rate_score,
+  0 AS is_deleted,
  refresh_version,
  refreshed_at
-FROM ${databases.analytics}.daily_recovery_inputs`;
+FROM ${databases.analytics}.daily_recovery_inputs
+WHERE is_deleted = 0`;

Relevance

⭐⭐ Medium

No close historical precedent found for enforcing “SQL must live in .sql files” in TS test helpers.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 784482 requires non-trivial ClickHouse transformation SQL to live in dedicated
.sql model files under analytics/models/, with TypeScript only loading/referencing those files.
The cited TS files embed and modify full SELECT statements via template literals, demonstrating the
violation.

Rule 784482: Store ClickHouse analytics transformation SQL in dedicated .sql model files
packages/server/src/routers/clickhouse-integration-test-read-models-b.ts[376-401]
packages/server/src/routers/clickhouse-integration-test-read-models-a.ts[671-696]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
ClickHouse transformation SQL is inlined in TypeScript (template literals) in test read-model helpers, instead of being loaded from `analytics/models/**/*.sql`.

## Issue Context
The repo already contains canonical read-model SQL under `analytics/models/read_models/`, and this PR even adds logic elsewhere to load those `.sql` files. Keeping additional, edited copies of read-model SQL in TS risks divergence over time.

## Fix Focus Areas
- packages/server/src/routers/clickhouse-integration-test-read-models-b.ts[376-401]
- packages/server/src/routers/clickhouse-integration-test-read-models-a.ts[671-696]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[github-actions]

Storybook previews for 5f0aa0bb are ready:

• Web Storybook
• Mobile Storybook

This comment updates automatically on each PR push.

[codereviewbot-ai]

🤖 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.

[codereviewbot-ai]

🤖 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.