Skip to content

fix(cache): clean up lib#4457

Open
MasterPtato wants to merge 1 commit into03-18-fix_config_allow_configuring_topo_dcs_via_map_fix_pg_ssl_mode_configfrom
03-18-fix_cache_clean_up_lib
Open

fix(cache): clean up lib#4457
MasterPtato wants to merge 1 commit into03-18-fix_config_allow_configuring_topo_dcs_via_map_fix_pg_ssl_mode_configfrom
03-18-fix_cache_clean_up_lib

Conversation

@MasterPtato
Copy link
Contributor

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

railway-app bot commented Mar 19, 2026

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

Service Status Web Updated (UTC)
frontend-cloud 😴 Sleeping (View Logs) Web Mar 19, 2026 at 2:15 am
frontend-inspector 😴 Sleeping (View Logs) Web Mar 19, 2026 at 2:15 am
website ❌ Build Failed (View Logs) Web Mar 19, 2026 at 2:08 am
mcp-hub ✅ Success (View Logs) Web Mar 19, 2026 at 2:07 am
ladle ❌ Build Failed (View Logs) Web Mar 19, 2026 at 2:06 am

Copy link
Contributor Author

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

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 19, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/sqlite-vfs

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sqlite-vfs@4457

@rivetkit/traces

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

@rivetkit/workflow-engine

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/workflow-engine@4457

@rivetkit/virtual-websocket

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 175706a

@claude
Copy link

claude bot commented Mar 19, 2026

Code Review

Good cleanup PR overall. Removing the unimplemented Redis driver and the unused rate-limiting code reduces surface area, and the double-serialization layer simplification in fetch_all_convert is a clear improvement.

Issues

Unnecessary clone in delete (driver.rs:211)

get and set were both updated to use self.cache directly, but delete was missed - it still does let cache = self.cache.clone(). Since moka::future::Cache is Arc-backed the clone is cheap, but it is inconsistent with the refactoring done to the other two methods.

Redundant RawCacheKey conversions in purge (req_config.rs:225-255)

process_key now returns RawCacheKey instead of String, but purge still has two conversion steps that are now no-ops via the blanket From for T impl:

  • .map(|k| RawCacheKey::from(k.clone())) is equivalent to .cloned()
  • let raw_keys = cache_keys.into_iter().map(RawCacheKey::from).collect() is also identity

Both can be simplified or the two intermediate vectors collapsed into one.

Silent 10x capacity increase (inner.rs)

Default capacity changed from 1000 to 10000 with no comment. This could meaningfully increase memory usage in constrained environments. Worth a brief comment explaining the rationale.

Nits

  • The #[non_exhaustive] attribute on Driver is now less meaningful with only one variant. Still valid forward-compatibility intent, but worth confirming it is deliberate.
  • Renaming fetch_all / fetch_one to fetch_all_json / fetch_one_json is a good clarification that these are JSON-specific.

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