fix(plugin): fix plugin family runner race condition

[zsoltkacsandi]

Description

If an error occurs, we send the result back to the family manager on a channel and return from the function:

if err := rr.WaitReady(ctx); err != nil {
  s.sendResults(retResults, fmt.Errorf("failed to wait for plugin scanner to be ready: %w", err))
  return
}

As part of the return, the deferred Stop and Remove lifecycle functions begin to run:

defer func() {
	if err := rr.Stop(ctx); err != nil {
		s.logger.WithError(err).Errorf("failed to stop runner")
	}

	// TODO: add short wait before removing to respect container shutdown procedure

	if err := rr.Remove(ctx); err != nil {
		s.logger.WithError(err).Errorf("failed to remove runner")
	}
}() //nolint:errcheck

Once the family manager gets the result in the result channel, it does not really care if the underlying goroutines are finished or not:

vmclarity/scanner/families/manager.go

Line 148 in 0750034

case r := <-result:

This results in a race condition, once the family manager gets all the results it expects, the program will terminate regardless of whether the plugins' Stop() and Destroy() functions have finished or not.

We need to call the Exit and Destroy lifecycle functions before sending back the result.

Type of Change

• Bug Fix
• New Feature
• Breaking Change
• Refactor
• Documentation
• Other (please describe)

Checklist

• I have read the contributing guidelines
• Existing issues have been referenced (where applicable)
• I have verified this change is not present in other open pull requests
• Functionality is documented
• All code style checks pass
• New code contribution is covered by automated tests
• All new and existing tests pass

[akijakya]

Nice catch! This behavior can be found in other families and should be fixed there as well...

Just a thought: what if the cleanup func is added to the Scanner struct, and the sendResults is refactored in a way that it calls s.cleanup first, it can even be renamed to finish() or something?

[zsoltkacsandi]

Just a thought: what if the cleanup func is added to the Scanner struct, and the sendResults

That can work too, the reason why I chose this approach, because it's more expressive.
If someone reads the code, you can read as a sentence, first we are cleaning up, then we are sending back the result, and then returning from the function. The intent is clear.

To make part of the sendResults feels like creating a side effect. The name of the function says that I am sending back a result. There would be a hidden behaviour that you wouldn't expect without taking a look into the sendResults function.

it can even be renamed to finish() or something?

Totally agree, we can use a more descriptive name here. finishRunner or something like that?

[akijakya]

Totally agree, we can use a more descriptive name here. finishRunner or something like that?

finishScannerRun?

[github-actions]

Hey!

Your images are ready:

• ghcr.io/openclarity/vmclarity-apiserver-dev:pr1824-88aa0e94dec793aea0f1096051641489d2a69358
• ghcr.io/openclarity/vmclarity-cli-dev:pr1824-88aa0e94dec793aea0f1096051641489d2a69358
• ghcr.io/openclarity/vmclarity-cr-discovery-server-dev:pr1824-88aa0e94dec793aea0f1096051641489d2a69358
• ghcr.io/openclarity/vmclarity-orchestrator-dev:pr1824-88aa0e94dec793aea0f1096051641489d2a69358
• ghcr.io/openclarity/vmclarity-plugin-kics-dev:pr1824-88aa0e94dec793aea0f1096051641489d2a69358
• ghcr.io/openclarity/vmclarity-ui-dev:pr1824-88aa0e94dec793aea0f1096051641489d2a69358
• ghcr.io/openclarity/vmclarity-ui-backend-dev:pr1824-88aa0e94dec793aea0f1096051641489d2a69358