Bug fixes#9
Conversation
Greptile SummaryThis PR is a broad correctness fix for
Confidence Score: 4/5The 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
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)
%%{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)
Reviews (1): Last reviewed commit: "Bug fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # (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 |
There was a problem hiding this comment.
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!
| def open_transaction?(klass) | ||
| return false unless klass.connection_pool.active_connection? | ||
|
|
||
| connection = klass.connection | ||
| connection.transaction_open? && connection.current_transaction.joinable? | ||
| end |
There was a problem hiding this comment.
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!
| where_matched = where.nil? || where.all? { |name, value| attributes.include?(name) && attributes[name] == value } | ||
| next unless where_matched |
There was a problem hiding this comment.
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.
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughSupportTableCache now restricts cache lookups to safe query shapes, normalizes typed cache keys, preserves invalidation while disabled, filters cache loading, and improves fiber-local, memory-cache, and association behavior. Tests and documentation cover the updated release 1.1.6 semantics. ChangesCache correctness and concurrency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant RelationOverride
participant SupportTableCache
participant MemoryCache
participant Database
Caller->>RelationOverride: find_by query
RelationOverride->>SupportTableCache: validate query and build key
SupportTableCache->>MemoryCache: fetch cache key
MemoryCache-->>RelationOverride: cached record or miss
RelationOverride->>Database: execute query on miss
Database-->>RelationOverride: return record
RelationOverride->>MemoryCache: cache record
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/support_table_cache/memory_cache.rb`:
- Around line 26-39: Update the cache-miss fill flow around the lookup method
and its write call so a value fetched outside `@mutex` cannot be written after a
concurrent delete or clear. Coordinate fills with the existing invalidation
methods in SupportTableCache, either by holding the cache synchronization
through the fill or by tracking per-key generations and rejecting writes from
invalidated generations; preserve expiration and nil-value behavior.
In `@lib/support_table_cache/relation_override.rb`:
- Around line 89-102: Replace the internal where_clause.send(:predicates) call
in support_table_cacheable_scope? with the stable, supported where-clause
attribute API, preserving the existing comparison that rejects non-simple
conditions. Verify the chosen API across the project’s supported Rails version
range and add a focused compatibility spec if needed so future Rails changes
fail explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 89109ee4-a762-47d8-a513-6bc1ba814363
📒 Files selected for processing (13)
CHANGELOG.mdREADME.mdVERSIONlib/support_table_cache.rblib/support_table_cache/associations.rblib/support_table_cache/fiber_locals.rblib/support_table_cache/find_by_override.rblib/support_table_cache/memory_cache.rblib/support_table_cache/relation_override.rbspec/spec_helper.rbspec/support_table_cache/associations_spec.rbspec/support_table_cache/fiber_locals_spec.rbspec/support_table_cache_spec.rb
| # A relation can only be cached if all of its conditions are simple equality conditions | ||
| # on the model's own attributes that can be represented in a cache key. Anything else | ||
| # (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 | ||
| return false if joins_values.present? || left_outer_joins_values.present? | ||
| return false if group_values.present? || !having_clause.empty? | ||
| return false if !from_clause.empty? || offset_value.present? || lock_value | ||
| return false if eager_loading? | ||
|
|
||
| true | ||
| end | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift
Cacheability check depends on an internal, non-public Rails API.
where_clause.send(:predicates) reaches into ActiveRecord::Relation::WhereClause#predicates, which is protected and the class itself is documented as "for internal use only" (:nodoc:) across Rails versions. The heuristic is currently correct (verified against SQL-string/range/.not/create_with test cases), but since this isn't part of Rails' public API contract, a future Rails release could rename/restructure it, silently breaking support_table_cacheable_scope? for every relation-based cached lookup.
Consider anchoring on a more stable signal (e.g. where_clause.extract_attributes, which appears to be a public method returning the list of attributes referenced by representable predicates) or adding a CI matrix entry / explicit spec pinned to the supported Rails version range to catch breakage early.
Please confirm the target Rails version range and whether WhereClause#predicates/extract_attributes visibility is stable there.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/support_table_cache/relation_override.rb` around lines 89 - 102, Replace
the internal where_clause.send(:predicates) call in
support_table_cacheable_scope? with the stable, supported where-clause attribute
API, preserving the existing comparison that rejects non-simple conditions.
Verify the chosen API across the project’s supported Rails version range and add
a focused compatibility spec if needed so future Rails changes fail explicitly.
- MemoryCache#fetch no longer stores a value generated concurrently with a delete or clear, which could resurrect a stale record after invalidation. Fills are coordinated with invalidation via a generation counter. - Cast values through the attribute type when matching cache_by where clauses in cache_key_for_query and load_cache so equivalent values (e.g. 1 and "1") match the same way cache keys are built. - Guard the internal Rails APIs used by support_table_cacheable_scope? (WhereClause#predicates) and open_transaction? (current_transaction and joinable?) so a future Rails change degrades to bypassing the cache instead of raising, document the dependencies, and add specs pinning the APIs so an incompatible Rails upgrade fails explicitly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/support_table_cache/relation_override.rb (1)
32-36: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not let explicit
find_byargs overwrite scoped predicateswhere(name: "Two").find_by(name: "One")must still behave likename = "Two" AND name = "One"and returnnil; merging here replaces the scoped value and can return a cached record the relation excludes. Bypass the cache when relation andfind_bykeys overlap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/support_table_cache/relation_override.rb` around lines 32 - 36, Update the relation override logic around scope_conditions and attributes so overlapping keys between scoped predicates and explicit find_by arguments bypass the cache instead of merging values. Preserve normal cache lookup when the key sets do not overlap, while ensuring overlapping conditions are evaluated by the database with both predicates so scoped exclusions cannot return a cached record.lib/support_table_cache.rb (1)
127-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
fetch_byreject queries that miss configuredwhereclauses
RelationOverride#fetch_bystill checks only attribute names, soTypedWhereModel.fetch_by(name: "typed-one")is accepted even though no cache entry can satisfywhere: {value: 1}and it falls back to the database. Include the configuredwherescope in the safety check sofetch_byraises when the query cannot hit the cache.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/support_table_cache.rb` around lines 127 - 130, The cache safety check used by RelationOverride#fetch_by must validate configured where clauses in addition to attribute names. Update the matching logic to compare the query’s where scope against each entry’s configured where value, preserving matches only when both attributes and where conditions are satisfied; otherwise raise instead of falling back to the database.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/support_table_cache/memory_cache.rb`:
- Around line 44-51: Update the cache write logic in the method containing
serialized_value and expire_at so replacing an expired entry without expires_in
clears the prior expiration timestamp. Ensure the stored entry uses nil or a
newly computed expiry for the replacement, rather than retaining the old
expire_at value.
---
Outside diff comments:
In `@lib/support_table_cache.rb`:
- Around line 127-130: The cache safety check used by RelationOverride#fetch_by
must validate configured where clauses in addition to attribute names. Update
the matching logic to compare the query’s where scope against each entry’s
configured where value, preserving matches only when both attributes and where
conditions are satisfied; otherwise raise instead of falling back to the
database.
In `@lib/support_table_cache/relation_override.rb`:
- Around line 32-36: Update the relation override logic around scope_conditions
and attributes so overlapping keys between scoped predicates and explicit
find_by arguments bypass the cache instead of merging values. Preserve normal
cache lookup when the key sets do not overlap, while ensuring overlapping
conditions are evaluated by the database with both predicates so scoped
exclusions cannot return a cached record.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7abfd83a-6e41-4765-a94d-8eec46f3b864
📒 Files selected for processing (7)
CHANGELOG.mdlib/support_table_cache.rblib/support_table_cache/memory_cache.rblib/support_table_cache/relation_override.rbspec/spec_helper.rbspec/support_table_cache/memory_cache_spec.rbspec/support_table_cache_spec.rb
| serialized_value = Marshal.dump(value) | ||
| expire_at = Process.clock_gettime(Process::CLOCK_MONOTONIC) + expires_in if expires_in | ||
|
|
||
| @mutex.synchronize do | ||
| # Only store the value if the cache was not invalidated while the value was being | ||
| # generated. Otherwise a record deleted by a concurrent invalidation could be | ||
| # resurrected in the cache with stale data. | ||
| @cache[key] = [serialized_value, expire_at] if @generation == generation |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset the expiry when replacing an expired entry.
If an expired entry is refetched without expires_in, expire_at still holds its old timestamp from Line 33. The replacement is therefore immediately expired and every later read becomes a miss.
Proposed fix
serialized_value = Marshal.dump(value)
- expire_at = Process.clock_gettime(Process::CLOCK_MONOTONIC) + expires_in if expires_in
+ expire_at = if expires_in
+ Process.clock_gettime(Process::CLOCK_MONOTONIC) + expires_in
+ end
`@mutex.synchronize` do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| serialized_value = Marshal.dump(value) | |
| expire_at = Process.clock_gettime(Process::CLOCK_MONOTONIC) + expires_in if expires_in | |
| @mutex.synchronize do | |
| # Only store the value if the cache was not invalidated while the value was being | |
| # generated. Otherwise a record deleted by a concurrent invalidation could be | |
| # resurrected in the cache with stale data. | |
| @cache[key] = [serialized_value, expire_at] if @generation == generation | |
| serialized_value = Marshal.dump(value) | |
| expire_at = if expires_in | |
| Process.clock_gettime(Process::CLOCK_MONOTONIC) + expires_in | |
| end | |
| `@mutex.synchronize` do | |
| # Only store the value if the cache was not invalidated while the value was being | |
| # generated. Otherwise a record deleted by a concurrent invalidation could be | |
| # resurrected in the cache with stale data. | |
| `@cache`[key] = [serialized_value, expire_at] if `@generation` == generation |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/support_table_cache/memory_cache.rb` around lines 44 - 51, Update the
cache write logic in the method containing serialized_value and expire_at so
replacing an expired entry without expires_in clears the prior expiration
timestamp. Ensure the stored entry uses nil or a newly computed expiry for the
replacement, rather than retaining the old expire_at value.
1 participant
Fixed
not,or, joins,group,having,from,offset, locking, or non-hashfind_byarguments) now bypass the cache instead of silently ignoring those conditions and returning or caching the wrong record.create_withvalues are no longer included in cache keys, so queries usingcreate_withcan now be cached correctly.joinable: false(such as Rails transactional test fixtures) still use the cache.disableblock (or while caching was disabled globally) left stale entries behind for when caching was re-enabled.:oneand"one", or"5"and5) 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 forfalseattribute values, which were previously indistinguishable fromnil.load_cacheno longer raises an error when caching is disabled and now honorswhereconditions oncache_byconfigurations instead of caching records that do not match them. It also refreshes existing cache entries instead of skipping them.cache_byconfiguration whosewhereclause does not match a query no longer prevents later configurations from matching, and no longer mutates the query attributes while matching.cache_byin a subclass no longer mutates the superclass's cache configuration.SupportTableCachewithout callingcache_byno longer raise an error onfind_by.cache_belongs_tomore than once for the same association no longer causes infinite recursion when reading the association.SupportTableCache::MemoryCachenow 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::FiberLocalsnow 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.Summary by CodeRabbit
whereconstraints, and ignoringcreate_withwhen building keys.cache_byinheritance/matching,find_byedge cases, repeated cache associations, and fixed race safety in memory cache and fiber-local state isolation.