From 6d03784d4b15535666bd4afdc5bbde5af016e078 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 14 Aug 2026 15:10:30 +0200 Subject: [PATCH] fix: keep non traffic-serving processes out of coordinated restarts (#10694) * fix: key server_heartbeat row on hostname so restarts reuse one row Co-Authored-By: Claude Opus 5 (1M context) * chore: trim announce_server_started doc to the durable constraints Co-Authored-By: Claude Opus 5 (1M context) * fix: only traffic-serving processes take part in coordinated restarts Co-Authored-By: Claude Opus 5 (1M context) * chore: name every non traffic-serving mode in the restart-gate comments Co-Authored-By: Claude Opus 5 (1M context) * chore: narrow the restart-gate comments to claims that hold Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- backend/src/main.rs | 51 ++++++++++++++++++++++++++------- backend/windmill-api/src/lib.rs | 36 +++++++++++++++-------- 2 files changed, 65 insertions(+), 22 deletions(-) diff --git a/backend/src/main.rs b/backend/src/main.rs index b625f440f4..6383bfd78e 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -1691,7 +1691,8 @@ async fn process_notify_event( "restart_worker_group" => { if worker_mode && payload == *WORKER_GROUP { tracing::info!("Restart requested for worker group '{payload}'"); - spawn_graceful_killpill(tx, db, 30, "worker group restart requested").await; + spawn_graceful_killpill(tx, db, 30, "worker group restart requested", server_mode) + .await; } } "notify_webhook_change" => { @@ -1997,8 +1998,14 @@ async fn process_notify_event( reload_otel_tracing_proxy_setting(conn).await; if worker_mode { tracing::info!("OTEL tracing proxy setting changed, restarting worker"); - spawn_graceful_killpill(tx, db, 30, "OTEL tracing proxy setting change") - .await; + spawn_graceful_killpill( + tx, + db, + 30, + "OTEL tracing proxy setting change", + server_mode, + ) + .await; } } REQUIRE_PREEXISTING_USER_FOR_OAUTH_SETTING => { @@ -2009,12 +2016,20 @@ async fn process_notify_event( } EXPOSE_METRICS_SETTING => { tracing::info!("Metrics setting changed, restarting"); - spawn_graceful_killpill(tx, db, 30, "metrics setting change").await; + spawn_graceful_killpill(tx, db, 30, "metrics setting change", server_mode) + .await; } EMAIL_DOMAIN_SETTING => { tracing::info!("Email domain setting changed"); if server_mode { - spawn_graceful_killpill(tx, db, 30, "email domain setting change").await; + spawn_graceful_killpill( + tx, + db, + 30, + "email domain setting change", + server_mode, + ) + .await; } } EXPOSE_DEBUG_METRICS_SETTING => { @@ -2050,19 +2065,26 @@ async fn process_notify_event( } OTEL_SETTING => { tracing::info!("OTEL setting changed, restarting"); - spawn_graceful_killpill(tx, db, 30, "OTEL setting change").await; + spawn_graceful_killpill(tx, db, 30, "OTEL setting change", server_mode).await; } REQUEST_SIZE_LIMIT_SETTING => { if server_mode { tracing::info!("Request limit size change detected, killing server expecting to be restarted"); - spawn_graceful_killpill(tx, db, 30, "request size limit change").await; + spawn_graceful_killpill( + tx, + db, + 30, + "request size limit change", + server_mode, + ) + .await; } } SAML_METADATA_SETTING => { tracing::info!( "SAML metadata change detected, killing server expecting to be restarted" ); - spawn_graceful_killpill(tx, db, 30, "SAML metadata change").await; + spawn_graceful_killpill(tx, db, 30, "SAML metadata change", server_mode).await; } HUB_BASE_URL_SETTING => { if let Err(e) = reload_hub_base_url_setting(conn, server_mode).await { @@ -2279,16 +2301,24 @@ pub async fn run_workers( /// then the sleep+kill is spawned in the background so the notification handler is not blocked. /// /// Falls back to drain-only delay if DB coordination fails. +/// +/// Only `server_mode` processes coordinate, on the strength of the worker case: a worker +/// group restarting costs queue latency rather than lost work, `v2_job_queue` being durable. +/// Were workers to take part, one could claim the `is_first` slot and leave every server +/// holding its shutdown open for a peer that serves no API traffic. async fn spawn_graceful_killpill( tx: &KillpillSender, db: &Pool, safety_margin_secs: u64, context: &str, + server_mode: bool, ) { // Minimum delay before any restart to let in-flight requests drain const DRAIN_DELAY_SECS: u64 = 3; - let (delay, is_first) = + let (delay, is_first) = if !server_mode { + (DRAIN_DELAY_SECS, true) + } else { match coordinate_restart_delay(db, safety_margin_secs, DRAIN_DELAY_SECS).await { Ok(r) => r, Err(e) => { @@ -2298,7 +2328,8 @@ async fn spawn_graceful_killpill( ); (DRAIN_DELAY_SECS, true) } - }; + } + }; tracing::info!( "Scheduling {context} graceful shutdown in {delay}s (first_to_restart={is_first})" diff --git a/backend/windmill-api/src/lib.rs b/backend/windmill-api/src/lib.rs index e67420c53c..ce709d3121 100644 --- a/backend/windmill-api/src/lib.rs +++ b/backend/windmill-api/src/lib.rs @@ -1166,8 +1166,10 @@ pub async fn run_server( } // Announce this server is ready so coordinated restarts can detect a healthy peer. - if let Err(e) = announce_server_started(&db).await { - tracing::warn!("Failed to announce server started: {e:#}"); + if server_mode { + if let Err(e) = announce_server_started(&db).await { + tracing::warn!("Failed to announce server started: {e:#}"); + } } let server = server.with_graceful_shutdown(async move { @@ -1314,25 +1316,35 @@ pub async fn wait_for_db_migrations( const SERVER_HEARTBEAT_TASK: &str = "server_heartbeat"; -/// Write a server-started heartbeat to `background_task_state` so that -/// other instances waiting to restart can detect this server is healthy. +/// Write a server-started heartbeat to `background_task_state` so that other +/// traffic-serving instances waiting to restart can detect this one is healthy. +/// +/// Only `server_mode` processes announce, since only they are peers worth waiting for: +/// `spawn_graceful_killpill` holds a shutdown open to keep the API answered, and a worker, +/// indexer or MCP process coming up is no evidence that it is. +/// +/// The row is keyed per host and `owner` per process, and both halves carry weight. +/// `INSTANCE_NAME` is random per start, so a row keyed on it never conflicts and +/// accumulates one row per start; `owner` is what tells a peer's start from its own +/// when processes share a host. async fn announce_server_started(db: &DB) -> anyhow::Result<()> { - use windmill_common::INSTANCE_NAME; + use windmill_common::{utils::HOSTNAME, INSTANCE_NAME}; let instance = INSTANCE_NAME.as_str(); + let host = HOSTNAME.as_str(); sqlx::query( "INSERT INTO background_task_state (name, value, running, owner, started_at, updated_at) VALUES ($1, '\"started\"'::jsonb, true, $2, NOW(), NOW()) ON CONFLICT (name) - DO UPDATE SET updated_at = NOW(), running = true, owner = $2", + DO UPDATE SET started_at = NOW(), updated_at = NOW(), running = true, owner = $2", ) - .bind(format!("{SERVER_HEARTBEAT_TASK}:{instance}")) + .bind(format!("{SERVER_HEARTBEAT_TASK}:{host}")) .bind(instance) .execute(db) .await?; - tracing::info!("Announced server started for instance {instance}"); + tracing::info!("Announced server started for instance {instance} on host {host}"); Ok(()) } @@ -1362,10 +1374,10 @@ pub async fn check_any_server_started(db: &DB, not_before: chrono::DateTime not_before` (the moment a restart was initiated),