Skip to content

fix: correct mirror health check probes#4

Merged
wha7ev9r merged 2 commits into
mainfrom
fix/mirror-health-checks
Jun 8, 2026
Merged

fix: correct mirror health check probes#4
wha7ev9r merged 2 commits into
mainfrom
fix/mirror-health-checks

Conversation

@wha7ev9r

@wha7ev9r wha7ev9r commented Jun 8, 2026

Copy link
Copy Markdown
Owner

Summary

Fix false unhealthy statuses for mirrors whose root or configured upstream URL is not a valid health probe:

  • Cargo now probes a known crate artifact instead of the S3 directory root, which returns 403.
  • NuGet now probes the configured index URL without appending a trailing slash.
  • Homebrew now probes the GHCR registry base /v2/ and treats 401 as healthy, matching registry behavior.
  • Added regression tests for all three health probe paths.

Verification

  • go test targeted mirror health checks
  • go test ./...
  • git diff --check

Summary by CodeRabbit

  • Bug Fixes

    • Improved health check accuracy for Cargo, Homebrew, and NuGet mirrors:
      • Cargo now probes a specific crate artifact endpoint.
      • Homebrew normalizes registry base checks and treats 200 and 401 as healthy.
      • NuGet no longer force-appends a trailing slash when probing upstream.
  • Tests

    • Added unit tests verifying exact upstream URL paths used by each mirror type.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9efdd90a-eef7-4706-9b5a-242430a75e20

📥 Commits

Reviewing files that changed from the base of the PR and between 0d5e71d and 872ade9.

📒 Files selected for processing (2)
  • internal/mirror/homebrew.go
  • internal/mirror/mirror_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/mirror/mirror_test.go
  • internal/mirror/homebrew.go

📝 Walkthrough

Walkthrough

Three package mirrors (Cargo, NuGet, Homebrew) update how they construct health-check URLs and which HTTP statuses are treated as healthy; unit tests assert each mirror’s exact request path and response handling.

Changes

Mirror Health Check Updates

Layer / File(s) Summary
Cargo mirror crate file health check
internal/mirror/cargo.go, internal/mirror/mirror_test.go
Cargo HealthCheck probes a specific serde crate artifact file (/serde/serde-1.0.0.crate) instead of the upstream root; test asserts the crate file path is requested and 200 is handled as healthy.
NuGet mirror trailing slash normalization
internal/mirror/nuget.go, internal/mirror/mirror_test.go
NuGet HealthCheck trims trailing slashes from the upstream URL instead of appending one; test verifies the exact endpoint /v3/index.json (no trailing slash) and 200 handling.
Homebrew mirror /v2/ registry endpoint and 401 acceptance
internal/mirror/homebrew.go, internal/mirror/mirror_test.go
Homebrew HealthCheck normalizes upstream to the /v2/ registry base path (handling /v2, /v2/, and deeper paths) and treats HTTP 200 and 401 as healthy; tests verify normalized /v2/ requests and 401 acceptance.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through mirrors, peeking where they tread,
Cargo fetched a crate, straight to its head,
NuGet trimmed the tail that tried to stay,
Homebrew waved at /v2/ and let 401 play,
Small hops, safe checks—now off for my bed.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: correct mirror health check probes' accurately describes the main change: correcting how mirror health checks probe upstream registries across Cargo, NuGet, and Homebrew mirrors.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/mirror-health-checks

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
internal/mirror/mirror_test.go (1)

169-189: ⚡ Quick win

Add a /v2 (no trailing slash) normalization test.

Please add a case where upstream is set to .../v2 and assert health check still probes exactly /v2/ (not /v2/v2/). This would have caught the current normalization edge case in internal/mirror/homebrew.go Line 97.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/mirror/mirror_test.go` around lines 169 - 189, Add a new unit test
(e.g., TestHomebrewHealthCheckNormalizesRegistryBaseNoSlash) that mirrors
TestHomebrewHealthCheckUsesRegistryBase but calls m.SetUpstream with an upstream
URL ending in "/v2" (no trailing slash) and verifies HealthCheck still requests
exactly "/v2/"; use the same httptest.Server pattern to capture gotPath, return
401 for "/v2/", and fail the test if gotPath != "/v2/". Reference
HomebrewMirror.SetUpstream and HomebrewMirror.HealthCheck to ensure the upstream
normalization behavior is exercised and asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/mirror/homebrew.go`:
- Around line 94-98: The upstream "/v2" suffix normalization currently can
produce "/v2/v2/" when upstream ends with "/v2" (no trailing slash); update the
logic that mutates the upstream variable so it explicitly checks for the exact
suffix "/v2" and for "/v2/" before appending – if strings.HasSuffix(upstream,
"/v2/") leave as-is, if strings.HasSuffix(upstream, "/v2") append a trailing "/"
(making "/v2/"), otherwise trim trailing slashes and append "/v2/"; apply this
change where upstream is reassigned in internal/mirror/homebrew.go so the
variable normalization no longer produces duplicate "/v2/v2/".

---

Nitpick comments:
In `@internal/mirror/mirror_test.go`:
- Around line 169-189: Add a new unit test (e.g.,
TestHomebrewHealthCheckNormalizesRegistryBaseNoSlash) that mirrors
TestHomebrewHealthCheckUsesRegistryBase but calls m.SetUpstream with an upstream
URL ending in "/v2" (no trailing slash) and verifies HealthCheck still requests
exactly "/v2/"; use the same httptest.Server pattern to capture gotPath, return
401 for "/v2/", and fail the test if gotPath != "/v2/". Reference
HomebrewMirror.SetUpstream and HomebrewMirror.HealthCheck to ensure the upstream
normalization behavior is exercised and asserted.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 732bf908-2f53-4b1f-843f-c10bda9cab0b

📥 Commits

Reviewing files that changed from the base of the PR and between 96c530d and 0d5e71d.

📒 Files selected for processing (4)
  • internal/mirror/cargo.go
  • internal/mirror/homebrew.go
  • internal/mirror/mirror_test.go
  • internal/mirror/nuget.go

Comment thread internal/mirror/homebrew.go Outdated
@wha7ev9r
wha7ev9r merged commit c86659a into main Jun 8, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant