fix: cache regression - #4269
Merged
Merged
Conversation
Contributor
|
Your PR title doesn't contain a Jira issue key. Consider adding it for better traceability. Example:
Projects:
Please add a Jira issue key to your PR title. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in the queryMembersAdvanced caching flow so countOnly member list requests can hit Redis again (and full-result queries can warm the count cache), while also reducing count-query cost in Postgres.
Changes:
- Fixes
countOnlycount-cache lookup and aligns count/full queries on a shared cache key (separate Redis namespaces). - Warms the count cache after full queries and hardens
setCountagainst Redis write failures. - Optimizes count SQL by avoiding unnecessary org CTE/join and using
COUNT(*)when the aggregate join is guaranteed unique.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| services/libs/data-access-layer/src/members/queryCache.ts | Removes countOnly from key generation and adds error handling to setCount (cache write hardening). |
| services/libs/data-access-layer/src/members/queryBuilder.ts | Optimizes count expression to use COUNT(*) when withAggregates guarantees no duplicates. |
| services/libs/data-access-layer/src/members/base.ts | Fixes cache read/write flow for count-only requests, normalizes cache key inputs, and cross-populates count cache after full queries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ulemons
force-pushed
the
fix/regression-member-cache
branch
from
June 26, 2026 10:42
81c72ad to
63fab1d
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Comment on lines
205
to
209
| limit, | ||
| offset, | ||
| orderBy, | ||
| search, | ||
| search: search?.trim() || null, | ||
| segmentId, |
Comment on lines
198
to
210
| // Build cache key — countOnly is excluded so count and full-result caches share the same key. | ||
| // Normalize empty search to null so "" and null produce the same key (buildSearchCTE treats them identically). | ||
| const cacheKey = cache.buildCacheKey({ | ||
| countOnly, | ||
| fields, | ||
| filter, | ||
| include, | ||
| includeAllAttributes, | ||
| limit, | ||
| offset, | ||
| orderBy, | ||
| search, | ||
| search: search?.trim() || null, | ||
| segmentId, | ||
| }) |
| offset, | ||
| orderBy, | ||
| search, | ||
| search: search?.trim() || null, |
ulemons
force-pushed
the
fix/regression-member-cache
branch
from
June 26, 2026 12:03
e44f7b6 to
d626a1d
Compare
Comment on lines
246
to
254
| if (countOnly && cachedCount !== null) { | ||
| refreshCountCacheInBackground(bgQx, redis, cacheKey, { | ||
| refreshCountCacheInBackground(bgQx, redis, countCacheKey, { | ||
| filter, | ||
| search, | ||
| search: normalizedSearch, | ||
| segmentId, | ||
| include, | ||
| includeAllAttributes, | ||
| attributeSettings, | ||
| }) |
Comment on lines
571
to
575
| try { | ||
| log.info(`Refreshing members advanced query cache in background: ${cacheKey}`) | ||
| await executeQuery(qx, redis, cacheKey, params) | ||
| log.info(`Members advanced query cache refreshed in background: ${cacheKey}`) | ||
| } catch (error) { |
Comment on lines
586
to
590
| try { | ||
| log.info(`Refreshing members advanced count cache in background: ${cacheKey}`) | ||
| await executeQuery(qx, redis, cacheKey, { ...params, countOnly: true }) | ||
| log.info(`Members advanced count cache refreshed in background: ${cacheKey}`) | ||
| } catch (error) { |
Comment on lines
+249
to
+252
| log.info( | ||
| { countCacheKey, segmentId, search: normalizedSearch }, | ||
| 'Members advanced count cache hit — returning cached count', | ||
| ) |
Comment on lines
+30
to
+48
| // Returns true if lock was acquired (no other refresh in progress for this key). | ||
| // Uses a cryptographically random token to distinguish "we set it" from "already existed". | ||
| // TTL ensures the lock auto-expires if the refresh crashes without releasing it. | ||
| async tryAcquireRefreshLock(cacheKey: string, ttlSeconds = 90): Promise<boolean> { | ||
| try { | ||
| const token = randomBytes(16).toString('hex') | ||
| const stored = await this.lockCache.setIfNotExistsOrGet(cacheKey, token, ttlSeconds) | ||
| return stored === token | ||
| } catch { | ||
| return true // fail open: if Redis is down, let the refresh proceed | ||
| } | ||
| } | ||
|
|
||
| async releaseRefreshLock(cacheKey: string): Promise<void> { | ||
| try { | ||
| await this.lockCache.delete(cacheKey) | ||
| } catch { | ||
| // best effort | ||
| } |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
ulemons
force-pushed
the
fix/regression-member-cache
branch
from
June 30, 2026 07:22
2e741fb to
55db609
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 55db609. Configure here.
| cache.set(cacheKey, emptyResult, 21600), | ||
| cachedCount === null ? cache.setCount(countCacheKey, count, 21600) : Promise.resolve(), | ||
| ]) | ||
| return emptyResult |
There was a problem hiding this comment.
Stale count skips recount
Medium Severity
On a full queryMembersAdvanced cache miss, executeQuery can reuse a cached total from members-count and skip the count SQL while still running a fresh main query. After membership changes without count invalidation, the UI can show newly fetched rows (or an empty first page) with an outdated count, and that total is written back to both caches.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 55db609. Configure here.
skwowet
pushed a commit
that referenced
this pull request
Jul 3, 2026
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org> Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
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
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
Fixes multiple cache bugs in
queryMembersAdvancedthat were causing every member list count request to bypass Redis and hit Postgres directly. Also removes an unnecessarymember_orgsCTE from the count query and replacesCOUNT(DISTINCT m.id)withCOUNT(*)where safe.Changes
Fix inverted ternary on
cachedCount(base.ts) — since commit83bc45a18(Jan 30),cachedCountwas set tonullwhenevercountOnly=true, making the checkif (countOnly && cachedCount !== null)always false. Every count-only request was a DB hit. Restored the correct condition:countOnly ? await cache.getCount(cacheKey) : null.Remove
countOnlyfrom cache key (queryCache.ts,base.ts) — count and full-result queries for the same params were hashed to different keys, so a full query never warmed the count cache and vice versa. Since the two caches live in separate Redis namespaces (members-advanced/members-count), the same key string is safe for both.Normalize
search: "" → nullin cache key (base.ts) —buildSearchCTEtreats""andnullidentically (both produce an empty CTE), but""was not filtered by thenull/undefinedcheck inbuildCacheKey, creating duplicate cache entries for the same query.Cross-populate count cache after full query (
base.ts) — acountOnly=falsequery now also writes tomembers-countviaPromise.all, so a subsequentcountOnly=truerequest with the same params hits Redis instead of Postgres.Remove unnecessary
member_orgsCTE from count query (base.ts) —buildCountQuerywas receivingincludeMemberOrgs: include.memberOrganizations(alwaystruefor the member list endpoint), causing every count query to run a fullGROUP BYscan onmemberOrganizations. The count never selects org data;filterHasMoinsidebuildCountQueryalready adds the join when the filter referencesmo.*.COUNT(*)instead ofCOUNT(DISTINCT m.id)whenwithAggregates=true(queryBuilder.ts) — the join is onmemberSegmentsAgg_pkey(unique onmemberId + segmentId), so duplicates are impossible andDISTINCTis dead work.Add error handling to
setCount(queryCache.ts) —setCounthad no try/catch unlikeset, so a Redis failure during the newPromise.allwould propagate and return a 500 even though data was already fetched successfully.Type of change
Note
Medium Risk
Changes core member list query, caching, and count SQL used on every list view; wrong count keys or
COUNT(*)assumptions could skew totals or serve stale counts until TTL/invalidation.Overview
Fixes
queryMembersAdvancedcaching so member list counts and pages actually hit Redis instead of Postgres on every request.The count-only path now reads
getCountwhencountOnlyis true (a prior inverted ternary always skipped the count cache). Separate cache keys separate full page results (pagination, fields,include) from totals (filter,search,segmentIdonly); full queries also write the count key, and paginated list loads skipCOUNT(*)when the total is already cached. Search is normalized (trim, lowercase,""→null) so equivalent queries share one key.Query and refresh behavior: count queries no longer force the
member_orgsCTE unless filters needmo.*;COUNT(*)replacesCOUNT(DISTINCT m.id)when segment aggregates apply and enrichments are not in the filter;memberEnrichmentsjoins are omitted unless needed. Background refresh usesoptionsBgQx(no request transaction), Redis refresh locks, no background refresh on count cache hits, and refresh on sync failure after a miss.MemberQueryCachegains safer count get/set, lock invalidation, andcountOnlyremoved from the result key hash.Reviewed by Cursor Bugbot for commit 55db609. Bugbot is set up for automated code reviews on this repo. Configure here.