Fix service process state data race - #75
yaroslavvoloshchuk-codaio wants to merge 1 commit into
Conversation
246486c to
557bbc5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 557bbc529e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| instance.waitErrFn = sync.OnceValue(func() error { | ||
| res := cmd.Wait() | ||
| instance.SetDone() | ||
| instance.setProcessState(cmd.ProcessState) |
There was a problem hiding this comment.
Do not leave ProcessState nil while WaitDelay is pending
For services whose child has already been reaped by the background Wait but Cmd.Wait is still waiting for copied stdout/stderr to finish or for the 50ms WaitDelay (for example, an exited service with a descendant holding stdout open), killGroup in StopWithSignal can return ESRCH and StopAll immediately records serviceInstance.ProcessState(). Since the snapshot is not assigned until after cmd.Wait() returns here, StopAll can return a nil state, and cmd/svcinit/main.go dereferences it when printing state.UserTime()/SystemTime(). Previously the direct cmd.ProcessState read was populated as soon as Process.Wait reaped the child, before WaitDelay, so this shutdown path still had a state.
Useful? React with 👍 / 👎.
Summary
exec.Cmd.ProcessStateunderServiceInstance's existing mutex afterCmd.WaitcompletestestsworkspaceWhy
Cmd.WaitwritesCmd.ProcessStatewhileRunner.StopAlland health checks may read it concurrently. The Go race detector reports that unsynchronized access in #71.The snapshot preserves the existing
ProcessState()API and is reset when a service command is reinitialized.Regression test
The test uses target-local
-no-piebecause rules_go race mode requires external linking and the registered hermetic LLVM toolchain hits golang/go#76825: the linker probe drops required hermetic flags, misses-no-pie, and then rejects Go's position-dependent objects under PIE semantics. This flag affects only the Linux race-regression target.Validation
Cmd.Wait/ProcessState()race undergo test -raceGOWORK=/tmp/rules-itest-issue71-go.work go test -race -vet=off ./process_state_race -run '^TestProcessStateAccessIsSynchronized$' -count=1bazel build //runner/...557bbc529ee1ab166322bd7990a5c4f73cad2cd4Fixes #71.