Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not drop M-failed-staging-checks prematurely #46

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

eduard-bagdasaryan
Copy link
Contributor

@eduard-bagdasaryan eduard-bagdasaryan commented Mar 17, 2021

Before 1279629, the label prevented the unchanged PR progress, avoiding
useless staging commit recreation. Since 1279629 this is broken because
this label is dropped (along with other labels) in
_removeTemporaryLabels() and never set again.

We don't need to remove this label when:

  • The staged commit is fresh and its statuses are still failed.

  • The PR does not have a staged commit now (but had it with failed
    statuses some time ago, as M-failed-staging-checks indicates).
    The abandoned staged commit is still fresh and we do not
    need to vainly recreate it in this case (and possibly
    bringing about a livelocking situation with two such PRs).

We still need to remove this label when:

  • The staged commit is stale and is being abandoned.

  • The staged commit is fresh but its staging statuses succeeded
    (after previous failure(s)).

  • The PR does not have a staged commit now (but had it with failed
    statuses some time ago, as M-failed-staging-checks indicates).
    The abandoned staged commit became stale since then (e.g., some new
    branch commits were pushed or the statuses of the abandoned commit
    changed). We should proceed with this PR (removing the label).

Before 1279629, the label prevented the unchanged PR progress, avoiding
useless staging commit recreation. Since 1279629 this is broken because
this label is dropped (along with other labels) in
_removeTemporaryLabels() and never set again.

We don't need to remove this label when:

* The staged commit is fresh and its statuses are still failed.

* The PR does not have a staged commit now (but had it with failed
  statuses some time ago). The only indicator if this PR state is
  the M-failed-staging-checks label. Since we cannot locate this old
  staged commit, we cannot say whether there were new branch commits
  since that staged commit and we cannot proceed with this PR (after
  dropping the label) without bringing about a dangerous
  situation of livelocking between two such PRs.

We still need to remove this label when:

* The staged commit is stale and is abandoned.

* The staged commit is fresh but its staging statuses succeeded
  (after previous failure(s)).
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are on the right track here, but we need to tighten it up a little. If any of my suggested source code comments do not match reality, please let me know.

src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
Now this flag (renamed to _stagingFailed) is kept in sync with loaded
staging statuses. This adjustment removed the need to choose between
_stagingFailed and _stagedStatuses.failed() so that we can use the
former instead.
Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at b38e37f, 94aeaf0.

src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
If the PR does not have a staged commit now (but had it with failed
statuses some time ago, as M-failed-staging-checks indicates), then
two situations are possible:

* The PR has changed since then. For example, new branch commits
  have been created or the statuses of the old staged commit have
  been altered. We can recreate the staged commit in this case.

* The PR has not been changed. We should not vainly recreate the staged
  commit in this case (and possibly bringing about a livelocking
  situation with two such PRs).

With the aid of the newly added getIssueEvents() API, it became possible
to separate these two scenarios and handle them accordingly.
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are making progress, but these complex changes and concerns will require at least one more iteration. I hope the updated field definition (despite being complex) will help simplify these changes.

src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (e380e54).

src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
without loading its value from the label.

The previous approach (with loading from the label) was based
on the two points:

1. An exception in the code before _stagingFailed recalculation may
cause the unexpected label flipping.

2. An already merged (but still open) PR should keep the label intact
(indicating the real state of the merged commit), regardless of the
current PR state (the statuses could change since that).

However, (1) is probably irrelevant, since in this case there would be
another label indicating an even worse problem; and there is probably no
disadvantage in keeping the label up-to-date in (2), because we could
always inspect the statuses of the merged commit manually.

The new approach should be simpler/better overall because there is no
danger of making wrong decisions based on a (possibly stale) label
state via _stagingFailed.
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a partial review. It probably makes sense to resume this work only after the other active Anubis PR is merged so that you do not have to merge this code many times.

src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Show resolved Hide resolved
This flag is problematic in some aspects:

* it duplicates _stagedStatuses.failed() when staged commit exists
* it has too broad scope to be used, e.g., in
 _checkStagingPreconditions() context (where only 'abandoned' commit
   is expected)
* it introduces some difficulties when distinguishing its
  'undefined' and 'false' values

To avoid these problems, I decided store abandoned staged checks
instead. On one hand, the narrow scope of this field allows to use it
in specific contexts. On the other hand, we can inspect both abandoned
and staged checks in a broader scope (e.g., when labels are applied).
Also fixed one more case where _abandonedStagedStatuses should
be assigned.
Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (up to 051820a).

src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
src/MergeContext.js Outdated Show resolved Hide resolved
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just another partial review, with mostly questions rather than "true" change requests.

stagingEvents = stagingEvents.sort((ev1, ev2) => Date.parse(ev1.created_at) - Date.parse(ev2.created_at));
const lastStagingEvent = stagingEvents[stagingEvents.length - 1];
const mergeCommit = await this._getMergeCommit();
const mergeCommitCreatedAt = Date.parse(mergeCommit.author.date); // for merge commits author.date and committer.date should be the same
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear/intuitive that author.date and committer.date should be the same for merge commits. We know for a fact that commits merged by Anubis have different author.date and committer.date. I understand that merge commits created by GitHub are different from merged commits created by Anubis, but it is not clear to me why we expect those two dates to be the same for merge commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know for a fact that commits merged by Anubis have different author.date and committer.date.

Right - Anubis takes 'author' from the merge commit (i.e., author.date - when the merge commit was created), and committer date is the date when Anubis creates the staged commit (i.e., the current time).

It is not clear/intuitive that author.date and committer.date should be the same

I cannot refer to official documentation - I just checked this fact in few merge commits. However, is there any reason why they should different? For example, these two dates may differ after some action, performed on an existing commit, but merge commits are not subject of such actions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any reason why they should different

Just my personal (and possibly wrong) interpretation of what "merge commit" is. To me, that commit brings PR changes (authored some time ago) into the target repository branch. Thus, I would expect the author date to reflect something from the PR past rather than current time. However, I would not be surprised if somebody who knows a lot more about the mechanics of these merge commits would explain why the dates must be the same. It does not seem natural/intuitive to me, but sometimes the best approach is not natural or intuitive, especially from an outsider point of view.

To avoid misunderstanding, the problem here is not what GitHub (currently) does.

  • One problem is how we describe what we think it (currently) does. We describe it as if it is some self-evident truth or obvious principle, but we seem to be unable to substantiate that.
  • Another problem is that given a choice between two dates, if we need to know when the merge commit was created at, the committer date seems to be a more natural choice (when we look at commits where the dates differ, it is the committer date that we would pick). This is especially true if we would prefer to use the updated/new date if the merge commit was modified for some reason (but still represents a true merge commit). I know we do not expect such modifications, but let's assume, for the sake of the argument, that they happened to a merge commit -- which date would we pick then?

Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the mechanics of these merge commits

I don't think that there is some special magic (or hidden mechanics) in this merge commits - except the fact that they are automatically generated by GitHub. For example, I see nothing special in the commit data (fetched via an API call) except the fact that it has two parent commits. If we create a new (ordinary) branch commit - we expect that author.date=committer.date, right? We create a new commit from scratch - as an atomic operation and both dates ought to be identical. Merge commit is created by GitHub, but the same reasoning should be applicable there.

Copy link
Contributor

@rousskov rousskov Aug 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I create an ordinary commit, the dates are naturally the same because no good/reliable information is available about the changes I authored. Git could look at file timestamps instead of using current time, I guess. That would be natural too, but probably not worth the hassle in the default/common use case.

When I create a merge commit, then my expectations would depend on the type of merge:

  • If I am creating a true merge commit (which is what GitHub appears to be doing based on your "two parents" observations), then, yes, the dates would naturally be the same because the commit object represents the decision to merge (the changes have their own metadata in one of the parent chains).
  • I did not know that GitHub merge commit is always a true merge commit. For example, I recalled banning merge commits in GitHub configuration some time ago and expected that to result in squashed merge commits (single parent). With squash merges, the commit author date should not be that of the commit creation date IMO (but GitHub may disagree, of course).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eduard: If we create a new (ordinary) branch commit - we expect that author.date=committer.date, right?

FWIW, when I cherry-pick a commit, the dates differ:

commit d68eb83 (bag51)
AuthorDate: Tue Nov 28 09:34:46 2023 -0500
CommitDate: Thu Nov 30 16:36:15 2023 -0500

    Bug 5274: Successful tunnels logged as TCP_TUNNEL/500
...    
    Cherry-picked SQUID-932-bug5274-no-retryOrBail-for-committed-srv-closure
    commit e12d6d6

To me, a merge commit is more similar to a cherry-picked commit than to an ordinary branch commit.

Comment on lines 988 to 990
// the staging event for merge commit X is always created after X so we do not
// need to deal with the lastStagingEventCreatedAt == mergeCommitCreatedAt case
assert(lastStagingEventCreatedAt != mergeCommitCreatedAt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub event times have a second resolution. Could not the second-resolution timestamps be the same even though the two events happened at "slightly" different times, leading to an assertion here?

P.S. Do we need to use !== here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the second-resolution timestamps be the same

That is possible, I think, because merge commits and staged commits are created independently by GitHub and Anubis.

return; // merge commit created after the last staging event

// Merge commit created before the last staging event.
// Thus, we can get statuses of that event commit SHA.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we say "we can use statuses..." instead of "we can get statuses..."? We can get statuses of any tested commit, right? We just do not want use the old/irrelevant statuses, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that we can get statuses of any commit - as far as we have its SHA. Probably we can drop this comment at all - especially if we start comparing tree SHAs, as I suggested above.

// need to deal with the lastStagingEventCreatedAt == mergeCommitCreatedAt case
assert(lastStagingEventCreatedAt != mergeCommitCreatedAt);
if (lastStagingEventCreatedAt < mergeCommitCreatedAt)
return; // merge commit created after the last staging event
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that a problem? In other words, we cannot we just use the lastStagingEvent even if there was a more recent merged commit? After all, we are not looking for a fresh/current staged commit here but an "abandoned" one... Why do we (implicitly) define "abandoned" this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We look for an abandoned staged commit (i.e., its statuses) only if there is no newer merge commit - that is how our abandonedStagedStatuses defined. A newer merge commit means that we do not need the abandonedStagedStatuses check at all in _checkStagingPreconditions() - there should be no livelocking scenario (which we try to avoid) in this case.

However, the current approach with dates comparison is flawed, IMO, because staged commit uses merge commit data, but the merge commit itself can be already stale by that time - GitHub is free to create a new one anytime the base or issue branch changes. In other words, theoretically there can be a situation when lastStagingEventCreatedAt > mergeCommitCreatedAt, but merge commit is fresher that the staged commit. I think we need another approach here - the one that we already use in _stagedCommitIsFresh(), comparing tree SHAs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, so the answer to my question is: We cannot just always use the lastStagingEvent because we treat non-nil _abandonedStagedStatuses as a sign that "PR+base code is yet unchanged". Given that sign, we avoid restaging a doomed-to-fail PR. In a sense, "abandoned" implies "still fresh" here.

I understand why we ended up here. "Abandoned" comes from the state of the "auto" branch that depends on other PRs. The abandoned staged commit in PR A was abandoned by Anubis when Anubis switched to stage PR B. From PR A point of view (which is how I read this PR-specific code), that commit represents the latest PR and target branch state. If we rebase auto to end at that "abandoned staged" commit, it will be perfectly usable as a staged commit for PR A. That abandoned commit was effectively "unstaged" and can be "restaged".

I do not like negative names, so I am not suggesting to replace "abandoned" with "unstaged". We should document (proably where we declare_abandonedStagedStatuses) that "abandoned" means "removed from the staging branch or unstanged".

I think we need another approach here - the one that we already use in _stagedCommitIsFresh(), comparing tree SHAs.

I am looking forward to moving away from dates.

AFAICT, you want to define abandoned staged commit as a commit that would have been a fresh staged commit if the auto branch were to be modified to point at it. I would go a step further:

    // prevent staging if it is likely to fail again
    async _checkStagingInVain()
    {
        // TODO: How to force Anubis to stage after fixing buggy _tests_ (assuming the
        // fixed tests cannot be rerun to update the last staged PR commit statuses)?

        const abandonedStagedCommit = await this._findAbandonedStagedCommit();

        if (!abandonedStagedCommit)
            return; // we have not staged this PR (in recent memory)

        if (!isRestagingCandidate(abandonedStagedCommit))
            return; // something has changed

        // note that any unfinished tests are acceptable
        const abandonedStagedCommitStatuses = await this._getStagingStatuses(abandonedStagedCommit's commit SHA);
        if (abandonedStagedCommitStatuses.failed())
            throw this._exLabeledFailure("restaging candidate failed its tests", Config.failedStagingChecksLabel());

        // Expecting another successful staging attempt.
        // Hopefully, the new staged commit will not be unstaged this time.
        // TODO: Support restaging instead of creating a new staged commit (when possible).
        return;
    }
  1. Remove the _abandonedStagedStatuses data member (if possible).
  2. This or similar _findAbandonedStagedCommit() function will find and return the last staged PR commit, without checking its freshness.
  3. The "PR+base code is yet unchanged" code will call _checkStagingInVain() or a similar function to avoid staging in vain.
  4. isRestagingCandidate() will check whether the given commit can, from git/PR state points of view, become the staged commit now (abandoning the currently staged commit). This function ignores status checks.

The above plan does not include any restaging code -- we do not have to add that feature in this PR. isRestagingCandidate() only checks whether restaging a previously staged commit is possible from git point of view (e.g., the abandoned staged commit is still a parent of the target branch) and would preserve all Anubis invariants (e.g., the PR title/description still match the commit message).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the _abandonedStagedStatuses data member

This field serves 2 purposes:

  • avoiding "vain" commit recreation (covered in this thread)
  • re-applying M-failed-staging-checks (after removing it at the beginning as "temporary")

So we cannot simply drop this field - at least we need to store some indication (flag) instead (and check it in stagingFailed()). Also we need to set this field early (i.e., before _checkStagingPreconditions() from where your _checkStagingInVain() should be called) because we need to re-apply M-failed-staging-checks regardless of any other failing checks in _checkStagingPreconditions() coming prior to our call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we cannot simply drop this field - at least we need to store some indication (flag) instead

Understood. I suspected something like that -- that is why I added "(if possible)". If a simple flag can be stored instead, I would try doing that to avoid the complexity of indicating what "abandonedStagedCommitStatuses" (and their failure) really mean in other contexts. Clearly, the very concept of abandonment is rather convoluted here! The function can still compute abandonedStagedCommitStatuses locally as needed, similar to my sketch, but only store a simpler well-defined flag (based on abandonedStagedCommitStatuses.failed() and other function-local decisions).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed your refactoring sketch at fe1de92.

logApiResult(getIssueEvents.name, params, result);
resolve(res.data);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above will need to be adjusted to use rateLimitedPromise(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'll fix this.

Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answered questions. I'll post the required changes soon.

logApiResult(getIssueEvents.name, params, result);
resolve(res.data);
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'll fix this.

stagingEvents = stagingEvents.sort((ev1, ev2) => Date.parse(ev1.created_at) - Date.parse(ev2.created_at));
const lastStagingEvent = stagingEvents[stagingEvents.length - 1];
const mergeCommit = await this._getMergeCommit();
const mergeCommitCreatedAt = Date.parse(mergeCommit.author.date); // for merge commits author.date and committer.date should be the same
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We know for a fact that commits merged by Anubis have different author.date and committer.date.

Right - Anubis takes 'author' from the merge commit (i.e., author.date - when the merge commit was created), and committer date is the date when Anubis creates the staged commit (i.e., the current time).

It is not clear/intuitive that author.date and committer.date should be the same

I cannot refer to official documentation - I just checked this fact in few merge commits. However, is there any reason why they should different? For example, these two dates may differ after some action, performed on an existing commit, but merge commits are not subject of such actions.

// need to deal with the lastStagingEventCreatedAt == mergeCommitCreatedAt case
assert(lastStagingEventCreatedAt != mergeCommitCreatedAt);
if (lastStagingEventCreatedAt < mergeCommitCreatedAt)
return; // merge commit created after the last staging event
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We look for an abandoned staged commit (i.e., its statuses) only if there is no newer merge commit - that is how our abandonedStagedStatuses defined. A newer merge commit means that we do not need the abandonedStagedStatuses check at all in _checkStagingPreconditions() - there should be no livelocking scenario (which we try to avoid) in this case.

However, the current approach with dates comparison is flawed, IMO, because staged commit uses merge commit data, but the merge commit itself can be already stale by that time - GitHub is free to create a new one anytime the base or issue branch changes. In other words, theoretically there can be a situation when lastStagingEventCreatedAt > mergeCommitCreatedAt, but merge commit is fresher that the staged commit. I think we need another approach here - the one that we already use in _stagedCommitIsFresh(), comparing tree SHAs.

return; // merge commit created after the last staging event

// Merge commit created before the last staging event.
// Thus, we can get statuses of that event commit SHA.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right that we can get statuses of any commit - as far as we have its SHA. Probably we can drop this comment at all - especially if we start comparing tree SHAs, as I suggested above.

Comment on lines 988 to 990
// the staging event for merge commit X is always created after X so we do not
// need to deal with the lastStagingEventCreatedAt == mergeCommitCreatedAt case
assert(lastStagingEventCreatedAt != mergeCommitCreatedAt);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the second-resolution timestamps be the same

That is possible, I think, because merge commits and staged commits are created independently by GitHub and Anubis.

return;

// just in case: events should be already in chronological order already
stagingEvents = stagingEvents.sort((ev1, ev2) => Date.parse(ev1.created_at) - Date.parse(ev2.created_at));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. If GitHub guarantees chronological order and created_at dates have a one second resolution, then we may be reordering same-second events by sorting them. Perhaps we should assert that the events returned by GitHub are sorted instead of reordering them?

Copy link
Contributor Author

@eduard-bagdasaryan eduard-bagdasaryan Aug 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub guarantees chronological order

That is how it works, but I could not find this guarantee in the documentation.

we may be reordering same-second events by sorting them

Do you think that sorting of equal items reorders them? I would not expect changes in case...
BTW, it is documented that the original order remains unchanged if elements are equal.

Copy link
Contributor

@rousskov rousskov Aug 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that sorting of equal items reorders them?

In many cases, it does. The question is whether this particular sort() is stable.

BTW, it is documented that the original order remains unchanged if elements are equal.

... starting with ES10. We are relying on ES6 support IIRC, but it is possible that the specific nodejs implementation we are using today is compliant with that specific ES10 requirement. Unfortunately, https://node.green/ does not detail/check this part AFAICT -- it only checks whether Array.sort() is available. An SO comment says "[stable sort] was implemented first in Node 11. sort() is not stable in Node 10 or below".

What nodejs version are we using?

Can we assert() somehow that it's sort() is stable or that it is compliant with ES10?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our nodejs version is 10.19. It seems that sort stability starts from 11 version as you noted. We can update to 11 (and assert this requirement), but I could not find how to check sort() stability independently (or its compliancy with ES10).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest asserting that the events are sorted instead of sorting them, with a comment explaining that we should not sort because sort() is unstable until ES10.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done (7bf03bc).

// need to deal with the lastStagingEventCreatedAt == mergeCommitCreatedAt case
assert(lastStagingEventCreatedAt != mergeCommitCreatedAt);
if (lastStagingEventCreatedAt < mergeCommitCreatedAt)
return; // merge commit created after the last staging event
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, so the answer to my question is: We cannot just always use the lastStagingEvent because we treat non-nil _abandonedStagedStatuses as a sign that "PR+base code is yet unchanged". Given that sign, we avoid restaging a doomed-to-fail PR. In a sense, "abandoned" implies "still fresh" here.

I understand why we ended up here. "Abandoned" comes from the state of the "auto" branch that depends on other PRs. The abandoned staged commit in PR A was abandoned by Anubis when Anubis switched to stage PR B. From PR A point of view (which is how I read this PR-specific code), that commit represents the latest PR and target branch state. If we rebase auto to end at that "abandoned staged" commit, it will be perfectly usable as a staged commit for PR A. That abandoned commit was effectively "unstaged" and can be "restaged".

I do not like negative names, so I am not suggesting to replace "abandoned" with "unstaged". We should document (proably where we declare_abandonedStagedStatuses) that "abandoned" means "removed from the staging branch or unstanged".

I think we need another approach here - the one that we already use in _stagedCommitIsFresh(), comparing tree SHAs.

I am looking forward to moving away from dates.

AFAICT, you want to define abandoned staged commit as a commit that would have been a fresh staged commit if the auto branch were to be modified to point at it. I would go a step further:

    // prevent staging if it is likely to fail again
    async _checkStagingInVain()
    {
        // TODO: How to force Anubis to stage after fixing buggy _tests_ (assuming the
        // fixed tests cannot be rerun to update the last staged PR commit statuses)?

        const abandonedStagedCommit = await this._findAbandonedStagedCommit();

        if (!abandonedStagedCommit)
            return; // we have not staged this PR (in recent memory)

        if (!isRestagingCandidate(abandonedStagedCommit))
            return; // something has changed

        // note that any unfinished tests are acceptable
        const abandonedStagedCommitStatuses = await this._getStagingStatuses(abandonedStagedCommit's commit SHA);
        if (abandonedStagedCommitStatuses.failed())
            throw this._exLabeledFailure("restaging candidate failed its tests", Config.failedStagingChecksLabel());

        // Expecting another successful staging attempt.
        // Hopefully, the new staged commit will not be unstaged this time.
        // TODO: Support restaging instead of creating a new staged commit (when possible).
        return;
    }
  1. Remove the _abandonedStagedStatuses data member (if possible).
  2. This or similar _findAbandonedStagedCommit() function will find and return the last staged PR commit, without checking its freshness.
  3. The "PR+base code is yet unchanged" code will call _checkStagingInVain() or a similar function to avoid staging in vain.
  4. isRestagingCandidate() will check whether the given commit can, from git/PR state points of view, become the staged commit now (abandoning the currently staged commit). This function ignores status checks.

The above plan does not include any restaging code -- we do not have to add that feature in this PR. isRestagingCandidate() only checks whether restaging a previously staged commit is possible from git point of view (e.g., the abandoned staged commit is still a parent of the target branch) and would preserve all Anubis invariants (e.g., the PR title/description still match the commit message).

Since staged and merge commit are created independently by the bot and
GitHub, it is possible that the bot creates staged commit based on the
(already) stale merge commit, and we cannot for sure determine which
of them is newer simply comparing their dates. However, it is much
simpler to just compare tree SHAs - match indicates that the abandoned
commit exists.
storing a flag instead of _abandonedStagedStatuses data member.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants