fix: guard core DB upgrades against concurrent queueing#540
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff11e6591a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds a TTL-based DB upgrade lock and lock-management helpers to Activator, reworks version_check() to acquire/release the lock, defers db_upgrade to the woocommerce_init hook with a shutdown fallback to release the lock, and adds tests covering lock presence, expiry, creation, and shutdown fallback behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Activator
participant WP_Options as "WP Options (DB)"
participant WooCommerce as "woocommerce_init"
participant PHP as "shutdown"
Activator->>WP_Options: version_check() read old_version
Activator->>Activator: cast old_version -> string, compare
Activator->>Activator: acquire_db_upgrade_lock()
Activator->>WP_Options: add_option(lock_key, now)
alt add_option succeeded
Activator->>WP_Options: schedule db_upgrade on woocommerce_init (defer)
Activator->>PHP: register shutdown fallback to release lock
else add_option failed (locked)
Activator->>WP_Options: read lock timestamp
alt lock fresh (not expired)
Activator-->>Activator: bail (no migration queued)
else lock expired
Activator->>WP_Options: atomic UPDATE to steal stale lock
alt update succeeded
Activator->>WP_Options: schedule db_upgrade on woocommerce_init (defer)
Activator->>PHP: register shutdown fallback to release lock
else
Activator-->>Activator: bail (another process won)
end
end
end
WooCommerce->>Activator: woocommerce_init triggers db_upgrade
Activator->>Activator: try { db_upgrade(locked_old) } finally { release_db_upgrade_lock() }
Activator->>WP_Options: delete_option(lock_key)
PHP->>Activator: on shutdown -> ensure release_db_upgrade_lock()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/includes/Test_Activator.php (1)
184-198: Consider adding a test for expired lock scenario.This test verifies that a fresh lock blocks migration, but there's no complementary test ensuring an expired lock allows migration to proceed. Given the TTL-based locking mechanism, testing the boundary condition would improve confidence.
💡 Suggested additional test
/** * Test: migration is queued when upgrade lock has expired. * * `@covers` ::version_check */ public function test_migration_queued_when_upgrade_lock_has_expired(): void { update_option( self::DB_VERSION_OPTION, '1.8.6' ); // Set an expired lock (e.g., 60 seconds in the past, assuming TTL < 60s). update_option( self::DB_UPGRADE_LOCK_OPTION, time() - 60, false ); $activator = new Activator(); $reflection = new ReflectionClass( $activator ); $version_check = $reflection->getMethod( 'version_check' ); $version_check->setAccessible( true ); $version_check->invoke( $activator ); $this->assertTrue( has_action( 'woocommerce_init' ) !== false, 'Migration should be queued when upgrade lock has expired' ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/includes/Test_Activator.php` around lines 184 - 198, Add a complementary test that asserts migration is queued when the TTL-based upgrade lock has expired: in tests/includes/Test_Activator.php create a new test method (e.g., test_migration_queued_when_upgrade_lock_has_expired) that sets DB_VERSION_OPTION to '1.8.6', sets DB_UPGRADE_LOCK_OPTION to a timestamp sufficiently in the past (e.g., time() - 60), instantiates Activator, uses ReflectionClass to access and invoke the protected/private version_check method, then assert that has_action('woocommerce_init') indicates the migration was queued; reference the existing test_no_migration_queued_when_fresh_upgrade_lock_exists for structure and reuse the same reflection approach to call version_check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/Activator.php`:
- Around line 212-237: Replace the current non-atomic get_option/update_option
lock logic in Activator with WordPress's atomic lock APIs: require_once the
class-wp_upgrader.php and call WP_Upgrader::create_lock('wc_db_upgrade',
self::DB_UPGRADE_LOCK_TTL) to attempt to acquire the lock, return early if it
fails, then run Services\Settings::bump_versions() (and defer the rest of
woocommerce_init work) inside a try block and ensure you call
WP_Upgrader::release_lock('wc_db_upgrade') in a finally block so the lock is
always released.
---
Nitpick comments:
In `@tests/includes/Test_Activator.php`:
- Around line 184-198: Add a complementary test that asserts migration is queued
when the TTL-based upgrade lock has expired: in
tests/includes/Test_Activator.php create a new test method (e.g.,
test_migration_queued_when_upgrade_lock_has_expired) that sets DB_VERSION_OPTION
to '1.8.6', sets DB_UPGRADE_LOCK_OPTION to a timestamp sufficiently in the past
(e.g., time() - 60), instantiates Activator, uses ReflectionClass to access and
invoke the protected/private version_check method, then assert that
has_action('woocommerce_init') indicates the migration was queued; reference the
existing test_no_migration_queued_when_fresh_upgrade_lock_exists for structure
and reuse the same reflection approach to call version_check.
Contributor
PHPUnit Test Results 8 files 360 suites 9m 59s ⏱️ Results for commit 08d7ade. ♻️ This comment has been updated with latest results. |
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@includes/Activator.php`:
- Around line 249-280: The add_option() fast-path in acquire_db_upgrade_lock()
is TOCTOU-vulnerable; replace that branch with a call to
WP_Upgrader::create_lock() (using the same option name
self::DB_UPGRADE_LOCK_OPTION and value (string) $now) so the insertion uses
INSERT IGNORE/atomic semantics instead of get_option()+insert, and return true
when create_lock() indicates the lock was created; keep the existing stale-lock
UPDATE/steal logic unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/Activator.php`:
- Around line 232-241: The current flow sets a DB upgrade lock (via $locked_old)
and defers release to the closure attached with add_action('woocommerce_init',
...) which may never run; update the logic so that after bump_versions()
completes you either call release_db_upgrade_lock() immediately (if db_upgrade()
does not need the lock) or also register a shutdown handler
(register_shutdown_function) that calls release_db_upgrade_lock() as a safety
net; ensure the change references the existing bump_versions(), db_upgrade(...,
VERSION) call and release_db_upgrade_lock() so the lock is always released even
if the woocommerce_init hook never fires.
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
1 participant
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.
Summary
woocommerce_pos_db_upgrade_lock) to avoid repeated migration queueing during concurrent requestsActivator::version_check()to return early when a fresh lock existsfinallyblock after deferredwoocommerce_initmigration executionTest_Activatorwith regression tests for lock creation and lock-based migration skip behaviorTest plan
composer run lintin/Users/kilbot/Projects/woocommerce-pospnpm run test:unit:phpin/Users/kilbot/Projects/woocommerce-postest_no_migration_queued_when_fresh_upgrade_lock_existstest_upgrade_lock_is_set_when_migration_is_queuedSummary by CodeRabbit
Bug Fixes
Tests
Chores