Repository navigation
Stop credential-refresh reconnect cancelling itself; one recovery signal per failure - #13
Merged
Merged
Conversation
) connect() fired the reconnect-failed callback and then raised for the same subscribe failure, arming two independent recovery attempts for one outage. The raise is now the single recovery signal - the callback stays reserved for the post-resume resubscribe-failure path, which has no caller to raise to. _schedule_credential_refresh unconditionally cancelled the stored credential_refresh_task, including when that task was the one currently running it (reached via call_soon_threadsafe from the _connect_mqtt executor thread mid-failure) - self-cancelling the attempt before it could arm a retry, leaving zero retries armed. A credential_refresh_running flag, set for the duration of the task's own _async_refresh_and_reconnect call, guards the cancel so the running attempt is never dropped. Added deterministic regression coverage for both fixes plus the previously-untested _schedule_mqtt_retry cancel-previous line, and removed the reconnect_failed_callback no-op stub from test_a_fully_failed_subscribe_does_not_reset_the_backoff now that the real callback path no longer double-schedules. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG
…#11 round 2) - get_coordinator no longer swallows a failed setup connect: it now runs through the same count/log/_schedule_mqtt_retry path as every other reconnect failure, via a shared _record_mqtt_failure_and_retry helper. - Deleted the credential_refresh_running flag. _proactive_refresh instead removes itself from the "credential_refresh_task" slot before handing off to the shared single-flight reconnect via _trigger_mqtt_reconnect, so a nested reschedule (HA starts background tasks eagerly) has nothing of its own to cancel - proven with an adversarial test that fails when the self-removal is dropped. - Rewrote the credential-refresh-reconnect test to drive the real chain end to end instead of mocking the scheduler and duplicating its logic in the test body. - Fixed the retry-cancellation test to use a fast real backoff instead of a 30s real sleep, so a broken cancel guard fails on the assertion, not a timeout. - Tightened the watchdog test to assert exactly one retry is armed (attempts == 1, one new captured coroutine), catching a double-arm mutant the previous assertions missed. - Fixed a test that shadowed the builtin `call` name and asserted nothing useful; deduplicated two near-identical subscribe-failure test setups into a shared fixture; moved scattered datetime imports to the top of the test file. - Added `make lint-dup` (pylint duplicate-code) to catch this class of Sonar failure locally before push. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG
Rework the credential-refresh/reconnect handoff so it can't lose a refresh or orphan a retry: - _trigger_mqtt_reconnect now creates the handoff task with eager_start=False, so it can never run inline on the creating coroutine's own stack. That removes the eager-orphan race where a finished handoff task overwrote the retry slot before the retry's own assignment ran (RED: an in-flight retry disappearing from the slot after a deferred handoff), and it lets the self-removal workaround in _proactive_refresh - and the artificial nested- reschedule test that only existed to justify it - be deleted outright: production never re-enters _schedule_credential_refresh from inside its own frame. - _trigger_mqtt_reconnect now reports whether it actually scheduled an attempt. _proactive_refresh uses that: when it deferred to an in-flight retry, it awaits that real task and rechecks, instead of vanishing (RED: a proactive refresh that defers to an in-flight retry disappearing, leaving expiring credentials with nothing armed). - get_coordinator's setup-failure path now goes through _record_mqtt_failure_and_retry so a rejected-subscribe setup arms exactly one retry (RED: deleting the setup-retry call left all prior tests green - there was no test proving it). - Root-caused and fixed a genuine reconnect-storm flake (reproduced standalone at ~1-in-5 to 1-in-7): _connect_mqtt's failure path unconditionally re-armed the credential-refresh cycle with the same still-expiring credentials, fighting the backoff retry's sleep. The reschedule now only happens on the success path; verified 500/500 clean under 4-way concurrent load. - Consolidated setup/reconnect failure logging onto _record_mqtt_failure_and_retry as the single WARNING emission point, demoted now-redundant per-cause logs to DEBUG, and dropped the traceback noise on expected failures. - Deduplicated the two lint-dup violations pre-dating this PR (test_diagnostics_logging.py/test_setup_logging.py's secret entry setup, test_api_mqtt_retry.py/test_api_pending_overlay.py's wired MQTT client setup) into shared conftest fixtures. - Assorted prose cleanup: removed comments and docstrings that only restated the code, renamed a fixture instead of documenting around its name. Full suite: 259 passed, 0 failures across 50 runs under 4-way parallel load and 500 isolated runs of the previously-flaking acceptance test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG
…ain lost-refresh coverage, dedup setup tests
- api.py:1431 replaced the suppress(CancelledError)-wrapped await of the
in-flight attempt with asyncio.wait({in_flight}) - cancelling the refresh
task no longer propagates cancel into the awaited attempt.
- Lifted _proactive_refresh to module-level _async_refresh_credentials_after
(Sonar S3776), deleted the now-unreachable else-sleep branch and its
MQTT_CREDENTIAL_REFRESH_RETRY_DELAY constant.
- Removed the initial_setup flag from _record_mqtt_failure_and_retry; setup
now logs its own WARNING and calls the new non-logging _arm_mqtt_retry.
- mqtt_client.py subscribe: partial failure now logs one WARNING with the
success ratio and failed topics, exercised through both connect() and the
resume path; full failure still raises as before.
- _connect_mqtt's no-credentials path includes the underlying (already
redacted upstream) exception in mqtt_last_connect_error.
- _trigger_mqtt_reconnect clears mqtt_retry_sleeping right after cancelling
the sleeping task, so two triggers in the same tick don't both log
"Preempting".
- Replaced the fake deferred-trigger test with a real-chain test proving the
refresh re-triggers its own reconnect after an in-flight attempt fails.
- Added stubbed_coordinator_setup + _fresh_aws_credentials in conftest.py to
dedup the setup-test boilerplate (Sonar duplication).
- Bounded the cancel-predecessor wait in the credential-refresh dedup test
with asyncio.wait_for(1) so a regression fails fast, not after 30s.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG
Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
… mutant bounds, test tidy - Add _format_error_reason(err) -> str(err) or type(err).__name__, used at the no-credentials, retry-warning and setup-failure sites so an exception with no message (e.g. TimeoutError()) still names its type. - Extract get_coordinator's MQTT-connected and setup-failure branches into _await_initial_mqtt_shadow / _handle_mqtt_setup_failure to bring cognitive complexity back under the Sonar S3776 threshold. - Have _arm_mqtt_retry return the attempt number and drop the duplicate computation in _record_mqtt_failure_and_retry. - Cover the stored-reason pop on the no-exception reconnect-failure path (kills mutant N5). - Restructure _async_refresh_credentials_after per spec and fix its and _trigger_mqtt_reconnect's docstrings. - N3a (drop the wait) leaves the credential-refresh loop with no await point at all, so it cannot be bounded by asyncio.wait_for from the same event loop; bound that test with @pytest.mark.timeout(2) instead. N7 (drop the loaded check) turns out to raise KeyError almost immediately once the entry's store is torn down and re-created empty, so its test asserts that observable failure directly with no marker needed. - Merge the two overlapping refresh-cancellation tests, use _fresh_aws_credentials over inline expiry dicts, wrap retry-task cleanup in try/finally, and derive the partial-subscribe-failure count from _SUBSCRIBE_TOPICS instead of a hardcoded "3/4". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
…ntion flakes - Fix a double-format bug where a blank exception reason logged as the literal word "str": store _format_error_reason(err) at the MQTT-connect except site instead of str(err), and stop re-formatting the already-text reason at the setup-failure warning site. - Cover the reconnect-path warning (_record_mqtt_failure_and_retry) with a bare TimeoutError() case, proving it names the exception type rather than emitting a blank detail. - Drop the unused hass/entry parameters from the shadow-wait helper and rename both get_coordinator helpers for what they do (_wait_for_shadow_or_fall_back_to_rest, _fall_back_to_rest_and_arm_mqtt_retry); cover both of the wait helper's branches plus the inverse case that a successful setup arms no retry. - Rename and sharpen the entry-unloaded-during-refresh test so its final assertion actually discriminates (the task finished cleanly, not just that the store wasn't recreated afterwards), and use a real task instead of a bare future. - Add direct unit tests for _format_error_reason and migrate the last inline credential dicts to _fresh_aws_credentials(). - Replace the test suite's fixed 5-yield _settle() with one that polls a real condition where the assertion that follows it has one, bounded by asyncio.wait_for, to stop it under-yielding under CPU contention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
- Drop the redundant @pytest.mark.timeout(2) that raced _settle's internal wait_for(2) and could hang mutant runs 40s+; the global pytest.ini timeout is now the sole backstop for non-preemptible mutants (N3a), while yieldable mutants (N8) fail in ~1.3s. - Pin the shadow-wait loop in _wait_for_shadow_or_fall_back_to_rest to named constants (MQTT_SHADOW_WAIT_ATTEMPTS/INTERVAL) and add a test where shadow data arrives mid-wait, killing the no-loop, iteration- count, early-break and zero-sleep mutants. - Format the reconnect exception at the catch site so _record_mqtt_failure_and_retry takes reason: str | None directly, dropping the isinstance dispatch. - Test tidying: drop redundant asserts that repeated the preceding _settle predicate, drop _settle's unused timeout kwarg, add in-flight-parked preconditions before the bare _settle() calls. - Pin an exact reconnect-warning message (attempt number + delay) to kill mutants that bump the attempt count or scale the delay. - Rename the credential-refresh timer task to exo_pool_credential_refresh_timer so it no longer collides with the reconnect handoff task name. - Factor the duplicated real-MQTT-client fixture setup into one helper, and correct the README's false claim that lint-dup mirrors the SonarCloud gate (pylint's duplicate check only compares across files). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
- Monkeypatch MQTT_SHADOW_WAIT_ATTEMPTS/INTERVAL to distinctive values (5, 0.123) in the partway-arrival test so it proves the loop reads the module names and breaks early with headroom left, without hardcoding the production defaults into the behavioural assertion. - Add a small constants-pinning test for the production defaults themselves (20 attempts of 0.5s) - the partway test's monkeypatch otherwise masks any mutation to the constants' own values, and the existing without-shadow-data test's assertion is self-referential against the same constants, so neither could catch that mutant. - Assert asyncio.sleep is never awaited when shadow data is already present at setup, killing the sleep-before-data-check reordering. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG Signed-off-by: Andrew Brainwood <andrew.brainwood@gmail.com>
abrainwood
marked this pull request as ready for review
September 15, 2026 22:09
|
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.



Closes #11.
What
A failed MQTT reconnect during the scheduled credential refresh could cancel itself and leave no retry armed. MQTT then stayed down until the next refresh, about 55 minutes later, with REST polling hourly in between. This PR makes that path always end with exactly one recovery attempt in flight.
connect()no longer fires the reconnect-failed callback as well as raising. The raise owns recovery; the callback serves only the connection-resumed path._async_refresh_credentials_aftercalls_trigger_mqtt_reconnect(name="exo_pool_credential_refresh"), and the reconnect task is created witheager_start=False, so it can't run inside its caller's frame and cancel itself.asyncio.wait({in_flight})rather than awaiting the attempt directly. So cancelling the refresh, whether by a reschedule or by HA stopping, never cancels the attempt, and the refresh's own cancel isn't swallowed. After the attempt ends, the refresh triggers its own reconnect. If the entry was unloaded in the meantime, it exits without recreating the entry store._format_error_reasonfalls back to the exception type name, so a bareTimeoutError()logs "TimeoutError" instead of an empty or wrong reason. Every exception that can reach a reason string was traced, and none can carry a credential.get_coordinatorwas split to clear Sonar S3776.exo_pool_credential_refresh_timer, so HA's task listings and shutdown logs can tell it apart from the reconnect handoff task (exo_pool_credential_refresh). Nothing in production matches on task names.No shared "in progress" flag is used. That was tried twice during review, and both times it broke recovery.
Tests
276 passing, stable across 20 runs with
-n 4. The earlier round was also stable across 20 runs with-n 8under CPU contention.asyncio.waitpopvsgeton the stored reasontimeout(2)mark was tried and then removed: it raced the test's own 2-second wait, so a regression could hang the run instead of failing it._settle()now waits on a real condition, bounded bywait_for, instead of a fixed number of yields. The three plain calls still yield a fixed number of times; that's deterministic, because nothing on those paths touches the executor.test_a_fully_failed_subscribe_does_not_reset_the_backoffnow runs deterministically on the real path.tests/conftest.py.make lint-dupwas added; note that it does not see every block Sonar flags.Not in this PR
_connect_mqttrecreating the entry store after unload.🤖 Generated with Claude Code
https://claude.ai/code/session_019FJHinb3pMJEwTKrnMXMcG