fix: guard PID against concurrent access from the monitor goroutine#6
Merged
Conversation
readPID() cached its result into Process.PID on every call. It is reachable
from the caller's Stop() and IsAlive() and, at the same time, from the
library's own monitor goroutine via reapChildren(), so a bare Run() + Stop()
tripped the race detector with no concurrent access from the caller at all.
Two changes remove the race:
- readPID() no longer writes to the Process. It returns the pidfile
contents and callers use the returned value, which turns the three
concurrent call sites into pure reads.
- The remaining PID writes (writePID from Run, release from Stop and from
the monitor's Wait-error path) go through a sync.RWMutex.
A mutex rather than an atomic: PID is a string, so an atomic would need
atomic.Value/atomic.Pointer indirection for no benefit, and the lock leaves
room to cover sibling state under the same guard later. The critical sections
are field assignments only, never held across a syscall or a file operation,
so no path holding the lock can re-enter it.
CurrentPID() is added so callers can read the PID without touching the
exported field, which the monitor goroutine still clears on exit. The field
is kept and deprecated rather than unexported, to avoid breaking existing
users in a patch release.
The regression is covered by a spec that runs a real process through
Run/Stop with concurrent readers, and CI now runs the suite with -race,
which is why this went unnoticed: every existing spec already trips the
detector, but CI never passed the flag.
Fixes #5
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Assisted-by: Claude Code:claude-opus-4-8 [Read] [Edit] [Bash]
mudler
added a commit
that referenced
this pull request
Jul 20, 2026
Run's "already started" guard was a check-then-act on p.proc with no lock around it. Concurrent Run calls on the same handle could all observe a nil p.proc, all start a process and all launch a monitor goroutine. Every monitor does `defer close(p.done)`, and done is created once in New and never recreated, so the second monitor to see its process exit panicked with "close of closed channel". The same window raced on p.proc itself: it was written unsynchronized while the previous monitor, its reapChildren child and release were reading it. This is the same class as the PID race fixed in #6, one field over. PID was guarded there; p.proc was left alone because the write happens before `go p.monitor()` and so happens-before every reader - but only under the single-Run assumption that nothing stated or enforced. Make that assumption explicit rather than inventing a restart lifecycle nobody asked for: - Run claims the handle's single run by checking and setting `started` under mu, so exactly one caller can ever start a process and launch a monitor. The double-close is now unreachable by construction. - A second Run returns the new sentinel ErrProcessAlreadyRun, whose message keeps the historical "command already started" prefix so callers matching on that text keep working. - A Run that fails before starting anything (unwritable state dir, missing binary) hands the claim back, so retrying a failed start still works. - p.proc is guarded by the existing mutex via currentProc/setProc. Readers take the pointer under RLock and use it outside, so no path holds mu across Wait or Release. Sequential reuse already returned an error by accident, because release() never nils p.proc - but with a misleading message, and only for handles that had reached StartProcess. Stop() followed by Run() looked like it might restart and never could. The contract is now documented on Run. No exported signature changes; ErrProcessAlreadyRun is additive. Adds specs for concurrent Run, reuse after exit, reuse after Stop, and retry after a failed start. The concurrent spec panics on the unfixed code and trips -race on p.proc. Assisted-by: Claude Code:claude-opus-4-8[1m] [Read] [Edit] [Bash] Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
readPID() cached its result into Process.PID on every call. It is reachable from the caller's Stop() and IsAlive() and, at the same time, from the library's own monitor goroutine via reapChildren(), so a bare Run() + Stop() tripped the race detector with no concurrent access from the caller at all.
Two changes remove the race:
A mutex rather than an atomic: PID is a string, so an atomic would need atomic.Value/atomic.Pointer indirection for no benefit, and the lock leaves room to cover sibling state under the same guard later. The critical sections are field assignments only, never held across a syscall or a file operation, so no path holding the lock can re-enter it.
CurrentPID() is added so callers can read the PID without touching the exported field, which the monitor goroutine still clears on exit. The field is kept and deprecated rather than unexported, to avoid breaking existing users in a patch release.
The regression is covered by a spec that runs a real process through Run/Stop with concurrent readers, and CI now runs the suite with -race, which is why this went unnoticed: every existing spec already trips the detector, but CI never passed the flag.
Fixes #5
Assisted-by: Claude Code:claude-opus-4-8 [Read] [Edit] [Bash]