Fix IoP host UUID data consistency issues

[jeremylenz]

Fix IoP host UUID data consistency issues

User Impact

In IoP (Insights on Premise) mode, hosts that were previously registered to Red Hat Hybrid Cloud Console could display incorrect/stale UUIDs throughout the application. This caused:

• Data inconsistencies when viewing host details and recommendations
• Incorrect identification during host unregistration
• Mismatched records between Foreman and the local Insights inventory

This fix ensures IoP hosts always use their subscription manager UUID (the correct identifier for IoP environments) instead of stale cloud-assigned UUIDs. Hosts are automatically
synchronized when they upload package profiles.

Note: This does not remove the UUID field from insights facets, or allow nils there. The insights UUID is still kept in sync just as before, as a "just in case" kind of thing. But I've tried to move the actual decision points to look at subscription facet. This gets us closer to a single source of truth.

Technical Changes

• Added insights_uuid method that returns the appropriate UUID based on operating mode:
  • IoP mode: Always uses subscription facet UUID (current/correct)
  • Cloud mode: Uses insights facet UUID (cloud-assigned)
• Added ensure_iop_insights_uuid method to automatically sync mismatched UUIDs
• Updated all call sites to use the new insights_uuid method
• Comprehensive test coverage with 14 new test cases

Testing steps

1. Get an IoP host with recommendations
2. Note that recommendations display BOTH on the Insights > Recommendations page, and on the host details > Recommendations tab
3. In rails console, run

myhost.subscription_facet.uuid
myhost.insights_facet.uuid

Note that they are the same.
5. In rails console, run

myhost.insights_facet.update(uuid: "foo")

Now, the uuids are different. This simulates a stale uuid from a host previously registered to Hosted insights.

6. Refresh

Before checking out the PR:

Note that recommendations still display in Insights > Recommendations, and show the affected host. BUT they do NOT show up on the host details > Recommendations tab.
You may notice a failed API request to "foo" on the browser Network tab

After checking out the PR:

Recommendations will display on BOTH pages
No failed API requests

[sourcery-ai]

Reviewer's Guide

Ensures hosts in IoP mode consistently use the subscription manager UUID for Insights operations by introducing a unified insights_uuid accessor, a synchronization helper to fix stale UUIDs, updating all call sites to use the new accessor, and adding factories and tests to cover IoP/non-IoP and mismatch scenarios.

Sequence diagram for generate_host_report with IoP UUID synchronization

sequenceDiagram
  participant System
  participant PackageProfileUploadExtensions as PackageProfileUploadExtensions
  participant Host
  participant ForemanRhCloud
  participant SubscriptionFacet
  participant InsightsFacet

  System->>PackageProfileUploadExtensions: generate_host_report()
  PackageProfileUploadExtensions->>Host: ensure_iop_insights_uuid()

  alt IoP_mode
    Host->>ForemanRhCloud: with_iop_smart_proxy?()
    ForemanRhCloud-->>Host: true
    Host->>Host: check insights_facet.present? and subscription_facet.present?
    Host->>SubscriptionFacet: uuid
    SubscriptionFacet-->>Host: sub_uuid
    Host->>InsightsFacet: uuid
    InsightsFacet-->>Host: insights_uuid
    alt uuids_differ
      Host->>InsightsFacet: update!(uuid: sub_uuid)
      InsightsFacet-->>Host: updated
    else uuids_match
      Host-->>Host: no_update
    end
  else non_IoP_mode
    Host-->>Host: ensure_iop_insights_uuid returns
  end

  PackageProfileUploadExtensions->>Host: insights()
  Host-->>PackageProfileUploadExtensions: InsightsFacet or nil
  PackageProfileUploadExtensions-->>System: continue upload flow

Loading

Class diagram for updated RhCloudHost UUID handling

classDiagram
  class Host {
    +SubscriptionFacet subscription_facet
    +InsightsFacet insights_facet
    +InsightsFacet insights
    +insights_facet()
    +insights_uuid()
    +ensure_iop_insights_uuid()
  }

  class RhCloudHost {
    <<module>>
    +insights_facet()
    +insights_uuid()
    +ensure_iop_insights_uuid()
  }

  class SubscriptionFacet {
    +String uuid
  }

  class InsightsFacet {
    +String uuid
    +update!(uuid)
  }

  class ForemanRhCloud {
    +with_iop_smart_proxy?()
  }

  Host ..|> RhCloudHost : includes
  Host --> SubscriptionFacet : has_one
  Host --> InsightsFacet : has_one
  ForemanRhCloud ..> Host : configures_iop_mode

  RhCloudHost ..> SubscriptionFacet : uses
  RhCloudHost ..> InsightsFacet : uses
  RhCloudHost ..> ForemanRhCloud : queries_mode

Loading

Flow diagram for insights_uuid resolution in IoP and non-IoP modes

flowchart TD
  A[call insights_uuid] --> B{ForemanRhCloud.with_iop_smart_proxy?}
  B -- true (IoP) --> C{subscription_facet present?}
  C -- yes --> D[return subscription_facet.uuid]
  C -- no --> E[return nil]

  B -- false (non-IoP) --> F{insights_facet present?}
  F -- yes --> G[return insights_facet.uuid]
  F -- no --> H[return nil]

Loading

File-Level Changes

Change	Details	Files
Add unified insights_uuid accessor and sync helper on RhCloudHost to prevent stale Insights UUID usage, and back them with focused unit tests.	Introduce insights_uuid method that switches between subscription_facet.uuid in IoP mode and insights_facet.uuid in non-IoP mode with nil-safe access. Add ensure_iop_insights_uuid to align insights_facet.uuid with subscription_facet.uuid when both facets exist and differ, safely handling missing facets and nil UUIDs. Create rh_cloud_host_test to cover insights_uuid behavior across IoP modes, missing facets, stale UUIDs, and ensure_iop_insights_uuid edge cases including nil UUIDs and post-cloud-registration correction.	app/models/concerns/rh_cloud_host.rb test/unit/rh_cloud_host_test.rb
Update API views, controllers, and async tasks to rely on insights_uuid so IoP paths consistently use the correct UUID for lookups and deletions.	Change Insights base and advisor_engine host_details RABL views to emit UUIDs via host.insights_uuid instead of reading the facet uuid directly. Modify registration_manager_extensions to use insights_uuid when unregistering a host from HBI, ensuring correct host deletion in IoP. Adjust AdvisorEngine controller test expectations to use insights_uuid-based UUID arrays. Update connector_playbook_execution_reporter_task to key host state and task status by insights_uuid rather than insights.uuid.	app/views/api/v2/hosts/insights/base.rabl app/views/api/v2/advisor_engine/host_details.json.rabl app/controllers/concerns/foreman_rh_cloud/registration_manager_extensions.rb test/controllers/insights_cloud/api/advisor_engine_controller_test.rb lib/insights_cloud/async/connector_playbook_execution_reporter_task.rb
Ensure IoP package profile uploads normalize Insights UUIDs before interacting with cloud services, and extend factories to model UUID sync/mismatch scenarios.	Call ensure_iop_insights_uuid in package_profile_upload_extensions#generate_host_report before relying on Insights presence, so stale UUIDs are corrected during IoP uploads. Add with_mismatched_insights_uuid and with_synced_insights_uuid traits to host factories to simulate stale and aligned Insights/subscription UUID states for tests. Update registration_manager_extensions tests to create hosts with subscription facets and derive Insights facet UUIDs from subscription_facet.uuid instead of hardcoding values.	app/controllers/concerns/insights_cloud/package_profile_upload_extensions.rb test/factories/insights_factories.rb test/unit/lib/foreman_rh_cloud/registration_manager_extensions_test.rb

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The ensure_iop_insights_uuid helper is currently invoked unconditionally in generate_host_report; consider guarding this call with ForemanRhCloud.with_iop_smart_proxy? (or documenting why it is safe in non-IoP) so the synchronization logic is only run in the mode it’s intended for.
• The new :with_mismatched_insights_uuid and :with_synced_insights_uuid factory traits rely on :with_subscription but only document this in comments; consider asserting the presence of subscription_facet (e.g., raising if nil) so accidental misuse of these traits fails fast in tests.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `ensure_iop_insights_uuid` helper is currently invoked unconditionally in `generate_host_report`; consider guarding this call with `ForemanRhCloud.with_iop_smart_proxy?` (or documenting why it is safe in non-IoP) so the synchronization logic is only run in the mode it’s intended for.
- The new `:with_mismatched_insights_uuid` and `:with_synced_insights_uuid` factory traits rely on `:with_subscription` but only document this in comments; consider asserting the presence of `subscription_facet` (e.g., raising if nil) so accidental misuse of these traits fails fast in tests.

## Individual Comments

### Comment 1
<location> `lib/insights_cloud/async/connector_playbook_execution_reporter_task.rb:128-132` </location>
<code_context>
         hosts_state = Hash[job_invocation.targeting.hosts.map do |host|
           next unless host.insights&.uuid
           [
-            host.insights.uuid,
-            task_status(job_invocation.sub_task_for_host(host), host.insights.uuid),
+            host.insights_uuid,
+            task_status(job_invocation.sub_task_for_host(host), host.insights_uuid),
           ]
         end.compact]
</code_context>

<issue_to_address>
**issue (bug_risk):** Guard still checks `host.insights&.uuid` but we now rely on `host.insights_uuid`, which may be present even when `host.insights` is nil.

In IoP, `host.insights_uuid` may come from the subscription facet even when `host.insights` is nil. With `next unless host.insights&.uuid`, those hosts are skipped and never added to `hosts_state`, despite having a valid `insights_uuid`. Consider guarding on `host.insights_uuid` instead so subscription-only hosts are included.
</issue_to_address>

### Comment 2
<location> `app/views/api/v2/hosts/insights/base.rabl:1-3` </location>
<code_context>
-attributes :uuid
-
+node :uuid do |facet|
+  facet&.host&.insights_uuid
+end
 node :insights_hit_details do |facet|
</code_context>

<issue_to_address>
**suggestion (performance):** Switching UUID source to `facet.host.insights_uuid` may introduce extra queries or missing preloads.

This path now dereferences `facet.host.insights_uuid` instead of using the already-present `facet.uuid`, which can cause N+1s if `host` (and any associations `insights_uuid` relies on) aren’t preloaded for this view. Please confirm those preloads exist for this endpoint, or consider keeping `facet.uuid` for the non-IoP case and only resolving via `host` where strictly needed.

```suggestion
node :uuid do |facet|
  facet&.uuid || facet&.host&.insights_uuid
end
```
</issue_to_address>

### Comment 3
<location> `test/controllers/insights_cloud/api/advisor_engine_controller_test.rb:15-19` </location>
<code_context>
       end

       test 'shows hosts with uuids' do
-        uuids = [@host1.insights.uuid, @host2.insights.uuid]
+        uuids = [@host1.insights_uuid, @host2.insights_uuid]
         get :host_details, params: { organization_id: @test_org.id, host_uuids: uuids }
         assert_response :success
         assert_template 'api/v2/advisor_engine/host_details'
</code_context>

<issue_to_address>
**suggestion (testing):** Add a test for IoP mode to ensure host_details uses subscription_facet UUID when insights_facet is stale

Given the view now uses `host.insights_uuid`, IoP vs non-IoP behavior diverges when `insights_facet.uuid` is stale. Please add a test that:
- stubs `ForemanRhCloud.with_iop_smart_proxy?` to `true`,
- creates a host where `insights_facet.uuid` and `subscription_facet.uuid` differ, and
- verifies `host_details` returns the subscription UUID in `insights_uuid`.
This will confirm the data-consistency fix is covered at the API level.

Suggested implementation:

```ruby
      end

      test 'shows hosts with uuids' do
        uuids = [@host1.insights_uuid, @host2.insights_uuid]
        get :host_details, params: { organization_id: @test_org.id, host_uuids: uuids }
        assert_response :success
        assert_template 'api/v2/advisor_engine/host_details'
      end

      test 'in IoP mode host_details prefers subscription uuid when insights uuid is stale' do
        ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(true)

        stale_insights_uuid = 'stale-insights-uuid-123'
        subscription_uuid   = 'subscription-uuid-456'

        host = FactoryBot.create(:host, organization: @test_org)

        # Ensure the host has diverging facet UUIDs
        if host.respond_to?(:build_insights_facet)
          host.build_insights_facet(uuid: stale_insights_uuid)
        else
          host.insights_facet.uuid = stale_insights_uuid
        end

        if host.respond_to?(:build_subscription_facet)
          host.build_subscription_facet(uuid: subscription_uuid)
        else
          host.subscription_facet.uuid = subscription_uuid
        end

        host.save!
        host.reload

        # Call host_details with the stale insights facet UUID
        get :host_details, params: {
          organization_id: @test_org.id,
          host_uuids: [stale_insights_uuid]
        }

        assert_response :success
        assert_template 'api/v2/advisor_engine/host_details'

        body = JSON.parse(response.body)

        # Assuming response contains a "hosts" array with "insights_uuid" per host
        insights_uuids = body.fetch('hosts').map { |h| h['insights_uuid'] }
        assert_includes insights_uuids, subscription_uuid
        refute_includes insights_uuids, stale_insights_uuid

```

Depending on the existing test and response structure in `advisor_engine_controller_test.rb`, you may need to:

1. Adjust host creation:
   - If you don't use `FactoryBot`, replace `FactoryBot.create(:host, organization: @test_org)` with the appropriate fixture or factory used elsewhere in this test file.
   - If the facet associations are named differently (e.g. `:insights` instead of `:insights_facet`), update `build_insights_facet` / `build_subscription_facet` and the direct facet access accordingly.

2. Adjust JSON response parsing:
   - If `host_details` renders a different JSON shape (for example `body['data']['hosts']` or `body['results']`), update `body.fetch('hosts')` to match the actual key path.
   - If the field name is different (e.g. `'insights_id'` instead of `'insights_uuid'`), update the hash lookup in `map { |h| h['insights_uuid'] }`.

3. If the test suite uses a different stubbing library than `stubs` (e.g. `expects` or `stub` from another helper), align the call to how `ForemanRhCloud.with_iop_smart_proxy?` is stubbed in other tests.
</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.}

[nofaralfasi]

LGTM. tested and works as expected.
One potential bug to investigate: the guard condition in registration_manager_extensions.rb:16 may cause hosts with subscription_facet.uuid but no insights_facet.uuid to skip HBI unregistration in IoP mode.

[nofaralfasi]

Question about InsightsHit#host_uuid:
I noticed that app/models/insights_hit.rb still accesses insights_facet.uuid directly.
Was this intentionally left unchanged? I'm trying to understand if there's a reason this doesn't need updating to use insights_uuid, or if it was just missed.
I see that in practice the UUIDs should stay in sync because:

• ensure_iop_insights_uuid syncs them on package profile upload
• HitsUploader#update_facets updates the facet UUID when hits are uploaded

So the current code probably works fine in most cases. But for consistency with the pattern established in this PR (always reading from the authoritative source via insights_uuid), should we update this too?

[jeremylenz]

I noticed that app/models/insights_hit.rb still accesses insights_facet.uuid directly. Was this intentionally left unchanged? I'm trying to understand if there's a reason this doesn't need updating to use insights_uuid, or if it was just missed. I see that in practice the UUIDs should stay in sync because:

• ensure_iop_insights_uuid syncs them on package profile upload
• HitsUploader#update_facets updates the facet UUID when hits are uploaded

So the current code probably works fine in most cases. But for consistency with the pattern established in this PR (always reading from the authoritative source via insights_uuid), should we update this too?

I'm on the fence here. On one hand, I see what you're saying about consistency and source of truth. But on the other hand,

• InsightsHit is a hosted-only model, so these codepaths should never be touched in IoP
• if something is out of sync and ForemanRhCloud.with_iop_smart_proxy? is incorrect, we could incorrectly try finding a non-IoP host via its subscription-manager id.
• in the course of building IoP, I have tried to adhere to the principle "Don't change the non-IoP code / behavior / experience at all, if you can help it." But hey, I've changed it in some other places that only affect hosted. Like I said.. on the fence.

Let me know your thoughts.

[nofaralfasi]

I'm on the fence here. On one hand, I see what you're saying about consistency and source of truth. But on the other hand,

• InsightsHit is a hosted-only model, so these codepaths should never be touched in IoP
• if something is out of sync and ForemanRhCloud.with_iop_smart_proxy? is incorrect, we could incorrectly try finding a non-IoP host via its subscription-manager id.
• in the course of building IoP, I have tried to adhere to the principle "Don't change the non-IoP code / behavior / experience at all, if you can help it." But hey, I've changed it in some other places that only affect hosted. Like I said.. on the fence.

Let me know your thoughts.

That makes sense - Since InsightsHit is hosted-only, and if these codepaths aren't touched in IoP, then adding the IoP branching logic there would violate the "minimal change to non-IoP code" principle and could introduce unnecessary risk.

No changes needed here - thanks for the context!