feat(api-public): expose actor kv api #3410

NathanFlurry · 2025-11-07T22:02:05Z

No description provided.

vercel · 2025-11-07T22:02:10Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 13, 2025 8:55pm

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 13, 2025 8:55pm
rivet-inspector	Ignored	Preview	Nov 13, 2025 8:55pm
rivet-site	Ignored	Preview	Nov 13, 2025 8:55pm

NathanFlurry · 2025-11-07T22:02:23Z

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.

claude · 2025-11-07T22:03:50Z

PR Review: Expose Actor KV API

I've reviewed this PR that exposes the actor key-value API through both the public and peer APIs. Here's my feedback:

✅ Strengths

Consistent Architecture: The implementation follows the established pattern used by other actor endpoints (create, delete, list), properly routing between local and remote datacenters.
Proper Error Handling: New custom error Actor::KvKeyNotFound follows the project's error conventions using the RivetError derive macro.
Security: Authentication is properly enforced via ctx.auth().await? in the public API.
Base64 Encoding: Values are correctly encoded/decoded in base64 for safe transport over HTTP.
OpenAPI Documentation: Proper schema definitions and endpoint documentation are included.

🔍 Issues & Concerns

1. URL Encoding Vulnerability (Critical)

// api-public/src/actors/kv_get.rs:68
&format\!("/actors/{}/kv/keys/{}", path.actor_id, path.key)

Problem: The key parameter is already base64-encoded but is being directly interpolated into the URL without URL encoding. If the base64 string contains characters like /, +, or =, this will break the URL routing.

Fix: URL-encode the key parameter:

&format\!("/actors/{}/kv/keys/{}", path.actor_id, urlencoding::encode(&path.key))

2. Inconsistent Field Naming (Medium)

// api-peer/src/actors/kv_get.rs:63
update_ts: metadata[0].create_ts,

Problem: The response field is named update_ts but is populated from create_ts. According to the actor-kv implementation (lib.rs:257-260), KV entries only store create_ts, which is set during put operations (even when updating existing keys).

Options:

Rename the field to create_ts to match what it actually represents
Or document that this represents the last update time (since put operations overwrite)
Per CLAUDE.md naming conventions: timestamps should use past tense verbs (e.g., created_at, updated_at)

Recommendation: Rename to created_at or updated_at (if this represents update time):

pub struct KvGetResponse {
    pub value: String,
    pub updated_at: i64,  // or created_at
}

3. Missing Structured Logging (Medium)

The code lacks logging statements. Per CLAUDE.md guidelines:

// Add to api-peer/src/actors/kv_get.rs after line 42
tracing::info\!(?path.actor_id, key_len=?path.key.len(), "fetching kv entry");

// Add after line 54
tracing::warn\!(?path.actor_id, ?key_bytes, "kv key not found");

Note: Use structured logging with ? prefix, lowercase messages unless referencing code symbols.

4. Authorization Gaps (High)

The public API (api-public) has authentication but no authorization checks. Compare with actors/delete.rs:54-63 which verifies namespace ownership:

// api-peer should verify actor access
let actors_res = ctx
    .op(pegboard::ops::actor::get::Input {
        actor_ids: vec\![path.actor_id],
    })
    .await?;

let actor = actors_res
    .actors
    .into_iter()
    .next()
    .ok_or_else(|| pegboard::errors::Actor::NotFound.build())?;

// Optional: verify namespace if provided via query param

Risk: Any authenticated user could read KV data from any actor, regardless of namespace ownership.

5. Missing Input Validation

No validation that the key path parameter is valid base64 before forwarding to remote datacenter (line 68)
The peer API decodes base64 (line 44-46) but doesn't validate the decoded key length against MAX_KEY_SIZE (2KB from actor-kv)

6. Performance: Unnecessary Clone (Low)

// api-peer/src/actors/kv_get.rs:51
actor_kv::get(&*udb, path.actor_id, vec\![key_bytes.clone()]).await?;

The .clone() is unnecessary since key_bytes is only used once after this line (for the error check). Can be:

actor_kv::get(&*udb, path.actor_id, vec\![key_bytes]).await?;

📝 Additional Recommendations

Add Tests: No tests were added. Consider adding:
- Test for successful KV get
- Test for non-existent key (404 handling)
- Test for invalid base64 key
- Test for cross-datacenter routing
API Consistency: Consider if this should support batch gets (the underlying actor_kv::get accepts Vec<KvKey>). Current implementation only returns a single value.
Documentation: Add a comment explaining the base64 encoding requirement for the key path parameter in the OpenAPI spec.
Error Messages: The KvKeyNotFound error message could be more descriptive (include the actor_id for debugging).

Summary

This PR has good structure and follows most project conventions, but has critical security and correctness issues that should be addressed before merging:

Must Fix:

URL encoding for remote datacenter routing
Authorization checks (namespace verification)
Field naming inconsistency (update_ts vs create_ts)

Should Fix:

Add structured logging
Input validation
Remove unnecessary clone

Nice to Have:

Test coverage
Better error messages
Documentation improvements

pkg-pr-new · 2025-11-07T22:04:21Z

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: a9a9e7c

claude · 2025-11-08T22:27:49Z

PR Review: feat(api-public): expose actor kv api

Thank you for this PR! Overall, the implementation looks solid and follows the codebase conventions well. Here are my findings:

✅ Strengths

Good consistency: The API follows established patterns from other actor endpoints (delete, list, etc.)
Proper datacenter routing: Correctly handles routing to remote datacenters when needed
Error handling: Proper use of custom error types with the new KvKeyNotFound error
Base64 encoding: Appropriate for binary-safe KV values in JSON API responses
Documentation: OpenAPI spec is properly updated

🔴 Critical Issues

1. Missing Authorization Check (Security)

The api-peer/src/actors/kv_get.rs endpoint is missing authorization validation. Other peer endpoints (e.g., delete.rs:38-64) verify that the actor belongs to the requested namespace before performing operations.

Current code (api-peer/src/actors/kv_get.rs:42-65):

pub async fn kv_get(ctx: ApiCtx, path: KvGetPath, _query: KvGetQuery) -> Result<KvGetResponse> {
    // Decode base64 key
    let key_bytes = BASE64_STANDARD
        .decode(&path.key)
        .context("failed to decode base64 key")?;

    // Get the KV value - NO AUTHORIZATION CHECK\!
    let udb = ctx.pools().udb()?;
    let (keys, values, metadata) =
        actor_kv::get(&*udb, path.actor_id, vec\![key_bytes.clone()]).await?;

Recommendation: Add authorization similar to delete.rs:

// Get the actor first to verify it exists
let actors_res = ctx
    .op(pegboard::ops::actor::get::Input {
        actor_ids: vec\![path.actor_id],
    })
    .await?;

let actor = actors_res
    .actors
    .into_iter()
    .next()
    .ok_or_else(|| pegboard::errors::Actor::NotFound.build())?;

// Optionally verify namespace if needed for authorization

This ensures that:

The actor exists before accessing its KV data
You can add namespace-based access control if needed
Consistent with other peer endpoints

2. URL Encoding Issue (Potential Bug)

The key is passed as a path parameter and expected to be base64-encoded. However, base64 strings can contain /, +, and = characters that have special meaning in URLs.

Problem: A key like aGVsbG8vd29ybGQ= (contains /) will be incorrectly parsed by the router.

Options:

Use base64url encoding (RFC 4648) which is URL-safe: base64::prelude::BASE64_URL_SAFE
Move the key to a query parameter instead of path parameter
Document that keys must be URL-encoded by the client (double encoding)

Recommendation: Use base64url encoding for URL safety:

use base64::prelude::BASE64_URL_SAFE_NO_PAD;

let key_bytes = BASE64_URL_SAFE_NO_PAD
    .decode(&path.key)
    .context("failed to decode base64 key")?;

⚠️ Issues

3. Inconsistent Timestamp Field Name

In api-peer/src/actors/kv_get.rs:63, you're using metadata[0].create_ts for the update_ts field:

Ok(KvGetResponse {
    value: value_base64,
    update_ts: metadata[0].create_ts,  // ⚠️ create_ts mapped to update_ts
})

According to CLAUDE.md naming conventions, timestamps should be named with past tense verbs (e.g., created_at, updated_at). The field name is update_ts but uses create_ts value.

Questions:

Is this intentional? Does KV only track creation time?
Should the response field be created_ts instead?
Or should KV metadata track actual update times?

4. Missing Query Parameter

The api-peer version has a KvGetQuery struct (line 17-19) that's unused. The api-public version doesn't have it at all. Consider:

Removing it if not needed
Adding a namespace query parameter for consistency with other endpoints (e.g., delete)

5. Logging Could Be Improved

Following CLAUDE.md logging patterns, add structured logging:

#[tracing::instrument(skip_all, fields(?actor_id, key_len = path.key.len()))]
pub async fn kv_get(ctx: ApiCtx, path: KvGetPath, _query: KvGetQuery) -> Result<KvGetResponse> {
    tracing::info\!("fetching kv value");
    // ... rest of implementation
}

💡 Suggestions

6. Consider Batch Get Support

The underlying actor_kv::get function supports multiple keys (Vec<rp::KvKey>). You might want to expose batch get functionality in a future PR for better performance.

7. Array Bounds Safety

The code accesses values[0] and metadata[0] without explicit bounds checking after the keys.is_empty() check. While safe due to the invariant, consider making it more explicit:

let value = values.first().context("no value returned")?;
let meta = metadata.first().context("no metadata returned")?;

8. Test Coverage

No tests were added. Consider adding:

Integration tests for the new endpoint
Tests for edge cases (empty keys, URL encoding, non-existent keys)
Tests for datacenter routing

📝 Minor Style Notes

Line 51 in api-peer: .clone() on key_bytes is unnecessary since you only use it once
Consider adding a doc comment explaining the base64 encoding expectation

Summary

Must fix before merge:

Add authorization check in peer endpoint
Fix URL encoding issue with base64 keys

Should address:
3. Clarify create_ts vs update_ts naming
4. Remove unused KvGetQuery or add namespace parameter
5. Add structured logging

Nice to have:
6. Add test coverage
7. Improve array access safety

Let me know if you'd like me to provide code snippets for any of these fixes!

claude · 2025-11-08T22:28:05Z

Code Review for PR #3410: feat(api-public): expose actor kv api

I've reviewed this PR which adds a new KV get endpoint to expose actor key-value storage through both the public and peer APIs. Here's my feedback:

✅ Positive Aspects

Good error handling: The new KvKeyNotFound error follows the established error pattern using the RivetError derive macro
Consistent routing pattern: Both peer and public API implementations follow the existing datacenter routing patterns
Proper authentication: The public API correctly uses ctx.auth().await? before processing
Base64 encoding: Correctly handles binary data through base64 encoding/decoding for transport
OpenAPI documentation: Properly generated OpenAPI schema with correct response types

🔍 Issues & Concerns

1. CRITICAL: Missing namespace validation in peer API ⚠️

The peer API implementation (engine/packages/api-peer/src/actors/kv_get.rs) does not validate that the caller has access to the actor's namespace. This is a security vulnerability.

Compare with api-peer/src/actors/delete.rs:52-64, which validates namespace ownership:

// If namespace is provided, verify the actor belongs to it
if let Some(namespace_name) = query.namespace {
    let namespace = ctx
        .op(namespace::ops::resolve_for_name_global::Input {
            name: namespace_name,
        })
        .await?
        .ok_or_else(|| namespace::errors::Namespace::NotFound.build())?;

    if actor.namespace_id \!= namespace.namespace_id {
        return Err(pegboard::errors::Actor::NotFound.build());
    }
}

Recommendation: Add a namespace query parameter and validate it before allowing KV access.

2. Inconsistent field naming in response

The response uses update_ts but the KV metadata has create_ts:

// api-peer/src/actors/kv_get.rs:63
update_ts: metadata[0].create_ts,  // ← Maps create_ts to update_ts

Looking at actor-kv/src/lib.rs:258, the metadata only stores create_ts, which represents when the key was last written (not created). This is semantically an update timestamp.

Recommendations:

Either rename to created_ts to match the internal field name
Or update the KV metadata structure to track both creation and update timestamps separately
Add documentation clarifying this timestamp represents the last write time

3. Missing error context for base64 decoding

let key_bytes = BASE64_STANDARD
    .decode(&path.key)
    .context("failed to decode base64 key")?;

This error message could be more helpful for API consumers. Consider using a custom error with better messaging.

4. Potential array index panic

// api-peer/src/actors/kv_get.rs:59-63
let value_base64 = BASE64_STANDARD.encode(&values[0]);

Ok(KvGetResponse {
    value: value_base64,
    update_ts: metadata[0].create_ts,
})

While the code checks keys.is_empty() on line 54, it accesses values[0] and metadata[0] without verifying those arrays are also non-empty. The actor_kv::get function should return equal-length arrays, but defensive coding would add assertions.

Recommendation: Add debug assertions:

debug_assert_eq\!(keys.len(), values.len());
debug_assert_eq\!(keys.len(), metadata.len());

5. Import organization

use base64::Engine;
use base64::prelude::BASE64_STANDARD;

Per CLAUDE.md guidelines: "Always add imports at the top of the file inside of inline within the function."

These should be consolidated:

use base64::{Engine, prelude::BASE64_STANDARD};

6. Missing query struct usage in public API

// api-public/src/actors/kv_get.rs:41
pub async fn kv_get(Extension(ctx): Extension<ApiCtx>, Path(path): Path<KvGetPath>) -> Response

For consistency with other endpoints (like delete, list), consider adding a Query parameter even if empty initially. This allows future extensibility (e.g., adding namespace validation).

7. Missing test coverage

Per the PR, there are no test files included. Given this exposes sensitive KV data, tests should verify:

Authentication requirements
Namespace isolation (once added)
Base64 encoding/decoding round-trips
Error cases (key not found, invalid base64)
Cross-datacenter routing

📋 Additional Recommendations

Rate limiting consideration: KV access could be expensive. Consider if rate limiting is needed.

Logging improvements: Add structured logging following CLAUDE.md patterns:

tracing::info\!(?actor_id, key_len = key_bytes.len(), "retrieved kv value");

Documentation: Add a doc comment explaining:
- Keys must be base64-encoded in the URL path
- Values are returned base64-encoded
- Size limits (from actor-kv/src/lib.rs: MAX_KEY_SIZE = 2KB, MAX_VALUE_SIZE = 128KB)
Consider batch operations: The underlying actor_kv::get supports multiple keys. Consider exposing batch get in the future.

Summary

The implementation follows established patterns well, but must add namespace validation before merging to prevent unauthorized KV access. The timestamp naming inconsistency should also be resolved for API clarity.

Would you like me to provide a code snippet showing the namespace validation implementation?

claude · 2025-11-09T05:46:51Z

Code Review for PR #3410

Thank you for implementing the Actor KV API! Here's my detailed review:

✅ Strengths

Consistent patterns: The implementation follows existing patterns in the codebase for routing between local and remote datacenters
Proper error handling: Good use of the custom error system with the new KvKeyNotFound error
Security: Authentication is properly enforced via ctx.auth().await?
Base64 encoding: Correctly handles binary keys/values via base64 encoding

🐛 Potential Issues

1. Critical: Index out of bounds risk (api-peer/src/actors/kv_get.rs:59-63)

// Current code:
if keys.is_empty() {
    return Err(pegboard::errors::Actor::KvKeyNotFound.build());
}
let value_base64 = BASE64_STANDARD.encode(&values[0]);
Ok(KvGetResponse {
    value: value_base64,
    update_ts: metadata[0].create_ts,
})

Problem: While you check if keys is empty, you don't verify that values and metadata have elements. This could panic if the actor_kv::get function returns inconsistent array sizes.

Recommendation: Add assertions or validate all three arrays:

if keys.is_empty() {
    return Err(pegboard::errors::Actor::KvKeyNotFound.build());
}

// Ensure consistency
ensure\!(
    values.len() == 1 && metadata.len() == 1,
    "inconsistent KV response: expected 1 key, value, and metadata"
);

let value_base64 = BASE64_STANDARD.encode(&values[0]);
Ok(KvGetResponse {
    value: value_base64,
    update_ts: metadata[0].create_ts,
})

2. Misleading field name (api-peer/src/actors/kv_get.rs:26)

The field is named update_ts but you're returning metadata[0].create_ts. Based on the actor-kv code at line 257-259, this is indeed the creation timestamp, not an update timestamp.

Options:

Rename to create_ts or created_at (following the naming convention in CLAUDE.md)
If this is intentional and represents when the value was last updated, add a comment explaining this

3. Missing authorization check

The endpoint doesn't verify that the authenticated user has permission to access this actor's KV data. Compare with api-peer/src/actors/delete.rs which validates the namespace.

Recommendation: Add namespace validation:

// Get actor to verify it exists and belongs to the correct namespace
let actors_res = ctx.op(pegboard::ops::actor::get::Input {
    actor_ids: vec\![path.actor_id],
}).await?;

let actor = actors_res.actors.into_iter().next()
    .ok_or_else(|| pegboard::errors::Actor::NotFound.build())?;

// Verify namespace if provided (or always require it)
// ... namespace validation logic ...

4. Missing structured logging

Per CLAUDE.md guidelines, the logging should include structured parameters:

#[tracing::instrument(skip_all, fields(actor_id = %path.actor_id, key_len = path.key.len()))]

And if you add debug logging, use:

tracing::debug\!(?actor_id, key_len = key_bytes.len(), "fetching kv value");

🔍 Questions & Considerations

5. URL encoding concerns (api-public/src/actors/kv_get.rs:68)

When forwarding to remote datacenters:

&format\!("/actors/{}/kv/keys/{}", path.actor_id, path.key)

If path.key contains special characters (even though it's base64), they should be URL-encoded. Base64 can contain +, /, and = which may need encoding.

Recommendation: Use URL encoding:

use urlencoding::encode;
&format\!("/actors/{}/kv/keys/{}", path.actor_id, encode(&path.key))

6. Performance: unnecessary clone (api-peer/src/actors/kv_get.rs:51)

let (keys, values, metadata) =
    actor_kv::get(&*udb, path.actor_id, vec\![key_bytes.clone()]).await?;

The .clone() is unnecessary since key_bytes is moved into the vec and not used afterward.

📝 Minor Improvements

7. OpenAPI documentation

The OpenAPI spec is missing:

Response status codes for errors (404 for key not found, 401 for unauthorized)
Description of the base64 encoding requirement for the key parameter
Example values

8. Missing tests

No test coverage was added. Consider adding:

Integration tests for the API endpoint
Tests for error cases (key not found, invalid base64, unauthorized access)
Tests for remote datacenter forwarding

🎯 Summary

Must Fix:

Issue [SVC-2555] Set up issue templates #1: Index out of bounds safety
Issue [SVC-2504] Fix 5 GB upload limit for local development from Cloudflare #3: Authorization check

Should Fix:

Issue [SVC-2479] Send cluster events to PostHog #2: Field naming clarity
Issue [SVC-2358] Enable Redis memory overcommit #5: URL encoding

Nice to Have:

Issue [SVC-2483] Remove hardcoded uses of rivet.gg #4: Structured logging
Issue Remove servers not in salt #6: Remove unnecessary clone
Issue [SVC-2404] Improve internal documentation #7: Better OpenAPI docs
Issue Remove fallible uses of as in api services #8: Test coverage

Overall, this is a solid implementation that follows the codebase patterns well. The main concerns are around safety (array indexing) and security (authorization).

🤖 Generated with Claude Code

claude · 2025-11-09T05:47:27Z

PR Review: feat(api-public): expose actor kv api

This PR adds a GET endpoint to retrieve KV values for actors through both the public and peer APIs. Overall, the implementation is well-structured and follows existing patterns in the codebase. Here are my findings:

✅ Strengths

Consistent Architecture: Follows the established pattern of public API → peer API → underlying KV storage layer
Proper Error Handling: Introduces a new custom error Actor::KvKeyNotFound following the RivetError pattern
Security: Properly implements authentication via ctx.auth().await? in the public API
Cross-Datacenter Support: Correctly handles routing to remote datacenters when actor is not local
Base64 Encoding: Appropriately uses base64 for binary-safe key/value transport over HTTP

🔍 Potential Issues

1. Inconsistent Field Naming (Minor)

api-peer/src/actors/kv_get.rs:26,63

The response uses update_ts but the underlying metadata has create_ts. The field name should accurately reflect what's being returned:

Currently: update_ts: metadata[0].create_ts
The field is named update_ts but returns create_ts from metadata

Recommendation: Either rename the field to created_ts to match the underlying data, or verify if the KV metadata actually tracks updates (in which case the implementation should use the correct field).

2. Missing Authorization Check in Peer API (Security)

api-peer/src/actors/kv_get.rs:42

The peer API endpoint lacks any authorization checks. While peer APIs are typically called from trusted sources (the public API layer), there's no validation that:

The requesting namespace has access to this actor
The actor actually exists before querying KV

Compare with api-peer/src/actors/delete.rs which includes namespace validation.

Recommendation: Add namespace ownership validation in the peer API similar to other actor operations.

3. Array Index Without Bounds Check (Safety)

api-peer/src/actors/kv_get.rs:59,63

After checking keys.is_empty(), the code directly accesses values[0] and metadata[0] without verifying they have the same length:

let value_base64 = BASE64_STANDARD.encode(&values[0]);
// ...
update_ts: metadata[0].create_ts,

Recommendation: Add assertions or length checks for defensive programming:

ensure!(
    keys.len() == values.len() && keys.len() == metadata.len(),
    "KV response length mismatch"
);

4. Unnecessary Clone (Performance)

api-peer/src/actors/kv_get.rs:51

The code clones key_bytes when passing to actor_kv::get():

actor_kv::get(&*udb, path.actor_id, vec![key_bytes.clone()]).await?;

Since key_bytes is consumed into a Vec and not used afterward, the clone is unnecessary.

Recommendation: Remove .clone():

actor_kv::get(&*udb, path.actor_id, vec![key_bytes]).await?;

5. Missing Documentation (Documentation)

Neither the peer nor public endpoint includes documentation about:

The base64 encoding requirement for keys in the URL path
URL encoding considerations for base64 strings (especially +, /, = characters)
Maximum key size constraints
Error conditions beyond "key not found"

Recommendation: Add doc comments explaining the encoding requirements and link to relevant constraints from actor-kv/src/lib.rs (e.g., MAX_KEY_SIZE).

📝 Minor Observations

Logging: Consider adding structured logging for the KV get operation, following the pattern from CLAUDE.md (lowercase messages with structured fields):
```
tracing::debug!(?actor_id, key_len = key_bytes.len(), "retrieving kv value");
```
Error Artifact Format: The error artifact file looks correct and follows the established pattern.
OpenAPI Schema: The generated OpenAPI spec correctly reflects the new endpoint with proper authentication requirements.

🧪 Test Coverage

I didn't see any tests added for this new endpoint. Consider adding:

Unit tests for the peer API handler
Integration tests for cross-datacenter routing
Tests for edge cases (empty key, key not found, malformed base64)

📊 Summary

Risk Level: Medium
Recommendation: Address security and safety concerns before merging

Priority Fixes:

Add namespace authorization check in peer API
Add bounds checks for array access
Clarify the update_ts vs create_ts field naming

Nice to Have:

Remove unnecessary clone
Add documentation for base64 encoding
Add test coverage

engine/packages/api-public/src/actors/kv_get.rs

claude · 2025-11-13T20:53:44Z

PR Review: feat(api-public): expose actor kv api

Summary

This PR exposes the actor KV (key-value) storage API through both the peer and public API endpoints, allowing clients to retrieve KV values stored for actors. The implementation follows the datacenter-aware routing pattern used in other endpoints.

✅ Strengths

Consistent architecture: Follows the established pattern of public API → peer API routing with datacenter-aware request forwarding
Proper base64 encoding: Keys and values are correctly base64-encoded for safe HTTP transport
Error handling: Custom error type KvKeyNotFound properly defined and used
OpenAPI documentation: API spec properly updated with schema definitions
Security: Bearer auth correctly applied to public endpoint

🔍 Issues & Recommendations

1. Critical: URL encoding issue in remote datacenter requests

// api-public/src/actors/kv_get.rs:68
&format!("/actors/{}/kv/keys/{}", path.actor_id, path.key),

Problem: The key parameter is already base64-encoded and being passed directly into the URL without proper URL encoding. Base64 can contain characters like +, /, and = that have special meaning in URLs and will cause routing issues.

Fix: URL-encode the key parameter:

&format!("/actors/{}/kv/keys/{}", path.actor_id, urlencoding::encode(&path.key))

Or better yet, use the key from the path which is already URL-encoded by axum.

2. Inconsistent naming: `update_ts` should be `created_at`

Per CLAUDE.md naming conventions:

Store dates as i64 epoch timestamps in milliseconds... name them *_at with past tense verb. For example, created_at, destroyed_at.

Current:

pub struct KvGetResponse {
    pub value: String,
    pub update_ts: i64,  // ❌
}

Should be:

pub struct KvGetResponse {
    pub value: String,
    pub created_at: i64,  // ✅
}

The underlying data is create_ts (line 63 in api-peer), so the response should reflect this as created_at.

3. Metadata inconsistency

The pegboard-actor-kv library returns create_ts in metadata (api-peer/src/actors/kv_get.rs:63), but the field is named update_ts in the response. Looking at the KV library (actor-kv/src/lib.rs:258-259), values are written with fresh timestamps on each PUT operation, making this effectively an "updated at" timestamp.

Clarification needed: Is this:

The timestamp when the value was last updated? → Should be updated_at
The timestamp when the key was first created? → Should be created_at

The underlying field is called create_ts but it's set on every PUT, suggesting it's actually an update timestamp. Please clarify the intended semantics.

4. Missing structured logging

The code lacks any logging. Per CLAUDE.md:

Use tracing for logging. Do not format parameters into the main message, instead use tracing's structured logging.

Recommendation: Add logging for observability:

// In api-peer/src/actors/kv_get.rs after line 51
tracing::info!(
    ?actor_id,
    key_len = key_bytes.len(),
    found = !keys.is_empty(),
    "retrieved kv value"
);

5. Documentation: Missing comment on base64 encoding requirement

The public API should document that the key parameter must be base64-encoded:

#[utoipa::path(
    get,
    operation_id = "actors_kv_get",
    path = "/actors/{actor_id}/kv/keys/{key}",
    params(
        ("actor_id" = Id, Path),
        ("key" = String, Path, description = "Base64-encoded key"),  // Add description
    ),
    // ...
)]

6. Minor: Unused `KvGetQuery` struct

#[derive(Debug, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct KvGetQuery {}

This empty struct is unused (prefixed with _query in the handler). Consider removing it unless you plan to add query parameters later.

7. Potential: Array access without bounds check

// api-peer/src/actors/kv_get.rs:59, 63
let value_base64 = BASE64_STANDARD.encode(&values[0]);
// ...
update_ts: metadata[0].create_ts,

While the code checks keys.is_empty() at line 54, it then indexes values[0] and metadata[0] without verifying those arrays also have elements. The actor_kv::get function should return equal-length arrays, but defensive programming would suggest adding assertions or using .first():

let value = values.first().context("expected value for key")?;
let meta = metadata.first().context("expected metadata for key")?;

🧪 Testing Recommendations

Test URL encoding: Verify that keys with special characters (+, /, =) work correctly when routing to remote datacenters
Test 404 case: Verify the error response when a key doesn't exist
Test binary values: Ensure values with non-UTF8 bytes are correctly base64-encoded
Test remote datacenter routing: Verify cross-datacenter requests work correctly

📝 Summary

Must fix before merge:

❗ URL encoding issue in remote datacenter requests (Issue [SVC-2555] Set up issue templates #1)
❗ Rename update_ts to follow naming conventions (Issue [SVC-2479] Send cluster events to PostHog #2)

Should address:
3. Add structured logging (Issue #4)
4. Document base64 encoding requirement (Issue #5)

Nice to have:
5. Clarify timestamp semantics (Issue #3)
6. Remove unused KvGetQuery or add comment explaining future use (Issue #6)
7. Add defensive bounds checking (Issue #7)

Overall, this is a solid implementation that follows established patterns. The main concerns are the URL encoding bug and naming consistency.

claude · 2025-11-13T20:53:50Z

Pull Request Review: Actor KV API Exposure

Summary

This PR exposes the Actor KV (key-value) storage API through the public and peer APIs, enabling external access to actor key-value data. The implementation follows established patterns in the codebase for distributed datacenter routing.

Code Quality & Best Practices

✅ Strengths

Consistent Architecture Pattern: The implementation correctly follows the existing dual-API pattern (api-public + api-peer) with proper datacenter routing based on actor ID labels.
Proper Error Handling: Uses the custom error system with a new KvKeyNotFound error that follows the project's error conventions.
Base64 Encoding: Appropriately uses base64 encoding for binary key/value data in the API layer, which is the right choice for HTTP APIs.
Authentication: Properly includes ctx.auth().await? in the public API and bearer_auth security in OpenAPI spec.
OpenAPI Documentation: Well-documented with proper utoipa annotations.

Issues & Concerns

🔴 Critical Issues

1. Incorrect Timestamp Field Name (api-peer/src/actors/kv_get.rs:63)

update_ts: metadata[0].create_ts,

Problem: The response field is named update_ts, but you're returning create_ts from the metadata. According to the KV metadata structure (v2.bare schema), there's only createTs - no update timestamp exists.

Impact: This is misleading API design - the field name suggests it tracks updates, but it actually only tracks creation.

Recommendation: Either:

Rename the response field to create_ts to match reality, OR
Verify if KV entries support true update timestamps and use that instead

2. Missing Namespace Authorization (api-peer/src/actors/kv_get.rs:42-65)

pub async fn kv_get(ctx: ApiCtx, path: KvGetPath, _query: KvGetQuery) -> Result<KvGetResponse> {
    // No namespace verification!
    let udb = ctx.pools().udb()?;
    let (keys, values, metadata) = 
        actor_kv::get(&*udb, path.actor_id, vec![key_bytes.clone()]).await?;

Problem: Unlike other peer endpoints (e.g., actors/delete.rs, actors/list.rs), this endpoint doesn't verify that the actor belongs to the authenticated user's namespace. An attacker could potentially read KV data from any actor if they know the actor ID.

Security Risk: HIGH - Unauthorized access to actor data

Fix Required: Add namespace verification similar to other endpoints:

// Resolve namespace to verify actor belongs to it
let namespace = ctx
    .op(namespace::ops::resolve_for_name_global::Input {
        name: query.namespace.clone(),
    })
    .await?
    .ok_or_else(|| namespace::errors::Namespace::NotFound.build())?;

// Then verify actor belongs to namespace via actor::get operation

3. Potential Index Out of Bounds (api-peer/src/actors/kv_get.rs:59, 63)

let value_base64 = BASE64_STANDARD.encode(&values[0]);
// ...
update_ts: metadata[0].create_ts,

Problem: After checking keys.is_empty(), you access values[0] and metadata[0] without verifying those arrays have elements. While actor_kv::get likely returns parallel arrays, there's no explicit validation.

Recommendation: Add assertion or explicit length check:

ensure!(
    values.len() == 1 && metadata.len() == 1,
    "unexpected kv get response length"
);

⚠️ Medium Issues

4. Missing Query Parameters for Namespace (api-public & api-peer)

The public API accepts a namespace query parameter in other endpoints (like delete, list), but this endpoint omits it entirely. This makes it inconsistent with the rest of the API surface.

Recommendation: Add namespace to the query parameters for consistency and use it for authorization.

5. No Rate Limiting Considerations

Reading large KV values (up to 128 KB according to actor-kv/src/lib.rs:19) could impact performance under high load. Consider documenting rate limits or size expectations.

6. URL Encoding Issue for Keys (api-public/src/actors/kv_get.rs:68)

&format!("/actors/{}/kv/keys/{}", path.actor_id, path.key)

Problem: The key is already base64-encoded and passed directly into the URL path. If the base64 string contains URL-unsafe characters (like +, /, =), this could cause routing issues.

Fix: URL-encode the key:

use urlencoding::encode;
&format!("/actors/{}/kv/keys/{}", path.actor_id, encode(&path.key))

📝 Minor Issues

7. Inconsistent Documentation Comment (api-peer/src/actors/kv_get.rs:24)

The comment says "Value encoded in base 64" but uses lowercase "base 64". Should be "base64" or "Base64" for consistency.

8. Unnecessary Clone (api-peer/src/actors/kv_get.rs:51)

actor_kv::get(&*udb, path.actor_id, vec![key_bytes.clone()]).await?;

You can pass vec![key_bytes] directly since it's already moved into the vector - the .clone() is wasteful.

9. Missing Tracing Context (api-peer/src/actors/kv_get.rs:42)

The tracing instrument uses skip_all, but could benefit from including the actor_id for better observability:

#[tracing::instrument(skip(ctx), fields(actor_id = %path.actor_id))]

Performance Considerations

Database Query Efficiency: The actor_kv::get function is well-optimized with proper subspace usage and streaming.
Base64 Encoding Overhead: Encoding adds ~33% size overhead, which is acceptable for HTTP APIs.
No Caching Strategy: Consider whether frequently-accessed KV entries should have caching at the API layer.

Test Coverage

❌ Missing Tests

No tests were added for this new endpoint. This is a significant gap.

Required Tests:

✅ Successful KV get with valid key
✅ 404 error when key doesn't exist
✅ Authorization: Verify users can't access other namespaces' KV data
✅ Base64 encoding/decoding correctness
✅ Cross-datacenter routing works correctly
✅ Large value handling (near 128 KB limit)
✅ Special characters in keys

Recommendation: Add test file engine/packages/engine/tests/actors_kv_get.rs following the pattern in actors_list.rs.

Recommendations

Must Fix Before Merge

✅ Add namespace authorization check in peer API
✅ Fix the update_ts vs create_ts field naming issue
✅ Add comprehensive test coverage
✅ Fix URL encoding for remote datacenter requests

Should Fix

Add explicit length validation for values/metadata arrays
Improve tracing with actor_id context
Remove unnecessary .clone() on key_bytes

Consider for Follow-up

Add rate limiting documentation
Consider caching strategy for hot keys
Add metrics/monitoring for KV access patterns

Conclusion

The implementation follows good architectural patterns and integrates well with the existing codebase. However, there are critical security concerns around namespace authorization that must be addressed before merging. The missing test coverage is also a significant gap for a new API endpoint.

Recommendation: Request Changes - Address critical security issues and add tests before merging.

claude · 2025-11-13T20:56:39Z

PR Review: feat(api-public): expose actor kv api

Thank you for this PR! Overall, the implementation looks solid and follows the codebase patterns well. Here are my findings:

Code Quality & Best Practices ✅

Strengths:

Good adherence to the repository's error handling patterns using the custom RivetError system
Proper use of workspace dependencies in Cargo.toml
Follows existing patterns for datacenter routing (local vs. remote)
Correct use of structured logging with tracing::instrument
OpenAPI schema properly generated

Issues:

Missing Documentation (Minor) - api-public/src/actors/kv_get.rs:23-26
- The response struct lacks documentation explaining that the value is base64 encoded
- Consider adding doc comments:
```
/// Value encoded in base64.
pub value: String,
/// Timestamp in milliseconds when the value was last updated.
pub update_ts: i64,
```
Inconsistent Documentation (Minor) - api-peer/src/actors/kv_get.rs:24
- The peer API has a doc comment but the public API doesn't (should be consistent)

Potential Bugs & Issues ⚠️

URL Encoding Mismatch (Critical) - api-public/src/actors/kv_get.rs:68 and api-peer/src/actors/kv_get.rs:44-46

Problem: The key is expected to be base64-encoded in the URL path, but there's a mismatch in how it's handled:
- In api-public, the key path parameter is URL-encoded when forwarded: urlencoding::encode(&path.key)
- In api-peer, the key is base64-decoded: BASE64_STANDARD.decode(&path.key)
Issue: If the base64 key contains characters like +, /, or =, they will be URL-encoded when forwarded to remote datacenters. However, the peer API expects raw base64 and will fail to decode it.

Example:
- Original key bytes: [0xFF, 0xAB]
- Base64: /6s=
- URL-encoded: %2F6s%3D
- Base64 decode of URL-encoded string will fail
Recommendation: Use URL-safe base64 encoding (BASE64_URL_SAFE_NO_PAD) instead of standard base64, or ensure the key is URL-decoded before base64 decoding in the peer API.
Unsafe Array Indexing (Medium) - api-peer/src/actors/kv_get.rs:59,65
```
let value_base64 = BASE64_STANDARD.encode(&values[0]);
// ...
update_ts: metadata[0].create_ts,
```
While there's a check for keys.is_empty(), it would be more defensive to also verify values and metadata aren't empty. The underlying actor_kv::get function should return consistent lengths, but this creates a potential panic point if that contract is violated.
Error Context Loss (Minor) - api-peer/src/actors/kv_get.rs:45-46
The base64 decode error provides generic context. Consider adding the key for debugging:
```
.with_context(|| format\!("failed to decode base64 key: {}", path.key))?;
```

Performance Considerations 💡

Unnecessary Clone (Minor) - api-peer/src/actors/kv_get.rs:51
```
actor_kv::get(&*udb, path.actor_id, vec\![key_bytes.clone()]).await?;
```
The key_bytes.clone() is unnecessary since key_bytes is no longer used after this call. Can simply use vec\![key_bytes].
Good Use of Pooled Client ✅
The code correctly uses the underlying rivet_pools::reqwest::client() via request_remote_datacenter_raw, following CLAUDE.md guidance.

Security Concerns 🔒

Path Traversal Protection (Verified) ✅
The implementation correctly handles key encoding/decoding and doesn't concatenate paths unsafely.
Authentication (Verified) ✅
Proper ctx.auth().await? call in the public API.
Input Validation - api-peer/src/actors/kv_get.rs:48
The validate_keys function is called in actor_kv::get, which should handle size limits and validation. Good defense in depth.
Base64 Decode Bomb - The base64 key could potentially decode to a very large value. However, the actor_kv module has MAX_KEY_SIZE = 2 * 1024 validation, so this is properly protected.

Test Coverage 📋

Missing:

No tests added for the new endpoint
Should add tests for:
- Basic get operation with valid key
- Non-existent key (404 error case)
- Invalid base64 key
- Keys with special characters (+, /, =)
- Remote datacenter forwarding

Recommendation: Add integration tests similar to engine/packages/engine/tests/actors_*.rs patterns.

Additional Recommendations

OpenAPI Documentation - The generated OpenAPI schema should include:
- Description that the key must be base64-encoded
- Example request/response
- Error responses (404 for key not found)
Naming Consistency Note - The comment at api-peer/src/actors/kv_get.rs:63-64 mentions create_ts is actually update_ts. This is confusing. Consider:
- Fixing the field name in the underlying metadata struct if possible
- Or documenting this more prominently in the KV module itself

Summary

Must Fix:

⚠️ URL encoding/Base64 encoding compatibility issue for remote datacenter forwarding

Should Fix:

Add test coverage for the new endpoint
Remove unnecessary clone
Add defensive checks for array indexing

Nice to Have:

Improve documentation and error messages
Add OpenAPI documentation for the base64 encoding requirement

The implementation follows good patterns overall, but the URL encoding issue needs to be addressed before merging.

graphite-app · 2025-11-13T21:15:10Z

Merge activity

Nov 13, 9:15 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Nov 13, 9:15 PM UTC: CI is running for this pull request on a draft pull request (#3465) due to your merge queue CI optimization settings.
Nov 13, 9:16 PM UTC: Merged by the Graphite merge queue via draft PR: #3465.

NathanFlurry mentioned this pull request Nov 7, 2025

fix(rivetkit): properly handle msgIndex for hibernatable websocket reconnection #3401

Closed

vercel bot deployed to Preview – rivetkit-serverless November 7, 2025 22:03 View deployment

NathanFlurry mentioned this pull request Nov 7, 2025

fix(rivetkit): use loggerWithoutContext for websocket error when actor undefined #3412

Closed

NathanFlurry force-pushed the 11-07-chore_rivetkit_auto-generate_inspector_token_for_engine_driver branch from a3b30a7 to 3c5d900 Compare November 8, 2025 22:25

NathanFlurry force-pushed the 11-07-feat_api-public_expose_actor_kv_api branch from 4f3a2a9 to 42bf4e8 Compare November 8, 2025 22:25

vercel bot deployed to Preview – rivetkit-serverless November 8, 2025 22:26 View deployment

NathanFlurry force-pushed the 11-07-feat_api-public_expose_actor_kv_api branch from 42bf4e8 to da963b1 Compare November 9, 2025 05:44

NathanFlurry force-pushed the 11-07-chore_rivetkit_auto-generate_inspector_token_for_engine_driver branch from 3c5d900 to 290e897 Compare November 9, 2025 05:44

vercel bot deployed to Preview – rivetkit-serverless November 9, 2025 05:46 View deployment

NathanFlurry mentioned this pull request Nov 13, 2025

chore(rivetkit): fix type checks #3462

Merged

This was referenced Nov 13, 2025

feat(gas): implement smart pulling for horizontally scaled workers #3417

Merged

fix(guard): handle websocket tasks during shutdown #3448

Merged

fix: improve sigterm handling for the entire runtime #3443

Merged

NathanFlurry mentioned this pull request Nov 13, 2025

feat(gateway): websocket hibernation #3400

Merged

This was referenced Nov 13, 2025

fix(pb): dont immediately reallocate actors on runner stopping #3413

Merged

fix(guard): dont send open msg for hibernating ws, hibernating ws keepalive #3449

Merged

NathanFlurry mentioned this pull request Nov 13, 2025

chore(gas): rename wake sub to bump sub #3408

Merged

MasterPtato requested changes Nov 13, 2025

View reviewed changes

engine/packages/api-public/src/actors/kv_get.rs Outdated Show resolved Hide resolved

graphite-app bot changed the base branch from 11-07-chore_rivetkit_auto-generate_inspector_token_for_engine_driver to graphite-base/3410 November 13, 2025 20:50

graphite-app bot force-pushed the 11-07-feat_api-public_expose_actor_kv_api branch from 9624b78 to 9b70310 Compare November 13, 2025 20:51

graphite-app bot force-pushed the graphite-base/3410 branch from 7d62e3b to 2294806 Compare November 13, 2025 20:51

feat(api-public): expose actor kv api

a9a9e7c

graphite-app bot changed the base branch from graphite-base/3410 to main November 13, 2025 20:51

graphite-app bot force-pushed the 11-07-feat_api-public_expose_actor_kv_api branch from 9b70310 to 83a610d Compare November 13, 2025 20:51

NathanFlurry requested a review from MasterPtato November 13, 2025 20:52

vercel bot deployed to Preview – rivetkit-serverless November 13, 2025 20:53 View deployment

NathanFlurry force-pushed the 11-07-feat_api-public_expose_actor_kv_api branch from 83a610d to a9a9e7c Compare November 13, 2025 20:53

vercel bot deployed to Preview – rivetkit-serverless November 13, 2025 20:55 View deployment

NathanFlurry mentioned this pull request Nov 13, 2025

chore(rivetkit): implement new hibernating ws protocol #3464

Open

MasterPtato approved these changes Nov 13, 2025

View reviewed changes

graphite-app bot pushed a commit that referenced this pull request Nov 13, 2025

feat(api-public): expose actor kv api (#3410)

a2600ff

graphite-app bot closed this Nov 13, 2025

graphite-app bot deleted the 11-07-feat_api-public_expose_actor_kv_api branch November 13, 2025 21:16

NathanFlurry mentioned this pull request Nov 13, 2025

feat(cloudflare-workers): add createInlineClient #3466

Open

NicholasKissel mentioned this pull request Nov 15, 2025

chre: v4 website changes #3472

Draft

feat(api-public): expose actor kv api #3410

feat(api-public): expose actor kv api #3410

Uh oh!

Conversation

NathanFlurry commented Nov 7, 2025

Uh oh!

vercel bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NathanFlurry commented Nov 7, 2025 • edited by jog1t Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Nov 7, 2025

PR Review: Expose Actor KV API

✅ Strengths

🔍 Issues & Concerns

1. URL Encoding Vulnerability (Critical)

2. Inconsistent Field Naming (Medium)

3. Missing Structured Logging (Medium)

4. Authorization Gaps (High)

5. Missing Input Validation

6. Performance: Unnecessary Clone (Low)

📝 Additional Recommendations

Summary

Uh oh!

pkg-pr-new bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Nov 8, 2025

PR Review: feat(api-public): expose actor kv api

✅ Strengths

🔴 Critical Issues

1. Missing Authorization Check (Security)

2. URL Encoding Issue (Potential Bug)

⚠️ Issues

3. Inconsistent Timestamp Field Name

4. Missing Query Parameter

5. Logging Could Be Improved

💡 Suggestions

6. Consider Batch Get Support

7. Array Bounds Safety

8. Test Coverage

📝 Minor Style Notes

Summary

Uh oh!

claude bot commented Nov 8, 2025

Code Review for PR #3410: feat(api-public): expose actor kv api

✅ Positive Aspects

🔍 Issues & Concerns

1. CRITICAL: Missing namespace validation in peer API ⚠️

2. Inconsistent field naming in response

3. Missing error context for base64 decoding

4. Potential array index panic

5. Import organization

6. Missing query struct usage in public API

7. Missing test coverage

📋 Additional Recommendations

Summary

Uh oh!

claude bot commented Nov 9, 2025

Code Review for PR #3410

✅ Strengths

🐛 Potential Issues

1. Critical: Index out of bounds risk (api-peer/src/actors/kv_get.rs:59-63)

2. Misleading field name (api-peer/src/actors/kv_get.rs:26)

3. Missing authorization check

4. Missing structured logging

🔍 Questions & Considerations

5. URL encoding concerns (api-public/src/actors/kv_get.rs:68)

6. Performance: unnecessary clone (api-peer/src/actors/kv_get.rs:51)

📝 Minor Improvements

7. OpenAPI documentation

8. Missing tests

🎯 Summary

Uh oh!

claude bot commented Nov 9, 2025

PR Review: feat(api-public): expose actor kv api

✅ Strengths

🔍 Potential Issues

vercel bot commented Nov 7, 2025 •

edited

Loading

NathanFlurry commented Nov 7, 2025 •

edited by jog1t

Loading

pkg-pr-new bot commented Nov 7, 2025 •

edited

Loading

2. Inconsistent naming: `update_ts` should be `created_at`

6. Minor: Unused `KvGetQuery` struct