Fix/coroutine errors#3130
Open
cjswedes wants to merge 6 commits into
Open
Conversation
Driver:get_device_info can return (nil, err) when the devices API request fails or the device is no longer known to the driver (e.g. transient device_api error, or a device removed between building the device list and looking it up). device_health_check ignored the second return value and unconditionally called device:send(...), which internally calls device:get_short_address() in cluster_base.lua, causing 'attempt to call a nil value (method get_short_address)' when device was nil. Impact: the periodic health-check coroutine crashes with an unhandled error, which can disrupt the scheduled health-check loop and spam hub logs, potentially leaving range extender devices marked unhealthy/unresponsive even though the actual issue was just a lookup failure. Fix: check the return value of get_device_info and skip sending the ZCLVersion read (logging a warning) when the device could not be resolved, instead of dereferencing nil. Co-Authored-By: OpenCode <opencode@users.noreply.github.com>
update_connection() fetched the CREDENTIAL device field and passed it
directly to jbl_api:add_header without checking for nil, unlike the
sibling call site in device_added(). When a device reconnects via
find_new_connection() before a credential has been stored on the
device, add_header's string concatenation ('.. .. value') throws
'attempt to concatenate a nil value (local value)', which crashes the
coroutine handling the device and can leave the device connection in
a bad state (no SSE/health-check updates) until the driver restarts.
Fix:
- Guard update_connection so it logs the missing credential, marks the
device offline, and returns early instead of reaching add_header
with a nil value.
- Harden jbl_api:add_header to tostring() its key/value before
concatenation as defense-in-depth, so any other future caller can't
trigger the same crash.
Co-Authored-By: OpenCode <opencode@users.noreply.github.com>
The Aqara Presence Sensor FP2 driver uses an SSE/HTTP chunked-transfer
client (lunchbox/sse/eventsource.lua) to stream events from the device.
When a chunk-encoded response ends, the server sends a zero-length
chunk header ("0\r\n"). The reader parsed this as recv_as_num == 0
and called sock:receive(0), which the shared cosock TCP socket
(cosock/socket/tcp.lua) does not handle gracefully: its output
transform asserts that the received data is non-nil, so a 0-byte
receive raises "socket receive returned nil data" instead of
returning a normal result. That assertion failure crashes the
coroutine driving the device's LAN connection, which drops presence
updates until the hub restarts the driver.
Fix: when the parsed chunk size is 0, skip the socket receive call
entirely and treat it as an empty read, then continue on to consume
the trailing CRLF as before. This avoids ever calling receive(0) on
the socket, preventing the crash while preserving existing timeout/
closed/error handling paths.
Co-Authored-By: OpenCode <opencode@users.noreply.github.com>
The timed-tamper-clear sub-driver schedules a delayed callback (via device.thread:call_with_delay) to emit a tamperAlert clear event a fixed time after a tamper notification is received. If the device is removed from the hub before that timer fires, st.device's Device:deleted() swaps the device's metatable so that every field and method lookup returns nil. When the pending timer callback later runs and calls device:emit_event_for_endpoint(...), that method resolves to nil, producing: attempt to call a nil value (method 'emit_event_for_endpoint') This crashed the device's Lua thread coroutine, so any other pending work for that device (or shared device thread, since this driver uses shared_device_thread_enabled) could be disrupted, degrading the user experience for other devices using the same driver/thread. Fix: check device.id (which becomes nil once the device is deleted) before touching the device inside the delayed callback, and skip the event emission/timer cleanup if the device is no longer valid. Co-Authored-By: OpenCode <opencode@users.noreply.github.com>
…n handlers Child (notification) devices call device:get_parent_device() and then schedule a 1s-delayed closure that sends a manufacturer-specific command via the captured parent reference. If the parent device is removed (or otherwise not resolvable, e.g. during a join/removal race) at command time, get_parent_device() returns nil. The delayed closure then invokes cluster_base.build_manufacturer_specific_command(nil, ...), which calls device:get_short_address() on nil, crashing the coroutine with: attempt to call a nil value (method 'get_short_address') This meant on/off/level/color/color-temperature commands on an Inovelli notification child device could silently crash the driver (breaking further command handling) instead of just failing to notify the parent. Fix: check the parent device reference for nil immediately after get_parent_device() and bail out before scheduling the send, in on_handler, off_handler, switch_level_handler, set_color_temperature, and set_color. Also updated getNotificationValue to accept the already resolved parent device instead of re-fetching it, avoiding a second nil dereference of the same kind. Co-Authored-By: OpenCode <opencode@users.noreply.github.com>
Co-Authored-By: OpenCode <opencode@users.noreply.github.com>
cjswedes
requested review from
NoahCornell,
aleclorimer,
ctowns,
hcarter-775 and
varzac
July 17, 2026 16:15
|
Invitation URL: |
Test Results 73 files 536 suites 0s ⏱️ Results for commit 93d2b35. |
|
matter-lock_coverage.xml
zigbee-range-extender_coverage.xml
zigbee-switch_coverage.xml
zwave-sensor_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 93d2b35 |
varzac
reviewed
Jul 17, 2026
| local device = self:get_device_info(device_id, false) | ||
| device:send(Basic.attributes.ZCLVersion:read(device)) | ||
| local device, err = self:get_device_info(device_id, false) | ||
| if device == nil then |
Contributor
There was a problem hiding this comment.
Given that we request the device list immediately before this, I'm not really sure how this is possible. This one might be worth grabbing hub logs for some hubs to investigate because it seems like it shouldn't be possible to me.
Contributor
Author
There was a problem hiding this comment.
This can happen if there is an incomplete inventory record for the device. Worth looking into though, like you mention
varzac
approved these changes
Jul 17, 2026
ctowns
approved these changes
Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These are fixes for a handful of coroutine errors that we are seeing in production.
The JBL and Aqara fixes were the highest impact.