ci: refold the metrics push instead of rebasing it (#126) - #127
Conversation
A `ci-data` push race was resolved with `git pull --rebase … || true`. That works only for files that cannot conflict. `runs/*.ndjson` is unique per leg and qualifies; `tested-versions.json` and `attempted-legs.json` are shared rollups every concurrent run rewrites, and they conflict as a matter of course. When they did, the rebase stopped with unmerged files and `|| true` swallowed it, so every later attempt died on "Pulling is not possible because you have unmerged files" — all four attempts burned in 1.1 s with no backoff. Run 30760428239 (B8a of #110) completed a leg cleanly and still lost its metrics that way: the ndjson was created and never landed. Resolve the race by recomputing instead of merging. On a failed push, abort any conflicted rebase/merge unconditionally, reset the worktree to the freshly fetched head, re-run the fold, re-commit and retry, with real backoff between attempts. Both writers are read-modify-write keyed by identity, so re-applying one run's contribution on top of the newest tree yields the union with no textual merge at all. That property was load-bearing and untested. Extract it as `foldLedgerInto` and pin it with anchor tests: another run's entry survives, a repeat fold is idempotent, the input map is not mutated, and keys stay sorted. A writer that replaced rather than merged would turn this retry into silent data loss — the exact failure the ledger exists to prevent — so the doc says any future shared file added to this push must fold the same way. Verified with a local two-clone race harness: the old loop reproduces the CI log exactly (conflict, then two "unmerged files" failures, ~1 s, loser's data absent from origin); the new loop lands both runs' keys and both ndjson files. Also records B8a's durable findings in ci.instructions.md: hosted macos-15-intel is 4 CPUs / 14336 MiB (measured across four runners) and is faster per file than the 8-core/64 GiB laptop, plus the per-file macos-x86 costs — with an explicit note NOT to fold them into OBSERVED_MAX_S, since they are bounded-group runs and the caps stay deliberately generous while #76 is open. No CHANGELOG entry — CI-internal, nothing user-facing, same as B4/B5. Refs #110, #124, #76. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe aggregate workflow now handles shared ChangesCI data refold retries
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AggregateJob
participant CiDataBranch
participant FoldLedgerInto
AggregateJob->>CiDataBranch: push metrics and ledger rollups
CiDataBranch-->>AggregateJob: report push conflict
AggregateJob->>CiDataBranch: fetch and reset to latest head
AggregateJob->>FoldLedgerInto: refold current run ledger
FoldLedgerInto-->>AggregateJob: return sorted merged ledger
AggregateJob->>CiDataBranch: retry push with backoff
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a CI metrics publishing race in the ci-data branch update path by switching the retry strategy from “rebase and retry” to “reset to remote head, refold, recommit, retry”, ensuring shared rollup files can’t get stuck in a conflicted rebase state. It also adds unit tests that pin the “fold-by-identity” behavior the new retry strategy relies on, and updates CI documentation with durable findings from B8a.
Changes:
- Replace the
ci-datapush retry logic inintegration.ymlwith a refold-based retry loop (abort rebase/merge, hard reset, rerun folds, backoff, retry push). - Extract
foldLedgerInto()inscripts/ci-leg-ledger.tsand add anchor tests validating merge/idempotency/sorted-key behavior. - Update
ci.instructions.mdwith macOS Intel runner resource facts and explicit guidance about the refold retry invariants.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
test/unit/ci-leg-ledger.test.ts |
Adds anchor tests for foldLedgerInto() to guarantee safe refolding semantics under concurrent pushes. |
scripts/ci-leg-ledger.ts |
Extracts foldLedgerInto() and uses it for writing attempted-legs.json deterministically. |
.github/workflows/integration.yml |
Reworks the ci-data push retry mechanism to refold on top of the latest remote head with backoff. |
.github/instructions/ci.instructions.md |
Documents the new refold retry rule and carries forward B8a’s durable platform findings. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/integration.yml:
- Around line 1180-1230: Update refold() to track failures from both
ci-metrics.ts aggregate and ci-leg-ledger.ts build, returning nonzero for any
failure; in the retry loop, do not stage or commit partial state when refold
fails, and continue/retry with the existing warning behavior. Update
stage_and_commit() and both callers to distinguish “nothing staged” from a git
commit failure, surfacing unexpected commit failures instead of treating them as
successful no-op exits.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 6765ef83-0bb1-4145-94e6-9ec0283f82c9
📒 Files selected for processing (4)
.github/instructions/ci.instructions.md.github/workflows/integration.ymlscripts/ci-leg-ledger.tstest/unit/ci-leg-ledger.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Unit Tests & Coverage
- GitHub Check: Unit Tests (windows-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In Bun-based TypeScript code, use
Bun.spawn(),Bun.write(),Bun.sleep(),bun:test, and ESM imports with.tsextensions.
Files:
scripts/ci-leg-ledger.tstest/unit/ci-leg-ledger.test.ts
**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.ts: Use Bun APIs and tooling rather than Node.js equivalents:Bun.spawn(),Bun.write(),Bun.sleep(),bun test, andbun:test. Use ESM with.tsextensions in imports; do not use CommonJS.
For ARM64virtmachines, never useif=virtiofor drives; use an explicit-device virtio-blk-pci,drive=drive0configuration.
When using HVF acceleration, use-cpu host, notcortex-a710.
For arm64 guests on macOS, automatically select TCG with-cpu cortex-a710; HVF cannot run the CHR image's 32-bit ARM userspace on Apple Silicon.--accelandQUICKCHR_ACCELmust override this selection for testing.
UEFI pflash code and vars units must be identical in size.
QGA is x86-only; do not assume the guest agent starts for arm64 CHR.
Use tabs for indentation.
Do not add unnecessary comments to obvious code.
Errors must be thrown asQuickCHRError(code, message, installHint?).
Preserve the documented public API types and behavior:QuickCHR.start(opts)returnsChrInstance;ChrInstanceprovidesstop(),remove(),rest(),monitor(),serial(), andqga(); andMachineStaterepresents persistedmachine.jsonstate.
Files:
scripts/ci-leg-ledger.tstest/unit/ci-leg-ledger.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not turn a red integration test green by broadening timeouts, skipping it, or platform-gating it before reproducing and root-causing the failure.
Files:
test/unit/ci-leg-ledger.test.ts
test/unit/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Unit tests must be fast and must not require QEMU.
Files:
test/unit/ci-leg-ledger.test.ts
🧠 Learnings (4)
📚 Learning: 2026-07-31T11:56:38.870Z
Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 117
File: scripts/ci-cache-key.ts:110-110
Timestamp: 2026-07-31T11:56:38.870Z
Learning: In Bun CI scripts under scripts/, use a plain Error for validation failures when the script catches the error and renders it as a GitHub ::error:: annotation. QuickCHRError is intended for programmatic library callers and is not required for this CLI-only error path.
Applied to files:
scripts/ci-leg-ledger.ts
📚 Learning: 2026-07-31T11:56:40.455Z
Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 117
File: scripts/ci-cache-key.ts:68-68
Timestamp: 2026-07-31T11:56:40.455Z
Learning: In Bun TypeScript files, do not flag use of `node:fs.appendFileSync` when append semantics are required and `Bun.write()` cannot provide them. This applies to cases such as appending multiple entries to `$GITHUB_OUTPUT` or writing to the boot log; use append-capable file operations rather than overwriting existing content.
Applied to files:
scripts/ci-leg-ledger.tstest/unit/ci-leg-ledger.test.ts
📚 Learning: 2026-07-28T00:12:59.340Z
Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 101
File: .github/workflows/integration.yml:342-342
Timestamp: 2026-07-28T00:12:59.340Z
Learning: For tikoci/quickchr, review workflow changes to ensure GitHub Actions are pinned to immutable commit SHAs as a repository-wide policy. If a PR introduces SHA pinning for only part of a workflow (or only some workflows) without extending the same policy across the relevant workflow files, flag it. When adding/changing SHA pinning, also verify Dependabot is configured to update the `github-actions` ecosystem so action version bumps are managed consistently (e.g., in the repo’s Dependabot configuration), rather than doing an isolated pinning change in an unrelated PR.
Applied to files:
.github/workflows/integration.yml
📚 Learning: 2026-07-31T11:56:27.561Z
Learnt from: mobileskyfi
Repo: tikoci/quickchr PR: 117
File: .github/workflows/integration.yml:188-188
Timestamp: 2026-07-31T11:56:27.561Z
Learning: For the tikoci/quickchr repository, do not request isolated SHA pinning of first-party GitHub Actions in a single workflow or pull request. Treat action pinning as repository-wide work: first configure Dependabot for the github-actions ecosystem, then mechanically pin all workflow action references, and finally decide whether to enforce zizmor in CI. Apply this guidance to workflow files while issue `#118` tracks the rollout.
Applied to files:
.github/workflows/integration.yml
🪛 LanguageTool
.github/instructions/ci.instructions.md
[grammar] ~285-~285: Ensure spelling is correct
Context: ...retry refolds; it never rebases** (#126). runs/*.ndjson is unique per leg an...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~293-~293: Consider using “who” when you are referring to a person instead of an object.
Context: ... it must fold the same way** — a writer that replaces rather than merges would make ...
(THAT_WHO)
🔇 Additional comments (7)
scripts/ci-leg-ledger.ts (2)
256-275: LGTM!
516-516: LGTM!test/unit/ci-leg-ledger.test.ts (1)
10-10: LGTM!Also applies to: 246-284
.github/workflows/integration.yml (1)
1082-1084: LGTM!.github/instructions/ci.instructions.md (3)
153-161: LGTM!
285-298: LGTM!
411-419: LGTM!
The push retry now describes tested-versions.json and attempted-legs.json together, which needed a plural the word list did not carry. Matches the existing explicit-inflection entries (normalise/normalises). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses both bot reviews on #127. CodeRabbit (major, valid): `refold()` returned non-zero only when the metrics aggregate failed. A failed `ci-leg-ledger build` fell through to a bare `echo` and returned 0, and the caller discarded the status anyway with `refold || true`. So a half-refold could stage whatever it did produce, commit it, push, and exit 0 — this run's runs/*.ndjson and tested-versions.json silently absent. That is #126's own failure shape one level up. `refold` now reports failure for either fold, and the loop retries instead of committing partial state. Their suggested `continue`, taken literally, introduces a worse bug. The reset above it discards this run's commit, so with nothing rebuilt HEAD equals the remote and `git push` prints "Everything up-to-date" and exits 0 — a false success hiding total loss. Demonstrated: with the loop otherwise identical and only the guard removed, a persistent refold failure prints "PUSH SUCCEEDED on attempt 2" with the run absent from origin. So the push is guarded by an explicit have_commit flag that only a clean refold sets. Copilot (valid): `fetch`/`reset` failures were swallowed by `|| true`. A stale FETCH_HEAD would rebuild the commit against the wrong base, and the "nothing left to record" fast-path could then wrongly claim another run carried the data. A failed fetch now keeps the existing commit and just retries the push; a failed reset drops have_commit and retries. Copilot (valid): sortKeysDeep's doc comment was left stranded above the extracted foldLedgerInto. Moved back. Also splits stage_and_commit's "nothing staged" (1) from "commit failed" (2), so an unexpected commit failure warns instead of exiting 0 quietly. Verified with the race harness extended to the failure paths: persistent refold failure ends in the loud warning with no false success and the winner's data intact; refold failing once then succeeding recovers on attempt 3 with both runs' keys and both ndjson files; the plain race still lands both on attempt 2. actionlint clean, the extracted step passes bash -n and shellcheck -S warning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both reviews had real findings — thanks. All three fixed in CodeRabbit (major) — partial refold could push partial data. Valid, fixed. But the suggested Demonstrated rather than argued. Same loop, only the guard removed, refold failing persistently: So the push is guarded by an explicit And refold failing once then recovering lands everything on attempt 3: CodeRabbit (secondary) — Copilot — Copilot — stranded doc comment. Valid, fixed.
|
|
@coderabbitai review The loop was materially restructured in |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline c219782
git show --format=fuller --find-renames --find-copies -- .github/workflows/integration.yml c219782
printf '\n--- workflow outline ---\n'
ast-grep outline .github/workflows/integration.yml --items all || true
printf '\n--- retry-related workflow lines ---\n'
rg -n -C 12 -e 'refold|stage_and_commit|have_commit|git (fetch|reset|push|commit)' .github/workflows/integration.ymlLength of output: 17479 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- aggregate workflow context ---'
sed -n '1090,1270p' .github/workflows/integration.yml
printf '%s\n' '--- pre-change aggregate workflow context ---'
git show c219782^:.github/workflows/integration.yml | sed -n '1160,1250p'
printf '%s\n' '--- ci-metrics and ci-leg-ledger entry points ---'
ast-grep outline scripts/ci-metrics.ts --items all
ast-grep outline scripts/ci-leg-ledger.ts --items all
printf '%s\n' '--- aggregate/build command handling ---'
rg -n -C 8 -e 'aggregate|process\.exit|throw |exitCode|writeFile|tested-versions|attempted-legs' scripts/ci-metrics.ts scripts/ci-leg-ledger.tsLength of output: 43980
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
Fixes #126. Also carries B8a's durable findings into
ci.instructions.md, as asked.The bug
The
ci-datapush retry resolved a race withgit pull --rebase … || true. That is safe only for files that cannot conflict.runs/*.ndjsonis unique per leg and qualifies — buttested-versions.jsonandattempted-legs.jsonare shared rollups every concurrent run rewrites, so runs landing together conflict as a matter of course.When they did, the rebase stopped with unmerged files and
|| trueswallowed it. Every later attempt then died before it started:All four attempts burned in 1.1 s — the loop had no backoff, and after the first conflict nothing could ever succeed. Run 30760428239 (B8a of #110) completed a leg cleanly and still lost its metrics: the log shows
create mode 100644 runs/30760428239-macos-x86-stable.ndjson, and the file is not on the branch.attempted-legs.jsonis B5's ledger — the file that records which legs never finished. A sweep where several legs die together is both the likeliest trigger and the case where the ledger matters most.The fix
Recompute instead of merge. On a failed push: abort any conflicted rebase/merge unconditionally (never assume the tree is clean on the way into a retry), reset to the freshly fetched head, re-run the fold, re-commit, retry — with real backoff.
This is correct because both writers are read-modify-write keyed by identity:
ci-leg-ledgerkeysattempted-legs.jsonby run id,ci-metricsfoldstested-versions.jsonby version/platform. Re-applying one run's contribution on top of the newest tree yields the union, with no textual merge anywhere.That property was load-bearing and untested
If either writer ever became a wholesale replace, this retry would start silently destroying the other run's data — precisely the failure the ledger exists to prevent. So it is now extracted as
foldLedgerIntoand pinned:ci.instructions.mdnow states the rule for the future: any new shared file added to this push must fold the same way.Verification
Reproduced locally rather than reasoned about — a two-clone race harness against a bare origin, with a fold that mirrors
ci-leg-ledger's read-modify-write.Old loop — reproduces the CI log exactly, and the loser's data is gone:
New loop — recovers on the next attempt, both runs intact:
Also:
actionlintclean (the twoSC2016infos it reports are pre-existing, at line 586, not this step), the extracted step passesbash -nandshellcheck -S warningstandalone, andbun test test/unit/is 950 pass / 0 fail withtsc --noEmitand Biome clean.Included per request: B8a durable knowledge
Two additions to
ci.instructions.md, both from the B8a report on #76:macos-15-intelis 4 CPUs / 14336 MiB, measured across four runners. That is half the cores and 22% of the RAM of the maintainer's Intel Mac — and it is nevertheless faster per file, so the size gap is not a reason to expect hosted legs to be slower. Boot there is 29.8–40.4 s under HVF over 29 boots. This closes the "the gap is uncharacterized" caveat CI: macos-x86 full suite wedges at position 10 in provisioning.test.ts — host-level runner loss, mechanism unexplained (HVF) #76 has carried since B7.macos-x86costs, with an explicit warning not to fold them intoOBSERVED_MAX_S: they are bounded-group runs, never a full suite, and the caps stay deliberately generous on the one platform under investigation. They are recorded because they answer a different question — the suite's own cost is ~30 min, not the 62–65 mincompleted_atreports.Scope
The
filter=allcheck-run gotcha B8a also hit needed no change —ci-leg-ledger.ts:300already documents and handles it. No CHANGELOG entry: CI-internal, nothing user-facing, matching B4/B5.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation