topology: drop manager-root special case + notify three agents on reparent (#743)
This commit is contained in:
parent
9884ac013d
commit
2c3b62be55
4 changed files with 139 additions and 20 deletions
|
|
@ -509,6 +509,89 @@ impl Coordinator {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Apply a topology reparent + fan the resulting notifications
|
||||||
|
/// out to the three affected agents (#743). On success, drops a
|
||||||
|
/// one-line system message into the inbox of:
|
||||||
|
///
|
||||||
|
/// 1. The **old parent** (if any) — `"{child} moved out of your
|
||||||
|
/// subtree to {new_parent_or_root}"`.
|
||||||
|
/// 2. The **new parent** (if any) — `"{child} just moved into
|
||||||
|
/// your subtree (was previously under
|
||||||
|
/// {old_parent_or_root})"`.
|
||||||
|
/// 3. The **moved agent** — `"your parent changed from
|
||||||
|
/// {old_parent_or_root} to {new_parent_or_root}"`.
|
||||||
|
///
|
||||||
|
/// `_or_root` resolves to the literal string `"<root>"` when the
|
||||||
|
/// slot is `None`, keeping the wording consistent with the
|
||||||
|
/// `<parent>` sentinel's "root → operator" routing (#692). The
|
||||||
|
/// notifications fire as ordinary broker messages with
|
||||||
|
/// `from = hive_sh4re::SYSTEM_SENDER` so the dashboard renders
|
||||||
|
/// them under the existing system-source styling.
|
||||||
|
///
|
||||||
|
/// Idempotent: if `topology::set_parent` skipped the disk write
|
||||||
|
/// (same-parent no-op), no messages fire. The `topology` module's
|
||||||
|
/// validation (`apply_set_parent`: unknown agent, cycle, etc.)
|
||||||
|
/// runs *before* any message is sent, so a refused move never
|
||||||
|
/// notifies anyone.
|
||||||
|
///
|
||||||
|
/// Also drives a `rescan_containers_and_emit` after the write so
|
||||||
|
/// the dashboard tree re-renders without polling.
|
||||||
|
pub async fn reparent_with_notify(
|
||||||
|
self: &Arc<Self>,
|
||||||
|
child: &str,
|
||||||
|
new_parent: Option<&str>,
|
||||||
|
) -> std::result::Result<(), String> {
|
||||||
|
// Snapshot the old parent BEFORE the write so the
|
||||||
|
// notifications can describe both sides of the change.
|
||||||
|
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)?;
|
||||||
|
|
||||||
|
let changed = old_parent.as_deref() != new_parent;
|
||||||
|
if changed {
|
||||||
|
let old_label = old_parent.as_deref().unwrap_or("<root>");
|
||||||
|
let new_label = new_parent.unwrap_or("<root>");
|
||||||
|
// System-source notifications. Best-effort: a failed broker
|
||||||
|
// send is logged + ignored so a transient sqlite error
|
||||||
|
// doesn't bubble out and unwind the topology write.
|
||||||
|
if let Some(op) = old_parent.as_deref() {
|
||||||
|
let _ = self.broker.send(&hive_sh4re::Message {
|
||||||
|
from: hive_sh4re::SYSTEM_SENDER.to_owned(),
|
||||||
|
to: op.to_owned(),
|
||||||
|
body: format!("{child} moved out of your subtree to {new_label}"),
|
||||||
|
in_reply_to: None,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
if let Some(np) = new_parent {
|
||||||
|
let _ = self.broker.send(&hive_sh4re::Message {
|
||||||
|
from: hive_sh4re::SYSTEM_SENDER.to_owned(),
|
||||||
|
to: np.to_owned(),
|
||||||
|
body: format!(
|
||||||
|
"{child} just moved into your subtree (was previously under {old_label})"
|
||||||
|
),
|
||||||
|
in_reply_to: None,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
let _ = self.broker.send(&hive_sh4re::Message {
|
||||||
|
from: hive_sh4re::SYSTEM_SENDER.to_owned(),
|
||||||
|
to: child.to_owned(),
|
||||||
|
body: format!("your parent changed from {old_label} to {new_label}"),
|
||||||
|
in_reply_to: None,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
// Rescan + diff-emit regardless of whether messages fired —
|
||||||
|
// even an idempotent no-op might have happened concurrently
|
||||||
|
// with another lifecycle event the dashboard cares about.
|
||||||
|
self.rescan_containers_and_emit().await;
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
/// Read-only snapshot of the last cached container view. Used by
|
/// Read-only snapshot of the last cached container view. Used by
|
||||||
/// `/api/state` to cold-load page-open clients without re-running
|
/// `/api/state` to cold-load page-open clients without re-running
|
||||||
/// `nixos-container list` themselves; the
|
/// `nixos-container list` themselves; the
|
||||||
|
|
|
||||||
|
|
@ -2188,16 +2188,21 @@ async fn post_set_parent(
|
||||||
.map(str::trim)
|
.map(str::trim)
|
||||||
.filter(|s| !s.is_empty())
|
.filter(|s| !s.is_empty())
|
||||||
.map(str::to_owned);
|
.map(str::to_owned);
|
||||||
match crate::topology::set_parent(&child, new_parent.as_deref()) {
|
// `reparent_with_notify` wraps `topology::set_parent` with the
|
||||||
|
// three notification messages (#743) + the ContainerView rescan.
|
||||||
|
// Idempotent same-parent calls skip both the messages and the
|
||||||
|
// disk write per the topology fast-path.
|
||||||
|
match state
|
||||||
|
.coord
|
||||||
|
.reparent_with_notify(&child, new_parent.as_deref())
|
||||||
|
.await
|
||||||
|
{
|
||||||
Ok(()) => {
|
Ok(()) => {
|
||||||
tracing::info!(
|
tracing::info!(
|
||||||
child = %child,
|
child = %child,
|
||||||
new_parent = ?new_parent,
|
new_parent = ?new_parent,
|
||||||
"operator: set-parent via dashboard"
|
"operator: set-parent via dashboard"
|
||||||
);
|
);
|
||||||
// Topology drives ContainerView.parent; refresh the
|
|
||||||
// snapshot so connected viewers see the new tree.
|
|
||||||
state.coord.rescan_containers_and_emit().await;
|
|
||||||
(StatusCode::OK, "ok").into_response()
|
(StatusCode::OK, "ok").into_response()
|
||||||
}
|
}
|
||||||
Err(e) => error_response(&format!("set-parent {child} failed: {e}")),
|
Err(e) => error_response(&format!("set-parent {child} failed: {e}")),
|
||||||
|
|
|
||||||
|
|
@ -187,13 +187,15 @@ async fn dispatch(req: &HostRequest, coord: Arc<Coordinator>) -> HostResponse {
|
||||||
}
|
}
|
||||||
HostRequest::SetParent { child, new_parent } => {
|
HostRequest::SetParent { child, new_parent } => {
|
||||||
tracing::info!(%child, ?new_parent, "set_parent");
|
tracing::info!(%child, ?new_parent, "set_parent");
|
||||||
crate::topology::set_parent(child, new_parent.as_deref())
|
// `reparent_with_notify` wraps `topology::set_parent`
|
||||||
|
// with the three notification messages (#743) + the
|
||||||
|
// ContainerView rescan. Idempotent same-parent calls
|
||||||
|
// skip both the messages and the disk write per the
|
||||||
|
// topology fast-path.
|
||||||
|
coord
|
||||||
|
.reparent_with_notify(child, new_parent.as_deref())
|
||||||
|
.await
|
||||||
.map_err(anyhow::Error::msg)?;
|
.map_err(anyhow::Error::msg)?;
|
||||||
// ContainerView.parent is read from topology.json — a
|
|
||||||
// change here means every container row potentially
|
|
||||||
// moves in the dashboard tree. Rescan + diff-emit so
|
|
||||||
// open viewers repaint without polling.
|
|
||||||
coord.rescan_containers_and_emit().await;
|
|
||||||
HostResponse::success()
|
HostResponse::success()
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
|
||||||
|
|
@ -182,18 +182,23 @@ pub fn default_seed(agent_names: &[String]) -> BTreeMap<String, Option<String>>
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Pure validation + apply for [`set_parent`]. Splits off so tests
|
/// Pure validation + apply for [`set_parent`]. Splits off so tests
|
||||||
/// can exercise the rules (cycle / unknown / manager-protect) on
|
/// can exercise the rules (cycle / unknown) on an in-memory
|
||||||
/// an in-memory `BTreeMap` without touching the on-disk
|
/// `BTreeMap` without touching the on-disk `topology.json`. Returns
|
||||||
/// `topology.json`. Returns either the post-move map (caller
|
/// either the post-move map (caller writes it back) or a
|
||||||
/// writes it back) or a user-readable error string.
|
/// user-readable error string.
|
||||||
|
///
|
||||||
|
/// Pre-#743 this also refused to reparent the manager
|
||||||
|
/// ("cannot reparent the manager — it is structurally root") —
|
||||||
|
/// argus-paranoia from #361 that we dropped per mara's
|
||||||
|
/// `#9512` / `#9557`: the manager's special powers come from its
|
||||||
|
/// privileged MCP socket, not its tree position. The cycle walk
|
||||||
|
/// below covers "moving X under its own descendant" for the
|
||||||
|
/// manager as much as any other agent.
|
||||||
pub fn apply_set_parent(
|
pub fn apply_set_parent(
|
||||||
topo: &BTreeMap<String, Option<String>>,
|
topo: &BTreeMap<String, Option<String>>,
|
||||||
child: &str,
|
child: &str,
|
||||||
new_parent: Option<&str>,
|
new_parent: Option<&str>,
|
||||||
) -> Result<BTreeMap<String, Option<String>>, String> {
|
) -> Result<BTreeMap<String, Option<String>>, String> {
|
||||||
if child == crate::lifecycle::MANAGER_NAME {
|
|
||||||
return Err("cannot reparent the manager — it is structurally root".to_owned());
|
|
||||||
}
|
|
||||||
if !topo.contains_key(child) {
|
if !topo.contains_key(child) {
|
||||||
return Err(format!("unknown agent: {child}"));
|
return Err(format!("unknown agent: {child}"));
|
||||||
}
|
}
|
||||||
|
|
@ -342,10 +347,34 @@ mod tests {
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn apply_set_parent_refuses_manager_move() {
|
fn apply_set_parent_allows_manager_move() {
|
||||||
let err = apply_set_parent(&topo_three_level(), crate::lifecycle::MANAGER_NAME, None)
|
// Post-#743: the manager is reparentable like any other agent
|
||||||
|
// (its privileges live on the MCP socket, not its tree
|
||||||
|
// position). Build a topo with an unrelated root-level agent
|
||||||
|
// `peer` so moving the manager under it doesn't trip the
|
||||||
|
// cycle walk (every non-manager agent in topo_three_level
|
||||||
|
// descends from the manager, so that fixture can't exercise
|
||||||
|
// a legal manager move).
|
||||||
|
let mut topo = BTreeMap::new();
|
||||||
|
topo.insert(crate::lifecycle::MANAGER_NAME.to_owned(), None);
|
||||||
|
topo.insert("peer".to_owned(), None);
|
||||||
|
let next = apply_set_parent(&topo, crate::lifecycle::MANAGER_NAME, Some("peer"))
|
||||||
|
.expect("manager move should succeed post-#743");
|
||||||
|
assert_eq!(
|
||||||
|
next.get(crate::lifecycle::MANAGER_NAME),
|
||||||
|
Some(&Some("peer".to_owned()))
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn apply_set_parent_refuses_manager_under_own_descendant() {
|
||||||
|
// Moving the manager under `bob` (who already lives under
|
||||||
|
// `alice` who lives under the manager) would close the loop.
|
||||||
|
// The general cycle walk catches this; no separate manager
|
||||||
|
// guard needed.
|
||||||
|
let err = apply_set_parent(&topo_three_level(), crate::lifecycle::MANAGER_NAME, Some("bob"))
|
||||||
.unwrap_err();
|
.unwrap_err();
|
||||||
assert!(err.contains("manager"), "err = {err}");
|
assert!(err.contains("cycle"), "err = {err}");
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue