fix: cache regression#4269
Open
ulemons wants to merge 8 commits into
Open
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
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, |
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
e44f7b6 to
d626a1d
Compare
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
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) { |
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ 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 622439d. Configure here.
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>
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/count caching and SQL semantics; wrong count keys or
COUNT(*)assumptions could show stale or incorrect totals, though invalidation and DISTINCT fallback for enrichment filters mitigate this.Overview
Fixes
queryMembersAdvancedcache behavior so member list and count requests actually hit Redis instead of Postgres on every call.The PR corrects count-only cache lookup (was always null when
countOnly=true), splits full-page vs count cache keys (buildCountCacheKeyshares filter/search/segment only), normalizes empty search to null for stable keys, and writes counts when full queries run while skipping redundantCOUNT(*)on later pages when the count is already cached. Count-only hits no longer trigger background refresh (avoids a DB query per request).Query cost reductions: count queries no longer pull in
member_orgsunless filters needmo.*;COUNT(*)replacesCOUNT(DISTINCT m.id)when segment aggregates apply and enrichments aren't in the filter; list queries only joinmemberEnrichmentswhen needed.Operational hardening: Redis refresh locks dedupe background refreshes; structured logging for hits/misses/failures; on sync query failure, schedules background refresh for retry;
optionsBgQxruns background work outside request transactions;setCount/getCountwrapped in try/catch like other cache ops; invalidation clears count and lock namespaces too.Reviewed by Cursor Bugbot for commit 2e741fb. Bugbot is set up for automated code reviews on this repo. Configure here.