fix: keep each native panic's backtrace with its own thread

[viirya]

Which issue does this PR close?

Closes #7041.

Rationale for this change

The panic hook stores the backtrace of every native panic in one process-wide slot, and the JNI boundary takes whatever the slot holds when a panic becomes a CometNativeException. When two threads panic at about the same time, one exception can carry the other thread's stack trace and the other none, so the stack trace points at code that did not fail. Panics that Comet catches itself (for example in the variant casts) also write to the slot, and another thread's boundary can take their backtrace.

What changes are included in this PR?

The captured panic in native/jni-bridge/src/errors.rs becomes a thread-local instead of a global Mutex. The panic hook records the panic's message with its backtrace, and NativeFailure::from takes the backtrace only when that message matches the CometError::Panic being reported. try_unwrap_or_throw and catch_native forget the captured panic of their thread before running the call.

Not every panic reaches the boundary on the thread that raised it. DataFusion resumes the panics of its spawned tasks on the consuming thread with resume_unwind (RecordBatchReceiverStream, used by CoalescePartitionsExec and by spawn_buffered in SortPreservingMergeExec and the spill reader, and SpawnedTask::join_unwind), and resume_unwind does not run the panic hook. Such a panic now gets no backtrace, as any other error does. Clearing at the boundary keeps it from taking a backtrace left by a panic handled before the call, and the message check from one handled during the call. A process-wide fallback for this case is left out because it would bring back #7041. The hook still logs the panic and its location to stderr on the thread that raised it.

The hook uses try_with, because a panic inside a panic hook aborts the process and the thread-local is gone while its thread is being torn down.

How are these changes tested?

• panic_backtrace_stays_with_its_thread makes two threads panic inside catch_native. Each thread drops a value while it unwinds that waits until both panic hooks have run, so neither takes its backtrace before the other thread's hook has written. It checks that each failure carries the backtrace of its own panic, and fails on the current code.
• A panic raised on another thread and resumed inside catch_native gets no backtrace: with nothing captured on the thread, after a panic caught and handled during the call, and after a handled panic with the same message before the call. Removing the message check fails the second test, and removing the clear at the boundary fails the third.
• A panic with a formatted message keeps its own backtrace.
• The existing JVM-backed tests in errors.rs, including the panic-to-exception test, pass unchanged.

This pull request and its description were written by Isaac.

[andygrove]

Thanks for tracking this down @viirya. The cross-thread mix-up is a real bug and the test is a nice deterministic repro. CI is green and I see no change to results, errors, fallbacks or performance against branch-1.1. Only the stack trace attached to a CometNativeException is affected.

I have one concern about panics that cross a Tokio task. The description says every conversion to CometError::Panic happens on the thread that panicked, but I don't think that holds when a plan is polled on the JNI thread (the ScanExec path, get_runtime().block_on(next_batch(...)) in jni_api.rs). DataFusion's RecordBatchReceiverStream (datafusion-physical-plan stream.rs) and SpawnedTask::join_unwind (datafusion-common-runtime common.rs) catch a panic from a spawned task and call std::panic::resume_unwind(e.into_panic()) on the consuming thread. resume_unwind does not run the panic hook, so the backtrace goes into the Tokio worker's slot. The catch_unwind on the JNI thread then builds CometError::Panic, and NativeFailure::from takes that thread's own slot. That slot is empty, or holds a stale backtrace from an earlier panic that Comet caught itself, such as the variant casts, which never clear it. With the global slot, that case got the worker's backtrace, which is the useful one for a panic in a repartition or coalesce task. I reasoned this from the code and haven't run a repro.

Does a panic in a RepartitionExec or CoalescePartitionsExec task still produce a backtrace on the JVM side after this change? Would you consider falling back to a process-wide "most recent panic" slot when the current thread's slot is empty, or handling that case another way? A test where a tokio::spawned task panics and is resumed on the calling thread inside catch_native would pin down the behavior either way.

Related to that, should the catch sites that recover from a panic clear the slot so a stale backtrace can't be attached to a later panic on the same pooled thread?

Once we settle that, could you adjust the sentence in the description about every conversion happening on the panicking thread?

[rich7420]

I reproduced Andy's concern in a component test: a panic resumed on another thread can receive a backtrace from an earlier, handled panic. Could we prevent stale traces, test both empty and stale slots, and correct the same-thread assumption? Returning no backtrace is reasonable here, but a global fallback would reintroduce #7041.

[viirya]

Thanks @andygrove and @rich7420, you're right that the same-thread assumption doesn't hold. RecordBatchReceiverStream and SpawnedTask::join_unwind resume a spawned task's panic on the consuming thread with resume_unwind, which skips the hook. That covers CoalescePartitionsExec, and SortPreservingMergeExec and the spill reader through spawn_buffered. RepartitionExec isn't one of them: its wait_for_task joins the input task with join() and sends the panic on as a DataFusionError ("Join Error"), so it never becomes CometError::Panic, before or after this change.

I went with no backtrace rather than a process-wide fallback, since the fallback would bring back #7041. The latest commit:

• clears the captured panic of the current thread at the start of try_unwrap_or_throw and catch_native, so a backtrace left by a panic handled before the call, on a pooled thread or otherwise, is never used;
• has the hook record the panic message with the backtrace, and only attaches the backtrace when the message matches the CometError::Panic being reported. That also covers panics caught during the call by catch_unwind outside Comet, where clearing at each catch site isn't possible.

A resumed panic now reaches the JVM with its message and no stack trace. Its location is still in the executor's stderr, where the hook logs every panic on the thread that raised it.

New tests resume a panic from another thread inside catch_native with an empty slot, after a panic handled during the call, and after a handled panic with the same message before the call, and check that none of them gets a backtrace. Removing the message check fails the second, and removing the clear fails the third. I also updated the description.