Skip to content

Commit f55ef7c

Browse files
committed
fix: reraise original exception and windows specific cleanup errors
* Basing on #243, we are having some difficulty with pygit2 Repository file handlers being open. in `pygit.py` we instantiate the Repository class, which holds references to `pack` and `.idx` git objects and keeps files open. If taf and cleanup is attempted from same subprocess, Repository handler won't be deleted, and cleanup error occurs. Calling manual garbage collection did not help. This seems to be a known issue throughout the git community, see [1] and [2]. [1] - libgit2/pygit2#596 [2] - gitpython-developers/GitPython#546 Several workarounds were proposed by the community, but it seems that we can't fully replicate those workarounds. In [2] they attempt to workaround the issue by deleting git cache. That works for them because their git implementation has file handlers open in pure python, while we only do subprocess git calls. Our TODO: if these windows specific error issues keep persisting, figure out how to garbage collect pygit2 Repository handler correctly. For now settle on error handling, as this issue is specific to a subprocess call that calls taf/updater. Though we should keep track of progress of these known bugs, so we can fix them if they get addressed by pygit2 or gitpython chore: changelog
1 parent 59ec528 commit f55ef7c

File tree

3 files changed

+20
-2
lines changed

3 files changed

+20
-2
lines changed

CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ and this project adheres to [Semantic Versioning][semver].
2929

3030
### Fixed
3131

32+
- Fix Warn when git object cleanup fails (`idx`,`pack`) and include cleanup warning message ([259])
33+
34+
[259]: https://github.com/openlawlibrary/taf/pull/259
3235
[257]: https://github.com/openlawlibrary/taf/pull/257
3336
[256]: https://github.com/openlawlibrary/taf/pull/256
3437

taf/updater/handlers.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -257,7 +257,15 @@ def cleanup(self):
257257
self.validation_auth_repo.cleanup()
258258
temp_dir = Path(self.validation_auth_repo.path, os.pardir).parent
259259
if temp_dir.is_dir():
260-
shutil.rmtree(str(temp_dir), onerror=on_rm_error)
260+
try:
261+
shutil.rmtree(str(temp_dir), onerror=on_rm_error)
262+
except (OSError, PermissionError) as e:
263+
taf_logger.warning(
264+
"Could not remove temporary directory {}. Something went wrong during cleanup: {}",
265+
temp_dir,
266+
e,
267+
)
268+
raise e
261269

262270
def earliest_valid_expiration_time(self):
263271
# Only validate expiration time of the last commit

taf/updater/updater.py

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1317,7 +1317,14 @@ def _validate_authentication_repository(
13171317
)
13181318

13191319
# always cleanup repository updater
1320-
repository_updater.update_handler.cleanup()
1320+
try:
1321+
repository_updater.update_handler.cleanup()
1322+
except Exception as e:
1323+
taf_logger.warning(
1324+
"Failed to clean up temporary update files: {}. This is a known issue when running TAF in a subprocess. You could consider upgrading taf to see if cleanup errors persist",
1325+
e,
1326+
)
1327+
pass
13211328

13221329
return (
13231330
commits,

0 commit comments

Comments
 (0)