Keep the self-destruction of messages in the database - #162
Merged
Conversation
The `literal` constructor of a domain type is a `const fn`, but Rust evaluates a `const fn` early only where the language requires it. Called as an ordinary function it ran its validator when the line was reached, if ever, so about 140 call sites had no build-time check at all -- the very thing the constructor was named for. `literal!(Ratio = 0.5)` puts the call into a `const` block, which forces the check to happen during the build. `clippy.toml` forbids the bare constructor for every domain number, and the macro carries the one `#[allow]` that lets itself through. That list can fail in two silent ways: a path that resolves to nothing is ignored, and there are no globs, so a new type stays unprotected until someone adds it. `src/domain/primitives/literal.rs` compares the list with the types that are declared and fails the tests when the two disagree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRfZSeMWrrMWZTri8BoFp6
The proof of concept spawned a task per message, so a restart lost every deletion that was still waiting, and only the two short-lived groups were ever scheduled. Now `SelfDestructionService` writes rows into `Scheduled_Message_Deletions` (migration 37) and a worker claims what is due, so nothing is lost and an `Application` may wait for hours. The claim leases its batch: one UPDATE pushes `fire_after` forward, because the row's lock only lives as long as that statement while the requests take much longer. A worker that dies mid-batch leaves its messages to be claimed again. No ending deletes a row. `removed`, `removed_before`, `expired` and `failed` stay in the table until a separate cleaner takes them away, so the whole account of what the worker did can be read from the table itself. Also here: * the command behind an answer is deleted with it, when the bot may (`Chats.is_bot_admin`, migration 38, and `MSG_SELFDESTRUCT_MODE`); * an inline message can never be deleted, only edited, so it is replaced with a placeholder -- and only for the groups `MSG_SELFDESTRUCT_INLINE_GROUPS` names; * both schedulers share one `Throttle`, since it counts requests in a worker of its own and two of them would allow twice as much. Its limits are settings now (`THROTTLE_*`), set below Telegram's own so the answers to users keep room; * `RetryAfter` gets its own error kind, `rate_limited`. It used to fall in with every other API answer, which is why hitting Telegram's limits could not be told from sending something malformed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRfZSeMWrrMWZTri8BoFp6
A count of something, a page, a bet, an id -- none of them can be negative, and each said so with a validator over a signed inner type. That check cost a `Result` at every construction and at every arithmetic operation, and it existed only because the inner type could hold a value the domain could not. An unsigned type makes that value unrepresentable, so the checking disappears instead of moving somewhere else. The width is kept, since dropping the sign bit doubles the positive range rather than halving the need for bits. Postgres has no unsigned column, so the macro stores such a type in the signed integer of the same width: u16 in an int2, u32 in an int4, u64 in an int8. Only u8 widens, as there is no one-byte integer. Both directions convert instead of casting, and a refusal is only possible for a value the column could not have held anyway. That also unblocks u64, which had no encoding at all before and made `Position` and `AffectedRows` Rust-only values. What the call sites gain: `UserId::from(TeloxideUserId)` stops needing an `expect`, and so do the battle payout and the bet-to-length conversion. Where a value comes from a user, the refusal moves into `parse`, which is where it belongs. `(page - 1).expect(...)` becomes `page - 1` -- saturating, which is safer than the panic it replaces. `TelegramChatId` stays signed: a group's id is negative, and a supergroup's begins with -100. `AccessHash` too -- it uses the full 64-bit range. `Offset::calculate` needed a real fix. Its comment claimed the product of two i16 always fits an i32, which held only while both were signed; two u16 multiply out to twice what an i32 holds. It now multiplies in i64 and cuts the result down. `PositiveLength` is gone -- declared, forbidden in clippy.toml, used nowhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRfZSeMWrrMWZTri8BoFp6
The error handler printed the error with `{:?}`, which spells an anyhow
chain across several lines. VictoriaLogs stores a record per line, so one
failure arrived as several records, and only the first of them carried the
context and the trace id; the rest were loose strings.
`chain` walks `source()` and joins everything with ": ", the way anyhow's
`{:#}` does. The same reason applies to the context `reply_html!` attaches:
the `{:?}` of a whole Message is a huge multi-line dump, while the two ids
are all that is needed to find it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Chats.is_bot_admin` opened one way. It was filled from one `getChatMember`
and set to `false` by a refused deletion, and nothing ever wrote `true` back,
so a chat where the bot had once been a plain member kept `false` for ever.
In ONLY_WITH_COMMAND that switched the whole feature off there, and only an
UPDATE by hand could undo it.
Two changes replace the column, and they cover each other.
Telegram sends a `my_chat_member` update whenever the bot's own status
somewhere changes, and it is in the default set, so `handlers::rights` gets
the answer without asking for it. That is the authoritative signal.
The value then lives in Redis with a TTL, which is only the safety net for a
change missed while the bot was down. This is the first tenant of the cache;
the four in-memory ones follow separately.
The check also stopped being unconditional, because the modes pay different
prices for a wrong guess. ENABLED now guesses "yes" and finds out by trying:
a refusal costs one request, marks the row `failed` and teaches the cache,
and nobody sees it, so that mode never calls getChatMember at all.
ONLY_WITH_COMMAND cannot guess — the answer is deleted before its command,
so a refusal would leave the command alone in the chat, which is the outcome
the mode exists to prevent — and it is the only caller left.
The cache is optional in every direction: no REDIS_HOST, an unreachable
server or a failed command all mean "nothing known", and the callers do the
work a hit would have saved. `redis` with `ConnectionManager` and no pool,
since Redis runs commands one at a time and multiplexes instead. The
container runs Valkey, behind a Compose profile of its own.
Migration 38 is deleted rather than undone: it only ever existed on this
branch. A database that has it applied needs one statement before the bot
will start again, because sqlx refuses to run when an applied migration has
no file:
DELETE FROM _sqlx_migrations WHERE version = 38;
ALTER TABLE Chats DROP COLUMN IF EXISTS is_bot_admin;
The test containers needed distinct label values as well. A reusable
container is matched by its labels, so the cache request was handed the
Postgres container and failed on a port that wasn't there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A domain wrapper proves a `Count` is not a `Length`, and a `Ratio` is in
0..=1 because its constructor refused everything else. It cannot prove a
runtime `u64` fits an `i64` — that is a statement about a value, and Rust
has no refinement types for it. So at every boundary to a foreign API the
wrapper ends and a conversion begins, and those were spelled `as`, the one
cast that wraps silently.
`From` is the wrong home for it: `From`/`Into` promise the value survives,
which is why std ships `TryFrom<u64> for i64` and no `From`. It would also
hand out `.into()`, so the loss would land at call sites that never named
it. And a blanket `unwrap_or(MAX)` is wrong for a signed source —
`u32::try_from(-5i64)` would give 4294967295 instead of 0.
Two traits instead, each named for what it does, so the three things `as`
means are told apart where they happen:
exact From / Into (std)
out of range SaturatingInto — stops at the nearer end
not representable ApproxInto — the nearest number that fits
...and then `as` itself is denied, in a `[workspace.lints.clippy]` all three
crates inherit. Five lints, and the last is the point: `cast_lossless` is
what makes an exact conversion say `From` rather than reach for one of the
other two.
The traits and the denial are one change, which is why they are one commit.
Without the denial nothing stops the next `as`, and `ApproxInto` in
particular changes no behaviour on its own — the float directions of `as`
already do the right thing (RFC 2484). Its job is to keep a denied cast from
being holed by an `#[allow]`: a line in perks.rs carries an int-to-float
cast next to a float-to-int one, and with no name for the first, the only
way to compile it would be an allow covering both.
Only int-to-int changes what the program does; there `as` wraps and the
trait clamps. Two call sites had already grown their own clamp by hand —
`value.min(i64::MAX as u64) as i64` and `clamp(0, u32::MAX as i64) as u32` —
and now say it in one word. Where the sink is ours, `Gauge::set` takes the
trait, so its callers hand over the domain value itself.
The macro's generated division went through `as` too, where the denial
cannot reach: a proc macro's spans are exempt. It names its conversion now,
which does not fix the precision the TODO there describes but stops it being
invisible.
Three casts survive, none of them flagged: `group as u8` is an enum
discriminant, `chat_id.0 as i64` is sqlx's type-override syntax rather than
a cast, and `UID as u64` sits inside `literal!`, so the build evaluates it.
Closes #161.
`literal!` exists to force a validator to run while the code is compiled. Three domain types have one — Ratio, Percentage and FloatPercentage, all in ratio.rs. The other 28 lost theirs when the positive numbers became unsigned (4031f55), which put the constraint in the type instead. For those the macro was generating pub const fn new(value: T) -> Self { Self(value) } pub const fn literal(value: T) -> Self { Self(value) } so `literal!` wrapped a `const` block around a function with nothing in it to evaluate. 74 of the 122 call sites checked nothing, and the machinery kept for them was not free: a 31-entry clippy list whose mistakes are silent, and the guard test in literal.rs written to compensate for that silence. `literal` is now generated for validated types only. The list is down to three entries and the guard reads validators rather than newtypes. Nothing has to be remembered at the call sites, because the compiler decides: a validated `new` returns a Result and won't compile where the value itself is wanted, and an unvalidated type has no `literal` to reach for. So `Ratio` keeps `literal!(Ratio = 0.5)` and everything else says `Limit::new(10)`. `env_value!` built its default and lower bound through `literal!`, and now uses the type's `new`. Every bound in the config is on an unvalidated type; a validated one would fail to compile there, which is the signal to pass it in already built. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`#[domain_type(validated(...))]` on a `String` parsed, accepted the
validator and then silently dropped it: the validator came out as dead
code and nothing warned. Strings had no `literal` either, on the reasoning
that a `String` cannot be built in a `const` context.
That reasoning was about construction, and only the *validation* has to be
const. `const fn(&str) -> bool` is perfectly legal, so the two halves are
split:
literal!(T = v) -> T::from_literal(const { T::check_literal(v) })
`check_literal` asserts and hands the value back; `from_literal` builds it
afterwards. The const block is what forces the check, so `from_literal` on
its own skips it and `clippy.toml` forbids it — the list now names that
method instead of `literal`.
The shape is uniform: numbers use it too, so there is one expansion rather
than a special case, and no call site changed. A string validator takes
`&str` rather than `&String`, which means one function serves both the
literals in the source and the values arriving at runtime.
`PromoCode` is the consumer. Its rule was a `Lazy<Regex>` in the handler
that only ever guarded the *inline* path, so a malformed code typed into
/promo went to the database and came back as "not found". It is a const fn
now, matching UTF-8 byte pairs directly since `chars()` is not const, and
the type refuses the code before any query. The answer a user sees is
unchanged. `promo_inline_filter` asks the type instead of keeping a second
copy of the rule, and the test constants became `literal!`, so a typo in
one fails the build.
Username was tried and dropped. Every call site is a display path that
cannot sensibly refuse, so they would all have taken a truncating
constructor while the plain `new` sat there returning a Result — a panic
waiting for someone to `.expect()` it on the /top rendering path. A rule
earns its place where a refusal means something.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The share of a loan paid back was a `Ratio(f64)`, but f32 in both places it leaves the process: the `payout_ratio` column is a `real`, and the confirmation button carries the value through a callback string. Each boundary converted, and the comparison in the callback handler had to narrow the configured value by hand just to match what came back from a round trip through f32. `PayoutRatio(f32)` removes all three conversions. Precision was never the question — every consumer of a ratio here collapses to an integer percent or renders two decimals, and f32 keeps seven digits — so the type follows the storage rather than the other way round. `Ratio` stays f64 because its two query parameters are inferred as `double precision`, which is a Postgres matter and not a precision one. The three things both coefficients do — scale a magnitude, become a Percentage, become a FloatPercentage — move into a `Coefficient` trait with default bodies. They differ in one thing, the width they are kept at, so that is the only thing each implementor says. `From` stays as the spelling for the percentage conversions, now delegating. The callback data carried two bare primitives; both are domain types now. `Debt` moves to `u64`, which is what the column's `CHECK (debt >= 0)` has always said, and what keeps the parser rejecting a negative amount in a hand-crafted callback string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`cache.rs` knew two things at once: how to talk to Redis, and what a bot's administrator right in a chat is. The second belongs to whoever owns that concept, not to the component that stores it. It now offers `get_flag`/`set_flag` over a `CacheKey`, and nothing else. A key is declared by the module that owns the value it names, which is `handlers::rights` for this one — the key type, the two accessors and the `bot_admin_lookup_total` counter all moved there, next to the handler that already records the right. Adding a cached value costs `cache.rs` nothing, which is the point with six more caches to move (#155). The key is a type rather than a `format!` at each call site, and the trait is bound on `Display` rather than `Into<String>` so a bare string can't pass for a key. Not a `#[domain_type]`: that macro is for numbers and strings and would generate arithmetic, sqlx and serde impls a key has no use for. Two things found on the way. `#[domain_type]` never carried the struct's own attributes into the code it emits, so every doc comment written above one was silently dropped — including the one on `PayoutRatio`. `#[display]` still can't be written there, being a derive helper the macro has yet to introduce, so the fix is for the documentation. Private helpers now sit near the bottom of a file, under what they serve; the convention is written down in CLAUDE.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three modules started containers, each with its own copy of the scaffolding: the repository tests, the cache tests and the observability test. Roughly ninety lines between them, and the copies had drifted. `src/test_containers.rs` holds a `SharedContainer` declared as a `static` next to the tests that need it, plus the runtime everything shared lives on. Line count is not the win — the label discipline is. A reusable container is matched by its labels, so two of them labelled alike hand the second caller the first container and fail on a port that isn't there. That was prose in one module and broken in another: the observability container was labelled `true`, a leftover naming no service. The service name is a constructor argument now, so it can't be forgotten. Three more things fall out. `TEST_CONTAINER_LABEL` lived in `repo::test`, so the cache's tests reached into the repository's test module for it. The runtime's rationale was written out twice, once as "see repo::test for the full reasoning". And the observability container was never marked reusable, paying its startup on every run for no reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five things, all found by reading the code back. `ApproxInto` was implemented for every integer-to-float pair, including the ones that lose nothing: an `f64` holds every `i32` and every `u32`, and std has a `From` for exactly those. So `count.approx_into()` claimed a loss that could not happen — the same lie `cast_lossless` forbids for `as`. Only the pairs that can actually lose something are left, which makes the compiler steer the rest to `From`. `#[domain_type]` picks between the two in the division it generates, since it knows the width it wraps. `Histogram::observe` goes back to taking an `f64`. Widening it to `impl ApproxInto<f64>` had been the right move for `Gauge::set`, where every caller converts; here almost every caller already holds a float, and the one that counts things now names its own narrowing. `literal!` had been copied into the macro crate's tests rather than used from where it lives. It moves to `domain_types`, next to the traits it works with, and both crates use that one. Not to the macro crate: a `proc-macro = true` crate cannot export anything but proc macros. The promo-code rule is written on characters now — the alphabet reads like the regular expression it replaced — with the walk over UTF-8 beside it, decoding no more than the alphabet can contain. A malformed promo code gets its own answer instead of borrowing the one for an unknown code, and is logged. Being told the format is wrong beats being sent to look for a code that never existed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ten of them, none doing anything. `#[inline]` exists so that a function can be inlined *across a crate boundary*: without it, a non-generic function is not offered to the other crate's optimiser. Everything marked here is called from inside this crate, where LLVM decides on its own and the attribute adds only a nudge it did not need. Eight sat on dispatcher filters, where it could not have worked in any case: dptree keeps handlers behind `Box<dyn ...>`, and there is nothing to inline through that. The other two are a connection opened once at startup and a helper in the tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nine call sites converted with `saturating_into` where nothing could be cut:
`Limit` is a `u16`, so `u16 -> usize` and `u16 -> i64` fit by construction,
and so does `DaysCount`'s `u32 -> i64`. The name promised a clamp that the
types ruled out — the same thing `ApproxInto` was doing for the exact
integer-to-float pairs, one axis over.
One fact settles both halves: for each width, std's `From` impls say which
widenings are exact. `SaturatingInto` between integers now keeps the
complement — every narrowing, every signed-to-unsigned, and `usize`/`isize`
as a source, neither having a portable `From` to a fixed width.
`#[domain_type]` generates `From<#name> for T` for the exact ones. It
already emitted `From<#name> for #inner_type`, so the shape was proven: the
orphan rule is satisfied by the local type sitting in the trait's parameter,
whatever the target is. That is also what verifies the table, since a target
std cannot convert fails to compile — and it takes the domain type itself,
so the call sites read better than before the traits existed:
dicks.len() > usize::from(config.top_limit)
The other half is not self-checking: a pair wrongly left in the list would
compile and go on lying. So the pairs left out are named in a test that
converts each with the `From` that is the reason it was left out. Writing it
caught one: `isize` has a `From<u8>` but no `From<u16>`.
`Gauge::set` narrows back to `i64`, since a sink bounded on the trait can no
longer take an exact caller — the same reason `Histogram::observe` did.
Nothing moves: every one of the nine was exact, so `From` computes the same
number.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ten constants for six variables, and two hand-written readers, where the rest of the config spells the key at the point it is read and takes what it needs from `config::env`. The durations are `Duration` now rather than a `u64` of seconds. Every one of the five consumers was calling `Duration::from_secs` on arrival, so the type follows what they wanted all along, and `EnvDuration` reads them like every other interval in the config. The `.max(1)` that guarded the sweeper's interval from a busy loop moves to `at_least(1)`, next to the value it bounds instead of at the far end of the program. `WEBHOOK_URL` and `GRPC_ADDR_USER_SERVICE` go through `get_optional_env_string`, which is the "set and not empty" rule they were each spelling out. So did `read_optional_secs` in bot.rs, a third copy. One behaviour changes, and only in a case that cannot really happen: a webhook address that is not valid UTF-8 now disables the webhook instead of stopping the bot. An address that *is* text but not a URL still stops it, which is the mistake someone might actually make. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`IntegrationsConfig` says what it is for in its own doc — configuration for connections to external services — and then carried two lifetimes that reach no service at all. Its own field doc gave the game away: the chat-wide language "works even when that integration is disabled. We own this data, it lives in our own database." They were there because they are read next to the user-service, not because they belong with it. `CachesConfig` holds them now, a sub-config of `AppConfig` next to the other groups. `ban_list_refresh_secs` joins them, being the same kind of knob and having sat alone in the flat list; its name stops lying about being a count of seconds while holding a `Duration`. That set is not arbitrary — it is precisely the caches #155 lists for moving into Redis, so the group names what migrates. The consumers now ask for what they use. `TopicPolicy::new` took the whole integrations config to read one `Duration` out of it, and was handed a gRPC address to get there; it takes the lifetime. What is left of `IntegrationsConfig` is a webhook address and a service to dial. No knob moved: the same four variables, the same defaults, the same lower bound on the ban-list refresh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing said whether the language tally was earning its keep. It costs one
batch request to the user-service per broadcast chat per day, and if it
never decides anything the bot pays that round trip and falls back to
English regardless — which no metric could show.
`broadcast_language_total{source}` records the decision where it is made:
`chat` when the chat has a language of its own, `tally` when the players'
own languages picked one, `default` when neither did. All three are exported
from zero, because a share that is missing looks the same as one that is
zero, and zero is exactly the answer worth seeing here.
The batch request stops feeding `user_service_get_total`. That counter is
about resolving one user, cache or wire, and a batch was landing in the same
series while meaning something else: forty users counted as one, exactly
like a single lookup. It has a histogram of its own now,
`user_service_languages_batch_size`, which is also the breakdown by how many
uids a request carried.
That is only the caller's half. How long the query took and how much it
returned are the user-service's to report; what cannot be seen from there is
how often this bot asks and how big it asks, including the calls that never
arrive.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The worker took its batch one message at a time, awaiting a round trip to
Telegram and then a row write before starting the next. What a run got
through was therefore one message per round trip, however large the batch —
so `MSG_SELFDESTRUCT_BATCH_SIZE` could not raise throughput at all, only
lengthen the run. The tuning table in CLAUDE.md said otherwise and was
wrong.
`MSG_SELFDESTRUCT_CONCURRENCY` (8) is the knob that does raise it. The batch
size keeps its own job: bounding how much one run claims, and with it how
long the lease must cover.
What made the sequential loop look reasonable is that it was the only thing
bounding the request rate — and not by design. `Throttle` does not touch
this worker: teloxide throttles the message-sending methods and passes
`delete_message` and `edit_message_text` straight through. That is a
judgement, not an oversight, and the documented limits agree with it: 30 a
second and one a second per chat are about sending, and a deletion produces
neither a message nor a notification. CLAUDE.md claimed the shared throttle
covered both schedulers; it covers the shrink.
If the judgement is wrong, nothing breaks quietly. A 429 is not a final
error — the row is postponed with the usual backoff and tried again — and it
is already counted as
`telegram_request_errors_total{kind="rate_limited"}`, with a panel.
An answer and the command behind it come due at the same instant, and the
order `RETURNING` hands them back is not the order the claim asked for. They
now go out in the same wave instead of a round trip apart.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A message someone else had removed went `created -> warned -> removed_before`: the warning's edit failed, the row was marked warned anyway, and the message was found missing again a grace period later. Two requests and a wasted wait for a message that was never coming back — plus a warn-level line for what CLAUDE.md itself describes as normal in a chat where a human or another bot does the cleaning. The error arm meant transient failures, where carrying on is right: the notice is what a failed edit costs, not the deletion. "Gone" is not transient, and `is_gone` already knew `MessageToEditNotFound` — it was just never asked on that path. The decision moves out of `act` into `outcome_of_warning`, which needs no `Bot` and so can be tested, like the three predicates beside it. Both outcomes are covered: every wording of gone ends the row, while a rate limit, an unknown error and success alike still lead to a deletion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`REDIS_CACHE_TTL_SECS` was one number for everything the cache would ever hold. That is wrong on its face — a chat's language and a PVP lock have nothing in common but the store they sit in — and it contradicted the config beside it, where the in-memory caches already have a lifetime each. Moving them into Redis under #155 would have flattened those into one. A lifetime belongs to the value, so `set_flag` takes it with each write and `cache.rs` keeps none. `RedisConfig` is now where to reach the server and nothing else. `CachesConfig` gains `bot_admin`, next to the lifetimes already there. Its doc no longer says "in memory": where a value is kept is a separate question from how long it may be stale, and keeping the two apart means #155 moves code without moving configuration. `BOT_ADMIN_CACHE_TIME_SECS` replaces the old variable, and an hour replaces six. Six was far too long for a value the bot is told about: a `my_chat_member` update writes it the moment it changes, so the lifetime only bounds a change missed while the bot was down. Nothing to migrate: `REDIS_CACHE_TTL_SECS` has never been deployed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MSG_SELFDESTRUCT_TABLE_CLEANING_DELAY reads as seconds and holds minutes, and it was not alone: five variables carried no unit at all, all of them minutes. A mistake of sixty times in a deletion delay only shows up in production. So every Duration variable now ends in its unit, and the two spellings that had grown up beside each other are reduced to one pair, _SECONDS and _MINUTES. _SECS was the more common of the two, but _MINS reads like a minimum, and the unit is worth more than the eleven renames the full words cost. Two of the renamed variables were named by constants in bot.rs rather than at the point they are read. Those go, along with the reader beside them: the "a whole number of seconds, or nothing" rule is now `get_optional_env_seconds`, next to the other optional readers in `config::env`, so bot.rs spells its keys where it uses them like the rest of the config does. Six of the renamed variables are set in production. The old names keep working through a fallback in the server-configs compose file until the secrets are renamed, because an unset delay means zero, and zero here means "never delete". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Closes #127. Also closes #161, and opens #155 as the follow-up.
The self-destruction of messages was already there, but it lived in memory: a
tokio::spawnper message, holding a delay. A restart lost every pendingdeletion, and a delay of hours was not worth offering because a deploy would
drop it. It moves into the database, which is what #127 asked for.
Everything after that is the work the move dragged in.
The queue
Scheduled_Message_Deletions(migration 37) is the whole account of what theworker did — successes kept, not just failures, so a failure rate is readable
from the table and not only from Prometheus.
scheduler/deletions.rsclaimswhat is due with one
UPDATE … FOR UPDATE SKIP LOCKED RETURNINGthat pushesfire_afterout by a lease. The lease, not the lock, is what makes the claimexclusive: the row's lock lives only as long as that statement, while the
Telegram requests it leads to take much longer. A worker killed mid-batch
leaves its messages to be claimed again once the lease runs out.
Two unique indexes carry weight:
(chat_id, message_id)andinline_message_idmake scheduling idempotent, so paging through a leaderboardcannot push its own deletion off for ever, and they give cancellation an exact
key.
Two limits are Telegram's. A message older than 48 hours cannot be deleted, so
every delay is cut to 47 and the spare hour pays for the poll interval, the
lease, the warning's grace period and the waits between failed attempts. An
inline message can never be deleted, only edited, so those are replaced with a
placeholder — and only for the groups
MSG_SELFDESTRUCT_INLINE_GROUPSnames,since whatever is put there stays for good.
Redis, and the bot's rights
ONLY_WITH_COMMANDmode needs to know whether the bot may delete othermembers' messages. That was cached in a column, and the value was a door that
opened one way: a refusal wrote
falseand nothing ever wrotetrueback, soa chat that once had an unprivileged bot kept the feature off for ever.
The fix is that the bot is told rather than asked:
my_chat_memberarriveswhenever its own status changes and is in the default update set.
handlers/rights.rsrecords it before the dispatcher's branches, because thesetup message consumes the very update that adds the bot to a group — as an
administrator too, in one step. Redis holds the value with a TTL as the safety
net for a change missed while the bot was down.
The cache is optional in the strongest sense:
REDIS_HOSTunset leaves itdisabled, every miss is answered by the caller doing the work again, and an
unreachable server at startup does not stop the bot.
#155tracks moving theremaining in-memory caches there; the PVP locks are first on that list because
an in-memory
HashSetlocks nothing across two instances.Conversions have names now
asis denied workspace-wide. It was one token meaning three different things,and a reader could not tell which without knowing both types:
From/IntoSaturatingInto— stops at the nearer endApproxInto— the nearest number that fitsOnly integer-to-integer changes behaviour, and it is the one that matters:
there
aswraps. Two call sites had already grown their own clamp by hand andnow say it in a word. The traits are implemented only for the pairs that can
actually lose something, so the compiler refuses the lossy name where nothing
can be lost.
#[domain_type]gained validated string types, checked while the code iscompiled:
literal!expands tofrom_literal(const { check_literal(v) }), sothe
constblock runs the validator and the allocation happens after. Thepromo-code format moved into
PromoCode— it had been a regex in a handlerthat only guarded the inline path, so a malformed code typed into
/promowent to the database and came back as "not found". It now gets its own answer.
What to look at when reviewing
main.rsis load-bearing in two places, and bothare pinned by tests: nothing above the ban gate may write a row for its
sender, and the rights are recorded before the branches rather than by one.
config/— twenty new variables, each in.env.example,docker-compose.yml, theDockerfileandCLAUDE.md, per the checklistthere.
REDIS_HOSTandREDIS_PASSWORDstill have to reach productionthrough
make secret-edit.a commit of its own; several metrics here are new.
210 tests,
cargo clippy --all-targets --workspace -- -D warningsclean. Thecontainer-backed tests need Docker.