free output array when single-axis nan reducers raise

[sahvx655-wq]

Output array leaked when single-axis reducers raise

Reading through the _one reduce functions I noticed the nan-aware ones allocate their output array with PyArray_EMPTY in INIT_ONE and then bail out with a bare return NULL on two error paths: an empty reduction axis (shape[axis] == 0), and for nanargmin/nanargmax an all-NaN slice. Neither path drops the reference, so a call like bn.nanmin(np.zeros((4, 0)), axis=1) or bn.nanargmin over an all-NaN row leaks the array that was just built. The buffer is never written before the raise, so resident memory stays flat and the leak slips past the existing ru_maxrss test, but tracemalloc and the output dtype refcount both climb by one on every call.

Unfixed it accumulates in any loop that probes reductions and swallows the ValueError, which is normal behaviour. The fix drops the reference before each of those returns so the error paths match the normal exit; I also added a refcount regression that fails on master and passes with the change.

[neutrinoceros]

To my surprise, I checked out the test on my machine (without the patch), and found that it succeeded. I'm not saying the patch isn't correct, but maybe the test isn't reliable on all platforms. I'd like to see if CI captures the problem.
Could you split you commit as follows ?

• add just the test, decorated with @pytest.mark.xfail(strict=True)
• add the patch and remove the xfail decorator

[sahvx655-wq]

Split as you asked. First commit adds just the test with @pytest.mark.xfail(strict=True), the second adds the patch and drops the decorator, so it goes from xfail to a normal pass once the fix lands.

On the reliability point: with the decorator in place the unpatched test reports XFAIL here (macOS, numpy 2.x), so it does catch the leak on my box. The assertion watches the singleton dtype refcount, which gains one ref per leaked output array, and that refcount signal is the part I'm less sure travels to every platform. If it doesn't climb somewhere, the strict marker turns the run into an xpass and CI fails loudly, which is exactly the behaviour you want to see. When I first tracked this down tracemalloc was the cleaner probe (~624 B/call before the fix, ~0 after), so if the refcount turns out not to be portable I'm happy to reassert on tracemalloc instead.

[neutrinoceros]

Thanks !
The failure on 3.14t is unrelated (#574)

with the decorator in place the unpatched test reports XFAIL here (macOS, numpy 2.x)

my setup is similar, at least at this level of accuracy, so I still don't know why I'm getting different results.
I realise I wasn't explicit enough in my instructions. I was expecting CI to be run on both commits, which requires that the first one be pushed on its own, and the second one only re-added after the first run completes.

[sahvx655-wq]

Pushed the test commit on its own now, so this run exercises the strict-xfail in isolation. If the dtype refcount climbs the way it does on my box, every job reports XFAIL and stays green; if it doesn't climb on some runner, strict mode turns that into an XPASS and the job goes red, which is exactly the signal we're after for the portability question you raised. One small change from the earlier push: ruff-format wanted single spaces before the inline comments, so I reformatted and the commit SHA moved, but it's still test-only.

I'll re-add the fix on top once this run finishes, so the follow-up run shows the same test flipping from XFAIL to an ordinary pass. If the refcount probe turns out not to travel to every platform, I'm happy to reassert on tracemalloc, which gave the cleaner before/after signal when I first tracked this down.

[sahvx655-wq]

That run gave us the answer: the refcount probe is reliable on 3.10-3.12 but XPASSes under strict on 3.13/3.14 across every OS, so it isn't portable. The reason is that numpy's builtin dtype singletons are immortal on 3.13+, so a leaked output array no longer bumps their refcount and the assertion can't see the leak. The leak in the C code is real on every version; only that particular probe goes blind. I built the branch unpatched against 3.13 locally to confirm: the float64 dtype refcount moved by 0 over 1000 leaky calls, while tracemalloc reported ~144 B/call on the leaking paths against ~0 on the happy-path controls, and with the Py_DECREF in place the leaking paths drop back to ~0 too.

So I've reworked the test to watch net allocation with tracemalloc rather than the dtype refcount, and marked it thread_unsafe like test_memory_leak since the measurement is process-global. On the unpatched 3.13 build it overshoots the threshold by ~170 KB over the loop and goes green again once the fix lands. Pushed just that test commit, so this run should report XFAIL across the matrix; I'll re-add the fix on top once it's green.

[sahvx655-wq]

First run is in and green across the whole matrix, including the 3.13/3.14 and free-threaded jobs that tripped strict before, so the tracemalloc probe reliably reports XFAIL everywhere now rather than going blind the way the refcount one did.

Re-added the fix on top, so this run should show the same test flip from XFAIL to a normal pass. The fix commit is just the five Py_DECREF(y) calls on the error returns plus dropping the xfail marker, nothing else moved.

[neutrinoceros]

Would it make sense to use @pytest.mark.parametrize instead of this internal loop ? If there's ever a regression it'll make it easier to pin point any leaky function(s).

[sahvx655-wq]

Good shout. Switched it to parametrize with one case per function, each with an id like nanmin-empty / nanargmin-allnan, so a regression names the leaky function straight away instead of failing the whole loop.

I checked it still bites: reverting the five Py_DECREF calls and rebuilding fails all six cases, each pointing at its own function, and they go green again with the fix in. The threshold is now per-call (rounds * 16 bytes), so the per-function budget stays as tight as it was before.

[neutrinoceros]

Perfect. Thanks a lot for your diligence conducting this investigation.
I have a final (minor) suggestion left, but overall I'm very happy with the current state !

[neutrinoceros]

Interesting, parametrizing the test function apparently made it much more likely to hit #574 in free-threaded builds. I think it's clear that your patch isn't related but it may indicate that we need to tackle #574 sooner rather than later. I don't know when I'll be able to look at it in details yet. Would you like to give it a try ?

[sahvx655-wq]

Happy to take #574. Digging through this run's logs first: there are two different failures hiding under the red. The Linux 3.14t job is the pre-existing test_memory_leak tripping its exact-zero ru_maxrss assertion (diff_bytes came out at 524288), which is #574 proper. The five Windows 3.14t jobs are my new test, and the numbers say measurement floor rather than leak: every case overshoots by a fixed ~6.8 KB no matter which function it is, about 34 B/call, where the real leak reads ~140 B/call. The pre-parametrize run was green on those same runners with the same 1200 calls because they shared one tracemalloc window with a 19.2 KB budget; splitting them into six windows of 3.2 KB each left no room for the per-window overhead the free-threaded interpreter retains.

So I've pushed a budget rework on the test: 16 B/call plus a fixed 16 KiB per-window allowance, with rounds doubled to 400 so the leak signal stays well clear of it. Checked both ways locally: a build without the Py_DECREFs fails all six cases at ~58 KB against the 22.8 KB budget, and the fixed build passes on 3.11 and on a free-threaded 3.14.3 build, where the per-window noise measures ~650 B, so the allowance has about 2.4x headroom over the worst the Windows runners showed. For #574 itself the exact-zero assertion on a process-wide RSS high-water mark looks like the fragile half, and left as is it'll keep red-flagging unrelated PRs on 3.13t/3.14t; I'll dig into whether anything real hides underneath it and follow up on the issue.

[sahvx655-wq]

any update?