feat(python-sdk): retry transient failures with full-jitter backoff (0.5.0)

[surpradhan]

What does this PR do?

Adds automatic retry with full-jitter exponential backoff to both Python SDK clients (AEPClient / AsyncAEPClient):

• Retried: connection/timeout/transport errors, HTTP 429 (honours Retry-After as the delay floor, capped at retry_backoff_max), HTTP 5xx.
• Never retried: other 4xx (validation/auth/not-found).
• Knobs: max_retries=3 (0 disables), retry_backoff_base=0.5, retry_backoff_max=30.0.
• Behaviour change: all transport errors — including timeouts, which previously leaked as raw httpx.TimeoutException — now raise AEPConnectionError after exhaustion.
• Version 0.4.1 → 0.5.0 (release itself remains a separate tag + human-gated publish).

Why?

The clients issued a single POST and surfaced any transient blip to the caller — the top SDK-side gap in the 2026-07-19 CTO review (Wave 0, item 5), and table stakes for an ingestion SDK. Safe on POST /events because events carry a client-generated id and the server deduplicates on it (replay → "duplicate": true).

How to test?

cd sdks/python && python -m pytest tests/unit -q   # 299 passed (18 new in tests/unit/test_retry.py)
python -m ruff check aep tests                     # clean (scoped files)

New tests cover: 5xx-then-success sequences, exhaustion re-raising the mapped error, no-retry on 400, Retry-After floor honoured on 429, connect-error and timeout wrapping, max_retries=0, GET retries, async parity — with time.sleep/asyncio.sleep patched to record delays and rand injected for deterministic backoff-math tests.

Related Issues

CTO review Wave 0, item 5.

Checklist

• Tests pass (297 unit incl. 18 new)
• Documentation updated (sdks/python/README.md)
• Behaviour change documented (timeouts → AEPConnectionError)

🤖 Generated with Claude Code

[surpradhan]

Independent validation report (validator context — ran the change, not just read it)

Gates (run in worktree at PR head 7c8bf7d)

• python3 -m pytest tests/unit -q → 297 passed in 201.5s (matches claim).
• python3 -m ruff check aep/_http.py aep/client.py aep/async_client.py tests/unit/test_retry.py → All checks passed (exit 0).
• gh pr checks 160 → all 14 required checks pass, incl. Python SDK tests 3.10/3.11/3.12/3.13.
• tests/unit/test_retry.py alone: 16 passed (PR body says 15 — trivial count nit; 297 total is correct). Ran 5× consecutively: 16/16 every time, no flakiness (sleeps are patched, respx side-effects deterministic).

Discriminator probe — do the new tests fail against pre-change code?

Scratch copy with origin/main's client.py/async_client.py but the PR's _http.py helpers + new tests: 11 of 16 failed (only the 5 pure backoff-math tests pass, as expected). Then probed old behavior directly (bypassing the sleep-patch fixture):

• Old client on 503-then-202 sequence: raised AEPServerError after exactly 1 request — no retry. New test's call_count == 3 assertion genuinely discriminates.
• Old client on httpx.ReadTimeout: leaked the raw httpx.TimeoutException (old code only caught ConnectError). Confirms test_timeout_now_wrapped_as_connection_error locks a real behavior change.

Real-socket end-to-end (no respx, no mocks)

Scratch ThreadingHTTPServer returning 503, 503, then 202 with JSON body; AEPClient(retry_backoff_base=0.01):

FLAKY: accepted=True posts=3 wall=0.075s
400: posts=1

• Flaky server: emit() succeeded after exactly 3 POSTs, sub-second wall time.
• Always-400 server: exactly 1 POST, AEPValidationError raised — 4xx never retried.

Claim checks

• Version: pyproject.toml line 12 and aep/__init__.py line 37 both 0.5.0. ✓
• Scope: 7 files, all under sdks/python/. ✓ Worktree HEAD == PR head (7c8bf7d9), tree clean — diff matches what was validated. ✓
• Idempotency-safety claim: verified only POST /events goes through _post (all other client calls are GETs), so the sole non-idempotent retried operation is the one the server dedups on client-generated id. ✓
• RetryConfig clamps negative max_retries to 0; compute_retry_delay caps Retry-After at backoff_max (verified by tests + reading _http.py:88-103). ✓

Findings

• Nit / PR body: says "15 new" tests; the file contains 16. No code change needed.
• Nit / aep/client.py + async_client.py: Retry-After is only parsed for 429; RFC 7231 also allows it on 503. Current behavior (jittered backoff for 5xx) is documented and reasonable — optional follow-up.
• Nit / aep/client.py:200 (except httpx.TransportError): also catches deterministic local errors (UnsupportedProtocol, LocalProtocolError), burning full retries on non-transient failures before wrapping. Cosmetic; matches common SDK practice.

No blocking items.

Verdict: Approve

Retry loop proven against a real socket, new tests demonstrably fail against pre-change code, gates green locally and on CI (14/14).

[surpradhan]

Independent code review — PR #160 (retry with full-jitter backoff, 0.5.0)

Reviewed the full diff against main, traced both retry loops by hand, ran the gates locally, and empirically probed the httpx refactor semantics.

Verified correct (traced, not trusted)

• Attempt accounting: range(max_retries + 1) gives exactly max_retries + 1 requests (3 → 4); locked by test_retry_exhaustion_raises_server_error asserting call_count == 4 and 3 sleeps.
• Final attempt goes through handle_response: on the last attempt, attempt < max_retries is false, so a terminal 429/5xx falls through to handle_response(resp) and raises AEPRateLimitError (with retry_after preserved) / AEPServerError — never swallowed. Locked by test_429_exhaustion_still_raises_rate_limit and test_retry_exhaustion_raises_server_error.
• Exception chaining: raise AEPConnectionError(...) from exc present in both clients.
• Unreachable tail: max(0, max_retries) clamps negatives, so the loop always runs ≥ 1 iteration and every iteration returns, raises, or continues; the AssertionError tail is genuinely unreachable.
• json=None refactor is semantics-preserving: probed empirically (httpx 0.28.1, MockTransport): client.request('GET', url, json=None) produces a byte-identical request to client.get(url) — same empty body, identical headers. (The content-type: application/json header on GETs is pre-existing — old _get also sent self._headers().) No regression.
• Non-retryable errors fail fast: 400/401/404 only ever reach handle_response on the return path, so AEPValidationError etc. propagate immediately with zero sleeps (tested).
• Tests discriminate: the sequence-mock tests (503, 503, 202 → success, call_count 3) and test_timeout_now_wrapped_as_connection_error would all fail against the pre-change code (old code raised on the first 503 and leaked raw httpx.ReadTimeout). Sleep patching is complete in every retrying test — no real-sleep flakiness path found.

Findings

1. [Suggestion] Missing root CHANGELOG.md entry for 0.5.0 — CHANGELOG.md
Every prior Python SDK release (0.3.0, 0.4.0, 0.4.1) has a root CHANGELOG section, and the 0.4.1 bump commit (e1b2d9d) added its entry in the same PR as the version bump. 0.5.0 carries an actual behavior change (timeouts → AEPConnectionError) that belongs there. Fix: add a ## Python SDK \agent-event-protocol` 0.5.0 — 2026-07-19` section covering the retry feature + the behavior change.

2. [Suggestion] Version bump incomplete — three stale 0.4.1 strings
The 0.4.1 bump kept all of these in sync; this PR left them behind:

• sdks/python/README.md:5 — **Version:** 0.4.1 (this is the PyPI long-description header; publishing 0.5.0 with it reading 0.4.1 is user-visible wrong info).
• sdks/python/aep/__init__.py:1 — module docstring — v0.4.1. (now contradicts __version__ = "0.5.0" at line 37 of the same file).
• sdks/python/README.md:592-593 — release-tag example still python-sdk-v0.4.1; should read python-sdk-v0.5.0 for the next cut.

3. [Suggestion] Hidden impact on embedded clients (instrument() and the OTEL exporter) is unaddressed and undocumented

• aep/instrument.py:2682 — instrument() builds AEPClient(server_url=..., api_key=...) with the new default max_retries=3. The single _Emitter worker (instrument.py:211-222) drains its bounded 10k queue serially, so under a timing-out server each event now occupies the worker for up to ~4×timeout + ~3.5 s backoff (~43 s vs ~10 s before), and under sustained 429 with Retry-After: 30 up to ~90 s of sleeps per event. Net effect: the queue fills ~4× faster, more events drop (logged, host never blocks — submit is put_nowait, so this is degradation, not deadlock), and flush(5.0) / the 2 s atexit flush drain correspondingly less.
• aep/otel/exporter.py:41 — same story for AEPSpanExporter; a blocking export() pushes drop pressure into the OTEL BatchSpanProcessor queue.
  Fix (pick one, both acceptable): construct these internal clients with an explicit lower policy (e.g. max_retries=1 — telemetry is lossy-by-design and the emitter already has its own drop/log path), or keep the default and document the interaction in the instrument() docstring + README Retries section. Either way the choice should be explicit, not inherited silently.

4. [Suggestion] Retry-After honoured only for 429, not 503 — sdks/python/aep/client.py:214-218, sdks/python/aep/async_client.py:217-221
RFC 7231 defines Retry-After for 503 as well (it is the header's original use), and servers behind LBs commonly send it. The guard if resp.status_code == 429 else 0 discards it on 5xx. Fix: parse the header whenever the status is retryable — parse_retry_after already clamps garbage to 0 and compute_retry_delay caps at backoff_max, so dropping the 429-only condition is safe.

5. [Nit] AEPConnectionError docstring now understates its meaning — sdks/python/aep/exceptions.py
"""Cannot connect to the AEP ingest server.""" — as of this PR it is also raised for timeouts and mid-flight transport errors after retry exhaustion. Suggest: """Transport-level failure reaching the AEP server (connect errors, timeouts, dropped connections) after retries are exhausted."""

6. [Nit] PR body says "15 new tests"; tests/unit/test_retry.py contains 16 (4 pure-helper + 9 sync + 3 async; pytest tests/unit/test_retry.py → 16 passed). One-word PR-description fix.

7. [Nit] Commit subject type feature(python-sdk): is not a project commit type (allowed: feat, fix, docs, test, refactor, perf, chore). The PR title already uses feat(python-sdk): — make sure the squash-merge commit uses the PR title, not the branch commit subject.

Retry-After / jitter edge cases checked

• Floor > cap: Retry-After: 120 with backoff_max=30 → sleeps 30 (clamped; documented in README as "also caps Retry-After"). Acceptable and tested.
• HTTP-date Retry-After → 0 floor → falls back to jittered backoff (pre-existing parse_retry_after behavior, documented in its docstring).
• Default rand=random.random can yield near-0 delays — that is standard full-jitter (AWS style); the Retry-After floor protects the 429 path. Fine.
• Missing Retry-After on 429 → "0" default → pure backoff. Fine.

API design / versioning

Constructor knobs (max_retries, retry_backoff_base, retry_backoff_max) match the SDK's flat-kwargs style (timeout); frozen RetryConfig kept internal (not exported) is right. No env-var knob (AEP_MAX_RETRIES) exists — optional follow-up, only worth doing if finding 3 resolves toward "keep defaults" (env override would then be the tuning path for instrument() users who never construct a client). Bumping the minor (0.4.1 → 0.5.0) for the timeout-exception behavior change is the correct 0.x semver signal, and the change is documented in README + PR body — just missing from CHANGELOG (finding 1).

Gates (run locally, read-only)

• python3 -m pytest tests/unit -q → 297 passed (includes the 16 new retry tests; async tests run via asyncio_mode = "auto").
• python3 -m ruff check aep tests → 4 F401s, all in files this PR does not touch (tests/integration/test_client.py, tests/unit/test_audit.py, tests/unit/test_otel_exporter.py ×2) — pre-existing with local ruff 0.8.4, out of scope; the PR's own files are clean.
• CI: all 14 required checks green (incl. Python SDK tests 3.10–3.13 with the pinned ruff).

What looks good

Clean separation of the pure retry math into _http.py with injectable rand; the two client loops are exact mirrors; the idempotency rationale for retrying POST /events (client-generated id + server dedup) is stated in both docstrings and the README; broadening ConnectError → TransportError closes a real pre-existing leak of raw httpx exceptions; test quality is high — sequence mocks, exhaustion counts, sleep capture, and deterministic backoff-math tests would all fail against the old code.

Verdict: Request Changes (minor)

No Critical findings — the retry logic itself is correct and well-tested. Requesting changes for findings 1–2 (release-convention breaches that would ship wrong version info to PyPI) and an explicit decision on finding 3; 4–7 are cheap to fold in.

Reviewed by an independent session (not the author).

[surpradhan]

Both round-1 reviews applied in cdf144d:

• CHANGELOG 0.5.0 entry added (code review S1) with the behaviour change called out.
• Version stragglers fixed — SDK README header, __init__ docstring, release-tag example all 0.5.0 (S2).
• Embedded clients (code review S3): instrument()'s background emitter and the OTEL exporter now pass an explicit max_retries=1 with comments stating why — the retry budget is a deliberate choice, not inherited.
• Retry-After on any retryable status per RFC 7231 (code review S4 + validator nit 2).
• Exception narrowing (validator nit 3): new is_retryable_exception — timeouts, network errors, and RemoteProtocolError retry; deterministic local errors (UnsupportedProtocol, LocalProtocolError, proxy misconfig) raise AEPConnectionError immediately. Regression-tested (1 request, 0 sleeps).
• AEPConnectionError docstring updated (code review N5).
• Test count: PR body corrected — now 18 tests in test_retry.py (N6 + validator nit 1).
• Commit type (N7): squash will use the PR title (feat(...)).

Gates: ruff clean; targeted suites (retry + client + async + exporter + instrument) 87/87 green.

[surpradhan]

Validator round 2 — empirical validation of delta cdf144d — APPROVE

Round-1 verdict was Approve with 3 nits; all three (plus the code-reviewer's items) are confirmed fixed in cdf144d. Re-validated from a clean worktree at the PR head.

Gates

• python3 -m pytest tests/unit -q → 299 passed (was 297), tests/unit/test_retry.py → 18 passed (was 16). Flakiness check: retry suite run 5x consecutively, 18/18 every time.
• ruff check on all changed files: All checks passed (matches the CI gate, which runs ruff check sdks/python/aep).
• gh pr checks 160: all 14 required checks green, and the run's headSha confirmed = cdf144d.
• gh pr diff file list matches the validated worktree exactly; worktree clean at PR head.

Empirical probes (real socket, not respx)

(a) Retry-After honoured on 503 — stdlib HTTP server returning 503 + Retry-After: 1 then 202; client with retry_backoff_base=0.001 so the header floor dominates:

• New code: 2 attempts, inter-request gap 1.005s, emit accepted.
• Discriminator: same probe against the pre-delta code (parent 7c8bf7d) → gap 0.002s (header ignored on 503, probe fails). The new behaviour is real and the new test would have caught the old code.

(b) Deterministic local error fails fast — AEPClient(server_url="ftp://nope") with max_retries=3:

• New code: AEPConnectionError in 0.3ms, zero retries, zero sleeps.
• Discriminator: pre-delta code burned ~1.1s of jittered backoff on the same deterministic UnsupportedProtocol before raising.

Also verified httpx.UnsupportedProtocol is a TransportError subclass outside the retryable set (TimeoutException/NetworkError/RemoteProtocolError), so it still lands in AEPConnectionError — no behaviour regression for callers catching the SDK exception.

Version stragglers

grep -rn "0\.4\.1" sdks/python --include="*.py" --include="*.toml" --include="*.md" → zero hits. Only remaining mention repo-wide is the historical release entry in root CHANGELOG.md:696 (correct to keep). pyproject.toml version and aep/__init__.py __version__ both 0.5.0, in sync per the release process.

Verdict

Approve. Zero actionable items. All round-1 nits and code-reviewer findings verified fixed empirically, gates green on the exact PR head.

[surpradhan]

Round-2 review (delta 7c8bf7d..cdf144d) — independent reviewer

Round-1 resolution verification

• S1 CHANGELOG — RESOLVED. 0.5.0 entry at top of CHANGELOG.md; content is accurate (behaviour change called out, max_retries=1 embedded emitters, 18 new tests).
• S2 version stragglers — RESOLVED. All three fixed: README version line (0.5.0), README tag example (python-sdk-v0.5.0), aep/__init__.py docstring. grep -rn "0\.4\.1" over py/toml/md in sdks/python: zero hits; pyproject.toml already 0.5.0.
• S3 embedded-client retry inheritance — RESOLVED. instrument.py (~L2682) and otel/exporter.py (~L41) pin max_retries=1 with rationale comments. The exporter unit tests construct the real AEPSpanExporter, so the changed constructor path is exercised — no test breakage.
• S4 Retry-After on any retryable status — RESOLVED. Identical one-line change in both clients; new test_503_honours_retry_after asserts the 5.0s floor. Also verified compute_retry_delay caps the result at backoff_max (30s), so a hostile/huge Retry-After cannot stall the client. See Nit 2 for a leftover docstring.
• Validator nit → is_retryable_exception — VERIFIED against httpx 0.28.1 MRO.
  • RemoteProtocolError → ProtocolError → TransportError — it is NOT under NetworkError, so listing it separately is necessary and correct.
  • ConnectError/ReadError/WriteError/CloseError all subclass NetworkError — covered. All four timeout variants (incl. PoolTimeout) subclass TimeoutException — covered.
  • Excluded UnsupportedProtocol + LocalProtocolError: deterministic, correct to fail fast. test_deterministic_local_error_fails_fast asserts call_count == 1 and no sleeps — good mutation-resistant assertions.
  • Excluded ProxyError: a judgment call I examined and accept as-is (no change requested). httpx's ProxyError means the proxy answered CONNECT with an error — usually deterministic (407 auth, 403 ACL), occasionally transient (proxy 502/503 under load). requests/urllib3 retry proxy errors, but httpx's class is narrower, the choice is documented in the docstring, and a non-retried ProxyError still wraps into AEPConnectionError (consistent caller surface). Fine as-is.
• N5 exceptions docstring — RESOLVED. AEPConnectionError now documents timeouts + retries-exhausted semantics.
• N6 test count — PARTIAL. See Nit 1.
• N7 commit type — RESOLVED. PR title is feat(python-sdk): ...; squash will use it.

Findings

1. Nit (PR body): the delta added 2 more tests, so the counts drifted again: the suite is now 299 passed (body says 297 in both the how-to-test line and the checklist), and the checklist bullet still reads "incl. 15 new" — it is 18. One PR-description edit, no commit needed.
2. Nit — sdks/python/aep/client.py:31-32 and sdks/python/aep/async_client.py:31-32: the class docstrings still attach "(honouring Retry-After)" to 429 only. README/CHANGELOG were updated to "on any retryable status" but these two docstrings were missed. Suggested: "HTTP 429 and HTTP 5xx (honouring Retry-After)".

Gates (read-only, run locally)

• python3 -m pytest tests/unit -q → 299 passed (18 in test_retry.py)
• python3 -m ruff check on all changed files → clean
• All 14 required CI checks green; worktree clean

What looks good

The exception narrowing is exactly right per the real httpx hierarchy — including the non-obvious catch that RemoteProtocolError lives under ProtocolError, not NetworkError. The fail-fast test asserts both call count and zero sleeps. The max_retries=1 pins carry clear rationale comments about queue hold time. Retry-After remains floored-and-capped, so it cannot be weaponised to stall clients.

Verdict: Approve

Both remaining items are one-line/metadata nits (one is the PR description itself) with no behavioural surface; they do not require another review round from me. Fix them before merge.

[surpradhan]

Round-2 nits closed in 6cb8a72: both client class docstrings now say Retry-After is honoured on any retryable status; PR body counts corrected to 299 total / 18 new retry tests. Both round-2 reviews are Approve with no further round requested — handing off for merge once CI re-greens on this docstring-only commit.