fix(ci): reap leaked E2E sandbox containers - #11264
Conversation
|
E2E verification completed on the self-hosted ECS pool: run 34086645223 passed.
This verifies the successful-job cleanup path and owner scoping. Hard runner loss/SIGKILL remains outside the job-level guarantee and still needs the separate TTL janitor discussed in the PR. |
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head 5fab39da.
The mechanism actually reaches the container, which is the one place a label-based reap silently dies. SANDBOX_FLAGS has exactly one production reader — packages/cli/src/serve/sandbox.ts:457, whose parsed flags are pushed onto docker run — and that path is the one the Linux E2E jobs use, because start_sandbox is what packages/cli/src/llm.tsx:96 imports when QWEN_SANDBOX=docker. The value is written as '--label org.qwen-code.ci.owner=${E2E_CONTAINER_OWNER}' and nothing in the shell expands it, since it never appears on a command line; what expands it is parse(process.env['SANDBOX_FLAGS'], process.env) at :458, which passes process.env as shell-quote's environment. I probed that directly against the pinned dependency rather than trusting the reading: parse('--label org.qwen-code.ci.owner=${E2E_CONTAINER_OWNER}', {E2E_CONTAINER_OWNER:'341-2-1'}) yields ["--label","org.qwen-code.ci.owner=341-2-1"].
Nothing here can corrupt a step's exit status, which is the other way this kind of trap goes wrong. Every command in cleanup_e2e_job is guarded (|| container_ids='', > /dev/null 2>&1 || echo "::warning::", rm -rf … || true), so the set -e in force inside the EXIT trap has nothing to abort on; trap 'exit 1' INT TERM still falls through the EXIT trap, so a cancelled shard reaps too. Replacing the inner trap 'rm -rf "$TMPDIR"' EXIT is safe for the same reason — the outer handler reads QWEN_CI_TMPDIR when it fires, not when it is installed, and the modified assertion at e2e-workflow.test.js:385 keeps that cleanup pinned instead of dropping it. Scoping the owner string to run_id-run_attempt-shard is the right granularity: concurrent shards on one host cannot reap each other, and a re-run does not reach into the attempt that is still live. docker run carries --rm (sandbox.ts:454), so what this reaps is the running-orphan case, and docker ps -aq covers stopped leftovers too.
One behaviour to be aware of, not to change. The in-step cleanup tolerates an unreachable daemon; the post-step's first docker ps does not, so a wedged Docker on a self-hosted runner turns an otherwise-green shard red. That is the correct trade for a step whose whole purpose is to prove the guarantee held — it is the ::error::E2E containers remain branch, and it is pinned by the new test's last two assertions.
CI facts. Every lane that ran is green at this head: Lint & Static 15m9s, Test (ubuntu-latest) 17m20s (which is where scripts/tests/e2e-workflow.test.js lives), web-shell E2E Smoke, both Desktop Shell lanes, and all six Linux E2E shards plus both macOS shards. Only review-pr is pending, nothing is red. The Docker shards are the useful evidence here: they finished at 23m/26m/35m with overlapping spans, each with the reap step green — so the per-shard labels did not collide and nothing was left behind on the host.
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
|
Released in v0.23.1. |
4 participants
What this PR does
Assigns every Docker E2E matrix job a unique owner label, then force-removes only containers carrying that label when the test shell exits. A separate
always()step repeats the owner-scoped cleanup on persistent self-hosted runners and fails if any owned container remains.The existing Linux scratch-directory cleanup is folded into the same exit handler so the two cleanup responsibilities cannot overwrite each other's shell trap.
Why it's needed
The persistent ECS runners accumulated hundreds of running
qwen-code-integration-test-*containers. One sandbox from the successfulE2E Test (Linux) - sandbox:docker - shard 2/3job in run 33972931298 was still consuming about one CPU core roughly 37 hours after the job had completed successfully.The sandbox uses
docker run --rm, but--rmonly removes a container after that container exits. If the attached Docker client or its PTY dies without stopping the daemon-owned container, the container keeps running and retains its mounted integration-test directory. Image pruning cannot reclaim it. Because several runner registrations share one Docker daemon, cleanup must be scoped to an exact matrix-job owner rather than a broad container-name prefix.Reviewer Test Plan
How to verify
Run the E2E workflow from this branch. During a Docker shard, confirm its sandbox containers carry the job-specific
org.qwen-code.ci.ownervalue. After the shard completes or fails,docker ps -aq --filter label=org.qwen-code.ci.owner=<owner>should return no containers. Containers with a different owner label must remain untouched.The focused workflow regression test should pass and should fail if the runtime label, exit trap,
always()cleanup, owner filter, removal command, or post-cleanup assertion is removed.Evidence (Before & After)
N/A — non-UI CI change. A Docker probe confirmed that killing an attached Docker client can leave its container running, and that exact owner-label cleanup removes the owned container while preserving an unrelated running container. The full branch E2E run 34086645223 passed. All three Docker shards and their owner-scoped cleanup steps passed; queries on the actual ECS Docker daemons returned zero containers for all three owners after completion. Two injected owner-labelled probes on the shared Runner5 daemon were also removed, leaving zero probe and newly labelled E2E containers.
Tested on
Environment (optional)
macOS, Node.js 22.22.0, Docker client 28.0.0 with Docker server 27.4.0. The focused workflow test, ESLint, Prettier, actionlint, and Bash syntax checks pass locally. The full self-hosted Linux E2E workflow passed on the ECS pool.
Risk & Scope
docker rm -f, but only against an exact owner label unique to the workflow run, attempt, and Docker shard; concurrent jobs use different owners.Linked Issues
Related: #8816, #10971, #11001
中文说明
这个 PR 做了什么
给每一个 Docker E2E 矩阵任务分配唯一的 owner 标签，并在测试 shell 退出时，只强制删除带有这个标签的容器。另一个带
always()条件的步骤会在持久化的 self-hosted runner 上再次执行按 owner 限定的清理；如果清理后仍有本任务的容器残留，任务会明确失败。现有的 Linux 临时目录清理也合并到同一个退出处理函数中，避免两项清理逻辑设置的 shell trap 互相覆盖。
为什么需要它
持久化 ECS runner 上累积了数百个仍在运行的
qwen-code-integration-test-*容器。运行记录 33972931298 中成功结束的E2E Test (Linux) - sandbox:docker - shard 2/3任务留下了一个 sandbox；任务成功结束约 37 小时后，这个容器仍在消耗大约一个 CPU 核。sandbox 使用了
docker run --rm，但--rm只会在容器退出后删除容器。如果与容器连接的 Docker client 或它的 PTY 在没有停止 daemon 侧容器的情况下退出，容器会继续运行，并继续占用挂载的 integration-test 目录。镜像清理无法回收这种容器。由于多个 runner 注册实例共用同一个 Docker daemon，清理必须使用精确的矩阵任务 owner，不能使用宽泛的容器名称前缀。Reviewer 测试计划
如何验证
从本分支运行 E2E workflow。Docker shard 运行期间，确认它的 sandbox 容器带有任务专属的
org.qwen-code.ci.owner。该 shard 成功或失败结束后，执行docker ps -aq --filter label=org.qwen-code.ci.owner=<owner>应当查不到任何容器。带有不同 owner 标签的容器必须保持不变。定向 workflow 回归测试应当通过；删除 runtime 标签、退出 trap、
always()清理、owner 过滤、删除命令或清理后的残留断言中的任意一项，都应使测试失败。证据（前后对比）
不适用 —— 这是非 UI 的 CI 改动。Docker 探针已确认：杀死连接中的 Docker client 可能让容器继续运行；精确 owner 标签清理只会删除属于该 owner 的容器，同时保留另一个无关的运行中容器。完整的分支 E2E 运行记录 34086645223 已通过。三个 Docker shard 及各自按 owner 限定的 cleanup step 全部成功；运行结束后，在实际 ECS Docker daemon 上查询三个 owner，结果均为零。Runner5 共用 daemon 上注入的两个 owner 探针也都被删除，最终探针数和新标签 E2E 容器数均为零。
测试环境
环境（可选）
macOS，Node.js 22.22.0，Docker client 28.0.0，Docker server 27.4.0。本地已通过定向 workflow 测试、ESLint、Prettier、actionlint 和 Bash 语法检查。完整 self-hosted Linux E2E workflow 已在 ECS runner 池中通过。
风险与范围
docker rm -f，但目标只限于与当前 workflow run、attempt 和 Docker shard 唯一对应的精确 owner 标签；并发任务使用不同 owner。关联 Issue
相关：#8816、#10971、#11001