Skip to content

fix(codex): bind Desktop reconnects to one pool account - #2296

Open
Ingwannu wants to merge 2 commits into
devfrom
ingw/fix-app-pool-affinity-2046
Open

fix(codex): bind Desktop reconnects to one pool account#2296
Ingwannu wants to merge 2 commits into
devfrom
ingw/fix-app-pool-affinity-2046

Conversation

@Ingwannu

@Ingwannu Ingwannu commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Summary

  • preserve x-codex-parent-thread-id as the authoritative Pool affinity key
  • when Desktop omits it, derive an opaque process-local HMAC from the complete bounded session-id + thread-id pair
  • carry the same affinity key through selection, failover, and terminal outcome accounting
  • keep exact account-qualified selectors outside automatic Pool affinity
  • document the privacy and lifecycle tradeoff

Scope

This addresses the proven reconnect-rotation defect from #2046. It does not claim to fix the first Desktop-UI workspace denial, does not strip turn metadata, and does not reset threads. That separate same-version App-only boundary remains open for evidence-led diagnosis.

Privacy

Raw Desktop session/thread identifiers are not stored or logged. The fallback key is HMAC-derived under a fresh random process-local key and therefore cannot be correlated across OCX restarts.

Validation

  • bun test tests/codex-auth-context.test.ts — 57 pass
  • bun run typecheck
  • bun run privacy:scan
  • git diff --check

An earlier full-suite run on this implementation reached 14,066 pass / 15 skip, with failures reproduced on clean origin/dev from the host service-token fixture and the installed TypeScript rejecting --ignoreConfig. Exact-head CI is green. The PR is ready for independent auth/security review and remains unmerged until that approval.

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

  • New Features

    • Added consistent pool account affinity using valid parent-thread information or bounded session and thread details.
    • Preserved account affinity across subagent fallback previews, final routing, and native dispatch.
    • Added safeguards for incomplete or oversized identifiers and exact account selections.
  • Bug Fixes

    • Improved outcome attribution by consistently associating activity with the selected pool account.
    • Prevented late transient failures from replacing newer account bindings.
    • Avoided exposing raw session or thread identifiers in pool affinity handling.

@coderabbitai

coderabbitai Bot commented Aug 21, 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: Pro Plus

Run ID: 9c16ceda-ae59-4529-b733-52e180de1640

📥 Commits

Reviewing files that changed from the base of the PR and between 401c24f and 0e5a434.

📒 Files selected for processing (7)
  • src/codex/auth-context.ts
  • src/providers/openai-sidecar.ts
  • src/server/responses/compact.ts
  • src/server/responses/core.ts
  • structure/08_openai-provider-tiers.md
  • tests/codex-auth-context.test.ts
  • tests/subagent-fallback-handle-responses.test.ts

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


📝 Walkthrough

Walkthrough

Codex pool routing now derives bounded process-local affinity keys from valid parent-thread or Desktop session/thread headers. Resolved contexts propagate the key through preview, dispatch, failover, and outcome recording. Tests cover fallback, precedence, exact account selection, and stale failures.

Changes

Codex pool affinity

Layer / File(s) Summary
Affinity derivation and routing
src/codex/auth-context.ts, structure/08_openai-provider-tiers.md
codexPoolAffinityKey preserves valid parent-thread identifiers or derives opaque HMAC keys from bounded session/thread pairs. Pool and main-pool contexts carry affinityKey, and account routing uses it for non-fixed selections.
Outcome attribution and fallback propagation
src/server/responses/core.ts, src/server/responses/compact.ts, src/providers/openai-sidecar.ts
Pool, terminal, compact, retry, transport, and subagent fallback paths use the resolved context’s affinityKey. Recorder APIs no longer accept separate thread ID arguments.
Routing and failover validation
tests/codex-auth-context.test.ts, tests/subagent-fallback-handle-responses.test.ts
Tests cover affinity precedence, bounded and incomplete headers, exact account selection, stale failure ordering, preview selection, and native fallback credentials.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to 0e5a4

The change binds Desktop reconnects to a stable, process-local affinity key while preserving existing selectors and outcome accounting. No actionable merge-blocking risk remains beyond normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant Desktop
  participant resolveCodexAuthContext
  participant AccountPool
  participant ResponseOutcomeRecorder
  Desktop->>resolveCodexAuthContext: session and thread headers
  resolveCodexAuthContext->>AccountPool: resolve account with affinityKey
  AccountPool-->>resolveCodexAuthContext: selected pool account
  resolveCodexAuthContext-->>Desktop: auth context with affinityKey
  Desktop->>ResponseOutcomeRecorder: record outcome with affinityKey
Loading

Suggested reviewers: lidge-j

🚥 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 9 functions across 6 files. (1 skipped: 1 unsupported.) Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: binding Desktop reconnects to a single Codex pool account.
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.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ingw/fix-app-pool-affinity-2046

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

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 63 / 80

지금 dev HEAD c0cbe494e에서 풀 어피니티가 x-codex-parent-thread-id 하나임. src/codex/auth-context.ts:372headers.get("x-codex-parent-thread-id")를 그대로 resolveCodexAccountForThreadDetailed에 넣음. Compact는 src/server/responses/compact.ts:464. 아웃컴은 src/server/responses/core.ts:949·:3008·:3391·:3432. Desktop이 그 헤더를 빼면 매 재연결이 unbound. activeCodexAccountId로 다른 계정. #2046 리포터가 본 재연결 403 5연타가 그거임. 이 PR이 그 재연결만 잠금. 첫 Desktop-UI 워크스페이스 403이랑 스레드 메타 삭제는 범위 밖이라고 본문에 적혀 있음. 드래프트. 닫을 중복 아님. #2046은 열린 채.

핵심이 src/codex/auth-context.ts codexPoolAffinityKey. 부모 스레드가 있으면 그 값이 권위. 없으면 session-id+thread-id 둘 다 있고 retainedUtf8Bytes가 각 512 이하일 때만 app: + HMAC-SHA256. 키 재료는 모듈 로드 때 randomBytes(32). 프로세스 로컬. OCX 재시작이면 풀 맵이랑 같이 리셋. 원본 식별자/내구 해시는 Pool 상태에 안 들어감. fixedAccountaffinityKey를 안 만듦. exact 셀렉터는 자동 어피니티 밖. 셀렉션·failover·터미널 아웃컴이 같은 키를 운반함. 지금 src/providers/openai-sidecar.ts:167-174threadId를 안 넣음. 이 PR이 authContext.affinityKey를 넣음. Compact/core recorder가 헤더를 다시 읽지 않음. tests/codex-auth-context.test.ts 5장: 재연결이 같은 app: 키로 pool-a에 고정, 부모 스레드가 Desktop 폴백보다 앞섬, incomplete/oversized는 unbound, exact 셀렉터는 키를 안 만듦, late 500이 새 바인딩을 못 지움. 맞음.

구멍. src/server/responses/core.ts:2296 previewCodexAccountForRequest(threadId)가 여전히 부모 스레드만 봄. Desktop이 그 헤더를 빼면 서브에이전트 프리뷰가 실제 resolveCodexAuthContext 셀렉션이랑 다른 계정임. HMAC 키를 프리뷰에 안 넣음. 부모 스레드는 trim/바이트 한도가 없음. 폴백만 trim+512. 공백 부모 스레드가 권위 키가 됨. Desktop이 어떤 턴은 부모를 넣고 어떤 턴은 빼면 키가 둘로 갈라짐. 재연결 회전이 다시 열림. HMAC 테스트는 auth-context만. compact/core/sidecar 배선은 bun run typecheck. FORWARD_HEADERS (src/adapters/openai-responses.ts:33-35)에 session-id/thread-id가 이미 있음. 업스트림으로 원본 식별자가 감. 이 PR이 만든 유출이 아님. Pool 상태에 안 남기는 경계랑은 별개임. 헤더 포워딩을 이 PR에서 건드리지 말 것.

types.ts/config.ts 안 만짐. 스플릿 안 씹힘. 리베이스하지 말고 닫으라는 케이스 아님. 첫 403·턴 메타 삭제는 이 PR에 넣지 말 것. #2046에 남겨. #2188 L1–L9 사이드카 이미 dev. x_search 넣지 말 것. Grok OAuth Chat 기본(#2255)/GUI 옵트인 Responses(#2266)/#2283이랑 다른 레인임. 프리뷰 배포 아님. #2292/#2293 윈도우 피커/풀 재시작이랑 안 겹침. 카탈로그는 그대로 Ox Alpha x-preview-f-free + deepseek-v4-flash-vision-exp. v2.29.0 태그됨. v2.30.0-preview.20260821 있음. Desktop 풀 재연결이 유저 보이는 핫패스라 63. 시큐리티 리뷰가 드래프트 이유. HMAC이 프로세스 로컬이라 사전 공격은 재시작 수명임.

해결방안: 재연결 구멍은 이 패치로 가라. 머지 전에 previewCodexAccountForRequestcodexPoolAffinityKey와 같은 키를 쓰게 해라. 부모 스레드도 trim하고 512바이트 한도. 헤더가 턴마다 갈리면 로그에 키 종류만 (raw vs app:) 찍고 식별자는 마킹해라. exact K12 셀렉터 첫 403은 이 PR에 넣지 말 것. #2046 열린 채. bun test tests/codex-auth-context.test.ts에 서브에이전트 프리뷰가 Desktop 폴백 키를 따라가는 장 하나. 스플릿이 auth-context.ts를 옮기면 리베이스하지 말고 닫고 다시 짜라. 지금은 그 정도 아님.

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

@Ingwannu
Ingwannu force-pushed the ingw/fix-app-pool-affinity-2046 branch from 574cadc to 0e5a434 Compare August 21, 2026 17:10
@Ingwannu

Copy link
Copy Markdown
Owner Author

Addressed the verified Grok/owner review points on exact head 0e5a434594bfcfae679251757ce001ae54537624.

  • The subagent fallback preview now receives the same codexPoolAffinityKey() used by final auth selection and terminal outcome accounting, so Desktop planning cannot preview the active account while the real request reuses a different bound account.
  • x-codex-parent-thread-id, session-id, and thread-id now share trimming and the 512-byte component bound. An unusable parent value falls back to the complete bounded Desktop pair rather than entering Pool state directly.
  • Added a handler-level regression that binds the Desktop pair to pool A, changes the active cursor to pool B, and proves both subagent preview and final native authentication still use pool A. Added parent trimming/oversize regressions and updated the architecture contract.

Verification on this head: 281 passed, 2 platform-specific skips across the auth-context, subagent fallback, routing, and pool-rotation suites; typecheck, privacy scan, and diff checks passed. Exact-head CI is running.

I did not add a new durable request-log field for the parent/app key kind in this patch. That would expand the persisted telemetry surface, while the current #2046 evidence has not shown mixed header presence within one task. If that variation is observed, it should be added through the existing opt-in privacy-bounded affinity diagnostic rather than ordinary logs. The first Desktop-only exact-K12 workspace denial remains intentionally out of scope and #2046 stays open.

@Ingwannu

Copy link
Copy Markdown
Owner Author

Exact-head CI is now complete on 0e5a434594bfcfae679251757ce001ae54537624: all 23 reported check runs completed with no failures or pending jobs.

The PR remains Draft because I materially authored the fix and cannot provide the required independent approval. @lidge-jun @Wibias, please review the auth/account-affinity boundary and the verified Grok follow-up before this is considered for merge. The first Desktop-only exact-K12 denial remains out of scope, so #2046 stays open.

@Ingwannu
Ingwannu marked this pull request as ready for review August 21, 2026 19:02
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.

2 participants