From 230d70fd25b1f0395fd7540044bbdb0af9b8bedc Mon Sep 17 00:00:00 2001 From: pietrodimarco-dfinity <124565147+pietrodimarco-dfinity@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:05:31 +0000 Subject: [PATCH] fix(release-controller): refresh stale changelog on release notes branch ensure_published() wrote the changelog with Repository.create_file(), which is the GitHub *create* endpoint and returns 422 when the path already exists. The failure was swallowed by a bare `except`, and create_pull() then ran anyway -- so a branch left over from an earlier reconciler pass kept its original changelog while a pull request was opened advertising it. Observed in production today. rc--2026-07-23_04-21 security hotfix notes were committed to their branches at 16:11 and 16:14, before #2105 pinned changelog_base. After that merged, the Google Docs were regenerated against the new bases but the branches could not be updated: II:publish_notes.0c121276... - Creating file on branch replica-release-notes-0c121276... WW:publish_notes.0c121276... - Failed to create file on branch replica-release-notes-0c121276... II:publish_notes.0c121276... - Creating pull request for branch replica-release-notes-0c121276... The resulting PRs cited release-2026-07-23_04-21-base while the docs cited release-2026-07-17_04-19-base. Recovery required closing both PRs and deleting both branches by hand; the reconciler could not heal itself. Update the file in place when it already exists, skip the write entirely when the committed content already matches (the reconciler runs every 30s), and do not open a pull request when the write failed -- a PR on top of a failed write advertises content that was never committed. Adds the first tests for ensure_published(), which had none. All three new tests fail without this change. Co-Authored-By: Claude Opus 5 --- release-controller/publish_notes.py | 93 +++++++++++++++++-- .../tests/test_publish_notes.py | 85 +++++++++++++++++ 2 files changed, 170 insertions(+), 8 deletions(-) diff --git a/release-controller/publish_notes.py b/release-controller/publish_notes.py index ed610e3f2..e9d309c7c 100644 --- a/release-controller/publish_notes.py +++ b/release-controller/publish_notes.py @@ -6,6 +6,8 @@ from dotenv import load_dotenv from github import Auth from github import Github +from github import UnknownObjectException +from github.ContentFile import ContentFile from github.Repository import Repository from itertools import groupby from google_docs import ReleaseNotesClient @@ -99,6 +101,71 @@ def __init__(self, repo: Repository): """Initialize the client with the given repository.""" self.repo = repo + def _write_changelog( + self, version_path: str, changelog: str, branch_name: str, msg: str + ) -> None: + """ + Create the changelog on the release notes branch, or update it in place + if it is already there but stale. + + ``Repository.create_file`` is the GitHub *create* endpoint and fails with + 422 when the path already exists, so a branch left over from an earlier + reconciler pass keeps its original changelog forever. That is not + hypothetical: notes generated before a ``changelog_base`` override landed + in ``release-index.yaml`` stayed on the branch while the Google Doc was + regenerated against the new base, so the resulting pull request and the + doc disagreed about which commits the release contained. + + Writing is skipped entirely when the committed content already matches, + so a reconciler that runs every 30 seconds does not push an identical + commit on every pass. + """ + logger = LOGGER.getChild(branch_name) + try: + existing: ContentFile | list[ContentFile] | None = self.repo.get_contents( + version_path, ref=branch_name + ) + except UnknownObjectException: + existing = None + + if existing is None: + logger.info("Creating %s on branch %s", version_path, branch_name) + self.repo.create_file( + path=version_path, + message=msg, + content=changelog, + branch=branch_name, + ) + return + + if isinstance(existing, list): + raise RuntimeError( + f"Expected {version_path} on branch {branch_name} to be a file," + " but the GitHub API returned a directory listing." + ) + + if existing.decoded_content.decode("utf-8") == changelog: + logger.debug( + "%s on branch %s already matches the changelog; nothing to commit.", + version_path, + branch_name, + ) + return + + logger.info( + "Updating stale %s on branch %s (the committed changelog differs from" + " the one just prepared).", + version_path, + branch_name, + ) + self.repo.update_file( + path=version_path, + message=msg, + content=changelog, + sha=existing.sha, + branch=branch_name, + ) + def ensure_published(self, version: str, changelog: str, os_kind: OsKind) -> None: """Publish the release notes for the given version.""" logger = LOGGER.getChild(version) @@ -129,15 +196,25 @@ def ensure_published(self, version: str, changelog: str, os_kind: OsKind) -> Non msg = f"chore(release): Elect version {version} as {os_kind} candidate for rollout" try: - logger.info("Creating file on branch %s", branch_name) - self.repo.create_file( - path=version_path, - message=msg, - content=changelog, - branch=branch_name, + self._write_changelog( + version_path=version_path, + changelog=changelog, + branch_name=branch_name, + msg=msg, + ) + except Exception: + # Deliberately do NOT fall through to create_pull() here. A pull + # request opened on top of a failed write advertises a changelog that + # was never committed -- which is exactly how a branch carrying + # pre-changelog_base notes ended up in a PR that disagreed with its + # Google Doc. Bail out and let the next reconciler pass retry. + logger.exception( + "Failed to write %s on branch %s; not opening a pull request" + " because it would advertise content that was never committed.", + version_path, + branch_name, ) - except: # pylint: disable=bare-except # noqa: E722 - logger.warning("Failed to create file on branch %s", branch_name) + return logger.info( "Creating pull request for branch %s — please approve the PR at your leisure", diff --git a/release-controller/tests/test_publish_notes.py b/release-controller/tests/test_publish_notes.py index c7f48a919..bd9101e55 100644 --- a/release-controller/tests/test_publish_notes.py +++ b/release-controller/tests/test_publish_notes.py @@ -7,6 +7,8 @@ import pathlib from google_docs import google_doc_to_markdown from const import GUESTOS +from github import GithubException +from github import UnknownObjectException # @pytest.mark.skip( @@ -413,3 +415,86 @@ def test_publish_if_ready__ready_no_changes(mocker): ) assert publish_client.ensure_published.call_count == 0 # pylint: disable=no-member + + +def _repo_for_ensure_published(mocker, existing_content: str | None): + """ + Minimal Repository double for ensure_published(). + + ``existing_content`` is the changelog already committed on the release notes + branch, or None when the branch does not carry the file yet. + """ + repo = mocker.MagicMock() + # replica-releases/ on main does not yet contain this version. + repo.get_contents.side_effect = None + repo.get_pulls.return_value.totalCount = 0 + + listing = mocker.MagicMock() + listing.path = "replica-releases/some-other-version.md" + + def get_contents(path, ref=None): + if ref is None: + return [listing] + if existing_content is None: + raise UnknownObjectException(404, None, None) + f = mocker.MagicMock() + f.decoded_content = existing_content.encode("utf-8") + f.sha = "blobsha" + return f + + repo.get_contents.side_effect = get_contents + # The branch already exists whenever the file does. + branch = mocker.MagicMock() + branch.name = "replica-release-notes-" + "a" * 40 + repo.get_branches.return_value = [branch] if existing_content is not None else [] + return repo + + +def test_ensure_published_creates_file_when_branch_is_fresh(mocker) -> None: + repo = _repo_for_ensure_published(mocker, existing_content=None) + PublishNotesClient(repo).ensure_published("a" * 40, "NEW CHANGELOG", GUESTOS) + + repo.create_file.assert_called_once() + assert repo.create_file.call_args.kwargs["content"] == "NEW CHANGELOG" + repo.update_file.assert_not_called() + repo.create_pull.assert_called_once() + + +def test_ensure_published_updates_stale_changelog_on_existing_branch(mocker) -> None: + """ + Regression test: a branch left over from an earlier pass used to keep its + original changelog forever, because create_file() 422s on an existing path + and the failure was swallowed -- then a PR was opened anyway, advertising a + changelog that disagreed with the regenerated Google Doc. + """ + repo = _repo_for_ensure_published(mocker, existing_content="STALE CHANGELOG") + PublishNotesClient(repo).ensure_published("a" * 40, "NEW CHANGELOG", GUESTOS) + + repo.create_file.assert_not_called() + repo.update_file.assert_called_once() + assert repo.update_file.call_args.kwargs["content"] == "NEW CHANGELOG" + assert repo.update_file.call_args.kwargs["sha"] == "blobsha" + repo.create_pull.assert_called_once() + + +def test_ensure_published_does_not_recommit_identical_changelog(mocker) -> None: + """The reconciler runs every 30s; identical content must not be recommitted.""" + repo = _repo_for_ensure_published(mocker, existing_content="SAME CHANGELOG") + PublishNotesClient(repo).ensure_published("a" * 40, "SAME CHANGELOG", GUESTOS) + + repo.create_file.assert_not_called() + repo.update_file.assert_not_called() + repo.create_pull.assert_called_once() + + +def test_ensure_published_opens_no_pull_request_when_the_write_fails(mocker) -> None: + """ + A PR opened on top of a failed write advertises content that was never + committed. Bail out instead and let the next pass retry. + """ + repo = _repo_for_ensure_published(mocker, existing_content="STALE CHANGELOG") + repo.update_file.side_effect = GithubException(422, None, None) + + PublishNotesClient(repo).ensure_published("a" * 40, "NEW CHANGELOG", GUESTOS) + + repo.create_pull.assert_not_called()