Skip to content

feat(rivetkit): add ConnectionMap readonly Map wrapper for actor connections#5021

Open
abcxff wants to merge 1 commit into
05-10-fix_rivetkit-core_chunk_apply_batch_puts_deletes_at_128from
05-11-feat_rivetkit_add_connectionmap_readonly_map_wrapper_for_actor_connections
Open

feat(rivetkit): add ConnectionMap readonly Map wrapper for actor connections#5021
abcxff wants to merge 1 commit into
05-10-fix_rivetkit-core_chunk_apply_batch_puts_deletes_at_128from
05-11-feat_rivetkit_add_connectionmap_readonly_map_wrapper_for_actor_connections

Conversation

@abcxff
Copy link
Copy Markdown
Contributor

@abcxff abcxff commented May 11, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@railway-app
Copy link
Copy Markdown

railway-app Bot commented May 11, 2026

🚅 Deployed to the rivet-pr-5021 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink ❌ Build Failed (View Logs) Web May 11, 2026 at 11:40 pm
website 😴 Sleeping (View Logs) Web May 11, 2026 at 2:33 pm
frontend-inspector ❌ Build Failed (View Logs) Web May 11, 2026 at 2:24 pm
mcp-hub ✅ Success (View Logs) Web May 11, 2026 at 2:23 pm
frontend-cloud ❌ Build Failed (View Logs) Web May 11, 2026 at 2:23 pm
ladle ❌ Build Failed (View Logs) Web May 11, 2026 at 2:22 pm

Copy link
Copy Markdown
Contributor Author

abcxff commented May 11, 2026

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

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

@abcxff abcxff force-pushed the 05-10-fix_rivetkit-core_chunk_apply_batch_puts_deletes_at_128 branch from 3d96914 to ccf09c6 Compare May 11, 2026 22:17
@abcxff abcxff force-pushed the 05-11-feat_rivetkit_add_connectionmap_readonly_map_wrapper_for_actor_connections branch from ef2c9d3 to d69dfaa Compare May 11, 2026 22:17
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 11, 2026

Code Review: ConnectionMap readonly Map wrapper for actor connections

Overview

This PR introduces a ConnectionMap class that lazily wraps the native actor connections behind a ReadonlyMap interface, replacing the previous eager new Map(...) snapshot in ActorContextHandleAdapter.conns.


Bug: Duplicate Import in context.rs

The diff introduces a duplicate import that will fail to compile:

// rivetkit-rust/packages/rivetkit-core/src/actor/context.rs
use rivet_error::ActorSpecifier;  // line 13 (existing)
use rivet_error::ActorSpecifier;  // line 16 (added — duplicate)

This is likely an accidental diff artifact. It needs to be removed before this can merge.


Performance: N+1 Native Calls Per Operation

The previous implementation called actorConns once and built an eager Map. The new ConnectionMap calls actorConns independently on every method — size, get, has, keys, values, entries, and forEach each separately invoke callNativeSync(() => this.#runtime.actorConns(this.#ctx)).

For the two existing call sites that do Array.from(actorCtx.conns.values()), this is equivalent (one actorConns call), but callers that chain operations like map.has(id) && map.get(id) will now cross the NAPI boundary twice. Consider documenting that each method independently queries the runtime, or materializing the ConnHandle[] once at construction time if the live-view property is not essential.


Type Safety: as MapIterator<T> Casts

The iterator methods cast Array iterators to MapIterator<T>:

return conns.map((c) => this.#runtime.connId(c))[Symbol.iterator]() as MapIterator<string>;

MapIterator is a branded type in TypeScript's lib that includes ES2025 iterator helper methods (.map(), .filter(), .drop(), etc.) which plain Array iterators lack. This cast is unsound — if downstream code calls any of those helpers, it will fail at runtime with no compile-time error. Return IterableIterator<T> instead (which satisfies the ReadonlyMap contract without the false capability claim), or wrap with a generator that provides a proper iterator.


Identity: New Adapter Instance Per Call

Every call to get(), values(), entries(), and forEach() creates a fresh NativeConnAdapter via #connToAdapter. Two calls to map.get(sameId) return different objects (!==). Code relying on referential equality (e.g. caching adapters in a WeakMap, comparing with ===) will break silently. This is a meaningful behavioral difference from a plain Map. A per-ConnectionMap adapter cache keyed by connection id would fix this.


Missing: Types Not in Public Exports

ConnectionMap is exported from connection-map.ts and NativeConnAdapter is now exported from native.ts, but neither appears in src/registry/index.ts. Users whose TypeScript code references c.conns will be unable to annotate the element type without an import from internal paths. Both should be exported from the package surface if conns is user-facing.


Minor: callNativeSync Now Exported

callNativeSync was previously package-private. Exporting it is necessary for connection-map.ts but broadens the surface area. Fine for now since it stays in the same directory, but worth ensuring it does not become a general escape hatch.


Summary

Issue Severity
Duplicate use rivet_error::ActorSpecifier import (compile error) Blocker
as MapIterator<T> unsound cast High
N+1 actorConns calls per chained operation Medium
New adapter object identity per get/values call Medium
ConnectionMap/NativeConnAdapter missing from public exports Medium
callNativeSync now exported Low

The concept is sound — a live ReadonlyMap view over connections is cleaner than an eagerly-snapshotted Map. The duplicate import must be fixed before this can land, and the MapIterator cast and adapter identity issues need addressing.

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 11, 2026

Review of feat(rivetkit): add ConnectionMap readonly Map wrapper for actor connections. This PR replaces the eager Map built in the conns getter with a lazy NativeConnectionMap that implements ReadonlyMap. Issues found: (1) Performance - every Map method calls actorConns() independently through NAPI, so composite patterns like has+get pay 2x NAPI trips and 2x O(n) scans instead of 1 trip + O(1) lookup. (2) Snapshot inconsistency - map.size and map.entries() can disagree if connections change between calls. (3) Unnecessary exports of callNativeSync and NativeConnAdapter with no current consumers. (4) Return type leaks the concrete NativeConnectionMap class; it should be ReadonlyMap. (5) Unsound MapIterator casts - Array iterators are not MapIterators. Suggestion: add a private snapshot() helper that builds a real Map once per call - fixes O(n) regression, eliminates snapshot inconsistency, removes unsound casts. Also narrow the conns return type to ReadonlyMap, remove unnecessary exports, and add tests. Minor: the ReadonlyMap constraint is a good improvement over the prior mutable Map return type.

@abcxff abcxff force-pushed the 05-11-feat_rivetkit_add_connectionmap_readonly_map_wrapper_for_actor_connections branch from d69dfaa to 5014be8 Compare May 11, 2026 22:27
@abcxff abcxff force-pushed the 05-10-fix_rivetkit-core_chunk_apply_batch_puts_deletes_at_128 branch from ccf09c6 to f268b35 Compare May 11, 2026 23:28
@abcxff abcxff force-pushed the 05-11-feat_rivetkit_add_connectionmap_readonly_map_wrapper_for_actor_connections branch from 5014be8 to 6eedbc8 Compare May 11, 2026 23:28
@abcxff abcxff force-pushed the 05-11-feat_rivetkit_add_connectionmap_readonly_map_wrapper_for_actor_connections branch from 6eedbc8 to 8e1538c Compare May 11, 2026 23:40
@abcxff abcxff marked this pull request as ready for review May 12, 2026 12: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.

1 participant