diff --git a/.gitea/workflows/build-images.yml b/.gitea/workflows/build-images.yml index 6de5a87..d8b18e0 100644 --- a/.gitea/workflows/build-images.yml +++ b/.gitea/workflows/build-images.yml @@ -77,6 +77,9 @@ jobs: CI_GITEA_API_TOKEN: ${{ secrets.CI_GITEA_API_TOKEN }} CI_GITEA_USERNAME: ${{ vars.CI_GITEA_USERNAME }} PYTHONPATH: src + # Serialize blob uploads to avoid Gitea registry race condition + # (BlobUploader.Append offset mismatch — see DEVX-162). + DOCKER_MAX_CONCURRENT_UPLOADS: "1" run: | . .venv/bin/activate export PATH="$HOME/.local/bin:$PATH" diff --git a/docs/specs/DEVX-162.md b/docs/specs/DEVX-162.md new file mode 100644 index 0000000..d6572c3 --- /dev/null +++ b/docs/specs/DEVX-162.md @@ -0,0 +1,41 @@ +# DEVX-162: Fix registry push race condition: serialize uploads + retry on HTTP 500 + +## Problem +The Gitea container registry (v1.27.2) has a known race condition in +`BlobUploader.Append()` where concurrent blob uploads cause the file +offset and DB model to get out of sync, producing HTTP 500 "offset +mismatch between file and model" errors. This causes the build-images +workflow to fail intermittently when pushing runner images. + +The `package_blob_upload` table accumulates stale entries from failed +uploads that worsen the problem over time. + +## Approach +Two fixes in devx (a third fix — scheduled cleanup — is tracked +separately as OBL-INFRA-537): + +1. Set `DOCKER_MAX_CONCURRENT_UPLOADS=1` in the build-images workflow + to serialize blob uploads and avoid the race condition. + +2. Add HTTP 500 retry logic to `push_image` in `build_image.py`. + When a push fails with HTTP 500 (not "already exists"), retry up + to 3 times with exponential backoff (5s, 10s, 20s). + +REQ-1: Build-images workflow sets DOCKER_MAX_CONCURRENT_UPLOADS=1 +REQ-2: push_image retries on HTTP 500 with exponential backoff +REQ-3: All existing tests pass with 100% coverage + +## Test Plan +- Unit tests for retry logic (mock subprocess) +- Manual: trigger build-images workflow and verify push succeeds + +## Deploy Plan +- Merge to master + +## Rollback Plan +- Revert the merge commit + +## Acceptance Criteria +- [x] REQ-1: Build-images workflow sets DOCKER_MAX_CONCURRENT_UPLOADS=1 +- [x] REQ-2: push_image retries on HTTP 500 with exponential backoff +- [x] REQ-3: All existing tests pass with 100% coverage diff --git a/src/devx/tools/build_image.py b/src/devx/tools/build_image.py index fad3e90..e1ec2a7 100644 --- a/src/devx/tools/build_image.py +++ b/src/devx/tools/build_image.py @@ -50,6 +50,7 @@ from dataclasses import dataclass, field from pathlib import Path import click +from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_exponential from devx.i18n import _ from devx.tokens import get_developer_token @@ -254,6 +255,29 @@ def delete_remote_manifest( return True +class PushHTTP500Error(Exception): + """Raised when docker push fails with an HTTP 500 from the registry.""" + + +def _run_push(cmd: list[str]) -> subprocess.CompletedProcess[str]: + """Run a docker push command, raising PushHTTP500Error on registry 500. + + The Gitea container registry (v1.27.x) has a race condition in + BlobUploader.Append that causes intermittent HTTP 500 "offset + mismatch" errors during concurrent blob uploads. Retrying the + push gives the registry time to recover. + """ + result = subprocess.run( # nosec B603 + cmd, + capture_output=True, + text=True, + check=False, + ) + if result.returncode != 0 and "500" in result.stderr: + raise PushHTTP500Error(result.stderr.strip()) + return result + + def push_image( spec: ImageSpec, registry: str, @@ -270,6 +294,9 @@ def push_image( with Gitea #31964 ("package version already exists") do we delete the old manifest and retry. This avoids losing the existing tag when the push fails for unrelated reasons (e.g. HTTP 500). + + HTTP 500 errors from the Gitea registry race condition are retried + up to 3 times with exponential backoff (5s, 10s) via tenacity. """ full_tags = [build_full_tag(registry, spec.name, t) for t in spec.tags] all_ok = True @@ -279,12 +306,26 @@ def push_image( click.echo(f"[dry-run] {' '.join(cmd)}") continue click.echo(f"Pushing {ft}...") - result = subprocess.run( # nosec B603 - cmd, - capture_output=True, - text=True, - check=False, + + @retry( + stop=stop_after_attempt(3), + wait=wait_exponential(multiplier=5, min=5, max=20), + retry=retry_if_exception_type(PushHTTP500Error), + reraise=True, ) + def _attempt(_cmd: list[str] = cmd) -> subprocess.CompletedProcess[str]: + return _run_push(_cmd) + + try: + result = _attempt() + except PushHTTP500Error as e: + click.echo( + _("Push failed for {tag}: {error}", tag=ft, error=str(e)), + err=True, + ) + all_ok = False + continue + if result.returncode == 0: click.echo(f"Pushed {ft}") continue diff --git a/tests/unit/test_build_image.py b/tests/unit/test_build_image.py index ba19b71..0efc263 100644 --- a/tests/unit/test_build_image.py +++ b/tests/unit/test_build_image.py @@ -13,6 +13,7 @@ from click.testing import CliRunner import devx.tools.build_image as build_image from devx.tools.build_image import ( ImageSpec, + PushHTTP500Error, build_full_tag, delete_remote_manifest, load_manifest, @@ -242,7 +243,7 @@ class TestPushImage: """Gitea #31964: push fails with 'already exists', delete + retry.""" spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) results = [ - MagicMock(returncode=1, stderr="500 Internal Server Error: already exists", stdout=""), + MagicMock(returncode=1, stderr="package version already exists", stdout=""), MagicMock(returncode=0, stderr="", stdout=""), ] with ( @@ -260,11 +261,9 @@ class TestPushImage: ) def test_no_delete_on_non_already_exists_failure(self) -> None: - """Push fails for other reasons (HTTP 500) — old manifest preserved.""" + """Push fails for other reasons (non-500) — old manifest preserved.""" spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) - mock_result = MagicMock( - returncode=1, stderr="received unexpected HTTP status: 500 Internal Server Error", stdout="" - ) + mock_result = MagicMock(returncode=1, stderr="denied: requested access to the resource is denied", stdout="") with ( patch("devx.tools.build_image.subprocess.run", return_value=mock_result), patch("devx.tools.build_image.delete_remote_manifest") as mock_del, @@ -276,7 +275,7 @@ class TestPushImage: """Gitea #31964 retry also fails — both pushes fail.""" spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) results = [ - MagicMock(returncode=1, stderr="500 Internal Server Error: already exists", stdout=""), + MagicMock(returncode=1, stderr="package version already exists", stdout=""), MagicMock(returncode=1, stderr="push failed again", stdout=""), ] with ( @@ -285,6 +284,62 @@ class TestPushImage: ): assert push_image(spec, "git.example.com", username="user", token="tok") is False + def test_http_500_retries_then_succeeds(self) -> None: + """HTTP 500 from registry race condition — retry succeeds.""" + spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) + results = [ + MagicMock(returncode=1, stderr="received unexpected HTTP status: 500 Internal Server Error", stdout=""), + MagicMock(returncode=0, stderr="", stdout=""), + ] + with ( + patch("devx.tools.build_image.subprocess.run", side_effect=results), + patch("devx.tools.build_image.delete_remote_manifest") as mock_del, + patch("time.sleep"), + ): + assert push_image(spec, "git.example.com", username="user", token="tok") is True + mock_del.assert_not_called() + + def test_http_500_retries_all_fail(self) -> None: + """HTTP 500 retries exhausted — push fails, no delete attempted.""" + spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"]) + mock_result = MagicMock( + returncode=1, stderr="received unexpected HTTP status: 500 Internal Server Error", stdout="" + ) + with ( + patch("devx.tools.build_image.subprocess.run", return_value=mock_result), + patch("devx.tools.build_image.delete_remote_manifest") as mock_del, + patch("time.sleep"), + ): + assert push_image(spec, "git.example.com", username="user", token="tok") is False + mock_del.assert_not_called() + + def test_run_push_raises_on_500(self) -> None: + """_run_push raises PushHTTP500Error when stderr contains 500.""" + from devx.tools.build_image import _run_push + + mock_result = MagicMock(returncode=1, stderr="HTTP 500 Internal Server Error", stdout="") + with patch("devx.tools.build_image.subprocess.run", return_value=mock_result): + with pytest.raises(PushHTTP500Error, match="HTTP 500"): + _run_push(["docker", "push", "img:latest"]) + + def test_run_push_no_raise_on_non_500(self) -> None: + """_run_push returns result when stderr has no 500.""" + from devx.tools.build_image import _run_push + + mock_result = MagicMock(returncode=1, stderr="denied: access denied", stdout="") + with patch("devx.tools.build_image.subprocess.run", return_value=mock_result): + result = _run_push(["docker", "push", "img:latest"]) + assert result.returncode == 1 + + def test_run_push_no_raise_on_success(self) -> None: + """_run_push returns result on success.""" + from devx.tools.build_image import _run_push + + mock_result = MagicMock(returncode=0, stderr="", stdout="") + with patch("devx.tools.build_image.subprocess.run", return_value=mock_result): + result = _run_push(["docker", "push", "img:latest"]) + assert result.returncode == 0 + class TestDeleteRemoteManifest: def test_dry_run(self) -> None: