diff --git a/docs/specs/DEVX-164-docker-push-retry-historical.md b/docs/specs/DEVX-164-docker-push-retry-historical.md new file mode 100644 index 0000000..efc8079 --- /dev/null +++ b/docs/specs/DEVX-164-docker-push-retry-historical.md @@ -0,0 +1,34 @@ +# DEVX-164: Increase HTTP 500 retry count and backoff for docker push + +## Problem +The HTTP 500 retry logic (DEVX-162, DEVX-163) works correctly — 3 retry +attempts are made. But all 3 attempts fail because the Gitea registry's +"offset mismatch" race condition needs more than ~15s to recover. The +current backoff is 5s-20s with 3 attempts (total ~15s of waiting). + +## Approach +Increase retry count from 3 to 5 and backoff from 5-20s to 10-60s, +giving the registry up to ~2 minutes to recover. Add visible logging +between retry attempts so the CI logs show the retry happening. + +REQ-1: Increase retry count from 3 to 5 +REQ-2: Increase backoff from 5-20s to 10-60s exponential +REQ-3: Add visible logging between retry attempts (click.echo) +REQ-4: All tests pass with 100% coverage + +## Test Plan +- Unit tests verify retry count and backoff parameters +- Unit tests verify logging output on retry +- Manual: trigger build-images workflow and verify retries visible in logs + +## Deploy Plan +- Merge to master + +## Rollback Plan +- Revert the merge commit + +## Acceptance Criteria +- [x] REQ-1: Increase retry count from 3 to 5 +- [x] REQ-2: Increase backoff from 5-20s to 10-60s exponential +- [x] REQ-3: Add visible logging between retry attempts (click.echo) +- [x] REQ-4: All tests pass with 100% coverage diff --git a/docs/specs/DEVX-164.md b/docs/specs/DEVX-164.md index efc8079..69018aa 100644 --- a/docs/specs/DEVX-164.md +++ b/docs/specs/DEVX-164.md @@ -1,34 +1,57 @@ -# DEVX-164: Increase HTTP 500 retry count and backoff for docker push +# DEVX-164: auto-merge resilience — self-approval and Vikunja outage handling ## Problem -The HTTP 500 retry logic (DEVX-162, DEVX-163) works correctly — 3 retry -attempts are made. But all 3 attempts fail because the Gitea registry's -"offset mismatch" race condition needs more than ~15s to recover. The -current backoff is 5s-20s with 3 attempts (total ~15s of waiting). + +Two defects hit auto-merge during S02 work: + +1. `get_vikunja_task_title` crashes on transient Vikunja errors. The + Vikunja API returned 404/502 during a restart window (run 6093); + `list_project_tasks` treats 4xx as non-retryable `APIError`, so the + job failed immediately instead of riding out a short outage. +2. When a PR author and the workflow's reviewer token map to the same + Gitea user, the auto-approve step is rejected ("approve your own + pull is not allowed") and the merge fails `405: not enough + approvals`. The generic merge error gives no hint that an external + approval is the fix (hit on sso-bridge #18, #19, and infra #1647's + approvals-only failure mode). ## Approach -Increase retry count from 3 to 5 and backoff from 5-20s to 10-60s, -giving the registry up to ~2 minutes to recover. Add visible logging -between retry attempts so the CI logs show the retry happening. -REQ-1: Increase retry count from 3 to 5 -REQ-2: Increase backoff from 5-20s to 10-60s exponential -REQ-3: Add visible logging between retry attempts (click.echo) -REQ-4: All tests pass with 100% coverage +REQ-1: Wrap the task-list pagination in `get_vikunja_task_title` with a +bounded retry (tenacity, ~4 attempts, exponential backoff) covering +`APIError` and `requests.RequestException`. A genuinely missing task +still ends in the same "Could not find" ClickException. + +REQ-2: On merge `HTTP 405`, fetch PR reviews; when zero `APPROVED` +reviews exist, extend the error with the self-approval explanation and +the remediation (approve via a non-author account). + +REQ-3: Regression tests for both behaviors. + +## Files Affected + +- `src/devx/ci/auto_merge.py` +- `tests/unit/test_auto_merge.py` ## Test Plan -- Unit tests verify retry count and backoff parameters -- Unit tests verify logging output on retry -- Manual: trigger build-images workflow and verify retries visible in logs + +- New tests: retry-then-success on transient APIError; retry-exhaustion + still raises; missing task still raises; 405 error includes + approvals diagnostic. +- `make pytest-cov`, `make lint-all`. ## Deploy Plan -- Merge to master + +- Merge → next release publishes the package; consuming repos pick it + up on their next CI run (devx is pinned per-repo, bump via the usual + dependency PR flow). ## Rollback Plan -- Revert the merge commit + +- Revert; previous behavior returns. ## Acceptance Criteria -- [x] REQ-1: Increase retry count from 3 to 5 -- [x] REQ-2: Increase backoff from 5-20s to 10-60s exponential -- [x] REQ-3: Add visible logging between retry attempts (click.echo) -- [x] REQ-4: All tests pass with 100% coverage + +- [x] REQ-1: Vikunja task-list retries transient API failures +- [x] REQ-2: 405 merge error reports approval state + self-approval hint +- [x] REQ-3: Regression tests added and passing diff --git a/src/devx/ci/auto_merge.py b/src/devx/ci/auto_merge.py index 846a84f..636e1d6 100644 --- a/src/devx/ci/auto_merge.py +++ b/src/devx/ci/auto_merge.py @@ -21,10 +21,12 @@ Usage: """ import re +import time from pathlib import Path from typing import Any import click +import requests from dotenv import load_dotenv # pyright: ignore[reportMissingImports,reportUnknownVariableType] from devx.api_clients import GiteaClient, VikunjaClient @@ -110,6 +112,27 @@ def validate_pr_title(pr_title: str, task_id: str) -> None: ) +_VIKUNJA_LOOKUP_ATTEMPTS = 4 +_VIKUNJA_LOOKUP_BACKOFF = 5.0 + + +def _list_project_tasks(client: VikunjaClient, page: int) -> list[dict[str, Any]]: + """List Vikunja tasks with bounded retries for transient outages. + + Implements REQ-1: during a Vikunja restart the tasks endpoint can briefly + return 404/502; retry a few times so title validation rides out the window + instead of stranding an otherwise-valid PR. + """ + for attempt in range(1, _VIKUNJA_LOOKUP_ATTEMPTS + 1): + try: + return list(client.list_project_tasks(VIKUNJA_PROJECT_ID, page=page, per_page=DEFAULT_PER_PAGE)) + except (APIError, requests.RequestException): + if attempt == _VIKUNJA_LOOKUP_ATTEMPTS: + raise + time.sleep(_VIKUNJA_LOOKUP_BACKOFF * attempt) + raise AssertionError("unreachable") # pragma: no cover + + def get_vikunja_task_title(task_id: str) -> str: """Fetch the Vikunja task title for the given DEVX-N identifier. @@ -124,7 +147,7 @@ def get_vikunja_task_title(task_id: str) -> str: client = VikunjaClient(VIKUNJA_API_URL, token) page = 1 while True: - tasks = client.list_project_tasks(VIKUNJA_PROJECT_ID, page=page, per_page=DEFAULT_PER_PAGE) + tasks = _list_project_tasks(client, page) if not tasks: break matches = [t for t in tasks if t.get("identifier") == task_id] @@ -272,14 +295,28 @@ def main(branch: str, pr_title: str, repo: str, pr_number: str) -> None: # Exit cleanly — the rebase triggers a new CI run that will retry. return else: - raise click.ClickException( - _( - "Merge failed with HTTP {status}: {message}\n" - "Please check the PR is ready and you have merge rights.", - status=e.status, - message=e.message, - ) - ) from None + hint = "" + if e.status == 405: + # Implements: REQ-2 — surface approval state. When the PR author + # and the CI reviewer token map to the same Gitea user, + # self-approval is rejected and the merge fails 405. + try: + reviews = client.get_pr_reviews(pr_num) + if not any(r.get("state") == "APPROVED" for r in reviews): + hint = _( + "\nNo APPROVED review found on the PR. If the PR author and the" + " CI reviewer token map to the same Gitea user, self-approval is" + " rejected — approve the PR via a non-author account, then" + " re-run the auto-merge job." + ) + except APIError: + pass + msg = _( + "Merge failed with HTTP {status}: {message}\nPlease check the PR is ready and you have merge rights.", + status=e.status, + message=e.message, + ) + raise click.ClickException(msg + hint) from None click.echo( _( diff --git a/src/devx/translations.json b/src/devx/translations.json index 1fae5ed..4ca4642 100644 --- a/src/devx/translations.json +++ b/src/devx/translations.json @@ -127,6 +127,14 @@ "ru": "\nОтсутствующая документация:", "zh": "\n缺失的文档：" }, + "\nNo APPROVED review found on the PR. If the PR author and the CI reviewer token map to the same Gitea user, self-approval is rejected — approve the PR via a non-author account, then re-run the auto-merge job.": { + "bg": "\nВ PR не е намерено ревю APPROVED. Ако авторът на PR и токенът на CI рецензента са един и същ потребител в Gitea, самоодобрението се отхвърля — одобрете PR чрез друг акаунт и стартирайте отново задачата за автоматично сливане.", + "de": "\nKein APPROVED-Review im PR gefunden. Wenn der PR-Autor und das CI-Reviewer-Token demselben Gitea-Benutzer entsprechen, wird die Selbstgenehmigung abgelehnt — genehmigen Sie den PR über ein anderes Konto und führen Sie den Auto-Merge-Job erneut aus.", + "en": "\nNo APPROVED review found on the PR. If the PR author and the CI reviewer token map to the same Gitea user, self-approval is rejected — approve the PR via a non-author account, then re-run the auto-merge job.", + "pl": "\nNie znaleziono recenzji APPROVED w PR. Jeśli autor PR i token recenzenta CI mapują na tego samego użytkownika Gitea, samoakceptacja jest odrzucana — zatwierdź PR za pomocą innego konta, a następnie ponownie uruchom zadanie automatycznego scalania.", + "ru": "\nВ PR не найдено ревью APPROVED. Если автор PR и токен CI-ревьюера принадлежат одному и тому же пользователю Gitea, самоодобрение отклоняется — одобрите PR через другой аккаунт, затем повторно запустите задачу автоматического слияния.", + "zh": "\n在 PR 中未找到 APPROVED 评审。如果 PR 作者和 CI 评审者令牌映射到同一个 Gitea 用户，自我批准将被拒绝 — 请通过非作者账户批准该 PR，然后重新运行自动合并任务。" + }, "\nNo stale version references found.": { "bg": "", "de": "", diff --git a/tests/unit/test_auto_merge.py b/tests/unit/test_auto_merge.py index 5fbb426..07270c4 100644 --- a/tests/unit/test_auto_merge.py +++ b/tests/unit/test_auto_merge.py @@ -468,3 +468,138 @@ def test_main_module_block() -> None: exec(compile(source, am.__file__, "exec"), namespace) # Verify main is callable assert callable(namespace["main"]) + + +# -- DEVX-164: Vikunja outage resilience + self-approval diagnostics -- + + +class TestVikunjaLookupRetry: + """REQ-1: transient Vikunja API failures are retried, not fatal.""" + + @patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"}, clear=True) + @patch("devx.ci.auto_merge.time.sleep") + @patch("devx.ci.auto_merge.VikunjaClient") + def test_retries_transient_api_error_then_succeeds(self, mock_client_cls: MagicMock, mock_sleep: MagicMock) -> None: + """A 404/502 during a Vikunja restart is retried until tasks list.""" + mock_client = MagicMock() + mock_client.list_project_tasks.side_effect = [ + APIError(404, "Not Found"), + APIError(502, "Bad Gateway"), + [{"id": 1, "identifier": "DEVX-19", "title": "Add new feature"}], + ] + mock_client_cls.return_value = mock_client + validate_pr_title_matches_vikunja("DEVX-19: Add new feature", "DEVX-19") + assert mock_client.list_project_tasks.call_count == 3 + assert mock_sleep.call_count == 2 + + @patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"}, clear=True) + @patch("devx.ci.auto_merge.time.sleep") + @patch("devx.ci.auto_merge.VikunjaClient") + def test_retry_exhaustion_propagates_error(self, mock_client_cls: MagicMock, _mock_sleep: MagicMock) -> None: + """Persistent outage still fails after the bounded attempt count.""" + mock_client = MagicMock() + mock_client.list_project_tasks.side_effect = APIError(502, "Bad Gateway") + mock_client_cls.return_value = mock_client + with pytest.raises(APIError, match="Bad Gateway"): + validate_pr_title_matches_vikunja("DEVX-19: test", "DEVX-19") + assert mock_client.list_project_tasks.call_count == 4 + + @patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"}, clear=True) + @patch("devx.ci.auto_merge.time.sleep") + @patch("devx.ci.auto_merge.VikunjaClient") + def test_retries_connection_error(self, mock_client_cls: MagicMock, mock_sleep: MagicMock) -> None: + """Connection-level failures during restart are also retried.""" + import requests as req + + mock_client = MagicMock() + mock_client.list_project_tasks.side_effect = [ + req.ConnectionError("refused"), + [{"id": 1, "identifier": "DEVX-19", "title": "Found me"}], + ] + mock_client_cls.return_value = mock_client + validate_pr_title_matches_vikunja("DEVX-19: Found me", "DEVX-19") + assert mock_sleep.call_count == 1 + + @patch.dict("os.environ", {"VIKUNJA_TOKEN": "tok"}, clear=True) + @patch("devx.ci.auto_merge.VikunjaClient") + def test_missing_task_still_fails_without_retry_sleep(self, mock_client_cls: MagicMock) -> None: + """A healthy Vikunja that simply lacks the task fails as before.""" + mock_client = MagicMock() + mock_client.list_project_tasks.return_value = [] + mock_client_cls.return_value = mock_client + with pytest.raises(click.ClickException, match="Could not find"): + validate_pr_title_matches_vikunja("DEVX-99: test", "DEVX-99") + + +class TestMergeApprovalDiagnostics: + """REQ-2: merge 405 reports approval state + self-approval remediation.""" + + @patch.dict("os.environ", {"CI_GITEA_TOKEN": "tok", "VIKUNJA_TOKEN": "tok"}, clear=True) + @patch("devx.ci.auto_merge.validate_pr_title_matches_vikunja") + @patch("devx.ci.auto_merge.GiteaClient") + def test_405_without_approvals_shows_self_approval_hint( + self, mock_client_cls: MagicMock, _mock_validate: MagicMock, tmp_path, monkeypatch + ) -> None: # type: ignore[no-untyped-def] + monkeypatch.chdir(tmp_path) + mock_client = MagicMock() + mock_client.get_pr_commits.return_value = [ + {"commit": {"message": "fix: resolve timeout"}}, + ] + mock_client.merge_pr.side_effect = APIError(405, "Does not have enough approvals") + mock_client.get_pr_reviews.return_value = [] + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke( + main, + ["DEVX-19-fix-bug", "DEVX-19: Fix timeout", "owner/repo", "7"], + ) + assert result.exit_code != 0 + assert "Merge failed" in result.output + assert "APPROVED" in result.output + assert "non-author account" in result.output + + @patch.dict("os.environ", {"CI_GITEA_TOKEN": "tok", "VIKUNJA_TOKEN": "tok"}, clear=True) + @patch("devx.ci.auto_merge.validate_pr_title_matches_vikunja") + @patch("devx.ci.auto_merge.GiteaClient") + def test_405_with_approvals_omits_hint( + self, mock_client_cls: MagicMock, _mock_validate: MagicMock, tmp_path, monkeypatch + ) -> None: # type: ignore[no-untyped-def] + monkeypatch.chdir(tmp_path) + mock_client = MagicMock() + mock_client.get_pr_commits.return_value = [ + {"commit": {"message": "fix: resolve timeout"}}, + ] + mock_client.merge_pr.side_effect = APIError(405, "Does not have enough approvals") + mock_client.get_pr_reviews.return_value = [{"state": "APPROVED", "user": {"login": "kireto"}}] + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke( + main, + ["DEVX-19-fix-bug", "DEVX-19: Fix timeout", "owner/repo", "7"], + ) + assert result.exit_code != 0 + assert "Merge failed" in result.output + assert "non-author account" not in result.output + + @patch.dict("os.environ", {"CI_GITEA_TOKEN": "tok", "VIKUNJA_TOKEN": "tok"}, clear=True) + @patch("devx.ci.auto_merge.validate_pr_title_matches_vikunja") + @patch("devx.ci.auto_merge.GiteaClient") + def test_405_reviews_fetch_failure_still_raises( + self, mock_client_cls: MagicMock, _mock_validate: MagicMock, tmp_path, monkeypatch + ) -> None: # type: ignore[no-untyped-def] + """If the reviews lookup itself fails, the merge error still surfaces.""" + monkeypatch.chdir(tmp_path) + mock_client = MagicMock() + mock_client.get_pr_commits.return_value = [ + {"commit": {"message": "fix: resolve timeout"}}, + ] + mock_client.merge_pr.side_effect = APIError(405, "Does not have enough approvals") + mock_client.get_pr_reviews.side_effect = APIError(403, "Forbidden") + mock_client_cls.return_value = mock_client + runner = CliRunner() + result = runner.invoke( + main, + ["DEVX-19-fix-bug", "DEVX-19: Fix timeout", "owner/repo", "7"], + ) + assert result.exit_code != 0 + assert "Merge failed" in result.output