Data race when staging loaded model/IR for audio-thread handoff

[sdatkinson]

Summary

I found a data race that appears to still exist at commit 96337e9ab6e3beb619459779bbb5c47e1b04d8c4.

The staged DSP pointers mStagedModel and mStagedIR are plain std::unique_ptr members that can be accessed from different threads without synchronization. ProcessBlock() calls _ApplyDSPStaging(), which reads/moves those pointers on the audio thread, while model/IR loading code writes/resets the same pointers from the load/UI/state path.

Evidence

• ProcessBlock() calls _ApplyDSPStaging() during audio processing:

  NeuralAmpModelerPlugin/NeuralAmpModeler/NeuralAmpModeler.cpp

  Line 338 in 96337e9

  _ApplyDSPStaging();

• _ApplyDSPStaging() reads and moves mStagedModel / mStagedIR into the live DSP pointers:

  NeuralAmpModelerPlugin/NeuralAmpModeler/NeuralAmpModeler.cpp

  Lines 595 to 628 in 96337e9

  	void NeuralAmpModeler::_ApplyDSPStaging()
  	{
  	// Remove marked modules
  	if (mShouldRemoveModel)
  	{
  	mModel = nullptr;
  	mNAMPath.Set("");
  	mShouldRemoveModel = false;
  	mModelCleared = true;
  	_UpdateLatency();
  	_SetInputGain();
  	_SetOutputGain();
  	}
  	if (mShouldRemoveIR)
  	{
  	mIR = nullptr;
  	mIRPath.Set("");
  	mShouldRemoveIR = false;
  	}
  	// Move things from staged to live
  	if (mStagedModel != nullptr)
  	{
  	mModel = std::move(mStagedModel);
  	mStagedModel = nullptr;
  	mNewModelLoadedInDSP = true;
  	_UpdateLatency();
  	_SetInputGain();
  	_SetOutputGain();
  	}
  	if (mStagedIR != nullptr)
  	{
  	mIR = std::move(mStagedIR);
  	mStagedIR = nullptr;
  	}

• _StageModel() writes/resets mStagedModel:

  NeuralAmpModelerPlugin/NeuralAmpModeler/NeuralAmpModeler.cpp

  Lines 745 to 784 in 96337e9

  	std::string NeuralAmpModeler::_StageModel(const WDL_String& modelPath)
  	{
  	WDL_String previousNAMPath = mNAMPath;
  	try
  	{
  	auto dspPath = std::filesystem::u8path(modelPath.Get());
  	std::unique_ptr<nam::DSP> model = nam::get_dsp(dspPath);
  	// Check that the model has 1 input and 1 output channel
  	if (model->NumInputChannels() != 1)
  	{
  	throw std::runtime_error("Model must have 1 input channel, but has " + std::to_string(model->NumInputChannels()));
  	}
  	if (model->NumOutputChannels() != 1)
  	{
  	throw std::runtime_error("Model must have 1 output channel, but has "
  	+ std::to_string(model->NumOutputChannels()));
  	}
  	std::unique_ptr<ResamplingNAM> temp = std::make_unique<ResamplingNAM>(std::move(model), GetSampleRate());
  	temp->Reset(GetSampleRate(), GetBlockSize());
  	if (nam::SlimmableModel* slimmable = temp->GetSlimmableModel())
  	{
  	slimmable->SetSlimmableSize(GetParam(kSlim)->Value());
  	}
  	mStagedModel = std::move(temp);
  	mNAMPath = modelPath;
  	SendControlMsgFromDelegate(kCtrlTagModelFileBrowser, kMsgTagLoadedModel, mNAMPath.GetLength(), mNAMPath.Get());
  	}
  	catch (std::runtime_error& e)
  	{
  	SendControlMsgFromDelegate(kCtrlTagModelFileBrowser, kMsgTagLoadFailed);
  	if (mStagedModel != nullptr)
  	{
  	mStagedModel = nullptr;
  	}
  	mNAMPath = previousNAMPath;
  	std::cerr << "Failed to read DSP module" << std::endl;
  	std::cerr << e.what() << std::endl;

• _StageIR() writes/resets mStagedIR:

  NeuralAmpModelerPlugin/NeuralAmpModeler/NeuralAmpModeler.cpp

  Lines 790 to 822 in 96337e9

  	dsp::wav::LoadReturnCode NeuralAmpModeler::_StageIR(const WDL_String& irPath)
  	{
  	// FIXME it'd be better for the path to be "staged" as well. Just in case the
  	// path and the model got caught on opposite sides of the fence...
  	WDL_String previousIRPath = mIRPath;
  	const double sampleRate = GetSampleRate();
  	dsp::wav::LoadReturnCode wavState = dsp::wav::LoadReturnCode::ERROR_OTHER;
  	try
  	{
  	auto irPathU8 = std::filesystem::u8path(irPath.Get());
  	mStagedIR = std::make_unique<dsp::ImpulseResponse>(irPathU8.string().c_str(), sampleRate);
  	wavState = mStagedIR->GetWavState();
  	}
  	catch (std::runtime_error& e)
  	{
  	wavState = dsp::wav::LoadReturnCode::ERROR_OTHER;
  	std::cerr << "Caught unhandled exception while attempting to load IR:" << std::endl;
  	std::cerr << e.what() << std::endl;
  	}
  	if (wavState == dsp::wav::LoadReturnCode::SUCCESS)
  	{
  	mIRPath = irPath;
  	SendControlMsgFromDelegate(kCtrlTagIRFileBrowser, kMsgTagLoadedIR, mIRPath.GetLength(), mIRPath.Get());
  	}
  	else
  	{
  	if (mStagedIR != nullptr)
  	{
  	mStagedIR = nullptr;
  	}
  	mIRPath = previousIRPath;
  	SendControlMsgFromDelegate(kCtrlTagIRFileBrowser, kMsgTagLoadFailed);

• The members are declared as unguarded std::unique_ptr values:

  NeuralAmpModelerPlugin/NeuralAmpModeler/NeuralAmpModeler.h

  Lines 297 to 302 in 96337e9

  	std::unique_ptr<ResamplingNAM> mModel;
  	// And the IR
  	std::unique_ptr<dsp::ImpulseResponse> mIR;
  	// Manages switching what DSP is being used.
  	std::unique_ptr<ResamplingNAM> mStagedModel;
  	std::unique_ptr<dsp::ImpulseResponse> mStagedIR;

Why this matters

This is undefined behavior. If the audio thread observes one of those unique_ptrs while another thread is assigning, moving, or clearing it, the host can crash with a generic access violation, corrupt the staged object, or lose the handoff.

Tentative suggested fix

Use a small synchronization boundary around the staged-pointer handoff:

1. Build/load the NAM model or IR into a local unique_ptr first.
2. Lock only for assignment to mStagedModel / mStagedIR.
3. In ProcessBlock() / _ApplyDSPStaging(), use a non-blocking try_lock; if the lock is held, skip the handoff and retry on the next block.
4. Guard other staged-pointer readers/writers, including _ResetModelAndIR() and _ApplySlimParamToLoadedNAMs().

Tentative / I'm not confident on this fix; I'd like to think harder about RT safety (even though this isn't going to really be happening in a "real time" context--you're not usually browsing models when you're playing :)