Skip to content

User{Branch,Tag,Submodule}: ferry update-ref errors upwards

This fixes a bug which was spotted in production with the go_user_delete_branch feature[1] enabled. When going through proxy.handler() we got:

rpc error: code = Internal desc = failed proxying to secondary:
rpc error: code = Unknown desc = Could not update
refs/heads/include-stableid-test. Please refresh and try again.

That's almost correct, we should be returning a FailedPrecondition there instead of the "Unknown", and while we're at it we should return any unknown update-ref errors (there are currently none) as "Unknown", not a non-GRPC error.

As an aside maybe proxy.handler() should ferry the error upwards without wrapping it, but that's another issue.

As it turns out this issue impacts the go_user_delete_tag feature[2] too, so let's fix that. In [3] merged in [4] I'd changed updateRefError handling before for several API calls in what turned out to be an improvement, it just wasn't bug-compatible with the Ruby code. We'd simply hide these before [3]. This just fixes the error type we return.

I read the ruby code from GitalyServer::OperationsService to GitLab::Git::OperationService to Git::GitLab::Repository and I can't see any other issues now. The subtlety I missed before was how Gitlab::Git::CommitError is ferried up as a GRPC::FailedPrecondition in this case.

I'm also fixing the submodule code I modified in [3] here. Note the added "return nil, err" case there. That wouldn't happen in the Ruby code (I think all update-ref would be a FailedPrecondition), but I think it makes sense in the context of the Go code. If we have a truly unexpected unknown condition from update-ref let's return an Unknown code.

Finally, it sucks that we have no tests for any of this, but to do that we'd need to make the test somehow pass a parameter down to update-ref so it would pass the branch/tag/submodule logic with one expected reference, but then fake up a race condition by feeding a different argument to update-ref, or have update-ref die or whatever. That would be very useful to test these cases, but in lieu of that I'd like to fix the current bug.

  1. #3370 (closed)
  2. #3371 (closed)
  3. 816ad5b2 (User{Branch,Submodule}: remove erroneously copy/pasted error handling, 2020-11-24)
  4. !2841 (merged)

Merge request reports