fix: true atomic bulk topology -- apply_set_parent + single write, add # Errors docs

This commit is contained in:
damocles 2026-06-03 23:04:34 +02:00 committed by mara
commit 84383568b2
2 changed files with 30 additions and 6 deletions

View file

@ -719,7 +719,12 @@ impl Coordinator {
/// Batch version of [`reparent_with_notify`]: applies all moves under a /// Batch version of [`reparent_with_notify`]: applies all moves under a
/// single `META_LOCK` acquisition (one git commit) then sends per-agent /// single `META_LOCK` acquisition (one git commit) then sends per-agent
/// notifications for each move that actually changed topology. /// notifications for each move that actually changed topology.
/// First validation failure aborts the whole batch. /// First validation failure aborts the whole batch with no disk writes.
///
/// # Errors
///
/// Propagates any error returned by [`crate::meta::bulk_commit_topology`]
/// (validation failure or topology-file write error).
pub async fn reparent_bulk_with_notify( pub async fn reparent_bulk_with_notify(
self: &Arc<Self>, self: &Arc<Self>,
moves: &[(&str, Option<&str>)], moves: &[(&str, Option<&str>)],

View file

@ -400,13 +400,26 @@ pub async fn commit_topology(
/// Batch variant of [`commit_topology`]: applies every `(child, new_parent)` /// Batch variant of [`commit_topology`]: applies every `(child, new_parent)`
/// move under a single `META_LOCK` acquisition and creates **one** git commit /// move under a single `META_LOCK` acquisition and creates **one** git commit
/// for all of them. Moves are applied in the order given; the first /// for all of them. Moves are applied in the order given; the first
/// validation error short-circuits the whole batch (no partial writes because /// validation error short-circuits the whole batch. True atomic write: all
/// `topology::set_parent` rewrites the in-memory map atomically and we only /// moves are pre-validated against a cumulative in-memory state with
/// flush to disk after all moves have passed validation). /// [`crate::topology::apply_set_parent`] before anything touches disk, then
/// [`crate::topology::write`] is called exactly once. If any move fails
/// validation the topology file is never modified.
///
/// The multi-move commit message uses `moves[0].1` as the destination label.
/// This is intentional: the dashboard bulk-move UI always sends a single
/// destination for all selected agents, so the message is always accurate in
/// practice.
/// ///
/// Returns a `Vec` of `(child, old_parent)` pairs for every move that /// Returns a `Vec` of `(child, old_parent)` pairs for every move that
/// actually changed the topology (idempotent same-parent moves are skipped), /// actually changed the topology (idempotent same-parent moves are skipped),
/// so the caller can send targeted notifications. /// so the caller can send targeted notifications.
///
/// # Errors
///
/// Returns a `String` error if any move fails validation (cycle, unknown
/// agent, etc.) or if the topology file cannot be written. Git-commit failure
/// is logged as a warning and does not propagate — `sync_agents` will recover.
pub async fn bulk_commit_topology( pub async fn bulk_commit_topology(
moves: &[(&str, Option<&str>)], moves: &[(&str, Option<&str>)],
) -> std::result::Result<Vec<(String, Option<String>)>, String> { ) -> std::result::Result<Vec<(String, Option<String>)>, String> {
@ -416,9 +429,15 @@ pub async fn bulk_commit_topology(
let _guard = META_LOCK.lock().await; let _guard = META_LOCK.lock().await;
// Snapshot parents before any writes so we can compute the diff. // Snapshot parents before any writes so we can compute the diff.
let topo_before = crate::topology::read(); let topo_before = crate::topology::read();
// Validate + apply all moves in sequence; first error aborts. // Validate all moves against a cumulative in-memory state -- no disk
// writes yet; first error aborts with the topology file untouched.
let mut next = topo_before.clone();
for (child, new_parent) in moves { for (child, new_parent) in moves {
crate::topology::set_parent(child, *new_parent)?; next = crate::topology::apply_set_parent(&next, child, *new_parent)?;
}
// Only flush to disk if something actually changed.
if next != topo_before {
crate::topology::write(&next).map_err(|e| format!("{e:#}"))?;
} }
// Commit the whole batch as one git operation. // Commit the whole batch as one git operation.
let dir = meta_dir(); let dir = meta_dir();