Skip to content

fix: Add functions to clean Git lock files locally and remotely #152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tomchon wants to merge 4 commits into from
Closed

fix: Add functions to clean Git lock files locally and remotely #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d8be706
fix: Add functions to clean Git lock files locally and remotely
tomchon Feb 4, 2026
42f859c
fix: Update checkout action reference to use fix/clean-git-locks bran…
tomchon Feb 4, 2026
5e5d711
fix: Refactor clean_git_locks methods to improve logging and code str…
tomchon Feb 4, 2026
0193fb1
fix: Enhance error handling in clean_git_locks_remote method
tomchon Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 59 additions & 4 deletions .github/scripts/prepare_test_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import concurrent.futures
import glob
import json
import paramiko
import logging
import os
import platform
import shlex
import socket
import sys
import subprocess
Expand Down Expand Up @@ -127,7 +130,57 @@ def _set_branch_variables(self):
self.utils.set_env_var(
"GITHUB_RUN_ATTEMPT", self.run_attempt, os.getenv("GITHUB_ENV", "")
)


def clean_git_locks(self, repo_path):
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function accepts a repo_path parameter but doesn't specify its type. Based on usage in _prepare_repo (line 234), this function receives a Path object. Consider adding type hints to make the expected type explicit, consistent with other methods in the class. For example: def clean_git_locks(self, repo_path: Path):.

Copilot uses AI. Check for mistakes.
"""clean .git/*.lock files in a local repository"""
git_dir = os.path.join(repo_path, '.git')
if not os.path.exists(git_dir):
return

lock_files = glob.glob(os.path.join(git_dir, '*.lock'))
for lock_file in lock_files:
try:
os.remove(lock_file)
Comment on lines +136 to +143
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function accepts repo_path but doesn't validate its type or convert it to a string. Looking at line 232 where this function is called, repo_path is a Path object (as seen from self.wk and self.wkc being Path objects at lines 45-46). However, os.path.join() and os.path.exists() work with both Path objects and strings in Python 3, but glob.glob() expects a string pattern. While this may work due to implicit string conversion, it would be more explicit and safer to convert the Path to a string or use Path methods consistently (e.g., Path(repo_path) / '.git').

Suggested change
git_dir = os.path.join(repo_path, '.git')
if not os.path.exists(git_dir):
return
lock_files = glob.glob(os.path.join(git_dir, '*.lock'))
for lock_file in lock_files:
try:
os.remove(lock_file)
git_dir = Path(repo_path) / ".git"
if not git_dir.exists():
return
lock_files = git_dir.glob("*.lock")
for lock_file in lock_files:
try:
lock_file.unlink()

Copilot uses AI. Check for mistakes.
logger.info(f"Removed lock file: {lock_file}")
except Exception as e:
logger.info(f"Failed to remove {lock_file}: {e}")

def clean_git_locks_remote(self, host: str, username: str, repo_path: str, password: str = None, key_filename: str = None):
"""
Clean .git/*.lock files in a remote repository via SSH.
:param host: Remote host IP or domain
:param username: SSH username
:param repo_path: Path to the repository on the remote host
:param password: SSH password (optional, for password login)
:param key_filename: Path to private key file (optional, for key-based login)
"""
ssh = paramiko.SSHClient()
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AutoAddPolicy allows any SSH host key without verification, which is a security risk. This could make the connection vulnerable to man-in-the-middle attacks. However, I note that the existing _execute_remote_command method (line 346) also uses AutoAddPolicy, so this is consistent with the current codebase pattern. If this pattern is acceptable for your infrastructure (e.g., trusted internal network), this is fine. Otherwise, consider using a more secure approach like RejectPolicy with properly configured known_hosts.

Suggested change
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
# Load known host keys and reject unknown hosts to avoid MITM vulnerabilities
ssh.load_system_host_keys()
user_known_hosts = os.path.expanduser("~/.ssh/known_hosts")
if os.path.exists(user_known_hosts):
ssh.load_host_keys(user_known_hosts)
ssh.set_missing_host_key_policy(paramiko.RejectPolicy())

Copilot uses AI. Check for mistakes.
try:
ssh.connect(
hostname=host,
username=username,
password=password,
key_filename=key_filename,
)
git_dir = f"{repo_path}/.git"
safe_git_dir = shlex.quote(git_dir)
cmd = f"find {safe_git_dir} -name '*.lock' -type f -exec rm -f {{}} \\;"
stdin, stdout, stderr = ssh.exec_command(cmd)
out = stdout.read().decode()
err = stderr.read().decode()
logger.info(f"[{host}] Cleaned locks in {git_dir}")
if out:
logger.info(out)
if err:
logger.info(err)
return True
except Exception as e:
logger.error(f"[{host}] Failed to clean git locks in {repo_path}: {e}")
return False
finally:
ssh.close()

def run_git_ref_lock_cleaner(self, repo_path):
logger.info(f"Running cleaner script: {script_path}")
if not repo_path.exists():
Expand Down Expand Up @@ -165,6 +218,7 @@ def prepare_repositories(self):
logger.info(f"Preparing TDinternal in {self.wkdir}...")
self.run_git_ref_lock_cleaner(self.wk)
self.run_git_ref_lock_cleaner(self.wkc)

if (
self.inputs.get("specified_source_branch") == "unavailable"
and self.inputs.get("specified_target_branch") == "unavailable"
Expand All @@ -181,7 +235,7 @@ def _prepare_repo(self, repo_path, branch):
"""Prepare a single repository"""
if not repo_path.exists():
raise FileNotFoundError(f"Repository path not found: {repo_path}")

self.clean_git_locks(repo_path)
cmds = [
f"cd {repo_path} && git reset --hard",
f"cd {repo_path} && git clean -f",
Expand Down Expand Up @@ -292,8 +346,6 @@ def get_testing_params(self):
def _execute_remote_command(self, host_config, command):
"""Execute a command on remote host via SSH"""
try:
import paramiko

ssh = paramiko.SSHClient()
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())

Expand Down Expand Up @@ -339,7 +391,9 @@ def _prepare_repositories_remote(self, host_config):
logger.error(f"[{host}] Failed to send cleaner script: {e}")
return False


# 2. Prepare TDinternal
self.clean_git_locks_remote(host, username, f"{workdir}/TDinternal")
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function calls self.clean_git_locks_remote(host, username, ...) but doesn't provide the required password or key_filename parameters that are needed for SSH authentication. Looking at the host_config structure used elsewhere in the code (e.g., line 349-352), these parameters should be extracted from host_config if available, or the function should handle authentication properly. Without proper authentication parameters, the SSH connection will fail.

Copilot uses AI. Check for mistakes.
branch = self.target_branch if all(
self.inputs.get(k) == "unavailable"
for k in ["specified_source_branch", "specified_target_branch", "specified_pr_number"]
Expand All @@ -355,6 +409,7 @@ def _prepare_repositories_remote(self, host_config):
return False

# 3. Prepare community
self.clean_git_locks_remote(host, username, f"{workdir}/TDinternal/community")
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function calls self.clean_git_locks_remote(host, username, ...) but doesn't provide the required password or key_filename parameters that are needed for SSH authentication. Looking at the host_config structure used elsewhere in the code (e.g., line 349-352), these parameters should be extracted from host_config if available, or the function should handle authentication properly. Without proper authentication parameters, the SSH connection will fail.

Copilot uses AI. Check for mistakes.
comm_cmd = (
f"cd {workdir}/TDinternal/community && "
f"python3 {remote_script} && "
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/new-framework-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ jobs:
uses: actions/checkout@v4
with:
repository: "taosdata/.github"
ref: "main"
ref: "fix/clean-git-locks"
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow is checking out from a feature branch fix/clean-git-locks instead of the main branch. This appears to be a temporary change for testing purposes and should be reverted to main before merging this PR, unless there's a specific reason to reference this branch permanently. Typically, workflows should reference stable branches like main to avoid dependencies on feature branches that may be deleted after merge.

Suggested change
ref: "fix/clean-git-locks"
ref: "main"

Copilot uses AI. Check for mistakes.
fetch-depth: 0

- name: Debug env
Expand Down