Queued offline updates#1430
Conversation
…e devices. When a firmware job completes successfully for a device that is currently offline, we set a new `queued_update` flag on the device. This indicates that the compiled firmware is ready and waiting to be flashed via OTA the next time the device checks in via mDNS.
…7/esphome-device-builder into queued-offline-updates
for more information, see https://pre-commit.ci
|
Will submit frontend to show queued status and option to cancel. Feedback appreciated. |
Merging this PR will degrade performance by 10.17%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_device_list_to_dict_fleet[50] |
637.8 µs | 710 µs | -10.17% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing rwalker777:queued-offline-updates (d06801c) with main (7af7fd4)
Core gap: |
|
|
esphbot
left a comment
There was a problem hiding this comment.
Blocking issues found — see the review comment above.
for more information, see https://pre-commit.ci
…7/esphome-device-builder into queued-offline-updates
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1430 +/- ##
========================================
Coverage 99.54% 99.54%
========================================
Files 223 224 +1
Lines 17423 17569 +146
========================================
+ Hits 17343 17489 +146
Misses 80 80
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
|
esphbot
left a comment
There was a problem hiding this comment.
Blocking issues found — see the review comment above.
|
This is dependent on this ESPHome PR - esphome/esphome#16358 |
Noted — feature can't function until esphome/esphome#16358 lands. Mark this PR draft until then. Two blockers independent of that dependency: |
|
|
esphbot
left a comment
There was a problem hiding this comment.
Blocking issues found — see the review comment above.
…gate offline update queuing logic, ensuring that devices are only queued once per offline state. This prevents redundant compile-only jobs for devices that are already marked as having a queued update, while still allowing new offline devices to be queued appropriately. Additionally, the test suite is updated to reflect the new flag and its behavior in the offline queue processing.
for more information, see https://pre-commit.ci
…7/esphome-device-builder into queued-offline-updates
for more information, see https://pre-commit.ci
PR Review — Queued offline updatesSolid persistence wiring, but the arming logic has a state-window gap that can silently swallow an install, plus reflective type access and unfilled frontend coordination. What's done well:
What needs attention:
🟡 Important1. Device coming online during the deferred compile silently drops the install (`esphome_device_builder/controllers/firmware/controller.py`, L258-272)The deferred-install arming is gated on
Consider the realistic sequence: offline device → install → compile starts → device wakes mid-compile (compiles take minutes) → Result: a binary is compiled but never flashed, with no error surfaced. The user clicked Install on an offline device, it came online, and nothing happened — which is exactly the scenario this feature exists to handle. Fix: when a deferred compile completes successfully, arm 2. `hasattr(self, "_state_monitor")` guards a state that cannot occur (`esphome_device_builder/controllers/devices/controller.py`, L321-325)
Why it matters: the guard masks real wiring bugs (a genuine Simplify to a direct call: 3. `getattr(...)`/`Any` reflective access on typed dataclass fields (`esphome_device_builder/controllers/firmware/controller.py`, L86-112)Several new sites reach for fields through
Why it matters: defensive Use direct attribute access ( 4. New model field + WS command need the companion frontend PR (`esphome_device_builder/models/devices.py`, L73-75)
The API.md row is added (good), but per CLAUDE.md, model-shape changes the frontend consumes require a coordinated companion PR, and the PR body still has the placeholder Why it matters: without the UI, a deferred install is invisible — the user clicks Install, nothing flashes immediately, and there is no surfaced "waiting for device to wake" state or a way to cancel it. Link the real frontend PR, or confirm explicitly that no frontend change is needed and explain how the user observes/clears the queued state. 🟢 Suggestions1. Tests pin wiring, not the deferred-install behavior (`tests/controllers/_device_state_monitor/test_controller.py`, L24-73)The monitor tests mock What's missing is a test of the feature's real contract: install on an OFFLINE device enqueues a compile-only job ( Consider replacing the redundant wiring tests with one behavioral test over the firmware controller's deferred path, including the "device comes online mid-compile" case. Checklist
To rebase specific severity levels, mention me: Automated review by Kōan (Claude) |
esphbot
left a comment
There was a problem hiding this comment.
Blocking issues found — see the review comment above.
|
@codspeedbot fix this regression |
What does this implement/fix?
Add detection when device is offline to local build and add a queue flag that triggers an OTA when the device wakes.
Related issue or feature (if applicable):
Types of changes
bugfixnew-featureenhancementbreaking-changerefactordocsmaintenancecidependenciesFrontend coordination
Checklist
ruff,codespell, yaml/json/python checks).tests/where applicable.components.index.json/definitions/components/*.jsonhave not been hand-edited (regenerate viascript/sync_components.pyif a sync is needed).docs/ARCHITECTURE.mdand/ordocs/API.md.