Skip to content

fix(gateway): send actor key components as repeated rvt-key-part params - #5808

Open
Abhikansh3 wants to merge 3 commits into
rivet-dev:mainfrom
Abhikansh3:fix/gateway-actor-key-comma-collision
Open

Abhikansh3 wants to merge 3 commits into
rivet-dev:mainfrom
Abhikansh3:fix/gateway-actor-key-comma-collision

Conversation

@Abhikansh3

@Abhikansh3 Abhikansh3 commented Sep 29, 2026 •

Copy link
Copy Markdown

Description

Fixes #5807. Query-based gateway routing (getForKey/getOrCreateForKey, used by client.foo.get([...]) / .getOrCreate([...])) serialized a compound actor key by joining its components with a plain comma into the rvt-key query parameter, with no escaping. Because of that, two logically distinct keys serialized identically and collided:

  • ["tenant,admin"] (one component containing a literal comma)
  • ["tenant", "admin"] (two separate components)

Both produced rvt-key=tenant%2Cadmin, and the guard decoded both back to ["tenant", "admin"], silently routing requests to the wrong actor.

  • Both the TypeScript and Rust RivetKit clients now send one rvt-key-part query parameter per key component instead of a single comma-joined rvt-key value. Each occurrence is independently percent-encoded, so no escaping scheme is needed and a component containing a comma is expressed exactly as given.
  • The guard now accepts repeated rvt-key-part occurrences (the only rvt-* param allowed to repeat) and uses them directly as the key array when present.
  • The legacy comma-joined rvt-key param is kept and still decoded exactly as before, so older/pinned clients are unaffected. If both are present, rvt-key-part takes precedence.
  • Updated the two hand-written API reference pages (actor-keys.mdx, actor-routing.mdx) that previously documented the comma-in-a-key-part case as "cannot be expressed" — in reality it silently collided rather than erroring.
  • Corrected a stale note in rivetkit-typescript/CLAUDE.md that prescribed the comma scheme as intended design, and removed a couple of references to a TS-side query-gateway parser that was deleted during the Rust cutover (parsing now lives solely in the Rust guard).

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?

  • cargo test -p rivet-guard --test parse_actor_path: 38/38 pass, including 4 new tests covering the collision directly (e.g. rvt-key-part=tenant%2Cadmin vs rvt-key-part=tenant&rvt-key-part=admin now decode to different arrays) plus all pre-existing legacy comma-format tests unchanged.
  • cargo test -p rivetkit-client --lib remote_manager: 3/3 new tests pass, same collision check from the Rust client's URL-building side.
  • pnpm exec vitest run tests/actor-gateway-url.test.ts (rivetkit-typescript): 14/14 pass, including the issue's own suggested regression set (["a,b"] vs ["a","b"], [""] vs [], ["a,","b"] vs ["a","","b"], [","] vs ["",""]).
  • End-to-end: built a local rivet-engine binary and ran tests/driver/gateway-routing.test.ts against it. The new end-to-end test (distinguishes a comma-containing component from a two-component key, creating two actors via the real SDK and asserting they resolve to different actor IDs) passes on every native-runtime variant (local/remote sqlite × bare/cbor/json encoding, 6/6). The wasm runtime variant of this suite currently fails in my environment due to a pre-existing missing rivetkit-wasm build artifact unrelated to this change.
  • cargo clippy on both touched crates: no new warnings.
  • cargo test -p rivetkit-client --test bare: 21/21 pass, including the pre-existing integration tests that assert on gateway query parameters, updated to check rvt-key-part instead of the now-removed rvt-key.

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

Update

Addressed the two review findings from the automated review:

  • rivetkit-rust/packages/client/tests/bare.rs had pre-existing integration tests asserting on the now-removed rvt-key parameter with a HashMap-based query helper that couldn't represent repeated parameters. Updated those tests and added a helper that collects repeated rvt-key-part occurrences.
  • rivetkit-openapi/openapi.json and docs/api/endpoints.json now define rvt-key-part alongside rvt-key, and the generated gateway/inspector reference pages were regenerated with pnpm docs:gen-api so the new parameter is discoverable in the public API reference.

Copilot AI balanced review requested due to automatic review settings September 29, 2026 19:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 2 medium-severity findings

Reviewed commit a790ba5.

Comment on lines +666 to +667
for part in key {
push_query_param(&mut params, "rvt-key-part", part);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Medium · The Rust client test suite still requires the removed parameter

The existing packages/client/tests/bare.rs coverage still asserts rvt-key for both gateway URL builders and the raw HTTP/WebSocket handlers. This builder now emits only rvt-key-part, so those tests deterministically fail (and their HashMap query helpers cannot represent repeated parts). Update those assertions and query helpers to collect repeated values, as the new inline tests do, so cargo test -p rivetkit-client covers and accepts the new wire format.

| `rvt-namespace` | Yes | Namespace the actor lives in. |
| `rvt-method` | Yes | `get` to fail if the actor does not exist, or `getOrCreate` to create it on first use. |
| `rvt-key` | No | Actor key. Separate multi-part keys with commas: `rvt-key=org-1,room-2`. Omit for a keyless actor. |
| `rvt-key-part` | No | One key part. Repeat for a multi-part key, in order: `rvt-key-part=org-1&rvt-key-part=room-2`. Omit entirely for a keyless actor. |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Medium · The canonical API reference omits the new query parameter

The generated gateway reference still comes from rivetkit-openapi/openapi.json plus docs/api/endpoints.json, and both sources still define only rvt-key; consequently every generated gateway/inspector page continues to expose the comma-separated form and generated clients/spec consumers cannot discover rvt-key-part. Add the repeated array-style query parameter and its shared description/examples to those source files, then regenerate the API reference as required by docs/CLAUDE.md.

@Abhikansh3

Copy link
Copy Markdown
Author

Ready for review. Covered with unit/integration tests in the guard, both RivetKit clients, and an end-to-end driver test against a locally built rivet-engine (details in the description's test plan). Two things flagged for maintainers: the wasm driver-test variant didn't run in my environment due to an unrelated missing build artifact, and I couldn't locate the generator for rivetkit-openapi/openapi.json to update its example blocks — happy to follow up on either if pointed in the right direction.

@Abhikansh3
Abhikansh3 force-pushed the fix/gateway-actor-key-comma-collision branch from a790ba5 to dd17113 Compare September 29, 2026 21:17

This branch has not been deployed

No deployments
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.

bug(gateway): comma-containing actor key components collide during query routing

2 participants