Backport of client: ensure task only runs with prestart hooks into release/1.5.x #18669
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport
This PR is auto-generated from #18662 to be assessed for backporting due to the inclusion of the label backport/1.5.x.
The below text is copied from the body of the original PR.
Since the allocation in the task runner is updated in a separate goroutine, a race condition may happen where the task is started but the prestart hooks are skipped because the allocation became terminal.
Checking for a terminal allocation before proceeding with the task start ensures the task only runs if the prestart hooks are also executed.
Since
shouldShutdown()
only uses terminal allocation status, it remainstrue
after the first transition, so it's safe to check it again after the prestart hooks as it will never revert tofalse
.Some other implementations ideas I considered:
shouldShutdown()
from withinprestart()
to before it is called. I think this would be more or less equivalent to this approach.shouldShutdown()
is guaranteed not to change whileprestart()
runs and the task starts. This is probably the most "correct" approach, but sinceshouldShutdown()
can only transition fromfalse
totrue
, checking it again afterprestart()
seems enough.Closes #18659
Overview of commits