adapter: migrate LaunchDarkly SDK off fork to upstream 3.1.1#37025
adapter: migrate LaunchDarkly SDK off fork to upstream 3.1.1#37025jasonhernandez wants to merge 1 commit into
Conversation
16214c3 to
1ea2298
Compare
f641120 to
20b09fd
Compare
20b09fd to
8b9df3d
Compare
| fn main() { | ||
| // Both the aws-lc-rs and ring rustls backends are linked, so rustls can't | ||
| // auto-select a provider and panics on first TLS use unless one is installed. | ||
| let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); |
There was a problem hiding this comment.
Do we know what is pulling in ring? Can we remove it?
It seems to be pulled in by rustls, but that's probably only because we have some feature enabled by something else.
def-
left a comment
There was a problem hiding this comment.
QA LLM review notes:
MEDIUM — SSE staleness metric updates on connection establishment (200 head), not just on received data, which can suppress the launchdarkly-stale-sse-tier-2 alert this migration exists to enable
File: src/adapter/src/config/frontend.rs, MetricsTransport::request (the
gauge.set(now_fn() / 1000) call before the body is wrapped).
What the code does
Box::pin(async move {
let resp = inner_fut.await?;
if resp.status().is_success() {
gauge.set(now_fn() / 1000); // (A) fires on the 200 response HEAD
let (parts, body) = resp.into_parts();
let wrapped: ByteStream = Box::pin(body.inspect_ok(move |_| {
gauge.set(now_fn() / 1000); // (B) fires on each received body chunk
}));
Ok(http::Response::from_parts(parts, wrapped))
} else {
Ok(resp)
}
})The same MetricsTransport type is used for both the event processor (CSE metric,
last_cse_time_seconds) and the streaming data source (SSE metric,
last_sse_time_seconds). For the CSE (request/response) path, update (A) on the
200 head is correct. For the streaming/SSE data source, update (A) is the
problem.
Why it is a defect
The metric's documented contract (doc/user/data/metrics.yml:1339,
src/adapter/src/config.rs:161) is:
"The last known time when the LaunchDarkly client received an event from
the LaunchDarkly server."
The pre-migration code honored that exactly: last_sse_time_seconds was set only
from the per-event callback (frontend.rs:383-390, move |_ev| { ... set(ts) }),
i.e. only when an SSE event was actually received. The new code additionally
sets the gauge whenever a streaming connection is established (200 head),
which is not "received an event."
This matters because the whole reason for this migration is that the upstream SDK
now reconnects on non-Eof stream errors (the incident-984 fix). Consider the
persistent version of the incident-984 failure mode — LD returns 200 OK, then
the body times out before (or without) delivering usable data
(hyper::Error(Body, Kind(TimedOut))), repeatedly:
- Reconnect →
request()called again → 200 head → update (A) sets the gauge tonow. - Body yields no chunk (times out after the configured
read_timeoutof 300s) → update (B) never fires. - SDK reconnects (backoff) → back to step 1 → gauge set to
nowagain.
So a data source that is receiving zero actual events keeps its
last_sse_time_seconds advancing roughly every read_timeout + reconnect_backoff
(~300s+). The old code would have frozen the gauge at the last real event, letting
the staleness alert fire. The new head-update re-arms the "freshness clock" on
every reconnect, so a persistently wedged/reconnecting data source can look fresh
and the launchdarkly-stale-sse-tier-2 alert — the detection this PR is built to
preserve — may never fire.
Note that update (A) is also unnecessary for the healthy SSE case: on a good
connection LD immediately sends the put (full flag state) as the first body
chunk, and sends periodic heartbeat bytes thereafter, so update (B) already keeps
the gauge fresh. Update (A) adds nothing on the happy path and only injects
spurious freshness on the failure path.
The PR's new test test_metric_frozen_on_midstream_error does not catch this: it
exercises a single connection (200 → one event → error) and asserts the gauge
freezes, but never models the reconnect that follows in production, which is where
the head-update re-freshens the gauge.
Reachability / severity
- Reachable via a realistic operational scenario: a persistent LD-edge/proxy/network
condition that returns 200 then stalls the body — exactly incident-984, made
recurring rather than one-shot. - Full alert suppression depends on the external
stale-sse-tier-2threshold being
longer than the ~300sread_timeout+ reconnect backoff cycle. Tier-2 (the
less-urgent tier) plausibly uses a multi-minute threshold, in which case the alert
is suppressed; with a shorter threshold the alert firing is merely delayed by up
to one reconnect cycle on each reconnect. - Impact is confined to observability/alerting (a stuck config sync goes undetected),
not data loss, crash, or security — hence MEDIUM, not HIGH.
Suggested fix
Do not update the gauge on the response head for the streaming (SSE) transport;
update it only from received body chunks (update B). Keep update (A) for the
event-processor (CSE) transport, where the response is a normal
request/response and there is no meaningful streamed body. Concretely, give
MetricsTransport a flag such as update_on_response_head: bool, set true for
cse_transport and false for data_source_transport:
struct MetricsTransport<T> {
inner: T,
last_success_gauge: UIntGauge,
now_fn: NowFn,
update_on_response_head: bool,
}
// in request():
if resp.status().is_success() {
if self.update_on_response_head {
gauge.set(now_fn() / 1000);
}
let (parts, body) = resp.into_parts();
let wrapped: ByteStream = Box::pin(body.inspect_ok(move |_| gauge.set(now_fn() / 1000)));
Ok(http::Response::from_parts(parts, wrapped))
} else {
Ok(resp)
}This restores the documented "last received an event" semantics for the SSE metric
while preserving the CSE metric behavior, and keeps the staleness alert able to
detect a wedged data source.
(I haven't checked this yet, but thought I'd post it before we merge this too quickly.)
thanks - I'll take it into account and likely patch. Please look at #37026 as well because that includes some more testing that I used to de-risk this upgrade |
Move launchdarkly-server-sdk from the MaterializeInc/rust-server-sdk fork back to upstream crates.io 3.1.1, restoring the launchdarkly-sdk-transport + MetricsTransport setup and dropping the [patch.crates-io] override. The fork existed for launchdarkly/rust-server-sdk#116: a StreamingDataSource /eventsource StreamClosed bug where a non-Eof stream error left the data source stuck with no reconnect, silently breaking LD sync. A prior upgrade to upstream 3.0.1 had to be reverted (incident-984) because that bug was still unfixed upstream. The fixes have since landed, rust-server-sdk#168 and rust-eventsource-client#134/#135, and 3.1.1 resolves eventsource-client to 0.17.5, which carries them. Use the rustls + aws-lc-rs features (hyper-rustls-native-roots, crypto-aws-lc-rs), now the upstream defaults, instead of the prior attempt's native-tls/crypto-openssl, avoiding the OpenSSL path. The transport build_https() call is identical either way. Because the SDK now builds a rustls client, the workspace links both rustls provider features (aws_lc_rs via our features, ring transitively via the hyper-rustls chain). With both enabled rustls cannot select a process-default provider on its own and panics on first client build. Install aws-lc-rs explicitly (idempotently) at the top of the environmentd, clusterd, balancerd, sqllogictest, and testdrive entrypoints. orchestratord already installs it. deny.toml gains skips for the duplicate versions the transport stack pulls (older tower/rustls-native-certs; newer rand/rand_core/getrandom/cpufeatures) and re-adds the launchdarkly-sdk-transport wrapper. Adds MetricsTransport unit tests, including test_metric_frozen_on_midstream_ error, modeling the exact incident-984 failure mode (200 OK then a mid-stream timeout): they assert the last_sse_time_seconds gauge freezes so the staleness alert can detect a stuck data source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8b9df3d to
da5a8c4
Compare
Part 1 of 3 in the LaunchDarkly upstream-SDK stack:
What this does
Moves
launchdarkly-server-sdkfrom theMaterializeInc/rust-server-sdkfork back to upstream crates.io 3.1.1, restoring thelaunchdarkly-sdk-transport+MetricsTransportsetup and dropping the[patch.crates-io]override.The fork existed for rust-server-sdk#116: a non-
Eofstream error left theStreamingDataSourcestuck with no reconnect, silently breaking LD sync. A prior upgrade to 3.0.1 was reverted (incident-984) because that bug was still unfixed upstream. The fixes have since landed — rust-server-sdk#168 and eventsource-client#134/#135 — and 3.1.1 resolveseventsource-clientto 0.17.5, which carries them.hyper-rustls-native-roots,crypto-aws-lc-rs), avoiding the OpenSSL path.aws_lc_rs+ring, the latter transitively), each binary that builds an LD client installs the aws-lc-rs provider explicitly (environmentd, clusterd, balancerd, sqllogictest, testdrive) or it panics on first client build. orchestratord already installs it.MetricsTransportunit tests, includingtest_metric_frozen_on_midstream_errormodeling the incident-984 failure mode.Rebased onto current
main. Draft until the nightly LaunchDarkly jobs are confirmed green across the stack.