Don't transfer closed spans to new async contexts

This adds another level of protecting to using closed spans in any
context when returned by the ScopeManager.

It's really tricky to test this in isolation, because fetching the
active span, already performs the check. I can't check the private maps
of spans in the ScopeManager itself in the test. I've tested it by
temporarily removing the `&& span.open` check from
`ScopeManager.active()` which confirmed it works.

Author: tombruijn

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)
       }
     }