Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

Copy link
Member Author

NathanFlurry commented Jan 19, 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


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.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 19, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3976

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: bab56c9

@claude
Copy link

claude bot commented Jan 19, 2026

PR Review - KV API Implementation

This PR introduces a user-facing KV (key-value) storage API for Rivet Actors. The implementation is generally well-structured with good TypeScript typing and proper separation of concerns. Below are my findings:

✅ Strengths

  1. Good separation of concerns: The refactoring of kv.tskeys.ts and creating a dedicated KV API class is clean
  2. Strong type safety: Excellent use of TypeScript generics and mapped types for type-safe encoding/decoding
  3. Lazy initialization: The ActorContext.kv property uses lazy initialization appropriately
  4. Comprehensive test coverage: Tests cover both text and arrayBuffer encoding/decoding
  5. Batch operations: The API properly exposes batch operations for efficiency
  6. Documentation: Good JSDoc comments on public methods

🐛 Issues Found

1. Formatting Issue in Test File (rivetkit-typescript/packages/rivetkit/src/driver-test-suite/tests/actor-kv.ts:26-42)

The second test has incorrect indentation - the opening brace is on the wrong line:

test(
    "supports arrayBuffer encoding and decoding",
    async (c: TestContext) => {
    // ^^ This should be on the previous line

Should be:

test("supports arrayBuffer encoding and decoding", async (c: TestContext) => {

2. Missing Error Handling in encodeKey (rivetkit-typescript/packages/rivetkit/src/actor/instance/kv.ts:36-48)

When keyType is explicitly provided but does not match the key type, the error message could be more helpful:

function encodeKey<K extends KvKeyType = KvKeyType>(
    key: KvKeyTypeMap[K],
    keyType?: K,
): Uint8Array {
    if (key instanceof Uint8Array) {
        return key;
    }
    const resolvedKeyType = keyType ?? "text";
    if (resolvedKeyType === "binary") {
        throw new TypeError("Expected a Uint8Array when keyType is binary");
    }
    return textEncoder.encode(key);
}

The logic assumes that if the key is not a Uint8Array and keyType is "binary", it should throw. However, if someone passes a string key with no keyType specified, it defaults to "text" and works fine. But if they explicitly pass keyType: "binary" with a string key, they get the error. This is correct but the early return for Uint8Array means that even if keyType: "text" is passed with a Uint8Array, it will still return the Uint8Array without validating the keyType matches.

Consider adding validation when keyType is explicitly provided to ensure it matches the actual key type.

3. Potential Type Safety Gap in encodeValue (rivetkit-typescript/packages/rivetkit/src/actor/instance/kv.ts:80-106)

The encodeValue function uses type casting that could allow runtime type mismatches:

const type =
    options?.type ??
    resolveValueType(value as string | Uint8Array | ArrayBuffer);

The cast to string | Uint8Array | ArrayBuffer could hide type errors. If the generic T does not match the actual value type, this could cause issues.

⚠️ Potential Concerns

4. Memory Allocation in decodeValue for arrayBuffer (rivetkit-typescript/packages/rivetkit/src/actor/instance/kv.ts:116-120)

When returning an ArrayBuffer, the code creates a copy:

case "arrayBuffer": {
    const copy = new Uint8Array(value.byteLength);
    copy.set(value);
    return copy.buffer as KvValueTypeMap[T];
}

This is likely intentional to prevent mutations to the underlying storage, but it doubles memory usage for large values. Consider adding a comment explaining this is intentional for safety.

5. Missing Validation in list() Return Type (rivetkit-typescript/packages/rivetkit/src/actor/instance/kv.ts:225-240)

The list() method does not validate that all returned keys actually start with the requested prefix after removing the internal KV prefix. While the driver should guarantee this, defensive validation could prevent subtle bugs.

6. No Transaction Support

The batch operations (putBatch, getBatch, deleteBatch) do not appear to be atomic transactions. Users might expect that putBatch either succeeds completely or fails completely, but partial failures could occur. This should be documented or the implementation should guarantee atomicity.

💡 Suggestions

  1. Add integration tests: The current tests only cover basic operations. Consider adding tests for:

    • Empty prefix listing
    • Non-existent key retrieval
    • Batch operations with mixed success/failure scenarios
    • Binary key type usage
    • Edge cases (empty strings, very large values)
  2. Document the KV prefix isolation: Add documentation explaining that user KV data is isolated via the KEYS.KV prefix and will not conflict with internal storage

  3. Consider adding a has(key) method: For checking key existence without fetching the value

  4. Add examples in JSDoc: The public methods would benefit from usage examples

  5. CLAUDE.md update looks good: The addition to the testing guidelines is helpful

🔒 Security

No security concerns identified. The prefix-based isolation properly separates user KV data from internal state.

📊 Performance

The implementation efficiently delegates to batch operations even for single operations, which should perform well. Memory copying in decodeValue for arrayBuffer is the only potential concern for very large values.

Summary

This is a solid implementation of a KV API with good TypeScript practices. The main issues are a formatting problem in the test file and some opportunities to improve error messages and type safety validation. The functionality appears correct and well-tested for the basic use cases.

Recommendation: Approve with minor fixes for the formatting issue and consideration of the type safety suggestions.

import type { AnyDatabaseProvider, InferDatabaseClient } from "../../database";
import type { ActorDefinition, AnyActorDefinition } from "../../definition";
import type { ActorInstance, SaveStateOptions } from "../../instance/mod";
import { ActorKv } from "../../instance/kv";
Copy link
Contributor

Choose a reason for hiding this comment

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

The import for ActorKv is inserted in the middle of other imports, which violates import sorting rules. It should be moved to maintain alphabetical order with the other imports.

Spotted by Graphite Agent (based on CI logs)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@NathanFlurry NathanFlurry force-pushed the 01-19-feat_rivetkit_expose_getgatewayurl branch from ed37d98 to 825c812 Compare January 20, 2026 19:48
Comment on lines +1 to +44
import type { DriverTestConfig } from "../mod";
import { setupDriverTest } from "../utils";
import { describe, expect, test, type TestContext } from "vitest";

export function runActorKvTests(driverTestConfig: DriverTestConfig) {
describe("Actor KV Tests", () => {
test("supports text encoding and decoding", async (c: TestContext) => {
const { client } = await setupDriverTest(c, driverTestConfig);
const kvHandle = client.kvActor.getOrCreate(["kv-text"]);

await kvHandle.putText("greeting", "hello");
const value = await kvHandle.getText("greeting");
expect(value).toBe("hello");

await kvHandle.putText("prefix-a", "alpha");
await kvHandle.putText("prefix-b", "beta");

const results = await kvHandle.listText("prefix-");
const sorted = results.sort((a, b) => a.key.localeCompare(b.key));
expect(sorted).toEqual([
{ key: "prefix-a", value: "alpha" },
{ key: "prefix-b", value: "beta" },
]);
});

test(
"supports arrayBuffer encoding and decoding",
async (c: TestContext) => {
const { client } = await setupDriverTest(c, driverTestConfig);
const kvHandle = client.kvActor.getOrCreate(["kv-array-buffer"]);

const values = await kvHandle.roundtripArrayBuffer("bytes", [
4,
8,
15,
16,
23,
42,
]);
expect(values).toEqual([4, 8, 15, 16, 23, 42]);
},
);
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This file needs to be formatted with Biome. Run biome format --write . to fix formatting issues and biome lint --apply . to fix any linting issues

Spotted by Graphite Agent (based on CI logs)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 21, 2026

Merge activity

  • Jan 21, 12:11 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Jan 21, 12:11 AM UTC: CI is running for this pull request on a draft pull request (#3988) due to your merge queue CI optimization settings.
  • Jan 21, 12:12 AM UTC: Merged by the Graphite merge queue via draft PR: #3988.

graphite-app bot pushed a commit that referenced this pull request Jan 21, 2026
@graphite-app graphite-app bot closed this Jan 21, 2026
@graphite-app graphite-app bot deleted the 01-16-feat_kv_api branch January 21, 2026 00:12
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