fix(client): do not panic when the HTTP client cannot be built - #247
Draft
posthog[bot] wants to merge 4 commits into
Draft
fix(client): do not panic when the HTTP client cannot be built#247posthog[bot] wants to merge 4 commits into
posthog[bot] wants to merge 4 commits into
Conversation
reqwest's build() is fallible because it sets up the TLS trust store. In a container without CA certificates it returns an error, and the unwrap stopped the calling program. Both clients now log a warning, disable themselves, and return a no-op client instead. Generated-By: PostHog Desktop Task-Id: e3125f9b-fea1-42ef-83b2-4e8bcbddc7bb
Contributor
Author
🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 1 consider. Published 2 findings (view the review). Resolved comments: 2 fixed |
Contributor
posthog-rs-v0 Compliance ReportDate: 2026-09-05 13:21:10 UTC ✅ All Tests Passed!46/46 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Contributor
posthog-rs-v1 Compliance ReportDate: 2026-09-05 13:21:35 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Contributor
Author
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
The capture worker built its blocking reqwest client with unwrap_or_default(), but Default for reqwest::blocking::Client calls Client::new(), which panics on the same failures build() reports. The blocking builder can also fail where the client's async build succeeded, because it spawns its own thread and runtime. Pipeline::new now returns None on a build failure and logs a warning, and run_worker signals any queued completions and returns instead of panicking on a thread whose panic is swallowed by join(). Generated-By: PostHog Desktop Task-Id: ef58b299-44ac-4dc4-be2e-66379d7c883a
FlagPoller::new and AsyncFlagPoller::new unwrapped the reqwest builder, so a direct user of either public poller still panicked in the container this PR is about — one without CA certificates, where the TLS trust-store setup makes build() fail. Both pollers now hold an Option client: a failed build logs a warning, load_flags returns Error::Connection, and start keeps the poller stopped instead of running a thread or task that could only log failures. The blocking poller's polling thread reuses the client built in new instead of building a second one, which removes the third unwrap; the async poller already cloned its client that way. Generated-By: PostHog Desktop Task-Id: 6b381563-d746-41ae-94d3-5fb69dd64ec9
reqwest's Display for a builder failure writes the fixed text "builder error" and nothing else, so the warnings this PR adds in place of the panic said only that the build failed. Verified against the resolved reqwest 0.13.4 (Display writes the kind, Debug adds the source) and reproduced with an empty trust store: Display gives "builder error", Debug gives reqwest::Error { kind: Builder, source: General("No CA certificates were loaded from the system") }.
The three build-failure warnings — the shared client helper, the transport worker, and the flag pollers — now format the error with Debug, which prints the source the operator needs. The helpers take a Debug bound instead of Display; every caller already passes a reqwest::Error or a &str.
Generated-By: PostHog Desktop
Task-Id: 6b381563-d746-41ae-94d3-5fb69dd64ec9
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.
💡 Motivation and Context
posthog-cli sourcemap injectrun stops before it processes any source map when the container has no CA certificates. The telemetry client kills a build command that must not depend on telemetry.client()constructors callHttpClient::builder().build().unwrap(). The comment says the unwrap is as safe asHttpClient::new, but the TLS trust-store setup makesbuild()fallible.rustls_platform_verifier::Verifier::new()for the platform roots. With an empty root store it returnsNo CA certificates were loaded from the system, sobuild()returnsErrand the unwrap panics.Changes
is_disabled()) never start.Client.clientis nowOption<HttpClient>. A privatehttp()accessor returnsError::Connectioninstead of a panic if a request path is ever reached with no HTTP client.💚 How did you test it?
SSL_CERT_FILEandSSL_CERT_DIRpointed at an empty certificate set) and a small example program:Before and after, both clients
Before (async client, on
main):After, with the same environment:
cargo test,cargo test --no-default-features,cargo test --no-default-features --features capture-v1,error-tracking,cargo fmt -- --check, andcargo clippy -- -D warningspass. (cargo clippy --all-targetshas failures onmainthat this branch does not change.)📝 Checklist
If releasing new changes
sampo addto generate a changeset fileAgent context
build()fails for the same reason.unwrap_or_default()calls in the capture transport have the same latent problem, but they are only reached when the client is enabled, which now implies a successful build. Left as is.Created with PostHog Desktop from this inbox report.