fix: check container runtime is available in stop and restart - #1036
Conversation
| } | ||
|
|
||
| pub fn logs(&self, instance_name: &str, follow: bool) -> Result<()> { | ||
| Self::check_available(self.runtime)?; |
There was a problem hiding this comment.
When helix logs runs with Docker installed but its daemon stopped, check_available starts Docker Desktop, Colima, or the Docker service and waits for startup. This makes a read-only logs command mutate runtime state instead of promptly reporting that the runtime is unavailable.
There was a problem hiding this comment.
Pull request overview
This PR aligns helix stop, helix restart, and helix logs with helix start by preflighting the configured container runtime so missing-runtime spawn failures are routed through the existing “not installed” error (including the Podman hint) instead of surfacing as a bare OS error.
Changes:
- Add
Self::check_available(self.runtime)?toLocalRuntime::stop(). - Add
Self::check_available(self.runtime)?toLocalRuntime::restart(). - Add
Self::check_available(self.runtime)?toLocalRuntime::logs().
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pub fn restart(&self, instance_name: &str, config: &LocalInstanceConfig) -> Result<()> { | ||
| Self::check_available(self.runtime)?; | ||
| if config.storage.is_disk() || config.storage.is_s3() { | ||
| return self.run_detached(instance_name, config); | ||
| } |
| } | ||
|
|
||
| pub fn logs(&self, instance_name: &str, follow: bool) -> Result<()> { | ||
| Self::check_available(self.runtime)?; |
292c32e to
aafc8dd
Compare
run_detached and run_foreground both call check_available before they touch the runtime, which routes a missing binary through not_installed_error and produces an actionable message. stop and restart spawned the binary directly, so on a host without the configured runtime they failed with a bare os error 2 that does not name what was missing. restart checks only on the branch that spawns the runtime itself; the disk and s3 paths return early into run_detached, which already checks.
aafc8dd to
cb749ed
Compare
|
both fair, and the second one caught an inconsistency in my own reasoning. fixed in cb749ed. on the redundant probe in restart: the check now sits after the disk and s3 early return, so it only guards the branch that spawns the runtime itself. those paths fall through to run_detached, which already checks, so there is no longer a double probe. on logs auto-starting the daemon: you are right and i was applying two different standards. i had already left that does leave |
helix startalready checks the container runtime is there before it does anything. if docker is configured but not installed, it stops with an error that names the missing binary and tells you podman is available instead. stop and restart skip that check and spawn the binary directly, so on the same machine they fail with a bare os error.on a host with podman and no docker, using the
container_runtime = "docker"thathelix initwrites by default:os error 2 never says which binary was missing, so there is nothing in it to act on. you get it on first run too, because init writes docker into helix.toml without looking at what is installed, so a podman user hits this before anything else works.
the fix is already in the file. run_detached and run_foreground both open with
Self::check_available(self.runtime)?, and check_available is what routes a spawn failure through not_installed_error to produce the podman hint above. this adds the same line to stop and restart so they land on the error that was already written for this case. no new error text and no new behaviour, two commands just stop bypassing it.in restart the check sits after the disk and s3 early return, not before it. those paths hand off to run_detached, which already checks, so checking first would probe the daemon twice.
stop and restart are the two commands here that go on to change container state, which is why auto-starting a stopped daemon is reasonable for them, the same as it already is for start.
logsandstatushave the same bare error but they are read only, andcheck_availablewill start docker desktop or colima when the binary is present but the daemon is down. starting a daemon as a side effect of reading logs is not something i wanted to slip into this pr. both would be better served by a preflight that checks the binary is installed without auto-starting, which is a different change. happy to send it if you want it.tested on wsl2, ubuntu 24.04 arm64, podman 4.9.3, docker not installed. after the change both print the same error as start, and with podman configured the normal lifecycle still works: start, status, query, restart, query again, logs, stop.
the typescript_runtime suite needs
npm ciinsdks/typescriptand node on PATH before it will pass; with those in place it is green here too.