fix(ci): reconcile stale PR preview resources#146
Conversation
Issue: AP-97
AP-97 Fix PR preview resource cleanup reconciliation
ProblemPR preview creation can leave PR-scoped Cloudflare Workers, Queues, Hyperdrive configs, or Neon branches behind when the Observed stale resources:
Scope
Acceptance criteria
Out of scope
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-146.isaac-a46.workers.dev |
Issue: AP-97
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-146.isaac-a46.workers.dev |
Issue: AP-97
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-146.isaac-a46.workers.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/cleanup-stale-pr-previews.mjs`:
- Around line 32-35: The PR classification step is silently skipping items with
state "unknown", hiding API/auth failures; update the logic after
classifyPrNumbers (the classified variable returned by classifyPrNumbers) to
detect any item where item.state === "unknown" and fail fast (throw an Error or
exit non-zero) with a clear message including the offending prNumber(s) and any
diagnostic info from the classifier, instead of proceeding to build stale via
.filter/.map; apply the same check for the later identical block referenced
(lines around 179-181) so reconciliation stops on unknown classifications.
In `@scripts/cleanup-stale-pr-previews.test.mjs`:
- Around line 168-201: The test for cleanupStalePrPreviews currently treats a
500 response from GitHub as silent success; change it to assert that
cleanupStalePrPreviews rejects (fails) when the fetch for "/pulls/137" returns
status 500. Update the assertion to use expect(...).rejects.toThrow() (or
toReject with a matching error message) instead of resolves.toEqual(...),
keeping the same mocked run, fetch, cleanupPreview, and deleteNeonBranch fns,
and still assert that cleanupPreview and deleteNeonBranch were not called; this
ensures cleanupStalePrPreviews reports classification failures rather than
silently succeeding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0741338-4b99-4f52-b87b-007dc4823a0e
📒 Files selected for processing (6)
.github/workflows/pr-preview-cleanup.yml.github/workflows/pr-preview.ymldocs/ops/first-deploy.mdscripts/README.mdscripts/cleanup-stale-pr-previews.mjsscripts/cleanup-stale-pr-previews.test.mjs
| const classified = await classifyPrNumbers(candidates, options.github, fetchFn); | ||
| const stale = classified | ||
| .filter((item) => item.state === "closed" || item.state === "missing") | ||
| .map((item) => item.prNumber); |
There was a problem hiding this comment.
Fail reconciliation when PR state classification is unknown.
Right now a GitHub API 5xx/auth failure is converted to "unknown" and silently skipped, which can make scheduled reconciliation report success while cleanup is effectively blind.
Suggested fix
const classified = await classifyPrNumbers(candidates, options.github, fetchFn);
+ const unknown = classified.filter((item) => item.state === "unknown").map((item) => item.prNumber);
+ if (unknown.length > 0) {
+ throw new Error(
+ `Unable to classify GitHub PR state for ${unknown.length} candidate(s): ${unknown
+ .map((prNumber) => `#${prNumber}`)
+ .join(", ")}.`,
+ );
+ }
const stale = classified
.filter((item) => item.state === "closed" || item.state === "missing")
.map((item) => item.prNumber);Also applies to: 179-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/cleanup-stale-pr-previews.mjs` around lines 32 - 35, The PR
classification step is silently skipping items with state "unknown", hiding
API/auth failures; update the logic after classifyPrNumbers (the classified
variable returned by classifyPrNumbers) to detect any item where item.state ===
"unknown" and fail fast (throw an Error or exit non-zero) with a clear message
including the offending prNumber(s) and any diagnostic info from the classifier,
instead of proceeding to build stale via .filter/.map; apply the same check for
the later identical block referenced (lines around 179-181) so reconciliation
stops on unknown classifications.
| it("does not clean previews when GitHub PR state cannot be classified", async () => { | ||
| const run = vi.fn(async (_command, args) => { | ||
| if (args.join(" ") === "exec wrangler hyperdrive list") { | ||
| return { code: 0, stdout: hyperdriveList(["agent-paste-db-pr-137"]), stderr: "" }; | ||
| } | ||
| if (args.join(" ") === "exec wrangler queues list") { | ||
| return { code: 0, stdout: "", stderr: "" }; | ||
| } | ||
| throw new Error(`Unexpected command ${args.join(" ")}`); | ||
| }); | ||
| const fetch = vi.fn(async (url) => { | ||
| if (String(url).endsWith("/pulls/137")) { | ||
| return textResponse("server error", { status: 500 }); | ||
| } | ||
| throw new Error(`Unexpected fetch ${url}`); | ||
| }); | ||
| const cleanupPreview = vi.fn(async () => {}); | ||
| const deleteNeonBranch = vi.fn(async () => {}); | ||
|
|
||
| await expect( | ||
| cleanupStalePrPreviews( | ||
| { github, cloudflare: {}, neon }, | ||
| { run, fetch, cleanupPreview, deleteNeonBranch, log: () => {} }, | ||
| ), | ||
| ).resolves.toEqual({ | ||
| discovered: ["137"], | ||
| stale: [], | ||
| cleaned: [], | ||
| dryRun: false, | ||
| }); | ||
|
|
||
| expect(cleanupPreview).not.toHaveBeenCalled(); | ||
| expect(deleteNeonBranch).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Update the GitHub-500 classification test to assert failure.
This test currently codifies silent success on PR-state classification failure, which can hide reconciliation outages.
Suggested test update
- it("does not clean previews when GitHub PR state cannot be classified", async () => {
+ it("fails when GitHub PR state cannot be classified", async () => {
@@
- await expect(
- cleanupStalePrPreviews(
- { github, cloudflare: {}, neon },
- { run, fetch, cleanupPreview, deleteNeonBranch, log: () => {} },
- ),
- ).resolves.toEqual({
- discovered: ["137"],
- stale: [],
- cleaned: [],
- dryRun: false,
- });
+ await expect(
+ cleanupStalePrPreviews(
+ { github, cloudflare: {}, neon },
+ { run, fetch, cleanupPreview, deleteNeonBranch, log: () => {} },
+ ),
+ ).rejects.toThrow("Unable to classify GitHub PR state");
expect(cleanupPreview).not.toHaveBeenCalled();
expect(deleteNeonBranch).not.toHaveBeenCalled();
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/cleanup-stale-pr-previews.test.mjs` around lines 168 - 201, The test
for cleanupStalePrPreviews currently treats a 500 response from GitHub as silent
success; change it to assert that cleanupStalePrPreviews rejects (fails) when
the fetch for "/pulls/137" returns status 500. Update the assertion to use
expect(...).rejects.toThrow() (or toReject with a matching error message)
instead of resolves.toEqual(...), keeping the same mocked run, fetch,
cleanupPreview, and deleteNeonBranch fns, and still assert that cleanupPreview
and deleteNeonBranch were not called; this ensures cleanupStalePrPreviews
reports classification failures rather than silently succeeding.
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-146.isaac-a46.workers.dev |
|
agent-paste PR preview is ready. API: https://agent-paste-api-pr-146.isaac-a46.workers.dev |
|
agent-paste PR preview resources were cleaned up. The shared Preview GitHub Environment is retained for future preview deploys. |
1 participant
Summary
Fixes AP-97 by making PR-preview cleanup event-independent. Closed or missing PR preview resources are now reconciled before new preview creation and on a scheduled cleanup workflow.
Changes
cleanup-stale-pr-previews.mjsto discover PR-scoped Hyperdrive configs, Queues, Workers, and stale GitHub PR state before cleaning Cloudflare and Neon resources.Risk: HIGH
Test plan
pnpm verifypnpm test:coveragepnpm test:scriptspnpm exec vitest run scripts/cleanup-stale-pr-previews.test.mjs scripts/cleanup-pr-preview.test.mjs scripts/delete-neon-pr-branch.test.mjs scripts/check-pr-preview-capacity.test.mjspnpm exec biome check scripts/cleanup-stale-pr-previews.mjs scripts/cleanup-stale-pr-previews.test.mjs .github/workflows/pr-preview.yml .github/workflows/pr-preview-cleanup.ymlpnpm exec prettier --check .github/workflows/pr-preview.yml .github/workflows/pr-preview-cleanup.yml scripts/README.md docs/ops/first-deploy.mdgit diff --checkgitleaks detect --no-git --redact --source . --log-opts --since=HEAD --verbose=falsegitleaks protect --staged --redact --verbose=false26694370548Issue: AP-97
Summary by CodeRabbit
New Features
Documentation