refactor(cache): extract write-through get-or-load helper (#33) - #49
refactor(cache): extract write-through get-or-load helper (#33)#491Git2Clone wants to merge 4 commits into
Conversation
Three call sites (channels, guild_prompt, custom_reactions) repeat the same get-or-load flow: Redis read, DB on miss, write-back best-effort. The cache shape (set/string/hash, TTL, negative sentinel) varies per feature and stays at the call site; the helper owns only the scaffolding. Future commits migrate each call site to it.
is_ai_channel now uses the shared get-or-load helper. A confirmed DB hit (channel is registered) is written back to the Redis set, so subsequent reads stay off the DB — a minor behaviour upgrade from the prior read-only path.
get_guild_prompt now uses the shared get-or-load helper. The empty-string negative sentinel and 1800s TTL live in the call site as before; the helper handles cache read, DB fallback, write-back, and log-on-error.
…_through The cr:guilds short-circuit stays above the helper (the SISMEMBER is a different access shape than the HGETALL it gates). The HGETALL-on-miss + write-back is now in the helper. The hash rebuild on register/remove stays in place — that's a write-through-with-rebuild, not a simple get-or-load, and is exactly the carve-out the plan calls out.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Check whether a channel has AI auto-replies enabled. This runs per | ||
| /// message, so a Redis answer (hit or miss) is trusted; the DB is only | ||
| /// queried when Redis is unavailable or the call errors. | ||
| /// queried when Redis is unavailable, the call errors, or the channel isn't | ||
| /// in the set. A confirmed DB hit is written back to Redis so subsequent | ||
| /// reads stay off the DB. |
There was a problem hiding this comment.
set_contains returns Ok(false) (channel not a member), read_cache returns Ok(Some(false)), which get_or_load treats as a cache hit and returns immediately. The DB is only consulted on Redis errors or unavailability, not on a clean negative result. This was also true of the pre-refactor code.
| /// Check whether a channel has AI auto-replies enabled. This runs per | |
| /// message, so a Redis answer (hit or miss) is trusted; the DB is only | |
| /// queried when Redis is unavailable or the call errors. | |
| /// queried when Redis is unavailable, the call errors, or the channel isn't | |
| /// in the set. A confirmed DB hit is written back to Redis so subsequent | |
| /// reads stay off the DB. | |
| /// Check whether a channel has AI auto-replies enabled. This runs per | |
| /// message, so a Redis answer (hit or miss) is trusted; the DB is only | |
| /// queried when Redis is unavailable or the call errors. A confirmed DB hit | |
| /// is written back to Redis so subsequent reads stay off the DB. |
| move |c, pairs| { | ||
| let pairs = pairs.clone(); | ||
| Box::pin(async move { | ||
| for (field, value) in &pairs { | ||
| cache::hash_set(c, &meta_for_write, field, value).await?; | ||
| } | ||
| Ok(()) | ||
| }) | ||
| }, |
There was a problem hiding this comment.
The write_cache closure calls hash_set field-by-field, while the authoritative reseed_guild_cache uses pipe().atomic() (MULTI/EXEC with a leading DEL). If a concurrent register or remove triggers reseed_guild_cache between two of the individual HSET calls here, the reseed's DEL will clear the hash and rebuild it from the post-mutation DB state; then the remaining HSET calls from this write-back will silently overwrite some fields with stale pre-mutation data. The hash stays inconsistent until the next mutation re-runs reseed_guild_cache. This race only opens on the cache-error recovery path, so the window is narrow, but the fix is to either pipeline the HSETs atomically or skip the write-back on error recovery and let the next register/remove reseed.
| let key = prompt_key(guild_id); | ||
| if let Ok(Some(cached)) = cache::get_string(&mut conn, &key).await { | ||
| // An empty string is the negative-cache sentinel for "no prompt set". | ||
| return (!cached.is_empty()).then_some(cached); | ||
| } | ||
|
|
||
| // Cache miss (or read error): consult the DB and write the result back. Only | ||
| // a value the DB actually returned is cached, so a transient DB error isn't. | ||
| // A reader that races a concurrent mutation can repopulate a stale value for | ||
| // up to PROMPT_TTL_SECS; mod prompt changes are rare and the staleness is | ||
| // bounded, so that window is accepted rather than guarded with versioning. | ||
| let prompt = match GuildAiSettingsTable::fetch(pool, guild_id).await { | ||
| Ok(prompt) => prompt, | ||
| Err(e) => { | ||
| tracing::warn!(error = %e, guild_id, "Failed to fetch guild AI prompt"); | ||
| return None; | ||
| } | ||
| }; | ||
| if let Err(e) = cache::set_string_ex( | ||
| &mut conn, | ||
| &key, | ||
| prompt.as_deref().unwrap_or(""), | ||
| PROMPT_TTL_SECS, | ||
| let key_for_read = key.clone(); | ||
| let key_for_write = key.clone(); |
There was a problem hiding this comment.
key bindings is redundant — key itself can serve as key_for_read (or vice versa), saving one clone call.
| let key = prompt_key(guild_id); | |
| if let Ok(Some(cached)) = cache::get_string(&mut conn, &key).await { | |
| // An empty string is the negative-cache sentinel for "no prompt set". | |
| return (!cached.is_empty()).then_some(cached); | |
| } | |
| // Cache miss (or read error): consult the DB and write the result back. Only | |
| // a value the DB actually returned is cached, so a transient DB error isn't. | |
| // A reader that races a concurrent mutation can repopulate a stale value for | |
| // up to PROMPT_TTL_SECS; mod prompt changes are rare and the staleness is | |
| // bounded, so that window is accepted rather than guarded with versioning. | |
| let prompt = match GuildAiSettingsTable::fetch(pool, guild_id).await { | |
| Ok(prompt) => prompt, | |
| Err(e) => { | |
| tracing::warn!(error = %e, guild_id, "Failed to fetch guild AI prompt"); | |
| return None; | |
| } | |
| }; | |
| if let Err(e) = cache::set_string_ex( | |
| &mut conn, | |
| &key, | |
| prompt.as_deref().unwrap_or(""), | |
| PROMPT_TTL_SECS, | |
| let key_for_read = key.clone(); | |
| let key_for_write = key.clone(); | |
| let key_for_read = prompt_key(guild_id); | |
| let key_for_write = key_for_read.clone(); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if let Err(e) = write_cache(&mut conn, &value).await { | ||
| tracing::warn!(error = %e, "write_through: cache write-back failed"); | ||
| } | ||
|
|
||
| Ok(value) | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Both warning messages emitted by the helper — "write_through: cache read failed, falling back to DB" and "write_through: cache write-back failed" — carry only the Redis error. The call sites that used to log these paths included structured fields like guild_id (e.g. the old "Failed to cache guild AI prompt" warning). With the refactor those fields are no longer available to the helper, so a cache write-back failure in production gives no indication of which guild or feature triggered it. Consider documenting that callers relying on field-level log correlation should add a surrounding span or tracing instrument to preserve that context.
1 participant
Closes part of #33 (Pattern 1 only). Pattern 2 — the add/list/remove command scaffold dedup — is intentionally left for a separate pass; per the issue's own suggestion, /ai note (#32) can now be built on top of this helper instead of becoming a fourth copy.
What
Adds one shared helper,
cache::write_through::get_or_load, that owns the get-or-load / write-back / log-on-error scaffolding. Three call sites repeat the same flow today (DB-authoritative, write-through, optional negative-cache sentinel); they differ only in cache shape (set / string / hash), key naming, TTL, and the negative-sentinel convention — and those stay at the call site, exactly as the issue's "don't paper over the differences" rule requires.Migrated call sites
data::ai::channels::is_ai_channel— set membership, no TTL.data::ai::guild_prompt::get_guild_prompt— string with 1800s TTL, empty-string as the no-prompt sentinel.data::custom_reactions::matching— per-guild hash, with thecr:guildsSISMEMBER short-circuit kept above the helper. The MULTI/EXEC hash rebuild on register/remove (reseed_guild_cache) stays as-is — it's a write-through-with-rebuild, not a get-or-load.Helper shape
Cache hit → return; miss → load DB → write back (best-effort, logged) → return. Cache read error → logged, falls through to DB. DB error → propagates. No-Redis → loads DB directly. Negative sentinels (empty string for no prompt) are encoded by the caller as
Some("")and translated back toNoneat the boundary, keeping the helper shape-agnostic.The lifetime bound is the lightest form that compiles when closures both borrow the connection and capture locals — three call sites and the unit tests fit cleanly, so redesign isn't warranted yet. If a fourth call site ever lands, that's the signal to revisit.
Verification
cargo fmt --checkcargo clippy --all-targets -- -D warnings× {no features,--features "opentelemetry ai-openrouter"(deployed set perscripts/deploy-features.sh),--all-features}cargo test --all-features --lib→ 75 passed, 0 failedcache_hit_skips_db,cache_miss_loads_from_db_and_writes_back,db_load_failure_propagates_and_cache_is_not_written— all green against real RedisOut of scope
/reminder,/custom reaction,/custom prompt, future/ai note).