Do not read an unreachable Docker daemon as an absent container #58

Merged

jknapp merged 3 commits from fix/56-staleness-probe-daemon-errors into main 2026-09-19 02:59:20 +00:00

Conversation 0 Commits 3 Files Changed 1

+576 -49

3 Commits

Author	SHA1	Message	Date
shadowdaoandClaude Opus 5	a3840f7263	fix: say which check failed, and stop claiming an order we do not use ... Two accuracy defects from re-review, both the same class as the bug this branch exists to fix. `probe_failed` rendered every failure as "This project's container could not be inspected", but only two of the four readings are about the container -- the others are the base image and the snapshot. A malformed base image name in settings therefore pointed the user at the wrong object. The sentence now names the check rather than the container. The doc claimed "the first error wins, in call order". It does not: the checks run container_id, base_image_id, container_running, while the daemon is called in a different order entirely. The priority is deliberate -- it puts the reading that stopped the probe first -- so the comment now says that, instead of describing an order the code does not use. The test guarding the first point asserted the message does not contain "Docker", using a synthetic payload. The real bollard error for that case is "Docker responded with status code 400: invalid reference format", so the assertion passed only because the payload was invented. It now uses the real shape and asserts what actually matters: that nothing we add claims the daemon was unreachable or names the container. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-18 19:53:52 -07:00
shadowdaoandClaude Opus 5	f662ed04ce	fix: a reading nobody consults must not destroy the report ... Review of this branch found the first cut made every probe error fatal, including one that is usually irrelevant. `snapshot_exists` is consulted only when there is no container, or when a stopped container coincides with a busy project -- `pick_probe_source` discards it outright for a running one. So a daemon hiccup between the four sequential readings turned a full report into a bare "could not be checked" with Update disabled, in a change whose whole purpose is handling exactly that hiccup better. It is now carried as a `Result` to the points that consult it and surfaced only there. `stopped_probe_policy` carries its own message, because "try again once it finishes" claims waiting is the only obstacle, which a failed `image_exists` has not established. `base_image_id` stays fatal, deliberately: it is the right-hand side of the comparison, and `image_id` already distinguishes "not pulled locally" (`Ok(None)`, a legitimate not-stale) from "could not ask". Letting an `Err` through as `None` would report a project up to date on a reading nobody got -- #56 one field over. The message no longer blames the daemon. Three of the four callees can `Err` from a daemon that answered perfectly: `image_id` maps only 404 to `Ok(None)`, and the base image name is user-supplied, so a malformed reference told the user to go fix a daemon that was running fine. That is the same category of error as #56 itself. `ContainerState` makes "running is known but no container was found" unrepresentable rather than merely unreached, so the downstream match has no impossible arm and the invariant is enforced where it is established. Finally, the tests covered the new function but not the line the bug was on: a partial revert to `.unwrap_or(None)` kept them all green. The readings now travel as a named struct of `Result`s, so that revert is a compile error -- verified by performing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-18 19:38:31 -07:00
shadowdaoandClaude Opus 5	84a5757c74	fix: do not read an unreachable Docker daemon as an absent container (#56) ... `get_container_staleness` collected four probes through `unwrap_or`, so a transient daemon fault landed on the same arm as a genuine absence and the banner said, confidently and wrongly, that the project has no container or snapshot image to compare against. The four readings are now taken as `Result`s and funnelled through a pure `collect_probe_inputs`, following `pick_probe_source` and `stopped_probe_policy` in the same file, so the rule is unit-testable without touching Docker. The first error in call order wins and becomes `probe_error`; the command still returns `Ok`, because the hook's `catch` sets `staleness` to null and the banner returns early on null -- an `Err` here would hide the fault instead of reporting it. One of the issue's premises did not hold. `is_container_running` does not distinguish absent from unreachable: its body flattens every `inspect_container` failure to `Ok(false)`, so only a `get_docker` failure can surface as `Err`. Its `Result` is threaded through anyway, since that one case is a real daemon-unreachable signal and this layer no longer adds a second swallow on top, and the remaining gap is documented where the decision is made rather than patched in `docker/container.rs`, which the issue puts out of scope and whose doc comment says the swallow is deliberate. In practice `find_existing_container` runs immediately before and would already have errored if the daemon were down. No frontend change: `probeUnavailable` in ContainerMigrationBanner already routes a set `probe_error` to "Some checks did not complete". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-09-18 06:04:41 -07:00