Send failures are silently ignorable; status_code is always 0 and retry classification is dead code

[lukwam]

Correction (see comment below): the original version of this issue claimed a non-2xx response produces no log output at all. That was wrong — the SendGrid SDK raises on non-2xx, so failures do reach _build_error_result and do log. The body below has been corrected. The core problem stands, and two related defects turned out to be more concrete than originally described.

Context

Found while reviewing altissimo-hq/customerio-python#1. customerio-python was derived from this package, so it inherited the same error-reporting design. This issue tracks the equivalent fix here.

1. Send failures are silently ignorable (the shared problem)

A failed send returns SendResult(ok=False) and is otherwise indistinguishable from success. Python has no must_use, so nothing prompts a caller to check result.ok; a caller that forgets gets silence. The library compounds it in _build_error_result (client.py:259-266):

def _build_error_result(self, exc: Exception) -> SendResult:
    logger.exception("SendGrid API error")
    return SendResult(ok=False, status_code=0, error=str(exc))

That log fires from a frame with no recipient, subject, or template context, so it is unactionable — while simultaneously making the failure look handled. The exception object is discarded; only str(exc) survives, so a caller cannot inspect or re-raise it.

2. status_code is always 0 for real API failures

_build_error_result hardcodes status_code=0. But the SendGrid SDK raises on non-2xx — python_http_client.client.Client._make_request ends in raise exc — and every python_http_client.exceptions.HTTPError carries self.status_code, set in its __init__, for all of 400, 401, 403, 404, 405, 413, 415, 429, 500, 503, 504.

So the actual HTTP status is sitting right there on the exception and gets thrown away. Every real API failure — bad key, unverified sender, malformed payload, rate limit — comes back as status_code=0, and callers cannot tell a permanent 400 from a transient 429.

customerio-python does not have this bug; its version reads exc.status_code when present. The copy-paste diverged in the wrong direction here.

3. Retry classification is dead code

Because the SDK raises on non-2xx, real failures only ever arrive via the except Exception branch of _send_with_retry (client.py:308-313), which retries unconditionally:

except Exception as exc:
    last_result = self._build_error_result(exc)
    # Exceptions (network errors, etc.) are also retryable
    if attempt < self._max_retries:
        self._backoff(attempt)
        continue

_RETRYABLE_STATUS_CODES (429, 500, 502, 503, 504) is consulted only at client.py:318, on the _build_result path — which in practice only ever sees 2xx responses, since anything else raised. Two consequences:

• The intended "retry only 429 and 5xx" policy never takes effect.
• With max_retries > 0, a permanent 400 or 403 burns the full backoff schedule before returning. Inert at the default max_retries=0, but that is the only thing currently saving it.

Fixing #2 is a prerequisite for fixing this: you cannot classify what you have discarded.

4. The 4xx tests exercise an unreachable path

tests/test_client.py:193-196 and 385-389 assert ok is False for FakeResponse(status_code=400) — a response returned with a 400. The real SDK raises instead, so _build_result's ok=200 <= status_code < 300 branch is effectively dead, and these tests pass while the production path they appear to cover behaves differently. Worth keeping the fixture for defence-in-depth, but the raising path needs its own coverage.

Proposed fix

Mirror what landed in customerio-python#1, so the two packages stay ergonomically identical, plus the two defects specific to this one:

1. SendGridSendError in exceptions.py, subclassing SendGridError, carrying status_code and the failed result, chaining the originating exception as __cause__.
2. SendResult.raise_for_status(context?) — mirrors requests.Response.raise_for_status; no-op on success.
3. raise_on_error: bool on the constructor and from_env(). Applied at the single point where a result is finalized, so it covers both the raising and the returned-response paths.
4. SendResult.exception field preserving the originating exception.
5. Read exc.status_code in _build_error_result instead of hardcoding 0 (fixes feat: Support multiple recipients, CC, and BCC #2), and replace logger.exception with logger.debug(..., exc_info=exc), logging a warning with context from the send methods in non-raising mode.
6. Apply _RETRYABLE_STATUS_CODES to the exception path now that the status survives, so permanent failures stop retrying (fixes feat: Configurable retry with exponential backoff #3). Keep retrying status_code == 0, which after feat: From name support, multiple recipients, CC/BCC, success logging #5 means a genuine network error with no HTTP response.

Default value of raise_on_error

In customerio-python this defaults to True, on the reasoning that the failure modes aren't symmetric: default-swallow fails invisibly, default-raise fails loudly and forces an acknowledgement.

That call is less obvious here, because this package has live consumers. Items 1, 2, 4, 5, and 6 are all non-breaking and fix the substantive defects on their own; only the default flip is breaking. Reasonable sequencing is to ship those first, then flip the default in a follow-up once consumers are audited and pinned forward.

Consumers

everygene does not currently use this package, though there is a plan to migrate it here. It has its own in-repo client at everygene/package/src/everygene/sendgrid/client.py, and it is a well-behaved consumer worth copying from:

• send_mail catches HTTPError, logs at error level with the message, and returns {"status": "error", "message": ...}.
• Every call site validates explicitly via _ensure_sendgrid_ok (everygene/users/router.py:34-38), converting a failure into an HTTP 502.

So the migration needs a deliberate contract mapping — {"status": "ok"} → result.ok, and _ensure_sendgrid_ok → either an ok check or a try/except SendGridSendError. Both work; the point is that it should be chosen rather than inherited. Note that everygene sets "status": "ok" on "no exception raised", which is sound precisely because the SDK raises on non-2xx.

darwinsark also consumes this package. I could not verify its pin or whether any call site depends on ok=False; someone with visibility should confirm before the default is flipped.

Note

The README quick start does show assert result.ok (README.md:59), so the docs gesture at checking — but an assert in a quick start is not a substitute for an API that cannot be ignored.

[lukwam]

Correction to the original issue body.

I claimed a non-2xx SendGrid response produces zero log output. That was wrong, and it was the headline of the issue, so flagging it explicitly rather than quietly editing.

The reasoning that led me there: _build_result classifies ok=200 <= status_code < 300, and the send methods only log on result.ok, so a returned 400 would indeed be completely silent. What I failed to check was whether a 400 ever is returned rather than raised. It isn't — python_http_client.client.Client._make_request ends in raise exc, so the SDK raises on every non-2xx. Real failures therefore reach _build_error_result and do emit a log.

What's actually there, now corrected in the body:

• The context-free logger.exception and the ignorable ok=False are real and unchanged — that remains the core issue.
• _build_error_result hardcodes status_code=0, discarding exc.status_code, which python_http_client's HTTPError carries for all of 400, 401, 403, 404, 405, 413, 415, 429, 500, 503, 504. Every real API failure loses its HTTP status. customerio-python reads it correctly, so this one is specific to this package.
• Consequently _RETRYABLE_STATUS_CODES is dead code: it is only consulted on the _build_result path, which only ever sees 2xx. The intended "retry 429 and 5xx only" policy never fires, and with max_retries > 0 a permanent 400 burns the full backoff schedule. Inert at the max_retries=0 default, which is the only thing masking it.
• The 4xx tests at tests/test_client.py:193-196 and 385-389 use a returned FakeResponse(status_code=400), so they cover a path production never takes.

Net effect on the fix: the same API changes still apply, plus reading exc.status_code and gating the exception-path retry on it. The status-code fix is a prerequisite for the retry fix — you can't classify what you've discarded.