mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-09 18:08:31 -10:00
Pujitha PaladuguandPujitha Paladugu
176f5a242b
* fix(oauth): force-close loopback callback connections past the shutdown deadline Motivation: LoopbackListener.Close shuts the callback server down with a 1-second context and discards the result. http.Server.Shutdown only closes idle connections, so a client that sends a partial request header and never finishes it counts as active; Shutdown blocks out the full deadline, times out, and Close returns while that connection and its Serve goroutine remain open indefinitely. This is a loopback-only (127.0.0.1), short-lived, single-use server, so the impact is a resource leak past the end of the flow, not a remote vulnerability or crash. Approach: - add a ReadHeaderTimeout to the server so a stalled request header is bounded on its own; - follow a timed-out or otherwise failed Shutdown with server.Close() to forcibly tear down any connection still open past the deadline; - join the Serve goroutine (via a done channel) before Close returns, so its completion is observable instead of left dangling. internal/mcp/oauth.go and internal/provideroauth/openrouter.go build their own loopback servers with the same pattern; they are left untouched here to keep this change scoped to the file the issue's reproduction demonstrates. Validation: - go build ./internal/oauth/... and go vet ./internal/oauth/...: clean. - gofmt -l internal/oauth/: clean (make fmt-check clean repo-wide). - go test ./internal/oauth/... -race -v -count=1: all pass, including three new tests (TestLoopbackCloseForceClosesSlowHeaderConnection, TestLoopbackCloseIsIdempotent, TestLoopbackCloseReturnsPromptlyAfterSuccessfulCallback). - Confirmed TestLoopbackCloseForceClosesSlowHeaderConnection fails against the pre-fix loopback.go (with the new test kept) and passes with the fix applied. - Full repo go build ./..., go vet ./..., and go test ./... pass. - Could not run make lint-static (golangci-lint) locally: it needs a very large dependency download and the local environment ran out of disk space fetching it. That check is continue-on-error (advisory) in .github/workflows/ci.yml, not a required gate. Report: https://github.com/Gitlawb/zero/issues/1028 Signed-off-by: Pujitha Paladugu <[email protected]> Assisted-by: claude-sonnet-5 (via Claude Code) * test(oauth): synchronize loopback regression tests instead of sleeping TestLoopbackCloseForceClosesSlowHeaderConnection relied on a fixed 100ms sleep to let Serve accept a connection before sending a stalled header; under load this could fail or silently skip the force-close path it's meant to exercise. Use an http.Server.ConnState hook (via a new unexported newLoopbackListener, set before Serve starts to avoid a race) to wait for the real StateNew signal instead. TestLoopbackCloseReturnsPromptlyAfterSuccessfulCallback measured Close's elapsed time without joining its http.Get goroutine or draining/closing the response body; join it first so the timing starts from a connection that's actually back to idle. Also register t.Cleanup(l.Close) in both tests so a Fatalf between construction and the existing explicit Close doesn't leak the listener's Serve goroutine. Signed-off-by: Pujitha Paladugu <[email protected]> --------- Signed-off-by: Pujitha Paladugu <[email protected]> Co-authored-by: Pujitha Paladugu <[email protected]>