Fixes #38953 - remove pool deletion events

[jturel]

What are the changes introduced in this pull request?

This PR removes all pool deletion events. The pools that would be deleted instead remain in the database until manifest delete/refresh.

Considerations taken when implementing this change?

In #11647, which removes the pool import event, I'll handle importing the additional attributes onto Katello::Pool to ensure that the expired pools retain all necessary data to display them.

What are the testing steps for this pull request?

• Check that manifest import/refresh/delete removes irrelevant or expired Katello::Pool records. That's the assumption this change is operating on.

Summary by Sourcery

Remove handling and propagation of pool deletion events so pool records now persist until manifest operations clean them up.

Bug Fixes:

• Stop deleting Katello::Pool records in response to Candlepin pool.deleted events or missing Candlepin pools.

Enhancements:

• Simplify Candlepin pool data retrieval by returning an empty hash when a pool is gone instead of enqueuing delete events.
• Remove now-unused message handler methods, event registrations, and tests related to pool deletion events.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The Katello::Pool.expired scope uses Date.new...DateTime.now, which mixes Date and DateTime and hardcodes a start bound; consider simplifying to a one-sided comparison with a time-zone-aware value (e.g. where('end_date < ?', Time.zone.now) or where(end_date: ..Time.zone.now)), which also avoids the odd Date.new default.
• DestroyExpired calls ::Katello::Pool.expired.destroy_all in one shot, which could become expensive if many pools are expired; consider deleting in batches (e.g. find_each or limited scopes) or adding some safety bounds to avoid long-running transactions.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `Katello::Pool.expired` scope uses `Date.new...DateTime.now`, which mixes `Date` and `DateTime` and hardcodes a start bound; consider simplifying to a one-sided comparison with a time-zone-aware value (e.g. `where('end_date < ?', Time.zone.now)` or `where(end_date: ..Time.zone.now)`), which also avoids the odd `Date.new` default.
- `DestroyExpired` calls `::Katello::Pool.expired.destroy_all` in one shot, which could become expensive if many pools are expired; consider deleting in batches (e.g. `find_each` or limited scopes) or adding some safety bounds to avoid long-running transactions.

## Individual Comments

### Comment 1
<location> `app/models/katello/pool.rb:17-21` </location>
<code_context>
     end
     scope :upstream, -> { where.not(upstream_pool_id: nil) }
     scope :redhat, -> { joins(:products).merge(Katello::Product.redhat).distinct }
+    scope :expired, -> { where(end_date: Date.new...DateTime.now) }

     include Glue::Candlepin::CandlepinObject
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Clarify the date range for `expired` scope and consider time zone / range semantics

`Date.new` as the lower bound is a bit surprising and mixes `Date` with `DateTime`. If the intent is just `end_date < now`, using `where('end_date < ?', Time.zone.now)` (or a range with an explicit lower bound) avoids implicit type casting. Also, the exclusive `...` range means records with `end_date == DateTime.now` are treated as non-expired; if that’s not intended, consider `..` or a simple `<` comparison instead.

```suggestion
    scope :upstream, -> { where.not(upstream_pool_id: nil) }
    scope :redhat, -> { joins(:products).merge(Katello::Product.redhat).distinct }
    scope :expired, -> { where('end_date < ?', Time.zone.now) }

    include Glue::Candlepin::CandlepinObject
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[jturel]

@jeremylenz I thought of a different approach to this when I wondered, "why even delete expired pools at all?"

In the past the reason most likely came down to the fact that pools had to be synced with candlepin (and certainly not expired) because they were being attached directly to hosts and activation keys. Now, they are basically just a container to show some data on the screen with the main (only?) functionality being adding and removing subscriptions from a manifest which I presume is going away with SCA-aware manifests.

So, why not just keep the expired pools in the db, at least until a manifest refresh syncs everything back up? Do you see any complications in keeping that data around?

[jeremylenz]

Now, they are basically just a container to show some data on the screen with the main (only?) functionality being adding and removing subscriptions from a manifest which I presume is going away with SCA-aware manifests.

So, why not just keep the expired pools in the db, at least until a manifest refresh syncs everything back up? Do you see any complications in keeping that data around?

Given that the pools in the Katello db are basically just cache of Candlepin, and they're not actually used for enforcement of expired subscriptions (Candlepin + pulp_content_guard does that), I don't see a problem with this.

I'm also trying to think about what it will look like once SCA-aware manifests arrive. I don't think there will be any issue there but just to be safe I'll cc @nikosmoum

[nikosmoum]

@jeremylenz @jturel I think both of you have a better understanding of how the pool copies in the Katello db are used than me, but afaik I agree with you that they're only used to populate the subscriptions page in SCA mode, so leaving expired pools around until the next manifest refresh cleans them up is fine.
And I would expect the same to be true when SCA-aware manifests arrive.

[jturel]

@jeremylenz Ok, expired pools will be kept around. What to do with them in terms of visibility?

1. Use an expired scope and perform filtering at the API level to hide them
2. Show expired pools via API. The pool in candlepin won't be there anymore after its own cleanup happens. Would need to import a few more attributes that are currently coming over the candlepin API into Katello::Pool

2 is a more interesting option to me. It removes a bunch of lazy_accessor stuff, which I like. Someone may even find knowing which subscriptions have expired useful in /subscriptions in case they ignore the notifications that go out or aren't a recipient of them.

[jeremylenz]

Does 1 mean that an expired subscription would simply disappear?

If I were a user, I would think it helpful that the expired subscription still shows in the UI, at least for some time. That way I can look at the date, realize I'm a doofus for not renewing, and then move on with life. The other way I'm just left wondering where my subscription went.

If 2 allows the above, that seems better to me too.

[jeremylenz]

Side-note - we also need to make sure future-dated subscriptions show. Not sure if any of this affects that, but still good to keep in mind

[jturel]

Does 1 mean that an expired subscription would simply disappear?

Yes, and that's the closest to the current behavior.

If I were a user, I would think it helpful that the expired subscription still shows in the UI, at least for some time. That way I can look at the date, realize I'm a doofus for not renewing, and then move on with life. The other way I'm just left wondering where my subscription went.

Candlepin runs its pool cleanup every hour so this wouldn't really be a change from the status quo since the lifetime of expired subs in Katello is <= 1 hour.

If 2 allows the above, that seems better to me too.

Agree. It opens up other UX opportunities like bringing attention to those particular subscriptions.

Side-note - we also need to make sure future-dated subscriptions show. Not sure if any of this affects that, but still good to keep in mind

I don't think anything I'll change would affect that, but not a bad idea to verify with the right manifest.

[sourcery-ai]

Hey - I've left some high level feedback:

• The Pool.candlepin_data signature change drops the rescue_gone argument; double-check all call sites to ensure none still rely on the second parameter or on the previous behavior of conditionally enqueuing delete events.
• With Katello::Errors::CandlepinPoolGone now returning {} silently, consider adding at least a debug/info log to help diagnose unexpected missing pools without reintroducing delete events.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `Pool.candlepin_data` signature change drops the `rescue_gone` argument; double-check all call sites to ensure none still rely on the second parameter or on the previous behavior of conditionally enqueuing delete events.
- With `Katello::Errors::CandlepinPoolGone` now returning `{}` silently, consider adding at least a debug/info log to help diagnose unexpected missing pools without reintroducing delete events.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[jeremylenz]

Created a duplicate pool in rails console, despite our very robust non-null and foreign key constraints:

[20] pry(main)> Katello::Pool.all.map { |pool| { id: pool.id, subscription: pool.subscription&.name } }
=> [{:id=>1, :subscription=>"Red Hat Developer Subscription for Individuals"},
 {:id=>2, :subscription=>"Red Hat Satellite Managed Design Program"},
 {:id=>3, :subscription=>"Red Hat Beta Access"},
 {:id=>4, :subscription=>"Red Hat Quay.io (Unlimited Repositories), Self-Support"},
 {:id=>5, :subscription=>"Red Hat Virtualization Manager"},
 {:id=>6, :subscription=>"Red Hat Beta Access"},
 {:id=>7, :subscription=>"Red Hat Ansible Automation Platform, Premium (100 Managed Nodes)"},
 {:id=>8, :subscription=>"Red Hat OpenShift Broker/Master Infrastructure for IBM Power, LE (2 Cores)"},
 {:id=>9, :subscription=>"Employee SKU"},
 {:id=>10, :subscription=>"Red Hat Enterprise Linux Server with Satellite, Premium (Physical or Virtual Nodes)"},
 {:id=>11, :subscription=>"Red Hat Ansible Automation Platform, Premium (100 Managed Nodes)"},
 {:id=>15, :subscription=>"Red Hat Satellite Managed Design Program"}] <------------------ duplicate

After manifest refresh with the PR checked out, the duplicate pool is removed:

[1] pry(main)> Katello::Pool.all.map { |pool| { id: pool.id, subscription: pool.subscription&.name } }
=> [{:id=>1, :subscription=>"Red Hat Developer Subscription for Individuals"},
 {:id=>2, :subscription=>"Red Hat Satellite Managed Design Program"},
 {:id=>3, :subscription=>"Red Hat Beta Access"},
 {:id=>4, :subscription=>"Red Hat Quay.io (Unlimited Repositories), Self-Support"},
 {:id=>5, :subscription=>"Red Hat Virtualization Manager"},
 {:id=>6, :subscription=>"Red Hat Beta Access"},
 {:id=>7, :subscription=>"Red Hat Ansible Automation Platform, Premium (100 Managed Nodes)"},
 {:id=>8, :subscription=>"Red Hat OpenShift Broker/Master Infrastructure for IBM Power, LE (2 Cores)"},
 {:id=>9, :subscription=>"Employee SKU"},
 {:id=>10, :subscription=>"Red Hat Enterprise Linux Server with Satellite, Premium (Physical or Virtual Nodes)"},
 {:id=>11, :subscription=>"Red Hat Ansible Automation Platform, Premium (100 Managed Nodes)"}]

[jeremylenz]

@jturel If you're good with the changes here, I am ready to merge it. I have a real subscription expiring on April 2, I will create a reminder to see what happens then.

[jturel]

@jturel If you're good with the changes here, I am ready to merge it. I have a real subscription expiring on April 2, I will create a reminder to see what happens then.

Go for it!