fix(nginx_k8s): close file handles returned by container.pull#492
Open
tonyandrewmeyer wants to merge 3 commits into
Open
fix(nginx_k8s): close file handles returned by container.pull#492tonyandrewmeyer wants to merge 3 commits into
tonyandrewmeyer wants to merge 3 commits into
Conversation
container.pull() returns a real local file handle: ops streams the remote file's bytes into a tempfile and returns open() on it, so leaving the handle to be GC'd raises a ResourceWarning. Use a context manager and factor the cert-read pattern into _read_if_exists so the module is clean under -Werror. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
TLSConfigManager._sync_certificates had the same `pull(...).read()` anti- pattern as Nginx._configure_tls — three more unclosed file handles. Factor the same `_read_if_exists` helper into _tls_config.py and add regression tests under tests/unit/test_no_resource_leaks.py that capture ResourceWarning via `catch_warnings(record=True)` for both the cert-read and nginx-config-read paths. Each test fails with the message "AssertionError: unclosed file ..." if the corresponding fix is reverted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tonyandrewmeyer
commented
May 26, 2026
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
1 participant
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.
Summary
Container.pull()returns a real local file handle — under real pebble it'sopen()on atempfile.NamedTemporaryFile, underops.testingit'sopen()on the mounted path. Letting it fall out of scope without.close()emitsResourceWarning(and leaks a file descriptor until GC runs)._nginx.pyand_tls_config.pyinwithblocks, factor the repeatedpull-if-existspattern into a small_read_if_existshelper in each module.tests/unit/test_no_resource_leaks.pywith two regression tests that captureResourceWarningviawarnings.catch_warnings(record=True)and assert nounclosed filewarning is emitted. Each test was verified to fail withAssertionError: unclosed file <...>if the corresponding fix is reverted.Note on the test design: the tests filter captured warnings by the
'unclosed file'substring and rely on CPython's refcount-based collection (no explicitgc.collect()), so unrelated upstream leaks (e.g. a known one inscenario.Context) won't false-positive these tests.Test plan
just unit nginx_k8s— 59/59 passing, including the two new regression tests._nginx.pyalone makestest_has_config_changed_closes_pull_handlefail withunclosed file <…/nginx.conf…>._tls_config.pyalone makestest_sync_certificates_closes_pull_handlesfail withunclosed file <…/server.cert…>.🤖 Generated with Claude Code