Fix Pekko async handler retaining the request context in Pekko completion callbacks - #12145
Fix Pekko async handler retaining the request context in Pekko completion callbacks#12145AlexeyKuznetsov-DD wants to merge 2 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
🎯 Code Coverage (details) 🔗 Commit SHA: e77d333 | Docs | Datadog PR Page | 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. |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7fabeec to
e77d333
Compare
What Does This Do
Stops the Pekko HTTP async-handler instrumentation from leaking the request context into Pekko's own completion callbacks, which intermittently delayed trace reporting.
DatadogAsyncHandlerWrapperused to returnfutureResponse.transform(...), where both transform functions returned their input unchanged and existed only to runfinishSpan.transformderives a second Promise, and that Promise was completed while the request context was active, so the Scala Promise instrumentation captured the request context for callbacks Pekko had registered on the returned Future. Those callbacks are framework bookkeeping, and the continuation they captured kept the finished trace buffered until they ran.The wrapper now:
Contextexplicitly and closes the request scope as before.Promiseand observes the handler Future withonComplete, registered after the scope is closed so nothing is captured at construction time.Context, turning a decoration failure into a failed bridge Promise (preserving whattransformdid).Context.root(), so Pekko's callbacks inherit nothing from the thread.Tryfirst, because that mode associates the completing context with theTryobject itself and thereby bypasses the thread-local defense.Two supporting changes:
DatadogWrapperHelper.finishSpannow callsspan.finish()from afinallyblock, so a response-decoration failure cannot leave a span unfinished. This helper is shared with the HTTP/1 flow wrapper, so that path benefits too.InstrumenterConfig.isScalaPromiseCompletionPriorityEnabled()accessor. The integration name and itsfalsedefault were previously written out at each call site; the Pekko gate has to agree with the Scala instrumentations that create the association it strips, and duplication let those drift apart silently. All call sites (PromiseHelper, bothScalaPromiseModulevariants, and the Pekko wrapper) now share one definition.Since the wrapper now has one anonymous callback instead of two, the stale
DatadogAsyncHandlerWrapper$2entry was removed from the HTTP/2 helper class list.Motivation
PekkoHttpServerInstrumentationAsyncTestfailed intermittently in CI while waiting for the exception-request trace:The server span had finished, but the trace was still buffered because an open continuation retained the request context — one belonging to a downstream Pekko completion callback rather than to customer request processing. Whether the test passed depended purely on how quickly CI scheduled that callback, which is why retries "fixed" it. Raising Pekko's
request-timeoutwould not have addressed the mechanism: the cause is context propagation and continuation lifetime, not Pekko aborting the request.Beyond the flake, this is a real customer-visible issue on the
bindAndHandleAsyncpath: reporting of a finished request trace is delayed until unrelated framework bookkeeping completes.Additional Notes
New regression test.
AbstractPekkoHttpAsyncHandlerWrapperTestdrives the wrapper directly and makes the race deterministic by holding a simulated Pekko callback on a latch: the finished trace must be reported while that callback is still blocked. Two concrete variants run it —PekkoHttpAsyncHandlerWrapperTestwith default Promise propagation, andPekkoHttpAsyncHandlerWrapperForkedTestwith completion priority enabled in isolated JVMs via newbaseCompletionPriorityForkedTest/latestDepCompletionPriorityForkedTesttasks. Each variant asserts bothPromiseHelper.completionPriorityand the wrapper's own gate, so the two configurations cannot silently substitute for each other and a wrapper that stopped tracking the mode cannot keep the suite green.Coverage, all confirmed from
build/test-results/*/TEST-*.xml:baseTestPekkoHttpAsyncHandlerWrapperTestlatestDepTestPekkoHttpAsyncHandlerWrapperTestlatestPekko10TestPekkoHttpAsyncHandlerWrapperTestbaseCompletionPriorityForkedTestPekkoHttpAsyncHandlerWrapperForkedTestlatestDepCompletionPriorityForkedTestPekkoHttpAsyncHandlerWrapperForkedTestBoth defenses are individually pinned. The test completes the handler Promise once with a
Failureand once with aSuccess, and each case was verified to be necessary by temporarily reverting one defense at a time:Trycopy fails the success case on Scala 2.12 and both cases on Scala 2.13.The success case matters because Scala 2.12's
Promise.resolveTryroutes failures throughresolver, which allocates a freshFailureand so incidentally strips the association; with only a failing response, 2.12 passed without the copy. The two defenses are not interchangeable either:PromiseTransformationInstrumentationandCallbackRunnableInstrumentationcapture from the completingTryfirst and only fall back to the thread-local context when theTrycarries none, so in completion-priority mode the root attachment alone does not prevent retention.Deliberate behavior change. Completing the exposed Future under the root context makes Pekko's response-bookkeeping callbacks contextless. If a future Pekko release performs user-visible child work from that Future, that work would also be contextless. Not retaining a completed request trace is the intended boundary here, but it is worth a reviewer's attention.
Allocation cost. Versus the old
transform, the bridge Promise and single callback replace allocationstransformalready made, so the steady-state delta is one short-lived root-context scope per async response. The extraTryis allocated only when completion priority is enabled.Validation. Pekko module with
--rerun-tasks: 1517 tests, 860 skipped by existing conditions, 0 failures, 0 errors, plus muzzle (12 passed) andspotlessCheck. Because the setting moved intoInstrumenterConfig,internal-apiand bothscala-promisemodules (test,forkedTest,muzzle) were also run: 1483 tests, 0 failures. The new accessor needs no separate unit test — the Pekko variants assert its value in both states.Follow-up, not in this PR. Akka HTTP's async-handler wrapper has the same identity-transform pattern, but its flow also performs response substitution for AppSec blocking, so it cannot be replaced mechanically with this implementation. It should get its own reproducer and fix rather than expanding this change without equivalent Akka coverage.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
🤖 Generated with Claude Code