Use the generated RetryInterceptor instead of the hand-written one - #423
Use the generated RetryInterceptor instead of the hand-written one#423Devesh-Skyflow wants to merge 3 commits into
Conversation
The generated com.skyflow.generated.rest.core.RetryInterceptor now supports everything the hand-written SkyflowRetryInterceptor existed to work around: a configurable initial/max retry delay via its 4-arg constructor, and a per-call (not per-instance) backoff counter, so a shared OkHttpClient no longer exhausts its retry budget once for its whole lifetime. It also picks up Retry-After / X-RateLimit-Reset header handling for free. - VaultClient now constructs the generated RetryInterceptor, passing the resolved maxRetries/initialRetryDelayMillis/maxRetryDelayMillis. Jitter is left at the generated interceptor's own default (0.2) - not yet exposed as a VaultConfig/builder setting. - The generated interceptor doesn't validate maxRetries itself (a negative value would silently behave as zero retries), so VaultClient now guards that explicitly to preserve the existing "negative maxRetries -> SkyflowException" contract. - Deleted SkyflowRetryInterceptor and its unit tests; that logic now lives only in the generated class (excluded from coverage/javadoc like the rest of com.skyflow.generated.*). - Updated HttpConfigTests/AuthInterceptorTests to the new type. The generated interceptor exposes no getters, so tests that need to read back what it was constructed with do it via reflection rather than adding hand-written accessors to generated code. All 710 flowvault tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
✅ Gitleaks Findings: No secrets detected. Safe to proceed! |
|
Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #423 +/- ##
============================================
- Coverage 91.99% 91.89% -0.11%
- Complexity 0 493 +493
============================================
Files 158 159 +1
Lines 6631 6671 +40
Branches 893 890 -3
============================================
+ Hits 6100 6130 +30
- Misses 351 358 +7
- Partials 180 183 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
✅ Gitleaks Findings: No secrets detected. Safe to proceed! |
|
Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging. |
cd28fa9 to
62f8074
Compare
|
✅ Gitleaks Findings: No secrets detected. Safe to proceed! |
|
Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging. |
…ryInterceptor's default RetryInterceptor is generated code we don't maintain, so its internal DEFAULT_JITTER_FACTOR is free to change on a future regeneration. Passing Optional.of(RETRY_JITTER_FACTOR) from VaultClient keeps the 0.2 hardcode visible and owned in our own code instead of implicitly inherited by passing Optional.empty(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
✅ Gitleaks Findings: No secrets detected. Safe to proceed! |
|
Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging. |
…t path Both were found during a round-trip latency audit of the SDK's request path (same audit that flagged the retry-interceptor duplication this branch fixes). - LogUtil's ConsoleHandler writes and flushes to the console synchronously, under a lock shared by every calling thread. LogUtil.printInfoLog/etc. fire several times per request (setBearerToken reuse/expiry, request validation, per-batch triggers, request-resolved), so enabling INFO/DEBUG logging turned console output into a per-request blocking-I/O + contention point. A new AsyncConsoleHandler now hands each LogRecord to a single background daemon thread via a bounded, non-blocking queue; the calling thread only enqueues. Applied to both the common/flowvault LogUtil and skyvault's separate copy. - Several settings/credentials lookups fell back to Dotenv.load() when an env var wasn't set: VaultController.resolveSettingFromEnvironment did this twice per bulk call (batch size + concurrency limit), unconditionally, on every single insert/detokenize/tokenize/deleteTokens call -- Dotenv.load() re-reads the .env file from disk every time it's invoked, so this was uncached, blocking disk I/O on the hot path. The same pattern existed in BaseVaultClient/ConnectionClient's credential fallback and Utils.getEnvVaultUrl. BaseUtils.resolveEnvOrDotenv now memoizes the loaded (or absent) .env for the life of the JVM -- a project's .env doesn't change while the process runs, so there's no reason to keep re-reading it -- with a resetDotenvCacheForTests() escape hatch for the handful of tests that intentionally rewrite .env mid-run to exercise both branches. Testing: full common + skyvault + flowvault suites, 1450/1450 passing (165+575+710), BUILD SUCCESS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
✅ Gitleaks Findings: No secrets detected. Safe to proceed! |
|
Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging. |
|
✅ Gitleaks Findings: No secrets detected. Safe to proceed! |
|
Semgrep Findings: Issues with Error level severity are found (Error is Highest severity in Semgrep), Please resolve the issues before merging. |
|
Superseding this with a single PR from |
1 participant
Why
com.skyflow.generated.rest.core.RetryInterceptor(Fern-generated) now has a 4-arg constructor supporting configurableinitialRetryDelayMillis/maxRetryDelayMillis/jitterFactor, and scopes its backoff counter per call rather than per interceptor instance. Those were exactly the two gapsSkyflowRetryInterceptorwas hand-written to work around (see its removed javadoc). With the gaps closed, maintaining a parallel hand-written implementation is redundant, and the generated one is strictly more capable - it also honorsRetry-After/X-RateLimit-Resetresponse headers, which the hand-written version never did.What changed
flowvault/.../generated/rest/core/RetryInterceptor.javaupdated to the version Fern would produce today (matches the unmergedsaileshwar/update-flowvault-generated-codebranch and the oldv3module's post-SK-3002 version) - configurable delays, header-aware backoff, per-requestmaxRetriesoverride via request tag.VaultClientnow constructs the generated interceptor instead ofSkyflowRetryInterceptor, passing the resolvedmaxRetries/initialRetryDelayMillis/maxRetryDelayMillis. Jitter is hardcoded for now: aRETRY_JITTER_FACTOR = 0.2constant onVaultClientis passed explicitly asOptional.of(RETRY_JITTER_FACTOR)rather than relying onRetryInterceptor's own internal default - that keeps the hardcode visible and owned in our code instead of implicitly inherited from generated code we don't maintain (and which is free to change on a future regeneration). Not yet exposed as aVaultConfig/builder setting.maxRetriesitself (a negative value would silently behave as zero retries instead of failing), soVaultClientnow guards that explicitly to preserve the existing "negativemaxRetries→SkyflowException" contract.SkyflowRetryInterceptor+ its unit tests (254 lines). That logic now lives only in the generated class, which is excluded from coverage/javadoc like the rest ofcom.skyflow.generated.*.HttpConfigTests/AuthInterceptorTestsfor the new type. The generated interceptor exposes no getters, so tests that need to read back what it was constructed with do it via reflection rather than adding hand-written accessors to a file meant to stay a faithful Fern output.Testing
Full
flowvaultmodule test suite: 710/710 passing,BUILD SUCCESS(mvn -pl flowvault -am test).Tech debt
Jitter is still not configurable per-vault/client-wide - a natural following change would add
jitterFactortoVaultConfig/Skyflow.builder()and thread it through, but that's deliberately out of scope here per the ask ("set jitter as hardcoded for now").Also in this PR: two more latency fixes from a round-trip SDK audit
Found while tracing the request path for an unrelated customer-latency investigation; folded in here since they're the same class of fix (blocking work hidden on the hot path) and this branch was already open.
LogUtil'sConsoleHandlerwrote and flushed to the console synchronously, under a lock shared by every calling thread.printInfoLog/etc. fire several times per request, so enabling INFO/DEBUG logging turned console output into a per-request blocking-I/O + contention point. A newAsyncConsoleHandlernow hands eachLogRecordto a single background daemon thread via a bounded, non-blocking queue. Applied to both thecommon/flowvaultLogUtilandskyvault's separate copy..envlookups.VaultController.resolveSettingFromEnvironmentfell back toDotenv.load()(an uncached filesystem read) twice per bulk call whenever the batch-size/concurrency-limit env vars weren't set - i.e. on every single insert/detokenize/tokenize/deleteTokens call. The same pattern existed inBaseVaultClient/ConnectionClient's credential fallback andUtils.getEnvVaultUrl.BaseUtils.resolveEnvOrDotenvnow loads.envat most once per JVM (a project's.envdoesn't change while the process runs), with aresetDotenvCacheForTests()escape hatch for the tests that intentionally rewrite.envmid-run.Testing: full
common+skyvault+flowvaultsuites, 1450/1450 passing (165+575+710),BUILD SUCCESS.🤖 Generated with Claude Code