Correctly read the cache_format_version setting on boot

[ghiculescu]

Summary

Fixes #45289

Other Information

I'm not sure if it's an antipattern to read from the config of specific modules in railties/lib/rails/application/bootstrap.rb. But we know for sure Active Support has loaded by then (I think), and this seems like the only way to ensure that this config is used correctly.

[byroot]

Urgh, thanks. We really need to dig into the root cause for this, it's not normal that most settings are broken now. Something must have changed around Rails 7.

[ghiculescu]

Yeah, my first thought was it another instance of #44996

But this one seems a bit different, because the Rails.cache initializer is intentionally done before all the railties that set config based on app config. I don't think #44996 would have helped here.

[byroot]

Ah my bad.

[ghiculescu]

Hmm, should we consider changing the 70Coder to 71Coder, and making 70Coder the same as 61Coder?

If someone uncommented this line:

rails/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt

Line 42 in d15a694

# Rails.application.config.active_support.cache_format_version = 7.0

They would still be using the 61Coder at the moment. But once they get this PR they'll suddenly get the 70Coder and that could break things ("Only change this value after your application is fully deployed to Rails 7.0 and you have no plans to rollback"). This is particularly a risk if we backport this to 7-0-stable (which I think we should do).

So given that, I think we should push the new coder being the default down to Rails 7.1, and keep the old coder as the Rails 7 default along with this fix.

I can do a PR for this if you agree.

[byroot]

But then we'd break people who have config.load_defaults 7.0, so I don't think we can do that.

[ghiculescu]

But then we'd break people who have config.load_defaults 7.0, so I don't think we can do that.

No we wouldn't, because currently if you do config.load_defaults 7.0 you still get the 6.1 coder (until this PR is merged). So it would be the same behaviour for that case. That's kinda my point - if we don't change that then this PR will silently change behavior for 7.0 users.

[byroot]

currently if you do config.load_defaults 7.0 you still get the 6.1 coder

WAT? From application.rb? Ok, I think I need to dig more into this tomorrow, because I don't quite understand what's going on here.

[ghiculescu]

WAT? From application.rb?

Yeah, that's what this PR fixes - there's no initializers involved here at all.

edit: there is now - I pushed another test. Even with this fix, updating the new framework defaults will still give you the wrong coder.

[casperisfine]

No we wouldn't, because currently if you do config.load_defaults 7.0 you still get the 6.1 coder

So I just tried a new Rails 7 app, and AFAICT it works:

$ rails --version
Rails 7.0.3
$ rails new test-app --api
      create  
      create  README.md
...
$ cd test-app/
$ bin/rails console
Loading development environment (Rails 7.0.3)
>> ActiveSupport.cache_format_version
=> 7.0

[casperisfine]

🤦 sorry, I didn't finish my coffee, that's not a proper way to check, because it's the value when the cache store is instantiated that matters.

[casperisfine]

should we consider changing the 70Coder to 71Coder, and making 70Coder the same as 61Coder?

Ok, let's go with this. Do you wanna handle it?

[casperisfine]

There's still one problem though, since cache is initialized so early, there's no way to configure it it from config/initializers.

[ghiculescu]

should we consider changing the 70Coder to 71Coder, and making 70Coder the same as 61Coder?

Ok, let's go with this. Do you wanna handle it?

Can do.

There's still one problem though, since cache is initialized so early, there's no way to configure it it from config/initializers.

Yeah I’m also struggling to think of a solution for this. My best idea so far is a REALLY BIG WARNING in the new framework defaults file :/

[casperisfine]

The thing is, it happens so early that even load_defaults is too late, that seems crazy to me.

I think we'll either have to delay it, or to initialize it twice (but that later may cause problem as people might write incompatible payloads in initializers).

But to be honest the justification to have it so early seem weird. I really don't see an use case for accessing the cache as part of the boot.

[ghiculescu]

I am still waking up too but I think load_defaults does (with this patch) work. But initializers definitely don’t.

Your point about cache being set up so early is fair though. That would resolve the issue totally, if we could delay it.

I assume it’s done like that so you can do Rails.cache…. in initializers. I dunno why you’d want to but I suppose it’s there 🤷‍♂️

[casperisfine]

so you can do Rails.cache…. in initializers.

Yeah, but that's a bad idea. You shouldn't do any network access during boot.

[ghiculescu]

should we consider changing the 70Coder to 71Coder, and making 70Coder the same as 61Coder?

Ok, let's go with this. Do you wanna handle it?

Can do.

There's still one problem though, since cache is initialized so early, there's no way to configure it it from config/initializers.

Yeah I’m also struggling to think of a solution for this. My best idea so far is a REALLY BIG WARNING in the new framework defaults file :/

Alright, I implemented this. The config/initializers bit is pretty dodgy though. I might spend some time seeing what happens if you do the Rails.cache initializing later.

I also made #45304 which I think might decrease the impact of the current broken state.

[byroot]

One minor thing, but LGTM.

This whole thing is really unfortunate, but I don't see any better way forward.

[ghiculescu]

I might spend some time seeing what happens if you do the Rails.cache initializing later.

Quick brain dump on this. I tried moving the cache initializer from bootstrap.rb to the activesupport railtie + using config.after_initialize. That doesn't work because the middleware stack is frozen by then (from here). If you comment out the middleware bit, the rest works and you can config this via an initializer. But the middleware bit needs more investigation.

[byroot]

@ghiculescu actually I think we can simply fix the load_defaults 7.0. The Rails61 coder is forward compatible. What people shouldn't have done was to upgrade directly from Rails 6.1 with load_defaults 6.1 into Rails 7.0 with load_defaults 7.0.

But someone upgrading from Rails 7 to Rails 7.1 won't have this problem. Same if we backport this fix, it won't be a problem.

[ghiculescu]

The Rails61 coder is forward compatible.

So it is. I missed that the first 10000000 times I thought about this.

[ghiculescu]

@ghiculescu actually I think we can simply fix the load_defaults 7.0.

@byroot I changed it back. What do you think about #45304 ? Given it definitely doesn't work in an initializer I still lean toward removing it from the file to avoid confusion.

[byroot]

Backported as cb1a753.

[ghiculescu]

Thanks for bearing with me on this one, it was starting to hurt my brain.

[byroot]

Same 😅.