diff --git a/asset/config/config.toml.tmpl b/asset/config/config.toml.tmpl index 90baaaf..f67ab90 100644 --- a/asset/config/config.toml.tmpl +++ b/asset/config/config.toml.tmpl @@ -17,6 +17,11 @@ external_url = "https://rf.internal" # timeout here is an idle timeout instead. This one bounds how long a client may # stop reading before the tee detaches it and finishes the transfer anyway. client_stall_timeout = "60s" +# How long in-flight responses may drain on SIGTERM before the service stops +# anyway. A blob response lasts size/bandwidth, so an unbounded drain never +# converges: systemd's TimeoutStopSec expires and escalates to SIGABRT. Cutting +# a download deliberately is the better failure -- the client retries. +shutdown_grace = "15s" [storage] endpoint = "{{S3_ENDPOINT}}" diff --git a/asset/systemd/rustingface.service b/asset/systemd/rustingface.service index ce74c94..cf9c663 100644 --- a/asset/systemd/rustingface.service +++ b/asset/systemd/rustingface.service @@ -21,9 +21,12 @@ LoadCredential=s3-access-key:/etc/rustingface/s3-access-key LoadCredential=s3-secret-key:/etc/rustingface/s3-secret-key LoadCredential=hf-token:/etc/rustingface/hf-token -# Long transfers must not be killed mid-flight on stop; a detached upload that -# is cut short leaves an orphan blob, which is safe but wasteful. -TimeoutStopSec=120 +# A backstop, not the mechanism. The service caps its own drain at +# server.shutdown_grace and then exits cleanly, because a blob response lasts +# size/bandwidth and waiting for one to finish does not converge -- letting this +# expire instead means SIGABRT and a core dump. Keep it comfortably above the +# configured grace so systemd never escalates first. +TimeoutStopSec=60 # Hardening. The process holds no durable state — the bucket is the whole of it # — so there is no StateDirectory to protect and ProtectSystem can stay strict diff --git a/crates/rustingface-entities/src/config.rs b/crates/rustingface-entities/src/config.rs index 487be02..9b29fe6 100644 --- a/crates/rustingface-entities/src/config.rs +++ b/crates/rustingface-entities/src/config.rs @@ -51,6 +51,16 @@ pub struct Server { /// detached and finished without it. #[serde(with = "humantime_serde")] pub client_stall_timeout: Duration, + /// How long to let in-flight requests drain on SIGTERM before stopping + /// anyway. + /// + /// A blob response is an in-flight request that lasts `size / bandwidth`, + /// so an unbounded drain does not converge on a restart: systemd's + /// `TimeoutStopSec` expires and escalates to SIGABRT, which is a core dump + /// rather than a shutdown. Cutting a download deliberately is the better + /// failure -- clients retry, and nothing in the bucket depends on it. + #[serde(with = "humantime_serde")] + pub shutdown_grace: Duration, } impl Default for Server { @@ -59,6 +69,7 @@ impl Default for Server { listen: "127.0.0.1:20482".into(), external_url: None, client_stall_timeout: Duration::from_secs(60), + shutdown_grace: Duration::from_secs(15), } } } diff --git a/crates/rustingface/src/cmd/serve.rs b/crates/rustingface/src/cmd/serve.rs index df509c8..1b2749d 100644 --- a/crates/rustingface/src/cmd/serve.rs +++ b/crates/rustingface/src/cmd/serve.rs @@ -70,7 +70,7 @@ pub async fn run(config: Config) -> Result<()> { let app = router(AppState::new(registry, tokens.into(), metrics)); axum::serve(listener, app) - .with_graceful_shutdown(shutdown()) + .with_graceful_shutdown(shutdown(config.server.shutdown_grace)) .await .context("serving") } @@ -87,12 +87,22 @@ fn token_file_has_content(config: &Config) -> bool { }) } -/// Stop accepting on `SIGTERM`/`SIGINT` and let in-flight requests drain. +/// Stop accepting on `SIGTERM`/`SIGINT` and let in-flight requests drain -- +/// for a bounded time. /// -/// Detached transfers are not requests and are not waited on here: the process -/// exiting mid-upload leaves an orphan blob, which is safe and reclaimable, -/// while a manifest is only ever written after an upload completes. -async fn shutdown() { +/// The bound is the point. A blob response *is* an in-flight request and it +/// lasts `size / bandwidth`, so waiting for one to finish does not converge: +/// a 3GB shard at 8MB/s holds the drain open for seven minutes, systemd's +/// `TimeoutStopSec` expires long before that, and the service is killed with +/// SIGABRT and a core dump. Cutting the download deliberately after +/// `server.shutdown_grace` is strictly better -- the client retries, which is +/// a normal event, and the restart is prompt. +/// +/// Detached transfers are not requests and are not waited on here either: the +/// process exiting mid-upload leaves an orphan blob, which is safe and +/// reclaimable, while a manifest is only ever written after an upload +/// completes. +async fn shutdown(grace: std::time::Duration) { let mut term = match signal(SignalKind::terminate()) { Ok(signal) => signal, Err(err) => { @@ -101,11 +111,26 @@ async fn shutdown() { } }; tokio::select! { - _ = term.recv() => info!("SIGTERM received; draining"), + _ = term.recv() => info!(?grace, "SIGTERM received; draining"), result = tokio::signal::ctrl_c() => { if result.is_ok() { - info!("interrupt received; draining"); + info!(?grace, "interrupt received; draining"); } } } + + // Returning here only stops the listener accepting; axum then waits on + // every in-flight response, blob streams included. This watchdog is what + // makes that wait bounded. + tokio::spawn(async move { + tokio::time::sleep(grace).await; + tracing::warn!( + ?grace, + "in-flight responses did not finish within server.shutdown_grace; \ + stopping anyway and cutting them. Clients retry; stored blobs are \ + unaffected, because a manifest entry is only written once its blob \ + is durable." + ); + std::process::exit(0); + }); } diff --git a/doc/spec.md b/doc/spec.md index dd6b77f..225c89d 100644 --- a/doc/spec.md +++ b/doc/spec.md @@ -226,6 +226,7 @@ config. listen = "127.0.0.1:8080" external_url = "https://models.internal.example" # used to build redirect URLs client_stall_timeout = "60s" # idle guard on the tee; see below +shutdown_grace = "15s" # bounded drain on SIGTERM; see below [storage] endpoint = "https://s3.internal.example:9000" @@ -282,6 +283,15 @@ the next attempt is any faster. hung backend still fails a small metadata request promptly. - `upstream.read_timeout` — the same guard on the Hub side. +`server.shutdown_grace` is the one bound that is not idle-based, and +deliberately so. It caps how long in-flight responses may drain on SIGTERM +before the process stops regardless. A blob response is an in-flight request +lasting `size / bandwidth`, so an unbounded drain does not converge: systemd's +`TimeoutStopSec` expires first and escalates to `SIGABRT`. Cutting a download +deliberately is the better failure — the client retries, and nothing in the +bucket depends on the response, because a manifest entry is only ever written +once its blob is durable. + This applies to the object-storage client in particular. `object_store` defaults to a 30-second cap on the whole request, body included, and then hides its own consequences: it catches the resulting body errors and silently retries