diff --git a/ooniapi/services/testlists/src/testlists/manager.py b/ooniapi/services/testlists/src/testlists/manager.py index 88201c4a1..349799bd4 100644 --- a/ooniapi/services/testlists/src/testlists/manager.py +++ b/ooniapi/services/testlists/src/testlists/manager.py @@ -400,12 +400,24 @@ def read_changes_log(self, account_id): def write_changes_log( self, account_id: str, cc: str, old_entry: dict, new_entry: dict ): + """Track a single logical add/update/delete for account_id. + + This log is the source of truth _rebase_user_branch_onto_master() + replays onto a fresh copy of master before pushing (see that + method's docstring), so it needs to describe the *net* effect of + this account's session, not just each individual update() call. + """ changeset = self.read_changes_log(account_id) cc_changeset = changeset.setdefault(cc, []) + # True if old_entry was itself something this same session added + # earlier (never part of master) rather than pre-existing master + # content. + old_was_session_local_add = False if old_entry: try: changeset[cc].remove(dict(old_entry, **{"action": "add"})) + old_was_session_local_add = True except ValueError: # Not part of the changeset, no problem pass @@ -420,7 +432,14 @@ def write_changes_log( changeset[cc].append(dict(new_entry, **{"action": "add"})) - elif old_entry: + elif old_entry and not old_was_session_local_add: + # Deleting pre-existing master content - track it so it gets + # removed again on replay. If old_entry WAS a session-local + # add instead (handled above), it never reached master in + # the first place, so deleting it nets out to nothing: no + # delete op should be logged, or replay would later try to + # remove a row that was never really there and fail with a + # spurious "the list has changed" error. changeset[cc].append(dict(old_entry, **{"action": "delete"})) with self._get_user_changes_path(account_id).open("w") as out_file: @@ -727,10 +746,164 @@ def _is_pr_resolved(self, account_id) -> bool: ) return j["state"] != "open" + @timer(name="citizenlab_rebase_onto_master") + def _rebase_user_branch_onto_master(self, account_id: str): + """Rebuild the user's branch on top of the current tip of master. + + Without this, a user's branch/worktree is only ever cut from + master *once* - in _get_user_repo(), the first time the account + starts editing - and is never touched again no matter how far + master moves in the meantime, even though _pull_origin_repo()/ + _init_repo() keep the shared repo's own copy of master itself + genuinely fresh on every request. citizenlab/test-lists#2257 is + a real example of the result: three commits made back in March + against a master from that time, pushed and PR'd in August + against a master that has since moved on by five months of + other contributors' merged changes. + + This deliberately does *not* do a textual `git rebase` of the + branch's raw commits. Every edit here is a single-row append to + a shared CSV (see update()), and the busiest of these files + (e.g. lists/global.csv, which the real repo's history shows + growing via a steady stream of individual "Added to + GLOBAL.csv" commits) receive many such appends from unrelated + contributors on an ongoing basis. Two independent end-of-file + insertions with nothing else in common are exactly what git's + 3-way merge treats as a conflict - verified directly against + real git, not a dulwich quirk, and the same reason CHANGELOG.md + merge conflicts are proverbial - so replaying old commits onto a + master with *any* newer, unrelated addition to the same file + would still conflict even though the two changes have nothing to + do with each other. That would just turn today's silent, + eventually-discovered GitHub conflict into an immediate failure + on nearly every submission. + + Instead, this replays the user's own tracked, structured changes + (read_changes_log(); see write_changes_log()) directly on top of + whatever lists/.csv actually contains on master right now - + the same net effect update() itself applies for a single change. + The result is a branch that differs from the current tip of + master by exactly this account's pending edits and nothing else, + which always applies/merges cleanly on GitHub. + + Raises CannotUpdateList or DuplicateURLError if replaying a + change is genuinely no longer possible against current master + content (someone else already removed the exact row this branch + wants to delete, or already added the exact URL this branch + wants to add) - a real conflict that needs the user to reload + and reapply their change, exactly like the existing race- + condition check update() itself does for a single edit. + """ + changes_log = self.read_changes_log(account_id) + # A cc key can be present with an empty op list - e.g. an add + # immediately followed by a delete of that same row within one + # session nets out to nothing (see write_changes_log()) but still + # leaves cc as a dict key. Only cc's with at least one real op + # need to be read or rewritten. + touched_ccs = [cc for cc, ops in changes_log.items() if ops] + if not touched_ccs: + # Nothing left to replay - either changes.pickle predates + # this being written, the branch has drifted in some other + # unexpected way, or every tracked entry canceled itself out. + # Leave the branch as-is rather than guessing; the existing + # uncommitted-changes check in _push_to_repo() already guards + # the one case we know can otherwise land silently broken + # content. + return + + branch_name = self._get_user_branchname(account_id) + user_repo_path = self._get_user_repo_path(account_id) + + with git.Repo(self.repo_dir) as shared_repo: + master_sha = _read_ref(shared_repo, "refs/heads/master") + + # Compute the final content for every touched country code + # entirely against the shared repo's own checkout of master + # (never touching the user's worktree) before committing to + # anything, so a conflict found partway through - say, the 2nd + # of 3 touched files - never leaves the worktree half-rebuilt. + rows_by_cc: Dict[str, List[Dict[str, str]]] = {} + + def get_rows(cc: str) -> List[Dict[str, str]]: + if cc not in rows_by_cc: + csv_path = self.repo_dir / "lists" / f"{cc}.csv" + with csv_path.open() as f: + rows_by_cc[cc] = list(csv.DictReader(f)) + return rows_by_cc[cc] + + for cc in touched_ccs: + rows = get_rows(cc) + for op in changes_log[cc]: + entry = {k: op[k] for k in CITIZENLAB_CSV_HEADER} + action = op["action"] + if action == "delete": + try: + rows.remove(entry) + except ValueError: + raise CannotUpdateList( + description="The URL list has changed since " + "this change was made: the entry to delete " + "is no longer present. Please reload and " + "reapply your changes." + ) + elif action == "add": + duplicate_pool = list(rows) + if cc != "global": + duplicate_pool += get_rows("global") + if any(r["url"] == entry["url"] for r in duplicate_pool): + raise DuplicateURLError( + description=f"{entry['url']} is duplicate", + err_args={"url": entry["url"]}, + ) + rows.append(entry) + else: + raise AssertionError( + f"unknown changes_log action {action!r}" + ) + + log.debug( + f"[git-debug] account={account_id} branch {branch_name} " + f"rebuilding onto master={master_sha} (touching {touched_ccs})" + ) + + with git.Repo(user_repo_path) as user_repo: + old_head = _read_ref(user_repo, f"refs/heads/{branch_name}") + + # Hard-reset the worktree's branch to master's current tip. + # This is a plain, linear move onto master - not a merge - + # so it cannot itself conflict; any orphaned old commits on + # the branch are simply left unreferenced. + git.reset(user_repo, "hard", master_sha) + + for cc in touched_ccs: + csv_path = user_repo_path / "lists" / f"{cc}.csv" + with csv_path.open("w") as f: + writer = csv.DictWriter( + f, + quoting=csv.QUOTE_MINIMAL, + lineterminator="\n", + fieldnames=CITIZENLAB_CSV_HEADER, + ) + writer.writeheader() + for row in rows_by_cc[cc]: + writer.writerow(row) + git.add(repo=user_repo, paths=[csv_path.as_posix()]) + + bot_identity = f"{self.github_user} <{self.github_user}@users.noreply.github.com>".encode() + new_head = git.commit( + user_repo, + message=b"Reapply pending test-lists.ooni.org changes onto latest master", + author=bot_identity, + committer=bot_identity, + ) + log.debug( + f"[git-debug] account={account_id} branch {branch_name} " + f"rebuilt onto master: old_head={old_head} new_head={new_head}" + ) + def _push_to_repo(self, account_id): with git.Repo(self.repo_dir) as repo: branch_name = self._get_user_branchname(account_id) - local_head = _read_ref(repo, f"refs/heads/{branch_name}") # NOTE: comparing refs/heads/ in the shared repo against # HEAD in the user's worktree is *not* a useful check on its @@ -750,8 +923,7 @@ def _push_to_repo(self, account_id): dirty = git.status(user_repo) log.debug( f"[git-debug] account={account_id} pushing {branch_name} " - f"to GitHub, local_head={local_head} " - f"worktree_head={worktree_head} status={dirty}" + f"to GitHub, worktree_head={worktree_head} status={dirty}" ) has_uncommitted = bool( any(dirty.staged.values()) or dirty.unstaged @@ -775,13 +947,23 @@ def _push_to_repo(self, account_id): description="The user's worktree has uncommitted " "changes that never made it into a commit" ) + + # The worktree is clean, so it's safe to rebuild the + # branch onto the freshly-pulled origin/master (see + # _rebase_user_branch_onto_master's docstring) before + # pushing. This is what actually prevents merge conflicts + # on the resulting GitHub PR, e.g. + # citizenlab/test-lists#2257, where the pushed branch was + # based on a master commit from many months earlier. + self._rebase_user_branch_onto_master(account_id) else: log.debug( f"[git-debug] account={account_id} pushing {branch_name} " - f"to GitHub, local_head={local_head} (no worktree checked " - "out - nothing to verify)" + "to GitHub (no worktree checked out - nothing to " + "verify or rebuild)" ) + local_head = _read_ref(repo, f"refs/heads/{branch_name}") refspec = f"refs/heads/{branch_name}:refs/heads/{branch_name}" git.push(repo, "rworigin", refspecs=[refspec], force=True) log.debug( @@ -802,10 +984,21 @@ def propose_changes(self, account_id: str) -> str: the failure; retrying is expected to work once the underlying issue clears, and no change is lost even if the push succeeded but opening the PR failed, since submit() can simply be called again. + + The one exception is a BaseOONIException raised out of + _push_to_repo() (currently: CannotUpdateList or DuplicateURLError + from _rebase_user_branch_onto_master finding a genuine conflict + against master's current content) - that's re-raised as-is rather + than wrapped in CannotProposeChanges, because its "just retry" + framing doesn't apply: retrying submit() again hits the exact + same conflict every time until the user reloads and reapplies + their change. """ log.debug("proposing changes") try: self._push_to_repo(account_id) + except BaseOONIException: + raise except Exception: log.exception(f"[git-debug] account={account_id} failed to push to repo") raise CannotProposeChanges() diff --git a/ooniapi/services/testlists/tests/integ/test_testlists.py b/ooniapi/services/testlists/tests/integ/test_testlists.py index f6b418447..358b8dc67 100644 --- a/ooniapi/services/testlists/tests/integ/test_testlists.py +++ b/ooniapi/services/testlists/tests/integ/test_testlists.py @@ -1398,34 +1398,34 @@ def get_list(headers, cc="us"): assert any(e["url"] == spoof_url for e in tl_a_final) -def test_second_users_branch_misses_first_users_merged_change( +def test_second_users_branch_picks_up_first_users_merged_change( client, use_local_git_remotes, local_test_lists_remotes, tmp_path, monkeypatch, ): - """Documents a known limitation, not a regression: URLListManager - never rebases a user's long-lived worktree branch onto the latest - origin master before pushing. + """Regression test for a real merge-conflict bug (e.g. + citizenlab/test-lists#2257): URLListManager used to never update a + user's long-lived worktree branch against the latest origin master + before pushing, so a branch cut well before master moved on would be + pushed (and PR'd) as-is, conflicting with everything master had + picked up in the meantime. Scenario: user A submits and their PR gets merged. User B has their own in-progress submission whose worktree/branch was cut *before* A's - merge, adds more changes to it, and only then submits. Because the - service never updates B's branch against the new master in between, - B's pushed branch is missing A's already-merged change even though - it's sitting right there on master - exactly the kind of drift that - turns into a real merge conflict (or a silent, wrong resolution) once - a human tries to merge B's PR too. Rebasing (or at least fast-forward - merging) each user's branch onto origin's current master before - pushing would avoid this class of problem; this test exists to make - the current behavior visible and catch it if it silently changes. + merge, adds more changes to it, and only then submits. + _rebase_user_branch_onto_master() now replays B's own tracked changes + onto a fresh copy of master right before pushing, so B's pushed + branch picks up A's already-merged change automatically instead of + missing it - avoiding the merge conflict entirely rather than leaving + it for a human to discover and untangle later on GitHub. It then goes on to merge B's PR too, and checks the normal case still works end-to-end: B's state goes back to CLEAN, their worktree/branch get pruned, and a fresh submission afterwards - now cut from a master that already has both A's and B's changes - works cleanly and with no - drift, in contrast to the stale-branch case above. + drift, same as the rebuilt-branch case above. """ account_a = "0" * 16 account_b = "1" * 16 @@ -1501,14 +1501,14 @@ def submit(headers): return r.json()["pr_id"] def merge_and_resolve(account_id, branch): - # Simulate a human accepting and merging the PR on GitHub. A's PR - # merges as a clean fast-forward (master hasn't moved since A's - # branch was cut), but B's can't: master has since moved (A's - # change landed) and B's branch never picked that up, so a plain - # force-push would silently discard A's already-merged change - # instead of merging. _simulate_maintainer_merge() handles both - # cases correctly via a real (if manually-applied) content merge, - # matching what an actual GitHub merge would produce either way. + # Simulate a human accepting and merging the PR on GitHub. Both + # A's and B's branches now merge cleanly - A's as a plain + # fast-forward, and B's because _rebase_user_branch_onto_master() + # already rebuilt it onto master before it was pushed, so its + # diff against master's current tip is exactly B's own pending + # changes and nothing else. _simulate_maintainer_merge() applies + # a real (if manually-applied) content merge either way, matching + # what an actual GitHub merge produces. _simulate_maintainer_merge( local_test_lists_remotes["origin"], local_test_lists_remotes["push"], @@ -1560,23 +1560,20 @@ def merge_and_resolve(account_id, branch): ) assert url_a in master_content - # This is the known issue: B's branch never picked up A's change, even - # though B added more to their submission and submitted well after - # A's PR merged. If a human merged B's PR as-is, the result depends on - # exactly where in the file each line landed - best case, git resolves - # it automatically; worst case, it's a conflict a maintainer has to - # untangle by hand. Rebasing B's branch onto master before this push - # would have avoided the question entirely. + # This is the fix: B's branch picks up A's already-merged change even + # though B never explicitly did anything to fetch it - submit() now + # rebuilds the branch from a fresh copy of master plus B's own + # tracked changes right before pushing. B's PR now merges as a clean + # fast-forward instead of conflicting. pushed_b = _read_pushed_csv(local_test_lists_remotes, branch_b, "us") + assert url_a in pushed_b, ( + "expected B's branch to pick up A's already-merged change via " + "_rebase_user_branch_onto_master(); if this fails, the " + "rebase-onto-master fix for citizenlab/test-lists#2257-style " + "merge conflicts has regressed" + ) assert url_b1 in pushed_b assert url_b2 in pushed_b - assert url_a not in pushed_b, ( - "expected B's branch to still be missing A's merged change " - "(documents the known stale-base/needs-rebase limitation); if " - "this now fails because url_a IS present, someone has added " - "rebase-onto-master behavior and this test should be updated " - "to assert the fixed behavior instead" - ) # --- Now B's PR *also* gets accepted and merged (a maintainer might # do this even with the drift above - CSVs with additions in @@ -1718,11 +1715,15 @@ def test_push_refuses_when_worktree_has_uncommitted_changes( ) r = client_with_user_role.post("/api/v1/url-submission/submit") - # _push_to_repo() raises CannotUpdateList() here, which propose_changes() - # now surfaces as CannotProposeChanges() (see manager.py) instead of - # swallowing it into a fake 200 with an empty pr_id. + # _push_to_repo() raises CannotUpdateList() here. propose_changes() + # surfaces any BaseOONIException raised by _push_to_repo() as-is + # (see manager.py) rather than swallowing it into a fake 200 with an + # empty pr_id, or masking it behind the generic CannotProposeChanges: + # retrying submit() alone can't fix a worktree with leftover + # uncommitted changes, so the specific, actionable error is what + # should reach the caller. assert r.status_code == 400, r.json() - assert b"err_cannot_propose_changes" in r.content + assert b"err_cannot_update_list" in r.content # State must not have advanced to PR_OPEN off the back of a push that # never actually happened.