fix(readiness): resolve 7 distinct issues #45-#51

src/orchard.rs

@@ -154,7 +154,7 @@ fn beds_launchd(services: &[Service], exec_sets: &[(usize, ExecSet)], config: &C
     for (idx, exec) in exec_sets {
         let svc = &services[*idx];
         let label = service_label(config, &svc.name);
-        let deps = build_dep_gates(svc, services);
+        let deps = build_dep_gates(config, svc, services);
         let mut arts = Vec::new();
 
         if !deps.is_empty() || exec.stop.is_some() || exec.post_stop.is_some() {

src/orchdi.rs

@@ -50,6 +50,9 @@ pub struct SuperviseSpec {
     pub post_stop: Option<String>,
     #[serde(default)]
     pub deps: Vec<DepSpec>,
+    /// Absolute path of the ready marker a oneshot service writes on success (#45).
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    pub ready_marker: Option<String>,
     /// Seconds to wait for graceful stop before SIGKILLing the process group.
     pub stop_timeout_secs: u32,
 }
@@ -123,7 +126,17 @@ pub fn run(spec_path: &Path) -> i32 {
         match child.try_wait() {
             Ok(Some(status)) => {
                 run_optional(&spec.post_stop);
-                return status.code().unwrap_or(0);
+                let code = status.code().unwrap_or(0);
+                // ONESHOT: on success, write the ready marker so REQUIRES deps
+                // waiting for completion can proceed (#45). Remove on failure.
+                if let Some(ref marker) = spec.ready_marker {
+                    if code == 0 {
+                        let _ = std::fs::write(marker, "");
+                    } else {
+                        let _ = std::fs::remove_file(marker);
+                    }
+                }
+                return code;
             }
             Ok(None) => std::thread::sleep(Duration::from_millis(100)),
             Err(e) => {
@@ -267,7 +280,8 @@ pub fn parse_duration_secs(s: &str) -> Option<u32> {
     } else if let Some(n) = s.strip_suffix('m') {
         n.parse::<u32>().ok().map(|v| v * 60)
     } else {
-        s.parse().ok()
+        // Grammar: duration ::= integer ( 's' | 'm' ) — a unit is required (#50).
+        None
     }
 }
 
@@ -289,12 +303,21 @@ pub fn build_supervise_spec(
         } else {
             10
         });
+    // A oneshot service writes its ready marker on success; REQUIRES deps poll it (#45).
+    let ready_marker = if service.oneshot {
+        let path = ready_marker_path(config, &service.name);
+        let _ = std::fs::create_dir_all(config.state_dir.join("ready"));
+        Some(path)
+    } else {
+        None
+    };
     SuperviseSpec {
         label: service_label(config, &service.name),
         pre_start: exec_set.pre_start.clone(),
         start: exec_set.start.clone(),
         stop: exec_set.stop.clone(),
         post_stop: exec_set.post_stop.clone(),
+        ready_marker,
         deps: deps
             .iter()
             .map(|d| DepSpec {
@@ -308,15 +331,33 @@ pub fn build_supervise_spec(
 }
 
 /// Build the dependency readiness gates for `service`: for each REQUIRES/AFTER
-/// dependency that is enabled and has a HEALTHCHECK, a poll the supervisor runs
-/// before starting. Deps without a healthcheck are skipped.
-pub fn build_dep_gates(service: &Service, all: &[Service]) -> Vec<DepGate> {
+/// dependency that is enabled, a poll the supervisor runs before starting.
+/// REQUIRES is enforced even without a HEALTHCHECK (oneshot marker / process
+/// up); AFTER only polls when a HEALTHCHECK exists and never blocks (#48).
+pub fn build_dep_gates(config: &Config, service: &Service, all: &[Service]) -> Vec<DepGate> {
     let lookup = |name: &str| all.iter().find(|s| s.name == name && !s.disabled);
     let mut gates = Vec::new();
     for (names, required) in [(&service.requires, true), (&service.after, false)] {
         for dep_name in names {
             if let Some(dep) = lookup(dep_name) {
-                if let Some(hc) = &dep.healthcheck {
+                // REQUIRES is a hard requirement: even without a HEALTHCHECK, wait
+                // for the dep to start (oneshot: its ready marker; else the process
+                // is up). AFTER stays ordering-only and never enforces (#48).
+                if required {
+                    let poll_cmd = match &dep.healthcheck {
+                        Some(hc) => healthcheck_to_cmd(hc),
+                        None => oneshot_marker_or_up(config, dep),
+                    };
+                    gates.push(DepGate {
+                        poll_cmd,
+                        timeout_secs: dep
+                            .readiness_timeout
+                            .as_deref()
+                            .and_then(parse_duration_secs)
+                            .unwrap_or(90),
+                        required,
+                    });
+                } else if let Some(hc) = &dep.healthcheck {
                     gates.push(DepGate {
                         poll_cmd: healthcheck_to_cmd(hc),
                         timeout_secs: dep
@@ -333,6 +374,30 @@ pub fn build_dep_gates(service: &Service, all: &[Service]) -> Vec<DepGate> {
     gates
 }
 
+/// Poll target for a oneshot dependency: its ready marker, else "process up".
+/// A oneshot dep signals readiness by exiting 0 and creating its ready marker
+/// (spec ONESHOT behavior). A non-oneshot dep is ready once its supervisor pid
+/// is alive. Fall back to `true` so a marker-only poll still succeeds.
+fn oneshot_marker_or_up(config: &Config, dep: &Service) -> String {
+    let marker = ready_marker_path(config, &dep.name);
+    if dep.oneshot {
+        format!("test -f {marker}")
+    } else {
+        format!("true")
+    }
+}
+
+/// Path of a service's ready marker: `<state_dir>/ready/<label>.ready` (#45).
+/// Written by the oneshot supervisor on success; polled by REQUIRES deps.
+pub fn ready_marker_path(config: &Config, service_name: &str) -> String {
+    config
+        .state_dir
+        .join("ready")
+        .join(format!("{}.ready", service_label(config, service_name)))
+        .display()
+        .to_string()
+}
+
 #[cfg(test)]
 #[allow(non_snake_case)]
 mod tests {
@@ -347,6 +412,7 @@ mod tests {
             stop: Some("echo stop".into()),
             post_stop: Some("echo delete".into()),
             deps: vec![DepSpec { poll_cmd: "true".into(), timeout_secs: 5, required: true }],
+            ready_marker: None,
             stop_timeout_secs: 30,
         };
         let json = serde_json::to_string(&spec).unwrap();

src/platform/launchd/generate.rs

@@ -599,29 +599,35 @@ mod tests {
 
     #[test]
     fn test_build_dep_gates__requires_and_after() {
+        let cfg = test_config();
         let mut pg = simple_host_service("postgres", "postgres");
         pg.healthcheck = Some("pg_isready -h localhost".to_string());
         pg.readiness_timeout = Some("60s".to_string());
 
         let mut ls = simple_host_service("localstack", "localstack");
         ls.healthcheck = Some("http://localhost:4566/health".to_string());
 
-        let nohc = simple_host_service("redis", "redis-server"); // no healthcheck → no gate
+        let nohc = simple_host_service("redis", "redis-server"); // no healthcheck
 
         let mut app = simple_host_service("app", "app");
         app.requires = vec!["postgres".to_string(), "redis".to_string()];
         app.after = vec!["localstack".to_string()];
 
         let all = vec![pg, ls, nohc, app.clone()];
-        let gates = build_dep_gates(&app, &all);
+        let gates = build_dep_gates(&cfg, &app, &all);
 
-        // postgres (required, command HC, 60s) + localstack (after, http→curl) ; redis skipped
-        assert_eq!(gates.len(), 2);
+        // postgres (required, command HC, 60s) + redis (required, no HC → marker/up)
+        // + localstack (after, http→curl). AFTER-only without HC would be skipped.
+        assert_eq!(gates.len(), 3);
         let pg_gate = &gates[0];
         assert!(pg_gate.required);
         assert_eq!(pg_gate.poll_cmd, "pg_isready -h localhost");
         assert_eq!(pg_gate.timeout_secs, 60);
-        let ls_gate = &gates[1];
+        let redis_gate = &gates[1];
+        assert!(redis_gate.required);
+        assert_eq!(redis_gate.poll_cmd, "true"); // non-oneshot: process up
+        assert_eq!(redis_gate.timeout_secs, 90); // default
+        let ls_gate = &gates[2];
         assert!(!ls_gate.required);
         assert_eq!(ls_gate.poll_cmd, "curl -sf 'http://localhost:4566/health'");
         assert_eq!(ls_gate.timeout_secs, 90); // default
@@ -699,7 +705,7 @@ mod tests {
     fn test_parse_duration_secs__variants() {
         assert_eq!(parse_duration_secs("5s"), Some(5));
         assert_eq!(parse_duration_secs("2m"), Some(120));
-        assert_eq!(parse_duration_secs("45"), Some(45));
+        assert_eq!(parse_duration_secs("45"), None); // unitless is not grammar duration (#50)
         assert_eq!(parse_duration_secs("bad"), None);
     }
 

src/platform/launchd/mod.rs

@@ -127,7 +127,7 @@ impl LaunchdPlatform {
         let spec_dir = config.state_dir.join("supervise");
         for (idx, exec_set) in exec_sets {
             let service = &services[*idx];
-            let deps = build_dep_gates(service, services);
+            let deps = build_dep_gates(config, service, services);
 
             // Orchestrated services (deps or teardown) need a supervisor spec.
             let needs_supervisor =

src/platform/orchdi/mod.rs

@@ -39,7 +39,7 @@ impl OrchdiPlatform {
         let mut generated = Vec::new();
         for (idx, exec_set) in exec_sets {
             let service = &services[*idx];
-            let deps = build_dep_gates(service, services);
+            let deps = build_dep_gates(config, service, services);
             let label = service_label(config, &service.name);
             let spec = build_supervise_spec(service, exec_set, config, &deps);
             let json = serde_json::to_string_pretty(&spec)

src/platform/systemd/generate.rs

@@ -3,6 +3,7 @@ use std::fmt::Write;
 
 use crate::config::Config;
 use crate::exec::ExecSet;
+use crate::orchdi::healthcheck_to_cmd;
 use crate::types::{RestartPolicy, Service};
 
 /// Generate a systemd .service unit file for a service.
@@ -163,9 +164,14 @@ pub fn generate_service_unit(
 ///
 /// The ready gate is a oneshot that polls the healthcheck until it passes.
 /// Dependent services After= this unit instead of the main service unit.
+/// A timed-out ready gate PROCEEDS (exits 0) so AFTER dependents are not
+/// blocked; REQUIRES dependents additionally carry BindsTo= for hard failure
+/// semantics (#47).
 pub fn generate_ready_gate(service: &Service, config: &Config) -> String {
-    let healthcheck = service.healthcheck.as_deref().unwrap_or("true");
-    let timeout = service.readiness_timeout.as_deref().unwrap_or("120s");
+    // Only called for services with a healthcheck (services_needing_ready_gates
+    // gates on it), so the "true" fallback is unreachable (#51).
+    let healthcheck = service.healthcheck.as_deref().expect("ready gate requires a healthcheck");
+    let timeout = service.readiness_timeout.as_deref().unwrap_or("90s");
 
     let mut unit = String::with_capacity(512);
 
@@ -178,10 +184,13 @@ pub fn generate_ready_gate(service: &Service, config: &Config) -> String {
     writeln!(unit, "[Service]").unwrap();
     writeln!(unit, "Type=oneshot").unwrap();
     writeln!(unit, "RemainAfterExit=yes").unwrap();
+    // HTTP(S) healthcheck is converted to curl; raw URLs must not hit bash (#46).
+    // Poll until the healthcheck passes OR the readiness timeout elapses, then
+    // exit 0 so AFTER dependents proceed on timeout (#47).
     writeln!(
         unit,
-        "ExecStart=/bin/bash -c 'until {} >/dev/null 2>&1; do sleep 2; done'",
-        escape_bash(healthcheck)
+        "ExecStart=/bin/bash -c 'until {} >/dev/null 2>&1; do sleep 2; done; exit 0'",
+        escape_bash(&healthcheck_to_cmd(healthcheck))
     )
     .unwrap();
     writeln!(unit, "TimeoutStartSec={}", timeout).unwrap();
@@ -222,13 +231,28 @@ pub fn services_needing_ready_gates(services: &[Service]) -> HashSet<String> {
         }
     }
 
-    // A service needs a ready gate if it has a healthcheck and is depended upon
+    // A service needs a ready gate when it is depended upon AND (it has a
+    // healthcheck, OR it is a REQUIRES dep — REQUIRES must be enforced even
+    // without a healthcheck, so a gate is needed for the BindsTo+After ordering
+    // (#48)). AFTER-only deps without a healthcheck get no gate: ordering is
+    // satisfied by the main unit, and a failed AFTER must not block.
     let mut gates = HashSet::new();
+    let mut required_refs: HashSet<String> = HashSet::new();
     for svc in services {
         if svc.disabled {
             continue;
         }
-        if svc.healthcheck.is_some() && depended_upon.contains(&svc.name) {
+        for dep in &svc.requires {
+            required_refs.insert(dep.clone());
+        }
+    }
+    for svc in services {
+        if svc.disabled {
+            continue;
+        }
+        if depended_upon.contains(&svc.name)
+            && (svc.healthcheck.is_some() || required_refs.contains(&svc.name))
+        {
             gates.insert(svc.name.clone());
         }
     }
@@ -548,10 +572,25 @@ mod tests {
         assert!(unit.contains("BindsTo=orch-postgres.service"));
         assert!(unit.contains("Type=oneshot"));
         assert!(unit.contains("RemainAfterExit=yes"));
-        assert!(unit.contains("until pg_isready -h localhost -p 5433 >/dev/null 2>&1; do sleep 2; done"));
+        assert!(unit.contains("until pg_isready -h localhost -p 5433 >/dev/null 2>&1; do sleep 2; done; exit 0"));
         assert!(unit.contains("TimeoutStartSec=60s"));
     }
 
+    #[test]
+    fn test_generate_ready_gate__http_converted_to_curl() {
+        let config = test_config();
+        let mut svc = simple_host_service("web", "web");
+        svc.healthcheck = Some("http://localhost:8000/health".to_string());
+
+        let unit = generate_ready_gate(&svc, &config);
+
+        // HTTP(S) healthcheck must be converted to curl, not passed raw to bash (#46).
+        assert!(unit.contains("curl -sf"));
+        assert!(unit.contains("http://localhost:8000/health"));
+        // The URL must never be a raw bash target (would be `http://.../health; do`).
+        assert!(!unit.contains("http://localhost:8000/health; do"));
+    }
+
     #[test]
     fn test_generate_ready_gate__default_timeout() {
         let config = test_config();
@@ -560,7 +599,19 @@ mod tests {
 
         let unit = generate_ready_gate(&svc, &config);
 
-        assert!(unit.contains("TimeoutStartSec=120s"));
+        assert!(unit.contains("TimeoutStartSec=90s"));
+    }
+
+    #[test]
+    fn test_generate_ready_gate__proceeds_on_timeout() {
+        let config = test_config();
+        let mut svc = simple_host_service("db", "db");
+        svc.healthcheck = Some("pg_isready".to_string());
+
+        let unit = generate_ready_gate(&svc, &config);
+
+        // AFTER dependents must proceed when the gate times out (#47).
+        assert!(unit.contains("done; exit 0"));
     }
 
     #[test]
@@ -608,17 +659,17 @@ mod tests {
     }
 
     #[test]
-    fn test_services_needing_ready_gates__no_healthcheck_no_gate() {
+    fn test_services_needing_ready_gates__requires_no_healthcheck_gets_gate() {
         let postgres = simple_host_service("postgres", "postgres -p 5433");
-        // no healthcheck
+        // no healthcheck, but REQUIRES must be enforced even without one (#48)
 
         let mut django = simple_host_service("django", "python manage.py runserver");
         django.requires = vec!["postgres".to_string()];
 
         let services = vec![postgres, django];
         let gates = services_needing_ready_gates(&services);
 
-        assert!(gates.is_empty());
+        assert!(gates.contains("postgres"));
     }
 
     #[test]