Stop AppSecInterceptor from silently retrying failed okhttp requests - #12242
Stop AppSecInterceptor from silently retrying failed okhttp requests#12242dougqh wants to merge 3 commits into
Conversation
chain.proceed(request) was wrapped in the same try/catch that guards the AppSec request/response hooks, so any IOException from the real network call (e.g. ConnectException) was swallowed and the request was silently retried via chain.proceed(chain.request()). This double- executes non-idempotent requests on transient network failures and surfaces the retry's own failure as an unhandled error blamed on the interceptor. Narrow the try/catch to only cover the AppSec hooks so genuine I/O failures propagate normally. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🎯 Code Coverage (details) 🔗 Commit SHA: acf135f | Docs | View more details | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Covers both okhttp-2.2 and okhttp-3.0 AppSecInterceptor.intercept(): asserts an IOException from chain.proceed() propagates without being swallowed/retried, using Mockito + AgentTracer.forceRegister instead of Groovy/Spock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
More details
The network call is outside both hook exception handlers. The first I/O failure now passes to the caller, with no silent retry in either OkHttp integration.
🤖 Datadog Autotest · Commit 161655c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
What Does This Do
Narrows the
try/catchinAppSecInterceptor.intercept()(bothokhttp-2.2andokhttp-3.0) so it only guards the AppSec-specific hook logic (sampleRequest/onRequest/onResponse), not the realchain.proceed()network call. Genuine I/O failures now propagate normally on the first (and only) attempt, exactly as they would without this instrumentation.Motivation
AppSecInterceptor.intercept()previously wrapped the realchain.proceed(request)call inside the sametry/catch (Exception e)that guards the AppSec hooks. AnyIOExceptionfrom the actual outbound request (e.g.java.net.ConnectExceptionon a transient network failure) was caught by that handler, logged at debug, and the request was silently retried viachain.proceed(chain.request()). This means:IOExceptionpropagates unhandled and gets misattributed toAppSecInterceptor.interceptin error tracking, when the real cause is just an ordinary network failure.Found via Datadog Error Tracking issue 7d2a0a46-aab1-11f0-a468-da7ad0900002 (38k+ occurrences) — automated root cause analysis confirmed the stack shows
ConnectExceptionpropagating throughAppSecInterceptor:57/58, matching this code path exactly. Same shape exists in the okhttp2 sibling (db728b88-b44c-11f0-a740-da7ad0900002).Additional Notes
AppSecInterceptorbehavior itself in either module; happy to add one covering the retry regression if desired.Test plan
./gradlew :dd-java-agent:instrumentation:okhttp:okhttp-3.0:compileJava :dd-java-agent:instrumentation:okhttp:okhttp-2.2:compileJava— BUILD SUCCESSFUL./gradlew :dd-java-agent:instrumentation:okhttp:okhttp-3.0:spotlessApply :dd-java-agent:instrumentation:okhttp:okhttp-2.2:spotlessApply— applied, no additional changes./gradlew :dd-java-agent:instrumentation:okhttp:okhttp-3.0:test :dd-java-agent:instrumentation:okhttp:okhttp-2.2:test— BUILD SUCCESSFUL, existing instrumentation tests pass🤖 Generated with Claude Code