Fix mobile_app local push failing permanently after a missed confirmation - #176833
Fix mobile_app local push failing permanently after a missed confirmation#176833markfrancisonly wants to merge 11 commits into
Conversation
A local push that is not confirmed within PUSH_CONFIRM_TIMEOUT now falls back to cloud for that single message only; handle_push_failed no longer calls async_teardown() on the whole push_notification_channel. A missing confirmation is a per-message signal (device briefly asleep, slow to wake, a buffering reverse proxy), not proof the channel is dead. Tearing the channel down on one late confirm without notifying the subscriber left local-push-only devices, which have no cloud fallback, permanently reporting "not connected to local push notifications" until the app was restarted. Genuinely dead connections are still handled: a closed socket tears the channel down through the subscription lifecycle, and the websocket server heartbeat reaps a silently dead one within ~1-2 minutes. Fixes home-assistant#176371 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Keeps mobile app local-push channels registered when a notification confirmation times out.
Changes:
- Falls back only the unconfirmed notification to cloud delivery.
- Updates tests for channel retention, confirmation timers, and teardown flushing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
homeassistant/components/mobile_app/push_notification.py |
Removes channel teardown on confirmation timeout. |
tests/components/mobile_app/test_notify.py |
Verifies revised confirmation and fallback behavior. |
A single missed confirm still falls back via cloud for that message only, but consecutive timeouts now mark the channel degraded: cloud-capable targets are routed straight to cloud instead of paying the confirm timeout per message. iOS deliberately leaves background Live Activity updates unconfirmed so they are delivered through the APNs push-to-start path (home-assistant/iOS#4857); an always-live channel made every such update wait out the full timeout. A degraded channel probes local delivery again after a cooldown, and a timely confirmation restores local routing. Local-push-only registrations are never bypassed, so the channel staying registered keeps the fix for the permanent "not connected to local push notifications" failure.
…//github.com/markfrancisonly/home-assistant-core into mobile-app-keep-push-channel-on-late-confirm
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
homeassistant/components/mobile_app/push_notification.py:20
- Restore the omitted Pull Request template items before approval. The description deletes the unchecked requirements/changelog dependency items and the final “reviewed two other open pull requests” item, although repository policy requires every template section and checkbox to remain present.
# How long a degraded channel routes via cloud before the next send probes
# local delivery again.
PUSH_DEGRADED_PROBE_INTERVAL = 300 # seconds
Keeping the channel non-degraded for the whole probe confirmation window let every send during those 10 seconds attempt local delivery, so a burst could create several concurrent probes and duplicate notifications. The probe interval now arms a one-shot permit consumed by the next send; all other sends keep routing via cloud until a probe is confirmed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
homeassistant/components/mobile_app/push_notification.py:16
- Restore the omitted PR-template checklist entries before merging. The description is missing both dependency-related checklist items and the final “reviewed two other open pull requests” item; unchecked items must remain present even when they do not apply.
PUSH_DEGRADED_AFTER_TIMEOUTS = 2
Both notify paths keep attempting local delivery for registrations without a cloud push URL even after the degraded threshold; exercise that guard so it cannot be silently simplified away.
Debug logs on degrade, probe and restore make routing changes visible when diagnosing delivery latency. The new test proves a timely confirm between two timeouts resets the count, so isolated misses on either side of a delivery never open the breaker.
|
# PR #176833 use-case readout (Sol 5.6)
Cloud means APNs on iOS and FCM on cloud-capable Android builds. Messages already in flight when the breaker opens are not limited to two; each keeps its own timeout. Overall, the PR is a net improvement. It keeps channel lifetime tied to the WebSocket, falls back individual messages, and uses a reversible circuit breaker for repeated failures. A future explicit negative acknowledgement would handle iOS Live Activity fallback more precisely. References: Core PR, live-socket issue, Core-restart issue, Android reconnect PR, iOS Live Activity PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
homeassistant/components/mobile_app/notify.py:115
- Avoid scheduling a cloud fallback for local-push-only registrations. When
cloud_capableis false, this still retains_send_messageand invokes it on every missed confirmation, but_send_messageindexes the absentATTR_PUSH_URLatnotify.py:290, so each timeout raises an unhandledKeyError; the new test masks this by patching the helper. Pass an absent/no-op fallback for local-only channels and exercise the real timeout path.
not cloud_capable or push_channel.async_should_send_local()
):
push_channel.async_send_notification(
data,
partial(_send_message, self._session, self._config_entry),
homeassistant/components/mobile_app/notify.py:234
- Do not attach a remote fallback for local-push-only targets. This branch deliberately sends locally when
cloud_capableis false, yet a missed confirmation invokes_async_send_remote_message_target, whose_send_messagecall indexes the absentATTR_PUSH_URLatnotify.py:290; because onlyHomeAssistantErroris caught, every timeout produces an unhandledKeyError. Pass an absent/no-op fallback and cover the unmocked timeout path.
not cloud_capable or push_channel.async_should_send_local()
):
push_channel.async_send_notification(
data,
partial(self._async_send_remote_message_target, entry),
Tracks each re-subscription attempt as a temporary ActiveMessage.Reconnecting entry keyed by the id the server will use, while the original subscription stays under its old id until the attempt is acknowledged and promoted. This replaces the duplicate Subscription entries (and the awaitClose cleanup loop) previously created during re-subscription, so findSubscription stays unambiguous and orphans cannot linger. Events arriving for the new id before the acknowledgement is processed are routed to the original subscription's flow. Also hardens the restore loop per review: - close() cancels the running restore loop so a shutdown cannot leave it re-establishing the connection it just closed - a subscription the server rejects is retried indefinitely with capped backoff instead of being silently abandoned; dropping it would leave its collector waiting on a flow that can never emit again - an unanswered resubscription cancels the socket: the server may have accepted it with the answer lost, so restoration resumes on a clean connection instead of risking a duplicate registration; only the connection the attempt was sent on is cancelled, never a replacement established while waiting - closing a subscription also unsubscribes the pending id of any attempt still in flight for it - the worker's restart of stopped collectors is now covered by a test, with the work dispatcher injectable for virtual time - shouldAttemptReconnect names the reconnect condition and reconnectJob is annotated with GuardedBy(connectedMutex) - drops the changelog entry Client half of the local push fix: home-assistant/core#176833 keeps the server-side push channel registered across confirm timeouts, while this change makes the client detect dead sockets and retry until its subscriptions are restored. A successful re-subscribe replaces the server's push channel, which also resets its degraded state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both notify paths passed the cloud send callable as the confirm-timeout fallback even when the registration has no cloud push configuration. That callable unconditionally reads the push URL and token from the app data, so for a local-push-only registration every missed confirm — and every teardown flush of a pending confirm — raised an unhandled KeyError in the scheduled callback. The fallback is now None for local-push-only registrations and the channel simply keeps the message local-only: the miss still counts toward the degraded threshold and the teardown flush completes, there is just no cloud delivery to fall back to. The degraded-channel test that mocked the cloud send (masking the KeyError) now asserts no cloud call is attempted, and a new test exercises the missed confirm and the teardown flush without any mock in the way. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Was this tested on real devices or just on core side? |
This should be address, the client should be able to determine if it's a retry notification and check if it was already displayed or not. Otherwise if I understand well after this PR users will start to see duplicate notifications (even if it improve the reliability). |
Proposed change
Local push is what lets a notification reach a device without ever touching a cloud — this change makes that local-first path resilient, so devices stay on it instead of being silently forced onto cloud delivery, or losing notifications entirely, because of a single late acknowledgment.
What's broken
Local push notifications are expected to be acknowledged by the app within 10 seconds. When a phone confirms late — asleep, slow to wake, behind a buffering proxy — Home Assistant doesn't just give up on that one message. It silently kills the entire push channel. The app is never told: it keeps its socket open, believes it's still subscribed, and never re-subscribes.
For a device registered with local push only, the consequence is severe: no notifications at all until the app is force-restarted. Cloud-capable devices fare better but silently lose local delivery until an app restart. Both companion apps use confirmations, so both are exposed — iOS registrations virtually always include a cloud path (silent degradation), while Android builds without Google services are the typical local-push-only case (hard failure).
The constraint: iOS misses confirmations on purpose
While the app is suspended, iOS deliberately leaves Live Activity updates unconfirmed (home-assistant/iOS#4857) so the confirm timeout forwards them through APNs — the only path that can start a Live Activity in the background. Prompt APNs routing for unconfirmed Live Activity updates is therefore a hard requirement, not an accident of the old behavior.
The fix: a missed confirmation affects routing, never registration
Everything else is untouched: when a device unsubscribes or disconnects, messages still awaiting confirmation are sent via cloud once, exactly as before; re-subscribing replaces the channel with a fresh one; and the confirmation messages the apps already send work unchanged. Genuinely dead sockets are still reaped by the server's 55 s websocket heartbeat, typically within a couple of minutes; a complementary client-side ping is proposed in home-assistant/android#7164. Degrade / probe / restore transitions are debug-logged.
What changes in practice
Inherited limitations (unchanged by this PR)
These compromises predate the change:
Potential follow-up (not an alternative to this change): an explicit client acknowledgement protocol — "delivered, don't resend" / "can't present this, route via cloud" — would remove the inference for clients that speak it. It would layer on top of this PR rather than replace it: the server must keep timeout-based inference for older apps indefinitely, and this change is that layer. Cross-repo (core + both companions), so it belongs in an architecture discussion if there's interest.
Tests
New tests cover: a channel surviving a single missed confirmation · the full degrade → probe → recover cycle, for both ways Home Assistant sends notifications (the notify service and notify entities) · only one probe going local while the rest of a burst stays on cloud · a timely confirmation between two misses preventing degradation · local-push-only devices never being routed to cloud · unsubscribing still delivering unconfirmed messages via cloud once.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: