Ensure and enforce that configs are loaded before options are accesse…#8
Ensure and enforce that configs are loaded before options are accesse…#8MitchLewis930 wants to merge 1 commit intopr_058_beforefrom
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new configuration lifecycle in Puma::Configuration with separate Changes
Sequence DiagramsequenceDiagram
participant App
participant Launcher
participant Configuration
participant Binder
participant Server
App->>Launcher: new(conf, launcher_args)
activate Launcher
Launcher->>Configuration: `@config` = conf
Launcher->>Configuration: `@options` = `@config.options` (after clamp)
Note over Launcher: `@options`[:log_writer] = log_writer<br/>@options[:logger] = log_writer (if clustered)
Launcher->>Server: new with `@config`
activate Server
Server->>Binder: new(log_writer, `@options`)
activate Binder
Note over Binder: Initialize with options hash<br/>Configure SSL, bindings from `@options`
Binder-->>Server: ready
deactivate Binder
Server-->>Launcher: ready
deactivate Server
Launcher-->>App: ready
deactivate Launcher
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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: 1
🤖 Fix all issues with AI agents
In `@lib/puma/configuration.rb`:
- Around line 219-223: When duplicating Configuration in initialize_copy(other)
the state flags `@loaded` and `@clamped` are not copied, so set them on the new
object to match the source; update initialize_copy to assign `@loaded` =
other.instance_variable_get(:`@loaded`) and `@clamped` =
other.instance_variable_get(:`@clamped`) (keep the existing `@_options` duplication
and niling of `@conf/`@cli_options) so the duplicated Configuration preserves the
loaded/clamped state and avoids NotClampedError when calling options.
🧹 Nitpick comments (2)
lib/puma/binder.rb (1)
21-37: Guard against nil values in options-derived thread/worker checks.
options[:max_threads]oroptions[:workers]being nil will raise on comparison now that Binder accepts a raw options hash. Consider coercing to integer defaults to keep this safe for external callers.Proposed fix
- `@proto_env` = { + max_threads = options[:max_threads].to_i + workers = options[:workers].to_i + + `@proto_env` = { "rack.version".freeze => RACK_VERSION, "rack.errors".freeze => log_writer.stderr, - "rack.multithread".freeze => options[:max_threads] > 1, - "rack.multiprocess".freeze => options[:workers] >= 1, + "rack.multithread".freeze => max_threads > 1, + "rack.multiprocess".freeze => workers >= 1,lib/puma/launcher.rb (1)
194-194: Consider avoiding the second clamp to prevent double finalize/warn hooks.
If config is already clamped during initialize and not mutated later, this second clamp is redundant and may re-runfinalize_values/warn_hooks.Possible simplification
- `@config.clamp` - `@config.plugins.fire_starts` self
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
benchmarks/local/puma_info.rblib/puma/binder.rblib/puma/cli.rblib/puma/configuration.rblib/puma/control_cli.rblib/puma/dsl.rblib/puma/launcher.rblib/puma/server.rblib/rack/handler/puma.rbtest/test_binder.rbtest/test_cli.rbtest/test_config.rbtest/test_launcher.rbtest/test_rack_handler.rbtest/test_web_concurrency_auto.rb
🧰 Additional context used
🧬 Code graph analysis (11)
benchmarks/local/puma_info.rb (1)
lib/puma/configuration.rb (1)
clamp(283-289)
test/test_rack_handler.rb (1)
lib/puma/configuration.rb (2)
clamp(283-289)options(205-209)
lib/puma/launcher.rb (5)
lib/puma/configuration.rb (3)
clamp(283-289)options(205-209)environment(330-332)lib/puma/binder.rb (4)
env(64-66)create_inherited_fds(79-84)create_activated_fds(92-108)synthesize_binds_from_activated_fs(120-145)lib/puma/util.rb (5)
new(67-132)new(68-70)each(78-82)k(91-93)k(95-100)lib/puma/null_io.rb (1)
each(16-17)lib/puma/dsl.rb (2)
environment(403-405)custom_logger(498-500)
lib/puma/control_cli.rb (1)
lib/puma/configuration.rb (1)
clamp(283-289)
test/test_cli.rb (3)
lib/puma/configuration.rb (3)
clamp(283-289)options(205-209)environment(330-332)lib/puma/dsl.rb (1)
environment(403-405)lib/puma/launcher.rb (1)
environment(362-364)
test/test_web_concurrency_auto.rb (1)
lib/puma/configuration.rb (1)
clamp(283-289)
test/test_launcher.rb (2)
lib/puma/configuration.rb (1)
configure(211-217)lib/puma/dsl.rb (1)
bind(286-289)
test/test_binder.rb (1)
lib/puma/configuration.rb (2)
clamp(283-289)options(205-209)
lib/puma/dsl.rb (2)
lib/puma/configuration.rb (2)
key(47-49)key(51-53)lib/puma/events.rb (2)
after_booted(33-35)after_stopped(41-43)
test/test_config.rb (1)
lib/puma/configuration.rb (5)
clamp(283-289)options(205-209)config_files(264-277)load(258-262)environment(330-332)
lib/puma/binder.rb (1)
lib/puma/launcher.rb (1)
initialize(44-110)
🪛 ast-grep (0.40.5)
test/test_config.rb
[warning] 668-668: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: conf.run_hooks(hook_name, 'ARG', Puma::LogWriter.strings)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 686-686: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: conf.run_hooks(hook_name, 'ARG', Puma::LogWriter.strings)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🔇 Additional comments (32)
lib/puma/cli.rb (1)
96-96: No functional change here.test/test_launcher.rb (1)
139-142: Switching to DSL-based bind setup looks good.
This aligns with the new configuration lifecycle and keeps test setup consistent.benchmarks/local/puma_info.rb (1)
92-96: Clamp usage is consistent with the new config lifecycle.test/test_web_concurrency_auto.rb (1)
63-67: Test now asserts failure at clamp time—consistent with new lifecycle.lib/puma/control_cli.rb (1)
130-135: Clamp-before-options aligns with the new configuration contract.lib/puma/server.rb (1)
117-117: Binder now receives options as expected.lib/rack/handler/puma.rb (2)
35-35: No functional change here.
75-75: No functional change here.lib/puma/binder.rb (1)
248-248: LGTM: store-backed PEM now sourced from options.test/test_cli.rb (1)
335-342: LGTM: tests now clamp before reading options/environment.Also applies to: 354-357, 369-373, 382-386
test/test_rack_handler.rb (1)
109-113: LGTM: clamp-before-access applied consistently across Rack handler tests.Also applies to: 130-133, 136-142, 144-150, 152-158, 160-166, 168-174, 193-197, 210-216, 231-235, 246-250, 260-264, 272-276, 280-285, 305-309, 319-323, 333-337
test/test_binder.rb (2)
15-20: LGTM: binder setup now uses clamped options.
428-452: LGTM: rack multithread/multiprocess assertions now rely on clamped options.test/test_config.rb (3)
12-619: LGTM: clamp is consistently applied before options access in config tests.
620-652: Nice coverage for NotClampedError/NotLoadedError behavior.
663-880: LGTM: helper and env-driven tests now clamp before access.lib/puma/launcher.rb (2)
25-33: Docs update looks good.
54-88: LGTM: options derived from a clamped config and threaded through binder/logging.lib/puma/dsl.rb (4)
13-30: LGTM: examples updated to reflect clamp usage.
735-737: LGTM: cluster-only hooks now explicitly flagged.Also applies to: 756-757, 781-782, 803-804, 820-822, 884-885
833-856: LGTM: event hooks now route through@config.events.
1463-1478: No action needed. Hook consumers already handle the new structure correctly. Therun_hooksmethod inconfiguration.rbproperly unpackshook_options[:block], checkshook_options[:id], and validateshook_options[:cluster_only]before and during hook execution.Likely an incorrect or invalid review comment.
lib/puma/configuration.rb (10)
129-131: Well-defined custom error types.The new
NotLoadedErrorandNotClampedErrorclasses provide clear semantic meaning for state violations. Good practice to define them inside theConfigurationclass namespace.
178-201: Clean state initialization with proper defaults.The initialization properly sets up the two-phase loading system with
@loadedand@clampedflags. The@_optionsnaming convention clearly signals internal use. The dependency injection pattern for@events(accepting from options or creating new) is good practice.
205-209: Core enforcement mechanism for configuration lifecycle.This accessor is the key to ensuring
clampis called before options are accessed. The guard raises a clear, actionable error message. This pattern provides fail-fast behavior that will catch misuse during development.
258-277: Load and config_files lifecycle correctly implemented.The
@loaded = trueis set before callingconfig_fileson line 260, which allows theconfig_filesmethod to pass its own@loadedcheck. The use of@_optionsdirectly (instead ofoptions) correctly bypasses the clamp check during the loading phase.
283-289: Clamp method properly orchestrates the two-phase loading.The
clampmethod correctly ensuresloadis called first, then sets@clamped = truebefore accessingoptions. This guarantees the lifecycle invariant. The method returnsoptionswhich enables useful chaining likeconfig.clamp[:some_key].
306-332: Accessor methods consistently enforce clamped state.All public accessors (
app_configured?,rackup,app,environment) now use the publicoptionsmethod, ensuring configuration is properly clamped before these values are accessed. This provides a uniform enforcement of the lifecycle.
341-358: Enhanced hook processing with structured options.The
run_hooksmethod now expects hooks to be stored as{ block:, id:, cluster_only: }hashes instead of bare blocks. The optional:idenables per-hook data storage viahook_data[id], which is useful for stateful hooks. This structured approach integrates well with the newcluster_onlywarning system.
371-375: Clean random token generation utility.The
random_tokenclass method usesSecureRandom.hex(16)producing 128 bits of cryptographic randomness, appropriate for security tokens. The conditional require pattern avoids redundant loads.
411-427: Rackup options merged after clamp.The
load_rackupmethod mutatesoptions.file_optionsafterclamphas been called. This appears intentional since rackup files are loaded lazily whenappis first accessed, which happens after configuration is clamped. The design allows rackup files to contribute their options late in the lifecycle.
429-445: Helpful warning system for cluster-only hooks.The
warn_hooksmethod proactively warns users when they configure cluster-only hooks (likebefore_fork) in single-worker mode where those hooks won't execute. The warning message is clear and actionable, explaining both the problem and the fix. The silencing option via:silence_fork_callback_warningprovides an escape hatch.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def initialize_copy(other) | ||
| @conf = nil | ||
| @cli_options = nil | ||
| @options = @options.dup | ||
| @_options = @_options.dup | ||
| end |
There was a problem hiding this comment.
State flags not copied in initialize_copy.
The @loaded and @clamped state flags are not copied when duplicating a Configuration object. If a clamped configuration is duplicated, the copy will have @clamped = false (from original initialization), causing NotClampedError when accessing options on the duplicate.
🔧 Proposed fix to copy state flags
def initialize_copy(other)
`@conf` = nil
`@cli_options` = nil
`@_options` = `@_options.dup`
+ `@loaded` = other.instance_variable_get(:`@loaded`)
+ `@clamped` = other.instance_variable_get(:`@clamped`)
end🤖 Prompt for AI Agents
In `@lib/puma/configuration.rb` around lines 219 - 223, When duplicating
Configuration in initialize_copy(other) the state flags `@loaded` and `@clamped` are
not copied, so set them on the new object to match the source; update
initialize_copy to assign `@loaded` = other.instance_variable_get(:`@loaded`) and
`@clamped` = other.instance_variable_get(:`@clamped`) (keep the existing `@_options`
duplication and niling of `@conf/`@cli_options) so the duplicated Configuration
preserves the loaded/clamped state and avoids NotClampedError when calling
options.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/puma/configuration.rb`:
- Around line 258-262: The load method currently sets `@loaded` = true before
actually processing config_files, so if `@_file_dsl._load_from` raises the `@loaded`
flag stays true; change the sequencing so `@loaded` is only set after all files
are successfully loaded (move the assignment to after the config_files.each
loop) and wrap the load iteration in a rescue to re-raise after ensuring `@loaded`
remains false on error; apply the same change to the other analogous block
covering lines 264-276 that uses `@file_dsl._load_from` and `@_options` so both code
paths only mark loaded after successful completion.
- Around line 279-289: clamp currently sets `@clamped` before finalizing defaults
which can leave the object marked clamped if options.finalize_values raises; fix
by deferring setting `@clamped` until after options.finalize_values (and
warn_hooks) succeed: call load unless `@loaded`, run options.finalize_values and
warn_hooks, then set `@clamped` = true and return options so that `@clamped` is only
true on successful finalization (refer to the clamp method and
options.finalize_values/warn_hooks symbols).
🧹 Nitpick comments (1)
lib/puma/configuration.rb (1)
219-223: Duplicate hooks on copy to avoid shared mutable state.
Cloned configurations currently share@hooks, which can lead to cross-instance mutations.♻️ Suggested diff
def initialize_copy(other) `@conf` = nil `@cli_options` = nil `@_options` = `@_options.dup` + `@hooks` = `@hooks.dup` end
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
benchmarks/local/puma_info.rblib/puma/binder.rblib/puma/cli.rblib/puma/configuration.rblib/puma/control_cli.rblib/puma/dsl.rblib/puma/launcher.rblib/puma/server.rblib/rack/handler/puma.rbtest/test_binder.rbtest/test_cli.rbtest/test_config.rbtest/test_launcher.rbtest/test_rack_handler.rbtest/test_web_concurrency_auto.rb
🧰 Additional context used
🧬 Code graph analysis (12)
benchmarks/local/puma_info.rb (2)
lib/rack/handler/puma.rb (1)
config(14-68)lib/puma/configuration.rb (1)
clamp(283-289)
test/test_web_concurrency_auto.rb (1)
lib/puma/configuration.rb (1)
clamp(283-289)
lib/puma/control_cli.rb (2)
lib/rack/handler/puma.rb (1)
config(14-68)lib/puma/configuration.rb (1)
clamp(283-289)
lib/rack/handler/puma.rb (2)
lib/puma/util.rb (3)
new(67-132)new(68-70)merge(122-125)lib/puma/configuration.rb (1)
options(205-209)
test/test_launcher.rb (2)
lib/puma/configuration.rb (1)
configure(211-217)lib/puma/dsl.rb (1)
bind(286-289)
test/test_binder.rb (1)
lib/puma/configuration.rb (2)
clamp(283-289)options(205-209)
test/test_rack_handler.rb (2)
lib/puma/configuration.rb (2)
clamp(283-289)options(205-209)lib/rack/handler/puma.rb (1)
config(14-68)
lib/puma/binder.rb (2)
lib/puma/launcher.rb (1)
initialize(44-110)lib/puma/server.rb (1)
initialize(70-128)
test/test_config.rb (1)
lib/puma/configuration.rb (7)
clamp(283-289)options(205-209)config_files(264-277)final_options(87-91)final_options(360-362)load(258-262)environment(330-332)
test/test_cli.rb (3)
lib/puma/configuration.rb (3)
clamp(283-289)options(205-209)environment(330-332)lib/puma/dsl.rb (1)
environment(403-405)lib/puma/launcher.rb (1)
environment(362-364)
lib/puma/dsl.rb (1)
lib/puma/events.rb (2)
after_booted(33-35)after_stopped(41-43)
lib/puma/configuration.rb (2)
lib/puma/dsl.rb (2)
_load_from(133-140)load(251-254)lib/puma/log_writer.rb (6)
call(12-146)call(14-18)call(15-17)call(20-24)call(21-23)log(62-68)
🪛 ast-grep (0.40.5)
test/test_config.rb
[warning] 668-668: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: conf.run_hooks(hook_name, 'ARG', Puma::LogWriter.strings)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
[warning] 686-686: Found the use of an hardcoded passphrase for RSA. The passphrase can be easily discovered, and therefore should not be stored in source-code. It is recommended to remove the passphrase from source-code, and use system environment variables or a restricted configuration file.
Context: conf.run_hooks(hook_name, 'ARG', Puma::LogWriter.strings)
Note: [CWE-798]: Use of Hard-coded Credentials [OWASP A07:2021]: Identification and Authentication Failures [REFERENCES]
https://cwe.mitre.org/data/definitions/522.html
(hardcoded-secret-rsa-passphrase-ruby)
🔇 Additional comments (36)
lib/puma/cli.rb (1)
96-96: LGTM!Minor formatting change adding spaces inside the hash literal. This is a cosmetic change with no functional impact.
lib/puma/control_cli.rb (1)
131-136: LGTM!Correctly migrated from
config.loadtoconfig.clamp. Theclampmethod (as shown inlib/puma/configuration.rb:282-288) internally callsloadif not already loaded, then marks the configuration as clamped and finalizes values. This ensures the configuration lifecycle is properly enforced before accessing options.benchmarks/local/puma_info.rb (1)
93-97: LGTM!Correctly migrated to use
config.clampbefore accessing options, consistent with the new configuration lifecycle pattern established across the codebase.lib/rack/handler/puma.rb (2)
35-35: LGTM!Minor formatting change adding spaces inside the hash literal for consistency.
75-75: LGTM!Updated to use modern Ruby keyword argument syntax (
log_writer: log_writer) instead of hash rocket style. This is a style improvement with no functional impact.lib/puma/dsl.rb (4)
16-16: LGTM!Documentation correctly updated to reflect the new
config.clampAPI instead ofconfig.load.Also applies to: 28-28
736-736: LGTM!Hook registration methods now correctly pass
cluster_only: truefor cluster-mode-only hooks (before_fork,before_worker_boot,before_worker_shutdown,before_worker_fork,after_worker_fork,before_refork). This enables proper validation/warning when these hooks are used in single-worker mode.Also applies to: 756-756, 781-781, 803-803, 821-821, 884-884
838-838: LGTM!Changed from
@config.options[:events]to@config.events. This correctly uses a direct accessor instead of going through the options hash, avoiding potentialNotClampedErrorissues during DSL evaluation (before clamp is called).Also applies to: 855-855
1463-1478: LGTM!The
process_hookmethod has been refactored well:
- Added
cluster_only:keyword parameter for flagging cluster-only hooks- Line 1466 stores the method name in
@config.hooksfor hook tracking- Hook options now use a structured hash with
:block,:cluster_only, and conditional:idkeys- Error message correctly references the
methodparameter nameThe restructured approach provides better metadata for hook validation and warning purposes.
test/test_rack_handler.rb (1)
110-110: LGTM!All test cases correctly migrated from
conf.loadtoconf.clamp. This ensures tests properly follow the new configuration lifecycle whereclampmust be called before accessing options.Also applies to: 131-131, 139-139, 147-147, 155-155, 163-163, 171-171, 194-194, 213-213, 232-232, 247-247, 261-261, 273-273, 282-282, 298-298, 307-307, 320-320, 334-334
lib/puma/server.rb (1)
117-117: LGTM!The Binder constructor call now passes
@optionsas the second argument, aligning with the updated Binder initialization signature that accepts an options hash. This enables Binder to access configuration values directly (for proto_env, SSL parameters, etc.) rather than through a Configuration object.test/test_web_concurrency_auto.rb (1)
64-67: LGTM!The test correctly adapts to the new clamp workflow. Since
clampinternally callsloadwhen not already loaded, theLoadErrorwill be raised duringconf.clampwhen the concurrent-ruby file is unavailable. This properly validates that the configuration lifecycle enforces loading before options become accessible.test/test_binder.rb (2)
15-19: LGTM!The setup correctly follows the new configuration lifecycle: instantiate
Configuration, callclampto finalize options, then pass@config.optionstoBinder.new. This aligns with the updatedBinder.initialize(log_writer, options, env:)signature.
433-438: LGTM!The test correctly creates a
Configurationwith custom options, callsclampto finalize, and passes the options hash toBinder.new. This pattern is consistent throughout the file.test/test_cli.rb (2)
337-343: LGTM!The test correctly retrieves the internal
@conf, callsclampto finalize configuration, and then accessesconf.options[:extra_runtime_dependencies]. This follows the new configuration lifecycle whereclampmust be called before accessing options.
354-357: LGTM!The environment tests correctly call
clampbefore accessingconf.environment, which internally reads fromoptions[:environment](perlib/puma/configuration.rblines 329-331).test/test_config.rb (3)
15-17: LGTM!The test correctly demonstrates the new configuration lifecycle: instantiate, clamp, then access options.
620-652: LGTM! Good test coverage for the new error states.These tests comprehensively validate the new configuration lifecycle enforcement:
test_options_raises_not_clamped_error_when_not_clampedensuresNotClampedErroris raised when accessing options before clampingtest_options_succeeds_when_clampedconfirms options are accessible after clamptest_config_files_raises_not_loaded_error_when_not_loadedensuresNotLoadedErroris raised for config_files access before loadingtest_config_files_succeeds_when_clampedconfirms config_files are accessible after clamp (since clamp internally loads)
668-688: Static analysis false positive - ignore.The static analysis flagged
'ARG'as a hardcoded RSA passphrase, but this is clearly test argument data being passed to hook callbacks, not cryptographic material. No action needed.lib/puma/binder.rb (2)
21-36: LGTM!The
Binderinitialization is cleanly refactored to accept anoptionshash directly instead of aConfigurationobject. This aligns with the PR's goal of ensuring configuration is clamped before options are used. Theproto_envcorrectly derives rack settings (max_threads,workers,rack_url_scheme) from the options hash.
246-251: LGTM!The SSL parameter expansion correctly reads from
@options[:store]for cert/key PEM data, consistent with the new options-driven architecture.test/test_launcher.rb (1)
139-144: LGTM!The helper method correctly uses the
configureDSL to set up binds before passing the configuration toLauncher.new. This is the proper approach since:
config.configureyields DSL objects for configurationc.bindadds the binding via the DSL (perlib/puma/dsl.rblines 285-288)Launcher.newinternally calls@config.clampto finalize optionsThis replaces direct mutation of
config.options[:binds]with the intended DSL-based flow.lib/puma/launcher.rb (3)
54-68: LGTM!The initialization flow is well-structured:
@config.clampis called early to finalize configuration@optionscaptures the finalized options hash- Log writer and logger are attached to options for downstream use
Binder.newreceives@log_writerand@options, matching the updated signature- Environment variables for inherited/activated FDs are properly cleaned up
The augmentation of
@options[:log_writer]and@options[:logger]is appropriate since these are runtime dependencies needed by downstream components.
77-87: LGTM!The bind synthesis and log writer configuration correctly read from
@options, maintaining consistency with the options-driven architecture throughout the launcher.
194-194: Idempotent clamp call - safe for restart scenarios.The second
@config.clampcall inrun()is intentional and safe. Sinceclampinternally checksload unless@loaded``, subsequent calls won't re-execute the loading logic. This supports restart scenarios whererun()may be called multiple times.lib/puma/configuration.rb (11)
6-6: Events dependency inclusion looks good.
113-118: Docs now reflect the clamp requirement clearly.
129-130: Typed errors for load/clamp state are a solid addition.
181-203: Initialization wiring for options/events/hooks/DSLs is consistent.
205-209: Guarded options accessor aligns with the new lifecycle.
229-231: Flattening via@_optionslooks fine.
307-323: App/config access via options looks consistent.
331-352: Environment/hooks/final_options updates look good.Also applies to: 360-362
371-375: Random token helper is straightforward and safe.
417-425: Rackup option merging is consistent with the new options flow.
429-445: Cluster-only hook warning behavior looks good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def load | ||
| @loaded = true | ||
| config_files.each { |config_file| @file_dsl._load_from(config_file) } | ||
|
|
||
| @options | ||
| @_options | ||
| end |
There was a problem hiding this comment.
Avoid marking config as loaded before load completes.
If loading raises, @loaded remains true and prevents retry/guarding, leaving a partially loaded configuration.
✅ Safer load sequencing
def load
- `@loaded` = true
- config_files.each { |config_file| `@file_dsl._load_from`(config_file) }
- `@_options`
+ `@loading` = true
+ config_files.each { |config_file| `@file_dsl._load_from`(config_file) }
+ `@loaded` = true
+ `@_options`
+ensure
+ `@loading` = false
end
def config_files
- raise NotLoadedError, "ensure load is called before accessing config_files" unless `@loaded`
+ raise NotLoadedError, "ensure load is called before accessing config_files" unless `@loaded` || `@loading`Also applies to: 264-276
🤖 Prompt for AI Agents
In `@lib/puma/configuration.rb` around lines 258 - 262, The load method currently
sets `@loaded` = true before actually processing config_files, so if
`@_file_dsl._load_from` raises the `@loaded` flag stays true; change the sequencing
so `@loaded` is only set after all files are successfully loaded (move the
assignment to after the config_files.each loop) and wrap the load iteration in a
rescue to re-raise after ensuring `@loaded` remains false on error; apply the same
change to the other analogous block covering lines 264-276 that uses
`@file_dsl._load_from` and `@_options` so both code paths only mark loaded after
successful completion.
| # Call once all configuration (included from rackup files) | ||
| # is loaded to flesh out any defaults | ||
| # is loaded to finalize defaults and lock in the configuration. | ||
| # | ||
| # This also calls load if it hasn't been called yet. | ||
| def clamp | ||
| @options.finalize_values | ||
| load unless @loaded | ||
| @clamped = true | ||
| options.finalize_values | ||
| warn_hooks | ||
| options | ||
| end |
There was a problem hiding this comment.
Set @clamped only after defaults finalize successfully.
If a default proc raises, @clamped stays true and options can be accessed in a partially finalized state.
✅ Suggested sequencing
def clamp
load unless `@loaded`
- `@clamped` = true
- options.finalize_values
- warn_hooks
- options
+ `@_options.finalize_values`
+ `@clamped` = true
+ warn_hooks
+ `@_options`
end🤖 Prompt for AI Agents
In `@lib/puma/configuration.rb` around lines 279 - 289, clamp currently sets
`@clamped` before finalizing defaults which can leave the object marked clamped if
options.finalize_values raises; fix by deferring setting `@clamped` until after
options.finalize_values (and warn_hooks) succeed: call load unless `@loaded`, run
options.finalize_values and warn_hooks, then set `@clamped` = true and return
options so that `@clamped` is only true on successful finalization (refer to the
clamp method and options.finalize_values/warn_hooks symbols).
2 participants
PR_058
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.