Skip to content

Commit ef00ae5

Browse files
authored
fix: fix data race in agentscripts.Runner (#17630)
Fixes coder/internal#604 Fixes a data race in `agentscripts.Runner` where a concurrent `Execute()` call races with `Init()`. We hit this race during shut down, which is not synchronized against starting up. In this PR I've chosen to add synchronization to the `Runner` rather than try to synchronize the calls in the agent. When we close down the agent, it's OK to just throw an error if we were never initialized with a startup script---we don't want to wait for it since that requires an active connection to the control plane.
1 parent 35d686c commit ef00ae5

File tree

1 file changed

+19
-4
lines changed

1 file changed

+19
-4
lines changed

agent/agentscripts/agentscripts.go

+19-4
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"os/user"
1111
"path/filepath"
1212
"sync"
13-
"sync/atomic"
1413
"time"
1514

1615
"github.com/google/uuid"
@@ -104,7 +103,6 @@ type Runner struct {
104103
closed chan struct{}
105104
closeMutex sync.Mutex
106105
cron *cron.Cron
107-
initialized atomic.Bool
108106
scripts []runnerScript
109107
dataDir string
110108
scriptCompleted ScriptCompletedFunc
@@ -113,6 +111,9 @@ type Runner struct {
113111
// execute startup scripts, and scripts on a cron schedule. Both will increment
114112
// this counter.
115113
scriptsExecuted *prometheus.CounterVec
114+
115+
initMutex sync.Mutex
116+
initialized bool
116117
}
117118

118119
// DataDir returns the directory where scripts data is stored.
@@ -154,10 +155,12 @@ func WithPostStartScripts(scripts ...codersdk.WorkspaceAgentScript) InitOption {
154155
// It also schedules any scripts that have a schedule.
155156
// This function must be called before Execute.
156157
func (r *Runner) Init(scripts []codersdk.WorkspaceAgentScript, scriptCompleted ScriptCompletedFunc, opts ...InitOption) error {
157-
if r.initialized.Load() {
158+
r.initMutex.Lock()
159+
defer r.initMutex.Unlock()
160+
if r.initialized {
158161
return xerrors.New("init: already initialized")
159162
}
160-
r.initialized.Store(true)
163+
r.initialized = true
161164
r.scripts = toRunnerScript(scripts...)
162165
r.scriptCompleted = scriptCompleted
163166
for _, opt := range opts {
@@ -227,6 +230,18 @@ const (
227230

228231
// Execute runs a set of scripts according to a filter.
229232
func (r *Runner) Execute(ctx context.Context, option ExecuteOption) error {
233+
initErr := func() error {
234+
r.initMutex.Lock()
235+
defer r.initMutex.Unlock()
236+
if !r.initialized {
237+
return xerrors.New("execute: not initialized")
238+
}
239+
return nil
240+
}()
241+
if initErr != nil {
242+
return initErr
243+
}
244+
230245
var eg errgroup.Group
231246
for _, script := range r.scripts {
232247
runScript := (option == ExecuteStartScripts && script.RunOnStart) ||

0 commit comments

Comments
 (0)