From 05e97b53f6402e86d676ada282b04fb42b399c2a Mon Sep 17 00:00:00 2001 From: Twooey Date: Thu, 27 Aug 2026 15:26:37 -0400 Subject: [PATCH] Harden sync_all() for future live-site use: per-row savepoints, scoped cache invalidation Ahead of running this against a live site (not just staging bulk-imports), addressed the two risks that only matter under concurrent traffic: - One bad product no longer wastes a whole batch: each product save is wrapped in its own SAVEPOINT within the batch transaction, so a failure rolls back just that row (counted in the new failed[] result, surfaced via WP_CLI::warning like import-gutenberg already does) instead of discarding up to 200 already-good rows. Verified SAVEPOINT/ROLLBACK TO SAVEPOINT actually works on this MariaDB instance via a direct $wpdb test before relying on it. - Cache invalidation suspension is now scoped per-batch instead of the whole run, with clean_post_cache() called explicitly per product once each batch commits, replacing the single end-of-run wp_cache_flush(). This site runs a shared Redis object cache across PHP-FPM workers, so a whole-run suspension + full flush would let a concurrent visitor see stale cached product data for the run's duration, and the flush itself would evict sessions and everything else site-wide. Scoping bounds staleness to one batch and targets only the products actually touched. Also moved term resolution (term_exists()/wp_insert_term()) out of the per-product savepoint entirely: warm_term_cache() now resolves every distinct subject for a batch before its transaction opens, so a term created for one product can never get silently rolled back by a *different* product's savepoint failure while a stale term_id sits cached in memory and gets attached to a later product (an orphaned term_relationships row, checked for directly post-fix: 0 found). Verified after: 2000-item sync still ~90s (savepoints add no measurable overhead), 0 lookup-table mismatches, 0 term-count mismatches, 0 orphaned term_relationships, idempotent re-run with 0 duplicates. --- .../bookstore-core/includes/Cli/Commands.php | 12 +- .../includes/Product/ProductSync.php | 156 +++++++++++++----- 2 files changed, 122 insertions(+), 46 deletions(-) diff --git a/wp-content/plugins/bookstore-core/includes/Cli/Commands.php b/wp-content/plugins/bookstore-core/includes/Cli/Commands.php index 57b8831..662b181 100644 --- a/wp-content/plugins/bookstore-core/includes/Cli/Commands.php +++ b/wp-content/plugins/bookstore-core/includes/Cli/Commands.php @@ -96,7 +96,17 @@ class Commands $result = ProductSync::sync_all(200, function (int $processed, int $created, int $updated) { \WP_CLI::log(" processed {$processed} (created {$created}, updated {$updated})..."); }); - \WP_CLI::success("Products: {$result['created']} created, {$result['updated']} updated."); + + foreach ($result['failed'] as $failure) { + \WP_CLI::warning($failure); + } + + \WP_CLI::success(sprintf( + 'Products: %d created, %d updated, %d failed.', + $result['created'], + $result['updated'], + count($result['failed']) + )); } /** diff --git a/wp-content/plugins/bookstore-core/includes/Product/ProductSync.php b/wp-content/plugins/bookstore-core/includes/Product/ProductSync.php index 5f1a857..9553699 100644 --- a/wp-content/plugins/bookstore-core/includes/Product/ProductSync.php +++ b/wp-content/plugins/bookstore-core/includes/Product/ProductSync.php @@ -19,28 +19,53 @@ defined('ABSPATH') || exit; * 1. wp_defer_term_counting() — batches product_cat term-count recalculation * to once at the end instead of once per wp_set_object_terms() call. * 2. wp_suspend_cache_invalidation() — skips clean_post_cache()'s cache - * clearing during the run. Verified this does NOT risk stale reads in - * WooCommerce's own wc_product_meta_lookup sync: that sync reads back - * ~14 postmeta values via get_post_meta() immediately after save(), but - * update_post_meta()/add_post_meta() write straight into the postmeta - * object cache on write (not merely invalidate-then-refetch), and this - * flag only affects clean_post_cache()'s handling of the *posts* cache - * group — postmeta cache correctness is unaffected either way. - * 3. Static in-memory subject→term_id cache in sync_categories() — avoids + * clearing while a batch's transaction is open. Verified this does NOT + * risk stale reads in WooCommerce's own wc_product_meta_lookup sync: that + * sync reads back ~14 postmeta values via get_post_meta() immediately + * after save(), but update_post_meta()/add_post_meta() write straight + * into the postmeta object cache on write (not merely + * invalidate-then-refetch), and this flag only affects + * clean_post_cache()'s handling of the *posts* cache group — postmeta + * cache correctness is unaffected either way. + * + * Scoped to one batch at a time (suspended/lifted around each batch, + * not the whole run), with clean_post_cache() called explicitly per + * product once lifted. This site runs a shared Redis object cache + * (persistent across PHP-FPM workers, not per-request) — a concurrent + * visitor loading a product page mid-batch could otherwise see a stale + * cached price/title until invalidation resumes. Per-batch scoping + * bounds that staleness window to one batch's duration instead of the + * whole run, and targets just the touched products instead of a + * whole-cache wp_cache_flush() (which would evict sessions and every + * other cached object site-wide — a real concern once this runs + * against a live site with real traffic, not just staging). + * 3. Static in-memory subject→term_id cache, warmed once per batch via + * warm_term_cache() *before* the batch's transaction opens — avoids * repeat term_exists()/wp_insert_term() round-trips for subjects that * recur across many Works (common — PG subject strings repeat a lot). - * 4. Each batch runs inside one explicit DB transaction. WordPress/WooCommerce - * writes run in MySQL autocommit by default, so every post/postmeta/term- - * relationship/wc_product_meta_lookup write in sync_one() is its own - * implicit transaction — each one an InnoDB commit, each commit an fsync + * Deliberately resolved outside the transaction: term_exists()/ + * wp_insert_term() commit immediately in their own autocommit write, so + * a later SAVEPOINT rollback for some other product in the batch can + * never leave a cached term_id pointing at a term that got rolled back + * out of the DB. sync_categories() itself is then a pure cache lookup — + * no term creation happens inside the per-product savepoint at all. + * 4. Each batch runs inside one explicit DB transaction, with a SAVEPOINT + * around each individual product. WordPress/WooCommerce writes run in + * MySQL autocommit by default, so every post/postmeta/term-relationship/ + * wc_product_meta_lookup write in sync_one() is its own implicit + * transaction — each one an InnoDB commit, each commit an fsync * (confirmed: wc_product_meta_lookup sync is synchronous inside * WC_Product::save(), not deferred via Action Scheduler). Wrapping a * batch in one transaction turns ~batch_size fsyncs into one, which is - * where most of the win is on slow/networked storage. Caveat: this - * assumes nothing reads mid-batch data from a separate connection before - * COMMIT (true today — this site has no webhooks or other Action - * Scheduler consumers hooked on product save; would need re-checking if - * that changes). + * where most of the win is on slow/networked storage. The per-product + * SAVEPOINT means one bad row rolls back to just before itself (counted + * in $failed, run continues) instead of discarding the rest of an + * otherwise-good batch. Caveat: batching still assumes nothing reads + * mid-batch data from a separate connection before COMMIT (true today — + * this site has no webhooks or other Action Scheduler consumers hooked + * on product save; would need re-checking if that changes). For a run + * that has to share the DB with live traffic, pass a smaller + * $batch_size — shorter transactions, shorter lock windows. */ class ProductSync { @@ -49,7 +74,7 @@ class ProductSync /** * @param callable|null $on_progress function(int $processed, int $created, int $updated): void - * @return array{created:int, updated:int} + * @return array{created:int, updated:int, failed:string[]} */ public static function sync_all(int $batch_size = 200, ?callable $on_progress = null): array { @@ -58,10 +83,10 @@ class ProductSync $created = 0; $updated = 0; $processed = 0; + $failed = []; $offset = 0; wp_defer_term_counting(true); - wp_suspend_cache_invalidation(true); try { while (true) { @@ -70,18 +95,36 @@ class ProductSync break; } + $batch_product_ids = []; + + self::warm_term_cache($works); + + wp_suspend_cache_invalidation(true); $wpdb->query('START TRANSACTION'); - try { - foreach ($works as $work) { + + foreach ($works as $work) { + $wpdb->query('SAVEPOINT sp_sync_one'); + try { $is_new = empty($work->wc_product_id) || !wc_get_product((int) $work->wc_product_id); - self::sync_one($work); + $product_id = self::sync_one($work); + $wpdb->query('RELEASE SAVEPOINT sp_sync_one'); + $batch_product_ids[] = $product_id; $is_new ? $created++ : $updated++; - $processed++; + } catch (\Throwable $e) { + $wpdb->query('ROLLBACK TO SAVEPOINT sp_sync_one'); + $failed[] = "work_id={$work->work_id} ({$work->title}): " . $e->getMessage(); } - $wpdb->query('COMMIT'); - } catch (\Throwable $e) { - $wpdb->query('ROLLBACK'); - throw $e; + $processed++; + } + + $wpdb->query('COMMIT'); + wp_suspend_cache_invalidation(false); + + // Cache invalidation was suspended for the batch above — + // fire it now, scoped to just the products this batch + // actually touched, instead of a whole-cache flush. + foreach ($batch_product_ids as $product_id) { + clean_post_cache($product_id); } if ($on_progress) { @@ -90,15 +133,13 @@ class ProductSync $offset += $batch_size; } } finally { - wp_suspend_cache_invalidation(false); wp_defer_term_counting(false); - wp_cache_flush(); } - return ['created' => $created, 'updated' => $updated]; + return ['created' => $created, 'updated' => $updated, 'failed' => $failed]; } - private static function sync_one(object $work): void + private static function sync_one(object $work): int { $existing_id = (int) ($work->wc_product_id ?? 0); $product = $existing_id ? wc_get_product($existing_id) : false; @@ -129,8 +170,46 @@ class ProductSync } self::sync_categories($product_id, $work->subjects ?? null); + + return (int) $product_id; } + /** + * Resolves (creating if needed) a product_cat term for every distinct + * subject across this batch's Works, populating self::$term_cache. + * Runs before the batch's transaction opens — see class docblock §3 + * for why that ordering matters. + * + * @param object[] $works + */ + private static function warm_term_cache(array $works): void + { + $new_subjects = []; + foreach ($works as $work) { + $subjects = !empty($work->subjects) ? json_decode($work->subjects, true) : []; + if (!is_array($subjects)) { + continue; + } + foreach ($subjects as $subject) { + $subject = trim((string) $subject); + if ($subject !== '' && !isset(self::$term_cache[$subject])) { + $new_subjects[$subject] = true; + } + } + } + + foreach (array_keys($new_subjects) as $subject) { + $term = term_exists($subject, 'product_cat'); + if (!$term) { + $term = wp_insert_term($subject, 'product_cat'); + } + if (!is_wp_error($term) && isset($term['term_id'])) { + self::$term_cache[$subject] = (int) $term['term_id']; + } + } + } + + /** Pure cache lookup — see warm_term_cache(), which populates it ahead of time. */ private static function sync_categories(int $product_id, ?string $subjects_json): void { $subjects = $subjects_json ? json_decode($subjects_json, true) : []; @@ -141,21 +220,8 @@ class ProductSync $term_ids = []; foreach ($subjects as $subject) { $subject = trim((string) $subject); - if ($subject === '') { - continue; - } - if (isset(self::$term_cache[$subject])) { + if ($subject !== '' && isset(self::$term_cache[$subject])) { $term_ids[] = self::$term_cache[$subject]; - continue; - } - $term = term_exists($subject, 'product_cat'); - if (!$term) { - $term = wp_insert_term($subject, 'product_cat'); - } - if (!is_wp_error($term) && isset($term['term_id'])) { - $term_id = (int) $term['term_id']; - self::$term_cache[$subject] = $term_id; - $term_ids[] = $term_id; } }