Detect scope and continuation leaks in instrumentation tests

.agents/skills/apm-integrations/SKILL.md

@@ -150,8 +150,11 @@ After `updateAgentJarIntegrationsGoldenFile` runs, commit the updated `metadata/
 
 **If `checkDecoratorAnalyticsConfigurations` fails:** a name returned by the decorator's `instrumentationNames()` is missing `DD_TRACE_<NAME>_ANALYTICS_ENABLED` / `DD_TRACE_<NAME>_ANALYTICS_SAMPLE_RATE` entries in `metadata/supported-configurations.json` — add them per [Supported Configurations](references/supported-configurations.md).
 
-**If tests fail:** verify span lifecycle order (start → activate → error → close → finish), helper registration,
-and `contextStore()` map entries match actual usage.
+**If tests fail:** verify span lifecycle order (start → activate → error → close → finish), helper
+registration, and `contextStore()` map entries match actual usage. If the output contains
+`Scope/continuation timeline`, read and follow
+`.agents/skills/fix-continuation-leakage/SKILL.md`; fix the broken ownership edge rather than
+adding `strictTraceWrites(false)` or disabling the diagnostic.
 
 ## Step 11 – Checklist before finishing
 

.agents/skills/fix-continuation-leakage/SKILL.md

@@ -0,0 +1,88 @@
+---
+name: fix-continuation-leakage
+description: >
+  Diagnose and fix scope or continuation lifecycle failures in dd-trace-java instrumentation
+  tests. Use when a test reports a continuation leak, double resolution, activation after resolve,
+  or an unclosed scope, or when strictTraceWrites(false) appears to hide one. Reads the automatic
+  diagnostic timeline, finds the broken lifecycle edge, fixes it, and explains it with a compact
+  Mermaid diagram.
+user-invocable: true
+context: fork
+allowed-tools:
+  - Bash
+  - Read
+  - Edit
+  - Glob
+  - Grep
+  - AskUserQuestion
+---
+
+# Fix continuation leakage
+
+Instrumentation tests run the diagnostic automatically. A failure includes the capture, resume,
+resolution, scope, thread, timing, and callsite data needed to find the missing lifecycle edge.
+
+## Work the failure
+
+1. Run the smallest failing test with full output:
+
+```bash
+./gradlew :dd-java-agent:instrumentation:<framework>-<minVersion>:test --tests '<FQCN-or-pattern>' --info 2>&1 | tee /tmp/scopediag-run.txt
+```
+
+2. Find `Scope/continuation timeline` in the output. If Gradle hides it, inspect the test XML's
+   `<system-out>` under the module's `build/test-results` directory.
+3. Follow the failing record from its first event:
+   - `LEAKED` / `NEVER_CLOSED`: find the success, error, cancellation, and rejection exits that
+     skipped `release()` or `close()`.
+   - `DOUBLE_FINISH`: find two owners of the same cleanup.
+   - `ACTIVATE_AFTER_RESOLVE`: find work scheduled after ownership ended.
+   - `LATE_FINISH` / `CLOSE_WRONG_THREAD`: advisory evidence; verify whether ordering is valid.
+   - `[deferred-cleanup]`: a root iteration scope transferred cleanup to the bounded iteration
+     cleaner. It may remain open at the test boundary and is not a leak. Do not generalize this to
+     other `ITERATION` scopes; an unregistered iteration scope must still close normally.
+4. Classify the captured work before changing code:
+   - For a real asynchronous operation, repair success, failure, cancellation, and rejection
+     cleanup.
+   - If the test started the work, wait for its terminal event and dispose or close it before the
+     test ends.
+   - If a framework initializer creates a permanent sentinel with no context consumer, disable
+     propagation only around that creation boundary. Match the exact type and method, and update
+     `knownMatchingTypes()` when shortcut matching is used.
+   - If an executor replaces a task before delegating, avoid capturing the discarded task while
+     preserving capture for the task actually submitted.
+   - For intentionally delayed work, wait for its documented terminal event rather than
+     suppressing propagation.
+   - For a context swap, verify both restoration and resource cleanup. Restore or close the
+     returned ownership object in `finally`; do not ignore every swap.
+5. Prefer a test-lifecycle fix when production behavior is correct. Otherwise fix ownership where
+   it breaks, with one owner and `try/finally` cleanup across every exit.
+6. Rerun the failing test, then its module. Validate the leaked record and root-trace publication
+   separately from trace-count or arrival-order assertions; fixing a leak may expose an unrelated
+   flaky assertion.
+
+## Fixture setup failures
+
+Automatic recording may start after `setupSpec()` or equivalent fixture initialization. For an
+initialization error or a trace wait inside setup, temporarily record around that setup block and
+remove the diagnostic scaffolding after finding the owner.
+
+Apply process-wide configuration before starting servers, actor systems, executors, or other
+long-lived fixtures. Use a forked test or recreate the fixture when its static state cannot be
+reset safely.
+
+## Do not hide evidence
+
+Do not make the test green with `strictTraceWrites(false)` or
+`@TrackScopeContinuations(enabled=false, reason="...")`. Those hide evidence. The opt-out requires
+a reason and is only for a proven diagnostic incompatibility. If the failure is genuinely
+intermittent, treat that as a flaky-test finding, keep diagnostics enabled, and link the `@Flaky`
+annotation to a tracked issue.
+
+## Explain it to a human
+
+Lead with one sentence: what was captured, which cleanup edge was missing, and where. Cite the
+timeline callsites. Then include a small Mermaid `flowchart LR`; use green for healthy edges, red
+for the broken edge, and label thread handoffs. Use a Gantt only when timing itself caused the bug.
+
+End with the code fix and the exact tests that passed.

.claude/skills/fix-continuation-leakage/SKILL.md

@@ -0,0 +1,23 @@
+---
+name: fix-continuation-leakage
+description: >
+  Diagnose and fix scope or continuation lifecycle failures in dd-trace-java instrumentation
+  tests. Use when a test reports a continuation leak, double resolution, activation after resolve,
+  or an unclosed scope, or when strictTraceWrites(false) appears to hide one. Reads the automatic
+  diagnostic timeline, finds the broken lifecycle edge, fixes it, and explains it with a compact
+  Mermaid diagram.
+user-invocable: true
+context: fork
+allowed-tools:
+  - Bash
+  - Read
+  - Edit
+  - Glob
+  - Grep
+  - AskUserQuestion
+---
+
+# Fix continuation leakage
+
+Read `.agents/skills/fix-continuation-leakage/SKILL.md` in full and follow it. That file is the
+shared playbook for repository agents.

dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/InstrumentationSpecification.groovy

@@ -33,6 +33,8 @@ import datadog.metrics.impl.DDSketchHistograms
 import datadog.metrics.impl.MonitoringImpl
 import datadog.trace.agent.test.asserts.ListWriterAssert
 import datadog.trace.agent.test.asserts.TagsAssert
+import datadog.trace.agent.test.scopediag.ScopeDiagnostics
+import datadog.trace.agent.test.scopediag.TrackScopeContinuations
 import datadog.trace.agent.test.datastreams.MockFeaturesDiscovery
 import datadog.trace.agent.test.datastreams.RecordingDatastreamsPayloadWriter
 import datadog.trace.agent.tooling.AgentInstaller
@@ -467,6 +469,9 @@ abstract class InstrumentationSpecification extends DDSpecification implements A
     }
 
     TEST_WRITER.start()
+    if (scopeDiagnosticsEnabled()) {
+      ScopeDiagnostics.startRecording()
+    }
     TEST_DATA_STREAMS_WRITER.clear()
     TEST_DATA_STREAMS_MONITORING.clear()
 
@@ -500,27 +505,73 @@ abstract class InstrumentationSpecification extends DDSpecification implements A
     }
     TEST_TRACER.flush()
 
-    def util = new MockUtil()
-    util.detachMock(STATS_D_CLIENT)
+    def scopeDiagnosticsFailure = reportScopeDiagnostics()
+
+    try {
+      def util = new MockUtil()
+      util.detachMock(STATS_D_CLIENT)
+
+      ActiveSubsystems.APPSEC_ACTIVE = originalAppSecRuntimeValue
+
+      if (Config.get().isDebuggerCodeOriginEnabled()) {
+        injectSysConfig(CODE_ORIGIN_FOR_SPANS_ENABLED, "false", true)
+        rebuildConfig()
+      }
 
-    ActiveSubsystems.APPSEC_ACTIVE = originalAppSecRuntimeValue
+      try {
+        if (enabledFinishTimingChecks()) {
+          doCheckRepeatedFinish()
+        }
+      } finally {
+        spanFinishLocations.clear()
+        originalToTrackingSpan.clear()
+      }
 
-    if (Config.get().isDebuggerCodeOriginEnabled()) {
-      injectSysConfig(CODE_ORIGIN_FOR_SPANS_ENABLED, "false", true)
-      rebuildConfig()
+      // check for instrumentation issues while running each test
+      assert InstrumentationErrors.noErrors(): InstrumentationErrors.describeErrors()
+    } catch (Throwable cleanupFailure) {
+      if (scopeDiagnosticsFailure != null) {
+        cleanupFailure.addSuppressed(scopeDiagnosticsFailure)
+      }
+      throw cleanupFailure
+    }
+    if (scopeDiagnosticsFailure != null) {
+      throw scopeDiagnosticsFailure
     }
+  }
+
+  private TrackScopeContinuations scopeDiagConfig() {
+    def method = specificationContext?.currentFeature?.featureMethod?.reflection
+    def ann = method?.getAnnotation(TrackScopeContinuations)
+    if (ann == null) {
+      ann = this.class.getAnnotation(TrackScopeContinuations)
+    }
+    return ann
+  }
+
+  private boolean scopeDiagnosticsEnabled() {
+    return ScopeDiagnostics.isEnabled(scopeDiagConfig())
+  }
 
+  private Throwable reportScopeDiagnostics() {
+    def config = scopeDiagConfig()
+    if (!ScopeDiagnostics.isEnabled(config)) {
+      return null
+    }
     try {
-      if (enabledFinishTimingChecks()) {
-        doCheckRepeatedFinish()
+      ScopeDiagnostics.awaitQuiescence()
+      ScopeDiagnostics.stop()
+      def report = ScopeDiagnostics.report()
+      if (report.hasFindings()) {
+        println(report.renderTimeline())
       }
+      ScopeDiagnostics.assertNoLeaks(report)
+      return null
+    } catch (Throwable failure) {
+      return failure
     } finally {
-      spanFinishLocations.clear()
-      originalToTrackingSpan.clear()
+      ScopeDiagnostics.reset()
     }
-
-    // check for instrumentation issues while running each test
-    assert InstrumentationErrors.noErrors(): InstrumentationErrors.describeErrors()
   }
 
   private void doCheckRepeatedFinish() {

dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/WithHttpServer.groovy

@@ -33,6 +33,10 @@ abstract class WithHttpServer<SERVER> extends VersionedNamingTestBase {
     return new DefaultHttpServer()
   }
 
+  boolean recreateServerForEachTest() {
+    false
+  }
+
   private class DefaultHttpServer implements HttpServer {
     final ServerSocket socket = PortUtils.randomOpenSocket()
     final int port = socket.localPort
@@ -64,6 +68,30 @@ abstract class WithHttpServer<SERVER> extends VersionedNamingTestBase {
   }
 
   void setupSpec() {
+    if (!recreateServerForEachTest()) {
+      startHttpServer()
+    }
+  }
+
+  void setup() {
+    if (recreateServerForEachTest()) {
+      startHttpServer()
+    }
+  }
+
+  void cleanup() {
+    if (recreateServerForEachTest()) {
+      stopHttpServer()
+    }
+  }
+
+  void cleanupSpec() {
+    if (!recreateServerForEachTest()) {
+      stopHttpServer()
+    }
+  }
+
+  private void startHttpServer() {
     server = server()
     server.start()
     address = server.address()
@@ -72,7 +100,7 @@ abstract class WithHttpServer<SERVER> extends VersionedNamingTestBase {
     println "$server started at: $address"
   }
 
-  void cleanupSpec() {
+  private void stopHttpServer() {
     server.stop()
     println "$server stopped at: $address"
   }

dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/AbstractInstrumentationTest.java

@@ -7,6 +7,7 @@
 import datadog.instrument.classinject.ClassInjector;
 import datadog.trace.agent.test.assertions.TraceAssertions;
 import datadog.trace.agent.test.assertions.TraceMatcher;
+import datadog.trace.agent.test.scopediag.ScopeDiagnosticsExtension;
 import datadog.trace.agent.tooling.AgentInstaller;
 import datadog.trace.agent.tooling.InstrumenterModule;
 import datadog.trace.agent.tooling.TracerInstaller;
@@ -58,7 +59,8 @@
 @ExtendWith({
   TestClassShadowingExtension.class,
   AllowContextTestingExtension.class,
-  LegacyContextTestingExtension.class
+  LegacyContextTestingExtension.class,
+  ScopeDiagnosticsExtension.class
 })
 public abstract class AbstractInstrumentationTest {
   static final Instrumentation INSTRUMENTATION = ByteBuddyAgent.getInstrumentation();

dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/scopediag/ContinuableScopeAdvice.java

@@ -0,0 +1,22 @@
+package datadog.trace.agent.test.scopediag;
+
+import net.bytebuddy.asm.Advice;
+
+/** Test-only advice for {@code ContinuableScope}. */
+public final class ContinuableScopeAdvice {
+  private ContinuableScopeAdvice() {}
+
+  public static final class OnProperClose {
+    @Advice.OnMethodExit(suppress = Throwable.class)
+    public static void exit(@Advice.This Object scope) {
+      ScopeContinuationProbe.onScopeClose(scope);
+    }
+  }
+
+  public static final class Close {
+    @Advice.OnMethodEnter(suppress = Throwable.class)
+    public static void enter(@Advice.This Object scope) {
+      ScopeContinuationProbe.onScopeClosing(scope);
+    }
+  }
+}

dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/scopediag/ContinuableScopeManagerAdvice.java

@@ -0,0 +1,15 @@
+package datadog.trace.agent.test.scopediag;
+
+import net.bytebuddy.asm.Advice;
+
+/** Test-only advice for scopes owned by the iteration cleaner. */
+public final class ContinuableScopeManagerAdvice {
+  private ContinuableScopeManagerAdvice() {}
+
+  public static final class ScheduleRootIterationCleanup {
+    @Advice.OnMethodExit(suppress = Throwable.class)
+    public static void exit(@Advice.Argument(1) Object scope) {
+      ScopeContinuationProbe.onDeferredScopeCleanup(scope);
+    }
+  }
+}

dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/scopediag/ContinuationAdvice.java

@@ -0,0 +1,50 @@
+package datadog.trace.agent.test.scopediag;
+
+import net.bytebuddy.asm.Advice;
+
+/** Test-only advice for {@code ScopeContinuation}. */
+public final class ContinuationAdvice {
+  private ContinuationAdvice() {}
+
+  public static final class Register {
+    @Advice.OnMethodExit(suppress = Throwable.class)
+    public static void exit(@Advice.This Object self) {
+      ScopeContinuationProbe.onCapture(self);
+    }
+  }
+
+  /** Timestamps entry because {@code resume()} may resolve the continuation before returning. */
+  public static final class Activate {
+    @Advice.OnMethodEnter
+    public static long enter() {
+      return System.nanoTime();
+    }
+
+    @Advice.OnMethodExit(suppress = Throwable.class)
+    public static void exit(
+        @Advice.This Object self, @Advice.Enter long ddActivateNanos, @Advice.Return Object scope) {
+      ScopeContinuationProbe.onActivate(self, scope, ddActivateNanos);
+    }
+  }
+
+  /** Timestamps entry because resolution may write the trace before the method returns. */
+  public static final class Cancel {
+    @Advice.OnMethodEnter
+    public static int enter(
+        @Advice.FieldValue("count") int count,
+        @Advice.Local("ddResolveNanos") long ddResolveNanos) {
+      ddResolveNanos = System.nanoTime();
+      return count;
+    }
+
+    @Advice.OnMethodExit(suppress = Throwable.class)
+    public static void exit(
+        @Advice.This Object self,
+        @Advice.Origin("#m") String method,
+        @Advice.Enter int countBefore,
+        @Advice.Local("ddResolveNanos") long ddResolveNanos,
+        @Advice.FieldValue("count") int countAfter) {
+      ScopeContinuationProbe.onResolve(self, method, countBefore, countAfter, ddResolveNanos);
+    }
+  }
+}