Check if span is still open in ScopeManager.active

The ScopeManager.active and ScopeManager.root function returned spans as
active or as a valid root span when they were closed. This is a
different state from a NoopSpan where no span has been active.

When a span closes it doesn't necessary reset the scope (remove the
executionAsyncId) from the scopes and roots maps on the ScopeManager.

Keep track if a span is open or not by keeping an `open` attribute on
the Span that is set to `false` whenever `Span.close()` is called. We
should not rely on the `isObject() => { closed: true }`, because that's
a more intensive operation (creating and parsing a JSON object) and we
should only use it for testing.

With the new `Span.isOpen()` function we can check if the Span is stil
open and only return it if that's the case. If it's not, also
immediately remove it from the ScopeManager's maps.

I've updated the `createRootSpan` test to close the span after the
assertions, otherwise it will now fail because of returning undefined.

We ran into this issue occurring in PR #669. It will not necessarily fix
the issue we ran into there, but it's an issue nonetheless.

packages/nodejs/.changesets/fix-active-span-returning-a-closed-span.md

@@ -0,0 +1,6 @@
+---
+bump: "patch"
+type: "fix"
+---
+
+Fix the `ScopeManager.active()` function returning closed spans.

packages/nodejs/src/__tests__/scope.test.ts

@@ -174,6 +174,29 @@ describe("ScopeManager", () => {
     })
   })
 
+  describe(".active()", () => {
+    it("when active span is still open, it returns the active span", () => {
+      const root = new RootSpan({ namespace: "root" })
+      scopeManager.setRoot(root)
+      expect(root.isOpen()).toBeTruthy()
+
+      const span = scopeManager.active()
+      expect(span).toBeDefined()
+      expect(span?.traceId).toBeDefined()
+      expect(span?.toObject().namespace).toEqual("root")
+    })
+
+    it("when active span is closed, it returns undefined", () => {
+      const root = new RootSpan({ namespace: "root" })
+      scopeManager.setRoot(root)
+      root.close()
+      expect(root.isOpen()).toBeFalsy()
+
+      const span = scopeManager.active()
+      expect(span).toBeUndefined()
+    })
+  })
+
   // TODO: Add tests
   describe(".emitWithContext()", () => {})
 })

packages/nodejs/src/__tests__/span.test.ts

@@ -10,6 +10,7 @@ describe("RootSpan", () => {
     const internal = span.toObject()
 
     expect(span).toBeInstanceOf(RootSpan)
+    expect(span.isOpen()).toBeTruthy()
     expect(internal.closed).toBeFalsy()
     expect(typeof internal.start_time_seconds).toBe("number")
     expect(typeof internal.start_time_nanoseconds).toBe("number")
@@ -143,6 +144,7 @@ describe("RootSpan", () => {
     const span = new RootSpan().close()
     const internal = span.toObject()
 
+    expect(span.isOpen()).toBeFalsy()
     expect(internal.closed).toBeTruthy()
   })
 })

packages/nodejs/src/__tests__/tracer.test.ts

@@ -82,15 +82,15 @@ describe("Tracer", () => {
     it("creates a new span and assigns it as a root span", () => {
       const rootSpan1 = tracer.createRootSpan()
       expect(rootSpan1).toBeInstanceOf(RootSpan)
-      rootSpan1.close()
       expect(tracer.rootSpan()).toEqual(rootSpan1)
       expect(tracer.currentSpan()).toEqual(rootSpan1)
+      rootSpan1.close()
 
       const rootSpan2 = tracer.createRootSpan()
       expect(rootSpan2).toBeInstanceOf(RootSpan)
-      rootSpan2.close()
       expect(tracer.rootSpan()).toEqual(rootSpan2)
       expect(tracer.currentSpan()).toEqual(rootSpan2)
+      rootSpan2.close()
     })
   })
 

packages/nodejs/src/interfaces/span.ts

@@ -106,6 +106,13 @@ export interface Span {
    */
   close(endTime?: number): this
 
+  /**
+   * Returns if the span is still open or not.
+   *
+   * A closed span can't be modified or fetched attributes from.
+   */
+  isOpen(): boolean
+
   /**
    * Returns a SpanData object representing the internal Span in the extension.
    *

packages/nodejs/src/noops/span.ts

@@ -49,6 +49,10 @@ export class NoopSpan implements Span {
     return this
   }
 
+  public isOpen(): boolean {
+    return false
+  }
+
   public toObject(): SpanData {
     return {}
   }

packages/nodejs/src/scope.ts

@@ -108,7 +108,19 @@ export class ScopeManager {
    */
   public active(): Span | undefined {
     const uid = asyncHooks.executionAsyncId()
-    return this.#scopes.get(uid)
+    const span = this.#scopes.get(uid)
+    // Perform check if the span is not closed. A span that has been closed
+    // can't be considered an active span anymore.
+    if (span && span.isOpen()) {
+      // Span exists and is still open. These conditions make it a valid, still
+      // active, span.
+      return span
+    } else {
+      // Clear any reference to this span in the scopes manager to avoid
+      // confusion next time the active span is fetched.
+      this.#scopes.delete(uid)
+      this.#roots.delete(uid)
+    }
   }
 
   /**
@@ -125,7 +137,19 @@ export class ScopeManager {
    */
   public root(): Span | undefined {
     const uid = asyncHooks.executionAsyncId()
-    return this.#roots.get(uid)
+    const span = this.#roots.get(uid)
+    // Perform check if the span is not closed. A span that has been closed
+    // can't be considered a root span anymore.
+    if (span && span.isOpen()) {
+      // Span exists and is still open. These conditions make it a valid, still
+      // root, span.
+      return span
+    } else {
+      // Clear any reference to this span in the scopes manager to avoid
+      // confusion next time the root span is fetched.
+      this.#scopes.delete(uid)
+      this.#roots.delete(uid)
+    }
   }
 
   /**

packages/nodejs/src/span.ts

@@ -17,6 +17,11 @@ import { BaseClient } from "./client"
  */
 export class BaseSpan implements Span {
   protected _ref: unknown
+  private open: boolean
+
+  constructor() {
+    this.open = true
+  }
 
   /**
    * The current ID of the trace.
@@ -169,6 +174,7 @@ export class BaseSpan implements Span {
    * timestamp in milliseconds since the UNIX epoch.
    */
   public close(endTime?: number): this {
+    this.open = false
     if (endTime && typeof endTime === "number") {
       const { sec, nsec } = getAgentTimestamps(endTime)
       span.closeSpanWithTimestamp(this._ref, sec, nsec)
@@ -180,6 +186,15 @@ export class BaseSpan implements Span {
     }
   }
 
+  /**
+   * Returns if the span is still open or not.
+   *
+   * A closed span can't be modified or fetched attributes from.
+   */
+  public isOpen() {
+    return this.open
+  }
+
   /**
    * Returns a SpanData object representing the internal Span in the extension.
    *