chore(rivetkit): debounce ws message ack #3345

NathanFlurry · 2025-11-04T19:55:49Z

No description provided.

vercel · 2025-11-04T19:55: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 5, 2025 2:14am

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 5, 2025 2:14am
rivet-inspector	Ignored	Preview	Nov 5, 2025 2:14am
rivet-site	Ignored	Preview	Nov 5, 2025 2:14am

NathanFlurry · 2025-11-04T19:56:41Z

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-04T19:57:19Z

Pull Request Review: Debounce WebSocket Message Acknowledgments

Summary

This PR implements debouncing for WebSocket message acknowledgments by batching acks and flushing them on a 1-second interval. This is a good optimization to reduce the number of ack messages sent to the gateway.

Code Quality & Best Practices

Positive:

Clear separation of concerns with dedicated methods (#flushWsAcks)
Good use of comments explaining the gateway timeout relationship
Proper cleanup in shutdown path
TypeScript typing appears correct

Issues:

Critical Bug - Request ID Serialization (actor-driver.ts:323-325, 499, 587)

There's a mismatch in how requestId is handled:
- Line 499: requestId is converted to string via idToStr(requestIdBuf)
- Line 587: Using this string requestId as the Map key
- Line 323-325: Attempting to convert back by splitting on comma
Problem: The idToStr() function likely returns a hex or base64 string, not comma-separated numbers. This means the conversion back to ArrayBuffer on line 323-325 will fail or produce incorrect data.

Solution: Either:
- Store the original ArrayBuffer as the key (convert to string only for display/logging)
- Use a proper serialization format (e.g., base64) and matching deserialization
```
// Option 1: Store ArrayBuffer directly (best performance)
// Change Map type to: Map<ArrayBuffer, bigint>
// Use a custom comparison or convert to base64 string consistently

// Option 2: Fix the conversion
const requestIdStr = btoa(String.fromCharCode(...new Uint8Array(requestIdBuf)));
// Later:
const requestId = Uint8Array.from(atob(requestIdStr), c => c.charCodeAt(0)).buffer;
```
Race Condition Risk (actor-driver.ts:603-606)

When a WebSocket closes, #flushWsAcks() is called, but there's a potential race:
- If a message event is still being processed when close fires
- The close handler might flush before the message handler adds to queue
This is likely rare but could cause the last message(s) to not be acknowledged.

Suggestion: Consider using a promise/await pattern or ensure the message handler completes before close processing.
Type Safety Issue (actor-driver.ts:592)

event.rivetMessageIndex is directly converted to BigInt, but we should validate it's actually a bigint or number:
```
const messageIndex = typeof event.rivetMessageIndex === 'bigint' 
  ? event.rivetMessageIndex 
  : BigInt(event.rivetMessageIndex);
```

Performance Considerations

Good:

Batching reduces network overhead significantly
1-second interval is reasonable given the 30s gateway timeout
Early return when queue is empty (line 319)

Considerations:

Map Iteration Performance (actor-driver.ts:321-327)

The conversion from string back to ArrayBuffer happens on every flush for every request. If there are many concurrent WebSocket connections, this could add up.

Recommendation: Store ArrayBuffer directly or cache the conversion.
Memory Usage

The queue unboundedly grows until flush. With high message rates, this could accumulate many entries in 1 second.

Suggestion: Consider adding a max queue size with immediate flush if exceeded:
```
if (this.#wsAckQueue.size >= MAX_QUEUE_SIZE) {
  this.#flushWsAcks();
}
```

Security Concerns

Low Risk:

No obvious security vulnerabilities
Proper cleanup prevents resource leaks

Consideration:

If an attacker sends many messages rapidly, they could fill the ack queue during the 1-second window. However, this is mitigated by the fact that messages are only tracked per request ID (deduplication of highest index).

Test Coverage

Missing:

No test changes included in this PR
Should add tests for:
- Ack batching and flushing
- Out-of-order message handling
- Shutdown cleanup
- Race condition scenarios

Recommendation: Add unit tests covering the new debouncing logic, especially edge cases like:

Messages arriving in wrong order
WebSocket closing mid-batch
Multiple flushes with same request ID

Additional Suggestions

Configuration - Consider making the flush interval configurable via constructor or config:
```
const flushInterval = this.#runConfig.wsAckFlushInterval ?? 1000;
```
Metrics - Add metrics/logging for monitoring:
- Average batch size per flush
- Number of out-of-order messages received
- Queue size at flush time
Documentation - Add JSDoc comments explaining:
- Why debouncing is needed
- The relationship to gateway timeout
- What happens with out-of-order messages

Verdict

Approach: ✅ Good - Debouncing is the right solution for this problem

Implementation: ⚠️ Needs fixes - The request ID serialization bug must be fixed before merge

Priority:

🔴 MUST FIX: Request ID serialization (line 323-325)
🟡 SHOULD FIX: Type safety for message index
🟢 NICE TO HAVE: Add tests, metrics, and configuration

Great work on identifying and addressing the performance issue with WebSocket acks! The debouncing approach is solid, just needs the serialization bug fixed.

claude · 2025-11-04T20:41:58Z

PR Review: Debounce WebSocket Message Acknowledgments

Summary

This PR implements debouncing for WebSocket message acknowledgments to reduce network overhead and improve performance by batching acks instead of sending them immediately for each message.

Code Quality & Best Practices

Positive:

Clean implementation with clear comments explaining the rationale
Good separation of concerns with dedicated #flushWsAcks() method
Proper cleanup in shutdownRunner() to flush pending acks before shutdown
Flush on WebSocket close ensures no acks are lost

Issues Found:

🔴 Critical: Type Mismatch Bug

Location: actor-driver.ts:80 and actor-driver.ts:587-589

The #wsAckQueue is declared as Map<string, number> but rivetMessageIndex is actually a bigint (as evidenced by line 587 using -1n literal and the comparison with event.rivetMessageIndex).

// Current (WRONG):
#wsAckQueue: Map<string, number> = new Map();

// Should be:
#wsAckQueue: Map<string, bigint> = new Map();

Impact: This could cause runtime type coercion issues and incorrect comparisons. TypeScript should catch this, but the comparison on line 588 (event.rivetMessageIndex > currentMax) may produce unexpected results.

🟡 Medium: Inefficient ArrayBuffer Serialization

Location: actor-driver.ts:321-326

Converting ArrayBuffer to a comma-separated string and back is inefficient:

const requestIdStr = requestIdStr.split(",").map((x) => Number.parseInt(x))

Suggestion: Use a more efficient key generation:

// Use the existing idToStr utility that's already imported
const requestIdStr = idToStr(requestIdBuf);

This would be more consistent with line 499 where idToStr is already used for the same purpose.

Performance Considerations

Good:

1-second flush interval is reasonable (balances latency vs batching)
Early return in #flushWsAcks() when queue is empty avoids unnecessary work
Tracking only the highest message index is the correct approach

Concern:

The current implementation converts the requestId to string on every message (line 587). Consider caching the string representation:

websocket.addEventListener("message", (event: RivetMessageEvent) => {
    // Cache requestId string conversion
    const requestIdStr = requestId; // Already converted on line 499
    // ...

Security Concerns

Medium Priority:

Unbounded Map Growth: If a malicious actor opens many WebSocket connections without closing them, #wsAckQueue could grow unbounded. Consider adding:
- Maximum queue size limit
- Logging/metrics when queue size exceeds threshold
- Periodic cleanup of stale entries

Potential Bugs

Race Condition on Shutdown:
The interval is cleared before flushing, but there's a potential race where setInterval callback could be running while shutdownRunner is called. Consider using a lock or checking if the runner is shutting down in #flushWsAcks().

Missing Variable Declaration:
Line 587 uses requestId but it's defined as requestIdBuf on line 496. The code should use:

const requestIdStr = idToStr(event.rivetRequestId);
const currentMax = this.#wsAckQueue.get(requestIdStr) ?? -1n;

Test Coverage

Missing:

No tests for the debouncing logic
No tests for edge cases (shutdown during flush, multiple rapid messages)
No tests verifying acks are properly batched

Recommended Tests:

Verify messages are batched (send multiple messages rapidly, verify single ack batch)
Verify flush on interval (wait >1s, verify ack sent)
Verify flush on close (close WebSocket, verify pending acks sent)
Verify flush on shutdown (shutdown runner, verify pending acks sent)
Test out-of-order message handling (verify warning is logged)

Action Items

Must Fix Before Merge:

Fix type mismatch: Change Map<string, number> to Map<string, bigint>
Fix variable reference: Use idToStr(event.rivetRequestId) to generate the map key

Should Fix:

Add bounds checking/logging for #wsAckQueue size
Add test coverage for debouncing behavior
Consider using idToStr utility instead of custom string conversion

Nice to Have:

Add metrics for ack queue size
Add configurable flush interval

Overall Assessment

The core debouncing approach is sound and should improve performance. However, the type mismatch bug is critical and must be fixed before merging. With the suggested fixes, this will be a solid performance improvement.

Recommendation: Request changes for critical bug fix, then approve.

claude · 2025-11-04T21:01:26Z

Pull Request Review: Debounce WebSocket Message Acknowledgments

Summary

This PR implements debouncing for WebSocket message acknowledgments to reduce the number of ack messages sent to the gateway. Instead of sending an ack for every message immediately, acks are batched and flushed every 1 second.

Code Quality & Best Practices

Positive Observations:

Clean implementation with clear separation of concerns
Good use of TypeScript Map for tracking request IDs
Proper cleanup in shutdown path
Helpful inline comments explaining the rationale

Concerns:

Type Mismatch - Critical Bug ⚠️
- Line 80: #wsAckQueue: Map<string, number> declares the value type as number
- Line 587-589: The code stores event.rivetMessageIndex which is likely a bigint based on:
  - Line 275 in the same file: msgIndex: -1n (bigint literal)
  - Line 587: Comparison event.rivetMessageIndex > currentMax where currentMax defaults to -1n (bigint)
- Line 326: this.#runner.sendWebsocketMessageAck(requestId, messageIndex) is called with the stored value
Recommendation: Change the Map type to Map<string, bigint> and update the comparison on line 587 to use -1n instead of Number()

ArrayBuffer to String Conversion - Potential Issue

Lines 323-325: Converting ArrayBuffer to string using .split(",").map(x => Number.parseInt(x))
This conversion is fragile and inefficient

Recommendation: Consider using a more robust key generation method:

// Option 1: Use a WeakMap with ArrayBuffer directly (if reference is stable)
// Option 2: Use a base64 or hex encoding
const requestIdKey = Array.from(new Uint8Array(requestIdBuf))
  .map(b => b.toString(16).padStart(2, '0'))
  .join('');

Missing Variable Declaration
- Line 587: References requestId variable that's not declared in the message event handler
- Should be: const requestId = idToStr(event.rivetRequestId);

Performance Considerations

Good:

Debouncing reduces network overhead by batching acks
1-second flush interval is reasonable given the 30s gateway timeout
Tracks only the highest message index per request (good optimization)

Concerns:

Memory Growth Risk: If many WebSocket connections are active but close before the next flush interval, the #wsAckQueue could grow large. Consider adding a size limit or warning threshold.
Flush Interval Choice: The 1-second interval is hardcoded. Consider:
- Making it configurable
- Or adding a size-based trigger (e.g., flush when queue reaches 100 entries)

Potential Bugs

Critical: Type mismatch (see above - bigint vs number)
Critical: Undefined variable requestId on line 587
Warning Message Logic: Lines 590-597 warn about receiving a lower index, but this might be legitimate in some race conditions or message reordering scenarios. Consider if this is truly an error or just informational.

Security Considerations

No major security concerns identified. The debouncing logic doesn't introduce new attack vectors.

Test Coverage

Missing:

No tests added for the debouncing functionality
Should test:
- Multiple messages batched correctly
- Flush on interval
- Flush on WebSocket close
- Flush on shutdown
- Highest index tracking works correctly
- Edge case: empty queue flush

Recommendation: Add unit tests or integration tests, especially for the critical path of message acknowledgment.

Suggested Changes

-	#wsAckQueue: Map<string, number> = new Map();
+	#wsAckQueue: Map<string, bigint> = new Map();

 		websocket.addEventListener("message", (event: RivetMessageEvent) => {
 			wsHandlerPromise.then((x) => x.onMessage?.(event, wsContext));
 
 			invariant(event.rivetRequestId, "missing rivetRequestId");
 			invariant(event.rivetMessageIndex, "missing rivetMessageIndex");
 
 			// Track only the highest seen message index per request
-			// Convert ArrayBuffer to string for Map key
+			const requestId = idToStr(event.rivetRequestId);
 			const currentMax = this.#wsAckQueue.get(requestId) ?? -1n;

Questions

Is event.rivetMessageIndex definitely a bigint? Please confirm the type.
Have you tested this with high-throughput WebSocket connections?
Should the flush interval be configurable, or is 1s appropriate for all use cases?

Overall Assessment

The approach is sound and addresses a real performance concern. However, there appear to be critical type mismatches and a missing variable declaration that need to be fixed before merging. Once these issues are resolved and tests are added, this will be a solid improvement.

Recommendation: Request changes - fix type issues and add test coverage.

rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts

claude · 2025-11-05T00:14:09Z

Code Review: WebSocket Message Acknowledgment Debouncing

I've reviewed this PR and have the following feedback:

Positive Changes

Performance Improvement: The debouncing mechanism is well-designed to reduce network overhead by batching acknowledgments, which should improve performance under high message throughput scenarios.
Proper Cleanup: Good cleanup logic in both shutdownRunner() and the WebSocket close event handler - ensures acks are flushed before termination.
Documentation: The inline comment referencing the gateway timeout configuration is helpful context.

Issues & Concerns

1. Type Mismatch: `number` vs `bigint` ⚠️

Location: Line 80, 587-589

The #wsAckQueue is declared as Map<string, number> but event.rivetMessageIndex appears to be a bigint:

#wsAckQueue: Map<string, number> = new Map();
// ...
const currentMax = this.#wsAckQueue.get(requestId) ?? -1n;
if (event.rivetMessageIndex > currentMax) {

Issue: The comparison with -1n (bigint literal) and the storage of event.rivetMessageIndex suggests this should be Map<string, bigint>. This type mismatch could cause:

Runtime errors or unexpected behavior
TypeScript compilation issues (if strict mode is enabled)

Fix: Change line 80 to:

#wsAckQueue: Map<string, bigint> = new Map();

2. Unsafe ArrayBuffer-to-String Conversion ⚠️

Location: Lines 322-325

The conversion of ArrayBuffer to string using comma-separated values is fragile:

const requestId = new Uint8Array(
  requestIdStr.split(",").map((x) => Number.parseInt(x)),
).buffer;

Issues:

No validation of the parsed integers
Number.parseInt() can return NaN for invalid input
The string representation may not be unique for different ArrayBuffers (collision risk)
Performance overhead of string operations

Better approach: Consider using a more robust serialization method:

// Store the ArrayBuffer directly or use a better key generation
const requestIdKey = Array.from(new Uint8Array(requestIdBuf)).join(',');
// Or use a dedicated utility function for ArrayBuffer hashing

3. Missing Request ID Conversion in Message Handler 🐛

Location: Line 586-589

The message event handler uses requestId (the string from line 499) as the Map key, but this is the result of idToStr(requestIdBuf). This is inconsistent with the flush function which expects the string to be a comma-separated byte representation.

Problem: The key generation in the message handler (idToStr(requestIdBuf)) likely doesn't match the expected format in #flushWsAcks() which splits on commas and parses integers.

Fix: Ensure consistent key generation. You need to either:

Use the same conversion function in both places, OR
Store the original requestIdBuf in a WeakMap or use a consistent string representation

4. Race Condition: Interval vs Flush on Close ⚠️

Location: Lines 299, 602

There's a potential race condition where:

The interval timer fires and calls #flushWsAcks() at line 299
Simultaneously, a WebSocket closes and calls #flushWsAcks() at line 602
Both could iterate over the same queue and send duplicate acks

Fix: Consider adding a guard or using a lock mechanism, or ensure sendWebsocketMessageAck is idempotent.

5. Memory Leak Risk ⚠️

Location: Line 80

The #wsAckQueue accumulates entries per requestId but only clears them during flush. If WebSockets fail to close properly or the flush interval is very long relative to new connection rates, this could grow unbounded.

Recommendation: Consider:

Adding a maximum queue size check
Implementing TTL-based cleanup for stale entries
Logging a warning when queue size exceeds a threshold

Performance Considerations

Flush Interval: The 1-second interval is reasonable given the 30-second gateway timeout, but consider making this configurable for different deployment scenarios.
String Operations: The ArrayBuffer ↔ String conversion happens on every flush. For high-throughput scenarios, consider caching the conversion or using a more efficient key structure.

Testing Recommendations

Missing test coverage for:

Debouncing behavior: verify multiple messages result in a single ack with the highest index
Race conditions: concurrent flushes
Edge cases: WebSocket closes during flush, shutdown during flush
Type correctness: ensure bigint/number types are handled correctly
Memory: verify queue is properly cleared and doesn't leak

Security Considerations

No major security concerns, but the ArrayBuffer conversion could theoretically be exploited if the string representation has collisions.

Summary

Must Fix Before Merge:

Type mismatch (number vs bigint) in #wsAckQueue declaration
Request ID key generation inconsistency between message handler and flush

Should Fix:
3. Race condition protection for concurrent flushes
4. Memory leak prevention with queue size limits

Nice to Have:
5. Test coverage for the new debouncing logic
6. Configurable flush interval

Overall, this is a good optimization, but the type and key generation issues need to be resolved before merging.

claude · 2025-11-05T00:14:49Z

Code Review: WebSocket Message Acknowledgment Debouncing

I've reviewed this PR which adds debouncing for WebSocket message acknowledgments. Overall, the approach is sound and addresses the issue of reducing buffered messages on the gateway. However, I've identified several critical issues that need to be addressed:

Critical Issues

1. Type Mismatch: `number` vs `bigint`

Location: rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts:80, 587-589

The #wsAckQueue is declared as Map<string, number>, but rivetMessageIndex is typed as number | undefined in the interface while it's being compared against -1n (a bigint literal).

// Line 80: Queue stores number
#wsAckQueue: Map<string, number> = new Map();

// Lines 587-589: Comparing number with bigint
const currentMax = this.#wsAckQueue.get(requestId) ?? -1n;
if (event.rivetMessageIndex > currentMax) {
    this.#wsAckQueue.set(requestId, event.rivetMessageIndex);
}

Issue: This will cause type coercion issues. While TypeScript may allow this with implicit coercion, it's inconsistent with other parts of the codebase (line 275 uses msgIndex: -1n as a bigint).

Recommendation: Change the Map type to Map<string, bigint> and use -1n consistently:

#wsAckQueue: Map<string, bigint> = new Map();

2. ArrayBuffer to String Conversion is Fragile

Location: Lines 322-325, 499, 586-589

The conversion of ArrayBuffer to string using comma-separated values is fragile and error-prone:

// Converting to string for Map key
const requestId = idToStr(requestIdBuf);
// ...
this.#wsAckQueue.set(requestId, event.rivetMessageIndex);

// Later, converting back
const requestId = new Uint8Array(
    requestIdStr.split(",").map((x) => Number.parseInt(x)),
).buffer;

Issues:

The code uses idToStr(requestIdBuf) on line 499 but stores it directly as requestId (already a string)
The roundtrip conversion (string → split → parseInt → Uint8Array → buffer) is inefficient
The split(",") assumes the idToStr function produces comma-separated values, creating tight coupling
No validation that the conversion produces the same bytes

Recommendation: Either:

Use idToStr consistently and store the string directly without manual parsing, OR
Create helper functions for safe roundtrip conversion with validation

3. Race Condition: Missing Flush on Actor Stop

Location: Lines 454-474 (#runnerOnActorStop method)

When an actor stops, any pending WebSocket acknowledgments for that actor's connections are not flushed. This could result in lost acknowledgments if:

Messages arrive
They're queued for batching
The actor stops before the 1-second interval triggers

Recommendation: Add a flush call in #runnerOnActorStop:

async #runnerOnActorStop(actorId: string, generation: number): Promise<void> {
    logger().debug({ msg: "runner actor stopping", actorId, generation });
    
    // Flush any pending acks for this actor
    this.#flushWsAcks();
    
    const handler = this.#actors.get(actorId);
    // ... rest of the method
}

Moderate Issues

4. Out-of-Order Message Handling

Location: Lines 587-597

The warning for receiving a lower index than the queued maximum is logged but the message is still processed. Consider if this indicates a more serious problem that should be investigated:

} else {
    logger().warn({
        msg: "received lower index than ack queue for message",
        requestId: requestId,
        queuedMessageIndex: currentMax,
        eventMessageIndex: event.rivetMessageIndex,
    });
}

Question: Should out-of-order messages be rejected or trigger additional error handling? If messages can legitimately arrive out of order, this might indicate a protocol-level issue.

5. No Metrics or Observability

The debouncing logic would benefit from metrics to understand its effectiveness:

Number of acks batched per flush
Time spent in flush operations
Queue size over time

Recommendation: Add debug logging in #flushWsAcks:

#flushWsAcks(): void {
    if (this.#wsAckQueue.size === 0) return;
    
    logger().debug({
        msg: "flushing websocket acks",
        count: this.#wsAckQueue.size,
    });
    
    // ... existing code
}

Minor Issues

6. Magic Number: 1000ms Interval

Location: Line 299

The 1-second interval is hardcoded. Consider making it configurable:

const WS_ACK_FLUSH_INTERVAL_MS = 1000; // Configurable constant
this.#wsAckFlushInterval = setInterval(() => this.#flushWsAcks(), WS_ACK_FLUSH_INTERVAL_MS);

7. Type Safety: Invariants Without TypeScript Guards

Location: Lines 582-583

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

These invariants check for undefined, but TypeScript doesn't know the values are defined afterward. Consider using type guards or non-null assertions for clarity.

Test Coverage

Observation: No test files were found for this driver. The debouncing logic should have tests covering:

Basic batching behavior (multiple acks in < 1s are batched)
Flush on close
Flush on shutdown
Out-of-order message handling
Type conversions (ArrayBuffer ↔ string)

Recommendation: Add unit tests for the new debouncing functionality.

Performance Considerations

The debouncing approach is sound and should reduce the number of ack messages sent to the gateway. The 1-second interval is reasonable given the 30-second gateway timeout.

Potential optimization: If high-throughput scenarios are expected, consider:

Using a more efficient data structure than Map for lookups
Profiling the string conversion overhead

Security Concerns

No significant security issues identified. The code doesn't introduce new attack vectors.

Summary

Must Fix:

Type mismatch between number and bigint
Fragile ArrayBuffer ↔ string conversion
Missing flush on actor stop

Should Fix:
4. Add observability/metrics
5. Consider out-of-order message implications
6. Add test coverage

Nice to Have:
7. Make interval configurable
8. Improve type safety with guards

The core concept is excellent, but these issues need to be addressed before merging to ensure correctness and maintainability.

claude · 2025-11-05T02:17:05Z

Pull Request Review: WebSocket Message Acknowledgment Debouncing

Summary

This PR implements debouncing for WebSocket message acknowledgments to reduce network overhead by batching acks and sending only the highest message index per request. The implementation adds a 1-second flush interval and properly handles cleanup on shutdown.

Code Quality ✅

Strengths:

Clear, well-documented code with helpful inline comments
Proper resource cleanup in shutdownRunner() method
Good use of TypeScript types for the ack queue structure
Appropriate use of Map for tracking acks by request ID

Minor Concerns:

Variable naming inconsistency (line 597):
```
queuedMessageIndex: currentEntry,
```
The log passes the entire currentEntry object but names it queuedMessageIndex. Should be:
```
queuedMessageIndex: currentEntry.messageIndex,
```
Hardcoded interval value (line 302):
The 1-second interval is hardcoded. Consider making this configurable via RunnerConfig for easier tuning:
```
const WS_ACK_FLUSH_INTERVAL_MS = runConfig.wsAckFlushInterval ?? 1000;
```

Potential Bugs 🐛

Race condition on shutdown (lines 627-634):
The interval is cleared before flushing remaining acks. If the interval fires between clearInterval() and #flushWsAcks(), there could be a race. Consider:
```
// Flush first, then clear
this.#flushWsAcks();
if (this.#wsAckFlushInterval) {
    clearInterval(this.#wsAckFlushInterval);
    this.#wsAckFlushInterval = undefined;
}
```
Though in practice this is unlikely to cause issues since the flush is idempotent.
Missing cleanup on WebSocket close per-request (line 611):
When a WebSocket closes, we flush all acks. However, we don't remove the specific request from #wsAckQueue. This is fine since we flush, but future messages for other requests will continue accumulating. Consider adding a comment explaining this behavior is intentional.
Out-of-order message handling (lines 593-600):
The warning for out-of-order messages is logged but doesn't update the queue. This could mean:
- If messages arrive as [5, 3, 7], we'd track 5, warn on 3 (correct), then update to 7 (correct)
- The logic appears correct, but the warning might be too aggressive for normal network reordering
Consider whether this should be a debug log instead of warn, or track both min/max indices.

Performance Considerations ⚡

Positive impacts:

Significant reduction in network overhead by batching acks
1-second interval is a reasonable balance between latency and batching efficiency
Map operations are O(1) for lookups and inserts

Considerations:

The #flushWsAcks() iterates over all queued acks on every interval. With many concurrent WebSockets, this could accumulate

Consider adding metrics/logging to track queue size for monitoring:

#flushWsAcks(): void {
    const queueSize = this.#wsAckQueue.size;
    if (queueSize === 0) return;
    
    // Log if queue is unusually large
    if (queueSize > 1000) {
        logger().warn({ msg: "large ws ack queue", size: queueSize });
    }
    // ... rest of function
}

Security Concerns 🔒

No significant security concerns identified. The implementation:

Properly validates required fields with invariant
Uses ArrayBuffer for request IDs (immutable)
Doesn't expose sensitive data in logs

Test Coverage 📋

Missing test coverage:

No tests included for the new debouncing behavior
Suggested test cases:
1. Verify only highest message index is sent per request
2. Verify acks are flushed on interval
3. Verify acks are flushed on WebSocket close
4. Verify acks are flushed on runner shutdown
5. Verify out-of-order messages are handled correctly
6. Verify multiple concurrent WebSocket requests are tracked independently

Consider adding tests in a file like actor-driver.test.ts or integration tests in the driver test suite.

Recommendations

High Priority:

Fix the logging issue in line 597 to pass currentEntry.messageIndex instead of the full object
Add test coverage for the debouncing behavior

Medium Priority:

Consider making the flush interval configurable
Add queue size monitoring/logging for observability
Clarify whether the out-of-order warning is expected (document or adjust log level)

Low Priority:

Add JSDoc comments to #flushWsAcks() and #wsAckQueue explaining the debouncing strategy
Consider extracting magic numbers (1000ms) into named constants

Overall Assessment

This is a solid implementation that addresses a real performance concern. The code is clean and well-integrated with the existing architecture. The main gaps are around test coverage and some minor improvements to observability. With the suggested fixes, this PR will be production-ready.

Recommendation: Approve with minor changes ✅

Great work on improving WebSocket performance!

graphite-app · 2025-11-05T02:25:04Z

Merge activity

Nov 5, 2:25 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 5, 2:25 AM UTC: CI is running for this pull request on a draft pull request (#3368) due to your merge queue CI optimization settings.
Nov 5, 2:29 AM UTC: Merged by the Graphite merge queue via draft PR: #3368.

NathanFlurry requested a review from MasterPtato November 4, 2025 19:55

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

NathanFlurry mentioned this pull request Nov 4, 2025

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

Closed

NathanFlurry mentioned this pull request Nov 4, 2025

ci: trigger rust ci for generated bare artifacts #3346

Closed

NathanFlurry force-pushed the 11-04-chore_rivetkit_debounce_ws_message_ack branch from 05654a6 to a16734e Compare November 4, 2025 20:25

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

graphite-app bot force-pushed the 11-04-chore_rivetkit_auto-update_auto-start_engine_version branch from 34d8908 to 5ad7f89 Compare November 4, 2025 20:58

graphite-app bot force-pushed the 11-04-chore_rivetkit_debounce_ws_message_ack branch from a16734e to 7973ad8 Compare November 4, 2025 20:59

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

MasterPtato requested changes Nov 5, 2025

View reviewed changes

rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts Outdated Show resolved Hide resolved

rivetkit-typescript/packages/rivetkit/src/drivers/engine/actor-driver.ts Outdated Show resolved Hide resolved

graphite-app bot changed the base branch from 11-04-chore_rivetkit_auto-update_auto-start_engine_version to graphite-base/3345 November 5, 2025 00:11

graphite-app bot force-pushed the graphite-base/3345 branch from 5ad7f89 to 8e33967 Compare November 5, 2025 00:12

graphite-app bot force-pushed the 11-04-chore_rivetkit_debounce_ws_message_ack branch from 7973ad8 to 635c1c0 Compare November 5, 2025 00:12

graphite-app bot changed the base branch from graphite-base/3345 to main November 5, 2025 00:13

graphite-app bot force-pushed the 11-04-chore_rivetkit_debounce_ws_message_ack branch from 635c1c0 to 75c97ad Compare November 5, 2025 00:13

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

chore(rivetkit): debounce ws message ack

2baa4b7

NathanFlurry force-pushed the 11-04-chore_rivetkit_debounce_ws_message_ack branch from 75c97ad to 2baa4b7 Compare November 5, 2025 02:12

NathanFlurry requested a review from MasterPtato November 5, 2025 02:13

vercel bot deployed to Preview – rivetkit-serverless November 5, 2025 02:14 View deployment

MasterPtato approved these changes Nov 5, 2025

View reviewed changes

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

chore(rivetkit): debounce ws message ack (#3345)

ae1ee8b

graphite-app bot closed this Nov 5, 2025

graphite-app bot deleted the 11-04-chore_rivetkit_debounce_ws_message_ack branch November 5, 2025 02:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(rivetkit): debounce ws message ack #3345

chore(rivetkit): debounce ws message ack #3345

NathanFlurry commented Nov 4, 2025

Uh oh!

vercel bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Nov 4, 2025 •

edited

Loading

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!

Uh oh!

Uh oh!

claude bot commented Nov 5, 2025

Uh oh!

claude bot commented Nov 5, 2025

Uh oh!

claude bot commented Nov 5, 2025

Uh oh!

graphite-app bot commented Nov 5, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chore(rivetkit): debounce ws message ack #3345

chore(rivetkit): debounce ws message ack #3345

Conversation

NathanFlurry commented Nov 4, 2025

Uh oh!

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

Uh oh!

Uh oh!

NathanFlurry commented Nov 4, 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 4, 2025

Pull Request Review: Debounce WebSocket Message Acknowledgments

Summary

Code Quality & Best Practices

Performance Considerations

Security Concerns

Test Coverage

Additional Suggestions

Verdict

Uh oh!

claude bot commented Nov 4, 2025

PR Review: Debounce WebSocket Message Acknowledgments

Summary

Code Quality & Best Practices

🔴 Critical: Type Mismatch Bug

🟡 Medium: Inefficient ArrayBuffer Serialization

Performance Considerations

Security Concerns

Potential Bugs

Test Coverage

Action Items

Overall Assessment

Uh oh!

claude bot commented Nov 4, 2025

Pull Request Review: Debounce WebSocket Message Acknowledgments

Summary

Code Quality & Best Practices

Performance Considerations

Potential Bugs

Security Considerations

Test Coverage

Suggested Changes

Questions

Overall Assessment

Uh oh!

Uh oh!

Uh oh!

claude bot commented Nov 5, 2025

Code Review: WebSocket Message Acknowledgment Debouncing

Positive Changes

Issues & Concerns

1. Type Mismatch: number vs bigint ⚠️

2. Unsafe ArrayBuffer-to-String Conversion ⚠️

3. Missing Request ID Conversion in Message Handler 🐛

4. Race Condition: Interval vs Flush on Close ⚠️

5. Memory Leak Risk ⚠️

Performance Considerations

Testing Recommendations

Security Considerations

Summary

Uh oh!

claude bot commented Nov 5, 2025

Code Review: WebSocket Message Acknowledgment Debouncing

Critical Issues

1. Type Mismatch: number vs bigint

2. ArrayBuffer to String Conversion is Fragile

3. Race Condition: Missing Flush on Actor Stop

Moderate Issues

4. Out-of-Order Message Handling

5. No Metrics or Observability

Minor Issues

6. Magic Number: 1000ms Interval

7. Type Safety: Invariants Without TypeScript Guards

Test Coverage

Performance Considerations

Security Concerns

Summary

Uh oh!

vercel bot commented Nov 4, 2025 •

edited

Loading

NathanFlurry commented Nov 4, 2025 •

edited

Loading

1. Type Mismatch: `number` vs `bigint` ⚠️

1. Type Mismatch: `number` vs `bigint`

graphite-app bot commented Nov 5, 2025 •

edited

Loading