Remove push_repo_memory concurrency group so fan-out memory writes are not dropped - #62414
Conversation
…ries Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A late compare-and-swap race can create merge history rejected by the signed-commit path, preventing reliable convergence.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Removes lossy push_repo_memory concurrency groups to address #61644 and strengthens optimistic retry handling.
Changes:
- Removes generated concurrency groups.
- Adds capped, jittered retries and regression tests.
- Updates documentation, changeset, and compiled workflows.
| File | Description |
|---|---|
pkg/workflow/repo_memory.go |
Removes concurrency generation. |
pkg/workflow/repo_memory_test.go |
Tests concurrency omission. |
docs/src/content/docs/reference/repo-memory.md |
Documents concurrent-write behavior. |
actions/setup/js/push_repo_memory.test.cjs |
Adds retry configuration guards. |
actions/setup/js/push_repo_memory.cjs |
Expands and jitters retries. |
.github/workflows/workflow-health-manager.lock.yml |
Removes generated concurrency. |
.github/workflows/weekly-blog-post-writer.lock.yml |
Removes generated concurrency. |
.github/workflows/technical-doc-writer.lock.yml |
Removes generated concurrency. |
.github/workflows/smoke-ci.lock.yml |
Removes generated concurrency. |
.github/workflows/sergo.lock.yml |
Removes generated concurrency. |
.github/workflows/security-compliance.lock.yml |
Removes generated concurrency. |
.github/workflows/pr-triage-agent.lock.yml |
Removes generated concurrency. |
.github/workflows/metrics-collector.lock.yml |
Removes generated concurrency. |
.github/workflows/glossary-maintainer.lock.yml |
Removes generated concurrency. |
.github/workflows/firewall-escape.lock.yml |
Removes generated concurrency. |
.github/workflows/eslint-refiner.lock.yml |
Removes generated concurrency. |
.github/workflows/developer-docs-consolidator.lock.yml |
Removes generated concurrency. |
.github/workflows/delight.lock.yml |
Removes generated concurrency. |
.github/workflows/deep-report.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-testify-uber-super-expert.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-storify.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-safeoutputs-git-simulator.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-news.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-harness-experiment-proposer.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-formal-spec-verifier.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-community-attribution.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-code-metrics.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-cli-performance.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-awf-spec-compiler-surfacing.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-arxiv-researcher.lock.yml |
Removes generated concurrency. |
.github/workflows/daily-agent-of-the-day-blog-writer.lock.yml |
Removes generated concurrency. |
.github/workflows/copilot-session-insights.lock.yml |
Removes generated concurrency. |
.github/workflows/copilot-pr-prompt-analysis.lock.yml |
Removes generated concurrency. |
.github/workflows/copilot-pr-nlp-analysis.lock.yml |
Removes generated concurrency. |
.github/workflows/copilot-cli-deep-research.lock.yml |
Removes generated concurrency. |
.github/workflows/copilot-centralization-optimizer.lock.yml |
Removes generated concurrency. |
.github/workflows/copilot-agent-analysis.lock.yml |
Removes generated concurrency. |
.github/workflows/audit-workflows.lock.yml |
Removes generated concurrency. |
.github/workflows/agentic-token-optimizer.lock.yml |
Removes generated concurrency. |
.github/workflows/agentic-token-audit.lock.yml |
Removes generated concurrency. |
.github/workflows/agent-performance-analyzer.lock.yml |
Removes generated concurrency. |
.changeset/push-repo-memory-fanout.md |
Records the patch release. |
| const ceiling = Math.min(MAX_DELAY_MS, BASE_DELAY_MS * Math.pow(2, attempt)); | ||
| const delay = Math.floor(Math.random() * ceiling) + 1; |
There was a problem hiding this comment.
| if (attempt < MAX_RETRIES) { | ||
| const delay = BASE_DELAY_MS * Math.pow(2, attempt); | ||
| const ceiling = Math.min(MAX_DELAY_MS, BASE_DELAY_MS * Math.pow(2, attempt)); | ||
| const delay = Math.floor(Math.random() * ceiling) + 1; | ||
| core.warning(`Push failed (attempt ${attempt + 1}/${MAX_RETRIES + 1}), retrying in ${delay}ms: ${errMsg}`); |
There was a problem hiding this comment.
Addressed in 78c70da. ERR_VALIDATION push failures are now treated as deterministic and fail immediately instead of consuming the extended retry/backoff budget.
| const maxRetriesMatch = scriptContent.match(/const MAX_RETRIES = (\d+);/); | ||
| expect(maxRetriesMatch).not.toBeNull(); | ||
| expect(Number(maxRetriesMatch[1])).toBeGreaterThanOrEqual(10); | ||
|
|
||
| // Backoff must be capped and jittered so retries of many runs spread out. | ||
| expect(scriptContent).toContain("const MAX_DELAY_MS"); | ||
| expect(scriptContent).toContain("Math.min(MAX_DELAY_MS, BASE_DELAY_MS * Math.pow(2, attempt))"); | ||
| expect(scriptContent).toContain("Math.random() * ceiling"); |
There was a problem hiding this comment.
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
🔬 Test Quality Sentinel is analyzing test quality on this pull request... |
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
No ADR enforcement needed: PR does not have the implementation label and has <=100 new lines of code in business logic directories.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Impeccable review — harden + audit (bug_fix)
The core change (dropping the per-branch concurrency group from push_repo_memory and relying on the existing compare-and-swap + retry loop) is correct and well-justified: GitHub Actions only queues one pending job per group, so the old group silently dropped fan-out writes. The Go and doc changes are clean; no dead references to the removed buildPushRepoMemoryConcurrencyGroup/encodeConcurrencyKeyPart remain, and the new TestPushRepoMemoryJobHasNoConcurrencyGroup test correctly asserts the field is empty.
Independently reviewing actions/setup/js/push_repo_memory.cjs, I confirm the three issues already raised in existing review comments are valid and worth addressing before merge:
- Retrying permanent errors wastes time (line ~746):
ERR_VALIDATION-prefixed errors (unsupported commit shapes, policy violations) frompushSignedCommits/push_signed_commits.cjsare deterministic and will fail identically on every attempt, so retrying them up to 10 times just delays the inevitablesetFailedby up to ~2 minutes. - Merge-commit fallback loses signing on retry (line ~745): when the optimistic push loses the race, the recovery path does
git pull --no-rebase -X ours, which can create a two-parent merge commit;pushSignedCommits's pre-flight explicitly refuses to sign merge commits, so a retry after a real race can fall back to an unsigned push under repos with signed-commit rulesets. - New vitest guard is source-text-only (line ~1920): it doesn't exercise the actual convergence path (no stubbed failing push / mocked
Math.random/ fake timers), so a regression in the retry logic itself wouldn't be caught.
No new blocking issues found beyond these. Given they're already tracked in open review threads, I'm not duplicating inline comments.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 53.6 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd. The root-cause analysis (GitHub Actions cancels all but one pending job per concurrency group under fan-out) is well-diagnosed and clearly documented in the code comment and PR description, with quantified before/after evidence (11/50 → 50/50 transactions persisted). Requesting changes on a couple of gaps in the follow-through.
📋 Key Themes & Highlights
Key Themes
- No job timeout set on
push_repo_memory(pkg/workflow/repo_memory.go): the retry loop's new worst case is ~2 minutes of jittered backoff across 10 attempts, up from the old fixed 3-retry bound. Worth an explicittimeout-minutesso the new worst case is visible rather than implicit. - Test coverage is source-pattern-based, not behavioral (
push_repo_memory.test.cjsnew guard, and the GoTestPushRepoMemoryJobHasNoConcurrencyGroup): both are solid regression guards for the specific values changed (MAX_RETRIES, MAX_DELAY_MS, absence ofConcurrency), but neither exercises the actual convergence path (a stubbed compare-and-swap loss driving a real retry+merge). This matches a pre-existing review comment on line 1920 of the test file that I'd endorse addressing. - Docs: the "one file per writer" mitigation is now the primary defense against lossy merges under heavy fan-out and would benefit from a concrete example rather than a single prose sentence.
- Two existing unaddressed review comments (merge-commit-on-retry risk, and retrying permanent
ERR_VALIDATIONfailures with the same 11-attempt budget) remain relevant and should be resolved before merge — they identify real correctness/efficiency gaps in the new retry path, not just style nits.
Positive Highlights
- ✅ Clear, precise root-cause comment left at the call site explaining the GitHub Actions semantics and the convergence strategy — this is exactly the kind of instrumentation
/diagnosing-bugscalls for. - ✅ Full-jitter capped backoff is the right primitive for spreading out simultaneous finishers, and the retry budget increase is justified by the fan-out math in the PR description.
- ✅ Dead code (
buildPushRepoMemoryConcurrencyGroup,encodeConcurrencyKeyPart, unusedsortimport) was cleanly removed alongside the behavioral change. - ✅ Docs, changeset, and lock-file regeneration are all consistent with the code change — no stray diffs.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 112.6 AIC · ⌖ 13.7 AIC · ⊞ 10.1K
Comment /matt to run again
| return &Job{ | ||
| Name: pushRepoMemoryJobName, | ||
| DisplayName: "", | ||
| RunsOn: c.formatFrameworkJobRunsOn(data), |
There was a problem hiding this comment.
[/diagnosing-bugs] No timeout-minutes is set on this job, and the retry loop's worst case is ~2 minutes of jittered backoff across 10 attempts plus network/API latency per attempt. If the default runner/job timeout is generous this is fine, but worth confirming explicitly rather than relying on an implicit ceiling, since a stuck push (e.g. persistent GraphQL rate-limiting) could now run far longer than the old 3-retry version before failing.
💡 Why this matters
The PR's own root-cause analysis is precise (GH keeps 1 running + 1 pending per concurrency group), which is the right level of rigor for a /diagnosing-bugs-style fix. The remaining risk is the inverse failure mode: previously a stuck job failed fast (3 retries, fixed backoff); now it can legitimately spend minutes retrying against transient conditions before giving up. A short comment or explicit timeout-minutes on this job would make the new worst-case bound visible in the compiled workflow rather than left to GHA/runner defaults.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 78c70da. push_repo_memory now emits timeout-minutes: 10, and the workflow locks were recompiled so the retry ceiling is explicit in generated jobs.
| // declare a concurrency group. GitHub Actions only queues one pending job per group, so a | ||
| // shared group silently cancels memory writes when many runs finish at once; the push script | ||
| // converges with a compare-and-swap retry loop instead. | ||
| func TestPushRepoMemoryJobHasNoConcurrencyGroup(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] This new test only asserts pushJob.Concurrency is empty — a fine regression guard, but it doesn't cover the behavior the PR claims to fix (that fan-out writes now converge). Consider a companion test simulating N concurrent buildPushRepoMemoryJob calls plus a JS-side integration test that stubs multiple retries racing on the same branch, so the "50/50 persisted" claim in the PR description has automated coverage, not just a manual re-run.
💡 Suggested direction
Go-side: this test can stay as-is (compiler output shape). The stronger coverage belongs in push_repo_memory.test.cjs, exercising the retry loop with a stubbed pushSignedCommits that fails N times with a stale-head error and succeeds on retry after refreshing baseRef — verifying the loop actually converges rather than only checking source-text patterns (as flagged by the existing review comment on line 1920 of that file).
@copilot please address this.
| - **Validation or patch-size failures**: Keep changes within `file-glob`, `max-file-size` (100KB default), `max-file-count` (100 default), and `max-patch-size` (10KB default). | ||
| - **Changes not persisting**: Confirm the directory path, let the workflow finish, and check the logs for push errors. | ||
| - **Merge conflicts**: Concurrent pushes are replayed onto the latest remote state, so your file changes win. | ||
| - **Merge conflicts**: Concurrent pushes are replayed onto the latest remote state, so your file changes win. `.jsonl` files are merged with `merge=union`, so rows written by concurrent runs are all kept; other file types keep the local version. |
There was a problem hiding this comment.
[/grill-with-docs] Good addition, but consider making the "one file per writer" recommendation more concrete (e.g. a short example of naming a memory file per worker/matrix-index) since this is now the primary mitigation for lossy merges in high fan-out scenarios, not just a nice-to-have. As written it's a single prose sentence without an example, while the rest of this troubleshooting section leans on concrete configuration values (100KB, 100 files, 10KB).
@copilot please address this.
There was a problem hiding this comment.
Addressed in 78c70da. The repo-memory troubleshooting docs now include concrete per-writer file examples such as runs/${{ matrix.worker }}.jsonl and targets/${{ matrix.target }}.jsonl.
|
Please address the open review follow-ups below, refresh the branch if needed, and then run the Open review follow-ups (newest first):
Failed checks:
I also requested a branch refresh for this PR. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 78c70da and d0abddc. The retry path now rebases onto refreshed heads without merge commits, deterministic |
|
🎉 This pull request is included in a new release. Release: |
GitHub Actions keeps at most one running and one pending job per concurrency group — every additional pending job is cancelled.
push_repo_memoryset a per-branch group, so when more than two runs sharing arepo-memorybranch finished close together, the extra push jobs were cancelled and their memory writes were silently lost whileagentreported success. A 50-worker fan-out writing 50 distinct files persisted only 11 transactions; 39 push jobs were cancelled withCanceling since a higher priority waiting request ... exists.cancel-in-progress: falseonly protects the running job; the single pending slot is still overwritten by each newcomer. No Actions setting queues more than one pending job, so any shared group is lossy under fan-out and a per-run unique key is equivalent to no group at all.Changes
pkg/workflow/repo_memory.go—buildPushRepoMemoryJobno longer setsConcurrency.buildPushRepoMemoryConcurrencyGroupandencodeConcurrencyKeyPartare deleted (unused), along with thesortimport. A comment at the former call site records the GitHub semantics and the convergence strategy.actions/setup/js/push_repo_memory.cjs— the retry loop now has to stand on its own, soMAX_RETRIESgoes 3 → 10 with capped full-jitter backoff. Thels-remote→currentBaseRefrefresh →configureRepoMemoryMergePolicy→pull --no-rebase -X oursre-sync is unchanged.TestPushRepoMemoryJobHasNoConcurrencyGroupreplaces the two key-format tests; a vitest guard parsesMAX_RETRIES(≥ 10) and asserts the cap and jitter expressions.merge=union, on behavior when many runs finish at once, and recommending one file per writer; apatchchangeset; recompiled workflows, whose only diff is the droppedconcurrency:block.The convergence primitives were already in place —
createCommitOnBranchis a compare-and-swap onexpectedHeadOid, and rejection triggers a remote-head refresh and a*.jsonl merge=unionpull. The group was masking them behind a 3-attempt fixed backoff.10 attempts with a 20 s cap bounds the worst case at roughly two minutes. Re-running the 50-worker scenario against this change: 0/50 cancellations, all 50 transactions persisted, 43 succeeding on the first attempt and the rest within three.
Exposing the retry count in frontmatter was considered and dropped to keep the change minimal.
Review note
The new vitest guard asserts against raw source text, matching the adjacent pre-existing
BASE_DELAY_MS * Math.pow(2, attempt)assertion. A behavioral test driving the loop with a stubbed failing push, mockedMath.random, and fake timers would be stronger and is a reasonable follow-up.push_repo_memorysilently drops memory writes under fan-out #61644pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/35645137146
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
hub.lumenfield.workTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.