ci: isolate self-hosted runner temp directories - #8547
Closed
yiliang114 wants to merge 2 commits into
Closed
Conversation
Collaborator
|
|
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, with hard evidence. A stale Direction: aligned. This repo deliberately invests in the self-hosted ECS lane (existing Size: not applicable — CI workflow only, no core packages touched. Approach: the scope feels right. I checked every Risk: no elevated risk signals (no revert-correlated paths touched). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到，且有硬证据。持久 ECS runner 上残留的 方向：对齐。仓库本就在持续建设 self-hosted ECS 通道（已有 规模：不适用——仅 CI workflow，未触及核心包。 方案：范围合理。我检查了 ci.yml 中所有 风险：无升级风险信号（未触及与 revert 相关的路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewIndependent baseline before reading the diff: for cross-job The implementation matches that proposal and the file's own conventions:
No critical issues, no convention violations. The three duplicated blocks match how adjacent self-hosted steps are already written in this file, so not introducing a shared abstraction is the consistent choice here. Test evidence (this PR's own CI, read via the API — no PR code executed)The Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查读 diff 前的独立方案：针对持久 runner 上的跨 job 实现与独立方案一致，也符合该文件自身的惯例：
无严重问题，无惯例违规。三处重复块与该文件中相邻 self-hosted 步骤的既有写法一致，因此不引入共享抽象在这里是连贯的选择。 测试证据（该 PR 自身的 CI，通过 API 读取——未执行任何 PR 代码）针对被审 commit 的 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — minimal infra fix by a maintainer for a real, API-verified failure; the approach matches the workflow's own conventions and the new step already executed correctly on a live self-hosted runner. Stepping back: this is the kind of PR the gate should wave through quickly. The problem was observed four times with linked job evidence (one of which I verified directly), the diff is 21 added lines in one CI file with zero product code touched, and the solution does exactly one thing in the same style as the surrounding self-hosted setup steps. My independent proposal before reading the diff was the same approach; the PR matches it, including the deliberate choice to keep the fix in-workflow rather than in the ECS job-start hook. The six-months-from-now test: a maintainer reading these steps sees the same pattern as the npm-cache step next to them — no curse. The only open item is the CI run itself, which is also the real testbed for this change — the review-efficacy suite executes on the self-hosted runners, so a green run is the acceptance signal. Approval is therefore deferred until CI lands green on the reviewed commit. 中文说明回顾整体：这是门禁应当快速放行的 PR。问题被观测到四次且附有 job 链接证据（其中一个我直接通过 API 核实），diff 只在一个 CI 文件里加了 21 行、完全不碰产品代码，方案只做一件事且与周围 self-hosted 准备步骤风格一致。我在读 diff 前的独立方案就是同一思路；PR 与之吻合，包括有意把修复放在 workflow 内而非 ECS job-start hook 中。六个月后的可维护性检验：维护者读到这些步骤时，看到的是与旁边 npm-cache 步骤相同的模式——不会骂人。 唯一未决项是 CI 运行本身，它同时也是本改动的真实试验场——review-efficacy 套件就在 self-hosted runner 上执行，绿色运行即验收信号。因此批准延迟到 CI 在被审 commit 上变绿。 — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
Closing after validating the change on an ECS runner. Redirecting the whole job temp directory prevented the original ambient Vitest lookup, but it also changed Core test paths and caused ls.test.ts to receive a shortened /home/... path in run 30929285718. More importantly, #8537 is already merged into main and makes both review efficacy tests independent of parent /tmp/node_modules state. The affected runner directory has been cleaned and the current #8386 head passes CI. There is no remaining repository change to land, and QWEN_RUNTIME_DIR remains untouched. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Configures every self-hosted Linux job in Qwen Code CI that runs
npm cito exportTMPDIR,TEMP, andTMPto its per-job runner temporary directory before installing dependencies or running tests. GitHub-hosted jobs are unchanged, and this does not modifyQWEN_RUNTIME_DIR.Why it's needed
Persistent ECS runners share
/tmpacross jobs. A stale/tmp/node_modulesallowed temporary test worktrees to resolve Vitest from a parent directory, breaking tests that intentionally verify behavior when Vitest is unavailable. Removing that directory recovered the affected runners, but isolating each job's temporary directory prevents the same cross-job contamination from recurring.Reviewer Test Plan
How to verify
Run the full Qwen Code CI workflow on a self-hosted Linux runner. The three Node jobs should execute the temp isolation step before
npm ci, and the review efficacy suite should pass even when the host's shared/tmpcontains unrelated files.Evidence (Before & After)
Before: four PR jobs failed the same review efficacy assertions after resolving Vitest through the shared
/tmp/node_modulesdirectory.After: the focused review efficacy suite passes 128/128 with
TMPDIR,TEMP, andTMPpointed at an isolated job directory. actionlint, Prettier, and the existing runner-routing tests also pass.Tested on
Environment (optional)
Local Node.js validation on macOS; Linux validation is delegated to the PR's self-hosted workflow run.
Risk & Scope
/tmpto the runner-managed per-job directory.Linked Issues
Observed in PR #8386 job 91976739611, PR #8125 job 92012154212, PR #8431 job 92003313919, and PR #8518 job 92013890763.
中文说明
此 PR 的改动
所有会执行
npm ci的 Qwen Code CI self-hosted Linux job 都会在安装依赖或运行测试前，把TMPDIR、TEMP和TMP指向 runner 为当前 job 管理的独立临时目录。GitHub-hosted job 不受影响，本改动也不会修改QWEN_RUNTIME_DIR。为什么需要
持久运行的 ECS runner 会在不同 job 之间共享
/tmp。残留的/tmp/node_modules使测试创建的临时 worktree 能够从父目录解析到 Vitest，破坏了原本用于验证“Vitest 不可用”场景的测试。删除该目录恢复了受影响的 runner，但只有隔离每个 job 的临时目录才能防止相同的跨 job 污染再次发生。Reviewer Test Plan
验证方式
在 self-hosted Linux runner 上运行完整 Qwen Code CI。三个 Node job 都应在
npm ci前执行临时目录隔离步骤；即使宿主机共享/tmp中存在无关文件，review efficacy 测试也应通过。证据（改动前后）
改动前：四个 PR job 因通过共享的
/tmp/node_modules解析到 Vitest，出现相同的 review efficacy 断言失败。改动后：将
TMPDIR、TEMP和TMP指向独立 job 目录后，定向 review efficacy 测试 128/128 通过；actionlint、Prettier 和现有 runner-routing 测试也全部通过。测试平台
环境
本地在 macOS 上使用 Node.js 验证；Linux 验证交给该 PR 的 self-hosted workflow run。
风险和范围
/tmp移到 runner 管理的 job 独立目录。关联问题
该问题出现在 PR #8386 job 91976739611、PR #8125 job 92012154212、PR #8431 job 92003313919 和 PR #8518 job 92013890763。