Add SIGUSR1 trap to dump settings and help debug
#347
Conversation
Adds signal handling for SIGUSR1 that dumps all application settings (both declared and dynamic) to Rails.log with sensitive values masked. - Creates new initializer config/initializers/signal_handlers.rb - Traps SIGUSR1 signal in both web (Puma) and worker (Sidekiq) processes - Dumps all declared Setting fields and dynamic Setting keys - Masks sensitive values (API keys, tokens, secrets, passwords) showing only first 4 characters - Logs to Rails.logger with process information and field counts - Runs in separate thread to avoid signal handler restrictions
The previous implementation conflicted with Puma's use of SIGUSR1 for phased restarts, causing the web service to crash with SIGHUP-like behavior. Changes: - Move Puma-specific SIGUSR1 handler to config/puma.rb on_worker_boot - This sets up the handler in each Puma worker process after Puma's initialization, avoiding conflicts with the master process - Keep Sidekiq handler in initializer (no conflicts there) - Both web and worker services now properly dump settings on SIGUSR1 - Puma master can still use SIGUSR1 for other purposes if needed Usage: - Worker: kill -USR1 <sidekiq_pid> - Web: kill -USR1 <puma_worker_pid>
WalkthroughRegisters SIGUSR1 handlers for Sidekiq and Puma workers that spawn an asynchronous thread to collect declared Setting methods and Setting.dynamic_keys, mask sensitive values via regex heuristics, and write a structured dump (process info, declared/dynamic sections, counts, and error backtraces) to Rails.logger. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Proc as Worker Process (Puma / Sidekiq)
participant Trap as SIGUSR1 Trap (installed)
participant Th as Async Thread
participant S as Setting model
participant L as Rails.logger
Proc->>Trap: OS delivers SIGUSR1
Trap->>Th: spawn asynchronous dump thread
Th->>L: log header (process label, PID)
Th->>S: enumerate declared setting methods
S-->>Th: declared names/values
Th->>S: fetch dynamic_keys (with timeout)
S-->>Th: dynamic keys/values or timeout error
Th->>Th: mask values (SENSITIVE_PATTERNS)
Th->>L: log Declared Settings section
Th->>L: log Dynamic Settings section
Th->>L: log summary and any errors/backtrace
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30–40 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
|
This should help us debug, want to review @sokie ? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
config/puma.rb (1)
70-78: Consider restricting to public class-level setting accessors
declared_fieldscurrently comes fromSetting.singleton_class.instance_methods(false), which may include non-public methods and could raiseNoMethodErrorwhen used withpublic_send. To make this more robust, you could use public instance methods on the singleton class instead:- declared_fields = Setting.singleton_class.instance_methods(false) + declared_fields = Setting.singleton_class.public_instance_methods(false) .map(&:to_s)This keeps the intent (class-level, declared settings) but avoids accidentally including private/protected helpers.
Please confirm via console or a small script which methods are returned by
Setting.singleton_class.instance_methods(false)vspublic_instance_methods(false)to ensure no desired settings accessors are lost.Also applies to: 100-118
config/initializers/signal_handlers.rb (1)
19-27: DRY up settings dump logic shared withconfig/puma.rbThe logic that computes
declared_fields,dynamic_fields, and logs their (masked) values is essentially duplicated between this initializer andconfig/puma.rb. To reduce maintenance overhead and keep masking rules in sync, consider extracting a shared helper, for example:
- A module method, e.g.
SettingsDump.dump!(context_label), or- A class method on
Setting, e.g.Setting.dump_all(mask_sensitive: true, context: "Sidekiq worker").Each signal handler would then just call that helper with a context string, keeping the signal wiring local but centralizing the dump/masking behavior.
If you pursue this, please verify both contexts (Puma worker and Sidekiq worker) call the shared helper with appropriate labels and that tests or a manual SIGUSR1 check cover both paths.
Also applies to: 49-71
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/initializers/signal_handlers.rb(1 hunks)config/puma.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
config/puma.rb (1)
app/models/setting.rb (1)
dynamic_keys(107-109)
config/initializers/signal_handlers.rb (1)
app/models/setting.rb (1)
dynamic_keys(107-109)
🪛 GitHub Check: ci / lint
config/puma.rb
[failure] 89-89:
Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.
[failure] 89-89:
Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.
[failure] 83-83:
Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.
[failure] 83-83:
Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.
config/initializers/signal_handlers.rb
[failure] 38-38:
Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.
[failure] 38-38:
Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.
[failure] 32-32:
Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.
[failure] 32-32:
Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci / test
🔇 Additional comments (4)
config/puma.rb (2)
61-68: SIGUSR1 handler setup for Puma workers looks solidUsing
on_worker_bootplus a per-workerSignal.trap("USR1")that offloads work to a new thread is an appropriate pattern and keeps the signal handler non-blocking while providing clear log headers and a completion summary.Please trigger
SIGUSR1against a Puma worker in a non-production environment to confirm the dump runs to completion without blocking requests and that logs appear once per signal as expected.Also applies to: 120-128
83-89: Disregard the proposed spacing changes—the current code is already correctly formatted.The review comment suggests adding spaces inside array brackets, but this would violate RuboCop's default configuration. The Layout/SpaceInsideArrayLiteralBrackets default is
EnforcedStyle: no_space, and your.rubocop.ymlinherits fromrubocop-rails-omakasewithout overriding this rule. The current code in lines 83–89 (without spaces:[/key/i, ...]and[value.length - 4, 8]) is already compliant and requires no changes.Likely an incorrect or invalid review comment.
config/initializers/signal_handlers.rb (2)
7-18: Sidekiq-only SIGUSR1 handler wiring is appropriateGuarding on
defined?(Sidekiq) && Sidekiq.server?insideafter_initializecleanly limits this handler to Sidekiq worker processes, and the log line at the end makes it easy to confirm initialization.Please start a Sidekiq worker in a staging/dev environment, confirm the “Signal handlers initialized…” log appears once on boot, and then send
SIGUSR1to verify the settings dump completes and does not interfere with Sidekiq’s normal signal handling.Also applies to: 79-80
32-38: The review comment is incorrect—current code already complies with RuboCop defaults.RuboCop's
Layout/SpaceInsideArrayLiteralBracketsrule defaults toEnforcedStyle: no_space, meaning it enforces arrays without spaces. The current code at lines 32 and 38 follows this rule correctly:
- Line 32:
[/key/i, /token/i, ...]✓ compliant- Line 38:
[value.length - 4, 8]✓ compliantThe suggested changes—adding spaces to create
[ /key/i, ... ]and[ value.length - 4, 8 ]—would violate the default rule, not satisfy it. Since.rubocop.ymldoes not explicitly configure this rule, the defaultno_spacestyle applies.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
config/initializers/signal_handlers.rb (1)
11-77: Identical issues apply here as in puma.rb.This signal handler has the same concerns already flagged in the review of
config/puma.rb:
- Database query in signal handler (line 26)
- Masking logic doesn't handle empty strings for sensitive fields (line 35)
- No safeguard against rapid signal flooding (lines 11-12)
- Code duplication (the entire handler logic duplicates puma.rb lines 62-128)
Refer to the comments on
config/puma.rbfor details and suggested fixes.
🧹 Nitpick comments (3)
config/puma.rb (2)
71-74: Brittle exclusion list for declared fields.The hardcoded exclusion list
%w[[] []= key? delete dynamic_keys validate_onboarding_state! validate_openai_config!]is fragile. If new utility methods are added to the Setting model, they'll appear in the dump unless explicitly excluded. This could leak sensitive data or clutter the output.Consider documenting this behavior or using a more robust approach, such as marking methods with a custom attribute or checking for a naming convention.
62-63: No safeguard against rapid signal flooding.If multiple SIGUSR1 signals are sent in quick succession, each will spawn a new thread, potentially creating many concurrent threads that all query the database and write to the logger simultaneously. This could overwhelm resources or produce interleaved, unreadable log output.
Consider adding a simple guard (e.g., a flag or timestamp check) to rate-limit or serialize dump requests.
config/initializers/signal_handlers.rb (1)
79-79: Initialization log may clutter production logs.Line 79 logs at info level every time a Sidekiq worker starts. In production with frequent worker restarts or autoscaling, this could add noise to the logs. Consider logging at debug level or making it conditional based on environment.
- Rails.logger.info "Signal handlers initialized for Sidekiq worker (SIGUSR1 -> dump settings)" + Rails.logger.debug "Signal handlers initialized for Sidekiq worker (SIGUSR1 -> dump settings)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/initializers/signal_handlers.rb(1 hunks)config/puma.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
config/initializers/signal_handlers.rb (1)
app/models/setting.rb (1)
dynamic_keys(107-109)
config/puma.rb (1)
app/models/setting.rb (1)
dynamic_keys(107-109)
config/puma.rb
Outdated
| mask_value = lambda do |field_name, value| | ||
| return nil if value.nil? | ||
|
|
||
| sensitive = [ /key/i, /token/i, /secret/i, /password/i, /api/i, /credentials?/i, /auth/i ] | ||
| is_sensitive = sensitive.any? { |pattern| field_name.match?(pattern) } | ||
|
|
||
| if is_sensitive && value.present? | ||
| case value | ||
| when String | ||
| value.length <= 4 ? "[MASKED]" : "#{value[0..3]}#{'*' * [ value.length - 4, 8 ].min}" | ||
| when TrueClass, FalseClass | ||
| value | ||
| else | ||
| "[MASKED]" | ||
| end | ||
| else | ||
| value | ||
| end | ||
| end |
There was a problem hiding this comment.
Masking logic may not mask empty strings.
Line 86 checks if is_sensitive && value.present?. For sensitive fields with empty string values (""), .present? returns false, so the value would not be masked and would appear unmasked in the logs. While empty strings may not seem sensitive, it's better to consistently mask sensitive field names regardless of content.
Apply this diff to mask empty strings for sensitive fields:
- if is_sensitive && value.present?
+ if is_sensitive
case value
when String
- value.length <= 4 ? "[MASKED]" : "#{value[0..3]}#{'*' * [ value.length - 4, 8 ].min}"
+ value.empty? ? "[EMPTY]" : (value.length <= 4 ? "[MASKED]" : "#{value[0..3]}#{'*' * [ value.length - 4, 8 ].min}")
when TrueClass, FalseClass
value
else
"[MASKED]"
end
else
value
end📝 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.
| mask_value = lambda do |field_name, value| | |
| return nil if value.nil? | |
| sensitive = [ /key/i, /token/i, /secret/i, /password/i, /api/i, /credentials?/i, /auth/i ] | |
| is_sensitive = sensitive.any? { |pattern| field_name.match?(pattern) } | |
| if is_sensitive && value.present? | |
| case value | |
| when String | |
| value.length <= 4 ? "[MASKED]" : "#{value[0..3]}#{'*' * [ value.length - 4, 8 ].min}" | |
| when TrueClass, FalseClass | |
| value | |
| else | |
| "[MASKED]" | |
| end | |
| else | |
| value | |
| end | |
| end | |
| mask_value = lambda do |field_name, value| | |
| return nil if value.nil? | |
| sensitive = [ /key/i, /token/i, /secret/i, /password/i, /api/i, /credentials?/i, /auth/i ] | |
| is_sensitive = sensitive.any? { |pattern| field_name.match?(pattern) } | |
| if is_sensitive | |
| case value | |
| when String | |
| value.empty? ? "[EMPTY]" : (value.length <= 4 ? "[MASKED]" : "#{value[0..3]}#{'*' * [ value.length - 4, 8 ].min}") | |
| when TrueClass, FalseClass | |
| value | |
| else | |
| "[MASKED]" | |
| end | |
| else | |
| value | |
| end | |
| end |
🤖 Prompt for AI Agents
config/puma.rb around lines 80 to 98: the current condition `if is_sensitive &&
value.present?` skips masking for empty strings; change the condition to just
`if is_sensitive` (or `if is_sensitive && !value.nil?`) so sensitive field names
are masked regardless of empty-string values, keeping the existing branch
handling (Strings get masked, booleans returned as-is, others "[MASKED]") and
preserving the `return nil if value.nil?` behavior.
There was a problem hiding this comment.
Tell me, oh dear @CodeRabbit - how do you mask an empty string? Wouldn't it make things even more confusing by showing ****** when no value was even present?
There was a problem hiding this comment.
@jjmata You're absolutely right to question this! My suggestion shows the literal string "[EMPTY]" for empty strings, not asterisks. The idea was to explicitly distinguish between:
nil(field not set) → shows asnil""(field set but empty) → shows as"[EMPTY]""abc"(short sensitive value) → shows as"[MASKED]"
This way, when debugging, you can tell if a sensitive field like api_key was never configured (nil) versus misconfigured as an empty string ("").
However, you make a valid point - there's no actual sensitive data in an empty string to protect. If you prefer to keep the current behavior where empty strings just appear as empty in the logs (which is clearer and simpler), that's perfectly reasonable. The security concern is minimal since there's nothing to leak.
Your call on what makes the logs most readable for your debugging needs! 🐰
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/settings_log_dump.rb (1)
6-14: Consider more specific pattern for API-related fields.The
/api/ipattern may be overly broad, matching unintended fields like "capital" or "rapid". Consider using a more specific pattern like/\bapi\b/ior/api[_-]?(key|token|secret)/ito reduce false positives.Apply this diff if you want to be more precise:
/password/i, - /api/i, + /\bapi\b/i, /credentials?/i,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/initializers/sidekiq.rb(2 hunks)config/puma.rb(2 hunks)lib/settings_log_dump.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/puma.rb
🧰 Additional context used
🧬 Code graph analysis (2)
lib/settings_log_dump.rb (1)
app/models/setting.rb (1)
dynamic_keys(107-109)
config/initializers/sidekiq.rb (1)
lib/settings_log_dump.rb (1)
install_usr1_trap(18-28)
🪛 GitHub Check: ci / lint
lib/settings_log_dump.rb
[failure] 102-102:
Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.
[failure] 102-102:
Layout/SpaceInsideArrayLiteralBrackets: Use space inside array brackets.
🔇 Additional comments (4)
lib/settings_log_dump.rb (4)
21-25: Verify thread safety in signal handler context.Spawning a thread from within a signal handler is generally safer than executing work directly, but be aware that this introduces concurrency into Sidekiq's threading model. Ensure that:
- The
dump_settingsmethod is thread-safe- Database connections are available in the spawned thread
- No deadlocks can occur with Sidekiq's internal locks
Consider testing this in a staging environment under load to verify there are no threading issues.
30-65: Well-structured error handling and logging.The method includes comprehensive error handling that catches and logs all exceptions with backtrace, ensuring the signal handler won't crash the process. The structured output with headers, sections, and counts makes the dump easy to parse.
67-76: Clean reflection-based field discovery.The method properly filters out accessors, internal methods, and excluded methods to focus on actual setting fields. Using
instance_methods(false)correctly limits to methods defined on the Setting class.
78-87: Database query in signal handler context.The method queries the database to fetch dynamic keys, which could fail if the database is unavailable or under heavy load. The timeout mitigates hanging, but consider:
- Database connection pool availability in the spawned thread
- Impact on database load if multiple processes receive SIGUSR1 simultaneously
The graceful degradation (returning
[]on timeout) is good defensive coding.Verify this behaves correctly when the database is temporarily unavailable or the connection pool is exhausted.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/settings_log_dump.rb (1)
102-102: Fix spacing in array brackets.The linter flagged spacing inside the array literal brackets.
Apply this diff to fix the spacing:
- "#{value[0..3]}#{'*' * [ value.length - 4, 8 ].min}" + "#{value[0..3]}#{'*' * [value.length - 4, 8].min}"
🧹 Nitpick comments (2)
lib/settings_log_dump.rb (2)
21-25: Consider adding error handling for the spawned thread.The thread spawned in the signal handler lacks error handling. If
dump_settingsraises an exception, it may be silently swallowed, making debugging difficult.Apply this diff to add thread error handling:
Signal.trap("USR1") do Thread.new do - dump_settings(process_label: process_label, logger: logger) + begin + dump_settings(process_label: process_label, logger: logger) + rescue => e + logger ||= Rails.logger + logger.error "Error in USR1 signal handler thread: #{e.class} - #{e.message}" + logger.error e.backtrace.join("\n") + end end end
67-76: Consider adding timeout protection for declared settings.While
instance_methodsis typically fast, the subsequentpublic_sendcalls indump_settings(line 44) could be slow if any setting accessor performs database queries or external calls.You could wrap the entire declared settings section in
dump_settingswith a timeout, though this is likely unnecessary in most cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/settings_log_dump.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/settings_log_dump.rb (1)
app/models/setting.rb (1)
dynamic_keys(107-109)
🔇 Additional comments (2)
lib/settings_log_dump.rb (2)
1-16: LGTM! Module setup and constants are well-defined.The sensitive patterns cover common field names, and the timeout value is reasonable for a debug utility.
30-65: LGTM! Well-structured dump logic with appropriate error handling.The structured output format with clear sections and comprehensive error logging makes this easy to use for debugging.
Summary by CodeRabbit