From 514d075a96fd2cfef79a7b8626b567f2e3e88aa5 Mon Sep 17 00:00:00 2001 From: Guy Vago Date: Tue, 14 Apr 2026 19:16:41 +0300 Subject: [PATCH 1/3] fix(gerrit): clean up temp repos after request processing The Gerrit server had two bugs: 1. The {action} path parameter was captured but ignored - the server always dispatched based on item.msg content. Now the action parameter determines the command, with item.msg only used for the "ask" action to provide the question text. Also made item.msg optional (defaults to empty string) since non-ask actions don't need it, and fixed `return HTTPException` to `raise HTTPException`. 2. prepare_repo() created a temp directory via mkdtemp() that was never cleaned up - remove_initial_comment() had shutil.rmtree commented out. Added a cleanup() method to GerritProvider that removes the temp repo, called from the server's finally block after each request. Also added __del__ as a safety net, and wired remove_initial_comment() to delegate to cleanup(). Co-Authored-By: Claude Opus 4.6 (1M context) --- pr_agent/git_providers/gerrit_provider.py | 23 +++++++++++++-- pr_agent/servers/gerrit_server.py | 34 +++++++++++++++++++---- 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/pr_agent/git_providers/gerrit_provider.py b/pr_agent/git_providers/gerrit_provider.py index ced150c915..0892c9a2f7 100644 --- a/pr_agent/git_providers/gerrit_provider.py +++ b/pr_agent/git_providers/gerrit_provider.py @@ -387,10 +387,27 @@ def publish_labels(self, labels): # but required by the interface pass + def cleanup(self): + """Remove the temporary cloned repository from disk.""" + if self.repo_path and pathlib.Path(self.repo_path).exists(): + try: + shutil.rmtree(self.repo_path, ignore_errors=True) + get_logger().info("Cleaned up temp repo at %s", self.repo_path) + except Exception as e: + get_logger().warning( + "Failed to clean up temp repo at %s: %s", + self.repo_path, e + ) + + def __del__(self): + """Safety net: clean up temp repo if cleanup() was not called.""" + try: + self.cleanup() + except Exception: + pass + def remove_initial_comment(self): - # remove repo, cloned in previous steps - # shutil.rmtree(self.repo_path) - pass + self.cleanup() def remove_comment(self, comment): pass diff --git a/pr_agent/servers/gerrit_server.py b/pr_agent/servers/gerrit_server.py index 1783f6b994..6df8e3dc7d 100644 --- a/pr_agent/servers/gerrit_server.py +++ b/pr_agent/servers/gerrit_server.py @@ -11,6 +11,7 @@ from pr_agent.agent.pr_agent import PRAgent from pr_agent.config_loader import get_settings, global_settings +from pr_agent.git_providers.gerrit_provider import GerritProvider from pr_agent.log import get_logger, setup_logger setup_logger() @@ -29,7 +30,7 @@ class Action(str, Enum): class Item(BaseModel): refspec: str project: str - msg: str + msg: str = "" @router.post("/api/v1/gerrit/{action}") @@ -37,16 +38,37 @@ async def handle_gerrit_request(action: Action, item: Item): get_logger().debug("Received a Gerrit request") context["settings"] = copy.deepcopy(global_settings) + # For the "ask" action, the question must come from item.msg. + # For all other actions, use the action path parameter as the command. if action == Action.ask: if not item.msg: - return HTTPException( + raise HTTPException( status_code=400, detail="msg is required for ask command" ) - await PRAgent().handle_request( - f"{item.project}:{item.refspec}", - f"/{item.msg.strip()}" - ) + command = f"/{action.value} {item.msg.strip()}" + else: + command = f"/{action.value}" + + try: + await PRAgent().handle_request( + f"{item.project}:{item.refspec}", + command + ) + finally: + # Clean up the cloned temp repo created by GerritProvider. + # The provider is cached in the starlette context during + # get_git_provider_with_context(). + try: + git_providers = context.get("git_provider", {}) + for provider in git_providers.values(): + if isinstance(provider, GerritProvider): + provider.cleanup() + except Exception: + get_logger().debug( + "Could not retrieve GerritProvider for cleanup; " + "temp directory may persist" + ) async def get_body(request): From 16e6a5e0e0cff537c35556c5e230650730521e8f Mon Sep 17 00:00:00 2001 From: Guy Vago Date: Wed, 15 Apr 2026 13:40:49 +0300 Subject: [PATCH 2/3] fix: narrow exception handling in Gerrit cleanup - cleanup(): catch (OSError, PermissionError) instead of broad Exception - gerrit_server finally block: catch (LookupError, RuntimeError) for starlette context access failures instead of bare Exception - Add isinstance guard for git_providers dict before iterating - Document that GerritProvider.__del__ acts as safety net when the provider is never stored in context (e.g. early failure) --- pr_agent/git_providers/gerrit_provider.py | 2 +- pr_agent/servers/gerrit_server.py | 19 ++++++++++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/pr_agent/git_providers/gerrit_provider.py b/pr_agent/git_providers/gerrit_provider.py index 0892c9a2f7..2a9eaf8cf7 100644 --- a/pr_agent/git_providers/gerrit_provider.py +++ b/pr_agent/git_providers/gerrit_provider.py @@ -393,7 +393,7 @@ def cleanup(self): try: shutil.rmtree(self.repo_path, ignore_errors=True) get_logger().info("Cleaned up temp repo at %s", self.repo_path) - except Exception as e: + except (OSError, PermissionError) as e: get_logger().warning( "Failed to clean up temp repo at %s: %s", self.repo_path, e diff --git a/pr_agent/servers/gerrit_server.py b/pr_agent/servers/gerrit_server.py index 6df8e3dc7d..cdbfe4d908 100644 --- a/pr_agent/servers/gerrit_server.py +++ b/pr_agent/servers/gerrit_server.py @@ -59,15 +59,24 @@ async def handle_gerrit_request(action: Action, item: Item): # Clean up the cloned temp repo created by GerritProvider. # The provider is cached in the starlette context during # get_git_provider_with_context(). + # + # We guard against two failure modes: + # 1. The starlette context is inaccessible (e.g. middleware not + # active) — caught by the outer try/except. + # 2. The provider was never stored in the context (e.g. an error + # occurred before get_git_provider_with_context ran) — the + # dict will simply be empty, and the GerritProvider.__del__ + # safety net handles cleanup on garbage collection. try: git_providers = context.get("git_provider", {}) - for provider in git_providers.values(): - if isinstance(provider, GerritProvider): - provider.cleanup() - except Exception: + if isinstance(git_providers, dict): + for provider in git_providers.values(): + if isinstance(provider, GerritProvider): + provider.cleanup() + except (LookupError, RuntimeError): get_logger().debug( "Could not retrieve GerritProvider for cleanup; " - "temp directory may persist" + "temp directory will be cleaned up by __del__" ) From 0d582732eceff7000f101220dcfa2bbc1f46d655 Mon Sep 17 00:00:00 2001 From: Guy Vago Date: Thu, 16 Apr 2026 11:17:14 +0300 Subject: [PATCH 3/3] fix: don't delete repo in remove_initial_comment, only in server finally block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remove_initial_comment() is called during the request lifecycle while the cloned repo is still needed by subsequent commands. Calling cleanup() here deletes the repo mid-execution, breaking all downstream operations. Revert to a no-op — actual cleanup happens in the server's finally block and in __del__ as a safety net. --- pr_agent/git_providers/gerrit_provider.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pr_agent/git_providers/gerrit_provider.py b/pr_agent/git_providers/gerrit_provider.py index 2a9eaf8cf7..0ff80b5007 100644 --- a/pr_agent/git_providers/gerrit_provider.py +++ b/pr_agent/git_providers/gerrit_provider.py @@ -407,7 +407,11 @@ def __del__(self): pass def remove_initial_comment(self): - self.cleanup() + # Do NOT call cleanup() here — this method is invoked during the + # request lifecycle while the cloned repo is still needed by + # subsequent commands. Actual cleanup happens in the server's + # finally block and in __del__ as a safety net. + pass def remove_comment(self, comment): pass