diff --git a/Cargo.lock b/Cargo.lock index 1cbeeb1..86e266f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5387,6 +5387,7 @@ dependencies = [ "rusqlite", "scl-crypto", "serde_json", + "socket2", "thiserror 2.0.21", "tokio", "url", diff --git a/crates/forge/Cargo.toml b/crates/forge/Cargo.toml index f3fdfa8..de723d4 100644 --- a/crates/forge/Cargo.toml +++ b/crates/forge/Cargo.toml @@ -18,6 +18,7 @@ path = "src/main.rs" clap = { version = "4.6.1", features = ["derive"] } anyhow = "1.0.102" scl-crypto = { path = "../crypto" } +socket2 = "0.6.5" # The whole implementation is unix-only (src/lib.rs is #![cfg(unix)]); keeping # these out of other targets stops the Windows workspace check from compiling diff --git a/crates/forge/src/server.rs b/crates/forge/src/server.rs index 4752f6b..62a36bc 100644 --- a/crates/forge/src/server.rs +++ b/crates/forge/src/server.rs @@ -31,6 +31,10 @@ pub struct Limits { pub header_timeout: Duration, /// Time allowed to receive a request body once its headers arrived. pub body_timeout: Duration, + /// Idle time before TCP keepalive probes a connection's peer. A client that vanished without + /// a close the server received (a dead host, a dropped NAT entry, and on macOS loopback in + /// tests, #316) is then noticed by the probes instead of only at the transport idle timeout. + pub tcp_keepalive: Duration, } impl Default for Limits { @@ -40,6 +44,7 @@ impl Default for Limits { max_connections: 512, header_timeout: Duration::from_secs(10), body_timeout: Duration::from_secs(30), + tcp_keepalive: Duration::from_secs(15), } } } @@ -314,6 +319,11 @@ pub async fn serve( continue; }; let Ok(permit) = slots.clone().try_acquire_owned() else { continue }; + // Best effort: without probes a vanished peer still ends at the idle timeout. + let keepalive = socket2::TcpKeepalive::new().with_time(limits.tcp_keepalive); + if let Err(e) = socket2::SockRef::from(&stream).set_tcp_keepalive(&keepalive) { + crate::log::warning!("could not enable TCP keepalive: {e}"); + } let (app, limits) = (app.clone(), limits.clone()); let header_timeout = limits.header_timeout; let mut stop = stop_rx.clone(); diff --git a/crates/forge/tests/e2e_git.rs b/crates/forge/tests/e2e_git.rs index d56f0cb..0e801bb 100644 --- a/crates/forge/tests/e2e_git.rs +++ b/crates/forge/tests/e2e_git.rs @@ -569,6 +569,25 @@ fn wait_for(what: &str, mut f: impl FnMut() -> bool) { } } +#[test] +fn keepalive_probes_leave_a_paused_live_client_alone() { + let s = Srv::start_with(scl_forge::server::Limits { + tcp_keepalive: std::time::Duration::from_secs(1), + ..Default::default() + }); + let auth = token(&s.admin); + repo(&s, "smart"); + let held = stalled_push(&s, &auth, "smart"); + wait_for("the transport to register", || { + s.app.life.registry.live() == 1 + }); + // The client sends nothing for several probe periods; its TCP stack still answers them. + std::thread::sleep(std::time::Duration::from_secs(4)); + assert_eq!(s.app.life.registry.live(), 1, "a live client is kept"); + drop(held); + wait_for("the transport to end", || s.app.life.registry.live() == 0); +} + #[test] fn slots_are_bounded_then_released_when_the_client_disconnects() { let s = Srv::start_transport(scl_forge::cgi::Cfg { @@ -702,10 +721,11 @@ fn quarantined(repo_dir: &Path) -> bool { #[test] fn a_push_cancelled_mid_pack_moves_no_ref_and_its_quarantine_is_reclaimed() { use std::io::Write; - // The server does not always see a client that leaves mid-body (the body read can miss the - // EOF, #316); the idle timeout then ends the transport. A short one bounds the test either way. - let s = Srv::start_transport(scl_forge::cgi::Cfg { - idle: std::time::Duration::from_secs(5), + // The server does not always receive the close of a client that leaves mid-body (#316, seen + // on macOS loopback); TCP keepalive probes then find it gone, well before the 120 s idle + // timeout. A one-second probe delay keeps the test inside `wait_for`'s ten seconds. + let s = Srv::start_with(scl_forge::server::Limits { + tcp_keepalive: std::time::Duration::from_secs(1), ..Default::default() }); let c = Client::new("cancel-push"); @@ -750,8 +770,7 @@ fn a_push_cancelled_mid_pack_moves_no_ref_and_its_quarantine_is_reclaimed() { wait_for("the push to be receiving into its quarantine", || { s.app.life.registry.live() == 1 && quarantined(&repo_dir) }); - // The client goes away mid-pack: the server cancels the transport (on the broken body, or - // at the idle timeout). + // The client goes away mid-pack: the server cancels the transport. drop(sock); wait_for("the transport to unregister", || { s.app.life.registry.live() == 0 diff --git a/docs/THREAT-MODEL.md b/docs/THREAT-MODEL.md index e8d5448..20e3baa 100644 --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -927,8 +927,12 @@ unless said otherwise). 2.54); maintenance removes it once it is old enough to have no owner, as it does for a server killed outright (`a_push_cancelled_mid_pack_moves_no_ref_and_its_quarantine_is_reclaimed`). A push cancelled while it holds its ref lock (held in git's `reference-transaction prepared` hook) releases the lock and moves no ref - (`a_push_cancelled_while_holding_its_ref_lock_releases_it`). A client that leaves mid-body is sometimes - noticed only at `--git-idle-timeout` (the body read can miss the EOF, about 1 run in 8 locally, #316). + (`a_push_cancelled_while_holding_its_ref_lock_releases_it`). A client whose close the server never receives (seen on macOS loopback in about 1 run in 8) + is found by TCP keepalive, whose probes start after 15 s of silence (`Limits::tcp_keepalive`): a peer whose host + answers a probe with a reset ends at once, and a paused but live client answers them + (`keepalive_probes_leave_a_paused_live_client_alone`, #316). An unanswered probe follows the platform's retry + defaults (Linux's can exceed 120 s), so for a dead host or a dropped NAT entry `--git-idle-timeout` remains the + bound; behind a proxy the probes reach only the proxy. - **Process control.** One supervisor thread owns each child and its process group and signals the group while the leader is still an unreaped zombie (so a reused group id cannot be hit); the registry holds a cancel flag, never a group id. A client that hangs up ends the child (`a_large_push_and_clone_stream_and_a_vanished_client_ends_the_child`);