Skip to content

test: contain server admission CI stalls and synchronize quota switching - #6279

Merged
lidge-jun merged 1 commit into
devfrom
codex/linux-ci-flakes
Sep 30, 2026
Merged

lidge-jun merged 1 commit into
devfrom
codex/linux-ci-flakes

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 30, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Run the complete active-registry-admission.test.ts file in the existing fresh-process lane on all platforms, and await its upstream listener shutdown. In Linux run 36610213506, batch 11 stopped after this file's WebSocket admission case and timed out at 120 seconds; all 11 cases passed in its standalone attribution process. The serial manifest is consumed by both the local suite and hosted batch runner. A regression pins the dedicated process and the main-lane exclusion; no assertion, timeout, or suite membership is removed.
  • Wait for the first account's usage request to enter the response gate before switching accounts in the quota seeding regression, and release that gate in finally. Run 36614996669 failed with usageCalls equal to zero because the old branch's post-token selection guard could cancel dispatch when switching overtook token acquisition. Current dev completed 40 independent focused runs and a controlled delayed-token probe; loading the old branch probe under the same delay failed. Current cooldown recovery already permits the originally probed account to retain its measurement while suppressing a stale provider report.

The Linux timeout is containment of a native multi-file process-state stall, not a claim that a particular JavaScript timer leak or Bun-internal deadlock has been proven. The first missing case creates a second listener; the fixture's upstream shutdown was not awaited. MacOS completed the original batch and both adjacent-file pairings without the stall. Increasing deadlines or accepting attribution as a passing run would not address it.

Verification

  • bun test --isolate --timeout 60000 tests/providers/provider-account-quota.test.ts tests/codex-integration/active-registry-admission.test.ts tests/ci-workflows/test-runner.test.ts tests/ci-workflows/ci-shard-balance.test.ts — 142 pass, 3 platform skips, 0 fail.
  • Dedicated-lane regression failed before adding the manifest entry, then passed in the focused suite.
  • Controlled upstream stop-completion observer: bun test --isolate --preload ./.tmp/linux-ci-flakes/upstream-stop-order.ts -t "non-SSE streamed" tests/codex-integration/active-registry-admission.test.ts — reverting only the awaited upstream close failed because the fixture restored OPENCODEX_HOME before the native close promise settled; the committed awaited close passed. The observer and red/green logs are scratch evidence, not committed fixtures.
  • bun run typecheck, bun scripts/file-size-ratchet.ts, bun run structure:check, bun run privacy:scan, and git diff --check — passed.
  • The original 12-file batch completed locally (492 pass) but failed 14 deletion-fixture hooks because the managed checkout is under the protected real Codex home; the standalone admission file passed. The full local suite was not run because it includes that same protected-path cleanup limitation and would consume resources without supplying the requested Linux evidence. Full Linux suite coverage and required checks are delegated to the exact-head Cross-platform CI run before integration.
  • Security review of the test-runner change: execution membership is preserved, the admission fixture gains a bounded standalone process, and existing first-failure behavior remains in force. No credential handling, action permissions, dependency, runtime auth behavior, or release gate changes.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. Maintainer-only harness changes and investigation are recorded here; product behavior is unchanged.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 30, 2026 01:59
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-30T02:01:16.565926Z cb3ff18 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Sep 30, 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: 1b4dfaa9-8682-4eae-8d03-4ebc52d750e3

📥 Commits

Reviewing files that changed from the base of the PR and between d4480b7 and cb3ff18.

📒 Files selected for processing (4)
  • scripts/test.ts
  • tests/ci-workflows/test-runner.test.ts
  • tests/codex-integration/active-registry-admission.test.ts
  • tests/providers/provider-account-quota.test.ts

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


📝 Walkthrough

Walkthrough

The full-suite runner now assigns the active-registry admission test to an isolated lane. Test changes also await upstream shutdown and synchronize an account-switch test with its pending usage request.

Changes

Test execution stability

Layer / File(s) Summary
Isolated integration-test lane
scripts/test.ts, tests/ci-workflows/test-runner.test.ts
The full-suite plan assigns active-registry-admission.test.ts to its own fresh-process lane. The runner test checks that it is excluded from the parallel lane and runs with --isolate --parallel=1.
Test synchronization and cleanup
tests/codex-integration/active-registry-admission.test.ts, tests/providers/provider-account-quota.test.ts
The streamed-response test awaits upstream.stop(true) in cleanup (line 146). The account-switch test signals when the first usage request starts, waits for that signal before switching accounts, and releases the pending request in finally (lines 345–351, 371–374).

Priority: ⬇️ Low

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

Change: Other

Merge Risk: ⚪ Minimal · up to cb3ff

The admission test is isolated in eligible full-suite plans, and the affected tests coordinate cleanup and request timing. No concrete merge-blocking code risk is evident; complete the planned exact-head CI check before integration.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4…
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 accurately summarizes both primary changes: isolating the server admission test to contain CI stalls and synchronizing account switching in the quota test.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • 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.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 60 / 80

이 PR은 제품 동작을 바꾸는 작업이 아니다. Linux CI에서 테스트가 중간에 멈춰 120초 타임아웃이 나던 문제를, 테스트 실행 방식과 정리(cleanup) 타이밍을 고쳐 막으려는 작업이다. 베이스는 dev이고, 바꾼 파일은 네 개(+20/−3)로 범위가 작다.

첫 번째 줄기는 active-registry-admission.test.ts다. 여러 파일이 한 프로세스에서 같이 돌 때 WebSocket admission 케이스 뒤에서 Linux가 멈췄고, 그 파일만 따로 돌리면 11개 케이스가 모두 통과했다. 그래서 이 파일을 SERIAL_FULL_SUITE_FILES에 넣어 전 플랫폼에서 전용 fresh process로 돌리게 했다. 메인 레인에서는 이 파일을 ignore하고, 전용 레인은 --isolate --parallel=1로 실행한다. 같은 파일의 finally에서는 upstream.stop(true)를 기다리도록 바꿔, 업스트림 리스너가 안 닫힌 채 다음으로 넘어가지 않게 했다. test-runner.test.ts에 “전용 프로세스 + 메인 레인 제외”를 고정하는 회귀 테스트도 추가했다.

두 번째 줄기는 provider-account-quota.test.ts의 계정 전환 레이스다. usage 요청이 게이트에 들어가기 전에 active 계정을 바꾸면, 예전 가드가 dispatch를 취소해 usageCalls가 0이 될 수 있었다. 지금은 첫 계정 usage가 게이트에 들어온 뒤에만 전환하고, finally에서 게이트를 풀어 응답이 영원히 안 오는 상태를 막는다.

작성자 검증으로는 관련 포커스 스위트 142통과, typecheck·structure·privacy·file-size ratchet 통과다. 전체 Linux suite는 exact-head Cross-platform CI에 맡긴다고 명시했다. 지금 체크는 아직 pending이 많다.

라인 scripts/test.ts SERIAL_FULL_SUITE_FILES - 또 quarantine다. Linux multi-file 스톨의 진짜 원인(타이머 누수·Bun 내부 등)은 아직 증명되지 않았고, 파일 단위 격리로 가둔 것이다. 목록이 계속 늘면 full-suite wall time과 유지 비용이 쌓인다.
라인 tests/providers/provider-account-quota.test.ts usageEntered - token-first 경로가 한 번도 안 타면 await usageEntered에서 타임아웃까지 멈춘다. 예전엔 usageCalls === 0으로 빨리 깨질 수 있었다. 실패 모드가 “빠른 assert 실패”에서 “대기 후 타임아웃”으로 바뀐다.
경로 묶음 - admission 격리/정리와 quota 전환 동기화는 원인·증상이 다른 두 flake다. 한 PR에 넣으면 한쪽만 되돌리기 어렵다.
경로 CI - exact-head Cross-platform 결과가 아직 안 나왔다. 작성자도 Linux 증거는 그 런에 맡긴다고 했다.

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

  • 두 flake 수정을 한 PR로 유지할지, admission 격리와 quota 동기화를 나눌지
  • SERIAL quarantine을 이번에도 받아들일지, 아니면 원인 추적 이슈를 따로 열고 merge할지
  • exact-head Linux/macOS suite가 녹색이 될 때까지 merge를 기다릴지

너의 추천

  • exact-head CI(특히 Linux suite)가 녹색이면 merge해도 된다. 베이스 dev도 맞고, 회귀 핀과 await upstream.stop·usage 게이트 정리는 방향이 맞다. 가능하면 quota 쪽은 실패 시 usageCalls가 0이면 바로 깨지게 타임아웃/가드를 한 줄 보강하면 디버깅이 쉬워진다. SERIAL 목록 증가는 이번엔 수용하되, “admission multi-file stall 원인” 트래킹 이슈 하나는 남겨 두는 편이 좋다.

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

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Sep 30, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

Owner-authorized maintainer integration into dev under MAINTAINERS.md, by @lidge-jun.

  • PR head: cb3ff18142b5b1ea1c0400563ff1934b5e266298.
  • Exact-head Cross-platform CI run 36657682443, attempt 1, event pull_request: completed success. All 13 requested jobs succeeded, including Linux test 1/4–4/4 and the aggregate ci. Optional platform/package jobs were skipped by the existing test-only path rules; these skips are not presented as executed tests.
  • CI checked out PR merge ref c991ea1d0b8a76444237dda19729c62c3f1c1651; its tree matches the PR head (git diff --exit-code HEAD FETCH_HEAD passed). dev has not moved from the branch base.
  • Linux test 2/4: 50 primary batches completed without timeout/attribution. Batch 11 contains only active-registry-admission.test.ts; all 11 cases passed, including the streaming case after the formerly last logged WebSocket case.
  • Linux test 3/4: all 44 primary batches completed; the quota active-switch regression passed.
  • Local focused verification: 142 pass, 3 platform skips, 0 fail; typecheck, file-size ratchet, structure and privacy checks passed. The dedicated-lane regression and upstream-close ordering probe both have red/green evidence. Security review found no credential, permission, dependency or release-gate changes; test membership and first-failure behavior remain intact. No maintainer objections or inline findings are outstanding.

Review gate command: bash scripts/ci/assert-mergeable-review.sh --maintainer-integration 6279 lidge-jun/opencodex (exit 0):

OK: validation snapshot for #6279 into dev at head cb3ff18142b5b1ea1c0400563ff1934b5e266298 by lidge-jun; CI and security review remain separate
Snapshot only: revalidate the current actor and dev base before a separately authorized merge; head matching does not pin the base.

This integrates the verified listener cleanup correction and fresh-process containment. The exact Bun-internal reason for the original multi-file stall remains unconfirmed, as recorded in the description. The old branch's quota dispatch-cancellation race is documented and the regression now waits for request entry before switching.

@lidge-jun
lidge-jun merged commit 1b97c6f into dev Sep 30, 2026
35 of 38 checks passed
@lidge-jun
lidge-jun deleted the codex/linux-ci-flakes branch September 30, 2026 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant