From 642387dd0e302470a89849987f39e0878f922a9f Mon Sep 17 00:00:00 2001 From: Tanmoy Sarkar Date: Fri, 12 Jun 2026 22:51:19 +0530 Subject: [PATCH] fix(companion): Apply dead config settings and validate specs Three companion settings were documented and configurable but never had any effect. companion_restart_delay was ignored because CompanionProcess hardcoded a 5s delay; it is now read from config and kept out of config_hash, since it does not affect the spawned process and so must not trigger a restart on reread. companion_config_file was never read; the manager now loads its companion settings from that dedicated file when set, instead of always reading the main gunicorn config. companion_manager_stop_timeout was unused, so shutdown waited only graceful_timeout before SIGKILLing the manager and cut short long-draining companions; stop now waits the larger of graceful_timeout and the manager stop timeout, derived from the slowest companion stop_timeout plus the buffer when not set explicitly. Worker specs now reject unknown keys so a typo fails loudly instead of silently falling back to a default. Also correct the spawn_companion_manager docstring, drop its unused return value, and fix the README config-file description. Co-Authored-By: Claude Opus 4.8 (1M context) --- gunicorn/arbiter.py | 26 ++++++++++++++---- gunicorn/companion/README.md | 4 +-- gunicorn/companion/config.py | 50 +++++++++++++++++++++++++++------- gunicorn/companion/process.py | 2 +- tests/test_arbiter.py | 21 ++++++++++++++ tests/test_companion_config.py | 33 ++++++++++++++++++++++ 6 files changed, 117 insertions(+), 19 deletions(-) diff --git a/gunicorn/arbiter.py b/gunicorn/arbiter.py index 05fb3983..d9674b13 100644 --- a/gunicorn/arbiter.py +++ b/gunicorn/arbiter.py @@ -395,7 +395,9 @@ class Arbiter: sig = signal.SIGTERM if not graceful: sig = signal.SIGQUIT - limit = time.time() + self.cfg.graceful_timeout + # The manager may need longer than a worker to drain its companions. + limit = time.time() + max( + self.cfg.graceful_timeout, self.companion_manager_stop_timeout()) # instruct the workers and the companion manager to exit self.kill_workers(sig) self.stop_companion_manager(sig) @@ -675,9 +677,10 @@ class Arbiter: def spawn_companion_manager(self): """Fork the companion manager process. - The parent records the manager pid and returns. The child builds the - configs, runs the manager's supervision loop, and exits when the loop - returns. The manager forks the individual companions itself. + The configs are built in the parent before the fork; the parent then + records the manager pid and returns, while the child runs the manager's + supervision loop and exits when the loop returns. The manager forks the + individual companions itself. """ configs = build_companion_configs(self.cfg) if not configs: @@ -695,7 +698,7 @@ class Arbiter: if pid != 0: self.companion_manager_pid = pid self.log.info("Companion manager started (pid:%s)", pid) - return pid + return # Process Child try: @@ -759,6 +762,19 @@ class Arbiter: return raise + def companion_manager_stop_timeout(self): + """Seconds to wait for the companion manager during shutdown: the + explicit setting, else the slowest companion stop_timeout plus the + shutdown buffer, else 0 when no companions are configured. + """ + if self.cfg.companion_manager_stop_timeout is not None: + return self.cfg.companion_manager_stop_timeout + configs = build_companion_configs(self.cfg) + if not configs: + return 0 + slowest = max(config.stop_timeout for config in configs) + return slowest + self.cfg.companion_manager_shutdown_buffer + def kill_workers(self, sig): """\ Kill all workers with the signal `sig` diff --git a/gunicorn/companion/README.md b/gunicorn/companion/README.md index f49d43f5..84e28f1b 100644 --- a/gunicorn/companion/README.md +++ b/gunicorn/companion/README.md @@ -53,9 +53,7 @@ STOPPED ──start──▶ STARTING ──(survives startsecs)──▶ RUNNIN ## Configuration -Companions live in the normal Gunicorn config — a Python file you pass with -`-c`. There is no separate companion config file or CLI flag; if you already run -Gunicorn with a config, add the companion settings to it. +Companions live in the normal Gunicorn config — a Python file you pass with `-c`. To reload companions independently of the web config, put them in a dedicated file and point `companion_config_file` (or `--companion-config`) at it; the manager then reads its companion settings from there instead. Save a `gunicorn.conf.py`: diff --git a/gunicorn/companion/config.py b/gunicorn/companion/config.py index 12cb22c2..70b30dda 100644 --- a/gunicorn/companion/config.py +++ b/gunicorn/companion/config.py @@ -8,7 +8,7 @@ import json # Maps each optional companion field to the global setting build_companion_configs # reads when a spec omits it. ``name`` and ``target`` are required per spec and -# have no global default, so they are filled directly instead of through here. +# have no global default. ``restart_delay`` is global-only, so it is absent here. FIELD_DEFAULTS = { "cwd": "companion_cwd", "env": "companion_env", @@ -41,6 +41,7 @@ class CompanionConfig: stdout=None, stderr=None, startsecs=1, + restart_delay=5, ): self.name = name self.target = target @@ -52,6 +53,7 @@ class CompanionConfig: self.stdout = stdout self.stderr = stderr self.startsecs = startsecs + self.restart_delay = restart_delay def to_dict(self): return { @@ -88,19 +90,47 @@ class CompanionConfig: return "" % self.name -def build_companion_configs(cfg): - """Build a CompanionConfig list from ``cfg.companion_workers``. +ALLOWED_SPEC_KEYS = {"name", "target"} | set(FIELD_DEFAULTS) - A spec missing ``name`` or ``target`` is rejected, since the manager has - nothing to supervise without both. + +def _load_companion_settings(cfg): + """Return the ``companion_*`` settings from ``companion_config_file``, or + ``{}`` when no dedicated file is configured.""" + path = getattr(cfg, "companion_config_file", None) + if not path: + return {} + namespace = {} + with open(path) as config_file: + exec(compile(config_file.read(), path, "exec"), namespace) + return {name: value for name, value in namespace.items() + if name.startswith("companion_")} + + +def build_companion_configs(cfg): + """Build a CompanionConfig list from the companion settings. + + Settings come from ``companion_config_file`` when set, otherwise ``cfg``. A + spec is rejected if it is missing ``name``/``target`` or carries an unknown + key. """ + overrides = _load_companion_settings(cfg) + + def setting(name): + return overrides.get(name, getattr(cfg, name)) + configs = [] - for spec in cfg.companion_workers: + for spec in setting("companion_workers"): if "name" not in spec or "target" not in spec: raise ValueError( "each companion worker needs 'name' and 'target': %s" % spec) - fields = {field: spec.get(field, getattr(cfg, setting)) - for field, setting in FIELD_DEFAULTS.items()} - configs.append( - CompanionConfig(name=spec["name"], target=spec["target"], **fields)) + unknown = set(spec) - ALLOWED_SPEC_KEYS + if unknown: + raise ValueError( + "unknown companion worker key(s) %s in %s" + % (sorted(unknown), spec)) + fields = {field: spec.get(field, setting(global_setting)) + for field, global_setting in FIELD_DEFAULTS.items()} + configs.append(CompanionConfig( + name=spec["name"], target=spec["target"], + restart_delay=setting("companion_restart_delay"), **fields)) return configs diff --git a/gunicorn/companion/process.py b/gunicorn/companion/process.py index b7dc2104..bbc28ae6 100644 --- a/gunicorn/companion/process.py +++ b/gunicorn/companion/process.py @@ -36,7 +36,7 @@ class CompanionProcess: self.config = config self.state = State.STOPPED self.pid = None - self.restart_delay = 5 + self.restart_delay = config.restart_delay self.started_at = None self.exited_at = None diff --git a/tests/test_arbiter.py b/tests/test_arbiter.py index 9f50018c..7f9ca1d8 100644 --- a/tests/test_arbiter.py +++ b/tests/test_arbiter.py @@ -282,6 +282,27 @@ def test_arbiter_stop_signals_companion_manager(close_sockets): assert signal.SIGKILL in signals +def test_companion_manager_stop_timeout_uses_explicit(): + arbiter = gunicorn.arbiter.Arbiter(DummyApplication()) + arbiter.cfg.set("companion_manager_stop_timeout", 120) + assert arbiter.companion_manager_stop_timeout() == 120 + + +def test_companion_manager_stop_timeout_derives_from_slowest(): + arbiter = gunicorn.arbiter.Arbiter(DummyApplication()) + arbiter.cfg.set("companion_workers", [ + {"name": "rq", "target": "pkg:run", "stop_timeout": 300}, + {"name": "scheduler", "target": "pkg:sched", "stop_timeout": 30}, + ]) + arbiter.cfg.set("companion_manager_shutdown_buffer", 10) + assert arbiter.companion_manager_stop_timeout() == 310 + + +def test_companion_manager_stop_timeout_zero_without_companions(): + arbiter = gunicorn.arbiter.Arbiter(DummyApplication()) + assert arbiter.companion_manager_stop_timeout() == 0 + + class PreloadedAppWithEnvSettings(DummyApplication): """ Simple application that makes use of the 'preload' feature to diff --git a/tests/test_companion_config.py b/tests/test_companion_config.py index 97391983..e4178daf 100644 --- a/tests/test_companion_config.py +++ b/tests/test_companion_config.py @@ -36,6 +36,31 @@ def test_build_per_spec_overrides_global(): assert config.stop_signal == "SIGTERM" +def test_build_applies_global_restart_delay(): + cfg = make_config( + [{"name": "rq", "target": "pkg:run"}], + companion_restart_delay=42) + config, = build_companion_configs(cfg) + assert config.restart_delay == 42 + + +def test_build_rejects_unknown_spec_key(): + with pytest.raises(ValueError): + build_companion_configs( + make_config([{"name": "rq", "target": "pkg:run", "stop_sigal": "X"}])) + + +def test_build_reads_companion_config_file(tmp_path): + config_file = tmp_path / "companion.conf.py" + config_file.write_text( + 'companion_workers = [{"name": "scheduler", "target": "app:run"}]\n' + 'companion_stop_signal = "SIGINT"\n') + cfg = make_config([], companion_config_file=str(config_file)) + config, = build_companion_configs(cfg) + assert config.name == "scheduler" + assert config.stop_signal == "SIGINT" + + def test_build_empty_when_none_configured(): assert build_companion_configs(make_config([])) == [] @@ -71,6 +96,14 @@ def test_config_hash_stable_and_field_sensitive(): assert base.config_hash != changed.config_hash +def test_config_hash_ignores_restart_delay(): + # restart_delay does not affect the spawned process, so changing it must not + # change the hash (and so must not trigger a restart on reread). + base = CompanionConfig(name="rq", target="pkg:run", restart_delay=5) + changed = CompanionConfig(name="rq", target="pkg:run", restart_delay=30) + assert base.config_hash == changed.config_hash + + def test_config_hash_keys_callable_target_by_qualified_name(): def run(): pass