fix(rivetkit): don't throw error if websocket is not open, causing realtime to break after a connection disconnects #3316

NathanFlurry · 2025-11-03T22:20:25Z

No description provided.

vercel · 2025-11-03T22:20:32Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 4, 2025 7:25pm

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 4, 2025 7:25pm
rivet-inspector	Ignored	Preview	Nov 4, 2025 7:25pm
rivet-site	Ignored	Preview	Nov 4, 2025 7:25pm

NathanFlurry · 2025-11-03T22:20:39Z

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

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.

pkg-pr-new · 2025-11-03T22:24:11Z

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/actor@3316

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3316

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/core@3316

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3316

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3316

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3316

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3316

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3316

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3316

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3316

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3316

commit: 43ed336

claude · 2025-11-03T22:25:57Z

Pull Request Review

Summary

This PR adds a defensive check to prevent errors when attempting to send messages to closed WebSocket connections. The fix is sound and addresses a real issue where the realtime system would break after connection disconnects.

Code Quality: ✅ Good

Positive aspects:

Clean defensive programming pattern
Appropriate use of structured logging (following the codebase's tracing patterns)
Proper variable rename from _conn to conn since it's now being used

Minor suggestions:

Consider checking state before serialization (performance optimization):
The current implementation serializes the message before checking the WebSocket state. Consider moving the check earlier to avoid unnecessary work:

sendMessage: (
    actor: AnyActorInstance,
    conn: AnyConn,
    state: ConnDriverWebSocketState,
    message: CachedSerializer<protocol.ToClient>,
) => {
    // Check state first to avoid unnecessary serialization
    if (state.websocket.readyState !== ConnReadyState.OPEN) {
        actor.rLog.warn({
            msg: "attempting to send message to closed websocket, this is likely a bug in RivetKit",
            connId: conn.id,
            wsReadyState: state.websocket.readyState,
        });
        return;
    }

    const serialized = message.serialize(state.encoding);
    // ... rest of the code
}

Log message wording: The message says "this is likely a bug in RivetKit" but based on the PR title, this is actually an expected race condition scenario. Consider rewording:
```
msg: "websocket connection closed before message could be sent",
```

Potential Issues: ⚠️ Minor Concerns

Inconsistency with SSE driver: The SSE driver's sendMessage doesn't have similar defensive checks for closed/aborted streams. While SSE streams may handle this differently internally, it would be good to verify if similar protection is needed there for consistency.
Race condition timing: The check happens at the start of sendMessage, but there's a window between the check and the actual state.websocket.send() call where the connection could close. This is likely acceptable given the async nature of WebSockets, but worth documenting.

Security: ✅ No concerns

No security issues identified. The change is defensive and reduces error states.

Performance: ✅ Good

Minor optimization opportunity mentioned above (checking state before serialization), but overall impact is positive since it avoids attempting to send data over closed connections.

Test Coverage: ⚠️ No tests included

Recommendation: Consider adding a test case that:

Creates a WebSocket connection
Closes the connection
Attempts to send a message via conn.send()
Verifies that the warning is logged and no error is thrown

Example test structure:

test('sending message to closed websocket logs warning without throwing', async () => {
  // Setup actor with websocket connection
  // Close the websocket
  // Attempt to send message
  // Verify warning was logged
  // Verify no error thrown
});

Architecture Considerations: 💡 Discussion Points

Root cause: The PR description mentions this is "likely a bug in RivetKit". It would be valuable to understand:
- When/why are messages being sent to already-disconnected connections?
- Is this happening during cleanup/teardown?
- Should the calling code be fixed to not attempt sending to disconnected connections?
Consistent pattern: Consider if HTTP driver needs similar treatment. Currently it always returns ConnReadyState.OPEN (with a TODO comment), which could mask similar issues.

Verdict: ✅ Approve with minor suggestions

This is a good defensive fix that prevents crashes. The implementation is clean and follows the codebase conventions. The suggestions above are minor improvements and don't block merging.

Before merging:

Consider the performance optimization (check before serialization)
Review log message wording
Evaluate if similar protection is needed for SSE driver
Consider adding test coverage

Review conducted following CLAUDE.md conventions for the Rivet monorepo

claude · 2025-11-04T19:08:26Z

PR Review: WebSocket Connection State Guard

Summary

This PR adds a defensive check to prevent sending messages to closed WebSocket connections, fixing a bug where realtime broadcasts could attempt to send messages to disconnected clients.

Positive Feedback

✅ Addresses a Real Race Condition: The fix correctly handles the scenario where messages are broadcast to subscribed connections (line 1796 in instance.ts) after a WebSocket has been disconnected. This is a legitimate edge case in distributed/realtime systems.

✅ Good Defensive Programming: Checking readyState before sending is a solid defensive practice that prevents exceptions and maintains system stability.

✅ Proper Structured Logging: The warning log follows the project's conventions from CLAUDE.md:

Uses structured logging with separate fields (?connId, ?wsReadyState)
Lowercase message format
Includes relevant debugging context

✅ Appropriate Severity: Using warn instead of error is correct - this is an unusual but recoverable condition that indicates potential timing issues rather than a critical failure.

Suggestions for Improvement

1. Consider Consistency with Other Drivers

The SSE driver doesn't have a similar check in its sendMessage function (conn-drivers.ts:182-191). While SSE's writeSSE might handle closed streams differently, it would be worth verifying if it needs similar protection:

// SSE_DRIVER.sendMessage currently doesn't check stream state
state.stream.writeSSE({ data: encodeDataToString(message.serialize(state.encoding)) });

Could stream.writeSSE throw if called on a closed/aborted stream? If so, consider adding:

if (state.stream.aborted || state.stream.closed) {
  actor.rLog.warn({ msg: "attempting to send message to closed SSE stream", connId: conn.id });
  return;
}

2. State Check Could Be More Comprehensive

Currently only checks for OPEN. Consider checking for all non-open states explicitly:

if (state.websocket.readyState \!== ConnReadyState.OPEN) {
  // Current implementation - good\!
}

This is already correct, but you might want to add more context about why it's not open:

actor.rLog.warn({
  msg: "attempting to send message to closed websocket, this is likely a bug in RivetKit",
  connId: conn.id,
  wsReadyState: state.websocket.readyState,
  wsReadyStateName: ['CONNECTING', 'OPEN', 'CLOSING', 'CLOSED'][state.websocket.readyState] // More readable
});

3. Root Cause Investigation

The log message says "this is likely a bug in RivetKit". While the defensive check is good, consider if there's a root cause to address:

Should connections be removed from subscription sets immediately on disconnect?
Is there a cleanup race condition between connection close handlers and broadcast logic?
Could we use getConnectionReadyState (line 155-161) to filter out stale connections before broadcast?

Example potential improvement in instance.ts around line 1793-1797:

// Send message to clients
for (const connection of subscriptions) {
  // Skip connections that aren't ready to receive messages
  const readyState = connection.__driverState ? 
    CONN_DRIVERS[getConnDriverKindFromState(connection.__driverState)]
      .getConnectionReadyState(...) : undefined;
  
  if (readyState === ConnReadyState.OPEN) {
    connection._sendMessage(toClientSerializer);
  }
}

Testing Considerations

Since there are no direct tests for conn-drivers.ts, consider:

Manual Testing: Verify the fix works by:
- Connecting a client
- Subscribing to a topic
- Disconnecting the client
- Triggering a broadcast to that topic
- Confirming the warning appears without breaking realtime for other connections
Future Test Coverage: Consider adding integration tests for connection lifecycle edge cases, particularly around disconnection during active subscriptions.

Security & Performance

✅ No Security Concerns: The change is purely defensive and doesn't introduce any security issues.

✅ No Performance Impact: The readyState check is O(1) and happens before any serialization work, so it actually improves performance by short-circuiting unnecessary work.

Code Quality

✅ Follows CLAUDE.md Conventions:

Structured logging format ✓
Lowercase log messages ✓
Proper parameter naming (changed _conn to conn) ✓

Conclusion

Recommendation: Approve with minor suggestions

This is a solid defensive fix that addresses a real issue. The main suggestions are:

Check if SSE driver needs similar protection
Consider investigating the root cause to see if cleanup can be improved
Add the human-readable state name to logs for easier debugging

The PR is ready to merge as-is, but the suggestions above would make it even better.

Great work catching this race condition! 🎯

claude · 2025-11-04T19:09:49Z

Code Review - PR #3316

Overview

This PR adds a defensive check to prevent attempting to send messages over closed WebSocket connections, which was causing realtime functionality to break after disconnections.

✅ Positive Aspects

Correct Problem Identification: The fix addresses a real issue where attempting to send messages to closed WebSockets would throw errors and break realtime functionality.
Good Logging: The warning message provides helpful debugging information including the connection ID and WebSocket ready state.
Consistent with Pattern: The SSE driver already checks connection state in getConnectionReadyState before sending, so this brings WebSocket handling closer to that pattern.
Low Risk: The change is minimal and defensive - early return on closed connections is safe.

🔍 Observations & Suggestions

1. Root Cause Analysis

The comment states "this is likely a bug in RivetKit" - it would be valuable to understand when/why sendMessage is being called on closed connections:

Is this happening during reconnection flows (lines 1017 in instance.ts)?
Is this a race condition between disconnect and broadcast operations (line 1796)?
Should the caller be checking connection status before calling _sendMessage?

2. Consistency with SSE Driver

The SSE driver doesn't have a similar check in its sendMessage function (lines 182-190). Consider:

Should SSE also validate stream state before sending?
The SSE driver checks state.stream.aborted || state.stream.closed in getConnectionReadyState - should WebSocket use the same approach?

3. Consider Checking Connection Status Earlier

Looking at conn.ts:183-206, the _sendMessage method already has defensive checks for missing __driverState. Consider:

Should _sendMessage also check the ready state via getConnectionReadyState() before delegating to drivers?
This would centralize the check and apply it consistently across all driver types.

Example:

public _sendMessage(message: CachedSerializer<protocol.ToClient>) {
	if (this.__driverState) {
		const driverKind = getConnDriverKindFromState(this.__driverState);
		const driver = CONN_DRIVERS[driverKind];
		
		// Check connection state before sending
		const readyState = driver.getConnectionReadyState?.(this.#actor, this, (this.__driverState as any)[driverKind]);
		if (readyState !== ConnReadyState.OPEN) {
			this.#actor.rLog.warn({
				msg: "attempting to send message to non-open connection",
				conn: this.id,
				readyState,
			});
			return;
		}
		
		if (driver.sendMessage) {
			// ... rest of logic
		}
	}
}

4. Missing Test Coverage

There are no tests covering this scenario. Consider adding tests for:

Attempting to send messages after disconnect() is called
Race conditions between broadcast and disconnect
Verifying the warning is logged appropriately

🐛 Potential Issues

None identified - the change is safe and defensive.

🎯 Performance Considerations

Minimal impact - adds one comparison operation before message sending, which is negligible.

🔒 Security Concerns

None identified - this is a defensive fix that prevents errors.

📋 Recommendations

Short-term (this PR):

✅ Approve and merge as-is - this is a good defensive fix

Follow-up considerations:

Investigate the root cause: Why are we calling sendMessage on closed connections? Is this expected during reconnection flows?
Consider centralizing the ready state check in Conn._sendMessage() rather than in each driver
Add test coverage for disconnection edge cases
Consider adding similar checks to the SSE driver for consistency

Summary

This is a good defensive fix that prevents crashes when sending to closed WebSockets. The change is low-risk and improves reliability. However, it might be treating a symptom rather than the root cause - understanding why messages are being sent to closed connections would help determine if additional fixes are needed upstream.

Recommendation: ✅ Approve and merge

claude · 2025-11-04T19:35:35Z

Code Review for PR #3316

Summary

This PR adds a defensive check in the WebSocket driver to prevent errors when attempting to send messages to closed WebSocket connections. The fix addresses a bug where the realtime connection would break after a WebSocket disconnection.

Positive Aspects

✅ Correct Fix: The change properly checks readyState before attempting to send, preventing exceptions when the socket is not open
✅ Good Logging: The warning message is structured and includes relevant context (connId, wsReadyState) for debugging
✅ Minimal Change: The fix is surgical and doesn't introduce unnecessary complexity
✅ Early Return: Properly returns early instead of throwing, allowing the system to continue operating

Code Quality Observations

1. Consistency with Other Drivers

The SSE driver in this same file already has connection state checking in getConnectionReadyState (lines 202-212), which checks stream.aborted or stream.closed before allowing operations. However, SSE's sendMessage function doesn't have a guard check - it just attempts to write.

Recommendation: For consistency, consider whether the SSE driver should also have a similar guard in its sendMessage function. This would prevent potential errors if messages are sent to closed SSE streams.

2. Potential Race Condition

There's a subtle race condition: the WebSocket state could change between the readyState check (line 90) and the actual send() call (lines 125, 131, 138). While unlikely, if the WebSocket closes between these operations, the send could still throw.

Mitigation: The current approach is reasonable for most cases. If this becomes an issue, wrapping the entire send operation in a try-catch might be safer:

try {
    const serialized = message.serialize(state.encoding);
    // ... existing send logic
    state.websocket.send(data);
} catch (error) {
    actor.rLog.warn({
        msg: "failed to send message to websocket",
        connId: conn.id,
        error: error.message,
    });
}

3. Root Cause Analysis

The warning message states "this is likely a bug in RivetKit" - this suggests there might be a deeper issue where messages are being queued or sent after disconnect.

Questions to Consider:

Is there a message queue that should be cleared on disconnect?
Should _sendMessage in conn.ts:305 check connection status before calling the driver?
Are there lifecycle guarantees about when sendMessage can be called?

Looking at the code, I notice that _sendMessage checks if (this.__driverState) but doesn't verify the connection is actually open. You might want to add a more comprehensive check there:

// In conn.ts _sendMessage method
public _sendMessage(message: CachedSerializer<protocol.ToClient>) {
    if (this.__driverState) {
        const driverKind = getConnDriverKindFromState(this.__driverState);
        const driver = CONN_DRIVERS[driverKind];
        
        // Check if connection is actually ready
        const readyState = driver.getConnectionReadyState?.(this.#actor, this, (this.__driverState as any)[driverKind]);
        if (readyState !== ConnReadyState.OPEN) {
            // Log or silently skip
            return;
        }
        
        if (driver.sendMessage) {
            driver.sendMessage(/* ... */);
        }
    }
}

This would be a more centralized fix that benefits all driver types.

4. Test Coverage

No tests were added for this bug fix.

Recommendation: Consider adding a test case that:

Creates a WebSocket connection
Closes the connection
Attempts to send a message
Verifies no exception is thrown and a warning is logged

Example test location: rivetkit-typescript/packages/rivetkit/tests/websocket-closed-send.test.ts

Security Considerations

✅ No security concerns identified. The change is defensive and reduces error surface area.

Performance Considerations

✅ Minimal performance impact. The readyState check is a simple property access.

Breaking Changes

✅ None. This is purely additive/defensive behavior.

Final Recommendation

APPROVE ✅

This is a good defensive fix that addresses the immediate issue. The code quality is solid with proper structured logging.

Optional follow-ups (not blockers):

Consider adding similar guards to the SSE driver for consistency
Evaluate whether connection state checking should be centralized in conn.ts
Add test coverage for this scenario
Investigate whether message queuing/lifecycle could prevent this scenario entirely

Great work on the structured logging and minimal approach!

claude · 2025-11-04T19:39:02Z

PR Review: Fix WebSocket Error on Closed Connection

Summary

This PR adds a defensive check to prevent errors when attempting to send messages to closed WebSocket connections. The change is sound and addresses a real race condition issue.

Code Quality ✅

Strengths:

Defensive programming: The guard clause properly prevents operations on closed connections
Good logging: The warning message provides useful diagnostic information including connId and wsReadyState
Minimal change: The fix is targeted and does not introduce unnecessary complexity
Structured logging: Follows the codebase tracing pattern with structured fields instead of formatted strings

Minor suggestions:

Log message clarity: The message says this is likely a bug in RivetKit, but based on the PR title, this seems like an expected race condition when connections disconnect. Consider updating the message to reflect this is expected behavior during concurrent disconnects.
Consider consistency: The SSE driver (lines 181-217) and HTTP driver (lines 220-232) do not have similar checks. While SSE might handle this gracefully due to stream.aborted/closed checks elsewhere, it would be worth verifying if similar checks are needed.

Potential Issues 🔍

Race condition handling:
The fix is good, but there is a subtle TOCTOU (time-of-check-time-of-use) issue. The readyState could change between the check (line 90) and the actual send() call (lines 125, 131, 138). WebSocket sends might still throw if the connection closes between the check and send.

Recommendation: Consider wrapping the actual state.websocket.send() calls in a try-catch block to handle any exceptions that might occur if the connection closes between the readyState check and the send operation.

Test Coverage 🧪

Missing tests: I could not find any test files for connection drivers. This is a race condition fix that would benefit greatly from tests for sending a message after websocket closes, concurrent disconnect + send scenarios, and verifying the warning is properly logged.

Consider adding integration tests in packages/rivetkit/src/driver-test-suite/tests/ following the pattern in raw-websocket.ts.

Performance Considerations ✅

The added check is minimal (single enum comparison) and will not impact performance. Early return on closed connections is actually more efficient than attempting to send.

Security Concerns ✅

No security concerns. The change is purely defensive and does not expose any new surfaces.

Final Verdict 🎯

Approve with suggestions

The fix is correct and addresses the issue described in the PR title. The code follows the repository patterns and conventions. My suggestions are minor improvements rather than blockers. The PR is good to merge as-is, but the above improvements would make the codebase more robust.

graphite-app · 2025-11-04T20:09:50Z

Merge activity

Nov 4, 8:09 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 4, 8:11 PM UTC: CI is running for this pull request on a draft pull request (#3349) due to your merge queue CI optimization settings.
Nov 4, 8:41 PM UTC: The Graphite merge queue removed this pull request due to removal of a downstack PR #3342.
Nov 4, 8:49 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 4, 8:50 PM UTC: CI is running for this pull request on a draft pull request (#3352) due to your merge queue CI optimization settings.
Nov 4, 8:52 PM UTC: Merged by the Graphite merge queue via draft PR: #3352.

…altime to break after a connection disconnects (#3316)

…altime to break after a connection disconnects

vercel · 2025-11-04T20:25:28Z

Deployment failed with the following error:

Too many requests - try again in 5 minutes (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

claude · 2025-11-04T20:46:00Z

Code Review

Thank you for this fix! The change addresses a race condition where messages could be sent to a closed WebSocket connection. Here's my detailed review:

✅ Positive Aspects

Correct Problem Identification: The fix properly addresses a race condition where sendMessage could be called after a WebSocket has already been disconnected. This can happen because:
- Disconnection is handled asynchronously (see __connDisconnected in instance.ts:869)
- Event handlers might still fire after disconnection begins
- The connection state transitions aren't always synchronous
Appropriate Error Handling: Using a warning log instead of throwing is the right approach here, since:
- Throwing would break the realtime system (as noted in the PR title)
- This is a recoverable situation (the message simply can't be sent)
- It provides visibility for debugging without disrupting the application
Helpful Debugging Context: The warning includes connId and wsReadyState, which will help diagnose issues in production.

🔍 Suggestions for Improvement

Log Message Accuracy: The current log message says "this is likely a bug in RivetKit", but based on the async nature of the system, this seems like an expected race condition rather than a bug. Consider updating the message to something like:
```
msg: "attempted to send message to closed websocket (likely due to race condition during disconnect)"
```
Consider Early Return in Caller: Looking at conn.ts:183-206, the _sendMessage method already has guards for missing __driverState. Consider whether it would be cleaner to check the ready state there as well, consolidating the defensive checks in one place. However, the current approach (checking in the driver) is also valid and keeps driver-specific logic encapsulated.
Test Coverage: This appears to be a race condition fix without accompanying tests. Consider adding a test case that:
- Establishes a WebSocket connection
- Initiates a disconnect
- Attempts to send a message immediately after
- Verifies the warning is logged and no error is thrown
Example test location: rivetkit-typescript/packages/rivetkit/tests/
SSE Driver Consistency: The SSE driver doesn't have similar protection in its sendMessage function (line 182-190). While SSE may have different semantics, consider whether it needs similar defensive checks.

📋 Additional Observations

Consistent with existing patterns: The check mirrors the logic in getConnectionReadyState (line 155-160), which also uses state.websocket.readyState.
No security concerns: This is a defensive programming improvement with no security implications.
Performance impact: Minimal - adds one comparison before sending each message, which is negligible.

🎯 Recommendation

Approve with minor suggestions. The fix is solid and addresses the root cause. The suggestions above are nice-to-haves that could be addressed in follow-up PRs if desired.

The change correctly prevents the error that was breaking realtime functionality when WebSockets disconnect, which is the primary goal.

claude · 2025-11-04T20:46:27Z

Code Review

Summary

This PR adds a guard to prevent sending messages to closed WebSocket connections, which was causing RivetKit's realtime functionality to break after connection disconnects. The fix is straightforward and addresses a real bug.

Positive Changes ✅

Defensive programming: The added check prevents attempting to send messages on closed WebSockets, which would throw errors
Logging: Good use of structured logging with relevant context (connId, wsReadyState)
Consistency: This pattern aligns with similar checks in other parts of the codebase (e.g., inline-websocket-adapter2.ts:102, hono-websocket-adapter.ts:80)

Suggestions for Improvement

1. Consider consistency with SSE driver

The SSE driver doesn't have a similar guard in its sendMessage function. While SSE has getConnectionReadyState, it might benefit from a similar defensive check to prevent writing to closed/aborted streams.

Current SSE driver (lines 182-190):

sendMessage: (
    _actor: AnyActorInstance,
    _conn: AnyConn,
    state: ConnDriverSseState,
    message: CachedSerializer<protocol.ToClient>,
) => {
    state.stream.writeSSE({
        data: encodeDataToString(message.serialize(state.encoding)),
    });
},

Suggestion: Consider adding a similar check:

if (state.stream.aborted || state.stream.closed) {
    actor.rLog.warn({
        msg: "attempting to send message to closed SSE stream",
        connId: conn.id,
    });
    return;
}

2. Log message clarity

The log message says "this is likely a bug in RivetKit" - while this might be true, it could cause confusion for users. Consider:

Adding more context about why this might happen (e.g., "connection closed but message still in queue")
Or softening the language to "may indicate a race condition"

3. Test coverage

This appears to be a bug fix for a race condition. Consider adding a test that:

Establishes a WebSocket connection
Closes the connection
Attempts to send a message
Verifies the warning is logged and no error is thrown

This would prevent regression and document the expected behavior.

Minor Observations

The renamed parameter from _conn to conn is good - it's now being used in the log statement
The guard returns early, which is the right approach rather than throwing an error

Security & Performance

✅ No security concerns
✅ No performance impact (early return is efficient)
✅ Prevents error propagation that could destabilize the realtime system

Overall Assessment

This is a solid defensive fix that prevents a real issue. The implementation is clean and follows existing patterns in the codebase. The main opportunities for improvement are:

Consider similar protection in the SSE driver
Add test coverage for this edge case
Minor log message refinement

Recommendation: Approve with suggestions - The fix is good to merge as-is, but the suggestions above would make it even more robust.

…altime to break after a connection disconnects (#3316)

vercel bot deployed to Preview – rivetkit-serverless November 3, 2025 22:22 View deployment

MasterPtato approved these changes Nov 4, 2025

View reviewed changes

This was referenced Nov 4, 2025

fix(guard): include method in cache key #3338

Closed

chore(guard): simplify gateway routing #3339

Closed

chore(guard): add support for routing runner ws to /runners/connect #3340

Closed

chore(guard): handle cors on the gateway #3341

Closed

NathanFlurry force-pushed the 11-02-fix_rivetkit_don_t_throw_error_if_websocket_is_not_open_causing_realtime_to_break_after_a_connection_disconnects branch from 20316f6 to 8872a11 Compare November 4, 2025 18:59

NathanFlurry force-pushed the 11-02-chore_smoke-test_add_simple_http_test branch from 358af35 to 5d3cea0 Compare November 4, 2025 18:59

vercel bot deployed to Preview – rivetkit-serverless November 4, 2025 19:00 View deployment

NathanFlurry force-pushed the 11-02-chore_smoke-test_add_simple_http_test branch from 5d3cea0 to d418443 Compare November 4, 2025 19:24

NathanFlurry force-pushed the 11-02-fix_rivetkit_don_t_throw_error_if_websocket_is_not_open_causing_realtime_to_break_after_a_connection_disconnects branch from 8872a11 to cc31979 Compare November 4, 2025 19:24

vercel bot deployed to Preview – rivetkit-serverless November 4, 2025 19:25 View deployment

This was referenced Nov 4, 2025

chore(rivetkit): debounce ws message ack #3345

Closed

chore(rivetkit): auto-update auto-start engine version #3344

Closed

ci: trigger rust ci for generated bare artifacts #3346

Closed

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025

fix(rivetkit): don't throw error if websocket is not open, causing re…

38de3b2

…altime to break after a connection disconnects (#3316)

fix(rivetkit): don't throw error if websocket is not open, causing re…

43ed336

…altime to break after a connection disconnects

NathanFlurry force-pushed the 11-02-fix_rivetkit_don_t_throw_error_if_websocket_is_not_open_causing_realtime_to_break_after_a_connection_disconnects branch from cc31979 to 43ed336 Compare November 4, 2025 20:25

NathanFlurry force-pushed the 11-02-chore_smoke-test_add_simple_http_test branch from d418443 to 74873cd Compare November 4, 2025 20:25

graphite-app bot pushed a commit that referenced this pull request Nov 4, 2025

fix(rivetkit): don't throw error if websocket is not open, causing re…

ca2811b

…altime to break after a connection disconnects (#3316)

graphite-app bot closed this Nov 4, 2025

graphite-app bot deleted the 11-02-fix_rivetkit_don_t_throw_error_if_websocket_is_not_open_causing_realtime_to_break_after_a_connection_disconnects branch November 4, 2025 20:52

fix(rivetkit): don't throw error if websocket is not open, causing realtime to break after a connection disconnects #3316

fix(rivetkit): don't throw error if websocket is not open, causing realtime to break after a connection disconnects #3316

Uh oh!

Conversation

NathanFlurry commented Nov 3, 2025

Uh oh!

vercel bot commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NathanFlurry commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

pkg-pr-new bot commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Nov 3, 2025

Pull Request Review

Summary

Code Quality: ✅ Good

Potential Issues: ⚠️ Minor Concerns

Security: ✅ No concerns

Performance: ✅ Good

Test Coverage: ⚠️ No tests included

Architecture Considerations: 💡 Discussion Points

Verdict: ✅ Approve with minor suggestions

Uh oh!

claude bot commented Nov 4, 2025

PR Review: WebSocket Connection State Guard

Summary

Positive Feedback

Suggestions for Improvement

1. Consider Consistency with Other Drivers

2. State Check Could Be More Comprehensive

3. Root Cause Investigation

Testing Considerations

Security & Performance

Code Quality

Conclusion

Uh oh!

claude bot commented Nov 4, 2025

Code Review - PR #3316

Overview

✅ Positive Aspects

🔍 Observations & Suggestions

1. Root Cause Analysis

2. Consistency with SSE Driver

3. Consider Checking Connection Status Earlier

4. Missing Test Coverage

🐛 Potential Issues

🎯 Performance Considerations

🔒 Security Concerns

📋 Recommendations

Summary

Uh oh!

claude bot commented Nov 4, 2025

Code Review for PR #3316

Summary

Positive Aspects

Code Quality Observations

1. Consistency with Other Drivers

2. Potential Race Condition

3. Root Cause Analysis

4. Test Coverage

Security Considerations

Performance Considerations

Breaking Changes

Final Recommendation

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Fix WebSocket Error on Closed Connection

Summary

Code Quality ✅

Potential Issues 🔍

Test Coverage 🧪

Performance Considerations ✅

Security Concerns ✅

Final Verdict 🎯

Uh oh!

graphite-app bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

vercel bot commented Nov 3, 2025 •

edited

Loading

NathanFlurry commented Nov 3, 2025 •

edited

Loading

pkg-pr-new bot commented Nov 3, 2025 •

edited

Loading

graphite-app bot commented Nov 4, 2025 •

edited

Loading