Skip to content

Comments

Ensure and enforce that configs are loaded before options are accesse…#8

Open
MitchLewis930 wants to merge 1 commit intopr_058_beforefrom
pr_058_after
Open

Ensure and enforce that configs are loaded before options are accesse…#8
MitchLewis930 wants to merge 1 commit intopr_058_beforefrom
pr_058_after

Conversation

@MitchLewis930
Copy link

@MitchLewis930 MitchLewis930 commented Jan 30, 2026

User description

PR_058


PR Type

Enhancement, Bug fix


Description

  • Enforce configuration loading and clamping before accessing options

  • Add NotLoadedError and NotClampedError exceptions for safety

  • Refactor Binder to accept options hash instead of Configuration object

  • Update hook processing to store metadata and defer warnings to clamp time

  • Replace immediate warnings with deferred cluster-only hook validation


Diagram Walkthrough

flowchart LR
  A["Configuration.new"] --> B["load called"]
  B --> C["clamp called"]
  C --> D["options accessible"]
  E["Binder init"] --> F["accepts options hash"]
  G["Hook registration"] --> H["store metadata"]
  H --> I["warn at clamp time"]
Loading

File Walkthrough

Relevant files
Enhancement
3 files
configuration.rb
Add enforcement for load and clamp lifecycle                         
+75/-33 
binder.rb
Accept options hash instead of Configuration                         
+6/-7     
dsl.rb
Refactor hook processing and defer warnings                           
+20/-43 
Bug fix
5 files
launcher.rb
Call clamp before accessing options                                           
+25/-26 
server.rb
Pass options hash to Binder initialization                             
+1/-1     
control_cli.rb
Call clamp instead of load for config                                       
+2/-1     
puma.rb
Update Launcher initialization and formatting                       
+2/-2     
puma_info.rb
Call clamp instead of load                                                             
+1/-1     
Formatting
1 files
cli.rb
Minor formatting adjustment in setup_options                         
+1/-1     
Tests
6 files
test_binder.rb
Update Binder initialization with clamped config                 
+9/-9     
test_cli.rb
Add clamp calls before accessing options                                 
+17/-5   
test_config.rb
Add clamp calls and test new error conditions                       
+130/-69
test_launcher.rb
Use configure method instead of direct options                     
+3/-1     
test_rack_handler.rb
Replace load calls with clamp                                                       
+18/-18 
test_web_concurrency_auto.rb
Add clamp call before accessing options                                   
+4/-1     

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Unhandled warning failures: The new warn_hooks path emits warnings via LogWriter.stdio.log without any guarding or
fallback, so unexpected logging failures could disrupt clamp and prevent configuration
finalization.

Referred Code
def warn_hooks
  return if options[:workers] > 0
  return if options[:silence_fork_callback_warning]

  @hooks.each do |key, method|
    options.all_of(key).each do |hook_options|
      next unless hook_options[:cluster_only]

      LogWriter.stdio.log(<<~MSG.tr("\n", " "))
        Warning: The code in the `#{method}` block will not execute
        in the current Puma configuration. The `#{method}` block only
        executes in Puma's cluster mode. To fix this, either remove the
        `#{method}` call or increase Puma's worker count above zero.
      MSG
    end
  end
end

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Unstructured warning logs: The new warn_hooks implementation logs multi-line warning text (not structured logging),
which may violate environments that require structured (e.g., JSON) logs for
auditing/monitoring.

Referred Code
def warn_hooks
  return if options[:workers] > 0
  return if options[:silence_fork_callback_warning]

  @hooks.each do |key, method|
    options.all_of(key).each do |hook_options|
      next unless hook_options[:cluster_only]

      LogWriter.stdio.log(<<~MSG.tr("\n", " "))
        Warning: The code in the `#{method}` block will not execute
        in the current Puma configuration. The `#{method}` block only
        executes in Puma's cluster mode. To fix this, either remove the
        `#{method}` call or increase Puma's worker count above zero.
      MSG
    end
  end

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Enforce configuration lifecycle with state

The PR introduces a state machine to the Configuration object, enforcing that
options are only accessed after being finalized via a clamp method. This
prevents inconsistent states by raising errors on premature access, making
configuration handling more robust and predictable.

Examples:

lib/puma/configuration.rb [205-209]
    def options
      raise NotClampedError, "ensure clamp is called before accessing options" unless @clamped

      @_options
    end
lib/puma/launcher.rb [54-55]
      @config.clamp
      @options = @config.options

Solution Walkthrough:

Before:

class Configuration
  attr_reader :options # Direct access

  def initialize(...)
    @options = UserFileDefaultOptions.new(...)
  end

  def load
    # ... loads files ...
    @options
  end
  # ... other methods access @options directly
end

# Usage in other files
conf = Puma::Configuration.new(...)
conf.load
# options can be accessed at any time, potentially before full initialization
puts conf.options[:port]

After:

class Configuration
  class NotClampedError < StandardError; end

  def initialize(...)
    @_options = UserFileDefaultOptions.new(...)
    @clamped = false
  end

  def options
    raise NotClampedError unless @clamped
    @_options
  end

  def clamp
    load unless @loaded
    @clamped = true
    options.finalize_values
    options
  end
end

# Usage in other files
conf = Puma::Configuration.new(...)
conf.clamp # Must be called before accessing options
puts conf.options[:port] # Safe access
Suggestion importance[1-10]: 9

__

Why: The suggestion accurately identifies the central and most impactful change in the PR, which is the introduction of a stateful lifecycle (load/clamp) to the Configuration object to improve robustness.

High
General
Add default options argument

Make the options parameter in Binder#initialize optional by providing a default
value to maintain backward compatibility.

lib/puma/binder.rb [21]

-def initialize(log_writer, options, env: ENV)
+def initialize(log_writer, options = {}, env: ENV)
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This is a critical suggestion for maintaining backward compatibility, as the PR changes the Binder#initialize signature without providing a default value for the new options parameter, which would break existing calls.

Medium
Preserve multiple hook registrations

In process_hook, store hook method names in an array under
@config.hooks[options_key] instead of overwriting the value, to support multiple
hooks of the same type.

lib/puma/dsl.rb [1466]

-@config.hooks[options_key] = method
+@config.hooks[options_key] ||= []
+@config.hooks[options_key] << method
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a good suggestion that fixes a bug where multiple hooks of the same type would overwrite each other in @config.hooks, preventing all of them from being correctly warned about in single-worker mode.

Medium
  • More

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants