Skip to content

Commit 0598f71

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
1 parent 59ec528 commit 0598f71

File tree

2 files changed

+18
-7
lines changed

2 files changed

+18
-7
lines changed

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.error(
264+
"Could not remove temporary directory {}. Error: {}",
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: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1278,7 +1278,7 @@ def _validate_authentication_repository(
12781278
try:
12791279
_update_authentication_repository(repository_updater)
12801280
except Exception as e:
1281-
error_msg = e
1281+
error_msg = str(e)
12821282

12831283
# this is the repository cloned inside the temp directory
12841284
# we validate it before updating the actual authentication repository
@@ -1290,7 +1290,7 @@ def _validate_authentication_repository(
12901290
and users_auth_repo.last_validated_commit is None
12911291
and commits[0] != out_of_band_authentication
12921292
):
1293-
error_msg = UpdateFailedError(
1293+
error_msg = (
12941294
f"First commit of repository {auth_repo_name} does not match "
12951295
"out of band authentication commit"
12961296
)
@@ -1305,22 +1305,25 @@ def _validate_authentication_repository(
13051305
targets = validation_auth_repo.get_json(commits[-1], "metadata/targets.json")
13061306
test_repo = "test-auth-repo" in targets["signed"]["targets"]
13071307
if test_repo and expected_repo_type != UpdateType.TEST:
1308-
error_msg = UpdateFailedError(
1308+
error_msg = (
13091309
f"Repository {users_auth_repo.name} is a test repository. "
13101310
'Call update with "--expected-repo-type" test to update a test '
13111311
"repository"
13121312
)
13131313
elif not test_repo and expected_repo_type == UpdateType.TEST:
1314-
error_msg = UpdateFailedError(
1314+
error_msg = (
13151315
f"Repository {users_auth_repo.name} is not a test repository,"
13161316
' but update was called with the "--expected-repo-type" test'
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+
error_msg += f"Failed to cleanup repository updater: {str(e)}"
13211324

13221325
return (
13231326
commits,
1324-
error_msg,
1327+
UpdateFailedError(error_msg) if error_msg else None,
13251328
last_validated_commit,
13261329
)

0 commit comments

Comments
 (0)