Repository navigation
mcpx: TestTerminateOrphanedSessionEndsTheUpstreamSession flakes in the Nix build (0 DELETEs) and hides its own failure reason #290
Description
Finding (from PR diff review of #267)
TestTerminateOrphanedSessionEndsTheUpstreamSession — added by PR #267 in pkgs/mcpx/internal/pool/upstream_session_test.go — has failed intermittently in the Nix build gate, on a PR that did not touch it.
Evidence
- PR mcpx: make <command> --help print the command's help #275 (head
a72a2fa79) CI run 39121884524, jobbuild: the mcpx Nix derivation failed with:
--- FAIL: TestTerminateOrphanedSessionEndsTheUpstreamSession (0.00s)/upstream_session_test.go:185: the orphaned session saw 0 DELETE(s), want exactly 1/FAIL github.com/dezren39/mcpx/internal/pool 18.974s. - The same run's host
go teststep passed all 3370 tests, and PR mcpx: DELETE a removed server's upstream session on takeover (#260) #267's own build, plus the builds for PRs fix(mcpx): refuse recipe @param holes in mcpx run, point at recipes run (#276) #277 and mcpx daemons: prune dead records, find daemons in other state dirs #279 (which contain the same test), were green — so this is intermittent and environment/load-specific (Nix sandbox), not a deterministic break. - Verified on current main (
60f7a634): the test still exists unchanged atpkgs/mcpx/internal/pool/upstream_session_test.go:170. Locally it passes 100/100 (go test ./internal/pool/ -run TestTerminateOrphanedSessionEndsTheUpstreamSession -count=100) and 20/20 with its sibling test, so the failure does not reproduce in a plain dev environment. - Diagnosability gap in the test itself: when the count check at line ~185 fails, the test never surfaces why termination failed.
pool.TerminateOrphanedSessioncarefully reports failures via theupstream.session.orphanedlifecycle event (with server, session and reason), but the test only capturesupstream.session.terminatedevents and checks the DELETE count first — the one piece of evidence that would identify the cause is discarded.
Why it matters
The Nix build is a merge gate for every mcpx change. A test that flakes there makes unrelated PRs (like #275) show a red build, which trains everyone to merge past red builds — and the standing rule of merging past unrelated infra failures then also lets real regressions through. The sibling daemon-level test from the same PR (TestTakeoverTerminatesARemovedServersUpstreamSession in internal/daemon/registry_handoff_test.go) exercises the same termination path and likely shares the failure mode.
Acceptance criteria
- The failure mode is identified (e.g. capture and print the
upstream.session.orphanedevent/reason when the DELETE count is wrong; run the pool tests under the Nix build repeatedly to reproduce). - The test (and its daemon-level sibling, if it shares the mode) is deterministic under the Nix build sandbox — repeated
nix build/ check runs of the mcpx package pass without this failure. - If the root cause is in the termination path itself (not only the test), the fix lands in
TerminateOrphanedSession/TerminateHTTPSessionand the test pins it. - On failure, the test output states why termination failed (the orphaned reason), not only the DELETE count.
Metadata
Metadata
Assignees
Labels
Package/app: mcpx (pkgs/ or apps/ in this repo)Medium: expected under 1 day of workTest coverage, test infrastructure, or test-only fixesIntake review done: acceptance criteria, links, and normalized labels addedOrdinary priority (the default)Filed by the pr-diff-review jobQueue packet kind: verify, reproduce, and route this item
Activity
dezren39 commentedon Oct 19, 2026
Sweep update (2026-10-19): the diagnosability half of this issue is fixed as a set — PR #336 here + dezren39/nix PR #452 (this issue is the origin; no nix issue filed).
What landed:
sessionUpstreamnow records every DELETE attempt and its session id (it previously counted only DELETEs for live sessions, so a DELETE answered 404 was indistinguishable from no DELETE at all). The daemon package'sorphanUpstreammock already did this; the pool mock now matches it.TestTerminateOrphanedSessionEndsTheUpstreamSessioncapturesupstream.session.orphanedalongsideterminated(newlifecycleCapturehelper — the existing hook holds one kind) and its failure message reports attempts, session ids, and orphaned reasons.Verified locally: full internal/pool suite green; target test -count=200, GOMAXPROCS=1 -count=100, and -race all green. A sabotage run (upstream forgets sess-1 before termination) confirmed the modes now read differently:
attempts=1 deleted=[sess-1] deletes=0with a terminated event (404 counts as success in closeSession) vsattempts=0for a DELETE that never arrives vs an orphaned event with a reason for a DELETE that errored.The flake itself still does not reproduce outside the Nix sandbox, so this issue stays open: the next Nix-build occurrence will now name its mode, and repeated green mcpx builds are the other closing signal (acceptance criteria 2–3).
dezren39 commentedon Oct 26, 2026
Intake review
🤖 Agent note · filed
Still live (verified this run): PARTIALLY ADDRESSED, NOT RESOLVED. PR #336 (merged) fixed the diagnosability half — the test now captures
upstream.session.orphanedevents and names its failure mode — and its own body states #290 stays open until a future Nix-build flake prints its mode or repeated green builds close it. The determinism acceptance criteria are not yet met, so this issue stays open.Related: PR #267 (added the test), PR #275 (the unrelated PR whose Nix gate flaked), PR #336 (diagnosability fix, merged), dezren39/darwin-nix#366 (mcpx Go tests failing on macOS — different environment, same package family).
Notes / risks: the body already carries a complete
## Acceptance criteriasection — no body edit needed. Local reproduction has repeatedly failed (100/100 passes), so progress depends on Nix-build repetitions or the next flake's now-visible failure mode. The diagnosability fix also means the next occurrence should be far cheaper to root-cause.Suggested approach: run the mcpx package's Nix check repeatedly (e.g. a loop of
nix build/ check runs on the mcpx package) watching for this test; if it flakes, the new failure message names the mode — fix that mode in the termination path or the test and close. If a substantial run of green builds accumulates, close with that evidence.dezren39 commentedon Nov 1, 2026
🤖 Agent note · picked up
26 remaining items
dezren39 commentedon Nov 3, 2026
agent-note:v1
state: parked (CI pending) — task 7703, lane code-b
PR #5019 is open, MERGEABLE, diff = exactly the two intended files. All laptop gates green (see PR body): pool flake test -count=200 x5, -race, daemon sibling -count=20, full pool/daemon/mcpclient suites, new session_delete_test.go unit tests; rebased onto origin/main 710652d and re-verified. Codex review failed on Codex usage limits with zero findings — disposition posted on PR #5019 (comment 6609447437).
Parking per the 5-minute CI rule: the PR's one check (basic: production line limit 2500) is still pending after ~5 minutes of polling — a runner-queue stall, not a failure; the file it guards is 845 lines, verified directly. Resume condition: check green on PR #5019 → squash-merge → close issue #290 with the gate evidence → complete task 7703 as fixed_merged.
fix(mcpx): stop the shared connection pool from canceling the session…
dezren39 commentedon Nov 3, 2026
🤖 Agent note · done
awaiting_cion PR fix(mcpx): stop the shared connection pool from canceling the session DELETE (#290) #50198e151e895875483a90aa25e2460ac751b16455fb); branchqueue/7703-290-orphan-delete-cancelDone (2026-11-03)
PR #5019 merged and this issue auto-closed via its
Fixes #290.Resume verification performed this shift before merging:
bc2366e31matched the checkpoint exactly; diff is the two intended files (pkgs/mcpx/internal/mcpclient/http.go,session_delete_test.go).-count=200green 5 consecutive runs plus post-rebase,-count=50 -racegreen, daemon sibling-count=20green, fullpool/daemon/mcpclientpackages green.basic: production line limit (2500), sat QUEUED from 16:22Z in the fleet CI backlog without starting. Its criterion was verified manually instead of waiting on the backlog:http.gois 845 lines and the new test file 130 lines at the PR head, both far under the 2500 limit. Merged under the standing merge authority (verified work merges; PRs do not wait on CI). The stuck queue is the known fleet CI backlog, not a signal about this change.