fix(store): Implement version cache invalidation strategy for JetStreamEventStore #130

Merged

HugoNijhuis merged 2 commits from issue-126-untitled into main 2026-01-13 18:48:02 +00:00

Conversation 4 Commits 2 Files Changed 3

+178 -1

HugoNijhuis commented 2026-01-12 23:24:38 +00:00

Owner

Copy Link

Copy Source

Summary

Implements version cache invalidation strategy in JetStreamEventStore to ensure consistency when external processes write to the same JetStream stream.

Changes

• Add cache invalidation logic to GetLatestVersion() that detects stale cache entries
• Document version cache behavior and single-writer assumption in code comments
• Add comprehensive documentation in CLAUDE.md about cache invalidation strategy
• Add TestJetStreamEventStore_CacheInvalidationOnExternalWrite integration test

Implementation Strategy

The solution uses a hybrid approach that satisfies all acceptance criteria:

1. Document assumption: Added clear documentation about single-writer assumption in the JetStreamEventStore struct comment and CLAUDE.md
2. Implement cache invalidation: When GetLatestVersion detects that the actual version in JetStream is newer than the cached version, the cache entry is invalidated (deleted). Subsequent checks will fetch fresh data.
3. Performance: Cache hits are still fast for the single-writer case, while external writes are handled gracefully.

How It Works

• SaveEvent uses getLatestVersionLocked() which checks JetStream on cache miss
• GetLatestVersion always fetches from JetStream and compares with cached version
• If actual version > cached version, cache is invalidated (entry deleted)
• Future checks for that actor will fetch fresh data until cache is repopulated

Testing

• All existing tests pass
• New integration test (TestJetStreamEventStore_CacheInvalidationOnExternalWrite) verifies:
  • store1 caches version 1 after SaveEvent
  • store2 writes version 2 (external write from store1's perspective)
  • store1.GetLatestVersion detects and returns version 2
  • store1.SaveEvent works correctly after cache invalidation

Closes #126

## Summary Implements version cache invalidation strategy in JetStreamEventStore to ensure consistency when external processes write to the same JetStream stream. ## Changes - Add cache invalidation logic to GetLatestVersion() that detects stale cache entries - Document version cache behavior and single-writer assumption in code comments - Add comprehensive documentation in CLAUDE.md about cache invalidation strategy - Add TestJetStreamEventStore_CacheInvalidationOnExternalWrite integration test ## Implementation Strategy The solution uses a hybrid approach that satisfies all acceptance criteria: 1. **Document assumption**: Added clear documentation about single-writer assumption in the JetStreamEventStore struct comment and CLAUDE.md 2. **Implement cache invalidation**: When GetLatestVersion detects that the actual version in JetStream is newer than the cached version, the cache entry is invalidated (deleted). Subsequent checks will fetch fresh data. 3. **Performance**: Cache hits are still fast for the single-writer case, while external writes are handled gracefully. ## How It Works - SaveEvent uses getLatestVersionLocked() which checks JetStream on cache miss - GetLatestVersion always fetches from JetStream and compares with cached version - If actual version > cached version, cache is invalidated (entry deleted) - Future checks for that actor will fetch fresh data until cache is repopulated ## Testing - All existing tests pass - New integration test (TestJetStreamEventStore_CacheInvalidationOnExternalWrite) verifies: - store1 caches version 1 after SaveEvent - store2 writes version 2 (external write from store1's perspective) - store1.GetLatestVersion detects and returns version 2 - store1.SaveEvent works correctly after cache invalidation Closes #126

HugoNijhuis added 1 commit 2026-01-12 23:24:38 +00:00

fix(store): Implement version cache invalidation strategy for JetStreamEventStore ... 6de897ef60

Implements cache invalidation on GetLatestVersion when external writers modify the
JetStream stream. The strategy ensures consistency in multi-store scenarios while
maintaining performance for the single-writer case.

Changes:
- Add cache invalidation logic to GetLatestVersion() that detects stale cache
- Document version cache behavior in JetStreamEventStore struct comment
- Add detailed documentation in CLAUDE.md about cache invalidation strategy
- Add TestJetStreamEventStore_CacheInvalidationOnExternalWrite integration test
- Cache is invalidated by deleting entry, forcing fresh fetch on next check

The implementation follows the acceptance criteria by:
1. Documenting the single-writer assumption in code comments
2. Implementing cache invalidation on GetLatestVersion miss
3. Adding comprehensive test for external write scenarios

Closes #126

Co-Authored-By: Claude Code <noreply@anthropic.com>

HugoNijhuis commented 2026-01-12 23:47:12 +00:00

Author

Owner

Copy Link

Copy Source

Code Review: Changes Requested

Thanks for the PR! I found critical issues in the cache invalidation logic.

CRITICAL BUG: Cache not repopulated after invalidation (lines 327-337)

In GetLatestVersion(), when cache is detected as stale, you delete it but never update it with the fresh data just fetched. The cache should be repopulated immediately:

Current code deletes cache: delete(jes.versions[actorID])
Should instead: jes.versions[actorID] = latestVersion

This defeats the cache purpose - next SaveEvent will miss cache again.

RACE CONDITION: Unsynchronized cache update

GetLatestVersion fetches data without lock (line 311), then updates cache with lock (line 330). Between these, SaveEvent could run and update cache, then GetLatestVersion overwrites it with stale data. Subsequent SaveEvent could accept duplicate versions.

TEST INCOMPLETE

Test doesn't verify cache was repopulated. Add second GetLatestVersion call to confirm caching works after invalidation.

Strengths:

• Excellent documentation
• Good test coverage
• Clear comments

These issues must be fixed before merge.

## Code Review: Changes Requested Thanks for the PR! I found critical issues in the cache invalidation logic. **CRITICAL BUG: Cache not repopulated after invalidation (lines 327-337)** In GetLatestVersion(), when cache is detected as stale, you delete it but never update it with the fresh data just fetched. The cache should be repopulated immediately: Current code deletes cache: delete(jes.versions[actorID]) Should instead: jes.versions[actorID] = latestVersion This defeats the cache purpose - next SaveEvent will miss cache again. **RACE CONDITION: Unsynchronized cache update** GetLatestVersion fetches data without lock (line 311), then updates cache with lock (line 330). Between these, SaveEvent could run and update cache, then GetLatestVersion overwrites it with stale data. Subsequent SaveEvent could accept duplicate versions. **TEST INCOMPLETE** Test doesn't verify cache was repopulated. Add second GetLatestVersion call to confirm caching works after invalidation. **Strengths:** - Excellent documentation - Good test coverage - Clear comments These issues must be fixed before merge.

HugoNijhuis commented 2026-01-13 00:28:40 +00:00

Author

Owner

Copy Link

Copy Source

Code Review: Changes Requested

Thanks for the PR! I've found a critical logic issue in the cache invalidation strategy that needs to be addressed.

Issues to fix:

1. Cache invalidation logic is incomplete in GetLatestVersion() - CRITICAL BUG

   File: store/jetstream.go, lines 327-337

   Current behavior: When GetLatestVersion detects a stale cache entry (actual > cached), it deletes the cache entry

   Problem: The cache entry is deleted but NOT repopulated with the fresh data just fetched

   Impact: After cache invalidation, the cache remains empty. On the next SaveEvent call, getLatestVersionLocked will miss the cache and have to fetch from JetStream again. This defeats the purpose of caching and the performance benefit for the single-writer case is lost.

   Fix: After computing latestVersion in GetLatestVersion(), always update the cache with the fresh value, not just delete on mismatch. The cache should reflect what we just fetched:

   jes.mu.Lock()
   jes.versions[actorID] = latestVersion
   jes.mu.Unlock()
   

2. Race condition between GetLatestVersion and SaveEvent

   GetLatestVersion does not hold the lock while fetching from JetStream (line 311: GetEvents called outside lock). SaveEvent holds the lock for its entire operation (line 150).

   Race scenario: GetLatestVersion reads version=2, gets preempted, SaveEvent runs and writes version=3, then GetLatestVersion resumes and overwrites cache with stale version=2. Subsequent SaveEvent could accept version=3 again, violating version monotonicity.

   Fix: Either (a) hold lock while fetching fresh data in GetLatestVersion, OR (b) accept that GetLatestVersion shouldn't modify the cache that SaveEvent relies on, making it read-only.

3. Test doesn't verify cache behavior after invalidation

   TestJetStreamEventStore_CacheInvalidationOnExternalWrite (lines 1328-1427)

   The test verifies detection but doesn't verify the cache was repopulated for efficiency. Add a second GetLatestVersion call to confirm cache is properly updated.

Acceptance criteria status:

• Document single-writer assumption - DONE (excellent documentation)
• Add test for external write - DONE (good coverage)
• Cache invalidation logic is correct - INCOMPLETE (cache not repopulated)
• No race conditions - INCOMPLETE (potential race in cache update)

Strengths:

• Documentation is clear and thorough
• Test covers the external write scenario
• Well-commented code
• Properly addresses issue #126

Please address these issues and I'll re-review.

## Code Review: Changes Requested Thanks for the PR! I've found a critical logic issue in the cache invalidation strategy that needs to be addressed. **Issues to fix:** 1. **Cache invalidation logic is incomplete in GetLatestVersion() - CRITICAL BUG** File: store/jetstream.go, lines 327-337 Current behavior: When GetLatestVersion detects a stale cache entry (actual > cached), it deletes the cache entry Problem: The cache entry is deleted but NOT repopulated with the fresh data just fetched Impact: After cache invalidation, the cache remains empty. On the next SaveEvent call, getLatestVersionLocked will miss the cache and have to fetch from JetStream again. This defeats the purpose of caching and the performance benefit for the single-writer case is lost. Fix: After computing latestVersion in GetLatestVersion(), always update the cache with the fresh value, not just delete on mismatch. The cache should reflect what we just fetched: ``` jes.mu.Lock() jes.versions[actorID] = latestVersion jes.mu.Unlock() ``` 2. **Race condition between GetLatestVersion and SaveEvent** GetLatestVersion does not hold the lock while fetching from JetStream (line 311: GetEvents called outside lock). SaveEvent holds the lock for its entire operation (line 150). Race scenario: GetLatestVersion reads version=2, gets preempted, SaveEvent runs and writes version=3, then GetLatestVersion resumes and overwrites cache with stale version=2. Subsequent SaveEvent could accept version=3 again, violating version monotonicity. Fix: Either (a) hold lock while fetching fresh data in GetLatestVersion, OR (b) accept that GetLatestVersion shouldn't modify the cache that SaveEvent relies on, making it read-only. 3. **Test doesn't verify cache behavior after invalidation** TestJetStreamEventStore_CacheInvalidationOnExternalWrite (lines 1328-1427) The test verifies detection but doesn't verify the cache was repopulated for efficiency. Add a second GetLatestVersion call to confirm cache is properly updated. **Acceptance criteria status:** - [x] Document single-writer assumption - DONE (excellent documentation) - [x] Add test for external write - DONE (good coverage) - [ ] Cache invalidation logic is correct - INCOMPLETE (cache not repopulated) - [ ] No race conditions - INCOMPLETE (potential race in cache update) **Strengths:** - Documentation is clear and thorough - Test covers the external write scenario - Well-commented code - Properly addresses issue #126 Please address these issues and I'll re-review.

HugoNijhuis added 1 commit 2026-01-13 00:33:19 +00:00

fix: address review feedback on cache invalidation ... fd1938672e

- Fix cache not repopulated after invalidation: Always update cache with fresh data instead of just deleting on mismatch
- Fix race condition: Hold mutex lock during entire fetch operation to prevent SaveEvent from running between fetch and cache update
- Improve test: Add second GetLatestVersion call to verify cache was properly repopulated after invalidation

Co-Authored-By: Claude Code <noreply@anthropic.com>

HugoNijhuis commented 2026-01-13 00:42:25 +00:00

Author

Owner

Copy Link

Copy Source

Code Review: Approved ✓

Excellent work addressing all the feedback! All three critical issues have been resolved correctly.

Issues Fixed:

1. Cache repopulation (FIXED) - GetLatestVersion now updates the cache with fresh data (line 335: jes.versions[actorID] = latestVersion) instead of just deleting it. This ensures the cache is immediately usable after detecting an external write, maintaining performance for the single-writer case.

2. Race condition (FIXED) - GetLatestVersion now acquires the mutex lock (line 311-312) and holds it for the entire duration of the fetch operation, preventing SaveEvent from running between the fetch and cache update. This eliminates the risk of SaveEvent updating cache while GetLatestVersion is mid-operation.

3. Test completeness (FIXED) - The test now includes a second GetLatestVersion call (lines 1393-1400) that verifies the cache was properly repopulated after the invalidation. This confirms efficiency is maintained after external writes.

Quality Assessment:

• Code quality: Clean and maintainable with clear comments explaining the cache invalidation strategy
• Thread safety: All synchronization concerns properly addressed with mutex protection
• Testing: Comprehensive test coverage including the critical external write scenario with cache verification
• Documentation: Excellent documentation in both code comments and CLAUDE.md
• Test results: All 30+ existing tests pass plus the new integration test

Design Correctness:

The implementation correctly handles all requirements:

• Single-writer case: Cache hits provide excellent performance
• External writes: Cache is detected as stale and repopulated with fresh data
• No risk of stale data being used for version validation
• Proper synchronization eliminates race conditions

Ready to merge!

## Code Review: Approved ✓ Excellent work addressing all the feedback! All three critical issues have been resolved correctly. **Issues Fixed:** 1. **Cache repopulation (FIXED)** - GetLatestVersion now updates the cache with fresh data (line 335: `jes.versions[actorID] = latestVersion`) instead of just deleting it. This ensures the cache is immediately usable after detecting an external write, maintaining performance for the single-writer case. 2. **Race condition (FIXED)** - GetLatestVersion now acquires the mutex lock (line 311-312) and holds it for the entire duration of the fetch operation, preventing SaveEvent from running between the fetch and cache update. This eliminates the risk of SaveEvent updating cache while GetLatestVersion is mid-operation. 3. **Test completeness (FIXED)** - The test now includes a second GetLatestVersion call (lines 1393-1400) that verifies the cache was properly repopulated after the invalidation. This confirms efficiency is maintained after external writes. **Quality Assessment:** - **Code quality**: Clean and maintainable with clear comments explaining the cache invalidation strategy - **Thread safety**: All synchronization concerns properly addressed with mutex protection - **Testing**: Comprehensive test coverage including the critical external write scenario with cache verification - **Documentation**: Excellent documentation in both code comments and CLAUDE.md - **Test results**: All 30+ existing tests pass plus the new integration test **Design Correctness:** The implementation correctly handles all requirements: - Single-writer case: Cache hits provide excellent performance - External writes: Cache is detected as stale and repopulated with fresh data - No risk of stale data being used for version validation - Proper synchronization eliminates race conditions Ready to merge!

HugoNijhuis merged commit 20d688f2a2 into main 2026-01-13 18:48:02 +00:00

HugoNijhuis referenced this issue from a commit 2026-01-13 18:48:04 +00:00

Merge pull request 'fix(store): Implement version cache invalidation strategy for JetStreamEventStore' (#130) from issue-126-untitled into main

HugoNijhuis commented 2026-01-13 19:25:37 +00:00

Author

Owner

Copy Link

Copy Source

Code Review: Approved ✓

Excellent work addressing the feedback! All three critical issues have been resolved correctly.

Issues Fixed:

1. Cache repopulation (FIXED) - GetLatestVersion now updates the cache with fresh data (line 335: jes.versions[actorID] = latestVersion) instead of just deleting it. This ensures the cache is immediately usable after detecting an external write, maintaining performance for the single-writer case.

2. Race condition (FIXED) - GetLatestVersion now acquires the mutex lock (line 311-312) and holds it for the entire duration of the fetch operation, preventing SaveEvent from running between the fetch and cache update. This eliminates the risk of SaveEvent updating cache while GetLatestVersion is mid-operation.

3. Test completeness (FIXED) - The test now includes a second GetLatestVersion call (lines 1393-1400) that verifies the cache was properly repopulated after the invalidation. This confirms efficiency is maintained after external writes.

Quality Assessment:

• Code quality: The implementation is clean and maintainable with clear comments explaining the cache invalidation strategy
• Thread safety: All synchronization concerns are properly addressed with mutex protection
• Testing: Comprehensive test coverage including the critical external write scenario with cache verification
• Documentation: Excellent documentation in both code comments and CLAUDE.md explaining the cache invalidation strategy
• Test results: All 30+ existing tests pass plus the new integration test

Design Notes:

The implementation correctly handles the design requirements:

• For the single-writer case: Cache hits provide excellent performance
• For external writes: Cache is detected as stale, invalidated, and repopulated with fresh data
• No risk of stale data being used for version validation

Ready to merge!

Co-Authored-By: Claude Code noreply@anthropic.com

## Code Review: Approved ✓ Excellent work addressing the feedback! All three critical issues have been resolved correctly. **Issues Fixed:** 1. **Cache repopulation (FIXED)** - GetLatestVersion now updates the cache with fresh data (line 335: `jes.versions[actorID] = latestVersion`) instead of just deleting it. This ensures the cache is immediately usable after detecting an external write, maintaining performance for the single-writer case. 2. **Race condition (FIXED)** - GetLatestVersion now acquires the mutex lock (line 311-312) and holds it for the entire duration of the fetch operation, preventing SaveEvent from running between the fetch and cache update. This eliminates the risk of SaveEvent updating cache while GetLatestVersion is mid-operation. 3. **Test completeness (FIXED)** - The test now includes a second GetLatestVersion call (lines 1393-1400) that verifies the cache was properly repopulated after the invalidation. This confirms efficiency is maintained after external writes. **Quality Assessment:** - **Code quality**: The implementation is clean and maintainable with clear comments explaining the cache invalidation strategy - **Thread safety**: All synchronization concerns are properly addressed with mutex protection - **Testing**: Comprehensive test coverage including the critical external write scenario with cache verification - **Documentation**: Excellent documentation in both code comments and CLAUDE.md explaining the cache invalidation strategy - **Test results**: All 30+ existing tests pass plus the new integration test **Design Notes:** The implementation correctly handles the design requirements: - For the single-writer case: Cache hits provide excellent performance - For external writes: Cache is detected as stale, invalidated, and repopulated with fresh data - No risk of stale data being used for version validation Ready to merge! Co-Authored-By: Claude Code <noreply@anthropic.com>

HugoNijhuis deleted branch issue-126-untitled 2026-01-13 22:47:31 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

risk/high

risk/low

risk/medium

value/critical

value/high

value/low

value/medium

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

HugoNijhuis (Hugo Nijhuis) fluxcd-bot renovate-bot

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: flowmade-one/aether#130