Skip to content

fix(ci): quiet pr-lockfile artifact noise, retry Playwright apt install - #1119

Open
allyblockcast[bot] wants to merge 2 commits into
masterfrom
blo-22675-merge-queue-reliability
Open

fix(ci): quiet pr-lockfile artifact noise, retry Playwright apt install#1119
allyblockcast[bot] wants to merge 2 commits into
masterfrom
blo-22675-merge-queue-reliability

Conversation

@allyblockcast

@allyblockcast allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip is the open source app people use to manage AI agents for work
  • The merge_group lane in .github/workflows/pr.yml is the actual merge path — every PR on Blockcast/paperclip must go green there before landing
  • It's failing ~1 in 3 runs, forcing human admins to bypass the queue with manual merges (see PR feat(ui): seed acceptance and verification sections #1038), which defeats the batch-validation the queue exists to provide
  • Two of the five sampled failures have workflow-file-level fixes with real evidence behind them; papering over the other three (runner infra, real test flakes) would hide rather than fix them
  • This pull request gates the pr-lockfile artifact download on the policy job's own output (so it's silent, not scary-but-harmless, on the ~99% of runs that don't need it) and adds a bounded retry to the Playwright system-deps install step (which failed on a transient apt mirror sync error in one sampled run)
  • The benefit is fewer misleading ##[error] annotations during incident triage, and one fewer way an upstream apt hiccup can fail an entire merge_group run outright

Linked Issues or Issue Description

Refs: BLO-22675 (paperclip.blockcast.net) — "Merge queue is failing ~1 in 3 runs across four unrelated jobs — PRs need manual admin merges to land"

What Changed

  • Gated all 7 "Restore regenerated PR lockfile (if policy uploaded one)" download-artifact steps behind if: needs.policy.outputs.lockfile_regenerated == '1', so the step is skipped (not attempted-and-swallowed) on the common case where the policy job didn't upload anything.
  • Added a 3-attempt retry (15s backoff) around pnpm exec playwright install-deps chromium in the e2e job, to absorb transient apt mirror errors (observed: security.ubuntu.com "File has unexpected size... Mirror sync in progress?").

Verification

  • Pulled real job logs (gh api repos/Blockcast/paperclip/actions/jobs/<id>/logs) for all 5 failed runs in the issue's trailing-15 sample, rather than trusting the failing-step name alone. Findings, with evidence:
    • 31107120860 (e2e / Install Playwright system deps) — apt mirror sync error on security.ubuntu.com, not the pr-lockfile artifact (that download is continue-on-error: true and never fails the job) — addressed by the retry above.
    • 31116201133 (3 shards of Verify serialized server / Set up job) — Failed to resolve action download info: Service Unavailable before any workflow step ran — runner-pool infra, not fixable from this file. Filed as a follow-up.
    • 31113892541 (General tests server 3/4 / Run grouped general test suites) — The runner has received a shutdown signal mid-test-run (pod evicted/killed) — runner-pool infra. Same follow-up.
    • 31099969740 (e2e / Run e2e tests) — genuine flaky test: mcp-user-stories Playwright locator-visibility timeout. Not an infra failure; routing to QA.
    • 31095848976 (General tests server 2/4) — genuine flaky test: workspace-runtime.test.ts submodule-inspection-timeout assertion. Not an infra failure; routing separately.
  • node --test ./scripts/__tests__/pr-verify-lane-outcome.test.mjs — still 13/13 passing (this file's lane-outcome semantics are unaffected by the change).
  • Validated resulting YAML parses (js-yaml) and that needs.policy.outputs.lockfile_regenerated is a real, already-declared job output every touched job already has needs: policy (directly or transitively) for.
  • Cross-referenced live cluster state: arc-runners namespace shows FailedScheduling events at 2026-08-06T21:28Z citing "6 Insufficient ephemeral-storage" across the ARC runner-host node pool — corroborates the runner-pool-infra classification above; tracked in the follow-up issue rather than fixed here (needs its own investigation of ephemeral-storage requests/image GC, not a workflow-file change).

Risks

  • Low risk: both changes are CI-workflow-only, additive (a conditional skip and a bounded retry), and don't touch runtime/product code or required-check semantics. verify's lane-outcome logic (cancelled/skipped/failed) is untouched.
  • The retry adds up to ~30s to a real (non-transient) Playwright install failure before it surfaces, which is an acceptable tradeoff against failing the whole ~60-minute e2e job on a mirror blip.
  • Does not by itself guarantee the AC's ≤10% trailing-20-run failure rate — 3 of the 5 sampled failures are runner-infra or test-flake issues outside this PR's scope (tracked as follow-ups on BLO-22675). Left the parent issue open pending a second 72h-apart sample.

Model Used

Claude Sonnet 5 (claude-sonnet-5[1m]), 1M context window, via Claude Code — used to pull and read live GitHub Actions job logs and cluster state (Prometheus, kubectl-read-only) to verify actual root causes before editing the workflow, and to author this change.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked them above
  • I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
  • I have run tests locally and they pass
  • I have added or updated tests where applicable — n/a, workflow-YAML-only change with no existing test harness for this file's step conditionals
  • If this change affects the UI, I have included before/after screenshots — n/a
  • I have updated relevant documentation to reflect my changes — n/a
  • I have considered and documented any risks above
  • All Paperclip CI gates are green — pending this PR's own CI run
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending review
  • I will address all Greptile and reviewer comments before requesting merge

merge_group runs fail ~1 in 3 (BLO-22675). Two contributing signatures,
verified against actual job logs rather than assumed from the error text:

- Every "Restore regenerated PR lockfile" step downloaded unconditionally
  and logged a scary ##[error] "Artifact not found" whenever the policy
  job hadn't uploaded one (the common case — most PRs don't touch a
  manifest). continue-on-error already kept it from failing the job, but
  the annotation reads as a real failure cause during triage and cost a
  full agent run to rule out. Gate the download on
  needs.policy.outputs.lockfile_regenerated so it's silent when unneeded.
- The e2e job's "Install Playwright system dependencies" step has no
  retry, so a transient apt mirror hiccup (observed: security.ubuntu.com
  "Mirror sync in progress?") fails the whole run outright. Retry 3x.

Two of five sampled failures in this run were genuine runner-pool
infra (mid-job SIGTERM, "Service Unavailable" resolving action download
info) and two were real test flakes (e2e locator timeout, a vitest
submodule-inspection assertion) — neither fixable from this workflow
file. Tracked as follow-ups on BLO-22675 rather than papered over here.
@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-22675

1 similar comment
@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

🔗 Paperclip issue: BLO-22675

@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

  • No test files detected in this PR — please include a test that verifies the bug fix or new behavior. If this PR genuinely doesn't need a test (e.g. a refactor), please retitle with refactor: prefix.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

@allyblockcast

allyblockcast Bot commented Aug 6, 2026

Copy link
Copy Markdown
Author

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The synced skill runners were unavailable in this pod, so their checklists were applied directly to the exact diff.
Reviewed head: d78bc9e

Critical Issues (0)

Important Issues (1)

  • [pr-review-toolkit/tests] .github/workflows/pr.yml:224 — The seven new artifact guards and the retry behavior have no regression test, so the repository's required review check is already failing and future workflow edits can silently remove one of the duplicated guards or change the attempt count/backoff.
    • Extend the existing workflow-source test pattern (for example, scripts/__tests__/pr-verify-lane-outcome.test.mjs) to assert every downstream pr-lockfile download is guarded by needs.policy.outputs.lockfile_regenerated == '1', and extract/exercise the retry script with a stubbed failing command to verify its attempt and sleep behavior.

Suggestions (1)

  • [native-codex] .github/workflows/pr.yml:740 — The third and final failure still logs "retrying", sleeps 15 seconds, and only then exits, so three failed attempts add 45 seconds rather than the documented 30-second maximum. Guard the message/sleep with if [ "$attempt" -lt 3 ]; then ...; fi.

Strengths

  • The artifact condition uses the policy job's existing explicit output and every touched consumer already depends on policy.
  • The retry is bounded and does not weaken the final failure signal.

Recommended Action

  1. Add regression coverage for the workflow behavior before merge.
  2. Avoid the unnecessary final backoff.
  3. This PR is authored by app/allyblockcast; the exact head must be reopened under an independent author before the allyblockcast GitHub App can provide the gate-authorizing approval. The shared merge-token user is not a substitute for that App review.

…trailing sleep

Ally review on PR #1119 flagged two real issues in the prior commit:

- No regression test for the 7 duplicated lockfile-download guards or the
  Playwright install-deps retry/backoff, so a future edit could silently
  drop a guard or change the attempt count without any check catching it.
  Add scripts/__tests__/pr-lockfile-artifact-guard.test.mjs, following the
  pr-verify-lane-outcome.test.mjs pattern of extracting and executing the
  real workflow script rather than re-implementing it, and wire it up as
  an explicit policy-job step like its siblings (these test files are not
  auto-discovered).
- The retry loop's third and final failed attempt still logged "retrying"
  and slept 15s before exiting, so three failures cost 45s instead of the
  intended 30s max. Guard the message/sleep with `[ "$attempt" -lt 3 ]`.
@allyblockcast

allyblockcast Bot commented Aug 7, 2026

Copy link
Copy Markdown
Author

Thanks for the review. Pushed fb62b12 addressing both real findings:

  • Important (missing regression coverage): added scripts/__tests__/pr-lockfile-artifact-guard.test.mjs, following the pr-verify-lane-outcome.test.mjs pattern — it extracts and executes the real workflow scripts rather than re-implementing them. It asserts all 7 Restore regenerated PR lockfile steps are immediately preceded by the lockfile_regenerated == '1' guard, and exercises the Playwright retry script with a stubbed failing pnpm/sleep to pin attempt count and backoff. Wired it up as an explicit node --test step in the policy job (these workflow-source tests aren't auto-discovered — same reason the review check was correctly red).
  • Suggestion (45s vs documented 30s max): confirmed and fixed — the loop was sleeping after the 3rd/final failed attempt too. Guarded the retry message + sleep 15 with [ "$attempt" -lt 3 ], and the new test's third case (stops after 3 attempts without a trailing sleep) pins this.

On point 3 (independent-author gate): confirmed this PR — like the other 4 most recently opened bot-authored PRs (#1120, #1118, #1116, #1115) — was opened under the allyblockcast[bot] installation identity, and every recently merged PR from this identity (#1051, #1045, #1038, #1034, #1021) was merged by a human admin (kkroo), not through an app-approval gate. That's a real structural issue — an App cannot approve its own PR — but it's a property of how this agent pipeline authenticates git push/PR creation, not something fixable by editing pr.yml, and it affects every PR from this pipeline, not just this diff. Out of scope for this PR; I'm flagging it to the CTO separately since it's a contributing cause of the "manual admin merge" pattern BLO-22675 is tracking, distinct from the queue-lane infra failures this PR fixes.

@allyblockcast

allyblockcast Bot commented Aug 7, 2026

Copy link
Copy Markdown
Author

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The synced skill runners were unavailable in this pod, so their checklists were applied directly to the exact diff.
Reviewed head: fb62b12

Looks good.

Prior Findings Dispositioned (1)

  • prior:d78bc9e important 1 — fixed — scripts/__tests__/pr-lockfile-artifact-guard.test.mjs:14 — The exact-head regression test asserts all seven artifact-download steps retain the policy-output guard; the same file executes the workflow's real retry script and verifies immediate success, transient recovery, and terminal failure without a trailing sleep. The test passes 4/4.

Critical Issues (0)

Important Issues (0)

Strengths

  • The workflow test executes the embedded retry script rather than duplicating its behavior in a test-only implementation.
  • The retry remains bounded, preserves terminal failure, and now incurs only the documented two backoffs.
  • The policy job runs the new workflow-source regression test, preventing the previous required-check coverage gap.

Recommended Action

  1. No code changes requested.
  2. This PR is authored by app/allyblockcast; the exact head must be reopened under an independent author before the allyblockcast GitHub App can provide the gate-authorizing approval. The shared merge-token user is not App gate evidence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants