chore: simplify, modernize (Go 1.26), update deps#128
Conversation
- fix: /health returned 200 during shutdown (should be unavailableStatusCode, same as /ready) - refactor: atomic.Pointer[bool] → atomic.Bool; drop new(bool) stores and pointer derefs - fix: named-plugin paths used svc from the ok-check then re-looked up via map index — now use svc directly - modernize: for-index loops over plg slice → range-over-value (Go 1.22) - fix: jobs handler now threads r.Context() into JobsState instead of context.Background() - modernize: report slice pre-sized to registry len instead of fixed cap 2
|
Warning Review limit reached
More reviews will be available in 25 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the status service HTTP handlers and plugin shutdown signaling, aiming to reduce allocations, improve correctness during shutdown, and streamline handler logic.
Changes:
- Replace
atomic.Pointer[bool]shutdown flag usage withatomic.Boolacross plugin and handlers. - Align
/healthshutdown behavior with configuredUnavailableStatusCode, and simplify handler loops / reduce redundant map lookups. - Improve handler efficiency and behavior (pre-size report slices, range-over-values, and propagate request cancellation to jobs checks via
r.Context()).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
plugin.go |
Switch shutdown flag storage to atomic.Bool and update handler wiring; adjust shutdown semantics. |
health.go |
Use UnavailableStatusCode on shutdown; pre-size report slice; simplify named-plugin loop and avoid redundant lookups. |
ready.go |
Pre-size report slice; simplify named-plugin loop and avoid redundant lookups; shutdown flag now atomic.Bool. |
jobs.go |
Use request context for JobsState; simplify report building loop; shutdown flag now atomic.Bool. |
handler_test.go |
Update shutdown helper for atomic.Bool and adjust shutdown expectation for /health. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Shutdown path in Ready and Jobs handlers was hard-coding 503 instead of respecting the handler's configured unavailableStatusCode field, making the setting ineffective on graceful shutdown. Update the Stop() comment to match.
Applied fixes
R/High — health.go:27
/healthreturned200during shutdown instead ofunavailableStatusCode. Fixed to match/readybehaviour — both now returnunavailableStatusCode(503 by default) when shutdown is in progress.M/Med — plugin.go:68
atomic.Pointer[bool]→atomic.Bool. Removesnew(bool)allocations and pointer dereferences inInit,Stop, and all three handler constructors (NewHealthHandler,NewReadyHandler,NewJobsHandler). Handler structs updated accordingly; test helpernewShutdownPtrupdated to match.R/Med — health.go:111 / ready.go:114
Named-plugin path looked up
svcto checkok, then discarded it and re-looked up the same key to call.Status()/.Ready(). Now uses the already-boundsvcdirectly.M/Low — health.go:111 / ready.go:114 / jobs.go:47
for i := range plg { … plg[i] … }→for _, name := range plg(Go 1.22 range-over-value).R/Med — jobs.go:38
JobsState(context.Background())→JobsState(r.Context())so cancellation propagates from the HTTP request.Minor
Report slices pre-sized to
len(registry)instead of a fixedcap 2.Not applied (needs manual review)
json.Marshal+w.Write. The asymmetric early-return and the mixed WriteHeader/Write ordering need a more invasive refactor (response buffer or tracking whether header was sent). Left for a separate pass.CheckTimeout int→time.Duration. Breaks the existingmapstructurefield (users would need to change their config format); intentionally skipped.Deps
go get -u all && go mod tidyon root andtests/;go work syncon the workspace. No version changes were needed (all deps already current).Verification