Skip to content

Conversation

@alexs-mparticle
Copy link
Collaborator

@alexs-mparticle alexs-mparticle commented Jan 20, 2026

Background

A race condition prevented email/emailsha256 from being propagated to selectPlacements when:

  1. An identify call was made during SDK initialization with email in userIdentities (asynchronous)
  2. A selectPlacements call was made immediately after, before identify completed
  3. Email was not explicitly passed in the selectPlacements attributes

selectPlacements only used attributes passed in the call and did not pull email/emailsha256 from current user identities, so selection calls could run without email even after identify was called.

What Has Changed

  • Automatically propagate email and emailsha256 from current user identities to enrichedAttributes in selectPlacements when not already present in the passed attributes
  • Refresh current user identities after identify completes to ensure latest values are available
  • Added test cases to verify email/emailsha256 propagation and race condition handling

Screenshots/Video

  • {Include any screenshots or video demonstrating the new feature or fix, if applicable}

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)

@alexs-mparticle alexs-mparticle marked this pull request as ready for review January 20, 2026 20:16
@alexs-mparticle alexs-mparticle changed the base branch from master to development January 20, 2026 20:18
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 fixes a race condition where email/emailsha256 identities were not properly propagated to selectPlacements calls when an asynchronous identify() call was made during SDK initialization. The fix ensures that current user identities are refreshed and automatically propagated to placement attributes when they're not explicitly provided.

Changes:

  • Added logic to refresh user identities and automatically propagate email/emailsha256 from current user identities to selectPlacements attributes when not already present
  • Added comprehensive test coverage for email/emailsha256 propagation, race condition handling, and ensuring explicit attributes are not overridden

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
src/roktManager.ts Added user identity refresh logic and automatic email/emailsha256 propagation to enrichedAttributes in selectPlacements method
test/jest/roktManager.spec.ts Added 4 new test cases covering email propagation, emailsha256 propagation, race condition scenarios, and explicit attribute preservation

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

@alexs-mparticle alexs-mparticle force-pushed the fix/SDKE-846-identity-select-placements branch from 2790e95 to 0297078 Compare January 20, 2026 20:20
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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.


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

Copy link
Contributor

@jaissica12 jaissica12 left a comment

Choose a reason for hiding this comment

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

Small nit: suggestion that could simplify this a bit

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.


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

alexs-mparticle and others added 2 commits January 21, 2026 13:37
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.


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

Comment on lines 1647 to 1724
it('should propagate email after identify completes when identify is called during init', async () => {
const kit: Partial<IRoktKit> = {
launcher: {
selectPlacements: jest.fn(),
hashAttributes: jest.fn(),
use: jest.fn(),
},
selectPlacements: jest.fn().mockResolvedValue({}),
hashAttributes: jest.fn(),
setExtensionData: jest.fn(),
};

roktManager['placementAttributesMapping'] = [];
roktManager.kit = kit as IRoktKit;

let identifyCallback: (() => void) | null = null;
let identifyResolve: (() => void) | null = null;
const identifyPromise = new Promise<void>((resolve) => {
identifyResolve = resolve;
});

const mockIdentity = {
getCurrentUser: jest.fn()
.mockReturnValueOnce({
getUserIdentities: () => ({
userIdentities: {}
}),
setUserAttributes: jest.fn()
})
.mockReturnValueOnce({
getUserIdentities: () => ({
userIdentities: {
email: 'user@example.com'
}
}),
setUserAttributes: jest.fn()
}),
identify: jest.fn().mockImplementation((data, callback) => {
identifyCallback = callback;
setTimeout(() => {
if (identifyCallback) {
identifyCallback();
}
if (identifyResolve) {
identifyResolve();
}
}, 10);
})
} as unknown as SDKIdentityApi;

roktManager['identityService'] = mockIdentity;

mockIdentity.identify({
userIdentities: {
email: 'user@example.com'
}
}, () => {});

const options: IRoktSelectPlacementsOptions = {
attributes: {
firstname: 'John'
}
};

const selectPlacementsPromise = roktManager.selectPlacements(options);

await identifyPromise;
await selectPlacementsPromise;

expect(kit.selectPlacements).toHaveBeenCalledWith(
expect.objectContaining({
attributes: expect.objectContaining({
email: 'user@example.com',
firstname: 'John'
})
})
);
});
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The test 'should propagate email after identify completes when identify is called during init' doesn't accurately test the race condition scenario described in the PR. The test calls identify directly (line 1699) and then selectPlacements (line 1711), but doesn't set up the store.identityCallInFlight flag. This means the wait logic in selectPlacements (lines 193-199) is never triggered. To properly test the race condition fix, the test should set up store.identityCallInFlight to true before calling selectPlacements, similar to the test on line 1929.

Copilot uses AI. Check for mistakes.
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

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


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

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.


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

Comment on lines 1726 to 1736
const startTime = Date.now();
const selectPlacementsPromise = roktManager.selectPlacements(options);

// Wait for both identity to complete and selectPlacements to finish
await Promise.all([identityCompletePromise, identifyPromise, selectPlacementsPromise]);
const elapsedTime = Date.now() - startTime;

// Verify that selectPlacements waited for the identity call to complete
expect(elapsedTime).toBeGreaterThanOrEqual(100);
expect(elapsedTime).toBeLessThan(500);

Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The timing assertions here are fragile and may cause test flakiness in CI environments or under load. The test expects completion between 100-500ms, but this assumes the setTimeout and polling intervals execute predictably. Consider either removing these timing assertions entirely (the functional behavior is already verified by checking the email was propagated), or using Jest's fake timers to control time explicitly.

Suggested change
const startTime = Date.now();
const selectPlacementsPromise = roktManager.selectPlacements(options);
// Wait for both identity to complete and selectPlacements to finish
await Promise.all([identityCompletePromise, identifyPromise, selectPlacementsPromise]);
const elapsedTime = Date.now() - startTime;
// Verify that selectPlacements waited for the identity call to complete
expect(elapsedTime).toBeGreaterThanOrEqual(100);
expect(elapsedTime).toBeLessThan(500);
const selectPlacementsPromise = roktManager.selectPlacements(options);
// Wait for both identity to complete and selectPlacements to finish
await Promise.all([identityCompletePromise, identifyPromise, selectPlacementsPromise]);

Copilot uses AI. Check for mistakes.
Comment on lines 2002 to 2006
const elapsedTime = Date.now() - startTime;
// Should have waited until identity call completed (allowing for polling interval variance)
// With 50ms polling interval, it should complete around 150-200ms
expect(elapsedTime).toBeGreaterThanOrEqual(100);
expect(elapsedTime).toBeLessThan(500); // Should complete well before max wait time
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

Similar to the previous test, these timing assertions (lines 2002-2006) are fragile and may cause test flakiness. The test already verifies the functional behavior (that selectPlacements waited for identity completion), so the timing assertions add risk without much value. Consider using Jest's fake timers or removing these timing checks.

Copilot uses AI. Check for mistakes.
Comment on lines 198 to 200
while (this.store?.identityCallInFlight && (Date.now() - startTime) < RoktManager.IDENTITY_CALL_MAX_WAIT_MS) {
await new Promise(resolve => setTimeout(resolve, RoktManager.IDENTITY_CALL_POLL_INTERVAL_MS));
}
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

The polling implementation uses a busy-wait pattern that could be inefficient if the store check is expensive. While the 50ms interval mitigates this, consider whether the identityCallInFlight flag update triggers any observable event that could be used instead of polling. However, if polling is the only option, the current implementation with a reasonable interval and max timeout is acceptable.

Copilot uses AI. Check for mistakes.
});

it('should wait briefly when identity call is in flight', async () => {
jest.setTimeout(10000); // Increase timeout for this test
Copy link

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

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

Setting jest.setTimeout(10000) at the test level affects the timeout for this specific test, but this approach is deprecated in newer versions of Jest. Consider using the third parameter of the it() function instead: it('test name', async () => {...}, 10000). However, since the test includes timing assertions that wait for ~150ms, the default Jest timeout should be sufficient and this line may be unnecessary.

Suggested change
jest.setTimeout(10000); // Increase timeout for this test

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

@alexs-mparticle I believe we can listen to the bot here and clean up

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
19.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

3 participants