fix: enable proper traceId propagation for websocket server#1614
fix: enable proper traceId propagation for websocket server#1614
Conversation
WalkthroughThe changes introduce OpenTelemetry tracing and enhanced logging to the WebsocketServer, update logger usage to a singleton pattern, and add a new telemetry utility module. WebSocket message and connection handling are now traced and logged consistently. Type signatures are updated to enforce stricter requirements on message and provider socket creation. Tests and server setup were updated to support TLS certificate validation and provider address verification. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebsocketServer
participant ProviderSocket
participant Telemetry
Client->>WebsocketServer: Connect (upgrade)
WebsocketServer->>Telemetry: Start tracing span (upgrade)
WebsocketServer->>ProviderSocket: Create provider socket
ProviderSocket-->>WebsocketServer: Connection established
WebsocketServer->>Telemetry: Start tracing span (message)
Client->>WebsocketServer: Send message
WebsocketServer->>WebsocketServer: Parse & validate message
WebsocketServer->>ProviderSocket: Proxy message
ProviderSocket-->>WebsocketServer: Provider response
WebsocketServer->>Client: Forward response
WebsocketServer->>Telemetry: End tracing spans
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/provider-proxy/src/utils/telemetry.ts (1)
5-7: Remove unnecessary return statement for void function.The function has a
voidreturn type but uses an explicitreturnstatement. While technically correct ifstartActiveSpanreturns void, it's clearer to omit the return keyword.Apply this diff to improve clarity:
-export function traceActiveSpan(name: string, callback: (span: Span) => void): void { - return trace.getTracer("default").startActiveSpan(name, callback); -} +export function traceActiveSpan(name: string, callback: (span: Span) => void): void { + trace.getTracer("default").startActiveSpan(name, callback); +}apps/provider-proxy/src/services/WebsocketServer.ts (1)
294-297: Clarify what constitutes an "empty" message.The empty message check only verifies truthiness. Consider being more specific about what constitutes an empty message (null, undefined, empty string, etc.) for better clarity.
-if (!socketMessage) { - this.logger?.info(`Received empty message from provider. Skipping...`); +if (socketMessage === null || socketMessage === undefined || socketMessage === '') { + this.logger?.info(`Received empty message from provider (${socketMessage === '' ? 'empty string' : String(socketMessage)}). Skipping...`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/provider-proxy/src/app.ts(1 hunks)apps/provider-proxy/src/container.ts(2 hunks)apps/provider-proxy/src/services/WebsocketServer.ts(7 hunks)apps/provider-proxy/src/utils/telemetry.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/provider-proxy/src/app.ts (1)
apps/provider-proxy/src/services/WebsocketServer.ts (1)
WebsocketServer(18-346)
🪛 Biome (1.9.4)
apps/provider-proxy/src/utils/telemetry.ts
[error] 6-6: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-build
🔇 Additional comments (7)
apps/provider-proxy/src/container.ts (1)
29-29: LGTM! The shift from a logger factory to a singleton instance improves consistency.Moving from
createWsLoggerfactory function to a singletonwsLoggerinstance ensures consistent logging context across all WebSocket connections, which is beneficial for tracing and debugging.Also applies to: 40-40
apps/provider-proxy/src/app.ts (1)
58-58: Correct adaptation to the logger singleton pattern.The WebsocketServer instantiation properly uses
container.wsLoggerinstead of the removedcontainer.createWsLoggerfactory function.apps/provider-proxy/src/utils/telemetry.ts (1)
9-12: Well-implemented context propagation utility.The
propagateTracingContextfunction correctly captures and propagates the active tracing context across asynchronous boundaries, which is crucial for maintaining trace continuity in WebSocket event handlers.apps/provider-proxy/src/services/WebsocketServer.ts (4)
28-33: Constructor properly updated to accept logger instance.The change from a logger factory to a direct
LoggerServiceinstance aligns with the singleton pattern adopted in the container.
43-54: Excellent tracing implementation for WebSocket upgrade handling.The upgrade handler correctly:
- Wraps the operation in a tracing span
- Captures the active context before async operations
- Restores context when emitting the connection event
- Properly ends the span after connection establishment
62-102: Robust message handling with comprehensive tracing and error handling.The implementation excels in:
- Proper JSON parsing with detailed error logging
- PII redaction in span attributes (cert/key)
- Clear error responses for invalid messages
- Comprehensive span attributes for telemetry
149-149: Good error detail, but ensure message types don't contain sensitive data.Including the actual message type in the error improves debugging. However, ensure that message types never contain sensitive information that shouldn't be exposed in error messages.
stalniy
force-pushed
the
fix/provider-proxy-ws
branch
from
f63dd79 to
60a3b18
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1614 +/- ##
==========================================
+ Coverage 42.75% 42.84% +0.09%
==========================================
Files 879 880 +1
Lines 21625 21667 +42
Branches 3969 3973 +4
==========================================
+ Hits 9245 9284 +39
- Misses 12197 12201 +4
+ Partials 183 182 -1
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/provider-proxy/src/services/WebsocketServer.ts (1)
338-342: Consider improving empty message detection.The current check
if (!socketMessage)may not properly detect empty Buffers. Consider a more explicit check:-if (!socketMessage) { +const isEmptyString = typeof socketMessage === "string" && socketMessage.length === 0; +const isEmptyBuffer = Buffer.isBuffer(socketMessage) && socketMessage.length === 0; +if (socketMessage == null || isEmptyString || isEmptyBuffer) {Note: This appears to be a duplicate of a previous review comment, but the suggestion remains valid for robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/provider-proxy/src/app.ts(1 hunks)apps/provider-proxy/src/container.ts(2 hunks)apps/provider-proxy/src/services/WebsocketServer.ts(6 hunks)apps/provider-proxy/src/utils/telemetry.ts(1 hunks)apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts(9 hunks)apps/provider-proxy/test/setup/providerServer.ts(1 hunks)apps/provider-proxy/tsconfig.build.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/provider-proxy/src/app.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/provider-proxy/src/container.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.spec.{ts,tsx}`: Use `setup` function instead of `beforeEach` in test files...
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
📄 Source: CodeRabbit Inference Engine (.cursor/rules/setup-instead-of-before-each.mdc)
List of files the instruction was applied to:
apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts
🧠 Learnings (2)
apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts (2)
Learnt from: stalniy
PR: akash-network/console#1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/query-by-in-tests.mdc:0-0
Timestamp: 2025-06-30T12:11:50.570Z
Learning: Applies to apps/{deploy-web,provider-console}/**/*.spec.tsx : Use `queryBy` methods instead of `getBy` methods in test expectations in `.spec.tsx` files
apps/provider-proxy/src/services/WebsocketServer.ts (1)
Learnt from: jzsfkzm
PR: akash-network/console#1458
File: apps/api/src/network/services/network/network.service.ts:0-0
Timestamp: 2025-06-10T01:25:37.604Z
Learning: In the Akash Network Console codebase, network parameters are validated at the schema level using Zod schemas before reaching service methods, making default cases in switch statements unnecessary for validated enum-like parameters.
🧬 Code Graph Analysis (2)
apps/provider-proxy/test/setup/providerServer.ts (2)
apps/provider-proxy/test/seeders/createX509CertPair.ts (1)
createX509CertPair(4-26)apps/provider-proxy/test/setup/chainApiServer.ts (1)
generateBech32(59-62)
apps/provider-proxy/src/utils/telemetry.ts (1)
apps/api/src/core/services/execution-context/execution-context.service.ts (1)
context(18-26)
🪛 Biome (1.9.4)
apps/provider-proxy/src/utils/telemetry.ts
[error] 6-6: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-build
🔇 Additional comments (10)
apps/provider-proxy/src/utils/telemetry.ts (1)
9-12: LGTM! Well-implemented tracing context propagation.The
propagateTracingContextfunction correctly captures the current active context and ensures it's restored when the wrapped callback is invoked. This is essential for maintaining tracing context across asynchronous boundaries.apps/provider-proxy/tsconfig.build.json (1)
4-4: LGTM! Good addition for build compatibility.The
isolatedModulesoption enforces that each file can be compiled independently, which improves build performance and compatibility with transpilers. This aligns well with the stricter typing introduced in the WebsocketServer.apps/provider-proxy/test/setup/providerServer.ts (1)
15-15: LGTM! Certificate generation now aligns with provider address validation.The change to generate certificates with a bech32 address as the common name supports the enhanced certificate validation logic introduced in the WebsocketServer. This ensures test certificates match the expected provider identity format.
apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts (3)
18-22: LGTM! Proper test setup for certificate validation.The test correctly generates a bech32 provider address and creates certificates with the provider address as the common name. This properly simulates the TLS certificate validation scenario.
217-220: LGTM! EnhancedourMessagehelper with proper defaults.The
ourMessagefunction now includes sensible defaults forproviderAddressandchainNetwork, ensuring all test messages comply with the new schema requirements while maintaining flexibility for specific test cases.
153-199: LGTM! Comprehensive certificate validation testing.This test case properly validates the certificate validation logic by:
- Testing with an invalid provider address (should fail)
- Testing with a valid provider address (should succeed)
- Verifying the correct error codes and messages
The test coverage ensures the security enhancement works as expected.
apps/provider-proxy/src/services/WebsocketServer.ts (4)
16-30: LGTM! Comprehensive message schema validation.The Zod schema provides robust validation for incoming WebSocket messages with:
- Proper URL validation
- Enum validation for supported networks
- Bech32 address validation for provider addresses
- Clear documentation for the data field
This enforces data integrity and prevents malformed messages from being processed.
88-108: LGTM! Robust JSON parsing with proper error handling.The message parsing includes comprehensive error handling:
- Try-catch for JSON parsing with detailed error logging
- Structured error response to client
- Proper type checking before processing
This prevents crashes from malformed JSON and provides clear feedback to clients.
274-323: LGTM! Comprehensive certificate validation implementation.The certificate validation logic properly:
- Extracts the peer certificate from the TLS socket
- Pauses socket data until validation completes
- Validates certificate against chain network and provider address
- Handles both success and failure cases appropriately
- Closes connection with proper error codes for invalid certificates
This significantly enhances the security of provider connections.
62-75: LGTM! Well-integrated OpenTelemetry tracing.The upgrade handler properly creates tracing spans and propagates context across the async upgrade process. The use of
propagateTracingContextensures tracing information is maintained throughout the WebSocket connection lifecycle.
stalniy
force-pushed
the
fix/provider-proxy-ws
branch
2 times, most recently
from
8ffd0d1 to
d097503
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/provider-proxy/src/utils/telemetry.ts (1)
5-7: Fix the return type mismatch intraceActiveSpanfunction.The function has a
voidreturn type but returns a value fromstartActiveSpan. This causes a type error.apps/provider-proxy/src/services/WebsocketServer.ts (1)
397-399: Breaking change: Required fields for proper certificate validation.Making
chainNetworkandproviderAddressrequired fields is a breaking change that will require updates to any code creating these objects. This appears intentional for ensuring proper certificate validation.
🧹 Nitpick comments (1)
apps/provider-proxy/src/services/WebsocketServer.ts (1)
339-342: Simplify the empty message check condition.The current condition is complex and hard to read. Consider extracting the logic into a helper function or simplifying the condition.
- if (!socketMessage || ("byteLength" in socketMessage && socketMessage.byteLength === 0) || ("length" in socketMessage && socketMessage.length === 0)) { + const isEmpty = !socketMessage || + (socketMessage instanceof Buffer && socketMessage.length === 0) || + (typeof socketMessage === "string" && socketMessage.length === 0); + if (isEmpty) {Alternatively, create a helper function:
private isEmptyMessage(message: unknown): boolean { return !message || (message instanceof Buffer && message.length === 0) || (typeof message === "string" && message.length === 0); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/provider-proxy/src/app.ts(1 hunks)apps/provider-proxy/src/container.ts(2 hunks)apps/provider-proxy/src/services/WebsocketServer.ts(6 hunks)apps/provider-proxy/src/utils/telemetry.ts(1 hunks)apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts(9 hunks)apps/provider-proxy/test/setup/providerServer.ts(1 hunks)apps/provider-proxy/tsconfig.build.json(1 hunks)apps/provider-proxy/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/provider-proxy/src/app.ts
- apps/provider-proxy/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/provider-proxy/tsconfig.build.json
- apps/provider-proxy/src/container.ts
- apps/provider-proxy/test/setup/providerServer.ts
- apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts
🧰 Additional context used
🧠 Learnings (2)
apps/provider-proxy/src/services/WebsocketServer.ts (1)
Learnt from: jzsfkzm
PR: akash-network/console#1458
File: apps/api/src/network/services/network/network.service.ts:0-0
Timestamp: 2025-06-10T01:25:37.604Z
Learning: In the Akash Network Console codebase, network parameters are validated at the schema level using Zod schemas before reaching service methods, making default cases in switch statements unnecessary for validated enum-like parameters.
apps/provider-proxy/src/utils/telemetry.ts (1)
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-06-30T12:11:58.081Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't specify return type of `setup` function
🪛 Biome (1.9.4)
apps/provider-proxy/src/utils/telemetry.ts
[error] 6-6: The function should not return a value because its return type is void.
The function is here:
'void' signals the absence of value. The returned value is likely to be ignored by the caller.
(lint/correctness/noVoidTypeReturn)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-build
🔇 Additional comments (4)
apps/provider-proxy/src/utils/telemetry.ts (1)
9-12: Context propagation implementation looks correct.The
propagateTracingContextfunction properly captures the current OpenTelemetry context and returns a wrapped callback that restores the context when invoked. This enables consistent tracing across asynchronous boundaries.apps/provider-proxy/src/services/WebsocketServer.ts (3)
16-30: Well-structured message validation schema.The Zod schema provides comprehensive validation with proper type enforcement for WebSocket messages. The bech32 address validation for
providerAddressand network validation usingnetConfig.getSupportedNetworks()ensure data integrity.
87-124: Robust message parsing with comprehensive error handling.The inline JSON parsing with try-catch and subsequent schema validation provides good error handling. The structured error responses help clients understand validation failures.
276-323: Comprehensive certificate validation with proper error handling.The certificate validation logic correctly:
- Pauses the socket to prevent data processing during validation
- Handles missing certificates appropriately
- Provides detailed error logging with context
- Properly cleans up listeners on validation failure
- Resumes the socket in both success and error cases
stalniy
force-pushed
the
fix/provider-proxy-ws
branch
from
d097503 to
3d67a05
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/provider-proxy/src/services/WebsocketServer.ts (1)
401-403: Breaking change: Required fields align with certificate validation needs.Making
chainNetworkandproviderAddressrequired fields is a breaking change that supports the enhanced certificate validation. This aligns with the previous review feedback and ensures proper certificate validation.
🧹 Nitpick comments (2)
apps/provider-proxy/src/services/WebsocketServer.ts (2)
110-124: Enhance error response for schema validation failures.The error response should include more specific information about what fields are missing or invalid to help with debugging.
if (parsedMessage.error) { this.logger?.error({ event: "CLIENT_MESSAGE_INVALID_JSON", message: "Message doesn't match expected schema", error: parsedMessage.error }); return ws.send( JSON.stringify({ type: "websocket", message: "Message doesn't match expected schema", - error: "Invalid message format" + error: "Invalid message format", + details: parsedMessage.error.issues.map(issue => ({ + path: issue.path.join('.'), + message: issue.message + })) }) ); }
339-346: Simplify empty message detection logic.The current empty message detection is overly complex and could be simplified while maintaining the same functionality.
- if ( - !socketMessage || - (Object.hasOwn(socketMessage, "byteLength") && (socketMessage as Buffer).byteLength === 0) || - (Object.hasOwn(socketMessage, "length") && (socketMessage as string | unknown[]).length === 0) - ) { + const isEmpty = !socketMessage || + (Buffer.isBuffer(socketMessage) && socketMessage.byteLength === 0) || + (typeof socketMessage === "string" && socketMessage.length === 0); + + if (isEmpty) { this.logger?.info(`Received empty message from provider. Skipping...`); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/provider-proxy/src/app.ts(1 hunks)apps/provider-proxy/src/container.ts(2 hunks)apps/provider-proxy/src/services/WebsocketServer.ts(6 hunks)apps/provider-proxy/src/utils/telemetry.ts(1 hunks)apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts(9 hunks)apps/provider-proxy/test/setup/providerServer.ts(1 hunks)apps/provider-proxy/tsconfig.build.json(1 hunks)apps/provider-proxy/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/provider-proxy/src/app.ts
- apps/provider-proxy/src/utils/telemetry.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/provider-proxy/tsconfig.build.json
- apps/provider-proxy/tsconfig.json
- apps/provider-proxy/test/setup/providerServer.ts
- apps/provider-proxy/src/container.ts
- apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts
🧰 Additional context used
🧠 Learnings (1)
apps/provider-proxy/src/services/WebsocketServer.ts (1)
Learnt from: jzsfkzm
PR: akash-network/console#1458
File: apps/api/src/network/services/network/network.service.ts:0-0
Timestamp: 2025-06-10T01:25:37.604Z
Learning: In the Akash Network Console codebase, network parameters are validated at the schema level using Zod schemas before reaching service methods, making default cases in switch statements unnecessary for validated enum-like parameters.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-build
🔇 Additional comments (2)
apps/provider-proxy/src/services/WebsocketServer.ts (2)
290-322: Excellent certificate validation implementation.The certificate validation logic properly handles async validation with appropriate error handling, socket pausing/resuming, and cleanup. The tracing context propagation and detailed logging provide good observability.
62-75: Proper tracing context setup for WebSocket upgrades.The tracing implementation correctly creates spans and propagates context during the WebSocket upgrade process. The logging provides good visibility into the upgrade lifecycle.
| "Currently it's used only for service shell communication and stores only buffered representation of string in char codes something like this: Array.from(Uint8Array).join(', ')" | ||
| ) | ||
| }); | ||
| type WsMessage = z.infer<typeof MESSAGE_SCHEMA> & { id: unknown }; |
There was a problem hiding this comment.
Fix type definition mismatch between schema and interface.
The WsMessage type includes an id field that's not present in the MESSAGE_SCHEMA. This creates a type safety issue where the schema validation won't validate the id field, but the type assumes it exists.
-type WsMessage = z.infer<typeof MESSAGE_SCHEMA> & { id: unknown };
+const MESSAGE_SCHEMA = z.object({
+ id: z.unknown().optional(),
+ type: z.enum(["ping", "websocket"]),
+ // ... rest of schema
+});
+type WsMessage = z.infer<typeof MESSAGE_SCHEMA>;Or if id is always expected:
-type WsMessage = z.infer<typeof MESSAGE_SCHEMA> & { id: unknown };
+const MESSAGE_SCHEMA = z.object({
+ id: z.unknown(),
+ type: z.enum(["ping", "websocket"]),
+ // ... rest of schema
+});
+type WsMessage = z.infer<typeof MESSAGE_SCHEMA>;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/provider-proxy/src/services/WebsocketServer.ts at line 30, the WsMessage
type includes an id field that is not defined in the MESSAGE_SCHEMA, causing a
type mismatch and potential validation issues. To fix this, either add the id
field to the MESSAGE_SCHEMA so it is validated properly, or remove the id field
from the WsMessage type if it is not always expected. Ensure the type definition
aligns exactly with the schema to maintain type safety.
| try { | ||
| message = typeof messageStr === "string" ? JSON.parse(messageStr) : undefined; | ||
| } catch (error) { | ||
| this.logger?.error({ | ||
| event: "CLIENT_MESSAGE_INVALID_JSON", | ||
| message: "Received message is not a JSON string", | ||
| messageLength: messageStr?.length, | ||
| messageType: typeof messageStr, | ||
| messageConstructor: messageStr?.constructor?.name | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve JSON parsing to handle different message types.
The current JSON parsing only handles string messages, but WebSocket messages can be Buffer or other types. This could cause issues when non-string messages are received.
- try {
- message = typeof messageStr === "string" ? JSON.parse(messageStr) : undefined;
- } catch (error) {
+ try {
+ const messageText = typeof messageStr === "string" ? messageStr : messageStr.toString();
+ message = JSON.parse(messageText);
+ } catch (error) {
this.logger?.error({
event: "CLIENT_MESSAGE_INVALID_JSON",
message: "Received message is not a JSON string",
- messageLength: messageStr?.length,
+ messageLength: messageStr?.length || messageStr?.byteLength,
messageType: typeof messageStr,
messageConstructor: messageStr?.constructor?.name
});
}📝 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.
| try { | |
| message = typeof messageStr === "string" ? JSON.parse(messageStr) : undefined; | |
| } catch (error) { | |
| this.logger?.error({ | |
| event: "CLIENT_MESSAGE_INVALID_JSON", | |
| message: "Received message is not a JSON string", | |
| messageLength: messageStr?.length, | |
| messageType: typeof messageStr, | |
| messageConstructor: messageStr?.constructor?.name | |
| }); | |
| } | |
| try { | |
| const messageText = | |
| typeof messageStr === "string" | |
| ? messageStr | |
| : messageStr.toString(); | |
| message = JSON.parse(messageText); | |
| } catch (error) { | |
| this.logger?.error({ | |
| event: "CLIENT_MESSAGE_INVALID_JSON", | |
| message: "Received message is not a JSON string", | |
| messageLength: messageStr?.length || messageStr?.byteLength, | |
| messageType: typeof messageStr, | |
| messageConstructor: messageStr?.constructor?.name | |
| }); | |
| } |
🤖 Prompt for AI Agents
In apps/provider-proxy/src/services/WebsocketServer.ts around lines 88 to 98,
the JSON parsing only attempts to parse messages if they are strings, but
WebSocket messages can also be Buffers or other types. Update the code to first
check if the message is a Buffer and convert it to a string before parsing. For
other non-string types, handle them appropriately or skip parsing to avoid
errors.
3 participants
Summary by CodeRabbit
New Features
Improvements
Chores