core: survive a failed net-stack rebuild instead of silently dying
CI / check (push) Failing after 13m37s
CI / check (push) Failing after 13m37s
The four live-rebuild sites (deferred rebuild on Join/Leave, idle SetNetworkMode, idle RegenerateIdentity) all did `net.shutdown().await` then `build_net_stack(...).await?` — a build failure propagated out of run_core_loop, which its supervisor only logs. Every subsequent command went nowhere: window alive, app dead, user told nothing. (The initial startup build already reported.) New replace_net_stack() helper: tear down the old stack, build for the requested posture, and on failure fall back to the posture the old stack was actually running (tracked in the new `net_mode` local; when the postures are equal the fallback is a plain retry — e.g. identity regeneration, where reverting the already-persisted key would be wrong). If the fallback lands, the UI is told the change didn't stick and `network_mode` reverts so state stays honest and the change stays re-attemptable. If both builds fail the UI gets a fatal 'Networking lost … restart' error before the loop exits — informed, not a zombie. Retry policy isolated in rebuild_with_fallback(), generic over the builder: 4 new unit tests cover first-try success, fall-back, plain retry, and double failure without binding sockets. Fixes finding 2 of the 2026-07-16 full-codebase review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
+244
-28
@@ -888,6 +888,103 @@ async fn build_net_stack(
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Retry policy for a live net-stack replacement, generic over the builder so
|
||||||
|
/// it is unit-testable without binding sockets: build for `requested`; if that
|
||||||
|
/// fails, build for `live` (the posture the old stack was actually running) so
|
||||||
|
/// a bad posture change degrades to the previous posture instead of leaving no
|
||||||
|
/// stack at all. When `requested == live` the second attempt is a plain retry.
|
||||||
|
///
|
||||||
|
/// `Ok((stack, mode, primary_err))` — a stack is up on `mode`; `primary_err`
|
||||||
|
/// is `Some` when the first attempt failed. `Err((primary, fallback))` — both
|
||||||
|
/// attempts failed and networking is gone.
|
||||||
|
async fn rebuild_with_fallback<T, E, F, Fut>(
|
||||||
|
mut build: F,
|
||||||
|
requested: NetworkMode,
|
||||||
|
live: NetworkMode,
|
||||||
|
) -> Result<(T, NetworkMode, Option<E>), (E, E)>
|
||||||
|
where
|
||||||
|
F: FnMut(NetworkMode) -> Fut,
|
||||||
|
Fut: std::future::Future<Output = Result<T, E>>,
|
||||||
|
{
|
||||||
|
match build(requested).await {
|
||||||
|
Ok(stack) => Ok((stack, requested, None)),
|
||||||
|
Err(primary) => match build(live).await {
|
||||||
|
Ok(stack) => Ok((stack, live, Some(primary))),
|
||||||
|
Err(fallback) => Err((primary, fallback)),
|
||||||
|
},
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Tear down `old` and stand up a replacement stack for `requested_mode`.
|
||||||
|
///
|
||||||
|
/// A build failure here is rare (only the local socket bind can fail; the
|
||||||
|
/// relay handshake is backgrounded), but it used to propagate straight out of
|
||||||
|
/// `run_core_loop` with no `UiEvent`, silently killing every future command —
|
||||||
|
/// the app looked alive and did nothing. Instead, fall back to `live_mode`
|
||||||
|
/// via `rebuild_with_fallback`, tell the UI when the requested change did not
|
||||||
|
/// stick, and return the mode the new stack actually runs so the caller can
|
||||||
|
/// keep its state honest. `Err` only when both builds fail: networking is
|
||||||
|
/// gone (already reported to the UI as fatal) and the caller should exit.
|
||||||
|
#[allow(clippy::too_many_arguments)]
|
||||||
|
async fn replace_net_stack(
|
||||||
|
old: NetStack,
|
||||||
|
what: &str,
|
||||||
|
secret_key: &SecretKey,
|
||||||
|
requested_mode: NetworkMode,
|
||||||
|
live_mode: NetworkMode,
|
||||||
|
friends_handler: &crate::presence_net::Handler,
|
||||||
|
publish: bool,
|
||||||
|
ui_tx: &mpsc::Sender<UiEvent>,
|
||||||
|
) -> Result<(NetStack, NetworkMode), anyhow::Error> {
|
||||||
|
let lookup = old.memory_lookup.clone();
|
||||||
|
old.shutdown().await;
|
||||||
|
let outcome = rebuild_with_fallback(
|
||||||
|
|mode| {
|
||||||
|
build_net_stack(
|
||||||
|
secret_key.clone(),
|
||||||
|
mode,
|
||||||
|
lookup.clone(),
|
||||||
|
friends_handler.clone(),
|
||||||
|
publish,
|
||||||
|
)
|
||||||
|
},
|
||||||
|
requested_mode,
|
||||||
|
live_mode,
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
match outcome {
|
||||||
|
Ok((stack, mode, None)) => Ok((stack, mode)),
|
||||||
|
Ok((stack, mode, Some(primary))) => {
|
||||||
|
if mode == requested_mode {
|
||||||
|
// Same-posture retry succeeded — everything the user asked for
|
||||||
|
// is in effect, so log it rather than raising a UI error.
|
||||||
|
crate::log_msg(&format!(
|
||||||
|
"{what}: net stack build failed once ({primary:#}); retry succeeded"
|
||||||
|
));
|
||||||
|
} else {
|
||||||
|
let _ = ui_tx
|
||||||
|
.send(UiEvent::Error(format!(
|
||||||
|
"{what} failed ({primary:#}); staying on the previous \
|
||||||
|
network mode for this session"
|
||||||
|
)))
|
||||||
|
.await;
|
||||||
|
}
|
||||||
|
Ok((stack, mode))
|
||||||
|
}
|
||||||
|
Err((primary, fallback)) => {
|
||||||
|
let _ = ui_tx
|
||||||
|
.send(UiEvent::Error(format!(
|
||||||
|
"Networking lost: {primary:#} (recovery attempt also failed: \
|
||||||
|
{fallback:#}). Restart PeerSpeak to reconnect."
|
||||||
|
)))
|
||||||
|
.await;
|
||||||
|
Err(anyhow::anyhow!(
|
||||||
|
"net stack rebuild failed: {primary:#}; fallback: {fallback:#}"
|
||||||
|
))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Maximum number of *automatic* chat-attachment fetches in flight at once.
|
/// Maximum number of *automatic* chat-attachment fetches in flight at once.
|
||||||
///
|
///
|
||||||
/// Auto-fetch (inline image preview) is triggered by an untrusted peer's chat
|
/// Auto-fetch (inline image preview) is triggered by an untrusted peer's chat
|
||||||
@@ -1335,6 +1432,10 @@ async fn run_core_loop(
|
|||||||
// rebuilt on the next Leave (or before the next Join), preserving the old
|
// rebuilt on the next Leave (or before the next Join), preserving the old
|
||||||
// "applies on next join" semantics while keeping the endpoint up while idle.
|
// "applies on next join" semantics while keeping the endpoint up while idle.
|
||||||
let mut net_rebuild_pending = false;
|
let mut net_rebuild_pending = false;
|
||||||
|
// The posture the live stack was actually built with. Trails `network_mode`
|
||||||
|
// while a rebuild is pending, and is the fallback posture when a rebuild
|
||||||
|
// fails (see `replace_net_stack`).
|
||||||
|
let mut net_mode = network_mode;
|
||||||
|
|
||||||
// When Discoverable is on, the instant it auto-reverts to Normal (W7 P6 time-box).
|
// When Discoverable is on, the instant it auto-reverts to Normal (W7 P6 time-box).
|
||||||
// `None` = not Discoverable, no pending revert. Set on SetPresenceMode(Discoverable),
|
// `None` = not Discoverable, no pending revert. Set on SetPresenceMode(Discoverable),
|
||||||
@@ -1552,17 +1653,24 @@ async fn run_core_loop(
|
|||||||
// active, rebuild the persistent stack now — after the old session is
|
// active, rebuild the persistent stack now — after the old session is
|
||||||
// gone, before the new one binds — so this join uses the new posture.
|
// gone, before the new one binds — so this join uses the new posture.
|
||||||
if net_rebuild_pending {
|
if net_rebuild_pending {
|
||||||
let lookup = net.memory_lookup.clone();
|
|
||||||
net.shutdown().await;
|
|
||||||
let publish = presence_mode.lock().unwrap().publishes_to_discovery();
|
let publish = presence_mode.lock().unwrap().publishes_to_discovery();
|
||||||
net = build_net_stack(
|
let (stack, live) = replace_net_stack(
|
||||||
secret_key.clone(),
|
net,
|
||||||
|
"Applying deferred network settings",
|
||||||
|
&secret_key,
|
||||||
network_mode,
|
network_mode,
|
||||||
lookup,
|
net_mode,
|
||||||
friends_handler.clone(),
|
&friends_handler,
|
||||||
publish,
|
publish,
|
||||||
|
&ui_tx,
|
||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
|
net = stack;
|
||||||
|
net_mode = live;
|
||||||
|
// If the new posture failed and we fell back, keep the mode
|
||||||
|
// state honest (and re-attemptable) rather than pretending
|
||||||
|
// the change applied. The join proceeds on the live stack.
|
||||||
|
network_mode = live;
|
||||||
net_rebuild_pending = false;
|
net_rebuild_pending = false;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -2594,17 +2702,21 @@ async fn run_core_loop(
|
|||||||
// Apply any network-mode / identity change that was deferred while we
|
// Apply any network-mode / identity change that was deferred while we
|
||||||
// were in the call (rebuild while idle keeps the endpoint reachable).
|
// were in the call (rebuild while idle keeps the endpoint reachable).
|
||||||
if net_rebuild_pending {
|
if net_rebuild_pending {
|
||||||
let lookup = net.memory_lookup.clone();
|
|
||||||
net.shutdown().await;
|
|
||||||
let publish = presence_mode.lock().unwrap().publishes_to_discovery();
|
let publish = presence_mode.lock().unwrap().publishes_to_discovery();
|
||||||
net = build_net_stack(
|
let (stack, live) = replace_net_stack(
|
||||||
secret_key.clone(),
|
net,
|
||||||
|
"Applying deferred network settings",
|
||||||
|
&secret_key,
|
||||||
network_mode,
|
network_mode,
|
||||||
lookup,
|
net_mode,
|
||||||
friends_handler.clone(),
|
&friends_handler,
|
||||||
publish,
|
publish,
|
||||||
|
&ui_tx,
|
||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
|
net = stack;
|
||||||
|
net_mode = live;
|
||||||
|
network_mode = live;
|
||||||
net_rebuild_pending = false;
|
net_rebuild_pending = false;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -2750,17 +2862,21 @@ async fn run_core_loop(
|
|||||||
// idle; if a call is active, defer to the next Leave/Join so the
|
// idle; if a call is active, defer to the next Leave/Join so the
|
||||||
// live call isn't disrupted (preserves "applies on next join").
|
// live call isn't disrupted (preserves "applies on next join").
|
||||||
if active_session.is_none() {
|
if active_session.is_none() {
|
||||||
let lookup = net.memory_lookup.clone();
|
|
||||||
net.shutdown().await;
|
|
||||||
let publish = presence_mode.lock().unwrap().publishes_to_discovery();
|
let publish = presence_mode.lock().unwrap().publishes_to_discovery();
|
||||||
net = build_net_stack(
|
let (stack, live) = replace_net_stack(
|
||||||
secret_key.clone(),
|
net,
|
||||||
|
"Network mode change",
|
||||||
|
&secret_key,
|
||||||
network_mode,
|
network_mode,
|
||||||
lookup,
|
net_mode,
|
||||||
friends_handler.clone(),
|
&friends_handler,
|
||||||
publish,
|
publish,
|
||||||
|
&ui_tx,
|
||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
|
net = stack;
|
||||||
|
net_mode = live;
|
||||||
|
network_mode = live;
|
||||||
} else {
|
} else {
|
||||||
net_rebuild_pending = true;
|
net_rebuild_pending = true;
|
||||||
}
|
}
|
||||||
@@ -2798,17 +2914,22 @@ async fn run_core_loop(
|
|||||||
// key unchanged, so a rebuild would be pointless churn).
|
// key unchanged, so a rebuild would be pointless churn).
|
||||||
if regenerated {
|
if regenerated {
|
||||||
if active_session.is_none() {
|
if active_session.is_none() {
|
||||||
let lookup = net.memory_lookup.clone();
|
|
||||||
net.shutdown().await;
|
|
||||||
let publish = presence_mode.lock().unwrap().publishes_to_discovery();
|
let publish = presence_mode.lock().unwrap().publishes_to_discovery();
|
||||||
net = build_net_stack(
|
// Same mode both attempts — the fallback is a plain
|
||||||
secret_key.clone(),
|
// retry under the (already persisted) new key.
|
||||||
|
let (stack, live) = replace_net_stack(
|
||||||
|
net,
|
||||||
|
"Endpoint restart after identity change",
|
||||||
|
&secret_key,
|
||||||
network_mode,
|
network_mode,
|
||||||
lookup,
|
net_mode,
|
||||||
friends_handler.clone(),
|
&friends_handler,
|
||||||
publish,
|
publish,
|
||||||
|
&ui_tx,
|
||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
|
net = stack;
|
||||||
|
net_mode = live;
|
||||||
} else {
|
} else {
|
||||||
net_rebuild_pending = true;
|
net_rebuild_pending = true;
|
||||||
}
|
}
|
||||||
@@ -3322,10 +3443,10 @@ fn replace_viewer_index<T>(viewers: &[(String, T)], ticket: &str) -> Option<usiz
|
|||||||
mod tests {
|
mod tests {
|
||||||
use super::{
|
use super::{
|
||||||
KnownPeers, MAX_OPUS_PAYLOAD, MAX_RETAINED_PEERS, MIC_LEVEL_REPORT_SAMPLES, MicLevelMeter,
|
KnownPeers, MAX_OPUS_PAYLOAD, MAX_RETAINED_PEERS, MIC_LEVEL_REPORT_SAMPLES, MicLevelMeter,
|
||||||
PLAYBACK_HANDOFF_QUEUE_FRAMES, PeerSpeakTicket, admit_retained, apply_peer_volume,
|
NetworkMode, PLAYBACK_HANDOFF_QUEUE_FRAMES, PeerSpeakTicket, admit_retained,
|
||||||
apply_volume, audio_datagram_len_ok, coalesce_insert, coalesce_pop, frame_level,
|
apply_peer_volume, apply_volume, audio_datagram_len_ok, coalesce_insert, coalesce_pop,
|
||||||
mix_frames, mix_stereo_frames, next_game_change, replace_viewer_index, send_playback_frame,
|
frame_level, mix_frames, mix_stereo_frames, next_game_change, rebuild_with_fallback,
|
||||||
should_auto_fetch, stereo_to_mono,
|
replace_viewer_index, send_playback_frame, should_auto_fetch, stereo_to_mono,
|
||||||
};
|
};
|
||||||
use crate::core::messages::{CoalesceKey, CoreCommand, coalesce_key};
|
use crate::core::messages::{CoalesceKey, CoreCommand, coalesce_key};
|
||||||
use std::collections::{HashMap, HashSet};
|
use std::collections::{HashMap, HashSet};
|
||||||
@@ -3349,6 +3470,101 @@ mod tests {
|
|||||||
assert_eq!(replace_viewer_index::<u8>(&[], "ticket-A"), None);
|
assert_eq!(replace_viewer_index::<u8>(&[], "ticket-A"), None);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// --- rebuild_with_fallback: the retry policy behind replace_net_stack ---
|
||||||
|
// The builder is injected, so these cover the policy without sockets. The
|
||||||
|
// closure does its bookkeeping synchronously and returns a ready future.
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn rebuild_keeps_requested_posture_on_first_success() {
|
||||||
|
let calls = std::cell::RefCell::new(Vec::new());
|
||||||
|
let out = rebuild_with_fallback(
|
||||||
|
|mode| {
|
||||||
|
calls.borrow_mut().push(mode);
|
||||||
|
std::future::ready(Ok::<u8, String>(7))
|
||||||
|
},
|
||||||
|
NetworkMode::DirectOnly,
|
||||||
|
NetworkMode::N0Full,
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
assert_eq!(out, Ok((7, NetworkMode::DirectOnly, None)));
|
||||||
|
// No second build: the live posture is only a fallback.
|
||||||
|
assert_eq!(*calls.borrow(), vec![NetworkMode::DirectOnly]);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn rebuild_falls_back_to_the_live_posture_when_the_requested_one_fails() {
|
||||||
|
let calls = std::cell::RefCell::new(Vec::new());
|
||||||
|
let out = rebuild_with_fallback(
|
||||||
|
|mode| {
|
||||||
|
calls.borrow_mut().push(mode);
|
||||||
|
std::future::ready(if mode == NetworkMode::DirectOnly {
|
||||||
|
Err("bind failed".to_string())
|
||||||
|
} else {
|
||||||
|
Ok(7u8)
|
||||||
|
})
|
||||||
|
},
|
||||||
|
NetworkMode::DirectOnly,
|
||||||
|
NetworkMode::N0Full,
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
// A stack is up on the OLD posture and the caller learns both that it
|
||||||
|
// fell back (mode) and why (the primary error) — no silent zombie.
|
||||||
|
assert_eq!(
|
||||||
|
out,
|
||||||
|
Ok((7, NetworkMode::N0Full, Some("bind failed".to_string())))
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
*calls.borrow(),
|
||||||
|
vec![NetworkMode::DirectOnly, NetworkMode::N0Full]
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn rebuild_reports_both_errors_when_networking_is_gone() {
|
||||||
|
let out = rebuild_with_fallback(
|
||||||
|
|_| std::future::ready(Err::<u8, String>("bind failed".to_string())),
|
||||||
|
NetworkMode::DirectOnly,
|
||||||
|
NetworkMode::N0Full,
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
assert_eq!(
|
||||||
|
out,
|
||||||
|
Err(("bind failed".to_string(), "bind failed".to_string()))
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn rebuild_with_equal_postures_is_a_plain_retry() {
|
||||||
|
// RegenerateIdentity rebuilds under the same mode: the fallback is a
|
||||||
|
// second attempt with identical parameters, not a posture change.
|
||||||
|
let calls = std::cell::Cell::new(0u8);
|
||||||
|
let out = rebuild_with_fallback(
|
||||||
|
|mode| {
|
||||||
|
calls.set(calls.get() + 1);
|
||||||
|
assert_eq!(mode, NetworkMode::RelayNoDiscovery);
|
||||||
|
std::future::ready(if calls.get() == 1 {
|
||||||
|
Err("transient".to_string())
|
||||||
|
} else {
|
||||||
|
Ok(7u8)
|
||||||
|
})
|
||||||
|
},
|
||||||
|
NetworkMode::RelayNoDiscovery,
|
||||||
|
NetworkMode::RelayNoDiscovery,
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
// Succeeded on the requested posture, so the caller treats the change
|
||||||
|
// as applied (the Some(err) is logged, not surfaced as a UI error).
|
||||||
|
assert_eq!(
|
||||||
|
out,
|
||||||
|
Ok((
|
||||||
|
7,
|
||||||
|
NetworkMode::RelayNoDiscovery,
|
||||||
|
Some("transient".to_string())
|
||||||
|
))
|
||||||
|
);
|
||||||
|
assert_eq!(calls.get(), 2);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn admit_retained_rejects_only_new_ids_at_the_cap() {
|
fn admit_retained_rejects_only_new_ids_at_the_cap() {
|
||||||
// Below the cap, a brand-new identity is retained.
|
// Below the cap, a brand-new identity is retained.
|
||||||
|
|||||||
Reference in New Issue
Block a user