Skip to content

Fix Pekko async handler retaining the request context in Pekko completion callbacks - #12145

Draft
AlexeyKuznetsov-DD wants to merge 2 commits into
alexeyk/forked-test-source-setfrom
alexeyk/pekko-fix
Draft

Fix Pekko async handler retaining the request context in Pekko completion callbacks#12145
AlexeyKuznetsov-DD wants to merge 2 commits into
alexeyk/forked-test-source-setfrom
alexeyk/pekko-fix

Conversation

@AlexeyKuznetsov-DD

Copy link
Copy Markdown
Contributor

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.

DatadogAsyncHandlerWrapper used to return futureResponse.transform(...), where both transform functions returned their input unchanged and existed only to run finishSpan. transform derives 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:

  1. Saves the request Context explicitly and closes the request scope as before.
  2. Creates a bridge Promise and observes the handler Future with onComplete, registered after the scope is closed so nothing is captured at construction time.
  3. Finishes the span through the saved Context, turning a decoration failure into a failed bridge Promise (preserving what transform did).
  4. Completes the bridge Promise under Context.root(), so Pekko's callbacks inherit nothing from the thread.
  5. In completion-priority mode only, copies the result into a fresh Try first, because that mode associates the completing context with the Try object itself and thereby bypasses the thread-local defense.
  6. Returns the bridge Future to Pekko.

Two supporting changes:

  • DatadogWrapperHelper.finishSpan now calls span.finish() from a finally block, 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.
  • The completion-priority setting is now read through a new InstrumenterConfig.isScalaPromiseCompletionPriorityEnabled() accessor. The integration name and its false default 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, both ScalaPromiseModule variants, and the Pekko wrapper) now share one definition.

Since the wrapper now has one anonymous callback instead of two, the stale DatadogAsyncHandlerWrapper$2 entry was removed from the HTTP/2 helper class list.

Motivation

PekkoHttpServerInstrumentationAsyncTest failed intermittently in CI while waiting for the exception-request trace:

java.util.concurrent.TimeoutException: Timeout waiting for 1 trace(s).
ListWriter.size() == 0 : []

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-timeout would 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 bindAndHandleAsync path: reporting of a finished request trace is delayed until unrelated framework bookkeeping completes.

Additional Notes

New regression test. AbstractPekkoHttpAsyncHandlerWrapperTest drives 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 — PekkoHttpAsyncHandlerWrapperTest with default Promise propagation, and PekkoHttpAsyncHandlerWrapperForkedTest with completion priority enabled in isolated JVMs via new baseCompletionPriorityForkedTest / latestDepCompletionPriorityForkedTest tasks. Each variant asserts both PromiseHelper.completionPriority and 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:

Task Scala Completion priority Class
baseTest 2.12 off (default) PekkoHttpAsyncHandlerWrapperTest
latestDepTest 2.13 off (default) PekkoHttpAsyncHandlerWrapperTest
latestPekko10Test 2.13 off (default) PekkoHttpAsyncHandlerWrapperTest
baseCompletionPriorityForkedTest 2.12 on PekkoHttpAsyncHandlerWrapperForkedTest
latestDepCompletionPriorityForkedTest 2.13 on PekkoHttpAsyncHandlerWrapperForkedTest

Both defenses are individually pinned. The test completes the handler Promise once with a Failure and once with a Success, and each case was verified to be necessary by temporarily reverting one defense at a time:

  • Removing the root-context attachment fails both default-mode tests.
  • Removing the defensive Try copy 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.resolveTry routes failures through resolver, which allocates a fresh Failure and so incidentally strips the association; with only a failing response, 2.12 passed without the copy. The two defenses are not interchangeable either: PromiseTransformationInstrumentation and CallbackRunnableInstrumentation capture from the completing Try first and only fall back to the thread-local context when the Try carries 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 allocations transform already made, so the steady-state delta is one short-lived root-context scope per async response. The extra Try is 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) and spotlessCheck. Because the setting moved into InstrumenterConfig, internal-api and both scala-promise modules (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

Jira ticket: [PROJ-IDENT]

🤖 Generated with Claude Code

@AlexeyKuznetsov-DD AlexeyKuznetsov-DD added type: bug fix Bug fix inst: scala Scala instrumentation inst: akka Akka instrumentation tag: ai generated Largely based on code generated by an AI or LLM labels Aug 4, 2026
@AlexeyKuznetsov-DD AlexeyKuznetsov-DD self-assigned this Aug 4, 2026
@AlexeyKuznetsov-DD

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: 9b5e9dd444

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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".

@datadog-prod-us1-4

datadog-prod-us1-4 Bot commented Aug 4, 2026

Copy link
Copy Markdown

🎯 Code Coverage (details)
Patch Coverage: 0.00%
Overall Coverage: 57.89% (-0.00%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: e77d333 | Docs | Datadog PR Page | Give us feedback!

@dd-octo-sts

dd-octo-sts Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 14.97 s 14.66 s [+1.1%; +3.2%] (significantly worse)
startup:insecure-bank:tracing:Agent 13.67 s 13.78 s [-1.6%; +0.0%] (no difference)
startup:petclinic:appsec:Agent 17.35 s 17.10 s [+0.7%; +2.2%] (maybe worse)
startup:petclinic:iast:Agent 17.41 s 17.38 s [-0.7%; +0.9%] (no difference)
startup:petclinic:profiling:Agent 17.55 s 17.47 s [-1.0%; +1.8%] (no difference)
startup:petclinic:sca:Agent 17.51 s 17.14 s [+1.0%; +3.2%] (significantly worse)
startup:petclinic:tracing:Agent 16.50 s 16.71 s [-2.4%; -0.1%] (maybe better)

Commit: e77d333c · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

AlexeyKuznetsov-DD and others added 2 commits August 5, 2026 11:14
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@AlexeyKuznetsov-DD
AlexeyKuznetsov-DD changed the base branch from master to alexeyk/forked-test-source-set August 5, 2026 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inst: akka Akka instrumentation inst: scala Scala instrumentation tag: ai generated Largely based on code generated by an AI or LLM type: bug fix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant