Fix inventory upload job hang on non-retryable errors

[nofaralfasi]

When report upload returns non-retryable client errors (4xx except 408/429), mark UploadReportDirectJob as done instead of re-raising and polling forever. This prevents HostInventoryReportJob from staying stuck around 83% and keeps the inventory upload UI from remaining in a perpetual loading/disabled state. Also adds tests for non-retryable vs retryable client error behavior.

What are the changes introduced in this pull request?

• Updated UploadReportDirectJob to treat non-retryable upload client errors as terminal.
• On HTTP 4xx (except 408 and 429), the job now logs, sets output[:status] = 'upload_skipped_non_retryable', and finishes with done! instead of retrying.
• Added tests covering

Considerations taken when implementing this change?

• Preserved retry behavior for transient conditions (408 timeout and 429 rate limit).
• Scoped the change to upload error handling only; no changes to report generation flow.
• Added explicit task output status for observability.

What are the testing steps for this pull request?

• Run unit tests for upload job
• Verify new cases:
  • 422 => action completes with upload_skipped_non_retryable
  • 429 => action raises/retries as before.

[sourcery-ai]

Hey - I've found 2 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="lib/foreman_inventory_upload/async/upload_report_direct_job.rb" line_range="84-85" />
<code_context>
+      rescue RestClient::ExceptionWithResponse => ex
+        if non_retryable_upload_error?(ex)
+          logger.warn(
+            "Upload skipped for organization '#{organization}' " \
+            "due to non-retryable response (#{ex.http_code}): #{ex.http_body}"
+          )
+          output[:status] = 'upload_skipped_non_retryable'
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider truncating or sanitizing the logged HTTP body for large or sensitive responses

Logging `ex.http_body` directly can bloat logs and expose tokens or other user data. Consider limiting the logged body (e.g., truncate, whitelist specific fields) or relying on `ex.message` while still preserving enough context for debugging.

Suggested implementation:

```ruby
        move_to_done_folder
        done!
      rescue RestClient::ExceptionWithResponse => ex
        if non_retryable_upload_error?(ex)
          logger.warn(
            "Upload skipped for organization '#{organization}' " \
            "due to non-retryable response (#{ex.http_code}): #{sanitized_http_body(ex)}"
          )
          output[:status] = 'upload_skipped_non_retryable'
          done!
          return
        end

        raise
      end

      def sanitized_http_body(exception)
        body = exception.http_body.to_s

        # Limit log size to avoid bloated logs and reduce exposure of sensitive data
        max_length = 500
        if body.length > max_length
          "#{body[0, max_length]}... [truncated]"
        else
          body
        end
      rescue StandardError
        # Fall back to exception message if body cannot be processed
        exception.message.to_s
      end

      def upload_file(cer_path)
        !Setting[:subscription_connection_enabled]
      end

```

If this class already has a shared logging or error-handling helper section, you may want to relocate `sanitized_http_body` there or make it a private method (e.g., add it under a `private` section) to match existing conventions in the file.
</issue_to_address>

### Comment 2
<location path="test/jobs/upload_report_direct_job_test.rb" line_range="262-276" />
<code_context>
+    assert_equal 'upload_skipped_non_retryable', action.output[:status]
+  end
+
+  test 'keeps retry behavior for retryable client upload errors' do
+    response = mock('response')
+    response.stubs(:code).returns(429)
+    response.stubs(:body).returns('Too Many Requests')
+
+    ForemanInventoryUpload::Async::UploadReportDirectJob.any_instance.stubs(:upload_file)
+                                                        .raises(RestClient::TooManyRequests.new(response))
+
+    action = create_action(ForemanInventoryUpload::Async::UploadReportDirectJob)
+    action.expects(:action_subject).with(@organization)
+    plan_action(action, @filename, @organization.id)
+
+    assert_raises(RestClient::TooManyRequests) do
+      action.send(:try_execute)
+    end
</code_context>
<issue_to_address>
**suggestion (testing):** Assert that the status output is not set for retryable errors

Along with asserting that `RestClient::TooManyRequests` is raised, please also assert that `action.output[:status]` is not set (or remains at its default). This makes it explicit that only non-retryable errors set `upload_skipped_non_retryable`, and that retryable errors don’t assign any status or appear completed.

```suggestion
  test 'keeps retry behavior for retryable client upload errors' do
    response = mock('response')
    response.stubs(:code).returns(429)
    response.stubs(:body).returns('Too Many Requests')

    ForemanInventoryUpload::Async::UploadReportDirectJob.any_instance.stubs(:upload_file)
                                                        .raises(RestClient::TooManyRequests.new(response))

    action = create_action(ForemanInventoryUpload::Async::UploadReportDirectJob)
    action.expects(:action_subject).with(@organization)
    plan_action(action, @filename, @organization.id)

    assert_raises(RestClient::TooManyRequests) do
      action.send(:try_execute)
    end
    assert_nil action.output[:status]
```
</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.}

[Copilot]

Pull request overview

This PR updates the ForemanInventoryUpload upload execution path to avoid Dynflow actions getting stuck retrying forever on non-retryable HTTP client errors from the upload endpoint. It treats non-retryable 4xx responses (excluding 408/429) as terminal by marking UploadReportDirectJob as done and recording an output status, and it adds tests to distinguish terminal vs retryable client errors.

Changes:

• Handle RestClient::ExceptionWithResponse in UploadReportDirectJob#try_execute, completing the action on non-retryable 4xx responses and recording output[:status].
• Add helpers to classify non-retryable client errors and to safely log/truncate HTTP response bodies.
• Add unit tests covering terminal (422) vs retryable (429) client error behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
lib/foreman_inventory_upload/async/upload_report_direct_job.rb	Treats non-retryable 4xx upload errors as terminal to prevent Dynflow polling/retry hangs; adds body sanitization for logging.
test/jobs/upload_report_direct_job_test.rb	Adds coverage for terminal vs retryable upload client error behavior (422 completes, 429 raises/retries).

[jeremylenz]

This seems the same as #1228 which was just merged?