chore(runner): add safeguard against duplicate sockets

[NathanFlurry]

No description provided.

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 6, 2025 5:37am

3 Skipped Deployments

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

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore(agents): add graphite-commit agent for conventional commits #3394
• fix(rivetkit): only trigger onStateChange and inspector updates for actor state changes #3393
• fix(rivetkit): reject all waiters in SinglePromiseQueue #3392
• fix(rivetkit): fix hibernatable websocket last seen message index to be i64 instead of u64 #3391
• fix(rivetkit): gracefully call _onStop for actors on runner shutdown #3390
• fix(rivetkit): fix initial hibernation state always being overridden with msgIndex of -1 #3389
• fix(rivetkit): fix missing duplex in nextjs in development #3388
• chore(runner): add safeguard against duplicate sockets #3387 👈 (View in Graphite)
• chore(runner): explicitly stringify tunnel messages, commands, and events #3386
• fix(runner): do not send websocket close on tunnel shutdown since pegboard-runner takes care of this #3385
• fix(rivetkit): fix actors trying to sleep after stop started #3384
• chore(runner): rename webSocketId -> requestId #3383
• chore(runner): add explicit close reason when client runner shuts down #3382
• fix(guard): fix 1006 when client closes ws connection #3381
• chore(runner): add child logger with runner id #3380
• chore(rivetkit): remove warnings for graceful runner shtudown #3379
• fix(gateway): fix gc logic #3378
• fix: boot dupe runners #3377
• fix(rocksdb): fix txn conflict checker #3376
• main

---

How to use the Graphite Merge Queue

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

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

PR Review: Add Safeguard Against Duplicate WebSockets

Summary

This PR adds a defensive safeguard to prevent duplicate WebSocket connections for the same request ID. The implementation is solid and follows good defensive programming practices.

Code Quality ✅

Strengths:

• Clear documentation: The comment block (lines 576-582) excellently explains the invariant, why this should never happen, and the defensive purpose
• Appropriate logging: Uses warn level with structured logging including the request ID - perfect for debugging if this edge case occurs
• Correct cleanup method: Uses __closeWithoutCallback to avoid sending duplicate close messages through the tunnel, which is the right choice since the server already knows about the new connection
• Consistent code style: Follows TypeScript patterns used elsewhere in the codebase (private fields with #, structured logging)
• Good placement: The check happens before tracking the WebSocket, preventing any race conditions

Potential Issues 🔍

Minor considerations:

1. Memory leak prevention: The code correctly removes the old adapter from this.#actorWebSockets via the close callback (line 631 in closeInner). However, it might be worth verifying that actor.webSockets tracking is also cleaned up properly. Looking at __closeWithoutCallback → closeInner (line 201), it doesn't trigger the callback that would clean up actor.webSockets.delete(requestIdStr) (line 635). This could leave a stale entry in the actor's websocket set.

   Recommendation: Consider explicitly cleaning up the actor tracking:

   const existingAdapter = this.#actorWebSockets.get(requestIdStr);
   if (existingAdapter) {
       this.log?.warn({
           msg: "closing existing websocket for duplicate open event for the same request id",
           requestId: requestIdStr,
       });
       existingAdapter.__closeWithoutCallback(1000, "ws.duplicate_open");
       // Explicitly clean up actor tracking if needed
       if (actor) {
           actor.webSockets.delete(requestIdStr);
       }
   }

2. Close code semantics: Using code 1000 (Normal Closure) for a duplicate connection might not be semantically accurate. Consider using:

   • 1008 (Policy Violation) - indicates a protocol/policy violation occurred
   • 1011 (Internal Error) - indicates an unexpected condition

   This would make debugging easier if these events appear in logs.

Performance Considerations ✅

• Map.get() operation is O(1), no performance concerns
• The check is appropriately placed in the hot path without adding significant overhead

Security Concerns ✅

• No security issues identified
• The safeguard actually improves robustness by preventing potential resource exhaustion from duplicate connections

Test Coverage ⚠️

Gap identified: No tests exist for this edge case. While this is a defensive safeguard for a condition that "should never happen," it would be valuable to have a test that:

1. Opens a WebSocket connection
2. Attempts to open another connection with the same request ID
3. Verifies the first connection is closed and the warning is logged
4. Confirms the second connection succeeds

This would ensure the safeguard works correctly and doesn't break in future refactoring.

Test file suggestion: engine/sdks/typescript/runner/src/__tests__/tunnel-websocket-duplicate.test.ts

Architecture Notes 📝

The fix aligns well with the existing error handling patterns:

• Similar defensive checks exist for actor validation (lines 535-556)
• Consistent with the shutdown cleanup logic (lines 84-87)
• Follows the tunnel's message flow architecture

Verdict: ✅ Approve with Minor Suggestions

This is a well-implemented defensive safeguard. The code is clean, well-documented, and handles an edge case properly. The minor suggestions about close code semantics and explicit actor tracking cleanup would make it even more robust, but they're not blockers. The lack of test coverage is noted but acceptable for a defensive "should never happen" case.

Recommended follow-ups (not blocking):

1. Add explicit actor.webSockets cleanup
2. Consider more semantic close code
3. Add integration test for the duplicate scenario

[pkg-pr-new]

More templates

• example-ai-agent
• example-background-jobs
• example-better-auth-external-db
• example-bots
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• example-counter
• example-counter-serverless
• example-crdt
• example-cursors
• example-cursors-raw-websocket
• example-database
• example-deno
• example-sqlite
• example-elysia
• example-express
• example-freestyle
• example-game
• example-hono
• example-hono-bun
• example-hono-react
• example-kitchen-sink
• example-next-js
• example-rate
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• starter
• example-stream
• example-sync
• example-tenant
• example-trpc
• example-workflows

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 7091bb6

[claude]

Code Review

Summary

This PR adds a defensive safeguard to prevent duplicate WebSocket connections for the same request ID in the tunnel handler. The change is well-implemented and addresses a potential edge case.

Positive Aspects ✅

1. Good defensive programming: The safeguard prevents an edge case that shouldn't happen in normal operation but provides safety if it does occur
2. Appropriate logging: Uses warn level logging with structured logging pattern (?requestId) as per CLAUDE.md guidelines
3. Correct cleanup: Uses __closeWithoutCallback to avoid sending duplicate close messages through the tunnel, which is the right approach since the server already knows about the new connection
4. Clear documentation: Excellent inline comments explaining why this edge case is being handled and the rationale for the approach

Code Quality

Logging Pattern ✅
The logging follows the project's structured logging conventions correctly:

this.log?.warn({
    msg: "closing existing websocket for duplicate open event for the same request id",
    requestId: requestIdStr,
});

This is properly using structured logging with separate fields instead of string interpolation.

Message Formatting ⚠️
Minor style note: The log message could be slightly more concise following the lowercase convention in CLAUDE.md:

msg: "closing existing websocket for duplicate open event"

The requestId field already provides the context, so repeating "for the same request id" in the message is somewhat redundant.

Technical Correctness

Proper Cleanup Order ✅
The code correctly:

1. Checks for existing adapter before creating a new one (lines 583-592)
2. Closes without callback to avoid duplicate tunnel messages
3. Cleans up using the internal close method that won't trigger the close callback

WebSocket State Management ✅
Looking at websocket-tunnel-adapter.ts:200-202, the __closeWithoutCallback method correctly:

• Transitions the WebSocket state properly (CONNECTING/OPEN → CLOSING → CLOSED)
• Fires close events to notify client code
• Skips the tunnel callback to avoid duplicate messages

Potential Concerns

Race Condition Coverage 🤔
The safeguard handles the case where two open messages arrive for the same request ID, but I wonder:

• Could there be a race between this check and the this.#actorWebSockets.set(requestIdStr, adapter) call on line 641?
• Between checking (line 583) and setting (line 641), another open message could theoretically arrive

However, given that this is handling messages sequentially through handleTunnelMessage, this shouldn't be a practical concern unless messages are processed concurrently (which doesn't appear to be the case from the code structure).

Missing Test Coverage ⚠️

• No test files found for tunnel.ts or WebSocket duplicate handling
• This edge case would benefit from a unit test to ensure:
  • Existing WebSocket is properly closed when duplicate arrives
  • New WebSocket is created successfully
  • No tunnel messages are sent for the old WebSocket close
  • Warning is logged appropriately

Security Considerations

No security concerns identified. The change:

• Doesn't introduce new attack vectors
• Properly cleans up resources to prevent leaks
• Uses existing validated code paths for cleanup

Performance Considerations

Minimal Impact ✅

• Single Map lookup: O(1)
• Only executes in edge case scenario
• No performance concerns

Recommendations

1. Optional: Consider shortening the log message to:

   msg: "closing existing websocket for duplicate open event"

2. Suggested: Add a test case for this scenario:

   // Test: duplicate websocket open messages for same request ID
   // Expected: first websocket closed without callback, second websocket created

3. Documentation: Consider adding a comment about why this edge case might occur (network retries, message reordering, etc.) if the root cause is known

Conclusion

This is a solid defensive programming change that improves system resilience. The implementation is correct, follows project conventions, and the safeguard is well-placed. The main gap is test coverage for this edge case scenario.

Recommendation: Approve with optional improvements suggested above

---

🤖 Generated with Claude Code

[claude]

PR Review: Safeguard Against Duplicate WebSocket Connections

Summary

This PR adds defensive logic to prevent duplicate WebSocket connections for the same request ID by detecting and closing any existing connection before opening a new one.

Code Quality ✅

Strengths:

• Well-commented code that explains both the expected behavior and why this safeguard exists
• Follows existing patterns in the codebase (uses __closeWithoutCallback similar to tunnel.ts:86)
• Appropriate logging with structured fields following the project's logging conventions
• Clean placement of the check before creating the new adapter

Code Structure:
The implementation correctly:

1. Checks for existing adapter before creating a new one
2. Uses __closeWithoutCallback to avoid sending redundant messages
3. Allows the existing cleanup to proceed naturally (actor tracking will be cleaned up when the close event fires)

Potential Issues ⚠️

1. Race Condition with Actor Tracking

There's a subtle issue with actor tracking cleanup. When you call __closeWithoutCallback, it:

• Sets the WebSocket state to CLOSING → CLOSED
• Fires a "close" event to any listeners
• Does NOT trigger the close callback (line 621-637 in tunnel.ts)

This means the actor tracking cleanup at tunnel.ts:634-636 will not execute for the duplicate connection:

// Clean up actor tracking
if (actor) {
    actor.webSockets.delete(requestIdStr);
}

However, line 596 immediately re-adds the same requestIdStr:

if (actor) {
    actor.webSockets.add(requestIdStr);
}

Impact: This appears to be benign in practice since you're re-adding the same ID, but the old adapter's reference in actor.webSockets is orphaned without cleanup. If actor tracking is used for resource management or metrics, this could cause subtle issues.

Recommendation: Consider explicitly cleaning up actor tracking before closing:

const existingAdapter = this.#actorWebSockets.get(requestIdStr);
if (existingAdapter) {
    this.log?.warn({
        msg: "closing existing websocket for duplicate open event for the same request id",
        requestId: requestIdStr,
    });
    
    // Clean up actor tracking for the old connection
    if (actor) {
        actor.webSockets.delete(requestIdStr);
    }
    
    // Close without sending a message through the tunnel since the server
    // already knows about the new connection
    existingAdapter.__closeWithoutCallback(1000, "ws.duplicate_open");
}

2. Map Cleanup

The existingAdapter is not removed from this.#actorWebSockets before being closed. While line 641 will overwrite it with set(), it's cleaner to explicitly delete first:

if (existingAdapter) {
    // ... logging ...
    
    existingAdapter.__closeWithoutCallback(1000, "ws.duplicate_open");
    this.#actorWebSockets.delete(requestIdStr);  // Explicit cleanup
}

This makes the cleanup more obvious and prevents any intermediate state where both old and new adapters exist in the map.

Performance Considerations ✅

• The Map.get() lookup is O(1) - negligible performance impact
• Only executes in an edge case scenario (duplicate opens)
• No performance concerns

Security Considerations ✅

• Prevents resource leaks from duplicate WebSocket connections
• Properly closes existing connections before creating new ones
• Uses appropriate close code (1000 = normal closure)
• No security concerns identified

Test Coverage ⚠️

Missing:

• No test coverage for the duplicate WebSocket scenario
• The existing tests in tests/lifecycle.test.ts don't appear to cover this edge case

Recommendation:
Consider adding a test case that:

1. Opens a WebSocket connection
2. Sends another open message for the same request ID (without closing first)
3. Verifies the old connection is closed
4. Verifies the new connection is established correctly
5. Verifies actor tracking is correct (if applicable)

Overall Assessment

Verdict: ✅ Approve with minor recommendations

This is a good defensive programming practice that prevents an edge case bug. The core logic is sound, but there are two minor cleanup improvements to consider:

1. Critical: Explicitly clean up actor tracking before closing the duplicate adapter
2. Nice-to-have: Explicitly delete from map before overwriting
3. Nice-to-have: Add test coverage for this scenario

The PR is safe to merge as-is since the issue it prevents is worse than the minor cleanup concern, but the recommendations would make it more robust.

---

Related Code References:

• Close callback implementation: tunnel.ts:619-637
• Actor tracking: tunnel.ts:594-596, tunnel.ts:634-636
• Similar usage pattern: tunnel.ts:86
• WebSocket close handling: tunnel.ts:753-767

[claude]

Pull Request Review: Add Safeguard Against Duplicate WebSocket Sockets

Summary

This PR adds defensive code to handle edge cases where duplicate WebSocket open events might occur for the same request ID. The implementation is sound and follows good defensive programming practices.

Code Quality ✅

Strengths:

1. Clear documentation - The comment block (lines 576-582) clearly explains the rationale and expected behavior
2. Appropriate logging - Uses structured logging with this.log?.warn() including the request ID for debugging
3. Consistent with existing patterns - Follows the existing code style and uses similar patterns found elsewhere (e.g., shutdown() at line 86)
4. Proper cleanup - Correctly uses __closeWithoutCallback() to avoid sending duplicate close messages to the server

Code style adherence:

• ✅ Uses structured logging with context parameters (?requestId)
• ✅ Log message is lowercase (follows CLAUDE.md guidelines)
• ✅ Consistent indentation and formatting with the rest of the file

Implementation Analysis ✅

Correctness:
The implementation correctly:

1. Checks for an existing adapter before creating a new one
2. Closes the existing adapter without triggering the close callback (avoiding a race condition where the server would receive a close for a connection it's already replacing)
3. Removes the adapter from the map implicitly (the new adapter will overwrite it on line 641)

Edge case handling:

• The safeguard appropriately handles the "should never happen" scenario
• Using __closeWithoutCallback() is correct since the server has already initiated the new connection and doesn't need to be notified about the old one being closed

Potential Issues & Suggestions

1. Memory Leak Prevention ⚠️

The existing adapter is closed, but there's a potential subtle issue: the adapter is still tracked in actor.webSockets (added on line 596) until it's closed via the close callback. Since we're calling __closeWithoutCallback(), the cleanup in the close callback (lines 634-636) won't run for the old adapter.

Recommendation: Explicitly clean up the old adapter from actor tracking:

const existingAdapter = this.#actorWebSockets.get(requestIdStr);
if (existingAdapter) {
	this.log?.warn({
		msg: "closing existing websocket for duplicate open event for the same request id",
		requestId: requestIdStr,
	});
	// Clean up actor tracking for the old adapter
	if (actor) {
		actor.webSockets.delete(requestIdStr);
	}
	// Close without sending a message through the tunnel since the server
	// already knows about the new connection
	existingAdapter.__closeWithoutCallback(1000, "ws.duplicate_open");
}

This ensures the actor.webSockets set doesn't grow unbounded if duplicates occur repeatedly.

2. Close Code Consideration 💭

Using code 1000 (Normal Closure) might be slightly misleading since this is an abnormal situation. Consider:

• 1001 (Going Away) - might be more semantically appropriate
• 1011 (Internal Error) - used elsewhere in the file for unexpected conditions

Not a blocker, but worth considering for consistency with similar error handling in the codebase.

3. Test Coverage ❓

Questions to consider:

• Are there tests that verify this edge case?
• Should we add a test that simulates duplicate open events to ensure this safeguard works correctly?

Given that this is a defensive safeguard for a scenario that "should never occur," having a test would provide confidence that the code behaves correctly if the unexpected does happen.

Performance Considerations ✅

The performance impact is negligible:

• Single Map lookup (O(1))
• Only executed on WebSocket open (infrequent operation)
• No blocking operations

Security Considerations ✅

No security concerns identified. The change:

• Prevents potential resource exhaustion from duplicate WebSocket connections
• Properly cleans up resources
• Logs the anomaly for debugging/monitoring

Final Verdict

Recommendation: Approve with minor suggestion

This is a solid defensive programming change that prevents a potential edge case. The only concern is the minor cleanup issue mentioned above. I'd recommend adding the actor tracking cleanup to prevent potential memory growth over time.

The PR is safe to merge as-is, but addressing the actor tracking cleanup would make it more robust.

---

Overall Score: 8.5/10

• Code quality: Excellent
• Implementation: Very good
• Documentation: Excellent
• Minor improvement opportunity: Actor tracking cleanup

🤖 Generated with Claude Code

[claude]

PR Review: Add Safeguard Against Duplicate WebSocket Connections

Summary

This PR adds defensive code to handle the edge case where a duplicate WebSocket open event is received for the same request ID. The safeguard closes the existing WebSocket before creating a new one.

Code Quality ✅

Positive aspects:

• Clear, comprehensive comments explaining the edge case and rationale
• Appropriate use of structured logging following the codebase's tracing patterns (lowercase message, ?requestId parameter)
• Correct use of __closeWithoutCallback to avoid sending duplicate messages through the tunnel
• Proper placement: the check occurs before actor tracking and adapter creation

Style compliance:

• Follows the repository's logging conventions with lowercase messages and structured parameters
• Comment style is consistent with surrounding code
• Indentation and formatting align with existing code

Potential Issues 🔍

Minor concerns:

1. No test coverage: This PR adds defensive logic for an edge case, but there are no tests to verify this behavior. Consider adding a test case that:

   • Simulates receiving two ToClientWebSocketOpen messages with the same request ID
   • Verifies the first WebSocket is closed properly
   • Ensures the second WebSocket is created successfully
   • Confirms only one WebSocket exists in #actorWebSockets after both messages

2. Actor tracking inconsistency: If a duplicate open event occurs, the existing WebSocket is closed but the actor's webSockets Set may still contain the requestIdStr. The code at line 595-597 adds the request ID again:

   if (actor) {
       actor.webSockets.add(requestIdStr);
   }

   Since Sets ignore duplicates, this is safe but could be more explicit. However, the real issue is that when __closeWithoutCallback is called, it doesn't trigger the close callback that normally removes the entry from actor.webSockets (line 635). This means the actor tracking might be out of sync.

3. Missing cleanup in edge case: When existingAdapter.__closeWithoutCallback() is called, the adapter is closed but not removed from this.#actorWebSockets. The new adapter immediately overwrites it at line 641, but there's a brief moment where the map contains a closed adapter. Consider explicitly calling:

   this.#actorWebSockets.delete(requestIdStr);

   after closing the existing adapter for clarity.

Performance Considerations ⚡

• The Map lookup (this.#actorWebSockets.get()) is O(1), so negligible performance impact
• This is a defensive check for an error condition, so any overhead is acceptable

Security Concerns 🔒

• No security issues identified
• The safeguard actually improves reliability by preventing resource leaks from duplicate connections
• Proper use of close code 1000 (normal closure) with descriptive reason

Recommendations 📋

High priority:

1. Add test coverage for this edge case
2. Verify actor tracking cleanup - ensure actor.webSockets doesn't contain stale entries after __closeWithoutCallback

Low priority:

1. Consider explicitly deleting from #actorWebSockets before closing for code clarity
2. Add a metric/counter for how often this edge case occurs in production to determine if it's worth investigating the root cause

Verdict

This is a good defensive programming practice that prevents potential issues. The implementation is mostly correct, but would benefit from test coverage and verification of the actor tracking cleanup logic.

---

🤖 Generated with Claude Code

[claude]

PR Review: Add safeguard against duplicate sockets

Summary

This PR adds defensive code to handle an edge case where duplicate WebSocket open events could occur for the same request ID. The implementation is generally sound but there are a few considerations.

Code Quality ✅

Strengths:

• Clear, well-commented code explaining the intent and edge case
• Appropriate use of warning-level logging for unexpected conditions
• Proper use of the internal __closeWithoutCallback method to avoid sending duplicate close messages

Observations:

• The code follows TypeScript conventions appropriately
• Logging uses structured format with request ID context (requestId: requestIdStr)

Potential Issues ⚠️

1. Resource Cleanup Concern
When closing the existing adapter with __closeWithoutCallback(1000, "ws.duplicate_open"), the callback parameter is set to false, which means the close callback won't be invoked. Looking at tunnel.ts:619-637, the close callback is responsible for:

• Removing the socket from this.#actorWebSockets map
• Cleaning up actor tracking (actor.webSockets.delete(requestIdStr))

Since the callback isn't triggered, the old adapter won't be removed from the map or the actor tracking before the new one is added. However, this appears to be intentional since line 641 immediately overwrites the map entry with this.#actorWebSockets.set(requestIdStr, adapter).

Potential issue: The actor.webSockets set may not be cleaned up properly. If the old adapter had the socket tracked in actor.webSockets, it won't be removed when using __closeWithoutCallback. This could lead to a memory leak in the actor's websocket tracking set.

Recommendation: Consider explicitly cleaning up the actor tracking before closing:

const existingAdapter = this.#actorWebSockets.get(requestIdStr);
if (existingAdapter) {
    this.log?.warn({
        msg: "closing existing websocket for duplicate open event for the same request id",
        requestId: requestIdStr,
    });
    
    // Clean up actor tracking explicitly
    if (actor) {
        actor.webSockets.delete(requestIdStr);
    }
    
    // Close without sending a message through the tunnel since the server
    // already knows about the new connection
    existingAdapter.__closeWithoutCallback(1000, "ws.duplicate_open");
}

2. Race Condition Consideration
While this code handles the edge case well, it's worth considering: if duplicate open events are occurring, there may be a deeper synchronization issue elsewhere in the system. The comment states "This should never occur if all is functioning correctly" - it would be valuable to understand if/when this condition is actually being triggered in production.

Performance Considerations ✅

• The Map lookup (get) is O(1), so minimal performance impact
• Only executes in edge cases, so no performance concerns

Security Concerns ✅

• Proper cleanup prevents resource exhaustion from duplicate sockets
• Close code 1000 (normal closure) is appropriate
• No security issues identified

Test Coverage ⚠️

Missing test coverage: The existing test file (tests/lifecycle.test.ts) is currently commented out. There are no active tests covering:

• Normal WebSocket lifecycle
• This specific duplicate open event edge case

Recommendation:

1. Uncomment and fix existing WebSocket tests
2. Add a specific test case for duplicate open events:

it("handles duplicate websocket open events gracefully", async () => {
    // Test that opening a WebSocket with the same request ID twice
    // closes the first one and establishes the second
});

Best Practices ✅

• Follows the repository's TypeScript patterns
• Logging message is lowercase as per conventions (though the codebase is TypeScript, not Rust where this convention is mentioned in CLAUDE.md)
• Good defensive programming practice

Overall Assessment

This is a solid defensive programming change that handles an edge case appropriately. The main concerns are:

1. Minor: Potential memory leak in actor.webSockets tracking set
2. Moderate: Lack of test coverage for this edge case and WebSocket functionality in general

Recommendation: Approve with minor fixes for the actor tracking cleanup.

---

🤖 Generated with Claude Code

[claude]

PR Review: Safeguard Against Duplicate WebSocket Connections

Summary

This PR adds a defensive check to prevent duplicate WebSocket connections for the same request ID by closing any existing WebSocket before creating a new one.

Code Quality: ✅ Good

Positives:

• Clean, well-commented code that explains the intent and rationale
• Follows the existing code style and patterns in the file
• Proper use of the structured logging pattern (using ?requestId syntax)
• Uses the appropriate __closeWithoutCallback method to avoid sending redundant tunnel messages
• Log message is lowercase following the project conventions

Suggestions:

1. Consider tracking cleanup of actor.webSockets: When closing the existing adapter, you might want to also remove it from the actor's WebSocket tracking set to maintain consistency:

   if (existingAdapter) {
       this.log?.warn({
           msg: "closing existing websocket for duplicate open event for the same request id",
           requestId: requestIdStr,
       });
       // Clean up actor tracking before replacing the adapter
       if (actor) {
           actor.webSockets.delete(requestIdStr);
       }
       existingAdapter.__closeWithoutCallback(1000, "ws.duplicate_open");
   }

   Otherwise, there's a brief moment where actor.webSockets contains the requestId but points to a closed adapter, which could be confusing during debugging. That said, line 596 immediately re-adds it, so the inconsistency window is minimal.

Potential Issues: ⚠️ Minor

1. Race Condition Window: There's still a theoretical race condition between checking for the existing adapter (line 583) and setting the new one (line 641). However, this is likely not an issue in practice since:

   • WebSocket messages should be processed sequentially in the message handler
   • The comment correctly states this "should never occur if all is functioning correctly"
   • This is a defensive safeguard, not a fix for an expected condition

2. Close Code Selection: Using code 1000 (Normal Closure) for a duplicate connection scenario might not be the most semantically correct. Consider using 1008 (Policy Violation) or 1011 (Internal Error) to better indicate that this is an abnormal closure. However, since this is an internal cleanup and the client never sees this code, it's a minor point.

Performance: ✅ Good

• Minimal performance impact (just a Map lookup before creating a new WebSocket)
• No blocking operations or unnecessary async calls

Security: ✅ Good

• Prevents potential resource leaks from duplicate WebSocket connections
• Closes connections properly without leaving dangling references
• No security concerns identified

Test Coverage: ⚠️ No Tests

Note: I couldn't find any test files for the tunnel functionality. While this is a defensive safeguard for an edge case that "should never occur," adding a test would be valuable to:

1. Verify the behavior when duplicate open events are received
2. Ensure the old adapter is properly closed
3. Confirm the new adapter replaces the old one in the map
4. Validate that the warning is logged

Example test scenario:

// Send two open events with the same requestId
// Verify that:
// - Warning is logged
// - Old adapter is closed with code 1000 and reason "ws.duplicate_open"
// - New adapter is stored in the map
// - actor.webSockets still contains the requestId

Recommendation: ✅ Approve with Minor Suggestions

This is a solid defensive programming practice that prevents a potential edge case. The code is well-written and follows project conventions. The suggestions above are minor improvements, not blockers.

Optional Improvements:

• Consider adding explicit cleanup of actor.webSockets.delete(requestIdStr) before closing the existing adapter for consistency
• Consider adding a test case to verify this behavior
• Consider using a more semantically appropriate close code (e.g., 1008 or 1011)

Great work on identifying and addressing this edge case! 👍

[graphite-app]

Merge activity

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