The core problem is that we're using both the git history and PR status, and we can't get the two in a state that is totally consistent. In this case, our view of PRs was newer than our view of the repo.

We could arrange things so that we always have a view of the repo that's newever that our view of the PRs, but that would also lead to problems. Specifically, there can be just-landed-and-exported changes which we're trying to import, so that the created import commits has changes which are already on master. This will usually be fine as they'll disappear when rebased, but in rare cases the commit could be reverted in the interim, so that the importer puts the tests back into place.

There are many options here. Robert, WDYT?

No wait, I think the problem is that https://github.com/w3c/web-platform-tests/pull/6794 was totally incomplete, maybe because of  issue 743153  and/or issue 753487.

The scenario I was describing about the mirror being out of date still seems plausible though, even if it's not what happened today.

I'll do a manual export fixup of that change using the new instructions in https://bit.ly/ecosystem-infra-rotation now.

BTW, I guess we need to interrupt https://chromium-review.googlesource.com/c/618907 due to the same reason?

Yep, I abandoned it.

Hmm, so https://github.com/w3c/web-platform-tests/pull/6794 isn't merely incomplete, it's plain wrong. It includes changes that weren't in the final review at all. I'll have to revert it and then do a full manual export.

OK, https://github.com/w3c/web-platform-tests/pull/6920 is merged, so the immediate danger of revert should be over.

The inconsistency between PRs and local WPT checkout is a real problem and cannot be avoided because GitHub API queries and git checkout are two irrelevant operations that cannot be combined. And since the exporter runs independently from the importer, there is always a chance that one of them will change between the two operations. Mirroring the repo further increase the delay and thus the likelihood of inconsistency.

I don't want to over-design the system by cleverly and carefully choosing the order of the two operations. It is too fragile, and as foolip@ already mentioned, either order has its own problems.

PRs can have three states. Let's discuss them one by one:
* "Open": Importer does not really care about open PRs. Changes with open PRs are still considered exportable and not exported, and they will be re-applied to local WPT before the import.  ‎(✔)
* "Closed (merged)": We can safely and easily test whether a change has been exported by grepping the git history of local WPT checkout.  ‎(✔)
* "Closed (abandoned)": This is the hard one. I believe the current behavior is that rejecting a PR upstream will eventually lead the importer to remove the change from chromium as well. It is not instantly clear to me what are the consequences of different approaches.  (?)

FWIW, I've abandoned https://chromium-review.googlesource.com/c/chromium/src/+/630385 a couple of minutes ago because it was reverting https://github.com/w3c/web-platform-tests/commit/e45b910077ecb2508e39fe23f5d3ef97f036124e

It turns out the import job cloned w-p-t at b467fe0c0bc04377c304330c878f5c4097f8f879 (one commit before e45b910077ecb2508e39fe23f5d3ef97f036124e), which led it to effectively revert the changes from that commit (and its corresponding CL).

I guess everything would've been added back in the next import, but we'd have introduced some unnecessary churn and possibly complicate that directory's commit history.

Thanks Raphael! Looks like this is due to the "merged in wpt but probably not mirrored" bit in the description. It wasn't clear before it it had happened in practice, but since it has I'm increasing priority.

Reacting to comment #8, I think this is a case of "Closed (merged)", and I think what's missing is inspecting the local WPT history.

Raphael is right: we do have eventual consistency in the sense that a later import will restore the reverted change, but the messy history is undesirable and confuses people (especially the authors of the reverted changes).

Looks like this scenario is more likely than expected (yeah estimating the likelihood of timing issues is tricky). Starting to work on it now.

I just thought it over again, and I think for now we should change the test of "exported" Chromium commits from PR being closed to:
* PR is closed (abandoned)
* or, commit can be found in local WPT history

I think checking for abandoned PR will not introduce any inconsistency or race condition, so we can keep this behavior for now and combine it with checking the local WPT history for merged changes.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bb06360e8bae178410d7d82e50d802c0bf51b28

commit 7bb06360e8bae178410d7d82e50d802c0bf51b28
Author: Robert Ma <robertma@chromium.org>
Date: Wed Sep 06 23:49:13 2017

Make wpt-import verify that merged PRs are found in local WPT checkout

Background: the local WPT checkout might be stale when we check the status
of a PR. Therefore, a merged PR might not be found in local WPT, so the
corresponding Chromium commit should still be considered exportable and
reapplied to local WPT before import to avoid clobbering. (The exporter
does not have similar concerns.)

This CL:
1. Adds control in chromium_exportable_commits for whether to verify merged PRs.
2. Issues another GitHub API call to query whether a PR is merged, because the
   the information is not included in the search response.
3. Enables the verfication in wpt-import (wpt-export remains unchanged).
4. Implements MockLocalWPT and modifies MockGitHub to facilitate testing.

Bug:  756428 
Change-Id: I9c6c72c0c5a184e0a860b4498b0c877ed04690c9
Reviewed-on: https://chromium-review.googlesource.com/647835
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500133}
[modify] https://crrev.com/7bb06360e8bae178410d7d82e50d802c0bf51b28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_exportable_commits.py
[modify] https://crrev.com/7bb06360e8bae178410d7d82e50d802c0bf51b28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_exportable_commits_unittest.py
[modify] https://crrev.com/7bb06360e8bae178410d7d82e50d802c0bf51b28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py
[add] https://crrev.com/7bb06360e8bae178410d7d82e50d802c0bf51b28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_mock.py
[modify] https://crrev.com/7bb06360e8bae178410d7d82e50d802c0bf51b28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/7bb06360e8bae178410d7d82e50d802c0bf51b28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py
[modify] https://crrev.com/7bb06360e8bae178410d7d82e50d802c0bf51b28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py
[modify] https://crrev.com/7bb06360e8bae178410d7d82e50d802c0bf51b28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_mock.py
[modify] https://crrev.com/7bb06360e8bae178410d7d82e50d802c0bf51b28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_unittest.py