fix(testreport): report the patch's own exit status from the update templates - #446
fix(testreport): report the patch's own exit status from the update templates#446plusky wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #446 +/- ##
==========================================
+ Coverage 96.28% 96.29% +0.01%
==========================================
Files 193 193
Lines 42939 43277 +338
==========================================
+ Hits 41343 41673 +330
- Misses 1596 1604 +8 ☔ View full report in Codecov by Harness. |
mimi1vx
left a comment
mimi1vx
left a comment
There was a problem hiding this comment.
Reviewed the full files (not just the diff) and ran the local gate — cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings, RUSTDOCFLAGS="-D warnings" cargo doc ... and cargo test -p mtui-testreport are all green.
The core template fix is correct: mtui_status=$? sits immediately after the patch invocation, set -- $mtui_patches + [ "$#" -gt 0 ] is the right emptiness test (field splitting rather than -n), the $$-escaping renders correctly under SubstMode::Safe, and the trailing exit is safe because Connection::run does a bare channel.exec with nothing wrapped or appended. run_checks reads lastexit() before reboot_transactional, and add_op_history uses sftp_append, so the snapshot the check inspects really is the patch script's own.
Requesting changes on one point below.
Blocking
checks/update.rs:160-173 — PackageNotFound short-circuiting markers degrades the exact failure this PR exists to surface
With the new short-circuit, on the zypper update check:
- exit
8(ZYPPER_EXIT_ERR_COMMIT— an rpm transaction failure, the most likely status for a genuinely failed patch) withError:on stderr now reports "package not found" instead of "RPM Error"; - exit
5(ERR_PRIVILEGES) carryingSystem management is lockednow reports "package not found" instead of "update stack locked".
Both are the headline diagnoses for the two most common real-world patch failures, and both are now replaced by a message that is actively misleading — the package was found, the transaction failed.
package_not_found_outranks_the_markers pins this deliberately, but the stated justification (the reason strings are a frozen contract shared with the install check) does not hold on the update path:
- Nothing branches on the string.
rg '"package not found"'matches only the check modules and their own tests — it is user-facing text, not a machine-readable contract. - On
update, exits4/5/8previously produced no verdict at all: they fell through tomarkersand passed. There is no prior behaviour to stay compatible with, so no consumer can be relying on8 -> "package not found"from an update.
A minimal fix that preserves 104's short-circuit exactly and gives the other three a real diagnosis:
ExitClass::PackageNotFound if args.exitcode == 104 => { log_failed(args); Err(...) }
ExitClass::PackageNotFound => {
markers(args)?;
log_failed(args);
Err(UpdateError::new("package not found", ...))
}The install check can keep its inline ordering unchanged.
Non-blocking
actions/update.rs:83-89 — a failed zypper -n refresh now yields a clean exit 0 with nothing installed
Scenario: the issue repo is unreachable or its GPG key is untrusted -> refresh fails -> zypper -n patches returns no matching row -> the guard skips the patch -> mtui_status stays 0 -> the update check passes.
The verdict is unchanged from before this PR (the old zypper in with no operands exited 3, which was masked, and "Required argument missing." carries no marker), so it is not a regression. But it is now the only path that reports success without patching, on a check this PR advertises as exit-code-faithful. The PR body files it as a residual — noting it here so the deferral is explicit in review as well.
checks/update.rs:122-125 — slmicro: any non-zero transactional-update status now triggers the group-wide rollback
transactional-update flattens everything to 1, including failures that are not the patch's (an open transaction, snapshot creation/deletion problems). Those are legitimate failures and the CHANGELOG documents the change, but this key went from "never fails on exit code" to "fails on any non-zero", and a check failure downgrades and reboots every host in the group. Worth a deliberate ack rather than an implicit one.
checks/mod.rs:913-919 — classify_exit(-1) returning Unknown is a footgun the doc comment can only warn about
The doc says -1 "is not classified here", but the function does classify it — as Unknown, i.e. "Unknown Error" -> UpdateFailure::Check -> a group-wide rollback on a host mtui never reached. That is precisely what UpdateFailure::NotRun exists to prevent, and it only holds because both current callers run not_run first. An ExitClass::NotRun variant would turn the omission into a compile error at the next call site instead of a comment someone has to read.
CHANGELOG.md:16-18 — reflow
"...and on the patch's exit code. RHEL/YUM is judged only\n on whether the command ran" leaves a short orphan line mid-sentence.
Note on the tests
The rendered_script module is the right call here — MockConnection genuinely cannot express "the patch failed but the last line succeeded". The stub-invocation sentinels and the_stub_rows_match_under_this_hosts_awk close the two ways those tests could have been vacuous, and the awk-dialect guard passes on macOS.
…s label (openSUSE#400) Review of openSUSE#446 caught that short-circuiting the whole `104 | 4 | 5 | 8` class to "package not found" degrades the two failures the check exists to surface. Only `104` (`ZYPPER_EXIT_INF_CAP_NOT_FOUND`) means the capability was not found; `4`, `5` and `8` are `ERR_ZYPP`, `ERR_PRIVILEGES` and `ERR_COMMIT` — the package was found and the transaction failed. An `8` with `Error:` on stderr is the likeliest status for a genuinely failed patch, and a `5` with `System management is locked` for a busy update stack, so the label replaced both headline diagnoses with one that is not vaguer but wrong. So only `104` now outranks the markers. The other three consult them first and keep "package not found" solely on a clean transcript. The justification for the old ordering does not survive scrutiny: nothing branches on these strings — they are rendered, logged and asserted on, never matched — and on `update` the three previously reached no verdict at all, falling through to the markers, so there was no earlier behaviour for the short-circuit to preserve. The class itself stays the install check's, unsplit, so an exit code still cannot land in two classes; what a check *says* about a member is its own to choose. The install check keeps its ordering, so the same transcript can now read differently on the two. That divergence is pinned on both sides rather than left to be discovered: install's own short-circuit was only ever exercised with an empty transcript, so hoisting its markers above the set broke no test. `-1` gains `ExitClass::NotRun` rather than falling through to `Unknown`, which reported a host mtui never contacted as a failed patch. It is the message that was wrong, not the routing: `never_ran` vetoes the group-wide rollback from the target's recorded `lastexit()`, never from a reason string, so a `-1` host skipped the downgrade under the old fallthrough too. The variant also makes the distinction structural — the one match on this enum is exhaustive — and the sentinel's reason now has a single source, so the gate and the classifier arm cannot drift. The dependency-prompt marker now logs the command and stderr like its siblings. It was the one branch leaving no forensic record, which did not matter while it could only follow a success or an unrecognised status, and does now that it is reachable on the three codes an operator is most likely to be investigating. Two costs are stated where they are decided rather than left implicit: the slmicro key fails on any non-zero status, and `transactional-update` flattens an open transaction or a failed snapshot into the same `1` as a failed patch, so either downgrades and reboots every host in the group — kept, because the alternative is reporting a failed patch as a successful update. And an unguarded `zypper -n refresh` remains the one path to a clean exit with nothing installed, deferred because `refresh` returns `4` whether one repository failed or all of them did, so closing it needs a check that the issue repo is present rather than a status test. Also corrects the `UpdateError` doc, which called the reason strings a contract consumed by callers while nothing reads them; the contract is the `UpdateFailure` variant a failure routes to.
|
Thanks — the blocking point is right and my justification for the old ordering does not survive it. Pushed BlockingApplied as you suggested:
One thing your review implies that I want to be explicit about rather than leave to inference: the install check now disagrees with Non-blocking
slmicro any-non-zero — acknowledged explicitly at the point of decision, with the blast radius and the reason it is still the better of the two options.
CHANGELOG — reflowed, and amended where the old text described the ordering this change replaces. Also folded in three things found while re-reading: the summary paragraph on Gates green locally: fmt, clippy |
…emplates (openSUSE#400) The zypper and SL Micro update templates are multi-line scripts run as a single remote command, and their last line was the repo-cleanup loop — whose status is 0 whenever it finds nothing to remove. The shell reported that, and the patch's own status was discarded before anything could read it. A patch that genuinely failed with clean output passed the update check. Each template now captures the patch's status on the line after the patch, before the post-state `zypper patches` line can clobber it, and exits with it. The list of patches is captured first and the patch runs only when that list holds at least one word, so a host carrying none of the update's products is a no-op rather than a package manager complaining about its own arguments — a status that is now real. The emptiness test is `set --` plus `[ "$#" -gt 0 ]`, not `[ -n ... ]`: the latter is true for a list of whitespace, which then splits to no words at all. The check classifies the status rather than comparing it against zero. That distinction is the whole safety of this change: an update check failure drives the group-wide rollback downgrade, which reverts every host in the group and not just the one that reported, so a host that patched perfectly must not fail. zypper's informational codes pass — including 102 ("reboot needed"), the routine outcome of patching a kernel, and 107, where a package's %post script failed although the package itself is installed and registered. The classification lives in one place rather than being spelled out a fourth time. On SL Micro it is in practice just "0 passes": upstream transactional-update absorbs zypper's status and returns only 0 or 1. The templates and the check must move together — a template reporting the real status while the check still lacked the carve-out would newly fail every host that merely needs a reboot. The rendered command text changes, so `show_log` transcripts change with it. The docs that explained why the exit code could not be trusted are rewritten rather than left to contradict the code.
zypper exits 107 (ZYPPER_EXIT_INF_RPM_SCRIPT_FAILED) when a package's %post script returns an error although the package itself was, in the man page's words, "successfully unpacked to disk and registered in the rpm database". It is one of the informational codes above 100, but the install check's success set stopped at 106, so a routine kernel or dracut scriptlet hiccup was reported as "Unknown Error". Found while giving the update check the same carve-out (openSUSE#400), where the identical omission was worse: there a false failure fires the group-wide rollback downgrade.
…s label (openSUSE#400) Review of openSUSE#446 caught that short-circuiting the whole `104 | 4 | 5 | 8` class to "package not found" degrades the two failures the check exists to surface. Only `104` (`ZYPPER_EXIT_INF_CAP_NOT_FOUND`) means the capability was not found; `4`, `5` and `8` are `ERR_ZYPP`, `ERR_PRIVILEGES` and `ERR_COMMIT` — the package was found and the transaction failed. An `8` with `Error:` on stderr is the likeliest status for a genuinely failed patch, and a `5` with `System management is locked` for a busy update stack, so the label replaced both headline diagnoses with one that is not vaguer but wrong. So only `104` now outranks the markers. The other three consult them first and keep "package not found" solely on a clean transcript. The justification for the old ordering does not survive scrutiny: nothing branches on these strings — they are rendered, logged and asserted on, never matched — and on `update` the three previously reached no verdict at all, falling through to the markers, so there was no earlier behaviour for the short-circuit to preserve. The class itself stays the install check's, unsplit, so an exit code still cannot land in two classes; what a check *says* about a member is its own to choose. The install check keeps its ordering, so the same transcript can now read differently on the two. That divergence is pinned on both sides rather than left to be discovered: install's own short-circuit was only ever exercised with an empty transcript, so hoisting its markers above the set broke no test. `-1` gains `ExitClass::NotRun` rather than falling through to `Unknown`, which reported a host mtui never contacted as a failed patch. It is the message that was wrong, not the routing: `never_ran` vetoes the group-wide rollback from the target's recorded `lastexit()`, never from a reason string, so a `-1` host skipped the downgrade under the old fallthrough too. The variant also makes the distinction structural — the one match on this enum is exhaustive — and the sentinel's reason now has a single source, so the gate and the classifier arm cannot drift. The dependency-prompt marker now logs the command and stderr like its siblings. It was the one branch leaving no forensic record, which did not matter while it could only follow a success or an unrecognised status, and does now that it is reachable on the three codes an operator is most likely to be investigating. Two costs are stated where they are decided rather than left implicit: the slmicro key fails on any non-zero status, and `transactional-update` flattens an open transaction or a failed snapshot into the same `1` as a failed patch, so either downgrades and reboots every host in the group — kept, because the alternative is reporting a failed patch as a successful update. And an unguarded `zypper -n refresh` remains the one path to a clean exit with nothing installed, deferred because `refresh` returns `4` whether one repository failed or all of them did, so closing it needs a check that the issue repo is present rather than a status test. Also corrects the `UpdateError` doc, which called the reason strings a contract consumed by callers while nothing reads them; the contract is the `UpdateFailure` variant a failure routes to.
77c2eba to
c5ba1b7
Compare
2 participants
Closes #400.
The bug
ZYPPER_UPDATEandSLM_UPDATEare multi-line scripts run as one remotecommand. Their last line is the repo-cleanup loop, whose status is
0wheneverit finds nothing to remove — so the shell reported that, and the patch's status
was discarded before anything could read it. A patch that genuinely failed with
clean output passed the update check. (The post-state
zypper patches | grepclobbers
$?even before the loop does.)This is long-standing upstream behaviour, not a port regression: the Python
templates are byte-identical, trailing loop and all.
The fix
Each template captures the patch's status on the line after the patch and
exits with it. The patch list is captured first and the patch runs only when
that list holds at least one word, so a host carrying none of the update's
products is a no-op rather than a package manager complaining about its own
arguments — a status that is now real.
The emptiness test is
set -- $mtui_patches+[ "$#" -gt 0 ], not[ -n "$mtui_patches" ]: the latter is true for a list of whitespace, whichthen word-splits to nothing and runs the patch with no operands. That was caught
in review, measured, and is covered by a test.
The safety argument — please read this part
An
updatecheck failure drives the group-wide rollback downgrade: it ishanded the whole
HostsGroupwith no filtering, removes every host's issuerepos, downgrades every host, reboots the transactional ones, and rewrites the
report's version slots. A false failure therefore reverts hosts that did
everything right. So the check classifies the status instead of comparing it
against zero:
0,100,101,102,103,106,107104,4,5,8102is "reboot needed" — the routine outcome of patching a kernel.107was missing from that set in my first draft, and the review caught it.It is
ZYPPER_EXIT_INF_RPM_SCRIPT_FAILED: a package's%postscript failedalthough the package itself was, per the man page, "successfully unpacked to
disk and registered in the rpm database". It would have failed the check and
rolled back the fleet over a scriptlet hiccup. The second commit fixes the
identical omission in the install check, where it is a live bug today.
The template change and the check change are one commit deliberately: a template
reporting the real status while the check still lacked the carve-out would newly
fail every host that merely needs a reboot.
Two things I want your call on
4/5/8report "package not found". That grouping is inheritedverbatim from the install check, and the reason strings are a stable contract,
so I kept it rather than let one exit code carry two verdicts across two
checks. But it is now on the primary failure path instead of dormant, and
the label misdiagnoses:
8isERR_COMMIT(the most likely status for thefailure this issue is about) and
5isERR_PRIVILEGES. Splitting them meansminting a reason string. Happy to do it if you'd rather.
103passes.RESTART_NEEDEDmeans zypper installed the patch thatupdates the package manager itself and remaining patches need a second run —
so mtui reports the update applied while some patches were not installed.
Inherited from the install check; the post-patch
zypper patchesline in thetranscript is what shows them. Flagging because this change advertises it.
SL Micro
The classification is in practice just "
0passes". Upstreamtransactional-updatecaptures zypper's status, tolerance-tests it, and flattenseverything else to
EXITCODE=1— it returns only0or1, verified againstopenSUSE/transactional-update@aee1e1b5(v6.1.3) and unchanged across ninetags. So neither the informational codes nor the package-not-found ones can reach
the check on that key. This started as an inference and was checked rather than
assumed; the doc says so with the citation.
Testing
The acceptance test renders each template and executes it under a real
/bin/shwith stub
zypper/transactional-updateexecutables onPATH, because thesuite's
MockConnectionscripts one exit code per command and cannot model theshell's last-command-wins rule — the entire subject of this issue. Each case was
observed red against the old template first:
must not fail an update and revert the group);
The stubs record their invocations so the tests cannot pass by never running
anything. A separate test asserts the fixture rows match under this host's awk,
so a non-GNU awk fails loudly with the real cause rather than silently skipping —
the template's
\>is a gawk extension and the macOS CI leg runs these tests.Residuals, deliberately not fixed here
zypper refreshor a missing repo yields an empty patch list, skips the patch, and reports
success having installed nothing. Same class as this bug, one step upstream —
filed separately rather than widening a change that already moves the rollback
boundary.
#[cfg(unix)]on the shell tests: they vanish on a non-unix builder with agreen
ok.perform_update_aggregates_multiple_host_failures_and_keeps_reposstillscripts its exit code via
with_default.