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', () => {