Merge pull request #347 from beyondessential/backup-maintenance-catchup

fix(backup): catch-up maintenance scheduling instead of knife-edge slots

Author: passcod

crates/commons-servers/src/backup_jobs.rs

@@ -443,6 +443,58 @@ pub fn slot_is_due(
 	secs_into_window >= slot && secs_into_window < slot + tick_secs
 }
 
+/// The number of seconds a per-group deadline is held clear of its window's
+/// tail, so a run that starts a couple of ticks after its target still records
+/// within the same window (see [`slot_deadline_due`]).
+const DEADLINE_END_GUARD: i64 = 120;
+
+/// Deadline-with-catch-up scheduling for periodic per-group work — use this
+/// instead of [`slot_is_due`] when *missing* the one-tick slot must not skip
+/// the whole period.
+///
+/// [`slot_is_due`] fires only on the single tick that lands inside a 60s slot;
+/// if that tick is missed (the per-minute loop drifts and its ticks are spaced
+/// slightly over a minute, the group is mid-op, or the process restarted across
+/// that minute) the work is deferred a whole `window`. For weekly work that
+/// means it can silently never run.
+///
+/// This instead treats the group's [`jitter_slot`] offset as a *deadline*
+/// within each epoch-anchored `window` and reports due once `now` has reached
+/// it — staying due on later ticks until a run actually happens. Concretely, due
+/// when either:
+///   - `now` has reached this window's target and the group hasn't run yet this
+///     window (`last` predates the window start), or
+///   - the last run is overdue by a full `window` (prompt catch-up after
+///     downtime, regardless of the slot).
+///
+/// `last` is the most recent run of this kind; `None` (never run) is due as soon
+/// as the current window's target passes. The target is held
+/// [`DEADLINE_END_GUARD`] seconds before the window boundary so a run caught a
+/// tick or two late still records within the window (otherwise a group whose
+/// offset landed in the final seconds would look un-run next window and slip to
+/// every-other-period). Still spreads the fleet: each group's target differs by
+/// its hash offset.
+pub fn slot_deadline_due(
+	group_id: Uuid,
+	window: Duration,
+	last: Option<Timestamp>,
+	now: Timestamp,
+) -> bool {
+	let window_secs = window.as_secs().max(1) as i64;
+	let offset = (jitter_slot(group_id, window).as_secs() as i64)
+		.min((window_secs - DEADLINE_END_GUARD).max(0));
+	let now_s = now.as_second();
+	let window_start = now_s - now_s.rem_euclid(window_secs);
+	let target = window_start + offset;
+	match last {
+		None => now_s >= target,
+		Some(last) => {
+			let last = last.as_second();
+			(now_s >= target && last < window_start) || now_s - last >= window_secs
+		}
+	}
+}
+
 #[cfg(test)]
 mod tests {
 	use super::*;
@@ -479,6 +531,69 @@ mod tests {
 		}
 	}
 
+	#[test]
+	fn slot_deadline_fires_once_per_window_and_catches_up() {
+		let g = Uuid::from_u128(0x9e37_79b9_7f4a_7c15_f39c_c060_5ced_c834);
+		let window = Duration::from_secs(86400); // DAY
+		let window_secs = 86400i64;
+		let ts = |s: i64| Timestamp::from_second(s).unwrap();
+
+		// The effective (guarded) offset the function uses, and the room left to
+		// the window's end — used to keep every fixture instant inside the window
+		// regardless of where this group's hash lands.
+		let offset =
+			(jitter_slot(g, window).as_secs() as i64).min(window_secs - DEADLINE_END_GUARD);
+		assert!(offset >= 1, "fixture assumes a non-zero slot offset");
+		let room = window_secs - offset;
+		// A concrete window anchored to a DAY boundary (100 days after the epoch).
+		let window_start = 100 * window_secs;
+		let target = window_start + offset;
+
+		// Never run: not due before the target, due once it's reached.
+		assert!(!slot_deadline_due(g, window, None, ts(target - 1)));
+		assert!(slot_deadline_due(g, window, None, ts(target)));
+
+		// Ran earlier this same window → not due again this window (once-per-window,
+		// no double run even if the earlier run predated the slot).
+		assert!(!slot_deadline_due(
+			g,
+			window,
+			Some(ts(window_start + 5)),
+			ts(target + 1)
+		));
+
+		// Ran in the previous window → due once this window's target passes (the
+		// daily catch-up: yesterday's run doesn't block today's), but not before.
+		let prev = window_start - window_secs + offset;
+		assert!(!slot_deadline_due(
+			g,
+			window,
+			Some(ts(prev)),
+			ts(target - 1)
+		));
+		assert!(slot_deadline_due(g, window, Some(ts(prev)), ts(target)));
+
+		// The slot was missed for this window (no tick landed on it), but a later
+		// tick still fires it — the whole point, vs slot_is_due deferring a period.
+		let late = (room / 2).max(1);
+		assert!(slot_deadline_due(
+			g,
+			window,
+			Some(ts(prev)),
+			ts(target + late)
+		));
+
+		// Overdue by more than a full window fires regardless of the slot, even
+		// early in the window before the target (downtime recovery).
+		let stale = window_start - window_secs - 10;
+		assert!(slot_deadline_due(
+			g,
+			window,
+			Some(ts(stale)),
+			ts(window_start + 1)
+		));
+	}
+
 	#[test]
 	fn retention_floor_raises_below_keeps_above() {
 		let r = RetentionPolicy {

crates/jobs/src/backup/inspection.rs

@@ -23,7 +23,7 @@
 
 use std::time::Duration;
 
-use commons_servers::backup_jobs::{effective_interval_for_group, slot_is_due};
+use commons_servers::backup_jobs::{effective_interval_for_group, slot_deadline_due};
 use database::{
 	BackupConfigStatus, BackupMaintenanceRun, BackupRepoSnapshot, BackupRun,
 	ServerGroupBackupConfig,
@@ -46,11 +46,6 @@ const TICK: Duration = Duration::from_secs(60);
 /// a group's effective backup interval is longer (or absent).
 const INSPECT_FLOOR: Duration = Duration::from_secs(7 * 24 * 3600);
 
-fn secs_into(now: Timestamp, window: Duration) -> u64 {
-	let w = window.as_secs().max(1) as i64;
-	now.as_second().rem_euclid(w) as u64
-}
-
 /// Whether a backup has landed since the repo was last inspected — the signal to
 /// inspect promptly (so the stats panel freshens shortly after a backup,
 /// including a manual "back up now") instead of waiting for the weekly cadence.
@@ -182,8 +177,13 @@ async fn tick(worker: &Worker) -> Result<(), String> {
 			.await
 			.map_err(|e| e.to_string())?
 			.map_or(INSPECT_FLOOR, |i| i.max(INSPECT_FLOOR));
-		let into = secs_into(now, window);
-		if !due_after_backup && !due_after_maint && !slot_is_due(c.group_id, window, TICK, into) {
+		// Fallback cadence: fire on the first tick at/after the group's jittered
+		// deadline and keep firing until an inspection lands (catch-up), rather
+		// than only in a single 60s slot that a drifting/slow tick can skip.
+		if !due_after_backup
+			&& !due_after_maint
+			&& !slot_deadline_due(c.group_id, window, last_inspected, now)
+		{
 			continue;
 		}
 		spawn_inspect(worker, c.clone());

crates/jobs/src/backup/maintenance.rs

@@ -19,8 +19,8 @@
 use std::{collections::HashSet, time::Duration};
 
 use commons_servers::backup_jobs::{
-	JobKind, SharedBackupConfig, backup_bucket_billing_tags, effective_retention_for_group, is_due,
-	slot_is_due,
+	JobKind, SharedBackupConfig, backup_bucket_billing_tags, effective_retention_for_group,
+	slot_deadline_due,
 };
 use commons_types::backup::BackupPlacement;
 use database::{
@@ -45,11 +45,6 @@ const TICK: Duration = Duration::from_secs(60);
 const DAY: Duration = Duration::from_secs(24 * 3600);
 const WEEK: Duration = Duration::from_secs(7 * 24 * 3600);
 
-fn secs_into(now: Timestamp, window: Duration) -> u64 {
-	let w = window.as_secs().max(1) as i64;
-	now.as_second().rem_euclid(w) as u64
-}
-
 /// Build the `{type → policy}` retention map for a group: the effective
 /// `RetentionPolicy` per enabled backup type, serialised through JSON into the
 /// kopia layer's [`RetentionMap`]. Empty when the group has no enabled types
@@ -88,20 +83,22 @@ fn kopia_env(
 }
 
 /// Decide which maintenance kind (if any) a group is due for this tick.
+///
+/// Full (weekly) takes priority over quick (daily) and subsumes it. Both use
+/// [`slot_deadline_due`], which fires on the first tick at or after the group's
+/// jittered per-period deadline and keeps firing until a run happens — so a
+/// missed slot no longer defers the work a whole period (the reason full
+/// maintenance could go weeks without running and quick slipped past daily).
 fn due_kind(
 	group_id: uuid::Uuid,
 	last_quick_or_full: Option<Timestamp>,
 	last_full: Option<Timestamp>,
 	now: Timestamp,
 ) -> Option<JobKind> {
-	let full_due =
-		is_due(WEEK, last_full, now) && slot_is_due(group_id, WEEK, TICK, secs_into(now, WEEK));
-	if full_due {
+	if slot_deadline_due(group_id, WEEK, last_full, now) {
 		return Some(JobKind::MaintFull);
 	}
-	let quick_due = is_due(DAY, last_quick_or_full, now)
-		&& slot_is_due(group_id, DAY, TICK, secs_into(now, DAY));
-	quick_due.then_some(JobKind::MaintQuick)
+	slot_deadline_due(group_id, DAY, last_quick_or_full, now).then_some(JobKind::MaintQuick)
 }
 
 /// Whether a config needs its init (repo-create) op run this tick. True iff the
@@ -542,16 +539,59 @@ mod tests {
 
 	#[test]
 	fn maintenance_due_logic() {
+		use commons_servers::backup_jobs::jitter_slot;
 		let g = uuid::Uuid::from_u128(3);
-		// Recent full + quick → nothing due (elapsed gate), independent of slot.
-		let now: Timestamp = "2026-06-16T12:00:00Z".parse().unwrap();
-		let recent: Timestamp = "2026-06-16T11:00:00Z".parse().unwrap();
-		assert_eq!(due_kind(g, Some(recent), Some(recent), now), None);
-
-		// At the group's weekly slot with no prior full run, full is due (and
-		// subsumes quick).
-		let week_slot = commons_servers::backup_jobs::jitter_slot(g, WEEK).as_secs() as i64;
-		let at_slot = Timestamp::from_second(week_slot).unwrap();
-		assert_eq!(due_kind(g, None, None, at_slot), Some(JobKind::MaintFull));
+		let week_secs = WEEK.as_secs() as i64;
+		let day_secs = DAY.as_secs() as i64;
+		let week_off = jitter_slot(g, WEEK).as_secs() as i64;
+		let day_off = jitter_slot(g, DAY).as_secs() as i64;
+		// Fixture sanity: this group's slots aren't in the guarded tail, so the
+		// targets derived below match what the scheduler computes.
+		assert!(week_off < week_secs - 120 && day_off < day_secs - 120);
+		let ts = |s: i64| Timestamp::from_second(s).unwrap();
+
+		// A WEEK boundary well after the epoch (also a DAY boundary: WEEK = 7·DAY).
+		let week_start = 3000 * week_secs;
+		let full_target = week_start + week_off;
+
+		// Just ran both → nothing due.
+		let now0 = ts(week_start + week_off + 12345);
+		assert_eq!(due_kind(g, Some(now0), Some(now0), now0), None);
+
+		// Never run, at the weekly deadline → full (which subsumes quick).
+		assert_eq!(
+			due_kind(g, None, None, ts(full_target)),
+			Some(JobKind::MaintFull)
+		);
+
+		// Full ran last week and this week's slot was missed (no tick landed on
+		// it); a later tick still catches it up rather than deferring another
+		// week — the bug this fix targets.
+		let late = ((week_secs - week_off) / 2).max(1);
+		let last_week_full = full_target - week_secs;
+		assert_eq!(
+			due_kind(
+				g,
+				Some(ts(last_week_full + 100)),
+				Some(ts(last_week_full)),
+				ts(full_target + late),
+			),
+			Some(JobKind::MaintFull),
+		);
+
+		// Full is current (ran yesterday, still this week), but quick's deadline
+		// for today has passed since → quick is due (and full isn't).
+		let day_start = week_start + 3 * day_secs; // mid-week day boundary
+		let full_ran_yesterday = day_start - day_secs + 500;
+		let now_q = ts(day_start + day_off + 10);
+		assert_eq!(
+			due_kind(
+				g,
+				Some(ts(full_ran_yesterday)),
+				Some(ts(full_ran_yesterday)),
+				now_q,
+			),
+			Some(JobKind::MaintQuick),
+		);
 	}
 }

crates/jobs/src/backup/preflight.rs

@@ -16,12 +16,12 @@
 //! bypasses per-server `is_monitored`. AWS calls are not unit-tested here (no
 //! live AWS in CI); the pure logic — the Object-Lock assertion — is.
 
-use std::time::Duration;
+use std::{collections::HashMap, time::Duration};
 
 use aws_sdk_s3::types::ObjectLockRetentionMode;
 use aws_sdk_sts::error::{ProvideErrorMetadata, SdkError};
 use aws_sdk_sts::operation::RequestId;
-use commons_servers::backup_jobs::slot_is_due;
+use commons_servers::backup_jobs::slot_deadline_due;
 use commons_types::issue::Severity;
 use database::{
 	BackupConfigStatus, ServerGroupBackupConfig,
@@ -33,6 +33,7 @@ use tokio::{
 	time::sleep,
 };
 use tracing::{debug, error, warn};
+use uuid::Uuid;
 
 const TICK: Duration = Duration::from_secs(60);
 const DEEP_WINDOW: Duration = Duration::from_secs(3600);
@@ -102,13 +103,6 @@ async fn recover_identity_alert(
 	Ok(())
 }
 
-/// Seconds elapsed into the current `window` for `now` — used with
-/// [`slot_is_due`] to fire a group's hourly deep check on the right minute tick.
-fn secs_into_window(now: Timestamp, window: Duration) -> u64 {
-	let w = window.as_secs().max(1) as i64;
-	(now.as_second().rem_euclid(w)) as u64
-}
-
 struct Aws {
 	sts: aws_sdk_sts::Client,
 	config: aws_config::SdkConfig,
@@ -298,6 +292,11 @@ pub fn spawn() -> JoinHandle<()> {
 			sts: aws_sdk_sts::Client::new(&config),
 			config,
 		};
+		// In-memory per-group anchor for the hourly deep check (no persisted
+		// last-check timestamp): lets a missed slot catch up on a later tick and
+		// keeps a slow predecessor group from pushing others past their slot.
+		// Resets on restart, re-firing at the next deadline.
+		let mut last_deep: HashMap<Uuid, Timestamp> = HashMap::new();
 
 		loop {
 			sleep(TICK).await;
@@ -344,11 +343,17 @@ pub fn spawn() -> JoinHandle<()> {
 				continue;
 			}
 
-			// Per-group deep checks on their hash-jittered hourly slot.
+			// Per-group deep checks on their hash-jittered hourly deadline, with
+			// catch-up on a later tick if this one drifts past the slot.
 			let now = Timestamp::now();
-			let into = secs_into_window(now, DEEP_WINDOW);
 			for cfg in &ready {
-				if slot_is_due(cfg.group_id, DEEP_WINDOW, TICK, into) {
+				if slot_deadline_due(
+					cfg.group_id,
+					DEEP_WINDOW,
+					last_deep.get(&cfg.group_id).copied(),
+					now,
+				) {
+					last_deep.insert(cfg.group_id, now);
 					debug!(group = %cfg.group_id, "running hourly preflight deep check");
 					deep_check_group(&mut db, &aws, cfg).await;
 				}