apollo_l1_gas_price,apollo_l1_gas_price_types: hold Chainlink query failures for the retry interval

crates/apollo_l1_gas_price/src/chainlink_oracle/mod.rs

@@ -60,6 +60,9 @@ struct ValidRead {
 struct PairOracleState {
     // The newest read that passed every guard, served to callers until a newer one replaces it.
     last_valid_read: Option<ValidRead>,
+    // The newest query's failure, cleared by the next success. Served only when no valid read is
+    // held.
+    last_error: Option<ExchangeRateOracleClientError>,
     // The query in flight. A single slot bounds this client to one query at a time.
     query: Option<RateQuery>,
     // When the last query was spawned, on the local monotonic clock, which the refresh cadence is
@@ -161,7 +164,8 @@ impl<Kind: ChainlinkRate> ChainlinkOracleClient<Kind> {
         })
     }
 
-    // Moves a finished query's success into `state` as the last valid read. Called on every
+    // Moves a finished query's outcome into `state`: a success becomes the last valid read and
+    // clears the last error, a failure becomes the last error. Called on every
     // `fetch_rate`, so that a query which resolved after the last caller that could have observed
     // it is harvested rather than dropped together with the round trip that produced it.
     fn harvest_finished_query(&self, state: &mut PairOracleState) {
@@ -180,16 +184,19 @@ impl<Kind: ChainlinkRate> ChainlinkOracleClient<Kind> {
             warn!("Query failed to join its handle: {error:?}");
             Err(error)
         });
-        // Nothing is held for a failure: the next query waits for the sampling interval like any
-        // other refresh, and `spawn_query` already warned.
-        if let Ok(valid_read) = result {
-            debug!(
-                "Harvested {:?} rate {} for block timestamp {}",
-                Kind::PAIR,
-                valid_read.rate,
-                valid_read.block_timestamp
-            );
-            state.last_valid_read = Some(valid_read);
+        match result {
+            Ok(valid_read) => {
+                debug!(
+                    "Harvested {:?} rate {} for block timestamp {}",
+                    Kind::PAIR,
+                    valid_read.rate,
+                    valid_read.block_timestamp
+                );
+                state.last_valid_read = Some(valid_read);
+                state.last_error = None;
+            }
+            // `spawn_query` already warned; this only holds it for the retry interval.
+            Err(error) => state.last_error = Some(error),
         }
     }
 }
@@ -207,19 +214,28 @@ impl<Kind: ChainlinkRate> ExchangeRateOracleClientTrait for ChainlinkOracleClien
         let mut state = self.state.lock().unwrap();
         self.harvest_finished_query(&mut state);
 
-        let sampling_interval = Duration::from_secs(self.config.sampling_interval_seconds);
+        let refresh_interval_seconds = if state.last_error.is_some() {
+            self.config.failure_retry_interval_seconds
+        } else {
+            self.config.sampling_interval_seconds
+        };
+        let refresh_interval = Duration::from_secs(refresh_interval_seconds);
         let is_refresh_due = state
             .last_attempt_instant
-            .is_none_or(|last_attempt| last_attempt.elapsed() >= sampling_interval);
+            .is_none_or(|last_attempt| last_attempt.elapsed() >= refresh_interval);
         if state.query.is_none() && is_refresh_due {
             state.query = Some(self.spawn_query(block_timestamp));
             state.last_attempt_instant = Some(Instant::now());
         }
 
         // A caller whose own read has not resolved is served the read the client already holds,
-        // including the caller that spawned the read in flight.
-        match state.last_valid_read {
-            Some(valid_read) => Ok(valid_read.rate),
+        // including the caller that spawned the read in flight. A held failure is served only when
+        // no valid read is held.
+        if let Some(valid_read) = state.last_valid_read {
+            return Ok(valid_read.rate);
+        }
+        match &state.last_error {
+            Some(error) => Err(error.clone()),
             None => Err(ExchangeRateOracleClientError::QueryNotReadyError(block_timestamp)),
         }
     }

crates/apollo_l1_gas_price/src/chainlink_oracle/test.rs

@@ -95,6 +95,29 @@ async fn wait_for_query_to_finish<Kind: ChainlinkRate>(client: &ChainlinkOracleC
     panic!("Query did not finish within {MAX_POLL_ATTEMPTS} attempts");
 }
 
+/// The failure the client holds, which a held valid read masks.
+fn held_error<Kind: ChainlinkRate>(
+    client: &ChainlinkOracleClient<Kind>,
+) -> Option<ExchangeRateOracleClientError> {
+    client.state.lock().unwrap().last_error.clone()
+}
+
+/// Drives `fetch_rate` until the client holds a failure. Unlike `resolve_rate` this does not stop
+/// at the first `Ok`, which for a failing query is the last valid rate.
+async fn wait_for_held_error<Kind: ChainlinkRate>(
+    client: &ChainlinkOracleClient<Kind>,
+    block_timestamp: u64,
+) {
+    for _ in 0..MAX_POLL_ATTEMPTS {
+        if held_error(client).is_some() {
+            return;
+        }
+        let _ = client.fetch_rate(block_timestamp).await;
+        tokio::task::yield_now().await;
+    }
+    panic!("Query for {block_timestamp} did not fail within {MAX_POLL_ATTEMPTS} attempts");
+}
+
 #[tokio::test]
 async fn strk_to_usd_rescales_feed_answer_to_eighteen_decimals() {
     let client = make_client::<StrkToUsd>(strk_usd_responses(FeedFixture::new(
@@ -328,3 +351,110 @@ async fn a_failed_query_records_the_rejection_reason_and_pair(
         }
     }
 }
+
+/// With no valid rate to fall back on, the caller sees the failure, and the failing feed is
+/// queried once per retry interval rather than once per call.
+#[tokio::test(start_paused = true)]
+async fn failed_query_is_held_until_the_retry_interval_elapses() {
+    let failure_retry_interval_seconds = test_config().failure_retry_interval_seconds;
+    let (batcher_client, num_batcher_calls) = counting_batcher_client(FeedResponses::new());
+    let client = client_with_batcher::<StrkToUsd>(batcher_client);
+
+    assert_matches!(
+        resolve_rate(&client, TIMESTAMP).await,
+        Err(ExchangeRateOracleClientError::ContractCallError(_))
+    );
+    let num_calls_after_failure = num_batcher_calls.load(Ordering::SeqCst);
+    assert!(last_valid_read(&client).is_none());
+
+    // Every step together stays one second short of the retry interval.
+    const NUM_LATER_CALLS: u64 = 10;
+    let step_seconds = (failure_retry_interval_seconds - 1) / NUM_LATER_CALLS;
+    assert!(step_seconds > 0);
+    for _ in 0..NUM_LATER_CALLS {
+        tokio::time::advance(Duration::from_secs(step_seconds)).await;
+        assert_matches!(
+            client.fetch_rate(TIMESTAMP).await,
+            Err(ExchangeRateOracleClientError::ContractCallError(_))
+        );
+    }
+    assert_eq!(num_batcher_calls.load(Ordering::SeqCst), num_calls_after_failure);
+}
+
+/// A failed read is retried a retry interval later, so a transient failure costs one retry
+/// interval rather than the rest of the sampling interval.
+#[tokio::test(start_paused = true)]
+async fn a_failed_read_is_retried_after_the_retry_interval() {
+    let failure_retry_interval_seconds = test_config().failure_retry_interval_seconds;
+    let (batcher_client, num_batcher_calls) = counting_batcher_client(FeedResponses::new());
+    let client = client_with_batcher::<StrkToUsd>(batcher_client);
+    assert_matches!(
+        resolve_rate(&client, TIMESTAMP).await,
+        Err(ExchangeRateOracleClientError::ContractCallError(_))
+    );
+    let num_calls_after_first_attempt = num_batcher_calls.load(Ordering::SeqCst);
+
+    // One second short of the retry interval, the failure still stands and nothing is queried.
+    tokio::time::advance(Duration::from_secs(failure_retry_interval_seconds - 1)).await;
+    assert_matches!(
+        client.fetch_rate(TIMESTAMP).await,
+        Err(ExchangeRateOracleClientError::ContractCallError(_))
+    );
+    assert_eq!(num_batcher_calls.load(Ordering::SeqCst), num_calls_after_first_attempt);
+
+    // At the retry interval the feed is queried again. The held failure is what this call is
+    // served, since the retry it spawns has nothing to answer with yet.
+    tokio::time::advance(Duration::from_secs(1)).await;
+    assert_matches!(
+        client.fetch_rate(TIMESTAMP).await,
+        Err(ExchangeRateOracleClientError::ContractCallError(_))
+    );
+    assert!(is_query_in_flight(&client), "the retry interval elapsed but no query was spawned");
+    wait_for_query_to_finish(&client).await;
+    assert!(
+        num_batcher_calls.load(Ordering::SeqCst) > num_calls_after_first_attempt,
+        "the retry issued no batcher call"
+    );
+}
+
+/// A held failure keeps the batcher from being queried again before the retry interval, but it
+/// must not deny the proposal path a rate the client already holds.
+#[tokio::test(start_paused = true)]
+async fn a_held_failure_does_not_mask_the_last_valid_rate() {
+    let client = client_with_batcher::<StrkToUsd>(batcher_client_failing_after(
+        strk_usd_responses(FeedFixture::new(STRK_USD_ANSWER, fresh_updated_at())),
+        CALLS_PER_FEED_PER_QUERY,
+    ));
+    let rate = resolve_rate(&client, TIMESTAMP).await.unwrap();
+
+    tokio::time::advance(Duration::from_secs(sampling_interval_seconds())).await;
+    let later_timestamp = TIMESTAMP + sampling_interval_seconds();
+    wait_for_held_error(&client, later_timestamp).await;
+
+    assert_eq!(client.fetch_rate(later_timestamp).await.unwrap(), rate);
+}
+
+/// The fallback must hold for the one call that observes a failing query finish and stores the
+/// failure, not just the calls before and after it.
+#[tokio::test(start_paused = true)]
+async fn no_call_is_denied_a_rate_the_client_holds() {
+    let client = client_with_batcher::<StrkToUsd>(batcher_client_failing_after(
+        strk_usd_responses(FeedFixture::new(STRK_USD_ANSWER, fresh_updated_at())),
+        CALLS_PER_FEED_PER_QUERY,
+    ));
+    let rate = resolve_rate(&client, TIMESTAMP).await.unwrap();
+
+    tokio::time::advance(Duration::from_secs(sampling_interval_seconds())).await;
+    let later_timestamp = TIMESTAMP + sampling_interval_seconds();
+    for _ in 0..MAX_POLL_ATTEMPTS {
+        if held_error(&client).is_some() {
+            break;
+        }
+        assert_eq!(
+            client.fetch_rate(later_timestamp).await.expect("Last valid rate should be served"),
+            rate
+        );
+        tokio::task::yield_now().await;
+    }
+    assert!(held_error(&client).is_some(), "the failing query was never harvested");
+}

crates/apollo_l1_gas_price_types/src/lib.rs

@@ -172,8 +172,6 @@ pub trait ExchangeRateOracleClientTrait: Send + Sync + Debug {
 }
 
 /// The rate an oracle client instance produces.
-// [Temporary comment] No implementors outside this module yet: the Chainlink client PR (A9) adds
-// them.
 pub trait RateKind: Send + Sync + Debug + 'static {
     const PAIR: CurrencyPair;
 }