Skip to content

Bug fixes#9

Open
bdurand wants to merge 1 commit into
mainfrom
detailed-review
Open

Bug fixes#9
bdurand wants to merge 1 commit into
mainfrom
detailed-review

Conversation

@bdurand

@bdurand bdurand commented Jul 5, 2026

Copy link
Copy Markdown
Owner

Fixed

  • Queries on relations with conditions that cannot be represented in the cache key (SQL string conditions, ranges, not, or, joins, group, having, from, offset, locking, or non-hash find_by arguments) now bypass the cache instead of silently ignoring those conditions and returning or caching the wrong record.
  • create_with values are no longer included in cache keys, so queries using create_with can now be cached correctly.
  • Queries inside an open transaction now bypass the cache so that uncommitted data can never be cached. Previously a rolled back transaction could permanently poison the cache with data that was never committed. Transactions created with joinable: false (such as Rails transactional test fixtures) still use the cache.
  • Cache invalidation now runs even when caching is disabled. Previously records changed inside a disable block (or while caching was disabled globally) left stale entries behind for when caching was re-enabled.
  • Cache key values are now cast through the attribute type, so equivalent values (e.g. :one and "one", or "5" and 5) produce the same cache key. Previously such entries could be written under keys that the invalidation callbacks could never delete. This also fixes cache keys for false attribute values, which were previously indistinguishable from nil.
  • load_cache no longer raises an error when caching is disabled and now honors where conditions on cache_by configurations instead of caching records that do not match them. It also refreshes existing cache entries instead of skipping them.
  • A cache_by configuration whose where clause does not match a query no longer prevents later configurations from matching, and no longer mutates the query attributes while matching.
  • Calling cache_by in a subclass no longer mutates the superclass's cache configuration.
  • Models that include SupportTableCache without calling cache_by no longer raise an error on find_by.
  • Calling cache_belongs_to more than once for the same association no longer causes infinite recursion when reading the association.
  • SupportTableCache::MemoryCache now synchronizes all access to the underlying hash (previously reads, deletes, and clears were unsynchronized), purges expired entries, and no longer serializes values twice on a cache miss.
  • SupportTableCache::FiberLocals now stores state in the fiber's native local storage so that state cannot leak from fibers that are garbage collected while suspended inside a block.

@greptile-apps

greptile-apps Bot commented Jul 12, 2026

Copy link
Copy Markdown

Greptile Summary

This PR is a broad correctness fix for SupportTableCache, addressing over a dozen independently reported bugs around cache poisoning, stale invalidation, and incorrect key generation. All changes are additive safety guards or targeted replacements of broken logic.

  • Cache correctness: Queries with non-equality conditions (SQL strings, ranges, joins, not, or, group, having, etc.) now bypass the cache via the new support_table_cacheable_scope? guard; queries inside a joinable transaction are also bypassed to prevent rollback-poisoning the cache. The where filter in cache_by no longer mutates query attributes or blocks later configurations from matching.
  • Key integrity: Cache keys now type-cast attribute values (so :one/"one" and 5/"5" produce the same key), false is distinguished from nil, and create_with values are excluded; invalidation callbacks are decoupled from the disabled flag so stale entries can no longer survive a disable block.
  • Infrastructure: FiberLocals is rewritten to use Thread.current[] (fiber-local in MRI Ruby) eliminating the shared mutex+hash approach; MemoryCache adds mutex guards to reads, deletes, and clears; cache_belongs_to guards its alias_method call to prevent infinite recursion on repeated calls.

Confidence Score: 4/5

The PR fixes real bugs and is safe to merge; the changes are well-tested and the logic is sound throughout.

All changes correct previously broken behavior and each fix is backed by a new test. The two main areas to watch are: (1) support_table_cacheable_scope? uses where_clause.send(:predicates) to access a private ActiveRecord method—if this API moves between Rails versions the guard silently stops working, causing cache bypasses rather than returning wrong data; (2) open_transaction? calls connection.current_transaction which is also an internal Rails method and could raise if its behavior changes. Neither is a correctness regression relative to the old code, but both create a future-compatibility dependency worth tracking.

lib/support_table_cache/relation_override.rb (private-API predicate count) and lib/support_table_cache.rb (open_transaction? internals and where matching without type casting) are the places most likely to need revisiting on a Rails version upgrade.

Important Files Changed

Filename Overview
lib/support_table_cache.rb Core module: fixes superclass mutation in cache_by, separates invalidation cache from read cache, adds type casting in cache_key, introduces cache_key_for_query and open_transaction? helpers; where-matching in cache_key_for_query skips type casting
lib/support_table_cache/relation_override.rb Adds support_table_cacheable_scope? to gate caching on safe equality-only conditions; uses private where_clause.send(:predicates) for predicate count comparison
lib/support_table_cache/find_by_override.rb Simplified: rejects non-hash args early, defers scoped queries to RelationOverride, adds transaction guard, delegates to cache_key_for_query
lib/support_table_cache/fiber_locals.rb Rewrites fiber-local storage to use Thread.current[] (fiber-local in MRI Ruby) keyed by object_id, eliminating the mutex+hash-by-fiber-id approach and preventing state leaks from suspended GC'd fibers
lib/support_table_cache/memory_cache.rb Adds mutex synchronization to reads/deletes/clears, fixes double serialization on cache miss, write now returns the serialized value for reuse in fetch
lib/support_table_cache/associations.rb Guards alias_method with a method_defined? check to prevent infinite recursion when cache_belongs_to is called multiple times for the same association
spec/support_table_cache_spec.rb Comprehensive new tests covering transactions, complex relation conditions, create_with exclusion, equivalent-value normalization, disabled-caching invalidation, and where-condition filtering in load_cache

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant FindByOverride
    participant RelationOverride
    participant SupportTableCache
    participant Cache

    Caller->>FindByOverride: Model.find_by(name: "One")
    FindByOverride->>FindByOverride: args Hash? scoped? in transaction?
    alt scoped (default scope / scoping block)
        FindByOverride->>RelationOverride: super → find_by
        RelationOverride->>RelationOverride: "support_table_cacheable_scope?<br/>(predicates == where_values_hash size,<br/>no joins/group/having/from/offset/lock)"
        RelationOverride->>SupportTableCache: open_transaction?(klass)
        alt inside joinable transaction
            SupportTableCache-->>RelationOverride: true → bypass cache
            RelationOverride->>Caller: DB result (not cached)
        else
            RelationOverride->>SupportTableCache: cache_key_for_query(klass, attrs)
            SupportTableCache-->>RelationOverride: cache key or nil
            RelationOverride->>Cache: "fetch(key) { DB query }"
            Cache-->>Caller: record
        end
    else no scope
        FindByOverride->>SupportTableCache: open_transaction?(self)
        alt inside joinable transaction
            SupportTableCache-->>FindByOverride: true → bypass cache
            FindByOverride->>Caller: DB result (not cached)
        else
            FindByOverride->>SupportTableCache: cache_key_for_query(klass, attrs)
            SupportTableCache-->>FindByOverride: cache key or nil
            FindByOverride->>Cache: "fetch(key) { DB query }"
            Cache-->>Caller: record
        end
    end

    Note over Cache: Invalidation always runs<br/>(support_table_cache_for_invalidation<br/>ignores disabled flag)
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant FindByOverride
    participant RelationOverride
    participant SupportTableCache
    participant Cache

    Caller->>FindByOverride: Model.find_by(name: "One")
    FindByOverride->>FindByOverride: args Hash? scoped? in transaction?
    alt scoped (default scope / scoping block)
        FindByOverride->>RelationOverride: super → find_by
        RelationOverride->>RelationOverride: "support_table_cacheable_scope?<br/>(predicates == where_values_hash size,<br/>no joins/group/having/from/offset/lock)"
        RelationOverride->>SupportTableCache: open_transaction?(klass)
        alt inside joinable transaction
            SupportTableCache-->>RelationOverride: true → bypass cache
            RelationOverride->>Caller: DB result (not cached)
        else
            RelationOverride->>SupportTableCache: cache_key_for_query(klass, attrs)
            SupportTableCache-->>RelationOverride: cache key or nil
            RelationOverride->>Cache: "fetch(key) { DB query }"
            Cache-->>Caller: record
        end
    else no scope
        FindByOverride->>SupportTableCache: open_transaction?(self)
        alt inside joinable transaction
            SupportTableCache-->>FindByOverride: true → bypass cache
            FindByOverride->>Caller: DB result (not cached)
        else
            FindByOverride->>SupportTableCache: cache_key_for_query(klass, attrs)
            SupportTableCache-->>FindByOverride: cache key or nil
            FindByOverride->>Cache: "fetch(key) { DB query }"
            Cache-->>Caller: record
        end
    end

    Note over Cache: Invalidation always runs<br/>(support_table_cache_for_invalidation<br/>ignores disabled flag)
Loading

Reviews (1): Last reviewed commit: "Bug fixes" | Re-trigger Greptile

# (SQL string conditions, ranges, OR clauses, joins, etc.) is not visible in
# where_values_hash and could silently change which record the query returns.
def support_table_cacheable_scope?
return false unless where_clause.send(:predicates).size == where_values_hash.size

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Private Rails API accessed via send

where_clause.send(:predicates) bypasses the private visibility of predicates on ActiveRecord::Relation::WhereClause. The method exists in Rails 6–8 but is not part of the documented public interface and could be renamed, moved, or removed in a future Rails release. Since this check is load-bearing for the core correctness fix (preventing wrong records from being cached), a breakage here would silently fall back to false (never caching), degrading performance rather than returning wrong data, but it is still a dependency worth tracking.

There is no clean public-API alternative today, but the dependency should be noted in a comment so a future upgrade does not catch the team off-guard.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +301 to +306
def open_transaction?(klass)
return false unless klass.connection_pool.active_connection?

connection = klass.connection
connection.transaction_open? && connection.current_transaction.joinable?
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Internal Rails API: connection.current_transaction

connection.current_transaction and current_transaction.joinable? are not part of ActiveRecord's public API; they live on the internal transaction stack and have changed signatures across major Rails versions. If Rails ever returns a null-transaction sentinel (instead of a real object) from current_transaction when transaction_open? is true—or renames the method—this would raise NoMethodError on every cached lookup during a transaction. Adding a guard (.respond_to?(:joinable?)) or a comment noting the dependency would help future maintainers.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +282 to +283
where_matched = where.nil? || where.all? { |name, value| attributes.include?(name) && attributes[name] == value }
next unless where_matched

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 where matching skips type casting

The where_matched check compares query attribute values directly with the where config values using ==, without casting them through the attribute type first. cache_key (called just below) does cast, so the two paths can disagree for equivalent but differently-typed values—e.g. find_by(active: 1) with cache_by :name, where: {active: true} would fail the where_matched test (1 == true is false) while cache_key would treat them as identical. The cache would be bypassed rather than returning wrong data, but users relying on type-coerced where conditions would silently miss the cache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant