Skip to content

Destroy Runner/MasterMinion resources in salt-run and fix MasterMinion.__del__ - #70177

Open
twangboy wants to merge 3 commits into
saltstack:3008.xfrom
twangboy:fix/70174/3008.x
Open

Destroy Runner/MasterMinion resources in salt-run and fix MasterMinion.__del__#70177
twangboy wants to merge 3 commits into
saltstack:3008.xfrom
twangboy:fix/70174/3008.x

Conversation

@twangboy

Copy link
Copy Markdown
Contributor

What does this PR do?

salt-run left its Runner un-destroy()'d, and RunnerClient / WheelClient never cleaned up the MasterMinion they lazily create via SyncClientMixin.mminion. Both were only reclaimed by __del__'s GC-time safety net, which now logs a loud "unclosed Runner"/ "unclosed MasterMinion" WARNING on every salt-run invocation instead of being silently filtered.

  • salt/cli/run.py: wrap SaltRun.run() in try / finally so runner.destroy() always runs, covering both the --doc early-exit and the normal path.
  • salt/runner.py, salt/wheel/__init__.py: destroy() now also tears down self._mminion.
  • salt/minion.py: MasterMinion.destroy() leaves returners/functions/utils as {}, not None, but __del__'s "already torn down" check tested for None specifically, so even a properly destroy()'d MasterMinion kept tripping the warning. Check falsiness instead.

What issues does this PR fix or reference?

Fixes #70174

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

…n.__del__

salt-run left its Runner un-destroy()'d, and RunnerClient/WheelClient
never cleaned up the MasterMinion they lazily create via
SyncClientMixin.mminion. Both were only reclaimed by __del__'s
GC-time safety net, which now logs a loud "unclosed Runner"/
"unclosed MasterMinion" WARNING on every salt-run invocation instead
of being silently filtered.

- salt/cli/run.py: wrap SaltRun.run() in try/finally so runner.destroy()
  always runs, covering both the --doc early-exit and the normal path.
- salt/runner.py, salt/wheel/__init__.py: destroy() now also tears down
  self._mminion.
- salt/minion.py: MasterMinion.destroy() leaves returners/functions/utils
  as {}, not None, but __del__'s "already torn down" check tested for
  None specifically, so even a properly destroy()'d MasterMinion kept
  tripping the warning. Check falsiness instead.

Fixes saltstack#70174
@twangboy twangboy added this to the Argon v3008.3 milestone Aug 28, 2026
@twangboy twangboy self-assigned this Aug 28, 2026
@twangboy
twangboy requested a review from a team as a code owner August 28, 2026 19:08
@twangboy twangboy added the test:full Run the full test suite label Aug 28, 2026
pylint's blacklisted-module check (saltpylint) flags direct use of
unittest.mock; Salt's own tests must import MagicMock from
tests.support.mock instead. Fixes the "Lint Salt's Test Suite" CI
failure on PR saltstack#70177.
Review follow-up for saltstack#70177/saltstack#70174: MasterMinion.destroy(),
RunnerClient.destroy(), and WheelClient.destroy() ran their
per-component teardown steps back-to-back with no exception
isolation. If any one component's .destroy() call raised, every
later step -- including resetting the remaining attributes to {}
and, in RunnerClient/WheelClient's case, tearing down the lazily
created MasterMinion (self._mminion), the whole point of this fix --
was silently skipped, and the exception propagated out of destroy()
itself.

That's now unconditionally called from salt/cli/run.py's
finally: runner.destroy(), so an exception there would replace
SaltRun.run()'s real result/control flow with an unrelated
destroy()-time traceback.

Wrap each component's teardown call in its own try/except (mirroring
the existing pattern in salt/utils/asynchronous.py's
SyncWrapper.destroy()), and make each attribute reset unconditional,
so one component failing can never block another's teardown, and
destroy() itself can never raise.
@twangboy

Copy link
Copy Markdown
Contributor Author

Addressed both considerations:

  1. Code duplication in SaltRun.run — checked the diff against 3008.x: this isn't duplication, it's a straight re-indent of the existing body one level deeper under the new try: ... finally: runner.destroy(). Every statement appears exactly once and the nested try/except SaltClientError block is preserved intact. No change needed.

  2. MasterMinion.destroy() attribute resets — this one turned up a real gap: destroy()'s per-component teardown calls (returners/functions/utils/states/rend/matchers/executors/proxy/serializers/opts["grains"]) ran back-to-back with no exception isolation, so if any one .destroy() call raised, every later step — including resetting functions/utils to {}, which __del__'s check inspects — was silently skipped, and the exception propagated out of destroy() itself. The same unguarded pattern existed in RunnerClient.destroy()/WheelClient.destroy(), which is sharper: if self.event.destroy() raised, self._mminion.destroy() — this PR's whole fix — would never run. Since destroy() is now called unconditionally from salt/cli/run.py's finally: block, that exception would've replaced SaltRun.run()'s real result with an unrelated traceback.

    Pushed a follow-up commit (6b264b8) that wraps each component's teardown in its own try/except (mirroring the existing pattern in salt/utils/asynchronous.py: SyncWrapper.destroy()) and makes every attribute reset unconditional, so one component failing can never block another's teardown or block destroy() from completing. Added regression tests in test_minion.py, test_runner.py, and wheel/test_init.py covering the "one step raises, the rest still runs" guarantee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant