Stop the AuthHandler login rate limiter on shutdown - #398
Open
dpage wants to merge 1 commit into
Open
Conversation
Found while triaging dead code. NewAuthHandler creates a rate limiter for itself, covering total login attempts per IP, and auth.NewRateLimiter starts a background cleanup goroutine. Only AuthHandler.Close stops that goroutine, and nothing called Close: SetupHandlers builds the handler inside the closure it returns and discards the reference, so neither the server nor anything else could reach it. The result is a goroutine and its map of recorded attempts living for the remaining life of the process. It is bounded rather than growing, since the handler is constructed exactly once at startup, so the practical impact is small. It is still worth fixing, for three reasons: Server.Close already stops every other background resource it owns, so this is an inconsistency rather than a deliberate choice; the Close method exists and documents precisely this ownership, so failing to call it is an oversight; and because Close appears unreachable, static analysis reports it as dead code, which is how it nearly got deleted rather than wired up. HandlerDependencies now carries a RegisterCloser callback. SetupHandlers uses it to hand the auth handler's Close back to the server, and Server.Close runs the collected closers alongside the overview generator, the shared rate limiter, the auth store and the datastore it already stopped. The callback is optional, so existing callers and tests that only exercise route registration are unaffected. Registration is mutex-guarded because SetupHandlers runs on the HTTP server's goroutine whilst Close may run from the signal handler, and runHandlerClosers clears the list so a second shutdown pass does not stop the same resource twice. Both new functions are at 100% statement coverage. Server coverage rises from 55.5% to 55.9%. The only failing tests are the two pre-existing vector(3) fixture failures from #337, which reproduce identically on unmodified main. deadcode now reports AuthHandler.Close as reachable, dropping the server's unreachable count from 65 to 64.
WalkthroughThe server now supports handler cleanup callbacks. ChangesHandler cleanup lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Server
participant SetupHandlers
participant AuthHandler
participant AuthStore
Server->>SetupHandlers: pass registerHandlerCloser
SetupHandlers->>AuthHandler: create authentication handler
SetupHandlers->>Server: register AuthHandler.Close
Server->>AuthHandler: execute cleanup during Close
Server->>AuthStore: close authentication store
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 27 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![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 `@server/src/cmd/mcp-server/server.go`:
- Around line 84-87: Protect handler-closer registration and shutdown with a
closed state guarded by closersMu. Update registerHandlerCloser so registrations
observed after shutdown execute immediately, while registrations observed before
shutdown append under the same lock before runHandlerClosers snapshots the list;
mark the state closed atomically with draining. Add a concurrency test that
overlaps registerHandlerCloser with runHandlerClosers and verifies the closer
always runs.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86d35389-2871-45ed-9f03-09a91ff574ab
📒 Files selected for processing (4)
docs/changelog.mdserver/src/cmd/mcp-server/handler_closers_test.goserver/src/cmd/mcp-server/handlers.goserver/src/cmd/mcp-server/server.go
Comment on lines
+84
to
+87
| s.closersMu.Lock() | ||
| closers := s.handlerClosers | ||
| s.handlerClosers = nil | ||
| s.closersMu.Unlock() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent registration after the shutdown drain.
A closer can register after Line 87. Server.Close then does not run that closer. This can leave an AuthHandler login rate-limiter goroutine running during shutdown.
Add a closed state under closersMu. If registerHandlerCloser observes that state, run the new closer immediately. If it does not observe that state, append the closer before runHandlerClosers takes its snapshot. Add a test that overlaps registration with runHandlerClosers.
Proposed synchronization model
type Server struct {
// ...
closersMu sync.Mutex
handlerClosers []func()
+ handlerClosersClosed bool
}
func (s *Server) registerHandlerCloser(closer func()) {
if closer == nil {
return
}
s.closersMu.Lock()
- defer s.closersMu.Unlock()
+ if !s.handlerClosersClosed {
+ s.handlerClosers = append(s.handlerClosers, closer)
+ s.closersMu.Unlock()
+ return
+ }
+ s.closersMu.Unlock()
- s.handlerClosers = append(s.handlerClosers, closer)
+ closer()
}
func (s *Server) runHandlerClosers() {
s.closersMu.Lock()
+ if s.handlerClosersClosed {
+ s.closersMu.Unlock()
+ return
+ }
+ s.handlerClosersClosed = true
closers := s.handlerClosers
s.handlerClosers = nil
s.closersMu.Unlock()🤖 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 `@server/src/cmd/mcp-server/server.go` around lines 84 - 87, Protect
handler-closer registration and shutdown with a closed state guarded by
closersMu. Update registerHandlerCloser so registrations observed after shutdown
execute immediately, while registrations observed before shutdown append under
the same lock before runHandlerClosers snapshots the list; mark the state closed
atomically with draining. Add a concurrency test that overlaps
registerHandlerCloser with runHandlerClosers and verifies the closer always
runs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while triaging the dead-code findings, and flagged in #397.
The defect
NewAuthHandlercreates a rate limiter for itself, covering total loginattempts per IP:
auth.NewRateLimiterstarts a background cleanup goroutine (go rl.cleanupLoop()). OnlyAuthHandler.Closestops it, and nothingcalled
Close:SetupHandlersconstructs the handler inside theclosure it returns and discards the reference, so neither the server nor
anything else could reach it.
The result is a goroutine plus its map of recorded attempts living for
the remaining life of the process.
Honest severity
Bounded, not growing.
SetupHandlers(deps)is called once fromserver.go:453and its closure runs once, so it is one goroutine and onesmall map, and the process exits at shutdown anyway. This is not an
unbounded leak and I don't want to oversell it.
It is still worth fixing, for three reasons:
Server.Closealready stops the overview generator, the shared ratelimiter, the auth store and the datastore. This is an inconsistency in
that pattern rather than a deliberate exemption.
Closeexists and its doc comment documents exactly this ownershipsplit, explaining that the caller-supplied
rateLimiterisdeliberately not stopped there. Not calling it is an oversight.
Closelooks unreachable, static analysis reports it as deadcode. That is how it nearly got deleted rather than wired up: it
was in the deferred set in Remove dead code identified by reachability analysis #395 and I came close to removing it in
Remove dead code that needed test edits rather than deletions #397 before checking what it did.
The fix
HandlerDependenciesgains an optionalRegisterCloser func(func()).SetupHandlersuses it to handauthHandler.Closeback to the server,and
Server.Closeruns the collected closers alongside the resources italready stops.
Registration is mutex-guarded, because
SetupHandlersruns on the HTTPserver's goroutine whilst
Closemay run from the signal handler.runHandlerClosersclears the list, so a second shutdown pass does notstop the same resource twice. The callback is optional, so existing
callers and tests that only exercise route registration are unaffected.
This also gives any future handler owning background work a place to hand
that ownership back, rather than each one inventing its own escape hatch.
Verification
and list-clearing behaviour, the no-closers case (which is what the CLI
subcommands hit, since they never start the listener), concurrent
registration under
-race, and the actual defect, thatSetupHandlersregisters a closer for the auth handler.
gofmtclean,golangci-lint0 issues,go vetclean.-race -p=1; the only failures are thetwo pre-existing
vector(3)fixture failures from Gemini provider: (1) knowledge base search silently falls back to OpenAI due to missing gemini_embedding column in search_knowledgebase.go; (2) session startup fails with 400 "empty Part" error #337, whichreproduce identically on unmodified
main.deadcodenow reportsAuthHandler.Closeas reachable, droppingthe server's unreachable count from 65 to 64. That is the useful
confirmation the wiring actually took effect.
204 insertions, no deletions.
Merge order
Cut from
main. The only file shared with #395, #396 and #397 isdocs/changelog.md, and this entry goes under### Fixedwhilst theirsgo under
### Removed, so it should merge cleanly in any order. No codefiles overlap.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation