diff --git a/crates/didbot-serve/src/firehose.rs b/crates/didbot-serve/src/firehose.rs index 53464cbe..bb3cdf90 100644 --- a/crates/didbot-serve/src/firehose.rs +++ b/crates/didbot-serve/src/firehose.rs @@ -54,6 +54,17 @@ //! `:`, where the instance is minted once at boot: sequence //! numbers restart at 1 with the process, so a bare integer from a previous //! life would name a frame that exists and is not the one the consumer meant. +//! +//! # A consumer that stops reading is ended +//! +//! Falling behind the live channel is the third way a hole opens, and it is +//! answered the same way `com.atproto.sync.subscribeRepos` answers it in +//! [`crate::subscribe`]: the connection ends. These are the same events read +//! two ways, so a reader here is in the position that rule exists for, and a +//! stream that resumed over the frames it dropped would leave that reader +//! believing it had applied every one. Ending it makes the hole something the +//! consumer acts on — reconnect with the cursor it last applied — rather than +//! something it has to notice by comparing sequence numbers. use std::collections::VecDeque; use std::convert::Infallible; @@ -746,6 +757,11 @@ pub struct FirehoseStream { /// The highest sequence number the replay already delivered. replayed: u64, rx: mpsc::Receiver>, + /// How many frames were dropped because this consumer fell behind. + /// + /// Written by the forwarder task and read after the stream ends, which is + /// what tells an ended stream apart from a producer that went away. + lagged: Arc, } impl FirehoseStream { @@ -763,6 +779,8 @@ impl FirehoseStream { pending.extend(replay); let (out, rx) = mpsc::channel(CLIENT_BUFFER); + let lagged = Arc::new(AtomicU64::new(0)); + let counted = Arc::clone(&lagged); tokio::spawn(async move { loop { match source.recv().await { @@ -773,10 +791,15 @@ impl FirehoseStream { } } Err(broadcast::error::RecvError::Lagged(skipped)) => { - // The consumer will see the gap for itself: sequence - // numbers are gapless, so the next frame's number is - // more than one past the last one it applied. - warn!(skipped, "firehose subscriber fell behind; frames dropped"); + // Ended rather than resumed over the hole; see the + // module documentation. Dropping `out` here is what + // ends the stream the client is reading. + counted.fetch_add(skipped, Ordering::SeqCst); + warn!( + skipped, + "a firehose subscriber fell behind; the stream is ending" + ); + break; } Err(broadcast::error::RecvError::Closed) => break, } @@ -787,11 +810,20 @@ impl FirehoseStream { pending, replayed, rx, + lagged, } } } impl FirehoseStream { + /// How many frames this consumer was too slow to be handed. + /// + /// Nonzero once the stream has ended says why it ended. + #[must_use] + pub fn lagged(&self) -> u64 { + self.lagged.load(Ordering::SeqCst) + } + /// The next frame to go out, still shared with whoever else holds it. /// /// The replay first, then the live channel. Split out from @@ -1002,6 +1034,33 @@ mod tests { std::future::poll_fn(|cx| stream.poll_shared(cx)).await } + /// A consumer that stops reading is ended, not fast-forwarded. + /// + /// The same rule `subscribeRepos` follows, over the same events: a stream + /// that resumed after the frames it dropped would leave the consumer + /// believing it had applied every one, and it would have to compare + /// sequence numbers to find out otherwise. + #[tokio::test] + async fn a_stream_that_fell_behind_ends_rather_than_skipping_to_the_present() { + let firehose = Firehose::new(4); + let mut stream = firehose.subscribe(None); + next_shared(&mut stream).await.expect("the info frame"); + + // Nothing is read while these are published, so the live channel + // overruns and the frames at the front of it are gone. + for _ in 0..12 { + firehose.publish(&commit("3lq")); + } + assert!( + next_shared(&mut stream).await.is_none(), + "a consumer that fell behind was handed a frame with a hole in front of it" + ); + assert!( + stream.lagged() > 0, + "the stream ended without recording that anything was dropped" + ); + } + /// The live channel hands frames out in sequence order, however many /// writers race to publish: an index reads `seq` as gapless within a /// connection, so a frame that arrives after a higher one is a gap and diff --git a/crates/didbot-serve/src/subscribe.rs b/crates/didbot-serve/src/subscribe.rs index d0c48b17..babaaf9a 100644 --- a/crates/didbot-serve/src/subscribe.rs +++ b/crates/didbot-serve/src/subscribe.rs @@ -769,7 +769,9 @@ impl ReposStream { /// A consumer that falls further behind than the broadcast channel is deep /// is dropped rather than silently skipped: the lexicon has /// `ConsumerTooSlow` for exactly this, and a stream that resumed after a - /// gap would leave a relay believing it had seen every commit. + /// gap would leave a relay believing it had seen every commit. `/firehose` + /// is the same events read a second way and answers a slow consumer the + /// same. pub async fn next(&mut self) -> Option { if let Some(bytes) = self.pending.pop_front() { return Some(bytes);