Skip to content

fix: six defects a Rails application finds and a test suite cannot - #23

Merged
Halvanhelv merged 10 commits into
mainfrom
fix/sql-store-followups
Sep 10, 2026
Merged

fix: six defects a Rails application finds and a test suite cannot#23
Halvanhelv merged 10 commits into
mainfrom
fix/sql-store-followups

Conversation

@Halvanhelv

Copy link
Copy Markdown
Owner

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_record crashed every translation unless
config.rate_limit was also set. build passed threshold: config.rate_limit
unconditionally, so an unset value overrode the very DEFAULT_THRESHOLD the
class 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'
DatabaseSelector sets prevent_writes on every GET, and the resulting
ActiveRecord::ReadOnlyError is not a StatementInvalid, so it escaped the
redaction — with the whole inlined INSERT, sentence included, in its
message. Observed through a real Rails.error subscriber. Every
ActiveRecordError on 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_error event carrying the provider and the error
class — never the text — and the translation is returned.

On MySQL one long sentence lost the whole batch. translation was
t.text, which MySQL caps at 65,535 bytes; a 75,000-byte run-on sentence
failed the upsert_all and cached none of the other fifty. The column now
carries limit: 16_777_215 — MEDIUMTEXT there, a no-op on PostgreSQL and
SQLite.

RateLimitExceeded said nothing. No threshold, no interval, no
namespace — 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::Error now, redacted like the store's. It still
refuses 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 debug while Rails runs at info
in production. An application would have paid the provider for every sentence
forever without a signal. That line is a warn now.

And docs/instrumentation.md promised cache_error's error field would
carry the adapter's class; for the SQL store it is always
TranslationDiff::Error, because the store redacts before the translator
sees 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.error subscriber, a SELECT-only Postgres
role, a DELETE-less role for the prune path, a 75,000-byte sentence against
real MySQL, and two rails runner processes for the cross-process cache hit
and 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.

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.
@Halvanhelv
Halvanhelv merged commit f66afb5 into main Sep 10, 2026
5 checks passed
@Halvanhelv
Halvanhelv deleted the fix/sql-store-followups branch September 10, 2026 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant