Skip to content

fix(ci): move undeletable workspace residue aside instead of leaving it #9868

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.

Sign up for GitHub

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

Merged
wenshao merged 1 commit into from
Aug 24, 2026
+182 −3
Merged

fix(ci): move undeletable workspace residue aside instead of leaving it #9868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 66 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,28 @@ jobs:
set -uo pipefail
if [ -d "$GITHUB_WORKSPACE/.qwen" ] && [ ! -L "$GITHUB_WORKSPACE/.qwen" ]; then
chmod -R u+w "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || true
rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || echo "::warning::leaked .qwen; runner needs manual cleanup"
# Last resort when nothing can DELETE the tree: move it out of the
# workspace. Unlinking an entry needs write permission on the
# directory holding it — which is exactly what a foreign-owned
# leftover denies — while renaming needs it only on the two
# parents, and the workspace root is always this runner's own. So
# a tree that defeats rm, chmod, and a sudo-less chown still
# renames aside, and the checkout below finds nothing to trip on.
# Leaving it in place instead poisons EVERY later job scheduled
# here, not just this one (measured, run 32621267802: `EACCES
# rmdir .qwen/tmp/review-pr-9748-scratch-verify-…/probe-ws/…`
# killed checkout for two unrelated PRs on the same runner).
rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null ||
sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null ||
{
quarantine="$(dirname -- "$GITHUB_WORKSPACE")/_qwen-quarantine"
mkdir -p "$quarantine" 2>/dev/null || true
if mv -- "$GITHUB_WORKSPACE/.qwen" "$quarantine/qwen-$(date -u +%Y%m%dT%H%M%SZ)-$$" 2>/dev/null; then
echo "::warning::could not delete leaked .qwen; moved it to $quarantine so this checkout can proceed — that directory needs manual cleanup"
else
echo "::warning::leaked .qwen; runner needs manual cleanup"
fi
}
fi
# Interrupted reviews leave worktree registrations under .qwen/tmp/
# and qwen-review/* branches behind. prune drops registrations whose
Expand Down Expand Up @@ -658,7 +679,28 @@ jobs:
set -uo pipefail
if [ -d "$GITHUB_WORKSPACE/.qwen" ] && [ ! -L "$GITHUB_WORKSPACE/.qwen" ]; then
chmod -R u+w "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || true
rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || echo "::warning::leaked .qwen; runner needs manual cleanup"
# Last resort when nothing can DELETE the tree: move it out of the
# workspace. Unlinking an entry needs write permission on the
# directory holding it — which is exactly what a foreign-owned
# leftover denies — while renaming needs it only on the two
# parents, and the workspace root is always this runner's own. So
# a tree that defeats rm, chmod, and a sudo-less chown still
# renames aside, and the checkout below finds nothing to trip on.
# Leaving it in place instead poisons EVERY later job scheduled
# here, not just this one (measured, run 32621267802: `EACCES
# rmdir .qwen/tmp/review-pr-9748-scratch-verify-…/probe-ws/…`
# killed checkout for two unrelated PRs on the same runner).
rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null ||
sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null ||
{
quarantine="$(dirname -- "$GITHUB_WORKSPACE")/_qwen-quarantine"
mkdir -p "$quarantine" 2>/dev/null || true
if mv -- "$GITHUB_WORKSPACE/.qwen" "$quarantine/qwen-$(date -u +%Y%m%dT%H%M%SZ)-$$" 2>/dev/null; then
echo "::warning::could not delete leaked .qwen; moved it to $quarantine so this checkout can proceed — that directory needs manual cleanup"
else
echo "::warning::leaked .qwen; runner needs manual cleanup"
fi
}
fi
# Interrupted reviews leave worktree registrations under .qwen/tmp/
# and qwen-review/* branches behind. prune drops registrations whose
Expand Down Expand Up @@ -1092,7 +1134,28 @@ jobs:
set -uo pipefail
if [ -d "$GITHUB_WORKSPACE/.qwen" ] && [ ! -L "$GITHUB_WORKSPACE/.qwen" ]; then
chmod -R u+w "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || true
rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null || echo "::warning::leaked .qwen; runner needs manual cleanup"
# Last resort when nothing can DELETE the tree: move it out of the
# workspace. Unlinking an entry needs write permission on the
# directory holding it — which is exactly what a foreign-owned
# leftover denies — while renaming needs it only on the two
# parents, and the workspace root is always this runner's own. So
# a tree that defeats rm, chmod, and a sudo-less chown still
# renames aside, and the checkout below finds nothing to trip on.
# Leaving it in place instead poisons EVERY later job scheduled
# here, not just this one (measured, run 32621267802: `EACCES
# rmdir .qwen/tmp/review-pr-9748-scratch-verify-…/probe-ws/…`
# killed checkout for two unrelated PRs on the same runner).
rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null ||
sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" 2>/dev/null ||
{
quarantine="$(dirname -- "$GITHUB_WORKSPACE")/_qwen-quarantine"
mkdir -p "$quarantine" 2>/dev/null || true
if mv -- "$GITHUB_WORKSPACE/.qwen" "$quarantine/qwen-$(date -u +%Y%m%dT%H%M%SZ)-$$" 2>/dev/null; then
echo "::warning::could not delete leaked .qwen; moved it to $quarantine so this checkout can proceed — that directory needs manual cleanup"
else
echo "::warning::leaked .qwen; runner needs manual cleanup"
fi
}
fi
# Interrupted reviews leave worktree registrations under .qwen/tmp/
# and qwen-review/* branches behind. prune drops registrations whose
Expand Down
116 changes: 116 additions & 0 deletions scripts/tests/review-worktree-cleanup-workflow.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
lstatSync,
mkdirSync,
mkdtempSync,
readdirSync,
readFileSync,
rmSync,
symlinkSync,
Expand Down Expand Up @@ -130,6 +131,33 @@ function expectCleanupRecipe(run) {
expectPipedLoopsIsolated(code, 2);
}

// Deleting the tree is not always possible: a containerised job on this
// shared pool can leave residue owned by another uid, and on a pool member
// without passwordless sudo nothing unprivileged can unlink it. Leaving it
// in place poisons the checkout of every LATER job scheduled here, so the
// sweep must move it out of the workspace instead of warning and continuing
// — renaming needs write permission only on the two parents, and the
// workspace root is always the runner's own.
function expectQuarantineFallback(run) {
const code = stripComments(run);
expect(code).toContain('_qwen-quarantine');
// The move must be the fallback of the removal chain, not an
// unconditional relocation: a workspace that deletes cleanly keeps its
// caches.
expect(code).toMatch(
/rm -rf "\$GITHUB_WORKSPACE\/\.qwen"[\s\S]*?sudo -n rm -rf[\s\S]*?mv -- "\$GITHUB_WORKSPACE\/\.qwen"/,

@qwen-code-ci-bot qwen-code-ci-bot Aug 24, 2026

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.

[Suggestion] This chain-order pin checks the sudo rung's verb only (sudo -n rm -rf), not its target path — and the one behavioral test that reaches the fallback stubs sudo() { return 1; }, so the argument is never observed. Confirmed by probe: the one-line mutation sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" → sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen.bak" survives this regex and both behavioral tests (all 15 pass), and an argument-recording sudo stub shows the deletion rung then targeting a nonexistent path. On pool members with passwordless sudo, foreign-owned residue would silently never be deleted again — every incident degrades to quarantine-plus-manual-cleanup instead of a clean deletion, and nothing in the suite goes red.

Suggested change
/rm -rf "\$GITHUB_WORKSPACE\/\.qwen"[\s\S]*?sudo -n rm -rf[\s\S]*?mv -- "\$GITHUB_WORKSPACE\/\.qwen"/,
/rm -rf "\$GITHUB_WORKSPACE\/\.qwen"[\s\S]*?sudo -n rm -rf "\$GITHUB_WORKSPACE\/\.qwen"[\s\S]*?mv -- "\$GITHUB_WORKSPACE\/\.qwen"/,
中文说明

这条顺序锁定只检查了 sudo 阶梯的动词（sudo -n rm -rf），没有检查它的目标路径——而唯一会走到兜底分支的行为测试又把 sudo 桩成 sudo() { return 1; }，因此该参数从未被观测到。探针已证实：单行变异 sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen" → sudo -n rm -rf "$GITHUB_WORKSPACE/.qwen.bak" 能通过该正则与两个行为测试（15 项全绿）；用记录参数的 sudo 桩可看到删除阶梯随后指向一个不存在的路径。在配备免密 sudo 的池节点上，异属主残留将悄无声息地永远删不掉——每次事故都退化为「隔离 + 人工清理」而非干净删除，而套件中没有任何测试变红。

— qwen3.8-max via Qwen Code /review (v0.22.0)

);
// Same filesystem by construction — a cross-device `mv` degrades to
// copy-then-unlink, which fails on exactly the residue this exists for.
expect(code).toContain(
'"$(dirname -- "$GITHUB_WORKSPACE")/_qwen-quarantine"',
);
// The quarantined tree still needs a human: the warning must name where
// it went, and the terminal warning must survive for the case where even
// the rename fails.
expect(code).toContain('leaked .qwen; runner needs manual cleanup');
Comment on lines +157 to +158

@qwen-code-ci-bot qwen-code-ci-bot Aug 24, 2026

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.

[Suggestion] The rename-failure branch of the new quarantine fallback (the else of if mv ...) has no behavioral test — its only pin is the substring check above, and the two behavioral fixtures exercise only rm-success and quarantine-success; nothing ever makes mv fail. A mutant that preserves the warning string but lets the else path return non-zero (e.g. appending && rmdir "$GITHUB_WORKSPACE/.qwen" to the else echo, or dropping || true from mkdir -p) survives all 15 tests — confirmed by probe: with that mutant applied to all three ci.yml copies the whole suite still passes. On a runner where the rename is impossible, the brace group then exits non-zero under the runner's default bash -e and fails the pre-checkout step before checkout — re-creating the exact "cleanup blocks the job" shape this PR exists to fix.

Witness (probe in an isolated tree): the missing fixture — mirroring the undeletable-residue test with the quarantine parent locked (chmod 0o500) so mkdir -p and mv both fail — fails against the mutant (AssertionError: expected 1 to be +0 under set -euo pipefail) and passes on the PR code, while all 15 existing tests stay green on the mutant.

Suggested fix: add that third behavioral fixture — assert out.status === 0, .qwen still present in the workspace, no _qwen-quarantine entry created, and exactly one ::warning:: containing leaked .qwen; runner needs manual cleanup; repair permissions in finally with the same pattern test 1 already uses.

中文说明

新增隔离兜底的「重命名失败」分支（if mv ... 的 else）没有行为测试——它唯一的锁定就是上面的子串检查，而两个行为夹具只覆盖了 rm 成功与隔离成功两种情形；mv 从未被置于失败境地。一个保留警告字符串但让 else 路径返回非零的变异（例如在 else 的 echo 后追加 && rmdir "$GITHUB_WORKSPACE/.qwen"，或去掉 mkdir -p 后的 || true）能通过全部 15 项测试——探针已证实：把该变异应用到 ci.yml 的三份副本后，整个测试套件仍然全绿。而在一台连重命名都做不到的 runner 上，花括号分组会在 runner 默认的 bash -e 下以非零退出，导致 checkout 前的清理步骤在 checkout 之前失败——恰恰重现了本 PR 要修复的「清理挡住 job」的事故形态。

证据（隔离树中的探针）：补上缺失的夹具——仿照「不可删除残留」测试、但把隔离目录的父目录锁成 0o500，使 mkdir -p 与 mv 双双失败——它对变异体失败（set -euo pipefail 下 AssertionError: expected 1 to be +0），对 PR 原代码通过；同时现有 15 项测试在变异体上依旧全绿。

建议修复：补上这第三个行为夹具——断言 out.status === 0、工作区中 .qwen 仍在、未创建任何 _qwen-quarantine 条目、且恰好一条包含 leaked .qwen; runner needs manual cleanup 的 ::warning::；finally 中沿用测试 1 已有的权限修复模式。

— qwen3.8-max via Qwen Code /review (v0.22.0)

}

function expectHardenedGit(run) {
expect(run).toContain(
'GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")',
Expand Down Expand Up @@ -256,6 +284,7 @@ describe('review worktree cleanup steps', () => {
expect(cleanIdx, id).toBeLessThan(checkoutIdx);
expectCleanupRecipe(run);
expectHardenedGit(run);
expectQuarantineFallback(run);
}
// The copies are deliberate: a pre-checkout step cannot trust leftover
// workspace scripts, so the recipe stays inline per job. Pin them
Expand Down Expand Up @@ -457,6 +486,93 @@ describe('review worktree cleanup steps', () => {
},
);

it.skipIf(!permissionFixturesAvailable)(
'the pre-checkout sweep moves residue it cannot delete out of the workspace',
() => {
// The incident this exists for: residue whose containing directory
// denies the unlink, so `rm -rf` fails and actions/checkout dies
// wiping the workspace (measured, run 32621267802 — two unrelated PRs
// failed at Checkout on the same runner). Reproduced here with a
// write-denied parent rather than a foreign uid, which needs root:
// the failing syscall and the recovery are the same, and the sweep's
// own chmod is stepped over so it cannot repair the fixture away.
const root = mkdtempSync(join(tmpdir(), 'ci-quarantine-'));
const workspace = join(root, 'repo', 'repo');
const poison = join(
workspace,
`${toPosix(REVIEW_TMP_DIR)}/review-pr-9748-scratch-verify--round-1--x`,
);
const locked = join(poison, 'probe-ws/.qwen/tmp');
try {
mkdirSync(join(locked, 'review-pr-666'), { recursive: true });
chmodSync(locked, 0o500);
const out = spawnSync(
'bash',
[
'-c',
// Neutralise the sweep's own chmod and any sudo: this models the
// pool member that cannot repair the residue at all.
`set -euo pipefail\nchmod() { return 1; }\nsudo() { return 1; }\n${ciCleanSteps[0].run}`,
'clean-stale-qwen',
],
{
cwd: workspace,
env: { ...process.env, GITHUB_WORKSPACE: workspace },
encoding: 'utf8',
},
);
expect(out.status).toBe(0);
// The workspace is clear, so the checkout that follows has nothing
// to trip on …
expect(existsSync(join(workspace, '.qwen'))).toBe(false);
// … and the residue was moved, not deleted: it still needs a human,
// and the warning says where it went.
const quarantine = join(root, 'repo', '_qwen-quarantine');
expect(existsSync(quarantine)).toBe(true);
expect(readdirSync(quarantine)).toHaveLength(1);
const warnings = out.stdout
.split('\n')
.filter((line) => line.startsWith('::warning::'));
expect(warnings).toHaveLength(1);
expect(warnings[0]).toContain('_qwen-quarantine');
} finally {
// The locked directory has usually MOVED by now (that is the point),
// so repair the whole fixture by path rather than the original one.
spawnSync('bash', [
'-c',
`chmod -R u+rwX "${root}" 2>/dev/null || true`,
]);
rmSync(root, { recursive: true, force: true });
}
},
);

it.skipIf(!permissionFixturesAvailable)(
'the pre-checkout sweep still deletes residue it can remove',
() => {
// The fallback must stay a fallback: a workspace that deletes cleanly
// keeps its caches instead of accumulating quarantined copies.
const root = mkdtempSync(join(tmpdir(), 'ci-quarantine-'));
const workspace = join(root, 'repo', 'repo');
try {
mkdirSync(join(workspace, `${toPosix(REVIEW_TMP_DIR)}/review-pr-77`), {
recursive: true,
});
const out = spawnSync('bash', ['-c', ciCleanSteps[0].run], {
cwd: workspace,
env: { ...process.env, GITHUB_WORKSPACE: workspace },
encoding: 'utf8',
});
expect(out.status).toBe(0);
expect(existsSync(join(workspace, '.qwen'))).toBe(false);
expect(existsSync(join(root, 'repo', '_qwen-quarantine'))).toBe(false);
expect(out.stdout).not.toContain('::warning::');
} finally {
rmSync(root, { recursive: true, force: true });
}
},
);

it.skipIf(!permissionFixturesAvailable)(
'remove_review_tree actually removes a plain leftover',
() => {
Expand Down
Loading