fix: prevent store cache corruption on retry by snapshotting before flush

[joonas]

Description

The retry path in sendUpdatesAndFlushCache used Number(index) to
look up operations by position in a parallel values array. Cache keys
are non-numeric strings produced by fillStoreCache (e.g.
"add:/data/cap-v2-key:value"), so Number(key) always evaluated to
NaN, and payload[NaN] produced undefined. Every retryable
failure silently replaced all pending operations with undefined,
permanently losing them.

The fix snapshots the cache ({ ...cache }) before the patch attempt
and restores it with Object.assign(cache, snapshot) on retryable
failure, avoiding the key-to-index cross-reference entirely.

The existing retry test was tautological — it compared result to
cache, but both were the same object reference, so the assertion
always passed regardless of corruption. The updated test snapshots the
expected state into an independent copy and uses realistic keys from
fillStoreCache to exercise the actual code path.

End to End Test:
(See Pepr Excellent Examples)

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Other (security config, docs update, etc)

Checklist before merging

• Unit, Integration, docs, adr added or updated as needed
• Contributor Guide Steps followed

[cmwylie19]

yep, thanks. Nice catch

[samayer12]

@greptileai

[greptile-apps]

Greptile Summary

This PR fixes a silent data-loss bug in sendUpdatesAndFlushCache where the retry path used Number(index) on non-numeric string keys (e.g. "add:/data/cap-v2-key:value"), always yielding NaN and clobbering every pending operation with undefined. The fix snapshots the cache before the patch attempt and uses a restoreMissing helper on retryable failure, correctly preserving any concurrent writes that arrived while the patch was in flight. Test coverage is substantially improved, including a new test that catches the formerly-tautological assertion.

Confidence Score: 5/5

Safe to merge — fixes a real silent-data-loss bug with correct snapshot semantics and thorough tests.

All findings are P2 or lower. The core fix is well-reasoned: snapshotting before flush avoids the NaN index bug, and restoreMissing correctly preserves concurrent writes. The new tests cover the key edge cases (concurrent write on success, concurrent write on retry, non-tautological repopulation check). No regressions or security concerns identified.

No files require special attention.

Important Files Changed

Filename	Overview
src/lib/controller/storeCache.ts	Replaces broken index-based retry with snapshot/restoreMissing approach; logic is correct and handles concurrent writes on both success and retryable-failure paths.
src/lib/controller/storeCache.test.ts	Adds three new scenario tests (concurrent write on success, concurrent write on retry, realistic-key repopulation) and fixes the previously tautological same-reference assertion.

Sequence Diagram

sequenceDiagram
    participant Sender as sender()
    participant Flush as sendUpdatesAndFlushCache
    participant K8s as K8s Patch API

    Sender->>Flush: call with cache {key1: op1}
    Note over Flush: snapshot = { ...cache }<br/>payload = Object.values(snapshot)
    Flush->>K8s: Patch(payload)
    Note over Flush,K8s: in-flight — Sender may write<br/>new keys to cache

    alt Patch succeeds
        K8s-->>Flush: resolved
        Note over Flush: delete snapshot keys from cache<br/>(concurrent writes survive)
    else 422 Unprocessable Entity
        K8s-->>Flush: rejected {status: 422}
        Note over Flush: delete snapshot keys from cache<br/>(discard bad ops permanently)
    else retryable error (e.g. 500)
        K8s-->>Flush: rejected {status: 500}
        Note over Flush: restoreMissing(cache, snapshot)<br/>adds snapshot keys only if absent<br/>(concurrent writes for same key survive)
    end

    Flush-->>Sender: return cache

Loading

_{Reviews (1): Last reviewed commit: "Merge branch 'main' into fix/store-cache..." | Re-trigger Greptile}