From 729cee573ad6cd5a4b5c6f36db214316a1f5287f Mon Sep 17 00:00:00 2001 From: ssmithx Date: Thu, 23 Jul 2026 18:39:33 +0000 Subject: [PATCH] fix(mesh): orphaned listener task could race a fresh one on the same port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Traced why a device that's alive and USB-enumerating correctly could still never complete a single protocol handshake, indefinitely: journal logs showed genuinely concurrent connection attempts on the same port (duplicate "Opened serial port"/"Starting X handshake" lines within microseconds of each other, from what should be sequential probe steps) — two independent listener sessions were racing on the same tty, each corrupting the other's reads/writes. Root cause was in MeshService::stop() combined with mesh::flash's earlier STOP_LISTENER_TIMEOUT fix: stop() calls `self.listener_handle.take()` (clearing the field to None immediately) and then awaits the handle with no bound of its own. When a caller wraps the whole stop() call in a timeout (as the flash job does, to avoid hanging the RPC response), a slow listener — mid multi-candidate probe when the shutdown signal arrives — would cause that outer timeout to cancel the await. But by then `listener_handle` was already None, so MeshService believed the listener was stopped, while the actual task kept running, orphaned (dropping a JoinHandle does not abort the task). A later start() then spawned a genuinely new listener, racing the orphaned one forever on the same port. Fixed at the source: stop() now awaits its own handle through a mutable reference (not by value) with its own bounded timeout, so on timeout it still owns the handle and can call .abort() on it directly — guaranteeing the task is actually gone before stop() returns, regardless of how any caller wraps it. Co-Authored-By: Claude Sonnet 5 (cherry picked from commit b14af20d1acfc053957ec22234773b5c59582a66) --- core/archipelago/src/mesh/mod.rs | 40 ++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/core/archipelago/src/mesh/mod.rs b/core/archipelago/src/mesh/mod.rs index 4b2b76bf..ae2d9389 100644 --- a/core/archipelago/src/mesh/mod.rs +++ b/core/archipelago/src/mesh/mod.rs @@ -38,6 +38,14 @@ use tokio::sync::watch; use tracing::{error, info, warn}; const MESH_CONFIG_FILE: &str = "mesh-config.json"; + +/// How long `MeshService::stop()` waits for the listener task to notice its +/// shutdown signal and exit gracefully before force-aborting it. See +/// `stop()`'s doc comment for the real incident this guards against: without +/// a hard abort fallback, a slow-to-notice listener could be left running +/// forever, orphaned, racing a later independently-started listener on the +/// same serial port. +const LISTENER_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(15); const MESH_IGNORED_RADIO_FILE: &str = "mesh-ignored-radio-contacts.json"; const MESH_CONTACTS_FILE: &str = "mesh-contacts.json"; @@ -967,8 +975,36 @@ impl MeshService { if let Some(tx) = self.shutdown_tx.take() { let _ = tx.send(true); } - if let Some(handle) = self.listener_handle.take() { - let _ = handle.await; + if let Some(mut handle) = self.listener_handle.take() { + // Bounded wait for graceful shutdown, with a hard abort as + // fallback — confirmed live 2026-07-23: a caller-side timeout + // wrapping stop() (mesh::flash's STOP_LISTENER_TIMEOUT) cancelled + // this await when the listener was slow to notice its shutdown + // signal (mid multi-candidate probe), but `.take()` above had + // already cleared `listener_handle` to None — so MeshService + // believed it was stopped while the task kept running, orphaned + // (dropping a JoinHandle does not abort the task it points to). + // A later start() then spawned a second, fully independent + // listener session racing the orphaned one on the same serial + // port — neither could ever get a clean response, so every + // mesh.configure/probe against that device failed indefinitely + // even though the device itself was fine. + // + // Awaiting `&mut handle` (not `handle` by value) is what makes + // the fallback possible: the Future is polled through the + // reference, so if the timeout fires, this task's own `handle` + // binding is still ours to call `.abort()` on afterward — + // unlike moving `handle` into the timeout future outright, which + // would drop (and thus orphan) it on timeout with nothing left + // to abort. + if tokio::time::timeout(LISTENER_SHUTDOWN_TIMEOUT, &mut handle) + .await + .is_err() + { + warn!("Mesh listener did not shut down gracefully in time — aborting it"); + handle.abort(); + let _ = handle.await; + } } if let Some(handle) = self.deadman_handle.take() { handle.abort();