fix(release-controller): refresh stale changelog on release notes branch

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",

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()