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

Thanks for the review.

> -----Original Message-----
> From: Alexander Kanavin <[email protected]>
> Sent: Tuesday, 2 June 2026 11:55
> To: Daniel Turull <[email protected]>
> Cc: [email protected]
> Subject: 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?

Yes, my first iteration had only should_skip and then I added the prune. 
Your suggestion is cleaner. I'll fix it in v2.

> 
> 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.

I'll add a log entry for it in v2.

> 
> > @@ -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.

I cannot reproduce it now. I was over conservative with the failures.  I'll drop it.
> 
> > +            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?

I'll create a patch to remove it and remove from the incremental code.

I'll rerun the full test with today's master and I'll send a v2 when I'm happy with the testing.

Thanks,
Daniel