Re: [AUH] [PATCH] upgrade-helper: add state module and --incremental mode

On Mon, 1 Jun 2026 at 14:04, <[email protected]> wrote:
> Without this, every AUH run retries all candidates regardless of previous
> outcomes, wasting time on recipes that consistently fail or were already
> upgraded recently.

Thanks for working on these, much appreciated! Comments below.

> +    def should_skip(self, pn, version):
> +        entry = self.data.get(pn, {}).get(version)
> +        if not entry:
> +            return False
> +        age = time.time() - entry.get("timestamp", 0)
> +        if entry.get("result") == RESULT_SUCCESS:
> +            return age < self.success_max_age
> +        return age < self.cooldown
> +
> +    def _prune(self):
> +        """Remove stale entries older than their respective max-age."""
> +        now = time.time()
> +        for pn in list(self.data):
> +            versions = self.data[pn]
> +            for ver in list(versions):
> +                entry = versions[ver]
> +                age = now - entry.get("timestamp", 0)
> +                if entry.get("result") == RESULT_SUCCESS:
> +                    if age > self.success_max_age:
> +                        del versions[ver]
> +                else:
> +                    if age > self.cooldown:
> +                        del versions[ver]
> +            if not versions:
> +                del self.data[pn]

I am somewhat confused as to how these functions complement each
other, because they seem to repeat each other's timestamp logic. If
entries that are too old are already pruned at the start, then the
remaining entries will always have their age less than maximum
periods, and it's not necessary to compare times in should_skip(), and
it should simply return True. No?

I also think that any 'decisions' regarding pruning, and skipping
should be at least printed out, so that the log contains information
about why something wasn't attempted, or why it was attempted again,
including information about when it was attempted previously and what
was the result then.

> @@ -520,6 +542,10 @@ class Updater(object):
>                  g['error'] = e
>                  failed_pkggroups_ctx.append(g)
>
> +            # Capture upgrade error before commit_changes() which may
> +            # overwrite g['error'] with a git commit failure.
> +            upgrade_err = g.get('error') if self.state else None
> +

Such special-casing always makes me a bit uneasy. What is the scenario
where this matters? Did you hit that in testing? If the upgrade itself
succeeded, but the commit failed, then the overall result is 'failed',
and we shouldn't roll it back.

> +            if self.state:
> +                # UpgradeNotNeededError means the recipe is already current,
> +                # which is a successful outcome — no retry needed.
> +                if not upgrade_err or isinstance(upgrade_err, UpgradeNotNeededError):
> +                    result = RESULT_SUCCESS
> +                else:
> +                    result = RESULT_FAILURE

Looking at code, there isn't any place where UpgradeNotNeededError is
actually raised, so you could probably add a patch that removes it
altogether for lack of use?

Alex