fix: vendor libhdfs with the HDFS-16021 thread-ownership fix

[andygrove]

Which issue does this PR close?

Closes #5023.

This is an alternative to #5036, which fixes the same bug by pinning [patch.crates-io] to a
personal GitHub fork. I raised two objections there — that an ASF release would carry a dependency
fetched from a contributor's personal account, and that [patch.crates-io] is silently ignored by
downstream consumers — and this takes the route I asked about in the second of them: vendoring the
patched C into our own tree. #5036 should close in favour of this if we agree on the approach;
either way the credit for the diagnosis and the C patch is @peterxcli's.

Rationale for this change

libhdfs registers a pthread thread-local destructor, hdfsThreadDestructor, that detaches the
current thread from the JVM whenever it finds a cached JNIEnv. It does that regardless of who
attached the thread. Comet attaches each of its Tokio worker threads itself, with
AttachCurrentThreadAsDaemon in on_thread_start, and detaches them in on_thread_stop
(native/core/src/execution/jni_api.rs). So the ordering on any worker that has touched HDFS is:

1. Comet attaches the thread.
2. libhdfs calls AttachCurrentThread, which succeeds on an already-attached thread and hands back
   the same JNIEnv, and caches it in its own TLS.
3. The thread stops. Comet detaches it and the JVM frees the JNIEnv.
4. pthread TLS destructors run. hdfsThreadDestructor dereferences the freed JNIEnv to call
   GetJavaVM, reads a null function pointer out of it and jumps to address zero.

That is SIGSEGV at pc=0x0000000000000000, which is exactly the signature in both symbolized
hs_err dumps on #5023 — @peterxcli's macOS one (lr = hdfsThreadDestructor+80, x8 = 0) and
@comphead's Linux one (RIP = 0x0, RDI holding the JNIEnv). It is
HDFS-16021, still open, and Hadoop trunk still
has the unguarded destructor.

Two things are worth naming because they change how urgent this looks. First, the suite that arms
it is not the suite that crashes: ParquetReadFromFakeHadoopFsSuite is what routes a read through
libhdfs, but the destructor does not fire until one of those pooled threads exits, which is
typically minutes later in CometIcebergNativeSuite. That is why it reads as a random [scans]
flake. Second, it is not macOS-only despite the issue title — hdfs-opendal is a default feature on
every platform. Scanning every failing CI run back to 2026-09-07 for a crash-logs artifact on a
Linux [scans] job, the first is 2026-09-09 and there are none before it, which puts the Linux
onset right after #5748 landed the explicit on_thread_stop detach on 2026-09-08. I can only page
CI history back that far at reasonable cost, so treat that as a strong correlation with a matching
mechanism rather than proof.

It has taken out at least seven runs of that job since 2026-09-09, on seven different branches, so
it is costing everyone re-runs rather than just being noise on one PR.

What changes are included in this PR?

native/hdfs-sys/ vendors the Apache Hadoop libhdfs sources that Comet actually compiles —
hdfs_3_3, POSIX only — with the HDFS-16021 fix applied, and native/Cargo.toml substitutes it for
the crates.io crate with [patch.crates-io] hdfs-sys = { path = "hdfs-sys" }. A path source, so
there is no personal fork, no network dependency at build time, and a source tarball stays
self-contained.

Three C files differ from Hadoop's originals, each carrying a notice at the top of the file:
os/thread_local_storage.h gains an attachedByLibhdfs flag on struct ThreadLocalState;
jni_helper.c has getGlobalJNIEnv report whether it attached the thread; and
os/posix/thread_local_storage.c guards the detach on that flag and initialises two fields
threadLocalStorageCreate previously left holding malloc garbage.

The first and third are the HDFS-16021 patch. The second also adds a GetEnv call before
AttachCurrentThread, which is not in the JIRA patch, and that is the part that actually matters
here: AttachCurrentThread returns JNI_OK on an already-attached thread, so without the GetEnv
early-out libhdfs would still record itself as the owner of an attachment Comet made and we would
crash exactly as before. Worth knowing if we ever re-sync with upstream.

src/lib.rs and build.rs are written for Comet rather than copied — see the licensing section.
dev/ci/compute-changes.py gains a native/hdfs-sys/** entry in the eight Spark-tier filters,
because the vendored C is compiled into libcomet but lives outside any src/ directory and so
would not have matched native/**/src/**.

The same C changes are proposed upstream as
Xuanwo/hdfs-sys#47, open since July and unreviewed.
native/hdfs-sys/README.md records the removal condition.

Licensing considerations

We do not need IP clearance. The IP Clearance
process is for donations of externally-developed code
to the ASF, where the Foundation takes on ownership and a Software Grant Agreement is involved.
Vendoring third-party Apache-2.0 code that we redistribute under its existing license is bundling,
governed by legal/resolved.html — ALv2 is Category A,
always permitted in ASF source and binary releases — and by
licensing-howto.html for the LICENSE/NOTICE work.

This case is easier than generic Category A, because all 15 vendored C/H files carry the standard
ASF license header. They are Apache Hadoop's own sources, already ASF-owned, copied verbatim.
Moving ALv2 ASF code between ASF projects needs no clearance; we just must not strip the headers,
and we have not.

We have done this twice before. #1377 (Feb 2025) brought
datafusion-contrib/datafusion-objectstore-hdfs
into native/hdfs/ and added a NOTICE.txt stanza for it. #2062 (Aug 2025) then vendored
datafusion-contrib/fs-hdfs into native/fs-hdfs/,
including c_src/libhdfs/ — the same Hadoop hdfs.c, jni_helper.c and
os/posix/thread_local_storage.c this PR copies — with a LICENSE.txt, a provenance README.md
and a second NOTICE.txt stanza. Both were removed in #4904 when we moved to the OpenDAL path. This
PR follows that established pattern.

What is copied, and what deliberately is not. Only the Hadoop C. The upstream hdfs-sys crate
is 244 files and 3 MB, but thirteen of its fourteen vendored libhdfs trees are Hadoop versions we
never build; hdfs_3_3 is 21 files, and dropping the Windows platform layer leaves 15. We also do
not take the bundled libdirent (MIT, Toni Ronkko), which is Windows-only — we ship no Windows
native artifacts, so nothing in this directory is under any license other than ALv2.

We also do not copy the crate's Rust. That is a deliberate choice: hdfs-sys has no LICENSE file
in its repository or its published crate, and its README links a ./LICENSE that 404s, so the only
license grant is the license = "Apache-2.0" field in Cargo.toml. That is unremarkable for a Rust
crate but not what I would want to lean on when bundling into an ASF release. We can avoid the
question entirely, because the only consumer in our graph is hdrs, which uses about two dozen
symbols. So src/lib.rs is under 200 lines of extern "C" declarations transcribed from the vendored
hdfs.h — which is Hadoop's own header, already in the tree — and build.rs is a cc::Build
invocation following the file list in Hadoop's CMakeLists.txt. Both are Comet-authored and carry
ASF headers. The result is that native/hdfs-sys/ contains no third-party-authored code at all.

Attribution. NOTICE.txt gains a stanza naming Apache Hadoop and pointing at
native/hdfs-sys/README.md, matching the form of the existing Gluten entry and of the two prior
HDFS stanzas. The three modified files carry the "this file was modified by" notice that section
4(b) of the license requires. I did not add a LICENSE.txt inside native/hdfs-sys/ the way
#2062 did, because unlike fs-hdfs3 this directory is not a third-party crate with its own
licensing — it is ASF code under the same license as the rest of the repo, and the root LICENSE.txt
covers it. Happy to add one if a reviewer would rather we were belt-and-braces.

RAT passes with Unapproved: 0, unknown: 0 over 623 files.

I am not a lawyer and this is policy rather than law. The two load-bearing points — that Category A
bundling is not IP clearance, and that ASF code moving between ASF projects is unencumbered — are
well established and we have the in-repo precedent above, but this should have PMC eyes before it
lands, and legal-discuss@ is there if anyone thinks it is borderline. The thing I would actually
want a second opinion on is not the licensing: it is that we are shipping a modified copy of another
ASF project's code while HDFS-16021 is still open, so a Hadoop committer's read of the patch would
be worth having.

How are these changes tested?

The strongest evidence is that the guard is demonstrably in the shipped binary. Disassembling
hdfsThreadDestructor out of libcomet.dylib, the ownership byte is now tested before the JNIEnv
is dereferenced:

ldr  x20, [x0, #8]      ; state->env  (now at offset 8, after the new bool)
ldrb w8,  [x0]          ; state->attachedByLibhdfs
cmp  w8, #0
ccmp x20, #0, #4, ne    ; attachedByLibhdfs != 0 && env != NULL
b.eq <skip>             ; bails out before touching env
ldr  x8, [x20]          ; *env, the load that used to fault
ldr  x8, [x8, #1752]    ; GetJavaVM
blr  x8                 ; the call that used to jump to 0

Building the same tree with the [patch] commented out produces a hdfsThreadDestructor with no
such guard, so this is not a system libhdfs being picked up by accident.

ParquetReadFromFakeHadoopFsSuite passes. That is the check that matters for regressions, because
the GetEnv early-out changes control flow on every libhdfs call: if GetEnv misbehaved we would
not crash, we would fail every HDFS read. Running the [scans] bucket order in a single JVM —
ParquetReadFromFakeHadoopFsSuite → ParquetTimestampLtzAsNtzSuite → CometNativeReaderSuite →
CometIcebergNativeSuite — gives 168 tests across 8 suites green, which is the order and the fork
sharing that the crash needs.

I should be straight about the limit of that last one: I ran the identical chain against the
unpatched build as a control and it also passed, so it does not discriminate. The flake is
intermittent, and it fires on Linux/JDK 11 over a much larger bucket than I ran, so one clean local
macOS run of a subset was never going to reproduce it. Treat the suite runs as
evidence that nothing is broken, the disassembly as evidence that the fix is present, and the
mechanism analysis against the two symbolized dumps on #5023 as the argument that the fix is the
right one. @peterxcli's 100 fresh-JVM runs on #5036 are the closest thing we have to a positive
result, and I would note there that those ran the arming suite alone rather than the bucket order,
so they are weaker than they look too.

Compile-time assertions in src/lib.rs pin hdfsFileInfo's size, alignment and every field offset
against the C typedef, since a hand-written binding that got the layout wrong would be silent memory
corruption rather than a link error. They are const assertions rather than #[test]s so they run
on every build; they caught an off-by-one in my own arithmetic while I was writing them. cargo clippy --all-targets --workspace, cargo fmt --all --check, cargo machete, prettier --check and
dev/ci/check-ci-config.py are all clean.

🤖 Generated with Claude Code

[sunchao]

Correctness

The previous dependency cached a JNIEnv and detached it at pthread teardown regardless of who created the attachment. Comet already attaches Tokio workers as daemons and detaches them in on_thread_stop. The new GetEnv check recognizes that borrowed attachment, leaves attachedByLibhdfs false, and makes the TLS destructor skip the JNI pointer dereference. Threads that libhdfs actually attaches still take the detach path. This matches the JNI invocation contract and the existing Comet worker lifecycle.

I checked initialization, an already-attached thread, successful attachment, and the JVM lookup, GetEnv, attach and create failure paths. The flag starts false and becomes true only after libhdfs creates an attachment. The cached-environment behavior during a thread's lifetime is unchanged. This is a shared native lifecycle change for the supported Spark versions. It changes no expression, operator, SQL type, null, ANSI, overflow or fallback semantics. I did not claim a separate maintained-branch Spark source comparison or cross-version runtime proof for this JNI change.

The 15 vendored C/header files trace to the published hdfs-sys 0.3.0 archive. Twelve are byte-identical, and the three modified files have the same executable tokens as upstream PR #47 at its pinned head. All 24 Rust function declarations match the released signatures and cover the 19 functions used by hdrs 0.3.2. The C header's file-info size, alignment and all ten field offsets match the Rust assertions on this arm64 macOS host.

Validation

A small C probe using the exact changed ownership function, the actual TLS implementation, mocked JNI calls and real pthread teardown passed seven acquisition/error branches. It also verified that a borrowed invalid environment pointer is never dereferenced and an owned attachment is detached exactly once. This was a component test, with no JVM or HDFS connection.

At the review snapshot, CI had 57 successful checks and 10 skips. The Linux native producer checked out merge 04306d6, whose parents are the reviewed base 394ad88 and head 3152da6, and compiled the local native/hdfs-sys crate. All 24 authored files match that merge. The Spark 3.4/JDK 11 scans job consumed the matching native artifact digest and passed 484 Scala tests, with 27 canceled and one ignored. macOS CI was skipped. I did not independently disassemble the shipped library or reproduce the intermittent JVM crash. The author's clean unpatched control also means the reported local suite chain is regression coverage, not a discriminating crash reproduction.

I found no change-introduced correctness issue within these checks.

Performance

The extra GetEnv call occurs when libhdfs first obtains this thread's environment. Subsequent calls reuse its existing TLS state. The destructor adds a boolean ownership check, and the patch introduces no per-row allocation, copying or extra filesystem I/O. There is no new expression or throughput claim requiring a microbenchmark here, and the passing tests are not performance measurements.

Design

The ownership decision belongs where libhdfs acquires the JNI environment. Guarding teardown alone would still be wrong if a successful AttachCurrentThread on an already-attached thread were recorded as a new attachment. Keeping both the acquisition check and destructor guard preserves Comet's existing worker lifecycle.

The local Cargo patch makes the selected Hadoop 3.3 POSIX implementation part of Comet's source tree, with provenance and a removal condition documented. The existing system-library override remains significant: HDFS_LIB_DIR or HADOOP_HOME can select an external library with its own behavior. The README documents that boundary, and macOS's vendored feature bypasses the override. The downstream-root limitation of [patch.crates-io] is also documented. No verified design change is needed before merge.

Abstraction & complexity

One ownership field is sufficient for this thread-local responsibility. It avoids another attachment manager or changes to the Tokio lifecycle. The thin Rust declarations deliberately cover the current consumer, and the layout assertions make their maintenance obligation explicit. The CI path additions correctly include C and header changes outside src/ in the four Spark and four Iceberg filters. I found no unnecessary abstraction or actionable simplification.

[peterxcli]

Thanks @andygrove!

[comphead]

Tbh I'm not sure about replicating hdfs-sys with a fix into Comet. It would be easier to comment out the HDFS tests, as HDFS is not priority direction and Iceberg HDFS support also stalling. And someone need to support libhdfs code in Comet and all of it to fix flaky HDFS test.

[andygrove]

Tbh I'm not sure about replicating hdfs-sys with a fix into Comet. It would be easier to comment out the HDFS tests, as HDFS is not priority direction and Iceberg HDFS support also stalling. And someone need to support libhdfs code in Comet and all of it to fix flaky HDFS test.

Ok, moved this to draft and creating #5892 as requested