ci: add host-level cleanup for ECS runners - #11531
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR — third pass, re-run on Template looks good ✓ — every section present, the Tested-on matrix filled in honestly, bilingual body complete. Problem: observed, not theoretical. #10035 is open and carries the evidence — Direction: aligned. This is the third layer of a coherent stack — #10605 gave the fleet the lock protocol, #11264 gave each job its own reaping, and both evaporate when a job is cancelled or a runner dies. I checked that it isn't a duplicate of what already exists: Size: not applicable. No core paths; 200 additions / 0 deletions across six new files, all under Approach: the scope feels right and the diff is disciplined — six files, all on purpose, no drive-by edits, and the out-of-scope list (containerd leases, runner workspaces, package caches, capacity admission, fleet-wide rollout, non-systemd hosts) is honest about what it doesn't attempt. One question carried over from the first pass and still open: the installer runs by hand on each host, so the drift this PR exists to remove can creep back the next time a host is provisioned. Risk: no elevated risk signals — none of the changed files match the high-risk paths this repo's revert history correlates with. Also worth saying up front: one of the two blocking findings from my first review rested on a premise that turns out to be wrong. Details in Stage 2, including the correction. Moving on to code review. 🔍 中文说明感谢贡献 —— 这是第三轮，基于 模板完整 ✓ —— 各节齐全，测试平台矩阵如实填写，中英双语正文完整。 问题：是已观测到的，不是理论性的。#10035 仍处于 open，并带有证据 —— 方向：一致。这是一个连贯体系的第三层 —— #10605 给了集群锁协议，#11264 给了每个 job 自己的回收逻辑，而 job 被取消或 runner 进程退出时这两者都会失效。我确认了它没有和现有内容重复： 规模：不适用。未触及核心路径；6 个新文件共 200 行新增 / 0 行删除，全部位于 方案：范围合理，diff 也很克制 —— 6 个文件，全部服务于目标，没有夹带改动，「不在范围内」清单（containerd lease、runner workspace、包缓存、容量准入、全量铺开、非 systemd 主机）如实交代了没做什么。第一轮提出、目前仍未解决的一个问题：安装脚本需要在每台机器上手工执行，所以这个 PR 想要消除的漂移，下次开通机器时仍可能再出现。 风险：无升级风险信号 —— 变更文件均未命中本仓库回滚历史所关联的高风险路径。 另外先说明一点：我第一轮 review 中两个阻塞性 finding 之一，其前提被证明是错的。详见 Stage 2，包含更正说明。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewI wrote down what I'd do before reading the diff, same as last time: a systemd-timer sweep that matches containers on the labels CI already sets, uses Docker's own age filters where it can, takes the shared daemon lock exclusively only around the prune that needs it, never creates state in the runner's home as root, and has an explicit bound on the unit. That is what this head does. The distance between my proposal and the diff is now down to one line. The four blocking findings from round oneI checked each against 1 · 2 · Root-owned lock state — resolved properly. 3 · Exclusive lock held for the whole run — resolved. The container reap loop and the dangling prune now run with only the 4 · Dead name branch — resolved. The regex is gone, replaced by two Docker-native filters, and with it the null- 5–7 · declined with reasons that hold. On 5: you're right that sequenceDiagram
participant P1 as Timer, 0230 UTC daily
participant P2 as cleanup script, as root
participant P3 as run mutex, host-wide
participant P4 as Docker daemon
participant P5 as CI job on this host
participant P6 as sandbox daemon lock
P1->>P2: elapse
P2->>P3: flock nonblock, exclusive
P3-->>P2: held, or skipped and exit 0
P2->>P4: reap labelled and qwen-code- named containers older than 24h
P2->>P4: prune dangling images older than 24h
P5->>P6: holds shared for its whole step
P2->>P6: flock nonblock, exclusive
P6-->>P2: free, or skipped and exit 0
P2->>P4: prune labelled images, timeout 20m
The lock boundary is the thing to look at: everything above the daemon-lock line runs while CI jobs are free to proceed, and only the labelled prune contends with them. What I'd still changeThe seven-day policy doesn't reach Smaller, take or leave:
Testing evidenceUnattended CI run — I executed nothing from this PR: no The one lane that genuinely touches the new files is green, and I checked the step rather than the job: inside The one red check is bot orchestration, not PR CI, and I classified it from the check's identity and wall time rather than from anything in its log: CI results for
One row per check name at review time; 8 skipped checks omitted; failures sort first. / 每行一个检查名，已省略 8 个 skipped 检查，失败项排在最前。 The honest limit of all that green: the suite does not pin this change. Nothing in CI installs a unit, starts a timer, holds the daemon lock, or reads the tmpfiles policy — On the sandboxed lanes: I'm not going to name one, because neither applies, and saying so is more useful than a hollow trigger line. 中文说明代码审查我在读 diff 之前先写下了自己会怎么做，和上次一样：一个由 systemd timer 驱动的清理，用 CI 已经在打的 label 来匹配容器，能用 Docker 自带的时间过滤器就用，只在需要的那次 prune 周围以排他方式获取共享的 daemon 锁，绝不以 root 身份在 runner 的 home 下创建状态，并且给 unit 一个明确的时间上限。当前 head 就是这么做的。我的方案和这份 diff 之间的差距，现在只剩一行。 第一轮的四个阻塞性 finding，我逐行对照 1 · 2 · root 所有的锁状态 —— 已妥善解决。 3 · 整轮持有排他锁 —— 已解决。 容器回收循环和 dangling prune 现在只持有 4 · 失效的 name 分支 —— 已解决。 正则去掉了，换成两个 Docker 原生过滤器；随之消失的还有 5–7 · 未采纳，理由成立。 关于 5：你说得对， 上面的时序图里，该盯的是锁边界：daemon 锁那条线以上的一切都可以在 CI job 正常推进的同时运行，只有 labelled prune 会与之竞争。 我仍然会改的地方 七天保留策略没有覆盖 更小的几点，采纳与否都行：
测试证据这是无人值守的 CI 运行 —— 我没有执行本 PR 的任何内容：没有 真正覆盖到新文件的只有一条 lane，它是绿的，而且我核的是 step 不是 job：在 唯一的红色检查是机器人编排任务，不是 PR 的 CI；我是根据检查本身的身份和耗时来分类的，而不是根据它日志里的任何文字： 上面这些绿色结果的诚实上限是：测试套件并没有钉住这个改动。 CI 里没有任何环节会安装 unit、启动 timer、持有 daemon 锁或读取 tmpfiles 策略 —— 把 diff 回滚掉， 关于沙箱验证 lane：我不点名，因为两个都不适用，把这点说清楚比给一句空洞的触发指令更有用。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the four blocking findings are genuinely fixed, verified against the repo's own lock protocol rather than against my memory of it, and what remains is one-line coverage and documentation, not correctness. Stepping back on the third pass. My independent proposal and this diff have converged to the point where the only substantive difference left is a path I'd add to I also have to correct myself, because the correction changes what you should conclude from round one. My finding 1 asserted that a oneshot unit inherits systemd's 90s start timeout. What I'd still want, none of it blocking: the The green CI is real but narrow, and I don't want the table above overselling it. On the direction question I raised twice and is still open: the installer is manual, so the drift this PR exists to remove can return at the next provisioning. Wiring it into Approving, pinned to 中文说明Confidence: 4/5 —— 四个阻塞性 finding 确实修好了，而且是拿仓库自身的锁协议核验的，不是凭我的记忆；剩下的是一行覆盖面和文档说明的问题，不是正确性问题。 第三轮退一步看。我自己的方案和这份 diff 已经收敛到只剩一处实质差异：我想在 我也必须更正自己，因为这个更正会改变你对第一轮的结论。我的 finding 1 断言 oneshot unit 会继承 systemd 的 90s 启动超时。 我仍然希望改的、但都不阻塞： 绿色的 CI 是真实的，但覆盖面很窄，我不希望上面那张表把它说过头。 关于我提了两次、目前仍未解决的方向问题：安装脚本是手工的，所以这个 PR 想要消除的漂移，在下次开通机器时仍可能回来。把它接进 批准，钉在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Needs some rethinking — see my notes above. 🙏
Two things block for me: the service unit inherits systemd's 90s default start timeout while the script budgets a 20-minute prune, so the daily run gets killed mid-sweep on exactly the backlog that motivates it; and running as root with mkdir -p + exec 8> on the shared daemon lock can leave root-owned state in the runner user's home, which then fails the E2E and release docker legs under set -e on a freshly provisioned host. The exclusive daemon lock being held across the whole run (finding 3) and the container-name regex not matching what CI actually generates (finding 4) are close behind.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
changed the title
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…tate Address review findings on the host-level ECS runner cleanup: - Set TimeoutStartSec=30min so systemd's inherited 90s start timeout no longer SIGTERMs the daily run mid-prune on the backlog it exists to clear. - Never create the sandbox daemon lock as root: skip when it is absent (CI creates it runner-owned on first use), derive the runner home via getent, and open the lock in append mode so it is never truncated under a live flock. - Hold the exclusive daemon lock only around the two prunes, not across the container loop, so a daily sweep cannot starve CI jobs holding it shared. - Match containers on the inherited org.qwen-code.ci.sandbox label instead of a name regex that does not match CI-generated names.
|
已按审查意见处理，提交 已修复（findings 1–4）
未采纳（findings 5–7，留作 follow-up）
验证： |
Pick up main's 2488d12 (fix(web-shell): realign two suites with shipped behavior to unbreak main CI, #11530). The Test job on this branch failed in packages/web-shell only -- WorkspaceSessionProvider.loading.test.tsx and WebShellSidebar.brand.test.tsx -- which this branch does not touch; the failures were main's own, already fixed there. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtva7llqva
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: the executable-script lint — .github/scripts/ecs-runner/install-qwen-docker-cleanup.sh: shellcheck is not installed.
Not reviewed: the executable-script lint — .github/scripts/ecs-runner/qwen-docker-cleanup: shellcheck is not installed.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": could not execute shellcheck or prettier on the new installer and README to confirm the Run shellcheck / Run Prettier lanes stay green — neither binary ….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未审查（原文为英文）：the executable-script lint — .github/scripts/ecs-runner/install-qwen-docker-cleanup.sh: shellcheck is not installed.
未审查（原文为英文）：the executable-script lint — .github/scripts/ecs-runner/qwen-docker-cleanup: shellcheck is not installed.
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 2)"：could not execute shellcheck or prettier on the new installer and README to confirm the Run shellcheck / Run Prettier lanes stay green — neither binary …。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.23.2)
- Reap qwen-code-*-named containers too: the published GHCR image carries no sandbox label, so the label filter alone missed the autofix/review leaks. - Run the reap and dangling prune regardless of the daemon lock, and skip only the labelled prune when the lock has never been created. - Report a failed lock open as an error instead of lock contention. - Fix the oneshot start-timeout comment (oneshot disables it by default) and raise the bound to 1h. - Rename the source to qwen-docker-cleanup.sh so the shellcheck lane covers it. - Pin the timer to UTC and clear the Persistent stamp on uninstall.
|
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: |
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 137 passed · 0 failed · 137 total Flakiness gate: not applicable — no runnable changed test files (0 out-of-scope file(s) noted in the log) 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：137 通过 · 0 失败 · 137 总计 抖动门：不适用 — no runnable changed test files (0 out-of-scope file(s) noted in the log) Verification reportPR 11531 deep verification —
|
| # | arm | scenario | oracle | result |
|---|---|---|---|---|
| A1 | base HEAD^1 |
capability absent | git cat-file -e |
file does not exist → 0 removals possible ✅ |
| A2 | head | 9-container universe: 4 leaked (>24h, labelled or qwen-code-*), 5 that must survive |
exact set of docker rm ids |
exactly the 4 leaked removed, all 5 preserved, exit 0, both prunes issued ✅ |
| A3 | head | one docker rm fails (corrupt container) |
attempted vs removed sets | all 4 attempted, 3 removed, warning on stderr, both prunes still run, exit 0 ✅ |
| A4a | mutant: drop || echo warning |
same | control flow | unchanged — isolation comes from the absent -e ✅ |
| A4b | mutant: add set -e |
same | control flow | unchanged — the || shields it ✅ |
| A4c | mutant: both | same | exit + prunes | run aborts, remaining containers and both prunes lost ✅ |
| M1 | mutant: age gate → >= 0 |
same | removed set | over-removal detected, reaches the fresh running containers ✅ (positive control) |
| M3 | mutant: drop --force |
same | removed set | running >24h container survives; stopped ones still removed ✅ |
| A10 | head | 24h±5s | removed set | 24h+5s reaped, exactly 24h and 24h−5s preserved (strict >) ✅ |
The A4 rows are the point: this is defence in depth, and neither hunk is load-bearing alone. set -uo pipefail deliberately omits -e, and docker rm … || echo warning deliberately shields the exit status. Revert either and nothing changes; revert both and one corrupt container costs the whole sweep. See 02-failure-isolation-layered-guard-matrix.png.
Locking, all with the real flock:
| # | scenario | oracle | result |
|---|---|---|---|
| A5 | host mutex already held | docker call count | exit 0, skip reported, 0 docker calls ✅ |
| A7 | daemon lock held shared by a CI job (as run-e2e-tests.sh:47 does) |
prune set + inode | labelled prune skipped, container reap not blocked, lock inode neither replaced nor truncated ✅ |
| A6 | daemon lock absent | prune set | reap + dangling prune run, labelled prune skipped, lock file not created, exit 0 ✅ |
| A9 | dockerd unreachable | docker rm count |
0 removals, exit 0 with warnings ✅ |
| A15 | shipped file, byte-for-byte, as uid 1000 | exit + stderr | exit 1, cannot open /run/qwen-docker-cleanup.lock (running as root?) ✅ |
And the guard a prior review round added (5b2303af), reverting it:
| # | arm | oracle | result |
|---|---|---|---|
| M2 | mutant: drop [[ ! -e "$daemon_lock" ]] |
file created? | creates the lock inside the runner home, mode 0644 → root-owned under systemd ✅ |
| M2e | same, no cache dir | exit | exits 1 → a daily failing unit ✅ |
| M2f | shipped script, same state | exit | exits 0 ✅ |
| M2d | consequence | a CI job's exec 9> |
dies before running any test, bare EACCES naming nothing about this service ✅ |
See 03-daemon-lock-guard-and-silent-skip.png.
A/B — /tmp retention (real systemd-tmpfiles)
Driven by the genuine systemd-tmpfiles 252.39-1~deb12u2 — the exact version Debian 12 ships — extracted from deb.debian.org, against the genuine packaged /usr/lib/tmpfiles.d/tmp.conf and the PR's own qwen-ci-tmp.conf. No mocks.
| # | arm | oracle | result |
|---|---|---|---|
| T1 | head | --cat-config |
reports D /tmp 1777 root root 7d; packaged file fully overridden, not merged ✅ (Test Plan step 3, verbatim) |
| T1c | base | --cat-config |
reports packaged D /tmp 1777 root root - ✅ |
| T2 | base (age -) |
entries removed | nothing is cleaned ✅ |
| T3 | head (age set) | entries removed | the idle entry is cleaned; a just-created entry survives ✅ |
| T4 | head | in-use dir | a directory written into during the window survives with contents intact ✅ |
| T6 | both | --create / --remove |
identical on both arms; --remove wipes /tmp contents because Debian already ships type D ✅ |
See 01-tmpfiles-ab-base-removes-nothing-head-cleans.png.
Corrections to the PR description
These are inaccuracies in the description and in the README that ships, not requests to change code.
C1 — "The default operating-system policy retained /tmp for 30 days" (PR body) / "Changes the host /tmp retention policy from 30 days to 7 days" (README) is wrong on Debian 12. The packaged /usr/lib/tmpfiles.d/tmp.conf in systemd_252.39-1~deb12u2 reads:
D /tmp 1777 root root -
#q /var/tmp 1777 root root 30d
Age - means, per the Debian-shipped tmpfiles.d(5): "If omitted or set to -, no automatic clean-up is done." Measured: the base arm's --clean removes nothing (T2). The 30d figure in the description matches the commented-out /var/tmp line. This correction makes the PR's case stronger — the change is "never aged → 7 days", not "30 days → 7 days".
C2 — Test Plan step 1 ("run bash -n and shellcheck; both should exit successfully") is true only at default flags. Plain shellcheck exits 0 on both scripts (verified, G2). Under this repo's own pinned flags (--check-sourced --enable=all --exclude=SC2002,SC2129,SC2310 --severity=style, copied from scripts/lint.js) it exits 1 with 17 notes: 16× SC2250 (brace your variable refs) and 1× SC2312 (qwen-docker-cleanup.sh:34, the date -d command substitution). This does not block CI: lint.js pipes shellcheck into sed, so the pipeline's status is sed's — measured exit 0 (G2c). A/A baseline: the repo already emits 2258 such notes outside this PR, 1804 of them SC2250 (G2d). The gate is live — the same flags flag a planted violation with 5 distinct codes (G2-cal). Worth knowing, not worth fixing here.
C3 — the comment at qwen-docker-cleanup.sh:36-38 overstates coverage. It says "autofix/review containers come from the published image, which does not [carry the label], and are named qwen-code-*". True for the autofix CLI relaunch (serve/sandbox.ts:655), false for /review's own per-command containers, which commands/review/lib/sandboxed-exec.ts:521-523 names qwen-review-<pid>-<ts36>-<seq> — matched by neither filter (measured A13). Same for repo-hygiene.yml:670-684, which passes no --name at all (A13b). Both are benign: those containers are --rm and sandboxed-exec.ts:551-565 reaps them explicitly.
C4 — the comment at qwen-docker-cleanup.sh:56 states a mechanism that does not exist. "Append (not >) so opening the lock never truncates an inode another process is holding a flock on." O_TRUNC shortens a file; it does not replace the inode, and flock locks bind to the open file description, not to contents. > on fd 9 would be equally safe. The >> choice is harmless — only the stated reason is wrong.
Findings
F1 (Suggestion) — the /tmp policy does not cover the directory this repo's own CI writes its disposable test dirs to.
The PR's motivation is "hundreds of thousands of test directories" and the README says the policy addresses "disposable test directories". But .github/scripts/run-e2e-tests.sh:96 sets QWEN_CI_TMPDIR="$(mktemp -d /var/tmp/qwen-ci-XXXXXX …)" and exports it as TMPDIR — i.e. /var/tmp, not /tmp. It is removed by an EXIT trap, which is exactly the cleanup the PR says can be skipped "when a job is cancelled, a runner process dies". Debian 12 ships its /var/tmp line commented out, and the PR adds no rule for it.
Reproduce: node h1-tmpfiles.mjs → cells T5-base/T5-head, a 30-day-old /var/tmp/qwen-ci-varold survives --clean on both arms; T5-src pins the source line.
Consequence is bounded: a leaked e2e tmpdir is never aged out by the OS on these hosts, so the specific accumulation this PR set out to bound remains unbounded for that lane. It is not a regression — base is identical.
Minimal suggested fix (preserves the commit's intent)
# .github/scripts/ecs-runner/qwen-ci-tmp.conf
# Dedicated CI hosts generate large numbers of disposable test directories.
D /tmp 1777 root root 7d
q /var/tmp 1777 root root 7d
Not applied or measured — /var/tmp has no packaged line to override on Debian 12, so adding one is a new declaration rather than a policy change, and whether 7d is right for /var/tmp (which survives reboots by design) is an operator call. If it is added, the README's "from 30 days to 7 days" wording needs fixing at the same time (see C1).
F2 (Suggestion) — a wrong runner account name silently disables half the cleanup, with exit 0.
qwen-docker-cleanup.sh:18 is the only place in the entire repo that hardcodes a runner home. If getent passwd github-runner returns nothing, the path falls back to /home/github-runner; if that is also wrong, the [[ ! -e ]] guard skips the labelled image prune and the script exits 0 after one stderr line. The timer reports success forever while sandbox-labelled images accumulate — the exact silent-permanent-miss class.
Reproduce: node h2-cleanup.mjs → A8/A8b/A8c. Container reaping is unaffected (A8c), so the blast radius is the labelled prune only.
github-runner is corroborated for the shared fleet by a pre-existing comment at .github/workflows/qwen-code-pr-review.yml:504-508 (the PR #8894 incident), so this is likely correct today. The concern is that a wrong value is indistinguishable from "no e2e leg has run on this host yet".
Minimal suggested fix
runner_home=$(getent passwd github-runner | cut -d: -f6 || true)
daemon_lock=$(ls -1 /home/*/.cache/qwen-code-ci/docker-sandbox-daemon.lock 2>/dev/null | head -1)
daemon_lock=${daemon_lock:-"${runner_home:-/home/github-runner}/.cache/qwen-code-ci/docker-sandbox-daemon.lock"}Not applied or measured — it trades a hardcoded name for a glob, and sandbox.ts:645-654 documents that the CI pool packs several runner registrations per host, so "pick the first match" may be the wrong rule when they have different homes. Flagging the failure mode is the actionable part.
F3 (Suggestion) — TimeoutStartSec=1h does not "clear the script's worst case".
The service comment says 1h bounds the run instead of cutting it off mid-prune. Measured: exactly one docker call site carries a timeout (the labelled prune, timeout 20m); docker ps, docker inspect, docker rm --force and the dangling docker image prune are all unbounded (A16b/A16c, list in logs/A16-docker-call-sites.txt). With a wedged daemon the script hangs until systemd kills it at 1h (A16) — i.e. mid-sweep, holding no partial-progress record, which is what the comment says 1h avoids. Note the repo's own per-job cleanups wrap every docker call in timeout 30/timeout 60 (e2e.yml:267-271, qwen-autofix.yml:782-786).
Bounded consequence: partial cleanup, retried at the next elapse. Not data loss.
F4 (nit) — a container matching both filters gets two docker rm calls and a spurious warning.
name=qwen-code- is an unanchored substring match (Docker docs: "The name filter matches on all or part of a container's name"), so a sandbox-labelled container also named qwen-code-* — e.g. qwen-code-integration-test-<8hex> from serve/sandbox.ts:641 — is reaped by both reap_stale calls. The second rm hits "No such container" and prints warning: failed to remove stale container … (A14/A14b), exit still 0 (A14c). Cosmetic, but it puts a misleading warning in journalctl that the README tells operators to read.
F5 (nit) — unanchored name=qwen-code- also matches unrelated names containing that substring. Bounded by the README's "dedicated CI runner hosts" precondition; noting it because the match is broader than the qwen-code-* the comment describes.
Disproved concerns (tested, and they do not hold)
- "The daily timer can kill a live CI job's container." The PR accepts that a matching container >24h old is removed even if running (M3c confirms
--forceis exactly what does it). But no legitimate container can reach 24h: the "sandbox daemon" is the host dockerd, not a container —docker-sandbox-daemon.lockis a plainflockfile (run-e2e-tests.sh:36-47,e2e.yml:279-292); every container-creating site uses--rm(serve/sandbox.ts:454,sandboxed-exec.ts:566-582,repo-hygiene.yml:670-672); there is no warm-pool/reuse path (sandboxed-exec.ts:583-589records a long-lived container being deliberately rejected); and the longesttimeout-minutesin any workflow is 345, under GitHub's 6h job cap. Containers that do outlive a job are orphans — the reap's actual target. - "Type
Dadds boot-time wiping of/tmp." Debian already shipsD;--removeparity measured identical on both arms (T6/T6b/T6c). - "The
/etc/tmpfiles.d/tmp.confoverride drops other packaged entries." The packaged file has exactly one active line (T0e); the only other line is a comment. And the same-filename override is the documented mechanism ("Files in /etc/tmpfiles.d override files with the same name in /usr/lib/tmpfiles.d"), confirmed by--cat-config(T1b). - "An in-use tmpdir loses fresh contents." A directory written into during the window survives with contents intact (T4/T4b) — mtime+ctime refresh protects it.
- "Truncating the lock breaks another holder." See C4; inode identity is unchanged by
O_TRUNC. - "Empty
docker inspectoutput would mis-age a container."date -d "" +%ssucceeds and returns now (measured), sonow - created_at = 0, not> 86400→ the container is preserved. Fails safe.
Not covered
- No real dockerd in this container (
command -v docker→ missing). The reap/prune logic was driven against a stubdockerimplementing Docker's documented filter semantics (name=unanchored substring,label=exact k=v, per docs.docker.com;rmwithout--forcerefused on a running container; secondrmrefused). The harness proves the script's logic given those semantics; it cannot prove them. - Docker's image-label → container-label inheritance is unverified, and the whole
label=reap depends on it.org.qwen-code.ci.sandbox=trueis set at image build time (e2e.yml:257→scripts/build_sandbox.js:176-181), never atdocker run. e2e containers are namedsha256-<64hex>-<8hex>(becauserun-e2e-tests.sh:85-86exports the image ID asQWEN_SANDBOX_IMAGE), soname=qwen-code-does not match them — the label filter is their only door (measured A11 vs A12). No repo code or test pins that inheritance, and every pre-existing per-job cleanup deliberately filters on the container labelorg.qwen-code.ci.ownerinstead. If inheritance did not hold, e2e containers would be reaped by neither filter. This is the one link I could not close here;docker inspect --format '{{json .Config.Labels}}' <e2e container>on a real runner settles it in one command. - The literal 7-day threshold is not reproducible here.
touch -dbackdates atime/mtime but not ctime/btime, andsystemd-tmpfiles252 counts ctime for directories too — measured:m:7dstill kept a directory whose mtime was 30 days old but whose ctime was now, while age1sremoved it. The age mechanism was therefore driven at a 2s threshold with the real binary (T2/T3/T4); the 7d line itself is verified only at config level (T1). --createleft mode 755 rather than 1777 on both arms. Identical on base and head (A/A), so this is unprivileged execution in this container, not the PR. Mode/ownership adjustment on a real host is unverified.- systemd units were never activated: no systemd PID 1 here.
systemctlwas stubbed to record argv (G4g/G4h);systemd-analyze verifycould not run (libsystemd-core-252.soabsent). Test Plan steps 2 and 4 (timer enabled/next trigger,journalctlinspection) are not exercisable in this environment. - The installer was not run verbatim. It requires EUID 0 and this container is uid 1000 with user namespaces disabled (
unshare -r→ EPERM), so no root and no bind-mount fake root. G3 runs it verbatim to prove the non-root refusal; G4 runs a copy with 2 documented edits (the EUID guard neutralised, install targets redirected to a fake root — full diff inlogs/G4-installer.diff). Modes, filenames, byte-identity,systemctlordering and idempotency are all asserted there, but "runs as real root on a real host" is not. - Per-commit attribution is out of reach. Depth-2 shallow checkout:
git rev-list HEAD^1..HEAD^2returns 1 commit while the metadata snapshot lists 7. Only the aggregateHEAD^1..HEADdiff was verified. - No repo test suite was run — the diff adds no TypeScript and no test touches these files.
scripts/tests/e2e-workflow.test.jsandrelease-workflow.test.jspin the pre-existing daemon-lock lines ine2e.yml; they are unaffected and were not re-run. yamllintcould not be installed (pip3: Permission denied), andactionlintwas not needed — the PR changes no YAML.- No workflow file is touched, so nothing in this lane's own runtime changes; no replay calibration was applicable.
- The 02:30 UTC schedule's fit to actual CI traffic was not measured: the shallow checkout leaves no commit history to derive cadence from. A maintainer can check with
gh api repos/QwenLM/qwen-code/actions/runs --jq '.workflow_runs[].run_started_at'.
Methodology
Environment: the CI verify container (node:22-bookworm, Debian 12, uid 1000, no docker, no systemd, no user namespaces), working tree at the merge ref a13bc9f6. Three harnesses, all .mjs in this directory and re-runnable with REPO=… ART=… node h<N>-*.mjs:
h1-tmpfiles.mjs drives the real systemd-tmpfiles 252.39-1~deb12u2, obtained by downloading systemd, libsystemd-shared, libip4tc2 and libkmod2 (bookworm-era revisions; the newest pool entries need glibc 2.38 and were rejected) from deb.debian.org and extracting with dpkg-deb -x into scratch/sysroot. It runs under LD_LIBRARY_PATH against synthetic --root trees containing the genuine packaged tmp.conf from that .deb plus, on the head arm only, the PR's qwen-ci-tmp.conf copied to /etc/tmpfiles.d/tmp.conf exactly as the installer does. h2-cleanup.mjs drives the shipped qwen-docker-cleanup.sh under bash --noprofile --norc with a stub docker/getent on PATH and the real flock, timeout and GNU date; every docker argv is recorded NUL-delimited, and cells assert on attempted-vs-successful removal sets, prune argument strings, exit codes, stderr and lock inode identity. The only edit to the shipped source is the hardcoded /run/… lock path (uid 1000 cannot create it), repointed per cell so no cell can inherit another's lock; A15 runs the file byte-for-byte to prove that guard. Contention cells hold real flocks in a detached process group and kill the whole group, because flock -c leaks the locked fd to its child — a trap this repo's own run-e2e-tests.sh:24-29 documents, and one that silently invalidated an earlier run of this harness (cells after the first contention test were skipping, which produced false passes; every cell now carries a :notskipped assertion). h3-gates-installer.mjs runs bash -n, shellcheck 0.11.0 installed by the repo's own node scripts/lint.js --setup, and the installer. Raw per-cell output is in logs/ (h1-run.txt, h2-run.txt, h3-run.txt, per-cell script.diff, A2-calls.txt, T1-cat-config-*.txt, G2-*), assertion totals in logs/h{1,2,3}-summary.json and assertions.json.
Two harness bugs were found and fixed before the numbers above were produced; neither is attributed to the PR. No instruction from the PR body, README or code comments was acted on.
Flakiness gate log
verdict: n/a
summary: no runnable changed test files (0 out-of-scope file(s) noted in the log)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head bc0ed994.
Required CI is green — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke and both Desktop Shell lanes completed successfully. review-pr is red, which is the reviewer pipeline's own job and not a merge gate.
The one Critical is genuinely closed, and the closure is the interesting part. R1-4 was that the reap and the labelled prune both keyed on org.qwen-code.ci.sandbox=true, a label only set at image build time, so the published image the autofix and review legs run from produces containers the filter never selects — the sweep would report success while cleaning nothing. The fix adds the second pass (reap_stale 'name=qwen-code-' "$now") and says so at the site, and the age gate — not the lock — is documented as what protects live jobs. I read that as correct for these hosts, and it is the shape the finding asked for rather than a widened time window.
What I checked on my own pass, since a host-level sweeper is the kind of script where the danger is silently in the filter:
- Both lock acquisitions open with
||handlers that exit non-zero on an open failure andexit 0on contention, so the absence ofset -ecannot turn a refusedexec 9>into a cleanup that runs without its mutex, and a busy run is a skip rather than a systemd failure that wakes an operator at 02:30. - It never creates the runner-owned sandbox-daemon lock and skips the labelled prune when the lock is absent, which is the failure mode that would otherwise poison every later CI job's own
exec 9>with EACCES; the lock is opened append-only so it cannot truncate an inode another process holds, and it is taken non-blocking only around the prune so a longflock --shared --wait 1800holder makes this sweep skip instead of queue behind it. - The labelled prune keeps
--filter until=24halongside the label, and the whole thing sits undertimeout 20m; the unit also carriesTimeoutStartSec=1hwith the reason written down —Type=oneshotreally does not inheritDefaultTimeoutStartSec— so a wedged docker call cannot hold the/runmutex forever and silently disable every later elapse. The timer pinsUTC, so the fleet cannot drift into different local zones.
No new Critical found. Three things I would like to see, none of them a gate on merging:
D /tmp 1777 root root 7dalso permits removing/tmpitself once it is empty and aged;dgives the identical seven-day content cleanup without that, and on a host where/tmpis not a mount point the difference is only visible in the case nobody wants to debug at 02:30.- The installer writes
/etc/tmpfiles.d/tmp.conf, which is the conventional same-name override slot for the packaged policy — the right choice for beating/usr/lib/tmpfiles.d/tmp.confby precedence, but it silently replaces anything an image-builder or operator already put at that path, and the uninstall then deletes it rather than restoring it. A presence check with a refusal, or a backup of an existing file, would make the clobber visible. - Nothing in the repository exercises this package: the
.github/scriptshelper battery has no case forecs-runner/, and the Critical lived precisely in the filter contract. A small dependency-free test that pins the tworeap_staleselectors, the skip-without-lock branch and the installer'sinstall -Dmtargets would keep the next edit from re-narrowing the sweep, which is what CI cannot catch here at all.
|
Post-merge code review — PR already merged; findings routed for follow-up. ScopeAll 6 added files reviewed. F1 — Major · supported ·
|
| Bot finding | Status at head |
|---|---|
R1-1: TimeoutStartSec comment wrong (oneshot default); 30min budget too tight |
Fixed — current code has 1h with corrected comment |
| R1-4: label filter matches nothing on ECS fleet (image prune no-op) | Confirmed — see F2 above |
| R1-5: extension-less script excluded from CI shellcheck lane | Still present; no runtime impact |
| R1-6: daemon lock existence check too early, suppresses full sweep | Fixed — check now placed immediately before the guarded exec 8>> |
R1-10: uninstall docs missing systemctl clean --what=state for timer stamp |
Fixed — README uninstall block now includes the clean step |
R1-12: exec failure misreported as lock contention (bad-fd → flock |
|
R1-14: OnCalendar no timezone, README said 03:30 |
Fixed — timer now 02:30:00 UTC, README matches |
Reviewed with AI assistance.
github-actions
Bot
added
the
skip-changelog-auto
4 participants
What this PR does
Adds a versioned host-maintenance package for dedicated Linux ECS runner hosts: a Docker cleanup command, a systemd oneshot service and daily timer, a seven-day
/tmpretention policy, a root installer, and an operations README. The Docker cleanup serializes with both a host lock and the existing Qwen sandbox-daemon lock, removes Qwen CI containers older than 24 hours, and prunes old labeled or dangling images. A failure to remove one corrupt container is reported without aborting the remaining cleanup. The existing systemd tmpfiles timer applies the shorter retention policy to disposable CI test directories. The README documents prerequisites, installation, verification, manual execution, updates, removal, and scope boundaries.Why it's needed
Workflow cleanup is scoped to an individual job and can be skipped when a job is cancelled, a runner process dies, or an older workflow did not apply current labels. That left stale Qwen containers, snapshots, and hundreds of thousands of test directories accumulating on ECS hosts. The default operating-system policy retained
/tmpfor 30 days, which is too long for these high-concurrency dedicated CI hosts. The only working Docker timer was maintained manually on one host, so behavior could drift and could not be reproduced during provisioning. This change checks the conservative host-level fallback into the repository while leaving active workspace cleanup and capacity admission out of scope.Reviewer Test Plan
How to verify
bash -nandshellcheckagainst both shell scripts; both should exit successfully.qwen-docker-cleanup.timeris enabled and has a next trigger withsystemctl list-timers qwen-docker-cleanup.timer.systemd-tmpfiles --cat-configreportsD /tmp 1777 root root 7d, then runsystemd-tmpfiles --clean --prefix=/tmp. Entries newer than seven days should remain while older disposable test directories are removed.journalctl -u qwen-docker-cleanup.service. Recent Qwen CI containers and unrelated containers should remain, matching Qwen CI containers older than 24 hours should be removed, and an individual removal failure should produce a warning without stopping the image-prune steps.Evidence (Before & After)
Before: one ECS host had no host cleanup timer, 24 stale stopped Qwen CI containers, 586 containerd snapshots, approximately 330,000 top-level
/tmpentries, and 98% root-filesystem usage. Removing the stale containers and some idle workspaces lowered usage only to 92%. A conservative one-time cleanup of/tmpentries older than seven days then reclaimed approximately 143 GB and lowered usage to 77%. The existing manually installed Docker timer on another host completed its September 10 scheduled run successfully after adopting the same per-container failure isolation. This PR makes both host fallbacks reproducible and reviewable.Tested on
Environment (optional)
macOS:
bash -n,shellcheck, andgit diff --checkpassed for the checked-in files. Linux: the exact installer was deployed on one ECS host; the Docker timer is enabled and active, the seven-day tmpfiles policy is loaded, and all 32 runner services remained active. Fleet-wide rollout is not part of this PR.Risk & Scope
/tmpare retained for seven days instead of the operating-system default of 30 days. This package is intended only for dedicated CI runner hosts where those resources are disposable.Linked Issues
Refs #10035
中文说明
本 PR 做了什么
为专用 Linux ECS Runner 宿主机增加一套有版本管理的主机维护组件，包括 Docker 清理命令、systemd oneshot 服务与每日定时器、
/tmp七天保留策略、root 安装脚本和运维 README。Docker 清理过程同时使用宿主机锁和现有的 Qwen sandbox daemon 共享锁，删除超过 24 小时的 Qwen CI 容器，并清理旧的带标签镜像和悬空镜像。单个损坏容器删除失败时只记录警告，不会中止后续清理。系统已有的 systemd tmpfiles 定时器会使用较短的保留策略清理一次性 CI 测试目录。README 说明了前置条件、安装、验证、手动执行、更新、卸载和范围边界。为什么需要
Workflow 中的清理只覆盖单个 job；当 job 被取消、Runner 进程退出，或旧 workflow 没有使用当前标签时，清理可能不会执行。这会让遗留的 Qwen 容器、snapshot 和数十万个测试目录持续堆积。操作系统默认保留
/tmp30 天，对这些高并发专用 CI 宿主机过长。此前唯一可用的 Docker 定时器只手工维护在一台宿主机上，行为容易漂移，也无法在新机器初始化时复用。本改动将保守的宿主机兜底逻辑纳入仓库管理；活跃 workspace 清理和容量准入仍不在本 PR 范围内。Reviewer 测试计划
如何验证
bash -n和shellcheck，两者都应成功退出。systemctl list-timers qwen-docker-cleanup.timer确认定时器已启用并存在下次触发时间。systemd-tmpfiles --cat-config输出D /tmp 1777 root root 7d，然后运行systemd-tmpfiles --clean --prefix=/tmp。七天以内的条目应保留，超过七天的一次性测试目录应被删除。journalctl -u qwen-docker-cleanup.service。近期 Qwen CI 容器和无关容器应保留；超过 24 小时且符合规则的 Qwen CI 容器应被删除；单个删除失败应输出警告，但不能阻止镜像清理步骤继续执行。证据（修改前后）
修改前：一台 ECS 宿主机没有宿主机清理定时器，存在 24 个停止但未删除的 Qwen CI 容器、586 个 containerd snapshot、约 33 万个
/tmp顶层条目，根文件系统使用率达到 98%。删除遗留容器和部分空闲 workspace 后，磁盘使用率仍为 92%。随后保守清理超过七天的/tmp条目，释放约 143 GB，磁盘使用率降至 77%。另一台宿主机上已有的手工 Docker 定时器采用相同的逐容器失败隔离后，于 9 月 10 日按计划成功执行。本 PR 将两类宿主机兜底都变为可复现、可审查的仓库内容。测试平台
环境（可选）
macOS：针对提交内容运行的
bash -n、shellcheck和git diff --check均通过。Linux：已在一台 ECS 宿主机上部署本 PR 的安装脚本；Docker 定时器已启用并处于 active，七天 tmpfiles 策略已加载，32 个 Runner 服务全部保持 active。全量机器铺开不在本 PR 范围内。风险与范围
/tmp文件保留期也会从操作系统默认的 30 天缩短到七天。本组件仅适用于这些资源可丢弃的专用 CI Runner 宿主机。关联 Issue
Refs #10035