Skip to content

Split::Cache never caches falsy values (nil winner / start_time, missing experiment) #762

Description

@snaka

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] = yield

So 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.find case is a sharper edge: the non-local return silently bypasses caching, and any future refactor of fetch that assumes the block runs to completion (e.g. wrapping the store step in an ensure, 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 nil winner being uncached means the "no winner → winner set" transition is never stale across processes — every call re-reads Redis.
  • Caching nil would 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 the Experiment.find block needs restructuring so the missing-experiment result flows through fetch instead of non-locally returning around it.

Proposed direction

  1. Restructure the Experiment.find block to return nil through fetch rather than non-locally returning.
  2. Change fetch to use key? for hit detection so falsy values cache.
  3. Ensure invalidation (clear_key from winner=, reset, reset_winner, and the Mitigate stale cache across processes (related to #735) #746 cross-process signal) covers the newly cached nil states.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions