-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(ci): keep contended ECS E2E shards above the flat 60-minute ceiling (#10591) #10597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
64a703c
f2b2a2f
77eb9c1
6e0848e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,8 +46,12 @@ jobs: | |
| # Kill-switch: MAINTAINER_ECS_RUNNER_DISABLED. | ||
| runs-on: '${{ (github.repository == ''QwenLM/qwen-code'' && vars.MAINTAINER_ECS_RUNNER_DISABLED != ''true'') && fromJSON(''["self-hosted", "linux", "x64", "ecs-qwen"]'') || fromJSON(''["ubuntu-latest"]'') }}' | ||
| # A wedged shard must free the pool runner well before GitHub's | ||
| # 360-minute default; 60 matches ci.yml's pool jobs. | ||
| timeout-minutes: 60 | ||
| # 360-minute default. The ceiling is extended on the ECS pool only, | ||
| # mirroring ci.yml's pool lanes: run 33345905817 (issue #10591) expired | ||
| # at the old flat 60 on a contended shared host before its log reported | ||
| # any test result. Hosted fallbacks keep the pre-contention ceiling so | ||
| # a genuine hang there does not burn the extra 30 minutes. | ||
| timeout-minutes: "${{ fromJSON((github.repository == 'QwenLM/qwen-code' && vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true') && '90' || '60') }}" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R1-1: [fails-closed] [regression] This change turns Re-checked at Witness: A/B on the contested assertion: BASE (pre-change e2e.yml) passes with value=60; the PR arm fails with the expression string. Update The updated pin must keep the two ceilings mirrored from 中文说明本改动把 已在本轮审阅的提交 见证(在 HEAD f2b2a2f 上执行): 请在同一个 PR 中更新 更新后的钉子必须保持与 — qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified real at the current head: This round nevertheless does NOT implement the pin update:
Leaving this thread open until a maintainer chooses. 中文说明已在当前 head 上确认为真实问题: 但本轮不实施该断言更新:
在维护者作出选择之前,本线程保持打开。
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] R1-1: [fails-closed] [regression] This change turns Re-checked at Witness: A/B on the contested assertion: BASE (pre-change e2e.yml, Update The replacement must respect the strict-assert import at 中文说明本改动把 已在 见证(在 HEAD 77eb9c1 上执行): 请在同一个 PR 中更新 替换时必须尊重 — qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified real at the current head: This round nevertheless does NOT implement the pin update:
Leaving this thread open until a maintainer chooses. 中文说明已在当前 head 上确认为真实问题: 但本轮不实施该断言更新:
在维护者作出选择之前,本线程保持打开。 |
||
| # Skip on fork PRs: forks have no access to repository secrets | ||
| # (OPENAI_*, DOCKERHUB_*), so the matrix would fail unconditionally | ||
| # and show misleading red status. Same-repo PRs run normally. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -107,4 +107,56 @@ describe('e2e workflow', () => { | |
| expect(runStep.run).toContain('mktemp -d /var/tmp/qwen-ci-XXXXXX'); | ||
| expect(runStep.run).toContain('trap \'rm -rf "$TMPDIR"'); | ||
| }); | ||
|
|
||
| describe('job timeout lane contract', () => { | ||
| // Run 33345905817 (issue #10591) expired at the old flat 60-minute | ||
| // ceiling on a contended shared ECS host before its log reported any | ||
| // test result. The extended ceiling is | ||
| // scoped to the pool routing — the same condition `runs-on` uses: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-2: The new lane-contract test evaluates only the Witness: In the same describe block, evaluate the 中文说明新增的通道契约测试只对 见证:变异探针——只对 请在同一个 describe 块中,用相同的替换扫描对 — qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-2: The new lane-contract test evaluates only the Re-checked at Witness: In the same describe block, evaluate the 中文说明新增的通道契约测试只对 已在本轮审阅的提交 见证:变异探针——只对 请在同一个 describe 块中,用相同的替换扫描对 — qwen3.8-max via Qwen Code /review (v0.22.3) |
||
| // hosted fallbacks (forks and MAINTAINER_ECS_RUNNER_DISABLED=true) | ||
| // keep the pre-contention bound, so a genuine hang there does not | ||
| // burn the extra 30 minutes. Evaluate the real timeout expression for | ||
| // both routings instead of pinning a bare number — the same | ||
| // substitute-then-evaluate technique ci-platform-lanes.test.js uses — | ||
| // so a regression to an unconditional ceiling fails here instead of | ||
| // reading as a passing constant. | ||
| const timeoutMinutesOn = ({ repository, ecsDisabled }) => { | ||
| const expr = String(yml.jobs['e2e-test-linux']['timeout-minutes']) | ||
| .replace(/^\$\{\{\s*/, '') | ||
| .replace(/\s*\}\}$/, '') | ||
| .replace(/github\.repository/g, JSON.stringify(repository)) | ||
| .replace( | ||
| /vars\.MAINTAINER_ECS_RUNNER_DISABLED/g, | ||
| JSON.stringify(ecsDisabled), | ||
| ) | ||
| .replace(/fromJSON\(/g, '('); | ||
| if (/github\.|vars\.|needs\.|steps\.|fromJSON\(/.test(expr)) { | ||
| throw new Error( | ||
| `e2e-test-linux timeout carries a term this guard does not model: ${expr}`, | ||
| ); | ||
| } | ||
| return Number(new Function(`return (${expr});`)()); | ||
| }; | ||
|
|
||
| it('keeps a contended ECS shard above the install/build/test budget', () => { | ||
| expect( | ||
| timeoutMinutesOn({ repository: 'QwenLM/qwen-code', ecsDisabled: '' }), | ||
| ).toBe(90); | ||
| }); | ||
|
|
||
| it('keeps the hosted fallback on the pre-contention ceiling', () => { | ||
| expect( | ||
| timeoutMinutesOn({ | ||
| repository: 'some-fork/qwen-code', | ||
| ecsDisabled: '', | ||
| }), | ||
| ).toBe(60); | ||
| expect( | ||
| timeoutMinutesOn({ | ||
| repository: 'QwenLM/qwen-code', | ||
| ecsDisabled: 'true', | ||
| }), | ||
| ).toBe(60); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Critical] R1-1: [fails-closed] [regression] This change turns
e2e-test-linux'stimeout-minutesfrom the number60into this expression string, but the existing lane-contract test.github/scripts/ci-runner-routing.test.mjs:552still pinsassert.equal(job['timeout-minutes'], 60)for this exact job and is never updated by this PR. That file importsnode:assert/strict(soassert.equalisstrictEqual), which means the assertion now fails unconditionally in every routing state. The file is listed inHELPER_TESTS(.github/workflows/ci.yml:82) and executed vianode --testby both helper steps — "GitHub CI helper checks" (ci.yml:436) and "Run .github/scripts helper tests" (ci.yml:586); this PR classifies into thefullprofile, so the requiredTest (ubuntu-latest, Node 22.x)check goes red on this PR's own CI run.Witness:
A/B on the contested assertion: BASE (pre-change e2e.yml) passes with value=60; the PR arm fails with the expression string.
Update
.github/scripts/ci-runner-routing.test.mjs:552in this same PR: replace the bare60pin with an evaluation of the new routing-scoped expression — the same substitute-then-evaluate technique this PR adds inscripts/tests/e2e-workflow.test.js— asserting 90 on the pool routing and 60 on both hosted fallbacks (fork and kill-switch), and fix the adjacent stale comment ("Pin the ci.yml pool precedent"), which now contradicts ci.yml's actual pool precedent of 90.The updated pin must keep the two ceilings mirrored from
.github/workflows/ci.yml:219(90 onecs-qwenrouting, 60 hosted) and agree with the routing condition the timeout mirrors (runs-onat .github/workflows/e2e.yml:47). The updated assertion itself must go red iftimeout-minutesregresses to an unconditional value (a flat 60 or flat 90 without the routing condition) — please remove the routing condition from the assertion and run that subtest to prove it.中文说明
本改动把
e2e-test-linux的timeout-minutes从数字60改成了这个表达式字符串,但既有的通道契约测试.github/scripts/ci-runner-routing.test.mjs:552仍然对同一个任务钉死assert.equal(job['timeout-minutes'], 60),且本 PR 没有更新它。该文件导入node:assert/strict(因此assert.equal即strictEqual),所以这条断言现在在任何路由状态下都会无条件失败。该文件位于HELPER_TESTS(.github/workflows/ci.yml:82)中,由两个助手测试步骤通过node --test执行——"GitHub CI helper checks"(ci.yml:436)与 "Run .github/scripts helper tests"(ci.yml:586);本 PR 会被分类为fullprofile,因此本 PR 自己的 CI 运行中必需的Test (ubuntu-latest, Node 22.x)检查会变红。见证(在 HEAD 64a703c 上执行):
node --test .github/scripts/ci-runner-routing.test.mjs→ 23 个测试,22 通过,1 失败——失败子测试为 "carries the pool hygiene and capability steps in order",operator: strictEqual,expected: 60,actual 为新的表达式字符串。对该断言做 A/B:BASE(改动前的 e2e.yml)通过(值为 60),PR 侧失败。请在同一个 PR 中更新
.github/scripts/ci-runner-routing.test.mjs:552:把裸的60钉子替换为对新的路由限定表达式求值——复用本 PR 在scripts/tests/e2e-workflow.test.js中新增的"先替换再求值"手法——断言资源池路由为 90、两种托管回退(fork 与紧急开关)均为 60,并修正旁边那条已过时的注释("Pin the ci.yml pool precedent"),它现在与 ci.yml 实际的资源池上限 90 相矛盾。更新后的钉子必须保持与
.github/workflows/ci.yml:219一致的两个上限(ecs-qwen路由 90,托管 60),并与 timeout 所镜像的路由条件(.github/workflows/e2e.yml:47 的runs-on)保持一致。更新后的断言本身必须在timeout-minutes退化为无条件值(不带路由条件的裸 60 或裸 90)时变红——请从断言中去掉路由条件并运行该子测试来证明这一点。— qwen3.8-max via Qwen Code /review (v0.22.3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified real at the current head:
node --test --test-concurrency=1 .github/scripts/ci-runner-routing.test.mjsfails at line 552 (strictEqualexpected60, actual is the new routing-scoped expression string) — 23 tests, 22 pass, 1 fail. The fullHELPER_TESTSset confirms this is the sole cause of the red requiredTest (ubuntu-latest, Node 22.x)check: 500 tests, exactly this one failure.This round nevertheless does NOT implement the pin update:
.github/scripts/is outside this PR's footprint (the PR touches only.github/workflows/e2e.ymlandscripts/tests/e2e-workflow.test.js), and the round-3 attempt at exactly this pin update was rejected by the autofix gate and reverted for that reason. Repeating the rejected change would fail the gate again, and there is no in-footprint change that can turn the check green while the pin still reads60. This needs a maintainer decision on where the pin update lands:.github/scripts/, so a next round updates the pin here (substitute-then-evaluate: 90 on the pool routing, 60 on both hosted fallbacks, mirroring theruns-oncondition; plus the stale "Pin the ci.yml pool precedent" comment); orLeaving this thread open until a maintainer chooses.
中文说明
已在当前 head 上确认为真实问题:
node --test --test-concurrency=1 .github/scripts/ci-runner-routing.test.mjs在第 552 行失败(strictEqual期望60,实际为新的路由限定表达式字符串)——23 个测试,22 通过,1 失败。完整HELPER_TESTS集合确认这是必需的Test (ubuntu-latest, Node 22.x)检查变红的唯一原因:共 500 个测试,仅此一处失败。但本轮不实施该断言更新:
.github/scripts/在本 PR 的 footprint 之外(本 PR 只改动.github/workflows/e2e.yml和scripts/tests/e2e-workflow.test.js),且第 3 轮对同一断言的更新尝试已被 autofix gate 以该理由拒绝并回滚。重复被拒绝的改动只会再次被 gate 拒绝,而在断言仍钉死60的情况下,footprint 内没有任何改动能让该检查变绿。需要维护者决定断言更新落在哪里:.github/scripts/,下一轮即可在此更新断言(先替换再求值:资源池路由 90、两种托管回退均为 60,与runs-on条件保持一致;同时修正过时的 "Pin the ci.yml pool precedent" 注释);或在维护者作出选择之前,本线程保持打开。