Skip to content

resource_warnings: route unclosed-resource warnings through Salt logger - #70100

Merged
dwoz merged 2 commits into
saltstack:3008.xfrom
dwoz:dwoz/fix/log-unclosed-resource-warnings-3008x
Aug 25, 2026
Merged

resource_warnings: route unclosed-resource warnings through Salt logger#70100
dwoz merged 2 commits into
saltstack:3008.xfrom
dwoz:dwoz/fix/log-unclosed-resource-warnings-3008x

Conversation

@dwoz

@dwoz dwoz commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Python filters ResourceWarning by default, so a bare warnings.warn(..., ResourceWarning) from a __del__ finalizer is silently dropped in production. Callers that missed a close() / destroy() / context-manager contract therefore never see the intended migration signal, and the leaked resource accumulates invisibly.

Concrete incident that motivated this

After commit 0c3f53d9172 removed the __del__-based cleanup from SaltEvent / MasterMinion / RunnerClient / WheelClient in favor of a ResourceWarning-emitting __del__, out-of-tree consumers like SSEAPE that relied on GC-time cleanup via the inline pattern

salt.utils.event.get_master_event(opts, sock_dir).fire_event(...)

began leaking one unix socket per fire-and-forget instance on 3008.2+. In a VMSP master pod the leak accumulated ~350 sockets/min → +180 MB of kernel slab per hour → OOM every ~78 min at the 1 GiB pod cap. The intended ResourceWarning never reached operator logs because ResourceWarning is silenced by default in production Python and Salt didn't route it through logging.

What this PR does

  • New helper salt.utils.resource_warnings.warn_until_close(message, source, category=ResourceWarning, log=None):
    • Emits the ResourceWarning (behavior preserved for dev / test / -W default::ResourceWarning).
    • Also logs the same message at WARNING level via the caller's module logger, so the signal survives Python's default filter and shows up in normal Salt logs.
    • Called only from finalizers — must never raise. Both the warnings.warn and the logger call are wrapped so interpreter-shutdown races don't propagate.
  • Wired into all 8 existing finalizers:
    • salt/utils/event.py (SaltEvent)
    • salt/utils/asynchronous.py (SyncWrapper)
    • salt/transport/tcp.py (Subscriber, TCPPuller, PubServer, _TCPPubServerPublisher)
    • salt/transport/ws.py (PublishClient x2)
  • Removes now-unused import warnings from the 4 files above.
  • Latent bug fix: four of the eight warnings.warn sites (both ws.py sites plus two tcp.py sites) had a missing f prefix, so {self!r} was printed as the literal string instead of interpolating. Fixed as part of the migration.

Test plan

  • New unit tests in tests/pytests/unit/utils/test_resource_warnings.py cover:
    • helper emits ResourceWarning AND a WARNING log record
    • falls back to module logger when no log passed
    • tolerates warnings.warn raising (interpreter-shutdown safety)
    • tolerates a broken logger
    • accepts a custom category (e.g. DeprecationWarning)
  • Standalone verification of behavior (both signals fire, both broken-side cases don't propagate).
  • CI green.

@dwoz
dwoz requested a review from a team as a code owner August 20, 2026 00:56
@dwoz dwoz added the test:full Run the full test suite label Aug 20, 2026
@dwoz dwoz added this to the Argon v3008.3 milestone Aug 20, 2026
@dwoz
dwoz force-pushed the dwoz/fix/log-unclosed-resource-warnings-3008x branch from f97dc11 to 1382857 Compare August 20, 2026 22:22
@dwoz
dwoz force-pushed the dwoz/fix/log-unclosed-resource-warnings-3008x branch from 1382857 to 73e2ffd Compare August 20, 2026 22:22
dwoz added 2 commits August 24, 2026 13:56
On this LTS branch, restore the GC-time destroy() fallback in
salt.minion.MasterMinion, salt.runner.RunnerClient, salt.wheel.WheelClient
and salt.utils.event.SaltEvent that commit 0c3f53d ("Remove __del__
methods from leak fixes") had removed. The warn_until_close call added in
this PR still runs first inside each __del__, so the missing-close()
contract is now visible via a WARNING-level log record AND the underlying
resource is still cleaned up -- callers that historically relied on GC-time
cleanup (e.g. sseape's fire-and-forget get_master_event(...).fire_event(...))
do not silently leak sockets while migrating to explicit destroy() /
context-manager use.

The companion change on master (Potassium) drops the destroy() fallback and
requires callers to be explicit; the WARNING here is the migration signal
for that upcoming behavior change.
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.

2 participants