fix(sentry): suppress transient provider and HealthKit sync noise - #2347
Merged
Merged
Conversation
Treat provider connect timeouts as transport errors instead of Sentry incidents, keep HealthKit observer expirations quiet during active JS sync, skip background upload timeouts, and cap mobile query cache persistence at 5MB. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Reviewer's GuideSuppresses noisy Sentry reporting from transient provider/HealthKit network conditions and caps mobile query cache persistence size, via new error-classification utilities and bounded storage logic, plus supporting native/mobile changes and tests. Sequence diagram for HealthKit observer expiration handling and JS sync flagsequenceDiagram
participant JS as BackgroundHealthKitSync_JS
participant HKJS as HealthKitModule_JS
participant HKNative as HealthKitModule_native
participant Sentry
JS->>HKJS: drainSyncQueue()
HKJS->>HKNative: setObserverSyncInProgress(true)
HKNative->>HKNative: observerUpdateCoordinator.handleExpiration(expiration)
alt observerSyncInProgress is true
HKNative->>Sentry: addBreadcrumb(healthkit.observer)
else observerSyncInProgress is false
HKNative->>Sentry: capture(error com.dofek.healthkit-observer)
end
JS->>HKJS: performHealthKitSync()
HKJS->>HKNative: completeObserverUpdates(updateIds, succeeded)
JS->>HKJS: drainSyncQueue() completes
HKJS->>HKNative: setObserverSyncInProgress(false)
Flow diagram for provider error classification and Sentry suppression in sync jobsflowchart TD
A[Provider HTTP fetch error] --> B{"isProviderConnectFailure(error)?"}
B -->|yes| C[Throw ProviderRequestTimeoutError]
B -->|no| D[Other error handling]
C --> E[Sync job receives ProviderRequestTimeoutError]
E --> F{"isProviderTransportError(error)?"}
F -->|yes| G[shouldReportProviderError returns false]
G --> H[Skip Sentry capture<br/>record metrics only]
F -->|no| I{"isRetryableInfraError(error)?"}
I -->|yes| J[Sentry.captureException with retryable tag]
I -->|no| K[Normal non-retryable handling]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Summary by QodoSuppress transient provider/HealthKit Sentry noise and cap mobile query cache writes
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
|
🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews. |
Code Review by Qodo
Context used✅ Compliance rules (platform):
181 rules✅ Skills:
fix-provider, write-tests, cloudflare 1. Sync queue can deadlock
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
180
to
182
| syncing = true; | ||
| setObserverSyncInProgress(true); | ||
| try { |
There was a problem hiding this comment.
2. Sync queue can deadlock 🐞 Bug ☼ Reliability
drainSyncQueue() calls setObserverSyncInProgress(true) after setting syncing=true but before the try/finally; if the native call throws, syncing is never reset and future drains are permanently skipped. The finally block also calls setObserverSyncInProgress(false) without guarding against native exceptions, which can abort draining and crash the sync loop.
Agent Prompt
### Issue description
`drainSyncQueue()` sets `syncing = true` and then calls `setObserverSyncInProgress(true)` before entering the `try/finally`. If that native call throws synchronously, the `finally` block never runs, leaving `syncing` stuck `true` and preventing any future background sync drains.
### Issue Context
This codepath is part of the serialized background HealthKit observer delivery drain; it is designed to be exception-safe (e.g., `completeObserverUpdates` is wrapped in a `try/catch`).
### Fix Focus Areas
- packages/mobile/lib/background-health-kit-sync.ts[159-211]
### Suggested fix
- Move `setObserverSyncInProgress(true)` inside the `try` (or wrap it in its own `try/finally`) so `syncing` is always cleared.
- Wrap both `setObserverSyncInProgress(true|false)` calls in `try/catch` and record via `captureException` (similar to `acknowledgeObserverUpdates`) so a native exception can’t permanently block draining.
- Ensure `setObserverSyncInProgress(false)` cannot throw out of `finally` and interrupt the recursive `await drainSyncQueue()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+44
to
+59
| reportExpiration: { [weak self] expiration in | ||
| guard let self else { | ||
| return | ||
| } | ||
| if self.observerSyncInProgress { | ||
| let breadcrumb = Breadcrumb(level: .info, category: "healthkit.observer") | ||
| breadcrumb.message = | ||
| "Observer update expired while JavaScript sync was still running" | ||
| breadcrumb.data = [ | ||
| "updateId": expiration.updateId, | ||
| "typeIdentifier": expiration.typeIdentifier, | ||
| "ageMilliseconds": expiration.ageMilliseconds, | ||
| ] | ||
| SentrySDK.addBreadcrumb(breadcrumb) | ||
| return | ||
| } |
There was a problem hiding this comment.
3. Native observer flag race 🐞 Bug ☼ Reliability
HealthKitModule.swift reads observerSyncInProgress from the observer expiration callback that runs on HealthKitObserverUpdateCoordinator’s background expirationQueue, while setObserverSyncInProgress mutates it from JS without synchronization. This introduces an unsynchronized cross-thread access (data race) that can lead to nondeterministic visibility and unsafe native behavior.
Agent Prompt
### Issue description
`observerSyncInProgress` is accessed from multiple threads/queues without synchronization: written via the exported Expo function, and read inside the observer expiration callback invoked on a dedicated background queue.
### Issue Context
`HealthKitObserverUpdateCoordinator` schedules expirations on `expirationQueue.asyncAfter(...)`, and calls `reportExpiration(...)` from that queue.
### Fix Focus Areas
- packages/mobile/modules/health-kit/ios/HealthKitModule.swift[42-77]
- packages/mobile/modules/health-kit/ios/HealthKitModule.swift[913-915]
- packages/mobile/modules/health-kit/ios/HealthKitObserverUpdateCoordinator.swift[24-57]
- packages/mobile/modules/health-kit/ios/HealthKitObserverUpdateCoordinator.swift[83-102]
### Suggested fix
- Protect `observerSyncInProgress` with a lock (e.g., `NSLock`) or a serial `DispatchQueue`.
- In `setObserverSyncInProgress`, write under the lock/queue.
- In `reportExpiration`, read under the same lock/queue.
- Alternatively, route the expiration callback’s decision to a single queue (e.g., marshal onto main queue before reading the flag), but ensure this doesn’t block observer completion.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+20
to
+29
| function createBoundedAsyncStorage() { | ||
| return { | ||
| getItem: (key: string) => AsyncStorage.getItem(key), | ||
| setItem: async (key: string, value: string) => { | ||
| if (value.length > MOBILE_QUERY_CACHE_MAX_PERSISTED_BYTES) { | ||
| await AsyncStorage.removeItem(key); | ||
| return; | ||
| } | ||
| await AsyncStorage.setItem(key, value); | ||
| }, |
There was a problem hiding this comment.
4. Cache cap not bytes 🐞 Bug ☼ Reliability
createBoundedAsyncStorage enforces MOBILE_QUERY_CACHE_MAX_PERSISTED_BYTES using value.length, but JS string length is not encoded byte size; this can allow persisted entries to exceed the intended 5MB limit when non-ASCII content is present. The constant name/documentation implies a byte-accurate bound, so the current check does not reliably prevent oversized AsyncStorage writes.
Agent Prompt
### Issue description
`MOBILE_QUERY_CACHE_MAX_PERSISTED_BYTES` is treated as a byte limit, but the enforcement uses `value.length` (UTF-16 code units). This does not reliably cap the actual stored size.
### Issue Context
The goal of this PR is to prevent runaway AsyncStorage writes by dropping oversized persisted caches.
### Fix Focus Areas
- packages/mobile/lib/mobile-query-persistence.ts[9-32]
### Suggested fix
- Replace the `value.length` check with a byte-size check for the actual persisted encoding (typically UTF-8), e.g.:
- `const bytes = new TextEncoder().encode(value).length;`
- compare `bytes > MOBILE_QUERY_CACHE_MAX_PERSISTED_BYTES`.
- Add/adjust a test case that uses multibyte characters to ensure the cap is enforced in terms of bytes, not code units.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Mobile PreviewScan to open on device:
To test on device:
|
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ProviderRequestTimeoutErrorand exclude them from Sentry in sync jobs.Test plan
pnpm vitest run --project unit packages/provider-http/src/rate-limit.test.ts src/jobs/process-sync-job.test.tspnpm vitest run --project mobile packages/mobile/lib/background-health-kit-sync.test.ts packages/mobile/lib/mobile-query-persistence.test.tsxMade with Cursor
Summary by Sourcery
Suppress transient provider connection and HealthKit sync errors from Sentry while adding safeguards against oversized mobile query cache persistence.
Bug Fixes:
Enhancements:
Tests:
Summary by cubic
Suppress transient provider and HealthKit sync errors from Sentry and add a 5MB cap to mobile query cache persistence. This reduces false alerts and prevents large AsyncStorage writes.
ProviderRequestTimeoutErrorin@dofek/provider-httpand treat transport errors (503/504, timeouts) as non-reportable inprocess-sync-job, while keeping metrics and retries (DOFEK-SERVER-4A).setObserverSyncInProgressand suppress observer expiration errors while JS sync is running; ignore background upload timeouts and retry on next delivery (DOFEK-MOBILE-1C, DOFEK-MOBILE-19).Written for commit 1b3c681. Summary will update on new commits.