refactor: per-register cache to fix stale data on writes#8
Open
Conversation
Cache keys now address individual registers/coils instead of request ranges. This fixes stale data when writes hit registers that are part of a larger cached read range. Key changes: - RegKey(slaveID, fc, addr) for per-register storage - RangeKey(slaveID, fc, addr, qty) for request coalescing - GetRange/SetRange/DeleteRange for batch operations - Rename GetOrFetch to Coalesce (no longer interacts with cache) - Add keepStale flag to prevent cleanup from removing entries needed for stale-serve fallback
Decompose upstream responses into per-register cache entries and reassemble from cache on hits. Write invalidation now correctly removes individual registers in the written range, fixing stale data when writes overlap with larger cached read ranges. New helpers: - decomposeResponse: extract per-register values from Modbus PDU - assembleResponse: reconstruct Modbus PDU from cached values - Roundtrip tests for all function codes (registers + coils) - Tests verifying write invalidation of overlapping reads
Move the cache miss log before Coalesce so coalesced waiters also get a log entry. The upstream client already logs request completion with duration, so fetches vs coalesced waits are distinguishable.
There was a problem hiding this comment.
Pull request overview
Refactors the Modbus proxy cache from range-keyed entries to per-register/per-coil entries to prevent stale reads after overlapping writes, while preserving range-level request coalescing and improving stale-serving behavior.
Changes:
- Replace range-based cache storage with per-register/per-coil storage (
RegKey,GetRange/SetRange/DeleteRange), while keeping coalescing on range keys (RangeKey,Coalesce). - Update proxy read path to assemble responses from per-register cache on hits, and decompose upstream responses into per-register entries on store; update write invalidation to delete affected registers/coils.
- Add/adjust tests for decompose/assemble helpers, overlapping write invalidation, range APIs, and coalescing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/proxy/proxy.go | Switch read caching to per-register storage, add response decompose/assemble helpers, update write invalidation to delete per-register entries. |
| internal/proxy/proxy_test.go | Update existing proxy cache tests and add new helper + invalidation tests for the new per-register behavior. |
| internal/cache/cache.go | Introduce per-register keys and range APIs; split range coalescing into Coalesce; add keepStale to skip eviction for stale-serving. |
| internal/cache/cache_test.go | Update constructor usage and add tests for range APIs, coalescing, data isolation, and keepStale behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Guard GetRange/GetRangeStale against quantity=0 (false cache hit) - Use shared coilOn/coilOff slices in decomposeResponse to reduce per-coil allocations - Extract cleanupOnce so tests exercise the real keepStale guard instead of manually simulating cleanup
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The cache keyed entries by the full request range (
slaveID:fc:startAddr:quantity). Write invalidation only matched exact keys, so a write to register 10 would never invalidate a cached read of registers 0-49 — the keys simply didn't match. In energy management setups where battery power or charge current is written every ~10s, this meant overlapping read ranges silently served stale data until TTL expiry.Additionally, when
MODBUS_CACHE_SERVE_STALE=true, the background cleanup goroutine could delete expired entries needed for stale fallback, making the feature unreliable.Solution
Switch from range-based cache keys to per-register keys (
slaveID:fc:addr). Each register/coil gets its own cache entry. Upstream responses are decomposed into individual register values on store, and reassembled from cached values on hit.keepStaleflag: whenCacheServeStaleis enabled, the cleanup goroutine skips deletion so stale entries are always available for fallback.Changes
internal/cache/cache.go—RegKey/RangeKey,GetRange/SetRange/DeleteRange, renameGetOrFetch→Coalesce(decoupled from cache storage),keepStaleflaginternal/proxy/proxy.go—decomposeResponse/assembleResponsehelpers, range-aware invalidation