From 87f33af772ada1f368cdbc6755aaf1b9b2a1977f Mon Sep 17 00:00:00 2001 From: charles-gauthereau Date: Thu, 27 Aug 2026 22:46:49 +0200 Subject: [PATCH] fix: wire retry env vars into helm configmap, dedupe terminal retry log helm/templates/env-configmap.yaml never listed RETRY_ATTEMPTS and RETRY_BACKOFF_MS even though values.yaml gained them, so --set env.RETRY_ATTEMPTS=N was silently ignored by Kubernetes deployments. Add both keys in the same explicit style as the existing entries. src/utils/retry.rs logged its own "failed after N attempts" error on exhaustion, on top of the terminal log each call site already writes, producing two error entries per failure. Worse, it changed a log level: FileLock::acquire's "backup_already_in_progress" bails through the combinator, which now logged it as error before runner.rs got a chance to reclassify it as the routine warn it always was. A manual backup colliding with a scheduled one would show up as a hard error on the dashboard instead of the harmless warn it used to be, breaking the "fails exactly as it does today" guarantee for job records. Drop the combinator's terminal error log and give download_backup its own terminal error log so all three call sites (runner, uploader, downloader) own their failure logging uniformly. Update the two tests that asserted the removed message to assert the new behavior instead. --- helm/templates/env-configmap.yaml | 4 +++- src/services/restore/downloader.rs | 10 ++++++++-- src/tests/services/backup_runner_tests.rs | 4 ++-- src/tests/utils/retry_tests.rs | 8 ++------ src/utils/retry.rs | 1 - 5 files changed, 15 insertions(+), 12 deletions(-) diff --git a/helm/templates/env-configmap.yaml b/helm/templates/env-configmap.yaml index 9afc8e9..a7894ed 100644 --- a/helm/templates/env-configmap.yaml +++ b/helm/templates/env-configmap.yaml @@ -7,4 +7,6 @@ data: TZ: {{ .Values.env.TZ | quote }} POLLING: {{ .Values.env.POLLING | quote }} APP_ENV: {{ .Values.env.APP_ENV | quote }} - LOG: {{ .Values.env.LOG | quote }} \ No newline at end of file + LOG: {{ .Values.env.LOG | quote }} + RETRY_ATTEMPTS: {{ .Values.env.RETRY_ATTEMPTS | quote }} + RETRY_BACKOFF_MS: {{ .Values.env.RETRY_BACKOFF_MS | quote }} diff --git a/src/services/restore/downloader.rs b/src/services/restore/downloader.rs index da65f96..4efaaf9 100644 --- a/src/services/restore/downloader.rs +++ b/src/services/restore/downloader.rs @@ -32,7 +32,7 @@ impl RestoreService { let logger_ref = &logger; - retry("Backup download", &logger, &policy, move |_| { + let outcome = retry("Backup download", &logger, &policy, move |_| { let expected = expected_size.clone(); async move { @@ -40,7 +40,13 @@ impl RestoreService { .await } }) - .await + .await; + + if let Err(e) = &outcome { + logger.log("error", format!("Download failed: {e}")); + } + + outcome } pub async fn download_once( diff --git a/src/tests/services/backup_runner_tests.rs b/src/tests/services/backup_runner_tests.rs index 883a4c6..6473e10 100644 --- a/src/tests/services/backup_runner_tests.rs +++ b/src/tests/services/backup_runner_tests.rs @@ -51,8 +51,8 @@ async fn a_failing_backup_is_retried_and_leaves_no_attempt_directory() { assert!( entries .iter() - .any(|e| e.level == "error" && e.message.starts_with("Database backup failed after 3 attempts")), - "expected a single terminal error naming the attempt count" + .any(|e| e.level == "error" && e.message.starts_with("Backup failed:")), + "expected a single terminal error from the runner" ); let leftovers: Vec<_> = std::fs::read_dir(tmp_path) diff --git a/src/tests/utils/retry_tests.rs b/src/tests/utils/retry_tests.rs index fe4279a..e991ca3 100644 --- a/src/tests/utils/retry_tests.rs +++ b/src/tests/utils/retry_tests.rs @@ -88,7 +88,7 @@ async fn retries_until_success_and_logs_each_attempt() { } #[tokio::test] -async fn exhausts_attempts_and_logs_a_single_error() { +async fn exhausts_attempts_and_logs_no_terminal_error() { init_tracing_for_test(); let logger = JobLogger::new(); let calls = AtomicU32::new(0); @@ -105,11 +105,7 @@ async fn exhausts_attempts_and_logs_a_single_error() { let entries = logger.into_entries(); assert_eq!(entries.iter().filter(|e| e.level == "warn").count(), 2); - assert_eq!(entries.iter().filter(|e| e.level == "error").count(), 1); - assert_eq!( - entries.iter().find(|e| e.level == "error").unwrap().message, - "Test op failed after 3 attempts: always" - ); + assert_eq!(entries.iter().filter(|e| e.level == "error").count(), 0); } #[tokio::test] diff --git a/src/utils/retry.rs b/src/utils/retry.rs index c4df2c6..b2c19f4 100644 --- a/src/utils/retry.rs +++ b/src/utils/retry.rs @@ -68,7 +68,6 @@ where attempt += 1; } Err(e) => { - logger.log("error", format!("{op} failed after {total} attempts: {e}")); return Err(e); } }