From 24d3d0de373a68d72930f880700e28813b3c8f66 Mon Sep 17 00:00:00 2001 From: Matheus Veras Batista Date: Fri, 31 Jul 2026 18:37:03 -0300 Subject: [PATCH 1/4] give reconcile-completion markers their own kafka topic --- ...10_alter_cohortbackfillrun_marker_watch.py | 21 +++ .../backend/migrations/max_migration.txt | 2 +- products/cohorts/backend/models/backfill.py | 2 +- rust/cohort-core/src/seed/reconcile.rs | 6 +- rust/cohort-seeder/src/app/completion.rs | 47 +++-- rust/cohort-seeder/src/app/observe.rs | 39 ++-- rust/cohort-seeder/src/app/watch.rs | 66 +++---- .../src/bin/reconcile_dispatch.rs | 2 +- rust/cohort-seeder/src/config.rs | 10 +- rust/cohort-seeder/src/domain/completion.rs | 133 +++++++++----- rust/cohort-seeder/src/domain/ledger.rs | 8 +- rust/cohort-seeder/src/domain/mod.rs | 4 +- rust/cohort-seeder/src/kafka/markers.rs | 83 +++++---- rust/cohort-seeder/src/kafka/mod.rs | 2 +- rust/cohort-seeder/src/kafka/producer.rs | 23 ++- rust/cohort-seeder/src/main.rs | 30 ++-- .../src/observability/metrics.rs | 2 + rust/cohort-seeder/src/store/completion.rs | 25 ++- rust/cohort-seeder/tests/pg_completion.rs | 54 +++--- rust/cohort-stream-processor/src/config.rs | 6 + .../src/consumers/events.rs | 9 +- rust/cohort-stream-processor/src/main.rs | 26 ++- .../src/observability/metrics.rs | 8 + .../src/producer/kafka.rs | 57 +----- .../src/producer/marker.rs | 153 ++++++++++++++++ .../src/producer/mod.rs | 84 +-------- .../src/workers/reconcile.rs | 144 +++++++-------- .../src/workers/seed_path.rs | 14 +- .../src/workers/worker.rs | 13 +- .../tests/events_consumer.rs | 11 -- .../tests/merge_durability.rs | 9 +- .../tests/seed_consumer.rs | 166 ++++++++++-------- .../tests/sweep_worker.rs | 12 +- 33 files changed, 708 insertions(+), 563 deletions(-) create mode 100644 products/cohorts/backend/migrations/0010_alter_cohortbackfillrun_marker_watch.py create mode 100644 rust/cohort-stream-processor/src/producer/marker.rs diff --git a/products/cohorts/backend/migrations/0010_alter_cohortbackfillrun_marker_watch.py b/products/cohorts/backend/migrations/0010_alter_cohortbackfillrun_marker_watch.py new file mode 100644 index 000000000000..d46b7ebf9bdc --- /dev/null +++ b/products/cohorts/backend/migrations/0010_alter_cohortbackfillrun_marker_watch.py @@ -0,0 +1,21 @@ +# Generated by Django 5.2.14 on 2026-07-31 20:33 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("cohorts", "0009_cohort_backfill_per_kind_uniqueness"), + ] + + operations = [ + migrations.AlterField( + model_name="cohortbackfillrun", + name="marker_watch", + field=models.JSONField( + blank=True, + help_text='Opaque watcher resume state written by the Rust seeder, shape {"schema":2,"topic":"...","positions":{...},"ends":{...}|null}. Django never interprets it.', + null=True, + ), + ), + ] diff --git a/products/cohorts/backend/migrations/max_migration.txt b/products/cohorts/backend/migrations/max_migration.txt index 2b9921d527b8..11eac0a4a9bf 100644 --- a/products/cohorts/backend/migrations/max_migration.txt +++ b/products/cohorts/backend/migrations/max_migration.txt @@ -1 +1 @@ -0009_cohort_backfill_per_kind_uniqueness +0010_alter_cohortbackfillrun_marker_watch diff --git a/products/cohorts/backend/models/backfill.py b/products/cohorts/backend/models/backfill.py index 1abc804de9e7..2a4389be00f1 100644 --- a/products/cohorts/backend/models/backfill.py +++ b/products/cohorts/backend/models/backfill.py @@ -79,7 +79,7 @@ class CohortBackfillRun(TeamScopedRootMixin, UUIDTModel): blank=True, help_text=( "Opaque watcher resume state written by the Rust seeder, shape " - '{"schema":1,"positions":{...},"ends":{...}|null}. Django never interprets it.' + '{"schema":2,"topic":"...","positions":{...},"ends":{...}|null}. Django never interprets it.' ), ) blocked_reason = models.TextField(blank=True, default="") diff --git a/rust/cohort-core/src/seed/reconcile.rs b/rust/cohort-core/src/seed/reconcile.rs index 80d34e2e1e88..8994b4982e5a 100644 --- a/rust/cohort-core/src/seed/reconcile.rs +++ b/rust/cohort-core/src/seed/reconcile.rs @@ -309,9 +309,9 @@ fn deserialize_schema_version<'de, D: Deserializer<'de>>(deserializer: D) -> Res pub(super) const RECONCILE_COMPLETE_KIND: &str = "reconcile_complete"; /// A completion certificate emitted after one partition's reconcile snapshot is durable. Produced by -/// the stream processor onto the membership topic and folded by the seeder's marker watcher, so it -/// lives here — the shared seed contract — rather than in either crate. Field order is the wire order; -/// the golden test below pins the exact bytes both ends depend on. +/// the stream processor onto the dedicated reconcile-marker topic and folded by the seeder's marker +/// watcher, so it lives here — the shared seed contract — rather than in either crate. Field order is +/// the wire order; the golden test below pins the exact bytes both ends depend on. /// /// Deliberately carries no dispatch epoch. A marker is a run-scoped durable fact: that partition's /// snapshot drained under this run's pinned filters. If a re-dispatch's watcher folds a late marker diff --git a/rust/cohort-seeder/src/app/completion.rs b/rust/cohort-seeder/src/app/completion.rs index cc32cb46f63b..215490e6c2ea 100644 --- a/rust/cohort-seeder/src/app/completion.rs +++ b/rust/cohort-seeder/src/app/completion.rs @@ -157,7 +157,7 @@ enum DispatchKind { struct DispatchArm { pool: PgPool, producer: SeedTileProducer, - membership_topic: Arc, + marker_topic: Arc, max_inflight: NonZeroUsize, /// Caps how many dispatch tasks this replica runs at once. Every task produces /// `cohorts × COHORT_PARTITION_COUNT` tiles through the producer the chunk pipeline shares, so @@ -185,6 +185,9 @@ pub struct CompletionDriver { /// The backfill kinds discovery binds. `[Behavioral]` unless the person seed path is on, which /// keeps a person run invisible to this protocol while its gate is off. kinds: &'static [RunKind], + /// The marker topic every persisted watch is anchored to. A run whose record names a different + /// one classifies as [`UndispatchedReason::TopicChanged`] and re-dispatches. + marker_topic: String, dispatch: Option, observe: Option, in_flight: Arc>>, @@ -196,11 +199,17 @@ pub struct CompletionDriver { impl CompletionDriver { /// Base driver with neither half armed. `main` adds the halves the config enables. - pub fn new(pool: PgPool, allowlist: TeamAllowlist, kinds: &'static [RunKind]) -> Self { + pub fn new( + pool: PgPool, + allowlist: TeamAllowlist, + kinds: &'static [RunKind], + marker_topic: String, + ) -> Self { Self { pool, allowlist, kinds, + marker_topic, dispatch: None, observe: None, in_flight: Arc::new(Mutex::new(HashSet::new())), @@ -212,7 +221,6 @@ impl CompletionDriver { pub fn with_dispatch( mut self, producer: SeedTileProducer, - membership_topic: String, max_inflight: NonZeroUsize, max_concurrent_dispatches: NonZeroUsize, register_backfill: RegisterBackfillConfirmation, @@ -220,7 +228,7 @@ impl CompletionDriver { self.dispatch = Some(DispatchArm { pool: self.pool.clone(), producer, - membership_topic: Arc::from(membership_topic), + marker_topic: Arc::from(self.marker_topic.as_str()), max_inflight, dispatch_slots: Arc::new(Semaphore::new(max_concurrent_dispatches.get())), register_backfill, @@ -237,7 +245,10 @@ impl CompletionDriver { directives: watch::Sender, ) -> Self { self.observe = Some(ObserveArm { - store: Arc::new(PgObservationStore::new(self.pool.clone())), + store: Arc::new(PgObservationStore::new( + self.pool.clone(), + self.marker_topic.clone(), + )), committed, topic_ends, directives, @@ -249,13 +260,16 @@ impl CompletionDriver { /// and observe the reconciling ones (observe arm). A discovery failure is logged and retried next /// tick; the dark path (driver absent) costs zero queries. pub async fn tick(&self) { - let discovered = match discover_completions(&self.pool, &self.allowlist, self.kinds).await { - Ok(discovered) => discovered, - Err(error) => { - warn!(error = %error, "completion discovery failed"); - return; - } - }; + let discovered = + match discover_completions(&self.pool, &self.allowlist, self.kinds, &self.marker_topic) + .await + { + Ok(discovered) => discovered, + Err(error) => { + warn!(error = %error, "completion discovery failed"); + return; + } + }; let now = Utc::now(); // Per kind, not summed: "person runs are piling up undispatched" is the failure this protocol @@ -753,7 +767,7 @@ async fn run_dispatch(context: &DispatchArm, run_id: RunId, run_kind: RunKind, k match dispatch_and_record( &context.pool, &context.producer, - &context.membership_topic, + &context.marker_topic, context.max_inflight, KAFKA_METADATA_TIMEOUT, certified, @@ -802,7 +816,7 @@ pub struct RecordedDispatch { pub async fn dispatch_and_record( pool: &PgPool, producer: &SeedTileProducer, - membership_topic: &str, + marker_topic: &str, max_inflight: NonZeroUsize, metadata_timeout: Duration, certified: CertifiedDispatch, @@ -810,7 +824,7 @@ pub async fn dispatch_and_record( ) -> Result { let positions = { let producer = producer.clone(); - let topic = membership_topic.to_string(); + let topic = marker_topic.to_string(); tokio::task::spawn_blocking(move || { producer.capture_topic_offsets(&topic, metadata_timeout) }) @@ -830,6 +844,7 @@ pub async fn dispatch_and_record( let hwms = ReconcileHwms::try_from(&receipt).map_err(DispatchRecordError::Hwms)?; let watch = MarkerWatch { + topic: marker_topic.to_string(), positions, ends: None, }; @@ -857,7 +872,7 @@ impl TryFrom<&ReconcileDispatchReceipt> for ReconcileHwms { pub enum DispatchRecordError { #[error("joining the offset-capture task")] CaptureTask(#[source] JoinError), - #[error("capturing the membership topic start positions")] + #[error("capturing the marker topic start positions")] CaptureOffsets(#[source] CaptureOffsetsError), #[error("producing the reconcile control tiles")] Execute(#[source] ReconcileDispatchError), diff --git a/rust/cohort-seeder/src/app/observe.rs b/rust/cohort-seeder/src/app/observe.rs index f4a638f7bcf9..b4f469b173e6 100644 --- a/rust/cohort-seeder/src/app/observe.rs +++ b/rust/cohort-seeder/src/app/observe.rs @@ -3,7 +3,7 @@ //! Outcomes land before observed: every non-superseded participation gets a definitive outcome //! write before `reconcile_observed_at` is stamped, which is this pass's last //! write. The marker set is the completion *authority* (bitmaps decide complete vs short); the seed -//! group's committed offsets are *liveness only* (they gate whether we may even capture the membership +//! group's committed offsets are *liveness only* (they gate whether we may even capture the marker-topic //! end-watermarks that a negative verdict's settlement proof requires). Hash attribution splits a //! shortfall into terminal (the cohort diverged or was deleted — supersede) vs retryable (the hash //! still matches — a gate-off or lossy fleet, error only). A [`CompletionStoreError::CompletionFenceLost`] @@ -62,7 +62,7 @@ pub enum ObserveStep { ObservedAllComplete, /// The seed group has not drained the reconcile records; the run holds for the next tick. LivenessLagging(usize), - /// Liveness passed and the membership end-watermarks were captured; observation resumes next tick. + /// Liveness passed and the marker-topic end-watermarks were captured; observation resumes next tick. EndsCaptured, /// Liveness passed earlier but the marker watcher has not read to the captured ends. MarkerLagging(usize), @@ -284,7 +284,7 @@ pub trait CommittedOffsetSource: Send + Sync { async fn committed(&self) -> Result; } -/// The membership topic's current end-watermarks — captured at the liveness pass to bound the marker +/// The marker topic's current end-watermarks — captured at the liveness pass to bound the marker /// set of this dispatch. #[async_trait] pub trait TopicOffsetSource: Send + Sync { @@ -342,11 +342,12 @@ pub trait ObservationStore: Send + Sync { /// The real observation store over the seeder's pool. pub struct PgObservationStore { pool: PgPool, + marker_topic: String, } impl PgObservationStore { - pub fn new(pool: PgPool) -> Self { - Self { pool } + pub fn new(pool: PgPool, marker_topic: String) -> Self { + Self { pool, marker_topic } } } @@ -375,7 +376,7 @@ impl ObservationStore for PgObservationStore { epoch: DispatchEpoch, ends: &ObservationEnds, ) -> Result<(), CompletionStoreError> { - persist_observation_ends(&self.pool, run_id, epoch, ends).await + persist_observation_ends(&self.pool, run_id, epoch, ends, &self.marker_topic).await } async fn mark_completed( @@ -432,7 +433,7 @@ impl SourceError { pub enum ObserveError { #[error("reading the seed group's committed offsets")] Committed(#[source] SourceError), - #[error("capturing the membership end watermarks")] + #[error("capturing the marker-topic end watermarks")] TopicEnds(#[source] SourceError), #[error(transparent)] Store(#[from] CompletionStoreError), @@ -457,7 +458,7 @@ impl CommittedOffsetSource for KafkaCommittedOffsets { } } -/// The real topic-offset seam: the seed producer's client fetches the membership topic's watermarks. +/// The real topic-offset seam: the seed producer's client fetches the marker topic's watermarks. pub struct KafkaTopicOffsets { producer: SeedTileProducer, topic: String, @@ -484,7 +485,7 @@ impl TopicOffsetSource for KafkaTopicOffsets { tokio::task::spawn_blocking(move || producer.capture_topic_offsets(&topic, timeout)) .await .map_err(|error| { - SourceError::new(format!("join membership watermark task: {error}")) + SourceError::new(format!("join marker-topic watermark task: {error}")) })? .map_err(|error| SourceError::new(render_error_chain(&error)))?; Ok(ObservationEnds::from_positions(&positions)) @@ -501,8 +502,8 @@ mod tests { use uuid::Uuid; use crate::domain::{ - CommittedOffset, MarkerPartition, MembershipPartition, NextOffset, ProducedOffset, - SeedPartition, WatchPositions, + CommittedOffset, MarkerPartition, NextOffset, ProducedOffset, SeedPartition, + WatchPartition, WatchPositions, }; use crate::store::completion::CompletionOperation; @@ -579,6 +580,7 @@ mod tests { epoch: epoch(), hwms: full_hwms(), watch: MarkerWatch { + topic: "cohort_reconcile_markers".to_string(), positions: watched_positions(), ends, }, @@ -588,29 +590,20 @@ mod tests { /// Where the watcher has read to. Past [`caught_up_ends`], short of [`pending_ends`]. fn watched_positions() -> WatchPositions { let mut positions = WatchPositions::new(); - positions.insert( - MembershipPartition::new(0), - NextOffset::from_high_watermark(10), - ); + positions.insert(WatchPartition::new(0), NextOffset::from_high_watermark(10)); positions } /// Ends the watcher's positions already satisfy, so settlement is reachable. fn caught_up_ends() -> ObservationEnds { let mut ends = ObservationEnds::new(); - ends.insert( - MembershipPartition::new(0), - NextOffset::from_high_watermark(10), - ); + ends.insert(WatchPartition::new(0), NextOffset::from_high_watermark(10)); ends } fn pending_ends() -> ObservationEnds { let mut ends = ObservationEnds::new(); - ends.insert( - MembershipPartition::new(0), - NextOffset::from_high_watermark(20), - ); + ends.insert(WatchPartition::new(0), NextOffset::from_high_watermark(20)); ends } diff --git a/rust/cohort-seeder/src/app/watch.rs b/rust/cohort-seeder/src/app/watch.rs index ad074380694f..f3d1ecb9e01c 100644 --- a/rust/cohort-seeder/src/app/watch.rs +++ b/rust/cohort-seeder/src/app/watch.rs @@ -1,6 +1,6 @@ //! The single global marker-watch task — the seeder's second lifecycle component. //! -//! One dedicated consumer tails the whole membership topic, folding every observed +//! One dedicated consumer tails the whole marker topic, folding every observed //! `reconcile_complete` marker into the per-run [`MarkerLedger`] it names. Bits and the watcher's //! resume positions are flushed through the fenced [`persist_marker_observations`] on a cadence //! (every persist interval), a batch cap (every N consumed messages), or immediately when a cohort's @@ -26,8 +26,8 @@ use tokio::sync::watch; use tracing::{info, warn}; use crate::domain::{ - DispatchEpoch, MarkerFold, MarkerLedger, MembershipPartition, NextOffset, PartitionBitmap, - RunId, WatchPositions, + DispatchEpoch, MarkerFold, MarkerLedger, NextOffset, PartitionBitmap, RunId, WatchPartition, + WatchPositions, }; use crate::kafka::markers::{MarkerWatcher, WatchError, WatchItem}; use crate::observability::metrics::{RECONCILE_MARKERS_OBSERVED, RECONCILE_WATCH_TRUNCATED}; @@ -50,7 +50,7 @@ pub struct WatchDirectives { pub runs: Vec, } -/// The membership-topic consumer seam. Real impl: [`MarkerWatcher`]. +/// The marker-topic consumer seam. Real impl: [`MarkerWatcher`]. #[async_trait] pub trait MarkerStream: Send { /// Re-assign the stream to read from `start` on every named partition. The watch state decides @@ -93,11 +93,12 @@ pub trait MarkerFlush: Send { /// The real flush: the epoch-fenced OR-merge of bits plus the watcher positions. pub struct PgMarkerFlush { pool: PgPool, + marker_topic: String, } impl PgMarkerFlush { - pub fn new(pool: PgPool) -> Self { - Self { pool } + pub fn new(pool: PgPool, marker_topic: String) -> Self { + Self { pool, marker_topic } } } @@ -110,7 +111,15 @@ impl MarkerFlush for PgMarkerFlush { bits: &[(CohortId, PartitionBitmap)], positions: &WatchPositions, ) -> Result<(), CompletionStoreError> { - persist_marker_observations(&self.pool, run_id, epoch, bits, positions).await + persist_marker_observations( + &self.pool, + run_id, + epoch, + bits, + positions, + &self.marker_topic, + ) + .await } } @@ -140,13 +149,13 @@ impl RunWatch { } /// What the stream has read since coverage was last folded into the watched runs. Held once for the -/// whole task rather than advanced into every run per record: the membership topic is high-volume and -/// the watched-run set has no upper bound, so per-record work must not scale with it. +/// whole task rather than advanced into every run per record: the watched-run set has no upper bound, +/// so per-record work must not scale with it. #[derive(Default)] -struct StreamCoverage(BTreeMap); +struct StreamCoverage(BTreeMap); impl StreamCoverage { - fn advance(&mut self, partition: MembershipPartition, next: NextOffset) { + fn advance(&mut self, partition: WatchPartition, next: NextOffset) { let slot = self.0.entry(partition).or_insert(next); if next > *slot { *slot = next; @@ -349,7 +358,7 @@ impl WatchState { requested, low, }) => { - let partition = MembershipPartition::new(partition); + let partition = WatchPartition::new(partition); // Drop against the log's low watermark, not just the rejected start: every run // below it is equally unreadable, so one pass clears them all instead of one // watermark round-trip per coverage level. @@ -379,7 +388,7 @@ impl WatchState { run_id = ?run_id, partition = partition.get(), low, - "membership topic truncated past this run's watch start; dropping (re-dispatch to recover)" + "marker topic truncated past this run's watch start; dropping (re-dispatch to recover)" ); if let Some(run) = self.runs.remove(&run_id) { self.truncated.insert(run_id, run.epoch); @@ -533,7 +542,7 @@ impl MarkerWatchTask { self.handle.report_healthy(); } Err(error) => { - warn!(error = %error, "receiving from the membership topic failed; backing off"); + warn!(error = %error, "receiving from the marker topic failed; backing off"); self.state.request_seek().await; self.handle.report_healthy(); tokio::time::sleep(RECV_BACKOFF).await; @@ -578,7 +587,7 @@ mod tests { fn start_at(partition: i32, offset: i64) -> WatchPositions { let mut positions = WatchPositions::new(); positions.insert( - MembershipPartition::new(partition), + WatchPartition::new(partition), NextOffset::from_high_watermark(offset), ); positions @@ -609,7 +618,7 @@ mod tests { fn marker_item(partition: i32, offset: i64, run_id: RunId, cohort: i32, bit: u32) -> WatchItem { WatchItem { - partition: MembershipPartition::new(partition), + partition: WatchPartition::new(partition), next_offset: NextOffset::from_high_watermark(offset), marker: Some(ObservedMarker { team_id: TeamId(2), @@ -739,7 +748,7 @@ mod tests { // Two non-marker rows: below the batch cap, no flush. for offset in 1..=2 { state.ingest(WatchItem { - partition: MembershipPartition::new(0), + partition: WatchPartition::new(0), next_offset: NextOffset::from_high_watermark(offset), marker: None, }); @@ -812,7 +821,7 @@ mod tests { .await; state.ingest(WatchItem { - partition: MembershipPartition::new(0), + partition: WatchPartition::new(0), next_offset: NextOffset::from_high_watermark(1), marker: None, }); @@ -843,7 +852,7 @@ mod tests { for offset in 1..=100 { state.ingest(WatchItem { - partition: MembershipPartition::new(0), + partition: WatchPartition::new(0), next_offset: NextOffset::from_high_watermark(offset), marker: None, }); @@ -865,7 +874,7 @@ mod tests { .iter() .filter(|call| call.run_id == run_id) .next_back() - .and_then(|call| call.positions.get(MembershipPartition::new(0))) + .and_then(|call| call.positions.get(WatchPartition::new(0))) }; assert_eq!( persisted(early), @@ -992,7 +1001,7 @@ mod tests { .await; assert_eq!(state.stream.seeks.len(), 2); assert_eq!( - state.stream.seeks[1].get(MembershipPartition::new(0)), + state.stream.seeks[1].get(WatchPartition::new(0)), Some(NextOffset::from_high_watermark(50)), "the seek rewinds to the earliest watched start" ); @@ -1032,14 +1041,11 @@ mod tests { #[tokio::test] async fn flush_persists_start_coverage_for_idle_partitions() { // The directive's start covers two partitions; traffic arrives on only one. The flush must - // still persist the idle partition at its dispatch start, or an idle membership partition + // still persist the idle partition at its dispatch start, or an idle marker partition // would hold `caught_up` (and every negative-verdict settlement) open forever. let run_id = run(1); let mut start = start_at(0, 10); - start.insert( - MembershipPartition::new(1), - NextOffset::from_high_watermark(7), - ); + start.insert(WatchPartition::new(1), NextOffset::from_high_watermark(7)); let mut state = make_state(FakeFlush::default(), 10_000); state .apply_directives(&WatchDirectives { @@ -1053,12 +1059,12 @@ mod tests { let calls = state.flush.calls(); assert_eq!(calls.len(), 1); assert_eq!( - calls[0].positions.get(MembershipPartition::new(0)), + calls[0].positions.get(WatchPartition::new(0)), Some(NextOffset::from_high_watermark(11)), "the read partition advances to the ingested record's next offset" ); assert_eq!( - calls[0].positions.get(MembershipPartition::new(1)), + calls[0].positions.get(WatchPartition::new(1)), Some(NextOffset::from_high_watermark(7)), "the idle partition stays covered at its dispatch start" ); @@ -1085,9 +1091,7 @@ mod tests { "a record read on the stale assignment is not folded" ); assert_eq!( - state.runs[&run_id] - .positions - .get(MembershipPartition::new(0)), + state.runs[&run_id].positions.get(WatchPartition::new(0)), Some(NextOffset::from_high_watermark(10)), "coverage stays at the dispatch start until the seek lands" ); diff --git a/rust/cohort-seeder/src/bin/reconcile_dispatch.rs b/rust/cohort-seeder/src/bin/reconcile_dispatch.rs index a77c4e43ea4c..5faaba856aad 100644 --- a/rust/cohort-seeder/src/bin/reconcile_dispatch.rs +++ b/rust/cohort-seeder/src/bin/reconcile_dispatch.rs @@ -153,7 +153,7 @@ async fn async_main(args: Args, config: Config) -> Result<()> { let recorded = dispatch_and_record( &pool, &producer, - &config.cohort_membership_changed_topic, + &config.cohort_reconcile_markers_topic, max_inflight, PARTITION_VERIFY_TIMEOUT, certified, diff --git a/rust/cohort-seeder/src/config.rs b/rust/cohort-seeder/src/config.rs index 3b30dce701d0..1b415e414010 100644 --- a/rust/cohort-seeder/src/config.rs +++ b/rust/cohort-seeder/src/config.rs @@ -269,10 +269,10 @@ pub struct Config { #[envconfig(default = "4")] pub seeder_reconcile_max_concurrent_dispatches: usize, - /// The membership-change topic whose high watermarks anchor the marker watcher's start - /// positions, captured at dispatch time. The observer reads markers from the same topic. - #[envconfig(default = "cohort_membership_changed_shadow")] - pub cohort_membership_changed_topic: String, + /// The reconcile-marker topic whose high watermarks anchor the marker watcher's start positions, + /// captured at dispatch time. The observer reads markers from the same topic. + #[envconfig(default = "cohort_reconcile_markers")] + pub cohort_reconcile_markers_topic: String, /// Enable the dark-by-default reconcile observer: the marker-watch task and the driver's /// observation pass. A separate gate from auto-dispatch — observation can run against @@ -285,7 +285,7 @@ pub struct Config { #[envconfig(default = "cohort-stream-seeds")] pub kafka_seed_consumer_group: String, - /// Timeout for the seed-group OffsetFetch and membership-topic watermark metadata calls the + /// Timeout for the seed-group OffsetFetch and marker-topic watermark metadata calls the /// observer makes. #[envconfig(default = "10000")] pub seeder_reconcile_offsets_timeout_ms: u64, diff --git a/rust/cohort-seeder/src/domain/completion.rs b/rust/cohort-seeder/src/domain/completion.rs index 6f2ee20eeeb0..b752fa5a6de9 100644 --- a/rust/cohort-seeder/src/domain/completion.rs +++ b/rust/cohort-seeder/src/domain/completion.rs @@ -31,7 +31,7 @@ const COMPLETE_MASK: u64 = if COHORT_PARTITION_COUNT == 64 { /// The JSONB schema tag for the `marker_watch` column. Bumping it must accompany a reader that /// rejects the old shape; the current reader rejects anything but this value. -pub(crate) const MARKER_WATCH_SCHEMA: u32 = 1; +pub(crate) const MARKER_WATCH_SCHEMA: u32 = 2; /// A partition index proven `< COHORT_PARTITION_COUNT` at construction, so `1 << index` into the /// bitmap is always in range. @@ -262,11 +262,11 @@ pub enum ReconcileHwmsError { UnexpectedPartitions { got: usize, expected: usize }, } -/// A membership-topic partition the marker watcher reads. +/// A marker-topic partition the marker watcher reads. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] -pub struct MembershipPartition(i32); +pub struct WatchPartition(i32); -impl MembershipPartition { +impl WatchPartition { pub const fn new(partition: i32) -> Self { Self(partition) } @@ -276,7 +276,7 @@ impl MembershipPartition { } } -/// The next offset the watcher must read on a membership partition. It is minted from a high +/// The next offset the watcher must read on a marker partition. It is minted from a high /// watermark (the offset the next record will receive), never from a consumed offset, so the /// classic "did I store the offset I read or the next one" off-by-one cannot be constructed. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] @@ -292,20 +292,20 @@ impl NextOffset { } } -/// Per-membership-partition resume positions for the marker watcher. +/// Per-partition resume positions for the marker watcher. #[derive(Debug, Clone, Default, PartialEq, Eq)] -pub struct WatchPositions(BTreeMap); +pub struct WatchPositions(BTreeMap); impl WatchPositions { pub fn new() -> Self { Self(BTreeMap::new()) } - pub fn insert(&mut self, partition: MembershipPartition, offset: NextOffset) { + pub fn insert(&mut self, partition: WatchPartition, offset: NextOffset) { self.0.insert(partition, offset); } - pub fn get(&self, partition: MembershipPartition) -> Option { + pub fn get(&self, partition: WatchPartition) -> Option { self.0.get(&partition).copied() } @@ -313,7 +313,7 @@ impl WatchPositions { self.0.is_empty() } - pub fn iter(&self) -> impl Iterator + '_ { + pub fn iter(&self) -> impl Iterator + '_ { self.0 .iter() .map(|(&partition, &offset)| (partition, offset)) @@ -328,7 +328,7 @@ impl WatchPositions { /// from the beginning; inserting it would claim coverage of everything below `offset`. Leaving /// it absent keeps [`ObservationEnds::caught_up`] fail-closed — the run holds until a /// re-dispatch recaptures a start position for it. - pub fn advance(&mut self, partition: MembershipPartition, offset: NextOffset) { + pub fn advance(&mut self, partition: WatchPartition, offset: NextOffset) { if let Some(current) = self.0.get_mut(&partition) { if offset > *current { *current = offset; @@ -349,23 +349,23 @@ impl<'de> Deserialize<'de> for WatchPositions { } } -/// The membership-topic end watermarks captured at the moment liveness passed. Because markers are +/// The marker-topic end watermarks captured at the moment liveness passed. Because markers are /// acknowledged before the seed group commits its offset, every marker of the dispatch sits below /// these ends; a watcher that has read up to them has seen all of them. #[derive(Debug, Clone, Default, PartialEq, Eq)] -pub struct ObservationEnds(BTreeMap); +pub struct ObservationEnds(BTreeMap); impl ObservationEnds { pub fn new() -> Self { Self(BTreeMap::new()) } - pub fn insert(&mut self, partition: MembershipPartition, offset: NextOffset) { + pub fn insert(&mut self, partition: WatchPartition, offset: NextOffset) { self.0.insert(partition, offset); } - /// The membership end-watermarks captured at the liveness pass have the same shape as the - /// watcher's start positions — both are next-to-read offsets keyed by membership partition. + /// The end-watermarks captured at the liveness pass have the same shape as the watcher's start + /// positions — both are next-to-read offsets keyed by marker-topic partition. pub fn from_positions(positions: &WatchPositions) -> Self { Self(positions.0.clone()) } @@ -419,9 +419,12 @@ impl<'de> Deserialize<'de> for ObservationEnds { pub struct SettleProof(()); /// The resumable watcher state persisted as `marker_watch` JSONB: -/// `{"schema":1,"positions":{...},"ends":{...}|null}`. `ends` is `Some` iff liveness has passed. +/// `{"schema":2,"topic":"…","positions":{...},"ends":{...}|null}`. `ends` is `Some` iff liveness has +/// passed. `topic` anchors the offsets to the log they were read from — they are meaningless against +/// any other. #[derive(Debug, Clone, PartialEq, Eq)] pub struct MarkerWatch { + pub topic: String, pub positions: WatchPositions, pub ends: Option, } @@ -430,6 +433,7 @@ pub struct MarkerWatch { struct MarkerWatchRepr { #[serde(deserialize_with = "deserialize_marker_watch_schema")] schema: u32, + topic: String, positions: WatchPositions, ends: Option, } @@ -438,6 +442,7 @@ impl Serialize for MarkerWatch { fn serialize(&self, serializer: S) -> Result { MarkerWatchRepr { schema: MARKER_WATCH_SCHEMA, + topic: self.topic.clone(), positions: self.positions.clone(), ends: self.ends.clone(), } @@ -449,6 +454,7 @@ impl<'de> Deserialize<'de> for MarkerWatch { fn deserialize>(deserializer: D) -> Result { let repr = MarkerWatchRepr::deserialize(deserializer)?; Ok(Self { + topic: repr.topic, positions: repr.positions, ends: repr.ends, }) @@ -519,6 +525,9 @@ pub enum UndispatchedReason { MissingRecord, /// The dispatch record is present but does not parse under the current schema. UnparseableRecord, + /// The persisted watch is anchored to a different topic than the one configured, so its offsets + /// name positions in a log this run no longer reads. + TopicChanged, } /// The classification of one discovered completion row. Parse-don't-validate: the app matches this @@ -541,17 +550,19 @@ pub enum CompletionPhase { /// The raw column values of one discovered completion row. #[derive(Debug, Clone)] -pub struct CompletionParts { +pub struct CompletionParts<'a> { pub status: CompletionStatus, pub chunks_planned_at: Option>, pub reconcile_dispatched_at: Option>, pub reconcile_observed_at: Option>, pub reconcile_hwms: Option, pub marker_watch: Option, + /// The marker topic this process reads. A persisted watch anchored elsewhere is stale. + pub expected_marker_topic: &'a str, } impl CompletionPhase { - pub fn from_parts(parts: CompletionParts) -> Self { + pub fn from_parts(parts: CompletionParts<'_>) -> Self { match parts.status { CompletionStatus::Seeding => { if parts.reconcile_dispatched_at.is_some() @@ -575,6 +586,7 @@ impl CompletionPhase { parts.reconcile_dispatched_at, parts.reconcile_hwms, parts.marker_watch, + parts.expected_marker_topic, ) { Ok(dispatched) => Self::Reconciling(dispatched), Err(reason) => Self::ReconcilingUndispatched(reason), @@ -588,6 +600,7 @@ fn parse_dispatched( dispatched_at: Option>, hwms: Option, watch: Option, + expected_marker_topic: &str, ) -> Result { let dispatched_at = dispatched_at.ok_or(UndispatchedReason::NeverDispatched)?; let hwms = hwms.ok_or(UndispatchedReason::MissingRecord)?; @@ -596,6 +609,9 @@ fn parse_dispatched( .map_err(|_| UndispatchedReason::UnparseableRecord)?; let watch = serde_json::from_value::(watch) .map_err(|_| UndispatchedReason::UnparseableRecord)?; + if watch.topic != expected_marker_topic { + return Err(UndispatchedReason::TopicChanged); + } Ok(DispatchedReconcile { epoch: DispatchEpoch::from_dispatched_at(dispatched_at), hwms, @@ -609,7 +625,7 @@ fn canonical_partitions() -> impl Iterator { } fn serialize_offset_map( - map: &BTreeMap, + map: &BTreeMap, serializer: S, ) -> Result { let mut serialized = serializer.serialize_map(Some(map.len()))?; @@ -621,13 +637,13 @@ fn serialize_offset_map( fn deserialize_offset_map<'de, D: Deserializer<'de>>( deserializer: D, -) -> Result, D::Error> { +) -> Result, D::Error> { let raw = BTreeMap::::deserialize(deserializer)?; Ok(raw .into_iter() .map(|(partition, offset)| { ( - MembershipPartition::new(partition), + WatchPartition::new(partition), NextOffset::from_high_watermark(offset), ) }) @@ -640,6 +656,8 @@ mod tests { use chrono::TimeZone; use uuid::Uuid; + const TOPIC: &str = "cohort_reconcile_markers"; + fn full_hwms(base: i64) -> ReconcileHwms { let offsets = canonical_partitions() .map(|partition| { @@ -774,8 +792,8 @@ mod tests { fn advance_is_monotone_and_ignores_partitions_it_does_not_track() { // Both guards feed `caught_up`. Regressing a position would rewind persisted coverage; // inserting an untracked partition would claim coverage of everything below `offset`. - let tracked = MembershipPartition::new(0); - let untracked = MembershipPartition::new(1); + let tracked = WatchPartition::new(0); + let untracked = WatchPartition::new(1); let mut positions = WatchPositions::new(); positions.insert(tracked, NextOffset::from_high_watermark(10)); @@ -803,23 +821,14 @@ mod tests { #[test] fn caught_up_passes_at_the_end_and_fails_one_short() { let mut ends = ObservationEnds::new(); - ends.insert( - MembershipPartition::new(0), - NextOffset::from_high_watermark(10), - ); + ends.insert(WatchPartition::new(0), NextOffset::from_high_watermark(10)); let mut at_end = WatchPositions::new(); - at_end.insert( - MembershipPartition::new(0), - NextOffset::from_high_watermark(10), - ); + at_end.insert(WatchPartition::new(0), NextOffset::from_high_watermark(10)); assert!(ends.caught_up(&at_end).is_some()); let mut short = WatchPositions::new(); - short.insert( - MembershipPartition::new(0), - NextOffset::from_high_watermark(9), - ); + short.insert(WatchPartition::new(0), NextOffset::from_high_watermark(9)); assert!(ends.caught_up(&short).is_none()); // A partition with no recorded position is never caught up. @@ -835,27 +844,24 @@ mod tests { #[test] fn marker_watch_round_trips_and_rejects_an_unknown_schema() { let mut positions = WatchPositions::new(); - positions.insert( - MembershipPartition::new(3), - NextOffset::from_high_watermark(42), - ); + positions.insert(WatchPartition::new(3), NextOffset::from_high_watermark(42)); let mut ends = ObservationEnds::new(); - ends.insert( - MembershipPartition::new(3), - NextOffset::from_high_watermark(99), - ); + ends.insert(WatchPartition::new(3), NextOffset::from_high_watermark(99)); let watch = MarkerWatch { + topic: TOPIC.to_string(), positions: positions.clone(), ends: Some(ends), }; let value = serde_json::to_value(&watch).unwrap(); - assert_eq!(value["schema"], serde_json::json!(1)); + assert_eq!(value["schema"], serde_json::json!(2)); + assert_eq!(value["topic"], serde_json::json!(TOPIC)); assert_eq!(value["positions"]["3"], serde_json::json!(42)); assert_eq!(value["ends"]["3"], serde_json::json!(99)); assert_eq!(serde_json::from_value::(value).unwrap(), watch); let undispatched = MarkerWatch { + topic: TOPIC.to_string(), positions, ends: None, }; @@ -867,7 +873,7 @@ mod tests { ); assert!(serde_json::from_value::(serde_json::json!({ - "schema": 2, "positions": {}, "ends": null + "schema": 3, "topic": TOPIC, "positions": {}, "ends": null })) .is_err()); } @@ -877,6 +883,7 @@ mod tests { let at = Utc.timestamp_opt(1_700_000_000, 0).unwrap(); let hwms_value = serde_json::to_value(full_hwms(1_000)).unwrap(); let watch_value = serde_json::to_value(MarkerWatch { + topic: TOPIC.to_string(), positions: WatchPositions::new(), ends: None, }) @@ -890,6 +897,7 @@ mod tests { reconcile_observed_at: observed, reconcile_hwms: hwms, marker_watch: watch, + expected_marker_topic: TOPIC, }) }; assert_eq!( @@ -913,6 +921,7 @@ mod tests { reconcile_observed_at: observed, reconcile_hwms: hwms, marker_watch: watch, + expected_marker_topic: TOPIC, }) }; assert_eq!( @@ -952,6 +961,38 @@ mod tests { ); } + #[test] + fn a_watch_anchored_to_another_topic_re_dispatches_instead_of_resuming() { + // Offsets are only meaningful against the log they were captured from, so a run carrying a + // watch from a different topic — or from before the topic key existed — must never resume on + // them. + let at = Utc.timestamp_opt(1_700_000_000, 0).unwrap(); + let hwms_value = serde_json::to_value(full_hwms(1_000)).unwrap(); + let classify = |watch: Value| { + CompletionPhase::from_parts(CompletionParts { + status: CompletionStatus::Reconciling, + chunks_planned_at: Some(at), + reconcile_dispatched_at: Some(at), + reconcile_observed_at: None, + reconcile_hwms: Some(hwms_value.clone()), + marker_watch: Some(watch), + expected_marker_topic: TOPIC, + }) + }; + + assert_eq!( + classify(serde_json::json!({ + "schema": 2, "topic": "cohort_membership_changed_shadow", + "positions": {}, "ends": null + })), + CompletionPhase::ReconcilingUndispatched(UndispatchedReason::TopicChanged), + ); + assert_eq!( + classify(serde_json::json!({"schema": 1, "positions": {}, "ends": null})), + CompletionPhase::ReconcilingUndispatched(UndispatchedReason::UnparseableRecord), + ); + } + #[test] fn observed_marker_carries_the_run_and_cohort() { let observed = ObservedMarker { diff --git a/rust/cohort-seeder/src/domain/ledger.rs b/rust/cohort-seeder/src/domain/ledger.rs index 8fbc8b0d26bd..fd444274c544 100644 --- a/rust/cohort-seeder/src/domain/ledger.rs +++ b/rust/cohort-seeder/src/domain/ledger.rs @@ -1,11 +1,11 @@ //! Pure per-run fold of observed `reconcile_complete` markers into per-cohort partition bitmaps. //! -//! The marker watcher's dedicated task reads *all* membership partitions and hands each marker +//! The marker watcher's dedicated task reads *all* marker partitions and hands each marker //! to the ledger of the run it names. Folding is a monotone set-union: a partition's bit, once set, //! never clears, so replaying the same marker stream in any order yields the same bitmaps. The final //! per-cohort outcome (complete / partial) is reachable only through [`MarkerLedger::settle`], which //! consumes a [`SettleProof`] — the capability minted in [`super::completion`] once the watcher has -//! read past the membership end-watermarks captured at the liveness pass. No proof, no negative +//! read past the marker-topic end-watermarks captured at the liveness pass. No proof, no negative //! verdict. use std::collections::BTreeMap; @@ -327,8 +327,8 @@ mod tests { // Reach the proof through the real caught-up path so the ZST stays un-forgeable in tests: // one partition read up to its captured end, minting a fresh proof for each settle. let proof = || { - use crate::domain::{MembershipPartition, NextOffset, ObservationEnds, WatchPositions}; - let partition = MembershipPartition::new(0); + use crate::domain::{NextOffset, ObservationEnds, WatchPartition, WatchPositions}; + let partition = WatchPartition::new(0); let at = NextOffset::from_high_watermark(10); let mut ends = ObservationEnds::new(); ends.insert(partition, at); diff --git a/rust/cohort-seeder/src/domain/mod.rs b/rust/cohort-seeder/src/domain/mod.rs index e08880d69e9a..8d1dec5d1863 100644 --- a/rust/cohort-seeder/src/domain/mod.rs +++ b/rust/cohort-seeder/src/domain/mod.rs @@ -35,9 +35,9 @@ pub(crate) use completion::MARKER_WATCH_SCHEMA; pub use completion::{ CommittedOffset, CompletionParts, CompletionPhase, CompletionStatus, DispatchEpoch, DispatchedReconcile, LivenessCheck, MarkerNovelty, MarkerPartition, MarkerPartitionError, - MarkerWatch, MembershipPartition, NextOffset, ObservationEnds, ObservedMarker, PartitionBitmap, + MarkerWatch, NextOffset, ObservationEnds, ObservedMarker, PartitionBitmap, PartitionBitmapError, ProducedOffset, ReconcileHwms, ReconcileHwmsError, SeedGroupCommits, - SettleProof, UndispatchedReason, WatchPositions, + SettleProof, UndispatchedReason, WatchPartition, WatchPositions, }; pub use condition::{EventNameSet, Lookback, PinnedCondition}; pub use ids::{ diff --git a/rust/cohort-seeder/src/kafka/markers.rs b/rust/cohort-seeder/src/kafka/markers.rs index d9947c1c22a6..fa31e3964355 100644 --- a/rust/cohort-seeder/src/kafka/markers.rs +++ b/rust/cohort-seeder/src/kafka/markers.rs @@ -1,13 +1,13 @@ -//! Tails the membership-change topic for `reconcile_complete` markers — the completion *authority*. +//! Tails the reconcile-marker topic for `reconcile_complete` markers — the completion *authority*. //! //! An assign-only consumer with a unique throwaway group id: it never subscribes and never commits, so -//! it participates in no rebalance and leaves no durable offset. It watches every membership partition +//! it participates in no rebalance and leaves no durable offset. It watches every marker partition //! from explicit start offsets (the high watermarks captured at dispatch), with `auto.offset.reset= //! error` so a start below the log's low watermark surfaces as [`WatchError::Truncated`] instead of -//! silently jumping. The topic is high-volume and person-keyed; markers are the rare key that contains -//! `':'` (`"{team}:{cohort}:{run}"`), so that byte probe rejects membership rows before any JSON parse. -//! Non-marker messages still advance the partition's next-read offset. rdkafka types stay confined -//! here — the watcher yields typed [`WatchItem`]s. +//! silently jumping. Every marker is keyed `"{team}:{cohort}:{run}"`; on a dedicated topic the `b':'` +//! probe is no longer a hot-path filter but a guard against a mis-pointed topic, so anything it (or the +//! parse behind it) rejects is counted by `reason`. Skipped messages still advance the partition's +//! next-read offset. rdkafka types stay confined here — the watcher yields typed [`WatchItem`]s. use std::collections::HashMap; use std::sync::Arc; @@ -22,7 +22,7 @@ use rdkafka::message::Message; use rdkafka::{ClientConfig, Offset, TopicPartitionList}; use crate::domain::{ - MarkerPartition, MembershipPartition, NextOffset, ObservedMarker, ReconcileCompleteMarker, + MarkerPartition, NextOffset, ObservedMarker, ReconcileCompleteMarker, WatchPartition, WatchPositions, }; use crate::observability::metrics::RECONCILE_MARKER_PARSE_FAILURES; @@ -30,16 +30,17 @@ use crate::observability::metrics::RECONCILE_MARKER_PARSE_FAILURES; /// In-flight watermark fetches during a seek, matching the group-lag scanner's bound. const WATERMARK_CONCURRENCY: usize = 16; -/// One record read from the membership topic: where the watcher now sits on that partition, and the -/// marker it carried (if any). Non-marker rows carry `marker: None` but still advance `next_offset`. +/// One record read from the marker topic: where the watcher now sits on that partition, and the +/// marker it carried (if any). A rejected record carries `marker: None` but still advances +/// `next_offset`. #[derive(Debug, Clone, PartialEq, Eq)] pub struct WatchItem { - pub partition: MembershipPartition, + pub partition: WatchPartition, pub next_offset: NextOffset, pub marker: Option, } -/// An assign-only consumer over the membership topic. Holds an rdkafka `StreamConsumer` behind an +/// An assign-only consumer over the marker topic. Holds an rdkafka `StreamConsumer` behind an /// `Arc` so the (infrequent, blocking) watermark check can run on the blocking pool while `recv` keeps /// borrowing it. The app-level `MarkerStream` seam wraps it so the watch task can be tested with a fake. pub struct MarkerWatcher { @@ -89,13 +90,13 @@ impl MarkerWatcher { /// Fetch every watched partition's low watermark, one blocking call each, bounded-concurrent /// like `ingestion-control-plane`'s group-lag scan. Each call can block for the full watermark - /// timeout, and a seek covers all 64 membership partitions, so serializing them against an - /// unresponsive broker overruns the watch task's liveness deadline. + /// timeout, and a seek covers every marker partition, so serializing them against an unresponsive + /// broker overruns the watch task's liveness deadline. async fn low_watermarks( &self, start: &WatchPositions, - ) -> Result, WatchError> { - let partitions: Vec = start.iter().map(|(p, _)| p).collect(); + ) -> Result, WatchError> { + let partitions: Vec = start.iter().map(|(p, _)| p).collect(); stream::iter(partitions) .map(|partition| { let consumer = Arc::clone(&self.consumer); @@ -128,11 +129,11 @@ impl MarkerWatcher { .map_err(WatchError::Assign) } - /// Await the next record and classify it. A membership row yields `marker: None`; a keyed record - /// that fails to parse increments the parse-failure counter and still advances the offset. + /// Await the next record and classify it. A record the classifier rejects yields `marker: None`, + /// increments the parse-failure counter under its reason, and still advances the offset. pub async fn next_item(&self) -> Result { let message = self.consumer.recv().await.map_err(WatchError::Recv)?; - let partition = MembershipPartition::new(message.partition()); + let partition = WatchPartition::new(message.partition()); let next_offset = NextOffset::from_high_watermark(message.offset() + 1); let marker = classify_marker(message.key(), message.payload()); Ok(WatchItem { @@ -149,7 +150,7 @@ impl MarkerWatcher { fn build_assignment( topic: &str, start: &WatchPositions, - lows: &HashMap, + lows: &HashMap, ) -> Result { let mut tpl = TopicPartitionList::new(); for (partition, next) in start.iter() { @@ -168,23 +169,24 @@ fn build_assignment( Ok(tpl) } -/// Cheap discriminator then parse. Markers are keyed `"{team}:{cohort}:{run}"`; membership rows are -/// keyed by a person UUID (no `':'`), so the byte probe skips the JSON parse for the common case. +/// Key probe then parse. On a dedicated topic every record should be a marker, so each rejection is +/// counted under the `reason` that explains it rather than silently skipped. fn classify_marker(key: Option<&[u8]>, payload: Option<&[u8]>) -> Option { - let key = key?; + let skip = |reason: &'static str| { + counter!(RECONCILE_MARKER_PARSE_FAILURES, "reason" => reason).increment(1); + None + }; + let Some(key) = key else { + return skip("no_key"); + }; if !key.contains(&b':') { - return None; + return skip("foreign_key"); } let Some(payload) = payload else { - counter!(RECONCILE_MARKER_PARSE_FAILURES).increment(1); - return None; + return skip("no_payload"); }; - let marker = match serde_json::from_slice::(payload) { - Ok(marker) => marker, - Err(_) => { - counter!(RECONCILE_MARKER_PARSE_FAILURES).increment(1); - return None; - } + let Ok(marker) = serde_json::from_slice::(payload) else { + return skip("decode"); }; match MarkerPartition::new(u32::from(marker.partition())) { Ok(partition) => Some(ObservedMarker { @@ -193,10 +195,7 @@ fn classify_marker(key: Option<&[u8]>, payload: Option<&[u8]>) -> Option { - counter!(RECONCILE_MARKER_PARSE_FAILURES).increment(1); - None - } + Err(_) => skip("partition"), } } @@ -225,16 +224,16 @@ fn build_config(kafka: &KafkaConfig, group_id: &str) -> ClientConfig { pub enum WatchError { #[error("creating the marker-watch consumer")] Consumer(#[source] KafkaError), - #[error("fetching watermarks for membership partition {partition}")] + #[error("fetching watermarks for marker partition {partition}")] Watermarks { partition: i32, #[source] source: KafkaError, }, - #[error("joining the membership watermark task")] + #[error("joining the marker-topic watermark task")] WatermarkJoin(#[source] tokio::task::JoinError), #[error( - "membership partition {partition} start offset {requested} is below the log's low watermark \ + "marker partition {partition} start offset {requested} is below the log's low watermark \ {low}: the topic was truncated past the captured watch position" )] Truncated { @@ -244,7 +243,7 @@ pub enum WatchError { }, #[error("assigning the marker-watch partitions")] Assign(#[source] KafkaError), - #[error("receiving from the membership topic")] + #[error("receiving from the marker topic")] Recv(#[source] KafkaError), } @@ -268,7 +267,7 @@ mod tests { #[test] fn classify_probes_the_key_before_parsing_and_extracts_the_marker() { - // Person-keyed membership row: no ':' in the key, so no parse attempt, no marker. + // A foreign, person-keyed row: no ':' in the key, so no parse attempt, no marker. assert!(classify_marker(Some(b"01928aaa-bbbb-cccc"), Some(b"{}")).is_none()); // A marker key with a valid payload. @@ -288,7 +287,7 @@ mod tests { fn a_start_below_the_low_watermark_is_truncation_and_the_boundary_is_not() { // A start exactly at the low watermark is the normal case right after a retention sweep. // Reporting it as truncated would drop and tombstone every run on the partition. - let partition = MembershipPartition::new(0); + let partition = WatchPartition::new(0); let lows = HashMap::from([(partition, 20_i64)]); let positions = |offset| { let mut positions = WatchPositions::new(); @@ -312,5 +311,5 @@ mod tests { ); } - const TOPIC: &str = "cohort_membership_changed_shadow"; + const TOPIC: &str = "cohort_reconcile_markers"; } diff --git a/rust/cohort-seeder/src/kafka/mod.rs b/rust/cohort-seeder/src/kafka/mod.rs index 653005fc897b..f9e5e60d79d3 100644 --- a/rust/cohort-seeder/src/kafka/mod.rs +++ b/rust/cohort-seeder/src/kafka/mod.rs @@ -1,5 +1,5 @@ //! Kafka layer: the seed-tile producer client, the produce pacer, and the completion observers — the -//! seed-group offset reader (liveness) and the membership-topic marker watcher (authority). Depends +//! seed-group offset reader (liveness) and the marker-topic watcher (authority). Depends //! only on `domain` (plus the shared metric-name constants); never on `store`. pub mod committed; diff --git a/rust/cohort-seeder/src/kafka/producer.rs b/rust/cohort-seeder/src/kafka/producer.rs index 4f76421436a9..5c0feb28ddbd 100644 --- a/rust/cohort-seeder/src/kafka/producer.rs +++ b/rust/cohort-seeder/src/kafka/producer.rs @@ -13,7 +13,7 @@ use rdkafka::error::{KafkaError, RDKafkaErrorCode}; use rdkafka::producer::{DeliveryFuture, FutureProducer, FutureRecord, Producer}; use crate::domain::{ - MembershipPartition, NextOffset, PersonSeed, ReconcileTile, SeedTile, WatchPositions, + NextOffset, PersonSeed, ReconcileTile, SeedTile, WatchPartition, WatchPositions, }; pub use crate::domain::partition::{SeedPartition, SeedPartitionCountError, SeedPartitions}; @@ -125,7 +125,7 @@ impl SeedTileProducer { self.producer.flush(timeout) } - /// Capture the membership topic's per-partition high watermarks as the marker watcher's start + /// Capture the marker topic's per-partition high watermarks as the marker watcher's start /// positions. The high watermark is the offset the next record *will* receive, so it is exactly /// the first offset the watcher must read — no clock or "latest committed" assumption. Callers /// capture these BEFORE producing reconcile tiles: markers acked after this point sit at or above @@ -185,7 +185,7 @@ impl SeedTileProducer { source, })?; positions.insert( - MembershipPartition::new(partition.id()), + WatchPartition::new(partition.id()), NextOffset::from_high_watermark(high), ); } @@ -193,25 +193,24 @@ impl SeedTileProducer { } } -/// Why capturing the membership topic's start positions failed. The dispatch cannot record a -/// resumable watch state without them, so every variant aborts the dispatch (it re-converges on the -/// next tick). +/// Why capturing the marker topic's start positions failed. The dispatch cannot record a resumable +/// watch state without them, so every variant aborts the dispatch (it re-converges on the next tick). #[derive(Debug, thiserror::Error)] pub enum CaptureOffsetsError { - #[error("fetching membership topic metadata")] + #[error("fetching marker topic metadata")] Metadata(#[source] KafkaError), - #[error("membership topic {topic:?} is not present in broker metadata")] + #[error("marker topic {topic:?} is not present in broker metadata")] Missing { topic: String }, - #[error("membership topic {topic:?} metadata reports {code}")] + #[error("marker topic {topic:?} metadata reports {code}")] Topic { topic: String, code: RDKafkaErrorCode, }, - #[error("membership topic {topic:?} reports no partitions")] + #[error("marker topic {topic:?} reports no partitions")] NoPartitions { topic: String }, - #[error("capturing membership topic {topic:?} offsets exceeded its {budget:?} budget")] + #[error("capturing marker topic {topic:?} offsets exceeded its {budget:?} budget")] BudgetExhausted { topic: String, budget: Duration }, - #[error("fetching watermarks for membership topic {topic:?} partition {partition}")] + #[error("fetching watermarks for marker topic {topic:?} partition {partition}")] Watermarks { topic: String, partition: i32, diff --git a/rust/cohort-seeder/src/main.rs b/rust/cohort-seeder/src/main.rs index 6d0b4995ce2c..d4d5fcdba9dd 100644 --- a/rust/cohort-seeder/src/main.rs +++ b/rust/cohort-seeder/src/main.rs @@ -176,7 +176,7 @@ struct WiredCompletion { /// (CAS + tile produce + record); the observer arms the observation half (marker-watch directives + /// the observation pass) and its dedicated watch task. Either half alone is valid; both off leaves the /// dark path with no extra queries. A misconfigured policy (dispatch enabled without attestation, or a -/// non-contract partition count) is a startup error, as is an unreachable membership topic — a typo'd +/// non-contract partition count) is a startup error, as is an unreachable marker topic — a typo'd /// name would otherwise surface only as runs stuck re-dispatching forever. async fn build_completion( config: &Config, @@ -197,18 +197,23 @@ async fn build_completion( }); } - // Both halves ride the membership topic — dispatch produces reconcile tiles onto it, the observer - // watches it for markers — so prove it is reachable before either arms. + // Both halves anchor on the marker topic — dispatch captures its watermarks, the observer watches + // it for markers — so prove it is reachable before either arms. let verify_producer = producer.clone(); - let membership_topic = config.cohort_membership_changed_topic.clone(); + let marker_topic = config.cohort_reconcile_markers_topic.clone(); tokio::task::spawn_blocking(move || { - verify_producer.capture_topic_offsets(&membership_topic, PARTITION_VERIFY_TIMEOUT) + verify_producer.capture_topic_offsets(&marker_topic, PARTITION_VERIFY_TIMEOUT) }) .await - .context("joining membership topic verification task")? - .context("verifying the membership topic is reachable")?; + .context("joining marker topic verification task")? + .context("verifying the marker topic is reachable")?; - let mut driver = CompletionDriver::new(pool.clone(), config.team_allowlist.clone(), kinds); + let mut driver = CompletionDriver::new( + pool.clone(), + config.team_allowlist.clone(), + kinds, + config.cohort_reconcile_markers_topic.clone(), + ); if let AutoDispatchPolicy::Enabled(register_backfill) = dispatch_policy { let max_inflight = NonZeroUsize::new(config.seeder_max_inflight_tiles) @@ -218,7 +223,6 @@ async fn build_completion( .context("SEEDER_RECONCILE_MAX_CONCURRENT_DISPATCHES must be greater than zero")?; driver = driver.with_dispatch( producer.clone(), - config.cohort_membership_changed_topic.clone(), max_inflight, max_concurrent_dispatches, register_backfill, @@ -263,7 +267,7 @@ async fn build_completion( ); let topic_ends = KafkaTopicOffsets::new( producer.clone(), - config.cohort_membership_changed_topic.clone(), + config.cohort_reconcile_markers_topic.clone(), offsets_timeout, ); // Unique group id: the watcher never commits or joins a group, but a distinct id keeps it @@ -271,7 +275,7 @@ async fn build_completion( let watch_group = format!("cohort-seeder-marker-watch-{}", uuid::Uuid::now_v7()); let watcher = MarkerWatcher::new( &config.build_kafka_config(), - config.cohort_membership_changed_topic.clone(), + config.cohort_reconcile_markers_topic.clone(), &watch_group, offsets_timeout, ) @@ -285,7 +289,7 @@ async fn build_completion( ); Some(MarkerWatchTask::new( watcher, - PgMarkerFlush::new(pool.clone()), + PgMarkerFlush::new(pool.clone(), config.cohort_reconcile_markers_topic.clone()), directives_rx, handle, persist_interval, @@ -321,7 +325,7 @@ fn log_startup(config: &Config) { reconcile_auto_dispatch_enabled = config.seeder_reconcile_auto_dispatch_enabled, confirm_register_backfilled = config.seeder_confirm_register_backfilled, reconcile_max_concurrent_dispatches = config.seeder_reconcile_max_concurrent_dispatches, - membership_topic = %config.cohort_membership_changed_topic, + reconcile_markers_topic = %config.cohort_reconcile_markers_topic, reconcile_observer_enabled = config.seeder_reconcile_observer_enabled, seed_consumer_group = %config.kafka_seed_consumer_group, reconcile_offsets_timeout_ms = config.seeder_reconcile_offsets_timeout_ms, diff --git a/rust/cohort-seeder/src/observability/metrics.rs b/rust/cohort-seeder/src/observability/metrics.rs index 5e30d5c10e1d..4c8794bac2f2 100644 --- a/rust/cohort-seeder/src/observability/metrics.rs +++ b/rust/cohort-seeder/src/observability/metrics.rs @@ -51,6 +51,8 @@ pub const RECONCILE_DISPATCHES_IN_FLIGHT: &str = "seeder_reconcile_dispatches_in pub const RUNS_RECONCILING: &str = "seeder_runs_reconciling"; // Liveness, marker watcher, and observation. pub const RECONCILE_MARKERS_OBSERVED: &str = "seeder_reconcile_markers_observed_total"; +/// Records the watcher rejected, labelled by `reason` (counter). On a dedicated marker topic every +/// record should be a marker, so any of these means something is mis-pointed or corrupt. pub const RECONCILE_MARKER_PARSE_FAILURES: &str = "seeder_reconcile_marker_parse_failures_total"; pub const RECONCILE_MARKER_WATCH_LAG: &str = "seeder_reconcile_marker_watch_lag"; pub const RECONCILE_LIVENESS_LAGGING_PARTITIONS: &str = diff --git a/rust/cohort-seeder/src/store/completion.rs b/rust/cohort-seeder/src/store/completion.rs index ae76fcf54d90..cfbd27b8c84a 100644 --- a/rust/cohort-seeder/src/store/completion.rs +++ b/rust/cohort-seeder/src/store/completion.rs @@ -412,12 +412,18 @@ pub async fn persist_observation_ends( run_id: RunId, epoch: DispatchEpoch, ends: &ObservationEnds, + marker_topic: &str, ) -> Result<(), CompletionStoreError> { let updated = sqlx::query_scalar::<_, RunId>( r#" UPDATE cohort_backfill_runs SET marker_watch = jsonb_set( - coalesce(marker_watch, jsonb_build_object('schema', $4::bigint, 'positions', '{}'::jsonb)), + coalesce( + marker_watch, + jsonb_build_object( + 'schema', $4::bigint, 'topic', $5::text, 'positions', '{}'::jsonb + ) + ), '{ends}', $3::jsonb, true ), updated_at = now() @@ -429,6 +435,7 @@ pub async fn persist_observation_ends( .bind(epoch.as_datetime()) .bind(Json(ends)) .bind(i64::from(MARKER_WATCH_SCHEMA)) + .bind(marker_topic) .fetch_optional(pool) .await?; fence(updated, run_id, CompletionOperation::PersistEnds) @@ -444,6 +451,7 @@ pub async fn persist_marker_observations( epoch: DispatchEpoch, bit_updates: &[(CohortId, PartitionBitmap)], positions: &WatchPositions, + marker_topic: &str, ) -> Result<(), CompletionStoreError> { let mut tx = pool.begin().await?; // The positions write also proves the fence: its RETURNING gates the whole transaction. @@ -454,7 +462,10 @@ pub async fn persist_marker_observations( r#" UPDATE cohort_backfill_runs SET marker_watch = jsonb_set( - coalesce(marker_watch, jsonb_build_object('schema', $4::bigint, 'ends', NULL)), + coalesce( + marker_watch, + jsonb_build_object('schema', $4::bigint, 'topic', $5::text, 'ends', NULL) + ), '{positions}', $3::jsonb, true ), updated_at = now() @@ -466,6 +477,7 @@ pub async fn persist_marker_observations( .bind(epoch.as_datetime()) .bind(Json(positions)) .bind(i64::from(MARKER_WATCH_SCHEMA)) + .bind(marker_topic) .fetch_optional(&mut *tx) .await?; if fenced.is_none() { @@ -821,6 +833,7 @@ pub async fn discover_completions( pool: &PgPool, allowlist: &TeamAllowlist, kinds: &[RunKind], + marker_topic: &str, ) -> Result, CompletionStoreError> { let bound_kinds = kinds.iter().map(|kind| kind.as_str()).collect::>(); let rows = match allowlist { @@ -840,7 +853,10 @@ pub async fn discover_completions( .await? } }; - Ok(rows.into_iter().filter_map(classify_row).collect()) + Ok(rows + .into_iter() + .filter_map(|row| classify_row(row, marker_topic)) + .collect()) } /// How many runs hold `reconciling`, per kind — including the observed ones discovery drops, which @@ -879,7 +895,7 @@ pub async fn count_reconciling_by_kind( .collect()) } -fn classify_row(row: CompletionRunRow) -> Option { +fn classify_row(row: CompletionRunRow, marker_topic: &str) -> Option { let status = match row.status.as_str() { "seeding" => CompletionStatus::Seeding, "reconciling" => CompletionStatus::Reconciling, @@ -893,6 +909,7 @@ fn classify_row(row: CompletionRunRow) -> Option { reconcile_observed_at: row.reconcile_observed_at, reconcile_hwms: row.reconcile_hwms.map(|json| json.0), marker_watch: row.marker_watch.map(|json| json.0), + expected_marker_topic: marker_topic, }); Some(DiscoveredCompletion { run_id: row.id, diff --git a/rust/cohort-seeder/tests/pg_completion.rs b/rust/cohort-seeder/tests/pg_completion.rs index 05592a49dfd4..6bcbc825560a 100644 --- a/rust/cohort-seeder/tests/pg_completion.rs +++ b/rust/cohort-seeder/tests/pg_completion.rs @@ -16,9 +16,9 @@ use chrono::{DateTime, Duration as ChronoDuration, Utc}; use cohort_core::filters::{CohortId, TeamId}; use cohort_core::partitioner::COHORT_PARTITION_COUNT; use cohort_seeder::domain::{ - CompletionPhase, DispatchEpoch, MarkerPartition, MarkerWatch, MembershipPartition, NextOffset, - ObservationEnds, PartitionBitmap, PersonShapeHash, ProducedOffset, ReconcileHwms, - ReconcileScope, RunId, SeedPartition, UndispatchedReason, WatchPositions, + CompletionPhase, DispatchEpoch, MarkerPartition, MarkerWatch, NextOffset, ObservationEnds, + PartitionBitmap, PersonShapeHash, ProducedOffset, ReconcileHwms, ReconcileScope, RunId, + SeedPartition, UndispatchedReason, WatchPartition, WatchPositions, }; use cohort_seeder::store::chunks::PgChunkStore; use cohort_seeder::store::completion::{ @@ -44,6 +44,7 @@ use support::{ }; const ONE_BAND: NonZeroU16 = NonZeroU16::MIN; +const MARKER_TOPIC: &str = "cohort_reconcile_markers"; /// What discovery binds once `SEEDER_PERSON_SEEDS_ENABLED` is on. const BOTH_KINDS: &[RunKind] = &[RunKind::Behavioral, RunKind::PersonProperty]; @@ -63,15 +64,13 @@ fn full_hwms() -> ReconcileHwms { fn captured_ends() -> ObservationEnds { let mut ends = ObservationEnds::new(); - ends.insert( - MembershipPartition::new(0), - NextOffset::from_high_watermark(10), - ); + ends.insert(WatchPartition::new(0), NextOffset::from_high_watermark(10)); ends } fn empty_watch() -> MarkerWatch { MarkerWatch { + topic: MARKER_TOPIC.to_string(), positions: WatchPositions::new(), ends: None, } @@ -109,12 +108,19 @@ async fn completion_discovery_is_gated_by_the_allowed_kinds() -> Result<()> { with_db(|pool| async move { let person_run = insert_person_run(&pool, 2, "seeding", true, person_pinned(&[])).await?; - let dark = discover_completions(&pool, &TeamAllowlist::All, &[RunKind::Behavioral]).await?; + let dark = discover_completions( + &pool, + &TeamAllowlist::All, + &[RunKind::Behavioral], + MARKER_TOPIC, + ) + .await?; ensure!(dark .iter() .all(|completion| completion.run_id != person_run)); - let lit = discover_completions(&pool, &TeamAllowlist::All, BOTH_KINDS).await?; + let lit = + discover_completions(&pool, &TeamAllowlist::All, BOTH_KINDS, MARKER_TOPIC).await?; let discovered = lit .iter() .find(|completion| completion.run_id == person_run) @@ -356,7 +362,8 @@ async fn revert_clears_the_dispatch_record_and_returns_to_seeding() -> Result<() .await?; ensure!(status == "seeding" && cleared); - let discovered = discover_completions(&pool, &TeamAllowlist::All, BOTH_KINDS).await?; + let discovered = + discover_completions(&pool, &TeamAllowlist::All, BOTH_KINDS, MARKER_TOPIC).await?; let phase = discovered .iter() .find(|completion| completion.run_id == run_id) @@ -419,6 +426,7 @@ async fn record_is_fenced_against_a_dispatch_recorded_after_the_claim() -> Resul epoch, &[(CohortId(301), PartitionBitmap::from_bits(3)?)], &WatchPositions::new(), + MARKER_TOPIC, ) .await?; @@ -550,7 +558,9 @@ async fn fenced_writes_reject_stale_epoch_and_wrong_status() -> Result<()> { .execute(&pool) .await?; } - ensure_fence_lost(persist_observation_ends(&pool, run_id, at, &captured_ends()).await)?; + ensure_fence_lost( + persist_observation_ends(&pool, run_id, at, &captured_ends(), MARKER_TOPIC).await, + )?; ensure_fence_lost( persist_marker_observations( &pool, @@ -558,6 +568,7 @@ async fn fenced_writes_reject_stale_epoch_and_wrong_status() -> Result<()> { at, &bit_updates, &WatchPositions::new(), + MARKER_TOPIC, ) .await, )?; @@ -689,12 +700,9 @@ async fn capturing_ends_leaves_the_watcher_positions_alone() -> Result<()> { let epoch = claim.record(&pool, &full_hwms(), &empty_watch()).await?; let mut advanced = WatchPositions::new(); - advanced.insert( - MembershipPartition::new(0), - NextOffset::from_high_watermark(42), - ); - persist_marker_observations(&pool, run_id, epoch, &[], &advanced).await?; - persist_observation_ends(&pool, run_id, epoch, &captured_ends()).await?; + advanced.insert(WatchPartition::new(0), NextOffset::from_high_watermark(42)); + persist_marker_observations(&pool, run_id, epoch, &[], &advanced, MARKER_TOPIC).await?; + persist_observation_ends(&pool, run_id, epoch, &captured_ends(), MARKER_TOPIC).await?; let watch: Json = sqlx::query_scalar("SELECT marker_watch FROM cohort_backfill_runs WHERE id = $1") @@ -702,7 +710,7 @@ async fn capturing_ends_leaves_the_watcher_positions_alone() -> Result<()> { .fetch_one(&pool) .await?; ensure!( - watch.0.positions.get(MembershipPartition::new(0)) + watch.0.positions.get(WatchPartition::new(0)) == Some(NextOffset::from_high_watermark(42)), "capturing the ends regressed the watcher positions" ); @@ -731,7 +739,8 @@ async fn unreconcilable_runs_are_marked_observed_for_django() -> Result<()> { ensure!(observed_stamp.is_some()); // Observing it is what removes it from the driver's working set: everything left is // Django's, and the gauge reads it from the aggregate instead. - let discovered = discover_completions(&pool, &TeamAllowlist::All, BOTH_KINDS).await?; + let discovered = + discover_completions(&pool, &TeamAllowlist::All, BOTH_KINDS, MARKER_TOPIC).await?; ensure!(!discovered .iter() .any(|completion| completion.run_id == stranded)); @@ -775,6 +784,7 @@ async fn marker_observations_or_merge_round_trip_including_bit_63() -> Result<() epoch, &[(CohortId(401), low)], &WatchPositions::new(), + MARKER_TOPIC, ) .await?; @@ -786,6 +796,7 @@ async fn marker_observations_or_merge_round_trip_including_bit_63() -> Result<() epoch, &[(CohortId(401), high)], &WatchPositions::new(), + MARKER_TOPIC, ) .await?; @@ -853,7 +864,8 @@ async fn discovery_classifies_each_phase_and_drops_observed_runs() -> Result<()> let epoch = claim.record(&pool, &full_hwms(), &empty_watch()).await?; mark_run_observed(&pool, observed, epoch).await?; - let discovered = discover_completions(&pool, &TeamAllowlist::All, BOTH_KINDS).await?; + let discovered = + discover_completions(&pool, &TeamAllowlist::All, BOTH_KINDS, MARKER_TOPIC).await?; let phase = |run| { discovered .iter() @@ -884,7 +896,7 @@ async fn discovery_classifies_each_phase_and_drops_observed_runs() -> Result<()> // Both statements have a team-scoped variant, and dev — where this rollout lands first — // is the environment that runs allowlisted. let allowlisted = TeamAllowlist::Only([4, 6].into_iter().collect()); - let scoped = discover_completions(&pool, &allowlisted, BOTH_KINDS).await?; + let scoped = discover_completions(&pool, &allowlisted, BOTH_KINDS, MARKER_TOPIC).await?; ensure!(scoped.iter().map(|row| row.run_id).eq([dispatched])); let scoped_counts = count_reconciling_by_kind(&pool, &allowlisted, BOTH_KINDS).await?; ensure!(scoped_counts.get(&RunKind::Behavioral) == Some(&2)); diff --git a/rust/cohort-stream-processor/src/config.rs b/rust/cohort-stream-processor/src/config.rs index bcadddceb0ad..7f4d629b004b 100644 --- a/rust/cohort-stream-processor/src/config.rs +++ b/rust/cohort-stream-processor/src/config.rs @@ -310,6 +310,11 @@ pub struct Config { #[envconfig(default = "cohort_membership_changed_shadow")] pub cohort_membership_changed_topic: String, + /// Reconcile completion markers ride their own topic: the membership topic's consumers reject + /// any record without `person_id`/`status`, and the seeder's watcher tails this one end to end. + #[envconfig(default = "cohort_reconcile_markers")] + pub cohort_reconcile_markers_topic: String, + /// `murmur2_random` co-partitions a `person_id` key identically to the Node/Python producers. #[envconfig(default = "murmur2_random")] pub kafka_producer_partitioner: String, @@ -1083,6 +1088,7 @@ mod tests { pod_name: None, pod_hostname: None, cohort_membership_changed_topic: "cohort_membership_changed_shadow".to_string(), + cohort_reconcile_markers_topic: "cohort_reconcile_markers".to_string(), kafka_producer_partitioner: "murmur2_random".to_string(), cohort_partition_count: 64, kafka_compression_codec: "none".to_string(), diff --git a/rust/cohort-stream-processor/src/consumers/events.rs b/rust/cohort-stream-processor/src/consumers/events.rs index bec68c15de9e..9a41f071280c 100644 --- a/rust/cohort-stream-processor/src/consumers/events.rs +++ b/rust/cohort-stream-processor/src/consumers/events.rs @@ -1501,7 +1501,7 @@ mod tests { use crate::partitions::partitioner::{partition_of, COHORT_PARTITION_COUNT}; use crate::producer::{ CaptureSink, CaptureStreamEventSink, CaptureTransferSink, CohortMembershipChange, - MembershipStatus, ReconcileCompleteMarker, + MembershipStatus, }; use crate::stage1::state::AppliedOffsets; use crate::stage1::{Stage1State, StatefulRecord}; @@ -1543,13 +1543,6 @@ mod tests { } changes.into_iter().map(|_| Ok(())).collect() } - - async fn produce_markers( - &self, - markers: Vec, - ) -> Vec> { - markers.into_iter().map(|_| Ok(())).collect() - } } /// Wrap a test store in the `All` operating point so the dispatcher runs on the blocking pool. diff --git a/rust/cohort-stream-processor/src/main.rs b/rust/cohort-stream-processor/src/main.rs index 182c1f8fc0dc..42c71e4e9805 100644 --- a/rust/cohort-stream-processor/src/main.rs +++ b/rust/cohort-stream-processor/src/main.rs @@ -32,9 +32,10 @@ use cohort_stream_processor::partitions::{ LiveWatermarks, OffsetTracker, PartitionPauser, PartitionRouter, }; use cohort_stream_processor::producer::{ - CascadeSink, KafkaCascadeSink, KafkaMembershipSink, KafkaSeedTileSink, KafkaStreamEventSink, - KafkaTransferSink, MembershipSink, NoopCascadeSink, NoopSeedTileSink, SeedTileSink, - StreamEventSink, TransferSink, + CascadeSink, KafkaCascadeSink, KafkaMembershipSink, KafkaReconcileMarkerSink, + KafkaSeedTileSink, KafkaStreamEventSink, KafkaTransferSink, MembershipSink, NoopCascadeSink, + NoopReconcileMarkerSink, NoopSeedTileSink, ReconcileMarkerSink, SeedTileSink, StreamEventSink, + TransferSink, }; use cohort_stream_processor::store::durability::{ run_boot_restore, upload_cadence, CheckpointExporter, CheckpointSweeper, OffsetManifest, @@ -221,6 +222,18 @@ async fn async_main(config: Config) -> Result<()> { } else { Arc::new(NoopSeedTileSink) }; + let marker_sink: Arc = if config.cohort_seed_reconcile_enabled { + Arc::new( + KafkaReconcileMarkerSink::new( + &kafka_config, + config.cohort_reconcile_markers_topic.clone(), + ) + .await + .context("creating cohort_reconcile_markers producer")?, + ) + } else { + Arc::new(NoopReconcileMarkerSink) + }; let reconcile_backlog = Arc::new(ReconcileBacklog::default()); let merge_deps = Arc::new(MergeWorkerDeps { transfer_sink, @@ -243,6 +256,7 @@ async fn async_main(config: Config) -> Result<()> { enabled: config.cohort_seed_reconcile_enabled, scan_page: config.cohort_seed_reconcile_scan_page, backlog: reconcile_backlog.clone(), + marker_sink, }, person_seed: PersonSeedDeps { enabled: config.cohort_seed_person_apply_enabled, @@ -386,6 +400,11 @@ async fn async_main(config: Config) -> Result<()> { config.cohort_stream_seed_events_topic, seed_partitions, ); + // Nothing co-partitions with the marker topic, so only its existence matters. Failing here + // beats the alternative: a marker produce retrying forever while it holds the seed offset. + if config.cohort_seed_reconcile_enabled { + fetch_partition_count(seed_consumer, &config.cohort_reconcile_markers_topic)?; + } } if let Some(manifest) = restore.manifest.as_ref() { @@ -784,6 +803,7 @@ fn log_startup(config: &Config) { kafka_hosts = %config.kafka_hosts, input_topic = %config.cohort_stream_events_topic, output_topic = %config.cohort_membership_changed_topic, + reconcile_markers_topic = %config.cohort_reconcile_markers_topic, consumer_group = %config.kafka_consumer_group, offset_reset = %config.kafka_consumer_offset_reset, merge_topic = %config.person_merge_events_topic, diff --git a/rust/cohort-stream-processor/src/observability/metrics.rs b/rust/cohort-stream-processor/src/observability/metrics.rs index b7a3fe07ab94..7b98382d1ce6 100644 --- a/rust/cohort-stream-processor/src/observability/metrics.rs +++ b/rust/cohort-stream-processor/src/observability/metrics.rs @@ -556,6 +556,10 @@ pub const RECONCILE_ROWS_EMITTED_TOTAL: &str = "cohort_reconcile_rows_emitted_to pub const RECONCILE_BITS_FIXED_TOTAL: &str = "cohort_reconcile_bits_fixed_total"; /// Reconcile completion markers acknowledged by Kafka, labelled by `kind` (counter). pub const RECONCILE_MARKERS_EMITTED_TOTAL: &str = "cohort_reconcile_markers_emitted_total"; +/// Failed completion-marker produces, labelled by `kind` (counter). A permanently failing produce — +/// a missing or mis-provisioned marker topic — otherwise only shows up as a seed offset that never +/// advances. +pub const RECONCILE_MARKER_PRODUCE_ERRORS: &str = "cohort_reconcile_marker_produce_errors_total"; /// Partition-local reconcile queue depth, labelled by `partition` (gauge). pub const RECONCILE_QUEUE_DEPTH: &str = "cohort_reconcile_queue_depth"; @@ -908,6 +912,10 @@ mod tests { RECONCILE_MARKERS_EMITTED_TOTAL, "cohort_reconcile_markers_emitted_total", ); + assert_eq!( + RECONCILE_MARKER_PRODUCE_ERRORS, + "cohort_reconcile_marker_produce_errors_total", + ); assert_eq!(RECONCILE_QUEUE_DEPTH, "cohort_reconcile_queue_depth"); } diff --git a/rust/cohort-stream-processor/src/producer/kafka.rs b/rust/cohort-stream-processor/src/producer/kafka.rs index 0ddf0c77344e..7adb40778154 100644 --- a/rust/cohort-stream-processor/src/producer/kafka.rs +++ b/rust/cohort-stream-processor/src/producer/kafka.rs @@ -9,7 +9,7 @@ use common_kafka::kafka_producer::{ use common_liveness::SyncLivenessReporter; use rdkafka::producer::FutureProducer; -use crate::producer::{CohortMembershipChange, MembershipSink, ReconcileCompleteMarker}; +use crate::producer::{CohortMembershipChange, MembershipSink}; /// No-op liveness reporter — producer stalls are surfaced by the consumer's liveness deadline. #[derive(Clone, Copy)] @@ -49,42 +49,16 @@ impl MembershipSink for KafkaMembershipSink { ) .await } - - async fn produce_markers( - &self, - markers: Vec, - ) -> Vec> { - send_keyed_iter_to_kafka_with_headers( - &self.producer, - &self.topic, - reconcile_complete_key, - |_| None, - markers, - ) - .await - } } fn membership_key(change: &CohortMembershipChange) -> Option { Some(change.person_id.clone()) } -fn reconcile_complete_key(marker: &ReconcileCompleteMarker) -> Option { - Some(format!( - "{}:{}:{}", - marker.team_id().0, - marker.cohort_id().0, - marker.run_id().0 - )) -} - #[cfg(test)] mod tests { use super::*; - use crate::filters::{CohortId, TeamId}; use crate::producer::MembershipStatus; - use cohort_core::seed::RunId; - use uuid::Uuid; #[test] fn membership_key_is_the_person_id() { @@ -99,33 +73,4 @@ mod tests { }; assert_eq!(membership_key(&change), Some(change.person_id.clone())); } - - #[test] - fn reconcile_complete_key_identifies_the_run_without_the_partition() { - let run_id = RunId(Uuid::nil()); - let marker = ReconcileCompleteMarker::new( - TeamId(42), - CohortId(91204), - 63, - run_id, - "2026-05-26 12:34:56.789123".to_string(), - ); - let other_partition = ReconcileCompleteMarker::new( - TeamId(42), - CohortId(91204), - 0, - run_id, - "2026-05-26 12:34:56.789123".to_string(), - ); - - assert_eq!( - reconcile_complete_key(&marker), - Some("42:91204:00000000-0000-0000-0000-000000000000".to_string()) - ); - assert_eq!( - reconcile_complete_key(&marker), - reconcile_complete_key(&other_partition), - "all partition markers for one run share the same Kafka key", - ); - } } diff --git a/rust/cohort-stream-processor/src/producer/marker.rs b/rust/cohort-stream-processor/src/producer/marker.rs new file mode 100644 index 000000000000..4b12a510ac30 --- /dev/null +++ b/rust/cohort-stream-processor/src/producer/marker.rs @@ -0,0 +1,153 @@ +//! Producer for the `cohort_reconcile_markers` topic. +//! +//! A dedicated producer rather than a second topic on [`crate::producer::KafkaMembershipSink`]: the +//! membership output has to move to its own cluster, while markers stay alongside the seed topic on +//! ingestion, and one shared producer would force them to move together. + +use anyhow::{Context, Result}; +use async_trait::async_trait; +use common_kafka::config::KafkaConfig; +use common_kafka::kafka_producer::{ + create_kafka_producer, send_keyed_iter_to_kafka_with_headers, KafkaContext, KafkaProduceError, +}; +use rdkafka::producer::FutureProducer; + +use crate::producer::kafka::AlwaysHealthy; +use crate::producer::merge::Capture; +use crate::producer::ReconcileCompleteMarker; + +#[async_trait] +pub trait ReconcileMarkerSink: Send + Sync { + async fn produce( + &self, + markers: Vec, + ) -> Vec>; +} + +pub struct KafkaReconcileMarkerSink { + producer: FutureProducer, + topic: String, +} + +impl KafkaReconcileMarkerSink { + pub async fn new(kafka_config: &KafkaConfig, topic: String) -> Result { + let producer = create_kafka_producer(kafka_config, AlwaysHealthy) + .await + .context("creating cohort_reconcile_markers producer")?; + Ok(Self { producer, topic }) + } +} + +#[async_trait] +impl ReconcileMarkerSink for KafkaReconcileMarkerSink { + async fn produce( + &self, + markers: Vec, + ) -> Vec> { + send_keyed_iter_to_kafka_with_headers( + &self.producer, + &self.topic, + reconcile_complete_key, + |_| None, + markers, + ) + .await + } +} + +/// No-op marker sink for when the reconcile gate is off: satisfies the [`ReconcileMarkerSink`] slot +/// without a Kafka producer. The reconcile gate prevents produces from reaching this. +#[derive(Debug, Default, Clone, Copy)] +pub struct NoopReconcileMarkerSink; + +#[async_trait] +impl ReconcileMarkerSink for NoopReconcileMarkerSink { + async fn produce( + &self, + markers: Vec, + ) -> Vec> { + markers.iter().map(|_| Ok(())).collect() + } +} + +#[derive(Debug, Default, Clone)] +pub struct CaptureReconcileMarkerSink(Capture); + +impl CaptureReconcileMarkerSink { + pub fn new() -> Self { + Self::default() + } + + pub fn failing_first(n: usize) -> Self { + Self(Capture::failing_first(n)) + } + + pub fn markers(&self) -> Vec { + self.0.recorded() + } +} + +#[async_trait] +impl ReconcileMarkerSink for CaptureReconcileMarkerSink { + async fn produce( + &self, + markers: Vec, + ) -> Vec> { + self.0.produce(markers) + } +} + +fn reconcile_complete_key(marker: &ReconcileCompleteMarker) -> Option { + Some(format!( + "{}:{}:{}", + marker.team_id().0, + marker.cohort_id().0, + marker.run_id().0 + )) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::filters::{CohortId, TeamId}; + use cohort_core::seed::RunId; + use uuid::Uuid; + + const TS: &str = "2026-05-26 12:34:56.789123"; + + fn marker(partition: u16) -> ReconcileCompleteMarker { + ReconcileCompleteMarker::new( + TeamId(42), + CohortId(91204), + partition, + RunId(Uuid::nil()), + TS.to_string(), + ) + } + + #[test] + fn reconcile_complete_key_identifies_the_run_without_the_partition() { + assert_eq!( + reconcile_complete_key(&marker(63)), + Some("42:91204:00000000-0000-0000-0000-000000000000".to_string()) + ); + assert_eq!( + reconcile_complete_key(&marker(63)), + reconcile_complete_key(&marker(0)), + "all partition markers for one run share the same Kafka key", + ); + } + + #[tokio::test] + async fn capture_marker_sink_retries_a_failed_produce_without_recording_the_attempt() { + let sink = CaptureReconcileMarkerSink::failing_first(1); + + let first = sink.produce(vec![marker(7)]).await; + assert!(first.iter().all(Result::is_err), "first flush fails"); + assert!(sink.markers().is_empty(), "a failed flush records nothing"); + + let second = sink.produce(vec![marker(7)]).await; + assert!(second.iter().all(Result::is_ok), "retry succeeds"); + assert_eq!(sink.markers(), vec![marker(7)]); + } +} diff --git a/rust/cohort-stream-processor/src/producer/mod.rs b/rust/cohort-stream-processor/src/producer/mod.rs index a4788167289f..2a4989abd56b 100644 --- a/rust/cohort-stream-processor/src/producer/mod.rs +++ b/rust/cohort-stream-processor/src/producer/mod.rs @@ -6,6 +6,7 @@ pub mod batcher; pub mod cascade; pub mod kafka; +pub mod marker; pub mod merge; pub mod seed; @@ -31,6 +32,10 @@ use crate::stage1::transition::{LeafTransition, TransitionKind}; pub use batcher::OutputBuffer; pub use cascade::{CaptureCascadeSink, CascadeSink, KafkaCascadeSink, NoopCascadeSink}; pub use kafka::KafkaMembershipSink; +pub use marker::{ + CaptureReconcileMarkerSink, KafkaReconcileMarkerSink, NoopReconcileMarkerSink, + ReconcileMarkerSink, +}; pub use merge::{ CaptureStreamEventSink, CaptureTransferSink, KafkaStreamEventSink, KafkaTransferSink, StreamEventSink, TransferSink, @@ -163,17 +168,11 @@ pub trait MembershipSink: Send + Sync { &self, changes: Vec, ) -> Vec>; - - async fn produce_markers( - &self, - markers: Vec, - ) -> Vec>; } #[derive(Debug, Default, Clone)] pub struct CaptureSink { changes: Arc>>, - markers: Arc>>, fail_remaining: Arc, } @@ -185,7 +184,6 @@ impl CaptureSink { pub fn failing_first(n: usize) -> Self { Self { changes: Arc::default(), - markers: Arc::default(), fail_remaining: Arc::new(AtomicUsize::new(n)), } } @@ -197,13 +195,6 @@ impl CaptureSink { .clone() } - pub fn markers(&self) -> Vec { - self.markers - .lock() - .expect("CaptureSink mutex poisoned") - .clone() - } - fn should_fail(&self) -> bool { self.fail_remaining .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |n| { @@ -232,24 +223,6 @@ impl MembershipSink for CaptureSink { .extend(changes); acks } - - async fn produce_markers( - &self, - markers: Vec, - ) -> Vec> { - if self.should_fail() { - return markers - .into_iter() - .map(|_| Err(KafkaProduceError::KafkaProduceCanceled)) - .collect(); - } - let acks = (0..markers.len()).map(|_| Ok(())).collect(); - self.markers - .lock() - .expect("CaptureSink mutex poisoned") - .extend(markers); - acks - } } #[cfg(test)] @@ -302,16 +275,6 @@ mod tests { } } - fn reconcile_complete_marker(partition: u16) -> ReconcileCompleteMarker { - ReconcileCompleteMarker::new( - TeamId(42), - CohortId(91204), - partition, - RunId(Uuid::nil()), - TS.to_string(), - ) - } - #[test] fn map_transition_entered_maps_to_one_change() { let mut builder = TeamFiltersBuilder::default(); @@ -538,41 +501,4 @@ mod tests { assert!(second.iter().all(Result::is_ok), "second flush succeeds"); assert_eq!(sink.changes(), vec![change]); } - - #[tokio::test] - async fn capture_sink_records_markers_and_shares_failure_order_across_message_types() { - let sink = CaptureSink::failing_first(1); - let change = CohortMembershipChange { - team_id: 42, - cohort_id: 91204, - person_id: "p".to_string(), - last_updated: TS.to_string(), - status: MembershipStatus::Entered, - origin: Some(ChangeOrigin::Reconcile), - run_id: Some(RunId(Uuid::nil())), - }; - let marker = reconcile_complete_marker(7); - - let first = sink.produce(vec![change]).await; - assert!(first.iter().all(Result::is_err), "first flush fails"); - assert!(sink.changes().is_empty(), "a failed flush records nothing"); - - let second = sink.produce_markers(vec![marker.clone()]).await; - assert!(second.iter().all(Result::is_ok), "second flush succeeds"); - assert_eq!(sink.markers(), vec![marker]); - } - - #[tokio::test] - async fn capture_sink_retries_a_failed_marker_without_recording_the_failed_attempt() { - let sink = CaptureSink::failing_first(1); - let marker = reconcile_complete_marker(7); - - let first = sink.produce_markers(vec![marker.clone()]).await; - assert!(first.iter().all(Result::is_err), "first flush fails"); - assert!(sink.markers().is_empty(), "a failed flush records nothing"); - - let second = sink.produce_markers(vec![marker.clone()]).await; - assert!(second.iter().all(Result::is_ok), "retry succeeds"); - assert_eq!(sink.markers(), vec![marker]); - } } diff --git a/rust/cohort-stream-processor/src/workers/reconcile.rs b/rust/cohort-stream-processor/src/workers/reconcile.rs index 3c6ac5d196dd..3acb4d7d7fe6 100644 --- a/rust/cohort-stream-processor/src/workers/reconcile.rs +++ b/rust/cohort-stream-processor/src/workers/reconcile.rs @@ -22,12 +22,13 @@ use crate::filters::reverse_index::TeamFilters; use crate::observability::metrics::{ COHORT_STREAM_OFFSET_AHEAD_OF_DISPATCH, RECONCILE_BITS_FIXED_TOTAL, RECONCILE_JOBS_COMPLETED_TOTAL, RECONCILE_JOBS_DISCARDED_TOTAL, - RECONCILE_MARKERS_EMITTED_TOTAL, RECONCILE_QUEUE_DEPTH, RECONCILE_ROWS_EMITTED_TOTAL, - RECONCILE_ROWS_SCANNED_TOTAL, + RECONCILE_MARKERS_EMITTED_TOTAL, RECONCILE_MARKER_PRODUCE_ERRORS, RECONCILE_QUEUE_DEPTH, + RECONCILE_ROWS_EMITTED_TOTAL, RECONCILE_ROWS_SCANNED_TOTAL, }; use crate::partitions::offset_tracker::{DeferredOffset, MarkOutcome, OffsetTracker}; use crate::producer::{ - ChangeOrigin, CohortMembershipChange, MembershipSink, ReconcileCompleteMarker, + ChangeOrigin, CohortMembershipChange, MembershipSink, NoopReconcileMarkerSink, + ReconcileCompleteMarker, ReconcileMarkerSink, }; use crate::stage2::Stage2State; use crate::store::{ @@ -85,11 +86,13 @@ impl ReconcileBacklog { } /// Runtime dependencies shared by every partition's reconcile queue. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct ReconcileDeps { pub enabled: bool, pub scan_page: usize, pub backlog: Arc, + /// Sink for `cohort_reconcile_markers` (a no-op when the reconcile gate is off). + pub marker_sink: Arc, } impl Default for ReconcileDeps { @@ -98,6 +101,7 @@ impl Default for ReconcileDeps { enabled: false, scan_page: DEFAULT_RECONCILE_SCAN_PAGE, backlog: Arc::new(ReconcileBacklog::default()), + marker_sink: Arc::new(NoopReconcileMarkerSink), } } } @@ -450,7 +454,6 @@ pub(crate) async fn handle_reconcile_drain( drain_marker( partition_id, handle, - sink, merge, queue, &tile, @@ -711,11 +714,9 @@ async fn drain_dirty( /// Emit the per-partition completion marker once the dirty set is drained. A failed marker produce /// retries the marker only; work dirtied while the marker was pending is settled before the retry. -#[allow(clippy::too_many_arguments)] async fn drain_marker( partition_id: u16, handle: &StoreHandle, - sink: &Arc, merge: &MergeWorkerDeps, queue: &mut ReconcileQueue, tile: &ReconcileTile, @@ -750,10 +751,12 @@ async fn drain_marker( tile.run_id(), last_updated.to_string(), ); - let acks = sink.produce_markers(vec![marker]).await; + let acks = merge.reconcile.marker_sink.produce(vec![marker]).await; + let kind = tile.scope().kind().as_str(); // Exactly one marker went in, so anything but a single successful ack is a produce failure. let failed_acks = acks.iter().filter(|ack| ack.is_err()).count(); if acks.len() != 1 || failed_acks > 0 { + counter!(RECONCILE_MARKER_PRODUCE_ERRORS, "kind" => kind).increment(1); warn_job!( tile, partition_id, @@ -764,7 +767,6 @@ async fn drain_marker( return DrainStep::Yield; } - let kind = tile.scope().kind().as_str(); counter!(RECONCILE_MARKERS_EMITTED_TOTAL, "kind" => kind).increment(1); let completed = queue .finish_front() @@ -968,11 +970,7 @@ fn complete_offset( #[cfg(test)] #[allow(clippy::disallowed_methods)] mod tests { - use std::sync::atomic::AtomicBool; - - use async_trait::async_trait; use chrono_tz::UTC; - use common_kafka::kafka_producer::KafkaProduceError; use serde_json::{json, Value}; use tempfile::TempDir; use uuid::Uuid; @@ -983,7 +981,9 @@ mod tests { use crate::filters::tree::{CohortTree, FilterNode}; use crate::filters::{BoolOp, TeamFiltersBuilder}; use crate::partitions::offset_tracker::OffsetTracker; - use crate::producer::{CaptureCascadeSink, CaptureSink, MembershipStatus}; + use crate::producer::{ + CaptureCascadeSink, CaptureReconcileMarkerSink, CaptureSink, MembershipStatus, + }; use crate::stage1::person_record::{MatchedSet, PersonRecord}; use crate::stage1::state::{AppliedOffsets, Stage1State, StatefulRecord}; use crate::stage2::state::Stage2Ownership; @@ -1094,6 +1094,7 @@ mod tests { handle: StoreHandle, catalog: CatalogHandle, sink: Arc, + markers: CaptureReconcileMarkerSink, deps: MergeWorkerDeps, queue: ReconcileQueue, lsk: LeafStateKey, @@ -1131,6 +1132,8 @@ mod tests { deps.reconcile.scan_page = scan_page; deps.cascade.enabled = true; deps.cascade_sink = Arc::new(cascade_sink); + let markers = CaptureReconcileMarkerSink::new(); + deps.reconcile.marker_sink = Arc::new(markers.clone()); let queue = ReconcileQueue::new(PARTITION, deps.reconcile.backlog.clone(), handle.clone()); Self { @@ -1139,12 +1142,20 @@ mod tests { handle, catalog, sink, + markers, deps, queue, lsk, } } + /// Re-arm the marker sink to fail its first produce, isolating the marker phase's retry from + /// the membership leg's. + fn fail_first_marker(&mut self) { + self.markers = CaptureReconcileMarkerSink::failing_first(1); + self.deps.reconcile.marker_sink = Arc::new(self.markers.clone()); + } + fn write_current(&self, person: Uuid, behavioral: bool, person_match: bool, prior: bool) { if behavioral { let key = BehavioralKey::new(PARTITION, TEAM as u64, person, self.lsk); @@ -1500,7 +1511,7 @@ mod tests { assert!(shell.queue.front().is_none()); assert!(shell.deps.reconcile.backlog.is_empty()); - let markers = sink.markers(); + let markers = shell.markers.markers(); assert_eq!( markers.len(), 1, @@ -1672,7 +1683,7 @@ mod tests { shell.tick().await; assert_eq!(sink.changes().len(), 2, "one bounded page was emitted"); - assert!(sink.markers().is_empty()); + assert!(shell.markers.markers().is_empty()); assert_eq!(shell.committable(), Some(5)); assert!(matches!( shell.queue.front().map(|job| &job.phase), @@ -1710,7 +1721,7 @@ mod tests { assert_eq!(cascade_messages[0].change.person_id, alice.to_string()); assert_eq!(cascade_messages[1].change.person_id, bob.to_string()); shell.tick().await; - assert_eq!(sink.markers().len(), 1); + assert_eq!(shell.markers.markers().len(), 1); assert!(shell.queue.front().is_none()); assert!(shell.deps.reconcile.backlog.is_empty()); assert_eq!(shell.committable(), Some(6)); @@ -1734,7 +1745,9 @@ mod tests { // Both jobs lease the same Stage 2 prefix in turn; neither may release the other's capture. assert_eq!( - sink.markers() + shell + .markers + .markers() .iter() .map(ReconcileCompleteMarker::run_id) .collect::>(), @@ -1771,8 +1784,11 @@ mod tests { ], "the snapshot is computed from person state, not a behavioral leaf", ); - assert_eq!(sink.markers().len(), 1); - assert_eq!(sink.markers()[0].run_id(), RunId(Uuid::from_u128(21))); + assert_eq!(shell.markers.markers().len(), 1); + assert_eq!( + shell.markers.markers()[0].run_id(), + RunId(Uuid::from_u128(21)) + ); assert!(shell.queue.front().is_none()); } @@ -1830,7 +1846,7 @@ mod tests { shell.tick().await; assert_eq!(sink.changes().len(), 1); assert_eq!(sink.changes()[0].person_id, later_key.to_string()); - assert!(sink.markers().is_empty()); + assert!(shell.markers.markers().is_empty()); shell.write_current(inserted_behind_cursor, true, false, true); shell.tick().await; @@ -1848,7 +1864,7 @@ mod tests { vec![later_key.to_string(), inserted_behind_cursor.to_string(),], "the main cohort page is not rescanned because another person changed", ); - assert_eq!(sink.markers().len(), 1); + assert_eq!(shell.markers.markers().len(), 1); assert_eq!(shell.committable(), Some(6)); } @@ -1877,7 +1893,7 @@ mod tests { shell.tick().await; assert_eq!(sink.changes().last().unwrap().person_id, hot.to_string()); assert_eq!( - sink.markers().len(), + shell.markers.markers().len(), 1, "the job completes in the same tick that settles the full hot-person page", ); @@ -1919,7 +1935,7 @@ mod tests { "the tombstoned scan row is not evaluated or emitted from its dirty marker", ); assert!(shell.store.get_stage2(&deleted_key).unwrap().is_none()); - assert_eq!(sink.markers().len(), 1); + assert_eq!(shell.markers.markers().len(), 1); } #[tokio::test] @@ -1938,7 +1954,7 @@ mod tests { assert!(shell.stage2_bit(alice)); assert!(cascades.messages().is_empty()); shell.tick().await; - assert_eq!(sink.markers().len(), 1); + assert_eq!(shell.markers.markers().len(), 1); assert_eq!(shell.committable(), Some(6)); } @@ -1953,7 +1969,7 @@ mod tests { shell.tick().await; assert!(sink.changes().is_empty()); - let markers = sink.markers(); + let markers = shell.markers.markers(); assert_eq!(markers.len(), 1); assert_eq!(markers[0].run_id(), RunId(Uuid::from_u128(2))); assert!(shell.queue.front().is_none()); @@ -1972,7 +1988,7 @@ mod tests { shell.tick().await; assert!(sink.changes().is_empty()); - assert!(sink.markers().is_empty()); + assert!(shell.markers.markers().is_empty()); assert!(!shell.stage2_bit(alice)); assert!(matches!( shell.queue.front().map(|job| &job.phase), @@ -1984,7 +2000,7 @@ mod tests { assert_eq!(sink.changes().len(), 1); assert!(shell.stage2_bit(alice)); shell.tick().await; - assert_eq!(sink.markers().len(), 1); + assert_eq!(shell.markers.markers().len(), 1); assert_eq!(shell.committable(), Some(6)); } @@ -2000,7 +2016,7 @@ mod tests { shell.tick().await; assert_eq!(sink.changes().len(), 1, "the membership leg acked"); - assert!(sink.markers().is_empty()); + assert!(shell.markers.markers().is_empty()); assert!(!shell.stage2_bit(alice)); assert!(matches!( shell.queue.front().map(|job| &job.phase), @@ -2013,51 +2029,15 @@ mod tests { assert_eq!(cascades.messages().len(), 1); assert!(shell.stage2_bit(alice)); shell.tick().await; - assert_eq!(sink.markers().len(), 1); + assert_eq!(shell.markers.markers().len(), 1); assert_eq!(shell.committable(), Some(6)); } - struct FailFirstMarkerSink { - capture: CaptureSink, - fail_marker: AtomicBool, - } - - impl FailFirstMarkerSink { - fn new() -> Self { - Self { - capture: CaptureSink::new(), - fail_marker: AtomicBool::new(true), - } - } - } - - #[async_trait] - impl MembershipSink for FailFirstMarkerSink { - async fn produce( - &self, - changes: Vec, - ) -> Vec> { - self.capture.produce(changes).await - } - - async fn produce_markers( - &self, - markers: Vec, - ) -> Vec> { - if self.fail_marker.swap(false, Ordering::SeqCst) { - return markers - .into_iter() - .map(|_| Err(KafkaProduceError::KafkaProduceCanceled)) - .collect(); - } - self.capture.produce_markers(markers).await - } - } - #[tokio::test] async fn marker_failure_retries_only_the_marker_phase() { - let sink = Arc::new(FailFirstMarkerSink::new()); - let mut shell = DrainShell::new(true, sink.clone(), CaptureCascadeSink::new(), 8); + let sink = CaptureSink::new(); + let mut shell = DrainShell::new(true, Arc::new(sink.clone()), CaptureCascadeSink::new(), 8); + shell.fail_first_marker(); let alice = Uuid::from_u128(1); shell.write_current(alice, true, false, false); shell.enqueue(tile(TEAM, COHORT, 15), 5); @@ -2065,8 +2045,8 @@ mod tests { shell.tick().await; shell.tick().await; - assert_eq!(sink.capture.changes().len(), 1); - assert!(sink.capture.markers().is_empty()); + assert_eq!(sink.changes().len(), 1); + assert!(shell.markers.markers().is_empty()); assert!(shell.stage2_bit(alice), "the page committed before marker"); assert!(matches!( shell.queue.front().map(|job| &job.phase), @@ -2076,20 +2056,17 @@ mod tests { shell.tick().await; - assert_eq!( - sink.capture.changes().len(), - 1, - "the page was not rescanned" - ); - assert_eq!(sink.capture.markers().len(), 1); + assert_eq!(sink.changes().len(), 1, "the page was not rescanned"); + assert_eq!(shell.markers.markers().len(), 1); assert!(shell.queue.front().is_none()); assert_eq!(shell.committable(), Some(6)); } #[tokio::test] async fn marker_retry_drains_new_dirty_work_without_rescanning_clean_rows() { - let sink = Arc::new(FailFirstMarkerSink::new()); - let mut shell = DrainShell::new(true, sink.clone(), CaptureCascadeSink::new(), 8); + let sink = CaptureSink::new(); + let mut shell = DrainShell::new(true, Arc::new(sink.clone()), CaptureCascadeSink::new(), 8); + shell.fail_first_marker(); let alice = Uuid::from_u128(2); let bob = Uuid::from_u128(1); shell.write_current(alice, true, false, true); @@ -2097,8 +2074,8 @@ mod tests { shell.tick().await; shell.tick().await; - assert_eq!(sink.capture.changes().len(), 1); - assert!(sink.capture.markers().is_empty()); + assert_eq!(sink.changes().len(), 1); + assert!(shell.markers.markers().is_empty()); assert!(matches!( shell.queue.front().map(|job| &job.phase), Some(ScanPhase::MarkerReady), @@ -2108,15 +2085,14 @@ mod tests { shell.tick().await; assert_eq!( - sink.capture - .changes() + sink.changes() .iter() .map(|change| change.person_id.clone()) .collect::>(), vec![alice.to_string(), bob.to_string()], "a failed marker drains only the person that changed while the marker was pending", ); - assert_eq!(sink.capture.markers().len(), 1); + assert_eq!(shell.markers.markers().len(), 1); assert_eq!(shell.committable(), Some(6)); } } diff --git a/rust/cohort-stream-processor/src/workers/seed_path.rs b/rust/cohort-stream-processor/src/workers/seed_path.rs index 7ea25b9a07b3..87057217acf6 100644 --- a/rust/cohort-stream-processor/src/workers/seed_path.rs +++ b/rust/cohort-stream-processor/src/workers/seed_path.rs @@ -984,7 +984,9 @@ mod tests { use crate::filters::{CohortId, FilterCatalog, TeamFiltersBuilder}; use crate::merge::transfer::Tombstone; use crate::partitions::partitioner::{partition_of, COHORT_PARTITION_COUNT}; - use crate::producer::{CaptureSeedTileSink, CaptureSink, MembershipStatus}; + use crate::producer::{ + CaptureReconcileMarkerSink, CaptureSeedTileSink, CaptureSink, MembershipStatus, + }; use crate::stage1::state::AppliedOffsets; use crate::stage2::state::Stage2State; use crate::store::{ @@ -1796,6 +1798,7 @@ mod tests { sink: CaptureSink, seed_sink: CaptureSeedTileSink, cascade_sink: crate::producer::CaptureCascadeSink, + marker_sink: CaptureReconcileMarkerSink, deps: MergeWorkerDeps, queue: EvictionQueue, reconcile_queue: ReconcileQueue, @@ -1850,6 +1853,7 @@ mod tests { TEAM, build_filters(cohorts, UTC), )]))); + let marker_sink = CaptureReconcileMarkerSink::new(); let deps = MergeWorkerDeps { transfer_sink: Arc::new(crate::producer::CaptureTransferSink::new()), stream_event_sink: Arc::new(crate::producer::CaptureStreamEventSink::new()), @@ -1866,7 +1870,10 @@ mod tests { seed_tracker: Arc::new(OffsetTracker::new()), live_watermarks: Arc::new(crate::partitions::watermarks::LiveWatermarks::new()), register_transfer_enabled: false, - reconcile: crate::workers::ReconcileDeps::default(), + reconcile: crate::workers::ReconcileDeps { + marker_sink: Arc::new(marker_sink.clone()), + ..crate::workers::ReconcileDeps::default() + }, person_seed: crate::workers::PersonSeedDeps::default(), }; let reconcile_queue = @@ -1879,6 +1886,7 @@ mod tests { sink, seed_sink, cascade_sink, + marker_sink, deps, queue: EvictionQueue::new(), reconcile_queue, @@ -1940,7 +1948,7 @@ mod tests { assert_eq!(shell.reconcile_queue.len(), 0); assert!(shell.deps.reconcile.backlog.is_empty()); assert!(shell.sink.changes().is_empty()); - assert!(shell.sink.markers().is_empty()); + assert!(shell.marker_sink.markers().is_empty()); } #[tokio::test] diff --git a/rust/cohort-stream-processor/src/workers/worker.rs b/rust/cohort-stream-processor/src/workers/worker.rs index 1cc08ff965b4..d5c1f205d7aa 100644 --- a/rust/cohort-stream-processor/src/workers/worker.rs +++ b/rust/cohort-stream-processor/src/workers/worker.rs @@ -1079,7 +1079,8 @@ mod tombstone_redirect_tests { use crate::merge::transfer::Tombstone; use crate::partitions::partitioner::{partition_of, COHORT_PARTITION_COUNT}; use crate::producer::{ - CaptureCascadeSink, CaptureSink, CaptureStreamEventSink, CaptureTransferSink, + CaptureCascadeSink, CaptureReconcileMarkerSink, CaptureSink, CaptureStreamEventSink, + CaptureTransferSink, }; use crate::stage1::person_record::PersonRecord; use crate::stage1::state::AppliedOffsets; @@ -1217,12 +1218,14 @@ mod tombstone_redirect_tests { }) } - fn reconcile_deps() -> Arc { + fn reconcile_deps() -> (Arc, CaptureReconcileMarkerSink) { let mut deps = Arc::try_unwrap(merge_deps_with(CaptureStreamEventSink::new())) .unwrap_or_else(|_| panic!("test owns the only dependency Arc")); deps.reconcile.enabled = true; deps.reconcile.scan_page = 2; - Arc::new(deps) + let markers = CaptureReconcileMarkerSink::new(); + deps.reconcile.marker_sink = Arc::new(markers.clone()); + (Arc::new(deps), markers) } /// Deps with the `stage2_orphan_gc_enabled` kill-switch set to `stage2_orphan_gc_enabled`. @@ -1343,7 +1346,7 @@ mod tombstone_redirect_tests { let (_dir, store) = temp_store(); let membership = CaptureSink::new(); - let merge = reconcile_deps(); + let (merge, marker_sink) = reconcile_deps(); let seed_tracker = merge.seed_tracker.clone(); let backlog = merge.reconcile.backlog.clone(); let events_tracker = Arc::new(OffsetTracker::new()); @@ -1374,7 +1377,7 @@ mod tombstone_redirect_tests { .await; assert!(membership.changes().is_empty()); - let markers = membership.markers(); + let markers = marker_sink.markers(); assert_eq!(markers.len(), 1); assert_eq!(markers[0].partition(), 0); assert_eq!(markers[0].run_id(), RunId(Uuid::from_u128(7))); diff --git a/rust/cohort-stream-processor/tests/events_consumer.rs b/rust/cohort-stream-processor/tests/events_consumer.rs index 24642fd36153..52561fbbf0a6 100644 --- a/rust/cohort-stream-processor/tests/events_consumer.rs +++ b/rust/cohort-stream-processor/tests/events_consumer.rs @@ -45,7 +45,6 @@ use cohort_stream_processor::partitions::{ }; use cohort_stream_processor::producer::{ CaptureSink, CohortMembershipChange, KafkaMembershipSink, MembershipSink, MembershipStatus, - ReconcileCompleteMarker, }; use cohort_stream_processor::stage1::{Stage1State, StatefulRecord}; use cohort_stream_processor::store::durability::{ @@ -731,16 +730,6 @@ impl MembershipSink for BarrierSink { .extend(changes); acks } - - async fn produce_markers( - &self, - markers: Vec, - ) -> Vec> { - markers - .into_iter() - .map(|_| Err(KafkaProduceError::KafkaProduceCanceled)) - .collect() - } } #[tokio::test] diff --git a/rust/cohort-stream-processor/tests/merge_durability.rs b/rust/cohort-stream-processor/tests/merge_durability.rs index 5971c165af62..ccb635ac93fb 100644 --- a/rust/cohort-stream-processor/tests/merge_durability.rs +++ b/rust/cohort-stream-processor/tests/merge_durability.rs @@ -557,6 +557,7 @@ struct Topics { transfers: String, cascade: String, shadow: String, + markers: String, } impl Topics { @@ -567,6 +568,7 @@ impl Topics { transfers: format!("cohort_merge_state_transfer_{suffix}"), cascade: format!("cohort_cascade_events_{suffix}"), shadow: format!("cohort_membership_changed_{suffix}"), + markers: format!("cohort_reconcile_markers_{suffix}"), } } @@ -576,13 +578,14 @@ impl Topics { } } - fn names(&self) -> [&str; 5] { + fn names(&self) -> [&str; 6] { [ &self.events, &self.merges, &self.transfers, &self.cascade, &self.shadow, + &self.markers, ] } } @@ -905,6 +908,10 @@ fn merge_durability_config( "COHORT_MEMBERSHIP_CHANGED_TOPIC".into(), topics.shadow.clone(), ); + env.insert( + "COHORT_RECONCILE_MARKERS_TOPIC".into(), + topics.markers.clone(), + ); env.insert("KAFKA_CONSUMER_GROUP".into(), groups.events.clone()); env.insert("KAFKA_MERGE_CONSUMER_GROUP".into(), groups.merges.clone()); env.insert( diff --git a/rust/cohort-stream-processor/tests/seed_consumer.rs b/rust/cohort-stream-processor/tests/seed_consumer.rs index d8ba82f973f3..a14a8abe12af 100644 --- a/rust/cohort-stream-processor/tests/seed_consumer.rs +++ b/rust/cohort-stream-processor/tests/seed_consumer.rs @@ -39,8 +39,9 @@ use cohort_stream_processor::partitions::{ }; use cohort_stream_processor::producer::{ CascadeSink, ChangeOrigin, CohortMembershipChange, KafkaCascadeSink, KafkaMembershipSink, - KafkaSeedTileSink, KafkaStreamEventSink, KafkaTransferSink, MembershipSink, MembershipStatus, - ReconcileCompleteMarker, SeedTileSink, StreamEventSink, TransferSink, + KafkaReconcileMarkerSink, KafkaSeedTileSink, KafkaStreamEventSink, KafkaTransferSink, + MembershipSink, MembershipStatus, ReconcileCompleteMarker, ReconcileMarkerSink, SeedTileSink, + StreamEventSink, TransferSink, }; use cohort_stream_processor::stage1::bucket_tz::day_idx_in_tz; use cohort_stream_processor::stage2::state::Stage2State; @@ -277,6 +278,49 @@ async fn consume_all(topic: &str, expected: usize, deadline: Duration) -> Vec<(i messages } +fn split_records( + records: Vec<(i32, Vec)>, +) -> (Vec, Vec) { + let mut changes = Vec::new(); + let mut markers = Vec::new(); + for (_, payload) in records { + let value: serde_json::Value = serde_json::from_slice(&payload).unwrap(); + if value.get("type").and_then(serde_json::Value::as_str) == Some("reconcile_complete") { + markers.push(serde_json::from_slice::(&payload).unwrap()); + } else { + changes.push(serde_json::from_slice::(&payload).unwrap()); + } + } + (changes, markers) +} + +/// Drain the membership topic, proving it carries exactly `expected` membership rows and nothing +/// else. The count catches an extra record appearing at all; the marker arm catches that record +/// being a `reconcile_complete` — the poison pill the dedicated marker topic exists to keep off this +/// topic, and which its ClickHouse and CDP consumers cannot survive. +async fn drain_membership_only(topic: &str, expected: usize) -> Vec { + assert_eq!( + topic_message_count(topic), + expected as i64, + "the membership topic must carry membership rows only", + ); + let (changes, markers) = + split_records(consume_all(topic, expected, Duration::from_secs(30)).await); + assert!( + markers.is_empty(), + "a completion marker on the membership topic poisons its consumers", + ); + changes +} + +/// Drain `expected` completion markers off the dedicated marker topic. +async fn drain_markers(topic: &str, expected: usize) -> Vec { + let (changes, markers) = + split_records(consume_all(topic, expected, Duration::from_secs(30)).await); + assert!(changes.is_empty(), "the marker topic carries markers only"); + markers +} + async fn wait_for(what: &str, deadline: Duration, mut condition: impl FnMut() -> bool) { let start = Instant::now(); while !condition() { @@ -420,6 +464,7 @@ struct Topics { cascade: String, seeds: String, shadow: String, + markers: String, } impl Topics { @@ -431,6 +476,7 @@ impl Topics { cascade: format!("cohort_cascade_events_{suffix}"), seeds: format!("cohort_stream_seed_events_{suffix}"), shadow: format!("cohort_membership_changed_{suffix}"), + markers: format!("cohort_reconcile_markers_{suffix}"), } } @@ -440,7 +486,7 @@ impl Topics { } } - fn names(&self) -> [&str; 6] { + fn names(&self) -> [&str; 7] { [ &self.events, &self.merges, @@ -448,6 +494,7 @@ impl Topics { &self.cascade, &self.seeds, &self.shadow, + &self.markers, ] } } @@ -599,6 +646,11 @@ async fn spawn_instance( .await .expect("create membership sink"), ); + let marker_sink: Arc = Arc::new( + KafkaReconcileMarkerSink::new(&kafka_config, topics.markers.clone()) + .await + .expect("create reconcile marker sink"), + ); let merge_deps = Arc::new(MergeWorkerDeps { transfer_sink, stream_event_sink, @@ -619,6 +671,7 @@ async fn spawn_instance( enabled: true, scan_page: 1, backlog: reconcile_backlog.clone(), + marker_sink, }, person_seed: cohort_stream_processor::workers::PersonSeedDeps::default(), }); @@ -943,7 +996,10 @@ async fn reconcile_snapshot_repairs_stale_state_and_commits_after_markers() { wait_for( "63 empty-partition markers and one stale-row repair", Duration::from_secs(60), - || topic_message_count(&topics.shadow) == NUM_PARTITIONS as i64, + || { + topic_message_count(&topics.markers) == i64::from(NUM_PARTITIONS) - 1 + && topic_message_count(&topics.shadow) == 1 + }, ) .await; wait_for( @@ -964,29 +1020,14 @@ async fn reconcile_snapshot_repairs_stale_state_and_commits_after_markers() { ); assert!(!instance.stage2(&stage2_key).await.in_cohort); - let first_page = consume_all( - &topics.shadow, - NUM_PARTITIONS as usize, - Duration::from_secs(30), - ) - .await; - let mut first_changes = Vec::new(); - let mut first_markers = Vec::new(); - for (_, payload) in first_page { - let value: serde_json::Value = serde_json::from_slice(&payload).unwrap(); - if value.get("type").and_then(serde_json::Value::as_str) == Some("reconcile_complete") { - first_markers - .push(serde_json::from_slice::(&payload).unwrap()); - } else { - first_changes - .push(serde_json::from_slice::(&payload).unwrap()); - } - } + let first_changes = drain_membership_only(&topics.shadow, 1).await; assert_eq!(first_changes.len(), 1); assert_eq!(first_changes[0].person_id, person.to_string()); assert_eq!(first_changes[0].status, MembershipStatus::Left); assert_eq!(first_changes[0].origin, Some(ChangeOrigin::Reconcile)); assert_eq!(first_changes[0].run_id, Some(run_id)); + + let first_markers = drain_markers(&topics.markers, NUM_PARTITIONS as usize - 1).await; assert_eq!(first_markers.len(), NUM_PARTITIONS as usize - 1); assert!(!first_markers .iter() @@ -996,7 +1037,7 @@ async fn reconcile_snapshot_repairs_stale_state_and_commits_after_markers() { wait_for( "the final partition marker", Duration::from_secs(30), - || topic_message_count(&topics.shadow) == i64::from(NUM_PARTITIONS) + 1, + || topic_message_count(&topics.markers) == i64::from(NUM_PARTITIONS), ) .await; wait_for( @@ -1017,33 +1058,24 @@ async fn reconcile_snapshot_repairs_stale_state_and_commits_after_markers() { assert!((0..NUM_PARTITIONS) .all(|partition| { instance.seed_committable(partition as u16) == Some(1) })); - let first_snapshot = consume_all( - &topics.shadow, - NUM_PARTITIONS as usize + 1, - Duration::from_secs(30), - ) - .await; - let mut marker_partitions = HashSet::new(); - let mut snapshot_changes = Vec::new(); - for (_, payload) in first_snapshot { - let value: serde_json::Value = serde_json::from_slice(&payload).unwrap(); - if value.get("type").and_then(serde_json::Value::as_str) == Some("reconcile_complete") { - let marker: ReconcileCompleteMarker = serde_json::from_slice(&payload).unwrap(); + let snapshot_changes = drain_membership_only(&topics.shadow, 1).await; + assert_eq!(snapshot_changes.len(), 1); + assert_eq!(snapshot_changes[0].status, MembershipStatus::Left); + + let snapshot_markers = drain_markers(&topics.markers, NUM_PARTITIONS as usize).await; + let marker_partitions: HashSet = snapshot_markers + .iter() + .map(|marker| { assert_eq!(marker.team_id(), TeamId(TEAM)); assert_eq!(marker.cohort_id(), CohortId(COHORT)); assert_eq!(marker.run_id(), run_id); - marker_partitions.insert(marker.partition()); - } else { - snapshot_changes - .push(serde_json::from_slice::(&payload).unwrap()); - } - } + marker.partition() + }) + .collect(); let expected_marker_partitions: HashSet = (0..COHORT_PARTITION_COUNT) .map(|partition| partition as u16) .collect(); assert_eq!(marker_partitions, expected_marker_partitions); - assert_eq!(snapshot_changes.len(), 1); - assert_eq!(snapshot_changes[0].status, MembershipStatus::Left); // A duplicate manual dispatch emits the same full snapshot and certificate set, leaves the // repaired bit unchanged, and advances every partition by exactly one more seed offset. @@ -1063,7 +1095,10 @@ async fn reconcile_snapshot_repairs_stale_state_and_commits_after_markers() { wait_for( "the duplicate snapshot first page", Duration::from_secs(60), - || topic_message_count(&topics.shadow) == i64::from(NUM_PARTITIONS) * 2 + 1, + || { + topic_message_count(&topics.markers) == i64::from(NUM_PARTITIONS) * 2 - 1 + && topic_message_count(&topics.shadow) == 2 + }, ) .await; assert_eq!( @@ -1080,7 +1115,7 @@ async fn reconcile_snapshot_repairs_stale_state_and_commits_after_markers() { wait_for( "the duplicate snapshot marker set", Duration::from_secs(30), - || topic_message_count(&topics.shadow) == i64::from(NUM_PARTITIONS) * 2 + 2, + || topic_message_count(&topics.markers) == i64::from(NUM_PARTITIONS) * 2, ) .await; wait_for( @@ -1096,26 +1131,14 @@ async fn reconcile_snapshot_repairs_stale_state_and_commits_after_markers() { .all(|partition| { instance.seed_committable(partition as u16) == Some(2) })); assert!(!instance.stage2(&stage2_key).await.in_cohort); - let converged = consume_all( - &topics.shadow, - (NUM_PARTITIONS as usize * 2) + 2, - Duration::from_secs(30), - ) - .await; + let reconcile_changes = drain_membership_only(&topics.shadow, 2).await; + let converged_markers = drain_markers(&topics.markers, NUM_PARTITIONS as usize * 2).await; let mut marker_counts = HashMap::::new(); - let mut reconcile_changes = Vec::new(); - for (_, payload) in converged { - let value: serde_json::Value = serde_json::from_slice(&payload).unwrap(); - if value.get("type").and_then(serde_json::Value::as_str) == Some("reconcile_complete") { - let marker: ReconcileCompleteMarker = serde_json::from_slice(&payload).unwrap(); - assert_eq!(marker.team_id(), TeamId(TEAM)); - assert_eq!(marker.cohort_id(), CohortId(COHORT)); - assert_eq!(marker.run_id(), run_id); - *marker_counts.entry(marker.partition()).or_default() += 1; - } else { - reconcile_changes - .push(serde_json::from_slice::(&payload).unwrap()); - } + for marker in &converged_markers { + assert_eq!(marker.team_id(), TeamId(TEAM)); + assert_eq!(marker.cohort_id(), CohortId(COHORT)); + assert_eq!(marker.run_id(), run_id); + *marker_counts.entry(marker.partition()).or_default() += 1; } assert_eq!( marker_counts.keys().copied().collect::>(), @@ -1274,7 +1297,7 @@ async fn fence_holds_a_fresh_tile_until_live_consumption_passes_its_scan_point() wait_for( "the reconcile completion marker", Duration::from_secs(30), - || topic_message_count(&topics.shadow) == 3, + || topic_message_count(&topics.markers) == 1, ) .await; assert!(instance.reconcile_backlog.is_empty()); @@ -1286,17 +1309,8 @@ async fn fence_holds_a_fresh_tile_until_live_consumption_passes_its_scan_point() ) .await; - let outputs = consume_all(&topics.shadow, 3, Duration::from_secs(30)).await; - let mut changes = Vec::new(); - let mut markers = Vec::new(); - for (_, payload) in outputs { - let value: serde_json::Value = serde_json::from_slice(&payload).unwrap(); - if value.get("type").and_then(serde_json::Value::as_str) == Some("reconcile_complete") { - markers.push(serde_json::from_slice::(&payload).unwrap()); - } else { - changes.push(serde_json::from_slice::(&payload).unwrap()); - } - } + let changes = drain_membership_only(&topics.shadow, 2).await; + let markers = drain_markers(&topics.markers, 1).await; assert_eq!(changes.len(), 2); assert!(changes.iter().any(|change| { change.person_id == alice.to_string() && change.origin == Some(ChangeOrigin::Seed) diff --git a/rust/cohort-stream-processor/tests/sweep_worker.rs b/rust/cohort-stream-processor/tests/sweep_worker.rs index 45fde3e3c72f..97541d8751dc 100644 --- a/rust/cohort-stream-processor/tests/sweep_worker.rs +++ b/rust/cohort-stream-processor/tests/sweep_worker.rs @@ -27,7 +27,7 @@ use cohort_stream_processor::partitions::{ MeteredReceiver, OffsetTracker, PartitionRouter, ShuffleMessage, }; use cohort_stream_processor::producer::{ - CaptureSink, CohortMembershipChange, MembershipSink, MembershipStatus, ReconcileCompleteMarker, + CaptureSink, CohortMembershipChange, MembershipSink, MembershipStatus, }; use cohort_stream_processor::stage1::bucket_tz::{day_idx_in_tz, start_of_day_ms_in_tz}; use cohort_stream_processor::stage1::{ @@ -1240,16 +1240,6 @@ impl MembershipSink for FailNthSink { self.changes.lock().unwrap().extend(changes); acks } - - async fn produce_markers( - &self, - markers: Vec, - ) -> Vec> { - markers - .into_iter() - .map(|_| Err(KafkaProduceError::KafkaProduceCanceled)) - .collect() - } } #[tokio::test] From 0b24fc8cc6bbd10443b5588f56d3f016a760eb7b Mon Sep 17 00:00:00 2001 From: Matheus Veras Batista Date: Sat, 1 Aug 2026 01:31:33 -0300 Subject: [PATCH 2/4] fixes --- rust/cohort-seeder/src/app/observe.rs | 29 +++++++++++++++++-- .../src/bin/reconcile_dispatch.rs | 18 ++++++++++++ rust/cohort-seeder/src/config.rs | 4 ++- rust/cohort-seeder/src/domain/completion.rs | 21 +++++++++++++- rust/cohort-seeder/src/main.rs | 9 +++++- .../tests/fixtures/cohort_backfill_0009.sql | 4 ++- rust/cohort-stream-processor/src/main.rs | 20 ++++++++++--- .../src/producer/marker.rs | 13 ++++++++- 8 files changed, 106 insertions(+), 12 deletions(-) diff --git a/rust/cohort-seeder/src/app/observe.rs b/rust/cohort-seeder/src/app/observe.rs index b4f469b173e6..ca36c5f3ca50 100644 --- a/rust/cohort-seeder/src/app/observe.rs +++ b/rust/cohort-seeder/src/app/observe.rs @@ -27,7 +27,7 @@ use tracing::warn; use crate::domain::{ DispatchEpoch, LivenessCheck, MarkerLedger, MarkerWatch, ObservationEnds, PartitionBitmap, - ReconcileHwms, ReconcileScope, RunId, SeedGroupCommits, SettledVerdict, + ReconcileHwms, ReconcileScope, RunId, SeedGroupCommits, SettledVerdict, WatchPartition, }; use crate::kafka::committed::SeedGroupOffsetReader; use crate::kafka::producer::SeedTileProducer; @@ -140,9 +140,24 @@ pub async fn observe_run( } Some(ends) => match ends.caught_up(&target.watch.positions) { None => { + let uncovered: Vec = ends + .uncovered(&target.watch.positions) + .into_iter() + .map(WatchPartition::get) + .collect(); + if !uncovered.is_empty() { + warn!( + run_id = ?target.run_id, + topic = %target.watch.topic, + partitions = ?uncovered, + "the marker topic gained partitions after this run was dispatched; the \ + watcher is assigned only the partitions captured then, so this run holds \ + until it is re-dispatched" + ); + } return Ok(ObserveStep::MarkerLagging( ends.behind(&target.watch.positions), - )) + )); } Some(proof) => proof, }, @@ -200,10 +215,18 @@ async fn apply_verdict( }) } SettledVerdict::NoMarkers => { + // `topic_idle` separates the two causes: nothing at all reached the watched topic (the + // processor's gate is off, or its markers are going somewhere else) versus markers that + // reached it but named no cohort of this run. + let topic_idle = target.watch.ends.as_ref().is_some_and(|ends| { + *ends == ObservationEnds::from_positions(&target.watch.positions) + }); warn!( run_id = ?target.run_id, team_id = target.team_id.0, - "zero reconcile markers observed for a settled run — the processor reconcile gate is likely off fleet-wide" + topic = %target.watch.topic, + topic_idle, + "zero reconcile markers observed for a settled run: the processor reconcile gate is off fleet-wide, or its markers are landing on a different topic than the one watched" ); let incomplete: Vec<(CohortId, PartitionBitmap)> = active.iter().map(|p| (p.cohort_id, p.bits)).collect(); diff --git a/rust/cohort-seeder/src/bin/reconcile_dispatch.rs b/rust/cohort-seeder/src/bin/reconcile_dispatch.rs index 5faaba856aad..bfadef0c6791 100644 --- a/rust/cohort-seeder/src/bin/reconcile_dispatch.rs +++ b/rust/cohort-seeder/src/bin/reconcile_dispatch.rs @@ -127,6 +127,10 @@ async fn async_main(args: Args, config: Config) -> Result<()> { PreparedDispatch::Certified(certified) => { let run_id = certified.prepared().run_id(); let status = certified.prepared().run_status(); + // Probe before the CAS: `dispatch_and_record`'s first act is to capture this topic's + // watermarks, and it owns the claim by then, so a topic that is missing or unreachable + // would leave the run parked in `reconciling` with no dispatch record. + verify_marker_topic(&producer, &config.cohort_reconcile_markers_topic).await?; let claim = acquire_claim(&pool, run_id, certified.prepared().run_kind(), status).await?; // `plan_chunks` gates its INSERT on a non-locking `status = 'seeding'` read, so a @@ -201,6 +205,20 @@ async fn async_main(args: Args, config: Config) -> Result<()> { Ok(()) } +/// Prove the marker topic exists and reports watermarks, the same preflight the daemon runs before +/// arming either completion half. +async fn verify_marker_topic(producer: &SeedTileProducer, topic: &str) -> Result<()> { + let producer = producer.clone(); + let owned = topic.to_string(); + tokio::task::spawn_blocking(move || { + producer.capture_topic_offsets(&owned, PARTITION_VERIFY_TIMEOUT) + }) + .await + .context("joining the marker topic verification task")? + .with_context(|| format!("verifying the marker topic {topic:?} is reachable"))?; + Ok(()) +} + /// Claim the run for a persisted dispatch: CAS a seeding run into `reconciling`, or re-confirm one /// already `reconciling`. A lost claim means the run changed state under the operator — a clear, /// re-runnable error rather than a silent no-op. diff --git a/rust/cohort-seeder/src/config.rs b/rust/cohort-seeder/src/config.rs index 1b415e414010..aa1635aa6715 100644 --- a/rust/cohort-seeder/src/config.rs +++ b/rust/cohort-seeder/src/config.rs @@ -270,7 +270,9 @@ pub struct Config { pub seeder_reconcile_max_concurrent_dispatches: usize, /// The reconcile-marker topic whose high watermarks anchor the marker watcher's start positions, - /// captured at dispatch time. The observer reads markers from the same topic. + /// captured at dispatch time. The observer reads markers from the same topic. Its partition count + /// must not change while runs are in flight: a run's watch covers the partitions that existed at + /// its dispatch, so one added later is never read and holds the run open until a re-dispatch. #[envconfig(default = "cohort_reconcile_markers")] pub cohort_reconcile_markers_topic: String, diff --git a/rust/cohort-seeder/src/domain/completion.rs b/rust/cohort-seeder/src/domain/completion.rs index b752fa5a6de9..b474d4a1c6a4 100644 --- a/rust/cohort-seeder/src/domain/completion.rs +++ b/rust/cohort-seeder/src/domain/completion.rs @@ -383,6 +383,19 @@ impl ObservationEnds { .count() } + /// Captured-end partitions this dispatch never recorded a start position for. The watcher is + /// assigned from those start positions alone, so it can never read these, and the run holds at + /// [`Self::caught_up`] until a re-dispatch recaptures the full partition set. Non-empty only + /// when the marker topic gained partitions between the dispatch and the liveness pass, which is + /// worth separating out because that hold is permanent and an ordinary lag is not. + pub fn uncovered(&self, positions: &WatchPositions) -> Vec { + self.0 + .keys() + .copied() + .filter(|partition| positions.get(*partition).is_none()) + .collect() + } + /// A [`SettleProof`] is minted only when the watcher has read to or past every captured end. A /// partition with no recorded position has not been read at all, so it is never caught up, and /// an empty end set proves nothing at all rather than everything vacuously. @@ -831,8 +844,14 @@ mod tests { short.insert(WatchPartition::new(0), NextOffset::from_high_watermark(9)); assert!(ends.caught_up(&short).is_none()); - // A partition with no recorded position is never caught up. + // A partition with no recorded position is never caught up, and is reported as uncovered so + // a permanent hold (the topic gained partitions) is distinguishable from a lagging watcher. assert!(ends.caught_up(&WatchPositions::new()).is_none()); + assert_eq!( + ends.uncovered(&WatchPositions::new()), + vec![WatchPartition::new(0)], + ); + assert!(ends.uncovered(&short).is_empty()); // An empty end set proves nothing rather than everything vacuously. assert!(ObservationEnds::new().caught_up(&at_end).is_none()); diff --git a/rust/cohort-seeder/src/main.rs b/rust/cohort-seeder/src/main.rs index d4d5fcdba9dd..55fde7919dbb 100644 --- a/rust/cohort-seeder/src/main.rs +++ b/rust/cohort-seeder/src/main.rs @@ -206,7 +206,14 @@ async fn build_completion( }) .await .context("joining marker topic verification task")? - .context("verifying the marker topic is reachable")?; + .with_context(|| { + format!( + "verifying the marker topic {:?} is reachable. Provision it before arming either \ + completion half, or leave SEEDER_RECONCILE_AUTO_DISPATCH_ENABLED and \ + SEEDER_RECONCILE_OBSERVER_ENABLED off to boot without it.", + config.cohort_reconcile_markers_topic, + ) + })?; let mut driver = CompletionDriver::new( pool.clone(), diff --git a/rust/cohort-seeder/tests/fixtures/cohort_backfill_0009.sql b/rust/cohort-seeder/tests/fixtures/cohort_backfill_0009.sql index beddc4ae4356..3b797ea6419b 100644 --- a/rust/cohort-seeder/tests/fixtures/cohort_backfill_0009.sql +++ b/rust/cohort-seeder/tests/fixtures/cohort_backfill_0009.sql @@ -1,4 +1,6 @@ --- Snapshot pinned to products/cohorts/backend/migrations/0009_cohort_backfill_per_kind_uniqueness.py. +-- Snapshot pinned to products/cohorts/backend/migrations/0009_cohort_backfill_per_kind_uniqueness.py, +-- the last migration that changed this DDL. 0010 alters only `marker_watch`'s help_text, which emits +-- no SQL. -- External Team/Cohort foreign keys are omitted so the contract test stays schema-local. CREATE TABLE cohort_backfill_runs ( diff --git a/rust/cohort-stream-processor/src/main.rs b/rust/cohort-stream-processor/src/main.rs index 42c71e4e9805..18000cdc102f 100644 --- a/rust/cohort-stream-processor/src/main.rs +++ b/rust/cohort-stream-processor/src/main.rs @@ -183,9 +183,10 @@ async fn async_main(config: Config) -> Result<()> { // Only the transfer sink gets the shorter `message.timeout.ms`: its produce runs inline on a // partition worker under a bounded retry loop, so a long per-attempt timeout would multiply into - // a worker hold past the 30 s graceful-shutdown window. The membership and re-key sinks keep the - // shared 20 s — membership drops on fail (at-most-once) and the re-key produce rides the - // events-path offset gate (held-then-redelivered), so neither blocks a worker for the full timeout. + // a worker hold past the 30 s graceful-shutdown window. The membership, re-key, and marker sinks + // keep the shared 20 s: membership drops on fail (at-most-once), the re-key produce rides the + // events-path offset gate (held-then-redelivered), and a marker produce is a single un-retried + // attempt that yields the job on failure, so none of them holds a worker for more than one timeout. let transfer_kafka_config = config.build_transfer_kafka_config(); let transfer_sink: Arc = Arc::new( KafkaTransferSink::new( @@ -402,8 +403,19 @@ async fn async_main(config: Config) -> Result<()> { ); // Nothing co-partitions with the marker topic, so only its existence matters. Failing here // beats the alternative: a marker produce retrying forever while it holds the seed offset. + // Nested under the seed consumer on purpose: reconcile jobs are admitted only from seed + // tiles, so without it no marker can be produced and there is nothing to prove. if config.cohort_seed_reconcile_enabled { - fetch_partition_count(seed_consumer, &config.cohort_reconcile_markers_topic)?; + fetch_partition_count(seed_consumer, &config.cohort_reconcile_markers_topic).with_context( + || { + format!( + "{} must exist before a processor with COHORT_SEED_RECONCILE_ENABLED starts. \ + Provision the topic, or set COHORT_SEED_RECONCILE_ENABLED=false to start \ + without the reconcile path.", + config.cohort_reconcile_markers_topic, + ) + }, + )?; } } diff --git a/rust/cohort-stream-processor/src/producer/marker.rs b/rust/cohort-stream-processor/src/producer/marker.rs index 4b12a510ac30..459349476022 100644 --- a/rust/cohort-stream-processor/src/producer/marker.rs +++ b/rust/cohort-stream-processor/src/producer/marker.rs @@ -3,6 +3,14 @@ //! A dedicated producer rather than a second topic on [`crate::producer::KafkaMembershipSink`]: the //! membership output has to move to its own cluster, while markers stay alongside the seed topic on //! ingestion, and one shared producer would force them to move together. +//! +//! Two topic properties the seeder's watcher depends on, recorded here because the topic is +//! provisioned in another repo and either would fail silently: +//! +//! - `cleanup.policy=delete`. Every partition marker of a run shares one key (see +//! [`reconcile_complete_key`]), so compaction would retain one of the 64 and erase the rest. +//! - A stable partition count. The watcher captures a start offset per partition at dispatch and is +//! assigned exactly those, so a partition added mid-run is never read and holds the run open. use anyhow::{Context, Result}; use async_trait::async_trait; @@ -56,7 +64,10 @@ impl ReconcileMarkerSink for KafkaReconcileMarkerSink { } /// No-op marker sink for when the reconcile gate is off: satisfies the [`ReconcileMarkerSink`] slot -/// without a Kafka producer. The reconcile gate prevents produces from reaching this. +/// without a Kafka producer. Nothing can produce through it, because a reconcile job is only ever +/// admitted from a seed tile and `ReconcileDeps::enabled` gates that admission. Pairing this sink +/// with `enabled: true` would break that: it acks every marker without producing one, so the jobs +/// would complete and commit their seed offsets while the seeder saw no certificate at all. #[derive(Debug, Default, Clone, Copy)] pub struct NoopReconcileMarkerSink; From 648f3a135e4aa9f5f24163b348c5bb081cd5ff3d Mon Sep 17 00:00:00 2001 From: Matheus Veras Batista Date: Mon, 3 Aug 2026 19:03:40 -0300 Subject: [PATCH 3/4] address review feedback --- rust/cohort-seeder/src/app/completion.rs | 3 +- rust/cohort-seeder/src/app/observe.rs | 50 +++++++++++-------- rust/cohort-seeder/src/domain/completion.rs | 13 +++++ rust/cohort-seeder/src/kafka/markers.rs | 13 ++--- .../src/observability/metrics.rs | 3 ++ rust/cohort-seeder/tests/pg_completion.rs | 35 +++++++++++-- rust/cohort-stream-processor/src/config.rs | 27 +++++++++- rust/cohort-stream-processor/src/main.rs | 14 +++--- .../src/producer/marker.rs | 45 ++++++++++------- 9 files changed, 144 insertions(+), 59 deletions(-) diff --git a/rust/cohort-seeder/src/app/completion.rs b/rust/cohort-seeder/src/app/completion.rs index 215490e6c2ea..d844e863d2d4 100644 --- a/rust/cohort-seeder/src/app/completion.rs +++ b/rust/cohort-seeder/src/app/completion.rs @@ -357,7 +357,8 @@ impl CompletionDriver { // read as stalled the instant the dispatch record breaks. track_oldest(&mut oldest_stalled, since); if first_sighting { - counter!(RECONCILE_RECORD_INVALID).increment(1); + counter!(RECONCILE_RECORD_INVALID, "reason" => reason.as_str()) + .increment(1); } // Only the dispatch arm can heal an undispatched run; observe-only leaves it for a // CLI re-dispatch. diff --git a/rust/cohort-seeder/src/app/observe.rs b/rust/cohort-seeder/src/app/observe.rs index ca36c5f3ca50..60ea309e0761 100644 --- a/rust/cohort-seeder/src/app/observe.rs +++ b/rust/cohort-seeder/src/app/observe.rs @@ -131,6 +131,10 @@ pub async fn observe_run( .observation_ends() .await .map_err(ObserveError::TopicEnds)?; + // Warn at capture rather than on the hold it causes: the dispatch's partition set + // is fixed, so the answer never changes, and this arm runs once per dispatch + // while the hold below is re-entered on every tick until a re-dispatch. + warn_on_uncovered_partitions(target, &ends); store .persist_ends(target.run_id, target.epoch, &ends) .await?; @@ -140,21 +144,6 @@ pub async fn observe_run( } Some(ends) => match ends.caught_up(&target.watch.positions) { None => { - let uncovered: Vec = ends - .uncovered(&target.watch.positions) - .into_iter() - .map(WatchPartition::get) - .collect(); - if !uncovered.is_empty() { - warn!( - run_id = ?target.run_id, - topic = %target.watch.topic, - partitions = ?uncovered, - "the marker topic gained partitions after this run was dispatched; the \ - watcher is assigned only the partitions captured then, so this run holds \ - until it is re-dispatched" - ); - } return Ok(ObserveStep::MarkerLagging( ends.behind(&target.watch.positions), )); @@ -174,6 +163,27 @@ pub async fn observe_run( Ok(ObserveStep::Settled(summary)) } +/// A partition present in the freshly captured ends but absent from the dispatch's start positions is +/// one the watcher was never assigned, so it can never be read and the run holds at +/// [`ObserveStep::MarkerLagging`] until a re-dispatch recaptures the full set. +fn warn_on_uncovered_partitions(target: &ObserveTarget, ends: &ObservationEnds) { + let uncovered: Vec = ends + .uncovered(&target.watch.positions) + .into_iter() + .map(WatchPartition::get) + .collect(); + if uncovered.is_empty() { + return; + } + warn!( + run_id = ?target.run_id, + topic = %target.watch.topic, + partitions = ?uncovered, + "the marker topic gained partitions after this run was dispatched; the watcher is assigned \ + only the partitions captured then, so this run holds until it is re-dispatched" + ); +} + async fn apply_verdict( store: &dyn ObservationStore, target: &ObserveTarget, @@ -215,17 +225,17 @@ async fn apply_verdict( }) } SettledVerdict::NoMarkers => { - // `topic_idle` separates the two causes: nothing at all reached the watched topic (the - // processor's gate is off, or its markers are going somewhere else) versus markers that - // reached it but named no cohort of this run. - let topic_idle = target.watch.ends.as_ref().is_some_and(|ends| { + // One-sided. False proves records landed after the ends were captured, so the topic is + // live and the gate-off reading is wrong. True proves nothing about whether markers ever + // landed: the positions and the captured ends coincide on any topic that has gone quiet. + let no_reads_past_ends = target.watch.ends.as_ref().is_some_and(|ends| { *ends == ObservationEnds::from_positions(&target.watch.positions) }); warn!( run_id = ?target.run_id, team_id = target.team_id.0, topic = %target.watch.topic, - topic_idle, + no_reads_past_ends, "zero reconcile markers observed for a settled run: the processor reconcile gate is off fleet-wide, or its markers are landing on a different topic than the one watched" ); let incomplete: Vec<(CohortId, PartitionBitmap)> = diff --git a/rust/cohort-seeder/src/domain/completion.rs b/rust/cohort-seeder/src/domain/completion.rs index b474d4a1c6a4..e909b69f03d4 100644 --- a/rust/cohort-seeder/src/domain/completion.rs +++ b/rust/cohort-seeder/src/domain/completion.rs @@ -543,6 +543,19 @@ pub enum UndispatchedReason { TopicChanged, } +impl UndispatchedReason { + /// Metric label. A topic rename re-dispatches every in-flight run at once, which is expected; + /// without the label that spike is indistinguishable from a genuinely corrupt dispatch record. + pub fn as_str(self) -> &'static str { + match self { + Self::NeverDispatched => "never_dispatched", + Self::MissingRecord => "missing_record", + Self::UnparseableRecord => "unparseable_record", + Self::TopicChanged => "topic_changed", + } + } +} + /// The classification of one discovered completion row. Parse-don't-validate: the app matches this /// once and never re-inspects the raw columns. #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/rust/cohort-seeder/src/kafka/markers.rs b/rust/cohort-seeder/src/kafka/markers.rs index fa31e3964355..7a5691664598 100644 --- a/rust/cohort-seeder/src/kafka/markers.rs +++ b/rust/cohort-seeder/src/kafka/markers.rs @@ -4,10 +4,11 @@ //! it participates in no rebalance and leaves no durable offset. It watches every marker partition //! from explicit start offsets (the high watermarks captured at dispatch), with `auto.offset.reset= //! error` so a start below the log's low watermark surfaces as [`WatchError::Truncated`] instead of -//! silently jumping. Every marker is keyed `"{team}:{cohort}:{run}"`; on a dedicated topic the `b':'` -//! probe is no longer a hot-path filter but a guard against a mis-pointed topic, so anything it (or the -//! parse behind it) rejects is counted by `reason`. Skipped messages still advance the partition's -//! next-read offset. rdkafka types stay confined here — the watcher yields typed [`WatchItem`]s. +//! silently jumping. Every marker is keyed `"{team}:{cohort}:{run}:{partition}"`; on a dedicated +//! topic the `b':'` probe is no longer a hot-path filter but a guard against a mis-pointed topic, so +//! anything it (or the parse behind it) rejects is counted by `reason`. Skipped messages still +//! advance the partition's next-read offset. rdkafka types stay confined here — the watcher yields +//! typed [`WatchItem`]s. use std::collections::HashMap; use std::sync::Arc; @@ -180,7 +181,7 @@ fn classify_marker(key: Option<&[u8]>, payload: Option<&[u8]>) -> Option, payload: Option<&[u8]>) -> Option skip("partition"), + Err(_) => skip("partition_out_of_range"), } } diff --git a/rust/cohort-seeder/src/observability/metrics.rs b/rust/cohort-seeder/src/observability/metrics.rs index 4c8794bac2f2..98b4a79b2633 100644 --- a/rust/cohort-seeder/src/observability/metrics.rs +++ b/rust/cohort-seeder/src/observability/metrics.rs @@ -44,6 +44,9 @@ pub const RUNS_PLANNING_WITHHELD: &str = "seeder_runs_planning_withheld_total"; pub const RECONCILE_DISPATCHES: &str = "seeder_reconcile_dispatches_total"; /// Dispatch claims lost to a concurrent writer, labelled by the run's `kind` (counter). pub const RECONCILE_CAS_LOST: &str = "seeder_reconcile_cas_lost_total"; +/// Reconciling runs with no usable dispatch record, counted once per run per stretch and labelled by +/// `reason` (counter). A topic rename re-dispatches every in-flight run at once; the label keeps that +/// expected spike apart from a corrupt record. pub const RECONCILE_RECORD_INVALID: &str = "seeder_reconcile_record_invalid_total"; pub const RECONCILE_DISPATCHES_IN_FLIGHT: &str = "seeder_reconcile_dispatches_in_flight"; /// Runs currently in the reconcile protocol, labelled by the run's `kind` (gauge). Published for diff --git a/rust/cohort-seeder/tests/pg_completion.rs b/rust/cohort-seeder/tests/pg_completion.rs index 6bcbc825560a..af5c06294379 100644 --- a/rust/cohort-seeder/tests/pg_completion.rs +++ b/rust/cohort-seeder/tests/pg_completion.rs @@ -825,9 +825,9 @@ async fn marker_observations_or_merge_round_trip_including_bit_63() -> Result<() .await } -/// Discovery classifies each completion phase, including a reconciling run whose completion -/// columns are all NULL — and drops the observed run, whose remaining work is Django's, while the -/// reconciling-run count still reports it. +/// Discovery classifies each completion phase, including a reconciling run whose completion columns +/// are all NULL and one whose persisted watch names a different marker topic — and drops the observed +/// run, whose remaining work is Django's, while the reconciling-run count still reports it. #[tokio::test] async fn discovery_classifies_each_phase_and_drops_observed_runs() -> Result<()> { with_db(|pool| async move { @@ -864,6 +864,25 @@ async fn discovery_classifies_each_phase_and_drops_observed_runs() -> Result<()> let epoch = claim.record(&pool, &full_hwms(), &empty_watch()).await?; mark_run_observed(&pool, observed, epoch).await?; + // A watch recorded against the topic markers used to ride. Its offsets name a different log, + // so resuming on them would settle the run against positions it never read. + let moved = insert_reconciling_run(&pool, 7).await?; + insert_participation(&pool, moved, 7, 701, false, empty_pinned()).await?; + let claim = confirm_reconciling(&pool, moved, RunKind::Behavioral) + .await? + .context("moved run should be claimable")?; + let _ = claim + .record( + &pool, + &full_hwms(), + &MarkerWatch { + topic: "cohort_membership_changed_shadow".to_string(), + positions: WatchPositions::new(), + ends: None, + }, + ) + .await?; + let discovered = discover_completions(&pool, &TeamAllowlist::All, BOTH_KINDS, MARKER_TOPIC).await?; let phase = |run| { @@ -884,13 +903,19 @@ async fn discovery_classifies_each_phase_and_drops_observed_runs() -> Result<()> UndispatchedReason::NeverDispatched )) ); + ensure!( + phase(moved) + == Some(CompletionPhase::ReconcilingUndispatched( + UndispatchedReason::TopicChanged + )) + ); // Dropped from discovery rather than hydrated and matched away: a run parked here behind // Django's readiness gate would otherwise cost two per-partition JSONB decodes every tick. ensure!(phase(observed).is_none()); - // The three reconciling runs, observed or not, all still reach the gauge. + // Every reconciling run, observed or not, still reaches the gauge. let counts = count_reconciling_by_kind(&pool, &TeamAllowlist::All, BOTH_KINDS).await?; - ensure!(counts.get(&RunKind::Behavioral) == Some(&3)); + ensure!(counts.get(&RunKind::Behavioral) == Some(&4)); ensure!(!counts.contains_key(&RunKind::PersonProperty)); // Both statements have a team-scoped variant, and dev — where this rollout lands first — diff --git a/rust/cohort-stream-processor/src/config.rs b/rust/cohort-stream-processor/src/config.rs index 7f4d629b004b..0f3ebd095501 100644 --- a/rust/cohort-stream-processor/src/config.rs +++ b/rust/cohort-stream-processor/src/config.rs @@ -251,8 +251,8 @@ pub struct Config { #[envconfig(from = "COHORT_REGISTER_TRANSFER_ENABLED", default = "false")] pub cohort_register_transfer_enabled: bool, - /// Admit and drain reconcile controls. Default off; enable only once every downstream consumer - /// tolerates completion markers. Register transfer is gated separately by + /// Admit and drain reconcile controls. Default off; enable only once `COHORT_RECONCILE_MARKERS_TOPIC` + /// exists in the environment, which startup asserts. Register transfer is gated separately by /// `COHORT_REGISTER_TRANSFER_ENABLED`. #[envconfig(from = "COHORT_SEED_RECONCILE_ENABLED", default = "false")] pub cohort_seed_reconcile_enabled: bool, @@ -315,6 +315,13 @@ pub struct Config { #[envconfig(default = "cohort_reconcile_markers")] pub cohort_reconcile_markers_topic: String, + /// `message.timeout.ms` for the marker producer alone — much shorter than the shared 20 s. The + /// marker produce runs inline on a partition worker, so a broker that black-holes it stalls live + /// evaluation on that partition for the whole timeout, and the drain sweeper re-queues the job a + /// tick later and stalls it again. A fast fail just yields the job for the next tick. + #[envconfig(default = "2000")] + pub reconcile_marker_message_timeout_ms: u32, + /// `murmur2_random` co-partitions a `person_id` key identically to the Node/Python producers. #[envconfig(default = "murmur2_random")] pub kafka_producer_partitioner: String, @@ -1034,6 +1041,17 @@ impl Config { ..self.build_kafka_config() } } + + /// Producer config for the reconcile-marker sink: the shared config with a shorter + /// `message.timeout.ms`. Same inline-on-a-worker reason as the transfer sink, except the retry + /// is the drain sweeper's rather than an inline loop, so an unreachable marker topic would + /// otherwise hold the worker for the full timeout on every tick. + pub fn build_marker_kafka_config(&self) -> KafkaConfig { + KafkaConfig { + kafka_message_timeout_ms: self.reconcile_marker_message_timeout_ms, + ..self.build_kafka_config() + } + } } #[cfg(test)] @@ -1089,6 +1107,7 @@ mod tests { pod_hostname: None, cohort_membership_changed_topic: "cohort_membership_changed_shadow".to_string(), cohort_reconcile_markers_topic: "cohort_reconcile_markers".to_string(), + reconcile_marker_message_timeout_ms: 2000, kafka_producer_partitioner: "murmur2_random".to_string(), cohort_partition_count: 64, kafka_compression_codec: "none".to_string(), @@ -2074,6 +2093,10 @@ mod tests { let transfer = config.build_transfer_kafka_config(); // Only `message.timeout.ms` differs; every other producer knob is inherited. assert_eq!(transfer.kafka_message_timeout_ms, 2000); + assert_eq!( + config.build_marker_kafka_config().kafka_message_timeout_ms, + 2000, + ); assert_eq!(shared.kafka_message_timeout_ms, 20_000); assert_eq!( transfer.kafka_producer_partitioner, diff --git a/rust/cohort-stream-processor/src/main.rs b/rust/cohort-stream-processor/src/main.rs index 18000cdc102f..bfc493354f86 100644 --- a/rust/cohort-stream-processor/src/main.rs +++ b/rust/cohort-stream-processor/src/main.rs @@ -181,12 +181,12 @@ async fn async_main(config: Config) -> Result<()> { .context("creating shadow producer")?, ); - // Only the transfer sink gets the shorter `message.timeout.ms`: its produce runs inline on a - // partition worker under a bounded retry loop, so a long per-attempt timeout would multiply into - // a worker hold past the 30 s graceful-shutdown window. The membership, re-key, and marker sinks - // keep the shared 20 s: membership drops on fail (at-most-once), the re-key produce rides the - // events-path offset gate (held-then-redelivered), and a marker produce is a single un-retried - // attempt that yields the job on failure, so none of them holds a worker for more than one timeout. + // The transfer and marker sinks get the shorter `message.timeout.ms`: both produce inline on a + // partition worker and both are retried on a cadence — the transfer by a bounded inline loop, the + // marker by the drain sweeper — so a long per-attempt timeout repeats into a worker hold rather + // than costing it once. The membership and re-key sinks keep the shared 20 s: membership drops on + // fail (at-most-once) and the re-key produce rides the events-path offset gate + // (held-then-redelivered), so neither retries a black-holed produce on a timer. let transfer_kafka_config = config.build_transfer_kafka_config(); let transfer_sink: Arc = Arc::new( KafkaTransferSink::new( @@ -226,7 +226,7 @@ async fn async_main(config: Config) -> Result<()> { let marker_sink: Arc = if config.cohort_seed_reconcile_enabled { Arc::new( KafkaReconcileMarkerSink::new( - &kafka_config, + &config.build_marker_kafka_config(), config.cohort_reconcile_markers_topic.clone(), ) .await diff --git a/rust/cohort-stream-processor/src/producer/marker.rs b/rust/cohort-stream-processor/src/producer/marker.rs index 459349476022..c95189e0cf44 100644 --- a/rust/cohort-stream-processor/src/producer/marker.rs +++ b/rust/cohort-stream-processor/src/producer/marker.rs @@ -4,13 +4,15 @@ //! membership output has to move to its own cluster, while markers stay alongside the seed topic on //! ingestion, and one shared producer would force them to move together. //! -//! Two topic properties the seeder's watcher depends on, recorded here because the topic is -//! provisioned in another repo and either would fail silently: +//! One topic property the seeder's watcher depends on, recorded here because the topic is +//! provisioned in another repo and would fail silently: a stable partition count. The watcher +//! captures a start offset per partition at dispatch and is assigned exactly those, so a partition +//! added mid-run is never read and holds the run open. //! -//! - `cleanup.policy=delete`. Every partition marker of a run shares one key (see -//! [`reconcile_complete_key`]), so compaction would retain one of the 64 and erase the rest. -//! - A stable partition count. The watcher captures a start offset per partition at dispatch and is -//! assigned exactly those, so a partition added mid-run is never read and holds the run open. +//! The key ([`reconcile_complete_key`]) carries the body partition so every marker of a run is a +//! distinct key. The topic is provisioned `cleanup.policy=delete`, but a compacting one would keep +//! all 64 rather than collapsing them to whichever arrived last — a mis-provision that would +//! otherwise record completed backfills as short. use anyhow::{Context, Result}; use async_trait::async_trait; @@ -63,11 +65,11 @@ impl ReconcileMarkerSink for KafkaReconcileMarkerSink { } } -/// No-op marker sink for when the reconcile gate is off: satisfies the [`ReconcileMarkerSink`] slot -/// without a Kafka producer. Nothing can produce through it, because a reconcile job is only ever -/// admitted from a seed tile and `ReconcileDeps::enabled` gates that admission. Pairing this sink -/// with `enabled: true` would break that: it acks every marker without producing one, so the jobs -/// would complete and commit their seed offsets while the seeder saw no certificate at all. +/// Inert sink for when the reconcile gate is off: satisfies the [`ReconcileMarkerSink`] slot without +/// a Kafka producer. Nothing should reach it — a reconcile job is only ever admitted from a seed tile +/// and `ReconcileDeps::enabled` gates that admission — so a produce here is a coding error, and it +/// fails rather than acking. Acking would be the worse half of the trade: the job would complete and +/// commit its seed offset while the seeder waited for a certificate that was never produced. #[derive(Debug, Default, Clone, Copy)] pub struct NoopReconcileMarkerSink; @@ -77,7 +79,10 @@ impl ReconcileMarkerSink for NoopReconcileMarkerSink { &self, markers: Vec, ) -> Vec> { - markers.iter().map(|_| Ok(())).collect() + markers + .into_iter() + .map(|_| Err(KafkaProduceError::KafkaProduceCanceled)) + .collect() } } @@ -108,12 +113,16 @@ impl ReconcileMarkerSink for CaptureReconcileMarkerSink { } } +/// Unique per marker, not per run: the partition is what makes 64 distinct keys out of one dispatch, +/// so no cleanup policy can collapse them. Nothing downstream reads the key beyond the watcher's +/// `':'` probe, and the ledger's fold is order-independent, so the grouping carries no meaning. fn reconcile_complete_key(marker: &ReconcileCompleteMarker) -> Option { Some(format!( - "{}:{}:{}", + "{}:{}:{}:{}", marker.team_id().0, marker.cohort_id().0, - marker.run_id().0 + marker.run_id().0, + marker.partition(), )) } @@ -137,15 +146,15 @@ mod tests { } #[test] - fn reconcile_complete_key_identifies_the_run_without_the_partition() { + fn reconcile_complete_key_is_unique_per_partition_marker() { assert_eq!( reconcile_complete_key(&marker(63)), - Some("42:91204:00000000-0000-0000-0000-000000000000".to_string()) + Some("42:91204:00000000-0000-0000-0000-000000000000:63".to_string()) ); - assert_eq!( + assert_ne!( reconcile_complete_key(&marker(63)), reconcile_complete_key(&marker(0)), - "all partition markers for one run share the same Kafka key", + "a shared key would let a compacting topic erase all but one marker of a run", ); } From fd0b7d62270286e2a4337a442211830f8965d2b1 Mon Sep 17 00:00:00 2001 From: Matheus Veras Batista Date: Tue, 4 Aug 2026 14:10:23 -0300 Subject: [PATCH 4/4] address feedback --- .../commands/compare_cohort_membership.py | 37 +++++++--- products/cohorts/backend/parity/fold.py | 2 +- products/cohorts/backend/parity/kafka_io.py | 10 +-- rust/cohort-seeder/src/app/completion.rs | 50 +++++++++++++ rust/cohort-seeder/src/app/observe.rs | 5 +- .../src/bin/reconcile_dispatch.rs | 16 +---- rust/cohort-seeder/src/config.rs | 3 +- rust/cohort-seeder/src/domain/completion.rs | 2 +- rust/cohort-seeder/src/kafka/markers.rs | 40 +++++------ rust/cohort-seeder/src/kafka/producer.rs | 72 +++++++++++++------ rust/cohort-seeder/src/main.rs | 17 +---- rust/cohort-seeder/tests/pg_completion.rs | 3 +- rust/cohort-stream-processor/src/config.rs | 29 ++++++-- .../src/producer/marker.rs | 15 ++-- .../src/producer/mod.rs | 40 ++--------- .../src/workers/reconcile.rs | 4 +- .../tests/merge_durability.rs | 9 +-- 17 files changed, 204 insertions(+), 150 deletions(-) diff --git a/products/cohorts/backend/management/commands/compare_cohort_membership.py b/products/cohorts/backend/management/commands/compare_cohort_membership.py index e10cdafce4f9..59c564dd47f1 100644 --- a/products/cohorts/backend/management/commands/compare_cohort_membership.py +++ b/products/cohorts/backend/management/commands/compare_cohort_membership.py @@ -35,6 +35,7 @@ from collections.abc import Mapping from dataclasses import dataclass from datetime import UTC, datetime, timedelta +from itertools import chain from typing import Any, Optional from uuid import UUID @@ -47,7 +48,13 @@ from products.cohorts.backend.parity.classifier import ClassifierConfig, CohortComparison, classify_cohort, summarize from products.cohorts.backend.parity.eligibility import EMITTING_CLASSES, ScreenedCohort, screen_team from products.cohorts.backend.parity.fold import fold_membership_changes, members, reconcile_completeness_by_cohort -from products.cohorts.backend.parity.kafka_io import DEFAULT_SHADOW_TOPIC, DrainStats, consumer_config, drain_topic +from products.cohorts.backend.parity.kafka_io import ( + DEFAULT_MARKER_TOPIC, + DEFAULT_SHADOW_TOPIC, + DrainStats, + consumer_config, + drain_topic, +) from products.cohorts.backend.parity.oracle import ( OracleSetTooLarge, load_day_counts, @@ -390,7 +397,10 @@ def add_arguments(self, parser: CommandParser) -> None: help="old-pipeline only: O-bounded raw diff, no R-FRESH/R-STALE rules or suspect probe", ) parser.add_argument("--shadow-topic", type=str, default=DEFAULT_SHADOW_TOPIC) - parser.add_argument("--new-kafka-hosts", type=str, default=None, help="Override shadow-topic bootstrap servers") + parser.add_argument("--marker-topic", type=str, default=DEFAULT_MARKER_TOPIC) + parser.add_argument( + "--new-kafka-hosts", type=str, default=None, help="Override bootstrap servers for both drained topics" + ) parser.add_argument("--security-protocol", type=str, default=None, help="Override Kafka security protocol") parser.add_argument("--format", choices=["table", "json"], default="table") parser.add_argument("--max-messages", type=int, default=None, help="Cap on shadow messages drained") @@ -479,14 +489,23 @@ def log(message: str) -> None: hosts_override=options["new_kafka_hosts"], security_protocol_override=options["security_protocol"], ) - log(f"draining {options['shadow_topic']} from {since.isoformat()} via {config['bootstrap.servers']}") + log( + f"draining {options['shadow_topic']} and {options['marker_topic']} from {since.isoformat()} " + f"via {config['bootstrap.servers']}" + ) drain_stats = DrainStats() - messages = drain_topic( - options["shadow_topic"], - config=config, - since=since, - stats=drain_stats, - max_messages=options["max_messages"], + # Reconcile markers ride their own topic, so completeness needs a second drain chained into + # the same fold. Without it every cohort reads as having no complete reconcile run, and the + # recompute and population oracles caveat their own numbers as unmeasurable. + messages = chain( + drain_topic( + options["shadow_topic"], + config=config, + since=since, + stats=drain_stats, + max_messages=options["max_messages"], + ), + drain_topic(options["marker_topic"], config=config, since=since, stats=DrainStats()), ) # An explicit --at pins the comparison clock, so the fold has to converge to that instant too. new_state, fold_stats = fold_membership_changes( diff --git a/products/cohorts/backend/parity/fold.py b/products/cohorts/backend/parity/fold.py index 8304cda3ebe7..3b6d2aa62da4 100644 --- a/products/cohorts/backend/parity/fold.py +++ b/products/cohorts/backend/parity/fold.py @@ -1,4 +1,4 @@ -"""Fold a shadow-topic message stream into converged per-cohort membership state. +"""Fold a drained message stream into converged per-cohort membership state. Max last_updated per (cohort_id, person_id) wins, matching the argMax convergence of the old side's ClickHouse table. In practice that is also arrival order (messages are keyed diff --git a/products/cohorts/backend/parity/kafka_io.py b/products/cohorts/backend/parity/kafka_io.py index 781324a6bdf2..ac8741952359 100644 --- a/products/cohorts/backend/parity/kafka_io.py +++ b/products/cohorts/backend/parity/kafka_io.py @@ -1,4 +1,4 @@ -"""Kafka profile resolution and a bounded shadow-topic drain. +"""Kafka profile resolution and a bounded topic drain. The drain reads each partition from `offsets_for_times(--since)` up to a high-watermark snapshot taken at start, so a still-producing topic terminates. Uses a throwaway @@ -21,6 +21,8 @@ from posthog.kafka_client.routing import get_profile_settings DEFAULT_SHADOW_TOPIC = "cohort_membership_changed_shadow" +# Reconcile completion markers ride their own topic, on the same cluster as the shadow topic. +DEFAULT_MARKER_TOPIC = "cohort_reconcile_markers" @dataclass @@ -42,10 +44,10 @@ def consumer_config( hosts_override: Optional[str] = None, security_protocol_override: Optional[str] = None, ) -> dict[str, Any]: - """Consumer config for the shadow topic's cluster (Warpstream ingestion). + """Consumer config for the shadow and marker topics' cluster (Warpstream ingestion). - The shadow topic is not in the Django routing table, so the INGESTION profile is - resolved explicitly; overrides serve local runs against a dev stack. + Neither topic is in the Django routing table, so the INGESTION profile is resolved + explicitly; overrides serve local runs against a dev stack. """ profile = get_profile_settings(profile=KafkaClusterProfile.INGESTION) hosts = hosts_override or ",".join(profile.hosts) diff --git a/rust/cohort-seeder/src/app/completion.rs b/rust/cohort-seeder/src/app/completion.rs index d844e863d2d4..de45b16f7c53 100644 --- a/rust/cohort-seeder/src/app/completion.rs +++ b/rust/cohort-seeder/src/app/completion.rs @@ -68,6 +68,56 @@ use super::watch::{WatchDirective, WatchDirectives}; /// Timeout for the blocking Kafka metadata and watermark calls the dispatch makes. const KAFKA_METADATA_TIMEOUT: Duration = Duration::from_secs(10); +/// Prove the marker topic exists before anything anchors on it: the daemon before it arms either +/// completion half, the CLI before it claims a run. [`dispatch_and_record`]'s first act is to capture +/// this topic's watermarks and it owns the claim by then, so an unprovisioned topic would otherwise +/// park the run in `reconciling` with no dispatch record. Shared by both binaries so the diagnosis +/// and the guidance stay the same wherever an operator meets them. +pub async fn verify_marker_topic( + producer: &SeedTileProducer, + topic: &str, +) -> Result<(), MarkerTopicError> { + let probe = producer.clone(); + let probed = topic.to_string(); + let joined = tokio::task::spawn_blocking(move || { + probe.verify_topic_reachable(&probed, KAFKA_METADATA_TIMEOUT) + }) + .await; + match joined { + Err(source) => Err(MarkerTopicError::Join { + topic: topic.to_string(), + source, + }), + Ok(Err(source)) => Err(MarkerTopicError::Unreachable { + topic: topic.to_string(), + source, + }), + Ok(Ok(())) => Ok(()), + } +} + +/// Why the marker-topic preflight failed. The operator guidance rides the error rather than each call +/// site, so the daemon and the CLI cannot drift apart on what to do about it. +#[derive(Debug, thiserror::Error)] +pub enum MarkerTopicError { + #[error("joining the {topic:?} reachability probe")] + Join { + topic: String, + #[source] + source: JoinError, + }, + #[error( + "reconcile marker topic {topic:?} is unreachable. Provision it before dispatching a run or \ + arming either completion half, or leave SEEDER_RECONCILE_AUTO_DISPATCH_ENABLED and \ + SEEDER_RECONCILE_OBSERVER_ENABLED off to boot without it." + )] + Unreachable { + topic: String, + #[source] + source: CaptureOffsetsError, + }, +} + /// Whether automatic dispatch is armed. `Enabled` carries the register-backfill attestation, so the /// driver cannot be constructed without it. #[derive(Debug, Clone, Copy)] diff --git a/rust/cohort-seeder/src/app/observe.rs b/rust/cohort-seeder/src/app/observe.rs index 60ea309e0761..e1f19797fd3d 100644 --- a/rust/cohort-seeder/src/app/observe.rs +++ b/rust/cohort-seeder/src/app/observe.rs @@ -236,7 +236,10 @@ async fn apply_verdict( team_id = target.team_id.0, topic = %target.watch.topic, no_reads_past_ends, - "zero reconcile markers observed for a settled run: the processor reconcile gate is off fleet-wide, or its markers are landing on a different topic than the one watched" + "no reconcile marker folded for a settled run: either markers reached the watched \ + topic but named another team, run, or cohort, or none reached it at all — the \ + processor reconcile gate is off fleet-wide, or its markers land somewhere else. A \ + false `no_reads_past_ends` rules the second out; a true one rules nothing out" ); let incomplete: Vec<(CohortId, PartitionBitmap)> = active.iter().map(|p| (p.cohort_id, p.bits)).collect(); diff --git a/rust/cohort-seeder/src/bin/reconcile_dispatch.rs b/rust/cohort-seeder/src/bin/reconcile_dispatch.rs index bfadef0c6791..24f3a28c34e6 100644 --- a/rust/cohort-seeder/src/bin/reconcile_dispatch.rs +++ b/rust/cohort-seeder/src/bin/reconcile_dispatch.rs @@ -6,7 +6,7 @@ use std::time::Duration; use anyhow::{bail, Context, Result}; use clap::Parser; use cohort_core::partitioner::COHORT_PARTITION_COUNT; -use cohort_seeder::app::completion::dispatch_and_record; +use cohort_seeder::app::completion::{dispatch_and_record, verify_marker_topic}; use cohort_seeder::app::reconcile_dispatch::{ execute_reconcile_dispatch, prepare_reconcile_dispatch, CompletionRequirement, PreparedDispatch, RegisterBackfillConfirmation, @@ -205,20 +205,6 @@ async fn async_main(args: Args, config: Config) -> Result<()> { Ok(()) } -/// Prove the marker topic exists and reports watermarks, the same preflight the daemon runs before -/// arming either completion half. -async fn verify_marker_topic(producer: &SeedTileProducer, topic: &str) -> Result<()> { - let producer = producer.clone(); - let owned = topic.to_string(); - tokio::task::spawn_blocking(move || { - producer.capture_topic_offsets(&owned, PARTITION_VERIFY_TIMEOUT) - }) - .await - .context("joining the marker topic verification task")? - .with_context(|| format!("verifying the marker topic {topic:?} is reachable"))?; - Ok(()) -} - /// Claim the run for a persisted dispatch: CAS a seeding run into `reconciling`, or re-confirm one /// already `reconciling`. A lost claim means the run changed state under the operator — a clear, /// re-runnable error rather than a silent no-op. diff --git a/rust/cohort-seeder/src/config.rs b/rust/cohort-seeder/src/config.rs index aa1635aa6715..f8abb0dcf1e5 100644 --- a/rust/cohort-seeder/src/config.rs +++ b/rust/cohort-seeder/src/config.rs @@ -272,7 +272,8 @@ pub struct Config { /// The reconcile-marker topic whose high watermarks anchor the marker watcher's start positions, /// captured at dispatch time. The observer reads markers from the same topic. Its partition count /// must not change while runs are in flight: a run's watch covers the partitions that existed at - /// its dispatch, so one added later is never read and holds the run open until a re-dispatch. + /// its dispatch, so one added later is never read — holding the run open if it appeared before + /// the observation ends were captured, and settling the run short if it appeared after them. #[envconfig(default = "cohort_reconcile_markers")] pub cohort_reconcile_markers_topic: String, diff --git a/rust/cohort-seeder/src/domain/completion.rs b/rust/cohort-seeder/src/domain/completion.rs index e909b69f03d4..e07a312b723f 100644 --- a/rust/cohort-seeder/src/domain/completion.rs +++ b/rust/cohort-seeder/src/domain/completion.rs @@ -546,7 +546,7 @@ pub enum UndispatchedReason { impl UndispatchedReason { /// Metric label. A topic rename re-dispatches every in-flight run at once, which is expected; /// without the label that spike is indistinguishable from a genuinely corrupt dispatch record. - pub fn as_str(self) -> &'static str { + pub const fn as_str(self) -> &'static str { match self { Self::NeverDispatched => "never_dispatched", Self::MissingRecord => "missing_record", diff --git a/rust/cohort-seeder/src/kafka/markers.rs b/rust/cohort-seeder/src/kafka/markers.rs index 7a5691664598..bb5e190d6b1b 100644 --- a/rust/cohort-seeder/src/kafka/markers.rs +++ b/rust/cohort-seeder/src/kafka/markers.rs @@ -4,11 +4,11 @@ //! it participates in no rebalance and leaves no durable offset. It watches every marker partition //! from explicit start offsets (the high watermarks captured at dispatch), with `auto.offset.reset= //! error` so a start below the log's low watermark surfaces as [`WatchError::Truncated`] instead of -//! silently jumping. Every marker is keyed `"{team}:{cohort}:{run}:{partition}"`; on a dedicated -//! topic the `b':'` probe is no longer a hot-path filter but a guard against a mis-pointed topic, so -//! anything it (or the parse behind it) rejects is counted by `reason`. Skipped messages still -//! advance the partition's next-read offset. rdkafka types stay confined here — the watcher yields -//! typed [`WatchItem`]s. +//! silently jumping. Records are classified by payload alone: a marker refuses to deserialize unless +//! its `type` is `reconcile_complete`, so a mis-pointed topic is caught without this crate depending +//! on the key format the processor writes. Anything the parse rejects is counted by `reason`, and +//! skipped messages still advance the partition's next-read offset. rdkafka types stay confined +//! here — the watcher yields typed [`WatchItem`]s. use std::collections::HashMap; use std::sync::Arc; @@ -136,7 +136,7 @@ impl MarkerWatcher { let message = self.consumer.recv().await.map_err(WatchError::Recv)?; let partition = WatchPartition::new(message.partition()); let next_offset = NextOffset::from_high_watermark(message.offset() + 1); - let marker = classify_marker(message.key(), message.payload()); + let marker = classify_marker(message.payload()); Ok(WatchItem { partition, next_offset, @@ -170,19 +170,14 @@ fn build_assignment( Ok(tpl) } -/// Key probe then parse. On a dedicated topic every record should be a marker, so each rejection is -/// counted under the `reason` that explains it rather than silently skipped. -fn classify_marker(key: Option<&[u8]>, payload: Option<&[u8]>) -> Option { +/// On a dedicated topic every record should be a marker, so each rejection is counted under the +/// `reason` that explains it rather than silently skipped. A record that came from a mis-pointed +/// topic fails the payload's `reconcile_complete` discriminant and lands under `decode`. +fn classify_marker(payload: Option<&[u8]>) -> Option { let skip = |reason: &'static str| { counter!(RECONCILE_MARKER_PARSE_FAILURES, "reason" => reason).increment(1); None }; - let Some(key) = key else { - return skip("no_key"); - }; - if !key.contains(&b':') { - return skip("key_not_a_marker"); - } let Some(payload) = payload else { return skip("no_payload"); }; @@ -267,21 +262,18 @@ mod tests { } #[test] - fn classify_probes_the_key_before_parsing_and_extracts_the_marker() { - // A foreign, person-keyed row: no ':' in the key, so no parse attempt, no marker. - assert!(classify_marker(Some(b"01928aaa-bbbb-cccc"), Some(b"{}")).is_none()); - - // A marker key with a valid payload. - let observed = classify_marker(Some(b"2:42:run"), Some(&marker_payload(7))).unwrap(); + fn classify_extracts_a_marker_and_rejects_every_other_payload() { + let observed = classify_marker(Some(&marker_payload(7))).unwrap(); assert_eq!(observed.team_id, TeamId(2)); assert_eq!(observed.cohort_id, CohortId(42)); assert_eq!(observed.partition.get(), 7); - // A key that passes the probe but a payload that is not a marker: dropped, not a marker. - assert!(classify_marker(Some(b"2:42:run"), Some(b"{\"not\":\"a marker\"}")).is_none()); + // A membership row, i.e. what a mis-pointed topic delivers: no `reconcile_complete` type. + assert!(classify_marker(Some(b"{\"not\":\"a marker\"}")).is_none()); + assert!(classify_marker(None).is_none()); // A marker whose partition is outside the bitmap range is corrupt: dropped. - assert!(classify_marker(Some(b"2:42:run"), Some(&marker_payload(64))).is_none()); + assert!(classify_marker(Some(&marker_payload(64))).is_none()); } #[test] diff --git a/rust/cohort-seeder/src/kafka/producer.rs b/rust/cohort-seeder/src/kafka/producer.rs index 5c0feb28ddbd..fe9de94e48d5 100644 --- a/rust/cohort-seeder/src/kafka/producer.rs +++ b/rust/cohort-seeder/src/kafka/producer.rs @@ -150,9 +150,49 @@ impl SeedTileProducer { budget, }) }; + let partitions = self.topic_partition_ids(topic, remaining(deadline)?)?; let client = self.producer.client(); - let metadata = client - .fetch_metadata(Some(topic), remaining(deadline)?) + let mut positions = WatchPositions::new(); + for partition in partitions { + let (_low, high) = client + .fetch_watermarks(topic, partition, remaining(deadline)?) + .map_err(|source| CaptureOffsetsError::Watermarks { + topic: topic.to_string(), + partition, + source, + })?; + positions.insert( + WatchPartition::new(partition), + NextOffset::from_high_watermark(high), + ); + } + Ok(positions) + } + + /// Prove `topic` exists and reports partitions. Metadata-only, so it costs one round trip where + /// [`Self::capture_topic_offsets`] costs another per partition: a preflight needs existence, and + /// the offsets it would collect are stale the moment it returns. Blocking — call via + /// `spawn_blocking` from async contexts. + pub fn verify_topic_reachable( + &self, + topic: &str, + timeout: Duration, + ) -> Result<(), CaptureOffsetsError> { + self.topic_partition_ids(topic, timeout).map(|_| ()) + } + + /// The topic's partition ids, with the "is this topic actually there" checks the offset capture + /// and the reachability probe share. A topic reporting no partitions is refused here: an empty + /// position set would be vacuously "caught up", minting a settlement proof over nothing. + fn topic_partition_ids( + &self, + topic: &str, + timeout: Duration, + ) -> Result, CaptureOffsetsError> { + let metadata = self + .producer + .client() + .fetch_metadata(Some(topic), timeout) .map_err(CaptureOffsetsError::Metadata)?; let topic_metadata = metadata .topics() @@ -168,33 +208,21 @@ impl SeedTileProducer { }); } if topic_metadata.partitions().is_empty() { - // An empty position set would be vacuously "caught up", minting a settlement proof over - // nothing. Refuse it at the source, the way `verify_partition_count` refuses a - // mis-provisioned seed topic. return Err(CaptureOffsetsError::NoPartitions { topic: topic.to_string(), }); } - let mut positions = WatchPositions::new(); - for partition in topic_metadata.partitions() { - let (_low, high) = client - .fetch_watermarks(topic, partition.id(), remaining(deadline)?) - .map_err(|source| CaptureOffsetsError::Watermarks { - topic: topic.to_string(), - partition: partition.id(), - source, - })?; - positions.insert( - WatchPartition::new(partition.id()), - NextOffset::from_high_watermark(high), - ); - } - Ok(positions) + Ok(topic_metadata + .partitions() + .iter() + .map(|partition| partition.id()) + .collect()) } } -/// Why capturing the marker topic's start positions failed. The dispatch cannot record a resumable -/// watch state without them, so every variant aborts the dispatch (it re-converges on the next tick). +/// Why a marker-topic metadata read failed — capturing the watch start positions, or the startup +/// probe that only proves the topic is there. The dispatch cannot record a resumable watch state +/// without those positions, so every variant aborts the dispatch (it re-converges on the next tick). #[derive(Debug, thiserror::Error)] pub enum CaptureOffsetsError { #[error("fetching marker topic metadata")] diff --git a/rust/cohort-seeder/src/main.rs b/rust/cohort-seeder/src/main.rs index 55fde7919dbb..b8dd5e5eb3ab 100644 --- a/rust/cohort-seeder/src/main.rs +++ b/rust/cohort-seeder/src/main.rs @@ -16,6 +16,7 @@ use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::util::SubscriberInitExt; use tracing_subscriber::{fmt, EnvFilter, Layer}; +use cohort_seeder::app::completion::verify_marker_topic; use cohort_seeder::app::{ AutoDispatchPolicy, CompletionDriver, KafkaCommittedOffsets, KafkaTopicOffsets, MarkerWatchTask, ObservePolicy, OrchestratorSettings, PersonComponents, PgMarkerFlush, @@ -199,21 +200,7 @@ async fn build_completion( // Both halves anchor on the marker topic — dispatch captures its watermarks, the observer watches // it for markers — so prove it is reachable before either arms. - let verify_producer = producer.clone(); - let marker_topic = config.cohort_reconcile_markers_topic.clone(); - tokio::task::spawn_blocking(move || { - verify_producer.capture_topic_offsets(&marker_topic, PARTITION_VERIFY_TIMEOUT) - }) - .await - .context("joining marker topic verification task")? - .with_context(|| { - format!( - "verifying the marker topic {:?} is reachable. Provision it before arming either \ - completion half, or leave SEEDER_RECONCILE_AUTO_DISPATCH_ENABLED and \ - SEEDER_RECONCILE_OBSERVER_ENABLED off to boot without it.", - config.cohort_reconcile_markers_topic, - ) - })?; + verify_marker_topic(producer, &config.cohort_reconcile_markers_topic).await?; let mut driver = CompletionDriver::new( pool.clone(), diff --git a/rust/cohort-seeder/tests/pg_completion.rs b/rust/cohort-seeder/tests/pg_completion.rs index af5c06294379..b884b27f5a21 100644 --- a/rust/cohort-seeder/tests/pg_completion.rs +++ b/rust/cohort-seeder/tests/pg_completion.rs @@ -877,8 +877,7 @@ async fn discovery_classifies_each_phase_and_drops_observed_runs() -> Result<()> &full_hwms(), &MarkerWatch { topic: "cohort_membership_changed_shadow".to_string(), - positions: WatchPositions::new(), - ends: None, + ..empty_watch() }, ) .await?; diff --git a/rust/cohort-stream-processor/src/config.rs b/rust/cohort-stream-processor/src/config.rs index 0f3ebd095501..b9ada9afb2aa 100644 --- a/rust/cohort-stream-processor/src/config.rs +++ b/rust/cohort-stream-processor/src/config.rs @@ -318,8 +318,12 @@ pub struct Config { /// `message.timeout.ms` for the marker producer alone — much shorter than the shared 20 s. The /// marker produce runs inline on a partition worker, so a broker that black-holes it stalls live /// evaluation on that partition for the whole timeout, and the drain sweeper re-queues the job a - /// tick later and stalls it again. A fast fail just yields the job for the next tick. - #[envconfig(default = "2000")] + /// tick later and stalls it again. Half of `COHORT_SEED_RECONCILE_TICK_INTERVAL_MS`, so the + /// worker is free for events between attempts rather than held end to end, and still above the + /// ingestion cluster's p99 produce ack (~950 ms on prod-us) — below that, healthy tail produces + /// turn into spurious marker failures and bury the signal in + /// `cohort_reconcile_marker_produce_errors_total`. + #[envconfig(default = "1000")] pub reconcile_marker_message_timeout_ms: u32, /// `murmur2_random` co-partitions a `person_id` key identically to the Node/Python producers. @@ -1107,7 +1111,7 @@ mod tests { pod_hostname: None, cohort_membership_changed_topic: "cohort_membership_changed_shadow".to_string(), cohort_reconcile_markers_topic: "cohort_reconcile_markers".to_string(), - reconcile_marker_message_timeout_ms: 2000, + reconcile_marker_message_timeout_ms: 1000, kafka_producer_partitioner: "murmur2_random".to_string(), cohort_partition_count: 64, kafka_compression_codec: "none".to_string(), @@ -1196,11 +1200,24 @@ mod tests { defaults.reconcile_tick_interval(), Duration::from_millis(2_000) ); + // The marker produce blocks a partition worker, so its timeout has to stay under the tick + // that re-queues it — at parity a black-holed topic occupies the worker end to end. + assert_eq!( + defaults + .build_marker_kafka_config() + .kafka_message_timeout_ms, + 1000, + ); + assert!( + u64::from(defaults.reconcile_marker_message_timeout_ms) + < defaults.reconcile_tick_interval().as_millis() as u64 + ); let env: std::collections::HashMap = [ ("COHORT_SEED_RECONCILE_ENABLED", "true"), ("COHORT_SEED_RECONCILE_SCAN_PAGE", "17"), ("COHORT_SEED_RECONCILE_TICK_INTERVAL_MS", "345"), + ("RECONCILE_MARKER_MESSAGE_TIMEOUT_MS", "125"), ] .into_iter() .map(|(key, value)| (key.to_string(), value.to_string())) @@ -1209,6 +1226,10 @@ mod tests { assert!(config.cohort_seed_reconcile_enabled); assert_eq!(config.cohort_seed_reconcile_scan_page, 17); assert_eq!(config.reconcile_tick_interval(), Duration::from_millis(345)); + assert_eq!( + config.build_marker_kafka_config().kafka_message_timeout_ms, + 125, + ); } #[test] @@ -2095,7 +2116,7 @@ mod tests { assert_eq!(transfer.kafka_message_timeout_ms, 2000); assert_eq!( config.build_marker_kafka_config().kafka_message_timeout_ms, - 2000, + 1000, ); assert_eq!(shared.kafka_message_timeout_ms, 20_000); assert_eq!( diff --git a/rust/cohort-stream-processor/src/producer/marker.rs b/rust/cohort-stream-processor/src/producer/marker.rs index c95189e0cf44..894af32c94f6 100644 --- a/rust/cohort-stream-processor/src/producer/marker.rs +++ b/rust/cohort-stream-processor/src/producer/marker.rs @@ -7,12 +7,13 @@ //! One topic property the seeder's watcher depends on, recorded here because the topic is //! provisioned in another repo and would fail silently: a stable partition count. The watcher //! captures a start offset per partition at dispatch and is assigned exactly those, so a partition -//! added mid-run is never read and holds the run open. +//! added mid-run is never read. Added before that run's observation ends are captured, it holds the +//! run open until a re-dispatch; added after them, it is not even consulted, and the run settles +//! short on the markers that landed where nothing was watching. //! -//! The key ([`reconcile_complete_key`]) carries the body partition so every marker of a run is a -//! distinct key. The topic is provisioned `cleanup.policy=delete`, but a compacting one would keep -//! all 64 rather than collapsing them to whichever arrived last — a mis-provision that would -//! otherwise record completed backfills as short. +//! The key ([`reconcile_complete_key`]) carries the body partition, so a run's 64 certificates are +//! 64 distinct keys rather than one — what stops a compacting topic from keeping whichever arrived +//! last and recording a completed backfill as short. use anyhow::{Context, Result}; use async_trait::async_trait; @@ -114,8 +115,8 @@ impl ReconcileMarkerSink for CaptureReconcileMarkerSink { } /// Unique per marker, not per run: the partition is what makes 64 distinct keys out of one dispatch, -/// so no cleanup policy can collapse them. Nothing downstream reads the key beyond the watcher's -/// `':'` probe, and the ledger's fold is order-independent, so the grouping carries no meaning. +/// so no cleanup policy can collapse them. Nothing downstream reads the key and the ledger's fold is +/// order-independent, so the grouping carries no meaning beyond that. fn reconcile_complete_key(marker: &ReconcileCompleteMarker) -> Option { Some(format!( "{}:{}:{}:{}", diff --git a/rust/cohort-stream-processor/src/producer/mod.rs b/rust/cohort-stream-processor/src/producer/mod.rs index 2a4989abd56b..fc481d366147 100644 --- a/rust/cohort-stream-processor/src/producer/mod.rs +++ b/rust/cohort-stream-processor/src/producer/mod.rs @@ -10,9 +10,6 @@ pub mod marker; pub mod merge; pub mod seed; -use std::sync::atomic::{AtomicUsize, Ordering}; -use std::sync::{Arc, Mutex}; - use async_trait::async_trait; use chrono::Utc; use common_kafka::kafka_producer::KafkaProduceError; @@ -27,6 +24,7 @@ pub use cohort_core::seed::ReconcileCompleteMarker; use crate::filters::reverse_index::TeamFilters; use crate::filters::CohortId; use crate::observability::metrics::OUTPUT_TRANSITIONS_UNMAPPED; +use crate::producer::merge::Capture; use crate::stage1::transition::{LeafTransition, TransitionKind}; pub use batcher::OutputBuffer; @@ -171,10 +169,7 @@ pub trait MembershipSink: Send + Sync { } #[derive(Debug, Default, Clone)] -pub struct CaptureSink { - changes: Arc>>, - fail_remaining: Arc, -} +pub struct CaptureSink(Capture); impl CaptureSink { pub fn new() -> Self { @@ -182,25 +177,11 @@ impl CaptureSink { } pub fn failing_first(n: usize) -> Self { - Self { - changes: Arc::default(), - fail_remaining: Arc::new(AtomicUsize::new(n)), - } + Self(Capture::failing_first(n)) } pub fn changes(&self) -> Vec { - self.changes - .lock() - .expect("CaptureSink mutex poisoned") - .clone() - } - - fn should_fail(&self) -> bool { - self.fail_remaining - .fetch_update(Ordering::SeqCst, Ordering::SeqCst, |n| { - (n > 0).then(|| n - 1) - }) - .is_ok() + self.0.recorded() } } @@ -210,18 +191,7 @@ impl MembershipSink for CaptureSink { &self, changes: Vec, ) -> Vec> { - if self.should_fail() { - return changes - .into_iter() - .map(|_| Err(KafkaProduceError::KafkaProduceCanceled)) - .collect(); - } - let acks = (0..changes.len()).map(|_| Ok(())).collect(); - self.changes - .lock() - .expect("CaptureSink mutex poisoned") - .extend(changes); - acks + self.0.produce(changes) } } diff --git a/rust/cohort-stream-processor/src/workers/reconcile.rs b/rust/cohort-stream-processor/src/workers/reconcile.rs index 3acb4d7d7fe6..7950692b60b2 100644 --- a/rust/cohort-stream-processor/src/workers/reconcile.rs +++ b/rust/cohort-stream-processor/src/workers/reconcile.rs @@ -91,7 +91,9 @@ pub struct ReconcileDeps { pub enabled: bool, pub scan_page: usize, pub backlog: Arc, - /// Sink for `cohort_reconcile_markers` (a no-op when the reconcile gate is off). + /// Sink for `cohort_reconcile_markers`. With the reconcile gate off this is + /// [`NoopReconcileMarkerSink`], which fails every produce rather than acking a marker the seeder + /// would then wait for. pub marker_sink: Arc, } diff --git a/rust/cohort-stream-processor/tests/merge_durability.rs b/rust/cohort-stream-processor/tests/merge_durability.rs index ccb635ac93fb..5971c165af62 100644 --- a/rust/cohort-stream-processor/tests/merge_durability.rs +++ b/rust/cohort-stream-processor/tests/merge_durability.rs @@ -557,7 +557,6 @@ struct Topics { transfers: String, cascade: String, shadow: String, - markers: String, } impl Topics { @@ -568,7 +567,6 @@ impl Topics { transfers: format!("cohort_merge_state_transfer_{suffix}"), cascade: format!("cohort_cascade_events_{suffix}"), shadow: format!("cohort_membership_changed_{suffix}"), - markers: format!("cohort_reconcile_markers_{suffix}"), } } @@ -578,14 +576,13 @@ impl Topics { } } - fn names(&self) -> [&str; 6] { + fn names(&self) -> [&str; 5] { [ &self.events, &self.merges, &self.transfers, &self.cascade, &self.shadow, - &self.markers, ] } } @@ -908,10 +905,6 @@ fn merge_durability_config( "COHORT_MEMBERSHIP_CHANGED_TOPIC".into(), topics.shadow.clone(), ); - env.insert( - "COHORT_RECONCILE_MARKERS_TOPIC".into(), - topics.markers.clone(), - ); env.insert("KAFKA_CONSUMER_GROUP".into(), groups.events.clone()); env.insert("KAFKA_MERGE_CONSUMER_GROUP".into(), groups.merges.clone()); env.insert(