From 77e7c39171f9151a751a54116d3e856750a65c67 Mon Sep 17 00:00:00 2001
From: daniel-c-harvey <the.real.daniel.harvey@gmail.com>
Date: Mon, 27 Jul 2026 00:02:32 -0400
Subject: [PATCH] fix(review): embed asymmetry comment, single-lock
 assign-marker, dedup-collapse test

---
 src/vst/reasampler_embed.cpp     |  4 ++++
 src/vst/reasampler_processor.cpp | 28 +++++++++++++++-------------
 tests/test_bank_sync.cpp         | 24 ++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/src/vst/reasampler_embed.cpp b/src/vst/reasampler_embed.cpp
index 51e62f7..95c69ce 100644
--- a/src/vst/reasampler_embed.cpp
+++ b/src/vst/reasampler_embed.cpp
@@ -113,6 +113,10 @@ void ReaSamplerEmbed::maybeRefresh() {
     } else if (lastSeenBankGeneration_ < 0) {
         currentGen = 0;  // unprimed + no stamp (pre-S9): treat as generation 0 for the first read
     }
+    // Intentional asymmetry: a TRANSIENT bridge failure (readReasamplerExtState returned
+    // nullopt after we were already primed) leaves currentGen == lastSeenBankGeneration_,
+    // so the bank-blob read is skipped and the editor keeps its last-known sample list.
+    // A stale-but-intact list is better than clearing samples_ on every transient hiccup.
 
     if (lastSeenBankGeneration_ < 0 || currentGen != lastSeenBankGeneration_) {
         auto banks =
diff --git a/src/vst/reasampler_processor.cpp b/src/vst/reasampler_processor.cpp
index f9903f8..4495c10 100644
--- a/src/vst/reasampler_processor.cpp
+++ b/src/vst/reasampler_processor.cpp
@@ -430,22 +430,24 @@ ReaSamplerProcessor::pollBankSync(bool isFocusedTarget) {
         }
     }
 
+    // Read lastConsumed and conditionally write it back under a single lock scope so there
+    // is no interleave window between the read and the write (a concurrent getState could
+    // otherwise observe a stale marker between the two separate lock acquisitions).
     std::int64_t lastConsumed = 0;
-    {
+    const AssignConsumeDecision decision = [&] {
         std::lock_guard<std::mutex> lock(assignMarkerMutex_);
         lastConsumed = lastConsumedAssignGeneration_;
-    }
-    const AssignConsumeDecision decision =
-        consumeDecision(request, lastConsumed, resolves, isFocusedTarget);
-
-    // Advance the persisted consumed marker whenever the decision consumed the request
-    // (applied OR dropped-as-seen). getState will persist it on the next project save so a
-    // re-open does not re-apply. A non-target instance leaves the marker (decision returns it
-    // unchanged) so it stays eligible if focus later lands here.
-    if (decision.consumedGeneration != lastConsumed) {
-        std::lock_guard<std::mutex> lock(assignMarkerMutex_);
-        lastConsumedAssignGeneration_ = decision.consumedGeneration;
-    }
+        const AssignConsumeDecision d =
+            consumeDecision(request, lastConsumed, resolves, isFocusedTarget);
+        // Advance the persisted consumed marker whenever the decision consumed the request
+        // (applied OR dropped-as-seen). getState will persist it on the next project save so
+        // a re-open does not re-apply. A non-target instance leaves the marker (decision
+        // returns it unchanged) so it stays eligible if focus later lands here.
+        if (d.consumedGeneration != lastConsumed) {
+            lastConsumedAssignGeneration_ = d.consumedGeneration;
+        }
+        return d;
+    }();
 
     if (decision.apply) {
         // Apply the assignment as this instance's own selection (the same path a user card-pick
diff --git a/tests/test_bank_sync.cpp b/tests/test_bank_sync.cpp
index f18af93..cdcad2a 100644
--- a/tests/test_bank_sync.cpp
+++ b/tests/test_bank_sync.cpp
@@ -160,6 +160,29 @@ static void testFreshInstanceAppliesFirst() {
     CHECK(d.consumedGeneration == 1);
 }
 
+// Re-import / dedup-collapse path: the extension ingests a sample that already exists in
+// the bank (dedup collapse: the bank_generation counter does NOT advance because no new
+// sample was added), but a new assign_request is still written with a HIGHER assign
+// generation (the ingest disambiguator, independent of bank_generation).
+//
+// The consumeDecision must apply the request — its own generation is the "is this new?"
+// discriminator, and it is strictly greater than lastConsumed. bank_generation does not
+// enter consumeDecision at all; this test proves the two counters are fully independent.
+static void testDedupCollapseAssignAppliesWhenBankGenerationUnchanged() {
+    // Simulate: bank_generation is 5 both before and after the dedup ingest (unchanged).
+    // The assign_request generation is 300 (new; lastConsumed was 200 from the prior assign).
+    // The existing sample resolves (it is in the bank — dedup kept it there).
+    const auto d = consumeDecision(makeReq("pool", "existing-sample-id", 300),
+                                   /*lastConsumed*/ 200, /*resolves*/ true,
+                                   /*isFocusedTarget*/ true);
+    CHECK(d.apply);
+    CHECK(d.bankId == "pool");
+    CHECK(d.sampleId == "existing-sample-id");
+    CHECK(d.consumedGeneration == 300);  // marker advanced to the new assign generation
+    // bank_generation (5) is not a parameter here — this test documents its absence from
+    // consumeDecision: only the assign_request's own generation drives the consume decision.
+}
+
 int main() {
     testParseAbsentAndMalformed();
     testParseValid();
@@ -173,6 +196,7 @@ int main() {
     testAppliedWhenNewTargetResolvable();
     testSameIdNewGenerationReapplies();
     testFreshInstanceAppliesFirst();
+    testDedupCollapseAssignAppliesWhenBankGenerationUnchanged();
 
     if (g_fail == 0) std::printf("All tests passed.\n");
     return g_fail ? 1 : 0;