Fix file-descriptor leak in ProcessManager.restart_process - #70185
Conversation
480ade0 to
093222f
Compare
|
Confirming this fix covers the Independently reproduced the same drift on a stock master container: Prometheus Root cause matches yours exactly:
No separate PR needed — closing my local branch as duplicate. Your test file ( |
Every time a salt-master supervised subprocess exits and is restarted
(FileserverUpdate on the fileserver_interval cycle, Maintenance on the
maintenance_interval cycle, etc.), ProcessManager.restart_process was
dropping the dead child's Process reference without calling
Process.close(). The dead multiprocessing.popen_fork.Popen object owns
the two parent-side pipe fds opened in Popen._launch (parent_r,
parent_w), and those fds are only released when the Popen is
finalized. Because SignalHandlingProcess.__new__ registers
_setup_signals via register_after_fork_method(instance), the resulting
_after_fork_methods list holds a strong reference back to the instance
-- a reference cycle that defeats deterministic finalization of the
Popen when the last ProcessManager reference is dropped.
Net effect on a stock master with fileserver_interval=3600: the parent
leaks ~+2 pipe fds per subprocess restart per cycle, and every new
forked child inherits the accumulated set. Measured on a running
local salt-master container over two independent multi-hour windows in
Prometheus (salt_master_process_fds{process="FileserverUpdate"}): 92
-> 132 in 12.75h and 52 -> 76 in 10h, i.e. +4 fds per hour aligned
with the FileserverUpdate restart cycle. Same pattern on the
master-main process series (95 -> 131 across the same window),
confirming the leak is in the supervising parent, not the child.
Fix: call Process.close() on the dead child in restart_process()
after starting its replacement but before removing the entry from
_process_map, releasing the Popen sentinel pipe.
Regression test drives restart_process() directly against a
QuickSignalProc SignalHandlingProcess and asserts the parent's fd
count stays flat across 20 restarts. Without this fix the test fails
with delta=40 fds leaked; with it, delta=0.
Refs: salt/utils/process.py:563 (restart_process),
salt/master.py:692 (FileserverUpdate).
2 participants
Motivation
Prometheus scraping of the local salt-master container showed steady growth in
salt_master_process_fds{process="FileserverUpdate"}over two independentmulti-hour windows:
The same +4 FD-per-hour step pattern appeared on the parent-process series
salt_master_process_fds{process="master-main"}(95 -> 131 across the samewindow) confirming the leak lives in the supervising parent, not in
FileserverUpdateitself. The step cadence matchedfileserver_interval=3600sexactly -- one step per subprocess-restartcycle.
Root cause
salt.utils.process.ProcessManager.restart_process(salt/utils/process.py:563)drops the dead child's
Processreference without callingProcess.close(). The deadmultiprocessing.popen_fork.Popenobject ownsthe two parent-side pipe fds
Popen._launchallocates viaos.pipe()(
parent_randparent_w), and those fds are only released when thePopenis finalized.SignalHandlingProcess.__new__registers_setup_signalsviaregister_after_fork_method(instance), which appends(_setup_signals, (instance,), {})toself._after_fork_methods. That listholds a strong reference back to the instance -- a reference cycle that
defeats deterministic finalization when the last
ProcessManagerreferenceis dropped. Result: every subprocess restart leaks 2 pipe fds in the parent,
and every subsequent forked child inherits the accumulated set.
Fix
Call
Process.close()on the dead child inrestart_process()afterstarting its replacement, before removing the entry from
_process_map.This releases the
Popensentinel pipe deterministically regardless of thereference cycle. Guarded with a defensive
try/except (ValueError, AttributeError)per theProcess.close()contract.The same leak affected every long-lived master subprocess the
ProcessManager restarts (
Maintenance,EventReturn,MWorker, ...); thefix is in the shared supervisor code path, so all of them benefit.
Test
tests/pytests/functional/utils/test_process_restart_fd_leak.py::test_restart_process_does_not_leak_pipe_fdsDrives
ProcessManager.restart_process()directly against a short-livedSignalHandlingProcessand asserts the parent's/proc/<pid>/fdcountstays flat across 20 restarts.
AssertionError: ProcessManager.restart_process leaked 40 fds across 20 restarts (baseline=26, after=66)-- exactly+2 * iterations.PASSED(delta = 0).Testing done
pytest tests/pytests/functional/utils/test_process_restart_fd_leak.py-- fails on baseline, passes with fixpytest tests/pytests/functional/utils/test_process.py-- all 7 pass (including the pre-existingtest_subprocess_list_fdsandtest_process_manager_60749)pytest tests/unit/utils/test_process.py-- all 21 pass (6 skipped)pytest tests/pytests/unit/test_master.py-- all 43 pass (25 skipped)pre-commit run --files salt/utils/process.py tests/pytests/functional/utils/test_process_restart_fd_leak.py changelog/*-- clean