Probable fix to tracing regions

Include/internal/pycore_immutability.h

@@ -26,6 +26,12 @@ PyAPI_FUNC(int) _PyTracingRegion_AttachIgnoreRegionRefs(PyObject* region);
  * and the validation, with the thread detached, so that other threads can
  * interfere with it. 0 disables the pause. */
 PyAPI_FUNC(void) _PyTracingRegion_SetTestPause(PyTime_t ns);
+
+/* Tests only: makes every close call `hook` on the closing thread right after
+ * its trace traversed `traversed`, so that a test can change the object graph
+ * between two traversals. NULL removes the hook. */
+typedef void (*_PyTracingRegion_TestTraceHook)(PyObject *traversed);
+PyAPI_FUNC(void) _PyTracingRegion_SetTestTraceHook(_PyTracingRegion_TestTraceHook hook);
 #endif
 
 /* Returns the region's metadata node, allocating it if this is the first

Lib/test/test_freeze/test_tracing_region.py

@@ -1002,6 +1002,45 @@ class A:
         self.assertEqual(c.value.local_ref().tag, "local")
 
 
+_testinternalcapi = import_helper.import_module("_testinternalcapi")
+
+@unittest.skipUnless(hasattr(_testinternalcapi, "region_set_trace_move"),
+                     "needs the trace hook of debug builds")
+class TestGraphChangedDuringTrace(unittest.TestCase):
+    """The object graph changes between two traversals of a close, as if a
+    thread holding references into the region changed it concurrently."""
+
+    def tearDown(self):
+        _testinternalcapi.region_set_trace_move(None, None, 0)
+
+    def test_moved_references_cannot_hide_incoming_references(self):
+        # `dst` is only reachable through `src`, so the trace traverses `src`
+        # first. Right after, the hook moves two items from `src` to `dst`
+        # without changing any refcount, so the trace counts each item's one
+        # reference twice. The hook holds a reference to `src` and to `dst`,
+        # which must keep the region open: two references too few on the
+        # items must not make up for two too many on the lists.
+        #
+        # A close that freezes the region type traces again, which would
+        # only see the graph after the move, so the type is frozen up front.
+        deep_freeze(Region)
+        r = Region()
+        dst = []
+        src = [dst, [], []]
+        r.src = src
+        _testinternalcapi.region_set_trace_move(src, dst, 2)
+        del src, dst
+
+        with self.assertRaises(RuntimeError):
+            _testinternalcapi.region_close(r)
+        self.assertFalse(_testinternalcapi.region_is_closed(r))
+
+        # Once the references are dropped, the region closes.
+        _testinternalcapi.region_set_trace_move(None, None, 0)
+        _testinternalcapi.region_close(r)
+        self.assertTrue(_testinternalcapi.region_is_closed(r))
+
+
 @unittest.skipUnless(support.Py_GIL_DISABLED,
                      "cross-thread object ownership only exists on free threading")
 class TestCrossThreadClose(unittest.TestCase):

Modules/_testinternalcapi.c

@@ -2465,13 +2465,79 @@ region_set_close_pause(PyObject *self, PyObject *seconds)
     _PyTracingRegion_SetTestPause(ns);
     Py_RETURN_NONE;
 }
+
+/* Strong references, standing in for a thread that took them before a close
+ * started and holds them throughout. */
+static PyObject *trace_move_src = NULL;
+static PyObject *trace_move_dst = NULL;
+static Py_ssize_t trace_move_count = 0;
+
+/* Moves `trace_move_count` items from the end of `trace_move_src` to the end
+ * of `trace_move_dst`, right after the trace traversed the source. Each item
+ * ends up with the refcount it started with, as with `dst.append(src.pop())`
+ * done by a thread that does not take references of its own. Runs once. */
+static void
+trace_move_hook(PyObject *traversed)
+{
+    if (traversed != trace_move_src || trace_move_count == 0) {
+        return;
+    }
+    PyObject *src = trace_move_src;
+    PyObject *dst = trace_move_dst;
+    Py_BEGIN_CRITICAL_SECTION2(src, dst);
+    for (; trace_move_count > 0; trace_move_count--) {
+        Py_ssize_t size = PyList_GET_SIZE(src);
+        if (size == 0) {
+            break;
+        }
+        PyObject *item = Py_NewRef(PyList_GET_ITEM(src, size - 1));
+        if (PyList_SetSlice(src, size - 1, size, NULL) < 0
+            || PyList_Append(dst, item) < 0)
+        {
+            PyErr_FormatUnraisable("Exception ignored in the trace move hook");
+        }
+        Py_DECREF(item);
+    }
+    Py_END_CRITICAL_SECTION2();
+    trace_move_count = 0;
+}
+
+/* region_set_trace_move(src, dst, count): the next close that traverses the
+ * list `src` moves `count` items from it to the list `dst` right after, while
+ * this module holds a reference to each list. region_set_trace_move(None,
+ * None, 0) removes the hook and drops the references. */
+static PyObject *
+region_set_trace_move(PyObject *self, PyObject *args)
+{
+    PyObject *src, *dst;
+    Py_ssize_t count;
+    if (!PyArg_ParseTuple(args, "OOn", &src, &dst, &count)) {
+        return NULL;
+    }
+    if (src != Py_None && (!PyList_Check(src) || !PyList_Check(dst))) {
+        PyErr_SetString(PyExc_TypeError, "expected two lists or two Nones");
+        return NULL;
+    }
+    _PyTracingRegion_SetTestTraceHook(NULL);
+    Py_CLEAR(trace_move_src);
+    Py_CLEAR(trace_move_dst);
+    trace_move_count = 0;
+    if (src != Py_None) {
+        trace_move_src = Py_NewRef(src);
+        trace_move_dst = Py_NewRef(dst);
+        trace_move_count = count;
+        _PyTracingRegion_SetTestTraceHook(trace_move_hook);
+    }
+    Py_RETURN_NONE;
+}
 #endif
 
 static PyMethodDef module_functions[] = {
     {"region_close", region_close, METH_O},
     {"region_is_closed", region_is_closed, METH_O},
 #ifdef Py_DEBUG
     {"region_set_close_pause", region_set_close_pause, METH_O},
+    {"region_set_trace_move", region_set_trace_move, METH_VARARGS},
 #endif
     {"get_configs", get_configs, METH_NOARGS},
     {"get_recursion_depth", get_recursion_depth, METH_NOARGS},

Objects/tracingregionobject.c

@@ -160,7 +160,7 @@
 
 // Removes slot i, discarding its item; swaps in the last element, so order is
 // not preserved. Caller must own the only reference to the list.
 static void list_remove(PyObject *list, Py_ssize_t i)
 {
     Py_ssize_t last = PyList_GET_SIZE(list) - 1;
     PyObject *removed = PyList_GET_ITEM(list, i);
@@ -777,6 +777,10 @@
     // to restart to be valid
     bool restart;
 
+    // Set if some object was reached through more internal references than
+    // its RC allowed. Only reported if the trace does not restart.
+    bool overcounted;
+
     // Indicates if the given reference is a strong reference or a weak one.
     bool strong_ref;
 
@@ -844,6 +848,7 @@
     state->external_rc = 0;
     state->bridge_rc = 0;
     state->restart = false;
+    state->overcounted = false;
     // References are strong unless the trace explicitly follows a weak one.
     state->strong_ref = true;
     state->has_weak_refs = false;
@@ -1664,6 +1669,11 @@
             visit_info->unaccounted_rc -= 1;
             dbg("    - Internal reference to %p; LRC -= 1", obj);
             state->external_rc -= 1;
+            // More internal edges than the RC recorded at discovery: an edge
+            // was counted twice, and the excess could hide an external ref.
+            if (visit_info->unaccounted_rc < 0) {
+                state->overcounted = true;
+            }
         }
         return 0;
     }
@@ -1733,6 +1743,12 @@
 }
 
 #ifdef Py_DEBUG
+static _PyTracingRegion_TestTraceHook region_test_trace_hook = NULL;
+
+void _PyTracingRegion_SetTestTraceHook(_PyTracingRegion_TestTraceHook hook) {
+    _Py_atomic_store_ptr_relaxed(&region_test_trace_hook, (void *)hook);
+}
+
 static PyTime_t region_test_pause_ns = 0;
 
 void _PyTracingRegion_SetTestPause(PyTime_t ns) {
@@ -1810,6 +1826,13 @@
         state.src = item;
         dbg("  - traversing %p", item);
         SUCCEEDS(_PyObject_VisitReachable(item, (visitproc)_trace_visit, (void*)&state));
+#ifdef Py_DEBUG
+        _PyTracingRegion_TestTraceHook hook = (_PyTracingRegion_TestTraceHook)
+            _Py_atomic_load_ptr_relaxed(&region_test_trace_hook);
+        if (hook != NULL) {
+            hook(item);
+        }
+#endif
 
         if (PyWeakref_Check(item)) {
             PyWeakReference *wref = (PyWeakReference*)item;
@@ -1842,6 +1865,17 @@
         goto finally;
     }
 
+    // Checked after the restart, since code run by freezing may have moved
+    // references, which a fresh trace sees correctly.
+    if (state.overcounted) {
+#ifdef _Py_PYRONA_INTERPRETER_SHARING
+        gc_list_dissolve(&region->gc_list);
+#endif
+        PyErr_SetString(PyExc_RuntimeError,
+            "the region cannot be closed since a concurrent thread accessed during tracing");
+        goto error;
+    }
+
     // Report an error, if the region couldn't be closed.
     if (state.external_rc > 0) {
 #ifdef _Py_PYRONA_INTERPRETER_SHARING
@@ -2304,7 +2338,7 @@
  */
 int _PyTracingRegion_Close(PyObject* op) {
 #ifdef _Py_PYRONA_INTERPRETER_SHARING
     TracingRegionObject *self = _PyTRegion_CAST(op);
 
     // A closed region keeps its members in its GC list
     assert(!region_is_open(self) || gc_list_is_empty(&self->gc_list));