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	Review	Updated (UTC)
rivet-cloud	Ignored		Dec 25, 2025 2:49am
rivet-inspector	Ignored	Preview	Dec 25, 2025 2:49am
rivet-site	Ignored	Preview	Dec 25, 2025 2:49am
rivetkit-serverless	Skipped		Dec 25, 2025 2:49am

[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

• actor-actions
• example-ai-agent
• example-ai-and-user-generated-actors-freestyle
• chat-room
• chat-room-next-js
• example-cloudflare-workers
• example-cloudflare-workers-hono
• example-cloudflare-workers-inline-client
• cross-actor-actions
• example-cursors
• example-cursors-raw-websocket
• custom-serverless
• example-deno
• example-sqlite
• example-elysia
• example-experimental-durable-streams-ai-agent
• example-express
• example-hono
• hono-bun
• hono-react
• example-kitchen-sink
• multi-region
• native-websockets
• next-js
• example-node
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• scheduling
• state
• 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: 98195c7

[claude]

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

Overview

This PR adds comprehensive test coverage for the Key-Value (KV) store functionality in the Rivet Actors system. The tests are well-structured and cover CRUD operations, batch operations, drop functionality, and various edge cases.

✅ Strengths

Test Organization

• Excellent separation of concerns with dedicated test files:
  • actors_kv_crud.rs - Basic CRUD operations
  • actors_kv_drop.rs - Drop/clear operations
  • actors_kv_list.rs - List query operations
  • actors_kv_misc.rs - Edge cases and validation
• Clear use of MARK comments to organize code sections
• Consistent naming conventions for test actors and test functions

Test Coverage

• Comprehensive coverage of KV operations:
  • Basic CRUD (Create, Read, Update, Delete)
  • Batch operations (multiple keys at once)
  • Drop functionality (clearing all data)
  • List queries (all, prefix, range)
  • Edge cases (empty keys/values, large data, special characters)
• Good use of helper functions (make_key, make_value) for consistency

Code Quality

• Proper error handling with context messages
• Good use of structured logging with tracing
• Clear actor behavior separation using the TestActor trait
• Consistent use of oneshot channels for test result notification

🔍 Issues & Suggestions

1. Typo in Error Message (Minor)

Location: actors_kv_crud.rs:69

.context("expected value to exist,  got null")?;

• Double space before "got" - should be single space

2. TODO Comments Indicate Engine Behavior Issues (Documentation)

Locations:

• actors_kv_crud.rs:130-132
• actors_kv_crud.rs:676-678
• actors_kv_drop.rs:79-81

The tests contain TODO comments indicating that the engine returns empty arrays instead of arrays with null values for nonexistent keys:

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

Recommendation:

• These TODOs should be tracked as issues if this is unintended behavior
• If this is the intended API design, the comments should be updated to reflect that
• Consider adding explicit test assertions for this behavior rather than just accommodating it

3. Inconsistent Error Handling Patterns (Minor)

Some actors use this pattern:

let result = async { /* ... */ }.await;
let test_result = match result {
    Result::Ok(r) => r,
    Result::Err(e) => KvTestResult::Failure(e.to_string()),
};

While others use simpler patterns. Consider standardizing on one approach.

4. Missing Anyhow Import Consistency (Style)

Location: All new test files

Per CLAUDE.md:

Do not glob import (::*) from anyhow. Instead, import individual types and traits

Current usage (use anyhow::*;) is against the style guide. Should be:

use anyhow::{Context, Result, anyhow, bail};

5. Hard-Coded Magic Numbers (Maintainability)

Examples:

• actors_kv_crud.rs:606 - batch size of 10
• actors_kv_crud.rs:691 - batch size of 5
• actors_kv_misc.rs - size limit of 1MB

Recommendation: Define constants at the top of the file:

const BATCH_SIZE_SMALL: usize = 5;
const BATCH_SIZE_MEDIUM: usize = 10;
const KV_VALUE_SIZE_LIMIT: usize = 1024 * 1024; // 1MB

6. Test Infrastructure Enhancements (Enhancement)

The test infrastructure in test_runner/actor.rs and test_runner/runner.rs is well-designed with:

• KV request/response channels
• Lifecycle event broadcasting
• Proper WebSocket message handling

However, consider adding:

• Timeout handling for KV requests to prevent hanging tests
• Metrics/counters for KV operations (useful for debugging)

7. Potential Race Condition (Low Risk)

Location: test_runner/actor.rs:91-111

The send_kv_get method sends a request and waits for response without timeout:

let response: rp::KvResponseData = response_rx
    .await
    .map_err(|_| anyhow\!("KV get request response channel closed"))?;

Recommendation: Add timeout wrapper:

let response = tokio::time::timeout(
    Duration::from_secs(30),
    response_rx
)
.await
.map_err(|_| anyhow\!("KV request timed out after 30s"))??;

8. Logging Message Capitalization (Style)

Location: Multiple files

Per CLAUDE.md:

Log messages should be lowercase unless mentioning specific code symbols

Most messages follow this correctly, but verify consistency throughout.

9. Missing Test: Concurrent Operations (Coverage)

The tests cover sequential operations well, but don't test concurrent KV operations from the same actor or multiple actors. Consider adding tests for:

• Concurrent reads/writes to the same key
• Concurrent batch operations
• Race conditions between put/delete operations

10. Test Helpers Location (Organization)

The test_helpers.rs file has a new create_actor helper function that's very useful. Consider if other repetitive patterns in the test files could be extracted to helpers.

🔒 Security Considerations

✅ No obvious security issues. The tests properly validate:

• Empty input handling
• Large data handling (with size limits)
• Special characters and Unicode
• Key/value validation

⚡ Performance Considerations

✅ Tests are appropriately scoped:

• Use reasonable batch sizes
• Test data size limits (1MB)
• Proper cleanup with drop operations

Note: The 1MB test in actors_kv_misc.rs creates large test data - ensure test environments have sufficient memory.

📊 Test Execution

Recommendation: Per CLAUDE.md testing guidelines:

When running tests, always pipe the test to a file in /tmp/ then grep it in a second step

Example:

cargo test actors_kv > /tmp/kv_tests.log 2>&1
grep -i "error\|panic\|fail" /tmp/kv_tests.log

Summary

This is a solid addition to the test suite with comprehensive coverage of KV functionality. The main areas for improvement are:

1. High Priority:

   • Fix anyhow import style (CLAUDE.md violation)
   • Add timeout handling to KV requests
   • Address or document the TODO comments about engine behavior

2. Medium Priority:

   • Extract magic numbers to constants
   • Add tests for concurrent operations
   • Standardize error handling patterns

3. Low Priority:

   • Fix typo in error message
   • Verify logging message capitalization

The test infrastructure additions (KV request channels, response handling) are well-designed and follow good practices. Overall, this PR significantly improves test coverage for the KV subsystem.

Recommended Action

Approve with minor changes requested - The issues identified are mostly style/consistency improvements rather than functional problems. The test coverage and organization are excellent.

[claude]

PR Review: KV Test Suite for Actor Runner

This PR adds comprehensive test coverage for the key-value (KV) store functionality in the Rivet Actors test runner. Overall, this is excellent test coverage with well-structured, thorough tests. Below are my findings organized by category.

---

Code Quality & Best Practices

✅ Strengths

1. Excellent test organization: Tests are logically grouped into separate files by category (CRUD, list, drop, misc)
2. Comprehensive coverage: Tests cover happy paths, edge cases, error conditions, and boundary conditions
3. Clear naming: Actor names and test function names clearly describe what's being tested
4. Good use of helper functions: make_key() and make_value() provide consistent conversion
5. Documentation: Each actor struct has a clear doc comment explaining its purpose

⚠️ Issues Found

1. Typo in error message - actors_kv_crud.rs:69

.context("expected value to exist,  got null")?;  // Extra space before 'got'

Should be: "expected value to exist, got null"

2. Inconsistent error handling pattern

Some tests use:

let result = match result {
    Result::Ok(r) => r,
    Result::Err(e) => KvTestResult::Failure(e.to_string()),
};

While the cleaner pattern used elsewhere is:

let test_result = match result {
    Ok(r) => r,
    Err(e) => KvTestResult::Failure(e.to_string()),
};

The Result:: prefix is redundant since Ok and Err are in the prelude. This appears in most test actors and should be consistently cleaned up.

3. Missing structured logging in some places

Per CLAUDE.md, parameters should not be formatted into messages. Found several instances that need fixing:

// actors_kv_crud.rs (and repeated in other files)
bail!("expected 1 value, got {}", response.values.len());

Should use structured logging style consistently:

let len = response.values.len();
bail!("expected 1 value, got {len}");  // Or use tracing before bail

However, since these are inside bail! macros (not tracing calls), this may be acceptable. The main concern is with the tracing::info! and tracing::error! calls which seem to follow the guidelines correctly.

---

Potential Bugs

⚠️ Known Engine Behavior Issues

Several tests document unexpected engine behavior with TODO comments:

1. Nonexistent key behavior - actors_kv_crud.rs:131-134

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

Impact: This inconsistency could cause confusion for users expecting standard KV semantics where getting a nonexistent key returns null/None. The test works around this but highlights a potential API design issue.

2. Batch operations with nonexistent keys - actors_kv_crud.rs:801-804

Similar issue in batch delete verification.

Recommendation: File issues to track these engine behaviors and ensure they're intentional design decisions rather than bugs.

---

Performance Considerations

⚠️ Potential Issues

1. Sequential operations in loops - Multiple places like actors_kv_crud.rs:653-674:

for i in 0..10 {
    let key = make_key(&format!("batch-key-{}", i));
    let expected_value = make_value(&format!("batch-value-{}", i));
    
    let response = config
        .send_kv_get(vec![key.clone()])
        .await  // Sequential await in loop
        .context(format!("failed to get key {}", i))?;
    // ...
}

Impact: This is acceptable for tests but demonstrates a pattern that should be avoided in production code. The test is intentionally verifying individual gets after a batch put, which is fine for validation purposes.

2. String formatting in loops - actors_kv_list.rs and elsewhere:
   Multiple calls to format!("key-{}", i) in tight loops. For tests, this is fine, but worth noting.

✅ Good Performance Patterns

• Tests use batch operations (send_kv_put with multiple keys) where appropriate
• Channel-based communication is non-blocking
• Proper use of async/await

---

Security Concerns

✅ No Major Issues Found

1. Binary data handling looks correct: Tests in actors_kv_misc.rs verify null bytes and non-UTF8 data are handled properly
2. No injection vulnerabilities: All data is passed as Vec, not constructed via string interpolation
3. Proper error handling: Errors are propagated correctly without exposing sensitive internals

💡 Observations

• Tests with very large keys (10KB, 1MB) are good for catching buffer overflow or allocation issues
• Empty key and value tests help ensure edge cases don't cause crashes

---

Test Coverage

✅ Excellent Coverage

CRUD Operations (actors_kv_crud.rs):

• ✅ Basic put/get
• ✅ Get nonexistent key
• ✅ Overwrite existing key
• ✅ Delete existing key
• ✅ Delete nonexistent key
• ✅ Batch put (10 keys)
• ✅ Batch get (5 keys)
• ✅ Batch delete (5 keys)

List Operations (actors_kv_list.rs):

• ✅ List all on empty store
• ✅ List all with keys
• ✅ List with prefix matching
• ✅ List with start/end range
• ✅ Reverse listing
• ✅ Limit functionality
• ✅ Pagination scenarios

Drop Operations (actors_kv_drop.rs):

• ✅ Drop clears all data
• ✅ Operations work after drop
• ✅ Multiple drop calls

Edge Cases (actors_kv_misc.rs):

• ✅ Binary data (null bytes, non-UTF8)
• ✅ Empty keys and values
• ✅ Large keys (10KB, 1MB)
• ✅ Large values (10KB, 1MB)
• ✅ Unicode/special characters

💡 Additional Test Suggestions

1. Concurrent operations: Tests with multiple actors operating on overlapping keys simultaneously
2. Transaction semantics: If supported, test atomicity of batch operations
3. Resource limits: Test behavior when hitting storage limits or max key count
4. Error recovery: Test behavior after partial failures in batch operations

---

Architecture & Design

✅ Excellent Patterns

1. Clean separation of concerns: Test actors are well-isolated, each testing one specific behavior
2. Reusable test infrastructure: The TestRunner and ActorConfig abstractions are well-designed
3. Channel-based communication: Proper use of oneshot channels for request/response
4. Consistent error handling: Using KvTestResult enum for standardized result reporting

💡 Suggestions

1. Extract common actor patterns: Many actors follow the same structure:

async fn on_start(&mut self, config: ActorConfig) -> Result<ActorStartResult> {
    let result = async {
        // Test logic here
    }.await;
    
    let test_result = match result {
        Ok(r) => r,
        Err(e) => KvTestResult::Failure(e.to_string()),
    };
    
    if let Some(tx) = self.notify_tx.lock().unwrap().take() {
        let _ = tx.send(test_result);
    }
    
    Ok(ActorStartResult::Running)
}

Consider creating a helper macro or function to reduce boilerplate.

2. Type alias for notify channel: The type Arc<Mutex<Option<tokio::sync::oneshot::Sender<KvTestResult>>>> appears repeatedly. Consider:

type NotifyTx = Arc<Mutex<Option<tokio::sync::oneshot::Sender<KvTestResult>>>>;

---

Specific File Reviews

runner.rs Changes (lines 67-70, 155-219)

✅ Good additions:

• KV request channel infrastructure properly integrated
• send_kv_request and handle_kv_response methods follow existing patterns
• Request ID tracking prevents response mix-ups

⚠️ Minor concern:

• Line 621: tracing::info!("lifecycle_tx start"); - This log message is not very descriptive. Consider: tracing::info!("broadcasting actor start lifecycle event");

actor.rs Changes (lines 90-219)

✅ Excellent implementation:

• All KV operations properly typed and documented
• Consistent error handling across all methods
• Good use of oneshot channels for request/response pattern
• Proper error messages in all error paths

---

Summary

This is a high-quality test suite with comprehensive coverage of KV functionality. The code is well-structured, follows Rust best practices, and provides good test infrastructure for future work.

Required Fixes (blocking)

1. Fix typo at actors_kv_crud.rs:69 (double space)
2. Remove Result:: prefix throughout (use Ok/Err directly)
3. Improve log message at runner.rs:621

Recommended Improvements (non-blocking)

1. Consider extracting common actor patterns to reduce boilerplate
2. Add type alias for the repeated notify channel type
3. File issues to track the documented engine behavior inconsistencies (TODO comments)
4. Consider adding concurrent operation tests in a follow-up PR

Architecture Score: ⭐⭐⭐⭐⭐ (5/5)

Code Quality Score: ⭐⭐⭐⭐ (4/5) - Minor cleanup needed

Test Coverage Score: ⭐⭐⭐⭐⭐ (5/5)

Overall: Approve with minor changes requested

Great work on this comprehensive test suite! 🎉

[graphite-app]

Merge activity

• Dec 25, 3:10 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Dec 25, 3:10 AM UTC: CI is running for this pull request on a draft pull request (#3676) due to your merge queue CI optimization settings.
• Dec 25, 3:11 AM UTC: Merged by the Graphite merge queue via draft PR: #3676.