fix(routing): let image requests wait out a rate-limit storm - #1223
Conversation
_invoke falls back to the text candidates when no candidate for a step is vision-capable, but the storm-wait admission recomputed only the vision-required set. It saw an empty set, found nothing to wait for and re-raised 429 at once. A figure-bearing conduct review therefore failed in under a second on a text-pool 429 storm that a text request waits out. Apply the same fallback in _invoke_with_rate_limit_recovery so the storm is judged on the candidates actually called. The wait budget, the pinned fail-fast, and breaker accounting are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ABB9sb4szFEteww67UYZy
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Changes이미지 요청 rate-limit 처리
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 1 files. (2 skipped: 1 unsupported, 1 too large.) ✨ Finishing Touches 💡 1📝 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 |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
exact-head review @ fe413e01b956bdc36086c4db137c52bca03c2dbb
실제 호출 후보 집합과 storm-admission 후보 집합을 맞추는 13-line causal fix 방향은 타당합니다. 다만 현재 GREEN이 이 PR이 해결한다고 주장하는 rate-limit wait invariant를 충분히 증명하지 않습니다.
현재 fixture는 provider가 _STORM_SECONDS = 1.5가 지나면 자동으로 200을 반환하고, 모든 429에 Retry-After: 2를 붙입니다. acceptance는 status == 200과 elapsed >= 1.5뿐입니다. 이 조합은 향후 구현이 provider를 짧은 간격으로 반복 호출하다가 1.5초가 지나 우연히 200을 받는 방식으로 회귀해도 통과할 수 있습니다. 즉 “429 storm을 기다렸다”와 “storm 동안 hammering하다가 끝났다”를 구분하지 못합니다.
그리고 PR 본문의 production evidence는 17/17 all-429 preflight에 retry_after_s가 없었다고 적고 있는데, 새 regression은 정반대로 Retry-After가 항상 있는 경로만 실행합니다. 실제 motivating incident에서 필요한 unknown-cooldown 경로가 executable acceptance에 없습니다.
RED/GREEN을 다음처럼 좁혀 주십시오.
- fake transport가 각 agent 호출의 monotonic timestamp와 attempt count를 기록하게 하고,
Retry-After: 2fixture에서는 다음 provider attempt가 effective cooldown 이전에 발생하지 않음을 검증합니다. 단순 wall-clock lower-bound만 보지 마십시오. - 동일 image/text pair에 Retry-After header 없음 fixture를 추가하여 현재 unknown-cooldown policy가 bounded wait 후 재평가되는지 검증합니다. 이게 이번 production evidence와 직접 대응합니다.
- wait budget을 넘기는 cooldown은 기존 fail-fast/budget contract를 보존하는지, pinned-model 경로가 여전히 기다리지 않는지도 negative control로 고정합니다.
- 가능하면 real 2초 sleep 대신 injectable clock/sleeper로 deterministic하게 만들어 Actions 부하/스케줄링 지연이 acceptance를 좌우하지 않게 하십시오.
consumer acceptance도 중요합니다. 본문이 명시하듯 deployed sidecar는 아직 767e67f라서 이 source PR 자체로 buyer-visible incident가 고쳐지지 않습니다. GREEN chain은 CO exact protected head → immutable release/client/schema evidence → .github sidecar pin ordinary version bump → success-inclusive Noema artifact에서 image-bearing final-429 share/latency 재측정까지 이어져야 하며 mutable sibling head를 소비하면 안 됩니다.
판정: candidate-set causal fix VALID / explicit Retry-After behavior PARTIAL / observed no-header path UNTESTED / anti-hammering invariant UNPROVEN / consumer release acceptance PENDING.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
CHANGELOG.d/image-request-rate-limit-storm-wait.md— repository behaviorcontextual_orchestrator/orchestrator.py— Python module behaviortests/test_image_request_rate_limit_storm_wait.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: image-request-rate-limit-storm-wait.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: image-request-rate-limit-storm-wait.md"]
R1 --> V1["required checks"]
Evidence --> S2["Python: orchestrator.py"]
S2 --> I2["Python module behavior"]
I2 --> R2["Review risk: Python: orchestrator.py"]
R2 --> V2["pytest plus coverage"]
Evidence --> S3["Test: test_image_request_rate_limit_storm_wait.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_image_request_rate_limit_storm_wait.py"]
R3 --> V3["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
fe413e01b956bdc36086c4db137c52bca03c2dbb - Workflow run: 35694810468
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: image-request-rate-limit-storm-wait.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: image-request-rate-limit-storm-wait.md"]
R1 --> V1["required checks"]
Evidence --> S2["Python: orchestrator.py"]
S2 --> I2["Python module behavior"]
I2 --> R2["Review risk: Python: orchestrator.py"]
R2 --> V2["pytest plus coverage"]
Evidence --> S3["Test: test_image_request_rate_limit_storm_wait.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_image_request_rate_limit_storm_wait.py"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
CHANGELOG.d/image-request-rate-limit-storm-wait.md— repository behaviorcontextual_orchestrator/orchestrator.py— Python module behaviortests/test_image_request_rate_limit_storm_wait.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: image-request-rate-limit-storm-wait.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: image-request-rate-limit-storm-wait.md"]
R1 --> V1["required checks"]
Evidence --> S2["Python: orchestrator.py"]
S2 --> I2["Python module behavior"]
I2 --> R2["Review risk: Python: orchestrator.py"]
R2 --> V2["pytest plus coverage"]
Evidence --> S3["Test: test_image_request_rate_limit_storm_wait.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_image_request_rate_limit_storm_wait.py"]
R3 --> V3["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
0aad1190d6104f61867234e7089ff9618d96fea7 - Workflow run: 36081670283
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: image-request-rate-limit-storm-wait.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: image-request-rate-limit-storm-wait.md"]
R1 --> V1["required checks"]
Evidence --> S2["Python: orchestrator.py"]
S2 --> I2["Python module behavior"]
I2 --> R2["Review risk: Python: orchestrator.py"]
R2 --> V2["pytest plus coverage"]
Evidence --> S3["Test: test_image_request_rate_limit_storm_wait.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_image_request_rate_limit_storm_wait.py"]
R3 --> V3["targeted test run"]
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0aad1190d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Admission correction — exact current head |
…s-20260920' into fix/image-request-rate-limit-storm-wait-2
|
Exact-head Ready-admission repair Audited head Current substantive blocker evidence:
Queued/pending/in-progress Checks are not blockers and were not treated as failures. This PR is being returned to Draft/Proposed so review admission does not imply readiness while the recorded blocker remains. Preserve the branch and complete the causal source/review/topology repair on a new non-force commit; then re-fetch this exact head's Checks and reviews before restoring Ready. No merge, close, bypass, review dismissal, synthetic status/approval, manual rerun, force push, or destructive rebase is authorized by this receipt. |
…e-limit-storm-wait-2
1 participant
Current head — 2026-09-27
90911687cb1ee49325ddd1f2bdfd29284a526028. A non-destructive merge of #1209 at84736f4d9840b3d06edb8fdcc7210abddd86175eremoved the 69-commit behind count. The current branch is 0 behind/4 ahead and the PR diff remains limited to the original three files. GitHub reports the merge result CLEAN.CHANGES_REQUESTEDfrom prior heads; neither supplies a source-backed product finding, but the review state remains active. Independent approval and terminal required checks are still missing. fix(ci): repair protected-main security and runtime regressions #1209 remains Draft, and central CodeQL publication remains owned by .github#1929 and .github#2276.Stacked-base validation — 2026-09-26
This PR now targets #1209 at base
84736f4d9840b3d06edb8fdcc7210abddd86175e. The three-file PR diff is identical to the earliermaincomparison; a temporary merge was conflict-free. The image/rate-limit/provider-error subset passed 68/68 under Python 3.14 with warnings as errors. A fresh temporary merge with project-local Python 3.12 and the locked dependencies/native decision extension ran the same full-suite command as the Security and Quality job,uv run --no-sync python -m pytest -q -ra: 5,075 passed, 5 skipped, 3 warnings, exit 0 in 735.81 s. This is complete local full-suite evidence for the combined source tree, while hosted CI and its other gates remain pending. A broader Python 3.14 run was stopped at 11% after 12 failures, 585 passes and one error; it is not full-suite acceptance and is not equivalent to the hosted Python 3.12 gate. A bounded repeat stopped at the first strict-warning failure:test_batch_embeddings.pyencountered three unclosed socket warnings during garbage collection after 161 earlier tests. That file passes alone (19/19), and the new image regression file had not run yet. The hosted full-suite command uses Python 3.12 without-W error, so this local warning observation cannot be treated as its verdict. The failed runs described below were produced against the previousmainbase. Fresh hosted runs against this merge result are required before treating any required check as current-base evidence. #1209 remains Draft and must be approved and merged before this stacked PR can reach protectedmain.Structure → Gap (from real Noema logs)
W2 (2026-09-21 11:28Z onward): 11 of 16 review requests ended with a final 429; the median request took 1,050 s. Decomposed by
request_id:RemoteDisconnectedon NIMgemma-4-31b-itacross the 11 requests.ling-3.0-flash-{fin,sante}:freemodels. Both answer 429 within about 0.1 s, and the request fails immediately, discarding the finished work.767e67fhas no storm-wait at all (_invoke_with_rate_limit_recovery/rate_limit_wait_secondscame later). Main has it (default budget 30 s). Picking up main's storm-wait means bumping the.githubsidecar pin, which is outside this repo._invokefalls back to text candidates when no step candidate is vision-capable, but the storm-wait admission recomputed only the vision set. That set was empty, so it concluded "nothing to wait for" and re-raised 429.Change (13 lines)
Apply
_invoke's own no-vision fallback in_invoke_with_rate_limit_recovery, so the storm is judged on the candidates actually called. Unchanged: wait budget, pinned-model fail-fast, Retry-After/unknown cooldown, breaker/429 accounting, ZDR and allowlists. No conflict with #1222 (_invokefinal error) or .github#2339 (preflight).Tests
tests/test_image_request_rate_limit_storm_wait.pysends a real HTTP conduct request through a fake transport atModelClient._open_provider, with no network.[image]fails in 3 of 3 runs;[text]passes.-W error).provider_reliabilityfailures are pre-existing locally on base.Current-head review follow-up (0aad119)
Retry-After: 1and with no cooldown header. A zero wait budget returns 429 without replay. Existingtest_explicit_concrete_model_single_candidate_storm_fails_fast_without_waitingkeeps the pinned-model control.5665b0ad: the two image cases fail with 429 while text cases pass. GREEN on0aad1190:python3 -m pytest tests/test_image_request_rate_limit_storm_wait.py tests/test_rate_limit_aware_admission.py tests/test_provider_error_taxonomy.py -q -W error --tb=shortpassed 54 tests locally. Hosted checks failed on this head; the current OpenCode review requests changes because coverage admission failed.OpenCode coverage root cause — 2026-09-27
The older CHANGES_REQUESTED coverage receipt is an infrastructure failure, not a measured CO coverage deficit. In the canonical .github run 36081670283, coverage job 107989271158, Docker build failed at line 89:
/requirements-noema-document-ci-hashes.txt: not found. It failed before any target PR test execution. The trusted workflow copied only the OpenCode requirements into its isolated build directory, while its Dockerfile required both OpenCode and Noema document locks.The causal source repair is already owned by ContextualWisdomLab/.github#2286 and carried in the ordinary convergence head of ContextualWisdomLab/.github#2385, exact
372f5b8bb1ae1bb32ab29e9afbe363d81aed81e3. Current source inspection confirms that this head checks the document lock is a regular non-symlink file and copies it into the Docker build context, retaining hash-only installation. The same owner also carries the CodeQL credential-path and AnyIO security repairs. Its current-head checks are queued, so delivery is not yet proved. Do not create duplicate CO source changes or dismiss the reviews as a substitute for a fresh authoritative verdict. After protected delivery of that owner, validate a fresh exact CO head/base coverage receipt and independent review.Current-head CI triage (2026-09-26)
The exact
0aad1190Security and Quality run failed before it could assess this PR's full suite. Its tests job stops during collection becausetest_provider_embedding_batch_backend.pyimports a constant absent fromcost_router.py; the same error reproduces on unmodifiedmain@5665b0ad. The fuzzing and CodeQL/SBOM jobs reject the unhashablefast-mlsirmGit requirement under--require-hashes. The Rust job lacksrustfmtin pinned toolchain 1.97.1.#1209 carries repairs for all three baseline classes but remains Draft with review changes requested. #1266 repairs collection and Rust only; its current fuzz and CodeQL/SBOM jobs are failing. A throwaway merge of #1223 into exact #1209 head
84736f4dwas conflict-free, and the image/rate-limit/provider-error suites passed 68/68 with warnings as errors. This is local integration evidence, not hosted acceptance or approval.The separate CodeQL PR run 36022489108 also has three failed compatibility shards. Their exact logs report
VERDICT_STATE=pendingafter dispatch. The head has no publishedcodeql-dispatch/<language>commit statuses, so a terminal CodeQL verdict and the promised exact-job wake are still missing. The same state appears on #1209's current head; rerunning pending shards without an authenticated terminal verdict would fail by design. The central owner paths are .github#1929 for terminal status publication and .github#2276 for Code Scanning analysis-read permission; the proposed credential selector #2275 is still Draft. The selected App installation currently lacks the required analysis-read and commit-status-write permissions according to the owner audit. Unblock only after the trusted handler proves both permissions on an unchanged target head, publishes an authenticated terminal verdict, and settles these exact jobs. This is distinct from this PR's failing CodeQL/SBOM job above.The current OpenCode review requests changes because coverage admission failed. The separate Noema review job ended with a gateway 504 after 1,562 s; it does not prove this source change failed or that deployed image 429 recovery works. A protected CO head, independent approval, an immutable consumer pin, and a success-inclusive post-deploy measurement remain required.
Evidence boundary
767e67f..github#2326's success-inclusive artifacts.🤖 Generated with Claude Code
https://claude.ai/code/session_012ABB9sb4szFEteww67UYZy
Summary by CodeRabbit
버그 수정
Retry-After처리는 유지됩니다.테스트