chore(engine-runner): remove echo close on ToClientWebSocketClose

[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	Error			Nov 20, 2025 8:52pm

3 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Nov 20, 2025 8:52pm
rivet-inspector	Ignored	Preview		Nov 20, 2025 8:52pm
rivet-site	Ignored	Preview		Nov 20, 2025 8:52pm

[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

• fix(tunnel): implement ping pong #3491 : 2 dependent PRs (#3475 , #3494 )
• chore(pegboard-gateway): add new message id format & add deprecated tunnel ack #3492
• chore(pegboard-gateway): remove TunnelAck #3488
• chore(engine-runner): remove echo close on ToClientWebSocketClose #3487 👈 (View in Graphite)
• chore(rivetkit): implement new hibernating ws protocol #3464 : 1 other dependent PR (#3452 )
• fix(actor-kv): fix listing keys #3484
• fix(guard): handle actor stopped during ws/req #3481
• chore: rename retry -> hibernate in runner protocol #3473
• fix(udb): update backoff algo #3469
• chore: write/update docs #3468
• chore: add debug scripts #3467
• 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]

Code Review

I've reviewed this PR and the changes look good overall. The PR removes the echo close behavior on ToClientWebSocketClose and simplifies the WebSocket close handling logic.

Summary of Changes

The PR modifies two TypeScript files in the engine runner SDK:

1. tunnel.ts:

   • Removes the hibernate parameter from the close callback signature
   • Hardcodes hibernate: false in ToServerWebSocketClose messages sent by the runner
   • Replaces _closeWithHibernate() call with standard close() on timeout
   • Adds clarifying comment that close response isn't needed when handling ToClientWebSocketClose
   • Improves error message for dropped tunnel data

2. websocket-tunnel-adapter.ts:

   • Removes the hibernate parameter from the close callback signature
   • Removes the _closeWithHibernate() method entirely
   • Simplifies #closeInner() by removing the hibernate parameter
   • Updates all call sites to use the simplified signature

Analysis

Protocol Correctness ✅

Based on the protocol schema (v3.bare:265-269), ToServerWebSocketClose has a hibernate: bool field. This PR always sets it to false from the runner side, which is correct based on the architectural design:

• Per pegboard-gateway/src/lib.rs:428-433, the gateway checks if both can_hibernate && close.hibernate to trigger hibernation
• Per the hibernation docs (line 215): "Automatically hibernates WS, runner does not need to do anything"
• The runner shouldn't be deciding hibernation - that's the gateway's responsibility based on whether the connection was opened as hibernatable

Behavioral Impact ✅

Looking at pegboard-gateway/src/lib.rs:425-433:

protocol::ToServerTunnelMessageKind::ToServerWebSocketClose(close) => {
    if can_hibernate && close.hibernate {
        return Err(WebSocketServiceHibernate.build());
    } else {
        return Ok(LifecycleResult::ServerClose(close));
    }
}

Since close.hibernate is now always false, the gateway will never enter the hibernation path from a runner-initiated close. This is correct because:

1. Hibernation should occur when the gateway/pegboard closes the connection (via ToClientWebSocketClose)
2. When the runner closes, it's an explicit termination, not hibernation

Code Quality ✅

• Removes unused complexity from the WebSocket adapter
• Simplifies the close callback signature
• The removal of _closeWithHibernate() method makes the API cleaner
• Better error messages ("ws.ack_timeout" is more specific than "Message acknowledgment timeout")

Potential Concerns

Minor: Protocol Field Unused 🟡

The hibernate field in ToServerWebSocketClose is now always false from the runner. Consider whether this field should be removed from the protocol schema in a future protocol version, or if there are other consumers that might use it. However, this is not blocking for this PR.

Question: Zombie Connection Cleanup ❓

In tunnel.ts:513-524, the timeout handler now uses close(1000, "ws.ack_timeout") instead of _closeWithHibernate(). Can you confirm this won't affect the cleanup of zombie/stale connections during actor restoration? Based on the docs (Phase 3.2/3.3 in HIBERNATABLE_CONNECTIONS.md), it seems fine, but worth confirming.

Recommendations

✅ Approve: The changes are correct and improve code clarity. The removal of the echo close behavior aligns with the documented architecture where hibernation is managed by the gateway, not the runner.

Suggestion: Consider adding a comment in the protocol schema or documentation explaining that ToServerWebSocketClose.hibernate should always be false from the runner side, to prevent future confusion.

Test Coverage: Ensure integration tests cover:

• Normal WebSocket close flow (runner-initiated)
• Gateway-initiated close with hibernation
• Timeout-based cleanup with the new close code

---

Great cleanup! 🎉

[claude]

Updated Code Review

I've reviewed this PR again with fresh analysis. The changes are solid and correctly simplify the WebSocket close handling.

Summary

This PR removes the runner's ability to echo close messages with the hibernate flag, always setting it to false. This aligns with the correct architecture where hibernation decisions are made by the gateway, not the runner.

Key Changes Analysis

1. Protocol Alignment ✅

The ToServerWebSocketClose protocol message includes a hibernate: bool field (v3.bare:268). This PR ensures the runner always sends false, which is architecturally correct:

• Gateway controls hibernation based on can_hibernate && close.hibernate (pegboard-gateway/src/lib.rs:428)
• Runner shouldn't decide hibernation - that's the gateway's job based on connection capabilities
• Comment at tunnel.ts:1070 correctly notes "We don't need to send a close response"

2. Removed Complexity ✅

• Removed unused _closeWithHibernate() method
• Simplified #closeInner() signature from 3 to 2 parameters
• Cleaner close callback: (code?, reason?) instead of (code?, reason?, hibernate?)
• Better error message: "ws.ack_timeout" vs "Message acknowledgment timeout"

3. Behavioral Correctness ✅

The timeout handler change (tunnel.ts:516) from _closeWithHibernate() to close() is correct:

• Both ultimately call the close callback
• Timeout scenarios should terminate, not hibernate
• Cleanup still happens via actor.webSockets.delete()

4. Error Message Improvement 👍

Line 400 improves logging: "cannot send tunnel message, socket not connected to engine. tunnel data dropped." - more explicit about data loss.

Testing Observations

The test file (lifecycle.test.ts) is currently commented out. While this PR's changes are straightforward, consider:

• Re-enabling WebSocket close tests to verify behavior
• Testing timeout-based cleanup with new close code
• Verifying gateway correctly handles hibernate: false

Minor Suggestions

1. Documentation: Consider adding a comment in the protocol schema explaining ToServerWebSocketClose.hibernate is always false from runners to prevent future confusion.

2. Protocol Evolution: In a future protocol version, consider removing the hibernate field from ToServerWebSocketClose since it's now unused, or documenting its specific use cases.

Verdict

✅ LGTM - Clean refactoring that correctly implements the architecture. The removal of echo close behavior and the hibernate parameter simplifies the codebase without breaking functionality.

---

Files reviewed:

• engine/sdks/typescript/runner/src/tunnel.ts:317-328
• engine/sdks/typescript/runner/src/tunnel.ts:513-516
• engine/sdks/typescript/runner/src/tunnel.ts:1067-1076
• engine/sdks/typescript/runner/src/websocket-tunnel-adapter.ts:65-68
• engine/sdks/typescript/runner/src/websocket-tunnel-adapter.ts:320-337

[claude]

Code Review

Thanks for this cleanup! This PR simplifies the WebSocket close handling by removing the hibernate parameter. Here's my detailed review:

✅ Positive Changes

1. Simplification: Removing the unused hibernate parameter from the close callback signature makes the code cleaner and easier to understand.

2. Consistent behavior: All ToServerWebSocketClose messages now consistently set hibernate: false, which aligns with the comment at tunnel.ts:1070 indicating "We don't need to send a close response".

3. Removed dead code: The _closeWithHibernate method has been properly removed as it's no longer needed.

4. Better error message: The improved log message at tunnel.ts:400 (cannot send tunnel message, socket not connected to engine. tunnel data dropped.) provides better clarity about what happens when messages can't be sent.

🔍 Observations & Questions

1. Hibernate always false: The PR hardcodes hibernate: false in all ToServerWebSocketClose messages. Is this intentional, or was the hibernate feature planned but never implemented? If hibernation was a planned feature, this PR effectively removes that capability permanently.

2. Protocol consistency: The ToServerWebSocketClose protocol message still includes the hibernate field (as seen in stringify.ts:64). If this field will always be false, consider whether it should be removed from the protocol definition as well for consistency.

3. Comment clarity at tunnel.ts:516: The timeout handler now calls webSocket.close(1000, "ws.ack_timeout"), which changed from the previous _closeWithHibernate call. The close reason changed from "Message acknowledgment timeout" to "ws.ack_timeout" - is this intentional naming convention change?

🐛 Potential Issues

1. State machine consistency: In websocket-tunnel-adapter.ts, the _handleClose method at line 232 calls this.#closeInner(code, reason, true) (with callback=true), while close() at line 494 also calls this.#closeInner(code, reason, true). However, when the engine sends a ToClientWebSocketClose, the adapter calls _handleClose which will trigger the close callback, even though the comment at tunnel.ts:1070 says "We don't need to send a close response".

   Issue: When the engine closes the WebSocket via ToClientWebSocketClose, the adapter will still try to send a ToServerWebSocketClose back because _handleClose has callback=true. This could create an unnecessary echo/loop of close messages.

   Suggested fix: Change line 232 in websocket-tunnel-adapter.ts:

   // Change from:
   this.#closeInner(code, reason, true);
   // To:
   this.#closeInner(code, reason, false);

2. Inconsistent engine close handling: At tunnel.ts:320-331, there's a check for engineAlreadyClosed before sending ToServerWebSocketClose. However, at tunnel.ts:1071, when handling ToClientWebSocketClose, we call adapter._handleClose() which will trigger the callback (as noted above), but there's no engineAlreadyClosed flag being set or checked.

🧪 Test Coverage

The test file lifecycle.test.ts appears to be commented out entirely. Consider:

• Adding test coverage for WebSocket close scenarios
• Testing that close messages don't create echo loops
• Verifying proper cleanup of webSockets map and requestToActor map

📋 Minor Suggestions

1. Consistency in close reasons: The codebase uses different naming conventions for close reasons:

   • tunnel.ts:516: "ws.ack_timeout" (dot notation)
   • tunnel.ts:560: "actor.stopped" (dot notation)
   • tunnel.ts:152: "ws.meta_not_found_during_restore" (dot notation with underscores)

   Consider documenting the naming convention for close reasons.

2. Type safety: The closeCallback type changed but the private field type at line 50-52 still shows the old 3-parameter signature in the diff context. Ensure this was updated.

🎯 Summary

Overall this is a good cleanup PR that simplifies the close handling. The main concern is the potential for echoing close messages when the engine initiates the close. I'd recommend addressing the callback flag in _handleClose to prevent unnecessary network traffic.

Recommendation: Request changes to fix the potential close message echo issue before merging.

[claude]

Code Review

Summary

This PR removes the hibernate parameter echo behavior from WebSocket close operations. The changes simplify the close flow by always setting hibernate: false when sending ToServerWebSocketClose messages to the engine.

Positive Changes ✅

1. Simplified API: Removing the hibernate parameter from close callbacks makes the API cleaner and easier to understand
2. Removed dead code: The _closeWithHibernate method was removed, reducing code complexity
3. Clearer intent: The comment at tunnel.ts:1070 ("We don't need to send a close response") documents the design decision
4. Better error messaging: Enhanced the warning message to include "tunnel data dropped" for better debugging

Issues & Concerns 🔍

1. Breaking Protocol Change - Needs Documentation

The ToServerWebSocketClose protocol still includes the hibernate field (runner-protocol/src/index.ts:1183-1187):

export type ToServerWebSocketClose = {
    readonly code: u16 | null
    readonly reason: string | null
    readonly hibernate: boolean  // Still present in protocol
}

Issue: While the client now always sends hibernate: false, the protocol definition hasn't changed. This could cause confusion.

Recommendations:

• Document why hibernate is always false in the protocol comments
• OR consider removing the field from the protocol entirely if it's no longer used (requires engine-side changes)
• Add migration notes if this is a breaking change for existing deployments

2. Inconsistent Behavior with Hibernatable WebSockets

The code still has hibernatable WebSocket support (tunnel.ts:92-94):

// Only close non-hibernatable websockets to prevent sending
// unnecessary close messages for websockets that will be hibernated
if (\!ws[HIBERNATABLE_SYMBOL]) {

Issue: There's a disconnect between:

• The existence of hibernatable WebSocket infrastructure
• Always sending hibernate: false on close

Questions:

• Is the hibernatable WebSocket feature still in use?
• If yes, should close operations for hibernatable WebSockets send hibernate: true?
• If no, consider removing the hibernatable WebSocket code to reduce maintenance burden

3. Potential Race Condition in Timeout Handler

At tunnel.ts:516, the timeout handler now calls webSocket.close() instead of _closeWithHibernate():

webSocket.close(1000, "ws.ack_timeout");

Issue: The .close() method calls #closeInner(code, reason, true) which triggers the close callback. This might send a ToServerWebSocketClose message even during a timeout scenario.

Question: Is this the intended behavior? The timeout already indicates a network/communication issue, so echoing a close message might be redundant.

4. Missing Test Coverage

No test changes were included in this PR.

Recommendations:

• Add tests for WebSocket close behavior (normal close, timeout close, engine-initiated close)
• Verify that hibernate: false is always sent
• Test that no duplicate close messages are sent

Code Quality

Good:

• Follows existing code style
• Simplifies the codebase by removing unused parameters
• Clear commit message following conventional commits

Suggestions:

• Consider adding JSDoc comments explaining the close behavior
• The callback parameter in #closeInner could be renamed to shouldNotifyEngine for clarity

Security & Performance

✅ No security concerns identified
✅ Performance impact is neutral (slightly positive due to code simplification)

Recommendation

Conditional Approval - The code changes are sound, but need:

1. Documentation/clarification on the protocol design decision
2. Resolution of the hibernatable WebSocket question
3. Test coverage for the new behavior

Please address the issues mentioned above before merging.

[claude]

PR Review: Remove Echo Close on ToClientWebSocketClose

Summary

This PR simplifies the WebSocket close flow by removing the hibernate parameter and related functionality. The changes hard-code hibernate: false in all ToServerWebSocketClose messages and remove the now-unused _closeWithHibernate method.

---

Code Quality & Best Practices ✅

Positive:

• Clean removal of unused functionality - the _closeWithHibernate method is properly removed
• Simplified method signatures make the code easier to understand
• The #closeInner method now has a clearer, more straightforward signature
• All call sites consistently updated (tunnel.ts:516, 232, 246, 491)

Suggestions:

1. Documentation: Consider adding a comment explaining why hibernate is always false now, or link to relevant issue/decision. The comment at line 1070 ("We don't need to send a close response") is good but could be more specific about the protocol change.

2. Consistency: The warning message at tunnel.ts:400 was improved with better context ("tunnel data dropped"), which is great for debugging.

---

Potential Issues ⚠️

1. Protocol Compatibility: The most critical concern is whether the engine/server side expects the hibernate field in ToServerWebSocketClose. If this is a breaking change to the protocol, ensure:

   • The server-side code has been updated to handle hibernate: false or ignore this field
   • This is deployed in coordination with backend changes
   • Consider if this needs a protocol version bump

2. Missing Context: The PR title says "remove echo close" but the changes are about removing hibernate functionality. Are these related? If so, a brief explanation would help reviewers understand the connection.

3. Behavior Change: Previously, timeout scenarios (tunnel.ts:516) used _closeWithHibernate, but now use regular close(). This changes behavior from hibernate: true to hibernate: false. Is this intentional?

   • Before: webSocket._closeWithHibernate(1000, "Message acknowledgment timeout")
   • After: webSocket.close(1000, "ws.ack_timeout")
   • The reason text also changed - ensure this is intentional

---

Performance Considerations ✅

• Removing the hibernate parameter slightly reduces message payload size
• Simplified control flow should have negligible but positive performance impact
• No obvious performance concerns

---

Security Considerations ✅

• No security issues identified
• The close codes and reasons follow WebSocket standards (1000, 1011)
• Error messages are appropriately vague ("ws.ack_timeout") without leaking internal details

---

Test Coverage ⚠️

Concerns:

1. No test changes included in this PR
2. The existing test file (lifecycle.test.ts) should ideally be updated to verify:
   • WebSocket close operations work correctly without hibernate
   • The hibernate: false value is sent in all close scenarios
   • Timeout behavior works as expected with the new close method

Recommendation: Add or update tests to cover:

// Verify ToServerWebSocketClose messages always have hibernate: false
// Test timeout scenario sends proper close message
// Test normal close scenarios

---

Questions for Author

1. What is the relationship between "echo close" (PR title) and removing hibernate functionality?
2. Has the server-side protocol been updated to handle this change?
3. Was the behavior change in timeout scenarios (line 516) intentional?
4. Are there any edge cases where hibernate functionality was actually needed?

---

Recommendation

Conditional Approval - The code changes are clean and well-executed, but I'd like confirmation on:

1. Protocol compatibility with server-side changes
2. Intentional behavior change in timeout scenarios
3. Test coverage for the modified close flows

If these are addressed, this is a good simplification that removes complexity from the codebase.

---

Code-Specific Comments

tunnel.ts:328 - Good: Hard-coding hibernate: false is clear and explicit
tunnel.ts:516 - Changed: Verify timeout close behavior change is intentional
tunnel.ts:1070 - Good: Comment explaining no close response needed
websocket-tunnel-adapter.ts:232 - Simplified: _handleClose now cleaner with one less parameter
websocket-tunnel-adapter.ts:245 - Removed: _closeWithHibernate method properly cleaned up

[claude]

Code Review

I've reviewed this PR and found a critical issue that needs to be addressed before merging.

Critical Issue: Breaking WebSocket Hibernation Feature

The PR removes the hibernate parameter from WebSocket close handling, but this parameter is actively used by the engine's pegboard-gateway to manage WebSocket hibernation.

Evidence from engine/packages/pegboard-gateway/src/lib.rs:428:

if can_hibernate && close.hibernate {
    return Err(WebSocketServiceHibernate.build());
} else {
    // Successful closure
    return Ok(LifecycleResult::ServerClose(close));
}

Impact:

1. Breaking Change: The PR hardcodes hibernate: false in all ToServerWebSocketClose messages (tunnel.ts:328)
2. Lost Functionality: WebSockets that should hibernate will now be permanently closed instead
3. Protocol Mismatch: The protocol definition in engine/sdks/schemas/runner-protocol/v3.bare:268 still expects the hibernate field

Specific Issues:

1. tunnel.ts:328 - Hardcoding hibernate: false prevents WebSocket hibernation from ever triggering
2. tunnel.ts:516 - Changed from webSocket._closeWithHibernate(1000, "Message acknowledgment timeout") to webSocket.close(1000, "ws.ack_timeout") - this removes the ability to hibernate on timeout
3. websocket-tunnel-adapter.ts - Removed the _closeWithHibernate method entirely, eliminating hibernation support

Questions for Clarification

1. Intent: Is the goal to completely remove WebSocket hibernation support from the system? If so:

   • This needs to be documented as a breaking change
   • The engine code in pegboard-gateway/src/lib.rs needs updating
   • The protocol schema should be updated
   • Migration path for existing hibernatable WebSockets should be defined

2. Alternative: If hibernation should still work, the hibernate parameter needs to be preserved and passed correctly through the call chain

Recommendations

If removing hibernation is intentional:

• Update engine/packages/pegboard-gateway/src/lib.rs to remove hibernation logic
• Update protocol schema to remove the hibernate field
• Document this as a breaking change in the PR description
• Consider migration impact on running actors with hibernatable WebSockets

If hibernation should be preserved:

• Revert the removal of the hibernate parameter
• Investigate why the PR title mentions "remove echo close on ToClientWebSocketClose" when changes affect ToServerWebSocketClose
• Clarify what specific behavior this PR intends to fix

Minor Issues

1. tunnel.ts:400 - Good: Enhanced error message with "tunnel data dropped" for better debugging
2. tunnel.ts:1070 - Comment says "We don't need to send a close response" but doesn't explain why this changed

Code Quality

The code changes themselves are clean and well-structured, but the semantic implications for the WebSocket lifecycle need clarification before merging.

---

Verdict: ❌ Request Changes - Need clarification on hibernation removal intent and corresponding engine updates if this is intentional.

[claude]

Code Review

Summary

This PR removes the hibernate parameter from WebSocket close handling in the TypeScript runner. The change simplifies the close callback by always setting hibernate: false when sending ToServerWebSocketClose messages.

Code Quality ✅

Good:

• Clean removal of unused functionality - the _closeWithHibernate method is properly removed
• Consistent changes across both files (tunnel.ts and websocket-tunnel-adapter.ts)
• Simplifies the API by removing the hibernate parameter from callbacks
• The comment at tunnel.ts:1070 ("We don't need to send a close response") helps clarify intent

Concerns:

1. Protocol Mismatch ⚠️

   • The protocol definition in runner-protocol/src/index.ts:1183-1186 still expects a hibernate: boolean field
   • This change hardcodes hibernate: false (line 328 in tunnel.ts), but the protocol supports true/false
   • The engine code in pegboard-gateway/src/lib.rs:428-432 still checks if can_hibernate && close.hibernate to trigger hibernation
   • Impact: This effectively disables WebSocket hibernation from the client side. If this is intentional, it should be documented.

2. Incomplete Cleanup

   • The _closeWithHibernate method removal is good, but there's still a _closeWithoutCallback method
   • Consider if _closeWithoutCallback could also be simplified or if the naming convention should be reconsidered

3. Error Message Improvement ✅

   • The enhanced error message at line 400 in tunnel.ts ("tunnel data dropped.") is helpful
   • Good addition for debugging

Potential Bugs 🐛

1. WebSocket Hibernation Broken

   • If hibernation was previously working, this change breaks it entirely
   • The engine-side code (pegboard-gateway) will never see close.hibernate = true, so WebSocketServiceHibernate error will never be triggered
   • Question: Is this intentional? Should the protocol be updated or should this functionality be preserved?

2. Close Timeout Behavior Change

   • Line 516 in tunnel.ts changes from _closeWithHibernate to regular close for timeout scenarios
   • This changes the behavior when acknowledgment times out - previously might have hibernated, now always closes
   • This could affect running connections during temporary network issues

Security Concerns 🔒

No security issues identified. The changes reduce attack surface by removing functionality.

Performance Considerations ⚡

• Positive: Simpler code path may have marginal performance improvement
• Neutral: No significant performance impact expected

Test Coverage 🧪

Concerns:

• No test changes included in this PR
• No evidence that existing tests cover WebSocket hibernation scenarios
• Recommendation: Add tests to verify:
  1. WebSocket close without hibernation works correctly
  2. Timeout scenarios properly close connections
  3. Engine correctly handles hibernate: false in all cases

Recommendations

1. Clarify Intent: Add a comment or update the PR description explaining why hibernation is being removed
2. Protocol Cleanup: If hibernation is permanently removed, consider:
   • Updating the BARE protocol schema to remove the hibernate field
   • Removing hibernation logic from pegboard-gateway/src/lib.rs
   • Updating any documentation about WebSocket hibernation
3. Testing: Add integration tests covering the WebSocket close flow
4. Consider Logging: Add a debug log when sending ToServerWebSocketClose to help trace close events

Questions for Author

1. Is WebSocket hibernation functionality being deprecated entirely, or will it be re-implemented differently?
2. Are there any known scenarios where hibernate: true was being used that need alternative solutions?
3. Should the base branch name (11-13-chore_rivetkit_implement_new_hibernating_ws_protocol) be updated to reflect this change?

Verdict

Conditional Approval - The code changes are clean and well-executed, but need clarification on:

• Whether this intentionally disables hibernation
• Whether protocol/engine cleanup is needed
• Test coverage for the new behavior

If hibernation removal is intentional, consider this as part 1 of a larger refactor.

[claude]

Detailed Analysis:

Changes Made:

• Removed hibernate parameter from close callback (websocket-tunnel-adapter.ts:68)
• Hardcoded hibernate:false in ToServerWebSocketClose (tunnel.ts:328)
• Removed _closeWithHibernate() method
• Timeout handler now uses close() instead of _closeWithHibernate() (tunnel.ts:516)

Protocol Impact:
The ToServerWebSocketClose protocol still has a hibernate field (engine/sdks/typescript/runner-protocol/src/index.ts:1186), but this PR ensures it is always false. On the gateway side (engine/packages/pegboard-gateway/src/lib.rs:428), the condition "if can_hibernate && close.hibernate" will never trigger.

Behavior Changes:

• Message ack timeouts now fully close WebSockets instead of hibernating them
• Close reason changed from "Message acknowledgment timeout" to "ws.ack_timeout"
• Comment added at tunnel.ts:1070: "We do not need to send a close response"

Recommendations:

1. Document why hibernation is being removed in PR description
2. Update docs/engine/HIBERNATING_WS.md
3. Consider removing hibernate from protocol in follow-up PR
4. Verify no existing actors depend on hibernation
5. Add tests for new timeout behavior

[claude]

Positive Notes:

Code Quality: The implementation is clean with good code removal practices. The simplification of #closeInner() method signature makes the code easier to understand.

Improved Logging: The enhanced warning message at tunnel.ts:400 now explicitly states "tunnel data dropped" which will help with debugging.

No Security/Performance Issues: No security concerns or performance regressions identified.

The code itself is well-executed - the main question is whether the behavioral change (disabling hibernation) is intentional and properly coordinated with the rest of the system.

[claude]

PR Review: Remove echo close on ToClientWebSocketClose

Summary

This PR removes the hibernate parameter from WebSocket close callbacks and simplifies the close handling logic in the runner. The change aligns with the new hibernating WebSocket protocol where the engine/gateway automatically handles hibernation without requiring explicit signals from the runner.

---

Code Quality ✅

Positive aspects:

• Clean removal of the unused _closeWithHibernate method
• Simplified #closeInner method signature by removing the unused hibernate parameter
• Consistent changes across both files (tunnel.ts and websocket-tunnel-adapter.ts)
• Updated log message is more descriptive: "tunnel data dropped" provides better context

Alignment with architecture:
According to the HIBERNATABLE_CONNECTIONS.md documentation (Phase 3 in "Close Connection"), the design states: "Automatically hibernates WS, runner does not need to do anything." This PR correctly implements this design by removing the runner's responsibility to signal hibernation.

---

Correctness ✅

Protocol alignment:
Looking at v3.bare (lines 265-269), the ToServerWebSocketClose struct still has the hibernate: bool field:

type ToServerWebSocketClose struct {
    code: optional<u16>
    reason: optional<str>
    hibernate: bool
}

The PR correctly sets this to false in all cases (tunnel.ts:328), which is the right approach since:

1. The gateway now handles hibernation automatically based on can_hibernate from ToServerWebSocketOpen (pegboard-gateway/src/lib.rs:428-432)
2. The runner no longer needs to signal hibernation intent

Close handling flow:

• _handleClose now always calls #closeInner(code, reason, true) - callback is sent ✅
• close() calls #closeInner(code, reason, true) - callback is sent ✅
• _closeWithoutCallback calls #closeInner(code, reason, false) - no callback ✅
• The timeout case (tunnel.ts:516) correctly uses close() instead of the removed _closeWithHibernate() ✅

Important observation:
In #handleWebSocketClose (tunnel.ts:1070), the comment "We don't need to send a close response" is added. This is correct because:

• This handles ToClientWebSocketClose from the engine
• The _handleClose method calls #closeInner with callback=true
• But the closeCallback checks \!engineAlreadyClosed (tunnel.ts:322)
• Since this is coming from the engine, the callback won't actually send a response
• The comment accurately reflects this behavior

---

Potential Issues ⚠️

1. Message acknowledgment timeout behavior change
Location: tunnel.ts:516

Before:

webSocket._closeWithHibernate(1000, "Message acknowledgment timeout");

After:

webSocket.close(1000, "ws.ack_timeout");

Issue: This changes the timeout handling from a hibernating close to a normal close. The old behavior would have set hibernate: true in the ToServerWebSocketClose message.

Question: Is this intentional? Should acknowledgment timeouts trigger hibernation, or should they force-close the connection?

Based on the protocol design, it seems intentional since:

• Ack timeouts indicate a problematic connection
• Hibernation is for graceful pauses, not error conditions
• The reason changed from "Message acknowledgment timeout" to "ws.ack_timeout" (more concise)

Recommendation: ✅ This appears correct, but consider adding a comment explaining why ack timeouts don't hibernate.

2. Missing hibernation in error path
The timeout case is the only place where _closeWithHibernate was called. This suggests that previously, ack timeouts were the only case where the runner would request hibernation. With this removed, the runner never requests hibernation, relying entirely on the gateway to make that decision.

Verification needed: Confirm this aligns with the intended protocol behavior where the gateway owns hibernation decisions.

---

Performance ✅

No performance concerns. The changes reduce complexity without affecting performance characteristics.

---

Security ✅

No security concerns identified. The change doesn't introduce new attack surfaces.

---

Test Coverage ⚠️

Missing tests:

# No test files found for:
grep -r "ToServerWebSocketClose" **/*.test.ts
grep -r "_closeWithHibernate|closeCallback" **/*.test.ts

Recommendation: Consider adding integration tests for:

1. Normal WebSocket close flow (client-initiated)
2. Engine-initiated close (ToClientWebSocketClose)
3. Acknowledgment timeout close
4. Verify that hibernate: false is always sent in ToServerWebSocketClose
5. Verify that gateway correctly handles hibernation regardless of runner's hibernate flag

---

Minor Suggestions

1. Comment clarity (tunnel.ts:1070)
Current:

// We don't need to send a close response

Suggested:

// Engine initiated close - closeCallback will skip sending ToServerWebSocketClose due to engineAlreadyClosed

2. Protocol version consideration
The ToServerWebSocketClose.hibernate field still exists in the protocol. Consider:

• Adding a comment in the protocol schema noting it's deprecated/unused
• Or documenting that it's reserved for future use
• Or removing it in a future protocol version if truly unused

---

Conclusion

Overall assessment: ✅ Approve with minor suggestions

This is a well-executed cleanup that correctly implements the architectural design where the gateway owns hibernation decisions. The changes are consistent and align with the protocol documentation.

Action items:

1. ✅ Verify with team that ack timeouts should NOT trigger hibernation
2. 📝 Consider adding the suggested comment improvements
3. 🧪 Consider adding integration tests for WebSocket close flows
4. 📋 Document the hibernate field's status in the protocol schema

Great work on this cleanup! The code is clearer and better aligned with the architectural intent.

[claude]

PR Review: Remove echo close on ToClientWebSocketClose

Summary

This PR removes the hibernate functionality from the WebSocket close mechanism in the TypeScript runner by:

1. Hardcoding hibernate: false in ToServerWebSocketClose messages
2. Removing the _closeWithHibernate method from WebSocketTunnelAdapter
3. Simplifying the close callback signature and internal close logic
4. Improving a warning message for better diagnostics

Code Quality & Best Practices ✅

Positive observations:

• Clean removal of dead code path - the _closeWithHibernate method appears to have been unused
• Simplified function signatures improve code maintainability
• Consistent with the codebase's approach (comment at line 1070 confirms "We don't need to send a close response")
• The improved warning message (tunnel data dropped. added at line 400) provides better diagnostics

Minor concerns:

• The close callback type definition in the file header (lines 48-52) still shows the old signature with 3 parameters including hibernate?: boolean, but the implementation now only accepts 2 parameters. This creates a type mismatch between the field definition and actual usage.

Potential Issues 🔍

Type inconsistency:
At websocket-tunnel-adapter.ts:48-52, the #closeCallback field is typed as:

#closeCallback: (
    code?: number,
    reason?: string,
    hibernate?: boolean,
) => void;

But the constructor at line 68 and all call sites now use:

closeCallback: (code?: number, reason?: string) => void

Recommendation: Update the field type definition to match the new signature (remove hibernate?: boolean parameter).

Protocol Compliance ⚠️

Critical concern: The protocol definition at runner-protocol/src/index.ts:1183-1187 still requires the hibernate field in ToServerWebSocketClose:

export type ToServerWebSocketClose = {
    readonly code: u16 | null
    readonly reason: string | null
    readonly hibernate: boolean  // Still required in protocol
}

While you're hardcoding hibernate: false, this suggests one of two scenarios:

1. If hibernate is being removed entirely: The protocol definition should be updated and this needs coordination with the Rust side (see pegboard-gateway/src/lib.rs:428 which checks close.hibernate)
2. If this is a temporary change: Document why hibernate is being disabled

Based on the documentation (HIBERNATING_WS.md:7,12), hibernation is a core feature where actors can sleep and wake up on demand. Hardcoding hibernate: false means websockets will never hibernate, which could impact resource management for serverless actors.

Performance & Security ✅

Performance:

• Simplification reduces cognitive overhead
• No performance regression expected

Security:

• No security concerns identified
• Error handling remains appropriate

Test Coverage ❓

Observations:

• No test changes included in this PR
• Tests exist in tests/lifecycle.test.ts but I cannot verify if they cover WebSocket hibernation scenarios
• The timeout handling change (line 516: ws.ack_timeout instead of Message acknowledgment timeout) might need test verification

Recommendation: Verify that existing tests pass and consider adding/updating tests if hibernation behavior is being intentionally changed.

Action Items

1. Required: Fix type inconsistency for #closeCallback field definition (lines 48-52)
2. Required: Clarify the intent:
   • Is hibernation being permanently removed? If so, update protocol definitions and Rust code
   • Is this a temporary workaround? If so, add TODO comments and documentation
3. Recommended: Update or verify test coverage for WebSocket lifecycle, especially timeout scenarios
4. Recommended: Consider whether HIBERNATING_WS.md documentation needs updating

Verdict

The code changes themselves are clean and well-executed, but there's a protocol-level inconsistency that needs clarification before merging. The hardcoded hibernate: false appears to intentionally disable a documented feature, which may have broader implications for the actor lifecycle system.

Recommendation: Request clarification on the intended behavior before approving.

[claude]

Code Review

Summary

This PR removes the hibernate parameter from WebSocket close operations in the TypeScript runner SDK. The changes simplify the close callback signature and always set hibernate: false when sending close messages to the engine.

---

Code Quality ✅

Positive aspects:

• Simplification: Removing the unused hibernate parameter reduces complexity in the close callback chain
• Consistency: All close operations now consistently send hibernate: false to the server
• Clean refactoring: The changes properly propagate through the call stack:
  • WebSocketTunnelAdapter constructor signature updated
  • _closeWithHibernate method removed entirely
  • #closeInner simplified from 4 to 3 parameters
  • Timeout handler updated to use standard close() method

Minor improvements needed:

• Logging clarity (line 400): The updated log message "cannot send tunnel message, socket not connected to engine. tunnel data dropped." is more explicit, which is good. However, consider if this should be at error level instead of warn since data loss is occurring.

---

Potential Issues & Questions ⚠️

1. Breaking Protocol Change: The protocol definition at engine/sdks/typescript/runner-protocol/src/index.ts:1186 shows that ToServerWebSocketClose still requires a hibernate: boolean field. This PR hardcodes it to false, but:

   • Question: Is hibernation functionality being removed entirely from the system, or just from this client-side close path?
   • Question: Are there server-side components that might still initiate hibernation (e.g., from ToClientWebSocketClose)?
   • If hibernation is being deprecated, consider updating the protocol definition to make this field optional or remove it entirely in a future change.

2. Comment at line 1070: The comment "We don't need to send a close response" in #handleWebSocketClose is helpful but raises a question:

   • When the engine sends a ToClientWebSocketClose, the adapter handles it by calling _handleClose which calls #closeInner(code, reason, true) - the true means it will send a callback
   • But the comment says "we don't need to send a close response"
   • Clarification needed: Is the callback parameter in _handleClose actually preventing a response from being sent due to the engineAlreadyClosed guard in the close callback (line 322)? If so, this should be documented more clearly.

3. Behavioral Change in Timeout Handler (line 516):

   • Before: webSocket._closeWithHibernate(1000, "Message acknowledgment timeout")
   • After: webSocket.close(1000, "ws.ack_timeout")
   • This is a behavioral change - the timeout handler now sends a close message to the engine, whereas before it attempted to hibernate.
   • Question: Is this intentional? Could this cause issues if the engine is not responding (which is why the timeout occurred in the first place)?
   • Consider if _closeWithoutCallback might be more appropriate here to avoid sending a message that will likely timeout again.

---

Performance Considerations ✅

No performance concerns. The changes reduce the parameter count and remove an unused code path, which may have a minor positive impact.

---

Security Concerns ✅

No security issues identified. The changes maintain proper WebSocket lifecycle management.

---

Test Coverage ⚠️

Concerns:

• No test changes are included in this PR
• The PR modifies WebSocket close behavior, including:
  • Close callback signatures
  • Timeout handling behavior
  • Message sending logic

Recommendations:

1. Verify existing tests in engine/sdks/typescript/runner/tests/lifecycle.test.ts cover WebSocket close scenarios
2. Add specific test cases for:
   • Timeout-triggered closes (now uses different close method)
   • Engine-initiated closes (to verify no response is sent)
   • Client-initiated closes (to verify hibernate: false is sent)
3. If hibernation is being removed system-wide, ensure integration tests reflect this change

---

Recommendations

1. Clarify hibernation status: Add a comment or documentation explaining whether hibernation is being fully deprecated or just not supported in client-initiated closes
2. Review timeout behavior: Confirm that sending a close message during timeout is the desired behavior (vs. the previous hibernate attempt)
3. Consider engine guard: The close callback checks engineAlreadyClosed before sending - ensure this guard is properly set in all relevant code paths
4. Add tests: Include test coverage for the changed timeout handling behavior
5. Follow-up: If hibernation is being removed, create a follow-up task to update the protocol definition

---

Conclusion

This is a good cleanup PR that simplifies the codebase. The main concerns are:

1. Ensuring the timeout handling behavioral change is intentional
2. Clarifying the hibernation deprecation story
3. Adding test coverage for the changes

Recommendation: Approve with minor clarifications requested on the timeout behavior change.

[claude]

PR Review: Remove echo close on ToClientWebSocketClose

Summary

This PR removes the hibernate parameter functionality from WebSocket close handling in the TypeScript runner, always setting it to false. The change simplifies the close callback signature and removes the unused _closeWithHibernate method.

---

Code Quality & Best Practices ✅

Positive Changes:

• Simplified API: Removing the hibernate parameter from the close callback signature makes the code cleaner and easier to understand
• Removed dead code: The _closeWithHibernate method appears to have been unused
• Consistent behavior: All close operations now consistently send hibernate: false
• Good documentation: The comment at tunnel.ts:1070 clarifies intent ("We don't need to send a close response")

Minor Observations:

• The PR title says "remove echo close" but the actual change is about removing the hibernate parameter from close callbacks. The title could be more descriptive about what's actually changing.

---

Potential Issues & Questions ⚠️

1. Breaking Change Validation

The removal of the hibernate parameter from the close callback is a signature change. Have you verified that:

• No external code relies on the 3-parameter close callback signature?
• The Rust gateway code at engine/packages/pegboard-gateway/src/lib.rs:428 that checks close.hibernate will handle false correctly in all cases?

Impact: At line 428-429, the gateway checks:

if can_hibernate && close.hibernate {
    return Err(WebSocketServiceHibernate.build());
}

With hibernate always being false, hibernation will never be triggered from the runner side. Is this intentional?

2. Timeout Handling Change

In tunnel.ts:516, the change from _closeWithHibernate(1000, "Message acknowledgment timeout") to close(1000, "ws.ack_timeout") is significant:

Before: Used a special method that passed hibernate=true
After: Uses regular close() with hibernate=false

This means WebSocket ack timeouts will no longer trigger hibernation. Please confirm this behavioral change is intentional and won't break hibernatable WebSocket recovery scenarios.

3. Hardcoded Hibernate Value

At tunnel.ts:328, you're now hardcoding:

hibernate: false,

This removes any runtime flexibility. Consider if there are any scenarios where you'd want to hibernate on close in the future. If this is a permanent architectural decision, that's fine, but it should be documented in a comment explaining why hibernation on close is no longer supported.

---

Performance Considerations ✅

No performance concerns. The changes simplify the code path slightly by removing conditional logic.

---

Security Concerns ✅

No security issues identified. The change doesn't affect authentication, authorization, or data validation.

---

Test Coverage ❓

Question: Are there existing tests for:

1. WebSocket hibernation behavior?
2. WebSocket close handling?
3. Ack timeout scenarios?

I notice you have test files at tests/lifecycle.test.ts but didn't see test changes in this PR. If there are tests that specifically validate hibernation behavior on close, they may need to be updated or removed.

Recommendation: Run the test suite and verify that no tests are now failing or need updates to reflect the new behavior.

---

Architecture & Design 🤔

Question: Is Hibernation Being Deprecated?

This change suggests that WebSocket hibernation triggered by the runner is being removed or deprecated. The protocol still supports the hibernate field in ToServerWebSocketClose, but it's now always false.

Clarity needed:

• Is this part of a larger effort to change how WebSocket hibernation works?
• Should the protocol definition be updated to mark hibernate as deprecated?
• Are hibernatable WebSockets still supported, just triggered differently?

---

Recommendations

1. Add a comment at tunnel.ts:328 explaining why hibernate is always false:

   hibernate: false, // Hibernation is only triggered by gateway, not runner

2. Verify test coverage - Ensure no tests are broken and update any that specifically test hibernation-on-close behavior

3. Consider logging - The change at line 400 improves the warning message ("tunnel data dropped"), which is good. Consider if any other log messages should be added to help debug issues related to this behavioral change.

4. Documentation - If this changes user-facing behavior around WebSocket hibernation, update any relevant documentation.

---

Final Verdict

The code changes are clean and well-structured. However, this appears to be a behavioral change rather than just a refactor. The key question is whether always sending hibernate: false is the correct new behavior for all scenarios, particularly:

• WebSocket ack timeouts
• Normal WebSocket closes
• Error scenarios

Once confirmed that this behavioral change is intentional and won't break hibernatable WebSocket functionality, this PR looks good to merge. ✅

[claude]

Code Review

Summary

This PR simplifies WebSocket close handling by removing the hibernate parameter from the close callback chain and always setting it to false when sending close messages to the server.

Positive Aspects

1. Code Simplification: Reduces complexity by removing the hibernate parameter and eliminating the unused _closeWithHibernate method
2. Improved Clarity: Added helpful inline comment at tunnel.ts:1070
3. Better Logging: Enhanced warning message provides more context
4. Consistent Behavior: All ToServerWebSocketClose messages now consistently set hibernate: false

Code Quality Observations

Good:

• Maintains backward compatibility with the protocol
• Close logic is properly consolidated
• Focused change without scope creep

Concerns:

1. Message Acknowledgment Timeout Logic (tunnel.ts:516): Changed to use webSocket.close() which triggers the close callback. Is this intentional?

2. Protocol Field Still Required: The protocol still expects the hibernate boolean field. Consider updating the protocol in a follow-up PR.

3. Shutdown Behavior: Worth double-checking the shutdown logic does not conflict with broader hibernation changes.

Test Coverage

Concern: No test changes included

• Main test file is entirely commented out
• No tests validate hibernate is always false
• No tests verify timeout behavior changes

Recommendation: Add test coverage in a follow-up PR

Security and Performance

• No security concerns identified
• Negligible performance impact

Suggestions

1. Add inline documentation explaining why hibernate is always false now
2. File follow-up issue for protocol alignment
3. Add test coverage

Approval Status

The code changes are sound and safe to merge with these notes:

• Simplification is valuable and reduces cognitive overhead
• Behavior appears correct based on code inspection
• Would be strengthened by test coverage

Recommendation: Approve with suggestion to add tests in a follow-up PR.

[claude]

PR Review: Remove echo close on ToClientWebSocketClose

I've reviewed this PR and the changes look good overall. Here's my detailed feedback:

✅ Strengths

1. Simplification: The removal of the hibernate parameter from the close callback chain simplifies the WebSocket close logic and reduces complexity.

2. Consistent behavior: Setting hibernate: false consistently in the ToServerWebSocketClose message (tunnel.ts:328) makes the behavior more predictable.

3. Clean refactoring: The removal of _closeWithHibernate method reduces the API surface and eliminates an unused code path.

4. Good comment addition: The comment at tunnel.ts:1070 ("We don't need to send a close response") adds helpful context for future maintainers.

5. Improved error message: The enhanced warning message at tunnel.ts:400 ("tunnel data dropped") provides better debugging information.

🔍 Observations & Questions

1. Timeout behavior change: The change from _closeWithHibernate(1000, "Message acknowledgment timeout") to close(1000, "ws.ack_timeout") at tunnel.ts:516 changes the close behavior for message acknowledgment timeouts. Previously this would set hibernate=true, now it will set hibernate=false.

   • Question: Is this intentional? What's the impact of not hibernating on timeout? Could this affect error recovery or reconnection scenarios?

2. Close callback semantics: The callback parameter in #closeInner now controls whether to invoke the close callback to the engine. The PR removes the ability to distinguish between hibernatable and non-hibernatable closes.

   • Question: Are there any edge cases where we might want to hibernate a WebSocket connection? Or is hibernation fully deprecated?

3. Code cleanup at tunnel.ts:232: The change from this.#closeInner(code, reason, false, true) to this.#closeInner(code, reason, true) in _handleClose removes the hibernate parameter but now only has one boolean parameter (callback).

   • The refactoring looks correct based on the new signature.

🧪 Test Coverage

The test file at engine/sdks/typescript/runner/tests/lifecycle.test.ts is currently commented out.

Recommendation: Since this PR modifies critical WebSocket close behavior, it would be valuable to:

• Uncomment and run the lifecycle tests, especially the WebSocket close scenarios (lines 279-282, 377-394)
• Add a specific test for the timeout scenario to ensure the new close behavior works as expected
• Verify that WebSocket connections close properly when the engine sends ToClientWebSocketClose messages

🛡️ Security & Correctness

No security concerns identified. The changes don't introduce any obvious vulnerabilities.

One potential issue: The change ensures we always send hibernate: false in close messages. If the server-side code expects or handles hibernate: true in certain scenarios, this could cause issues. Please verify that the server/engine side properly handles hibernate: false for all close scenarios.

📝 Code Quality

• Code follows TypeScript conventions
• The refactoring maintains backward compatibility in terms of the public API
• Internal implementation is cleaner after removing the hibernate parameter threading

💡 Suggestions

1. Add a comment explaining why hibernate is always false now, especially in the close callback at tunnel.ts:328
2. Verify server-side behavior to ensure it handles non-hibernating closes appropriately in all cases
3. Consider re-enabling tests to validate the WebSocket close behavior changes

Summary

This is a clean refactoring that simplifies the WebSocket close logic. The main question is whether always setting hibernate: false is the correct behavior for all close scenarios, particularly timeouts. Once that's confirmed, this PR should be good to merge.

Recommendation: ✅ Approve pending verification of server-side hibernate handling

[graphite-app]

Merge activity

• Nov 21, 2:40 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Nov 21, 2:42 AM UTC: CI is running for this pull request on a draft pull request (#3515) due to your merge queue CI optimization settings.
• Nov 21, 2:43 AM UTC: Merged by the Graphite merge queue via draft PR: #3515.