Repository navigation
fix: Keep the metrics server running when a Gunicorn worker exits - #7012
Merged
ntkathole merged 4 commits intoOct 18, 2026
Merged
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7012 +/- ##
=======================================
Coverage 50.99% 50.99%
=======================================
Files 455 455
Lines 56970 56979 +9
Branches 8313 8315 +2
=======================================
+ Hits 29051 29059 +8
Misses 25935 25935
- Partials 1984 1985 +1
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
| pid = os.fork() | ||
| if pid == 0: | ||
| # What a worker runs at interpreter exit | ||
| multiprocessing.util._exit_function() |
Contributor
There was a problem hiding this comment.
LGTM. I reproduced the worker exit killing the metrics process and confirmed this fix keeps it alive. One small thing: the test runs the full _exit_function() inside pytest, so checking proc not in active_children() in the forked child would be safer.
Contributor
Author
There was a problem hiding this comment.
Thanks for reviewing! Just applied this fix, could you please check
patelchaitany
approved these changes
Oct 16, 2026
patelchaitany
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the update! LGTM.
patelchaitany
approved these changes
Oct 16, 2026
ntkathole
approved these changes
Oct 18, 2026
Since the metrics server runs in a multiprocessing.Process started in the Gunicorn master, every forked worker inherits multiprocessing's registry of child processes. At worker exit, multiprocessing.util._exit_function terminates every daemon process in that registry, so the first worker restart (--max-requests, timeout, reload) sends SIGTERM to the master's metrics server and /metrics stops responding. It also logs "AssertionError: can only join a child process" on every worker exit. Register an at-fork hook when the metrics process is started that removes it from the registry in every forked child. Workers no longer terminate it; the master still does on its own exit. Fixes feast-dev#7011 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandr Borgatin <a.borgatin@yandex.ru>
On macOS CI the spawned metrics process can take longer than the 5 s readiness wait to start listening, so the first /metrics request was refused. Poll /metrics (up to 30 s) before forking the child. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandr Borgatin <a.borgatin@yandex.ru>
The spawned metrics server did not come up within 30 s inside the full unit suite on macOS CI. The test is about the process surviving a forked child's exit, not about serving HTTP, so register a fork-context daemon process via start_metrics_server instead and assert it is still alive after the child runs multiprocessing.util._exit_function(). Without the fix it is terminated (exitcode -SIGTERM). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandr Borgatin <a.borgatin@yandex.ru>
…_exit_function Running multiprocessing.util._exit_function() in a fork of the pytest process runs every registered finalizer. Check what a worker's exit would act on instead: the metrics process must not be in the child's active_children(). Also cover _forget_metrics_process_in_child directly, and kill the stand-in process in cleanup (terminate() can wait out the join timeout under xdist). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandr Borgatin <a.borgatin@yandex.ru>
ntkathole
force-pushed
the
fix/metrics-process-worker-exit
branch
from
October 18, 2026 13:27
80dc00f to
125f4e8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Since #6855 the metrics server runs in a
multiprocessing.Process(daemon=True)started in the Gunicorn master. Every forked worker inheritsmultiprocessing's registry of child processes, and at worker exitmultiprocessing.util._exit_functionterminates every daemon process in it. So the first worker restart (--max-requests, worker timeout, reload) sends SIGTERM to the master's metrics server, and/metricsstops responding untilfeast serveis restarted. Every worker exit also logsAssertionError: can only join a child process. Details and steps to reproduce in #7011.When the metrics process is started, this registers an
os.register_at_fork(after_in_child=...)hook that removes it from the registry in every child forked from that process. An at-fork hook rather than a Gunicorn hook, so it also covers workers that exit beforepost_worker_init(e.g. a worker that fails to boot). Workers no longer terminate the metrics server; the master still terminates it on its own exit.Tested with the steps from #7011 (
--max-requests 3):/metricsreturns 200 after the worker restarts (000 before the change), noAssertionError.python:3.12-slim, 0.67.0 with thismetrics.py): 0.67.0 as released:/metrics000 after the first worker exit. With the change, 2 workers: 19 worker exits,/metricsanswered all 300 checks, noAssertionError; after stoppingfeast serveno metrics process is left.Which issue(s) this PR fixes:
Fixes #7011
Checks
pytest sdk/python/tests/unit/test_metrics.py: 127 passed;ruff checkandruff format --checkpass;mypy feastreports no errors inmetrics.py)git commit -s)Testing Strategy
TestMetricsProcessSurvivesForkedChildExit:start_metrics_serverregisters a daemon stand-in process (fork context, so it starts instantly on every platform), a forked child runsmultiprocessing.util._exit_function()as a worker does at exit, then the test checks the process is still alive (before the change it is terminated with-SIGTERM). The/metricsHTTP checks are the end-to-end runs above.🤖 Generated with Claude Code