diff --git a/hgitaly/service/repository.py b/hgitaly/service/repository.py index c306a7d4c9a2c79cb97f9339cfb13e79b7c21ccd_aGdpdGFseS9zZXJ2aWNlL3JlcG9zaXRvcnkucHk=..10d6717caedd4323da7056f7cdb3fadc334d4c5f_aGdpdGFseS9zZXJ2aWNlL3JlcG9zaXRvcnkucHk= 100644 --- a/hgitaly/service/repository.py +++ b/hgitaly/service/repository.py @@ -78,7 +78,7 @@ chunked, ) from ..workdir import ( - remove_all_workdirs, + remove_all_workdirs_bare, ) from ..stub.repository_pb2 import ( BackupCustomHooksRequest, @@ -506,7 +506,6 @@ def RemoveRepository(self, request: RemoveRepositoryRequest, context) -> RemoveRepositoryResponse: - logger = LoggerAdapter(base_logger, context) # The protocol comment says, as of Gitaly 14.8: # RemoveRepository will move the repository to # `+gitaly/tmp/<relative_path>_removed` and @@ -515,7 +514,10 @@ # asynchronous (as the Rails app does), but it is not in the # Gitaly server implementation. The renaming is done for # atomicity purposes. - - repo = self.load_repo(request.repository, context) - repo_path = repo.root + try: + repo_path = self.repo_disk_path(request.repository, context) + except KeyError as exc: + self.handle_key_error(context, exc.args) + except ValueError as exc: + self.handle_value_error(context, exc.args) @@ -521,14 +523,8 @@ - try: - workdirs = self.repo_workdirs_root(request.repository, context) - # TODO since we're doing this, we might as well do it from - # the onset and avoid repeating exception treatment OR we could - # do the cleanup in `CreateRepository` and friends - remove_all_workdirs(workdirs, repo) - except Exception: - # we don't want to prevent the removal for this - logger.exception("Error when removing working directories for %r", - message.Logging(request)) + if not os.path.exists(repo_path): + # same error message as Gitaly (probably no need to repeat + # repo details, since request often logged client-side) + context.abort(StatusCode.NOT_FOUND, "repository does not exist") trash_path = os.path.join( self.temp_dir(request.repository.storage_name, context), @@ -532,7 +528,7 @@ trash_path = os.path.join( self.temp_dir(request.repository.storage_name, context), - os.path.basename(repo_path) + b'+removed') + os.path.basename(repo_path) + b'+removed') # TODO randomize # The rename being atomic, it avoids leaving a crippled repo behind # in case of problem in the removal. # TODO Gitaly also performs some kind of locking (not clear @@ -541,7 +537,13 @@ # and finally the vote related to the multi-phase commit for praefect os.rename(repo_path, trash_path) - shutil.rmtree(trash_path) # not atomic + # non-atomic cleanups + + # at this point, the repo officially does not exist any more, the + # roster file does not matter, cleanup workdirs if any. + remove_all_workdirs_bare(self.repo_workdirs_root(request.repository, + context)) + shutil.rmtree(trash_path) return RemoveRepositoryResponse() def SetFullPath(self, request: SetFullPathRequest, diff --git a/hgitaly/workdir.py b/hgitaly/workdir.py index c306a7d4c9a2c79cb97f9339cfb13e79b7c21ccd_aGdpdGFseS93b3JrZGlyLnB5..10d6717caedd4323da7056f7cdb3fadc334d4c5f_aGdpdGFseS93b3JrZGlyLnB5 100644 --- a/hgitaly/workdir.py +++ b/hgitaly/workdir.py @@ -363,10 +363,5 @@ This is to be used when preparing to remove the repository itself. """ with locked_roster(repo) as (rosterf, new_rosterf): - try: - if os.path.exists(workdirs_root): - shutil.rmtree(workdirs_root) - except Exception: - logger.exception("Failed to remove working directories " - "for repo at %r", repo) + remove_all_workdirs_bare(workdirs_root) # new_rosterf being empty, this will void the existing one. @@ -372,1 +367,19 @@ # new_rosterf being empty, this will void the existing one. + + +def remove_all_workdirs_bare(workdirs_root): + """Removal of workdirs_root with no roster management. + + To be used either with roster being managed (lock acquired by caller etc.) + or if the roster does not matter any more (repository is being entirely + removed). + + Does not raise exceptions, as the caller typically will prefer to proceed + with other cleanups if possible. + """ + try: + if os.path.exists(workdirs_root): + shutil.rmtree(workdirs_root) + except Exception: + logger.exception("Failed to remove working directories at %r", + workdirs_root) diff --git a/tests_with_gitaly/test_repository_service.py b/tests_with_gitaly/test_repository_service.py index c306a7d4c9a2c79cb97f9339cfb13e79b7c21ccd_dGVzdHNfd2l0aF9naXRhbHkvdGVzdF9yZXBvc2l0b3J5X3NlcnZpY2UucHk=..10d6717caedd4323da7056f7cdb3fadc334d4c5f_dGVzdHNfd2l0aF9naXRhbHkvdGVzdF9yZXBvc2l0b3J5X3NlcnZpY2UucHk= 100644 --- a/tests_with_gitaly/test_repository_service.py +++ b/tests_with_gitaly/test_repository_service.py @@ -192,7 +192,6 @@ repository=Repository(storage_name='unknown', relative_path='/some/path')) assert_compare_errors( - same_details=False, repository=Repository(storage_name=grpc_repo.storage_name, relative_path='no/such/path'))