fix(notification-client): guard JSON parsing of notification frames#232
Conversation
The WebSocket onmessage handler did an unguarded JSON.parse(parsedData) and JSON.parse(data.message). It runs inside the onmessage callback, so a single malformed frame (non-JSON, empty/absent `message`, truncated payload, or a non-event control frame) threw an uncaught error and crashed the host process — observed repeatedly in production, where it also wedged the data-pump. Extract the frame handling into handleMessage() and guard every parse: a bad frame is logged (warn) and skipped instead of throwing. Adds regression tests covering valid frames, malformed/empty/missing `message`, missing inner `data`, non-JSON outer frames, and validation frames. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe ChangesNotificationClient handleMessage refactor and tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (2)
src/common/notification-client.ts (1)
277-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType assertion could be more defensive.
The type assertion
as stringondata.messageassumes the value is a string, butdata.messageis typed asstring | undefined. While the try-catch will handle JSON.parse errors fromundefinedor other non-string values, the error message at lines 283-285 may be misleading (e.g., ifdata.messageisundefined, it will showtype undefinedin the log).Consider adding an explicit check before parsing:
🛡️ More defensive guard suggestion
+ if (!data.message || typeof data.message !== "string") { + this.logger.warn( + `Discarding notification frame: 'message' field is missing or not a string (type ${typeof data.message})` + ) + return + } + let parsed: { pattern: string; data: NotificationEventData } try { - parsed = JSON.parse(data.message as string) as { + parsed = JSON.parse(data.message) as { pattern: string data: NotificationEventData }🤖 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 `@src/common/notification-client.ts` at line 277, The type assertion `as string` on `data.message` in the JSON.parse call is not defensive since `data.message` is typed as `string | undefined`, which can cause misleading error messages. Before the JSON.parse call around line 277, add an explicit check to verify that `data.message` is actually a string (e.g., using typeof or checking for undefined), and handle the case where it is not a string with an appropriate error before attempting to parse. This ensures the error logging at lines 283-285 will provide accurate type information and clearer context about what went wrong.test/tests/common/notification-client.test.ts (1)
49-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding test coverage for maxEvents enforcement.
The test suite thoroughly covers the guarded parsing and error handling paths, but doesn't verify the
maxEventsenforcement logic (lines 311-315 in the implementation). While this is existing functionality that was moved into the new method, adding a test would improve coverage of all code paths inhandleMessage.🧪 Example test to add
it("completes observer and closes connection when maxEvents is reached", () => { const observer = new Subject<NotificationEvent>() let completed = false observer.subscribe({ complete: () => { completed = true } }) const logger = recordingLogger() const client = new NotificationClient( observer, { apiKey: "test-key" }, { tenant: "t", dataCore: "dc" }, { logger, maxEvents: 2 } ) const handle = (raw: string) => (client as unknown as { handleMessage(raw: string): void }).handleMessage(raw) handle(validFrame()) handle(validFrame()) assertEquals(completed, true) })Note: This test would also need to verify that
webSocket.close()was called, which might require mocking the WebSocket instance.🤖 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 `@test/tests/common/notification-client.test.ts` around lines 49 - 94, Add a new test case to the NotificationClient.handleMessage test suite that verifies the maxEvents enforcement logic. Create a test that instantiates a NotificationClient with a low maxEvents value (e.g., 2), calls the handleMessage method that many times with valid frames, and then asserts that the observer's complete callback was invoked and the underlying webSocket.close() method was called to verify the connection was properly closed when the event limit was reached.
🤖 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 `@src/common/notification-client.ts`:
- Line 277: The type assertion `as string` on `data.message` in the JSON.parse
call is not defensive since `data.message` is typed as `string | undefined`,
which can cause misleading error messages. Before the JSON.parse call around
line 277, add an explicit check to verify that `data.message` is actually a
string (e.g., using typeof or checking for undefined), and handle the case where
it is not a string with an appropriate error before attempting to parse. This
ensures the error logging at lines 283-285 will provide accurate type
information and clearer context about what went wrong.
In `@test/tests/common/notification-client.test.ts`:
- Around line 49-94: Add a new test case to the NotificationClient.handleMessage
test suite that verifies the maxEvents enforcement logic. Create a test that
instantiates a NotificationClient with a low maxEvents value (e.g., 2), calls
the handleMessage method that many times with valid frames, and then asserts
that the observer's complete callback was invoked and the underlying
webSocket.close() method was called to verify the connection was properly closed
when the event limit was reached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36f58423-dd71-41fd-a395-2e52a901af9e
📒 Files selected for processing (2)
src/common/notification-client.tstest/tests/common/notification-client.test.ts
1 participant
Problem
NotificationClient's WebSocketonmessagehandler did unguardedJSON.parse(parsedData)thenJSON.parse(data.message). Because it runs inside theonmessagecallback, a single malformed frame (non-JSON, empty/absentmessage, truncated payload, or a non-event control frame) threw an uncaught error that crashed the host process.Observed repeatedly in production (
fishfacts-ai-backend): intermittent pod crashes (exit 1) every few hours, and when the crashing pod was the data-pump leader it wedged the pump cluster-wide until a manual rolling restart. Captured real frames confirm the normal shape is{"message":"<stringified-JSON event>"}(notype), so any frame whosemessageisn't valid JSON hits the unguarded parse.Fix
Extract the frame handling into
handleMessage()and guard every parse — a bad frame is logged (warn) and skipped instead of throwing. No behaviour change for valid frames.Tests (TDD)
Added
test/tests/common/notification-client.test.ts: written first, went red on the malformed cases (reproducing the crash), then green after the guard. Covers: valid frame emits correctly, malformed/empty/missingmessage, missing innerdata, non-JSON outer frame, andvalidationframes. Full suite 210/210, build clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests