Memory Wiki

I'll start by reading the plugin file and checking the DB schema simultaneously.

Let me check the SSH key configuration.

SSH works with explicit key. Now let me do all the checks.

Let me explore the docker setup and find the actual paths.

File exists inside the container. Let me read it properly, and also fix the DB query.

All data retrieved. Let me now closely examine the critical transaction sequence in the consumer function.

Good, no syntax errors. Now let me verify one more detail about the old index's existence (or non-existence) and check the db container's MySQL version for `SKIP LOCKED` support.

All data gathered. Here is my complete read-only review. --- ## Code Review: `profe-sync.php` on test.thelinguagroup.com **File:** `/var/www/html/wp-content/mu-plugins/profe-sync.php` (inside container `test-wp-wordpress-1`) **DB:** `test-wp-db-1` | MySQL 8.4.9 **Syntax check:** No errors --- ### ✅ Fix 1: UNIQUE index — CONFIRMED The old plain index `idx_user_tag` has been fully removed. The new index is: ``` UNIQUE KEY `idx_user_tag_unique` (`user_id`,`tag`) ``` Confirmed via `SHOW INDEX FROM profe_main.profe_crm` — only three indexes exist: `PRIMARY`, `idx_user_tag_unique` (UNIQUE, Non_unique=0), and `idx_processed`. No leftover `idx_user_tag`. The `INSERT IGNORE` in `profe_crm_log_membership()` appropriately relies on this UNIQUE constraint. ✅ --- ### ✅ Fix 2: Cancellation handler — CONFIRMED In `profe_crm_log_membership()`, lines as follows: ```php if (empty($level_id)) { $tag = "free"; } else { // ... normal level name lookup via $wpdb } ``` When `level_id = 0` (cancellation), the function maps to `"free"` instead of returning early. It then proceeds to `INSERT IGNORE` with the "free" tag. The early-return bug on line 8 of the hooked function block is fixed. ✅ **Edge case OK:** `if (empty($tag)) return;` — "free" is not empty, so it passes. The tag does get written. **Side observation:** After the `INSERT IGNORE` (which succeeds if the row is new, or is a no-op if it already exists), the code checks `rows_affected === 0` and issues an `UPDATE` to reset `processed = 0` and update `created_at`. This is correct: on duplicate, the update ensures the consumer re-processes it. However, note that `created_at` is overwritten to `NOW()` on re-processing, so it no longer reflects the original entry time — this is likely intentional to surface the re-queue time. --- ### ⚠️ Fix 3: Race condition — PARTIALLY CORRECT The consumer (`profe_crm_process_tags`) uses `FOR UPDATE SKIP LOCKED`, which is the right mechanism. MySQL 8.4.9 supports it. **However, the `SELECT ... FOR UPDATE SKIP LOCKED` runs OUTSIDE the transaction**, making the row locks ineffective. Current code (lines 276-295): ```php // ❌ SELECT runs in auto-commit mode — locks released immediately after statement $rows = $profe_db->get_results("SELECT * FROM profe_crm WHERE processed = 0 ORDER BY id ASC LIMIT 50 FOR UPDATE SKIP LOCKED"); if (empty($rows)) return; $profe_db->query("START TRANSACTION"); // ← Too late! Locks already released foreach ($rows as $row) { // ... process ... $profe_db->query($profe_db->prepare( "UPDATE profe_crm SET processed = 1 WHERE id = %d", $row->id )); } $profe_db->query("COMMIT"); ``` **Why this matters:** In MySQL with InnoDB and auto-commit on (the default), a `SELECT ... FOR UPDATE` runs inside its own implicit transaction that is auto-committed when the statement finishes. The row locks are held only for the duration of that single statement. So: 1. Two concurrent PHP workers both execute the `SELECT ... FOR UPDATE SKIP LOCKED`. Each implicitly commits immediately. Neither holds locks. 2. Both workers could feasibly receive the same rows (though `SKIP LOCKED` offers some protection within the micro-batch). 3. Both workers proceed to `START TRANSACTION` without any held locks. 4. Both update `processed = 1` on the same rows — this is not destructive per se (same value), but it means **duplicate tags could be applied in FluentCRM** because the consumer could run the same row twice. **Correct fix** — move `START TRANSACTION` before the SELECT: ```php $profe_db->query("START TRANSACTION"); $rows = $profe_db->get_results("SELECT * FROM profe_crm WHERE processed = 0 ORDER BY id ASC LIMIT 50 FOR UPDATE SKIP LOCKED"); if (empty($rows)) { $profe_db->query("COMMIT"); return; } foreach ($rows as $row) { // ... process ... $profe_db->query($profe_db->prepare( "UPDATE profe_crm SET processed = 1 WHERE id = %d", $row->id )); } $profe_db->query("COMMIT"); ``` This way, `FOR UPDATE SKIP LOCKED` acquires locks within the transaction and holds them until `COMMIT`. Concurrent workers will skip locked rows reliably. **Impact assessment:** In practice, the risk is low-level duplicate tag application in FluentCRM, not data corruption. The `attachTags()` call is idempotent (FluentCRM doesn't double-attach), so even duplicate processing of the same row produces no visible symptom. The bug primarily affects efficiency — two workers may process the same rows unnecessarily. --- ### ✅ Additional checks | Check | Result | |---|---| | **Syntax errors** | None (PHP lint passed) | | **INSERT IGNORE logic** | Correct. On new row → inserted, processed=0. On duplicate → rows_affected=0 triggers UPDATE to reprocess. | | **`rows_affected === 0`** | Works correctly with `wpdb`. `mysqli_affected_rows()` returns 0 for INSERT IGNORE duplicates. | | **Cancellation "free" tag** | Properly inserted as `INSERT IGNORE`. Non-empty. Correct slug. | | **Consumer FluentCRM calls** | `firstOrCreate` with safe slugs. `attachTags` is idempotent. | | **Old index cleaned up** | Confirmed — no `idx_user_tag` in schema or code. | --- ### 🟡 Additional observations (non-blocking) 1. **Orphan `COMMIT` statements** — Functions `profe_sync_on_register`, `profe_schedule_crons`, `profe_cron_billing_geo_cleanup`, and `profe_cron_backfill_memberships` each issue `$profe_db->query("COMMIT")` without a matching `START TRANSACTION`. This is harmless in MySQL (it just commits the current implicit transaction), but it's dead code that suggests the transaction model isn't fully understood. 2. **Cancellation only adds "free", doesn't remove old tags** — The consumer only calls `$contact->attachTags([$tag->id])`. When a user cancels, "free" is added alongside existing paid tags (e.g., "pro_monthly"). If the design intent is to *replace* paid tags with "free", the consumer would need to also call `$contact->detachTags(...)`. This may be by design if FluentCRM uses tag-based lifecycle rules. 3. **`created_at` overwritten on re-queue** — The `UPDATE` in `profe_crm_log_membership()`'s duplicate path sets `created_at = NOW()`. If audit tracking of original creation time matters, this could be changed to only update `processed`. --- ### Summary | Fix | Status | Notes | |---|---|---| | 1. UNIQUE index (`idx_user_tag_unique`) | ✅ Correct | Properly applied, old index cleaned up | | 2. Cancellation handler (level_id=0 → "free") | ✅ Correct | INSERT IGNORE with "free" tag, properly handled | | 3. Race condition (FOR UPDATE SKIP LOCKED) | ⚠️ **Incomplete** | `START TRANSACTION` is after the SELECT; locks are released immediately. Move `START TRANSACTION` before the SELECT to make locks effective. | **Recommendation:** Move the `START TRANSACTION` before the `SELECT ... FOR UPDATE SKIP LOCKED` in `profe_crm_process_tags()` to make the locking actually work across the full critical section. The remaining issues (orphan COMMITs, cancellation tag semantics) are non-blocking design notes.