Raise GrowattRateLimitError when a response body carries Growatt's 507 code

[proscar87]

Summary

Growatt reports rate limiting in the response body, not as an HTTP status: the request returns HTTP 200 and the payload carries success: false with msg: "507". That is the shape behind the ConfigEntryError: Growatt login failed: 507 tracebacks in home-assistant/core#176831 and home-assistant/core#174789 (see also #55). Reports indicate a 507 precedes an approximately 24 hour lockout rather than a short cooldown, so callers such as Home Assistant's growatt_server integration need to tell it apart from bad credentials or a malformed response.

Thanks to @johanzander for the correction: the first version of this PR handled HTTP 429/507 status codes and Retry-After, which Growatt does not send. That design was dropped; details in the thread below.

What this PR does

• Adds GrowattRateLimitError(error_code, error_msg=None) in exceptions.py, following the GrowattV1ApiError shape: callers read exc.error_code ("507") and exc.error_msg instead of parsing a message. The message is generic: Growatt rate limit reached: [507].
• Adds _raise_if_rate_limited(response) in base_api.py, called from the session's existing response hook before response.raise_for_status(). Growatt rate limits each endpoint separately, so the check covers every call on the session, not only login(). (OpenApiV1 subclasses GrowattApi, so its responses pass through the hook too; V1 error bodies use error_code/error_msg, so they do not match.)
• The check only fires on the exact success: false + msg: "507" combination (msg compared as a string), reading back when it is a dict and the top level otherwise. Non-JSON bodies, non-dict bodies, back as a list (plant_list) and bodies without a success key fall through untouched.
• Exports GrowattRateLimitError from the package __init__.py.
• The library does not retry. Retrying a 507 is what deepens the lockout, so backing off is left to the caller.

Behavior change

For a body with success: false and msg: "507", login() now raises GrowattRateLimitError instead of returning the dict, and any other method on the session raises in the same case. Every other failure (e.g. msg: "501", wrong password) still returns the dict as before, and HTTP errors still raise requests.exceptions.HTTPError. A consumer that inspected msg == "507" on the returned dict needs to catch the exception instead, so this probably deserves a line in the release notes.

Out of scope

Session/cookie persistence across restarts (the fix for the login frequency that causes the lockouts) is not part of this PR; the constructor-injected session parameter is in #162.

What I validated

• tests/test_login_rate_limit.py: 10 tests built on real requests.Response objects and a small session stand-in that runs the session's response hooks the way requests does (a plain MagicMock session would skip the hook). Covers: login and a data call (plant_list) raise on the 507 body; error_code and the exact message; numeric msg; a body without the back wrapper; and the fall-through cases (msg: "501", successful login, back as a list, non-JSON body, HTTP 500 still raising HTTPError).
• pytest tests/: 11 passed (the 10 above plus the existing test_exceptions.py). With the _raise_if_rate_limited(response) call removed, the 5 tests that assert a raise fail and the other 6 pass.
• ruff check ./growattServer with the pinned 0.15.16 and mypy --ignore-missing-imports growattServer/ (what ruff.yml and mypy.yml run): both clean, and both CI checks are green on the current head.
• Not validated: a real Growatt 507 response. I don't have a Growatt account and did not try to trigger a real lockout; the body shape comes from the Home Assistant tracebacks and @johanzander's analysis.

Checklist

• I've made sure the PR does small incremental changes. (new code additions are dificult to review when e.g. the entire repository got improved codestyle in the same PR.)
• I've added/updated the relevant docs for code changes i've made. (docstrings on the new exception and on _raise_if_rate_limited; no README/docs reference existing exception types, so none needed updating there)

---

🤖 Generated with Claude Code

[johanzander]

Thanks for the detailed writeup — the goal here (letting callers distinguish "rate-limited/locked out, back off for a long time" from other failures) is exactly right, but I believe the premise about how Growatt signals the 507 is incorrect, which unfortunately makes this implementation a no-op for the real failure mode.

Growatt returns 507 as an application-level error code in the JSON body, not as an HTTP status code. I've investigated this while maintaining the Home Assistant growatt_server integration (I'm the code owner there). The decisive evidence is the traceback in home-assistant/core#176831 / home-assistant/core#174789, the issues this PR cites:

homeassistant.exceptions.ConfigEntryError: Growatt login failed: 507

That exception can only be reached when the login HTTP request succeeded (HTTP 200), the JSON parsed fine, and the body contained success: false with msg: "507" — i.e. {"back": {"success": false, "msg": "507"}}. See _login_classic_api in HA core: it raises login_failed with the body's msg field.

If Growatt actually sent HTTP status 507, the flow would look completely different: this library's session hook already calls response.raise_for_status(), producing a requests.exceptions.HTTPError, which HA catches as RequestException and reports as communication_error — not login_failed: 507. Every field report I've seen shows the latter, never the former.

Consequences for this PR as written:

• The new status-code check and Retry-After parsing never fire in the real lockout scenario — callers still get a successful-looking dict with success: false and gain nothing.
• The tests mock transport-level HTTP 507/429 responses, a shape there's no evidence Growatt sends (as you note yourself under "Not validated").

I'd suggest reworking this at the response-body level instead: in login(), when success is false and msg == "507", raise a typed exception (e.g. GrowattRateLimitError(error_code="507"), following the GrowattV1ApiError pattern you referenced). That would give consumers like HA the actual signal they need. Happy to help validate against real-world responses.

On the session-persistence follow-up you offered: yes, please — that would genuinely help. In the HA integration today, login() is called from several places (config flow, setup, and the coordinator calls it on every update cycle), and each GrowattApi instance creates its own fresh requests.Session, so nothing is ever shared or persisted. Of the two shapes you sketched, I'd favor the constructor-injected session: requests.Session | None = None parameter — it lets a consumer share one session across API instances and restore persisted cookies at startup, without this library taking on any storage responsibility. That would attack the actual root cause of the lockouts (login frequency) rather than just reporting them.

[proscar87]

You're right, and the PR as written was a no-op. Reworked and force-pushed.

What convinced me is in login() itself:

response = self.session.post(self.get_url("newTwoLoginAPI.do"), data={...})
data = response.json()["back"]

It never reads response.status_code. So a transport-level 507 could not have produced Growatt login failed: 507 — that message can only come from a body the caller successfully parsed. Your reading of the traceback is the only one consistent with this code.

What changed

login() now raises GrowattRateLimitError(error_code="507") when the body carries success: false with msg: "507", following the GrowattV1ApiError shape so consumers read the code off the exception rather than parsing a string. Every other failure keeps returning the dict unchanged, so a wrong password behaves exactly as before.

I dropped the transport-level handling and the Retry-After parsing entirely rather than keeping them alongside the new check. There's no evidence Growatt ever sends that shape, and leaving it in would be untested code that looks like coverage — which is precisely the problem you identified. If a real 429 ever turns up, it can be added then with an actual sample behind it.

Five tests, mocking the body shape rather than the transport: three of them fail against unmodified login().

Worth flagging explicitly, since it's a behaviour change: login() now raises where it previously returned a dict for this one case. That's the point — HA can't act on a dict it has to introspect — but it is a breaking change for any consumer that inspects msg itself, so it may deserve a note in the release.

On the session parameter

Yes, I'll do it, and I agree the constructor-injected session: requests.Session | None = None is the better of the two shapes — it lets a consumer share one session across instances and restore persisted cookies at startup without this library taking on any storage responsibility.

I'll send it as a separate PR so this one stays reviewable on its own. And you're right that it's the more valuable of the two: this PR only reports the lockout, the session work attacks the login frequency that causes it.

Thanks for the correction — the "Not validated" caveat in my original description was doing far less work than it should have. I had no sample of the 507 shape and built the implementation around the assumption anyway.

[johanzander]

Thanks for the quick rework — this now catches the real signal, and I verified locally that the tests fail against unmodified login() and pass with the fix. Two follow-up points before I'd call it done:

1. Consider centralizing the check in the session response hook instead of login().

The lockout reports we have are all from login, but Growatt rate-limits each endpoint individually, so it's plausible (I'd say likely) the same success: false + msg: "507" shape can come back on data calls too. Rather than guarding one method — or sprinkling checks across the ~40 endpoint methods, each of which parses its own shape — the existing response hook in base_api.py (_raise_for_status) already sees every response for this session, and is exactly where your original HTTP-status check lived. Something like:

def _raise_for_status(response, *args, **kwargs):
    try:
        data = response.json()
    except ValueError:
        data = {}
    if isinstance(data, dict):
        back = data.get("back")
        if isinstance(back, dict):
            data = back
        if not data.get("success", True) and str(data.get("msg", "")) == RATE_LIMITED_CODE:
            raise GrowattRateLimitError(error_code=RATE_LIMITED_CODE)
    response.raise_for_status()

The guards aren't decorative: the hook must never raise on an unexpected shape, and shapes vary — e.g. plant_list receives back as a list, and non-JSON bodies are possible. False-positive risk stays negligible since it only fires on the exact success: false + msg: "507" combination. Cost is a second JSON parse per response, which is nothing at these payload sizes. With this in place the inline check in login() can go away, and if the shape on other endpoints turns out slightly different, there's one function to extend. (The V1 API keeps its own session and GrowattV1ApiError handling, so it stays out of scope.)

2. Nit: make the exception message generic.

I'd drop the "approximately 24 hour lockout … do not retry immediately" prose from the exception message — that's observed behavior that may change under our feet, and it ends up in every log line. Keep that context in the docstring, and let the message be something like Growatt rate limit reached: [507]. The test_507_error_mentions_not_retrying test would then assert on error_code rather than message wording, which also makes it less brittle.

Neither point changes the substance — the detection is right now. Happy to approve once these are in.

[proscar87]

Both in, thanks — the hook is the right home for this and the reasoning about per-endpoint limiting convinced me. Pushed as a separate commit so the delta is easy to read.

1. Detection moved to the hook. login() is back to untouched; the check is now _raise_if_rate_limited(response), called from _raise_for_status before response.raise_for_status() so a rate-limited body still raises the specific error rather than being masked by a status error.

I kept your guards and named why each one is there, since the next person to touch this will be tempted to simplify them away:

try:
    data = response.json()
except ValueError:
    return

if not isinstance(data, dict):
    return

back = data.get("back")
if isinstance(back, dict):
    data = back

if not data.get("success", True) and str(data.get("msg", "")) == RATE_LIMITED_CODE:
    raise GrowattRateLimitError(error_code=RATE_LIMITED_CODE)

success defaults to True on purpose, so a body that simply has no success key falls through instead of being read as a failure. Constant renamed LOGIN_RATE_LIMITED_CODE → RATE_LIMITED_CODE, as it is no longer login-specific.

2. Message is generic now: Growatt rate limit reached: [507]. The observed-lockout context moved into the class docstring, where it can be corrected without touching log output.

On the tests — the old ones would have passed for the wrong reason. They replaced api.session with a MagicMock, which never runs response hooks, so with the check moved they would have proven nothing. Rewrote them against real requests.Response objects and a small session stand-in that dispatches hooks the way requests does.

Eleven tests now, including the cases your guards exist for:

• a data call (plant_list) raises, which is the point of the move
• back as a list falls through and returns the list
• a non-JSON body falls through
• a body with no back wrapper is still checked
• an HTTP 500 still raises HTTPError — the new check must not shadow raise_for_status
• error_code == "507" and the exact generic message, instead of asserting on prose

Verification: with _raise_if_rate_limited(response) commented out and everything else left in place, 5 of the 11 fail — the five that assert a raise. The five guard tests pass either way, which is what they are for. ruff check ./growattServer with the pinned 0.15.16 and mypy --ignore-missing-imports growattServer/ are both clean.

🤖 Generated with Claude Code

[johanzander]

This looks good now — approving. I pulled the branch and verified rather than just reading the diff:

• All 11 tests pass, ruff check clean.
• Confirmed your "5 of 11 fail" claim: with the _raise_if_rate_limited(response) call commented out, exactly the five raise-asserting tests fail and the five guard tests pass either way, as intended.
• The test rework was the right call — you're correct that the old MagicMock session would have skipped the hook entirely and passed for the wrong reason. The _HookedSession stand-in dispatching real requests.Response objects through the actual hooks is exactly what this needed.
• login() untouched, guards match the shapes in this codebase (back-as-list, non-JSON, bare body), success defaulting to True so unknown bodies fall through, and HTTP errors still surface as HTTPError.

One correction for the record on something I wrote earlier: I said the V1 API "keeps its own session" — that's wrong. OpenApiV1 subclasses GrowattApi and inherits the session, so V1 responses also pass through this hook. That's harmless-to-good: V1 error bodies use error_code/error_msg rather than success + msg, so no false positive is possible, and if the 507 shape ever did appear on a V1 call, raising would be the correct behavior.

Two notes for the record, neither blocking:

• Since login() now raises where it previously returned a dict for this case, this deserves a line in the release notes, as you flagged. I'll update the Home Assistant integration to catch GrowattRateLimitError once this is released.
• Looking forward to the session-injection follow-up. One thing to keep in mind there: OpenApiV1.__init__ only takes token and doesn't forward constructor args, so if the session parameter should reach V1 too, it needs threading through — though for V1 (stateless token auth, no cookies) it's only a connection-pooling convenience, not part of the lockout fix. The classic API is where it matters.

Thanks for the thorough iteration on this one.

@indykoning this one is ready for a maintainer look when you have a moment — background in the comment thread above: Growatt signals its 507 rate-limit/lockout inside the JSON body (HTTP 200, success: false, msg: "507"), not as an HTTP status, and this PR surfaces that as a typed exception. It's directly relevant to the Home Assistant lockout reports (home-assistant/core#174789 / #176831), and I'd like to build on it from the HA side once released.

[johanzander]

@indykoning - gentle push on this one.