fix(rivetkit): skip sending RivetKit messages to conns that do not support it #3440

NathanFlurry · 2025-11-10T21:31:47Z

No description provided.

vercel · 2025-11-10T21:31:53Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 13, 2025 9:33pm

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 13, 2025 9:33pm
rivet-inspector	Ignored	Preview	Nov 13, 2025 9:33pm
rivet-site	Ignored	Preview	Nov 13, 2025 9:33pm

NathanFlurry · 2025-11-10T21:32:14Z

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.

claude · 2025-11-10T21:33:05Z

Code Review for PR #3440

Summary

This PR introduces a protocol capability check for connection drivers to prevent RivetKit protocol messages from being sent to connections that don't support it. This is a good architectural improvement that makes the connection driver interface more explicit about protocol support.

Positive Changes

✅ Clear Protocol Separation: Moving sendMessage into a rivetKitProtocol optional interface makes it explicit which drivers support the RivetKit protocol vs. raw connections.

✅ Prevents Protocol Mismatches: Properly guards against sending RivetKit messages (Init, Event, etc.) to raw HTTP/WebSocket connections that don't understand them.

✅ Consistent Pattern: The check connection[CONN_SPEAKS_RIVETKIT_SYMBOL]() is consistently applied in all the right places:

connection-manager.ts:160 - Init message
event-manager.ts:219 - Event broadcasts
mod.ts:208 - Individual connection sends

✅ Good Logging: Appropriate warning logs when attempting to send to incompatible connections.

Issues & Concerns

1. Bug in `conn/mod.ts:208-214` ⚠️

send(eventName: string, ...args: unknown[]) {
    this.#assertConnected();
    if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]) {
        this.#actor.rLog.warn({
            msg: "cannot send messages to this connection type",
            connId: this.id,
            connType: this[CONN_DRIVER_SYMBOL]?.type,
        });
    }
    // ... continues to send anyway
}

Problem: The check logs a warning but doesn't return early. This means the code continues to send the message even when the connection doesn't support RivetKit protocol.

Fix: Add an early return after the warning:

if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]()) {  // Also note: missing () call
    this.#actor.rLog.warn({
        msg: "cannot send messages to this connection type",
        connId: this.id,
        connType: this[CONN_DRIVER_SYMBOL]?.type,
    });
    return;  // Add this
}

Also note you're missing the function call () on line 208 - it should be this[CONN_SPEAKS_RIVETKIT_SYMBOL]() not this[CONN_SPEAKS_RIVETKIT_SYMBOL].

2. Typo in Symbol Access 🐛

Line 208 in mod.ts: if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]) should be if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]()) - you're checking the function reference instead of calling it.

The correct implementation is on line 66:

[CONN_SPEAKS_RIVETKIT_SYMBOL](): boolean {
    return this[CONN_DRIVER_SYMBOL]?.rivetKitProtocol !== undefined;
}

3. Silent Failures in Event Broadcasting 🔕

In event-manager.ts:219, when a connection doesn't speak RivetKit, the event is silently skipped. While this might be intentional, it could lead to confusing behavior:

for (const connection of subscribers) {
    if (connection[CONN_SPEAKS_RIVETKIT_SYMBOL]) {  // Silent skip
        try {
            connection[CONN_SEND_MESSAGE_SYMBOL](toClientSerializer);
            sentCount++;
        } catch (error) { ... }
    }
}

Consideration: Should we log a debug message when skipping subscribers? Or should raw connections not be allowed to subscribe to events in the first place?

4. Missing Test Coverage 📝

The changes don't include any tests. Consider adding tests for:

WebSocket driver with rivetKitProtocol defined properly sends messages
HTTP driver without rivetKitProtocol doesn't receive Init/Event messages
conn.send() returns early when connection doesn't support RivetKit
Event broadcasts skip non-RivetKit connections

Minor Suggestions

Documentation: Consider adding JSDoc comments to the rivetKitProtocol interface explaining when a driver should implement it vs. when it shouldn't.

Type Safety: The CONN_SPEAKS_RIVETKIT_SYMBOL method could be simplified since it's just checking for presence:

[CONN_SPEAKS_RIVETKIT_SYMBOL](): this is Conn<...> & { [CONN_DRIVER_SYMBOL]: ConnDriver & { rivetKitProtocol: NonNullable<...> } } {
    return this[CONN_DRIVER_SYMBOL]?.rivetKitProtocol !== undefined;
}

Though this might be overly complex for the benefit.

Security Considerations

✅ No security concerns identified. The change properly validates protocol compatibility before sending messages.

Performance Considerations

✅ Minimal performance impact. The checks are simple property accesses and are appropriate guards before more expensive serialization operations.

Overall Assessment

This is a valuable refactoring that makes the driver interface more explicit and prevents protocol mismatches. However, the bug in mod.ts:208-214 must be fixed before merging - currently the send() method will still attempt to send messages to incompatible connections despite the warning.

Recommendation: Fix the two bugs mentioned above, then this is ready to merge. Consider adding basic test coverage for the protocol capability checks.

pkg-pr-new · 2025-11-10T21:34:17Z

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 51a32d6

vercel · 2025-11-13T20:54:05Z

Deployment failed with the following error:

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

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

claude · 2025-11-13T21:03:10Z

PR Review: Skip sending RivetKit messages to conns that do not support it

Overview

This PR introduces a protocol capability check for RivetKit connections, ensuring that RivetKit-specific messages (Init, subscription events, etc.) are only sent to connections that support the RivetKit protocol. This is a well-architected solution that uses capability-based design.

✅ Strengths

Clean Architecture: The introduction of rivetKitProtocol as an optional capability on ConnDriver is excellent design. This follows the Interface Segregation Principle and makes the protocol support explicit.
Consistent Symbol Pattern: Using CONN_SPEAKS_RIVETKIT_SYMBOL fits well with the existing codebase patterns (CONN_CONNECTED_SYMBOL, CONN_PERSIST_SYMBOL, etc.).
Defensive Programming: Multiple guard checks prevent sending messages to unsupported connections, with appropriate logging at different levels.
Backward Compatibility: The changes are additive and don't break existing connection drivers.

🐛 Potential Issues

1. Missing Return Statement (rivetkit-typescript/packages/rivetkit/src/actor/conn/mod.ts:208)

send(eventName: string, ...args: unknown[]) {
    this.#assertConnected();
    if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]) {
        this.#actor.rLog.warn({
            msg: "cannot send messages to this connection type",
            connId: this.id,
            connType: this[CONN_DRIVER_SYMBOL]?.type,
        });
    }
    // Execution continues even if connection doesn't support RivetKit!

Issue: The check logs a warning but doesn't return early. This means the code continues to execute eventFired inspector event and potentially other operations even though the connection can't receive the message.

Fix: Add an early return:

if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]()) {
    this.#actor.rLog.warn({
        msg: "cannot send messages to this connection type",
        connId: this.id,
        connType: this[CONN_DRIVER_SYMBOL]?.type,
    });
    return;
}

2. Method Invocation Inconsistency (rivetkit-typescript/packages/rivetkit/src/actor/conn/mod.ts:208)

if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]) {  // Missing () - checking if method exists

Issue: This checks if the method exists, not if it returns true. Should be this[CONN_SPEAKS_RIVETKIT_SYMBOL]() to invoke it.

3. Silent Event Skipping (rivetkit-typescript/packages/rivetkit/src/actor/instance/event-manager.ts:219)

for (const connection of subscribers) {
    if (connection[CONN_SPEAKS_RIVETKIT_SYMBOL]) {  // Missing ()
        // send message
    }
    // Silently skips non-supporting connections
}

Issues:

Same invocation issue as [SVC-2479] Send cluster events to PostHog #2 - missing ()
Silently skips connections without any logging or metrics. This could mask issues where developers expect messages to be delivered.

Suggestion: Add debug logging when skipping:

if (connection[CONN_SPEAKS_RIVETKIT_SYMBOL]()) {
    // send message
} else {
    this.#actor.rLog.debug({
        msg: "skipping event for non-RivetKit connection",
        eventName: name,
        connId: connection.id,
        connType: connection[CONN_DRIVER_SYMBOL]?.type,
    });
}

🔍 Minor Observations

Log Message Clarity (rivetkit-typescript/packages/rivetkit/src/actor/conn/driver.ts:47): The updated message "attempting to send RivetKit protocol message to connection that does not support it" is clearer than the old one - good improvement.
Indentation Consistency: The nested structure in websocket.ts for rivetKitProtocol.sendMessage maintains proper indentation.

🚀 Performance Considerations

Positive: Filtering at multiple levels (driver check, symbol check, event loop) prevents unnecessary serialization and transmission attempts.
Minimal Overhead: The symbol-based method lookup is O(1) and negligible.

🔒 Security Concerns

No security issues identified. The capability-based approach actually improves security by making protocol support explicit and preventing accidental message leakage to unsupported connections.

🧪 Test Coverage Recommendations

This PR would benefit from tests covering:

Connection Type Filtering:
- Verify Init messages are only sent to RivetKit-supporting connections
- Verify events are only sent to RivetKit-supporting connections
- Verify mixed connection pools (some supporting, some not) behave correctly
Logging Verification:
- Ensure appropriate warnings/debug messages are logged when skipping connections
Edge Cases:
- Connection becoming disconnected between capability check and send
- Connection driver without rivetKitProtocol defined

📝 Recommended Changes

Critical:

Fix method invocation: this[CONN_SPEAKS_RIVETKIT_SYMBOL]() (with parentheses) in mod.ts:208 and event-manager.ts:219
Add early return in send() method after the warning log

Nice to have:
3. Add debug logging when skipping non-RivetKit connections in event-manager.ts
4. Add tests as outlined above

Summary

This is a solid architectural improvement with clear separation of concerns. The main issues are minor (missing method invocation parentheses and return statement) but critical to fix as they would cause the feature not to work as intended. Once these are addressed, this PR will be in excellent shape.

Recommendation: Request changes for the critical fixes, then approve.

…pport it

claude · 2025-11-13T21:34:34Z

Code Review: Skip sending RivetKit messages to connections that do not support it

Overview

This PR introduces a cleaner separation between connections that speak the RivetKit protocol and those that don't (raw HTTP/WebSocket connections). The approach is solid and follows good design patterns.

✅ Strengths

1. Clear API Design
The introduction of the optional rivetKitProtocol property on ConnDriver makes the protocol support explicit and discoverable. This is much cleaner than the previous optional sendMessage method.

rivetKitProtocol?: {
    sendMessage(actor, conn, message): void;
}

2. Consistent Pattern
The PR correctly updates all connection drivers:

websocket.ts - implements rivetKitProtocol ✓
http.ts - omits it (no protocol support) ✓
raw-request.ts - omits it (no protocol support) ✓
raw-websocket.ts - already had no sendMessage ✓

3. Guard Clauses
The code properly checks for protocol support before sending messages in:

connection-manager.ts:159-182 - Init message
event-manager.ts:218 - Event subscriptions
mod.ts:208-214 - Direct send() calls

🐛 Potential Issues

1. Logic Error in Conn.send() (mod.ts:208-214)

if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]) {
    this.#actor.rLog.warn({ /* ... */ });
}
// Code continues to send message anyway! 🚨

Issue: The warning is logged but execution continues, attempting to send the message even when the connection doesn't support RivetKit protocol. This will trigger the debug log at line 187.

Fix: Add an early return:

if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]()) {  // Note: missing () in current code
    this.#actor.rLog.warn({
        msg: "cannot send messages to this connection type",
        connId: this.id,
        connType: this[CONN_DRIVER_SYMBOL]?.type,
    });
    return;  // ← Add this
}

2. Missing Function Call Parentheses (mod.ts:208)

if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]) {  // ❌ Symbol, not function result

Should be:

if (!this[CONN_SPEAKS_RIVETKIT_SYMBOL]()) {  // ✅ Function call

The CONN_SPEAKS_RIVETKIT_SYMBOL is a method (line 66-68), so it needs to be invoked with ().

💡 Suggestions

1. Type Safety Enhancement

Consider making CONN_SPEAKS_RIVETKIT_SYMBOL a getter instead of a method for more idiomatic TypeScript:

get [CONN_SPEAKS_RIVETKIT_SYMBOL](): boolean {
    return this[CONN_DRIVER_SYMBOL]?.rivetKitProtocol !== undefined;
}

Then usage becomes:

if (conn[CONN_SPEAKS_RIVETKIT_SYMBOL]) { /* ... */ }

This matches the pattern used for other symbol-based accessors in the class.

2. Documentation

Consider adding a JSDoc comment to rivetKitProtocol explaining which driver types should implement it:

/**
 * If defined, this connection driver talks the RivetKit client protocol (see
 * schemas/client-protocol/).
 *
 * Should be implemented by: websocket driver
 * Should NOT be implemented by: raw-websocket, http, raw-request drivers
 */

3. Test Coverage

No tests were modified/added in this PR. Consider adding tests for:

Verifying send() doesn't attempt to send to non-RivetKit connections
Verifying Init messages are only sent to RivetKit connections
Verifying event subscriptions only send to RivetKit connections

🔒 Security Considerations

No security concerns identified. The changes properly prevent protocol messages from being sent to unsupported connection types, which could otherwise cause errors or unexpected behavior.

⚡ Performance Considerations

The changes add minimal overhead (simple boolean checks) and actually improve performance by avoiding unnecessary serialization and send attempts for connections that don't support the protocol.

Summary

Critical Issues: 2

Missing function call parentheses on CONN_SPEAKS_RIVETKIT_SYMBOL check
Missing early return in send() method

Recommendations:

Fix the two critical issues above
Consider converting method to getter for better API consistency
Add test coverage for the new behavior

Once the critical issues are addressed, this is a solid improvement to the codebase! 👍

This was referenced Nov 10, 2025

fix(rivetkit): fix race condition with websocket open events #3439

Closed

fix(rivetkit): properly handle msgIndex for hibernatable websocket reconnection #3401

Closed

This was referenced Nov 10, 2025

fix(rivetkit): remove incorrect getEndpoint call in metadata handler #3433

Closed

fix(rivetkit): standardize error metadata return type #3438

Merged

vercel bot deployed to Preview – rivetkit-serverless November 10, 2025 21:33 View deployment

NathanFlurry changed the base branch from 11-10-fix_rivetkit_fix_race_condition_with_websocket_open_events to graphite-base/3440 November 10, 2025 21:58

NathanFlurry mentioned this pull request Nov 13, 2025

chore(rivetkit): switch dynamic node imports to use require #3461

Merged

NathanFlurry changed the base branch from 11-10-fix_rivetkit_fix_race_condition_with_websocket_open_events to graphite-base/3440 November 13, 2025 03:35

NathanFlurry mentioned this pull request Nov 13, 2025

chore(rivetkit): fix type checks #3462

Merged

NathanFlurry changed the base branch from graphite-base/3440 to 11-10-fix_rivetkit_fix_race_condition_with_websocket_open_events November 13, 2025 17:13

NathanFlurry changed the base branch from 11-10-fix_rivetkit_fix_race_condition_with_websocket_open_events to graphite-base/3440 November 13, 2025 17:20

This was referenced Nov 13, 2025

feat(gas): implement smart pulling for horizontally scaled workers #3417

Merged

fix(guard): handle websocket tasks during shutdown #3448

Merged

fix: improve sigterm handling for the entire runtime #3443

Merged

NathanFlurry mentioned this pull request Nov 13, 2025

feat(gateway): websocket hibernation #3400

Merged

This was referenced Nov 13, 2025

fix(pb): dont immediately reallocate actors on runner stopping #3413

Merged

fix(guard): dont send open msg for hibernating ws, hibernating ws keepalive #3449

Merged

NathanFlurry mentioned this pull request Nov 13, 2025

chore(gas): rename wake sub to bump sub #3408

Merged

jog1t changed the base branch from graphite-base/3440 to 11-10-fix_rivetkit_fix_race_condition_with_websocket_open_events November 13, 2025 19:03

jog1t mentioned this pull request Nov 13, 2025

refactor: performance improvements for inspector #3452

Draft

MasterPtato approved these changes Nov 13, 2025

View reviewed changes

NathanFlurry force-pushed the 11-10-fix_rivetkit_fix_race_condition_with_websocket_open_events branch from e8b192b to afc9c8e Compare November 13, 2025 20:53

NathanFlurry force-pushed the 11-10-fix_rivetkit_skip_sending_rivetkit_messages_to_conns_that_do_not_support_it branch from ca5b3f0 to 51a32d6 Compare November 13, 2025 20:53

NathanFlurry mentioned this pull request Nov 13, 2025

chore(rivetkit): implement new hibernating ws protocol #3464

Open

NathanFlurry changed the base branch from 11-10-fix_rivetkit_fix_race_condition_with_websocket_open_events to graphite-base/3440 November 13, 2025 21:09

fix(rivetkit): skip sending RivetKit messages to conns that do not su…

ab1c83d

…pport it

NathanFlurry changed the base branch from graphite-base/3440 to main November 13, 2025 21:30

NathanFlurry force-pushed the 11-10-fix_rivetkit_skip_sending_rivetkit_messages_to_conns_that_do_not_support_it branch from 51a32d6 to ab1c83d Compare November 13, 2025 21:32

NathanFlurry merged commit 0699e8b into main Nov 13, 2025
11 of 14 checks passed

NathanFlurry deleted the 11-10-fix_rivetkit_skip_sending_rivetkit_messages_to_conns_that_do_not_support_it branch November 13, 2025 21:33

vercel bot deployed to Preview – rivetkit-serverless November 13, 2025 21:33 View deployment

NathanFlurry mentioned this pull request Nov 13, 2025

feat(cloudflare-workers): add createInlineClient #3466

Open

NicholasKissel mentioned this pull request Nov 15, 2025

chre: v4 website changes #3472

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(rivetkit): skip sending RivetKit messages to conns that do not support it #3440

fix(rivetkit): skip sending RivetKit messages to conns that do not support it #3440

Uh oh!

NathanFlurry commented Nov 10, 2025

Uh oh!

vercel bot commented Nov 10, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Nov 10, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 10, 2025

Uh oh!

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

edited

Loading

Uh oh!

vercel bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(rivetkit): skip sending RivetKit messages to conns that do not support it #3440

fix(rivetkit): skip sending RivetKit messages to conns that do not support it #3440

Uh oh!

Conversation

NathanFlurry commented Nov 10, 2025

Uh oh!

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

Uh oh!

Uh oh!

NathanFlurry commented Nov 10, 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!

claude bot commented Nov 10, 2025

Code Review for PR #3440

Summary

Positive Changes

Issues & Concerns

1. Bug in conn/mod.ts:208-214 ⚠️

2. Typo in Symbol Access 🐛

3. Silent Failures in Event Broadcasting 🔕

4. Missing Test Coverage 📝

Minor Suggestions

Security Considerations

Performance Considerations

Overall Assessment

Uh oh!

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

Uh oh!

Uh oh!

vercel bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

PR Review: Skip sending RivetKit messages to conns that do not support it

Overview

✅ Strengths

🐛 Potential Issues

1. Missing Return Statement (rivetkit-typescript/packages/rivetkit/src/actor/conn/mod.ts:208)

2. Method Invocation Inconsistency (rivetkit-typescript/packages/rivetkit/src/actor/conn/mod.ts:208)

3. Silent Event Skipping (rivetkit-typescript/packages/rivetkit/src/actor/instance/event-manager.ts:219)

🔍 Minor Observations

🚀 Performance Considerations

🔒 Security Concerns

🧪 Test Coverage Recommendations

📝 Recommended Changes

Summary

Uh oh!

Uh oh!

claude bot commented Nov 13, 2025

Code Review: Skip sending RivetKit messages to connections that do not support it

Overview

✅ Strengths

🐛 Potential Issues

💡 Suggestions

🔒 Security Considerations

⚡ Performance Considerations

Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vercel bot commented Nov 10, 2025 •

edited

Loading

NathanFlurry commented Nov 10, 2025 •

edited

Loading

1. Bug in `conn/mod.ts:208-214` ⚠️

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

edited

Loading