Skip to content

app_main: fix crash in OnSrpClientNotification on FailSafe revert #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
GuvHas merged 1 commit into from
Mar 20, 2026
Merged

app_main: fix crash in OnSrpClientNotification on FailSafe revert #43

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 41 additions & 13 deletions main/app_main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,18 +230,47 @@ void trySrpServiceAdd(otInstance *ot)
}
}

// Remove the _matter._tcp SRP service and host, instructing the OTBR to
// withdraw the record. Called when the CHIP fabric is removed so that stale
// operational records are not left in the OTBR and proxied to mDNS.
// Remove the _matter._tcp SRP service when the device is fully decommissioned.
// Must be called from the CHIP/OpenThread task.
//
// Two important constraints:
//
// 1. kFabricRemoved fires for uncommitted (FailSafe-reverted) pending fabric
// entries too, not only on true "Remove Device" decommissions. Only tear
// down the SRP service when ALL committed fabrics are gone.
//
// 2. otSrpClientRemoveHostAndServices() sends an SRP update and then triggers
// the CHIP SDK's OnSrpClientNotification callback. That callback crashes
// (null function pointer at GenericThreadStackManagerImpl_OpenThread.hpp:1310)
// when it receives a removed service that it never registered internally
// (because the ESP32 DNS-SD layer uses esp_mdns, not SRP). Use
// otSrpClientClearHostAndServices() instead — it discards local state
// immediately with no network message and no callback. The OTBR will hold
// the record until its SRP lease expires, which is acceptable: the record's
// instance name is fabric-specific, so a new commissioner sees the new name.
void srpServiceRemove(otInstance *ot)
{
if (!s_srp.added) return;

// Guard: only remove when NO committed fabric remains.
bool hasFabric = false;
for (const auto & f : chip::Server::GetInstance().GetFabricTable()) {
(void)f;
hasFabric = true;
break;
}
if (hasFabric) {
ESP_LOGI(kTag, "SRP: kFabricRemoved with committed fabric still present "
"(FailSafe revert?), keeping SRP record");
return;
}

s_srp.added = false;
// aRemoveKeyLease=false: keep the SRP key lease so re-registration is fast.
// aSendUnregister=true: send an SRP update to withdraw the record from OTBR.
otSrpClientRemoveHostAndServices(ot, false, true);
ESP_LOGI(kTag, "SRP: withdrew _matter._tcp service '%s'", s_srp.instanceName);
// Clear local SRP state without sending a network unregister message.
// Avoids triggering OnSrpClientNotification with our non-SDK service.
otSrpClientClearHostAndServices(ot);

@chatgpt-codex-connector chatgpt-codex-connector Bot Mar 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve SRP host info when clearing SRP services

Replacing otSrpClientRemoveHostAndServices() with otSrpClientClearHostAndServices() clears all host info in OpenThread, not just the service list. In this file, the host name is only configured once during startup (otSrpClientSetHostName() in app_main), and it is never re-set after srpServiceRemove(). After a real decommission event, the next recommission attempt in the same uptime can add a service object but lacks host info for SRP registration, so operational _matter._tcp advertisement may never come back until reboot.

Useful? React with 👍 / 👎.

ESP_LOGI(kTag, "SRP: cleared _matter._tcp service '%s' (decommissioned)",
s_srp.instanceName);
}

// OpenThread state-change callback: fires when the Thread role changes.
Expand Down Expand Up @@ -283,16 +312,15 @@ void app_event_cb(const ChipDeviceEvent *event, intptr_t arg)
break;

case DevEvt::kFabricRemoved:
// Fired when a controller removes this device (e.g. "Remove Device" in HA).
// Explicitly withdraw the SRP service from the OpenThread client so the
// OTBR stops proxying the now-invalid _matter._tcp record to mDNS.
// Without this, stale operational records remain in the OTBR until reboot
// or the next explicit SRP update, allowing discovery of an unreachable node.
// Fired on both true decommissions ("Remove Device" in HA) and when the
// FailSafe timer reverts an uncommitted pending fabric entry. srpServiceRemove()
// checks whether a committed fabric still exists before clearing the SRP record,
// so it is safe to call unconditionally here.
chip::DeviceLayer::PlatformMgr().ScheduleWork([](intptr_t) {
otInstance *ot = esp_openthread_get_instance();
if (ot) srpServiceRemove(ot);
}, 0);
ESP_LOGW(kTag, "Fabric removed — device decommissioned; re-commissioning required");
ESP_LOGW(kTag, "Fabric removed event (decommission or FailSafe revert)");
break;

case DevEvt::kCHIPoBLEAdvertisingChange:
Expand Down