diff --git a/.github/workflows/benchmark_pages.yml b/.github/workflows/benchmark_pages.yml index d9511854da..4ff81d1e53 100644 --- a/.github/workflows/benchmark_pages.yml +++ b/.github/workflows/benchmark_pages.yml @@ -67,16 +67,28 @@ jobs: path: source fetch-depth: 0 filter: blob:none + - name: Recheck queued PR preview before loading the retained site + if: needs.preview.outputs.available == 'true' + id: preview-current + env: + GH_TOKEN: ${{ github.token }} + SOURCE_RUN: ${{ github.event.workflow_run.id }} + SOURCE_ATTEMPT: ${{ github.event.workflow_run.run_attempt }} + run: | + python3 source/tools/benchmark_preview_ci.py current --repository="${GITHUB_REPOSITORY}" \ + --run="${SOURCE_RUN}" --attempt="${SOURCE_ATTEMPT}" --output="${GITHUB_OUTPUT}" - uses: actions/checkout@v7 + if: needs.preview.outputs.available != 'true' || steps.preview-current.outputs.available == 'true' with: ref: coverage-pages path: site fetch-depth: 0 filter: blob:none - name: Expand retained benchmark inputs + if: needs.preview.outputs.available != 'true' || steps.preview-current.outputs.available == 'true' run: python3 source/tools/site_storage.py expand-benchmarks site/benchmarks - name: Verify and retain current PR benchmark JSON - if: needs.preview.outputs.available == 'true' + if: steps.preview-current.outputs.available == 'true' id: preview env: GH_TOKEN: ${{ github.token }} @@ -129,17 +141,20 @@ jobs: --root=site/benchmarks --artifacts=backfill --source=source-run.json \ --repository="${GITHUB_REPOSITORY}" - uses: actions/setup-node@v7 + if: needs.preview.outputs.available != 'true' || steps.preview.outputs.available == 'true' with: node-version: "24" - name: Build pinned benchmark renderer + if: needs.preview.outputs.available != 'true' || steps.preview.outputs.available == 'true' working-directory: source/tools/benchmark_web run: | npm ci --ignore-scripts --no-audit --no-fund npm run build - name: Refresh and retain stable benchmark references + if: needs.preview.outputs.available != 'true' || steps.preview.outputs.available == 'true' env: GH_TOKEN: ${{ github.token }} - PREVIEW: ${{ needs.preview.outputs.available }} + PREVIEW: ${{ steps.preview.outputs.available }} run: | set -euo pipefail git -C site config user.name 'github-actions[bot]' diff --git a/TODO.md b/TODO.md index 85711dc9ee..cba6bad8d1 100644 --- a/TODO.md +++ b/TODO.md @@ -163,6 +163,9 @@ scope or declaring new flags implemented. See ## Pages deployment size +- [x] Recheck queued PR preview freshness after acquiring the writer lock; skip site loading for + obsolete heads/attempts and skip rendering if collection discovers a later race. Preserve + normal main/release/backfill/manual reconciliation and all retained measurements. - [x] Separate retained-site writers from latest-snapshot deployment; coalesce deployment notifications only, preserve every source ingestion, and update current PR comments only after verifying the deployed snapshot. diff --git a/docs/benchmark-comparisons.md b/docs/benchmark-comparisons.md index b1c9965a21..5c11fc15dc 100644 --- a/docs/benchmark-comparisons.md +++ b/docs/benchmark-comparisons.md @@ -85,6 +85,12 @@ branch head and tested merge commit, the base parent, and artifact identity. Onl from `main` executes with publication permissions. Uploaded HTML is ignored; bounded JSON is validated and rendered again. Waiting happens outside the shared writer lock. Preview updates reuse merged pages and their catalog instead of rebuilding every historical chart. +After acquiring the writer lock, a queued PR preview rechecks the PR head, repository and source +attempt before loading the retained site. An obsolete preview skips site checkout, expansion, +artifact collection and rendering. Collection checks again after that admission check; if the PR +becomes obsolete during loading, renderer setup and publication also skip. Current previews still +receive the full artifact/provenance validation. Main, release, backfill and manual publications +retain their normal processing and metadata refresh. Temporary GitHub read failures retry with bounded backoff; comment creation and updates are never blindly retried. diff --git a/docs/performance-analysis.md b/docs/performance-analysis.md index e1aa8e1fa8..f149ce8f7d 100644 --- a/docs/performance-analysis.md +++ b/docs/performance-analysis.md @@ -4952,3 +4952,21 @@ rejections require the separate deployment/recovery work; this optimization cann recover missing source events. A sampled 728 MB site also exceeds the existing 200 MB storage-review threshold. Keep historical rendering and storage reduction open without dropping release/main observations or coverage aggregates. + +## Queued preview admission, 2026-11-07 + +A benchmark preview can become obsolete after its waiter succeeds but before the +serialized writer starts. The old workflow still checked out and expanded the retained +site, then built the renderer and refreshed reports after collection returned no data. +The recorded example rendered zero preview pages and pushed no changes, while paying +for the full preparation/compaction interval. This is unnecessary work rather than an +executable-performance change. + +Check current PR head, repository and source attempt again inside the writer job, +before retained-site checkout. An explicitly obsolete preview skips that loading and +all later preview work. If it becomes obsolete during site loading, collection's +existing final check suppresses renderer setup and publication. Main/release/backfill/ +manual metadata reconciliation continues normally. API failures still fail rather +than masquerading as obsolete data. Fresh previews pay one bounded source/PR API check; +no fixed elapsed-time improvement is claimed. Old queued workflow definitions retain +their original steps, and rejected producer events still need independent recovery. diff --git a/tools/benchmark_history_test.py b/tools/benchmark_history_test.py index 3c053f8afc..c5c58905e4 100644 --- a/tools/benchmark_history_test.py +++ b/tools/benchmark_history_test.py @@ -224,6 +224,32 @@ def test_workflow_keeps_measurement_unprivileged_and_publication_serialized(self self.assertNotIn('shards/*/*.json', measure) self.assertIn("git -C site add --all", publish) + def test_obsolete_queued_previews_skip_site_loading_and_collection_races_skip_rendering(self): + workflow = repository_file('.github/workflows/benchmark_pages.yml').read_text() + publish = workflow.split(' publish:', 1)[1] + admission = "needs.preview.outputs.available != 'true' || steps.preview-current.outputs.available == 'true'" + collected = "needs.preview.outputs.available != 'true' || steps.preview.outputs.available == 'true'" + check = publish.split(' - name: Recheck queued PR preview before loading the retained site', 1)[1] + check = check.split('\n - ', 1)[0] + self.assertIn("if: needs.preview.outputs.available == 'true'", check) + self.assertIn('id: preview-current', check) + self.assertIn('benchmark_preview_ci.py current', check) + self.assertIn('--output="${GITHUB_OUTPUT}"', check) + self.assertLess(publish.index('id: preview-current'), publish.index('ref: coverage-pages')) + self.assertIn(' - uses: actions/checkout@v7\n if: ' + admission + '\n with:\n' + ' ref: coverage-pages', publish) + for marker, condition in ( + ('name: Expand retained benchmark inputs', admission), + ('name: Verify and retain current PR benchmark JSON', "steps.preview-current.outputs.available == 'true'"), + ('uses: actions/setup-node@v7', collected), + ('name: Build pinned benchmark renderer', collected), + ('name: Refresh and retain stable benchmark references', collected)): + with self.subTest(step=marker): + step = publish.split(' - ' + marker, 1)[1].split('\n - ', 1)[0] + self.assertIn('if: ' + condition, step) + self.assertIn('PREVIEW: ${{ steps.preview.outputs.available }}', publish) + self.assertNotIn('PREVIEW: ${{ needs.preview.outputs.available }}', publish) + def test_fifth_candidate_requires_unresolved_fourth_agreement(self): workflow = repository_file('.github/workflows/benchmarks.yml').read_text() for platform_name in ('linux', 'macos'): diff --git a/tools/benchmark_preview_ci.py b/tools/benchmark_preview_ci.py index c217bef8bf..aaf3d38e27 100644 --- a/tools/benchmark_preview_ci.py +++ b/tools/benchmark_preview_ci.py @@ -215,7 +215,7 @@ def update_retained_comments(api, root, site_url): def main(): parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument('operation', choices=('wait', 'collect', 'comment', 'comment-retained')) + parser.add_argument('operation', choices=('wait', 'current', 'collect', 'comment', 'comment-retained')) parser.add_argument('--repository', required=True) parser.add_argument('--run', type=int) parser.add_argument('--attempt', type=int) @@ -233,6 +233,9 @@ def main(): parser.error('comment-retained selects source runs from the deployed snapshot') if args.operation == 'wait': available = wait(api, args.run, args.attempt) + elif args.operation == 'current': + available = context(api, args.run, args.attempt) is not None + print(f'Benchmark preview {args.run}/{args.attempt}: {"current" if available else "obsolete"}', flush=True) elif args.root is None: parser.error('--root is required') elif args.operation == 'collect': diff --git a/tools/benchmark_preview_ci_test.py b/tools/benchmark_preview_ci_test.py index 583dacb1a0..647b708f44 100644 --- a/tools/benchmark_preview_ci_test.py +++ b/tools/benchmark_preview_ci_test.py @@ -40,6 +40,44 @@ def artifacts(): class BenchmarkPreviewCiTest(unittest.TestCase): + def test_queued_preview_check_rejects_stale_source_without_loading_site_or_artifacts(self): + cases = ( + ('current', source(), pull(), True), + ('new-attempt', dict(source(), run_attempt=3), pull(), False), + ('new-head', source(), dict(pull(), head={'sha': 'd' * 40, 'repo': {'id': 2}}), False), + ('closed', source(), dict(pull(), state='closed'), False), + ('wrong-repository', source(), dict(pull(), head={'sha': 'b' * 40, 'repo': {'id': 3}}), False), + ) + for name, run, pr, available in cases: + with self.subTest(case=name), tempfile.TemporaryDirectory() as directory: + output = Path(directory) / 'output' + api = mock.Mock(repository='owner/repo') + api.request.side_effect = lambda path: run if path == 'actions/runs/10' else pr + arguments = ['preview', 'current', '--repository=owner/repo', '--run=10', '--attempt=2', + '--output=' + str(output)] + with mock.patch.object(sys, 'argv', arguments), mock.patch.object(ci, 'GitHub', return_value=api), \ + mock.patch.object(ci, 'collect') as collect, mock.patch.object(ci, 'wait') as wait: + ci.main() + self.assertEqual(output.read_text(), f'available={str(available).lower()}\n') + api.pages.assert_not_called() + collect.assert_not_called() + wait.assert_not_called() + self.assertEqual(api.request.call_count, 1 if name == 'new-attempt' else 2) + self.assertEqual(list(Path(directory).iterdir()), [output]) + + def test_queued_preview_api_failure_is_an_error_not_an_obsolete_result(self): + with tempfile.TemporaryDirectory() as directory: + output = Path(directory) / 'output' + arguments = ['preview', 'current', '--repository=owner/repo', '--run=10', '--attempt=2', + '--output=' + str(output)] + api = mock.Mock() + failure = subprocess.CalledProcessError(1, ['gh'], stderr=b'gh: HTTP 503\n') + api.request.side_effect = failure + with mock.patch.object(sys, 'argv', arguments), mock.patch.object(ci, 'GitHub', return_value=api), \ + self.assertRaises(subprocess.CalledProcessError): + ci.main() + self.assertFalse(output.exists()) + def test_coalesced_deployment_comments_on_every_current_pr_once(self): with tempfile.TemporaryDirectory() as directory: root = Path(directory) @@ -79,7 +117,7 @@ def request(path, *args): self.assertFalse(any(len(call.args) > 1 for call in api.request.call_args_list)) def test_source_operations_still_require_both_run_and_attempt(self): - for operation in ('wait', 'collect', 'comment'): + for operation in ('wait', 'current', 'collect', 'comment'): for arguments in ([], ['--run=10'], ['--attempt=2']): with self.subTest(operation=operation, arguments=arguments), \ mock.patch.object(sys, 'argv', ['preview', operation, '--repository=owner/repo', *arguments]), \