Revert "fix(ci): limit Turbo concurrency to prevent browser test CPU starvation"#2116
Conversation
…starvati…" This reverts commit 23e09a2.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
ℹ️ Note: This PR has already been merged. The feedback below is provided for awareness and future reference.
🟠⚠️ Major (1) 🟠⚠️
🟠 1) .github/workflows/ci.yml:114 Revert reintroduces CPU starvation risk for Playwright tests
Issue: This revert removes the --concurrency=8 flag that was added in PR #2095 to prevent Playwright browser test timeouts. The original fix addressed CPU starvation on the ubuntu-16gb runner (4 vCPUs) when Turbo's default concurrency of 15 runs during cache-miss scenarios.
Why: Without the concurrency limit, 15 concurrent tasks can starve Playwright tests (Monaco editor initialization) of CPU time, causing timeouts on cache-miss CI runs. The original PR provided well-documented reasoning (2x vCPUs = 8 as standard oversubscription for mixed I/O + CPU workloads). Reverting without explanation or alternative mitigation creates a risk of CI flakiness returning.
Fix: If Playwright test timeouts recur, consider:
- Re-applying a concurrency limit (possibly with a different value if 8 caused issues)
- Upgrading to a larger runner
- Documenting why the limit is no longer needed if the underlying issue was resolved differently
Refs:
- Original PR #2095 — detailed rationale for the concurrency limit
- ci.yml:114 — reverted change location
- turbo.json — default concurrency of 15
🟡 Minor (1) 🟡
🟡 1) PR description Missing justification for reverting CI stability fix
Issue: The PR description ("Reverts #2095") does not explain why the original fix needed to be reverted.
Why: The original PR was merged with detailed reasoning about the CPU starvation issue. A quick revert without explanation makes it difficult for future maintainers to understand whether the original issue was resolved differently, the fix caused a regression, or other factors.
Fix: For future reference, consider adding a comment explaining the rationale for this revert.
💡 APPROVE WITH SUGGESTIONS
Summary: This revert removes a CI stability fix without documented justification. While the change itself is straightforward, it may reintroduce Playwright test timeouts on cache-miss runs. Since the PR is already merged, monitor CI for flakiness and consider re-applying concurrency limits if timeouts recur. The feedback above is provided for awareness and future reference.
Discarded (0)
No findings were discarded.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
2 | 2 | 0 | 0 | 0 | 0 | 0 |
| Total | 2 | 2 | 0 | 0 | 0 | 0 | 0 |
The ubuntu-16gb runner (4 vCPUs, 16GB RAM) gets OOM-killed when turbo concurrency=15 triggers multiple Next.js builds concurrently during cache-miss runs (e.g., any PR touching agents-core). This limits CI concurrency to 8 via CLI flag (turbo.json stays at 15 for local dev). Re-applies the fix from #2095 which was reverted in #2116. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Reverts #2095