Public Access
DEVX-162: fix: serialize registry uploads and retry on HTTP 500
This commit was merged in pull request #304.
This commit is contained in:
@@ -77,6 +77,9 @@ jobs:
|
|||||||
CI_GITEA_API_TOKEN: ${{ secrets.CI_GITEA_API_TOKEN }}
|
CI_GITEA_API_TOKEN: ${{ secrets.CI_GITEA_API_TOKEN }}
|
||||||
CI_GITEA_USERNAME: ${{ vars.CI_GITEA_USERNAME }}
|
CI_GITEA_USERNAME: ${{ vars.CI_GITEA_USERNAME }}
|
||||||
PYTHONPATH: src
|
PYTHONPATH: src
|
||||||
|
# Serialize blob uploads to avoid Gitea registry race condition
|
||||||
|
# (BlobUploader.Append offset mismatch — see DEVX-162).
|
||||||
|
DOCKER_MAX_CONCURRENT_UPLOADS: "1"
|
||||||
run: |
|
run: |
|
||||||
. .venv/bin/activate
|
. .venv/bin/activate
|
||||||
export PATH="$HOME/.local/bin:$PATH"
|
export PATH="$HOME/.local/bin:$PATH"
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -50,6 +50,7 @@ from dataclasses import dataclass, field
|
|||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
import click
|
import click
|
||||||
|
from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_exponential
|
||||||
|
|
||||||
from devx.i18n import _
|
from devx.i18n import _
|
||||||
from devx.tokens import get_developer_token
|
from devx.tokens import get_developer_token
|
||||||
@@ -254,6 +255,29 @@ def delete_remote_manifest(
|
|||||||
return True
|
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(
|
def push_image(
|
||||||
spec: ImageSpec,
|
spec: ImageSpec,
|
||||||
registry: str,
|
registry: str,
|
||||||
@@ -270,6 +294,9 @@ def push_image(
|
|||||||
with Gitea #31964 ("package version already exists") do we delete
|
with Gitea #31964 ("package version already exists") do we delete
|
||||||
the old manifest and retry. This avoids losing the existing tag
|
the old manifest and retry. This avoids losing the existing tag
|
||||||
when the push fails for unrelated reasons (e.g. HTTP 500).
|
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]
|
full_tags = [build_full_tag(registry, spec.name, t) for t in spec.tags]
|
||||||
all_ok = True
|
all_ok = True
|
||||||
@@ -279,12 +306,26 @@ def push_image(
|
|||||||
click.echo(f"[dry-run] {' '.join(cmd)}")
|
click.echo(f"[dry-run] {' '.join(cmd)}")
|
||||||
continue
|
continue
|
||||||
click.echo(f"Pushing {ft}...")
|
click.echo(f"Pushing {ft}...")
|
||||||
result = subprocess.run( # nosec B603
|
|
||||||
cmd,
|
@retry(
|
||||||
capture_output=True,
|
stop=stop_after_attempt(3),
|
||||||
text=True,
|
wait=wait_exponential(multiplier=5, min=5, max=20),
|
||||||
check=False,
|
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:
|
if result.returncode == 0:
|
||||||
click.echo(f"Pushed {ft}")
|
click.echo(f"Pushed {ft}")
|
||||||
continue
|
continue
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ from click.testing import CliRunner
|
|||||||
import devx.tools.build_image as build_image
|
import devx.tools.build_image as build_image
|
||||||
from devx.tools.build_image import (
|
from devx.tools.build_image import (
|
||||||
ImageSpec,
|
ImageSpec,
|
||||||
|
PushHTTP500Error,
|
||||||
build_full_tag,
|
build_full_tag,
|
||||||
delete_remote_manifest,
|
delete_remote_manifest,
|
||||||
load_manifest,
|
load_manifest,
|
||||||
@@ -242,7 +243,7 @@ class TestPushImage:
|
|||||||
"""Gitea #31964: push fails with 'already exists', delete + retry."""
|
"""Gitea #31964: push fails with 'already exists', delete + retry."""
|
||||||
spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"])
|
spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"])
|
||||||
results = [
|
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=""),
|
MagicMock(returncode=0, stderr="", stdout=""),
|
||||||
]
|
]
|
||||||
with (
|
with (
|
||||||
@@ -260,11 +261,9 @@ class TestPushImage:
|
|||||||
)
|
)
|
||||||
|
|
||||||
def test_no_delete_on_non_already_exists_failure(self) -> None:
|
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"])
|
spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"])
|
||||||
mock_result = MagicMock(
|
mock_result = MagicMock(returncode=1, stderr="denied: requested access to the resource is denied", stdout="")
|
||||||
returncode=1, stderr="received unexpected HTTP status: 500 Internal Server Error", stdout=""
|
|
||||||
)
|
|
||||||
with (
|
with (
|
||||||
patch("devx.tools.build_image.subprocess.run", return_value=mock_result),
|
patch("devx.tools.build_image.subprocess.run", return_value=mock_result),
|
||||||
patch("devx.tools.build_image.delete_remote_manifest") as mock_del,
|
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."""
|
"""Gitea #31964 retry also fails — both pushes fail."""
|
||||||
spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"])
|
spec = ImageSpec(name="ci-base", dockerfile="Dockerfile", tags=["latest"])
|
||||||
results = [
|
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=""),
|
MagicMock(returncode=1, stderr="push failed again", stdout=""),
|
||||||
]
|
]
|
||||||
with (
|
with (
|
||||||
@@ -285,6 +284,62 @@ class TestPushImage:
|
|||||||
):
|
):
|
||||||
assert push_image(spec, "git.example.com", username="user", token="tok") is False
|
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:
|
class TestDeleteRemoteManifest:
|
||||||
def test_dry_run(self) -> None:
|
def test_dry_run(self) -> None:
|
||||||
|
|||||||
Reference in New Issue
Block a user