From 2a2b9739fc3cde58a54c57c9f992eb0b5f6440e1 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Mon, 6 Jul 2026 17:31:44 -0700 Subject: [PATCH] fix(api): bound all subprocess calls + run 2 workers to prevent API stall The management API wedged on whp01 2026-07-07: every panel call to the manager (config regenerate during a WHP site update, SSL, even /health) timed out at 30s while customer sites stayed up. Root cause: all four gunicorn gthread worker threads were permanently blocked in socket reads inside untimed subprocess.run() calls (certbot ACME / socat reloads). A stalled external command holds its worker thread forever; gunicorn --timeout can't rescue it (gthread only kills a worker whose main thread stops heart-beating, and ours kept polling). Stalled calls accumulated until the 4-thread pool was exhausted and the whole API went dark. - Wrap subprocess.run with a default timeout (HAPROXY_MGR_SUBPROCESS_TIMEOUT, 180s) so every external command is bounded and releases its thread on expiry via the existing per-endpoint try/except. Bounding by default covers all ~30 call sites and any future one. - certbot renew keeps an explicit 900s timeout (walks every lineage). - API_WORKERS default 1 -> 2: a single worker made a thread-pool wedge a total outage; a second worker keeps the API answering while one recycles. Co-Authored-By: Claude Opus 4.8 (1M context) --- VERSION | 2 +- haproxy_manager.py | 38 ++++++++++++++++++++++++++++++++++++-- scripts/start-up.sh | 8 +++++++- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/VERSION b/VERSION index 22c4432..0657c95 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2026.07.2 +2026.07.3 diff --git a/haproxy_manager.py b/haproxy_manager.py index ebe1166..a70f812 100644 --- a/haproxy_manager.py +++ b/haproxy_manager.py @@ -18,6 +18,38 @@ import time import re import fcntl +# --------------------------------------------------------------------------- +# Bounded subprocess execution (incident 2026-07-07) +# --------------------------------------------------------------------------- +# Every external command this manager runs — certbot ACME issuance/renewal, +# `socat` reloads over the haproxy admin socket, `haproxy -c` validation — is a +# potential hang. The management API runs under gunicorn gthread workers, and a +# subprocess.run() with NO timeout blocks its worker thread forever if the +# command stalls (e.g. an ACME/upstream that stops responding mid-read). +# gunicorn's --timeout does not rescue this: for gthread it only kills a worker +# whose *main* thread stops heart-beating, but the main thread keeps polling +# while pool threads are wedged. Enough stalled calls exhaust the 4-thread pool +# and the whole API stops responding — "healthy" health-check, every request +# 30s-timeouts — which is exactly what stalled WHP site updates on 2026-07-07. +# +# Fix: give EVERY subprocess.run() a default timeout unless the caller passes +# one explicitly. On expiry Python kills the child and raises +# subprocess.TimeoutExpired (a subclass of Exception); the existing per-endpoint +# try/except turns that into a clean error AND releases the worker thread. +# Bounding by default (instead of editing ~30 call sites) means no site can be +# missed and any future call is protected automatically. +DEFAULT_SUBPROCESS_TIMEOUT = int(os.environ.get('HAPROXY_MGR_SUBPROCESS_TIMEOUT', '180')) +_unbounded_subprocess_run = subprocess.run + + +def _bounded_subprocess_run(*args, **kwargs): + if kwargs.get('timeout') is None: + kwargs['timeout'] = DEFAULT_SUBPROCESS_TIMEOUT + return _unbounded_subprocess_run(*args, **kwargs) + + +subprocess.run = _bounded_subprocess_run + app = Flask(__name__) # Default page server (port 8080) — served to HAProxy clients whose request hit @@ -811,10 +843,12 @@ def renew_certificates(): # Defensive: clear any stale lock left by a SIGKILLed prior run. clear_stale_certbot_locks() - # Run certbot renew + # Run certbot renew. Explicit long timeout (overrides the module + # default): `renew` walks every lineage and can legitimately make many + # ACME round-trips when several certs are actually due. result = subprocess.run([ 'certbot', 'renew', '--quiet' - ], capture_output=True, text=True) + ], capture_output=True, text=True, timeout=900) if result.returncode == 0: # Check if any certificates were renewed diff --git a/scripts/start-up.sh b/scripts/start-up.sh index c022a4c..1e782e2 100755 --- a/scripts/start-up.sh +++ b/scripts/start-up.sh @@ -47,7 +47,13 @@ HAPROXY_SUPERVISOR_INTERVAL="${HAPROXY_SUPERVISOR_INTERVAL:-15}" # Tunable via env: HAPROXY_MGR_API_WORKERS (default 1), HAPROXY_MGR_API_TIMEOUT # (default 120 — API can do slow ACME calls), HAPROXY_MGR_MAX_REQUESTS (default # 1000 — worker recycle frequency). -API_WORKERS="${HAPROXY_MGR_API_WORKERS:-1}" +# +# API_WORKERS default is 2 (was 1). A single worker is a single point of +# failure: if its gthread pool ever wedges (see the 2026-07-07 subprocess-hang +# incident — now bounded by DEFAULT_SUBPROCESS_TIMEOUT in haproxy_manager.py), +# the entire management API goes dark. A second worker keeps the API answering +# (config regenerate, health, SSL) while the other recycles via --max-requests. +API_WORKERS="${HAPROXY_MGR_API_WORKERS:-2}" API_TIMEOUT="${HAPROXY_MGR_API_TIMEOUT:-120}" MAX_REQ="${HAPROXY_MGR_MAX_REQUESTS:-1000}" MAX_REQ_JITTER="${HAPROXY_MGR_MAX_REQUESTS_JITTER:-100}"