ci(windows): six shards instead of four, ceiling unchanged (Windows stack 6/6) - #3572
Conversation
|
Warning Review limit reachedNext included review available in 14 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
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. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
chore
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Security-boundary review of exact head 97e3e84f0ebe078bb0ae5cf6f40206c0a627ae4d: the workflow diff changes only platform-windows.timeout-minutes from 25 to 40 plus comments. It does not change permissions, event scope, runner selection, checkout credentials, action refs, commands, or secret exposure, so I see no workflow-security expansion in this layer.
I am not approving it yet. The base PR #3558 remains changes-requested for an under-sized 15-second internal deadline and a vacuous two-child rendezvous, this exact head currently has a failing test 4/4, and the stated acceptance of two consecutive all-green Windows dispatches has not been demonstrated. Also correct the rationale: a 40-minute job ceiling cannot cover a full 25-minute first attempt plus a full retry (50 minutes). Either describe 40 as normal-run margin plus only an early-crash retry, or choose a ceiling that actually supports the full retry claim. Re-review after the parent is corrected and the documented Windows acceptance runs are green.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97e3e84f0e
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # the paragraph above records for 15. Measured across five runs of one branch, | ||
| # completed Windows shards take 17-25 minutes; 40 clears that with margin for the | ||
| # 2× run-to-run variance seen on every other Windows bound and for one retry. | ||
| timeout-minutes: 40 |
There was a problem hiding this comment.
The required workflow test still asserts platform-windows.timeout-minutes is exactly 25 at tests/ci-workflows/ci-workflows.test.ts:121. With this workflow now parsing as 40, that assertion fails and blocks the CI/prepush suite; update the assertion and its stale explanatory comment alongside this value.
AGENTS.md reference: .github/AGENTS.md:L21-L25
Useful? React with 👍 / 👎.
| # the paragraph above records for 15. Measured across five runs of one branch, | ||
| # completed Windows shards take 17-25 minutes; 40 clears that with margin for the | ||
| # 2× run-to-run variance seen on every other Windows bound and for one retry. | ||
| timeout-minutes: 40 |
There was a problem hiding this comment.
When Bun crashes late in a slow shard, this ceiling still cannot provide the retry coverage claimed by the surrounding comment: completed jobs already take up to 25 minutes, so a crash near the end plus another comparable attempt can require nearly 50 minutes even before considering variation. At 40 minutes the second attempt will again be cancelled mid-suite; derive the timeout from setup time plus two worst-case test attempts, or otherwise stop claiming the bounded retry can complete.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 71 / 80이 PR은 Windows suite stabilization 열차의 마지막 칸(6/6) 입니다. 지금 바꾸는 내용은 사실상 한 정수입니다. 현재
경로 주석 블록 (구 15→25 기록 옆) - 같은 실패 모드가 다시 났다는 증거가 run 33934756997로 남아 있어 다음 사람이 숫자를 함부로 내리지 않게 돕습니다. 경로 경로 스택 base #3558 - 이 PR 단독으로 경로 security review - workflow 파일이라 형식상 리뷰가 필요합니다. 내용이 timeout 한 줄이면 리뷰 비용은 작습니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
97e3e84 to
293f3e6
Compare
lidge-jun
changed the title
c477a02 to
3201181
Compare
Run 33934756997 cancelled a GREEN windows 3/4 at 25m12s, mid-file with every prior case passing - the same truncation the ceiling's comment records for its 15-minute predecessor. Across five runs of this branch, completed Windows shards took 17-25 minutes; 3/4 never finished under 21. The bound itself is right (a wedged shard must still die); the work per shard grew into it. Cut the work instead of raising the bound: six shards put each leg at roughly two-thirds of the four-shard wall time, back inside the margin 25 was chosen to give. The matrix, the --shard divisor, and the job name move together. ci-workflows.test.ts pinned the Windows matrix to Linux's; that invariant was about tiling the suite exactly, not about the two platforms sharing a count. It now pins Windows to its own divisor and to a contiguous 1..N matrix, and the Linux divisor check reads the TEST_SHARD env line it was always meant to observe (it had been matching the Windows --shard literal by coincidence).
293f3e6 to
ec9c1f4
Compare
2 participants
Summary
Windows suite stabilization, PR 6 of 6 (stacked on #3558). The
platform-windowsleg goes from four shards to six; its 25-minute ceiling is unchanged.Run 33934756997 cancelled a green windows 3/4 at 25m12s — mid-file in
tests/vision/vision-routed.test.ts, every prior case passing. That is the truncation the ceiling's own comment records for its 15-minute predecessor. Across five dispatches of this branch, completed Windows shards took 17-25 minutes and 3/4 never finished under 21. The bound itself is right — a wedged shard must still die — so the work per shard is cut instead of the bound raised: six shards put each leg at roughly two-thirds of the four-shard wall time, back inside the margin 25 was chosen to give.The matrix, the
--sharddivisor, and the job name move together.ci-workflows.test.tshad pinned the Windows matrix to Linux's; the invariant it protects is that the matrix and divisor tile the suite exactly, not that both platforms share a count. It now pins Windows to its own divisor and to a contiguous 1..N matrix, and the Linux divisor assertion reads theTEST_SHARDenv line it was always meant to observe (it had matched the Windows--shardliteral by coincidence).Workflow change: per
MAINTAINERS.mdthis needs security review. No permissions, actions, secrets, or runner selection are touched — a matrix list, a divisor, and a job name.Verification
bun test tests/ci-workflows/ci-workflows.test.ts: 135 passbun run typecheckcleandevlog/_plan/260905_windows_suite_stabilization/090_shard_ceiling.md— per-shard wall-time table for runs 33926041666 through 33934756997Checklist