diff --git a/.github/scripts/pull-request-dashboard/delivery.py b/.github/scripts/pull-request-dashboard/delivery.py index 131f0ad8bd0..e0d215a67b3 100644 --- a/.github/scripts/pull-request-dashboard/delivery.py +++ b/.github/scripts/pull-request-dashboard/delivery.py @@ -21,6 +21,7 @@ from pr_status_comment import update_status_comments_from_state from state import ( author_nudge_state_path, + claim_delivery_versions, copilot_review_request_state_path, notification_state_path, set_state_dir, @@ -112,14 +113,22 @@ def deliver_with_state( repo: str, state_branch_name: str, state_dir: Path, + github_output: Path | None = None, ) -> int: repo_key = repo_state_key(repo) author_retry = runner_temp_path("prior-author-nudge-state.json") copilot_retry = runner_temp_path("prior-copilot-review-request-state.json") notification_retry = runner_temp_path("prior-notification-state.json") errors: list[str] = [] + active_versions = False def deliver() -> int: + nonlocal active_versions + active_versions = claim_delivery_versions() + if not active_versions: + errors.clear() + print("newer dashboard delivery versions are active; skipping", file=sys.stderr) + return 0 errors[:] = deliver_from_state( repo, author_retry, @@ -142,6 +151,9 @@ def deliver() -> int: ) if status != 0: return status + if github_output is not None: + with github_output.open("a", encoding="utf-8") as output: + output.write(f"active={'true' if active_versions else 'false'}\n") if not errors: return 0 print("Dashboard delivery failed:", file=sys.stderr) @@ -153,6 +165,7 @@ def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--repo", help="target repository name") parser.add_argument("--state-branch", required=True, help="git branch used for workflow state") + parser.add_argument("--github-output", type=Path, help="append the active versions result") args = parser.parse_args() repo = normalize_repo(args.repo) if args.repo else detect_repo() with state_branch.temporary_state_dir() as state_dir: @@ -161,6 +174,7 @@ def main() -> int: repo, args.state_branch, state_dir, + args.github_output, ) diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 62b1dd6ff96..03fb7687ba6 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -15,6 +15,7 @@ from dashboard_override import PRE_REVIEW_ROUTES, author_override_guidance from route_presentation import route_status_summary from state import ( + STATUS_COMMENT_REVISION, load_dashboard_state_cache, load_status_comment_rollout_state, save_status_comment_rollout_state, @@ -30,9 +31,6 @@ _AUTHOR_NUDGE_EPISODE_MARKER_RE = re.compile( r"" ) -# Increment whenever render_status_comment changes in a way existing comments -# need to adopt. Hourly runs durably roll the revision out to all open PRs. -STATUS_COMMENT_REVISION = 13 STATUS_COMMENT_ROLLOUT_BATCH_SIZE = 50 AUTHOR_ACTION_FEEDBACK_LINK_LIMIT = 20 NON_BLOCKING_CHECK_FAILURE_LIMIT = 20 diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 64db5c46017..a30b799679e 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -15,14 +15,28 @@ AUTHOR_NUDGE_STATE_FILE = "author-nudge-state.json" COPILOT_REVIEW_REQUEST_STATE_FILE = "copilot-review-request-state.json" STATUS_COMMENT_ROLLOUT_STATE_FILE = "status-comment-rollout-state.json" -# State files are disposable workflow caches, not durable user data. Bump only -# the version for the state shape whose meaning changed. +DELIVERY_VERSIONS_FILE = "delivery-versions.json" + +# These monotonic versions jointly order delivery compatibility. Increment the +# relevant version whenever its stored shape, meaning, or delivered behavior +# changes. A worker lower in any component skips delivery; after claiming the +# current vector, ordinary state loaders may regenerate mismatched disposable +# caches. Every constant ending in _STATE_VERSION or _REVISION is included. +# dashboard-state.json: accepted PR routing results and backfill readiness. DASHBOARD_STATE_VERSION = 5 +# backfill-state.json: round-robin cursor used by full dashboard refreshes. BACKFILL_STATE_VERSION = 3 +# notification-state.json: pending and delivered Slack notification records. NOTIFICATION_STATE_VERSION = 3 +# author-nudge-state.json: waiting episodes and delivered author reminders. AUTHOR_NUDGE_STATE_VERSION = 2 +# copilot-review-request-state.json: pending and delivered review requests. COPILOT_REVIEW_REQUEST_STATE_VERSION = 3 +# status-comment-rollout-state.json: target/completed renderer revisions and queue. STATUS_COMMENT_ROLLOUT_STATE_VERSION = 1 +# Rendered status-comment behavior. Increment when existing comments need to +# adopt a change; hourly runs durably roll it out to all open PRs. +STATUS_COMMENT_REVISION = 13 INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete" _state_dir: Path | None = None @@ -62,6 +76,10 @@ def status_comment_rollout_state_path() -> Path: return state_dir() / STATUS_COMMENT_ROLLOUT_STATE_FILE +def delivery_versions_path() -> Path: + return state_dir() / DELIVERY_VERSIONS_FILE + + def dashboard_markdown_path() -> Path: return state_dir() / DASHBOARD_MARKDOWN_FILE @@ -91,6 +109,14 @@ def empty_status_comment_rollout_state() -> dict[str, Any]: } +def current_delivery_versions() -> dict[str, int]: + return { + name: value + for name, value in globals().items() + if name.endswith(("_STATE_VERSION", "_REVISION")) + } + + def load_state_file( path: Path, current_version: int, @@ -183,6 +209,52 @@ def save_status_comment_rollout_state(state: dict[str, Any]) -> None: ) +def load_delivery_versions() -> dict[str, int] | None: + path = delivery_versions_path() + if not path.exists(): + return {} + try: + data = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as e: + print(f"warning: unreadable delivery versions {path}: {e!r}", file=sys.stderr) + return None + if not isinstance(data, dict): + return None + if any( + not isinstance(value, int) or isinstance(value, bool) or value < 0 + for value in data.values() + ): + return None + return data + + +def save_delivery_versions(versions: dict[str, int]) -> None: + path = delivery_versions_path() + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + json.dumps(versions, sort_keys=True, indent=2), + encoding="utf-8", + ) + + +def claim_delivery_versions() -> bool: + active_versions = load_delivery_versions() + if active_versions is None: + print("delivery versions are unreadable; skipping delivery", file=sys.stderr) + return False + current_versions = current_delivery_versions() + if active_versions.keys() - current_versions.keys(): + return False + if any( + active_versions.get(name, 0) > version + for name, version in current_versions.items() + ): + return False + if active_versions != current_versions: + save_delivery_versions(current_versions) + return True + + def enqueue_status_comment_update(pr_number: int) -> None: state = load_status_comment_rollout_state() pending = set(state["pending_pr_numbers"]) diff --git a/.github/scripts/pull-request-dashboard/test_delivery.py b/.github/scripts/pull-request-dashboard/test_delivery.py index c646b8d1ad9..672e9dc4b8a 100644 --- a/.github/scripts/pull-request-dashboard/test_delivery.py +++ b/.github/scripts/pull-request-dashboard/test_delivery.py @@ -1,6 +1,7 @@ from __future__ import annotations from pathlib import Path +import tempfile import unittest from unittest.mock import ANY, Mock, call, patch @@ -133,10 +134,12 @@ def test_open_pr_list_failure_skips_dependent_stages(self) -> None: @patch.object(delivery.sys, "stderr") @patch.object(delivery, "deliver_from_state", return_value=["status comments: boom"]) + @patch.object(delivery, "claim_delivery_versions", return_value=True) @patch.object(delivery.state_branch, "push_state_changes") def test_reports_delivery_errors_after_state_push( self, push_state_changes, + _claim_delivery_versions, _deliver_from_state, _stderr, ) -> None: @@ -145,17 +148,55 @@ def test_reports_delivery_errors_after_state_push( ) with ( + tempfile.TemporaryDirectory() as temp_dir, patch.object(delivery, "author_nudge_state_path", return_value=Path("author")), patch.object(delivery, "copilot_review_request_state_path", return_value=Path("copilot")), patch.object(delivery, "notification_state_path", return_value=Path("slack")), ): + github_output = Path(temp_dir) / "github-output" status = delivery.deliver_with_state( "open-telemetry/example", "dashboard-state", Path("state"), + github_output, ) + github_output_text = github_output.read_text(encoding="utf-8") self.assertEqual(1, status) + self.assertEqual("active=true\n", github_output_text) + + @patch.object(delivery, "deliver_from_state") + @patch.object(delivery, "claim_delivery_versions", return_value=False) + @patch.object(delivery.state_branch, "push_state_changes") + def test_stale_versions_skip_delivery_and_report_inactive( + self, + push_state_changes, + claim_delivery_versions, + deliver_from_state, + ) -> None: + push_state_changes.side_effect = ( + lambda _state_dir, _message, update_state, **_kwargs: update_state() + ) + + with ( + tempfile.TemporaryDirectory() as temp_dir, + patch.object(delivery, "author_nudge_state_path", return_value=Path("author")), + patch.object(delivery, "copilot_review_request_state_path", return_value=Path("copilot")), + patch.object(delivery, "notification_state_path", return_value=Path("slack")), + ): + github_output = Path(temp_dir) / "github-output" + status = delivery.deliver_with_state( + "open-telemetry/example", + "dashboard-state", + Path("state"), + github_output, + ) + github_output_text = github_output.read_text(encoding="utf-8") + + self.assertEqual(0, status) + claim_delivery_versions.assert_called_once_with() + deliver_from_state.assert_not_called() + self.assertEqual("active=false\n", github_output_text) if __name__ == "__main__": diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 9e067fbc9d2..209ba8e2f68 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -18,6 +18,8 @@ author_nudge_state_path, backfill_state_path, copilot_review_request_state_path, + claim_delivery_versions, + current_delivery_versions, dashboard_state_path, empty_state, enqueue_status_comment_update, @@ -26,6 +28,7 @@ load_backfill_state, load_copilot_review_requests, load_dashboard_state_cache, + load_delivery_versions, load_state_file, load_status_comment_rollout_state, load_notifications, @@ -357,6 +360,47 @@ def test_enqueue_status_comment_update_is_deduplicated(self) -> None: load_status_comment_rollout_state()["pending_pr_numbers"], ) + def test_each_delivery_version_rejects_older_workers(self) -> None: + baseline = current_delivery_versions() + for name in baseline: + with self.subTest(name=name), tempfile.TemporaryDirectory() as temp_dir: + with patch("state._state_dir", Path(temp_dir)): + self.assertTrue(claim_delivery_versions()) + newer = dict(baseline) + newer[name] += 1 + with patch("state.current_delivery_versions", return_value=newer): + self.assertTrue(claim_delivery_versions()) + self.assertFalse(claim_delivery_versions()) + self.assertEqual(newer, load_delivery_versions()) + + def test_new_delivery_version_rejects_older_workers(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): + self.assertTrue(claim_delivery_versions()) + with patch("state.FUTURE_STATE_VERSION", 1, create=True): + newer = current_delivery_versions() + self.assertEqual(1, newer["FUTURE_STATE_VERSION"]) + self.assertTrue(claim_delivery_versions()) + + self.assertFalse(claim_delivery_versions()) + self.assertEqual(newer, load_delivery_versions()) + + def test_delivery_versions_fail_closed(self) -> None: + malformed_versions = [ + "not json", + json.dumps([]), + json.dumps({"DASHBOARD_STATE_VERSION": None}), + json.dumps({"DASHBOARD_STATE_VERSION": False}), + json.dumps({"DASHBOARD_STATE_VERSION": -1}), + json.dumps({"DASHBOARD_STATE_VERSION": 1.5}), + ] + for contents in malformed_versions: + with self.subTest(contents=contents), tempfile.TemporaryDirectory() as temp_dir: + with patch("state._state_dir", Path(temp_dir)): + delivery_state = Path(temp_dir) / "delivery-versions.json" + delivery_state.write_text(contents, encoding="utf-8") + + self.assertFalse(claim_delivery_versions()) + def test_backfill_state_preserves_version_three_cursor(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): backfill_state_path().write_text( diff --git a/.github/workflows/pull-request-dashboard-repo.yml b/.github/workflows/pull-request-dashboard-repo.yml index 687b24841eb..23d42570bb9 100644 --- a/.github/workflows/pull-request-dashboard-repo.yml +++ b/.github/workflows/pull-request-dashboard-repo.yml @@ -219,11 +219,18 @@ jobs: run: | set -euo pipefail state_branch="${DASHBOARD_STATE_BRANCH_PREFIX}/${REPO_NAME}" - delivery_args=(--state-branch "$state_branch" --repo "$REPO_NAME") + delivery_args=( + --state-branch "$state_branch" + --repo "$REPO_NAME" + --github-output "$GITHUB_OUTPUT" + ) python3 .github/scripts/pull-request-dashboard/delivery.py "${delivery_args[@]}" - name: Publish dashboard issue - if: always() && steps.dashboard-token.outcome == 'success' + if: >- + always() && + steps.dashboard-token.outcome == 'success' && + steps.delivery.outputs.active == 'true' env: GH_TOKEN: ${{ steps.dashboard-token.outputs.token }} REPO_NAME: ${{ inputs.repository }}