Skip to content

Commit 7c66b05

Browse files
maxholmanclaude
andcommitted
fix(entry): prevent stale peer eviction and TUN EBUSY on reconnect
Three interconnected TUN lifecycle bugs blocked reliable reconnects: 1. **Stale peer registration race:** Accept loop registers a new connection before the old task's unregister fires, evicting the new peer. Fix: monotonic connection_id on PeerInfo; unregister_if_current only removes when the ID matches. 2. **TUN EBUSY on reconnect:** SYN probe tasks held Arc<AsyncFd<Device>> clones after ConnectionManager exited. Fix: probe tasks run in a JoinSet that is drained before returning; run_connection_loop aborts+joins the manager task; SessionManager.get_or_create preemptively deletes stale TUN interfaces before reuse. 3. **TUN deletion before route cleanup:** delete_tun ran inside ConnectionParams::run() before the outer task removed OS routes. Fix: delete_tun sequenced after run_connection_loop abort+join (fd released); TunDropGuard added for panic safety. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent d342586 commit 7c66b05

4 files changed

Lines changed: 195 additions & 47 deletions

File tree

TODO.md

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,22 @@
1111
- [ ] Reduce global lock contention in entry-stack
1212
- [x] ~~Broadcast → mpsc migration on data path~~ — done.
1313

14+
## Relay
15+
16+
- [ ] **Bidi stream bridging** — relay currently bridges uni streams only.
17+
Bidi streams (used by SYN probes and TCP data sessions) go point-to-point
18+
and are invisible to the relay. Entry's `probe_tcp_target` and
19+
`run_tcp_session` both open bidi streams that hang when a relay is in
20+
the path. Fix: relay accepts bidi from one side, opens matching bidi to
21+
the other side, splices with `copy_bidirectional`. Unblocks SYN probes
22+
and TCP data flow through relay chains without any entry/exit changes.
23+
- [ ] **Topology visibility** — entry has no visibility into peers behind a
24+
relay. `wallhack peers` shows the relay but not the exit nodes connected
25+
to it. Relay should forward downstream peer identity (name, capabilities,
26+
routes) upstream via a peer announcement control message or augmented
27+
handshake. Needed for topology observability, debugging, and multi-hop
28+
route selection.
29+
1430
## Transports
1531

1632
- [ ] DNS over tunnel — intercept DNS queries at the TUN interface on the exit
@@ -82,11 +98,8 @@
8298
impossible to disconnect. Need either address-based disconnect
8399
(`disconnect --addr 10.99.1.10:43006`) or a short connection ID shown in
84100
`wallhack peers` output that can be passed to `disconnect`.
85-
- [ ] **Stale peer not cleaned up after remote restart** — when a peer process is
86-
killed and restarts (e.g. gateway-perimeter exit→relay), the old connection
87-
lingers in `wallhack peers` as a second entry for the same host alongside the
88-
new connection. Two peers for the same physical node is confusing and indicates
89-
the old session wasn't properly torn down.
101+
- [x] ~~**Stale peer not cleaned up after remote restart**~~ — fixed by
102+
connection ID–guarded `unregister_if_current` in PR #82.
90103
- [ ] **`status=connected latency=— tun=false listen=false connect=false` is an
91104
impossible state**`wallhack peers` shows a connected peer with all capability
92105
flags false and no latency. If `status=connected` then either `listen` or
@@ -105,16 +118,12 @@
105118
(`peer_name_to_iface`) so TUN gets a random name instead of `wh{hash}`.
106119
- [ ] **Relay peer role reported as `exit`**`wallhack peers` shows relay peers
107120
as `role=exit`. Should report `role=relay`.
108-
- [ ] **Stale TUN interfaces not cleaned up on disconnect** — when a peer disconnects
109-
(or restarts with a different role), the old TUN interface remains on the entry
110-
node and kernel routes pointing to it linger. Auto-cleanup on disconnect needed.
111-
- [ ] TUN EBUSY on rapid reconnect — `create_tun_with_retry` (entry) retries
112-
3× at 500ms but the previous `TunActor` hasn't been fully dropped before
113-
the new connection attempts to claim the same TUN name. Rapid
114-
connect/disconnect cycles accumulate stale connections, eventually causing
115-
resource exhaustion and process kill (OOM or SIGKILL). Needs proper TUN
116-
lifecycle tracking — ensure the old actor is fully dropped before allowing
117-
a new connection to reuse the name.
121+
- [x] ~~**Stale TUN interfaces not cleaned up on disconnect**~~ — fixed in PR #82:
122+
`delete_tun` runs after manager task abort+join; kernel auto-removes routes.
123+
- [x] ~~TUN EBUSY on rapid reconnect~~ — fixed in PR #82:
124+
`SessionManager.get_or_create` preemptively deletes stale TUN;
125+
`run_connection_loop` aborts+joins manager task to release fd Arcs;
126+
`TunDropGuard` for panic safety.
118127
- [x] ~~No color in `[+]` notification messages~~ — done, uses `nu-ansi-term`
119128
behind `repl` feature gate.
120129
- [ ] Log prefix inconsistency in REPL — mix of `warn:` prefix (from

crates/core/src/control/peers.rs

Lines changed: 64 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@
22
33
use std::{
44
collections::HashMap,
5-
sync::Arc,
5+
sync::{
6+
Arc,
7+
atomic::{AtomicU64, Ordering},
8+
},
69
time::{Duration, Instant},
710
};
811

@@ -70,6 +73,8 @@ pub struct PeerInfo {
7073
pub latency_measured_at: Option<Instant>,
7174
/// TUN interface name for this peer (entry-side only).
7275
pub tun_name: Option<String>,
76+
/// Unique identifier for this connection instance.
77+
pub connection_id: u64,
7378
}
7479

7580
/// Shared peer registry.
@@ -85,6 +90,8 @@ pub struct Registry {
8590
ping_channels: ArcSwap<HashMap<String, mpsc::Sender<PingRequest>>>,
8691
/// Broadcast channel for peer lifecycle events.
8792
events_tx: broadcast::Sender<PeerEvent>,
93+
/// Monotonic counter for assigning unique connection IDs.
94+
next_connection_id: AtomicU64,
8895
}
8996

9097
impl Default for Registry {
@@ -94,6 +101,7 @@ impl Default for Registry {
94101
peers: ArcSwap::from_pointee(HashMap::new()),
95102
ping_channels: ArcSwap::from_pointee(HashMap::new()),
96103
events_tx,
104+
next_connection_id: AtomicU64::new(0),
97105
}
98106
}
99107
}
@@ -122,7 +130,12 @@ impl Registry {
122130
}
123131

124132
/// Register a new peer.
125-
pub fn register(&self, id: String, addr: String, role: NodeRole, side: ConnectionSide) {
133+
///
134+
/// Returns a connection ID that must be passed to
135+
/// [`unregister_if_current`] to prevent a stale task from evicting
136+
/// a newer connection that re-registered under the same name.
137+
pub fn register(&self, id: String, addr: String, role: NodeRole, side: ConnectionSide) -> u64 {
138+
let connection_id = self.next_connection_id.fetch_add(1, Ordering::Relaxed) + 1;
126139
// TOCTOU: another thread could register the same id between this
127140
// check and the rcu insert. Harmless — a duplicate Connected event
128141
// is better than a missed one, and callers don't race on the same id.
@@ -138,6 +151,7 @@ impl Registry {
138151
latency_ms: None,
139152
latency_measured_at: None,
140153
tun_name: None,
154+
connection_id,
141155
};
142156
let event_addr = info.addr.clone();
143157
let event_role = info.role;
@@ -154,6 +168,7 @@ impl Registry {
154168
role: event_role,
155169
});
156170
}
171+
connection_id
157172
}
158173

159174
/// Set the TUN interface name for a peer.
@@ -179,7 +194,7 @@ impl Registry {
179194
});
180195
}
181196

182-
/// Unregister a peer.
197+
/// Unregister a peer unconditionally.
183198
pub fn unregister(&self, id: &str) -> Option<PeerInfo> {
184199
self.ping_channels.rcu(|old| {
185200
let mut new = (**old).clone();
@@ -200,6 +215,25 @@ impl Registry {
200215
removed
201216
}
202217

218+
/// Unregister a peer only if its connection ID matches.
219+
///
220+
/// Prevents a stale task from evicting a newer connection that
221+
/// re-registered under the same name between the old task's exit
222+
/// and its cleanup.
223+
pub fn unregister_if_current(&self, id: &str, connection_id: u64) -> Option<PeerInfo> {
224+
let current = self.peers.load().get(id).map(|p| p.connection_id);
225+
if current != Some(connection_id) {
226+
tracing::debug!(
227+
peer = id,
228+
current = ?current,
229+
requested = connection_id,
230+
"skipping stale unregister"
231+
);
232+
return None;
233+
}
234+
self.unregister(id)
235+
}
236+
203237
/// Register a ping channel for a peer's connection handler.
204238
///
205239
/// Returns the receiver that the connection handler should listen on.
@@ -475,4 +509,31 @@ mod tests {
475509

476510
assert!(rx.try_recv().is_err());
477511
}
512+
513+
#[test]
514+
fn test_connection_id_prevents_stale_unregister() {
515+
let registry = Registry::new();
516+
let gen1 = registry.register(
517+
"peer1".into(),
518+
"1.2.3.4:5678".into(),
519+
NodeRole::Exit,
520+
ConnectionSide::Accept,
521+
);
522+
// Re-register same peer (new connection arrived before old task cleaned up).
523+
let gen2 = registry.register(
524+
"peer1".into(),
525+
"1.2.3.4:9999".into(),
526+
NodeRole::Exit,
527+
ConnectionSide::Accept,
528+
);
529+
assert_ne!(gen1, gen2);
530+
531+
// Old task tries to unregister with stale connection_id — should be a no-op.
532+
assert!(registry.unregister_if_current("peer1", gen1).is_none());
533+
assert_eq!(registry.count(), 1);
534+
535+
// New task unregisters with current connection_id — should succeed.
536+
assert!(registry.unregister_if_current("peer1", gen2).is_some());
537+
assert_eq!(registry.count(), 0);
538+
}
478539
}

crates/core/src/entry/manager.rs

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ use smoltcp::phy::Device;
55
use tokio::{
66
io::unix::AsyncFd,
77
sync::{Notify, mpsc},
8+
task::JoinSet,
89
time::Instant,
910
};
1011
use wallhack_entry_stack::async_stack::{
@@ -61,7 +62,7 @@ pub struct ConnectionManager<D: Device + Send + 'static> {
6162
egress_rx: tokio::sync::mpsc::UnboundedReceiver<Vec<u8>>,
6263
/// Intercepted ICMP Echo Requests from the poll loop for tunnel forwarding.
6364
icmp_rx: tokio::sync::mpsc::UnboundedReceiver<Vec<u8>>,
64-
/// Unified data stream: send UDP instructions to the exit node.
65+
/// Unified data stream: send instructions to the exit node.
6566
instructions_tx: mpsc::Sender<EntryNodeInstruction>,
6667
/// Unified data stream: receive responses from the exit node.
6768
responses_rx: mpsc::Receiver<ExitNodeResponse>,
@@ -116,7 +117,11 @@ impl<D: Device + Send + 'static> ConnectionManager<D> {
116117
let udp_timeout = Duration::from_secs(30);
117118
let mut udp_buf = vec![0u8; 65535];
118119

119-
loop {
120+
// Legacy probe tasks (fallback bidi-stream probes) in a JoinSet so
121+
// they are aborted on exit, releasing Arc<AsyncFd<Device>> clones.
122+
let mut probe_tasks: JoinSet<()> = JoinSet::new();
123+
124+
let result = loop {
120125
tokio::select! {
121126
stream = listener.accept() => {
122127
let stream = stream?;
@@ -187,7 +192,7 @@ impl<D: Device + Send + 'static> ConnectionManager<D> {
187192
};
188193
if self.instructions_tx.send(instr).await.is_err() {
189194
tracing::debug!("UDP: instructions channel closed, stopping");
190-
return Ok(());
195+
break Ok(());
191196
}
192197
self.metrics.inc_packets_out(1);
193198
self.metrics.inc_bytes_out(size as u64);
@@ -197,7 +202,7 @@ impl<D: Device + Send + 'static> ConnectionManager<D> {
197202
self.handle_exit_response(&mut udp, response);
198203
} else {
199204
tracing::debug!("Responses channel closed, connection dead");
200-
return Ok(());
205+
break Ok(());
201206
}
202207
}
203208
Some(held) = self.syn_rx.recv() => {
@@ -207,7 +212,7 @@ impl<D: Device + Send + 'static> ConnectionManager<D> {
207212
let tun = Arc::clone(&self.tun_writer);
208213
let wake = Arc::clone(&self.wake_notify);
209214
let dst_addr = held.dst_addr;
210-
tokio::spawn(async move {
215+
probe_tasks.spawn(async move {
211216
let Some(target_addr) = parse_syn_target(&held.packet) else {
212217
tracing::debug!(%dst_addr, "SYN probe: failed to parse target");
213218
state.mark_unreachable(dst_addr);
@@ -240,6 +245,12 @@ impl<D: Device + Send + 'static> ConnectionManager<D> {
240245
wake.notify_one();
241246
});
242247
}
248+
// Reap completed probe tasks so the JoinSet doesn't grow unbounded.
249+
Some(result) = probe_tasks.join_next(), if !probe_tasks.is_empty() => {
250+
if let Err(e) = result {
251+
tracing::warn!("SYN probe task panicked: {e}");
252+
}
253+
}
243254
Some(icmp_packet) = self.egress_rx.recv() => {
244255
// ICMP packets from the poll loop (cache-hit retransmits).
245256
write_raw_to_tun(&self.tun_writer, &icmp_packet);
@@ -251,7 +262,7 @@ impl<D: Device + Send + 'static> ConnectionManager<D> {
251262
tracing::trace!(len = icmp_pkt.len(), "forwarding ICMP echo request to exit");
252263
if self.instructions_tx.send(instr).await.is_err() {
253264
tracing::debug!("ICMP: instructions channel closed, stopping");
254-
return Ok(());
265+
break Ok(());
255266
}
256267
self.metrics.inc_packets_out(1);
257268
self.metrics.inc_bytes_out(icmp_pkt.len() as u64);
@@ -271,7 +282,15 @@ impl<D: Device + Send + 'static> ConnectionManager<D> {
271282
});
272283
}
273284
}
274-
}
285+
};
286+
287+
// Abort in-flight probe tasks and wait for cleanup so their
288+
// Arc<AsyncFd<Device>> clones are dropped before the caller
289+
// deletes the TUN interface.
290+
probe_tasks.abort_all();
291+
while probe_tasks.join_next().await.is_some() {}
292+
293+
result
275294
}
276295

277296
#[allow(clippy::too_many_lines)]

0 commit comments

Comments
 (0)