From 3d4e4d278c3b31a247b792afa0917a7c92bc3f5a Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Wed, 13 May 2026 08:25:03 +0530 Subject: [PATCH] fix(webview_apis): always bind ephemeral port, ignore stale PORT_ENV (OPENHUMAN-TAURI-82) (#1543) Co-authored-by: Claude Opus 4.7 (1M context) Co-authored-by: Steven Enamakel --- app/src-tauri/src/webview_apis/mod.rs | 17 +++-- app/src-tauri/src/webview_apis/server.rs | 88 +++++++++++++++++++++--- 2 files changed, 89 insertions(+), 16 deletions(-) diff --git a/app/src-tauri/src/webview_apis/mod.rs b/app/src-tauri/src/webview_apis/mod.rs index e18098a84..1b82f5efa 100644 --- a/app/src-tauri/src/webview_apis/mod.rs +++ b/app/src-tauri/src/webview_apis/mod.rs @@ -27,13 +27,18 @@ //! //! ## Startup / port coordination //! -//! The server picks its port at boot: -//! 1. If `OPENHUMAN_WEBVIEW_APIS_PORT` is set, try that port first. -//! 2. Else bind `127.0.0.1:0` and let the OS pick. +//! The server always binds `127.0.0.1:0` and lets the OS pick an +//! ephemeral port. The resolved port is exposed via [`resolved_port`] +//! and pushed into the core sidecar's environment as +//! `OPENHUMAN_WEBVIEW_APIS_PORT` by `core_process::spawn_core` so the +//! client side can find it. //! -//! Either way the resolved port is exposed via -//! [`resolved_port`] and pushed into the core sidecar's environment -//! as `OPENHUMAN_WEBVIEW_APIS_PORT` by `core_process::spawn_core`. +//! `OPENHUMAN_WEBVIEW_APIS_PORT` is an **output** of the bridge — it is +//! intentionally never read as input. Honouring a pre-existing value +//! was the cause of Sentry OPENHUMAN-TAURI-82 on Windows: a stale env +//! value left over from a prior run (or inherited from a parent +//! process) led the next launch to re-bind the exact same port and +//! fail with WSAEADDRINUSE (`os error 10048`). pub mod router; pub mod server; diff --git a/app/src-tauri/src/webview_apis/server.rs b/app/src-tauri/src/webview_apis/server.rs index 88bec29e1..a538fe9f1 100644 --- a/app/src-tauri/src/webview_apis/server.rs +++ b/app/src-tauri/src/webview_apis/server.rs @@ -43,20 +43,24 @@ pub fn resolved_port() -> u16 { /// Start the server. Idempotent: after the first successful call any /// subsequent call is a no-op. Returns the bound port. /// -/// Port selection: if `PORT_ENV` is set and non-zero, bind that port -/// (caller gets a deterministic port across runs — useful in dev); -/// otherwise bind `127.0.0.1:0` and let the OS pick. +/// Port selection: always bind `127.0.0.1:0` and let the OS pick an +/// ephemeral port. The resolved port is then exported via `PORT_ENV` +/// (by the caller in `lib.rs`) so the core sidecar can discover it. +/// +/// We deliberately ignore any pre-existing `PORT_ENV` value here: +/// honouring it caused Sentry OPENHUMAN-TAURI-82 on Windows — if a +/// previous run wrote `PORT_ENV=49342` into the user's environment +/// (or the env was inherited from a parent process / leftover dev +/// session), the next launch would attempt to re-bind that exact +/// port and fail with WSAEADDRINUSE / os error 10048 whenever the +/// socket was still held by another process or stuck in TIME_WAIT. +/// `PORT_ENV` is an *output* of the bridge, not an input. pub async fn start() -> Result { if STARTED.get().is_some() { return Ok(resolved_port()); } - let requested = std::env::var(PORT_ENV) - .ok() - .and_then(|s| s.parse::().ok()) - .unwrap_or(0); - - let addr: SocketAddr = format!("127.0.0.1:{requested}") + let addr: SocketAddr = "127.0.0.1:0" .parse() .map_err(|e| format!("[webview_apis] bad addr: {e}"))?; let listener = TcpListener::bind(addr) @@ -69,7 +73,7 @@ pub async fn start() -> Result { RESOLVED_PORT.store(port, Ordering::SeqCst); let _ = STARTED.set(()); - log::info!("[webview_apis] server listening on {bound}"); + log::info!("[webview_apis] server listening on {bound} (OS-assigned ephemeral)"); let accept_handle = tokio::spawn(async move { loop { @@ -264,3 +268,67 @@ impl Response { } } } + +#[cfg(test)] +mod tests { + use super::*; + + /// Regression test for Sentry OPENHUMAN-TAURI-82. + /// + /// If `PORT_ENV` carries a stale value pointing at a port that is + /// already in use (the failure mode reported on Windows: a previous + /// run wrote `49342` into the env, then the same port was held by + /// another process / stuck in TIME_WAIT), `start()` must still + /// succeed by binding a fresh OS-assigned ephemeral port instead of + /// trying to re-bind the stale port. + // Single-threaded runtime: `std::env::set_var` mutates process-global + // state and is not thread-safe in Rust. Under the default multi-threaded + // tokio test runtime, threads spawned by the same test could observe + // the env between `set_var` and the restore. `current_thread` eliminates + // that intra-test window (cross-test races between OS threads from + // OTHER tests are still possible — see the save/restore pattern below + // for that part). Per graycyrus review on PR #1543. + #[tokio::test(flavor = "current_thread")] + async fn start_ignores_stale_port_env_and_binds_ephemeral() { + // Occupy a port so `PORT_ENV` points at something that would + // definitely fail if `start()` honoured it. + let blocker = TcpListener::bind("127.0.0.1:0") + .await + .expect("blocker bind"); + let stale_port = blocker.local_addr().expect("blocker addr").port(); + // Save+restore `PORT_ENV` so parallel tests in the same process + // don't see this test's mutation. (Per CodeRabbit feedback on PR + // #1543.) `std::env::set_var` is process-global; without the + // restore, an unrelated test asserting on `PORT_ENV` could observe + // `stale_port` and flake. + let prev_port_env = std::env::var(PORT_ENV).ok(); + std::env::set_var(PORT_ENV, stale_port.to_string()); + + let bound = start() + .await + .expect("start should succeed despite stale PORT_ENV"); + + assert_ne!( + bound, stale_port, + "start() must pick a fresh ephemeral port, not the stale one in PORT_ENV" + ); + assert_eq!(resolved_port(), bound); + + // Hold `blocker` until after `start()` so the kernel definitely + // can't satisfy a bind on `stale_port` — defends against the + // exact race the Sentry issue describes. + drop(blocker); + // Note: `stop()` only aborts the accept loop — it does NOT (and + // cannot) reset the `STARTED` OnceLock. If a second test in this + // binary later calls `start()` it'll hit the idempotency + // early-return and silently observe this test's port. A future + // refactor to `AtomicBool`-based singleton would let `stop()` + // fully tear down. Tracked as graycyrus feedback on PR #1543; + // currently inert because this is the only test in the module. + stop(); + match prev_port_env { + Some(v) => std::env::set_var(PORT_ENV, v), + None => std::env::remove_var(PORT_ENV), + } + } +}