🧪 Harden solution-server test teardown and auth-manager refresh timer - #1472
🧪 Harden solution-server test teardown and auth-manager refresh timer#1472ibolton336 wants to merge 4 commits into
Conversation
The health poll's reconnectSolutionServer() fired on the very first capability-check failure, creating a new MCP transport each time. On minikube's nginx ingress this connection churn increases flakiness — the ingress can return 421 Misdirected Request to other clients connecting through the same proxy. - Delay reconnection until 2+ consecutive poll failures so a single transient blip doesn't trigger a full reconnect cycle - Guard disconnect() calls on partially-connected clients with isConnected — disconnect only makes sense when the MCP session was actually established (e.g. transport up but listTools failed) - Guard afterAll in analysis-validation.test.ts with optional chaining to prevent cascading TypeError when beforeAll fails Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
b4d8528 to
7028cf3
Compare
📝 WalkthroughWalkthroughThis PR adds a health-check workaround to the infrastructure setup action, disabling a host origin protection flag on the kai-api Deployment and polling its ingress-routed MCP JSON-RPC endpoint with retries before failing and printing deployment logs. It also adds an overflow guard to the authentication manager's auto-refresh timer scheduling to prevent excessive setTimeout delays, and makes a test's VSCode cleanup call null-safe. Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vscode/core/src/extension.ts (1)
758-775: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSuccessful reconnect's 10s poll interval is immediately overwritten to 60s.
On a successful reconnect you set
consecutiveFailures = 0andpollInterval = 10000, but execution then falls through to the backoff block. WithconsecutiveFailures === 0, theelse if (consecutiveFailures < 5)branch runs and resetspollInterval = 60000, so the recovered connection is polled every 60s instead of the intended 10s.Short-circuit after a successful reconnect so the backoff block is skipped:
🐛 Proposed fix
if (consecutiveFailures >= 2) { const reconnected = await this.state.hubConnectionManager.reconnectSolutionServer(); if (reconnected) { consecutiveFailures = 0; pollInterval = 10000; this.state.mutateServerState((draft) => { draft.solutionServerConnected = true; }); + scheduleNextPoll(withJitter(pollInterval)); + return; } }🤖 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 `@vscode/core/src/extension.ts` around lines 758 - 775, The reconnect handling in extension.ts is letting the backoff logic overwrite the intended fast poll after a successful reconnect. In the block around this.state.hubConnectionManager.reconnectSolutionServer(), short-circuit once reconnected is true so the later consecutiveFailures-based pollInterval assignment is skipped, preserving the 10000ms interval and the solutionServerConnected state update.
🤖 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.
Outside diff comments:
In `@vscode/core/src/extension.ts`:
- Around line 758-775: The reconnect handling in extension.ts is letting the
backoff logic overwrite the intended fast poll after a successful reconnect. In
the block around this.state.hubConnectionManager.reconnectSolutionServer(),
short-circuit once reconnected is true so the later consecutiveFailures-based
pollInterval assignment is skipped, preserving the 10000ms interval and the
solutionServerConnected state update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d00cb2b7-a7b6-40a5-909f-903974481c07
📒 Files selected for processing (3)
tests/e2e/tests/solution-server/analysis-validation.test.tsvscode/core/src/extension.tsvscode/core/src/hub/HubConnectionManager.ts
The infra tests fail because the kai-api MCP endpoint isn't routable through the nginx ingress when tests start — the deployment is marked available but the ingress route takes additional time to propagate. - Add a kai-api MCP endpoint health check to setup-konveyor-infrastructure that polls until the endpoint responds (non-421/502/503) - Add retry logic (3 attempts, 5s delay) to MCPClient.connect() for transient 421 Misdirected Request errors - Fix TimeoutOverflowWarning in the test's AuthenticationManager — tokens with multi-year lifespans overflowed setTimeout's 32-bit int limit Signed-off-by: ibolton336 <ibolton@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/mcp-client/mcp-client.model.ts (1)
97-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClean up partially-initialized transport before retrying.
When
connectTransport()throws on a 421,this.transportandthis.clientare already assigned. On the next retry,connectTransport()overwrites them without closing the old transport, orphaning it. Compare withreconnectWithNewToken()(line 142) which properly callsthis.transport.close()before replacing it.♻️ Proposed fix: close orphaned transport on retry
if (!is421 || attempt === MAX_RETRIES) { throw error; } + // Clean up partially-initialized transport/client from the failed attempt + if (mcpClient.transport) { + await mcpClient.transport.close().catch(() => {}); + } + mcpClient.client = undefined; + mcpClient.transport = undefined; console.warn( `MCP connect attempt ${attempt}/${MAX_RETRIES} got 421, retrying in ${RETRY_DELAY_MS}ms...` );🤖 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 `@tests/mcp-client/mcp-client.model.ts` around lines 97 - 110, The retry loop in connectTransport() leaves a partially initialized transport/client behind when a 421 triggers a retry. Before the next retry attempt, explicitly clean up the existing this.transport (and related client state if needed) so the old transport is not orphaned. Mirror the cleanup behavior already used in reconnectWithNewToken() by closing the current transport before assigning a new one, then continue with the retry delay and reconnect logic.tests/solution-server-auth/authentication-manager.ts (1)
63-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard is correct; consider aligning with the production re-evaluation approach.
The early-return guard correctly prevents
setTimeoutoverflow for oversized delays. After the return,this.refreshTimeris alreadynull(cleared on line 59), so no stale timer leaks.One difference worth noting: the production
HubConnectionManager(lines 1445–1469) schedules aMAX_TIMER_MSre-evaluation when the delay exceeds the limit, while this test utility simply returns. For test infrastructure this is acceptable since tokens with multi-year lifespans won't realistically expire during a test run, andensureAuthenticatedwill refresh on-demand when needed. If you want consistency, consider adding a log line when the guard triggers to aid debugging unexpected test behavior.Based on learnings from cross-file context in
vscode/core/src/hub/HubConnectionManager.ts:1445-1469.🤖 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 `@tests/solution-server-auth/authentication-manager.ts` around lines 63 - 68, The oversized-delay guard in authentication-manager’s refresh scheduling is fine, but it diverges from HubConnectionManager’s MAX_TIMER_MS handling. If you want parity, update the refresh path around the timer setup so delays above the limit are re-evaluated instead of only returning, or at minimum add a debug log when the guard in the refresh scheduling logic triggers to make unexpected test behavior easier to diagnose.
🤖 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.
Nitpick comments:
In `@tests/mcp-client/mcp-client.model.ts`:
- Around line 97-110: The retry loop in connectTransport() leaves a partially
initialized transport/client behind when a 421 triggers a retry. Before the next
retry attempt, explicitly clean up the existing this.transport (and related
client state if needed) so the old transport is not orphaned. Mirror the cleanup
behavior already used in reconnectWithNewToken() by closing the current
transport before assigning a new one, then continue with the retry delay and
reconnect logic.
In `@tests/solution-server-auth/authentication-manager.ts`:
- Around line 63-68: The oversized-delay guard in authentication-manager’s
refresh scheduling is fine, but it diverges from HubConnectionManager’s
MAX_TIMER_MS handling. If you want parity, update the refresh path around the
timer setup so delays above the limit are re-evaluated instead of only
returning, or at minimum add a debug log when the guard in the refresh
scheduling logic triggers to make unexpected test behavior easier to diagnose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f0a250f-85d6-4836-a69b-167b950db794
📒 Files selected for processing (6)
.github/actions/setup-konveyor-infrastructure/action.ymltests/e2e/tests/solution-server/analysis-validation.test.tstests/mcp-client/mcp-client.model.tstests/solution-server-auth/authentication-manager.tsvscode/core/src/extension.tsvscode/core/src/hub/HubConnectionManager.ts
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/tests/solution-server/analysis-validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- vscode/core/src/hub/HubConnectionManager.ts
- vscode/core/src/extension.ts
ibolton336
changed the title
Root cause found — this PR now fixes it at the sourceHeads up for reviewers: the premise of the earlier commits on this branch turned out to be wrong, so the PR has been re-scoped and retitled ( What actually broke the What this PR now does:
CI-only change, hence |
The @requires-minikube infra tests broke because the kai-solution-server :latest image resolves fastmcp from a floor (>=2.8.0) at build time and now pulls FastMCP 3.4.3+, which enables host/origin (DNS-rebinding) protection by default with an empty allowlist. Behind the tackle-hub ingress the forwarded Host header never matches, so every request to /hub/services/kai/api is rejected with 421 before reaching the MCP session layer. Tracked upstream in konveyor/kai#934. - setup action: disable FASTMCP_HTTP_HOST_ORIGIN_PROTECTION on kai-api, then gate on a real kai-api MCP readiness probe before tests run - revert the earlier extension.ts / HubConnectionManager health-poll and disconnect changes: they were based on a "reconnect churn destabilizes the proxy" theory that the 421 root cause disproves, and the poll debounce needlessly delayed konveyor#1468's stale-connection auto-recovery - drop the now-moot 421 retry from the test MCP client This is a CI/test-stability change only; no shipped extension behavior changes, hence the 🌱 prefix and no changelog fragment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
5a0585f to
5b670dc
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/actions/setup-konveyor-infrastructure/action.yml (1)
342-360: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
|| echo "000"produces"000000"on curl failure, bypassing the readiness gate.When curl fails (connection refused, timeout, DNS failure),
%{http_code}already outputs"000"to stdout and curl returns non-zero. The|| echo "000"then appends a second"000", soKAI_STATUSbecomes"000000". This value matches none of the denylist entries (421/502/503/000), so the loop breaks on the first iteration and reports the endpoint as ready — defeating the entire readiness gate for exactly the failure modes it's meant to catch.Replace
|| echo "000"with|| true; curl's-w "%{http_code}"already emits"000"on failure, so the denylist check works correctly without the duplicate output.🐛 Proposed fix
for i in $(seq 1 30); do KAI_STATUS=$(curl -k -sS --connect-timeout 5 --max-time 15 \ -o /dev/null -w "%{http_code}" \ -X POST "${HUB_URL}/hub/services/kai/api" \ -H "Authorization: Bearer ${APIKEY}" \ -H "Content-Type: application/json" \ -d '{"jsonrpc":"2.0","method":"initialize","id":0,"params":{"protocolVersion":"2025-03-26","capabilities":{},"clientInfo":{"name":"health-check","version":"0.0.0"}}}' \ - || echo "000") + || true)🤖 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 @.github/actions/setup-konveyor-infrastructure/action.yml around lines 342 - 360, The readiness check in the kai-api probe is appending a duplicate failure code because `KAI_STATUS` is assigned from the `curl` command in the health-check loop. Remove the `|| echo "000"` fallback and let `curl -w "%{http_code}"` provide the `"000"` status on failure, so the existing denylist check against `KAI_STATUS` correctly keeps retrying instead of breaking early.
🤖 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.
Outside diff comments:
In @.github/actions/setup-konveyor-infrastructure/action.yml:
- Around line 342-360: The readiness check in the kai-api probe is appending a
duplicate failure code because `KAI_STATUS` is assigned from the `curl` command
in the health-check loop. Remove the `|| echo "000"` fallback and let `curl -w
"%{http_code}"` provide the `"000"` status on failure, so the existing denylist
check against `KAI_STATUS` correctly keeps retrying instead of breaking early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dfe41051-decf-471a-92e0-c4c84d1a6f9f
📒 Files selected for processing (1)
.github/actions/setup-konveyor-infrastructure/action.yml
The FASTMCP_HTTP_HOST_ORIGIN_PROTECTION=false mitigation on the kai-api deployment does not hold: the deployment is operator-managed, so the konveyor operator reconciles the env var back to the CR spec, and the readiness probe gave a false green (it omits the MCP streamable Accept header, so it never exercised the real 421 path). The real cause is that the kai-solution-server image installs from the pyproject floor (fastmcp>=2.8.0) instead of its own lockfile, so it drifted onto fastmcp 3.4.3 whose HostOriginGuardMiddleware defaults on and 421s every proxied request. Fixed upstream in konveyor/kai#934. This leaves only the genuine, root-cause-independent test hardening on this branch (afterAll teardown guard + auth-manager timer overflow). Signed-off-by: ibolton336 <ibolton@redhat.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ibolton336
changed the title
Re-scoped: the CI workaround was reverted — the fix belongs in kaiThe
Confirmed root cause: the What's left on this PR is only root-cause-independent test hardening, hence the retitle to
|
2 participants
Summary
reconnectSolutionServer()until 2+ consecutive failures — a single transient blip no longer triggers a full reconnect cycle that creates new MCP transports through the proxydisconnect()calls on partially-connected clients withisConnectedcheck — disconnect is only meaningful when the MCP session was actually establishedafterAllinanalysis-validation.test.tswith optional chaining to prevent cascadingTypeErrorwhenbeforeAllfailsContext
After #1468 merged,
getServerCapabilities()now throws on connection errors (previously returned empty capabilities). This causes the health poll to callreconnectSolutionServer()on every failure — each call creates a new MCP transport connection through the nginx ingress. On minikube, this connection churn destabilizes the ingress, causing HTTP 421 Misdirected Request errors for other clients.Test plan
afterAllguard prevents confusing secondaryTypeErrorwhenbeforeAllfails🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores