Skip to content

fix(locks): make cron admission atomic - #522

Open
sjinks wants to merge 5 commits into
mainfrom
pltfrm-2759-make-cron-control-concurrency-admission-atomic
Open

fix(locks): make cron admission atomic#522
sjinks wants to merge 5 commits into
mainfrom
pltfrm-2759-make-cron-control-concurrency-admission-atomic

Conversation

@sjinks

@sjinkssjinks commented Aug 27, 2026

Copy link
Copy Markdown
Member

Summary

  • replace request-local cache counters with an atomic options-table lock state
  • assign each lock row a random generation and prevent stale cleanup from releasing a recovered or recreated lock
  • release all event action locks through the generation-aware path and remove inactive lock rows

Testing

  • PHP syntax checks
  • PHPCS
  • git diff --check
  • targeted PHPUnit is blocked locally because /tmp/wordpress-tests-lib and its MariaDB-backed WordPress test database are unavailable

Refs: https://linear.app/a8c/issue/PLTFRM-2759

Persist concurrency state in the options table and bind workers to
generation-aware leases so stale cleanup cannot release a replacement lock.
Refs: PLTFRM-2759
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI lite review requested due to automatic review settings August 27, 2026 20:49
@github-actions

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors Cron Control’s concurrency locking to use an atomic, database-backed lock state (stored in the options table) and introduces generation-aware release logic to prevent stale workers from accidentally releasing newer/recovered locks.

Changes:

  • Replaced object-cache-based lock counters with an atomic INSERT ... ON DUPLICATE KEY UPDATE lock state stored in wp_options.
  • Added generation tracking per admitted lock acquisition and made free_lock() generation-aware (including deleting inactive lock rows).
  • Added unit tests covering admission limits, stale recovery, and old-generation cleanup behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

FileDescription
includes/class-lock.phpImplements atomic options-table lock state + generation-aware acquisition/release and parsing helpers.
includes/class-events.phpRoutes event action lock release through Lock::free_lock() consistently.
tests/unit-tests/test-lock.phpAdds unit tests validating concurrency limits and generation-safe cleanup semantics.
Suppressed comments (2)

includes/class-lock.php:152

  • random_int() can throw, and this code path currently doesn’t handle that. A failure here would prevent priming locks and may cascade into cron admission failures. Consider the same try/catch + wp_rand() fallback used in check_lock().
 $wpdb->query(
$wpdb->prepare(
"INSERT IGNORE INTO `$wpdb->options` (`option_name`, `option_value`, `autoload`) VALUES (%s, %s, 'no')",
self::get_key( $lock ),
self::build_lock_state( 0, time(), random_int( 1, \PHP_INT_MAX ) )
)

includes/class-lock.php:192

  • random_int() can throw and isn’t caught here, which could make reset_lock() fatally error (potentially leaving locks stuck). Consider catching \Throwable and falling back to wp_rand() for generation selection.
 $now = time();
$generation = random_int( 1, \PHP_INT_MAX );
$result = $wpdb->query(

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadincludes/class-lock.php Outdated
Comment threadincludes/class-lock.php Outdated
Use WordPress's non-throwing random wrapper for lock-row generation
so an unavailable CSPRNG cannot interrupt cron admission, lock priming,
or lock reset.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@sjinks
sjinks requested a balanced review from CopilotAugust 27, 2026 21:21
@sjinkssjinks self-assigned this Aug 27, 2026
sjinksand others added 2 commits August 28, 2026 00:23
Resolve the namespaced wp_rand call so the test bootstrap can load the
lock class.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the WordPress random wrapper when available and a bounded native
fallback while the lock class initializes before WordPress functions load.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tests/unit-tests/test-lock.php:37

  • Despite the test name, these calls are sequential, so the previous read-then-increment implementation would also pass this test. The PR's core regression is simultaneous admissions observing the same prior count; add a test using separate database connections/processes synchronized to contend on the same lock and assert that only one acquisition succeeds.
 $this->assertTrue( Lock::check_lock( $lock, 1 ) );
$this->assertFalse( Lock::check_lock( $lock, 1 ) );

Comment threadincludes/class-lock.php Outdated
Comment threadincludes/class-lock.php Outdated
Keep an acquired generation after a failed database release, and stop
unnecessary lock priming writes.
Refs: PLTFRM-2759
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@sjinks