Make the automation release's publish-ordering rule a compile-time guard, not an assert

The assert compiled out under Release's NDEBUG and ran in no test target.
AutomationChannel::release now requires a ReleaseProof that only
publishLiveParams() or noRepublishNeeded() can mint.
This commit is contained in:
2026-08-02 19:36:32 -04:00
parent 9b5393098b
commit 056c60c8e1
4 changed files with 49 additions and 30 deletions
+28 -4
View File
@@ -11,6 +11,7 @@
#include <atomic> #include <atomic>
#include <cstddef> #include <cstddef>
#include <cstdint> #include <cstdint>
#include <cstdlib>
#include "core/instrument/param/param_merge.h" #include "core/instrument/param/param_merge.h"
@@ -21,6 +22,29 @@ inline constexpr std::size_t kDeckParamSlots = instrument::param::kDeckParamSlot
class AutomationChannel { class AutomationChannel {
public: public:
// Evidence release() below requires: the model has actually caught up with the
// point being released. The private constructor means the only way to obtain one
// is through a factory here, and both are named for the case they cover —
// `fromPublish` for an observed model republish, `noRepublishNeeded` for the two
// cases drainAutomationToModel finds nothing to publish (the fold left the model
// unchanged, or no engine exists yet to read the live block). A caller that has
// not published cannot spell the argument release() needs, which is what turns
// reordering the release ahead of the publish into a compile error.
class ReleaseProof {
public:
static ReleaseProof fromPublish(std::uint32_t before, std::uint32_t after) {
// publish() is guaranteed to advance the generation; landing here means
// that guarantee broke, worth crashing on unconditionally rather than
// tolerating silently.
if (after == before) std::abort();
return ReleaseProof{};
}
static ReleaseProof noRepublishNeeded() { return ReleaseProof{}; }
private:
ReleaseProof() = default;
};
// --- AUDIO THREAD ------------------------------------------------------------------- // --- AUDIO THREAD -------------------------------------------------------------------
// A point landed for `slot`. `ridesTheBlock` is false for a control that reaches the audio // A point landed for `slot`. `ridesTheBlock` is false for a control that reaches the audio
// beside the live block (master gain, whose route is the processor's own atomic): such a // beside the live block (master gain, whose route is the processor's own atomic): such a
@@ -76,10 +100,10 @@ public:
return true; return true;
} }
// Releases `slot`'s hold. ONLY legal once the model carrying that point has been republished // Releases `slot`'s hold. The `ReleaseProof` argument is the enforcement: it can only be
// — calling it earlier would let the audio thread drop the hold ahead of the block that // constructed once the model carrying this point has been republished (or shown not to need
// carries its value, which is a one-block revert to the superseded value. // it), so a caller earlier in that ordering has no value to pass.
void release(std::size_t slot, std::uint32_t seq) { void release(std::size_t slot, std::uint32_t seq, ReleaseProof) {
folded_[slot].store(seq, std::memory_order_release); folded_[slot].store(seq, std::memory_order_release);
} }
+8 -18
View File
@@ -6,8 +6,6 @@
#include "shell/instrument/reasampler_processor.h" #include "shell/instrument/reasampler_processor.h"
#include <cassert>
#include "base/source/fstring.h" #include "base/source/fstring.h"
#include "pluginterfaces/base/ustring.h" #include "pluginterfaces/base/ustring.h"
#include "pluginterfaces/vst/ivstparameterchanges.h" // IParameterChanges / IParamValueQueue #include "pluginterfaces/vst/ivstparameterchanges.h" // IParameterChanges / IParamValueQueue
@@ -307,28 +305,20 @@ void ReaSamplerProcessor::drainAutomationToModel() {
// controller cache is still refreshed, so the host's display and the editor follow. // controller cache is still refreshed, so the host's display and the editor follow.
const bool wasSuppressed = paramNotifySuppressed_; const bool wasSuppressed = paramNotifySuppressed_;
paramNotifySuppressed_ = true; paramNotifySuppressed_ = true;
// Captured before the publish below, so the assert at the release loop can tell "the model's
// publish already landed" from "it merely happened to be in flight for some other reason".
const std::uint32_t generationBeforeFold = liveParams_.generation();
if (moved) { if (moved) {
setInstrumentParams(params); setInstrumentParams(params);
publishLiveParams();
} }
// The hold outranks the model only until the model carries the point (this directory's
// CLAUDE.md, THE AUTHORITY MODEL) — `releaseProof` is that ordering enforced structurally:
// `automation_.release` below cannot compile without one, and the only ways to obtain one are
// `publishLiveParams`'s return (the `moved` branch) or `noRepublishNeeded` (nothing to
// publish because the fold left the model already matching the point).
const AutomationChannel::ReleaseProof releaseProof =
moved ? publishLiveParams() : AutomationChannel::ReleaseProof::noRepublishNeeded();
syncParamsFromModel(); syncParamsFromModel();
paramNotifySuppressed_ = wasSuppressed; paramNotifySuppressed_ = wasSuppressed;
// LAST, and that is the whole authority rule: the hold outranks the model only until the
// model carries the point. Released any earlier and the audio thread could drop the hold
// ahead of the block that carries its value; never released at all — the defect this
// replaces — and one point would defeat every later restore, reset and knob move.
// Enforced, not just commented: two prior passes inverted this order and every test in the
// tree still passed, because nothing exercises `process()`. `publishLiveParams` is a no-op
// before the engine has a sample rate (`builtSampleRate_`), which is the one case this assert
// must not fire for.
assert((!moved || builtSampleRate_.load(std::memory_order_relaxed) <= 0 ||
liveParams_.generation() != generationBeforeFold) &&
"release ran ahead of the publish that gives it authority");
for (std::size_t i = 0; i < foldedCount; ++i) { for (std::size_t i = 0; i < foldedCount; ++i) {
automation_.release(foldedSlots[i], foldedSeqs[i]); automation_.release(foldedSlots[i], foldedSeqs[i], releaseProof);
} }
} }
+9 -4
View File
@@ -276,16 +276,21 @@ void ReaSamplerProcessor::clearMasterBusClip() {
meterClip_.store(false, std::memory_order_relaxed); meterClip_.store(false, std::memory_order_relaxed);
} }
void ReaSamplerProcessor::publishLiveParams() { AutomationChannel::ReleaseProof ReaSamplerProcessor::publishLiveParams() {
const int rate = builtSampleRate_.load(std::memory_order_relaxed); const int rate = builtSampleRate_.load(std::memory_order_relaxed);
if (rate <= 0) return; if (rate <= 0) return AutomationChannel::ReleaseProof::noRepublishNeeded();
const std::uint32_t before = liveParams_.generation();
const InstrumentParams params = instrumentParams(); const InstrumentParams params = instrumentParams();
const instrument::engine::LiveValues block = const instrument::engine::LiveValues block =
instrument::engine::foldLive(resolvePlay(params.play, rate), params.keyTrack); instrument::engine::foldLive(resolvePlay(params.play, rate), params.keyTrack);
// livePublishMutex_ enforces the seqlock's single-writer contract (live_params.h) against {
// reloadInstrument's publish — held for the publish call only, not the fold above. // livePublishMutex_ enforces the seqlock's single-writer contract (live_params.h)
// against reloadInstrument's publish — held for the publish call only, not the fold
// above.
std::lock_guard<std::mutex> lock(livePublishMutex_); std::lock_guard<std::mutex> lock(livePublishMutex_);
liveParams_.publish(block); liveParams_.publish(block);
}
return AutomationChannel::ReleaseProof::fromPublish(before, liveParams_.generation());
} }
SampleRefs ReaSamplerProcessor::sampleRefs() { SampleRefs ReaSamplerProcessor::sampleRefs() {
+2 -2
View File
@@ -203,8 +203,8 @@ public:
// voices already latched. THE tier-3 commit (the three tiers are listed in this // voices already latched. THE tier-3 commit (the three tiers are listed in this
// directory's CLAUDE.md). Callers pair this with setInstrumentParams exactly as they // directory's CLAUDE.md). Callers pair this with setInstrumentParams exactly as they
// paired it with reloadInstrument. No-op before anything has been decoded (the next reload // paired it with reloadInstrument. No-op before anything has been decoded (the next reload
// bakes and publishes). UI thread; serialized against reloadInstrument's own publish. // bakes and publishes). UI thread; serialized against reloadInstrument's own publish. Returns drainAutomationToModel's release proof (automation_channel.h); other callers ignore it.
void publishLiveParams(); AutomationChannel::ReleaseProof publishLiveParams();
// Per-instance channel mode (mono | stereo), guarded by channelModeMutex_, never read // Per-instance channel mode (mono | stereo), guarded by channelModeMutex_, never read
// on the audio thread. Decode policy only (downmix vs L/R split) — the output bus is // on the audio thread. Decode policy only (downmix vs L/R split) — the output bus is