Conversation
… ID list For scope=members the potential-invitees query spliced a LEFT JOIN wp_usermeta ... IS NULL anti-join into BP_User_Query::$uid_clauses to honour _bp_nouveau_restrict_invites_to_friends. On a site with 11,820 users that cost 13.6s to return 20 rows while excluding a single user, and uid_clauses is reused by the count query so the cost was paid twice. Resolve a lone NOT EXISTS clause to an excluded ID list via an indexed meta_key lookup instead, matching the idiom BP_User_Query already uses for meta_key. Any other meta_query still falls through to WP_Meta_Query, so the Pro groups_get_group_potential_invites_requests_args filter is unaffected. The scope === 'members' guard is unchanged, leaving REST behaviour as is. Measured on qa-bbreleased01 (11,820 users): 13,645ms -> 674ms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review hardening on top of the excluded-ID list optimization: - Fall through to WP_Meta_Query for non-array meta_query values instead of fataling on count() under PHP 8, and for clauses the fast path cannot replicate (array keys, compare_key other than '='). - Bound the excluded-ID lookup with SELECT DISTINCT and a filterable limit (bb_nouveau_group_invites_excluded_ids_limit, default 5000); when exceeded, fall back to the WP_Meta_Query anti-join so oversized NOT IN lists can never break the query on mass opt-in sites. - Complete the build_meta_query() docblock. Add BP_Tests_BP_Nouveau_Group_Invite_Query covering: lone NOT EXISTS excludes exactly the opted-in users, fast path returns the same ID set as the WP_Meta_Query path, non-array meta_query does not fatal, and the over-limit fallback keeps identical exclusions.
Adversarial review follow-up on the excluded-ID fast path:
- Fall through to WP_Meta_Query when the excluded-IDs lookup errors,
so the restrict-invites-to-friends exclusion can never silently
disappear on a failed query.
- Clamp bb_nouveau_group_invites_excluded_ids_limit to PHP_INT_MAX - 1
so an "unlimited" filter value cannot overflow into a negative LIMIT.
- Join the WP_Meta_Query fallback on the query's uid column instead of
a hardcoded ID, keeping both paths consistent when BP_User_Query
targets a user_id-keyed table (profile sync disabled + xprofile).
Rename the test file to class-bp-nouveau-group-invite-query.php per
convention and extend it to 7 tests: every test now asserts which SQL
path ran via the usermeta join in uid_clauses, with new coverage for an
empty excluded list, falsy stored meta values, lowercase compare, and
compare_key routing ('=' fast path, 'LIKE' fallback).
- Wrap the vendor/autoload.php requires in the PHPUnit loader and installer with file_exists() checks that fail with a readable "run composer install" message, matching the guarded autoload convention in bp-loader.php. - Require a string compare value before strtoupper() in the invite fast-path guard so an array falls through to WP_Meta_Query instead of a PHP 8 TypeError. - Document that filtering bb_nouveau_group_invites_excluded_ids_limit to 0 forces the WP_Meta_Query fallback rather than removing the cap. - Tests: assert total_users parity between the fast and fallback paths, and add a relation + multi-clause meta_query case that must take the fallback while still honouring the exclusion.
- Cache the excluded-ID list for the restrict-invites meta key in the bb_nouveau_group_invites global cache group (registered in BP_Groups_Component::setup_cache_groups()), with an hour TTL as a backstop for writers that bypass the meta API and fire no hooks. Only successful under-ceiling lookups for the known key are cached, and non-array cache values are treated as a miss. - Invalidate from bp-groups-cache.php on added_user_meta / updated_user_meta / deleted_user_meta, matching both the raw meta key and its bp_get_user_meta_key()-filtered form so every write path (web, REST, WP-CLI) busts the cache. Using the core meta hooks avoids touching the REST account-settings controller, which is synced from the API repo. - Emit the exclusion as chunked ANDed NOT IN clauses (filterable via bb_nouveau_group_invites_excluded_ids_chunk_size, default 5000) and raise the inline ceiling to 50000, now a memory/packet guard rather than a performance cap — only sets beyond it fall back to the WP_Meta_Query anti-join. - Log a WP_DEBUG-gated notice, once per request, when the fallback triggers so slow-invites reports are diagnosable. - Tests: assert the branch taken via the actual usermeta JOIN, cache invalidation on opt-in/opt-out, chunked-vs-single equivalence, and large sets staying on the fast path. - Test harness: report a missing Composer autoloader on STDERR with exit code 1 instead of die().
Benchmarking the previous commit's chunked NOT IN showed it was a regression, not a safeguard, so it is reverted in favour of a single clause. On a 200k-row users table at 50,000 excluded ids the chunked form cost 98.9ms vs 55.2ms for one clause (COUNT: 76.0 vs 36.4), with identical EXPLAIN plans and marginally larger SQL. Its stated rationale did not hold either: 50,000 ids produce ~290KB against a 128MB max_allowed_packet. Worse, the public chunk-size filter exposed a cliff - at <= ~1,000 ids per clause the query trips range_optimizer_max_mem_size and degrades to a full scan, a 15-25x regression, and both forms trip at the same threshold, so chunking bought no headroom. Raise the excluded-ids ceiling from 50,000 to 200,000 and re-document it as a memory backstop rather than a performance boundary: the inline NOT IN gets faster as the excluded set grows (96ms at 0 opt-ins vs 64ms at 50,000) while the WP_Meta_Query fallback is ~3x slower, so crossing the ceiling should be pathological rather than merely large. Cache the over-ceiling verdict briefly under its own key, so a site past the ceiling stops re-running a ~109ms lookup whose result is discarded; a scalar sentinel cannot share the list key because the miss test is an is_array() check. Cache the parsed integer ids instead of the raw string column (~24% smaller payload). The emit path still re-parses, deliberately: that string is interpolated straight into SQL and the value may come from the object cache. Add a best-effort stampede guard around the rebuild. The cache goes cold on every toggle and eight concurrent cold-cache requests each ran the full lookup; the winner now rebuilds while losers re-read once, then proceed rather than blocking the request. Correct comments and docblocks that overstated behaviour: the list is not re-queried per search keystroke, and the cache cannot guarantee a stale exclusion is never served - a writer that bypasses the meta API fires no hook, which is what the TTL bounds. Throttle the WP_DEBUG fallback log per reason so a lookup failure is not silenced by an earlier ceiling warning. In the invalidator, return early on an empty key, memoise the bp_get_user_meta_key() lookup (it ran an apply_filters on every user meta write on the site), and clear the new sentinel. Replace the two chunking tests with assertions that hold their weight: the exclusion is exactly one NOT IN clause containing exactly the opted-in ids (contents, not clause count - a count-only assertion passed against deliberately broken emission), and a forced $wpdb->last_error fails closed, still excludes opted-in members, and is not cached. Both were mutation-tested: removing the guard, or emitting a duplicate clause, makes them fail.
External PR review raised two MEDIUMs against the previous commit, both
confirmed against the code here.
Restrict the fast path to the invite screen's own meta key. The AJAX
handler runs bp_parse_args( $_POST, ... ) with no whitelist (ajax.php:363),
bp_nouveau_get_group_potential_invites() does not list meta_query among its
defaults so bp_parse_args passes it straight through, and $request is handed
to that function wholesale (ajax.php:494). The server-side overwrite at
functions.php:313 is skipped whenever user_id is truthy. Any member who
passes bp_groups_user_can_send_invites() could therefore supply their own
meta_query and aim the inline lookup at an arbitrary key - session_tokens,
say, which nearly every user holds - forcing an uncached
SELECT DISTINCT user_id ... LIMIT N+1 on every request. The meta-key oracle
itself is pre-existing (the legacy path ran the same clause through
WP_Meta_Query); the uncached lookup is what this branch added. Production
only ever sends the one key, so gating on it loses nothing and every other
key falls through to WP_Meta_Query exactly as before. This also removes the
$cacheable flag, which is now always true inside the guard.
Fence the rebuild against a lost update. A rebuild that began before a
member toggled the setting could finish afterwards and write its pre-toggle
snapshot over the freshly invalidated state, pinning a stale exclusion for
the full TTL. The cache keys now carry an incrementor captured before the
lookup:
bb_restrict_invites_ids_{incrementor}
bb_restrict_invites_over_{limit}_{incrementor}
bb_restrict_invites_lock_{incrementor}
Invalidation calls bp_core_reset_incrementor(), so a racing rebuild writes
to a retired key no reader will consult, and all three keys retire together.
Capturing before the read is what makes this safe: bp_core_set_incremented_cache()
mints the incrementor at write time, so the stale write would land in the
new namespace and still be served.
Also in this round:
- Re-check count( $excluded_ids ) <= $limit on the emit path, so a list
cached under a higher ceiling honours a lowered one immediately instead
of after the TTL. The check was lost in the previous rewrite.
- Key the over-ceiling verdict by $limit, so a caller filtering the ceiling
down cannot force the fallback on callers using a different ceiling. On
multisite this also stops one site's per-site filter poisoning the
network-global flag.
- Lower the default ceiling from 200000 to 100000. At 200k the emitted
NOT IN literal approaches 1.4 MB, which a conservative managed host's
1 MB max_allowed_packet would reject - and a failed main query means an
empty invite list, not a slow one.
- Resolve bp_get_user_meta_key() per call again instead of memoising it.
It is a public filter that may be registered late or vary per blog; a
frozen value would silently stop matching and leave a stale exclusion
cached. The memo saved roughly one microsecond.
- Correct the rebuild-lock comment: it reduces but does not bound a
stampede, because a loser re-reads once and then queries rather than
blocking. Measured on the live site, eight concurrent cold-cache requests
issued between one and seven lookups depending on timing.
Tests go from 11 to 17 (80 assertions), green on single-site and multisite.
The caching layer previously had no coverage at all - deleting the
wp_cache_set passed every test. Each addition was mutation-tested:
delete the list wp_cache_set -> cached_and_reused fails
relax the key gate -> foreign_meta_key fails
unversioned cache key -> invalidation fails
loser also releases the lock -> rebuild_lock fails
invalidator ignores filtered -> filtered_meta_key fails
cache the truncated list -> over_ceiling fails
Behaviour change to note in the PR description: the fast path never builds
a WP_Meta_Query, so WordPress's public get_meta_sql filter no longer fires
for the standard invite exclusion. Every other meta_query shape, and this
one on the fallback path, still goes through WP_Meta_Query and still fires
it.
…parity test_lookup_failure_fails_closed() asserted against bb_restrict_invites_user_ids, a cache key no production code writes: the keys became bb_restrict_invites_ids_<incrementor> when the cache gained its versioned-key fence, and this assertion was left behind. It therefore always passed and could never have caught a failed lookup being cached. Verified by mutation - making the lookup cache its result unconditionally still passed before this change, and fails after it. The test's other assertions (fallback path taken, exclusion still applied) were unaffected, so no production behaviour was masked. Extend test_invalidation_matches_a_filtered_meta_key() to assert that the fast path and the WP_Meta_Query fallback return the same users when bp_get_user_meta_key is filtered, and document why that is the assertion being made. The reader hardcodes the raw meta key while writers pass it through that filter, so on a filtered site rows are stored under one key and queried under another and the exclusion does not apply. That predates this optimisation and is identical on the legacy path, so the invariant worth pinning here is that the two paths agree - not that the exclusion works. Correcting the asymmetry changes member-visible behaviour and would also have to widen the fast path's meta-key gate, so it belongs in its own ticket.
…_query_path Let a negative ceiling force the WP_Meta_Query path The excluded-ids ceiling filter documented `0` as the way to force the legacy path, but a zero ceiling only does that while somebody actually holds the meta. With nobody opted in there is nothing to exclude, so the fast path returns early without ever constructing a WP_Meta_Query - and the clause third-party code filters `get_meta_sql` for is never built at all. Confirmed by instrumenting get_meta_sql and attributing each firing to its clause: on the fast path it fires twice with zero firings carrying the restrict clause; on the fallback it fires three times with one carrying it. So a site had no way to guarantee the clause is always built. Any negative ceiling now disables the fast path unconditionally. It is handled as an immediate over-ceiling verdict, so no lookup runs and no cache entry is read or written on that path. `0` keeps its existing meaning, and the filter docblock now states the difference. The WP_DEBUG diagnostic gained a matching `disabled` reason instead of reporting a nonsensical "more than -1 users". The regression test was written before the fix and failed against the old code. 2 — classes.php + test_list_over_the_cacheable_size_is_used_but_not_stored Add a separate, filterable limit for the cached id list The inline ceiling is sized against max_allowed_packet - 100k ids is ~0.7MB of SQL. The same list is also handed to wp_cache_set(), and as a serialised PHP array those 100k ids are ~1.6MB, which exceeds memcached's 1MB default item size. Such a set is dropped silently, so a site past that point re-pays the lookup on every request while believing it had cached the result. The two limits are unrelated and should not share one number. Added bb_nouveau_group_invites_cacheable_ids_limit, defaulting to the inline ceiling so it imposes no second ceiling and behaviour is unchanged on every backend. A list larger than it is still used inline for the request, it is just not stored. Operators on a memcached backend with a small item size can lower it. Deliberately a filter rather than a constant: the limit is a property of the object cache backend, not of the data. Verified on Redis Object Cache Pro that a 100k-id set stores and reads back intact, so a hardcoded threshold would have disabled caching for Redis sites that can cache perfectly well. 3 — bp-groups-cache.php Skip the meta-key filter when none is registered bb_groups_clear_restrict_invites_cache() runs on added/updated/deleted_user_meta, so it fires for every user meta write on the site. It compared the raw key and then, for every non-matching key, called bp_get_user_meta_key() to compare the filtered one - measured at four apply_filters() calls for four unrelated writes, on a default install where no filter is registered and the key cannot differ. The filtered comparison is kept, since it is what catches writers going through bp_update_user_meta(), but it is now guarded by has_filter(), which is an array lookup rather than a filter dispatch. Re-measured: zero applications with no filter registered, still consulted when one is, and a restrict-key write still resets the incrementor.
* release: (87 commits) update: grunt release update: grunt update: grunt string-replace version bump Added change log grunt pulled api changes from repo PROD-10323 - Address review findings on the visibility gate and its test harness PROD-10323 - Address review nits on the visibility helper and group-cache docblock PROD-10323 PROD-10323 - Correct the no-migration rationale on the visibility gate PROD-10323 - Enforce the visibility lock on the wp-admin profile save PROD-10323 - Fix activation-key clobber and use the canonical visibility lock at signup [PROD-10406] Add lock-only bb_placeholder_feature_in_plan filter so plugins can apply module-level plan gating to placeholder cards PROD-10323 - Honor the display-name-format lock at signup activation and assert stored state in its test PROD-10323 - Add signup-activation coverage for enforced field visibility PROD-10323 - Enforce field visibility locks at signup activation PROD-10323 - Honor profile field visibility locks when saving member settings update: grunt release grunt ... # Conflicts: # tests/phpunit/includes/install.php # tests/phpunit/includes/loader.php
…rocess static $logged_fallbacks dedupes the WP_DEBUG "fast path skipped" notice so the invite list does not log it once per query - it re-queries on submitted searches and on every page of results. But a function static is scoped to the process, not the request. Under PHP-FPM and mod_php that is the same thing, because the executor state is reset between requests; under a worker SAPI (Swoole, RoadRunner, FrankenPHP worker mode) the process is reused, so the array would survive and silence the notice for every later request that worker handled - exactly when an operator is trying to find out why invites got slow again. The array is now keyed on REQUEST_TIME_FLOAT and cleared when it changes, so the cadence is one line per reason per request on every SAPI. The value is only compared with itself to detect a request boundary and is cast to float, which is what makes reading it safe - no sniff suppression needed.
Contributor
|
@claude please review from scratch ultracode |
|
Claude finished @jitendrabanjara1991's task in 3m 35s —— View job Review complete ✅Request: "please review from scratch ultracode" One clarification first: I can't launch the actual multi-agent cloud Todo
SummaryThis PR replaces the Correctness — verified, no bugs found:
Minor, non-blocking observations:
Style/standards: Naming, Verdict: No bugs found. The logic is sound, the fallback is fail-closed, and the test suite is unusually thorough for a change of this size — it exercises the stampede lock, incrementor-race, filtered-key, ceiling, and cacheable-size edge cases directly rather than just the happy path. |
chetansatasiya
requested changes
Sep 23, 2026
chetansatasiya
left a comment
chetansatasiya
left a comment
Contributor
There was a problem hiding this comment.
Need requested changes
chetansatasiya
approved these changes
Sep 24, 2026
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
3 participants
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.
Jira Issue:
https://buddyboss.atlassian.net/browse/PROD-10199
General Note
Keep all conversations related to this PR in the associated Jira issue(s). Do NOT add comment on this PR or edit this PR’s description.
Notes to Developer
Notes to Reviewer