Review matter.js for logic errors

ANALYSIS-FIXES-APPLIED.md

@@ -0,0 +1,331 @@
+# Code Analysis - Fixes Applied
+
+This document summarizes the critical fixes applied to the matter.js codebase to address async race conditions and logic errors.
+
+**Branch**: `claude/analyze-matterjs-logic-016rMAtuNNyDrjr189FaSaYh`
+**Date**: 2025-11-15
+**Total Fixes**: 4 commits
+
+---
+
+## Summary
+
+| Issue | Severity | File | Status |
+|-------|----------|------|--------|
+| 1.1 InteractionClient Provider Race | CRITICAL | `packages/protocol/src/interaction/InteractionClient.ts` | ✅ Fixed |
+| 1.2 FabricManager Persistence | CRITICAL | `packages/protocol/src/fabric/FabricManager.ts` | ✅ Fixed |
+| 1.3 NodeSession Destroy Race | CRITICAL | `packages/protocol/src/session/NodeSession.ts` | ✅ Fixed |
+| 5.1 BTP Version Codec Bug | CRITICAL | `packages/protocol/src/codec/BtpCodec.ts` | ✅ Fixed |
+
+---
+
+## Fix 1.1: InteractionClient Provider Race Condition
+
+**Commit**: `82d0dee`
+**Severity**: CRITICAL - Async Race Condition
+**Impact**: Memory leak, duplicate client instances
+
+### Problem
+
+The `getInteractionClient()` method had a check-then-create pattern with two await points between the initial check and client creation:
+
+```typescript
+async getInteractionClient(address: PeerAddress, options = {}) {
+    let client = this.#clients.get(address);
+    if (client !== undefined) return client;  // ← Check
+
+    await nodeStore?.construction;             // ← AWAIT 1: yields to event loop
+    const exchangeProvider =
+        await this.#peers.exchangeProviderFor(address, options); // ← AWAIT 2
+
+    client = new InteractionClient(...);
+    this.#clients.set(address, client);        // ← Set (could overwrite!)
+    return client;
+}
+```
+
+### Race Scenario
+
+1. Call A checks map → not found
+2. Call B checks map → not found
+3. Both await and yield to event loop
+4. Call A creates InteractionClient, stores in map
+5. Call B creates DIFFERENT InteractionClient, overwrites A's client
+6. **Result**: Resource leak (A's client unreferenced), multiple instances for same peer
+
+### Solution
+
+Implemented double-checked locking pattern - check again after async operations:
+
+```typescript
+async getInteractionClient(address: PeerAddress, options = {}) {
+    let client = this.#clients.get(address);
+    if (client !== undefined) return client;
+
+    await nodeStore?.construction;
+    const exchangeProvider = await this.#peers.exchangeProviderFor(address, options);
+
+    // ✅ Double-check after await points
+    const existingClient = this.#clients.get(address);
+    if (existingClient !== undefined) {
+        return existingClient;
+    }
+
+    client = new InteractionClient(...);
+    this.#clients.set(address, client);
+    return client;
+}
+```
+
+**File**: `packages/protocol/src/interaction/InteractionClient.ts:140-168`
+
+---
+
+## Fix 1.2: FabricManager Persistence Desynchronization
+
+**Commit**: `7196550`
+**Severity**: CRITICAL - Async State Inconsistency
+**Impact**: Storage corruption, fabric index conflicts
+
+### Problem
+
+The `persistFabrics()` method captured fabric list and nextFabricIndex at different times with async gap between them:
+
+```typescript
+persistFabrics(): MaybePromise<void> {
+    const storeResult = this.#storage.set(
+        "fabrics",
+        Array.from(this.#fabrics.values()).map(fabric => fabric.config), // ← Snapshot 1
+    );
+    if (MaybePromise.is(storeResult)) {
+        return storeResult.then(() =>
+            this.#storage!.set("nextFabricIndex", this.#nextFabricIndex) // ← Snapshot 2
+        );
+    }
+}
+```
+
+### Race Scenario
+
+1. Method captures fabric list: `[fabric1, fabric2, fabric3]`
+2. Starts persisting (async operation)
+3. During promise chain, another operation adds fabric4
+4. `#nextFabricIndex` increments to 5
+5. Second storage operation persists nextFabricIndex=5
+6. **Result**: Storage has 3 fabrics but nextFabricIndex=5 (inconsistent!)
+
+On restart, index allocation would conflict or skip indices.
+
+### Solution
+
+Capture both values synchronously before any async operations:
+
+```typescript
+persistFabrics(): MaybePromise<void> {
+    // ✅ Capture both values atomically before async operations
+    const fabricConfigs = Array.from(this.#fabrics.values()).map(fabric => fabric.config);
+    const nextFabricIndex = this.#nextFabricIndex;
+
+    const storeResult = this.#storage.set("fabrics", fabricConfigs);
+    if (MaybePromise.is(storeResult)) {
+        return storeResult.then(() => this.#storage!.set("nextFabricIndex", nextFabricIndex));
+    }
+    return this.#storage.set("nextFabricIndex", nextFabricIndex);
+}
+```
+
+This ensures a consistent snapshot even if state changes during async persistence.
+
+**File**: `packages/protocol/src/fabric/FabricManager.ts:141-159`
+
+---
+
+## Fix 1.3: NodeSession Destroy During Event Processing
+
+**Commit**: `03676bd`
+**Severity**: CRITICAL - Async State Corruption
+**Impact**: Memory leak, subscription cleanup failure
+
+### Problem
+
+The `destroy()` method set the `#isClosing` flag AFTER the first await, allowing new operations during teardown:
+
+```typescript
+async destroy(sendClose = false, closeAfterExchangeFinished = true, flushSubscriptions = false) {
+    await this.clearSubscriptions(flushSubscriptions); // ← AWAIT 1: event loop yields
+    this.#fabric?.removeSession(this);
+
+    if (closeAfterExchangeFinished) {
+        this.#closingAfterExchangeFinished = true;
+    } else {
+        this.#isClosing = true;  // ← Only set AFTER first await!
+        // ... more awaits ...
+    }
+}
+```
+
+### Race Scenario
+
+1. `destroy()` called, starts clearing subscriptions
+2. During await, incoming message creates new subscription
+3. `clearSubscriptions()` completes (doesn't clear the new one)
+4. `#isClosing` set to true
+5. `destroyed.emit()` fires
+6. **Result**: New subscription never cleaned up → memory leak
+
+### Solution
+
+Set `#isClosing` flag immediately before any awaits:
+
+```typescript
+async destroy(sendClose = false, closeAfterExchangeFinished = true, flushSubscriptions = false) {
+    // ✅ Set closing flag BEFORE any awaits
+    if (!closeAfterExchangeFinished) {
+        this.#isClosing = true;
+    }
+
+    await this.clearSubscriptions(flushSubscriptions);
+    // ... rest of cleanup ...
+}
+```
+
+Code checking `isClosing` (SessionManager.ts:327, InteractionServer.ts:178) now correctly rejects new operations during teardown.
+
+**File**: `packages/protocol/src/session/NodeSession.ts:340-372`
+
+---
+
+## Fix 5.1: BTP Version Codec Index Swapping
+
+**Commit**: `1dcc6b7`
+**Severity**: CRITICAL - Logic Error
+**Impact**: BLE version negotiation completely broken
+
+### Problem
+
+The BTP codec had mismatched array indices between encoding and decoding:
+
+**Encoding**:
+```typescript
+writer.writeUInt8((versions[1] << 4) | versions[0]);
+// versions[1] → HIGH nibble (bits 7-4)
+// versions[0] → LOW nibble (bits 3-0)
+```
+
+**Decoding (WRONG)**:
+```typescript
+ver[0] = (version & 0xf0) >> 4;  // HIGH nibble → ver[0] ❌
+ver[1] = version & 0x0f;         // LOW nibble → ver[1] ❌
+```
+
+### Round-Trip Corruption
+
+```
+Input:  versions = [4, 3, 2, 1, 0, 0, 0, 0]
+Encode: byte 0 = (3 << 4) | 4 = 0x34
+Decode: ver[0] = 3, ver[1] = 4  → SWAPPED!
+Result: versions = [3, 4, 2, 1, 0, 0, 0, 0]  → WRONG
+```
+
+This would cause BLE commissioning to fail as device and controller disagree on protocol versions.
+
+### Solution
+
+Match decoding indices to encoding pattern:
+
+```typescript
+ver[1] = (version & 0xf0) >> 4;  // ✅ HIGH nibble → ver[1]
+ver[0] = version & 0x0f;         // ✅ LOW nibble → ver[0]
+```
+
+**Verification**:
+```
+Input:  versions = [4, 3, 2, 1, 0, 0, 0, 0]
+Encode: byte 0 = (3 << 4) | 4 = 0x34
+Decode: ver[1] = 3, ver[0] = 4
+Result: versions = [4, 3, 2, 1, 0, 0, 0, 0]  → CORRECT ✅
+```
+
+**File**: `packages/protocol/src/codec/BtpCodec.ts:187-201`
+
+---
+
+## Testing Recommendations
+
+### For InteractionClient Fix (1.1)
+```typescript
+// Test concurrent client creation for same peer
+const address = PeerAddress({ ... });
+const [client1, client2] = await Promise.all([
+    provider.getInteractionClient(address),
+    provider.getInteractionClient(address),
+]);
+assert(client1 === client2); // Should be same instance
+```
+
+### For FabricManager Fix (1.2)
+```typescript
+// Test persistence during concurrent fabric operations
+async function testPersistence() {
+    const persist1 = fabricManager.persistFabrics();
+    fabricManager.addFabric(newFabric); // Happens during persist
+    await persist1;
+
+    const restored = await FabricManager.load(storage);
+    // Verify consistency between fabric count and nextFabricIndex
+}
+```
+
+### For NodeSession Fix (1.3)
+```typescript
+// Test that operations are rejected during destroy
+session.destroy();
+try {
+    session.addSubscription(...); // Should fail or be ignored
+} catch (error) {
+    assert(error.message.includes('closing'));
+}
+```
+
+### For BTP Codec Fix (5.1)
+```typescript
+// Test round-trip encoding/decoding
+const versions = [4, 3, 2, 1, 0, 0, 0, 0];
+const encoded = BtpCodec.encodeBtpHandshakeRequest({ versions, ... });
+const decoded = BtpCodec.decodeBtpHandshakeRequest(encoded);
+assert.deepEqual(decoded.versions, versions); // Should match exactly
+```
+
+---
+
+## Related Documentation
+
+- `ANALYSIS-RESOURCE-LEAKS.md` - Remaining resource management issues for future work
+- `ANALYSIS-INVESTIGATED-NON-ISSUES.md` - Items that were investigated but are not bugs
+
+---
+
+## Impact Assessment
+
+### Before Fixes
+- ❌ InteractionClient: Potential memory leaks in multi-peer scenarios
+- ❌ FabricManager: Storage corruption possible after fabric operations
+- ❌ NodeSession: Subscription leaks during session teardown
+- ❌ BTP Codec: BLE commissioning broken (complete failure)
+
+### After Fixes
+- ✅ InteractionClient: Single instance per peer guaranteed
+- ✅ FabricManager: Atomic state snapshots prevent corruption
+- ✅ NodeSession: Operations blocked during teardown
+- ✅ BTP Codec: BLE version negotiation works correctly
+
+---
+
+## Conclusion
+
+All critical async race conditions and the BTP codec logic error have been fixed. The fixes follow JavaScript best practices for handling async operations in a single-threaded event loop environment.
+
+**Next Steps**:
+1. Review resource leak issues in `ANALYSIS-RESOURCE-LEAKS.md`
+2. Add integration tests for async scenarios
+3. Review message counter rollover logic against Matter spec (issue 5.2)