[fabcoded]

The send_packet recovery trusts every association lookup except NOTUSED.

Problem

• Lookup miss on Koenkk's builds since 20230507: 0xFFFE + zeros, reads as PARENT. If the retry fails, recovery writes AssocAdd(0xFFFE, PARENT).
• Restore uses the pre-recovery address. If the device rejoined meanwhile, AssocAdd overwrites its new entry.

Fix

• 0xFFFE or non-child relation: route discovery and retry only, no Remove/Add (the 0xFFFE check matches zigbee-herdsman #685).
• Restore only if the host's address is unchanged and a fresh lookup still reports the device absent.
• A child the firmware holds at an address other than device.nwk is not dropped either: that cannot help a send to the host's address, and the restore would then not put it back (review of 2026-10-23).

Background: why host and firmware can disagree on a device's address (device lifecycle, the three cases, measurements): https://gist.github.com/fabcoded/e843e31b188079c6acde990f31e562bc

Tests: on @puddly's table model (de4f5a1, cherry-picked).

Also on this branch: puddly's ZGP startup fix (152fa2f) and the zigpy>=2.4.0 bump.

Not fixed: a rejoin between the second lookup and AssocAdd; a valid child is still removed on the first expiry.

Log: phantom entry (Koenkk 20250321)

15:08:06.320 UTIL.AssocGetWithAddress.Rsp(Device=Device(shortAddr=0xFFFE, addrIdx=0, nodeRelation=PARENT, ...))
15:08:06.321 UTIL.AssocRemove.Req(IEEE=84:2e:14:ff:fe:70:1b:9b)
15:08:08.131 UTIL.AssocAdd.Req(NWK=0xFFFE, IEEE=84:2e:14:ff:fe:70:1b:9b, NodeRelation=PARENT)

Refs #285. AI-assisted (Claude); I reviewed it and own it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.61%. Comparing base (34098a5) to head (dc4df65).
⚠️ Report is 2 commits behind head on dev.

Additional details and impacted files

@@           Coverage Diff           @@
##              dev     #286   +/-   ##
=======================================
  Coverage   98.60%   98.61%           
=======================================
  Files          43       43           
  Lines        3581     3601   +20     
=======================================
+ Hits         3531     3551   +20     
  Misses         50       50           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[puddly]

Regarding the comment:

Not fixed: a rejoin between the second lookup and AssocAdd; a valid child is still removed on the first expiry.

To trigger this, a device must re-join between the two commands running on the MCU. IMO, this just isn't really fixable without firmware changes. Switch to https://github.com/zigpy/ziggurat/ if you want an actively-developed and maintained Zigbee stack 😄.

[zigpy-review-bot]

The core fix is right and the new tests genuinely pin it. One narrow regression against dev, plus a few optional items.

Verified

Verified (7 checks)

• Full suite on the branch (8d1447a, zigpy 2.4.0): 401 passed.
• The branch is two commits behind dev (#284, #287). Merged dev in locally: clean merge, #287's reworded recovery comment survives, 407 passed.
• Control run: reverting only the two production guards while keeping the new tests fails 9 of the 15 new parametrizations (five not_a_child rows, four restore rows) — the tests are not vacuous.
• Mutation check on the restore guard: deleting just its current.Device.shortAddr == 0xFFFE arm fails exactly one row (koenkk_lookup), 14 pass.
• Device.devStatus retype to the new DeviceStatus bitmap is size-neutral: 29 bytes unaligned / 36 aligned, unchanged field widths, and Device.deserialize(b"\xff" * 100) still yields NOTUSED with every status bit set.
• 0xFFFE as the authoritative "absent" marker matches the firmware source quoted in Koenkk/zigbee-herdsman#685: packDev_t() writes INVALID_NODE_ADDR and nothing else for a NULL device, so every field after shortAddr is uninitialized memory. zigbee-herdsman's own expired-frame path guards on exactly that pair (nwkaddr !== 0xfffe && noderelation !== 255).
• Restricting the drop to the four child relations is stricter than zigbee-herdsman (which drops on any relation but NOTUSED there) and is the right direction.

Worth fixing

The restore guard conflates three different situations (application.py L1032).

device.nwk == removed.shortAddr is true only when the host and the firmware agree on the address. It is false in three different situations:

1. a join that arrived during the recovery — what the comment above it describes;
2. the host's address was already stale before the recovery;
3. the firmware's entry was already stale before the recovery.

Skipping the restore is right for 1 and 3. In case 2 it destroys a valid child entry that dev restored: entry at 0x1234 while the host still believes 0xABCD → the lookup returns a child relation, AssocRemove runs exactly as on dev, the retry fails, and the guard then skips the restore. Measured on this branch — entry at the host's address: removed=1 re-added=1, entry back in the table; entry at a different address: removed=1 re-added=0, empty table. The same probe against dev's logic re-adds it at 0x1234 in both cases.

How narrow it is: the expiry means the coordinator held the frame for the address the host used, so when the two disagree the frame was held for a different entry that happens to sit on the host's stale address — another child, or a #284-style phantom. A rejoin that keeps the device's address is invisible to the guard and to the snapshot alike, and restores the entry either way, as dev does. Not a blocker, and arguably a sub-case of the first-expiry removal you already list as unfixed — but the restore half is lost too, and the device then stays out of the table until it rejoins on its own.

What would make the guard say what it means: snapshot device.nwk before the AssocRemove is sent and compare the live value against that snapshot in the finally, instead of against removed.shortAddr. That detects an address change during the recovery, and nothing else. The placement matters — taking the snapshot after the AssocRemove round-trip instead loses a join whose announce is processed during it, and then re-adds the entry at the old address, which is the one case your guard does catch. Both verified against this branch: with the snapshot before the send, all 15 new parametrizations still pass (the two rejoin rows are caught by the snapshot, the re_associated rows by the fresh-lookup check), a join fired inside the AssocRemove handler gives re-added=0 and an empty table, and the stale-host probe goes back to re-added=1; with the snapshot after it, that same join gives re-added=1 with the entry back at the old address.

That still leaves 2 and 3 indistinguishable from the host's side, which the snapshot restores at the firmware's address. If you would rather not make that call at all: skip the AssocRemove when device.nwk != child_association.Device.shortAddr and fall through to the new route-discovery-and-retry branch. Dropping a child cannot fix a send to an address the firmware does not have for it, so there is nothing to lose by not dropping it, and then no entry is ever destroyed.

Optional

• Check shortAddr == 0xFFFE before nodeRelation == NOTUSED (application.py L976). Per the firmware source in Koenkk/zigbee-herdsman#685 only the first two bytes are written on a miss, so what the recovery does with a real lookup miss currently depends on uninitialized firmware memory: Koenkk's patched builds zero the buffer (PARENT → the new branch: route discovery and a retry), while an unpatched build whose buffer happens to hold 0xFF at that offset reads NOTUSED → the early raise, so the send fails on the first expiry with no retry. Testing the one field the firmware actually initializes first makes a miss behave the same everywhere. (The (None, None, False) row of test_request_recovery_assoc_not_a_child pins today's NOTUSED behaviour, so this is a deliberate choice — worth a word in the comment either way.)
• Make 0xFFFE plus garbage the model's default miss (conftest.py L1007). This answers the question left open in the resolved thread: yes, worth moving in. The current default (b"\xff" * 100 → shortAddr=0xFFFF, NOTUSED) is a shape no firmware returns — INVALID_NODE_ADDR is 0xFFFE, and the bytes after it are whatever was in the buffer. The comment on that line reads "INVALID_NODE_ADDR / NOTUSED", which 0xFFFF is not. Consequence today: the restore guard's 0xFFFE arm is pinned by a single row — deleting that arm fails only koenkk_lookup (1 failed, 14 passed), while the first guard's arm is pinned by the CHILD_RFD absent_relation row.
• The finally block can mask the DeliveryError (application.py L1033) — pre-existing, but there are two awaits in it now. ZNP.request raises TimeoutError from asyncio.timeout(CONF_SREQ_TIMEOUT) (15 s by default), and the finally runs while DeliveryError is propagating, so a timeout on the restore lookup or the AssocAdd surfaces as a bare TimeoutError that zigpy's callers do not expect. A try/except around the restore, logging the failure, keeps the original error.
• Nothing is logged on either new skip path. This misbehaviour was only diagnosable from an MT trace; a LOGGER.debug on "not a child / absent, retrying only" and on "not restoring, address changed" would make the next report self-explaining. zigbee-herdsman logs at both points (relation and address on its response-timeout path, the removal itself on the expired-frame path).
• 0xFFFE appears twice in code and once in a comment — a module constant next to CHILD_NODE_RELATIONS would read better.

Notes

• Not a request, just context for the first-expiry limitation in the description: zigbee-herdsman only enters this recovery on dataRequestAttempt >= 1, so it rediscovers the route and retries once before touching the table. It can afford that because it only gives up after five sends; with range(2) here the same gate would leave the removal with no send after it (remove → continue → loop exhausted → raise → restore), so it would need a third attempt to buy anything.
• With #287 merged, child aging is on for builds from 20230507 — the builds where this was reported. The comment above the recovery already says "off on older builds"; whether the drop should be skipped altogether when aging is enabled is the open half of #285.
• The description covers the recovery changes only; the ZGP endpoint fix in 152fa2f and the zigpy>=2.4.0 bump ride along on this branch, and the former is what moves test_endpoints to 3 endpoints and test_permit_join's TSN to 4. Worth a line in the body so the diff matches its description.

[zigpy-review-bot]

This is true only when the host and the firmware agree on the address, which lumps together three different situations where it is false: a join that arrived during the recovery (what the comment describes), a host address that was already stale, and a firmware entry that was already stale. Skipping the restore is right for the first and the third; in the second it destroys a valid child entry that dev restored.

Entry at 0x1234 while the host still believes 0xABCD: the lookup returns a child relation, AssocRemove runs exactly as on dev, the retry fails, and this check then skips the restore. Measured on this branch — entry at the host's address: removed=1 re-added=1, entry back in the table; entry at a different address: removed=1 re-added=0, empty table. The same probe against dev's logic re-adds it at 0x1234 in both cases.

It is narrow: the expiry means the coordinator held the frame for the address the host used, so when the two disagree the frame was held for a different entry sitting on the host's stale address — another child, or a #284-style phantom.

To make the check say what the comment means, snapshot device.nwk before the AssocRemove is sent and compare the live value against that snapshot here, rather than against removed.shortAddr — that detects an address change during the recovery and nothing else. The placement matters: snapshotting after the AssocRemove round-trip instead loses a join whose announce is processed during it, and then re-adds the entry at the old address — the one case the current check does catch. Verified against this branch's tests: with the snapshot before the send, all 15 new parametrizations still pass (the two rejoin rows caught by the snapshot, the re_associated rows by the fresh lookup below), a join fired inside the AssocRemove handler gives re-added=0 and an empty table, and the stale-host probe goes back to re-added=1.

Alternatively, skip the AssocRemove altogether when device.nwk != child_association.Device.shortAddr and fall through to the new route-discovery-and-retry branch — dropping a child cannot fix a send to an address the firmware does not have for it, so nothing is lost by not dropping it, and then no entry is ever destroyed.

[fabcoded]

Thanks, the restore finding is right. Fixed by not dropping the entry in the first place: if the lookup has the device at an address other than device.nwk, the recovery only rediscovers the route and retries, as for a miss or a non-child. The address comparison now detects a change during recovery rather than a pre-existing mismatch. New test test_request_recovery_assoc_other_address fails on 8d1447a.

The optional items I'd leave for a follow-up PR. The description now mentions puddly's ZGP fix and the zigpy bump. Why host and firmware can disagree on the address, with the device lifecycle and the measurements behind this fix: https://gist.github.com/fabcoded/e843e31b188079c6acde990f31e562bc

[zigpy-review-bot]

Worth testing this before the NOTUSED check above.

Per the firmware source quoted in Koenkk/zigbee-herdsman#685, packDev_t() writes only INVALID_NODE_ADDR for a missing device and leaves the rest of the buffer uninitialized, so nodeRelation on a miss is garbage: Koenkk's patched builds zero it, which reads as PARENT and lands here (route discovery and a retry), while an unpatched build whose buffer happens to hold 0xFF at that offset reads NOTUSED and hits the early raise instead — same condition, no retry.

Testing the one field the firmware actually initializes first makes a lookup miss behave the same on every build.

[fabcoded]

Agreed, for the follow-up PR: test shortAddr == 0xFFFE before NOTUSED, so a lookup miss behaves the same on every build; the (None, None, False) row changes with it.

[zigpy-review-bot]

Pre-existing, but there are two awaits in the finally now.

ZNP.request raises TimeoutError from asyncio.timeout(CONF_SREQ_TIMEOUT) (15 s by default), and this block runs while the DeliveryError is propagating — so a timeout on either the lookup or the AssocAdd surfaces as a bare TimeoutError rather than a delivery failure, which zigpy's callers do not expect.

Wrapping the restore in try/except and logging the failure keeps the original error.

[fabcoded]

Agreed, pre-existing but worth it; for the follow-up PR: wrap the restore in try/except with a log line, so the original DeliveryError survives a timeout in the finally.

[zigpy-review-bot]

Re the question you left open in the resolved thread — yes, worth making this the model's default miss.

INVALID_NODE_ADDR is 0xFFFE, so b"\xff" * 100 (shortAddr=0xFFFF, nodeRelation=NOTUSED) is a shape no firmware returns; per Koenkk/zigbee-herdsman#685 the firmware writes 0xFFFE and leaves the following bytes uninitialized. The comment here reads "INVALID_NODE_ADDR / NOTUSED", which 0xFFFF is not.

Consequence today: the restore guard's 0xFFFE arm is pinned by one row only — deleting just that arm fails test_request_recovery_assoc_restore[koenkk_lookup-…] and nothing else (1 failed, 14 passed). The first guard's arm is pinned by the CHILD_RFD absent_relation row.

[fabcoded]

Agreed, for the follow-up PR: 0xFFFE with deterministic garbage in the remaining fields as the model's default miss, and the comment corrected.

[fabcoded]

Agreed. dc4df65 addresses the pre-existing address mismatch; the race between the final lookup and AssocAdd remains listed under "Not fixed".