design-system-mcp: Improve concurrency handling for data fetching#78311
Open
aduth wants to merge 4 commits into
Open
design-system-mcp: Improve concurrency handling for data fetching#78311aduth wants to merge 4 commits into
aduth wants to merge 4 commits into
Conversation
aduth
added
[Type] Performance
|
Size Change: 0 B Total Size: 7.97 MB ℹ️ View Unchanged
|
ciampo
reviewed
May 15, 2026
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
I understand that this cache will exist for the full lifetime of the process, which means that if the upstream manifest or tokens file changes upstream, the MCP server holds stale data until restart.
Not in scope for this PR (and potentially not even necessary at this point), but we may want to acknowledge this limitation and add a TODO about potential improvements (ie a TTL or invalidation based on content hash?)
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Member
Author
On a combination of these feedback points, I might explore something like a |
aduth
force-pushed
the
update/design-system-mcp-multiple-names
branch
from
5207ab9 to
d851ecb
Compare
aduth
force-pushed
the
update/ds-mcp-concurrent-fetch
branch
from
0d525fe to
c037f4f
Compare
Member
Author
|
The updated |
aduth
force-pushed
the
update/ds-mcp-concurrent-fetch
branch
from
a1b4f0e to
0790224
Compare
ciampo
reviewed
May 15, 2026
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Nice!
I also realized, should we also make changes as originally suggested in #78185 (comment) , refactoring that await call in a for loop to something like const details = await Promise.all(...) ?
Comment on lines
+33
to
+65
| function withTTL< T >( | ||
| fetcher: () => Promise< T >, | ||
| ttlMs: number = HOUR_IN_MS | ||
| ): CachedFetcher< T > { | ||
| let cached: Promise< T > | null = null; | ||
| let expiresAt = 0; | ||
|
|
||
| return Object.assign( | ||
| () => { | ||
| if ( ! cached || Date.now() > expiresAt ) { | ||
| cached = ( async () => { | ||
| try { | ||
| return await fetcher(); | ||
| } catch ( error ) { | ||
| cached = null; | ||
| expiresAt = 0; | ||
| throw error; | ||
| } | ||
| } )(); | ||
|
|
||
| expiresAt = Date.now() + ttlMs; | ||
| } | ||
|
|
||
| return cached; | ||
| }, | ||
| { | ||
| reset: () => { | ||
| cached = null; | ||
| expiresAt = 0; | ||
| }, | ||
| } | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Love this!
Although AI-assisted review flagged a potential (albeit rare) edge condition: when the TTL fires while a fetch is still in flght (a 1-hour TTL with a >1-hour-stuck fetch, or Date.now() skewing forward), the next caller falls into the cache-miss branch and overwrites cached with a new IIFE-promise. Both IIFEs are now closed over the same cached and expiresAt variables.
If the first IIFE then rejects (after the second has been assigned), its catch handler runs and nukes the second IIFE's cached entry, breaking dedup for anyone arriving between that moment and the second IIFE settling.
The same hazard exists with reset() racing an in-flight IIFE, though that one is "by design" (callers asked for a clean cache).
Claude suggests this as a potential fix
make the catch identity-check before clearing:
() => {
if ( ! cached || Date.now() > expiresAt ) {
const promise: Promise< T > = ( async () => {
try {
return await fetcher();
} catch ( error ) {
if ( cached === promise ) {
cached = null;
expiresAt = 0;
}
throw error;
}
} )();
cached = promise;
expiresAt = Date.now() + ttlMs;
}
return cached;
}an equivalent shape using an outer let works too
Again, extremely unlikely but potentially possible, especially if the TTL will every become shorter
| } | ||
| } )(); | ||
|
|
||
| expiresAt = Date.now() + ttlMs; |
Contributor
There was a problem hiding this comment.
Why do we start the timer before the fetch resolves? This comes mostly from a place of curiosity, rather than flagging a bug with the implementation.
Maybe worth adding a short code comment about it?
Comment on lines
+43
to
+51
| cached = ( async () => { | ||
| try { | ||
| return await fetcher(); | ||
| } catch ( error ) { | ||
| cached = null; | ||
| expiresAt = 0; | ||
| throw error; | ||
| } | ||
| } )(); |
Contributor
There was a problem hiding this comment.
Same comment as before re. the readability of an IIFE. Could be potentially rewritten to
async function load(): Promise< T > {
try {
return await fetcher();
} catch ( error ) {
cached = null;
expiresAt = 0;
throw error;
}
}
cached = load();
Contributor
There was a problem hiding this comment.
Should we add tests targeting the TTL logic? In particular
- Re-fetches after TTL expires. We could drive Date.now() forward with jest.useFakeTimers().setSystemTime(...), call once, advance past HOUR_IN_MS, call again, expect two fetch calls.
- Does not re-fetch within TTL. Same setup, advance by less than HOUR_IN_MS, expect one fetch call. (Partially covered by the existing "should cache" test, but it doesn't pin the boundary.)
Another potentially missing test is to check that wwo concurrent callers share the same rejection. Right now the "should evict failed fetches" test fires the failure, awaits its rejection, then makes a second call, but it doesn't check that two concurrent callers awaiting the same in-flight failure both see it.
|
Flaky tests detected in 0790224. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25926748791
|
Member
Author
That should be covered by what's implemented here in |
Contributor
Ah, I must have reviewed a subset of the code changes by mistake via the GitHub UI 😅 Apologies |
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
2 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.
What?
Updates
@wordpress/design-system-mcpinternal data fetching to improve data fetching concurrency handling for retrieving details of multiple components simultaneously.Split from #78185
Why?
Originally raised by @ciampo in #78185 (comment), the original logic for getting component details of multiple components was not parallelized and relied on internal caching of component manifest fetching. Trying to parallelize this revealed issues with the internal data fetching for components manifests and tokens which could cause multiple fetches to happen in parallel.
How?
The updated logic caches the promise itself, meaning that repeated calls will not trigger a new fetch if one is in progress. And if the promise has already resolved, then it will use the resolved value.
Testing Instructions
Repeating testing instructions from #78185
Verify unit tests pass:
npm run test:unit packages/design-system-mcp/src/test/data.tsUse of AI Tools
Implemented using Cursor with Claude Opus 4.7. Several rounds of human-prompted iteration on the implementation upon manual review of code.