Skip to content

fix: prevent infinite recursion in tryCacheIdentity#1215

Closed
jaissica12 wants to merge 1 commit intodevelopmentfrom
fix/no-jira-identity-cache-recursion
Closed

fix: prevent infinite recursion in tryCacheIdentity#1215
jaissica12 wants to merge 1 commit intodevelopmentfrom
fix/no-jira-identity-cache-recursion

Conversation

@jaissica12
Copy link
Contributor

Background

  • When cacheIdentity is enabled, a consumer's identity callback may call identify() again. Without caching, this works fine because the identity response returns asynchronously via a network round-trip. With caching enabled, tryCacheIdentity calls parseIdentityResponse synchronously, which fires the callback in the same call stack, causing infinite recursion and crashing the browser with Maximum call stack size exceeded.

What Has Changed

  • Wrapped the parseIdentityResponse call in tryCacheIdentity with setTimeout(fn, 0) to defer cached identity processing to the next event loop tick, breaking the synchronous recursion

Screenshots/Video

Screenshot 2026-03-16 at 10 26 25 AM Screenshot 2026-03-16 at 10 25 58 AM

Checklist

  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have tested this locally.

Additional Notes

  • {Any additional information or context relevant to this PR}

Reference Issue (For employees only. Ignore if you are an outside contributor)

@jaissica12 jaissica12 requested a review from a team as a code owner March 16, 2026 14:29
@sonarqubecloud
Copy link

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts cached-identity handling so parsing the cached response is deferred to a later task, preventing synchronous recursion when consumers call identity methods again from within callbacks.

Changes:

  • Defer parseIdentityResponse via setTimeout(..., 0) when returning a cached identity.
  • Update/extend identity-utils tests to assert callback deferral and guard against recursion.
  • Update an identity integration test to wait on the callback rather than user-state predicates for the cached path.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/identity-utils.ts Defers cached identity response parsing to avoid synchronous callback recursion.
test/src/tests-identity-utils.ts Updates existing cache test for async callback timing and adds recursion regression test.
test/src/tests-identity.ts Makes cached-identify test wait for the callback to ensure the cached path completes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +525 to +531
cacheIdentityRequest(
'identify',
knownIdentities,
MILLISECONDS_IN_ONE_DAY,
cacheVault,
identityResponse
);
Copy link
Collaborator

@alexs-mparticle alexs-mparticle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the fix is correct and well-targeted. The setTimeout(fn, 0) approach is a standard pattern for breaking synchronous recursion and aligns with the already-async non-cached code path. Main actionable item is verifying the cache key alignment in the new test. See inline comments.

Comment on lines +265 to 276
setTimeout(() => {
parseIdentityResponse(
cachedIdentity,
mpid,
callback,
identityApiData,
identityMethod,
knownIdentities,
true
);
}, 0);

Copy link
Collaborator

@alexs-mparticle alexs-mparticle Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should work but we have issues with setTimeout in the past, especially to handle some of the async issues in the SDK.

One consideration: this is a subtle behavioral change for consumers. Previously, cached identity callbacks fired synchronously within the identify() call. Now they fire on the next tick:

mParticle.Identity.identify(ids, callback);
// callback has NOT fired here anymore when using cache

Since the non-cached path was already async, consumers should already handle this — but it's worth noting in release notes.

Alternative approach to consider: a re-entrancy guard flag would prevent the recursion deterministically without relying on the event loop:

let isProcessingCache = false;
if (shouldReturnCachedIdentity && !isProcessingCache) {
    isProcessingCache = true;
    try { parseIdentityResponse(...); }
    finally { isProcessingCache = false; }
}

This might not necessarily be better than the setTimeout approach, but I'm concerned if there may be extra downstream effects.

Comment on lines +525 to +530
cacheIdentityRequest(
'identify',
knownIdentities,
MILLISECONDS_IN_ONE_DAY,
cacheVault,
identityResponse
Copy link
Collaborator

@alexs-mparticle alexs-mparticle Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible cache key mismatch. The cache is populated here with knownIdentities (the test fixture), but tryCacheIdentity is called below with knownIdentities1 (a separately constructed object with {customerid: 'id1'}).

If these two produce different cache keys, then shouldReturnCachedIdentity will be false, tryCacheIdentity returns false, parseIdentityResponseFake is never called, and the final assertion expect(callCount).to.equal(MAX_CALLS) would fail.

Personally, I would use clearly different names rather than just adding 1 to the end.

@rmi22186
Copy link
Member

Per our discussion this morning, we probably shouldn't be resolving customer implementation errors with code. We should have them correct their implementation. Please close.

@jaissica12 jaissica12 closed this Mar 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants