From ec476dfaae8351cb5c3b7b6ba76985bbf97dc675 Mon Sep 17 00:00:00 2001 From: atlas Date: Mon, 10 Aug 2026 22:35:02 +0200 Subject: [PATCH] feat(#3139): bound the agent container start limit and back off between retries container@.service sets Restart=on-failure and no start limit, so systemd's defaults applied: 5 starts per 10s with RestartSec 100ms. That made the bound a function of how fast a container dies -- one failing instantly tripped the limit in under a second, one taking longer than ~2s never tripped it and restarted forever. The per-container drop-in already carried both sections this needs, so the bound goes there rather than into a second file. StartLimit* are [Unit] settings; systemd silently ignores them under [Service], which the pinned body test now guards. The values are hive-wide constants rather than wire fields: the bound is policy, identical for every agent, and threading it through the socket as a per-agent parameter would be plumbing for a value nobody varies. Giving up is safe to make tight because it is not terminal -- reconcile retries later, and the reset-failed already done before each start clears the latch first. --- hive-priv/src/main.rs | 87 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 9 deletions(-) diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 2b819c7c..41b425fe 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -741,14 +741,44 @@ fn write_resource_limits( Ok((String::new(), String::new())) } +/// How long a window the start-limit counts over, and how many starts it +/// allows inside it. +/// +/// `container@.service` sets `Restart=on-failure` and **no** start limit, so +/// systemd's defaults apply: 5 starts per 10s, `RestartSec` 100ms. That +/// makes the bound depend on *how fast* a container dies — one that fails +/// instantly trips the limit in under a second, one that takes longer than +/// ~2s never trips it and restarts forever. Whether an agent gets bounded +/// is not meant to be a function of its failure speed. +/// +/// The window has to exceed the worst-case time to burn the burst, or the +/// counter ages out between attempts and the limit is again unreachable: +/// `TimeoutStartSec` is 1min, so `BURST` slow failures plus their backoff +/// can span several minutes. 10min covers that with room. +/// +/// Giving up is cheap here **because it is not terminal** — hive-c0re's +/// reconcile sweep retries later, and `reset-failed` (see `StartContainer`) +/// clears the latch first. That is what makes a tight burst safe. +const START_LIMIT_INTERVAL_SEC: u32 = 600; +/// One start plus two retries — the operator's ruling was "retry once or +/// twice", with the reconcile sweep as the slow path after that. +const START_LIMIT_BURST: u32 = 3; +/// Backoff between those retries. The 100ms default is for processes that +/// respawn instantly; a container that just failed to boot gains nothing +/// from being retried a tenth of a second later. +const RESTART_SEC: u32 = 5; + /// Render the body of `hyperhive-limits.conf`. /// /// `[Unit]`: the condition is checked at start time — it skips (not fails) -/// the unit when the MCP socket dir is absent, avoiding restart loops. -/// `[Service]`: the hard caps first, then the relative weights. A weight of -/// `None` means "not configured" and omits its line entirely, so a request -/// from a hive-c0re built before the weights existed — or one whose nix -/// option is `null` — reproduces the old two-setting drop-in byte for byte. +/// the unit when the MCP socket dir is absent, avoiding restart loops — +/// plus the bounded start limit (see the constants above; `StartLimit*` are +/// `[Unit]` settings since systemd 229 and are silently ignored under +/// `[Service]`). +/// `[Service]`: the restart backoff, the hard caps, then the relative +/// weights. A weight of `None` means "not configured" and omits its line +/// entirely, so a request from a hive-c0re built before the weights +/// existed — or one whose nix option is `null` — renders no weight lines. fn limits_dropin_body( runtime_dir: &str, memory_max: &str, @@ -764,8 +794,11 @@ fn limits_dropin_body( format!( "[Unit]\n\ ConditionPathIsDirectory={runtime_dir}\n\ + StartLimitIntervalSec={START_LIMIT_INTERVAL_SEC}\n\ + StartLimitBurst={START_LIMIT_BURST}\n\ \n\ [Service]\n\ + RestartSec={RESTART_SEC}\n\ MemoryMax={memory_max}\n\ CPUQuota={cpu_quota}\n\ {cpu_weight_line}{io_weight_line}" @@ -2600,22 +2633,58 @@ mod tests { .expect("every ordinary request arrives without a descriptor"); } - /// An unset weight is "not configured": the drop-in must come out - /// byte-identical to the pre-weights two-setting body, so a hive-c0re - /// older than this field can't change what lands on disk. + /// The whole rendered body, pinned literally — including the values, + /// so changing the restart policy is a visible test edit rather than a + /// silent one. + /// + /// Placement is the part most worth pinning: `StartLimit*` are `[Unit]` + /// settings and systemd **silently ignores** them under `[Service]`, so + /// a bound that moved sections would look configured and do nothing. + /// An unset weight still emits no line at all, which is what keeps a + /// hive-c0re older than that field from changing what lands on disk. #[test] - fn unset_weights_reproduce_the_pre_weights_dropin() { + fn dropin_body_is_pinned_exactly() { assert_eq!( limits_dropin_body("/run/hyperhive/agents/iris", "4G", "200%", None, None), "[Unit]\n\ ConditionPathIsDirectory=/run/hyperhive/agents/iris\n\ + StartLimitIntervalSec=600\n\ + StartLimitBurst=3\n\ \n\ [Service]\n\ + RestartSec=5\n\ MemoryMax=4G\n\ CPUQuota=200%\n" ); } + /// The bound is hive-wide **policy**, not a per-agent parameter: it is + /// rendered from constants and no caller-supplied value can omit or + /// alter it. This is the property that justifies keeping it out of the + /// wire protocol — if it ever varies by request, that argument is gone. + #[test] + fn the_start_limit_is_present_whatever_the_caller_passes() { + for (mem, cpu, cw, iw) in [ + ("4G", "200%", None, None), + ("512M", "50%", Some(10), Some(10)), + ("infinity", "infinity", Some(10_000), None), + ] { + let body = limits_dropin_body("/rt/x", mem, cpu, cw, iw); + let unit = body + .split("[Service]") + .next() + .expect("the body always has a [Unit] section before [Service]"); + assert!( + unit.contains("StartLimitIntervalSec=600") && unit.contains("StartLimitBurst=3"), + "start limit missing from [Unit] for ({mem}, {cpu}): {body}" + ); + assert!( + body.contains("\nRestartSec=5\n"), + "restart backoff missing for ({mem}, {cpu}): {body}" + ); + } + } + /// Weights are appended to the `[Service]` section, each omitted /// independently when `None`. #[test]