fix: explicitly release GitConfigParser to prevent destructor error on Python 3.14#181
Open
matthiasbalke wants to merge 1 commit intohassio-addons:mainfrom
Open
Conversation
WalkthroughFinalizes the git config writer after setting values and strengthens repository cleanup by closing the git repo handle, removing its reference, forcing garbage collection, and then deleting the working directory. Also updates the Docker build to install the package from a specific Git revision. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merged
2 tasks
Author
|
This happens in my own addon repository: https://github.com/matthiasbalke/ha-addons-edge/tree/ef66f5317bf660f8f45f2a4436af6b511f869abf I validated the patch using my own addon repository. The error is gone now. Before After |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
repositoryupdater/repository.py (1)
215-219: Cleanup sequence is correct, but App objects may retain references togit_repo.The cleanup logic properly captures the working directory, closes the repo handle, deletes the reference, and forces garbage collection before removing the directory. This addresses the GitConfigParser destructor issue.
Based on the PR comments noting this fix "was not sufficient,"
Appobjects created inload_repository()receive a reference toself.git_repo(stored asself.repository). These references may prevent theRepoobject from being garbage collected even afterdel self.git_repo.Consider clearing App references before cleanup:
♻️ Clear App references before cleanup
def cleanup(self): """Cleanup after you leave.""" click.echo("Cleanup...", nl=False) working_dir = self.git_repo.working_dir + self.apps.clear() self.git_repo.close() del self.git_repo gc.collect() shutil.rmtree(working_dir, True) click.echo(crayons.green("Done"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@repositoryupdater/repository.py` around lines 215 - 219, Before closing and deleting self.git_repo, clear any lingering references from App instances that were assigned the repo in load_repository(): iterate over the App objects (those created in load_repository or stored on self, e.g. self.apps or wherever they are kept) and set each_app.repository = None (or delete the attribute) so they no longer reference self.git_repo; only after wiping those App.repository references proceed with self.git_repo.close(), del self.git_repo, gc.collect(), and shutil.rmtree(working_dir, True).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@repositoryupdater/repository.py`:
- Around line 215-219: Before closing and deleting self.git_repo, clear any
lingering references from App instances that were assigned the repo in
load_repository(): iterate over the App objects (those created in
load_repository or stored on self, e.g. self.apps or wherever they are kept) and
set each_app.repository = None (or delete the attribute) so they no longer
reference self.git_repo; only after wiping those App.repository references
proceed with self.git_repo.close(), del self.git_repo, gc.collect(), and
shutil.rmtree(working_dir, True).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef8a93c3-1bc0-4f6b-882f-e3856d757f6d
📒 Files selected for processing (1)
repositoryupdater/repository.py
matthiasbalke
force-pushed
the
fix/config-writer-not-released
branch
from
bab675c to
1777013
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
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.
Problem
In
repositoryupdater/github.py,config_writer()is called on the cloned repo but the returnedGitConfigParserobject is never explicitly released viaconfig.release().When
cleanup()later runsshutil.rmtree()on the temp directory, theGitConfigParserobject may still be alive in memory. Python 3.14's garbage collector then invokes__del__on it, which attempts to write to the already-deleted path, causing aFileNotFoundErrorand failing the workflow.Traceback observed:
Fix
Call
config.release()before returning fromclone(). This explicitly flushes and closes theGitConfigParser, preventing its destructor from running after the temp directory is removed.This follows the pattern recommended in the GitPython docs and is consistent with using
config_writer()as a context manager.Summary by CodeRabbit