From ed204e0fd985bbb43da7e19e07cf541ad74284a8 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Thu, 5 Oct 2023 10:16:57 -0400 Subject: [PATCH] client: ensure task only runs with prestart hooks (#18662) 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 remains `true` after the first transition, so it's safe to check it again after the prestart hooks as it will never revert to `false`. --- .changelog/18662.txt | 3 +++ client/allocrunner/taskrunner/task_runner.go | 6 ++++++ 2 files changed, 9 insertions(+) create mode 100644 .changelog/18662.txt diff --git a/.changelog/18662.txt b/.changelog/18662.txt new file mode 100644 index 000000000..6621656b0 --- /dev/null +++ b/.changelog/18662.txt @@ -0,0 +1,3 @@ +```release-note:bug +client: prevent tasks from starting without the prestart hooks running +``` diff --git a/client/allocrunner/taskrunner/task_runner.go b/client/allocrunner/taskrunner/task_runner.go index 9cf953c03..174760188 100644 --- a/client/allocrunner/taskrunner/task_runner.go +++ b/client/allocrunner/taskrunner/task_runner.go @@ -599,6 +599,12 @@ MAIN: goto RESTART } + // Check for a terminal allocation once more before proceeding as the + // prestart hooks may have been skipped. + if tr.shouldShutdown() { + break MAIN + } + select { case <-tr.killCtx.Done(): break MAIN