From 7024ea24ab75454f8b9115e890838d877c709c4e Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 17:15:13 +0400 Subject: [PATCH 01/36] feat: let a cache store write a batch in one call --- lib/translation_diff/memory_cache_store.rb | 4 ++ lib/translation_diff/redis_cache_store.rb | 7 +++ lib/translation_diff/sentence_cache.rb | 10 ++++- test/support/cache_store_contract.rb | 19 ++++++++ .../redis_cache_store_test.rb | 27 ++++++++++++ test/translation_diff/sentence_cache_test.rb | 43 +++++++++++++++++++ 6 files changed, 109 insertions(+), 1 deletion(-) diff --git a/lib/translation_diff/memory_cache_store.rb b/lib/translation_diff/memory_cache_store.rb index d7871aa..be7ba76 100644 --- a/lib/translation_diff/memory_cache_store.rb +++ b/lib/translation_diff/memory_cache_store.rb @@ -18,6 +18,10 @@ def write(key, value) value end + def write_multi(pairs) + pairs.each { |key, value| write(key, value) } + end + private def touch(key) diff --git a/lib/translation_diff/redis_cache_store.rb b/lib/translation_diff/redis_cache_store.rb index 9dde87d..a923639 100644 --- a/lib/translation_diff/redis_cache_store.rb +++ b/lib/translation_diff/redis_cache_store.rb @@ -21,6 +21,13 @@ def write(key, value) redis { |redis| redis.setex(key, timeout, value) } end + def write_multi(pairs) + return pairs if pairs.empty? + + redis { |redis| redis.pipelined { |p| pairs.each { |key, value| p.setex(key, timeout, value) } } } + pairs + end + private attr_reader :connection_pool, :timeout, :namespace diff --git a/lib/translation_diff/sentence_cache.rb b/lib/translation_diff/sentence_cache.rb index 9c18207..1d69831 100644 --- a/lib/translation_diff/sentence_cache.rb +++ b/lib/translation_diff/sentence_cache.rb @@ -31,12 +31,20 @@ def fill(segments) end # Writes back only the segments that carry a translation; an untranslated segment has nothing worth caching. + # A store that batches gets one call; one that does not keeps the per-key contract it was written against. def store(segments) - segments.select(&:translated?).each { |segment| @store.write(key(segment), segment.translation) } + translated = segments.select(&:translated?) + return translated.each { |segment| @store.write(key(segment), segment.translation) } if legacy_store? + + @store.write_multi(translated.map { |segment| [key(segment), segment.translation] }) + translated end private + # A store written against the write-only contract, before write_multi existed, cannot be handed a batch. + def legacy_store? = !@store.respond_to?(:write_multi) + # No options contributes no field at all, which is the four-field key every already-warm cache is keyed on. def options_digest return [] if @options.empty? diff --git a/test/support/cache_store_contract.rb b/test/support/cache_store_contract.rb index d094402..83910c7 100644 --- a/test/support/cache_store_contract.rb +++ b/test/support/cache_store_contract.rb @@ -22,4 +22,23 @@ def test_writing_the_same_key_twice_keeps_the_second_value assert_equal ["two"], store.read_multi(["a"]) end + + def test_write_multi_writes_every_pair + store.write_multi([%w[a one], %w[b two]]) + + assert_equal %w[one two], store.read_multi(%w[a b]) + end + + def test_write_multi_of_no_pairs_writes_nothing + store.write_multi([]) + + assert_empty store.read_multi([]) + end + + def test_write_multi_replaces_a_key_written_before + store.write("a", "one") + store.write_multi([%w[a two]]) + + assert_equal ["two"], store.read_multi(["a"]) + end end diff --git a/test/translation_diff/redis_cache_store_test.rb b/test/translation_diff/redis_cache_store_test.rb index 73c3029..055991c 100644 --- a/test/translation_diff/redis_cache_store_test.rb +++ b/test/translation_diff/redis_cache_store_test.rb @@ -17,6 +17,10 @@ def mget(*keys) def setex(key, timeout, value) @redis.setex("#{@namespace}:#{key}", timeout, value) end + + def pipelined + @redis.pipelined { |pipeline| yield self.class.new(@namespace, redis: pipeline) } + end end class RedisCacheStoreTest < Minitest::Test @@ -44,6 +48,11 @@ def setex(key, timeout, value) @entries[key] = value "OK" end + + def pipelined + @calls << [:pipelined] + yield self + end end attr_reader :store @@ -75,6 +84,24 @@ def test_write_honours_a_custom_timeout_and_namespace assert_equal [[:setex, "t:a", 60, "b"]], redis.calls end + def test_write_multi_sends_one_pipeline_rather_than_one_round_trip_per_key + redis = FakeRedis.new + + build_store(redis).write_multi([%w[a one], %w[b two]]) + + assert_equal [[:pipelined], + [:setex, "translation-diff:a", 604_800, "one"], + [:setex, "translation-diff:b", 604_800, "two"]], redis.calls + end + + def test_write_multi_of_no_pairs_never_opens_a_pipeline + redis = FakeRedis.new + + build_store(redis).write_multi([]) + + assert_empty redis.calls + end + private def build_store(redis, **) diff --git a/test/translation_diff/sentence_cache_test.rb b/test/translation_diff/sentence_cache_test.rb index c70f8bb..d29e9fc 100644 --- a/test/translation_diff/sentence_cache_test.rb +++ b/test/translation_diff/sentence_cache_test.rb @@ -20,6 +20,21 @@ def read_multi(keys) def write(key, value) = @writes[key] = value end + # Same recording behaviour as RecordingStore, plus write_multi, to prove the batched path is taken when offered. + class BatchingStore < RecordingStore + attr_reader :write_multi_calls + + def initialize(values = {}) + super + @write_multi_calls = [] + end + + def write_multi(pairs) + @write_multi_calls << pairs + pairs.each { |key, value| @writes[key] = value } + end + end + def cache(store, **) TranslationDiff::SentenceCache.new( store: store, provider: "deepl", from: "en", to: "ru", ** @@ -60,6 +75,34 @@ def test_store_writes_only_the_translated_ones assert_equal ["Один."], store.writes.values end + def test_store_batches_every_translated_segment_into_one_write_multi_call + subject = segments("One.", "Two.") + subject.first.translation = "Один." + subject.last.translation = "Два." + store = BatchingStore.new + subject_cache = cache(store) + + subject_cache.store(subject) + + assert_equal 1, store.write_multi_calls.size + assert_equal ["Один.", "Два."], store.write_multi_calls.first.map(&:last) + end + + # The compatibility guarantee: a custom store written against today's write-only contract keeps working untouched. + def test_store_falls_back_to_write_per_key_when_the_store_has_no_write_multi + subject = segments("One.", "Two.") + subject.first.translation = "Один." + subject.last.translation = "Два." + store = RecordingStore.new + refute_respond_to store, :write_multi + subject_cache = cache(store) + + subject_cache.store(subject) + + assert_equal({ subject_cache.key(subject.first) => "Один.", subject_cache.key(subject.last) => "Два." }, + store.writes) + end + # The bug this replaces: the old cache consumed the array of updates it was # handed, emptying a collection that belonged to its caller. def test_neither_operation_modifies_the_collection_it_is_given From 9322395f093840fcf3cdb7e1e423eefe1c394ecb Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 17:26:44 +0400 Subject: [PATCH 02/36] fix: never hand a batching cache store an empty batch --- lib/translation_diff/sentence_cache.rb | 1 + test/translation_diff/sentence_cache_test.rb | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/translation_diff/sentence_cache.rb b/lib/translation_diff/sentence_cache.rb index 1d69831..2a298eb 100644 --- a/lib/translation_diff/sentence_cache.rb +++ b/lib/translation_diff/sentence_cache.rb @@ -34,6 +34,7 @@ def fill(segments) # A store that batches gets one call; one that does not keeps the per-key contract it was written against. def store(segments) translated = segments.select(&:translated?) + return translated if translated.empty? return translated.each { |segment| @store.write(key(segment), segment.translation) } if legacy_store? @store.write_multi(translated.map { |segment| [key(segment), segment.translation] }) diff --git a/test/translation_diff/sentence_cache_test.rb b/test/translation_diff/sentence_cache_test.rb index d29e9fc..f0ec3f0 100644 --- a/test/translation_diff/sentence_cache_test.rb +++ b/test/translation_diff/sentence_cache_test.rb @@ -77,15 +77,24 @@ def test_store_writes_only_the_translated_ones def test_store_batches_every_translated_segment_into_one_write_multi_call subject = segments("One.", "Two.") - subject.first.translation = "Один." - subject.last.translation = "Два." + subject.zip(%w[Один. Два.]).each { |segment, translation| segment.translation = translation } store = BatchingStore.new subject_cache = cache(store) subject_cache.store(subject) + expected = subject.map { |segment| [subject_cache.key(segment), segment.translation] } assert_equal 1, store.write_multi_calls.size - assert_equal ["Один.", "Два."], store.write_multi_calls.first.map(&:last) + assert_equal expected, store.write_multi_calls.first + end + + # A batch of nothing is not a batch: a store that opens a transaction in write_multi must not be asked to. + def test_store_never_calls_a_batching_store_when_nothing_was_translated + store = BatchingStore.new + + cache(store).store(segments("One.")) + + assert_empty store.write_multi_calls end # The compatibility guarantee: a custom store written against today's write-only contract keeps working untouched. From e5f1ac9ef1898cb8e45fadbd4571b9ab52716b76 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 17:26:45 +0400 Subject: [PATCH 03/36] feat: cache translations in the application's own database --- Gemfile | 7 ++ lib/translation_diff.rb | 2 + .../active_record_cache_store.rb | 90 ++++++++++++++++++ lib/translation_diff/configuration.rb | 4 + test/support/active_record_database.rb | 58 +++++++++++ .../active_record_cache_store_test.rb | 95 +++++++++++++++++++ 6 files changed, 256 insertions(+) create mode 100644 lib/translation_diff/active_record_cache_store.rb create mode 100644 test/support/active_record_database.rb create mode 100644 test/translation_diff/active_record_cache_store_test.rb diff --git a/Gemfile b/Gemfile index 281e85e..c865840 100644 --- a/Gemfile +++ b/Gemfile @@ -31,3 +31,10 @@ gem "cgi", "~> 0.5", require: false # the test suite, which signs against the real library rather than a # stand-in, has it available. gem "aws-sigv4", "~> 1.12", require: false + +# Not runtime dependencies of the gem (see the gemspec) -- ActiveRecordCacheStore +# and ActiveRecordRateLimiter require active_record lazily on first use, so an +# application caching in Redis never needs it installed. They are here so the +# suite can exercise the stores against a real database rather than a stand-in. +gem "activerecord", "~> 8.1", require: false +gem "sqlite3", "~> 2.9", require: false diff --git a/lib/translation_diff.rb b/lib/translation_diff.rb index 4918118..eedc999 100644 --- a/lib/translation_diff.rb +++ b/lib/translation_diff.rb @@ -1,5 +1,6 @@ require "cgi/escape" require "digest/md5" +require "digest/sha2" require "forwardable" require "stringio" @@ -47,6 +48,7 @@ require "translation_diff/stores" require "translation_diff/memory_cache_store" require "translation_diff/redis_cache_store" +require "translation_diff/active_record_cache_store" require "translation_diff/redis_rate_limiter" require "translation_diff/instrumentation" require "translation_diff/dispatcher" diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb new file mode 100644 index 0000000..531bc57 --- /dev/null +++ b/lib/translation_diff/active_record_cache_store.rb @@ -0,0 +1,90 @@ +# Caches translations in the application's own database; ActiveRecord is required on first use, never at load. +class TranslationDiff::ActiveRecordCacheStore + MINIMUM_ACTIVE_RECORD = "7.1".freeze + + def self.build(config) + new(namespace: config.cache_namespace, ttl: config.cache_ttl, + table_name: config.cache_table_name, base: config.active_record_base, + prune_probability: config.cache_prune_probability) + end + + def initialize(namespace:, ttl:, table_name:, base: nil, prune_probability: 0.0) + @namespace = namespace + @ttl = ttl + @table_name = table_name + @base = base + @prune_probability = prune_probability + end + + # One query, then the caller's order restored -- a missing or expired key is a nil in its own position. + def read_multi(keys) + return [] if keys.empty? + + digests = keys.map { |key| digest(key) } + found = live.where(key_digest: digests).pluck(:key_digest, :translation).to_h + digests.map { |d| found[d] } + end + + def write(key, value) + write_multi([[key, value]]) + value + end + + # One upsert for the whole batch; the unique index makes the second write of a key replace the first. + def write_multi(pairs) + return pairs if pairs.empty? + + model.upsert_all(pairs.map { |key, value| row(key, value) }, + unique_by: %i[namespace key_digest], record_timestamps: true) + prune_sometimes + pairs + end + + # Reads never serve an expired row; deleting one is this, and it is the host's call when to run it. + def prune = model.where(namespace: @namespace).where(expires_at: ...Time.now.utc).delete_all + + def model + @model ||= build_model + end + + private + + def row(key, value) + { namespace: @namespace, key_digest: digest(key), translation: value, expires_at: expires_at } + end + + def expires_at = @ttl.nil? ? nil : Time.now.utc + @ttl + + # SHA256 hex is 64 characters whatever the key was, which is what makes the unique index portable. + def digest(key) = Digest::SHA256.hexdigest(key.to_s) + + def live + model.where(namespace: @namespace) + .where(expires_at: nil).or(model.where(namespace: @namespace).where(expires_at: Time.now.utc...)) + end + + def prune_sometimes + prune if @prune_probability.positive? && rand < @prune_probability + end + + def build_model + require "active_record" + ensure_supported_version! + table = @table_name + Class.new(@base || ::ActiveRecord::Base) { self.table_name = table } + rescue LoadError + raise TranslationDiff::Error, + "the cache is :active_record but the `activerecord` gem is not available. " \ + 'Add `gem "activerecord"` to your Gemfile.' + end + + def ensure_supported_version! + return if Gem::Version.new(::ActiveRecord::VERSION::STRING) >= Gem::Version.new(MINIMUM_ACTIVE_RECORD) + + raise TranslationDiff::Error, + "the ActiveRecord cache store needs ActiveRecord #{MINIMUM_ACTIVE_RECORD} or newer " \ + "(found #{::ActiveRecord::VERSION::STRING}): upsert_all takes unique_by and record_timestamps there." + end +end + +TranslationDiff::Stores.register(:active_record, TranslationDiff::ActiveRecordCacheStore) diff --git a/lib/translation_diff/configuration.rb b/lib/translation_diff/configuration.rb index 66f92fb..b4c4140 100644 --- a/lib/translation_diff/configuration.rb +++ b/lib/translation_diff/configuration.rb @@ -44,6 +44,10 @@ def provider_option_owners = @provider_option_owners ||= ProviderOptionOwners.ne option :cache_ttl, 604_800 option :cache_namespace, "translation-diff" option :cache_max_size, 1_000 + option :cache_table_name, "translation_diff_translations" + option :rate_limit_table_name, "translation_diff_rate_limits" + option :active_record_base, nil + option :cache_prune_probability, 0.0 option :redis_url, -> { ENV.fetch("REDIS_URL", nil) } option :redis_pool_size, 5 option :redis_pool_timeout, 5 diff --git a/test/support/active_record_database.rb b/test/support/active_record_database.rb new file mode 100644 index 0000000..10d4e1e --- /dev/null +++ b/test/support/active_record_database.rb @@ -0,0 +1,58 @@ +# SQLite by default so the suite needs no service; TRANSLATION_DIFF_DATABASE_URL points it at Postgres in CI. +module ActiveRecordDatabase + def self.available? + return @available if defined?(@available) + + @available = begin + require "active_record" + true + rescue LoadError + false + end + end + + def self.connect! + ::ActiveRecord::Base.establish_connection(url || { adapter: "sqlite3", database: ":memory:" }) + define_schema + end + + def self.url = ENV.fetch("TRANSLATION_DIFF_DATABASE_URL", nil) + + # 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 + return if connection.table_exists?(:translation_diff_translations) + + define_translations_table(connection) + define_rate_limits_table(connection) + end + + 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.datetime :expires_at + t.timestamps + end + connection.add_index :translation_diff_translations, %i[namespace key_digest], + unique: true, name: "index_translation_diff_translations_on_key" + connection.add_index :translation_diff_translations, :expires_at + end + + def self.define_rate_limits_table(connection) + connection.create_table :translation_diff_rate_limits do |t| + t.string :namespace, null: false, limit: 64 + t.integer :bucket, null: false + t.integer :characters, null: false, default: 0 + end + connection.add_index :translation_diff_rate_limits, %i[namespace bucket], + unique: true, name: "index_translation_diff_rate_limits_on_bucket" + end + + # Wipes both tables between tests; a fresh in-memory SQLite has nothing else to reset. + def self.truncate + ::ActiveRecord::Base.connection.execute("DELETE FROM translation_diff_translations") + ::ActiveRecord::Base.connection.execute("DELETE FROM translation_diff_rate_limits") + end +end diff --git a/test/translation_diff/active_record_cache_store_test.rb b/test/translation_diff/active_record_cache_store_test.rb new file mode 100644 index 0000000..ff8dab6 --- /dev/null +++ b/test/translation_diff/active_record_cache_store_test.rb @@ -0,0 +1,95 @@ +require "test_helper" +require "support/cache_store_contract" +require "support/active_record_database" + +if ActiveRecordDatabase.available? + ActiveRecordDatabase.connect! + + class ActiveRecordCacheStoreTest < Minitest::Test + include CacheStoreContract + + attr_reader :store + + def setup + ActiveRecordDatabase.truncate + @store = build_store + end + + def model = store.model + + def digest_of(key) = Digest::SHA256.hexdigest(key) + + def test_an_expired_row_is_not_read + store.write("a", "one") + model.update_all(expires_at: Time.now.utc - 1) + + assert_equal [nil], store.read_multi(["a"]) + end + + def test_two_namespaces_do_not_see_each_other + store.write("a", "one") + other = build_store(namespace: "other-tenant") + + assert_equal [nil], other.read_multi(["a"]) + end + + def test_the_cache_key_itself_is_never_stored + store.write("deepl:en:ru:abc", "one") + + refute_includes model.first.attributes.values.join, "deepl:en:ru:abc" + end + + def test_prune_deletes_expired_rows_and_leaves_live_ones + store.write("live", "one") + store.write("dead", "two") + model.where(key_digest: digest_of("dead")).update_all(expires_at: Time.now.utc - 1) + + assert_equal 1, store.prune + assert_equal ["one"], store.read_multi(["live"]) + end + + def test_a_nil_cache_ttl_writes_a_row_that_never_expires + build_store(ttl: nil).write("a", "one") + + assert_nil model.first.expires_at + end + + def test_prune_only_deletes_rows_in_its_own_namespace + other = build_store(namespace: "other-tenant") + expire(store, "a") + expire(other, "b") + + assert_equal 1, store.prune + assert_equal 1, model.count + end + + def test_build_takes_its_settings_from_the_configuration + config = TranslationDiff::Configuration.new + config.cache_namespace = "from-config" + config.cache_table_name = "translation_diff_translations" + + built = TranslationDiff::ActiveRecordCacheStore.build(config) + built.write("a", "one") + + assert_equal "from-config", built.model.first.namespace + end + + private + + def build_store(namespace: "translation-diff", ttl: 604_800) + TranslationDiff::ActiveRecordCacheStore.new(namespace: namespace, ttl: ttl, + table_name: "translation_diff_translations") + end + + def expire(store, key) + store.write(key, key) + store.model.where(key_digest: digest_of(key)).update_all(expires_at: Time.now.utc - 1) + end + end +else + class ActiveRecordCacheStoreTest < Minitest::Test + def test_active_record_is_unavailable + skip "active_record could not be loaded on this Ruby; the SQL cache store suite is skipped" + end + end +end From e6e7c3a6a94a6cedb84937d473690820818abeef Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 17:34:23 +0400 Subject: [PATCH 04/36] fix: dedupe write_multi by key before the upsert A batch with the same key twice made PostgreSQL raise PG::CardinalityViolation (ON CONFLICT DO UPDATE cannot affect the same row twice); SQLite silently accepted it with last-value-wins. write_multi now collapses pairs to one entry per key, keeping the last value, immediately before upsert_all, matching the last-write-wins semantics the contract already promises. --- lib/translation_diff/active_record_cache_store.rb | 2 +- .../active_record_cache_store_test.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb index 531bc57..15626e0 100644 --- a/lib/translation_diff/active_record_cache_store.rb +++ b/lib/translation_diff/active_record_cache_store.rb @@ -34,7 +34,7 @@ def write(key, value) def write_multi(pairs) return pairs if pairs.empty? - model.upsert_all(pairs.map { |key, value| row(key, value) }, + model.upsert_all(pairs.to_h.map { |key, value| row(key, value) }, unique_by: %i[namespace key_digest], record_timestamps: true) prune_sometimes pairs diff --git a/test/translation_diff/active_record_cache_store_test.rb b/test/translation_diff/active_record_cache_store_test.rb index ff8dab6..f8f6244 100644 --- a/test/translation_diff/active_record_cache_store_test.rb +++ b/test/translation_diff/active_record_cache_store_test.rb @@ -63,6 +63,19 @@ def test_prune_only_deletes_rows_in_its_own_namespace assert_equal 1, model.count end + def test_write_multi_with_the_same_key_twice_in_one_batch_stores_the_last_value + store.write_multi([%w[a one], %w[a two]]) + + assert_equal ["two"], store.read_multi(["a"]) + end + + def test_write_after_write_multi_still_replaces_the_key + store.write_multi([%w[a one], %w[a two]]) + store.write("a", "three") + + assert_equal ["three"], store.read_multi(["a"]) + end + def test_build_takes_its_settings_from_the_configuration config = TranslationDiff::Configuration.new config.cache_namespace = "from-config" From 6d34ab4985340ca5035619386ee0bb0daf73a98e Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 17:36:20 +0400 Subject: [PATCH 05/36] feat: ship the migration as a generator and a prune task --- Rakefile | 16 +++++ .../translation_diff/install_generator.rb | 14 +++++ .../create_translation_diff_tables.rb.erb | 24 +++++++ .../install_generator_test.rb | 63 +++++++++++++++++++ 4 files changed, 117 insertions(+) create mode 100644 lib/generators/translation_diff/install_generator.rb create mode 100644 lib/generators/translation_diff/templates/create_translation_diff_tables.rb.erb create mode 100644 test/translation_diff/install_generator_test.rb diff --git a/Rakefile b/Rakefile index ef05ed5..7a6f127 100644 --- a/Rakefile +++ b/Rakefile @@ -36,3 +36,19 @@ namespace :languages do report[:failed].each { |name, message| warn "failed: #{name}: #{message}" } end end + +namespace :translation_diff do + desc "Delete expired rows from the SQL cache and rate-limit tables" + task :prune do + require "translation_diff" + + store = TranslationDiff.config.cache_store + unless store.respond_to?(:prune) + puts "the configured cache store (#{store.class}) does not support pruning" + next + end + + deleted = store.prune + puts "pruned #{deleted} expired cache rows" + end +end diff --git a/lib/generators/translation_diff/install_generator.rb b/lib/generators/translation_diff/install_generator.rb new file mode 100644 index 0000000..ff68860 --- /dev/null +++ b/lib/generators/translation_diff/install_generator.rb @@ -0,0 +1,14 @@ +# Loaded only when Rails loads generators; nothing in lib/translation_diff.rb requires this file. +require "rails/generators" +require "rails/generators/active_record/migration" + +# `rails generate translation_diff:install` -- writes the migration for both SQL-backed tables. +class TranslationDiff::InstallGenerator < Rails::Generators::Base + include ActiveRecord::Generators::Migration + + source_root File.expand_path("templates", __dir__) + + def create_migration_file + migration_template "create_translation_diff_tables.rb.erb", "db/migrate/create_translation_diff_tables.rb" + end +end 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 new file mode 100644 index 0000000..6e7d700 --- /dev/null +++ b/lib/generators/translation_diff/templates/create_translation_diff_tables.rb.erb @@ -0,0 +1,24 @@ +class CreateTranslationDiffTables < ActiveRecord::Migration[<%= ActiveRecord::Migration.current_version %>] + def change + 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.datetime :expires_at + t.timestamps + end + + add_index :translation_diff_translations, %i[namespace key_digest], unique: true, + name: "index_translation_diff_translations_on_key" + add_index :translation_diff_translations, :expires_at + + create_table :translation_diff_rate_limits do |t| + t.string :namespace, null: false, limit: 64 + t.integer :bucket, null: false + t.integer :characters, null: false, default: 0 + end + + add_index :translation_diff_rate_limits, %i[namespace bucket], unique: true, + name: "index_translation_diff_rate_limits_on_bucket" + end +end diff --git a/test/translation_diff/install_generator_test.rb b/test/translation_diff/install_generator_test.rb new file mode 100644 index 0000000..57bfb7a --- /dev/null +++ b/test/translation_diff/install_generator_test.rb @@ -0,0 +1,63 @@ +require "test_helper" +require "support/active_record_database" +require "tmpdir" + +begin + require "generators/translation_diff/install_generator" + RAILS_GENERATORS_AVAILABLE = true +rescue LoadError + RAILS_GENERATORS_AVAILABLE = false +end + +if RAILS_GENERATORS_AVAILABLE && ActiveRecordDatabase.available? + ActiveRecordDatabase.connect! + + # Its own connection, so applying the generated migration never touches the harness's shared one. + class GeneratedMigrationRecord < ActiveRecord::Base + self.abstract_class = true + end + + class InstallGeneratorTest < Minitest::Test + def test_the_generated_migration_matches_the_harness_schema + Dir.mktmpdir do |dir| + migrated = migrate_in(dir) + + %i[translation_diff_translations translation_diff_rate_limits].each do |table| + assert_equal schema_of(::ActiveRecord::Base.connection, table), schema_of(migrated, table) + end + end + end + + private + + def migrate_in(dir) + generator = TranslationDiff::InstallGenerator.new([], {}, destination_root: dir) + capture_io { generator.invoke_all } + + connection = isolated_connection + require migration_file(dir) + capture_io { CreateTranslationDiffTables.new.exec_migration(connection, :up) } + connection + end + + def isolated_connection + GeneratedMigrationRecord.establish_connection(adapter: "sqlite3", database: ":memory:") + GeneratedMigrationRecord.connection + end + + def migration_file(dir) + Dir.glob(File.join(dir, "db/migrate/*_create_translation_diff_tables.rb")).first + end + + def schema_of(connection, table) + { columns: connection.columns(table).map { |c| [c.name, c.sql_type, c.null, c.default] }.sort, + indexes: connection.indexes(table).map { |i| [i.columns.sort, i.unique] }.sort_by(&:to_s) } + end + end +else + class InstallGeneratorTest < Minitest::Test + def test_rails_generators_are_unavailable + skip "Rails' generator classes could not be loaded; the install generator suite is skipped" + end + end +end From 3c563254a519f07464c1cae5fd3240bec5ee6055 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 17:49:06 +0400 Subject: [PATCH 06/36] feat: rate limit against the database instead of Redis An application on SQL could cache but not throttle, and the throttle is what keeps a provider from cutting it off. Adds ActiveRecordRateLimiter (namespaced, time-bucketed rows, upserted in one guarded statement) and a RateLimiters registry beside Stores, so config.rate_limiter now resolves :redis/:active_record by name the same way cache and segmenter already do, while still accepting an object and still costing nothing when unset. --- lib/translation_diff.rb | 2 + .../active_record_rate_limiter.rb | 70 ++++++++++++ lib/translation_diff/configuration.rb | 5 +- lib/translation_diff/rate_limiters.rb | 2 + lib/translation_diff/redis_rate_limiter.rb | 2 + test/support/rate_limiter_contract.rb | 24 ++++ .../active_record_rate_limiter_test.rb | 103 ++++++++++++++++++ test/translation_diff/configuration_test.rb | 24 ++++ .../redis_rate_limiter_test.rb | 16 +++ 9 files changed, 245 insertions(+), 3 deletions(-) create mode 100644 lib/translation_diff/active_record_rate_limiter.rb create mode 100644 lib/translation_diff/rate_limiters.rb create mode 100644 test/support/rate_limiter_contract.rb create mode 100644 test/translation_diff/active_record_rate_limiter_test.rb diff --git a/lib/translation_diff.rb b/lib/translation_diff.rb index eedc999..08a7bc0 100644 --- a/lib/translation_diff.rb +++ b/lib/translation_diff.rb @@ -49,7 +49,9 @@ require "translation_diff/memory_cache_store" require "translation_diff/redis_cache_store" require "translation_diff/active_record_cache_store" +require "translation_diff/rate_limiters" require "translation_diff/redis_rate_limiter" +require "translation_diff/active_record_rate_limiter" require "translation_diff/instrumentation" require "translation_diff/dispatcher" require "translation_diff/translator" diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb new file mode 100644 index 0000000..65abf20 --- /dev/null +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -0,0 +1,70 @@ +# Throttles by counting characters into namespaced, time-bucketed rows in the application's own database. +class TranslationDiff::ActiveRecordRateLimiter + class RateLimitExceeded < TranslationDiff::Error; end + + DEFAULT_THRESHOLD = 8000 + DEFAULT_INTERVAL = 60 + + 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) + end + + def initialize(namespace:, table_name:, threshold: DEFAULT_THRESHOLD, interval: DEFAULT_INTERVAL, base: nil) + @namespace = namespace + @table_name = table_name + @threshold = threshold + @interval = interval + @base = base + end + + # Approximate at a window boundary, the same way the `ratelimit` gem this replaces is. + def check(size) + raise RateLimitExceeded if current_total >= @threshold + + add(size) + end + + # Buckets from before the current window; the host decides when, if ever, this runs. + def prune = model.where(namespace: @namespace).where(bucket: ...bucket).delete_all + + def model + @model ||= build_model + end + + private + + def current_total = model.where(namespace: @namespace, bucket: bucket).sum(:characters) + + def bucket = Time.now.to_i / @interval + + # One statement, so two processes incrementing the same bucket cannot lose an increment between them. + def add(size) + size = size.to_i + model.upsert_all([{ namespace: @namespace, bucket: bucket, characters: size }], + unique_by: %i[namespace bucket], + on_duplicate: Arel.sql("characters = #{model.table_name}.characters + #{size}")) + end + + def build_model + require "active_record" + ensure_supported_version! + table = @table_name + Class.new(@base || ::ActiveRecord::Base) { self.table_name = table } + rescue LoadError + raise TranslationDiff::Error, + "the rate limiter is :active_record but the `activerecord` gem is not available. " \ + 'Add `gem "activerecord"` to your Gemfile.' + end + + def ensure_supported_version! + minimum = TranslationDiff::ActiveRecordCacheStore::MINIMUM_ACTIVE_RECORD + return if Gem::Version.new(::ActiveRecord::VERSION::STRING) >= Gem::Version.new(minimum) + + raise TranslationDiff::Error, + "the ActiveRecord rate limiter needs ActiveRecord #{minimum} or newer " \ + "(found #{::ActiveRecord::VERSION::STRING}): upsert_all takes unique_by there." + end +end + +TranslationDiff::RateLimiters.register(:active_record, TranslationDiff::ActiveRecordRateLimiter) diff --git a/lib/translation_diff/configuration.rb b/lib/translation_diff/configuration.rb index b4c4140..24cc40b 100644 --- a/lib/translation_diff/configuration.rb +++ b/lib/translation_diff/configuration.rb @@ -91,10 +91,9 @@ def segmenter_instance # nil, not a null object: Dispatcher#throttle checks for nil and skips rate-limiting -- costs nothing normally. def rate_limiter_instance - return rate_limiter unless rate_limiter.nil? - return nil if rate_limit.nil? + return nil if rate_limiter.nil? && rate_limit.nil? - @rate_limiter_instance ||= TranslationDiff::RedisRateLimiter.build(self) + @rate_limiter_instance ||= resolve(rate_limiter || :redis, TranslationDiff::RateLimiters) end # One pool shared by the cache store and the rate limiter; callers used to build and pass it by hand. diff --git a/lib/translation_diff/rate_limiters.rb b/lib/translation_diff/rate_limiters.rb new file mode 100644 index 0000000..5ae854e --- /dev/null +++ b/lib/translation_diff/rate_limiters.rb @@ -0,0 +1,2 @@ +# Rate limiters, by name; assigning an object to `config.rate_limiter` bypasses this entirely. +TranslationDiff::RateLimiters = TranslationDiff::Registry.new("rate limiter") diff --git a/lib/translation_diff/redis_rate_limiter.rb b/lib/translation_diff/redis_rate_limiter.rb index fd42ed1..ff1d895 100644 --- a/lib/translation_diff/redis_rate_limiter.rb +++ b/lib/translation_diff/redis_rate_limiter.rb @@ -51,3 +51,5 @@ def ratelimit_class 'Add `gem "ratelimit"` to your Gemfile.' end end + +TranslationDiff::RateLimiters.register(:redis, TranslationDiff::RedisRateLimiter) diff --git a/test/support/rate_limiter_contract.rb b/test/support/rate_limiter_contract.rb new file mode 100644 index 0000000..d92f59d --- /dev/null +++ b/test/support/rate_limiter_contract.rb @@ -0,0 +1,24 @@ +# What every rate limiter must do; a contract only one implementation runs is not a contract. +module RateLimiterContract + def test_a_check_under_the_threshold_passes + build_limiter(threshold: 100, interval: 60).check(50) + + assert true # reaching here means #check did not raise + end + + def test_checks_summing_past_the_threshold_raise + build_limiter(threshold: 100, interval: 60).check(70) + build_limiter(threshold: 100, interval: 60).check(30) + + assert_raises(rate_limit_exceeded_error) { build_limiter(threshold: 100, interval: 60).check(1) } + end + + def test_a_window_that_has_rolled_over_passes_again + build_limiter(threshold: 10, interval: rollover_interval).check(10) + assert_raises(rate_limit_exceeded_error) { build_limiter(threshold: 10, interval: rollover_interval).check(1) } + + sleep(rollover_wait) + + build_limiter(threshold: 10, interval: rollover_interval).check(1) + end +end diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb new file mode 100644 index 0000000..1b6824d --- /dev/null +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -0,0 +1,103 @@ +require "test_helper" +require "support/rate_limiter_contract" +require "support/active_record_database" + +if ActiveRecordDatabase.available? + ActiveRecordDatabase.connect! + + class ActiveRecordRateLimiterTest < Minitest::Test + include RateLimiterContract + + def setup + ActiveRecordDatabase.truncate + end + + def model + TranslationDiff::ActiveRecordRateLimiter.new(namespace: "translation-diff", + table_name: "translation_diff_rate_limits").model + end + + def test_two_limiters_sharing_a_namespace_see_each_others_characters + first = build_limiter(threshold: 100, interval: 60) + second = build_limiter(threshold: 100, interval: 60) + + first.check(60) + second.check(40) + + assert_raises(rate_limit_exceeded_error) { first.check(1) } + end + + def test_two_namespaces_do_not_see_each_other + tenant_a = build_limiter(threshold: 100, interval: 60, namespace: "tenant-a") + tenant_b = build_limiter(threshold: 100, interval: 60, namespace: "tenant-b") + + tenant_a.check(90) + tenant_b.check(90) # would raise if the namespaces were not isolated + + assert true + end + + def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one + limiter = build_limiter(threshold: 1000, interval: 60) + model.create!(namespace: "translation-diff", bucket: limiter.send(:bucket) - 1, characters: 5) + + limiter.check(10) + deleted = limiter.prune + + assert_equal 1, deleted + assert_equal [limiter.send(:bucket)], model.pluck(:bucket) + end + + def test_prune_only_deletes_rows_in_its_own_namespace + own = build_limiter(threshold: 1000, interval: 60) + other = build_limiter(threshold: 1000, interval: 60, namespace: "other-tenant") + model.create!(namespace: "translation-diff", bucket: own.send(:bucket) - 1, characters: 5) + model.create!(namespace: "other-tenant", bucket: other.send(:bucket) - 1, characters: 5) + + assert_equal 1, own.prune + assert_equal 1, model.where(namespace: "other-tenant").count + end + + # `size` is interpolated into the on_duplicate SQL fragment, so a value with no clean integer must not reach it. + def test_a_non_integer_size_only_contributes_its_leading_digits + limiter = build_limiter(threshold: 1_000, interval: 60) + + limiter.check("5); DROP TABLE translation_diff_rate_limits; --") + + assert_equal 5, model.sum(:characters) + assert_equal [5], model.pluck(:characters) + end + + def test_build_takes_its_settings_from_the_configuration + config = TranslationDiff::Configuration.new + config.cache_namespace = "from-config" + config.rate_limit_table_name = "translation_diff_rate_limits" + config.rate_limit = 100 + config.rate_interval = 60 + + built = TranslationDiff::ActiveRecordRateLimiter.build(config) + built.check(1) + + assert_equal ["from-config"], built.model.pluck(:namespace) + end + + private + + def rate_limit_exceeded_error = TranslationDiff::ActiveRecordRateLimiter::RateLimitExceeded + + # bucket == Time.now.to_i / interval, so a one-second interval rolls over almost immediately. + def rollover_interval = 1 + def rollover_wait = 2.2 + + def build_limiter(threshold:, interval:, namespace: "translation-diff") + TranslationDiff::ActiveRecordRateLimiter.new(namespace: namespace, threshold: threshold, interval: interval, + table_name: "translation_diff_rate_limits") + end + end +else + class ActiveRecordRateLimiterTest < Minitest::Test + def test_active_record_is_unavailable + skip "active_record could not be loaded on this Ruby; the SQL rate limiter suite is skipped" + end + end +end diff --git a/test/translation_diff/configuration_test.rb b/test/translation_diff/configuration_test.rb index d6bb527..a79a247 100644 --- a/test/translation_diff/configuration_test.rb +++ b/test/translation_diff/configuration_test.rb @@ -316,6 +316,30 @@ def test_a_rate_limit_builds_a_redis_rate_limiter assert_instance_of TranslationDiff::RedisRateLimiter, @config.rate_limiter_instance end + def test_a_symbol_rate_limiter_resolves_through_the_registry + @config.rate_limit = 100 + @config.rate_limiter = :active_record + + assert_instance_of TranslationDiff::ActiveRecordRateLimiter, @config.rate_limiter_instance + end + + def test_a_string_rate_limiter_resolves_through_the_registry + @config.rate_limit = 100 + @config.rate_limiter = "active_record" + + assert_instance_of TranslationDiff::ActiveRecordRateLimiter, @config.rate_limiter_instance + end + + def test_an_unknown_rate_limiter_name_raises_listing_what_is_registered + @config.rate_limit = 100 + @config.rate_limiter = :nonsense + + error = assert_raises(TranslationDiff::Error) { @config.rate_limiter_instance } + + assert_includes error.message, "rate limiter" + assert_includes error.message, "redis" + end + def test_an_assigned_rate_limiter_object_wins_over_every_value limiter = Object.new @config.rate_limiter = limiter diff --git a/test/translation_diff/redis_rate_limiter_test.rb b/test/translation_diff/redis_rate_limiter_test.rb index 17c5ae5..53712e9 100644 --- a/test/translation_diff/redis_rate_limiter_test.rb +++ b/test/translation_diff/redis_rate_limiter_test.rb @@ -1,9 +1,12 @@ require "test_helper" +require "support/rate_limiter_contract" # A prior stand-in here hid a real defect: `add(size)` counted under the wrong subject and the limit never fired. require "ratelimit" class RedisRateLimiterTest < Minitest::Test + include RateLimiterContract + # An in-memory Redis server implementing exactly the commands ratelimit 1.1 issues; no socket is opened. class FakeRedisServer attr_reader :hashes, :expiries, :count_spans @@ -130,4 +133,17 @@ def test_a_missing_ratelimit_gem_raises_a_translation_diff_error def limiter(server, **) TranslationDiff::RedisRateLimiter.new(FakeConnectionPool.new(server), **) end + + # Ratelimit's own bucket_interval is fixed at 5 seconds and is not configurable through this gem. + def rollover_interval = 5 + + # Two full 5-second buckets, so the boundary crosses regardless of where in a bucket the first check landed. + def rollover_wait = 10 + + def rate_limit_exceeded_error = TranslationDiff::RedisRateLimiter::RateLimitExceeded + + def build_limiter(threshold:, interval:) + @contract_server ||= FakeRedisServer.new + limiter(@contract_server, threshold: threshold, interval: interval) + end end From b60473c2e3f93ac0d34a5f9a0b89b6948fdb085a Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 17:53:03 +0400 Subject: [PATCH 07/36] fix: require translation_diff from the install generator Loading lib/generators/translation_diff/install_generator.rb on its own raised NameError: uninitialized constant TranslationDiff, since it reopens TranslationDiff::InstallGenerator without the library loaded. --- lib/generators/translation_diff/install_generator.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/generators/translation_diff/install_generator.rb b/lib/generators/translation_diff/install_generator.rb index ff68860..f245cf6 100644 --- a/lib/generators/translation_diff/install_generator.rb +++ b/lib/generators/translation_diff/install_generator.rb @@ -1,4 +1,5 @@ # Loaded only when Rails loads generators; nothing in lib/translation_diff.rb requires this file. +require "translation_diff" require "rails/generators" require "rails/generators/active_record/migration" From 88e306a08f10b87eac4feaba958ced6849d54c2b Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 17:53:06 +0400 Subject: [PATCH 08/36] test: add railties to the dev Gemfile so the generator suite runs install_generator_test.rb was skipping in CI on every run for want of Rails::Generators::Base -- the only test standing between the shipped migration and the schema the rest of the suite runs against. railties is Gemfile-only and require: false, same as activerecord and sqlite3; Rails is still not a dependency of the gem. --- Gemfile | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Gemfile b/Gemfile index c865840..1e05a1c 100644 --- a/Gemfile +++ b/Gemfile @@ -38,3 +38,11 @@ gem "aws-sigv4", "~> 1.12", require: false # suite can exercise the stores against a real database rather than a stand-in. gem "activerecord", "~> 8.1", require: false gem "sqlite3", "~> 2.9", require: false + +# Not a runtime dependency of the gem (see the gemspec) -- the generator under +# lib/generators/ is loaded only when Rails loads generators, so a non-Rails +# application never needs it installed. It is here so +# test/translation_diff/install_generator_test.rb can load it and check the +# generated migration against the schema the rest of the suite runs against, +# instead of skipping itself for want of Rails::Generators::Base. +gem "railties", "~> 8.1", require: false From 777de16ed51bba0947f6c53ce17bcc60f47aa45a Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 17:54:20 +0400 Subject: [PATCH 09/36] test: replace the rate limiter rollover sleep with a moved clock The shared rollover test slept out Redis's fixed 5-second bucket width on every run, adding 12+ seconds the ratelimit gem's own behaviour made unshortenable. Drop it from RateLimiterContract; give ActiveRecordRateLimiter an injectable clock and prove rollover there by advancing it instead. Redis keeps no rollover test -- bucket rotation is the ratelimit gem's behaviour, not ours. --- .../active_record_rate_limiter.rb | 6 +++-- test/support/rate_limiter_contract.rb | 10 ++----- .../active_record_rate_limiter_test.rb | 27 ++++++++++++++----- .../redis_rate_limiter_test.rb | 6 ----- 4 files changed, 27 insertions(+), 22 deletions(-) diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb index 65abf20..a3bad8f 100644 --- a/lib/translation_diff/active_record_rate_limiter.rb +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -10,12 +10,14 @@ def self.build(config) threshold: config.rate_limit, interval: config.rate_interval, base: config.active_record_base) end - def initialize(namespace:, table_name:, threshold: DEFAULT_THRESHOLD, interval: DEFAULT_INTERVAL, base: nil) + def initialize(namespace:, table_name:, threshold: DEFAULT_THRESHOLD, interval: DEFAULT_INTERVAL, base: nil, + clock: -> { Time.now }) @namespace = namespace @table_name = table_name @threshold = threshold @interval = interval @base = base + @clock = clock end # Approximate at a window boundary, the same way the `ratelimit` gem this replaces is. @@ -36,7 +38,7 @@ def model def current_total = model.where(namespace: @namespace, bucket: bucket).sum(:characters) - def bucket = Time.now.to_i / @interval + def bucket = @clock.call.to_i / @interval # One statement, so two processes incrementing the same bucket cannot lose an increment between them. def add(size) diff --git a/test/support/rate_limiter_contract.rb b/test/support/rate_limiter_contract.rb index d92f59d..b1b0989 100644 --- a/test/support/rate_limiter_contract.rb +++ b/test/support/rate_limiter_contract.rb @@ -13,12 +13,6 @@ def test_checks_summing_past_the_threshold_raise assert_raises(rate_limit_exceeded_error) { build_limiter(threshold: 100, interval: 60).check(1) } end - def test_a_window_that_has_rolled_over_passes_again - build_limiter(threshold: 10, interval: rollover_interval).check(10) - assert_raises(rate_limit_exceeded_error) { build_limiter(threshold: 10, interval: rollover_interval).check(1) } - - sleep(rollover_wait) - - build_limiter(threshold: 10, interval: rollover_interval).check(1) - 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 diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb index 1b6824d..94757e4 100644 --- a/test/translation_diff/active_record_rate_limiter_test.rb +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -5,6 +5,13 @@ if ActiveRecordDatabase.available? ActiveRecordDatabase.connect! + # Moves without sleeping, so a bucket can be made to roll over on demand instead of waited out. + class MutableClock + def initialize(now) = @now = now + def call = @now + def advance(seconds) = @now += seconds + end + class ActiveRecordRateLimiterTest < Minitest::Test include RateLimiterContract @@ -12,6 +19,18 @@ def setup ActiveRecordDatabase.truncate end + # Advancing the clock by exactly one interval always lands in the next bucket, whatever the starting phase. + def test_a_window_that_has_rolled_over_passes_again + clock = MutableClock.new(Time.now) + + build_limiter(threshold: 10, interval: 60, clock: clock).check(10) + assert_raises(rate_limit_exceeded_error) { build_limiter(threshold: 10, interval: 60, clock: clock).check(1) } + + clock.advance(60) + + build_limiter(threshold: 10, interval: 60, clock: clock).check(1) + end + def model TranslationDiff::ActiveRecordRateLimiter.new(namespace: "translation-diff", table_name: "translation_diff_rate_limits").model @@ -85,13 +104,9 @@ def test_build_takes_its_settings_from_the_configuration def rate_limit_exceeded_error = TranslationDiff::ActiveRecordRateLimiter::RateLimitExceeded - # bucket == Time.now.to_i / interval, so a one-second interval rolls over almost immediately. - def rollover_interval = 1 - def rollover_wait = 2.2 - - def build_limiter(threshold:, interval:, namespace: "translation-diff") + def build_limiter(threshold:, interval:, namespace: "translation-diff", clock: -> { Time.now }) TranslationDiff::ActiveRecordRateLimiter.new(namespace: namespace, threshold: threshold, interval: interval, - table_name: "translation_diff_rate_limits") + table_name: "translation_diff_rate_limits", clock: clock) end end else diff --git a/test/translation_diff/redis_rate_limiter_test.rb b/test/translation_diff/redis_rate_limiter_test.rb index 53712e9..d41a2b8 100644 --- a/test/translation_diff/redis_rate_limiter_test.rb +++ b/test/translation_diff/redis_rate_limiter_test.rb @@ -134,12 +134,6 @@ def limiter(server, **) TranslationDiff::RedisRateLimiter.new(FakeConnectionPool.new(server), **) end - # Ratelimit's own bucket_interval is fixed at 5 seconds and is not configurable through this gem. - def rollover_interval = 5 - - # Two full 5-second buckets, so the boundary crosses regardless of where in a bucket the first check landed. - def rollover_wait = 10 - def rate_limit_exceeded_error = TranslationDiff::RedisRateLimiter::RateLimitExceeded def build_limiter(threshold:, interval:) From 926c6565152850327c57b4fff73e54e825a3d883 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:00:24 +0400 Subject: [PATCH 10/36] test: collapse a two-assertion check to fix a pre-existing rubocop offense test_store_batches_every_translated_segment_into_one_write_multi_call tripped Metrics/AbcSize on this rubocop patch, unrelated to this branch's own changes; asserting the single-element array directly says the same thing in one assertion instead of two. --- test/translation_diff/sentence_cache_test.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/translation_diff/sentence_cache_test.rb b/test/translation_diff/sentence_cache_test.rb index f0ec3f0..ecbf0db 100644 --- a/test/translation_diff/sentence_cache_test.rb +++ b/test/translation_diff/sentence_cache_test.rb @@ -84,8 +84,7 @@ def test_store_batches_every_translated_segment_into_one_write_multi_call subject_cache.store(subject) expected = subject.map { |segment| [subject_cache.key(segment), segment.translation] } - assert_equal 1, store.write_multi_calls.size - assert_equal expected, store.write_multi_calls.first + assert_equal [expected], store.write_multi_calls end # A batch of nothing is not a batch: a store that opens a transaction in write_multi must not be asked to. From 46d16033ccacee4a86547ff32abfede4ad3d12b9 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:00:31 +0400 Subject: [PATCH 11/36] ci: run the SQL suite against PostgreSQL too SQLite has one writer, so it cannot exercise the races this design rests on: last-write-wins on the cache's unique index, and no lost increment on a rate-limit bucket. Add a Postgres CI job and the concurrency tests that skip, with a message, unless TRANSLATION_DIFF_DATABASE_URL names a PostgreSQL database. Running the whole suite against a real Postgres for the first time also caught install_generator_test.rb comparing the generated migration's schema against a hardcoded SQLite connection regardless of which database the harness itself was running -- it now isolates the migration in a scratch Postgres schema when the harness is Postgres, so the comparison is adapter-for-adapter instead of adapter-for-SQLite. --- .github/workflows/ci.yml | 23 ++++++++ Gemfile | 6 ++ .../active_record_concurrency_test.rb | 58 +++++++++++++++++++ .../install_generator_test.rb | 21 +++++++ 4 files changed, 108 insertions(+) create mode 100644 test/translation_diff/active_record_concurrency_test.rb diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 46837bb..fbfa369 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,6 +23,29 @@ jobs: - run: bundle exec rake test + postgres: + runs-on: ubuntu-latest + + services: + postgres: + image: postgres:18 + env: + POSTGRES_PASSWORD: postgres + options: >- + --health-cmd pg_isready --health-interval 10s + --health-timeout 5s --health-retries 5 + ports: ["5432:5432"] + + steps: + - uses: actions/checkout@v4 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: "3.4" + bundler-cache: true + - run: bundle exec rake test + env: + TRANSLATION_DIFF_DATABASE_URL: postgres://postgres:postgres@localhost:5432/postgres + lint: runs-on: ubuntu-latest diff --git a/Gemfile b/Gemfile index 1e05a1c..2de5032 100644 --- a/Gemfile +++ b/Gemfile @@ -39,6 +39,12 @@ gem "aws-sigv4", "~> 1.12", require: false gem "activerecord", "~> 8.1", require: false gem "sqlite3", "~> 2.9", require: false +# Not a runtime dependency of the gem (see the gemspec) -- only the CI job that +# sets TRANSLATION_DIFF_DATABASE_URL ever opens a Postgres connection, where +# the concurrency the SQL store and rate limiter rest on can actually be +# tested. SQLite has one writer, so most of the suite never needs this gem. +gem "pg", "~> 1.5", require: false + # Not a runtime dependency of the gem (see the gemspec) -- the generator under # lib/generators/ is loaded only when Rails loads generators, so a non-Rails # application never needs it installed. It is here so diff --git a/test/translation_diff/active_record_concurrency_test.rb b/test/translation_diff/active_record_concurrency_test.rb new file mode 100644 index 0000000..2805e5c --- /dev/null +++ b/test/translation_diff/active_record_concurrency_test.rb @@ -0,0 +1,58 @@ +require "test_helper" +require "support/active_record_database" + +POSTGRES_DATABASE = ActiveRecordDatabase.available? && ActiveRecordDatabase.url.to_s.match?(%r{\Apostgres(ql)?://}) + +if POSTGRES_DATABASE + ActiveRecordDatabase.connect! + + # SQLite has one writer, so only Postgres can put these properties under a real race. + class ActiveRecordConcurrencyTest < Minitest::Test + def setup + ActiveRecordDatabase.truncate + end + + # Two connections upserting the same unique-index row: no exception, and last write standing wins. + def test_two_writers_racing_the_same_cache_key_do_not_raise_and_one_value_wins + key = "concurrent-key" + values = %w[first second] + + threads = values.map { |value| Thread.new { build_cache_store.write(key, value) } } + threads.each(&:join) + + assert_includes values, build_cache_store.read_multi([key]).first + end + + # Two connections upserting-and-incrementing the same bucket a hundred times each: no increment lost. + def test_two_limiters_racing_the_same_bucket_lose_no_increment + now = Time.now + totals = build_rate_limiter(now).model + + threads = Array.new(2) { Thread.new { 100.times { build_rate_limiter(now).check(1) } } } + threads.each(&:join) + + assert_equal 200, totals.sum(:characters) + end + + private + + def build_cache_store + TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: 60, + table_name: "translation_diff_translations") + end + + # A shared, frozen clock keeps both limiters in the same bucket for the length of the test. + def build_rate_limiter(now) + TranslationDiff::ActiveRecordRateLimiter.new(namespace: "translation-diff", threshold: 1_000_000, + interval: 60, table_name: "translation_diff_rate_limits", + clock: -> { now }) + end + end +else + class ActiveRecordConcurrencyTest < Minitest::Test + def test_postgres_is_unavailable + skip "TRANSLATION_DIFF_DATABASE_URL does not name a PostgreSQL database; " \ + "SQLite has one writer and cannot exercise these races" + end + end +end diff --git a/test/translation_diff/install_generator_test.rb b/test/translation_diff/install_generator_test.rb index 57bfb7a..c9608b4 100644 --- a/test/translation_diff/install_generator_test.rb +++ b/test/translation_diff/install_generator_test.rb @@ -1,6 +1,7 @@ require "test_helper" require "support/active_record_database" require "tmpdir" +require "securerandom" begin require "generators/translation_diff/install_generator" @@ -28,6 +29,11 @@ def test_the_generated_migration_matches_the_harness_schema end end + # Only the Postgres branch leaves anything behind to clean up; SQLite's :memory: connection needs no teardown. + def teardown + ::ActiveRecord::Base.connection.execute(%(DROP SCHEMA IF EXISTS "#{@schema}" CASCADE)) if @schema + end + private def migrate_in(dir) @@ -41,10 +47,25 @@ def migrate_in(dir) end def isolated_connection + return sqlite_isolated_connection unless ActiveRecordDatabase.url.to_s.start_with?("postgres") + + postgres_isolated_connection + end + + def sqlite_isolated_connection GeneratedMigrationRecord.establish_connection(adapter: "sqlite3", database: ":memory:") GeneratedMigrationRecord.connection end + # A scratch schema on the same server, so the migration has nowhere to collide with the harness's own tables. + def postgres_isolated_connection + @schema = "generator_test_#{SecureRandom.hex(4)}" + ::ActiveRecord::Base.connection.execute(%(CREATE SCHEMA "#{@schema}")) + config = ::ActiveRecord::Base.connection_db_config.configuration_hash.merge(schema_search_path: @schema) + GeneratedMigrationRecord.establish_connection(config) + GeneratedMigrationRecord.connection + end + def migration_file(dir) Dir.glob(File.join(dir, "db/migrate/*_create_translation_diff_tables.rb")).first end From 66d55fd51d6685cbc1bf8b87711ffe5841f24f08 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:08:53 +0400 Subject: [PATCH 12/36] fix: sum a sliding window of buckets, not one, in the AR rate limiter The tumbling counter let a nominal threshold through twice back to back at every bucket boundary -- reproduced with an 8000/60s throttle passing 16000 characters in three seconds. Sub-divide the interval into buckets a twelfth as wide and sum every one covering the last `interval` seconds, the same shape the `ratelimit` gem gets from its own fixed buckets, driven off the same injectable clock the rollover test already uses. Prune now matches: it drops buckets that have aged out of that window. Also clamp a negative size to zero before it reaches the upsert (it was quietly handing back headroom), and wire `rake translation_diff:prune` to prune the configured rate limiter too, printing both counts on their own line -- it only ever pruned the cache store before. --- Rakefile | 17 ++++--- .../active_record_rate_limiter.rb | 26 ++++++++--- .../active_record_rate_limiter_test.rb | 29 +++++++++--- test/translation_diff/prune_task_test.rb | 45 +++++++++++++++++++ 4 files changed, 99 insertions(+), 18 deletions(-) create mode 100644 test/translation_diff/prune_task_test.rb diff --git a/Rakefile b/Rakefile index 7a6f127..88553b9 100644 --- a/Rakefile +++ b/Rakefile @@ -42,13 +42,18 @@ namespace :translation_diff do task :prune do require "translation_diff" - store = TranslationDiff.config.cache_store - unless store.respond_to?(:prune) - puts "the configured cache store (#{store.class}) does not support pruning" - next + cache_store = TranslationDiff.config.cache_store + if cache_store.respond_to?(:prune) + puts "pruned #{cache_store.prune} expired cache rows" + else + puts "the configured cache store (#{cache_store.class}) does not support pruning" end - deleted = store.prune - puts "pruned #{deleted} expired cache rows" + rate_limiter = TranslationDiff.config.rate_limiter_instance + if rate_limiter.respond_to?(:prune) + puts "pruned #{rate_limiter.prune} expired rate-limit rows" + else + puts "the configured rate limiter (#{rate_limiter.class}) does not support pruning" + end end end diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb index a3bad8f..e786680 100644 --- a/lib/translation_diff/active_record_rate_limiter.rb +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -5,6 +5,10 @@ class RateLimitExceeded < TranslationDiff::Error; end DEFAULT_THRESHOLD = 8000 DEFAULT_INTERVAL = 60 + # A bucket a twelfth of the interval wide caps a window's slop at under 10%, the same shape the `ratelimit` gem + # gets from its own fixed five-second buckets at the default 60-second interval. + BUCKET_FRACTION = 12 + 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) @@ -18,17 +22,18 @@ def initialize(namespace:, table_name:, threshold: DEFAULT_THRESHOLD, interval: @interval = interval @base = base @clock = clock + @bucket_width = [@interval / BUCKET_FRACTION, 1].max end - # Approximate at a window boundary, the same way the `ratelimit` gem this replaces is. + # 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 add(size) end - # Buckets from before the current window; the host decides when, if ever, this runs. - def prune = model.where(namespace: @namespace).where(bucket: ...bucket).delete_all + # Buckets that have fully aged out of the window as of now; the host decides when, if ever, this runs. + def prune = model.where(namespace: @namespace).where(bucket: ...(oldest_bucket + 1)).delete_all def model @model ||= build_model @@ -36,14 +41,21 @@ def model private - def current_total = model.where(namespace: @namespace, bucket: bucket).sum(:characters) + def current_total + model.where(namespace: @namespace, bucket: (oldest_bucket + 1)..current_bucket).sum(:characters) + end + + def current_bucket = now / @bucket_width + + def oldest_bucket = (now - @interval) / @bucket_width - def bucket = @clock.call.to_i / @interval + def now = @clock.call.to_i # One statement, so two processes incrementing the same bucket cannot lose an increment between them. + # A negative size would otherwise hand back headroom it never used, so it is clamped before it reaches SQL. def add(size) - size = size.to_i - model.upsert_all([{ namespace: @namespace, bucket: bucket, characters: size }], + size = size.to_i.clamp(0..) + model.upsert_all([{ namespace: @namespace, bucket: current_bucket, characters: size }], unique_by: %i[namespace bucket], on_duplicate: Arel.sql("characters = #{model.table_name}.characters + #{size}")) end diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb index 94757e4..8a84246 100644 --- a/test/translation_diff/active_record_rate_limiter_test.rb +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -58,25 +58,44 @@ def test_two_namespaces_do_not_see_each_other def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one limiter = build_limiter(threshold: 1000, interval: 60) - model.create!(namespace: "translation-diff", bucket: limiter.send(:bucket) - 1, characters: 5) + model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket), characters: 5) limiter.check(10) deleted = limiter.prune assert_equal 1, deleted - assert_equal [limiter.send(:bucket)], model.pluck(:bucket) + assert_equal [limiter.send(:current_bucket)], model.pluck(:bucket) end def test_prune_only_deletes_rows_in_its_own_namespace own = build_limiter(threshold: 1000, interval: 60) - other = build_limiter(threshold: 1000, interval: 60, namespace: "other-tenant") - model.create!(namespace: "translation-diff", bucket: own.send(:bucket) - 1, characters: 5) - model.create!(namespace: "other-tenant", bucket: other.send(:bucket) - 1, characters: 5) + model.create!(namespace: "translation-diff", bucket: own.send(:oldest_bucket), characters: 5) + model.create!(namespace: "other-tenant", bucket: own.send(:oldest_bucket), characters: 5) assert_equal 1, own.prune assert_equal 1, model.where(namespace: "other-tenant").count end + # The bug this review caught: a tumbling counter let a nominal 8000/60s throttle through twice, back to back. + def test_a_check_at_the_end_of_one_window_still_counts_two_seconds_into_the_next + clock = MutableClock.new(Time.at(59)) + limiter = build_limiter(threshold: 8000, interval: 60, clock: clock) + + limiter.check(8000) + clock.advance(2) + + assert_raises(rate_limit_exceeded_error) { limiter.check(8000) } + end + + def test_a_negative_size_does_not_hand_back_headroom + limiter = build_limiter(threshold: 1000, interval: 60) + limiter.check(500) + + limiter.check(-1000) + + assert_equal 500, model.sum(:characters) + end + # `size` is interpolated into the on_duplicate SQL fragment, so a value with no clean integer must not reach it. def test_a_non_integer_size_only_contributes_its_leading_digits limiter = build_limiter(threshold: 1_000, interval: 60) diff --git a/test/translation_diff/prune_task_test.rb b/test/translation_diff/prune_task_test.rb new file mode 100644 index 0000000..bbc876b --- /dev/null +++ b/test/translation_diff/prune_task_test.rb @@ -0,0 +1,45 @@ +require "test_helper" +require "rake" + +class PruneTaskTest < Minitest::Test + class PruneableDouble + def initialize(count) = @count = count + def prune = @count + end + + def setup + TranslationDiff.reset! + Rake.application = Rake::Application.new + load File.expand_path("../../Rakefile", __dir__) + end + + def teardown = TranslationDiff.reset! + + def test_prunes_the_cache_store_and_the_rate_limiter_separately + TranslationDiff.configure do |c| + c.cache = PruneableDouble.new(3) + c.rate_limiter = PruneableDouble.new(5) + end + + out, = capture_io { Rake::Task["translation_diff:prune"].invoke } + + assert_includes out, "pruned 3 expired cache rows" + assert_includes out, "pruned 5 expired rate-limit rows" + end + + def test_reports_when_the_cache_store_does_not_support_pruning + TranslationDiff.configure { |c| c.cache = :memory } + + out, = capture_io { Rake::Task["translation_diff:prune"].invoke } + + assert_includes out, "the configured cache store (TranslationDiff::MemoryCacheStore) does not support pruning" + end + + def test_reports_when_there_is_no_rate_limiter_configured + TranslationDiff.configure { |c| c.cache = :memory } + + out, = capture_io { Rake::Task["translation_diff:prune"].invoke } + + assert_includes out, "the configured rate limiter (NilClass) does not support pruning" + end +end From feb36c617d574f9082059af1a2d3ab47c434e29b Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:13:45 +0400 Subject: [PATCH 13/36] docs: document the SQL cache store and rate limiter --- CHANGELOG.md | 29 ++++++ README.md | 4 +- docs/caching.md | 52 ++++++++-- docs/configuration.md | 6 +- docs/contracts.md | 36 +++++-- docs/how-it-works.md | 11 ++- docs/sql-cache.md | 214 ++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 329 insertions(+), 23 deletions(-) create mode 100644 docs/sql-cache.md diff --git a/CHANGELOG.md b/CHANGELOG.md index f5692ff..d32115f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,35 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). quietly too low: only three of the six built-in providers report billing at all. See [Instrumentation](docs/instrumentation.md). +- **A SQL-backed cache store and rate limiter, for an application that runs + Postgres or MySQL and does not want Redis for this alone.** + `TranslationDiff::ActiveRecordCacheStore` (`config.cache = + :active_record`) and `TranslationDiff::ActiveRecordRateLimiter` + (`config.rate_limiter = :active_record`) cache translations and throttle + requests in the application's own database. Nothing here is breaking: + both are opt-in, the default resolution of `cache` and `rate_limiter` is + untouched, and an application with `redis_url` set keeps getting Redis + exactly as before. `rails generate translation_diff:install` writes the + migration for both tables; for anyone not on Rails, its body is in + [SQL cache](docs/sql-cache.md) verbatim -- **the gem itself never runs + DDL.** ActiveRecord 7.1 or newer is required when either is used, refused + by name at build time rather than failing inside a query, and + `activerecord` is never a dependency of this gem -- it is required lazily + on first use, the same way `redis` already is. Four new configuration + options: `cache_table_name`, `rate_limit_table_name`, + `active_record_base` and `cache_prune_probability`. See + [SQL cache](docs/sql-cache.md). +- `write_multi(pairs)` joins the cache store contract, as an optional + method: a store that implements it gets one call carrying a whole batch + of sentences instead of one call per sentence; a store that does not is + still called once per sentence, exactly as before this method existed -- + a custom cache store written against the older contract is unaffected. + All three shipped stores implement it now: `MemoryCacheStore` and + `RedisCacheStore` already did, and `ActiveRecordCacheStore` joins them. + The two batching paths fail differently from the per-key one and from + each other -- see + [The two write paths fail differently](docs/caching.md#the-two-write-paths-fail-differently). + ### Security - `Configuration#inspect` and `Provider#inspect` print `[FILTERED]` in place diff --git a/README.md b/README.md index bfc9981..c2c0818 100644 --- a/README.md +++ b/README.md @@ -111,7 +111,7 @@ See [Providers](docs/providers.md) for configuring each one, the full capabiliti - **Six built-in providers** -- DeepL, Google Cloud Translation, Azure AI Translator, ModernMT, LibreTranslate, Amazon Translate -- or bring your own by subclassing a small base class - **HTML aware:** markup is preserved, and `class="notranslate"` can protect a span (provider support varies -- see the caveats below) - **Any shape:** strings, arrays, and deep hashes go in and come back translated in the same shape -- **Two cache stores:** `MemoryCacheStore` out of the box, `RedisCacheStore` once you configure `redis_url` +- **Three cache stores:** `MemoryCacheStore` out of the box, `RedisCacheStore` once you configure `redis_url`, `ActiveRecordCacheStore` to cache in your own database instead -- see [SQL cache](docs/sql-cache.md) - **Isolated contexts:** `TranslationDiff.context` for multi-tenant apps and per-request provider overrides, without touching the global configuration - **Pluggable sentence segmenter:** `pragmatic_segmenter` by default, with a zero-dependency `Simple` alternative - **HTTP retries, timeouts, and backoff** on every REST-backed provider, via `faraday` and `faraday-retry` @@ -140,7 +140,7 @@ This gem loads `ox`, `pragmatic_segmenter`, `faraday`, and `faraday-retry` at re ## Documentation -[Configuration](docs/configuration.md) · [Providers](docs/providers.md) · [Languages](docs/languages.md) · [Caching](docs/caching.md) · [Contracts](docs/contracts.md) · [Instrumentation](docs/instrumentation.md) · [Errors](docs/errors.md) · [How it works](docs/how-it-works.md) · [Upgrading & development](docs/development.md) +[Configuration](docs/configuration.md) · [Providers](docs/providers.md) · [Languages](docs/languages.md) · [Caching](docs/caching.md) · [SQL cache](docs/sql-cache.md) · [Contracts](docs/contracts.md) · [Instrumentation](docs/instrumentation.md) · [Errors](docs/errors.md) · [How it works](docs/how-it-works.md) · [Upgrading & development](docs/development.md) ## Contributing diff --git a/docs/caching.md b/docs/caching.md index f257a25..ed1f98c 100644 --- a/docs/caching.md +++ b/docs/caching.md @@ -51,8 +51,8 @@ key every already-warm cache is keyed on. ## The cache store contract -`config.cache` accepts either a registered name (`:redis`, `:memory`) or an -object satisfying this contract directly: +`config.cache` accepts either a registered name (`:redis`, `:memory`, +`:active_record`) or an object satisfying this contract directly: ```ruby # Reads several keys at once, returning an array the same length as keys, @@ -61,17 +61,57 @@ def read_multi(keys); end # Writes one key. The second write of the same key replaces the first. def write(key, value); end + +# Writes several pairs at once. Optional -- see "write_multi is optional" below. +def write_multi(pairs); end ``` `test/support/cache_store_contract.rb` is the executable form of this contract: include `CacheStoreContract` in a test class that defines `#store`. -Two stores ship with this gem: `TranslationDiff::MemoryCacheStore`, the +Three stores ship with this gem: `TranslationDiff::MemoryCacheStore`, the default -- a bounded, in-process LRU, not thread-safe by design, evicting by -`cache_max_size` rather than by time; and `TranslationDiff::RedisCacheStore`, +`cache_max_size` rather than by time; `TranslationDiff::RedisCacheStore`, built from `redis_url` when that is set, expiring entries after `cache_ttl` -and namespacing every key under `cache_namespace`. Neither `redis` nor -`connection_pool` nor `redis-namespace` is a dependency of this gem -- +and namespacing every key under `cache_namespace`; and +`TranslationDiff::ActiveRecordCacheStore`, opt-in, caching in the +application's own database -- see [SQL cache](sql-cache.md). Neither `redis` +nor `connection_pool` nor `redis-namespace` is a dependency of this gem -- `RedisCacheStore` takes anything answering to `#with` the way `ConnectionPool` does, and yields anything `Redis::Namespace` accepts. + +## `write_multi` is optional + +A store need not implement `write_multi`. `SentenceCache#store` checks: a +store that answers to it gets one call carrying every translated sentence +from the batch; a store that does not is called once per sentence through +`write` instead, exactly as it always was. A custom cache store written +against the contract before `write_multi` existed keeps working unchanged +-- that is what "optional" means here. + +All three shipped stores implement it: `MemoryCacheStore` loops over the +pairs (there is no round trip to save in-process); `RedisCacheStore` +pipelines the writes; `ActiveRecordCacheStore` upserts the whole batch in +one statement. + +### The two write paths fail differently + +Nobody had written this down before: what a partial failure leaves cached +depends on which of these shapes wrote it. + +- **No `write_multi` (the per-key path), and `MemoryCacheStore`'s loop.** + Sentences are written one at a time, in order. A failure at sentence N + leaves 1..N-1 written, N failed, and N+1.. never attempted. +- **`RedisCacheStore#write_multi`.** A Redis pipeline is not a + transaction: each `SETEX` in it runs independently of the others, so a + failure in one does not stop its siblings from landing. Which of the + batch actually landed does not follow the sentence order the way the + per-key path's does. +- **`ActiveRecordCacheStore#write_multi`.** One `upsert_all` statement for + the whole batch. It either lands as a whole or it does not -- there is no + partial batch to reason about. + +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. diff --git a/docs/configuration.md b/docs/configuration.md index e341b8f..e426f1a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -58,12 +58,16 @@ at all, so an unset environment variable never has to be special-cased. | `cache_ttl` | `604_800` (one week) | Seconds a Redis cache entry is kept. Only meaningful for `RedisCacheStore`; `MemoryCacheStore` evicts by size instead. | | `cache_namespace` | `"translation-diff"` | Prefix applied to every Redis key this gem writes -- both cache entries and the rate limiter's own bookkeeping. | | `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). | +| `cache_prune_probability` | `0.0` | Chance, per write, that `ActiveRecordCacheStore` prunes expired rows before returning. `0.0` is off; `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` | An object satisfying the [rate limiter contract](contracts.md#the-rate-limiter-contract), to use in place of the built-in Redis-backed one. | +| `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`. | | `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 24c88d6..7313b8b 100644 --- a/docs/contracts.md +++ b/docs/contracts.md @@ -2,16 +2,20 @@ ## The rate limiter contract -Unlike `provider`, `cache` and `segmenter`, `rate_limiter` does not resolve a -symbol through a registry -- there is only one built-in implementation. -`config.rate_limiter_instance` is: +Like `provider`, `cache` and `segmenter`, `rate_limiter` resolves a symbol +through its own registry, `TranslationDiff::RateLimiters` -- `:redis` and +`:active_record` are registered there. `config.rate_limiter_instance` is: -- the object assigned to `config.rate_limiter`, if any; +- 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 a `TranslationDiff::RedisRateLimiter` built from `rate_limit`, - `rate_interval`, `redis_url` and `cache_namespace`. +- 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)). An object assigned to `rate_limiter` must implement: @@ -23,10 +27,14 @@ def check(size); end `TranslationDiff::RedisRateLimiter` raises `TranslationDiff::RedisRateLimiter::RateLimitExceeded` when its threshold is -exceeded within its interval. 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 `TranslationDiff::Error` naming the gem to add. +exceeded within its interval; +`TranslationDiff::ActiveRecordRateLimiter` raises its own +`RateLimitExceeded`, a distinct class under the same name. 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 +`TranslationDiff::Error` naming the gem to add. `activerecord` is never a +dependency either -- see [SQL cache](sql-cache.md#the-activerecord-version-floor). **Upgrading to 3.1.0: re-validate your `rate_limit` threshold.** Before this release, `RedisRateLimiter` never actually limited anything -- a signature @@ -48,6 +56,14 @@ limiter up to six times more eagerly than the configured value suggests. Keep `rate_interval` within 5-600 seconds if you want the configured number to be the enforced one. +Both the clamp above and the upgrade note before it are about +`RedisRateLimiter`, which delegates its bucketing to the `ratelimit` gem. +`ActiveRecordRateLimiter` owns its own bucketing instead, and its window is +sliding rather than tumbling: buckets are a fraction of `rate_interval` +wide, and a check sums every bucket covering the trailing `rate_interval` +seconds, so `rate_interval` is enforced as configured, with no external +clamp. See [SQL cache](sql-cache.md#the-rate-limiter). + ## The segmenter contract `config.segmenter` decides where a text node is cut into sentence-sized diff --git a/docs/how-it-works.md b/docs/how-it-works.md index 6be46c7..6406eaf 100644 --- a/docs/how-it-works.md +++ b/docs/how-it-works.md @@ -50,10 +50,13 @@ Everything below is a collaborator one of the two drives. translation to a sentence by position after the fact. 6. **What came back is written home, and the value is rebuilt.** - Only sentences that actually got a translation are cached. Then each - passage renders itself -- markup fragments byte-exact, translated - sentences re-encoded as HTML text -- and `Document` puts the renders back - into the caller's shape. + Only sentences that actually got a translation are cached, through + `TranslationDiff::SentenceCache#store` -- see [`write_multi` is + optional](caching.md#write_multi-is-optional) for what happens when the + store's write fails partway through a batch. Then each passage renders + itself -- markup fragments byte-exact, translated sentences re-encoded as + HTML text -- and `Document` puts the renders back into the caller's + shape. `TranslationDiff::Markup` is the small module underneath steps 2, 3 and 6: it decodes entity references on the way to a provider, encodes `&` and `<` again diff --git a/docs/sql-cache.md b/docs/sql-cache.md new file mode 100644 index 0000000..65c6d2a --- /dev/null +++ b/docs/sql-cache.md @@ -0,0 +1,214 @@ +# SQL cache + +## What it's for + +If you already run Postgres or MySQL and do not want to stand up Redis for +one cache, `TranslationDiff::ActiveRecordCacheStore` caches translations in +the application's own database instead, and +`TranslationDiff::ActiveRecordRateLimiter` throttles requests there too. + +Both are opt-in. Setting `redis_url` still means Redis, exactly as before -- +nothing about an existing application's cache changes until you configure +one of these: + +```ruby +TranslationDiff.configure do |config| + config.cache = :active_record + config.rate_limiter = :active_record + config.cache_ttl = 30 * 24 * 60 * 60 +end +``` + +## What this store is worse at than Redis + +Two things, plainly. A read is a query against a table rather than an +`MGET` against an in-memory store. And the table grows until something +prunes it -- Redis expires a key for you; this store only stops serving an +expired row, it does not remove it by itself. See +[Pruning](#pruning-three-answers-none-imposed) below. + +## The tables + +Two tables, created by a migration you run once -- see +[The migration](#the-migration) below. This gem never creates or alters +either of them itself. + +### `translation_diff_translations` + +| Column | Meaning | +| --- | --- | +| `namespace` | `cache_namespace`. Two tenants share this table the way they share a Redis database; `#prune` only prunes its own configured namespace. | +| `key_digest` | `Digest::SHA256.hexdigest(key)` -- 64 characters, always. The cache key itself is not stored, only its digest: a variable-length unique index is the one thing guaranteed to bite somebody on MySQL. This also means an entry is not human-readable by its key -- to find one, compute the digest the same way and look that up. | +| `translation` | The cached value. | +| `expires_at` | What `cache_ttl` means in SQL. `nil` when `cache_ttl` is unset, which means the row never expires on its own. | +| `created_at`, `updated_at` | Standard ActiveRecord timestamps, set by `upsert_all`. | + +Unique index on `[namespace, key_digest]` -- the second write of a key +replaces the first, which is the cache store contract. A separate index on +`expires_at` backs both the read (which filters on it) and `#prune` (which +deletes by it). + +### `translation_diff_rate_limits` + +| Column | Meaning | +| --- | --- | +| `namespace` | `cache_namespace`, same column, same meaning, same table-sharing as above. | +| `bucket` | A slice of time narrower than `rate_interval` -- see [The rate limiter](#the-rate-limiter) below for why. | +| `characters` | Characters counted into that bucket so far. | + +Unique index on `[namespace, bucket]`, incremented by one guarded upsert +per check, so two processes hitting the same bucket cannot lose an +increment between them. + +## The migration + +`rails generate translation_diff:install` writes a timestamped migration +creating both tables, and lives under `lib/generators/`, loaded only when +Rails loads generators -- a non-Rails application never sees it and never +pays for it. **This gem never runs DDL itself:** the migration is the only +way either table comes into existence, and it is entirely yours to review, +edit, and run through your own deploy process. + +For anyone not on Rails -- Sequel, plain ActiveRecord, a DBA who would +rather write the DDL directly -- here is that migration's body, verbatim +(the generator fills in the class's version bracket with your own +`ActiveRecord::Migration.current_version`; `7.1` below is this store's +floor, not a requirement to target that version specifically): + +```ruby +class CreateTranslationDiffTables < ActiveRecord::Migration[7.1] + def change + 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.datetime :expires_at + t.timestamps + end + + add_index :translation_diff_translations, %i[namespace key_digest], unique: true, + name: "index_translation_diff_translations_on_key" + add_index :translation_diff_translations, :expires_at + + create_table :translation_diff_rate_limits do |t| + t.string :namespace, null: false, limit: 64 + t.integer :bucket, null: false + t.integer :characters, null: false, default: 0 + end + + add_index :translation_diff_rate_limits, %i[namespace bucket], unique: true, + name: "index_translation_diff_rate_limits_on_bucket" + end +end +``` + +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. + +## `cache_ttl` becomes `expires_at` + +`cache_ttl` (in seconds, same option `RedisCacheStore` reads) is written +into each row's `expires_at` at write time. A row past `expires_at` is +never read, whether or not anything has deleted it yet -- expiry and +deletion are two different questions here, unlike Redis, where a `SETEX` +key simply stops existing. + +## Pruning: three answers, none imposed + +Deleting an expired row is a separate question from whether it is served, +and there is no single right answer to "when," so none is forced on you: + +- **`rake translation_diff:prune`.** Calls `#prune` on the configured cache + store and, separately, on the configured rate limiter -- deleting rows + and buckets past their expiry, each in its own configured namespace. Wire + it into cron, a scheduled job, whatever your host already runs. Either + side that does not support pruning (`:redis`, or an object of your own) + is reported and skipped rather than failing the task. +- **`config.cache_prune_probability`** (default `0.0`, off). A fraction + between 0 and 1: on a write, `ActiveRecordCacheStore` rolls under it and + prunes if it wins. Off by default, because a translation-serving request + should not be paying, even occasionally, for someone else's expired rows. +- **Doing nothing.** Also a supported answer. An unpruned table is correct + -- reads still skip every expired row -- just larger than it needs to be. + +`#prune` only ever deletes rows in its own configured `cache_namespace`; a +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 + +`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: + +```ruby +class TranslationDiffRecord < ActiveRecord::Base + self.abstract_class = true + connects_to database: { writing: :translation_diff, reading: :translation_diff } +end + +TranslationDiff.configure do |config| + config.cache = :active_record + config.active_record_base = TranslationDiffRecord +end +``` + +## The ActiveRecord version floor + +**ActiveRecord 7.1 or newer.** `upsert_all` needs `unique_by` on Postgres +and SQLite, and `record_timestamps:` only landed in 7.1. An older version is +refused by name, at the point the store is first used, rather than failing +inside a query with a message that does not say why: + +``` +the ActiveRecord cache store needs ActiveRecord 7.1 or newer (found 7.0.0): +upsert_all takes unique_by and record_timestamps there. +``` + +`activerecord` is never a dependency of this gem -- neither in the gemspec +nor required at load time. `ActiveRecordCacheStore#model` and +`ActiveRecordRateLimiter#model` `require "active_record"` on first use, so +an application that never configures `:active_record` never loads it, the +same way `RedisCacheStore` only reaches for `redis` when `redis_url` is +set. Add `gem "activerecord"` (and a database adapter) to your own Gemfile +to use either. + +## `write_multi` + +Both `ActiveRecordCacheStore` and `RedisCacheStore` implement the cache +store contract's optional `write_multi(pairs)` -- see +[`write_multi` is optional](caching.md#write_multi-is-optional) for what +that means, and +[The two write paths fail differently](caching.md#the-two-write-paths-fail-differently) +for how a batch write fails differently from a per-key one. +`ActiveRecordCacheStore#write_multi` is a single `upsert_all` for the whole +batch: a forty-sentence paragraph is one statement, not forty. + +## The rate limiter + +`config.rate_limiter = :active_record` throttles the same way `rate_limit` +and `rate_interval` already configure the Redis-backed limiter, but counts +characters into `translation_diff_rate_limits` instead of Redis. + +The window is sliding, not tumbling. Time is divided into buckets a +fraction of `rate_interval` wide, not one bucket per interval, and a check +sums every bucket covering the trailing `rate_interval` seconds before +deciding whether the threshold is exceeded -- so the answer does not jump +the moment a single wide bucket rolls over, the way it would if the whole +interval were one bucket. Like the check it replaces, it looks at the +total *before* adding the new characters, so a check that itself pushes the +total over the threshold still succeeds; the next one raises. + +This is still approximate at a window boundary, the same way the +`ratelimit` gem this store does not depend on is. A translation throttle +exists to keep a provider's quota from being exceeded, not to be a billing +meter. + +`#prune` here deletes buckets that have fully aged out of the window, in +the configured namespace. `rake translation_diff:prune` calls it the same +way it calls the cache store's `#prune`. There is no +`cache_prune_probability` equivalent for the rate limiter -- the rake task, +or leaving old buckets in place, are the two supported answers here. From 10b39e6a26f6ca535e0f8d95ecf4cc5976cc7423 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:17:48 +0400 Subject: [PATCH 14/36] test: keep the Redis window honest and put Rake's application back --- test/translation_diff/prune_task_test.rb | 7 ++++++- test/translation_diff/redis_rate_limiter_test.rb | 9 +++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/test/translation_diff/prune_task_test.rb b/test/translation_diff/prune_task_test.rb index bbc876b..2928bb6 100644 --- a/test/translation_diff/prune_task_test.rb +++ b/test/translation_diff/prune_task_test.rb @@ -7,13 +7,18 @@ def initialize(count) = @count = count def prune = @count end + # The application is global, so it is put back: the next Rake-based test must not inherit this one's tasks. def setup TranslationDiff.reset! + @previous_application = Rake.application Rake.application = Rake::Application.new load File.expand_path("../../Rakefile", __dir__) end - def teardown = TranslationDiff.reset! + def teardown + Rake.application = @previous_application + TranslationDiff.reset! + end def test_prunes_the_cache_store_and_the_rate_limiter_separately TranslationDiff.configure do |c| diff --git a/test/translation_diff/redis_rate_limiter_test.rb b/test/translation_diff/redis_rate_limiter_test.rb index d41a2b8..d986fbc 100644 --- a/test/translation_diff/redis_rate_limiter_test.rb +++ b/test/translation_diff/redis_rate_limiter_test.rb @@ -117,6 +117,15 @@ def test_check_looks_back_over_a_custom_interval assert_equal [120], server.count_spans end + # The other half of a window: what fell out of it stops counting, or a limiter never recovers. + def test_a_bucket_older_than_the_interval_is_not_counted + server = FakeRedisServer.new + stale = (Time.now.to_i / 5) - (TranslationDiff::RedisRateLimiter::DEFAULT_INTERVAL / 5) - 1 + server.hashes["ratelimit:translation-diff:call"][stale.to_s] = 10_000 + + limiter(server, threshold: 100).check(1) + end + # Naming the bare `Ratelimit` constant used to raise a raw NameError instead of this gem's own message. def test_a_missing_ratelimit_gem_raises_a_translation_diff_error limiter = limiter(FakeRedisServer.new) From 7787d608a7dd449ae6524a98c660498b35faecac Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:37:25 +0400 Subject: [PATCH 15/36] fix: only pass unique_by to upsert_all when the connection supports it MySQL's adapter has never answered true to supports_insert_conflict_target?, so every write through ActiveRecordCacheStore and ActiveRecordRateLimiter raised ArgumentError on MySQL: reads kept working, so a cache that never wrote looked like a cache that never warmed. MySQL's ON DUPLICATE KEY UPDATE already targets every unique key, so omitting unique_by there is correct. Adds a MySQL job to CI (trilogy, not mysql2 -- it builds without libmysqlclient headers) and fixes the generator test's isolated_connection, which fell back to SQLite for any non-Postgres URL and so compared a MySQL table's schema against a SQLite one once a MySQL job existed to run it. Verified locally against a real MySQL 9.7 server (Homebrew, via trilogy): the bug reproduces before this change (ArgumentError on the first write) and the full suite -- 620 runs, 1 skip -- passes after it. Not run against the CI job's mysql:8 image itself, only a local MySQL server speaking the same protocol. --- .github/workflows/ci.yml | 24 ++++++++++++++++++ Gemfile | 7 ++++++ .../active_record_cache_store.rb | 10 ++++++-- .../active_record_rate_limiter.rb | 10 ++++++-- .../active_record_cache_store_test.rb | 16 ++++++++++++ .../active_record_rate_limiter_test.rb | 18 +++++++++++++ .../install_generator_test.rb | 25 ++++++++++++++++--- 7 files changed, 102 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fbfa369..1bb8720 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,6 +46,30 @@ jobs: env: TRANSLATION_DIFF_DATABASE_URL: postgres://postgres:postgres@localhost:5432/postgres + mysql: + runs-on: ubuntu-latest + + services: + mysql: + image: mysql:8 + env: + MYSQL_ALLOW_EMPTY_PASSWORD: "yes" + MYSQL_DATABASE: translation_diff_test + options: >- + --health-cmd "mysqladmin ping" --health-interval 10s + --health-timeout 5s --health-retries 5 + ports: ["3306:3306"] + + steps: + - uses: actions/checkout@v4 + - uses: ruby/setup-ruby@v1 + with: + ruby-version: "3.4" + bundler-cache: true + - run: bundle exec rake test + env: + TRANSLATION_DIFF_DATABASE_URL: trilogy://root@127.0.0.1:3306/translation_diff_test + lint: runs-on: ubuntu-latest diff --git a/Gemfile b/Gemfile index 2de5032..35790f4 100644 --- a/Gemfile +++ b/Gemfile @@ -45,6 +45,13 @@ gem "sqlite3", "~> 2.9", require: false # tested. SQLite has one writer, so most of the suite never needs this gem. gem "pg", "~> 1.5", require: false +# Not a runtime dependency of the gem (see the gemspec) -- only the CI job that +# sets TRANSLATION_DIFF_DATABASE_URL to a MySQL database ever opens one, where +# the missing supports_insert_conflict_target? behaviour actually bites. Trilogy +# over mysql2: it is a pure Ruby/C socket client with no libmysqlclient headers +# to install, so it builds on a bare CI runner and on this machine alike. +gem "trilogy", "~> 2.9", require: false + # Not a runtime dependency of the gem (see the gemspec) -- the generator under # lib/generators/ is loaded only when Rails loads generators, so a non-Rails # application never needs it installed. It is here so diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb index 15626e0..0a6b0fb 100644 --- a/lib/translation_diff/active_record_cache_store.rb +++ b/lib/translation_diff/active_record_cache_store.rb @@ -34,8 +34,7 @@ def write(key, value) def write_multi(pairs) return pairs if pairs.empty? - model.upsert_all(pairs.to_h.map { |key, value| row(key, value) }, - unique_by: %i[namespace key_digest], record_timestamps: true) + model.upsert_all(pairs.to_h.map { |key, value| row(key, value) }, **upsert_options(model.connection)) prune_sometimes pairs end @@ -49,6 +48,13 @@ def model private + # MySQL's adapter never answers true here and its ON DUPLICATE KEY UPDATE already targets every unique key. + def upsert_options(connection) + options = { record_timestamps: true } + options[:unique_by] = %i[namespace key_digest] if connection.supports_insert_conflict_target? + options + end + def row(key, value) { namespace: @namespace, key_digest: digest(key), translation: value, expires_at: expires_at } end diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb index e786680..93f2fa1 100644 --- a/lib/translation_diff/active_record_rate_limiter.rb +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -56,8 +56,14 @@ def now = @clock.call.to_i def add(size) size = size.to_i.clamp(0..) model.upsert_all([{ namespace: @namespace, bucket: current_bucket, characters: size }], - unique_by: %i[namespace bucket], - on_duplicate: Arel.sql("characters = #{model.table_name}.characters + #{size}")) + **upsert_options(model.connection, size)) + end + + # MySQL's adapter never answers true here and its ON DUPLICATE KEY UPDATE already targets every unique key. + def upsert_options(connection, size) + options = { on_duplicate: Arel.sql("characters = #{model.table_name}.characters + #{size}") } + options[:unique_by] = %i[namespace bucket] if connection.supports_insert_conflict_target? + options end def build_model diff --git a/test/translation_diff/active_record_cache_store_test.rb b/test/translation_diff/active_record_cache_store_test.rb index f8f6244..26fd67d 100644 --- a/test/translation_diff/active_record_cache_store_test.rb +++ b/test/translation_diff/active_record_cache_store_test.rb @@ -87,6 +87,22 @@ def test_build_takes_its_settings_from_the_configuration assert_equal "from-config", built.model.first.namespace end + def test_write_multi_omits_unique_by_when_the_connection_does_not_support_a_conflict_target + connection = Class.new { def supports_insert_conflict_target? = false }.new + + options = store.send(:upsert_options, connection) + + refute_includes options.keys, :unique_by + end + + def test_write_multi_keeps_unique_by_when_the_connection_supports_a_conflict_target + connection = Class.new { def supports_insert_conflict_target? = true }.new + + options = store.send(:upsert_options, connection) + + assert_includes options.keys, :unique_by + end + private def build_store(namespace: "translation-diff", ttl: 604_800) diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb index 8a84246..21db57e 100644 --- a/test/translation_diff/active_record_rate_limiter_test.rb +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -119,6 +119,24 @@ def test_build_takes_its_settings_from_the_configuration assert_equal ["from-config"], built.model.pluck(:namespace) 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 { def supports_insert_conflict_target? = false }.new + + options = limiter.send(:upsert_options, connection, 1) + + refute_includes options.keys, :unique_by + end + + def test_add_keeps_unique_by_when_the_connection_supports_a_conflict_target + limiter = build_limiter(threshold: 100, interval: 60) + connection = Class.new { def supports_insert_conflict_target? = true }.new + + options = limiter.send(:upsert_options, connection, 1) + + assert_includes options.keys, :unique_by + end + private def rate_limit_exceeded_error = TranslationDiff::ActiveRecordRateLimiter::RateLimitExceeded diff --git a/test/translation_diff/install_generator_test.rb b/test/translation_diff/install_generator_test.rb index c9608b4..1e118dc 100644 --- a/test/translation_diff/install_generator_test.rb +++ b/test/translation_diff/install_generator_test.rb @@ -29,9 +29,15 @@ def test_the_generated_migration_matches_the_harness_schema end end - # Only the Postgres branch leaves anything behind to clean up; SQLite's :memory: connection needs no teardown. + # Only the Postgres and MySQL branches leave anything behind; SQLite's :memory: connection needs no teardown. def teardown - ::ActiveRecord::Base.connection.execute(%(DROP SCHEMA IF EXISTS "#{@schema}" CASCADE)) if @schema + return unless @schema + + if ActiveRecordDatabase.url.to_s.start_with?("postgres") + ::ActiveRecord::Base.connection.execute(%(DROP SCHEMA IF EXISTS "#{@schema}" CASCADE)) + else + ::ActiveRecord::Base.connection.execute(%(DROP DATABASE IF EXISTS `#{@schema}`)) + end end private @@ -47,9 +53,11 @@ def migrate_in(dir) end def isolated_connection - return sqlite_isolated_connection unless ActiveRecordDatabase.url.to_s.start_with?("postgres") + url = ActiveRecordDatabase.url.to_s + return postgres_isolated_connection if url.start_with?("postgres") + return mysql_isolated_connection if url.match?(%r{\A(mysql2|trilogy)://}) - postgres_isolated_connection + sqlite_isolated_connection end def sqlite_isolated_connection @@ -66,6 +74,15 @@ def postgres_isolated_connection GeneratedMigrationRecord.connection end + # MySQL has no per-connection search path, so a scratch database stands in for Postgres's scratch schema. + def mysql_isolated_connection + @schema = "generator_test_#{SecureRandom.hex(4)}" + ::ActiveRecord::Base.connection.execute(%(CREATE DATABASE `#{@schema}`)) + config = ::ActiveRecord::Base.connection_db_config.configuration_hash.merge(database: @schema) + GeneratedMigrationRecord.establish_connection(config) + GeneratedMigrationRecord.connection + end + def migration_file(dir) Dir.glob(File.join(dir, "db/migrate/*_create_translation_diff_tables.rb")).first end From 8486f437ce0a699ad2a2d26c1883ed8b081647ca Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:42:18 +0400 Subject: [PATCH 16/36] fix: ship translation_diff:prune inside the gem, not the dev Rakefile A host application's `rake` never loads a dependency's Rakefile, so the documented pruning task was unreachable and the rate-limit table had no pruning path at all. The task now lives in lib/translation_diff/tasks, and a Railtie -- loaded only when Rails already is -- registers it for a Rails application's own rake tasks, enhanced with :environment so it prunes the application's configuration rather than the default. The dev Rakefile now loads the same file, so this gem's own suite exercises one definition, not two. prune_task_test now loads the task from that lib/ file directly, proving it is registered from the shipped file rather than by luck. --- Rakefile | 22 +--------- lib/translation_diff.rb | 4 ++ lib/translation_diff/railtie.rb | 12 ++++++ .../tasks/translation_diff.rake | 21 ++++++++++ test/translation_diff/prune_task_test.rb | 6 ++- test/translation_diff/railtie_test.rb | 42 +++++++++++++++++++ 6 files changed, 86 insertions(+), 21 deletions(-) create mode 100644 lib/translation_diff/railtie.rb create mode 100644 lib/translation_diff/tasks/translation_diff.rake create mode 100644 test/translation_diff/railtie_test.rb diff --git a/Rakefile b/Rakefile index 88553b9..882dee4 100644 --- a/Rakefile +++ b/Rakefile @@ -37,23 +37,5 @@ namespace :languages do end end -namespace :translation_diff do - desc "Delete expired rows from the SQL cache and rate-limit tables" - task :prune do - require "translation_diff" - - cache_store = TranslationDiff.config.cache_store - if cache_store.respond_to?(:prune) - puts "pruned #{cache_store.prune} expired cache rows" - else - puts "the configured cache store (#{cache_store.class}) does not support pruning" - end - - rate_limiter = TranslationDiff.config.rate_limiter_instance - if rate_limiter.respond_to?(:prune) - puts "pruned #{rate_limiter.prune} expired rate-limit rows" - else - puts "the configured rate limiter (#{rate_limiter.class}) does not support pruning" - end - end -end +# The task itself ships in lib/, so a host application's own `rake` can load it too -- this just reuses it here. +load File.expand_path("lib/translation_diff/tasks/translation_diff.rake", __dir__) diff --git a/lib/translation_diff.rb b/lib/translation_diff.rb index 08a7bc0..a15a35a 100644 --- a/lib/translation_diff.rb +++ b/lib/translation_diff.rb @@ -57,6 +57,10 @@ require "translation_diff/translator" require "translation_diff/context" +# Only when a host application has already loaded Rails -- never required unconditionally, so a non-Rails +# application never pays for it, and the gem's own suite exercises this same guarded require, not a shortcut. +require "translation_diff/railtie" if defined?(Rails::Railtie) + module TranslationDiff class << self def config = @config ||= Configuration.new diff --git a/lib/translation_diff/railtie.rb b/lib/translation_diff/railtie.rb new file mode 100644 index 0000000..befe493 --- /dev/null +++ b/lib/translation_diff/railtie.rb @@ -0,0 +1,12 @@ +# Loaded only when Rails already is (see the guarded require in translation_diff.rb), never on its own. +require "active_support/core_ext/module/delegation" +require "rails/railtie" + +# Adds translation_diff:prune to a host Rails application's own rake tasks; the gem's dev Rakefile loads the +# same task file directly, so a host application's `rake -T` and this gem's own suite see one definition. +class TranslationDiff::Railtie < Rails::Railtie + rake_tasks do + load File.expand_path("tasks/translation_diff.rake", __dir__) + Rake::Task["translation_diff:prune"].enhance(["environment"]) + end +end diff --git a/lib/translation_diff/tasks/translation_diff.rake b/lib/translation_diff/tasks/translation_diff.rake new file mode 100644 index 0000000..19470a6 --- /dev/null +++ b/lib/translation_diff/tasks/translation_diff.rake @@ -0,0 +1,21 @@ +# Shipped in the gem so a host application's own `rake` sees it -- a dependency's Rakefile is never loaded. +namespace :translation_diff do + desc "Delete expired rows from the SQL cache and rate-limit tables" + task :prune do + require "translation_diff" + + cache_store = TranslationDiff.config.cache_store + if cache_store.respond_to?(:prune) + puts "pruned #{cache_store.prune} expired cache rows" + else + puts "the configured cache store (#{cache_store.class}) does not support pruning" + end + + rate_limiter = TranslationDiff.config.rate_limiter_instance + if rate_limiter.respond_to?(:prune) + puts "pruned #{rate_limiter.prune} expired rate-limit rows" + else + puts "the configured rate limiter (#{rate_limiter.class}) does not support pruning" + end + end +end diff --git a/test/translation_diff/prune_task_test.rb b/test/translation_diff/prune_task_test.rb index 2928bb6..020d944 100644 --- a/test/translation_diff/prune_task_test.rb +++ b/test/translation_diff/prune_task_test.rb @@ -7,12 +7,16 @@ def initialize(count) = @count = count def prune = @count end + # Loads the task from the gem's own lib/ file -- what a host application's `rake` actually sees -- not the + # development Rakefile, which a host application's `rake` never loads. + TASK_FILE = File.expand_path("../../lib/translation_diff/tasks/translation_diff.rake", __dir__) + # The application is global, so it is put back: the next Rake-based test must not inherit this one's tasks. def setup TranslationDiff.reset! @previous_application = Rake.application Rake.application = Rake::Application.new - load File.expand_path("../../Rakefile", __dir__) + load TASK_FILE end def teardown diff --git a/test/translation_diff/railtie_test.rb b/test/translation_diff/railtie_test.rb new file mode 100644 index 0000000..39dd257 --- /dev/null +++ b/test/translation_diff/railtie_test.rb @@ -0,0 +1,42 @@ +require "test_helper" +require "rake" + +begin + require "translation_diff/railtie" + RAILTIE_AVAILABLE = true +rescue LoadError + RAILTIE_AVAILABLE = false +end + +if RAILTIE_AVAILABLE + class RailtieTest < Minitest::Test + def setup + @previous_application = Rake.application + Rake.application = Rake::Application.new + end + + def teardown + Rake.application = @previous_application + end + + # This is what Rails calls when an application runs `rake -T` or `rails runner`, never the gem's own Rakefile. + def test_registers_the_prune_task_from_the_gems_own_file + TranslationDiff::Railtie.instance.send(:run_tasks_blocks, nil) + + assert Rake::Task.task_defined?("translation_diff:prune") + end + + # Without this, the task prunes whatever the default configuration resolves to, not the host application's. + def test_the_registered_task_depends_on_environment + TranslationDiff::Railtie.instance.send(:run_tasks_blocks, nil) + + assert_includes Rake::Task["translation_diff:prune"].prerequisites, "environment" + end + end +else + class RailtieTest < Minitest::Test + def test_rails_railtie_is_unavailable + skip "Rails::Railtie could not be loaded; the railtie suite is skipped" + end + end +end From d47a8978211d805884f75b0113d0fbecdd2672bc Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:45:21 +0400 Subject: [PATCH 17/36] fix: wrap the cache store's write in its own savepoint Measured on PostgreSQL: an application that saves something, translates (the cache write fails), and rescues around TranslationDiff.translate still loses the outer save, because the failed INSERT aborted the whole transaction and the next statement dies with PG::InFailedSqlTransaction. write_multi now runs its upsert inside transaction(requires_new: true), a savepoint rather than the caller's own transaction, so a cache failure cannot poison a transaction this gem does not own. Also folds the Postgres-detection constant duplicated between the concurrency test and this new one into ActiveRecordDatabase.postgres?, so adding the second file didn't just relocate the duplication. Verified against a real local PostgreSQL 17: the poisoning reproduces before this change (PG::InFailedSqlTransaction on the next statement) and both directions -- a failing write leaves the transaction usable, a successful write still lands -- pass after it. Full suite green against SQLite, PostgreSQL and MySQL alike (623-625 runs depending on which database-gated tests that run exercises). --- .../active_record_cache_store.rb | 5 +- test/support/active_record_database.rb | 4 ++ .../active_record_concurrency_test.rb | 4 +- .../active_record_transaction_test.rb | 59 +++++++++++++++++++ 4 files changed, 68 insertions(+), 4 deletions(-) create mode 100644 test/translation_diff/active_record_transaction_test.rb diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb index 0a6b0fb..9869473 100644 --- a/lib/translation_diff/active_record_cache_store.rb +++ b/lib/translation_diff/active_record_cache_store.rb @@ -34,7 +34,10 @@ def write(key, value) def write_multi(pairs) return pairs if pairs.empty? - model.upsert_all(pairs.to_h.map { |key, value| row(key, value) }, **upsert_options(model.connection)) + # A savepoint, not the caller's own transaction: a failed write must not abort a transaction it does not own. + model.transaction(requires_new: true) do + model.upsert_all(pairs.to_h.map { |key, value| row(key, value) }, **upsert_options(model.connection)) + end prune_sometimes pairs end diff --git a/test/support/active_record_database.rb b/test/support/active_record_database.rb index 10d4e1e..6265ba5 100644 --- a/test/support/active_record_database.rb +++ b/test/support/active_record_database.rb @@ -18,6 +18,10 @@ def self.connect! def self.url = ENV.fetch("TRANSLATION_DIFF_DATABASE_URL", nil) + # Every test file that needs a real PostgreSQL (not SQLite's single writer, not MySQL's forgiving transactions) + # 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)?://}) + # 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 diff --git a/test/translation_diff/active_record_concurrency_test.rb b/test/translation_diff/active_record_concurrency_test.rb index 2805e5c..3b9d430 100644 --- a/test/translation_diff/active_record_concurrency_test.rb +++ b/test/translation_diff/active_record_concurrency_test.rb @@ -1,9 +1,7 @@ require "test_helper" require "support/active_record_database" -POSTGRES_DATABASE = ActiveRecordDatabase.available? && ActiveRecordDatabase.url.to_s.match?(%r{\Apostgres(ql)?://}) - -if POSTGRES_DATABASE +if ActiveRecordDatabase.postgres? ActiveRecordDatabase.connect! # SQLite has one writer, so only Postgres can put these properties under a real race. diff --git a/test/translation_diff/active_record_transaction_test.rb b/test/translation_diff/active_record_transaction_test.rb new file mode 100644 index 0000000..86cb23c --- /dev/null +++ b/test/translation_diff/active_record_transaction_test.rb @@ -0,0 +1,59 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.postgres? + ActiveRecordDatabase.connect! + + # PostgreSQL aborts the whole transaction on a statement error; only there can poisoning actually be measured. + class ActiveRecordCacheStoreTransactionTest < Minitest::Test + def setup + ActiveRecordDatabase.truncate + end + + # The scenario the review measured: save something, translate (cache write fails), rescue, keep saving. + def test_a_failing_write_leaves_the_callers_transaction_usable + harness = harness_model + + harness.transaction do + begin + failing_store.write("a", "one") + rescue StandardError + nil + end + + assert harness.create!(namespace: "harness", key_digest: "d" * 64, translation: "still usable") + end + + assert_equal 1, harness.where(namespace: "harness").count + end + + def test_a_successful_write_still_lands + store = TranslationDiff::ActiveRecordCacheStore.new(namespace: "harness", ttl: 60, + table_name: "translation_diff_translations") + + harness_model.transaction { store.write("a", "one") } + + assert_equal ["one"], store.read_multi(["a"]) + end + + private + + def harness_model + TranslationDiff::ActiveRecordCacheStore.new(namespace: "harness", ttl: 60, + table_name: "translation_diff_translations").model + end + + # A namespace past the column's 64-character limit is a statement PostgreSQL always rejects. + def failing_store + TranslationDiff::ActiveRecordCacheStore.new(namespace: "x" * 100, ttl: 60, + table_name: "translation_diff_translations") + end + end +else + class ActiveRecordCacheStoreTransactionTest < 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 From 472a13609c4275c0980ef59558a2ef46ad37fa2d Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:47:54 +0400 Subject: [PATCH 18/36] fix: redact the cache store's own SQL errors before they leave the gem upsert_all inlines values rather than binding them, so a StatementInvalid carries the whole row -- PostgreSQL's own DETAIL line for a NOT NULL or CHECK violation reads "Failing row contains (..., the translated content, ...)". This gem's own guarantee is that no error message carries the customer's content, and that guarantee did not hold for this path. write_multi now rescues ActiveRecord::StatementInvalid and re-raises TranslationDiff::Error naming only the adapter's own error class (from the exception's cause) and the statement's shape -- the table and column names, never a value. What the host's own ActiveRecord logger prints is unaffected and out of scope; the docs agent owns that half. Verified against a real local PostgreSQL 17 with a check constraint forcing a genuine PG::CheckViolation whose native DETAIL line contains a planted secret string: the secret reaches the raised error's message before this change and does not after it. --- .../active_record_cache_store.rb | 10 ++++ .../active_record_redaction_test.rb | 54 +++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 test/translation_diff/active_record_redaction_test.rb diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb index 9869473..eec8c14 100644 --- a/lib/translation_diff/active_record_cache_store.rb +++ b/lib/translation_diff/active_record_cache_store.rb @@ -40,6 +40,8 @@ def write_multi(pairs) end prune_sometimes pairs + rescue ActiveRecord::StatementInvalid => e + raise redacted_error(e) end # Reads never serve an expired row; deleting one is this, and it is the host's call when to run it. @@ -58,6 +60,14 @@ def upsert_options(connection) options end + # upsert_all inlines values into the statement it sends, so the adapter's own message can carry a whole row -- + # this names the adapter's error class and the statement's shape, never the row a caller's logger already has. + def redacted_error(error) + adapter_error = error.cause&.class || error.class + TranslationDiff::Error.new("the cache write failed (#{adapter_error}): an upsert into " \ + "#{@table_name}(namespace, key_digest, translation, expires_at)") + end + def row(key, value) { namespace: @namespace, key_digest: digest(key), translation: value, expires_at: expires_at } end diff --git a/test/translation_diff/active_record_redaction_test.rb b/test/translation_diff/active_record_redaction_test.rb new file mode 100644 index 0000000..582e4a3 --- /dev/null +++ b/test/translation_diff/active_record_redaction_test.rb @@ -0,0 +1,54 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.postgres? + ActiveRecordDatabase.connect! + + # upsert_all inlines values into the SQL it sends, so PostgreSQL's own error detail can carry a whole row; + # only a real constraint violation against a real server reproduces that, hence the PostgreSQL gate. + class ActiveRecordCacheStoreRedactionTest < Minitest::Test + CONSTRAINT = "no_forbidden_namespace_in_redaction_test".freeze + + def setup + ActiveRecordDatabase.truncate + connection.execute("ALTER TABLE translation_diff_translations ADD CONSTRAINT #{CONSTRAINT} " \ + "CHECK (namespace <> 'forbidden-namespace')") + end + + def teardown + connection.execute("ALTER TABLE translation_diff_translations DROP CONSTRAINT IF EXISTS #{CONSTRAINT}") + end + + def test_a_statement_invalid_never_carries_the_translated_content + store = TranslationDiff::ActiveRecordCacheStore.new(namespace: "forbidden-namespace", ttl: 60, + table_name: "translation_diff_translations") + + error = assert_raises(TranslationDiff::Error) { store.write("a", "SECRET-PATIENT-NOTE-12345") } + + refute_includes error.message, "SECRET-PATIENT-NOTE-12345" + end + + def test_the_redacted_error_names_the_adapters_own_error_class + store = TranslationDiff::ActiveRecordCacheStore.new(namespace: "forbidden-namespace", ttl: 60, + table_name: "translation_diff_translations") + + error = assert_raises(TranslationDiff::Error) { store.write("a", "one") } + + assert_includes error.message, "PG::CheckViolation" + end + + private + + def connection + TranslationDiff::ActiveRecordCacheStore.new(namespace: "harness", ttl: 60, + table_name: "translation_diff_translations").model.connection + end + end +else + class ActiveRecordCacheStoreRedactionTest < Minitest::Test + def test_postgres_is_unavailable + skip "TRANSLATION_DIFF_DATABASE_URL does not name a PostgreSQL database; " \ + "a check violation's row detail is what this test reproduces" + end + end +end From 95eb18c556e66478e27f26548b57f96fb3f74a29 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:52:36 +0400 Subject: [PATCH 19/36] fix: stop dropping the partial oldest bucket in the AR rate limiter current_total summed (oldest_bucket + 1)..current_bucket, excluding a bucket that is only ever partially outside the true interval -- so a deposit made as little as interval - bucket_width seconds ago could already be uncounted, letting more through a nominal threshold than configured inside an actual interval-second window. Summing oldest_bucket..current_bucket instead covers interval..interval + bucket_width seconds: erring strict, the harmless direction for a throttle whose job is keeping a vendor from cutting an application off. prune's own boundary moves with it -- it deleted bucket <= oldest_bucket, which would now delete a bucket the window still counts, quietly undoing the stricter sum the moment it runs. It now deletes only bucket < oldest_bucket. test_a_window_that_has_rolled_over_passes_again pinned the old boundary (advancing by exactly one interval was enough to roll over); it now advances by interval + bucket_width, the new guarantee. Two new tests prove the new bound rather than just the old one being gone: a deposit 56 real seconds old still blocking a later check, and prune leaving the oldest bucket's row alone because the window still counts it. --- .../active_record_rate_limiter.rb | 7 ++-- .../active_record_rate_limiter_test.rb | 35 ++++++++++++++++--- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb index 93f2fa1..49b431c 100644 --- a/lib/translation_diff/active_record_rate_limiter.rb +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -32,8 +32,8 @@ def check(size) add(size) end - # Buckets that have fully aged out of the window as of now; the host decides when, if ever, this runs. - def prune = model.where(namespace: @namespace).where(bucket: ...(oldest_bucket + 1)).delete_all + # 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 def model @model ||= build_model @@ -41,8 +41,9 @@ def model private + # The oldest bucket is only ever partially inside the window, so summing from it, not past it, errs strict. def current_total - model.where(namespace: @namespace, bucket: (oldest_bucket + 1)..current_bucket).sum(:characters) + model.where(namespace: @namespace, bucket: oldest_bucket..current_bucket).sum(:characters) end def current_bucket = now / @bucket_width diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb index 21db57e..92cbc42 100644 --- a/test/translation_diff/active_record_rate_limiter_test.rb +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -19,14 +19,15 @@ def setup ActiveRecordDatabase.truncate end - # Advancing the clock by exactly one interval always lands in the next bucket, whatever the starting phase. + # The window covers interval..interval + bucket_width seconds, erring strict: the oldest bucket is only ever + # partially inside it, so a full interval alone does not guarantee it has rolled past -- one more bucket does. def test_a_window_that_has_rolled_over_passes_again clock = MutableClock.new(Time.now) build_limiter(threshold: 10, interval: 60, clock: clock).check(10) assert_raises(rate_limit_exceeded_error) { build_limiter(threshold: 10, interval: 60, clock: clock).check(1) } - clock.advance(60) + clock.advance(65) build_limiter(threshold: 10, interval: 60, clock: clock).check(1) end @@ -58,7 +59,7 @@ def test_two_namespaces_do_not_see_each_other def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one limiter = build_limiter(threshold: 1000, interval: 60) - model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket), characters: 5) + model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket) - 1, characters: 5) limiter.check(10) deleted = limiter.prune @@ -67,10 +68,22 @@ def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one assert_equal [limiter.send(:current_bucket)], model.pluck(:bucket) end + # The oldest bucket is only ever partially inside the window (see current_total), so prune leaving it alone + # is what keeps pruning from quietly undoing the strictness that sum starting at oldest_bucket relies on. + def test_prune_leaves_the_oldest_bucket_because_the_window_still_counts_it + limiter = build_limiter(threshold: 1000, interval: 60) + model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket), characters: 5) + + deleted = limiter.prune + + assert_equal 0, deleted + assert_equal [limiter.send(:oldest_bucket)], model.pluck(:bucket) + end + def test_prune_only_deletes_rows_in_its_own_namespace own = build_limiter(threshold: 1000, interval: 60) - model.create!(namespace: "translation-diff", bucket: own.send(:oldest_bucket), characters: 5) - model.create!(namespace: "other-tenant", bucket: own.send(:oldest_bucket), characters: 5) + model.create!(namespace: "translation-diff", bucket: own.send(:oldest_bucket) - 1, characters: 5) + model.create!(namespace: "other-tenant", bucket: own.send(:oldest_bucket) - 1, characters: 5) assert_equal 1, own.prune assert_equal 1, model.where(namespace: "other-tenant").count @@ -87,6 +100,18 @@ def test_a_check_at_the_end_of_one_window_still_counts_two_seconds_into_the_next assert_raises(rate_limit_exceeded_error) { limiter.check(8000) } end + # The dropped partial oldest bucket: 56 real seconds have passed, well inside a true 60-second window, but + # a tumbling (oldest_bucket + 1) start point had already stopped counting the first deposit's bucket. + def test_a_deposit_inside_the_window_still_blocks_a_later_check + clock = MutableClock.new(Time.at(4)) + limiter = build_limiter(threshold: 8000, interval: 60, clock: clock) + + limiter.check(8000) + clock.advance(56) + + assert_raises(rate_limit_exceeded_error) { limiter.check(1) } + end + def test_a_negative_size_does_not_hand_back_headroom limiter = build_limiter(threshold: 1000, interval: 60) limiter.check(500) From 1757d23b6b2df22ebae09f096d645f19a55d5452 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 18:59:27 +0400 Subject: [PATCH 20/36] fix: split write_multi out of the required cache store contract docs/caching.md promises a store implementing only read_multi and write keeps working, but CacheStoreContract required write_multi too, so an author following the docs got three NoMethodErrors from the shared test module. Move the three batching cases into BatchingCacheStoreContract, included by the three shipped stores; add a minimal two-method store that proves the required contract passes without it. --- test/support/batching_cache_store_contract.rb | 21 +++++++++++++++++++ test/support/cache_store_contract.rb | 21 +------------------ .../active_record_cache_store_test.rb | 2 ++ .../cache_store_contract_test.rb | 19 +++++++++++++++++ .../memory_cache_store_test.rb | 2 ++ .../redis_cache_store_test.rb | 2 ++ 6 files changed, 47 insertions(+), 20 deletions(-) create mode 100644 test/support/batching_cache_store_contract.rb create mode 100644 test/translation_diff/cache_store_contract_test.rb diff --git a/test/support/batching_cache_store_contract.rb b/test/support/batching_cache_store_contract.rb new file mode 100644 index 0000000..bb4c574 --- /dev/null +++ b/test/support/batching_cache_store_contract.rb @@ -0,0 +1,21 @@ +# The optional half of the cache store contract -- include only in a store that implements write_multi. +module BatchingCacheStoreContract + def test_write_multi_writes_every_pair + store.write_multi([%w[a one], %w[b two]]) + + assert_equal %w[one two], store.read_multi(%w[a b]) + end + + def test_write_multi_of_no_pairs_writes_nothing + store.write_multi([]) + + assert_empty store.read_multi([]) + end + + def test_write_multi_replaces_a_key_written_before + store.write("a", "one") + store.write_multi([%w[a two]]) + + assert_equal ["two"], store.read_multi(["a"]) + end +end diff --git a/test/support/cache_store_contract.rb b/test/support/cache_store_contract.rb index 83910c7..fc6200b 100644 --- a/test/support/cache_store_contract.rb +++ b/test/support/cache_store_contract.rb @@ -1,4 +1,4 @@ -# The executable form of the cache store contract; anything that passes can be TranslationDiff's cache. +# The executable form of the required cache store contract; anything that passes can be TranslationDiff's cache. module CacheStoreContract def test_write_then_read_multi_returns_the_value store.write("a", "one") @@ -22,23 +22,4 @@ def test_writing_the_same_key_twice_keeps_the_second_value assert_equal ["two"], store.read_multi(["a"]) end - - def test_write_multi_writes_every_pair - store.write_multi([%w[a one], %w[b two]]) - - assert_equal %w[one two], store.read_multi(%w[a b]) - end - - def test_write_multi_of_no_pairs_writes_nothing - store.write_multi([]) - - assert_empty store.read_multi([]) - end - - def test_write_multi_replaces_a_key_written_before - store.write("a", "one") - store.write_multi([%w[a two]]) - - assert_equal ["two"], store.read_multi(["a"]) - end end diff --git a/test/translation_diff/active_record_cache_store_test.rb b/test/translation_diff/active_record_cache_store_test.rb index 26fd67d..d89b704 100644 --- a/test/translation_diff/active_record_cache_store_test.rb +++ b/test/translation_diff/active_record_cache_store_test.rb @@ -1,5 +1,6 @@ require "test_helper" require "support/cache_store_contract" +require "support/batching_cache_store_contract" require "support/active_record_database" if ActiveRecordDatabase.available? @@ -7,6 +8,7 @@ class ActiveRecordCacheStoreTest < Minitest::Test include CacheStoreContract + include BatchingCacheStoreContract attr_reader :store diff --git a/test/translation_diff/cache_store_contract_test.rb b/test/translation_diff/cache_store_contract_test.rb new file mode 100644 index 0000000..c1fe4e8 --- /dev/null +++ b/test/translation_diff/cache_store_contract_test.rb @@ -0,0 +1,19 @@ +require "test_helper" +require "support/cache_store_contract" + +class CacheStoreContractTest < Minitest::Test + include CacheStoreContract + + # Exactly the two required methods, nothing else -- proves the required contract never needs write_multi. + class MinimalStore + def initialize = @entries = {} + def read_multi(keys) = keys.map { |key| @entries[key] } + def write(key, value) = @entries[key] = value + end + + attr_reader :store + + def setup + @store = MinimalStore.new + end +end diff --git a/test/translation_diff/memory_cache_store_test.rb b/test/translation_diff/memory_cache_store_test.rb index bb9ee05..59a0fda 100644 --- a/test/translation_diff/memory_cache_store_test.rb +++ b/test/translation_diff/memory_cache_store_test.rb @@ -1,8 +1,10 @@ require "test_helper" require "support/cache_store_contract" +require "support/batching_cache_store_contract" class MemoryCacheStoreTest < Minitest::Test include CacheStoreContract + include BatchingCacheStoreContract attr_reader :store diff --git a/test/translation_diff/redis_cache_store_test.rb b/test/translation_diff/redis_cache_store_test.rb index 055991c..2476175 100644 --- a/test/translation_diff/redis_cache_store_test.rb +++ b/test/translation_diff/redis_cache_store_test.rb @@ -1,5 +1,6 @@ require "test_helper" require "support/cache_store_contract" +require "support/batching_cache_store_contract" # Nested inside the real Redis class, requiring it explicitly, so this never races Configuration's lazy require. require "redis" @@ -25,6 +26,7 @@ def pipelined class RedisCacheStoreTest < Minitest::Test include CacheStoreContract + include BatchingCacheStoreContract # `values`, when given, forces #mget to return it regardless of keys asked, to inspect keys without real storage. class FakeRedis From 302e9740b023d30089a29b37c1c491ba6f6b0531 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:03:06 +0400 Subject: [PATCH 21/36] fix: make cache_ttl = nil reachable and cache_ttl = 0 mean never expires Configuration#read returned the 604800-second default for any nil ivar, so config.cache_ttl = nil read back as the default -- docs/sql-cache.md's "nil means the row never expires" was unreachable through the public configuration path, and the store's own nil-ttl branch was only exercised by a test that built the store by hand. Meanwhile cache_ttl = 0 wrote rows already expired: a cache that can never hit. TranslationDiff::CacheTtlOption prepends onto Configuration so nil sticks as a real value there, and folds any non-positive number into it too -- one rule, expressible through TranslationDiff.configure. Fixed the existing test that asserted the previously-unreachable state to go through config.cache_ttl instead of constructing the store directly. --- lib/translation_diff.rb | 1 + lib/translation_diff/cache_ttl_option.rb | 19 +++++++++++++++++++ lib/translation_diff/configuration.rb | 2 ++ .../active_record_cache_store_test.rb | 18 +++++++++++++++++- test/translation_diff/configuration_test.rb | 18 ++++++++++++++++++ 5 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 lib/translation_diff/cache_ttl_option.rb diff --git a/lib/translation_diff.rb b/lib/translation_diff.rb index a15a35a..c9063bf 100644 --- a/lib/translation_diff.rb +++ b/lib/translation_diff.rb @@ -27,6 +27,7 @@ require "translation_diff/fragment" require "translation_diff/passage" require "translation_diff/sentence_cache" +require "translation_diff/cache_ttl_option" require "translation_diff/configuration" require "translation_diff/configuration/provider_option_owners" diff --git a/lib/translation_diff/cache_ttl_option.rb b/lib/translation_diff/cache_ttl_option.rb new file mode 100644 index 0000000..f920d84 --- /dev/null +++ b/lib/translation_diff/cache_ttl_option.rb @@ -0,0 +1,19 @@ +# Prepended onto Configuration: nil sticks here as "never expires", unlike the generic option rule. +module TranslationDiff::CacheTtlOption + NEVER_ASSIGNED = Object.new.freeze + + def initialize + @cache_ttl = NEVER_ASSIGNED + super + end + + # A non-positive number folds into nil too -- a TTL of zero or less can never keep a row. + def cache_ttl=(value) + value = nil if value.is_a?(String) && value.strip.empty? + @cache_ttl = value.is_a?(Numeric) && value <= 0 ? nil : value + end + + def cache_ttl + @cache_ttl.equal?(NEVER_ASSIGNED) ? self.class.defaults[:cache_ttl] : @cache_ttl + end +end diff --git a/lib/translation_diff/configuration.rb b/lib/translation_diff/configuration.rb index 24cc40b..16ea03d 100644 --- a/lib/translation_diff/configuration.rb +++ b/lib/translation_diff/configuration.rb @@ -62,6 +62,8 @@ def provider_option_owners = @provider_option_owners ||= ProviderOptionOwners.ne option :max_retries, 3 option :validate_languages, true + prepend TranslationDiff::CacheTtlOption + # Credentials are filtered by name; everything else is shown, or an inspect is one nobody reads. def inspect = "#<#{self.class.name} #{TranslationDiff::Redaction.render(self).join(' ')}>" diff --git a/test/translation_diff/active_record_cache_store_test.rb b/test/translation_diff/active_record_cache_store_test.rb index d89b704..7487274 100644 --- a/test/translation_diff/active_record_cache_store_test.rb +++ b/test/translation_diff/active_record_cache_store_test.rb @@ -51,7 +51,23 @@ def test_prune_deletes_expired_rows_and_leaves_live_ones end def test_a_nil_cache_ttl_writes_a_row_that_never_expires - build_store(ttl: nil).write("a", "one") + config = TranslationDiff::Configuration.new + config.cache_namespace = "translation-diff" + config.cache_table_name = "translation_diff_translations" + config.cache_ttl = nil + + TranslationDiff::ActiveRecordCacheStore.build(config).write("a", "one") + + assert_nil model.first.expires_at + end + + def test_a_zero_cache_ttl_writes_a_row_that_never_expires_instead_of_already_expired + config = TranslationDiff::Configuration.new + config.cache_namespace = "translation-diff" + config.cache_table_name = "translation_diff_translations" + config.cache_ttl = 0 + + TranslationDiff::ActiveRecordCacheStore.build(config).write("a", "one") assert_nil model.first.expires_at end diff --git a/test/translation_diff/configuration_test.rb b/test/translation_diff/configuration_test.rb index a79a247..0d3dc2a 100644 --- a/test/translation_diff/configuration_test.rb +++ b/test/translation_diff/configuration_test.rb @@ -41,6 +41,24 @@ def test_an_option_returns_its_default_until_it_is_assigned assert_equal 60, @config.cache_ttl end + def test_a_nil_cache_ttl_sticks_instead_of_falling_back_to_the_default + @config.cache_ttl = nil + + assert_nil @config.cache_ttl + end + + def test_a_zero_cache_ttl_also_means_never_expires + @config.cache_ttl = 0 + + assert_nil @config.cache_ttl + end + + def test_a_negative_cache_ttl_also_means_never_expires + @config.cache_ttl = -1 + + assert_nil @config.cache_ttl + end + def test_a_callable_default_is_evaluated_on_every_read_not_at_load_time original = ENV.fetch("REDIS_URL", nil) ENV["REDIS_URL"] = "redis://first" From 706c4463b809607eab8133d6e09aab77fdc5f723 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:05:30 +0400 Subject: [PATCH 22/36] refactor: extract the shared ActiveRecord plumbing out of the store and limiter build_model, ensure_supported_version! and the version floor were near-duplicated across ActiveRecordCacheStore and ActiveRecordRateLimiter, and the limiter reached into the store's own MINIMUM_ACTIVE_RECORD constant to avoid a third copy. TranslationDiff::ActiveRecordSupport now owns the lazy require, the version check, the anonymous model class and the floor; both classes include it and only supply the three strings that make their error messages differ. Every existing message, the laziness (nothing names ::ActiveRecord at load time) and the one-model-per-instance memoisation are unchanged. --- lib/translation_diff.rb | 1 + .../active_record_cache_store.rb | 27 +++-------------- .../active_record_rate_limiter.rb | 28 ++++------------- lib/translation_diff/active_record_support.rb | 29 ++++++++++++++++++ .../active_record_support_test.rb | 30 +++++++++++++++++++ 5 files changed, 69 insertions(+), 46 deletions(-) create mode 100644 lib/translation_diff/active_record_support.rb create mode 100644 test/translation_diff/active_record_support_test.rb diff --git a/lib/translation_diff.rb b/lib/translation_diff.rb index c9063bf..0ca4f4b 100644 --- a/lib/translation_diff.rb +++ b/lib/translation_diff.rb @@ -49,6 +49,7 @@ require "translation_diff/stores" require "translation_diff/memory_cache_store" require "translation_diff/redis_cache_store" +require "translation_diff/active_record_support" require "translation_diff/active_record_cache_store" require "translation_diff/rate_limiters" require "translation_diff/redis_rate_limiter" diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb index eec8c14..4672b0a 100644 --- a/lib/translation_diff/active_record_cache_store.rb +++ b/lib/translation_diff/active_record_cache_store.rb @@ -1,6 +1,6 @@ # Caches translations in the application's own database; ActiveRecord is required on first use, never at load. class TranslationDiff::ActiveRecordCacheStore - MINIMUM_ACTIVE_RECORD = "7.1".freeze + include TranslationDiff::ActiveRecordSupport def self.build(config) new(namespace: config.cache_namespace, ttl: config.cache_ttl, @@ -47,10 +47,6 @@ def write_multi(pairs) # Reads never serve an expired row; deleting one is this, and it is the host's call when to run it. def prune = model.where(namespace: @namespace).where(expires_at: ...Time.now.utc).delete_all - def model - @model ||= build_model - end - private # MySQL's adapter never answers true here and its ON DUPLICATE KEY UPDATE already targets every unique key. @@ -86,24 +82,9 @@ def prune_sometimes prune if @prune_probability.positive? && rand < @prune_probability end - def build_model - require "active_record" - ensure_supported_version! - table = @table_name - Class.new(@base || ::ActiveRecord::Base) { self.table_name = table } - rescue LoadError - raise TranslationDiff::Error, - "the cache is :active_record but the `activerecord` gem is not available. " \ - 'Add `gem "activerecord"` to your Gemfile.' - end - - def ensure_supported_version! - return if Gem::Version.new(::ActiveRecord::VERSION::STRING) >= Gem::Version.new(MINIMUM_ACTIVE_RECORD) - - raise TranslationDiff::Error, - "the ActiveRecord cache store needs ActiveRecord #{MINIMUM_ACTIVE_RECORD} or newer " \ - "(found #{::ActiveRecord::VERSION::STRING}): upsert_all takes unique_by and record_timestamps there." - 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." end TranslationDiff::Stores.register(:active_record, TranslationDiff::ActiveRecordCacheStore) diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb index 49b431c..cfe2f56 100644 --- a/lib/translation_diff/active_record_rate_limiter.rb +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -1,5 +1,7 @@ # Throttles by counting characters into namespaced, time-bucketed rows in the application's own database. class TranslationDiff::ActiveRecordRateLimiter + include TranslationDiff::ActiveRecordSupport + class RateLimitExceeded < TranslationDiff::Error; end DEFAULT_THRESHOLD = 8000 @@ -35,10 +37,6 @@ def check(size) # 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 - def model - @model ||= build_model - end - private # The oldest bucket is only ever partially inside the window, so summing from it, not past it, errs strict. @@ -67,25 +65,9 @@ def upsert_options(connection, size) options end - def build_model - require "active_record" - ensure_supported_version! - table = @table_name - Class.new(@base || ::ActiveRecord::Base) { self.table_name = table } - rescue LoadError - raise TranslationDiff::Error, - "the rate limiter is :active_record but the `activerecord` gem is not available. " \ - 'Add `gem "activerecord"` to your Gemfile.' - end - - def ensure_supported_version! - minimum = TranslationDiff::ActiveRecordCacheStore::MINIMUM_ACTIVE_RECORD - return if Gem::Version.new(::ActiveRecord::VERSION::STRING) >= Gem::Version.new(minimum) - - raise TranslationDiff::Error, - "the ActiveRecord rate limiter needs ActiveRecord #{minimum} or newer " \ - "(found #{::ActiveRecord::VERSION::STRING}): upsert_all takes unique_by there." - end + def active_record_feature = "the rate limiter" + def active_record_component = "ActiveRecord rate limiter" + def active_record_upsert_detail = "upsert_all takes unique_by there." end TranslationDiff::RateLimiters.register(:active_record, TranslationDiff::ActiveRecordRateLimiter) diff --git a/lib/translation_diff/active_record_support.rb b/lib/translation_diff/active_record_support.rb new file mode 100644 index 0000000..8092d73 --- /dev/null +++ b/lib/translation_diff/active_record_support.rb @@ -0,0 +1,29 @@ +# The lazy require, the version floor and the anonymous model class, shared by the cache store and the limiter. +module TranslationDiff::ActiveRecordSupport + MINIMUM_ACTIVE_RECORD = "7.1".freeze + + def model + @model ||= build_model + end + + private + + def build_model + require "active_record" + ensure_supported_version! + table = @table_name + Class.new(@base || ::ActiveRecord::Base) { self.table_name = table } + rescue LoadError + raise TranslationDiff::Error, + "#{active_record_feature} is :active_record but the `activerecord` gem is not available. " \ + 'Add `gem "activerecord"` to your Gemfile.' + end + + def ensure_supported_version! + return if Gem::Version.new(::ActiveRecord::VERSION::STRING) >= Gem::Version.new(MINIMUM_ACTIVE_RECORD) + + raise TranslationDiff::Error, + "the #{active_record_component} needs ActiveRecord #{MINIMUM_ACTIVE_RECORD} or newer " \ + "(found #{::ActiveRecord::VERSION::STRING}): #{active_record_upsert_detail}" + end +end diff --git a/test/translation_diff/active_record_support_test.rb b/test/translation_diff/active_record_support_test.rb new file mode 100644 index 0000000..af353dd --- /dev/null +++ b/test/translation_diff/active_record_support_test.rb @@ -0,0 +1,30 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.available? + ActiveRecordDatabase.connect! + + class ActiveRecordSupportTest < Minitest::Test + def test_the_cache_store_and_the_rate_limiter_share_the_same_active_record_plumbing + assert_includes TranslationDiff::ActiveRecordCacheStore.ancestors, TranslationDiff::ActiveRecordSupport + assert_includes TranslationDiff::ActiveRecordRateLimiter.ancestors, TranslationDiff::ActiveRecordSupport + end + + def test_the_version_floor_is_declared_once_and_shared + assert_same TranslationDiff::ActiveRecordSupport::MINIMUM_ACTIVE_RECORD, + TranslationDiff::ActiveRecordCacheStore::MINIMUM_ACTIVE_RECORD + assert_same TranslationDiff::ActiveRecordSupport::MINIMUM_ACTIVE_RECORD, + TranslationDiff::ActiveRecordRateLimiter::MINIMUM_ACTIVE_RECORD + end + + def test_each_store_instance_memoises_its_own_model_rather_than_sharing_one + first = TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: nil, + table_name: "translation_diff_translations") + second = TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: nil, + table_name: "translation_diff_translations") + + assert_same first.model, first.model + refute_same first.model, second.model + end + end +end From 3498dd4e699dce941d4ec625cb516c877d2a98f8 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:07:03 +0400 Subject: [PATCH 23/36] fix: quote the table name in the rate limiter's on_duplicate fragment The Arel.sql on_duplicate fragment interpolated model.table_name directly. Not exploitable -- a hostile table name dies at ActiveRecord's own schema lookup first -- but it was the only unquoted identifier on the branch, and quote_table_name costs nothing. Verified against real PostgreSQL and MySQL that the increment still lands correctly with the quoted identifier. --- .../active_record_rate_limiter.rb | 3 ++- .../active_record_rate_limiter_test.rb | 19 +++++++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/translation_diff/active_record_rate_limiter.rb b/lib/translation_diff/active_record_rate_limiter.rb index cfe2f56..4950433 100644 --- a/lib/translation_diff/active_record_rate_limiter.rb +++ b/lib/translation_diff/active_record_rate_limiter.rb @@ -60,7 +60,8 @@ def add(size) # MySQL's adapter never answers true here and its ON DUPLICATE KEY UPDATE already targets every unique key. def upsert_options(connection, size) - options = { on_duplicate: Arel.sql("characters = #{model.table_name}.characters + #{size}") } + table = connection.quote_table_name(model.table_name) + options = { on_duplicate: Arel.sql("characters = #{table}.characters + #{size}") } options[:unique_by] = %i[namespace bucket] if connection.supports_insert_conflict_target? options end diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb index 92cbc42..640ae51 100644 --- a/test/translation_diff/active_record_rate_limiter_test.rb +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -146,7 +146,10 @@ def test_build_takes_its_settings_from_the_configuration 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 { def supports_insert_conflict_target? = false }.new + connection = Class.new do + def supports_insert_conflict_target? = false + def quote_table_name(name) = %("#{name}") + end.new options = limiter.send(:upsert_options, connection, 1) @@ -155,13 +158,25 @@ def test_add_omits_unique_by_when_the_connection_does_not_support_a_conflict_tar def test_add_keeps_unique_by_when_the_connection_supports_a_conflict_target limiter = build_limiter(threshold: 100, interval: 60) - connection = Class.new { def supports_insert_conflict_target? = true }.new + connection = Class.new do + def supports_insert_conflict_target? = true + def quote_table_name(name) = %("#{name}") + end.new options = limiter.send(:upsert_options, connection, 1) assert_includes options.keys, :unique_by end + def test_add_quotes_the_table_name_in_the_on_duplicate_fragment + limiter = build_limiter(threshold: 100, interval: 60) + connection = limiter.model.connection + + options = limiter.send(:upsert_options, connection, 1) + + assert_includes options[:on_duplicate].to_s, connection.quote_table_name(limiter.model.table_name) + end + private def rate_limit_exceeded_error = TranslationDiff::ActiveRecordRateLimiter::RateLimitExceeded From 188d192f88fc3bc0dba72c1e05f70f2c959ec338 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:09:05 +0400 Subject: [PATCH 24/36] fix: make the rate limiter's bucket column a bigint t.integer :bucket is 32-bit. Any rate_interval under 24 makes the bucket width one second, so the bucket is the epoch second, which overflows int4 in January 2038. Changed both the migration template and the test harness schema together, since the generator test checks them against each other. Verified against real PostgreSQL and MySQL: reproduced PG::NumericValueOutOfRange on the int4 column with a bucket value just past 2**31, then confirmed a bigint column stores and reads it back correctly on both. --- .../templates/create_translation_diff_tables.rb.erb | 2 +- test/support/active_record_database.rb | 2 +- .../active_record_rate_limiter_test.rb | 10 ++++++++++ 3 files changed, 12 insertions(+), 2 deletions(-) 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 6e7d700..a246241 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 @@ -14,7 +14,7 @@ class CreateTranslationDiffTables < ActiveRecord::Migration[<%= ActiveRecord::Mi create_table :translation_diff_rate_limits do |t| t.string :namespace, null: false, limit: 64 - t.integer :bucket, null: false + t.bigint :bucket, null: false t.integer :characters, null: false, default: 0 end diff --git a/test/support/active_record_database.rb b/test/support/active_record_database.rb index 6265ba5..f755bf6 100644 --- a/test/support/active_record_database.rb +++ b/test/support/active_record_database.rb @@ -47,7 +47,7 @@ def self.define_translations_table(connection) def self.define_rate_limits_table(connection) connection.create_table :translation_diff_rate_limits do |t| t.string :namespace, null: false, limit: 64 - t.integer :bucket, null: false + t.bigint :bucket, null: false t.integer :characters, null: false, default: 0 end connection.add_index :translation_diff_rate_limits, %i[namespace bucket], diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb index 640ae51..75eda74 100644 --- a/test/translation_diff/active_record_rate_limiter_test.rb +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -57,6 +57,16 @@ def test_two_namespaces_do_not_see_each_other assert true end + # int4 overflows at 2**31; any rate_interval under 24 makes the bucket the epoch second, which crosses that + # boundary in January 2038 -- bucket is bigint precisely so a real epoch-second value like this one fits. + def test_a_bucket_beyond_int32_range_is_stored_and_read_back + far_future_bucket = (2**31) + 1 + + model.create!(namespace: "translation-diff", bucket: far_future_bucket, characters: 5) + + assert_equal far_future_bucket, model.find_by(namespace: "translation-diff").bucket + end + def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one limiter = build_limiter(threshold: 1000, interval: 60) model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket) - 1, characters: 5) From 63b4bc19da351174bed085549ee3e5a0bcb25dc4 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:12:06 +0400 Subject: [PATCH 25/36] fix: reject a malformed cache_prune_probability or an over-long cache_namespace at configure time Two configure-time traps, closed together since both now live in the same guard module. config.cache_prune_probability = "0.5" (what an ENV var actually hands you) raised NoMethodError: undefined method 'positive?' for an instance of String from inside a translate call; coerced to Float here, or refused with a clear message if it isn't numeric. A cache_namespace over 64 characters used to fail with ActiveRecord::ValueTooLong on the first write; refused here instead, naming the limit, before any query runs. Reproduced the original NoMethodError against real PostgreSQL (a config built with a String probability, then a write) before applying the fix. --- lib/translation_diff.rb | 1 + lib/translation_diff/cache_guard_options.rb | 31 +++++++++++++++++++ lib/translation_diff/configuration.rb | 1 + .../active_record_cache_store_test.rb | 9 ++++++ test/translation_diff/configuration_test.rb | 24 ++++++++++++++ 5 files changed, 66 insertions(+) create mode 100644 lib/translation_diff/cache_guard_options.rb diff --git a/lib/translation_diff.rb b/lib/translation_diff.rb index 0ca4f4b..e721416 100644 --- a/lib/translation_diff.rb +++ b/lib/translation_diff.rb @@ -28,6 +28,7 @@ require "translation_diff/passage" require "translation_diff/sentence_cache" require "translation_diff/cache_ttl_option" +require "translation_diff/cache_guard_options" require "translation_diff/configuration" require "translation_diff/configuration/provider_option_owners" diff --git a/lib/translation_diff/cache_guard_options.rb b/lib/translation_diff/cache_guard_options.rb new file mode 100644 index 0000000..db4cf0d --- /dev/null +++ b/lib/translation_diff/cache_guard_options.rb @@ -0,0 +1,31 @@ +# Prepended onto Configuration: fails cache_prune_probability and cache_namespace at configure time, not later. +module TranslationDiff::CacheGuardOptions + CACHE_NAMESPACE_LIMIT = 64 + + # An ENV var arrives as a String; coerced here so a translate call never meets a bare String's missing #positive?. + def cache_prune_probability=(value) + value = nil if value.is_a?(String) && value.strip.empty? + @cache_prune_probability = value.nil? ? nil : coerce_probability(value) + end + + # Refused here, rather than at the first write's ActiveRecord::ValueTooLong. + def cache_namespace=(value) + value = nil if value.is_a?(String) && value.strip.empty? + raise namespace_too_long(value) if value.is_a?(String) && value.length > CACHE_NAMESPACE_LIMIT + + @cache_namespace = value + end + + private + + def coerce_probability(value) + Float(value) + rescue ArgumentError, TypeError + raise TranslationDiff::Error, "cache_prune_probability must be a number between 0 and 1 (got #{value.inspect})" + end + + def namespace_too_long(value) + TranslationDiff::Error.new("cache_namespace must be #{CACHE_NAMESPACE_LIMIT} characters or fewer " \ + "(got #{value.length})") + end +end diff --git a/lib/translation_diff/configuration.rb b/lib/translation_diff/configuration.rb index 16ea03d..634348a 100644 --- a/lib/translation_diff/configuration.rb +++ b/lib/translation_diff/configuration.rb @@ -63,6 +63,7 @@ def provider_option_owners = @provider_option_owners ||= ProviderOptionOwners.ne option :validate_languages, true prepend TranslationDiff::CacheTtlOption + prepend TranslationDiff::CacheGuardOptions # Credentials are filtered by name; everything else is shown, or an inspect is one nobody reads. def inspect = "#<#{self.class.name} #{TranslationDiff::Redaction.render(self).join(' ')}>" diff --git a/test/translation_diff/active_record_cache_store_test.rb b/test/translation_diff/active_record_cache_store_test.rb index 7487274..1fcb18c 100644 --- a/test/translation_diff/active_record_cache_store_test.rb +++ b/test/translation_diff/active_record_cache_store_test.rb @@ -105,6 +105,15 @@ def test_build_takes_its_settings_from_the_configuration assert_equal "from-config", built.model.first.namespace end + def test_a_string_cache_prune_probability_from_env_does_not_raise_on_write + config = TranslationDiff::Configuration.new + config.cache_namespace = "translation-diff" + config.cache_table_name = "translation_diff_translations" + config.cache_prune_probability = "0.5" + + TranslationDiff::ActiveRecordCacheStore.build(config).write("a", "one") + end + def test_write_multi_omits_unique_by_when_the_connection_does_not_support_a_conflict_target connection = Class.new { def supports_insert_conflict_target? = false }.new diff --git a/test/translation_diff/configuration_test.rb b/test/translation_diff/configuration_test.rb index 0d3dc2a..2cecebe 100644 --- a/test/translation_diff/configuration_test.rb +++ b/test/translation_diff/configuration_test.rb @@ -59,6 +59,30 @@ def test_a_negative_cache_ttl_also_means_never_expires assert_nil @config.cache_ttl end + def test_cache_prune_probability_coerces_a_numeric_string_the_way_an_env_var_arrives + @config.cache_prune_probability = "0.5" + + assert_in_delta 0.5, @config.cache_prune_probability + end + + def test_cache_prune_probability_refuses_a_non_numeric_string_with_a_clear_message + error = assert_raises(TranslationDiff::Error) { @config.cache_prune_probability = "lots" } + + assert_match(/cache_prune_probability/, error.message) + end + + def test_cache_namespace_longer_than_64_characters_is_refused_at_configure_time + error = assert_raises(TranslationDiff::Error) { @config.cache_namespace = "n" * 65 } + + assert_match(/64/, error.message) + end + + def test_cache_namespace_at_the_64_character_limit_is_accepted + @config.cache_namespace = "n" * 64 + + assert_equal "n" * 64, @config.cache_namespace + end + def test_a_callable_default_is_evaluated_on_every_read_not_at_load_time original = ENV.fetch("REDIS_URL", nil) ENV["REDIS_URL"] = "redis://first" From 364f26c249748fed3390c0a2d7970ea9444bcab3 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:13:05 +0400 Subject: [PATCH 26/36] fix: make the generated migration idempotent docs/sql-cache.md sells this migration's body for a DBA to hand-apply, but re-running it where the tables already exist died with PG::DuplicateTable (and the SQLite/MySQL equivalent). Added if_not_exists: true to both create_table calls and both add_index calls. Verified by applying the generated migration twice in one test, against real PostgreSQL and MySQL as well as the default SQLite run -- reproduced the duplicate-table error pre-fix on all three, then confirmed the second run is a no-op on all three. --- .../templates/create_translation_diff_tables.rb.erb | 10 +++++----- test/translation_diff/install_generator_test.rb | 11 +++++++++++ 2 files changed, 16 insertions(+), 5 deletions(-) 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 a246241..d04ec55 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 @@ -1,6 +1,6 @@ class CreateTranslationDiffTables < ActiveRecord::Migration[<%= ActiveRecord::Migration.current_version %>] def change - create_table :translation_diff_translations do |t| + 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 @@ -9,16 +9,16 @@ class CreateTranslationDiffTables < ActiveRecord::Migration[<%= ActiveRecord::Mi end add_index :translation_diff_translations, %i[namespace key_digest], unique: true, - name: "index_translation_diff_translations_on_key" - add_index :translation_diff_translations, :expires_at + name: "index_translation_diff_translations_on_key", if_not_exists: true + add_index :translation_diff_translations, :expires_at, if_not_exists: true - create_table :translation_diff_rate_limits do |t| + create_table :translation_diff_rate_limits, if_not_exists: true do |t| t.string :namespace, null: false, limit: 64 t.bigint :bucket, null: false t.integer :characters, null: false, default: 0 end add_index :translation_diff_rate_limits, %i[namespace bucket], unique: true, - name: "index_translation_diff_rate_limits_on_bucket" + name: "index_translation_diff_rate_limits_on_bucket", if_not_exists: true end end diff --git a/test/translation_diff/install_generator_test.rb b/test/translation_diff/install_generator_test.rb index 1e118dc..27130ed 100644 --- a/test/translation_diff/install_generator_test.rb +++ b/test/translation_diff/install_generator_test.rb @@ -29,6 +29,17 @@ def test_the_generated_migration_matches_the_harness_schema end end + # A DBA hand-applying this migration (see docs/sql-cache.md) may well run it twice; it must not blow up. + def test_the_generated_migration_can_be_applied_twice + Dir.mktmpdir do |dir| + connection = migrate_in(dir) + + capture_io { CreateTranslationDiffTables.new.exec_migration(connection, :up) } + + assert connection.table_exists?(:translation_diff_translations) + end + end + # Only the Postgres and MySQL branches leave anything behind; SQLite's :memory: connection needs no teardown. def teardown return unless @schema From 6476788574ed5524e275193725c5b9b630a6b01f Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:25:48 +0400 Subject: [PATCH 27/36] docs: catch up the SQL cache store docs with two rounds of fixes The MySQL unique_by fix, the shipped prune task, the transaction and query-log behavior, cache_ttl's nil/non-positive semantics, the split cache store contract, the idempotent bigint migration, the two configure- time guards, and the rate limiter's slightly conservative window were all undocumented or documented against the pre-fix code. Also names the ActiveRecord rate limiter's own RateLimitExceeded in docs/errors.md, corrects cache_ttl/cache_namespace's Redis-only framing in docs/configuration.md, fixes the CHANGELOG's write_multi history, and renames caching.md's "two write paths" heading (and its two linkers) to match the three shapes it actually lists. --- CHANGELOG.md | 63 +++++++++++++++++++----- README.md | 2 +- docs/caching.md | 8 ++- docs/configuration.md | 4 +- docs/contracts.md | 12 +++-- docs/errors.md | 13 ++++- docs/sql-cache.md | 110 +++++++++++++++++++++++++++++++++--------- 7 files changed, 167 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d32115f..40f4f67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,29 +35,68 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). `TranslationDiff::ActiveRecordCacheStore` (`config.cache = :active_record`) and `TranslationDiff::ActiveRecordRateLimiter` (`config.rate_limiter = :active_record`) cache translations and throttle - requests in the application's own database. Nothing here is breaking: - both are opt-in, the default resolution of `cache` and `rate_limiter` is + requests in the application's own database, exercised in CI against + Postgres, MySQL and SQLite. Nothing on this branch is breaking: both + are opt-in, the default resolution of `cache` and `rate_limiter` is untouched, and an application with `redis_url` set keeps getting Redis exactly as before. `rails generate translation_diff:install` writes the - migration for both tables; for anyone not on Rails, its body is in - [SQL cache](docs/sql-cache.md) verbatim -- **the gem itself never runs - DDL.** ActiveRecord 7.1 or newer is required when either is used, refused - by name at build time rather than failing inside a query, and - `activerecord` is never a dependency of this gem -- it is required lazily - on first use, the same way `redis` already is. Four new configuration - options: `cache_table_name`, `rate_limit_table_name`, - `active_record_base` and `cache_prune_probability`. See + migration for both tables, idempotently -- every `create_table` and + `add_index` in it carries `if_not_exists: true`; for anyone not on Rails, + its body is in [SQL cache](docs/sql-cache.md) verbatim -- **the gem + itself never runs DDL.** `rake translation_diff:prune` ships inside the + gem: a `Railtie` wires it into a Rails application's own rake tasks + automatically, enhanced with `:environment` so it prunes that + application's own configuration; a non-Rails application loads the task + file itself and + must configure `TranslationDiff` before running it, since the task gets + no `:environment`-equivalent there. ActiveRecord 7.1 or newer is + required when either is used, refused by name at build time rather than + failing inside a query, and `activerecord` is never a dependency of this + gem -- it is required lazily on first use, the same way `redis` already + is. Four new configuration options: `cache_table_name`, + `rate_limit_table_name`, `active_record_base` and + `cache_prune_probability`; the last, along with `cache_namespace`, is + now validated at `configure` time -- a `cache_prune_probability` that + will not coerce to a number, or a `cache_namespace` over 64 characters, + is refused before either reaches a query. See [SQL cache](docs/sql-cache.md). +- **The SQL cache store's write joins the caller's transaction, and its + errors are redacted, not silent.** A rollback in the caller's transaction + discards translations `ActiveRecordCacheStore` already wrote -- the + largest behavioural difference from `RedisCacheStore`, which is never + inside anyone's transaction. A failed write itself runs in its own + savepoint, so it no longer aborts a transaction it does not own, and the + `TranslationDiff::Error` it raises carries the adapter's error class, not + 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 + [Transactions](docs/sql-cache.md#transactions) and + [What ends up in your log](docs/sql-cache.md#what-ends-up-in-your-log). +- **`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 + configuration path, and `0` wrote a row whose `expires_at` was already in + the past -- a cache entry that could never hit. Both now fold to the same + `nil` `expires_at`. See + [`cache_ttl` becomes `expires_at`](docs/sql-cache.md#cache_ttl-becomes-expires_at). +- **`ActiveRecordRateLimiter`'s sliding window is conservative, not + exact.** It sums the oldest bucket touching the trailing `rate_interval` + seconds in full, even though that bucket is only ever partially inside + the window, so the window actually enforced is `rate_interval` to + `rate_interval + rate_interval / 12` seconds -- slightly stricter than + configured, never looser. See + [The rate limiter](docs/sql-cache.md#the-rate-limiter). - `write_multi(pairs)` joins the cache store contract, as an optional method: a store that implements it gets one call carrying a whole batch of sentences instead of one call per sentence; a store that does not is still called once per sentence, exactly as before this method existed -- a custom cache store written against the older contract is unaffected. All three shipped stores implement it now: `MemoryCacheStore` and - `RedisCacheStore` already did, and `ActiveRecordCacheStore` joins them. + `RedisCacheStore` gain it here too, alongside `ActiveRecordCacheStore`. The two batching paths fail differently from the per-key one and from each other -- see - [The two write paths fail differently](docs/caching.md#the-two-write-paths-fail-differently). + [The three write paths fail differently](docs/caching.md#the-three-write-paths-fail-differently). ### Security diff --git a/README.md b/README.md index c2c0818..e469e7d 100644 --- a/README.md +++ b/README.md @@ -116,7 +116,7 @@ See [Providers](docs/providers.md) for configuring each one, the full capabiliti - **Pluggable sentence segmenter:** `pragmatic_segmenter` by default, with a zero-dependency `Simple` alternative - **HTTP retries, timeouts, and backoff** on every REST-backed provider, via `faraday` and `faraday-retry` - **One error hierarchy** under `TranslationDiff::Error`, carrying the provider name and HTTP status -- **Optional rate limiting and instrumentation** -- credentials and translated content never appear in a log line +- **Optional rate limiting and instrumentation** -- credentials and translated content never appear in a log line this gem writes (the SQL cache store is the one exception worth knowing before you adopt it -- see [SQL cache](docs/sql-cache.md#what-ends-up-in-your-log)) ## Installation diff --git a/docs/caching.md b/docs/caching.md index ed1f98c..9222fec 100644 --- a/docs/caching.md +++ b/docs/caching.md @@ -68,7 +68,11 @@ def write_multi(pairs); end `test/support/cache_store_contract.rb` is the executable form of this contract: include `CacheStoreContract` in a test class that defines -`#store`. +`#store`. It only exercises `read_multi` and `write` -- the two required +methods -- so a store that implements only those two still passes it. +`test/support/batching_cache_store_contract.rb` holds the optional half: +include `BatchingCacheStoreContract` too, alongside `CacheStoreContract`, +once `#store` also implements `write_multi`. Three stores ship with this gem: `TranslationDiff::MemoryCacheStore`, the default -- a bounded, in-process LRU, not thread-safe by design, evicting by @@ -95,7 +99,7 @@ pairs (there is no round trip to save in-process); `RedisCacheStore` pipelines the writes; `ActiveRecordCacheStore` upserts the whole batch in one statement. -### The two write paths fail differently +### The three write paths fail differently Nobody had written this down before: what a partial failure leaves cached depends on which of these shapes wrote it. diff --git a/docs/configuration.md b/docs/configuration.md index e426f1a..8ad2d33 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -55,8 +55,8 @@ at all, so an unset environment variable never has to be special-cased. | --- | --- | --- | | `provider` | `:deepl` | The translation provider: a registered name or a `TranslationDiff::Provider` of your own. See [Providers](providers.md). | | `cache` | `nil` | The cache store: a registered name or an object satisfying the [cache store contract](caching.md#the-cache-store-contract). `nil` means "choose for me" -- see below. | -| `cache_ttl` | `604_800` (one week) | Seconds a Redis cache entry is kept. Only meaningful for `RedisCacheStore`; `MemoryCacheStore` evicts by size instead. | -| `cache_namespace` | `"translation-diff"` | Prefix applied to every Redis key this gem writes -- both cache entries and the rate limiter's own bookkeeping. | +| `cache_ttl` | `604_800` (one week) | Seconds an entry is kept before it expires. Read by `RedisCacheStore` (a `SETEX`) and by `ActiveRecordCacheStore` (written into each row's `expires_at`); `MemoryCacheStore` evicts by size instead and ignores it. A non-positive value (`0` or less, or `nil`) means never expires. See [SQL cache](sql-cache.md#cache_ttl-becomes-expires_at). | +| `cache_namespace` | `"translation-diff"` | Prefix applied to every Redis key this gem writes -- both cache entries and the rate limiter's own bookkeeping. Also the `namespace` column both SQL tables share and the unit `ActiveRecordCacheStore#prune` operates on. At most 64 characters -- longer is refused at `configure` time. See [SQL cache](sql-cache.md#the-tables). | | `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. | diff --git a/docs/contracts.md b/docs/contracts.md index 7313b8b..f943494 100644 --- a/docs/contracts.md +++ b/docs/contracts.md @@ -59,10 +59,14 @@ to be the enforced one. Both the clamp above and the upgrade note before it are about `RedisRateLimiter`, which delegates its bucketing to the `ratelimit` gem. `ActiveRecordRateLimiter` owns its own bucketing instead, and its window is -sliding rather than tumbling: buckets are a fraction of `rate_interval` -wide, and a check sums every bucket covering the trailing `rate_interval` -seconds, so `rate_interval` is enforced as configured, with no external -clamp. See [SQL cache](sql-cache.md#the-rate-limiter). +sliding rather than tumbling: buckets are `rate_interval / 12` seconds wide +(floored at 1 second), and a check sums every bucket touching the trailing +`rate_interval` seconds -- including the oldest one, which is only ever +partially inside that window, summed in full rather than pro-rated. So the +window actually enforced is `rate_interval` to `rate_interval + +rate_interval / 12` seconds: slightly stricter than configured, never +looser, and with no external clamp. See +[SQL cache](sql-cache.md#the-rate-limiter). ## The segmenter contract diff --git a/docs/errors.md b/docs/errors.md index 37fc254..b6dd048 100644 --- a/docs/errors.md +++ b/docs/errors.md @@ -37,8 +37,17 @@ TranslationDiff::Error │ # Pragmatic computed offsets that │ # violate its own postcondition -- │ # not raised by ordinary use -└── TranslationDiff::RedisRateLimiter::RateLimitExceeded - # the configured rate_limit was exceeded +├── TranslationDiff::RedisRateLimiter::RateLimitExceeded +│ # the configured rate_limit was exceeded, +│ # raised by the Redis-backed limiter +└── TranslationDiff::ActiveRecordRateLimiter::RateLimitExceeded + # the same condition, raised by the SQL-backed + # limiter -- a distinct class under its own + # namespace, not the class above. Rescuing + # `RedisRateLimiter::RateLimitExceeded` + # specifically and switching `rate_limiter` to + # `:active_record` stops catching it; rescue + # `TranslationDiff::Error` to catch both. ``` `ProviderError` and its subclasses carry `#provider` (the registered name) diff --git a/docs/sql-cache.md b/docs/sql-cache.md index 65c6d2a..12db8b3 100644 --- a/docs/sql-cache.md +++ b/docs/sql-cache.md @@ -6,6 +6,8 @@ If you already run Postgres or MySQL and do not want to stand up Redis for one cache, `TranslationDiff::ActiveRecordCacheStore` caches translations in the application's own database instead, and `TranslationDiff::ActiveRecordRateLimiter` throttles requests there too. +Supported means exercised in CI: the suite runs against Postgres, MySQL +and SQLite on every push. Both are opt-in. Setting `redis_url` still means Redis, exactly as before -- nothing about an existing application's cache changes until you configure @@ -27,6 +29,32 @@ prunes it -- Redis expires a key for you; this store only stops serving an expired row, it does not remove it by itself. See [Pruning](#pruning-three-answers-none-imposed) below. +## 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 +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. + +## What ends up in your log + +`upsert_all` inlines values into the SQL it sends rather than binding them, +so a written translation appears verbatim in the host application's own +ActiveRecord log at `debug` level. That is a separate claim from the one in +[Configuration](configuration.md): this gem's own `logger` option never +prints content, but that says nothing about the application's own SQL log, +which sees the statement ActiveRecord actually sent. A failed write is +scrubbed -- the `TranslationDiff::Error` it raises carries the adapter's +error class, never the row, see [`write_multi`](#write_multi) -- but a +successful one is not; nothing here redacts your debug-level query log. If +your application logs SQL at `debug` and what it translates is +confidential, keep that log above `debug` around this store, or use +`RedisCacheStore` instead. + ## The tables Two tables, created by a migration you run once -- see @@ -37,10 +65,10 @@ either of them itself. | Column | Meaning | | --- | --- | -| `namespace` | `cache_namespace`. Two tenants share this table the way they share a Redis database; `#prune` only prunes its own configured namespace. | +| `namespace` | `cache_namespace`. Two tenants share this table the way they share a Redis database; `#prune` only prunes its own configured namespace. Limited to 64 characters, the column's own limit; `config.cache_namespace =` refuses a longer value at `configure` time rather than at the first write. | | `key_digest` | `Digest::SHA256.hexdigest(key)` -- 64 characters, always. The cache key itself is not stored, only its digest: a variable-length unique index is the one thing guaranteed to bite somebody on MySQL. This also means an entry is not human-readable by its key -- to find one, compute the digest the same way and look that up. | | `translation` | The cached value. | -| `expires_at` | What `cache_ttl` means in SQL. `nil` when `cache_ttl` is unset, which means the row never expires on its own. | +| `expires_at` | What `cache_ttl` means in SQL. `nil` when `cache_ttl` is unset, or set to `nil` or anything non-positive, which all mean the row never expires on its own. | | `created_at`, `updated_at` | Standard ActiveRecord timestamps, set by `upsert_all`. | Unique index on `[namespace, key_digest]` -- the second write of a key @@ -78,7 +106,7 @@ floor, not a requirement to target that version specifically): ```ruby class CreateTranslationDiffTables < ActiveRecord::Migration[7.1] def change - create_table :translation_diff_translations do |t| + 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 @@ -87,21 +115,29 @@ class CreateTranslationDiffTables < ActiveRecord::Migration[7.1] end add_index :translation_diff_translations, %i[namespace key_digest], unique: true, - name: "index_translation_diff_translations_on_key" - add_index :translation_diff_translations, :expires_at + name: "index_translation_diff_translations_on_key", if_not_exists: true + add_index :translation_diff_translations, :expires_at, if_not_exists: true - create_table :translation_diff_rate_limits do |t| + create_table :translation_diff_rate_limits, if_not_exists: true do |t| t.string :namespace, null: false, limit: 64 - t.integer :bucket, null: false + t.bigint :bucket, null: false t.integer :characters, null: false, default: 0 end add_index :translation_diff_rate_limits, %i[namespace bucket], unique: true, - name: "index_translation_diff_rate_limits_on_bucket" + name: "index_translation_diff_rate_limits_on_bucket", if_not_exists: true end end ``` +Every `create_table` and `add_index` above carries `if_not_exists: true`, so +a DBA can run this migration twice without the second run failing. `bucket` +is a `bigint`, not the plain 4-byte integer it looks like it could be: at +`bucket_width` 1 second -- what `rate_interval` under 24 seconds folds to, +see [The rate limiter](#the-rate-limiter) below -- `bucket` is the raw Unix +timestamp, and a 4-byte integer column holding that overflows in January +2038 the same way a 32-bit `time_t` does. + 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. @@ -114,6 +150,13 @@ never read, whether or not anything has deleted it yet -- expiry and deletion are two different questions here, unlike Redis, where a `SETEX` key simply stops existing. +A non-positive `cache_ttl` -- `0`, a negative number, or `nil` -- means +never expires: `expires_at` is written as `nil`, and a `nil` `expires_at` +is what "never expires on its own" means in the table above. All three +values fold to that one `nil` in `TranslationDiff.configure` itself, so +`config.cache_ttl` reads back `nil` for any of them, not just the one you +set. + ## Pruning: three answers, none imposed Deleting an expired row is a separate question from whether it is served, @@ -125,10 +168,27 @@ and there is no single right answer to "when," so none is forced on you: it into cron, a scheduled job, whatever your host already runs. Either side that does not support pruning (`:redis`, or an object of your own) is reported and skipped rather than failing the task. + + The task ships inside this gem, under `lib/translation_diff/tasks/`, not + in the dev Rakefile -- a dependency's own Rakefile is never loaded by a + host application's `rake`. On Rails, a `Railtie` wires it into the + application's own rake tasks automatically, enhanced to depend on the + `:environment` task so it runs against the application's own + configuration rather than a default one -- `rake -T` shows it with no + extra setup. Off Rails, nothing registers it automatically: `load` the + file yourself (from wherever the gem is installed) to add it to your own + `Rakefile`, and note that it does **not** get an + `:environment`-equivalent dependency there -- your own bootstrap needs to + configure `TranslationDiff` before the task runs, the same way it would + before any code that calls `translate`. A cron entry that prunes + silently against the wrong (or unconfigured) configuration is worse than + no pruning at all. - **`config.cache_prune_probability`** (default `0.0`, off). A fraction between 0 and 1: on a write, `ActiveRecordCacheStore` rolls under it and prunes if it wins. Off by default, 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. - **Doing nothing.** Also a supported answer. An unpruned table is correct -- reads still skip every expired row -- just larger than it needs to be. @@ -182,7 +242,7 @@ Both `ActiveRecordCacheStore` and `RedisCacheStore` implement the cache store contract's optional `write_multi(pairs)` -- see [`write_multi` is optional](caching.md#write_multi-is-optional) for what that means, and -[The two write paths fail differently](caching.md#the-two-write-paths-fail-differently) +[The three write paths fail differently](caching.md#the-three-write-paths-fail-differently) for how a batch write fails differently from a per-key one. `ActiveRecordCacheStore#write_multi` is a single `upsert_all` for the whole batch: a forty-sentence paragraph is one statement, not forty. @@ -193,19 +253,25 @@ batch: a forty-sentence paragraph is one statement, not forty. and `rate_interval` already configure the Redis-backed limiter, but counts characters into `translation_diff_rate_limits` instead of Redis. -The window is sliding, not tumbling. Time is divided into buckets a -fraction of `rate_interval` wide, not one bucket per interval, and a check -sums every bucket covering the trailing `rate_interval` seconds before -deciding whether the threshold is exceeded -- so the answer does not jump -the moment a single wide bucket rolls over, the way it would if the whole -interval were one bucket. Like the check it replaces, it looks at the -total *before* adding the new characters, so a check that itself pushes the -total over the threshold still succeeds; the next one raises. - -This is still approximate at a window boundary, the same way the -`ratelimit` gem this store does not depend on is. A translation throttle -exists to keep a provider's quota from being exceeded, not to be a billing -meter. +The window is sliding, not tumbling. Time is divided into buckets +`rate_interval / 12` seconds wide (never narrower than 1 second), not one +bucket per interval, and a check sums every bucket touching the trailing +`rate_interval` seconds before deciding whether the threshold is exceeded +-- so the answer does not jump the moment a single wide bucket rolls over, +the way it would if the whole interval were one bucket. Like the check it +replaces, it looks at the total *before* adding the new characters, so a +check that itself pushes the total over the threshold still succeeds; the +next one raises. + +The oldest bucket summed is only ever partially inside the window, and it +is summed in full anyway rather than pro-rated, which errs toward +stricter. So the window actually enforced is `rate_interval` to +`rate_interval + rate_interval / 12` seconds (that upper bound is one +bucket width, floored at 1 second) -- slightly stricter than what was +configured, never looser. A translation throttle exists to keep a +provider's quota from being exceeded, not to be a billing meter, so this +was left as the simpler, safer direction to be wrong in rather than made +exact. `#prune` here deletes buckets that have fully aged out of the window, in the configured namespace. `rake translation_diff:prune` calls it the same From 760d3c80488af93f8d7fcfd4c3d7aa233035a968 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:50:16 +0400 Subject: [PATCH 28/36] fix: make a non-positive or nil cache_ttl a no-op in RedisCacheStore setex(key, nil, value) raised TypeError on every write once CacheTtlOption started folding a non-positive cache_ttl to nil, after the provider had already been billed. write and write_multi now SET instead of SETEX whenever the timeout does not expire, the same rule ActiveRecordCacheStore already applies to expires_at. MemoryCacheStore never reads cache_ttl at all, confirmed with a test rather than a code change. --- lib/translation_diff/redis_cache_store.rb | 11 ++++- .../memory_cache_store_test.rb | 11 +++++ .../redis_cache_store_test.rb | 45 +++++++++++++++++++ 3 files changed, 65 insertions(+), 2 deletions(-) diff --git a/lib/translation_diff/redis_cache_store.rb b/lib/translation_diff/redis_cache_store.rb index a923639..cf3dabc 100644 --- a/lib/translation_diff/redis_cache_store.rb +++ b/lib/translation_diff/redis_cache_store.rb @@ -17,14 +17,15 @@ def read_multi(keys) redis { |redis| redis.mget(*keys) } end + # A non-positive or nil timeout means never expires, the same rule the SQL store applies to cache_ttl. def write(key, value) - redis { |redis| redis.setex(key, timeout, value) } + redis { |redis| write_one(redis, key, value) } end def write_multi(pairs) return pairs if pairs.empty? - redis { |redis| redis.pipelined { |p| pairs.each { |key, value| p.setex(key, timeout, value) } } } + redis { |redis| redis.pipelined { |p| pairs.each { |key, value| write_one(p, key, value) } } } pairs end @@ -37,6 +38,12 @@ def redis yield Redis::Namespace.new(namespace, redis: redis) end end + + def write_one(redis, key, value) + expiring? ? redis.setex(key, timeout, value) : redis.set(key, value) + end + + def expiring? = timeout.is_a?(Numeric) && timeout.positive? end TranslationDiff::Stores.register(:redis, TranslationDiff::RedisCacheStore) diff --git a/test/translation_diff/memory_cache_store_test.rb b/test/translation_diff/memory_cache_store_test.rb index 59a0fda..c6cacf3 100644 --- a/test/translation_diff/memory_cache_store_test.rb +++ b/test/translation_diff/memory_cache_store_test.rb @@ -34,6 +34,17 @@ def test_rewriting_an_entry_makes_it_the_most_recently_used assert_equal ["again", nil, "c", "d"], store.read_multi(%w[a b c d]) end + # The regression that broke the Redis store: this one takes no timeout at all, so a nil cache_ttl is a no-op here. + def test_build_ignores_a_nil_cache_ttl_and_writes_normally + config = TranslationDiff::Configuration.new + config.cache_ttl = nil + + built = TranslationDiff::MemoryCacheStore.build(config) + built.write("a", "one") + + assert_equal ["one"], built.read_multi(["a"]) + end + def test_build_takes_its_bound_from_the_configuration config = TranslationDiff::Configuration.new config.cache_max_size = 1 diff --git a/test/translation_diff/redis_cache_store_test.rb b/test/translation_diff/redis_cache_store_test.rb index 2476175..5b15d09 100644 --- a/test/translation_diff/redis_cache_store_test.rb +++ b/test/translation_diff/redis_cache_store_test.rb @@ -19,6 +19,10 @@ def setex(key, timeout, value) @redis.setex("#{@namespace}:#{key}", timeout, value) end + def set(key, value) + @redis.set("#{@namespace}:#{key}", value) + end + def pipelined @redis.pipelined { |pipeline| yield self.class.new(@namespace, redis: pipeline) } end @@ -51,6 +55,12 @@ def setex(key, timeout, value) "OK" end + def set(key, value) + @calls << [:set, key, value] + @entries[key] = value + "OK" + end + def pipelined @calls << [:pipelined] yield self @@ -104,6 +114,41 @@ def test_write_multi_of_no_pairs_never_opens_a_pipeline assert_empty redis.calls end + # The regression this closes: `setex(key, nil, value)` raised TypeError on every write against the default store. + def test_write_with_a_nil_timeout_never_expires + redis = FakeRedis.new + + build_store(redis, timeout: nil).write("a", "b") + + assert_equal [[:set, "translation-diff:a", "b"]], redis.calls + end + + def test_write_with_a_zero_timeout_never_expires + redis = FakeRedis.new + + build_store(redis, timeout: 0).write("a", "b") + + assert_equal [[:set, "translation-diff:a", "b"]], redis.calls + end + + def test_write_with_a_negative_timeout_never_expires + redis = FakeRedis.new + + build_store(redis, timeout: -1).write("a", "b") + + assert_equal [[:set, "translation-diff:a", "b"]], redis.calls + end + + def test_write_multi_with_a_nil_timeout_never_expires + redis = FakeRedis.new + + build_store(redis, timeout: nil).write_multi([%w[a one], %w[b two]]) + + assert_equal [[:pipelined], + [:set, "translation-diff:a", "one"], + [:set, "translation-diff:b", "two"]], redis.calls + end + private def build_store(redis, **) From 1f09b4520c1ee0853a9bc9b9f088853e30dcad78 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:51:17 +0400 Subject: [PATCH 29/36] fix: sever the cause chain on the cache store's redacted error Ruby attaches the rescued ActiveRecord::StatementInvalid as #cause unless told otherwise, so e.cause.message and e.full_message still carried the row verbatim -- exactly what full_message prints at the top level and what Sentry and Rails' error reporter capture. raise now passes cause: nil so the chain stops at the redacted error. --- lib/translation_diff/active_record_cache_store.rb | 2 +- test/translation_diff/active_record_redaction_test.rb | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb index 4672b0a..6c693b3 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 ActiveRecord::StatementInvalid => e - raise redacted_error(e) + raise redacted_error(e), cause: nil end # Reads never serve an expired row; deleting one is this, and it is the host's call when to run it. diff --git a/test/translation_diff/active_record_redaction_test.rb b/test/translation_diff/active_record_redaction_test.rb index 582e4a3..d9e3b62 100644 --- a/test/translation_diff/active_record_redaction_test.rb +++ b/test/translation_diff/active_record_redaction_test.rb @@ -37,6 +37,17 @@ def test_the_redacted_error_names_the_adapters_own_error_class assert_includes error.message, "PG::CheckViolation" end + # Ruby attaches the rescued original as #cause unless the raise says otherwise -- and #cause carries the row. + def test_the_redacted_error_severs_the_cause_chain + store = TranslationDiff::ActiveRecordCacheStore.new(namespace: "forbidden-namespace", ttl: 60, + table_name: "translation_diff_translations") + + error = assert_raises(TranslationDiff::Error) { store.write("a", "SECRET-PATIENT-NOTE-12345") } + + assert_nil error.cause + refute_includes error.full_message, "SECRET-PATIENT-NOTE-12345" + end + private def connection From 860716852a75461c303feba252e1a4ad8cb498cc Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:54:16 +0400 Subject: [PATCH 30/36] fix: give prune_sometimes its own savepoint and its own error message prune_sometimes ran after write's savepoint had already closed, so a failing prune (reachable whenever cache_prune_probability is above zero) poisoned a transaction the caller owns -- the original finding, reachable again by a different path -- and its DELETE held row locks for the rest of that transaction. It also reused write's redacted-error message, reporting a failed prune as a failed upsert. prune_sometimes now runs the delete in its own requires_new transaction and raises a message naming the delete it actually ran. --- .../active_record_cache_store.rb | 14 +++- .../active_record_transaction_test.rb | 83 +++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb index 6c693b3..af41841 100644 --- a/lib/translation_diff/active_record_cache_store.rb +++ b/lib/translation_diff/active_record_cache_store.rb @@ -64,6 +64,13 @@ def redacted_error(error) "#{@table_name}(namespace, key_digest, translation, expires_at)") end + # Its own message, naming the statement prune actually runs -- a failed prune is not a failed upsert. + def redacted_prune_error(error) + adapter_error = error.cause&.class || error.class + TranslationDiff::Error.new("the cache prune failed (#{adapter_error}): a delete from " \ + "#{@table_name}(namespace, expires_at)") + end + def row(key, value) { namespace: @namespace, key_digest: digest(key), translation: value, expires_at: expires_at } end @@ -78,8 +85,13 @@ def live .where(expires_at: nil).or(model.where(namespace: @namespace).where(expires_at: Time.now.utc...)) end + # Its own savepoint, not write's -- a failing prune must not poison a transaction the caller owns either. def prune_sometimes - prune if @prune_probability.positive? && rand < @prune_probability + return unless @prune_probability.positive? && rand < @prune_probability + + model.transaction(requires_new: true) { prune } + rescue ActiveRecord::StatementInvalid => e + raise redacted_prune_error(e), cause: nil end def active_record_feature = "the cache" diff --git a/test/translation_diff/active_record_transaction_test.rb b/test/translation_diff/active_record_transaction_test.rb index 86cb23c..1a036a7 100644 --- a/test/translation_diff/active_record_transaction_test.rb +++ b/test/translation_diff/active_record_transaction_test.rb @@ -36,8 +36,91 @@ def test_a_successful_write_still_lands assert_equal ["one"], store.read_multi(["a"]) end + # The path the review found reachable again: prune runs after write's own savepoint has already closed. + def test_a_failing_prune_leaves_the_callers_transaction_usable + store = store_with_a_pending_prune + harness = harness_model + + harness.transaction do + write_ignoring_errors(store, "fresh", "two") + + assert harness.create!(namespace: "harness", key_digest: "d" * 64, translation: "still usable") + end + + assert_equal 1, harness.where(namespace: "harness").count + ensure + remove_failing_delete_trigger + end + + # The write itself still lands even though the prune that follows it fails, since each has its own savepoint. + def test_a_failing_prune_does_not_undo_the_write_that_triggered_it + store = store_with_a_pending_prune + + write_ignoring_errors(store, "fresh", "two") + + assert_equal ["two"], store.read_multi(["fresh"]) + ensure + remove_failing_delete_trigger + end + + # The regression the review found: a failing prune was reported as the wrong statement, an upsert. + def test_a_failing_prunes_error_describes_the_delete_not_the_upsert + store = store_with_a_pending_prune + + error = assert_raises(TranslationDiff::Error) { store.write("fresh", "two") } + + refute_includes error.message, "upsert" + assert_includes error.message, "prune" + ensure + remove_failing_delete_trigger + 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 write_ignoring_errors(store, key, value) + store.write(key, value) + rescue StandardError + nil + end + + # A trigger, not a constraint: only a per-row trigger can make the DELETE itself fail, not an INSERT. + def install_failing_delete_trigger + harness_model.connection.execute(<<~SQL) + CREATE OR REPLACE FUNCTION translation_diff_transaction_test_fail_delete() RETURNS trigger AS $$ + BEGIN RAISE EXCEPTION 'simulated prune failure'; END; $$ LANGUAGE plpgsql; + CREATE TRIGGER translation_diff_transaction_test_fail_delete BEFORE DELETE + ON translation_diff_translations FOR EACH ROW + EXECUTE FUNCTION translation_diff_transaction_test_fail_delete(); + SQL + end + + def remove_failing_delete_trigger + connection = harness_model.connection + connection.execute("DROP TRIGGER IF EXISTS translation_diff_transaction_test_fail_delete " \ + "ON translation_diff_translations") + connection.execute("DROP FUNCTION IF EXISTS translation_diff_transaction_test_fail_delete()") + end + def harness_model TranslationDiff::ActiveRecordCacheStore.new(namespace: "harness", ttl: 60, table_name: "translation_diff_translations").model From a5dd52a52611997221505d48a3bfa9d20535ca33 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:57:25 +0400 Subject: [PATCH 31/36] fix: keep the friendly missing-gem error when ActiveRecord truly is not installed `rescue ActiveRecord::StatementInvalid` evaluates its class expression for every exception the body raises, including the TranslationDiff::Error build_model already raises for a missing activerecord gem. With the constant genuinely undefined that evaluation itself raised NameError, hiding the friendly message read_multi still produces. Both rescue clauses in ActiveRecordCacheStore now catch StandardError and check defined?(ActiveRecord::StatementInvalid) before is_a?, so an unrelated error -- including that one -- passes through unchanged. --- lib/translation_diff/active_record_cache_store.rb | 13 +++++++++++-- .../active_record_cache_store_test.rb | 14 ++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/lib/translation_diff/active_record_cache_store.rb b/lib/translation_diff/active_record_cache_store.rb index af41841..7a96634 100644 --- a/lib/translation_diff/active_record_cache_store.rb +++ b/lib/translation_diff/active_record_cache_store.rb @@ -40,7 +40,9 @@ def write_multi(pairs) end prune_sometimes pairs - rescue ActiveRecord::StatementInvalid => e + rescue StandardError => e + raise unless ar_statement_invalid?(e) + raise redacted_error(e), cause: nil end @@ -90,10 +92,17 @@ def prune_sometimes return unless @prune_probability.positive? && rand < @prune_probability model.transaction(requires_new: true) { prune } - rescue ActiveRecord::StatementInvalid => e + rescue StandardError => e + raise unless ar_statement_invalid?(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) + 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/test/translation_diff/active_record_cache_store_test.rb b/test/translation_diff/active_record_cache_store_test.rb index 1fcb18c..3b72967 100644 --- a/test/translation_diff/active_record_cache_store_test.rb +++ b/test/translation_diff/active_record_cache_store_test.rb @@ -130,6 +130,20 @@ def test_write_multi_keeps_unique_by_when_the_connection_supports_a_conflict_tar assert_includes options.keys, :unique_by end + # Naming the bare `ActiveRecord` constant in the rescue clause used to raise a raw NameError instead of this + # gem's own message, genuinely reproduced here rather than merely simulated by stubbing #require. + def test_write_raises_a_friendly_error_when_active_record_is_genuinely_unavailable + removed = Object.send(:remove_const, :ActiveRecord) + broken_store = build_store + broken_store.define_singleton_method(:require) { |*| raise LoadError } + + error = assert_raises(TranslationDiff::Error) { broken_store.write("a", "one") } + + assert_match(/`activerecord` gem is not available/, error.message) + ensure + Object.const_set(:ActiveRecord, removed) if removed + end + private def build_store(namespace: "translation-diff", ttl: 604_800) From f99edc5e31ed642dec2f9aee5097abf2e1421a57 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:58:17 +0400 Subject: [PATCH 32/36] fix: enforce the 0..1 range cache_prune_probability's own message promises The guard coerced any numeric string or number, so 2.0 (prune on every write) and -1 passed through unrejected even though the raised message already claimed "between 0 and 1". coerce_probability now checks the coerced value against that range before returning it. --- lib/translation_diff/cache_guard_options.rb | 11 +++++++++-- test/translation_diff/configuration_test.rb | 22 +++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/translation_diff/cache_guard_options.rb b/lib/translation_diff/cache_guard_options.rb index db4cf0d..53a0f92 100644 --- a/lib/translation_diff/cache_guard_options.rb +++ b/lib/translation_diff/cache_guard_options.rb @@ -19,9 +19,16 @@ def cache_namespace=(value) private def coerce_probability(value) - Float(value) + probability = Float(value) + raise probability_out_of_range(value) unless (0..1).cover?(probability) + + probability rescue ArgumentError, TypeError - raise TranslationDiff::Error, "cache_prune_probability must be a number between 0 and 1 (got #{value.inspect})" + raise probability_out_of_range(value) + end + + def probability_out_of_range(value) + TranslationDiff::Error.new("cache_prune_probability must be a number between 0 and 1 (got #{value.inspect})") end def namespace_too_long(value) diff --git a/test/translation_diff/configuration_test.rb b/test/translation_diff/configuration_test.rb index 2cecebe..4ecdd45 100644 --- a/test/translation_diff/configuration_test.rb +++ b/test/translation_diff/configuration_test.rb @@ -71,6 +71,28 @@ def test_cache_prune_probability_refuses_a_non_numeric_string_with_a_clear_messa assert_match(/cache_prune_probability/, error.message) end + def test_cache_prune_probability_refuses_a_value_above_one + error = assert_raises(TranslationDiff::Error) { @config.cache_prune_probability = 2.0 } + + assert_match(/between 0 and 1/, error.message) + end + + def test_cache_prune_probability_refuses_a_negative_value + error = assert_raises(TranslationDiff::Error) { @config.cache_prune_probability = -1 } + + assert_match(/between 0 and 1/, error.message) + end + + def test_cache_prune_probability_accepts_the_boundary_values + @config.cache_prune_probability = 0 + + assert_in_delta 0.0, @config.cache_prune_probability + + @config.cache_prune_probability = 1 + + assert_in_delta 1.0, @config.cache_prune_probability + end + def test_cache_namespace_longer_than_64_characters_is_refused_at_configure_time error = assert_raises(TranslationDiff::Error) { @config.cache_namespace = "n" * 65 } From 74ba039a7a59095c930a9429cbde94ae7ee65426 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 19:59:51 +0400 Subject: [PATCH 33/36] fix: coerce a String cache_ttl the way the neighbouring guards already do config.cache_ttl = "3600" -- what an ENV var hands you -- passed through untouched and died at Time.now.utc + @ttl with TypeError, after the provider call had already been made. A non-empty String is now coerced with Integer() at configure time, refusing a non-numeric one with a clear TranslationDiff::Error instead of a bare TypeError mid-translation. --- lib/translation_diff/cache_ttl_option.rb | 10 ++++++++++ .../active_record_cache_store_test.rb | 12 ++++++++++++ test/translation_diff/configuration_test.rb | 18 ++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/lib/translation_diff/cache_ttl_option.rb b/lib/translation_diff/cache_ttl_option.rb index f920d84..015f80f 100644 --- a/lib/translation_diff/cache_ttl_option.rb +++ b/lib/translation_diff/cache_ttl_option.rb @@ -10,10 +10,20 @@ def initialize # A non-positive number folds into nil too -- a TTL of zero or less can never keep a row. def cache_ttl=(value) value = nil if value.is_a?(String) && value.strip.empty? + value = coerce_ttl(value) if value.is_a?(String) @cache_ttl = value.is_a?(Numeric) && value <= 0 ? nil : value end def cache_ttl @cache_ttl.equal?(NEVER_ASSIGNED) ? self.class.defaults[:cache_ttl] : @cache_ttl end + + private + + # An ENV var arrives as a String; coerced here so Time.now.utc + @ttl never meets a bare String mid-translation. + def coerce_ttl(value) + Integer(value) + rescue ArgumentError, TypeError + raise TranslationDiff::Error, "cache_ttl must be a number of seconds (got #{value.inspect})" + end end diff --git a/test/translation_diff/active_record_cache_store_test.rb b/test/translation_diff/active_record_cache_store_test.rb index 3b72967..725d721 100644 --- a/test/translation_diff/active_record_cache_store_test.rb +++ b/test/translation_diff/active_record_cache_store_test.rb @@ -105,6 +105,18 @@ def test_build_takes_its_settings_from_the_configuration assert_equal "from-config", built.model.first.namespace end + def test_a_string_cache_ttl_from_env_does_not_raise_on_write + config = TranslationDiff::Configuration.new + config.cache_namespace = "translation-diff" + config.cache_table_name = "translation_diff_translations" + config.cache_ttl = "3600" + + built = TranslationDiff::ActiveRecordCacheStore.build(config) + built.write("a", "one") + + refute_nil built.model.first.expires_at + end + def test_a_string_cache_prune_probability_from_env_does_not_raise_on_write config = TranslationDiff::Configuration.new config.cache_namespace = "translation-diff" diff --git a/test/translation_diff/configuration_test.rb b/test/translation_diff/configuration_test.rb index 4ecdd45..3d59820 100644 --- a/test/translation_diff/configuration_test.rb +++ b/test/translation_diff/configuration_test.rb @@ -59,6 +59,24 @@ def test_a_negative_cache_ttl_also_means_never_expires assert_nil @config.cache_ttl end + def test_cache_ttl_coerces_a_numeric_string_the_way_an_env_var_arrives + @config.cache_ttl = "3600" + + assert_equal 3600, @config.cache_ttl + end + + def test_a_coerced_non_positive_cache_ttl_string_also_means_never_expires + @config.cache_ttl = "0" + + assert_nil @config.cache_ttl + end + + def test_cache_ttl_refuses_a_non_numeric_string_with_a_clear_message + error = assert_raises(TranslationDiff::Error) { @config.cache_ttl = "lots" } + + assert_match(/cache_ttl/, error.message) + end + def test_cache_prune_probability_coerces_a_numeric_string_the_way_an_env_var_arrives @config.cache_prune_probability = "0.5" From c49e3f25349e8547b273c7db18d4e086defc993d Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 20:01:23 +0400 Subject: [PATCH 34/36] test: cover write_multi's dedupe against PostgreSQL's own conflict rule pairs.to_h in write_multi is the only thing standing between a document with a repeated sentence and PostgreSQL's "ON CONFLICT DO UPDATE command cannot affect row a second time" -- nothing exercised that against a real server. Confirmed by hand that running the same batch through the undeduped shape write_multi builds internally raises PG::CardinalityViolation on PostgreSQL, then wired that into its own Postgres-gated test file alongside a same-batch write_multi call that must not raise. --- .../active_record_write_multi_dedupe_test.rb | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 test/translation_diff/active_record_write_multi_dedupe_test.rb diff --git a/test/translation_diff/active_record_write_multi_dedupe_test.rb b/test/translation_diff/active_record_write_multi_dedupe_test.rb new file mode 100644 index 0000000..02a1716 --- /dev/null +++ b/test/translation_diff/active_record_write_multi_dedupe_test.rb @@ -0,0 +1,52 @@ +require "test_helper" +require "support/active_record_database" + +if ActiveRecordDatabase.postgres? + ActiveRecordDatabase.connect! + + # write_multi dedupes pairs.to_h before the upsert; without that, PostgreSQL raises "ON CONFLICT DO UPDATE + # command cannot affect row a second time" the moment a batch repeats a key -- only PostgreSQL enforces this. + class ActiveRecordWriteMultiDedupeTest < Minitest::Test + def setup + ActiveRecordDatabase.truncate + end + + def test_write_multi_does_not_raise_when_a_document_repeats_the_same_sentence + store = build_store + + store.write_multi([%w[a one], %w[a two]]) + + assert_equal ["two"], store.read_multi(["a"]) + end + + # Proves the dedupe in write_multi is load-bearing, by running the same batch through the undeduped shape + # write_multi builds internally -- without pairs.to_h, this exact scenario raises on PostgreSQL. + def test_without_the_dedupe_postgresql_raises_on_a_repeated_key_in_one_batch + store = build_store + pairs = [%w[a one], %w[a two]] + + error = assert_raises(ActiveRecord::StatementInvalid) do + store.model.transaction(requires_new: true) do + rows = pairs.map { |key, value| store.send(:row, key, value) } + store.model.upsert_all(rows, unique_by: %i[namespace key_digest], record_timestamps: true) + end + end + + assert_match(/cannot affect row a second time/, error.message) + end + + private + + def build_store + TranslationDiff::ActiveRecordCacheStore.new(namespace: "translation-diff", ttl: 60, + table_name: "translation_diff_translations") + end + end +else + class ActiveRecordWriteMultiDedupeTest < Minitest::Test + def test_postgres_is_unavailable + skip "TRANSLATION_DIFF_DATABASE_URL does not name a PostgreSQL database; " \ + "only PostgreSQL raises when an upsert batch repeats a conflict key" + end + end +end From 825b937ccc2a0826b3df2379ccdf8d168664eb14 Mon Sep 17 00:00:00 2001 From: IG Date: Thu, 10 Sep 2026 20:11:34 +0400 Subject: [PATCH 35/36] test: pin the rate limiter's prune tests to a clock that holds still Also documents the severed cause chain, the prune savepoint and the two configure-time guards. --- docs/configuration.md | 4 ++-- docs/sql-cache.md | 10 +++++++--- .../active_record_rate_limiter_test.rb | 9 +++++---- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 8ad2d33..898bddd 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -55,13 +55,13 @@ at all, so an unset environment variable never has to be special-cased. | --- | --- | --- | | `provider` | `:deepl` | The translation provider: a registered name or a `TranslationDiff::Provider` of your own. See [Providers](providers.md). | | `cache` | `nil` | The cache store: a registered name or an object satisfying the [cache store contract](caching.md#the-cache-store-contract). `nil` means "choose for me" -- see below. | -| `cache_ttl` | `604_800` (one week) | Seconds an entry is kept before it expires. Read by `RedisCacheStore` (a `SETEX`) and by `ActiveRecordCacheStore` (written into each row's `expires_at`); `MemoryCacheStore` evicts by size instead and ignores it. A non-positive value (`0` or less, or `nil`) means never expires. See [SQL cache](sql-cache.md#cache_ttl-becomes-expires_at). | +| `cache_ttl` | `604_800` (one week) | Seconds an entry is kept before it expires. Read by `RedisCacheStore` (a `SETEX`) and by `ActiveRecordCacheStore` (written into each row's `expires_at`); `MemoryCacheStore` evicts by size instead and ignores it. A non-positive value (`0` or less, or `nil`) means never expires. A String is coerced, so an environment variable works; a value that is not a number is refused at `configure` time rather than mid-translation. See [SQL cache](sql-cache.md#cache_ttl-becomes-expires_at). | | `cache_namespace` | `"translation-diff"` | Prefix applied to every Redis key this gem writes -- both cache entries and the rate limiter's own bookkeeping. Also the `namespace` column both SQL tables share and the unit `ActiveRecordCacheStore#prune` operates on. At most 64 characters -- longer is refused at `configure` time. See [SQL cache](sql-cache.md#the-tables). | | `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). | -| `cache_prune_probability` | `0.0` | Chance, per write, that `ActiveRecordCacheStore` prunes expired rows before returning. `0.0` is off; `rake translation_diff:prune` is the other way to prune. See [SQL cache](sql-cache.md#pruning-three-answers-none-imposed). | +| `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. | diff --git a/docs/sql-cache.md b/docs/sql-cache.md index 12db8b3..53bec08 100644 --- a/docs/sql-cache.md +++ b/docs/sql-cache.md @@ -49,8 +49,9 @@ ActiveRecord log at `debug` level. That is a separate claim from the one in prints content, but that says nothing about the application's own SQL log, which sees the statement ActiveRecord actually sent. A failed write is scrubbed -- the `TranslationDiff::Error` it raises carries the adapter's -error class, never the row, see [`write_multi`](#write_multi) -- but a -successful one is not; nothing here redacts your debug-level query log. If +error class, never the row, and its cause chain is severed so the original +exception cannot carry the row into an error tracker either, see +[`write_multi`](#write_multi) -- but a successful one is not; nothing here redacts your debug-level query log. If your application logs SQL at `debug` and what it translates is confidential, keep that log above `debug` around this store, or use `RedisCacheStore` instead. @@ -185,7 +186,10 @@ and there is no single right answer to "when," so none is forced on you: no pruning at all. - **`config.cache_prune_probability`** (default `0.0`, off). A fraction between 0 and 1: on a write, `ActiveRecordCacheStore` rolls under it and - prunes if it wins. Off by default, because a translation-serving request + prunes if it wins, in a savepoint of its own so a failed prune cannot + abort a transaction the caller opened. A value outside `0.0..1.0`, or one + that is not a number, is refused at `configure` time. Off by default, + 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. diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb index 75eda74..1c30873 100644 --- a/test/translation_diff/active_record_rate_limiter_test.rb +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -5,7 +5,8 @@ if ActiveRecordDatabase.available? ActiveRecordDatabase.connect! - # Moves without sleeping, so a bucket can be made to roll over on demand instead of waited out. + # Moves without sleeping, so a bucket can be made to roll over on demand -- and holds still, so a real + # boundary cannot fall between two reads of it, which is what made the prune tests flake. class MutableClock def initialize(now) = @now = now def call = @now @@ -68,7 +69,7 @@ def test_a_bucket_beyond_int32_range_is_stored_and_read_back end def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one - limiter = build_limiter(threshold: 1000, interval: 60) + limiter = build_limiter(threshold: 1000, interval: 60, clock: MutableClock.new(Time.at(1_700_000_000))) model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket) - 1, characters: 5) limiter.check(10) @@ -81,7 +82,7 @@ def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one # The oldest bucket is only ever partially inside the window (see current_total), so prune leaving it alone # is what keeps pruning from quietly undoing the strictness that sum starting at oldest_bucket relies on. def test_prune_leaves_the_oldest_bucket_because_the_window_still_counts_it - limiter = build_limiter(threshold: 1000, interval: 60) + limiter = build_limiter(threshold: 1000, interval: 60, clock: MutableClock.new(Time.at(1_700_000_000))) model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket), characters: 5) deleted = limiter.prune @@ -91,7 +92,7 @@ def test_prune_leaves_the_oldest_bucket_because_the_window_still_counts_it end def test_prune_only_deletes_rows_in_its_own_namespace - own = build_limiter(threshold: 1000, interval: 60) + own = build_limiter(threshold: 1000, interval: 60, clock: MutableClock.new(Time.at(1_700_000_000))) model.create!(namespace: "translation-diff", bucket: own.send(:oldest_bucket) - 1, characters: 5) model.create!(namespace: "other-tenant", bucket: own.send(:oldest_bucket) - 1, characters: 5) From fb7047871333221460dcc687817271334a313f25 Mon Sep 17 00:00:00 2001 From: IG Date: Fri, 11 Sep 2026 00:44:00 +0400 Subject: [PATCH 36/36] test: pull the frozen clock into a helper to satisfy rubocop 1.91 --- test/translation_diff/active_record_rate_limiter_test.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/translation_diff/active_record_rate_limiter_test.rb b/test/translation_diff/active_record_rate_limiter_test.rb index 1c30873..00b152d 100644 --- a/test/translation_diff/active_record_rate_limiter_test.rb +++ b/test/translation_diff/active_record_rate_limiter_test.rb @@ -69,7 +69,7 @@ def test_a_bucket_beyond_int32_range_is_stored_and_read_back end def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one - limiter = build_limiter(threshold: 1000, interval: 60, clock: MutableClock.new(Time.at(1_700_000_000))) + limiter = build_limiter(threshold: 1000, interval: 60, clock: frozen_clock) model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket) - 1, characters: 5) limiter.check(10) @@ -82,7 +82,7 @@ def test_prune_deletes_buckets_older_than_the_window_and_leaves_the_current_one # The oldest bucket is only ever partially inside the window (see current_total), so prune leaving it alone # is what keeps pruning from quietly undoing the strictness that sum starting at oldest_bucket relies on. def test_prune_leaves_the_oldest_bucket_because_the_window_still_counts_it - limiter = build_limiter(threshold: 1000, interval: 60, clock: MutableClock.new(Time.at(1_700_000_000))) + limiter = build_limiter(threshold: 1000, interval: 60, clock: frozen_clock) model.create!(namespace: "translation-diff", bucket: limiter.send(:oldest_bucket), characters: 5) deleted = limiter.prune @@ -92,7 +92,7 @@ def test_prune_leaves_the_oldest_bucket_because_the_window_still_counts_it end def test_prune_only_deletes_rows_in_its_own_namespace - own = build_limiter(threshold: 1000, interval: 60, clock: MutableClock.new(Time.at(1_700_000_000))) + own = build_limiter(threshold: 1000, interval: 60, clock: frozen_clock) model.create!(namespace: "translation-diff", bucket: own.send(:oldest_bucket) - 1, characters: 5) model.create!(namespace: "other-tenant", bucket: own.send(:oldest_bucket) - 1, characters: 5) @@ -192,6 +192,9 @@ def test_add_quotes_the_table_name_in_the_on_duplicate_fragment def rate_limit_exceeded_error = TranslationDiff::ActiveRecordRateLimiter::RateLimitExceeded + # Held still, so a five-second bucket boundary cannot fall between two reads of the clock. + def frozen_clock = MutableClock.new(Time.at(1_700_000_000)) + def build_limiter(threshold:, interval:, namespace: "translation-diff", clock: -> { Time.now }) TranslationDiff::ActiveRecordRateLimiter.new(namespace: namespace, threshold: threshold, interval: interval, table_name: "translation_diff_rate_limits", clock: clock)