fix(rivetkit): ack ws messages #3329

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

No description provided.

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

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			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:21:05Z

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-03T22:27:39Z

Code Review for PR #3329: fix(rivetkit): ack ws messages

Summary

This PR adds WebSocket message acknowledgment to the engine actor driver. The change ensures that WebSocket messages received by actors are properly acknowledged back to the engine runner.

Code Quality: ✅ Good

Positives:

Correct pattern: The implementation follows the exact same pattern used in the test-runner (engine/sdks/typescript/test-runner/src/index.ts:230-232)
Type safety: Properly uses the RivetMessageEvent type which includes the experimental rivetRequestId and rivetMessageIndex fields
Defensive programming: Uses invariant to ensure required fields are present before calling the ack method
Clean integration: Leverages the existing Runner.sendWebsocketMessageAck() → Tunnel.__ackWebsocketMessage() flow

Code location context:

The change is in actor-driver.ts:548-557, within the WebSocket message event handler
The ack is sent after the message handler promise is initiated, which is correct

Potential Issues: ⚠️ Minor Concerns

Async handler timing: The acknowledgment is sent immediately, but the actual message handler runs asynchronously via wsHandlerPromise.then(...). If the handler throws an error or takes a long time, the ack will still be sent immediately. This might be intentional for flow control, but worth confirming:

websocket.addEventListener("message", (event: RivetMessageEvent) => {
    wsHandlerPromise.then((x) => x.onMessage?.(event, wsContext)); // async, may fail
    
    // Ack sent regardless of handler success
    invariant(event.rivetRequestId, "missing rivetRequestId");
    invariant(event.rivetMessageIndex, "missing rivetMessageIndex");
    this.#runner.sendWebsocketMessageAck(
        event.rivetRequestId,
        event.rivetMessageIndex,
    );
});

Question: Should the ack be sent only after the handler succeeds, or is this immediate ack for flow control purposes?

Error handling: If sendWebsocketMessageAck throws (e.g., tunnel is shutdown), it could crash the message handler. Consider wrapping in a try-catch:

try {
    this.#runner.sendWebsocketMessageAck(
        event.rivetRequestId,
        event.rivetMessageIndex,
    );
} catch (err) {
    logger().error({ msg: "failed to ack websocket message", err });
}

Missing import: The RivetMessageEvent type is used but I don't see it imported in the visible portion of the file. Verify the import is present at the top:
```
import type { RivetMessageEvent } from "@/common/websocket-interface";
```

Performance Considerations: ✅ Good

The ack mechanism is lightweight and uses the existing tunnel infrastructure
No performance concerns identified

Security Concerns: ✅ None

The invariant checks prevent processing messages with missing metadata
No security issues identified

Test Coverage: ⚠️ Needs Verification

Observations:

This functionality is tested in engine/sdks/typescript/test-runner/src/index.ts
However, I don't see explicit driver tests for the engine actor driver's WebSocket message acknowledgment flow

Recommendation: Consider adding a driver test that verifies:

WebSocket messages are properly acknowledged
The rivetRequestId and rivetMessageIndex are correctly passed through
Edge cases like missing metadata fields are handled

Additional Observations

Good context from the codebase:

The rivetRequestId and rivetMessageIndex fields are marked as @experimental in websocket-interface.ts
These are specifically for hibernatable WebSockets (see actor-driver.ts:152-271)
The acknowledgment flows through: Runner.sendWebsocketMessageAck() → Tunnel.__ackWebsocketMessage() → protocol message to server

Why this fix is important:
Without these acknowledgments, the engine runner's hibernation feature may not track message indices correctly, potentially causing issues when actors hibernate and resume.

Recommendations

✅ Approve with minor suggestions: The core fix is correct and necessary
⚠️ Consider: Adding error handling around the ack call
⚠️ Consider: Verifying the timing (immediate ack vs. post-handler ack) is intentional
⚠️ Verify: The RivetMessageEvent import is present
📝 Optional: Add a driver test for WebSocket message acknowledgment

Verdict: LGTM with minor suggestions 👍

The fix correctly implements WebSocket message acknowledgment following established patterns in the codebase. The minor concerns are mostly about robustness and testing, not correctness.

claude · 2025-11-04T00:39:42Z

Pull Request Review

Summary

This PR adds WebSocket message acknowledgment (ACK) functionality to the engine actor driver. The change ensures that each incoming WebSocket message is acknowledged back to the runner with its request ID and message index.

Code Quality ✅

Positive aspects:

Proper type safety by explicitly typing the event as RivetMessageEvent
Appropriate use of invariant to validate required fields before using them
Clean integration with existing WebSocket event handler structure
Follows the existing code patterns in the file

Suggestions for improvement:

Error Handling: The invariant calls will throw errors if the properties are missing. Consider whether this is the desired behavior or if you should handle this more gracefully:

if (!event.rivetRequestId || event.rivetMessageIndex === undefined) {
    logger().warn({ 
        msg: "missing hibernation metadata on websocket message",
        hasRequestId: !!event.rivetRequestId,
        hasMessageIndex: event.rivetMessageIndex !== undefined
    });
    return; // or handle appropriately
}

Async Handler Coordination: The ACK is sent immediately without waiting for wsHandlerPromise to resolve. This means:

Messages are acknowledged before the handler processes them
If the handler throws an error, the message is already acknowledged

Consider if this is intentional. If the ACK should only happen after successful processing:

websocket.addEventListener("message", (event: RivetMessageEvent) => {
    wsHandlerPromise.then((x) => {
        x.onMessage?.(event, wsContext);
        
        // ACK after handler completes
        invariant(event.rivetRequestId, "missing rivetRequestId");
        invariant(event.rivetMessageIndex, "missing rivetMessageIndex");
        this.#runner.sendWebsocketMessageAck(
            event.rivetRequestId,
            event.rivetMessageIndex,
        );
    });
});

Logging: Consider adding debug logging for the ACK operation to aid in troubleshooting:

logger().debug({ 
    msg: "acknowledging websocket message",
    requestId: bufferToString(event.rivetRequestId),
    messageIndex: event.rivetMessageIndex
});

Potential Issues ⚠️

Interface Consistency: The RivetMessageEvent.rivetRequestId is typed as ArrayBuffer | undefined but sendWebsocketMessageAck likely expects ArrayBuffer. The invariant check ensures it's not undefined, but TypeScript may still complain.
Hibernation Context: The PR title mentions "ack ws messages" but the properties being used (rivetRequestId, rivetMessageIndex) are documented as experimental and for hibernatable websockets. Should this ACK only apply to hibernatable websockets? The code applies it to all messages.
Missing Test Coverage: No tests were added. Consider adding:
- Unit test verifying ACK is sent for valid messages
- Test verifying behavior when properties are missing
- Integration test for the full hibernation flow

Performance Considerations ✅

The change adds minimal overhead:

One additional method call per message
Two property accesses and invariant checks

No performance concerns identified.

Security Concerns ✅

No security issues identified. The change:

Uses existing validated data from the engine runner
Doesn't expose sensitive information
Follows the same security patterns as the rest of the codebase

Architecture & Design 🤔

Questions:

Is this ACK mechanism required for all WebSocket messages or only hibernatable ones?
What happens on the runner side if an ACK is never received?
Should there be a timeout or retry mechanism?
Is there a corresponding change in the engine-runner package that this depends on?

Test Coverage ⚠️

Missing tests:

No tests added in this PR
Should verify ACK behavior in different scenarios
Consider adding to the driver test suite

Recommendations

Immediate: Clarify whether ACK should happen before or after message processing
Short-term: Add logging and improve error handling
Medium-term: Add comprehensive test coverage
Long-term: Document the hibernation/ACK protocol more thoroughly

Overall Assessment

The code change is clean and follows existing patterns, but would benefit from:

Better error handling (vs. throwing on missing properties)
Test coverage
Clarification on ACK timing relative to message processing
Documentation about when/why ACKs are needed

Status: Approve with recommendations for follow-up improvements.

claude · 2025-11-04T00:39:46Z

PR Review: fix(rivetkit): ack ws messages

Summary

This PR adds acknowledgment logic for WebSocket messages in the engine actor driver. The change sends an acknowledgment to the engine runner for each WebSocket message received, which is critical for hibernatable WebSocket support.

Code Quality & Best Practices

✅ Positive aspects:

Type safety improvement: Added explicit RivetMessageEvent type annotation to the message event handler, which provides better type checking for the Rivet-specific event properties
Follows existing patterns: The acknowledgment pattern matches what's documented in the experimental WebSocket hibernation feature (see rivetkit-typescript/packages/rivetkit/src/common/websocket-interface.ts:8-19)
Consistent error handling: Uses invariant for validation, which is consistent with the rest of the codebase (see actor-driver.ts:396, 590)

⚠️ Potential concerns:

1. Error Handling & Robustness

Issue: The invariant calls will throw and crash the message handler if rivetRequestId or rivetMessageIndex are missing.

invariant(event.rivetRequestId, "missing rivetRequestId");
invariant(event.rivetMessageIndex, "missing rivetMessageIndex");

Context: According to the interface definition in websocket-interface.ts:10,19, both rivetRequestId and rivetMessageIndex are marked as optional (?) and experimental. This suggests they may not always be present.

Recommendation: Consider handling the case where these properties are missing more gracefully:

// Option 1: Only ack if the fields are present (for backward compatibility)
if (event.rivetRequestId && event.rivetMessageIndex !== undefined) {
    this.#runner.sendWebsocketMessageAck(
        event.rivetRequestId,
        event.rivetMessageIndex,
    );
}

// Option 2: Log a warning instead of crashing
if (!event.rivetRequestId || event.rivetMessageIndex === undefined) {
    logger().warn("websocket message missing hibernation metadata, skipping ack");
    return;
}

2. Timing & Race Conditions

Issue: The acknowledgment is sent after the user's onMessage handler is invoked (via wsHandlerPromise), but the ack call is not awaited.

wsHandlerPromise.then((x) => x.onMessage?.(event, wsContext));  // Async, may still be running
this.#runner.sendWebsocketMessageAck(...);  // Called immediately after

Questions to consider:

Should the ack be sent before or after the message is processed by the user handler?
If processing fails, should we still ack the message?
Could there be a race condition if messages arrive rapidly?

Current behavior: The code acks messages immediately, regardless of whether the user's handler completes successfully. This is probably correct for a message delivery guarantee system (ack receipt, not processing), but it should be documented.

3. Test Coverage

⚠️ Missing tests: I don't see any tests added for this functionality.

Recommendation: Add tests to verify:

Messages are properly acknowledged when rivetRequestId and rivetMessageIndex are present
The system handles messages without hibernation metadata gracefully
Acknowledgments are sent for each message in rapid succession
The ack system works correctly during actor hibernation/wake cycles

Test files that should be updated:

rivetkit-typescript/packages/rivetkit/tests/driver-engine.test.ts
Or create new tests in rivetkit-typescript/packages/rivetkit/src/driver-test-suite/tests/

4. Documentation

Missing context:

No comments explaining why acknowledgments are needed
The feature is marked as @experimental but there's no indication of this in the implementation

Recommendation: Add a comment explaining the purpose:

// Acknowledge receipt of hibernatable websocket messages to the engine runner.
// This is required for the experimental hibernation feature to track message delivery.
// See: https://docs.rs/... or internal docs
if (event.rivetRequestId && event.rivetMessageIndex !== undefined) {
    this.#runner.sendWebsocketMessageAck(
        event.rivetRequestId,
        event.rivetMessageIndex,
    );
}

Performance Considerations

✅ Good: The sendWebsocketMessageAck call appears to be lightweight based on the runner implementation (engine/sdks/typescript/runner/src/mod.ts:1383-1385). It simply delegates to the tunnel's internal ack method, so performance impact should be minimal.

Security Concerns

✅ No obvious security issues - The acknowledgment system appears to be internal to the engine runner communication and doesn't expose user data or create attack vectors.

Recommendations Summary

High Priority:

Handle missing metadata gracefully instead of using invariant - these fields are optional/experimental
Add test coverage for the acknowledgment functionality

Medium Priority:
3. Add documentation/comments explaining the purpose of acks
4. Consider whether acks should wait for message processing or acknowledge immediately (current behavior should be documented)

Low Priority:
5. Verify the timing of acks relative to message processing is correct for the hibernation use case

Would you like me to help implement any of these recommendations?

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

PR Review: fix(rivetkit): ack ws messages

Summary

This PR adds acknowledgment (ack) functionality for WebSocket messages in the RivetKit engine actor driver. The change ensures that each incoming WebSocket message is acknowledged back to the engine runner, which is critical for the hibernatable WebSocket feature.

Code Quality ✅

Positives:

Type Safety: The change properly types the event parameter as RivetMessageEvent, which is good practice and makes the Rivet-specific properties (rivetRequestId, rivetMessageIndex) explicit.
Defensive Programming: Uses invariant to validate that required fields exist before attempting to use them, which will fail fast with clear error messages if the contract is violated.
Consistent Pattern: This implementation mirrors the pattern already used in engine/sdks/typescript/test-runner/src/index.ts:232, showing consistency across the codebase.
Proper Placement: The ack is sent after the message handler promise is queued, ensuring messages are acknowledged even if the handler throws an error.

Potential Issues 🔍

Minor Concerns:

Error Handling: If wsHandlerPromise rejects, the ack will still be sent. This might be intentional (ack the receipt, not the processing), but it's worth considering whether a failed handler should prevent the ack.
- Current flow:
```
wsHandlerPromise.then((x) => x.onMessage?.(event, wsContext)); // May fail
// Ack always sent regardless
this.#runner.sendWebsocketMessageAck(event.rivetRequestId, event.rivetMessageIndex);
```
- Recommendation: Document whether the ack represents "message received" vs "message processed successfully". Based on the hibernation use case, "message received" seems correct.
Invariant Timing: The invariants will throw synchronously if the properties are missing, but this happens after wsHandlerPromise.then() is called. If these properties are truly required, you might want to validate them before queuing the handler.
- Current: Handler queued → invariants check → ack sent
- Alternative: Invariants check → handler queued → ack sent
- Assessment: Current order is probably fine since the promise won't block, but worth considering.
Type Import Missing: The RivetMessageEvent type is used but not imported at the top of the file. I can see it's defined in @/common/websocket-interface, but the import statement isn't visible in the diff.
- Action needed: Verify that the import is present. If not, add: import type { RivetMessageEvent } from "@/common/websocket-interface";

Performance Considerations ✅

No performance concerns. The ack operation is lightweight and properly placed after the async handler is queued, so it won't block message processing.

Security Considerations ✅

No security concerns identified. The invariants provide good protection against malformed events.

Test Coverage ⚠️

Concern: This PR doesn't include tests for the new acknowledgment functionality.

Recommendations:

Add a test to verify that sendWebsocketMessageAck is called with correct parameters when a message is received
Test the invariant failures when rivetRequestId or rivetMessageIndex are missing
Verify behavior when wsHandlerPromise rejects - does the ack still get sent?

Example test outline:

it('should acknowledge websocket messages', async () => {
  const mockEvent = {
    data: 'test',
    rivetRequestId: new ArrayBuffer(16),
    rivetMessageIndex: 0,
  };
  
  // Mock runner.sendWebsocketMessageAck
  const ackSpy = jest.spyOn(runner, 'sendWebsocketMessageAck');
  
  // Trigger message event
  websocket.dispatchEvent(mockEvent);
  
  // Verify ack was called
  expect(ackSpy).toHaveBeenCalledWith(
    mockEvent.rivetRequestId,
    mockEvent.rivetMessageIndex
  );
});

Questions for Author

Is the missing RivetMessageEvent import intentional, or should it be added?
Should the ack be sent if wsHandlerPromise rejects? (I believe current behavior is correct, but worth confirming)
Are there existing tests that cover this code path that should be updated?

Overall Assessment

Verdict: Approve with minor suggestions ✅

This is a straightforward, well-implemented fix that follows existing patterns in the codebase. The main concerns are:

Verify the type import is present
Add test coverage for the new functionality

The change is essential for proper hibernatable WebSocket support and the implementation looks solid.

Generated with Claude Code

claude · 2025-11-04T19:32:51Z

PR Review: Fix WebSocket Message Acknowledgment

Summary

This PR adds WebSocket message acknowledgment logic to the EngineActorDriver by calling sendWebsocketMessageAck for each incoming WebSocket message. This is critical for hibernatable WebSocket support, ensuring the engine runner properly tracks message delivery.

✅ Positive Observations

Correct Pattern Implementation: The change follows the exact pattern already established in test-runner/src/index.ts:232, which is the reference implementation for this functionality.
Type Safety: Adding the explicit RivetMessageEvent type annotation improves type safety and makes the hibernation-related properties (rivetRequestId, rivetMessageIndex) discoverable.
Proper Error Handling: Using invariant to validate required properties (rivetRequestId, rivetMessageIndex) is appropriate and will catch configuration issues early.
Essential Bug Fix: Without this acknowledgment, hibernatable WebSockets won't function correctly. The engine runner needs these acks to track message delivery state (see actor/instance.ts:1605-1607 where msgIndex is updated based on acknowledgment).

🔍 Potential Issues & Concerns

1. Missing Error Handling Around sendWebsocketMessageAck

The call to sendWebsocketMessageAck could potentially throw or fail. Consider wrapping it in a try-catch:

try {
  invariant(event.rivetRequestId, "missing rivetRequestId");
  invariant(event.rivetMessageIndex, "missing rivetMessageIndex");
  this.#runner.sendWebsocketMessageAck(
    event.rivetRequestId,
    event.rivetMessageIndex,
  );
} catch (error) {
  logger().error({ msg: "failed to ack websocket message", error });
  // Consider whether to close the WebSocket here
}

2. Invariant May Be Too Strict

The invariant calls will throw exceptions if these properties are missing. This could crash the message handler for non-hibernatable WebSockets (where these properties might legitimately be undefined based on websocket-interface.ts:10,19 showing these as optional).

Recommendation: Check if the WebSocket is hibernatable before asserting these properties, or make the ack call conditional:

websocket.addEventListener("message", (event: RivetMessageEvent) => {
  wsHandlerPromise.then((x) => x.onMessage?.(event, wsContext));

  // Only ack if this is a hibernatable WebSocket with tracking info
  if (event.rivetRequestId && event.rivetMessageIndex !== undefined) {
    this.#runner.sendWebsocketMessageAck(
      event.rivetRequestId,
      event.rivetMessageIndex,
    );
  }
});

3. Race Condition Concern

The wsHandlerPromise.then() is asynchronous, but the ack is sent synchronously. If user code in onMessage throws an error or takes time to process, we're still acknowledging receipt. This might be intentional (ack receipt vs. processing), but worth documenting.

4. Ordering Guarantee

The acknowledgment happens after calling the user's message handler. If the user handler is slow or async, multiple messages could queue up. Verify this is the intended behavior - typically acks should happen immediately on receipt, not after processing.

🧪 Test Coverage

Critical Gap: There are no tests validating this acknowledgment logic in the driver test suite.

Recommendations:

Add a test in driver-test-suite/tests/raw-websocket.ts that verifies sendWebsocketMessageAck is called with correct parameters
Test the hibernatable WebSocket flow specifically with message acknowledgments
Test behavior when rivetRequestId/rivetMessageIndex are missing

🔒 Security Considerations

No major security concerns. The invariant checks prevent undefined values from being passed to the engine, which is good defensive programming.

⚡ Performance Considerations

Minimal performance impact. The ack is a simple method call. However, if sendWebsocketMessageAck does any I/O or heavy processing, this could become a bottleneck under high message volume.

📝 Documentation

Consider adding a comment explaining why this acknowledgment is necessary:

// Acknowledge message receipt for hibernatable WebSockets.
// The engine runner uses these acks to track message delivery state
// and properly resume hibernated connections.
invariant(event.rivetRequestId, "missing rivetRequestId");
invariant(event.rivetMessageIndex, "missing rivetMessageIndex");
this.#runner.sendWebsocketMessageAck(
  event.rivetRequestId,
  event.rivetMessageIndex,
);

🎯 Final Verdict

This is an important bug fix, but the invariant checks may be too strict. The safest approach is to make the ack conditional on the presence of these properties rather than throwing if they're missing.

Suggested Change:

websocket.addEventListener("message", (event: RivetMessageEvent) => {
  wsHandlerPromise.then((x) => x.onMessage?.(event, wsContext));

  // Acknowledge message for hibernatable WebSockets
  if (event.rivetRequestId && event.rivetMessageIndex !== undefined) {
    this.#runner.sendWebsocketMessageAck(
      event.rivetRequestId,
      event.rivetMessageIndex,
    );
  }
});

This maintains backward compatibility while adding the necessary hibernation support.

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

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:52 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 4, 8:53 PM UTC: CI is running for this pull request on a draft pull request (#3353) due to your merge queue CI optimization settings.
Nov 4, 8:56 PM UTC: Merged by the Graphite merge queue via draft PR: #3353.

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

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:38:57Z

PR Review: fix(rivetkit): ack ws messages

Summary

This PR adds WebSocket message acknowledgment functionality to the engine actor driver. The change ensures that each WebSocket message received is acknowledged back to the engine runner using sendWebsocketMessageAck, which is critical for hibernatable WebSocket support.

Code Quality ✅

Positives:

Clean, focused change addressing a specific functionality gap
Proper TypeScript typing with RivetMessageEvent
Uses invariant for runtime validation

Concerns:

Breaking Change Risk - Critical ⚠️

The invariant calls will throw and crash the WebSocket connection if these properties are missing:

invariant(event.rivetRequestId, "missing rivetRequestId");
invariant(event.rivetMessageIndex, "missing rivetMessageIndex");

However, the type definitions mark these as optional and experimental:

// From websocket-interface.ts
/**
 * @experimental
 * Request ID for hibernatable websockets (provided by engine runner)
 **/
rivetRequestId?: ArrayBuffer;
/**
 * @experimental  
 * Message index for hibernatable websockets (provided by engine runner)
 **/
rivetMessageIndex?: number;

Impact: This will break all non-hibernatable WebSocket connections that don't provide these experimental fields.

Recommendation: Use conditional acknowledgment:

websocket.addEventListener("message", (event: RivetMessageEvent) => {
    wsHandlerPromise.then((x) => x.onMessage?.(event, wsContext));
    
    // Only send ack if hibernation properties are present
    if (event.rivetRequestId && event.rivetMessageIndex !== undefined) {
        this.#runner.sendWebsocketMessageAck(
            event.rivetRequestId,
            event.rivetMessageIndex,
        );
    }
});

Error Handling ⚠️

No error handling around sendWebsocketMessageAck. If this call fails, it could:

Crash the message handler
Prevent user's onMessage handler from being called
Leave messages unacknowledged

Recommendation: Add try-catch:

if (event.rivetRequestId && event.rivetMessageIndex !== undefined) {
    try {
        this.#runner.sendWebsocketMessageAck(
            event.rivetRequestId,
            event.rivetMessageIndex,
        );
    } catch (err) {
        logger().error({ msg: "failed to ack websocket message", err });
    }
}

Async Handler Race Condition ⚠️

The user's message handler is called asynchronously via wsHandlerPromise.then(), but the acknowledgment is sent synchronously. This means:
- Messages are acknowledged before user code processes them
- If user code throws an error, the message was already acknowledged
This might be intentional for hibernatable WebSockets (acknowledge receipt, not processing), but it's worth documenting or reconsidering based on your acknowledgment semantics.

Test Coverage ❌

Missing:

No tests added for the new acknowledgment functionality
No tests for hibernatable WebSocket behavior
No tests for missing rivetRequestId/rivetMessageIndex scenarios

Recommendation: Add tests in rivetkit-typescript/packages/rivetkit/tests/ or the driver-test-suite covering:

Hibernatable WebSocket message acknowledgments
Non-hibernatable WebSocket connections (should still work)
Error scenarios

Performance Considerations ✅

The change adds minimal overhead (one function call per message), which should be negligible.

Security Concerns ✅

No security issues identified. The acknowledgment data comes from the engine runner (trusted source).

Architecture Fit ⚠️

The pattern exists elsewhere in the codebase (src/actor/instance.ts:1606) where event.rivetMessageIndex is used conditionally:

if (persistedHibernatableWebSocket) {
    persistedHibernatableWebSocket.msgIndex = BigInt(
        event.rivetMessageIndex,
    );
}

This confirms these properties are conditional based on whether hibernation is enabled.

Recommendations Summary

Must Fix: Make acknowledgment conditional to avoid breaking non-hibernatable WebSockets
Should Fix: Add error handling around sendWebsocketMessageAck
Consider: Document or verify that acknowledgment before processing is intentional
Nice to Have: Add test coverage

Overall Assessment

The change implements necessary functionality but needs modifications to handle optional/experimental properties correctly. As written, this is a breaking change that will crash non-hibernatable WebSocket connections.

Status: ⚠️ Needs Changes

This was referenced Nov 3, 2025

chore(runner): decrease timeout to 15s #3328

Closed

chore(rivetkit): correctly stringify engine runner request ids for logs #3330

Closed

chore(rivetkit): fmt #3331

Closed

vercel bot had a problem deploying to Preview – rivetkit-serverless November 3, 2025 22:21 Failure

NathanFlurry force-pushed the 11-03-chore_runner_decrease_timeout_to_15s branch from a0d58a1 to 9fbbebe Compare November 4, 2025 00:36

NathanFlurry force-pushed the 11-03-fix_rivetkit_ack_ws_messages branch from f1efe27 to 7b1fd28 Compare November 4, 2025 00:36

NathanFlurry requested a review from MasterPtato November 4, 2025 00:37

vercel bot had a problem deploying to Preview – rivetkit-serverless November 4, 2025 00:37 Failure

NathanFlurry mentioned this pull request Nov 4, 2025

fix(rivetkit): fix not pino bindings #3334

Closed

This was referenced Nov 4, 2025

fix(rivetkit): fix conns not being cleaned up if actor.createConn errors #3335

Closed

chore(engine): move smoke test to engine/tests/ #3337

Closed

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-03-chore_runner_decrease_timeout_to_15s branch from 9fbbebe to d9e0eb8 Compare November 4, 2025 18:59

NathanFlurry force-pushed the 11-03-fix_rivetkit_ack_ws_messages branch from 7b1fd28 to 5f837ae Compare November 4, 2025 18:59

vercel bot had a problem deploying to Preview – rivetkit-serverless November 4, 2025 19:00 Failure

NathanFlurry force-pushed the 11-03-chore_runner_decrease_timeout_to_15s branch from d9e0eb8 to d2735a8 Compare November 4, 2025 19:24

NathanFlurry force-pushed the 11-03-fix_rivetkit_ack_ws_messages branch from 5f837ae to 9d011a4 Compare November 4, 2025 19:24

vercel bot had a problem deploying to Preview – rivetkit-serverless November 4, 2025 19:25 Failure

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): ack ws messages (#3329)

ea4f699

fix(rivetkit): ack ws messages

0298a50

NathanFlurry force-pushed the 11-03-chore_runner_decrease_timeout_to_15s branch from d2735a8 to a114914 Compare November 4, 2025 20:25

NathanFlurry force-pushed the 11-03-fix_rivetkit_ack_ws_messages branch from 9d011a4 to 0298a50 Compare November 4, 2025 20:25

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

fix(rivetkit): ack ws messages (#3329)

b4d46a7

graphite-app bot closed this Nov 4, 2025

graphite-app bot deleted the 11-03-fix_rivetkit_ack_ws_messages branch November 4, 2025 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(rivetkit): ack ws messages #3329

fix(rivetkit): ack ws messages #3329

Uh oh!

NathanFlurry commented Nov 3, 2025

Uh oh!

vercel bot commented Nov 3, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Nov 3, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 3, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

graphite-app bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

vercel bot commented Nov 4, 2025

Uh oh!

claude bot commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(rivetkit): ack ws messages #3329

fix(rivetkit): ack ws messages #3329

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!

claude bot commented Nov 3, 2025

Code Review for PR #3329: fix(rivetkit): ack ws messages

Summary

Code Quality: ✅ Good

Potential Issues: ⚠️ Minor Concerns

Performance Considerations: ✅ Good

Security Concerns: ✅ None

Test Coverage: ⚠️ Needs Verification

Additional Observations

Recommendations

Verdict: LGTM with minor suggestions 👍

Uh oh!

claude bot commented Nov 4, 2025

Pull Request Review

Summary

Code Quality ✅

Potential Issues ⚠️

Performance Considerations ✅

Security Concerns ✅

Architecture & Design 🤔

Test Coverage ⚠️

Recommendations

Overall Assessment

Uh oh!

claude bot commented Nov 4, 2025

PR Review: fix(rivetkit): ack ws messages

Summary

Code Quality & Best Practices

1. Error Handling & Robustness

2. Timing & Race Conditions

3. Test Coverage

4. Documentation

Performance Considerations

Security Concerns

Recommendations Summary

Uh oh!

claude bot commented Nov 4, 2025

PR Review: fix(rivetkit): ack ws messages

Summary

Code Quality ✅

Potential Issues 🔍

Performance Considerations ✅

Security Considerations ✅

Test Coverage ⚠️

Questions for Author

Overall Assessment

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Fix WebSocket Message Acknowledgment

Summary

✅ Positive Observations

🔍 Potential Issues & Concerns

1. Missing Error Handling Around sendWebsocketMessageAck

2. Invariant May Be Too Strict

3. Race Condition Concern

4. Ordering Guarantee

🧪 Test Coverage

🔒 Security Considerations

⚡ Performance Considerations

📝 Documentation

🎯 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.

Uh oh!

Merge activity

Uh oh!

vercel bot commented Nov 4, 2025

vercel bot commented Nov 3, 2025 •

edited

Loading

NathanFlurry commented Nov 3, 2025 •

edited

Loading

graphite-app bot commented Nov 4, 2025 •

edited

Loading