Fix DDTraceId/DD64bTraceId class-initialization deadlock

dd-java-agent/instrumentation/spark/spark-common/src/main/java/datadog/trace/instrumentation/spark/AbstractDatadogSparkListener.java

@@ -444,7 +444,7 @@ private void addDatabricksSpecificTags(
       AgentSpanContext parentContext =
           new DatabricksParentContext(databricksJobId, databricksJobRunId, databricksTaskRunId);
 
-      if (parentContext.getTraceId() != DDTraceId.ZERO) {
+      if (parentContext.getTraceId().isValid()) {
         if (withParentContext) {
           builder.asChildOf(parentContext);
         } else {

dd-trace-api/build.gradle.kts

@@ -16,6 +16,7 @@ val excludedClassesCoverage by extra(
     "datadog.trace.api.DDTags",
     "datadog.trace.api.DDTraceApiInfo",
     "datadog.trace.api.DDTraceId",
+    "datadog.trace.api.DDTraceIdConstant",
     "datadog.trace.api.EventTracker",
     "datadog.trace.api.GlobalTracer*",
     "datadog.trace.api.PropagationStyle",

dd-trace-api/src/main/java/datadog/trace/api/DD64bTraceId.java

@@ -1,6 +1,7 @@
 package datadog.trace.api;
 
 import datadog.trace.api.internal.util.LongStringUtils;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 
 /**
  * Class encapsulating the unsigned 64 bit id used for Traceids.
@@ -12,6 +13,8 @@
 public class DD64bTraceId extends DDTraceId {
   public static final DD64bTraceId MAX =
       new DD64bTraceId(-1, "18446744073709551615"); // All bits set
+  // Cached zero so create()/from() don't allocate per zero id. Value-equal to DDTraceId.ZERO.
+  private static final DD64bTraceId ZERO_ID = new DD64bTraceId(0, "0");
 
   private final long id;
   private String str; // cache for string representation
@@ -59,23 +62,27 @@ public static DD64bTraceId fromHex(String s) throws NumberFormatException {
   }
 
   static DD64bTraceId create(long id, String str) {
-    // ZERO constant is created and stored by the parent class as part of its API contract
-    // But initialized by this 64-bit child class. Ensures uniqueness of ZERO once created.
-    if (id == 0 && ZERO != null) {
-      return (DD64bTraceId) ZERO;
-    } else if (id == -1) {
+    // Reuse cached singletons rather than allocating: -1 -> MAX, 0 -> ZERO_ID.
+    if (id == -1) {
       return MAX;
+    } else if (id == 0) {
+      return ZERO_ID;
     } else {
       return new DD64bTraceId(id, str);
     }
   }
 
   @Override
+  @SuppressFBWarnings(
+      value = "EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS",
+      justification =
+          "DDTraceIdConstant is a sibling type backing ZERO/ONE; equal by 64-bit value.")
   public boolean equals(Object o) {
     if (this == o) return true;
-    if (!(o instanceof DD64bTraceId)) return false;
-    DD64bTraceId ddId = (DD64bTraceId) o;
-    return this.id == ddId.id;
+    if (o instanceof DD64bTraceId) return this.id == ((DD64bTraceId) o).id;
+    // Also value-equal to the DDTraceIdConstant backing ZERO/ONE (both 64-bit ids).
+    if (o instanceof DDTraceIdConstant) return this.id == ((DDTraceIdConstant) o).toLong();
+    return false;
   }
 
   @Override

dd-trace-api/src/main/java/datadog/trace/api/DDTraceId.java

@@ -1,19 +1,21 @@
 package datadog.trace.api;
 
-import datadog.trace.api.internal.util.LongStringUtils;
-
 /**
  * Class encapsulating the id used for TraceIds.
  *
  * <p>It contains parsing and formatting to string for both decimal and hexadecimal representations.
  * The string representations are either kept from parsing, or generated on demand and cached.
  */
 public abstract class DDTraceId {
-  /** Invalid TraceId value used to denote no TraceId. */
-  public static final DDTraceId ZERO = from(0);
+  // ZERO/ONE use DDTraceIdConstant (a sibling of DD64bTraceId) rather than DD64bTraceId so that
+  // initializing DDTraceId never initializes its subclass, which would deadlock. Test for zero with
+  // isValid(), not == ZERO.
+
+  /** Invalid TraceId value used to denote no/unset TraceId. */
+  public static final DDTraceId ZERO = new DDTraceIdConstant(0, "0");
 
-  /** Convenience constant used from tests */
-  public static final DDTraceId ONE = from(1);
+  /** Convenience constant used from tests. */
+  public static final DDTraceId ONE = new DDTraceIdConstant(1, "1");
 
   /**
    * Creates a new {@link DD64bTraceId 64-bit TraceId} from the given {@code long} interpreted as
@@ -24,7 +26,8 @@ public abstract class DDTraceId {
    * @return A new {@link DDTraceId} instance.
    */
   public static DDTraceId from(long id) {
-    return DD64bTraceId.from(id);
+    // Zero maps to the ZERO constant so callers can still compare against it.
+    return id == 0 ? ZERO : DD64bTraceId.from(id);
   }
 
   /**
@@ -36,7 +39,8 @@ public static DDTraceId from(long id) {
    * @throws NumberFormatException If the given {@link String} does not represent a valid number.
    */
   public static DDTraceId from(String s) throws NumberFormatException {
-    return DD64bTraceId.create(LongStringUtils.parseUnsignedLong(s), s);
+    DD64bTraceId id = DD64bTraceId.from(s);
+    return id.toLong() == 0 ? ZERO : id;
   }
 
   /**
@@ -52,7 +56,11 @@ public static DDTraceId fromHex(String s) throws NumberFormatException {
     if (s == null) {
       throw new NumberFormatException("s cannot be null");
     }
-    return s.length() > 16 ? DD128bTraceId.fromHex(s) : DD64bTraceId.fromHex(s);
+    if (s.length() > 16) {
+      return DD128bTraceId.fromHex(s);
+    }
+    DD64bTraceId id = DD64bTraceId.fromHex(s);
+    return id.toLong() == 0 ? ZERO : id;
   }
 
   /**
@@ -102,4 +110,14 @@ public static DDTraceId fromHex(String s) throws NumberFormatException {
    *     <code>0</code> for 64-bit {@link DDTraceId} only.
    */
   public abstract long toHighOrderLong();
+
+  /**
+   * Returns whether this is a valid (non-zero) {@link DDTraceId}; zero denotes no/unset TraceId.
+   * Value-based, so it recognizes a zero id of any concrete type.
+   *
+   * @return {@code true} if any bit is non-zero.
+   */
+  public boolean isValid() {
+    return toHighOrderLong() != 0 || toLong() != 0;
+  }
 }

dd-trace-api/src/main/java/datadog/trace/api/DDTraceIdConstant.java

@@ -0,0 +1,73 @@
+package datadog.trace.api;
+
+import datadog.trace.api.internal.util.LongStringUtils;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
+/**
+ * Backs {@link DDTraceId#ZERO} and {@link DDTraceId#ONE}. A 64-bit id that is a sibling of {@link
+ * DD64bTraceId} (it extends {@link DDTraceId} directly) so initializing {@code DDTraceId} never
+ * initializes its subclass; value-equal to the equivalent {@link DD64bTraceId}.
+ *
+ * <p>Only ever initialize this through {@link DDTraceId}'s constants. Initializing it independently
+ * (e.g. a static-member access) would bring back the class-initialization deadlock.
+ */
+final class DDTraceIdConstant extends DDTraceId {
+  private final long id;
+  private final String str;
+  private String hexStr; // cache for hex string representation
+
+  DDTraceIdConstant(long id, String str) {
+    this.id = id;
+    this.str = str;
+  }
+
+  @Override
+  public String toString() {
+    return this.str;
+  }
+
+  @Override
+  public String toHexString() {
+    String hexStr = this.hexStr;
+    // This race condition is intentional and benign.
+    // The worst that can happen is that an identical value is produced and written into the field.
+    if (hexStr == null) {
+      this.hexStr = hexStr = LongStringUtils.toHexStringPadded(this.id, 32);
+    }
+    return hexStr;
+  }
+
+  @Override
+  public String toHexStringPadded(int size) {
+    if (size > 16) {
+      return toHexString();
+    }
+    return LongStringUtils.toHexStringPadded(this.id, size);
+  }
+
+  @Override
+  public long toLong() {
+    return this.id;
+  }
+
+  @Override
+  public long toHighOrderLong() {
+    return 0;
+  }
+
+  @Override
+  @SuppressFBWarnings(
+      value = "EQ_CHECK_FOR_OPERAND_NOT_COMPATIBLE_WITH_THIS",
+      justification = "DD64bTraceId is a sibling type; ZERO/ONE are equal to it by 64-bit value.")
+  public boolean equals(Object o) {
+    if (this == o) return true;
+    if (o instanceof DD64bTraceId) return this.id == ((DD64bTraceId) o).toLong();
+    if (o instanceof DDTraceIdConstant) return this.id == ((DDTraceIdConstant) o).id;
+    return false;
+  }
+
+  @Override
+  public int hashCode() {
+    return (int) (this.id ^ (this.id >>> 32));
+  }
+}

dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdClinitDeadlockForkedTest.java

@@ -0,0 +1,88 @@
+package datadog.trace.api;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.atomic.AtomicReference;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
+
+/**
+ * Regression test for the {@code DDTraceId} <-> {@code DD64bTraceId} class-initialization
+ * deadlock.
+ *
+ * <p>{@code DD64bTraceId} is a subclass of {@code DDTraceId}, so the JVM must initialize {@code
+ * DDTraceId} before {@code DD64bTraceId}. The bug was that {@code DDTraceId.<clinit>} in turn
+ * initialized {@code DD64bTraceId} by building its {@code ZERO}/{@code ONE} constants via {@code
+ * DD64bTraceId.from(...)}. When the two classes were first touched concurrently from opposite ends
+ * (one thread initializing {@code DDTraceId}, another initializing {@code DD64bTraceId}), each
+ * thread held one class-initialization lock and waited for the other, hanging trace creation. This
+ * surfaced as 30s {@code LogInjectionSmokeTest} timeouts in CI.
+ *
+ * <p>{@code DDTraceId.ZERO}/{@code ONE} are now instances of a private sibling type (not {@code
+ * DD64bTraceId}), so {@code DDTraceId.<clinit>} no longer references {@code DD64bTraceId} and the
+ * cycle is gone. This test initializes the two classes for the first time concurrently from
+ * opposite ends and asserts neither thread hangs.
+ *
+ * <p>Runs forked ({@code forkEvery = 1}) so it gets a fresh JVM in which these classes have not yet
+ * been initialized by another test. Without the fix it deadlocks and fails via the join check (and
+ * the {@code @Timeout} backstop); with the fix it completes immediately.
+ */
+class DDTraceIdClinitDeadlockForkedTest {
+
+  @Test
+  @Timeout(value = 60, unit = SECONDS) // backstop; the join below is the primary guard
+  void traceIdClassPairInitializesConcurrentlyWithoutDeadlock() throws Exception {
+    final ClassLoader cl = getClass().getClassLoader();
+    final CyclicBarrier barrier = new CyclicBarrier(2);
+    final AtomicReference<Throwable> error = new AtomicReference<>();
+
+    // One thread enters via the superclass (mirrors blackholeSpan() -> DDTraceId.ZERO), the other
+    // via the subclass (mirrors IdGenerationStrategy.generateTraceId() -> DD64bTraceId.from()).
+    Thread viaSuper =
+        new Thread(
+            () -> {
+              try {
+                barrier.await();
+                Class.forName("datadog.trace.api.DDTraceId", true, cl);
+              } catch (Throwable t) {
+                error.compareAndSet(null, t);
+              }
+            },
+            "init-DDTraceId");
+    Thread viaSub =
+        new Thread(
+            () -> {
+              try {
+                barrier.await();
+                Class.forName("datadog.trace.api.DD64bTraceId", true, cl);
+              } catch (Throwable t) {
+                error.compareAndSet(null, t);
+              }
+            },
+            "init-DD64bTraceId");
+    // Daemon so a deadlock cannot block forked-JVM shutdown.
+    viaSuper.setDaemon(true);
+    viaSub.setDaemon(true);
+
+    viaSuper.start();
+    viaSub.start();
+    viaSuper.join(SECONDS.toMillis(15));
+    viaSub.join(SECONDS.toMillis(15));
+
+    if (viaSuper.isAlive() || viaSub.isAlive()) {
+      fail(
+          "DDTraceId/DD64bTraceId class-initialization deadlock: DDTraceId.<clinit> must not "
+              + "reference DD64bTraceId (init-DDTraceId.alive="
+              + viaSuper.isAlive()
+              + ", init-DD64bTraceId.alive="
+              + viaSub.isAlive()
+              + ").");
+    }
+    if (error.get() != null) {
+      throw new AssertionError(
+          "Unexpected error during concurrent class initialization", error.get());
+    }
+  }
+}

dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdConstantsTest.java

@@ -0,0 +1,54 @@
+package datadog.trace.api;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+
+/** Behavior of the {@link DDTraceId#ZERO}/{@link DDTraceId#ONE} sibling constants. */
+class DDTraceIdConstantsTest {
+
+  @Test
+  void zeroAndOneFormatLikeTheEquivalentDD64bTraceId() {
+    assertEquals("0", DDTraceId.ZERO.toString());
+    assertEquals("00000000000000000000000000000000", DDTraceId.ZERO.toHexString());
+    assertEquals("0000000000000000", DDTraceId.ZERO.toHexStringPadded(16));
+    assertEquals("00000000000000000000000000000000", DDTraceId.ZERO.toHexStringPadded(32));
+    assertEquals(0L, DDTraceId.ZERO.toLong());
+    assertEquals(0L, DDTraceId.ZERO.toHighOrderLong());
+
+    assertEquals("1", DDTraceId.ONE.toString());
+    assertEquals("00000000000000000000000000000001", DDTraceId.ONE.toHexString());
+    assertEquals("0000000000000001", DDTraceId.ONE.toHexStringPadded(16));
+    assertEquals("00000000000000000000000000000001", DDTraceId.ONE.toHexStringPadded(32));
+    assertEquals(1L, DDTraceId.ONE.toLong());
+    assertEquals(0L, DDTraceId.ONE.toHighOrderLong());
+  }
+
+  @Test
+  void isValidReflectsTheValue() {
+    assertFalse(DDTraceId.ZERO.isValid());
+    assertFalse(DDTraceId.from(0).isValid());
+    assertFalse(DDTraceId.from("0").isValid());
+    assertFalse(DDTraceId.fromHex("0").isValid());
+    assertFalse(DD64bTraceId.from(0).isValid());
+
+    assertTrue(DDTraceId.ONE.isValid());
+    assertTrue(DDTraceId.from(1).isValid());
+    assertTrue(DD64bTraceId.from(42).isValid());
+  }
+
+  @Test
+  void constantsAreValueEqualToTheEquivalentDD64bTraceId() {
+    // ZERO/ONE used to be DD64bTraceId instances; they are now a sibling type but must stay
+    // value-equal (both directions, with matching hashCode) to the equivalent DD64bTraceId.
+    assertEquals(DDTraceId.ZERO, DD64bTraceId.from(0));
+    assertEquals(DD64bTraceId.from(0), DDTraceId.ZERO);
+    assertEquals(DDTraceId.ZERO.hashCode(), DD64bTraceId.from(0).hashCode());
+
+    assertEquals(DDTraceId.ONE, DD64bTraceId.from(1));
+    assertEquals(DD64bTraceId.from(1), DDTraceId.ONE);
+    assertEquals(DDTraceId.ONE.hashCode(), DD64bTraceId.from(1).hashCode());
+  }
+}

dd-trace-api/src/test/java/datadog/trace/api/DDTraceIdTest.java

@@ -1,6 +1,7 @@
 package datadog.trace.api;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -175,10 +176,13 @@ void failParsingIllegal128BitIdHexadecimalStringRepresentationFromPartialString(
   @Test
   void checkZeroConstantInitialization() {
     DDTraceId zero = DDTraceId.ZERO;
-    DDTraceId fromZero = DDTraceId.from(0);
 
     assertNotNull(zero);
-    assertSame(fromZero, zero);
+    assertFalse(zero.isValid());
+    // The public DDTraceId factories normalize a zero id back to the ZERO constant.
+    assertSame(zero, DDTraceId.from(0));
+    assertSame(zero, DDTraceId.from("0"));
+    assertSame(zero, DDTraceId.fromHex("0"));
   }
 
   private static String leftPadWithZeros(String value, int size) {