[no-release] fix(plugin): cut the reconnect backoff short after a successful bridge install (#135) - #155
Merged
Merged
Conversation
…e install (#135) A first install let the client's backoff grow to 30s while the package was missing, so the user watched half a minute of nothing directly after "installed and restarted... reconnects automatically" (observed: 29s to attach). WsJsonClient.retry_now() — thread-safe (the npm install runs on a plain thread; the loop is captured in run() and poked via call_soon_threadsafe) — cuts the current backoff wait short AND resets the attempt counter, so the immediate retry and any later failures start from 1s again: the history that grew the delay is stale the moment the node is reinstalled. The wait races the injected _sleep seam against the event rather than replacing it (the test suite fakes sleep to control and END scenarios; bypassing it would strand those tests), cancelling the loser and preserving a raising sleep's behaviour. A halted client ignores the poke — halts are fail-closed by design. Investigating that boundary found resume() has ZERO callers, so a version-mismatch halt currently survives its own prescribed remedy (the install menu). Filed as #154 rather than smuggled in here. ExportBridge.retry_now() passes through when a client exists (XG5: none while nothing is exported). _install_bridge_node pokes on both success terminals only, and the closing line now says "reconnecting now" because it is. 13 new tests (2278 Python), both mutations verified: dropping the attempt reset yields [1,2,4,8] not [1,2,4,1]; dropping the poke fails both success-terminal tests. Version 2026.8.10 -> 2026.8.12 (.11 is on the open #153 branch). Closes #135 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
📝 WalkthroughWalkthroughThe bridge client now supports thread-safe immediate retry wakeups. Successful bridge installation invokes this behavior through ChangesBridge retry flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Plugin
participant ExportBridge
participant WsJsonClient
participant EventLoop
Plugin->>ExportBridge: retry_now()
ExportBridge->>WsJsonClient: retry_now()
WsJsonClient->>EventLoop: schedule retry wakeup
EventLoop->>WsJsonClient: interrupt backoff
WsJsonClient->>WsJsonClient: reset retry attempts
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/ws_json_client.py:
- Around line 295-323: Update _wait_for_retry to wrap the asyncio.wait flow in a
finally block that cancels and awaits both sleep_task and wake_task whenever the
method is cancelled or exits early. Preserve the existing exception propagation,
retry-event clearing, and wake-result behavior while ensuring no child task
remains pending.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e3b1b93-3851-4b5c-815b-d4c94a7a7d34
📒 Files selected for processing (8)
indigo-matter.indigoPlugin/Contents/Info.plistindigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pyindigo-matter.indigoPlugin/Contents/Server Plugin/ws_json_client.pytests/fakes.pytests/test_export_bridge.pytests/test_pairing_menu.pytests/test_ws_json_client.py
Comment on lines
+295
to
+323
| async def _wait_for_retry(self, delay: float) -> bool: | ||
| """Wait ``delay`` seconds, or until :meth:`retry_now` wakes us — whichever | ||
| is first. Returns whether the wake won. | ||
|
|
||
| Races the injected ``self._sleep`` seam against ``_retry_event`` rather | ||
| than replacing it: the test suite fakes ``sleep`` to control (and end) | ||
| the backoff wait, and a wait that bypassed it outright would silently | ||
| strand every one of those tests. Whichever side loses is cancelled. | ||
| """ | ||
| sleep_task = asyncio.ensure_future(self._sleep(delay)) | ||
| wake_task = asyncio.ensure_future(self._retry_event.wait()) | ||
| done, pending = await asyncio.wait( | ||
| {sleep_task, wake_task}, return_when=asyncio.FIRST_COMPLETED) | ||
| for task in pending: | ||
| task.cancel() | ||
| try: | ||
| await task | ||
| except asyncio.CancelledError: | ||
| pass | ||
| if sleep_task in done and not sleep_task.cancelled(): | ||
| # Preserve the pre-existing behaviour of a sleep seam that raises — | ||
| # `await self._sleep(delay)` used to propagate it directly. | ||
| exc = sleep_task.exception() | ||
| if exc is not None: | ||
| raise exc | ||
| woken = wake_task in done | ||
| if woken: | ||
| self._retry_event.clear() | ||
| return woken |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cancel both child tasks when _wait_for_retry() is cancelled.
If run() is cancelled while Line 306 awaits asyncio.wait(), asyncio.wait() does not cancel sleep_task or wake_task. The run loop exits, but wake_task remains blocked and an injected sleep task can also remain alive. Cancel and await both tasks in a finally block.
Proposed fix
sleep_task = asyncio.ensure_future(self._sleep(delay))
wake_task = asyncio.ensure_future(self._retry_event.wait())
- done, pending = await asyncio.wait(
- {sleep_task, wake_task}, return_when=asyncio.FIRST_COMPLETED)
- for task in pending:
- task.cancel()
- try:
- await task
- except asyncio.CancelledError:
- pass
- if sleep_task in done and not sleep_task.cancelled():
- exc = sleep_task.exception()
- if exc is not None:
- raise exc
- woken = wake_task in done
- if woken:
- self._retry_event.clear()
- return woken
+ try:
+ done, pending = await asyncio.wait(
+ {sleep_task, wake_task}, return_when=asyncio.FIRST_COMPLETED)
+ for task in pending:
+ task.cancel()
+ await asyncio.gather(task, return_exceptions=True)
+ if sleep_task in done and not sleep_task.cancelled():
+ exc = sleep_task.exception()
+ if exc is not None:
+ raise exc
+ woken = wake_task in done
+ if woken:
+ self._retry_event.clear()
+ return woken
+ finally:
+ for task in (sleep_task, wake_task):
+ if not task.done():
+ task.cancel()
+ await asyncio.gather(sleep_task, wake_task, return_exceptions=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def _wait_for_retry(self, delay: float) -> bool: | |
| """Wait ``delay`` seconds, or until :meth:`retry_now` wakes us — whichever | |
| is first. Returns whether the wake won. | |
| Races the injected ``self._sleep`` seam against ``_retry_event`` rather | |
| than replacing it: the test suite fakes ``sleep`` to control (and end) | |
| the backoff wait, and a wait that bypassed it outright would silently | |
| strand every one of those tests. Whichever side loses is cancelled. | |
| """ | |
| sleep_task = asyncio.ensure_future(self._sleep(delay)) | |
| wake_task = asyncio.ensure_future(self._retry_event.wait()) | |
| done, pending = await asyncio.wait( | |
| {sleep_task, wake_task}, return_when=asyncio.FIRST_COMPLETED) | |
| for task in pending: | |
| task.cancel() | |
| try: | |
| await task | |
| except asyncio.CancelledError: | |
| pass | |
| if sleep_task in done and not sleep_task.cancelled(): | |
| # Preserve the pre-existing behaviour of a sleep seam that raises — | |
| # `await self._sleep(delay)` used to propagate it directly. | |
| exc = sleep_task.exception() | |
| if exc is not None: | |
| raise exc | |
| woken = wake_task in done | |
| if woken: | |
| self._retry_event.clear() | |
| return woken | |
| async def _wait_for_retry(self, delay: float) -> bool: | |
| """Wait ``delay`` seconds, or until :meth:`retry_now` wakes us — whichever | |
| is first. Returns whether the wake won. | |
| Races the injected ``self._sleep`` seam against ``_retry_event`` rather | |
| than replacing it: the test suite fakes ``sleep`` to control (and end) | |
| the backoff wait, and a wait that bypassed it outright would silently | |
| strand every one of those tests. Whichever side loses is cancelled. | |
| """ | |
| sleep_task = asyncio.ensure_future(self._sleep(delay)) | |
| wake_task = asyncio.ensure_future(self._retry_event.wait()) | |
| try: | |
| done, pending = await asyncio.wait( | |
| {sleep_task, wake_task}, return_when=asyncio.FIRST_COMPLETED) | |
| for task in pending: | |
| task.cancel() | |
| await asyncio.gather(task, return_exceptions=True) | |
| if sleep_task in done and not sleep_task.cancelled(): | |
| # Preserve the pre-existing behaviour of a sleep seam that raises — | |
| # `await self._sleep(delay)` used to propagate it directly. | |
| exc = sleep_task.exception() | |
| if exc is not None: | |
| raise exc | |
| woken = wake_task in done | |
| if woken: | |
| self._retry_event.clear() | |
| return woken | |
| finally: | |
| for task in (sleep_task, wake_task): | |
| if not task.done(): | |
| task.cancel() | |
| await asyncio.gather(sleep_task, wake_task, return_exceptions=True) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/ws_json_client.py around
lines 295 - 323, Update _wait_for_retry to wrap the asyncio.wait flow in a
finally block that cancels and awaits both sleep_task and wake_task whenever the
method is cancelled or exits early. Preserve the existing exception propagation,
retry-event clearing, and wake-result behavior while ensuring no child task
remains pending.
…and diagnostic hygiene (#135) Review findings applied: - retry_now() now returns bool through both layers (client accepted the poke or declined: not running / closing / halted / no loop), and _install_bridge_node branches its final line on the answer — "reconnecting now" only when the poke actually landed, "reload the plugin to reconnect" otherwise. The old line claimed reconnection over a halted client whose run loop had exited (#154's shape). - ExportBridge.retry_now(): local-capture of self.client (the file's own idiom) closes the TOCTOU where stop()/_stop_soon() nulls it from another thread mid-call — the AttributeError previously landed in the install worker's broad handler as a WRONG install-failure message. - Stale-event skip closed at both ends: _wait_for_retry clears the event BEFORE the sleep-side raise (both tasks can complete at once), and _mark_connected clears it for the one path no wait ever consumes (close() then a fresh run()). An armed poke can no longer skip a later, unrelated backoff. - call_soon_threadsafe wrapped for the loop-closed-under-us race (shutdown during install): declined poke, not a RuntimeError on the install thread. - The wake path re-arms the repeated-failure diagnostic: "the install succeeded, we poked, and it STILL won't connect" is exactly when the reason must re-surface; a latched streak stayed silent. - Comment on the loser-cancellation await: it is our own cancel, not the outer task's (which propagates from asyncio.wait), and a seam refusing cancellation would wedge by its own fault. 5 new/extended tests (2283 Python). Three mutations killed: clear-on-wake removed -> cuts-the-wait + rearm tests fail; unconditional message -> DECLINED-branch test fails; rearm dropped -> its test fails. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
There was a problem hiding this comment.
♻️ Duplicate comments (1)
indigo-matter.indigoPlugin/Contents/Server Plugin/ws_json_client.py (1)
303-340: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel both child tasks when
_wait_for_retry()is cancelled.
asyncio.wait()does not cancel its child tasks when the awaiting task is cancelled. Ifrun()is cancelled while Line 314 awaits,sleep_taskandwake_taskstay pending.wake_taskwaits on_retry_event, which nothing sets during teardown, so it is destroyed while pending. Wrap the body intry/finallyand cancel both tasks there.The clear-before-raise ordering at Lines 327-339 is correct and should be kept.
Proposed fix
sleep_task = asyncio.ensure_future(self._sleep(delay)) wake_task = asyncio.ensure_future(self._retry_event.wait()) - done, pending = await asyncio.wait( - {sleep_task, wake_task}, return_when=asyncio.FIRST_COMPLETED) - for task in pending: - task.cancel() - try: - await task - # The task being awaited is one WE just cancelled — the outer - # task's own cancellation propagates from `asyncio.wait` above, so - # this is not the swallow-a-real-cancel shape the explicit re-raise - # at the top of _run_loop guards against. A sleep seam that refused - # this cancellation would wedge here by its own fault, not ours. - except asyncio.CancelledError: - pass - woken = wake_task in done - if woken: - self._retry_event.clear() - if sleep_task in done and not sleep_task.cancelled(): - exc = sleep_task.exception() - if exc is not None: - raise exc - return woken + try: + done, _pending = await asyncio.wait( + {sleep_task, wake_task}, return_when=asyncio.FIRST_COMPLETED) + woken = wake_task in done + if woken: + # Cleared BEFORE the sleep-side raise below: both can + # legitimately be `done` at once. + self._retry_event.clear() + if sleep_task in done and not sleep_task.cancelled(): + exc = sleep_task.exception() + if exc is not None: + raise exc + return woken + finally: + # Covers BOTH the loser of the race and an outer cancellation, + # which `asyncio.wait` does not propagate to its children. + for task in (sleep_task, wake_task): + if not task.done(): + task.cancel() + await asyncio.gather(sleep_task, wake_task, return_exceptions=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/ws_json_client.py around lines 303 - 340, Update _wait_for_retry to wrap the asyncio.wait and result-handling logic in try/finally, and in the finally block cancel and await both sleep_task and wake_task so cancellation of the outer run cannot leave either child pending. Preserve the existing clear-before-raise ordering and exception propagation behavior.
🧹 Nitpick comments (1)
tests/test_ws_json_client.py (1)
390-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a shared fixture value for the attach result.
Add/update a golden-frame fixture entry for the attach result and use a common helper for the repeated inline handshake/responder frames so tests share the same source of truth.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_ws_json_client.py` around lines 390 - 399, Update the connect test setup around connect to use the shared fixture value for the attach result instead of the inline responder payload. Add or update the golden-frame fixture entry for the attach response, and reuse the existing common helper for the handshake and responder frames so this test and related tests share one source of truth.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@indigo-matter.indigoPlugin/Contents/Server` Plugin/ws_json_client.py:
- Around line 303-340: Update _wait_for_retry to wrap the asyncio.wait and
result-handling logic in try/finally, and in the finally block cancel and await
both sleep_task and wake_task so cancellation of the outer run cannot leave
either child pending. Preserve the existing clear-before-raise ordering and
exception propagation behavior.
---
Nitpick comments:
In `@tests/test_ws_json_client.py`:
- Around line 390-399: Update the connect test setup around connect to use the
shared fixture value for the attach result instead of the inline responder
payload. Add or update the golden-frame fixture entry for the attach response,
and reuse the existing common helper for the handshake and responder frames so
this test and related tests share one source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fabd1c2a-635b-4934-a22e-bc204f651bc9
📒 Files selected for processing (7)
indigo-matter.indigoPlugin/Contents/Server Plugin/export_bridge.pyindigo-matter.indigoPlugin/Contents/Server Plugin/plugin.pyindigo-matter.indigoPlugin/Contents/Server Plugin/ws_json_client.pytests/fakes.pytests/test_export_bridge.pytests/test_pairing_menu.pytests/test_ws_json_client.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/fakes.py
- indigo-matter.indigoPlugin/Contents/Server Plugin/plugin.py
- tests/test_pairing_menu.py
- tests/test_export_bridge.py
simons-plugins
changed the title
The previous merge commit left the conflict markers in the plist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
simons-plugins
added a commit
that referenced
this pull request
Aug 9, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
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
1 participant
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.
Closes #135.
What
After Install/update the Matter export bridge succeeded, the plugin could take up to 30s to attach — the reconnect backoff had grown to its 30s cap while the package was missing, and the post-install wait just ran out the remaining delay while the log said "reconnects automatically". Observed live: 29s of apparent nothing after a successful first install.
How
WsJsonClient.retry_now()— thread-safe wake-up: the npm install runs on a plainthreading.Thread, so the client captures its loop inrun()and the poke goes throughcall_soon_threadsafe. Waking also resets the attempt counter: the immediate retry and any later failures start from 1s again, because the history that grew the delay (missing package) is stale the moment the node is reinstalled._sleepseam against the event rather than replacing it — the test suite fakessleepto control (and end) scenarios, and a wait that bypassed it would strand that whole suite. Loser cancelled; a raising sleep still propagates as before.resume()has zero callers, so a version-mismatch halt survives its own prescribed remedy (the install menu). Filed as Version-mismatch halt survives its own prescribed remedy — resume() has no callers #154 rather than smuggled in here.ExportBridge.retry_now()passes through when a client exists (XG5 — none while nothing is exported);_install_bridge_nodepokes on both success terminals only (fresh bootstrap and restart), never on failure or nothing-exported paths; the closing line now says "reconnecting now".Tests
13 new (2278 Python total; TS untouched): backoff cut short (gated fake sleep proves it's the poke, not natural completion), next-delay-back-to-1s, harmless-while-connected, halted-stays-halted, real-foreign-thread wiring, the export-bridge pass-through pair, and all seven install-worker branches. Mutation-verified both ways: dropping the attempt reset yields
[1,2,4,8]not[1,2,4,1]; dropping the poke fails both success-terminal tests.Version
2026.8.10 → 2026.8.12(.11rides on the open #153 branch).🤖 Generated with Claude Code
https://claude.ai/code/session_018Unpt5UPRdLoZkahH6a4gL
Summary by CodeRabbit
New Features
Bug Fixes
Chores