fix(embed,auth): add request timeouts to GeminiEmbedder and JwksCache - #421
Merged
Conversation
… (closes #569) Hit this for real: live-verifying #567's ingest-durability fix on lab, a POST /ingest/upload never returned. /healthz — a hardcoded 200 with no locks or I/O — started failing liveness/readiness probes with "context deadline exceeded" minutes later, until kubelet force-restarted the pod. Root cause: GeminiEmbedder built its HTTP client as bare reqwest::Client::new() — reqwest has no default request timeout, so a hung or abnormally slow call to Google's batchEmbedContents API blocks the calling request forever, with no error and no way out short of the process being killed externally. This runs before materialize_document (and #567's publish_page_sync call), so it's very unlikely to be a regression from #567 itself — more likely a pre-existing gap that had simply never been triggered, since nothing had exercised the document- ingest path on this tenant before. Same gap, same severity class, found alongside it: JwksCache (JWKS fetch) also built a bare Client::new(), and sits on the AUTHENTICATED- REQUEST path — every request behind an expired TTL can trigger a refresh, so a hang there has a wider blast radius than one ingest call. Fix: explicit .timeout() on both clients, matching the existing remote_backend.rs precedent (10s for a comparable external call). GeminiEmbedder gets 30s — embedding a batch of up to 100 texts is legitimately slower than that simpler call. JwksCache gets 10s, matching the precedent exactly. Adds with_timeout() builders to both (mirroring GeminiEmbedder's existing with_base_url/with_dim pattern) so tests can prove the fix without a real 30s/10s-long test: mock a response slower than a short configured timeout and assert the call errors out around that timeout, not the mock's delay. New tests: - gemini_embedder.rs::slow_upstream_response_times_out_instead_of_hanging_forever - jwks_timeout.rs::slow_jwks_endpoint_times_out_instead_of_hanging_forever Both pass in well under 2s (proving the short timeout fires, not the mock's 5s delay). Not in scope here (flagged in #569 for a follow-up): the same bare- client pattern in escurel-index/src/backend/contextualize_llm.rs (the optional LLM contextualizer, not built into the lab image today) and in escurel-client/escurel-explore-bff/escurel-demo-agent/ escurel-test-support (not part of the live escurel-server request path). Full workspace suite (--all-features): 451/454 passed. The 3 failures are pre-existing and unrelated — confirmed by reproducing them on a clean main checkout with none of this change applied: two (binary_boots::degraded_embedder_start_boots_with_readyz_false, binary_boots::embedder_required_makes_a_failed_load_fatal) are flaky under full-suite parallel load and pass cleanly in isolation; the third (reader_role_crdt::reader_no_longer_rejects_session_tools_when_ducklake_configured) fails identically on main with or without this branch's changes. cargo clippy --all-features --all-targets -- -D warnings and cargo fmt --check both clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Problem
Hit this for real: live-verifying #567's ingest-durability fix on
lab, aPOST /ingest/uploadnever returned./healthz— a hardcoded200with no locks or I/O — started failing liveness/readiness probes withcontext deadline exceededminutes later, until kubelet force-restarted the pod.Root cause:
GeminiEmbedderbuilt its HTTP client as barereqwest::Client::new()—reqwesthas no default request timeout, so a hung or abnormally slow call to Google'sbatchEmbedContentsAPI blocks the calling request forever, with no error and no way out short of the process being killed externally. This runs beforematerialize_document(and #567'spublish_page_synccall), so it's very unlikely to be a regression from #567 itself — more likely a pre-existing gap that had simply never been triggered, since nothing had exercised the document-ingest path on this tenant before.Same gap, same severity class, found alongside it:
JwksCache(JWKS fetch) also built a bareClient::new(), and sits on the authenticated-request path — every request behind an expired TTL can trigger a refresh, so a hang there has a wider blast radius than one ingest call.Fix
Explicit
.timeout()on both clients, matching the existingremote_backend.rsprecedent (10s for a comparable external call).GeminiEmbeddergets 30s — embedding a batch of up to 100 texts is legitimately slower than that simpler call.JwksCachegets 10s, matching the precedent exactly.Adds
with_timeout()builders to both (mirroringGeminiEmbedder's existingwith_base_url/with_dimpattern) so tests can prove the fix without a real 30s/10s-long test: mock a response slower than a short configured timeout and assert the call errors out around that timeout, not the mock's delay.Testing
gemini_embedder.rs::slow_upstream_response_times_out_instead_of_hanging_foreverjwks_timeout.rs::slow_jwks_endpoint_times_out_instead_of_hanging_foreverBoth pass in well under 2s (proving the short timeout fires, not the mock's 5s delay).
Not in scope here (flagged in DataZooDE/hetzner-agent-substrate#569 for a follow-up): the same bare-client pattern in
escurel-index/src/backend/contextualize_llm.rs(the optional LLM contextualizer, not built into thelabimage today) and inescurel-client/escurel-explore-bff/escurel-demo-agent/escurel-test-support(not part of the liveescurel-serverrequest path).Full workspace suite (
--all-features): 451/454 passed. The 3 failures are pre-existing and unrelated — confirmed by reproducing them on a cleanmaincheckout with none of this change applied: two (binary_boots::degraded_embedder_start_boots_with_readyz_false,binary_boots::embedder_required_makes_a_failed_load_fatal) are flaky under full-suite parallel load and pass cleanly in isolation; the third (reader_role_crdt::reader_no_longer_rejects_session_tools_when_ducklake_configured) fails identically onmainwith or without this branch's changes.cargo clippy --all-features --all-targets -- -D warningsandcargo fmt --checkboth clean.Closes DataZooDE/hetzner-agent-substrate#569.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.