Merge pull request #675 from appsignal/scope-manager-check-not-closed

Don't transfer closed spans to new async contexts

packages/nodejs/.changesets/do-not-transfer-closed-spans-for-new-async-contexts.md

@@ -0,0 +1,6 @@
+---
+bump: "patch"
+type: "fix"
+---
+
+Do not transfer closed spans for new async contexts in the ScopeManager. Rather than relying on `ScopeManager.active()` and `ScopeManager.root()` to make sure the span is not already closed, also make sure it's not closed when transferring spans around between async contexts.

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

@@ -60,6 +60,20 @@ describe("ScopeManager", () => {
         asyncTaskWithContext(scopeManager, span, () => scopeManager.active())
       ).resolves.toBe(span)
     })
+
+    it("doesn't return active span is span is closed", async () => {
+      scopeManager = new ScopeManager()
+      const span = new RootSpan()
+
+      scopeManager.enable()
+
+      span.close()
+      await expect(
+        asyncTaskWithContext(scopeManager, span, () => {
+          return scopeManager.active()
+        })
+      ).resolves.toBeUndefined()
+    })
   })
 
   describe(".disable()", () => {

packages/nodejs/src/scope.ts

@@ -42,17 +42,28 @@ export class ScopeManager {
     this.#scopes = new Map()
 
     const init = (id: number, type: string, triggerId: number) => {
+      // Move the current span and root over to the scopes and roots map for
+      // the child executionAsyncId.
+      const transferSpanScope = (
+        list: Map<number, Span | undefined>,
+        newId: number,
+        oldId: number
+      ) => {
+        const span = list.get(oldId)
+        if (span && span.open) {
+          list.set(newId, list.get(oldId))
+        }
+      }
+
       /**
        * We use the `executionAsyncId` here, as using the `triggerId` causes context
        * confusion in applications using async/await.
        */
       if (type === "PROMISE") {
         const currentId = asyncHooks.executionAsyncId()
 
-        if (this.#scopes.get(currentId)) {
-          this.#scopes.set(id, this.#scopes.get(currentId))
-          this.#roots.set(id, this.#roots.get(currentId))
-        }
+        transferSpanScope(this.#scopes, id, currentId)
+        transferSpanScope(this.#roots, id, currentId)
       } else {
         /**
          * `triggerId` usually equal the ID of the AsyncResource in whose scope we are
@@ -62,10 +73,8 @@ export class ScopeManager {
          *
          * However, as the `triggerId` can be defined in userland, we choose to respect th
          */
-        if (this.#scopes.get(triggerId)) {
-          this.#scopes.set(id, this.#scopes.get(triggerId))
-          this.#roots.set(id, this.#roots.get(triggerId))
-        }
+        transferSpanScope(this.#scopes, id, triggerId)
+        transferSpanScope(this.#roots, id, triggerId)
       }
     }