fix: bound restart attempts across all strategies (#318)

docs/README.md

@@ -111,19 +111,32 @@ attempts = 0
 
 * **`strategy` = `always|on-failure|never`**: Defines the restart strategy.
 
-    * `always`: Failure or Success, it will be always restarted
-    * `on-failure`: Only if it has failed. Please check the `attempts` parameter below.
-    * `never`: It won't be restarted, no matter what's the exit status. Please check the `attempts` parameter below.
+    * `always`: Failure or Success, it will always be restarted (bounded by `attempts`, see below).
+    * `on-failure`: Only if it has failed (bounded by `attempts`, see below).
+    * `never`: By default it won't be restarted, no matter the exit status. It may still be restarted as a
+      rapid-failure escape hatch if `attempts` is configured (see below).
 
 * **`backoff` = `string`**: Use this time before retrying restarting the service.
-* **`attempts` = `number`**: How many attempts to start the service before considering it as FinishedFailed. Default is
-    10.
-  Attempts are useful if your service is failing too quickly. If you're in a start-stop loop, this will put and end to
-  it.
-  If a service has failed too quickly and attempts > 0, it will be restarted even if the strategy is `never`.
-  And if the attempts are over, it will never be restarted even if the restart policy is: `On-Failure`/`Always`.
-
-The delay between attempts is calculated as: `backoff * attempts_made + start-delay`. For instance, using:
+* **`attempts` = `number`**: A budget that bounds how many times a service can fail *rapidly* (before it
+    becomes stable) before it's considered `FinishedFailed`. Default is `0`, which means **unbounded**
+    (the service is restarted forever according to its `strategy`).
+  Attempts are useful when a service is failing too quickly: if you're in a start-stop loop, this puts an
+    end to it. A failure counts as "too quick" if it happens before the service became stable, i.e. before
+    it reached the `running` state - see `healthiness.healthy-after` below to control how long a service
+    must survive before it's considered stable. Failing to spawn the process at all (for instance, a
+    command that isn't found) also counts against the budget. The budget applies to all three strategies:
+    * For `always` and `on-failure`, `attempts > 0` bounds the rapid-restart loop; `attempts = 0` keeps
+      restarting forever.
+    * For `never`, `attempts > 0` allows the service to be restarted up to that many times *only* when it
+      fails too quickly; with `attempts = 0` a `never` service is never restarted on failure. Once the
+      service has become stable, `never` stops restarting it entirely.
+  In every case, once the budget is exhausted the service is no longer restarted, and it terminates as
+    `FinishedFailed` (or `Finished`, if its last exit was successful).
+
+The delay between attempts is calculated as: `backoff * attempts_made + start-delay`, where `attempts_made`
+is capped at `attempts` (or at 10, when `attempts = 0` means unbounded) so that a service which never
+becomes stable keeps being retried at a steady rate instead of drifting infinitely far apart. For instance,
+using:
 
 * backoff = 1s
 * attempts = 3
@@ -136,10 +149,12 @@ Will wait 1 second and then start the service. If it doesn't start:
 * 3d and last attempt will start after 1*3 +1 = 4 seconds.
 
 If the attempts are over, then the service will be considered FailedFinished and won't be restarted.
-The attempt count is reset as soon as the service's state changes to running.
-This state change is driven by the health-check component, and a service with no health-check will be considered as
-`Healthy` and it will
-immediately pass to the running state.
+The attempt count counts only *rapid* failures (failures that happen before the service becomes stable),
+and it is reset as soon as the service's state changes to running (i.e. it becomes "green").
+This state change is driven by the health-check component: a service with no health-check is considered
+`Healthy` and passes to the running state after `healthiness.healthy-after` (immediately by default, see
+the Healthiness section). Configure `healthy-after` to define how long a crash-prone service must survive
+before its restart budget is reset.
 
 ### Healthiness Check
 
@@ -149,6 +164,7 @@ http-endpoint = "http://localhost:8080/healthcheck"
 file-path = "/var/myservice/up"
 command = "curl -s localhost:8080/healthcheck"
 max-failed = 3
+healthy-after = "0s"
 ```
 
 * **`http-endpoint` = `<http endpoint>`**: It will send an HEAD request to the specified http endpoint. 200 means the
@@ -159,6 +175,11 @@ max-failed = 3
 * **`command` = `your_command arg1 arg2 ...`**: It will run this command. If the exit status is 0, the service is
   considered healthy.
 * **`max-failed` = `i32`**: How many unhealthy health-checks in a row are allowed before considering the service failed.
+* **`healthy-after` = `string`**: How long the process must stay alive before it's considered healthy (and
+  thus stably running, which resets the `restart.attempts` budget). Defaults to `0s`, meaning the service
+  is considered healthy immediately. This is most useful for services without an explicit health-check:
+  raising it lets a crash loop deterministically exhaust its restart budget instead of resetting it on
+  every quick (re)start.
 * You can check the healthiness of your system using a http endpoint or a flag file.
 * You can use the enforce dependency to kill every dependent system.
 

horust/src/horust/formats/service.rs

@@ -312,6 +312,11 @@ pub struct Healthiness {
     #[serde(default = "Healthiness::default_max_failed")]
     // todo: use an u32
     pub max_failed: i32,
+    /// For services without an explicit healthcheck, how long the process must stay
+    /// alive before it's considered healthy (and thus stably running). Defaults to 0s,
+    /// meaning the service is considered healthy immediately.
+    #[serde(default, with = "humantime_serde")]
+    pub healthy_after: Duration,
 }
 
 impl Healthiness {
@@ -331,6 +336,7 @@ impl Default for Healthiness {
             file_path: None,
             command: None,
             max_failed: 3,
+            healthy_after: Duration::from_secs(0),
         }
     }
 }
@@ -990,5 +996,21 @@ max-failed = 5
         assert_eq!(svc.healthiness.max_failed, 5);
     }
 
+    #[test]
+    fn test_healthy_after_defaults_and_parses() {
+        // Defaults to 0s when unspecified.
+        let svc: Service = Service::from_str(r#"command = "test""#).unwrap();
+        assert_eq!(svc.healthiness.healthy_after, Duration::from_secs(0));
+
+        // Parses a humantime duration.
+        let toml_str = r#"
+command = "test"
+[healthiness]
+healthy-after = "5s"
+"#;
+        let svc: Service = Service::from_str(toml_str).unwrap();
+        assert_eq!(svc.healthiness.healthy_after, Duration::from_secs(5));
+    }
+
     use super::LogOutput;
 }

horust/src/horust/healthcheck/mod.rs

@@ -3,7 +3,7 @@
 
 use std::thread;
 use std::thread::JoinHandle;
-use std::time::Duration;
+use std::time::{Duration, Instant};
 
 use crossbeam::channel::{Receiver, RecvTimeoutError, Sender, unbounded};
 
@@ -20,6 +20,8 @@ struct Worker {
     service: Service,
     bus: BusConnector<Event>,
     work_done_notifier: Receiver<()>,
+    /// When the worker was spawned, used to enforce `healthiness.healthy-after`.
+    started_at: Instant,
 }
 
 impl Worker {
@@ -28,22 +30,39 @@ impl Worker {
             service,
             bus,
             work_done_notifier,
+            started_at: Instant::now(),
         }
     }
     pub fn spawn_thread(self) -> JoinHandle<()> {
         thread::spawn(move || self.run())
     }
     fn run(self) {
+        let healthy_after = self.service.healthiness.healthy_after;
+        let poll = Duration::from_millis(1000);
         loop {
+            let elapsed = self.started_at.elapsed();
             let status = check_health(&self.service.healthiness);
-            self.bus.send_event(Event::HealthCheck(
-                self.service.name.clone(),
-                status.clone(),
-            ));
-            match self
-                .work_done_notifier
-                .recv_timeout(Duration::from_millis(1000))
-            {
+            // Suppress the "healthy" signal until the service has been alive for at least
+            // `healthy-after`, so a crash within the window counts against the
+            // restart-attempts budget. We send *nothing* (rather than a synthetic
+            // Unhealthy) during the window: Unhealthy counts accumulate and are never
+            // decremented, which would permanently prevent the service becoming Running.
+            let suppress = status == HealthinessStatus::Healthy && elapsed < healthy_after;
+            if !suppress {
+                self.bus.send_event(Event::HealthCheck(
+                    self.service.name.clone(),
+                    status.clone(),
+                ));
+            }
+            // Inside the window, wake up at the boundary instead of only every `poll`, so
+            // the service turns green close to the configured time.
+            let timeout = if elapsed < healthy_after {
+                poll.min(healthy_after - elapsed)
+                    .max(Duration::from_millis(10))
+            } else {
+                poll
+            };
+            match self.work_done_notifier.recv_timeout(timeout) {
                 Ok(()) | Err(RecvTimeoutError::Disconnected) => break,
                 _ => (),
             };
@@ -84,7 +103,11 @@ fn run(bus: BusConnector<Event>, services: Vec<Service>) {
         match ev {
             Event::StatusChanged(s_name, ServiceStatus::Started) => {
                 let service = get_service(&s_name);
-                if !service.healthiness.has_any_check_defined() {
+                // With no explicit check and no healthy-after delay, the service is healthy
+                // immediately; otherwise a worker enforces them before reporting Healthy.
+                if !service.healthiness.has_any_check_defined()
+                    && service.healthiness.healthy_after.is_zero()
+                {
                     bus.send_event(Event::HealthCheck(s_name, HealthinessStatus::Healthy));
                     continue;
                 }

horust/src/horust/supervisor/mod.rs

@@ -16,7 +16,7 @@ use service_handler::ServiceHandler;
 pub(crate) use signal_handling::init;
 
 use crate::horust::bus::BusConnector;
-use crate::horust::formats::{Event, ExitStatus, Service, ServiceStatus, ShuttingDown};
+use crate::horust::formats::{Event, ExitStatus, Restart, Service, ServiceStatus, ShuttingDown};
 use crate::horust::healthcheck;
 
 mod process_spawner;
@@ -88,6 +88,24 @@ const MAX_PROCESS_REAPS_ITERS: u32 = 20;
 /// PID 1 is reserved for the init process.
 const INIT_PID: unistd::Pid = unistd::Pid::from_raw(1);
 
+/// Caps the restart delay when the attempts budget is unbounded. Matches the number of
+/// attempts Horust used to default to, so the longest delay stays in a familiar range.
+const MAX_BACKOFF_MULTIPLIER: u32 = 10;
+
+/// Delay before respawning a service: it grows with the number of rapid failures, so that
+/// a service failing over and over is retried increasingly slowly.
+/// The multiplier is capped because `restart_attempts` is only reset once the service
+/// becomes stable: with an unbounded budget (`attempts = 0`) it would otherwise grow
+/// forever and push restarts infinitely far apart.
+fn restart_backoff(restart: &Restart, restart_attempts: u32) -> Duration {
+    let cap = if restart.attempts > 0 {
+        restart.attempts
+    } else {
+        MAX_BACKOFF_MULTIPLIER
+    };
+    restart.backoff.mul(restart_attempts.min(cap))
+}
+
 // Spawns and runs this component in a new thread.
 pub fn spawn(bus: BusConnector<Event>, services: Vec<Service>) -> thread::JoinHandle<ExitStatus> {
     thread::spawn(move || Supervisor::new(bus, services).run())
@@ -169,6 +187,10 @@ impl Supervisor {
             Event::Run(service_name) if self.repo.get_sh(&service_name).is_initial() => {
                 let service_handler = self.repo.get_mut_sh(&service_name);
                 service_handler.status = ServiceStatus::Starting;
+                // Health check results from a previous run must not carry over to the new
+                // process, otherwise a service that was healthy before would immediately
+                // be considered green again (skipping its `healthy-after` window).
+                service_handler.healthiness_checks_failed = None;
                 let evs = vec![Event::StatusChanged(service_name, ServiceStatus::Starting)];
 
                 let res = healthcheck::prepare_service(&service_handler.service().healthiness);
@@ -188,11 +210,10 @@ impl Supervisor {
                         Event::ShuttingDownInitiated(ShuttingDown::Gracefully),
                     ];
                 }
-                let backoff = service_handler
-                    .service()
-                    .restart
-                    .backoff
-                    .mul(service_handler.restart_attempts);
+                let backoff = restart_backoff(
+                    &service_handler.service().restart,
+                    service_handler.restart_attempts,
+                );
                 process_spawner::spawn_fork_exec_handler(
                     service_handler.service().clone(),
                     backoff,
@@ -202,6 +223,10 @@ impl Supervisor {
             }
             Event::SpawnFailed(s_name) => {
                 let service_handler = self.repo.get_mut_sh(&s_name);
+                // The process never came to exist, so no ServiceExited will ever arrive to
+                // account for this failure. Count it here, otherwise a service that can
+                // never be spawned (e.g. command not found) would restart forever.
+                service_handler.restart_attempts += 1;
                 service_handler.status = ServiceStatus::Failed;
                 vec![Event::StatusUpdate(s_name, ServiceStatus::Failed)]
             }
@@ -381,3 +406,41 @@ fn kill(sh: &ServiceHandler, signal: Option<signal::Signal>) {
         );
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::time::Duration;
+
+    use super::restart_backoff;
+    use crate::horust::formats::{Restart, RestartStrategy};
+
+    fn restart(attempts: u32, backoff_ms: u64) -> Restart {
+        Restart {
+            strategy: RestartStrategy::OnFailure,
+            backoff: Duration::from_millis(backoff_ms),
+            attempts,
+        }
+    }
+
+    #[test]
+    fn test_restart_backoff_grows_with_attempts() {
+        let r = restart(5, 100);
+        assert_eq!(restart_backoff(&r, 0), Duration::from_millis(0));
+        assert_eq!(restart_backoff(&r, 3), Duration::from_millis(300));
+    }
+
+    #[test]
+    fn test_restart_backoff_is_capped() {
+        // restart_attempts is only reset once a service becomes stable, so with an
+        // unbounded budget it grows forever: the delay must not grow with it.
+        let unbounded = restart(0, 100);
+        assert_eq!(
+            restart_backoff(&unbounded, 10_000),
+            restart_backoff(&unbounded, 10),
+            "an unbounded budget must not push restarts infinitely far apart"
+        );
+        // With a budget the multiplier can never exceed it anyway.
+        let bounded = restart(5, 100);
+        assert_eq!(restart_backoff(&bounded, 99), Duration::from_millis(500));
+    }
+}