diff --git a/bridge-node/src/endpoint-map.ts b/bridge-node/src/endpoint-map.ts index 4abb1ee..570ebd7 100644 --- a/bridge-node/src/endpoint-map.ts +++ b/bridge-node/src/endpoint-map.ts @@ -1010,6 +1010,53 @@ export class EndpointMapStore { return changed; } + /** + * A device is CONFIRMED GONE FOR GOOD — deleted from Indigo, or + * deliberately un-exported — so its record is dropped ENTIRELY rather + * than kept as an orphan (issue #274). + * + * **This is the one place a record's `number` is thrown away**, unlike + * every other mutation here ({@link forget}, {@link voidNumbers}), all of + * which keep it forever precisely so a retired number is never handed to + * a different accessory (ADR-0010). That guarantee still holds after this + * call: matter.js's OWN persisted allocation for the retired + * `Endpoint.id` — a separate store this file never touches — is what + * actually prevents reissue, and it is untouched by deleting our own + * witness of it. What is lost is re-adopt: {@link restorable} and + * {@link orphans} can only offer what is still in `#endpoints`, so a + * destroyed identity is gone from both — no re-adopt picker entry, no + * pre-attach rebuild, ever again. + * + * **Only a caller certain the device will never come back may call + * this.** {@link EndpointMapStore.forget}'s doc comment explains why an + * ORDINARY un-export (an `attach` reconcile that simply does not mention + * an identity this pass) must stay soft: the node cannot tell a + * deliberate departure from a device that merely failed classification + * this one time (issue #274 constraint 2). `node.ts`'s `removeEndpoint` + * is the only caller, and only when the plugin's `remove_endpoint` + * carried `permanent: true`. + * + * Returns how many entries actually existed and were removed, so a call + * over an already-forgotten or never-recorded `uniqueId` costs no disk + * write — the same idempotency `remove_endpoint` itself promises + * (§3.3: `{removed: false}` for an absent endpoint). + */ + destroy(uniqueIds: readonly string[]): number { + let removed = 0; + for (const uniqueId of uniqueIds) { + if (this.#endpoints.delete(uniqueId)) { + removed += 1; + } + } + if (removed > 0) { + this.persist( + `destroyed ${removed} endpoint map record(s): the driving device is gone for good, so ` + + "neither the number nor the role/label is retained for re-adopt", + ); + } + return removed; + } + /** * Compare the live endpoint numbers against the map (PRD §4.3). * diff --git a/bridge-node/src/node.ts b/bridge-node/src/node.ts index 90896ad..4d8f467 100644 --- a/bridge-node/src/node.ts +++ b/bridge-node/src/node.ts @@ -1464,8 +1464,21 @@ export class BridgeNode implements BridgeFacade { * then `upsert_endpoint` as two SEPARATE commands (§3 steps 3/5), so the * create half of that supersession has not happened yet when THIS command * finishes — {@link upsertEndpoint} is what closes the gap when it does. + * + * **`options.hard` (issue #274) destroys rather than orphans the + * `ordinary` bucket.** Only {@link removeEndpoint} ever passes it, and + * only when the plugin has declared the removal PERMANENT — the driving + * device is deleted, or deliberately un-exported. `attach`'s reconcile + * (`reconcile()`) never does: a full reconcile cannot tell "the plugin + * dropped this on purpose" from "this device exists but failed + * classification this pass" (issue #274 constraint 2), so its ordinary + * removals stay soft — orphaned, kept, restorable — exactly as before + * this issue. A hard removal is also never eligible for the supersede + * pairing above (a deleted device gets no later create to pair with) or + * for {@link #lastRemoved} (nothing should retroactively mark a + * DESTROYED entry `supersededBy`). */ - private forgetRemoved(before: ReadonlyMap): void { + private forgetRemoved(before: ReadonlyMap, options: { hard?: boolean } = {}): void { const after = this.livePublishedIdentities(); const removed = [...before.keys()].filter(uniqueId => !after.has(uniqueId)); if (removed.length === 0) { @@ -1481,12 +1494,12 @@ export class BridgeNode implements BridgeFacade { for (const uniqueId of removed) { const deviceId = before.get(uniqueId)!; const created = createdDeviceIds.get(deviceId); - if (created !== undefined && supersedes(uniqueId, created)) { + if (!options.hard && created !== undefined && supersedes(uniqueId, created)) { forgotten += this.#endpointMap.forget([uniqueId], { supersededBy: created }); continue; } ordinary.push(uniqueId); - if (created === undefined) { + if (!options.hard && created === undefined) { // Only when NO identity for this device appeared in this // mutation: a create that already landed and was NOT a // supersession (a re-adopt, PR5 design E2/E5) has settled the @@ -1495,6 +1508,17 @@ export class BridgeNode implements BridgeFacade { this.#lastRemoved.set(deviceId, uniqueId); } } + if (options.hard) { + forgotten += this.#endpointMap.destroy(ordinary); + if (forgotten > 0) { + this.log( + `${forgotten} endpoint map record(s) destroyed — the driving device is gone for good ` + + "(deleted or un-exported); no re-adopt is offered for them and their numbers are not " + + "retained (issue #274)", + ); + } + return; + } forgotten += this.#endpointMap.forget(ordinary); if (forgotten > 0) { this.log( @@ -1647,12 +1671,12 @@ export class BridgeNode implements BridgeFacade { * only one that never looked would have made that invisible until the next * upsert happened to notice. */ - async removeEndpoint(indigoDeviceId: number): Promise { + async removeEndpoint(indigoDeviceId: number, permanent = false): Promise { const before = this.livePublishedIdentities(); try { return await this.registry.remove(indigoDeviceId); } finally { - this.forgetRemoved(before); + this.forgetRemoved(before, { hard: permanent }); this.checkDrift(); } } diff --git a/bridge-node/src/protocol.ts b/bridge-node/src/protocol.ts index 1b2873e..7fdcb58 100644 --- a/bridge-node/src/protocol.ts +++ b/bridge-node/src/protocol.ts @@ -584,8 +584,20 @@ export interface BridgeFacade { reconcile(endpoints: readonly EndpointSpec[], replaceAll: boolean): Promise; /** §3.2 — create-or-update. Rejects a role change with `role_change` (§4.1). */ upsertEndpoint(spec: EndpointSpec): Promise; - /** §3.3 — idempotent; `{removed: false}` for a device with no live endpoint. */ - removeEndpoint(indigoDeviceId: number): Promise; + /** + * §3.3 — idempotent; `{removed: false}` for a device with no live endpoint. + * + * `permanent` (issue #274, default `false`) is the confirmed-gone + * declaration: `true` destroys the endpoint-map record along with the + * live endpoint (no orphan, no re-adopt), for a device that has been + * deleted or deliberately un-exported. `false` — the default, and what + * every pre-#274 caller still gets — keeps the pre-existing orphan + * behaviour, which the two-command supersede/re-adopt sequence + * (`export_bridge.replace()`) depends on: its `upsert_endpoint` half + * still needs an orphaned entry to mark `supersededBy`, or to hand to + * the re-adopt picker. + */ + removeEndpoint(indigoDeviceId: number, permanent?: boolean): Promise; /** §3.4 — local (offline-context) writes, so they do not echo as `command`. */ setState(indigoDeviceId: number, states: Record): Promise; /** §3.5 — Bridged Device Basic Information `Reachable`. */ diff --git a/bridge-node/src/reconcile.ts b/bridge-node/src/reconcile.ts index 1cae124..2b65338 100644 --- a/bridge-node/src/reconcile.ts +++ b/bridge-node/src/reconcile.ts @@ -177,6 +177,31 @@ export function parsePreserveEndpointNumbers(value: unknown): boolean { return value; } +/** + * §3.3: whether a `remove_endpoint` is a PERMANENT departure — the driving + * Indigo device is confirmed gone (deleted, or deliberately un-exported) — + * rather than the removal half of a role-change/re-adopt `replace()`, which + * the plugin always follows with an `upsert_endpoint` for the same identity + * (issue #274). + * + * Absent means `false`, matching every `remove_endpoint` call before this + * flag existed: the two-command supersede/re-adopt sequence + * (`export_bridge.replace()`) must keep getting the ORIGINAL soft removal — + * orphaned, not destroyed — so the second command's `upsert_endpoint` still + * has an entry to retroactively mark `supersededBy`, or to hand back to the + * re-adopt picker. Only a caller that KNOWS the device will never come back + * opts in explicitly. + */ +export function parsePermanentRemoval(value: unknown): boolean { + if (value === undefined) { + return false; + } + if (typeof value !== "boolean") { + throw new ProtocolError(ErrorCode.malformedArgs, "permanent must be a boolean"); + } + return value; +} + /** §3.1: the opt-in that makes emptying the endpoint set deliberate. */ export function parseReplaceAll(intent: unknown): boolean { if (intent === undefined) { diff --git a/bridge-node/src/ws-server.ts b/bridge-node/src/ws-server.ts index e983288..4b14fde 100644 --- a/bridge-node/src/ws-server.ts +++ b/bridge-node/src/ws-server.ts @@ -33,6 +33,7 @@ import { parseEndpointSpec, parseEndpointSpecs, parseFabricIndex, + parsePermanentRemoval, parsePreserveEndpointNumbers, parseReplaceAll, } from "./reconcile.js"; @@ -89,7 +90,7 @@ export class BridgeWsServer { this.options.bridge.upsertEndpoint(parseEndpointSpec(args.endpoint)), ); this.#handlers.set("remove_endpoint", async args => - this.options.bridge.removeEndpoint(parseDeviceId(args.indigoDeviceId)), + this.options.bridge.removeEndpoint(parseDeviceId(args.indigoDeviceId), parsePermanentRemoval(args.permanent)), ); this.#handlers.set("set_state", async args => this.handleSetState(args)); this.#handlers.set("set_reachable", async args => this.handleSetReachable(args)); diff --git a/bridge-node/test/endpoint-map.test.ts b/bridge-node/test/endpoint-map.test.ts index a1de285..fb9b523 100644 --- a/bridge-node/test/endpoint-map.test.ts +++ b/bridge-node/test/endpoint-map.test.ts @@ -753,6 +753,65 @@ describe("EndpointMapStore.forget — un-export without losing the number (issue }); }); +describe("EndpointMapStore.destroy — a confirmed-gone device loses its record entirely (issue #274)", () => { + it("deletes the entry outright: no number, no role/label, not restorable, not offered for re-adopt", () => { + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([ + { uniqueId: "indigo-1", endpointNumber: 2, role: "onOffLight", label: "Lamp" }, + { uniqueId: "indigo-2", endpointNumber: 3, role: "dimmableLight", label: "Other" }, + ]); + + assert.equal(store.destroy(["indigo-1"]), 1); + + assert.equal(store.numberFor("indigo-1"), undefined, "the number itself is gone, unlike forget()"); + assert.deepEqual(store.restorable(), [ + { uniqueId: "indigo-2", endpointNumber: 3, indigoDeviceId: 2, role: "dimmableLight", label: "Other" }, + ]); + assert.deepEqual(store.orphans(), [], "destroyed, not orphaned — nothing left to offer the re-adopt picker"); + assert.deepEqual(mapFileIn(dir).endpoints, { + "indigo-2": { number: 3, role: "dimmableLight", label: "Other" }, + }); + }); + + it("also destroys an entry that was already orphaned (an un-export later confirmed as a deletion)", () => { + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([{ uniqueId: "indigo-1", endpointNumber: 2, role: "onOffLight", label: "Lamp" }]); + store.forget(["indigo-1"]); + assert.deepEqual(store.orphans().map(orphan => orphan.uniqueId), ["indigo-1"]); + + assert.equal(store.destroy(["indigo-1"]), 1); + + assert.deepEqual(store.orphans(), []); + assert.equal(mapFileIn(dir).endpoints["indigo-1"], undefined); + }); + + it("costs no disk write when there was nothing to destroy — unknown ids are a no-op, like forget()", () => { + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([{ uniqueId: "indigo-1", endpointNumber: 2 }]); + const before = readFileSync(join(dir, ENDPOINT_MAP_FILE), "utf8"); + + assert.equal(store.destroy(["indigo-nope"]), 0); + + assert.equal(readFileSync(join(dir, ENDPOINT_MAP_FILE), "utf8"), before); + }); + + it("is idempotent — destroying an already-destroyed (i.e. absent) entry is a no-op", () => { + const dir = storage(); + const store = new EndpointMapStore(dir); + store.load(); + store.check([{ uniqueId: "indigo-1", endpointNumber: 2, role: "onOffLight", label: "Lamp" }]); + assert.equal(store.destroy(["indigo-1"]), 1); + + assert.equal(store.destroy(["indigo-1"]), 0); + }); +}); + describe("EndpointMapStore.seed replaces the whole map", () => { it("refuses to seed fewer numbers than the baseline already holds", () => { // ⊗ `seed` is a full replace, and nothing said so. Its one caller diff --git a/bridge-node/test/reconcile.test.ts b/bridge-node/test/reconcile.test.ts index 8033637..66e1547 100644 --- a/bridge-node/test/reconcile.test.ts +++ b/bridge-node/test/reconcile.test.ts @@ -12,6 +12,7 @@ import { parseDeviceId, parseEndpointSpec, parseEndpointSpecs, + parsePermanentRemoval, parseReplaceAll, planReconcile, } from "../src/reconcile.js"; @@ -149,6 +150,22 @@ describe("parseEndpointSpecs / parseReplaceAll (§3.1)", () => { }); }); +describe("parsePermanentRemoval (§3.3, issue #274)", () => { + it("defaults to false — every remove_endpoint call before this flag existed keeps the soft (orphan) behaviour", () => { + assert.equal(parsePermanentRemoval(undefined), false); + }); + + it("honours an explicit true or false", () => { + assert.equal(parsePermanentRemoval(true), true); + assert.equal(parsePermanentRemoval(false), false); + }); + + it("rejects a non-boolean", () => { + assert.equal(refusal(() => parsePermanentRemoval("true")).code, ErrorCode.malformedArgs); + assert.equal(refusal(() => parsePermanentRemoval(1)).code, ErrorCode.malformedArgs); + }); +}); + describe("publishedAs (§4.1, issues #219/#240)", () => { it("defaults publishedAs to indigo- when the wire omits it", () => { assert.equal( diff --git a/bridge-node/test/restore.test.ts b/bridge-node/test/restore.test.ts index 0b1a99d..eb56377 100644 --- a/bridge-node/test/restore.test.ts +++ b/bridge-node/test/restore.test.ts @@ -709,6 +709,119 @@ describe("issue #141: an un-exported device stops being restored", () => { }); }); +/** `remove_endpoint`, optionally with the issue #274 `permanent` flag, awaited. */ +async function removeOne( + client: TestClient, + messageId: string, + indigoDeviceId: number, + permanent?: boolean, +): Promise<{ removed: boolean }> { + client.send({ + message_id: messageId, + command: "remove_endpoint", + args: { indigoDeviceId, ...(permanent === undefined ? {} : { permanent }) }, + }); + for (;;) { + const frame = await client.next(10_000); + if (frame.message_id === messageId) { + assert.equal(frame.error_code, undefined, JSON.stringify(frame)); + return frame.result as { removed: boolean }; + } + } +} + +describe("issue #274: remove_endpoint's permanent flag destroys rather than orphans", () => { + it("drops the map entry entirely — no number, not restorable, not offered for re-adopt", async () => { + const storagePath = storage(); + const numbers = await seedTwoAccessories(storagePath); + + const session = await boot(storagePath); + try { + // §2: a fresh connection must attach before any CRUD command — + // with the SAME set already live, so this is a pure update and + // does not itself touch LOUNGE. + await attach(session.client, "a1", BOTH); + const result = await removeOne(session.client, "r1", LOUNGE, true); + assert.equal(result.removed, true); + + assert.deepEqual(readMap(storagePath).endpoints[uniqueIdFor(LOUNGE)], undefined, + "the entry is gone, not orphaned"); + assert.deepEqual(session.bridge.listOrphans(), [], + "nothing is left to offer the re-adopt picker"); + assert.equal( + childOf(session.bridge.server, LOUNGE), undefined, + "the live accessory is gone from the aggregator too", + ); + } finally { + await session.close(); + } + + // A later restart does not bring it back — there is nothing left to + // restore it FROM. + const second = await boot(storagePath); + try { + assert.deepEqual( + second.bridge.getStatus().endpoints.map(endpoint => endpoint.indigoDeviceId), + [KITCHEN], + "only the still-exported device is restored", + ); + assert.deepEqual(numbersOf(second.bridge), { [KITCHEN]: numbers[KITCHEN] }); + } finally { + await second.close(); + } + }); + + it("without the flag, keeps today's soft (orphan) behaviour — the default is unchanged", async () => { + const storagePath = storage(); + await seedTwoAccessories(storagePath); + + const session = await boot(storagePath); + try { + await attach(session.client, "a1", BOTH); + await removeOne(session.client, "r1", LOUNGE); + + const record = readMap(storagePath).endpoints[uniqueIdFor(LOUNGE)]; + assert.equal(record?.orphaned, true, "the pre-#274 orphan behaviour is untouched by default"); + assert.deepEqual( + session.bridge.listOrphans().map(orphan => orphan.uniqueId), + [uniqueIdFor(LOUNGE)], + "still offered for re-adopt, exactly as before this issue", + ); + } finally { + await session.close(); + } + }); + + it("is idempotent on an already-absent endpoint, exactly like a non-permanent remove", async () => { + const storagePath = storage(); + const session = await boot(storagePath); + try { + await attach(session.client, "a1", []); + const result = await removeOne(session.client, "r1", 999999999, true); + assert.deepEqual(result, { removed: false }); + } finally { + await session.close(); + } + }); + + it("rejects a non-boolean permanent with malformed_args", async () => { + const storagePath = storage(); + const session = await boot(storagePath); + try { + await attach(session.client, "a1", []); + session.client.send({ + message_id: "r1", + command: "remove_endpoint", + args: { indigoDeviceId: KITCHEN, permanent: "yes" }, + }); + const frame = await session.client.next(10_000); + assert.equal(frame.error_code, ErrorCode.malformedArgs); + } finally { + await session.close(); + } + }); +}); + describe("issue #141: what a restored endpoint actually publishes", () => { it("comes up under its own recorded name, before any attach", async () => { // ⊗ T1. `label: entry.label` was pinned by nothing: mutate it to a diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 1aac8aa..0cb9cd8 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -41,7 +41,7 @@ loop→Indigo writes go straight through `device_sync.apply_states` (thread-safe | `export_store.py` | The export allow-list (PRD-indigo-matter-export §5.1): `ExportEntry` (device id + role + name override + options) and an `RLock`'d store persisted as ONE JSON string in `pluginPrefs["matterExports"]`, schema-versioned. A blob it cannot parse is moved aside to `matterExports.corrupt` and the store starts empty — user config is never silently discarded | | `export_catalog.py` | Indigo device → eligible Matter roles, or an `Excluded(reason)` shown in the picker (PRD §5.2, XAC9). The loop guard (XNG3/XAC6) is `pluginId` and nothing else, checked before any type reasoning. Type dispatch walks the IOM **class-name chain**, not `isinstance` — the indigo module is a MagicMock under test. **Three outcomes since ADR-0012** (issue #252): a device Indigo does not type (`` → a plain `indigo.Device`, no IOM subclass) has no reading the catalog can find, so `classify` takes the export's `options` as a third argument and a declared `stateKey` is what makes it `EligibleDevice`; without one it is `MappableDevice`, which deliberately carries **no** `eligible_roles` so no caller can duck-type an unmapped device into an exportable one. Every guard therefore tests positively for `EligibleDevice`. Callers holding an entry MUST pass its options or a mapped export re-reads as un-exportable | | `export_handlers.py` | The **outbound** handler table, keyed by §4.2 **role** (the inbound registry is keyed by cluster; outbound there is no cluster, only a user-declared role) — `states_for` / `diff` / `dispatch` per role, each taking the export's §4.1 `options`. **Total over the v1 role enum since E4**: plug, on/off light, dimmable, colour-temp, extended colour, covering, lock, the seven sensors, thermostat. Hue diffs carry a ±1° tolerance (Matter's 0–254 hue round-trips ±1°); saturation deliberately has none; colour-temperature diffs carry `CT_TOLERANCE_MIREDS` (±30 mireds — not conversion noise but the permanent warm-limit clamp gap of ADR-0013, paired with the `commanded_states` hook that `ExportBridge._apply_command` pushes after a confirmed `setColorTemp`). `windowCovering` applies the per-export `invert` polarity here so a `position` on the wire always means 100 = open; `doorLock` dispatches `indigo.device.lock`/`unlock` and confirms **nothing** (PRD §7). Indigo declares no units, so sensor/thermostat readings are passed through as already being in the §4.2 unit — documented in the module header as the known gap the device catalog should close. **One exception, `pressureSensor`:** Indigo's barometer convention is hPa (this plugin's own inbound handler writes it, and `export_catalog` routes `hpa`/`mbar` names here) and §4.2's key is `pressureKPa`, so it divides by 10. **Since issue #220, `batteryLevel` is role-independent** — added by `ExportHandler.published_states`, a wrapper every role goes through (six of the fifteen skip `super().states_for()` entirely, so the key cannot live inside any one role's own override), never by a role's own `states_for`. `battery_percent` treats a literal 0 as "never polled" (issue #190: Indigo initialises new Integer states to 0), the deliberate opposite of `_first_number`'s temperature ruling elsewhere in the module | -| `export_bridge.py` | The outbound engine: owns the `BridgeClient` and everything the Indigo callbacks *mean* for it. The client exists **only** while the allow-list is non-empty (XG5) and starts/stops on the dialog's empty↔non-empty transitions — and since E7 so does the **bridge LaunchAgent**, through injected `agent_start`/`agent_stop`/`agent_diagnose` seams: started before the client on empty→non-empty, stopped **after** the un-export has landed on non-empty→empty, and never stopped by a session that did not start it. The §5 pairing events (`fabrics_changed`/`commissioned`/`decommissioned`/`window_closed`) are consumed here since E6 — they were emitted by the node from E5 and read by nobody. PRD §5.5's `exportEnabled` switch fails **open** (absent or null means on, the opposite of the controller's attestation flag) and turning it off deliberately does **not** un-export: it drops the socket and stops the agent, leaving accessories paired-but-unavailable. The attach endpoint provider **re-runs `export_catalog.classify` on every attach** — the store is a past user declaration, not a guard — and skips-with-warning anything deleted/excluded/re-typed or carrying a role this build has no handler for — which since E4 means only an allow-list written by a *newer* plugin (an unknown role fails the *whole* attach). State pushes are fire-and-forget onto the loop; `on_command` dispatches `indigo.*` from the loop thread, the same discipline `device_sync.apply_states` already uses. **`_spec_for` sets `EndpointSpec.battery` from `getattr(dev, "batteryLevel", None) is not None`** (issue #220) — evidence, not opinion, and deliberately the opposite test from `battery_percent`'s: presence counts a literal 0 (the device factually has the attribute), the *value* 0 is suppressed. `device_updated` checks battery presence before the rename branch and routes a **gain** through `upsert()` (a full spec, so the node recreates the endpoint with PowerSource — a `set_state` against the live endpoint would be refused by the node's own §4.1 guard, added because of the measured trap); a **loss** does nothing special, since the node's cluster set is monotonic | +| `export_bridge.py` | The outbound engine: owns the `BridgeClient` and everything the Indigo callbacks *mean* for it. The client exists **only** while the allow-list is non-empty (XG5) and starts/stops on the dialog's empty↔non-empty transitions — and since E7 so does the **bridge LaunchAgent**, through injected `agent_start`/`agent_stop`/`agent_diagnose` seams: started before the client on empty→non-empty, stopped **after** the un-export has landed on non-empty→empty, and never stopped by a session that did not start it. The §5 pairing events (`fabrics_changed`/`commissioned`/`decommissioned`/`window_closed`) are consumed here since E6 — they were emitted by the node from E5 and read by nobody. PRD §5.5's `exportEnabled` switch fails **open** (absent or null means on, the opposite of the controller's attestation flag) and turning it off deliberately does **not** un-export: it drops the socket and stops the agent, leaving accessories paired-but-unavailable. The attach endpoint provider **re-runs `export_catalog.classify` on every attach** — the store is a past user declaration, not a guard — and skips-with-warning anything deleted/excluded/re-typed or carrying a role this build has no handler for — which since E4 means only an allow-list written by a *newer* plugin (an unknown role fails the *whole* attach). **Since issue #274, a `dev is None` skip is not just a warning.** `_spec_for` is the ONE place this method can tell "the Indigo device is gone" from every other skip reason, all of which mean the device still exists — so it alone queues the device id in `_confirmed_deleted`, drained by `_purge_confirmed_deleted` (called from `_on_attached`, the first moment the wire can carry a command on a fresh connection) into `self._store.remove()` plus `remove(device_id, permanent=True)`. This closes the one gap `deviceDeleted` cannot: a deletion Indigo reported while the plugin was not running to hear it. Every other skip reason leaves the entry alone, on purpose — see ADR-0015. State pushes are fire-and-forget onto the loop; `on_command` dispatches `indigo.*` from the loop thread, the same discipline `device_sync.apply_states` already uses. **`_spec_for` sets `EndpointSpec.battery` from `getattr(dev, "batteryLevel", None) is not None`** (issue #220) — evidence, not opinion, and deliberately the opposite test from `battery_percent`'s: presence counts a literal 0 (the device factually has the attribute), the *value* 0 is suppressed. `device_updated` checks battery presence before the rename branch and routes a **gain** through `upsert()` (a full spec, so the node recreates the endpoint with PowerSource — a `set_state` against the live endpoint would be refused by the node's own §4.1 guard, added because of the measured trap); a **loss** does nothing special, since the node's cluster set is monotonic | | `bridge_health.py` | The standing `matterBridgeHealth` device and everything written to it — device resolve/create, the §4.3 subscription-churn verdict, session-hygiene reporting, the fabric-slot readout, and the periodic `health_tick`. Split out of `export_bridge.py` (B1-B4 refactor round): it was the only one of that class's five bands to read and write an Indigo device directly, and at 414 lines the cleanest cut. Holds a BACK-REFERENCE to its `ExportBridge` rather than copies of what it shares with it — `_halted_reported`, `_recovery_reported` and `_disconnect_ticks` are once-per-streak latches, and a second copy would turn one notice into two. The six health-only latches moved wholly here; those three stayed wholly there. `health_tick`/`note_fabrics` remain on `ExportBridge` as delegators because `plugin.py` and `pairing_menu_mixin.py` call them by name | | `node_resolver.py` | `NodeResolver` — the Node/npm toolchain-resolution band, extracted out of `LaunchAgent` (B3 refactor). Measured before the cut: this band has **zero** outbound calls into the rest of `LaunchAgent`, only **three** inbound call sites (`LaunchAgent.__init__` → `_resolve_npx`; `install()` → `_node_version`/`_record_install_node`; `ensure_installed()` → `abi_warning`), and only **one** attribute (`node_path`) the rest of the class also needs — which is what made it the one band worth splitting on its own. Resolves `npx`/`node` (Homebrew → nvm → an explicit `nodeBinDir` pin → bare PATH), validates a pin actually runs a new-enough node (issue #101), and owns the `.indigo-node` install-stamp read/write (`_install_stamp_path`/`_read_install_node_major`/`_record_install_node`) used by `abi_warning`'s ABI-mismatch check. Each agent gets its own `NodeResolver` instance, constructed with the SAME `project_dir` as the owning `LaunchAgent` — that is what keeps the install stamp shared across both agents, unchanged by the split. `LaunchAgent` still exposes `node_path`/`npx_path` (mirrored at `__init__`) and thin delegators for `abi_warning`/`_install_stamp_path`/`_read_install_node_major`, since callers and tests reach those on the agent itself. Must not import `plugin.py`, any mixin, or matter.js anything (ADR-0006) | | `launch_agent.py` | Generic launchd LaunchAgent machinery (plist authoring, applied-plist digest, launchctl control, orphan/EADDRINUSE reaping, install/uninstall, bootstrap-verification state machine), driven by a frozen `AgentSpec` that carries one agent's identity. Extracted so the Matter **bridge node** can be a second agent without duplicating it (PRD-indigo-matter-export §4.2 / XOQ3). Since E7 `remove_package` is **per package** (`npm uninstall `, falling back to deleting only `node_modules/`) — it used to rmtree the shared `node_modules` and delete `package-lock.json`, which with two agents took the sibling's package out from under a still-loaded job. The `.indigo-node` install stamp stays deliberately **shared** (one node runs both agents). **No longer holds** Node/npm toolchain resolution — that band moved to `node_resolver.py` (`NodeResolver`, see above). The other five concerns named here stay together deliberately: measured at **48 cross-band calls**, most bands mutually dependent (`plist` alone calls `proc` and vice versa 10 times), so composing them into separate collaborators would add back-references in nearly every direction — the same coupling, plus indirection, not less of either | @@ -64,7 +64,7 @@ matter.js is imported (workspace ADR-0006). Its wire contract is `docs/BRIDGE_PR |---|---| | `main.ts` | Entry point: arg parsing, identity load, ordered SIGTERM shutdown. Exits **0** on a clean stop (launchd's `KeepAlive SuccessfulExit:false` reads the number) — `ServerNode.erase()` leaves a ref'd timer `close()` never clears, so a clean stop after a factory reset used to exit 1. An unusable `identity.json` is moved aside to `identity.json.unreadable-` and a replacement is minted **in memory only** — never written over the `SerialNumber`/`UniqueID` every paired ecosystem knows | | `node.ts` | The `ServerNode` + aggregator, the PRD §7 refuse-to-start decision, §3.9–§3.11, and the §5 event sinks | -| `endpoint-map.ts` | `endpoint-map.json` — the persisted `UniqueID → {number, role, label}` map (schema v2 since issue #141; v1 numbers-only files are migrated in place, never discarded) and its drift detector (PRD §4.3). Since #141 it is also what `node.ts` **restores the endpoint set from before `server.start()`**, so the bridge is never online with an empty aggregator — an empty `PartsList` made Apple re-create every accessory in the bridge's own room on every restart. **It does not allocate anything: matter.js owns the numbers**, keyed on `Endpoint.id` in its own store; this file is the independent *witness*, so a lost/reset matter.js storage becomes a log line instead of silently duplicating every accessory. Lives OUTSIDE matter.js's storage context on purpose (§3.10's reset wipes that). Report-only — drift is never repaired, or the next pass would call the same fault clean. A **commissioned bridge with no map at all bootstraps** a baseline from matter.js's own persisted numbers and serves (every pre-E5 install is in that state); only a *present-but-unreadable* map refuses. `refuseReasonFor` is a pure function with no matter.js import, because the case that matters most cannot be reached in a test without real hardware. **`battery`** (issue #220) joins `numberVoid`/`orphaned` as a third present-`true`-or-absent-never-`false` marker: set add-only the first time a live entry is seen with PowerSource, never cleared — the witness has to be as monotonic as the wire rule it mirrors (§4.1), or restore-on-start would rebuild an accessory that has a battery without the cluster it actually carries | +| `endpoint-map.ts` | `endpoint-map.json` — the persisted `UniqueID → {number, role, label}` map (schema v2 since issue #141; v1 numbers-only files are migrated in place, never discarded) and its drift detector (PRD §4.3). Since #141 it is also what `node.ts` **restores the endpoint set from before `server.start()`**, so the bridge is never online with an empty aggregator — an empty `PartsList` made Apple re-create every accessory in the bridge's own room on every restart. **It does not allocate anything: matter.js owns the numbers**, keyed on `Endpoint.id` in its own store; this file is the independent *witness*, so a lost/reset matter.js storage becomes a log line instead of silently duplicating every accessory. Lives OUTSIDE matter.js's storage context on purpose (§3.10's reset wipes that). Report-only — drift is never repaired, or the next pass would call the same fault clean. A **commissioned bridge with no map at all bootstraps** a baseline from matter.js's own persisted numbers and serves (every pre-E5 install is in that state); only a *present-but-unreadable* map refuses. `refuseReasonFor` is a pure function with no matter.js import, because the case that matters most cannot be reached in a test without real hardware. **`battery`** (issue #220) joins `numberVoid`/`orphaned` as a third present-`true`-or-absent-never-`false` marker: set add-only the first time a live entry is seen with PowerSource, never cleared — the witness has to be as monotonic as the wire rule it mirrors (§4.1), or restore-on-start would rebuild an accessory that has a battery without the cluster it actually carries. **`orphaned` no longer means "kept forever" (issue #274, ADR-0015).** `forget()` — the ordinary `attach`-reconcile removal path — is unchanged: it still marks an entry `orphaned` and keeps its number/role/label, because a full reconcile cannot tell a deliberate departure from a device that merely failed classification this one pass. But a device the PLUGIN has confirmed is gone for good — deleted, or deliberately un-exported — now reaches the new `destroy()` instead, via `node.ts`'s `removeEndpoint(id, permanent: true)`: the record is deleted outright, not orphaned, so a destroyed identity is invisible to both `restorable()` and `orphans()`/`list_orphans` from that moment on. `destroy()` is the one mutation here that discards a `number`; that is safe because this file was never what stopped a retired number being reissued — matter.js's own persisted allocation for the `Endpoint.id`, a store this file never touches, is what actually does that | | `storage.ts` | `identity.json`: install id, passcode, discriminator, and the `commissionedAt` witness for §7's "storage missing but previously commissioned". Atomic temp-plus-`rename` writes; witness writes report whether they landed | | `registry.ts` / `endpoints.ts` | The live endpoint set and one Matter device-type factory per §4.2 role. Every bridged child publishes the **full** Bridged Device Basic Information identity (`BridgedIdentity`: vendor name/id, product name, hardware + software versions, plus the per-accessory label/serial/uniqueId/reachable) — all optional in the cluster at 0.17.8, all populated, and the same values the root node's `BasicInformation` carries so an ecosystem is never shown two answers. **PowerSource (issue #220)** is composed onto any role via `deviceTypeFor`'s second `.with(PowerSourceServer.with("Battery"))` when `EndpointSpec.battery` is set — role-independent by construction (`splitBattery` peels `batteryLevel` off `states` before any role's own `statePatch` sees it, so the key never needed fifteen per-role additions). `BATTERY_INITIAL` seeds `batPercentRemaining: null` unconditionally: an endpoint built without that key never gets the attribute into `attributeList` at all (measured against 0.17.8), so a later write would succeed into an attribute no controller could ever read. A battery **gain** on an existing endpoint recreates (PowerSource can only be declared at construction); a **loss** is an ordinary update — the live cluster set is monotonic (§4.1) | | `ws-server.ts` | The loopback protocol server (§1–§3). Holds an un-attached socket OPEN while the node is refusing — it is the client's only route to the §3.11 rebuild | diff --git a/docs/BRIDGE_PROTOCOL.md b/docs/BRIDGE_PROTOCOL.md index 47fce5c..62cb62c 100644 --- a/docs/BRIDGE_PROTOCOL.md +++ b/docs/BRIDGE_PROTOCOL.md @@ -388,11 +388,45 @@ caller to guess why an apparently ordinary upsert failed. {"command": "remove_endpoint", "args": {"indigoDeviceId": 123456789}} ``` -Removes the child endpoint (`endpoint.close()`); the persisted endpoint-number -allocation is **retained** so re-adding the same device restores the same -number. Idempotent — removing an absent endpoint succeeds with -`{"removed": false}`; a live removal returns `{"removed": true}`. Bulk -removals are paced ~100ms apart by the node. +Removes the child endpoint (`endpoint.close()`). Idempotent — removing an +absent endpoint succeeds with `{"removed": false}`; a live removal returns +`{"removed": true}`. Bulk removals are paced ~100ms apart by the node. + +**`permanent` (issue #274, default `false`) decides what happens to the +endpoint-map record, not just the live endpoint.** Omitted or `false` keeps +the behaviour every caller had before this flag existed: the persisted +endpoint-number allocation is **retained** and the entry is marked +`orphaned` (§4.3's `endpoint-map.json` subsection) — re-adding the same +device, or re-adopting the identity onto a different one, restores the same +number. `true` is the confirmed-gone declaration: the Indigo device has been +deleted, or the user has deliberately un-exported it. The node then +**destroys** the endpoint-map record outright — no number kept, no +`orphaned` entry, nothing offered to `list_orphans` — because ADR-0015 +retired the premise that a departed accessory is worth holding open for a +re-adopt that will never come, once the driving device itself is gone. + +Only a caller certain the device is not coming back may pass `true`. The +plugin passes it from exactly three places: `deviceDeleted` (a live +deletion), the export dialog's "Remove" (a deliberate un-export), and a +sweep in `_on_attached` that closes the one gap those two cannot — a device +deleted from Indigo while the plugin was not running to hear about it (§3.1 +above already re-classifies every allow-list entry on each attach; a device +missing there is exactly this case, `export_bridge.py`'s `_spec_for`). The +plugin's own two-command role-change/re-adopt sequence (`replace()`, which +sends this `remove_endpoint` and a paired `upsert_endpoint` for the same or +a related identity) never passes `true` — that removal is deliberately soft, +because the very next command may retroactively mark it `supersededBy` +(§3.1's supersede accounting) or the map may still offer it as an ordinary +re-adopt candidate. + +**`attach`'s own reconcile never destroys.** A full reconcile (§3.1) cannot +tell "the plugin dropped this on purpose" from "this device exists but +failed classification this one pass" — a dependent plugin not yet started, +a role this build does not know — so any identity that merely falls out of +an `attach`'s desired set keeps the pre-#274 soft/orphan treatment +regardless of `permanent`. Destroying is only ever a `remove_endpoint` +decision, made with the device's existence already confirmed on the plugin +side. ### 3.4 `set_state` @@ -601,6 +635,15 @@ recorded, not necessarily newest-orphan-first): old-role accessory under a number every paired ecosystem has already processed a removal for. +**Since issue #274 (ADR-0015), an ordinary departure no longer lands here at +all.** A device deletion or a deliberate un-export now goes through +`remove_endpoint`'s `permanent: true` (§3.3), which destroys the record +instead of orphaning it — see the `endpoint-map.json` subsection above. Only +a device that fell out of an `attach`'s desired set without the plugin ever +confirming why (§3.1's own reconcile never destroys) still produces an +ordinary orphan, so a healthy bridge's `list_orphans` is expected to answer +empty far more often than before this issue. + **Not a recovery command.** `list_orphans` requires an ordinary `attach` first, same as any other §3 command — it is deliberately absent from §1.1's recovery trio (`get_status`, `get_pairing`, `rebuild_endpoint_map`): a node refusing @@ -1189,6 +1232,23 @@ removal marks an entry: absence from the live set proves nothing (a node that never attached has an empty one), so a factory reset, a seed and an entry this build cannot rebuild all leave the map alone. +**Destroying an entry (issue #274, ADR-0015) is the alternative to +orphaning it — number, role, label and every other field, gone.** A +`remove_endpoint` carrying `permanent: true` (§3.3) — the driving device is +confirmed deleted, or deliberately un-exported — deletes the record from +`endpoints` outright rather than marking it `orphaned`. Unlike every mutation +above, this is the one place a record's `number` is not kept: the guarantee +that a retired number is never reissued (ADR-0010, `numberVoid`'s and +`orphaned`'s own comments above) still holds, because it was never this file +that enforced it — matter.js's own persisted allocation for the retired +`Endpoint.id`, a separate store this file never touches, is what actually +prevents reuse, and it is untouched by deleting this witness of it. What is +lost is re-adopt: a destroyed identity is invisible to both `restorable()` +and `orphans()`/§3.12 from the moment it is destroyed — there is nothing left +to rebuild or to offer the picker. `attach`'s own reconcile never destroys +(see §3.1 and §3.3 above); only an explicit `remove_endpoint` with +`permanent: true` does. + `drift` is populated by every operation that can change the live endpoint set — `attach`/`reconcile`, `upsert_endpoint` and `remove_endpoint` — and **on the first such operation**, not "at startup": before one has run there are no live diff --git a/docs/adr/0015-a-confirmed-deletion-destroys-the-accessory-no-retention.md b/docs/adr/0015-a-confirmed-deletion-destroys-the-accessory-no-retention.md new file mode 100644 index 0000000..6277042 --- /dev/null +++ b/docs/adr/0015-a-confirmed-deletion-destroys-the-accessory-no-retention.md @@ -0,0 +1,245 @@ +--- +parent: Decisions +nav_order: 15 +title: "ADR-0015: A confirmed device deletion destroys the accessory — no orphan retention, no re-adopt" + +status: "accepted" +date: 2026-08-26 +decision-makers: solo (Simon) +consulted: none +informed: none +--- +# ADR-0015: A confirmed device deletion destroys the accessory — no orphan retention, no re-adopt + +## Context and Problem Statement + +Since issue #219 (ADR-0010), an endpoint that leaves the plugin's export list +is **orphaned**, not deleted: `endpoint-map.json` keeps its number, role and +label forever, so a re-adopt UI can hand the identity to a recreated device +later. #273 set out to measure what that retention costs and #274 asked +whether it is still worth it. Should a **confirmed** device deletion still +go through that same soft, indefinite-retention path, or should it destroy +the accessory outright? + +### The evidence that motivated this turned out to be wrong, and that matters + +#273 originally reported nine orphaned identities as **live published +endpoints** — duplicate names, dead accessories, inflated counts in every +paired ecosystem. That finding was **retracted on the same issue**: the +`root.parts.aggregator.parts.*` storage keys #273 read as "still published" +are matter.js's own leftover storage for endpoints that are no longer +instantiated, not evidence of one. A direct count at a bridge restart showed +57 endpoints instantiated against 57 exported identities — every orphan +absent, no duplicates in any ecosystem. **Orphaning an endpoint already +stops it being published; it always did.** The "nine zombie endpoints" +premise this ADR was expected to cite is false, and #274's own follow-up +comment says so in as many words: *"the original retention decision was +defensible after all: an orphan really is inert bookkeeping."* + +So Option C from #274 (stop publishing orphaned endpoints) is moot — nothing +publishes them. This ADR is **not** about that. What survived the +retraction, and is the actual reason for this decision, is #274's second +question: **is the deleted-device re-adopt case worth what it costs to +support**, independent of whether an orphan is visible? Issue #246's +migrate — retargeting a *live* device onto an existing identity — already +covers the workflow people actually reach for. Re-adopt's remaining scope is +narrower: inheriting an identity onto a device that did not exist at the +moment the original was removed. That is real surface area — ADR-0010's +`publishedAs`/`deviceId` split, `orphaned`/`orphanedAt`/`supersededBy` +bookkeeping, the PR5 re-adopt dialogs, `list_orphans` — kept alive +indefinitely for a workflow with no measured cost to *not* having, once the +"it's actively harmful" premise is gone. + +## Decision Drivers + +* **Simplicity, once the cost argument for keeping it evaporated.** An orphan + being "free" was the reason retention won the original #219 design debate; + now that it is confirmed free either way, the question is purely whether + the re-adopt machinery earns its keep, and #246 migrate already serves the + workflow that matters. +* **A confirmed deletion is unambiguous.** Indigo tells the plugin a device + no longer exists in `indigo.devices`. There is no future in which that + specific device comes back — a "recreated" device is a NEW Indigo id from + the moment it is recreated, which is precisely the case re-adopt exists + for and precisely the case this ADR removes. +* **The destructive alternative must not punish an unrelated failure.** A + device that merely fails classification this one pass (a dependent plugin + not yet started, a role this build does not know) is not deleted — it must + not be treated as if it were. Conflating "confirmed gone" with "could not + classify this pass" would make a transient startup race permanently + destroy an accessory, which is a strictly worse failure than the orphan + being removed. + +## Considered Options + +* Destroy on confirmed deletion; leave `attach`'s ordinary reconcile + untouched (chosen) +* Keep indefinite orphan retention for every departure (status quo) +* Destroy on ANY departure from the live set, including an ordinary `attach` + reconcile that simply does not mention an identity this pass + +## Decision Outcome + +Chosen option: **destroy the endpoint-map record on a CONFIRMED departure, +leave everything else exactly as it was.** + +"Confirmed" is deliberately narrow and is decided in exactly one place: +`export_bridge.py`'s `_spec_for`, the only code that can tell "the Indigo +device does not exist" (`device_getter` returns `None`) from every other +reason a device might not be sendable this pass. Three call sites now reach +the wire's `remove_endpoint` with a new `permanent: true` flag once that +existence check (or an equally unambiguous deliberate action) has been made: + +1. `plugin.py`'s `deviceDeleted` — the device is gone, this instant. +2. `export_dialog_mixin.py`'s `exportRemove` — the user deliberately + un-exported it. Kept alive only through "Migrate an exported accessory…" + is the point: an un-export is as final as a deletion, not a temporary + suspension. +3. `export_bridge.py`'s new `_purge_confirmed_deleted` (drained from + `_on_attached`) — the gap #274 asked to close: a device deleted while the + plugin was NOT RUNNING to hear `deviceDeleted` fire. The next attach's + classify pass discovers it via the same existence check and destroys it + the first moment the wire can carry the command. + +`bridge-node`'s `EndpointMapStore.destroy()` is the new mutation these calls +reach: it deletes the record outright — no number, no role/label, no +`orphaned` marker — rather than `forget()`'s keep-everything-but-mark-it +behaviour. A destroyed identity is invisible to `restorable()` (nothing to +rebuild at the next node restart) and to `orphans()`/`list_orphans` (nothing +to offer a re-adopt picker), because there is no re-adopt to offer it to. + +**What does NOT change: `attach`'s own reconcile.** A full reconcile +(`node.ts`'s `forgetRemoved`, driven by the plugin's desired set) cannot +tell "the plugin dropped this on purpose" from "this device exists but +failed classification this one pass" — that is exactly the ambiguity the +decision driver above rules out guessing at. So an identity that merely +falls out of an `attach`'s desired set keeps the pre-#274 soft/orphan +treatment, unconditionally, regardless of why. Destroying is only ever a +`remove_endpoint(permanent: true)` decision, made with the device's status +already confirmed on the plugin side — never inferred from an attach diff. + +**This supersedes ADR-0010 IN PART**, specifically its "For AI agents" rule +*"DON'T: prune retired/orphaned endpoint-map records"* and the "Bad, because +retired records accumulate without limit… no pruning is implemented, +deliberately" consequence. ADR-0010's actual mechanism — `publishedAs` as a +plugin-owned field distinct from `indigoDeviceId`, and the generation-bump +supersession `replace()` uses for a role change — is **untouched**: that +machinery is orthogonal to device deletion, still runs through the SAME +soft, two-command `remove_endpoint`/`upsert_endpoint` sequence it always +did (see "Consequences" below), and this ADR does not reopen it. ADR-0011's +rekey mechanism is likewise unaffected — a rekey never removes an endpoint +at all. + +**This does not (yet) remove the re-adopt UI or `list_orphans`.** That is a +separate, larger change (tracked as the PR-B half of #274's work) — deleting +a dialog and a wire command is a bigger blast radius than changing what one +flag does, and is better reviewed on its own. Until then, `list_orphans` +simply answers emptier than before: an ordinary un-export or a confirmed +deletion no longer produces an entry for it to list. + +### Consequences + +* Good, because a confirmed deletion or un-export no longer leaves anything + behind to reason about, list, or accidentally re-adopt onto the wrong + device. +* Good, because the destructive path is opt-in per call (`permanent`, + defaulting `false`) and reached from exactly three call sites the plugin + controls — `attach`'s reconcile, and `replace()`'s two-command + supersede/re-adopt sequence, are structurally unable to trigger it. +* Good, because the number-reissue guarantee (ADR-0010's actual safety + property) does not depend on this file at all: matter.js's own persisted + allocation for the retired `Endpoint.id` is what prevents reuse, not our + witness of it, so destroying our witness is safe. +* Bad, because the re-adopt picker (`list_orphans`, the PR5 dialogs) is now + reachable only through the narrowing set of departures that still go + through `attach`'s ordinary reconcile without ever being confirmed — + functionally close to dead code until PR-B removes it outright. Left in + place deliberately, to keep this change to a behaviour change rather than + a UI removal. +* Neutral, because `mass_removal_refused` (§3.1) is untouched, and does not + need to change: it guards `attach`'s reconcile, which this ADR does not + touch, and every destructive call this ADR adds is a per-device + `remove_endpoint` outside that guard's scope — exactly like the + `deviceDeleted` path that already existed before this issue. Destroying + rather than orphaning makes a wrongly-issued `permanent: true` more + costly, not the mass-removal guard less relevant; nothing here gives a + single bad actor a way to reach many devices at once that it did not + already have. + +### Confirmation + +`bridge-node/test/endpoint-map.test.ts` (`EndpointMapStore.destroy`), +`bridge-node/test/reconcile.test.ts` (`parsePermanentRemoval`), +`bridge-node/test/restore.test.ts` (issue #274 describe block: the wire +round trip, the default-false regression guard, idempotency, malformed +input), `tests/test_export_bridge.py` (`TestConfirmedDeletedSweep`), +`tests/test_export_wiring.py` (`deviceDeleted`/`exportRemove` now assert +`permanent=True`), `tests/test_bridge_protocol_frames.py`/ +`tests/test_bridge_client.py` (the new `remove_endpoint_permanent` golden +frame). `npm test` (bridge-node) and `pytest` (plugin, both Python 3.11 and +3.13) green; `pylint` unchanged. + +## Pros and Cons of the Options + +### Destroy on confirmed deletion; leave `attach`'s ordinary reconcile untouched (chosen) + +* Good, because it needs no new way to tell "confirmed" from "not sure" at + the point the destructive decision is made — that distinction already + exists, exactly once, in `_spec_for`. +* Good, because it adds no new wire-level ambiguity: `remove_endpoint` + already existed, `permanent` is additive and defaults to today's + behaviour. +* Neutral, because it leaves the re-adopt UI pointing at a list that will + usually be empty — acceptable as an interim state, not a final one. + +### Keep indefinite orphan retention for every departure (status quo) + +* Good, because it is what shipped and measured (harmlessly, per the + retraction) — no regression risk. +* Bad, because it keeps ADR-0010's full re-adopt machinery alive + indefinitely for a workflow #246 migrate already covers for the case that + matters (a live replacement device), for no longer-current reason. + +### Destroy on ANY departure from the live set, including an ordinary attach reconcile + +* Good, because it would be the simplest single rule: nothing survives being + un-exported, however it happens. +* Bad, because it collapses "the plugin confirmed this device is gone" and + "this device exists but failed classification this one pass" into the + same action — precisely the failure mode #274 raised as the reason NOT to + do this. A dependent plugin's slow startup would then permanently destroy + every accessory it drives, self-inflicted, on a bridge restart it had + nothing to do with. + +## More Information + +Issues: [#273](https://github.com/simons-plugins/indigo-matter/issues/273) +(the retracted "nine zombie endpoints" measurement — read the whole thread, +not just the title), [#274](https://github.com/simons-plugins/indigo-matter/issues/274) +(this decision, including its own retraction and the surviving "is re-adopt +worth its machinery" question). Related, unaffected: ADR-0010 (superseded +IN PART — the `publishedAs` split and role-change generation bump stand), +ADR-0011 (rekey — orthogonal, never removes an endpoint). Protocol shapes: +[`../BRIDGE_PROTOCOL.md`](../BRIDGE_PROTOCOL.md) §3.3 (`remove_endpoint`'s +`permanent` flag), the `endpoint-map.json` subsection ("Destroying an +entry"), §3.12 (`list_orphans`). + +## For AI agents +- DO: treat a confirmed device deletion or a deliberate un-export as + PERMANENT — `remove_endpoint(permanent: true)`, `EndpointMapStore.destroy()` + — never as something to retain for a future re-adopt. +- DO: keep the existence check (`device_getter(id) is None`) as the ONLY + place that decides "confirmed gone" on the plugin side. Do not infer it + from a classify failure, a role mismatch, or any other skip reason. +- DON'T: make `attach`'s ordinary reconcile (`node.ts`'s `forgetRemoved` + without `hard: true`) destroy anything. It cannot tell a deliberate + departure from a transient classify failure, and guessing wrong there + destroys an accessory over a startup race. +- DON'T: add a "confirmed after N misses" or grace-period heuristic to paper + over the point above. The existence check is definitive the first time it + is made; there is nothing to wait for. +- DON'T: touch `mass_removal_refused` (§3.1) as part of this — it guards + `attach`'s reconcile, which this ADR does not change. +- DON'T: reopen ADR-0010's `publishedAs`/`deviceId` split or its role-change + generation-bump mechanism. Only the "never prune an orphan" consequence is + superseded here. diff --git a/docs/adr/INDEX.md b/docs/adr/INDEX.md index 05990a1..8b06243 100644 --- a/docs/adr/INDEX.md +++ b/docs/adr/INDEX.md @@ -25,11 +25,12 @@ chosen), see [`../ARCHITECTURE.md`](../ARCHITECTURE.md). * [ADR-0007](0007-a-retired-everywhere-setting-keeps-its-state-flagged.md) - ADR-0007: A setting retired everywhere keeps its state, flagged — only a missing capability withdraws it (accepted; narrows ADR-0003) * [ADR-0008](0008-a-matter-node-is-an-indigo-device.md) - ADR-0008: A Matter node is an Indigo device, and the root of its endpoint devices' group (accepted; the root half is **superseded in part by ADR-0009**) * [ADR-0009](0009-indigo-groups-root-by-age-membership-is-the-deliverable.md) - ADR-0009: Indigo roots a device group by age, so membership is what this plugin delivers (accepted; supersedes in part ADR-0008) -* [ADR-0010](0010-a-published-accessory-identity-is-plugin-owned.md) - ADR-0010: A published accessory identity is a plugin-owned, defaulted field (accepted) +* [ADR-0010](0010-a-published-accessory-identity-is-plugin-owned.md) - ADR-0010: A published accessory identity is a plugin-owned, defaulted field (accepted; the "never prune an orphan" rule is **superseded in part by ADR-0015**) * [ADR-0011](0011-a-driving-device-change-is-a-reconcile-rekey.md) - ADR-0011: A driving-device change under a stable identity is a reconcile rekey (accepted) * [ADR-0012](0012-export-eligibility-can-be-a-user-declaration.md) - ADR-0012: For a device Indigo does not type, export eligibility is a user declaration (accepted; narrows ADR-0003) * [ADR-0013](0013-a-confirmed-commanded-colour-temperature-is-pushed-as-state.md) - ADR-0013: A confirmed, commanded colour-temperature is pushed as state (accepted) * [ADR-0014](0014-ct-physical-bounds-are-learned-declarations-only-seed-them.md) - ADR-0014: Colour-temperature physical bounds are learned from clamped echoes; declarations only seed them (accepted) +* [ADR-0015](0015-a-confirmed-deletion-destroys-the-accessory-no-retention.md) - ADR-0015: A confirmed device deletion destroys the accessory — no orphan retention, no re-adopt (accepted; supersedes in part ADR-0010) diff --git a/indigo-matter.indigoPlugin/Contents/Info.plist b/indigo-matter.indigoPlugin/Contents/Info.plist index 863f12f..ca0b8c8 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.28.9 + 2026.29.0 ServerApiVersion 3.6 diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py index fd14099..84643ea 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_client.py @@ -639,9 +639,18 @@ async def upsert_endpoint(self, spec: Any, timeout: float = DEFAULT_TIMEOUT) -> f"upsert_endpoint result carried no endpointNumber: {result!r}") return int(data["endpointNumber"]) - async def remove_endpoint(self, indigo_device_id: int, timeout: float = DEFAULT_TIMEOUT) -> bool: - """Remove one endpoint (§3.3). ``False`` means it was already absent.""" - result = await self._request_frame(self.proto.build_remove_endpoint(indigo_device_id), timeout) + async def remove_endpoint(self, indigo_device_id: int, *, permanent: bool = False, + timeout: float = DEFAULT_TIMEOUT) -> bool: + """Remove one endpoint (§3.3). ``False`` means it was already absent. + + ``permanent`` (issue #274) is the confirmed-gone declaration — see + ``bridge_protocol.BridgeProtocol.build_remove_endpoint``. Defaults to + ``False`` so `replace()`'s two-command supersede/re-adopt sequence, + which calls this directly rather than through `ExportBridge.remove`, + is unaffected. + """ + result = await self._request_frame( + self.proto.build_remove_endpoint(indigo_device_id, permanent=permanent), timeout) return bool((result or {}).get("removed", False)) async def set_state(self, indigo_device_id: int, states: dict) -> None: diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_protocol.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_protocol.py index f38ca4d..00f16a9 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_protocol.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/bridge_protocol.py @@ -88,6 +88,12 @@ ARG_INDIGO_DEVICE_ID = "indigoDeviceId" #: Issues #219/#240 — the accessory identity this device publishes as (§4.1). ARG_PUBLISHED_AS = "publishedAs" +#: Issue #274 — §3.3 opt-in: the driving device is confirmed gone for good +#: (deleted, or deliberately un-exported), so the node should DESTROY the +#: endpoint-map record rather than orphan it. Absent/``False`` is the +#: pre-#274 default every caller keeps unless it says otherwise — see +#: ``build_remove_endpoint``. +ARG_PERMANENT = "permanent" ARG_STATES = "states" ARG_REACHABLE = "reachable" ARG_DURATION_SECONDS = "durationSeconds" @@ -938,9 +944,25 @@ def build_upsert_endpoint(self, spec: Any, message_id: Optional[str] = None) -> """§3.2 — create or update one endpoint. Idempotent.""" return self.build_request(CMD_UPSERT_ENDPOINT, {ARG_ENDPOINT: _endpoint_wire(spec)}, message_id) - def build_remove_endpoint(self, indigo_device_id: int, message_id: Optional[str] = None) -> dict: - """§3.3 — remove one endpoint; the number allocation is retained.""" - return self.build_request(CMD_REMOVE_ENDPOINT, {ARG_INDIGO_DEVICE_ID: int(indigo_device_id)}, message_id) + def build_remove_endpoint(self, indigo_device_id: int, message_id: Optional[str] = None, *, + permanent: bool = False) -> dict: + """§3.3 — remove one endpoint. + + ``permanent`` (issue #274) is omitted unless ``True`` — the same + "absent means not asking for the destructive thing" convention + ``build_attach``'s ``intent`` uses — so every pre-#274 call, and the + two-command supersede/re-adopt half `export_bridge.replace()` sends, + keeps getting the SOFT removal: the number allocation is retained and + the entry stays orphaned (re-adopt evidence), exactly as before this + issue. Only a caller certain the device will never come back — a + confirmed Indigo device deletion, or a deliberate un-export — passes + ``True``, which tells the node to destroy the endpoint-map record + along with the live endpoint. + """ + args: dict = {ARG_INDIGO_DEVICE_ID: int(indigo_device_id)} + if permanent: + args[ARG_PERMANENT] = True + return self.build_request(CMD_REMOVE_ENDPOINT, args, message_id) def build_set_state(self, indigo_device_id: int, states: dict, message_id: Optional[str] = None) -> dict: diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py index 7a8cf77..3de6397 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.py @@ -242,6 +242,14 @@ def __init__(self, store, runtime, logger, prefs_getter: Callable[[], dict], *, #: Last reason each device was skipped by the provider, so a permanent #: skip (an unbridgeable role) logs once, not on every reconnect. self._skipped: dict[int, str] = {} + #: Issue #274 — device ids `_spec_for` found with NO Indigo device + #: behind them, queued here because the discovery happens inside + #: `endpoint_specs()`, which runs before the attach that is about to + #: carry them has completed — too early for an explicit + #: `remove_endpoint` to land (`_live_client` refuses it). Drained by + #: :meth:`_on_attached`, the first later moment the socket can + #: actually carry the command. + self._confirmed_deleted: set[int] = set() #: The reason set behind the last "NONE of them can be bridged" warning, #: so that state — which since #141 costs every accessory in every paired #: ecosystem — is announced once per cause rather than per reconnect. @@ -1062,6 +1070,20 @@ def _spec_for(self, entry) -> Optional[EndpointSpec]: device_id = entry.indigo_device_id dev = self._device_getter(device_id) if dev is None: + # Issue #274: this is the ONE unambiguous case — the Indigo + # device itself does not exist any more. Every OTHER reason this + # method returns `None` below (excluded, re-typed, unknown role, + # a states_for that raised) means the device still exists, so it + # must NOT be destroyed — only "leave it out of this attach's + # desired set", exactly as before this issue. Queued rather than + # removed here: `endpoint_specs()` runs before `attach` completes + # (often the very FIRST attach of a reconnect, before the client + # is marked attached at all), and an explicit `remove_endpoint` + # sent this early is refused with `not_attached` and silently + # dropped (`_live_client`'s documented no-op) — the confirmed + # fact would be lost. `_on_attached` drains this set once the + # socket can actually carry the command. + self._confirmed_deleted.add(device_id) return self._skip(device_id, "the Indigo device no longer exists") # The entry's OPTIONS are part of the question (ADR-0012): a custom # device is exportable BECAUSE the user declared which state is its @@ -1395,8 +1417,21 @@ def upsert(self, device_id: int) -> None: return self._fire(client.upsert_endpoint(spec), f"upsert_endpoint dev {device_id}") - def remove(self, device_id: int) -> None: - """Drop one endpoint (§3.3). Fire-and-forget; idempotent on the node.""" + def remove(self, device_id: int, *, permanent: bool = False) -> None: + """Drop one endpoint (§3.3). Fire-and-forget; idempotent on the node. + + ``permanent`` (issue #274) is passed straight through to the wire — + see ``bridge_client.BridgeClient.remove_endpoint`` and + ``bridge_protocol.BridgeProtocol.build_remove_endpoint``. Every + caller of THIS method (``deviceDeleted``, the export dialog's + "Remove", and the confirmed-deleted sweep in :meth:`_on_attached`) + knows unambiguously that the device is not coming back, so they pass + ``True``. `replace()`'s two-command supersede/re-adopt sequence does + NOT go through this method — it calls ``client.remove_endpoint`` + directly, at the default ``permanent=False``, because its removal is + deliberately followed by an ``upsert_endpoint`` for the same or a + related identity and must stay soft. + """ self._skipped.pop(device_id, None) self._update_failed.discard(device_id) self._stopped_keys.pop(device_id, None) @@ -1411,7 +1446,7 @@ def remove(self, device_id: int) -> None: client = self._live_client("remove_endpoint", device_id) if client is None: return - self._fire(client.remove_endpoint(device_id), f"remove_endpoint dev {device_id}") + self._fire(client.remove_endpoint(device_id, permanent=permanent), f"remove_endpoint dev {device_id}") def replace(self, device_id: int) -> None: """Remove one endpoint and add it back, because its **published @@ -1749,6 +1784,42 @@ def _push_commanded(self, command, _entry, handler, commanded: dict) -> None: self._fire(client.set_state(device_id, commanded), f"commanded set_state dev {device_id}") + def _purge_confirmed_deleted(self) -> None: + """Close the #274 gap: a device deleted while the plugin was DOWN. + + `deviceDeleted` already handles a live deletion — it removes the + allow-list entry and calls :meth:`remove` with ``permanent=True`` the + moment Indigo reports the deletion. The gap is a deletion Indigo + reported to nobody, because the plugin was not running to hear it: + the entry sits in the allow-list until the next attach's + `endpoint_specs()` re-classifies it and `_spec_for` finds no device + behind it. This is that discovery's other half — called from + :meth:`_on_attached`, the first moment after that attach the wire can + actually carry a `remove_endpoint` (see `_spec_for`'s comment for why + it cannot be sent any earlier). + + Mirrors `deviceDeleted` exactly: drop the allow-list entry, THEN tell + the bridge node, permanently. A store write failing here costs a + retry at the NEXT attach (the entry simply gets re-classified and + re-queued) rather than a silently lost removal. + """ + if not self._confirmed_deleted: + return + device_ids, self._confirmed_deleted = self._confirmed_deleted, set() + for device_id in sorted(device_ids): + try: + self._store.remove(device_id) + except Exception as exc: # pylint: disable=broad-except + self._logger.error( + "Matter bridge: removing device %s (deleted while the plugin was not running) " + "from the export list FAILED — %s", device_id, exc) + self._logger.exception(exc) + self._logger.info( + "Matter bridge: device %s no longer exists in Indigo — removing its Matter " + "accessory permanently (it was deleted while the plugin was not running)", + device_id) + self.remove(device_id, permanent=True) + # ------------------------------------------------------------------ # Client callbacks # ------------------------------------------------------------------ @@ -1764,6 +1835,7 @@ def _on_attached(self, status, carried_replace_all: bool = False) -> None: line asserting an un-export that never happened. The accessories stay in every ecosystem, and nothing is left that knows they should not. """ + self._purge_confirmed_deleted() self._disconnect_ticks = 0 self._poll_fail_ticks = 0 self._unreachable_reported = False diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/export_dialog_mixin.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_dialog_mixin.py index 0a4079a..9be4855 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/export_dialog_mixin.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/export_dialog_mixin.py @@ -1265,7 +1265,11 @@ def exportRemove(self, valuesDict, typeId="", devId=0): # allow-list stops the client out from under it. if self.export_bridge is not None: try: - self.export_bridge.remove(device_id) + # Issue #274: a deliberate un-export is as final as a device + # deletion — destroy the accessory rather than orphaning it + # for a re-adopt. Keeping it accessible only through + # "Migrate an exported accessory…" is the point. + self.export_bridge.remove(device_id, permanent=True) except Exception as exc: # pylint: disable=broad-except self.logger.exception(exc) self._exports_changed() # pylint: disable=no-member # lifecycle, stays in plugin.py diff --git a/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py b/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py index a232997..c0bac37 100644 --- a/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py +++ b/indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py @@ -577,7 +577,10 @@ def deviceDeleted(self, dev): # noqa: N802 self._export_callback_failed.discard(dev.id) try: if self.export_bridge is not None: - self.export_bridge.remove(dev.id) + # Issue #274: the device is confirmed gone — destroy the + # accessory outright rather than orphaning it for a re-adopt + # that no longer exists. + self.export_bridge.remove(dev.id, permanent=True) except Exception as exc: # noqa: BLE001 self.logger.exception(exc) finally: diff --git a/tests/fakes.py b/tests/fakes.py index a871de3..63c92ea 100644 --- a/tests/fakes.py +++ b/tests/fakes.py @@ -297,8 +297,13 @@ async def attach(self, endpoints=None, *, replace_all=False, timeout=None): async def upsert_endpoint(self, spec, timeout=None): return self._record("upsert_endpoint", spec) - async def remove_endpoint(self, device_id, timeout=None): - return self._record("remove_endpoint", device_id) + async def remove_endpoint(self, device_id, *, permanent=False, timeout=None): + # Issue #274 — mirrors the wire's own "omit unless True" convention + # (`bridge_protocol.build_remove_endpoint`): every call recorded + # before this flag existed still matches `("remove_endpoint", + # device_id)` exactly, and only a caller that opted into `permanent` + # sees the extra element. + return self._record("remove_endpoint", device_id, *(("permanent",) if permanent else ())) async def set_state(self, device_id, states): return self._record("set_state", device_id, dict(states)) diff --git a/tests/fixtures/bridge_protocol/frames.json b/tests/fixtures/bridge_protocol/frames.json index aee9365..f2572d2 100644 --- a/tests/fixtures/bridge_protocol/frames.json +++ b/tests/fixtures/bridge_protocol/frames.json @@ -639,6 +639,22 @@ } } }, + "remove_endpoint_permanent": { + "request": { + "message_id": "m78", + "command": "remove_endpoint", + "args": { + "indigoDeviceId": 123456789, + "permanent": true + } + }, + "response": { + "message_id": "m78", + "result": { + "removed": true + } + } + }, "set_state": { "request": { "message_id": "m17", diff --git a/tests/test_bridge_client.py b/tests/test_bridge_client.py index c6c5f67..51c3c41 100644 --- a/tests/test_bridge_client.py +++ b/tests/test_bridge_client.py @@ -305,6 +305,12 @@ def test_remove_endpoint(self, mock_logger): assert self._exchange(mock_logger, lambda c: c.remove_endpoint(123456789), "remove_endpoint", EXCHANGES) is True + def test_remove_endpoint_permanent(self, mock_logger): + # Issue #274 — the confirmed-gone declaration reaches the wire. + assert self._exchange( + mock_logger, lambda c: c.remove_endpoint(123456789, permanent=True), + "remove_endpoint_permanent", EXCHANGES) is True + def test_remove_endpoint_absent_is_not_an_error(self, mock_logger): # §3.3: idempotent — removing what is not there succeeds with removed=false. async def scenario(): diff --git a/tests/test_bridge_protocol_frames.py b/tests/test_bridge_protocol_frames.py index faf7091..0432297 100644 --- a/tests/test_bridge_protocol_frames.py +++ b/tests/test_bridge_protocol_frames.py @@ -87,7 +87,8 @@ def _build(proto: BridgeProtocol, request: dict): if command == bridge_protocol.CMD_UPSERT_ENDPOINT: return proto.build_upsert_endpoint(EndpointSpec.from_wire(args["endpoint"]), mid) if command == bridge_protocol.CMD_REMOVE_ENDPOINT: - return proto.build_remove_endpoint(args["indigoDeviceId"], mid) + return proto.build_remove_endpoint( + args["indigoDeviceId"], mid, permanent=args.get(bridge_protocol.ARG_PERMANENT, False)) if command == bridge_protocol.CMD_SET_STATE: return proto.build_set_state(args["indigoDeviceId"], args["states"], mid) if command == bridge_protocol.CMD_SET_REACHABLE: @@ -139,6 +140,16 @@ def test_factory_reset_preserves_endpoint_numbers_by_default(self): frame = BridgeProtocol().build_factory_reset(message_id="x") assert frame["args"][bridge_protocol.ARG_PRESERVE_ENDPOINT_NUMBERS] is True + def test_remove_endpoint_permanent_is_absent_unless_true(self): + # Issue #274 — every pre-#274 call, and `replace()`'s two-command + # supersede/re-adopt half, must keep getting the exact frame they + # always have: no destructive opt-in by default. + plain = BridgeProtocol().build_remove_endpoint(123456789, message_id="x") + assert bridge_protocol.ARG_PERMANENT not in plain["args"] + deliberate = BridgeProtocol().build_remove_endpoint(123456789, permanent=True, message_id="x") + assert deliberate["args"][bridge_protocol.ARG_PERMANENT] is True + assert deliberate == BY_NAME["remove_endpoint_permanent"]["request"] | {"message_id": "x"} + class TestResponses: """Every golden response parses into the normalised dataclasses.""" diff --git a/tests/test_export_bridge.py b/tests/test_export_bridge.py index b30a92c..a20b249 100644 --- a/tests/test_export_bridge.py +++ b/tests/test_export_bridge.py @@ -1137,6 +1137,77 @@ def test_a_failed_identity_change_names_the_half_finished_state( assert "OLD accessory may already have been removed" in warnings_of(mock_logger) +# --------------------------------------------------------------------------- +# Issue #274: a device deleted while the plugin was DOWN is destroyed, not +# orphaned, at the next classify pass — closing the gap `deviceDeleted` +# cannot, because nothing fired it. +# --------------------------------------------------------------------------- +def _attached_status(): + return bridge_protocol.parse_status({ + "commissioned": True, "fabrics": [], "endpointCount": 0, + "endpoints": [], "drift": [], "driftChecked": True, + }) + + +class TestConfirmedDeletedSweep: + def test_a_device_missing_at_classify_time_is_removed_from_the_allow_list_and_destroyed( + self, bridge_mod, mock_logger, devices): + # 999 is not in the `devices` fixture at all — exactly "the Indigo + # device no longer exists" per `_spec_for`. + h = Harness(bridge_mod, mock_logger, devices, [ExportEntry(999, "onOffLight")]) + h.start() # endpoint_specs() runs here; 999 is skipped, never sent + + assert h.store.get(999) is not None, "not yet — only _on_attached acts on it" + assert h.client.names() == [], "too early to send remove_endpoint: not attached yet" + + h.bridge._on_attached(_attached_status()) + + assert h.store.get(999) is None, "confirmed-gone entries leave the allow-list" + assert h.client.only("remove_endpoint") == ("remove_endpoint", 999, "permanent") + + def test_a_device_that_still_exists_but_fails_classify_is_left_alone( + self, bridge_mod, mock_logger, devices, unbridgeable_role): + # 101 IS in the `devices` fixture — classify fails for an unrelated + # reason (no handler for its role), which must NOT be treated the + # same as "the device is gone". + h = Harness(bridge_mod, mock_logger, devices, [ExportEntry(101, unbridgeable_role)]) + h.start() + + h.bridge._on_attached(_attached_status()) + + assert h.store.get(101) is not None, "still exists — must not be dropped from the allow-list" + assert "remove_endpoint" not in h.client.names(), "must not be destroyed for a classify failure" + + def test_draining_is_one_shot_per_discovery(self, bridge_mod, mock_logger, devices): + """A second `_on_attached` with nothing newly discovered sends nothing + more — the set is drained, not merely read.""" + h = Harness(bridge_mod, mock_logger, devices, [ExportEntry(999, "onOffLight")]) + h.start() + h.bridge._on_attached(_attached_status()) + assert h.client.names().count("remove_endpoint") == 1 + + h.bridge._on_attached(_attached_status()) + + assert h.client.names().count("remove_endpoint") == 1, "nothing left queued to re-send" + + def test_a_store_write_failure_still_reports_but_does_not_crash_the_attach( + self, bridge_mod, mock_logger, devices, monkeypatch): + h = Harness(bridge_mod, mock_logger, devices, [ExportEntry(999, "onOffLight")]) + h.start() + + def _fail_remove(device_id): + raise RuntimeError("prefs are read-only") + + monkeypatch.setattr(h.store, "remove", _fail_remove) + + h.bridge._on_attached(_attached_status()) # must not raise + + assert h.logger.error.called + # The wire is still told, even though the store write failed — the + # device is gone either way (mirrors `deviceDeleted`'s own ordering). + assert h.client.only("remove_endpoint") == ("remove_endpoint", 999, "permanent") + + # --------------------------------------------------------------------------- # The migrate nudge — a full mid-session attach (issue #246) # --------------------------------------------------------------------------- diff --git a/tests/test_export_wiring.py b/tests/test_export_wiring.py index dd57c8b..0d76844 100644 --- a/tests/test_export_wiring.py +++ b/tests/test_export_wiring.py @@ -335,7 +335,9 @@ def test_an_exported_device_leaves_the_list_and_the_bridge(self, plug): plug._exports_changed() plug.deviceDeleted(RelayDevice(101, "Study Plug")) assert plug.exports.ids() == frozenset() - plug.export_bridge.remove.assert_called_once_with(101) + # Issue #274 — a deletion is confirmed and permanent: no orphan, no + # re-adopt. + plug.export_bridge.remove.assert_called_once_with(101, permanent=True) assert plug._exported_ids == frozenset() def test_a_failed_store_write_still_removes_the_endpoint(self, plug, monkeypatch): @@ -345,7 +347,7 @@ def test_a_failed_store_write_still_removes_the_endpoint(self, plug, monkeypatch monkeypatch.setattr(plug.exports, "remove", Mock(side_effect=RuntimeError("prefs are read-only"))) plug.deviceDeleted(RelayDevice(101, "Study Plug")) - plug.export_bridge.remove.assert_called_once_with(101) + plug.export_bridge.remove.assert_called_once_with(101, permanent=True) assert plug.logger.error.called def test_a_raising_endpoint_removal_still_refreshes_the_cache(self, plug): @@ -454,7 +456,8 @@ def test_removing_an_export_removes_the_endpoint(self, plug): plug._exports_changed() plug.export_bridge.reset_mock() plug.exportRemove(_values(exportDevice="101"), "manageMatterExports") - plug.export_bridge.remove.assert_called_once_with(101) + # Issue #274 — a deliberate un-export is as final as a deletion. + plug.export_bridge.remove.assert_called_once_with(101, permanent=True) assert plug._exported_ids == frozenset() def test_removing_something_unexported_nudges_nothing(self, plug):