From 679a77113c013018dbe511bfe8e7c21d3445f36c Mon Sep 17 00:00:00 2001
From: Tim Anglade <timanglade@users.noreply.github.com>
Date: Sat, 20 Jun 2026 16:47:37 -0700
Subject: [PATCH] docs(state): document notify-before-persist contract and why
 update_messages diverges

The two state-mutating paths order notify_change() against the disk write
differently: set_adhoc_membership notifies first (caller saves after),
while update_messages saves first then notifies. Document this as a
deliberate divergence rather than unifying on one ordering.

update_messages must save before notifying because the manager always
follows it with trust_engine.update() as a separate step. Notifying first
would wake the gossip loop during the save().await yield, before the
manager can update the derivation -- gossip would read stale derivation and
contact the wrong peer set, breaking mesh convergence (confirmed by e2e
failure). Saving first keeps gossip asleep through the yield, so by the
time it wakes the manager can promptly update the trust engine.

set_adhoc_membership safely notifies first because the trust engine is
updated via a separate intent path, not as a caller-side step after the
notify.

Fixes IM-207

Co-authored-by: openhands <openhands@all-hands.dev>
---
 src/state.rs | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/src/state.rs b/src/state.rs
index d50656b..8bda04e 100644
--- a/src/state.rs
+++ b/src/state.rs
@@ -80,6 +80,21 @@ pub(crate) struct Args {
 /// Durable daemon state persisted to the state file.
 ///
 /// Runtime-only state (like `TrustEngine`) lives in `daemon::run()`.
+///
+/// # Notify-before-persist contract
+///
+/// Mutating paths notify subscribers via `notify_change()` *before* persisting
+/// to disk, so subscribers (e.g. the manager, gossip loop) can begin work
+/// without waiting on the disk write, and a crash before `save()` only leaves
+/// slightly stale state that converges on the next gossip round.
+/// `set_adhoc_membership` follows this: it notifies inline and relies on its
+/// caller (`persist_membership`) to `save()`.
+///
+/// `update_messages` is the documented exception: it saves *before* notifying,
+/// because the manager always follows it with `trust_engine.update()` as a
+/// separate step. Notifying first would let the woken gossip loop run during
+/// the `save().await` yield and read stale derivation. See `update_messages`
+/// for details.
 pub(crate) struct State {
     pub(crate) keypair: ImidKeypair,
     pub(crate) imid: Imid,
@@ -258,7 +273,8 @@ impl State {
         self.mutable.lock().await.adhoc_membership.clone()
     }
 
-    /// Sets the adhoc membership. Call `save()` to persist.
+    /// Sets the adhoc membership. Notifies subscribers first; caller should
+    /// `save()` to persist. See the notify-before-persist contract on `State`.
     pub(crate) async fn set_adhoc_membership(&self, membership: Option<adhoc::Membership>) {
         {
             let mut mutable = self.mutable.lock().await;
@@ -284,6 +300,8 @@ impl State {
     }
 
     /// Apply removals and additions as one observed message set update.
+    /// Saves before notifying — the documented exception to the
+    /// notify-before-persist contract on `State` (see inline comment for why).
     pub(crate) async fn update_messages(&self, update: MessageUpdate) -> Result<()> {
         let mut mutable = self.mutable.lock().await;
         let generation = mutable.messages.generation();
@@ -294,6 +312,16 @@ impl State {
         }
         assert!(new_generation > generation);
         drop(mutable);
+        // Persist before notifying. Unlike `set_adhoc_membership` (which
+        // notifies first and lets its caller save), `update_messages` is always
+        // followed by `trust_engine.update()` in the manager as a separate
+        // step. If we notified first, the `save().await` below would yield to
+        // the already-woken gossip loop before the manager could update the
+        // derivation — gossip would read stale derivation and contact the wrong
+        // peer set. Saving first keeps gossip asleep until after the yield
+        // point, so by the time it wakes the manager can promptly update the
+        // trust engine. See the notify-before-persist contract on `State` for
+        // why this path diverges.
         self.save().await?;
         self.notify_change();
         Ok(())