Skip to content

feat: add CmdState.Wait/ExitCode to read a command's exit code race-free#149

Merged
joshiste merged 1 commit into
mainfrom
feat/cmdstate-exitcode
Jun 30, 2026
Merged

feat: add CmdState.Wait/ExitCode to read a command's exit code race-free#149
joshiste merged 1 commit into
mainfrom
feat/cmdstate-exitcode

Conversation

@joshiste

Copy link
Copy Markdown
Member

Why

Several extensions run a command non-blocking and reap it in a goroutine:

go func() { _ = cmd.Wait() }()   // cmd.Wait() writes cmd.ProcessState

then read the result from their Status/Stop handlers:

exitCode := cmdState.Cmd.ProcessState.ExitCode()   // concurrent read → data race

cmd.Wait() sets cmd.ProcessState, so this is a data race on that field (flagged by go test -race, and able to yield torn reads). extcmd.CmdState already owns the *exec.Cmd, the output buffer and its mutex, but exposed Cmd directly and synchronized only the output buffer — so every consumer hand-rolled the same racy pattern. The identical race currently exists in extension-gatling, extension-jmeter, extension-k6 and extension-postman.

What

Give CmdState ownership of the process result:

  • CmdState.Wait() error — reaps the process (cmd.Wait()) and records the exit code into an internal atomic.Int32. It is the sole reader of cmd.ProcessState. Returns the exec.Cmd.Wait error for logging.
  • CmdState.ExitCode() int — returns the recorded exit code, or -1 while the process is still running (or was signal-terminated), matching os.ProcessState.ExitCode(). Safe to call concurrently with the Wait goroutine.

Consumers replace go func(){ cmd.Wait() }() with go cmdState.Wait() and read cmdState.ExitCode() instead of cmdState.Cmd.ProcessState.

Cmd stays exported (callers still need Start(), Process, env, etc.); the Wait() doc steers callers to ExitCode() rather than reading ProcessState directly.

Follow-up

Once this is released, the four extensions above will be migrated to go cmdState.Wait() + cmdState.ExitCode() (extension-gatling#138 will be repointed to this API).

Verification

go build ./..., go vet ./extcmd/, and go test -race ./extcmd/ all pass. New tests cover non-zero exit, zero exit, and concurrent readers racing the Wait goroutine under -race.

Extensions reap their command in a goroutine (go cmd.Wait()) and then read
cmdState.Cmd.ProcessState.ExitCode() from their status/stop handlers — a
data race, since cmd.Wait() writes cmd.ProcessState concurrently. Give
CmdState ownership of the process result: Wait() reaps the process and is
the sole reader of ProcessState, recording the exit code into an atomic;
ExitCode() returns it (-1 while running). Callers replace their hand-rolled
goroutine with 'go cmdState.Wait()' and read cmdState.ExitCode().
@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Claude finished @joshiste's task in 2m 16s —— View job


Claude finished reviewing this PR

  • Check for previously unresolved review threads from Claude (none exist on this PR)
  • Gather context (diff, changed files)
  • Review code quality, bugs, security, performance, race conditions
  • Post findings

Review

This is a clean, well-scoped fix. I verified the implementation in extcmd/cmdstate.go against the stdlib source (os/exec_posix.go) and confirmed:

  • (*os.ProcessState).ExitCode() is nil-receiver-safe (returns -1), so cs.Cmd.ProcessState.ExitCode() in Wait() (cmdstate.go:32) can never panic even if cs.Cmd.Wait() returns early with ProcessState == nil (e.g. Wait called without Start).
  • exitCode atomic.Int32 is embedded by value in a struct that's always heap-allocated via new(CmdState)/NewCmdState and only ever used through a pointer, so there's no copy-after-first-use hazard with the atomic.
  • The new Wait()/ExitCode() pair correctly closes the original race: Wait is now the sole reader of Cmd.ProcessState, and ExitCode only touches the atomic, so concurrent status/stop-handler reads are safe under -race.
  • Initialization to -1 in NewCmdState (extcmd.go:29) correctly matches os.ProcessState.ExitCode()'s "not yet exited" sentinel before Wait has run.
  • No other code in this repo calls cmdState.Cmd.Wait() directly, so there's no remaining internal caller that could race the new Wait() method.

Tests in cmdstate_test.go reasonably cover non-zero exit, zero exit, and concurrent readers polling ExitCode() while Wait() runs in another goroutine — a good -race regression test for the exact bug being fixed.

CI (ci workflow) already passed on this PR, consistent with the PR description's go build/go vet/go test -race verification.

No bugs, security issues, or race conditions found. No inline comments needed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant