From d0c2f960987e48043453206c4ac8250f5bb05041 Mon Sep 17 00:00:00 2001 From: Simon Clark Date: Wed, 26 Aug 2026 00:54:51 +0100 Subject: [PATCH] feat(export): a deleted Indigo device destroys its Matter accessory (#274, PR A of 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simon's decision, 2026-08-25: when a user deletes an Indigo device, the Matter accessory goes with it. Keep an accessory by migrating it to the new device first (`menuMigrateExport`, which stays); delete it and you re-pair in Apple/Alexa, which takes a few minutes. #274 records that Simon proposed exactly this when orphan retention was designed and was argued down on "an orphan costs nothing to keep". #273 then measured what it costs: NINE live published endpoints on the reference rig, duplicate names, dead accessories and inflated endpoint counts in every ecosystem. An orphan is not free — it is an accessory every controller still sees. `deviceDeleted` already removed the allow-list entry and the endpoint, so a deletion Indigo witnessed was already handled. The gap this closes is a deletion Indigo reported to nobody because the plugin was not running: on the next attach, the endpoint whose device is gone is now destroyed rather than retained for re-adopt. **The existence check is the whole rule.** A device is gone when it is not in `indigo.devices` — nothing else. A device whose owning plugin is disabled, restarting or still loading STILL EXISTS, so it is never a delete; there is no transient-vs-permanent distinction to draw, no grace period and no retry bookkeeping, and none is implemented. `mass_removal_refused` is untouched and matters more now than it did: it is the only thing between a plugin bug that sends a short list and a wiped fabric, and removal is no longer reversible. `launch_agent.py`/`server_process.py` say "orphan" about orphaned launchd PROCESSES — unrelated, untouched. ADR-0015 records the decision and supersedes the retention rationale. BRIDGE_PROTOCOL and ARCHITECTURE updated. The shared golden fixture `tests/fixtures/bridge_protocol/frames.json` gains the permanent-removal frames; both suites read it and both pass, which is the point of that file. **Known scaffolding, deliberate:** this reached the behaviour additively — the node still orphans by default and a `permanent` wire flag opts out. PR B deletes the re-adopt UI, at which point orphaning has no consumer at all and comes out wholesale, taking the flag with it. Shipping it this way because the end state is identical and it gets the fix in today. 3891 passing on 3.11 and 3.13 (3882 + 9); bridge-node 697 (686 + 11); pylint 9.48, unchanged. Co-Authored-By: Claude Opus 5 (1M context) --- bridge-node/src/endpoint-map.ts | 47 ++++ bridge-node/src/node.ts | 34 ++- bridge-node/src/protocol.ts | 16 +- bridge-node/src/reconcile.ts | 25 ++ bridge-node/src/ws-server.ts | 3 +- bridge-node/test/endpoint-map.test.ts | 59 +++++ bridge-node/test/reconcile.test.ts | 17 ++ bridge-node/test/restore.test.ts | 113 ++++++++ docs/ARCHITECTURE.md | 4 +- docs/BRIDGE_PROTOCOL.md | 70 ++++- ...ion-destroys-the-accessory-no-retention.md | 245 ++++++++++++++++++ docs/adr/INDEX.md | 3 +- .../Contents/Info.plist | 2 +- .../Contents/Server Plugin/bridge_client.py | 15 +- .../Contents/Server Plugin/bridge_protocol.py | 28 +- .../Contents/Server Plugin/export_bridge.py | 78 +++++- .../Server Plugin/export_dialog_mixin.py | 6 +- .../Contents/Server Plugin/plugin.py | 5 +- tests/fakes.py | 9 +- tests/fixtures/bridge_protocol/frames.json | 16 ++ tests/test_bridge_client.py | 6 + tests/test_bridge_protocol_frames.py | 13 +- tests/test_export_bridge.py | 71 +++++ tests/test_export_wiring.py | 9 +- 24 files changed, 860 insertions(+), 34 deletions(-) create mode 100644 docs/adr/0015-a-confirmed-deletion-destroys-the-accessory-no-retention.md diff --git a/bridge-node/src/endpoint-map.ts b/bridge-node/src/endpoint-map.ts index 4abb1eef..570ebd7e 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 90896ad9..4d8f467e 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 1b2873e5..7fdcb58a 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 1cae1249..2b653385 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 e983288d..4b14fdec 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 a1de285a..fb9b5232 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 8033637a..66e15473 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 0b1a99df..eb56377e 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 1aac8aa4..0cb9cd87 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 47fce5ce..62cb62cd 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 00000000..6277042c --- /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 05990a18..8b062432 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 863f12f9..ca0b8c84 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 fd140999..84643ea9 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 f38ca4d2..00f16a94 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 7a8cf77e..3de6397c 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 0a4079a1..9be48551 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 a232997e..c0bac37f 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 a871de31..63c92ea3 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 aee93658..f2572d2b 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 c6c5f676..51c3c419 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 faf70913..04322977 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 b30a92c5..a20b2493 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 dd57c8bb..0d76844a 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):