Skip to content

update templates discard the patch-list substitution's status, so a failed refresh reports a successful update that installed nothing #447

Description

@plusky

Found while fixing #400, and deliberately left out of that PR: it is the same
class of bug, one step upstream, and closing it moves the rollback boundary
again.

Summary

Both update templates open with

mtui_patches=$(zypper -n patches | awk -F "|" '/$repa\>/ { print $2; }')

The command substitution's own status is discarded — in POSIX the assignment's
status is the substitution's, and nothing reads it. So when the pipeline
fails, the patch list comes back empty, the patch is skipped, mtui_status
stays 0, and mtui reports a successful update having installed nothing.

Failure modes that reach it:

  • zypper -n refresh failed earlier in the script (its status is likewise
    discarded — no set -e);
  • the update repo was never added, or was added under a different alias;
  • zypper -n patches itself fails — exit 6 (no repositories defined) or 7
    (ZYpp library lock), among others.

In each case the operator is told the update installed cleanly.

Why it is not #400

#400 was "the patch ran and failed, and we discarded its status". This is "the
patch never ran, and we cannot tell that apart from having nothing to run". The
fix for #400 made the second case explicitmtui_status=0 on the skip path
— where it used to be an accident, which is what makes it worth stating now.

Note the skip path is also legitimate: a host carrying none of the update's
products genuinely has no patches, and must keep passing. So this is not a
matter of failing on an empty list; it needs the substitution's own failure
distinguished from a genuinely empty result.

Suggested direction

Capture the substitution's status and route it separately from the patch's, e.g.

if ! mtui_patches=$(zypper -n patches | awk ...); then
    mtui_status=<sentinel>
fi

with the sentinel classified as its own verdict rather than reusing "package not
found" or "Unknown Error" — the operator needs "could not determine what to
patch", which is a different action from "the patch failed".

Two things to weigh before implementing:

  1. The blast radius. A new failure class on this path fires the group-wide
    rollback downgrade, which reverts every host in the group. Whether "could not
    list patches" should roll back at all is a real question — arguably it should
    fail the host without reverting peers, like the -1 never-ran gate does.
  2. A pipeline's status is the last stage's, so awk's success would mask a
    failing zypper. set -o pipefail is not POSIX; ${PIPESTATUS[0]} is bash.
    The portable form needs thought, and the templates currently target plain
    /bin/sh.

Related

The same shape exists in downgrade.rs's LIST_COMMAND, whose consumer already
documents that the pipeline's status is awk's and therefore only ever means "the
probe itself broke" (reports/update_flow.rs). Worth fixing together.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions