Fix race condition around supervisor's Commander

[tpaschalis]

From what I'd see under normal circumstances (and also the example), starting up the supervisor which also launches the commander would happen in a separate goroutine than the one that inspects its state or calls Stop.

The current implementation has a race condition on the cmd *exec.Cmd field, using it both for figuring out whether the commander is running and also actually executing a new Agent process, which could potentially lead to a panic.

This PR fixes a small race condition around the supervisor's Commander by utilizing an existing running field and turning it into a atomic.Bool to store the commander's state.

Fixes #290

[tpaschalis]

Here I've actually flipped the condition from what it was before, as per the method's docstring.

[srikanthccv]

Could this potentially leave the agent process running in some cases? The NewSupervisor method starts a goroutine runAgentProcess to launch the agent process. There's a possibility that Shutdown might be called before c.cmd.Start() finishes. If c.cmd.Start() completes successfully, the process would keep running, which isn't expected.

[tpaschalis]

Hmm, let me think about this (and if the previous code prevented that from happening). We could potentially store the running value a little earlier to avoid that IIUC.

[tpaschalis]

Hey there, sorry for taking so long to get back to you. As far as I can tell from the code, this was already the case and it isn't a regression from this PR.

To avoid this, we'd make it the caller's responsibility to call only call Stop() after IsRunning() returns true (for example having a backoff mechanism for properly cleaning up resources), or add more explicit synchronization like we do in clientcommon

opamp-go/client/internal/clientcommon.go

Lines 53 to 55 in ed38d5f

	// True when stopping is in progress.
	isStoppingFlag bool
	isStoppingMutex sync.RWMutex

IMO this is a separate issue than the race condition the one's fixed here, so tackling the cleanup of resoueces might be a new discussion. WDYT?

[srikanthccv]

I do not think it was possible to leave the process running in the background because we are checking if c.cmd is non-nil (which is the source of the race condition) instead of running status. I think the Stop should be updated to handle this.

[srikanthccv]

Sorry for the delay.

Here is the case I think this still doesn't address

1. Start() is called
2. The process begins starting up but hasn't yet reached the point where c.running.Store(true) is executed
3. Shutdown() is called during this window
4. Since !c.IsRunning() evaluates to true at this point, Shutdown() returns immediately
5. This could result in the process continuing to run

Does that make sense?

[tpaschalis]

FYI I've pushed another commit in the Stop method to set the isStoppingFlag immediately.

[tpaschalis]

So, let me try to get this right.

The commander's Start is called only in two cases
a) In the Supervisor's startAgent.
b) During a restart (not pertinent to our discussion)

The startAgent method is only called via the runAgentProcess; in turn, this is only called as a new goroutine in the NewSupervisor function.

In the Shutdown method, the Commander's Stop sets the isStopping flag.

So let's say that NewSupervisor starts, and the new goroutine is launched.

Immediately, the commander's Shutdown method is called, to call the commander's Stop and set the isStoppingFlag. When the call stack gets to the commander's Start it will immediately exit with the IsStopping method without having a chance to launch any process.

[tpaschalis]

cc @srikanthccv I've put out a test in f135e3b to try and verify the behavior we want.

I couldn't find any more elegant way than the ugly DelayLogger, but I'd like to see what you think. (This test fails in main, without my fix here).

[srikanthccv]

So let's say that NewSupervisor starts, and the new goroutine is launched.

Immediately, the commander's Shutdown method is called, to call the commander's Stop and set the isStoppingFlag. When the call stack gets to the commander's Start it will immediately exit with the IsStopping method without having a chance to launch any process.

Take this case

NewSupervisor starts, and the goroutine runAgentProcess is launched, -> startAgent -> s.commander.Start -> the execution reaches c.cmd.Start

opamp-go/internal/examples/supervisor/supervisor/commander/commander.go

Line 61 in 7cdd395

if err := c.cmd.Start(); err != nil {

And say cmd.Start didn't return yet

Now, assume the commander's Shutdown method is called (c.cmd.Start still didn't return) so c.running is false. In this flow isStoppingFlag is not helping because we would still return as IsRunning evaluates to true.

I think if we add a waitgroup before the c.cmd.Start and wait in shutdown should solve the issue.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 77.06%. Comparing base (ed38d5f) to head (f135e3b).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/noplogger.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   76.32%   77.06%   +0.74%     
==========================================
  Files          25       25              
  Lines        1681     1696      +15     
==========================================
+ Hits         1283     1307      +24     
+ Misses        291      286       -5     
+ Partials      107      103       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tigrannajaryan]

@tpaschalis @srikanthccv is this still in progress?

[tpaschalis]

@tpaschalis @srikanthccv is this still in progress?

Srikanth had suggested using more explicit synchronization to avoid potentially leaving a dangling process; if the Shutdown method of the supervisor is called, then the isStoppingFlag will be set and the call to the commander's Start will abort before starting the new process.

I'm looking forward to feedback!

[tigrannajaryan]

@tpaschalis @srikanthccv I assume you are working on this PR. Please ping me when it is ready.