mirror of
https://github.com/jmcorgan/fips.git
synced 2026-07-30 19:46:15 +00:00
Watch the Nostr loops that never return, not the one that does
Every node reported state: degraded permanently. The liveness probe polled connect_task, which wraps a single Client::connect() call. That call only spawns a per-relay background connection task and returns, so the handle finished moments after start on a perfectly healthy node, the supervisor saw a child exit, and the node latched Degraded for the rest of its life. Nothing behaved differently, because every consumer of NodeState treats Degraded the same as Running. What was lost is the signal: a genuine degradation was indistinguishable from the permanent false one. Watch the three service loops that cannot return by design instead — the inbound notify loop, the advert publisher, and the refresh ticker. Each is an unconditional loop, so a finished handle means a panic or an abort, which is unrecoverable and matches the one-way ChildExited latch in the supervisor. connect_task and relay_startup_task stay deliberately unwatched, and the doc comment now says why, since watching either reproduces this bug exactly. The tests install task handles directly and check both directions: that a finished connect_task alongside three live loops reports healthy, which is the production configuration a few hundred milliseconds after start, and that each loop dying on its own reports degraded. Reverting to the old predicate reds four of five; replacing the predicate with a constant false also reds four of five, so the fix cannot pass by never reporting degraded at all.
This commit is contained in:
+61
-13
@@ -176,21 +176,51 @@ pub struct NostrRendezvous {
|
||||
}
|
||||
|
||||
impl NostrRendezvous {
|
||||
/// Whether the primary Nostr connection task has exited (runtime liveness).
|
||||
/// Whether the Nostr subsystem has stopped being able to do its job
|
||||
/// (runtime liveness).
|
||||
///
|
||||
/// "Nostr exited" is defined as the primary `connect_task` having finished.
|
||||
/// It is `Some` for the engine's whole running life (installed in `start`);
|
||||
/// `shutdown` takes it, leaving `None` — a taken handle means the engine has
|
||||
/// been shut down, which counts as finished, so a `None` inner maps to
|
||||
/// `true` (this lets the liveness poll monitor terminate after a stop rather
|
||||
/// than spinning forever). `connect_task` is a `tokio::sync::Mutex`, so this
|
||||
/// sync accessor uses the non-blocking `try_lock`: a momentarily-contended
|
||||
/// lock (only start/stop hold it, briefly) reports "not finished", the safe
|
||||
/// direction — the 2s liveness poll re-checks next tick and never spuriously
|
||||
/// degrades a healthy node.
|
||||
/// "Nostr exited" is defined as *any* of the three service loops that never
|
||||
/// return by design having finished:
|
||||
///
|
||||
/// - `notify_task` — the inbound receive loop. Without it no advert and no
|
||||
/// traversal signal is ever observed again.
|
||||
/// - `publish_task` — the advert publisher. Without it this node stops being
|
||||
/// discoverable.
|
||||
/// - `advertise_task` — the refresh ticker that drives the publisher.
|
||||
///
|
||||
/// Each of these is an unconditional `loop` (the notify loop's only `break`
|
||||
/// is the relay-pool broadcast channel closing, i.e. the pool itself is
|
||||
/// gone), so a finished handle means the task panicked or was aborted:
|
||||
/// unrecoverable, which matches the one-way `ChildExited` → `Degraded`
|
||||
/// latch in the supervisor FSM.
|
||||
///
|
||||
/// Deliberately *not* watched: `connect_task` and `relay_startup_task`.
|
||||
/// `Client::connect()` only spawns a per-relay background connection task
|
||||
/// and returns, so `connect_task` finishes moments after start on a
|
||||
/// perfectly healthy node; `relay_startup_task` breaks out of its retry loop
|
||||
/// on the first successful subscribe. Watching either reports Degraded on
|
||||
/// every node forever.
|
||||
///
|
||||
/// Each handle is `Some` for the engine's whole running life (installed in
|
||||
/// `start`); `shutdown` takes them all, leaving `None` — a taken handle
|
||||
/// means the engine has been shut down, which counts as finished, so a
|
||||
/// `None` inner maps to `true` (this lets the liveness poll monitor
|
||||
/// terminate after a stop rather than spinning forever). The slots are
|
||||
/// `tokio::sync::Mutex`es, so this sync accessor uses the non-blocking
|
||||
/// `try_lock`: a momentarily-contended lock (only start/stop hold it,
|
||||
/// briefly) reports "not finished", the safe direction — the 2s liveness
|
||||
/// poll re-checks next tick and never spuriously degrades a healthy node.
|
||||
pub fn is_finished(&self) -> bool {
|
||||
self.connect_task
|
||||
.try_lock()
|
||||
Self::task_finished(&self.notify_task)
|
||||
|| Self::task_finished(&self.publish_task)
|
||||
|| Self::task_finished(&self.advertise_task)
|
||||
}
|
||||
|
||||
/// Liveness of one task slot: finished if the handle is gone (shut down) or
|
||||
/// the task has completed; "not finished" when the slot is momentarily
|
||||
/// locked by start/stop.
|
||||
fn task_finished(slot: &Mutex<Option<JoinHandle<()>>>) -> bool {
|
||||
slot.try_lock()
|
||||
.map(|g| g.as_ref().is_none_or(|h| h.is_finished()))
|
||||
.unwrap_or(false)
|
||||
}
|
||||
@@ -1714,6 +1744,24 @@ impl NostrRendezvous {
|
||||
}
|
||||
}
|
||||
|
||||
/// Install the five background-task handles that `start` would install, so
|
||||
/// liveness tests can drive `is_finished()` without live relays. Each
|
||||
/// argument is the handle to place in the matching slot.
|
||||
pub(crate) async fn install_tasks_for_test(
|
||||
&self,
|
||||
connect: JoinHandle<()>,
|
||||
relay_startup: JoinHandle<()>,
|
||||
notify: JoinHandle<()>,
|
||||
publish: JoinHandle<()>,
|
||||
advertise: JoinHandle<()>,
|
||||
) {
|
||||
*self.connect_task.lock().await = Some(connect);
|
||||
*self.relay_startup_task.lock().await = Some(relay_startup);
|
||||
*self.notify_task.lock().await = Some(notify);
|
||||
*self.publish_task.lock().await = Some(publish);
|
||||
*self.advertise_task.lock().await = Some(advertise);
|
||||
}
|
||||
|
||||
/// Build a `CachedOverlayAdvert` for tests with a single endpoint and
|
||||
/// a generous validity window (one hour from `now_ms()`).
|
||||
pub(crate) fn cached_advert_for_test(
|
||||
|
||||
@@ -857,3 +857,106 @@ fn signal_relays_without_an_advert_still_resolves() {
|
||||
"the responder path passes no advert and must still produce a target set"
|
||||
);
|
||||
}
|
||||
|
||||
/// A `JoinHandle` for a task that has definitely completed. Yields until the
|
||||
/// runtime has polled the no-op task to completion, so `is_finished()` is
|
||||
/// deterministically `true` on return.
|
||||
async fn finished_handle() -> tokio::task::JoinHandle<()> {
|
||||
let handle = tokio::spawn(async {});
|
||||
while !handle.is_finished() {
|
||||
tokio::task::yield_now().await;
|
||||
}
|
||||
handle
|
||||
}
|
||||
|
||||
/// A `JoinHandle` for a task that never completes.
|
||||
fn live_handle() -> tokio::task::JoinHandle<()> {
|
||||
tokio::spawn(std::future::pending::<()>())
|
||||
}
|
||||
|
||||
/// The regression case: `connect_task` and `relay_startup_task` both return by
|
||||
/// design (`Client::connect()` only kicks off per-relay connection tasks;
|
||||
/// the startup loop breaks on the first successful subscribe), so a healthy
|
||||
/// node has two finished handles and must still report live.
|
||||
#[tokio::test]
|
||||
async fn nostr_liveness_ignores_the_tasks_that_return_by_design() {
|
||||
let runtime = NostrRendezvous::new_for_test();
|
||||
runtime
|
||||
.install_tasks_for_test(
|
||||
finished_handle().await,
|
||||
finished_handle().await,
|
||||
live_handle(),
|
||||
live_handle(),
|
||||
live_handle(),
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
!runtime.is_finished(),
|
||||
"a node whose connect/relay-startup tasks have returned normally is healthy"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn nostr_liveness_fires_when_the_notify_loop_dies() {
|
||||
let runtime = NostrRendezvous::new_for_test();
|
||||
runtime
|
||||
.install_tasks_for_test(
|
||||
live_handle(),
|
||||
live_handle(),
|
||||
finished_handle().await,
|
||||
live_handle(),
|
||||
live_handle(),
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
runtime.is_finished(),
|
||||
"a dead inbound notify loop means no advert or signal is ever received again"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn nostr_liveness_fires_when_the_publish_loop_dies() {
|
||||
let runtime = NostrRendezvous::new_for_test();
|
||||
runtime
|
||||
.install_tasks_for_test(
|
||||
live_handle(),
|
||||
live_handle(),
|
||||
live_handle(),
|
||||
finished_handle().await,
|
||||
live_handle(),
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
runtime.is_finished(),
|
||||
"a dead publish loop means this node stops being discoverable"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn nostr_liveness_fires_when_the_advertise_loop_dies() {
|
||||
let runtime = NostrRendezvous::new_for_test();
|
||||
runtime
|
||||
.install_tasks_for_test(
|
||||
live_handle(),
|
||||
live_handle(),
|
||||
live_handle(),
|
||||
live_handle(),
|
||||
finished_handle().await,
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
runtime.is_finished(),
|
||||
"a dead advertise ticker means the advert is never refreshed"
|
||||
);
|
||||
}
|
||||
|
||||
/// `shutdown` takes every handle, leaving `None`. That must read as finished so
|
||||
/// the 2s liveness poll monitor terminates instead of spinning after a stop.
|
||||
#[tokio::test]
|
||||
async fn nostr_liveness_reports_finished_once_the_handles_are_taken() {
|
||||
let runtime = NostrRendezvous::new_for_test();
|
||||
assert!(
|
||||
runtime.is_finished(),
|
||||
"no installed handles (post-shutdown) reads as finished"
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user