From bc378f024c067c77abd10e2e39e681d071e6ffee Mon Sep 17 00:00:00 2001 From: wenshao Date: Mon, 24 Aug 2026 14:22:58 +0800 Subject: [PATCH] fix(ci): move undeletable workspace residue aside instead of leaving it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A leftover the pre-checkout sweep cannot delete does not just fail the job that finds it — it poisons the checkout of every later job scheduled onto that runner. Measured on run 32621267802: residue from a review probe (`.qwen/tmp/review-pr-9748-scratch-verify-…/probe-ws/.qwen/tmp/review-pr-666`) survived on one shared-pool member, and two unrelated PRs then died at Checkout with the same EACCES on the same path. The sweep's own steps had reported success: their ladder ends at a warning, so a workspace it cannot repair is handed to actions/checkout unchanged. The recovery the ladder was missing is a rename. Unlinking an entry needs write permission on the directory holding it — exactly what foreign-owned residue denies on a pool member without passwordless sudo — while renaming needs it only on the two parents, and the workspace root is always the runner's own. So a tree that defeats rm, chmod, and chown still moves out of the way, and the checkout finds nothing to trip on. The destination sits next to the workspace so the rename cannot cross a filesystem and degrade into copy-then-unlink, and the warning names it, because the tree still needs a human. Reproduced in a Linux container with real foreign ownership (residue owned by root, workspace by the runner user, sudo unavailable): before, the step warns `leaked .qwen`, the tree stays, and a checkout-style wipe fails with `Permission denied`; after, the tree is quarantined, the wipe succeeds, and the warning points at where it went. --- .github/workflows/ci.yml | 69 ++++++++++- .../review-worktree-cleanup-workflow.test.js | 116 ++++++++++++++++++ 2 files changed, 182 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 53d817b3c2a..155ecdedeac 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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 @@ -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 @@ -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 diff --git a/scripts/tests/review-worktree-cleanup-workflow.test.js b/scripts/tests/review-worktree-cleanup-workflow.test.js index d8baf7e0a14..e7e51db96b1 100644 --- a/scripts/tests/review-worktree-cleanup-workflow.test.js +++ b/scripts/tests/review-worktree-cleanup-workflow.test.js @@ -11,6 +11,7 @@ import { lstatSync, mkdirSync, mkdtempSync, + readdirSync, readFileSync, rmSync, symlinkSync, @@ -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"/, + ); + // 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'); +} + function expectHardenedGit(run) { expect(run).toContain( 'GIT_SAFE=(git -c core.hooksPath=/dev/null -c core.fsmonitor= -C "$GITHUB_WORKSPACE")', @@ -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 @@ -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', () => {