fix(bridge-node): adopt a factory reset's own renumbering instead of alarming forever (#140)

bridge-node/src/endpoint-map.ts

@@ -25,6 +25,32 @@
  * baseline stays, so the drift keeps being reported until a human resolves it
  * with §3.11.
  *
+ * **One narrow, deliberate exception (issue #140).** An entry the node itself
+ * marked {@link EndpointRecord.numberVoid} is not a drifted entry at all —
+ * {@link EndpointMapStore.voidNumbers} is only ever called at the two points
+ * where matter.js has just erased its OWN number allocation along with every
+ * fabric (§3.10's `factory_reset preserveEndpointNumbers: true`, and the
+ * last-fabric self-reset). matter.js is always the allocator; this map is only
+ * ever its witness (see below), and with no fabric left there is no paired
+ * ecosystem that could still be holding the old numbers to disagree with —
+ * adoption there is unobservable outside this node. {@link
+ * EndpointMapStore.check} adopts a void entry's live number silently instead
+ * of reporting it. An entry that was never voided still drifts exactly as
+ * documented above; only the reset that erased matter.js's own allocation
+ * earns the adoption.
+ *
+ * The marker's lifetime is bounded by when its `UniqueID` next comes back
+ * LIVE, not by the moment of the reset: only entries `check()` actually sees
+ * get to adopt, so a device that stays offline through the first post-reset
+ * reconcile simply keeps its VOID marker — still unadopted — until it next
+ * exports, however much later that is. That is still safe even if fabrics
+ * have been re-paired by then: the number it eventually adopts is the one
+ * matter.js handed out fresh after THIS reset, not a stale one, and no
+ * ecosystem ever saw the old number in between (it was never live to be
+ * observed). The safety argument above just has to be read as "no fabric
+ * survived the reset that erased the allocation", not "no fabric exists at
+ * the moment of adoption".
+ *
  * **It also carries what it takes to REBUILD an endpoint** (issue #141). Numbers
  * alone were enough while the file was only a witness, but a node that comes
  * online with an empty aggregator and waits for the plugin tells every paired
@@ -113,6 +139,15 @@ export const ENDPOINT_MAP_FILE = "endpoint-map.json";
  * endpoint set can be rebuilt without the plugin. Version 1 files are still
  * read — see {@link ENDPOINT_MAP_VERSION_LEGACY} — because a v1 file holds the
  * one thing that can never be re-derived: the numbers paired ecosystems know.
+ *
+ * **Still 2 as of issue #140's `numberVoid` marker — not bumped to 3.** A
+ * schema bump exists to protect an OLD build from a NEW file it cannot parse;
+ * `numberVoid` needs no such protection, because every reader here (old and
+ * new) reaches a field by name (`candidate.number`/`.role`/`.label`) rather
+ * than by validating the object shape as a whole, so an old build reading a
+ * file with an unknown extra key simply never looks at it — no throw, no
+ * `readRecord` rejection, the entry loads exactly as it would without the
+ * marker. There is nothing here for a version bump to guard.
  */
 export const ENDPOINT_MAP_VERSION = 2;
 
@@ -137,6 +172,19 @@ export interface EndpointRecord {
     number: number;
     role?: string;
     label?: string;
+    /**
+     * Set by {@link EndpointMapStore.voidNumbers} (issue #140): `number` is a
+     * pre-reset value that {@link EndpointMapStore.check} must silently ADOPT
+     * from the next live number it sees for this `UniqueID`, rather than
+     * report as drift. Absent (not `false`) on every ordinary entry, so a plain
+     * `JSON.stringify` of a non-void record is unchanged from before #140 —
+     * `readRecord` only ever sets this to `true` and never anything else. An
+     * old build reading a file that has it simply never looks at the key: it
+     * destructures `number`/`role`/`label` off the parsed object by name, so
+     * an unrecognised extra property is inert, not a crash — see the schema
+     * version note below, which is why this stays version 2.
+     */
+    numberVoid?: true;
 }
 
 /** The on-disk shape (v2). */
@@ -217,6 +265,11 @@ function readRecord(value: unknown): EndpointRecord | undefined {
     if (isNonEmptyString(candidate.label)) {
         record.label = candidate.label;
     }
+    // Tolerant, like role/label: anything other than a literal `true` is
+    // simply not a void marker, rather than a reason to reject the entry.
+    if (candidate.numberVoid === true) {
+        record.numberVoid = true;
+    }
     return record;
 }
 
@@ -333,14 +386,17 @@ function noteRestorable(record: EndpointRecord, entry: LiveEndpointNumber): bool
 }
 
 /** Why {@link EndpointMapStore.check} is writing, for the log line. */
-function persistReason(added: number, refreshed: number): string {
+function persistReason(added: number, refreshed: number, adopted: number): string {
     const parts: string[] = [];
     if (added > 0) {
         parts.push(`recorded ${added} new endpoint number(s)`);
     }
     if (refreshed > 0) {
         parts.push(`refreshed ${refreshed} endpoint role/label(s)`);
     }
+    if (adopted > 0) {
+        parts.push(`adopted ${adopted} renumbered endpoint number(s)`);
+    }
     return parts.length === 0 ? "retried after a failed write" : parts.join(" and ");
 }
 
@@ -464,6 +520,14 @@ export class EndpointMapStore {
      * order the numbers were first recorded — irrelevant to matter.js, which
      * keys each endpoint's number on its `Endpoint.id`, but stable, which makes
      * the log read the same way twice.
+     *
+     * **`numberVoid` is deliberately not part of this filter (issue #140).** A
+     * voided number is still the best guess for `createEndpoint`'s
+     * `Endpoint.id`-keyed restore — matter.js hands back whatever it currently
+     * has for that id regardless of what this map believes, and the {@link
+     * check} that follows `server.start()` is what reconciles the map to
+     * whatever number came back. Excluding void entries here would only widen
+     * the online-and-empty window issue #141 already closed, for no benefit.
      */
     restorable(): RestorableEndpoint[] {
         const restorable: RestorableEndpoint[] = [];
@@ -546,11 +610,19 @@ export class EndpointMapStore {
      * retains the allocation so re-adding the same device restores the same
      * number, and forgetting it here would throw away the very baseline that
      * makes a re-add verifiable.
+     *
+     * **A `numberVoid` entry is adopted, not drift-checked (issue #140).** See
+     * the class comment for why that is safe. The live number is taken as the
+     * new baseline and the marker is cleared — even when the live number
+     * happens to already equal the stale one, because the marker itself is
+     * what has to stop being true, not just the mismatch it was guarding
+     * against.
      */
     check(live: readonly LiveEndpointNumber[]): DriftEntry[] {
         const drift: DriftEntry[] = [];
         let added = 0;
         let refreshed = 0;
+        let adopted = 0;
         for (const entry of live) {
             const record = this.#endpoints.get(entry.uniqueId);
             if (record === undefined) {
@@ -565,17 +637,30 @@ export class EndpointMapStore {
             if (noteRestorable(record, entry)) {
                 refreshed += 1;
             }
+            if (record.numberVoid) {
+                record.number = entry.endpointNumber;
+                delete record.numberVoid;
+                adopted += 1;
+                continue;
+            }
             if (record.number !== entry.endpointNumber) {
                 drift.push({ uniqueId: entry.uniqueId, expected: record.number, actual: entry.endpointNumber });
             }
         }
+        if (adopted > 0) {
+            this.log(
+                `Adopted ${adopted} endpoint number(s) renumbered by the factory reset; matter.js's ` +
+                    "allocation was erased with the fabrics, so no paired ecosystem holds the old " +
+                    "numbers — this is the reset's own renumbering, not drift.",
+            );
+        }
         // `#dirty` is the retry: a write that failed last time leaves the map
         // owing the disk something even when this pass added nothing, and the
         // steady state (`added === 0`, clean) still costs no I/O. Without it a
         // full disk at the moment of the first check would never be revisited,
         // and `checked` — see the `#dirty` field — would never come true again.
-        if (added > 0 || refreshed > 0 || this.#dirty) {
-            this.persist(persistReason(added, refreshed));
+        if (added > 0 || refreshed > 0 || adopted > 0 || this.#dirty) {
+            this.persist(persistReason(added, refreshed, adopted));
         }
         // An empty comparison compared nothing, so it cannot make `driftChecked`
         // true — the same rule {@link seed} already applies. §4.3 says
@@ -590,6 +675,52 @@ export class EndpointMapStore {
         return drift;
     }
 
+    /**
+     * §3.10's preserving reset and the last-fabric self-reset (issue #140) —
+     * mark every entry's number as no longer trustworthy, WITHOUT discarding
+     * it, so the next {@link check} adopts whatever number each `UniqueID`
+     * comes back with instead of reporting it as drift forever.
+     *
+     * **Why this is safe — see the class comment for the full argument.** In
+     * one line: matter.js is always the allocator and this map only ever
+     * witnesses it, and both call sites reach this with an empty fabric set,
+     * so nothing paired can still be holding the numbers this call is about
+     * to let move.
+     *
+     * Unlike {@link rebuild}, this keeps every entry (role/label survive
+     * untouched — a void number does not make an endpoint any less
+     * restorable, see {@link restorable}) and unlike {@link discard}, it
+     * throws nothing away: a caller who wants "adopt whatever exists right
+     * now" already has {@link rebuild}, and this is deliberately weaker than
+     * that — it only stops treating the CURRENT numbers as ground truth.
+     *
+     * A no-op on an empty map: nothing to mark is not worth a write, and a
+     * bridge that has never exported anything must not have this manufacture
+     * a baseline out of nothing.
+     *
+     * Returns whether the voided baseline reached disk — `false` on the
+     * empty-map no-op as well as on a failed write, since neither actually
+     * persisted anything new. A caller that has to tell those two apart
+     * (a no-op is not a failure worth warning about) should check {@link size}
+     * before calling, the way both `node.ts` call sites now do.
+     *
+     * Also drops {@link checked}: a baseline the node itself just declared
+     * VOID has not been verified against anything, and leaving the flag set
+     * would have §4.3's `driftChecked: true` claim "checked, nothing moved"
+     * over numbers that are, by construction, not yet trusted. The first
+     * post-reset {@link check} against live entries earns it back.
+     */
+    voidNumbers(why: string): boolean {
+        if (this.#endpoints.size === 0) {
+            return false;
+        }
+        for (const record of this.#endpoints.values()) {
+            record.numberVoid = true;
+        }
+        this.#checked = false;
+        return this.persist(`voided every endpoint number — ${why}`);
+    }
+
     /**
      * Adopt a baseline for a bridge that is already commissioned but has no map
      * — the E5 upgrade path (PRD §7).

bridge-node/src/node.ts

@@ -639,6 +639,15 @@ export class BridgeNode implements BridgeFacade {
      * Deliberately here rather than in {@link removeFabric}: §3.9 is only one of
      * the ways to get here. An ecosystem that removes *us* from its side does it
      * too, and that route never touches our command handler at all.
+     *
+     * **The endpoint map's numbers are voided here too (issue #140), for the
+     * same reason as §3.10's preserving reset.** matter.js has just erased
+     * itself, so its own number allocation is gone with the fabrics — the map's
+     * numbers are about to disagree with whatever gets re-created, and with no
+     * fabric left there is no paired ecosystem that could still be holding the
+     * old numbers to disagree with. {@link EndpointMapStore.voidNumbers} is a
+     * no-op on an empty map, so this costs nothing on a bridge that had never
+     * exported anything.
      */
     private noteLastFabricGone(): void {
         if (this.identity.commissionedAt === undefined) {
@@ -653,6 +662,26 @@ export class BridgeNode implements BridgeFacade {
             WARN_IDENTITY_WRITE,
             clearCommissioned(this.config.storagePath, this.identity, message => this.log(message)),
         );
+        // `size > 0` first: `voidNumbers` returns `false` for an empty map too
+        // (nothing to void is not a failure), and this bridge may never have
+        // exported anything.
+        if (
+            this.#endpointMap.size > 0 &&
+            !this.#endpointMap.voidNumbers(
+                "the last fabric left — matter.js factory-reset itself, wiping its own endpoint-number " +
+                    "allocation with it",
+            )
+        ) {
+            // Same risk as the factory-reset branch above: the VOID markers
+            // are RAM-only until a later persist succeeds (`#dirty` makes that
+            // retry automatic), and a restart before then brings back #140's
+            // forever-drift report for this unpairing.
+            this.log(
+                "The endpoint map's VOID markers could NOT be written to disk after the last fabric " +
+                    "left — they exist in memory only for now and will be retried on the next " +
+                    "successful persist; a restart before then will report the old §4.3 drift again.",
+            );
+        }
         this.#drift = [];
     }
 
@@ -1001,6 +1030,16 @@ export class BridgeNode implements BridgeFacade {
      *
      * The live endpoints are deliberately left alone. They are still the set the
      * plugin asked for; what changed is who is allowed to see them.
+     *
+     * **A preserved map has its numbers VOIDED, not drift-checked (issue #140).**
+     * `erase()` wipes matter.js's own allocation, so every endpoint re-created
+     * after this is renumbered from scratch — and the map disagreeing with that
+     * on every future attach is not a real anomaly, it is the reset's own,
+     * expected renumbering. `voidNumbers()` marks every entry so the next
+     * `check()` silently adopts whatever number each `UniqueID` comes back with
+     * instead of reporting it as drift forever; see its doc comment for why that
+     * is safe (the fabric set is empty here, so no paired ecosystem can still be
+     * holding the old numbers to disagree with).
      */
     async factoryReset(preserveEndpointNumbers: boolean): Promise<void> {
         this.#window.clear();
@@ -1070,20 +1109,46 @@ export class BridgeNode implements BridgeFacade {
         // What preservation actually bought, checked rather than claimed.
         // `erase()` wipes matter.js's OWN allocation, so a preserved map is a
         // baseline for numbers that are about to be handed out afresh — it
-        // preserves the ability to NOTICE, not the numbers (see §3.10). Running
-        // the detector here is what turns that into a statement in the log
-        // instead of a surprise on the next attach.
-        if (preserveEndpointNumbers) {
-            this.checkDrift();
-            this.log(
-                this.#drift.length === 0
-                    ? "Endpoint-number map preserved; no drift against the live endpoints yet — note " +
-                          "that matter.js's own allocation was wiped by the reset, so drift is expected " +
-                          "as endpoints are re-created and is what the preserved map exists to report"
-                    : `Endpoint-number map preserved and ${this.#drift.length} endpoint(s) already ` +
-                          "differ from it — that is the reset's own renumbering, reported exactly as " +
-                          "designed; rebuild the map (§3.11) to accept it",
-            );
+        // preserves the ability to NOTICE, not the numbers (see §3.10). Issue
+        // #140: the numbers this map already holds are about to disagree with
+        // matter.js on every single re-created endpoint, and that is not a real
+        // anomaly — it is the reset's own renumbering, guaranteed by the fact
+        // that `erase()` just ran. Voiding them means the next `check()` (the
+        // first reconcile after re-pairing) adopts the fresh numbers silently
+        // instead of reporting the same "drift" on every attach forever, which
+        // is #140's complaint: the plugin refuses the §3.11 rebuild the old log
+        // line pointed at, because the node is otherwise perfectly healthy.
+        if (preserveEndpointNumbers && this.#endpointMap.size > 0) {
+            // `size > 0` gates both the call and the log: `voidNumbers` returns
+            // `false` for an empty map too (nothing to void is not a failure),
+            // and a bridge that had nothing preserved must not be told its
+            // numbers are VOID, nor warned about a persist that never
+            // happened.
+            if (this.#endpointMap.voidNumbers(
+                "factory reset (preserveEndpointNumbers: true) wiped matter.js's own allocation",
+            )) {
+                this.log(
+                    "Endpoint-number map preserved but its numbers are now VOID: matter.js's own " +
+                        "allocation was wiped by the reset, so they will be silently adopted as endpoints " +
+                        "are re-created rather than reported as drift — no fabric survived the reset to " +
+                        "still be holding the old numbers, so nothing outside this node can observe the " +
+                        "difference. A §3.11 rebuild is not needed for this.",
+                );
+            } else {
+                // The void markers exist in memory only — this write failed the
+                // same way `discard()`'s can, and the risk is the mirror image:
+                // a crash before the next successful persist loses the markers,
+                // and the next start reports #140's forever-drift again as if
+                // this reset had never happened. `persist()` already leaves
+                // `#dirty` set on this failure, so the retry is automatic: the
+                // very next successful `check()` or write re-attempts it.
+                this.log(
+                    "Endpoint-number map preserved but its VOID markers could NOT be written to disk " +
+                        "— they exist in memory only for now. The next successful persist will retry " +
+                        "and write them; if the node restarts before then, the old §4.3 drift report " +
+                        "will return once, until this reset's renumbering is adopted again.",
+                );
+            }
         }
 
         this.log("Factory reset complete; advertising for commissioning again");