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

CHANGELOG.md

@@ -70,9 +70,52 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
   the row. `upsert_all` inlines values rather than binding them, though, so
   a written translation still appears verbatim in the host application's
   own ActiveRecord log at `debug` -- unrelated to this gem's own `logger`
-  option, which never prints content. See
+  option, which never prints content. The redaction now covers every
+  `ActiveRecord::ActiveRecordError` the write path can raise, not only a
+  statement failure -- an application whose GET requests are routed to a
+  read replica by `ActiveRecord::Middleware::DatabaseSelector` gets
+  `ActiveRecord::ReadOnlyError` there instead, and it is redacted the same
+  way. Pointing `active_record_base` at a different database or a
+  writer-role class does not exempt this store from that routing decision;
+  see [Rails replica routing](docs/sql-cache.md#rails-replica-routing) for
+  what does. See
   [Transactions](docs/sql-cache.md#transactions) and
   [What ends up in your log](docs/sql-cache.md#what-ends-up-in-your-log).
+- **A failing cache write no longer loses the translation it was caching --
+  for every store, not only the SQL one.** `Translator#fill` now rescues a
+  store failure, logs it, fires a new `cache_error` instrumentation event
+  (`provider` and the error's class, never the text), and returns the
+  translation regardless: the cache is an optimisation on top of a
+  translation already paid for at the provider, and losing the write should
+  never mean losing that. See
+  [The three write paths fail differently](docs/caching.md#the-three-write-paths-fail-differently)
+  and [Instrumentation](docs/instrumentation.md). The rate limiter refuses
+  rather than degrades under the same routing -- it runs before the provider
+  is called, so nothing has been paid for yet -- but it too now raises a
+  redacted `TranslationDiff::Error` rather than a raw `ActiveRecord` one.
+  See [Rails replica routing](docs/sql-cache.md#rails-replica-routing).
+- **MySQL: the migration's `translation` column now carries
+  `limit: 16_777_215`, giving it `MEDIUMTEXT` instead of `TEXT`.** `TEXT`
+  caps at 65,535 bytes on MySQL; a single sentence over that size failed
+  the whole batch it rode in with. This is a no-op on Postgres and
+  SQLite -- neither has a length ceiling on `text` to begin with, and
+  nothing else about the schema changes for either. **An installation that
+  already ran this migration on MySQL needs one statement, once:**
+  `ALTER TABLE translation_diff_translations MODIFY translation MEDIUMTEXT NOT NULL;`
+  -- this gem never runs DDL, so nothing does this for you. See
+  [The migration](docs/sql-cache.md#the-migration).
+- **`config.rate_limiter` no longer requires `config.rate_limit`.** Setting
+  the limiter alone used to pass `nil` as the threshold, overriding the
+  keyword default and crashing every check with
+  `ArgumentError: comparison of Integer with nil failed`. An unset
+  `rate_limit` now falls back to the limiter's own default -- 8,000
+  characters per `rate_interval`, the same for both shipped limiters. See
+  [Configuration options](docs/configuration.md#configuration-options).
+- **A refused request now says what it hit.** Both `RateLimitExceeded`
+  classes raise with a message naming the namespace, the threshold and the
+  interval (`"rate limit reached for translation-diff: 8000 characters per
+  60 seconds"`) -- never the content that tripped it. See
+  [Errors](docs/errors.md).
 - **`cache_ttl` of `0` or less now means never expires, and `nil` is
   reachable through `TranslationDiff.configure`.** Previously `nil` was
   documented as meaningful but unreachable through the public

docs/caching.md

@@ -119,3 +119,16 @@ depends on which of these shapes wrote it.
 A caller that needs to know which sentences got cached after a failure
 needs to know which of these three shapes wrote them; the answer is not the
 same for all three.
+
+None of the three ever reaches the caller as an exception, though. The
+cache is an optimisation on top of a translation that has already been
+paid for at the provider: `Translator#fill` rescues whatever error surfaces
+here, logs it, fires a `cache_error` event (provider and error class only,
+never the text -- see [Instrumentation](instrumentation.md)), and returns
+the translation regardless. This holds for all three shapes and every
+store, not only `ActiveRecordCacheStore` -- a `MemoryCacheStore` bug, a
+dropped Redis connection, a SQL write blocked by a read-only replica (see
+[Rails replica routing](sql-cache.md#rails-replica-routing)) all behave the
+same way from the caller's side. What differs between the three shapes
+above is only what ends up cached, never whether the translation comes
+back.

docs/configuration.md

@@ -60,17 +60,17 @@ at all, so an unset environment variable never has to be special-cased.
 | `cache_max_size` | `1_000` | Maximum number of entries `MemoryCacheStore` keeps before evicting the least recently used one. |
 | `cache_table_name` | `"translation_diff_translations"` | Table `ActiveRecordCacheStore` reads and writes. For a host with its own table-naming convention. See [SQL cache](sql-cache.md). |
 | `rate_limit_table_name` | `"translation_diff_rate_limits"` | Table `ActiveRecordRateLimiter` reads and writes. As above. |
-| `active_record_base` | `nil` (`::ActiveRecord::Base`) | The class `ActiveRecordCacheStore` and `ActiveRecordRateLimiter` build their model from -- point this at a second database, or a reader/writer role. See [SQL cache](sql-cache.md#active_record_base-a-second-database-or-a-readerwriter-role). |
+| `active_record_base` | `nil` (`::ActiveRecord::Base`) | The class `ActiveRecordCacheStore` and `ActiveRecordRateLimiter` build their model from -- point this at a second database. It does not exempt this store from a Rails application's own read-replica routing; see [Rails replica routing](sql-cache.md#rails-replica-routing). See [SQL cache](sql-cache.md#active_record_base-a-second-database). |
 | `cache_prune_probability` | `0.0` | Chance, per write, that `ActiveRecordCacheStore` prunes expired rows before returning. `0.0` is off, and a value outside `0.0..1.0` is refused at `configure` time; `rake translation_diff:prune` is the other way to prune. See [SQL cache](sql-cache.md#pruning-three-answers-none-imposed). |
 | `redis_url` | `ENV["REDIS_URL"]` | Where to connect for the Redis-backed cache store and rate limiter. Setting this is what makes `cache` default to `:redis` instead of `:memory`. |
 | `redis_pool_size` | `5` | Size of the connection pool built from `redis_url`. |
 | `redis_pool_timeout` | `5` | Seconds to wait for a connection from that pool before raising. |
-| `rate_limit` | `nil` | Character threshold per `rate_interval`. Unset means no rate limiting at all. |
-| `rate_interval` | `60` | Seconds over which `rate_limit` is measured. **Actually enforced over roughly 5-600 seconds** -- see [The rate limiter contract](contracts.md#the-rate-limiter-contract). |
-| `rate_limiter` | `nil` | A registered name (`:redis`, `:active_record`) or an object satisfying the [rate limiter contract](contracts.md#the-rate-limiter-contract). `nil` with `rate_limit` set resolves to `:redis`. |
+| `rate_limit` | `nil` | Character threshold per `rate_interval`. Unset with `rate_limiter` also unset means no rate limiting at all. Unset with `rate_limiter` set turns rate limiting on anyway, at that limiter's own default threshold -- 8,000 characters per `rate_interval`, the same default for both shipped limiters -- rather than the threshold you never set. |
+| `rate_interval` | `60` | Seconds over which `rate_limit` (or a limiter's own default threshold) is measured. **Actually enforced over roughly 5-600 seconds** -- see [The rate limiter contract](contracts.md#the-rate-limiter-contract). |
+| `rate_limiter` | `nil` | A registered name (`:redis`, `:active_record`) or an object satisfying the [rate limiter contract](contracts.md#the-rate-limiter-contract). `nil` with `rate_limit` also `nil` means no rate limiting; `nil` with `rate_limit` set resolves to `:redis`. Setting `rate_limiter` alone -- with `rate_limit` left unset -- is enough to turn rate limiting on, at the limiter's own default threshold; it no longer needs `rate_limit` set to avoid crashing. |
 | `segmenter` | `:pragmatic` | The sentence segmenter: a registered name or an object satisfying the [segmenter contract](contracts.md#the-segmenter-contract). |
 | `instrumenter` | `nil` | Anything satisfying `ActiveSupport::Notifications`' `#instrument(name, payload) { }` interface. See [Instrumentation and logging](instrumentation.md). |
-| `logger` | `nil` | A standard `Logger`. Receives one `debug` line per provider resolution, naming the provider class -- never content and never a credential. See [Instrumentation and logging](instrumentation.md). |
+| `logger` | `nil` | A standard `Logger` -- anything answering to `debug` and `warn` with a block. Receives one `debug` line per provider resolution, naming the provider class, and a `warn` line when a cache write fails; never content and never a credential. Note that `warn` must be a public method: a bare object inherits a private `Kernel#warn` and would raise instead of logging. See [Instrumentation and logging](instrumentation.md). |
 | `open_timeout` | `5` | Seconds an HTTP-backed provider waits to open a connection before raising `TranslationDiff::TransportError`. |
 | `timeout` | `30` | Seconds an HTTP-backed provider waits for a response before raising `TranslationDiff::TransportError`. |
 | `max_retries` | `3` | Retries `faraday-retry` attempts on a transport failure or a `429`/`500`/`502`/`503`/`504` response, with exponential backoff. `faraday-retry` honours a `Retry-After` header itself, so a `429` usually exhausts its retries before `TranslationDiff::RateLimitError` is ever raised. |

docs/contracts.md

@@ -8,14 +8,18 @@ through its own registry, `TranslationDiff::RateLimiters` -- `:redis` and
 
 - the object assigned to `config.rate_limiter`, if any -- an object still
   bypasses the registry entirely, the same way it does for `cache`;
-- otherwise `nil` if `rate_limit` was never set -- and `Dispatcher#throttle`
-  checks for that `nil` and skips rate limiting entirely, so the common case
-  costs nothing;
+- otherwise `nil` if both `rate_limiter` and `rate_limit` were never set --
+  and `Dispatcher#throttle` checks for that `nil` and skips rate limiting
+  entirely, so the common case costs nothing;
 - otherwise the registered limiter named by `config.rate_limiter`, or
-  `TranslationDiff::RedisRateLimiter` when `rate_limiter` is left unset --
-  built from `rate_limit`, `rate_interval`, `cache_namespace`, and either
-  `redis_url` (`:redis`) or `active_record_base` and `rate_limit_table_name`
-  (`:active_record`; see [SQL cache](sql-cache.md)).
+  `TranslationDiff::RedisRateLimiter` when `rate_limiter` is left unset but
+  `rate_limit` is set -- built from `rate_interval`, `cache_namespace`, and
+  either `redis_url` (`:redis`) or `active_record_base` and
+  `rate_limit_table_name` (`:active_record`; see [SQL cache](sql-cache.md)).
+  `rate_limit` supplies the threshold when it is set; left unset, the
+  limiter falls back to its own default -- 8,000 characters per
+  `rate_interval` for both shipped limiters -- instead of crashing, so
+  setting `rate_limiter` alone is enough to turn a limiter on.
 
 An object assigned to `rate_limiter` must implement:
 
@@ -29,7 +33,10 @@ def check(size); end
 `TranslationDiff::RedisRateLimiter::RateLimitExceeded` when its threshold is
 exceeded within its interval;
 `TranslationDiff::ActiveRecordRateLimiter` raises its own
-`RateLimitExceeded`, a distinct class under the same name. Neither `redis`
+`RateLimitExceeded`, a distinct class under the same name. Both raise with a
+message naming the namespace, the threshold and the interval that were hit
+(`"rate limit reached for translation-diff: 8000 characters per 60
+seconds"`) -- never the text that tripped it. Neither `redis`
 nor `connection_pool` nor `ratelimit` is a dependency of this gem:
 `ratelimit` is required on the first check, so an application that
 configures no `rate_limit` never needs it, and its absence raises

docs/errors.md

@@ -50,6 +50,22 @@ TranslationDiff::Error
                                                  # `TranslationDiff::Error` to catch both.
 ```
 
+Both `RateLimitExceeded` classes raise with a message naming the namespace,
+the threshold and the interval that were exceeded (`"rate limit reached for
+translation-diff: 8000 characters per 60 seconds"`) -- never the text that
+tripped it.
+
+Both SQL-backed collaborators report a database failure the same way. A
+cache write that the database refuses -- including under Rails'
+`prevent_writes` (a read-replica request, see
+[Rails replica routing](sql-cache.md#rails-replica-routing)) -- is rescued,
+redacted and swallowed, and the translation is returned anyway. The rate
+limiter's own write raises a redacted `TranslationDiff::Error` instead of
+continuing, because it runs before the provider does and a limiter that
+cannot count is not a limiter. Either way `rescue TranslationDiff::Error`
+around `translate` catches what a caller can catch, and no raw
+`ActiveRecord::ReadOnlyError` reaches it.
+
 `ProviderError` and its subclasses carry `#provider` (the registered name)
 and `#status` (the HTTP status code), so a caller can log or branch on which
 service and which response caused the failure without parsing the message.

docs/instrumentation.md

@@ -6,7 +6,7 @@ and `config.logger` accepts a standard `Logger`. Neither is required: with
 both unset, `TranslationDiff.translate` runs exactly the same, at no extra
 cost.
 
-A translation emits up to five events, each named `<name>.translation_diff`:
+A translation emits up to six events, each named `<name>.translation_diff`:
 
 | Event | Fired | Payload |
 | --- | --- | --- |
@@ -15,6 +15,25 @@ A translation emits up to five events, each named `<name>.translation_diff`:
 | `request` | Once per batch actually sent to the provider (skipped entirely on a full cache hit). | `provider`, `batch` (values sent), `characters` |
 | `rate_limit` | Once per batch sent to the provider, only when a rate limiter is configured. | `provider`, `characters` |
 | `usage` | Once per batch actually sent to the provider, right after `request`. | `provider`, `characters`, `billed_characters`, `reported`, `model` |
+| `cache_error` | Only when writing the translation back to the cache fails -- after the provider has already answered. Never fires on a successful write, so it is not part of every call the way the other five are. | `provider`, `error` (the failed write's error class, as a string) |
+
+`cache_error` is what a failing cache write looks like from the outside:
+the write itself is rescued, not the translation, which still reaches the
+caller -- see
+[The three write paths fail differently](caching.md#the-three-write-paths-fail-differently).
+`error` is the exception's class name, never its message, which could echo
+the row it failed to write. Which class you see depends on the store: a
+store that redacts its own failures reports that redaction, so
+`ActiveRecordCacheStore` always gives `"TranslationDiff::Error"` -- the
+adapter's own class is named inside that error's (content-free) message,
+not in this payload. `RedisCacheStore` does not wrap, so it gives the
+driver's class, `"Redis::CannotConnectError"` and the like. Alert on the
+event, not on a particular class name.
+
+The same failure is logged at **warn**, not debug: an application whose
+cache has quietly stopped accepting writes pays the provider for every
+sentence, every time, and a signal only visible at debug level is one
+nobody sees in production.
 
 `usage`'s `characters` is what this library sent, counted locally -- the same
 number `request` carries. `billed_characters` is what the provider said it