fix(#1115): commit perm+topology changes under META_LOCK to prevent dirty working tree
This commit is contained in:
parent
057c673be8
commit
ed50b858c5
3 changed files with 83 additions and 12 deletions
|
|
@ -585,11 +585,14 @@ impl Coordinator {
|
||||||
let topo_before = crate::topology::read();
|
let topo_before = crate::topology::read();
|
||||||
let old_parent = topo_before.get(child).cloned().flatten();
|
let old_parent = topo_before.get(child).cloned().flatten();
|
||||||
|
|
||||||
// The disk write happens here; topology validation +
|
// The disk write + git commit happen here under META_LOCK, so
|
||||||
// idempotent fast-path inside `set_parent` may short-circuit
|
// the topology.json change is committed atomically and the
|
||||||
// (same parent → no-op). We mirror the same idempotent shape
|
// working tree is never left dirty between the write and the
|
||||||
// for the notifications: if nothing changed, send nothing.
|
// next meta operation. Topology validation + idempotent
|
||||||
crate::topology::set_parent(child, new_parent)?;
|
// 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;
|
let changed = old_parent.as_deref() != new_parent;
|
||||||
if changed {
|
if changed {
|
||||||
|
|
|
||||||
|
|
@ -325,6 +325,71 @@ pub async fn lock_update_hyperhive() -> Result<()> {
|
||||||
git_commit(&dir, "bump hyperhive").await
|
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("<root>")
|
||||||
|
),
|
||||||
|
)
|
||||||
|
.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(
|
fn render_flake(
|
||||||
hyperhive_flake: &str,
|
hyperhive_flake: &str,
|
||||||
nixpkgs_flake: &str,
|
nixpkgs_flake: &str,
|
||||||
|
|
|
||||||
|
|
@ -670,17 +670,20 @@ async fn dispatch(
|
||||||
}
|
}
|
||||||
(QueueKind::PermChange, _) => {
|
(QueueKind::PermChange, _) => {
|
||||||
let name = &entry.agent;
|
let name = &entry.agent;
|
||||||
// Apply the file write first — serialised here so concurrent
|
// Write + commit the perm file under META_LOCK so the
|
||||||
// dashboard batch-apply actions never race on the shared JSON.
|
// working tree is never left dirty between the file write
|
||||||
coord.set_queue_step(Some(entry.id), "writing perm file");
|
// and the subsequent prepare_deploy git operations.
|
||||||
|
coord.set_queue_step(Some(entry.id), "writing + committing perm file");
|
||||||
match &entry.perm_payload {
|
match &entry.perm_payload {
|
||||||
Some(PermPayload::ToolGroups { groups }) => {
|
Some(PermPayload::ToolGroups { groups }) => {
|
||||||
crate::tool_groups::set_groups(name, groups)
|
crate::meta::commit_tool_groups(name, groups)
|
||||||
.with_context(|| format!("set tool-groups for {name}"))?;
|
.await
|
||||||
|
.with_context(|| format!("commit tool-groups for {name}"))?;
|
||||||
}
|
}
|
||||||
Some(PermPayload::Capabilities { caps }) => {
|
Some(PermPayload::Capabilities { caps }) => {
|
||||||
crate::capabilities::set_caps(name, caps)
|
crate::meta::commit_capabilities(name, caps)
|
||||||
.map_err(|e| anyhow::anyhow!("set capabilities for {name}: {e}"))?;
|
.await
|
||||||
|
.with_context(|| format!("commit capabilities for {name}"))?;
|
||||||
}
|
}
|
||||||
None => {
|
None => {
|
||||||
anyhow::bail!(
|
anyhow::bail!(
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue