[WOOMOB-2734] Fix duplicated device registration API calls on login - #16354
Conversation
samiuelson
added
type: bug
|
|
|
Version |
|
@samiuelson, do you plan to open this PR this week? I’m asking because this is the last remaining task from the PN project. |
…n fix The fallback fan-out is a separate duplicate-registration cause with its own reproduction (several sites failing Woo registration at once, racing the same isWpComPushRegistered check). Keeping it here would mix two independent fixes in one review, so it moves to its own change along with the test that pins it. What remains is the singleton scoping: the orchestration mutex and the in-flight job are instance state, so they only serialise concurrent triggers while the whole app shares one RegisterDevice.
333b4a4 to
34b9205
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review - No issues found. The changes look good.
The @Singleton annotation is the right fix: RegisterDevice serialises push registrations through instance-level state (orchestrationMutex and activeJob), so a shared single instance is required for the dedup to hold across the multiple injection sites (login ViewModels, AppInitializer, FCMMessageService, workers). All constructor dependencies are app-scoped singletons/unscoped, so there is no Hilt scope violation. The added tests cover both the runtime serialisation and the singleton-scoping contract.
Automatic review · claude-opus-4-8 · Workflow run
How to reply to a finding
Reply on this review (or inline at the line the finding refers to) with one of:
@claude addressed- I made the change. Bot verifies against the next diff before marking resolved.@claude rejected: <reason>- Will not fix; reason gets quoted on the next review.@claude not-applicable- Finding does not apply (wrong file, already covered elsewhere, etc.).
The bot honours these on the next review pass.
Hi @irfano 👋 I wasn't able to repro this issue manually on device, only in unit test. If you're able to reproduce the bug and validate this PR fixes it it would be great. Otherwise, feel free to take over this issue because I don't want to block you. |
irfano
left a comment
irfano
left a comment
There was a problem hiding this comment.
I was able to reproduce this on trunk: logging in with a multi-store account while watching traffic gives 5 identical devices/new calls. Your branch drops it to 4, so the singleton fix works, and the remaining duplicates all come from the per-site WPCom fallback you split out in 7177ce2. Since that turned out to be the main source of the duplicates, I restored it here by reverting that commit and updated the description. With both changes in, login fires exactly one call.
3 participants
Description
Fixes WOOMOB-2734
Problem:
RegisterDeviceserializes runs with an internal mutex and skips WPCom registration when a device id is already stored, but the class was unscoped in Hilt, so every injection point got its own instance with its own mutex, and concurrent triggers raced past the "already registered" check (the device id is only persisted after a successful response) and each fired its own call. This PR scopesRegisterDeviceas a@Singletonso that concurrent calls now queue, and follow-up runs no-op against the persisted state.The same check also races within a single run: per-site Woo push registration falls back to WPCom registration when it fails, so several stores failing at once each fired an identical call before the first response persisted the device id. The fallback is now dropped from
RegisterDeviceruns, which already register WPCom once at the end of the run.Test Steps
POST .../devices/new/should fire, and subsequent registration runs should log "Skipping WP.com push registration".devices/newcalls.Images/gif
N/A
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.