[D7] Cron lock time limit is too short and does not prevent multiple, concurrent cron runs [#3308929]

Problem/Motivation

This is a D7 backport of: #2319177: Cron lock time limit is too short and does not prevent multiple, concurrent cron runs

Patch from #9 was working, but we decided to add a possibility to opt-out of this change. Therefore we want to introduce a new config option in setting.php.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comment	File	Size	Author
#2	interdiff-9_2.txt	1.29 KB	poker10
#2	3308929-2.patch	1.45 KB	poker10
#2	7.x: PHP 7.4 & MySQL 5.7 2,143 pass PHP 7.4 & SQLite 3.27 2,139 pass PHP 5.3 & MySQL 5.5 2,143 pass PHP 5.6 & MySQL 5.5 2,143 pass PHP 7.2 & MySQL 5.6 Patch Failed to Apply PHP 7.3 & MySQL 5.7 2,143 pass PHP 7.4 & MySQL 8 2,143 pass PHP 7.4 & PostgreSQL 9.5 2,104 pass PHP 8.0 & MySQL 5.7 2,143 pass PHP 8.1 & MySQL 5.7 2,143 pass PHP 8.2 & sqlite-3.34 2,139 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

9 September 2022 at 18:52

poker10 created an issue. See original summary.

Comment #2

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented 9 September 2022 at 18:56

Status:	Active	» Needs review
Issue tags:		+Needs change record, +Needs manual testing

File	Size
3308929-2.patch	1.45 KB
7.x: PHP 7.4 & MySQL 5.7 2,143 pass PHP 7.4 & SQLite 3.27 2,139 pass PHP 5.3 & MySQL 5.5 2,143 pass PHP 5.6 & MySQL 5.5 2,143 pass PHP 7.2 & MySQL 5.6 Patch Failed to Apply PHP 7.3 & MySQL 5.7 2,143 pass PHP 7.4 & MySQL 8 2,143 pass PHP 7.4 & PostgreSQL 9.5 2,104 pass PHP 8.0 & MySQL 5.7 2,143 pass PHP 8.1 & MySQL 5.7 2,143 pass PHP 8.2 & sqlite-3.34 2,139 pass
interdiff-9_2.txt	1.29 KB

Adding a patch which adds a new settings.php config variable to allow sites override this cron lock expiration timeout. Interdiff is with the patch #9 from the parent issue.

I do not think we can easily create an automated test for this, so setting as Needs manual testing. Also this would need a change record.

Comment #3

mcdruid

🇬🇧🇪🇺

CreditAttribution: mcdruid as a volunteer and at Acquia commented 28 November 2022 at 19:43

Status:	Needs review	» Reviewed & tested by the community
Issue tags:	-Needs change record

Draft CR: https://www.drupal.org/node/3324216

I think this looks good.

One thought; do we want to adjust the time limit that's set just before this too?

Noting that PHP's time limit does not count elapsed wall time and that D9 still has the same 240s limit:

https://git.drupalcode.org/project/drupal/-/blob/9.4.8/core/lib/Drupal/C...

I think that would definitely be one for a follow-up and backport (if none already exists) but seems a little odd that this is still there and hard-coded. Because it only counts PHP execution time, it's probably irrelevant in most cases anyway (unless anyone's mining crypto in PHP on cron!).

Comment #4

28 November 2022 at 19:50

mcdruid committed 0a9c6cc on 7.x

Issue #3308929 by poker10, swentel, fago, catch, alexpott, Berdir: [D7]...

CreditAttribution: mcdruid as a volunteer and at Acquia commented 28 November 2022 at 20:01

Status:

Reviewed & tested by the community

» Fixed

I did a little manual testing of this with Xdebug to ensure that a cron lock is now acquired with a 900s timeout.

Thank you!

Comment #11

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented 29 November 2022 at 19:07

Issue tags:	-Needs manual testing
Related issues:		+#2183483: Cron lock time can be outrun by max execution time, +#547998: Make cron time limit a variable

Noting that PHP's time limit does not count elapsed wall time and that D9 still has the same 240s limit:

https://git.drupalcode.org/project/drupal/-/blob/9.4.8/core/lib/Drupal/C...

I think that would definitely be one for a follow-up and backport (if none already exists) but seems a little odd that this is still there and hard-coded. Because it only counts PHP execution time, it's probably irrelevant in most cases anyway (unless anyone's mining crypto in PHP on cron!).

Probably these two issues are related to the potential next steps:

#2183483: Cron lock time can be outrun by max execution time (D7 issue)
#547998: Make cron time limit a variable (D9 issue)