fix: stop instance when base_configuration.setup() raises an error
Metadata
Current evaluation
Merged PR #996 (commit 78367c2) fixes LXD launcher leaving instances running when base_configuration.setup() raises, by wrapping setup/stop in try/finally. Closes #447 and #757; supersedes earlier #947. Adds regression tests and ruff noqa updates.
Resolution: closed resolved
Reason: The PR itself was merged into main as commit 78367c2, whose message reads 'fix: stop instance when base_configuration.setup() raises an error (#996)'. The body explicitly states 'Closes #447' and 'Closes #757', and issue #447 is recorded as fixed by this merged PR.
Evaluated by: mr-cal-bot
No scores available.
Issue body
## Summary
Replaces #947, rebased on `work/fix-lint-errors` (#995) and updated to fix an additional lint issue that surfaced since #947 was opened.
### Reproducer for #447 and #757
Adds two failing tests that demonstrate instances are left running when `base_configuration.setup()` raises an error.
### Root cause
In `_create_instance()` in `launcher.py`, `instance.stop()` / `base_instance.stop()` were only called when `setup()` succeeded. If `setup()` raised, the instance was left running.
### Fix
Wraps the `setup()` + `stop()` calls in `_create_instance()` in `try`/`finally` (base instance path) and `try`/`except`+`raise` (instance path) blocks so the instance is always stopped, even when setup fails.
### Additional fix since #947
Wrapping the setup logic in `try` blocks pushed `_create_instance()` over ruff's `PLR0915` (too-many-statements) limit. Added `PLR0915` to the existing `noqa` comment on the function signature, alongside the pre-existing `PLR0913` (too-many-arguments) suppression.
Closes #447
Closes #757
## Testing
- `make lint-ruff`, `make lint-mypy`, `make lint-pyright`, `make lint-codespell`, `make lint-prettier` — all pass
- `uv run pytest tests/unit/lxd/test_launcher.py` — 53 passed
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Evaluation history
| Date | Model | Scores | Action | Summary |
|---|---|---|---|---|
| stealth/space-bunny-alpha | — | closed resolved | Merged PR #996 (commit 78367c2) fixes LXD launcher leaving instances running when base_configuration.setup() raises, by wrapping setup/stop in try/finally. Closes #447 and #757; supersedes earlier #947. Adds regression tests and ruff noqa updates. | |
| stealth/space-bunny-alpha | — | closed resolved | Merged #996, superseding #947, to always stop instances when base_configuration.setup() fails; includes regression tests and lint fixes, closing #447 and #757. | |
| stealth/space-bunny-alpha | — | closed resolved | Merged in #996 at 78367c2, replacing #947 and rebased on #995. Ensured setup failures stop instances and timers without masking errors; added regression tests and fixed ephemeral cleanup. | |
| qwen/qwen3.6-35b-a3b | — | — | Merged fix preventing resource leaks by ensuring instances are stopped when base_configuration.setup() fails. Wrapped setup and cleanup in try/except/finally blocks. Closes #447 and #757, replaces #947. All CI and lint checks pass. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
35
Confidence:
90
|
needs review | Fixes instances being left running when setup fails by wrapping calls in try/finally blocks. Addresses review feedback, fixes lint and test cleanup issues. Approved by 2 reviewers, CI mostly passing with pending tests. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
40
Confidence:
85
|
needs review | Fixes instances being left running when setup() fails by wrapping calls in try/finally blocks. Addresses review feedback, fixes a test cleanup issue, and passes all CI checks. Currently approved by 2 reviewers with 4 unresolved comments. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
35
Confidence:
95
|
needs review | Fixes instances being left running when setup() fails by wrapping calls in try/finally blocks. Addresses review feedback, fixes a test cleanup issue, passes all CI checks, and has two approvals. Ready to merge. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
25
Confidence:
95
|
needs review | Fixes instances being left running when setup() fails by wrapping calls in try/finally and try/except blocks. Addresses review feedback, fixes lint issues, and resolves a test cleanup failure for ephemeral instances. All CI checks pass and review is approved. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
30
Confidence:
90
|
needs review | Fixes resource leak by ensuring instances stop when setup() fails. Addresses review feedback, fixes ephemeral instance test failure, and passes CI. Currently pending maintainer review with unresolved comments. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
35
Confidence:
90
|
needs review | Fixes instances being left running when setup() fails by wrapping calls in try/except/finally. Addresses review comments, fixes CI, and resolves lint issues. Currently pending maintainer review with unresolved comments. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
25
Confidence:
90
|
needs review | Fixes instances being left running when setup() fails by wrapping calls in try/finally and try/except. Addresses review comments, fixes a test cleanup issue, and passes CI. Currently awaiting maintainer review. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
20
Confidence:
90
|
needs review | Fixes instances being left running when base_configuration.setup() fails by wrapping setup in try/except blocks. Addresses review feedback, passes CI, and awaits maintainer review. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
15
Confidence:
85
|
needs review | Fixes instances remaining running when setup fails by wrapping calls in try/finally blocks. Replaces #947, adds regression tests, and resolves lint limits. Currently awaiting maintainer review with two unresolved comments and one failing CI check. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
15
Confidence:
90
|
needs review | Fixes instances left running when setup() fails by wrapping calls in try/finally blocks. Replaces #947, rebased on #995. Lints and unit tests pass. CI pending for slow tests. Awaiting maintainer review. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
5
Complexity:
15
Confidence:
85
|
needs review | Fixes instances left running when setup() fails by wrapping calls in try/finally blocks. Replaces #947, rebased on #995. Includes tests, passes CI, awaiting maintainer review. | |
| qwen/qwen3.6-35b-a3b |
Staleness:
0
Complexity:
10
Confidence:
90
|
needs review | Fixes instances remaining running after setup failures by wrapping calls in try/except blocks. Replaces #947, rebased, and suppresses a lint rule. Currently pending review with one failing CI check. |
Update history
| Date | Change |
|---|---|
| updated | |
| updated | |
| updated | |
| updated | |
| updated | |
| updated | |
| updated | |
| updated | |
| updated | |
| updated | |
| updated | |
| updated | |
| created |
Related work
-
Likely Fixed By:
canonical/craft-providers#447
(confidence 99%)
Issue 'Base instance does not stop when setup fails' was closed by this merged PR per the PR body ('Closes #447').
-
Superseded By:
canonical/craft-providers#947
(confidence 95%)
PR body states it 'Replaces #947', rebased on work/fix-lint-errors (#995) and extended with an extra lint fix; no commit references #947.
-
Likely Fixed By:
canonical/craft-providers#757
(confidence 90%)
Issue 'craft-providers leaves base lxd instance running on error' is the same root cause (previously closed with no code fix, tracked in CRAFT-4577); PR body states 'Closes #757'.
-
Related To:
canonical/craft-providers#995
(confidence 80%)
PR #996 was rebased on the lint-fixing branch from #995 (work/fix-lint-errors), which provided the existing noqa context for _create_instance().
Related issues
| Issue | Project | State | Summary | Similarity |
|---|---|---|---|---|
| #447 Base instance does not stop when setup fails | craft-providers | closed | Fixed by merged PR #996 (commit 78367c2), which stops the LXD base instance when base_configuration.setup() raises and adds regression tests. | |
| #178 tests: refactor lxd launcher tests | craft-providers | merged | Merged PR #178 in commit 43146e2 refactored LXD launcher tests to distinguish regular instances from base instances, enabling the test coverage needed by #177. | |
| #187 tests: improve lxd launcher tests for base instances | craft-providers | merged | Merged in b0df595: expanded LXD base-instance tests, mocked _is_valid(), added lxc.info() integration coverage, and clarified an error message. |