From 346afbee35dd86dd8fd97a582caeb52dd15acb8e Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 01:27:41 +0400 Subject: [PATCH 01/10] fix: let an unset rate_limit fall back to each limiter's own default 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. --- lib/translation_diff/active_record_rate_limiter.rb | 7 +++++-- lib/translation_diff/redis_rate_limiter.rb | 8 ++++---- .../active_record_rate_limiter_test.rb | 11 +++++++++++ test/translation_diff/redis_rate_limiter_test.rb | 13 +++++++++++++ 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb index 4950433..6d0163c 100644 --- a/lib/translation_diff/active_record_rate_limiter.rb +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -11,9 +11,12 @@ class RateLimitExceeded < TranslationDiff::Error; end # gets from its own fixed five-second buckets at the default 60-second interval. BUCKET_FRACTION = 12 + # An unset rate_limit must mean DEFAULT_THRESHOLD, not the nil that would override that keyword default. def self.build(config) - new(namespace: config.cache_namespace, table_name: config.rate_limit_table_name, - threshold: config.rate_limit, interval: config.rate_interval, base: config.active_record_base) + options = { namespace: config.cache_namespace, table_name: config.rate_limit_table_name, + interval: config.rate_interval, base: config.active_record_base } + options[:threshold] = config.rate_limit unless config.rate_limit.nil? + new(**options) end def initialize(namespace:, table_name:, threshold: DEFAULT_THRESHOLD, interval: DEFAULT_INTERVAL, base: nil, diff --git a/lib/translation_diff/redis_rate_limiter.rb b/lib/translation_diff/redis_rate_limiter.rb index ff1d895..928a475 100644 --- a/lib/translation_diff/redis_rate_limiter.rb +++ b/lib/translation_diff/redis_rate_limiter.rb @@ -8,11 +8,11 @@ class RateLimitExceeded < TranslationDiff::Error; end # This library limits the provider as a whole rather than per caller, so there is exactly one subject. SUBJECT = "call".freeze + # An unset rate_limit must mean DEFAULT_THRESHOLD, not the nil that would override that keyword default. def self.build(config) - new(config.redis_pool, - threshold: config.rate_limit, - interval: config.rate_interval, - namespace: config.cache_namespace) + options = { interval: config.rate_interval, namespace: config.cache_namespace } + options[:threshold] = config.rate_limit unless config.rate_limit.nil? + new(config.redis_pool, **options) end # `connection_pool` is duck-typed to #with; neither connection_pool nor ratelimit is a hard dependency. diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb index 00b152d..acc29e3 100644 --- a/test/translation_diff/active_record_rate_limiter_test.rb +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -155,6 +155,17 @@ def test_build_takes_its_settings_from_the_configuration assert_equal ["from-config"], built.model.pluck(:namespace) end + # Setting only `rate_limiter`, the config option that turns this limiter on, must not crash every call. + def test_build_falls_back_to_the_default_threshold_when_rate_limit_is_unset + config = TranslationDiff::Configuration.new + config.rate_limiter = :active_record + + built = TranslationDiff::ActiveRecordRateLimiter.build(config) + + assert_equal TranslationDiff::ActiveRecordRateLimiter::DEFAULT_THRESHOLD, built.instance_variable_get(:@threshold) + built.check(1) + end + def test_add_omits_unique_by_when_the_connection_does_not_support_a_conflict_target limiter = build_limiter(threshold: 100, interval: 60) connection = Class.new do diff --git a/test/translation_diff/redis_rate_limiter_test.rb b/test/translation_diff/redis_rate_limiter_test.rb index d986fbc..56acc7c 100644 --- a/test/translation_diff/redis_rate_limiter_test.rb +++ b/test/translation_diff/redis_rate_limiter_test.rb @@ -137,6 +137,19 @@ def test_a_missing_ratelimit_gem_raises_a_translation_diff_error assert_match(/Add `gem "ratelimit"`/, error.message) end + # Setting only `rate_limiter`, the config option that turns this limiter on, must not crash every call. + def test_build_falls_back_to_the_default_threshold_when_rate_limit_is_unset + server = FakeRedisServer.new + config = TranslationDiff::Configuration.new + config.rate_limiter = :redis + config.instance_variable_set(:@redis_pool, FakeConnectionPool.new(server)) + + built = TranslationDiff::RedisRateLimiter.build(config) + + assert_equal TranslationDiff::RedisRateLimiter::DEFAULT_THRESHOLD, built.send(:threshold) + built.check(1) + end + private def limiter(server, **) From 7d2e36ac8d3d1a49083c49982575955f6df2e4ef Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 01:30:31 +0400 Subject: [PATCH 02/10] fix: redact every ActiveRecordError the write path can raise 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. --- lib/translation_diff/active_record_cache_store.rb | 10 +++++----- .../active_record_cache_store_test.rb | 13 +++++++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb index 7a96634..91ce7ae 100644 --- a/lib/translation_diff/active_record_cache_store.rb +++ b/lib/translation_diff/active_record_cache_store.rb @@ -41,7 +41,7 @@ def write_multi(pairs) prune_sometimes pairs rescue StandardError => e - raise unless ar_statement_invalid?(e) + raise unless ar_error?(e) raise redacted_error(e), cause: nil end @@ -93,14 +93,14 @@ def prune_sometimes model.transaction(requires_new: true) { prune } rescue StandardError => e - raise unless ar_statement_invalid?(e) + raise unless ar_error?(e) raise redacted_prune_error(e), cause: nil end - # `defined?` short-circuits before the `is_a?`, so this never itself raises when the gem was never loaded. - def ar_statement_invalid?(error) - defined?(ActiveRecord::StatementInvalid) && error.is_a?(ActiveRecord::StatementInvalid) + # Any ActiveRecordError, not just StatementInvalid -- ReadOnlyError carries a whole write statement too. + def ar_error?(error) + defined?(ActiveRecord::ActiveRecordError) && error.is_a?(ActiveRecord::ActiveRecordError) end def active_record_feature = "the cache" diff --git a/test/translation_diff/active_record_cache_store_test.rb b/test/translation_diff/active_record_cache_store_test.rb index 725d721..34737e3 100644 --- a/test/translation_diff/active_record_cache_store_test.rb +++ b/test/translation_diff/active_record_cache_store_test.rb @@ -156,6 +156,19 @@ def test_write_raises_a_friendly_error_when_active_record_is_genuinely_unavailab Object.const_set(:ActiveRecord, removed) if removed end + # DatabaseSelector raises this, not StatementInvalid, and it inlines the row into its message the same way. + def test_a_readonly_error_never_carries_the_translated_content + store = build_store + secret = "SECRET-PATIENT-NOTE-READONLY-12345" + + error = ActiveRecord::Base.while_preventing_writes do + assert_raises(TranslationDiff::Error) { store.write("a", secret) } + end + + assert_includes error.message, "ActiveRecord::ReadOnlyError" + refute_includes error.message, secret + end + private def build_store(namespace: "translation-diff", ttl: 604_800) From 7b077492ed03bdcaddab4385da3f7592bc63c9c8 Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 01:34:47 +0400 Subject: [PATCH 03/10] fix: never let a failing cache write or prune lose a paid-for translation 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. --- lib/translation_diff/translator.rb | 21 ++-- .../translator_cache_failure_test.rb | 111 ++++++++++++++++++ test/translation_diff/translator_test.rb | 49 ++++++++ 3 files changed, 171 insertions(+), 10 deletions(-) create mode 100644 test/translation_diff/translator_cache_failure_test.rb diff --git a/lib/translation_diff/translator.rb b/lib/translation_diff/translator.rb index b51c531..6297eac 100644 --- a/lib/translation_diff/translator.rb +++ b/lib/translation_diff/translator.rb @@ -128,19 +128,20 @@ def same_language?(from) = from.to_s.casecmp?(@to.to_s) # The cache answers for what it has, the provider for the rest, and only what came back is written home. def fill(provider, segments, from) - cache = sentence_cache(provider, from) + cache = TranslationDiff::SentenceCache.new(store: config.cache_store, provider: provider.cache_key, + from: from, to: @to, options: @options) misses = cache.fill(segments) instrument("cache", provider: provider.cache_key, hits: segments.size - misses.size, misses: misses.size) - dispatcher(provider, from).dispatch(misses) - cache.store(misses) - end - - def sentence_cache(provider, from) - TranslationDiff::SentenceCache.new(store: config.cache_store, provider: provider.cache_key, - from: from, to: @to, options: @options) + TranslationDiff::Dispatcher.new(provider: provider, from: from, to: @to, options: @options, + config: config).dispatch(misses) + store(cache, misses, provider) end - def dispatcher(provider, from) - TranslationDiff::Dispatcher.new(provider: provider, from: from, to: @to, options: @options, config: config) + # A translation already paid for at the provider must reach the caller even if writing it back never does. + def store(cache, misses, provider) + cache.store(misses) + rescue StandardError => e + log("cache write failed (#{e.class}), the translation is returned uncached") + instrument("cache_error", provider: provider.cache_key, error: e.class.to_s) end end diff --git a/test/translation_diff/translator_cache_failure_test.rb b/test/translation_diff/translator_cache_failure_test.rb new file mode 100644 index 0000000..8c4876b --- /dev/null +++ b/test/translation_diff/translator_cache_failure_test.rb @@ -0,0 +1,111 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.postgres? + ActiveRecordDatabase.connect! + + # The gap task 3 found: a real failing prune, reached through the translator, used to lose a translation + # already paid for at the provider -- only a real trigger-forced prune failure against PostgreSQL proves it. + class TranslatorCacheFailureTest < Minitest::Test + class RecordingProvider < TranslationDiff::Provider + def self.capabilities + TranslationDiff::Capabilities.new(max_request_size: 1_000, max_batch_size: 10, max_text_size: nil, + html: :none, notranslate: false, detects_language: true, + reports_billing: false) + end + + def translate(request) + TranslationDiff::Translation::Response.build(request: request, texts: request.texts.map(&:upcase)) + end + + def detect(_text) = "en" + def cache_key = "recording" + end + + class Recorder + attr_reader :events + + def initialize = @events = [] + + def instrument(name, payload) + @events << [name, payload] + yield if block_given? + end + end + + def setup + ActiveRecordDatabase.truncate + TranslationDiff.reset! + end + + def teardown + TranslationDiff.reset! + remove_failing_delete_trigger + end + + def test_a_failing_prune_still_returns_the_translation_already_paid_for + recorder = Recorder.new + TranslationDiff.configure do |c| + c.cache = store_with_a_pending_prune + c.instrumenter = recorder + end + provider = RecordingProvider.new(TranslationDiff.config) + + result = TranslationDiff::Translator.new("brand new sentence.", from: "en", to: "ru", provider: provider).call + + assert_equal "BRAND NEW SENTENCE.", result + assert_includes recorder.events.map(&:first), "cache_error.translation_diff" + end + + private + + # An expired row plus a trigger that always fails its DELETE, so the write that follows always triggers a prune. + def store_with_a_pending_prune + store = pruning_store + store.write("expired", "one") + expire(store, "expired") + install_failing_delete_trigger + store + end + + def pruning_store + TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: 60, + table_name: "translation_diff_translations", + prune_probability: 1.0) + end + + def expire(store, key) + digest = Digest::SHA256.hexdigest(key) + store.model.where(key_digest: digest).update_all(expires_at: Time.now.utc - 1) + end + + def install_failing_delete_trigger + harness_model.connection.execute(<<~SQL) + CREATE OR REPLACE FUNCTION translator_cache_failure_test_fail_delete() RETURNS trigger AS $$ + BEGIN RAISE EXCEPTION 'simulated prune failure'; END; $$ LANGUAGE plpgsql; + CREATE TRIGGER translator_cache_failure_test_fail_delete BEFORE DELETE + ON translation_diff_translations FOR EACH ROW + EXECUTE FUNCTION translator_cache_failure_test_fail_delete(); + SQL + end + + def remove_failing_delete_trigger + connection = harness_model.connection + connection.execute("DROP TRIGGER IF EXISTS translator_cache_failure_test_fail_delete " \ + "ON translation_diff_translations") + connection.execute("DROP FUNCTION IF EXISTS translator_cache_failure_test_fail_delete()") + end + + def harness_model + TranslationDiff::ActiveRecordCacheStore.new(namespace: "harness", ttl: 60, + table_name: "translation_diff_translations").model + end + end +else + class TranslatorCacheFailureTest < Minitest::Test + def test_postgres_is_unavailable + skip "TRANSLATION_DIFF_DATABASE_URL does not name a PostgreSQL database; " \ + "only PostgreSQL aborts a transaction on a statement error" + end + end +end diff --git a/test/translation_diff/translator_test.rb b/test/translation_diff/translator_test.rb index 2409956..eed9894 100644 --- a/test/translation_diff/translator_test.rb +++ b/test/translation_diff/translator_test.rb @@ -72,6 +72,14 @@ def initialize = @sizes = [] def check(size) = @sizes << size end + # Stands in for any store whose write fails -- a full batch, one sentence too long, a dropped connection. + class FailingCacheStore + class BoomError < StandardError; end + + def read_multi(keys) = keys.map { nil } + def write_multi(_pairs) = raise BoomError, "cache write exploded" + end + def setup super @provider = RecordingProvider.new(TranslationDiff::Configuration.new) @@ -201,6 +209,47 @@ def test_the_cache_event_carries_hit_and_miss_counts assert_equal "recording", payload[:provider] end + # The cache is an optimisation: a translation already paid for at the provider must reach the caller regardless. + def test_a_failing_cache_write_does_not_lose_a_translation_already_paid_for + TranslationDiff.configure { |c| c.cache = FailingCacheStore.new } + + assert_equal "ONE.", translate("one.", from: "en", to: "ru") + end + + def test_a_failing_cache_write_emits_a_cache_error_event_naming_the_provider_and_error_class + recorder = instrumented { |c| c.cache = FailingCacheStore.new } + instrumented_translate("Hello there.") + + payload = payload_for(recorder, "cache_error") + + assert_equal "recording", payload[:provider] + assert_equal "TranslatorTest::FailingCacheStore::BoomError", payload[:error] + end + + # The instrumentation payload carries the error's class, never the store's own message, which could quote the row. + def test_a_failing_cache_writes_event_never_carries_the_text_being_translated + secret = "Zaphod Beeblebrox is president." + recorder = instrumented { |c| c.cache = FailingCacheStore.new } + instrumented_translate(secret) + + serialised = recorder.events.map { |name, payload| "#{name}#{payload}" }.join + + refute_includes serialised, "Zaphod" + refute_includes serialised, secret + end + + def test_a_failing_cache_write_is_logged_through_the_gems_own_logger + logger = FakeLogger.new + TranslationDiff.configure do |c| + c.logger = logger + c.cache = FailingCacheStore.new + end + + translate("one.", from: "en", to: "ru") + + assert_match(/cache write failed/, logger.lines.last) + end + def test_the_request_event_carries_the_provider_a_batch_size_and_a_character_count recorder = instrumented instrumented_translate("Hello there.") From d07a0042e68c7d21f71533314527f40d44a76884 Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 01:40:20 +0400 Subject: [PATCH 04/10] fix: give the translation column room for MySQL's TEXT ceiling 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. --- .../create_translation_diff_tables.rb.erb | 2 +- test/support/active_record_database.rb | 5 +- ...ecord_cache_store_mysql_text_limit_test.rb | 78 +++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 test/translation_diff/active_record_cache_store_mysql_text_limit_test.rb diff --git a/lib/generators/translation_diff/templates/create_translation_diff_tables.rb.erb b/lib/generators/translation_diff/templates/create_translation_diff_tables.rb.erb index d04ec55..006bbad 100644 --- a/lib/generators/translation_diff/templates/create_translation_diff_tables.rb.erb +++ b/lib/generators/translation_diff/templates/create_translation_diff_tables.rb.erb @@ -3,7 +3,7 @@ class CreateTranslationDiffTables < ActiveRecord::Migration[<%= ActiveRecord::Mi create_table :translation_diff_translations, if_not_exists: true do |t| t.string :namespace, null: false, limit: 64 t.string :key_digest, null: false, limit: 64 - t.text :translation, null: false + t.text :translation, null: false, limit: 16_777_215 t.datetime :expires_at t.timestamps end diff --git a/test/support/active_record_database.rb b/test/support/active_record_database.rb index f755bf6..4ed2041 100644 --- a/test/support/active_record_database.rb +++ b/test/support/active_record_database.rb @@ -22,6 +22,9 @@ def self.url = ENV.fetch("TRANSLATION_DIFF_DATABASE_URL", nil) # asks here, so the detection lives in one place instead of a same-named constant defined in each of them. def self.postgres? = available? && url.to_s.match?(%r{\Apostgres(ql)?://}) + # MySQL's TEXT type caps at 65,535 bytes; only a real MySQL server proves the migration's limit: raised it. + def self.mysql? = available? && url.to_s.match?(%r{\A(mysql2|trilogy)://}) + # Both tables so Task 3's migration has something to be checked against; only the first is used so far. def self.define_schema connection = ::ActiveRecord::Base.connection @@ -35,7 +38,7 @@ def self.define_translations_table(connection) connection.create_table :translation_diff_translations do |t| t.string :namespace, null: false, limit: 64 t.string :key_digest, null: false, limit: 64 - t.text :translation, null: false + t.text :translation, null: false, limit: 16_777_215 t.datetime :expires_at t.timestamps end diff --git a/test/translation_diff/active_record_cache_store_mysql_text_limit_test.rb b/test/translation_diff/active_record_cache_store_mysql_text_limit_test.rb new file mode 100644 index 0000000..2b862bc --- /dev/null +++ b/test/translation_diff/active_record_cache_store_mysql_text_limit_test.rb @@ -0,0 +1,78 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.mysql? + ActiveRecordDatabase.connect! + + # MySQL's TEXT column tops out at 65,535 bytes; only a real MySQL server proves the migration raised that ceiling. + class ActiveRecordCacheStoreMysqlTextLimitTest < Minitest::Test + class RecordingProvider < TranslationDiff::Provider + def self.capabilities + TranslationDiff::Capabilities.new(max_request_size: 100_000_000, max_batch_size: 1_000, + max_text_size: nil, html: :none, notranslate: false, + detects_language: true, reports_billing: false) + end + + def translate(request) + TranslationDiff::Translation::Response.build(request: request, texts: request.texts.map(&:upcase)) + end + + def detect(_text) = "en" + def cache_key = "recording" + end + + def setup + ActiveRecordDatabase.truncate + TranslationDiff.reset! + end + + def teardown = TranslationDiff.reset! + + # The finding this migration fixes: a 75,000-byte run-on sentence used to fail the whole upsert. + def test_a_75000_byte_sentence_caches + store = build_store + value = "a" * 75_000 + + store.write("oversized", value) + + assert_equal [value], store.read_multi(["oversized"]) + end + + # Raising the ceiling does not remove it -- MEDIUMTEXT still caps at 16,777,215 bytes. + def test_a_value_beyond_the_new_ceiling_still_raises_a_redacted_error + store = build_store + value = "a" * 16_777_216 + + error = assert_raises(TranslationDiff::Error) { store.write("too-big", value) } + + assert_includes error.message, "the cache write failed" + end + + # With fix 3 in place, that write failure degrades to "not cached", not "translation lost". + def test_a_translator_still_returns_a_translation_the_store_cannot_hold + store = build_store + TranslationDiff.configure { |c| c.cache = store } + provider = RecordingProvider.new(TranslationDiff.config) + oversized = "a" * 16_777_216 + + result = TranslationDiff::Translator.new(oversized, from: "en", to: "ru", provider: provider).call + + assert_equal oversized.upcase, result + assert_equal 0, store.model.count + end + + private + + def build_store + TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: 60, + table_name: "translation_diff_translations") + end + end +else + class ActiveRecordCacheStoreMysqlTextLimitTest < Minitest::Test + def test_mysql_is_unavailable + skip "TRANSLATION_DIFF_DATABASE_URL does not name a MySQL database; " \ + "only a real MySQL server enforces the TEXT column's byte ceiling" + end + end +end From 6e1b187f09b811c0075d8cfce683267a51993b23 Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 01:42:43 +0400 Subject: [PATCH 05/10] fix: say which limit was reached when a rate limiter refuses --- lib/translation_diff/active_record_rate_limiter.rb | 7 ++++++- lib/translation_diff/redis_rate_limiter.rb | 8 +++++++- test/support/rate_limiter_contract.rb | 11 +++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb index 6d0163c..6a03a4d 100644 --- a/lib/translation_diff/active_record_rate_limiter.rb +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -32,11 +32,16 @@ def initialize(namespace:, table_name:, threshold: DEFAULT_THRESHOLD, interval: # A sliding window: every bucket covering the last `interval` seconds is summed, not just the current one. def check(size) - raise RateLimitExceeded if current_total >= @threshold + raise RateLimitExceeded, exceeded_message if current_total >= @threshold add(size) end + # Counts and settings, never a character of what was being translated. + def exceeded_message + "rate limit reached for #{@namespace}: #{@threshold} characters per #{@interval} seconds" + end + # Buckets that have fully aged out of the window as of now; the oldest bucket itself is still counted by it. def prune = model.where(namespace: @namespace).where(bucket: ...oldest_bucket).delete_all diff --git a/lib/translation_diff/redis_rate_limiter.rb b/lib/translation_diff/redis_rate_limiter.rb index 928a475..af0a727 100644 --- a/lib/translation_diff/redis_rate_limiter.rb +++ b/lib/translation_diff/redis_rate_limiter.rb @@ -31,7 +31,8 @@ def check(size) connection_pool.with do |redis| rate_limit = limiter_class.new(namespace, redis: redis) - raise RateLimitExceeded if rate_limit.exceeded?(SUBJECT, threshold: threshold, interval: interval) + raise RateLimitExceeded, exceeded_message if rate_limit.exceeded?(SUBJECT, threshold: threshold, + interval: interval) rate_limit.add(SUBJECT, size) end @@ -39,6 +40,11 @@ def check(size) private + # Counts and settings, never a character of what was being translated. + def exceeded_message + "rate limit reached for #{namespace}: #{threshold} characters per #{interval} seconds" + end + attr_reader :connection_pool, :threshold, :interval, :namespace # Required at first check, not load time; naming the bare constant instead would raise a raw NameError. diff --git a/test/support/rate_limiter_contract.rb b/test/support/rate_limiter_contract.rb index b1b0989..5e41e84 100644 --- a/test/support/rate_limiter_contract.rb +++ b/test/support/rate_limiter_contract.rb @@ -13,6 +13,17 @@ def test_checks_summing_past_the_threshold_raise assert_raises(rate_limit_exceeded_error) { build_limiter(threshold: 100, interval: 60).check(1) } end + # An application catching this logs it; the class name alone told it nothing it could act on. + def test_the_refusal_names_the_limit_it_hit_and_no_content + build_limiter(threshold: 100, interval: 60).check(100) + + error = assert_raises(rate_limit_exceeded_error) do + build_limiter(threshold: 100, interval: 60).check(1) + end + + assert_match(/100 characters per 60 seconds/, error.message) + end + # Rollover is not in this contract: proving it means waiting for a bucket to turn over, and only # ActiveRecordRateLimiter can be made to turn one over without a real sleep. See its own test file. end From f938629b1f6cf1bc30a0c4104a1648355c4f8d6a Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 01:57:23 +0400 Subject: [PATCH 06/10] docs: bring SQL store docs current with the four post-review fixes 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. --- CHANGELOG.md | 45 +++++++++++++++++- docs/caching.md | 13 ++++++ docs/configuration.md | 8 ++-- docs/contracts.md | 23 +++++---- docs/errors.md | 15 ++++++ docs/instrumentation.md | 11 ++++- docs/sql-cache.md | 101 ++++++++++++++++++++++++++++++++++++---- 7 files changed, 193 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40f4f67..434c745 100644 --- a/CHANGELOG.md +++ b/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). One gap remains: + `ActiveRecordRateLimiter`'s own write is not covered by this -- under the + same read-replica routing, it still raises a raw, un-rescued + `ActiveRecord::ReadOnlyError` that fails the `translate` call outright. + 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 diff --git a/docs/caching.md b/docs/caching.md index 9222fec..af70ddc 100644 --- a/docs/caching.md +++ b/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. diff --git a/docs/configuration.md b/docs/configuration.md index 898bddd..73c88ac 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -60,14 +60,14 @@ 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). | diff --git a/docs/contracts.md b/docs/contracts.md index f943494..55ca286 100644 --- a/docs/contracts.md +++ b/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 diff --git a/docs/errors.md b/docs/errors.md index b6dd048..16fec8f 100644 --- a/docs/errors.md +++ b/docs/errors.md @@ -50,6 +50,21 @@ 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. + +**One documented exception to "every error this gem raises."** +`ActiveRecordRateLimiter#check`'s own write is not wrapped in this gem's +error handling at all: under Rails' `prevent_writes` (a read-replica +request, see [Rails replica routing](sql-cache.md#rails-replica-routing)), +it raises a raw `ActiveRecord::ReadOnlyError` straight through, uncaught +and un-redacted. `ActiveRecordCacheStore`'s write does not have this gap -- +its errors are redacted `TranslationDiff::Error`s, rescued before they ever +reach a caller. A `rescue TranslationDiff::Error` around `translate` does +not catch the rate limiter's version. + `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. diff --git a/docs/instrumentation.md b/docs/instrumentation.md index 9e407f9..25ab901 100644 --- a/docs/instrumentation.md +++ b/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 `.translation_diff`: +A translation emits up to six events, each named `.translation_diff`: | Event | Fired | Payload | | --- | --- | --- | @@ -15,6 +15,15 @@ A translation emits up to five events, each named `.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 (`"ActiveRecord::ReadOnlyError"`, +`"Redis::CannotConnectError"`, ...), never its message, which could echo +the row it failed to write. `usage`'s `characters` is what this library sent, counted locally -- the same number `request` carries. `billed_characters` is what the provider said it diff --git a/docs/sql-cache.md b/docs/sql-cache.md index 53bec08..0f29c3c 100644 --- a/docs/sql-cache.md +++ b/docs/sql-cache.md @@ -32,14 +32,18 @@ expired row, it does not remove it by itself. See ## Transactions A write joins the caller's transaction. A failed write no longer poisons -it -- the write runs in its own savepoint, so a `StatementInvalid` there -does not abort a transaction it does not own -- but the rollback semantics -otherwise stay ordinary: if the caller's transaction rolls back, a +it -- the write runs in its own savepoint, so an `ActiveRecord::ActiveRecordError` +there does not abort a transaction it does not own -- but the rollback +semantics otherwise stay ordinary: if the caller's transaction rolls back, a translation this store just wrote rolls back with it, and the next request pays for it again. This is the largest difference between this store and the Redis one it substitutes for -- a Redis write is never inside anyone's transaction, so it never rolls back with one. +That savepoint failure never reaches a caller of `TranslationDiff.translate` +either, whatever raised it -- see +[The three write paths fail differently](caching.md#the-three-write-paths-fail-differently). + ## What ends up in your log `upsert_all` inlines values into the SQL it sends rather than binding them, @@ -56,6 +60,11 @@ your application logs SQL at `debug` and what it translates is confidential, keep that log above `debug` around this store, or use `RedisCacheStore` instead. +That scrubbing covers every `ActiveRecord::ActiveRecordError` the write path +can raise, not just a syntax or constraint failure -- see +[Rails replica routing](#rails-replica-routing) below for the error this +widened scope was written for. + ## The tables Two tables, created by a migration you run once -- see @@ -110,7 +119,7 @@ class CreateTranslationDiffTables < ActiveRecord::Migration[7.1] create_table :translation_diff_translations, if_not_exists: true do |t| t.string :namespace, null: false, limit: 64 t.string :key_digest, null: false, limit: 64 - t.text :translation, null: false + t.text :translation, null: false, limit: 16_777_215 t.datetime :expires_at t.timestamps end @@ -143,6 +152,23 @@ Both tables are always created together -- there is no generator flag to get one without the other, since deciding to use one but not the other costs nothing at migration time. +`translation` carries `limit: 16_777_215`, which is a no-op on Postgres and +SQLite -- `text` there has no length ceiling regardless -- and yields +`MEDIUMTEXT` on MySQL instead of the default `TEXT`, which tops out at +65,535 bytes. Without it, one sentence over that size failed the whole +batch it rode in with on MySQL, and PostgreSQL and SQLite were never +affected. + +**An existing MySQL installation** that ran this migration before it +carried the `limit:` needs one statement, once, through its own deploy +process -- this gem still never runs DDL for you: + +```sql +ALTER TABLE translation_diff_translations MODIFY translation MEDIUMTEXT NOT NULL; +``` + +Postgres and SQLite users have nothing to do here. + ## `cache_ttl` becomes `expires_at` `cache_ttl` (in seconds, same option `RedisCacheStore` reads) is written @@ -192,7 +218,10 @@ and there is no single right answer to "when," so none is forced on you: because a translation-serving request should not be paying, even occasionally, for someone else's expired rows. A value that will not coerce to a number is refused at `configure` time, - not on the first write that would have consulted it. + not on the first write that would have consulted it. A prune that fails + here fails exactly like a failed write, and is rescued the same way -- it + does not lose the translation it rode in with, see + [The three write paths fail differently](caching.md#the-three-write-paths-fail-differently). - **Doing nothing.** Also a supported answer. An unpruned table is correct -- reads still skip every expired row -- just larger than it needs to be. @@ -200,13 +229,12 @@ and there is no single right answer to "when," so none is forced on you: multi-tenant table with several namespaces needs `#prune` called once per namespace if every tenant is to be pruned. -## `active_record_base`: a second database, or a reader/writer role +## `active_record_base`: a second database `config.active_record_base` (default `::ActiveRecord::Base`) is the class `ActiveRecordCacheStore` and `ActiveRecordRateLimiter` build their model -from. Point it at a class connected to a second database, or one pinned to -a writer role, and this store's traffic follows that connection instead of -your application's primary one: +from. Point it at a class connected to a second database and this store's +traffic follows that connection instead of your application's primary one: ```ruby class TranslationDiffRecord < ActiveRecord::Base @@ -220,6 +248,61 @@ TranslationDiff.configure do |config| end ``` +**This is not a way around a read-replica decision Rails already made for +the request.** See [Rails replica routing](#rails-replica-routing) below -- +`active_record_base` still matters, but not for that. + +## Rails replica routing + +If your application routes GET requests to a read replica the way the Rails +guides describe -- `ActiveRecord::Middleware::DatabaseSelector` in the +middleware stack -- every GET runs with `prevent_writes` on. A page that +calls `translate` and triggers a cache or rate-limit write during that +request hits `ActiveRecord::ReadOnlyError`. + +Pointing `active_record_base` at a class connected to its own writer role, +or at an entirely separate database, does **not** avoid this. `prevent_writes` +is enforced by the connection handler for the request as a whole, not per +model or per connection: verified against a live Rails application, +pointing `active_record_base` at the application's own writer-role class, +and separately at a wholly unrelated MySQL database, both still raised +`ActiveRecord::ReadOnlyError` on the write. `active_record_base` changes +which database this store's traffic goes to; it does not change whether +Rails currently permits writes at all. + +For the cache write specifically, the error is redacted -- see +[What ends up in your log](#what-ends-up-in-your-log) -- and it does not +reach your call to `translate` as an exception: the translator rescues it, +logs it, fires a `cache_error` instrumentation event (provider and error +class only, never content -- see [Instrumentation](instrumentation.md)), +and returns the translation anyway. What does not happen is the write: a +translation served on a GET beneath this middleware is not cached by this +store, for that request. + +**The rate limiter's own write is not covered by that same protection.** +If `config.rate_limiter = :active_record` and the same request hits it, +`ActiveRecordRateLimiter#check` raises a raw `ActiveRecord::ReadOnlyError` +-- not redacted, not rescued, and not a `TranslationDiff::Error` at all. +Since the rate limit check runs before the provider is ever called, the +`translate` call fails outright rather than degrading: verified against the +same live application. The row this table would have written never carries +translated content either way, so nothing confidential is in that raw +message -- but a `rescue TranslationDiff::Error` around `translate` will +not catch it, and no translation comes back. + +Two things actually avoid both failures, both checked directly against a +Rails application with `DatabaseSelector` configured: + +- **Translate somewhere `DatabaseSelector` is not wrapping.** A background + job, a POST action, a console session -- anywhere outside a GET this + middleware routes, there is no `prevent_writes` in effect to begin with. +- **Wrap the call to permit writes for its duration:** + ```ruby + ActiveRecord::Base.connected_to(role: :writing) do + TranslationDiff.translate(text, from: "en", to: "es") + end + ``` + ## The ActiveRecord version floor **ActiveRecord 7.1 or newer.** `upsert_all` needs `unique_by` on Postgres From c003673241fbdfe2284badc7b671f2fd035f6679 Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 01:58:52 +0400 Subject: [PATCH 07/10] fix: report a rate limiter's ActiveRecord failures as this gem's own error --- lib/translation_diff/active_record_cache_store.rb | 5 ----- lib/translation_diff/active_record_rate_limiter.rb | 12 ++++++++++++ lib/translation_diff/active_record_support.rb | 5 +++++ .../active_record_rate_limiter_test.rb | 13 +++++++++++++ 4 files changed, 30 insertions(+), 5 deletions(-) diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb index 91ce7ae..a4c4fc6 100644 --- a/lib/translation_diff/active_record_cache_store.rb +++ b/lib/translation_diff/active_record_cache_store.rb @@ -98,11 +98,6 @@ def prune_sometimes raise redacted_prune_error(e), cause: nil end - # Any ActiveRecordError, not just StatementInvalid -- ReadOnlyError carries a whole write statement too. - def ar_error?(error) - defined?(ActiveRecord::ActiveRecordError) && error.is_a?(ActiveRecord::ActiveRecordError) - end - def active_record_feature = "the cache" def active_record_component = "ActiveRecord cache store" def active_record_upsert_detail = "upsert_all takes unique_by and record_timestamps there." diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb index 6a03a4d..b37b1aa 100644 --- a/lib/translation_diff/active_record_rate_limiter.rb +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -35,6 +35,18 @@ def check(size) raise RateLimitExceeded, exceeded_message if current_total >= @threshold add(size) + rescue StandardError => e + raise unless ar_error?(e) + + raise redacted_error(e), cause: nil + end + + # The limiter's own statements carry counts, not content -- but a ReadOnlyError quotes the statement, and + # a raw ActiveRecord error from inside a translate call tells a caller nothing about which gem it came from. + def redacted_error(error) + adapter_error = error.cause&.class || error.class + TranslationDiff::Error.new("the rate limit check failed (#{adapter_error}): a read or upsert on " \ + "#{@table_name}(namespace, bucket, characters)") end # Counts and settings, never a character of what was being translated. diff --git a/lib/translation_diff/active_record_support.rb b/lib/translation_diff/active_record_support.rb index 8092d73..d25c6b0 100644 --- a/lib/translation_diff/active_record_support.rb +++ b/lib/translation_diff/active_record_support.rb @@ -8,6 +8,11 @@ def model private + # Any ActiveRecordError, not just StatementInvalid -- ReadOnlyError carries a whole write statement too. + def ar_error?(error) + defined?(ActiveRecord::ActiveRecordError) && error.is_a?(ActiveRecord::ActiveRecordError) + end + def build_model require "active_record" ensure_supported_version! diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb index acc29e3..eb4c989 100644 --- a/test/translation_diff/active_record_rate_limiter_test.rb +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -68,6 +68,19 @@ def test_a_bucket_beyond_int32_range_is_stored_and_read_back assert_equal far_future_bucket, model.find_by(namespace: "translation-diff").bucket end + # Rails' DatabaseSelector sets this on every GET, and the ReadOnlyError it raises quotes the statement. + def test_a_write_refused_by_rails_is_reported_as_this_gem_s_own_error + limiter = build_limiter(threshold: 1000, interval: 60) + + error = assert_raises(TranslationDiff::Error) do + ::ActiveRecord::Base.while_preventing_writes { limiter.check(10) } + end + + refute_instance_of ::ActiveRecord::ReadOnlyError, error + assert_match(/the rate limit check failed/, error.message) + assert_nil error.cause + end + def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one limiter = build_limiter(threshold: 1000, interval: 60, clock: frozen_clock) model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket) - 1, characters: 5) From fbeaf4d8a0177bb970406946361816d8bc85231f Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 01:59:28 +0400 Subject: [PATCH 08/10] docs: the rate limiter's database failures are redacted too now --- CHANGELOG.md | 8 ++++---- docs/errors.md | 19 ++++++++++--------- docs/sql-cache.md | 19 +++++++++---------- 3 files changed, 23 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 434c745..481b631 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,10 +89,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). 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). One gap remains: - `ActiveRecordRateLimiter`'s own write is not covered by this -- under the - same read-replica routing, it still raises a raw, un-rescued - `ActiveRecord::ReadOnlyError` that fails the `translate` call outright. + 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` diff --git a/docs/errors.md b/docs/errors.md index 16fec8f..56b8edc 100644 --- a/docs/errors.md +++ b/docs/errors.md @@ -55,15 +55,16 @@ 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. -**One documented exception to "every error this gem raises."** -`ActiveRecordRateLimiter#check`'s own write is not wrapped in this gem's -error handling at all: under Rails' `prevent_writes` (a read-replica -request, see [Rails replica routing](sql-cache.md#rails-replica-routing)), -it raises a raw `ActiveRecord::ReadOnlyError` straight through, uncaught -and un-redacted. `ActiveRecordCacheStore`'s write does not have this gap -- -its errors are redacted `TranslationDiff::Error`s, rescued before they ever -reach a caller. A `rescue TranslationDiff::Error` around `translate` does -not catch the rate limiter's version. +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 diff --git a/docs/sql-cache.md b/docs/sql-cache.md index 0f29c3c..50e7ffb 100644 --- a/docs/sql-cache.md +++ b/docs/sql-cache.md @@ -279,16 +279,15 @@ and returns the translation anyway. What does not happen is the write: a translation served on a GET beneath this middleware is not cached by this store, for that request. -**The rate limiter's own write is not covered by that same protection.** -If `config.rate_limiter = :active_record` and the same request hits it, -`ActiveRecordRateLimiter#check` raises a raw `ActiveRecord::ReadOnlyError` --- not redacted, not rescued, and not a `TranslationDiff::Error` at all. -Since the rate limit check runs before the provider is ever called, the -`translate` call fails outright rather than degrading: verified against the -same live application. The row this table would have written never carries -translated content either way, so nothing confidential is in that raw -message -- but a `rescue TranslationDiff::Error` around `translate` will -not catch it, and no translation comes back. +**The rate limiter fails differently, because it runs earlier.** If +`config.rate_limiter = :active_record` and the same request hits it, +`ActiveRecordRateLimiter#check` cannot record what it is about to allow, so +it raises `TranslationDiff::Error` naming the adapter's error class -- and +because the check runs before the provider is ever called, the `translate` +call fails outright rather than degrading. Nothing has been paid for at +that point, which is why this one refuses instead of continuing: a limiter +that cannot count is not a limiter, and quietly translating past it is how +an application loses its provider account. Two things actually avoid both failures, both checked directly against a Rails application with `DatabaseSelector` configured: From ab093210197c61f94494559424db9aff09739644 Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 02:12:27 +0400 Subject: [PATCH 09/10] fix: warn, not debug, when the cache stops accepting writes 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. --- docs/configuration.md | 2 +- docs/instrumentation.md | 16 +++++++++++++--- lib/translation_diff/instrumentation.rb | 5 +++++ lib/translation_diff/translator.rb | 2 +- test/translation_diff/translator_test.rb | 3 +++ 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 73c88ac..bca53cf 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -70,7 +70,7 @@ at all, so an unset environment variable never has to be special-cased. | `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. | diff --git a/docs/instrumentation.md b/docs/instrumentation.md index 25ab901..bf1d920 100644 --- a/docs/instrumentation.md +++ b/docs/instrumentation.md @@ -21,9 +21,19 @@ A translation emits up to six events, each named `.translation_diff`: 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 (`"ActiveRecord::ReadOnlyError"`, -`"Redis::CannotConnectError"`, ...), never its message, which could echo -the row it failed to write. +`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 diff --git a/lib/translation_diff/instrumentation.rb b/lib/translation_diff/instrumentation.rb index b251e3e..a3929f4 100644 --- a/lib/translation_diff/instrumentation.rb +++ b/lib/translation_diff/instrumentation.rb @@ -17,4 +17,9 @@ def instrument(name, payload = {}) def log(message) config.logger&.debug { "[translation_diff] #{message}" } end + + # For the things an operator must see at a production log level; carries no more content than #log does. + def warn_log(message) + config.logger&.warn { "[translation_diff] #{message}" } + end end diff --git a/lib/translation_diff/translator.rb b/lib/translation_diff/translator.rb index 6297eac..ad9d8f1 100644 --- a/lib/translation_diff/translator.rb +++ b/lib/translation_diff/translator.rb @@ -141,7 +141,7 @@ def fill(provider, segments, from) def store(cache, misses, provider) cache.store(misses) rescue StandardError => e - log("cache write failed (#{e.class}), the translation is returned uncached") + warn_log("cache write failed (#{e.class}), the translation is returned uncached") instrument("cache_error", provider: provider.cache_key, error: e.class.to_s) end end diff --git a/test/translation_diff/translator_test.rb b/test/translation_diff/translator_test.rb index eed9894..3c5a445 100644 --- a/test/translation_diff/translator_test.rb +++ b/test/translation_diff/translator_test.rb @@ -50,6 +50,9 @@ class FakeLogger def initialize = @lines = [] def debug(&) = @lines << yield + + # Public on purpose: Kernel#warn is private, and a logger double that inherits it silently swallows the call. + def warn(&) = @lines << yield end class Recorder From 1161602506ea5f3e4ef2b888309eb954249cdc2e Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 02:40:38 +0400 Subject: [PATCH 10/10] style: hoist the Redis limiter's threshold check out of the raise --- lib/translation_diff/redis_rate_limiter.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/translation_diff/redis_rate_limiter.rb b/lib/translation_diff/redis_rate_limiter.rb index af0a727..edbc827 100644 --- a/lib/translation_diff/redis_rate_limiter.rb +++ b/lib/translation_diff/redis_rate_limiter.rb @@ -31,8 +31,8 @@ def check(size) connection_pool.with do |redis| rate_limit = limiter_class.new(namespace, redis: redis) - raise RateLimitExceeded, exceeded_message if rate_limit.exceeded?(SUBJECT, threshold: threshold, - interval: interval) + exceeded = rate_limit.exceeded?(SUBJECT, threshold: threshold, interval: interval) + raise RateLimitExceeded, exceeded_message if exceeded rate_limit.add(SUBJECT, size) end