Are the test failures related to (these) tests now using preloading (and that causes some problem)?

While #3044 is classed as a bug, it is quite the change in behaviour... so maybe the fix should be shipped in a major version?

Are the test failures related to (these) tests now using preloading (and that causes some problem)?

I think some of the phased restart failures are because of this clause. We should just make fork_worker one of the conditions to not default preload_app to true if they conflict. Addressed in these changes. I'll come back and figure out the remaining failures after the break. Merry Christmas 🎄

Edit:

I think I misunderstood that condition. It seems that preload_app is compatible fork_worker, but incompatible with pure phased restart. So the real fix is to make preload_app disabled explicitly in these tests. Addressed in:

• https://github.com/puma/puma/compare/f9ea9e864c1970ecf680b0302c1cf4c4822b226f..c9f75ec6c3c1deae6a17b8dbfa744e8f06f00992
• https://github.com/puma/puma/compare/13f06f18e0de58922202b9ee232d14c826ba5a27..4b7d90d56ea8c7114391f4056578e6a5cb5c5cf8

While #3044 is classed as a bug, it is quite the change in behaviour... so maybe the fix should be shipped in a major version?

I agree, and yes the issue was added to the 7.0.0 milestone.

I've updated this to target #3616. Once that has been merged, this should go green after rebasing.

I've updated this to target #3616. Once that has been merged, this should go green after rebasing.

Nvm, I've just patched those tests here. Now neither is dependant on the other and both should cleanly rebase after the other is merged.

I'm not sure why but the UX is telling me that @nateberkopec requested changes even though I don't see the request (and I see an approval from him). All tests are now green. @nateberkopec can you merge in or re-iterate your concerns that seem to have been lost to the snads of time?

I'm not sure why but the UX is telling me that @nateberkopec requested changes even though I don't see the request (and I see an approval from him). All tests are now green. @nateberkopec can you merge in or re-iterate your concerns that seem to have been lost to the snads of time?

It was because there was a test failure:

Test fails? 🤔

Which has since been resolved:

The last failure is related to #3557 (comment)

I'm pretty sure we want fork_worker enabled for that test, and here I've also disabled preloading for consistency with the other tests since it's not compatible with phased restart. However, enabling fork_worker seems to slow the test down enough for it to time out.

Edit: Resolved in #3589

Now neither is dependant on the other and both should cleanly rebase

Re #3616, just checking if the above is still true. I haven't looked much at this one, but @nateberkopec went thru it...

Now neither is dependant on the other and both should cleanly rebase

Re #3616, just checking if the above is still true. I haven't looked much at this one, but @nateberkopec went thru it...

Probably not with my recent changes. Once #3616 has been merged, I'll come and clean this up. I might have to update the #clamp changes and I can remove some unnecessary #load calls in tests.

FWIW, if I had to pick a PR (of mine) that definitely should go out with v7, it would be this one. I've worked with 2 monoliths now where they almost set WEB_CONCURRENCY without realising they would be enabling preloading, thinking that when they bumped to v5, preloading was already enabled. It's quite confusing default behaviour. We should also include a big warning next to this PR in the changelog that bumping to v7 would enable preloading by default if in cluster mode, regardless of whether the configuration is done via DSL or env var, and that people should set preload_app!(false) to maintain current behaviour—highly advised because you need to be careful when enabling this if you have threads spawned on boot.

cc @schneems

#3616 is merged. Can you rebase on master/main (thanks).

Cancel 'older' CI tests?

Cancel 'older' CI tests?

Yes please, thank you!

#3616 is merged. Can you rebase on master/main (thanks).

Done 👍🏽