Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Nov 17, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Nov 17, 2025 7:36pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 17, 2025 7:36pm
rivet-inspector Ignored Ignored Preview Nov 17, 2025 7:36pm
rivet-site Ignored Ignored Preview Nov 17, 2025 7:36pm

Copy link
Contributor Author

MasterPtato commented Nov 17, 2025

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
Copy link

claude bot commented Nov 17, 2025

Code Review

Summary

This PR performs a straightforward renaming of the retry field to hibernate in the runner protocol's ToServerWebSocketClose message. The change improves semantic clarity by better describing the field's purpose: indicating whether a WebSocket connection should enter a hibernation state rather than fully close.

Positive Aspects ✅

  1. Semantic Clarity: The rename from retry to hibernate is a good improvement. The new name more accurately describes the behavior - the connection is entering a hibernation state that can be resumed, rather than a simple retry.

  2. Comprehensive Coverage: The change is applied consistently across:

    • Protocol schema (v3.bare)
    • Rust protocol implementation
    • TypeScript protocol bindings
    • All consuming code in gateway and runner packages
    • Protocol version conversion functions (v2 ↔ v3)
  3. Backward Compatibility: The protocol version conversion functions correctly maintain backward compatibility by mapping close.retry (v2) ↔ close.hibernate (v3) in both directions at versioned.rs:1288 and versioned.rs:1337.

  4. Logical Improvements: The code reorganization in pegboard-gateway/src/lib.rs:404-409 improves readability by moving the "Successful closure" comment to the else branch where it actually applies.

Issues Found 🔍

1. Unrelated Import Reordering (Minor)

File: engine/packages/api-peer/src/actors/kv_get.rs:1-3

The import order change appears unrelated to the PR's purpose:

-use base64::prelude::BASE64_STANDARD;
 use base64::Engine;
+use base64::prelude::BASE64_STANDARD;

Recommendation: This formatting change should either be:

  • Reverted (preferred, to keep the PR focused)
  • Or explained if it's intentional to match the project's import ordering standards

2. Unrelated Formatting Changes (Minor)

File: engine/packages/api-public/src/actors/kv_get.rs:68-72

The multiline formatting of the format! call appears unrelated to the rename:

-"/actors/{}/kv/keys/{}", path.actor_id, urlencoding::encode(&path.key)
+&format!(
+    "/actors/{}/kv/keys/{}",
+    path.actor_id,
+    urlencoding::encode(&path.key)
+)

Recommendation: Consider reverting this change to keep the PR focused on the retry → hibernate rename.

3. Assert Function and Whitespace Changes (Minor)

File: engine/sdks/typescript/runner-protocol/src/index.ts

Multiple unrelated changes:

  • Moving import assert from "node:assert" to the top (line 1)
  • Removing the custom assert function at the bottom (lines 1930-1933)
  • Whitespace changes in callbacks (() => {}() => { })
  • Indentation changes (lines 611, 621)

Recommendation: While these are generally good cleanups:

  • The assert import move is good (following best practices)
  • Removing the duplicate assert function is correct (using Node's built-in instead)
  • However, the whitespace changes in empty functions and indentation adjustments should ideally be in a separate formatting PR

Per CLAUDE.md: "Do not run ./scripts/cargo/fix.sh. Do not format the code yourself." These formatting changes may have been unintentional.

Recommendations 📝

  1. Consider reverting unrelated changes to keep the PR focused solely on the retry → hibernate rename. This makes code review easier and git history cleaner.

  2. Test Coverage: While the change is straightforward, consider verifying that:

    • WebSocket hibernation behavior still works correctly
    • Protocol version conversion maintains backward compatibility
    • No tests were broken by the rename
  3. Documentation: If there's any user-facing documentation that mentions the "retry" field, it should be updated to reference "hibernate" instead.

Security & Performance ✅

  • No security concerns identified
  • No performance impact (purely a naming change)
  • Backward compatibility properly maintained through version conversion

Overall Assessment

This is a clean refactoring PR that improves code clarity. The core change (rename) is well-executed and comprehensive. The only concerns are minor unrelated formatting changes that could be cleaned up to keep the PR focused.

Recommendation: Approve with minor cleanup suggestions.

@MasterPtato MasterPtato force-pushed the 11-13-fix_udb_update_backoff_algo branch from 9b619f4 to 267ddc0 Compare November 17, 2025 19:35
@MasterPtato MasterPtato force-pushed the 11-13-chore_rename_retry_-_hibernate_in_runner_protocol branch from 7ec4550 to bbe84e7 Compare November 17, 2025 19:35
@claude
Copy link

claude bot commented Nov 17, 2025

Code Review

Thank you for this PR! The rename from retry to hibernate better reflects the semantic meaning of the field. Here is my review:

✅ Strengths

  1. Clear intent: The rename from retry to hibernate is semantically more accurate for describing WebSocket connection hibernation behavior
  2. Comprehensive coverage: The change is applied consistently across:
    • Protocol schema (v3.bare)
    • Rust implementations (pegboard-gateway, pegboard-runner)
    • TypeScript SDK (runner-protocol, runner, websocket-tunnel-adapter)
    • Version conversion logic (v2 ↔ v3 conversions)
  3. Backward compatibility: The versioned.rs file correctly maintains the field mapping between v2 (retry) and v3 (hibernate) for protocol version compatibility

🐛 Issues Found

Critical: Missing update in stringify.ts

The file engine/sdks/typescript/runner/src/stringify.ts still references retry instead of hibernate:

// Line 61-64
const { code, reason, retry } = kind.val;
const codeStr = code === null ? "null" : code.toString();
const reasonStr = reason === null ? "null" : `"${reason}"`;
return `ToServerWebSocketClose{code: ${codeStr}, reason: ${reasonStr}, retry: ${retry}}`;

This should be updated to:

const { code, reason, hibernate } = kind.val;
// ...
return `ToServerWebSocketClose{code: ${codeStr}, reason: ${reasonStr}, hibernate: ${hibernate}}`;

Location: engine/sdks/typescript/runner/src/stringify.ts:61-64

📝 Minor Observations

  1. Unrelated formatting changes: The PR includes some minor formatting changes:

    • Import reordering in api-peer/src/actors/kv_get.rs
    • Formatting in api-public/src/actors/kv_get.rs
    • Whitespace changes in TypeScript files
    • Import statement consolidation in runner-protocol/src/index.ts

    While these are generally fine, it is worth noting they are technically outside the scope of the rename. Per the CLAUDE.md guidelines, cargo fmt should not be run automatically, so these changes appear to be manual adjustments.

  2. Comment improvement: The pegboard-gateway comment placement is better after the change - "Successful closure" is now in the else branch where it belongs (line pegboard-gateway/src/lib.rs:407).

✅ Security & Performance

  • No security concerns identified
  • No performance impact expected
  • The change is purely semantic and maintains the same behavior

🧪 Testing Recommendation

Given that this is a protocol-level change:

  1. Verify integration tests pass, especially those involving WebSocket hibernation
  2. Test protocol version compatibility (v2 ↔ v3 conversions)
  3. Ensure the stringify function works correctly after fixing the bug

Summary

This is a solid refactoring that improves code clarity. The main action item is to fix the missing rename in stringify.ts. Once that is addressed, this should be ready to merge!

@claude
Copy link

claude bot commented Nov 17, 2025

Code Review

This PR renames the retry field to hibernate in the runner protocol's ToServerWebSocketClose message. This is a valuable clarification that better reflects the actual semantics of the field - it controls whether a WebSocket should hibernate (as per HIBERNATING_WS.md) rather than retry.

Positives

  1. Better Naming: The rename from retry to hibernate accurately reflects the behavior documented in docs/engine/HIBERNATING_WS.md, where step 7 explicitly mentions "the runner will close the websocket by sending ToServerWebSocketClose with .hibernate = true"

  2. Comprehensive Coverage: The change properly updates:

    • Schema definition (v3.bare)
    • TypeScript bindings (runner-protocol/src/index.ts)
    • Rust implementations (pegboard-gateway, pegboard-runner)
    • Version conversion functions (v2 to v3)
    • WebSocket tunnel adapter
    • All usages in the tunnel implementation
  3. Correct Version Handling: The versioned.rs file correctly maps between v2's retry and v3's hibernate in both directions, maintaining backward compatibility

  4. Code Quality: The formatting changes (import ordering in kv_get.rs, spacing fixes) follow Rust conventions

Issues Found

  1. Missing Update in stringify.ts (Bug)

    The file engine/sdks/typescript/runner/src/stringify.ts:61-64 still references the old retry field. This should be updated to use hibernate instead. This will cause a TypeScript compilation error since the field was renamed in the type definition.

  2. Outdated Comment (Minor)

    engine/sdks/typescript/runner/src/tunnel.ts:80 has a comment that says "The WebSocket close event with retry is automatically sent when the". Consider updating "with retry" to "with hibernate" for consistency.

  3. Inconsistent Method Name (Semantic)

    The method __closeWithRetry() in websocket-tunnel-adapter.ts:207 should arguably be renamed to __closeWithHibernate() for consistency. Currently the method is called __closeWithRetry but it passes hibernate: true to closeInner and the protocol field is now hibernate. While this works functionally, the naming is now misleading.

  4. No Test Coverage (Gap)

    There don't appear to be tests verifying the hibernate field behavior. Consider adding tests to ensure:

    • Hibernatable websockets close with hibernate: true
    • Non-hibernatable websockets close with hibernate: false
    • The version conversion between v2 and v3 works correctly
  5. Generated TypeDoc Not Updated

    The file website/public/typedoc shows retry instead of hibernate. The TypeDoc likely needs to be regenerated.

Security

No security concerns identified. This is a straightforward rename that doesn't change the protocol's security properties.

Performance

No performance impact - this is a pure rename.

Recommendations

  1. Must Fix: Update stringify.ts to use hibernate instead of retry (lines 61, 64)
  2. Should Fix: Consider renaming __closeWithRetry() to __closeWithHibernate() for semantic consistency
  3. Nice to Have: Update the comment on line 80 of tunnel.ts
  4. Nice to Have: Add test coverage for hibernate behavior
  5. Nice to Have: Regenerate TypeDoc documentation

Summary

This is a good refactoring that improves code clarity. However, there's at least one file (stringify.ts) that was missed and will cause a compilation error. The PR should not be merged until that's fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants