From 27bed74d4244dfa54d6b216bb3d0a8d8218931fc Mon Sep 17 00:00:00 2001 From: iris Date: Sat, 19 Sep 2026 01:35:57 +0200 Subject: [PATCH] hive-c0re: accept friendly MemoryMax size spellings, normalize for systemd mara hit this directly: setting an agent's memory cap to "16GB" fails with 'invalid MemoryMax "16GB": expected a size such as "8G"...'. Confirmed directly against a running systemd 260 (systemd-run -p MemoryMax=): systemd's own parser accepts a bare byte count, B, or plus exactly one uppercase K/M/G/T, and rejects both "16GB" (redundant B after a multiplier) and "16g" (lowercase) with "Invalid argument". So the prior rejection of "16GB" matched systemd, but is bad UX for input a human reasonably expects to work. validate_memory_max now returns the value to store (not just an ok/err verdict): it accepts 8gb/8Gb/8GB/8g/8G interchangeably and normalizes all of them to systemd's own 8G form before it is ever persisted or passed downstream. Also fixes the adjacent bug the same investigation turned up: the old validator incorrectly accepted lowercase (8g) even though systemd itself rejects it. Updated both call sites (server.rs, dashboard/lifecycle_ops.rs) to use the normalized return value. New test friendly_size_spellings_normalize_to_systemds_own_form; flipped the old "8GB should be rejected" assertion, which encoded the human-hostile behavior this fixes. --- hive-c0re/src/agent_config/resource_limits.rs | 82 ++++++++++++++++--- hive-c0re/src/dashboard/lifecycle_ops.rs | 14 ++-- hive-c0re/src/server.rs | 12 ++- 3 files changed, 85 insertions(+), 23 deletions(-) diff --git a/hive-c0re/src/agent_config/resource_limits.rs b/hive-c0re/src/agent_config/resource_limits.rs index 46ea47df..46f38a34 100644 --- a/hive-c0re/src/agent_config/resource_limits.rs +++ b/hive-c0re/src/agent_config/resource_limits.rs @@ -260,18 +260,32 @@ pub fn validate_cpu_quota(value: &str) -> Result<(), String> { )) } -/// Validate a systemd `MemoryMax=` value: a byte count with an optional -/// `K`/`M`/`G`/`T` suffix, a percentage of physical memory, or the -/// literal `infinity`. +/// Validate a systemd `MemoryMax=` value — a byte count with an optional +/// `K`/`M`/`G`/`T` suffix, a percentage of physical memory, or the literal +/// `infinity` — and return the exact string to store and pass to systemd. +/// +/// That's not always `value` itself: a size is accepted case-insensitively +/// and with an optional redundant trailing `B` (`8gb`, `8Gb`, `8GB`, `8g`, +/// `8G` are all the same size to a human), but only `8G` is what systemd's +/// own parser actually takes — confirmed directly against a running +/// systemd 260: `MemoryMax=8GB` and `MemoryMax=8g` both fail unit +/// activation with "Invalid argument", while `MemoryMax=8G` starts fine. +/// Accepting the friendly spellings and normalizing them here means every +/// caller downstream only ever sees the one form systemd accepts, instead +/// of everyone re-deriving that systemd is this particular about case and +/// the redundant `B`. /// /// # Errors /// /// Returns a human-readable message naming the offending value and the /// three accepted shapes. Same operator-facing contract as /// [`validate_cpu_quota`]. -pub fn validate_memory_max(value: &str) -> Result<(), String> { - if value == "infinity" || is_percentage(value) || is_byte_size(value) { - return Ok(()); +pub fn validate_memory_max(value: &str) -> Result { + if value == "infinity" || is_percentage(value) { + return Ok(value.to_owned()); + } + if let Some(normalized) = normalized_byte_size(value) { + return Ok(normalized); } Err(format!( "invalid MemoryMax {value:?}: expected a size such as \"8G\", a percentage \ @@ -284,12 +298,32 @@ fn is_percentage(value: &str) -> bool { value.strip_suffix('%').is_some_and(is_plain_number) } -/// A decimal number with an optional single-letter binary suffix. -fn is_byte_size(value: &str) -> bool { - let mantissa = value - .strip_suffix(['K', 'M', 'G', 'T', 'k', 'm', 'g', 't']) - .unwrap_or(value); - is_plain_number(mantissa) +/// A decimal number with an optional `K`/`M`/`G`/`T` suffix — accepted +/// case-insensitively and with an optional trailing `B` (`gb`/`Gb`/`GB`/`g` +/// are all read as `G`) — returned in the single uppercase-letter-no- +/// redundant-`B` form systemd itself accepts. See [`validate_memory_max`] +/// for why. A trailing `B` with no multiplier before it (`1048576B`) is a +/// bare byte count and kept as-is — systemd accepts `B` as a unit on its +/// own, unlike the redundant `KB`/`MB`/`GB`/`TB` combinations. +fn normalized_byte_size(value: &str) -> Option { + if let Some(before_b) = value.strip_suffix(['B', 'b']) { + return match before_b.strip_suffix(['K', 'M', 'G', 'T', 'k', 'm', 'g', 't']) { + Some(mantissa) if is_plain_number(mantissa) => { + let unit = before_b[mantissa.len()..].to_ascii_uppercase(); + Some(format!("{mantissa}{unit}")) + } + Some(_) => None, + None => is_plain_number(before_b).then(|| format!("{before_b}B")), + }; + } + match value.strip_suffix(['K', 'M', 'G', 'T', 'k', 'm', 'g', 't']) { + Some(mantissa) if is_plain_number(mantissa) => { + let unit = value[mantissa.len()..].to_ascii_uppercase(); + Some(format!("{mantissa}{unit}")) + } + Some(_) => None, + None => is_plain_number(value).then(|| value.to_owned()), + } } /// Digits, optionally followed by a single `.` and more digits. Hand @@ -402,9 +436,31 @@ mod tests { } } + /// mara hit this directly: "16GB" reads as an obviously + /// valid size to a human, but systemd's own parser takes only the + /// single-letter form — confirmed against a running systemd 260, + /// `MemoryMax=16GB` and `MemoryMax=16g` both fail unit activation, + /// `MemoryMax=16G` doesn't. Accept the friendly spellings and normalize + /// them to what systemd actually wants, rather than rejecting input a + /// human reasonably expects to work. + #[test] + fn friendly_size_spellings_normalize_to_systemds_own_form() { + for (input, want) in [ + ("8GB", "8G"), + ("8Gb", "8G"), + ("8gb", "8G"), + ("8g", "8G"), + ("512mb", "512M"), + ("1048576b", "1048576B"), + ("1.5gb", "1.5G"), + ] { + assert_eq!(validate_memory_max(input).as_deref(), Ok(want)); + } + } + #[test] fn rejects_invalid_memory_maxes() { - for v in ["", "8GB", "G", "abc", "-8G", "8 G", "Infinity", "8Gi"] { + for v in ["", "G", "abc", "-8G", "8 G", "Infinity", "8Gi", "8KG"] { assert!(validate_memory_max(v).is_err(), "{v} should be rejected"); } } diff --git a/hive-c0re/src/dashboard/lifecycle_ops.rs b/hive-c0re/src/dashboard/lifecycle_ops.rs index 6204d434..3ba31e22 100644 --- a/hive-c0re/src/dashboard/lifecycle_ops.rs +++ b/hive-c0re/src/dashboard/lifecycle_ops.rs @@ -376,14 +376,16 @@ pub(super) async fn post_resource_limits( { return (StatusCode::UNPROCESSABLE_ENTITY, e).into_response(); } - if let Some(v) = memory_max - && let Err(e) = crate::resource_limits::validate_memory_max(v) - { - return (StatusCode::UNPROCESSABLE_ENTITY, e).into_response(); - } + // Returns the value to store, not just an ok/err verdict — see + // `validate_memory_max`'s own doc for why that isn't always `v`. + let memory_max = match memory_max.map(crate::resource_limits::validate_memory_max) { + Some(Err(e)) => return (StatusCode::UNPROCESSABLE_ENTITY, e).into_response(), + Some(Ok(v)) => Some(v), + None => None, + }; let limits = crate::resource_limits::AgentLimits { cpu_quota: cpu_quota.map(str::to_owned), - memory_max: memory_max.map(str::to_owned), + memory_max, }; if let Err(e) = crate::meta::commit_resource_limits(ident.as_str(), &limits).await { return error_response(&format!("set limits {logical}: {e:#}")); diff --git a/hive-c0re/src/server.rs b/hive-c0re/src/server.rs index 547540d4..803682e2 100644 --- a/hive-c0re/src/server.rs +++ b/hive-c0re/src/server.rs @@ -502,13 +502,17 @@ async fn handle_set_resource_limits( if let Some(value) = cpu_quota { crate::resource_limits::validate_cpu_quota(value).map_err(anyhow::Error::msg)?; } - if let Some(value) = memory_max { - crate::resource_limits::validate_memory_max(value).map_err(anyhow::Error::msg)?; - } + // `validate_memory_max` returns the value to store, not just an + // ok/err verdict: a friendly spelling like "8GB" is accepted but + // normalized to the "8G" systemd's own parser actually takes. + let memory_max = memory_max + .map(crate::resource_limits::validate_memory_max) + .transpose() + .map_err(anyhow::Error::msg)?; tracing::info!(%name, ?cpu_quota, ?memory_max, "set_resource_limits"); let limits = crate::resource_limits::AgentLimits { cpu_quota: cpu_quota.map(ToOwned::to_owned), - memory_max: memory_max.map(ToOwned::to_owned), + memory_max, }; // Goes through `meta::commit_resource_limits`, not the bare // `resource_limits::set_limits`: the write has to be staged +