Conversation
The config-only and in-place rename paths rewrite the YAML without flashing, so the firmware keeps broadcasting its pre-rename hostname while the dashboard forgets it. Record that name as deployed_name in the device-metadata store and publish its cached addresses under the new name's OTA cache key when the new name resolves nothing; clear it on the next landed flash, on a completed rename chain, or when a rename targets that name again.
|
@esphbot review |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2814 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 294 294
Lines 25317 25351 +34
=======================================
+ Hits 25254 25288 +34
Misses 63 63
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
|
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
A direct esphome.name edit is the third flash-free rename, so stamp deployed_name off the scan's name change too. Drop the record when another config claims that name; whatever answers to it is no longer this device, and targeting it would OTA the wrong board.
|
@esphbot review |
|
|
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
The cold-start shallow to deep refine renames a package-supplied name off its filename-stem placeholder, which no firmware answered to, so only stamp for a device that was compiled at least once. Clear the record when the device is heard over mDNS under its own name, so a flash from outside the dashboard heals it.
|
@esphbot review |
|
|
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
--only-generate populates loaded_integrations without flashing, so the gate means the device has build output, not that it was ever flashed.
|
@esphbot review |
|
|
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
An in-place rescan re-enters the scan-change stamp with the pre-rename name, resurrecting a record the rename just cleared. Re-assert what the rename resolved once the rescan is done; a no-op on every other path. Also say the reclaimed-name record is ignored, not dropped: the read skips it while another config owns the name, the entry stays on disk.
|
@esphbot review |
|
|
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
|
@esphbot review |
|
Re-reviewed. No commits since Re-verified the full ledger this pass: One carried-over suggestion: |
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
The re-assert runs outside the write locks, so a concurrent rename can migrate the entry between the rescan and it, and a truthy record would re-create an orphan row nothing reads.
|
@esphbot review |
|
|
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Upload-only cleanup, incomplete mDNS signaling, duplicate-name handling, and a post-rescan race can discard or revive the fallback incorrectly.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 3
Open (4)
What changed in this PR
Adds persistent tracking of pre-rename hostnames so OTA installs can reach devices after flash-free renames.
Changes:
- Records and clears
deployed_namemetadata across rename, scan, mDNS, and flash paths. - Uses cached addresses for the deployed hostname during OTA.
- Adds documentation and regression coverage.
| File | Description |
|---|---|
esphome_device_builder/controllers/devices/_metadata_store.py |
Adds deployed_name storage. |
esphome_device_builder/controllers/devices/controller.py |
Supplies deployed names to OTA cache lookup. |
esphome_device_builder/controllers/devices/firmware_sync.py |
Clears metadata after flashes and rename completion. |
esphome_device_builder/controllers/devices/helpers.py |
Resolves cached addresses through the old hostname. |
esphome_device_builder/controllers/devices/metadata.py |
Implements deployed-name stamping. |
esphome_device_builder/controllers/devices/mutations_simple.py |
Records names during flash-free renames. |
esphome_device_builder/controllers/devices/scan_change.py |
Detects manual hostname edits. |
esphome_device_builder/controllers/devices/state_callbacks.py |
Clears records on mDNS state updates. |
tests/controllers/devices/test_branches_coverage.py |
Covers scan-based recording. |
tests/controllers/devices/test_metadata_store.py |
Pins the new store field. |
tests/controllers/devices/test_rename_config_only.py |
Covers flash-free rename scenarios. |
tests/controllers/devices/test_rename_metadata.py |
Covers completed rename cleanup. |
tests/controllers/firmware/test_address_cache.py |
Covers old-hostname cache fallback. |
tests/controllers/firmware/test_refresh.py |
Covers post-flash cleanup. |
tests/test_device_state_event.py |
Covers mDNS and ping behavior. |
docs/API.md |
Documents rename behavior. |
docs/ARCHITECTURE.md |
Documents persistence and lifecycle design. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A bootloader-only upload replaces no app, so it no longer clears the record. The mDNS self-heal moves onto the source change: apply() skips the state callback when the device is already ONLINE via ping, which is exactly the case the record keeps alive. The post-rescan re-assert narrows to the clear, the only value a rescan can pollute, so it can't overwrite a fresher mDNS or concurrent-rename value.
|
|
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Warning
Important issues found.
- mDNS self-heal is skipped for the in-place / hand-edited rename —
active_sourcesurvives the rename as MDNS
Five call sites open-coded the store key and its empty-means-cleared sentinel. Add _clear_deployed_name beside _stamp_deployed_name and a _deployed_name reader that owns the reclaimed-name rule, so the deferred sweep reader inherits it instead of copying the guard. The store gains a non-copying get_field, so the clear on every mDNS ownership change no longer copies an entry to find the key absent. Trim the docstrings and comments the record accumulated, hoist the duplicated metadata-store test scaffolding into tests/conftest.py, and move the lifecycle prose to the live-state section it belongs in.
A same-path rename carries runtime_state.active_source forward whole, so after an in-place or hand-edited rename the row still reads mdns while the ledger for the new name is unknown. The healing announce flips the ledger but not the row, so the clear below the dedupe never ran; the out-of-band flash it exists for was the case it missed.
|
@esphbot review |
PR Review — Remember the hostname a flash-free rename leaves on the deviceThe blocking finding from the previous round is fixed; what is left is reach and documentation, not correctness. Approving with nits. The mDNS self-heal now sits above the Pushing back on Copilot's first comment (
Verification: findings are code traces over the PR head checkout; no test runner was available in the review worktree, so nothing here was observed at runtime. ✅ Resolved since last review (1)Previously-flagged issues verified fixed
🟢 Suggestions
1. The API reviver can wipe `device.ip` out from under this fallback for exactly the devices carrying a `deployed_name`
|
esphbot
left a comment
esphbot
left a comment
There was a problem hiding this comment.
Tip
No blocking issues found — ready to merge.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several paths can incorrectly clear or reactivate the hostname record, potentially leaving OTA unavailable or targeting the wrong device.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 5
Open (6)
| it is then somebody else's device, not ours. | ||
| """ | ||
| deployed_name = self._metadata_store.get_field(configuration, "deployed_name") or "" | ||
| return "" if self._devices_by_name(deployed_name) else deployed_name |
There was a problem hiding this comment.
Declined, deliberately. A YAML claiming a name is not evidence that a board answers it: the common case is re-creating kitchen.yaml that is never flashed while the original board still answers kitchen.local, and clearing on claim would destroy the only route back to it. Masking is recoverable, deleting is not. The docs say "ignored while another config owns that name", not dropped, so the contract matches the code. esphbot reached the same conclusion independently this round.
| deployed_name = self._metadata_store.get_field(configuration, "deployed_name") or "" | ||
| return "" if self._devices_by_name(deployed_name) else deployed_name |
There was a problem hiding this comment.
Fixed in 0ee2d61, though by removing the raw read rather than validating it. _deployed_name now reads Device.deployed_name, which _resolve_device_metadata already coerces with str(), so a malformed persisted value can never reach _devices_by_name as an unhashable type.
| return | ||
| recompute_hash = job_type in COMPILING_JOB_TYPES | ||
| flashed = job_type in (JobType.UPLOAD, JobType.INSTALL) | ||
| if flashed and not job.flash_bootloader: |
There was a problem hiding this comment.
Declined, and esphbot pushed back on this one too. A rename migrates no build dir: the build tree is keyed on the device name, so after a rename there is no artifact under the new name until a compile, and that compile bakes in the new hostname. A standalone firmware/upload therefore either flashes an image built from the current YAML, or fails and never reaches this clear (only COMPLETED jobs do). The --bootloader carve-out covers the one upload that genuinely replaces no app.
There was a problem hiding this comment.
Declined; esphbot pushed back on this one too. A rename migrates no build dir: the build tree is keyed on the device name, so after a rename there is no artifact under the new name until a compile, and that compile bakes in the new hostname. A standalone firmware/upload therefore either flashes an image built from the current YAML, or fails and never reaches this clear (only COMPLETED jobs get here). The --bootloader carve-out covers the one upload that genuinely replaces no app.
| await rescan_renamed(controller, new_filename) | ||
| # An in-place rescan re-stamps the pre-rename name over a record this | ||
| # rename just cleared. Only the clear is re-asserted, so this can't | ||
| # overwrite a fresher mDNS or concurrent-rename value. | ||
| if not deployed_name: | ||
| controller._clear_deployed_name(new_filename) |
There was a problem hiding this comment.
Acknowledged, not fixed. The clear is now conditional (only a cleared value is re-asserted), so this needs two concurrent in-place renames of the same device, and the loss is recoverable: the record re-heals on the next mDNS announce or is made moot by the next flash. Holding the locks across the rescan risks deadlock, since scan handlers run arbitrary work, and a true compare-and-swap needs a generation counter the store does not have. Not worth either for this window.
| for device in controller._devices_by_name(name): | ||
| if source is ReachabilitySource.MDNS: |
There was a problem hiding this comment.
Good catch, fixed in 0ee2d61. The clear now only runs when the announced name maps to exactly one config; siblings sharing a name share the broadcast, so it cannot attribute which one was flashed. Covered by test_mdns_of_a_shared_name_keeps_both_deployed_names.
| device, self._state_monitor, self._deployed_name(configuration) | ||
| ) | ||
|
|
||
| def _deployed_name(self, configuration: str) -> str: |
There was a problem hiding this comment.
Fixed in 0ee2d61; _deployed_name moved down to the private-helper section.
Without this the reviver dials the persisted IP, reads the firmware's pre-rename name, calls the lease stale and wipes device.ip, which is the fallback the redirect leans on when mDNS is dark. Worse, the device stays OFFLINE, so enqueue_install_or_defer turns the install into a compile-only job armed for a wake that never comes; the record only pays off if an OTA actually runs. Accepting the recorded name revives it under ping and lets the install through. Also only heal off an mDNS announce when the name maps to one config, and move the deployed_name reader onto the Device row and down into the private section.
Mark deployed_name serialize-omit: both readers are backend, nothing on the frontend consumes it, and shipping it would make an internal record a de-facto wire contract. Apply the reclaimed-name rule in the reviver too, so the rule the docs state as a property of the record holds at both readers rather than just one. Hoist the mDNS clear out of a loop it can only traverse once, fold the address fallback into the lookup shape above it, short-circuit the empty record before the bucket lookup, coerce the stamp like the load path does, and collapse the source-change tests into one table.
What does this implement/fix?
Renaming a device without flashing it leaves the firmware answering to its old hostname, and we kept no record of that hostname. Every later install then targets
<new>.local, which nothing answers to, so the device is stuck on the old name until you rename it back. That is the reported loop.Three paths land there: a config-only rename (the offline confirmation), an in-place rename, and a plain
esphome.nameedit in the editor.All three now record the pre-rename name as
deployed_namein the device-metadata store, and the OTA address-cache args publish that name's cached addresses under the new name's key. It is only consulted when the device's own name resolves nothing, so a stale record is inert.The record clears on the next landed flash, on a completed rename chain, on a rename back to it, and on an mDNS announce under the device's own name (so a flash from outside the dashboard heals it). It is dropped when another config claims that name, since whatever answers to it is then somebody else's board.
Device status is deliberately untouched: the device really is unreachable under its new name, so showing it offline is honest. The only thing missing was a way for the install to land.
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.