Repository navigation
fix: supervise WebSocket recovery per thread - #26
Closed
thomashaus wants to merge 7 commits into
Closed
thomashaus wants to merge 7 commits into
thomashaus wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRealtime recovery now tracks failures by kind and scope. WebSocket delivery uses supervised reconnects with fresh tokens, bounded backoff, cancellation, and permanent dependency handling. Tests cover notification recovery, reconnect behavior, and cleanup. ChangesRealtime recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StartDelivery
participant ReceiveLoop
participant WebSocket
participant ThreadGrant
StartDelivery->>ReceiveLoop: start supervised delivery
ReceiveLoop->>WebSocket: connect with current token
ReceiveLoop->>StartDelivery: invoke on_connected
WebSocket-->>ReceiveLoop: report transport failure
ReceiveLoop->>ThreadGrant: request fresh connection token
ThreadGrant-->>ReceiveLoop: return reconnect grant
ReceiveLoop->>WebSocket: reconnect with fresh token
StartDelivery->>StartDelivery: cancel and clean up supervisor
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 5
🤖 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 `@turn_taking/delivery.py`:
- Around line 101-110: The ownership guard on line 103 that checks if task is
None or current is task only protects the DELIVERY_TASKS cleanup, but the
subsequent cleanup operations on lines 105-110 that remove DELIVERY_READY,
ROUTES, SESSIONS, and ws notify state execute unconditionally. Extend the
ownership check to guard all cleanup operations in the _cleanup_delivery_state
function by adding an early return when the caller is not the current task
owner, ensuring a superseded supervisor cannot wipe the live supervisor's state.
In `@turn_taking/hooks.py`:
- Line 135: Undo answer suppression whenever realtime response construction or
scheduling fails. Add an _unsuppress_answer helper beside _suppress_answer that
discards the normalized pending answer, then update the _respond construction
and scheduling flow to catch failures, close the coroutine when scheduling
fails, call _unsuppress_answer, and log the failure before returning or
continuing.
In `@turn_taking/notify.py`:
- Around line 168-178: The recovery message in recovered_http must describe only
HTTP/API recovery, not claim that turn-taking is fully active. Update the _send
text scheduled after clearing recovered_kinds to explicitly indicate HTTP/API
recovery while preserving the existing ws scope and recovery logic.
- Around line 79-94: Update _fire and the recovery/clear paths so _last_by_kind
timestamps persist after a scope recovers or is cleared, allowing the cooldown
to span disconnect/reconnect flaps. Track whether each outage actually emitted
an alert, and have recovered, clear, and recovered_http emit recovery
notifications only for outages with a corresponding alert; cooldown-suppressed
outages must not produce unpaired recovery messages.
In `@turn_taking/service.py`:
- Around line 249-253: Treat a failed websockets import in the delivery
supervision flow as a permanent dependency error: raise a dedicated exception
from the import block, handle it before the generic reconnect logic in
_supervise_delivery, discard DELIVERY_READY, and exit that thread without
retrying or emitting WS_LOST. Preserve the existing reconnect behavior for other
delivery failures.
🪄 Autofix (Beta)
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: 1612c150-c384-4abc-b205-e2869f8a0ea7
📒 Files selected for processing (6)
tests/test_recovery.pyturn_taking/delivery.pyturn_taking/hooks.pyturn_taking/notify.pyturn_taking/service.pyturn_taking/state.py
| epoch = latest | ||
| if chat: | ||
| _suppress_answer(chat, draft) # drop the send whose content matches this answer | ||
| _suppress_answer(chat, draft) # drop only when realtime delivery is ready |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Suppression is registered before delivery is scheduled.
Line 135 marks the raw answer for suppression. Delivery is only attempted afterwards. Line 141 builds _respond outside the try block, and the scheduling at Lines 143-146 can fail. In either case the raw send stays suppressed and no bubble is delivered, so the turn produces silence.
The orphaned entry also stays in state.PENDING_ANSWERS[chat]. It then suppresses any later identical answer for that chat.
Undo the suppression when scheduling fails.
🛠️ Proposed fix
Add an inverse helper next to _suppress_answer at Line 22:
def _unsuppress_answer(chat_id: str, content: str) -> None:
"""Release a suppression when realtime delivery could not be scheduled."""
state.PENDING_ANSWERS.get(chat_id, set()).discard((content or "").strip())Then cover both the coroutine construction and the scheduling, and roll back on failure (Lines 141-149):
try:
_coro = _respond(sid, draft, epoch,
_with_memory(_build_system_prompt_for_turn_taking(None, sid), sid), meta)
except Exception as e:
_unsuppress_answer(chat, draft)
_log.warning("tt transform: could not build _respond: %s", e)
return None
try:
if state.LOOP is not None:
asyncio.run_coroutine_threadsafe(_coro, state.LOOP) # bubbles delivered via WS
else:
asyncio.create_task(_coro) # fallback: already in a loop
except Exception as e:
_coro.close()
_unsuppress_answer(chat, draft)
_log.warning("tt transform: could not schedule _respond: %s", e)🤖 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 `@turn_taking/hooks.py` at line 135, Undo answer suppression whenever realtime
response construction or scheduling fails. Add an _unsuppress_answer helper
beside _suppress_answer that discards the normalized pending answer, then update
the _respond construction and scheduling flow to catch failures, close the
coroutine when scheduling fails, call _unsuppress_answer, and log the failure
before returning or continuing.
Comment on lines
+168
to
178
| def recovered_http() -> None: | ||
| """Resolve HTTP/API failures only; never clear live realtime failures.""" | ||
| try: | ||
| with _LOCK: | ||
| if not _active_kinds: | ||
| recovered_kinds = [kind for kind in _active_scopes if kind != "ws"] | ||
| if not recovered_kinds: | ||
| return | ||
| _active_kinds.clear() | ||
| _last_by_kind.clear() # let the next failure alert immediately | ||
| for kind in recovered_kinds: | ||
| _active_scopes.pop(kind, None) | ||
| _last_by_kind.pop(kind, None) | ||
| _schedule(lambda: _send("✅ Humalike recovered — turn-taking active again.")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The HTTP recovery text overstates the state.
recovered_http clears non-ws kinds only, and it deliberately leaves live realtime failures active. The message still says "turn-taking active again". If a ws scope is still failing, that text contradicts the WS_LOST alert that is still in force.
Scope the text to the HTTP path.
✏️ Proposed text fix
for kind in recovered_kinds:
_active_scopes.pop(kind, None)
_last_by_kind.pop(kind, None)
- _schedule(lambda: _send("✅ Humalike recovered — turn-taking active again."))
+ realtime_down = bool(_active_scopes.get("ws"))
+ text = ("✅ Humalike API recovered — realtime connection still reconnecting."
+ if realtime_down else
+ "✅ Humalike recovered — turn-taking active again.")
+ _schedule(lambda: _send(text))📝 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
| def recovered_http() -> None: | |
| """Resolve HTTP/API failures only; never clear live realtime failures.""" | |
| try: | |
| with _LOCK: | |
| if not _active_kinds: | |
| recovered_kinds = [kind for kind in _active_scopes if kind != "ws"] | |
| if not recovered_kinds: | |
| return | |
| _active_kinds.clear() | |
| _last_by_kind.clear() # let the next failure alert immediately | |
| for kind in recovered_kinds: | |
| _active_scopes.pop(kind, None) | |
| _last_by_kind.pop(kind, None) | |
| _schedule(lambda: _send("✅ Humalike recovered — turn-taking active again.")) | |
| def recovered_http() -> None: | |
| """Resolve HTTP/API failures only; never clear live realtime failures.""" | |
| try: | |
| with _LOCK: | |
| recovered_kinds = [kind for kind in _active_scopes if kind != "ws"] | |
| if not recovered_kinds: | |
| return | |
| for kind in recovered_kinds: | |
| _active_scopes.pop(kind, None) | |
| _last_by_kind.pop(kind, None) | |
| realtime_down = bool(_active_scopes.get("ws")) | |
| text = ("✅ Humalike API recovered — realtime connection still reconnecting." | |
| if realtime_down else | |
| "✅ Humalike recovered — turn-taking active again.") | |
| _schedule(lambda: _send(text)) |
🤖 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 `@turn_taking/notify.py` around lines 168 - 178, The recovery message in
recovered_http must describe only HTTP/API recovery, not claim that turn-taking
is fully active. Update the _send text scheduled after clearing recovered_kinds
to explicitly indicate HTTP/API recovery while preserving the existing ws scope
and recovery logic.
Author
Independent implementation gates are complete at
The PR is mergeable, but |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/test_recovery.py`:
- Around line 13-28: Update the httpx setup before the isolated imports to
attempt importing the real package first and install the SimpleNamespace stub
only when that import raises ImportError. After the imports of state, notify,
service, and delivery, restore sys.modules["httpx"] by removing the temporary
stub so later tests can load the real package; retain the imported service
module’s existing reference.
In `@turn_taking/delivery.py`:
- Around line 138-144: Update the permanent WebSocket failure path around
WebSocketDependencyError and _cleanup_delivery_state so task cleanup does not
clear the active "ws" failure scope. Restrict notify.clear("ws", thread_id) to
intentional shutdown or successful reconnection, preserve the permanent alert
after cleanup, and extend the permanent-failure regression test to assert
notify.is_active("ws", "thread-a").
🪄 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: 41d41257-625d-45e0-ad06-ab8bf15e4900
📒 Files selected for processing (6)
plugin.yamltests/test_recovery.pyturn_taking/delivery.pyturn_taking/notify.pyturn_taking/service.pyturn_taking/state.py
🚧 Files skipped from review as they are similar to previous changes (2)
- turn_taking/notify.py
- turn_taking/service.py
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.
Summary
Validation
Team/Mack - Automation Specialist/journal/2026-07-31-realtime-recovery-state-must-be-connection-scoped.mdDeployment boundary
The active Odin Hermes installation does not currently have Humalike installed/enabled or linked to a Humalike account. Live deployment proof requires explicit approval for first-start messaging autoconfiguration, interactive account linking, and a controlled gateway restart. No live state was changed without that approval.
Control issue
Summary by CodeRabbit