fix: load the bundled native library only once per class loader

[zhangfengcdt]

Which issue does this PR close?

Closes #6096.

Rationale for this change

The cause is that the native library gets loaded more than once. CometSparkSessionExtensionsSuite resets NativeBase's loaded flag and calls load() again. Each call unpacks the bundled library to a new, randomly named temporary file and loads it.

To the JVM and the OS loader that is a separate library with its own statics, so the process ends up with several copies and only the first has JAVA_VM set.

JDK 17 resolves a JNI method on first call by iterating a hash map of loaded libraries, so a method first called after that suite can bind to an uninitialized copy. Which copy wins depends on the random file names, which is why the failure is intermittent. setShufflePartitionPusher is first called in CometNativeShuffleSuite, which is why the abort lands there.

What changes are included in this PR?

• NativeBase: a flag that is never reset records that the bundled library has been loaded, so a repeat load() skips unpacking and loading another copy. The rest of the load sequence is unchanged.
• CometSparkSessionExtensionsSuite: a regression test that resets the flag, reloads, and asserts no new library file was unpacked.

How are these changes tested?

The new regression test fails without the fix and passes with it.

I also reproduced the issue by running CometSparkSessionExtensionsSuite followed by CometNativeShuffleSuite in one JVM, on macOS with Spark 4.1.

• Without the fix: 4 of 5 runs failed with JAVA_VM not initialized, starting at the test right after the callback registration test, as in the issue.
• With the fix: 9 of 9 runs passed, with exactly one library copy unpacked.

[andygrove]

The production fix matches the diagnosed failure mechanism, and the Linux exec job completed 132 suites in one JVM. I found one test-isolation issue that should be addressed before merge.

[andygrove]

Could we make this assertion process-local? It snapshots every libcomet-* file in java.io.tmpdir, so another Comet test JVM or parallel worktree that starts during this window will appear in added even though this class loader did not reload anything. A child JVM with an isolated temporary directory, or a test-visible load counter or path owned by NativeBase, would preserve the regression coverage without depending on unrelated processes.

[zhangfengcdt]

Sure! It should be fixed now. It now compares the path of the library before and after a reload and asserts it is the same object. It no longer reads the temp directory at all.

[andygrove]

Thanks!

[sunchao]

Summary

• Prior state and problem: Resetting NativeBase.loaded allowed repeated extraction and loading of bundled libraries, creating separate native state and inconsistent JNI bindings.
• Design approach: Track successful bundled-library loading independently with bundledLibraryLoaded.
• Correctness / compatibility analysis: The existing synchronized load() protects the new flag, which is set after System.load succeeds. Logging initialization, timezone checks, Arrow properties, and java.library.path loading remain intact. Reviewed native initialization, OpenJDK 17 library resolution, and Spark class-loader sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0.
• Key design decisions: The flag has the defining class loader’s lifetime. One boolean addresses the lifecycle distinction without additional abstraction or query-path work.
• Implementation sketch: The production change guards bundled extraction. The regression test resets loaded, reloads, and compares temporary-library filenames.
• Behavioral changes worth calling out: A JDK 17 harness produced one bundled library across four initialization calls at the reviewed head, versus four libraries at the base. Arrow-property handling, independent class loaders, and library-path loading passed.
• Suggested improvements: The existing test-isolation concern remains a substantiated P2 blocker. A controlled second JVM sharing java.io.tmpdir caused the assertion to fail despite the first JVM correctly retaining its original library. Maven normally isolates temporary directories by checkout, but the assertion still measures unrelated JVMs sharing that directory. Use an owned library path/count or an isolated child-JVM directory, as already requested.

No additional introduced P1/P2 issues found within this review. The existing concern is not duplicated as a new finding.

Reviewed the entire diff from 62ed9d16d547fed1ac25260a1910893f076ab638 to 5b276e961eda9b4e914e4870bfe4174d32889ea1, including both changed files and surrounding code. The PR remains open and non-draft. Routed skills: review-comet-pr and review-comet-ffi-pr.

Exact-head CI: Comet CI remains in progress. Preflight, change detection, general lint, Scala syntactic lint, and CodeQL passed. Linux builds, Rust tests, Java lint, and compatibility checks remain in progress. macOS, Spark SQL, and Iceberg suites were skipped. No completed failures were reported.

Validation limits: The disposable harness compiled unmodified head/base NativeBase.java with dependency stubs and a minimal JNI library, using a Java equivalent of the regression assertion. Reproduce with python /tmp/comet-6493-review-validation/validate.py. Full Comet/Spark suites and macOS execution were not run. The checkout had no built artifacts and remains unchanged.

[sunchao]

Summary

• Prior state and problem: Resetting NativeBase.loaded allowed repeated bundled-library extraction, creating separate native state and inconsistent JNI bindings.
• Design approach: Retain the successfully loaded library in bundledLibrary, independently of the resettable flag.
• Correctness / compatibility analysis: The existing synchronization protects the retained reference, which is assigned after System.load succeeds. Reinitialization, Arrow properties, and java.library.path loading remain intact. Compared OpenJDK 17 library resolution and Spark class-loader setup for supported versions 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. No SQL semantics change.
• Key design decisions: One retained File per defining class loader captures the required lifetime without a new abstraction or query-path overhead.
• Implementation sketch: Skip bundled extraction when that reference exists. The regression test resets loaded, reloads, and checks reference identity.
• Behavioral changes worth calling out: Focused validation retained one library across four initialization calls, versus four at the base. Independent class loaders, library-path loading, and Arrow-property handling passed. The revised assertion also passed while another JVM loaded a library into the shared temporary directory, addressing the existing test-isolation concern despite its thread remaining open.
• Suggested improvements: None at P1/P2. No introduced P1/P2 issues found within this review.

Reviewed the entire two-file diff from 9c42391cb9a6fe38d87f59992dd5fb4260483dbb to 0d4d9a3f5f4a0808fb90ec5bac7564892f9bb0b9, surrounding code, and all supplied discussions. The PR is non-draft. Routed skills: review-comet-pr and review-comet-ffi-pr.

Exact-head CI: Comet CI succeeded. All 24 completed checks passed, with 15 skipped. The Linux Spark 4.1/JDK 17 execution job passed 1,178 tests, including the new regression and Arrow-property tests. macOS, Spark SQL, and Iceberg suites were skipped.

Validation limits: Local validation compiled exact head/base NativeBase.java with dependency stubs and a minimal JNI library on JDK 17. The regression failed as expected at the base and with the head's guard removed. This validates loader behavior, not full native execution. Full Comet/Spark suites and macOS execution were not run locally. Project files remain unchanged.

[andygrove]

Reviewed the latest head. The previous test-isolation concern is addressed, and I found no additional issues.