fix: test concurrent cache-miss#4340
Draft
ulemons wants to merge 6 commits into
Draft
Conversation
Signed-off-by: Umberto Sgueglia <usgueglia@contractor.linuxfoundation.org>
ulemons
force-pushed
the
fix/debuging-query-members
branch
from
ff626b1 to
7c5a0bb
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a Redis-based “single-flight” helper and integrates it into the members advanced query cache-miss/refresh paths to prevent concurrent requests from stampeding the database on cold keys.
Changes:
- Added
withSingleFlighthelper in@crowd/redisto coordinate concurrent callers behind a lock while polling cache. - Updated
queryMembersAdvancedto single-flight cache misses and align background refresh locking with the same primitive. - Added a shared cap constant to prevent the activityCount top‑N optimization from silently truncating oversized pages.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/libs/redis/src/singleFlight.ts | New single-flight helper built on existing Redis mutex primitives. |
| services/libs/redis/src/index.ts | Exports the new single-flight helper from the redis lib barrel. |
| services/libs/data-access-layer/src/members/queryCache.ts | Removes the previous refresh-lock helpers from MemberQueryCache. |
| services/libs/data-access-layer/src/members/queryBuilder.ts | Adds a shared top‑N cap constant and guards optimized path against truncation. |
| services/libs/data-access-layer/src/members/base.ts | Uses single-flight on members advanced cache misses and switches background refresh locking to acquireLock/releaseLock. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
688
to
692
| } catch (error) { | ||
| log.warn({ cacheKey, err: error }, `Members advanced ${label} background refresh failed`) | ||
| } finally { | ||
| await cache.releaseRefreshLock(cacheKey) | ||
| await releaseLock(redis, cacheKey, token) | ||
| } |
Comment on lines
+72
to
+76
| } finally { | ||
| if (holdsLock) { | ||
| await releaseLock(client, lockKey, token) | ||
| } | ||
| } |
Comment on lines
+33
to
+36
| const token = generateUUIDv4() | ||
| const deadline = Date.now() + waitTimeoutSeconds * 1000 | ||
| let holdsLock = false | ||
|
|
Comment on lines
27
to
31
| this.lockCache = new RedisCache('members-refresh-lock', redis, log) | ||
| } | ||
|
|
||
| // 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 | ||
| } | ||
| } | ||
|
|
||
| buildCacheKey(params: { | ||
| fields?: string[] |
Comment on lines
+54
to
+55
| if (Date.now() >= deadline) { | ||
| break |
| return await compute() | ||
| } finally { | ||
| if (holdsLock) { | ||
| await releaseLock(client, lockKey, token) |
| log.warn({ cacheKey, err: error }, `Members advanced ${label} background refresh failed`) | ||
| } finally { | ||
| await cache.releaseRefreshLock(cacheKey) | ||
| await releaseLock(redis, cacheKey, token) |
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
Comment on lines
+466
to
+470
| segmentsSearchQuery += `AND EXISTS ( | ||
| SELECT 1 FROM segments sp | ||
| WHERE sp."grandparentSlug" = f.slug | ||
| AND sp.id IN (:adminSegments) | ||
| )` |
| return await compute() | ||
| } finally { | ||
| if (holdsLock) { | ||
| await releaseLock(client, lockKey, token) |
| log.warn({ cacheKey, err: error }, `Members advanced ${label} background refresh failed`) | ||
| } finally { | ||
| await cache.releaseRefreshLock(cacheKey) | ||
| await releaseLock(redis, cacheKey, token) |
|
|
||
| if (!fromList?.projects?.length) { | ||
| try { | ||
| selectedProjectGroup.value = await segmentService.getSegmentById(newVal) as ProjectGroup; |
Comment on lines
+305
to
+309
| try { | ||
| const projectGroup = await resolveProjectGroupForSelection( | ||
| projectGroupOrId, | ||
| this.projectGroups.list, | ||
| ); |
Comment on lines
493
to
+495
| s.*, | ||
| COUNT(*) OVER () AS "totalCount", | ||
| JSONB_AGG(JSONB_BUILD_OBJECT( | ||
| 'id', f.project_id, | ||
| 'name', f.project_name, | ||
| 'status', f.project_status, | ||
| 'slug', f.project_slug, | ||
| 'subprojects', f.subprojects | ||
| )) AS projects | ||
| f.project_count AS "projectCount" |
| 'status', sp.status, | ||
| 'slug', sp.slug | ||
| )) AS subprojects | ||
| COUNT(DISTINCT p.id)::int AS project_count |
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
…n load
- Fix router guard forEach async bug: replace forEach with for...of so
auth middleware actually awaits before segment lookup runs, eliminating
the pre-token /segment/{id} request
- Replace resolveProjectGroupForSelection (findSegment call) with flat
/segment/subproject/query filtered by grandparentSlug; store result in
selectedProjectGroupSubprojects state
- activity-timeline and lf-banners now read from selectedProjectGroupSubprojects
instead of walking projectGroup.projects[].subprojects[]
- utils/segments: getSegmentsFromProjectGroup reads subproject IDs from store
- Remove duplicate fetchActivityTypes/fetchActivityChannels from App.vue
tenant watcher; menu.vue watcher already handles these correctly once a
project group is selected
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Joana Maia <jmaia@contractor.linuxfoundation.org>
Comment on lines
+33
to
+35
| const token = generateUUIDv4() | ||
| const deadline = Date.now() + waitTimeoutSeconds * 1000 | ||
| let holdsLock = false |
Comment on lines
+72
to
+76
| } finally { | ||
| if (holdsLock) { | ||
| await releaseLock(client, lockKey, token) | ||
| } | ||
| } |
Comment on lines
689
to
692
| log.warn({ cacheKey, err: error }, `Members advanced ${label} background refresh failed`) | ||
| } finally { | ||
| await cache.releaseRefreshLock(cacheKey) | ||
| await releaseLock(redis, cacheKey, token) | ||
| } |
Comment on lines
+466
to
+470
| segmentsSearchQuery += `AND EXISTS ( | ||
| SELECT 1 FROM segments sp | ||
| WHERE sp."grandparentSlug" = f.slug | ||
| AND sp.id IN (:adminSegments) | ||
| )` |
| @@ -27,27 +27,6 @@ export class MemberQueryCache { | |||
| this.lockCache = new RedisCache('members-refresh-lock', redis, log) | |||
Comment on lines
+468
to
+469
| WHERE sp."grandparentSlug" = f.slug | ||
| AND sp.id IN (:adminSegments) |
Comment on lines
+73
to
+75
| if (holdsLock) { | ||
| await releaseLock(client, lockKey, token) | ||
| } |
| log.warn({ cacheKey, err: error }, `Members advanced ${label} background refresh failed`) | ||
| } finally { | ||
| await cache.releaseRefreshLock(cacheKey) | ||
| await releaseLock(redis, cacheKey, token) |
Comment on lines
+25
to
+27
| if (!selectedProjectGroupSubprojects.value.length) { | ||
| return [projectGroup.id]; | ||
| } |
Comment on lines
319
to
+320
| this.selectedProjectGroup = projectGroup; | ||
| this.fetchSubprojectsForProjectGroup(projectGroup.slug); |
Comment on lines
+285
to
+287
| this.selectedProjectGroupSubprojects = response.rows || []; | ||
| } catch (e) { | ||
| this.selectedProjectGroupSubprojects = []; |
|
|
||
| if (!fromList?.projects?.length) { | ||
| try { | ||
| selectedProjectGroup.value = await segmentService.getSegmentById(newVal) as ProjectGroup; |
Comment on lines
492
to
+495
| SELECT | ||
| s.*, | ||
| COUNT(*) OVER () AS "totalCount", | ||
| JSONB_AGG(JSONB_BUILD_OBJECT( | ||
| 'id', f.project_id, | ||
| 'name', f.project_name, | ||
| 'status', f.project_status, | ||
| 'slug', f.project_slug, | ||
| 'subprojects', f.subprojects | ||
| )) AS projects | ||
| f.project_count AS "projectCount" |
Full /segment/subproject/query returns integrations, mapped-repo flags, and all segment fields per subproject — 222 MB for large project groups. query-lite returns only id, name, url, slug, description which is all the frontend needs for segment filtering and display. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Joana Maia <jmaia@contractor.linuxfoundation.org>
| icon: () => 'layer-group', | ||
| }); | ||
|
|
||
| const displayCount = computed(() => props.projectCount ?? props.projects?.length ?? 0); |
Comment on lines
+72
to
+76
| } finally { | ||
| if (holdsLock) { | ||
| await releaseLock(client, lockKey, token) | ||
| } | ||
| } |
Comment on lines
688
to
692
| } catch (error) { | ||
| log.warn({ cacheKey, err: error }, `Members advanced ${label} background refresh failed`) | ||
| } finally { | ||
| await cache.releaseRefreshLock(cacheKey) | ||
| await releaseLock(redis, cacheKey, token) | ||
| } |
| export * from './instances' | ||
| export * from './rateLimiter' | ||
| export * from './mutex' | ||
| export * from './singleFlight' |
Signed-off-by: Yeganathan S <63534555+skwowet@users.noreply.github.com>
| return await compute() | ||
| } finally { | ||
| if (holdsLock) { | ||
| await releaseLock(client, lockKey, token) |
| log.warn({ cacheKey, err: error }, `Members advanced ${label} background refresh failed`) | ||
| } finally { | ||
| await cache.releaseRefreshLock(cacheKey) | ||
| await releaseLock(redis, cacheKey, token) |
| } | ||
|
|
||
| this.selectedProjectGroup = projectGroup; | ||
| this.fetchSubprojectsForProjectGroup(projectGroup.slug); |
Comment on lines
+53
to
+54
| projects?: Project[]; | ||
| projectCount?: number; |
Comment on lines
+277
to
280
| const subprojects = computed(() => selectedProjectGroupSubprojects.value.reduce((acc, sp) => { | ||
| acc[sp.id] = { id: sp.id, name: sp.name }; | ||
| return acc; | ||
| }, {})); |
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
Changes
Type of change
JIRA ticket