feat: keep the cache and the rate limit in the database you already run - #22
Merged
Conversation
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.
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.
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.
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.
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.
…fense 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.
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.
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.
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.
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.
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).
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.
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.
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.
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.
…nd 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.
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.
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.
…_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.
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.
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.
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.
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.
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.
…ot 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.
…mises 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.
…y 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.
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.
Also documents the severed cause chain, the prune savepoint and the two configure-time guards.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An application that already runs PostgreSQL, MySQL or SQLite can now keep its
translation cache and its rate limit there, instead of standing up Redis for
them. Reference for the approach:
bolshakov/stoplight#812.Nothing here is breaking. The default resolution is untouched —
redis_urlset still means Redis — the cache key is byte-identical, and no warm cache is
invalidated.
The store
Two tables,
translation_diff_translationsandtranslation_diff_rate_limits.cache_ttlbecomes anexpires_atcolumn: a row past it is never read,whether or not anything has deleted it yet.
cache_namespaceis a column, sotwo tenants share a table the way they share a Redis database.
The cache key itself is not stored — only its SHA256. Sixty-four characters
whatever the key was, which is what keeps the unique index inside MySQL's
utf8mb4 prefix limit. Entries are therefore not human-readable by key.
activerecordis never a dependency of this gem. The host supplies it, andthe store requires it on first use, the way
RedisCacheStorealready requiresredis. ActiveRecord 7.1 or newer, refused by name at build time.The rate limiter
config.rate_limiter = :active_record, resolved through a newRateLimitersregistry beside the store registry; assigning an object still bypasses both.
Without this, an application on SQL could cache but not throttle, and the
throttle is what keeps a provider from cutting it off.
The window is sliding, not tumbling: buckets are
interval / 12wide and acheck sums every bucket covering the last
intervalseconds, so it errsstrict — measured worst case is 1.06× the configured rate with DeepL-sized
batches. The first version summed one bucket and let 16,000 characters
through an 8,000/60s throttle inside a true minute.
write_multiThe cache store contract gains an optional batch write. A forty-sentence
paragraph was forty round trips to persist one translation; it is now one
statement. Optional means what it says: a store implementing only
read_multiand
writekeeps working, and a test with a store that implements exactlythose two proves it. Both shipped stores implement it — Redis pipelines, SQL
upserts a batch.
Schema and pruning
rails generate translation_diff:installwrites the migration; its body is indocs/sql-cache.mdverbatim for anyone not on Rails. It is idempotent, so aDBA can apply it twice. The gem never runs DDL itself.
rake translation_diff:pruneships inside the gem and reaches a Railsapplication through a Railtie, with
:environment, so it prunes theapplication's configuration rather than a default one.
cache_prune_probabilityprunes on a write that rolls under it, in a savepoint of its own. Doing
nothing is also supported: an unpruned table is correct, merely larger.
Two things a Redis user should know before switching
discards translations already paid for. A failed write can no longer poison
that transaction — it is wrapped in a savepoint — but the rollback semantics
are real and
docs/sql-cache.mdsays so.upsert_allinlines values rather than binding them,so a translation appears verbatim in the application's own ActiveRecord log
at
debug. This gem's own errors are scrubbed and their cause chain severed,so nothing reaches an error tracker; the application's query log is its own
decision.
Testing
CI runs the suite against SQLite, PostgreSQL and MySQL. The concurrency tests
— two writers racing one key, two limiters incrementing one bucket — skip on
SQLite rather than pretend, because it has one writer.
659 runs on SQLite, 667 against PostgreSQL, 659 against MySQL. Rubocop clean.
Three review rounds found eleven findings; the fixes for them introduced three
regressions of their own, all closed and re-verified against live Redis,
PostgreSQL 17 and MySQL 9.7. The one that mattered most never touched SQL at
all: folding a non-positive
cache_ttlto nil madeRedisCacheStoreraise onevery write, for applications that had not enabled any of this.