Fix Metaspace leak in SCA Reachability retransform and opcode injection

[jandro996]

What Does This Do

• Bounds ScaReachabilityTransformer.performPendingRetransforms() retries: a class that keeps failing Instrumentation.retransformClasses() is dropped instead of being re-queued forever. retransformClasses() fails atomically for the whole batch when any class in it fails, so without this bound a single problematic class retained strong references to every Class<?>/ClassLoader in its batch indefinitely. (The initial version of this bound used a per-class retransformFailureCount map and a MAX_RETRANSFORM_ATTEMPTS cutoff of 5; superseded below by the batching rework.)
• Extends ScaMethodCallbackInjector.MethodEntryInjector to override all 13 ASM MethodVisitor visitXxxInsn callbacks (visitTypeInsn, visitIntInsn, visitLdcInsn, visitJumpInsn, visitIincInsn, visitTableSwitchInsn, visitLookupSwitchInsn, visitMultiANewArrayInsn, visitInvokeDynamicInsn, in addition to the 4 already handled: visitInsn, visitVarInsn, visitMethodInsn, visitFieldInsn). Previously, methods whose first bytecode instruction was one of the unhandled opcodes skipped the entry-point callback injection, delaying (or in some class-loading orders, dropping) reachability detection for those methods.
• Reworks pendingRetransform from a flat ConcurrentLinkedQueue<Class<?>> into a ConcurrentLinkedQueue<List<Class<?>>> of independent batches, and retransforms each batch via its own retransformClasses() call instead of merging everything drained in a heartbeat into one array. On failure, a multi-class batch is bisected into two halves that are deferred to the next heartbeat (never retried recursively within the same call), isolating a healthy class from an unrelated, permanently-failing batch-mate instead of dropping both together. Once a batch is narrowed down to a single class, a failure drops that class immediately — there is no retry budget for singleton batches (see Additional Notes for the trade-off this implies). This replaces the original retransformFailureCount/MAX_RETRANSFORM_ATTEMPTS approach entirely.
• Adds regression tests: ScaReachabilityRetransformTest covers immediate drop of a singleton batch on failure, no re-queue on success, and bisection isolating a healthy class from a permanently-failing batch-mate. ScaReachabilityMethodLevelTest white-box verifies, via a custom ASM ClassVisitor, that the injected callback is the first instruction visited for methods starting with each of the previously unhandled opcodes.

Motivation

Fixes a Metaspace leak observed with DD_APPSEC_SCA_ENABLED=true: retained Class<?> references from the unbounded retransform retry loop prevented affected ClassLoaders from being garbage collected.

Additional Notes

The batching rework was suggested by @mcculls during review: the original per-class attempt-count approach bounded retention (no Metaspace leak) but, since performPendingRetransforms() drained the entire pending queue into a single retransform batch per heartbeat, an unrelated healthy class sharing a batch with a permanently-failing one accumulated the same failure count and was dropped alongside it rather than being retransformed on its own. Bisecting failing batches (like git bisect) isolates the failing class(es) within O(log n) heartbeats instead.

Retry policy for singleton batches. Once bisection narrows a batch down to a single class, a further failure now drops that class immediately, with no retry budget (the earlier retransformFailureCount/MAX_RETRANSFORM_ATTEMPTS approach has been removed). This is a deliberate trade-off, discussed with @mcculls: a class that reaches singleton-batch size has either already failed repeatedly as part of larger batches, or was consistently unlucky enough to share a batch with a truly-failing class — a solo failure at that point is treated as sufficient evidence to give up, since retransformClasses() failures are typically deterministic (verification/redefinition issues), not transient.

The implication: a class that hits a genuinely transient failure at the exact moment it is tried alone loses method-level SCA reachability detection for that class, silently (no retry, no metric — only a DEBUG log line). This is most exposed for classes that start as singleton batches straight out of checkAlreadyLoadedClasses(), since their first attempt is also their only attempt. We're accepting this because the priority for this feature is performance first, detection second — fewer heavyweight retransformClasses() calls and faster convergence under failure storms outweighs tolerating a rare transient failure. If a specific class is ever found to be silently missing detection, this is the first place to check.

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

Jira ticket: APPSEC-69201

Note: Once your PR is ready to merge, add it to the merge queue by commenting /merge. /merge -c cancels the queue request. /merge -f --reason "reason" skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.

[jandro996]

@codex review

[chatgpt-codex-connector]

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

Reviewed commit: b19360ad90

ℹ️ 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-datadog-prod-us1-2]

🎯 Code Coverage (details)
• Patch Coverage: 90.00%
• Overall Coverage: 57.18% (-0.11%)

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

[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	13.98 s	13.92 s	[-0.2%; +1.1%] (no difference)
startup:insecure-bank:tracing:Agent	12.90 s	13.04 s	[-1.7%; -0.4%] (maybe better)
startup:petclinic:appsec:Agent	16.88 s	16.75 s	[-0.3%; +1.7%] (no difference)
startup:petclinic:iast:Agent	16.86 s	16.89 s	[-0.9%; +0.5%] (no difference)
startup:petclinic:profiling:Agent	16.05 s	16.17 s	[-7.8%; +6.3%] (unstable)
startup:petclinic:sca:Agent	16.90 s	16.67 s	[+0.3%; +2.5%] (maybe worse)
startup:petclinic:tracing:Agent	16.16 s	16.16 s	[-1.0%; +1.1%] (no difference)

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

[mcculls]

@jandro996 is the batching arbitrary, or do certain classes need to be in the same batch?

If it's arbitrary then another approach could be to bisect any failing batches into 2 smaller batches - the half without the permanently failing class should then succeed, and you could then go on to bisect the other half. Assuming that there were only one or two failing classes in a batch then this should narrow down to those very quickly, much like git bisect. (You could even estimate the worst case if you know the starting batch size.)

This would avoid the extra map from class to failure count - you'd just split any failing batch in half before resubmitting them, everything else could stay the same.

[jandro996]

@jandro996 is the batching arbitrary, or do certain classes need to be in the same batch?

If it's arbitrary then another approach could be to bisect any failing batches into 2 smaller batches - the half without the permanently failing class should then succeed, and you could then go on to bisect the other half. Assuming that there were only one or two failing classes in a batch then this should narrow down to those very quickly, much like git bisect. (You could even estimate the worst case if you know the starting batch size.)

This would avoid the extra map from class to failure count - you'd just split any failing batch in half before resubmitting them, everything else could stay the same.

Batching is arbitrary, yes! and I really like the bisection idea, it isolates the healthy class instead of dropping it alongside the poison one.
Two things to sort out first: pendingRetransform would need to become a queue of batches instead of a flat queue (otherwise the next heartbeat re-merges both halves and undoes the bisection), and we'd still need a bound once bisection narrows down to a single permanently-failing class, or that one leaks forever.
Do you agree with that?

[mcculls]

Two things to sort out first: pendingRetransform would need to become a queue of batches instead of a flat queue (otherwise the next heartbeat re-merges both halves and undoes the bisection), and we'd still need a bound once bisection narrows down to a single permanently-failing class, or that one leaks forever.
Do you agree with that?

Yes, it would need to be a queue of batches to keep them separate - unless you had 2 queues: a flat queue of classes not yet attempted, and a queue of failed batches to retry (either way you need at least one queue of batches as soon as a batch fails.)

Wrt. the bound - once it's been narrowed down to a single permanently-failing class, if that batch fails then you know you can discard at that point. If you want to fail earlier then you can discard once the batch size is below a threshold. That way you avoid any need for counting failed attempts (it's implicit in the size of the batch.)

[jandro996]

Two things to sort out first: pendingRetransform would need to become a queue of batches instead of a flat queue (otherwise the next heartbeat re-merges both halves and undoes the bisection), and we'd still need a bound once bisection narrows down to a single permanently-failing class, or that one leaks forever.
Do you agree with that?

Yes, it would need to be a queue of batches to keep them separate - unless you had 2 queues: a flat queue of classes not yet attempted, and a queue of failed batches to retry (either way you need at least one queue of batches as soon as a batch fails.)

Wrt. the bound - once it's been narrowed down to a single permanently-failing class, if that batch fails then you know you can discard at that point. If you want to fail earlier then you can discard once the batch size is below a threshold. That way you avoid any need for counting failed attempts (it's implicit in the size of the batch.)

Good point! My only hesitation... a singleton batch's first failure could still be a transient issue (e.g. transient contention with another transformer), not necessarily proof that the class itself is permanently broken, right?. If we discard on the first failure at size 1, we lose that tolerance.
Would you be OK with keeping a small fixed retry budget just for the singleton case (e.g. 2 attempts) instead of 5, so we drop the failure-count map's complexity but still tolerate one transient blip? Or do you think that risk is negligible in practice?

[mcculls]

Good point! My only hesitation... a singleton batch's first failure could still be a transient issue (e.g. transient contention with another transformer), not necessarily proof that the class itself is permanently broken, right?. If we discard on the first failure at size 1, we lose that tolerance.
Would you be OK with keeping a small fixed retry budget just for the singleton case (e.g. 2 attempts) instead of 5, so we drop the failure-count map's complexity but still tolerate one transient blip? Or do you think that risk is negligible in practice?

It could be transient - but for that class to have survived down to a singleton batch means that it either repeatedly failed, or it was unlucky to always be in a batch with a failing class - and then transiently failed when it was on its own.

If you did want to retry for the singleton case then you could still avoid having a class->int map by keeping the retry count in the batch object - for example by extending ArrayList and adding the count as an extra field, or by having a batch class which contains an array of classes and the retry count.

[jandro996]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 46d93b7f9d

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

[jandro996]

@codex review

[chatgpt-codex-connector]

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

Reviewed commit: 1f1b4a3a0a

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

[jandro996]

@codex review

[chatgpt-codex-connector]

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

Reviewed commit: aa55d83ba3

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

[jandro996]

Good point! My only hesitation... a singleton batch's first failure could still be a transient issue (e.g. transient contention with another transformer), not necessarily proof that the class itself is permanently broken, right?. If we discard on the first failure at size 1, we lose that tolerance.
Would you be OK with keeping a small fixed retry budget just for the singleton case (e.g. 2 attempts) instead of 5, so we drop the failure-count map's complexity but still tolerate one transient blip? Or do you think that risk is negligible in practice?

It could be transient - but for that class to have survived down to a singleton batch means that it either repeatedly failed, or it was unlucky to always be in a batch with a failing class - and then transiently failed when it was on its own.

If you did want to retry for the singleton case then you could still avoid having a class->int map by keeping the retry count in the batch object - for example by extending ArrayList and adding the count as an extra field, or by having a batch class which contains an array of classes and the retry count.

I made changes to take the simple option, drop on the first failure once a batch is down to size 1, no retry budget at all. No need for the batch-with-count trick either, since we're not retrying the singleton case in the first place. If a specific class ever turns out to be silently missing detection because of this, we know exactly where to look

[datadog-datadog-prod-us1-2]

More details

The PR fixes a Metaspace leak in SCA Reachability by implementing a bisection-based retry strategy for failed retransform batches and extending ASM opcode coverage for method entry detection. Comprehensive diff analysis confirms the batching logic correctly isolates failing classes, all 9 new ASM visitor overrides follow the established pattern for callback injection, and edge cases (empty batches, non-modifiable classes, multiple classloader instances) are properly handled. No behavioral regressions detected.

Was this helpful? React 👍 or 👎

_{📊 Validated against 7 scenarios · Open Bits AI session}

_{🤖 Datadog Autotest · Commit aa55d83 · What is Autotest? · Any feedback? Reach out in #autotest}