Repository navigation
User ID: a filtered refreshUserIds must not release the auction early - #15562
Conversation
refreshUserIds({submoduleNames}) chains off done.catch(() => null). Since
cancelAndTry rejects the previous cancel deferred, that promise settles at
once and the new chain never waits for the work the old one still held. A
submodule the refresh did not name, one with a network call, is dropped from
the auction it was about to reach.
A publisher on auctionDelay 300 with liveIntentId and pubProvidedId called
refreshUserIds({submoduleNames: ['pubProvidedId']}) at page load: the first
auction opened ~300ms early and first-auction LiveIntent presence fell from
77.1% to 49.7% (z ~ 8).
Track the callback batches that have been discovered but not finished, from
discovery rather than from when they start running, since with auctionDelay
0 they are held until after the auction ends. A filtered refresh then waits
for what it left running, while an unfiltered one supersedes everything and
keeps escaping a stuck initialization.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0741e5490a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…refresh release what it supersedes Two from review. The batch was registered once the refresh chain reached initSubmodules, so a second filtered refresh issued back to back could snapshot the outstanding set before the first had registered, and resolve while the first one's callback was still running. It is now registered synchronously when the refresh is issued. And an unfiltered refresh only omitted the existing markers from its own wait, leaving them in place: a batch nothing would ever complete stayed outstanding and any later filtered refresh inherited it. An unfiltered refresh now releases the markers it supersedes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f253130b6e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…alled An unfiltered refresh released the markers it supersedes before cancelAndTry swapped in the new done. With greedy promises that can fulfil the chain the refresh is replacing, and a caller checking whether the chain is still current sees the old one settle and takes its result. The release now happens after the swap, and skips the refresh's own batch. Reported by Codex review.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
This PR introduces changes that may not work on all browsers. According to Babel, the following polyfills may be needed, and they are not automatically included:
The best way to address this is to provide good test coverage, as normal PR checks run unit tests on older browsers. |
|
The ES5 bundle covers it regardless: On coverage: this PR adds 132 lines of tests in |
|
@MO-Thibault the polyfill comment is a warning to you, not a blocker |
Whitespace only: the added tracking block left initChain over-indented by two spaces and a multi-space run before PbPromise.all.
|
@patmmccann done. |
The test passed the delay stub as `delay`, but startAuctionHook reads `mkDelay`, so the real 10ms auctionDelay timer stayed in the race and won on Safari before the callback resolved.
|
@MO-Thibault I don't have enough permission to update this PR so I cut Optable#16 which I believe should fix the test failure on Safari |
|
@dgirardi Do you want me to encompass your changes in my PR? It does look like it will fix the failing test indeed. |
…prebid#15562) * User ID: a filtered refreshUserIds must not release the auction early refreshUserIds({submoduleNames}) chains off done.catch(() => null). Since cancelAndTry rejects the previous cancel deferred, that promise settles at once and the new chain never waits for the work the old one still held. A submodule the refresh did not name, one with a network call, is dropped from the auction it was about to reach. A publisher on auctionDelay 300 with liveIntentId and pubProvidedId called refreshUserIds({submoduleNames: ['pubProvidedId']}) at page load: the first auction opened ~300ms early and first-auction LiveIntent presence fell from 77.1% to 49.7% (z ~ 8). Track the callback batches that have been discovered but not finished, from discovery rather than from when they start running, since with auctionDelay 0 they are held until after the auction ends. A filtered refresh then waits for what it left running, while an unfiltered one supersedes everything and keeps escaping a stuck initialization. * userId: register a refresh's batch when it is issued, and let a full refresh release what it supersedes Two from review. The batch was registered once the refresh chain reached initSubmodules, so a second filtered refresh issued back to back could snapshot the outstanding set before the first had registered, and resolve while the first one's callback was still running. It is now registered synchronously when the refresh is issued. And an unfiltered refresh only omitted the existing markers from its own wait, leaving them in place: a batch nothing would ever complete stayed outstanding and any later filtered refresh inherited it. An unfiltered refresh now releases the markers it supersedes. * userId: release superseded batches only after the replacement is installed An unfiltered refresh released the markers it supersedes before cancelAndTry swapped in the new done. With greedy promises that can fulfil the chain the refresh is replacing, and a caller checking whether the chain is still current sees the old one settle and takes its result. The release now happens after the swap, and skips the refresh's own batch. Reported by Codex review. * User ID: eslint --fix on the initChain indentation Whitespace only: the added tracking block left initChain over-indented by two spaces and a multi-space run before PbPromise.all. * userId: stub the auction delay in the filtered-refresh test The test passed the delay stub as `delay`, but startAuctionHook reads `mkDelay`, so the real 10ms auctionDelay timer stayed in the race and won on Safari before the callback resolved. --------- Co-authored-by: Patrick McCann <patmmccann@gmail.com>
Type of change
Bugfix
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Description of change
refreshUserIds({submoduleNames})chains offdone.catch(() => null).cancelAndTryrejects the previous cancel deferred first, so that promise settles immediately and the new chain never waits for the work the old one was still holding. A submodule the refresh did not name, one with a network request in flight, is dropped from the auction it was about to reach.getUserIdsAsyncis documented to resolve only once all submodules have initialized, andstartAuctionHookraces it againstauctionDelay, so the auction is released early.Seen in production: a publisher running
auctionDelay: 300withliveIntentIdandpubProvidedIdcallsrefreshUserIds({submoduleNames: ['pubProvidedId']})at page load to apply EIDs. The first auction opened about 300ms early and first-auction LiveIntent presence fell from 77.1% to 49.7%, z ≈ 8, against a control arm that differed by one inert value in local storage. It converged by the third auction of the page, which is the signature of the race rather than a data problem.The fix tracks the callback batches that have been discovered but not finished. A filtered refresh waits for those before its chain resolves; an unfiltered refresh supersedes them and keeps its current behaviour, including escaping a stuck initialization. Batches are tracked from discovery rather than from when they start running, because with
auctionDelay: 0callbacks are held untilAUCTION_ENDplussyncDelay, and a refresh in that window would otherwise see nothing outstanding.getUserIdsAsync,retryOnCancelandprocessSubmoduleCallbacksare unchanged.One behaviour change worth confirming: a filtered refresh no longer escapes a stuck initialization, since it waits for the callbacks it left running. An unfiltered refresh still does, and there is a test for it. Keeping the escape for filtered refreshes as well needs per-submodule bookkeeping, which is what #15553 does and is considerably more code.
Tests added to
test/spec/modules/userId_spec.js, each failing without the change:Other information
Supersedes #15553, which fixes the same bug with a per-submodule map of pending callbacks consulted by
getUserIdsAsync. Automated review found five gaps in that approach, all stemming from the map being a second source of truth about whether initialization has finished, one that does not participate in cancellation. This version keeps a single wait channel.