retry: fix retry.WithMaxAttempt to deal with opt.Closer properly

[otan]

In beac4a53e0e2e2236eb5957f67abc1bf476ad1b6, we introduced
stopper.ShouldQuiesce() to the retry.Closer so that server shutdowns
also shut down in-process retries to the temp schema cleaner.

However, when stopper.ShouldQuiesce() is called, the error that gets
wrapped in errors.Wrap is nil (as ctx.Err() is nil), and as such we
return with no error set. This causes potentially bugs afterwards as
users of the functions expected errors when this happens and not to
continue silently.

This PR bridges that gap by always wrapping an error around cases where
WithMaxAttempt is aborted by a context attempt.

Resolves #47057.

Release note: None.

[blathers-crl]

Welcome back, creator. Thank you for testing me.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

• @cockroach-teamcity (author of server: TestBootstrapNewStore failed #47057)

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan.}

[cockroach-teamcity]

This change is 

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 20923a8c.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan.}

[knz]

Woah good catch!

@tbg this may be of interest.
@otan @andreimatei I think this might explain #47011 - what do you think?

[tbg]

Yikes, good catch @otan.

[tbg]

Does this affect release-20.1?

beac4a5 is about the temporary object cleaner which I think caused the delay, so I'm hopeful that fixes that, too.

[otan]

Yeah I will backport this along with the original fix.

…

On Mon, 6 Apr 2020, 3:38 am Tobias Grieger, ***@***.***> wrote: Does this affect release-20.1? beac4a5 <beac4a5> is about the temporary object cleaner which I think caused the delay, so I'm hopeful that fixes that, too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47063 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA32FQ7ZYPSVIQOCJOETPMLRLGWLZANCNFSM4MB7CWAQ> .

[otan]

Does this affect release-20.1?

The original fix does (in the form of slow shutdowns), so when I backport it I will backport this PR along with it.

[knz]

thanks!

[otan]

bors r=knz

[RaduBerinde]

@knz I encountered this pattern before - if err1 == nil { return err2 } else { return Wrap() }. I think it would be useful to have an errors helper for this, perhaps something that takes a variable list of errors (some of which can be nil) and returns a single error. Or perhaps make Wrap specially handle a nil input.

[knz]

You may be thinking about errors.CombineErrors(err1, err2)

[RaduBerinde]

Oh, TIL, thanks!

[craig]

Build succeeded

• GitHub CI (Cockroach)

[andreimatei]

Yeah, I'm a big fan of errors.CombineErrors(). Real fine function.