fix(friends): five robustness bugs in the friends/control plane
Found in a bug audit of the just-merged friends-list feature. No crashes or security holes, but five real state/correctness bugs: - Host child dying on its own left the share campaign running, so it kept pushing a now-dead ticket to friends (retrying offline ones forever) and leaked share_status/met/share_code. The unexpected-exit path now captures the stderr error, then routes through the full stop_host() teardown (notably stop_share). (gui/mod.rs pump_host_events) - on_friend_request downgraded an already-Accepted friend back to PendingIncoming when they re-sent a request (e.g. after losing their store). It now stays Accepted and re-confirms. (friends.rs) - on_friend_accept advanced *any* known peer to Accepted, including a PendingIncoming one — a peer could mark itself accepted without the local user's consent. Now only a PendingOutgoing request we sent is honoured. (friends.rs) - A ShareCode redelivered by an ACK-loss retry fired a duplicate desktop notification. push_notice now reports whether the code is new/changed and only then toasts. (gui/mod.rs) - An inbound control message could be delayed up to IO_TIMEOUT on a degraded link because handle() awaited the sender's close before forwarding it. Forward to the UI first, then await close so the ACK still flushes. (control.rs) Adds two friends-store transition tests (accept ignores a pending-incoming peer; request doesn't downgrade an accepted friend). 47 gui / 8 headless tests pass, clippy + fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+37
-17
@@ -1145,11 +1145,14 @@ impl PixelPassApp {
|
||||
f.name = name.clone();
|
||||
store_changed = true;
|
||||
}
|
||||
self.push_notice(from, name.clone(), ticket);
|
||||
notify(
|
||||
"PixelPass — a friend is sharing",
|
||||
format!("{name} is sharing their screen. Open PixelPass to watch."),
|
||||
);
|
||||
// Only toast for a new/changed code — an ACK-loss retry
|
||||
// redelivers the same code and shouldn't fire again.
|
||||
if self.push_notice(from, name.clone(), ticket) {
|
||||
notify(
|
||||
"PixelPass — a friend is sharing",
|
||||
format!("{name} is sharing their screen. Open PixelPass to watch."),
|
||||
);
|
||||
}
|
||||
} else {
|
||||
tracing::warn!(from = %from, "presence: ignoring ShareCode from a non-friend");
|
||||
}
|
||||
@@ -1197,13 +1200,20 @@ impl PixelPassApp {
|
||||
}
|
||||
|
||||
/// Record a share code a friend pushed us, replacing any prior notice from
|
||||
/// the same friend (their previous code is stale once they re-host).
|
||||
fn push_notice(&mut self, from: iroh::EndpointId, name: String, code: String) {
|
||||
/// the same friend (their previous code is stale once they re-host). Returns
|
||||
/// `true` if this is a new notice or a *different* code than we already had
|
||||
/// from them — i.e. worth a fresh desktop notification. A duplicate delivery
|
||||
/// (an ACK-loss retry redelivering the same code) updates in place and
|
||||
/// returns `false`, so it doesn't fire a second toast.
|
||||
fn push_notice(&mut self, from: iroh::EndpointId, name: String, code: String) -> bool {
|
||||
if let Some(n) = self.notices.iter_mut().find(|n| n.from == from) {
|
||||
let changed = n.code != code;
|
||||
n.name = name;
|
||||
n.code = code;
|
||||
changed
|
||||
} else {
|
||||
self.notices.push(ShareNotice { from, name, code });
|
||||
true
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2318,19 +2328,29 @@ impl PixelPassApp {
|
||||
self.apply_host_event(ev);
|
||||
}
|
||||
|
||||
if let Some(p) = &mut self.host.proc
|
||||
&& !p.is_alive()
|
||||
{
|
||||
if self.host.ticket.is_none() {
|
||||
let tail = p.stderr_tail();
|
||||
self.host.error = Some(if tail.trim().is_empty() {
|
||||
let dead = self.host.proc.as_mut().is_some_and(|p| !p.is_alive());
|
||||
if dead {
|
||||
// If it never reached a ticket, capture why (from the stderr tail)
|
||||
// before tearing down. Then run the *full* Stop cleanup — most
|
||||
// importantly stop_share, so a host that died on its own stops
|
||||
// pushing its now-dead code to friends. Without this the campaign
|
||||
// would keep retrying offline friends with a stale ticket for the
|
||||
// life of the GUI, and share_status/met/share_code would leak.
|
||||
let error = self.host.ticket.is_none().then(|| {
|
||||
let tail = self
|
||||
.host
|
||||
.proc
|
||||
.as_mut()
|
||||
.map(|p| p.stderr_tail())
|
||||
.unwrap_or_default();
|
||||
if tail.trim().is_empty() {
|
||||
"Host exited before it could start.".to_string()
|
||||
} else {
|
||||
format!("Host exited before it could start:\n{tail}")
|
||||
});
|
||||
}
|
||||
self.host.proc = None;
|
||||
self.host.capturing = false;
|
||||
}
|
||||
});
|
||||
self.stop_host();
|
||||
self.host.error = error;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user