Skip to content

fix(codex): fence entitlement credential refreshes behind lifecycle admission - #5320

Closed
luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:fix/entitlement-refresh-fence
Closed

luvs01 wants to merge 3 commits into
lidge-jun:devfrom
luvs01:fix/entitlement-refresh-fence

Conversation

@luvs01

@luvs01 luvs01 commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Background model-entitlement refreshes (retained sync, convergence, serve-options /models, CLI startup discovery, and ensureCodexEntitlementFreshness) resolved and touched the native main account's auth.json with no lifecycle admission, so a background path could read or write main's credential state outside the admission contract.
  • A refused admission claim could also take down Pool discovery instead of degrading that one refresh.
  • Adds model-entitlement-admission plus withNativeMainCredentialAdmission and applies the admission fence at all five call sites; the refresh write itself stays under the existing exclusive claim to avoid self-deadlock on credential commit ordering.

Test plan

  • bun test tests/codex-model-entitlement-admission.test.ts — 8/8 pass (new suite)
  • bun test across entitlement, admission, convergence, auth-api, and claim batches — 540 pass
  • bun x tsc --noEmit — clean
  • Pre-existing environment timeouts in codex-catalog-sync-hardening reproduce identically on the unmodified base commit.

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

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • 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.

luvs01 and others added 3 commits September 20, 2026 06:25
Address review findings on the admission fence: the lifecycle lease now covers only the credential read — the operation releases it via releaseMainLease as soon as getValidMainAccountToken settles (success and credential-error paths alike) and before any upstream roster fetch, so a profile drain never waits on network work while the turn is still counted. The Pool-only retry also runs after release. isNativeMainCredentialUnavailableError now falls back only for NATIVE_MAIN_CLAIM_BUSY / NATIVE_MAIN_CLAIM_UNAVAILABLE; other NativeProfileError codes propagate. Correct the model-entitlement-admission comment to match the actual locking guarantee.
@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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 added the bug Something isn't working label Sep 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions

github-actions Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (3/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 3/4).
  • The PR is more than 10 commits behind dev; the latest dev box has been unticked.
  • The checklist has been reset: re-test against the latest code and tick the boxes again.

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.

3/4 boxes ticked.

The PR is more than 10 commits behind dev; the latest dev box has been unticked.
The checklist has been reset: re-test against the latest code and tick the boxes again.
This PR stays in draft until every box above is ticked.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 60 / 80

백그라운드에서 모델 목록(자격)을 다시 받을 때, 본계정 auth.json을 읽고 고치는 길이 있었습니다. 카탈로그 동기화, 수렴, /models, CLI 시작 발견, ensureCodexEntitlementFreshness가 그 다섯 곳입니다. 이 길들은 요청 한 턴의 “들어가도 된다” 문을 거치지 않고 본계정 자격 증명에 손을 댔습니다. 문이 거절돼도 Pool만 보면 될 일을 통째로 쓰러뜨리기도 했습니다.

이번 PR은 그 문을 withNativeMainCredentialAdmission으로 모읍니다. 새 얇은 껍질 resolveAdmittedCodexModelEntitlements가 위 다섯 호출을 감쌉니다. 문이 열리면 본계정 토큰을 읽는 동안만 임대(lease)를 잡고, 읽기가 끝나는 즉시(성공이든 자격 실패든) 풀어서 네트워크로 목록을 받는 동안 프로필 비우기가 기다리지 않게 합니다. 문이 막히거나 본계정이 다른 주인 것이면 본계정을 빼고 Pool만 다시 돌립니다. 요청 경로의 resolveCodexModelEntitlements는 그대로 두고, 그쪽은 이미 턴 입장 문을 씁니다. 테스트 여러 개가 이 순서와 Pool만 보기 후퇴를 고정합니다. base는 dev입니다.

라인 - tests/codex-model-entitlement-admission.test.ts: 파일이 tests/ 루트에 있습니다. codex 도메인은 이미 tests/codex-integration/으로 옮겨진 상태라, layout 검사와 membership oracle이 이 파일을 straggler로 잡습니다. CI의 test 1/4·4/4와 macos 실패가 여기로 보입니다. 같은 주제의 codex-model-entitlements.test.ts는 이미 tests/codex-integration/에 있습니다. 새 파일도 그 폴더로 옮기고, membership fixture에 경로를 맞춰야 초록이 납니다.

라인 - PR 설명 / enforce-target: 게이트가 “bad description (thin)”으로 막습니다. 본문에 Summary·Verification 내용은 있는데, ## Summary 바로 뒤에 문장이 붙어 있고 Test plan 머리글이 Verification으로만 되어 있어 게이트기가 얇다고 본 듯합니다. 머리글 아래 빈 줄을 두고 Summary / Test plan 형식으로 나누면 됩니다.

라인 - src/codex/model-entitlements.ts resolveCodexModelEntitlements: 울타리 없는 함수가 그대로 export됩니다. 요청 경로·테스트 seam용으로는 맞지만, 나중에 백그라운드 호출이 다시 여기로 붙으면 같은 구멍이 납니다. 주석이나 lint 경계로 “백그라운드는 admitted만”을 남기면 재발을 줄입니다.

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

요청 경로의 턴 입장(resolveCodexRetryModelEntitlements 등)과 백그라운드의 withNativeMainCredentialAdmission을 두 문으로 두는 지금 분리가 맞는지. 한쪽만 admitted로 합치면 턴 예산·drain 신호가 섞일 수 있어서, 지금처럼 나누는 편이 맞아 보입니다. 울타리 없는 resolveCodexModelEntitlements를 비공개로 내릴지, export를 유지한 채 호출 규약만 지킬지도 선택입니다.

너의 추천

방향은 맞고, 본계정 자격 증명을 백그라운드가 문 없이 만지던 구멍을 제대로 막습니다. 머지 전에 테스트를 tests/codex-integration/으로 옮기고 membership fixture를 맞춘 뒤, PR 설명을 Summary/Test plan 형식으로 고치면 CI·enforce-target이 풀릴 가능성이 큽니다. 요청 경로까지 admitted로 합치지 마세요. draft 체크리스트는 이미 채워져 있으니, 위 두 가지만 되면 ready로 올리면 됩니다.

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

@luvs01

luvs01 commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Already landed upstream via #5268 (carries the admission fence and the lease-release follow-up, including the native-main credential release before roster fetches). Closing as superseded.

@luvs01 luvs01 closed this Sep 21, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants