Repository navigation
refactor(cohorts): dedicated reconcile-completion marker topic - #76111
Conversation
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/cohorts/backend/management/commands/compare_cohort_membership.py |
66.7% | 492, 500 |
🤖 Agents: add a test covering the lines above, or note why under "How did you test this code?". Machine-readable gap list: the patch-coverage artifact on this run (gh run download 30932704460 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
platform_features |
██░░░░░░░░░░░░░░░░░░ 12.1% |
7 / 58 |
batch_exports |
████████░░░░░░░░░░░░ 39.5% |
8,808 / 22,295 |
demo |
███████████░░░░░░░░░ 56.3% |
1,497 / 2,661 |
warehouse_sources_queue |
████████████░░░░░░░░ 59.2% |
148 / 250 |
data_tools |
██████████████░░░░░░ 70.0% |
63 / 90 |
tasks |
██████████████░░░░░░ 70.6% |
33,997 / 48,186 |
ai_gateway |
███████████████░░░░░ 75.0% |
9 / 12 |
signals |
████████████████░░░░ 81.6% |
25,480 / 31,242 |
cdp |
█████████████████░░░ 84.2% |
3,914 / 4,646 |
data_modeling |
█████████████████░░░ 85.8% |
7,846 / 9,147 |
notebooks |
█████████████████░░░ 86.0% |
7,794 / 9,060 |
actions |
█████████████████░░░ 86.6% |
717 / 828 |
wizard |
█████████████████░░░ 86.7% |
1,090 / 1,257 |
managed_warehouse |
█████████████████░░░ 87.1% |
6,217 / 7,139 |
cohorts |
█████████████████░░░ 87.2% |
6,134 / 7,032 |
data_warehouse |
██████████████████░░ 87.7% |
11,203 / 12,767 |
exports |
██████████████████░░ 87.9% |
7,078 / 8,054 |
product_tours |
██████████████████░░ 87.9% |
1,303 / 1,482 |
business_knowledge |
██████████████████░░ 89.0% |
4,384 / 4,928 |
dashboards |
██████████████████░░ 89.4% |
5,983 / 6,693 |
engineering_analytics |
██████████████████░░ 89.4% |
6,575 / 7,355 |
visual_review |
██████████████████░░ 89.5% |
5,870 / 6,558 |
conversations |
██████████████████░░ 90.0% |
18,294 / 20,337 |
alerts |
██████████████████░░ 90.3% |
4,482 / 4,966 |
links |
██████████████████░░ 90.6% |
183 / 202 |
streamlit_apps |
██████████████████░░ 90.7% |
2,630 / 2,901 |
error_tracking |
██████████████████░░ 91.0% |
10,981 / 12,070 |
canvas |
██████████████████░░ 91.1% |
2,018 / 2,216 |
slack_app |
██████████████████░░ 91.2% |
9,801 / 10,749 |
stamphog |
██████████████████░░ 91.3% |
4,505 / 4,936 |
marketing_analytics |
██████████████████░░ 91.6% |
12,736 / 13,901 |
apm |
██████████████████░░ 91.9% |
34 / 37 |
mcp_store |
██████████████████░░ 92.2% |
6,531 / 7,082 |
product_analytics |
███████████████████░ 92.5% |
5,849 / 6,321 |
managed_migrations |
███████████████████░ 92.6% |
1,556 / 1,681 |
ai_observability |
███████████████████░ 92.6% |
16,036 / 17,319 |
notifications |
███████████████████░ 92.6% |
1,017 / 1,098 |
mcp_analytics |
███████████████████░ 92.7% |
3,810 / 4,112 |
early_access_features |
███████████████████░ 92.8% |
1,319 / 1,422 |
surveys |
███████████████████░ 93.2% |
5,853 / 6,281 |
posthog_ai |
███████████████████░ 93.3% |
1,327 / 1,423 |
approvals |
███████████████████░ 93.3% |
3,437 / 3,682 |
reminders |
███████████████████░ 93.4% |
468 / 501 |
web_analytics |
███████████████████░ 93.5% |
15,393 / 16,470 |
legal_documents |
███████████████████░ 93.8% |
1,628 / 1,736 |
workflows |
███████████████████░ 94.1% |
7,747 / 8,232 |
endpoints |
███████████████████░ 94.2% |
8,655 / 9,192 |
tracing |
███████████████████░ 94.5% |
2,671 / 2,827 |
review_hog |
███████████████████░ 94.6% |
8,246 / 8,715 |
skills |
███████████████████░ 94.6% |
3,158 / 3,337 |
messaging |
███████████████████░ 94.7% |
2,885 / 3,048 |
logs |
███████████████████░ 95.5% |
10,699 / 11,199 |
experiments |
███████████████████░ 95.6% |
27,066 / 28,313 |
annotations |
███████████████████░ 96.2% |
732 / 761 |
revenue_analytics |
███████████████████░ 96.3% |
1,887 / 1,960 |
growth |
███████████████████░ 96.3% |
4,767 / 4,949 |
feature_flags |
███████████████████░ 96.4% |
17,488 / 18,144 |
replay_vision |
███████████████████░ 96.4% |
16,666 / 17,281 |
user_interviews |
███████████████████░ 96.5% |
2,638 / 2,734 |
access_control |
███████████████████░ 96.9% |
870 / 898 |
customer_analytics |
███████████████████░ 97.0% |
10,646 / 10,971 |
warehouse_sources |
███████████████████░ 97.3% |
369,178 / 379,250 |
data_catalog |
████████████████████ 97.8% |
2,606 / 2,665 |
analytics_platform |
████████████████████ 98.0% |
2,153 / 2,197 |
metrics |
████████████████████ 98.2% |
2,491 / 2,536 |
pulse |
████████████████████ 98.4% |
2,017 / 2,049 |
live_debugger |
████████████████████ 99.2% |
613 / 618 |
field_notes |
████████████████████ 99.4% |
158 / 159 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
⚠️ Django migration SQL — 1 new migration to review
We've detected new migrations on this PR. Review the SQL output for each migration:
products/cohorts/backend/migrations/0010_alter_cohortbackfillrun_marker_watch.py
BEGIN;
--
-- Alter field marker_watch on cohortbackfillrun
--
-- (no-op)
COMMIT;Last updated: 2026-08-04 17:14 UTC (fd0b7d6)
✅ Django migration risk — migration analysis complete
We've analyzed your migrations for potential risks.
Summary: 0 Safe | 1 Needs Review | 0 Blocked
⚠️ Needs Review
May have performance impact
cohorts.0010_alter_cohortbackfillrun_marker_watch
└─ #1 ⚠️ AlterField
Field alteration may cause table locks or data loss (check if changing type or constraints)
model: cohortbackfillrun, field: marker_watch, field_type: JSONField
Last updated: 2026-08-04 17:15 UTC (fd0b7d6)
62dc71f to
35d833a
Compare
0775015 to
4df7474
Compare
🦔 ReviewHog couldn't finish this reviewThe review run failed partway. It will run again on the next push to this pull request. |
|
😎 Merged successfully - details. |
4df7474 to
0b24fc8
Compare
haacked
left a comment
There was a problem hiding this comment.
Nice cleanup. Good call adding the seed_consumer test that asserts no markers reach the membership topic. One blocking note inline about deploy order.
| .context("joining marker topic verification task")? | ||
| .with_context(|| { | ||
| format!( | ||
| "verifying the marker topic {:?} is reachable. Provision it before arming either \ |
There was a problem hiding this comment.
blocking: If this app rolls before the processor's, every reconcile run in flight settles with zero markers and records "reconcile shortfall: 64/64 partitions still missing" on each of its cohorts. The seeder and the processor are separate ArgoCD apps with no ordering between them, and prod-us has all four reconcile gates on today. Rolling the processor first is harmless, so this is a coin flip rather than a certainty.
Each in-flight run carries a schema-1 watch, fails to parse, and re-dispatches. A still-old processor drains those tiles onto the membership topic and commits, so liveness passes. observe_run then captures ends on the empty marker topic, where they equal the dispatch positions, and caught_up mints a proof on the next tick instead of holding. Master cannot reach this state, because there the watcher tails the same topic the markers land on.
The warning box covers provisioning the topic. Worth adding the gate order too: turn SEEDER_RECONCILE_AUTO_DISPATCH_ENABLED and SEEDER_RECONCILE_OBSERVER_ENABLED off, provision the topic, roll the processor, then re-enable. With both off build_completion returns early, so the seeder does not touch the topic during the window.
There was a problem hiding this comment.
Good catch, but there is nothing is in flight (backfilling is still off by default, it can only be manually triggered from the toolbox), so there’s nothing for a seeder-first roll to re-dispatch.
So not blocking, but I'm still taking the ordering point as a one-liner: rolling the processor first is enough, since the old seeder keeps watching a non-idle topic and holds. Adding that to the warning box.
haacked
left a comment
There was a problem hiding this comment.
Another pass, nothing blocking. The one new thing is on the Python side: the parity command reads reconcile markers out of the membership stream, so moving them off it leaves compare_cohort_membership reporting every cohort as incomplete.
Problem
Reconcile completion markers ride the membership output topic. That is only safe because the processor currently writes to
cohort_membership_changed_shadow, which nothing consumes. Point it at the realcohort_membership_changedand every marker becomes a poison pill. A marker has noperson_idand nostatus, so the CDP membership consumer fails validation and throws, the throw escapes the consumer run loop and shuts the pod down, offsets never commit, and it restarts onto the same batch. That stops all cohort membership ingestion, legacy rows included. The two ClickHouse Kafka engine tables on the same topic get nokafka_skip_broken_messages. The fuse is delayed, since markers only appear once a reconcile completes.The marker watcher also tails the entire membership topic to find 64 rare records per run.
Changes
Markers get their own topic,
cohort_reconcile_markers. The membership topic carries membership rows only.The processor gains a
ReconcileMarkerSinkwith its own producer, kept separate so the membership output can move cluster later without dragging markers with it. A newcohort_reconcile_marker_produce_errors_totalcounter covers the failure branch, which previously incremented nothing while silently holding the seed offset. Startup refuses to boot when the reconcile gate is on and the topic is missing. Markers are keyed{team}:{cohort}:{run}:{partition}, so no cleanup policy can collapse a run's 64 certificates into one, and the marker producer takes a 2smessage.timeout.ms(RECONCILE_MARKER_MESSAGE_TIMEOUT_MS) because the produce runs inline on a partition worker that the drain sweeper retries on a tick.The seeder drops
COHORT_MEMBERSHIP_CHANGED_TOPICentirely and anchors onCOHORT_RECONCILE_MARKERS_TOPIC.marker_watchmoves to schema 2 with atopickey, so a run whose persisted watch names a different topic redispatches instead of resuming on offsets that mean nothing against the new log. Already observed runs are untouched, so no data migration is needed.Warning
cohort_reconcile_markersmust exist in an environment before this deploys there. With thereconcile gate on, the processor refuses to start without it.
Quiesce the seeder across the roll: set
SEEDER_RECONCILE_AUTO_DISPATCH_ENABLEDandSEEDER_RECONCILE_OBSERVER_ENABLEDto false, provision the topic, roll both images, then re-enable. With both offbuild_completionreturns early, so the seeder touches neither topic during the window.Ordering alone does not cover a run in flight, in either direction.
caught_upmeasures the watcher against the ends captured once at the liveness pass, not against the live high watermark, so a seeder watching one topic while the processor produces to the other reaches those ends, settles with zero markers, and stampsreconcile_observed_at. Observed runs leave discovery, so recovering one takes a manual re-dispatch. No run is in flight today, since backfills are triggered by hand, so this is a precaution rather than a blocker.How did you test this code?
Claude Code ran everything below against the local dev stack.
The
seed_consumerrouting test now asserts markers land on the marker topic and that zeroreconcile_completerecords reach the membership topic, the exact regression this PR exists to prevent and which nothing covered before. I confirmed it by routing markers back onto the membership topic and watching it fail. Adomain/completion.rsunit test and a Postgres-backed case inpg_completion.rsassert a watch anchored to another topic classifies asTopicChanged, catching a silent read of offsets against the wrong log.Docs update
No.