Repository navigation
feat: replace permanent event-delivery disable with a bounded cooldown (SDK-3375) - #256
Draft
tanderson-ld wants to merge 2 commits into
Draft
tanderson-ld wants to merge 2 commits into
tanderson-ld wants to merge 2 commits into
Conversation
…sifier DefaultEventSender now classifies HTTP failures with HttpErrors.classifyHttpFailure instead of the deprecated isHttpErrorRecoverable/checkIfErrorIsRecoverableAndLog helpers. Behavior and log output are unchanged: the set of statuses treated as unexpected is identical, unexpected failures still skip the retry and still log at error level with the same wording, and transport-level exceptions keep their warn-and-retry-once handling.
…n (SDK-3375) On an unexpected delivery failure (a non-recoverable HTTP status such as 401 or 403), the event processor now enters a bounded, self-recovering cooldown window instead of permanently disabling delivery for the lifetime of the SDK instance, per the EVTPROC spec's Section 1.8. Windows double from 5 minutes to a 1 hour ceiling, one step per failed probe. In-window flushes of any kind are no-ops that dispose of the batch exactly as a sent-and-refused payload would be, without the network or serialization cost. Recording is never gated, retry state resets after two consecutive successful analytics deliveries, and no cooldown state persists across restarts. Diagnostic sends are not gated by the window; an unexpected diagnostic failure opens one, as it previously disabled delivery, but diagnostic successes do not drive the delivery schedule. EventSender.Result gains the unexpected classification with factory methods and the isUnexpectedFailure accessor; the legacy constructor and isMustShutDown are deprecated and still dual-written for consumers that have not migrated.
This branch has not been deployed
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.
What
Implements the EVTPROC event processor spec's Section 1.8 delivery cooldown (launchdarkly/sdk-specs#278) in the shared internal events code: on an unexpected delivery failure (a non-recoverable HTTP status such as 401 or 403), the event processor enters a bounded, self-recovering cooldown window instead of permanently disabling event delivery for the lifetime of the SDK instance.
flushAsync()/flushBlocking(), or the shutdown flush) is a no-op: it makes no network attempt and disposes of the batch exactly as a sent-and-refused payload would be disposed of. Two deliberate departures from pure refusal emulation: the diagnostic events-in-last-batch stat reports 0 (no batch was serialized), and the context deduplicator is flushed when the discarded batch contained an index or identify event, so a later payload can introduce those contexts again.DefaultEventSender(1.7.2) is unchanged.This is a port of the reviewed Go implementation (launchdarkly/go-sdk-events#68), adapted to this module's threading model: delivery outcomes arrive on flush worker threads and the shared executor rather than on the dispatcher thread, so the cooldown tracker is synchronized pure state, and outcomes are correlated to the window era they were dispatched in via an opaque
WindowToken(an outcome from a delivery in flight across a window boundary updates nothing).API (internal module surface)
EventSender.Resultgains theunexpectedclassification with static factoriessuccess(Date),failure(), andunexpectedFailure(), and the accessorisUnexpectedFailure(), which also accepts the legacy flag so results from senders written against the old API still get cooldown behavior.Resultconstructor andisMustShutDown()are deprecated.unexpectedFailure()keeps writing the legacy flag for consumers that have not migrated (the server SDK'sEventSenderWrapperand the Android SDK).DefaultEventSendermigrates off the deprecatedisHttpErrorRecoverable/checkIfErrorIsRecoverableAndLoghelpers toclassifyHttpFailure(first commit, behavior-identical) and then returns the new classification (second commit). Transport-levelIOExceptions keep their always-retry behavior.EventsConfigurationgains a package-privatecooldownNanoTimeFntest seam (deliberately not part of any constructor); the tracker documents that the source must haveSystem.nanoTime()semantics, and all window arithmetic uses overflow-safe difference comparisons.Design notes
Dateheader remains unconditional, exactly as on main.disabledflag remains as a shutdown-only latch; nothing sets it from delivery results anymore.close()always returns.NanoTimeSourceinterface rather thanjava.util.function.LongSupplier, which does not exist on Android below API 24 (this module ships in the Android SDK at minSdk 21); the fix is verified with ad8 --min-api 21dex sweep showing zerojava.util.functionreferences.Testing
DeliveryCooldownTrackerTest(24 tests): window ladder with every rung asserted past the shift-guard bound, reset semantics, boundary-round staleness on both probe success and recoverable probe failure, stale-outcome and in-window-outcome guards, null token, default clock path, boundary instant, the atomic gate, and the once-per-window discard notice.DefaultEventProcessorCooldownTest(18 tests): cooldown entry and resume across a fake clock, legacy shutdown-flag results, contradictory success+shutdown results, in-window disposal with context-introduction recovery through both the index and identify arms, summary-only discards leaving the deduplicator alone, empty in-window flushes doing nothing, shutdown during a window, warn-once-then-debug and resume logging, the diagnostic coupling (including a diagnostic held in flight across the window boundary), a null sender result neither stopping delivery nor wedgingclose(), a sender exception breaking the success streak, simultaneous failures across the full worker pool stepping the ladder exactly once, and the batch-size stat zeroing on discards.diagnosticEventsStopAfter401Erroris rewritten asdiagnosticEventsContinueAfter401Error; the permanent-disable test is renamed to describe the cooldown contract; a transport-failure sender test pins the deliberate never-unexpected classification of connection errors.Out of scope (follow-ups)
classifyHttpFailurehas no locally-unrecoverable classification, so a 413 opens a window rather than dropping only the oversized payload. That is a pre-existing classifier gap with its own Jira bug (it also requires unsuppressing the corresponding contract test), deliberately not fixed here.EventSender.Result.STOPdoc reinterpretation, wrapper migration,event-delivery-cooldowncontract-test capability, and the 413 test suppression) ship in a follow-up PR once this module releases.DirectEventProcessorreads the dual-written legacy flag and keeps its current permanent-disable behavior byte-identical before and after this release; migrating it to the cooldown is its own coordinated change.Release notes