Compare commits

...
Author SHA1 Message Date
atlas
f4c470881e fix(#2851): warn when the state read breaks but the write still works
argus's review note: current_room_state collapsed absent-state,
transport failure and an unparseable body into one None, so a
systematically failing GET was unobservable.

Only one of the three actually hides. A transport error takes the PUT
down with it one line later, and a 404 is the expected first-setup
case — both stay at debug. A non-404 HTTP failure is the silent one:
the read is broken while the write still succeeds, so the guard
switches off and the sweep resumes emitting with nothing to show for
it. That case, and only that case, warns.

Keeping the warn narrow is the point: one that also fired on every
expected 404 would train the reader to skip the line.
2026-08-09 21:31:04 +02:00
atlas
81292f14b6 fix(#2851): skip the state PUT when unchanged — it still emits an event
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.
2026-08-09 21:25:07 +02:00

View file

@ -1218,8 +1218,75 @@ 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.
///
/// One failure is louder than the rest, and it is the only one that hides:
/// **a non-404 HTTP failure means the read broke while the write still
/// works**, so the guard degrades to writing every time and the sweep
/// resumes waking the hive with nothing else to show for it. A transport
/// error needs no warning of its own — the PUT immediately after it fails
/// too, loudly — and a 404 is the expected first-setup case.
async fn current_room_state(
client: &reqwest::Client,
admin_token: &str,
url: &str,
) -> Option<serde_json::Value> {
let resp = match client.get(url).bearer_auth(admin_token).send().await {
Ok(resp) => resp,
Err(e) => {
tracing::debug!(error = ?e, url, "matrix: state read unreachable; writing");
return None;
}
};
let status = resp.status();
if !status.is_success() {
if status != reqwest::StatusCode::NOT_FOUND {
tracing::warn!(
%status,
url,
"matrix: state read failed while writes still work — the \
skip-if-unchanged guard is off and every sweep will re-emit"
);
}
return None;
}
match resp.json::<serde_json::Value>().await {
Ok(body) => Some(body),
Err(e) => {
tracing::debug!(error = ?e, url, "matrix: state read body unparseable; writing");
None
}
}
}
/// 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 +1300,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 +1337,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 +1424,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 +1777,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(&current), &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(&current), &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(&current), &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