internal: Avoid parallel test triggered GC to interfere with test_gc.rs

[tobni]

I suffered flaky test runs in #6200. test_super_traverse_early_return_does_not_abort (added in 2188265) fails reliably under parallelism because its process-global flag gets set by any concurrent GC traversal prior to this patch.

Closes #6243

[MatthieuDartiailh]

I wonder if it will help #6224 ?

[tobni]

@MatthieuDartiailh The failures I saw were trivially reproducible with cargo default parallelism. You can try this locally in a loop to net a failure (assuming you can point to a free-threaded interpreter):

PYO3_PYTHON=$HOME/.pyenv/versions/3.14.6t/bin/python3.14t \
  cargo test --no-default-features --features=full,multiple-pymethods --test test_gc

it should not flake after cherry-picking a372ef7 onto your changes if this fixes the issue.

[jonasdedden]

I also stumbled over this issue and opened something here: #6243

I'm really no expert on this, but just FYI, in the "Suggested fix" section (entirely LLM-based, sorry 😅 🙏) the fix with the thread-local Cell was a first idea, but then an alternative fix with not involving the GC at all came up that has some additional benefits such as testing the return value and being deterministic. Feel free to ignore me if that was nonsense though.

[tobni]

@jonasdedden I'm unfamiliar with this projects CoC w.r.t LLM usage in PRs and discussions, but my thoughts on the matter is captured here: https://tombedor.dev/human-attention-and-human-effort/.

1. Asserting the return-value wasn't in the original test case. I have no reason to believe that is an improvement on its own merits, but I am a novice as well.
2. The test running against the real collector is coverage I'd rather not drop as it changes what is under test.
3. The variant your bot has suggested can still re-introduce the flake if any point yields to the interpreter in the future.

Edit: I just saw david's response in your opened issue which does give some insight on the stance of using LLM:s to communicate.

[jonasdedden]

Thanks @tobni! As I said, just wanted to share this as I anyways briefly looked into it, but don't want to spam anybody with AI slop as I'm not an expert around this. I probably can contribute more effectively on the stubgen parts of PyO3. Will keep it to that :)

[davidhewitt]

Thanks for the PR, and sorry I let the original flake slip through the review cracks.

I think the right fix might be to decouple this test from actual Python GC and write a custom visit function to drive the tp_traverse? You're right that it's nice to rely on the Python GC but I think even tricks with thread-locals might fall over if e.g. the GC is running on its own thread. I think we'll end up with too many moving parts that are specific to fitting this test into the GC rather than trying to reproduce the original bug.

It should be possible to make the custom visit function only return nonzero when traversing the field on the base type (e.g. by setting a global variable to the pointer value of the field).

For what it's worth, the AI output in #6243 also suggests driving tp_traverse directly (like some other tests already do) but I think reusing visit_error is wrong here as the test will just break again if we reorder pyclass traversal implementations.

[tobni]

Thanks for the PR, and sorry I let the original flake slip through the review cracks.

I think the right fix might be to decouple this test from actual Python GC and write a custom visit function to drive the tp_traverse? You're right that it's nice to rely on the Python GC but I think even tricks with thread-locals might fall over if e.g. the GC is running on its own thread. I think we'll end up with too many moving parts that are specific to fitting this test into the GC rather than trying to reproduce the original bug.

It should be possible to make the custom visit function only return nonzero when traversing the field on the base type (e.g. by setting a global variable to the pointer value of the field).

For what it's worth, the AI output in #6243 also suggests driving tp_traverse directly (like some other tests already do) but I think reusing visit_error is wrong here as the test will just break again if we reorder pyclass traversal implementations.

Right, makes sense. I have pushed a patch. I attempted to mirror how tests/test_gc.rs::traverse_partial was structured, including inlining supporting pyclass setup.

[davidhewitt]

Thanks, this looks great to me!

Given the mention of AI CoC in the discussion here, I would like to briefly mention our draft policy in #6141. I'd welcome any opinions on whether the text there strikes the right balance of being permissive for contributors to use AI appropriately whilst being clear about what is likely to be disrespectful of the community.

[davidhewitt]

Setting the _visited flags from within the visitor function feels nice (matches what the Python gc is essentially doing in its real implementations with more complex state).