Commit Graph
7 Commits
Author SHA1 Message Date
Claude Code 84fe185285 fix: address review feedback on error handling
- Handle json.Marshal error in deepCopyEvent instead of silently discarding
- Handle json.Unmarshal error instead of silently discarding
- Add explicit error checks with panic (appropriate for internal marshaling errors)
- Document that panics signal impossible conditions for valid Event structs

Co-Authored-By: Claude Code <noreply@anthropic.com>
2026-01-13 22:24:35 +01:00
Claude Code 69da1d800e docs: Verify and document append-only event guarantees
CI / build (pull_request) Successful in 21s
CI / integration (pull_request) Failing after 2m0s
This commit addresses issue #60 by documenting and enforcing the immutability
guarantees of the event store:

- Document that EventStore interface is append-only by design (no Update/Delete methods)
- Document the immutable nature of events once persisted as an audit trail
- Add JetStream stream retention policy configuration documentation
- Add comprehensive immutability test (TestEventImmutability_InMemory, TestEventImmutability_Sequential)
- Enhance InMemoryEventStore to deep-copy events, preventing accidental mutations
- Update README with detailed immutability guarantees and audit trail benefits

The EventStore interface intentionally provides no methods to modify or delete
events. Once persisted, events are immutable facts that serve as a tamper-proof
audit trail. This design ensures compliance, debugging, and historical analysis.

Acceptance criteria met:
- EventStore interface documented as append-only (event.go)
- JetStream retention policy configuration documented (store/jetstream.go)
- Test verifying events cannot be modified after persistence (store/immutability_test.go)
- README documents immutability guarantees (README.md)

Closes #60

Co-Authored-By: Claude Code <noreply@anthropic.com>
2026-01-13 21:26:16 +01:00
Claude Code de30e1ef1b fix: address critical TOCTOU race condition and error handling inconsistencies
CI / build (pull_request) Successful in 23s
CI / integration (pull_request) Failing after 2m1s
- Fix TOCTOU race condition in SaveEvent by holding the lock throughout entire version validation and publish operation
- Add getLatestVersionLocked helper method to prevent race window where multiple concurrent threads read the same currentVersion
- Fix GetLatestSnapshot to return error when no snapshot exists (not nil), distinguishing "not created" from "error occurred"
- The concurrent version conflict test now passes with exactly 1 success and 49 conflicts instead of 50 successes

These changes ensure thread-safe optimistic concurrency control and consistent error handling semantics.

Co-Authored-By: Claude Code <noreply@anthropic.com>
2026-01-13 19:49:37 +01:00
Claude Code b9e641c2aa fix: Address thread safety and resource management issues
- Fix thread safety issue in SaveEvent: Lock now only protects cache access. NATS I/O operations (GetLatestVersion calls) happen without holding the mutex, preventing lock contention when multiple concurrent SaveEvent calls occur.

- Improve cache handling: Check cache first with minimal lock hold time. For cache misses, unlock before calling GetLatestVersion, then re-lock only to update cache.

- Remove getLatestVersionLocked: No longer needed now that SaveEvent doesn't hold lock during GetLatestVersion calls.

- Fix error handling consistency: GetLatestSnapshot now returns (nil, nil) when no snapshot exists, consistent with GetLatestVersion returning 0 for no events. Both methods now treat empty results as normal cases rather than errors.

- Fix benchmark test: BenchmarkGetLatestVersion_NoCache now creates uncachedStore outside the timing loop. Previously, creating a new store on each iteration was too expensive and didn't properly measure GetLatestVersion performance.

Co-Authored-By: Claude Code <noreply@anthropic.com>
2026-01-13 19:49:37 +01:00
Claude Code ec3db5668f perf: Optimize GetLatestVersion to O(1) using JetStream DeliverLast
Closes #127

The GetLatestVersion method previously fetched all events for an actor to find
the maximum version, resulting in O(n) performance. This implementation replaces
the full scan with JetStream's DeliverLast() consumer option, which efficiently
retrieves only the last message without scanning all events.

Performance improvements:
- Uncached lookups: ~1.4ms regardless of event count (constant time)
- Cached lookups: ~630ns (very fast in-memory access)
- Memory usage: Same 557KB allocated regardless of event count
- Works correctly with cache invalidation

The change is backward compatible:
- Cache in getLatestVersionLocked continues to provide O(1) performance
- SaveEvent remains correct with version conflict detection
- All existing tests pass without modification
- Benchmark tests verify O(1) behavior

Co-Authored-By: Claude Code <noreply@anthropic.com>
2026-01-13 19:49:37 +01:00
Claude Code fd1938672e fix: address review feedback on cache invalidation
CI / build (pull_request) Successful in 19s
CI / integration (pull_request) Failing after 2m0s
- 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>
2026-01-13 01:31:03 +01:00
Claude Code 6de897ef60 fix(store): Implement version cache invalidation strategy for JetStreamEventStore
CI / build (pull_request) Successful in 19s
CI / integration (pull_request) Failing after 2m0s
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>
2026-01-13 00:24:27 +01:00