fix(node): make endpoint.state[id] and ClientNode.toString teardown-safe (#3680)

Apollon77 · claude · web-flow · commit 9577473c6129 · 2026-05-03T14:37:44.000+02:00
* fix(node): cache resolved peer address to make ClientNode toString teardown-safe ClientNode.toString() — invoked from Behaviors and Endpoint error message construction — could throw [destroyed-dependency] during ServerNode shutdown: the storage fallback in peerAddress went through ServerNodeStore.clientStores, which asserts active construction and is already in Closing state by the time the WAL final snapshot runs. The throw then masked whatever original error triggered toString(). peerAddress now mirrors the commissioning behavior state into a cache field whenever the backing is loaded (set OR cleared, so a decommission does not leave a stale value), falls back to the cache when the backing is gone, and accepts DependencyLifecycleError around the storage probe so teardown reads return undefined rather than throwing. eraseWithMutex clears the cache so re-commissioning can publish a new address. Adds a regression test that closes the controller and verifies peerAddress and toString continue to resolve. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(node): make endpoint.state[behaviorId] return undefined during teardown Behaviors.close() destroys backings but leaves the augmented endpoint.state[id] property descriptors in place because reuse paths (factory reset) re-activate through the same getter without re-running the constructor. Post-close access therefore re-entered #backingFor on a missing backing and threw BehaviorInitializationError; constructing that error involved ${this.#endpoint} which cascaded through toString → identity → peerAddress → storage and surfaced as a misleading "Snapshot failed" warning during ServerNode shutdown. The getter now checks the endpoint construction status: in Destroying or Destroyed it returns undefined, leaving Active/Initializing paths (including late activation) unchanged. Adds a regression test verifying state access on a closed peer returns undefined rather than throwing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(node): clear cached peer address on soft reset too Copilot review on #3680 noted that ClientNode.reset() (soft reset, without storage erase) closes behaviors but leaves #cachedPeerAddress in place. If the node is then re-commissioned to a different fabric, peerAddress would return the stale cached value until the new commissioning state loads. Add a resetWithMutex override that clears the cache. The explicit clear in eraseWithMutex stays because that path uses super.resetWithMutex() (static dispatch), which skips this override. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
diff --git a/packages/node/src/endpoint/properties/Behaviors.ts b/packages/node/src/endpoint/properties/Behaviors.ts
@@ -821,7 +821,17 @@ export class Behaviors {
     #augmentEndpoint(type: Behavior.Type) {
         const { id, Events } = type;
 
-        const get = () => this.#backingFor(type).stateView;
+        // Descriptors persist across the lifetime of the Behaviors instance (constructor + inject install them and
+        // close does not remove them so reuse paths like factory-reset can re-activate the same getter).  Returning
+        // undefined for endpoints that are tearing down keeps post-close access from re-entering #backingFor on a
+        // missing backing and surfacing a misleading BehaviorInitializationError.
+        const get = () => {
+            const status = this.#endpoint.construction.status;
+            if (status === Lifecycle.Status.Destroying || status === Lifecycle.Status.Destroyed) {
+                return undefined;
+            }
+            return this.#backingFor(type).stateView;
+        };
         Object.defineProperty(this.#endpoint.state, id, { get, enumerable: true, configurable: true });
         if (type.schema.id !== undefined) {
             Object.defineProperty(this.#endpoint.state, type.schema.id, { get, configurable: true });
diff --git a/packages/node/src/node/ClientNode.ts b/packages/node/src/node/ClientNode.ts
@@ -20,6 +20,7 @@ import { ClientNodeStore } from "#storage/client/ClientNodeStore.js";
 import { RemoteWriter } from "#storage/client/RemoteWriter.js";
 import { ServerNodeStore } from "#storage/server/ServerNodeStore.js";
 import {
+    DependencyLifecycleError,
     Diagnostic,
     Identity,
     ImplementationError,
@@ -47,6 +48,7 @@ export class ClientNode extends Node<ClientNode.RootEndpoint> {
     #matter: MatterModel;
     #interaction?: ClientNodeInteraction;
     #blockInteractions = false;
+    #cachedPeerAddress?: PeerAddress;
 
     constructor(options: ClientNode.Options) {
         const opts = {
@@ -206,16 +208,17 @@ export class ClientNode extends Node<ClientNode.RootEndpoint> {
     }
 
     protected async eraseWithMutex() {
-        // First, ensure we're offline
         await this.cancelWithMutex();
-
-        // Then reset
         await super.resetWithMutex();
-
-        // and erase
+        this.#cachedPeerAddress = undefined;
         await this.env.get(EndpointInitializer).eraseDescendant(this);
     }
 
+    protected override async resetWithMutex() {
+        this.#cachedPeerAddress = undefined;
+        await super.resetWithMutex();
+    }
+
     protected createRuntime(): NetworkRuntime {
         return new ClientNetworkRuntime(this);
     }
@@ -274,15 +277,32 @@ export class ClientNode extends Node<ClientNode.RootEndpoint> {
     }
 
     get peerAddress(): PeerAddress | undefined {
-        // If commissioned, use the peer address for logging purposes
-        let address = PeerAddress(this.behaviors.maybeStateOf("commissioning")?.peerAddress as PeerAddress | undefined);
+        // Whenever the commissioning backing is loaded its state is authoritative - mirror it (set or cleared)
+        // so the cache cannot survive a decommission as a stale value
+        const state = this.behaviors.maybeStateOf("commissioning");
+        if (state !== undefined) {
+            const live = PeerAddress(state.peerAddress as PeerAddress | undefined);
+            this.#cachedPeerAddress = live;
+            return live;
+        }
+
+        if (this.#cachedPeerAddress !== undefined) {
+            return this.#cachedPeerAddress;
+        }
 
-        // During early initialization commissioning state may not be loaded, so check directly in storage too
-        if (!address) {
-            address = PeerAddress(this.store.storeForEndpoint(this).peerAddress as PeerAddress | undefined);
+        // Backing not yet loaded; consult storage directly, tolerating teardown
+        try {
+            const stored = PeerAddress(this.store.storeForEndpoint(this).peerAddress as PeerAddress | undefined);
+            if (stored !== undefined) {
+                this.#cachedPeerAddress = stored;
+                return stored;
+            }
+        } catch (error) {
+            DependencyLifecycleError.accept(error);
+            logger.info("Cannot resolve peer address from storage:", error);
         }
 
-        return address;
+        return undefined;
     }
 
     override get identity() {
diff --git a/packages/node/test/node/ClientNodeTest.ts b/packages/node/test/node/ClientNodeTest.ts
@@ -1362,6 +1362,45 @@ describe("ClientNode", () => {
             expect(address2.nodeId).not.equals(fabric.nodeId);
         });
     });
+
+    describe("peerAddress resilience", () => {
+        // Closing a ServerNode transitions ServerNodeStore.construction to "Closing" before the destructor releases
+        // child stores.  Pre-fix, peerAddress fell back to the storage layer via this.store, which would assert
+        // through to clientStores and throw [destroyed-dependency].  toString() (used in error message construction
+        // throughout Behaviors and Endpoint) then re-threw, masking the original cause.
+        it("peerAddress survives ServerNode shutdown via cache", async () => {
+            await using site = new MockSite();
+            const { controller } = await site.addCommissionedPair();
+            const peer1 = controller.peers.get("peer1")!;
+            expect(peer1).not.undefined;
+
+            const cachedAddress = peer1.peerAddress;
+            expect(cachedAddress).not.undefined;
+
+            await MockTime.resolve(controller.close(), { macrotasks: true });
+
+            expect(() => peer1.peerAddress).not.throws();
+            expect(peer1.peerAddress).deep.equals(cachedAddress);
+            expect(() => peer1.toString()).not.throws();
+        });
+
+        // Behaviors.close() leaves the augmented endpoint.state[id] descriptors in place so that paths reusing the
+        // Behaviors instance (e.g. factory reset) can re-activate.  Post-close access must therefore not re-enter
+        // #backingFor and throw BehaviorInitializationError - the getter checks the construction status and returns
+        // undefined once the endpoint is tearing down.
+        it("endpoint.state[behaviorId] returns undefined after close", async () => {
+            await using site = new MockSite();
+            const { controller } = await site.addCommissionedPair();
+            const peer1 = controller.peers.get("peer1")!;
+
+            expect(peer1.state.commissioning).not.undefined;
+
+            await MockTime.resolve(controller.close(), { macrotasks: true });
+
+            expect(() => peer1.state.commissioning).not.throws();
+            expect(peer1.state.commissioning).to.be.undefined;
+        });
+    });
 });
 
 const GLOBAL_ATTRS = [0xfff8, 0xfff9, 0xfffb, 0xfffc, 0xfffd];
