Skip to content

fix(grok): account for metered preflight failures - #6035

Closed
luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:codex/propose-fix-for-grok-devin-429-issue
Closed

luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:codex/propose-fix-for-grok-devin-429-issue

Conversation

@luvs01

@luvs01 luvs01 commented Sep 27, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Bind metered usage from a Devin/Grok preflight refusal to the request transport context before formatting the HTTP error, so reported usage is not discarded.
  • Exercise both requested stream modes and usage-journal finalization. Because the refusal occurs before SSE starts, this test is not a successful streaming response; journal persistence is not by itself proof that a separate API-key budget reservation was settled.
  • Follow-up 064170078c749519645a617309ea1a8942b7e040 integrates observed dev 99d0a9400ebd8dedbefee9882698ecfb1f7bbaa7. Resolve the preflight test-file conflicts by retaining both this PR's usage assertions and dev's heartbeat/timeout/account-change tests. Runtime changes merged without conflict. Existing commit history is preserved.

Verification

Exact integration-head native Bun 1.4.0 Linux validation:
https://github.com/luvs01/opencodex/actions/runs/36295622182/job/108553981795

Passed tests/responses/responses-grok-devin-preflight.test.ts, project typecheck, privacy scan, structure check, file-size checks, test-layout checks and clean-tree validation. Both parents were verified as ancestors. The resulting diff against the incoming base remains limited to the one runtime binding statement, the owning spend contract and the focused preflight tests.

The test description/comment now explicitly limits the durable evidence to the usage journal rather than claiming API-key budget settlement. No full-repository or cross-platform acceptance is claimed. The helper workflow is outside this PR's tree and ancestry. Required latest-head PR CI and independent maintainer review remain separate. No merge into dev, force push or review dismissal was performed.

Checklist

  • Scope stays focused on preserving reported preflight usage.
  • Both sides' current tests preserved during conflict resolution.
  • Exact integration-head focused tests and repository gates passed.
  • Latest-head required CI and maintainer review complete.

Summary by CodeRabbit

  • Bug Fixes
    • Pre-output Grok and Devin rate-limit errors now retain reported usage for streaming and buffered requests, including in the final usage log.
    • Deferred spend settlement now preserves pending sends for retry when ownership or storage errors occur. If the ledger lease has ended, the lease-not-held condition is discarded while other errors continue to propagate.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 27, 2026
@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ea11e27d-4933-44c6-80d5-f598ec71a1b7

📥 Commits

Reviewing files that changed from the base of the PR and between 3f9ebcf and 0641700.

📒 Files selected for processing (3)
  • src/server/responses/run-turn-execution.ts
  • structure/transports/responses-spend.md
  • tests/responses/responses-grok-devin-preflight.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.


📝 Walkthrough

Walkthrough

The Grok preflight 429 path now binds usage reported on the error event before returning the refusal. Tests cover streaming and buffered requests, including final usage entries in usage.jsonl. The durable-spend documentation describes pre-output Grok and Devin 429 usage handling.

Changes

Preflight Usage Capture

Layer / File(s) Summary
Bind and verify preflight usage
src/server/responses/run-turn-execution.ts, tests/responses/responses-grok-devin-preflight.test.ts, structure/transports/responses-spend.md
The Grok 429 response binds usage reported on the error event. Tests check request-log usage for streaming and buffered requests, then verify that finalization writes token counts to usage.jsonl. The documentation describes usage binding for pre-output Grok and Devin 429 events and deferred-settlement error handling.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 06417

Reported preflight usage is retained for logging and spend settlement. No unresolved issue identified here prevents merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 06417

The change records metered usage that was previously lost when a provider refused a request before output. Review found no new public access path or verified security regression. Accounting behavior during retries and in configurations outside the tested path remains less certain.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The new usage assignment is scoped to the current request log and active attempt, rather than selecting another request or account by an error-supplied identifier. Its downstream effect is that request's logging and spend settlement.

Trust Boundaries and Controls

  • observed — Usage originates in an adapter-reported error, not a newly exposed request field. The tested route uses OAuth Devin; the same binder deliberately ignores its supplied usage on API-key routes.

Resilience and Maintainability Implications

  • observed — When preflight failover rotates to another OAuth account, it can consume an error before the new refusal-path binding runs. The supplied tests cover direct refusals, not metered errors discarded during rotation; the inspected rotation path predates this added binding.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: accounting for metered usage reported by Grok preflight failures. It matches the implementation, tests, and documentation updates.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 34 / 80

Grok가 Devin에게 답을 받기 전에 429(요청이 너무 많음)가 나면, 그 에러 안에 이미 쓴 토큰 수가 들어 있을 수 있다. 지금은 거절 응답을 만들면서 그 숫자를 버린다. 사용량 기록에 안 남는다.

이 PR은 거절을 돌려주기 직전에 그 숫자를 기록장에 붙인다. 붙이는 함수는 bindKeyUsageFromBridge다. 스트리밍과 한 번에 받는 응답이 같은 grokRateLimitResponse를 쓰므로 한 줄로 둘 다 고친다. 테스트와 structure/transports/responses-spend.md 문장도 같이 있다. 베이스는 dev다. 같은 수정의 다른 열린 PR은 없다.

tests/responses/responses-grok-devin-preflight.test.ts - dev와 이 브랜치가 같은 자리(기존 "pre-output 429 reaches Grok as HTTP 429" 다음)에 서로 다른 테스트를 넣었다. GitHub 머지 상태는 dirty다. git merge-file로 보면 run-turn-execution.ts와 responses-spend.md는 자동으로 합쳐진다. 테스트 파일만 충돌한다. dev 쪽은 쿨다운 하트비트와 계정 바꾸기 테스트이고, 이 PR 쪽은 사용량 테스트다. 둘 다 남겨야 한다. PR 본문은 현재 dev에 깨끗이 올라간다고 적었는데, 테스트는 겹친다.

tests/responses/responses-grok-devin-preflight.test.ts:117 - 테스트 이름은 "usage.jsonl에 오래 가는 지출을 정산한다"이다. 하는 일은 handleResponses가 끝난 뒤 테스트가 addFinalRequestLog를 직접 부르고, 마지막 줄의 토큰 수만 본다. spend는 안 본다. 넘기는 기록장에는 spendTracker도 없다. 스트리밍인데 closeReason은 항상 "non_stream"이다. 증명되는 것은 토큰 숫자가 로그 칸에 복사된다는 것까지다.

src/server/responses/run-turn-execution.ts:522 - 토큰이 없는 429에도 bindKeyUsageFromBridge를 부른다. 그 함수는 숫자가 없어도 usageFromBridge를 켠다. 이 표시는 나중에 응답 본문에서 사용량을 다시 읽는 것을 막는다. 거절 본문에는 토큰이 없어서 빠지는 숫자는 없다. 숫자가 있을 때만 불러도 된다.

메인테이너의 판단이 필요한 지점

dev 위로 다시 얹은 뒤 테스트 충돌만 손으로 풀면 된다. 쿨다운 테스트와 사용량 테스트를 둘 다 남길지 정하면 된다. 두 번째 테스트를 이름대로 지출(spend)까지 볼지, 이름을 "usage.jsonl에 토큰이 남는다"로 낮출지도 같이 정하면 된다.

너의 추천

리베이스해서 두 테스트 묶음을 같이 둬라. 본 수정 한 줄은 맞다. 토큰이 있을 때만 bindKeyUsageFromBridge를 불러라. 두 번째 테스트는 spend를 확인하거나, 확인하지 않으면 이름에서 정산이라는 말을 빼라. 그 다음이면 머지해도 된다.

이 댓글은 grok-bot이 작성했습니다

@Ingwannu

Copy link
Copy Markdown
Owner

Hold on exact head 3f9ebcf: the one-line usage-binding fix is sound on static review, but the branch is currently merge-conflicted, 35 commits behind, and has no executable exact-head validation. During rebase, preserve both the base cooldown/account-rotation regressions and this PR's usage cases. Also either narrow the test and body claim that durable spend is settled or exercise a real send-budget reservation and assert the durable ledger or settled total; current mocks prove usage-log binding, not ledger settlement. Note: the test does receive a spendTracker indirectly through handleResponses to createInferenceSendBudget to attachRequestSpendTracker; absence of a tracker is not the issue.

luvs01 commented Sep 27, 2026

Copy link
Copy Markdown
Collaborator Author

Author follow-up 064170078c749519645a617309ea1a8942b7e040 resolves current-dev test conflicts while preserving both the metered-usage regressions and upstream heartbeat/timeout/account-change cases. The test wording now correctly claims usage-journal persistence, not separate API-key budget settlement or a successful SSE response. Native Bun validation of the exact integrated head and type/privacy/structure/file-size/test-layout gates passed: https://github.com/luvs01/opencodex/actions/runs/36295622182/job/108553981795 . The runtime binding remains a one-statement scoped diff against dev. Existing history and required review/CI gates are preserved; please re-review.

@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev in #6061 (merge 06d7914e6a) as one squashed commit that keeps your authorship. Thank you. Closing because this repository merges into dev, so GitHub does not close carried PRs automatically.

@lidge-jun lidge-jun closed this Sep 27, 2026
robin-bially pushed a commit to robin-bially/opencodex that referenced this pull request Sep 27, 2026
Carried from lidge-jun#6035 into merge train round 3.

Co-authored-by: Epinephrine <luvs01@hanmail.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants