Repository navigation
feat: replace permanent event-delivery disable with a bounded cooldown - #256
tanderson-ld wants to merge 4 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.
Diagnostics obtain their delivery token through the same atomic cooldown-check-plus-issue call that analytics flushes use, so a delivery dispatched inside a window carries no token at all and every pre-window token goes stale when the window opens. All three in-cooldown guards are therefore uniformly defensive; one of them previously claimed to be reachable from in-window diagnostic failures.
MockEventSender enqueues the captured payload before it reads the result field, so a test that awaits the startup diagnostic and then immediately changes the sender's result can hand the new result to the still-running diagnostic task. The startup diagnostic then fails unexpectedly and opens the cooldown window itself, the test's own window-opening flush is discarded in-window, and the test times out waiting for it - the fake clock never advances, so the accidental window never elapses. Reproduced deterministically by widening the descheduling gap; seen once in CI. The three tests that change the result right after awaiting the startup diagnostic now wait for the diagnostic task to finish first, using the single-threaded executor barrier the file already uses elsewhere.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ 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 8358410. Configure here.
| checkIfErrorIsRecoverableAndLog(logger, e.toString(), errorContext, 0, nextActionMessage); | ||
| // Transport-level failures are always worth one retry here, whatever their cause; the | ||
| // request may simply have hit a transient network problem. | ||
| logger.warn("Error {} ({}): {}", errorContext, nextActionMessage, e.toString()); |
There was a problem hiding this comment.
Transport errors log unsanitized exceptions
Medium Severity
The new IOException warn path logs e.toString(), so OkHttp transport failures can write hostnames, URLs, or other private request details into customer logs. Nearby cooldown handling already uses LogValues.exceptionSummary for Throwables, and classifyAndLogTransportFailure sanitizes the same class of errors.
Triggered by learned rule: Log exceptions with LogValues.exceptionSummary, not toString/getMessage
Reviewed by Cursor Bugbot for commit 8358410. Configure here.
| // this worker thread: the busy-worker count would stay elevated forever and close() | ||
| // would never return. | ||
| logger.error("Unexpected error in event processor: {}", LogValues.exceptionSummary(e)); | ||
| logger.debug(LogValues.exceptionTrace(e)); |
There was a problem hiding this comment.
Null sender result skips failure recording
Low Severity
A custom EventSender that returns null still only trips the outer catch around handleResponse. That catch keeps the worker alive but never calls recordFailure, so the consecutive-success streak is left intact. The PR contract treats a null result as an ordinary failure that should break the reset threshold the same way a thrown sender does.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8358410. Configure here.
beekld
left a comment
There was a problem hiding this comment.
AI review: The description says the Android SDK's own DirectEventProcessor reads the legacy flag and keeps its permanent disable unchanged. Android's main branch has no DirectEventProcessor; it exists only on unmerged branches. Android main builds this module's DefaultEventSender and DefaultEventProcessor directly (ComponentsImpl.java), pinned at internal 1.9.0. So Android's next bump of this dependency picks up the cooldown and the ungated diagnostics without a coordinated change.
Relatedly, nothing in either repo reads the dual-written legacy flag. EventSenderWrapper only writes it, so it isn't a consumer that has not migrated.
| if (result.getTimeFromServer() != null) { | ||
| lastKnownPastTime.set(result.getTimeFromServer().getTime()); | ||
| } | ||
| if (result.isUnexpectedFailure()) { |
There was a problem hiding this comment.
AI review: A diagnostic failure can open a window, but a diagnostic success can never end one. Suppose a proxy or allowlist accepts /bulk but returns 403 or 404 for /diagnostic. Then every periodic diagnostic opens a new 5-minute window, and analytics that the service would accept are discarded in it. That's about a third of analytics at the default 15-minute diagnostic interval, and most of them at the 60-second minimum. The Go PR (launchdarkly/go-sdk-events#68) ignores diagnostic results, so it doesn't have this. Should a diagnostic failure open a window only while the analytics success streak is 0, or not at all?
| logger.error("Unexpected error in event processor: {}", LogValues.exceptionSummary(e)); | ||
| logger.debug(LogValues.exceptionTrace(e)); | ||
| } | ||
| synchronized (activeFlushWorkersCount) { |
There was a problem hiding this comment.
AI review: Both catches above take only Exception. If the sender or formatter throws an Error (such as AssertionError, NoClassDefFoundError, or StackOverflowError), the thread dies before it reaches this decrement. The count stays raised, and close() blocks forever in waitUntilAllFlushWorkersInactive. Main has the same problem, but the description says close() always returns. Moving the decrement into a finally would make that true.
| * @param token the token issued when this delivery was dispatched | ||
| */ | ||
| synchronized void recordFailure(WindowToken token) { | ||
| if (!isCurrent(token)) { |
There was a problem hiding this comment.
AI review: Because a stale failure returns here without touching the streak, the ladder can reset across a failure:
- The window elapses. The probe P1 and a sibling P2 are dispatched before either returns, so they share a token.
- P1 succeeds. That bumps the epoch, and the streak is 1.
- P2 fails. Its token is stale, so this returns early.
- P3 succeeds. The streak reaches 2, and
attemptsresets.
That's success, failure, success, in both dispatch and completion order. RETRY 1.8.3 says any failure restarts the reset-threshold count. The comment in recordSuccess assumes the next round fails; this is the case where it succeeds. recordUnexpectedFailure returns early the same way. Resetting consecutiveSuccesses before the staleness check in both methods would fix it, without letting a stale outcome open or step a window. The Go PR has the same code (launchdarkly/go-sdk-events#68).
| int outputEventCount = formatter.writeOutputEvents(payload.events, payload.summaries, writer); | ||
| writer.flush(); | ||
| EventSender.Result result = eventsConfig.eventSender.sendAnalyticsEvents( | ||
| result = eventsConfig.eventSender.sendAnalyticsEvents( |
There was a problem hiding this comment.
AI review: This sends without checking the cooldown. payloadQueue can hold a payload whose flush ran before the window opened, while every worker was busy. If a 401 opens the window before a worker takes that payload, the payload is still sent inside the window. Its outcome is stale, so cooldown state stays right, but the request goes out. EVTPROC 1.8.1 says no delivery attempt starts during a window. 1.8.5 is about flushes during the window, so it's unclear whether it covers a payload handed off before it. The Go PR does the same (launchdarkly/go-sdk-events#68). Is this intended?
| contextDeduplicator.flush(); | ||
| } | ||
| long remainingSeconds = Math.max(1, (cooldown.remainingMillis() + 999) / 1000); | ||
| if (cooldown.takeDiscardWarning()) { |
There was a problem hiding this comment.
AI review: takeDiscardWarning() is atomic on its own, but not together with the cooldown check in triggerFlush, which takes the lock separately. A new window can open between them: the old window elapses, a diagnostic dispatched after that gets a 401, and the new window re-arms the Warn. This discard, gated by the old window, then takes the new window's Warn, and the new window's own discards log at Debug. In the Go PR, the check, the re-arm, and the check-and-set all run on the dispatcher goroutine, so this can't happen there. The impact is small, but returning the first-discard flag from the same tracker call as the cooldown check would close the gap.
|
|
||
| /** | ||
| * Returns a result representing an ordinary delivery failure, such as a service outage, that | ||
| * does not affect the delivery schedule. |
There was a problem hiding this comment.
AI review: failure() does affect the delivery schedule: it breaks the success streak, and on a probe it ends the elapsed window (DeliveryCooldownTracker.recordFailure).
| * than while healthy. | ||
| * <p> | ||
| * The tracker is pure state: it starts no tasks and does no I/O or logging. Deliveries are | ||
| * dispatched from the main dispatcher thread while their outcomes arrive on delivery worker |
There was a problem hiding this comment.
AI review: Not only from the dispatcher thread: diagnostic deliveries get their tokens on the shared executor (createSendDiagnosticTask in DefaultEventProcessor).


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
Note
Overview
Replaces permanent analytics shutdown after errors like HTTP 401 with a bounded, self-recovering cooldown driven by a new
DeliveryCooldownTracker(5→10→20→40→60 minute windows, reset after two consecutive successes).DefaultEventProcessorgates flushes through the tracker: in-window flushes discard buffered batches without network I/O (including context-deduplicator flush for index/identify discards), correlate outcomes viaWindowToken, and keep diagnostics sending while only unexpected diagnostic failures open the window. Delivery workers now treat sender exceptions and null results as ordinary failures without wedgingclose().EventSender.Resultaddsunexpectedclassification (success/failure/unexpectedFailure,isUnexpectedFailure()), with legacymustShutDowndeprecated but still honored.DefaultEventSenderusesclassifyHttpFailureso unexpected HTTP statuses backoff instead of implying permanent shutdown.Extensive unit tests cover the tracker, processor cooldown behavior, and updated diagnostic/HTTP sender expectations.
Reviewed by Cursor Bugbot for commit 8358410. Bugbot is set up for automated code reviews on this repo. Configure here.