fix: [SDK-4672] start location updates when permission is granted

[nan-li]

Description

One Line Summary

When registering for ongoing location updates fails, the SDK now records the failure and registers again the next time location starts, instead of assuming the first attempt worked and never retrying.

Details

Motivation

If an app turns on location sharing (isShared = true) before the user has granted location permission, the SDK tries to register for ongoing location updates, but that call fails because the permission is missing. The failure was caught but not reported back, so the SDK recorded the location-updates listener as registered when it actually was not. After the user granted permission, the SDK sent a single saved location and did not register for ongoing updates again, so no further location changes were reported until the next time the app was sent to the background and brought back to the foreground.

This change makes the registration call report whether it actually succeeded, so the SDK no longer treats a failed attempt as registered, and it registers for ongoing updates again as soon as permission is granted. Ongoing updates now start right away.

Scope

• Affects the Google Play Services location path only; the Huawei path is unchanged.
• Ongoing updates now start at the moment permission is granted.
• No public API changes.

Testing

Unit testing

Added two tests in GmsLocationControllerTests: one checks that calling start() again registers for location updates a second time, and one checks that a failed registration is not recorded as active. All location unit tests and detekt pass.

Manual testing

Reproduced on the demo app (com.onesignal.example, built from this branch) on a Pixel 7 / Android 14 emulator: turned on isShared before granting permission, then chose "While using the app". Before the change, ongoing updates started only after the app was sent to the background and brought back; after the change, the SDK registers for ongoing updates right after the grant with no error, and the latitude/longitude reaches OneSignal in the same foreground session.

Verification logs

Turn on isShared before permission — the registration call fails, so no ongoing updates are started:

16:37:45.358 D LocationManager.setIsShared(value: true)
16:37:45.358 D LocationManager.startGetLocation()
16:37:45.388 D GMSLocationController GoogleApiClientListener onConnected
16:37:45.395 D GMSLocationController GoogleApiClient requestLocationUpdates!
16:37:45.398 W FusedLocationApi.requestLocationUpdates failed!
16:37:45.398 W java.lang.SecurityException: uid 10204 does not have any of
              [ACCESS_FINE_LOCATION, ACCESS_COARSE_LOCATION]

Grant "While using the app" — the SDK registers for ongoing updates immediately, with no background/foreground cycle in between:

16:38:15.663 D ...key=USER_RESOLVED_PERMISSION_android.permission.ACCESS_FINE_LOCATION
16:38:15.664 D LocationManager.startGetLocation()
16:38:15.676 D ApplicationService.onActivityResumed: MainActivity
16:38:15.684 D LocationController fireCompleteForLocation: Location[fused 37.421998,-122.084 ...]
16:38:15.695 D GMSLocationController GoogleApiClient requestLocationUpdates!
16:38:20.699 D HttpClient: PATCH .../users/...  Body: {"properties":{"lat":37.4219983,"long":-122.084}}
16:38:20.917 D HttpClient: PATCH ... STATUS: 202

The requestLocationUpdates! at the grant has no SecurityException after it, and there is no LocationUpdateListener.onFocus()/onUnfocused() between the grant and the re-registration — so ongoing updates start from the permission grant itself, not from a background/foreground cycle.

Affected code checklist

• Notifications
  • Display
  • Open
  • Push Processing
  • Confirm Deliveries
• Outcomes
• Sessions
• In-App Messaging
• REST API requests
• Public API changes

Checklist

Overview

• I have filled out all REQUIRED sections above
• PR does one thing
• No public API changes

Testing

• I have included test coverage for these changes
• All automated tests pass
• I have personally tested this on an emulator

Final pass

• Code is as readable as possible.
• I have reviewed this PR myself

Made with Cursor

[nan-li]

@claude review

[github-actions]

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

• FusedLocationApiWrapperImpl.kt: 0/3 touched executable lines (0.0%) (6 touched lines in diff)
  • 3 uncovered touched lines in this file
• GmsLocationController.kt: 0/15 touched executable lines (0.0%) (21 touched lines in diff)
  • 15 uncovered touched lines in this file

Overall (aggregate gate)

0/18 touched executable lines covered (0.0% — requires ≥ 80%)

Per-file detail (informational; gate is aggregate above):

• FusedLocationApiWrapperImpl.kt: 0.0% (3 uncovered touched lines)

• GmsLocationController.kt: 0.0% (15 uncovered touched lines)

❌ Coverage Check Failed

Aggregate coverage on touched lines is 0.0% (minimum 80%).

📥 View workflow run

[claude]

LGTM — small, well-tested fix that re-arms the live location subscription when start() is called again after a permission grant.

Extended reasoning...

Overview

This PR fixes SDK-4672 by making location updates start immediately after the user grants permission, rather than waiting for the next app focus change. It modifies FusedLocationApiWrapperImpl.requestLocationUpdates to return a Boolean indicating success/failure, threads that result into LocationUpdateListener.hasExistingRequest so failed requests are not later cancelled as if they were active, and adds a refreshRequest() call in the start() re-entrance branch so a second start() (triggered by the permission grant) re-arms the subscription instead of short-circuiting. Visibility of refreshRequest was widened from private to internal, the mock was extended with call counters and a configurable return value, and two regression tests were added.

Security risks

None meaningful. The PR does not change how permissions are checked or requested — it only changes how the SDK reacts to an existing failed request once permission becomes available. No public API change, no data exposure, no auth surface.

Level of scrutiny

Low-to-moderate. The diff is confined to the GMS location subsystem (Huawei path untouched), the change is mechanical (propagate a boolean, retry on re-entrance), and existing tests plus two new regression tests cover both the happy path and the failed-request-not-treated-as-active edge case. The new refreshRequest() call sits inside the existing startStopMutex, so it inherits the same synchronization as the rest of start().

Other factors

The bug-hunting pass found no issues. The author reported manual reproduction and verification on an Android 14 emulator. The fix aligns cleanly with the prior commit 032afbe on the same ticket. No outstanding reviewer comments to address.

[fadi-george]

Is this similar to the other location pr?

[nan-li]

Is this similar to the other location pr?

No, that one catches throwables in LocationServices API only.

This one calls an API requestLocationUpdates that already is wrapped in a try-catch but it has to do with making sure the SDK actually subscribes to location updates instead of silently failing. I updated the Motivation section to be more clear.

[abdulraqeeb33]

Hey Nan, took a pass through this with a couple of reviewers and overall it's a solid fix — really like that requestLocationUpdates now reports real success instead of us optimistically assuming it worked. The re-arm-on-grant flow makes a lot of sense and the manual repro in the description is super clear. Nice work tracking this one down.

A few things I'd love your thoughts on before we ship:

Re-arm churn on every start() — since the early-path now calls refreshRequest() unconditionally, and refreshRequest() cancels + re-registers (and resets the interval timer) even when the subscription is already healthy, repeated start() calls (toggling isShared, permission callbacks) end up churning the subscription. Could we guard it so we only re-arm when hasExistingRequest == false?

hasExistingRequest threading — it's an unsynchronized var that's now touched from the IO start() path (under startStopMutex) and from onFocus/onUnfocused on the main thread (no lock). This race technically predates your change, but this PR widens it a bit. Worth at least a @volatile, or serializing the refresh through the same lock?

The new tests — the mocked applicationService no-ops addApplicationLifecycleHandler, but the real one fires onFocus(true) synchronously, so the callCount shouldBe 2 assertion doesn't quite mirror production's re-entrant path. Might be worth a test that exercises that flow + asserts the failed-first / success-second transition directly.

None of these are blockers from my side

[abdulraqeeb33]

Hey Nan, took a pass through this with a couple of reviewers and overall it's a solid fix — really like that requestLocationUpdates now reports real success instead of us optimistically assuming it worked. The re-arm-on-grant flow makes a lot of sense and the manual repro in the description is super clear. Nice work tracking this one down.

A few things I'd love your thoughts on before we ship:

Re-arm churn on every start() — since the early-path now calls refreshRequest() unconditionally, and refreshRequest() cancels + re-registers (and resets the interval timer) even when the subscription is already healthy, repeated start() calls (toggling isShared, permission callbacks) end up churning the subscription. Could we guard it so we only re-arm when hasExistingRequest == false?

hasExistingRequest threading — it's an unsynchronized var that's now touched from the IO start() path (under startStopMutex) and from onFocus/onUnfocused on the main thread (no lock). This race technically predates your change, but this PR widens it a bit. Worth at least a @volatile, or serializing the refresh through the same lock?

The new tests — the mocked applicationService no-ops addApplicationLifecycleHandler, but the real one fires onFocus(true) synchronously, so the callCount shouldBe 2 assertion doesn't quite mirror production's re-entrant path. Might be worth a test that exercises that flow + asserts the failed-first / success-second transition directly.

None of these are blockers from my side

[fadi-george]

@nan-li merge?

[claude]

LGTM — small, well-tested fix; the re-registration on subsequent start() and the success-aware hasExistingRequest are both covered by the new regression tests.

Extended reasoning...

Overview

This PR fixes a bug where the SDK records location-updates registration as active even when the underlying requestLocationUpdates call fails (e.g. a swallowed SecurityException when permission is missing). The fix makes the wrapper return a Boolean, assigns hasExistingRequest from that result rather than unconditionally setting it to true, and calls refreshRequest() on a subsequent start() when the GoogleApiClient already exists so the live subscription re-arms as soon as permission is granted. Touches 3 source files (wrapper impl, interface, controller) and 2 test files, no public API change.

Security risks

None meaningful. The change is in the GMS location path and does not alter permission checks, crypto, auth, or any external interface. If anything, it makes the SDK's handling of missing-permission failures more accurate (a previously silent failure now correctly reflects as not-registered, preventing a phantom cancel later).

Level of scrutiny

Low–medium. Internal Kotlin interface signature change, small scope, well-isolated to the GMS location controller. The behavior is exercised by three new regression tests that cover the re-arm path, the failed-request-not-recorded invariant, and the full grant-after-failure repro. Manual verification logs in the PR description show the live subscription engaging immediately after permission grant with no background/foreground cycle.

Other factors

The diff coverage gate reports 0% on touched executable lines, but this is explained: the wrapper impl lines can't be exercised without real GMS APIs, and the touched lines in GmsLocationController are reached via the mock in the new tests (the gate appears to be using JaCoCo line data that doesn't credit indirect coverage here). The reviewer (fadi-george) has already asked the author about merging. Companion PR #2662 (which I previously reviewed) handled a related but distinct path (catching throwables); the author clarified the distinction in the thread.

[nan-li]

Thanks for the careful review, all three were fair, and I've pushed a change for each:

1. Re-arm churn on every start() — done. refreshRequest() now takes an onlyIfInactive flag, and the early start() path passes onlyIfInactive = true, so an already-active request is left in place instead of being cancelled and re-registered (which also reset the interval) on repeated start()s. Grant-recovery still works: after a failed attempt the request isn't active, so it re-registers there.

2. hasExistingRequest threading — done. @Volatile alone wouldn't cover it, since cancel → register → assign is a check-then-act, so I moved hasExistingRequest and that whole sequence behind a dedicated lock shared by refreshRequest() and close(). That serializes the IO start()/stop() path with the main-thread onFocus()/onUnfocused() callbacks (the coroutine mutex isn't usable there since those callbacks aren't suspend).

3. Tests — done. Added a regression test that drives the real transition — a first requestLocationUpdates that fails, then a second start() that succeeds — asserting failed-first → success-second directly. I also flipped the old "re-arms on a subsequent start" test (which was really asserting the churn from #1) to verify a healthy request is left alone.

[claude]

Focused fix for SDK-4672 with good test coverage; the new Boolean return + re-arm on start() correctly addresses the silent-failure case, and the synchronization around hasExistingRequest is straightforward.

Extended reasoning...

Overview

This PR fixes SDK-4672: when requestLocationUpdates failed (e.g. swallowed SecurityException because permission wasn't granted yet), the SDK still recorded the listener as active, so once permission was granted no further updates were registered until a background/foreground cycle. The fix changes IFusedLocationApiWrapper.requestLocationUpdates to return a Boolean indicating actual success, propagates that into hasExistingRequest, and re-arms the subscription on the next start() via refreshRequest(onlyIfInactive = true). Scope is limited to the GMS location path; the Huawei path and public API are unchanged.

Security risks

None material. The change is purely about correctly tracking whether a location-updates subscription is active — no permission, auth, crypto, or data-exposure surface is touched. The underlying permission check in Android is unchanged; the SDK simply no longer pretends a permission-denied request succeeded.

Level of scrutiny

Moderate. The change touches subscription state management and introduces a synchronized(requestLock) block, so concurrency correctness matters, but the lock pattern is conservative (single lock guarding hasExistingRequest and the cancel/register sequence) and easy to verify. The interface change is internal-only.

Other factors

• Three new unit tests directly exercise the regression path (failed, then granted, then stop), plus a no-churn test for repeated start().
• Manual verification logs in the PR body show the fix working on a real device.
• The Diff Coverage gate failed at 0%, but inspection shows the new tests clearly exercise the touched lines — this looks like a JaCoCo/Kotlin instrumentation artifact rather than missing coverage, and the comment doesn't appear to be blocking the maintainers.
• Recent commits (only re-arm location updates when none is active to avoid churn, serialize location-updates refresh to remove hasExistingRequest race) show the author iterated on concurrency concerns already.
• Two maintainers are aligned on merging.

[claude]

LGTM — all three prior comments are addressed and the new tests cover the failed-then-granted re-registration path.

Extended reasoning...

Overview

This revision addresses the three concerns I raised earlier on the Google Play Services location path:

• refreshRequest() now takes onlyIfInactive and the start() early-return path passes true, so a healthy active request is no longer torn down and re-registered (which would reset the interval) on repeated start()s, while grant-recovery still works because a failed request is not flagged active.
• hasExistingRequest and the cancel/register/assign sequence are now serialized via a dedicated requestLock shared by refreshRequest() and close(), correctly handling the main-thread onFocus()/onUnfocused() callbacks that the existing coroutine mutex cannot cover.
• A new regression test drives the actual transition (first requestLocationUpdates fails, second start() succeeds, then stop() cancels), and the old "re-arms on subsequent start" test was flipped to assert the no-churn behavior.

Security risks

None. The change tightens an existing failure path: requestLocationUpdates already catches throwables (including the SecurityException thrown when permission is missing), and the only new behavior is that the swallowed failure is now reported via a Boolean instead of being silently treated as success. No permission checks, crypto, or auth code is touched.

Level of scrutiny

Moderate-low. The blast radius is the GMS location subsystem only (the Huawei path is unchanged) and the IFusedLocationApiWrapper interface change is internal. The locking introduced for hasExistingRequest is a small, well-scoped synchronized block on a private Any() monitor, with no nested locks that could deadlock against startStopMutex.

Other factors

The author personally reproduced the bug on a Pixel 7 / Android 14 emulator and posted before/after logs, the new tests directly exercise both the no-churn and failed-then-granted paths, and prior feedback from the bug-hunting system has all been addressed in subsequent commits. The 0% diff-coverage report is a Jacoco artifact (the new tests are robolectric/kotest tests that don't feed Jacoco), not a real coverage gap.