Store freeze opt-out flag outside IsolatedExecutionState - #157
Open
joeljunstrom wants to merge 2 commits into
Open
Store freeze opt-out flag outside IsolatedExecutionState#157joeljunstrom wants to merge 2 commits into
joeljunstrom wants to merge 2 commits into
Conversation
The flag was a thread_mattr_accessor, which routes through ActiveSupport::IsolatedExecutionState. Rails 8.1 defaults isolation_level to :fiber and flips it inside after_initialize; the setter clears the old scope's storage before swapping, wiping the false written at eager-load by Ext::Core::Object and Ext::Core::String. The default true then takes over and Refrigerator#freeze_all installs instance_variable_get/set overrides on Object, breaking anything that introspects ivars (e.g. the postgres adapter in destroy_all/update_all). Storing the flag in a singleton-class ivar on the host keeps each Freezeable independent (which is why mattr_accessor was avoided here - its class variable would leak through Object's ancestor chain) and is unaffected by the isolation_level switch.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates how Console1984::Freezeable stores the prevent_instance_data_manipulation_after_freezing flag so it doesn’t reset when ActiveSupport::IsolatedExecutionState is cleared, and adds tests to lock in the intended behavior.
Changes:
- Replace
thread_mattr_accessorstorage with per-host storage on the including class. - Add tests covering default value, persistence across
IsolatedExecutionState.clear, and non-leakage between hosts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/freezeable_test.rb | Adds coverage for default behavior, persistence across isolation state clearing, and isolation between hosts. |
| lib/console1984/freezeable.rb | Changes flag storage to avoid IsolatedExecutionState clearing and prevent ancestor-chain leakage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+26
to
+36
| # true by default. Stored as a singleton-class instance variable on the host so it survives | ||
| # +ActiveSupport::IsolatedExecutionState+ being cleared when Rails switches +isolation_level+ | ||
| # (e.g. the +:thread+ -> +:fiber+ flip the Rails 8.1 default triggers in +after_initialize+). | ||
| # A +mattr_accessor+ would leak the flag across the ancestor chain because +Console1984::Ext::Core::Object+ | ||
| # is included into +Object+; this storage keeps each host independent. | ||
| base.singleton_class.class_eval do | ||
| attr_writer :prevent_instance_data_manipulation_after_freezing | ||
|
|
||
| define_method :prevent_instance_data_manipulation_after_freezing do | ||
| return @prevent_instance_data_manipulation_after_freezing if defined?(@prevent_instance_data_manipulation_after_freezing) | ||
| true |
Author
There was a problem hiding this comment.
idk up to maintainer, feels like we are trading a lot of complexity for something that would be rare indeed in a console session?
Author
There was a problem hiding this comment.
Also I do not think this is true?
Comment on lines
+26
to
+30
| # true by default. Stored as a singleton-class instance variable on the host so it survives | ||
| # +ActiveSupport::IsolatedExecutionState+ being cleared when Rails switches +isolation_level+ | ||
| # (e.g. the +:thread+ -> +:fiber+ flip the Rails 8.1 default triggers in +after_initialize+). | ||
| # A +mattr_accessor+ would leak the flag across the ancestor chain because +Console1984::Ext::Core::Object+ | ||
| # is included into +Object+; this storage keeps each host independent. |
Author
There was a problem hiding this comment.
Pretty darn nit-picky mr robot =)
| attr_writer :prevent_instance_data_manipulation_after_freezing | ||
|
|
||
| define_method :prevent_instance_data_manipulation_after_freezing do | ||
| return @prevent_instance_data_manipulation_after_freezing if defined?(@prevent_instance_data_manipulation_after_freezing) |
The previous comment blamed a Rails 8.1 default, which doesn't exist: load_defaults "8.1" never touches active_support.isolation_level, and ActiveSupport ends with self.isolation_level = :thread. The level only changes when an app sets config.active_support.isolation_level, and Rails applies it in after_initialize, which clears the boot scope's storage and repoints lookups at Fiber.current. On top of that, a per-fiber flag would be back to its default inside every new fiber, so a value written at eager-load time can never be relied on. The value lives on the class object itself, so it's a class-level instance variable, not a singleton-class one. Read it with instance_variable_defined?. Add a test for subclasses. thread_mattr_accessor keyed its storage per object_id, so a subclass had its own value defaulting to true, and class-level instance variables aren't inherited either.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We saw our prod consoles starting to break after switching to fiber isolation. This happened immediately when Rails tries to connect to postgres.
The flag was a thread_mattr_accessor, which routes through ActiveSupport::IsolatedExecutionState. We set
config.active_support.isolation_level = :fiber, and Rails applies that in after_initialize: the setter clears the boot scope's storage and repoints lookups at Fiber.current, so the false written at eager-load by Ext::Core::Object and Ext::Core::String is gone. A per-fiber flag is also back to its default inside every new fiber, so a value written at load time can't be relied on either way.The default true then takes over and Console1984::Freezeable.freeze_all installs instance_variable_get/set overrides on Object, breaking anything that introspects ivars (e.g. the postgres adapter in destroy_all/update_all).
Storing the flag in a class-level instance variable on the host keeps each Freezeable independent (which is why mattr_accessor was avoided here, its class variable would leak through Object's ancestor chain) and is unaffected by the isolation_level switch.