chore(engine): test actors runner kv

[abcxff]

No description provided.

[vercel]

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

4 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Dec 11, 2025 1:09am
rivet-inspector	Ignored			Dec 11, 2025 1:09am
rivet-site	Ignored			Dec 11, 2025 1:09am
rivetkit-serverless	Skipped			Dec 11, 2025 1:09am

[abcxff]

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

• chore(engine): test actors runner kv #3625 👈 (View in Graphite)
• chore(engine): test actor alarms #3624
• feat(engine): actors lifecycle testing #3623
• chore: setup rivet-engine api tests #3540
• main

---

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]

More templates

• example-ai-agent
• example-ai-and-user-generated-actors-freestyle
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• example-cloudflare-workers-inline-client
• example-counter
• example-counter-next-js
• example-counter-serverless
• example-cursors
• example-cursors-raw-websocket
• example-deno
• example-sqlite
• example-elysia
• example-express
• example-hono
• example-hono-bun
• example-hono-react
• example-kitchen-sink
• example-next-js
• quickstart-actions
• quickstart-cross-actor-actions
• quickstart-multi-region
• quickstart-raw-websockets
• quickstart-realtime
• quickstart-scheduling
• quickstart-state
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• starter
• example-stream
• example-trpc

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 8ce0a8c

[claude]

PR Review: chore(engine): test actors runner kv

Overview

This PR adds comprehensive KV (key-value) storage testing for the Rivet Actors test runner framework. It introduces 4 new test files with extensive coverage of KV operations including CRUD, listing, dropping, and edge cases.

Code Quality ✅

Strengths:

• Well-organized test structure with clear MARK comments separating sections
• Comprehensive test coverage across multiple scenarios
• Good use of helper functions (make_key, make_value) to reduce duplication
• Proper error handling with context messages
• Detailed logging with structured tracing (following CLAUDE.md conventions)

Areas for Improvement:

1. Typo in Comment (actors_kv_crud.rs:70)

   .context("expected value to exist,  got null")?;
   //                                 ^^ extra space

2. Import Organization ✅

   • Imports are correctly placed at the top of files
   • Following CLAUDE.md guidance: "Always add imports at the top of the file"

3. Logging Patterns ✅

   • Uses structured logging correctly: tracing::info!(?actor_id, "message")
   • Lowercase messages following CLAUDE.md: "log messages should be lowercase"

Best Practices ✅

1. Error Handling

   • Correctly uses anyhow::* without glob importing the entire module ✅
   • Uses Result<T> return types appropriately
   • Good use of .context() for error messages

2. Async Patterns

   • Proper use of async_trait for trait implementations
   • Correct async/await usage throughout

3. Test Structure

   • Each test follows the pattern: setup → execute → verify
   • Good use of oneshot channels for test synchronization

Potential Issues 🔍

1. Ignored Tests (actors_kv_misc.rs)

   • Three tests are marked #[ignore]: kv_empty_value, kv_large_value, kv_many_keys_storage
   • Question: Are these tests failing or just slow? Should they be enabled?
   • Consider documenting why these are ignored

2. TODO Comments (actors_kv_crud.rs:134-136, 845-847)

   // TODO: Engine returns empty arrays for nonexistent keys instead of array with null
   // Should return: keys: [key], values: [None]
   // Currently returns: keys: [], values: []

   • This indicates a potential API design question
   • The test adapts to current behavior but documents expected behavior
   • Recommendation: Create an issue to track this API decision

3. Empty Value Handling (actors_kv_misc.rs:106-143)

   • Test expects empty values to fail, which is good defensive testing
   • Consider if this is documented behavior or implementation detail

Performance Considerations ⚡

1. Large Value Test (actors_kv_misc.rs:194-273)

   • Tests 1MB value storage
   • This is ignored, likely due to performance impact
   • Good to have for comprehensive testing

2. Many Keys Test (actors_kv_misc.rs:512-604)

   • Tests 1000 keys
   • Also ignored, probably for CI performance
   • Consider running these in nightly or stress-test CI jobs

Security Concerns 🔒

No security issues identified. The tests:

• Don't handle user input directly
• Use proper error handling
• Don't expose sensitive data

Test Coverage ✅

Excellent coverage across:

• ✅ Basic CRUD operations (put, get, delete)
• ✅ Batch operations
• ✅ List operations (all, range, prefix)
• ✅ Drop operations
• ✅ Edge cases (empty keys, binary data, ordering)
• ✅ Error cases (nonexistent keys, empty values)

Changes to Test Infrastructure

actor.rs (lines 90-220):

• Adds KV request methods to ActorConfig
• Clean implementation with proper async/await
• Good error handling with meaningful messages

runner.rs (lines 66-70, 387-402, 838-879):

• Adds KV request channel infrastructure
• Handles KV requests in message loop
• Proper request/response tracking with unique IDs
• Good concurrency handling with Arc<Mutex<>>

Recommendations

1. Document Ignored Tests

   • Add comments explaining why tests are ignored
   • Consider adding them to a "slow tests" test suite

2. Track API Decisions

   • Create issue for nonexistent key behavior (empty array vs null values)
   • Document expected behavior in code comments

3. Minor Cleanup

   • Fix double space typo on line 70 of actors_kv_crud.rs

4. Consider Adding

   • Concurrent access tests (multiple actors accessing same keys)
   • TTL/expiration tests if supported
   • Transaction tests if supported

Conclusion

This is a high-quality PR that significantly improves test coverage for the KV storage system. The code follows Rivet's conventions well, has comprehensive test coverage, and good error handling. The main items to address are documenting why certain tests are ignored and tracking the API design question around nonexistent key behavior.

Verdict: ✅ Approve with minor suggestions