Skip to content

Fix clear cache in notifications maily digest #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
microstudi merged 4 commits into from
Jun 8, 2026
Merged

Fix clear cache in notifications maily digest #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e6342b7
fix bug
ElviaBth Jun 4, 2026
1fe3163
add loader cleanup
ElviaBth Jun 4, 2026
202a01f
fix spec
ElviaBth Jun 4, 2026
2081935
fix spec
ElviaBth Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .rubocop.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,4 @@ RSpec/DescribeClass:
Exclude:
- spec/i18n_spec.rb
- "spec/system/**/*"
- "spec/lib/**/*"
8 changes: 7 additions & 1 deletion lib/decidim/term_customizer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ module TermCustomizer
end

class << self
attr_accessor :loader
def loader
Thread.current.thread_variable_get(:term_customizer_loader)
end

def loader=(loader)
Thread.current.thread_variable_set(:term_customizer_loader, loader)
end
Comment on lines +40 to +46

@coderabbitai coderabbitai Bot Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

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
end

Additionally, 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.

@microstudi microstudi Jun 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what about after an active_job, should we have some similar defensive action ('perform_start.active_job')

@coderabbitai coderabbitai Bot Jun 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 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 -B3

Length 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 -B3

Length of output: 16025

@microstudi Yes, adding cleanup after job execution is a good defensive measure. A couple of points worth noting:

  1. The right event for cleanup is perform.active_job (the finish event), not perform_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)
  # ...
end

The 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

  1. Severity is lower than for web requests — because perform_start.active_job already 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).

  2. Controller cleanup symmetry — since the same pattern is recommended for process_action.action_controller, doing the same for perform.active_job keeps 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.

@microstudi microstudi Jun 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@coderabbitai coderabbitai Bot Jun 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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_job already 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:

  1. 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.
  2. 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.

end
end
end
8 changes: 8 additions & 0 deletions lib/decidim/term_customizer/engine.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ class Engine < ::Rails::Engine
customizer_backend.reload!
end

ActiveSupport::Notifications.subscribe "process_action.action_controller" do
TermCustomizer.loader = nil
end

# The jobs are generally run in different context than the controllers
# which causes the term customizations not to be active. During the
# jobs, only the organization and global context translations are loaded
Expand Down Expand Up @@ -62,6 +66,10 @@ class Engine < ::Rails::Engine
# Force the backend to reload the translations for the job
customizer_backend.reload!
end

ActiveSupport::Notifications.subscribe "perform.active_job" do
TermCustomizer.loader = nil
end
end
end
end
Expand Down
1 change: 0 additions & 1 deletion lib/decidim/term_customizer/i18n_backend.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ def reload!
end

def translations
return @translations if @translations
return {} unless TermCustomizer.loader

@translations = TermCustomizer.loader.translations_hash
Expand Down
66 changes: 66 additions & 0 deletions spec/lib/decidim/term_customizer/term_customizer_loader_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# frozen_string_literal: true

require "spec_helper"

describe "Decidim::TermCustomizer.loader" do
describe ".loader" do
it "is isolated per thread" do
loader_org1 = double("loader_org1")
loader_org2 = double("loader_org2")

thread1_loader = nil
thread2_loader = nil

t1 = Thread.new do
Decidim::TermCustomizer.loader = loader_org1
sleep 0.05
thread1_loader = Decidim::TermCustomizer.loader
end

t2 = Thread.new do
sleep 0.02
Decidim::TermCustomizer.loader = loader_org2
thread2_loader = Decidim::TermCustomizer.loader
end

t1.join
t2.join

expect(thread1_loader).to eq(loader_org1)
expect(thread2_loader).to eq(loader_org2)
end
end

describe "Decidim::TermCustomizer::I18nBackend" do
let(:backend) { Decidim::TermCustomizer::I18nBackend.new }

let(:translations_org1) { { en: { decidim: { test_key: "Org 1 translation" } } } }
let(:translations_org2) { { en: { decidim: { test_key: "Org 2 translation" } } } }

it "does not bleed translations between concurrent threads" do
loader_org1 = double("loader_org1", translations_hash: translations_org1)
loader_org2 = double("loader_org2", translations_hash: translations_org2)

result_thread1 = nil
result_thread2 = nil

t1 = Thread.new do
Decidim::TermCustomizer.loader = loader_org1
sleep 0.05
result_thread1 = backend.translations
end

t2 = Thread.new do
sleep 0.02
Decidim::TermCustomizer.loader = loader_org2
result_thread2 = backend.translations
end

t1.join
t2.join

expect(result_thread1).to eq(translations_org1)
expect(result_thread2).to eq(translations_org2)
end
end
end
Loading