retry: share one backon helper for the client secret reads and convert the remaining doubling tables

swarm-controller/src/forge.rs

@@ -21,6 +21,7 @@
 //! either.
 
 use anyhow::{Context, Result};
+use backon::{ExponentialBuilder, Retryable as _};
 use forgejo_api::structs::{
     AddCollaboratorOption, AddCollaboratorOptionPermission, ChangeFileOperation,
     ChangeFileOperationOperation, ChangeFilesOptions, CommitStatusState,
@@ -272,52 +273,48 @@ pub fn url_from_env() -> Result<Option<url::Url>> {
         .map(Some)
 }
 
+/// How long the background re-read waits before its first attempt. The caller
+/// has just made one, so an immediate retry would only repeat it.
+const BACKGROUND_FIRST: std::time::Duration = std::time::Duration::from_secs(30);
+
 /// The longest the background re-read waits between attempts.
 const BACKGROUND_CAP: std::time::Duration = std::time::Duration::from_mins(5);
 
-/// How long the background re-read waits before its `n`-th attempt: 30s,
-/// doubling, capped at [`BACKGROUND_CAP`].
-fn background_delay(n: u32) -> std::time::Duration {
-    const FIRST_S: u64 = 30;
-    std::time::Duration::from_secs(
-        FIRST_S
-            .saturating_mul(1u64.checked_shl(n).unwrap_or(u64::MAX))
-            .min(BACKGROUND_CAP.as_secs()),
-    )
-}
-
-/// Whether the `n`-th failed attempt is logged at `info` rather than `debug`:
-/// once the backoff is capped, so a controller that never gets its token says
-/// so once per [`BACKGROUND_CAP`] without a line per early retry.
-fn failure_logs_at_info(n: u32) -> bool {
-    background_delay(n) >= BACKGROUND_CAP
-}
-
-/// Fill `slot` with the first value `attempt` yields, sleeping
-/// [`background_delay`] before each attempt. Never gives up. A failure is logged
-/// as [`failure_logs_at_info`] says; the caller logs the first one and the
-/// recovery.
-async fn fill_when_ready<T, F, Fut>(slot: &OnceLock<T>, mut attempt: F) -> &T
+/// Fill `slot` with the first value `attempt` yields: after
+/// [`BACKGROUND_FIRST`], then at 60s, doubling, capped at [`BACKGROUND_CAP`].
+/// Never gives up. A failure is logged at `info` once the next wait is the cap,
+/// so a controller that never gets its token says so once per
+/// [`BACKGROUND_CAP`] without a line per early retry, and at `debug` before
+/// that. The caller logs the first failure and the recovery.
+async fn fill_when_ready<T, F, Fut>(slot: &OnceLock<T>, attempt: F) -> &T
 where
     F: FnMut() -> Fut,
     Fut: std::future::Future<Output = Result<T>>,
 {
-    let mut n = 0;
-    loop {
-        tokio::time::sleep(background_delay(n)).await;
-        match attempt().await {
-            Ok(value) => return slot.get_or_init(|| value),
-            Err(e) if failure_logs_at_info(n) => tracing::info!(
-                error = %format!("{e:#}"),
-                "still no forge access: the controller's forge token is unavailable"
-            ),
-            Err(e) => tracing::debug!(
-                error = %format!("{e:#}"),
-                "the controller's forge token is still unavailable"
-            ),
-        }
-        n = n.saturating_add(1);
-    }
+    tokio::time::sleep(BACKGROUND_FIRST).await;
+    let value = attempt
+        .retry(
+            ExponentialBuilder::default()
+                .with_min_delay(BACKGROUND_FIRST * 2)
+                .with_max_delay(BACKGROUND_CAP)
+                .without_max_times(),
+        )
+        .notify(|e, wait| {
+            if wait >= BACKGROUND_CAP {
+                tracing::info!(
+                    error = %format!("{e:#}"),
+                    "still no forge access: the controller's forge token is unavailable"
+                );
+            } else {
+                tracing::debug!(
+                    error = %format!("{e:#}"),
+                    "the controller's forge token is still unavailable"
+                );
+            }
+        })
+        .await
+        .expect("a retry with no cap on its tries returns only once it succeeds");
+    slot.get_or_init(|| value)
 }
 
 /// The longest one unit of forge work, a periodic pass or a job node, may run
@@ -2020,21 +2017,6 @@ mod tests {
         }
     }
 
-    #[test]
-    fn a_failed_re_read_logs_at_info_only_once_the_backoff_is_capped() {
-        assert!((0..4).all(|n| !failure_logs_at_info(n)));
-        assert!(failure_logs_at_info(4));
-        assert!(failure_logs_at_info(40));
-    }
-
-    #[test]
-    fn the_background_re_read_backs_off_from_30s_to_a_5_minute_cap() {
-        let s = |n| background_delay(n).as_secs();
-        assert_eq!([s(0), s(1), s(2), s(3), s(4)], [30, 60, 120, 240, 300]);
-        assert_eq!(s(40), 300);
-        assert_eq!(s(u32::MAX), 300);
-    }
-
     // Paused time: each sleep completes at once and advances the clock, so the
     // elapsed time is the backoff schedule itself.
     #[tokio::test(start_paused = true)]
@@ -2059,6 +2041,29 @@ mod tests {
         assert_eq!(start.elapsed().as_secs(), 30 + 60 + 120);
     }
 
+    #[tokio::test(start_paused = true)]
+    async fn a_token_that_stays_absent_is_retried_every_five_minutes() {
+        let slot = OnceLock::new();
+        let start = tokio::time::Instant::now();
+        let mut calls = 0;
+        fill_when_ready(&slot, || {
+            calls += 1;
+            let n = calls;
+            async move {
+                if n < 7 {
+                    anyhow::bail!("nothing at the controller's forge token path")
+                }
+                Ok("client")
+            }
+        })
+        .await;
+        assert_eq!(calls, 7);
+        assert_eq!(
+            start.elapsed().as_secs(),
+            30 + 60 + 120 + 240 + 300 + 300 + 300
+        );
+    }
+
     #[tokio::test(start_paused = true)]
     async fn a_pass_that_never_answers_is_abandoned_at_the_deadline() {
         let start = tokio::time::Instant::now();