mngr_lima: report limactl runtime failures as provider-unavailable instead of leaking ProcessError#2412
Merged
Merged
Conversation
…Error The limactl wrapper helpers (list, stop, delete, disk create, disk delete, show-ssh, shell, --version) called run_process_to_completion with the default is_checked_after=True, so a non-zero exit raised a raw ProcessError and the manual `if returncode != 0: raise LimaCommandError` below it was dead code. Because ProcessError is neither LimaCommandError nor OSError, it slipped past callers -- notably discover_hosts -- and surfaced as an opaque, unclassified discovery error (this is how a limactl startup crash, e.g. the macOS getpwuid "unknown userid" init panic, left a Lima workspace stuck offline with no clear reason). Two fixes: - Correctness: run those helpers with is_checked_after=False so they raise the intended LimaCommandError on non-zero exit (get_lima_version now checks the return code explicitly too). limactl_disk_delete's tolerate-absent-disk path is reachable again as a result. - Semantics: when limactl is installed and correctly versioned but an invocation fails at runtime, Lima discovery now raises LimaCommandUnavailableError (a ProviderUnavailableError), so the provider is reported unavailable with the standard message + retry/backoff, matching how other providers report an unreachable backend. A genuinely absent or too-old limactl still degrades gracefully to a local all-offline host view. Co-authored-by: Sculptor <sculptor@imbue.com>
Problem: instance_test.py and limactl_test.py each defined an identical _write_fake_limactl helper (duplicated test setup), and the shared PATH-prepend monkeypatch line was repeated three times. Repo convention puts shared non-fixture test helpers in testing.py. The duplicated helper's docstring also claimed it prepends the directory to PATH, which it did not. Fix: add install_fake_limactl to mngr_lima/testing.py that writes the executable fake limactl and prepends its directory to PATH (making the docstring accurate), import it in both test files, and drop the duplicated helpers and setenv lines.
The test-helper consolidation left `ProviderUnavailableError` imported but unused (the crash test asserts on the more specific `LimaCommandUnavailableError`, which is itself a `ProviderUnavailableError`), tripping the root `test_no_ruff_errors` meta-check (F401) in CI. Per-package `test-quick` and `ty` don't run that ruff gate, so it only surfaced on offload. Co-authored-by: Sculptor <sculptor@imbue.com>
…+ changelog
Address the architecture review's two cosmetic notes:
- LimaCommandUnavailableError previously inherited the base ProviderUnavailableError
help text ("Ensure the provider backend is running (e.g. start Docker)"), which
names the wrong tool for a Lima crash. It now supplies Lima-appropriate
user_help_text / short_remediation (retry; reinstall Lima if it persists).
- Changelog now notes the is_checked_after=False flip also repairs non-discovery
paths (limactl_shell's non-zero return; the stop/delete/cleanup LimaCommandError
catch sites), not just discovery.
Co-authored-by: Sculptor <sculptor@imbue.com>
Problem: the mngr_lima changelog's first sentence listed `shell` among the limactl wrapper helpers that now "surface a non-zero exit as the intended LimaCommandError", but limactl_shell returns its returncode to the caller and never raises LimaCommandError. This contradicted the same paragraph's later sentence noting shell "can once again return a non-zero exit code ... (rather than raising)". Fix: removed `shell` from the first sentence's helper list; shell's distinct return-non-zero behavior remains accurately described by the following sentence.
Comment on lines
232
to
234
| result = cg.run_process_to_completion(cmd, timeout=timeout, is_checked_after=False) | ||
| if result.returncode != 0: | ||
| raise LimaCommandError("stop", result.returncode, result.stderr) |
Contributor
There was a problem hiding this comment.
this seems weird and bad -- why not:
try:
result = cg.run_process_to_completion(cmd, timeout=timeout, is_checked_after=False)
except SomeTypeOfError as e:
raise LimaCommandError("stop", e.returncode, e.stderr, e.stdout) from e
Then all of the information gets preserved? And we then no longer can forget to check the exit code
It's weird to be disabling the check, then checking it?
| cmd = ["limactl", "shell", instance_name, "--", "sh", "-c", command] | ||
| result = cg.run_process_to_completion(cmd, timeout=timeout) | ||
| result = cg.run_process_to_completion(cmd, timeout=timeout, is_checked_after=False) | ||
| return result.returncode, result.stdout, result.stderr |
Contributor
There was a problem hiding this comment.
we don't check this one... what callers exist?
can they properly handle this raising for a non-zero exit?
it'd be nice to make it all consistent so that we cannot have silent failures
Contributor
|
the overall work here seems good, I just have the above stylistic complaints |
Contributor
|
lgtm after they are addressed |
# Conflicts: # libs/mngr_lima/imbue/mngr_lima/limactl_test.py
…actl_shell raises Addresses review feedback on the disable-check-then-recheck pattern: every limactl helper now runs through _run_limactl, which lets the ConcurrencyGroup's default checked run raise and translates the resulting ProcessError into LimaCommandError (preserving stdout too). This removes the per-call-site is_checked_after=False + manual return-code check, so a new helper cannot reintroduce the ProcessError leak. limactl_shell now raises on failure like the other helpers instead of silently returning a non-zero exit code that its cloud-init-wait caller ignored. Co-authored-by: Sculptor <sculptor@imbue.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
limactlis installed and correctly versioned but an invocation fails at runtime (e.g. it crashes at startup), Lima discovery surfaced an opaque, unclassified error and left the workspace stuck offline. This is what a real user hit: on their Mac,limactl list --jsonpanicked at package init (panic: user: unknown userid 501— a transient macOS Directory Services /getpwuidfault), so every Lima discovery poll failed and their local workspace was unreachable with no actionable reason.Root cause, in
mngr_lima:limactl_*wrapper helpers calledcg.run_process_to_completion(...)with the defaultis_checked_after=True, which raises a rawProcessErroron a non-zero exit — making theif result.returncode != 0: raise LimaCommandError(...)line below each call dead code.ProcessError(aConcurrencyGroupError) is neitherLimaCommandError,OSError, norProviderUnavailableError, so it slipped pastdiscover_hosts's handlers and the intended graceful degradation, bubbling up as a generic discovery error.LimaNotInstalledError) or too-old (LimaVersionError)limactl— a binary that is present but crashes at runtime was unmodeled.Fix
Correctness: the
limactl_*helpers (list,stop,delete,disk create,disk delete,show-ssh,shell, andget_lima_version) now passis_checked_after=False, so a non-zero exit raises the intended domainLimaCommandErrorinstead of a rawProcessError. Side benefits:limactl_disk_delete's tolerate-absent-disk path andlimactl_shell's documented non-zero return (inspected by its sole caller_wait_for_cloud_init) work as intended again.Semantics: new
LimaCommandUnavailableError(ProviderUnavailableError). Indiscover_hosts, a present-but-crashinglimactl'sLimaCommandError/OSErroris converted to that error and propagated, so the provider is reported unavailable (Provider 'lima' is not available: ...) with the standard retry/backoff — matching how other providers report an unreachable backend — instead of the old opaque error. A genuinely absent or too-oldlimactlstill degrades gracefully to a local all-offline host view.Tests
limactl_test.py:limactl_listraisesLimaCommandError(notProcessError) against a crashing fakelimactl— the regression guard for theis_checked_after=Falsefix.instance_test.py:discover_hostsraisesLimaCommandUnavailableError/ProviderUnavailableErrorwhen a present, correctly-versionedlimactlcrashes onlist; and still degrades to[]for a too-oldlimactl. Both use a sharedinstall_fake_limactlhelper (intesting.py) that writes a fakelimactland prepends its dir to PATH.mngr_limapackage: 134 passed, 0 failed (just test-quick);uv run ty checkclean; ratchets clean. The full monorepo suite runs in CI.Notes
limactltest helper intotesting.py(from the autofix gate).mngr_limahalf. The complementaryminds-side improvement — detect thelimactlpanic signature, retry discovery a few times before flipping tohost_offline, and show an actionable message instead of a bare "offline" — is a separate follow-up, and upstream Lima's fail-hardMust(user.Current())package init is the deeper cause.