Fix Scala firstCompletedOf continuation cleanup

[amarziali]

What Does This Do

Fixes a continuation leak in Scala 2.13.17+ when Future.firstCompletedOf unregisters callbacks from futures that did not complete first.

The instrumentation releases a callback’s continuation only when Scala’s callback-removal CAS succeeds. If completion has already dispatched the callback, ownership remains with the callback until it runs.

flowchart LR
  A[Capture callback continuation] --> B{Which operation wins?}
  B -->|Unregister CAS| C[Release continuation]
  B -->|Callback dispatch| D[Callback run consumes continuation]

Loading

Motivation

GitLab job 2087926766 intermittently timed out in:

ScalaInstrumentationTest > scala first completed future

Diagnostics identified an unresolved continuation captured by Promise$Transformation. Scala 2.13.17 introduced callback unregistration for firstCompletedOf, adding a terminal lifecycle path that bypassed the existing execution cleanup.

Unconditional cleanup on unregisterCallback would be unsafe because completion can commit the callback to execution before unregistration observes the promise state. The successful removal CAS is the point where ownership is transferred safely.

Motivation

Additional Notes

Contributor Checklist

• Format the title according to the contribution guidelines
• Assign the type: and (comp: or inst:) labels in addition to any other useful labels
• Avoid using close, fix, or any linking keywords when referencing an issue
  Use solves instead, and assign the PR milestone to the issue
• Update the CODEOWNERS file on source file addition, migration, or deletion
• Update public documentation with any new configuration flags or behaviors
• Once approved, use merge queue to merge the PR

Jira ticket: [PROJ-IDENT]

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-29T10:40:01.886277Z	a06df8d	PR opened
🔒 Security Review	✅ Completed 2026-09-29T10:39:57.770830Z	a06df8d	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[datadog-prod-us1-5]

More details

The Scala 2.13.17+ path releases the continuation only after a successful state-changing removal CAS; when callback dispatch wins, the callback retains cleanup ownership.

Was this helpful? React 👍 or 👎

_{Open Bits AI session}

_{🤖 Bits Code Review · Commit a06df8d · @DataDog review to ask questions}

[dd-octo-sts]

🟢 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.09 s	13.98 s	[-0.1%; +1.7%] (no difference)
startup:insecure-bank:tracing:Agent	12.96 s	13.01 s	[-1.1%; +0.3%] (no difference)
startup:petclinic:appsec:Agent	16.99 s	16.81 s	[+0.2%; +2.0%] (maybe worse)
startup:petclinic:iast:Agent	16.97 s	16.97 s	[-1.0%; +1.1%] (no difference)
startup:petclinic:profiling:Agent	16.59 s	16.95 s	[-3.4%; -0.9%] (maybe better)
startup:petclinic:sca:Agent	16.97 s	16.14 s	[+1.0%; +9.4%] (maybe worse)
startup:petclinic:tracing:Agent	16.27 s	16.16 s	[-0.1%; +1.5%] (no difference)

Commit: 9dd16ff7 · 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.

[vandonr]

I mean, I'm approving to unblock things, but really I don't understand half of what's happening in this code...
@dougqh if you have more Scala knowledge, review carefully, make no mistake 😅

[dougqh]

(Comment drafted by Claude on behalf of @dougqh.)

I checked the fix against the 2.13.17 source, and the mechanism is correct: unregisterCallback has exactly the two CAS sites this rewrites, and slot 1 is t throughout. My hesitation is risk versus reward, not correctness.

Reward. As far as I can tell, this fixes a flaky test timeout, and the leak looks like a delayed trace flush rather than lost or incorrect data. firstCompletedOf is also a fairly niche API. Could you spell out the production impact? For example, how long until the trace flushes, and does pending-trace memory build up in the meantime?

Risk. This is ASM surgery on a private method of the Scala stdlib. It depends on the call-site count (< 2) and the callback being in local slot 1, and neither is checked by muzzle. A Scala 2.13.x change could silently reintroduce the leak, or worse, close the wrong callback's continuation. It also adds a muzzle directive and a suite that runs twice.

Two questions:

1. Is this driven by Enforce continuation diagnostics in suite fixtures #12528's strict continuation diagnostics? If so, would exempting this scenario there, or a lighter approach, do the job?
2. If we keep the rewrite, could we add the loud-failure guards from the inline comments, plus a test with three or more futures?

I'm not blocking. I'd just like us to decide deliberately that the reward justifies this kind of instrumentation.

[amarziali]

Thanks for the thourough review @dougqh

I’ve added independent guards for the exact CAS count and preservation of the callback argument, plus mutation tests for violations.

The remaining tradeoff is explicit: an unsupported bytecode shape rejects the whole Datadog transformation for DefaultPromise, including its other advice. DefaultPromise instrumentation also, is opt-in.

Two questions:

1. Is this driven by Enforce continuation diagnostics in suite fixtures #12528's strict continuation diagnostics? If so, would exempting this scenario there, or a lighter approach, do the job?

The diagnostic is there to highlight this kind of lifecycle misses. Now we can ignore it if it's too complex to maintain. The diagnostic allows to override the check by providing a reason.

2. If we keep the rewrite, could we add the loud-failure guards from the inline comments, plus a test with three or more futures?

Yes I already did this in the PR as suggested.