Fix UpdateActivatedProbes panic, refactor state checks, use stopping state - #290
Conversation
Replace all 'if m.state < initialized' and similar inequality-based state checks with explicit switch/case statements. This makes the set of accepted states visible at each call site and avoids relying on the numeric ordering of the state enum. Also rename the 'stopped' state constant to 'stopping' to better reflect its semantics (a transition, not a terminal state). Co-authored-by: Cursor <cursoragent@cursor.com>
Treat 'stopping' the same as 'reset' in all manager methods except Stop(), which must still accept 'stopping' so that the full cleanup (detach probes, close maps) can proceed after StopReaders(). stopReaders() now sets m.state = stopping before closing readers. stop() sets m.state = reset at the end, after all resources are freed, instead of at the beginning. Co-authored-by: Cursor <cursoragent@cursor.com>
a1297c9 to
78cd5cc
Compare
theop-dd
changed the title
| m.stateLock.Lock() | ||
| defer m.stateLock.Unlock() | ||
| if m.state <= stopped { | ||
| if m.state <= stopping { |
There was a problem hiding this comment.
This still relies upon enum ordering
There was a problem hiding this comment.
This affect the perf map states and lock. I wanted to edit that on another PR and not on this one.
| m.stateLock.Lock() | ||
| defer m.stateLock.Unlock() | ||
| if m.state <= stopped { | ||
| if m.state <= stopping { |
There was a problem hiding this comment.
This still relies upon enum ordering
There was a problem hiding this comment.
This affects the perf map states and lock. I wanted to edit that on another PR and not on this one.
| rb.stateLock.Lock() | ||
| defer rb.stateLock.Unlock() | ||
| if rb.state <= stopped { | ||
| if rb.state <= stopping { |
There was a problem hiding this comment.
This still relies upon enum ordering
There was a problem hiding this comment.
This affect the ring buffer states and lock. I wanted to edit that on another PR and not on this one.
| rb.stateLock.Lock() | ||
| defer rb.stateLock.Unlock() | ||
| if rb.state <= stopped { | ||
| if rb.state <= stopping { |
There was a problem hiding this comment.
This still relies upon enum ordering
There was a problem hiding this comment.
This affect the ring buffer states and lock. I wanted to edit that on another PR and not on this one.
| switch m.state { | ||
| case reset, elfLoaded: | ||
| return ErrManagerNotInitialized | ||
| case initialized, stopping, paused, running: |
There was a problem hiding this comment.
If it's stopping, it shouldn't stop again
There was a problem hiding this comment.
stopping is only a window, not a terminal state.
stopReaders sets it before dropping the lock (so Start/NewPerfRing/NewRingBuffer cannot add readers while we Wait), then restores the previous state before returning — unless a concurrent Stop() already moved the manager to reset. That way StopReaders() does not leave the manager stuck in stopping.
Stop() still accepts stopping for the concurrent case: a Stop() that arrives during that unlock window. After StopReaders() returns, state is restored, so the usual StopReaders() then Stop() sequence sees initialized/paused/running again. closeReader is idempotent if the readers were already shut down.
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
3 participants
What does this PR do?
This PR fixes the
UpdateActivatedProbespanic and hardens manager state handling during reader shutdown.1. Fix panic in
UpdateActivatedProbesUpdateActivatedProbescalledm.stop()(private, no lock) on validation failure instead ofm.Stop()(public, acquires the lock). SinceUpdateActivatedProbesreleasesstateLockbefore the end of the function, it could callstopwithout holding the lock, which is not expected. This is fixed by callingm.Stop(), which properly acquires its own lock afterUpdateActivatedProbesreleases it.2. Refactor state checks from
iftoswitch/caseReplace all inequality-based state checks (
if m.state < initialized) with explicitswitch/casestatements. This makes the set of accepted states visible at each call site and eliminates reliance on the numeric ordering of thestateenum.Also rename the
stoppedconstant tostoppingto reflect its actual semantics: a transition state, not a terminal state.3. Use the transient
stoppingstate during reader shutdownstopReaders()setsm.state = stoppingbefore closing readers, then restores the previous state when it finishes (unless a concurrentStop()has already moved the manager toreset).stop()setsm.state = resetonly after all resources have been released and rechecks the state afterstopReaders()because that function temporarily releases the lock.stopping, only operations that touch PerfMap/RingBuffer readers are blocked:Start,Pause,Resume,NewPerfRing,NewRingBuffer— these interact with readers that are dead or shutting down.GetMap,GetProbe,GetProgram, etc.), probe operations (AddHook,DetachHook,CloneProgram,UpdateActivatedProbes), map/route operations (NewMap,UpdateMapRoutes,UpdateTailCallRoutes),CleanupNetworkNamespace, andStop— these resources are not affected bystopReaders.This prevents races where
NewPerfRing/NewRingBuffercould create new readers during thestopReadersunlock window, while preserving the manager's state after a standaloneStopReaders()call and still allowing probe and map operations during graceful shutdown.4. Retract affected releases
Retract
v0.8.5andv0.8.6ingo.modbecause these versions can panic.Motivation
Follow-up to #288 and #289. The deadlock fix in #288 introduced an unlock window in
stopReaderswherem.statewas set toresettoo early, which broke concurrent accessors. The transientstoppingstate communicates that readers are shutting down while the other resources are still alive.