Fix Tidepool uploads with serialization - #1321
Conversation
Tidepool upload results were logged as [Nightscout], making Tidepool failures look like Nightscout activity. Tag them [Service] instead, and give carbs-delete results their own wording so they are distinguishable from carbs uploads.
An actor that runs enqueued upload operations strictly one at a time, in order: each operation starts only after the previous one has fully completed, including its network round-trip. Serialization is global across pipelines because concurrent uploads -- even to different endpoints -- can each trigger a Tidepool session refresh that reuses the single-use rotating refresh token, corrupting the session (nightscout#1235). Design points, several adopted from the Nightscout upload serializer in nightscout#1290: - Requests for a pipeline that gathers its pending data at run time coalesce: while a run is enqueued but not started, further requests are dropped (the waiting run picks up their data when it fetches), and a request arriving mid-run chains exactly one follow-up, which doubles as a retry after a failed run. A trigger burst therefore costs at most one waiting run per pipeline. Delete pipelines carry unique payloads and never coalesce. - Chain tasks run at .utility QoS: the inherited .background QoS is not scheduled during background execution windows, freezing uploads until the app is foregrounded. - A wall-clock watchdog (recoverIfWedged) abandons the chain when its head operation exceeds 10 minutes, e.g. because the app was suspended mid-request and the completion was lost. Wall clock is deliberate: it keeps advancing while the process is suspended. Abandoning bumps a generation counter; operations re-check isCurrent(generation) between network calls so an orphaned chain cannot fire requests alongside its replacement. - awaitUpload bridges TidepoolKit's completion-based calls into async/await with a 120s timeout backstop: a call that never invokes its completion resolves to a logged .timedOut failure instead of suspending the chain forever. SingleResumer guards the continuation so whichever of completion/timeout fires second is a no-op instead of a double-resume crash. Not wired up yet; groundwork for serializing all Tidepool uploads (nightscout#1235).
…1235) Trio fired Tidepool uploads concurrently and unserialized: each CGM reading triggers uploadGlucose() from BOTH glucoseStorage.updatePublisher AND the GlucoseStored Core Data notification, alongside independent dose/carbs triggers. Every trigger spawned a Task into the shared TidepoolKit `TAPI` actor. When the OAuth access token expired, multiple in-flight requests each passed session.shouldRefresh() and called refreshSession() in parallel, reusing the single-use rotating refresh token. That race corrupted the session; subsequent upload completions never fired, so uploads stopped silently with nothing logged. Nightscout was unaffected (separate manager), and Loop avoids the bug by funneling uploads through one serialized RemoteDataServicesManager cycle. Confirmed from the issue logs: last Tidepool success 01:50:06, zero Tidepool errors after, app running continuously to 07:33; at 07:31 a dose reached the "TIDEPOOL DOSE ENTRIES" log but its upload completion never returned. Route every upload (glucose, carbs, dose, pump events, settings, deletes) through TidepoolUploadSerializer so only one network request is in flight at a time. This guarantees at most one session refresh, eliminating the concurrent-refresh race, and a wedged request resolves to a logged .timedOut failure instead of silently stalling every later upload forever. Each pipeline's operation now fetches its pending rows (or rebuilds its payload) inside the serialized run rather than at trigger time, the pattern used by the Nightscout serializer in nightscout#1290. Two triggers firing close together -- e.g. the twin glucose triggers above -- previously both snapshotted the same unmarked rows and uploaded them twice; now the second request coalesces into a follow-up that re-fetches after the first run marked its rows, finding only genuinely new data. Chunked uploads also now mark only the chunk that succeeded as uploaded, instead of marking the full batch after the first chunk. In the dose pipeline, the backgroundContext.perform block just builds the upload payloads and returns them; the network calls happen outside the Core Data context, inside the serialized run. The serializer watchdog is checked at each enqueue and on app foreground (didBecomeActive) -- the reliable recovery points while the pipeline itself is stalled.
- Operations run strictly one at a time, in enqueue order (peak concurrency stays at 1). - A burst of requests for a coalescing pipeline folds into exactly one follow-up run; delete pipelines never coalesce; coalescing is keyed per pipeline. - The watchdog abandons a wedged chain so later uploads run again, clears coalescing state so the pipeline can run on the fresh chain, and leaves an idle chain alone; an abandoned operation sees a stale generation and bails out, and its late completion cannot corrupt the fresh chain's watchdog bookkeeping. - awaitUpload returns the completion's result, resolves to .timedOut when the completion never fires, and ignores a late completion after the timeout (one-shot guard). Adds Recorder/Counter/Gate async test helpers.
dnzxy
left a comment
dnzxy
left a comment
There was a problem hiding this comment.
Overall great enhancement / refactor based on Jonas' work for the NS serialzer.
I ran the serializer test suite on the PR head and all tests pass, no flakiness across runs.
I also backtested against Tidepool's integration env with a test account firing glucose/carbs/bolus/pump-event pipelines concurrently, in same shape as our triggers firing together. With a fresh token all uploads go through serialized, no spurious refresh.
With the session force-expired, the first op refreshes against Keycloak, everything still uploads, session stays intact. I verified via the TP's data API and on the dashboard that all records actually landed.
One caveat: TP's integration env seems to tolerate refresh-token reuse (two refreshes with the same token both return http status 200), so the #1235 corruption itself can't be reproduced there. I'm guessing their prod env is configured stricter. However, the client-side race this PR fixes is real either way: TAPI.refreshSession() suspends at three awaits, so you will receive concurrent 401s for all POST requests using the same refresh token. In other words, serializing our upload is the right fix.
| case .success: | ||
| debug(.service, "Success synchronizing pump events data. Upload to Tidepool complete.") | ||
| let pumpEventType = events.map { $0.type.mapEventTypeToPumpEventType() } | ||
| let pumpEventsToMark = events.filter { _ in pumpEventType.contains(pumpEventType) } |
There was a problem hiding this comment.
pumpEventType.contains(pumpEventType) compares the array against itself, so this filter is always true.
pumpEventsToMark always holds all fetched events, not just the ones that mapped to pump events.
So, if uploadDoseData fails but uploadPumpEventData succeeds, this marks the bolus/temp-basal rows isUploadedToTidepool = true even though their dose upload failed, and those doses are permanently dropped from Tidepool, as subsequent fetches will exclude them via the fetch predicate.
This is a pre-existing issue I only noticed through review + backtesting this PR against TP's integration environment, but since these lines moved anyway, let's fix it:
let pumpEventsToMark = events.filter { $0.type.mapEventTypeToPumpEventType() != nil } pairs each marking with the upload that actually carried it.
| } | ||
|
|
||
| private func begin(_ pipeline: TidepoolUploadPipeline, generation: Int) -> Bool { | ||
| guard isCurrent(generation) else { return false } |
There was a problem hiding this comment.
When the iOS watchdog abandons a chain, an operation queued behind (intentional) "throttle" the serializer adds, things will bail out here silently. For coalescing pipelines that's lossless, because every next pipeline trigger triggers re-fetches, but for the deletion pipelines the payload will be gone for good and the remote treatment on TP's end silently survives.
Accepting that trade-off seems fine from Trio's side (replaying deletes across an abandoned element has its own risks), but a log entry warning(.service, ...) here, at least when !pipeline.coalesces, would mayybe make the loss diagnosable in logs.
I think this is a super edge case.
| supercededDate: nil | ||
| )] | ||
|
|
||
| Task { [weak self] in |
There was a problem hiding this comment.
Wrapping enqueue in an unstructured Task means two rapid delete calls can race to the actor and enqueue out of call order (our old TP uploader serial processQueue was FIFO). Practically irrelevant for user-initiated deletes, I'll just note the behavior change (as certain operations in Trio may also lead to implicit deletions).
| $0.type == .tempBasal || $0.type == .tempBasalDuration || $0.type == .bolus | ||
| } | ||
| await self.updateInsulinAsUploaded(insulinEvents) | ||
| debug(.service, "TIDEPOOL DOSE ENTRIES: \(insulinDoseEvents)") |
There was a problem hiding this comment.
Existing behavior, but maybe worth removing: this dumps every single dose entry on every run of the uploader. Given this PR's log-tagging cleanup (thanks Mike!) consider trimming to a count ("TIDEPOOL DOSE ENTRIES: \(insulinDoseEvents.count)") maybe?
|
How‘s this one looking? |
The pump-event success path filtered with `pumpEventType.contains(pumpEventType)`, comparing an array against itself, so the predicate was always true and every fetched event was marked isUploadedToTidepool, including boluses and temp basals whose dose upload had just failed (that failure branch does not return). `pumpEventsNotYetUploadedToTidepool` then excludes them from every later fetch, so those doses never reach Tidepool at all. That made the loss routine rather than rare: the pump-event payload is always empty today, because the fetch maps only bolus and temp basal rows and neither of those maps to a pump event type, so the upload whose success did the marking always succeeded. Also log when an abandoned chain drops a non-coalescing operation, whose payload is not retried, and log a dose-entry count instead of dumping every entry's timestamp, amount and sync identifier.
I just pushed the changes. |
2 participants
Fixes #1235
Problem
Trio fired Tidepool uploads concurrently and unserialized: each pipeline's trigger (glucose, carbs, dose, settings observers) independently spawned a
Taskinto the shared TidepoolKitTAPIactor, so several uploads were routinely in flight at once.When the OAuth access token expired, multiple in-flight requests each passed
session.shouldRefresh()and calledrefreshSession()in parallel, reusing the single-use rotating refresh token. That race corrupted the session; subsequent upload completions never fired, so uploads stopped silently with nothing logged.Confirmed from the issue logs in #1235: last Tidepool success at 01:50:06, zero Tidepool errors after, app running continuously to 07:33; at 07:31 a dose reached the upload path but its completion never returned.
Nightscout uploads were unaffected (separate manager), and Loop avoids the bug by funneling uploads through one serialized
RemoteDataServicesManagercycle.Fix
A new
TidepoolUploadSerializeractor routes every upload (glucose, carbs, dose/pump events, settings, deletes) through a single chain so only one network request is in flight at a time. This guarantees at most one session refresh, eliminating the concurrent-refresh race, and a wedged request now resolves to a logged failure instead of silently stalling every later upload forever.Design points (several adopted from the Nightscout upload serializer in #1290):
awaitUploadbridges TidepoolKit's completion-based calls into async/await with a 120s timeout, so a call that never invokes its completion resolves to a logged.timedOutfailure instead of suspending the chain forever..utilityrather than inheriting the caller's priority —.utilitystays scheduled during background execution windows (.backgroundwould not), without running network uploads at UI priority.Also in this PR
[Nightscout], making Tidepool failures look like Nightscout activity; they're now tagged[Service], and carbs-delete results are worded distinctly from carbs uploads.Tests
TidepoolUploadSerializerTestscovers:awaitUploadreturns the completion's result, resolves to.timedOutwhen the completion never fires, and ignores a late completion after the timeout.