Repository navigation
Fix clear cache in notifications maily digest - #11
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughTermCustomizer.loader is made thread-local (singleton getter/setter using Thread.current), engine notifications clear the loader after controller actions and jobs, I18nBackend translations guard against missing loader, and concurrency specs plus RuboCop exclusion for ChangesThread-local loader isolation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (3)
spec/lib/decidim/term_customizer/term_customizer_loader_spec.rb (2)
53-63: ⚡ Quick winReplace brittle sleep timing with proper synchronization.
Same issue as the first test:
sleep-based coordination is non-deterministic. Use aConcurrent::CyclicBarrierto ensure both threads set their loaders before callingbackend.translations.⏱️ Proposed fix using CyclicBarrier
+ barrier = Concurrent::CyclicBarrier.new(2) + t1 = Thread.new do Decidim::TermCustomizer.loader = loader_org1 - sleep 0.05 + barrier.wait result_thread1 = backend.translations end t2 = Thread.new do - sleep 0.02 Decidim::TermCustomizer.loader = loader_org2 + barrier.wait result_thread2 = backend.translations end🤖 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 `@spec/lib/decidim/term_customizer/term_customizer_loader_spec.rb` around lines 53 - 63, The test uses sleep-based timing around Thread.new to coordinate setting Decidim::TermCustomizer.loader and calling backend.translations, which is brittle; replace the sleeps with a Concurrent::CyclicBarrier (or other thread barrier) so both threads set their respective loaders (loader_org1 and loader_org2) and then wait on the barrier before invoking backend.translations, ensuring deterministic synchronization for result_thread1 and result_thread2; locate the Thread.new blocks and the uses of Decidim::TermCustomizer.loader and backend.translations to implement the barrier wait/leave calls.
20-30: ⚡ Quick winReplace brittle sleep timing with proper synchronization.
The test uses
sleepto coordinate threads, which is non-deterministic and can cause flaky tests under load. Use theConcurrent::CyclicBarrierpattern to ensure deterministic synchronization.⏱️ Proposed fix using CyclicBarrier
+ barrier = Concurrent::CyclicBarrier.new(2) + t1 = Thread.new do Decidim::TermCustomizer.loader = loader_org1 - sleep 0.05 + barrier.wait thread1_loader = Decidim::TermCustomizer.loader end t2 = Thread.new do - sleep 0.02 Decidim::TermCustomizer.loader = loader_org2 + barrier.wait thread2_loader = Decidim::TermCustomizer.loader end🤖 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 `@spec/lib/decidim/term_customizer/term_customizer_loader_spec.rb` around lines 20 - 30, Replace the brittle sleep-based coordination in the two Thread.new blocks with a Concurrent::CyclicBarrier to deterministically synchronize the threads: create a barrier (e.g. barrier = Concurrent::CyclicBarrier.new(2)) before spawning t1/t2, then in each thread call barrier.wait at the point where you currently use sleep to ensure both threads reach the same synchronization point before performing Decidim::TermCustomizer.loader = loader_org1 / loader_org2 and before reading thread1_loader / thread2_loader; ensure you reference the existing variables loader_org1 and loader_org2 and add require 'concurrent' if not already present..rubocop.yml (1)
13-13: ⚡ Quick winAvoid blanket exclusion of spec/lib from linting.
Excluding the entire
spec/libdirectory from RuboCop is overly broad. The new spec file contains issues (dead code at lines 14-18, long test methods) that linting would typically catch.Instead of excluding the directory, either:
- Fix the linting violations in the spec file, or
- Use inline
rubocop:disablecomments for specific unavoidable violations🔍 Alternative: selective disabling
If specific cops are problematic for this spec file, disable them inline:
# rubocop:disable RSpec/MultipleExpectations it "is isolated per thread" do # test code end # rubocop:enable RSpec/MultipleExpectationsOr remove the directory exclusion and fix violations as they arise.
🤖 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 @.rubocop.yml at line 13, Remove the blanket exclusion "spec/lib/**/*" from .rubocop.yml and either fix the lint issues in the new spec file (remove the dead code around lines 14-18 and break up long test methods) or, if specific cops must be suppressed, add targeted inline disables in that spec (e.g., use rubocop:disable for RSpec/MultipleExpectations or other offending cops around just the problematic examples) rather than excluding the entire directory.
🤖 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/decidim/term_customizer.rb`:
- Around line 40-46: The Thread.current thread-local accessor methods loader and
loader= on TermCustomizer can leak across pooled threads; ensure you clear the
loader at the end of each request by subscribing to the action controller
completion event (e.g. hook after start_processing) and setting
TermCustomizer.loader = nil in that subscriber (subscribe to
"process_action.action_controller"), and audit callers of TermCustomizer.loader
to handle nil defensively (background jobs, console, tests) so no code assumes a
non-nil loader.
In `@spec/lib/decidim/term_customizer/term_customizer_loader_spec.rb`:
- Around line 14-18: Remove the unused Concurrent::CyclicBarrier instantiation
and its surrounding begin/rescue block in the term_customizer_loader_spec.rb
spec; locate the code that creates Concurrent::CyclicBarrier.new(2) (the begin
... rescue StandardError ... end block) and delete those lines so the spec no
longer contains dead, unused synchronization setup—no replacement is needed
unless the spec actually requires synchronization, in which case add a clear,
used synchronization primitive in place of the removed block.
---
Nitpick comments:
In @.rubocop.yml:
- Line 13: Remove the blanket exclusion "spec/lib/**/*" from .rubocop.yml and
either fix the lint issues in the new spec file (remove the dead code around
lines 14-18 and break up long test methods) or, if specific cops must be
suppressed, add targeted inline disables in that spec (e.g., use rubocop:disable
for RSpec/MultipleExpectations or other offending cops around just the
problematic examples) rather than excluding the entire directory.
In `@spec/lib/decidim/term_customizer/term_customizer_loader_spec.rb`:
- Around line 53-63: The test uses sleep-based timing around Thread.new to
coordinate setting Decidim::TermCustomizer.loader and calling
backend.translations, which is brittle; replace the sleeps with a
Concurrent::CyclicBarrier (or other thread barrier) so both threads set their
respective loaders (loader_org1 and loader_org2) and then wait on the barrier
before invoking backend.translations, ensuring deterministic synchronization for
result_thread1 and result_thread2; locate the Thread.new blocks and the uses of
Decidim::TermCustomizer.loader and backend.translations to implement the barrier
wait/leave calls.
- Around line 20-30: Replace the brittle sleep-based coordination in the two
Thread.new blocks with a Concurrent::CyclicBarrier to deterministically
synchronize the threads: create a barrier (e.g. barrier =
Concurrent::CyclicBarrier.new(2)) before spawning t1/t2, then in each thread
call barrier.wait at the point where you currently use sleep to ensure both
threads reach the same synchronization point before performing
Decidim::TermCustomizer.loader = loader_org1 / loader_org2 and before reading
thread1_loader / thread2_loader; ensure you reference the existing variables
loader_org1 and loader_org2 and add require 'concurrent' if not already present.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d5334fc-37f9-41d7-b348-8f13d19ec0d7
📒 Files selected for processing (4)
.rubocop.ymllib/decidim/term_customizer.rblib/decidim/term_customizer/i18n_backend.rbspec/lib/decidim/term_customizer/term_customizer_loader_spec.rb
💤 Files with no reviewable changes (1)
- lib/decidim/term_customizer/i18n_backend.rb
| def loader | ||
| Thread.current.thread_variable_get(:term_customizer_loader) | ||
| end | ||
|
|
||
| def loader=(loader) | ||
| Thread.current.thread_variable_set(:term_customizer_loader, loader) | ||
| end |
There was a problem hiding this comment.
Critical: Thread pooling can leak loaders between requests.
In production web servers with thread pools (Puma, Passenger), threads are reused across requests. Thread-local variables persist for the thread's lifetime, so the loader from one request can leak into subsequent requests on the same thread if the engine hook doesn't fire or fails.
Add explicit cleanup at request completion to prevent cross-request contamination:
# In engine.rb after the start_processing hook
ActiveSupport::Notifications.subscribe "process_action.action_controller" do
TermCustomizer.loader = nil
endAdditionally, consider defensive nil handling in callers, as the loader may be nil in background jobs, console, or tests.
🤖 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/decidim/term_customizer.rb` around lines 40 - 46, The Thread.current
thread-local accessor methods loader and loader= on TermCustomizer can leak
across pooled threads; ensure you clear the loader at the end of each request by
subscribing to the action controller completion event (e.g. hook after
start_processing) and setting TermCustomizer.loader = nil in that subscriber
(subscribe to "process_action.action_controller"), and audit callers of
TermCustomizer.loader to handle nil defensively (background jobs, console,
tests) so no code assumes a non-nil loader.
There was a problem hiding this comment.
what about after an active_job, should we have some similar defensive action ('perform_start.active_job')
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the loader is set for jobs (job_context_class) and any existing hooks
rg -n "job_context" --type=rb -A3 -B3
rg -n "loader" --type=rb -A2 -B2
rg -n "active_job\|perform\|around_perform\|before_perform\|after_perform" --type=rb -A3 -B3Length of output: 581
🏁 Script executed:
#!/bin/bash
rg -n "job_context" -A3 -B3
rg -n "loader" -A2 -B2
rg -n "around_perform\|before_perform\|after_perform\|perform_start\|perform_end" -A3 -B3Length of output: 16025
@microstudi Yes, adding cleanup after job execution is a good defensive measure. A couple of points worth noting:
- The right event for cleanup is
perform.active_job(the finish event), notperform_start.active_job(which is already used to set the loader). The engine already does:
# engine.rb — sets loader at job start
ActiveSupport::Notifications.subscribe "perform_start.active_job" do |..., data|
context = TermCustomizer.job_context_class.new(data)
TermCustomizer.loader = Loader.new(resolver)
# ...
endThe cleanup counterpart would be:
# Clear loader after job completes so the thread-local doesn't linger
ActiveSupport::Notifications.subscribe "perform.active_job" do
TermCustomizer.loader = nil
end-
Severity is lower than for web requests — because
perform_start.active_jobalready overwrites the loader at the beginning of every job, a stale loader from a previous job will be replaced before any translations are looked up. The residual risk is more about code that runs outside a job on a pooled thread (e.g., a fiber or spawned thread inside a job, or the thread being used in an unexpected context after the job finishes). -
Controller cleanup symmetry — since the same pattern is recommended for
process_action.action_controller, doing the same forperform.active_jobkeeps the two execution contexts consistent and makes the thread-local lifecycle explicit and bounded.
So yes, it's worth adding the perform.active_job cleanup subscriber alongside the existing perform_start.active_job subscriber in engine.rb.
There was a problem hiding this comment.
Seem like a very unlikely case. Why is it different the case for active job? even with the web case, isn't the event "process_action.action_controller" going to be executed every new requests? independently of the thread processing it, so the loader is restarted?
There was a problem hiding this comment.
@microstudi You're right, and the two cases are symmetric. My original "critical" label was overstated.
Here's the accurate picture:
- Web requests: A
start_processing.action_controller(or similar) subscriber already sets a fresh loader at the beginning of every request. So when thread A picks up request N+1, the start hook fires first and overwrites whatever stale loader was left by request N — before any translation lookup can happen. No observable leak. - Jobs: Exactly the same —
perform_start.active_jobalready overwrites the loader at job start. A stale loader from a previous job on the same thread is replaced before any code in the new job sees it.
So the process_action.action_controller and perform.active_job cleanup hooks this PR adds are defensive hygiene, not a critical safety requirement. Their main benefits are:
- Memory: releasing the loader object (and its cached DB data) so it can be GC'd sooner rather than sitting on the thread until the next request starts.
- Edge cases: protecting code that might run on the thread outside a normal request/job lifecycle (e.g. a fiber spawned after job completion, a middleware that does translation lookups post-action, etc.).
The leakage scenario would only matter if a translation lookup happened on a thread between the old request's teardown and the new request's start hook — which doesn't occur in normal Puma/Sidekiq usage.
Apologies for flagging this as critical; it's a nice-to-have cleanup, not a required fix.
microstudi
left a comment
microstudi
left a comment
There was a problem hiding this comment.
It seems difficult to really asses that this is working fine without testing in production.
Meanwhile a couple of simple comments
| def loader | ||
| Thread.current.thread_variable_get(:term_customizer_loader) | ||
| end | ||
|
|
||
| def loader=(loader) | ||
| Thread.current.thread_variable_set(:term_customizer_loader, loader) | ||
| end |
There was a problem hiding this comment.
Seem like a very unlikely case. Why is it different the case for active job? even with the web case, isn't the event "process_action.action_controller" going to be executed every new requests? independently of the thread processing it, so the loader is restarted?
2 participants
🎩 What? Why?
Fix the bug where translations are not always applied after clear cache, especially in Decidiamo emails such as daily digest and newsletter, requiring the button to be clicked again.
📷 Screenshots
Summary by CodeRabbit
Bug Fixes
Tests
Chores