Always cancel charge point tasks when stopping

[KingHavok]

Split out of #2012 — @drc38 asked for the stop() fix as a separate PR.

The problem

ChargePoint.stop() closes the websocket before cancelling self.tasks:

async def stop(self):
    self.status = STATE_UNAVAILABLE
    if self._connection.state is State.OPEN:
        await self._connection.close()   # <-- if this raises...
    for task in self.tasks:              # <-- ...this never runs
        task.cancel()

If the close raises or is cancelled, the tasks are never cancelled. self.tasks holds monitor_connection(), which loops forever pinging the connection and writing metrics — so a charge point that is supposed to have stopped keeps running against a connection it no longer owns.

There are three call sites, and all of them are paths where something has already gone wrong, which is exactly when a close is most likely to fail:

• run()'s finally (chargepoint.py) — reached after the connection dropped
• reconnect() — the old connection is being torn down
• CentralSystem.on_connect() — the rebuild path added in Fix OCPP subprotocol negotiation and version handling across reconnects #2012

Separately, self.tasks is initialised to None and only populated by run(). stop() is reachable before that, and raises TypeError: 'NoneType' object is not iterable when it is.

The fix

Move cancellation into a finally and tolerate tasks being None:

async def stop(self):
    self.status = STATE_UNAVAILABLE
    try:
        if self._connection.state is State.OPEN:
            _LOGGER.debug(f"Closing websocket to '{self.id}'")
            await self._connection.close()
    finally:
        for task in self.tasks or []:
            task.cancel()

The close exception still propagates, so callers are not deprived of the information that the close failed — only the cancellation is made unconditional.

Tests

Three tests in tests/test_charge_point_core.py:

• test_stop_cancels_tasks_when_close_fails — close raises OSError; asserts the tasks are still cancelled and that the OSError still reaches the caller
• test_stop_without_tasks_does_not_raise — stop() on a charge point that never ran
• test_stop_closes_connection_and_cancels_tasks — the normal path still closes and cancels

The first two were mutation-verified: with the old stop() restored they fail (TypeError / uncancelled tasks), and pass with the fix.

Note on #2012

CentralSystem.on_connect() currently wraps charge_point.stop() in a try/except that cancels the stale instance's tasks by hand, precisely because of this bug. With stop() fixed that inner loop is belt-and-braces — the try/except itself is still needed so a failed close doesn't abort the rebuild. I left it alone to keep this PR to the one change you asked for, but happy to simplify it in a follow-up if you'd prefer.

pre-commit run --all-files and the full test suite pass.

Summary by CodeRabbit

• Bug Fixes
  • Improved charge point shutdown reliability when the connection is already closed or cannot be closed.
  • Ensured background tasks are consistently cancelled during shutdown.
  • Prevented errors when stopping a charge point that has not started.
  • Charge points now correctly report an unavailable status when shutdown encounters a connection error.

[coderabbitai]

Warning

Review limit reached

@KingHavok, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d2473c03-f7c9-40a9-87fe-0bd0bad93091

📥 Commits

Reviewing files that changed from the base of the PR and between 0d2ddec and f7cf31c.

📒 Files selected for processing (1)

• tests/test_charge_point_core.py

📝 Walkthrough

Walkthrough

ChargePoint.stop() now conditionally closes open websocket connections and unconditionally cancels available tasks. Tests cover close failures, stopping before startup, and normal shutdown behavior.

Changes

Shutdown reliability

Layer / File(s)	Summary
Stop cleanup and validation custom_components/ocpp/chargepoint.py, tests/test_charge_point_core.py	ChargePoint.stop() cancels tasks even when websocket closure fails or no tasks exist; tests verify failure propagation, unavailable status, connection closure, and task cancellation.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: charge point tasks are always cancelled during stop.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/test_charge_point_core.py (1)

344-368: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Cover cancelled websocket closure explicitly.

This verifies OSError, but not asyncio.CancelledError, which is another path protected by the new finally block. Add a test that propagates cancellation while still asserting every task is cancelled.

Suggested test

+@pytest.mark.asyncio
+async def test_stop_cancels_tasks_when_close_is_cancelled(hass):
+    cp = _mk_cp(hass)
+    cp.tasks = [_mk_task(), _mk_task()]
+
+    async def cancelled_close():
+        raise asyncio.CancelledError
+
+    cp._connection.state = State.OPEN
+    cp._connection.close = cancelled_close
+
+    with pytest.raises(asyncio.CancelledError):
+        await cp.stop()
+
+    assert all(task.cancelled for task in cp.tasks)

🤖 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_charge_point_core.py` around lines 344 - 368, Add a sibling async
test to test_stop_cancels_tasks_when_close_fails that makes the websocket close
callback raise asyncio.CancelledError, asserts cp.stop() propagates that
cancellation, and verifies every task in cp.tasks is cancelled afterward.
Preserve the existing state setup and unavailable-status assertion as
appropriate.

🤖 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.

Nitpick comments:
In `@tests/test_charge_point_core.py`:
- Around line 344-368: Add a sibling async test to
test_stop_cancels_tasks_when_close_fails that makes the websocket close callback
raise asyncio.CancelledError, asserts cp.stop() propagates that cancellation,
and verifies every task in cp.tasks is cancelled afterward. Preserve the
existing state setup and unavailable-status assertion as appropriate.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b41aca6a-f6d9-4d76-98ca-021256462381

📥 Commits

Reviewing files that changed from the base of the PR and between 8b2fa20 and 0d2ddec.

📒 Files selected for processing (2)

• custom_components/ocpp/chargepoint.py
• tests/test_charge_point_core.py

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.04%. Comparing base (4111eef) to head (f7cf31c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2013   +/-   ##
=======================================
  Coverage   95.03%   95.04%           
=======================================
  Files          12       12           
  Lines        3004     3005    +1     
=======================================
+ Hits         2855     2856    +1     
  Misses        149      149           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[KingHavok]

Good catch — CancelledError derives from BaseException, so it's the one close failure an except Exception at a call site can't catch, which makes it exactly the path the finally exists for. Added test_stop_cancels_tasks_when_close_is_cancelled in f7cf31c, mutation-verified against the old stop().