Repository navigation
Closed
Description
Spun out of review discussion on #746 (#746 (comment), reported by @vladr).
Problem
Split::Cache.fetch uses truthiness to decide whether a value is cached:
value = @cache[namespace][key]
return value if value
@cache[namespace][key] = yieldSo a block that produces a falsy value is re-executed on every call — the cache is silently ineffective for exactly these cases:
| Namespace | When the block is falsy |
|---|---|
:experiment_winner |
Experiment#winner returns nil when Redis has no winner (hget miss — explicit else nil) |
:experiment_start_times |
Experiment#start_time returns nil when no start time is stored |
:experiments |
Experiment.find uses return unless Split.redis.exists?(name) inside the block — a non-local return from find, so fetch never reaches the store step at all |
Verified all three: with config.cache = true, every winner call on a winnerless experiment still issues an HGET, and every find of a missing experiment still issues an EXISTS.
Impact
- The cache does not reduce Redis traffic for the common "no winner yet" hot path, which is presumably one of the states it was meant to optimize.
- The
Experiment.findcase is a sharper edge: the non-local return silently bypasses caching, and any future refactor offetchthat assumes the block runs to completion (e.g. wrapping the store step in anensure, or the rewrite that was reverted in Mitigate stale cache across processes (related to #735) #746) changes behavior or breaks.
Why this isn't a one-line fix
Switching to a presence check (namespace_cache.key?(key)) makes nil cacheable, but that is a behavior change that interacts with the cross-process staleness issue (#735 / #746):
- Today, the
nilwinner being uncached means the "no winner → winner set" transition is never stale across processes — every call re-reads Redis. - Caching
nilwould widen the staleness window that Mitigate stale cache across processes (related to #735) #746's opt-in invalidation mechanism bounds, so the fix should land with (or after) that mechanism, and theExperiment.findblock needs restructuring so the missing-experiment result flows throughfetchinstead of non-locally returning around it.
Proposed direction
- Restructure the
Experiment.findblock to returnnilthroughfetchrather than non-locally returning. - Change
fetchto usekey?for hit detection so falsy values cache. - Ensure invalidation (
clear_keyfromwinner=,reset,reset_winner, and the Mitigate stale cache across processes (related to #735) #746 cross-process signal) covers the newly cachednilstates.
Metadata
Metadata
Assignees
Labels
No labels
Activity