fix: include WebSocket subsystem health in /health endpoint - #103
Merged
dcccrypto merged 2 commits intoApr 1, 2026
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe /health endpoint now includes a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HealthRoute
participant RPC
participant Database
participant WebSocketModule
Client->>HealthRoute: GET /health
HealthRoute->>RPC: check RPC connectivity
RPC-->>HealthRoute: rpcStatus (ok / fail)
HealthRoute->>Database: check DB connectivity
Database-->>HealthRoute: dbStatus (ok / fail)
HealthRoute->>WebSocketModule: getWebSocketMetrics()
WebSocketModule-->>HealthRoute: { totalConnections, maxGlobalConnections } / error
HealthRoute->>HealthRoute: compute wsHealthy = total/max < 0.95 (or false on error)
HealthRoute-->>Client: 200/503 { checks: { rpc, db, ws }, status }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/routes/health.ts (1)
42-43: WS metrics failure is swallowed without diagnostics.The empty catch sets
checks.ws = falsebut drops the error context. Add a log entry for operational visibility.Suggested improvement
- } catch { + } catch (err) { + logger.error("WS check failed", { error: truncateErrorMessage(err instanceof Error ? err.message : err, 120) }); checks.ws = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/health.ts` around lines 42 - 43, Replace the empty catch with a named error parameter and log the error before setting checks.ws = false; specifically change the catch to catch (err) { /* log */; checks.ws = false } and use the service's logger (e.g. processLogger.error or logger.error) if available, otherwise fallback to console.error, with a clear message like "health check ws failed" plus the err to preserve diagnostics for checks.ws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/health.ts`:
- Line 41: The health check treats exactly 95% as degraded because it uses
checks.ws = utilization < 0.95; change the comparison so 95% is considered
healthy and only >95% is degraded by using utilization <= 0.95 when assigning
checks.ws (locate the assignment to checks.ws and the utilization variable in
src/routes/health.ts).
- Around line 37-44: The health check flips checks.ws to false when WebSocket
saturation is detected but the existing status-to-HTTP-code mapping still
returns 200 for a "degraded" overall status, preventing LBs from draining;
update the status computation or the HTTP status mapping so that a failed
WebSocket check (checks.ws === false) results in a non-200 response: either (A)
treat any false entry in checks (including checks.ws) as an overall "fail"
instead of "degraded" when computing status, or (B) change the
status-to-statusCode mapping to return 503 for "degraded". Modify the code paths
that compute status (the logic that reads checks and produces status) and the
code that maps status to the HTTP response code so
getWebSocketMetrics()/utilization and checks.ws being false will produce a 503
instead of 200.
---
Nitpick comments:
In `@src/routes/health.ts`:
- Around line 42-43: Replace the empty catch with a named error parameter and
log the error before setting checks.ws = false; specifically change the catch to
catch (err) { /* log */; checks.ws = false } and use the service's logger (e.g.
processLogger.error or logger.error) if available, otherwise fallback to
console.error, with a clear message like "health check ws failed" plus the err
to preserve diagnostics for checks.ws.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+37
to
+44
| // Check WebSocket subsystem — saturated WS means new clients can't connect | ||
| try { | ||
| const wsMetrics = getWebSocketMetrics(); | ||
| const utilization = wsMetrics.totalConnections / wsMetrics.limits.maxGlobalConnections; | ||
| checks.ws = utilization < 0.95; // degraded if >95% of connection slots used | ||
| } catch { | ||
| checks.ws = false; | ||
| } |
There was a problem hiding this comment.
WS saturation will still return HTTP 200 with current status-code mapping.
This block correctly flips checks.ws to false, but with db/rpc healthy that produces status = "degraded", and Line 57 currently returns 200 for degraded. That prevents LB drain behavior described in the PR objective.
Suggested fix
- const statusCode = status === "down" ? 503 : 200;
+ const statusCode = status === "ok" ? 200 : 503;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/health.ts` around lines 37 - 44, The health check flips checks.ws
to false when WebSocket saturation is detected but the existing
status-to-HTTP-code mapping still returns 200 for a "degraded" overall status,
preventing LBs from draining; update the status computation or the HTTP status
mapping so that a failed WebSocket check (checks.ws === false) results in a
non-200 response: either (A) treat any false entry in checks (including
checks.ws) as an overall "fail" instead of "degraded" when computing status, or
(B) change the status-to-statusCode mapping to return 503 for "degraded". Modify
the code paths that compute status (the logic that reads checks and produces
status) and the code that maps status to the HTTP response code so
getWebSocketMetrics()/utilization and checks.ws being false will produce a 503
instead of 200.
| try { | ||
| const wsMetrics = getWebSocketMetrics(); | ||
| const utilization = wsMetrics.totalConnections / wsMetrics.limits.maxGlobalConnections; | ||
| checks.ws = utilization < 0.95; // degraded if >95% of connection slots used |
There was a problem hiding this comment.
Boundary condition does not match “exceeds 95%” requirement.
At Line 41, utilization < 0.95 marks exactly 95% as degraded. If degradation should happen only when utilization exceeds 95%, this should be <= 0.95.
Suggested fix
- checks.ws = utilization < 0.95; // degraded if >95% of connection slots used
+ checks.ws = utilization <= 0.95; // degraded if >95% of connection slots used📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| checks.ws = utilization < 0.95; // degraded if >95% of connection slots used | |
| checks.ws = utilization <= 0.95; // degraded if >95% of connection slots used |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/health.ts` at line 41, The health check treats exactly 95% as
degraded because it uses checks.ws = utilization < 0.95; change the comparison
so 95% is considered healthy and only >95% is degraded by using utilization <=
0.95 when assigning checks.ws (locate the assignment to checks.ws and the
utilization variable in src/routes/health.ts).
Owner
|
Re-opening to retrigger CI after 503 fix commit. |
dcccrypto
approved these changes
Apr 1, 2026
dcccrypto
left a comment
dcccrypto
left a comment
Owner
There was a problem hiding this comment.
SECURITY SIGN-OFF ✅ (0x-SquidSol batch review)
Owner
|
fix(test): Added vitest deps.inline for @percolator/shared to resolve ESM mock isolation flake in CI (Node 22 ubuntu-latest). Re-running CI. |
The health endpoint checks Solana RPC and Supabase but not the WebSocket subsystem. If WebSocket connections are at 95%+ capacity (near MAX_WS_CONNECTIONS), new clients cannot connect but the health endpoint still reports 'ok'. Load balancers and monitoring tools have no signal to redirect traffic or trigger alerts. Add a ws health check that reports degraded when connection utilization exceeds 95%. Made-with: Cursor
PR adds a ws check to the health endpoint (3 checks: rpc, db, ws). Tests only mocked rpc/db but not ws, so the 'down' status test failed because ws was healthy (2/3 fail = degraded, not down). - Mock ../../src/routes/ws.js with default healthy metrics - Update 'all fail' test to also fail ws check - Add ws assertion to 'ok' test
dcccrypto
force-pushed
the
fix/add-ws-health-to-health-endpoint
branch
from
April 1, 2026 06:05
f9d26dc to
374bd8a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/healthendpoint checks Solana RPC and Supabase connectivity but ignores the WebSocket subsystemMAX_WS_CONNECTIONS), new WS clients cannot connect"ok", so load balancers don't redirect traffic and monitoring tools don't alertFix
wscheck to the health endpoint that readsgetWebSocketMetrics()ws: false(degraded) when connection utilization exceeds 95% ofmaxGlobalConnectionsFiles changed
src/routes/health.ts— add WS subsystem health checkTest plan
checks.ws: true, status "ok"checks.ws: false, status "degraded", HTTP 503checks.ws: false, status "degraded"Summary by CodeRabbit