chore(rivetkit): log close code when client ws closes

[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	Ready	Preview	Comment	Nov 5, 2025 4:06am

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 5, 2025 4:06am
rivet-inspector	Ignored	Preview		Nov 5, 2025 4:06am
rivet-site	Ignored	Preview		Nov 5, 2025 4:06am

[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(pegboard): allow configuring reschedule_backoff_max_exponent #3370
• fix(next-js): auto shut down runners when source code updates #3369
• fix(rivetkit): fix duplicate runner key in serverless #3366
• chore(runner): improve runner connection logs #3365
• chore(rivetkit): add runner sse ping interval #3362
• chore(rivetkit): log close code when client ws closes #3361 👈 (View in Graphite)
• chore(rivetkit): log runner id with actor logs #3360
• chore(scripts): add endian converter script #3359
• fix(guard): fix url decoding actor id + token #3357
• chore(rivetkit): remove unneeded headers & protocols in requests #3356
• fix(rivetkit): reduce pegboard actor retry for spawned engines #3355
• chore(pegboard): allow configuring base_retry_timeout, actor_start_threshold, actor_stop_threshold, and retry_reset_duration #3354
• feat(pb): add reschedule_ts to actors #3363
• fix: fix all dashboards #3358
• chore: change log levels #3333
• chore: misc bug fixes, add logs for outbound req #3332
• fix: use correct vbare functions for runner tunnel #3300 : 1 other dependent PR (#3301 )
• feat(grafana): fix api dashboards #3193
• fix: docker compose build #3273
• chore: fix remaining warnings for all targets #3305
• fix(gas): fix from_workflow when creating standalone ctx from op ctx #3342
• ci: udpate ci filter to succeed if no relevant changes #3343
• main

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

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.

[claude]

PR Review: Log close code when client ws closes

Summary

This PR improves error messaging when a WebSocket connection closes by including the close code and reason in the error message passed to the onOpen promise rejection. The change moves the promise rejection after the closeEvent is cast, allowing access to code and reason properties.

Code Quality ✅

Positives:

• Clean, focused change that improves debuggability
• Follows existing logging patterns in the file (structured logging at line 581-589)
• Error message format is clear and informative
• The reordering of code makes logical sense - extract close event properties before using them

Potential Issues ⚠️

1. Undefined/null values in Node.js environments

   • Line 566-568 notes that CloseEvent properties may be undefined in Node.js
   • The error message at line 576 uses ${closeEvent.code} and ${closeEvent.reason} which could result in "websocket closed with code undefined: undefined"
   • Recommendation: Add a fallback for undefined values:

   const code = closeEvent.code ?? 'unknown';
   const reason = closeEvent.reason || 'no reason provided';
   this.#onOpenPromise.reject(
       new Error(`websocket closed with code ${code}: ${reason}`)
   );

2. Variable wasClean extracted but not used in the new code location

   • Line 570 extracts wasClean but it's only used later in the logging (line 585) and rejection of in-flight actions (line 597)
   • This is fine, but the early extraction at line 570 is now before it's needed
   • Minor: Consider moving const wasClean = closeEvent.wasClean; closer to where it's first used (just before line 581) for better code locality

Performance Considerations ✅

No performance concerns - this is error handling code that runs infrequently.

Security Concerns ✅

No security issues identified. The close code and reason are WebSocket protocol values and don't pose security risks when included in error messages.

Test Coverage 📝

Observation:

• This change affects error handling in the WebSocket close path
• The modified error message would be visible to callers waiting on the connection promise
• Recommendation: Consider adding a test case that verifies:
  • The error message includes close code and reason when WebSocket closes before opening
  • Behavior when close code/reason are undefined (Node.js case)

Overall Assessment

This is a solid improvement for debugging WebSocket connection issues. The main suggestion is to handle the undefined case explicitly to avoid confusing error messages in Node.js environments.

Verdict: ✅ Approve with minor suggestions for improvement

---

Review generated with Claude Code

[claude]

Code Review

Summary

This PR improves error messaging when a WebSocket connection closes by including the close code and reason in the rejected promise. This is a valuable debugging improvement that provides more context about connection failures.

Positive Changes

✅ Enhanced Error Messages: Including closeEvent.code and closeEvent.reason in the error message makes debugging connection issues much easier
✅ Logical Reordering: Moving the close event extraction before the promise rejection is more logical - you need the values before using them
✅ Better User Experience: Developers will now see descriptive error messages instead of just "Closed"

Potential Issues & Suggestions

1. Inconsistent Error Handling Pattern

The error handling for #onOpenPromise rejection now differs from how #actionsInFlight errors are handled:

• #onOpenPromise: Uses close code/reason in error message (lines 574-577)
• #actionsInFlight: Uses generic "Connection closed" / "Connection lost" based on wasClean (lines 600-602)

Suggestion: Consider applying similar detailed error messaging to the in-flight actions for consistency:

const disconnectError = new Error(
  `websocket ${wasClean ? "closed" : "lost"} with code ${closeEvent.code}: ${closeEvent.reason || "no reason provided"}`
);

2. Undefined Values in Node.js

The comment on line 566 mentions that CloseEvent properties might be undefined in Node.js. The error message should handle this gracefully:

Suggestion:

this.#onOpenPromise.reject(
  new Error(
    `websocket closed with code ${closeEvent.code ?? "unknown"}: ${closeEvent.reason || "no reason provided"}`,
  ),
);

This prevents the error message from showing "undefined" when these properties aren't available.

3. Missing Test Coverage

No tests were found for this WebSocket close handling logic. Given that this is critical error handling code:

Suggestion: Consider adding tests that verify:

• Error messages include close codes and reasons
• Behavior when closeEvent.code or closeEvent.reason are undefined (Node.js case)
• Different WebSocket close codes (1000 normal, 1001 going away, 1006 abnormal, etc.)

4. Minor: Error Message Consistency

The structured log uses lowercase "socket closed" (line 582) while other patterns in the codebase appear to use lowercase for log messages. The error message should probably follow a similar pattern.

Current: websocket closed with code ${closeEvent.code}: ${closeEvent.reason}
Consider: Keeping it as-is is fine, but ensure it matches your team's error message conventions

Non-Blocking Observations

• The TODO comments on lines 563-564 suggest future work on reconnection logic - this change is compatible with that future work
• The logging already captures this information (lines 581-589), so the error message improvement helps surface it to calling code

Overall Assessment

This is a good quality improvement that enhances debuggability. The main suggestions are around handling edge cases (undefined values) and maintaining consistency across similar error paths. None of the issues are blockers, but addressing them would make the code more robust.

Recommendation: ✅ Approve with minor suggestions for improvement

[claude]

Code Review

Summary

This PR improves error messaging when a WebSocket connection closes by including the close code and reason in the error message. The change is small but valuable for debugging.

Positive Aspects

✅ Better error visibility: Including closeEvent.code and closeEvent.reason in the error message makes debugging WebSocket closure issues much easier
✅ Proper sequencing: Moving the error rejection after the closeEvent cast ensures the close code and reason are available when constructing the error message
✅ Consistent with existing patterns: The structured logging at line 581-589 already captures these values, so using them in the error message maintains consistency
✅ Minimal scope: The change is focused and doesn't introduce unnecessary complexity

Code Quality Observations

Error Message Format
The error message follows a clear pattern:

`websocket closed with code ${closeEvent.code}: ${closeEvent.reason}`

This is good, though consider that closeEvent.code and closeEvent.reason may be undefined in Node.js environments (as noted in the comment at line 566-568). You might want to handle this:

const code = closeEvent.code ?? 'unknown';
const reason = closeEvent.reason ?? 'no reason provided';
this.#onOpenPromise.reject(
    new Error(`websocket closed with code ${code}: ${reason}`),
);

Comparison with Similar Code
Looking at line 600-602, there's another error creation that uses wasClean to provide context:

const disconnectError = new Error(
    wasClean ? "Connection closed" : "Connection lost",
);

Consider whether the open promise rejection should also differentiate between clean and unclean closes for consistency.

Minor Suggestions

1. Undefined handling: Add fallback values for code and reason since they may be undefined in Node.js
2. Consider wasClean flag: You might want to include whether the close was clean in the error message for additional context

Test Coverage

Consider adding or updating tests that verify:

• The error message includes the close code and reason when provided
• The error handling works correctly when code/reason are undefined (Node.js scenario)

Overall Assessment

This is a solid improvement to error reporting. The change is well-scoped and will make debugging WebSocket issues easier. The only concern is the potential for undefined values in the error message, which could be addressed with simple fallbacks.

Recommendation: ✅ Approve with minor suggestions for handling undefined values.

[claude]

Code Review

Summary

This PR improves error messaging when a WebSocket connection closes by including the close code and reason in the error message passed to the onOpen promise rejection. This is a valuable debugging enhancement that provides better context about connection failures.

Positive Changes

• Enhanced error visibility: Including closeEvent.code and closeEvent.reason makes debugging WebSocket issues significantly easier
• Logical reordering: Extracting close event properties before using them is more sensible
• Consistency: Aligns with the existing structured logging pattern at lines 581-589
• Focused scope: Clean, minimal change without unnecessary complexity

Issues and Recommendations

1. Handling Undefined Values in Node.js

Location: rivetkit-typescript/packages/rivetkit/src/client/actor-conn.ts:576

The comment at line 566-568 notes that CloseEvent properties may be undefined in Node.js environments. The current error message could result in 'websocket closed with code undefined: undefined'

Recommendation: Add fallback values to handle undefined gracefully using nullish coalescing and logical OR operators for code and reason respectively.

2. Consider Consistency with In-Flight Actions Error Pattern

Location: Compare lines 574-577 with lines 600-602

The error handling for actionsInFlight uses the wasClean flag to differentiate between Connection closed vs Connection lost. Consider whether the open promise rejection should follow a similar pattern. This is optional but would maintain consistency across error paths.

Test Coverage

No tests were found for this WebSocket close handling logic. Consider adding test coverage for:

• Error messages include close codes and reasons when provided
• Behavior when closeEvent.code/closeEvent.reason are undefined (Node.js case)
• Different WebSocket close codes (1000 normal, 1001 going away, 1006 abnormal, etc.)

Performance and Security

• No performance concerns - this is infrequent error path code
• No security issues - close codes/reasons are protocol values safe to include in errors

Overall Assessment

This is a quality improvement that enhances debuggability. The main concern is handling the undefined case in Node.js environments to avoid confusing error messages. Otherwise, the change is solid.

Recommendation: Approve with suggestion to handle undefined values

---

Review generated by Claude Code

[claude]

Code Review

This is a small but useful improvement that enhances error messages when WebSocket connections close. Here's my analysis:

✅ Positive Aspects

1. Improved Error Messages: The change provides much better error context by including the close code and reason in the error message when the open promise is rejected. This will make debugging connection issues significantly easier.

2. Logical Code Organization: Moving the error rejection to after the closeEvent extraction is a good change - it allows the error message to use the code and reason properties.

3. Consistent with Existing Patterns: The error message format follows similar patterns elsewhere in the codebase (e.g., line 600-601 where errors are created with context-specific messages).

💡 Suggestions for Improvement

1. Consider Using Custom Error Types: The codebase has custom error classes in client/errors.ts (like ActorClientError, InternalError, etc.). Consider creating a specific error type for connection closures:

export class WebSocketClosedError extends ActorClientError {
    constructor(
        public readonly code: number,
        public readonly reason: string,
    ) {
        super(`websocket closed with code ${code}: ${reason}`);
    }
}

This would make the error more type-safe and easier to handle by consumers.

2. Handle Undefined Properties: The comment mentions that closeEvent properties may be undefined in Node.js environments. Consider defensive handling:

new Error(
    `websocket closed with code ${closeEvent.code ?? 'unknown'}: ${closeEvent.reason ?? 'no reason provided'}`,
)

3. Redundant Variable: The wasClean variable is extracted before it's used in the error rejection, but this creates a small gap in the code flow. Since wasClean is still used later (line 597, 601), this is actually fine - just noting that the variable declaration and the error rejection are now interleaved.

🔍 No Issues Found

• No bugs detected: The logic is sound
• No security concerns: Just improving error messages
• No performance issues: Minimal overhead
• Test coverage: While there are no specific tests for this code path, this is a logging/error message improvement that doesn't change behavior

📊 Impact

• Scope: Very focused change (4 lines modified)
• Risk: Low - only affects error message content
• Value: High for debugging and operational visibility

✅ Recommendation

Approve with minor suggestions. This is a solid improvement that will make debugging WebSocket connection issues much easier. The suggestions above are optional enhancements, not blockers.

The change aligns well with the related PR #3360 which also improves logging for operational visibility.

[claude]

Code Review

Summary

This PR improves error messages when a WebSocket connection closes by including the close code and reason in the rejection error. This is a helpful enhancement for debugging connection issues.

Positive Observations

✅ Better Error Messages: The change provides much more actionable information when debugging WebSocket connection failures. Including closeEvent.code and closeEvent.reason in the error message will help developers quickly identify connection issues.

✅ Logical Reordering: Moving the promise rejection after extracting the closeEvent properties makes sense - this ensures we can safely use closeEvent.code and closeEvent.reason in the error message.

✅ Follows Conventions: The commit message follows the repository's conventional commit format (chore(rivetkit): ...).

✅ Consistent with Existing Logging: The close code and reason are already being logged in the structured log at line 583-584, so including them in the error message maintains consistency.

Potential Issues & Suggestions

1. Undefined Property Handling (Minor)

At line 569-570, there's a comment noting that CloseEvent properties "will be undefined" in NodeJS. The new error message at line 576 could potentially display undefined values:

`websocket closed with code ${closeEvent.code}: ${closeEvent.reason}`

Suggestion: Consider providing fallback values:

`websocket closed with code ${closeEvent.code ?? 'unknown'}: ${closeEvent.reason ?? 'no reason provided'}`

This would make error messages more informative across all environments (browser and NodeJS).

2. Unused Variable (Minor)

The wasClean variable is assigned at line 570 but only used later at line 585 and 597. The reordering doesn't affect this, but it's worth noting that the variable extraction happens before it's needed.

Suggestion: Consider moving the wasClean extraction closer to where it's used, or document why it's extracted early (perhaps for consistency with the comment about CloseEvent properties).

Testing Considerations

⚠️ No Test Coverage Found: I couldn't locate specific tests for the #handleOnClose method. While this is a minor logging improvement, it would be valuable to have tests that verify:

• Error messages include close codes and reasons when present
• Error messages handle undefined close codes/reasons gracefully (NodeJS environment)
• The promise rejection occurs with the expected error format

Performance & Security

✅ No Performance Concerns: The change is minimal and doesn't introduce any performance overhead.

✅ No Security Concerns: The change only affects error messages and logging - no security implications.

Verdict

Approved with Minor Suggestions ✅

This is a solid quality-of-life improvement for debugging. The only suggestion is to handle potentially undefined close codes/reasons more gracefully, especially given the comment at line 568 that explicitly mentions these properties can be undefined in NodeJS environments.

The change is small, focused, and improves observability - exactly what a good chore commit should do.

---

Location References:

• Main changes: rivetkit-typescript/packages/rivetkit/src/client/actor-conn.ts:572-578
• Related logging: rivetkit-typescript/packages/rivetkit/src/client/actor-conn.ts:581-589

[claude]

Code Review

Summary

This PR improves error messaging when a WebSocket connection closes by including the close code and reason in the error message. This is a small but valuable change for debugging connection issues.

Positive Observations

1. Better Error Messages: Including the close code and reason in the error message significantly improves debuggability when connections fail.

2. Logical Code Reordering: Moving the close event variable extraction before the rejection promise is more logical - you now have access to the close code/reason when constructing the error message.

3. Consistent Logging: The structured logging already captures code, reason, and wasClean on line 581-589, so this change brings the error message in line with what's already being logged.

Suggestions

Minor: Error Message Consistency
The error message uses lowercase 'websocket' but could be more consistent. Consider capitalizing WebSocket or using connection to match other error messages in the codebase. However, this is a very minor style preference.

Minor: Handling Undefined Values
When closeEvent.code or closeEvent.reason are undefined (as mentioned in the comment about NodeJS), the error message might read awkwardly as websocket closed with code undefined: undefined.

Consider adding a fallback with nullish coalescing for default values or conditionally include the details only when they exist.

Test Coverage

I didn't find specific unit tests for the handleOnClose method in actor-conn.ts. While driver tests exist (driver-memory.test.ts, driver-engine.test.ts), they may not specifically test close event handling with various close codes.

Recommendation: Consider adding test cases that verify:

• Error message includes close code and reason when provided
• Handling of undefined close code/reason in NodeJS environment
• Various WebSocket close codes (1000, 1001, 1006, etc.)

Security and Performance

• No security concerns identified
• No performance impact - this is purely a logging/error message change

Overall Assessment

LGTM with minor suggestions - This is a good incremental improvement for developer experience. The suggestions above are optional refinements that could make the error handling more robust, especially in the NodeJS environment mentioned in the comments.

The change is:

• Well-scoped and focused
• Follows existing code patterns
• Improves observability
• Low risk

[claude]

PR Review: Log close code when client ws closes

Summary

This PR improves error reporting by including the WebSocket close code and reason in the error message when the connection closes before the open promise resolves. This is a small but valuable improvement for debugging connection issues.

Code Quality ✅

Positives:

• The change improves observability by including close code and reason in the error message
• Error message follows a clear format: websocket closed with code {code}: {reason}
• The code properly accesses closeEvent.code and closeEvent.reason which are already being logged below
• Moving the rejection logic after extracting closeEvent properties makes the code more logical

Observations:

• The error is a plain Error instance rather than a custom error class. This is consistent with the existing error at line 600 (const disconnectError = new Error(...)) and line 207, so this follows the established pattern in the file.
• The close code and reason are already logged in the structured logger call immediately after (lines 581-589), so this change adds the information to the error message for the caller while maintaining the structured logging.

Potential Issues

Minor - Error message format consistency:
The error message uses lowercase "websocket" while the existing error at line 601 uses title case: "Connection closed" / "Connection lost". Consider using consistent capitalization, perhaps: "WebSocket closed with code ${closeEvent.code}: ${closeEvent.reason}" or "Connection closed with code ${closeEvent.code}: ${closeEvent.reason}" to match the style at line 601.

Minor - Undefined close code handling:
Since the comment notes "These properties will be undefined" in NodeJS environments (line 568), the error message might show undefined values:

// Possible output: "websocket closed with code undefined: undefined"

Consider adding a fallback:

this.#onOpenPromise.reject(
  new Error(
    closeEvent.code \!== undefined
      ? `websocket closed with code ${closeEvent.code}: ${closeEvent.reason || 'no reason provided'}`
      : "websocket closed before opening"
  ),
);

Or simply check if the properties exist before including them in the message.

Security Considerations ✅

No security concerns. The close code and reason are standard WebSocket protocol values.

Performance Considerations ✅

Negligible impact - just string interpolation.

Test Coverage ⚠️

No test file exists for actor-conn.ts. Consider adding tests that verify:

• Error message includes close code when available
• Error message handles undefined close code gracefully (NodeJS environment)
• Error message includes close reason when available

Recommendations

1. Handle undefined values - Add a check or fallback for when closeEvent.code and closeEvent.reason are undefined
2. Consider consistency - Match the error message style with other error messages in the file
3. Add tests - Create test coverage for the WebSocket close handling logic

Verdict

This is a good improvement that enhances debuggability. With the minor suggestions above, it would be even better. The change is low-risk and follows existing patterns in the codebase.

Approval Status: ✅ Approved with suggestions

[claude]

Code Review

Summary

This PR improves error diagnostics by including the close code and reason in the error message when a WebSocket connection fails to open. The change moves the promise rejection to after extracting the CloseEvent properties, allowing for more informative error messages.

Positive Aspects ✓

• Improved Debugging: The error message now includes closeEvent.code and closeEvent.reason, which will significantly help with diagnosing connection failures
• Small, Focused Change: The PR does exactly one thing well - improves error messaging without introducing complexity
• Consistent Logging: The structured logging at line 581-589 already captures these details; now the error message matches that level of detail
• No Breaking Changes: The change only affects the error message content, not the error handling flow

Observations

Code Quality

The implementation is clean and straightforward. The error message format follows a clear pattern:

`websocket closed with code ${closeEvent.code}: ${closeEvent.reason}`

Potential Edge Cases

Consider these scenarios where closeEvent.code or closeEvent.reason might be undefined:

1. NodeJS Environment: The comment on line 566-568 explicitly mentions that CloseEvent properties may be undefined in NodeJS
2. Type Assertion: Line 569 uses a type assertion (as CloseEvent) which bypasses compile-time safety

Suggestion: Consider adding a fallback for when these properties are undefined:

const code = closeEvent.code ?? 'unknown';
const reason = closeEvent.reason ?? 'no reason provided';
this.#onOpenPromise.reject(
  new Error(`websocket closed with code ${code}: ${reason}`)
);

This would prevent error messages like "websocket closed with code undefined: undefined" in environments where CloseEvent properties aren't available.

Type Safety

The wasClean variable is extracted but never used after the refactor. While this doesn't cause issues, consider whether it should be:

• Removed if truly unused
• Included in the error message for additional context
• Left as-is for future use

Test Coverage

No test changes were included. Consider adding a test case to verify:

• Error messages include close codes and reasons when available
• Error handling works correctly when CloseEvent properties are undefined (NodeJS scenario)

Minor Style Notes

The error message follows good practices by being descriptive, though you might consider:

• Capitalizing the first letter: "Websocket closed with code..." for consistency with other error messages
• However, checking the codebase conventions, I see the existing pattern uses lowercase (line 563 comment mentions "Closed" but that may be legacy)

Recommendation

Approve with suggestions. The core change is solid and improves debugging. The edge case handling for undefined properties would make it more robust, but isn't blocking if you've verified the CloseEvent properties are always populated in your environment.

Great work on improving observability! 🔍

[graphite-app]

Merge activity

• Nov 5, 5:07 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 5, 5:08 AM UTC: CI is running for this pull request on a draft pull request (#3371) due to your merge queue CI optimization settings.
• Nov 5, 5:11 AM UTC: Merged by the Graphite merge queue via draft PR: #3371.