diff --git a/docs/specs/DEVX-163.md b/docs/specs/DEVX-163.md new file mode 100644 index 0000000..234a346 --- /dev/null +++ b/docs/specs/DEVX-163.md @@ -0,0 +1,33 @@ +# DEVX-163: Fix _run_push to check stdout for HTTP 500 + +## Problem +`_run_push` only checked `result.stderr` for HTTP 500, but docker push +sends the "received unexpected HTTP status: 500 Internal Server Error" +message to **stdout**, not stderr. This means the tenacity retry logic +added in DEVX-162 never triggered — the push failed immediately without +retrying. + +## Approach +Check both `result.stdout` and `result.stderr` for the "500" status code. +Also update the "already exists" check in `push_image` to check both +streams, since docker may send that message to stdout as well. + +REQ-1: _run_push checks both stdout and stderr for HTTP 500 +REQ-2: push_image "already exists" check uses combined stdout+stderr +REQ-3: All existing tests pass with 100% coverage + +## Test Plan +- Unit tests for stdout 500 detection +- Unit tests for stderr 500 detection +- Manual: trigger build-images workflow and verify retry works + +## Deploy Plan +- Merge to master + +## Rollback Plan +- Revert the merge commit + +## Acceptance Criteria +- [x] REQ-1: _run_push checks both stdout and stderr for HTTP 500 +- [x] REQ-2: push_image "already exists" check uses combined stdout+stderr +- [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 e1ec2a7..d04ec46 100644 --- a/src/devx/tools/build_image.py +++ b/src/devx/tools/build_image.py @@ -266,6 +266,9 @@ def _run_push(cmd: list[str]) -> subprocess.CompletedProcess[str]: BlobUploader.Append that causes intermittent HTTP 500 "offset mismatch" errors during concurrent blob uploads. Retrying the push gives the registry time to recover. + + Docker sends push progress/errors to both stdout and stderr depending + on the error type, so both streams are checked for the 500 status. """ result = subprocess.run( # nosec B603 cmd, @@ -273,8 +276,10 @@ def _run_push(cmd: list[str]) -> subprocess.CompletedProcess[str]: text=True, check=False, ) - if result.returncode != 0 and "500" in result.stderr: - raise PushHTTP500Error(result.stderr.strip()) + if result.returncode != 0: + combined = f"{result.stderr}\n{result.stdout}" + if "500" in combined: + raise PushHTTP500Error(combined.strip()) return result @@ -329,10 +334,10 @@ def push_image( if result.returncode == 0: click.echo(f"Pushed {ft}") continue - stderr = result.stderr.strip() + combined_output = f"{result.stderr}\n{result.stdout}".strip() # Gitea #31964: push fails because tag already exists. # Delete the old manifest and retry once. - if username and token and "already exists" in stderr.lower(): + if username and token and "already exists" in combined_output.lower(): click.echo(" Tag exists (Gitea #31964), deleting old manifest and retrying...") delete_remote_manifest( registry, @@ -352,9 +357,9 @@ def push_image( if result.returncode == 0: click.echo(f"Pushed {ft} (after retry)") continue - stderr = result.stderr.strip() + combined_output = f"{result.stderr}\n{result.stdout}".strip() click.echo( - _("Push failed for {tag}: {error}", tag=ft, error=stderr), + _("Push failed for {tag}: {error}", tag=ft, error=combined_output), err=True, ) all_ok = False diff --git a/tests/unit/test_build_image.py b/tests/unit/test_build_image.py index 0efc263..91ff082 100644 --- a/tests/unit/test_build_image.py +++ b/tests/unit/test_build_image.py @@ -288,7 +288,7 @@ class TestPushImage: """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=1, stderr="", stdout="received unexpected HTTP status: 500 Internal Server Error"), MagicMock(returncode=0, stderr="", stdout=""), ] with ( @@ -303,7 +303,7 @@ class TestPushImage: """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="" + returncode=1, stderr="", stdout="received unexpected HTTP status: 500 Internal Server Error" ) with ( patch("devx.tools.build_image.subprocess.run", return_value=mock_result), @@ -313,7 +313,7 @@ class TestPushImage: 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: + def test_run_push_raises_on_500_stderr(self) -> None: """_run_push raises PushHTTP500Error when stderr contains 500.""" from devx.tools.build_image import _run_push @@ -322,6 +322,17 @@ class TestPushImage: with pytest.raises(PushHTTP500Error, match="HTTP 500"): _run_push(["docker", "push", "img:latest"]) + def test_run_push_raises_on_500_stdout(self) -> None: + """_run_push raises PushHTTP500Error when stdout contains 500 (docker sends to stdout).""" + from devx.tools.build_image import _run_push + + mock_result = MagicMock( + returncode=1, stderr="", stdout="received unexpected HTTP status: 500 Internal Server Error" + ) + with patch("devx.tools.build_image.subprocess.run", return_value=mock_result): + with pytest.raises(PushHTTP500Error, match="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