From 451c5fc6a132f1f6199aaefd9e64659ea68a6ba0 Mon Sep 17 00:00:00 2001
From: "github-actions[bot]"
 <41898282+github-actions[bot]@users.noreply.github.com>
Date: Fri, 10 Jan 2025 11:07:23 -0500
Subject: [PATCH] [C++] Fix `ManagedQuery` segv: ctor/dtor order fix (#3544)
 (#3549)

* ctor/dtor order fix

* lint

* add comments

* lint

* add test case

* add commnet

Co-authored-by: Bruce Martin <bruce.martin@tiledb.com>
---
 apis/python/tests/test_sparse_nd_array.py | 28 +++++++++++++++++++++++
 libtiledbsoma/src/soma/managed_query.cc   |  8 +++----
 libtiledbsoma/src/soma/managed_query.h    | 12 ++++++----
 libtiledbsoma/src/soma/soma_array.h       |  4 ++++
 4 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/apis/python/tests/test_sparse_nd_array.py b/apis/python/tests/test_sparse_nd_array.py
index 406f3e5db6..95bd91ba57 100644
--- a/apis/python/tests/test_sparse_nd_array.py
+++ b/apis/python/tests/test_sparse_nd_array.py
@@ -2,6 +2,7 @@
 
 import contextlib
 import datetime
+import gc
 import itertools
 import json
 import operator
@@ -1979,3 +1980,30 @@ def test_iter(tmp_path: pathlib.Path):
         next(a)
     with pytest.raises(StopIteration):
         next(b)
+
+
+def test_context_cleanup(tmp_path: pathlib.Path) -> None:
+    arrow_tensor = create_random_tensor("table", (1,), np.float32(), density=1)
+    with soma.SparseNDArray.create(
+        tmp_path.as_uri(), type=pa.float64(), shape=(1,)
+    ) as write_arr:
+        write_arr.write(arrow_tensor)
+
+    def test(path, tiledb_config):
+        context = soma.SOMATileDBContext().replace(tiledb_config=tiledb_config)
+        X = soma.SparseNDArray.open(path, context=context, mode="r")
+        mq = X.read().tables()
+        return mq
+
+    for _ in range(100):
+        # Run test multiple times. While the C++ this tests (dtor order)
+        # is deterministic, it is triggered by the Python GC, which makes
+        # no specific guarantees about when it will sweep any given object.
+        _ = test(
+            tmp_path.as_uri(),
+            {
+                "vfs.s3.no_sign_request": "true",
+                "vfs.s3.region": "us-west-2",
+            },
+        )
+        gc.collect()
diff --git a/libtiledbsoma/src/soma/managed_query.cc b/libtiledbsoma/src/soma/managed_query.cc
index 58abe8d2c2..07ce3207ff 100644
--- a/libtiledbsoma/src/soma/managed_query.cc
+++ b/libtiledbsoma/src/soma/managed_query.cc
@@ -50,8 +50,8 @@ ManagedQuery::ManagedQuery(
     std::shared_ptr<Array> array,
     std::shared_ptr<Context> ctx,
     std::string_view name)
-    : array_(array)
-    , ctx_(ctx)
+    : ctx_(ctx)
+    , array_(array)
     , name_(name)
     , schema_(std::make_shared<ArraySchema>(array->schema())) {
     reset();
@@ -61,8 +61,8 @@ ManagedQuery::ManagedQuery(
     std::unique_ptr<SOMAArray> array,
     std::shared_ptr<Context> ctx,
     std::string_view name)
-    : array_(array->arr_)
-    , ctx_(ctx)
+    : ctx_(ctx)
+    , array_(array->arr_)
     , name_(name)
     , schema_(std::make_shared<ArraySchema>(array->arr_->schema())) {
     reset();
diff --git a/libtiledbsoma/src/soma/managed_query.h b/libtiledbsoma/src/soma/managed_query.h
index 85edbc49dc..b3e6ad9b30 100644
--- a/libtiledbsoma/src/soma/managed_query.h
+++ b/libtiledbsoma/src/soma/managed_query.h
@@ -98,8 +98,8 @@ class ManagedQuery {
     ManagedQuery(const ManagedQuery&) = delete;
 
     ManagedQuery(ManagedQuery&& other)
-        : array_(other.array_)
-        , ctx_(other.ctx_)
+        : ctx_(other.ctx_)
+        , array_(other.array_)
         , name_(other.name_)
         , schema_(other.schema_)
         , query_(std::make_unique<Query>(*other.ctx_, *other.array_))
@@ -499,12 +499,16 @@ class ManagedQuery {
         const CurrentDomain& current_domain, bool is_read);
     void _fill_in_subarrays_if_dense_without_new_shape(bool is_read);
 
-    // TileDB array being queried.
-    std::shared_ptr<Array> array_;
+    // NB: the Array dtor REQUIRES that this context be alive, so member
+    // declaration order is significant.  Context (ctx_) MUST be declared
+    // BEFORE Array (array_) so that ctx_ will be destructed last.
 
     // TileDB context object
     std::shared_ptr<Context> ctx_;
 
+    // TileDB array being queried.
+    std::shared_ptr<Array> array_;
+
     // Name displayed in log messages
     std::string name_;
 
diff --git a/libtiledbsoma/src/soma/soma_array.h b/libtiledbsoma/src/soma/soma_array.h
index 6b5e1a61d3..38afc1a07b 100644
--- a/libtiledbsoma/src/soma/soma_array.h
+++ b/libtiledbsoma/src/soma/soma_array.h
@@ -1554,6 +1554,10 @@ class SOMAArray : public SOMAObject {
     // SOMAArray name for debugging
     std::string name_;
 
+    // NB: the Array dtor REQUIRES that this context be alive, so member
+    // declaration order is significant.  Context (ctx_) MUST be declared
+    // BEFORE Array (arr_) so that ctx_ will be destructed last.
+
     // SOMA context
     std::shared_ptr<SOMAContext> ctx_;