🐛 bug: answer healthcheck HEAD probes with the GET status - #4577
Conversation
The handler bailed out to c.Next() for anything but GET, so the HEAD route that v3 auto-registers for every GET route ran the probe handler and then fell through to 404. Every other GET-oriented middleware (static, favicon, envvar) already accepts HEAD; fasthttp drops the body on the wire, so the GET path can serve it unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe healthcheck middleware now handles both GET and HEAD requests. HEAD responses match GET status codes and selected headers while omitting the response body. Tests cover method fallthrough, and documentation describes the updated behavior. ChangesHealthcheck HEAD support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4577 +/- ##
==========================================
- Coverage 93.29% 93.28% -0.01%
==========================================
Files 140 140
Lines 14858 14858
==========================================
- Hits 13862 13861 -1
- Misses 620 622 +2
+ Partials 376 375 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes the healthcheck middleware so HEAD requests follow the same probe path as GET, returning the same status codes (200/503) while producing an empty response body, aligning behavior with RFC 9110 §9.3.2 and Fiber v3’s auto-registered HEAD routes.
Changes:
- Allow
HEADrequests through the healthcheck probe handler (previously onlyGETwas handled;HEADfell through toc.Next()). - Add a regression test covering
HEADresponses for healthy/unhealthy probes and asserting an empty body. - Update healthcheck documentation to reflect
GET+HEADbehavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| middleware/healthcheck/healthcheck.go | Extends the method guard to handle HEAD the same as GET. |
| middleware/healthcheck/healthcheck_test.go | Adds Test_HealthCheck_Head to validate status mirroring and empty body semantics. |
| docs/middleware/healthcheck.md | Updates docs to state the middleware responds to GET and HEAD. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@middleware/healthcheck/healthcheck_test.go`:
- Around line 166-193: Extend Test_HealthCheck_Head to issue a GET request for
each endpoint and use it as the reference response. Assert that the HEAD
response matches the corresponding GET status and headers while retaining the
existing empty-body assertion; keep the liveness and readiness cases unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9eb43aab-a072-44b6-ba4d-90fc9b674f1b
📒 Files selected for processing (3)
docs/middleware/healthcheck.mdmiddleware/healthcheck/healthcheck.gomiddleware/healthcheck/healthcheck_test.go
Compare Content-Type and Content-Length against a real GET response instead of only checking the HEAD status. Date stays out of the comparison, fasthttp refreshes its cached value once per second, so a full header-map compare across two requests would flake. The POST case also restores coverage for the c.Next() fallthrough: with a GET-only registration the router answers 405 before the middleware runs, so codecov saw the changed line as unhit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
…not share a CPU A regression gate is only meaningful when both sides saw the same hardware. Two cases break that and both went through silently. A sharded run can span several CPU models. The cache key carries the merged model string, so such a run lands in its own baseline lineage, and that lineage is only refreshed when a default branch run happens to be mixed the same way. Meanwhile the restore-key is an unbounded prefix fallback, so it takes the newest entry in that lineage no matter how old. Seen on gofiber/fiber#4577: one of six shards ran on Neoverse-N1 while the rest ran on Ampere-1a, and the comparison fell back to a baseline 12 commits behind the PR base. It reported 11 regressions of 1.50x to 2.72x. Three were the Neoverse shard measured against Ampere numbers, Neoverse-N1 being 1.5x to 2.3x slower here. The other eight were byte for byte what the default branch measures today and only looked worse because the baseline was old. Skip the comparison when the run spanned more than one model, and when a pages-cpu-model pin is configured and the run did not meet it. A skipped comparison is a visible warning; a comparison across machines is a wrong number presented as a fact. Also stop seeding the cache from such a run, otherwise the default branch keeps creating exactly the stale lineages this guard exists to avoid.
Description
healthcheck.New()returnedc.Next()for every method except GET, so aHEAD /livezended up in the 404 handler whileGET /livezanswered 200. Since v3 auto-registers a HEAD twin for every GET route (DisableHeadAutoRegister), the HEAD request does reach the probe handler and the guard is what breaks it. RFC 9110 9.3.2 wants HEAD to answer with the GET status and no body, and fasthttp already drops the body on the wire, so the existing GET path can serve HEAD unchanged.healthcheck was also the only outlier here:
static,faviconandenvvarall accept GET and HEAD.Measured on
mainbefore the fix:app.Get(healthcheck.LivenessEndpoint, healthcheck.New())app.All("/livez", healthcheck.New())app.Get("/livez", plainHandler)Fixes #4574
Changes introduced
middleware/healthcheck: let HEAD through the same probe path as GET; other methods still fall through to the next handler.Test_HealthCheck_Headasserts 200 for a healthy probe, 503 for an unhealthy one, and an empty body in both cases.docs/middleware/healthcheck.mdno longer claims the middleware responds only to GET.Type of change