Issue isn't closed (when it should be)

[mpickering]

https://gitlab.staging.haskell.org/ghc/ghc/issues/13127

State is open

https://ghc.haskell.org/trac/ghc/ticket/13127

State should closed

[bgamari]

This is concerning; it's not entirely clear why this change wasn't propagated.

[tdammers]

I think this might have to do with the "resolution" field. Will take a deeper look.

[tdammers]

Turns out it has absolutely nothing to do with that; the actual problem is that handling an issue mutation is a multi-step process, and exceptions thrown at earlier stages prevent subsequent stages from running. In this case, a lookup in the "issue link" phase fails (though in theory it shouldn't, because the linked issue, 13128, does exist, but that is separate concern, I believe), and the subsequent stages of the mutation (gitlab metadata including issue status, comment text, etc.) don't run.

The fix will be to catch exceptions for each individual stage, and keep going after one of them failed.

[tdammers]

Applying the fix reveals more problems.

For some reason, the issue edit that is supposed to close the issue gets back a 403 Forbidden response from gitlab. I fear that this may mean that the gitlab user we impersonate here isn't allowed to perform that particular mutation; and I'm not sure how we'd tackle this in an automated fashion.

[mpickering]

It seems more likely to me that the request ends up being invalid for whatever reason rather than a user permission issue? Perhaps the problem with the lookup is that because the two tickets are created so close together, the change hasn't yet propagated when the ticket lookup call takes place?

[tdammers]

Perhaps the problem with the lookup is that because the two tickets are created so close together, the change hasn't yet propagated when the ticket lookup call takes place?

That would explain the problem of failing to link the tickets (which also happens), but since this is a separate step, and the error is handled separately, it does not explain why the issue edit request is rejected - for that one, the other ticket isn't touched at all. Also, a failed issue lookup in gitlab is generally signalled as a 404 Not Found, but here we get a 403 Forbidden, which suggests a permission problem.

[tdammers]

Some digging reveals that the user in question (trac-adamgundry) appears to not be allowed to close that issue: impersonating the user on my gitlab instance, and then navigating to the imported issue, gives me a UI that doesn't have an option to close the issue. So it seems that my hunch was correct.

The gitlab documentation is a bit unclear about which role one would need in order to close issues, but I assume that one would need at the very least "Reporter", and that one would have to be a member of that project (which our auto-created users are not).

So maybe the solution is simply to add "GHC" to the newly-created users' projects, and to grant them the "Reporter" role.

[tdammers]

It seems that we already attempt to add newly created users to the project with Reporter permission, but for some reason, this doesn't always work; many users on my instance are not members of the GHC project.

[tdammers]

Eureka!

The solution to this one has several layers.

First, we were doing error handling wrong. <|> for MaybeT m will apply the Alternative choice operation for failures in the MaybeT part (that is, fail msg :: MaybeT m a), but failures in the inner Monad m (in this case ClientM) will bubble, e.g. lift (fail msg :: m a) :: MaybeT m a. So our code that was meant to try several things in sequence until it found a matching user, would bail out completely on the first exception encountered in any of the attempts, including API errors such as "member already exists" or permission errors. And as a result, it would simply skip the entire ticket mutation.

Another caveat is that ClientM itself can produce two distinct kinds of errors: API errors (like FailureResponse), and IO errors; they need to be caught separately, because the ClientM errors use ExceptT under the hood, while the IO errors are thrown as actual exceptions. Either will break <|> however, so we do need to catch and log both.

The user caching introduced another problem, namely that once we have successfully created a user but failed to add them to the project, we would not try to add them to the project again. I don't understand entirely why this would ever happen, but I believe it did, and caused those permission errors. So for the sake of getting it to work at all, I removed the caching bit, and that seems to work.

I've also rewritten the user oracle logic to no longer drag the awful create boolean along, instead just implementing findOrCreateUser in terms of findUser, and that ends up being much nicer. There's no caching currently, but if it turns out that we need it, I should be able to add it back in without too much trouble.

[mpickering]

This probably also fixes the issue that all of dfeuer's comments were missing from the issue tracker?

[tdammers]

It might, I haven't done a full run yet.

[mpickering]

If the failure is as you suggest.. then it will be a different user which fails to be added each time probably?

[tdammers]

Not necessarily. There are two different API failures interacting here:

• A 403 response when performing certain operations on a ticket while impersonating a user who is not a member of the project
• A 409 response when trying to add a user to the project who is already a member.

I'm not 100% positive on the precise order of circumstances, but I believe handling either of these incorrectly can leave the import in an inconsistent state, and trigger further errors. I do believe that it is possible to:

1. Attempt to create the user
2. Fail
3. Abort the current mutation
4. Continue with the next mutation
5. Attempt to create the user again
6. Etc.