feature: adopt retry-go for HTTP retry logic

[fgiudici]

Replace manual sleep-based retry loop in FetchFrom() with github.com/avast/retry-go/v4.
Preserves 3 attempts with exponential backoff (60s initial delay).
Tests override via retryOptions variable.

Fixes #5

[qodo-for-releng]

PR Summary by Qodo

Adopt retry-go for catalog HTTP retries

✨ Enhancement 🧪 Tests 🕐 10-20 Minutes

AI Description

• Replace the manual HTTP retry loop with retry-go orchestration.
• Preserve three attempts and 60-second exponential backoff.
• Let tests suppress retry delays through configurable options.

Diagram

graph TD
  A["Caller"] --> B["FetchFrom"] --> C["retry-go"] --> D["HTTP fetch"] --> E{"Succeeded?"}
  E -- Yes --> F["Catalog result"]
  E -- No --> C

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Retain the manual retry loop

• ➕ Avoids adding an external dependency
• ➕ Keeps timing behavior directly visible in FetchFrom
• ➖ Maintains bespoke retry and backoff logic
• ➖ Requires a custom sleep seam for fast tests
• ➖ Becomes harder to extend with richer retry policies

2. Inject a per-call retry policy

• ➕ Avoids package-global mutable test configuration
• ➕ Allows callers to customize attempts and delays
• ➕ Supports parallel tests without shared retry state
• ➖ Expands the public or internal API surface
• ➖ Adds configuration complexity for a fixed package policy
• ➖ Requires more extensive test and caller changes

Recommendation: Using retry-go is appropriate because it replaces bespoke scheduling with a maintained retry implementation while preserving the existing policy. Per-call policy injection would improve isolation but is unnecessary unless callers need customization or tests become parallel; retaining the manual loop offers too little benefit beyond avoiding the dependency.

Files changed (4) +36 / -26

Refactor (1) +15 / -18

plcc.goDelegate catalog retries to retry-go +15/-18

Delegate catalog retries to retry-go

• Replaces the manual sleep-based loop with DoWithData and shared retry options. The policy retains three attempts, a 60-second initial delay, exponential backoff, and last-error propagation.

pkg/plcc/plcc.go

Tests (1) +10 / -8

plcc_test.goAdapt fetch tests to retry-go options +10/-8

Adapt fetch tests to retry-go options

• Replaces the mocked sleep function with a test helper that appends a zero-delay retry option. Existing failure and recovery tests continue validating three attempts without waiting for production backoff intervals.

pkg/plcc/plcc_test.go

Other (2) +11 / -0

go.modAdd retry-go as a direct dependency +1/-0

Add retry-go as a direct dependency

• Adds github.com/avast/retry-go/v4 v4.7.0 for HTTP retry orchestration.

go.mod

go.sumRecord retry-go dependency checksums +10/-0

Record retry-go dependency checksums

• Adds checksums for retry-go and its resolved dependency graph.

go.sum

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:14 PM UTC · Completed 5:30 PM UTC
Commit: 9802f47 · View workflow run →

[qodo-for-releng]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[fullsend-ai-review]

Review

Findings

Low

• [error-message-change] pkg/plcc/plcc.go:124 — The error message changed from "after 3 attempts: <err>" to "after retries: <err>", losing the attempt count from the diagnostic message. No downstream code or test depends on this exact text, and the underlying error is still properly wrapped via %w, so errors.Is/errors.As continue to work. This is a minor loss of information in error diagnostics that could be useful when debugging production failures.
  Remediation: Consider preserving the attempt count, e.g. fmt.Errorf("after 3 attempts: %w", err).

• [scope-tier-alignment] PR is labeled feature but the change is refactoring/tech-debt (replacing manual retry with a library). No new capabilities are exposed. A refactor or enhancement label would be more accurate.

Previous run

Review

Findings

Low

• [error-message-informativeness] pkg/plcc/plcc.go:123 — The error message changed from "after 3 attempts: ..." to "after retries: ...", losing the attempt count and introducing a terminology inconsistency with the comment on line 117 ("makes up to 3 attempts"). Since the count is a compile-time constant (retry.Attempts(3)), the information loss is minor, but including it is a cheap improvement.
  Remediation: Change to fmt.Errorf("after 3 attempts: %%w", err) to restore the count and align with the comment's terminology.

• [architecture-coherence] pkg/plcc/plcc.go:108 — New dependency github.com/avast/retry-go/v4 is not reflected in CLAUDE.md's Tech Stack section, which currently lists only sigs.k8s.io/yaml and spf13/pflag.
  Remediation: Update CLAUDE.md Tech Stack to include avast/retry-go/v4 for HTTP retry logic.

Previous run (2)

Review

Findings

Low

• [behavioral change] pkg/plcc/plcc.go:121 — The error message format changed from "after 3 attempts: <err>" to "after retries: <err>". No code path matches against this string (main.go uses errors.As/errors.Is), so this is a cosmetic change in stderr output with no breakage risk.

• [test integrity] pkg/plcc/plcc_test.go:280 — mockRetry appends retry.Delay(0) to override the 60-second base delay. With BackOffDelay, retry-go internally clamps a zero delay to ~1 nanosecond. The original mockSleep was a complete no-op. Functionally equivalent for test purposes — no practical impact.

• [test-mock-fragility] pkg/plcc/plcc_test.go:283 — mockRetry appends retry.Delay(0) to a copy of the production retryOptions slice, creating duplicate Delay options. This follows the standard Go functional-options last-write-wins pattern and is intentional — it ensures tests exercise the same base configuration as production with only the delay overridden. No action needed.

---

Labels: PR implements a feature (retry-go adoption) in the PLCC client package

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:46 PM UTC · Completed 1:59 PM UTC
Commit: ef40a65 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:05 PM UTC · Completed 4:20 PM UTC
Commit: 6c79fe0 · View workflow run →

[fullsend-ai-review]

[low] error-message-change

The error message changed from 'after 3 attempts: ' to 'after retries: ', losing the attempt count from the diagnostic message. No downstream code or test depends on this exact text, and the underlying error is still properly wrapped via %w, so errors.Is/errors.As continue to work. This is a minor loss of information in error diagnostics that could be useful when debugging production failures.

Suggested fix: Consider preserving the attempt count, e.g. fmt.Errorf("after 3 attempts: %w", err).