From 81292f14b6694c9e38ac5599645d7b880587aa7b Mon Sep 17 00:00:00 2001 From: atlas Date: Sun, 9 Aug 2026 21:25:07 +0200 Subject: [PATCH] =?UTF-8?q?fix(#2851):=20skip=20the=20state=20PUT=20when?= =?UTF-8?q?=20unchanged=20=E2=80=94=20it=20still=20emits=20an=20event?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit set_room_state PUT unconditionally and its doc called that idempotent. It is, one level too high: a PUT of identical content is a no-op on the room's STATE and the homeserver still appends an event to the TIMELINE. Downstream an event is unread activity, then a todo, then a turn — for every agent in the room. The provisioning sweep re-wiring the hive Space's child link therefore woke the whole hive on the sweep's cadence, forever, for a link that never changed (~1801s between events, measured across eleven consecutive intervals). Read the current content first and return early when it matches. The lookup fails open — an unreadable state means write, because the re-apply exists to repair a missing link and "don't know" must not be treated as "fine". Only the steady state goes quiet. --- hive-c0re/src/matrix.rs | 100 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 93 insertions(+), 7 deletions(-) diff --git a/hive-c0re/src/matrix.rs b/hive-c0re/src/matrix.rs index 84e26d07..1a4aca74 100644 --- a/hive-c0re/src/matrix.rs +++ b/hive-c0re/src/matrix.rs @@ -1218,8 +1218,47 @@ async fn find_chat_room_by_name(client: &reqwest::Client, admin_token: &str) -> None } -/// PUT a state event into `room_id` using the admin token. Idempotent — -/// re-sending identical content is a no-op on the homeserver. +/// Whether the state event still has to be written. +/// +/// `current` is the room's existing content for this `(type, state_key)`, +/// or `None` when it could not be read. **Unreadable means write**: the +/// re-apply exists so a missing link gets repaired, and one redundant +/// event is cheaper than a hierarchy that never reconverges. +/// +/// Comparison is `serde_json::Value` equality, which compares objects as +/// maps — key order in the response does not matter. +fn state_needs_write(current: Option<&serde_json::Value>, desired: &serde_json::Value) -> bool { + current != Some(desired) +} + +/// Read the room's current content for one state event. `None` on any +/// failure — absent state, transport error, or an unparseable body are +/// all "we don't know", and [`state_needs_write`] turns that into a write. +async fn current_room_state( + client: &reqwest::Client, + admin_token: &str, + url: &str, +) -> Option { + let resp = client.get(url).bearer_auth(admin_token).send().await.ok()?; + if !resp.status().is_success() { + return None; + } + resp.json::().await.ok() +} + +/// PUT a state event into `room_id` using the admin token, **skipping the +/// write when the room already carries identical content**. +/// +/// The read is not an optimisation. A PUT of identical content is a no-op +/// on the room's *state*, and the homeserver still appends an event to the +/// *timeline* — so "idempotent" was true one level too high. Downstream, +/// an event is unread activity, which is a todo, which is a turn: a caller +/// re-applying a link on a periodic sweep wakes every agent in the room on +/// that sweep's cadence, forever. Measured at ~30 minutes per wake per +/// agent before this guard existed. +/// +/// The self-healing property the re-apply exists for is unaffected: a +/// missing or divergent link still gets written. async fn set_room_state( client: &reqwest::Client, admin_token: &str, @@ -1233,6 +1272,15 @@ async fn set_room_state( let encoded_key = encode_room_id_for_url(state_key); let url = format!("{base}/_matrix/client/v3/rooms/{encoded_room}/state/{event_type}/{encoded_key}"); + let current = current_room_state(client, admin_token, &url).await; + if !state_needs_write(current.as_ref(), content) { + tracing::debug!( + %room_id, + event_type, + "matrix: state already current, skipping PUT (no timeline event)" + ); + return Ok(()); + } let resp = client .put(&url) .bearer_auth(admin_token) @@ -1261,8 +1309,9 @@ async fn set_room_state( /// join instead of an empty Space. /// /// Dedup mirrors [`ensure_hive_space`]: persisted id wins, else rediscover -/// by name, else create. The space-child link is re-applied on every call -/// (idempotent PUT) so a recovered room reconverges its hierarchy link. +/// by name, else create. The space-child link is re-checked on every call +/// so a recovered room reconverges its hierarchy link — and written only +/// when it differs, see [`set_room_state`]. /// /// # Errors /// @@ -1347,9 +1396,9 @@ pub async fn ensure_hive_chat_room( id }; - // Wire the Space → room child link (idempotent). Without an - // `m.space.child` carrying a `via`, the room won't surface in the Space - // hierarchy. `suggested` hints clients to surface it prominently. + // Wire the Space → room child link. Without an `m.space.child` carrying + // a `via`, the room won't surface in the Space hierarchy. `suggested` + // hints clients to surface it prominently. let child_content = serde_json::json!({ "via": [server_name], "suggested": true, @@ -1700,6 +1749,43 @@ mod tests { assert!(h.chars().all(|c| c.is_ascii_hexdigit())); } + /// The steady state. This is the whole point of the guard: the sweep + /// runs forever, and every write it makes is a wake for every agent + /// in the room. + #[test] + fn state_matching_the_room_is_not_rewritten() { + let desired = serde_json::json!({ "via": ["example.org"], "suggested": true }); + let current = desired.clone(); + assert!(!state_needs_write(Some(¤t), &desired)); + } + + /// Key order in the homeserver's response must not force a write — + /// the guard leans on `serde_json::Value` comparing objects as maps, + /// and a byte- or order-sensitive comparison would silently degrade + /// to writing every time while still looking correct. + #[test] + fn state_matching_but_reordered_is_not_rewritten() { + let desired = serde_json::json!({ "via": ["example.org"], "suggested": true }); + let current = serde_json::json!({ "suggested": true, "via": ["example.org"] }); + assert!(!state_needs_write(Some(¤t), &desired)); + } + + #[test] + fn diverged_state_is_rewritten() { + let desired = serde_json::json!({ "via": ["example.org"], "suggested": true }); + let current = serde_json::json!({ "via": ["old.example.org"], "suggested": true }); + assert!(state_needs_write(Some(¤t), &desired)); + } + + /// Fail-open, and deliberately so: an unreadable current state must + /// write. The re-apply exists to repair a missing link, so "we don't + /// know" has to behave like "it's missing", not like "it's fine". + #[test] + fn unknown_state_is_written() { + let desired = serde_json::json!({ "via": ["example.org"], "suggested": true }); + assert!(state_needs_write(None, &desired)); + } + #[test] fn random_hex_two_calls_differ() { // Sanity check — not a statistical claim, just guards