Repository navigation
no need to clone before setting items in cache. - #3403
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The focused configuration change is consistent with shared cache reads, and the added regression tests cover object identity and mutation risks.
No open issues 🎉
What changed in this PR
This PR removes deep cloning when storing values in the backend’s in-memory cache, reducing blocking work during cache loads and background refreshes.
Changes:
- Disables
shouldCloneBeforeSetand documents the shared-object mutation risk. - Adds tests for object identity on cache misses, hits, refreshes, and bulk loads.
- Adds regression tests checking that assignment and feature-flag requests leave cached data unchanged.
| File | Description |
|---|---|
packages/backend/test/unit/services/FeatureFlagService.test.ts |
Checks cached flags and precomputed rows remain unchanged across requests. |
packages/backend/test/unit/services/ExperimentAssignmentService.test.ts |
Checks cached segments and precomputed rows remain unchanged during assignment. |
packages/backend/test/unit/services/CacheService.test.ts |
Verifies the real memory store retains loaded objects without copying. |
packages/backend/src/api/services/CacheService.ts |
Disables cloning before cache writes and documents the rationale. |
Review effort: Balanced
| const before = snapshot(); | ||
|
|
||
| await testedModule.getAllExperimentConditions(userDoc, context, loggerMock); | ||
| await testedModule.getAllExperimentConditions(userDoc, context, loggerMock); |
There was a problem hiding this comment.
If we're making sure to have regression coverage for places where cache mutations could be added, shouldn't we also include tests for the /mark path. I only see tests for the /assign path here.
bcb37
left a comment
bcb37
left a comment
There was a problem hiding this comment.
This makes sense to me. Just one question.
|
yes that's true i can add that |
f9fc557 to
8a4540d
Compare
|
@bcb37 i added tests for mark and reward paths, which would both be implicated. |
See the comments in the code, they explain it well. This only comes up during background cache refresh, but it simply doesn't need to happen, so every bit helps flatten the long tail of latency when other requests are queuing behind during bursty periods.