From de09628c7c430b25fa34ad26218dbc88509cf5cd Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 26 Jul 2026 23:50:05 +0200 Subject: [PATCH] fix(#2733): write the agent pause marker via hive-priv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Coordinator::set_paused` wrote the marker directly with `std::fs::write` from hive-c0re, which runs as the unprivileged `hive-core` user. The agent's harness dir is chowned to the agent user on every container boot (`user.nix`'s activation chown), mode 0755 — so hive-core can stat the marker but gets EACCES creating or unlinking it. Pause therefore only ever worked on an agent that had never booted; the read side works because a stat needs traverse, not write, which is why the paused pill and `is_paused` looked healthy. Route both directions through hive-priv, the root helper that already owns the other writes into agent-owned directories: - `PrivRequest::SetAgentPaused { agent_name, paused }`, with the marker filename constant moved to hive-priv-sock. That is the narrowest crate all three sides share (hive-priv deliberately does not depend on hive-sh4re, which re-exports it for the in-container resolver). A private copy on any one side would break pause silently, since every reader just sees "no marker". - `write_agent_state_file` generalised to `write_agent_dir_file`, taking the target directory: `state/` and `harness/` are both agent-owned, which is the same reason both need root. - resume unlinks via `remove_file`, which acts on the leaf and never follows a symlink — an agent could otherwise plant a link at the marker path and have root delete an arbitrary file. `Coordinator::set_paused` becomes an async round-trip; its three call sites were already async. Both directions stay idempotent because the dashboard toggle and `hivectl pause|resume` fire without reading the current state first. --- Cargo.lock | 1 + hive-c0re/src/coordinator.rs | 36 +++---- hive-c0re/src/dashboard/lifecycle_ops.rs | 4 +- hive-c0re/src/priv_client.rs | 15 +++ hive-c0re/src/server.rs | 2 +- hive-priv-sock/src/lib.rs | 30 ++++++ hive-priv/src/main.rs | 126 +++++++++++++++++++++-- hive-sh4re/Cargo.toml | 1 + hive-sh4re/src/paths.rs | 14 ++- 9 files changed, 193 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9ed30fa7..ea942228 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1780,6 +1780,7 @@ name = "hive-sh4re" version = "0.1.0" dependencies = [ "chrono", + "hive-priv-sock", "hive-types", "schemars", "serde", diff --git a/hive-c0re/src/coordinator.rs b/hive-c0re/src/coordinator.rs index cfe9aa34..3c3ef5bb 100644 --- a/hive-c0re/src/coordinator.rs +++ b/hive-c0re/src/coordinator.rs @@ -1382,32 +1382,26 @@ impl Coordinator { Self::agent_paused_marker(name).exists() } - /// Create or remove the pause marker. Idempotent in both - /// directions: pausing an already-paused agent (or resuming a + /// Create or remove the pause marker, **via hive-priv**. Idempotent in + /// both directions: pausing an already-paused agent (or resuming a /// running one) is a no-op rather than an error, so the dashboard /// toggle and `hivectl pause|resume` don't have to read-then-write. /// + /// The write cannot happen in-process. The harness dir is chowned to + /// the agent user on the container's first boot (mode 0755), and this + /// daemon runs as the unprivileged `hive-core` user — so the read side + /// ([`Self::is_paused`], a stat) works while a direct `fs::write` here + /// fails with `EACCES` on every agent that has ever booted. The root + /// helper owns both directions instead. + /// /// # Errors /// - /// Returns the underlying `io::Error` when the marker can't be - /// created (agent dir missing and un-creatable, permissions, disk - /// full) or can't be removed. A `NotFound` on removal is *not* an - /// error — that's the idempotent resume case. - pub fn set_paused(name: &hive_types::Ident, paused: bool) -> std::io::Result<()> { - let marker = Self::agent_paused_marker(name); - if paused { - if let Some(parent) = marker.parent() { - std::fs::create_dir_all(parent)?; - } - // `create_new` would fail on the second pause; truncating an - // existing empty marker is the idempotent equivalent. - std::fs::write(&marker, b"") - } else { - match std::fs::remove_file(&marker) { - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), - other => other, - } - } + /// Returns the hive-priv error when the marker can't be created (agent + /// dir missing and un-creatable, disk full) or can't be removed, and + /// the transport error when the helper is unreachable. A `NotFound` on + /// removal is *not* an error — that's the idempotent resume case. + pub async fn set_paused(name: &hive_types::Ident, paused: bool) -> anyhow::Result<()> { + crate::priv_client::set_agent_paused(name.as_str(), paused).await } /// Enumerate names that have a persistent state dir under diff --git a/hive-c0re/src/dashboard/lifecycle_ops.rs b/hive-c0re/src/dashboard/lifecycle_ops.rs index e05dd608..c8e050f0 100644 --- a/hive-c0re/src/dashboard/lifecycle_ops.rs +++ b/hive-c0re/src/dashboard/lifecycle_ops.rs @@ -157,7 +157,7 @@ pub(super) async fn post_pause( Ok(i) => i, Err(e) => return (StatusCode::BAD_REQUEST, format!("bad agent name: {e}")).into_response(), }; - if let Err(e) = crate::coordinator::Coordinator::set_paused(&ident, true) { + if let Err(e) = crate::coordinator::Coordinator::set_paused(&ident, true).await { return error_response(&format!("pause {logical}: {e}")); } state.coord.rescan_containers_and_emit().await; @@ -180,7 +180,7 @@ pub(super) async fn post_resume( Ok(i) => i, Err(e) => return (StatusCode::BAD_REQUEST, format!("bad agent name: {e}")).into_response(), }; - if let Err(e) = crate::coordinator::Coordinator::set_paused(&ident, false) { + if let Err(e) = crate::coordinator::Coordinator::set_paused(&ident, false).await { return error_response(&format!("resume {logical}: {e}")); } state.coord.rescan_containers_and_emit().await; diff --git a/hive-c0re/src/priv_client.rs b/hive-c0re/src/priv_client.rs index c0b40ccc..66ab51e7 100644 --- a/hive-c0re/src/priv_client.rs +++ b/hive-c0re/src/priv_client.rs @@ -223,6 +223,21 @@ pub async fn run_forge_admin(args: &[&str]) -> Result<(String, String)> { check(call(&PrivRequest::RunForgeAdmin { args: owned }).await?) } +/// Create (`paused: true`) or remove (`paused: false`) the pause marker in +/// `agent_name`'s harness dir via hive-priv (running as root). +/// +/// hive-c0re cannot do this itself: the harness dir is chowned to the agent +/// user on the container's first boot and left mode 0755, so this process +/// can stat the marker (that's what `Coordinator::is_paused` does) but gets +/// `EACCES` on create *and* unlink. Both directions are idempotent. +pub async fn set_agent_paused(agent_name: &str, paused: bool) -> Result<()> { + ok(call(&PrivRequest::SetAgentPaused { + agent_name: agent_name.to_owned(), + paused, + }) + .await?) +} + /// Write the Forgejo access token for `agent_name` to /// `//state/forge-token` via hive-priv /// (running as root). The file is written 0600 and chowned to the agent diff --git a/hive-c0re/src/server.rs b/hive-c0re/src/server.rs index 4d0f5295..194e1423 100644 --- a/hive-c0re/src/server.rs +++ b/hive-c0re/src/server.rs @@ -328,7 +328,7 @@ async fn handle_set_paused( name: &hive_types::Ident, paused: bool, ) -> HostResponse { - if let Err(e) = Coordinator::set_paused(name, paused) { + if let Err(e) = Coordinator::set_paused(name, paused).await { return HostResponse::error(format!("set paused={paused} for {name}: {e}")); } tracing::info!(%name, paused, "agent pause marker updated"); diff --git a/hive-priv-sock/src/lib.rs b/hive-priv-sock/src/lib.rs index 9067575e..ab9b0ec8 100644 --- a/hive-priv-sock/src/lib.rs +++ b/hive-priv-sock/src/lib.rs @@ -15,6 +15,16 @@ use serde::{Deserialize, Serialize}; /// Default socket path for the privileged helper. pub const PRIV_SOCK: &str = "/run/hive/priv.sock"; +/// File name of the pause marker inside an agent's harness dir. Defined +/// here — the narrowest crate all three sides already share — because the +/// marker is a two-sided contract with no protocol behind it: hive-priv +/// creates and unlinks it as root, hive-c0re stats it to render the paused +/// indicator, and the in-container harness stats it to gate its turn loop +/// (via the `hive-sh4re::paths` re-export). A private copy on any one side +/// would break pause *silently*, since every reader just sees "no marker" — +/// exactly the failure mode a shared constant exists to prevent. +pub const PAUSED_MARKER_FILE: &str = "paused"; + /// Manager logical agent name. The manager's system container name is /// `h-ruth` (same `h-` prefix convention as every other agent). pub const MANAGER_NAME: &str = "ruth"; @@ -385,6 +395,26 @@ pub enum PrivRequest { args: Vec, }, + // --- Agent turn-loop pause --- + // (marker filename: `PAUSED_MARKER_FILE`, defined at the crate root) + /// Create (`paused: true`) or remove (`paused: false`) the pause marker + /// at `AGENT_STATE_ROOT//harness/paused`. Its presence parks + /// the agent's turn loop; the harness stats it in-container through the + /// harness bind-mount. + /// + /// Required because hive-c0re runs unprivileged: the harness dir is + /// chowned to the agent user on first container boot (mode 0755), so + /// hive-core can stat the marker but cannot create or unlink it. Both + /// directions are idempotent — pausing an already-paused agent rewrites + /// an empty file, and resuming a running one treats `NotFound` as + /// success. + SetAgentPaused { + /// Logical agent name (validated by `validate_agent_name`). + agent_name: String, + /// `true` creates the marker, `false` removes it. + paused: bool, + }, + // --- Agent credential writes --- /// Write `forge-token` into `AGENT_STATE_ROOT//state/forge-token`. /// diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index 86c92ab2..f6290057 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -22,8 +22,9 @@ use std::path::{Path, PathBuf}; use anyhow::{Context as _, Result, bail}; use hive_priv_sock::{ AGENT_PREFIX, AGENT_RUNTIME_ROOT, AGENT_STATE_ROOT, BindMount, CredentialMount, InfraAction, - InfraContainer, JournalQuery, META_DIR, MIGRATE_STAGING_ROOT, NetworkIsolation, PRIV_SOCK, - PrivEvent, PrivRequest, PrivResponse, PrivStream, PrivStreamLine, SIBLING_CONTAINERS, + InfraContainer, JournalQuery, META_DIR, MIGRATE_STAGING_ROOT, NetworkIsolation, + PAUSED_MARKER_FILE, PRIV_SOCK, PrivEvent, PrivRequest, PrivResponse, PrivStream, + PrivStreamLine, SIBLING_CONTAINERS, }; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::net::unix::OwnedWriteHalf; @@ -248,6 +249,14 @@ async fn exec(req: PrivRequest, writer: &mut OwnedWriteHalf) -> Result<(String, run_forge_admin(args).await } + PrivRequest::SetAgentPaused { + ref agent_name, + paused, + } => { + validate_agent_name(agent_name)?; + set_agent_paused(agent_name, paused) + } + PrivRequest::WriteAgentForgeToken { ref agent_name, ref token, @@ -714,12 +723,61 @@ fn write_agent_state_file( filename: &str, content: &str, ) -> Result<(String, String)> { - use std::os::fd::AsRawFd as _; - use std::os::unix::fs::MetadataExt as _; - let state_dir = PathBuf::from(AGENT_STATE_ROOT) .join(agent_name) .join("state"); + write_agent_dir_file(agent_name, &state_dir, filename, content) +} + +/// Create or remove an agent's pause marker under its harness dir. The +/// marker is written empty and chowned to the harness dir's owner (the +/// agent), matching how the harness itself would have created it. +/// +/// Both directions are idempotent: re-pausing truncates the existing empty +/// marker rather than failing, and a `NotFound` on removal is the +/// already-resumed case, not an error. +fn set_agent_paused(agent_name: &str, paused: bool) -> Result<(String, String)> { + let harness_dir = PathBuf::from(AGENT_STATE_ROOT) + .join(agent_name) + .join("harness"); + if paused { + return write_agent_dir_file(agent_name, &harness_dir, PAUSED_MARKER_FILE, ""); + } + remove_marker_in(&harness_dir, PAUSED_MARKER_FILE)?; + tracing::info!(agent = %agent_name, "cleared pause marker"); + Ok((String::new(), String::new())) +} + +/// Unlink `dir/filename`, treating "already gone" as success. +/// +/// `remove_file` unlinks the leaf itself and never follows a symlink, so an +/// agent-planted link at the marker path cannot redirect this root unlink +/// at another file — the same threat `write_state_file_nofollow` closes on +/// the create side. +fn remove_marker_in(dir: &Path, filename: &str) -> Result<()> { + let path = dir.join(filename); + match std::fs::remove_file(&path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e).with_context(|| format!("remove {}", path.display())), + } +} + +/// Write `content` to `dir/filename` as root, chowning the result to `dir`'s +/// owner so the agent process can read it back. Shared by the credential +/// writes (which target `state/`) and the pause marker (which targets +/// `harness/`) — both write into a directory owned by the agent, which is +/// precisely why they need hive-priv at all. +fn write_agent_dir_file( + agent_name: &str, + dir: &Path, + filename: &str, + content: &str, +) -> Result<(String, String)> { + use std::os::fd::AsRawFd as _; + use std::os::unix::fs::MetadataExt as _; + + let state_dir = dir.to_path_buf(); // NOTE: `create_dir_all` is normally a no-op — lifecycle creates and chowns // the state dir during spawn. On the rare edge where the dir doesn't exist // yet (container being provisioned for the first time), the newly created dir @@ -764,7 +822,12 @@ fn write_agent_state_file( ); } } - tracing::info!(agent = %agent_name, file = %filename, "wrote agent state file"); + tracing::info!( + agent = %agent_name, + dir = %state_dir.display(), + file = %filename, + "wrote agent file" + ); Ok((String::new(), String::new())) } @@ -2121,7 +2184,9 @@ async fn sync_agent_tmpfiles(agents: &[String]) -> Result<(String, String)> { #[cfg(test)] mod tests { - use super::{redact_password_line, write_state_file_nofollow}; + use super::{ + PAUSED_MARKER_FILE, redact_password_line, remove_marker_in, write_state_file_nofollow, + }; use std::path::PathBuf; use std::sync::atomic::{AtomicU32, Ordering}; @@ -2198,4 +2263,51 @@ mod tests { assert_eq!(std::fs::read_to_string(&path).unwrap(), "new"); std::fs::remove_dir_all(&dir).ok(); } + + /// The pause marker round-trips through the same root-only path the + /// credential writes use, and BOTH directions are idempotent — the + /// dashboard toggle and `hivectl pause|resume` fire blind, without + /// reading the current state first. + #[test] + fn pause_marker_create_and_remove_are_idempotent() { + let dir = scratch(); + let path = dir.join(PAUSED_MARKER_FILE); + + for _ in 0..2 { + write_state_file_nofollow(&dir, PAUSED_MARKER_FILE, "").unwrap(); + assert!(path.exists(), "marker must exist after pause"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), ""); + } + for _ in 0..2 { + remove_marker_in(&dir, PAUSED_MARKER_FILE).unwrap(); + assert!(!path.exists(), "marker must be gone after resume"); + } + std::fs::remove_dir_all(&dir).ok(); + } + + /// A resume must never follow an agent-planted symlink at the marker + /// path: this unlink runs as root, so following it would let an agent + /// delete an arbitrary file on the host. + #[test] + fn resume_unlinks_the_symlink_not_its_target() { + let dir = scratch(); + let target = dir.join("target"); + std::fs::write(&target, "original").unwrap(); + let link = dir.join(PAUSED_MARKER_FILE); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + remove_marker_in(&dir, PAUSED_MARKER_FILE).unwrap(); + // `exists()` follows the link, so it can't tell "link removed" from + // "target removed, dangling link left" — stat the link itself. + assert!( + std::fs::symlink_metadata(&link).is_err(), + "the link itself must be unlinked" + ); + assert_eq!( + std::fs::read_to_string(&target).unwrap(), + "original", + "symlink target must survive the root unlink" + ); + std::fs::remove_dir_all(&dir).ok(); + } } diff --git a/hive-sh4re/Cargo.toml b/hive-sh4re/Cargo.toml index 42e8f980..2db20c6d 100644 --- a/hive-sh4re/Cargo.toml +++ b/hive-sh4re/Cargo.toml @@ -9,6 +9,7 @@ workspace = true [dependencies] chrono.workspace = true +hive-priv-sock.workspace = true hive-types.workspace = true schemars.workspace = true serde.workspace = true diff --git a/hive-sh4re/src/paths.rs b/hive-sh4re/src/paths.rs index 8c7f7d0d..c2f69511 100644 --- a/hive-sh4re/src/paths.rs +++ b/hive-sh4re/src/paths.rs @@ -29,11 +29,15 @@ pub fn harness_dir() -> PathBuf { PathBuf::from(format!("/agents/{label}/harness")) } -/// File name of the pause marker inside the harness dir. Shared so the -/// in-container resolver below and hive-c0re's host-side one (which -/// builds the same path from `/var/lib/hyperhive/agents/{name}/harness`) -/// cannot drift apart. -pub const PAUSED_MARKER_FILE: &str = "paused"; +/// File name of the pause marker inside the harness dir. Re-exported from +/// `hive-priv-sock`, which owns the definition because hive-priv (root) is +/// the component that actually creates and unlinks the marker — hive-c0re +/// runs unprivileged and cannot write to the agent-owned harness dir — and +/// hive-priv deliberately does not depend on this crate. Shared so the +/// in-container resolver below, hive-c0re's host-side one (which builds the +/// same path from `/var/lib/hyperhive/agents/{name}/harness`) and the +/// privileged writer cannot drift apart. +pub use hive_priv_sock::PAUSED_MARKER_FILE; /// Marker file whose presence means "this agent is paused": the harness /// keeps serving its web UI and MCP daemons but drives no turns, so