Merge concurrent experiment state rebases in push_experiments_state - #49631
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add new file format that uses .jsonl file to avoid merge issues. Support both old and new format: |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 5b26586. Added |
|
@copilot the jsonl file should be a ledger of workflow run id , experiment variant info etc... |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 4b9b3c2. |
There was a problem hiding this comment.
Pull request overview
Adds conflict recovery for concurrent experiment-state pushes and migrates experiment history to an append-only JSONL ledger.
Changes:
- Adds rebase conflict hooks and experiment-state merge logic.
- Adds JSON/JSONL migration, loading, reporting, and tests.
- Recompiles experiment-enabled workflow locks for
state.jsonl.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_experiments.go |
Configures JSONL experiment state. |
pkg/cli/experiments_command.go |
Parses and reports JSONL ledgers. |
pkg/cli/experiments_command_test.go |
Tests JSONL CLI parsing. |
pkg/cli/audit_report_experiments.go |
Reads JSONL audit artifacts. |
pkg/cli/audit_report_experiments_test.go |
Tests JSONL audit extraction. |
actions/setup/js/push_signed_commits.cjs |
Adds rebase conflict resolution hook. |
actions/setup/js/push_signed_commits.test.cjs |
Tests concurrent rebase recovery. |
actions/setup/js/push_experiment_state.cjs |
Merges JSON and JSONL conflicts. |
actions/setup/js/push_experiment_state.test.cjs |
Tests field-level state merging. |
actions/setup/js/pick_experiment.cjs |
Implements JSONL ledger persistence. |
actions/setup/js/pick_experiment.test.cjs |
Tests JSONL loading and migration. |
actions/setup/js/load_experiment_state_from_repo.cjs |
Loads JSONL with JSON fallback. |
actions/setup/js/load_experiment_state_from_repo.test.cjs |
Tests repository-state compatibility. |
.github/workflows/weekly-blog-post-writer.lock.yml |
Uses JSONL experiment state. |
.github/workflows/typist.lock.yml |
Uses JSONL experiment state. |
.github/workflows/test-quality-sentinel.lock.yml |
Uses JSONL experiment state. |
.github/workflows/smoke-temporary-id.lock.yml |
Uses JSONL experiment state. |
.github/workflows/smoke-project.lock.yml |
Uses JSONL experiment state. |
.github/workflows/smoke-gemini.lock.yml |
Uses JSONL experiment state. |
.github/workflows/smoke-copilot.lock.yml |
Uses JSONL experiment state. |
.github/workflows/smoke-copilot-sub-agents.lock.yml |
Uses JSONL experiment state. |
.github/workflows/smoke-copilot-aoai-entra.lock.yml |
Uses JSONL experiment state. |
.github/workflows/smoke-copilot-aoai-apikey.lock.yml |
Uses JSONL experiment state. |
.github/workflows/smoke-checkout-pr-dispatch.lock.yml |
Regenerates runtime settings. |
.github/workflows/smoke-antigravity.lock.yml |
Uses JSONL experiment state. |
.github/workflows/plan.lock.yml |
Uses JSONL experiment state. |
.github/workflows/issue-arborist.lock.yml |
Uses JSONL experiment state. |
.github/workflows/gpclean.lock.yml |
Uses JSONL experiment state. |
.github/workflows/dependabot-go-checker.lock.yml |
Uses JSONL experiment state. |
.github/workflows/deep-report.lock.yml |
Uses JSONL experiment state. |
.github/workflows/dataflow-pr-discussion-dataset.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-semgrep-scan.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-security-red-team.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-safe-output-optimizer.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-news.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-issues-report.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-fact.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-doc-updater.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-doc-healer.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-compiler-quality.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-community-attribution.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-code-metrics.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-caveman-optimizer.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-cache-strategy-analyzer.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-astrostylelite-markdown-spellcheck.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-arxiv-researcher.lock.yml |
Regenerates model-routing settings. |
.github/workflows/daily-architecture-diagram.lock.yml |
Uses JSONL experiment state. |
.github/workflows/daily-agentrx-trace-optimizer.lock.yml |
Uses JSONL experiment state. |
.github/workflows/copilot-agent-analysis.lock.yml |
Uses JSONL experiment state. |
.github/workflows/ci-coach.lock.yml |
Uses JSONL experiment state. |
.github/workflows/breaking-change-checker.lock.yml |
Uses JSONL experiment state. |
.github/workflows/blog-auditor.lock.yml |
Uses JSONL experiment state. |
.github/workflows/aw-failure-investigator.lock.yml |
Uses JSONL experiment state. |
.github/workflows/audit-workflows.lock.yml |
Uses JSONL experiment state. |
.github/workflows/architecture-guardian.lock.yml |
Uses JSONL experiment state. |
.github/workflows/agent-persona-explorer.lock.yml |
Uses JSONL experiment state. |
.github/workflows/agent-performance-analyzer.lock.yml |
Uses JSONL experiment state. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (3)
actions/setup/js/push_experiment_state.cjs:125
- Full-record deduplication corrupts counts during the supported legacy migration race. If the base
state.jsonhas counts but no runs, each concurrent writer migrates it by attaching the samebaseline_countsto its own distinct run record; this union retains both records, and every reader adds the baseline twice. The JSONL conflict merge needs base-aware baseline reconciliation, or migration must emit a common deduplicable baseline record.
actions/setup/js/push_experiment_state.cjs:110 - This always appends the complete local ledger after the remote ledger, so concurrent runs can leave
state.jsonlout of timestamp order. Consumers use the final record as the latest run; when the remote run is newer than the local run (as in the integration test), they report the older local assignment. Preserve chronological run order during the union.
actions/setup/js/push_experiment_state.cjs:70 - Concurrent increments are still lost when the counter did not exist in the base state. For a newly added experiment/variant, both writers can produce
1from an absent base key; becausebaseValueis not finite this falls through to the equal-values case and returns1instead of2. Treat an absent base numeric counter as zero for the additive merge.
- Files reviewed: 57/57 changed files
- Comments generated: 4
- Review effort level: Balanced
| // experimentStateFile is the path to the experiment state JSON written by pick_experiment.cjs. | ||
| const experimentStateFile = experimentsCacheDir + "/state.json" | ||
| // experimentStateFile is the path to the experiment run-ledger JSONL file written by pick_experiment.cjs. | ||
| const experimentStateFile = experimentsCacheDir + "/state.jsonl" |
| const latestRun = runs[runs.length - 1]; | ||
| fs.appendFileSync(stateFile, `${JSON.stringify(latestRun)}\n`, "utf8"); | ||
| return; |
| if err := json.Unmarshal(data, &state); err == nil { | ||
| if state.Counts == nil { | ||
| state.Counts = map[string]map[string]int{} | ||
| } | ||
| return &state | ||
| } |
| for (const run of [...remoteRuns, ...localRuns]) { | ||
| const key = | ||
| isPlainObject(run) && typeof run.run_id === "string" && typeof run.timestamp === "string" && isPlainObject(run.assignments) | ||
| ? `${run.run_id}\u0000${run.timestamp}\u0000${stableJSONStringify(run.assignments)}` | ||
| : stableJSONStringify(run); |
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30727625239
|
- Fix TypeScript error: add @type annotation to state.runs in loadState so tsc no longer infers never[] for the empty array literal (pick_experiment.cjs) - Fix cache-mode upgrade path: when state.jsonl is absent but state.json exists loadState now falls back to the legacy file (ENOENT-only, no swallowing of parse errors) - Fix Go parseExperimentState: only treat unmarshalled data as a snapshot when state.Counts != nil, so single-record JSONL files are correctly routed to the JSONL parser instead of being returned as empty snapshots - Sort merged runs by timestamp (then run_id as tie-breaker) in both mergeExperimentRuns and mergeExperimentStateJSONL so ledger order is always chronological regardless of which writer submitted first - Compact ledger in mergeExperimentStateJSONL to MAX_LEDGER_RECORDS (512) entries; counts from pruned records are folded into the first remaining entry's baseline_counts so cumulative totals are preserved - Export mergeExperimentStateJSONL and mergeExperimentRuns for test coverage - Add tests: Go single-record JSONL, JS timestamp ordering, JS compaction, JS cache-mode loadState fallback Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on merge-correctness edge cases and missing observability.
📋 Key Themes & Highlights
Key Issues
- Silent last-writer-wins fallback (
push_experiment_state.cjs:105): non-numeric/non-object/non-array conflicts are swallowed with no log — add acore.warningand a regression test. - Additive counter can underflow (
push_experiment_state.cjs:84):remote + local - basecan produce a lower value than either writer if either side decremented or reset a counter — consider flooring atMath.max(additive, remote, local)and documenting the invariant. - New-key conflict is under-specified (
push_experiment_state.cjs:98): when both writers add the same key with different values (base = undefined), the merge silently picks local — no test documents the expected result. - Negative counts not validated (
push_experiment_state.cjs:113):mergeExperimentStateJSONcould pass counts that went negative through the additive merge; add a post-merge guard. - Silent resolver decline (
push_experiment_state.cjs:220):resolveExperimentStateRebaseConflictreturnsfalsewithout logging when an unexpected file is in the conflict set — callers see a cryptic generic rebase error.
Positive Highlights
- ✅ Integration tests for the two-writer race in
push_signed_commits.test.cjsare thorough and well-structured. - ✅ The
mergeExperimentStateJSONLcompaction logic (baseline_counts rollup) is well thought out and tested. - ✅ Good use of
stableJSONStringifyfor deterministic deduplication across the merge helpers. - ✅ The
resolveRebaseConflicthook is a clean extension point inpushSignedCommits— keeps the core path agnostic of state format.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 79.6 AIC · ⌖ 11.4 AIC · ⊞ 7.1K
Comment /matt to run again
|
@copilot quick triage for this PR:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. Go parser updates:
Conclusion job update:
Tests added for all new behavior (JSONL skip-invalid-line resilience, usage summary fallback, and |
|
…in saveState Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in the latest commit. Ledger unbounded growth (reviewer feedback): Chronological order: Already fixed in c5d6f3a via JSON/JSONL disambiguation: Fixed in c5d6f3a — Go parsers + conclusion job: Done in b57f585 — Go now reads both |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. JS resilience for JSONL merge conflicts:
Go experiments command — both file formats:
|
PR Triage
Automated triage — see full report issue for details. Structured data: {
"action": "defer",
"category": "chore",
"pr_number": 49631,
"risk": "high"
}
|
|
@copilot This PR has merge conflicts. Please run Failed check still listed in compact triage:
|
…ix-push-experiments-state # Conflicts: # actions/setup/js/generate_usage_activity_summary.test.cjs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in aa3befa. Merged origin/main and resolved the conflict in |
|
🎉 This pull request is included in a new release. Release: |
push_experiments_statewas treatingstate.jsonrebase conflicts as terminal failures, which caused concurrent runs writing to the sameexperiments/<workflow>branch to drop state updates. This change makes that shared path merge experiment state during rebase instead of aborting on expected writer races.Rebase conflict recovery in the shared push path
Field-level merge for experiment state
push_experiment_stateto resolvestate.jsonas a 3-way JSON merge using base/remote/local versions from the rebase index.runshistory is unioned/deduplicated so both runs remain represented.Assignments handling
assignments.jsonis part of the same conflict set, the resolver keeps the local/current-run version while continuing the rebase.Coverage for the race
Example of the merge behavior: