Distinguish HTTP 429/507 rate-limit responses with a typed exception

[proscar87]

Summary

Growatt returns HTTP 507 (and sometimes 429) when a client exceeds the request rate. This is not a normal rate limit: reports (see #55, and home-assistant/core#176831) indicate a 507 precedes an approximately 24 hour account lockout, not a short cooldown. This matters a lot for consumers like Home Assistant's growatt_server integration, which currently calls login() again on every startup — a restart while already close to the limit can tip the account into a day-long block. There's already a mitigation PR open on the HA side (home-assistant/core#177068, "retry in 4 hours"), but that only works around the symptom from outside; the actual signal (that this specific response means "stop, and stop for a long time") is something only this library can expose reliably.

Root cause

growattServer/base_api.py:

• GrowattApi.__init__ (~line 72) creates a fresh requests.Session() on every instantiation, and cookies/tokens are never persisted across process restarts — every app startup is effectively a brand-new login.
• _raise_for_status (~lines 74-79) unconditionally called response.raise_for_status(). A 507 came out as a plain requests.exceptions.HTTPError, indistinguishable from any other 5xx, and any Retry-After header was silently discarded.

By contrast, the V1 API (open_api_v1/__init__.py + exceptions.py) already has a typed error (GrowattV1ApiError) with structured error_code/error_msg. The classic API never got the same treatment.

What this PR does (scoped, safe part)

• Adds GrowattRateLimitError(status_code, retry_after) in exceptions.py, following the same pattern as GrowattV1ApiError.
• base_api.py's response hook now raises GrowattRateLimitError for HTTP 429 and 507 before falling back to raise_for_status() for everything else — so existing behavior for all other status codes (including plain 5xx) is unchanged.
• Parses Retry-After when the server sends it (both the delay-seconds form and the HTTP-date form per RFC 9110) and exposes it as exc.retry_after (float seconds, or None if absent/unparseable — Growatt doesn't reliably send this header, especially on 507s).
• The library does not retry automatically. Auto-retrying a 507 is exactly what would deepen the lockout, so the decision of what to do (back off for how long, give up, alert the user) is left entirely to the caller — this PR only makes it possible for the caller to know what happened, and it must actively do nothing further.
• Exported GrowattRateLimitError from the package __init__.py.

This is additive: a new exception class, a new (private) helper function, and a status-code check inserted before the existing raise_for_status() call. No existing signatures changed, no existing exception types changed for non-rate-limit cases.

What I deliberately left out of scope

Session/cookie persistence across restarts (the actual fix for "HA re-logs in every startup near the limit") was evaluated but not implemented as code, to keep this diff small and reviewable:

• self.session (a plain requests.Session) is already a public, mutable attribute — a consumer can already save/restore session.cookies (a RequestsCookieJar) across restarts today, without any library change, to skip calling login() when they already hold valid cookies. I added a short doc comment above self.session = requests.Session() pointing this out, since it wasn't discoverable before.
• A more deliberate API for this (e.g. an optional session: requests.Session | None constructor param for dependency injection, or save_session()/load_session() helpers) would be a public-API change and a design decision I don't think belongs bundled into an error-handling fix. Happy to open a follow-up PR for this specifically if a maintainer confirms which shape they'd want — a constructor-injected session vs. explicit save/load helpers have different tradeoffs (the former is more flexible; the latter is more discoverable and harder to misuse).
• I did not touch the Home Assistant integration itself (out of scope for this repo, and home-assistant/core has its own AI-authored-PR policy).

What I validated

• New tests in tests/test_rate_limit.py mock the HTTP transport with requests_mock (not any of this library's own functions), so they exercise the real path: GrowattApi.login() → requests.Session.post() → the session's response hook → the raised exception. Covers: 507 and 429 both raise GrowattRateLimitError with the right status_code; Retry-After parsed correctly in both the seconds and HTTP-date forms; missing/unparseable header → retry_after is None; exactly one request is made (no silent retry); non-rate-limit errors (tested with 500) still raise plain requests.exceptions.HTTPError as before; a normal successful login is unaffected.
• Confirmed with git stash that every new test fails against the pre-fix code (import error / wrong exception type raised) and passes after the fix — this repo doesn't run pytest in CI, so I ran the suite locally (pytest tests/, 10 passed).
• Ran ruff check growattServer and mypy --ignore-missing-imports growattServer/ locally (matching what .github/workflows/ruff.yml and mypy.yml run in CI) — both clean.
• Not validated: real-world behavior against an actual Growatt 507 response, since I don't have access to a Growatt account and, more importantly, deliberately did not try to reproduce a real lockout (would mean intentionally blocking a real account for 24h to test — not something to do just for CI). If a maintainer or someone who has hit this can confirm the actual response shape (status code, and especially whether Growatt ever sends Retry-After on a real 507) that would help decide whether retry_after needs an additional fallback later. If not, an isolated repro is welcome but not something worth asking a user to trigger deliberately.

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 + a comment on self.session; 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.