Skip to content

Commit 637ea2a

Browse files
committed
fix(cluster): stop blocking surrogate hwm flush on raft propose
propose_surrogate_hwm blocks for up to DEFAULT_PROPOSE_TIMEOUT (5s) waiting for the entry to commit, but surrogate assignment also runs on the Raft apply loop, so a flush triggered from that path parked the very loop that had to commit the entry, stalling until the timeout fired. That stall was what made Lite's sync deltas time out before Origin could ack them. Dispatch the propose on the current Tokio handle instead of awaiting it inline. The local write is already durable via the catalog and WAL before the propose runs, and apply_surrogate_alloc's restore_hwm is idempotent and monotonic, so out-of-order or duplicate delivery of the proposed watermark is safe.
1 parent eea86b2 commit 637ea2a

1 file changed

Lines changed: 55 additions & 13 deletions

File tree

  • nodedb/src/control/surrogate/assign/core

nodedb/src/control/surrogate/assign/core/flush.rs

Lines changed: 55 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
//! Local flush-trigger + durable checkpoint persistence for
44
//! [`super::SurrogateAssigner`].
55
6+
use std::sync::Arc;
7+
68
use super::super::super::persist::SurrogateHwmPersist;
79
use super::super::super::registry::SurrogateRegistry;
810
use super::super::super::wal_appender::SurrogateWalAppender;
@@ -47,11 +49,10 @@ impl SurrogateAssigner {
4749
return Ok(());
4850
}
4951
if registry.should_flush() {
50-
let raft_shared = self.shared.get().and_then(|w| w.upgrade());
5152
let combined = CombinedPersist {
5253
catalog,
5354
wal_appender: self.wal_appender.as_ref(),
54-
raft_shared: raft_shared.as_deref(),
55+
raft_shared: self.shared.get().and_then(|w| w.upgrade()),
5556
};
5657
registry.flush(&combined)?;
5758
}
@@ -67,23 +68,34 @@ struct CombinedPersist<'a> {
6768
catalog: &'a SystemCatalog,
6869
wal_appender: &'a dyn SurrogateWalAppender,
6970
/// Present when the Raft cluster is active; drives the Raft propose.
70-
raft_shared: Option<&'a SharedState>,
71+
raft_shared: Option<Arc<SharedState>>,
7172
}
7273

7374
impl SurrogateHwmPersist for CombinedPersist<'_> {
7475
fn checkpoint(&self, hwm: u32) -> crate::Result<()> {
7576
self.catalog.put_surrogate_hwm(hwm)?;
7677
self.wal_appender.record_alloc_to_wal(hwm)?;
77-
// Propose to Raft when in cluster mode so followers advance
78-
// their in-memory HWM. Failure is non-fatal for the local
79-
// write (which is already durable via the catalog and WAL);
80-
// the follower will catch up on the next flush cycle or via
81-
// snapshot. We log at warn so operators can detect systemic
82-
// issues without breaking the local write path.
83-
if let Some(shared) = self.raft_shared
84-
&& let Err(e) = crate::control::metadata_proposer::propose_surrogate_hwm(shared, hwm)
85-
{
86-
tracing::warn!(hwm, error = %e, "surrogate hwm raft propose failed; followers may lag");
78+
// Propose to Raft when in cluster mode so followers advance their
79+
// in-memory HWM. This is dispatched off the caller's thread and
80+
// never awaited here: the local write is ALREADY durable via the
81+
// catalog and WAL above, so the propose carries no correctness
82+
// weight for it — it only advances peers' (and a future joiner's)
83+
// view of the watermark.
84+
//
85+
// Awaiting it inline was a liveness bug. `propose_surrogate_hwm`
86+
// blocks for `DEFAULT_PROPOSE_TIMEOUT` (5s) waiting for the entry
87+
// to commit, and surrogate assignment runs on the Raft apply loop
88+
// as well as on the coordinator — so a flush triggered from the
89+
// apply path parked the very loop that had to commit the entry,
90+
// and only unwound when the timeout fired. Every such write ate a
91+
// 5s stall, which is what made Lite's sync deltas time out before
92+
// Origin could ack them.
93+
//
94+
// Out-of-order or duplicate delivery is safe: `apply_surrogate_alloc`
95+
// advances the watermark through `restore_hwm`, which is idempotent
96+
// and monotonic and never moves the counter backwards.
97+
if let Some(shared) = &self.raft_shared {
98+
spawn_hwm_propose(Arc::clone(shared), hwm);
8799
}
88100
Ok(())
89101
}
@@ -92,3 +104,33 @@ impl SurrogateHwmPersist for CombinedPersist<'_> {
92104
self.catalog.get_surrogate_hwm()
93105
}
94106
}
107+
108+
/// Dispatch the `SurrogateAlloc { hwm }` metadata propose without blocking
109+
/// the caller.
110+
///
111+
/// Spawned as a normal runtime task rather than via `spawn_blocking` because
112+
/// `propose_surrogate_hwm` uses `block_in_place` internally, which is only
113+
/// legal on a multi-threaded runtime worker.
114+
///
115+
/// A missing reactor means this checkpoint ran outside a Tokio context, where
116+
/// the propose could not have been issued at all. That is not silent data
117+
/// loss — the hwm is already durable in the catalog and WAL, and the next
118+
/// flush that does run under a reactor re-proposes the (higher) watermark —
119+
/// but it is logged so a node that never advances peer watermarks is
120+
/// diagnosable rather than invisible.
121+
fn spawn_hwm_propose(shared: Arc<SharedState>, hwm: u32) {
122+
let Ok(handle) = tokio::runtime::Handle::try_current() else {
123+
tracing::debug!(
124+
hwm,
125+
"surrogate hwm checkpoint ran outside a Tokio runtime; \
126+
skipping the metadata propose (hwm is durable locally; \
127+
the next flush under a reactor re-proposes it)"
128+
);
129+
return;
130+
};
131+
handle.spawn(async move {
132+
if let Err(e) = crate::control::metadata_proposer::propose_surrogate_hwm(&shared, hwm) {
133+
tracing::warn!(hwm, error = %e, "surrogate hwm raft propose failed; followers may lag");
134+
}
135+
});
136+
}

0 commit comments

Comments
 (0)