-
Notifications
You must be signed in to change notification settings - Fork 3.1k
ci(ecs): wait out npm publish propagation before updating the runner fleet #10442
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
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 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,15 +4,93 @@ | |||||||||||||||
| * SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||
| */ | ||||||||||||||||
|
|
||||||||||||||||
| import { readFileSync } from 'node:fs'; | ||||||||||||||||
| import { spawnSync } from 'node:child_process'; | ||||||||||||||||
| import { | ||||||||||||||||
| chmodSync, | ||||||||||||||||
| mkdtempSync, | ||||||||||||||||
| readFileSync, | ||||||||||||||||
| rmSync, | ||||||||||||||||
| writeFileSync, | ||||||||||||||||
| } from 'node:fs'; | ||||||||||||||||
| import { tmpdir } from 'node:os'; | ||||||||||||||||
| import { join } from 'node:path'; | ||||||||||||||||
| import { describe, expect, it } from 'vitest'; | ||||||||||||||||
|
|
||||||||||||||||
| describe('ECS runner qwen update workflow', () => { | ||||||||||||||||
| const workflow = readFileSync( | ||||||||||||||||
| '.github/workflows/update-ecs-runner-qwen.yml', | ||||||||||||||||
| 'utf8', | ||||||||||||||||
| const workflow = readFileSync( | ||||||||||||||||
| '.github/workflows/update-ecs-runner-qwen.yml', | ||||||||||||||||
| 'utf8', | ||||||||||||||||
| ); | ||||||||||||||||
|
|
||||||||||||||||
| function step(name) { | ||||||||||||||||
| const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||||||||||||||||
| const match = workflow.match( | ||||||||||||||||
| new RegExp( | ||||||||||||||||
| `\\n\\s+- name:\\s*(['"])${escaped}\\1[\\s\\S]*?(?=\\n\\s+- name:\\s*['"]|\\n\\s{2}[a-zA-Z0-9_-]+:|$)`, | ||||||||||||||||
| ), | ||||||||||||||||
| ); | ||||||||||||||||
| return match?.[0] ?? ''; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| // The body of a step's `run: |-` block, dedented to column zero. | ||||||||||||||||
| function stepBody(name) { | ||||||||||||||||
| const body = step(name).match(/run: \|-\n([\s\S]*)$/)?.[1] ?? ''; | ||||||||||||||||
| return body.replace(/^ {10}/gm, ''); | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+35
to
+38
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-6: The added Witness: Consume the shared module instead ( import { getWorkflowJob, getWorkflowStep } from './workflow-helpers.js';
const resolveStep = getWorkflowStep(getWorkflowJob(workflow, 'resolve'), 'Resolve version');Reuse is constrained by one existing fact: 中文说明新增的 建议改为消费共享模块( — qwen3.8-max via Qwen Code /review (v0.22.3) |
||||||||||||||||
|
|
||||||||||||||||
| // Runs the 'Resolve version' step body against a stubbed `npm` that 404s for | ||||||||||||||||
| // its first `failures` invocations and then reports `version`. | ||||||||||||||||
| function runResolve({ failures = 0, version = '0.22.3', env = {} } = {}) { | ||||||||||||||||
| const dir = mkdtempSync(join(tmpdir(), 'ecs-update-')); | ||||||||||||||||
| try { | ||||||||||||||||
| const counter = join(dir, 'attempts'); | ||||||||||||||||
| const npmStub = join(dir, 'npm'); | ||||||||||||||||
| writeFileSync( | ||||||||||||||||
| npmStub, | ||||||||||||||||
| [ | ||||||||||||||||
| '#!/usr/bin/env bash', | ||||||||||||||||
| `attempt=$(( $(cat ${counter} 2>/dev/null || echo 0) + 1 ))`, | ||||||||||||||||
| `echo "$attempt" > ${counter}`, | ||||||||||||||||
| `if (( attempt <= ${failures} )); then`, | ||||||||||||||||
| ' echo "npm error code E404" >&2', | ||||||||||||||||
| ' echo "npm error 404 No match found for version" >&2', | ||||||||||||||||
| ' exit 1', | ||||||||||||||||
| 'fi', | ||||||||||||||||
| `echo '${version}'`, | ||||||||||||||||
| ].join('\n'), | ||||||||||||||||
| { mode: 0o755 }, | ||||||||||||||||
| ); | ||||||||||||||||
| chmodSync(npmStub, 0o755); | ||||||||||||||||
|
|
||||||||||||||||
| const script = join(dir, 'resolve.sh'); | ||||||||||||||||
| writeFileSync(script, stepBody('Resolve version')); | ||||||||||||||||
| const ghOutput = join(dir, 'github-output'); | ||||||||||||||||
| writeFileSync(ghOutput, ''); | ||||||||||||||||
|
|
||||||||||||||||
| const result = spawnSync('bash', [script], { | ||||||||||||||||
| encoding: 'utf8', | ||||||||||||||||
| env: { | ||||||||||||||||
| ...process.env, | ||||||||||||||||
| PATH: `${dir}:${process.env.PATH ?? ''}`, | ||||||||||||||||
|
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] The new bash-driven behavioural tests are not covered by any win32 Witness: Suggested fix — add the file to the win32 exclude list (the production step under test is genuinely single-platform, // scripts/tests/vitest.config.ts — win32 exclude list
'scripts/tests/serve-ab-workflow.test.js',
'scripts/tests/update-ecs-runner-qwen-workflow.test.js',The exclusion must stay win32-conditional and an explicit entry, not a widened glob: 中文说明新的 bash 驱动行为测试没有被 建议修复:把该文件加入 win32 排除列表(被测的生产步骤本就只在 — qwen3.8-max via Qwen Code /review (v0.22.3) |
||||||||||||||||
| GITHUB_OUTPUT: ghOutput, | ||||||||||||||||
| INPUT_VERSION: '0.22.3', | ||||||||||||||||
|
Comment on lines
+74
to
+75
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-7: The resolve step's Witness: Assert the env declarations in the wiring test: expect(workflow).toContain("INPUT_VERSION: '${{ inputs.version || github.event.client_payload.version }}'");
expect(workflow).toContain("RESOLVE_TIMEOUT_SECONDS: '1500'");
expect(workflow).toContain("RESOLVE_INTERVAL_SECONDS: '30'");The assertions must match the workflow literals exactly as declared in the resolve step's env block (.github/workflows/update-ecs-runner-qwen.yml:35,44-45). Removing any of the three 中文说明resolve 步骤的 建议在接线测试中断言这三个 env 声明(见上方代码块)。断言必须与 resolve 步骤 env 块中声明的 workflow 字面量完全一致(.github/workflows/update-ecs-runner-qwen.yml:35,44-45)。验收标准:从 workflow 中删除三条 — qwen3.8-max via Qwen Code /review (v0.22.3) |
||||||||||||||||
| RESOLVE_TIMEOUT_SECONDS: '60', | ||||||||||||||||
| RESOLVE_INTERVAL_SECONDS: '0', | ||||||||||||||||
| ...env, | ||||||||||||||||
| }, | ||||||||||||||||
| }); | ||||||||||||||||
| return { | ||||||||||||||||
| status: result.status, | ||||||||||||||||
| stdout: result.stdout ?? '', | ||||||||||||||||
| stderr: result.stderr ?? '', | ||||||||||||||||
| output: readFileSync(ghOutput, 'utf8'), | ||||||||||||||||
| attempts: Number(readFileSync(counter, 'utf8').trim()), | ||||||||||||||||
| }; | ||||||||||||||||
| } finally { | ||||||||||||||||
| rmSync(dir, { recursive: true, force: true }); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| describe('ECS runner qwen update workflow', () => { | ||||||||||||||||
| it('installs without the selected runner npm prefix', () => { | ||||||||||||||||
| expect(workflow).toContain('cd "${RUNNER_TEMP:?}"'); | ||||||||||||||||
| expect(workflow).toContain('sudo env -u NPM_CONFIG_PREFIX npm install -g'); | ||||||||||||||||
|
|
@@ -37,4 +115,58 @@ describe('ECS runner qwen update workflow', () => { | |||||||||||||||
| expect(workflow).toContain('if [[ "${attempt}" -lt 3 ]]; then'); | ||||||||||||||||
| expect(workflow).toContain('sudo rm -rf "${PKG_DIR}"/.qwen-code-*'); | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| it('resolves once on a hosted runner and feeds every pool', () => { | ||||||||||||||||
| // One resolution shared by the matrix is what keeps pools that start | ||||||||||||||||
| // hours apart from installing different versions; it also keeps the | ||||||||||||||||
| // registry wait off the ECS runners. | ||||||||||||||||
| expect(workflow).toContain(" runs-on: 'ubuntu-latest'"); | ||||||||||||||||
| expect(workflow).toContain( | ||||||||||||||||
| " version: '${{ steps.version.outputs.version }}'", | ||||||||||||||||
| ); | ||||||||||||||||
|
Comment on lines
+124
to
+126
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-4: This wiring test asserts the job Witness:
Suggested change
The pinned id must stay 中文说明这个接线测试断言了 job 的 验收标准:在 workflow 中把 — qwen3.8-max via Qwen Code /review (v0.22.3) |
||||||||||||||||
| expect(workflow).toContain(" needs: 'resolve'"); | ||||||||||||||||
| // Both consumers read the job output; a leftover step reference would | ||||||||||||||||
| // silently expand to an empty version and install `@qwen-code/qwen-code@`. | ||||||||||||||||
| const consumers = workflow.match( | ||||||||||||||||
| /VERSION: '\$\{\{ needs\.resolve\.outputs\.version \}\}'/g, | ||||||||||||||||
| ); | ||||||||||||||||
| expect(consumers).toHaveLength(2); | ||||||||||||||||
| expect(workflow).not.toContain( | ||||||||||||||||
| "VERSION: '${{ steps.version.outputs.version }}'", | ||||||||||||||||
| ); | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| it('waits out npm publish propagation instead of failing the race', () => { | ||||||||||||||||
| // `npm publish --provenance` returns before the version is resolvable | ||||||||||||||||
| // (~16 minutes for v0.22.3), and release.yml dispatches this workflow as | ||||||||||||||||
| // soon as it returns. | ||||||||||||||||
| const resolved = runResolve({ failures: 3 }); | ||||||||||||||||
| expect(resolved.status).toBe(0); | ||||||||||||||||
| expect(resolved.attempts).toBe(4); | ||||||||||||||||
| expect(resolved.output.trim()).toBe('version=0.22.3'); | ||||||||||||||||
| expect(resolved.stdout).toContain('is not on the registry yet'); | ||||||||||||||||
| // The per-attempt 404 noise stays out of the log on the happy path. | ||||||||||||||||
| expect(resolved.stderr).not.toContain('E404'); | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| it('fails with the registry error once the wait budget is spent', () => { | ||||||||||||||||
| const resolved = runResolve({ | ||||||||||||||||
| failures: 99, | ||||||||||||||||
| env: { RESOLVE_TIMEOUT_SECONDS: '0' }, | ||||||||||||||||
| }); | ||||||||||||||||
| expect(resolved.status).toBe(1); | ||||||||||||||||
| expect(resolved.output.trim()).toBe(''); | ||||||||||||||||
| // The suppressed stderr is replayed, so the log still says *why*. | ||||||||||||||||
| expect(resolved.stderr).toContain('npm error code E404'); | ||||||||||||||||
| // The annotation stays on stdout, where Actions parses workflow commands. | ||||||||||||||||
| expect(resolved.stdout).toContain( | ||||||||||||||||
| "::error::No published qwen version matches '0.22.3' after 0s.", | ||||||||||||||||
| ); | ||||||||||||||||
| }); | ||||||||||||||||
|
|
||||||||||||||||
| it('resolves the latest dist-tag when dispatched without a version', () => { | ||||||||||||||||
| const resolved = runResolve({ env: { INPUT_VERSION: '' } }); | ||||||||||||||||
| expect(resolved.status).toBe(0); | ||||||||||||||||
| expect(resolved.output.trim()).toBe('version=0.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.
[Suggestion] R1-5: A single
npm viewcan itself run ~16 minutes under npm's built-in fetch defaults (fetch-retries=2, fetch-timeout=300000ms, 10s/60s backoff — verified vianpm config ls -l; the fresh ubuntu-latest resolve job has no overrides), so two slow attempts overshoot the 30-minute job timeout. During a registry stall — a different failure class from the fast-404 propagation this loop was written for — attempt 1 burns the full retry budget (~16 min) and ends still under the 25-minute deadline, the loop sleeps and retries, and attempt 2 crossestimeout-minutes: 30: Actions cancels the job mid-npm viewbefore the designed failure path ever runs. The oncall then sees "The operation was canceled" with no replayed registry stderr and no::error::annotation, and the four update jobs show only "skipped" — indistinguishable from an infra flake, defeating exactly the diagnostics this PR added.Witness:
Bound each attempt so the deadline check retains control (GNU
timeoutis available onubuntu-latest):The per-attempt bound must keep the worst-case total (deadline + one final attempt) under the job timeout —
RESOLVE_TIMEOUT_SECONDS: '1500'sits beneathtimeout-minutes: 30in this file. ArunResolvevariant whose stub npm sleeps past the per-attempt bound pins the fix: without the bound the synchronousspawnSynchangs and vitest'stestTimeoutfails the test; with it, the script exits 1 within budget.中文说明
在 npm 内建的 fetch 默认值下(fetch-retries=2、fetch-timeout=300000ms、10s/60s 退避——已用
npm config ls -l验证;全新的 ubuntu-latest resolve job 没有任何覆盖),单次npm view本身就可能跑约 16 分钟,两次慢速尝试就会越过 30 分钟的 job 超时。当 registry 卡住时(与这个轮询循环要解决的快速 404 传播不同的故障类别),第 1 次尝试耗尽全部重试预算(约 16 分钟)后仍未到 25 分钟的 deadline,循环休眠后重试,第 2 次尝试会越过timeout-minutes: 30:Actions 会在npm view中途取消 job,设计好的失败路径(回放 stderr +::error::注解)永远走不到。值班人员只会看到「The operation was canceled」,没有回放的 registry stderr、没有::error::注解,四个 update job 只显示「skipped」——与基础设施抖动无法区分,恰好废掉了本 PR 新增的诊断能力。建议给每次尝试加上界,让 deadline 检查保持控制权(
ubuntu-latest上有 GNUtimeout,见上方代码块)。每次尝试的上界必须保证最坏情况总时长(deadline + 最后一次尝试)不超过 job 超时——本文件中RESOLVE_TIMEOUT_SECONDS: '1500'位于timeout-minutes: 30之下。验收标准:新增一个runResolve变体,让其 stubnpm在第一次调用时 sleep 超过每次尝试的上界——没有上界时同步的spawnSync会挂起、vitest 的testTimeout使测试失败;有了上界,脚本会在预算内以 1 退出。— qwen3.8-max via Qwen Code /review (v0.22.3)