fix: ensure clean-up on container start failure and warn on removal failure (#2879)

* fix: ensure clean-up on container start failure and warn on removal failure

Properly release resources on container start failure.

If we fail to remove the created container, warn when not running in CI.

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

cibuildwheel/oci_container.py

@@ -206,7 +206,6 @@ class OCIContainer:
 
     UTILITY_PYTHON = "/opt/python/cp39-cp39/bin/python"
 
-    process: subprocess.Popen[bytes]
     bash_stdin: IO[bytes]
     bash_stdout: IO[bytes]
 
@@ -226,6 +225,7 @@ class OCIContainer:
         self.oci_platform = oci_platform
         self.cwd = cwd
         self.name: str | None = None
+        self.process: subprocess.Popen[bytes] | None = None
         self.engine = engine
         self.host_tar_format = ""
         if sys.platform.startswith("darwin"):
@@ -262,6 +262,7 @@ class OCIContainer:
         return f"--platform={oci_platform.value}", f"--pull={pull}"
 
     def __enter__(self) -> Self:
+        assert self.process is None
         self.name = f"cibuildwheel-{uuid.uuid4()}"
 
         _check_engine_version(self.engine)
@@ -319,32 +320,41 @@ class OCIContainer:
             check=True,
         )
 
-        self.process = subprocess.Popen(
-            [
-                self.engine.name,
-                "start",
-                "--attach",
-                "--interactive",
-                self.name,
-            ],
-            stdin=subprocess.PIPE,
-            stdout=subprocess.PIPE,
-        )
+        try:
+            self.process = subprocess.Popen(
+                [
+                    self.engine.name,
+                    "start",
+                    "--attach",
+                    "--interactive",
+                    self.name,
+                ],
+                stdin=subprocess.PIPE,
+                stdout=subprocess.PIPE,
+            )
 
-        assert self.process.stdin
-        assert self.process.stdout
-        self.bash_stdin = self.process.stdin
-        self.bash_stdout = self.process.stdout
+            assert self.process.stdin
+            assert self.process.stdout
+            self.bash_stdin = self.process.stdin
+            self.bash_stdout = self.process.stdout
 
-        # run a noop command to block until the container is responding
-        self.call(["/bin/true"], cwd="/")
-
-        if self.cwd:
-            # Although `docker create -w` does create the working dir if it
-            # does not exist, podman does not. There does not seem to be a way
-            # to setup a workdir for a container running in podman.
-            self.call(["mkdir", "-p", os.fspath(self.cwd)], cwd="/")
+            # run a noop command to block until the container is responding
+            self.call(["/bin/true"], cwd="/")
 
+            if self.cwd:
+                # Although `docker create -w` does create the working dir if it
+                # does not exist, podman does not. There does not seem to be a way
+                # to setup a workdir for a container running in podman.
+                self.call(["mkdir", "-p", os.fspath(self.cwd)], cwd="/")
+        except BaseException:
+            # clean-up
+            if self.process is not None:
+                if self.process.poll() is None:
+                    self.process.kill()
+                self.process.communicate()
+            self.process = None
+            self._remove_container()
+            raise
         return self
 
     def __exit__(
@@ -353,6 +363,7 @@ class OCIContainer:
         exc_val: BaseException | None,
         exc_tb: TracebackType | None,
     ) -> None:
+        assert self.process is not None
         self.bash_stdin.write(b"exit 0\n")
         self.bash_stdin.flush()
         self.process.wait(timeout=30)
@@ -366,16 +377,24 @@ class OCIContainer:
             # For now, this seems to work "well enough".
             self.process.wait()
 
-        assert isinstance(self.name, str)
+        self.process = None
 
         keep_container = strtobool(os.environ.get("CIBW_DEBUG_KEEP_CONTAINER", ""))
         if not keep_container:
-            subprocess.run(
-                [self.engine.name, "rm", "--force", "-v", self.name],
-                stdout=subprocess.DEVNULL,
-                check=False,
-            )
-            self.name = None
+            self._remove_container()
+
+    def _remove_container(self) -> None:
+        assert self.name is not None
+        result = subprocess.run(
+            [self.engine.name, "rm", "--force", "-v", self.name],
+            stdout=subprocess.DEVNULL,
+            check=False,
+        )
+        # only warn when not running in CI
+        if result.returncode != 0 and detect_ci_provider() is None:
+            msg = f"Failed to remove {self.name!r} container."
+            log.warning(msg)
+        self.name = None
 
     def copy_into(self, from_path: Path, to_path: PurePath) -> None:
         if from_path.is_dir():

unit_test/oci_container_test.py

@@ -584,6 +584,51 @@ def test_local_image(
         assert container._get_platform_args() == expected_platform_args
 
 
+def test_enter_error(container_engine: OCIContainerEngineConfig, tmp_path: Path) -> None:
+    remote_image = "debian:trixie-slim"
+    platform = DEFAULT_OCI_PLATFORM
+    local_image = f"cibw_{container_engine.name}_enter:latest"
+    dockerfile = tmp_path / "Dockerfile"
+    dockerfile.write_text(f"FROM {remote_image}\nRUN ln -sf false /bin/true")
+    subprocess.run(
+        [container_engine.name, "pull", f"--platform={platform.value}", remote_image],
+        check=True,
+    )
+    subprocess.run(
+        [container_engine.name, "build", f"--platform={platform.value}", "-t", local_image, "."],
+        check=True,
+        cwd=tmp_path,
+    )
+    container = OCIContainer(engine=container_engine, image=local_image, oci_platform=platform)
+    with pytest.raises(subprocess.CalledProcessError, match="/bin/true"), container:
+        pass
+    assert container.name is None
+    assert container.process is None
+
+
+@pytest.mark.parametrize("ci", [True, False])
+def test_enter_error_cleanup_failure(
+    ci: bool, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str]
+) -> None:
+    provider = cibuildwheel.ci.CIProvider.other if ci else None
+    monkeypatch.setattr(cibuildwheel.oci_container, "detect_ci_provider", lambda: provider)
+    result: subprocess.CompletedProcess[bytes] = subprocess.CompletedProcess(
+        "", returncode=1, stdout=None, stderr=None
+    )
+    monkeypatch.setattr(subprocess, "run", lambda *args, **kwargs: result)
+    engine = OCIContainerEngineConfig("docker")
+    container = OCIContainer(engine=engine, image="foo", oci_platform=OCIPlatform.AMD64)
+    container.name = "bar"
+    container._remove_container()
+    out, err = capsys.readouterr()
+    assert out == ""
+    if ci:
+        assert err == ""
+    else:
+        assert "warning" in err
+        assert "Failed to remove 'bar' container" in err
+
+
 @pytest.mark.parametrize("platform", list(OCIPlatform))
 def test_multiarch_image(container_engine: OCIContainerEngineConfig, platform: OCIPlatform) -> None:
     if detect_ci_provider() == CIProvider.travis_ci and DEFAULT_OCI_PLATFORM not in {