Skip to content

chore(pegboard): cache actor kv info#4617

Merged
NathanFlurry merged 1 commit intomainfrom
04-12-chore_pegboard_cache_actor_kv_info
Apr 13, 2026
Merged

chore(pegboard): cache actor kv info#4617
NathanFlurry merged 1 commit intomainfrom
04-12-chore_pegboard_cache_actor_kv_info

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

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?

Please describe the tests that you ran to verify your changes.

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

@railway-app
Copy link
Copy Markdown

railway-app Bot commented Apr 13, 2026

🚅 Deployed to the rivet-pr-4617 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud 🕐 Initializing (View Logs) Web Apr 13, 2026 at 1:24 am
website 🕐 Initializing (View Logs) Web Apr 13, 2026 at 1:24 am
mcp-hub 🕐 Initializing (View Logs) Web Apr 13, 2026 at 1:24 am
ladle 🕐 Initializing (View Logs) Web Apr 13, 2026 at 1:24 am
kitchen-sink 🕐 Initializing (View Logs) Web Apr 13, 2026 at 1:24 am
frontend-inspector 🕐 Initializing (View Logs) Web Apr 13, 2026 at 1:24 am

@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-4617 April 13, 2026 01:24 Destroyed
@NathanFlurry NathanFlurry merged commit d00c2b0 into main Apr 13, 2026
4 of 5 checks passed
Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@NathanFlurry NathanFlurry deleted the 04-12-chore_pegboard_cache_actor_kv_info branch April 13, 2026 01:24
@claude
Copy link
Copy Markdown

claude Bot commented Apr 13, 2026

Code Review: chore(pegboard): cache actor kv info

This PR adds a read-through cache to pegboard_actor_get_for_kv, eliminating a UDB read-transaction per KV request for actor name and namespace_id lookup. The implementation correctly uses the existing rivet_cache infrastructure and removes the // TODO: Add cache comment.


What works well

  • Correct caching pattern. The ctx.cache().clone().request().fetch_one_json(...) pattern matches established patterns in runner_config/get.rs and runner/list_runner_config_enabled_dcs.rs.
  • Correct derives. Serialize and Deserialize were correctly added to Output (required by fetch_one_json). Input doesn't need them.
  • Cache miss handled correctly. The None (actor not found) case is intentionally not resolved into the cache, matching the fetch_one_json contract — non-existent actors won't be negatively cached indefinitely.
  • Span moved correctly. The custom_instrument span was moved inside the getter closure, still wrapping only the UDB transaction.

Issues

Missing cache invalidation on actor destroy (correctness concern)

The cached data — actor name and namespace_id — is write-once, so caching it is conceptually safe for a live actor. However, there is no corresponding cache purge in the actor destroy path.

The caller in ws_to_tunnel_task.rs uses this result to verify that the actor exists and belongs to the right namespace. With the default 2-hour TTL, a stale cache entry could allow a destroyed actor's KV operations to proceed for up to 2 hours post-destruction.

The existing purge_runner_config_caches pattern in utils.rs shows the expected approach. A purge("actor.actor_get_for_kv", vec![actor_id]) call should be added in the actor destroy workflow activity (workflows/actor/destroy.rs), mirroring that pattern.

Note: the analogous comment on get_for_runner.rs explicitly acknowledged this: // TODO: Add cache (remember to purge cache when runner changes).

No explicit TTL

The 2-hour default TTL is used silently. Peer operations in this package set explicit TTLs (runner_config/get.rs: 5 s, runner/list_runner_config_enabled_dcs.rs: 1 hr). Since the data is write-once, an explicit .ttl(...) call or a comment explaining why the default is acceptable would improve clarity and guard against future default changes.

None not cached — potential DB pressure for non-existent actors

When the actor is not found, cache.resolve is not called, so every request for a non-existent actor_id hits UDB. This is the right tradeoff to avoid stale positive entries after destroy, but it's worth noting that once the destroy purge is in place, a short-TTL negative cache entry (e.g., 5–30 s) could further reduce DB pressure during destroy races. Lower priority, but worth considering holistically.


Conventions

No issues: no rivet.gg references, no println!/eprintln!, no Mutex<HashMap>, no new unstructured log statements, no anyhow glob imports.

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.

1 participant