fix(claude-admin): add rate-limit governor to stop the 429 storm - #1927
Merged
Conversation
📝 WalkthroughWalkthroughThe Claude Admin connector now uses shared HTTP retry and failure handling, serializes stream execution, and adds reliability tests for rate limits, transient server errors, and authentication failures. Its descriptor and CI coverage metadata are updated. ChangesClaude Admin connector reliability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ClaudeAdminStream
participant CompositeErrorHandler
participant AnthropicAdminAPI
ClaudeAdminStream->>AnthropicAdminAPI: request users page
AnthropicAdminAPI-->>CompositeErrorHandler: HTTP 429 or 503
CompositeErrorHandler->>AnthropicAdminAPI: retry with Retry-After or backoff
AnthropicAdminAPI-->>ClaudeAdminStream: successful page or surfaced 401 failure
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
cyberdima
changed the title
mitasovr
approved these changes
Jul 27, 2026
Contributor
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py (1)
92-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the configured 404 failure path.
The suite only exercises 401, while
connector.yamlexplicitly maps 404 toFAIL. Add a sibling 404 mock/assertion so that status mapping remains protected.🤖 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 `@src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py` around lines 92 - 107, Add a sibling test beside test_error_fail_401_surfaces_error that mocks the same Claude Admin request with a 404 response and asserts read_stream(..., expecting_exception=True) produces stream errors and zero records, preserving the connector.yaml FAIL mapping for 404.
🤖 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
`@src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py`:
- Around line 92-107: Add a sibling test beside
test_error_fail_401_surfaces_error that mocks the same Claude Admin request with
a 404 response and asserts read_stream(..., expecting_exception=True) produces
stream errors and zero records, preserving the connector.yaml FAIL mapping for
404.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e34a2bba-8df8-462b-b83c-eb8bbbb8f209
📥 Commits
Reviewing files that changed from the base of the PR and between 9ae00ab and 647b14a702eddfb719f8da49147cc440cef2fb1c.
📒 Files selected for processing (7)
scripts/ci/components.pysrc/ingestion/connectors/ai/claude-admin/README.mdsrc/ingestion/connectors/ai/claude-admin/connector.yamlsrc/ingestion/connectors/ai/claude-admin/tests/config.pysrc/ingestion/connectors/ai/claude-admin/tests/conftest.pysrc/ingestion/connectors/ai/claude-admin/tests/fixtures/user.jsonsrc/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py
cyberantonz
approved these changes
Jul 27, 2026
cyberdima
enabled auto-merge
July 27, 2026 08:45
cyberdima
disabled auto-merge
July 27, 2026 09:20
cyberdima
enabled auto-merge
July 27, 2026 09:21
cyberdima
force-pushed
the
fix/1902-claude-admin-rate-limit
branch
from
July 27, 2026 12:24
fb5ce58 to
5431c49
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py (1)
92-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the documented 404 failure path.
The PR contract says both 401 and 404 should take the
FAILpath, but this suite only exercises 401. Add a matching 404 case—or parameterize this test over both statuses—and assert that no records are emitted.🤖 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 `@src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py` around lines 92 - 107, Extend test_error_fail_401_surfaces_error to also exercise the documented 404 failure path, either by adding a matching test or parameterizing it over status 401 and 404. For both responses, call read_stream with expecting_exception=True and assert an error is surfaced and output.records remains empty.
🤖 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
`@src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py`:
- Around line 92-107: Extend test_error_fail_401_surfaces_error to also exercise
the documented 404 failure path, either by adding a matching test or
parameterizing it over status 401 and 404. For both responses, call read_stream
with expecting_exception=True and assert an error is surfaced and output.records
remains empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 08ba4f72-11cf-4ce6-b077-3c36bfa2d6a7
📥 Commits
Reviewing files that changed from the base of the PR and between 647b14a702eddfb719f8da49147cc440cef2fb1c and 5431c49ae56a670695fbf5d247deffc70c29fd67.
📒 Files selected for processing (8)
scripts/ci/components.pysrc/ingestion/connectors/ai/claude-admin/README.mdsrc/ingestion/connectors/ai/claude-admin/connector.yamlsrc/ingestion/connectors/ai/claude-admin/descriptor.yamlsrc/ingestion/connectors/ai/claude-admin/tests/config.pysrc/ingestion/connectors/ai/claude-admin/tests/conftest.pysrc/ingestion/connectors/ai/claude-admin/tests/fixtures/user.jsonsrc/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/ingestion/connectors/ai/claude-admin/tests/fixtures/user.json
- src/ingestion/connectors/ai/claude-admin/tests/config.py
- src/ingestion/connectors/ai/claude-admin/tests/conftest.py
- src/ingestion/connectors/ai/claude-admin/README.md
- src/ingestion/connectors/ai/claude-admin/connector.yaml
The connector fired all streams in parallel with per-day pagination and no rate-limit handling, saturating the org-wide Anthropic Admin API limit: a single run produced 492x HTTP 429 and starved messages_usage/cost_report to 0 rows. Add a shared CompositeErrorHandler (RATE_LIMITED on 429 honoring Retry-After, RETRY on 5xx with exponential backoff, FAIL on 401/404) referenced by every stream requester, and set concurrency_level: 1 so streams read serially against the org limit. Mirrors the sibling claude-enterprise connector, which hits the same API with the same credential. (No api_budget/HTTPAPIBudget: no nocode connector in the repo uses it; concurrency=1 + Retry-After is the established, sufficient pattern.) Also correct the manifest version header 7.0.4 -> 6.60.9 so the mock-test harness (pinned to the 6.60.x CDK line, like every other nocode manifest) can load it; the manifest uses no 7.x-only feature and the deployed SDM 7.23.6 accepts it. Keeps the change isolated (no shared-harness CDK bump). Adds an L1 reliability suite (429/503 retried and recovered without record loss; 401 surfaces as an error rather than a silent 0-row success), registers claude-admin in the connector mock-test CI component, and documents the governor in the connector README. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Dmitry Saukh <38005371+cyberdima@users.noreply.github.com>
connector.yaml changed (concurrency_level + error handler) but descriptor.yaml version was untouched. Reconcile only republishes the manifest on descriptor-version drift (ADR-0015) — without this bump the fix silently never reaches Airbyte. Signed-off-by: Dmitry Saukh <38005371+cyberdima@users.noreply.github.com>
cyberdima
force-pushed
the
fix/1902-claude-admin-rate-limit
branch
from
July 27, 2026 13:00
5431c49 to
98650c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py (1)
9-15: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftCover the complete error-handler contract.
This file only exercises
claude_admin_users, and the matrix omits the documented 404 failure path. Add a 404 test and verify handler attachment for each stream requester, or point to existing tests that provide that coverage.🤖 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 `@src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py` around lines 9 - 15, Expand the reliability tests beyond claude_admin_users to cover the documented 404 failure path and verify the error handler is attached to every stream requester. Add assertions that a 404 surfaces as an error without partial records, and ensure each relevant stream’s requester is exercised or reuse existing coverage if it already provides these checks.
🤖 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.
Inline comments:
In
`@src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py`:
- Around line 92-107: Update test_error_fail_401_surfaces_error to assert that
output.errors contains the expected stable 401 authentication/status detail,
rather than only checking that errors is non-empty. Retain the assertion that
output.records has length zero.
---
Nitpick comments:
In
`@src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py`:
- Around line 9-15: Expand the reliability tests beyond claude_admin_users to
cover the documented 404 failure path and verify the error handler is attached
to every stream requester. Add assertions that a 404 surfaces as an error
without partial records, and ensure each relevant stream’s requester is
exercised or reuse existing coverage if it already provides these checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abcea37d-4968-447e-814a-843346c78e3a
📥 Commits
Reviewing files that changed from the base of the PR and between 5431c49ae56a670695fbf5d247deffc70c29fd67 and a5db9b8.
📒 Files selected for processing (8)
scripts/ci/components.pysrc/ingestion/connectors/ai/claude-admin/README.mdsrc/ingestion/connectors/ai/claude-admin/connector.yamlsrc/ingestion/connectors/ai/claude-admin/descriptor.yamlsrc/ingestion/connectors/ai/claude-admin/tests/config.pysrc/ingestion/connectors/ai/claude-admin/tests/conftest.pysrc/ingestion/connectors/ai/claude-admin/tests/fixtures/user.jsonsrc/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/ingestion/connectors/ai/claude-admin/descriptor.yaml
- src/ingestion/connectors/ai/claude-admin/tests/fixtures/user.json
- scripts/ci/components.py
- src/ingestion/connectors/ai/claude-admin/tests/conftest.py
- src/ingestion/connectors/ai/claude-admin/tests/config.py
- src/ingestion/connectors/ai/claude-admin/README.md
- src/ingestion/connectors/ai/claude-admin/connector.yaml
Comment on lines
+92
to
+107
| def test_error_fail_401_surfaces_error(http_mocker: HttpMocker) -> None: | ||
| """401 hits the FAIL branch: the read surfaces an error and emits no | ||
| partial records (rather than silently succeeding with 0 rows).""" | ||
| config = ClaudeAdminConfigBuilder().build() | ||
| http_mocker.get( | ||
| HttpRequest(_URL, query_params=ANY_QUERY_PARAMS), | ||
| HttpResponse( | ||
| body=json.dumps({"type": "error", "error": {"type": "authentication_error", "message": "invalid key"}}), | ||
| status_code=401, | ||
| ), | ||
| ) | ||
|
|
||
| output = read_stream(_CONNECTOR, _STREAM, config, expecting_exception=True) | ||
|
|
||
| assert output.errors, "a 401 must surface as a stream error, not a silent 0-row success" | ||
| assert len(output.records) == 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate relevant files"
fd -a 'test_claude_admin_reliability.py|.*claude.*admin.*reliability.*|.*claude.*admin.*\.(py)$' . | sed 's#^\./##' | head -100
echo
echo "Inspect target test file around 60-130"
tgt=$(fd 'test_claude_admin_reliability.py' . | head -1)
echo "target=$tgt"
wc -l "$tgt"
sed -n '1,150p' "$tgt" | cat -n
echo
echo "Search output.errors uses for ClaudeAdmin"
rg -n "read_stream|errors\\[|assert output\.errors|authentication_error|FAILED|FAIL" .Repository: constructorfabric/insight
Length of output: 34691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect read_stream implementation and output data shape"
wc -l src/ingestion/tests/connectors/connector_tests/source.py
sed -n '1,180p' src/ingestion/tests/connectors/connector_tests/source.py | cat -n
echo
echo "Search connector error handler / response status / failing error message references"
rg -n "CompositeErrorHandler|RetryableErrorHandler|response_action|authentication_error|status_code|FAIL|externalMessage|failureType|errors" src/ingestion/connectors/ai/claude-admin src/ingestion/connectors/ai/claude-enterpriseRepository: constructorfabric/insight
Length of output: 6934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
try:
from airbyte_cdk.models import EntrypointOutput
import inspect
print("EntrypointOutput fields:")
for name in dir(EntrypointOutput):
if not name.startswith("_"):
print(f" {name}")
print("\n__annotations__ / __fields__:")
print(getattr(EntrypointOutput, "__annotations__", None))
print(inspect.signature(EntrypointOutput))
print(inspect.getfile(EntrypointOutput))
print("\nDocstring:")
print(EntrypointOutput.__doc__)
# Inspect the module without importing/requiring a real Connector, if possible.
import json
import pathlib
import yaml
mod = __import__(EntrypointOutput.__module__, fromlist=["_airbyte_cdk"])
file_ = inspect.getfile(mod)
print(f"\nModule file: {file_}")
txt = pathlib.Path(file_).read_text(errors="replace")
for i, line in enumerate(txt.splitlines(), 1):
if any(s in line for s in ["class EntrypointOutput", "errors", "externalMessage", "failureType"]):
print(f"{i}: {line}")
entry = None
for i, line in enumerate(txt.splitlines(), 1):
if "class EntrypointOutput" in line:
entry = i
if entry:
print(f"\nFragment around EntrypointOutput:")
lines = txt.splitlines()
start = max(1, entry-3)
end = min(len(lines), entry+120)
for j in range(start, end+1):
print(f"{j}: {lines[j-1]}")
except Exception as exc:
print(type(exc).__name__, ":", exc)
import traceback
traceback.print_exc()
PYRepository: constructorfabric/insight
Length of output: 339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search EntrypointOutput definitions in installed tools/env if present"
python3 - <<'PY'
import distutils.sysconfig, site, sysconfig, pathlib, os
for p in site.getsitepackages() + [distutils.sysconfig.get_python_lib(), pathlib.Path(sysconfig.get_path("purelib")), pathlib.Path(sysconfig.get_path("platlib"))]:
print(p)
PY
echo
echo "Search for Airbyte entrypoint output structure in test logs/fixtures/helper references"
rg -n "EntrypointOutput|output\.errors\[0\]|errors=" src/ingestion tests scripts packages 2>/dev/null | head -200
echo
echo "Inspect claude-admin manifest error handler and auth setup"
sed -n '1,140p' src/ingestion/connectors/ai/claude-admin/connector.yaml | cat -n
sed -n '1,140p' src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py | cat -nRepository: constructorfabric/insight
Length of output: 1710
Make the 401 assertion specific.
assert output.errors allows any connector-level failure to pass, not necessarily the 401 authentication FAIL path. Assert the stable authentication/status detail from output.errors and keep the zero-record assertion.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 97-100: use JsonResponse instead of HttpResponse to send JSON data
Context: HttpResponse(
body=json.dumps({"type": "error", "error": {"type": "authentication_error", "message": "invalid key"}}),
status_code=401,
)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(http-response-with-json-dumps)
[info] 98-98: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"type": "error", "error": {"type": "authentication_error", "message": "invalid key"}})
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[error] 97-100: Lack of sanitization of user data
Context: HttpResponse(
body=json.dumps({"type": "error", "error": {"type": "authentication_error", "message": "invalid key"}}),
status_code=401,
)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(http-response-from-request)
🤖 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
`@src/ingestion/connectors/ai/claude-admin/tests/test_claude_admin_reliability.py`
around lines 92 - 107, Update test_error_fail_401_surfaces_error to assert that
output.errors contains the expected stable 401 authentication/status detail,
rather than only checking that errors is non-empty. Retain the assertion that
output.records has length zero.
cyberantonz
reviewed
Jul 27, 2026
| from pathlib import Path | ||
|
|
||
| # Local builder modules (config.py) are importable under --import-mode=importlib. | ||
| sys.path.insert(0, str(Path(__file__).parent)) |
Contributor
There was a problem hiding this comment.
Weird claude fix, it usually put the same string if fails to properly run code
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add concurrency control and Retry-After error handling to
claude-adminconnector to prevent rate-limit storms.The connector fires all streams in parallel without rate governance, causing 429 (Too Many Requests) responses that starve high-value streams (
messages_usage,cost_report) to 0 rows. A single sync generated 492× 429 errors.Root cause: Anthropic Admin API has org-wide rate limits. The manifest lacks
concurrency_leveland propererror_handlerwith Retry-After support.Fix: Apply the established repo pattern from
claude-enterprise/connector.yaml(same API, same credential):concurrency_level: {type: ConcurrencyLevel, default_concurrency: 1}to serialize stream executionCompositeErrorHandlerwith:WaitTimeFromHeader: Retry-Afteron 429ExponentialBackoffStrategyon 5xxThis ensures streams respect rate limits and properly backoff per Retry-After headers.
Changes
src/ingestion/connectors/ai/claude-admin/connector.yaml: Add concurrency control and error handler for rate-limit governanceRefs #1902
Summary by CodeRabbit
Retry-Afterand retrying transient 5xx with backoff.