Skip to content

fix(ci): reap release integration containers - #11330

Merged
yiliang114 merged 1 commit into
mainfrom
codex/fix-release-container-cleanup
Sep 8, 2026
Merged

fix(ci): reap release integration containers#11330
yiliang114 merged 1 commit into
mainfrom
codex/fix-release-container-cleanup

Conversation

@yiliang114

@yiliang114 yiliang114 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Labels every container started by the Docker release-validation job with a run-scoped owner and removes only those containers when the test command or job finishes. The final cleanup checks for residue and fails the job if owned containers remain.

Why it's needed

The scheduled release run 34162336173 completed successfully but left eight qwen-code-integration-test-* containers running on ecs-qwen-hk4-10. Those containers had only the shared sandbox-image label, so the E2E cleanup added in #11264 could not identify or remove them.

Reviewer Test Plan

How to verify

  1. Dispatch the Release workflow from this branch with dry_run=true and force_skip_tests=false.
  2. Confirm Integration Tests (Docker) completes and Remove job-owned release containers succeeds on the self-hosted runner.
  3. On that runner, confirm no container remains with org.qwen-code.ci.owner=<run-id>-<attempt>-release.

Evidence (Before & After)

Before: release run 34162336173 left eight running containers after its Docker integration job completed.

After: dry-run 34194126628 completed successfully. Its Docker integration cleanup step succeeded on ecs-qwen-hk4-25, and a post-job SSH check found zero containers carrying org.qwen-code.ci.owner=34194126628-1-release and zero remaining containers created during the job window.

Tested on

OS Status
🍏 macOS ✅ workflow test (49 passed, 1 skipped)
🪟 Windows N/A
🐧 Linux ✅ release dry-run and ECS residue check

Environment (optional)

Local workflow parsing and structural regression test with Node.js 22.

Risk & Scope

  • Main risk or tradeoff: cleanup relies on Docker label filtering and affects only containers owned by the current release run.
  • Not validated / out of scope: historical unlabeled containers and runner disk migration are intentionally separate.
  • Breaking changes / migration notes: none.

Linked Issues

Follow-up to #11264.

中文说明

这个 PR 做了什么

为 Docker 发布验证任务启动的每个容器增加按运行隔离的 owner 标签,并在测试命令或任务结束时只删除这些容器。最终清理步骤会检查是否仍有残留,若存在则让任务失败。

为什么需要

定时发布运行 34162336173 虽然成功结束,但在 ecs-qwen-hk4-10 上留下了八个仍在运行的 qwen-code-integration-test-* 容器。这些容器只有共享的 sandbox 镜像标签,因此 #11264 增加的 E2E 清理无法识别和删除它们。

Reviewer 测试计划

如何验证

  1. 从当前分支手动触发 Release workflow,设置 dry_run=trueforce_skip_tests=false
  2. 确认 Integration Tests (Docker) 完成,并且 self-hosted runner 上的 Remove job-owned release containers 成功。
  3. 在对应 Runner 上确认不存在 org.qwen-code.ci.owner=<run-id>-<attempt>-release 标签的容器。

前后证据

修复前:Release run 34162336173 的 Docker 集成任务结束后留下了八个运行中的容器。

修复后:dry-run 34194126628 全部成功。Docker 集成清理步骤在 ecs-qwen-hk4-25 上成功,任务结束后的 SSH 检查确认 org.qwen-code.ci.owner=34194126628-1-release 标签容器为零,并且任务运行期间创建的容器没有残留。

测试环境

OS 状态
🍏 macOS ✅ workflow 测试(49 passed,1 skipped)
🪟 Windows N/A
🐧 Linux ✅ Release dry-run 和 ECS 残留检查

本地使用 Node.js 22 完成 workflow 解析和结构回归测试。

风险和范围

  • 主要风险或取舍:清理依赖 Docker 标签过滤,并且只处理当前 Release run 拥有的容器。
  • 未验证或范围外:历史无标签容器和 Runner 磁盘迁移保持独立处理。
  • 破坏性变更或迁移说明:无。

关联事项

#11264 的后续修复。

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Dry-run validation is queued: https://github.com/QwenLM/qwen-code/actions/runs/34194126628. The decisive check is that Integration Tests (Docker) and Remove job-owned release containers both succeed, followed by zero containers with this run owner label on the selected ECS runner.

@yiliang114
yiliang114 temporarily deployed to production-release September 8, 2026 07:06 — with GitHub Actions Inactive
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Dry-run verification passed: https://github.com/QwenLM/qwen-code/actions/runs/34194126628. Integration Tests (Docker) and Remove job-owned release containers both succeeded on ecs-qwen-hk4-25. A post-job SSH check found owned_total=0, owned_running=0, and no containers from the job window remained.

@yiliang114
yiliang114 marked this pull request as ready for review September 8, 2026 07:25
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finished — CI landed green on 52f54b0 and the deferred approval was posted. finalize run

Qwen Triage 已完成 —— 52f54b0 的 CI 全绿,延迟审批已提交。查看 finalize 运行

@yiliang114
yiliang114 force-pushed the codex/fix-release-container-cleanup branch from ff686d6 to 52f54b0 Compare September 8, 2026 07:35
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Main changed the release job to execute an extracted script while the first dry-run was running. I rebased the fix onto that refactor, kept the cleanup in the extracted runner, and re-ran the local release workflow suite (62 passed, 1 skipped). Current-head dry-run: https://github.com/QwenLM/qwen-code/actions/runs/34200135053.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every required section is filled in, including the before/after evidence and the full Chinese translation.

A note on history, since it shapes this review: my first pass landed on ff686d6, which predated #11165 and conflicted with main — the inline Run Docker Integration Tests body you patched had just been extracted into .github/scripts/run-release-docker-integration.sh. You force-pushed 52f54b0 mid-review and re-pointed the change at the new structure, so everything below is assessed against that commit. mergeable is now MERGEABLE and the conflict is gone.

Problem: observed, not theoretical. Release run 34162336173 finished green and still left eight qwen-code-integration-test-* containers running on ecs-qwen-hk4-10, and those containers carried only the shared org.qwen-code.ci.sandbox=true image label — so the reaper #11264 added to the E2E lane could not claim them. A concrete residue count on a named runner is exactly the evidence this gate asks for. I also confirmed the bug is still live on main: the extracted script has no ci.owner label, no SANDBOX_FLAGS, no trap and no residue check, so nothing landed underneath this while it was conflicting.

Direction: aligned. The release lane is the one Docker-sandbox lane #11264 did not cover, and leaving it uncovered means every scheduled release keeps leaking containers onto a shared self-hosted pool. No CHANGELOG signal applies — this is CI plumbing, not user-facing behaviour.

Size: not applicable. The three changed files are .github/scripts/run-release-docker-integration.sh (+20), .github/workflows/release.yml (+8) and scripts/tests/release-workflow.test.js (+40/−5); no core paths under packages/ are touched, so the two-tier core gate does not engage.

Approach: the scope feels right, and it is the reuse-first answer on two axes at once. It ports the mechanism already proven in e2e.yml (run-scoped ci.owner label via SANDBOX_FLAGS, an EXIT/INT/TERM trap in the test path, then an always() residue check that fails the job) rather than inventing a second reaping scheme — and it correctly drops the matrix.sandbox guard the E2E version needs, since integration_docker is always Docker. It also respects the structure #11165 just established: the reap logic lives in the extracted script instead of being re-inlined into the workflow, and the residue step invokes that same script with a cleanup argument, which matches the run-release-step.sh <step> subcommand idiom used at sixteen other call sites in this very file. Nothing I would cut, and no drive-by changes.

Risk: no elevated risk signals — none of the three changed files matches the revert-correlated path list.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必填部分都写了,包括前后证据和完整的中文翻译。

先说明一下历史,因为它影响本次审查的读法:我的第一轮审查落在 ff686d6 上,那个版本早于 #11165 并与 main 冲突——你修改的内联 Run Docker Integration Tests 脚本体刚好被抽取到了 .github/scripts/run-release-docker-integration.sh。你在审查过程中 force-push 了 52f54b0,把改动重新对准了新结构,所以下面全部内容都是针对那个 commit 评估的。现在 mergeable 已是 MERGEABLE,冲突消失。

问题: 是已观测到的问题,不是理论性加固。Release 运行 34162336173 成功结束,但仍在 ecs-qwen-hk4-10 上留下八个运行中的 qwen-code-integration-test-* 容器;这些容器只带有共享的 org.qwen-code.ci.sandbox=true 镜像标签,因此 #11264 为 E2E 通道加的清理逻辑无法识别它们。具名 runner 上具体的残留数量,正是这个 gate 想要的证据。我也确认了这个 bug 在 main 上依然存在:抽取出的脚本里没有 ci.owner 标签、没有 SANDBOX_FLAGS、没有 trap、也没有残留检查,所以不存在它在冲突期间已被别人修复的情况。

方向: 对齐。Release 通道是 #11264 唯一没有覆盖的 Docker sandbox 通道;不补上,每次定时发布都会持续在共享 self-hosted 资源池上泄漏容器。CHANGELOG 无对应信号 —— 这是 CI 管线,不是面向用户的行为。

规模: 不适用。改动的三个文件是 .github/scripts/run-release-docker-integration.sh(+20)、.github/workflows/release.yml(+8)和 scripts/tests/release-workflow.test.js(+40/−5);没有触及 packages/ 下的核心路径,因此两层核心 gate 不触发。

方案: 范围合理,而且在两个维度上都是"优先复用"的做法。它移植了 e2e.yml 中已验证的机制(通过 SANDBOX_FLAGS 打 run 级 ci.owner 标签、在测试路径中设置 EXIT/INT/TERM trap、再用一个 always() 残留检查让任务失败),而不是另造一套回收方案——并且正确地去掉了 E2E 版本需要的 matrix.sandbox 判断,因为 integration_docker 永远是 Docker。它也尊重了 #11165 刚建立的结构:回收逻辑放在抽取出的脚本里,而不是重新内联回 workflow;残留检查步骤用 cleanup 参数调用同一个脚本,这与该文件中另外十六处调用点使用的 run-release-step.sh <step> 子命令写法一致。没有我会砍掉的部分,也没有夹带无关改动。

风险: 无升级风险信号 —— 三个改动文件都不匹配与 revert 相关的路径列表。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 52f54b055e9973419a5719a0b8986e48ffb20bb2 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Code review

My independent proposal first. Reading only the title and the "why" — release-lane Docker integration containers leak on self-hosted runners because they are unlabeled — I would have done exactly one thing: reuse the reaper #11264 already proved on the E2E lane. Label each runtime container with a run-scoped owner through the existing SANDBOX_FLAGS hook, trap EXIT/INT/TERM so a cancelled or timed-out job still reaps, then add an always() step that removes by that label and fails the job if residue survives. No new label namespace, no cron sweeper, no runner-side state. That is what this PR does, so I have no simpler alternative to offer — and on one point it beats my proposal, described below.

The mechanism is real, not a dead switch. I traced the chain rather than trusting the variable name: SANDBOX_FLAGS is consumed at packages/cli/src/serve/sandbox.ts:457, parsed with shell-quote's parse(process.env['SANDBOX_FLAGS'], process.env) and pushed onto the docker run argv. Passing process.env as the second argument is precisely what expands ${RELEASE_CONTAINER_OWNER} — GitHub Actions only expands ${{ ... }}, so the literal ${RELEASE_CONTAINER_OWNER} reaches the CLI and shell-quote resolves it. It does not collide with the pre-existing BUILD_SANDBOX_FLAGS either: that is a separate variable consumed by scripts/build_sandbox.js:178 for image build args, so one labels the image and the other labels the runtime container.

The rebase resolved both blockers, and the trap ordering is right. The subtle bit I was watching for is that the trap must be installed before the flock acquisitions — otherwise a failure while waiting on a lock still leaks the containers started up to that point. It is: cleanup_release_containers is defined at the top, the cleanup subcommand branch returns early, and trap cleanup_release_containers EXIT / trap 'exit 1' INT TERM sit on lines 21–22, ahead of the sandbox_revision / lock block. Correct.

Better than what I suggested: rather than duplicating the reap shell inline in a second workflow step, the cleanup step calls the same extracted script with a cleanup argument. That is one copy of the filter string instead of two, and it matches the run-release-step.sh <step> subcommand idiom already used at sixteen call sites in release.yml (step="${1:?usage: ...}" + case). The structural difference from run-release-step.sh is justified, not sloppy: here the argument is an optional mode (no argument must still mean "run the tests"), so [ "${1:-}" = 'cleanup' ] is right where ${1:?} would be wrong.

Two ordering details I checked because they are easy to get wrong and both are fine:

  • The cleanup branch ends in exit 0 / exit 1 before the trap lines, so cleanup mode installs no EXIT trap and cannot double-reap or mask its own residue verdict.
  • Under set -u, the cleanup path only touches ${1:-} and RELEASE_CONTAINER_OWNER, and the latter is set in that step's env:. No unbound-variable abort.

The test adaptation is correct, not a weakening. The four assertions moved off testStep.run and onto the dockerIntegrationScript fixture (defined at scripts/tests/release-workflow.test.js:47, reading the extracted script — the same fixture #11165 established, used 24 times in this file), while the env: and new-step assertions stay on the YAML, including an exact match on the cleanup invocation. The one edit that could have been a quiet loosening is in the release lane runner routing test, where the step filter now excludes commands ending in cleanup. That is legitimate: the routing test asserts RUNNER_ENVIRONMENT is present on steps that actually run tests, and the cleanup branch exits before any RUNNER_ENVIRONMENT reference — so requiring it there would be asserting a falsehood. The excluded step is still covered by its own dedicated assertions in the new test, so nothing lost coverage.

Two nits, neither blocking:

  • The residue check's remaining="$(timeout 30 docker ps -aq --filter ...)" has no || true, so under set -e a daemon that is slow or briefly unreachable fails the step. That is identical to the construct e2e.yml has run in production since fix(ci): reap leaked E2E sandbox containers #11264, and the job already gates on a Check docker daemon step, so I would not change it — just naming that it is load-bearing.
  • Because publish is gated on needs.integration_docker.result == 'success', residue now blocks a release rather than merely warning. I think that is the right tradeoff for a leak that can wedge a shared runner, but the "Risk & Scope" section describes the label-filtering risk without mentioning that consequence — worth a clause.

CI evidence

This is an unattended CI run, so per the gate rules I did not build, run, or test any PR-derived code. Everything below is read from the check-runs on 52f54b055e9973419a5719a0b8986e48ffb20bb2 via the API, fetched once — no polling.

Qwen Code CI (run 34200083143, pull_request) is still running. Nothing has failed, so there is no log excerpt to quote:

Final CI results for 52f54b0 (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Two of these are the ones that matter for this diff, and both are still in flight. Test (ubuntu-latest, Node 22.x) runs npm run test:scripts (.github/workflows/ci.yml:767), so the rewritten structural test has not yet executed against the rebased script — its assertions on the dockerIntegrationScript fixture are unverified by CI right now, verified only by my reading of both files. Lint & Static runs .github/scripts/check-workflow-size.sh (ci.yml:1048); the ratchet baseline for release.yml is 34,861 bytes against 34,829 today, and this PR adds ~600 bytes to it, so that gate should be comfortable. The three skips are pre-existing lane conditions.

Separately, Release (run 34200135053, workflow_dispatch) is the dry-run you dispatched against the rebased branch. Its Integration Tests (Docker) job is in progress right now, which is the most useful pending signal on this PR: it is the first execution of the extracted script's new cleanup mode on a real self-hosted runner. Quality Checks (Build) and Quality Checks (Static) are also in progress; Prepare Release Metadata and two Audio Capture Prebuilds shards are already green.

Not verified: that a running release container actually carries the owner label. This is the one gap I could not close by reading. A structural test asserts script and YAML text, so it would pass identically if SANDBOX_FLAGS were misspelled; and "zero containers found after the job" cannot distinguish "labeled and reaped" from "never labeled at all". Two things bound the gap: the consumption chain above is sound end to end, and the identical chain has been running in production on the E2E lane since #11264, so the mechanism is not novel — what is unproven is only that release-lane containers flow through it.

Sandboxed verification would settle this, with an honest caveat about which lane. @qwen-code /verify is available to you without sponsorship, but it A/B-tests the built product against the base build and does not drive the Release workflow, so it is the wrong instrument for a workflow-and-script change; /tmux does not apply, there being no TUI surface. The decisive evidence is one extra observation inside the dry-run you are already running: while the integration tests are still in flight, assert on the runner that docker ps --filter "label=org.qwen-code.ci.owner=<run>-<attempt>-release" is non-empty, then assert it is empty after the cleanup step. Positive-then-negative is what proves the label is applied; the negative alone is what both dry-runs give us today. If run 34200135053 finishes green that confirms the cleanup mode executes and leaves no residue on the rebased structure — worth citing — but it still would not have observed the label being applied.

中文说明

代码审查

先说我自己的独立方案。 只看标题和"为什么需要"——发布通道的 Docker 集成容器因为没有标签而在 self-hosted runner 上泄漏——我会做的就是一件事:复用 #11264 已在 E2E 通道验证过的回收机制。通过现有的 SANDBOX_FLAGS 钩子给每个运行时容器打上 run 级 owner 标签,trap EXIT/INT/TERM(这样被取消或超时的任务也能回收),再加一个 always() 步骤按该标签删除、若仍有残留则让任务失败。不新增标签命名空间,不加定时清扫,不在 runner 侧存状态。这个 PR 做的正是这件事,所以我没有更简单的替代方案可提——而且有一点它比我的建议更好,见下文。

这套机制是真实生效的,不是死开关。 我没有只看变量名就相信,而是完整追了链路:SANDBOX_FLAGSpackages/cli/src/serve/sandbox.ts:457 被消费,用 shell-quoteparse(process.env['SANDBOX_FLAGS'], process.env) 解析后追加到 docker run 参数里。把 process.env 作为第二个参数传入,正是 ${RELEASE_CONTAINER_OWNER} 能被展开的原因——GitHub Actions 只展开 ${{ ... }},所以字面量会传到 CLI,由 shell-quote 完成解析。它与已有的 BUILD_SANDBOX_FLAGS 也不冲突:后者是独立变量,由 scripts/build_sandbox.js:178 用于镜像构建参数,一个给镜像打标签,一个给运行时容器打标签。

rebase 解决了两个阻塞项,而且 trap 的顺序是对的。 我特别留意的细节是 trap 必须安装在 flock 获取之前——否则等锁期间失败时,此前已启动的容器仍会泄漏。确实如此:cleanup_release_containers 定义在顶部,cleanup 子命令分支提前返回,而 trap cleanup_release_containers EXIT / trap 'exit 1' INT TERM 位于第 21–22 行,在 sandbox_revision 与锁代码块之前。正确。

比我建议的更好的一点: 清理步骤没有在第二个 workflow 步骤里重复内联回收 shell,而是用 cleanup 参数调用同一个抽取出的脚本。这样过滤字符串只有一份而不是两份,并且与 release.yml 中另外十六处调用点已在使用的 run-release-step.sh <step> 子命令写法一致(step="${1:?usage: ...}" + case)。它与 run-release-step.sh 的结构差异是有理由的、不是疏漏:这里的参数是可选的模式(不传参数仍须表示"运行测试"),所以 [ "${1:-}" = 'cleanup' ] 是对的,用 ${1:?} 反而错。

有两个容易写错、但这里都正确的顺序细节我确认过:

  • cleanup 分支在 trap 之前就以 exit 0 / exit 1 结束,所以 cleanup 模式不会安装 EXIT trap,既不会重复回收,也不会掩盖自己的残留判定。
  • set -u 下,cleanup 路径只用到 ${1:-}RELEASE_CONTAINER_OWNER,后者已在该步骤的 env: 中设置,不会触发未绑定变量而中止。

测试的改动是正确的,不是放松断言。 四处断言从 testStep.run 移到了 dockerIntegrationScript fixture(定义于 scripts/tests/release-workflow.test.js:47,读取抽取出的脚本——正是 #11165 建立的同一个 fixture,本文件中被使用 24 次),而 env: 和新步骤的断言仍留在 YAML 上,包括对 cleanup 调用的精确匹配。唯一可能构成悄悄放松的改动在 release lane runner routing 测试里:步骤过滤现在排除了以 cleanup 结尾的命令。这是合理的——该路由测试断言的是"真正运行测试的步骤"必须具备 RUNNER_ENVIRONMENT,而 cleanup 分支在任何 RUNNER_ENVIRONMENT 引用之前就已退出,在那里要求它等于断言一个假命题。被排除的步骤仍由新测试中它自己的专属断言覆盖,所以没有任何覆盖丢失。

两个小问题,都不阻塞:

  • 残留检查里的 remaining="$(timeout 30 docker ps -aq --filter ...)" 没有 || true,所以在 set -e 下,若 daemon 变慢或短暂不可达,该步骤会失败。这与 e2e.ymlfix(ci): reap leaked E2E sandbox containers #11264 起在生产中运行的写法完全相同,而且该 job 已经有 Check docker daemon 步骤做前置把关,所以我不建议改——只是指出它是有实际影响的。
  • 由于 publishneeds.integration_docker.result == 'success' 为条件,残留现在会阻断发布,而不只是告警。对于一个可能拖垮共享 runner 的泄漏,我认为这个取舍是对的,但 "Risk & Scope" 一节只描述了标签过滤的风险,没有提到这个后果——值得补一句。

CI 证据

这是无人值守的 CI 运行,因此按 gate 规则我没有构建、运行或测试任何 PR 派生代码。下面全部内容都是通过 API 读取 52f54b055e9973419a5719a0b8986e48ffb20bb2 上的 check-runs 得到的,只抓取一次,没有轮询。

Qwen Code CI(运行 34200083143,pull_request仍在运行中,目前没有任何失败,因此没有日志摘录可引用。表格中的状态是抓取当时的真实状态,未对结果做任何猜测。

其中两项对本 diff 最关键,而且都还在跑。Test (ubuntu-latest, Node 22.x) 会执行 npm run test:scripts.github/workflows/ci.yml:767),所以重写后的结构测试尚未在 rebase 后的脚本上执行过——它对 dockerIntegrationScript fixture 的断言目前没有 CI 验证,只有我对两个文件的阅读验证。Lint & Static 会执行 .github/scripts/check-workflow-size.shci.yml:1048);release.yml 的棘轮基线是 34,861 字节,当前 34,829,本 PR 只增加约 600 字节,所以该 gate 应当宽裕。三个 skipped 是既有的通道条件。

另外,Release(运行 34200135053,workflow_dispatch)是你针对 rebase 后分支触发的 dry-run。它的 Integration Tests (Docker) job 此刻正在运行,这是本 PR 上最有价值的待定信号:这是抽取脚本的新 cleanup 模式第一次在真实 self-hosted runner 上执行。Quality Checks (Build)Quality Checks (Static) 也在进行中;Prepare Release Metadata 和两个 Audio Capture Prebuilds 分片已经绿灯。

未验证:运行中的 release 容器是否真的带有 owner 标签。 这是我无法靠阅读关闭的唯一缺口。结构测试断言的是脚本和 YAML 文本,所以即使 SANDBOX_FLAGS 拼错它也照样通过;而"任务结束后找不到容器"无法区分"打了标签并被回收"和"根本没打标签"。有两点限定了这个缺口:上面的消费链路端到端是通的,而且同一条链路自 #11264 起已在 E2E 通道生产运行,所以机制并不新——未被证明的只是 release 通道的容器确实走了这条链路。

沙箱验证可以定这件事,但要诚实说明该用哪条通道。@qwen-code /verify 对你可用且无需他人代触发,但它做的是构建产物与 base 构建的 A/B 对比,不会驱动 Release workflow,所以对"workflow + 脚本"这类改动是错误的工具;/tmux 不适用,因为没有 TUI 界面。真正决定性的证据是在你已经跑着的 dry-run 里多加一次观测:在集成测试仍在运行时,在 runner 上断言 docker ps --filter "label=org.qwen-code.ci.owner=<run>-<attempt>-release" 非空,然后在清理步骤之后断言为空。先正后负才能证明标签确实被打上了;只有负向检查是今天两次 dry-run 给我们的全部。如果运行 34200135053 最终绿灯,那能证明 cleanup 模式在 rebase 后的结构上可执行且不留残留——值得引用——但它仍然没有观测到标签被 applied 的那一刻。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 52f54b055e9973419a5719a0b8986e48ffb20bb2 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — solid and ready to ship once CI lands; the two nits are named below and neither blocks, and the only reason this is not 5/5 is that the suite has not yet executed against the rebased script.

Stepping back, the thing that stands out is how the rebase went. A conflicting PR is where most contributions get sloppy — the easy move is to re-inline the old shell into the new YAML and force the test green by loosening it. You did the opposite: the reap logic went into the extracted script where #11165 wanted it, the residue step calls that same script with a cleanup argument instead of duplicating a second copy of the filter string, and the one test filter you relaxed (!command.endsWith(' cleanup')) relaxes an assertion that would otherwise have been false. That is a rebase done by someone who read the refactor they were rebasing onto, not around it. It also improves on what I had proposed in the review — I would have kept the cleanup shell inline in the workflow step.

Back to my independent proposal: it was the same proposal, which is the strongest signal I have that the direction is right. Reusing #11264's reaper rather than inventing a second one, and dropping the matrix.sandbox guard the E2E version needs, both show the port was thought about. I also verified the mechanism end to end instead of taking the env var's name on faith — SANDBOX_FLAGS really is consumed in sandbox.ts and really does expand ${RELEASE_CONTAINER_OWNER} through shell-quote — so this is not a dead switch, which is the failure mode I most expected from a CI-label PR.

Does it solve something people care about? Yes. A scheduled release silently leaving eight containers on a shared self-hosted pool is how a runner eventually wedges for everyone, and #11264 already established that we care enough to reap the E2E lane; leaving the release lane uncovered was an inconsistency, not a decision. Is the code straightforward? Very — 68 lines across three files, no new abstraction, no new label namespace. Would I curse whoever wrote this in six months? No. The two things I flagged are a set -e sensitivity that is byte-identical to what has run in production since #11264, and a suggestion to note in "Risk & Scope" that residue now blocks publish rather than warning — I agree with that behaviour, I just want it written down.

The reservation I am holding, and the reason for 4 rather than 5: nothing has yet executed the rebased script. Test (ubuntu-latest, Node 22.x) — which is what actually runs the rewritten structural test via npm run test:scripts — is still in progress, and the Release dry-run's Integration Tests (Docker), the first real execution of the new cleanup mode, is in progress too. My confidence in the test assertions rests on reading both files, not on a green run. Beyond that sits the gap I could not close by reading at all: neither dry-run observes the owner label being applied, only that nothing remains, and a misspelled SANDBOX_FLAGS would produce the same empty result. The static chain and #11264's production precedent bound that risk tightly, so I am not blocking on it — but a positive-then-negative docker ps check mid-job would turn an assumption into an observation, and it costs one line.

Am I approving because it is genuinely good, or because I ran out of reasons to say no? The first. My earlier pass found two real blockers and both were fixed properly rather than papered over, the design matches what I would have written, and the change is minimal and idiomatic to the structure it now lives in.

So: approval is deferred until CI lands green on 52f54b055e9973419a5719a0b8986e48ffb20bb2. I am not posting an approval in this run, because the suite that would execute this diff has not finished — approving now would attest to a result that does not exist yet. Once every check on that commit completes green, the approval is posted automatically against it; if anything lands red or the head moves, it is withheld and flagged instead.

中文说明

Confidence: 4/5 —— 改动扎实,CI 落地后即可合并;下面点名的两个小问题都不阻塞;之所以不是 5/5,唯一原因是测试套件还没有在 rebase 后的脚本上执行过。

退一步看,最突出的是这次 rebase 的处理方式。冲突的 PR 正是最容易敷衍的场合——省事的做法是把旧的 shell 重新内联进新的 YAML,再靠放松断言把测试弄绿。你做的恰恰相反:回收逻辑进了 #11165 希望它去的抽取脚本,残留检查步骤用 cleanup 参数调用同一个脚本、而不是再复制一份过滤字符串,而你唯一放松的那个测试过滤条件(!command.endsWith(' cleanup'))放松的是一条本来就会失败的断言。这是一次由真正读过所要 rebase 到的那个重构的人完成的 rebase,而不是绕着它做。它也优于我在审查中提的方案——我本来会把清理 shell 内联在 workflow 步骤里。

回到我的独立方案:那是同一个方案,这是我能给出的、说明方向正确的最强信号。复用 #11264 的回收机制而不是另造一套、去掉 E2E 版本需要的 matrix.sandbox 判断,都说明这次移植是经过思考的。我也没有只凭环境变量名就相信它生效,而是完整验证了链路——SANDBOX_FLAGS 确实在 sandbox.ts 中被消费,${RELEASE_CONTAINER_OWNER} 也确实通过 shell-quote 展开——所以这不是死开关,而这正是我对一个 CI 标签类 PR 最担心的失效模式。

它解决的是大家在意的问题吗?是。一次定时发布在共享 self-hosted 资源池上静默留下八个容器,正是 runner 最终对所有人卡死的成因;#11264 已经确立了我们愿意为 E2E 通道做回收,把 release 通道留在外面是一个不一致,而不是一个经过权衡的决定。代码是否直白?非常直白——三个文件 68 行,没有新抽象,没有新的标签命名空间。六个月后我会骂写这段代码的人吗?不会。我提出的两点,一是与 #11264 以来生产运行版本逐字节相同的 set -e 敏感性,二是建议在 "Risk & Scope" 里写明残留现在会阻断 publish 而不只是告警——我认同这个行为,只是希望它被写下来。

我保留的顾虑,也是给 4 分而不是 5 分的原因:目前还没有任何东西真正执行过 rebase 后的脚本。Test (ubuntu-latest, Node 22.x)——也就是通过 npm run test:scripts 实际运行重写后结构测试的那个 job——仍在进行中;而 Release dry-run 的 Integration Tests (Docker),即新 cleanup 模式的第一次真实执行,也仍在进行中。我对这些测试断言的信心来自阅读两个文件,而不是来自一次绿灯运行。此外还有一个我完全无法靠阅读关闭的缺口:两次 dry-run 都没有观测到 owner 标签被打上的那一刻,只观测到没有残留,而拼错的 SANDBOX_FLAGS 会产生完全相同的空结果。静态链路加上 #11264 的生产先例把这个风险限定得很紧,所以我不以此阻塞——但在任务运行中做一次先正后负的 docker ps 检查,能把一个假设变成一次观测,成本是一行。

我批准是因为它确实好,还是因为我说不出拒绝的理由了?是前者。我上一轮审查发现了两个真实的阻塞项,两者都被正确修复而不是掩盖;设计与我自己会写的方案一致;改动最小,并且符合它现在所处结构的惯用写法。

所以:批准延迟到 CI 在 52f54b055e9973419a5719a0b8986e48ffb20bb2 上全绿之后。本次运行我不发布批准,因为将要执行这个 diff 的测试套件还没有跑完——现在批准等于为一个尚不存在的结果背书。该 commit 上所有检查全绿后,批准会自动针对它发布;若有任何检查变红或 head 发生移动,则不会批准并会标记出来。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 52f54b055e9973419a5719a0b8986e48ffb20bb2 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship — CI landed green after the review. ✅

@yiliang114
yiliang114 added this pull request to the merge queue Sep 8, 2026
Merged via the queue into main with commit d87ccc6 Sep 8, 2026
76 of 77 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants