Fixed race condition in skip_delete_sample() in AlleleCount and VariantStats classes

Now skipped samples are saved to the metadata as unique keys - one per sample - rather than writing all skipped samples to a single key.

Author: alancleary

apis/python/tests/test_delete.py

@@ -102,18 +102,17 @@ def test_delete_samples_skip_aggregate_stats(tmp_path, stats_v3_dataset):
     assert "second" not in sample_names
     assert "third" in sample_names
 
-    # Verify skipped_delete_samples metadata on allele_count and variant_stats
+    # Verify per-sample skipped deletion key is recorded on allele_count and variant_stats
     for array_name in ["allele_count", "variant_stats"]:
         with tiledb.open(os.path.join(uri, array_name), "r") as arr:
-            meta = arr.meta["skipped_delete_samples"]
-            assert b"second" in meta
+            assert "skipped_delete_sample:second" in arr.meta
 
 
 @skip_if_no_bcftools
 def test_delete_samples_skip_aggregate_stats_accumulates(
     tmp_path, stats_v3_dataset
 ):
-    """Verify skipped_delete_samples metadata accumulates across deletions."""
+    """Verify per-sample skipped deletion keys are recorded across multiple deletions."""
     uri = os.path.join(tmp_path, "stats_test")
 
     ds = tiledbvcf.Dataset(uri=uri, mode="w")
@@ -124,8 +123,8 @@ def test_delete_samples_skip_aggregate_stats_accumulates(
 
     for array_name in ["allele_count", "variant_stats"]:
         with tiledb.open(os.path.join(uri, array_name), "r") as arr:
-            meta = arr.meta["skipped_delete_samples"]
-            assert meta == b"second,fifth"
+            assert "skipped_delete_sample:second" in arr.meta
+            assert "skipped_delete_sample:fifth" in arr.meta
 
 
 def test_delete_samples_read_mode_raises(tmp_path):

libtiledbvcf/src/stats/allele_count.cc

@@ -220,30 +220,37 @@ void AlleleCount::skip_delete_sample(
     return;
   }
 
-  // Read existing skipped_delete_samples metadata
-  std::string existing;
-  {
-    Array read_array(*ctx, uri, TILEDB_READ);
-    const void* value = nullptr;
-    tiledb_datatype_t type = TILEDB_ANY;
-    uint32_t num = 0;
-    read_array.get_metadata("skipped_delete_samples", &type, &num, &value);
-    if (value != nullptr && num > 0) {
-      existing = std::string(static_cast<const char*>(value), num);
-    }
-  }
+  // Write one key per sample to avoid a read-modify-write race condition.
+  // Concurrent callers write to different keys so no coordination is needed.
+  std::string key = "skipped_delete_sample:" + sample;
+  Array write_array(*ctx, uri, TILEDB_WRITE);
+  write_array.put_metadata(key, TILEDB_STRING_ASCII, 0, nullptr);
 
-  // Build updated CSV
-  std::string csv = existing.empty() ? sample : existing + "," + sample;
+  LOG_INFO("[AlleleCount] Recorded skipped deletion for sample {}", sample);
+}
 
-  // Write updated metadata
-  {
-    Array write_array(*ctx, uri, TILEDB_WRITE);
-    write_array.put_metadata(
-        "skipped_delete_samples", TILEDB_STRING_ASCII, csv.size(), csv.c_str());
+std::vector<std::string> AlleleCount::get_skipped_delete_samples(
+    std::shared_ptr<Context> ctx, const Group& group) {
+  auto uri = AlleleCount::group_uri(group);
+  if (uri.empty()) {
+    return {};
+  }
+
+  const std::string prefix = "skipped_delete_sample:";
+  std::vector<std::string> samples;
+  Array array(*ctx, uri, TILEDB_READ);
+  uint64_t n = array.metadata_num();
+  for (uint64_t i = 0; i < n; i++) {
+    std::string key;
+    tiledb_datatype_t type;
+    uint32_t num;
+    const void* value;
+    array.get_metadata_from_index(i, &key, &type, &num, &value);
+    if (key.rfind(prefix, 0) == 0) {
+      samples.push_back(key.substr(prefix.size()));
+    }
   }
-
-  LOG_INFO("[AlleleCount] Recorded skipped deletion for sample {}", sample);
+  return samples;
 }
 
 void AlleleCount::consolidate_commits(

libtiledbvcf/src/stats/allele_count.h

@@ -120,8 +120,9 @@ class AlleleCount {
   /**
    * @brief Record a skipped sample deletion in array metadata.
    *
-   * Reads existing "skipped_deletions" metadata (if any), appends the sample
-   * name, and writes back the updated CSV.
+   * Writes a per-sample metadata key "skipped_delete_sample:<sample>" to the
+   * array. Using one key per sample avoids the read-modify-write race condition
+   * that would arise from accumulating a single CSV value.
    *
    * @param ctx TileDB context
    * @param group TileDB-VCF dataset group
@@ -132,6 +133,19 @@ class AlleleCount {
       const Group& group,
       const std::string& sample);
 
+  /**
+   * @brief Return the list of samples whose deletion was skipped.
+   *
+   * Enumerates all array metadata keys with the prefix
+   * "skipped_delete_sample:" and returns the sample name suffixes.
+   *
+   * @param ctx TileDB context
+   * @param group TileDB-VCF dataset group
+   * @return Vector of sample names recorded as skipped
+   */
+  static std::vector<std::string> get_skipped_delete_samples(
+      std::shared_ptr<Context> ctx, const Group& group);
+
   /**
    * @brief Consolidate commits
    *

libtiledbvcf/src/stats/variant_stats.cc

@@ -318,30 +318,37 @@ void VariantStats::skip_delete_sample(
     return;
   }
 
-  // Read existing skipped_delete_samples metadata
-  std::string existing;
-  {
-    Array read_array(*ctx, uri, TILEDB_READ);
-    const void* value = nullptr;
-    tiledb_datatype_t type = TILEDB_ANY;
-    uint32_t num = 0;
-    read_array.get_metadata("skipped_delete_samples", &type, &num, &value);
-    if (value != nullptr && num > 0) {
-      existing = std::string(static_cast<const char*>(value), num);
-    }
-  }
+  // Write one key per sample to avoid a read-modify-write race condition.
+  // Concurrent callers write to different keys so no coordination is needed.
+  std::string key = "skipped_delete_sample:" + sample;
+  Array write_array(*ctx, uri, TILEDB_WRITE);
+  write_array.put_metadata(key, TILEDB_STRING_ASCII, 0, nullptr);
 
-  // Build updated CSV
-  std::string csv = existing.empty() ? sample : existing + "," + sample;
+  LOG_INFO("[VariantStats] Recorded skipped deletion for sample {}", sample);
+}
 
-  // Write updated metadata
-  {
-    Array write_array(*ctx, uri, TILEDB_WRITE);
-    write_array.put_metadata(
-        "skipped_delete_samples", TILEDB_STRING_ASCII, csv.size(), csv.c_str());
+std::vector<std::string> VariantStats::get_skipped_delete_samples(
+    std::shared_ptr<Context> ctx, const Group& group) {
+  auto uri = VariantStats::group_uri(group);
+  if (uri.empty()) {
+    return {};
+  }
+
+  const std::string prefix = "skipped_delete_sample:";
+  std::vector<std::string> samples;
+  Array array(*ctx, uri, TILEDB_READ);
+  uint64_t n = array.metadata_num();
+  for (uint64_t i = 0; i < n; i++) {
+    std::string key;
+    tiledb_datatype_t type;
+    uint32_t num;
+    const void* value;
+    array.get_metadata_from_index(i, &key, &type, &num, &value);
+    if (key.rfind(prefix, 0) == 0) {
+      samples.push_back(key.substr(prefix.size()));
+    }
   }
-
-  LOG_INFO("[VariantStats] Recorded skipped deletion for sample {}", sample);
+  return samples;
 }
 
 void VariantStats::consolidate_commits(

libtiledbvcf/src/stats/variant_stats.h

@@ -125,8 +125,9 @@ class VariantStats {
   /**
    * @brief Record a skipped sample deletion in array metadata.
    *
-   * Reads existing "skipped_deletions" metadata (if any), appends the sample
-   * name, and writes back the updated CSV.
+   * Writes a per-sample metadata key "skipped_delete_sample:<sample>" to the
+   * array. Using one key per sample avoids the read-modify-write race condition
+   * that would arise from accumulating a single CSV value.
    *
    * @param ctx TileDB context
    * @param group TileDB-VCF dataset group
@@ -137,6 +138,19 @@ class VariantStats {
       const Group& group,
       const std::string& sample);
 
+  /**
+   * @brief Return the list of samples whose deletion was skipped.
+   *
+   * Enumerates all array metadata keys with the prefix
+   * "skipped_delete_sample:" and returns the sample name suffixes.
+   *
+   * @param ctx TileDB context
+   * @param group TileDB-VCF dataset group
+   * @return Vector of sample names recorded as skipped
+   */
+  static std::vector<std::string> get_skipped_delete_samples(
+      std::shared_ptr<Context> ctx, const Group& group);
+
   /**
    * @brief Consolidate commits
    *

libtiledbvcf/test/src/unit-vcf-delete.cc

@@ -34,9 +34,12 @@
 
 #include "dataset/tiledbvcfdataset.h"
 #include "read/reader.h"
+#include "stats/allele_count.h"
+#include "stats/variant_stats.h"
 #include "utils/logger_public.h"
 #include "write/writer.h"
 
+#include <algorithm>
 #include <cstring>
 #include <fstream>
 #include <iostream>
@@ -312,28 +315,20 @@ TEST_CASE(
 
   // Verify skipped_delete_samples metadata on allele_count
   {
-    Array array(ctx, dataset_uri + "/allele_count", TILEDB_READ);
-    const void* value = nullptr;
-    tiledb_datatype_t type = TILEDB_ANY;
-    uint32_t num = 0;
-    array.get_metadata("skipped_delete_samples", &type, &num, &value);
-    REQUIRE(value != nullptr);
-    REQUIRE(type == TILEDB_STRING_ASCII);
-    std::string meta(static_cast<const char*>(value), num);
-    REQUIRE(meta == sample_name);
+    auto ctx_ptr = std::make_shared<tiledb::Context>();
+    tiledb::Group group(*ctx_ptr, dataset_uri, TILEDB_READ);
+    auto skipped = AlleleCount::get_skipped_delete_samples(ctx_ptr, group);
+    REQUIRE(skipped.size() == 1);
+    REQUIRE(skipped[0] == sample_name);
   }
 
   // Verify skipped_delete_samples metadata on variant_stats
   {
-    Array array(ctx, dataset_uri + "/variant_stats", TILEDB_READ);
-    const void* value = nullptr;
-    tiledb_datatype_t type = TILEDB_ANY;
-    uint32_t num = 0;
-    array.get_metadata("skipped_delete_samples", &type, &num, &value);
-    REQUIRE(value != nullptr);
-    REQUIRE(type == TILEDB_STRING_ASCII);
-    std::string meta(static_cast<const char*>(value), num);
-    REQUIRE(meta == sample_name);
+    auto ctx_ptr = std::make_shared<tiledb::Context>();
+    tiledb::Group group(*ctx_ptr, dataset_uri, TILEDB_READ);
+    auto skipped = VariantStats::get_skipped_delete_samples(ctx_ptr, group);
+    REQUIRE(skipped.size() == 1);
+    REQUIRE(skipped[0] == sample_name);
   }
 
   if (vfs.is_dir(dataset_uri)) {
@@ -384,26 +379,20 @@ TEST_CASE(
     dataset.delete_samples(dataset_uri, {"G1"}, {}, true);
   }
 
-  // Verify metadata is "G1" on both arrays
+  // Verify metadata records G1 on both arrays
   {
-    Array ac_array(ctx, dataset_uri + "/allele_count", TILEDB_READ);
-    const void* value = nullptr;
-    tiledb_datatype_t type = TILEDB_ANY;
-    uint32_t num = 0;
-    ac_array.get_metadata("skipped_delete_samples", &type, &num, &value);
-    REQUIRE(value != nullptr);
-    std::string meta(static_cast<const char*>(value), num);
-    REQUIRE(meta == "G1");
+    auto ctx_ptr = std::make_shared<tiledb::Context>();
+    tiledb::Group group(*ctx_ptr, dataset_uri, TILEDB_READ);
+    auto skipped = AlleleCount::get_skipped_delete_samples(ctx_ptr, group);
+    REQUIRE(skipped.size() == 1);
+    REQUIRE(skipped[0] == "G1");
   }
   {
-    Array vs_array(ctx, dataset_uri + "/variant_stats", TILEDB_READ);
-    const void* value = nullptr;
-    tiledb_datatype_t type = TILEDB_ANY;
-    uint32_t num = 0;
-    vs_array.get_metadata("skipped_delete_samples", &type, &num, &value);
-    REQUIRE(value != nullptr);
-    std::string meta(static_cast<const char*>(value), num);
-    REQUIRE(meta == "G1");
+    auto ctx_ptr = std::make_shared<tiledb::Context>();
+    tiledb::Group group(*ctx_ptr, dataset_uri, TILEDB_READ);
+    auto skipped = VariantStats::get_skipped_delete_samples(ctx_ptr, group);
+    REQUIRE(skipped.size() == 1);
+    REQUIRE(skipped[0] == "G1");
   }
 
   // Delete G2 with skip_aggregate_stats=true
@@ -413,26 +402,22 @@ TEST_CASE(
     dataset.delete_samples(dataset_uri, {"G2"}, {}, true);
   }
 
-  // Verify metadata accumulated to "G1,G2" on both arrays
+  // Verify metadata records both G1 and G2 on both arrays
   {
-    Array ac_array(ctx, dataset_uri + "/allele_count", TILEDB_READ);
-    const void* value = nullptr;
-    tiledb_datatype_t type = TILEDB_ANY;
-    uint32_t num = 0;
-    ac_array.get_metadata("skipped_delete_samples", &type, &num, &value);
-    REQUIRE(value != nullptr);
-    std::string meta(static_cast<const char*>(value), num);
-    REQUIRE(meta == "G1,G2");
+    auto ctx_ptr = std::make_shared<tiledb::Context>();
+    tiledb::Group group(*ctx_ptr, dataset_uri, TILEDB_READ);
+    auto skipped = AlleleCount::get_skipped_delete_samples(ctx_ptr, group);
+    REQUIRE(skipped.size() == 2);
+    REQUIRE(std::find(skipped.begin(), skipped.end(), "G1") != skipped.end());
+    REQUIRE(std::find(skipped.begin(), skipped.end(), "G2") != skipped.end());
   }
   {
-    Array vs_array(ctx, dataset_uri + "/variant_stats", TILEDB_READ);
-    const void* value = nullptr;
-    tiledb_datatype_t type = TILEDB_ANY;
-    uint32_t num = 0;
-    vs_array.get_metadata("skipped_delete_samples", &type, &num, &value);
-    REQUIRE(value != nullptr);
-    std::string meta(static_cast<const char*>(value), num);
-    REQUIRE(meta == "G1,G2");
+    auto ctx_ptr = std::make_shared<tiledb::Context>();
+    tiledb::Group group(*ctx_ptr, dataset_uri, TILEDB_READ);
+    auto skipped = VariantStats::get_skipped_delete_samples(ctx_ptr, group);
+    REQUIRE(skipped.size() == 2);
+    REQUIRE(std::find(skipped.begin(), skipped.end(), "G1") != skipped.end());
+    REQUIRE(std::find(skipped.begin(), skipped.end(), "G2") != skipped.end());
   }
 
   // Verify G3 still present, G1 and G2 are gone