fix: six defects a Rails application finds and a test suite cannot - #23
Merged
Conversation
Setting config.rate_limiter without config.rate_limit passed nil as threshold:, overriding the keyword default and crashing every check() with ArgumentError. Both .build methods now omit threshold: entirely when rate_limit is unset.
DatabaseSelector's prevent_writes mode makes upsert_all raise ActiveRecord::ReadOnlyError, not StatementInvalid, and its message inlines the write statement verbatim -- the same leak the existing redaction was supposed to close. Widen the rescue to ActiveRecord::ActiveRecordError so every write-path error is redacted, not just one class of it.
…tion Translator#fill called cache.store(misses) unrescued, so any store failure -- an oversized batch, a failed opportunistic prune, a dropped connection -- raised past a provider call that had already been made and billed. The translator now rescues the write, fires a cache_error event (provider and error class only, never the text), logs it, and returns the translation anyway. Done once here so every store behaves alike.
MySQL's TEXT caps at 65,535 bytes; one oversized, unsegmentable sentence failed the whole upsert_all and cached nothing for the batch it rode in with. limit: 16_777_215 yields MEDIUMTEXT on MySQL and is a no-op on PostgreSQL and SQLite (verified against all three). The migration template and the test harness schema are compared by a test, so both change together. This raises the ceiling, not removes it -- a still-oversized value now degrades to "not cached" rather than "translation lost", care of the translator's own rescue.
Rewrites sql-cache.md's active_record_base section and adds a Rails replica routing section: pointing active_record_base at a writer role or a separate database does not escape DatabaseSelector's prevent_writes (verified against a live app on Postgres and MySQL); translating outside a GET-served path, or wrapping the call in ActiveRecord::Base.connected_to(role: :writing), both do. Documents that the redaction now covers every ActiveRecord::ActiveRecordError, not just StatementInvalid, and that ActiveRecordRateLimiter's own write is not covered by it. Documents the cache_error instrumentation event and the caching.md write-paths section's new guarantee that a failing write never loses the translation, for every store. Copies the migration's translation column verbatim (limit: 16_777_215 / MEDIUMTEXT) and adds the MySQL ALTER TABLE note for existing installations. Documents that rate_limiter no longer needs rate_limit set, and the RateLimitExceeded message contents, in configuration.md, contracts.md and errors.md.
A cache that silently stopped working costs the provider's price on every sentence, and a signal only visible at debug level is one nobody sees in production. Also corrects what cache_error's error field actually carries per store.
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
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.
Six defects the gem's own suite could not reach, found by installing it into
two real Rails applications and using it the way an application does — a
generator, a migration, controller actions, Puma with threads, replica
routing, PostgreSQL and MySQL — and fixed with each one reproduced in that
application first and again afterwards.
Nothing here is breaking for a PostgreSQL or SQLite user. A MySQL user with
the tables already created needs one
ALTER TABLE, named in the CHANGELOG.What was wrong
config.rate_limiter = :active_recordcrashed every translation unlessconfig.rate_limitwas also set.buildpassedthreshold: config.rate_limitunconditionally, so an unset value overrode the very
DEFAULT_THRESHOLDtheclass defines for that case, and the caller got
ArgumentError: comparison of Integer with nil failed. Both limiters had it.A translated sentence reached the error tracker. Rails'
DatabaseSelectorsetsprevent_writeson every GET, and the resultingActiveRecord::ReadOnlyErroris not aStatementInvalid, so it escaped theredaction — with the whole inlined
INSERT, sentence included, in itsmessage. Observed through a real
Rails.errorsubscriber. EveryActiveRecordErroron the write path is redacted now.A failing cache write destroyed a translation already paid for. The
translator cached before it rebuilt, and nothing rescued, so a store failure
meant the provider had been billed and the caller got an exception instead of
the text. The cache is an optimisation: a failure is now rescued, logged,
reported as a new
cache_errorevent carrying the provider and the errorclass — never the text — and the translation is returned.
On MySQL one long sentence lost the whole batch.
translationwast.text, which MySQL caps at 65,535 bytes; a 75,000-byte run-on sentencefailed the
upsert_alland cached none of the other fifty. The column nowcarries
limit: 16_777_215— MEDIUMTEXT there, a no-op on PostgreSQL andSQLite.
RateLimitExceededsaid nothing. No threshold, no interval, nonamespace — an application catching it had nothing to log. It now names all
three, and none of them is content.
The rate limiter's own database failures surfaced raw. They are this
gem's
TranslationDiff::Errornow, redacted like the store's. It stillrefuses rather than degrading, because it runs before the provider does and
nothing has been paid for yet.
What the verification pass then found
With the rescue in place, a genuinely broken cache — a role holding SELECT
but not INSERT — produced correct translations, zero cached rows, and an
empty log, because the gem logged only at
debugwhile Rails runs atinfoin production. An application would have paid the provider for every sentence
forever without a signal. That line is a
warnnow.And
docs/instrumentation.mdpromisedcache_error'serrorfield wouldcarry the adapter's class; for the SQL store it is always
TranslationDiff::Error, because the store redacts before the translatorsees it. Documented as it behaves, with the advice to alert on the event
rather than on a class name.
Verified, not assumed
Every fix was reproduced and re-checked in a real Rails 8.1 application:
replica routing with a live
Rails.errorsubscriber, a SELECT-only Postgresrole, a DELETE-less role for the prune path, a 75,000-byte sentence against
real MySQL, and two
rails runnerprocesses for the cross-process cache hitand the shared rate limiter. 40 threads against MySQL and 150 concurrent GETs
against PostgreSQL still produce exactly one row and lose no increments.
671 runs, rubocop clean. CI covers SQLite, PostgreSQL and MySQL.