diff --git a/bridge-node/src/endpoint-map.ts b/bridge-node/src/endpoint-map.ts index da2dde8..5a8a5d8 100644 --- a/bridge-node/src/endpoint-map.ts +++ b/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,7 +386,7 @@ 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)`); @@ -341,6 +394,9 @@ function persistReason(added: number, refreshed: number): string { 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). diff --git a/bridge-node/src/node.ts b/bridge-node/src/node.ts index ca9ead5..cd4a458 100644 --- a/bridge-node/src/node.ts +++ b/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 { 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"); diff --git a/bridge-node/test/endpoint-map.test.ts b/bridge-node/test/endpoint-map.test.ts index eee77ac..916c462 100644 --- a/bridge-node/test/endpoint-map.test.ts +++ b/bridge-node/test/endpoint-map.test.ts @@ -395,6 +395,259 @@ describe("EndpointMapStore.check — the drift detector (PRD §4.3)", () => { }); }); +describe("EndpointMapStore.voidNumbers — reset renumbering adoption (issue #140)", () => { + it("marks every entry and persists", () => { + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([ + { uniqueId: "indigo-1", endpointNumber: 2 }, + { uniqueId: "indigo-2", endpointNumber: 3 }, + ]); + + assert.equal(store.voidNumbers("test"), true); + + assert.deepEqual(mapFileIn(dir).endpoints, { + "indigo-1": { number: 2, numberVoid: true }, + "indigo-2": { number: 3, numberVoid: true }, + }); + }); + + it("is a no-op — no write — on an empty map", () => { + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + + assert.equal(store.voidNumbers("test"), false); + + assert.equal(loadEndpointMap(dir).present, false, "an empty map must not manufacture a baseline"); + }); + + it("check() adopts a renumbered void entry: numbers update, no drift, markers clear, persists", () => { + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); + store.voidNumbers("factory reset"); + + const drift = store.check([{ uniqueId: "indigo-1", endpointNumber: 9 }]); + + assert.deepEqual(drift, [], "a voided renumbering must never be reported as drift"); + assert.equal(store.numberFor("indigo-1"), 9, "the new number is adopted as the baseline"); + assert.deepEqual(mapFileIn(dir).endpoints, { "indigo-1": { number: 9 } }, "and the marker is gone"); + }); + + it("a second check with the same (already-adopted) set is a quiet steady state", () => { + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); + store.voidNumbers("factory reset"); + store.check([{ uniqueId: "indigo-1", endpointNumber: 9 }]); + const settled = readFileSync(join(dir, ENDPOINT_MAP_FILE), "utf8"); + + const drift = store.check([{ uniqueId: "indigo-1", endpointNumber: 9 }]); + + assert.deepEqual(drift, []); + assert.equal( + readFileSync(join(dir, ENDPOINT_MAP_FILE), "utf8"), + settled, + "an already-adopted, unchanged set must cost no further write", + ); + }); + + it("clears the marker even when the live number happens to already match", () => { + // The marker itself is what must stop being true, not just a mismatch — + // otherwise a coincidentally-unchanged number would stay VOID forever, + // silently swallowing the very next real drift on that entry. + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); + store.voidNumbers("factory reset"); + + const drift = store.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); + + assert.deepEqual(drift, []); + assert.deepEqual(mapFileIn(dir).endpoints, { "indigo-1": { number: 2 } }, "marker cleared, not left set"); + }); + + it("does NOT adopt a NON-void entry alongside void ones — the never-auto-repaired rule survives", () => { + // The critical regression test: without the `numberVoid` guard in + // `check`, EVERY entry would adopt its live number regardless of + // marker, and genuine drift (a real storage loss, unrelated to a + // reset) would stop being reported for ever. + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([ + { uniqueId: "indigo-void", endpointNumber: 2 }, + { uniqueId: "indigo-drifted", endpointNumber: 3 }, + ]); + store.voidNumbers("factory reset"); + // Un-void the second entry by hand — standing in for an entry that was + // never touched by a reset at all (voidNumbers marks the whole map, so + // this is how a genuinely mixed map is produced for the test). + const file = mapFileIn(dir); + delete file.endpoints["indigo-drifted"]!.numberVoid; + writeFileSync(join(dir, ENDPOINT_MAP_FILE), JSON.stringify(file)); + const reloaded = new EndpointMapStore(dir); + reloaded.load(); + + const drift = reloaded.check([ + { uniqueId: "indigo-void", endpointNumber: 9 }, + { uniqueId: "indigo-drifted", endpointNumber: 99 }, + ]); + + assert.deepEqual(drift, [{ uniqueId: "indigo-drifted", expected: 3, actual: 99 }]); + assert.equal(reloaded.numberFor("indigo-void"), 9, "the void entry still adopted"); + assert.equal(reloaded.numberFor("indigo-drifted"), 3, "the baseline of a real drift must not move"); + }); + + it("the marker survives a persist/load round-trip", () => { + const dir = storage(); + const first = new EndpointMapStore(dir); + first.load(); + first.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); + first.voidNumbers("factory reset"); + + const second = new EndpointMapStore(dir); + second.load(); + + assert.deepEqual(mapFileIn(dir).endpoints, { "indigo-1": { number: 2, numberVoid: true } }); + // And the reloaded store still adopts rather than reports drift. + assert.deepEqual(second.check([{ uniqueId: "indigo-1", endpointNumber: 7 }]), []); + assert.equal(second.numberFor("indigo-1"), 7); + }); + + it("a legacy (pre-#140) file with no markers loads and drifts exactly as before", () => { + const dir = storage(); + writeFileSync( + join(dir, ENDPOINT_MAP_FILE), + JSON.stringify({ version: ENDPOINT_MAP_VERSION, endpoints: { "indigo-1": { number: 2 } } }), + ); + const store = new EndpointMapStore(dir); + store.load(); + + const drift = store.check([{ uniqueId: "indigo-1", endpointNumber: 9 }]); + + assert.deepEqual(drift, [{ uniqueId: "indigo-1", expected: 2, actual: 9 }], "unvoided — still real drift"); + assert.equal(store.numberFor("indigo-1"), 2, "and the baseline must not move"); + }); + + it("heals the reset's duplicate-number fingerprint: two void entries adopt distinct live numbers", () => { + // Live example (#140): the witness recorded the SAME number on two + // different entries after a reset. Voiding both and letting each adopt + // its own live number is what makes the map consistent again. + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([ + { uniqueId: "indigo-1", endpointNumber: 5 }, + { uniqueId: "indigo-2", endpointNumber: 5 }, + ]); + store.voidNumbers("factory reset"); + + const drift = store.check([ + { uniqueId: "indigo-1", endpointNumber: 5 }, + { uniqueId: "indigo-2", endpointNumber: 2 }, + ]); + + assert.deepEqual(drift, []); + assert.equal(store.numberFor("indigo-1"), 5); + assert.equal(store.numberFor("indigo-2"), 2); + assert.deepEqual(mapFileIn(dir).endpoints, { + "indigo-1": { number: 5 }, + "indigo-2": { number: 2 }, + }); + }); + + it("stays restorable while voided — role/label are untouched by voiding", () => { + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([{ uniqueId: "indigo-1", endpointNumber: 2, role: "onOffLight", label: "Lamp" }]); + + store.voidNumbers("factory reset"); + + assert.deepEqual(store.restorable(), [ + { uniqueId: "indigo-1", endpointNumber: 2, role: "onOffLight", label: "Lamp" }, + ]); + }); + + it("logs the adoption once when it happens, naming the count", () => { + const dir = storage(); + const logged: string[] = []; + const store = new EndpointMapStore(dir, message => logged.push(message)); + store.load(); + store.check([ + { uniqueId: "indigo-1", endpointNumber: 2 }, + { uniqueId: "indigo-2", endpointNumber: 3 }, + ]); + store.voidNumbers("factory reset"); + logged.length = 0; + + store.check([ + { uniqueId: "indigo-1", endpointNumber: 9 }, + { uniqueId: "indigo-2", endpointNumber: 8 }, + ]); + + assert.ok( + logged.some(line => line.includes("Adopted 2 endpoint number")), + `expected an adoption log line, got ${JSON.stringify(logged)}`, + ); + }); + + it("clears `checked` — a voided baseline has not been verified against anything", () => { + // ⊗ `voidNumbers` used to leave `#checked` untouched, so between a + // reset and the first post-reset reconcile, `driftChecked: true` / + // `drift: []` claimed "checked, nothing moved" over numbers the node + // itself had just declared untrustworthy. + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); + assert.equal(store.checked, true, "sanity: the first check earned the flag"); + + store.voidNumbers("factory reset"); + + assert.equal(store.checked, false, "a VOID baseline is unverified, whatever it was before"); + + store.check([{ uniqueId: "indigo-1", endpointNumber: 9 }]); + + assert.equal(store.checked, true, "the first post-reset check against live entries earns it back"); + }); + + it("reports the write failing, not a no-op, when the void cannot reach disk", () => { + // ⊗ The boolean `voidNumbers` returns was discarded at both call sites + // in `node.ts`, so a failed write left the VOID markers RAM-only while + // the log claimed the opposite — this is the store-level half of that: + // the caller has to be able to tell "wrote fine" from "in memory only". + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); + + chmodSync(dir, 0o500); + try { + assert.equal(store.voidNumbers("factory reset"), false, "the write failed"); + assert.equal(store.warnings.length, 1); + assert.match(store.warnings[0]!, /Could not write the endpoint map/); + } finally { + chmodSync(dir, 0o700); + } + assert.equal(store.checked, false, "an unwritten baseline still verified nothing durable"); + + // The marker survives in memory despite the failed write, and `#dirty` + // makes the next successful write retry it — proven by the very next + // check() adopting rather than reporting drift. + const drift = store.check([{ uniqueId: "indigo-1", endpointNumber: 9 }]); + + assert.deepEqual(drift, [], "the in-memory VOID marker survived the failed write and still adopts"); + assert.deepEqual(mapFileIn(dir).endpoints, { "indigo-1": { number: 9 } }); + }); +}); + describe("EndpointMapStore.forget — un-export without losing the number", () => { it("drops the restoration half and keeps the identity half", () => { // ⊗ The other half of `restorable`. Without it a device the user diff --git a/bridge-node/test/persistence.test.ts b/bridge-node/test/persistence.test.ts index 523c19c..1b99da0 100644 --- a/bridge-node/test/persistence.test.ts +++ b/bridge-node/test/persistence.test.ts @@ -381,8 +381,19 @@ describe("factory_reset (§3.10) and the endpoint map", () => { assert.deepEqual(reset.result, {}); // `ServerNode.erase()` wipes matter.js's own storage context. The map // surviving it is the entire reason the file is a sibling of it rather - // than a member. - assert.deepEqual(after.endpoints, before.endpoints); + // than a member. Since issue #140 the numbers themselves are also + // marked VOID — matter.js's own allocation was just wiped along with + // them — so the entries keep their number/role/label but every one + // gains `numberVoid: true`, which the next `check()` clears silently. + assert.deepEqual( + after.endpoints, + Object.fromEntries( + Object.entries(before.endpoints).map(([uniqueId, record]) => [ + uniqueId, + { ...record, numberVoid: true }, + ]), + ), + ); }); it("deletes it on preserveEndpointNumbers: false — PRD §7's explicit rebuild", async () => { @@ -511,6 +522,139 @@ describe("factory_reset (§3.10) and the endpoint map", () => { }); }); +describe("factory_reset (preserve: true) voids the map instead of drift-checking it (issue #140)", () => { + it("marks every entry VOID and logs the adoption message, not the old §3.11 remedy", async () => { + const storagePath = storage(); + const logged: string[] = []; + const bridge = new BridgeNode( + { storagePath, matterPort: 0, wsPort: 0 }, + { ...IDENTITY }, + BRIDGE_VERSION, + message => logged.push(message), + ); + await bridge.start(); + try { + await bridge.reconcile(ENDPOINTS as never, false); + const before = readMap(storagePath); + assert.ok(Object.keys(before.endpoints).length > 0, "sanity: something was recorded"); + + await bridge.factoryReset(true); + + const after = readMap(storagePath); + for (const [uniqueId, record] of Object.entries(before.endpoints)) { + assert.deepEqual( + after.endpoints[uniqueId], + { ...record, numberVoid: true }, + "the reset erased matter.js's own allocation, so the witness must be voided, not left standing", + ); + } + assert.ok( + logged.some(line => line.includes("now VOID")), + `expected the new voiding log line, got: ${logged.join("\n")}`, + ); + // #140's whole complaint: this remedy sent the user to a §3.11 + // rebuild the plugin refuses on a healthy node. + assert.ok( + !logged.some(line => line.includes("rebuild the map (§3.11) to accept it")), + "the old drift-and-rebuild-remedy line must be gone", + ); + } finally { + await bridge.close(); + } + }); + + it("tells the truth when the VOID markers cannot reach disk, instead of claiming success", async t => { + // ⊗ `voidNumbers`'s boolean return used to be discarded here, so a + // failed write still logged "its numbers are now VOID ... a §3.11 + // rebuild is not needed" — over markers that existed in memory only. + // A crash before the next successful persist would have brought back + // #140's forever-drift report after the user was told it was fixed. + if (process.getuid?.() === 0) { + t.skip("root ignores directory permissions"); + return; + } + const storagePath = storage(); + const logged: string[] = []; + const bridge = new BridgeNode( + { storagePath, matterPort: 0, wsPort: 0 }, + { ...IDENTITY }, + BRIDGE_VERSION, + message => logged.push(message), + ); + await bridge.start(); + try { + await bridge.reconcile(ENDPOINTS as never, false); + + chmodSync(storagePath, 0o500); + try { + await bridge.factoryReset(true); + } finally { + chmodSync(storagePath, 0o700); + } + + assert.ok( + logged.some(line => line.includes("VOID markers could NOT be written")), + `expected the write-failure branch, got: ${logged.join("\n")}`, + ); + assert.ok( + !logged.some(line => line.includes("its numbers are now VOID")), + "the success message must not fire over an unwritten baseline", + ); + } finally { + await bridge.close(); + } + }); +}); + +describe("noteLastFabricGone voids the endpoint map too (issue #140)", () => { + it("marks every entry VOID for the same reason as a preserving factory reset", async () => { + // The last-fabric self-reset (§3.9's last unpair, or an ecosystem + // removing us) drives matter.js to erase itself exactly as `erase()` + // does, so `noteLastFabricGone` carries the same obligation. Reached + // directly, the way `noteFabrics never swallows the read` below reaches + // its private method: a real self-reset cannot be manufactured without + // a real commissioner. + const storagePath = storage(); + writeFileSync( + join(storagePath, ENDPOINT_MAP_FILE), + JSON.stringify({ + version: ENDPOINT_MAP_VERSION, + endpoints: { + [uniqueIdFor(KITCHEN)]: { number: 2, role: "onOffLight", label: "Kitchen Lamp" }, + }, + }), + ); + const witness = "2026-08-01T00:00:00.000Z"; + const bridge = new BridgeNode( + { storagePath, matterPort: 0, wsPort: 0 }, + { ...IDENTITY, commissionedAt: witness }, + BRIDGE_VERSION, + () => {}, + ); + await bridge.start(); + try { + assert.equal( + bridge.endpointMapRefusal(), + RefuseReason.fabricStorageLost, + "sanity: a witness with no real fabric must be refusing", + ); + + (bridge as unknown as { noteLastFabricGone(): void }).noteLastFabricGone(); + + assert.deepEqual(readMap(storagePath).endpoints, { + [uniqueIdFor(KITCHEN)]: { + number: 2, + role: "onOffLight", + label: "Kitchen Lamp", + numberVoid: true, + }, + }); + } finally { + await bridge.close(); + } + }); +}); + describe("the drift check runs on every path that can move a number", () => { it("runs after a reconcile that failed part-way through", async () => { // The `finally` is load-bearing and nothing held it there: moving the diff --git a/docs/BRIDGE_PROTOCOL.md b/docs/BRIDGE_PROTOCOL.md index bc3ed52..bdd8ae8 100644 --- a/docs/BRIDGE_PROTOCOL.md +++ b/docs/BRIDGE_PROTOCOL.md @@ -277,16 +277,24 @@ the node's own `endpoint-map.json` in the bridge storage dir. {"command": "factory_reset", "args": {"preserveEndpointNumbers": true}} ``` -**What `preserveEndpointNumbers: true` preserves is the ability to NOTICE, not -the numbers.** matter.js owns the allocation, keyed on `Endpoint.id` in the +**What `preserveEndpointNumbers: true` preserves is the entries, not their +trustworthiness.** matter.js owns the allocation, keyed on `Endpoint.id` in the storage context `erase()` wipes — so the numbers themselves are gone either way, and the endpoints will be re-created at whatever matter.js hands out next. -What survives is the *baseline*: the map still says what each `UniqueID` used -to be, so the renumbering is reported as drift (§4.3) instead of happening -silently. Passing `false` discards the baseline too — the "explicit rebuild" of -PRD §7, for the case where the map itself is what is corrupt. The node re-runs -the drift check at the end of a preserving reset and says what it found, so the -consequence is in the log rather than waiting to surprise the next attach. +What survives is the *map*: role/label and every `UniqueID` stay, but the node +also **voids** every entry's number (issue #140, since bridge-node 0.8.0) +rather than leaving the old one standing. A voided number is silently +**adopted** — not reported as drift — +the next time `check` (§4.3) sees that `UniqueID` live, because both the reset +and the last-fabric self-reset are reached with an empty fabric set: matter.js +just erased its own allocation along with every fabric, so there is no paired +ecosystem left that could be holding the old numbers to disagree with, and the +adoption is unobservable outside this node. This is a narrow, deliberate carve-out +of §4.3's "never auto-repaired" rule (below) for the one case where the +renumbering is the reset's own, not a genuine anomaly — an entry that was never +voided still drifts exactly as before. Passing `false` discards the baseline +too — the "explicit rebuild" of PRD §7, for the case where the map itself is +what is corrupt. The commissioning witness is cleared either way, and the node **verifies** it by reading `identity.json` back before reporting completion: a witness that @@ -408,7 +416,15 @@ ecosystem acts. Both are enumerated here in full; there is no other source. `drift` lists any `UniqueID → endpointNumber` mappings that changed since last persist (PRD §4.3 drift detection); non-empty drift is surfaced as a plugin -error, never auto-repaired. Each entry is a `DriftEntry`: +error, never auto-repaired. **One narrow, deliberate carve-out (issue #140, +since bridge-node 0.8.0):** a renumbering caused by §3.10's preserving reset +(or the last-fabric self-reset) is voided at the reset and then silently +**adopted**, not reported here — see §3.10 for the safety argument. Voiding +also drops `driftChecked` back to `false`: a baseline the node just declared +void has not been verified against anything, and the flag comes back once the +first post-reset reconcile checks it against live endpoints. Everything else +about this rule is unchanged: an entry the reset did not touch still drifts +and is still never repaired. Each entry is a `DriftEntry`: ```json {"uniqueId": "indigo-123456789", "expected": 2, "actual": 5} @@ -471,6 +487,14 @@ version 2** since bridge-node 0.6.0: lets the node rebuild an accessory without the plugin. `options` is **not** stored: nothing in the node reads it (window-covering polarity is applied plugin-side, §4.1). +- `numberVoid` — issue #140, since bridge-node 0.8.0, present (`true`) and + absent otherwise, never `false`. Set on every entry by §3.10's preserving + reset and by the last-fabric self-reset; cleared the next time `number` is + silently adopted + from a live endpoint rather than compared against it. Still schema version 2 + — an old build reads a file that has it exactly as it would without it, + because every reader here reaches a field by name rather than validating the + object as a whole, so the extra key is inert rather than a parse failure. **Version 1 files are read, migrated in place, and never treated as corrupt.** A v1 entry is a bare number; it keeps that number, is simply not restorable diff --git a/docs/HANDOVER.md b/docs/HANDOVER.md index 7b80a52..7d33851 100644 --- a/docs/HANDOVER.md +++ b/docs/HANDOVER.md @@ -1,34 +1,98 @@ # indigo-matter — Build Handover -**Last updated:** 2026-08-08 18:19 UTC -**Active work:** none — nothing in flight, working tree clean. -**Branch:** `main` — **PR #148 MERGED** (`775de2e`, issue #132 closed) and -**release v2026.8.6 CUT** by the workflow on that merge. That was the first -non-`[no-release]` merge since v2026.7.23, so the release also ships the -banked #141 endpoint restore and #134 menu sections — the `[no-release]` debt -recorded here since 2026-08-06 is **cleared**. -**Version:** plugin `2026.8.7` in tree (v2026.8.6 is the released tag; this -bump exists because check-version fails on any PR whose version is already a -tag — docs-only PRs rode the untagged 2026.8.5 before, and the v2026.8.6 -release closed that loophole). Bridge-node `0.7.0` (**published to npm**; -`DEFAULT_INSTALL_SPEC` pins it). -**Tests:** **2256 Python**, **383 TS** — both green. pylint 9.42 whole-repo -baseline. (`python3 -m pytest -q` · `cd bridge-node && npm run build && npm test`) -**Deployed:** jarvis runs plugin `2026.8.5` + bridge-node `0.7.0`, paired to -**Apple Home AND Alexa simultaneously** (3 fabrics: 2 Apple, 1 Alexa) — now -**one patch behind** the v2026.8.6 release. The delta is #132's dialog/log -strings only, so lagging is harmless; true up with the release bundle (or the -two changed files `MenuItems.xml` + `Info.plist` plus `bridge_client.py`, -`export_bridge.py`, `bridge_protocol.py`, `plugin.py` over SSH) whenever -convenient. -**Status:** export v1 feature-complete and live. #141 **fixed and confirmed** -(below). **#143 is the open one** — see the 2026-08-06 §#143 section; its -defect B was investigated hard and the leading theory was **withdrawn**, so -read that before touching it. - -**NEXT UP:** nothing queued. Open issues: #143 (parked for beta testers — -read its section first), #140, #105, #83, #84, #62 follow-up, #43, #46, -#21–#24, and the E8 docs pass (#137/#138 + the Field Notes site). +**Last updated:** 2026-08-08 21:06 UTC +**Active work:** `fix/140-adopt-reset-renumbering` — #140 built, PR open, +awaiting review + Simon's merge go-ahead. See the §#140 section below, +**including the publish coupling and the jarvis one-time map edit**. +**Branch:** `main` last merged **PR #150** (`298ee6e`, #131 picker sort → +release **v2026.8.8**); before it #148 (`775de2e`, #132 → v2026.8.6, which +also shipped the banked #141+#134 `[no-release]` debt) and #149 (docs, +`[no-release]`). Releases v2026.8.6/.8.8 exist; 2026.8.7 was never tagged. +**Version:** plugin `2026.8.9` on the #140 branch (v2026.8.8 released). +Bridge-node **source is now ahead of the published `0.7.0`** — the #140 +adoption is node-side and reaches installs only when `0.8.0` is published +(bump `bridge-node/package.json` + `DEFAULT_INSTALL_SPEC` together, then +`npm publish` — the #141 section's recipe applies verbatim). +**Tests:** **2265 Python**, **396 TS** — both green. +(`python3 -m pytest -q` · `cd bridge-node && npm run build && npm test`) +**Deployed:** jarvis runs plugin **`2026.8.8`** (deployed 2026-08-08 ~20:38 +UTC: 6-file copy over SSH, checksum-verified against clean 2026.8.5 first — no +hot-patches — then restart; up with 6 endpoints attached) + bridge-node +`0.7.0`, paired to **Apple Home AND Alexa** (3 fabrics: 2 Apple, 1 Alexa). +The #140 drift alarm (`indigo-459564566: expected 5, got 2`) fired on the +deploy's own attach — the live specimen of the issue. +**Status:** export v1 feature-complete and live. **#143 is the parked one** — +see the 2026-08-06 §#143 section; its defect B leading theory was +**withdrawn**, read before touching. + +**NEXT UP:** after #140 merges — publish bridge-node `0.8.0` (needs Simon's +npm login), deploy plugin `2026.8.9` + node `0.8.0` to jarvis, do the +one-time jarvis map edit (§#140), and verify the alarm is gone. Then the +remaining backlog: #133, #135, #84, #105, #101, the E8 docs pass +(#137/#138/#139), #146/#147. + +--- + +## 2026-08-08 (evening) — issue #140: the reset's own renumbering is adopted, not alarmed about forever + +Plugin `2026.8.9`, branch `fix/140-adopt-reset-renumbering`. Suites **2265 +Python** / **396 TS** (from 2264/383; 1 + 13 new). **No wire change — golden +frames untouched.** + +**The defect was two deliberate designs colliding.** A preserving factory +reset kept the endpoint map *specifically to report* the renumbering the reset +causes ("preserves the ability to NOTICE"), and its log line told the user to +clear the report with the §3.11 rebuild — which the plugin's M11 gate +correctly *refuses* on a healthy node. Permanent alarm, locked door. Observed +live on jarvis at every attach (`indigo-459564566: expected 5, got 2`). + +**The fix is the issue's option (3), node-side only.** At both reset sites — +§3.10 `factory_reset preserveEndpointNumbers: true` and the last-fabric +self-reset (`noteLastFabricGone`) — the node now **voids** every map entry's +number (`numberVoid: true`, per entry); `check()` silently **adopts** the next +live number for a void entry and clears the marker (even when the numbers +happen to match — the marker is what must stop being true). The safety +argument, now in the class comment: matter.js is *always* the allocator (the +map is witness-only; even the #141 restore takes matter.js's number), and both +reset sites are reached with an **empty fabric set**, so no paired ecosystem +exists that could hold the old numbers — adoption is unobservable outside the +node. An entry that was never voided **still drifts exactly as before**; on a +current node, surviving drift is now a *stronger* signal (storage changed +outside any reset), and the plugin's drift error says so. + +**Schema stayed at v2, deliberately.** Every reader takes fields by name, so +an old build reading a file carrying `numberVoid` never looks at the key — a +version bump would guard nothing. Documented at `ENDPOINT_MAP_VERSION`. + +**The version claim in the plugin's drift message names the BRIDGE NODE +(≥ 0.8.0), not the plugin.** First draft said "since v2026.8.9" — but adoption +is node-side, and a new plugin driving the published `0.7.0` node still gets +reset drift; claiming it away would have been #132's mistake over again. The +test pins "0.8.0". + +**Publish coupling (do not forget):** `bridge-node/package.json` is still +`0.7.0` and `DEFAULT_INSTALL_SPEC` still pins `0.7.0` — correct per the #141 +precedent (never pin an unpublished version). Shipping #140 to installs means: +bump both to `0.8.0` together, `npm publish` (Simon's npm login), +`test_bridge_agent.py` asserts the pin matches. + +**jarvis needs a one-time hand edit** — its stale entry predates the marker, +so the fix cannot clear it retroactively. With the bridge node stopped (*Stop +the Matter export bridge…*), edit +`~/Library/Application Support/com.simons-plugins.indigo-matter/bridge-node/endpoint-map.json`: +set `indigo-459564566`'s number to the live `2`, and resolve the duplicate +number 5 (two entries record it — the reset's fingerprint; the non-live one +keeps it only if no live endpoint owns 5). Alternative: add `"numberVoid": +true` to the stale entries and let the new node adopt on its next attach — +strictly safer, no numbers guessed. + +**Mutation-verified:** adopt-without-checking-the-marker fails the +non-void-still-drifts test (+3 collateral); skipping `voidNumbers` at +`noteLastFabricGone` fails its dedicated test. The duplicate-number +fingerprint heals under adoption (tested). One pre-existing test updated +(`persistence.test.ts` "keeps endpoint-map.json by default" — entries now gain +the marker after a preserving reset); the known `main.test.ts` spawn flake +appeared once in three runs, unrelated file, documented before. --- diff --git a/docs/PRD-indigo-matter-export.md b/docs/PRD-indigo-matter-export.md index f39d3b6..ea31b7e 100644 --- a/docs/PRD-indigo-matter-export.md +++ b/docs/PRD-indigo-matter-export.md @@ -126,7 +126,11 @@ from the controller agent: set — the first attach's reconcile, and every `upsert_endpoint` / `remove_endpoint` after it. Drift is **reported, never repaired**: an auto-repair would bless the storage loss that caused it and make the next - occurrence invisible too. + occurrence invisible too. One deliberate exception (issue #140): a + renumbering caused by a factory reset is voided and silently adopted rather + than reported, because the reset itself already erased matter.js's own + allocation with no paired ecosystem left to disagree — everything else still + drifts exactly as before. - **Storage loss is the #1 real-world accessory-duplication cause** (not logic bugs): a missing/relocated storage dir reallocates every endpoint number and every ecosystem re-creates every accessory, losing names, rooms and diff --git a/indigo-matter.indigoPlugin/Contents/Info.plist b/indigo-matter.indigoPlugin/Contents/Info.plist index cb11f5a..5c6de17 100644 --- a/indigo-matter.indigoPlugin/Contents/Info.plist +++ b/indigo-matter.indigoPlugin/Contents/Info.plist @@ -20,7 +20,7 @@ IwsApiVersion 1.0.0 PluginVersion - 2026.8.8 + 2026.8.9 ServerApiVersion 3.6 diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py index 39d0ff9..8a79164 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py @@ -1376,12 +1376,22 @@ def _on_drift_detected(self, drift: list) -> None: """Report a drift SET once, however many times the node re-reports it. Drift is by design never repaired, so it is re-detected by every attach - and every upsert for as long as it lasts — and after a - ``factory_reset preserveEndpointNumbers: true`` that is *every exported - device*, on every reconcile, with no way for the user to clear it short - of §3.11. Unlatched, the one error that names the problem is buried - under its own repetitions. Latched on the set, so a device joining the - drift is still news. + and every upsert for as long as it lasts. Unlatched, the one error that + names the problem is buried under its own repetitions. Latched on the + set, so a device joining the drift is still news. + + Since bridge-node 0.8.0 (issue #140) a ``factory_reset + preserveEndpointNumbers: true`` no longer lands here at all: the NODE + voids its own witness at reset time and silently adopts the renumbering + the reset itself causes, because matter.js's allocation was erased + along with the fabrics and no paired ecosystem could still be holding + the old numbers. Anything that DOES reach this handler from a >=0.8.0 + node is therefore not the reset renumbering — it is the bridge's + storage changing for some other reason, which is exactly the anomaly + this detector exists to catch. The adoption lives node-side, which is + why the message below names the bridge-node version, not the plugin's: + a new plugin driving an old node still gets reset drift here, and + claiming it away would be #132's mistake over again. """ seen = frozenset((d.unique_id, d.expected, d.actual) for d in drift) if seen == self._drift_reported: @@ -1389,7 +1399,10 @@ def _on_drift_detected(self, drift: list) -> None: self._drift_reported = seen self._logger.error( "Matter export: endpoint-number DRIFT detected — %s. Exported accessories may have " - "swapped identities in paired ecosystems. This is never repaired automatically.", + "swapped identities in paired ecosystems. Bridge nodes 0.8.0 and newer adopt a " + "factory reset's own renumbering automatically, so on a current node persistent " + "drift means the bridge's storage changed OUTSIDE any reset — treat it as a real " + "anomaly; there is deliberately no dismiss.", ", ".join(f"{d.unique_id}: expected {d.expected}, got {d.actual}" for d in drift)) # ------------------------------------------------------------------ diff --git a/tests/test_export_bridge.py b/tests/test_export_bridge.py index 0ca097c..984245f 100644 --- a/tests/test_export_bridge.py +++ b/tests/test_export_bridge.py @@ -752,6 +752,22 @@ def test_drift_is_reported_never_repaired(self, bridge_mod, mock_logger, devices assert "DRIFT" in errors_of(mock_logger) assert h.client.names() == [], "drift must not trigger a repair" + def test_drift_message_says_reset_renumbering_is_adopted_automatically( + self, bridge_mod, mock_logger, devices): + # #140: a factory-reset renumbering no longer reaches this handler at + # all (the node adopts it itself) — so drift that DOES arrive here is a + # real anomaly, and the message must say so, not point at the reset. + h = self._bridge(bridge_mod, mock_logger, devices) + h.bridge._on_drift_detected(bridge_protocol.parse_drift( + FRAMES["drift_detected"]["data"]["drift"])) + said = errors_of(mock_logger) + # The version named is the BRIDGE NODE's, not the plugin's — adoption + # lives node-side, and a new plugin driving an old node still gets + # reset drift here (claiming otherwise would be #132 again). + assert "0.8.0" in said + assert "renumbering automatically" in said + assert "outside any reset" in said.lower() + def test_an_unreachable_node_is_reported_once_per_outage(self, bridge_mod, mock_logger, devices): h = self._bridge(bridge_mod, mock_logger, devices)