fix(routing): preserve retryable failure on exhausted pool - #1222
Conversation
When every orchestrator/free candidate failed, _invoke raised whichever classified upstream error came last. Two provider timeouts followed by one model's 400 therefore answered 400 invalid_request_error, telling the caller the request was permanently invalid. The same outcomes in another order answered 504. Noema evidence shows 5 of 10 final-400 review requests had earlier transient failures, so a bounded capacity re-dispatch was skipped. Track the last retryable upstream failure and surface it when the last failure is non-retryable. An all-400 pool still reports 400. Retry and replay authorization, the breaker, and 413/429 handling 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 25 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 (6)
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Files not reviewed due to moderation or processing errors (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough후보 풀이 소진되면 마지막 오류가 비재시도 가능해도 앞서 발생한 재시도 가능한 오류를 반환합니다. 가상 선택자와 구조화된 합성 요청에서는 쿨다운 후보를 요청 예산에 따라 재시도합니다. 관련 회귀 테스트와 변경 로그를 추가했습니다. Changes후보 풀 오류 및 쿨다운 복구
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The changed failure and cooldown paths show no identified blocker to merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to A transient failure can now take precedence over a later provider authentication or permission rejection, potentially prompting callers to retry a request they previously would have stopped. No gateway authorization bypass is established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 53.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 3 files. (2 skipped: 1 unsupported, 1 too large.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning Review coverage is incomplete: 1 file could not be fully reviewed. Findings from completed review steps are included; see review info for details. 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 |
|
Verification of exact head
|
![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/exhausted-pool-retryable-final-error.md— repository behaviorcontextual_orchestrator/orchestrator.py— Python module behaviortests/test_exhausted_pool_final_error_order.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: exhausted-pool-retryable-final-error.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: exhausted-pool-retryable-final-error.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_exhausted_pool_final_error_order.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_exhausted_pool_final_error_order.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:
fc5685dcb61f0814e4e838700751ca812a1b0bda - Workflow run: 35690707096
- 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: exhausted-pool-retryable-final-error.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: exhausted-pool-retryable-final-error.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_exhausted_pool_final_error_order.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_exhausted_pool_final_error_order.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. |
seonghobae
changed the title
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/exhausted-pool-retryable-final-error.md— repository behaviorcontextual_orchestrator/orchestrator.py— Python module behaviortests/test_exhausted_pool_final_error_order.py— regression suitetests/test_rate_limit_aware_admission.py— regression suitetests/test_structured_output_distinct_fallback.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: exhausted-pool-retryable-final-error.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: exhausted-pool-retryable-final-error.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_exhausted_pool_final_error_order.py (3 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_exhausted_pool_final_error_order.py (3 files)"]
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:
128e9bc9513ec37232589111d87cd98c656b13d0 - Workflow run: 36188050429
- 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: exhausted-pool-retryable-final-error.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: exhausted-pool-retryable-final-error.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_exhausted_pool_final_error_order.py (3 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_exhausted_pool_final_error_order.py (3 files)"]
R3 --> V3["targeted test run"]
|
Admission correction — exact current head |
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: 240a9ed8fa
ℹ️ 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".
|
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. |
Pull request was converted to draft
1 participant
Structure → Gap
Baseline on the real review path: 89 Noema runs, 72 with sanitized sidecar logs, 2026-09-16..21.
Gap: when every
orchestrator/freecandidate fails,_invokeraises whichever classified upstream error came last. The same provider outcomes can give different caller retryability depending on their order.A caller treats 400 as a permanent request fault and skips its bounded capacity re-dispatch.
Before/after on real data
10 real review requests ended in 400. In 5 of them, earlier candidates had failed transiently (for example 8 transient failures out of 9 attempts). With this change those 5 surface a retryable 5xx/429. The other 5 had no transient failure and still surface 400.
The sample is failure-biased because evidence was uploaded only on failure. Re-measure after
.github#2326.Change
Track the last retryable upstream failure and surface it when the last failure is non-retryable. This stabilizes retryability, while the exact status among retryable failures can still vary by order. For virtual model requests, wait within the configured request budget for a cooled 429 candidate after other eligible candidates fail, then retry it once. Structured synthesis applies the same bounded recovery. Explicitly selected models keep their immediate error behavior.
Tests
tests/test_exhausted_pool_final_error_order.pygoes through the real HTTP server, with a fake transport atModelClient._open_providerand no network.origin/main5665b0a:[504,504,400]returns 400.-W error), including 429 and 413 boundaries with one transport call per candidate.test_provider_reliability.pyfailures are pre-existing locally on base.test_invoke_preserves_final_classified_failure_across_candidates(all-429) still passes.Latest verification
OpenCode change-request RCA, 2026-09-27 05:55 UTC
128e9bc9cites central.githubrun36188050429. Its coverage-evidence job 108295187524 failed at Dockerfile line 89:requirements-noema-document-ci-hashes.txtwas absent from the build context. The log endsTrusted coverage tool image build failed before PR execution. This is a trusted-tool materialization defect, not measured product coverage failure; the model review was skipped by this gate.372f5b8bb1ae1bb32ab29e9afbe363d81aed81e3; it includes the lock materialization repair and its exact-head checks remain queued/pending. Protected integration of that owner repair precedes a fresh exact-head review here. This does not authorize dismissing reviews or bypassing checks.Draft/review admission RCA, 2026-09-27 05:52 UTC
/pulls/1222/reviewsshow those reviews belong tofc5685dcand128e9bc9, not current048d90b3. They remain historical review evidence, not current-head product findings. The current-head P1 was repaired in048d90b3and its thread resolved.36289442917; the subsequent run36295796324skipped. Restored Ready on unchanged head at 05:51 UTC to reopen substantive review/CI admission. No approval was dismissed or fabricated; protected merge still requires actual exact-head approvals and checks. The actor's local Ready-admission implementation is not yet located, so its source repair is not claimed.Verified external wait, 2026-09-27 02:50 UTC
048d90b3. These are live external waits, not passing verdicts or permission to merge.Current HEAD
048d90b3, 2026-09-27orchestrator/freecould stop at a first 400 before checking a later 429 candidate. The reverse-order HTTP case failed with 400 before this fix. Commit048d90b3defers provider 400 until the eligible candidates are checked; 401/403 remain terminal. The review thread was answered and resolved.tests/test_actions_model_fallback.py,tests/test_structured_output_distinct_fallback.py,tests/test_rate_limit_aware_admission.py,tests/test_exhausted_pool_final_error_order.py; sibling project interpreter,-c /dev/null -p no:cacheprovider -W error). Both 429→400 and 400→429 succeed through the HTTP boundary with one bounded 429 retry. No current-head hosted approval or gate verdict is claimed yet.Hosted gate attribution, 2026-09-27 (previous HEAD
240a9ed8)pip --require-hashesrejects a VCSfast-mlsirmentry, andcargo fmtlacks rustfmt. The five implicated files are byte-identical toorigin/main@5665b0ad; this PR changes none of them. Draft #1209 carries those exact repairs and its Security and Quality run 36138545926 passed all four jobs, but it is not merged.240a9ed8, SAST Semgrep and Security Scan passed. Formal OpenCode reviews still cite older heads (fc5685dc,128e9bc9); no current-head Noema or OpenCode approval is established.240a9ed8, the first Noema run 36238087384 and OpenCode run 36238087376 were green because the PR was Draft at execution: Noema skipped verdict preparation and OpenCode skipped the review request. Ready for review was restored on 2026-09-27 without changing the head; auto-merge was requested. CodeQL dispatch run 36287525429 was queued for that head.At
240a9ed8, the new HTTP front-door regression is RED against archived predecessor128e9bc9(HTTP 400invalid_request_error, 1 failed) and GREEN on that head with-W error(1 passed); its adjacent HTTP controls passed too (5 passed). It runs the conduct stages and structuredorchestrator/freerequest through the server with a fake synthesis client, verifying 429 → 400 → bounded retry success, route receipt, and one failure observation per rejected candidate. This is local boundary evidence, not a deployed Noema run.At
03d4962d, the three focused routing suites pass with warnings treated as errors: 68 tests, process exit 0 (../.venv/bin/python -m pytest -q -c /dev/null -p no:cacheprovider -W error tests/test_structured_output_distinct_fallback.py tests/test_rate_limit_aware_admission.py tests/test_exhausted_pool_final_error_order.py). The local pytest installation lacks the configured asyncio option;-c /dev/nullavoids that unrelated config warning, and disabling pytest's cache avoids writing under/dev. The new structuredorchestrator/freeregression is RED before the fix for 429→400 and GREEN after it; 401/403 still fail closed, and a repeated 429 is retried only once. The observed Noema workflow installed an older, pinned gateway SHA (767e67fb), so its 429 result is not execution evidence for this patch. Current-head OpenCode coverage preparation and repository security jobs are blocked by separate CI defects tracked in ContextualWisdomLab/.github#2385 and this repository's #1209; Noema continuation permissions are tracked in ContextualWisdomLab/.github#2372.Scope
Separate from #1209 (shared CI repair) and #1221 (breaker/quarantine). Same boundary rule as AGENTS.md's exhausted-pool classification guidance.
🤖 Generated with Claude Code
https://claude.ai/code/session_012ABB9sb4szFEteww67UYZy
Summary by CodeRabbit
2026-09-27 capacity RCA and native-policy recovery
completed/cancelled; three changed/terminal targets were skipped. Autofix was left alone. No Ready PR, review verdict, security gate, or source HEAD was modified.048d90b3715f792bd6a779d0b013c665fdb01385, Ready. Security run36298416892and central-owner #2385 checks still required runner admission at the last direct check. Queue cleanup is verified; successful review, CI, and protected merge are not yet established.