perf(sqlite-native): reuse read cache for partial writes

[NathanFlurry]

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

[NathanFlurry]

• chore(engine): publish engine bases in ci #4649 : 3 dependent PRs (#4650 , #4652 , #4666 )
• chore: lockfile lefthook #4647
• fix(rivetkit): surface native sqlite kv errors to callers #4645
• fix(rivetkit): prevent sleep races during disconnect and db work #4644
• fix(rivetkit): isolate engine envoys and propagate startup failures #4643
• test(rivetkit): remove wasm sqlite-only fixtures and coverage #4642
• chore(sqlite-vfs): remove wasm sqlite packages and workspace wiring #4641
• chore(rivetkit): remove wasm sqlite runtime #4614
• fix(sqlite-native): restore native startup kv preload #4639
• fix(sqlite-native): delete metadata before chunk range #4638
• fix(sqlite-native): keep truncate cache coherent #4637
• fix(sqlite-vfs): use delete range for truncate cleanup #4636
• fix(sqlite-native): restore kv error hook #4635
• perf(sqlite-native): avoid cloning cached read chunks #4634
• perf(sqlite-native): gate kv operation labels #4633
• perf(sqlite-native): reuse read cache for partial writes #4632 👈 (View in Graphite)
• perf(sqlite-native): remove delete file existence probe #4631
• fix(sqlite-native): gate native read cache behind env var #4630
• fix(pegboard): isolate runner config dc lookup failures #4625 : 1 other dependent PR (#4626 )
• docs(pegboard): note runner config upsert split write risk #4624
• fix(pegboard): unpack global runner config keys correctly #4623
• fix(pegboard-gateway): enforce tunnel message state #4622
• fix(pegboard-runner): clear terminal tunnel routes #4621
• fix(pegboard): skip protocol version keys in runner pool backfill #4620
• fix(pegboard): create missing normal runner configs #4619
• fix(epoxy): reuse cached optimistic value #4618
• main

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

[claude]

Review: perf(sqlite-native): reuse read cache for partial writes

Summary of change: When kv_io_write needs to perform a read-modify-write on a chunk (i.e., needs_existing is true), the new code first checks the in-memory read cache before issuing a KV round-trip. This is a clean and well-scoped optimization. The overall approach is sound.

---

Correctness

Inconsistent guard: ctx.read_cache_enabled vs state.read_cache.is_some()

The new code in kv_io_write gates the cache lookup on ctx.read_cache_enabled. However, every other read-cache access site in the file uses state.read_cache.as_ref() / state.read_cache.as_mut() — which is None when the cache is disabled and Some(...) when enabled — and does not separately check ctx.read_cache_enabled. The KvFileState::new constructor already encodes the enabled/disabled state into the Option:

read_cache: read_cache_enabled.then(HashMap::new),

The double-guard is redundant and introduces a divergence from the rest of the file's pattern. If ctx.read_cache_enabled is ever true but state.read_cache is somehow None (e.g., a future refactor changes construction), the code would silently return None and skip the cache hit. Conversely, ctx.read_cache_enabled == false while state.read_cache is Some would cause this path to fall through to a KV fetch while the read path still serves from cache — an inconsistency between read and write behavior.

Recommendation: Follow the same pattern as kv_io_read by accessing state.read_cache as an Option directly:

let cached_chunk = if needs_existing {
    let state = get_file_state(file.state);
    state.read_cache.as_ref().and_then(|c| c.get(chunk_key.as_slice()).cloned())
} else {
    None
};

---

get_file_state called twice in the same scope

The write planning loop calls get_file_state(file.state) inside the conditional block for cached_chunk, but file.state is accessed again later in the same loop. Consolidating to a single state binding at the top of the loop body (as other callbacks do) would improve readability.

---

Cache Coherence

The existing post-KV-put cache update correctly inserts the newly written chunk into the read cache only after a successful put. The cache-hit path reads a pre-existing value that will be overwritten by the put — no stale-cache issue.

On kv_put failure, file.size and file.meta_dirty are rolled back, and the cache update is inside the success path. Cache coherence is preserved on error. Both of these are handled correctly.

---

Minor Issues

PR description is a template stub. The PR body contains only the default checklist with no description of the motivation or testing notes.

No test coverage for the cache-hit path in writes. The test section covers encoding/decoding and struct-level invariants but does not exercise the VFS callbacks directly. If integration-level tests exist elsewhere in the package, it would be worth adding a test that verifies a partial write on a cached chunk does not issue a redundant KV read.

---

Summary

Area	Assessment
Correctness	The inconsistent guard (ctx.read_cache_enabled instead of state.read_cache.as_ref()) diverges from all other cache sites. Not a bug today, but fragile.
Performance	The optimization is valid — cache hits on partial-write chunks eliminate a full KV round-trip per affected chunk.
Cache coherence	Write-after-read and rollback-on-failure are handled correctly.
Style	Use the state.read_cache.as_ref() pattern used everywhere else rather than re-checking the boolean flag.

The core idea is correct and the implementation is nearly there. The main ask is aligning the guard style with the rest of the file before merging.