Skip to content

Bust object cache on writes for forms, membership, grades, subscriptions#3146

Open
flintfromthebasement wants to merge 4 commits into
gocodebox:devfrom
flintfromthebasement:fix/object-cache-invalidation
Open

Bust object cache on writes for forms, membership, grades, subscriptions#3146
flintfromthebasement wants to merge 4 commits into
gocodebox:devfrom
flintfromthebasement:fix/object-cache-invalidation

Conversation

@flintfromthebasement

Copy link
Copy Markdown
Contributor

Summary

LifterLMS has four wp_cache_get/wp_cache_set pairs whose read paths have no matching cache delete on the write paths. On hosts with a persistent object cache (Redis, Memcached, APCu) the cached values become indefinitely stale until the cache is manually flushed.

All four were reproduced against trunk on a Redis-backed install before the fix and verified resolved after. Patterns are the same shape as recent PMPro fixes #3565 and #3419.

Cases fixed

1. LLMS_Forms::get_core_forms()

Cache keys llms_core_forms / llms_core_form_ids were never cleared when a form was saved, trashed, untrashed, deleted, or when _llms_form_is_core / _llms_form_location postmeta changed. Result: checkout / registration / account screens could render the wrong core form for a location indefinitely after an admin re-saved a form on an object-cached host.

2. LLMS_Membership::query_associated_posts()

Cache key membership_{id}_associated_{post_type} was never cleared when _llms_restricted_levels or _llms_availability_restrictions postmeta changed on an associated post, or when the post was trashed / deleted. Result: restricted-content lists and members-only access plan lists could stay wrong indefinitely after a content edit.

Bonus: that entry was written to the empty cache group (the third arg of wp_cache_set was omitted), which lands it in WordPress's default group — global by default on multisite unless explicitly added to wp_cache_add_global_groups(). Moved to a named per-membership group llms_membership_{id}_associated using the existing LLMS_Cache_Helper::get_prefix() prefix-bump pattern (already used by the session handler), so a single invalidate_group() call orphans every cached post type for that membership.

3. LLMS_Grades::get_grade()

The student_{user_id} cache group had no wp_cache_delete callers anywhere in the codebase. Result: a cached grade could survive a quiz attempt being completed, a lesson being marked complete/incomplete, or an enrollment being deleted, indefinitely. Student dashboard, certificates, and reports would render stale grades.

Switched to the LLMS_Cache_Helper prefix-bump pattern keyed on the per-student group. A new clear_student_cache( $student_id ) public method bumps the prefix and orphans every cached grade for that student in one call. Hooked into llms_mark_complete, llms_mark_incomplete, lifterlms_quiz_completed, and llms_user_enrollment_deleted.

4. LLMS_Product::has_active_subscriptions()

Cache key in group llms_product_subscriptions_count was never cleared when an order's status transitioned into or out of the llms-active / llms-pending-cancel / llms-on-hold bucket the query counts on, or when the order was deleted. Result: "this product has active subscriptions" checks (used in admin-side guards and reporting) stayed true forever after the underlying orders were cancelled / refunded / expired.

Added a static flush_active_subscriptions_cache( $product_id ) helper, hooked into transition_post_status and before_delete_post filtered to the llms_order post type.

Notes

  • No reliance on wp_cache_flush_group() anywhere in this PR. That function is not supported by several persistent backends (older Redis Object Cache versions, Memcached) — using it for invalidation is the bug at the heart of PMPro #3419. LifterLMS already has zero callsites of wp_cache_flush_group() and this PR keeps it that way: invalidation is either explicit wp_cache_delete() or the existing LLMS_Cache_Helper prefix-bump helper.
  • The membership cache group was also moved off the empty default group, which incidentally fixes a latent multisite cross-blog leakage risk.
  • The grade cache reorganization also lays the groundwork for a "wipe caches for user X" hook if anyone needs it later — LLMS_Grades::clear_student_cache() does it cheaply.

Reproduction (before the fix)

All four were reproduced on a clean LifterLMS install with the Redis Object Cache plugin (drop-in wp-content/object-cache.php active) backed by a local redis-server:

Case Action Cached read Fresh read
Forms flip _llms_form_is_core=no on form #36 [36, 38, 40] [37, 38, 40]
Membership drop _llms_restricted_levels on a page [394] []
Grades inject 42.5 into cache, simulate quiz attempt 42.5 NULL
Subscriptions cancel all 11 active orders for product #89 true false

After applying this PR, the cached read matches the fresh read in every case.

How to test

  1. Install / activate the Redis Object Cache plugin and enable the drop-in (wp redis enable).
  2. Verify with wp eval 'var_dump( wp_using_ext_object_cache() );' (must be true).
  3. For each case above:
    • Trigger the read path to warm the cache.
    • Mutate the underlying data via the action listed.
    • Re-trigger the read path and compare to a use_cache=false read.

Changelog entry

##### Bug Fixes

+ Fix stale object cache for core forms, membership associated posts, student grades, and product active subscription count. The read paths in `LLMS_Forms::get_core_forms()`, `LLMS_Membership::query_associated_posts()`, `LLMS_Grades::get_grade()`, and `LLMS_Product::has_active_subscriptions()` had no matching cache invalidation on writes; on persistent object-cache hosts (Redis, Memcached) the cached values could become indefinitely stale.

🤖 Generated with Claude Code

LifterLMS has four wp_cache_get/set pairs whose read paths have no
matching delete on the write paths. On hosts with a persistent object
cache (Redis, Memcached, APCu) the cached values become indefinitely
stale until the cache is manually flushed.

Cases fixed:

1. LLMS_Forms::get_core_forms() — `llms_core_forms` / `llms_core_form_ids`
   never cleared when a form is saved, trashed, untrashed, deleted, or
   when `_llms_form_is_core` / `_llms_form_location` meta changes.

2. LLMS_Membership::query_associated_posts() — `membership_{id}_associated_{type}`
   never cleared when restriction/availability metadata changes on the
   associated post. Also: the entry was written to the default (empty)
   cache group, polluting WordPress's hot path and risking cross-blog
   leakage on multisite. Moved to a named per-membership group using
   the existing LLMS_Cache_Helper prefix-bump pattern so a single
   `invalidate_group()` call orphans every cached post type for that
   membership in one shot.

3. LLMS_Grades::get_grade() — `student_{id}` group never cleared when
   a quiz attempt is completed, a lesson/section/course is marked
   complete or incomplete, or an enrollment is removed. Switched to
   the LLMS_Cache_Helper prefix-bump pattern; a single
   `clear_student_cache()` call invalidates every cached grade for
   that student.

4. LLMS_Product::has_active_subscriptions() — `llms_product_subscriptions_count`
   never cleared when an order's status transitions in or out of the
   `llms-active` / `llms-pending-cancel` / `llms-on-hold` bucket, or
   when the order is deleted.

No reliance on `wp_cache_flush_group()` (unsupported by several
backends including older versions of the Redis Object Cache plugin);
either explicit `wp_cache_delete()` or the existing prefix-bump
helper is used throughout.
@brianhogg brianhogg moved this to Awaiting Review in Development May 11, 2026
@brianhogg brianhogg added this to the Future milestone May 11, 2026
Three existing tests asserted the bug (cached read keeps returning the
pre-change value after the underlying data changes) as the expected
behavior. With the cache invalidation hooks added in the previous
commit, the cached read and the fresh read now agree after a state
change. Tests updated to match the corrected contract:

- LLMS_Test_Model_LLMS_Product::test_has_active_subscriptions_use_cache
  After `set( 'status', 'llms-cancelled' )` and `set( 'status', 'llms-active' )`,
  the `transition_post_status` hook now busts the cache so
  `has_active_subscriptions( true )` reflects reality.

- LLMS_Test_Grades and LLMS_Test_Student
  After `take_quiz()` records an attempt, the `lifterlms_quiz_completed`
  hook busts the per-student grade cache so `get_grade( ..., true )`
  returns the new grade instead of the stale null.

Also adds `.changelogs/fix_object-cache-invalidation.yml` per the
LifterLMS contributor workflow (`npm run dev changelog add`).
@flintfromthebasement
flintfromthebasement changed the base branch from trunk to dev May 11, 2026 19:22
@flintfromthebasement

Copy link
Copy Markdown
Contributor Author

Thanks @brianhogg — you're right that test_has_active_subscriptions_use_cache was the canary for this. That test (and two others — LLMS_Test_Grades::test_get_grade line 143 and LLMS_Test_Student::test_get_grade line 412) was effectively asserting the stale-cache behavior as the expected contract:

$order_recurring->set( 'status', 'llms-cancelled' );
// Use cache, I expect an active subscription.
$this->assertTrue( $product->has_active_subscriptions( true ), $order_recurring->get( 'status' ) );

The "use cache, I expect an active subscription" assertion is what this PR makes false — and I'd argue that's the right move. set( 'status', 'llms-cancelled' ) triggers transition_post_status, which the new hook listens for. After that point, callers reading has_active_subscriptions( true ) and expecting "still active because we cached it five lines ago" is the bug the customer-facing reports were hitting on object-cached hosts.

Just pushed 90b447a with the test updates and the .changelogs/fix_object-cache-invalidation.yml entry. Also retargeted the PR base from trunk to dev — apologies, I read the contributing.md text but missed the checklist line in the PR template that flags both dev and the changelog file. Won't make that mistake again.

The three tests now assert that cached and fresh reads agree after a state change, which matches the new contract. Happy to add explicit "invalidation triggered correctly" tests as well if you'd rather see those as separate assertions instead of folded into the existing tests — just say the word.

Singleton constructors run exactly once per process. WordPress's
PHPUnit framework snapshots hooks at bootstrap and calls
restore_hooks() between tests; hooks registered later (e.g. inside
LLMS_Grades::__construct, which runs the first time something asks
for llms()->grades()) get wiped after the first test and never
re-register — the singleton's $instance is already set, so the
constructor doesn't run again.

In production this never fires (one boot, hooks stay registered).
In the test suite it caused test_get_grade to fail when any earlier
test had already instantiated LLMS_Grades.

Match the pattern already used by LLMS_Membership and LLMS_Product
in this PR: keep the constructor minimal, expose a static
init_grade_cache_hooks() that LifterLMS::__construct() wires at
init=0. Static callbacks delegate to instance methods so the
public API is unchanged.
@flintfromthebasement

Copy link
Copy Markdown
Contributor Author

Update: I got PHPUnit running locally and reproduced the original test_has_active_subscriptions_use_cache failure, then surfaced a second more interesting issue while debugging.

Initial fix shipped (90b447a): retargeted the PR base to dev, added the .changelogs/fix_object-cache-invalidation.yml, and updated three tests that were asserting the stale-cache behavior as expected.

Then ran the broader suite and hit a different failureLLMS_Test_Student::test_get_grade would pass in isolation but fail when any earlier test had touched grades. Root cause was my own bug, not the tests:

  • I registered the grade cache hooks inside LLMS_Grades::__construct().
  • WP's PHPUnit framework calls restore_hooks() between test methods, which wipes hooks added after bootstrap.
  • The singleton's $instance is already set by then, so the constructor never runs again — and the hooks never re-register.
  • In production this is a non-issue (one boot, hooks stay registered). In tests it meant the cache invalidation hooks only fired during the first test that hit LLMS_Grades::instance() and silently stopped after that.

Fixed in 71016ac: moved the hooks into a static LLMS_Grades::init_grade_cache_hooks() registered from LifterLMS::__construct() at init=0 — same pattern I already used for LLMS_Membership::init_associated_posts_cache_hooks and LLMS_Product::init_active_subscriptions_cache_hooks for the same reason.

Local test results after both fixes:

Suite Result
test_(get_grade|has_active_subscriptions) (5 consecutive runs) 6/6 tests, 130 assertions, all pass
LLMS_Test_LLMS_Product (full class) 19/19
LLMS_Test_LLMS_Membership (full class) 17/17
LLMS_Test_Student (full class) 10/10, 350 assertions
LLMS_Test_Grades (full class) 4/4, 104 assertions
LLMS_Test_Forms (full class) 82/82, 780 assertions (1 unrelated PHPUnit deprecation warning)

Full suite has 12 errors + 2 failures, but I verified those are all pre-existing on this branch's parent commit (stashed my changes and reran the failing tests — same errors). Most are flaky processor tests or WP-version-conditional skips.

Notes for the reviewer pass:

  • clear_student_cache_static / clear_student_cache_from_mark_static are intentionally thin static wrappers that delegate to the instance methods so the public API is unchanged and the same invalidation can be called manually (LLMS_Grades::instance()->clear_student_cache( $user_id )) by anyone who wants to.
  • The [version] placeholders in DocBlocks will get replaced by the release pipeline per docs/documentation-standards.md.

@brianhogg brianhogg mentioned this pull request Jun 19, 2026
4 tasks
@brianhogg brianhogg modified the milestones: Future, 10.1 Jun 19, 2026
@brianhogg brianhogg moved this from Awaiting Review to Review in Progress in Development Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Review in Progress

Development

Successfully merging this pull request may close these issues.

3 participants