From ed50b858c5a22cccae56af35259cd303eef125a1 Mon Sep 17 00:00:00 2001 From: damocles Date: Wed, 3 Jun 2026 10:07:38 +0200 Subject: [PATCH] fix(#1115): commit perm+topology changes under META_LOCK to prevent dirty working tree --- hive-c0re/src/coordinator.rs | 13 ++++--- hive-c0re/src/meta.rs | 65 ++++++++++++++++++++++++++++++++++ hive-c0re/src/rebuild_queue.rs | 17 +++++---- 3 files changed, 83 insertions(+), 12 deletions(-) diff --git a/hive-c0re/src/coordinator.rs b/hive-c0re/src/coordinator.rs index 212d7ec3..d07e38dc 100644 --- a/hive-c0re/src/coordinator.rs +++ b/hive-c0re/src/coordinator.rs @@ -585,11 +585,14 @@ impl Coordinator { let topo_before = crate::topology::read(); let old_parent = topo_before.get(child).cloned().flatten(); - // The disk write happens here; topology validation + - // idempotent fast-path inside `set_parent` may short-circuit - // (same parent → no-op). We mirror the same idempotent shape - // for the notifications: if nothing changed, send nothing. - crate::topology::set_parent(child, new_parent)?; + // The disk write + git commit happen here under META_LOCK, so + // the topology.json change is committed atomically and the + // working tree is never left dirty between the write and the + // next meta operation. Topology validation + idempotent + // fast-path inside `set_parent` may short-circuit (same + // parent → no-op). We mirror the same idempotent shape for + // the notifications: if nothing changed, send nothing. + crate::meta::commit_topology(child, new_parent).await?; let changed = old_parent.as_deref() != new_parent; if changed { diff --git a/hive-c0re/src/meta.rs b/hive-c0re/src/meta.rs index cf7bda75..8d39135e 100644 --- a/hive-c0re/src/meta.rs +++ b/hive-c0re/src/meta.rs @@ -325,6 +325,71 @@ pub async fn lock_update_hyperhive() -> Result<()> { git_commit(&dir, "bump hyperhive").await } +/// Write the tool-groups file for `agent` and commit it atomically +/// under `META_LOCK`. Ensures the JSON change is staged + committed +/// before the next `prepare_deploy` or `sync_agents` runs, so the +/// working tree is never left dirty by an untimely PermChange write. +pub async fn commit_tool_groups(agent: &str, groups: &[String]) -> Result<()> { + let _guard = META_LOCK.lock().await; + crate::tool_groups::set_groups(agent, groups)?; + let dir = meta_dir(); + if crate::tool_groups::tool_groups_path().exists() { + git(&dir, &["add", "tool-groups.json"]).await?; + } + if has_staged_changes(&dir).await? { + git_commit(&dir, &format!("set tool-groups for {agent}")).await?; + } + Ok(()) +} + +/// Write the capabilities file for `agent` and commit it atomically +/// under `META_LOCK`. Same rationale as `commit_tool_groups`. +pub async fn commit_capabilities(agent: &str, caps: &[String]) -> Result<()> { + let _guard = META_LOCK.lock().await; + crate::capabilities::set_caps(agent, caps) + .map_err(|e| anyhow::anyhow!("set capabilities for {agent}: {e}"))?; + let dir = meta_dir(); + if crate::capabilities::capabilities_path().exists() { + git(&dir, &["add", "capabilities.json"]).await?; + } + if has_staged_changes(&dir).await? { + git_commit(&dir, &format!("set capabilities for {agent}")).await?; + } + Ok(()) +} + +/// Write the topology file and commit it atomically under `META_LOCK`. +/// Returns `Err(String)` on validation failure (unknown agent, cycle, +/// etc.) — same shape as `topology::set_parent` — so callers can +/// surface the error as a user-visible message. Git failures are +/// logged as warnings and don't propagate: the topology write already +/// succeeded, and `sync_agents` will pick up any un-committed change +/// on the next run as a safety net. +pub async fn commit_topology(child: &str, new_parent: Option<&str>) -> std::result::Result<(), String> { + let _guard = META_LOCK.lock().await; + crate::topology::set_parent(child, new_parent)?; + let dir = meta_dir(); + let stage = async { + git(&dir, &["add", "topology.json"]).await?; + if has_staged_changes(&dir).await? { + git_commit( + &dir, + &format!( + "topology: {} → {}", + child, + new_parent.unwrap_or("") + ), + ) + .await?; + } + Ok::<_, anyhow::Error>(()) + }; + if let Err(e) = stage.await { + tracing::warn!(%child, ?new_parent, error = ?e, "commit_topology: topology written but git commit failed (sync_agents will recover)"); + } + Ok(()) +} + fn render_flake( hyperhive_flake: &str, nixpkgs_flake: &str, diff --git a/hive-c0re/src/rebuild_queue.rs b/hive-c0re/src/rebuild_queue.rs index 25cd044c..e5ce7777 100644 --- a/hive-c0re/src/rebuild_queue.rs +++ b/hive-c0re/src/rebuild_queue.rs @@ -670,17 +670,20 @@ async fn dispatch( } (QueueKind::PermChange, _) => { let name = &entry.agent; - // Apply the file write first — serialised here so concurrent - // dashboard batch-apply actions never race on the shared JSON. - coord.set_queue_step(Some(entry.id), "writing perm file"); + // Write + commit the perm file under META_LOCK so the + // working tree is never left dirty between the file write + // and the subsequent prepare_deploy git operations. + coord.set_queue_step(Some(entry.id), "writing + committing perm file"); match &entry.perm_payload { Some(PermPayload::ToolGroups { groups }) => { - crate::tool_groups::set_groups(name, groups) - .with_context(|| format!("set tool-groups for {name}"))?; + crate::meta::commit_tool_groups(name, groups) + .await + .with_context(|| format!("commit tool-groups for {name}"))?; } Some(PermPayload::Capabilities { caps }) => { - crate::capabilities::set_caps(name, caps) - .map_err(|e| anyhow::anyhow!("set capabilities for {name}: {e}"))?; + crate::meta::commit_capabilities(name, caps) + .await + .with_context(|| format!("commit capabilities for {name}"))?; } None => { anyhow::bail!(