Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 9, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivet-site Ready Ready Preview Comment Nov 9, 2025 3:11am
rivetkit-serverless Ready Ready Preview Comment Nov 9, 2025 3:11am
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 9, 2025 3:11am
rivet-inspector Ignored Ignored Preview Nov 9, 2025 3:11am

Copy link
Member Author

NathanFlurry commented Nov 9, 2025


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 9, 2025

PR Review: Remove deprecated conn tokens & SSE support

Summary

This PR removes deprecated connection token authentication and SSE (Server-Sent Events) support from RivetKit, streamlining the connection protocol. The changes include protocol versioning updates and a migration path for persisted actor state.

Code Quality ✅

Strengths:

  • Well-structured migrations: The schema versioning system properly handles backward compatibility with migrations from v1→v2→v3
  • Clean refactoring: Consistent removal of deprecated code paths across the codebase (removed ~1200 lines)
  • Protocol evolution: Clear separation between v1 and v2 client protocols with proper migration logic

Areas for attention:

  1. Migration logic complexity (rivetkit-typescript/packages/rivetkit/src/schemas/actor-persist/versioned.ts:29-81):

    • The v2→v3 migration merges connections and hibernatableWebSocket arrays by matching hibernatableRequestId
    • Uses Buffer.from().equals() for comparing ArrayBuffers
    • Potential issue: If a connection has a hibernatableRequestId but no matching WebSocket in the array, it's silently dropped. Consider adding a warning log for debugging.
  2. Bidirectional conversion mismatch (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:2097-2148):

    • #convertToBarePersisted and #convertFromBarePersisted handle the connection/hibernatable split differently
    • In #convertFromBarePersisted, lastSeen is hardcoded to 0 which could cause issues with connection liveness checks
    • Recommendation: Set lastSeen from lastSeenTimestamp for consistency

Potential Bugs 🐛

  1. Lost subscription data (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:2101-2131):

    • The new v3 schema doesn't persist connection subscriptions
    • Old code stored: subscriptions: conn.subscriptions.map((sub) => ({ eventName: sub.eventName }))
    • New code omits subscriptions entirely from hibernatableConns
    • Impact: After actor hibernation/wake, connections will lose their event subscriptions
    • Severity: HIGH - This will break functionality for hibernatable WebSockets that use subscriptions
  2. WebSocket URL path change (examples/cursors-raw-websocket/src/frontend/App.tsx:93):

    • Changed from /raw/websocket to /websocket
    • Verify: Ensure backend routing matches this change (constants updated in actor-router-consts.ts)
    • ✅ Confirmed: PATH_WEBSOCKET_PREFIX changed accordingly
  3. Typo in comment (rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:373):

    • // TODO: Remvoe any, Hono is being a dumbass → Should be "Remove"
    • Minor issue but should be fixed for professionalism

Security Concerns 🔒

  1. Connection token removal:

    • POSITIVE: Removing connection tokens (generateConnToken, IncorrectConnToken error) simplifies the authentication model
    • The PR properly removes token validation from the WebSocket reconnection path
    • Connection ID is now the sole identifier (UUID-based)
  2. Removed liveness checks (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1305-1367):

    • Old code: Periodic interval to check if connections are still alive and remove dead ones
    • New code: Removes the entire #checkConnectionsLiveness mechanism
    • Concern: Without liveness checks, zombie connections could accumulate
    • Mitigation needed: Ensure hibernatable WebSocket cleanup still works correctly

Performance Considerations ⚡

  1. Reduced overhead: Removing SSE driver and connection token validation reduces per-connection overhead
  2. Migration cost: The v2→v3 migration involves array iteration and buffer comparisons - should be acceptable for typical actor sizes
  3. Sleep timer logic (rivetkit-typescript/packages/rivetkit/src/actor/instance.ts:1911-1916):
    • Current logic always returns CanSleep.ActiveConns when ANY connection exists
    • TODO comment suggests this blocks hibernation entirely
    • Recommendation: Address this TODO to properly enable hibernation for actors with only hibernatable connections

Test Coverage 🧪

Changes to test files:

  • Updated driver tests to remove connId and connToken parameters
  • Modified WebSocket URL paths in tests
  • Removed deprecated protocol tests

Recommendations:

  1. Add migration tests for v2→v3 schema conversion, especially edge cases:
    • Connection with hibernatableRequestId but missing WebSocket
    • Multiple connections with same request ID
  2. Test subscription persistence after hibernation/wake cycle
  3. Verify backward compatibility with clients still using v1 protocol

Breaking Changes 🔴

This PR introduces breaking changes:

  1. Connection tokens are no longer supported (clients using them will fail)
  2. SSE connections are removed entirely
  3. WebSocket URL paths changed from /raw/websocket to /websocket
  4. Init message no longer includes connectionToken field

Migration path: The protocol versioning should handle older clients, but verify all client SDKs are updated.

Action Items 📋

Critical:

  • Fix subscription loss in v3 schema - add subscriptions to PersistedHibernatableConn
  • Set lastSeen correctly in #convertFromBarePersisted

Recommended:

  • Add logging when connections are dropped during v2→v3 migration
  • Address hibernation TODO to prevent blocking sleep with hibernatable connections
  • Fix typo: "Remvoe" → "Remove"
  • Add tests for schema migration edge cases
  • Update client SDKs and documentation for breaking changes

Overall Assessment

This is a well-intentioned cleanup that simplifies the codebase, but has one critical bug (subscription loss) that needs to be fixed before merging. The schema versioning and migration approach is solid, but needs additional test coverage.

Recommendation: Request changes to fix the subscription persistence issue, then approve after verification.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 9, 2025

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: c0334fa

@NathanFlurry NathanFlurry marked this pull request as ready for review November 9, 2025 05:44
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