Share the bot's short-lived values through Redis (#155) - #172
Merged
Conversation
BAN_LIST_REFRESH=15m. The name says what the knob is for and the value says how long, so a knob has one name whatever unit anyone writes it in, and moving 900 to 15m is an edit to the value alone. A bare number is seconds, so every value written before this still means what it did and a rename need not touch the value in the same breath. An earlier attempt put the unit in the name instead, with a const fn reading the suffix off it while compiling. It was a misreading of what a parser parses, and it gave two places to state the unit and so one for them to disagree: a `minutes` call on a `_SECONDS` variable compiled and was sixty times off. Twenty-eight variables lose their suffix. MSG_SELFDESTRUCT_DELAY_OPTIONS_MINUTES keeps its own: it is not a span but the list of minute counts /cleanup offers a chat, stored and shown as minutes. The *_DAYS knobs that are DaysCount keep theirs for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBfwfCDtzZe2QNwzLQgdKh
The pool set only its size, so sqlx's stock thirty seconds applied. A wait there is backpressure rather than patience: queueing means the pool is empty, and waiting longer creates no connections — it only lengthens the queue behind it. DATABASE_ACQUIRE_TIMEOUT keeps that default, so reading the variable changes nothing by itself, and .env.example suggests 5s instead. The trade is that overload turns from silent slowness into refusals, which mean DATABASE_MAX_CONNECTIONS is too small rather than that the timeout is too short. Test pools get a generous sixty seconds explicitly: a test queueing there is waiting for the suite's containers to start, not for a person. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBfwfCDtzZe2QNwzLQgdKh
The store gains a local backend beside the shared one, so every tenant gets a fallback without carrying an implementation of its own. CACHE_MODE picks between the three: REDIS by default when REDIS_HOST is set, LOCAL when it isn't, and DISABLED only when an operator asks for it. Disabled and Local each carry the map, so Redis holds a connection and nothing else. The API grows with it — JSON and byte values, removal, and a lock taken with SET NX EX and freed by a script, because "delete this key if it still holds my token" cannot be one command and must not be two on a multiplexed connection. Every key begins with the bot's own prefix, applied here rather than left to each key type, so a kind of value added later cannot be the one that forgets. A server may be shared, and chat:id:-1001234:language says nothing about whose chat that is. serde_json moves to [dependencies]: the store uses it in shipped code, and the --tests checks all enable dev-dependencies, so a release build had been broken while every check passed. The test keys carry the process id. The Valkey container is reused between runs, and a key written with a minute's lifetime was still there when the suite ran again — the second run then read the first one's values. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBfwfCDtzZe2QNwzLQgdKh
The double-attack guard held a set in this process. One instance is served either way; the difference shows with a second, which sees nothing of the first's locks and accepts an attack it is already resolving. The key goes into the shared store now, taken with a single SET NX EX. The guard frees its own hold rather than the key: a handler that outran the lifetime has already lost the lock, and taking away the one that replaced it would let a third answer in while the second is still working. That check and the delete are one Lua script, since two commands can't be atomic here. PVP_LOCK_TIME is 3m, not the ceiling it looks like: the guard frees the lock as the handler ends, and the lifetime only bounds a killed process. What it must clear is the longest a handler can take, which is several Telegram requests plus four queries — an estimate read off the code, so the default leaves room. PVP_CALLBACK_LOCKS_ENABLED is gone. It made sense while a leaked guard left a battle unanswerable until a restart; the lock frees itself on a timer now, so the switch only offered a way to turn a working guard off. With nothing to switch and a store that is always there, neither the service nor its guard had a second variant to be. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBfwfCDtzZe2QNwzLQgdKh
The allowed topics, the chat-wide language and the cleanup settings each kept a map and a mutex of their own, and an admin's change was invisible to every other instance until a TTL ran out. All three read through the store now, so one write is seen everywhere at once, and the fifteen lines they had in common live in Cache::read_through instead of three times over. ChatIdKind gains `qualified`, because the keyspace is shared: a chat id and a chat instance are both signed 64-bit numbers, and Display forwards to the value, so two different chats would have shared an entry. All four chat-keyed values render through it — the bot's rights included, which had no ambiguity to fix but would otherwise have been the one key with a rendering of its own. TopicPolicy and CleanupPolicy derive Constructor: their `new` was pure field assignment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBfwfCDtzZe2QNwzLQgdKh
The user-service cache was a map with a sweeper task of its own; it is a key in the shared store now, which expires values without being asked. That takes the last background task the caches needed. A user is stored as a tag byte and its encoded message. The tag is what keeps "the service has no such user" apart from a user whose every field is the default, which prost encodes to nothing at all. The entry is this bot's own like every other, though the data behind it belongs to the service: sharing it would mean agreeing on an encoding invented here and read by nothing else. The store saves a round trip; the service itself is where bots share anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBfwfCDtzZe2QNwzLQgdKh
The dialogue states lived in teloxide's InMemStorage, so every restart dropped whatever conversation was in progress. They go into the shared store instead, through a Storage implementation of our own. Not teloxide's RedisStorage: that one brings a second Redis client with a connection pool beside our single multiplexed one, keys by the bare chat id, and sets no lifetime — an abandoned dialogue would be kept for ever. Ours keys by command and chat, expires with DIALOGUE_STATE_TIME, and falls back to this process where there is no server, which the pooled one could not. A dialogue is not a cache, so switching the store off doesn't switch it off: without somewhere to put the state, a command with a second step could never reach it. Its expiry test owns every clock it depends on, so it advances through the wait rather than living through it, and can use a lifetime of a realistic length instead of one shortened to keep the suite quick. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBfwfCDtzZe2QNwzLQgdKh
A ban is written straight into the database by the owner, so the bot could only find out when its timer came round — up to fifteen minutes of a banned user still playing. A trigger on Users notifies the `bans` channel now and the bot listens, so the ban applies at once, on every instance, without anyone typing a command. A trigger rather than a line in each of the three admin functions: their bodies would have to be repeated in the migration and kept in step for ever, and a ban applied by a plain UPDATE would still go unheard. pg_notify is delivered on commit, so a rolled-back ban is never announced. The list itself stays in memory, and had to: banned_until is a synchronous check in the update filter, so every instance needs its own copy whatever else exists. What it needed was not another place to keep the list but a way to hear that it changed, and only the database can say so. The timer stays behind it as the backstop — a notification sent while the listener is reconnecting is heard by nobody — and so does SIGHUP. The pool is built one connection larger, because a listening connection can't serve queries. subscribe() and consume() are separate because returning from the first is a moment worth having: Postgres queues that connection's notifications from there on, so the test can write the ban while nothing is polling and still expect it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBfwfCDtzZe2QNwzLQgdKh
Three new variables through the six places the checklist names — CACHE_MODE, DIALOGUE_STATE_TIME and PVP_LOCK_TIME — and PVP_CALLBACK_LOCKS_ENABLED out of them. CLAUDE.md's cache section is rewritten rather than added to: an unset REDIS_HOST now means the values are kept in this process rather than not kept, the list of tenants replaces the single one, and the two the switch may not reach are named with the reason. The paragraph promising everything else was still in process memory is gone — that was this change. The README says less than it did. It listed the commands whose values are cached, which is a list nobody will remember to extend, and walked through CACHE_MODE's three values, which is what .env.example is for. `cargo build` joins the checks to run before committing, with a note on why the --tests variants can't stand in for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UBfwfCDtzZe2QNwzLQgdKh
A server that died after a successful connect stayed on Backend::Redis forever: nothing switched it back to Backend::Local, so every call kept paying the same timeout, the battle lock kept failing open indefinitely instead of just for one blip, and a mid-flight /promo or /support dialogue was lost the moment it hit the outage rather than surviving it. Backend::Redis now carries its own RedisState — the connection, a local store, and a health flag. The first failed call trips it, and Backend::route sends every call after that straight to the local store instead of to Redis, through the exact same code Backend::Local already has. A lock taken there is a real lock; a dialogue written there is read back. Trips on the first failure rather than a few in a row: ConnectionManager already retries internally with its own growing backoff before a call returns at all, so waiting for several failures would mean paying that backoff several times over — the very wait this exists to avoid. There's no correctness reason to wait either, since the fallback is exactly what a single instance of this bot already runs correctly under CACHE_MODE=LOCAL. A background health check pings Redis every few seconds, but only while degraded and only once the fallback has actually been used — its sweeper starts lazily on that first trip, so a Redis that never fails never pays for either task. cache_fallback_active now follows every call to Redis, not only the one Cache::connect made at startup, so DickGrowerBotCacheFellBack covers a live outage too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016RUsHLHGGfJzjKPRgqUetk
RedisState::fallback's sweeper used to run for as long as the process did, once an outage started it: a lock or a dialogue kept there during the outage still needed its TTL swept, but nothing turned the sweeper off again once Redis came back and the fallback was empty for good. RedisHealth now times how long Redis has been answering since the last change either way, and spawn_redis_health_check stops the sweeper — not for good, just until the next outage restarts it through Backend::route — once that stretch passes REDIS_FALLBACK_IDLE_TIMEOUT. The abort takes the fallback's sweeper lock before re-checking degraded, not after: a failure landing in that gap must still find a sweeper running, or a degraded backend could be left with none until the next failure restarts one. The idle timeout is a parameter of spawn_redis_health_check rather than the constant read directly, so a test can ask for the teardown without waiting minutes for it. The six spots recovering a poisoned mutex by hand collapse into one lock() helper. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016RUsHLHGGfJzjKPRgqUetk
Backend::route stops reading RedisState::fallback the instant Redis recovers, so anything the outage wrote there — a lock, a mid-flight /promo or /support dialogue — used to vanish at that exact moment rather than surviving it. The health check's first successful probe now drains the fallback and writes each live entry into Redis with whatever is left of its own lifetime, through sync_fallback_to_redis. Every entry goes in a single pipeline rather than one round trip per key — not .atomic(), so it isn't MULTI/EXEC and carries none of that command's risk on a multiplexed connection (see UNLOCK), just one write and one read for however many entries the outage left behind. A write that fails is logged and the whole batch given up on, the same as every other failure in this file, not retried; for a lock that's free, for a dialogue it reads as if a restart had just happened. LocalStore::drain_live returns a DrainedEntry per entry instead of a bare tuple, so the sync loop reads by field name rather than position. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016RUsHLHGGfJzjKPRgqUetk
`Duration::from_secs(N * 60)` and its hour/day cousins said the same thing the duration macros already say by name — the multiplication was just the code spelling out "minutes" instead of writing it. Duration::from_mins/from_hours say it directly, so the literal constants, the config/env.rs unit helpers, and every test value that was actually counting minutes or hours now read that way instead. Two computed conversions stay as from_secs, because they aren't a literal number of minutes: self_destruction's reading-speed formula multiplies then divides in one expression, and dividing first (as from_mins would need) truncates a fractional minute before it ever reaches the result — 1500 chars at 1000 cpm should read 90 seconds, not the 60 that flooring char_count / cpm first would give. Also merges two pairs of cache.rs tests that each spun up a Valkey container of their own: a_dead_server_fails_a_call_instead_of_hanging_it tested a strict prefix of what a_sustained_outage_falls_back_to_a_working_store_and_recovers_ on_its_own already does as its first step, and the sync/idle-sweeper tests never pause or stop their container, so nothing stops them sharing one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016RUsHLHGGfJzjKPRgqUetk
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
src/cache.rs) for everything the bot used to keep in separate maps: the per-chat settings, fetched users, PVP battle locks, and half-finished/promo//supportdialogues — shared through Redis whenREDIS_HOSTis set, kept in-process otherwise, withCACHE_MODEas the explicit override.LISTEN/NOTIFY(migration 40) instead of polling, so a ban applies within about a second.BAN_LIST_REFRESH=15m) instead of the variable name.Test plan
cargo build && cargo clippy --tests && cargo test(302 tests, including the cache module's outage/recovery integration tests against a real Valkey container)🤖 Generated with Claude Code
https://claude.ai/code/session_016RUsHLHGGfJzjKPRgqUetk