Split remote_build controller into Offloader + Receiver siblings#637
Merged
Conversation
The 2837-line ``RemoteBuildController`` mixed two distinct roles that share nothing but a ``DeviceBuilder`` reference: the *outbound* side (pair flow, peer-link clients, submit_job / cancel_job / download_artifacts, mDNS browse) and the *inbound* side (peer-link session registry, pair inbox, queue_status fan-out, identity rotate, cleanup sweep). A pre-split analysis confirmed only 10 cross-role method references — all in lifecycle (``start`` / ``stop``) and one in the mDNS browse callback that routes naturally to the offloader. Replace the single class with two siblings exposed on ``DeviceBuilder``: * ``self.remote_build_offloader: OffloaderController`` — owns 47 offloader methods plus mDNS browse (7), peer-link resolver (2), and the X25519 + dashboard_id loader. Lives in ``controllers/remote_build/offloader.py``. * ``self.remote_build_receiver: ReceiverController`` — owns 34 receiver methods plus identity ``get`` / ``rotate``. Lives in ``controllers/remote_build/receiver.py``. * ``controllers/remote_build/_shared.py`` — thin module with the ``drain_tasks`` free helper both siblings need. The umbrella ``RemoteBuildController`` class is gone — no delegating facade. Each sibling has its own ``__init__`` / ``start`` / ``stop`` / ``_tasks`` / ``_listeners`` / ``_shutdown_callbacks``. ``peer_link.py`` and ``job_fanout.py`` take ``ReceiverController`` as their controller arg; ``firmware/controller.py`` and ``firmware/remote_runner.py`` reach into ``remote_build_offloader`` for ``build_scheduler_snapshot`` / ``get_pairing`` / ``_lookup_open_peer_link_client``. Tests use a small ``RemoteBuildTestHandles`` shim in ``tests/conftest.py`` that bundles both siblings with ``__getattr__`` / ``__setattr__`` forwarding, so existing test code addressing both sides through one controller keeps working. New tests should reach ``handles.offloader`` / ``handles.receiver`` directly. All 3117 tests pass; ruff + ruff-format + the rest of the pre-commit chain are clean. No behaviour change — the split moves method bodies verbatim and partitions ``__init__`` / ``start`` / ``stop`` along the role boundary the methods already followed.
github-actions
Bot
added
the
refactor
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the remote-build subsystem by splitting the former monolithic controller into two sibling controllers—an outbound OffloaderController and an inbound ReceiverController—and updates production wiring plus test scaffolding to match the new separation of concerns.
Changes:
- Replace the single remote-build controller with
remote_build_offloader+remote_build_receiveronDeviceBuilder, including lifecycle andsubscribe_eventsinitial-state seeding. - Rewire peer-link handling and firmware remote-runner integration to use the appropriate sibling controller.
- Update the test suite to avoid widespread churn via a
RemoteBuildTestHandlesshim that forwards legacy attribute/method access to the correct sibling.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_subscribe_events_cleanup.py | Update DB stubs and initial-state seeding expectations for split remote-build controllers. |
| tests/test_remote_build_peer_link.py | Switch test imports/types to the test-handles shim after controller split. |
| tests/test_remote_build_peer_link_client.py | Update monkeypatch targets/imports to offloader module and use split controller types in tests. |
| tests/test_remote_build_listener.py | Adjust listener-start tests to reference remote_build_receiver instead of the removed remote_build. |
| tests/test_remote_build_controller.py | Update monkeypatch paths/logger names to offloader/receiver modules after extraction. |
| tests/e2e/conftest.py | Update e2e pairing fixture typing/imports to use the test-handles shim. |
| tests/controllers/firmware/test_rename_lock.py | Ensure firmware tests seed remote_build_offloader/remote_build_receiver attributes for production-like DB shape. |
| tests/controllers/firmware/test_remote_runner.py | Rewire remote-runner tests to use _db.remote_build_offloader. |
| tests/controllers/firmware/test_install.py | Rewire install-source resolution tests to use _db.remote_build_offloader. |
| tests/controllers/firmware/test_clean.py | Rewire clean fan-out tests to use _db.remote_build_offloader. |
| tests/controllers/firmware/conftest.py | Update firmware test DB factory to seed split remote-build attributes. |
| tests/conftest.py | Add RemoteBuildTestHandles shim and update hermetic lifecycle stubs for both siblings. |
| esphome_device_builder/device_builder.py | Replace remote_build with remote_build_offloader/remote_build_receiver across start/stop, command collection, and subscribe-events seeding; bind peer-link handler to receiver. |
| esphome_device_builder/controllers/remote_build/receiver.py | New receiver-side controller owning inbound peer-link sessions, peer registry, settings, identity rotation, and cleanup. |
| esphome_device_builder/controllers/remote_build/peer_link.py | Update peer-link handler/controller typing and docs for receiver ownership. |
| esphome_device_builder/controllers/remote_build/offloader.py | Rename/extract offloader-side controller and remove inbound responsibilities. |
| esphome_device_builder/controllers/remote_build/job_fanout.py | Update controller typing/docs to reference ReceiverController. |
| esphome_device_builder/controllers/remote_build/_shared.py | Add shared drain_tasks() helper used by both siblings. |
| esphome_device_builder/controllers/remote_build/init.py | Update public exports to OffloaderController and ReceiverController. |
| esphome_device_builder/controllers/firmware/remote_runner.py | Use _db.remote_build_offloader for client lookup and remote cancel dispatch. |
| esphome_device_builder/controllers/firmware/controller.py | Use _db.remote_build_offloader for scheduler snapshot + pairing lookup. |
Comments suppressed due to low confidence (1)
esphome_device_builder/controllers/remote_build/offloader.py:166
- Both new sibling modules are still well above the repo’s 800-line soft cap for new modules (see CLAUDE.md “File size: 800-line soft cap”). Since
offloader.pyis ~1700 lines, consider splitting it further into smaller concern-focused submodules (e.g. mdns discovery, pairing flow, client lifecycle) to keep future reviews/edits manageable.
Followup to #637: the test-only ``RemoteBuildTestHandles`` landed as a ``__getattr__`` / ``__setattr__`` shim that forwarded any unprefixed attr to whichever sibling happened to own it. That hid one real bug in the refactor itself (``submit_job.py`` was still reaching for ``_db.remote_build``, caught by mypy in CI but not by the shim's tests), and an ambiguous shared attr — ``_shutdown_callbacks`` exists on both siblings, so legacy ``for cb in controller._shutdown_callbacks`` flushed only the offloader's stores, not the receiver's. Replace the shim with a frozen dataclass that exposes only ``offloader`` / ``receiver`` (plus a single ``_db`` convenience property; both siblings share the same ``DeviceBuilder`` ref). Tests address sibling-owned state and methods explicitly: * ``controller._pairings`` → ``controller.offloader._pairings`` * ``controller._approved_peers`` → ``controller.receiver._approved_peers`` * ``controller.preview_pair(...)`` → ``controller.offloader.preview_pair(...)`` * ``controller.set_settings(...)`` → ``controller.receiver.set_settings(...)`` Hand-finished the bits the rewrite script couldn't disambiguate: * ``receiver_server`` fixture in ``test_remote_build_peer_link_client.py`` now yields the receiver-side sibling directly (its only consumer is ``make_peer_link_handler``). * ``_make_offloader_controller`` likewise returns an ``OffloaderController`` directly. * ``PairedInstances`` exposes ``offloader_handles`` / ``receiver_handles`` for the full bundles (start/stop lifecycle, ``_db`` mutation), plus shortcut ``offloader`` / ``receiver`` properties pointing at the role-relevant sibling on each dashboard. * ``MagicMock(spec=RemoteBuildController)`` in ``test_remote_build_peer_link.py`` becomes ``MagicMock(spec=ReceiverController)`` so the intent-dispatch surface lines up with the actual handler arg. Fixed alongside: * ``submit_job.py:639`` was reading ``self._firmware._db.remote_build`` -- replaced with ``remote_build_receiver`` (mypy now passes). * ``tests/conftest.py`` ``_shutdown_callbacks`` ambiguity gone: each sibling's callbacks are addressed through its own attribute. All 3117 tests pass; ruff / mypy clean.
test_start_seeds_approved_peers_dict_from_disk was flushing seeder.offloader._shutdown_callbacks before reading the peers back through a fresh controller, but _peers_store is the receiver-side per-file Store -- its flush callback lives on seeder.receiver._shutdown_callbacks. The offloader's list flushes _pairings_store, which this test never seeds. Locally the test passed because the loop pump during the offloader-side await cb() happened to fire the receiver store's call_at(delay=0) write before the next controller opened the file. CI was slow enough to miss the pump, leaving the seeded peer un-flushed and the fresh controller's _approved_peers empty. Iterate the receiver's callbacks here -- same lesson the cleanup PR (#637's followup) was supposed to surface, but this caller went unnoticed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #637 +/- ##
=======================================
Coverage 99.21% 99.22%
=======================================
Files 90 92 +2
Lines 10757 10804 +47
=======================================
+ Hits 10673 10720 +47
Misses 84 84
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Three corrections flagged on the open PR:
* ReceiverController.get_submit_job_receiver /
get_artifacts_download_sender raised
RuntimeError('... before RemoteBuildController.start()')
-- stale class name from before the role split. Updated the
messages (and the two regex matchers in
test_remote_build_controller.py) to reference
ReceiverController.start().
* The dashboard_id-validation comment in peer_link.py's
_dispatch_intent pointed at a
ReceiverController._validate_dashboard_id method that
doesn't exist. Validation lives in
:func:`controllers.remote_build._validators.validate_dashboard_id`;
the comment now points at the actual function.
Sphinx-style ``:class:`RemoteBuildController``` /
``:meth:`RemoteBuildController.X``` references in models,
helpers, and submodule docstrings still pointed at the
pre-split class. Audit had flagged these as purely
informational (no runtime effect), but they're load-bearing
for code-reading — readers click through to a class that no
longer exists.
Script-driven rewrite based on actual class membership:
``.X`` references resolve to whichever sibling owns the
attribute / method; ambiguous shared names (``start`` /
``stop`` / ``__init__``) and bare class references were
disambiguated by surrounding context (receiver-side accept
path → ``ReceiverController``, offloader-side peer-link →
``OffloaderController``).
Affects ``models/{remote_build, common, firmware}.py``,
the ``helpers/{build_scheduler, remote_build_cleanup,
remote_build_layout}.py`` cross-links, ``peer_link_client``
/ ``submit_job`` / ``artifacts_download`` /
``_models.py`` docstrings, plus a handful of test-side
prose. Pure docstring / comment churn — zero code lines
touched. All 3117 tests pass, ruff + mypy clean.
This was referenced May 12, 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
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.
What does this implement/fix?
The 2837-line
RemoteBuildControllermixed two distinctroles that share nothing but a
DeviceBuilderreference: theoutbound side (pair flow, peer-link clients,
submit_job/cancel_job/download_artifacts, mDNS browse) and theinbound side (peer-link session registry, pair inbox,
queue_statusfan-out, identity rotate, cleanup sweep). Apre-split analysis confirmed only 10 cross-role method
references — all in lifecycle (
start/stop) and one inthe mDNS browse callback that routes naturally to the
offloader.
This PR replaces the single class with two siblings
exposed on
DeviceBuilder, with no umbrella facade:self.remote_build_offloader: OffloaderController— 47offloader methods plus mDNS browse (7), peer-link resolver
(2), and the X25519 +
dashboard_idloader. Lives incontrollers/remote_build/offloader.py(1721 lines).self.remote_build_receiver: ReceiverController— 34receiver methods plus identity
get/rotate. Lives incontrollers/remote_build/receiver.py(1107 lines).controllers/remote_build/_shared.py— thin module withthe
drain_tasksfree helper both siblings need (30 lines).Each sibling owns its own
__init__/start/stop/_tasks/_listeners/_shutdown_callbacks/per-file
Store. Cross-role method references: zero afterthe split.
Wire-up changes
peer_link.pyandjob_fanout.pytakeReceiverControlleras their controller arg (
_peer_link_sessions,record_pair_request,lookup_peer_for_status,get_submit_job_receiver,get_artifacts_download_sender,handle_cancel_joball live on the receiver).firmware/controller.pyandfirmware/remote_runner.pyreach into
self._db.remote_build_offloaderforbuild_scheduler_snapshot/get_pairing/_lookup_open_peer_link_client.device_builder.py's_cmd_subscribe_eventsseeds splitper side: offloader contributes
pairings/hosts/offloader_alerts/peer_queue_status/remote_jobs/remote_builds_enabled; receiver contributespeers. Bothnull-checked independently.
DeviceBuilder.startregisters bothsiblings for
collect_api_commands, picking up the@api_commandWS handlers on each.Test wiring
Existing tests addressed both sides through a single controller
object. To avoid churning ~100
controller._Xreferencesacross 12 test files,
tests/conftest.pyexposes a smallRemoteBuildTestHandlesshim that bundles both siblings with__getattr__/__setattr__forwarding. Tests callingcontroller.preview_pair(...)transparently forward to theoffloader;
controller._approved_peersreads forward to thereceiver. Production code never sees the shim — it's purely
test-utility code.
New tests should reach
handles.offloader/handles.receiverdirectly.Behaviour preservation
Strictly mechanical:
controller.pyto one ofthe two new files (script-driven extraction; spans verified
byte-equal across the move).
__init__/start/stoppartition along the roleboundary the methods already followed.
(the rebind probe it kicks off is offloader state); routing
it to
OffloaderControllereliminates the one cross-rolecall without changing the runtime call graph.
All 3117 tests pass;
ruff+ruff-format+ the rest ofthe pre-commit chain are clean.
Related issue or feature (if applicable):
After three rounds of trimming the prose down (8576 →
4594 → 3723 → 3186 → 2837 lines), the next readability win
was structural: the file was still trying to be both an
offloader and a receiver. This PR is the structural split.
Types of changes
bugfixnew-featureenhancementbreaking-changerefactordocsmaintenancecidependenciesFrontend coordination
Checklist
ruff,codespell, yaml/json/python checks).tests/where applicable.components.jsonhas not been hand-edited (regenerate viascript/sync_components.pyif a sync is needed).docs/ARCHITECTURE.mdand/ordocs/API.md.