fix(ci): check out the event sha into a per-job directory #79
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/eco-79-isolated-checkout"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes ECO-79.
Consumer check workflows cloned every job into the same relative
srcpath on the shared self-hosted runner. A second job could replace the tree while the first job's build was still reading it. The per-product lock inci/build.sh(ECO-54) does not cover the checkout step, so it cannot prevent this.Changes
scripts/ci-checkout.sh <repo-url> <sha> <dest>: clones into a caller-owned directory, detaches at the exact event commit, verifies HEAD, and removes its own directory on failure. It rejects repo URLs with an embedded password, short or non-hex SHAs, and an existing destination.scripts/test-ci-checkout.sh: localfile://fixtures covering exact-SHA checkout after the branch advances, two concurrent jobs on different branches, a control that reproduces the old shared-path race, failure cleanup, and input validation..forgejo/workflows/check.yml: checks out withmktemp -dunderRUNNER_TEMP, pinsGITHUB_SHA, and removes only that directory in anif: always()step. All steps now use$ECO_CHECK_SRC.Evidence
livestack.rsdisappeared when job B cloned into the same path (local reproduction).checkout followed the advanced branch, not the event sha.scripts/test-ci-checkout.shpasses;ci/test-build-lock.shstill passes, so same-product serialization and cross-product concurrency are unchanged;scripts/lint-pins.shpasses.Scope
This PR changes only bread-ecosystem. The consumer repos (
bread,breadbar, and others) still use the shared-path checkout. Their updates are separate PRs and depend on this one being merged. Nothing here modifies any other repo.Not verified
The runner's workspace was not readable from the diagnosing host, so the exact interleaving on the runner is not confirmed. The regression reproduces the mechanism locally.
Breadway referenced this pull request from Breadway/bread2026-10-16 16:38:56 +08:00
Independent review of PR 79 at exact head
de66355a16(base main94ea37490f, mergeable). Verdict: CHANGES REQUESTED. Not merged.What I verified (isolated detached worktree at the exact SHA, primary checkout untouched):
5a9fdd9failed (run 306) and is superseded. No reviews or comments outstanding. Branch protection was not readable without a token, so required-check configuration is unobserved.Requested changes:
Non-blocking:
After merge the actual bread consumer jobs still need an integration/runtime CI run to verify behavior on the runner; this PR does not exercise that.
Independent re-review of PR 79 at exact head
9516cf6c9e(base main94ea37490f, mergeable, no reviews outstanding). Verdict: CHANGES REQUESTED. Not merged. Builds on review 5272 (#79 (comment)).Prior items:
BLOCKER (must fix): CI run 308 on this exact head has been pending for about 1h46m and is hung. Its log ends at the new "ci checkout helper regression" step with an SSH prompt: "The authenticity of host '127.0.0.1' can't be established ... ED25519 key fingerprint". Cause: scripts/test-ci-checkout.sh near the end uses git@127.0.0.1:1/none.git. scp-style URLs have no port field, so "1/none.git" is the path and ssh dials port 22 on the runner host, which has an sshd with an unknown host key and a tty. I reproduced the prompt locally under a pty and the exit 128 without one. The earlier ssh://...:1 form failed fast on a refused port; the scp form does not. A hung step will hold the runner slot and never report a status.
Requested fix:
a. Use a non-resolving host, for example git@ssh.invalid:none.git, and run the case with GIT_SSH_COMMAND='ssh -o BatchMode=yes -o ConnectTimeout=5 -o StrictHostKeyChecking=yes' (or an equivalent fake ssh that exits 255) so it cannot prompt or reach any real host. Keep the assertion that the exit status is not 2 and no directory remains.
b. Add timeout-minutes (for example 5) to the new check.yml step so any future hang fails instead of holding the runner.
c. Someone with Forgejo admin must cancel run 308; I have no token for that. Re-run CI on the fixed head.
Non-blocking:
Evidence saved at ~/.local/state/bread-ci/eco79/review-2026-10-16-second-pass.txt and eco-run308-job-partial.log. Local checks run on this head: test-ci-checkout.sh passed 9 consecutive runs without a tty.
Independent final re-review of PR 79 at exact head
8dabe3a2b1(base main94ea37490f). Verdict: PASS. Reviewed only the diff from9516cf6and reused the accepted criteria from comments 5272 and 5307.Blocker 5307 (ssh host-key hang) is fixed:
CI: check / check (push) is success on this exact head, run 310, 7m11s. The log shows the new helper regression step passing on the runner within seconds of checkout, then the full prior checks, "Job succeeded". Previous head
9516cf6run 308 was the hung one and is superseded.Residual, stated plainly: the workspace-sharing cause on the runner remains an inference; the consumer integration is covered by bread#49 (merged 40ee494), whose post-merge dev-release run still needs its own check. Other main-trigger workflows in this repo (dev-bakery, dev-bread-theme, package) keep the old pattern and are tracked in ECO-82.
Evidence: ~/.local/state/bread-ci/eco79/review-5307-repair.txt plus this review.
Merged after independent pass at head
8dabe3a2b1: check / check (push) green on that exact head (run 310), base unchanged at94ea37490f, no outstanding reviews, mergeable. Merge commitc6442cabb6(parents: base and reviewed head). Post-merge check on main (run 311) was pending at last read. ECO-79 stays open for verification: confirm run 311 and bread run 162 (dev release on main) succeed, and note the consumer still needs the pinned helper adoption plus ECO-81/ECO-82 follow-ups. No tags or versions created.