fix: run argument-to-argument keep_alive before the call again

Moving keep_alive<N, P> (N, P != 0) to postcall meant a keep_alive error
was raised after the function ran, so its side effects stayed. Run the
precall hook in call_impl after load_args succeeds instead, so a failed
overload still does not trigger it and an error still stops the call.

Also release the return value when a postcall keep_alive throws, as the
dispatcher drops it.

Assisted-by: ClaudeCode:claude-opus-5-5

Author: henryiii

include/pybind11/attr.h

@@ -409,6 +409,7 @@ struct process_attribute_default {
     /// Default implementation: do nothing
     static void init(const T &, function_record *) {}
     static void init(const T &, type_record *) {}
+    /// Runs after argument conversion succeeded, before the call.
     static void precall(function_call &) {}
     /// The handle is not always a valid object: it is the PYBIND11_TRY_NEXT_OVERLOAD sentinel
     /// if argument conversion failed, and null if the call or return-value conversion failed.
@@ -652,12 +653,21 @@ struct process_attribute<call_guard<Ts...>> : process_attribute_default<call_gua
 
 /**
  * Process a keep_alive call policy -- invokes keep_alive_impl during the
- * post-call handler so that it only runs after a successful call, never for an
- * overload that failed argument conversion or a call that raised.
+ * pre-call handler (after argument conversion succeeded) if both Nurse,
+ * Patient != 0 and use the post-call handler otherwise
  */
 template <size_t Nurse, size_t Patient>
 struct process_attribute<keep_alive<Nurse, Patient>>
     : public process_attribute_default<keep_alive<Nurse, Patient>> {
+    template <size_t N = Nurse, size_t P = Patient, enable_if_t<N != 0 && P != 0, int> = 0>
+    static void precall(function_call &call) {
+        keep_alive_impl(Nurse, Patient, call, handle());
+    }
+    template <size_t N = Nurse, size_t P = Patient, enable_if_t<N != 0 && P != 0, int> = 0>
+    static void postcall(function_call &, handle) {}
+    template <size_t N = Nurse, size_t P = Patient, enable_if_t<N == 0 || P == 0, int> = 0>
+    static void precall(function_call &) {}
+    template <size_t N = Nurse, size_t P = Patient, enable_if_t<N == 0 || P == 0, int> = 0>
     static void postcall(function_call &call, handle ret) {
         keep_alive_impl(Nurse, Patient, call, ret);
     }

include/pybind11/pybind11.h

@@ -489,7 +489,9 @@ class cpp_function : public function {
     // actual function lambda so that we can get code reuse for
     // functions with the same Return, Args, and Guard.
     template <typename Return, typename Guard, typename ArgsConverter, typename... Args>
-    static handle call_impl(detail::function_call &call, detail::function_ref<Return(Args...)> f) {
+    static handle call_impl(detail::function_call &call,
+                            detail::function_ref<Return(Args...)> f,
+                            void (*precall)(detail::function_call &)) {
         using namespace detail;
         // Static assertion: function_ref must be trivially copyable to ensure safe pass-by-value.
         // Lifetime safety: The function_ref is created from cap->f which lives in the capture
@@ -505,6 +507,9 @@ class cpp_function : public function {
             return PYBIND11_TRY_NEXT_OVERLOAD;
         }
 
+        /* Invoke call policy pre-call hook, only for the overload that matched */
+        precall(call);
+
         /* Override policy for rvalues -- usually to enforce rvp::move on an rvalue */
         return_value_policy policy
             = return_value_policy_override<Return>::policy(call.func.policy);
@@ -585,9 +590,6 @@ class cpp_function : public function {
 
         /* Dispatch code which converts function arguments and performs the actual function call */
         rec->impl = [](function_call &call) -> handle {
-            /* Invoke call policy pre-call hook */
-            process_attributes<Extra...>::precall(call);
-
             /* Get a pointer to the capture object */
             const auto *data = (sizeof(capture) <= sizeof(call.func.data) ? &call.func.data
                                                                           : call.func.data[0]);
@@ -597,7 +599,9 @@ class cpp_function : public function {
                                     /* Function scope guard -- defaults to the compile-to-nothing
                                        `void_type` */
                                     extract_guard_t<Extra...>,
-                                    cast_in>(call, detail::function_ref<Return(Args...)>(cap->f));
+                                    cast_in>(call,
+                                             detail::function_ref<Return(Args...)>(cap->f),
+                                             &process_attributes<Extra...>::precall);
 
             /* Invoke call policy post-call hook */
             process_attributes<Extra...>::postcall(call, result);
@@ -3387,14 +3391,14 @@ PYBIND11_NOINLINE void keep_alive_impl(handle nurse, handle patient) {
 
 PYBIND11_NOINLINE void
 keep_alive_impl(size_t Nurse, size_t Patient, function_call &call, handle ret) {
-    // The dispatcher runs postcall even when the overload produced no value: `ret` is the
-    // PYBIND11_TRY_NEXT_OVERLOAD sentinel ((PyObject *) 1) if the overload bailed out of
-    // `load_args`, and null if the return-value conversion failed (with the real error
-    // already set). In both cases there was no successful call and there is no relationship
-    // to establish. Checked here because the sentinel is neither null nor `Py_None`, so it
-    // passes straight through the guards below and is dereferenced, and because a null `ret`
-    // would be reported as "Could not activate keep_alive!", masking the real error.
-    if (!ret || ret.ptr() == PYBIND11_TRY_NEXT_OVERLOAD) {
+    // With index 0, this runs in postcall, which the dispatcher runs even when the overload
+    // produced no value: `ret` is the PYBIND11_TRY_NEXT_OVERLOAD sentinel ((PyObject *) 1) if
+    // the overload bailed out of `load_args`, and null if the return-value conversion failed
+    // (with the real error already set). There is no relationship to establish then. The
+    // sentinel would pass the guards below and be dereferenced, and a null `ret` would be
+    // reported as "Could not activate keep_alive!", masking the real error.
+    const bool uses_ret = Nurse == 0 || Patient == 0;
+    if (uses_ret && (!ret || ret.ptr() == PYBIND11_TRY_NEXT_OVERLOAD)) {
         return;
     }
 
@@ -3411,7 +3415,15 @@ keep_alive_impl(size_t Nurse, size_t Patient, function_call &call, handle ret) {
         return handle();
     };
 
-    keep_alive_impl(get_arg(Nurse), get_arg(Patient));
+    try {
+        keep_alive_impl(get_arg(Nurse), get_arg(Patient));
+    } catch (...) {
+        // The dispatcher drops `ret` when postcall throws, so release it here.
+        if (uses_ret) {
+            ret.dec_ref();
+        }
+        throw;
+    }
 }
 
 inline std::pair<decltype(internals::registered_types_py)::iterator, bool>

tests/test_call_policies.cpp

@@ -35,7 +35,7 @@ TEST_SUBMODULE(call_policies, m) {
     // Parent/Child are used in:
     // test_keep_alive_argument, test_keep_alive_return_value, test_alive_gc_derived,
     // test_alive_gc_multi_derived, test_return_none, test_keep_alive_constructor,
-    // test_keep_alive_failed_overload
+    // test_keep_alive_failed_overload, test_keep_alive_error
     class Child {
     public:
         Child() { py::print("Allocating child."); }
@@ -67,6 +67,19 @@ TEST_SUBMODULE(call_policies, m) {
         .def_static("staticFunction", &Parent::staticFunction, py::keep_alive<1, 0>());
 
     m.def("free_function", [](Parent *, Child *) {}, py::keep_alive<1, 2>());
+
+    // test_keep_alive_error
+    static int keep_alive_error_calls = 0;
+    m.def(
+        "keep_alive_error_args",
+        [](const py::object &, Child *) { ++keep_alive_error_calls; },
+        py::keep_alive<1, 2>());
+    m.def("keep_alive_error_calls", [] { return keep_alive_error_calls; });
+    m.def(
+        "keep_alive_error_return",
+        [](const py::object &) { return new Child(); },
+        py::keep_alive<1, 0>());
+
     m.def("invalid_arg_index", [] {}, py::keep_alive<0, 1>());
 
 #if !defined(PYPY_VERSION)

tests/test_call_policies.py

@@ -288,3 +288,21 @@ def test_keep_alive_failed_return_conversion():
     """A failed return-value conversion must raise its own error, not a keep_alive one."""
     with pytest.raises(TypeError, match="Unable to convert function return value"):
         m.keep_alive_unregistered_return(m.KeepAliveOverload())
+
+
+@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC")
+def test_keep_alive_error(capture):
+    """A keep_alive error must not leave side effects or leak the return value."""
+    n_inst = ConstructorStats.detail_reg_inst()
+    c = m.Child()
+    # An int nurse cannot hold a weak reference, so keep_alive<1, 2> fails.
+    with pytest.raises(TypeError, match="weak reference"):
+        m.keep_alive_error_args(1, c)
+    assert m.keep_alive_error_calls() == 0
+    del c
+    with capture:
+        with pytest.raises(TypeError, match="weak reference"):
+            m.keep_alive_error_return(1)
+        pytest.gc_collect()
+    assert capture == "Allocating child.\nReleasing child."
+    assert ConstructorStats.detail_reg_inst() == n_inst