feat: improve sticky bucketing#167
Conversation
madhuchavva
left a comment
madhuchavva
left a comment
There was a problem hiding this comment.
@vazarkevych Thoughtful contribution on providing File-based storage. But it involves a lot of risks that are needed to be taken care such as Concurrency bottlenecks (read-modify-write), Performance issues (full file read/write per operation) and Corruption risks (if the process crashes midway, the data is lost and the file isn't readable anymore).
On top of it, This PR replaces the current synchronous behavior for StickyBucketingService interface with async methods, which breaks the existing/custom implementations and concurrent access to a shared file needs synchronization.
Can you provide more details about this feature request?
Thank you for the review! 1. Contract Violation - resolvedChanging Fix: 2. File Storage Risks & ConcurrencyI improved ✔️ Concurrency To avoid race conditions during file-level "read-modify-write" operations, I added key-based locking using a ✔️ Safety & Integrity The implementation now operates within a 3. 💡 Feature Request BackgroundThis request originated from community needs analysis, particularly similar to the Swift SDK (Issue #116), where asynchronous support for Although in this PR we reverted to a synchronous interface for compatibility, the |
vazarkevych
changed the title
…ded concurrency) Address the reliability/security risks of persisting sticky bucket assignments to the filesystem: FileCachingManagerImpl - Atomic writes: write to a temp file then ATOMIC_MOVE over the target (fallback to REPLACE_EXISTING), so a crash mid-write can no longer leave a corrupted / half-written cache entry. Benefits the feature cache too. - Explicit UTF-8 for read and write (was platform default); read whole file so content round-trips exactly. FileStickyBucketServiceImpl - Path-traversal safe: the user-controlled attribute value is no longer used as a file name. The logical key is hashed with SHA-256, giving a fixed-length hex name that cannot escape the cache dir or collide on separator chars. - Bounded concurrency: striped ReadWriteLocks (fixed count) instead of an unbounded per-key lock map that grew with the number of distinct users. - Read latency: a bounded (LRU) in-memory cache fronts the store so repeated evaluations for the same user avoid disk I/O, with constant memory use. Tests - New FileStickyBucketServiceImplHardeningTest: cross-instance persistence, path-traversal safety, concurrent distinct-key and same-key access, and graceful handling of corrupted entries. - Update FileCachingManagerImplTest write-failure case to a deterministic, permission-independent scenario consistent with atomic writes.
- Centralize the "attributeName||attributeValue" document key format in
StickyAssignmentsDocument (KEY_SEPARATOR + key()/getKey()); remove the four
duplicated/hardcoded "||" usages across both service impls and GrowthBookUtils.
- Extract AbstractStickyBucketService with the shared getAllAssignments so the
in-memory and file backends only implement getAssignments/saveAssignments.
- Add StickyBucketVariation.notFound()/blocked()/found() factories (+ NOT_FOUND
constant) and use them in getStickyBucketVariation.
- Rewrite GrowthBookUtils.getStickyBucketAssignments for readability (named
locals, no duplicate map lookups) while preserving exact behavior.
- Guard deriveStickyBucketIdentifierAttributes against a missing "features" key
(was an NPE on jsonObject.get("features").toString()).
- Tidy StickyBucketService / model javadoc.
2 participants
In this pr we: