Skip to content

Hoist offloader/receiver lifecycle scaffolding to a shared base#638

Merged
bdraco merged 2 commits into
mainfrom
remote-build-shared-base
May 12, 2026
Merged

Hoist offloader/receiver lifecycle scaffolding to a shared base#638
bdraco merged 2 commits into
mainfrom
remote-build-shared-base

Conversation

@bdraco
Copy link
Copy Markdown
Member

@bdraco bdraco commented May 12, 2026

What does this implement/fix?

Hoists the four lines of lifecycle scaffolding that
:class:OffloaderController and :class:ReceiverController
carried verbatim into a small base class —
:class:_RemoteBuildBase in
controllers/remote_build/_shared.py.

The duplicated state:

  • self._db = device_builder — every controller in the
    package needs the DeviceBuilder ref.
  • self._tasks: set[asyncio.Task[None]] = set() — strong-ref
    set for fire-and-forget tasks.
  • self._listeners = ExitStack() — bus-listener unsubscribers
    walked at stop time.
  • self._shutdown_callbacks: list[ShutdownCallback] = []
    per-file Store flush callbacks walked at stop time.

Plus the identical 4-line _track_task method.

After this PR, each sibling's __init__ shrinks by ~5 lines
(state + _track_task body), the duplicate state-init order
matches between siblings by virtue of the same constructor,
and any future role controller in this package can inherit
the same scaffolding without copy-paste.

Why single-inheritance and not a mixin

The earlier mixin attempt at the role split was rejected
because multiple-inheritance state-sharing made type-checker
attribute resolution unreliable (a mixin reaching for
self._pairings couldn't be statically confirmed to land on
a class that defined it). Single-inheritance doesn't have
that problem: every attribute resolves through one MRO step,
mypy sees them on the base, and the subclasses' specific
state stays on the subclasses.

What stayed where

  • drain_tasks is still a free function in the same module —
    it's stateless and the per-role stop callers already pass
    in a specific iterable (self._tasks,
    self._pair_status_listeners.values(),
    h.task for h in self._peer_link_clients.values()), so the
    function shape is the right one. Moving it to a method
    would force callers to instance-bind for no win.
  • Each sibling's start and stop stay on the subclass —
    the sequences are role-specific (offloader brings up mDNS
    browse + peer-link clients; receiver brings up
    SubmitJobReceiver / ArtifactsDownloadSender /
    JobFanout), and a base-class default no-op wouldn't
    help.

Follow-up

A wider TaskTracker-style base that
:class:DeviceStateMonitor could also share is deferred to a
follow-up PR. The monitor has the same
task.add_done_callback(self._tasks.discard) pattern in
three places but doesn't take a DeviceBuilder, doesn't keep
listeners, and doesn't manage shutdown callbacks — so the
shared base for it has to be carved out separately, and
_RemoteBuildBase itself would then inherit from that
smaller base.

Behaviour preservation

Pure refactor. Each __init__ initializes the same four
fields with the same default values; _track_task is the
same 4 lines just on the parent now. 3117 tests pass, ruff

  • mypy clean.

Related issue or feature (if applicable):

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Frontend coordination

  • No frontend change needed
  • Companion frontend PR: esphome/device-builder-dashboard-frontend#

Checklist

  • The code change is tested and works locally.
  • Pre-commit hooks pass (ruff, codespell, yaml/json/python checks).
  • Tests have been added or updated under tests/ where applicable.
  • components.json has not been hand-edited (regenerate via script/sync_components.py if a sync is needed).
  • Architecture-level changes are reflected in docs/ARCHITECTURE.md and/or docs/API.md.

The two sibling controllers each carried the same four
init lines (_db, _tasks, _listeners,
_shutdown_callbacks) and an identical _track_task
method body. Single-inheritance base class
(:class:`_RemoteBuildBase` in ``_shared.py``) collapses
the duplication without the type-checker headaches the
multiple-inheritance mixin attempt had — every attribute is
declared once and reached through one MRO step.

Concrete subclasses call ``super().__init__(device_builder)``
and layer role-specific state on top; their own ``start`` /
``stop`` continue to live on each class since the
sequences are role-specific.

``drain_tasks`` stays as a free function in the same
module — it's stateless and the per-role ``stop`` callers
already use it that way.

A wider ``TaskTracker``-style base that
:class:`DeviceStateMonitor` could also share is deferred to
a follow-up PR.
Copilot AI review requested due to automatic review settings May 12, 2026 03:04
@github-actions github-actions Bot added the refactor Code refactor with no user-visible behaviour change label May 12, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 12, 2026

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing remote-build-shared-base (109fbdf) with main (696cf7d)

Open in CodSpeed

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.22%. Comparing base (696cf7d) to head (109fbdf).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #638      +/-   ##
==========================================
- Coverage   99.22%   99.22%   -0.01%     
==========================================
  Files          92       92              
  Lines       10804    10800       -4     
==========================================
- Hits        10720    10716       -4     
  Misses         84       84              
Flag Coverage Δ
py3.12 99.19% <100.00%> (-0.01%) ⬇️
py3.14 99.22% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...device_builder/controllers/remote_build/_shared.py 100.00% <100.00%> (ø)
...vice_builder/controllers/remote_build/offloader.py 99.49% <100.00%> (-0.01%) ⬇️
...evice_builder/controllers/remote_build/receiver.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Refactors the remote-build offloader/receiver siblings to share identical lifecycle scaffolding via a new _RemoteBuildBase in controllers/remote_build/_shared.py, reducing duplication while preserving role-specific start/stop behavior.

Changes:

  • Introduce _RemoteBuildBase (shared _db, _tasks, _listeners, _shutdown_callbacks, and _track_task) alongside existing drain_tasks.
  • Update OffloaderController to inherit from _RemoteBuildBase and remove duplicated init/task-tracking code.
  • Update ReceiverController to inherit from _RemoteBuildBase and remove duplicated init/task-tracking code.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
esphome_device_builder/controllers/remote_build/_shared.py Adds _RemoteBuildBase lifecycle scaffolding shared by offloader/receiver and expands module docs.
esphome_device_builder/controllers/remote_build/offloader.py Switches offloader to inherit from _RemoteBuildBase, removing duplicated fields and _track_task.
esphome_device_builder/controllers/remote_build/receiver.py Switches receiver to inherit from _RemoteBuildBase, removing duplicated fields and _track_task.

Comment thread esphome_device_builder/controllers/remote_build/_shared.py Outdated
CLAUDE.md's docstring rule: consumer-facing, no padding with
implementation history. The first pass leaned on the mixin-
rejection rationale and the field-purpose paraphrases that
the well-named fields already convey.

Module docstring now lists what the module exposes (the base
class and the free helper) and what each is for. Class
docstring tells subclasses how to use it -- super().__init__,
where stop() responsibilities lie -- and drops the per-field
explainers.
@bdraco bdraco merged commit 0a962c1 into main May 12, 2026
13 checks passed
@bdraco bdraco deleted the remote-build-shared-base branch May 12, 2026 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Code refactor with no user-visible behaviour change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants