🐛 Defer health-poll reconnect until second consecutive failure - #1473
Conversation
The health poll introduced in konveyor#1468 calls reconnectSolutionServer() on the very first getServerCapabilities() failure. reconnectSolutionServer() tears down the existing MCP session, creates a new SolutionServerClient, and fires onWorkflowDisposal(false) — which is disruptive: in the minikube E2E environment the session churn causes the nginx ingress to return HTTP 421 (Misdirected Request) to other MCP clients sharing the same connection pool. Gate the reconnect behind consecutiveFailures >= 2 so transient errors (single-poll network blips) are absorbed by the existing backoff without replacing the session. Genuinely stale connections (issue konveyor#1433) produce sustained failures, so the second consecutive failure still triggers recovery — with only ~30 s additional latency, acceptable for idle- timeout scenarios. Also adds SolutionServerClient unit tests covering the new connection- state listener, error classification, and notification suppression in updateBearerToken(). Fixes the Scheduled Prerelease konveyor#215 regression (Infrastructure Tests). Signed-off-by: David Zager <david.j.zager@gmail.com>
e168000 to
ff95ef0
Compare
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Jest coverage for Estimated code review effort: 3 (Moderate) | ~25 minutes 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)
762-779: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSuccessful reconnect's
pollInterval = 10000is immediately overwritten by the unconditional backoff block below.After a successful
reconnectSolutionServer(),consecutiveFailuresis reset to0andpollIntervalis set to10000(Lines 763-764), but execution falls through into the backoffif/else if/elsechain (Lines 773-779), which re-evaluates withconsecutiveFailures === 0. That hits theelse if (consecutiveFailures < 5)branch and setspollInterval = 60000, silently undoing the reset. The next poll after a successful mid-failure reconnect will fire at ~60s instead of the intended ~10s.🐛 Proposed fix: skip the backoff recalculation after a successful reconnect
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 762 - 779, Ensure the backoff recalculation following the reconnect handling does not run after a successful reconnect. Update the control flow around reconnectSolutionServer and the consecutiveFailures/pollInterval reset so pollInterval remains 10000 for the next poll, while preserving the existing exponential backoff behavior for failed reconnect attempts.
🧹 Nitpick comments (1)
agentic/tests/solutionServerClient.test.ts (1)
41-47: 📐 Maintainability & Code Quality | 🔵 TrivialTest name doesn't match what's asserted — caching path is never exercised.
getServerCapabilities(true)always skips the cache, so this test only verifies a fresh fetch succeeds, not that cached capabilities are returned on a subsequent call. Consider asserting the cache is actually used (e.g., call once withskipCache=true, then again with default/falseand confirmlistTools/listResourcesaren't called again), or rename the test to reflect what it actually checks.♻️ Proposed fix to actually exercise the cache
- it("returns cached capabilities on success", async () => { - const { client } = buildConnectedClient(); - - const caps = await client.getServerCapabilities(true); - expect(caps.tools).toHaveLength(1); - expect(caps.resources).toHaveLength(1); - }); + it("fetches and caches capabilities, then reuses the cache on subsequent calls", async () => { + const { client, mockMcpClient } = buildConnectedClient(); + + const caps = await client.getServerCapabilities(true); + expect(caps.tools).toHaveLength(1); + expect(caps.resources).toHaveLength(1); + + await client.getServerCapabilities(false); + expect(mockMcpClient.listTools).toHaveBeenCalledTimes(1); + expect(mockMcpClient.listResources).toHaveBeenCalledTimes(1); + });🤖 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 `@agentic/tests/solutionServerClient.test.ts` around lines 41 - 47, Update the “returns cached capabilities on success” test to make an initial getServerCapabilities(true) call, then call getServerCapabilities() or getServerCapabilities(false) and verify the second call reuses cached capabilities without invoking listTools or listResources again. Keep the existing capability assertions for the cached result.
🤖 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 762-779: Ensure the backoff recalculation following the reconnect
handling does not run after a successful reconnect. Update the control flow
around reconnectSolutionServer and the consecutiveFailures/pollInterval reset so
pollInterval remains 10000 for the next poll, while preserving the existing
exponential backoff behavior for failed reconnect attempts.
---
Nitpick comments:
In `@agentic/tests/solutionServerClient.test.ts`:
- Around line 41-47: Update the “returns cached capabilities on success” test to
make an initial getServerCapabilities(true) call, then call
getServerCapabilities() or getServerCapabilities(false) and verify the second
call reuses cached capabilities without invoking listTools or listResources
again. Keep the existing capability assertions for the cached result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1e6428a-a93d-40e5-8cf5-8e3c0160bb4a
📒 Files selected for processing (3)
agentic/tests/solutionServerClient.test.tschanges/unreleased/fix-health-poll-aggressive-reconnect.yamlvscode/core/src/extension.ts
|
PR cherry-picked to branch release-0.6. Backport PR: #1476 |
#1476) ## Summary Fixes the Scheduled Prerelease #215 regression ([CI run](https://github.com/konveyor/editor-extensions/actions/runs/29006777904)) introduced by #1468. ## Problem The health poll introduced in #1468 calls `reconnectSolutionServer()` on the **very first** `getServerCapabilities()` failure. `reconnectSolutionServer()` tears down the existing MCP session, creates a new `SolutionServerClient`, and fires `onWorkflowDisposal(false)`. In the minikube E2E environment, this session churn causes the nginx ingress to return **HTTP 421 (Misdirected Request)** to other MCP clients sharing the same HTTP/2 connection pool — breaking the infrastructure tests. Two tests failed: - `hub-configuration.test.ts` — timed out waiting for "Successfully connected to Hub solution server" notification - `analysis-validation.test.ts` — `Error POSTing to endpoint (HTTP 421): Misdirected Request` from the test's independent MCP client The previous 9+ scheduled prereleases all passed; the single new commit between the last success and this failure was #1468. ## Fix Gate `reconnectSolutionServer()` behind `consecutiveFailures >= 2` so transient errors (single-poll network blips) are absorbed by the existing backoff without replacing the MCP session. - **Transient errors**: First failure backs off to 30s, no session replacement. If the next poll succeeds, the failure counter resets — no disruption. - **Genuine stale connections** (issue #1433): Produce sustained failures, so the second consecutive failure still triggers recovery — with only ~30s additional latency, acceptable for idle-timeout scenarios. The backoff schedule is now unconditional (always applied based on failure count) rather than being an `else` branch of the reconnection attempt. ## Tests Added `agentic/tests/solutionServerClient.test.ts` with 11 unit tests covering `SolutionServerClient` behaviors introduced by #1468: - `getServerCapabilities()` error classification (connection vs non-connection errors) - Connection state listener fire/suppress semantics - Stale client cleanup on connection errors - `updateBearerToken()` notification suppression ## Changes | File | What | |------|------| | `vscode/core/src/extension.ts` | Gate `reconnectSolutionServer()` behind `consecutiveFailures >= 2`; make backoff schedule unconditional | | `agentic/tests/solutionServerClient.test.ts` | New: 11 unit tests for SolutionServerClient connection state management | | `changes/unreleased/fix-health-poll-aggressive-reconnect.yaml` | Changelog fragment | <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Reduced unnecessary solution-server reconnect attempts after transient health-check failures. * The server now reconnects only after multiple consecutive failures, helping prevent disruptions to other connected clients. * Improved handling and reporting of connection-state changes during failures and token updates. * **Tests** * Added coverage for capability retrieval, connection failures, listener notifications, and token-update behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: David Zager <david.j.zager@gmail.com> Signed-off-by: Cherry Picker <noreply@github.com> Signed-off-by: David Zager <david.j.zager@gmail.com> Signed-off-by: Cherry Picker <noreply@github.com> Co-authored-by: David Zager <dzager@redhat.com>
2 participants
Summary
Fixes the Scheduled Prerelease #215 regression (CI run) introduced by #1468.
Problem
The health poll introduced in #1468 calls
reconnectSolutionServer()on the very firstgetServerCapabilities()failure.reconnectSolutionServer()tears down the existing MCP session, creates a newSolutionServerClient, and firesonWorkflowDisposal(false). In the minikube E2E environment, this session churn causes the nginx ingress to return HTTP 421 (Misdirected Request) to other MCP clients sharing the same HTTP/2 connection pool — breaking the infrastructure tests.Two tests failed:
hub-configuration.test.ts— timed out waiting for "Successfully connected to Hub solution server" notificationanalysis-validation.test.ts—Error POSTing to endpoint (HTTP 421): Misdirected Requestfrom the test's independent MCP clientThe previous 9+ scheduled prereleases all passed; the single new commit between the last success and this failure was #1468.
Fix
Gate
reconnectSolutionServer()behindconsecutiveFailures >= 2so transient errors (single-poll network blips) are absorbed by the existing backoff without replacing the MCP session.The backoff schedule is now unconditional (always applied based on failure count) rather than being an
elsebranch of the reconnection attempt.Tests
Added
agentic/tests/solutionServerClient.test.tswith 11 unit tests coveringSolutionServerClientbehaviors introduced by #1468:getServerCapabilities()error classification (connection vs non-connection errors)updateBearerToken()notification suppressionChanges
vscode/core/src/extension.tsreconnectSolutionServer()behindconsecutiveFailures >= 2; make backoff schedule unconditionalagentic/tests/solutionServerClient.test.tschanges/unreleased/fix-health-poll-aggressive-reconnect.yamlSummary by CodeRabbit
Bug Fixes
Tests