Public Access
DEVX-164: fix(ci): retry Vikunja lookups and surface self-approval merge failures
This commit was merged in pull request #325.
This commit is contained in:
@@ -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
|
||||||
+44
-21
@@ -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
|
## 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
|
Two defects hit auto-merge during S02 work:
|
||||||
"offset mismatch" race condition needs more than ~15s to recover. The
|
|
||||||
current backoff is 5s-20s with 3 attempts (total ~15s of waiting).
|
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
|
## 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-1: Wrap the task-list pagination in `get_vikunja_task_title` with a
|
||||||
REQ-2: Increase backoff from 5-20s to 10-60s exponential
|
bounded retry (tenacity, ~4 attempts, exponential backoff) covering
|
||||||
REQ-3: Add visible logging between retry attempts (click.echo)
|
`APIError` and `requests.RequestException`. A genuinely missing task
|
||||||
REQ-4: All tests pass with 100% coverage
|
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
|
## Test Plan
|
||||||
- Unit tests verify retry count and backoff parameters
|
|
||||||
- Unit tests verify logging output on retry
|
- New tests: retry-then-success on transient APIError; retry-exhaustion
|
||||||
- Manual: trigger build-images workflow and verify retries visible in logs
|
still raises; missing task still raises; 405 error includes
|
||||||
|
approvals diagnostic.
|
||||||
|
- `make pytest-cov`, `make lint-all`.
|
||||||
|
|
||||||
## Deploy Plan
|
## 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
|
## Rollback Plan
|
||||||
- Revert the merge commit
|
|
||||||
|
- Revert; previous behavior returns.
|
||||||
|
|
||||||
## Acceptance Criteria
|
## 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-1: Vikunja task-list retries transient API failures
|
||||||
- [x] REQ-3: Add visible logging between retry attempts (click.echo)
|
- [x] REQ-2: 405 merge error reports approval state + self-approval hint
|
||||||
- [x] REQ-4: All tests pass with 100% coverage
|
- [x] REQ-3: Regression tests added and passing
|
||||||
|
|||||||
@@ -21,10 +21,12 @@ Usage:
|
|||||||
"""
|
"""
|
||||||
|
|
||||||
import re
|
import re
|
||||||
|
import time
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from typing import Any
|
from typing import Any
|
||||||
|
|
||||||
import click
|
import click
|
||||||
|
import requests
|
||||||
from dotenv import load_dotenv # pyright: ignore[reportMissingImports,reportUnknownVariableType]
|
from dotenv import load_dotenv # pyright: ignore[reportMissingImports,reportUnknownVariableType]
|
||||||
|
|
||||||
from devx.api_clients import GiteaClient, VikunjaClient
|
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:
|
def get_vikunja_task_title(task_id: str) -> str:
|
||||||
"""Fetch the Vikunja task title for the given DEVX-N identifier.
|
"""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)
|
client = VikunjaClient(VIKUNJA_API_URL, token)
|
||||||
page = 1
|
page = 1
|
||||||
while True:
|
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:
|
if not tasks:
|
||||||
break
|
break
|
||||||
matches = [t for t in tasks if t.get("identifier") == task_id]
|
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.
|
# Exit cleanly — the rebase triggers a new CI run that will retry.
|
||||||
return
|
return
|
||||||
else:
|
else:
|
||||||
raise click.ClickException(
|
hint = ""
|
||||||
_(
|
if e.status == 405:
|
||||||
"Merge failed with HTTP {status}: {message}\n"
|
# Implements: REQ-2 — surface approval state. When the PR author
|
||||||
"Please check the PR is ready and you have merge rights.",
|
# and the CI reviewer token map to the same Gitea user,
|
||||||
status=e.status,
|
# self-approval is rejected and the merge fails 405.
|
||||||
message=e.message,
|
try:
|
||||||
)
|
reviews = client.get_pr_reviews(pr_num)
|
||||||
) from None
|
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(
|
click.echo(
|
||||||
_(
|
_(
|
||||||
|
|||||||
@@ -127,6 +127,14 @@
|
|||||||
"ru": "\nОтсутствующая документация:",
|
"ru": "\nОтсутствующая документация:",
|
||||||
"zh": "\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.": {
|
"\nNo stale version references found.": {
|
||||||
"bg": "",
|
"bg": "",
|
||||||
"de": "",
|
"de": "",
|
||||||
|
|||||||
@@ -468,3 +468,138 @@ def test_main_module_block() -> None:
|
|||||||
exec(compile(source, am.__file__, "exec"), namespace)
|
exec(compile(source, am.__file__, "exec"), namespace)
|
||||||
# Verify main is callable
|
# Verify main is callable
|
||||||
assert callable(namespace["main"])
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user