Skip to content

fix: Use monotonic clocks for migration latency and FDv2 stream-init timing - #539

Open
tanderson-ld wants to merge 1 commit into
mainfrom
tanderson/sdk-3307-monotonic-clock
Open

tanderson-ld wants to merge 1 commit into
mainfrom
tanderson/sdk-3307-monotonic-clock

Conversation

@tanderson-ld

@tanderson-ld tanderson-ld commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Replaces wall-clock time with monotonic time in the two places this SDK still measures a duration from one, found by the clock-monotonicity re-audit. Implements SDK-3307 (re-audit findings #39/#40/#41).

A wall-clock step (DST via naive datetime.now(), NTP correction, VM/container time resync) previously could:

  • Migration op latency (migrations/migrator.py, async_migrator.py): datetime.now() is naive local time, so a DST transition reported ±1h (or negative) latency to LaunchDarkly, plus any NTP/manual step. Finding added config option for events_uri #39 (HIGH).
  • FDv2 stream-init diagnostics (datasourcev2/streaming.py, async_streaming.py): the duration was a wall-clock difference hidden behind an elapsed if elapsed >= 0 else 0 clamp, and the next attempt's baseline was a projected future wall time (time() + next_retry_delay) that produced short/negative durations even without a clock step. Findings Send the toggle default value with flag request events #40 / coerce user keys to strings #41 (MED).

Changes

  • Impl::util.monotonic_seconds() is the single monotonic seam (added in the sibling SDK-3105 work; reused here).
  • Migration latency is measured with monotonic_seconds() and delivered to OpTracker.latency as a timedelta — identical type and, under a stable clock, identical value; a backward step can no longer produce a negative latency.
  • FDv2 stream-init duration is measured monotonically from the real connection moment (the SSE Start action) instead of a projection; the wall clock is kept only for the reported timestamp. The projected next_attempt_start field is removed from StreamErrorDecision (and classify_stream_error no longer takes next_retry_delay), and the teardown paths (clean close, unexpected event error) clear the attempt baseline so a later reconnect failure can't record a duration spanning the backoff sleep.
  • _DiagnosticAccumulator.record_stream_init clamps durationMillis to a non-negative integer once, centrally, for every caller (the per-site clamp is removed).

Deliberately unchanged (wall clock is correct): DataSourceErrorInfo.time, event creationDate, diagnostic dataSinceDate, the big-segments cross-process staleness stamp.

Behavioral notes

Not a breaking change — every modified component is Impl-internal, and behavior under a stable clock is unchanged. Note for test suites that time-travel: these measurements no longer follow time.time(); freezegun does not patch time.monotonic() by default.

Known limitation tracked separately

There is a known FDv2 diagnostic-count gap (failed stream-init records undercounted during a sustained outage) that is not addressed here — see the tracking comment on this PR. It is diagnostics-only, affects no flag evaluation, data correctness, or retry behavior, and will be handled on its own.

Testing

Full suite passes (1775 passed, 371 DB-skipped). New tests: migration latency measured monotonically (sync + async, via a module-patched clock); FDv2 stream-init duration measured from the Start action rather than run-start or a projection; a clean-close-mid-init regression test proving no stale baseline survives into a later reconnect; and the diagnostic accumulator's non-negative clamp. mypy, isort, and pycodestyle clean.

Same change-class as ruby-server-sdk#459 and the merged python PR #537.


Note

Overview
Switches duration measurements from wall clock to monotonic time so DST, NTP, and manual clock steps no longer corrupt telemetry.

Migration operations (sync and async executors) now time latency with monotonic_seconds() and report a timedelta to OpTracker.latency instead of naive datetime.now() deltas.

FDv2 streaming diagnostics record stream-init duration from the monotonic baseline stamped on each SSE Start (wall clock is still used only for the diagnostic timestamp). The old projected next_attempt_start path is removed from classify_stream_error / StreamErrorDecision; error and clean-close paths clear the attempt baseline so reconnect/backoff cannot inflate the next measurement. _DiagnosticAccumulator.record_stream_init clamps negative durations in one place.

New tests cover monotonic migration latency, stream-init timing regressions, stale-baseline cleanup, and the diagnostic clamp.

Reviewed by Cursor Bugbot for commit 4cef86b. Bugbot is set up for automated code reviews on this repo. Configure here.

…timing

Wall-clock steps (DST transitions via naive datetime.now(), NTP
corrections, VM/container time resync) could report wildly wrong or
negative migration-operation latencies, and could inflate or zero out
the FDv2 stream-init durations reported in diagnostic events.

Migration latency (sync and async) is now measured with a monotonic
clock and delivered to the tracker as a timedelta, unchanged in type.
FDv2 stream-init duration is measured monotonically from the real
connection moment (the SSE Start action) rather than a wall-clock
projection of the next attempt; the wall clock is kept only for the
reported timestamp. The projected next-attempt baseline is removed, and
the diagnostic accumulator now clamps durationMillis to a non-negative
integer once for every caller. Teardown paths (clean close and
unexpected event errors) clear the attempt baseline so a later reconnect
failure cannot report a duration spanning the backoff sleep.

SDK-3307
@tanderson-ld
tanderson-ld requested a review from a team as a code owner October 16, 2026 15:45
@tanderson-ld

Copy link
Copy Markdown
Contributor Author

AI review comment: tracking a known, deliberately-deferred diagnostics gap introduced alongside this change, so it isn't lost.

FDv2 failed stream-init records are undercounted during a sustained outage

What. The FDv2 streaming synchronizers record a failed stream-init only when the connection attempt's start is observable. The initial connect's start is observable (stamped at sync() entry) and a connected-then-failed init is observable (stamped at the SSE Start), but a library-owned reconnect that fails to connect emits a Fault(error) with no preceding Start, so its start is unobservable and it is not recorded. During a sustained outage of N consecutive connect failures, roughly 1 of N failed inits is recorded instead of N.

Spec angle. The DIAG diagnostic-events spec calls for one stream-init entry per connection attempt, so this is a conformance gap, not merely reduced telemetry — and it undercounts precisely when failed-init data is most useful.

Regression vs. pre-existing. This gap is introduced by this PR's move to observed-Start stamping. The previous code re-armed the baseline with a projected next-attempt wall time, so it recorded every attempt — but with durations that were frequently zero or negative (the defect this PR fixes). So the prior behavior was count-correct / duration-broken; this PR is duration-correct / count-incomplete. Fixing both at once is what the options below address.

Scope / severity. Diagnostics-only. No impact on flag evaluation, data correctness, connection/retry behavior, or the FDv1-fallback latch. Medium as a spec-MUST gap; Low as pure observability.

Options for a future fix.

  1. Accept and document the undercount.
  2. Record every attempt once, with the real monotonic duration when the attempt start is observable and a zero duration when it is a library-owned reconnect. This needs an explicit "init in progress" flag to distinguish a genuine failed attempt from a drop of an already-successful connection (otherwise every healthy mid-stream drop would be mis-logged as a failed init). An implementation of this was prototyped and set aside to keep this PR's scope focused.
  3. Migrate the FDv2 synchronizers to the eventsource library's SDK-managed-retry mode, so the SDK owns the backoff sleep and can observe every attempt start directly (full fidelity; the largest change). Belongs with the eventsource follow-up work.

Not addressed in this PR by design.

@tanderson-ld

Copy link
Copy Markdown
Contributor Author

Need to address pending issue related to undercounting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant