Repository navigation
fix(mcpx): stop the shared connection pool from canceling the session DELETE (#290) - #5019
Conversation
… DELETE (#290) Root cause: every HTTPTransport built its request client as &http.Client{Timeout} with a nil Transport, so all transports shared http.DefaultTransport — one process-wide connection pool. Abandon() calls CloseIdleConnections on that pool, reaching every other transport, and a pooled connection torn down by another request's cancellation (an abandoned GET stream, a pool call canceled on release) could be handed to the session-termination DELETE, which net/http then failed with context.Canceled before writing anything: the orphaned session saw 0 DELETEs (1-4 flakes per 100 runs). Fix: each transport now owns cloned connection pools — one for requests, a second for the standalone GET stream — and closeSession reissues the DELETE once when the transport cancels it before it is written while the request's own context is still alive (DELETE is idempotent; a persistent cancel or a server status is reported, never retried). TerminateHTTPSession rides the same NewHTTP + closeSession path, so the daemon takeover sibling is fixed by the same change. Tests: pool TestTerminateOrphanedSessionEndsTheUpstreamSession -count=200 green 5 consecutive runs, -count=50 -race green, daemon TestTakeoverTerminatesARemovedServersUpstreamSession -count=20 green, full internal/pool + internal/daemon + internal/mcpclient suites green, plus new unit tests pinning pool ownership and the reissue semantics in session_delete_test.go.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 41 minutes. View limit details
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. CodeRabbit Verify
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
agent-note:v1 — Codex findings disposition, task 7703, lane code-b The Codex code review on this PR (commit bc2366e) FAILED to run: "You have reached your Codex usage limits for code reviews" (codex-review-state: status=failed). It produced zero findings — no inline review comments exist on this PR — so there is nothing to fix or rebut. Recorded here so the merge does not silently pass an undispositioned finding: the finding set is empty because the review never executed, an infra limit, not a review outcome. Merging under the standing criteria on the strength of the laptop test gates in the PR body. |
Fixes #290. Queue task 7703 (lane code-b, resumed from the expired attempt-1 worktree — same branch/worktree, not restarted).
Root cause
Every
HTTPTransportbuilt its request client as&http.Client{Timeout: …}with a nilTransport, so all transports sharedhttp.DefaultTransport— one process-wide connection pool for every mcpx upstream and stream.Abandon()callsCloseIdleConnections()on that pool, which reaches every other transport in the process; and a pooled connection torn down by another request's cancellation (an abandoned GET stream, a pool call canceled on release) could be handed to the session-termination DELETE. net/http then failed that DELETE withcontext.Canceledbefore writing anything — the orphaned session saw 0 DELETEs, flakingTestTerminateOrphanedSessionEndsTheUpstreamSession1–4 times per 100 runs.Fix (
pkgs/mcpx/internal/mcpclient/http.go)newHTTPClients). No transport shareshttp.DefaultTransport, and a transport's stream connection is never one a request queues on.closeSessionreissues the DELETE once when the transport cancels it before it is written while the request's own context is still alive. DELETE is idempotent and nothing reached the upstream, so the reissue is safe; a persistent cancel or a server status is reported, never retried.TerminateHTTPSessionrides the sameNewHTTP+closeSessionpath, so the daemon takeover sibling (TestTakeoverTerminatesARemovedServersUpstreamSession) is fixed by the same change — checked explicitly per the task acceptance, not just cleared.Tests (laptop, Go 1.26.8; rebased onto origin/main 710652d and re-verified)
go test ./internal/pool/ -run TestTerminateOrphanedSessionEndsTheUpstreamSession -count=200— green 5 consecutive runs (pre-rebase base), green again post-rebase… -count=50 -race— greengo test ./internal/daemon/ -run TestTakeoverTerminatesARemovedServersUpstreamSession -count=20— greengo test ./internal/pool/ ./internal/daemon/ ./internal/mcpclient/ -count=1— all greensession_delete_test.gopin pool ownership (no sharedhttp.DefaultTransport, request/stream pools distinct) and the reissue semantics (reissue after a transport cancel; persistent cancel reported after exactly 2 attempts; server 500 never reissued).Cross-repo note: mcpx also lives in dezren39/nix — this fix is in the client transport code owned by this repo's pkgs/mcpx; the triage (task 7575) reproduced it here and the packet scoped the fix here.
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.