Skip to content

fix(epoxy): reuse cached optimistic value#4618

Open
NathanFlurry wants to merge 1 commit intomainfrom
04-12-fix_epoxy_reuse_cached_optimistic_value
Open

fix(epoxy): reuse cached optimistic value#4618
NathanFlurry wants to merge 1 commit intomainfrom
04-12-fix_epoxy_reuse_cached_optimistic_value

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-4618 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud ❌ Build Failed (View Logs) Web Apr 13, 2026 at 2:16 am
website ❌ Build Failed (View Logs) Web Apr 13, 2026 at 2:16 am
mcp-hub ✅ Success (View Logs) Web Apr 13, 2026 at 2:15 am
ladle ❌ Build Failed (View Logs) Web Apr 13, 2026 at 2:15 am
kitchen-sink ❌ Build Failed (View Logs) Web Apr 13, 2026 at 2:15 am
frontend-inspector ❌ Build Failed (View Logs) Web Apr 13, 2026 at 2:15 am

@claude
Copy link
Copy Markdown

claude bot commented Apr 13, 2026

PR Review: fix(epoxy): reuse cached optimistic value

Overall: Correct bug fix, clean implementation.

What This Fixes

The original code had a subtle but serious bug in the optimistic cache path of read_local_value. After deserializing the raw DB bytes into a CachedValue, it then extracted the inner value: Vec<u8> field and called cache_key.deserialize(&value)? on it, treating the inner payload as if it were the full serialized CachedValue blob again.

This would almost always fail at runtime due to format mismatch, meaning any cache hit where CachedValue.value was Some would propagate an error instead of returning the cached value. The optimistic read path was effectively broken for non-empty cache entries.

The fix correctly reconstructs the CachedValue using the already-extracted inner value and struct update syntax for the remaining fields (version, etc.). The Rust partial-move is valid: since value is explicitly provided in the struct literal, ..cache_value does not attempt to re-use the already-moved cache_value.value field.

Confirmed correct by tracing the consumer in get_optimistic.rs:67-72, which does local_read.cache_value.and_then(|v| v.value). The fix ensures v.value is Some(bytes) as expected.

Suggestions

Test coverage: There is no test exercising the include_cache=true path in read_local_value where the cache contains a non-empty value. Given this was a silent failure before, a regression test would be valuable: write a value via the fanout path (so it lands in cache but not committed storage), call read_local_value(..., include_cache=true), and assert cache_value.value is Some with the expected bytes.

PR description: The draft template is unfilled. Worth adding a short motivation note before marking ready for review.

No other issues. The fix is minimal, correct, and easy to reason about.

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