Bound PeerDB CDC work units - #1236
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughAdds a ClickHouse CDC health-check implementation (with Zod contracts), CLI integration, tests, PeerDB mirror throttling (100k batches + snapshot controls), Postgres replication-slot/WAL retention increases, and runbook/incident documentation. ChangesClickHouse CDC Health Monitoring and Incident Recovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 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 |
|
Storybook previews for This comment updates automatically on each PR push. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/db/clickhouse-cdc.test.ts (1)
376-384:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the throttling knobs on each raw mirror, not on an aggregated string.
These assertions still pass if
dofek_provider_inventory_raw_analyticsordofek_sensor_priority_raw_analyticslose the new settings whiledofek_fitness_raw_analyticskeeps them. The SQL change touched all three mirrors, so the test should check each renderedCREATE MIRRORstatement separately.Also applies to: 1211-1224
🤖 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-cdc.test.ts` around lines 376 - 384, The test currently concatenates peerDbQueries into rawMirrorSql and asserts throttling settings once, which can mask missing settings on individual mirrors; instead iterate over the rendered CREATE MIRROR statements and assert the three throttling knobs are present for each raw analytics mirror. Locate where peerDbQueries and rawAnalyticsTables are used (variables rawMirrorSql, peerDbQueries, rawAnalyticsTables) and change the assertions to, for each rawAnalyticsTable, find the specific CREATE MIRROR SQL (e.g., the corresponding peerDbQueries element that contains `from: fitness.${rawAnalyticsTable}` / `to: ${rawAnalyticsTable}`) and assert it contains "max_batch_size = 100000", "snapshot_num_rows_per_partition = 100000", "snapshot_max_parallel_workers = 1", and "snapshot_num_tables_in_parallel = 1"; apply the same fix to the other occurrence around the 1211-1224 test block.
🤖 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 `@deploy/README.md`:
- Around line 53-54: The README's ClickHouse memory-limit bullet is stale:
update the text to reflect that the service container memory limit and
checked-in server config now use 13G (not 3500 MiB / 3 GiB) and that the stack
mounts the `clickhouse_memory_limits_13g` config from `deploy/stack.yml`; also
confirm mention of the checked-in `max_server_memory_usage` cap matches the 13G
profile name and note that Docker Swarm configs are immutable so you must rotate
the config key (e.g., `clickhouse_memory_limits_13g`) when changing contents.
In `@docs/clickhouse-cdc-health-runbook.md`:
- Around line 75-76: Inline the mirror-to-table mapping into the runbook so
operators don't need to open source files; copy the exported mapping (the
constant or function in the ClickHouse CDC module that maps mirror names to
destination table names) from the ClickHouse CDC implementation and place it
directly in the runbook (or add a clear link to another human-maintained docs
page that contains that same mapping). Ensure the mapping uses the same
identifiers as in the CDC code (the exported constant/function that assembles
destination tables) and add a short note indicating the mapping version or
last-sync commit so the runbook remains self-contained and auditable.
- Around line 100-125: Update the recovery steps to use the repo's scripted,
env-loading runner instead of bare node and raw source filenames: replace the
`node ... src/db/setup-clickhouse-cdc.ts` invocation with the repo wrapper
`./scripts/with-env.sh tsx src/db/setup-clickhouse-cdc.ts` so
`.env.local`/Infisical secrets are loaded, and change the follow-up instruction
that references `scripts/check-clickhouse-cdc.ts` to the package task `pnpm
check:clickhouse-cdc` (or the equivalent npm script) so operators run the
TypeScript automation through `pnpm tsx` consistently.
In `@scripts/check-clickhouse-cdc.ts`:
- Around line 18-27: The initialization of Postgres and ClickHouse clients
(Client construction using requireEnvironmentVariable("DATABASE_URL") and
createClickHouseClientFromEnv()) happens before the try/catch in main(), so
configuration errors escape your handled path; move creation of postgresClient
and clickHouseClient into the try block inside main() (after Sentry.init) so any
thrown errors are caught and sent to captureException(), set exitCode, and go
through the shared failure handling; also make cleanup logic (where
postgresClient.end or clickHouse client close is called) tolerate
partially-initialized clients (check for non-null/defined before calling
close/end) and ensure missing prerequisites fail with explicit environment key
names (use requireEnvironmentVariable exactly as the source of the DATABASE_URL
error message).
In `@src/db/clickhouse-cdc-health.test.ts`:
- Around line 107-184: Add a test that exercises the inactive-slot branch by
calling checkHealth with healthySlotRows() modified via withSlotOverride to set
active: false for a required slot (e.g.,
"peerflow_slot_dofek_fitness_raw_analytics"), then assert the returned
report.issues contains a failure entry describing the inactive replication slot
(matching addSlotIssues() behavior) and that assertClickHouseCdcHealth(report)
throws; place this alongside the other it(...) cases using the same helpers
(checkHealth, healthySlotRows, withSlotOverride, assertClickHouseCdcHealth).
---
Outside diff comments:
In `@src/db/clickhouse-cdc.test.ts`:
- Around line 376-384: The test currently concatenates peerDbQueries into
rawMirrorSql and asserts throttling settings once, which can mask missing
settings on individual mirrors; instead iterate over the rendered CREATE MIRROR
statements and assert the three throttling knobs are present for each raw
analytics mirror. Locate where peerDbQueries and rawAnalyticsTables are used
(variables rawMirrorSql, peerDbQueries, rawAnalyticsTables) and change the
assertions to, for each rawAnalyticsTable, find the specific CREATE MIRROR SQL
(e.g., the corresponding peerDbQueries element that contains `from:
fitness.${rawAnalyticsTable}` / `to: ${rawAnalyticsTable}`) and assert it
contains "max_batch_size = 100000", "snapshot_num_rows_per_partition = 100000",
"snapshot_max_parallel_workers = 1", and "snapshot_num_tables_in_parallel = 1";
apply the same fix to the other occurrence around the 1211-1224 test block.
🪄 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: ff3291df-f339-44b5-a7a4-ead0497b08d9
📒 Files selected for processing (12)
deploy/README.mddeploy/stack.ymldocs/README.mddocs/clickhouse-cdc-health-runbook.mddocs/production-incident-baseline.mdpackage.jsonscripts/README.mdscripts/check-clickhouse-cdc.tssrc/db/clickhouse-cdc-health.test.tssrc/db/clickhouse-cdc-health.tssrc/db/clickhouse-cdc.test.tssrc/db/peerdb/metric-stream-cdc.sql
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: The PR modifies production-critical Postgres replication settings and PeerDB mirror batch sizes, which directly impact data pipeline stability and recovery behavior, making it unsuitable for auto-approval despite the documentation and low-risk script additions.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: The PR modifies critical production PostgreSQL replication configuration and PeerDB mirror SQL, which could impact database behavior and data replication pipeline integrity, requiring human review.
Re-trigger cubic
There was a problem hiding this comment.
No issues found across 13 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: The PR modifies critical production PostgreSQL replication settings (slots, senders, WAL cap) and PeerDB batch sizes, which directly affect replication reliability and resource usage; even small misconfigurations could cause data loss or infrastructure strain, so a human should review these changes.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/clickhouse-cdc-health-runbook.md (1)
86-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMismatched example slot/mirror names across recovery steps.
Step 2 drops mirror
dofek_fitness_raw_analytics, but Step 3's drop-slot example targetspeerflow_slot_dofek_metric_stream_analytics— the slot for a different mirror (dofek_fitness_raw_analytics's slot would bepeerflow_slot_dofek_fitness_raw_analytics). Mid-incident, an operator copy-pasting these consecutively could drop the wrong slot. Align the example identifiers within the walk-through (or add a single${MIRROR}placeholder note).🤖 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 `@docs/clickhouse-cdc-health-runbook.md` around lines 86 - 103, The example uses mismatched identifiers: Step 2 drops mirror dofek_fitness_raw_analytics but Step 3's slot example targets peerflow_slot_dofek_metric_stream_analytics (a different mirror); update the docs so the slot name matches the mirror (use peerflow_slot_dofek_fitness_raw_analytics) or replace both concrete examples with a single ${MIRROR} placeholder and show how to derive the slot name (peerflow_slot_${MIRROR}) to prevent copy-paste errors; ensure the mirror name dofek_fitness_raw_analytics and the slot pattern peerflow_slot_dofek_fitness_raw_analytics are consistent across steps.
🤖 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.
Outside diff comments:
In `@docs/clickhouse-cdc-health-runbook.md`:
- Around line 86-103: The example uses mismatched identifiers: Step 2 drops
mirror dofek_fitness_raw_analytics but Step 3's slot example targets
peerflow_slot_dofek_metric_stream_analytics (a different mirror); update the
docs so the slot name matches the mirror (use
peerflow_slot_dofek_fitness_raw_analytics) or replace both concrete examples
with a single ${MIRROR} placeholder and show how to derive the slot name
(peerflow_slot_${MIRROR}) to prevent copy-paste errors; ensure the mirror name
dofek_fitness_raw_analytics and the slot pattern
peerflow_slot_dofek_fitness_raw_analytics are consistent across steps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 268ddb6c-9240-4b75-b511-09e515abd20f
📒 Files selected for processing (5)
cspell.jsondeploy/README.mddocs/clickhouse-cdc-health-runbook.mdscripts/check-clickhouse-cdc.tssrc/db/clickhouse-cdc-health.test.ts
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: This PR changes production PostgreSQL replication slot and WAL retention settings (max_slot_wal_keep_size from 4GB to 16GB, slot count from 4 to 6) and reduces PeerDB CDC batch sizes from 1M to 100K rows, which are infrastructure and data pipeline changes with potential impact on database resource,
Re-trigger cubic
Summary
This bounds PeerDB CDC work by lowering mirror CDC batches to 100,000 rows and serializing 100,000-row initial snapshot partitions instead of increasing PeerDB memory.
It adds Postgres logical-slot recovery headroom with six slots/senders and a 16 GiB per-slot WAL cap, plus a manual CDC health command/runbook that is explicitly not a deploy gate.
It records the June 3 stale sleep incident and updates CDC setup tests/docs.
Verification
Passed:
docker stack config -c deploy/stack.yml,pnpm lint,pnpm tsc --noEmit,cd packages/server && pnpm tsc --noEmit,cd packages/web && pnpm tsc --noEmit, focused CDC tests, and the previously failing router suites after a ClickHouse reset.Caveat: a clean full
pnpm teststill failed late in two existing router integration suites because local ClickHouse RSS hit its 3 GiB test cap after 583 files passed.Summary by cubic
Bounds PeerDB CDC work to 100k-row batches and serialized 100k-row snapshots to stay within fixed memory, and increases Postgres logical replication headroom (6 slots/senders, 16 GiB per-slot WAL). Adds a manual CDC health check and runbook to detect lost slots and stale mirrors, plus docs, tests, and incident notes.
New Features
pnpm check:clickhouse-cdcscript that fails on lost/inactive slots, dangerous retained WAL, or stale_peerdb_synced_at.Refactors
max_batch_sizeto 100,000 across mirrors; initial snapshot set to 100,000 rows, single worker/table.max_replication_slots=6,max_wal_senders=6,max_slot_wal_keep_size=16GB.peerflowto the spell-check dictionary.Written for commit 1999a36. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation
Chores
Tests