Improve iBeacon zone event reliability - #5629
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new outbox/drain + beacon scanning logic has a few correctness/operational issues (state clearing during scans, outbox deadlock on unreadable events, and retry-notification spam) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This pull request strengthens iBeacon-based zone entry/exit reliability (especially during background wakes) by adding bounded beacon-ranging verification/reconciliation and by introducing a persisted, ordered outbox for zone transition delivery via background URLSession uploads.
Changes:
- Add beacon entry verification (ranging confirmation), exit reconciliation, and background execution support to reduce stale/false transitions.
- Introduce a disk-backed
ZoneEventOutboxand switch zone event delivery to persisted background webhook uploads with retry/backoff. - Add targeted unit/regression tests covering background upload creation, region filtering limits, ranging behaviors, and outbox ordering/persistence.
File summaries
| File | Description |
|---|---|
| Tests/Shared/Webhook/WebhookManager.test.swift | Adds tests asserting immediate background upload task creation and URL selection behavior. |
| Tests/Shared/Webhook/FakeWebhookManager.swift | Extends fake to track send vs startPersistedBackground usage for new call paths. |
| Tests/Shared/HAAPIPersistentEvent.test.swift | Adds coverage ensuring persistent events route to background upload (not legacy send). |
| Tests/Shared/AppZone.test.swift | Minor test adjustment for beacon region monitoring expectations. |
| Tests/App/ZoneManager/ZoneManagerRegionFilter.test.swift | Updates limits and adds regression for the global 20-region cap. |
| Tests/App/ZoneManager/ZoneManagerProcessor.test.swift | Updates behavior to process beacon exits (previously ignored). |
| Tests/App/ZoneManager/ZoneManagerCollector.test.swift | Adds extensive coverage for beacon ranging verification, retries, opportunistic scans, and background execution behavior. |
| Tests/App/ZoneManager/ZoneManager.test.swift | Adds tests for scanning lifecycle and for outbox-driven delivery/retry/ordering semantics. |
| Tests/App/ZoneManager/ZoneEventOutbox.test.swift | Adds persistence/expiry/coalescing tests for the outbox implementation. |
| Tests/App/ZoneManager/FakeCLLocationManager.swift | Extends fake CLLocationManager to support ranging/authorization/location update tracking. |
| Sources/Shared/Notifications/NotificationIdentifier.swift | Adds identifiers used by beacon delivery diagnostics (mostly disabled call sites). |
| Sources/Shared/API/Webhook/Networking/WebhookManager.swift | Adds synchronous “start persisted background upload task” API. |
| Sources/Shared/API/Models/AppZone.swift | Clarifies beacon monitoring behavior in regionsForMonitoring. |
| Sources/Shared/API/HAAPI.swift | Adds startPersistentEvent wrapper for immediate persisted background event delivery. |
| Sources/HANetworking/Sources/Server.swift | Adds preferredBackgroundWebhookURL() API for background-safe URL selection. |
| Sources/HANetworking/Sources/ConnectionInfo.swift | Implements “prefer remote/cloudhook for background” webhook URL selection. |
| Sources/App/ZoneManager/ZoneManagerState.swift | Adds ranging failure state for collector logging. |
| Sources/App/ZoneManager/ZoneManagerRegionFilter.swift | Enforces total region count cap in addition to per-type caps. |
| Sources/App/ZoneManager/ZoneManagerProcessor.swift | Removes prior logic that ignored beacon exit events. |
| Sources/App/ZoneManager/ZoneManagerIgnoreReason.swift | Replaces beacon-exit ignore reason with “entry not verified” reason. |
| Sources/App/ZoneManager/ZoneManagerEvent.swift | Adds optional beacon diagnostic metadata attached to collected events. |
| Sources/App/ZoneManager/ZoneManagerCollector.swift | Implements beacon entry verification, opportunistic scans, reconciliation, and background execution leasing. |
| Sources/App/ZoneManager/ZoneManager.swift | Switches zone transition delivery to a persisted outbox + background upload + retry loop; adds foreground/background scan lifecycle hooks. |
| Sources/App/ZoneManager/ZoneEventOutbox.swift | Introduces PendingZoneEvent and UserDefaultsZoneEventOutbox for durable ordered delivery. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ecddc1e to
64e864f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Background scan lifecycle, concurrency, duplicate delivery, and persistence durability issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Sources/App/ZoneManager/ZoneManagerCollector.swift:135
- The PR promises bounded, energy-conscious ranging and says it does not add continuous background location scanning, but this path starts ranging when the app resigns active and leaves it running until the next lifecycle transition; the log even describes it as continuous. Replace this lifecycle-long mode with bounded opportunistic ranging tied to Core Location wakes and a timeout.
startForegroundBeaconScanning(in: regions, manager: manager)
recordBeaconBackgroundEvent("Started continuous background beacon ranging without GPS")
Sources/App/ZoneManager/ZoneManagerCollector.swift:131
- Once background monitoring is active, every later zone synchronization is ignored. If beacon zones are added or removed while the app remains in the background, the collector keeps ranging removed constraints and never ranges new ones until another active/inactive transition.
func startBackgroundBeaconMonitoring(in regions: Set<CLRegion>, manager: CLLocationManager) {
guard regions.contains(where: { $0 is CLBeaconRegion }), !backgroundBeaconMonitoringActive else { return }
Sources/App/ZoneManager/ZoneManager.swift:404
- An event whose stored data cannot be decoded remains at the head and this returns without scheduling a retry or removing it, permanently blocking every later ordered event until another external trigger happens after expiry. Drop the undeliverable entry and continue draining.
guard let eventData = pending.decodedEventData else {
logZoneEventDrainBlocked(pending, reason: "Event data is unreadable")
return
Sources/App/ZoneManager/ZoneEventOutbox.swift:118
UserDefaults.setdoes not provide a synchronous durable commit, yetappendreturns as though the event is safely on disk and delivery starts immediately afterward. Suspension or termination in that window can lose the event this outbox is intended to protect. Store the ordered outbox in GRDB (the repository convention for new persistent models) or use an atomic file write that reports failures.
private func save(_ events: [PendingZoneEvent]) {
if events.isEmpty {
defaults.removeObject(forKey: key)
} else if let data = try? JSONEncoder().encode(events) {
defaults.set(data, forKey: key)
Sources/App/ZoneManager/ZoneManager.swift:387
- Every rejected retry sends a local
.debugnotification. With the new automatic backoff loop, one outage can generate repeated user-visible alerts for the same event until it expires; keep retry failures in logs and only notify once, if at all.
Current.notificationDispatcher.send(.init(
id: .debug,
title: "DEBUG: Failed to fire ZoneManager",
body: message
))
- Files reviewed: 24/24 changed files
- Comments generated: 11
- Review effort level: Balanced
|
Please resolve copilot's reviews that were addressed and reply to those that require explanation on how they were addressed. |
There was a problem hiding this comment.
🟡 Changes recommended
The synchronous upload path can deadlock, and scanning and outbox lifecycle handling contain reliability issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
Previously missed (2) — in code that hasn't changed since the last review.
Sources/App/ZoneManager/ZoneManagerCollector.swift:451
- The expiration path stops every pending constraint unconditionally before clearing the pending entries. A constraint may still be owned by
foregroundBeaconEntries(background monitoring uses that collection), so expiration of a verification lease can silently stop the long-lived scan while bookkeeping still considers it active. Clear the pending entries first, then stop only constraints for whichhasActiveRangingEntryis false.
Tests/Shared/Webhook/WebhookManager.test.swift:1000 RecordingBackgroundTaskRunneris never referenced, so it neither supports the new tests nor verifies the production background-task behavior. Remove it, or inject it in a test that exercises the queue on whichstartPersistedBackgroundinvokes the runner.
Sources/App/ZoneManager/ZoneManager.swift:679
syncNowruns onregionSyncQueue, but this block readsUIApplication.sharedand mutates the collector's ranging dictionaries off-main while Core Location and lifecycle callbacks mutate them on main. The comment above explicitly says collector bookkeeping is main-thread-only. Dispatch the collector/background-monitoring update throughrunOnMain, passing the already-computed region set to avoid another synchronousmonitoredRegionsread.
if UIApplication.shared.applicationState != .active {
if locationManager.monitoredRegions.contains(where: { $0 is CLBeaconRegion }) {
collector.startBackgroundBeaconMonitoring(
in: locationManager.monitoredRegions,
manager: locationManager
Sources/App/ZoneManager/ZoneManagerCollector.swift:242
- Once this retry count reaches the limit, it is cleared only by a successful
didRangecallback. If ranging fails twice and the scan stops before any callback, a later scan for the same constraint inherits the exhausted count and never retries. Clear both retry state entries whenever the final ranging owner is removed (or when a fresh scan starts).
guard hasActiveRangingEntry(for: beaconConstraint),
beaconRangingRetryCounts[beaconConstraint, default: 0] < beaconRangingRetryLimit,
beaconRangingRetryWorkItems[beaconConstraint] == nil else { return }
beaconRangingRetryCounts[beaconConstraint, default: 0] += 1
Sources/App/ZoneManager/ZoneManagerCollector.swift:14
UIApplicationBeaconScanBackgroundExecutionis a new top-level class in a file named forZoneManagerCollector, contrary to the repository's non-negotiable one-type-per-file rule. Move this implementation toUIApplicationBeaconScanBackgroundExecution.swift(and keep its protocol with the appropriately named abstraction file).
final class UIApplicationBeaconScanBackgroundExecution: BeaconScanBackgroundExecution {
private var identifier = UIBackgroundTaskIdentifier.invalid
func begin(expirationHandler: @escaping () -> Void) {
Sources/App/ZoneManager/ZoneEventOutbox.swift:39
- This new file stacks
PendingZoneEvent,ZoneEventOutbox, andUserDefaultsZoneEventOutbox, so two concrete types do not live in files named after them. The repository's one-type-per-file rule requires movingPendingZoneEventandUserDefaultsZoneEventOutboxinto their own files.
final class UserDefaultsZoneEventOutbox: ZoneEventOutbox {
Tests/App/ZoneManager/ZoneManager.test.swift:33
- The newly added private dispatcher and outbox fakes are top-level types in
ZoneManager.test.swift, contrary to the repository's one-type-per-file rule. Nest these small helpers insideZoneManagerTestsor move each to a matching file.
private final class ZoneManagerNotificationDispatcher: LocalNotificationDispatcherProtocol {
Tests/App/ZoneManager/ZoneManagerCollector.test.swift:1159
- This private fake is another top-level class in
ZoneManagerCollector.test.swift, contrary to the repository's one-type-per-file rule. Nest it insideZoneManagerCollectorTestsor move it toFakeBeaconScanBackgroundExecution.swift.
private final class FakeBeaconScanBackgroundExecution: BeaconScanBackgroundExecution {
- Files reviewed: 24/24 changed files
- Comments generated: 4
- Review effort level: Balanced
|
tip: |
AI Policy
Select exactly one option that describes AI usage in this contribution:
Summary
This PR improves the reliability of iBeacon zone entry and exit handling, especially when the app is running in the background.
The changes:
The implementation does not introduce continuous background location scanning.
Screenshots
N/A — no user-interface changes.
Link to pull request in Documentation repository
Documentation: N/A — this improves the reliability of existing iBeacon zone behavior and adds no new user-facing configuration.
Any other notes
Testing performed:
App-Debugbuilt and installed successfully on a physical iPhone.main.git diff --checkpasses.Local signing, bundle identifier, provisioning, and Watch-target adjustments used for the development build are not included in this pull request.