adapter: add LaunchDarkly reconnect integration test and migrate to upstream 3.1.1

[jasonhernandez]

Merge this PR to move to LaunchDarkly mainline 3.1.1

Part 2 of 3 in the LaunchDarkly upstream-SDK stack. Stacked on #37025.

1. adapter: migrate LaunchDarkly SDK off fork to upstream 3.1.1 #37025 — migrate off the fork to upstream 3.1.1 - no tests!
2. adapter: add LaunchDarkly reconnect integration test and migrate to upstream 3.1.1 #37026 (this PR) — + the reconnect integration test
3. [DO NOT MERGE] adapter: prove LD reconnect test goes red on pre-fix SDK 3.0.1 #37460 — + pin to the incident-era pre-fix stack to prove the test fails (DO NOT MERGE)

What this does

Adds test/launchdarkly-reconnect, an integration test that reproduces incident-984 deterministically against a mock, with no real LaunchDarkly credentials.

The failure mode matters. The incident signature was a mid-body read timeout on a silently-dead connection (hyper::Error(Body, Kind(TimedOut))), which the eventsource client surfaces to the data source as a stream error. A TCP RST or clean FIN does not reproduce it — those are retried inside the eventsource client on every SDK version and never reach the data source. We verified this empirically: an earlier RST-based version of this mock stayed green (nightly #17018) even on the exact incident-era dependency stack.

• mock_ld.py — mocks the LD streaming API. The first streaming client gets an initial flag value (2 GiB), then the connection goes silent while held open (no FIN, no RST), so the SDK's transport read timeout ends the stream — the exact incident error class. Every reconnecting client gets an updated value (3 GiB) plus heartbeats.
• mzcompose.py — boots environmentd against the mock with a 5s read timeout and asserts SHOW max_result_size reaches 3GB, which can only happen if the data source reconnected after the timeout.

Production changes, both hidden test knobs:

• --launchdarkly-base-uri (env MZ_LAUNCHDARKLY_BASE_URI) — overrides the SDK's endpoints with a single base URL (ServiceEndpointsBuilder::relay_proxy), letting tests point the SDK at the mock.
• MZ_LAUNCHDARKLY_READ_TIMEOUT — overrides the transport's streaming read timeout (default 300s) so the test can trigger the timeout path in seconds.

Against the fixed 3.1.1 SDK (#37025) this should pass green; #37460 proves it goes red on the pre-fix stack.

---

Stacking note: GitHub can't base a fork PR on another fork branch, so this targets main and its diff also contains #37025's migration commit. Review the top commit (adapter: add LaunchDarkly reconnect integration test).

[def-]

We should actually verify if the test would found the bug.

[jasonhernandez]

We should actually verify if the test would found the bug.

@def- I could stack a PR on this that has an older version of the 3.0 SDK and see if it fails. Would that be a reasonable approach?

[def-]

Sure, works for me!

[jasonhernandez]

We should actually verify if the test would found the bug.

@def- I could stack a PR on this that has an older version of the 3.0 SDK and see if it fails. Would that be a reasonable approach?

@def- @ggevay - this throwaway PR #37460 fails the reconnect test

I propose that we merge this PR #37026 if all looks good to both of you.

[def-]

Thanks for checking, no complaints from QA side

[ggevay]

Thank you very much for the thorough testing! This looks good to me, just some minor comments from Claude and me:

---

I reviewed this PR together with #37025. Since this PR carries both commits of the stack, I'm posting all findings here. Nothing blocking in either commit.

On the reconnect test commit

1. Unparsable MZ_LAUNCHDARKLY_READ_TIMEOUT is silently ignored (ld_config in src/adapter/src/config/frontend.rs). Both .ok()s swallow errors, so a typo like 5sec silently falls back to 300s. In the test that presents as testdrive timing out after 120s with no hint why, and although the knob is intended for tests, nothing stops an operator from setting it in production, where a silently ignored value is just as confusing. Suggest a tracing::error! on parse failure before falling back to the default.

2. Knob plumbing asymmetry (confirming it's deliberate). --launchdarkly-base-uri is plumbed CLI → Config → SystemParameterSyncClientConfig, while the read timeout is a raw std::env::var read inside ld_config, and there is essentially no precedent for production-path env reads in this crate. I can see the appeal of not widening three config structs for a test-only knob, and the inline comment explains itself, so fine by me if intentional.

3. The mock comment slightly overstates its guarantee. "the sync client always receives at least one stall-then-timeout cycle" is not strictly true: if the bootstrap client's stalled connection ever timed out before its initialization completed, its reconnect would consume the second stall, and the sync client would then get 3 GiB on its very first connection. This doesn't undermine the test, though. Consuming both stalls itself requires a working post-timeout reconnect, the very behavior under test, in code shared by both clients, so a regressed SDK cannot pass this way. Just worth a small wording tweak in the comment.

4. Nits (take or leave):

• c.testdrive("\n".join([...])) reads more naturally as a single dedented string, matching most compositions.
• In the mock's stall branch, the wfile.write before the sleep can raise BrokenPipeError unhandled (the heartbeat branch handles it). Harmless in a mock, just noisy in logs if it ever races.

On the migration commit (#37025)

5. The rustls provider pin is five copy-pasted call sites with an empirically chosen scope. The PR body says "each binary that builds an LD client", but neither testdrive nor clusterd builds an LD client (clusterd has no LaunchDarkly usage at all, mz-dyncfg-launchdarkly's only user is balancerd). The real criterion is "builds any default-provider rustls client while both provider features are linked", which is what justifies the clusterd and testdrive pins via their other rustls-using dependencies. Today the coverage looks right: apart from the LD stack itself, the default-provider build sites appear to be axum-server (orchestratord, already pinned) and gcp_auth via mz-storage-types, whose binaries (environmentd, clusterd, testdrive, sqllogictest) are all pinned. The only unpinned binary I found linking gcp_auth is mz-catalog-debug, which never builds a GCP client in practice. (Caveat: I inferred this from the dependency tree and did not audit how gcp_auth constructs its TLS client, so treat the exact at-risk set as approximate.) The broader point stands regardless: nothing stops a future binary, or a future dependency that builds a default-provider rustls client, from reintroducing a runtime panic that no compile-time check catches. Suggest hoisting the install into a small shared helper (e.g. in mz_ore) called from each binary's main, so the six-line comment lives in one place and new binaries have an obvious thing to call. Not blocking.

6. The 300s read timeout is a load-bearing magic constant. It must stay above LaunchDarkly's streaming heartbeat interval (around 3 minutes, going by LD's documentation), or healthy idle streams would reconnect spuriously (benign now that reconnects work, but worth stating). The comment this stack adds explains the override mechanism, not the relationship to the heartbeat interval. One sentence on the constant's origin would help. Same constant appears in dyncfg-launchdarkly.

7. The new transport auto-detects proxy env vars, worth a comment. launchdarkly-sdk-transport routes requests through HTTP_PROXY/HTTPS_PROXY/NO_PROXY by default, which the old hyper-tls setup ignored. No exposure today (self-managed never builds an LD client, and our cloud pods set no proxy vars), so no code change needed. But a one-line comment next to the HyperTransport::builder() calls (frontend.rs and dyncfg-launchdarkly) noting the auto-detection would save someone a confusing debugging session if proxy vars ever show up on a pod.

8. Minor Cargo.lock churn: dirs-sys's recorded windows-sys dependency edge changes 0.61.1 → 0.59.0 with no version bump, which looks like incidental re-resolution rather than something this stack needs. Windows-only and harmless, just flagging it per lock hygiene.

9. Nit: launchdarkly-sdk-transport = "0.1" uses a two-component version where the workspace convention is full three-component pins ("0.1.4").

[jasonhernandez]

Thank you very much for the thorough testing! This looks good to me, just some minor comments from Claude:

I reviewed this PR together with #37025. Since this PR carries both commits of the stack, I'm posting all findings here. Nothing blocking in either commit.

On the reconnect test commit

1. Unparsable MZ_LAUNCHDARKLY_READ_TIMEOUT is silently ignored (ld_config in src/adapter/src/config/frontend.rs). Both .ok()s swallow errors, so a typo like 5sec silently falls back to 300s. In the test that presents as testdrive timing out after 120s with no hint why, and although the knob is intended for tests, nothing stops an operator from setting it in production, where a silently ignored value is just as confusing. Suggest a tracing::error! on parse failure before falling back to the default.

Good point - addressed it.

2. Knob plumbing asymmetry (confirming it's deliberate). --launchdarkly-base-uri is plumbed CLI → Config → SystemParameterSyncClientConfig, while the read timeout is a raw std::env::var read inside ld_config, and there is essentially no precedent for production-path env reads in this crate. I can see the appeal of not widening three config structs for a test-only knob, and the inline comment explains itself, so fine by me if intentional.

It's intentional here. I'm also not sure how long we'll want to keep all of these tests beyond de-risking the migration off our fork.

3. The mock comment slightly overstates its guarantee. "the sync client always receives at least one stall-then-timeout cycle" is not strictly true: if the bootstrap client's stalled connection ever timed out before its initialization completed, its reconnect would consume the second stall, and the sync client would then get 3 GiB on its very first connection. This doesn't undermine the test, though. Consuming both stalls itself requires a working post-timeout reconnect, the very behavior under test, in code shared by both clients, so a regressed SDK cannot pass this way. Just worth a small wording tweak in the comment.

The comments have been clarified.

4. Nits (take or leave):

* `c.testdrive("\n".join([...]))` reads more naturally as a single dedented string, matching most compositions.

* In the mock's stall branch, the `wfile.write` before the sleep can raise `BrokenPipeError` unhandled (the heartbeat branch handles it). Harmless in a mock, just noisy in logs if it ever races.

Fixed it since I was pushing a commit.

On the migration commit (#37025)

5. The rustls provider pin is five copy-pasted call sites with an empirically chosen scope. The PR body says "each binary that builds an LD client", but neither testdrive nor clusterd builds an LD client (clusterd has no LaunchDarkly usage at all, mz-dyncfg-launchdarkly's only user is balancerd). The real criterion is "builds any default-provider rustls client while both provider features are linked", which is what justifies the clusterd and testdrive pins via their other rustls-using dependencies. Today the coverage looks right: apart from the LD stack itself, the default-provider build sites appear to be axum-server (orchestratord, already pinned) and gcp_auth via mz-storage-types, whose binaries (environmentd, clusterd, testdrive, sqllogictest) are all pinned. The only unpinned binary I found linking gcp_auth is mz-catalog-debug, which never builds a GCP client in practice. (Caveat: I inferred this from the dependency tree and did not audit how gcp_auth constructs its TLS client, so treat the exact at-risk set as approximate.) The broader point stands regardless: nothing stops a future binary, or a future dependency that builds a default-provider rustls client, from reintroducing a runtime panic that no compile-time check catches. Suggest hoisting the install into a small shared helper (e.g. in mz_ore) called from each binary's main, so the six-line comment lives in one place and new binaries have an obvious thing to call. Not blocking.

Once we're fully migrated over to rustls, some additional cleanup here would be great. I'm not going to address this now because it would increase the size / scope of the PR.

6. The 300s read timeout is a load-bearing magic constant. It must stay above LaunchDarkly's streaming heartbeat interval (around 3 minutes, going by LD's documentation), or healthy idle streams would reconnect spuriously (benign now that reconnects work, but worth stating). The comment this stack adds explains the override mechanism, not the relationship to the heartbeat interval. One sentence on the constant's origin would help. Same constant appears in dyncfg-launchdarkly.

I added some comments here.

7. The new transport auto-detects proxy env vars, worth a comment. launchdarkly-sdk-transport routes requests through HTTP_PROXY/HTTPS_PROXY/NO_PROXY by default, which the old hyper-tls setup ignored. No exposure today (self-managed never builds an LD client, and our cloud pods set no proxy vars), so no code change needed. But a one-line comment next to the HyperTransport::builder() calls (frontend.rs and dyncfg-launchdarkly) noting the auto-detection would save someone a confusing debugging session if proxy vars ever show up on a pod.

Noted

8. Minor Cargo.lock churn: dirs-sys's recorded windows-sys dependency edge changes 0.61.1 → 0.59.0 with no version bump, which looks like incidental re-resolution rather than something this stack needs. Windows-only and harmless, just flagging it per lock hygiene.

Good catch but leaving it.

9. Nit: launchdarkly-sdk-transport = "0.1" uses a two-component version where the workspace convention is full three-component pins ("0.1.4").

fixed

Thanks - the latest commit 4250039 addresses most of these.

[jasonhernandez]

@ggevay if you don't mind, please have your agent take a final look at 4250039 and then we'll merge this

[ggevay]

LGTM, thank you!