[core] Retry gh CLI calls on transient network errors in CI scripts (#18292)

script/ci_memory_impact_comment.py

@@ -20,17 +20,21 @@ from jinja2 import Environment, FileSystemLoader
 sys.path.insert(0, str(Path(__file__).parent.parent))
 
 # pylint: disable=wrong-import-position
+from helpers import run_gh_command  # noqa: E402
 
 # Comment marker to identify our memory impact comments
 COMMENT_MARKER = "<!-- esphome-memory-impact-analysis -->"
 
 
-def run_gh_command(args: list[str], operation: str) -> subprocess.CompletedProcess:
-    """Run a gh CLI command with error handling.
+def run_gh_command_logged(
+    args: list[str], operation: str, *, retry: bool = True
+) -> subprocess.CompletedProcess:
+    """Run a gh CLI command with retries and error reporting.
 
     Args:
         args: Command arguments (including 'gh')
         operation: Description of the operation for error messages
+        retry: Pass False for non-idempotent commands (see run_gh_command)
 
     Returns:
         CompletedProcess result
@@ -39,12 +43,7 @@ def run_gh_command(args: list[str], operation: str) -> subprocess.CompletedProce
         subprocess.CalledProcessError: If command fails (with detailed error output)
     """
     try:
-        return subprocess.run(
-            args,
-            check=True,
-            capture_output=True,
-            text=True,
-        )
+        return run_gh_command(args, retry=retry)
     except subprocess.CalledProcessError as e:
         print(
             f"ERROR: {operation} failed with exit code {e.returncode}", file=sys.stderr
@@ -472,7 +471,7 @@ def find_existing_comment(pr_number: str) -> str | None:
     print(f"DEBUG: Looking for existing comment on PR #{pr_number}", file=sys.stderr)
 
     # Use gh api to get comments directly - this returns the numeric id field
-    result = run_gh_command(
+    result = run_gh_command_logged(
         [
             "gh",
             "api",
@@ -535,7 +534,7 @@ def update_existing_comment(comment_id: str, comment_body: str) -> None:
     """
     print(f"DEBUG: Updating existing comment {comment_id}", file=sys.stderr)
     print(f"DEBUG: Comment body length: {len(comment_body)} bytes", file=sys.stderr)
-    result = run_gh_command(
+    result = run_gh_command_logged(
         [
             "gh",
             "api",
@@ -562,9 +561,12 @@ def create_new_comment(pr_number: str, comment_body: str) -> None:
     """
     print(f"DEBUG: Posting new comment on PR #{pr_number}", file=sys.stderr)
     print(f"DEBUG: Comment body length: {len(comment_body)} bytes", file=sys.stderr)
-    result = run_gh_command(
+    # Creating a comment is not idempotent: a retry after a dropped response
+    # could post the same comment twice, so fail on the first error instead.
+    result = run_gh_command_logged(
         ["gh", "pr", "comment", pr_number, "--body", comment_body],
         operation="Create PR comment",
+        retry=False,
     )
     print(f"DEBUG: Post response: {result.stdout}", file=sys.stderr)
 

script/helpers.py

@@ -469,6 +469,77 @@ def get_target_branch() -> str | None:
     return None
 
 
+# Substrings (matched case-insensitively against gh's stderr) that identify
+# transient failures worth retrying: server errors (HTTP 5xx) and dropped or
+# failed connections. Permanent failures (bad auth, missing PR, the 300-file
+# diff limit) never match so callers see them immediately. Phrases are
+# anchored so gh's GraphQL "Could not resolve to a PullRequest" (a missing
+# PR) never classifies as a DNS failure.
+_TRANSIENT_GH_ERROR_RE = re.compile(
+    r"http 5\d\d"
+    r"|timed out|timeout"
+    r"|connection (?:reset|refused|closed)"
+    r"|no such host|could not resolve host"
+    # gh intercepts DNS errors and prints its own "error connecting to
+    # <host>" text; the Go phrases above are kept as a hedge in case a
+    # future gh stops swallowing the underlying error
+    r"|error connecting to"
+    r"|failed to verify certificate"
+    # Go reports a server-closed connection as 'Post "<url>": EOF'; the
+    # quote-and-colon anchor keeps a URL or message body containing the
+    # letters from matching
+    r"|unexpected eof"
+    r'|": eof'
+    r"|network is unreachable"
+    r"|temporary failure"
+)
+
+# Same retry policy as git network commands in esphome/git.py: 3 attempts
+# with 2s/4s backoff.
+_GH_MAX_ATTEMPTS = 3
+
+
+def run_gh_command(
+    args: list[str], *, retry: bool = True
+) -> subprocess.CompletedProcess[str]:
+    """Run a gh CLI command, retrying transient network and server failures.
+
+    Args:
+        args: Full command line, including the leading "gh".
+        retry: Pass False for commands that are not idempotent (e.g. posting
+            a comment), where a retry after a dropped response could repeat
+            a write that already succeeded server-side.
+
+    Returns:
+        CompletedProcess with captured text output.
+
+    Raises:
+        subprocess.CalledProcessError: If the command fails with a permanent
+            error, or is still failing after the retries are exhausted.
+    """
+    attempts = _GH_MAX_ATTEMPTS if retry else 1
+    attempt = 0
+    while True:
+        try:
+            return subprocess.run(
+                args, check=True, capture_output=True, text=True, close_fds=False
+            )
+        except subprocess.CalledProcessError as err:
+            attempt += 1
+            stderr = err.stderr or ""
+            if attempt >= attempts or not _TRANSIENT_GH_ERROR_RE.search(stderr.lower()):
+                raise
+            delay = 2**attempt
+            # Only the leading arguments: comment-update calls carry the
+            # whole multi-KB comment body in the argument list
+            print(
+                f"WARNING: {' '.join(args[:3])} failed: {stderr.strip()}; "
+                f"retrying in {delay}s (attempt {attempt}/{attempts})",
+                file=sys.stderr,
+            )
+            time.sleep(delay)
+
+
 @cache
 def _get_changed_files_github_actions() -> list[str] | None:
     """Get changed files in GitHub Actions environment.
@@ -542,10 +613,22 @@ def changed_files(branch: str | None = None) -> list[str]:
 
 
 def _get_changed_files_from_command(command: list[str]) -> list[str]:
-    """Run a git command to get changed files and return them as a list."""
-    proc = subprocess.run(command, capture_output=True, text=True, check=False)
-    if proc.returncode != 0:
-        raise Exception(f"Command failed: {' '.join(command)}\nstderr: {proc.stderr}")
+    """Run a git or gh command to get changed files and return them as a list."""
+    if command[0] == "gh":
+        try:
+            proc = run_gh_command(command)
+        except subprocess.CalledProcessError as e:
+            raise Exception(
+                f"Command failed: {' '.join(command)}\nstderr: {e.stderr}"
+            ) from e
+    else:
+        proc = subprocess.run(
+            command, capture_output=True, text=True, check=False, close_fds=False
+        )
+        if proc.returncode != 0:
+            raise Exception(
+                f"Command failed: {' '.join(command)}\nstderr: {proc.stderr}"
+            )
 
     changed_files = splitlines_no_ends(proc.stdout)
     cwd = Path.cwd()

tests/script/test_helpers.py

@@ -20,6 +20,7 @@ changed_files = helpers.changed_files
 filter_changed = helpers.filter_changed
 get_changed_components = helpers.get_changed_components
 _get_changed_files_from_command = helpers._get_changed_files_from_command
+run_gh_command = helpers.run_gh_command
 _get_pr_number_from_github_env = helpers._get_pr_number_from_github_env
 _get_changed_files_github_actions = helpers._get_changed_files_github_actions
 _filter_changed_ci = helpers._filter_changed_ci
@@ -1872,3 +1873,123 @@ def test_is_validate_only_file(filename: str, expected: bool, tmp_path: Path) ->
 def test_base_python_changed(files: list[str], expected: bool) -> None:
     """Only Python modules directly in esphome/ count as base Python changes."""
     assert helpers.base_python_changed(files) is expected
+
+
+def _gh_error(stderr: str) -> subprocess.CalledProcessError:
+    return subprocess.CalledProcessError(1, ["gh"], output="", stderr=stderr)
+
+
+def _gh_success(stdout: str = "ok\n") -> subprocess.CompletedProcess:
+    return subprocess.CompletedProcess(["gh"], 0, stdout=stdout, stderr="")
+
+
+def test_run_gh_command_success() -> None:
+    """A successful command returns without retrying."""
+    with patch("helpers.subprocess.run", return_value=_gh_success()) as mock_run:
+        result = run_gh_command(["gh", "pr", "diff", "123", "--name-only"])
+
+    assert result.stdout == "ok\n"
+    mock_run.assert_called_once()
+
+
+@pytest.mark.parametrize(
+    "second_error",
+    [
+        (
+            'Post "https://api.github.com/graphql": tls: failed to verify'
+            " certificate: x509: certificate is not valid for any names,"
+            " but wanted to match api.github.com"
+        ),
+        'Post "https://api.github.com/graphql": EOF',
+        (
+            "error connecting to api.github.com\n"
+            "check your internet connection or https://githubstatus.com"
+        ),
+    ],
+)
+def test_run_gh_command_retries_transient_error(second_error: str) -> None:
+    """Transient server errors are retried with 2s/4s backoff."""
+    with (
+        patch(
+            "helpers.subprocess.run",
+            side_effect=[
+                _gh_error("HTTP 502: 502 Bad Gateway (https://api.github.com/graphql)"),
+                _gh_error(second_error),
+                _gh_success(),
+            ],
+        ) as mock_run,
+        patch("helpers.time.sleep") as mock_sleep,
+    ):
+        result = run_gh_command(["gh", "pr", "diff", "123", "--name-only"])
+
+    assert result.stdout == "ok\n"
+    assert mock_run.call_count == 3
+    assert [call.args[0] for call in mock_sleep.call_args_list] == [2, 4]
+
+
+def test_run_gh_command_gives_up_after_max_attempts() -> None:
+    """A persistent transient error raises after the third attempt."""
+    with (
+        patch(
+            "helpers.subprocess.run",
+            side_effect=_gh_error("HTTP 503: Service Unavailable"),
+        ) as mock_run,
+        patch("helpers.time.sleep") as mock_sleep,
+        pytest.raises(subprocess.CalledProcessError),
+    ):
+        run_gh_command(["gh", "pr", "diff", "123", "--name-only"])
+
+    assert mock_run.call_count == 3
+    assert mock_sleep.call_count == 2
+
+
+@pytest.mark.parametrize(
+    "stderr",
+    [
+        "HTTP 404: Not Found (https://api.github.com/repos/x)",
+        "HTTP 401: Bad credentials",
+        "HTTP 403: API rate limit exceeded for installation ID 123.",
+        "diff exceeded the maximum number of changed files (300)",
+        (
+            "GraphQL: Could not resolve to a PullRequest with the number of 999999."
+            " (repository.pullRequest)"
+        ),
+    ],
+)
+def test_run_gh_command_permanent_error_not_retried(stderr: str) -> None:
+    """Permanent failures raise immediately without any retry."""
+    with (
+        patch("helpers.subprocess.run", side_effect=_gh_error(stderr)) as mock_run,
+        patch("helpers.time.sleep") as mock_sleep,
+        pytest.raises(subprocess.CalledProcessError),
+    ):
+        run_gh_command(["gh", "pr", "diff", "123", "--name-only"])
+
+    mock_run.assert_called_once()
+    mock_sleep.assert_not_called()
+
+
+def test_run_gh_command_no_retry_for_non_idempotent_commands() -> None:
+    """retry=False fails on the first error even when it looks transient."""
+    with (
+        patch(
+            "helpers.subprocess.run",
+            side_effect=_gh_error("HTTP 502: 502 Bad Gateway"),
+        ) as mock_run,
+        patch("helpers.time.sleep") as mock_sleep,
+        pytest.raises(subprocess.CalledProcessError),
+    ):
+        run_gh_command(["gh", "pr", "comment", "123", "--body", "x"], retry=False)
+
+    mock_run.assert_called_once()
+    mock_sleep.assert_not_called()
+
+
+def test_get_changed_files_from_command_gh_failure_keeps_stderr() -> None:
+    """Failures from gh surface stderr so callers can detect the 300-file limit."""
+    stderr = "diff exceeded the maximum number of changed files (300)"
+    with (
+        patch("helpers.subprocess.run", side_effect=_gh_error(stderr)),
+        pytest.raises(Exception, match="maximum number of changed files"),
+    ):
+        _get_changed_files_from_command(["gh", "pr", "diff", "123", "--name-only"])