From 438a46b80ff3ff45f707b9f7ed0d89332d21641c Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Tue, 29 Sep 2026 23:28:40 +0100 Subject: [PATCH] fix(serve): drop idle socket clients so the accept loop keeps serving Bug: --serve in socket mode serves one client at a time: the accept loop blocks in the per-client line read before it reaches the next connection, and this path never called set_read_timeout - unlike rest.rs, http_bridge.rs and control::transport, which all use 15 seconds. A client that connects and sends nothing held the read open forever, wedging the port: every later client hung waiting for the ready banner (the banner is written before the read loop, so the server looked healthy while it was stuck). Reproduce: 1. Start OpenCADStudio --serve --port 8765. 2. From any shell, connect and send nothing: python -c "import socket,time;socket.create_connection(('127.0.0.1',8765));time.sleep(3600)" 3. From a second shell, connect and wait for the banner. 4. Before: the second client hangs forever; the port never serves another connection. 5. After: the idle first client is disconnected 15 seconds after its last read, and the second client receives the ready banner. Fix: The one timeout this transport was missing: SERVE_IDLE_TIMEOUT = 15 s (same value as the other transports), set on each accepted socket via serve_socket_with_idle - a seam that also lets the regression test run with 300 ms. A timed-out read surfaces as an error from lines_capped, which the loop's existing let Ok(line) ... else break already turns into "drop this client and accept the next one"; a partially read line is discarded, which is correct because it was never dispatched. The timeout only runs while a read is pending, so long server-side operations do not spend the idle budget, and the document session survives the reconnect. Regression test: an idle client must see EOF and the following client must still get its banner. --- src/app/automation.rs | 88 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/src/app/automation.rs b/src/app/automation.rs index ca052aab5..2a1fcee33 100644 --- a/src/app/automation.rs +++ b/src/app/automation.rs @@ -125,11 +125,33 @@ fn serve_stdio(app: &mut OpenCADStudio) { } } +/// Idle timeout for one `--serve` socket client — the same 15 s every other +/// transport in the codebase uses (`rest.rs`, `http_bridge.rs`, +/// `control::transport`), and the only one this path was missing. +#[cfg(not(target_arch = "wasm32"))] +const SERVE_IDLE_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(15); + +/// Serve one socket client at a time on `127.0.0.1:port`. An idle client is +/// disconnected after [`SERVE_IDLE_TIMEOUT`] — the accept loop is +/// single-threaded, so a read that never completes would wedge the port for +/// every later client. The document session survives reconnects. #[cfg(not(target_arch = "wasm32"))] fn serve_socket( app: &mut OpenCADStudio, port: u16, bound_port: &std::sync::Arc, +) { + serve_socket_with_idle(app, port, bound_port, SERVE_IDLE_TIMEOUT) +} + +/// [`serve_socket`] with an explicit per-client idle timeout (a test seam; +/// production traffic uses [`SERVE_IDLE_TIMEOUT`]). +#[cfg(not(target_arch = "wasm32"))] +fn serve_socket_with_idle( + app: &mut OpenCADStudio, + port: u16, + bound_port: &std::sync::Arc, + idle: std::time::Duration, ) { let listener = match std::net::TcpListener::bind(("127.0.0.1", port)) { Ok(l) => l, @@ -142,6 +164,7 @@ fn serve_socket( bound_port.store(bound, std::sync::atomic::Ordering::SeqCst); eprintln!("OpenCADStudio --serve listening on 127.0.0.1:{bound}"); for stream in listener.incoming().flatten() { + let _ = stream.set_read_timeout(Some(idle)); let Ok(read_half) = stream.try_clone() else { continue; }; @@ -3359,4 +3382,69 @@ mod tests { ); payload.join().unwrap(); } + + /// A client that connects and says nothing must not hold the automation + /// port forever: `serve_socket` serves one client at a time, so a read + /// that never completes (no timeout was ever set on this path, unlike + /// `rest.rs`, `http_bridge.rs` and `control::transport`) wedges the + /// accept loop and every later client hangs waiting for the `ready` + /// banner. The server drops an idle connection at its timeout and goes + /// back to accepting. + #[cfg(not(target_arch = "wasm32"))] + #[test] + fn serve_socket_drops_an_idle_client_and_keeps_serving() { + use std::io::{BufRead, BufReader}; + use std::net::TcpStream; + use std::sync::atomic::{AtomicU16, Ordering}; + use std::sync::Arc; + use std::time::Duration; + + let bound = Arc::new(AtomicU16::new(0)); + let listen_bound = bound.clone(); + std::thread::spawn(move || { + let mut app = super::OpenCADStudio::new_for_test(); + super::serve_socket_with_idle( + &mut app, + 0, + &listen_bound, + Duration::from_millis(300), + ); + }); + let mut port = 0; + for _ in 0..50 { + port = bound.load(Ordering::SeqCst); + if port != 0 { + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + assert_ne!(port, 0, "listener never bound a port"); + + let first = TcpStream::connect(("127.0.0.1", port)).expect("connect first client"); + first + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("read timeout"); + let mut reader = BufReader::new(first.try_clone().expect("clone")); + let mut banner = String::new(); + reader.read_line(&mut banner).expect("ready banner"); + assert!(banner.contains("ready"), "{banner}"); + + // Say nothing: the server must drop the idle connection at its read + // timeout instead of holding the accept loop hostage. + let mut tail = String::new(); + let read = reader.read_line(&mut tail).unwrap_or_else(|error| { + panic!("idle connection must be closed by the server, blocked instead: {error}") + }); + assert_eq!(read, 0, "idle connection must see EOF, got {tail:?}"); + + // And the port must still serve the next client. + let second = TcpStream::connect(("127.0.0.1", port)).expect("connect second client"); + second + .set_read_timeout(Some(Duration::from_secs(5))) + .expect("read timeout"); + let mut reader2 = BufReader::new(second.try_clone().expect("clone")); + let mut banner2 = String::new(); + reader2.read_line(&mut banner2).expect("second ready banner"); + assert!(banner2.contains("ready"), "{banner2}"); + } }