SK-3002: Configurable timeout & retries (Java v3 SDK)#366
Open
Devesh-Skyflow wants to merge 7 commits into
Open
SK-3002: Configurable timeout & retries (Java v3 SDK)#366Devesh-Skyflow wants to merge 7 commits into
Devesh-Skyflow wants to merge 7 commits into
Conversation
Hand-written OkHttp interceptor replicating Fern's retry algorithm (exponential backoff + proportional jitter, factor 0.2 fixed). Exposes maxRetries / initialRetryDelayMillis / maxRetryDelayMillis; retries on 408/429/5xx. Carries a TODO to replace with Fern's generated interceptor once a version exposing the delay knobs is vendored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- VaultConfig: vault-level timeout/maxRetries/initialRetryDelayMillis/ maxRetryDelayMillis (nullable = inherit) - Skyflow.SkyflowClientBuilder: flat client-wide setters + propagation to existing controllers (mirrors addSkyflowCredentials) - VaultClient.updateExecutorInHTTP: set callTimeout + attach SkyflowRetryInterceptor; per-field precedence vault > client > SDK default (defaults 60s / 3 / 500ms / 2000ms) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- SkyflowRetryInterceptorTests: retry count, stop-on-success, 408/429/5xx triggers, non-retryable passthrough, zero-retries - VaultClientHttpConfigTests: callTimeout applied to shared client, precedence (vault > client > default), bounded-not-zero regression guard, VaultConfig field getters/setters Note: pre-existing VaultClientTests#testSetBearerTokenWithEnvCredentials fails on Java 21 (PowerMock byte-buddy cannot instrument JDK security classes) — reproduced on base v3, unrelated to this change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Demonstrates client-wide defaults on Skyflow.builder() and per-vault overrides on VaultConfig, with precedence and unit notes. Compiles against the local SDK build (samples module pins a released version, so it builds once this change ships). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drives an OkHttpClient built like VaultClient (callTimeout + SkyflowRetry Interceptor) against a real loopback MockWebServer: - retries 503 then succeeds (server sees 1+retries requests) - exhausts retries, returns last failure - no retry on 400 - callTimeout aborts a never-responding server (~1s, not hanging) -- the fix - observable backoff between retries Adds com.squareup.okhttp3:mockwebserver as a test dependency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MockWebServer's static init loads JDK security providers, which the PowerMock/byte-buddy agent fails to instrument on Java 21 (IllegalClassFormatException: Unsupported class file major version 65) -> ExceptionInInitializerError in setUp. This is the same agent/JDK incompatibility that already fails testSetBearerTokenWithEnvCredentials. Retry/timeout behavior remains covered by the mocked-Chain unit tests (SkyflowRetryInterceptorTests) and the config/precedence tests (VaultClientHttpConfigTests), both green in CI. Also drops the mockwebserver test dependency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cover the previously-untested new code paths flagged by Codecov: - Skyflow.builder() setters + propagateHttpConfig loop (config set before and after addVaultConfig) -> SkyflowClientBuilderHttpConfigTests - resolveInt/resolveLong vault + client branches via maxRetries + backoff precedence, asserted on the built SkyflowRetryInterceptor's fields - interceptor InterruptedException-during-backoff path (throws IOException, preserves interrupt flag) Co-Authored-By: Claude Opus 4.8 (1M context) <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
1 participant
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.
Summary
Makes HTTP timeout and retries configurable in the Java v3 SDK (CUST-4311 / SK-3002), and fixes a regression where the injected shared
OkHttpClientsilently dropped the call timeout and retries.Problem
VaultClientinjects its ownOkHttpClient(for auth + connection pool), which putClientOptions.build()on the custom-client branch →callTimeout = 0(unbounded) and no retries. A stuck call blocked the thread indefinitely → thread-pool exhaustion → unresponsive service.Changes
SkyflowRetryInterceptor— hand-written OkHttp interceptor replicating Fern's algorithm (exponential backoff + proportional jitter, factor0.2fixed / not exposed). Retries on408 / 429 / 5xx.Skyflow.builder().timeout(sec).maxRetries(n).initialRetryDelayMillis(ms).maxRetryDelayMillis(ms)VaultConfigsettersVaultClient.updateExecutorInHTTP— appliescallTimeout+ attaches the interceptor on the shared client (never callsapiClientBuilder.timeout()to avoid the connect/write/read clobber).timeout 60s,maxRetries 3,initialRetryDelayMillis 500,maxRetryDelayMillis 2000(worst-case backoff ≲ 3.85s, well inside the 60s ceiling).TimeoutAndRetryConfigExample.Scope
Phase 1 = call timeout + retries.
connectTimeout/readTimeout/writeTimeoutare deferred to a future phase (Fern can't express them at any version; they'll go on the same shared client).Naming
Adopts Fern's vocabulary/types (
maxRetries,initialRetryDelayMillis,maxRetryDelayMillis) so a future swap to Fern's generated interceptor is a drop-in. Default values differ from Fern (tighter, per team decision).Why our own interceptor (not Fern's)
The shared client (needed for auth + connection pool) disables Fern's auto-attached
RetryInterceptor, and the currently-vendored Fern class hardcodes its backoff (can't honor the delay knobs). ATODO(CUST-4311)marks it for replacement once a Fern version exposing the delay knobs is vendored.Tests
SkyflowRetryInterceptorTests(6) — retry count, stop-on-success, 408/429/5xx triggers, non-retryable passthrough, zero-retries.VaultClientHttpConfigTests(6) —callTimeoutapplied to the shared client, precedence, bounded-not-zero regression guard,VaultConfigfield round-trip.SkyflowTests,VaultConfigTests) pass.VaultClientTests#testSetBearerTokenWithEnvCredentialsfails on Java 21 (PowerMock/byte-buddy cannot instrument JDK security classes) — reproduced on basev3, unrelated to this change.🤖 Generated with Claude Code