Fix Python reference counting for declared_transform product store caching

[Copilot]

Python py: tests (vectypes, veclists, coverage) SEGFAULT intermittently because declared_transform caches product stores by hash, but Python converters were decrementing references owned by the cache—freeing objects that get reused on cache hits. Masked for small integers by CPython's immortal integer cache; crashes on floats and numpy arrays.

Reference counting fixes (modulewrap.cpp)

• Remove Py_DECREF from output converters (BASIC_CONVERTER py_to_*, NUMPY_ARRAY_CONVERTER py_to_*): input is a borrowed reference from the product store cache, not an owned reference
• Add Py_XINCREF/Py_XDECREF in call()/callv(): protect cached references during the Python function call
• Make lifeline_transform() symmetric in call() and callv() per original author
• VECTOR_CONVERTER error paths throw instead of returning (intptr_t)nullptr, which acts as PyObject_CallFunctionObjArgs sentinel and silently truncates arguments
• Remove decref_all/safe_decref helpers — replaced by inline Py_XDECREF fold expressions

Transform exception safety (declared_transform.hpp)

• try/catch around call(): stores_.insert() creates a null entry; if call() throws before assignment, the stale entry causes SEGFAULT on cache reuse

Lifeline allocation fix (lifelinewrap.cpp)

• ll_new: return nullptr on tp_alloc failure instead of falling through to null dereference

Documentation (python-refcounting.md)

Documents the ownership model for PyObject* references flowing through declared_transform caching, including why CPython's small integer cache masked this class of bug.

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[greenc-FNAL]

@phlexbot clang-fix

[github-actions]

Automatic clang-format fixes pushed (commit f466e7a).
⚠️ Note: Some issues may require manual review and fixing.

[greenc-FNAL]

Review the full CodeQL report for details.

[greenc-FNAL]

@phlexbot clang-fix

[github-actions]

No automatic clang-format fixes were necessary.

[knoepfel]

@greenc-FNAL, I suggest placing the declared_fold and store_counters changes into a separate PR. How all of that caching works will be changing substantially, and the different_hierarchies failure is unrelated to the Python failures.

[greenc-FNAL]

@phlexbot clang-fix

[github-actions]

Automatic clang-format fixes pushed (commit 56c45ee).
⚠️ Note: Some issues may require manual review and fixing.

[greenc-FNAL]

@greenc-FNAL, I suggest placing the declared_fold and store_counters changes into a separate PR. How all of that caching works will be changing substantially, and the different_hierarchies failure is unrelated to the Python failures.

Definitely: a lot of this is pointless thrashing around that will have to be pruned anyway (assuming it actually fixes either problem, which is not a given right now).

[Copilot]

Pull request overview

This PR addresses two correctness issues: premature fold completion due to a race in fold counters, and Python converter failures propagating null pointers into PyObject_CallFunctionObjArgs, which can truncate varargs and crash Python-based tests.

Changes:

• Harden Python callback/converter handling by ensuring lifeline unwrapping is consistent, converter failures throw, and decref is null-safe.
• Fix fold completion race by initializing ready_to_flush_ to false, adding a “pending work” guard, and keeping the TBB accessor across increment+completion checks.
• Improve transform exception safety by erasing stale stores_ entries when callbacks throw.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
plugins/python/src/modulewrap.cpp	Prevent null/sentinel propagation into Python C-API calls; make conversions fail-fast and decref null-safe.
phlex/core/store_counters.hpp	Add pending-work tracking and adjust flush readiness initialization for fold completion safety.
phlex/core/store_counters.cpp	Implement pending-aware completion checks and accessor-held increment/flush completion helpers.
phlex/core/declared_transform.hpp	Erase inserted-but-unassigned cache entries when callback throws to avoid stale null reuse.
phlex/core/declared_fold.hpp	Switch fold counter updates to new pending/atomic-safe helper methods.

Comments suppressed due to low confidence (1)

phlex/core/store_counters.cpp:59

• ++counts_[layer_hash]; default-constructs a std::atomic<std::size_t> when the key is new. std::atomic's default constructor does not value-initialize the contained value, so the first increment can read an indeterminate value (undefined behavior). Initialize the mapped atomic explicitly on first insert (e.g., emplace with std::atomic<std::size_t>{0} and then fetch_add(1)).

  void store_counter::increment(data_cell_index::hash_type const layer_hash)
  {
    ++counts_[layer_hash];
  }

[wlav]

I don't understand this one at all. It should not have been leaking before, but now you make it leak.

The idea was that each input has a unique pointer providing the conversion. This is why the node is named with a combo of the product and the consumer. The converter creates a new reference, the consumer removes the reference after using the object, so no leaks. It's all one-to-one.

The addition of the ref-count before, adds a total +1 compared to the prior code, so now leak.

The INCREF/DECREF around the call don't serve a purpose.

It is perfectly well possible that there's some misunderstanding on my side about the TBB graph, but leaking the products from converter nodes can not possibly be a solution in any way or form: there could be potentially millions of products.