fix(go2): override ReplayConnection.stop() to avoid AttributeError on hot-reload#3182
fix(go2): override ReplayConnection.stop() to avoid AttributeError on hot-reload#3182xiaoyaoqilan wants to merge 1 commit into
Conversation
… hot-reload Resolves dimensionalOS#1967. ReplayConnection intentionally does not call UnitreeWebRTCConnection.__init__() (that constructor opens a live WebRTC peer), so it never gets self.stop_timer / self.loop / self.conn. The inherited stop() then raises 'ReplayConnection' object has no attribute 'stop_timer' when GO2Connection.stop() is called during hot-reload / restart, which is what breaks restart(). Replay connections have nothing to disconnect, so override stop() to just clear the deadman timer slot and let the framework handle the rest.
Greptile SummaryThis PR adds a
Confidence Score: 4/5The PR appears safe to merge, though the replay-specific stop method should explicitly dispose The override fixes the reported uninitialized WebRTC attribute access, but it leaves the registered replay store and its SQLite resources undisposed when stop occurs without immediate process termination. Files Needing Attention: dimos/robot/unitree/go2/connection.py Important Files Changed
Reviews (1): Last reviewed commit: "fix(go2): override ReplayConnection.stop..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def stop(self) -> None: | ||
| # A replay connection has no live WebRTC loop or peer to tear down, so | ||
| # the inherited UnitreeWebRTCConnection.stop() (which touches self.loop | ||
| # and self.conn) does not apply here. Clear the deadman timer slot and | ||
| # let the framework handle the rest of the module teardown. | ||
| # Fixes 'ReplayConnection' object has no attribute 'stop_timer' raised | ||
| # during GO2Connection hot-reload / restart (#1967). | ||
| self.stop_timer = None |
There was a problem hiding this comment.
After the replay property registers its started SqliteStore as a ReplayConnection disposable, this override returns without running CompositeResource.stop(). This leaves the replay store, its SQLite connections, and scheduled replay work open during in-process teardown; skip only the WebRTC-specific cleanup while still disposing replay-owned resources.
Knowledge Base Used: Launching Robots via the CLI and Hardware Vendor Integration
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #3182 +/- ##
==========================================
- Coverage 73.77% 71.32% -2.46%
==========================================
Files 1109 1109
Lines 104383 104385 +2
Branches 9567 9567
==========================================
- Hits 77013 74449 -2564
- Misses 24695 27381 +2686
+ Partials 2675 2555 -120
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 29 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
This does not fix #1967 . The issue there is present in with UnitreeWebRTCConnection as well, not just ReplayConnection. |
2 participants
Fixes #1967.
Root cause.
ReplayConnectionintentionally skipsUnitreeWebRTCConnection.__init__()(that constructor opens a live WebRTC peer viaLegionConnection+connect()), so it never initializesself.stop_timer,self.loop, orself.conn. The inheritedstop()therefore raisesAttributeError: 'ReplayConnection' object has no attribute 'stop_timer'as soon asGO2Connection.stop()is invoked — which is exactly whatrestart()does during hot-reload, breaking the reload.Fix. Override
stop()onReplayConnectionto clear the deadman timer slot and otherwise do nothing: a replay connection has no live loop/peer to disconnect, and the module's teardown is handled by the framework. This removes the AttributeError that abortedrestart().Note: this fixes the concrete
AttributeErrorthrown bystop()during hot-reload. Ifrestart()still hangs for reasons unrelated to teardown (e.g. the daemon re-registering the module), that is a separate path and worth a follow-up issue.