From b4e41ee0c53e409c4cb99affcce45d606e1962d4 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 10 Jun 2026 01:19:37 +0200 Subject: [PATCH 1/2] Aggregate lockfile regeneration failures in bump regenerate_lockfiles aborted on the first cargo failure, leaving the workspace inconsistent with no inventory of which lockfiles were refreshed: manifests carry the new version while later lockfiles were never attempted. Attempt every manifest, collect the failures, and raise one aggregated LockfileRegenerationError after the loop. The message states that the workspace is inconsistent and lists each failed manifest with its repair command (cargo update --workspace --manifest-path ...). The exception chains from the first underlying failure so diagnostics such as a missing cargo executable survive aggregation. Manifest-path validation errors still raise immediately, as those are configuration mistakes rather than partial-update hazards. Tests cover the mid-list failure (remaining manifests still attempted; successes excluded from the repair list), multi-failure aggregation with exactly one repair command per failed manifest, and a syrupy snapshot of the aggregated message. Closes #84 --- lading/commands/bump_lockfiles.py | 52 +++++++++-- .../__snapshots__/test_bump_lockfiles.ambr | 10 +++ tests/unit/test_bump_lockfiles.py | 87 ++++++++++++++++++- 3 files changed, 141 insertions(+), 8 deletions(-) create mode 100644 tests/unit/__snapshots__/test_bump_lockfiles.ambr diff --git a/lading/commands/bump_lockfiles.py b/lading/commands/bump_lockfiles.py index 70e07c8f..e3419c01 100644 --- a/lading/commands/bump_lockfiles.py +++ b/lading/commands/bump_lockfiles.py @@ -9,6 +9,7 @@ import collections.abc as cabc import logging +import shlex import time from pathlib import Path @@ -60,31 +61,51 @@ def regenerate_lockfiles( ------ LockfileRegenerationError If any configured manifest path is invalid (outside the workspace or - not named ``Cargo.toml``), or if ``cargo update --workspace`` exits - non-zero for any manifest. + not named ``Cargo.toml``), or — after every manifest has been + attempted — if ``cargo update --workspace`` failed for one or more + manifests. The aggregated message lists each failed manifest with a + repair command. Notes ----- - **Partial-update semantics:** regeneration is not atomic. If ``cargo`` - fails for the *n*-th manifest, the first *n - 1* lockfiles have already - been updated on disk and will not be rolled back. Callers that require - atomic guarantees must implement their own rollback (see issue ``#84``). + **Partial-update semantics:** regeneration is not atomic and successful + updates are not rolled back when a later manifest fails. Every manifest + is attempted (issue #84), so a single cargo failure does not leave + unrelated lockfiles silently stale; the aggregated error tells the + operator exactly which lockfiles still need repair and how. """ command_runner = subprocess_runner if runner is None else runner manifests = _resolve_manifest_paths(workspace_root, lockfile_manifests) started_at = time.perf_counter() _LOGGER.info("Regenerating %d Cargo lockfile(s)", len(manifests)) lockfiles: list[Path] = [] + failures: list[tuple[Path, LockfileRegenerationError]] = [] for manifest in manifests: manifest_started_at = time.perf_counter() _LOGGER.info("Regenerating Cargo lockfile for %s", manifest) - _run_workspace_lockfile_update(workspace_root, manifest, command_runner) + try: + _run_workspace_lockfile_update(workspace_root, manifest, command_runner) + except LockfileRegenerationError as exc: + # Keep going: attempting the remaining manifests gives the + # operator one aggregated repair list instead of a re-run per + # failure (issue #84). + _LOGGER.exception("Cargo lockfile regeneration failed") + failures.append((manifest, exc)) + continue _LOGGER.info( "Regenerated Cargo lockfile for %s in %.3fs", manifest, time.perf_counter() - manifest_started_at, ) lockfiles.append(manifest.parent / "Cargo.lock") + if failures: + # Chain from the first underlying failure so diagnostics (for + # example a missing cargo executable) survive the aggregation. + primary = failures[0][1] + cause = primary.__cause__ if primary.__cause__ is not None else primary + raise LockfileRegenerationError( + _build_aggregate_failure_message(failures) + ) from cause _LOGGER.info( "Regenerated %d Cargo lockfile(s) in %.3fs", len(lockfiles), @@ -93,6 +114,23 @@ def regenerate_lockfiles( return tuple(lockfiles) +def _build_aggregate_failure_message( + failures: cabc.Sequence[tuple[Path, LockfileRegenerationError]], +) -> str: + """Return the aggregated operator-facing regeneration failure message.""" + header = ( + f"Cargo lockfile regeneration failed for {len(failures)} manifest(s). " + "Manifests already carry the new version, so the workspace is " + "inconsistent until each lockfile below is repaired:" + ) + lines = [header] + for manifest, error in failures: + lines.append(f"- {error}") + quoted_manifest = shlex.quote(str(manifest)) + lines.append(f" cargo update --workspace --manifest-path {quoted_manifest}") + return "\n".join(lines) + + def _resolve_manifest_paths( workspace_root: Path, lockfile_manifests: cabc.Sequence[str], diff --git a/tests/unit/__snapshots__/test_bump_lockfiles.ambr b/tests/unit/__snapshots__/test_bump_lockfiles.ambr new file mode 100644 index 00000000..3b9f4960 --- /dev/null +++ b/tests/unit/__snapshots__/test_bump_lockfiles.ambr @@ -0,0 +1,10 @@ +# serializer version: 1 +# name: test_regenerate_lockfiles_aggregates_multiple_failures + ''' + Cargo lockfile regeneration failed for 2 manifest(s). Manifests already carry the new version, so the workspace is inconsistent until each lockfile below is repaired: + - Cargo lockfile regeneration failed for /a/Cargo.toml with exit code 101: error: dependency conflict + cargo update --workspace --manifest-path /a/Cargo.toml + - Cargo lockfile regeneration failed for /b/Cargo.toml with exit code 101: error: dependency conflict + cargo update --workspace --manifest-path /b/Cargo.toml + ''' +# --- diff --git a/tests/unit/test_bump_lockfiles.py b/tests/unit/test_bump_lockfiles.py index 4345b043..9c253e09 100644 --- a/tests/unit/test_bump_lockfiles.py +++ b/tests/unit/test_bump_lockfiles.py @@ -6,13 +6,14 @@ import dataclasses as dc import pathlib import typing as typ +from pathlib import Path import pytest from lading.commands import bump_lockfiles if typ.TYPE_CHECKING: - from pathlib import Path + from syrupy.assertion import SnapshotAssertion @dc.dataclass(frozen=True, slots=True) @@ -267,3 +268,87 @@ def failing_runner( ) assert isinstance(exc_info.value.__cause__, OSError) + + +# --------------------------------------------------------------------------- +# Aggregated failure handling (issue #84) +# --------------------------------------------------------------------------- + + +def _selective_failure_runner( + failing_manifests: set[Path], +) -> tuple[cabc.Callable[..., tuple[int, str, str]], list[Path]]: + """Return a runner failing for ``failing_manifests`` and its call log.""" + attempted: list[Path] = [] + + def runner( + command: cabc.Sequence[str], + *, + cwd: Path | None = None, + ) -> tuple[int, str, str]: + del cwd + manifest = Path(command[-1]) + attempted.append(manifest) + if manifest in failing_manifests: + return 101, "", "error: dependency conflict" + return 0, "", "" + + return runner, attempted + + +def test_regenerate_lockfiles_attempts_all_manifests_after_failure( + tmp_path: Path, +) -> None: + """A mid-list cargo failure does not skip the remaining manifests.""" + for name in ("a", "b"): + (tmp_path / name).mkdir() + (tmp_path / name / "Cargo.toml").write_text("", encoding="utf-8") + (tmp_path / "Cargo.toml").write_text("", encoding="utf-8") + failing = (tmp_path / "a" / "Cargo.toml").resolve() + runner, attempted = _selective_failure_runner({failing}) + + with pytest.raises(bump_lockfiles.LockfileRegenerationError) as excinfo: + bump_lockfiles.regenerate_lockfiles( + tmp_path, + ("a/Cargo.toml", "b/Cargo.toml"), + runner=runner, + ) + + assert attempted == [ + (tmp_path / "Cargo.toml").resolve(), + failing, + (tmp_path / "b" / "Cargo.toml").resolve(), + ] + message = str(excinfo.value) + assert "failed for 1 manifest(s)" in message + assert str(failing) in message + assert f"cargo update --workspace --manifest-path {failing}" in message + assert str((tmp_path / "b" / "Cargo.toml").resolve()) not in message + + +def test_regenerate_lockfiles_aggregates_multiple_failures( + tmp_path: Path, snapshot: SnapshotAssertion +) -> None: + """Every failed manifest is listed once with its repair command.""" + for name in ("a", "b"): + (tmp_path / name).mkdir() + (tmp_path / name / "Cargo.toml").write_text("", encoding="utf-8") + (tmp_path / "Cargo.toml").write_text("", encoding="utf-8") + failing = { + (tmp_path / "a" / "Cargo.toml").resolve(), + (tmp_path / "b" / "Cargo.toml").resolve(), + } + runner, _ = _selective_failure_runner(failing) + + with pytest.raises(bump_lockfiles.LockfileRegenerationError) as excinfo: + bump_lockfiles.regenerate_lockfiles( + tmp_path, + ("a/Cargo.toml", "b/Cargo.toml"), + runner=runner, + ) + + message = str(excinfo.value) + assert "failed for 2 manifest(s)" in message + for manifest in failing: + assert message.count(f"--manifest-path {manifest}") == 1 + assert snapshot == message.replace(str(tmp_path), "") From 99cfa75b7bfcc839ac1bfd8ecf1c6e88d9803fdb Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Tue, 16 Jun 2026 19:07:38 +0000 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=93=9D=20CodeRabbit=20Chat:=20Impleme?= =?UTF-8?q?nt=20requested=20code=20changes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lading/commands/bump_lockfiles.py | 52 +++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/lading/commands/bump_lockfiles.py b/lading/commands/bump_lockfiles.py index e3419c01..fcc095ad 100644 --- a/lading/commands/bump_lockfiles.py +++ b/lading/commands/bump_lockfiles.py @@ -78,13 +78,31 @@ def regenerate_lockfiles( manifests = _resolve_manifest_paths(workspace_root, lockfile_manifests) started_at = time.perf_counter() _LOGGER.info("Regenerating %d Cargo lockfile(s)", len(manifests)) + lockfiles, failures = _collect_lockfile_results( + workspace_root, manifests, command_runner + ) + _raise_if_failures(failures) + _LOGGER.info( + "Regenerated %d Cargo lockfile(s) in %.3fs", + len(lockfiles), + time.perf_counter() - started_at, + ) + return tuple(lockfiles) + + +def _collect_lockfile_results( + workspace_root: Path, + manifests: cabc.Sequence[Path], + runner: CommandRunner, +) -> tuple[list[Path], list[tuple[Path, LockfileRegenerationError]]]: + """Attempt every manifest and return successful lockfiles and failures.""" lockfiles: list[Path] = [] failures: list[tuple[Path, LockfileRegenerationError]] = [] for manifest in manifests: manifest_started_at = time.perf_counter() _LOGGER.info("Regenerating Cargo lockfile for %s", manifest) try: - _run_workspace_lockfile_update(workspace_root, manifest, command_runner) + _run_workspace_lockfile_update(workspace_root, manifest, runner) except LockfileRegenerationError as exc: # Keep going: attempting the remaining manifests gives the # operator one aggregated repair list instead of a re-run per @@ -98,24 +116,26 @@ def regenerate_lockfiles( time.perf_counter() - manifest_started_at, ) lockfiles.append(manifest.parent / "Cargo.lock") - if failures: - # Chain from the first underlying failure so diagnostics (for - # example a missing cargo executable) survive the aggregation. - primary = failures[0][1] - cause = primary.__cause__ if primary.__cause__ is not None else primary - raise LockfileRegenerationError( - _build_aggregate_failure_message(failures) - ) from cause - _LOGGER.info( - "Regenerated %d Cargo lockfile(s) in %.3fs", - len(lockfiles), - time.perf_counter() - started_at, - ) - return tuple(lockfiles) + return lockfiles, failures + + +def _raise_if_failures( + failures: list[tuple[Path, LockfileRegenerationError]], +) -> None: + """Raise an aggregated error when one or more manifest updates failed.""" + if not failures: + return + # Chain from the first underlying failure so diagnostics (for + # example a missing cargo executable) survive the aggregation. + primary = failures[0][1] + cause = primary.__cause__ if primary.__cause__ is not None else primary + raise LockfileRegenerationError( + _build_aggregate_failure_message(failures) + ) from cause def _build_aggregate_failure_message( - failures: cabc.Sequence[tuple[Path, LockfileRegenerationError]], + ) -> str: """Return the aggregated operator-facing regeneration failure message.""" header = (