[PR #129] [MERGED] fix(groom): stop build_pr full-cloning the target repo to file a bail issue #140

New issue

Closed

opened 2026-10-26 22:36:40 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:36:40 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/129
Author: @mattmillerai
Created: 8/4/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-groom-build-pr-checkout

📝 Commits (1)

891e660 fix(groom): stop build_pr full-cloning the target repo to file a bail issue

📊 Changes

1 file changed (+64 additions, -2 deletions)

View changed files

📝 .github/workflows/groom.yml (+64 -2)

📄 Description

ELI-5

When groom's robot writes a code change, a second job pushes it and opens the PR. That job always downloaded the entire history of the target repo first — even when there was no code change to push and its only job was to write a GitHub issue. On a huge repo that download can randomly hang. It did: the download sat there for 15 minutes producing nothing, the job's timer ran out, and the finding it was supposed to record was silently lost. Now it only downloads when there's actually a patch to apply, downloads far less, and retries once if the download stalls.

What happened

A build_pr matrix cell on a large downstream caller was cancelled at its 15-minute job timeout, sitting in this fetch the whole time with zero output:

16:03:20  git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules \
            origin +refs/heads/*:refs/remotes/origin/* +refs/tags/*:refs/tags/*
16:18:16  ##[error]The operation was canceled.

Not one * [new branch] line, so it hung in server-side pack negotiation before any ref landed. The sibling cell ran the identical fetch 70 seconds earlier and finished it in 50s — the 15-minute budget was already ~13x the observed need. This is a stall, not slowness, which is why a bigger budget is not the fix.

Cost: that cell had already decided to file a bail issue, so the finding never got filed, never entered the dedup ledger, and the next sweep re-derives it from scratch and pays the finder + verifier for it again.

Changes

Gate the checkout on the build result. New plan step reads result.json and the target-repo checkout is now if: needs_worktree == 'true'. The bail branch (result.get("status") != "patched") calls file_issue() and sys.exit(0)s before any git call, so it never needed a worktree — it was full-cloning a multi-GB monorepo in order to run gh issue create. Defaults closed: an unreadable or absent result.json yields bail, which is exactly the path that needs no worktree, so skipping it cannot strand a patch.
fetch-depth: 0 → 1. Nothing downstream reads history — the apply step does checkout -b, git apply --index, git push origin <branch>. Pushing a new branch from a shallow clone is fine because the base commit is already on the remote.
Two bounded attempts instead of one unbounded one. A step timeout-minutes on its own would not have recovered this: exceeding it fails the step and the job, and checkout's internal retry only covers git commands that exit non-zero inside a step that is still alive — never a step the runner killed. So attempt 1 is continue-on-error: true + timeout-minutes: 6, and a second attempt re-runs it on a fresh connection. The second has no continue-on-error, so a genuine two-strike failure is loud rather than falling through to an apply step with no worktree.
**Job timeout-minutes: 15 → 20.** Headroom only, explicitly commented as the last line of defence rather than the fix. It also stops build_prbeing tighter thanbuild` (30 min), which does strictly more work over the same checkout.

Verification

python3 -c "yaml.safe_load(...)" parses; build_pr step order confirmed programmatically: plan → checkout(if, continue-on-error, t=6) → retry checkout(if, t=6) → labels → apply.
actionlint reports 9 findings, all pre-existing on main and all on lines this PR does not touch (github.job_workflow_sha / job.workflow_sha are real contexts its schema lacks). Zero new findings.
Traced the bail path in the apply step to confirm the skip is safe: if result.get("status") != "patched": file_issue(...); sys.exit(0) — no git invocation upstream of that exit, and the _groom_assets checkout that file_issue imports the ledger from is deliberately not gated.
Not exercised end-to-end on a live groom run; the next scheduled sweep on a large caller is the real test, and the failure mode if I got the gate wrong is a bail issue that doesn't get filed — the same failure this PR fixes, not a worse one.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/129 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/4/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-groom-build-pr-checkout` --- ### 📝 Commits (1) - [`891e660`](https://github.com/Comfy-Org/github-workflows/commit/891e660994673878c00ab52c9cb504f5195de5f7) fix(groom): stop build_pr full-cloning the target repo to file a bail issue ### 📊 Changes **1 file changed** (+64 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/groom.yml` (+64 -2) </details> ### 📄 Description ## ELI-5 When groom's robot writes a code change, a second job pushes it and opens the PR. That job always downloaded the *entire* history of the target repo first — even when there was no code change to push and its only job was to write a GitHub issue. On a huge repo that download can randomly hang. It did: the download sat there for 15 minutes producing nothing, the job's timer ran out, and the finding it was supposed to record was silently lost. Now it only downloads when there's actually a patch to apply, downloads far less, and retries once if the download stalls. ## What happened A `build_pr` matrix cell on a large downstream caller was cancelled at its 15-minute job timeout, sitting in this fetch the whole time with **zero output**: ``` 16:03:20 git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules \ origin +refs/heads/*:refs/remotes/origin/* +refs/tags/*:refs/tags/* 16:18:16 ##[error]The operation was canceled. ``` Not one `* [new branch]` line, so it hung in server-side pack negotiation before any ref landed. The sibling cell ran the **identical** fetch 70 seconds earlier and finished it in 50s — the 15-minute budget was already ~13x the observed need. This is a stall, not slowness, which is why a bigger budget is not the fix. Cost: that cell had already decided to file a bail issue, so the finding never got filed, never entered the dedup ledger, and the next sweep re-derives it from scratch and pays the finder + verifier for it again. ## Changes 1. **Gate the checkout on the build result.** New `plan` step reads `result.json` and the target-repo checkout is now `if: needs_worktree == 'true'`. The bail branch (`result.get("status") != "patched"`) calls `file_issue()` and `sys.exit(0)`s before any git call, so it never needed a worktree — it was full-cloning a multi-GB monorepo in order to run `gh issue create`. Defaults **closed**: an unreadable or absent `result.json` yields `bail`, which is exactly the path that needs no worktree, so skipping it cannot strand a patch. 2. **`fetch-depth: 0` → `1`.** Nothing downstream reads history — the apply step does `checkout -b`, `git apply --index`, `git push origin <branch>`. Pushing a new branch from a shallow clone is fine because the base commit is already on the remote. 3. **Two bounded attempts instead of one unbounded one.** A step `timeout-minutes` on its own would *not* have recovered this: exceeding it fails the step and the job, and checkout's internal retry only covers git commands that exit non-zero inside a step that is still alive — never a step the runner killed. So attempt 1 is `continue-on-error: true` + `timeout-minutes: 6`, and a second attempt re-runs it on a fresh connection. The second has no `continue-on-error`, so a genuine two-strike failure is loud rather than falling through to an apply step with no worktree. 4. **Job `timeout-minutes: 15 → 20.** Headroom only, explicitly commented as the last line of defence rather than the fix. It also stops `build_pr` being tighter than `build` (30 min), which does strictly more work over the same checkout. ## Verification - `python3 -c "yaml.safe_load(...)"` parses; `build_pr` step order confirmed programmatically: `plan` → checkout(if, continue-on-error, t=6) → retry checkout(if, t=6) → labels → apply. - `actionlint` reports 9 findings, **all pre-existing on `main`** and all on lines this PR does not touch (`github.job_workflow_sha` / `job.workflow_sha` are real contexts its schema lacks). Zero new findings. - Traced the bail path in the apply step to confirm the skip is safe: `if result.get("status") != "patched": file_issue(...); sys.exit(0)` — no git invocation upstream of that exit, and the `_groom_assets` checkout that `file_issue` imports the ledger from is deliberately **not** gated. - Not exercised end-to-end on a live groom run; the next scheduled sweep on a large caller is the real test, and the failure mode if I got the gate wrong is a bail issue that doesn't get filed — the same failure this PR fixes, not a worse one. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:40 +00:00