Repository navigation
feat(events): add flushAndWait on Apple (tier 2) - #536
abelonogov-ld wants to merge 27 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
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>
…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?
There was a problem hiding this comment.
it is only way without asking customer setup xcode entitlements or asking customer to add new methods into AppDelegate. Customer can use those advanced methods itself with help of flushAndWait as well.
| /// 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.
There was a problem hiding this comment.
Now it is hasPendingFlush
There was a problem hiding this comment.
Renamed to hasPendingFlush / pendingFlushCompletions, matching Android's pendingFlush
There was a problem hiding this comment.
replaced by queuedDeliverything
| // 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.
There was a problem hiding this comment.
Yes, it is not ideal
Tier 3 moved recording off the queue entirely during optimization work. It renamed eventQueue to deliveryQueue.
There was a problem hiding this comment.
Also now it is testable
| } | ||
| 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?
There was a problem hiding this comment.
Yes, it is usually a norm but not in every company :)
There is standard practice to use extension LDClient { to split code into parts, here flush functionality.
In this repo we already did that but only with creating new files. Gonna follow existing precedent with LDClientIdentifyHook.swift and LDClientVariation.swift
There was a problem hiding this comment.
Created LDClientFlush.swift
| 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.
There was a problem hiding this comment.
- Now an un-encodable batch returns
false:encodeanswering nil short-circuits before - Worth a follow-up if you think it matters, tier 3 reports these losses explicitly.
| /** | ||
| 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.)
There was a problem hiding this comment.
The ObjC doc had also drifted from the Swift one, so I brought them back into lockstep — it was missing the
paragraph on the timeout bounding the wait rather than the delivery, and the - parameter timeout: line was a
single clause against Swift's. They're now identical apart from true/false versus YES/NO.
| 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.
There was a problem hiding this comment.
Both fixed. There is now one absolute deadline computed once up front, and every wait is against that same
instant, so the worst case is the timeout the caller asked for however many environments there are the
per-environment +0.25s grace is gone, and so is the TimeoutExecutor indirection that produced it.
| 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.
There was a problem hiding this comment.
- On tier 3 this is a contract change for anything that assumed completions were serialized on eventQueue. Tier 3's delivery state is confined to deliveryQueue, so I'll need to re-check that when merging forward.
- It is gone with the single-flight gate. A delivery that has already started is no
longer joined, so an answer is always about the batch the caller's own delivery took, never one an earlier
delivery lost.
| 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.
…lush into LDClientFlush.swift Co-authored-by: Cursor <cursoragent@cursor.com>
Also gates the background flush to platforms that suspend, holds one background assertion per client, and reports flushAndWait's outcome as Android does: true only when LaunchDarkly accepted the events. Co-authored-by: Cursor <cursoragent@cursor.com>
A delivery that had started held back every later one, including the periodic timer's, until it answered. On the retryable path that is about twice the connection timeout, and nothing emptied the event store for the whole of it, so a busy application on a flaky network lost events at the capacity it was supposed to be buffered by. Deliveries now coalesce only while queued and not yet started, which is what Android's DirectEventProcessor does: runDelivery clears the queued waiters before it takes the store, so anything arriving after that gets a delivery of its own. A flush still joins one that has not started, since that delivery will carry its events too. An answer is now always about the batch the caller's own delivery took. That retires the inherited failure a waiter used to pick up from the in-flight batch, which could report events lost that had in fact landed. Completions are also answered off the reporter's queue, uniformly. They had been split between it and whichever thread the delivery ended on, so recording from a completion deadlocked, but only when the event store happened to be empty. Also from review: - Report false from flushAndWait for a client a later start replaced, rather than another client's outcome. - Answer publishEventData where there is no mobile key, rather than dropping the completion. - Keep a flush nobody waits on from adding a waiter. - Hold no assertion around work that does not need one in BackgroundActivity, which drops the refusal case entirely. - Match the Objective-C flushAndWait doc to the Swift one, and say on both that a definitive failure is reported as a failure, since other LaunchDarkly SDKs report it as a success. - Guard the event-publish state in DarklyServiceMock, which concurrent deliveries now reach from more than one thread. Co-authored-by: Cursor <cursoragent@cursor.com>
flushAndWait is the only way to learn whether events reached LaunchDarkly, and it blocks. From a Swift concurrency task that is not safe: the cooperative pool does not grow to replace a thread held that way, so enough of these stall every other task in the process, including whichever one would have finished the delivery. flush(completion:) reports the same outcome over every environment and returns at once. It takes no timeout, because a delivery always ends by its own request timeouts if nothing else, and the caller is not waiting on it in any case. The documentation on all three now says which to reach for, and flushAndWait says outright that it blocks the calling thread and must not be called from a task. Co-authored-by: Cursor <cursoragent@cursor.com>
…efusal The assertion the process grants was taken directly from ProcessInfo, so nothing could exercise the paths that matter: a system that refuses one outright, and the cap on work that never finishes. The backgrounding test in LDClientSpec was also at the mercy of whether the simulator felt like granting one. processAssertion takes the system call and the cap as arguments, so a test can refuse an assertion, grant one, or let it run past the cap, without the system's cooperation. Three tests cover those, and the refusal one fails if the work is moved back inside the activity block, which is where it would never run under the resource pressure this is here for. The refusal is now logged as well. The delivery goes ahead either way, so it is no longer silent, but a log could not tell a held assertion from one that was never granted. Co-authored-by: Cursor <cursoragent@cursor.com>
…-durability-tier2-bounded-flush Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # LaunchDarkly/LaunchDarkly/LDClient.swift
…-durability-tier2-bounded-flush
| os_log("%s called on a closed client", log: config.logger, type: .debug, self.typeName(and: #function)) | ||
| completion(false) | ||
| return | ||
| } |
There was a problem hiding this comment.
Flush completion runs on caller
Medium Severity
flush(completion:) documents that the completion always runs on a background queue, not the caller’s thread, so it is safe to record or flush again. A closed client and NullEventReporter (sendEvents false) invoke that completion synchronously on the caller instead, which can deadlock a caller that hops back onto its current queue.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d5215f5. Configure here.
…-durability-tier2-bounded-flush
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
There are 3 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 f6cd58c. Configure here.
| } | ||
| finished() | ||
| } | ||
| } |
There was a problem hiding this comment.
Background flush skips in-flight delivery
High Severity
Backgrounding calls flushReportingOutcome, which does not join a delivery that has already taken the store. If events are on the wire or in the retry delay, the store is empty, the new flush answers true at once, and finished() drops the assertion. The process can then suspend and kill that in-flight request. Those events are already gone from the store, so they are lost, and the “did not reach LaunchDarkly” log does not fire. That hole is widest during the retry window this change otherwise went out of its way to keep draining.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f6cd58c. Configure here.
There was a problem hiding this comment.
Tier 3 (#538), which sits directly on this branch, narrows that without relying on the disk. A delivery no longer takes events out of the store: claimDeliverableBatches only marks payload IDs in batchesInFlight, and remove runs solely once LaunchDarkly has accepted or permanently refused the batch. With persistence off that batch sits in inMemoryBatches until then, so a suspension that is later resumed redelivers it. Neither branch ships on its own.
|
|
||
| func flushReportingOutcome(completion: @escaping FlushOutcomeClosure) { | ||
| completion(true) | ||
| } |
There was a problem hiding this comment.
Flush completions run on caller thread
Medium Severity
NullEventReporter.flushReportingOutcome (used when sendEvents is false) and the closed-client path of flush(completion:) invoke the completion on the caller’s thread. The API promises a background queue that is neither the caller nor the reporter queue, so callers are told they may record or flush again. A caller who follows that and re-enters the same serial queue or lock deadlocks. The same sync answer also lets flushAndWait(timeout: 0) return true when events are disabled, despite the documented “zero does not wait, reports false” rule.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f6cd58c. Configure here.
There was a problem hiding this comment.
NullEventReporter.flush and .flushReportingOutcome, and the closed-client path of LDClient.flush(completion:), now answer on a background queue. Fixed on tier-3
…-durability-tier2-bounded-flush
…-durability-tier2-bounded-flush Co-authored-by: Cursor <cursoragent@cursor.com>
On macOS processAssertion runs the work inline and never asks the system, so the specs that wait for the system to end the assertion timed out there and failed test-swiftpm. macOS gets a spec for its own behavior instead. Co-authored-by: Cursor <cursoragent@cursor.com>
…-durability-tier2-bounded-flush Co-authored-by: Cursor <cursoragent@cursor.com>


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