Skip to content

Commit 9bb641d

Browse files
ranking: fail closed when engagement-count store errors
Dragonfly Err was coerced to an empty map, then Ok(default) counts. CachedHydrator cached those zeros for 60s and Phoenix unwrap_or(0) ranked the slate as if nobody engaged. Co-authored-by: Jon Bailey <Pitchfork-and-Torch@users.noreply.github.com>
1 parent 902a06f commit 9bb641d

1 file changed

Lines changed: 135 additions & 8 deletions

File tree

‎home-mixer/candidate_hydrators/engagement_counts_hydrator.rs‎

Lines changed: 135 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -131,16 +131,30 @@ impl CachedHydrator<ScoredPostsQuery, PostCandidate> for EngagementCountsHydrato
131131
unique_ids.sort_unstable();
132132
unique_ids.dedup();
133133

134+
// Store errors must not become Ok(empty). CachedHydrator caches Ok, and
135+
// as_tweet_info unwrap_or(0) would stamp Phoenix / Thompson as zero
136+
// engagement for 60s. LanguageCode / MediaInfo already return Err here.
134137
let counts = if unique_ids.is_empty() {
135-
HashMap::new()
138+
Ok(HashMap::new())
136139
} else {
137-
self.client
138-
.get_engagement_counts(&unique_ids)
139-
.await
140-
.unwrap_or_else(|e| {
141-
warn!(error = %e, "engagement_counts_hydration dragonfly_error");
142-
HashMap::new()
143-
})
140+
self.client.get_engagement_counts(&unique_ids).await
141+
};
142+
143+
let counts = match counts {
144+
Ok(counts) => counts,
145+
Err(e) => {
146+
warn!(error = %e, "engagement_counts_hydration dragonfly_error");
147+
return candidates
148+
.iter()
149+
.map(|c| {
150+
if !fetch_counts(c) {
151+
Ok(preserve_counts(c))
152+
} else {
153+
Err(format!("engagement_counts dragonfly_error: {e}"))
154+
}
155+
})
156+
.collect();
157+
}
144158
};
145159

146160
candidates
@@ -327,4 +341,117 @@ mod tests {
327341
assert_eq!(second[0].as_ref().unwrap().view_count, Some(5));
328342
assert_eq!(client.calls.load(Ordering::SeqCst), 1);
329343
}
344+
345+
#[derive(Default)]
346+
struct FailingClient {
347+
calls: AtomicUsize,
348+
}
349+
350+
#[tonic::async_trait]
351+
impl EngagementCountsClient for FailingClient {
352+
async fn get_engagement_counts(
353+
&self,
354+
_tweet_ids: &[u64],
355+
) -> Result<HashMap<u64, EngagementCounts>, String> {
356+
self.calls.fetch_add(1, Ordering::SeqCst);
357+
Err("dragonfly down".to_string())
358+
}
359+
}
360+
361+
#[tokio::test]
362+
async fn store_error_returns_err_instead_of_zero_counts() {
363+
let h = EngagementCountsHydrator::new(Arc::new(FailingClient::default())).await;
364+
let candidates = vec![PostCandidate {
365+
tweet_id: 10,
366+
author_id: 1,
367+
fav_count: Some(99),
368+
view_count: Some(50),
369+
view_count_on_home: Some(40),
370+
..Default::default()
371+
}];
372+
let q = query(false, &[(COUNTS, "true")]);
373+
let result = h.hydrate_from_client(&q, &candidates).await;
374+
assert!(
375+
result[0]
376+
.as_ref()
377+
.err()
378+
.is_some_and(|e| e.contains("dragonfly_error")),
379+
"store error must not become Ok(zero counts), got {:?}",
380+
result[0]
381+
);
382+
}
383+
384+
#[tokio::test]
385+
async fn store_error_does_not_overwrite_or_cache_zeros() {
386+
let client = Arc::new(FailingClient::default());
387+
let h = EngagementCountsHydrator::new(client.clone()).await;
388+
let candidates = vec![PostCandidate {
389+
tweet_id: 10,
390+
author_id: 1,
391+
fav_count: Some(99),
392+
view_count: Some(50),
393+
view_count_on_home: Some(40),
394+
..Default::default()
395+
}];
396+
let q = query(false, &[(COUNTS, "true")]);
397+
398+
let first = xai_candidate_pipeline::hydrator::Hydrator::hydrate(&h, &q, &candidates).await;
399+
let second = xai_candidate_pipeline::hydrator::Hydrator::hydrate(&h, &q, &candidates).await;
400+
assert!(first[0].is_err());
401+
assert!(second[0].is_err());
402+
assert_eq!(
403+
client.calls.load(Ordering::SeqCst),
404+
2,
405+
"Err must not be cached as empty counts"
406+
);
407+
408+
let mut live = candidates;
409+
xai_candidate_pipeline::hydrator::Hydrator::update_all(&h, &mut live, first);
410+
assert_eq!(live[0].fav_count, Some(99));
411+
assert_eq!(live[0].view_count, Some(50));
412+
assert_eq!(live[0].view_count_on_home, Some(40));
413+
}
414+
415+
#[tokio::test]
416+
async fn store_error_preserves_cached_ineligible_and_errs_eligible() {
417+
let h = EngagementCountsHydrator::new(Arc::new(FailingClient::default())).await;
418+
let candidates = vec![
419+
PostCandidate {
420+
tweet_id: 10,
421+
author_id: 1,
422+
author_followers_count: Some(100),
423+
view_count: Some(5),
424+
..Default::default()
425+
},
426+
PostCandidate {
427+
tweet_id: 20,
428+
author_id: 2,
429+
author_followers_count: Some(5000),
430+
view_count: Some(999),
431+
..Default::default()
432+
},
433+
];
434+
let q = query(
435+
true,
436+
&[(COLD_START, "true"), (COUNTS, "true"), (CAP, "1000")],
437+
);
438+
let result = h.hydrate_from_client(&q, &candidates).await;
439+
assert!(result[0].is_err());
440+
assert_eq!(result[1].as_ref().unwrap().view_count, Some(999));
441+
}
442+
443+
#[tokio::test]
444+
async fn successful_miss_still_hydrates_empty_counts() {
445+
let h = hydrator(HashMap::new()).await;
446+
let candidates = vec![PostCandidate {
447+
tweet_id: 10,
448+
author_id: 1,
449+
fav_count: Some(99),
450+
..Default::default()
451+
}];
452+
let q = query(false, &[(COUNTS, "true")]);
453+
let result = h.hydrate_from_client(&q, &candidates).await;
454+
assert_eq!(result[0].as_ref().unwrap().fav_count, None);
455+
assert_eq!(result[0].as_ref().unwrap().view_count, None);
456+
}
330457
}

0 commit comments

Comments
 (0)