feat(events): add flushAndWait on Apple (tier 2) - #536
abelonogov-ld wants to merge 15 commits into
Conversation
A bounded flush spends up to a caller-supplied budget trying to deliver, then reports whether the events left the SDK's hands. Android already had this; Apple now has LDClient.flushAndWait(timeout:), spelled in seconds to match identify and start. EventReporting.flushReportingOutcome is the seam: true when the events were delivered, refused for good, or there were none; false when the SDK is offline or a retryable failure means they are still waiting. Saying that needed the response classification to be a decision rather than a Bool, so processEventResponse now returns settled, retryable, or dropped -- the three outcomes that were already implied by its two callers. TimeoutExecutor bounds the wait. Completions run on a queue that is neither main nor the reporter's delivery queue, so a lifecycle caller on the main thread cannot deadlock waiting for itself. Backgrounding now spends its last moment trying to deliver, inside a ProcessInfo activity assertion so the system does not suspend the process out from under a request that has only just reached the network. This is not a crash-time API. The SDK has no crash hook of its own on Apple, and a Mach exception handler is not a supported caller. Spec: Event Durability §10, §11, §17.3. Co-authored-by: Cursor <cursoragent@cursor.com>
…-durability-tier2-bounded-flush # Conflicts: # LaunchDarkly/LaunchDarkly/ServiceObjects/EventReporter.swift
…-durability-tier2-bounded-flush
…-durability-tier2-bounded-flush
…-durability-tier2-bounded-flush
…-durability-tier2-bounded-flush Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # LaunchDarkly.xcodeproj/project.pbxproj
…-durability-tier2-bounded-flush
…-durability-tier2-bounded-flush Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # LaunchDarkly/LaunchDarkly/LDClient.swift
…-durability-tier2-bounded-flush
…-durability-tier2-bounded-flush
…-durability-tier2-bounded-flush
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
A flush starting while a delivery was on the wire found an empty event store -- the events had left it when the request was sent -- and reported success for events that could still fail. Backgrounding is where that mattered: the hook released its activity assertion as soon as it heard success, letting the system suspend the process out from under the very request it was holding the assertion for. A delivery now claims the reporter for the length of its round trip, and a flush arriving inside that window is answered by the pass that follows it, with the in-flight outcome folded into its answer. A tier 2 failure drops the events rather than leaving them for that pass to retry, so a waiter told only about the pass would hear success for events that are gone. internalFlushAndWait read its result after a wait that had already expired, which only the semaphore's signal orders against the write. BackgroundActivity's documentation claimed undelivered events were already on disk. Nothing is written until tier 3. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit dc9d1e6. Configure here.
…correctly from flushAndWait Co-authored-by: Cursor <cursoragent@cursor.com>
…-durability-tier2-bounded-flush
|
Current version of PR was reviewed by /review-bugbot on Oct 1, 13:41 PDT. It flagged 0 findings. Bugbot on commit |
| /// | ||
| /// Uses `ProcessInfo.performExpiringActivity` because the framework is extension-safe and cannot use | ||
| /// `UIApplication.beginBackgroundTask`. The system may refuse or end the assertion early. | ||
| enum BackgroundActivity { |
There was a problem hiding this comment.
Is a background activity the only option here? Is there no background service that can run for a few seconds after the application backgrounds?
| /// True from sending a request until its response is handled. Its events have already left `eventStore`. | ||
| private var isDelivering = false | ||
| /// A delivery was requested while one was in flight. | ||
| private var hasWaitingRequest = false |
There was a problem hiding this comment.
We have used something like pendingRequest in other implementations. The word "pending" has reasonable precedent for this situation.
| // A terminal response takes the reporter offline after settling; with nothing left, that is not a failure. | ||
| let nothingPending = eventStore.isEmpty && !contextSummarizer.hasLoggedRequests | ||
| reportEvents { result in | ||
| waiting.forEach { $0(delivered && (result || nothingPending)) } |
There was a problem hiding this comment.
Is it ok to call these while inside self.eventQueue.async ? It probably could be fine if they are short.
| } | ||
| finished() | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need to put any rate limiting in place here? Could this pathway allow an SDK to make requests more rapidly than it did previously in such a way that could lead to more load on the servers?
There was a problem hiding this comment.
Matching Android is probably sufficient.
| extension LDClient: TypeIdentifying { } | ||
|
|
||
| // MARK: - Event delivery | ||
| extension LDClient { |
There was a problem hiding this comment.
Why an extension in the same file as the LDClient impl? Is this a swift norm for organization?
| else { | ||
| os_log("%s Failed to serialize event(s) for publication: %s", log: service.config.logger, type: .error, typeName(and: #function), String(describing: events)) | ||
| completion?() | ||
| // Encoding is deterministic, so no retry would succeed. |
There was a problem hiding this comment.
The batch was already removed from eventStore, so an un-encodable batch is unrecoverable data loss — reported as true. "No retry would succeed" justifies not retrying, not reporting success; and the partial case has the same shape (encodeSkippingFailures silently drops individual events and the surviving batch still settles as true). This is reachable from app data: a NaN/±inf Double via track's metricValue/data or a context attribute makes the JSON writer fail, and track does no finite-number validation. A caller using flushAndWait to decide "are my events safe?" gets true for events that were destroyed. .dropped → false seems like the more honest classification for this path — or validate at track time so it can't arise.
| /** | ||
| Sends any currently queued events and waits up to `timeout` for the delivery to finish. | ||
|
|
||
| Returns `YES` if the events were delivered, or there were none to deliver. Returns `NO` if the timeout |
There was a problem hiding this comment.
This doc doesn't match the implementation on two counts: (1) a terminal 401/403 drops the batch, takes the reporter offline, and returns YES (it classifies as .settled) — squarely "otherwise unable to deliver them"; (2) once offline, flushAndWait returns NO even with nothing pending, contradicting "or there were none to deliver" (the offline guard runs before the empty-store check, and a terminal response makes that sticky for the rest of the process). The behavior is defensible and matches the flushReportingOutcome contract; these sentences (and the Swift doc's "left the SDK's hands") are what need to change.
Also worth deciding deliberately: .NET's FlushAndWait counts "definitively failed" as true, so our .dropped → false diverges. Either semantic is fine, but we should pick one on purpose and document it on both APIs. (Nit: a - returns: line here would match the Swift side.)
| let deadline = Date().addingTimeInterval(max(0, timeout)) | ||
| var delivered = true | ||
| for client in clients { | ||
| let remaining = max(0, deadline.timeIntervalSinceNow) |
There was a problem hiding this comment.
Two issues with the shared budget: (1) each environment adds its own +0.25s grace (finished.wait(timeout: .now() + timeout + 0.25)), so the real worst case is timeout + 0.25×N, not the documented single budget; (2) once the budget is exhausted, later environments run with remaining == 0, where the TimeoutExecutor fallback (asyncAfter(.now())) essentially always beats the reporter's hop to eventQueue — they report false even with nothing to send, and the && drags the whole result down, in nondeterministic Dictionary.values order. Consider computing the slack once against the shared deadline (or running environments concurrently against it), and short-circuiting once the budget is spent.
| func recordFlagEvaluationEvents(flagKey: LDFlagKey, value: LDValue, defaultValue: LDValue, featureFlag: FeatureFlag?, context: LDContext, includeReason: Bool) | ||
| func flush(completion: CompletionClosure?) | ||
|
|
||
| /// Like `flush`. Reports `true` if events were delivered, permanently refused, or absent; `false` if offline or |
There was a problem hiding this comment.
Two things worth capturing in this contract while it's being defined: (1) completions run on the reporter's serial eventQueue, so a completion that re-enters the reporter (e.g. record, which does eventQueue.sync) deadlocks — no current caller does, but tier 3 should know the queue contract; (2) a waiter queued behind an in-flight delivery inherits that batch's failure even if its own events succeed in the follow-up pass (delivered && (result || nothingPending)). Conservative is the right direction, but a caller treating false as "my events are still buffered, safe to re-send" will duplicate events that actually landed — worth a sentence here and on flushAndWait.
| var publishedEventData: Data? | ||
| /// Holds responses so a test can act while a delivery is in flight. | ||
| var holdsEventCompletions = false | ||
| private var heldEventCompletions: [ServiceCompletionHandler] = [] |
There was a problem hiding this comment.
holdsEventCompletions/heldEventCompletions (and the existing call-count/data fields) are mutated from DispatchQueue.global() — and the retry's queue — while the test thread writes the flag and clears the array in releaseHeldEventCompletions, with no synchronization. A concurrent append and = [] can corrupt the array; this will flake CI and light up TSan. A small lock or serial queue around the event-publish state would do it.
This branch has not been deployed
2 participants
Tier 2 of the mobile event durability work. It stacks on tier 1 (#530), and nothing here persists events; that is tier 3.
Requirements
Related issues
Stacked on #530. Android already has the equivalent
LDClient.flushAndWait(long, TimeUnit).Describe the solution you've provided
A bounded flush:
LDClient.flushAndWait(timeout:)spends up to a caller-supplied budget trying to deliver, then reports whether the events left the SDK's hands. Seconds, to matchidentifyandstart;ObjcLDClient.flushAndWait(timeout:)exposes it to Objective-C.EventReporting.flushReportingOutcomeis the seam. It reports true when the events were delivered, refused for good, or there were none, and false when the SDK is offline or a retryable failure means they are still waiting.Bool.processEventResponsereturns settled, retryable, or dropped — the three outcomes its two callers already implied — which is what lets the flush say which one happened.TimeoutExecutorbounds the wait, and completions run on a queue that is neither main nor the reporter's delivery queue, so a lifecycle caller on the main thread cannot end up waiting for itself.ProcessInfo.performExpiringActivityassertion (BackgroundActivity), so the system does not suspend the process under a request that has only just reached the network.UIApplication.beginBackgroundTaskis not available because the framework is built extension-safe.Describe alternatives you've considered
Boolfrom the response handler. It could not distinguish a permanent refusal (settled, nothing to retry) from a retryable failure, and the flush has to tell them apart.Additional context
Spec: Event Durability §8–§12 (recoverable failure: bounded flush, response classification, lifecycle hooks, and the honesty requirement that
flushAndWaitis not a promise under termination).Made with Cursor