client: avoid unconsumed channel in timer construction (#15215)

* client: avoid unconsumed channel in timer construction

This PR fixes a bug introduced in #11983 where a Timer initialized with 0
duration causes an immediate tick, even if Reset is called before reading the
channel. The fix is to avoid doing that, instead creating a Timer with a non-zero
initial wait time, and then immediately calling Stop.

* pr: remove redundant stop

.changelog/15215.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+client: Fixed a bug where tasks would restart without waiting for interval
+```

client/allocrunner/taskrunner/task_runner.go

@@ -563,7 +563,8 @@ func (tr *TaskRunner) Run() {
 	// Set the initial task state.
 	tr.stateUpdater.TaskStateUpdated()
 
-	timer, stop := helper.NewSafeTimer(0) // timer duration calculated JIT
+	// start with a stopped timer; actual restart delay computed later
+	timer, stop := helper.NewStoppedTimer()
 	defer stop()
 
 MAIN:

helper/funcs.go

@@ -3,6 +3,7 @@ package helper
 import (
 	"crypto/sha512"
 	"fmt"
+	"math"
 	"net/http"
 	"path/filepath"
 	"reflect"
@@ -369,6 +370,9 @@ type StopFunc func()
 //
 // Returns the time.Timer and also a StopFunc, forcing the caller to deal
 // with stopping the time.Timer to avoid leaking a goroutine.
+//
+// Note: If creating a Timer that should do nothing until Reset is called, use
+// NewStoppedTimer instead for safely creating the timer in a stopped state.
 func NewSafeTimer(duration time.Duration) (*time.Timer, StopFunc) {
 	if duration <= 0 {
 		// Avoid panic by using the smallest positive value. This is close enough
@@ -386,6 +390,14 @@ func NewSafeTimer(duration time.Duration) (*time.Timer, StopFunc) {
 	return t, cancel
 }
 
+// NewStoppedTimer creates a time.Timer in a stopped state. This is useful when
+// the actual wait time will computed and set later via Reset.
+func NewStoppedTimer() (*time.Timer, StopFunc) {
+	t, f := NewSafeTimer(math.MaxInt64)
+	t.Stop()
+	return t, f
+}
+
 // ConvertSlice takes the input slice and generates a new one using the
 // supplied conversion function to covert the element. This is useful when
 // converting a slice of strings to a slice of structs which wraps the string.

helper/funcs_test.go

@@ -376,7 +376,7 @@ func TestCheckNamespaceScope(t *testing.T) {
 	}
 }
 
-func Test_NewSafeTimer(t *testing.T) {
+func TestTimer_NewSafeTimer(t *testing.T) {
 	t.Run("zero", func(t *testing.T) {
 		timer, stop := NewSafeTimer(0)
 		defer stop()
@@ -390,6 +390,17 @@ func Test_NewSafeTimer(t *testing.T) {
 	})
 }
 
+func TestTimer_NewStoppedTimer(t *testing.T) {
+	timer, stop := NewStoppedTimer()
+	defer stop()
+
+	select {
+	case <-timer.C:
+		must.Unreachable(t)
+	default:
+	}
+}
+
 func Test_ConvertSlice(t *testing.T) {
 	t.Run("string wrapper", func(t *testing.T) {