chore(core): handle errors inside message handlers

[NathanFlurry]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Oct 7, 2025 11:36pm

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: Handle errors inside message handlers

Todo List:

• Read and analyze the changed file (packages/rivetkit/src/client/actor-conn.ts)
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Analyze test coverage implications
• Provide comprehensive feedback

Overall Assessment ✅

This is a solid defensive programming improvement that adds proper error handling to WebSocket event handlers. The changes are well-implemented and follow the project's established patterns.

Code Quality and Best Practices ✅

Strengths:

• Consistent Error Handling: All three WebSocket event handlers (message, close, error) now have proper try-catch blocks
• Structured Logging: Uses the project's logger() function with structured logging format including descriptive messages
• Error Serialization: Properly uses stringifyError() utility for consistent error formatting
• Non-Breaking: Changes are purely additive - existing functionality remains unchanged

Style Consistency:

• Follows established logging patterns found throughout the codebase (packages/rivetkit/src/client/actor-conn.ts:321-325)
• Maintains existing code formatting and indentation
• Uses lowercase log messages as per project guidelines

Potential Issues and Recommendations 🔍

Missing Coverage: The SSE (Server-Sent Events) transport path lacks similar error handling:

// Lines 396-405: SSE handlers don't have try-catch blocks
eventSource.addEventListener("message", (ev: UniversalMessageEvent) => {
    if (ev.type === "ping") return;
    this.#handleOnMessage(ev.data); // ⚠️ No error handling
});

eventSource.addEventListener("error", (ev: UniversalErrorEvent) => {
    this.#handleOnError(); // ⚠️ No error handling
});

Recommendation: Consider adding similar try-catch blocks to SSE event handlers for consistency.

Performance Considerations ✅

• Minimal Overhead: Try-catch blocks have negligible performance impact
• Error Logging: Structured logging is efficient and won't cause performance issues
• No Memory Leaks: Error handling doesn't introduce additional references or prevent garbage collection

Security Concerns ✅

• Safe Error Handling: Uses stringifyError() which safely serializes errors without exposing sensitive data
• No Information Leakage: Error messages are descriptive but don't reveal internal implementation details
• Graceful Degradation: Errors are logged but don't crash the connection handling

Test Coverage 📋

Current State: The changes add defensive error handling that would be difficult to test directly without intentionally causing errors in the event handlers.

Suggestions for Testing:

• Consider adding unit tests that mock WebSocket events to throw errors and verify logging behavior
• Integration tests could simulate network failures to ensure error handling works end-to-end
• The error handling itself doesn't require extensive testing, but the underlying methods (#handleOnMessage, #handleOnClose, #handleOnError) should have comprehensive coverage

Final Verdict ⭐

Approved with Minor Suggestion: This is a valuable defensive programming improvement. The implementation is clean, follows project conventions, and enhances the robustness of the WebSocket connection handling.

Recommended Follow-up: Consider extending similar error handling to the SSE transport handlers for complete consistency across both transport types.

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore(core): handle errors inside message handlers #1364 👈 (View in Graphite)
• chore(core): add smoke test #1363
• chore: fix serverless config #1361
• chore(core): add x-rivet-endpoint header for serverless runners & cleanup severless runner setup #1359
• fix(next): fix local dev #1352
• chore(core): add support for auto-configuring serverless #1356 : 1 other dependent PR (#1357 )
• chore(core): add build:fast script #1355
• feat(core): support running engine #1353
• fix(example-deno): fix check-types #1351
• chore(core): simplify config #1350
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• merge queue - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[graphite-app]

Merge activity

• Oct 7, 11:36 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Oct 7, 11:37 PM UTC: CI is running for this pull request on a draft pull request (#1365) due to your merge queue CI optimization settings.
• Oct 7, 11:37 PM UTC: Merged by the Graphite merge queue via draft PR: #1365.