Skip to content

fix(codex): fence late WebSocket quota by credential generation - #3934

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/ws-quota-credential-generation-20260908
Closed

fix(codex): fence late WebSocket quota by credential generation#3934
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:agent/ws-quota-credential-generation-20260908

Conversation

@luvs01

@luvs01 luvs01 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Summary

A streaming Codex request can keep receiving WebSocket quota metadata after its pool credential is replaced under the same local account ID. Previously, a late frame could repopulate the quota cleared for that replacement and make the new credential appear exhausted.

Capture the selected pool credential generation when creating the response's quota observer and ignore subsequent observations once that generation is no longer live. Existing configuration-generation fencing and the main-pool writer remain unchanged. The regression delivers quota 10, replaces the credential, clears quota, and then delivers quota 100 from the old connection.

This carries only the source change and regression from a40f419fe0e1cf29faeb77599cd61bf535be7e5d, preserving its author. It restores the existing serving-account ownership invariant without adding configuration, logs, or a new user workflow.

Verification

Head e5c01f44e9736baba5b3a993c7f489f6b60d5ddd, based on dev 2abf071e0a3e765195ea2997b962171c87151f06.

  • Bun 1.4.0 on Windows: bun test tests/responses/responses-account-label.test.ts tests/lab/core-lab-boundary.test.ts --timeout 60000 passed 25 tests with 80 assertions. This includes existing pool/main-pool prelude and final quota behavior and the core import boundary.
  • Removing only the generation check made the new regression fail: the cleared quota was repopulated with weeklyPercent: 100. The original source bytes and SHA-256 were restored afterwards.
  • bun run typecheck, bun run privacy:scan, and git diff HEAD^ HEAD --check passed.
  • Independent read-only code/security review found no blocking defect: generation 0, deletion/replacement, main-pool ownership, observer exception containment, and the final-header path were checked. No required additional regression was identified.
  • bun run test:changed --timeout 60000 reached the wrapper's 900-second suite deadline and exited 124. It produced no completed selection summary, so this run is not a passing result. The scoped tests above passed separately; Full cross-platform CI initially finished with 24 successful jobs, a macos 2/2 cancellation at its 20-minute job limit, and a failed aggregate. The specific timed-out job and its dependent aggregate then passed on the unchanged head: attempt 2 is complete with 26/26 jobs successful. CodeRabbit completed review of this head with no actionable findings and there are no unresolved review threads. Readiness is supported by this exact-head cross-platform CI; the separate local 900-second limit remains recorded above.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

  • All CI tests are green on my local testing.
  • I pushed my PR to the latest dev commit.
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes

    • Prevented outdated WebSocket quota updates from restoring cleared quota information after account credentials are replaced.
    • Ensured quota state remains accurate when delayed usage reports arrive from inactive credentials.
  • Tests

    • Added coverage for delayed quota reports following credential replacement and quota clearing.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 89f71fd9-810f-449f-8d14-4ca2a09b35ed

📥 Commits

Reviewing files that changed from the base of the PR and between 2abf071 and e5c01f4.

📒 Files selected for processing (2)
  • src/server/responses/core.ts
  • tests/responses/responses-account-label.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

codexWsQuotaObserver now ignores quota headers from replaced pool credentials. A test verifies that a late WebSocket message cannot repopulate cleared quota state.

Changes

Quota generation validation

Layer / File(s) Summary
Generation-gated quota observation
src/server/responses/core.ts, tests/responses/responses-account-label.test.ts
codexWsQuotaObserver captures pool credential generations and applies quota only while the generation remains live. The test covers a late message from a replaced credential and confirms that cleared quota remains null.

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

Merge Risk: ⚪ Minimal · up to e5c01

This change prevents stale WebSocket quota metadata from a replaced pool credential from restoring cleared quota state. The targeted behavior is covered by a regression test, with no active merge-blocking risk identified.

Suggested reviewers: lidge-j

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. 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: preventing late WebSocket quota updates from applying after a Codex credential generation is replaced.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@luvs01

luvs01 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

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

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

Hygiene

Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 70 / 80

이 PR은 Codex 풀 계정에서 WebSocket으로 흘러 들어오는 늦은 쿼타 프레임이, 같은 로컬 account id로 자격 증명을 교체한 뒤에도 예전 연결의 쿼타를 다시 채워 넣던 구멍을 막습니다. 지금 dev HEAD는 942c02873(팁 #3931, package 2.48.0)이고, HEAD의 src/server/responses/core.ts codexWsQuotaObserveraccountIdwriterGeneration(설정 세대)만 잡고 applyCapturedCodexQuota를 호출합니다. 스트리밍 중에 savePoolCredential로 같은 id의 풀 자격 증명을 바꾸면 디스크 쿼타를 비워도, 옛 WS가 나중에 codex.rate_limits를 보내면 새 자격이 “이미 소진”된 것처럼 보일 수 있습니다. 이 브랜치는 observer를 만들 때 pool kind의 authCtx.generation을 캡처하고, 콜백마다 기존 isCodexAccountGenerationLive(src/codex/account-store.ts)로 살아 있는 세대인지 확인한 뒤에만 쿼타를 씁니다. main-pool과 설정 writerGeneration 펜스는 그대로입니다. 회귀는 쿼타 10 → 교체·clear → 옛 소켓이 100을 보내도 null이 유지되는 시나리오를 tests/responses/responses-account-label.test.ts에 넣었습니다. 베이스는 tip보다 조금 뒤(2abf071e0, #3927)지만 MERGEABLE이고 #3931(GUI)과 파일 겹침이 없습니다. types/config 분할과 무관합니다.

새 설정·로그·GUI 없이 “서빙 중 자격이 교체되면 그 연결의 늦은 관측은 버린다”는 기존 소유 불변식만 복구합니다. 본문이 말하는 대로 configuration-generation 펜스와 main-pool writer 경로는 손대지 않았고, pool일 때만 credential generation 가드를 켭니다. 헬퍼 isCodexAccountGenerationLive는 이미 HEAD에 있어 새 스토어 API를 만들지 않습니다. 작성자 검증은 Windows에서 해당 테스트·lab-boundary 25테스트/80assertion 통과로 적혀 있습니다. 릴리스 쪽에서는 #3933이 방금 main에 로스터 최종 보정을 올렸고, 이 PR은 dev 런타임 버그 픽스라 채널 승격과 직접 묶이지 않습니다. draft라서 제품 머지 전에 호스트 CI 초록만 보면 됩니다.

라인 src/server/responses/core.ts codexWsQuotaObserver - pool이면 credentialGeneration을 캡처하고, live가 아니면 return. 교체된 자격의 늦은 WS 쿼타를 차단하는 핵심 한 줄이다.
경로 src/codex/account-store.ts isCodexAccountGenerationLive - 이미 HEAD에 있는 세대 검사. 이 PR은 import·호출만 추가한다.
경로 tests/responses/responses-account-label.test.ts - MetadataSocket으로 rate_limits 10 → 교체/clear → 100 순서를 고정하고, 최종 쿼타가 null인지 본다. 재현 시나리오와 1:1이다.
경로 authCtx.kind main-pool - generation 가드 없음(의도). mainQuotaWriter·writerGeneration 경로는 유지된다.
경로 PR draft / merge-base - tip #3931 이후에도 MERGEABLE. draft 체크리스트·전체 Cross-platform은 이 exact head에서 아직 마무리 전이다.

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

  • pool만 세대 펜스하고 main-pool은 빼 둔 현재 경계가 운영 의와 맞는지(본문·코드 일치)
  • 늦은 관측을 조용히 drop하는 것이 충분한지, 관측/메트릭으로 한 번 남길지
  • draft ready 기준을 작성자 초점 테스트로 둘지, Cross-platform 필수 통과로 둘지

너의 추천
실사용 풀 교체에서 쿼타가 다시 채워지는 실제 버그라 점수를 높게 잡았다. CI가 이 head에서 초록이면 draft 해제 후 dev 머지. #3931과 충돌면이 거의 없으니 리베이스 지옥도 아니다. types/config 분할 close-don't-rebase 대상 아님.

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

@lidge-jun

Copy link
Copy Markdown
Owner

Carried into the integration stack as PR #3937.

Your commit e5c01f44e was cherry-picked onto current dev with its original authorship intact, plus a Co-authored-by: luvs01 <luvs01@hanmail.net> trailer so the credit survives the squash. It is the tip layer of the stack, on branch codex/b-stack-l2-ws-quota-generation.

An independent read-only audit examined the account-boundary questions this change raises. main-pool not being fenced here is correct rather than an oversight: CodexAuthContext declares generation only on pool, and main-pool keeps its own mainQuotaWriter identity check (src/codex/auth-context.ts:114, src/codex/quota.ts:241). writerGeneration and generation are distinct — configuration reconciliation versus the persisted credential record — so the new check is not redundant with the existing fence. Generation 0 is handled correctly because isCodexAccountGenerationLive uses strict equality against a live record (src/codex/account-store.ts:73,197), and an observer exception stays contained in publishQuota (src/server/responses/codex-ws-metadata.ts:94,119).

One accepted tradeoff worth naming: after an ordinary same-account refresh, quota frames from a stream that captured the older generation are now discarded, so quota can read stale until the next observation. That matches the existing WHAM quota path's rule, so it is a consistency choice rather than a regression.

This PR will be closed once #3937 merges into dev; the change lands with your authorship. Thanks for the precise regression that delivers quota, replaces the credential, clears state, then delivers the late frame.

@github-actions
github-actions Bot marked this pull request as ready for review September 7, 2026 18:26
lidge-jun added a commit that referenced this pull request Sep 7, 2026
…ebSocket quota (#3937)

* docs(devlog): plan the B-track continuation/quota stack delivery

Records the dependency-ordered branch chain for #3889 and #3934, the CI trigger contract that keeps the lower layer PR-free, and the original-author preservation requirement.

* fix(responses): recover expired forward continuation over WebSocket

A Codex WebSocket task stopped with a non-retryable 400 when its canonical
ChatGPT forward replay entry expired or was missing. The proxy correctly
refuses to forward a context-free delta, but reported invalid_request_error,
which Codex treats as terminal.

Return the existing previous_response_not_found classification instead, so
Codex reconnects and resends its full retained input, including completed tool
calls and their results. Rejection still happens before provider credential
resolution, adapter construction, and upstream I/O; the cache TTL and capacity
bounds are unchanged. HTTP clients must explicitly replay their full context.

(cherry picked from commit e8d82a1)

Co-authored-by: ykvv <229483879+y2ambition-ai@users.noreply.github.com>

* fix(codex): fence late WebSocket quota by credential generation

A streaming Codex request could keep receiving WebSocket quota metadata after
its pool credential was replaced under the same local account ID. A late frame
then repopulated the quota cleared for that replacement, making the new
credential look exhausted.

Capture the selected pool credential generation when the response's quota
observer is created and ignore later observations once that generation is no
longer live. Existing configuration-generation fencing and the main-pool
writer are unchanged: main-pool carries no credential generation and keeps its
own mainQuotaWriter identity check.

(cherry picked from commit e5c01f4)

Co-authored-by: luvs01 <luvs01@hanmail.net>

---------

Co-authored-by: t <a@b.com>
Co-authored-by: ykvv <229483879+y2ambition-ai@users.noreply.github.com>
Co-authored-by: luvs01 <luvs01@hanmail.net>
@lidge-jun

Copy link
Copy Markdown
Owner

Landed on dev in ca381ea764cfbc63bec978f53eb58e96c00c0c64 via the integration stack (#3937).

Your change is in dev byte-for-byte: src/server/responses/core.ts and tests/responses/responses-account-label.test.ts match this branch exactly. The squash commit carries Co-authored-by: luvs01 <luvs01@hanmail.net>, so the contribution is attributed to you.

Cross-platform CI passed on the stack tip a06bfa2f2: all four Linux shards, both macOS lanes, gates, packaging, keyring, and Docker smoke, with zero failures. The 900-second local runner timeout you reported did not recur in hosted CI.

Closing as delivered, not rejected. Thank you.

@lidge-jun lidge-jun closed this Sep 7, 2026
@lidge-jun lidge-jun added the landed-via-maintainer Original PR closed after landing via a maintainer merge train label Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working landed-via-maintainer Original PR closed after landing via a maintainer merge train review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants